Merge patch series "'eeprom' command improvements"

Marek Behún <kabel@kernel.org> says:

This series contains improvements for the 'eeprom' command:
- refactors
- fixes
- improvements
- ability to use driver model EEPROMs (uclass UCLASS_I2C_EEPROM)
- more flexible EEPROM layout support

It should not cause any behavior change for any existing board.

This series is a dependency for some DDR issue fixes for Turris Omnia.
I will be sending that one separately.

github PR link (with CI):
  https://github.com/u-boot/u-boot/pull/540
- there is a failure for
    test.py for sandbox sandbox_clang
  but it seems unrelated to these changes

cmd/Kconfig

@@ -804,9 +804,16 @@ config CMD_EEPROM_LAYOUT
 			types of eeprom fields. Can be used for defining
 			custom layouts.
 
+config EEPROM_LAYOUT_VERSIONS
+	bool "Support specifying eeprom layout version"
+	depends on CMD_EEPROM_LAYOUT
+	help
+	  Support specifying eeprom layout version in the 'eeprom' command
+	  via the -l option.
+
 config EEPROM_LAYOUT_HELP_STRING
 	  string "Tells user what layout names are supported"
-	  depends on CMD_EEPROM_LAYOUT
+	  depends on EEPROM_LAYOUT_VERSIONS
 	  default "<not defined>"
 	  help
 	    Help printed with the LAYOUT VERSIONS part of the 'eeprom'

cmd/eeprom.c

@@ -21,8 +21,10 @@
 
 #include <config.h>
 #include <command.h>
+#include <dm.h>
 #include <eeprom.h>
 #include <i2c.h>
+#include <i2c_eeprom.h>
 #include <eeprom_layout.h>
 #include <vsprintf.h>
 #include <linux/delay.h>
@@ -208,41 +210,95 @@ static long parse_numeric_param(char *str)
 	return (*endptr != '\0') ? -1 : value;
 }
 
+struct eeprom_dev_spec {
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	struct udevice *dev;
+#endif
+	int i2c_bus;
+	ulong i2c_addr;
+};
+
+static void eeprom_dev_spec_init(struct eeprom_dev_spec *dev)
+{
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	if (!dev->dev)
+#endif
+		eeprom_init(dev->i2c_bus);
+}
+
+static int eeprom_dev_spec_read(struct eeprom_dev_spec *dev,
+				unsigned offset, uchar *buffer, unsigned cnt)
+{
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	if (dev->dev)
+		return i2c_eeprom_read(dev->dev, offset, buffer, cnt);
+#endif
+	return eeprom_read(dev->i2c_addr, offset, buffer, cnt);
+}
+
+static int eeprom_dev_spec_write(struct eeprom_dev_spec *dev,
+				 unsigned offset, uchar *buffer, unsigned cnt)
+{
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	if (dev->dev)
+		return i2c_eeprom_write(dev->dev, offset, buffer, cnt);
+#endif
+	return eeprom_write(dev->i2c_addr, offset, buffer, cnt);
+}
+
 /**
- * parse_i2c_bus_addr - parse the i2c bus and i2c devaddr parameters
+ * parse_eeprom_dev_spec - parse the eeprom device specifier
  *
- * @i2c_bus:	address to store the i2c bus
+ * @dev:	pointer to eeprom device specifier
- * @i2c_addr:	address to store the device i2c address
+ * @argc:	count of command line arguments that can be used to parse
- * @argc:	count of command line arguments left to parse
+ *		the device specifier
  * @argv:	command line arguments left to parse
- * @argc_no_bus_addr:	argc value we expect to see when bus & addr aren't given
  *
  * @returns:	number of arguments parsed or CMD_RET_USAGE if error
  */
