Re: [PATCH v2 05/15] clk: sunxi-ng: Add support for the A523/T527 CCU PLLs

[Andre Przywara <andre.przywara@arm.com>]

On Tue, 18 Feb 2025 20:03:38 +0100
Jernej Škrabec <jernej.skrabec@gmail.com> wrote:

Hi Jernej,

many thanks for having a look!

> Dne petek, 14. februar 2025 ob 13:53:49 Srednjeevropski standardni čas je Andre Przywara napisal(a):
> > Add the PLL clocks of the main CCU of the Allwinner A523 and T527 SoCs.
> > The clocks were modelled after the A523 and T527 manual, and double
> > checked by writing all 1's into the respective register, to spot all
> > implemented bits.
> > 
> > The PLL and mod clocks for the two CPU clusters and the DSU are part of
> > a separate CCU, also most audio clocks are collected in a DSP CCU, so
> > both of these clock groups are missing from this driver.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  drivers/clk/sunxi-ng/Kconfig           |   5 +
> >  drivers/clk/sunxi-ng/Makefile          |   2 +
> >  drivers/clk/sunxi-ng/ccu-sun55i-a523.c | 481 +++++++++++++++++++++++++
> >  drivers/clk/sunxi-ng/ccu-sun55i-a523.h |  14 +
> >  drivers/clk/sunxi-ng/ccu_mp.h          |  14 +-
> >  5 files changed, 510 insertions(+), 6 deletions(-)
> >  create mode 100644 drivers/clk/sunxi-ng/ccu-sun55i-a523.c
> >  create mode 100644 drivers/clk/sunxi-ng/ccu-sun55i-a523.h
> > 
> > diff --git a/drivers/clk/sunxi-ng/Kconfig b/drivers/clk/sunxi-ng/Kconfig
> > index b547198a2c654..04efbda847cf9 100644
> > --- a/drivers/clk/sunxi-ng/Kconfig
> > +++ b/drivers/clk/sunxi-ng/Kconfig
> > @@ -52,6 +52,11 @@ config SUN50I_H6_R_CCU
> >  	default y
> >  	depends on ARM64 || COMPILE_TEST
> >  
> > +config SUN55I_A523_CCU
> > +	tristate "Support for the Allwinner A523/T527 CCU"
> > +	default y
> > +	depends on ARM64 || COMPILE_TEST
> > +
> >  config SUN4I_A10_CCU
> >  	tristate "Support for the Allwinner A10/A20 CCU"
> >  	default y
> > diff --git a/drivers/clk/sunxi-ng/Makefile b/drivers/clk/sunxi-ng/Makefile
> > index 6b3ae2b620db6..01a887f7824bb 100644
> > --- a/drivers/clk/sunxi-ng/Makefile
> > +++ b/drivers/clk/sunxi-ng/Makefile
> > @@ -33,6 +33,7 @@ obj-$(CONFIG_SUN50I_A100_R_CCU)	+= sun50i-a100-r-ccu.o
> >  obj-$(CONFIG_SUN50I_H6_CCU)	+= sun50i-h6-ccu.o
> >  obj-$(CONFIG_SUN50I_H6_R_CCU)	+= sun50i-h6-r-ccu.o
> >  obj-$(CONFIG_SUN50I_H616_CCU)	+= sun50i-h616-ccu.o
> > +obj-$(CONFIG_SUN55I_A523_CCU)	+= sun55i-a523-ccu.o
> >  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
> > @@ -58,6 +59,7 @@ sun50i-a100-r-ccu-y		+= ccu-sun50i-a100-r.o
> >  sun50i-h6-ccu-y			+= ccu-sun50i-h6.o
> >  sun50i-h6-r-ccu-y		+= ccu-sun50i-h6-r.o
> >  sun50i-h616-ccu-y		+= ccu-sun50i-h616.o
> > +sun55i-a523-ccu-y		+= ccu-sun55i-a523.o
> >  sun4i-a10-ccu-y			+= ccu-sun4i-a10.o
> >  sun5i-ccu-y			+= ccu-sun5i.o
> >  sun6i-a31-ccu-y			+= ccu-sun6i-a31.o
> > diff --git a/drivers/clk/sunxi-ng/ccu-sun55i-a523.c b/drivers/clk/sunxi-ng/ccu-sun55i-a523.c
> > new file mode 100644
> > index 0000000000000..8374e841e9d82
> > --- /dev/null
> > +++ b/drivers/clk/sunxi-ng/ccu-sun55i-a523.c
> > @@ -0,0 +1,481 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2023-2024 Arm Ltd.
> > + * Based on the D1 CCU driver:
> > + *   Copyright (c) 2020 huangzhenwei@allwinnertech.com
> > + *   Copyright (C) 2021 Samuel Holland <samuel@sholland.org>
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "../clk.h"
> > +
> > +#include "ccu_common.h"
> > +#include "ccu_reset.h"
> > +
> > +#include "ccu_div.h"
> > +#include "ccu_gate.h"
> > +#include "ccu_mp.h"
> > +#include "ccu_mult.h"
> > +#include "ccu_nk.h"
> > +#include "ccu_nkm.h"
> > +#include "ccu_nkmp.h"
> > +#include "ccu_nm.h"
> > +
> > +#include "ccu-sun55i-a523.h"
> > +
> > +/*
> > + * The 24 MHz oscillator, the root of most of the clock tree.
> > + * .fw_name is the string used in the DT "clock-names" property, used to
> > + * identify the corresponding clock in the "clocks" property.
> > + */
> > +static const struct clk_parent_data osc24M[] = {
> > +	{ .fw_name = "hosc" }
> > +};
> > +
> > +/**************************************************************************
> > + *                              PLLs                                      *
> > + **************************************************************************/
> > +
> > +/* Some PLLs are input * N / div1 / P. Model them as NKMP with no K */
> > +#define SUN55I_A523_PLL_DDR0_REG		0x010
> > +static struct ccu_nkmp pll_ddr0_clk = {
> > +	.enable		= BIT(27),
> > +	.lock		= BIT(28),
> > +	.n		= _SUNXI_CCU_MULT_MIN(8, 8, 11),
> > +	.m		= _SUNXI_CCU_DIV(1, 1), /* input divider */  
> 
> Newer manuals, for example A523 manual v1.4, don't mention input dividers anymore.
> Newer BSP driver doesn't have them either. Should we drop them (for all PLLs)?

I don't know, as you figured, I found them in my copy of the manual. For
the PLLs we need today (PLL_PERIPH0) it shouldn't matter anyway: it's
programmed already, and AFAICS Linux actually never reprograms it(?).

In any case, I just left it in for now: the bit is definitely there, I
verified that in U-Boot by only seeing half of the MMC performance with
bit 1 set. We can remove them anytime later, without issue, can't we?

Actually, thinking about that: the manual pretty clearly says that for
instance PLL_PERIPH0 should be set to 1.2GHz and never changed or even
touched. I wonder if we should honour this somehow in the code? It seems
like the CCF doesn't do this anyway at the moment, but it could, I guess?

> 
> > +	.p		= _SUNXI_CCU_DIV(0, 1), /* output divider */
> > +	.common		= {
> > +		.reg		= 0x010,
> > +		.hw.init	= CLK_HW_INIT_PARENTS_DATA("pll-ddr0", osc24M,
> > +							   &ccu_nkmp_ops,
> > +							   CLK_SET_RATE_GATE |
> > +							   CLK_IS_CRITICAL),
> > +	},
> > +};
> > +

