[U-Boot] [PATCH 1/4 v4] arm: add support for the mgcoge2_arm_p1a board from keymile

[Prafulla Wadaskar]

> -----Original Message-----
> From: Heiko Schocher [mailto:hs at denx.de] 
> Sent: Monday, February 15, 2010 1:37 PM
> To: Prafulla Wadaskar
> Cc: U-Boot user list; Wolfgang Denk; Scott Wood; Stefan Roese
> Subject: Re: [PATCH 1/4 v4] arm: add support for the 
> mgcoge2_arm_p1a board from keymile
> 
> Hello Prafulla,
> 
> Prafulla Wadaskar wrote:
> >> -----Original Message-----
> >> From: Heiko Schocher [mailto:hs at denx.de]
> >> Sent: Friday, February 12, 2010 1:36 PM
> >> To: U-Boot user list
> >> Cc: Wolfgang Denk; Prafulla Wadaskar; Scott Wood; Stefan Roese
> >> Subject: [PATCH 1/4 v4] arm: add support for the
> >> mgcoge2_arm_p1a board from keymile
> >>
> >> Add support for the ARM part of the mgcoge2. This board
> >> is based on the Marvell Kirkwood (88F6281) SoC. As there
> >> come more board variants, common code is stored in
> >> board/keymile/km_arm/km_arm.c
> >>
> >> Signed-off-by: Holger Brunck <holger.brunck at keymile.com>
> >> Signed-off-by: Stefan Roese <sr at denx.de>
> >> Signed-off-by: Heiko Schocher <hs at denx.de>

...snip...

> >> diff --git a/board/keymile/common/common.c
> >> b/board/keymile/common/common.c
> >> index ec27bda..7b4eefd 100644
> >> --- a/board/keymile/common/common.c
> >> +++ b/board/keymile/common/common.c
> >> @@ -35,6 +35,7 @@
> >>  #include <libfdt.h>
> >>  #endif
> >>
> >> +#include "../common/common.h"
> > 
> > Can't you use "common.h" here ?
> 
> No, this is "just" a keymile specific header for including
> functions, which are "common" for all keymile boards ...

More important common.h that you are referring here is missing in this patch.
Secondly, since you are in the same folder you should use "common.h" instead of absolute path"

...snip...
> >> +
> >> +     /*
> >> +      * arch number of board
> >> +      */
> >> +     gd->bd->bi_arch_number = MACH_TYPE_SUEN3;
> > 
> > This does not match with the board you are supporting, 
> better to use similar name
> 
> As I said above, this boards are all the same, just different
> hardwarerevisions, so theys all have one MACH_TYPE_ ...

To me, two boards are two different hardware and must have associated with two different machine ids.
How will you manage picking different board setups in kernel for same machine ID?

If not Machine ID, you can think of using EEPROM available on board to store board version info and use it selectively.

> 
> >> +
> >> +     /* address of boot parameters */
> >> +     gd->bd->bi_boot_params = kw_sdram_bar(0) + 0x100;
> >> +
> >> +#if defined(CONFIG_KIRKWOOD_GPIO)
> > 
> > Avoid this at least for this board patch
> 
> Don;t understand this! Why? The board uses this pins for
> I2C bitbang, so I must initialize the pins ...

why you need to ifdef this? For this particular board it should be hard coded, I think you can do this for other board support if needed.

First patch in this series has to be clean standard board support without any ifdefs, subsequent patches add support for different hardware version, there if you use ifdef it makes more sense

> 
> >> +     /* init the GPIO for I2C Bitbang driver */
> >> +     kw_gpio_set_valid(SUEN3_SDA_PIN, 1);
> >> +     kw_gpio_set_valid(SUEN3_SCL_PIN, 1);
> >> +     kw_gpio_direction_output(SUEN3_SDA_PIN, 0);
> >> +     kw_gpio_direction_output(SUEN3_SCL_PIN, 0);
> >> +#if defined(CONFIG_SYS_EEPROM_WREN)
> >> +     kw_gpio_set_valid(SUEN3_ENV_WP, 38);
> >> +     kw_gpio_direction_output(SUEN3_ENV_WP, 1);
> >> +#endif
> >> +#endif
> >> +     return 0;
> >> +}

Regards..
Prafulla . .