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?
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
