Hi Phil,
On 23.12.2015 12:30, Phil Sutter wrote:
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.
Nice! ;)
+#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.
Thanks.
+/* 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?
Yes. How about CONFIG_DDR_32BIT? Its using in some FSL MPC8xxx board already.
IMHO the division between Kconfig symbols and board header defines is a bit inconsistent.
It definitely is, yes.
Thanks, Stefan