[U-Boot] [linux-sunxi] [PATCH] Revert "sunxi: board: Print error after power initialization fails"
Michael Trimarchi
michael at amarulasolutions.com
Mon Dec 31 11:27:40 UTC 2018
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
> > > <jagan at amarulasolutions.com> wrote:
> > >
> > > On Wed, Dec 19, 2018 at 6:32 PM Andre Przywara <andre.przywara at arm.com> wrote:
> > >
> > >
> > > From: "From: Karl Palsson" <karlp at tweak.net.au>
> > >
> > > 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 <plaes at plaes.org>
> > > Signed-off-by: Karl Palsson <karlp at tweak.net.au>
> > > Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> > >
> > >
> > > 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 |
More information about the U-Boot
mailing list