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 <[email protected]>
> > ---
>
> [...]
>
> > @@ -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.
> > + 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