-static int parse_i2c_bus_addr(int *i2c_bus, ulong *i2c_addr, int argc,
+static int parse_eeprom_dev_spec(struct eeprom_dev_spec *dev, int argc,
-			      char *const argv[], int argc_no_bus_addr)
+				 char *const argv[])
 {
-	int argc_no_bus = argc_no_bus_addr + 1;
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
-	int argc_bus_addr = argc_no_bus_addr + 2;
+	if (argc == 0) {
+		if (!uclass_first_device_err(UCLASS_I2C_EEPROM, &dev->dev))
+			return 0;
+	}
+
+	if (argc == 1) {
+		if (!uclass_get_device_by_name(UCLASS_I2C_EEPROM, argv[0],
+					       &dev->dev))
+			return 1;
+
+		/*
+		 * If we could not find the device by name and the parameter is
+		 * not numeric (and so won't be handled later), fail.
+		 */
+		if (parse_numeric_param(argv[0]) == -1) {
+			printf("Can't get eeprom device: %s\n", argv[0]);
+			return CMD_RET_USAGE;
+		}
+	}
+#endif
 
 #ifdef CONFIG_SYS_I2C_EEPROM_ADDR
-	if (argc == argc_no_bus_addr) {
+	if (argc == 0) {
-		*i2c_bus = -1;
+		dev->i2c_bus = -1;
-		*i2c_addr = CONFIG_SYS_I2C_EEPROM_ADDR;
+		dev->i2c_addr = CONFIG_SYS_I2C_EEPROM_ADDR;
 
 		return 0;
 	}
 #endif
-	if (argc == argc_no_bus) {
+	if (argc == 1) {
-		*i2c_bus = -1;
+		dev->i2c_bus = -1;
-		*i2c_addr = parse_numeric_param(argv[0]);
+		dev->i2c_addr = parse_numeric_param(argv[0]);
 
 		return 1;
 	}
 
-	if (argc == argc_bus_addr) {
+	if (argc == 2) {
-		*i2c_bus = parse_numeric_param(argv[0]);
+		dev->i2c_bus = parse_numeric_param(argv[0]);
-		*i2c_addr = parse_numeric_param(argv[1]);
+		dev->i2c_addr = parse_numeric_param(argv[1]);
 
 		return 2;
 	}
@@ -252,16 +308,19 @@ static int parse_i2c_bus_addr(int *i2c_bus, ulong *i2c_addr, int argc,
 
 #ifdef CONFIG_CMD_EEPROM_LAYOUT
 
+#ifdef CONFIG_EEPROM_LAYOUT_VERSIONS
 __weak int eeprom_parse_layout_version(char *str)
 {
 	return LAYOUT_VERSION_UNRECOGNIZED;
 }
+#endif
 
 static unsigned char eeprom_buf[CONFIG_SYS_EEPROM_SIZE];
 
 #endif
 
 enum eeprom_action {
+	EEPROM_LIST,
 	EEPROM_READ,
 	EEPROM_WRITE,
 	EEPROM_PRINT,
@@ -271,6 +330,10 @@ enum eeprom_action {
 
 static enum eeprom_action parse_action(char *cmd)
 {
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	if (!strncmp(cmd, "list", 4))
+		return EEPROM_LIST;
+#endif
 	if (!strncmp(cmd, "read", 4))
 		return EEPROM_READ;
 	if (!strncmp(cmd, "write", 5))
@@ -285,68 +348,115 @@ static enum eeprom_action parse_action(char *cmd)
 	return EEPROM_ACTION_INVALID;
 }
 
-static int eeprom_execute_command(enum eeprom_action action, int i2c_bus,
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
-				  ulong i2c_addr, int layout_ver, char *key,
+static int do_eeprom_list(void)
-				  char *value, ulong addr, ulong off, ulong cnt)
+{
+	struct udevice *dev;
+	struct uclass *uc;
+	int err;
+
+	err = uclass_get(UCLASS_I2C_EEPROM, &uc);
+	if (err)
+		return CMD_RET_FAILURE;
+
+	uclass_foreach_dev(dev, uc)
+		printf("%s (%s)\n", dev->name, dev->driver->name);
+
+	return CMD_RET_SUCCESS;
+}
+#endif
+
+static int do_eeprom_rw(struct eeprom_dev_spec *dev, bool read,
+			ulong addr, ulong off, ulong cnt)
 {
-	int rcode = 0;
 	const char *const fmt =
 		"\nEEPROM @0x%lX %s: addr 0x%08lx  off 0x%04lx  count %ld ... ";
-#ifdef CONFIG_CMD_EEPROM_LAYOUT
+	uchar *memloc = (uchar *)addr;
-	struct eeprom_layout layout;
+	int ret;
-#endif
 
-	if (action == EEPROM_ACTION_INVALID)
+	printf(fmt, dev->i2c_addr, read ? "read" : "write", addr, off, cnt);
-		return CMD_RET_USAGE;
+	if (read)
+		ret = eeprom_dev_spec_read(dev, off, memloc, cnt);
+	else
+		ret = eeprom_dev_spec_write(dev, off, memloc, cnt);
+	puts("done\n");
 
-	eeprom_init(i2c_bus);
+	return ret;
-	if (action == EEPROM_READ) {
+}
-		printf(fmt, i2c_addr, "read", addr, off, cnt);
-
-		rcode = eeprom_read(i2c_addr, off, (uchar *)addr, cnt);
-
-		puts("done\n");
-		return rcode;
-	} else if (action == EEPROM_WRITE) {
-		printf(fmt, i2c_addr, "write", addr, off, cnt);
-
-		rcode = eeprom_write(i2c_addr, off, (uchar *)addr, cnt);
-
-		puts("done\n");
-		return rcode;
-	}
 
 #ifdef CONFIG_CMD_EEPROM_LAYOUT
-	rcode = eeprom_read(i2c_addr, 0, eeprom_buf, CONFIG_SYS_EEPROM_SIZE);
-	if (rcode < 0)
-		return rcode;
 
-	eeprom_layout_setup(&layout, eeprom_buf, CONFIG_SYS_EEPROM_SIZE,
+static int do_eeprom_layout(struct eeprom_dev_spec *dev, int layout_ver,
+			    struct eeprom_layout *layout)
+{
+	eeprom_layout_setup(layout, eeprom_buf, CONFIG_SYS_EEPROM_SIZE,
 			    layout_ver);
 
-	if (action == EEPROM_PRINT) {
+	return eeprom_dev_spec_read(dev, 0, eeprom_buf, layout->data_size);
-		layout.print(&layout);
+}
-		return 0;
-	}
 
-	layout.update(&layout, key, value);
+static int do_eeprom_print(struct eeprom_dev_spec *dev, int layout_ver)
+{
+	struct eeprom_layout layout;
+	int ret;
+
+	ret = do_eeprom_layout(dev, layout_ver, &layout);
+	if (ret)
+		return ret;
+
+	layout.print(&layout);
+
+	return 0;
+}
+
+static int do_eeprom_update(struct eeprom_dev_spec *dev, int layout_ver,
+			    char *key, char *value)
+{
+	struct eeprom_layout layout;
+	int ret;
+
+	ret = do_eeprom_layout(dev, layout_ver, &layout);
+	if (ret)
+		return ret;
+
+	ret = layout.update(&layout, key, value);
+	if (ret)
+		return CMD_RET_FAILURE;
+
+	return eeprom_dev_spec_write(dev, 0, layout.data, layout.data_size);
+}
 
-	rcode = eeprom_write(i2c_addr, 0, layout.data, CONFIG_SYS_EEPROM_SIZE);
 #endif
 
-	return rcode;
+static int eeprom_action_expected_argc(enum eeprom_action action)
+{
+	switch (action) {
+	case EEPROM_LIST:
+		return 0;
+	case EEPROM_READ:
+	case EEPROM_WRITE:
+		return 3;
+	case EEPROM_PRINT:
+		return 0;
+	case EEPROM_UPDATE:
+		return 2;
+	default:
+		return CMD_RET_USAGE;
+	}
 }
 
 #define NEXT_PARAM(argc, index)	{ (argc)--; (index)++; }
 int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
-	int layout_ver = LAYOUT_VERSION_AUTODETECT;
 	enum eeprom_action action = EEPROM_ACTION_INVALID;
-	int i2c_bus = -1, index = 0;
+	struct eeprom_dev_spec dev;
-	ulong i2c_addr = -1, addr = 0, cnt = 0, off = 0;
+	ulong addr = 0, cnt = 0, off = 0;
-	int ret;
+	int ret, index = 0;
+#ifdef CONFIG_CMD_EEPROM_LAYOUT
 	char *field_name = "";
 	char *field_value = "";
+	int layout_ver = LAYOUT_VERSION_AUTODETECT;
+#endif
 
 	if (argc <= 1)
 		return CMD_RET_USAGE;
@@ -359,7 +469,12 @@ int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	if (action == EEPROM_ACTION_INVALID)
 		return CMD_RET_USAGE;
 
-#ifdef CONFIG_CMD_EEPROM_LAYOUT
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	if (action == EEPROM_LIST)
+		return do_eeprom_list();
+#endif
+
+#ifdef CONFIG_EEPROM_LAYOUT_VERSIONS
 	if (action == EEPROM_PRINT || action == EEPROM_UPDATE) {
 		if (!strcmp(argv[index], "-l")) {
 			NEXT_PARAM(argc, index);
@@ -369,25 +484,9 @@ int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	}
 #endif
 
-	switch (action) {
+	ret = parse_eeprom_dev_spec(&dev,
-	case EEPROM_READ:
+				    argc - eeprom_action_expected_argc(action),
-	case EEPROM_WRITE:
+				    argv + index);
-		ret = parse_i2c_bus_addr(&i2c_bus, &i2c_addr, argc,
-					 argv + index, 3);
-		break;
-	case EEPROM_PRINT:
-		ret = parse_i2c_bus_addr(&i2c_bus, &i2c_addr, argc,
-					 argv + index, 0);
-		break;
-	case EEPROM_UPDATE:
-		ret = parse_i2c_bus_addr(&i2c_bus, &i2c_addr, argc,
-					 argv + index, 2);
-		break;
-	default:
-		/* Get compiler to stop whining */
-		return CMD_RET_USAGE;
-	}
-
 	if (ret == CMD_RET_USAGE)
 		return ret;
 
@@ -411,24 +510,64 @@ int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	}
 #endif
 
-	return eeprom_execute_command(action, i2c_bus, i2c_addr, layout_ver,
+	eeprom_dev_spec_init(&dev);
-				      field_name, field_value, addr, off, cnt);
+
+	switch (action) {
+	case EEPROM_READ:
+	case EEPROM_WRITE:
+		return do_eeprom_rw(&dev, action == EEPROM_READ,
+				    addr, off, cnt);
+#ifdef CONFIG_CMD_EEPROM_LAYOUT
+	case EEPROM_PRINT:
+		return do_eeprom_print(&dev, layout_ver);
+	case EEPROM_UPDATE:
+		return do_eeprom_update(&dev, layout_ver,
+					field_name, field_value);
+#endif
+	default:
+		return CMD_RET_USAGE;
+	}
 }
 
+#ifdef CONFIG_EEPROM_LAYOUT_VERSIONS
+#define EEPROM_LAYOUT_SPEC	"[-l <layout_version>] "
+#else
+#define EEPROM_LAYOUT_SPEC	""
+#endif
+
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+# define EEPROM_DEV_SPEC	"[device_specifier]"
+#else
+# define EEPROM_DEV_SPEC	"[[bus] devaddr]"
+#endif
+
 U_BOOT_CMD(
 	eeprom,	8,	1,	do_eeprom,
 	"EEPROM sub-system",
-	"read  <bus> <devaddr> addr off cnt\n"
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
-	"eeprom write <bus> <devaddr> addr off cnt\n"
+	"list\n"
+	"eeprom "
+#endif
+	"read  " EEPROM_DEV_SPEC " addr off cnt\n"
+	"eeprom write " EEPROM_DEV_SPEC " addr off cnt\n"
 	"       - read/write `cnt' bytes from `devaddr` EEPROM at offset `off'"
 #ifdef CONFIG_CMD_EEPROM_LAYOUT
 	"\n"
-	"eeprom print [-l <layout_version>] <bus> <devaddr>\n"
+	"eeprom print " EEPROM_LAYOUT_SPEC EEPROM_DEV_SPEC "\n"
 	"       - Print layout fields and their data in human readable format\n"
-	"eeprom update [-l <layout_version>] <bus> <devaddr> field_name field_value\n"
+	"eeprom update " EEPROM_LAYOUT_SPEC EEPROM_DEV_SPEC " field_name field_value\n"
 	"       - Update a specific eeprom field with new data.\n"
-	"         The new data must be written in the same human readable format as shown by the print command.\n"
+	"         The new data must be written in the same human readable format as shown by the print command."
-	"\n"
+#endif
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	"\n\n"
+	"DEVICE SPECIFIER - the eeprom device can be specified\n"
+	"  [dev_name] - by device name (devices can listed with the eeprom list command)\n"
+	"  [[bus] devaddr] - or by I2C bus and I2C device address\n"
+	"If no device specifier is given, the first driver-model found device is used."
+#endif
+#ifdef CONFIG_EEPROM_LAYOUT_VERSIONS
+	"\n\n"
 	"LAYOUT VERSIONS\n"
 	"The -l option can be used to force the command to interpret the EEPROM data using the chosen layout.\n"
 	"If the -l option is omitted, the command will auto detect the layout based on the data in the EEPROM.\n"

common/eeprom/eeprom_field.c

@@ -56,8 +56,8 @@ static int __eeprom_field_update_bin(struct eeprom_field *field,
 			tmp[k] = value[reverse ? i - 1 + k : i + k];
 		}
 
-		byte = simple_strtoul(tmp, &endptr, 0);
+		byte = simple_strtoul(tmp, &endptr, 16);
-		if (*endptr != '\0' || byte < 0)
+		if (*endptr != '\0')
 			return -1;
 
 		field->buf[j] = byte;

common/eeprom/eeprom_layout.c

@@ -56,6 +56,28 @@ static void eeprom_layout_print(const struct eeprom_layout *layout)
 		fields[i].print(&fields[i]);
 }
 
+/*
+ * eeprom_layout_find_field() - finds a layout field by name
+ * @layout:	A pointer to an existing struct layout.
+ * @field_name:	The name of the field to update.
+ * @warn:	Whether to print a warning if the field is not found.
+ *
+ * Returns: a pointer to the found field or NULL on failure.
+ */
+struct eeprom_field *eeprom_layout_find_field(struct eeprom_layout *layout,
+					      char *field_name, bool warn)
+{
+	for (int i = 0; i < layout->num_of_fields; i++)
+		if (layout->fields[i].name != RESERVED_FIELDS &&
+		    !strcmp(layout->fields[i].name, field_name))
+			return &layout->fields[i];
+
+	if (warn)
+		printf("No such field '%s'\n", field_name);
+
+	return NULL;
+}
+
 /*
  * eeprom_layout_update_field() - update a single field in the layout data.
  * @layout:	A pointer to an existing struct layout.
@@ -67,8 +89,8 @@ static void eeprom_layout_print(const struct eeprom_layout *layout)
 static int eeprom_layout_update_field(struct eeprom_layout *layout,
 				      char *field_name, char *new_data)
 {
-	int i, err;
+	struct eeprom_field *field;
-	struct eeprom_field *fields = layout->fields;
+	int err;
 
 	if (new_data == NULL)
 		return 0;
@@ -76,21 +98,15 @@ static int eeprom_layout_update_field(struct eeprom_layout *layout,
 	if (field_name == NULL)
 		return -1;
 
-	for (i = 0; i < layout->num_of_fields; i++) {
+	field = eeprom_layout_find_field(layout, field_name, true);
-		if (fields[i].name == RESERVED_FIELDS ||
+	if (field == NULL)
-		    strcmp(fields[i].name, field_name))
+		return -1;
-			continue;
 
-		err = fields[i].update(&fields[i], new_data);
+	err = field->update(field, new_data);
-		if (err)
+	if (err)
-			printf("Invalid data for field %s\n", field_name);
+		printf("Invalid data for field %s\n", field_name);
 
-		return err;
+	return err;
-	}
-
-	printf("No such field '%s'\n", field_name);
-
-	return -1;
 }
 
 /*
@@ -111,14 +127,14 @@ void eeprom_layout_setup(struct eeprom_layout *layout, unsigned char *buf,
 	else
 		layout->layout_version = layout_version;
 
+	layout->data_size = buf_size;
+	layout->print = eeprom_layout_print;
+	layout->update = eeprom_layout_update_field;
+
 	eeprom_layout_assign(layout, layout_version);
 	layout->data = buf;
 	for (i = 0; i < layout->num_of_fields; i++) {
 		layout->fields[i].buf = buf;
 		buf += layout->fields[i].size;
 	}
-
-	layout->data_size = buf_size;
-	layout->print = eeprom_layout_print;
-	layout->update = eeprom_layout_update_field;
 }

configs/cm_fx6_defconfig

@@ -46,6 +46,7 @@ CONFIG_SYS_MAXARGS=32
 CONFIG_CMD_GREPENV=y
 CONFIG_CMD_EEPROM=y
 CONFIG_CMD_EEPROM_LAYOUT=y
+CONFIG_EEPROM_LAYOUT_VERSIONS=y
 CONFIG_EEPROM_LAYOUT_HELP_STRING="v2, v3"
 CONFIG_SYS_I2C_EEPROM_BUS=2
 CONFIG_SYS_EEPROM_PAGE_WRITE_BITS=4

configs/cm_t43_defconfig

@@ -50,6 +50,7 @@ CONFIG_SYS_PROMPT="CM-T43 # "
 CONFIG_CMD_ASKENV=y
 CONFIG_CMD_EEPROM=y
 CONFIG_CMD_EEPROM_LAYOUT=y
+CONFIG_EEPROM_LAYOUT_VERSIONS=y
 CONFIG_EEPROM_LAYOUT_HELP_STRING="v2, v3"
 CONFIG_SYS_EEPROM_PAGE_WRITE_BITS=4
 CONFIG_SYS_EEPROM_PAGE_WRITE_DELAY_MS=5

include/eeprom_layout.h

@@ -9,6 +9,8 @@
 #ifndef _LAYOUT_
 #define _LAYOUT_
 
+#include <eeprom_field.h>
+
 #define RESERVED_FIELDS			NULL
 #define LAYOUT_VERSION_UNRECOGNIZED	-1
 #define LAYOUT_VERSION_AUTODETECT	-2
@@ -24,6 +26,8 @@ struct eeprom_layout {
 		      char *new_data);
 };
 
+struct eeprom_field *eeprom_layout_find_field(struct eeprom_layout *layout,
+					      char *field_name, bool warn);
 void eeprom_layout_setup(struct eeprom_layout *layout, unsigned char *buf,
 			 unsigned int buf_size, int layout_version);
 __weak void __eeprom_layout_assign(struct eeprom_layout *layout,