Re: [PATCH 5/7] clk: amlogic: Add S7 PLL clock controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] clk: meson: s7-pll: HDMIPLL_CTRL1 initialized after
  s7_hdmi_pll_div0p5 rate
--

commit 6a51c26cbe37d4a7e3b2407e903405378e1e348b
Author: Carlo Caione <ccaione@baylibre.com>

clk: amlogic: Add S7 PLL clock controller driver

This commit adds the PLL clock controller driver for the Amlogic S7 SoC,
providing the fixed PLL and its dividers, the GP0, HIFI0 and HIFI1 PLLs,
and the HDMI PLL.

> diff --git a/drivers/clk/meson/s7-pll.c b/drivers/clk/meson/s7-pll.c
> new file mode 100644
> index 0000000000000..2aa253a5f2111
> --- /dev/null
> +++ b/drivers/clk/meson/s7-pll.c

[ ... ]

> +static const struct reg_sequence s7_hdmi_pll_init_regs[] = {
> +	{ .reg = HDMIPLL_CTRL0,	.def = 0x00881000 },
> +	{ .reg = HDMIPLL_CTRL1,	.def = 0x2c6011cc },
> +	{ .reg = HDMIPLL_CTRL2,	.def = 0x86801000 },
> +	{ .reg = HDMIPLL_CTRL3,	.def = 0x00000000 },
> +};
> +
> +static const struct clk_div_table s7_hdmi_pll_div0p5_table[] = {
> +	{ .val = 0, .div = 1 },
> +	{ .val = 1, .div = 2 },
> +	{ /* sentinel */ }
> +};
> +
> +static struct clk_regmap s7_hdmi_pll_div0p5 = {
> +	.data = &(struct clk_regmap_div_data) {
> +		.offset = HDMIPLL_CTRL1,
> +		.shift = 19,
> +		.width = 1,
> +		.table = s7_hdmi_pll_div0p5_table,
> +		.flags = CLK_DIVIDER_READ_ONLY,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "hdmi_pll_div0p5",
> +		.ops = &clk_regmap_divider_ro_ops,
> +		.parent_data = &(const struct clk_parent_data) {
> +			.fw_name = "hdmi",
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap s7_hdmi_pll_vco = {
> +	.data = &(struct meson_clk_pll_data) {
> +		.en = {
> +			.reg_off = HDMIPLL_CTRL0,
> +			.shift   = 28,
> +			.width   = 1,
> +		},
> +		.m = {
> +			.reg_off = HDMIPLL_CTRL0,
> +			.shift   = 0,
> +			.width   = 9,
> +		},
> +		.frac = {
> +			.reg_off = HDMIPLL_CTRL3,
> +			.shift   = 0,
> +			.width   = 17,
> +		},
> +		.l = {
> +			.reg_off = HDMIPLL_STS,
> +			.shift   = 31,
> +			.width   = 1,
> +		},
> +		.rst = {
> +			.reg_off = HDMIPLL_CTRL0,
> +			.shift   = 29,
> +			.width   = 1,
> +		},
> +		.range = &s7_hdmi_pll_mult_range,
> +		.init_regs = s7_hdmi_pll_init_regs,
> +		.init_count = ARRAY_SIZE(s7_hdmi_pll_init_regs),
> +		.flags = CLK_MESON_PLL_RST_ACTIVE_LOW |
> +			 CLK_MESON_PLL_NOINIT_ENABLED,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "hdmi_pll_vco",
> +		.ops = &meson_clk_pll_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&s7_hdmi_pll_div0p5.hw
> +		},
> +		.num_parents = 1,
> +	},
> +};

[ ... ]

> +static struct clk_hw *s7_pll_hws[] = {

[ ... ]

> +	[CLKID_HDMI_PLL_DIV0P5]		= &s7_hdmi_pll_div0p5.hw,
> +	[CLKID_HDMI_PLL_VCO]		= &s7_hdmi_pll_vco.hw,

[Severity: Medium]
Can s7_hdmi_pll_div0p5 cache a stale divide-by-2 rate before
s7_hdmi_pll_vco initializes HDMIPLL_CTRL1?

meson_clkc_init() registers s7_pll_hws[] in index order, so
s7_hdmi_pll_div0p5 reads HDMIPLL_CTRL1 bit 19 and caches its rate before
s7_hdmi_pll_vco runs meson_clk_pll_init() and writes
s7_hdmi_pll_init_regs (which clears HDMIPLL_CTRL1 bit 19 to 0 when the
PLL is not already enabled by firmware). Because s7_hdmi_pll_div0p5 uses
clk_regmap_divider_ro_ops without CLK_GET_RATE_NOCACHE, its cached rate
is never updated after HDMIPLL_CTRL1 is overwritten, which would leave
s7_hdmi_pll_vco and all downstream HDMI PLL clocks calculating rates off
by a factor of 2 if bit 19 was set prior to initialization.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-upstream-s7-clk-v1-0-7d501d62621f@baylibre.com?part=5

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