From mboxrd@z Thu Jan  1 00:00:00 1970
From: Daniel Gorsulowski <Daniel.Gorsulowski@esd.eu>
Date: Thu, 10 Sep 2009 16:07:29 +0200
Subject: [U-Boot] [PATCH 1/1] at91: Update MEESC board support
In-Reply-To: <20090909085825.2BF15832E8DE@gemini.denx.de>
References: <12524805241911-git-send-email-Daniel.Gorsulowski@esd.eu>
	<20090909085825.2BF15832E8DE@gemini.denx.de>
Message-ID: <4AA90821.1060106@esd.eu>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Wolfgang Denk,

Wolfgang Denk wrote:
> Dear Daniel Gorsulowski,
> 
> In message <12524805241911-git-send-email-Daniel.Gorsulowski@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@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.
> 
I'll do so.

>> +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?
>
Good idea...

>> +#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?
> 
You are right, I'll rework it.

>>  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?
> 
We had, but this does not initialize unused hardware. This is needed for
detecting hw_type and setting correct arch_number.
> 
> Best regards,
> 
> Wolfgang Denk
> 
Kind regards,
Daniel Gorsulowski