From mboxrd@z Thu Jan 1 00:00:00 1970 From: Baruch Siach Date: Tue, 14 Jan 2020 12:18:39 +0200 Subject: [PATCH 03/10] cmd: add sys_eeprom command In-Reply-To: References: Message-ID: <20200114101839.5xj32u4wdtfriebd@sapphire.tkos.co.il> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > Shouldn't you add the copyrights from the original patch: > > Add to support 'sys_eeprom' command (common part) > > Copyright (C) 2013 Curt Brune > Copyright (C) 2014 Srideep > Copyright (C) 2013 Miles Tseng > Copyright (C) 2014,2016 david_yang > > ? 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#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 -