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 |
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
