On Wed, 31 Jan 2024 14:26:02 +0100
Ludwig Kormann <ludwig.korm...@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.korm...@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.korm...@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
>
