Re: [PATCH 07/27] clk: rockchip: Add RK3572 clock and reset support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] clk: rockchip: missing CLK_AUDIO_FRAC_0..2 in rk3572_clk_branches
- [Medium] clk: rockchip: internal FACTOR(0) branches overwrite PLL_PPLL in rk3572_clk_init()
--

commit 9602441daaa896927eb56cc9b1d0e4d99173c907
Author: Cunhao Lu <1579567540@qq.com>

clk: rockchip: Add RK3572 clock and reset support

This commit adds clock and soft-reset controller driver support for the
Rockchip rk3572 SoC.

> diff --git a/drivers/clk/rockchip/clk-rk3572.c b/drivers/clk/rockchip/clk-rk3572.c
> new file mode 100644
> index 0000000000000..78fd35f5f27d1
> --- /dev/null
> +++ b/drivers/clk/rockchip/clk-rk3572.c

[ ... ]

> +PNAME(mux_24m_frac_int_mclk4_p) = {
> +	"xin24m", "clk_audio_frac_0", "clk_audio_frac_1", "clk_audio_frac_2",
> +	"clk_audio_frac_3", "clk_audio_int_0", "clk_audio_int_1", "clk_audio_int_2",
> +	"sai0_mclk_in", "sai1_mclk_in", "sai2_mclk_in", "sai3_mclk_in",
> +};

[ ... ]

> +PNAME(mux_24m_frac_int_mclki_p) = {
> +	"xin24m", "clk_audio_int_0", "clk_audio_int_1", "clk_audio_int_2", "clk_audio_frac_0",
> +	"clk_audio_frac_1", "clk_audio_frac_2", "clk_audio_frac_3", "sai0_mclk_in",
> +	"sai1_mclk_in", "sai2_mclk_in", "sai3_mclk_in", "sai4_mclk_in",
> +};
> +
> +PNAME(mux_24m_frac_int_mclki_sclki_p) = {
> +	"xin24m", "clk_audio_int_0", "clk_audio_int_1", "clk_audio_int_2", "clk_audio_frac_0",
> +	"clk_audio_frac_1", "clk_audio_frac_2", "clk_audio_frac_3", "sai0_mclk_in",
> +	"sai1_mclk_in", "sai2_mclk_in", "sai3_mclk_in", "sai4_mclk_in", "sai0_sclk_in",
> +	"sai1_sclk_in", "sai2_sclk_in", "sai3_sclk_in", "sai4_sclk_in",
> +};

[ ... ]

