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

[Phil Sutter <phil@nwl.cc>]

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.

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

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

A pending issue for me is inability to 'saveenv' - the flash seems
read-only in that spot. Does it work for you?

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

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

Cheers, Phil