-----Original Message-----
From: Bjorn Andersson <redacted>
Sent: Monday, October 4, 2021 8:59 PM
To: Rakesh Pillai <redacted>
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 Mon 15 Mar 07:08 CDT 2021, Rakesh Pillai wrote:

quoted

quoted

-----Original Message-----
From: Bjorn Andersson <redacted>
Sent: Wednesday, March 10, 2021 10:15 PM
To: Rakesh Pillai <redacted>
Cc: agross@kernel.org; ohad@wizery.com; mathieu.poirier@linaro.org;
robh+dt@kernel.org; p.zabel@pengutronix.de; sibis@codeaurora.org;
robh+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:

quoted

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 <redacted>
---
 drivers/remoteproc/qcom_q6v5_adsp.c | 77

++++++++++++++++++++++++++++++++++++-

quoted

 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c

b/drivers/remoteproc/qcom_q6v5_adsp.c

quoted

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);

quoted

+
+	/* 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,

quoted

quoted

&val);

quoted

+		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);

quoted

+
+	/* 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,

quoted

quoted

&val);

quoted

+		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)

quoted

 		dev_err(&pdev->dev, "unable to allocate

remoteproc\n");

quoted

quoted

quoted

 		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)

quoted

 	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

= {

quoted

 	.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",

quoted

 		"qdsp6ss_xo", "qdsp6ss_sleep", "qdsp6ss_core", NULL

@@ -

528,6

quoted

quoted

quoted

+585,8 @@ static const struct adsp_pil_data cdsp_resource_init

= {

quoted

 	.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",

quoted

 		"q6_axim", NULL

@@ -535,7 +594,23 @@ static const struct adsp_pil_data

cdsp_resource_init = {

quoted

 	.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

Hi Bjorn,
Yes, the wpss remoteproc is used by ath11k, where it takes care of starting
the rproc during init.
Ideally the wpss is not supposed to be started until the wifi driver comes
up.

If wifi is started/enabled, the wifi driver can take care of starting the
wpss.
This is the reason behind keeping auto_boot as false for wpss.

Thanks,
Rakesh Pillai

quoted

quoted

quoted

2.7.4