Re: [PATCH] arm: mvebu: Enable bootstd and other modernization for Synology DS414 (Armada XP) board

[Phil Sutter <phil@nwl.cc>]

Hi,

On Sun, Jun 09, 2024 at 01:51:34PM -0700, Tony Dinh wrote:
> On Sat, Jun 8, 2024 at 4:32 PM Phil Sutter <phil@nwl.cc> wrote:
> > On Fri, Jun 07, 2024 at 04:14:26PM -0700, Tony Dinh wrote:
> > > On Fri, Jun 7, 2024 at 6:45 AM Phil Sutter <phil@nwl.cc> wrote:
> > > > On Thu, Jun 06, 2024 at 04:44:36PM -0700, Tony Dinh wrote:
> > > > > - Switch to standard boot (in include/configs/ds414.h and
> > > > > configs/ds414_defconfig)
> > > > > - Implement board_late_init() to ensure successful enumeration
> > > > > of USB3 devices
> > > >
> > > > Oh, this makes the rear USB3 ports usable? I had failed to get there
> > > > when working on DS414 support.
> > >
> > > Yes! There have been a lot of updates in the PCIe MVEBU area. So it
> > > could have been a different reason why it did not work when you added
> > > support for this board.
> >
> > Nice!
> >
> > > Still, without this board_late_init(), we would need to run "pci enum"
> > > and then "usb start" or "usb reset" to bring up the USB3 ports. The
> > > USB3 controller is on the PCIe bus, and MVEBU PCIe drivers don't
> > > initialize everything when it comes up.
> >
> > Yeah, at some point I still had
> > | CONFIG_PREBOOT="usb start; sf probe"
> > in ds414_defconfig. Looking at my initial DS414 support patch from 2015,
> > at least 'usb start' seemed to be necessary for USB to work in Linux.
> > This might have changed later as I dropped the config symbol six years
> > later in e471ddf0f3472 ("arm: mvebu: board/Synology: Unify legacy kernel
> > support").
> >
> > > > > - Remove unnecessary misc_init_r(), since NET_RANDOM_ETHADDR is now
> > > > > configured in
> > > >
> > > > So u-boot will assign random MAC addresses to the NICs instead of the
> > > > real ones? Won't this potentially break PXE boot setups?
> > >
> > > I did not see any problem using a random MAC address in my test using
> > > bootstd. Is there a setup where this board MAC address must be known
> > > when we boot with PXE? my impression is that only ipaddr and serverip
> > > are needed, and the MAC address could be whatever we define for the
> > > board.
> >
> > I haven't played with PXE for a long time, but I assumed the DHCP daemon
> > might send different options to different hosts identified by their MAC
> > address. The latter being random obviously defeats such setups.
> >
> > Does the random MAC persist in Linux, BTW? I don't recall Linux
> > extracting data from the vendor-specific flash storage. Unless I'm
> > mistaken, it should be u-boot's responsibility to setup MAC addresses in
> > the NICs (to the vendor-assigned ones).
> 
> Yes, it persists. Whatever was assigned to ethaddr and eth1addr are
> passed to the Linux kernel. So the random local MAC address is
> recognized as the hardware MAC address.

Thanks for confirming. I'll try board_eth_init() in one of the next
evenings - my DS414 needs a revamp anyway, the rootfs on that bloody
USB stick I boot from keeps disintegrating and the box is FUBAR ATM.

> As an aside, there is a bug in bootstd that "bootflow scan -l" will go
> into a seemingly infinite loop during PXE hunting if eth1addr is not
> defined. I've not figured out how to fix this error recovery. I might
> just give up and send the bug report to Simon :)

Fixing the bug as well as making sure eth1addr is properly set should
not hurt. :)

> > > > > - Remove unnecessary checkboard()
> > > > > - Updated IDENT_STRING to indicate this u-boot supports both Synology
> > > > > DS414 and DS214+ boards
> > > > > - Add SYS_THUMB_BUILD to reduce binary size
> > > > > - Add NET_RANDOM_ETHADDR
> > > > > - Add CONFIG_LBA48 and CONFIG_SYS_64BIT_LBA to support >2TB HDD/SDD
> > > > >
> > > > > Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> > > > > ---
> > > > >
> > > > >  board/Synology/ds414/ds414.c | 15 +++--------
> > > > >  configs/ds414_defconfig      | 20 ++++++--------
> > > > >  include/configs/ds414.h      | 51 ++++++++++++++++++++++--------------
> > > > >  3 files changed, 42 insertions(+), 44 deletions(-)
> > > > >
> > > > > diff --git a/board/Synology/ds414/ds414.c b/board/Synology/ds414/ds414.c
> > > > > index abe6f9eb5e..f0b55fa095 100644
> > > > > --- a/board/Synology/ds414/ds414.c
> > > > > +++ b/board/Synology/ds414/ds414.c
> > > > > @@ -181,18 +181,9 @@ int board_init(void)
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > -int misc_init_r(void)
> > > > > +int board_late_init(void)
> > > > >  {
> > > > > -     if (!env_get("ethaddr")) {
> > > > > -             puts("Incomplete environment, populating from SPI flash\n");
> > > > > -             do_syno_populate(0, NULL);
> > > > > -     }
> > > > > -     return 0;
> > > > > -}
> > > >
> > > > This is useful for first boot of flashed vanilla u-boot. Can this be
> > > > retained somehow?
> > >
> > > We could, but with NET_RANDOM_ETHADDR configured in, this routine will
> > > do nothing. Perhaps the fact that the envs will show CRC errors in the
> > > 1st boot should be an indicator that it needs to be populated?
> >
> > Maybe this env population approach is not the right one to begin with.
> > Reading doc/README.enetaddr, it seems we want something like step 1 in
> > "Correct flow of setting up the MAC address", though I'm not sure which
> > initialize() function this is about (board_eth_init() maybe?). Also we
> > need a working spi flash driver at that point.
> >
> > > With standard boot enabled in this patch, the default envs can be
> > > scripted in /boot/boot.scr or /boot.scr on the rootfs disk storage, or
> > > on some flash location, or on the network . So we don't even need the
> > > full envs on SPI flash like we used to. It might still be useful but
> > > not necessary.
> >
> > Maybe one could implement a translator which presents the 'vendor'
> > partition contents as such default boot env.
> 
> Are you still using the stock Linux kernel at all?

