Re: [RFC PATCH v2 1/1] powerpc/embedded6xx: Add support for Motorola/Emerson MVME5100.

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Tue, 2013-09-10 at 18:47 -0500, Scott Wood wrote:

> No blank line before }
> 
> > +CONFIG_CMDLINE_BOOL=y
> > +CONFIG_CMDLINE="console=ttyS0,9600 ip=dhcp root=/dev/nfs"
> 
> I take it there's no way to pass a command line in from whatever loader
> this board uses... but you could put it in the dts instead.

No, please don't put that in the device-tree. Somebody might want
different settings, this is typically what .config is for.

> Did you ever figure out the problem with the combined defconfig?
> 
> > +	help
> > +          This option enables support for the Motorola (now Emerson) MVME5100
> > +	  board.
> 
> Whitespace
> 
> > +/* Board register addresses. */
> > +#define	BOARD_STATUS_REG	0xfef88080
> > +#define	BOARD_MODFAIL_REG	0xfef88090
> > +#define	BOARD_MODRST_REG	0xfef880a0
> > +#define	BOARD_TBEN_REG		0xfef880c0
> > +#define BOARD_SW_READ_REG	0xfef880e0
> > +#define	BOARD_GEO_ADDR_REG	0xfef880e8
> > +#define	BOARD_EXT_FEATURE1_REG	0xfef880f0
> > +#define	BOARD_EXT_FEATURE2_REG	0xfef88100
> 
> Use a space rather than a tab after #define.
> 
> > +static unsigned int pci_membase;
> 
> phys_addr_t
> 
> > +static void mvme5100_restart(char *cmd)
> > +{
> > +	u_char			*restart;
> 
> Is all that tabbing before "*restart" really necessary?
> 
> > +	restart = ioremap(BOARD_MODRST_REG, 4);
> > +	local_irq_disable();
> > +	mtmsr(mfmsr() | MSR_IP);
> > +
> > +	out_8((u_char *) restart, 0x01);
> 
> If ioremap() fails you'll panic here.
> 
> In any case, you should map things at boot time.
> 
> -Scott
> 
>