[v1,1/1] drivers: remoteproc: Add bus scaling capability during bootup

Message ID 35eb4ce2bb8f3bb78a616c071a012f1f49d8b593.1666127246.git.quic_gokukris@quicinc.com
State New
Headers
Series [v1,1/1] drivers: remoteproc: Add bus scaling capability during bootup |

Commit Message

Gokul krishna Krishnakumar Oct. 18, 2022, 9:10 p.m. UTC
  During bootup since remote processors cannot request for
additional bus bandwidth from the interconect framework,
platform driver should provide the proxy resources. Make
a proxy vote for maximizing the bus bandwidth during bootup
for a remote processor and remove it once processor is up.

Change-Id: I798b3b000aef4761a7ff8cb6390b7ecd98f925b7
Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
---
 drivers/remoteproc/qcom_q6v5_pas.c | 106 ++++++++++++++++++++++++++++++++++---
 1 file changed, 100 insertions(+), 6 deletions(-)
  

Comments

Guru Das Srinagesh Oct. 19, 2022, 1:15 a.m. UTC | #1
On Oct 18 2022 14:10, Gokul krishna Krishnakumar wrote:
> During bootup since remote processors cannot request for
> additional bus bandwidth from the interconect framework,
> platform driver should provide the proxy resources. Make
> a proxy vote for maximizing the bus bandwidth during bootup
> for a remote processor and remove it once processor is up.
> 
> Change-Id: I798b3b000aef4761a7ff8cb6390b7ecd98f925b7

Please remove Change-Ids - they are not for upstream.

Thank you.

Guru Das.
  
Krzysztof Kozlowski Oct. 19, 2022, 1:55 a.m. UTC | #2
On 18/10/2022 17:10, Gokul krishna Krishnakumar wrote:
> During bootup since remote processors cannot request for
> additional bus bandwidth from the interconect framework,
> platform driver should provide the proxy resources. Make
> a proxy vote for maximizing the bus bandwidth during bootup
> for a remote processor and remove it once processor is up.
> 

(...)

>  
> @@ -265,6 +340,7 @@ static void qcom_pas_handover(struct qcom_q6v5 *q6v5)
>  	clk_disable_unprepare(adsp->aggre2_clk);
>  	clk_disable_unprepare(adsp->xo);
>  	adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
> +	do_bus_scaling(adsp, false);
>  }
>  
>  static int adsp_stop(struct rproc *rproc)
> @@ -383,6 +459,22 @@ static int adsp_init_regulator(struct qcom_adsp *adsp)
>  	return 0;
>  }
>  
> +static void adsp_init_bus_scaling(struct qcom_adsp *adsp)
> +{
> +	if (scm_perf_client)
> +		goto get_rproc_client;
> +
> +	scm_perf_client = of_icc_get(adsp->dev, "crypto_ddr");

Aren't you adding here new DT properties to the device? If so, this
requires updating bindings.

> +	if (IS_ERR(scm_perf_client))
> +		dev_warn(adsp->dev, "Crypto scaling not setup\n");
> +
> +get_rproc_client:
> +	adsp->bus_client = of_icc_get(adsp->dev, "rproc_ddr");
> +	if (IS_ERR(adsp->bus_client))
> +		dev_warn(adsp->dev, "%s: No bus client\n", __func__);
> +
> +}


Best regards,
Krzysztof
  
Neil Armstrong Oct. 19, 2022, 6:49 a.m. UTC | #3
Hi Gokul,

On 18/10/2022 23:10, Gokul krishna Krishnakumar wrote:
> During bootup since remote processors cannot request for
> additional bus bandwidth from the interconect framework,
> platform driver should provide the proxy resources. Make
> a proxy vote for maximizing the bus bandwidth during bootup
> for a remote processor and remove it once processor is up.

A similar change has been upstreamed in
- [0] 65b7ebda5028 ("firmware: qcom_scm: Add bw voting support to the SCM interface")
- [1] 8d9be5c6bdcd ("remoteproc: qcom: q6v5: Add interconnect path proxy vote")

