[PATCH 1/1] sunxi: sun4i: Reduce cpu clock at SPL initialization to 144 MHz

[Andre Przywara]

On Wed, 31 Jan 2024 14:26:02 +0100
Ludwig Kormann <ludwig.kormann at ict42.de> wrote:

Hi,

> thanks for your feedback!

thanks for the quick reply!

> Am 31.01.24 um 13:36 schrieb Andre Przywara:
> > On Wed, 31 Jan 2024 11:49:43 +0100
> > Ludwig Kormann <ludwig.kormann at ict42.de> wrote:
> >
> > Hi Ludwig,
> >
> > thanks for taking care and sending a patch, though I scratch my head about
> > this a bit. My main concern is why this would be an issue *now*, 11 years
> > after the A20's release, and with tons of boards out there in operation.
> > Also 144 MHz seem a somewhat drastic reduction?
> >  
> We began seeing this issue beginning in early 2023 and it seems to affect
> only a very small percentage of the units. We had to introduce this 
> patch for
> our customers and wanted to also share it with the community.

Thanks for that, much appreciated.

> >> Up until now cpu clock gets initialized at 384 MHz, which is
> >> the highest supported cpu clock.  
> > What do you mean with "highest supported"? Surely the A20 goes up to
> > 960 MHz?  
> You're right, I must have mixed something up there.
> 
> > Also please note that 384 MHz is the PLL1 reset configuration, so it's not
> > something we came up with, but probably some safe value that Allwinner
> > burned into their chips.
> >  
> >> Recent A20 batches show an increased percentage of modules
> >> reacting very sensitive to operating conditions outside the
> >> specifications.  
> > What are those specifications, exactly? Do you have any more reliable
> > data? The datasheet is very quiet on those conditions, it seems.
> > In particular, I couldn't find any official frequency/voltage
> > combinations, it seems like the values in the DTs are just passed on from
> > some BSP drop?  
> Yes, it's hard/impossible to find any reliable information on this.
> Our main reference have been the values in the DTs.
> 
> >  
> >> The cpu dies very shortly after PLLs, core frequency or cpu
> >> voltage are missconfigured. E.g.:
> >> - uboot SPL selects 384 MHz as cpu clock which requires a cpu
> >>    voltage of at least 1.1 V.
> >> - Linux CPU Frequency scaling with most sun7i dts will reduce
> >>    cpu voltage down to 1.0 V.  
> > How so? The mainline DT suggests 1.1V for anything above 312 MHz, and
> > even above 144 MHz for the BananaPi. Are you using any OPs that differ
> > from that?
> >  
> >> - When intiating a reboot or reset from linux the cpu voltage
> >>    may keep the 1.0 V configuration and the cpu dies during SPL
> >>    initialization.  
> > Ah, so you mean we run (in Linux) on a 1.0V OP, probably at a very low CPU
> > frequency, and then the CPU cores reset, leaving the PMIC at 1.0V? And
> > then the SPL programs 384 MHz, which is too high, even for the brief period
> > until we program DCDC2 to 1.4V?  
> Yes, the CPU dies before the voltage gets updated.
> > If you have evidence (those "newer batches"? A20 batches in 2024?) for
> > that, what about 312 MHz? Does that work?  
> The batches are actually from 2022+. We went for 144MHz as it's the lowest
> of the "default" speeds, that also ensures we're "low enough" to (hopefully)
> never trigger the issue again.
> It seems like there's some variation in A20 production that triggers the 
> issue
> and as we don't know any "official" voltage/frequency limits it's better 
> to have
> some safety margin.
> 
> >  
> >> Therefore reduce cpu clock at uboot SPL initialization down
> >> to 144 MHz from 384 MHz.  
> > I am bit concerned about slowing down the initial SPL phase that much, for
> > *all* A20 users. We run the DRAM init with that initial clock, even though
> > the voltage is already up at this point.  
> In my opinion the impact / additional delay for the initial SPL phase 
> should not
> be in a very relevant range actually, as it usually only takes a few 
> hundred milliseconds.

Well, I heard in the past about users that care a lot about boot times,
and were looking for ways to shave of a few dozen milliseconds from the
boot. So a "few hundred ms" would probably upset them. And while I
personally don't really care about this range either, there are a lot of
A20 users out there, so I want to keep the disturbance as low as possible.

