Re: [PATCH RFC 3/3] clk: meson: t7: Model PLL pre-divider as a divider clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: meson: t7: PLL pre-dividers permanently stuck at 0 due to missing CLK_SET_RATE_PARENT
- [Low] clk: meson: build failure in drivers after struct meson_clk_pll_data change
--

commit 03f0a62ef49427b4fecb0de15b51e80cb45be623
Author: Jian Hu <jian.hu@amlogic.com>

clk: meson: t7: Model PLL pre-divider as a divider clock

This commit replaces the dedicated PLL pre-divider with a standalone
divider clock in the Meson T7 clock driver. The PLL DCO clocks are 
updated to take the new pre-divider clock as their parent instead of 
the input clock directly.

> diff --git a/drivers/clk/meson/t7-pll.c b/drivers/clk/meson/t7-pll.c
> index 0a622f45fa368..049f0c879c458 100644
> --- a/drivers/clk/meson/t7-pll.c
> +++ b/drivers/clk/meson/t7-pll.c
> @@ -71,6 +71,34 @@
>  #define MCLK_PLL_CNTL4		0x10
>  #define MCLK_PLL_STS		0x14
>  
> +static const struct clk_div_table t7_prediv_div_table[] = {
> +	{ .val = 1, .div = 1 },
> +	{ /* sentinel */ }
> +};
> +
> +static struct clk_regmap t7_gp0_prediv = {
> +	.data = &(struct clk_regmap_div_data){
> +		.offset = GP0PLL_CTRL0,
> +		.shift = 10,
> +		.width = 5,
> +		.table = t7_prediv_div_table,
> +		/*
> +		 * The hardware reset value is 0. Allow it during clock registration
> +		 * to avoid a warning from the common divider code.
> +		 * set_rate() will program the valid divider value (1).
> +		 */
> +		.flags = CLK_DIVIDER_ALLOW_ZERO,

[ ... ]

> @@ -97,11 +125,6 @@ static struct clk_regmap t7_gp0_pll_dco = {
>  			.shift   = 0,
>  			.width   = 8,
>  		},
> -		.n = {
> -			.reg_off = GP0PLL_CTRL0,
> -			.shift   = 10,
> -			.width   = 5,
> -		},

[Severity: Low]
When removing the .n parameter initialization here (and presumably from
struct meson_clk_pll_data in a prerequisite commit), does this patch series
also update other Meson drivers like drivers/clk/meson/gxbb.c and
drivers/clk/meson/axg.c? 

Looking at other drivers in the tree, they appear to still statically
initialize .n in their declarations:

drivers/clk/meson/gxbb.c:gxbb_fixed_pll_dco() {
        ...
        .n = {
            .reg_off = HHI_MPLL_CNTL,
            .shift   = 9,
            .width   = 5,
        },
        ...
}

Will leaving these initializations in place cause a build failure with a
'has no member' error for those drivers?

>  		.l = {
>  			.reg_off = GP0PLL_STS,
>  			.shift   = 31,
> @@ -119,8 +142,8 @@ static struct clk_regmap t7_gp0_pll_dco = {
>  	.hw.init = &(struct clk_init_data){
>  		.name = "gp0_pll_dco",
>  		.ops = &meson_clk_pll_ops,
> -		.parent_data = &(const struct clk_parent_data) {
> -			.fw_name = "in0",
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&t7_gp0_prediv.hw
>  		},
>  		.num_parents = 1,
>  	},

[Severity: High]
Since the pre-divider relies on .set_rate() to correct the invalid
hardware reset value of 0, will the common clock framework ever actually
call .set_rate() on the pre-divider?

Without adding the CLK_SET_RATE_PARENT flag here on the DCO clock
(t7_gp0_pll_dco), rate requests to the DCO won't propagate up to the
pre-divider. Could this leave the pre-divider stuck at the invalid 0 state
and permanently prevent the PLL from functioning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-meson_refactor_n-v1-0-3a8ce27121a2@amlogic.com?part=3

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic