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

[Wolfgang Denk <wd@denx.de>]

Dear Christophe,

In message <20170706144516.9DF0269745@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@c-s.fr>
> ---
>  v3: Takes into account comments received from Heiko and Wolfgang

Thanks.

> +/* -------------------------------------------------------------------------
> + * Device Tree Support
> + */

Incorrect multiline comment style...  Please fix globally.

> +#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.

> +#define ADDR_FLASH_ENV_AREA	((unsigned short *)(ADDR_FLASH + 0x40000))

Is this not a redundant setting?   Can you not use CONFIG_ENV_ADDR
instead?

> +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...

> +	/* 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...



Reviewed-by: Wolfgang Denk <wd@denx.de>


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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
"Everybody is talking about the  weather  but  nobody  does  anything
about it."                                               - Mark Twain