[PATCH linux dev-4.10 v3] ARM: aspeed: Add Mellanox MSN machine (aspeed arch)

[Andrew Jeffery]

Hi Mykola,

As Joel's been looking at the series for you I'll defer to his
preferences, but I've made some comments below.

On Mon, 2017-05-29 at 12:15 +0300, Mykola Kostenok wrote:
> Initial introduction of Mellanox switches of MSNXXXX family equipped
> with Aspeed 2520 BMC SoC. This adds the platform early initialization.
> 
> > Signed-off-by: Mykola Kostenok <c_mykolak at mellanox.com>
> ---
> v1->v2
> Fixed issues pointed out by Joel:
> - Make commit title shorter.
> - Replace flash layout from separate dtsi to dts.
> - Change compatible = "mellanox,msnxxxx-bmc" to "mellanox,msn-bmc".
> - Remove no-hw-checksum from dts.
> - Add comments.
> - Remove WD2 disable from aspeed.c
> - Add wdt2 to dts.
> 
> v2->v3
> - Split v2 patch into three separate.
> ---
>  arch/arm/mach-aspeed/aspeed.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/arm/mach-aspeed/aspeed.c b/arch/arm/mach-aspeed/aspeed.c
> index 0f1a536ba1b2..942cdffed9bd 100644
> --- a/arch/arm/mach-aspeed/aspeed.c
> +++ b/arch/arm/mach-aspeed/aspeed.c
> @@ -188,6 +188,27 @@ static void __init do_lanyang_setup(void)
> >  	writel(reg & ~BIT(4), AST_IO(AST_BASE_LPC | 0x98));
>  }
>  
> +static void __init do_mellanox_setup(void)
> +{
> > +	unsigned long reg;
> +
> > +	do_common_setup();
> +
> +	/* Set strap to RGMII for dedicated PHY networking */

Nitpicking here, but the comment seems misleading as it's only true for
one of the interfaces despite configuring both. The sequence below sets
MAC1 to RMII/NCSI and MAC2 to RGMII.

> +	reg = readl(AST_IO(AST_BASE_SCU | 0x70));
> > +	reg |= BIT(7);
> > +	reg &= ~BIT(6);
> > +	writel(reg, AST_IO(AST_BASE_SCU | 0x70));
> +
> > +	/* Disable UART1 Reset from LPC */
> +	writel(0x00000A00, AST_IO(AST_BASE_LPC | 0x98));

My reading of the AST2500 datasheet suggests 0x00000A00 is a reserved
value? Given this isn't a read/modify/write sequence I think it
deserves more documentation than just mentioning disabling UART1, i.e.
bits that you're touching that you might not care about. We've had this
lack of information in the past in the board file and it makes life
more difficult when removing the hacks when we eventually have driver
support for the configurations.

Zaius and Lanyang both have more documentation and do
read/modify/write. I think either the Zaius/Lanyang approach should be
used, or the commentary improved.

Cheers,

Andrew

> +
> > +	/* Enable RMII1 50MHz RCLK output.*/
> > +	reg = readl(AST_IO(AST_BASE_SCU | 0x48));
> > +	reg |= BIT(29);
> > +	writel(reg, AST_IO(AST_BASE_SCU | 0x48));
> +}
> +
> >  #define SCU_PASSWORD	0x1688A8A8
>  
>  static void __init aspeed_init_early(void)
> @@ -227,6 +248,8 @@ static void __init aspeed_init_early(void)
> >  		do_romulus_setup();
> >  	if (of_machine_is_compatible("inventec,lanyang-bmc"))
> >  		do_lanyang_setup();
> > +	if (of_machine_is_compatible("mellanox,msn-bmc"))
> > +		do_mellanox_setup();
>  }
>  
>  static void __init aspeed_map_io(void)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170530/c1b614f8/attachment.sig>