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

[Markus Hubig]

Hello Andreas,

thanks for your responce. I will provide an updated patch based on
your comments!

On Wed, Aug 01, 2012 at 11:58:22AM +0200, Andreas Bießmann wrote:
> ---8<---
> andreas at andreas-mbp % ./tools/checkpatch.pl
> U-Boot-at91-Add-support-for-taskit-AT91SAM9G20-boards..patch
> WARNING: Whitespace before semicolon
> #214: FILE: board/taskit/stamp9g20/stamp9g20.c:123:
> +		;
> 
> total: 0 errors, 1 warnings, 484 lines checked
> 
> NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> MULTISTATEMENT_MACRO_USE_DO_WHILE
> 
> U-Boot-at91-Add-support-for-taskit-AT91SAM9G20-boards..patch has style
> problems, please review.
> 
> If any of these errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
> --->8---
> 
> I know this part is copied from other at91 boards but I wonder how we
> should handle these while-loops without content.

Oh I didn't recognice there is a version of checkpatch provided with
u-boot. I used the one from kernel.org which didn't put a warning out
for this one ...

> On 30.07.12 20:01, 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>
> > ---
> >  board/taskit/stamp9g20/Makefile    |   52 ++++++++
> >  board/taskit/stamp9g20/stamp9g20.c |  199 +++++++++++++++++++++++++++++++
> >  boards.cfg                         |    2 +
> >  include/configs/stamp9g20.h        |  225 ++++++++++++++++++++++++++++++++++++
> 
> MAINTAINER entry is missing

Fixed!