No, but I still have it on flash. When working on vanilla u-boot, being
able to boot into stock OS and having it appear to work fine was a
criteria for me. Also it is nice if other users are able to replace the
crappy Synology u-boot without being forced to also boot a custom OS.

> > > > [...]
> > > > > diff --git a/include/configs/ds414.h b/include/configs/ds414.h
> > > > > index 9446acba79..bbefe632e0 100644
> > > > > --- a/include/configs/ds414.h
> > > > > +++ b/include/configs/ds414.h
> > > > [...]
> > > > > @@ -38,23 +34,38 @@
> > > > >   * L2 cache thus cannot be used.
> > > > >   */
> > > > >
> > > > > -/* SPL */
> > > > > -/* Defines for SPL */
> > > > > +/* Keep device tree and initrd in lower memory so the kernel can access them */
> > > > > +#define RELOCATION_LIMITS_ENV_SETTINGS  \
> > > > > +     "fdt_high=0x10000000\0"         \
> > > > > +     "initrd_high=0x10000000\0"
> > > > > +
> > > > > +/*
> > > > > + * mv-common.h should be defined after CMD configs since it used them
> > > > > + * to enable certain macros
> > > > > + */
> > > > > +#include "mv-common.h"
> > > > > +
> > > > > +#ifndef CONFIG_SPL_BUILD
> > > > >
> > > > > -/* Default Environment */
> > > > > +#define KERNEL_ADDR_R        __stringify(0x1000000)
> > > > > +#define FDT_ADDR_R   __stringify(0x2000000)
> > > > > +#define RAMDISK_ADDR_R       __stringify(0x2200000)
> > > > > +#define SCRIPT_ADDR_R        __stringify(0x1800000)
> > > > > +#define PXEFILE_ADDR_R       __stringify(0x1900000)
> > > > >
> > > > > -#define CFG_EXTRA_ENV_SETTINGS                               \
> > > > > -     "initrd_high=0xffffffff\0"                              \
> > > > > -     "ramdisk_addr_r=0x8000000\0"                            \
> > > > > -     "usb0Mode=host\0usb1Mode=host\0usb2Mode=device\0"       \
> > > > > -     "ethmtu=1500\0eth1mtu=1500\0"                           \
> > > > > -     "update_uboot=sf probe; dhcp; "                         \
> > > > > -             "mw.b ${loadaddr} 0x0 0xd0000; "                \
> > > > > -             "tftpboot ${loadaddr} u-boot-with-spl.kwb; "    \
> > > > > -             "sf update ${loadaddr} 0x0 0xd0000\0"
> > > >
> > > > Are these dropped or am I missing a generic spot which adds them? IIRC,
> > > > the usb*mode and eth*mtu settings are required, is that wrong? Lastly,
> > >
> > > MTU is default to 1500 in mvneta.c. So the eth*mtu env is no longer necessary.
> > >
> > > Also, I believe usb*mode is no longer necessary with the USB driver
> > > model. I have not come across their usage anywhere for a long time.
> > > The USB driver model seems to handle everything pretty well.
> >
> > I think both are required for stock Synology Linux, see
> > setup_board_tags() in board/Synology/common/legacy.c. One could just
> > hard-code sane fallback values into that routine though.
> 
> I see. Now I realized it's my bad that I did not consider stock OS at
> all! The purpose of this patch is to get u-boot to use bootstd to boot
> modern distros using a distro boot script. Should I put back these
> envs "usb0Mode=host\0usb1Mode=host\0usb2Mode=device\0"
> "ethmtu=1500\0eth1mtu=1500\0"  so we can support stock FW?

Yes, please. It's the simplest way to retain that functionality. If
CONFIG_NET_RANDOM_ETHADDR populates eth*addr env vars, we may even keep
that (and fetching "the real" addresses becomes merely a bonus).

Cheers, Phil