From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Trimarchi Date: Mon, 31 Dec 2018 12:27:40 +0100 Subject: [U-Boot] [linux-sunxi] [PATCH] Revert "sunxi: board: Print error after power initialization fails" In-Reply-To: References: <20181219130039.16990-1-andre.przywara@arm.com> <80C20E95-C350-4419-A1FF-BEA53B7E2641@schinagl.nl> <6db349c6-8781-73f9-3d26-ea8d88b8fdbc@arm.com> Message-ID: <20181231112738.GA18570@panicking> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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. > > 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) > > > > > 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 don't have a board to test and I don't know why was not implemented Michael -- 2.17.1 > > > > > > Curiosity is getting the better of me and I cant seem to be able to > > > reproduce the problem. So could you be a little bit more specific on the > > > bug please? > > > > As I wrote in the commit message, the OrangePi Zero breaks straight away > > with this patch, all of the time: 0 MB DRAM. I don't have any other > > affected board to test on, but there were reports of more boards not > > working as well. > > > > Since we shouldn't allow such drastic regressions, I believe it's best > > to just revert the patch, given the short time to the release. Since I > > consider the original patch more cosmetic than anything else, I believe > > this is justified. > > > > Cheers, > > Andre > > > > > On December 29, 2018 7:53:49 PM GMT+01:00, Jagan Teki > > > wrote: > > > > > > On Wed, Dec 19, 2018 at 6:32 PM Andre Przywara wrote: > > > > > > > > > From: "From: Karl Palsson" > > > > > > Commit a8011eb84dfa("sunxi: board: Print error after power > > > initialization > > > fails") moved the DRAM init after the increase of the CPU clock > > > frequency. This lead to various DRAM initialisation failures on some > > > boards (hangs or wrong size reported, on a NanoPi Duo2 and OrangePi > > > Zero, for instance). Lowering the CPU frequency significantly > > > (for instance > > > to 408 MHz) seems to work around the problem, so this points to > > > some timing > > > issues in the DRAM code. > > > > > > Debugging this sounds like a larger job, so let's just revert > > > this patch > > > to bring back those boards. > > > Beside this probably unintended change the patch just moved the > > > error > > > message around, so reverting this is not a real loss. > > > > > > > > > Better mark this as TODO somewhere, may be some one look it later. > > > > > > > > > This reverts commit a8011eb84dfac5187cebf00ed8bc981bdb5c1fa1. > > > > > > Tested-By: Priit Laes > > > Signed-off-by: Karl Palsson > > > Signed-off-by: Andre Przywara > > > > > > > > > Applied to u-boot-sunxi/master > > > > > > > > > -- > > > Sent from my Android device with K-9 Mail. Please excuse my brevity. > > > _______________________________________________ > U-Boot mailing list > U-Boot at lists.denx.de > https://lists.denx.de/listinfo/u-boot -- | Michael Nazzareno Trimarchi Amarula Solutions BV | | COO - Founder Cruquiuskade 47 | | +31(0)851119172 Amsterdam 1018 AM NL | | [`as] http://www.amarulasolutions.com |