On Sun, 23 Mar 2025 12:56:55 +0100
Jernej Škrabec <jernej.skra...@gmail.com> wrote:
Hi Jernej,
thanks for having a look!
> Dne nedelja, 23. marec 2025 ob 12:35:11 Srednjeevropski standardni čas je
> Andre Przywara napisal(a):
> > U-Boot drivers often revert to using C structures for modelling hardware
> > register frames. This creates some problems:
> > - A "struct" is a C language construct to group several variables
> > together. The details of the layout of this struct are partly subject
> > to the compiler's discretion (padding and alignment).
> > - The "packed" attribute would force a certain layout, but we are not
> > using it.
> > - The actual source of information from the data sheet is the register
> > offset. Here we create an artificial struct, carefully tuning the
> > layout (with a lot of reserved members) to match that offset. To help
> > with correctness, we put the desired information as a *comment*,
> > though this is purely for the human reader, and has no effect on the
> > generated layout. This sounds all very backwards.
> > - Using a struct suggests we can assign a pointer and then access the
> > register content via the members. But this is not the case, instead
> > every MMIO register access must go through specific accessor functions,
> > to meet the ordering and access size guarantees the hardware requires.
> > - We share those structs in code shared across multiple SoC families,
> > though most SoCs define their own version of the struct. Members must
> > match in their name, across every SoC, otherwise compilation will fail.
> > We work around this with even more #ifdefs in the shared code.
> > - Some SoCs have an *almost* identical layout, but differ in a few
> > registers. This requires hard to maintain #ifdef's in the struct
> > definition.
> > - Some of the register frames are huge: the H6 CCU device defines 127
> > registers. We use 15 of them. Still the whole frame would need to be
> > described, which is very tedious, but for no reason.
> > - Adding a new SoC often forces people to decide whether to share an
> > existing struct, or to create a new copy. For some cases (say like 80%
> > similarity) this works out badly either way.
> >
> > The Linux kernel heavily frowns upon those register structs, and instead
> > uses a much simpler solution: #define REG_NAME <offset>
> > This easily maps to the actual information from the data sheet, and can
> > much simpler be shared across multiple SoCs, as it allows to have all
> > SoC versions visible, so we can use C "if" statements instead of #ifdef's.
> > Also it requires to just define the registers we need, and we can use
> > alternative locations for some registers much more easily.
> >
> > Drop the usage of "struct sunxi_ccm_reg" in the H6 SPL clock code, by
> > defining the respective register names and their offsets, then adding
> > them to the base pointer.
> > We cannot drop the struct definition quite yet, as it's also used in
> > other drivers, still.
> >
> > Signed-off-by: Andre Przywara <andre.przyw...@arm.com>
> > ---
> > .../include/asm/arch-sunxi/clock_sun50i_h6.h | 12 +++++
> > arch/arm/mach-sunxi/clock_sun50i_h6.c | 52 +++++++++----------
> > 2 files changed, 36 insertions(+), 28 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun50i_h6.h
> > b/arch/arm/include/asm/arch-sunxi/clock_sun50i_h6.h
> > index 76dd33c9477..a485e00f1f3 100644
> > --- a/arch/arm/include/asm/arch-sunxi/clock_sun50i_h6.h
> > +++ b/arch/arm/include/asm/arch-sunxi/clock_sun50i_h6.h
> > @@ -13,6 +13,18 @@
> > #include <linux/bitops.h>
> > #endif
> >
> > +#define CCU_H6_PLL1_CFG 0x000
> > +#define CCU_H6_PLL5_CFG 0x010
> > +#define CCU_H6_PLL6_CFG 0x020
> > +#define CCU_H6_CPU_AXI_CFG 0x500
> > +#define CCU_H6_PSI_AHB1_AHB2_CFG 0x510
> > +#define CCU_H6_AHB3_CFG 0x51c
> > +#define CCU_H6_APB1_CFG 0x520
> > +#define CCU_H6_APB2_CFG 0x524
> > +#define CCU_H6_MBUS_CFG 0x540
> > +#define CCU_H6_UART_GATE_RESET 0x90c
> > +#define CCU_H6_I2C_GATE_RESET 0x91c
>
> I appreciate this work very much and thanks for doing it!
Ah, great, I was hoping for this kind of feedback. I voiced my concern
about this many years ago, but this was met with some reservations back
then.
> However, I
> have just one more request - PLL1, PLL5, PLL6 and similar names are
> relics of very old Allwinner SoC drivers. IMO it would be better to follow
> names from user manual. This would lower confusion what are they to
> uninitiated people.
I agree, and that's literally patch 37/34 of that branch I took this
series from ;-), so stay tuned for that. The reason this is not in here
or before is that it affects all SoCs, and I wanted to keep that
separate and not blocking the A523 series.
Cheers,
Andre
>
> Best regards.
> Jernej
>
> > +
> > struct sunxi_ccm_reg {
> > u32 pll1_cfg; /* 0x000 pll1 (cpux) control */
> > u8 reserved_0x004[12];
> > diff --git a/arch/arm/mach-sunxi/clock_sun50i_h6.c
> > b/arch/arm/mach-sunxi/clock_sun50i_h6.c
> > index b424a7893ea..482d9e0c695 100644
> > --- a/arch/arm/mach-sunxi/clock_sun50i_h6.c
> > +++ b/arch/arm/mach-sunxi/clock_sun50i_h6.c
> > @@ -6,8 +6,7 @@
> > #ifdef CONFIG_XPL_BUILD
> > void clock_init_safe(void)
> > {
> > - struct sunxi_ccm_reg *const ccm =
> > - (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> > + void *const ccm = (void *)SUNXI_CCM_BASE;
> > struct sunxi_prcm_reg *const prcm =
> > (struct sunxi_prcm_reg *)SUNXI_PRCM_BASE;
> >
> > @@ -32,60 +31,59 @@ void clock_init_safe(void)
> >
> > clock_set_pll1(408000000);
> >
> > - writel(CCM_PLL6_DEFAULT, &ccm->pll6_cfg);
> > - while (!(readl(&ccm->pll6_cfg) & CCM_PLL6_LOCK))
> > + writel(CCM_PLL6_DEFAULT, ccm + CCU_H6_PLL6_CFG);
> > + while (!(readl(ccm + CCU_H6_PLL6_CFG) & CCM_PLL6_LOCK))
> > ;
> >
> > - clrsetbits_le32(&ccm->cpu_axi_cfg, CCM_CPU_AXI_APB_MASK |
> > CCM_CPU_AXI_AXI_MASK,
> > + clrsetbits_le32(ccm + CCU_H6_CPU_AXI_CFG,
> > + CCM_CPU_AXI_APB_MASK | CCM_CPU_AXI_AXI_MASK,
> > CCM_CPU_AXI_DEFAULT_FACTORS);
> >
> > - writel(CCM_PSI_AHB1_AHB2_DEFAULT, &ccm->psi_ahb1_ahb2_cfg);
> > + writel(CCM_PSI_AHB1_AHB2_DEFAULT, ccm + CCU_H6_PSI_AHB1_AHB2_CFG);
> > #ifdef CCM_AHB3_DEFAULT
> > - writel(CCM_AHB3_DEFAULT, &ccm->ahb3_cfg);
> > + writel(CCM_AHB3_DEFAULT, ccm + CCU_H6_AHB3_CFG);
> > #endif
> > - writel(CCM_APB1_DEFAULT, &ccm->apb1_cfg);
> > + writel(CCM_APB1_DEFAULT, ccm + CCU_H6_APB1_CFG);
> >
> > /*
> > * The mux and factor are set, but the clock will be enabled in
> > * DRAM initialization code.
> > */
> > - writel(MBUS_CLK_SRC_PLL6X2 | MBUS_CLK_M(3), &ccm->mbus_cfg);
> > + writel(MBUS_CLK_SRC_PLL6X2 | MBUS_CLK_M(3), ccm + CCU_H6_MBUS_CFG);
> > }
> >
> > void clock_init_uart(void)
> > {
> > - struct sunxi_ccm_reg *const ccm =
> > - (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> > + void *const ccm = (void *)SUNXI_CCM_BASE;
> >
> > /* uart clock source is apb2 */
> > writel(APB2_CLK_SRC_OSC24M|
> > APB2_CLK_RATE_N_1|
> > APB2_CLK_RATE_M(1),
> > - &ccm->apb2_cfg);
> > + ccm + CCU_H6_APB2_CFG);
> >
> > /* open the clock for uart */
> > - setbits_le32(&ccm->uart_gate_reset,
> > + setbits_le32(ccm + CCU_H6_UART_GATE_RESET,
> > 1 << (CONFIG_CONS_INDEX - 1));
> >
> > /* deassert uart reset */
> > - setbits_le32(&ccm->uart_gate_reset,
> > + setbits_le32(ccm + CCU_H6_UART_GATE_RESET,
> > 1 << (RESET_SHIFT + CONFIG_CONS_INDEX - 1));
> > }
> >
> > void clock_set_pll1(unsigned int clk)
> > {
> > - struct sunxi_ccm_reg * const ccm =
> > - (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> > + void *const ccm = (void *)SUNXI_CCM_BASE;
> > u32 val;
> >
> > /* Do not support clocks < 288MHz as they need factor P */
> > if (clk < 288000000) clk = 288000000;
> >
> > /* Switch to 24MHz clock while changing PLL1 */
> > - val = readl(&ccm->cpu_axi_cfg);
> > + val = readl(ccm + CCU_H6_CPU_AXI_CFG);
> > val &= ~CCM_CPU_AXI_MUX_MASK;
> > val |= CCM_CPU_AXI_MUX_OSC24M;
> > - writel(val, &ccm->cpu_axi_cfg);
> > + writel(val, ccm + CCU_H6_CPU_AXI_CFG);
> >
> > /* clk = 24*n/p, p is ignored if clock is >288MHz */
> > val = CCM_PLL1_CTRL_EN | CCM_PLL1_LOCK_EN | CCM_PLL1_CLOCK_TIME_2;
> > @@ -94,20 +92,19 @@ void clock_set_pll1(unsigned int clk)
> > val |= CCM_PLL1_OUT_EN;
> > if (IS_ENABLED(CONFIG_SUNXI_GEN_NCAT2))
> > val |= CCM_PLL1_OUT_EN | CCM_PLL1_LDO_EN;
> > - writel(val, &ccm->pll1_cfg);
> > - while (!(readl(&ccm->pll1_cfg) & CCM_PLL1_LOCK)) {}
> > + writel(val, ccm + CCU_H6_PLL1_CFG);
> > + while (!(readl(ccm + CCU_H6_PLL1_CFG) & CCM_PLL1_LOCK)) {}
> >
> > /* Switch CPU to PLL1 */
> > - val = readl(&ccm->cpu_axi_cfg);
> > + val = readl(ccm + CCU_H6_CPU_AXI_CFG);
> > val &= ~CCM_CPU_AXI_MUX_MASK;
> > val |= CCM_CPU_AXI_MUX_PLL_CPUX;
> > - writel(val, &ccm->cpu_axi_cfg);
> > + writel(val, ccm + CCU_H6_CPU_AXI_CFG);
> > }
> >
> > int clock_twi_onoff(int port, int state)
> > {
> > - struct sunxi_ccm_reg *const ccm =
> > - (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> > + void *const ccm = (void *)SUNXI_CCM_BASE;
> > struct sunxi_prcm_reg *const prcm =
> > (struct sunxi_prcm_reg *)SUNXI_PRCM_BASE;
> > u32 value, *ptr;
> > @@ -120,7 +117,7 @@ int clock_twi_onoff(int port, int state)
> > ptr = &prcm->twi_gate_reset;
> > } else {
> > shift = port;
> > - ptr = &ccm->twi_gate_reset;
> > + ptr = ccm + CCU_H6_I2C_GATE_RESET;
> > }
> >
> > /* set the apb clock gate and reset for twi */
> > @@ -136,9 +133,8 @@ int clock_twi_onoff(int port, int state)
> > /* PLL_PERIPH0 clock, used by the MMC driver */
> > unsigned int clock_get_pll6(void)
> > {
> > - struct sunxi_ccm_reg *const ccm =
> > - (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> > - uint32_t rval = readl(&ccm->pll6_cfg);
> > + void *const ccm = (void *)SUNXI_CCM_BASE;
> > + uint32_t rval = readl(ccm + CCU_H6_PLL6_CFG);
> > int n = ((rval & CCM_PLL6_CTRL_N_MASK) >> CCM_PLL6_CTRL_N_SHIFT) + 1;
> > int div2 = ((rval & CCM_PLL6_CTRL_DIV2_MASK) >>
> > CCM_PLL6_CTRL_DIV2_SHIFT) + 1;
> >
>
>
>
>
>
