[PATCH] arm: kirkwood: Enable uart0 dm-pre-reloc for Kirkwood boards
Pali Rohár
pali at kernel.org
Tue Mar 14 22:09:07 CET 2023
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. 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).
More information about the U-Boot
mailing list