Dear Christophe, In message <20170706144516.9df0269...@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.le...@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 <w...@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: w...@denx.de "Everybody is talking about the weather but nobody does anything about it." - Mark Twain _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot