Re: [PATCH 1/7] drivers: clk: meson: Add Amlogic T7 fix pll support

[Jerome Brunet]

On mer. 18 févr. 2026 at 11:17, Ronald Claveau <linux-kernel-dev@xxxxxxxx> wrote:

> Add PLL for the clock controller of the Amlogic T7 SoC family.
>

As Krzysztof pointed out, a series like this needs a cover letter to explain
what you are trying to acheive overall, and proper threading.

The description above is too vague.

> Signed-off-by: Ronald Claveau <linux-kernel-dev@xxxxxxxx>
> ---
>  drivers/clk/meson/t7-pll.c | 257 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 257 insertions(+)
>
> diff --git a/drivers/clk/meson/t7-pll.c b/drivers/clk/meson/t7-pll.c
> index 0a622f45fa36..3dd3aca50b7c 100644
> --- a/drivers/clk/meson/t7-pll.c
> +++ b/drivers/clk/meson/t7-pll.c
> @@ -71,6 +71,15 @@
>  #define MCLK_PLL_CNTL4		0x10
>  #define MCLK_PLL_STS		0x14
>  
> +#define FPLL_CTRL0		0x00
> +#define FPLL_CTRL1		0x04
> +#define FPLL_CTRL2		0x08
> +#define FPLL_CTRL3		0x0c
> +#define FPLL_CTRL4		0x10
> +#define FPLL_CTRL5		0x14
> +#define FPLL_CTRL6		0x18
> +#define FPLL_STS		0x1c

The PLL you are adding is described in the datasheet as the MPLL. FPLL
is nowhere to be found. Prefer using names that relate to the
documentation.

If you must make a name up, you need to have a very good reason and to
explain it.

Still from the public documentation, this PLL belong in the same device as
MPLL0, MPLL1, etc ... BUT, I remember correctly the T7 initial
submission, the fixed PLL and fdivs are supposed to be provided through
SCMI clocks. Have you checked that ?

