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

[Ludwig Kormann]

Hi Andre,

thanks for your feedback!

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.

>> 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.
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:
>     /* 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