The main difference with this patch are:
- [0] only votes for each scm calls, not across multiple calls, is that a problem ?
- [0] & [1] votes (0, UINT_MAX) but you change votes (UINT_MAX, UINT_MAX), does this make a difference ?

[0] https://github.com/torvalds/linux/commit/65b7ebda5028
[1] https://github.com/torvalds/linux/commit/8d9be5c6bdcd

> 
> Change-Id: I798b3b000aef4761a7ff8cb6390b7ecd98f925b7
> Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
> ---
>   drivers/remoteproc/qcom_q6v5_pas.c | 106 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 100 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 6afd094..b02a1dc 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -21,6 +21,7 @@
>   #include <linux/qcom_scm.h>
>   #include <linux/regulator/consumer.h>
>   #include <linux/remoteproc.h>
> +#include <linux/interconnect.h>
>   #include <linux/soc/qcom/mdt_loader.h>
>   #include <linux/soc/qcom/smem.h>
>   #include <linux/soc/qcom/smem_state.h>
> @@ -30,8 +31,14 @@
>   #include "qcom_q6v5.h"
>   #include "remoteproc_internal.h"
>   
> +#define PIL_TZ_AVG_BW	UINT_MAX
> +#define PIL_TZ_PEAK_BW	UINT_MAX
>   #define ADSP_DECRYPT_SHUTDOWN_DELAY_MS	100
>   
> +static struct icc_path *scm_perf_client;
> +static int scm_pas_bw_count;
> +static DEFINE_MUTEX(scm_pas_bw_mutex);
> +
>   struct adsp_data {
>   	int crash_reason_smem;
>   	const char *firmware_name;
> @@ -67,6 +74,7 @@ struct qcom_adsp {
>   
>   	int pas_id;
>   	unsigned int minidump_id;
> +	struct icc_path *bus_client;
>   	int crash_reason_smem;
>   	bool has_aggre2_clk;
>   	bool decrypt_shutdown;
> @@ -136,6 +144,44 @@ static void adsp_pds_disable(struct qcom_adsp *adsp, struct device **pds,
>   	}
>   }
>   
> +static int scm_pas_enable_bw(void)
> +{
> +	int ret = 0;
> +
> +	if (IS_ERR(scm_perf_client))
> +		return -EINVAL;
> +
> +	mutex_lock(&scm_pas_bw_mutex);
> +	if (!scm_pas_bw_count) {
> +		ret = icc_set_bw(scm_perf_client, PIL_TZ_AVG_BW,
> +						PIL_TZ_PEAK_BW);
> +		if (ret)
> +			goto err_bus;
> +	}
> +
> +	scm_pas_bw_count++;
> +	mutex_unlock(&scm_pas_bw_mutex);
> +	return ret;
> +
> +err_bus:
> +	pr_err("scm-pas: Bandwidth request failed (%d)\n", ret);
> +	icc_set_bw(scm_perf_client, 0, 0);
> +
> +	mutex_unlock(&scm_pas_bw_mutex);
> +	return ret;
> +}
> +
> +static void scm_pas_disable_bw(void)
> +{
> +	if (IS_ERR(scm_perf_client))
> +		return;
> +
> +	mutex_lock(&scm_pas_bw_mutex);
> +	if (scm_pas_bw_count-- == 1)
> +		icc_set_bw(scm_perf_client, 0, 0);
> +	mutex_unlock(&scm_pas_bw_mutex);
> +}
> +
>   static int adsp_shutdown_poll_decrypt(struct qcom_adsp *adsp)
>   {
>   	unsigned int retry_num = 50;
> @@ -174,15 +220,35 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
>   	if (ret)
>   		return ret;
>   
> +	scm_pas_enable_bw();
>   	ret = qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, adsp->pas_id,
>   				    adsp->mem_region, adsp->mem_phys, adsp->mem_size,
>   				    &adsp->mem_reloc);
>   	if (ret)
> -		return ret;
> +		goto exit;
>   
>   	qcom_pil_info_store(adsp->info_name, adsp->mem_phys, adsp->mem_size);
> +exit:
> +	scm_pas_disable_bw();
> +	return ret;
> +}
>   
> -	return 0;
> +static int do_bus_scaling(struct qcom_adsp *adsp, bool enable)
> +{
> +	int rc = 0;
> +	u32 avg_bw = enable ? PIL_TZ_AVG_BW : 0;
> +	u32 peak_bw = enable ? PIL_TZ_PEAK_BW : 0;
> +
> +	if (IS_ERR(adsp->bus_client))
> +		dev_err(adsp->dev, "Bus scaling not setup for %s\n",
> +			adsp->rproc->name);
> +	else
> +		rc = icc_set_bw(adsp->bus_client, avg_bw, peak_bw);
> +
> +	if (rc)
> +		dev_err(adsp->dev, "bandwidth request failed(rc:%d)\n", rc);
> +
> +	return rc;
>   }
>   
>   static int adsp_start(struct rproc *rproc)
> @@ -194,10 +260,14 @@ static int adsp_start(struct rproc *rproc)
>   	if (ret)
>   		return ret;
>   
> -	ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
> +	ret = do_bus_scaling(adsp, true);
>   	if (ret < 0)
>   		goto disable_irqs;
>   
> +	ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
> +	if (ret < 0)
> +		goto unscale_bus;
> +
>   	ret = clk_prepare_enable(adsp->xo);
>   	if (ret)
>   		goto disable_proxy_pds;
> @@ -218,6 +288,8 @@ static int adsp_start(struct rproc *rproc)
>   			goto disable_cx_supply;
>   	}
>   
> +	scm_pas_enable_bw();
> +
>   	ret = qcom_scm_pas_auth_and_reset(adsp->pas_id);
>   	if (ret) {
>   		dev_err(adsp->dev,
> @@ -234,9 +306,10 @@ static int adsp_start(struct rproc *rproc)
>   
>   	qcom_scm_pas_metadata_release(&adsp->pas_metadata);
>   
> -	return 0;
> -
>   disable_px_supply:
> +	scm_pas_disable_bw();
> +	if(!ret)
> +		goto exit;
>   	if (adsp->px_supply)
>   		regulator_disable(adsp->px_supply);
>   disable_cx_supply:
> @@ -248,9 +321,11 @@ static int adsp_start(struct rproc *rproc)
>   	clk_disable_unprepare(adsp->xo);
>   disable_proxy_pds:
>   	adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
> +unscale_bus:
> +	do_bus_scaling(adsp, false);
>   disable_irqs:
>   	qcom_q6v5_unprepare(&adsp->q6v5);
> -
> +exit:
>   	return ret;
>   }
>   
> @@ -265,6 +340,7 @@ static void qcom_pas_handover(struct qcom_q6v5 *q6v5)
>   	clk_disable_unprepare(adsp->aggre2_clk);
>   	clk_disable_unprepare(adsp->xo);
>   	adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
> +	do_bus_scaling(adsp, false);
>   }
>   
>   static int adsp_stop(struct rproc *rproc)
> @@ -383,6 +459,22 @@ static int adsp_init_regulator(struct qcom_adsp *adsp)
>   	return 0;
>   }
>   
> +static void adsp_init_bus_scaling(struct qcom_adsp *adsp)
> +{
> +	if (scm_perf_client)
> +		goto get_rproc_client;
> +
> +	scm_perf_client = of_icc_get(adsp->dev, "crypto_ddr");
> +	if (IS_ERR(scm_perf_client))
> +		dev_warn(adsp->dev, "Crypto scaling not setup\n");
> +
> +get_rproc_client:
> +	adsp->bus_client = of_icc_get(adsp->dev, "rproc_ddr");
> +	if (IS_ERR(adsp->bus_client))
> +		dev_warn(adsp->dev, "%s: No bus client\n", __func__);
> +
> +}
> +
>   static int adsp_pds_attach(struct device *dev, struct device **devs,
>   			   char **pd_names)
>   {
> @@ -525,6 +617,8 @@ static int adsp_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto free_rproc;
>   
> +	adsp_init_bus_scaling(adsp);
> +
>   	ret = adsp_pds_attach(&pdev->dev, adsp->proxy_pds,
>   			      desc->proxy_pd_names);
>   	if (ret < 0)
  

Patch

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 6afd094..b02a1dc 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -21,6 +21,7 @@ 
 #include <linux/qcom_scm.h>
 #include <linux/regulator/consumer.h>
 #include <linux/remoteproc.h>
+#include <linux/interconnect.h>
 #include <linux/soc/qcom/mdt_loader.h>
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/smem_state.h>
@@ -30,8 +31,14 @@ 
 #include "qcom_q6v5.h"
 #include "remoteproc_internal.h"
 
+#define PIL_TZ_AVG_BW	UINT_MAX
+#define PIL_TZ_PEAK_BW	UINT_MAX
 #define ADSP_DECRYPT_SHUTDOWN_DELAY_MS	100
 
+static struct icc_path *scm_perf_client;
+static int scm_pas_bw_count;
+static DEFINE_MUTEX(scm_pas_bw_mutex);
+
 struct adsp_data {
 	int crash_reason_smem;
 	const char *firmware_name;
@@ -67,6 +74,7 @@  struct qcom_adsp {
 
 	int pas_id;
 	unsigned int minidump_id;
+	struct icc_path *bus_client;
 	int crash_reason_smem;
 	bool has_aggre2_clk;
 	bool decrypt_shutdown;
@@ -136,6 +144,44 @@  static void adsp_pds_disable(struct qcom_adsp *adsp, struct device **pds,
 	}
 }
 
+static int scm_pas_enable_bw(void)
+{
+	int ret = 0;
+
+	if (IS_ERR(scm_perf_client))
+		return -EINVAL;
+
+	mutex_lock(&scm_pas_bw_mutex);
+	if (!scm_pas_bw_count) {
+		ret = icc_set_bw(scm_perf_client, PIL_TZ_AVG_BW,
+						PIL_TZ_PEAK_BW);
+		if (ret)
+			goto err_bus;
+	}
+
+	scm_pas_bw_count++;
+	mutex_unlock(&scm_pas_bw_mutex);
+	return ret;
+
+err_bus:
+	pr_err("scm-pas: Bandwidth request failed (%d)\n", ret);
+	icc_set_bw(scm_perf_client, 0, 0);
+
+	mutex_unlock(&scm_pas_bw_mutex);
+	return ret;
+}
+
+static void scm_pas_disable_bw(void)
+{
+	if (IS_ERR(scm_perf_client))
+		return;
+
+	mutex_lock(&scm_pas_bw_mutex);
+	if (scm_pas_bw_count-- == 1)
+		icc_set_bw(scm_perf_client, 0, 0);
+	mutex_unlock(&scm_pas_bw_mutex);
+}
+
 static int adsp_shutdown_poll_decrypt(struct qcom_adsp *adsp)
 {
 	unsigned int retry_num = 50;
@@ -174,15 +220,35 @@  static int adsp_load(struct rproc *rproc, const struct firmware *fw)
 	if (ret)
 		return ret;
 
+	scm_pas_enable_bw();
 	ret = qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, adsp->pas_id,
 				    adsp->mem_region, adsp->mem_phys, adsp->mem_size,
 				    &adsp->mem_reloc);
 	if (ret)
-		return ret;
+		goto exit;
 
 	qcom_pil_info_store(adsp->info_name, adsp->mem_phys, adsp->mem_size);
+exit:
+	scm_pas_disable_bw();
+	return ret;
+}
 
-	return 0;
+static int do_bus_scaling(struct qcom_adsp *adsp, bool enable)
+{
+	int rc = 0;
+	u32 avg_bw = enable ? PIL_TZ_AVG_BW : 0;
+	u32 peak_bw = enable ? PIL_TZ_PEAK_BW : 0;
+
+	if (IS_ERR(adsp->bus_client))
+		dev_err(adsp->dev, "Bus scaling not setup for %s\n",
+			adsp->rproc->name);
+	else
+		rc = icc_set_bw(adsp->bus_client, avg_bw, peak_bw);
+
+	if (rc)
+		dev_err(adsp->dev, "bandwidth request failed(rc:%d)\n", rc);
+
+	return rc;
 }
 
 static int adsp_start(struct rproc *rproc)
@@ -194,10 +260,14 @@  static int adsp_start(struct rproc *rproc)
 	if (ret)
 		return ret;
 
-	ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
+	ret = do_bus_scaling(adsp, true);
 	if (ret < 0)
 		goto disable_irqs;
 
+	ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
+	if (ret < 0)
+		goto unscale_bus;
+
 	ret = clk_prepare_enable(adsp->xo);
 	if (ret)
 		goto disable_proxy_pds;
@@ -218,6 +288,8 @@  static int adsp_start(struct rproc *rproc)
 			goto disable_cx_supply;
 	}
 
