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 >