[PATCH] arm: kirkwood: Enable uart0 dm-pre-reloc for Kirkwood boards
Tony Dinh
mibodhi at gmail.com
Tue Mar 14 22:30:02 CET 2023
Hi Pali,
On Tue, Mar 14, 2023 at 2:09 PM Pali Rohár <pali at kernel.org> wrote:
>
> On Tuesday 14 March 2023 13:59:21 Tony Dinh wrote:
> > Hi all,
> >
> > I'm closing the loop on this thread. Please see below.
> >
> > On Fri, Feb 10, 2023 at 6:29 PM Tony Dinh <mibodhi at gmail.com> wrote:
> > >
> > > Hi Pali,
> > >
> > > On Fri, Feb 10, 2023 at 1:44 PM Pali Rohár <pali at kernel.org> wrote:
> > > >
> > > > On Friday 10 February 2023 13:38:44 Tony Dinh wrote:
> > > > > Hi Stefan,
> > > > > Hi Pali,
> > > > >
> > > > > On Fri, Feb 10, 2023 at 9:29 AM Pali Rohár <pali at kernel.org> wrote:
> > > > > >
> > > > > > On Thursday 09 February 2023 17:37:54 Tony Dinh wrote:
> > > > > > > Hi Pali,
> > > > > > >
> > > > > > > On Thu, Feb 9, 2023 at 1:45 PM Pali Rohár <pali at kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Thursday 09 February 2023 13:33:23 Tony Dinh wrote:
> > > > > > > > > Hi Stefan,
> > > > > > > > >
> > > > > > > > > On Wed, Feb 8, 2023 at 11:15 PM Stefan Roese <sr at denx.de> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Tony,
> > > > > > > > > >
> > > > > > > > > > On 2/9/23 04:17, Tony Dinh wrote:
> > > > > > > > > > > Hi Stefan,
> > > > > > > > > > >
> > > > > > > > > > > On Fri, Feb 3, 2023 at 4:11 PM Tony Dinh <mibodhi at gmail.com> wrote:
> > > > > > > > > > >>
> > > > > > > > > > >> Hi Pali,
> > > > > > > > > > >>
> > > > > > > > > > >> On Fri, Feb 3, 2023 at 2:05 PM Pali Rohár <pali at kernel.org> wrote:
> > > > > > > > > > >>>
> > > > > > > > > > >>> On Thursday 02 February 2023 19:04:45 Pali Rohár wrote:
> > > > > > > > > > >>>> On Wednesday 01 February 2023 13:13:16 Tony Dinh wrote:
> > > > > > > > > > >>>>> Hi all,
> > > > > > > > > > >>>>>
> > > > > > > > > > >>>>> On Wed, Feb 1, 2023 at 11:05 AM Pali Rohár <pali at kernel.org> wrote:
> > > > > > > > > > >>>>>>
> > > > > > > > > > >>>>>> On Wednesday 01 February 2023 09:17:15 Michael Walle wrote:
> > > > > > > > > > >>>>>>>>>> When DM_SERIAL is enabled, the device-tree property dm-pre-reloc is
> > > > > > > > > > >>>>>>>>>> required to boot over UART with kwboot. Enable this in a Kirkwood
> > > > > > > > > > >>>>>>>>>> common u-boot dtsi.
> > > > > > > > > > >>>>>>>>>
> > > > > > > > > > >>>>>>>>> My (dev) board unfortunately, have a bootloader which can't boot over
> > > > > > > > > > >>>>>>>>> serial.
> > > > > > > > > > >>>>>>>>
> > > > > > > > > > >>>>>>>> This is feature of Marvell BootROM and does not require any special from
> > > > > > > > > > >>>>>>>> Bootloader. So you should be able to boot over UART (if you have
> > > > > > > > > > >>>>>>>> accessible pins).
> > > > > > > > > > >>>>>>>
> > > > > > > > > > >>>>>>> I know, but there are known versions ob the bootrom where uart boot
> > > > > > > > > > >>>>>>> isn't supported (correctly).
> > > > > > > > > > >>>>>>
> > > > > > > > > > >>>>>> I heard about it... maybe it is a bug in client software (kwboot)? I do
> > > > > > > > > > >>>>>> not have such board if you are interested in it I could try to send some
> > > > > > > > > > >>>>>> details how to debug it.
> > > > > > > > > > >>>>>
> > > > > > > > > > >>>>> The Kirkwood SoCs came with different BootROM versions. Version 1.1
> > > > > > > > > > >>>>> cannot be booted over UART, but version 1.2 can. I think there must
> > > > > > > > > > >>>>> be a bug in the BootROM 1.1. The older Kirkwood such as Sheevaplug,
> > > > > > > > > > >>>>> Dockstar, iConnect boards come with BootROM 1.1. Later version of
> > > > > > > > > > >>>>> Sheevaplug, GoFlex Home, GoFlex Net, Dreamplug, Pogoplug V4, Zyxel
> > > > > > > > > > >>>>> NSA310S, NSA320, NSA325 come with BootROM 1.2. So even though it is
> > > > > > > > > > >>>>> the same SoC, eg. 6281, they are actually produced at a different time
> > > > > > > > > > >>>>> and have different BootROM versions.
> > > > > > > > > > >>>>
> > > > > > > > > > >>>> There are always multiple revisions of the same SoC. So it is possible
> > > > > > > > > > >>>> that something was broken on first revision of 88F6281 and in next
> > > > > > > > > > >>>> revision was updated BootROM with some fixes. Revision is written on
> > > > > > > > > > >>>> package label and for Armada SoCs it is available also in some register
> > > > > > > > > > >>>> (not sure about Kirkwood). It looks like that there is at least revision
> > > > > > > > > > >>>> Z0 and revision A0 of some Kirkwood SoC.
> > > > > > > > > > >>>>
> > > > > > > > > > >>>> If there is a bug in first revisions then it should be documented in
> > > > > > > > > > >>>> some Kirkwood Errata document. Unfortunately I have never seen it, it is
> > > > > > > > > > >>>> not public, so I have no idea. In any case, if somebody has access to
> > > > > > > > > > >>>> Marvell documents, interesting are these document numbers:
> > > > > > > > > > >>>>
> > > > > > > > > > >>>> * MV-S105223-001 - Differences Between the 88F6192, and 88F6281 Stepping Z0 and A0
> > > > > > > > > > >>>> * MV-S501081-00 - 88F6180, 88F6192, and 88F6281 Functional Errata, Interface Guidelines, and Restrictions
> > > > > > > > > > >>>> * MV-S501157-U0 - 88F6180, 88F6190, 88F6192, and 88F6281 Functional Errata, Interface Guidelines, and Restrictions
> > > > > > > > > > >>>>
> > > > > > > > > > >>>> One of the option how to investigate or debug this issue without
> > > > > > > > > > >>>> documentation is to dump both BootROM versions (1.1, 1.2) and compare
> > > > > > > > > > >>>> them. Either there is different UART protocol for booting (which needs
> > > > > > > > > > >>>> to be implemented) or UART protocol is buggy and needs some workaround
> > > > > > > > > > >>>> or it is completely broken and does not work.
> > > > > > > > > > >>>
> > > > > > > > > > >>> BootROM is mapped to 0xF8000000 address and is 64 kB long. So via U-Boot
> > > > > > > > > > >>> md command it is possible to dump it over console. Or via ext4write
> > > > > > > > > > >>> command store it so ext4 fs on sd card / sata disk.
> > > > > > > > > > >>
> > > > > > > > > > >> Thanks for the info. It will be a while before I can get back to this
> > > > > > > > > > >> BootROM 1.1 vs 1.2 topic.
> > > > > > > > > > >>
> > > > > > > > > > >
> > > > > > > > > > > I have run some more tests, and am quite positive that if the uart0
> > > > > > > > > > > node exists in the DTS, it must be tagged with dm,pre-reloc. That is
> > > > > > > > > > > consistent with the serial documentation in
> > > > > > > > > > > doc/develop/driver-model/serial-howto.rst. The issue is whether the
> > > > > > > > > > > uart0 node is specified in the DTS. If it is, the dm-pre-reloc tag
> > > > > > > > > > > must also be used.
> > > > > > > > > > >
> > > > > > > > > > > To prove my theory, I've modified the Pogo V4 DTS to look exactly like
> > > > > > > > > > > the NSA310S, as far as uart0 is concerned. The NSA310S does not even
> > > > > > > > > > > have a uart0 node! and DM_SERIAL boots fine with this box. With the
> > > > > > > > > > > patch below, the Pogo V4 can be kwboot and run normally. No
> > > > > > > > > > > dm-pre-reloc tag is needed.
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/arch/arm/dts/kirkwood-pogoplug-series-4.dts
> > > > > > > > > > > b/arch/arm/dts/kirkwood-pogoplug-series-4.dts
> > > > > > > > > > > index 5aa4669ae2..227d5ff802 100644
> > > > > > > > > > > --- a/arch/arm/dts/kirkwood-pogoplug-series-4.dts
> > > > > > > > > > > +++ b/arch/arm/dts/kirkwood-pogoplug-series-4.dts
> > > > > > > > > > > @@ -24,7 +24,8 @@
> > > > > > > > > > > };
> > > > > > > > > > >
> > > > > > > > > > > chosen {
> > > > > > > > > > > - stdout-path = "uart0:115200n8";
> > > > > > > > > > > + bootargs = "console=ttyS0,115200";
> > > > > > > > > > > + stdout-path = &uart0;
> > > > > > > > > > > };
> > > > > > > > > > >
> > > > > > > > > > > gpio_keys {
> > > > > > > > > > > @@ -97,10 +98,6 @@
> > > > > > > > > > > };
> > > > > > > > > > > };
> > > > > > > > > > >
> > > > > > > > > > > -&uart0 {
> > > > > > > > > > > - status = "okay";
> > > > > > > > > > > -};
> > > > > > > > > > > -
> > > > > > > > > > > /*
> > > > > > > > > > > * This PCIE controller has a USB 3.0 XHCI controller at 1,0
> > > > > > > > > > > */
> > > > > > > > > >
> > > > > > > > > > FWIU, you are hitting a bug in the early serial console code, perhaps
> > > > > > > > > > in serial_check_stdout()?
> > > > > > > > >
> > > > > > > > > Quite possible. But I recall trying the same stdout path as the
> > > > > > > > > NSA310S, i.e. stdout-path = &uart0. So the stdout-path itself is not
> > > > > > > > > the reason. Perhaps the bug is in another part of the function.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Could you please check if Pogo V4 boots with
> > > > > > > > > > CONFIG_REQUIRE_SERIAL_CONSOLE unset?
> > > > > > > > > >
> > > > > > > > > Indeed, you're right! unset CONFIG_REQUIRE_SERIAL_CONSOLE allows the
> > > > > > > > > Pogo V4 to boot OK, without dm-pre-reloc needed. And it booted
> > > > > > > > > normally with kwboot, all serial input/outputs are OK.
> > > > > > > > >
> > > > > > > > > > > Also, note that just enabling DEBUG_UART would always make the board
> > > > > > > > > > > frozen when u-boot starts.
> > > > > > > > > >
> > > > > > > > > > This should not be the case of course. Are you sure you've correctly
> > > > > > > > > > configured the DEBUG UART?
> > > > > > > > >
> > > > > > > > > I think I did that correctly (we've used that to debug the orion_timer
> > > > > > > > > issue previously). Here is how I've configured it:
> > > > > > > > >
> > > > > > > > > +CONFIG_DEBUG_UART_BASE=0xf1012000
> > > > > > > > > +CONFIG_DEBUG_UART_CLOCK=250000000
> > > > > > > > > +CONFIG_DEBUG_UART=y
> > > > > > > >
> > > > > > > > Maybe you hit same issue in mach-kirkwood as me in the past for
> > > > > > > > mach-mvebu? U-Boot code was trying to initialize UART at the _new_ base
> > > > > > > > register address (0xf1012000) when U-Boot was still using the _old_ base
> > > > > > > > register address (0xd0012000).
> > > > > > > >
> > > > > > > > I fixed it by moving code which changes base register address from
> > > > > > > > arch_cpu_init() function to arch_very_early_init() function in commit:
> > > > > > > > https://source.denx.de/u-boot/u-boot/-/commit/5bb2c550b11eb087437740b2a0d1fe780be5aec3
> > > > > > >
> > > > > > > Thanks for the info! Looking at that patch, I think I'm out of my
> > > > > > > depth :)
> > > > > >
> > > > > > That is pretty simple. It is something like this:
> > > > > >
> > > > > > diff --git a/arch/arm/mach-kirkwood/Kconfig b/arch/arm/mach-kirkwood/Kconfig
> > > > > > index c8a193dd4cdf..ba39e9ae416e 100644
> > > > > > --- a/arch/arm/mach-kirkwood/Kconfig
> > > > > > +++ b/arch/arm/mach-kirkwood/Kconfig
> > > > > > @@ -5,9 +5,11 @@ config FEROCEON_88FR131
> > > > > >
> > > > > > config KW88F6192
> > > > > > bool
> > > > > > + select ARCH_VERY_EARLY_INIT
> > > > > >
> > > > > > config KW88F6281
> > > > > > bool
> > > > > > + select ARCH_VERY_EARLY_INIT
> > > > > >
> > > > > > config SHEEVA_88SV131
> > > > > > bool
> > > > > > diff --git a/arch/arm/mach-kirkwood/Makefile b/arch/arm/mach-kirkwood/Makefile
> > > > > > index 3b2eef8d5419..0fb5a2326f5f 100644
> > > > > > --- a/arch/arm/mach-kirkwood/Makefile
> > > > > > +++ b/arch/arm/mach-kirkwood/Makefile
> > > > > > @@ -6,6 +6,7 @@
> > > > > >
> > > > > > obj-y = cpu.o
> > > > > > obj-y += cache.o
> > > > > > +obj-y += lowlevel.o
> > > > > > obj-y += mpp.o
> > > > > >
> > > > > > # cpu.o and cache.o contain CP15 instructions which cannot be run in
> > > > > > diff --git a/arch/arm/mach-kirkwood/cpu.c b/arch/arm/mach-kirkwood/cpu.c
> > > > > > index df3e8f11782a..2b493b36c20d 100644
> > > > > > --- a/arch/arm/mach-kirkwood/cpu.c
> > > > > > +++ b/arch/arm/mach-kirkwood/cpu.c
> > > > > > @@ -189,9 +189,6 @@ int arch_cpu_init(void)
> > > > > > struct kwcpu_registers *cpureg =
> > > > > > (struct kwcpu_registers *)KW_CPU_REG_BASE;
> > > > > >
> > > > > > - /* Linux expects the internal registers to be at 0xf1000000 */
> > > > > > - writel(KW_REGS_PHY_BASE, KW_OFFSET_REG);
> > > > > > -
> > > > > > /* Enable and invalidate L2 cache in write through mode */
> > > > > > writel(readl(&cpureg->l2_cfg) | 0x18, &cpureg->l2_cfg);
> > > > > > invalidate_l2_cache();
> > > > > > diff --git a/arch/arm/mach-kirkwood/lowlevel.S b/arch/arm/mach-kirkwood/lowlevel.S
> > > > > > index e69de29bb2d1..3b339f97f056 100644
> > > > > > --- a/arch/arm/mach-kirkwood/lowlevel.S
> > > > > > +++ b/arch/arm/mach-kirkwood/lowlevel.S
> > > > > > @@ -0,0 +1,12 @@
> > > > > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > > > > +
> > > > > > +#include <config.h>
> > > > > > +#include <linux/linkage.h>
> > > > > > +
> > > > > > +ENTRY(arch_very_early_init)
> > > > > > + /* Move internal registers from KW_OFFSET_REG to KW_REGS_PHY_BASE */
> > > > > > + ldr r0, =KW_REGS_PHY_BASE
> > > > > > + ldr r1, =KW_OFFSET_REG
> > > > > > + str r0, [r1]
> > > > > > + bx lr
> > > > > > +ENDPROC(arch_very_early_init)
> > > > >
> > > > > Thanks! I'll try this eventually. But please see my response to Stefan
> > > > > below. Would the forced setting CONFIG_DEBUG_UART_BASE=0xd0012000 give
> > > > > us some output from uart0 before arch_early_init ?
> > > >
> > > > I do not know if it gives us any output. It depends on when u-boot start
> > > > printing something on UART. Beware that initialization of UART can be
> > > > more earlier than printing something. Note that after arch_cpu_init() is
> > > > 0xd0012000 invalid. So if initialization happens before arch_cpu_init()
> > > > but printing starts after arch_cpu_init() then nothing would be visible
> > > > and it will always fail.
> > > >
> > > > > > > For now I will fix this Pogo V4 with the -u-boot.dtsi, so as
> > > > > > > not to mess up anybody who might try UART booting with kwboot.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Tony
> > > > > > >
> > > > > > > > > > > So this was another factor why it took me
> > > > > > > > > > > so long to figure this out :) I've just restored everything and
> > > > > > > > > > > removed the uart0 node from the Pogo V4 DTS like above and it is
> > > > > > > > > > > working.
> > > > > > > > > > >
> > > > > > > > > > > But of course, we don't want to change the Pogo V4 DTS, so I will
> > > > > > > > > > > submit another patch just for the Pogo V4 to add -u-boot.dtsi for this
> > > > > > > > > > > box to enable the dm-pre-reloc tag for uart0.
> > > > > > > > > >
> > > > > > > > > > Understood.
> > > > >
> > > > > Regarding Stefan's suggestion about trying using
> > > > > CONFIG_DEBUG_UART_BASE=0xd0012000. Yes, I also tried that address (I'm
> > > > > aware of this address while building kernels for some old Armada 370
> > > > > boards such as the Mirabox and Netgear RN102). I suspect for some
> > > > > reason, i.e. bug, at the pre-relocation phase we don't even have the
> > > > > uart0 device so DEBUG UART printch() just hangs.
> > > > >
> > > > > Thanks,
> > > > > Tony
> > > >
> > > > DEBUG UART does not use DM / DT devices like uart0. DEBUG UART does not
> > > > use DTS file. So even with broken DTS file DEBUG UART should work fine.
> > >
> > > Sorry, I meant the serial device that DEBUG UART uses, not the
> > > specific uart0! I think that device does not exist at that point.
> > >
> > > Anyway, I tried your patch for ARCH_VERY_EARLY_INIT, and the behavior
> > > is the same. Subsequently, with this patch I also tried taking out all
> > > printch() calls (to make sure nothing is output). The board is just
> > > frozen right when u-boot starts.
> > >
> > > I think I'll let this problem stew for a while, and come back later. I
> > > will turn off DM_SERIAL and run some tests then.
> > >
> > > In the meantime, if anybody has any Marvell SoC defconfig in which
> > > DEBUG_UART was working with DM_SERIAL, I'd appreciate it if you can
> > > send that defconfig.
> >
> > OK so finally Debug UART is working on this board. Thanks to Pali's
> > "[PATCH RFC u-boot-mvebu] arm: kirkwood: Move internal registers in
> > arch_very_early_init() function" here:
> > https://lists.denx.de/pipermail/u-boot/2023-March/511973.html
> >
> > The reason that among the Kirkwood boards, only this board seems to be
> > frozen upon starting was this sequence:
> >
> > 1. The stdout-path = "uart0:115200n8" was not recognized as a valid
> > path by serial_check_stdout() (in drivers/serial/serial-uclass.c). So
> > it failed to initialize the serial uclass, and invoke panic_str(). We
> > knew that was what happened, but it should have reset the board when
> > panic was called.
> >
> > 2. The panic_finish() and do_reset() functions did not work correctly
> > in this scenario, because it is too early to have a timer running. So
> > u-boot went into a seemingly infinite loop, because the timer getcount
> > failed at each increment.
> >
> > https://github.com/u-boot/u-boot/blob/master/lib/panic.c#L27
> > https://github.com/u-boot/u-boot/blob/master/arch/arm/lib/reset.c#L37
> >
> > IMO, it is unnecessary to sleep to flush the text outputs "No serial
> > driver found" and "resetting ...". When Debug UART is enabled, the
> > text always comes out OK. It is just a precaution, which we might want
> > to do when there is a lot going on after relocation. But it seems a
> > bit overkill and not working in the early phase.
> >
> > I'd suggest that we either remove these sleep periods, or find some
> > way to enable it only after relocation.
> >
> > Thanks,
> > Tony
>
> Decision is not "after relocation" but rather "full-feature UART driver
> is in use". Moreover there is already new function named flush() which
> does "wait until stdout message was sent" and can be used instead of
> those sleeps.
That's great!
> I have already did it on some places (see git history for
> flush function) but seems that you find some more. And flush() should be
> compatible also with early UART (and if not then it can be fixed).
flush() is a much better implementation, even if the timer is working.
I'll try that.
Thanks,
Tony
More information about the U-Boot
mailing list