Dear Wolfgang,
Le 06/07/2017 à 17:23, Wolfgang Denk a écrit :
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.
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;elsestr[0] = (((version_cpld >> 12) & 0x000f) - 0x000a) +0x41;if (((version_cpld >> 8) & 0x000f) < 0x000a)str[1] = ((version_cpld >> 8) & 0x000f) + 0x30;elsestr[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@denx.de
Best regards,
Wolfgang Denk
Best regards Christophe