Revert "sunxi: board: Print error after power initialization fails"

PATCH New Andre Przywara 5 participants 10 messages Dec 19, 2018 – Dec 31, 2018

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm/Allwinner sunXi SoC support

lore

Share & tools

lore

Andre Przywara

author

2018-12-19 13:00 UTC #0 lore raw

PATCH 1/1 Revert "sunxi: board: Print error after power initialization fails"

From: "From: Karl Palsson" <[email protected]>

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 <[email protected]>

Signed-off-by Karl Palsson <[email protected]>

Signed-off-by Andre Przywara <[email protected]>

---
 board/sunxi/board.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

board/sunxi/board.c +8 ~~-10~~

index 917f5b18f6..f022f365e9 100644
@@ -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

Signature · 1 line

2.14.5

Jagan Teki

2018-12-29 18:53 UTC #1 lore raw

PATCH 1/1 Revert "sunxi: board: Print error after power initialization fails"

Quoted 11 lines · Andre Przywara

From: "From: Karl Palsson" <[email protected]>
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.

Quoted 4 lines

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.

Quoted 6 lines

This reverts commit a8011eb84dfac5187cebf00ed8bc981bdb5c1fa1.
Tested-by Priit Laes <[email protected]>
Signed-off-by Karl Palsson <[email protected]>
Signed-off-by Andre Przywara <[email protected]>

Applied to u-boot-sunxi/master

Olliver Schinagl

2018-12-29 22:10 UTC #2 lore raw

PATCH 1/1 Revert "sunxi: board: Print error after power initialization fails"

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?

Quoted 32 lines · Jagan Teki

Quoted 4 lines · Andre Przywara
From: "From: Karl Palsson" <[email protected]>
Commit a8011eb84dfa("sunxi: board: Print error after power
initialization
Quoted 4 lines
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
Quoted 1 line
to 408 MHz) seems to work around the problem, so this points to some
timing
Quoted 3 lines
issues in the DRAM code.
Debugging this sounds like a larger job, so let's just revert this
patch
Quoted 3 lines
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.
Quoted 6 lines
This reverts commit a8011eb84dfac5187cebf00ed8bc981bdb5c1fa1.
Tested-by Priit Laes <[email protected]>
Signed-off-by Karl Palsson <[email protected]>
Signed-off-by Andre Przywara <[email protected]>
Applied to u-boot-sunxi/master

Signature · 1 line

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

Priit Laes

2018-12-30 04:28 UTC #3 lore raw

PATCH 1/1 Revert "sunxi: board: Print error after power initialization fails"

Quoted 3 lines · 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?

It broke on some H2+ devices.

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

Quoted 37 lines · Jagan Teki

Quoted 32 lines
Quoted 4 lines · Andre Przywara
From: "From: Karl Palsson" <[email protected]>
Commit a8011eb84dfa("sunxi: board: Print error after power
initialization
Quoted 4 lines
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
Quoted 1 line
to 408 MHz) seems to work around the problem, so this points to some
timing
Quoted 3 lines
issues in the DRAM code.
Debugging this sounds like a larger job, so let's just revert this
patch
Quoted 3 lines
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.
Quoted 6 lines
This reverts commit a8011eb84dfac5187cebf00ed8bc981bdb5c1fa1.
Tested-by Priit Laes <[email protected]>
Signed-off-by Karl Palsson <[email protected]>
Signed-off-by Andre Przywara <[email protected]>
Applied to u-boot-sunxi/master
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.

André Przywara

author

2018-12-30 23:23 UTC #4 lore raw

PATCH 1/1 Revert "sunxi: board: Print error after power initialization fails"

In reply to Olliver Schinagl

On 29/12/2018 22:10, Olliver Schinagl wrote:

Hi Olliver,

Quoted 2 lines

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

....

Quoted 4 lines

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

Quoted 42 lines

On December 29, 2018 7:53:49 PM GMT+01:00, Jagan Teki <[email protected]> wrote:
On Wed, Dec 19, 2018 at 6:32 PM Andre Przywara <[email protected]> wrote:
From: "From: Karl Palsson" <[email protected]>
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 <[email protected]>
Signed-off-by Karl Palsson <[email protected]>
Signed-off-by Andre Przywara <[email protected]>
Applied to u-boot-sunxi/master
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Olliver Schinagl

2018-12-31 10:34 UTC #5 lore raw

PATCH 1/1 Revert "sunxi: board: Print error after power initialization fails"

Hey André,

Quoted 7 lines · André Przywara

On 29/12/2018 22:10, Olliver Schinagl wrote:
Hi Olliver,
Quoted 2 lines
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 ...

Quoted 6 lines

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)

Quoted 8 lines

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.

Quoted 78 lines

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);
....
Quoted 4 lines
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
Quoted 42 lines
On December 29, 2018 7:53:49 PM GMT+01:00, Jagan Teki <[email protected]> wrote:
On Wed, Dec 19, 2018 at 6:32 PM Andre Przywara <[email protected]> wrote:
From: "From: Karl Palsson" <[email protected]>
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 <[email protected]>
Signed-off-by Karl Palsson <[email protected]>
Signed-off-by Andre Przywara <[email protected]>
Applied to u-boot-sunxi/master
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Michael Trimarchi

