[U-Boot] [PATCH] mgcoge, mgsuvd: added support for the IVM EEprom.

[Heiko Schocher]

Hello Wolfgang,

Wolfgang Denk wrote:
> In message <48E08A41.1090708 at denx.de> you wrote:
>> The EEprom contains some Manufacturerinformation,
>> which are read from u-boot at boot time, and saved
>> in same Environmentvars.
>>
>> Signed-off-by: Heiko Schocher <hs at denx.de>
>> ---
>>  board/keymile/common/common.c |  285 +++++++++++++++++++++++++++++++++++++++++
>>  include/configs/mgcoge.h      |    5 +
>>  include/configs/mgsuvd.h      |    5 +
>>  lib_ppc/board.c               |    6 +
>>  4 files changed, 301 insertions(+), 0 deletions(-)
>>
>> diff --git a/board/keymile/common/common.c b/board/keymile/common/common.c
>> index a6ed379..25c5900 100644
>> --- a/board/keymile/common/common.c
>> +++ b/board/keymile/common/common.c
>> @@ -38,6 +38,291 @@
>>  extern int i2c_mgsuvd_read (void);
>>  #endif
>>
>> +int ivm_calc_crc (unsigned char *buf, int len)
>> +{
>> +	const unsigned short cCrc16Table[16] = {
>> +		0x0000, 0xCC01, 0xD801, 0x1400,
>> +		0xF001, 0x3C00, 0x2800, 0xE401,
>> +		0xA001, 0x6C00, 0x7800, 0xB401,
>> +		0x5000, 0x9C01, 0x8801, 0x4400};
> 
> If we need support for CRC16, it should be factored out and move to
> lib_generic.

OK.

>> +#define BTChar	unsigned char
> 
> Please get rid of this #define.

Hmm... I got a source from the Boardmanufacturer, and I wanted to stay
in sync with this code as close as possible, so this looked as the best
way for me ... this affects also the most comments from you.
What to do with such code in common?

[...]
>> +	/* IVM_MacAddress */
>> +	sprintf ((char *)valbuf, "%02X:%02X:%02X:%02X:%02X:%02X",
>> +			buf[1],
>> +			buf[2],
>> +			buf[3],
>> +			buf[4],
>> +			buf[5],
>> +			buf[6]);	
>> +	setenv ("IVM_MacAddress", (char *)valbuf);
> 
> Do we really need this? We already have "ethaddr" ?

We dont need this, but the manufacturer of the Hardware
wants explicitly such a Environmentvariable.

>> +	if (getenv ("ethaddr") == NULL)
>> +		setenv ((char *)"ethaddr", (char *)valbuf);
> 
> Oops? You are even aware of this.

Yes.

> Please avoid duplicate data.

Hmm... the boardmanufacturer wants explicitly a "IVM_" before
all of "his" variables from the EEprom.

[...]
>> +	unsigned short	val;
>> +	unsigned char	valbuf[CFG_IVM_EEPROM_PAGE_LEN];
>> +	unsigned char	*tmp;
>> +
>> +	/* used Code from ivm/src/devsrv_baseproductdata.cpp */
> 
> What is ivm/src/devsrv_baseproductdata.cpp ?

Thats the source from the boardmanufacturer ...

>> +	ivm_get_value (buf, CFG_IVM_EEPROM_PAGE_LEN, "IVM_BoardId", 0, 1);
>> +	val = ivm_get_value (buf, CFG_IVM_EEPROM_PAGE_LEN, "IVM_HWKey", 6, 1);
>> +	if (val != 0xffff) {
>> +		sprintf ((char *)valbuf, "%x", ((val /100) % 10));
>> +		setenv ("IVM_HWVariant", (char *)valbuf);
>> +		sprintf ((char *)valbuf, "%x", (val % 100));
>> +		setenv ("IVM_HWVersion", (char *)valbuf);
>> +	}
> 
> You're really polluting the environment here.

I know.

> Is this reasonable?

Hmm... I think so, because the manufacturer wants all of this vars, and
if I am right, you or Detlev arranged it.

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany