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@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 <mhu...@imko.de>
> > Cc: Andreas Bießmann <andreas.de...@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!?
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