> But you're right of course, this would force the lower value onto all 
> A20 users.
> 
> >
> > So if you see issues with those "newer batches" only(?), and since I
> > haven't heard about any issues about that before, can we make this a
> > Kconfig choice? We could make it simple, forcing K to 1, so we just need
> > to divide the frequency by 24 and shift by 8 to get to the register value?  
> I will try to look into this and provide an update.
> >> Signed-off-by: Ludwig Kormann <ludwig.kormann at ict42.de>
> >> ---
> >>   arch/arm/include/asm/arch-sunxi/clock_sun4i.h | 2 +-
> >>   arch/arm/mach-sunxi/clock_sun4i.c             | 2 ++
> >>   2 files changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun4i.h b/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
> >> index 2cec91cb20..252c4c693e 100644
> >> --- a/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
> >> +++ b/arch/arm/include/asm/arch-sunxi/clock_sun4i.h
> >> @@ -141,7 +141,7 @@ struct sunxi_ccm_reg {
> >>   #define CCM_PLL1_CFG_SIG_DELT_PAT_EN_SHIFT	2
> >>   #define CCM_PLL1_CFG_FACTOR_M_SHIFT		0
> >>   
> >> -#define PLL1_CFG_DEFAULT	0xa1005000
> >> +#define PLL1_CFG_DEFAULT	0xa1004c01
> >>   
> >>   #if defined CONFIG_OLD_SUNXI_KERNEL_COMPAT && defined CONFIG_MACH_SUN5I
> >>   /*
> >> diff --git a/arch/arm/mach-sunxi/clock_sun4i.c b/arch/arm/mach-sunxi/clock_sun4i.c
> >> index 8f1d1b65f0..ac3b7a801f 100644
> >> --- a/arch/arm/mach-sunxi/clock_sun4i.c
> >> +++ b/arch/arm/mach-sunxi/clock_sun4i.c
> >> @@ -25,6 +25,7 @@ void clock_init_safe(void)
> >>   	       APB0_DIV_1 << APB0_DIV_SHIFT |
> >>   	       CPU_CLK_SRC_OSC24M << CPU_CLK_SRC_SHIFT,
> >>   	       &ccm->cpu_ahb_apb0_cfg);
> >> +	sdelay(20);  
> > Where does that come from? Can you mention that in the commit message?  
> 
> This delay is required after switching the clock source.
> 
> See "A20 Reference manual v1.4" Page 50 / section "1.5.4.16. 
> CPU/AHB/APB0 CLOCK RATIO(DEFAULT: 0X00010010)":
> "If the clock source is changed, at most to wait for 8 present running 
> clock cycles."
> 
> Also these delays are already correctly beeing used later on in the same 
> file e.g. within clock_set_pll1(...) on line 153:

Ah, very good, thanks for digging that out. Can you make this a separate
patch, then? I am happy to merge this ASAP, and this wouldn't need to be
held up on this discussion here.

Cheers,
Andre.

> >     /* Switch to 24MHz clock while changing PLL1 */
> >     writel(AXI_DIV_1 << AXI_DIV_SHIFT |
> >            AHB_DIV_2 << AHB_DIV_SHIFT |
> >            APB0_DIV_1 << APB0_DIV_SHIFT |
> >            CPU_CLK_SRC_OSC24M << CPU_CLK_SRC_SHIFT,
> >            &ccm->cpu_ahb_apb0_cfg);
> >     sdelay(20);  
> 
> 
> kind regards
> 
> Ludwig
> 
> > Cheers,
> > Andre
> >  
> >>   	writel(PLL1_CFG_DEFAULT, &ccm->pll1_cfg);
> >>   	sdelay(200);
> >>   	writel(AXI_DIV_1 << AXI_DIV_SHIFT |
> >> @@ -32,6 +33,7 @@ void clock_init_safe(void)
> >>   	       APB0_DIV_1 << APB0_DIV_SHIFT |
> >>   	       CPU_CLK_SRC_PLL1 << CPU_CLK_SRC_SHIFT,
> >>   	       &ccm->cpu_ahb_apb0_cfg);
> >> +	sdelay(20);
> >>   #ifdef CONFIG_MACH_SUN7I
> >>   	setbits_le32(&ccm->ahb_gate0, 0x1 << AHB_GATE_OFFSET_DMA);
> >>   #endif  
>