From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Wed, 23 Dec 2015 12:43:38 +0100 Subject: [U-Boot] [PATCH v2 7/8] mvebu: Support Synology DS414 In-Reply-To: <20151223112846.0096761219@mail.nwl.cc> References: <1450740358-5014-1-git-send-email-phil@nwl.cc> <20151221232435.DDE6361201@mail.nwl.cc> <5679123F.8080608@denx.de> <20151223003003.22F7161216@mail.nwl.cc> <567A53B7.8070802@denx.de> <20151223112846.0096761219@mail.nwl.cc> Message-ID: <567A88EA.3010906@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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