* [[PATCH v2] 0/4] cmd: tlv_eeprom: global variables and error cleanup
@ 2023-05-05 8:20 Josua Mayer
2023-05-05 8:20 ` [[PATCH v2] 1/4] cmd: tlv_eeprom: remove use of global variable current_dev Josua Mayer
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Josua Mayer @ 2023-05-05 8:20 UTC (permalink / raw)
To: u-boot; +Cc: Josua Mayer
This patch-set removes some uses of global variables, and improves error
reporting for the "read" command.
It is intended to help switching to a split tlv library eventually,
but general enough to apply independently.
Changes since v1:
- rebased on master 0160d58
- added first patch!!! I had forgotten to send in v1
- added reviewed-tags by Stefan for patches 2-4
Josua Mayer (4):
cmd: tlv_eeprom: remove use of global variable current_dev
cmd: tlv_eeprom: remove use of global variable has_been_read
cmd: tlv_eeprom: handle -ENODEV error from read_eeprom function
cmd: tlv_eeprom: enable 'dev' subcommand before 'read'
cmd/tlv_eeprom.c | 107 +++++++++++++++++++++++++------------------
include/tlv_eeprom.h | 3 +-
2 files changed, 64 insertions(+), 46 deletions(-)
--
2.35.3
^ permalink raw reply [flat|nested] 8+ messages in thread
* [[PATCH v2] 1/4] cmd: tlv_eeprom: remove use of global variable current_dev
2023-05-05 8:20 [[PATCH v2] 0/4] cmd: tlv_eeprom: global variables and error cleanup Josua Mayer
@ 2023-05-05 8:20 ` Josua Mayer
2023-05-08 8:48 ` Stefan Roese
2023-05-05 8:20 ` [[PATCH v2] 2/4] cmd: tlv_eeprom: remove use of global variable has_been_read Josua Mayer
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Josua Mayer @ 2023-05-05 8:20 UTC (permalink / raw)
To: u-boot; +Cc: Josua Mayer, Stefan Roese, Baruch Siach, Heinrich Schuchardt
Make tlv_eeprom command device selection an explicit parameter of all
function calls.
Signed-off-by: Josua Mayer <josua@solid-run.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Baruch Siach <baruch@tkos.co.il>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
cmd/tlv_eeprom.c | 50 ++++++++++++++++++++++----------------------
include/tlv_eeprom.h | 3 ++-
2 files changed, 27 insertions(+), 26 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index 4591ff336bb..8049bf9843c 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -29,18 +29,18 @@ DECLARE_GLOBAL_DATA_PTR;
/* File scope function prototypes */
static bool is_checksum_valid(u8 *eeprom);
-static int read_eeprom(u8 *eeprom);
-static void show_eeprom(u8 *eeprom);
+static int read_eeprom(int devnum, u8 *eeprom);
+static void show_eeprom(int devnum, u8 *eeprom);
static void decode_tlv(struct tlvinfo_tlv *tlv);
static void update_crc(u8 *eeprom);
-static int prog_eeprom(u8 *eeprom);
+static int prog_eeprom(int devnum, 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);
+static void show_tlv_devices(int current_dev);
/* Set to 1 if we've read EEPROM into memory */
static int has_been_read;
@@ -48,7 +48,6 @@ static int has_been_read;
static u8 eeprom[TLV_INFO_MAX_LEN];
static struct udevice *tlv_devices[MAX_TLV_DEVICES];
-static unsigned int current_dev;
#define to_header(p) ((struct tlvinfo_header *)p)
#define to_entry(p) ((struct tlvinfo_tlv *)p)
@@ -125,7 +124,7 @@ static bool is_checksum_valid(u8 *eeprom)
*
* Read the EEPROM into memory, if it hasn't already been read.
*/
-static int read_eeprom(u8 *eeprom)
+static int read_eeprom(int devnum, u8 *eeprom)
{
int ret;
struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
@@ -135,12 +134,11 @@ static int read_eeprom(u8 *eeprom)
return 0;
/* Read the header */
- ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, current_dev);
+ ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, devnum);
/* If the header was successfully read, read the TLVs */
if (ret == 0 && is_valid_tlvinfo_header(eeprom_hdr))
ret = read_tlv_eeprom((void *)eeprom_tlv, HDR_SIZE,
- be16_to_cpu(eeprom_hdr->totallen),
- current_dev);
+ be16_to_cpu(eeprom_hdr->totallen), devnum);
// If the contents are invalid, start over with default contents
if (!is_valid_tlvinfo_header(eeprom_hdr) ||
@@ -165,7 +163,7 @@ static int read_eeprom(u8 *eeprom)
*
* Display the contents of the EEPROM
*/
-static void show_eeprom(u8 *eeprom)
+static void show_eeprom(int devnum, u8 *eeprom)
{
int tlv_end;
int curr_tlv;
@@ -180,7 +178,7 @@ static void show_eeprom(u8 *eeprom)
return;
}
- printf("TLV: %u\n", current_dev);
+ printf("TLV: %u\n", devnum);
printf("TlvInfo Header:\n");
printf(" Id String: %s\n", eeprom_hdr->signature);
printf(" Version: %d\n", eeprom_hdr->version);
@@ -389,7 +387,7 @@ static void update_crc(u8 *eeprom)
*
* Write the EEPROM data from CPU memory to the hardware.
*/
-static int prog_eeprom(u8 *eeprom)
+static int prog_eeprom(int devnum, u8 *eeprom)
{
int ret = 0;
struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
@@ -398,7 +396,7 @@ static int prog_eeprom(u8 *eeprom)
update_crc(eeprom);
eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen);
- ret = write_tlv_eeprom(eeprom, eeprom_len);
+ ret = write_tlv_eeprom(eeprom, eeprom_len, devnum);
if (ret) {
printf("Programming failed.\n");
return -1;
@@ -433,11 +431,12 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
{
char cmd;
struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
+ static unsigned int current_dev;
// If no arguments, read the EERPOM and display its contents
if (argc == 1) {
- read_eeprom(eeprom);
- show_eeprom(eeprom);
+ read_eeprom(current_dev, eeprom);
+ show_eeprom(current_dev, eeprom);
return 0;
}
@@ -448,7 +447,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
// Read the EEPROM contents
if (cmd == 'r') {
has_been_read = 0;
- if (!read_eeprom(eeprom))
+ if (!read_eeprom(current_dev, eeprom))
printf("EEPROM data loaded from device to memory.\n");
return 0;
}
@@ -463,7 +462,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
if (argc == 2) {
switch (cmd) {
case 'w': /* write */
- prog_eeprom(eeprom);
+ prog_eeprom(current_dev, eeprom);
break;
case 'e': /* erase */
strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING);
@@ -476,7 +475,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
show_tlv_code_list();
break;
case 'd': /* dev */
- show_tlv_devices();
+ show_tlv_devices(current_dev);
break;
default:
return CMD_RET_USAGE;
@@ -883,7 +882,7 @@ static int set_bytes(char *buf, const char *string, int *converted_accum)
return 0;
}
-static void show_tlv_devices(void)
+static void show_tlv_devices(int current_dev)
{
unsigned int dev;
@@ -953,14 +952,14 @@ int read_tlv_eeprom(void *eeprom, int offset, int len, int dev_num)
/**
* write_tlv_eeprom - write the hwinfo to i2c EEPROM
*/
-int write_tlv_eeprom(void *eeprom, int len)
+int write_tlv_eeprom(void *eeprom, int len, int dev)
{
if (!(gd->flags & GD_FLG_RELOC))
return -ENODEV;
- if (!tlv_devices[current_dev])
+ if (!tlv_devices[dev])
return -ENODEV;
- return i2c_eeprom_write(tlv_devices[current_dev], 0, eeprom, len);
+ return i2c_eeprom_write(tlv_devices[dev], 0, eeprom, len);
}
int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr,
@@ -1015,10 +1014,11 @@ int mac_read_from_eeprom(void)
int maccount;
u8 macbase[6];
struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
+ int devnum = 0; // TODO: support multiple EEPROMs
puts("EEPROM: ");
- if (read_eeprom(eeprom)) {
+ if (read_eeprom(devnum, eeprom)) {
printf("Read failed.\n");
return -1;
}
@@ -1083,7 +1083,7 @@ int mac_read_from_eeprom(void)
*
* This function must be called after relocation.
*/
-int populate_serial_number(void)
+int populate_serial_number(int devnum)
{
char serialstr[257];
int eeprom_index;
@@ -1092,7 +1092,7 @@ int populate_serial_number(void)
if (env_get("serial#"))
return 0;
- if (read_eeprom(eeprom)) {
+ if (read_eeprom(devnum, eeprom)) {
printf("Read failed.\n");
return -1;
}
diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h
index a2c333e7446..fd45e5f6ebb 100644
--- a/include/tlv_eeprom.h
+++ b/include/tlv_eeprom.h
@@ -84,11 +84,12 @@ int read_tlv_eeprom(void *eeprom, int offset, int len, int dev);
* write_tlv_eeprom - Write the entire EEPROM binary data to the hardware
* @eeprom: Pointer to buffer to hold the binary data
* @len : Maximum size of buffer
+ * @dev : EEPROM device to write
*
* Note: this routine does not validate the EEPROM data.
*
*/
-int write_tlv_eeprom(void *eeprom, int len);
+int write_tlv_eeprom(void *eeprom, int len, int dev);
/**
* read_tlvinfo_tlv_eeprom - Read the TLV from EEPROM, and validate
--
2.35.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [[PATCH v2] 2/4] cmd: tlv_eeprom: remove use of global variable has_been_read
2023-05-05 8:20 [[PATCH v2] 0/4] cmd: tlv_eeprom: global variables and error cleanup Josua Mayer
2023-05-05 8:20 ` [[PATCH v2] 1/4] cmd: tlv_eeprom: remove use of global variable current_dev Josua Mayer
@ 2023-05-05 8:20 ` Josua Mayer
2023-05-05 8:20 ` [[PATCH v2] 3/4] cmd: tlv_eeprom: handle -ENODEV error from read_eeprom function Josua Mayer
2023-05-05 8:20 ` [[PATCH v2] 4/4] cmd: tlv_eeprom: enable 'dev' subcommand before 'read' Josua Mayer
3 siblings, 0 replies; 8+ messages in thread
From: Josua Mayer @ 2023-05-05 8:20 UTC (permalink / raw)
To: u-boot; +Cc: Josua Mayer, Stefan Roese, Baruch Siach, Heinrich Schuchardt
has_been_read is only used as an optimization for do_tlv_eeprom.
Explicitly use and set inside this function, thus making read_eeprom
stateless.
Signed-off-by: Josua Mayer <josua@solid-run.com>
Reviewed-by: Stefan Roese <sr@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Baruch Siach <baruch@tkos.co.il>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
cmd/tlv_eeprom.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index 8049bf9843c..d36401e9138 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -42,8 +42,6 @@ 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(int current_dev);
-/* Set to 1 if we've read EEPROM into memory */
-static int has_been_read;
/* The EERPOM contents after being read into memory */
static u8 eeprom[TLV_INFO_MAX_LEN];
@@ -130,9 +128,6 @@ static int read_eeprom(int devnum, u8 *eeprom)
struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
struct tlvinfo_tlv *eeprom_tlv = to_entry(&eeprom[HDR_SIZE]);
- if (has_been_read)
- return 0;
-
/* Read the header */
ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, devnum);
/* If the header was successfully read, read the TLVs */
@@ -149,10 +144,8 @@ static int read_eeprom(int devnum, u8 *eeprom)
update_crc(eeprom);
}
- has_been_read = 1;
-
#ifdef DEBUG
- show_eeprom(eeprom);
+ show_eeprom(devnum, eeprom);
#endif
return ret;
@@ -432,10 +425,16 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
char cmd;
struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
static unsigned int current_dev;
+ /* Set to 1 if we've read EEPROM into memory */
+ static int has_been_read;
+ int ret;
// If no arguments, read the EERPOM and display its contents
if (argc == 1) {
- read_eeprom(current_dev, eeprom);
+ if (!has_been_read) {
+ if (read_eeprom(current_dev, eeprom) == 0)
+ has_been_read = 1;
+ }
show_eeprom(current_dev, eeprom);
return 0;
}
@@ -447,8 +446,10 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
// Read the EEPROM contents
if (cmd == 'r') {
has_been_read = 0;
- if (!read_eeprom(current_dev, eeprom))
+ if (read_eeprom(current_dev, eeprom) == 0) {
printf("EEPROM data loaded from device to memory.\n");
+ has_been_read = 1;
+ }
return 0;
}
--
2.35.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [[PATCH v2] 3/4] cmd: tlv_eeprom: handle -ENODEV error from read_eeprom function
2023-05-05 8:20 [[PATCH v2] 0/4] cmd: tlv_eeprom: global variables and error cleanup Josua Mayer
2023-05-05 8:20 ` [[PATCH v2] 1/4] cmd: tlv_eeprom: remove use of global variable current_dev Josua Mayer
2023-05-05 8:20 ` [[PATCH v2] 2/4] cmd: tlv_eeprom: remove use of global variable has_been_read Josua Mayer
@ 2023-05-05 8:20 ` Josua Mayer
2023-05-05 8:20 ` [[PATCH v2] 4/4] cmd: tlv_eeprom: enable 'dev' subcommand before 'read' Josua Mayer
3 siblings, 0 replies; 8+ messages in thread
From: Josua Mayer @ 2023-05-05 8:20 UTC (permalink / raw)
To: u-boot; +Cc: Josua Mayer, Stefan Roese, Baruch Siach, Heinrich Schuchardt
When tlv eeprom does not exist, return error code instead of quietly
making up tlv structure in memory.
Signed-off-by: Josua Mayer <josua@solid-run.com>
Reviewed-by: Stefan Roese <sr@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Baruch Siach <baruch@tkos.co.il>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
cmd/tlv_eeprom.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index d36401e9138..636c1fe32ef 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -134,6 +134,8 @@ static int read_eeprom(int devnum, u8 *eeprom)
if (ret == 0 && is_valid_tlvinfo_header(eeprom_hdr))
ret = read_tlv_eeprom((void *)eeprom_tlv, HDR_SIZE,
be16_to_cpu(eeprom_hdr->totallen), devnum);
+ else if (ret == -ENODEV)
+ return ret;
// If the contents are invalid, start over with default contents
if (!is_valid_tlvinfo_header(eeprom_hdr) ||
@@ -432,8 +434,13 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
// If no arguments, read the EERPOM and display its contents
if (argc == 1) {
if (!has_been_read) {
- if (read_eeprom(current_dev, eeprom) == 0)
- has_been_read = 1;
+ ret = read_eeprom(current_dev, eeprom);
+ if (ret) {
+ printf("Failed to read EEPROM data from device.\n");
+ return 0;
+ }
+
+ has_been_read = 1;
}
show_eeprom(current_dev, eeprom);
return 0;
@@ -446,11 +453,14 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
// Read the EEPROM contents
if (cmd == 'r') {
has_been_read = 0;
- if (read_eeprom(current_dev, eeprom) == 0) {
- printf("EEPROM data loaded from device to memory.\n");
- has_been_read = 1;
+ ret = read_eeprom(current_dev, eeprom);
+ if (ret) {
+ printf("Failed to read EEPROM data from device.\n");
+ return 0;
}
- return 0;
+
+ printf("EEPROM data loaded from device to memory.\n");
+ has_been_read = 1;
}
// Subsequent commands require that the EEPROM has already been read.
--
2.35.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [[PATCH v2] 4/4] cmd: tlv_eeprom: enable 'dev' subcommand before 'read'
2023-05-05 8:20 [[PATCH v2] 0/4] cmd: tlv_eeprom: global variables and error cleanup Josua Mayer
` (2 preceding siblings ...)
2023-05-05 8:20 ` [[PATCH v2] 3/4] cmd: tlv_eeprom: handle -ENODEV error from read_eeprom function Josua Mayer
@ 2023-05-05 8:20 ` Josua Mayer
2023-05-08 14:42 ` Simon Glass
3 siblings, 1 reply; 8+ messages in thread
From: Josua Mayer @ 2023-05-05 8:20 UTC (permalink / raw)
To: u-boot; +Cc: Josua Mayer, Stefan Roese, Baruch Siach, Heinrich Schuchardt
Move the handler for "tlv_eeprom dev X" command to the beginning of
do_tlv_eeprom, to allow using it before issuing a "read" command for
currently selected eeprom.
Also remove the check if eeprom exists, since that can only work after
the first execution of read_eeprom triggered device lookup.
Instead accept values up to the defined array size (MAX_TLV_DEVICES).
Signed-off-by: Josua Mayer <josua@solid-run.com>
Reviewed-by: Stefan Roese <sr@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Baruch Siach <baruch@tkos.co.il>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
cmd/tlv_eeprom.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index 636c1fe32ef..79796394c5c 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -450,6 +450,22 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
// "reset" will both be treated as "read".
cmd = argv[1][0];
+ // select device
+ if (cmd == 'd') {
+ /* 'dev' command */
+ unsigned int devnum;
+
+ devnum = simple_strtoul(argv[2], NULL, 0);
+ if (devnum >= MAX_TLV_DEVICES) {
+ printf("Invalid device number\n");
+ return 0;
+ }
+ current_dev = devnum;
+ has_been_read = 0;
+
+ return 0;
+ }
+
// Read the EEPROM contents
if (cmd == 'r') {
has_been_read = 0;
@@ -508,16 +524,6 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
tlvinfo_delete_tlv(eeprom, tcode);
if (argc == 4)
tlvinfo_add_tlv(eeprom, tcode, argv[3]);
- } else if (cmd == 'd') { /* 'dev' command */
- unsigned int devnum;
-
- devnum = simple_strtoul(argv[2], NULL, 0);
- if (devnum > MAX_TLV_DEVICES || !tlv_devices[devnum]) {
- printf("Invalid device number\n");
- return 0;
- }
- current_dev = devnum;
- has_been_read = 0;
} else {
return CMD_RET_USAGE;
}
--
2.35.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [[PATCH v2] 1/4] cmd: tlv_eeprom: remove use of global variable current_dev
2023-05-05 8:20 ` [[PATCH v2] 1/4] cmd: tlv_eeprom: remove use of global variable current_dev Josua Mayer
@ 2023-05-08 8:48 ` Stefan Roese
0 siblings, 0 replies; 8+ messages in thread
From: Stefan Roese @ 2023-05-08 8:48 UTC (permalink / raw)
To: Josua Mayer, u-boot; +Cc: Baruch Siach, Heinrich Schuchardt
On 5/5/23 10:20, Josua Mayer wrote:
> Make tlv_eeprom command device selection an explicit parameter of all
> function calls.
>
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Baruch Siach <baruch@tkos.co.il>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
All patches:
Applied to u-boot-marvell/master
Thanks,
Stefan
> ---
> cmd/tlv_eeprom.c | 50 ++++++++++++++++++++++----------------------
> include/tlv_eeprom.h | 3 ++-
> 2 files changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
> index 4591ff336bb..8049bf9843c 100644
> --- a/cmd/tlv_eeprom.c
> +++ b/cmd/tlv_eeprom.c
> @@ -29,18 +29,18 @@ DECLARE_GLOBAL_DATA_PTR;
>
> /* File scope function prototypes */
> static bool is_checksum_valid(u8 *eeprom);
> -static int read_eeprom(u8 *eeprom);
> -static void show_eeprom(u8 *eeprom);
> +static int read_eeprom(int devnum, u8 *eeprom);
> +static void show_eeprom(int devnum, u8 *eeprom);
> static void decode_tlv(struct tlvinfo_tlv *tlv);
> static void update_crc(u8 *eeprom);
> -static int prog_eeprom(u8 *eeprom);
> +static int prog_eeprom(int devnum, 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);
> +static void show_tlv_devices(int current_dev);
>
> /* Set to 1 if we've read EEPROM into memory */
> static int has_been_read;
> @@ -48,7 +48,6 @@ static int has_been_read;
> static u8 eeprom[TLV_INFO_MAX_LEN];
>
> static struct udevice *tlv_devices[MAX_TLV_DEVICES];
> -static unsigned int current_dev;
>
> #define to_header(p) ((struct tlvinfo_header *)p)
> #define to_entry(p) ((struct tlvinfo_tlv *)p)
> @@ -125,7 +124,7 @@ static bool is_checksum_valid(u8 *eeprom)
> *
> * Read the EEPROM into memory, if it hasn't already been read.
> */
> -static int read_eeprom(u8 *eeprom)
> +static int read_eeprom(int devnum, u8 *eeprom)
> {
> int ret;
> struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
> @@ -135,12 +134,11 @@ static int read_eeprom(u8 *eeprom)
> return 0;
>
> /* Read the header */
> - ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, current_dev);
> + ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, devnum);
> /* If the header was successfully read, read the TLVs */
> if (ret == 0 && is_valid_tlvinfo_header(eeprom_hdr))
> ret = read_tlv_eeprom((void *)eeprom_tlv, HDR_SIZE,
> - be16_to_cpu(eeprom_hdr->totallen),
> - current_dev);
> + be16_to_cpu(eeprom_hdr->totallen), devnum);
>
> // If the contents are invalid, start over with default contents
> if (!is_valid_tlvinfo_header(eeprom_hdr) ||
> @@ -165,7 +163,7 @@ static int read_eeprom(u8 *eeprom)
> *
> * Display the contents of the EEPROM
> */
> -static void show_eeprom(u8 *eeprom)
> +static void show_eeprom(int devnum, u8 *eeprom)
> {
> int tlv_end;
> int curr_tlv;
> @@ -180,7 +178,7 @@ static void show_eeprom(u8 *eeprom)
> return;
> }
>
> - printf("TLV: %u\n", current_dev);
> + printf("TLV: %u\n", devnum);
> printf("TlvInfo Header:\n");
> printf(" Id String: %s\n", eeprom_hdr->signature);
> printf(" Version: %d\n", eeprom_hdr->version);
> @@ -389,7 +387,7 @@ static void update_crc(u8 *eeprom)
> *
> * Write the EEPROM data from CPU memory to the hardware.
> */
> -static int prog_eeprom(u8 *eeprom)
> +static int prog_eeprom(int devnum, u8 *eeprom)
> {
> int ret = 0;
> struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
> @@ -398,7 +396,7 @@ static int prog_eeprom(u8 *eeprom)
> update_crc(eeprom);
>
> eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen);
> - ret = write_tlv_eeprom(eeprom, eeprom_len);
> + ret = write_tlv_eeprom(eeprom, eeprom_len, devnum);
> if (ret) {
> printf("Programming failed.\n");
> return -1;
> @@ -433,11 +431,12 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> {
> char cmd;
> struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
> + static unsigned int current_dev;
>
> // If no arguments, read the EERPOM and display its contents
> if (argc == 1) {
> - read_eeprom(eeprom);
> - show_eeprom(eeprom);
> + read_eeprom(current_dev, eeprom);
> + show_eeprom(current_dev, eeprom);
> return 0;
> }
>
> @@ -448,7 +447,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> // Read the EEPROM contents
> if (cmd == 'r') {
> has_been_read = 0;
> - if (!read_eeprom(eeprom))
> + if (!read_eeprom(current_dev, eeprom))
> printf("EEPROM data loaded from device to memory.\n");
> return 0;
> }
> @@ -463,7 +462,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> if (argc == 2) {
> switch (cmd) {
> case 'w': /* write */
> - prog_eeprom(eeprom);
> + prog_eeprom(current_dev, eeprom);
> break;
> case 'e': /* erase */
> strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING);
> @@ -476,7 +475,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> show_tlv_code_list();
> break;
> case 'd': /* dev */
> - show_tlv_devices();
> + show_tlv_devices(current_dev);
> break;
> default:
> return CMD_RET_USAGE;
> @@ -883,7 +882,7 @@ static int set_bytes(char *buf, const char *string, int *converted_accum)
> return 0;
> }
>
> -static void show_tlv_devices(void)
> +static void show_tlv_devices(int current_dev)
> {
> unsigned int dev;
>
> @@ -953,14 +952,14 @@ int read_tlv_eeprom(void *eeprom, int offset, int len, int dev_num)
> /**
> * write_tlv_eeprom - write the hwinfo to i2c EEPROM
> */
> -int write_tlv_eeprom(void *eeprom, int len)
> +int write_tlv_eeprom(void *eeprom, int len, int dev)
> {
> if (!(gd->flags & GD_FLG_RELOC))
> return -ENODEV;
> - if (!tlv_devices[current_dev])
> + if (!tlv_devices[dev])
> return -ENODEV;
>
> - return i2c_eeprom_write(tlv_devices[current_dev], 0, eeprom, len);
> + return i2c_eeprom_write(tlv_devices[dev], 0, eeprom, len);
> }
>
> int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr,
> @@ -1015,10 +1014,11 @@ int mac_read_from_eeprom(void)
> int maccount;
> u8 macbase[6];
> struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
> + int devnum = 0; // TODO: support multiple EEPROMs
>
> puts("EEPROM: ");
>
> - if (read_eeprom(eeprom)) {
> + if (read_eeprom(devnum, eeprom)) {
> printf("Read failed.\n");
> return -1;
> }
> @@ -1083,7 +1083,7 @@ int mac_read_from_eeprom(void)
> *
> * This function must be called after relocation.
> */
> -int populate_serial_number(void)
> +int populate_serial_number(int devnum)
> {
> char serialstr[257];
> int eeprom_index;
> @@ -1092,7 +1092,7 @@ int populate_serial_number(void)
> if (env_get("serial#"))
> return 0;
>
> - if (read_eeprom(eeprom)) {
> + if (read_eeprom(devnum, eeprom)) {
> printf("Read failed.\n");
> return -1;
> }
> diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h
> index a2c333e7446..fd45e5f6ebb 100644
> --- a/include/tlv_eeprom.h
> +++ b/include/tlv_eeprom.h
> @@ -84,11 +84,12 @@ int read_tlv_eeprom(void *eeprom, int offset, int len, int dev);
> * write_tlv_eeprom - Write the entire EEPROM binary data to the hardware
> * @eeprom: Pointer to buffer to hold the binary data
> * @len : Maximum size of buffer
> + * @dev : EEPROM device to write
> *
> * Note: this routine does not validate the EEPROM data.
> *
> */
> -int write_tlv_eeprom(void *eeprom, int len);
> +int write_tlv_eeprom(void *eeprom, int len, int dev);
>
> /**
> * read_tlvinfo_tlv_eeprom - Read the TLV from EEPROM, and validate
Viele Grüße,
Stefan Roese
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [[PATCH v2] 4/4] cmd: tlv_eeprom: enable 'dev' subcommand before 'read'
2023-05-05 8:20 ` [[PATCH v2] 4/4] cmd: tlv_eeprom: enable 'dev' subcommand before 'read' Josua Mayer
@ 2023-05-08 14:42 ` Simon Glass
2023-05-13 8:39 ` Josua Mayer
0 siblings, 1 reply; 8+ messages in thread
From: Simon Glass @ 2023-05-08 14:42 UTC (permalink / raw)
To: Josua Mayer; +Cc: u-boot, Stefan Roese, Baruch Siach, Heinrich Schuchardt
Hi,
On Fri, 5 May 2023 at 02:22, Josua Mayer <josua@solid-run.com> wrote:
>
> Move the handler for "tlv_eeprom dev X" command to the beginning of
> do_tlv_eeprom, to allow using it before issuing a "read" command for
> currently selected eeprom.
>
> Also remove the check if eeprom exists, since that can only work after
> the first execution of read_eeprom triggered device lookup.
> Instead accept values up to the defined array size (MAX_TLV_DEVICES).
>
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Baruch Siach <baruch@tkos.co.il>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> cmd/tlv_eeprom.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
Can someone take a look at fixing up the driver model code?
For example:
- it maintains its own array of eproms, when it should use driver
model to find them.
- the has_been_read flag seems to track only one EEPROM, but the code
indicates there might be two (should be in the device's private data)
- current_dev should be a pointer to a device
- ideally all state should be either in the device or the uclass, so
it can be used before relocation, etc.
Regards,
Simon
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [[PATCH v2] 4/4] cmd: tlv_eeprom: enable 'dev' subcommand before 'read'
2023-05-08 14:42 ` Simon Glass
@ 2023-05-13 8:39 ` Josua Mayer
0 siblings, 0 replies; 8+ messages in thread
From: Josua Mayer @ 2023-05-13 8:39 UTC (permalink / raw)
To: Simon Glass; +Cc: u-boot, Stefan Roese, Baruch Siach, Heinrich Schuchardt
Hi Simon,
I have a private rewrite of the tlv functionality as a seperate library
outside of the command implementation.
Originally I had hoped to finish upstreaming that about a year ago but
got sidetracked.
Hopefully within the coming weeks I can submit another version as RFC
that can address your concerns more easily than the current eeprom_tlv
implementation.
Sincerely
Josua Mayer
Am 08.05.23 um 17:42 schrieb Simon Glass:
> Hi,
>
> On Fri, 5 May 2023 at 02:22, Josua Mayer <josua@solid-run.com> wrote:
>> Move the handler for "tlv_eeprom dev X" command to the beginning of
>> do_tlv_eeprom, to allow using it before issuing a "read" command for
>> currently selected eeprom.
>>
>> Also remove the check if eeprom exists, since that can only work after
>> the first execution of read_eeprom triggered device lookup.
>> Instead accept values up to the defined array size (MAX_TLV_DEVICES).
>>
>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>> Reviewed-by: Stefan Roese <sr@denx.de>
>> Cc: Stefan Roese <sr@denx.de>
>> Cc: Baruch Siach <baruch@tkos.co.il>
>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> cmd/tlv_eeprom.c | 26 ++++++++++++++++----------
>> 1 file changed, 16 insertions(+), 10 deletions(-)
> Can someone take a look at fixing up the driver model code?
>
> For example:
> - it maintains its own array of eproms, when it should use driver
> model to find them.
> - the has_been_read flag seems to track only one EEPROM, but the code
> indicates there might be two (should be in the device's private data)
> - current_dev should be a pointer to a device
> - ideally all state should be either in the device or the uclass, so
> it can be used before relocation, etc.
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-05-13 8:39 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-05 8:20 [[PATCH v2] 0/4] cmd: tlv_eeprom: global variables and error cleanup Josua Mayer
2023-05-05 8:20 ` [[PATCH v2] 1/4] cmd: tlv_eeprom: remove use of global variable current_dev Josua Mayer
2023-05-08 8:48 ` Stefan Roese
2023-05-05 8:20 ` [[PATCH v2] 2/4] cmd: tlv_eeprom: remove use of global variable has_been_read Josua Mayer
2023-05-05 8:20 ` [[PATCH v2] 3/4] cmd: tlv_eeprom: handle -ENODEV error from read_eeprom function Josua Mayer
2023-05-05 8:20 ` [[PATCH v2] 4/4] cmd: tlv_eeprom: enable 'dev' subcommand before 'read' Josua Mayer
2023-05-08 14:42 ` Simon Glass
2023-05-13 8:39 ` Josua Mayer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.