....

> > diff --git a/drivers/clk/sunxi-ng/ccu_mp.h b/drivers/clk/sunxi-ng/ccu_mp.h
> > index 687bd2ec798e2..5311835a4db60 100644
> > --- a/drivers/clk/sunxi-ng/ccu_mp.h
> > +++ b/drivers/clk/sunxi-ng/ccu_mp.h
> > @@ -100,20 +100,22 @@ struct ccu_mp {  
> 
> These changes doesn't belong in this commit.

Ah, indeed, I missed that. Moved that out into another patch now.

Cheers,
Andre

> Other than that, this looks like a good start. Thanks!
> 
> Best regards,
> Jernej
> 
> >  				   _muxshift, _muxwidth,		\
> >  				   0, _flags)
> >  
> > -#define SUNXI_CCU_MP_DATA_WITH_MUX_GATE_FEAT(_struct, _name, _parents, _reg, \
> > +#define SUNXI_CCU_MP_MUX_GATE_POSTDIV_FEAT(_struct, _name, _parents, _reg, \
> >  					_mshift, _mwidth,		\
> >  					_pshift, _pwidth,		\
> >  					_muxshift, _muxwidth,		\
> > -					_gate, _flags,			\
> > -					_features)			\
> > +					_gate, _postdiv,		\
> > +					_flags, _features)		\
> >  	struct ccu_mp _struct = {					\
> >  		.enable	= _gate,					\
> >  		.m	= _SUNXI_CCU_DIV(_mshift, _mwidth),		\
> >  		.p	= _SUNXI_CCU_DIV(_pshift, _pwidth),		\
> >  		.mux	= _SUNXI_CCU_MUX(_muxshift, _muxwidth),		\
> > +		.fixed_post_div = _postdiv,				\
> >  		.common	= {						\
> >  			.reg		= _reg,				\
> > -			.features	= _features,			\
> > +			.features	= CCU_FEATURE_FIXED_POSTDIV |	\
> > +						_features,		\
> >  			.hw.init	= CLK_HW_INIT_PARENTS_DATA(_name, \
> >  								   _parents, \
> >  								   &ccu_mp_ops,\
> > @@ -126,11 +128,11 @@ struct ccu_mp {
> >  					_pshift, _pwidth,		\
> >  					_muxshift, _muxwidth,		\
> >  					_gate, _flags)			\
> > -	SUNXI_CCU_MP_DATA_WITH_MUX_GATE_FEAT(_struct, _name, _parents,	\
> > +	SUNXI_CCU_MP_MUX_GATE_POSTDIV_FEAT(_struct, _name, _parents,	\
> >  					     _reg, _mshift, _mwidth,	\
> >  					     _pshift, _pwidth,		\
> >  					     _muxshift, _muxwidth,	\
> > -					     _gate, _flags, 0)
> > +					     _gate, 1, _flags, 0)
> >  
> >  #define SUNXI_CCU_MP_DATA_WITH_MUX(_struct, _name, _parents, _reg,	\
> >  				   _mshift, _mwidth,			\
> >   
> 
> 
> 
>