> > +#ifdef CONFIG_MACB
> > +static void stamp9G20_macb_hw_init(void)
> > +{
> > +	struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
> > +	struct at91_port *pioa = (struct at91_port *)ATMEL_BASE_PIOA;
> > +	struct at91_rstc *rstc = (struct at91_rstc *)ATMEL_BASE_RSTC;
> > +	unsigned long erstl;
> > +
> > +	/* Enable MACB Chip, this is the enable PIN on Stamp Adaptor*/
> This comment is a bit misleading. Isn't MACB the SoC MAC component? Why
> should we switch an external element to enable an internal part? Can you
> please rewrite the comment to be more precise?

Hmm yes you're right. That pin enables the PHY Chip which of course is
an external component ...

> > +	at91_set_gpio_output(AT91_PIN_PA26, 0);
> > +
> > +	/* Enable EMAC clock */
> > +	writel(1 << ATMEL_ID_EMAC0, &pmc->pcer);
> 
> Is it possible to move this 'enable clock' into at91_macb_hw_init()?
> Does your PHY address setting still work then?
> If yes, can you please send a separate patch first adding the 'enable
> clock' into the correct at91_macb_hw_init()?

Yes it works. I put it into arch/arm/cpu/arm926ejs/at91/at91sam9260_devices.c
and will send a separate patch.

> > +	/* Need to reset PHY -> 500ms reset */
> > +	writel(AT91_RSTC_KEY | AT91_RSTC_MR_ERSTL(13) |
> > +		AT91_RSTC_MR_URSTEN, &rstc->mr);
> 
> Hmm ... is it OK to generate the user reset here? I know this is the
> same in at least at91sam9263ek, can you please check if we should
> instead delete that bit in MR?

MR? Sorry I don't get this one. Please explain a bit ...

> > +
> > +	writel(AT91_RSTC_KEY | AT91_RSTC_CR_EXTRST, &rstc->cr);
> > +
> > +	/* Wait for end hardware reset */
> > +	while (!(readl(&rstc->sr) & AT91_RSTC_SR_NRSTL))
> > +		;
> 
> Endless loop if bit is not toggling, can you please add some watchdog
> reset (if you still use wdt in your board) and some timeout?
> This will also solve the warning detected by checkpatch.

Maybe something like this?

| /* Wait for end of hardware reset */
| while (!(readl(&rstc->sr) & AT91_RSTC_SR_NRSTL))
| {
| 	/* avoid shutdown by watchdog */
| 	hw_watchdog_reset();
| 	
| 	mdelay(10);
| 	timeout--;
| 	
| 	/* timeout for not getting stuck in an endless loop */
| 	if (timeout <= 0) {
| 		debug("ERROR: Timeout waiting for PHY reset!\n");
| 		break;
| 	};
| };

> > +int board_early_init_f(void)
> > +{
> > +	struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
> > +
> > +	/* Enable clocks for all PIOs */
> > +	writel((1 << ATMEL_ID_PIOA) | (1 << ATMEL_ID_PIOB) |
> > +		(1 << ATMEL_ID_PIOC), &pmc->pcer);
> 
> you should initialize seriald_hw here to avoid strange characters on
> serial line when switching from at91bootstrap to u-boot.

If I do so I don't get serial output any more ;-(

> > +#ifdef CONFIG_PORTUXG20
> > +	gd->bd->bi_arch_number = MACH_TYPE_PORTUXG20;
> > +#else
> > +	gd->bd->bi_arch_number = MACH_TYPE_STAMP9G20;
> > +#endif
> 
> I would favor the generic CONFIG_MACH_TYPE here -> see
> arch/arm/lib/board.c:401
> Just add the definition in your board config header and remove these
> lines here.

Fixed!

> > +	/* adress of boot parameters */
> > +	gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
> > +
> > +	at91_set_gpio_output(AT91_PIN_PC9, 1);
> > +	at91_set_gpio_output(AT91_PIN_PC5, 1);
> 
> Can you please add some comment why switching these pins?

I which I could! My documentation is not detailed enough to
figure it out. But I will write an email to taskit ...

AT91_PIN_PC9 is connected to TIOB0/NCS5/CFS1
AT91_PIN_PC5 is connected to A24/SPI1_NPCS1 (maybe the red LED)

> > +#ifdef CONFIG_MACB
> > +void reset_phy(void)
> > +{
> > +	/*
> > +	 * Initialize ethernet HW addr prior to starting Linux,
> > +	 * needed for nfsroot
> > +	 */
> > +	eth_init(gd->bd);
> 
> This is not longer required, the HW address should always be set by
> generic code. Can you please check this?

Fixed!

> > +}
> > +
> > +int board_eth_init(bd_t *bis)
> > +{
> > +	int rc = 0;
> > +	rc = macb_eth_initialize(0, (void *)ATMEL_BASE_EMAC0, 0x00);
> > +	return rc;
> 
> return macb_eth_initialize()?

Jup!

> > +/* Misc CPU related settings */
> > +#define CONFIG_ARCH_CPU_INIT		/* call arch_cpu_init()              */
> > +#undef  CONFIG_USE_IRQ			/* we don't need IRQ stuff           */
> 
> just remove that, it was not defined before

Fixed!

> > +/*
> > + * Initial stack pointer: 4k - GENERATED_GBL_DATA_SIZE in internal SRAM,
> > + * leaving the correct space for initial global data structure above that
> > + * address while providing maximum stack area below.
> > + */
> > +#define CONFIG_STACKSIZE        (32 * 1024)     /* 32k regular stack size   */
> 
> CONFIG_STACKSIZE is nowhere used, please remove.

Fixed!

> > +#define	CONFIG_USART_ID			ATMEL_ID_SYS
> -----------^
> no tab here please

Fixed!

> > +#define CONFIG_BAUDRATE			115200
> > +#define CONFIG_SYS_BAUDRATE_TABLE	{115200, 19200, 38400, 57600, 9600}
> 
> This is same as generic table, please remove (was added in
> 26750c8aee2383a026e0cf89e9310628d3a5a6a0)

Fixed!

> > +#define CONFIG_USART3			/* USART 3 is used as DBGU          */
> Well, this is a remnant ... please remove (BTW: the statement in comment
> is wrong, USART3 is available beside DBGU ... ;)

Fixed!

> > +
> > +/* Ethernet configuration */
> > +#define CONFIG_MACB			/* initialize the ethernet port     */
> ------------------------------------------------------------------------^
> Can you please remove these white space? I know there are a lot of at91
> config header formatted like this, but I personally dislike this style.
> Especially in this case where the previous intention of aligning all
> these '*/' on the 80th char of a line is still broken by other lines. I
> think just a single white space is good here.

Formating source code with tabs is already broken by default! You can never get
it right ... ;-( If I look at it with my vim all is well formated ...  

Fixed ...

> > +#ifdef CONFIG_MACB
> 
> Well, you defined it some lines above, is there any case where you want
> to switch the network off? If no please remove this ifdef block and just
> define required stuff.

I thought about making it easy to disable the eth feature if one is not using
it with the stamp9g20 CPU Board ... but anyway you're right!

> > +# define CONFIG_RMII			/* use reduced MII inteface         */
> > +# define CONFIG_RESET_PHY_R		/* call reset_phy() after reloc.    */
> I think this could be remove once you checked if eth_init() is really
> required.

It's gone ...

> > +/* BOOTP options */
> > +#ifdef CONFIG_MACB
> 
> same comment as above, if you do not want to disable MACB, remove this
> ifdef.

Fixed!

> > + * The NAND Flash partitions:
> > + * ==========================================
> > + * 0x0000000-0x001ffff -> 128k, bootstrap
> > + * 0x0020000-0x005ffff -> 256k, u-boot
> > + * 0x0060000-0x007ffff -> 128k, env1
> > + * 0x0080000-0x009ffff -> 128k, env2 (backup)
> > + * 0x0100000-0x03fffff ->   2M, kernel
> 
> You should think about bad blocks ... 2MiB for kernel is IMHO way to
> small for normal configurations (especially when you care about boot
> time, think about io-/cpu-bound stuff: high compression vs fast access).
> The space will be even smaller if you hit some bad blocks in this area
> ... I really recommend to use a bit more here for your kernel, but its
> up to you whether you change it in your next patch version or not.

Increased it to 6Mb ... 

> > +	"basicargs=console=ttyS0,115200 mem=64M\0"			\
> 
> Only a side note ... isn't the mem parameter handled by ATAGS correctly?
> Or why do you add this here?

Fixed!

> > +	"sdboot=setenv bootargs ${basicargs} ${mtdparts} "		\
> > +		"root=/dev/mmcblk0p1 rootdelay=1; "			\
> 
> another side note ... you should read about rootwait in kernel docs

Fixed ;-)

> > +	"flashboot=setenv bootargs ${basicargs} ${mtdparts} "		\
> > +		"root=/dev/mtdblock5 rootfstype=jffs2; "		\
> 
> another side note ... you should read about ubifs ;)
> Don't get me wrong, these side notes are just for you information and
> not a request for change!

Keep on writing side notes! I learned a lot the last couple of hours ;-)

> > +#undef CONFIG_CMD_SOURCE
> 
> The source command is a really useful command for scripting, you should
> not disable it except you need to e.g. save space in text section. I
> guess this not the case for you cause it saves just about 700 bytes in
> code section.
> Beside that I can not see another way for updating your device. You
> disabled flash access to bootstrap, u-boot and env sections in mtdparts
> so you will need u-boot to write to these sections or you need to change
> these parameters in u-boot. For all these cases scripting in u-boot is
> really viable. Think about ... (just another side note ;)

Yes I definitly have to think about a deployment scenario and than this
will be really useful. But i just overlooked it until now ...

> > +#ifdef CONFIG_USE_IRQ
> > +# error CONFIG_USE_IRQ not supported
> > +#endif
> 
> I tend to say, remove that. I can not imagine a case where one enables
> irq accidentially, this should only be done intentionally. Then this
> part is not really useful.
> But I know this is copied from other at91 config files.

It's gone! :-)

Thank you very much for this good review! I Learned a lot and will post
the updated patches soon!

Cheers, Markus

PS.: I will send a new patch with all your requested changes already
included, not a couple of small ones based in this one? Right!?