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 <bar...@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 <c...@cumulusnetworks.com>
Copyright (C) 2014 Srideep <srideep_devire...@dell.com>
Copyright (C) 2013 Miles Tseng <miles_ts...@accton.com>
Copyright (C) 2014,2016 david_yang <david_y...@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
