Re: [PATCH v7 6/8] clk: sunxi-ng: a733: Add mod clocks support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] clk: sunxi-ng: a733: unintended CLK_SET_RATE_PARENT on
  eink_clk
- [Medium] clk: sunxi-ng: a733: missing CLK_SET_RATE_PARENT on media mod
  clocks
--

commit 6e107a79cabf47c1760cd7e323625401bc6ffc47
Author: Junhui Liu <junhui.liu@pigmoral.tech>

clk: sunxi-ng: a733: Add mod clocks support

This commit adds module clock definitions for the Allwinner A733 SoC to the
sun60i-a733 CCU driver.

> diff --git a/drivers/clk/sunxi-ng/ccu-sun60i-a733.c b/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
> index 8bc603393f5a5..5ea65bf5b5e05 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun60i-a733.c

[ ... ]

> @@ -511,6 +521,893 @@ static SUNXI_CCU_M_DATA_WITH_MUX_GATE_FEAT(mbus_clk, "mbus", mbus_parents, 0x588

[ ... ]

> +static const struct clk_hw *eink_parents[] = {
> +	&pll_periph0_480M_clk.common.hw,
> +	&pll_periph0_400M_clk.hw,
> +};
> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(eink_clk, "eink", eink_parents, 0xa60,
> +				    0, 5,	/* M */
> +				    24, 3,	/* mux */
> +				    BIT(31),	/* gate */
> +				    CLK_SET_RATE_PARENT);

[Severity: Medium]
Should eink_clk omit CLK_SET_RATE_PARENT?  Unlike eink_panel_clk, eink_parents
only contains pll_periph0_480M_clk and pll_periph0_400M_clk.  Because
pll_periph0_480M_clk uses writable ccu_div_ops without read-only or gate
protection, calling clk_set_rate() on eink_clk can reprogram the shared
480 MHz peripheral PLL divider used by UART, GIC, MBUS, SPI, UFS, and USB
clocks.

[ ... ]

> +static const struct clk_hw *npu_parents[] = {
> +	&pll_npu_clk.common.hw,
> +	&pll_periph0_800M_clk.common.hw,
> +	&pll_periph0_600M_clk.hw,
> +	&pll_periph0_480M_clk.common.hw,
> +	&pll_ve0_clk.common.hw,
> +	&pll_ve1_clk.common.hw,
> +	&pll_de_3x_clk.common.hw,
> +};
> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(npu_clk, "npu", npu_parents, 0xb00,
> +				    0, 5,	/* M */
> +				    24, 3,	/* mux */
> +				    BIT(31),	/* gate */
> +				    0);
> +
> +/*
> + * GPU_CLK = ClockSource * ((16 - M) / 16)
> + * Here we use a div_table to select M values that result in integer divisors.
> + */
> +static struct clk_div_table gpu_div_table[] = {
> +	{ .val = 0, .div = 1 },
> +	{ .val = 8, .div = 2 },
> +	{ .val = 12, .div = 4 },
> +	{ .val = 14, .div = 8 },
> +	{ .val = 15, .div = 16 },
> +	{ /* sentinel */ },
> +};
> +static const struct clk_parent_data gpu_parents[] = {
> +	{ .hw = &pll_gpu0_clk.common.hw, },
> +	{ .hw = &pll_periph0_800M_clk.common.hw, },
> +	{ .hw = &pll_periph0_600M_clk.hw, },
> +	{ .hw = &pll_periph0_400M_clk.hw, },
> +	{ .hw = &pll_periph0_300M_clk.hw, },
> +	{ .hw = &pll_periph0_200M_clk.hw, },
> +};
> +static struct ccu_div gpu0_clk = {
> +	.enable		= BIT(31),
> +	.div		= _SUNXI_CCU_DIV_TABLE(0, 4, gpu_div_table),
> +	.mux		= _SUNXI_CCU_MUX(24, 3),
> +	.common		= {
> +		.reg		= 0xb20,
> +		.features	= CCU_FEATURE_UPDATE_BIT,
> +		.hw.init	= CLK_HW_INIT_PARENTS_DATA("gpu0", gpu_parents,
> +							   &ccu_div_ops, 0),
> +	}
> +};

[ ... ]

> +static const struct clk_hw *i2spcm_parents[] = {
> +	&pll_audio0_4x_clk.common.hw,
> +	&pll_audio1_div2_clk.hw,
> +	&pll_audio1_div5_clk.hw,
> +	&pll_periph0_200M_clk.hw,
> +};
> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(i2spcm0_clk, "i2spcm0", i2spcm_parents, 0x1200,
> +				    0, 5,	/* M */
> +				    24, 3,	/* mux */
> +				    BIT(31),	/* gate */
> +				    0);

[ ... ]

> +static const struct clk_parent_data display_parents[] = {
> +	{ .hw = &pll_video0_4x_clk.common.hw },
> +	{ .hw = &pll_video1_4x_clk.common.hw },
> +	{ .hw = &pll_video2_4x_clk.common.hw },
> +	{ .hw = &pll_periph0_2x_clk.common.hw },
> +};
> +static SUNXI_CCU_M_DATA_WITH_MUX_GATE(tcon_lcd0_clk, "tcon-lcd0", display_parents, 0x1500,
> +				      0, 5,	/* M */
> +				      24, 3,	/* mux */
> +				      BIT(31),	/* gate */
> +				      0);

[Severity: Medium]
Are the display (tcon_lcd0..2_clk, combphy0..1_clk, edp_tv_clk, hdmi_tv_clk),
audio (i2spcm0..4_clk, i2spcm2_asrc_clk, spdif_tx_clk, dmic_clk), GPU
(gpu0_clk), and NPU (npu_clk) module clocks missing CLK_SET_RATE_PARENT?

The parent video and audio PLL post-dividers (pll_video0..2_4x/3x_clk and
pll_audio1_div2/div5_clk) set CLK_SET_RATE_PARENT specifically so downstream
module clocks can adjust the dedicated video and audio PLLs, and pll_audio0,
pll_gpu0, and pll_npu are likewise dedicated to those blocks.  Without
CLK_SET_RATE_PARENT on these module clocks, clk_set_rate() is limited to
dividing the boot-time PLL rate and cannot configure exact pixel clocks, audio
sample rates, or GPU/NPU frequencies.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-a733-clk-v7-0-db7c5451ffa2@pigmoral.tech?part=6