From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olliver Schinagl Date: Mon, 31 Dec 2018 20:31:19 +0100 Subject: [U-Boot] [linux-sunxi] [PATCH] Revert "sunxi: board: Print error after power initialization fails" In-Reply-To: <7a76b629-db30-1ff5-6185-e0a6601fb7a0@arm.com> References: <20181219130039.16990-1-andre.przywara@arm.com> <80C20E95-C350-4419-A1FF-BEA53B7E2641@schinagl.nl> <6db349c6-8781-73f9-3d26-ea8d88b8fdbc@arm.com> <20181231112738.GA18570@panicking> <7a76b629-db30-1ff5-6185-e0a6601fb7a0@arm.com> Message-ID: <7370589b-ac7c-745e-132a-56db3faa056a@schinagl.nl> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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. >