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

[Phil Sutter]

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 at 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).

> > > - 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 at 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.

> > [...]
> > > 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.

> > that update_uboot macro at least helps preventing people from writing
> > into wrong spots. What's the cause for dropping it?
> 
> I think we usually use mtdparts to define where u-boot is. So users
> know where the right place to flash is. But you are right, it only
> helps to describe exactly how to flash a new u-boot. So I'll put that
> back in the V2 patch.

Cool, thanks!

> Thanks for taking time to review the patch. And I'm glad to see you
> are still active with the u-boot community!

My DS414 is still alive (and running in production most of the time), so
I benefit from improved u-boot support as well! :)

Cheers, Phil