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

[Michael Trimarchi]

Hi

On Mon, Dec 31, 2018 at 01:10:43PM +0000, 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.
> 

According to my experience on A33 the idea is to use min_timing for all the supported
frequency but even there I found some mistakes.

We need to add anyway a function to set parameters manually for all the
boards. If the problem is timing they should crash in linux too. I was looking
on get_timer and delay implementation if those can depend on cpu(s) but look
like that this is not a possibility.

Anyway without board I can not help

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

You are right :( I just wrote to give an idea.

Michael

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

-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |