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

[Tony Dinh]

Hi Phil,

On Fri, Jun 28, 2024 at 3:04 PM Tony Dinh <mibodhi at gmail.com> wrote:
>
> Hi Phil,
>
> On Wed, Jun 26, 2024 at 3:31 AM Phil Sutter <phil at nwl.cc> wrote:
> >
> > Hi Tony,
> >
> > On Sat, Jun 15, 2024 at 03:06:54PM -0700, Tony Dinh wrote:
> > [...]
> > > 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;
> > > -}
> >
> > Could you please leave misc_init_r() in place (along with MISC_INIT_R in
> > defconfig)? A closer look at doc/README.enetaddr suggests that
> > implementing this board-specific function which acts if the environment
> > does not have ethaddr defined already is exactly the right thing to do.
> > Also, it doesn't conflict with NET_RANDOM_ETHADDR: If do_syno_populate()
> > succeeds, no random MAC addresses are generated. If I manually remove
> > the call, random MACs come into play. So having this option enabled
> > serves as a fallback for boxes which lack the data in flash.
>
> Sure I will do that. I was mistaken in assuming that network random
> addresses were already generated before misc_init_r(). Thanks!
>
> >
> > > -int checkboard(void)
> > > -{
> > > -     puts("Board: DS414\n");
> > > -
> > > +     /* Do late init to ensure successful enumeration of XHCI devices */
> > > +     pci_init();
> > >       return 0;
> > >  }
> >
> > FWIW, booting from rear USB port worked fine for me!
>
> Cool! That was the main thing I was interested in for sending in this
> patch. One thing led to another... :)
>
> >
> > > diff --git a/configs/ds414_defconfig b/configs/ds414_defconfig
> > > index ecf9501ba5..501ed51129 100644
> > > --- a/configs/ds414_defconfig
> > > +++ b/configs/ds414_defconfig
> > [...]
> > > @@ -25,44 +26,40 @@ CONFIG_SPL_BSS_MAX_SIZE=0x4000
> > >  CONFIG_SPL=y
> > >  CONFIG_DEBUG_UART_BASE=0xf1012000
> > >  CONFIG_DEBUG_UART_CLOCK=250000000
> > > +CONFIG_IDENT_STRING="\nSynology DS214+/DS414 2/4-Bay Diskstation"
> > >  CONFIG_SYS_LOAD_ADDR=0x800000
> > >  CONFIG_PCI=y
> > >  CONFIG_DEBUG_UART=y
> > > +CONFIG_BOOTSTD_FULL=y
> > >  CONFIG_BOOTDELAY=3
> > >  CONFIG_USE_BOOTARGS=y
> > > -CONFIG_BOOTARGS="console=ttyS0,115200 ip=off initrd=0x8000040,8M root=/dev/md0 rw syno_hw_version=DS414r1 ihd_num=4 netif_num=2 flash_size=8 SataLedSpecial=1 HddHotplug=1"
> > > -CONFIG_USE_BOOTCOMMAND=y
> > > -CONFIG_BOOTCOMMAND="sf probe; sf read ${loadaddr} 0xd0000 0x2d0000; sf read ${ramdisk_addr_r} 0x3a0000 0x430000; bootm ${loadaddr} ${ramdisk_addr_r}"
> >
> > So these should not be necessary anymore with BOOTSTD. I wonder if it is
> > possible to provide a static bootmeth (or bootflow?) with low priority
> > which boots the legacy OS from flash. It could hold all the details
> > instead of the *_legacy env vars introduced below.
>
> I recall from bootstd documentation that it is possible to have a
> bootflow on flash. But it seems a bit too much for this patch. I think
> we'll have to repartition or find some space on the flash to store
> that bootflow script.
>

Sorry I've misread your comment "it is possible to provide a static
bootmeth (or bootflow?) ". I think by "static bootflow" you must have
meant we would define it in the board code, and convince bootstd to
use it as the last boot method (without hunting for it on flash). I'm
not sure. Perhaps "global bootflow" is something that can be defined
and used here. I'll have to dig into bootstd documentation a bit more.

All the best,
Tony

> >
> > A pending issue for me is inability to 'saveenv' - the flash seems
> > read-only in that spot. Does it work for you?
>
> Yes it does work for me. With the latest u-boot SPI flash driver, as I
> recall, the entire flash is automatically unprotected at
> initialization. But we need to be careful using saveenv running stock
> FW, because it could overwrite the rootfs on flash. It is common for
> Synology NAS that the envs location is at the same place as the stock
> rootfs. Since I don't care about stock FW, I saved the envs and used
> the location for u-boot envs in Linux /etc/fw_env.config. I'm not sure
> if my DS214+ box can boot stock FW anymore :)
>
> >
> > >  # CONFIG_DISPLAY_BOARDINFO is not set
> > >  CONFIG_DISPLAY_BOARDINFO_LATE=y
> > > -CONFIG_MISC_INIT_R=y
> > > +CONFIG_BOARD_LATE_INIT=y
> > >  CONFIG_SPL_MAX_SIZE=0x1bfd0
> > >  CONFIG_SPL_SYS_MALLOC_SIMPLE=y
> > >  # CONFIG_SPL_SHARES_INIT_SP_ADDR is not set
> > >  CONFIG_SPL_I2C=y
> > > +CONFIG_SYS_PROMPT="DS414> "
> >
> > Can we detect whether it is 414 or 214, BTW? bootargs_legacy contains
> > 414-specific info as well.
>
> It's a bit hard to do without creating another defconfig. AFAICT, the
> only difference between DS414 and DS214+ is the number of SATA bays.
> But currently u-boot SATA does not work for this box. U-Boot sata_mv
> driver is a subset of Linux sata_mv, and we don't have support for the
> right PCIe SATA chip in u-boot sata_mv.
>
> >
> > [...]
> > > diff --git a/include/configs/ds414.h b/include/configs/ds414.h
> > > index 9446acba79..89e55bf0d4 100644
> > > --- a/include/configs/ds414.h
> > > +++ b/include/configs/ds414.h
> > [...]
> > > @@ -16,14 +17,9 @@
> > >   * U-Boot into it.
> > >   */
> > >
> > > -/* I2C */
> > >  #define CFG_I2C_MVTWSI_BASE0         MVEBU_TWSI_BASE
> > >
> > > -/*
> > > - * mv-common.h should be defined after CMD configs since it used them
> > > - * to enable certain macros
> > > - */
> > > -#include "mv-common.h"
> > > +#define PHY_ANEG_TIMEOUT     16000   /* PHY needs a longer aneg time */
> >
> > AIUI, this is a limitation of my local NIC, not the DS414 one. It just
> > took too long for link detection and autoneg when directly connected. A
> > switch should have helped. Or do you have more details?
>
> Ah! The typical PHY_ANEG_TIMEOUT on other Armada 38x boards is 8000. I
> thought the 16000 was for Armada XP when I saw that code. I think it's
> OK to use a larger timeout. It really does not hurt?
>
> All the best,
> Tony
>
> > Cheers, Phil