Re: [PATCH 3/3] clk: sunxi-ng: add Allwinner B288 CCU driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] clk: sunxi-ng: missing CPUX mux and PLL notifiers in
  sun8i-b288-ccu
- [Low] clk: sunxi-ng: inconsistent module object name for
  sun8i-b288-ccu
--

commit 2d968133b3c297b695cc7e5349c65174d952448b
Author: Pablo Mazzini <pmazzini@gmail.com>

clk: sunxi-ng: add Allwinner B288 CCU driver

This commit adds a table-based sunxi-ng Clock Control Unit driver for the
Allwinner B288 (sun8iw10p1) SoC.

> diff --git a/drivers/clk/sunxi-ng/Makefile b/drivers/clk/sunxi-ng/Makefile
> index 796f443f4d765..debae396555f6 100644
> --- a/drivers/clk/sunxi-ng/Makefile
> +++ b/drivers/clk/sunxi-ng/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_SUN4I_A10_CCU)	+= sun4i-a10-ccu.o
>  obj-$(CONFIG_SUN5I_CCU)		+= sun5i-ccu.o
>  obj-$(CONFIG_SUN6I_A31_CCU)	+= sun6i-a31-ccu.o
>  obj-$(CONFIG_SUN6I_RTC_CCU)	+= sun6i-rtc-ccu.o
> +obj-$(CONFIG_SUN8I_B288_CCU)	+= ccu-sun8i-b288.o
>  obj-$(CONFIG_SUN8I_A23_CCU)	+= sun8i-a23-ccu.o
>  obj-$(CONFIG_SUN8I_A33_CCU)	+= sun8i-a33-ccu.o
>  obj-$(CONFIG_SUN8I_A83T_CCU)	+= sun8i-a83t-ccu.o

[Severity: Low]
Should this build sun8i-b288-ccu.o (with sun8i-b288-ccu-y +=
ccu-sun8i-b288.o below) like the other drivers in this Makefile?  Adding
ccu-sun8i-b288.o directly names the module ccu-sun8i-b288.ko instead of
sun8i-b288-ccu.ko.

> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-b288.c b/drivers/clk/sunxi-ng/ccu-sun8i-b288.c
> new file mode 100644
> index 0000000000000..6e12ed8248f20
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-b288.c

[ ... ]

> +static const struct ccu_tbl_clk sun8i_b288_clks[CLK_NUMBER] = {
> +	/*
> +	 * PLL_24M @ 0x034 -- B288 only. It feeds the PLLs 24 MHz whatever
> +	 * the crystal: it converts a 26 MHz one and is bypassed with a
> +	 * 24 MHz one.
> +	 */
> +	CCU_TBL_FIXED_RATE(CLK_PLL_24M, "pll-24m", CCU_TBL_HOSC, 24000000),
> +
> +	CCU_TBL_STD1(CLK_PLL_CPUX, "pll-cpux", PLL_CPU, CLK_PLL_24M, UNGATE),

[ ... ]

> +	CCU_TBL_STD(CLK_CPUX, "cpux", CPUX, cpux_parents, CRIT | RATE_PARENT),

[Severity: High]
Can changing the rate of CLK_CPUX hang the system because no CPU mux or
PLL rate-change notifiers are registered?

CLK_CPUX sets CLK_SET_RATE_PARENT, so clk_set_rate() propagates to
pll-cpux and updates its A31-style NKMP factors in place while the CPU is
running from it.  Other A31-family CCU drivers that allow rate propagation
from cpux to pll-cpux (such as ccu-sun50i-a64.c, ccu-sun8i-a33.c and
ccu-sun8i-h3.c) register ccu_mux_notifier_register() to temporarily
reparent cpux to the 24 MHz oscillator during the rate change and
ccu_pll_notifier_register() to gate and ungate the PLL after updating its
factors, whereas sunxi_ccu_table_platform_probe() registers neither.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108155702.361022-1-pmazzini@gmail.com?part=3