[PATCH 03/10] cmd: add sys_eeprom command

Tue Jan 14 11:18:39 CET 2020

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?

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

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

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -