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

[Stefan Roese <sr@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