[U-Boot] [PATCH v2 7/8] mvebu: Support Synology DS414

[Phil Sutter]

Hi Stefan,

On Wed, Dec 23, 2015 at 08:56:39AM +0100, Stefan Roese wrote:
> >>> diff --git a/arch/arm/mach-mvebu/serdes/axp/board_env_spec.h b/arch/arm/mach-mvebu/serdes/axp/board_env_spec.h
> >>> index f00f327..3dca6a1 100644
> >>> --- a/arch/arm/mach-mvebu/serdes/axp/board_env_spec.h
> >>> +++ b/arch/arm/mach-mvebu/serdes/axp/board_env_spec.h
> >>> @@ -43,8 +43,9 @@
> >>>    #define RD_78460_SERVER_REV2_ID		(DB_78X60_PCAC_REV2_ID + 1)
> >>>    #define DB_784MP_GP_ID			(RD_78460_SERVER_REV2_ID + 1)
> >>>    #define RD_78460_CUSTOMER_ID		(DB_784MP_GP_ID + 1)
> >>> -#define MV_MAX_BOARD_ID			(RD_78460_CUSTOMER_ID + 1)
> >>> -#define INVALID_BAORD_ID		0xFFFFFFFF
> >>> +#define SYNOLOGY_DS414_ID		(RD_78460_CUSTOMER_ID + 1)
> >>> +#define MV_MAX_BOARD_ID			(SYNOLOGY_DS414_ID + 1)
> >>> +#define INVALID_BOARD_ID		0xFFFFFFFF
> >>
> >> Do you really need these changes here?
> >
> > Sadly, yes. See high_speed_env_lib.c for details: There it is needed by
> > serdes_phy_config() to get the right satr11 value via board_id_get().
> > Maybe this should be refactored to always use board_sat_r_get() and the
> > latter return a static value from a macro which board configs may define
> > instead of reading from i2c.
> 
> Yes, a refactorization would be good here. One idea is, to provide a
> _weak version of board_sat_r_get() in high_speed_env_lib.c used on
> all of these Marvell boards with a BOARD_ID. And custom boards, like
> your DS414 can provide a board specific version of board_sat_r_get()
> overwriting the weak default. And returning the board specific value.
> This way, all new custom boards would not have to touch this file
> any more.
> 
> Could you try to implement it this way?

Actually, it's already done. ;)

I started yesterday and it turned out to be much more straightforward
than I had expected. And yes, the __weak trick came to my mind then,
too.

> >>> +#define CONFIG_MV78230			/* SoC Version */
> >>> +#define CONFIG_SYNOLOGY_DS414		/* board target name for DDR training and PCIe init */
> >
> > I will review those as well. Maybe the CONFIG_ARMADA_XP solution could
> > be implemented for SoC types, too?
> 
> Yes, please. I also thought about this. Perhaps something like this
> in the mach-mvebu/Kconfig:
> 
> config ARMADA_38X
> 	bool
> 
> config ARMADA_XP
> 	bool
> 
> config MV78230
> 	select ARMADA_XP
> 	bool
> 
> config MV78260
> 	select ARMADA_XP
> 	bool
> 
> config MV78460
> 	select ARMADA_XP
> 	bool
> 
> config 88F6810
> 	select ARMADA_38X
> 	bool
> 
> config 88F6820
> 	select ARMADA_38X
> 	bool
> 
> config 88F6828
> 	select ARMADA_38X
> 	bool
> 
> config ARMADA_38X
> 	bool
> 
> config ARMADA_XP
> 	bool
> 
> ...
> 
> config TARGET_DB_MV784MP_GP
> 	bool "Support db-mv784mp-gp"
> 	select MV78460
> 
> ...
> 
> What do you think?

Looks good, I'll check.

> >>> +/* Enable DDR support in SPL (DDR3 training from Marvell bin_hdr) */
> >>> +#define CONFIG_SYS_MVEBU_DDR_AXP
> >>> +#define MV_DDR_32BIT
> >>
> >> These 2 lines can also be removed with the new patches.
> >
> > OK, CONFIG_SYS_MVEBU_DDR_AXP seems not to be used anymore at all. But
> > without MV_DDR_32BIT, BUS_WIDTH defaults to 64 which is wrong on DS414.
> 
> Ah, okay.

Maybe this should be renamed to into a CONFIG_* macro then? IMHO the
division between Kconfig symbols and board header defines is a bit
inconsistent.

Thanks, Phil