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 <mibo...@gmail.com> wrote: > > On Wed, Jun 26, 2024 at 3:31 AM Phil Sutter <p...@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