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

[Tony Dinh]

Hi Phil,

On Thu, Jul 4, 2024 at 4:27 AM Phil Sutter <phil at nwl.cc> wrote:
>
> 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. :(
>

Happens to me quite often, our old memories need to be filed away in
some brain cabinet  :)

... Perhaps the switch to Driver Model was when it started behaving
like it's not working, but actually not broken. The "pci init" is
needed to enumerate the XHCI controller first, before "usb start" can
work normally as before.

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

Yes, my bad. Should be 'setenv bootargs $bootargs_legacy'. Thanks!

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

I think they did not care. Their u-boot only uses internal envs.

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

DS414> sf probe
SF: Detected n25q064 with page size 256 Bytes, erase size 4 KiB, total 8 MiB

I have some stashed SPI flash patches that I need to look at again. I
vaguely recall that some SPI flash chips were not automatically
unlocked, even u-boot intention is to unlock all SPI flash like Linux
kernel has been doing.

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

Linux sata_mv driver (123K code) has a chip type for 88SX7042 and
parameters associated with it. U-Boot sata_mv (29K code) does not have
any chip type specifically called out. Apparently, it was ported from
Linux too long ago.

I did not try very hard to get it working after I saw the difference,
though. I'll make another attempt when I have some free time.

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

I used this definition for booting with modern distro only. U-boot
takes the 1st MB, and u-boot envs at 0x7E0000. So I can flash u-boot
and u-boot envs images in Linux.

mtdparts=spi0.0:1m(u-boot),7040k(kernel),64k(u-boot-env),-(data)

All the best,
Tony

>
> Cheers, Phil