[PATCH 1/2] remoteproc: qcom: Add bus scaling capability during bootup

[PATCH 1/2] remoteproc: qcom: Add bus scaling capability during bootup

[Rishabh Bhatnagar]

During bootup since remote processors cannot request for
additional bus bandwidth from the interconect framework,
platform driver should provide the proxy resources. This
is useful for scenarios where the Q6 tries to access the DDR
memory in the initial stages of bootup. For e.g. during
bootup or after recovery modem Q6 tries to zero out the bss
section in the DDR. Since this is a big chunk of memory if
don't bump up the bandwidth we might encounter timeout issues.
This patch makes a proxy vote for maximizing the bus bandwidth
during bootup and removes it once processor is up.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pas.c | 43 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index edf9d0e..8f5db8d 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -20,6 +20,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>
@@ -28,6 +29,9 @@
 #include "qcom_q6v5.h"
 #include "remoteproc_internal.h"
 
+#define PIL_TZ_AVG_BW  0
+#define PIL_TZ_PEAK_BW UINT_MAX
+
 struct adsp_data {
 	int crash_reason_smem;
 	const char *firmware_name;
@@ -62,6 +66,7 @@ struct qcom_adsp {
 	int proxy_pd_count;
 
 	int pas_id;
+	struct icc_path *bus_client;
 	int crash_reason_smem;
 	bool has_aggre2_clk;
 
@@ -124,6 +129,25 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
 
 }
 
+static int do_bus_scaling(struct qcom_adsp *adsp, bool enable)
+{
+	int rc;
+	u32 avg_bw = enable ? PIL_TZ_AVG_BW : 0;
+	u32 peak_bw = enable ? PIL_TZ_PEAK_BW : 0;
+
+	if (adsp->bus_client) {
+		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;
+		}
+	} else
+		dev_info(adsp->dev, "Bus scaling not setup for %s\n",
+			adsp->rproc->name);
+	return 0;
+}
+
 static int adsp_start(struct rproc *rproc)
 {
 	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
@@ -131,9 +155,13 @@ static int adsp_start(struct rproc *rproc)
 
 	qcom_q6v5_prepare(&adsp->q6v5);
 
+	ret = do_bus_scaling(adsp, true);
+	if (ret)
+		goto disable_irqs;
+
 	ret = adsp_pds_enable(adsp, adsp->active_pds, adsp->active_pd_count);
 	if (ret < 0)
-		goto disable_irqs;
+		goto unscale_bus;
 
 	ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
 	if (ret < 0)
@@ -183,6 +211,8 @@ static int adsp_start(struct rproc *rproc)
 	adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
 disable_active_pds:
 	adsp_pds_disable(adsp, adsp->active_pds, adsp->active_pd_count);
+unscale_bus:
+	do_bus_scaling(adsp, false);
 disable_irqs:
 	qcom_q6v5_unprepare(&adsp->q6v5);
 
@@ -198,6 +228,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)
@@ -280,6 +311,14 @@ static int adsp_init_regulator(struct qcom_adsp *adsp)
 	return PTR_ERR_OR_ZERO(adsp->px_supply);
 }
 
+static void adsp_init_bus_scaling(struct qcom_adsp *adsp)
+{
+	adsp->bus_client = of_icc_get(adsp->dev, NULL);
+	if (!adsp->bus_client)
+		dev_warn(adsp->dev, "%s: unable to get bus client \n",
+			__func__);
+}
+
 static int adsp_pds_attach(struct device *dev, struct device **devs,
 			   char **pd_names)
 {
@@ -410,6 +449,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->active_pds,
 			      desc->active_pd_names);
 	if (ret < 0)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] remoteproc: qcom: Add bus scaling capability during bootup

[Mathieu Poirier]

Hi Rishabh,

