[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Jerry Van Baren]

Joakim Tjernlund wrote:
> On Tue, 2006-11-28 at 16:16 -0600, Timur Tabi wrote:
>> Joakim Tjernlund wrote:
>>
>>> No, see attached patch(s)
>> Ah, I see.
>>
>>> Not tested in your tree as I don't use that one (yet)
>> Git didn't like your patches, for some reason, so I had to apply them by hand, 
>> but everything seems to be okay.  I will apply them to our tree for Wolfgang's 
>> convenience.
>>
> 
> While I am at it, I would also like to see this in u-boot
> We use I2C as HRCW since we wan't to haw our flash reset connetced to
> HRESET, otherwise you might be unable to boot if the flash is in non
> read array mode when the board resets.
> We also need to have the version info in the begining of the flash so
> we can identify what version of u-boot we have installed. 
> 
> ------------------------------------------------------------------------
> 
> From 89b60f21af0d04959d93ccb70fd781c8aba9e66c Mon Sep 17 00:00:00 2001
> From: Joakim Tjernlund <Joakim.Tjernlund at transmode.se>
> Date: Tue, 28 Nov 2006 23:42:31 +0100
> Subject: [PATCH] Make HRCW and version info in data segment configurable.
> 
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund at transmode.se>
> ---
>  cpu/mpc83xx/start.S |   28 +++++++++++++++++-----------
>  1 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/cpu/mpc83xx/start.S b/cpu/mpc83xx/start.S
> index 0f27bb6..44bca26 100644
> --- a/cpu/mpc83xx/start.S
> +++ b/cpu/mpc83xx/start.S
> @@ -77,20 +77,12 @@
>  	END_GOT
>  
>  /*
> - * Version string - must be in data segment because MPC83xx uses the
> - * first 256 bytes for the Hard Reset Configuration Word table (see
> - * below).  Similarly, can't have the U-Boot Magic Number as the first
> - * thing in the image - don't know how this will affect the image tools,
> - * but I guess I'll find out soon.
> + * MPC83xx can use the first 0x40 bytes for the Hard Reset Configuration Word 
> + * table (see below) if so configured.
>   */
> -	.data
> -	.globl	version_string
> -version_string:
> -	.ascii U_BOOT_VERSION
> -	.ascii " (", __DATE__, " - ", __TIME__, ")"
> -	.ascii " ", CONFIG_IDENT_STRING, "\0"
>  
>  	.text
> +#ifndef CFG_HRCW_IN_I2C_EEPROM
>  #define _HRCW_TABLE_ENTRY(w)		\
>  	.fill	8,1,(((w)>>24)&0xff);	\
>  	.fill	8,1,(((w)>>16)&0xff);	\
> @@ -99,7 +91,21 @@ version_string:
>  
>  	_HRCW_TABLE_ENTRY(CFG_HRCW_LOW)
>  	_HRCW_TABLE_ENTRY(CFG_HRCW_HIGH)
> +#endif
>  
> +/*
> + * Version string - May be in data segment if one wants to reserve the
> + * space left to address 0x100 for future expansion of HRCW bytes.
> + */
> +#ifdef CFG_VERSION_STRING_IN_DATA
> +	.data
> +#endif
> +        .long   0x27051956              /* U-Boot Magic Number */
> +	.globl	version_string
> +version_string:
> +	.ascii U_BOOT_VERSION
> +	.ascii " (", __DATE__, " - ", __TIME__, ")"
> +	.ascii " ", CONFIG_IDENT_STRING, "\0"
>  
>  #ifndef CONFIG_DEFAULT_IMMR
>  #error CONFIG_DEFAULT_IMMR must be defined

Hi Timur,

I don't believe you want to do this: you are removing the HRCW entirely 
from flash if you have it in I2C (CFG_HRCW_IN_I2C_EEPROM).  Unless I'm 
missing something (possible), I don't see why you even need a 
configuration option CFG_HRCW_IN_I2C_EEPROM.

* You can chose to ignore the flash-HRCW and use the I2C one simply by 
pulling a config pin up/down on power up (if I understand the manuals 
correctly), so you can store your HRCW in both places and chose which 
you use via a hardware configuration jumper.  This is a Very Good 
Thing[tm].  Suppressing the flash version simply because you chose not 
to use it is not something _I_ want to do on my 8360.

* If you put your HRCW in I2C _and_ in flash, you will be able to boot 
via the flash HRCW if your I2C version gets hosed.  If you don't, you 
will be hosed.

* Having a redundant HRCW in flash is essentially free: it costs you 64 
bytes of otherwise unused memory (well, other than the version string 
which you moved there, but there should be room for both).

My suggestion for putting the u-boot version string in a fixed place in 
flash was to put it _after_ the HRCW (e.g. at offset 0x40).  0x60 (96) 
bytes should be plenty for the version string.  Instead, you've unwisely 
(IMHO) _replaced_ the HRCW.  Browsing some of the other PowerPC CPU 
types, the ones that can appear to put the version string at the very 
start of flash.  This implies putting the version string in flash at 
offset 0x40 is OK (I was concerned about relocation fixup issues with 
pointers to flash vs. RAM after relocation).

On a related topic, the comment at the head of start.S says
/*
  * Version string - must be in data segment because MPC83xx uses the
  * first 256 bytes for the Hard Reset Configuration Word table (see
  * below).  Similarly, can't have the U-Boot Magic Number as the first
  * thing in the image - don't know how this will affect the image tools,
  * but I guess I'll find out soon.
  */
I see the comment was cut'n'pasted from the 8260 start.S.  Actually only 
the first 64 (0x40) bytes of memory are used for the HRCW, _not_ all 256 
bytes.

Looks like I (or somebody) needs to submit a clean up patch for that 
comment for the 8260 and 8360.  Sigh.  I just _had_ to look.  Curiosity 
will be the death of me yet.  ;-)

gvb