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

[Tony Dinh]

Hi Phil,

On Fri, Jun 7, 2024 at 6:45 AM Phil Sutter <phil at nwl.cc> wrote:
>
> Hi Tony,
>
> 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.

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.

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

>
> > - 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?

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.

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

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

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

All the best,
Tony

>
> Thanks, Phil