> +
>  static const struct pll_mult_range t7_media_pll_mult_range = {
>  	.min = 125,
>  	.max = 250,
> @@ -1047,6 +1056,253 @@ static const struct meson_clkc_data t7_mclk_data = {
>  	},
>  };
>  
> +static struct clk_regmap t7_fpll_dco = {
> +	.data = &(struct meson_clk_pll_data){
> +		.en = {
> +			.reg_off = FPLL_CTRL0,
> +			.shift   = 28,
> +			.width   = 1,
> +		},
> +		.m = {
> +			.reg_off = FPLL_CTRL0,
> +			.shift   = 0,
> +			.width   = 8,
> +		},
> +		.n = {
> +			.reg_off = FPLL_CTRL0,
> +			.shift   = 10,
> +			.width   = 5,
> +		},
> +		.frac = {
> +			.reg_off = FPLL_CTRL1,
> +			.shift   = 0,
> +			.width   = 17,
> +		},
> +		.l = {
> +			.reg_off = FPLL_CTRL0,
> +			.shift   = 31,
> +			.width   = 1,
> +		},
> +		.rst = {
> +			.reg_off = FPLL_CTRL0,
> +			.shift   = 29,
> +			.width   = 1,
> +		},
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fpll_dco",
> +		.ops = &meson_clk_pll_ro_ops,
> +		.parent_data = &(const struct clk_parent_data) {
> +			.fw_name = "xtal",
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap t7_fpll = {
> +	.data = &(struct clk_regmap_div_data){
> +		.offset = FPLL_CTRL0,
> +		.shift = 16,
> +		.width = 2,
> +		.flags = CLK_DIVIDER_POWER_OF_TWO,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fpll",
> +		.ops = &clk_regmap_divider_ro_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&t7_fpll_dco.hw
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_fixed_factor t7_fdiv2_div = {
> +	.mult = 1,
> +	.div = 2,
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fdiv2_div",
> +		.ops = &clk_fixed_factor_ops,
> +		.parent_hws = (const struct clk_hw *[]) { &t7_fpll.hw },
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap t7_fdiv2 = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = FPLL_CTRL1,
> +		.bit_idx = 24,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fdiv2",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&t7_fdiv2_div.hw
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_IS_CRITICAL,
> +	},
> +};
> +
> +static struct clk_fixed_factor t7_fdiv2p5_div = {
> +	.mult = 2,
> +	.div = 5,
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fdiv2p5_div",
> +		.ops = &clk_fixed_factor_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&t7_fpll.hw
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap t7_fdiv2p5 = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = FPLL_CTRL1,
> +		.bit_idx = 25,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fdiv2p5",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&t7_fdiv2p5_div.hw
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_fixed_factor t7_fdiv3_div = {
> +	.mult = 1,
> +	.div = 3,
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fdiv3_div",
> +		.ops = &clk_fixed_factor_ops,
> +		.parent_hws = (const struct clk_hw *[]) { &t7_fpll.hw },
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap t7_fdiv3 = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = FPLL_CTRL1,
> +		.bit_idx = 20,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fdiv3",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&t7_fdiv3_div.hw
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_IS_CRITICAL,
> +	},
> +};
> +
> +static struct clk_fixed_factor t7_fdiv4_div = {
> +	.mult = 1,
> +	.div = 4,
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fdiv4_div",
> +		.ops = &clk_fixed_factor_ops,
> +		.parent_hws = (const struct clk_hw *[]) { &t7_fpll.hw },
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap t7_fdiv4 = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = FPLL_CTRL1,
> +		.bit_idx = 21,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fdiv4",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&t7_fdiv4_div.hw
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_IS_CRITICAL,
> +	},
> +};
> +
> +static struct clk_fixed_factor t7_fdiv5_div = {
> +	.mult = 1,
> +	.div = 5,
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fdiv5_div",
> +		.ops = &clk_fixed_factor_ops,
> +		.parent_hws = (const struct clk_hw *[]) { &t7_fpll.hw },
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap t7_fdiv5 = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = FPLL_CTRL1,
> +		.bit_idx = 22,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fdiv5",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&t7_fdiv5_div.hw
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_IS_CRITICAL,
> +	},
> +};
> +
> +static struct clk_fixed_factor t7_fdiv7_div = {
> +	.mult = 1,
> +	.div = 7,
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fdiv7_div",
> +		.ops = &clk_fixed_factor_ops,
> +		.parent_hws = (const struct clk_hw *[]) { &t7_fpll.hw },
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap t7_fdiv7 = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = FPLL_CTRL1,
> +		.bit_idx = 23,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fdiv7",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&t7_fdiv7_div.hw
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_IS_CRITICAL,
> +	},
> +};
> +
> +static struct clk_hw *t7_fpll_hw_clks[] = {
> +	[CLKID_FPLL_DCO]	= &t7_fpll_dco.hw,
> +	[CLKID_FPLL]		= &t7_fpll.hw,
> +	[CLKID_FDIV2_DIV]	= &t7_fdiv2_div.hw,
> +	[CLKID_FDIV2]		= &t7_fdiv2.hw,
> +	[CLKID_FDIV2P5_DIV]	= &t7_fdiv2p5_div.hw,
> +	[CLKID_FDIV2P5]		= &t7_fdiv2p5.hw,
> +	[CLKID_FDIV3_DIV]	= &t7_fdiv3_div.hw,
> +	[CLKID_FDIV3]		= &t7_fdiv3.hw,
> +	[CLKID_FDIV4_DIV]	= &t7_fdiv4_div.hw,
> +	[CLKID_FDIV4]		= &t7_fdiv4.hw,
> +	[CLKID_FDIV5_DIV]	= &t7_fdiv5_div.hw,
> +	[CLKID_FDIV5]		= &t7_fdiv5.hw,
> +	[CLKID_FDIV7_DIV]	= &t7_fdiv7_div.hw,
> +	[CLKID_FDIV7]		= &t7_fdiv7.hw,
> +};
> +
> +static const struct meson_clkc_data t7_fpll_data = {
> +	.hw_clks = {
> +		.hws = t7_fpll_hw_clks,
> +		.num = ARRAY_SIZE(t7_fpll_hw_clks),
> +	},
> +};
> +
>  static const struct of_device_id t7_pll_clkc_match_table[] = {
>  	{ .compatible = "amlogic,t7-gp0-pll",	.data = &t7_gp0_data, },
>  	{ .compatible = "amlogic,t7-gp1-pll",	.data = &t7_gp1_data, },
> @@ -1055,6 +1311,7 @@ static const struct of_device_id t7_pll_clkc_match_table[] = {
>  	{ .compatible = "amlogic,t7-mpll",	.data = &t7_mpll_data, },
>  	{ .compatible = "amlogic,t7-hdmi-pll",	.data = &t7_hdmi_data, },
>  	{ .compatible = "amlogic,t7-mclk-pll",	.data = &t7_mclk_data, },
> +	{ .compatible = "amlogic,t7-fpll",	.data = &t7_fpll_data, },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, t7_pll_clkc_match_table);

-- 
Jerome