+	scm_pas_enable_bw();
+
 	ret = qcom_scm_pas_auth_and_reset(adsp->pas_id);
 	if (ret) {
 		dev_err(adsp->dev,
@@ -234,9 +306,10 @@  static int adsp_start(struct rproc *rproc)
 
 	qcom_scm_pas_metadata_release(&adsp->pas_metadata);
 
-	return 0;
-
 disable_px_supply:
+	scm_pas_disable_bw();
+	if(!ret)
+		goto exit;
 	if (adsp->px_supply)
 		regulator_disable(adsp->px_supply);
 disable_cx_supply:
@@ -248,9 +321,11 @@  static int adsp_start(struct rproc *rproc)
 	clk_disable_unprepare(adsp->xo);
 disable_proxy_pds:
 	adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
+unscale_bus:
+	do_bus_scaling(adsp, false);
 disable_irqs:
 	qcom_q6v5_unprepare(&adsp->q6v5);
-
+exit:
 	return ret;
 }
 
@@ -265,6 +340,7 @@  static void qcom_pas_handover(struct qcom_q6v5 *q6v5)
 	clk_disable_unprepare(adsp->aggre2_clk);
 	clk_disable_unprepare(adsp->xo);
 	adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
+	do_bus_scaling(adsp, false);
 }
 
 static int adsp_stop(struct rproc *rproc)
@@ -383,6 +459,22 @@  static int adsp_init_regulator(struct qcom_adsp *adsp)
 	return 0;
 }
 
+static void adsp_init_bus_scaling(struct qcom_adsp *adsp)
+{
+	if (scm_perf_client)
+		goto get_rproc_client;
+
+	scm_perf_client = of_icc_get(adsp->dev, "crypto_ddr");
+	if (IS_ERR(scm_perf_client))
+		dev_warn(adsp->dev, "Crypto scaling not setup\n");
+
+get_rproc_client:
+	adsp->bus_client = of_icc_get(adsp->dev, "rproc_ddr");
+	if (IS_ERR(adsp->bus_client))
+		dev_warn(adsp->dev, "%s: No bus client\n", __func__);
+
+}
+
 static int adsp_pds_attach(struct device *dev, struct device **devs,
 			   char **pd_names)
 {
@@ -525,6 +617,8 @@  static int adsp_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
+	adsp_init_bus_scaling(adsp);
+
 	ret = adsp_pds_attach(&pdev->dev, adsp->proxy_pds,
 			      desc->proxy_pd_names);
 	if (ret < 0)