[U-Boot] [PATCH v3] powerpc, 8xx: Add support for MCR3000 board from CSSI

[Christophe LEROY]

Dear Wolfgang,

Le 06/07/2017 à 17:23, Wolfgang Denk a écrit :
> Dear Christophe,
> 
> In message <20170706144516.9DF0269745 at pc13941vm.idsi0.si.c-s.fr> you wrote:
>> CS Systemes d'Information (CSSI) manufactures two boards, named MCR3000
>> and CMPC885 which are respectively based on MPC866 and MPC885 processors.
>>
>> This patch adds support for the first board.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy at c-s.fr>
>> ---
>>   v3: Takes into account comments received from Heiko and Wolfgang
> 
> Thanks.
> 
>> +/* -------------------------------------------------------------------------
>> + * Device Tree Support
>> + */
> 
> Incorrect multiline comment style...  Please fix globally.

Oops, I only fixed the ones in the .h
Done everywhere now.

> 
>> +#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_OF_LIBFDT)
>> +static int fdt_set_node_and_value(void *blob, char *nodename, char *regname,
>> +				  void *var, int size)
> 
> This is apparently ancient code.  No other board does it this way
> any more.  It seems do_fixup_by_path_*() is preferred now.

Ok

> 
>> +#define ADDR_FLASH_ENV_AREA	((unsigned short *)(ADDR_FLASH + 0x40000))
> 
> Is this not a redundant setting?   Can you not use CONFIG_ENV_ADDR
> instead?

Yes you are right, fixed it.

> 
>> +struct environment {
>> +	uint32_t	crc;
>> +	uint8_t		data[];
>> +};
> 
> This is a (incomplete) redefine of struct environment_s.  Why cannot
> you use that?
> 
>> +	/* verifying environment variable area */
>> +	env = (struct environment *)CONFIG_ENV_ADDR;
>> +	crc = crc32(0, env->data, (CONFIG_ENV_SIZE - sizeof(uint32_t)));
>> +	if (crc != env->crc) {
>> +		/* It can be an update request */
>> +		for (i = 0; i < 8; i++) {
>> +			str[i] = *(((uint8_t *)CONFIG_ENV_ADDR) + i);
>> +			str[(i+1)] = 0;
>> +		}
>> +		if (strcmp(str, "ETHADDR=") == 0) {
>> +			/* getting saved value */
>> +			i = 0;
>> +			for (j = 0; j < 4; j++) {
>> +				while (*(((uint8_t *)CONFIG_ENV_ADDR) + i) !=
>> +				       '=')
>> +					i++;
>> +				switch (j) {
>> +				case 0:
>> +					s = s_ethaddr;
>> +					break;
>> +				case 1:
>> +					s = s_num_serie;
>> +					break;
>> +				case 2:
>> +					s = s_id_cpld;
>> +					break;
>> +				case 3:
>> +					s = s_password;
>> +					break;
>> +				}
>> +				do {
>> +					i++;
>> +					*s = *(((uint8_t *)CONFIG_ENV_ADDR) + i);
>> +					s++;
>> +				} while (*(((uint8_t *)CONFIG_ENV_ADDR) + i)
>> +					 != 0x00);
>> +			}
>> +
>> +			/* creating or updating environment variable */
>> +			if (s_ethaddr[0] != 0x00)
>> +				setenv("ethaddr", s_ethaddr);
>> +			if (s_num_serie[0] != 0x00)
>> +				setenv("num_serie", s_num_serie);
>> +			if (s_id_cpld[0] != 0x00)
>> +				setenv("id_cpld", s_id_cpld);
>> +			if (s_password[0] != 0x00)
>> +				setenv("password", s_password);
>> +			saveenv();
>> +		}
>> +	}
> 
> This code is pretty scary...

Ok, I remove it for the time being, this code is not necessary for 
starting the board. Will fix that later in a more correct way.

> 
>> +	/* we do not modify environment variable area if CRC is false */
>> +	crc = crc32(0, env->data, (CONFIG_ENV_SIZE - sizeof(uint32_t)));
>> +	if (crc == env->crc) {
>> +		/* getting version value in CPLD register */
>> +		for (i = 0; i < LEN_STR; i++)
>> +			str[i] = 0;
>> +		version_cpld = *ADDR_CPLD_R_ETAT & R_ID_CPLD_MASK;
>> +		if (((version_cpld >> 12) & 0x000f) < 0x000a)
>> +			str[0] = ((version_cpld >> 12) & 0x000f) + 0x30;
>> +		else
>> +			str[0] = (((version_cpld >> 12) & 0x000f) - 0x000a) +
>> +				 0x41;
>> +		if (((version_cpld >> 8) & 0x000f) < 0x000a)
>> +			str[1] = ((version_cpld >> 8) & 0x000f) + 0x30;
>> +		else
>> +			str[1] = (((version_cpld >> 8) & 0x000f) - 0x000a) +
>> +				 0x41;
>> +		str[2] = 0x30;
>> +		str[3] = 0x30;
>> +
>> +		/* updating "id_cpld" variable if not corresponding */
>> +		s = getenv("id_cpld");
>> +		if ((s == NULL) || (strcmp(s, str) != 0)) {
>> +			setenv("id_cpld", str);
>> +			saveenv();
> 
> And again comment (we do not modify environment variable area) and
> code (call to saveenv()) do not agree.
> 
> This code is giving me the creeps...

Ok, removed (see above).

> 
> 
> 
> Reviewed-by: Wolfgang Denk <wd at denx.de>
> 
> 
> Best regards,
> 
> Wolfgang Denk
> 

Best regards
Christophe