| Message ID | 1615361290-19238-1-git-send-email-pillair@codeaurora.org |
|---|---|
| Headers | show |
| Series | Add support for sc7280 WPSS PIL loading | expand |
> -----Original Message----- > From: Bjorn Andersson <bjorn.andersson@linaro.org> > Sent: Wednesday, March 10, 2021 10:15 PM > To: Rakesh Pillai <pillair@codeaurora.org> > Cc: agross@kernel.org; ohad@wizery.com; mathieu.poirier@linaro.org; > robh+dt@kernel.org; p.zabel@pengutronix.de; sibis@codeaurora.org; linux- > arm-msm@vger.kernel.org; linux-remoteproc@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 2/2] remoteproc: qcom: q6v5_wpss: Add support for > sc7280 WPSS > > On Wed 10 Mar 01:28 CST 2021, Rakesh Pillai wrote: > > > Add support for PIL loading of WPSS processor for SC7280 > > WPSS boot will be requested by the wifi driver and hence > > disable auto-boot for WPSS. Also add a separate shutdown > > sequence handler for WPSS. > > > > Signed-off-by: Rakesh Pillai <pillair@codeaurora.org> > > --- > > drivers/remoteproc/qcom_q6v5_adsp.c | 77 > ++++++++++++++++++++++++++++++++++++- > > 1 file changed, 76 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c > b/drivers/remoteproc/qcom_q6v5_adsp.c > > index e024502..dc6b91d 100644 > > --- a/drivers/remoteproc/qcom_q6v5_adsp.c > > +++ b/drivers/remoteproc/qcom_q6v5_adsp.c > > @@ -58,6 +58,8 @@ struct adsp_pil_data { > > const char *ssr_name; > > const char *sysmon_name; > > int ssctl_id; > > + bool is_wpss; > > + bool auto_boot; > > > > const char **clk_ids; > > int num_clks; > > @@ -96,8 +98,54 @@ struct qcom_adsp { > > struct qcom_rproc_glink glink_subdev; > > struct qcom_rproc_ssr ssr_subdev; > > struct qcom_sysmon *sysmon; > > + > > + int (*shutdown)(struct qcom_adsp *adsp); > > }; > > > > +static int qcom_wpss_shutdown(struct qcom_adsp *adsp) > > +{ > > + unsigned long timeout; > > + unsigned int val; > > + int ret; > > + > > + regmap_write(adsp->halt_map, adsp->halt_lpass + > LPASS_HALTREQ_REG, 1); > > + > > + /* Wait for halt ACK from QDSP6 */ > > + timeout = jiffies + msecs_to_jiffies(ACK_TIMEOUT); > > + for (;;) { > > + ret = regmap_read(adsp->halt_map, > > + adsp->halt_lpass + LPASS_HALTACK_REG, > &val); > > + if (ret || val || time_after(jiffies, timeout)) > > + break; > > + > > + usleep_range(1000, 1100); > > + } > > + > > + /* Place the WPSS processor into reset */ > > + reset_control_assert(adsp->restart); > > + /* wait after asserting subsystem restart from AOSS */ > > + usleep_range(100, 105); > > + /* Remove the WPSS reset */ > > + reset_control_deassert(adsp->restart); > > + > > + usleep_range(100, 105); > > + > > + regmap_write(adsp->halt_map, adsp->halt_lpass + > LPASS_HALTREQ_REG, 0); > > + > > + /* Wait for halt ACK from QDSP6 */ > > + timeout = jiffies + msecs_to_jiffies(ACK_TIMEOUT); > > + for (;;) { > > + ret = regmap_read(adsp->halt_map, > > + adsp->halt_lpass + LPASS_HALTACK_REG, > &val); > > + if (ret || !val || time_after(jiffies, timeout)) > > + break; > > + > > + usleep_range(1000, 1100); > > + } > > + > > + return 0; > > +} > > + > > static int qcom_adsp_shutdown(struct qcom_adsp *adsp) > > { > > unsigned long timeout; > > @@ -270,7 +318,7 @@ static int adsp_stop(struct rproc *rproc) > > if (ret == -ETIMEDOUT) > > dev_err(adsp->dev, "timed out on wait\n"); > > > > - ret = qcom_adsp_shutdown(adsp); > > + ret = adsp->shutdown(adsp); > > if (ret) > > dev_err(adsp->dev, "failed to shutdown: %d\n", ret); > > > > @@ -439,6 +487,8 @@ static int adsp_probe(struct platform_device > *pdev) > > dev_err(&pdev->dev, "unable to allocate remoteproc\n"); > > return -ENOMEM; > > } > > + > > + rproc->auto_boot = desc->auto_boot; > > rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE); > > > > adsp = (struct qcom_adsp *)rproc->priv; > > @@ -447,6 +497,11 @@ static int adsp_probe(struct platform_device > *pdev) > > adsp->info_name = desc->sysmon_name; > > platform_set_drvdata(pdev, adsp); > > > > + if (desc->is_wpss) > > + adsp->shutdown = qcom_wpss_shutdown; > > + else > > + adsp->shutdown = qcom_adsp_shutdown; > > + > > ret = adsp_alloc_memory_region(adsp); > > if (ret) > > goto free_rproc; > > @@ -515,6 +570,8 @@ static const struct adsp_pil_data adsp_resource_init > = { > > .ssr_name = "lpass", > > .sysmon_name = "adsp", > > .ssctl_id = 0x14, > > + .is_wpss = false, > > + .auto_boot = true; > > .clk_ids = (const char*[]) { > > "sway_cbcr", "lpass_ahbs_aon_cbcr", > "lpass_ahbm_aon_cbcr", > > "qdsp6ss_xo", "qdsp6ss_sleep", "qdsp6ss_core", NULL > > @@ -528,6 +585,8 @@ static const struct adsp_pil_data cdsp_resource_init > = { > > .ssr_name = "cdsp", > > .sysmon_name = "cdsp", > > .ssctl_id = 0x17, > > + .is_wpss = false, > > + .auto_boot = true; > > .clk_ids = (const char*[]) { > > "sway", "tbu", "bimc", "ahb_aon", "q6ss_slave", > "q6ss_master", > > "q6_axim", NULL > > @@ -535,7 +594,23 @@ static const struct adsp_pil_data > cdsp_resource_init = { > > .num_clks = 7, > > }; > > > > +static const struct adsp_pil_data wpss_resource_init = { > > + .crash_reason_smem = 626, > > + .firmware_name = "wpss.mdt", > > + .ssr_name = "wpss", > > + .sysmon_name = "wpss", > > + .ssctl_id = 0x19, > > + .is_wpss = true, > > + .auto_boot = false; > > Why is auto_boot false for the WPSS? Wifi driver will start the remote processor when it comes up. We do not want to load it at the start. > > > + .clk_ids = (const char*[]) { > > + "gcc_wpss_ahb_bdg_mst_clk", "gcc_wpss_ahb_clk", > > + "gcc_wpss_rscp_clk", NULL > > + }, > > + .num_clks = 3, > > +}; > > + > > static const struct of_device_id adsp_of_match[] = { > > + { .compatible = "qcom,sc7280-wpss-pil", .data = &wpss_resource_init > }, > > Nit. Please keep things like this sorted alphabetically. Will fix this in the next patchset. Thanks, Rakesh > > Regards, > Bjorn > > > { .compatible = "qcom,qcs404-cdsp-pil", .data = &cdsp_resource_init > }, > > { .compatible = "qcom,sdm845-adsp-pil", .data = > &adsp_resource_init }, > > { }, > > -- > > 2.7.4 > >
On 2021-03-10 12:58, Rakesh Pillai wrote: > Add support for PIL loading of WPSS processor for SC7280 > WPSS boot will be requested by the wifi driver and hence > disable auto-boot for WPSS. Also add a separate shutdown > sequence handler for WPSS. > > Signed-off-by: Rakesh Pillai <pillair@codeaurora.org> > --- > drivers/remoteproc/qcom_q6v5_adsp.c | 77 > ++++++++++++++++++++++++++++++++++++- > 1 file changed, 76 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c > b/drivers/remoteproc/qcom_q6v5_adsp.c > index e024502..dc6b91d 100644 > --- a/drivers/remoteproc/qcom_q6v5_adsp.c > +++ b/drivers/remoteproc/qcom_q6v5_adsp.c > @@ -58,6 +58,8 @@ struct adsp_pil_data { > const char *ssr_name; > const char *sysmon_name; > int ssctl_id; > + bool is_wpss; > + bool auto_boot; > > const char **clk_ids; > int num_clks; > @@ -96,8 +98,54 @@ struct qcom_adsp { > struct qcom_rproc_glink glink_subdev; > struct qcom_rproc_ssr ssr_subdev; > struct qcom_sysmon *sysmon; > + > + int (*shutdown)(struct qcom_adsp *adsp); > }; > > +static int qcom_wpss_shutdown(struct qcom_adsp *adsp) > +{ > + unsigned long timeout; > + unsigned int val; > + int ret; > + > + regmap_write(adsp->halt_map, adsp->halt_lpass + LPASS_HALTREQ_REG, > 1); > + > + /* Wait for halt ACK from QDSP6 */ > + timeout = jiffies + msecs_to_jiffies(ACK_TIMEOUT); > + for (;;) { > + ret = regmap_read(adsp->halt_map, > + adsp->halt_lpass + LPASS_HALTACK_REG, &val); > + if (ret || val || time_after(jiffies, timeout)) > + break; > + > + usleep_range(1000, 1100); > + } please use regmap_read_poll_timeout instead. > + > + /* Place the WPSS processor into reset */ > + reset_control_assert(adsp->restart); > + /* wait after asserting subsystem restart from AOSS */ > + usleep_range(100, 105); > + /* Remove the WPSS reset */ > + reset_control_deassert(adsp->restart); > + > + usleep_range(100, 105); usleep shouldn't be required since aoss-reset drivers put in a usleep of 200-300 on reset assert and de-assert. > + > + regmap_write(adsp->halt_map, adsp->halt_lpass + LPASS_HALTREQ_REG, > 0); > + > + /* Wait for halt ACK from QDSP6 */ > + timeout = jiffies + msecs_to_jiffies(ACK_TIMEOUT); > + for (;;) { > + ret = regmap_read(adsp->halt_map, > + adsp->halt_lpass + LPASS_HALTACK_REG, &val); > + if (ret || !val || time_after(jiffies, timeout)) > + break; > + > + usleep_range(1000, 1100); > + } please use regmap_read_poll_timeout instead. > + > + return 0; > +} > + > static int qcom_adsp_shutdown(struct qcom_adsp *adsp) > { > unsigned long timeout; > @@ -270,7 +318,7 @@ static int adsp_stop(struct rproc *rproc) > if (ret == -ETIMEDOUT) > dev_err(adsp->dev, "timed out on wait\n"); > > - ret = qcom_adsp_shutdown(adsp); > + ret = adsp->shutdown(adsp); > if (ret) > dev_err(adsp->dev, "failed to shutdown: %d\n", ret); > > @@ -439,6 +487,8 @@ static int adsp_probe(struct platform_device *pdev) > dev_err(&pdev->dev, "unable to allocate remoteproc\n"); > return -ENOMEM; > } > + > + rproc->auto_boot = desc->auto_boot; > rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE); > > adsp = (struct qcom_adsp *)rproc->priv; > @@ -447,6 +497,11 @@ static int adsp_probe(struct platform_device > *pdev) > adsp->info_name = desc->sysmon_name; > platform_set_drvdata(pdev, adsp); > > + if (desc->is_wpss) > + adsp->shutdown = qcom_wpss_shutdown; > + else > + adsp->shutdown = qcom_adsp_shutdown; > + > ret = adsp_alloc_memory_region(adsp); > if (ret) > goto free_rproc; > @@ -515,6 +570,8 @@ static const struct adsp_pil_data > adsp_resource_init = { > .ssr_name = "lpass", > .sysmon_name = "adsp", > .ssctl_id = 0x14, > + .is_wpss = false, > + .auto_boot = true; > .clk_ids = (const char*[]) { > "sway_cbcr", "lpass_ahbs_aon_cbcr", "lpass_ahbm_aon_cbcr", > "qdsp6ss_xo", "qdsp6ss_sleep", "qdsp6ss_core", NULL > @@ -528,6 +585,8 @@ static const struct adsp_pil_data > cdsp_resource_init = { > .ssr_name = "cdsp", > .sysmon_name = "cdsp", > .ssctl_id = 0x17, > + .is_wpss = false, > + .auto_boot = true; > .clk_ids = (const char*[]) { > "sway", "tbu", "bimc", "ahb_aon", "q6ss_slave", "q6ss_master", > "q6_axim", NULL > @@ -535,7 +594,23 @@ static const struct adsp_pil_data > cdsp_resource_init = { > .num_clks = 7, > }; > > +static const struct adsp_pil_data wpss_resource_init = { > + .crash_reason_smem = 626, > + .firmware_name = "wpss.mdt", > + .ssr_name = "wpss", > + .sysmon_name = "wpss", > + .ssctl_id = 0x19, > + .is_wpss = true, > + .auto_boot = false; > + .clk_ids = (const char*[]) { > + "gcc_wpss_ahb_bdg_mst_clk", "gcc_wpss_ahb_clk", > + "gcc_wpss_rscp_clk", NULL > + }, > + .num_clks = 3, > +}; > + > static const struct of_device_id adsp_of_match[] = { > + { .compatible = "qcom,sc7280-wpss-pil", .data = &wpss_resource_init > }, should be in sort order. > { .compatible = "qcom,qcs404-cdsp-pil", .data = &cdsp_resource_init > }, > { .compatible = "qcom,sdm845-adsp-pil", .data = &adsp_resource_init > }, > { }, -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoting Rakesh Pillai (2021-03-09 23:28:08) > Add support for PIL loading of WPSS co-processor for SC7280 SOCs. > > Rakesh Pillai (2): > dt-bindings: remoteproc: qcom: Add SC7280 WPSS support > remoteproc: qcom: q6v5_wpss: Add support for sc7280 WPSS > Is this patch series going to be resent?
On 2021-07-23 03:42, Stephen Boyd wrote: > Quoting Rakesh Pillai (2021-03-09 23:28:08) >> Add support for PIL loading of WPSS co-processor for SC7280 SOCs. >> >> Rakesh Pillai (2): >> dt-bindings: remoteproc: qcom: Add SC7280 WPSS support >> remoteproc: qcom: q6v5_wpss: Add support for sc7280 WPSS >> > > Is this patch series going to be resent? Hi Stephen, I posted a v2 for this patch series, with the dt-bindings converted to yaml. Thanks, Rakesh Pillai.
On Mon 15 Mar 07:08 CDT 2021, Rakesh Pillai wrote: > > > > -----Original Message----- > > From: Bjorn Andersson <bjorn.andersson@linaro.org> > > Sent: Wednesday, March 10, 2021 10:15 PM > > To: Rakesh Pillai <pillair@codeaurora.org> > > Cc: agross@kernel.org; ohad@wizery.com; mathieu.poirier@linaro.org; > > robh+dt@kernel.org; p.zabel@pengutronix.de; sibis@codeaurora.org; linux- > > arm-msm@vger.kernel.org; linux-remoteproc@vger.kernel.org; > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH 2/2] remoteproc: qcom: q6v5_wpss: Add support for > > sc7280 WPSS > > > > On Wed 10 Mar 01:28 CST 2021, Rakesh Pillai wrote: > > > > > Add support for PIL loading of WPSS processor for SC7280 > > > WPSS boot will be requested by the wifi driver and hence > > > disable auto-boot for WPSS. Also add a separate shutdown > > > sequence handler for WPSS. > > > > > > Signed-off-by: Rakesh Pillai <pillair@codeaurora.org> > > > --- > > > drivers/remoteproc/qcom_q6v5_adsp.c | 77 > > ++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 76 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c > > b/drivers/remoteproc/qcom_q6v5_adsp.c > > > index e024502..dc6b91d 100644 > > > --- a/drivers/remoteproc/qcom_q6v5_adsp.c > > > +++ b/drivers/remoteproc/qcom_q6v5_adsp.c > > > @@ -58,6 +58,8 @@ struct adsp_pil_data { > > > const char *ssr_name; > > > const char *sysmon_name; > > > int ssctl_id; > > > + bool is_wpss; > > > + bool auto_boot; > > > > > > const char **clk_ids; > > > int num_clks; > > > @@ -96,8 +98,54 @@ struct qcom_adsp { > > > struct qcom_rproc_glink glink_subdev; > > > struct qcom_rproc_ssr ssr_subdev; > > > struct qcom_sysmon *sysmon; > > > + > > > + int (*shutdown)(struct qcom_adsp *adsp); > > > }; > > > > > > +static int qcom_wpss_shutdown(struct qcom_adsp *adsp) > > > +{ > > > + unsigned long timeout; > > > + unsigned int val; > > > + int ret; > > > + > > > + regmap_write(adsp->halt_map, adsp->halt_lpass + > > LPASS_HALTREQ_REG, 1); > > > + > > > + /* Wait for halt ACK from QDSP6 */ > > > + timeout = jiffies + msecs_to_jiffies(ACK_TIMEOUT); > > > + for (;;) { > > > + ret = regmap_read(adsp->halt_map, > > > + adsp->halt_lpass + LPASS_HALTACK_REG, > > &val); > > > + if (ret || val || time_after(jiffies, timeout)) > > > + break; > > > + > > > + usleep_range(1000, 1100); > > > + } > > > + > > > + /* Place the WPSS processor into reset */ > > > + reset_control_assert(adsp->restart); > > > + /* wait after asserting subsystem restart from AOSS */ > > > + usleep_range(100, 105); > > > + /* Remove the WPSS reset */ > > > + reset_control_deassert(adsp->restart); > > > + > > > + usleep_range(100, 105); > > > + > > > + regmap_write(adsp->halt_map, adsp->halt_lpass + > > LPASS_HALTREQ_REG, 0); > > > + > > > + /* Wait for halt ACK from QDSP6 */ > > > + timeout = jiffies + msecs_to_jiffies(ACK_TIMEOUT); > > > + for (;;) { > > > + ret = regmap_read(adsp->halt_map, > > > + adsp->halt_lpass + LPASS_HALTACK_REG, > > &val); > > > + if (ret || !val || time_after(jiffies, timeout)) > > > + break; > > > + > > > + usleep_range(1000, 1100); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > static int qcom_adsp_shutdown(struct qcom_adsp *adsp) > > > { > > > unsigned long timeout; > > > @@ -270,7 +318,7 @@ static int adsp_stop(struct rproc *rproc) > > > if (ret == -ETIMEDOUT) > > > dev_err(adsp->dev, "timed out on wait\n"); > > > > > > - ret = qcom_adsp_shutdown(adsp); > > > + ret = adsp->shutdown(adsp); > > > if (ret) > > > dev_err(adsp->dev, "failed to shutdown: %d\n", ret); > > > > > > @@ -439,6 +487,8 @@ static int adsp_probe(struct platform_device > > *pdev) > > > dev_err(&pdev->dev, "unable to allocate remoteproc\n"); > > > return -ENOMEM; > > > } > > > + > > > + rproc->auto_boot = desc->auto_boot; > > > rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE); > > > > > > adsp = (struct qcom_adsp *)rproc->priv; > > > @@ -447,6 +497,11 @@ static int adsp_probe(struct platform_device > > *pdev) > > > adsp->info_name = desc->sysmon_name; > > > platform_set_drvdata(pdev, adsp); > > > > > > + if (desc->is_wpss) > > > + adsp->shutdown = qcom_wpss_shutdown; > > > + else > > > + adsp->shutdown = qcom_adsp_shutdown; > > > + > > > ret = adsp_alloc_memory_region(adsp); > > > if (ret) > > > goto free_rproc; > > > @@ -515,6 +570,8 @@ static const struct adsp_pil_data adsp_resource_init > > = { > > > .ssr_name = "lpass", > > > .sysmon_name = "adsp", > > > .ssctl_id = 0x14, > > > + .is_wpss = false, > > > + .auto_boot = true; > > > .clk_ids = (const char*[]) { > > > "sway_cbcr", "lpass_ahbs_aon_cbcr", > > "lpass_ahbm_aon_cbcr", > > > "qdsp6ss_xo", "qdsp6ss_sleep", "qdsp6ss_core", NULL > > > @@ -528,6 +585,8 @@ static const struct adsp_pil_data cdsp_resource_init > > = { > > > .ssr_name = "cdsp", > > > .sysmon_name = "cdsp", > > > .ssctl_id = 0x17, > > > + .is_wpss = false, > > > + .auto_boot = true; > > > .clk_ids = (const char*[]) { > > > "sway", "tbu", "bimc", "ahb_aon", "q6ss_slave", > > "q6ss_master", > > > "q6_axim", NULL > > > @@ -535,7 +594,23 @@ static const struct adsp_pil_data > > cdsp_resource_init = { > > > .num_clks = 7, > > > }; > > > > > > +static const struct adsp_pil_data wpss_resource_init = { > > > + .crash_reason_smem = 626, > > > + .firmware_name = "wpss.mdt", > > > + .ssr_name = "wpss", > > > + .sysmon_name = "wpss", > > > + .ssctl_id = 0x19, > > > + .is_wpss = true, > > > + .auto_boot = false; > > > > Why is auto_boot false for the WPSS? > > Wifi driver will start the remote processor when it comes up. We do not want > to load it at the start. > Can you please explain this further? We've had several cases in the past where functional drivers controls a remoteproc instance and makes assumptions about when the remoteproc is up or not. I would like to ensure that we don't design ourselves into such corner (even though I see that the ath11k code for this was merged a long time ago) Regards, Bjorn > > > > > + .clk_ids = (const char*[]) { > > > + "gcc_wpss_ahb_bdg_mst_clk", "gcc_wpss_ahb_clk", > > > + "gcc_wpss_rscp_clk", NULL > > > + }, > > > + .num_clks = 3, > > > +}; > > > + > > > static const struct of_device_id adsp_of_match[] = { > > > + { .compatible = "qcom,sc7280-wpss-pil", .data = &wpss_resource_init > > }, > > > > Nit. Please keep things like this sorted alphabetically. > > Will fix this in the next patchset. > > Thanks, > Rakesh > > > > > Regards, > > Bjorn > > > > > { .compatible = "qcom,qcs404-cdsp-pil", .data = &cdsp_resource_init > > }, > > > { .compatible = "qcom,sdm845-adsp-pil", .data = > > &adsp_resource_init }, > > > { }, > > > -- > > > 2.7.4 > > > >