Re: [PATCH] ARM: SPEAr600: Add device-tree support to SPEAr600 boards

[Stefan Roese <sr-ynQEQJNshbs@public.gmane.org>]

Hi Arnd,

On Tuesday 13 March 2012 17:44:11 Arnd Bergmann wrote:
> On Tuesday 13 March 2012, Stefan Roese wrote:
> > This patch adds a generic target for SPEAr600 board that can be
> > configured via the device-tree. Currently only interrupts are
> > configured via device-tree. Other peripheral devices (e.g.
> > ethernet, I2C, SMI flash, FSMC NAND flash etc) will follow in
> > later patches.
> > 
> > Only the spear600-evb is currently supported. Other SPEAr600
> > based boards will follow later.
> > 
> > Signed-off-by: Stefan Roese <sr-ynQEQJNshbs@public.gmane.org>
> > Cc: Viresh Kumar <viresh.kumar-qxv4g6HH51o@public.gmane.org>
> 
> Hi Stefan,
> 
> This is very cool, welcome onboard for the DT conversion with spear600!
> 
> > +static struct amba_device *amba_devs[] __initdata = {
> > +	&gpio_device[0],
> > +	&gpio_device[1],
> > +	&gpio_device[2],
> > +	&uart_device[0],
> > +	&uart_device[1],
> > +};
> 
> I would suggest you convert these to DT next so you can remove the
> amba_devs list. Which devices are these? If they are pl061 and
> pl010/pl011, the binding should be really easy to do.

Yes. I really wanted to do that. But from a quick look at the pl011 driver 
(drivers/tty/serial/amba-pl011.c), this driver doesn't support DT probing. I 
might have missed something here though. And ideas?
 
> > +static struct platform_device *plat_devs[] __initdata = {
> > +};
> 
> This could be dropped right away, because you would never
> add anything here.

Yes.
 
> > +static void __init spear600_dt_init(void)
> > +{
> > +	unsigned int i;
> > +
> > +	/* call spear600 machine init function */
> > +	spear600_init();
> 
> spear600_init currently is an empty function, I think you can
> drop that one too.

Okay.
 
> > +	/* Add Platform Devices */
> > +	platform_add_devices(plat_devs, ARRAY_SIZE(plat_devs));
> > +
> > +	/* Add Amba Devices */
> > +	for (i = 0; i < ARRAY_SIZE(amba_devs); i++)
> > +		amba_device_register(amba_devs[i], &iomem_resource);
> 
> So although all of this can go away soon, you will have to
> add a call to of_platform_populate() here in order to add the
> devices from the device tree.

Okay.

> > +}
> > +
> > +static const char *spear600_dt_board_compat[] = {
> > +	"st,spear600-evb",
> > +	NULL
> > +};
> > +
> > +static const struct of_device_id vic_of_match[] __initconst = {
> > +	{ .compatible = "arm,pl190-vic", .data = vic_of_init, },
> > +	{ /* Sentinel */ }
> > +};
> > +
> > +static void __init spear6xx_dt_init_irq(void)
> > +{
> > +	of_irq_init(vic_of_match);
> > +}
> > +
> > +DT_MACHINE_START(SPEAR600_DT, "ST-SPEAR600-DT")
> > +	.map_io		=	spear6xx_map_io,
> > +	.init_irq	=	spear6xx_dt_init_irq,
> > +	.handle_irq	=	vic_handle_irq,
> > +	.timer		=	&spear6xx_timer,
> > +	.init_machine	=	spear600_dt_init,
> > +	.restart	=	spear_restart,
> > +	.dt_compat	=	spear600_dt_board_compat,
> > +MACHINE_END
> 
> Since there is only one upstream board file and that is for the same board,
> we can soon collapse all of it into the base platform support.
> 
> I think you should add all the code from this file to spear6xx.c instead
> of adding a new file. We can then delete the spear600.c and spear600_evb.c
> files once the DT support has matured.

Sounds like a plan. I'll rework the patch soon.

Thanks,
Stefan