Re: [PATCH 6/9] remoteproc: qcom_q6v5_wcss: support IPQ9574

[Konrad Dybcio]

On 12/19/25 5:34 AM, Alexandru Gagniuc wrote:
> Q6 based firmware loading is also present on IPQ9574, when coupled
> with a wifi-6 device, such as QCN5024. Populate driver data for
> IPQ9574 with values from the downstream 5.4 kerrnel.
> 
> Add the new sequences for the WCSS reset and stop. The downstream
> 5.4 kernel calls these "Q6V7", so keep the name. This is still worth
> using with the "q6v5" driver because all other parts of the driver
> can be seamlessly reused.
> 
> The IPQ9574 uses two sets of clocks. the first, dubbed "q6_clocks"
> must be enabled before the Q6 is started by writing the Q6SS_RST_EVB
> register. The second set of clocks, "clks" should only be enabled
> after the Q6 is placed out of reset. Otherwise, the host CPU core that
> tries to start the remoteproc will hang.
> 
> The downstream kernel had a funny comment, "Pray god and wait for
> reset to complete", which I decided to keep for entertainment value.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@xxxxxxxxx>
> ---

[...]

> @@ -128,6 +137,12 @@ struct q6v5_wcss {
>  	struct clk *qdsp6ss_xo_cbcr;
>  	struct clk *qdsp6ss_core_gfmux;
>  	struct clk *lcc_bcr_sleep;
> +	struct clk_bulk_data *clks;
> +	/* clocks that must be started before the Q6 is booted */
> +	struct clk_bulk_data *q6_clks;

"pre_boot_clks" or something along those lines?

In general i'm not super stoked to see another platform where manual and
through-TZ bringup of remoteprocs is supposed to be supported in parallel..

Are you sure your firmware doesn't allow you to just do a simple
qcom_scm_pas_auth_and_reset() like in the multipd series?


> +	int num_clks;
> +	int num_q6_clks;
> +
>  	struct regulator *cx_supply;
>  	struct qcom_sysmon *sysmon;
>  
> @@ -236,6 +251,87 @@ static int q6v5_wcss_reset(struct q6v5_wcss *wcss)
>  	return 0;
>  }
>  
> +static int q6v7_wcss_reset(struct q6v5_wcss *wcss, struct rproc *rproc)
> +{
> +	int ret;
> +	u32 val;
> +
> +	/*1. Set TCSR GLOBAL CFG1*/

Please add a space between the comment markers and the contents

> +	ret = regmap_update_bits(wcss->halt_map,
> +				 wcss->halt_nc + TCSR_GLOBAL_CFG1,
> +				 0xff00, 0x1100);

GENMASK(15, 8), BIT(8) | BIT(12)

> +	if (ret) {
> +		dev_err(wcss->dev, "TCSE_GLOBAL_CFG1 failed\n");

I don't think we should count on regmap to ever fail

> +		return ret;
> +	}
> +
> +	/* Enable Q6 clocks */

Right, this naming gets even more confusing


> +	ret = clk_bulk_prepare_enable(wcss->num_q6_clks, wcss->q6_clks);
> +	if (ret) {
> +		dev_err(wcss->dev, "failed to enable clocks, err=%d\n", ret);
> +		return ret;
> +	};
> +
> +	/* Write bootaddr to EVB so that Q6WCSS will jump there after reset */

That's what a boot address is generally for, no? ;)

> +	writel(rproc->bootaddr >> 4, wcss->reg_base + Q6SS_RST_EVB);
> +
> +	/*2. Deassert AON Reset */
> +	ret = reset_control_deassert(wcss->wcss_aon_reset);
> +	if (ret) {
> +		dev_err(wcss->dev, "wcss_aon_reset failed\n");
> +		clk_bulk_disable_unprepare(wcss->num_clks, wcss->clks);
> +		return ret;
> +	}
> +
> +	/*8. Set mpm configs*/

"MPM"

Why are the indices of your comments not in numerical order?

> +	/*set CFG[18:15]=1*/
> +	val = readl(wcss->rmb_base + SSCAON_CONFIG);
> +	val &= ~SSCAON_MASK;
> +	val |= SSCAON_BUS_EN;
> +	writel(val, wcss->rmb_base + SSCAON_CONFIG);
> +
> +	/*9. Wait for SSCAON_STATUS */
> +	ret = readl_poll_timeout(wcss->rmb_base + SSCAON_STATUS,
> +				 val, (val & 0xffff) == 0x10, 1000,
> +				 Q6SS_TIMEOUT_US * 1000);
> +	if (ret) {
> +		dev_err(wcss->dev, " Boot Error, SSCAON=0x%08X\n", val);
> +		return ret;

You left the clocks on in this path

> +	}
> +
> +	/*3. BHS require xo cbcr to be enabled */
> +	val = readl(wcss->reg_base + Q6SS_XO_CBCR);
> +	val |= 0x1;

That's BIT(0)

In qcom_q6v5_mss.c you'll notice this is defined as Q6SS_CBCR_CLKEN

If you dig a little deeper, you'll also notice a similar name in
drivers/clk/qcom/clk-branch.[ch].. I suppose they just reused the same
kind of HW on the remoteproc side

> +	writel(val, wcss->reg_base + Q6SS_XO_CBCR);
> +
> +	/*4. Enable core cbcr*/
> +	val = readl(wcss->reg_base + Q6SS_CORE_CBCR);
> +	val |= 0x1;
> +	writel(val, wcss->reg_base + Q6SS_CORE_CBCR);
> +
> +	/*5. Enable sleep cbcr*/
> +	val = readl(wcss->reg_base + Q6SS_SLEEP_CBCR);
> +	val |= 0x1;
> +	writel(val, wcss->reg_base + Q6SS_SLEEP_CBCR);
> +
> +	/*6. Boot core start */
> +	writel(0x1, wcss->reg_base + Q6SS_BOOT_CORE_START);
> +	writel(0x1, wcss->reg_base + Q6SS_BOOT_CMD);
> +
> +	/*7. Pray god and wait for reset to complete*/

"ora et labora" - you've done your work, so I'd assume we can
expect success now

> +	ret = readl_poll_timeout(wcss->reg_base + Q6SS_BOOT_STATUS, val,
> +				 (val & 0x01), 20000, 1000);

The timeout is smaller than the retry delay value, this will only spin
once

0x01 is also BIT(0)

But since you never check whether that timeout has actually been
reached, I assume you really stand by the comment!

(you need this hunk):
if (ret) {
	dev_err(wcss->dev, "WCSS boot timed out\n");
	// cleanup
	return -ETIMEDOUT;
}

> +
> +	/* Enable non-Q6 clocks */
> +	ret = clk_bulk_prepare_enable(wcss->num_clks, wcss->clks);
> +	if (ret) {
> +		dev_err(wcss->dev, "failed to enable clocks, err=%d\n", ret);

the previous set of clocks will be left enabled in this case too

> +		return ret;
> +	};
> +
> +	return 0;

If you return ret here, you can drop the return in the above scope

> +}
> +
>  static int q6v5_wcss_start(struct rproc *rproc)
>  {
>  	struct q6v5_wcss *wcss = rproc->priv;
> @@ -270,10 +366,20 @@ static int q6v5_wcss_start(struct rproc *rproc)
>  	if (ret)
>  		goto wcss_q6_reset;
>  
> -	/* Write bootaddr to EVB so that Q6WCSS will jump there after reset */
> -	writel(rproc->bootaddr >> 4, wcss->reg_base + Q6SS_RST_EVB);
> +	switch (wcss->version) {
> +	case WCSS_QCS404:
> +	case WCSS_IPQ8074:
> +		/* Write bootaddr to EVB so that Q6WCSS will jump there after
> +		 * reset.
> +		 */

/* foo */?

[...]

> +static void q6v7_q6_powerdown(struct q6v5_wcss *wcss)
> +{
> +	uint32_t val;

"u32"

> +
> +	q6v5_wcss_halt_axi_port(wcss, wcss->halt_map, wcss->halt_q6);
> +
> +	/* Disable Q6 Core clock -- we don't know what bit 0 means */

I would assume clearing it muxes the clocksource to XO

[...]

> +static int ipq9574_init_clocks(struct q6v5_wcss *wcss)
> +{
> +	static const char *const q6_clks[] = {
> +		"anoc_wcss_axi_m", "q6_ahb", "q6_ahb_s", "q6_axim", "q6ss_boot",
> +		"mem_noc_q6_axi", "sys_noc_wcss_ahb", "wcss_acmt", "wcss_ecahb",
> +		"wcss_q6_tbu" };
> +	static const char *const clks[] = {
> +		"q6_axim2", "wcss_ahb_s", "wcss_axi_m" };

static local variables that we point to? eeeeeeh

> +	int i, ret;
> +
> +	wcss->num_clks = ARRAY_SIZE(clks);
> +	wcss->num_q6_clks = ARRAY_SIZE(q6_clks);
> +
> +	wcss->q6_clks = devm_kcalloc(wcss->dev, wcss->num_q6_clks,
> +				     sizeof(*wcss->q6_clks), GFP_KERNEL);
> +	if (!wcss->q6_clks)
> +		return -ENOMEM;
> +
> +	wcss->clks = devm_kcalloc(wcss->dev, wcss->num_clks,
> +				  sizeof(*wcss->clks), GFP_KERNEL);
> +	if (!wcss->clks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < wcss->num_q6_clks; i++)
> +		wcss->q6_clks[i].id = q6_clks[i];
> +
> +	for (i = 0; i < wcss->num_clks; i++)
> +		wcss->clks[i].id = clks[i];
> +
> +	ret = devm_clk_bulk_get(wcss->dev, wcss->num_q6_clks, wcss->q6_clks);
> +	if (ret < 0)
> +		return ret;
> +
> +	return devm_clk_bulk_get(wcss->dev, wcss->num_clks, wcss->clks);
> +}
> +
> +

double \n

Konrad