On Fri, 27 Mar 2020 at 18:59, Rishabh Bhatnagar <rishabhb@codeaurora.org> wrote:
>
> During bootup since remote processors cannot request for
> additional bus bandwidth from the interconect framework,
> platform driver should provide the proxy resources. This
> is useful for scenarios where the Q6 tries to access the DDR
> memory in the initial stages of bootup. For e.g. during
> bootup or after recovery modem Q6 tries to zero out the bss
> section in the DDR. Since this is a big chunk of memory if
> don't bump up the bandwidth we might encounter timeout issues.
> This patch makes a proxy vote for maximizing the bus bandwidth
> during bootup and removes it once processor is up.
>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>

The title of this patch contains "[PATCH 1/2]" but only one patch was
sent to the linux-remoteproc mailing list.  Is this a mistake and this
is a stand alone patch or another patch did not reach the list?

Thanks,
Mathieu

> ---
>  drivers/remoteproc/qcom_q6v5_pas.c | 43 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index edf9d0e..8f5db8d 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -20,6 +20,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>
> @@ -28,6 +29,9 @@
>  #include "qcom_q6v5.h"
>  #include "remoteproc_internal.h"
>
> +#define PIL_TZ_AVG_BW  0
> +#define PIL_TZ_PEAK_BW UINT_MAX
> +
>  struct adsp_data {
>         int crash_reason_smem;
>         const char *firmware_name;
> @@ -62,6 +66,7 @@ struct qcom_adsp {
>         int proxy_pd_count;
>
>         int pas_id;
> +       struct icc_path *bus_client;
>         int crash_reason_smem;
>         bool has_aggre2_clk;
>
> @@ -124,6 +129,25 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
>
>  }
>
> +static int do_bus_scaling(struct qcom_adsp *adsp, bool enable)
> +{
> +       int rc;
> +       u32 avg_bw = enable ? PIL_TZ_AVG_BW : 0;
> +       u32 peak_bw = enable ? PIL_TZ_PEAK_BW : 0;
> +
> +       if (adsp->bus_client) {
> +               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;
> +               }
> +       } else
> +               dev_info(adsp->dev, "Bus scaling not setup for %s\n",
> +                       adsp->rproc->name);
> +       return 0;
> +}
> +
>  static int adsp_start(struct rproc *rproc)
>  {
>         struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
> @@ -131,9 +155,13 @@ static int adsp_start(struct rproc *rproc)
>
>         qcom_q6v5_prepare(&adsp->q6v5);
>
> +       ret = do_bus_scaling(adsp, true);
> +       if (ret)
> +               goto disable_irqs;
> +
>         ret = adsp_pds_enable(adsp, adsp->active_pds, adsp->active_pd_count);
>         if (ret < 0)
> -               goto disable_irqs;
> +               goto unscale_bus;
>
>         ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
>         if (ret < 0)
> @@ -183,6 +211,8 @@ static int adsp_start(struct rproc *rproc)
>         adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
>  disable_active_pds:
>         adsp_pds_disable(adsp, adsp->active_pds, adsp->active_pd_count);
> +unscale_bus:
> +       do_bus_scaling(adsp, false);
>  disable_irqs:
>         qcom_q6v5_unprepare(&adsp->q6v5);
>
> @@ -198,6 +228,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)
> @@ -280,6 +311,14 @@ static int adsp_init_regulator(struct qcom_adsp *adsp)
>         return PTR_ERR_OR_ZERO(adsp->px_supply);
>  }
>
> +static void adsp_init_bus_scaling(struct qcom_adsp *adsp)
> +{
> +       adsp->bus_client = of_icc_get(adsp->dev, NULL);
> +       if (!adsp->bus_client)
> +               dev_warn(adsp->dev, "%s: unable to get bus client \n",
> +                       __func__);
> +}
> +
>  static int adsp_pds_attach(struct device *dev, struct device **devs,
>                            char **pd_names)
>  {
> @@ -410,6 +449,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->active_pds,
>                               desc->active_pd_names);
>         if (ret < 0)
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH 1/2] remoteproc: qcom: Add bus scaling capability during bootup