> +static struct rockchip_pll_clock rk3572_pll_clks[] __initdata = {
> +	/* Keep BPLL and LPLL enabled for firmware consumers. */
> +	[bpll] = PLL(pll_rk3588_core, PLL_BPLL, "bpll", mux_pll_p,
> +		     CLK_IGNORE_UNUSED, RK3572_PLL_CON(0),
> +		     RK3572_MODE_CON1, 0, 15, 0, rk3572_pll_rates),
> +	[lpll] = PLL(pll_rk3588_core, PLL_LPLL, "lpll", mux_pll_p,
> +		     CLK_IGNORE_UNUSED, RK3572_LPLL_CON(16),
> +		     RK3572_LPLL_MODE_CON0, 0, 15, 0, rk3572_pll_rates),
> +	[vpll] = PLL(pll_rk3588, PLL_VPLL, "vpll", mux_pll_p,
> +		     0, RK3572_PLL_CON(88),
> +		     RK3572_MODE_CON0, 4, 15, 0, rk3572_pll_rates),
> +	[aupll] = PLL(pll_rk3588, PLL_AUPLL, "aupll", mux_aupll_p,
> +		     0, RK3572_PLL_CON(96),
> +		     RK3572_MODE_CON0, 6, 15, 0, rk3572_aupll_rates),
> +	[cpll] = PLL(pll_rk3588, PLL_CPLL, "cpll", mux_pll_p,
> +		     0, RK3572_PLL_CON(104),
> +		     RK3572_MODE_CON0, 8, 15, 0, rk3572_pll_rates),
> +	[gpll] = PLL(pll_rk3588, PLL_GPLL, "gpll", mux_pll_p,
> +		     CLK_IGNORE_UNUSED, RK3572_PLL_CON(112),
> +		     RK3572_MODE_CON0, 2, 15, 0, rk3572_pll_rates),
> +	[ppll] = PLL(pll_rk3588_ddr, PLL_PPLL, "ppll", mux_pll_p,
> +		     0, RK3572_PPLL_CON(128),
> +		     RK3572_PPLL_CON(134), 15, 15, 0, rk3572_ppll_rates),
> +};
> +
> +static struct rockchip_clk_branch rk3572_clk_branches[] __initdata = {
> +	/* fixed */
> +	FACTOR(0, "xin12m", "xin24m", 0, 1, 2),
> +
> +	COMPOSITE_FRACMUX(CLK_32K, "clk_32k", "xin24m", CLK_IS_CRITICAL,
> +			  RK3572_PMU_CLKSEL_CON(21), 0,
> +			RK3572_PMU_CLKGATE_CON(7), 14, GFLAGS, NULL),
> +
> +	FACTOR(0, "clk_spll_div12", "spll", 0, 1, 12),
> +	FACTOR(0, "clk_spll_div6", "spll", 0, 1, 6),
> +	FACTOR(0, "clk_spll_div4", "spll", 0, 1, 4),
> +	FACTOR(0, "clk_spll_div2", "spll", 0, 1, 2),

[Severity: Medium]
Will registering these fixed-factor branches with an id of 0 overwrite the
ppll entry in ctx->clk_data.clks[0]?

In include/dt-bindings/clock/rockchip,rk3572-cru.h, the binding id for ppll
is 0:

    #define PLL_PPLL				0

During rk3572_clk_init() in drivers/clk/rockchip/clk-rk3572.c,
rockchip_clk_register_plls() runs before rockchip_clk_register_branches():

    rockchip_clk_register_plls(ctx, rk3572_pll_clks,
                               ARRAY_SIZE(rk3572_pll_clks),
                               RK3572_GRF_SOC_STATUS0);
    ...
    rockchip_clk_register_branches(ctx, rk3572_clk_branches,
                                   ARRAY_SIZE(rk3572_clk_branches));

When rockchip_clk_register_branches() iterates over rk3572_clk_branches[],
it unconditionally calls rockchip_clk_set_lookup(ctx, clk, list->id) in
drivers/clk/rockchip/clk.h:

    static inline void rockchip_clk_set_lookup(struct rockchip_clk_provider *ctx,
                                               struct clk *clk, unsigned int id)
    {
            ctx->clk_data.clks[id] = clk;
    }

Because xin12m (which also has no consumers in this file) and
clk_spll_div12, clk_spll_div6, clk_spll_div4, and clk_spll_div2 all pass 0
as their id, rockchip_clk_register_branches() overwrites
ctx->clk_data.clks[0] with clk_spll_div2. Any device tree consumer
requesting ppll will receive clk_spll_div2 instead.

[ ... ]

> +	MUX(CLK_AUDIO_FRAC_0_SRC, "clk_audio_frac_0_src", gpll_cpll_aupll_24m_p, 0,
> +	    RK3572_CLKSEL_CON(13), 0, 2, MFLAGS),
> +	MUX(CLK_AUDIO_FRAC_1_SRC, "clk_audio_frac_1_src", gpll_cpll_aupll_24m_p, 0,
> +	    RK3572_CLKSEL_CON(15), 0, 2, MFLAGS),
> +	MUX(CLK_AUDIO_FRAC_2_SRC, "clk_audio_frac_2_src", gpll_cpll_aupll_24m_p, 0,
> +	    RK3572_CLKSEL_CON(17), 0, 2, MFLAGS),
> +	MUX(CLK_AUDIO_FRAC_3_SRC, "clk_audio_frac_3_src", gpll_cpll_aupll_24m_p, 0,
> +	    RK3572_CLKSEL_CON(19), 0, 2, MFLAGS),
> +	COMPOSITE_FRAC(CLK_AUDIO_FRAC_3, "clk_audio_frac_3", "clk_audio_frac_3_src", 0,
> +		       RK3572_CLKSEL_CON(18), 0,
> +			RK3572_CLKGATE_CON(1), 14, GFLAGS),

[Severity: Medium]
Are the fractional divider definitions for clk_audio_frac_0,
clk_audio_frac_1, and clk_audio_frac_2 missing from rk3572_clk_branches[]
here?

In include/dt-bindings/clock/rockchip,rk3572-cru.h, all four fractional
audio clocks and their parent muxes are defined:

    #define CLK_AUDIO_FRAC_0			25
    #define CLK_AUDIO_FRAC_1			26
    #define CLK_AUDIO_FRAC_2			27
    #define CLK_AUDIO_FRAC_3			28
    #define CLK_AUDIO_FRAC_0_SRC		29
    #define CLK_AUDIO_FRAC_1_SRC		30
    #define CLK_AUDIO_FRAC_2_SRC		31
    #define CLK_AUDIO_FRAC_3_SRC		32

However, rk3572_clk_branches[] in drivers/clk/rockchip/clk-rk3572.c only
registers the four source muxes (clk_audio_frac_0_src through
clk_audio_frac_3_src) and clk_audio_frac_3, omitting clk_audio_frac_0,
clk_audio_frac_1, and clk_audio_frac_2.

Because mux_24m_frac_int_mclk4_p, mux_24m_frac_int_mclki_p, and
mux_24m_frac_int_mclki_sclki_p list clk_audio_frac_0, clk_audio_frac_1, and
clk_audio_frac_2 as parents, 13 downstream audio clocks (clk_pdm0_src_top,
mclk_asrc0_src_0, mclk_asrc1_src_1, mclk_pdm0_src_top, mclk_sai0 through
mclk_sai5, and mclk_spdif_tx0 through mclk_spdif_tx2) will fail to resolve
those parents when rk3572_clk_init() -> rockchip_clk_register_branches()
runs, and device tree lookups for those three clock ids will fail to find a
registered clock.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-add-board-ok3572-c-v1-0-0b0fd85a31ee@qq.com?part=7