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

[Phil Sutter]

Hi,

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.

> > 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. :)

[...]

> > 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.

[...]

> > +/* 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?

[...]

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

Thanks, Phil