[U-Boot] [PATCH v4] at91: Add support for taskit AT91SAM9G20 boards.

[Markus Hubig]

On Mon, Aug 06, 2012 at 03:01:40PM +0200, Andreas Bießmann wrote:
> On 06.08.2012 11:11, Markus Hubig wrote:
> > This adds support for the AT91SAM9G20 boards by taskit GmbH.
> > Both boards, Stamp9G20 and PortuxG20, are integrated in one
> > file. PortuxG20 is basically a SBC built around the Stamp9G20.
> > 
> > Signed-off-by: Markus Hubig <mhubig at imko.de>
> > Cc: Andreas Bießmann <andreas.devel at googlemail.com>
> > ---
> 
> please read http://www.denx.de/wiki/U-Boot/Patches, especially
> http://www.denx.de/wiki/view/U-Boot/Patches#Sending_updated_patch_versions
> 
> (patch changes missing)

OK will include them onto my next patch!

> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 84413de..7eb55db 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -243,12 +243,6 @@ Klaus Heydeck <heydeck at kieback-peter.de>
> >  	KUP4K		MPC855
> >  	KUP4X		MPC859
> >  
> > -Ilko Iliev <iliev at ronetix.at>
> > -
> > -	PM9261		AT91SAM9261
> > -	PM9263		AT91SAM9263
> > -	PM9G45		ARM926EJS (AT91SAM9G45 SoC)
> > -
> 
> NAK, this is a separate patch (already on the list)

Ups ... Fixed!

> > +static unsigned int saved_state[3] = {STATUS_LED_OFF,
> > +	STATUS_LED_OFF, STATUS_LED_OFF};
> > +
> > +void coloured_LED_init(void)
> > +{
> > +	/* Clock is enabled in board_early_init_f() */
> 
> But coloured_LED_init() is called way before!
> 
> Beware, this coloured LED stuff is sometimes used for very early debug
> purposes. To get this working you will need to switch the clocks here.
> 
> If you not insist on this colour LED framework I would appreciate to not
> add this coloured_LED_init for the stamp9g20 but use the status LED
> framework -> README.LED

OK I'll try to get rid of the coloured_LED stuff but the README.LED should
better be called a CONFUSEME.LED ... Maybe later I can include a better
example ...

> > +#ifdef CONFIG_MACB
> > +int board_eth_init(bd_t *bis)
> > +{
> > +	int rc = 0;
> please remove this unused value

Fixed!

> > +/* Misc CPU related settings */
> > +#define CONFIG_ARCH_CPU_INIT		/* call arch_cpu_init() */
> please remove this, it is not longer required cause of
> a21c65115bd95572cc80092a31b0e9ecb8710e9f

Fixed!

> > +/* setting board spezific options */
> -----------------------^

Fixed!

> > +#ifdef CONFIG_PORTUXG20
> > +# define CONFIG_MACH_TYPE		MACH_TYPE_PORTUXG20
> > +# define CONFIG_MACB
> 
> This looks really good to me. Maybe a short comment showing why you
> disable MACB for the stamp by default would be useful.

There is already a comment, but a bit later at the ethernet "section".
I think this is the best place, someone who is missing ethernet support
will have a look ...

> > +/*
> > + * SDRAM: 1 bank, 64 MB, base address 0x20000000
> > + * Already initialized before u-boot gets started.
> > + */
> > +#define CONFIG_NR_DRAM_BANKS		1
> > +#define CONFIG_SYS_SDRAM_BASE		ATMEL_BASE_CS1
> > +#define CONFIG_SYS_SDRAM_SIZE		(64 * (1024 << 10))
> 
> just a side note: shifting by 10 bit is KiB, shifting by 20 bit is MiB
> -> 64<<20 == 64*(1024<<10)

Hmm OK. Now I unified all calculations to use the shifting OP '<<'.

> 
> I think some minor tweaks for the led stuff and then we can apply this
> patch.

Fine! ;-)

Cheers, Markus