[U-Boot] [PATCH v2] Add support for KMC KZM-ARM11-01 board

[U-Boot] [PATCH v2] Add support for KMC KZM-ARM11-01 board

[Atsuo Igarashi]

Hi,

I've fixed which Jean-Christophe pointed out.

Regards,
Atsuo
---
This patch adds support for KMC KZM-ARM11-01 board.

Signed-off-by Atsuo Igarashi <atsuo_igarashi@tripeaks.co.jp>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kzm_arm11fix.diff
Type: text/x-diff
Size: 23939 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080909/76b1b36d/attachment-0001.diff 

[U-Boot] [PATCH v2] Add support for KMC KZM-ARM11-01 board

[Wolfgang Denk]

Dear Atsuo Igarashi,

In message <48C60E4F.6010704@tripeaks.co.jp> you wrote:
>
> Hi,
> 
> I've fixed which Jean-Christophe pointed out.
> 
> Regards,
> Atsuo
> ---
> This patch adds support for KMC KZM-ARM11-01 board.
> 
> Signed-off-by Atsuo Igarashi <atsuo_igarashi@tripeaks.co.jp>

The you have the content of the message swapped. The "Hi, I've
..Regards, Atsuo" part belongs *below the '---' line, and the "This
patch ... Signed-off-by:" part belongs *above* it.

The text above the '---' line will become the commit message, the text
below it is just comment.