[Mathieu Poirier]

On Mon, 30 Mar 2020 at 10:06, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> Hi Rishabh,
>
> On Fri, 27 Mar 2020 at 18:59, Rishabh Bhatnagar <rishabhb@codeaurora.org> wrote:
> >
> > During bootup since remote processors cannot request for
> > additional bus bandwidth from the interconect framework,
> > platform driver should provide the proxy resources. This
> > is useful for scenarios where the Q6 tries to access the DDR
> > memory in the initial stages of bootup. For e.g. during
> > bootup or after recovery modem Q6 tries to zero out the bss
> > section in the DDR. Since this is a big chunk of memory if
> > don't bump up the bandwidth we might encounter timeout issues.
> > This patch makes a proxy vote for maximizing the bus bandwidth
> > during bootup and removes it once processor is up.
> >
> > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>
> The title of this patch contains "[PATCH 1/2]" but only one patch was
> sent to the linux-remoteproc mailing list.  Is this a mistake and this
> is a stand alone patch or another patch did not reach the list?
>

I see that you sent another patch [1] that seems to be stand alone but
when looking into the code function of_icc_get() is called, which does
reference the bindings in [1].

[1]. https://patchwork.kernel.org/patch/11463381/

> Thanks,
> Mathieu
>
> > ---
> >  drivers/remoteproc/qcom_q6v5_pas.c | 43 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 42 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > index edf9d0e..8f5db8d 100644
> > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > @@ -20,6 +20,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>
> > @@ -28,6 +29,9 @@
> >  #include "qcom_q6v5.h"
> >  #include "remoteproc_internal.h"
> >
> > +#define PIL_TZ_AVG_BW  0
> > +#define PIL_TZ_PEAK_BW UINT_MAX
> > +
> >  struct adsp_data {
> >         int crash_reason_smem;
> >         const char *firmware_name;
> > @@ -62,6 +66,7 @@ struct qcom_adsp {
> >         int proxy_pd_count;
> >
> >         int pas_id;
> > +       struct icc_path *bus_client;
> >         int crash_reason_smem;
> >         bool has_aggre2_clk;
> >
> > @@ -124,6 +129,25 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
> >
> >  }
> >
> > +static int do_bus_scaling(struct qcom_adsp *adsp, bool enable)
> > +{
> > +       int rc;
> > +       u32 avg_bw = enable ? PIL_TZ_AVG_BW : 0;
> > +       u32 peak_bw = enable ? PIL_TZ_PEAK_BW : 0;
> > +
> > +       if (adsp->bus_client) {
> > +               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;
> > +               }
> > +       } else
> > +               dev_info(adsp->dev, "Bus scaling not setup for %s\n",
> > +                       adsp->rproc->name);
> > +       return 0;
> > +}
> > +
> >  static int adsp_start(struct rproc *rproc)
> >  {
> >         struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
> > @@ -131,9 +155,13 @@ static int adsp_start(struct rproc *rproc)
> >
> >         qcom_q6v5_prepare(&adsp->q6v5);
> >
> > +       ret = do_bus_scaling(adsp, true);
> > +       if (ret)
> > +               goto disable_irqs;
> > +
> >         ret = adsp_pds_enable(adsp, adsp->active_pds, adsp->active_pd_count);
> >         if (ret < 0)
> > -               goto disable_irqs;
> > +               goto unscale_bus;
> >
> >         ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
> >         if (ret < 0)
> > @@ -183,6 +211,8 @@ static int adsp_start(struct rproc *rproc)
> >         adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
> >  disable_active_pds:
> >         adsp_pds_disable(adsp, adsp->active_pds, adsp->active_pd_count);
> > +unscale_bus:
> > +       do_bus_scaling(adsp, false);
> >  disable_irqs:
> >         qcom_q6v5_unprepare(&adsp->q6v5);
> >
> > @@ -198,6 +228,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)
> > @@ -280,6 +311,14 @@ static int adsp_init_regulator(struct qcom_adsp *adsp)
> >         return PTR_ERR_OR_ZERO(adsp->px_supply);
> >  }
> >
> > +static void adsp_init_bus_scaling(struct qcom_adsp *adsp)
> > +{
> > +       adsp->bus_client = of_icc_get(adsp->dev, NULL);
> > +       if (!adsp->bus_client)
> > +               dev_warn(adsp->dev, "%s: unable to get bus client \n",
> > +                       __func__);
> > +}
> > +
> >  static int adsp_pds_attach(struct device *dev, struct device **devs,
> >                            char **pd_names)
> >  {
> > @@ -410,6 +449,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->active_pds,
> >                               desc->active_pd_names);
> >         if (ret < 0)
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > a Linux Foundation Collaborative Project

