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

[André Przywara]

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.

>> 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.