...
> +int board_init (void)
> +{
> +	int i;
...
> +	/* RedBoot also has an empty loop with 100000 iterations here -
> +	 * clock doesn't run yet */

Incorrect multiline comment.

> +	for (i = 0; i < 100000; i++)
> +		;
...
> +	/* clock still doesn't run */
> +	for (i = 0; i < 100000; i++)
> +		;

Be careful here, this might not do what you think it does. The
compiler might optimize these loops away.

Why are they needed in the first place?

> +/*
> + * The MX31ADS board seems to have a hardware "peculiarity" confirmed under
> + * U-Boot, RedBoot and Linux: the ethernet Rx signal is reaching the CS8900A
> + * controller inverted. The controller is capable of detecting and correcting
> + * this, but it needs 4 network packets for that. Which means, at startup, you
> + * will not receive answers to the first 4 packest, unless there have been some
> + * broadcasts on the network, or your board is on a hub. Reducing the ARP
> + * timeout from default 5 seconds to 200ms we speed up the initial TFTP
> + * transfer, should the user wish one, significantly.
> + */
> +#define CONFIG_ARP_TIMEOUT	200UL


Is this really needed on your hardware, too? I doubt it.

> +#define CFG_HZ			CONFIG_MX31_CLK32 /* use 32kHz clock as source */

This is wrong. CFG_HZ must always be defined as 1000; it is a
constant.

> +/* Address and size of Redundant Environment Sector	*/
> +#define CFG_ENV_OFFSET_REDUND	(CFG_ENV_OFFSET + CFG_ENV_SIZE)
> +#define CFG_ENV_SIZE_REDUND	CFG_ENV_SIZE
> +
> +/* S29WS256N NOR flash has 4 32KiB small sectors at the beginning and at the end.
> + * The rest of 32MiB is in 128KiB big sectors. U-Boot occupies the low 4 sectors,
> + * if we put environment next to it, we will have to occupy 128KiB for it.
> + * Putting it at the top of flash we use only 32KiB. */
> +#define CFG_ENV_ADDR		(CFG_MONITOR_BASE + CFG_ENV_SECT_SIZE)

This is a very ugly set up, because you split the U-Boot related
informatiomn, and you block both ends of your flash memory. Why don;t
you locate the environment for example in the third and fourts
sectors, i. e. use such a setup (called "embedded environment"):

	sector 1:  32 KiB - U-Boot code
	sector 2:  32 KiB - more U-Boot code
	sector 3:  32 KiB - environment, first copy
	sector 4:  32 KiB - environment, second (redundant) copy
	sector 5: 128 KiB - more U-Boot code
	...

?

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
Every living thing wants to survive.
	-- Spock, "The Ultimate Computer", stardate 4731.3

[U-Boot] [PATCH v2] Add support for KMC KZM-ARM11-01 board

[Atsuo Igarashi]

Dear Wolfgang Denk,

> Dear Atsuo Igarashi,
> 
> In message <48C60E4F.6010704@tripeaks.co.jp> you wrote:
>> Hi,
>>
>> I've fixed which Jean-Christophe pointed out.
>>
>> Regards,
>> Atsuo
>> ---
>> This patch adds support for KMC KZM-ARM11-01 board.
>>
>> Signed-off-by Atsuo Igarashi <atsuo_igarashi@tripeaks.co.jp>
> 
> The you have the content of the message swapped. The "Hi, I've
> ..Regards, Atsuo" part belongs *below the '---' line, and the "This
> patch ... Signed-off-by:" part belongs *above* it.
> 
> The text above the '---' line will become the commit message, the text
> below it is just comment.

Sorry, I'll do so next time.

> 
> ...
>> +int board_init (void)
>> +{
>> +	int i;
> ...
>> +	/* RedBoot also has an empty loop with 100000 iterations here -
>> +	 * clock doesn't run yet */
> 
> Incorrect multiline comment.
> 
>> +	for (i = 0; i < 100000; i++)
>> +		;
> ...
>> +	/* clock still doesn't run */
>> +	for (i = 0; i < 100000; i++)
>> +		;
> 
> Be careful here, this might not do what you think it does. The
> compiler might optimize these loops away.
> 
> Why are they needed in the first place?
> 

Indeed. Since these are based on Freescale's ADS board,
I'm not sure what means these loops, but these not necessary for my board.
I'll remove these code of the CPLD initializing.


>> +/*
>> + * The MX31ADS board seems to have a hardware "peculiarity" confirmed under
>> + * U-Boot, RedBoot and Linux: the ethernet Rx signal is reaching the CS8900A
>> + * controller inverted. The controller is capable of detecting and correcting
>> + * this, but it needs 4 network packets for that. Which means, at startup, you
>> + * will not receive answers to the first 4 packest, unless there have been some
>> + * broadcasts on the network, or your board is on a hub. Reducing the ARP
>> + * timeout from default 5 seconds to 200ms we speed up the initial TFTP
>> + * transfer, should the user wish one, significantly.
>> + */
>> +#define CONFIG_ARP_TIMEOUT	200UL
> 
> 
> Is this really needed on your hardware, too? I doubt it.
> 

Probably, this is not needed.
I'll remove this.

>> +#define CFG_HZ			CONFIG_MX31_CLK32 /* use 32kHz clock as source */
> 
> This is wrong. CFG_HZ must always be defined as 1000; it is a
> constant.
> 

Okay, and it seems that I should fix something else too.

>> +/* Address and size of Redundant Environment Sector	*/
>> +#define CFG_ENV_OFFSET_REDUND	(CFG_ENV_OFFSET + CFG_ENV_SIZE)
>> +#define CFG_ENV_SIZE_REDUND	CFG_ENV_SIZE
>> +
>> +/* S29WS256N NOR flash has 4 32KiB small sectors at the beginning and at the end.
>> + * The rest of 32MiB is in 128KiB big sectors. U-Boot occupies the low 4 sectors,
>> + * if we put environment next to it, we will have to occupy 128KiB for it.
>> + * Putting it at the top of flash we use only 32KiB. */
>> +#define CFG_ENV_ADDR		(CFG_MONITOR_BASE + CFG_ENV_SECT_SIZE)
> 
> This is a very ugly set up, because you split the U-Boot related
> informatiomn, and you block both ends of your flash memory. Why don;t
> you locate the environment for example in the third and fourts
> sectors, i. e. use such a setup (called "embedded environment"):
> 
> 	sector 1:  32 KiB - U-Boot code
> 	sector 2:  32 KiB - more U-Boot code
> 	sector 3:  32 KiB - environment, first copy
> 	sector 4:  32 KiB - environment, second (redundant) copy
> 	sector 5: 128 KiB - more U-Boot code
> 	...
> 
> ?

Indeed. I think so too.
I'll rework.

Regards,
Atsuo Igarashi

> 
> Best regards,
> 
> Wolfgang Denk
> 

[U-Boot] [PATCH v2] Add support for KMC KZM-ARM11-01 board

[Atsuo Igarashi]

Dear Wolfgang Denk,

>>> +/*
>>> + * The MX31ADS board seems to have a hardware "peculiarity" 
>>> confirmed under
>>> + * U-Boot, RedBoot and Linux: the ethernet Rx signal is reaching the 
>>> CS8900A
>>> + * controller inverted. The controller is capable of detecting and 
>>> correcting
>>> + * this, but it needs 4 network packets for that. Which means, at 
>>> startup, you
>>> + * will not receive answers to the first 4 packest, unless there 
>>> have been some
>>> + * broadcasts on the network, or your board is on a hub. Reducing 
>>> the ARP
>>> + * timeout from default 5 seconds to 200ms we speed up the initial TFTP
>>> + * transfer, should the user wish one, significantly.
>>> + */
>>> +#define CONFIG_ARP_TIMEOUT    200UL
>>
>>
>> Is this really needed on your hardware, too? I doubt it.
>>
> 
> Probably, this is not needed.
> I'll remove this.
> 

In our board which has SMC9118 controller, it seems that it needs a few hundred
milliseconds wait after the initialization. So this is needed.
I'll modify only these comment.


>>> +/* Address and size of Redundant Environment Sector    */
>>> +#define CFG_ENV_OFFSET_REDUND    (CFG_ENV_OFFSET + CFG_ENV_SIZE)
>>> +#define CFG_ENV_SIZE_REDUND    CFG_ENV_SIZE
>>> +
>>> +/* S29WS256N NOR flash has 4 32KiB small sectors at the beginning 
>>> and at the end.
>>> + * The rest of 32MiB is in 128KiB big sectors. U-Boot occupies the 
>>> low 4 sectors,
>>> + * if we put environment next to it, we will have to occupy 128KiB 
>>> for it.
>>> + * Putting it at the top of flash we use only 32KiB. */
>>> +#define CFG_ENV_ADDR        (CFG_MONITOR_BASE + CFG_ENV_SECT_SIZE)
>>
>> This is a very ugly set up, because you split the U-Boot related
>> informatiomn, and you block both ends of your flash memory. Why don;t
>> you locate the environment for example in the third and fourts
>> sectors, i. e. use such a setup (called "embedded environment"):
>>
>>     sector 1:  32 KiB - U-Boot code
>>     sector 2:  32 KiB - more U-Boot code
>>     sector 3:  32 KiB - environment, first copy
>>     sector 4:  32 KiB - environment, second (redundant) copy
>>     sector 5: 128 KiB - more U-Boot code
>>     ...
>>
>> ?
> 
> Indeed. I think so too.
> I'll rework.
> 

In our board which has S29GL512N NOR flash, it has 512 128KiB big sector only.
So I leave this, and I'll modify only these comment.

     sector 1: 128 KiB - U-Boot code
     sector 2: 128 KiB - environment, first copy
     sector 3: 128 KiB - environment, second (redundant) copy
     sector 4: 128 KiB - more U-Boot code


Regards,
Atsuo Igarashi