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
<ja...@amarulasolutions.com> wrote:
On Wed, Dec 19, 2018 at 6:32 PM Andre Przywara <andre.przyw...@arm.com>
wrote:
From: "From: Karl Palsson" <ka...@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 <pl...@plaes.org>
Signed-off-by: Karl Palsson <ka...@tweak.net.au>
Signed-off-by: Andre Przywara <andre.przyw...@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@lists.denx.de
https://lists.denx.de/listinfo/u-boot