Re: [PATCH 1/2] remoteproc: qcom: Add bus scaling capability during bootup

[rishabhb]

On 2020-03-30 09:13, Mathieu Poirier wrote:
> On Mon, 30 Mar 2020 at 10:06, Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
>> 
>> Hi Rishabh,
>> 
>> On Fri, 27 Mar 2020 at 18:59, Rishabh Bhatnagar 
>> <rishabhb@codeaurora.org> wrote:
>> >
>> > During bootup since remote processors cannot request for
>> > additional bus bandwidth from the interconect framework,
>> > platform driver should provide the proxy resources. This
>> > is useful for scenarios where the Q6 tries to access the DDR
>> > memory in the initial stages of bootup. For e.g. during
>> > bootup or after recovery modem Q6 tries to zero out the bss
>> > section in the DDR. Since this is a big chunk of memory if
>> > don't bump up the bandwidth we might encounter timeout issues.
>> > This patch makes a proxy vote for maximizing the bus bandwidth
>> > during bootup and removes it once processor is up.
>> >
>> > Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>> 
>> The title of this patch contains "[PATCH 1/2]" but only one patch was
>> sent to the linux-remoteproc mailing list.  Is this a mistake and this
>> is a stand alone patch or another patch did not reach the list?
>> 
> 
> I see that you sent another patch [1] that seems to be stand alone but
> when looking into the code function of_icc_get() is called, which does
> reference the bindings in [1].
> 
> [1]. https://patchwork.kernel.org/patch/11463381/
> 
>> Thanks,
>> Mathieu
Yes its supposed to be a patchset of 2, somehow the numbering got messed 
up.
I'll resend the two patches so that you can take a look.
>> 
>> > ---
>> >  drivers/remoteproc/qcom_q6v5_pas.c | 43 +++++++++++++++++++++++++++++++++++++-
>> >  1 file changed, 42 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
>> > index edf9d0e..8f5db8d 100644
>> > --- a/drivers/remoteproc/qcom_q6v5_pas.c
>> > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
>> > @@ -20,6 +20,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>
>> > @@ -28,6 +29,9 @@
>> >  #include "qcom_q6v5.h"
>> >  #include "remoteproc_internal.h"
>> >
>> > +#define PIL_TZ_AVG_BW  0
>> > +#define PIL_TZ_PEAK_BW UINT_MAX
>> > +
>> >  struct adsp_data {
>> >         int crash_reason_smem;
>> >         const char *firmware_name;
>> > @@ -62,6 +66,7 @@ struct qcom_adsp {
>> >         int proxy_pd_count;
>> >
>> >         int pas_id;
>> > +       struct icc_path *bus_client;
>> >         int crash_reason_smem;
>> >         bool has_aggre2_clk;
>> >
>> > @@ -124,6 +129,25 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
>> >
>> >  }
>> >
>> > +static int do_bus_scaling(struct qcom_adsp *adsp, bool enable)
>> > +{
>> > +       int rc;
>> > +       u32 avg_bw = enable ? PIL_TZ_AVG_BW : 0;
>> > +       u32 peak_bw = enable ? PIL_TZ_PEAK_BW : 0;
>> > +
>> > +       if (adsp->bus_client) {
>> > +               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;
>> > +               }
>> > +       } else
>> > +               dev_info(adsp->dev, "Bus scaling not setup for %s\n",
>> > +                       adsp->rproc->name);
>> > +       return 0;
>> > +}
>> > +
>> >  static int adsp_start(struct rproc *rproc)
>> >  {
>> >         struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
>> > @@ -131,9 +155,13 @@ static int adsp_start(struct rproc *rproc)
>> >
>> >         qcom_q6v5_prepare(&adsp->q6v5);
>> >
>> > +       ret = do_bus_scaling(adsp, true);
>> > +       if (ret)
>> > +               goto disable_irqs;
>> > +
>> >         ret = adsp_pds_enable(adsp, adsp->active_pds, adsp->active_pd_count);
>> >         if (ret < 0)
>> > -               goto disable_irqs;
>> > +               goto unscale_bus;
>> >
>> >         ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
>> >         if (ret < 0)
>> > @@ -183,6 +211,8 @@ static int adsp_start(struct rproc *rproc)
>> >         adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
>> >  disable_active_pds:
>> >         adsp_pds_disable(adsp, adsp->active_pds, adsp->active_pd_count);
>> > +unscale_bus:
>> > +       do_bus_scaling(adsp, false);
>> >  disable_irqs:
>> >         qcom_q6v5_unprepare(&adsp->q6v5);
>> >
>> > @@ -198,6 +228,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)
>> > @@ -280,6 +311,14 @@ static int adsp_init_regulator(struct qcom_adsp *adsp)
>> >         return PTR_ERR_OR_ZERO(adsp->px_supply);
>> >  }
>> >
>> > +static void adsp_init_bus_scaling(struct qcom_adsp *adsp)
>> > +{
>> > +       adsp->bus_client = of_icc_get(adsp->dev, NULL);
>> > +       if (!adsp->bus_client)
>> > +               dev_warn(adsp->dev, "%s: unable to get bus client \n",
>> > +                       __func__);
>> > +}
>> > +
>> >  static int adsp_pds_attach(struct device *dev, struct device **devs,
>> >                            char **pd_names)
>> >  {
>> > @@ -410,6 +449,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->active_pds,
>> >                               desc->active_pd_names);
>> >         if (ret < 0)
>> > --
>> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> > a Linux Foundation Collaborative Project

