[U-Boot] [PATCH 1/1] at91: Update MEESC board support

[Wolfgang Denk]

Dear Daniel Gorsulowski,

In message <12524805241911-git-send-email-Daniel.Gorsulowski at esd.eu> you wrote:
> This patch implements several updates:
> -Disable CONFIG_ENV_OVERWRITE
> -Add new hardware style variants and set the arch numbers appropriate (autodetect)
> -Pass the serial# and hardware revision to the kernel
> 
> Signed-off-by: Daniel Gorsulowski <Daniel.Gorsulowski at esd.eu>
> ---

You should indicate that this is version 2 of an earlier patch, and
describe what has been changed compared to earlier versions.

And as it's a single patch, it makes no sense to number it, i. e.
please omit the "1/1" part.

> +static void meesc_set_arch_number(void)
> +{
> +	/* read the "Type" register of the ET1100 controller */
> +	hw_type = readb(CONFIG_ET1100_BASE);
> +
> +	switch (hw_type) {
> +		case 0x11:
> +		case 0x3F:
> +			/* ET1100 present,
> +			   arch number of MEESC-Board */
> +			gd->bd->bi_arch_number = MACH_TYPE_MEESC;
> +			break;
> +		case 0xFF:
> +			/* no ET1100 present,
> +			   arch number of EtherCAN/2-Board */
> +			gd->bd->bi_arch_number = MACH_TYPE_ETHERCAN2;
> +			break;
> +		default:
> +			/* assume, no ET1100 present,
> +			   arch number of EtherCAN/2-Board */
> +			gd->bd->bi_arch_number = MACH_TYPE_ETHERCAN2;
> +			break;
> +	}

You have the same switch() in checkboard() - maybe you move this code
there, so you can avoid the whole function?

> +#ifdef CONFIG_SERIAL_TAG
> +void get_board_serial(struct tag_serialnr *serialnr)
> +{
> +	char *str;
> +
> +	str = strchr(getenv("serial#"), '_');
> +	if (str) {
> +		serialnr->high =	(*(str + 1) << 8) | *(str + 2);
> +		serialnr->low =		simple_strtoul(str + 3, NULL, 16);

Hm... that looks dangerous to me. Who tells you that the value of the
"serial#" envrionment variable has that many characters?

>  int board_init(void)
>  {
>  	/* Peripheral Clock Enable Register */
> @@ -174,8 +234,15 @@ int board_init(void)
>  					1 << AT91SAM9263_ID_PIOB |
>  					1 << AT91SAM9263_ID_PIOCDE);
>  
> -	/* arch number of MEESC-Board */
> -	gd->bd->bi_arch_number = MACH_TYPE_MEESC;
> +	/* initialize ET1100 Controller */
> +	meesc_ethercat_hw_init();

I thought we had agreed not to initialize the Ethernet hardware if it
not used by U-Boot?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There is, however, a strange, musty smell in the air that reminds  me
of something...hmm...yes...I've got it...there's a VMS nearby, or I'm
a Blit.          - Larry Wall in Configure from the perl distribution