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

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

[Andre Przywara]

From: "From: Karl Palsson" <karlp@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.

This reverts commit a8011eb84dfac5187cebf00ed8bc981bdb5c1fa1.

Tested-By: Priit Laes <plaes@plaes.org>
Signed-off-by: Karl Palsson <karlp@tweak.net.au>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 board/sunxi/board.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 917f5b18f6..f022f365e9 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -637,6 +637,13 @@ void sunxi_board_init(void)
 	power_failed |= axp_set_sw(IS_ENABLED(CONFIG_AXP_SW_ON));
 #endif
 #endif
+	printf("DRAM:");
+	gd->ram_size = sunxi_dram_init();
+	printf(" %d MiB\n", (int)(gd->ram_size >> 20));
+	if (!gd->ram_size)
+		hang();
+
+	sunxi_spl_store_dram_size(gd->ram_size);
 
 	/*
 	 * Only clock up the CPU to full speed if we are reasonably
@@ -645,16 +652,7 @@ void sunxi_board_init(void)
 	if (!power_failed)
 		clock_set_pll1(CONFIG_SYS_CLK_FREQ);
 	else
-		printf("Error setting up the power controller.\n"
-		       "CPU frequency not set.\n");
-
-	printf("DRAM:");
-	gd->ram_size = sunxi_dram_init();
-	printf(" %d MiB\n", (int)(gd->ram_size >> 20));
-	if (!gd->ram_size)
-		hang();
-
-	sunxi_spl_store_dram_size(gd->ram_size);
+		printf("Failed to set core voltage! Can't set CPU frequency\n");
 }
 #endif
 
-- 
2.14.5

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

[Jagan Teki]

On Wed, Dec 19, 2018 at 6:32 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> From: "From: Karl Palsson" <karlp@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@plaes.org>
> Signed-off-by: Karl Palsson <karlp@tweak.net.au>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Applied to u-boot-sunxi/master

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

[Olliver Schinagl]

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

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?

On December 29, 2018 7:53:49 PM GMT+01:00, Jagan Teki <jagan@amarulasolutions.com> wrote:
>On Wed, Dec 19, 2018 at 6:32 PM Andre Przywara <andre.przywara@arm.com>
>wrote:
>>
>> From: "From: Karl Palsson" <karlp@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@plaes.org>
>> Signed-off-by: Karl Palsson <karlp@tweak.net.au>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>
>Applied to u-boot-sunxi/master

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

[Priit Laes]

On Sat, Dec 29, 2018 at 11:10:36PM +0100, Olliver Schinagl wrote:
> 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 ...
> 
> 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?

It broke on some H2+ devices.

Just tested this also on H3 Orange Pi PC Plus and here it works fine.

> 
> On December 29, 2018 7:53:49 PM GMT+01:00, Jagan Teki <jagan@amarulasolutions.com> wrote:
> >On Wed, Dec 19, 2018 at 6:32 PM Andre Przywara <andre.przywara@arm.com>
> >wrote:
> >>
> >> From: "From: Karl Palsson" <karlp@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@plaes.org>
> >> Signed-off-by: Karl Palsson <karlp@tweak.net.au>
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >
> >Applied to u-boot-sunxi/master
> 
> -- 
> Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

[André Przywara]

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.

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 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);

....

> 
> 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@amarulasolutions.com> wrote:
> 
>     On Wed, Dec 19, 2018 at 6:32 PM Andre Przywara <andre.przywara@arm.com> wrote:
> 
> 
>         From: "From: Karl Palsson" <karlp@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@plaes.org>
>         Signed-off-by: Karl Palsson <karlp@tweak.net.au>
>         Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> 
>     Applied to u-boot-sunxi/master
> 
> 
> -- 
> Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

[Olliver Schinagl]

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);
> 
> ....
> 
>>
>> 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@amarulasolutions.com> wrote:
>>
>>      On Wed, Dec 19, 2018 at 6:32 PM Andre Przywara <andre.przywara@arm.com> wrote:
>>
>>
>>          From: "From: Karl Palsson" <karlp@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@plaes.org>
>>          Signed-off-by: Karl Palsson <karlp@tweak.net.au>
>>          Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>
>>
>>      Applied to u-boot-sunxi/master
>>
>>
>> -- 
>> Sent from my Android device with K-9 Mail. Please excuse my brevity.
> 

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

[Michael Trimarchi]

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@amarulasolutions.com> wrote:
> > > 
> > >      On Wed, Dec 19, 2018 at 6:32 PM Andre Przywara <andre.przywara@arm.com> wrote:
> > > 
> > > 
> > >          From: "From: Karl Palsson" <karlp@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@plaes.org>
> > >          Signed-off-by: Karl Palsson <karlp@tweak.net.au>
> > >          Signed-off-by: Andre Przywara <andre.przywara@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               |

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

[André Przywara]

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.

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

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

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

[Olliver Schinagl]

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

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

[Priit Laes]

On Wed, Dec 19, 2018 at 01:45:29AM +0000, André Przywara wrote:
> On 19/12/2018 00:51, André Przywara wrote:
> > On 18/12/2018 12:06, Jagan Teki wrote:
> >> On Tue, Dec 18, 2018 at 4:09 PM <karlp@tweak.net.au> wrote:
> >>>
> >>> From: Karl Palsson <karlp@tweak.net.au>
> >>>
> >>> This reverts commit a8011eb84dfac5187cebf00ed8bc981bdb5c1fa1
> >>>
> >>> This causes DRAM init failures on (at least)
> >>> * allwinner h3 nanopi-duo2
> >>> * allwinner h2+ orangepi zero v1.1
> >>>
> >>> Signed-off-by: Karl Palsson <karlp@tweak.net.au>
> >>> ---
> >>>
> >>> Ideally, this should get into 2019.01, so that this release is not
> >>> broken on those targets.
> >>
> >> Better to understand the issue here, since I have BPI-M2+ which boots
> >> fine w/o this revert.
> > 
> > Could this be a .bss issue? This lies in DRAM and is thus only available
> > *after* DRAM init. IIRC we silently rely on not accessing anything in
> > .bss before the DRAM is up, see 59d07ee08e85 for instance.
> > I don't immediately spot any .bss usage in clock_set_pll1(), though.
> > 
> > Or is the 1GHz CPU clock speed too fast for the DRAM init? If I am not
> > mistaken, we run with 24MHz before, so there might be some "natural"
> > delay in some setup routines. Some DRAM chips or board layout might be
> > more susceptible to this than others, which might explain why it only
> > fails on some boards.
> 
> Just did some testing on my OrangePi Zero: if I force the CPU frequency
> to 408 MHz, it works, but fails with the standard 1008 MHz.
> So this smells that we indeed miss some delays in the DRAM setup code,
> which sounds tedious to find. There are a number of delays, but those
> are fine as they are timer controlled, so independent from the CPU
> frequency.

With latest u-boot, I got it only working when lowering 
DRAM_CLK to 312MHz (default being 624 MHz(. Even with 336 and 360 MHz
it hung just after displaying the 'Trying to boot from FEL' message,
while reporting wrong DRAM size (1GiB and 2048MiB).

> 
> For now I think that reverting would be the easiest solution. Somewhat
> in contrast to what the commit message says, we don't really change
> anything in terms of error *checking*, as the code carries on anyways
> (just without increasing the CPU frequency). The only real difference is
> the order of CPU clock setup and DRAM init.
> 
> Karl, can you please amend the commit message to mention the CPU
> frequency issue?
> 
> > So if the original patch is about bailing out on error early, can't we
> > just do *that* before the DRAM init, keeping the CPU clock setup still
> > after DRAM?
> 
> Just checked, this works as well, but is a bit pointless. Just reverting
> this and meanwhile checking the DRAM init code seems the easier solution.
> 
> Cheers,
> Andre
> 
> -- 
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

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

[Michael Nazzareno Trimarchi]

Hi André

On Wed, Dec 19, 2018 at 2:46 AM André Przywara <andre.przywara@arm.com> wrote:
>
> On 19/12/2018 00:51, André Przywara wrote:
> > On 18/12/2018 12:06, Jagan Teki wrote:
> >> On Tue, Dec 18, 2018 at 4:09 PM <karlp@tweak.net.au> wrote:
> >>>
> >>> From: Karl Palsson <karlp@tweak.net.au>
> >>>
> >>> This reverts commit a8011eb84dfac5187cebf00ed8bc981bdb5c1fa1
> >>>
> >>> This causes DRAM init failures on (at least)
> >>> * allwinner h3 nanopi-duo2
> >>> * allwinner h2+ orangepi zero v1.1
> >>>
> >>> Signed-off-by: Karl Palsson <karlp@tweak.net.au>
> >>> ---
> >>>
> >>> Ideally, this should get into 2019.01, so that this release is not
> >>> broken on those targets.
> >>
> >> Better to understand the issue here, since I have BPI-M2+ which boots
> >> fine w/o this revert.
> >
> > Could this be a .bss issue? This lies in DRAM and is thus only available
> > *after* DRAM init. IIRC we silently rely on not accessing anything in
> > .bss before the DRAM is up, see 59d07ee08e85 for instance.
> > I don't immediately spot any .bss usage in clock_set_pll1(), though.
> >
> > Or is the 1GHz CPU clock speed too fast for the DRAM init? If I am not
> > mistaken, we run with 24MHz before, so there might be some "natural"
> > delay in some setup routines. Some DRAM chips or board layout might be
> > more susceptible to this than others, which might explain why it only
> > fails on some boards.
>
> Just did some testing on my OrangePi Zero: if I force the CPU frequency
> to 408 MHz, it works, but fails with the standard 1008 MHz.
> So this smells that we indeed miss some delays in the DRAM setup code,
> which sounds tedious to find. There are a number of delays, but those
> are fine as they are timer controlled, so independent from the CPU
> frequency.
>
> For now I think that reverting would be the easiest solution. Somewhat
> in contrast to what the commit message says, we don't really change
> anything in terms of error *checking*, as the code carries on anyways
> (just without increasing the CPU frequency). The only real difference is
> the order of CPU clock setup and DRAM init.
>
> Karl, can you please amend the commit message to mention the CPU
> frequency issue?
>
> > So if the original patch is about bailing out on error early, can't we
> > just do *that* before the DRAM init, keeping the CPU clock setup still
> > after DRAM?
>
> Just checked, this works as well, but is a bit pointless. Just reverting
> this and meanwhile checking the DRAM init code seems the easier solution.
>

Definitely have the same impression. Please send the same revert but
adjust the commit
message

Michael


> Cheers,
> Andre
> _______________________________________________
> 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               |

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

[André Przywara]

On 19/12/2018 00:51, André Przywara wrote:
> On 18/12/2018 12:06, Jagan Teki wrote:
>> On Tue, Dec 18, 2018 at 4:09 PM <karlp@tweak.net.au> wrote:
>>>
>>> From: Karl Palsson <karlp@tweak.net.au>
>>>
>>> This reverts commit a8011eb84dfac5187cebf00ed8bc981bdb5c1fa1
>>>
>>> This causes DRAM init failures on (at least)
>>> * allwinner h3 nanopi-duo2
>>> * allwinner h2+ orangepi zero v1.1
>>>
>>> Signed-off-by: Karl Palsson <karlp@tweak.net.au>
>>> ---
>>>
>>> Ideally, this should get into 2019.01, so that this release is not
>>> broken on those targets.
>>
>> Better to understand the issue here, since I have BPI-M2+ which boots
>> fine w/o this revert.
> 
> Could this be a .bss issue? This lies in DRAM and is thus only available
> *after* DRAM init. IIRC we silently rely on not accessing anything in
> .bss before the DRAM is up, see 59d07ee08e85 for instance.
> I don't immediately spot any .bss usage in clock_set_pll1(), though.
> 
> Or is the 1GHz CPU clock speed too fast for the DRAM init? If I am not
> mistaken, we run with 24MHz before, so there might be some "natural"
> delay in some setup routines. Some DRAM chips or board layout might be
> more susceptible to this than others, which might explain why it only
> fails on some boards.

Just did some testing on my OrangePi Zero: if I force the CPU frequency
to 408 MHz, it works, but fails with the standard 1008 MHz.
So this smells that we indeed miss some delays in the DRAM setup code,
which sounds tedious to find. There are a number of delays, but those
are fine as they are timer controlled, so independent from the CPU
frequency.

For now I think that reverting would be the easiest solution. Somewhat
in contrast to what the commit message says, we don't really change
anything in terms of error *checking*, as the code carries on anyways
(just without increasing the CPU frequency). The only real difference is
the order of CPU clock setup and DRAM init.

Karl, can you please amend the commit message to mention the CPU
frequency issue?

> So if the original patch is about bailing out on error early, can't we
> just do *that* before the DRAM init, keeping the CPU clock setup still
> after DRAM?

Just checked, this works as well, but is a bit pointless. Just reverting
this and meanwhile checking the DRAM init code seems the easier solution.

Cheers,
Andre

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

[André Przywara]

On 18/12/2018 12:06, Jagan Teki wrote:
> On Tue, Dec 18, 2018 at 4:09 PM <karlp@tweak.net.au> wrote:
>>
>> From: Karl Palsson <karlp@tweak.net.au>
>>
>> This reverts commit a8011eb84dfac5187cebf00ed8bc981bdb5c1fa1
>>
>> This causes DRAM init failures on (at least)
>> * allwinner h3 nanopi-duo2
>> * allwinner h2+ orangepi zero v1.1
>>
>> Signed-off-by: Karl Palsson <karlp@tweak.net.au>
>> ---
>>
>> Ideally, this should get into 2019.01, so that this release is not
>> broken on those targets.
> 
> Better to understand the issue here, since I have BPI-M2+ which boots
> fine w/o this revert.

Could this be a .bss issue? This lies in DRAM and is thus only available
*after* DRAM init. IIRC we silently rely on not accessing anything in
.bss before the DRAM is up, see 59d07ee08e85 for instance.
I don't immediately spot any .bss usage in clock_set_pll1(), though.

Or is the 1GHz CPU clock speed too fast for the DRAM init? If I am not
mistaken, we run with 24MHz before, so there might be some "natural"
delay in some setup routines. Some DRAM chips or board layout might be
more susceptible to this than others, which might explain why it only
fails on some boards.

So if the original patch is about bailing out on error early, can't we
just do *that* before the DRAM init, keeping the CPU clock setup still
after DRAM?

Cheers,
Andre.

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

[Priit Laes]

On Tue, Dec 18, 2018 at 09:17:27PM +0100, Michael Nazzareno Trimarchi wrote:
> Hi Karl
> 
> On Tue, Dec 18, 2018 at 7:12 PM Karl Palsson <karlp@tweak.net.au> wrote:
> >
> >
> > Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > It's 4 days before, "U-Boot] sunxi: regression in dram init for
> > > h3 board"
> > >
> > > 7 days is not 4-days :)
> >
> > True, it was last week, not a week ago.
> >
> > > I would expect some debugging on this during these days and
> > > find the real root cause.
> >
> > Just so there's no unmet expectations here, I'm not doing any
> > _debugging_ of this. (Nor do I have any planned) Given that this
> > is a regression in working devices, I would _presume_ (quite
> > likely wrongly) that the honus of debugging this would be on the
> > people who suggested this patch, not people affected by it. I'm
> > happy to test things, but I have zero experience with dram init,
> > (only marginally greater than zero experience with uboot overall)
> > and no sane methods for debugging this further, nor enough
> > devices to give any test confidence that any debugging I could do
> > would be useful.
> >
> 
> Do you have error after relocation or before relocation? Does it go
> outside and print DRAM size?

When I tested, I replaced hang there with reboot and got following
outputs after multiple FEL uboot uploads commands:

[snip]
U-Boot SPL 2019.01-rc1-00341-gb2f6eec007-dirty (Dec 18 2018 - 09:31:01 +0200)
DRAM: 0 MiB
resetting ...

U-Boot SPL 2019.01-rc1-00341-gb2f6eec007-dirty (Dec 18 2018 - 09:31:01 +0200)
DRAM: 2048 MiB
Trying to boot from FEL

U-Boot SPL 2019.01-rc1-00341-gb2f6eec007-dirty (Dec 18 2018 - 09:31:01 +0200)
DRAM: 1024 MiB
Trying to boot from FEL
[/snip]

> 
> Michael
> 
> > Sincerely,
> > Karl P_______________________________________________
> > 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               |
> 
> -- 
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

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

[Karl Palsson]

Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:

> 
> Do you have error after relocation or before relocation? Does
> it go outside and print DRAM size?

The only output I get is what I posted in
https://lists.denx.de/pipermail/u-boot/2018-December/352036.html
(bad output below)

>>> Bad behaviour:
U-Boot spl 2019.01-rc1-00101-ga8011eb84d (Dec 13 2018 - 16:44:48
+0000) DRAM: 0 MiB
### ERROR ### Please RESET the board ###

If there's debug options you'd like turned on and rebuilt, I can
certainly do that.

Cheers,
Karl P
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.html
Type: application/pgp-signature
Size: 1161 bytes
Desc: OpenPGP Digital Signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181218/0cf4f78e/attachment.sig>

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

[Michael Nazzareno Trimarchi]

Hi Karl

On Tue, Dec 18, 2018 at 7:12 PM Karl Palsson <karlp@tweak.net.au> wrote:
>
>
> Jagan Teki <jagan@amarulasolutions.com> wrote:
> > It's 4 days before, "U-Boot] sunxi: regression in dram init for
> > h3 board"
> >
> > 7 days is not 4-days :)
>
> True, it was last week, not a week ago.
>
> > I would expect some debugging on this during these days and
> > find the real root cause.
>
> Just so there's no unmet expectations here, I'm not doing any
> _debugging_ of this. (Nor do I have any planned) Given that this
> is a regression in working devices, I would _presume_ (quite
> likely wrongly) that the honus of debugging this would be on the
> people who suggested this patch, not people affected by it. I'm
> happy to test things, but I have zero experience with dram init,
> (only marginally greater than zero experience with uboot overall)
> and no sane methods for debugging this further, nor enough
> devices to give any test confidence that any debugging I could do
> would be useful.
>

Do you have error after relocation or before relocation? Does it go
outside and print DRAM size?

Michael

> Sincerely,
> Karl P_______________________________________________
> 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               |

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

[Karl Palsson]

Jagan Teki <jagan@amarulasolutions.com> wrote:
> It's 4 days before, "U-Boot] sunxi: regression in dram init for
> h3 board"
> 
> 7 days is not 4-days :)

True, it was last week, not a week ago.

> I would expect some debugging on this during these days and
> find the real root cause.

Just so there's no unmet expectations here, I'm not doing any
_debugging_ of this. (Nor do I have any planned) Given that this
is a regression in working devices, I would _presume_ (quite
likely wrongly) that the honus of debugging this would be on the
people who suggested this patch, not people affected by it. I'm
happy to test things, but I have zero experience with dram init,
(only marginally greater than zero experience with uboot overall)
and no sane methods for debugging this further, nor enough
devices to give any test confidence that any debugging I could do
would be useful.

Sincerely,
Karl P
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.html
Type: application/pgp-signature
Size: 1161 bytes
Desc: OpenPGP Digital Signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181218/c8300815/attachment-0001.sig>

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

[Jagan Teki]

On Tue, Dec 18, 2018 at 6:02 PM Karl Palsson <karlp@tweak.net.au> wrote:
>
>
> Jagan Teki <jagan@amarulasolutions.com> wrote:
> > On Tue, Dec 18, 2018 at 4:09 PM <karlp@tweak.net.au> wrote:
> > >
> > > From: Karl Palsson <karlp@tweak.net.au>
> > >
> > > This reverts commit a8011eb84dfac5187cebf00ed8bc981bdb5c1fa1
> > >
> > > This causes DRAM init failures on (at least)
> > > * allwinner h3 nanopi-duo2
> > > * allwinner h2+ orangepi zero v1.1
> > >
> > > Signed-off-by: Karl Palsson <karlp@tweak.net.au>
> > > ---
> > >
> > > Ideally, this should get into 2019.01, so that this release is not
> > > broken on those targets.
> >
> > Better to understand the issue here, since I have BPI-M2+ which
> > boots fine w/o this revert.
>
> Fine with me, I posted about it as a "please look into this" a
> week ago, but sending a revert draws more attention it seems :) I

It's 4 days before, "U-Boot] sunxi: regression in dram init for h3 board"

7 days is not 4-days :)

I would expect some debugging on this during these days and find the
real root cause.

> only have a duo2 to test with for h3/h2+ devices, (plaes
> confirmed on the orangepi zero) but it's 100% works/doesn't work
> before/after this commit for me, not a question of reliability or
> "fails sometimes"

BPI-M2+ is H3 board.

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

[Karl Palsson]

Jagan Teki <jagan@amarulasolutions.com> wrote:
> On Tue, Dec 18, 2018 at 4:09 PM <karlp@tweak.net.au> wrote:
> >
> > From: Karl Palsson <karlp@tweak.net.au>
> >
> > This reverts commit a8011eb84dfac5187cebf00ed8bc981bdb5c1fa1
> >
> > This causes DRAM init failures on (at least)
> > * allwinner h3 nanopi-duo2
> > * allwinner h2+ orangepi zero v1.1
> >
> > Signed-off-by: Karl Palsson <karlp@tweak.net.au>
> > ---
> >
> > Ideally, this should get into 2019.01, so that this release is not
> > broken on those targets.
> 
> Better to understand the issue here, since I have BPI-M2+ which
> boots fine w/o this revert.

Fine with me, I posted about it as a "please look into this" a
week ago, but sending a revert draws more attention it seems :) I
only have a duo2 to test with for h3/h2+ devices, (plaes
confirmed on the orangepi zero) but it's 100% works/doesn't work
before/after this commit for me, not a question of reliability or
"fails sometimes"

Cheers,
Karl P
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.html
Type: application/pgp-signature
Size: 1161 bytes
Desc: OpenPGP Digital Signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181218/b8ffa827/attachment-0001.sig>

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

[Jagan Teki]

On Tue, Dec 18, 2018 at 4:09 PM <karlp@tweak.net.au> wrote:
>
> From: Karl Palsson <karlp@tweak.net.au>
>
> This reverts commit a8011eb84dfac5187cebf00ed8bc981bdb5c1fa1
>
> This causes DRAM init failures on (at least)
> * allwinner h3 nanopi-duo2
> * allwinner h2+ orangepi zero v1.1
>
> Signed-off-by: Karl Palsson <karlp@tweak.net.au>
> ---
>
> Ideally, this should get into 2019.01, so that this release is not
> broken on those targets.

Better to understand the issue here, since I have BPI-M2+ which boots
fine w/o this revert.