Re: [PATCH RFC 1/3] clk: meson: pll: Remove the dedicated n parameter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] clk: meson: struct meson_clk_pll_data removal of 'n' breaks compilation
- [High] clk: meson: pll unconditionally requires range, breaking table-based PLLs
- [High] clk: meson: external pre-divider violates PLL disable/reset sequencing
--

commit 9a0d9eb075576d244c161f62134109ad2353a8b7
Author: Jian Hu <jian.hu@amlogic.com>

clk: meson: pll: Remove the dedicated n parameter

The Meson PLL framework models the PLL pre-divider (N) using a dedicated
field in struct meson_clk_pll_data. Since the common clock framework already
provides a generic divider implementation, this patch removes the dedicated
n parameter from the framework and simplifies the PLL rate calculation.

> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
[ ... ]
> @@ -119,145 +107,48 @@ static unsigned int __pll_params_with_frac(unsigned long rate,
[ ... ]
> -static int meson_clk_get_pll_table_index(unsigned int index,
> -					 unsigned int *m,
> -					 unsigned int *n,
> -					 struct meson_clk_pll_data *pll)
> +static int meson_clk_get_pll_multiplier(unsigned long rate,
> +					unsigned long parent_rate,
> +					unsigned int *m,
> +					struct meson_clk_pll_data *pll)
>  {
> -	if (!pll->table[index].n)
> +	if (!pll->range)
>  		return -EINVAL;

[Severity: High]
Does this unconditionally reject any PLL that lacks a .range definition?

Many table-based PLLs in Meson SoC drivers (like axg_gp0_pll_dco) leave
.range as NULL and rely strictly on .table. Without .range, rate calculations
for these PLLs will deterministically fail at runtime.

[ ... ]
> @@ -420,7 +311,7 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
[ ... ]
>  	enabled = meson_parm_read(clk->map, &pll->en);
>  	if (enabled)
>  		meson_clk_pll_disable(hw);
>  
> -	meson_parm_write(clk->map, &pll->n, n);

[Severity: High]
Does stripping the write to the N pre-divider from this protected atomic
section violate the hardware requirement to place the PLL in a disabled and
reset state before its input frequency changes?

By modelling the pre-divider as an external generic CCF clk_divider,
its .set_rate will modify the hardware register immediately without
notifying the running downstream PLL. Can this cause the active PLL to
lose lock and produce an unstable clock, potentially locking up the system?

>  	meson_parm_write(clk->map, &pll->m, m);
>  
>  	if (MESON_PARM_APPLICABLE(&pll->frac)) {
> --- a/drivers/clk/meson/clk-pll.h
> +++ b/drivers/clk/meson/clk-pll.h
> @@ -11,29 +11,17 @@
>  #include <linux/regmap.h>
>  #include "parm.h"
>  
> -struct pll_params_table {
> -	unsigned int	m;
> -	unsigned int	n;
> -};
> -

[Severity: High]
Does removing pll_params_table also remove the framework's capability to
enforce non-contiguous PLL multiplier rules?

>  struct pll_mult_range {
>  	unsigned int	min;
>  	unsigned int	max;
>  };
[ ... ]
>  struct meson_clk_pll_data {
>  	struct parm en;
>  	struct parm m;
> -	struct parm n;

[Severity: Critical]
Does removing the n and table fields from struct meson_clk_pll_data break
the build for untouched SoC platform drivers?

Drivers such as axg.c continue to initialize these fields in their static PLL
definitions (like axg_sys_pll_dco):

static struct clk_regmap axg_sys_pll_dco = {
	.data = &(struct meson_clk_pll_data){
		...
		.n = {
			.reg_off = HHI_SYS_PLL_CNTL,
			.shift   = 9,
			.width   = 5,
		},

This will cause an immediate compiler error because the types have been
removed. Will this breakage persist and break git bisect?

>  	struct parm frac;
>  	struct parm l;
>  	struct parm rst;
[ ... ]
> @@ -41,7 +29,6 @@ struct meson_clk_pll_data {
>  	struct parm l_detect;
>  	const struct reg_sequence *init_regs;
>  	unsigned int init_count;
> -	const struct pll_params_table *table;
>  	const struct pll_mult_range *range;
>  	unsigned int frac_max;
>  	u8 flags;

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

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