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

Thu Jul 4 13:27:36 CEST 2024

Hi Tony,

On Fri, Jun 28, 2024 at 03:44:01PM -0700, Tony Dinh wrote:
> On Fri, Jun 28, 2024 at 3:04 PM Tony Dinh <mibodhi at gmail.com> wrote:
> > On Wed, Jun 26, 2024 at 3:31 AM Phil Sutter <phil at nwl.cc> wrote:
> > > 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!

It's nice that we may keep both so there's a fallback if data extraction
from flash fails.

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

Which is a bit odd TBH: Scrolling through git history, I found commit
c57f920504297 ("arm: mvebu: configs: ds414: Enable XHCI_PCI by default")
in which I claimed the rear USB ports to be functional after enabling
XHCI_PCI. Took me only three years to forget all the details. :(

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

Yes, that would be great. Booting legacy should be fairly simple by
calling 'run bootcmd_legacy', though automatically falling back to it is
even better.

BTW: $bootcmd_legacy contains 'run bootargs_legacy' instruction. Are you
sure this is going to work? Shouldn't this be 'setenv bootargs
$bootargs_legacy' instead?

> > > 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 :)

The saveenv problem exists only with stock u-boot as Synology apparently
didn't care to adjust CONFIG_ENV_OFFSET (or they failed). If I got that
right, it should be set to 0x100000 and thus points into "zImage" area.

Upstream we have ENV_OFFSET at 0x7E0000, i.e. the start of "RedBoot
config" (which is unused by stock firmware).

What's odd though, is that I fail to write to SPI flash using u-boot
built from current HEAD. Trying legacy boot had failed with "Ramdisk
image is corrupt or invalid" message, so I tried flashing a dump of
rd.gz and ended with "flash area is locked" error message. Despite
calling 'sf protect unlock ...' in beforehand. I wonder why you don't
have these issues. What does 'sf probe' print on your box?

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

Back in 2015, u-boot's sata_mv didn't have PCI support to begin with. I
see you spent much more time with sata_mv than I did. What's missing to
support the Marvell 88SX7042 in u-boot?

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

It does not - this change merely makes u-boot wait for longer until
giving up waiting for link. My setup involves a direct link between the
DS414 and a Realtek NIC, so it is rather surprising 16s timeout is
sufficient. ;)

Ah, one more thing: You mentioned mtdparts to define a flash map which
should simplify flashing u-boot into the right spot. I haven't had any
luck doing so for SPI flash, do you?

Cheers, Phil