Re: [PATCH 1/2] remoteproc: qcom: Add bus scaling capability during bootup

[Bjorn Andersson]

On Fri 27 Mar 17:59 PDT 2020, Rishabh Bhatnagar wrote:

> During bootup since remote processors cannot request for
> additional bus bandwidth from the interconect framework,
> platform driver should provide the proxy resources. This
> is useful for scenarios where the Q6 tries to access the DDR
> memory in the initial stages of bootup. For e.g. during
> bootup or after recovery modem Q6 tries to zero out the bss
> section in the DDR. Since this is a big chunk of memory if
> don't bump up the bandwidth we might encounter timeout issues.
> This patch makes a proxy vote for maximizing the bus bandwidth
> during bootup and removes it once processor is up.
> 
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pas.c | 43 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index edf9d0e..8f5db8d 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -20,6 +20,7 @@
>  #include <linux/qcom_scm.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/remoteproc.h>
> +#include <linux/interconnect.h>

These are sorted alphabetically, please maintain this.

>  #include <linux/soc/qcom/mdt_loader.h>
>  #include <linux/soc/qcom/smem.h>
>  #include <linux/soc/qcom/smem_state.h>
> @@ -28,6 +29,9 @@
>  #include "qcom_q6v5.h"
>  #include "remoteproc_internal.h"
>  
> +#define PIL_TZ_AVG_BW  0
> +#define PIL_TZ_PEAK_BW UINT_MAX

Please just inline these in do_bus_scaling().

