[U-Boot] [PATCH 3/3] mips: mt76xx: gardena-smart-gateway: Add factory data variable handling

[Stefan Roese]

On 28.11.18 23:41, Daniel Schwierzeck wrote:
> 
> 
> Am 28.11.18 um 08:40 schrieb Stefan Roese:
>> Some factory data is stored in the SPI NOR and needs to get extracted
>> from there into U-Boot environment variables.
>>
>> This patch also includes a board-specific command "fd_write" to
>> provide some dummy / default values for this factory-data in the SPI
>> NOR flash. This should only be necessary for testing purposes though.
>>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
>> ---
>>   board/gardena/smart-gateway-mt7688/board.c | 266 +++++++++++++++++++++
>>   1 file changed, 266 insertions(+)
> 
> please send a v2 only of this patch. I already applied the other two
> patches. Thanks.

v2 will follow shortly.
  
>>
>> diff --git a/board/gardena/smart-gateway-mt7688/board.c b/board/gardena/smart-gateway-mt7688/board.c
>> index 3a1838e44a..185bdfb71d 100644
>> --- a/board/gardena/smart-gateway-mt7688/board.c
>> +++ b/board/gardena/smart-gateway-mt7688/board.c
>> @@ -4,11 +4,44 @@
>>    */
>>   
>>   #include <common.h>
>> +#include <environment.h>
>>   #include <led.h>
>> +#include <net.h>
>> +#include <spi.h>
>> +#include <spi_flash.h>
>> +#include <uuid.h>
>> +#include <linux/ctype.h>
>>   #include <linux/io.h>
>>   
>>   #define MT76XX_AGPIO_CFG	0x1000003c
>>   
>> +#define FACTORY_DATA_OFFS	0xc0000
>> +#define FACTORY_DATA_SECT_SIZE	0x10000
>> +#if ((CONFIG_ENV_OFFSET_REDUND + CONFIG_ENV_SIZE_REDUND) > FACTORY_DATA_OFFS)
>> +#error "U-Boot image with environment too big (overlapping with factory-data)!"
>> +#endif
>> +#define FACTORY_DATA_USER_OFFS	0x140
>> +#define FACTORY_DATA_SIZE	0x1f0
>> +#define FACTORY_DATA_CRC_LEN	(FACTORY_DATA_SIZE -			\
>> +				 FACTORY_DATA_USER_OFFS - sizeof(u32))
>> +
>> +#define FACTORY_DATA_MAGIC	0xCAFEBABE
>> +
>> +struct factory_data_values {
>> +	u8 pad_1[4];
>> +	u8 wifi_mac[6];		/* offs: 0x004: binary value */
>> +	u8 pad_2[30];
>> +	u8 eth_mac[6];		/* offs: 0x028: binary value */
>> +	u8 pad_3[FACTORY_DATA_USER_OFFS - 4 - 6 - 30 - 6];
>> +	/* User values start here at offset 0x140 */
>> +	u32 crc;
>> +	u32 magic;
>> +	u32 version;
>> +	char ipr_id[UUID_STR_LEN];	/* UUID as string w/o ending \0 */
>> +	char hqv_id[UUID_STR_LEN];	/* UUID as string w/o ending \0 */
>> +	char unielec_id[UUID_STR_LEN];	/* UUID as string w/o ending \0 */
>> +};
>> +
>>   int board_early_init_f(void)
>>   {
>>   	void __iomem *gpio_mode;
>> @@ -20,10 +53,243 @@ int board_early_init_f(void)
>>   	return 0;
>>   }
>>   
>> +static int prepare_uuid_var(char *fd_ptr, char *env_var_name, char errorchar)
> 
> const for the char pointers and bool as return value?

Ack.
  
>> +{
>> +	char str[UUID_STR_LEN + 1];	/* Enough for UUID stuff */
>> +	bool env_updated = 0;
>> +	char *env;
>> +	int i;
>> +
>> +	memset(str, 0, sizeof(str));
>> +	memcpy(str, fd_ptr, UUID_STR_LEN);
> 
> you can avoid the memset() with str[UUID_STR_LEN] = 0;

Ack.
  
>> +
>> +	/* Convert non-ascii character to 'X' */
>> +	for (i = 0; i < UUID_STR_LEN; i++) {
>> +		if (!(isascii(str[i]) && isprint(str[i])))
>> +			str[i] = errorchar;
> 
> can it happen that the string value is shorter than UUID_STR_LEN and is
> zero-terminated? If yes is it intented to fill the remaining characters
> with errorchar?

The string might be shorter and we intentionally want to fill the string
with these chars. This makes not that much sense with '\0', but it
simplifies the code this way.

>> +	}
>> +
>> +	env = env_get(env_var_name);
>> +	if (strcmp(env, str)) {
>> +		env_set(env_var_name, str);
>> +		env_updated = 1;
>> +	}
>> +
>> +	return env_updated;
>> +}
>> +
>> +static void factory_data_env_config(void)
>> +{
>> +	struct factory_data_values *fd;
>> +	struct spi_flash *sf;
>> +	int env_updated = 0;
>> +	char str[UUID_STR_LEN + 1];	/* Enough for UUID stuff */
>> +	char *env;
>> +	u8 *buf;
>> +	u32 crc;
>> +	int ret;
>> +	u8 *ptr;
>> +
>> +	buf = malloc(FACTORY_DATA_SIZE);
>> +	if (!buf) {
>> +		printf("F-Data:Unable to allocate buffer\n");
>> +		goto err;
> 
> this jump to 'err' is incorrect as you would free a NULL poinzer

Ack.

Thanks for the review. I'll change all the other issues as well for
v2.

Thanks,
Stefan