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

[Stefan Roese]

Hi Phil,

On 23.12.2015 01:31, Phil Sutter wrote:
> On Tue, Dec 22, 2015 at 10:05:03AM +0100, Stefan Roese wrote:
>> I've consolidated a bit of the Armada XP / 38x defines and
>> Kconfig options just very recently. Please take a look at
>> these two patches from yesterday:
>>
>> http://patchwork.ozlabs.org/patch/559579/
>> http://patchwork.ozlabs.org/patch/559580/
>>
>> It would be great if you could base your DS414 support on
>> top of these as well (sorry, I missed adding you on CC for these
>> patches). It should fairly easy and make things clearer (e.g.
>> CONFIG_ARMADA_XP only defined on AXP devices).
>
> I see.
>
>> I could also add a new git branch available, with the latest
>> versions of all the mvebu related patches applied. Just let
>> me know if this would help.
>
> No problem, I just fetched the relevant patches from the list and
> applied them locally.
>
> [...]
>
>>> SATA Support
>>> ------------
>>>
>>> There is a Marvell 88SX7042 controller attached to PCIe which is
>>> supported by Linux's sata_mv driver but sadly not U-Boot's sata_mv.
>>> I'm not sure if extending the latter to support PCI devices is worth the
>>> effort at all.
>>
>> Yes, this is probably not worth the effort.
>
> What bothers me is that Synology's patched U-Boot supports it, so I
> consider it a regression for users wanting to switch to mainline. And
> since there is sata_mv in U-Boot now and Linux's sata_mv simply supports
> both the platform device and the various PCI devices, one might think it
> should be straightforward to add support to it. But Linux's sata_mv with
> it's 4.5k LoC is quite a thing, too.
>
> [...]
>
>>> EHCI Support
>>> ------------
>>>
>>> This seems functional after issuing 'usb start'. At least it detects USB
>>> storage devices, and IIRC reading from them was OK. OTOH Linux fails to
>>> register the controller if 'usb start' wasn't given before in U-Boot.
>>>
>>> According to Synology sources, this board seems to support USB device
>>> (gadget?) mode. Though I didn't play around with it.
>>
>> This is something that should be fixed. Linux should be able to
>> use all devices without any bootloader interference. So it might
>> be, that some USB related configuration is missing here. Perhaps
>> in the USB PHY (setup setup_usb_phys in cpu.c).
>>
>> Does this work in Linux when the original U-Boot is used?
>
> I will check. Also printing the relevant registers in Linux and
> comparing the values in working and broken condition might help.

Yes, thanks. Please keep me informed on this.

>>> PCIe Support
>>> ------------
>>>
>>> This is fine, but trying to gate the clocks of unused lanes will hang
>>> PCI enum. In addition to that, pci_mvebu seems not to support DM_PCI.
>>
>> Right. Patches welcome. ;)
>
> I actually started, thought it can't be that hard. So far I'm stuck at a
> point where enumeration gets just 0xfbfb for both vendor and product
> IDs. Looks like a follow-up project to me. :)

Yes, that would be really great! :)

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

> [...]
>
>>> +/* DDR3 static MC configuration */
>>> +
>>> +/* 1G_v1 (4x2Gbits) adapted by DS414 */
>>> +MV_DRAM_MC_INIT syno_ddr3_b0_667_1g_v1[MV_MAX_DDR3_STATIC_SIZE] = {
>>> +	{0x00001400, 0x73014A28},	/*DDR SDRAM Configuration Register */
>>> +	{0x00001404, 0x30000800},	/*Dunit Control Low Register */
>>> +	{0x00001408, 0x44148887},	/*DDR SDRAM Timing (Low) Register */
>>> +	/* {0x0000140C, 0x38000C6}, */	/*DDR SDRAM Timing (High) Register */
>>> +	{0x0000140C, 0x3AD83FEA},	/*DDR SDRAM Timing (High) Register */
>>
>> Why do you have commented-out values here? Its generally not allowed
>> to add "dead code". So please either add it in some comment, so that
>> it can be identified as a important comment. Or remove it completely.
>
> Took that over from Synology's sources (mainly for the sake of
> completeness). But you're right, without explanation (which I couldn't
> find, either) it's pretty much useless.
>
> [...]
>
>>> +	/* gate unused clocks */
>>> +	/* Note: disabling unused PCIe lanes will hang PCI bus scan */
>>> +	pwr_mng_ctrl_reg = reg_read(POWER_MNG_CTRL_REG);
>>> +	pwr_mng_ctrl_reg &= ~(BIT(0));				/* Audio */
>>> +	pwr_mng_ctrl_reg &= ~(BIT(1) | BIT(2));			/* GE3, GE2 */
>>> +	//pwr_mng_ctrl_reg &= ~(BIT(10) | BIT(11) | BIT(12));	/* PCIe11 - PCIe13 */
>>
>> C++ style comments are not allowed, sorry. And again the same comment
>> about dead code.
>
> Ah, yes. Left it there in hopes to solve this PCI enum problem soon
> (probably will be when DM_PCI works as that relieves us from doing full
> bus scan), will put it into the note above instead.
>
> [...]
>
>>> +/*
>>> + * High Level Configuration Options (easy to change)
>>> + */
>>> +#define CONFIG_ARMADA_XP		/* SOC Family Name */
>>
>> This will not be needed any more with the 2 patches I reference
>> above. The SoC is selected via Kconfig then.
>
> OK.
>
>>> +#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?

> [...]
>
>>> +/* 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.

Thanks,
Stefan