> +
>  struct adsp_data {
>  	int crash_reason_smem;
>  	const char *firmware_name;
> @@ -62,6 +66,7 @@ struct qcom_adsp {
>  	int proxy_pd_count;
>  
>  	int pas_id;
> +	struct icc_path *bus_client;

Please rename this proxy_path

>  	int crash_reason_smem;
>  	bool has_aggre2_clk;
>  
> @@ -124,6 +129,25 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
>  
>  }
>  
> +static int do_bus_scaling(struct qcom_adsp *adsp, bool enable)

adsp_bus_vote()

> +{
> +	int rc;

This driver uses "int ret".

> +	u32 avg_bw = enable ? PIL_TZ_AVG_BW : 0;

No need to carry a variable for 0 or 0, jut pass 0 in the function call
directly.

> +	u32 peak_bw = enable ? PIL_TZ_PEAK_BW : 0;
> +
> +	if (adsp->bus_client) {

No need for this check, icc_set_bw(NULL, ..) is a nop.

> +		rc = icc_set_bw(adsp->bus_client, avg_bw, peak_bw);
> +		if (rc) {
> +			dev_err(adsp->dev, "bandwidth request failed(rc:%d)\n",

"failed to request bandwidth: %d\n"

> +				rc);
> +			return rc;
> +		}
> +	} else
> +		dev_info(adsp->dev, "Bus scaling not setup for %s\n",

No need to print this.

> +			adsp->rproc->name);
> +	return 0;
> +}
> +
>  static int adsp_start(struct rproc *rproc)
>  {
>  	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
> @@ -131,9 +155,13 @@ static int adsp_start(struct rproc *rproc)
>  
>  	qcom_q6v5_prepare(&adsp->q6v5);
>  
> +	ret = do_bus_scaling(adsp, true);
> +	if (ret)
> +		goto disable_irqs;
> +
>  	ret = adsp_pds_enable(adsp, adsp->active_pds, adsp->active_pd_count);
>  	if (ret < 0)
> -		goto disable_irqs;
> +		goto unscale_bus;
>  
>  	ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
>  	if (ret < 0)
> @@ -183,6 +211,8 @@ static int adsp_start(struct rproc *rproc)
>  	adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
>  disable_active_pds:
>  	adsp_pds_disable(adsp, adsp->active_pds, adsp->active_pd_count);
> +unscale_bus:
> +	do_bus_scaling(adsp, false);
>  disable_irqs:
>  	qcom_q6v5_unprepare(&adsp->q6v5);
>  
> @@ -198,6 +228,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)
> @@ -280,6 +311,14 @@ static int adsp_init_regulator(struct qcom_adsp *adsp)
>  	return PTR_ERR_OR_ZERO(adsp->px_supply);
>  }
>  
> +static void adsp_init_bus_scaling(struct qcom_adsp *adsp)
> +{
> +	adsp->bus_client = of_icc_get(adsp->dev, NULL);
> +	if (!adsp->bus_client)

!adsp->bus_client means there's no interconnects property in the DT
node, you still need to test for errors with IS_ERR().

And in particular you're not guaranteed that the provider has probed, so
you need to propagate EPROBE_DEFER.

> +		dev_warn(adsp->dev, "%s: unable to get bus client \n",
> +			__func__);

This is a dev_err() for the case of IS_ERR().

And please drop the __func__, it doesn't add any value.

> +}
> +
>  static int adsp_pds_attach(struct device *dev, struct device **devs,
>  			   char **pd_names)
>  {
> @@ -410,6 +449,8 @@ static int adsp_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto free_rproc;
>  
> +	adsp_init_bus_scaling(adsp);
> +

As stated above, you need to propagate actual errors here (i.e. not the
case where of_icc_get() returned NULL, but when it returned IS_ERR())

Regards,
bjorn
>  	ret = adsp_pds_attach(&pdev->dev, adsp->active_pds,
>  			      desc->active_pd_names);
>  	if (ret < 0)
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project