[U-Boot] [linux-sunxi] [PATCH] Revert "sunxi: board: Print error after power initialization fails"

[Olliver Schinagl]

Hey André,

On 31-12-2018 14:10, André Przywara wrote:
> On 31/12/2018 11:27, Michael Trimarchi wrote:
>> Hi
>>
>> On Mon, Dec 31, 2018 at 11:34:51AM +0100, Olliver Schinagl wrote:
>>> Hey André,
>>>
>>> On 31-12-2018 00:23, André Przywara wrote:
>>>> On 29/12/2018 22:10, Olliver Schinagl wrote:
>>>>
>>>> Hi Olliver,
>>>>
>>>>> Luckily we have had no problem with this on our boards, but its sad to
>>>>> see this patch reverted due to the buggy ddr implementation ...
>>>>
>>>> This whole SPL is quite a sensitive construct, so moving things around
>>>> can have interesting effects. For instance try to use any .bss variables
>>>> (global variables initialised to 0) before the DRAM init.
>>>> Also especially the DRAM controller driver was written basically without
>>>> any single line of documentation. So bear with us if we didn't get
>>>> everything 100% correct.
>>> Actually, not exactly true. If you compare the DDR init with the
>>> leaked/shared boot0 I think it was called; you'll see a lot of similarities.
>>> So it's not hard to imageine it was at least inspired by boot0.
> 
> Yeah, possibly. I did look at disassembly and decompilation of libdram
> myself. But still the code was not good quality, and if you look at the
> timing parameters, they don't seem to be quite right, for instance many
> values don't match the JEDEC recommendations, and on most boards we run
> DDR3-1600 chips at 672 MHz (so using 1333 timings).
> So yeah, a lot of guesswork.

Well we also have official source release from Allwinner; 
https://github.com/allwinner-zh/bootloader/blob/master/basic_loader/bsp/bsp_for_a20/init_dram/dram_init.c

Which first was leaked; but after it turned out to contain GPL source; 
was released anyway.

But sure; even allwinner did a lot of guesswork there :)

Olliver

> 
>>> That said, we still have no documentation, no idea who's IP it shares, so it
>>> is really still shooting in the dark here and we are just happy we have
>>> something that does work :) (albeit fragile it turns out now)
> 
> Indeed. I think we have some idea on the origins of the IP (Designware),
> but that doesn't help too much, for various reasons.
> 
>>>>
>>>> I actually wanted to ask you: what was your patch meaning to fix in the
>>>> first place? All the patch did was to move the DRAM init after the CPU
>>>> clock setting, which was the exact reason for the breakage.
>>>> What that power_failed check does is to avoid increasing the CPU
>>>> frequency, before and after that patch. There are no other consequences.
>>>> So the effect would be just a mere change in the order of reporting,
>>>> since we continue execution in any case.
>>>
>>> So it was cosemtic to the code. Mostly 'logical ordering'. First setup the
>>> power, CPU etc, if all that is setup properly, setup the DRAM. With the
>>> hoped side effect that with the faster running CPU init would happen faster
>>> (I guess it does but fails :).
>>>
>>> It was a little weird to first setup the DRAM and only then check if we can
>>> even setup the CPU/PLL properly ...
>>>
>>> It just made more sense to do it that way. I just realized I do have an OPie
>>> zero; so maybe I'll look into it again in a few months to see what's going
>>> wrong and where we can improve.
>>>>
>>>> So was that the only purpose of the patch? I believe that could be
>>>> better done this way, without any side effects:
>>>> ....
>>>> #endif
>>>> #endif
>>>>
>>>> 	if (power_failed)
>>>> 		printf("Error setting up the power controller.\n"
>>>> 		       "CPU frequency not set.\n");
>>>>    <... DRAM init ...>
>>>> 	
>>>> 	if (!power_failed)
>>>> 		clock_set_pll1(CONFIG_SYS_CLK_FREQ);
>>>>
>>>> ....
>>>>
>>
>> diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
>> index ee387127f3..296f9d11bc 100644
>> --- a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
>> +++ b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
>> @@ -208,6 +208,7 @@ struct sunxi_ccm_reg {
>>   #define CCM_PLL1_CTRL_N(n)		((((n) - 1) & 0x1f) << 8)
>>   #define CCM_PLL1_CTRL_P(n)		(((n) & 0x3) << 16)
>>   #define CCM_PLL1_CTRL_EN		(0x1 << 31)
>> +#define CCM_PLL1_CTRL_LOCK		(0x1 << 28)
>>   
>>   #define CCM_PLL3_CTRL_M_SHIFT		0
>>   #define CCM_PLL3_CTRL_M_MASK		(0xf << CCM_PLL3_CTRL_M_SHIFT)
>> diff --git a/arch/arm/mach-sunxi/clock_sun6i.c b/arch/arm/mach-sunxi/clock_sun6i.c
>> index 1628f3a7b6..10759b7775 100644
>> --- a/arch/arm/mach-sunxi/clock_sun6i.c
>> +++ b/arch/arm/mach-sunxi/clock_sun6i.c
>> @@ -142,6 +142,9 @@ void clock_set_pll1(unsigned int clk)
>>   	       ATB_DIV_2 << ATB_DIV_SHIFT |
>>   	       CPU_CLK_SRC_PLL1 << CPU_CLK_SRC_SHIFT,
>>   	       &ccm->cpu_axi_cfg);
>> +
>> +	while (!(readl(&ccm->pll1_cfg) & CCM_PLL1_CTRL_LOCK))
>> +		;
>>   }
>>   #endif
>>
>> And waiting the pll1 to be lock does not change anything?
> 
> I have a similar patch on this matter ready, but it didn't fix the
> issue. Btw: you would need to do this before switching the clock over,
> so replacing the sdelay() call.
> 
>> I don't have a board to test and I don't know why was not implemented
> 
> No idea either, but definitely the current sdelay() is way too short: I
> counted 136 iterations of the LOCK bit loop, without the sdelay. With
> the sdelay(200) there were still 101 iterations left.
> 
> Cheers,
> Andre.
>