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 mhubig@imko.de Cc: Andreas Bießmann andreas.devel@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!?