2018-12-31 11:27 UTC #6 lore raw

PATCH 1/1 Revert "sunxi: board: Print error after power initialization fails"

Quoted 38 lines · Olliver Schinagl

Hey André,
Quoted 7 lines · André Przywara
On 29/12/2018 22:10, Olliver Schinagl wrote:
Hi Olliver,
Quoted 2 lines
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 ...
Quoted 6 lines
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)
Quoted 8 lines
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 :).

Quoted 23 lines

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.
Quoted 17 lines
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);
....

arch/arm/include/asm/arch-sunxi/clock_sun6i.h +1 -0

index ee387127f3..296f9d11bc 100644
@@ -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)

arch/arm/mach-sunxi/clock_sun6i.c +3 -0

index 1628f3a7b6..10759b7775 100644
@@ -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

Signature · 1 line

2.17.1

Quoted 65 lines

Quoted 10 lines
Quoted 4 lines
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.
Quoted 51 lines
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
Quoted 42 lines
On December 29, 2018 7:53:49 PM GMT+01:00, Jagan Teki <[email protected]> wrote:
On Wed, Dec 19, 2018 at 6:32 PM Andre Przywara <[email protected]> wrote:
From: "From: Karl Palsson" <[email protected]>
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 <[email protected]>
Signed-off-by Karl Palsson <[email protected]>
Signed-off-by Andre Przywara <[email protected]>
Applied to u-boot-sunxi/master
Signature · 1 line
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

Signature · 4 lines

| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

André Przywara

author

2018-12-31 13:10 UTC #7 lore raw

PATCH 1/1 Revert "sunxi: board: Print error after power initialization fails"

Quoted 22 lines · Michael Trimarchi

Hi
Quoted 19 lines · Olliver Schinagl
Hey André,
Quoted 7 lines · André Przywara
On 29/12/2018 22:10, Olliver Schinagl wrote:
Hi Olliver,
Quoted 2 lines
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 ...
Quoted 6 lines
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.

Quoted 3 lines

Quoted 3 lines
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.

Quoted 64 lines

Quoted 37 lines
Quoted 8 lines
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.
Quoted 17 lines
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);
....
arch/arm/include/asm/arch-sunxi/clock_sun6i.h +1 -0
index ee387127f3..296f9d11bc 100644
@@ -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)
arch/arm/mach-sunxi/clock_sun6i.c +3 -0
index 1628f3a7b6..10759b7775 100644
@@ -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
 

Quoted 1 line

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.

Quoted 1 line

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 Trimarchi

2018-12-31 13:38 UTC #8 lore raw

PATCH 1/1 Revert "sunxi: board: Print error after power initialization fails"

Quoted 24 lines · André Przywara

Quoted 22 lines · Michael Trimarchi
Hi
Quoted 19 lines · Olliver Schinagl
Hey André,
Quoted 7 lines · André Przywara
On 29/12/2018 22:10, Olliver Schinagl wrote:
Hi Olliver,
Quoted 2 lines
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 ...
Quoted 6 lines
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.

Quoted 7 lines

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

Quoted 71 lines

Quoted 3 lines
Quoted 3 lines
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.
Quoted 64 lines
Quoted 37 lines
Quoted 8 lines
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.
Quoted 17 lines
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);
....
arch/arm/include/asm/arch-sunxi/clock_sun6i.h +1 -0
index ee387127f3..296f9d11bc 100644
@@ -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)
arch/arm/mach-sunxi/clock_sun6i.c +3 -0
index 1628f3a7b6..10759b7775 100644
@@ -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
 

Quoted 6 lines

Quoted 1 line
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

Quoted 8 lines

Quoted 1 line
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.

Signature · 4 lines

| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Olliver Schinagl

2018-12-31 19:31 UTC #9 lore raw

PATCH 1/1 Revert "sunxi: board: Print error after power initialization fails"

In reply to André Przywara

Hey André,

Quoted 24 lines · André Przywara

Quoted 22 lines · Michael Trimarchi
Hi
Quoted 19 lines · Olliver Schinagl
Hey André,
Quoted 7 lines · André Przywara
On 29/12/2018 22:10, Olliver Schinagl wrote:
Hi Olliver,
Quoted 2 lines
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 ...
Quoted 6 lines
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.

Quoted 6 lines

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

Quoted 87 lines

Quoted 4 lines
Quoted 4 lines
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.
Quoted 64 lines
Quoted 37 lines
Quoted 8 lines
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.
Quoted 17 lines
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);
....
arch/arm/include/asm/arch-sunxi/clock_sun6i.h +1 -0
index ee387127f3..296f9d11bc 100644
@@ -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)
arch/arm/mach-sunxi/clock_sun6i.c +3 -0
index 1628f3a7b6..10759b7775 100644
@@ -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
 
Quoted 1 line
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.
Quoted 1 line
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.