Message-ID: <41616f5e-68a6-4354-8520-4321e747efc9@oss.qualcomm.com>
Date: Wed, 24 Dec 2025 15:14:56 +0530
From: Vignesh Viswanathan <vignesh.viswanathan@....qualcomm.com>
To: "Alex G." <mr.nuke.me@...il.com>, andersson@...nel.org,
        mathieu.poirier@...aro.org,
        Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
        linux-kernel@...r.kernel.org
Cc: krzk+dt@...nel.org, Philipp Zabel <p.zabel@...gutronix.de>,
        linux-arm-msm@...r.kernel.org, linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH 6/9] remoteproc: qcom_q6v5_wcss: support IPQ9574



On 12/24/2025 1:51 AM, Alex G. wrote:
> On Friday, December 19, 2025 7:20:04 AM CST Konrad Dybcio wrote:
>> 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@...il.com>
>>> ---
>>
>> [...]
>>
>>> @@ -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?
> 
> I like "pre_boot_clocks".
> 
>> 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?
> 
> I am approaching this from the perspective of an aftermarket OS, like OpenWRT. 
> I don't know if the firmware will do the right thing. I can mitigate this for 
> OS-loaded firmware, like ath11k 16/m3 firmware, because I can test the driver 
> and firmware together. I can't do that for bootloader-loaded firmware, so I try 
> to depend on it as little as possible. I hope that native remoterproc loading 
> for IPQ9574 will be allowed.

Hi Alex,

Does this rproc start sequence work on IPQ9574 without using the 
qcom_scm_pas_auth_and_reset ?

Thanks,
Vignesh

> 
>>> +	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)
>  
> I find GENMASK() and or'ed BIT()s harder to read than plain hex constants. 
> Maybe we should use macros, but what should be the names of these two 
> constants?
> 
>>> +	if (ret) {
>>> +		dev_err(wcss->dev, "TCSE_GLOBAL_CFG1 failed\n");
>>
>> I don't think we should count on regmap to ever fail
> 
> I was following q6v5_wcss_start(), which also handles regmap failures. Do you 
> want me to ignore regmap return codes in the code that is added, at the cost 
> of some  inconsistency??
> 
>>> +		return ret;
>>> +	}
>>> +
>>> +	/* Enable Q6 clocks */
>>
>> Right, this naming gets even more confusing
> 
> I'll name it "pre_boot_clocks" and drop the comment in the interest of self-
> documenting code.
> 
>>> +	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? ;)
> 
> I used the same comment from q6v5_wcss_start(). I will shorten the comment.
> 
>>> +	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?
>  
> I started with the spaghetti sequence from the downstream kernel. I unravelled 
> it, and was so happy the code worked, that I forgot to check the numbering. 
> I'll remove the numbers, as they don't add much value..
> 
>>> +	/*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
> 
> Good catch! I will use "goto" centralized exiting to turn off resources on 
> failure.
> 
>>> +	}
>>> +
>>> +	/*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
> 
> I'll use the macro name as suggested. Thank you!
> 
>>> +	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;
>> }
> 
> Good catches! Yes, I definitely meant 20 millisecond timeout (not 1 ms). I will 
> also add the error checking.
> 
>>> +
>>> +	/* 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
> 
> This part will get changed a bit by the centralized exiting. It will be a 
> "goto" (on error) followed by "return 0" (on success).
> 
>>> +}
>>> +
>>>
>>>  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 */?
> 
> I was trying to keep it at 80 characters, but since I will shorten this 
> comment on the new code paths, I will shorten it here too.
> 
>> [...]
>>
>>> +static void q6v7_q6_powerdown(struct q6v5_wcss *wcss)
>>> +{
>>> +	uint32_t val;
>>
>> "u32"
> 
> Okay.
> 
>>> +
>>> +	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
> 
> I wanted "const char *clks[]" originally. I changed it to this in order to 
> appease checkpatch. Should I use my original "const char * []" instead?
> 
> [...]
> 
> Alex
> 
> 
> 
> 

Powered by blists - more mailing lists