[PATCH 03/10] cmd: add sys_eeprom command

[Stefan Roese]

Hi Baruch,

On 14.01.20 11:18, Baruch Siach wrote:
> Hi Stefan,
> 
> On Mon, Jan 13, 2020 at 08:11:05AM +0100, Stefan Roese wrote:
>> On 25.11.19 11:30, Baruch Siach wrote:
>>> Based on U-Boot patch from the Open Compute project:
>>>
>>>     https://github.com/opencomputeproject/onie/blob/ec87e872d46b9805565d2c6124b2f701ef1c07b1/patches/u-boot/common/feature-sys-eeprom-tlv-common.patch
>>>
>>> Keep only I2C EEPROM support. Use the generic eeprom driver.
>>>
>>> Add support for multiple EEPROM TLV stores on the same system.
>>
>> Could you please explain, what TLV stands for? Is it "Type Length
>> Value"? Please add it to the description.
> 
> Will do.
> 
>>> This is
>>> useful in case of SOM and carrier that both provide ID and hardware
>>> configuration information.
>>>
>>> Add option to enable for SPL. This allows selection of RAM configuration
>>> based on EEPROM stored board identification.
>>>
>>> Signed-off-by: Baruch Siach <baruch at tkos.co.il>
>>
>> Shouldn't you add the copyrights from the original patch:
>>
>> Add to support 'sys_eeprom' command (common part)
>>
>> Copyright (C) 2013 Curt Brune <curt at cumulusnetworks.com>
>> Copyright (C) 2014 Srideep <srideep_devireddy at dell.com>
>> Copyright (C) 2013 Miles Tseng <miles_tseng at accton.com>
>> Copyright (C) 2014,2016 david_yang <david_yang at accton.com>
>>
>> ?
> 
> Copyright information is not usually listed in commit log messages. Is there
> any reason to do that in this patch?

No. I meant putting it into the source code file instead.
  
>>> ---
>>>    cmd/Kconfig          |   12 +
>>>    cmd/Makefile         |    2 +
>>>    cmd/sys_eeprom.c     | 1078 ++++++++++++++++++++++++++++++++++++++++++
>>>    include/sys_eeprom.h |  169 +++++++
>>>    4 files changed, 1261 insertions(+)
>>>    create mode 100644 cmd/sys_eeprom.c
>>>    create mode 100644 include/sys_eeprom.h
>>>
>>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>>> index 99b8a0e21822..483663404da4 100644
>>> --- a/cmd/Kconfig
>>> +++ b/cmd/Kconfig
>>> @@ -234,6 +234,18 @@ config CMD_REGINFO
>>>    	help
>>>    	  Register dump
>>> +config CMD_SYS_EEPROM
>>> +	bool "sys_eeprom"
>>> +	depends on I2C_EEPROM
>>> +	help
>>> +	  Display and program the system EEPROM data block.
>>> +
>>> +config SPL_CMD_SYS_EEPROM
>>> +	bool "sys_eeprom for SPL"
>>> +	depends on SPL_I2C_EEPROM
>>> +	help
>>> +	  Read system EEPROM data block.
>>> +
>>
>> I'm not sure, if this description is enough. This sounds too generic
>> for my taste as I assume that there are multiple formats to store system
>> data in an EEPROM. It might make sense to mention TLV here. And perhaps
>> its also better to name the files differently by adding "tlv" as well.
>>
>> What do you think?
> 
> Sounds reasonable. I'll rename to something less generic.

Thanks.
  
>>>    endmenu
>>>    menu "Boot commands"
>>> diff --git a/cmd/Makefile b/cmd/Makefile
>>> index 2d723ea0f07d..cbb1d46b8f50 100644
>>> --- a/cmd/Makefile
>>> +++ b/cmd/Makefile
>>> @@ -180,6 +180,8 @@ obj-$(CONFIG_X86) += x86/
>>>    obj-$(CONFIG_ARCH_MVEBU) += mvebu/
>>>    endif # !CONFIG_SPL_BUILD
>>> +obj-$(CONFIG_$(SPL_)CMD_SYS_EEPROM) += sys_eeprom.o
>>> +
>>>    # core command
>>>    obj-y += nvedit.o
>>> diff --git a/cmd/sys_eeprom.c b/cmd/sys_eeprom.c
>>> new file mode 100644
>>> index 000000000000..373673a5266c
>>> --- /dev/null
>>> +++ b/cmd/sys_eeprom.c
>>> @@ -0,0 +1,1078 @@
>>> +/*
>>> + * See file CREDITS for list of people who contributed to this
>>> + * project.
>>> + *
>>> + * SPDX-License-Identifier:	GPL-2.0+
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <command.h>
>>> +#include <dm.h>
>>> +#include <i2c.h>
>>> +#include <i2c_eeprom.h>
>>> +#include <env.h>
>>> +#include <linux/ctype.h>
>>> +
>>> +#include "sys_eeprom.h"
>>> +
>>> +#define MAX_TLV_DEVICES	2
>>> +
>>> +/* File scope function prototypes */
>>> +static bool is_checksum_valid(u8 *eeprom);
>>> +static int read_eeprom(u8 *eeprom);
>>> +static void show_eeprom(u8 *eeprom);
>>> +static void decode_tlv(tlvinfo_tlv_t * tlv);
>>> +static void update_crc(u8 *eeprom);
>>> +static int prog_eeprom(u8 * eeprom);
>>> +static bool tlvinfo_find_tlv(u8 * eeprom, u8 tcode, int * eeprom_index);
>>> +static bool tlvinfo_delete_tlv(u8 * eeprom, u8 code);
>>> +static bool tlvinfo_add_tlv(u8 * eeprom, int tcode, char * strval);
>>> +static int set_mac(char *buf, const char *string);
>>> +static int set_date(char *buf, const char *string);
>>> +static int set_bytes(char *buf, const char *string, int * converted_accum);
>>> +static void show_tlv_devices(void);
>>> +
>>> +/* Set to 1 if we've read EEPROM into memory */
>>> +static int has_been_read = 0;
>>> +/* The EERPOM contents after being read into memory */
>>> +static u8 eeprom[TLV_INFO_MAX_LEN];
>>> +
>>> +static struct udevice *tlv_devices[MAX_TLV_DEVICES];
>>> +static unsigned current_dev;
>>> +
>>> +/**
>>> + *  is_valid_tlv
>>> + *
>>> + *  Perform basic sanity checks on a TLV field. The TLV is pointed to
>>> + *  by the parameter provided.
>>> + *      1. The type code is not reserved (0x00 or 0xFF)
>>> + */
>>> +static inline bool is_valid_tlv(tlvinfo_tlv_t *tlv)
>>> +{
>>> +	return( (tlv->type != 0x00) &&
>>> +		(tlv->type != 0xFF) );
>>
>> This will generate checkpatch errors. I know that you copied this code
>> but still we should not add code that is not checkpatch clean.
>>
>> Please rework this file so that it at least matches the basic U-Boot
>> coding style rules.
>>
>> I'm stopping my review here for now and will review the new version.
> 
> Thanks for your review so far. I'll update and resend.

Good.

Thanks,
Stefan