[PATCH 00/12] split tlv_eeprom command into a separate library
The tlv_eeprom command provides much more than just a cli command: - de- and encoding tlv format - for reading and writing eeprom - setting the eth?addr environment variable - setting the serial# environment variable
One device (Clearfog) is already using the decoding functionality to choose the correct memory size based on eeprom data.
This patchset massages the implementation in many places removing stateful behaviour and global variables as much as possible and changing some functions to be usable as a library. Then finally everything but the handling of the cli command is split off into a separate tlv library that can be used independently from the command.
SolidRun is starting to flash more devices with tlv data at the factory, and we plan to use this information for device identification purposes in future board files. This refactoring will make those implementations easier and reduce the amount of duplicate code to carry around.
Josua Mayer (12): cmd: tlv_eeprom: remove use of global variable current_dev cmd: tlv_eeprom: remove use of global variable has_been_read cmd: tlv_eeprom: do_tlv_eeprom: stop using non-api read_eeprom function cmd: tlv_eeprom: convert functions used by command to api functions cmd: tlv_eeprom: remove empty function implementations from header cmd: tlv_eeprom: move missing declarations and defines to header cmd: tlv_eeprom: hide access to static tlv_devices array behind accessor cmd: tlv_eeprom: clean up two defines for one thing cmd: tlv_eeprom: add my copyright cmd: tlv_eeprom: split off tlv library from command arm: mvebu: clearfog: enable tlv library for spl in favour of eeprom cmd lib: tlv_eeprom: add function for reading one entry into a C string
cmd/Kconfig | 2 + cmd/tlv_eeprom.c | 817 ++----------------------------------- configs/clearfog_defconfig | 3 +- include/tlv_eeprom.h | 122 +++++- lib/Kconfig | 2 + lib/Makefile | 2 + lib/tlv/Kconfig | 15 + lib/tlv/Makefile | 5 + lib/tlv/tlv_eeprom.c | 775 +++++++++++++++++++++++++++++++++++ 9 files changed, 944 insertions(+), 799 deletions(-) create mode 100644 lib/tlv/Kconfig create mode 100644 lib/tlv/Makefile create mode 100644 lib/tlv/tlv_eeprom.c
Cc: Jon Nettleton jon@solid-run.com
Make tlv_eeprom command device selection an explicit parameter of all function calls.
Signed-off-by: Josua Mayer josua@solid-run.com --- 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 bf8d453dc5..f91c11b304 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: cmd_usage(cmdtp); @@ -886,7 +885,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;
@@ -956,14 +955,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, @@ -1018,10 +1017,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; } @@ -1086,7 +1086,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; @@ -1095,7 +1095,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 a2c333e744..fd45e5f6eb 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
Hi Josua,
a few general comments about this series:
- A cover letter for this patchset would be very helpful, so that reviewers have a summary of the changes. - Please Cc the original author of this code Baruch Siach in the next version as well (if there is next version that is).
On 02.05.22 16:18, 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
Reviewed-by: Stefan Roese sr@denx.de
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 bf8d453dc5..f91c11b304 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);@@ -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");@@ -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;@@ -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;@@ -886,7 +885,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;
@@ -956,14 +955,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,
@@ -1018,10 +1017,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; }
@@ -1086,7 +1086,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; @@ -1095,7 +1095,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 a2c333e744..fd45e5f6eb 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
Hi Stefan,
Thank you for starting to review this patchset!
Am 03.05.22 um 09:09 schrieb Stefan Roese:
Hi Josua,
a few general comments about this series:
- A cover letter for this patchset would be very helpful, so that
reviewers have a summary of the changes.
I have sent a cover-letter to the list, but you are not in the To field - perhaps you can find it - as it does give an overview what I am trying to do (subject [PATCH 00/12] split tlv_eeprom command into a separate library)
- Please Cc the original author of this code Baruch Siach in the
next version as well (if there is next version that is).
Made a mental note, will do.
On 02.05.22 16:18, 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
Reviewed-by: Stefan Roese sr@denx.de
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 bf8d453dc5..f91c11b304 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: cmd_usage(cmdtp); @@ -886,7 +885,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; @@ -956,14 +955,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, @@ -1018,10 +1017,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; } @@ -1086,7 +1086,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; @@ -1095,7 +1095,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 a2c333e744..fd45e5f6eb 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
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 --- cmd/tlv_eeprom.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index f91c11b304..bfd4882e0d 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,15 @@ 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 devnum if we've read EEPROM into memory */ + static int has_been_read = -1;
// If no arguments, read the EERPOM and display its contents if (argc == 1) { - read_eeprom(current_dev, eeprom); + if (has_been_read != current_dev) { + if (read_eeprom(current_dev, eeprom) == 0) + has_been_read = current_dev; + } show_eeprom(current_dev, eeprom); return 0; } @@ -446,14 +444,16 @@ 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)) + has_been_read = -1; + if (read_eeprom(current_dev, eeprom) == 0) { printf("EEPROM data loaded from device to memory.\n"); + has_been_read = current_dev; + } return 0; }
// Subsequent commands require that the EEPROM has already been read. - if (!has_been_read) { + if (has_been_read != current_dev) { printf("Please read the EEPROM data first, using the 'tlv_eeprom read' command.\n"); return 0; } @@ -509,7 +509,6 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) return 0; } current_dev = devnum; - has_been_read = 0; } else { cmd_usage(cmdtp); }
On 02.05.22 16:18, Josua Mayer wrote:
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
cmd/tlv_eeprom.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index f91c11b304..bfd4882e0d 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,15 @@ 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 devnum if we've read EEPROM into memory */
- static int has_been_read = -1;
You are not introducing this variable but still it would be "better" to change it to bool instead IMHO.
Other than this:
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
// If no arguments, read the EERPOM and display its contents if (argc == 1) {
read_eeprom(current_dev, eeprom);
if (has_been_read != current_dev) {if (read_eeprom(current_dev, eeprom) == 0)has_been_read = current_dev;}@@ -446,14 +444,16 @@ 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))
has_been_read = -1;if (read_eeprom(current_dev, eeprom) == 0) { printf("EEPROM data loaded from device to memory.\n");has_been_read = current_dev;}return 0; }
// Subsequent commands require that the EEPROM has already been read.
- if (!has_been_read) {
- if (has_been_read != current_dev) { printf("Please read the EEPROM data first, using the 'tlv_eeprom read' command.\n"); return 0; }
@@ -509,7 +509,6 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) return 0; } current_dev = devnum;
has_been_read = 0;
Viele Grüße, Stefan Roese
IN the scope of do_tlv_eeprom, the error-checking provided by the read_eeprom function is not required. Instead use the API function read_tlv_eeprom.
Signed-off-by: Josua Mayer josua@solid-run.com --- cmd/tlv_eeprom.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index bfd4882e0d..00c5b5f840 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -431,7 +431,7 @@ 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 != current_dev) { - if (read_eeprom(current_dev, eeprom) == 0) + if (read_tlv_eeprom(eeprom, 0, TLV_INFO_MAX_LEN, current_dev) == 0) has_been_read = current_dev; } show_eeprom(current_dev, eeprom); @@ -445,7 +445,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 = -1; - if (read_eeprom(current_dev, eeprom) == 0) { + if (read_tlv_eeprom(eeprom, 0, TLV_INFO_MAX_LEN, current_dev) == 0) { printf("EEPROM data loaded from device to memory.\n"); has_been_read = current_dev; }
On 02.05.22 16:18, Josua Mayer wrote:
IN the scope of do_tlv_eeprom, the error-checking provided by the
Nitpicking: "In ..."
read_eeprom function is not required. Instead use the API function read_tlv_eeprom.
Signed-off-by: Josua Mayer josua@solid-run.com
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
cmd/tlv_eeprom.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index bfd4882e0d..00c5b5f840 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -431,7 +431,7 @@ 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 != current_dev) {
if (read_eeprom(current_dev, eeprom) == 0)
if (read_tlv_eeprom(eeprom, 0, TLV_INFO_MAX_LEN, current_dev) == 0) has_been_read = current_dev;@@ -445,7 +445,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 = -1;
if (read_eeprom(current_dev, eeprom) == 0) {
if (read_tlv_eeprom(eeprom, 0, TLV_INFO_MAX_LEN, current_dev) == 0) { printf("EEPROM data loaded from device to memory.\n"); has_been_read = current_dev;
Viele Grüße, Stefan Roese
- prog_eeprom: write_tlvinfo_tlv_eeprom - update_crc: tlvinfo_update_crc - is_valid_tlv: is_valid_tlvinfo_entry - is_checksum_valid: tlvinfo_check_crc
Signed-off-by: Josua Mayer josua@solid-run.com --- cmd/tlv_eeprom.c | 56 +++++++++++++++---------------------------- include/tlv_eeprom.h | 57 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 37 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 00c5b5f840..1b4f2537f6 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR; #define MAX_TLV_DEVICES 2
/* File scope function prototypes */ -static bool is_checksum_valid(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(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); @@ -58,18 +54,6 @@ static inline bool is_digit(char c) return (c >= '0' && c <= '9'); }
-/** - * 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(struct tlvinfo_tlv *tlv) -{ - return((tlv->type != 0x00) && (tlv->type != 0xFF)); -} - /** * is_hex * @@ -83,14 +67,12 @@ static inline u8 is_hex(char p) }
/** - * is_checksum_valid - * * Validate the checksum in the provided TlvInfo EEPROM data. First, * verify that the TlvInfo header is valid, then make sure the last * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data * and compare it to the value stored in the EEPROM CRC-32 TLV. */ -static bool is_checksum_valid(u8 *eeprom) +bool tlvinfo_check_crc(u8 *eeprom) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_crc; @@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom)
// If the contents are invalid, start over with default contents if (!is_valid_tlvinfo_header(eeprom_hdr) || - !is_checksum_valid(eeprom)) { + !tlvinfo_check_crc(eeprom)) { strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); eeprom_hdr->version = TLV_INFO_VERSION; eeprom_hdr->totallen = cpu_to_be16(0); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); }
#ifdef DEBUG @@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom) tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (curr_tlv < tlv_end) { eeprom_tlv = to_entry(&eeprom[curr_tlv]); - if (!is_valid_tlv(eeprom_tlv)) { + if (!is_valid_tlvinfo_entry(eeprom_tlv)) { printf("Invalid TLV field starting at EEPROM offset %d\n", curr_tlv); return; @@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom) }
printf("Checksum is %s.\n", - is_checksum_valid(eeprom) ? "valid" : "invalid"); + tlvinfo_check_crc(eeprom) ? "valid" : "invalid");
#ifdef DEBUG printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN); @@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv) }
/** - * update_crc + * tlvinfo_update_crc * * This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then * one is added. This function should be called after each update to the * EEPROM structure, to make sure the CRC is always correct. */ -static void update_crc(u8 *eeprom) +void tlvinfo_update_crc(u8 *eeprom) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_crc; @@ -376,20 +358,20 @@ static void update_crc(u8 *eeprom) }
/** - * prog_eeprom + * write_tlvinfo_tlv_eeprom * - * Write the EEPROM data from CPU memory to the hardware. + * Write the TLV data from CPU memory to the hardware. */ -static int prog_eeprom(int devnum, u8 *eeprom) +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev) { int ret = 0; struct tlvinfo_header *eeprom_hdr = to_header(eeprom); int eeprom_len;
- update_crc(eeprom); + tlvinfo_update_crc(eeprom);
eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); - ret = write_tlv_eeprom(eeprom, eeprom_len, devnum); + ret = write_tlv_eeprom(eeprom, eeprom_len, dev); if (ret) { printf("Programming failed.\n"); return -1; @@ -462,13 +444,13 @@ 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(current_dev, eeprom); + write_tlvinfo_tlv_eeprom(eeprom, current_dev); break; case 'e': /* erase */ strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); eeprom_hdr->version = TLV_INFO_VERSION; eeprom_hdr->totallen = cpu_to_be16(0); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); printf("EEPROM data in memory reset.\n"); break; case 'l': /* list */ @@ -549,7 +531,7 @@ U_BOOT_CMD(tlv_eeprom, 4, 1, do_tlv_eeprom, * An offset from the beginning of the EEPROM is returned in the * eeprom_index parameter if the TLV is found. */ -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_tlv; @@ -561,7 +543,7 @@ static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) eeprom_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (*eeprom_index < eeprom_end) { eeprom_tlv = to_entry(&eeprom[*eeprom_index]); - if (!is_valid_tlv(eeprom_tlv)) + if (!is_valid_tlvinfo_entry(eeprom_tlv)) return false; if (eeprom_tlv->type == tcode) return true; @@ -594,7 +576,7 @@ static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - tlength); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); return true; } return false; @@ -695,7 +677,7 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) // Update the total length and calculate (add) a new CRC-32 TLV eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + ENT_SIZE + new_tlv_len); - update_crc(eeprom); + tlvinfo_update_crc(eeprom);
return true; } @@ -986,7 +968,7 @@ int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, be16_to_cpu(tlv_hdr->totallen), dev_num); if (ret < 0) return ret; - if (!is_checksum_valid(eeprom)) + if (!tlvinfo_check_crc(eeprom)) return -EINVAL;
*hdr = tlv_hdr; diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h index fd45e5f6eb..30626a1067 100644 --- a/include/tlv_eeprom.h +++ b/include/tlv_eeprom.h @@ -111,6 +111,51 @@ int write_tlv_eeprom(void *eeprom, int len, int dev); int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, struct tlvinfo_tlv **first_entry, int dev);
+/** + * Write TLV data to the EEPROM. + * + * - Only writes length of actual tlv data + * - updates checksum + * + * @eeprom: Pointer to buffer to hold the binary data. Must point to a buffer + * of size at least TLV_INFO_MAX_LEN. + * @dev : EEPROM device to write + * + */ +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev); + +/** + * tlvinfo_find_tlv + * + * This function finds the TLV with the supplied code in the EERPOM. + * An offset from the beginning of the EEPROM is returned in the + * eeprom_index parameter if the TLV is found. + */ +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index); + +/** + * tlvinfo_update_crc + * + * This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then + * one is added. This function should be called after each update to the + * EEPROM structure, to make sure the CRC is always correct. + * + * @eeprom: Pointer to buffer to hold the binary data. Must point to a buffer + * of size at least TLV_INFO_MAX_LEN. + */ +void tlvinfo_update_crc(u8 *eeprom); + +/** + * Validate the checksum in the provided TlvInfo EEPROM data. First, + * verify that the TlvInfo header is valid, then make sure the last + * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data + * and compare it to the value stored in the EEPROM CRC-32 TLV. + * + * @eeprom: Pointer to buffer to hold the binary data. Must point to a buffer + * of size at least TLV_INFO_MAX_LEN. + */ +bool tlvinfo_check_crc(u8 *eeprom); + #else /* !CONFIG_IS_ENABLED(CMD_TLV_EEPROM) */
static inline int read_tlv_eeprom(void *eeprom, int offset, int len, int dev) @@ -150,4 +195,16 @@ static inline bool is_valid_tlvinfo_header(struct tlvinfo_header *hdr) (be16_to_cpu(hdr->totallen) <= TLV_TOTAL_LEN_MAX)); }
+/** + * 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_tlvinfo_entry(struct tlvinfo_tlv *tlv) +{ + return((tlv->type != 0x00) && (tlv->type != 0xFF)); +} + #endif /* __TLV_EEPROM_H_ */
On 02.05.22 16:18, Josua Mayer wrote:
- prog_eeprom: write_tlvinfo_tlv_eeprom
- update_crc: tlvinfo_update_crc
- is_valid_tlv: is_valid_tlvinfo_entry
- is_checksum_valid: tlvinfo_check_crc
So while creating a new API it makes sense to prepend the function names identical IMHO to not "pollute" the namespace. Something like
- tlv_is_valid_entry - tlv_check_crc ...
Just examples, you get the idea.
Thanks, Stefan
Signed-off-by: Josua Mayer josua@solid-run.com
cmd/tlv_eeprom.c | 56 +++++++++++++++---------------------------- include/tlv_eeprom.h | 57 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 37 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 00c5b5f840..1b4f2537f6 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR; #define MAX_TLV_DEVICES 2
/* File scope function prototypes */ -static bool is_checksum_valid(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(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); @@ -58,18 +54,6 @@ static inline bool is_digit(char c) return (c >= '0' && c <= '9'); }
-/**
- 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(struct tlvinfo_tlv *tlv) -{
- return((tlv->type != 0x00) && (tlv->type != 0xFF));
-}
- /**
- is_hex
@@ -83,14 +67,12 @@ static inline u8 is_hex(char p) }
/**
- is_checksum_valid
*/
- Validate the checksum in the provided TlvInfo EEPROM data. First,
- verify that the TlvInfo header is valid, then make sure the last
- TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data
- and compare it to the value stored in the EEPROM CRC-32 TLV.
-static bool is_checksum_valid(u8 *eeprom) +bool tlvinfo_check_crc(u8 *eeprom) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_crc; @@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom)
// If the contents are invalid, start over with default contents if (!is_valid_tlvinfo_header(eeprom_hdr) ||
!is_checksum_valid(eeprom)) {
!tlvinfo_check_crc(eeprom)) {
update_crc(eeprom);
tlvinfo_update_crc(eeprom);}
#ifdef DEBUG
@@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom) tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (curr_tlv < tlv_end) { eeprom_tlv = to_entry(&eeprom[curr_tlv]);
if (!is_valid_tlv(eeprom_tlv)) {
if (!is_valid_tlvinfo_entry(eeprom_tlv)) { printf("Invalid TLV field starting at EEPROM offset %d\n", curr_tlv); return;@@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom) }
printf("Checksum is %s.\n",
is_checksum_valid(eeprom) ? "valid" : "invalid");
tlvinfo_check_crc(eeprom) ? "valid" : "invalid");#ifdef DEBUG printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN);
@@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv) }
/**
- update_crc
*/
- tlvinfo_update_crc
- This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then
- one is added. This function should be called after each update to the
- EEPROM structure, to make sure the CRC is always correct.
-static void update_crc(u8 *eeprom) +void tlvinfo_update_crc(u8 *eeprom) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_crc; @@ -376,20 +358,20 @@ static void update_crc(u8 *eeprom) }
/**
- prog_eeprom
- write_tlvinfo_tlv_eeprom
- Write the EEPROM data from CPU memory to the hardware.
*/
- Write the TLV data from CPU memory to the hardware.
-static int prog_eeprom(int devnum, u8 *eeprom) +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev) { int ret = 0; struct tlvinfo_header *eeprom_hdr = to_header(eeprom); int eeprom_len;
- update_crc(eeprom);
tlvinfo_update_crc(eeprom);
eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen);
- ret = write_tlv_eeprom(eeprom, eeprom_len, devnum);
- ret = write_tlv_eeprom(eeprom, eeprom_len, dev); if (ret) { printf("Programming failed.\n"); return -1;
@@ -462,13 +444,13 @@ 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(current_dev, eeprom);
write_tlvinfo_tlv_eeprom(eeprom, current_dev); break;
update_crc(eeprom);
tlvinfo_update_crc(eeprom); printf("EEPROM data in memory reset.\n"); break;@@ -549,7 +531,7 @@ U_BOOT_CMD(tlv_eeprom, 4, 1, do_tlv_eeprom,
- An offset from the beginning of the EEPROM is returned in the
- eeprom_index parameter if the TLV is found.
*/ -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_tlv; @@ -561,7 +543,7 @@ static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) eeprom_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (*eeprom_index < eeprom_end) { eeprom_tlv = to_entry(&eeprom[*eeprom_index]);
if (!is_valid_tlv(eeprom_tlv))
if (!is_valid_tlvinfo_entry(eeprom_tlv)) return false;@@ -594,7 +576,7 @@ static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - tlength);
update_crc(eeprom);
tlvinfo_update_crc(eeprom);@@ -695,7 +677,7 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) // Update the total length and calculate (add) a new CRC-32 TLV eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + ENT_SIZE + new_tlv_len);
- update_crc(eeprom);
tlvinfo_update_crc(eeprom);
return true; }
@@ -986,7 +968,7 @@ int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, be16_to_cpu(tlv_hdr->totallen), dev_num); if (ret < 0) return ret;
- if (!is_checksum_valid(eeprom))
if (!tlvinfo_check_crc(eeprom)) return -EINVAL;
*hdr = tlv_hdr;
diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h index fd45e5f6eb..30626a1067 100644 --- a/include/tlv_eeprom.h +++ b/include/tlv_eeprom.h @@ -111,6 +111,51 @@ int write_tlv_eeprom(void *eeprom, int len, int dev); int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, struct tlvinfo_tlv **first_entry, int dev);
+/**
- Write TLV data to the EEPROM.
- Only writes length of actual tlv data
- updates checksum
- @eeprom: Pointer to buffer to hold the binary data. Must point to a buffer
of size at least TLV_INFO_MAX_LEN.
- @dev : EEPROM device to write
- */
+int write_tlvinfo_tlv_eeprom(void *eeprom, int dev);
+/**
- tlvinfo_find_tlv
- This function finds the TLV with the supplied code in the EERPOM.
- An offset from the beginning of the EEPROM is returned in the
- eeprom_index parameter if the TLV is found.
- */
+bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index);
+/**
- tlvinfo_update_crc
- This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then
- one is added. This function should be called after each update to the
- EEPROM structure, to make sure the CRC is always correct.
- @eeprom: Pointer to buffer to hold the binary data. Must point to a buffer
of size at least TLV_INFO_MAX_LEN.- */
+void tlvinfo_update_crc(u8 *eeprom);
+/**
- Validate the checksum in the provided TlvInfo EEPROM data. First,
- verify that the TlvInfo header is valid, then make sure the last
- TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data
- and compare it to the value stored in the EEPROM CRC-32 TLV.
- @eeprom: Pointer to buffer to hold the binary data. Must point to a buffer
of size at least TLV_INFO_MAX_LEN.- */
+bool tlvinfo_check_crc(u8 *eeprom);
#else /* !CONFIG_IS_ENABLED(CMD_TLV_EEPROM) */
static inline int read_tlv_eeprom(void *eeprom, int offset, int len, int dev)
@@ -150,4 +195,16 @@ static inline bool is_valid_tlvinfo_header(struct tlvinfo_header *hdr) (be16_to_cpu(hdr->totallen) <= TLV_TOTAL_LEN_MAX)); }
+/**
- 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_tlvinfo_entry(struct tlvinfo_tlv *tlv) +{
- return((tlv->type != 0x00) && (tlv->type != 0xFF));
+}
- #endif /* __TLV_EEPROM_H_ */
Viele Grüße, Stefan Roese
Am 03.05.22 um 09:16 schrieb Stefan Roese:
On 02.05.22 16:18, Josua Mayer wrote:
- prog_eeprom: write_tlvinfo_tlv_eeprom
- update_crc: tlvinfo_update_crc
- is_valid_tlv: is_valid_tlvinfo_entry
- is_checksum_valid: tlvinfo_check_crc
So while creating a new API it makes sense to prepend the function names identical IMHO to not "pollute" the namespace. Something like
- tlv_is_valid_entry
- tlv_check_crc
...
Just examples, you get the idea.
Yes. The hard part in this particular implementation is that the naming is not consistent.
The most sense I could make is that prefix tlvinfo indicates all tlv data, i.e. working with the whole structure, while tlvinfo_tlv indicates working with one data entry. Further write, read and is_ are currently prefixed in the header, but for previously static functions in the C file it was put in the middle ...
I found it quite difficult to prepare for splitting off a library in a way that preserves history, i.e. diffs should still be readable for spotting mistakes.
I was considering to at the very end do a mass-rename and come up with better naming, something like tlv_{set,get}_{blob,string,mac} tlv_find_entry tlv_{read,write}_eeprom
But this is pending a refactoring and extension of the tlv parsing code in board/solidrun/common/tlv_data.*, to figure out what is required or useful.
Thanks, Stefan
Signed-off-by: Josua Mayer josua@solid-run.com
cmd/tlv_eeprom.c | 56 +++++++++++++++---------------------------- include/tlv_eeprom.h | 57 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 37 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 00c5b5f840..1b4f2537f6 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR; #define MAX_TLV_DEVICES 2 /* File scope function prototypes */ -static bool is_checksum_valid(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(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); @@ -58,18 +54,6 @@ static inline bool is_digit(char c) return (c >= '0' && c <= '9'); } -/**
- * 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(struct tlvinfo_tlv *tlv) -{ - return((tlv->type != 0x00) && (tlv->type != 0xFF)); -}
/** * is_hex * @@ -83,14 +67,12 @@ static inline u8 is_hex(char p) } /**
- * is_checksum_valid
* Validate the checksum in the provided TlvInfo EEPROM data. First, * verify that the TlvInfo header is valid, then make sure the last * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data * and compare it to the value stored in the EEPROM CRC-32 TLV. */ -static bool is_checksum_valid(u8 *eeprom) +bool tlvinfo_check_crc(u8 *eeprom) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_crc; @@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom) // If the contents are invalid, start over with default contents if (!is_valid_tlvinfo_header(eeprom_hdr) || - !is_checksum_valid(eeprom)) { + !tlvinfo_check_crc(eeprom)) { strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); eeprom_hdr->version = TLV_INFO_VERSION; eeprom_hdr->totallen = cpu_to_be16(0); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); } #ifdef DEBUG @@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom) tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (curr_tlv < tlv_end) { eeprom_tlv = to_entry(&eeprom[curr_tlv]); - if (!is_valid_tlv(eeprom_tlv)) { + if (!is_valid_tlvinfo_entry(eeprom_tlv)) { printf("Invalid TLV field starting at EEPROM offset %d\n", curr_tlv); return; @@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom) } printf("Checksum is %s.\n", - is_checksum_valid(eeprom) ? "valid" : "invalid"); + tlvinfo_check_crc(eeprom) ? "valid" : "invalid"); #ifdef DEBUG printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN); @@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv) } /**
- * update_crc
- * tlvinfo_update_crc
* * This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then * one is added. This function should be called after each update to the * EEPROM structure, to make sure the CRC is always correct. */ -static void update_crc(u8 *eeprom) +void tlvinfo_update_crc(u8 *eeprom) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_crc; @@ -376,20 +358,20 @@ static void update_crc(u8 *eeprom) } /**
- * prog_eeprom
- * write_tlvinfo_tlv_eeprom
*
- * Write the EEPROM data from CPU memory to the hardware.
- * Write the TLV data from CPU memory to the hardware.
*/ -static int prog_eeprom(int devnum, u8 *eeprom) +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev) { int ret = 0; struct tlvinfo_header *eeprom_hdr = to_header(eeprom); int eeprom_len; - update_crc(eeprom); + tlvinfo_update_crc(eeprom); eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); - ret = write_tlv_eeprom(eeprom, eeprom_len, devnum); + ret = write_tlv_eeprom(eeprom, eeprom_len, dev); if (ret) { printf("Programming failed.\n"); return -1; @@ -462,13 +444,13 @@ 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(current_dev, eeprom); + write_tlvinfo_tlv_eeprom(eeprom, current_dev); break; case 'e': /* erase */ strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); eeprom_hdr->version = TLV_INFO_VERSION; eeprom_hdr->totallen = cpu_to_be16(0); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); printf("EEPROM data in memory reset.\n"); break; case 'l': /* list */ @@ -549,7 +531,7 @@ U_BOOT_CMD(tlv_eeprom, 4, 1, do_tlv_eeprom, * An offset from the beginning of the EEPROM is returned in the * eeprom_index parameter if the TLV is found. */ -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_tlv; @@ -561,7 +543,7 @@ static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) eeprom_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (*eeprom_index < eeprom_end) { eeprom_tlv = to_entry(&eeprom[*eeprom_index]); - if (!is_valid_tlv(eeprom_tlv)) + if (!is_valid_tlvinfo_entry(eeprom_tlv)) return false; if (eeprom_tlv->type == tcode) return true; @@ -594,7 +576,7 @@ static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - tlength); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); return true; } return false; @@ -695,7 +677,7 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) // Update the total length and calculate (add) a new CRC-32 TLV eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + ENT_SIZE + new_tlv_len); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); return true; } @@ -986,7 +968,7 @@ int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, be16_to_cpu(tlv_hdr->totallen), dev_num); if (ret < 0) return ret; - if (!is_checksum_valid(eeprom)) + if (!tlvinfo_check_crc(eeprom)) return -EINVAL; *hdr = tlv_hdr; diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h index fd45e5f6eb..30626a1067 100644 --- a/include/tlv_eeprom.h +++ b/include/tlv_eeprom.h @@ -111,6 +111,51 @@ int write_tlv_eeprom(void *eeprom, int len, int dev); int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, struct tlvinfo_tlv **first_entry, int dev); +/**
- Write TLV data to the EEPROM.
- Only writes length of actual tlv data
- updates checksum
- @eeprom: Pointer to buffer to hold the binary data. Must point to
a buffer
- * of size at least TLV_INFO_MAX_LEN.
- @dev : EEPROM device to write
- */
+int write_tlvinfo_tlv_eeprom(void *eeprom, int dev);
+/**
- * tlvinfo_find_tlv
- * This function finds the TLV with the supplied code in the EERPOM.
- * An offset from the beginning of the EEPROM is returned in the
- * eeprom_index parameter if the TLV is found.
- */
+bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index);
+/**
- * tlvinfo_update_crc
- * This function updates the CRC-32 TLV. If there is no CRC-32 TLV,
then
- * one is added. This function should be called after each update
to the
- * EEPROM structure, to make sure the CRC is always correct.
- @eeprom: Pointer to buffer to hold the binary data. Must point to
a buffer
- * of size at least TLV_INFO_MAX_LEN.
- */
+void tlvinfo_update_crc(u8 *eeprom);
+/**
- * Validate the checksum in the provided TlvInfo EEPROM data. First,
- * verify that the TlvInfo header is valid, then make sure the last
- * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data
- * and compare it to the value stored in the EEPROM CRC-32 TLV.
- @eeprom: Pointer to buffer to hold the binary data. Must point to
a buffer
- * of size at least TLV_INFO_MAX_LEN.
- */
+bool tlvinfo_check_crc(u8 *eeprom);
#else /* !CONFIG_IS_ENABLED(CMD_TLV_EEPROM) */ static inline int read_tlv_eeprom(void *eeprom, int offset, int len, int dev) @@ -150,4 +195,16 @@ static inline bool is_valid_tlvinfo_header(struct tlvinfo_header *hdr) (be16_to_cpu(hdr->totallen) <= TLV_TOTAL_LEN_MAX)); } +/**
- * 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_tlvinfo_entry(struct tlvinfo_tlv *tlv) +{ + return((tlv->type != 0x00) && (tlv->type != 0xFF)); +}
#endif /* __TLV_EEPROM_H_ */
Viele Grüße, Stefan Roese
Hi Josua,
On 03.05.22 09:17, Josua Mayer wrote:
Am 03.05.22 um 09:16 schrieb Stefan Roese:
On 02.05.22 16:18, Josua Mayer wrote:
- prog_eeprom: write_tlvinfo_tlv_eeprom
- update_crc: tlvinfo_update_crc
- is_valid_tlv: is_valid_tlvinfo_entry
- is_checksum_valid: tlvinfo_check_crc
So while creating a new API it makes sense to prepend the function names identical IMHO to not "pollute" the namespace. Something like
- tlv_is_valid_entry
- tlv_check_crc
...
Just examples, you get the idea.
Yes. The hard part in this particular implementation is that the naming is not consistent.
The most sense I could make is that prefix tlvinfo indicates all tlv data, i.e. working with the whole structure, while tlvinfo_tlv indicates working with one data entry. Further write, read and is_ are currently prefixed in the header, but for previously static functions in the C file it was put in the middle ...
I found it quite difficult to prepare for splitting off a library in a way that preserves history, i.e. diffs should still be readable for spotting mistakes.
Yes, a decent history would be welcome. But still, when going global here with a new API this should be consistant.
I was considering to at the very end do a mass-rename and come up with better naming, something like tlv_{set,get}_{blob,string,mac} tlv_find_entry tlv_{read,write}_eeprom
But this is pending a refactoring and extension of the tlv parsing code in board/solidrun/common/tlv_data.*, to figure out what is required or useful.
So your plan is to this: a) Get this patchset included b) Use it in board specific code, e.g. solidrun c) Do the mass-rename
Is this correct? If yes, why is it better to do the renaming at the end?
Thanks, Stefan
Thanks, Stefan
Signed-off-by: Josua Mayer josua@solid-run.com
cmd/tlv_eeprom.c | 56 +++++++++++++++---------------------------- include/tlv_eeprom.h | 57 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 37 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 00c5b5f840..1b4f2537f6 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR; #define MAX_TLV_DEVICES 2 /* File scope function prototypes */ -static bool is_checksum_valid(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(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); @@ -58,18 +54,6 @@ static inline bool is_digit(char c) return (c >= '0' && c <= '9'); } -/**
- * 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(struct tlvinfo_tlv *tlv) -{ - return((tlv->type != 0x00) && (tlv->type != 0xFF)); -}
/** * is_hex * @@ -83,14 +67,12 @@ static inline u8 is_hex(char p) } /**
- * is_checksum_valid
* Validate the checksum in the provided TlvInfo EEPROM data. First, * verify that the TlvInfo header is valid, then make sure the last * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data * and compare it to the value stored in the EEPROM CRC-32 TLV. */ -static bool is_checksum_valid(u8 *eeprom) +bool tlvinfo_check_crc(u8 *eeprom) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_crc; @@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom) // If the contents are invalid, start over with default contents if (!is_valid_tlvinfo_header(eeprom_hdr) || - !is_checksum_valid(eeprom)) { + !tlvinfo_check_crc(eeprom)) { strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); eeprom_hdr->version = TLV_INFO_VERSION; eeprom_hdr->totallen = cpu_to_be16(0); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); } #ifdef DEBUG @@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom) tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (curr_tlv < tlv_end) { eeprom_tlv = to_entry(&eeprom[curr_tlv]); - if (!is_valid_tlv(eeprom_tlv)) { + if (!is_valid_tlvinfo_entry(eeprom_tlv)) { printf("Invalid TLV field starting at EEPROM offset %d\n", curr_tlv); return; @@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom) } printf("Checksum is %s.\n", - is_checksum_valid(eeprom) ? "valid" : "invalid"); + tlvinfo_check_crc(eeprom) ? "valid" : "invalid"); #ifdef DEBUG printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN); @@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv) } /**
- * update_crc
- * tlvinfo_update_crc
* * This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then * one is added. This function should be called after each update to the * EEPROM structure, to make sure the CRC is always correct. */ -static void update_crc(u8 *eeprom) +void tlvinfo_update_crc(u8 *eeprom) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_crc; @@ -376,20 +358,20 @@ static void update_crc(u8 *eeprom) } /**
- * prog_eeprom
- * write_tlvinfo_tlv_eeprom
*
- * Write the EEPROM data from CPU memory to the hardware.
- * Write the TLV data from CPU memory to the hardware.
*/ -static int prog_eeprom(int devnum, u8 *eeprom) +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev) { int ret = 0; struct tlvinfo_header *eeprom_hdr = to_header(eeprom); int eeprom_len; - update_crc(eeprom); + tlvinfo_update_crc(eeprom); eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); - ret = write_tlv_eeprom(eeprom, eeprom_len, devnum); + ret = write_tlv_eeprom(eeprom, eeprom_len, dev); if (ret) { printf("Programming failed.\n"); return -1; @@ -462,13 +444,13 @@ 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(current_dev, eeprom); + write_tlvinfo_tlv_eeprom(eeprom, current_dev); break; case 'e': /* erase */ strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); eeprom_hdr->version = TLV_INFO_VERSION; eeprom_hdr->totallen = cpu_to_be16(0); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); printf("EEPROM data in memory reset.\n"); break; case 'l': /* list */ @@ -549,7 +531,7 @@ U_BOOT_CMD(tlv_eeprom, 4, 1, do_tlv_eeprom, * An offset from the beginning of the EEPROM is returned in the * eeprom_index parameter if the TLV is found. */ -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_tlv; @@ -561,7 +543,7 @@ static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) eeprom_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (*eeprom_index < eeprom_end) { eeprom_tlv = to_entry(&eeprom[*eeprom_index]); - if (!is_valid_tlv(eeprom_tlv)) + if (!is_valid_tlvinfo_entry(eeprom_tlv)) return false; if (eeprom_tlv->type == tcode) return true; @@ -594,7 +576,7 @@ static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - tlength); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); return true; } return false; @@ -695,7 +677,7 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) // Update the total length and calculate (add) a new CRC-32 TLV eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + ENT_SIZE + new_tlv_len); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); return true; } @@ -986,7 +968,7 @@ int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, be16_to_cpu(tlv_hdr->totallen), dev_num); if (ret < 0) return ret; - if (!is_checksum_valid(eeprom)) + if (!tlvinfo_check_crc(eeprom)) return -EINVAL; *hdr = tlv_hdr; diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h index fd45e5f6eb..30626a1067 100644 --- a/include/tlv_eeprom.h +++ b/include/tlv_eeprom.h @@ -111,6 +111,51 @@ int write_tlv_eeprom(void *eeprom, int len, int dev); int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, struct tlvinfo_tlv **first_entry, int dev); +/**
- Write TLV data to the EEPROM.
- Only writes length of actual tlv data
- updates checksum
- @eeprom: Pointer to buffer to hold the binary data. Must point to
a buffer
- * of size at least TLV_INFO_MAX_LEN.
- @dev : EEPROM device to write
- */
+int write_tlvinfo_tlv_eeprom(void *eeprom, int dev);
+/**
- * tlvinfo_find_tlv
- * This function finds the TLV with the supplied code in the EERPOM.
- * An offset from the beginning of the EEPROM is returned in the
- * eeprom_index parameter if the TLV is found.
- */
+bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index);
+/**
- * tlvinfo_update_crc
- * This function updates the CRC-32 TLV. If there is no CRC-32 TLV,
then
- * one is added. This function should be called after each update
to the
- * EEPROM structure, to make sure the CRC is always correct.
- @eeprom: Pointer to buffer to hold the binary data. Must point to
a buffer
- * of size at least TLV_INFO_MAX_LEN.
- */
+void tlvinfo_update_crc(u8 *eeprom);
+/**
- * Validate the checksum in the provided TlvInfo EEPROM data. First,
- * verify that the TlvInfo header is valid, then make sure the last
- * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data
- * and compare it to the value stored in the EEPROM CRC-32 TLV.
- @eeprom: Pointer to buffer to hold the binary data. Must point to
a buffer
- * of size at least TLV_INFO_MAX_LEN.
- */
+bool tlvinfo_check_crc(u8 *eeprom);
#else /* !CONFIG_IS_ENABLED(CMD_TLV_EEPROM) */ static inline int read_tlv_eeprom(void *eeprom, int offset, int len, int dev) @@ -150,4 +195,16 @@ static inline bool is_valid_tlvinfo_header(struct tlvinfo_header *hdr) (be16_to_cpu(hdr->totallen) <= TLV_TOTAL_LEN_MAX)); } +/**
- * 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_tlvinfo_entry(struct tlvinfo_tlv *tlv) +{ + return((tlv->type != 0x00) && (tlv->type != 0xFF)); +}
#endif /* __TLV_EEPROM_H_ */
Viele Grüße, Stefan Roese
Viele Grüße, Stefan Roese
\o/
Am 03.05.22 um 13:54 schrieb Stefan Roese:
Hi Josua,
On 03.05.22 09:17, Josua Mayer wrote:
Am 03.05.22 um 09:16 schrieb Stefan Roese:
On 02.05.22 16:18, Josua Mayer wrote:
- prog_eeprom: write_tlvinfo_tlv_eeprom
- update_crc: tlvinfo_update_crc
- is_valid_tlv: is_valid_tlvinfo_entry
- is_checksum_valid: tlvinfo_check_crc
So while creating a new API it makes sense to prepend the function names identical IMHO to not "pollute" the namespace. Something like
- tlv_is_valid_entry
- tlv_check_crc
...
Just examples, you get the idea.
Yes. The hard part in this particular implementation is that the naming is not consistent.
The most sense I could make is that prefix tlvinfo indicates all tlv data, i.e. working with the whole structure, while tlvinfo_tlv indicates working with one data entry. Further write, read and is_ are currently prefixed in the header, but for previously static functions in the C file it was put in the middle ...
I found it quite difficult to prepare for splitting off a library in a way that preserves history, i.e. diffs should still be readable for spotting mistakes.
Yes, a decent history would be welcome. But still, when going global here with a new API this should be consistant.
My view more like - patches 1-10 are not really new API, in that I am only changing what is necessary to allow splitting the code.
I was considering to at the very end do a mass-rename and come up with better naming, something like tlv_{set,get}_{blob,string,mac} tlv_find_entry tlv_{read,write}_eeprom
But this is pending a refactoring and extension of the tlv parsing code in board/solidrun/common/tlv_data.*, to figure out what is required or useful.
So your plan is to this: a) Get this patchset included b) Use it in board specific code, e.g. solidrun c) Do the mass-rename
Is this correct?
This is close. I would say 1) get the split merged 2) rebase board-specific code 2a) figure out what kinds of set/get/add/remove functions are useful 3) redesign api - there are inconsistencies with the order of function arguments - read/write functions imo should use the tlv header to determine size, not a function argument - at least one of the tlv data types can appear multiple times, existing code does not support this 4) submit any renames and extensions to the tlv library 5) submit board-specific use of tlv eeprom data
If yes, why is it better to do the renaming at the end?
It is very difficult to work on a patch-set that touches the same code before and after moving it to a different file, which I found myself doing a lot while prototyping this. So if now I went ahead and figured out proper names and purposes for all functions that I think should be the tlv api, then I have to divide that into those parts that are renames or refactoring of existing functionality, and those parts that are strictly new - putting the former before splitting off the library, and the latter to after.
I am not sure I can manage this level of complexity.
- Josua Mayer
Thanks, Stefan
Thanks, Stefan
Signed-off-by: Josua Mayer josua@solid-run.com
cmd/tlv_eeprom.c | 56 +++++++++++++++---------------------------- include/tlv_eeprom.h | 57 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 37 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 00c5b5f840..1b4f2537f6 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR; #define MAX_TLV_DEVICES 2 /* File scope function prototypes */ -static bool is_checksum_valid(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(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); @@ -58,18 +54,6 @@ static inline bool is_digit(char c) return (c >= '0' && c <= '9'); } -/**
- * 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(struct tlvinfo_tlv *tlv) -{ - return((tlv->type != 0x00) && (tlv->type != 0xFF)); -}
/** * is_hex * @@ -83,14 +67,12 @@ static inline u8 is_hex(char p) } /**
- * is_checksum_valid
* Validate the checksum in the provided TlvInfo EEPROM data. First, * verify that the TlvInfo header is valid, then make sure the last * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data * and compare it to the value stored in the EEPROM CRC-32 TLV. */ -static bool is_checksum_valid(u8 *eeprom) +bool tlvinfo_check_crc(u8 *eeprom) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_crc; @@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom) // If the contents are invalid, start over with default contents if (!is_valid_tlvinfo_header(eeprom_hdr) || - !is_checksum_valid(eeprom)) { + !tlvinfo_check_crc(eeprom)) { strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); eeprom_hdr->version = TLV_INFO_VERSION; eeprom_hdr->totallen = cpu_to_be16(0); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); } #ifdef DEBUG @@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom) tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (curr_tlv < tlv_end) { eeprom_tlv = to_entry(&eeprom[curr_tlv]); - if (!is_valid_tlv(eeprom_tlv)) { + if (!is_valid_tlvinfo_entry(eeprom_tlv)) { printf("Invalid TLV field starting at EEPROM offset %d\n", curr_tlv); return; @@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom) } printf("Checksum is %s.\n", - is_checksum_valid(eeprom) ? "valid" : "invalid"); + tlvinfo_check_crc(eeprom) ? "valid" : "invalid"); #ifdef DEBUG printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN); @@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv) } /**
- * update_crc
- * tlvinfo_update_crc
* * This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then * one is added. This function should be called after each update to the * EEPROM structure, to make sure the CRC is always correct. */ -static void update_crc(u8 *eeprom) +void tlvinfo_update_crc(u8 *eeprom) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_crc; @@ -376,20 +358,20 @@ static void update_crc(u8 *eeprom) } /**
- * prog_eeprom
- * write_tlvinfo_tlv_eeprom
*
- * Write the EEPROM data from CPU memory to the hardware.
- * Write the TLV data from CPU memory to the hardware.
*/ -static int prog_eeprom(int devnum, u8 *eeprom) +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev) { int ret = 0; struct tlvinfo_header *eeprom_hdr = to_header(eeprom); int eeprom_len; - update_crc(eeprom); + tlvinfo_update_crc(eeprom); eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); - ret = write_tlv_eeprom(eeprom, eeprom_len, devnum); + ret = write_tlv_eeprom(eeprom, eeprom_len, dev); if (ret) { printf("Programming failed.\n"); return -1; @@ -462,13 +444,13 @@ 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(current_dev, eeprom); + write_tlvinfo_tlv_eeprom(eeprom, current_dev); break; case 'e': /* erase */ strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); eeprom_hdr->version = TLV_INFO_VERSION; eeprom_hdr->totallen = cpu_to_be16(0); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); printf("EEPROM data in memory reset.\n"); break; case 'l': /* list */ @@ -549,7 +531,7 @@ U_BOOT_CMD(tlv_eeprom, 4, 1, do_tlv_eeprom, * An offset from the beginning of the EEPROM is returned in the * eeprom_index parameter if the TLV is found. */ -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_tlv; @@ -561,7 +543,7 @@ static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) eeprom_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (*eeprom_index < eeprom_end) { eeprom_tlv = to_entry(&eeprom[*eeprom_index]); - if (!is_valid_tlv(eeprom_tlv)) + if (!is_valid_tlvinfo_entry(eeprom_tlv)) return false; if (eeprom_tlv->type == tcode) return true; @@ -594,7 +576,7 @@ static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - tlength); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); return true; } return false; @@ -695,7 +677,7 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) // Update the total length and calculate (add) a new CRC-32 TLV eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + ENT_SIZE + new_tlv_len); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); return true; } @@ -986,7 +968,7 @@ int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, be16_to_cpu(tlv_hdr->totallen), dev_num); if (ret < 0) return ret; - if (!is_checksum_valid(eeprom)) + if (!tlvinfo_check_crc(eeprom)) return -EINVAL; *hdr = tlv_hdr; diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h index fd45e5f6eb..30626a1067 100644 --- a/include/tlv_eeprom.h +++ b/include/tlv_eeprom.h @@ -111,6 +111,51 @@ int write_tlv_eeprom(void *eeprom, int len, int dev); int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, struct tlvinfo_tlv **first_entry, int dev); +/**
- Write TLV data to the EEPROM.
- Only writes length of actual tlv data
- updates checksum
- @eeprom: Pointer to buffer to hold the binary data. Must point
to a buffer
- * of size at least TLV_INFO_MAX_LEN.
- @dev : EEPROM device to write
- */
+int write_tlvinfo_tlv_eeprom(void *eeprom, int dev);
+/**
- * tlvinfo_find_tlv
- * This function finds the TLV with the supplied code in the EERPOM.
- * An offset from the beginning of the EEPROM is returned in the
- * eeprom_index parameter if the TLV is found.
- */
+bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index);
+/**
- * tlvinfo_update_crc
- * This function updates the CRC-32 TLV. If there is no CRC-32
TLV, then
- * one is added. This function should be called after each update
to the
- * EEPROM structure, to make sure the CRC is always correct.
- @eeprom: Pointer to buffer to hold the binary data. Must point
to a buffer
- * of size at least TLV_INFO_MAX_LEN.
- */
+void tlvinfo_update_crc(u8 *eeprom);
+/**
- * Validate the checksum in the provided TlvInfo EEPROM data. First,
- * verify that the TlvInfo header is valid, then make sure the last
- * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data
- * and compare it to the value stored in the EEPROM CRC-32 TLV.
- @eeprom: Pointer to buffer to hold the binary data. Must point
to a buffer
- * of size at least TLV_INFO_MAX_LEN.
- */
+bool tlvinfo_check_crc(u8 *eeprom);
#else /* !CONFIG_IS_ENABLED(CMD_TLV_EEPROM) */ static inline int read_tlv_eeprom(void *eeprom, int offset, int len, int dev) @@ -150,4 +195,16 @@ static inline bool is_valid_tlvinfo_header(struct tlvinfo_header *hdr) (be16_to_cpu(hdr->totallen) <= TLV_TOTAL_LEN_MAX)); } +/**
- * 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_tlvinfo_entry(struct tlvinfo_tlv *tlv) +{ + return((tlv->type != 0x00) && (tlv->type != 0xFF)); +}
#endif /* __TLV_EEPROM_H_ */
Viele Grüße, Stefan Roese
Viele Grüße, Stefan Roese
Hi Josua,
On 03.05.22 21:09, Josua Mayer wrote:
\o/
Am 03.05.22 um 13:54 schrieb Stefan Roese:
Hi Josua,
On 03.05.22 09:17, Josua Mayer wrote:
Am 03.05.22 um 09:16 schrieb Stefan Roese:
On 02.05.22 16:18, Josua Mayer wrote:
- prog_eeprom: write_tlvinfo_tlv_eeprom
- update_crc: tlvinfo_update_crc
- is_valid_tlv: is_valid_tlvinfo_entry
- is_checksum_valid: tlvinfo_check_crc
So while creating a new API it makes sense to prepend the function names identical IMHO to not "pollute" the namespace. Something like
- tlv_is_valid_entry
- tlv_check_crc
...
Just examples, you get the idea.
Yes. The hard part in this particular implementation is that the naming is not consistent.
The most sense I could make is that prefix tlvinfo indicates all tlv data, i.e. working with the whole structure, while tlvinfo_tlv indicates working with one data entry. Further write, read and is_ are currently prefixed in the header, but for previously static functions in the C file it was put in the middle ...
I found it quite difficult to prepare for splitting off a library in a way that preserves history, i.e. diffs should still be readable for spotting mistakes.
Yes, a decent history would be welcome. But still, when going global here with a new API this should be consistant.
My view more like - patches 1-10 are not really new API, in that I am only changing what is necessary to allow splitting the code.
I was considering to at the very end do a mass-rename and come up with better naming, something like tlv_{set,get}_{blob,string,mac} tlv_find_entry tlv_{read,write}_eeprom
But this is pending a refactoring and extension of the tlv parsing code in board/solidrun/common/tlv_data.*, to figure out what is required or useful.
So your plan is to this: a) Get this patchset included b) Use it in board specific code, e.g. solidrun c) Do the mass-rename
Is this correct?
This is close. I would say
- get the split merged
- rebase board-specific code
2a) figure out what kinds of set/get/add/remove functions are useful 3) redesign api - there are inconsistencies with the order of function arguments - read/write functions imo should use the tlv header to determine size, not a function argument - at least one of the tlv data types can appear multiple times, existing code does not support this 4) submit any renames and extensions to the tlv library 5) submit board-specific use of tlv eeprom data
If yes, why is it better to do the renaming at the end?
It is very difficult to work on a patch-set that touches the same code before and after moving it to a different file, which I found myself doing a lot while prototyping this. So if now I went ahead and figured out proper names and purposes for all functions that I think should be the tlv api, then I have to divide that into those parts that are renames or refactoring of existing functionality, and those parts that are strictly new - putting the former before splitting off the library, and the latter to after.
I am not sure I can manage this level of complexity.
To cut it short, if you plan to rename the API to some consitant naming in the end, then I am okay with moving forward without a renaming at this early stage.
Thanks, Stefan
- Josua Mayer
Thanks, Stefan
Thanks, Stefan
Signed-off-by: Josua Mayer josua@solid-run.com
cmd/tlv_eeprom.c | 56 +++++++++++++++---------------------------- include/tlv_eeprom.h | 57 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 37 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 00c5b5f840..1b4f2537f6 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR; #define MAX_TLV_DEVICES 2 /* File scope function prototypes */ -static bool is_checksum_valid(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(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); @@ -58,18 +54,6 @@ static inline bool is_digit(char c) return (c >= '0' && c <= '9'); } -/**
- * 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(struct tlvinfo_tlv *tlv) -{ - return((tlv->type != 0x00) && (tlv->type != 0xFF)); -}
/** * is_hex * @@ -83,14 +67,12 @@ static inline u8 is_hex(char p) } /**
- * is_checksum_valid
* Validate the checksum in the provided TlvInfo EEPROM data. First, * verify that the TlvInfo header is valid, then make sure the last * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data * and compare it to the value stored in the EEPROM CRC-32 TLV. */ -static bool is_checksum_valid(u8 *eeprom) +bool tlvinfo_check_crc(u8 *eeprom) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_crc; @@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom) // If the contents are invalid, start over with default contents if (!is_valid_tlvinfo_header(eeprom_hdr) || - !is_checksum_valid(eeprom)) { + !tlvinfo_check_crc(eeprom)) { strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); eeprom_hdr->version = TLV_INFO_VERSION; eeprom_hdr->totallen = cpu_to_be16(0); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); } #ifdef DEBUG @@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom) tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (curr_tlv < tlv_end) { eeprom_tlv = to_entry(&eeprom[curr_tlv]); - if (!is_valid_tlv(eeprom_tlv)) { + if (!is_valid_tlvinfo_entry(eeprom_tlv)) { printf("Invalid TLV field starting at EEPROM offset %d\n", curr_tlv); return; @@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom) } printf("Checksum is %s.\n", - is_checksum_valid(eeprom) ? "valid" : "invalid"); + tlvinfo_check_crc(eeprom) ? "valid" : "invalid"); #ifdef DEBUG printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN); @@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv) } /**
- * update_crc
- * tlvinfo_update_crc
* * This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then * one is added. This function should be called after each update to the * EEPROM structure, to make sure the CRC is always correct. */ -static void update_crc(u8 *eeprom) +void tlvinfo_update_crc(u8 *eeprom) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_crc; @@ -376,20 +358,20 @@ static void update_crc(u8 *eeprom) } /**
- * prog_eeprom
- * write_tlvinfo_tlv_eeprom
*
- * Write the EEPROM data from CPU memory to the hardware.
- * Write the TLV data from CPU memory to the hardware.
*/ -static int prog_eeprom(int devnum, u8 *eeprom) +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev) { int ret = 0; struct tlvinfo_header *eeprom_hdr = to_header(eeprom); int eeprom_len; - update_crc(eeprom); + tlvinfo_update_crc(eeprom); eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); - ret = write_tlv_eeprom(eeprom, eeprom_len, devnum); + ret = write_tlv_eeprom(eeprom, eeprom_len, dev); if (ret) { printf("Programming failed.\n"); return -1; @@ -462,13 +444,13 @@ 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(current_dev, eeprom); + write_tlvinfo_tlv_eeprom(eeprom, current_dev); break; case 'e': /* erase */ strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); eeprom_hdr->version = TLV_INFO_VERSION; eeprom_hdr->totallen = cpu_to_be16(0); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); printf("EEPROM data in memory reset.\n"); break; case 'l': /* list */ @@ -549,7 +531,7 @@ U_BOOT_CMD(tlv_eeprom, 4, 1, do_tlv_eeprom, * An offset from the beginning of the EEPROM is returned in the * eeprom_index parameter if the TLV is found. */ -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_tlv; @@ -561,7 +543,7 @@ static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) eeprom_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (*eeprom_index < eeprom_end) { eeprom_tlv = to_entry(&eeprom[*eeprom_index]); - if (!is_valid_tlv(eeprom_tlv)) + if (!is_valid_tlvinfo_entry(eeprom_tlv)) return false; if (eeprom_tlv->type == tcode) return true; @@ -594,7 +576,7 @@ static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - tlength); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); return true; } return false; @@ -695,7 +677,7 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) // Update the total length and calculate (add) a new CRC-32 TLV eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + ENT_SIZE + new_tlv_len); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); return true; } @@ -986,7 +968,7 @@ int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, be16_to_cpu(tlv_hdr->totallen), dev_num); if (ret < 0) return ret; - if (!is_checksum_valid(eeprom)) + if (!tlvinfo_check_crc(eeprom)) return -EINVAL; *hdr = tlv_hdr; diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h index fd45e5f6eb..30626a1067 100644 --- a/include/tlv_eeprom.h +++ b/include/tlv_eeprom.h @@ -111,6 +111,51 @@ int write_tlv_eeprom(void *eeprom, int len, int dev); int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, struct tlvinfo_tlv **first_entry, int dev); +/**
- Write TLV data to the EEPROM.
- Only writes length of actual tlv data
- updates checksum
- @eeprom: Pointer to buffer to hold the binary data. Must point
to a buffer
- * of size at least TLV_INFO_MAX_LEN.
- @dev : EEPROM device to write
- */
+int write_tlvinfo_tlv_eeprom(void *eeprom, int dev);
+/**
- * tlvinfo_find_tlv
- * This function finds the TLV with the supplied code in the EERPOM.
- * An offset from the beginning of the EEPROM is returned in the
- * eeprom_index parameter if the TLV is found.
- */
+bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index);
+/**
- * tlvinfo_update_crc
- * This function updates the CRC-32 TLV. If there is no CRC-32
TLV, then
- * one is added. This function should be called after each update
to the
- * EEPROM structure, to make sure the CRC is always correct.
- @eeprom: Pointer to buffer to hold the binary data. Must point
to a buffer
- * of size at least TLV_INFO_MAX_LEN.
- */
+void tlvinfo_update_crc(u8 *eeprom);
+/**
- * Validate the checksum in the provided TlvInfo EEPROM data. First,
- * verify that the TlvInfo header is valid, then make sure the last
- * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data
- * and compare it to the value stored in the EEPROM CRC-32 TLV.
- @eeprom: Pointer to buffer to hold the binary data. Must point
to a buffer
- * of size at least TLV_INFO_MAX_LEN.
- */
+bool tlvinfo_check_crc(u8 *eeprom);
#else /* !CONFIG_IS_ENABLED(CMD_TLV_EEPROM) */ static inline int read_tlv_eeprom(void *eeprom, int offset, int len, int dev) @@ -150,4 +195,16 @@ static inline bool is_valid_tlvinfo_header(struct tlvinfo_header *hdr) (be16_to_cpu(hdr->totallen) <= TLV_TOTAL_LEN_MAX)); } +/**
- * 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_tlvinfo_entry(struct tlvinfo_tlv *tlv) +{ + return((tlv->type != 0x00) && (tlv->type != 0xFF)); +}
#endif /* __TLV_EEPROM_H_ */
Viele Grüße, Stefan Roese
Viele Grüße, Stefan Roese
Viele Grüße, Stefan Roese
tlv_eeprom exposed functions are independent from platforms, hence no stubs are required.
Signed-off-by: Josua Mayer josua@solid-run.com --- include/tlv_eeprom.h | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-)
diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h index 30626a1067..55fd72d6d2 100644 --- a/include/tlv_eeprom.h +++ b/include/tlv_eeprom.h @@ -65,7 +65,8 @@ struct __attribute__ ((__packed__)) tlvinfo_tlv { #define TLV_CODE_VENDOR_EXT 0xFD #define TLV_CODE_CRC_32 0xFE
-#if CONFIG_IS_ENABLED(CMD_TLV_EEPROM) +/* how many EEPROMs can be used */ +#define TLV_MAX_DEVICES 2
/** * read_tlv_eeprom - Read the EEPROM binary data from the hardware @@ -156,27 +157,6 @@ void tlvinfo_update_crc(u8 *eeprom); */ bool tlvinfo_check_crc(u8 *eeprom);
-#else /* !CONFIG_IS_ENABLED(CMD_TLV_EEPROM) */ - -static inline int read_tlv_eeprom(void *eeprom, int offset, int len, int dev) -{ - return -ENOSYS; -} - -static inline int write_tlv_eeprom(void *eeprom, int len) -{ - return -ENOSYS; -} - -static inline int -read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, - struct tlvinfo_tlv **first_entry, int dev) -{ - return -ENOSYS; -} - -#endif /* CONFIG_IS_ENABLED(CMD_TLV_EEPROM) */ - /** * is_valid_tlvinfo_header *
In preparation of splitting the tlv_eeprom command into a separate library, add function declarations and defines used by the command logic to the tlv_eeprom header file.
Signed-off-by: Josua Mayer josua@solid-run.com --- cmd/tlv_eeprom.c | 59 ++++++++++++++++++++------------------------ include/tlv_eeprom.h | 29 ++++++++++++++++++++-- 2 files changed, 54 insertions(+), 34 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 1b4f2537f6..57468edb1c 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -31,8 +31,6 @@ DECLARE_GLOBAL_DATA_PTR; 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 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); @@ -46,9 +44,6 @@ static struct udevice *tlv_devices[MAX_TLV_DEVICES]; #define to_header(p) ((struct tlvinfo_header *)p) #define to_entry(p) ((struct tlvinfo_tlv *)p)
-#define HDR_SIZE sizeof(struct tlvinfo_header) -#define ENT_SIZE sizeof(struct tlvinfo_tlv) - static inline bool is_digit(char c) { return (c >= '0' && c <= '9'); @@ -84,14 +79,14 @@ bool tlvinfo_check_crc(u8 *eeprom) return false;
// Is the last TLV a CRC? - eeprom_crc = to_entry(&eeprom[HDR_SIZE + - be16_to_cpu(eeprom_hdr->totallen) - (ENT_SIZE + 4)]); + eeprom_crc = to_entry(&eeprom[TLV_INFO_HEADER_SIZE + + be16_to_cpu(eeprom_hdr->totallen) - (TLV_INFO_ENTRY_SIZE + 4)]); if (eeprom_crc->type != TLV_CODE_CRC_32 || eeprom_crc->length != 4) return false;
// Calculate the checksum calc_crc = crc32(0, (void *)eeprom, - HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen) - 4); + TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen) - 4); stored_crc = (eeprom_crc->value[0] << 24) | (eeprom_crc->value[1] << 16) | (eeprom_crc->value[2] << 8) | @@ -108,13 +103,13 @@ static int read_eeprom(int devnum, u8 *eeprom) { int ret; struct tlvinfo_header *eeprom_hdr = to_header(eeprom); - struct tlvinfo_tlv *eeprom_tlv = to_entry(&eeprom[HDR_SIZE]); + struct tlvinfo_tlv *eeprom_tlv = to_entry(&eeprom[TLV_INFO_HEADER_SIZE]);
/* Read the header */ - ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, devnum); + ret = read_tlv_eeprom((void *)eeprom_hdr, 0, TLV_INFO_HEADER_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, + ret = read_tlv_eeprom((void *)eeprom_tlv, TLV_INFO_HEADER_SIZE, be16_to_cpu(eeprom_hdr->totallen), devnum);
// If the contents are invalid, start over with default contents @@ -161,8 +156,8 @@ static void show_eeprom(int devnum, u8 *eeprom)
printf("TLV Name Code Len Value\n"); printf("-------------------- ---- --- -----\n"); - curr_tlv = HDR_SIZE; - tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); + curr_tlv = TLV_INFO_HEADER_SIZE; + tlv_end = TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (curr_tlv < tlv_end) { eeprom_tlv = to_entry(&eeprom[curr_tlv]); if (!is_valid_tlvinfo_entry(eeprom_tlv)) { @@ -171,7 +166,7 @@ static void show_eeprom(int devnum, u8 *eeprom) return; } decode_tlv(eeprom_tlv); - curr_tlv += ENT_SIZE + eeprom_tlv->length; + curr_tlv += TLV_INFO_ENTRY_SIZE + eeprom_tlv->length; }
printf("Checksum is %s.\n", @@ -339,10 +334,10 @@ void tlvinfo_update_crc(u8 *eeprom) if (!tlvinfo_find_tlv(eeprom, TLV_CODE_CRC_32, &eeprom_index)) { unsigned int totallen = be16_to_cpu(eeprom_hdr->totallen);
- if ((totallen + ENT_SIZE + 4) > TLV_TOTAL_LEN_MAX) + if ((totallen + TLV_INFO_ENTRY_SIZE + 4) > TLV_TOTAL_LEN_MAX) return; - eeprom_index = HDR_SIZE + totallen; - eeprom_hdr->totallen = cpu_to_be16(totallen + ENT_SIZE + 4); + eeprom_index = TLV_INFO_HEADER_SIZE + totallen; + eeprom_hdr->totallen = cpu_to_be16(totallen + TLV_INFO_ENTRY_SIZE + 4); } eeprom_crc = to_entry(&eeprom[eeprom_index]); eeprom_crc->type = TLV_CODE_CRC_32; @@ -350,7 +345,7 @@ void tlvinfo_update_crc(u8 *eeprom)
// Calculate the checksum calc_crc = crc32(0, (void *)eeprom, - HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen) - 4); + TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen) - 4); eeprom_crc->value[0] = (calc_crc >> 24) & 0xFF; eeprom_crc->value[1] = (calc_crc >> 16) & 0xFF; eeprom_crc->value[2] = (calc_crc >> 8) & 0xFF; @@ -370,7 +365,7 @@ int write_tlvinfo_tlv_eeprom(void *eeprom, int dev)
tlvinfo_update_crc(eeprom);
- eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); + eeprom_len = TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen); ret = write_tlv_eeprom(eeprom, eeprom_len, dev); if (ret) { printf("Programming failed.\n"); @@ -539,15 +534,15 @@ bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index)
// Search through the TLVs, looking for the first one which matches the // supplied type code. - *eeprom_index = HDR_SIZE; - eeprom_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); + *eeprom_index = TLV_INFO_HEADER_SIZE; + eeprom_end = TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (*eeprom_index < eeprom_end) { eeprom_tlv = to_entry(&eeprom[*eeprom_index]); if (!is_valid_tlvinfo_entry(eeprom_tlv)) return false; if (eeprom_tlv->type == tcode) return true; - *eeprom_index += ENT_SIZE + eeprom_tlv->length; + *eeprom_index += TLV_INFO_ENTRY_SIZE + eeprom_tlv->length; } return(false); } @@ -558,7 +553,7 @@ bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) * This function deletes the TLV with the specified type code from the * EEPROM. */ -static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) +bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) { int eeprom_index; int tlength; @@ -568,9 +563,9 @@ static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) // Find the TLV and then move all following TLVs "forward" if (tlvinfo_find_tlv(eeprom, code, &eeprom_index)) { eeprom_tlv = to_entry(&eeprom[eeprom_index]); - tlength = ENT_SIZE + eeprom_tlv->length; + tlength = TLV_INFO_ENTRY_SIZE + eeprom_tlv->length; memcpy(&eeprom[eeprom_index], &eeprom[eeprom_index + tlength], - HDR_SIZE + + TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen) - eeprom_index - tlength); eeprom_hdr->totallen = @@ -589,7 +584,7 @@ static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) * the format in which it will be stored in the EEPROM. */ #define MAX_TLV_VALUE_LEN 256 -static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) +bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_tlv; @@ -656,7 +651,7 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) }
// Is there room for this TLV? - if ((be16_to_cpu(eeprom_hdr->totallen) + ENT_SIZE + new_tlv_len) > + if ((be16_to_cpu(eeprom_hdr->totallen) + TLV_INFO_ENTRY_SIZE + new_tlv_len) > TLV_TOTAL_LEN_MAX) { printf("ERROR: There is not enough room in the EERPOM to save data.\n"); return false; @@ -666,9 +661,9 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) if (tlvinfo_find_tlv(eeprom, TLV_CODE_CRC_32, &eeprom_index)) eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - - ENT_SIZE - 4); + TLV_INFO_ENTRY_SIZE - 4); else - eeprom_index = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); + eeprom_index = TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen); eeprom_tlv = to_entry(&eeprom[eeprom_index]); eeprom_tlv->type = tcode; eeprom_tlv->length = new_tlv_len; @@ -676,7 +671,7 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval)
// Update the total length and calculate (add) a new CRC-32 TLV eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + - ENT_SIZE + new_tlv_len); + TLV_INFO_ENTRY_SIZE + new_tlv_len); tlvinfo_update_crc(eeprom);
return true; @@ -954,7 +949,7 @@ int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, struct tlvinfo_tlv *tlv_ent;
/* Read TLV header */ - ret = read_tlv_eeprom(eeprom, 0, HDR_SIZE, dev_num); + ret = read_tlv_eeprom(eeprom, 0, TLV_INFO_HEADER_SIZE, dev_num); if (ret < 0) return ret;
@@ -964,7 +959,7 @@ int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr,
/* Read TLV entries */ tlv_ent = to_entry(&tlv_hdr[1]); - ret = read_tlv_eeprom(tlv_ent, HDR_SIZE, + ret = read_tlv_eeprom(tlv_ent, TLV_INFO_HEADER_SIZE, be16_to_cpu(tlv_hdr->totallen), dev_num); if (ret < 0) return ret; diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h index 55fd72d6d2..dc7952da6b 100644 --- a/include/tlv_eeprom.h +++ b/include/tlv_eeprom.h @@ -24,11 +24,11 @@ struct __attribute__ ((__packed__)) tlvinfo_header { };
// Header Field Constants +#define TLV_INFO_HEADER_SIZE sizeof(struct tlvinfo_header) #define TLV_INFO_ID_STRING "TlvInfo" #define TLV_INFO_VERSION 0x01 #define TLV_INFO_MAX_LEN 2048 -#define TLV_TOTAL_LEN_MAX (TLV_INFO_MAX_LEN - \ - sizeof(struct tlvinfo_header)) +#define TLV_TOTAL_LEN_MAX (TLV_INFO_MAX_LEN - TLV_INFO_HEADER_SIZE)
/* * TlvInfo TLV: Layout of a TLV field @@ -39,6 +39,7 @@ struct __attribute__ ((__packed__)) tlvinfo_tlv { u8 value[0]; };
+#define TLV_INFO_ENTRY_SIZE sizeof(struct tlvinfo_tlv) /* Maximum length of a TLV value in bytes */ #define TLV_VALUE_MAX_LEN 255
@@ -134,6 +135,30 @@ int write_tlvinfo_tlv_eeprom(void *eeprom, int dev); */ bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index);
+/** + * tlvinfo_add_tlv + * + * This function adds a TLV to the EEPROM, converting the value (a string) to + * the format in which it will be stored in the EEPROM. + * @eeprom: Pointer to buffer to hold the binary data. Must point to a buffer + * of size at least TLV_INFO_MAX_LEN. + * @code The TLV Code for the new entry. + * @eeprom_index success offset into EEPROM where the new entry has been stored + * + */ +bool tlvinfo_add_tlv(u8 *eeprom, int code, char *strval); + +/** + * tlvinfo_delete_tlv + * + * This function deletes the TLV with the specified type code from the + * EEPROM. + * @eeprom: Pointer to buffer to hold the binary data. Must point to a buffer + * of size at least TLV_INFO_MAX_LEN. + * @code The TLV Code of the entry to delete. + */ +bool tlvinfo_delete_tlv(u8 *eeprom, u8 code); + /** * tlvinfo_update_crc *
The tlv_eeprom command logic checks the static tlv_devices array to validate the eeprom number. This array will be move to a separate tlv library. Hide this access behind a new function exists_tlv_eeprom.
Signed-off-by: Josua Mayer josua@solid-run.com --- cmd/tlv_eeprom.c | 12 ++++++++++-- include/tlv_eeprom.h | 7 +++++++ 2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 57468edb1c..f51a9666bf 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -44,6 +44,14 @@ static struct udevice *tlv_devices[MAX_TLV_DEVICES]; #define to_header(p) ((struct tlvinfo_header *)p) #define to_entry(p) ((struct tlvinfo_tlv *)p)
+/** + * Check whether eeprom device exists. + */ +bool exists_tlv_eeprom(int dev) +{ + return dev < TLV_MAX_DEVICES && tlv_devices[dev] != 0; +} + static inline bool is_digit(char c) { return (c >= '0' && c <= '9'); @@ -481,7 +489,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) unsigned int devnum;
devnum = simple_strtoul(argv[2], NULL, 0); - if (devnum > MAX_TLV_DEVICES || !tlv_devices[devnum]) { + if (!exists_tlv_eeprom(devnum)) { printf("Invalid device number\n"); return 0; } @@ -866,7 +874,7 @@ static void show_tlv_devices(int current_dev) unsigned int dev;
for (dev = 0; dev < MAX_TLV_DEVICES; dev++) - if (tlv_devices[dev]) + if (exists_tlv_eeprom(dev)) printf("TLV: %u%s\n", dev, (dev == current_dev) ? " (*)" : ""); } diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h index dc7952da6b..c81c58837d 100644 --- a/include/tlv_eeprom.h +++ b/include/tlv_eeprom.h @@ -69,6 +69,13 @@ struct __attribute__ ((__packed__)) tlvinfo_tlv { /* how many EEPROMs can be used */ #define TLV_MAX_DEVICES 2
+/** + * Check whether eeprom device exists. + * + * @dev: EEPROM device to check. + */ +bool exists_tlv_eeprom(int dev); + /** * read_tlv_eeprom - Read the EEPROM binary data from the hardware * @eeprom: Pointer to buffer to hold the binary data
MAX_TLV_DEVICES defined in C, and TLV_MAX_DEVICES defined in the header serve the same purpose. Replace all occurences of the former by the latter.
Signed-off-by: Josua Mayer josua@solid-run.com --- cmd/tlv_eeprom.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index f51a9666bf..c110927cb5 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -25,8 +25,6 @@
DECLARE_GLOBAL_DATA_PTR;
-#define MAX_TLV_DEVICES 2 - /* File scope function prototypes */ static int read_eeprom(int devnum, u8 *eeprom); static void show_eeprom(int devnum, u8 *eeprom); @@ -39,7 +37,7 @@ static void show_tlv_devices(int current_dev); /* The EERPOM contents after being read into memory */ static u8 eeprom[TLV_INFO_MAX_LEN];
-static struct udevice *tlv_devices[MAX_TLV_DEVICES]; +static struct udevice *tlv_devices[TLV_MAX_DEVICES];
#define to_header(p) ((struct tlvinfo_header *)p) #define to_entry(p) ((struct tlvinfo_tlv *)p) @@ -873,7 +871,7 @@ static void show_tlv_devices(int current_dev) { unsigned int dev;
- for (dev = 0; dev < MAX_TLV_DEVICES; dev++) + for (dev = 0; dev < TLV_MAX_DEVICES; dev++) if (exists_tlv_eeprom(dev)) printf("TLV: %u%s\n", dev, (dev == current_dev) ? " (*)" : ""); @@ -890,7 +888,7 @@ static int find_tlv_devices(struct udevice **tlv_devices_p) ret = uclass_next_device_check(&dev)) { if (ret == 0) tlv_devices_p[count_dev++] = dev; - if (count_dev >= MAX_TLV_DEVICES) + if (count_dev >= TLV_MAX_DEVICES) break; }
@@ -899,7 +897,7 @@ static int find_tlv_devices(struct udevice **tlv_devices_p)
static struct udevice *find_tlv_device_by_index(int dev_num) { - struct udevice *local_tlv_devices[MAX_TLV_DEVICES] = {}; + struct udevice *local_tlv_devices[TLV_MAX_DEVICES] = {}; struct udevice **tlv_devices_p; int ret;
@@ -926,7 +924,7 @@ int read_tlv_eeprom(void *eeprom, int offset, int len, int dev_num) { struct udevice *dev;
- if (dev_num >= MAX_TLV_DEVICES) + if (dev_num >= TLV_MAX_DEVICES) return -EINVAL;
dev = find_tlv_device_by_index(dev_num);
While each of the previous individual changes is small, accumulated they amount to a substantial effort. Explicitly add a Copyright declaration.
Signed-off-by: Josua Mayer josua@solid-run.com --- cmd/tlv_eeprom.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index c110927cb5..c66116b2c4 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -7,6 +7,7 @@ * Copyright (C) 2014 Srideep srideep_devireddy@dell.com * Copyright (C) 2013 Miles Tseng miles_tseng@accton.com * Copyright (C) 2014,2016 david_yang david_yang@accton.com + * Copyright (C) 2022 Josua Mayer josua@solid-run.com */
#include <common.h>
The eeprom command includes functions for reading and writing tlv-formatted data from an eeprom, as well as an implementation of the cli command tlv_eeprom.
Split off the parsing, read and write into a standalone tlv library.
Signed-off-by: Josua Mayer josua@solid-run.com --- cmd/Kconfig | 2 + cmd/tlv_eeprom.c | 742 +----------------------------------------- lib/Kconfig | 2 + lib/Makefile | 2 + lib/tlv/Kconfig | 15 + lib/tlv/Makefile | 5 + lib/tlv/tlv_eeprom.c | 750 +++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 786 insertions(+), 732 deletions(-) create mode 100644 lib/tlv/Kconfig create mode 100644 lib/tlv/Makefile create mode 100644 lib/tlv/tlv_eeprom.c
diff --git a/cmd/Kconfig b/cmd/Kconfig index 2b575a2b42..821b5e9d6b 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -166,6 +166,7 @@ config CMD_REGINFO
config CMD_TLV_EEPROM bool "tlv_eeprom" + select EEPROM_TLV_LIB depends on I2C_EEPROM help Display and program the system EEPROM data block in ONIE Tlvinfo @@ -173,6 +174,7 @@ config CMD_TLV_EEPROM
config SPL_CMD_TLV_EEPROM bool "tlv_eeprom for SPL" + select SPL_EEPROM_TLV_LIB depends on SPL_I2C_EEPROM select SPL_DRIVERS_MISC help diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index c66116b2c4..99b79cad8b 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -10,131 +10,26 @@ * Copyright (C) 2022 Josua Mayer josua@solid-run.com */
-#include <common.h> #include <command.h> -#include <dm.h> -#include <i2c.h> -#include <i2c_eeprom.h> -#include <env.h> -#include <init.h> -#include <net.h> -#include <asm/global_data.h> -#include <linux/ctype.h> -#include <u-boot/crc.h> - -#include "tlv_eeprom.h" - -DECLARE_GLOBAL_DATA_PTR; +#include <linux/kernel.h> +#include <stdio.h> +#include <tlv_eeprom.h> +#include <vsprintf.h>
/* File scope function prototypes */ -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 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(int current_dev); +static inline const char *tlv_type2name(u8 type); +static void decode_tlv(struct tlvinfo_tlv *tlv); +static int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]); +static void show_tlv_code_list(void);
/* The EERPOM contents after being read into memory */ static u8 eeprom[TLV_INFO_MAX_LEN];
-static struct udevice *tlv_devices[TLV_MAX_DEVICES]; - #define to_header(p) ((struct tlvinfo_header *)p) #define to_entry(p) ((struct tlvinfo_tlv *)p)
-/** - * Check whether eeprom device exists. - */ -bool exists_tlv_eeprom(int dev) -{ - return dev < TLV_MAX_DEVICES && tlv_devices[dev] != 0; -} - -static inline bool is_digit(char c) -{ - return (c >= '0' && c <= '9'); -} - -/** - * is_hex - * - * Tests if character is an ASCII hex digit - */ -static inline u8 is_hex(char p) -{ - return (((p >= '0') && (p <= '9')) || - ((p >= 'A') && (p <= 'F')) || - ((p >= 'a') && (p <= 'f'))); -} - -/** - * Validate the checksum in the provided TlvInfo EEPROM data. First, - * verify that the TlvInfo header is valid, then make sure the last - * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data - * and compare it to the value stored in the EEPROM CRC-32 TLV. - */ -bool tlvinfo_check_crc(u8 *eeprom) -{ - struct tlvinfo_header *eeprom_hdr = to_header(eeprom); - struct tlvinfo_tlv *eeprom_crc; - unsigned int calc_crc; - unsigned int stored_crc; - - // Is the eeprom header valid? - if (!is_valid_tlvinfo_header(eeprom_hdr)) - return false; - - // Is the last TLV a CRC? - eeprom_crc = to_entry(&eeprom[TLV_INFO_HEADER_SIZE + - be16_to_cpu(eeprom_hdr->totallen) - (TLV_INFO_ENTRY_SIZE + 4)]); - if (eeprom_crc->type != TLV_CODE_CRC_32 || eeprom_crc->length != 4) - return false; - - // Calculate the checksum - calc_crc = crc32(0, (void *)eeprom, - TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen) - 4); - stored_crc = (eeprom_crc->value[0] << 24) | - (eeprom_crc->value[1] << 16) | - (eeprom_crc->value[2] << 8) | - eeprom_crc->value[3]; - return calc_crc == stored_crc; -} - -/** - * read_eeprom - * - * Read the EEPROM into memory, if it hasn't already been read. - */ -static int read_eeprom(int devnum, u8 *eeprom) -{ - int ret; - struct tlvinfo_header *eeprom_hdr = to_header(eeprom); - struct tlvinfo_tlv *eeprom_tlv = to_entry(&eeprom[TLV_INFO_HEADER_SIZE]); - - /* Read the header */ - ret = read_tlv_eeprom((void *)eeprom_hdr, 0, TLV_INFO_HEADER_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, TLV_INFO_HEADER_SIZE, - be16_to_cpu(eeprom_hdr->totallen), devnum); - - // If the contents are invalid, start over with default contents - if (!is_valid_tlvinfo_header(eeprom_hdr) || - !tlvinfo_check_crc(eeprom)) { - strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); - eeprom_hdr->version = TLV_INFO_VERSION; - eeprom_hdr->totallen = cpu_to_be16(0); - tlvinfo_update_crc(eeprom); - } - -#ifdef DEBUG - show_eeprom(devnum, eeprom); -#endif - - return ret; -} - /** * show_eeprom * @@ -323,70 +218,10 @@ static void decode_tlv(struct tlvinfo_tlv *tlv) printf("%-20s 0x%02X %3d %s\n", name, tlv->type, tlv->length, value); }
-/** - * tlvinfo_update_crc - * - * This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then - * one is added. This function should be called after each update to the - * EEPROM structure, to make sure the CRC is always correct. - */ -void tlvinfo_update_crc(u8 *eeprom) -{ - struct tlvinfo_header *eeprom_hdr = to_header(eeprom); - struct tlvinfo_tlv *eeprom_crc; - unsigned int calc_crc; - int eeprom_index; - - // Discover the CRC TLV - if (!tlvinfo_find_tlv(eeprom, TLV_CODE_CRC_32, &eeprom_index)) { - unsigned int totallen = be16_to_cpu(eeprom_hdr->totallen); - - if ((totallen + TLV_INFO_ENTRY_SIZE + 4) > TLV_TOTAL_LEN_MAX) - return; - eeprom_index = TLV_INFO_HEADER_SIZE + totallen; - eeprom_hdr->totallen = cpu_to_be16(totallen + TLV_INFO_ENTRY_SIZE + 4); - } - eeprom_crc = to_entry(&eeprom[eeprom_index]); - eeprom_crc->type = TLV_CODE_CRC_32; - eeprom_crc->length = 4; - - // Calculate the checksum - calc_crc = crc32(0, (void *)eeprom, - TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen) - 4); - eeprom_crc->value[0] = (calc_crc >> 24) & 0xFF; - eeprom_crc->value[1] = (calc_crc >> 16) & 0xFF; - eeprom_crc->value[2] = (calc_crc >> 8) & 0xFF; - eeprom_crc->value[3] = (calc_crc >> 0) & 0xFF; -} - -/** - * write_tlvinfo_tlv_eeprom - * - * Write the TLV data from CPU memory to the hardware. - */ -int write_tlvinfo_tlv_eeprom(void *eeprom, int dev) -{ - int ret = 0; - struct tlvinfo_header *eeprom_hdr = to_header(eeprom); - int eeprom_len; - - tlvinfo_update_crc(eeprom); - - eeprom_len = TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen); - ret = write_tlv_eeprom(eeprom, eeprom_len, dev); - if (ret) { - printf("Programming failed.\n"); - return -1; - } - - printf("Programming passed.\n"); - return 0; -} - /** * show_tlv_code_list - Display the list of TLV codes and names */ -void show_tlv_code_list(void) +static void show_tlv_code_list(void) { int i;
@@ -404,7 +239,7 @@ void show_tlv_code_list(void) * * This function implements the tlv_eeprom command. */ -int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) +static 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); @@ -526,348 +361,6 @@ U_BOOT_CMD(tlv_eeprom, 4, 1, do_tlv_eeprom, " - List the understood TLV codes and names.\n" );
-/** - * tlvinfo_find_tlv - * - * This function finds the TLV with the supplied code in the EERPOM. - * An offset from the beginning of the EEPROM is returned in the - * eeprom_index parameter if the TLV is found. - */ -bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) -{ - struct tlvinfo_header *eeprom_hdr = to_header(eeprom); - struct tlvinfo_tlv *eeprom_tlv; - int eeprom_end; - - // Search through the TLVs, looking for the first one which matches the - // supplied type code. - *eeprom_index = TLV_INFO_HEADER_SIZE; - eeprom_end = TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen); - while (*eeprom_index < eeprom_end) { - eeprom_tlv = to_entry(&eeprom[*eeprom_index]); - if (!is_valid_tlvinfo_entry(eeprom_tlv)) - return false; - if (eeprom_tlv->type == tcode) - return true; - *eeprom_index += TLV_INFO_ENTRY_SIZE + eeprom_tlv->length; - } - return(false); -} - -/** - * tlvinfo_delete_tlv - * - * This function deletes the TLV with the specified type code from the - * EEPROM. - */ -bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) -{ - int eeprom_index; - int tlength; - struct tlvinfo_header *eeprom_hdr = to_header(eeprom); - struct tlvinfo_tlv *eeprom_tlv; - - // Find the TLV and then move all following TLVs "forward" - if (tlvinfo_find_tlv(eeprom, code, &eeprom_index)) { - eeprom_tlv = to_entry(&eeprom[eeprom_index]); - tlength = TLV_INFO_ENTRY_SIZE + eeprom_tlv->length; - memcpy(&eeprom[eeprom_index], &eeprom[eeprom_index + tlength], - TLV_INFO_HEADER_SIZE + - be16_to_cpu(eeprom_hdr->totallen) - eeprom_index - - tlength); - eeprom_hdr->totallen = - cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - - tlength); - tlvinfo_update_crc(eeprom); - return true; - } - return false; -} - -/** - * tlvinfo_add_tlv - * - * This function adds a TLV to the EEPROM, converting the value (a string) to - * the format in which it will be stored in the EEPROM. - */ -#define MAX_TLV_VALUE_LEN 256 -bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) -{ - struct tlvinfo_header *eeprom_hdr = to_header(eeprom); - struct tlvinfo_tlv *eeprom_tlv; - int new_tlv_len = 0; - u32 value; - char data[MAX_TLV_VALUE_LEN]; - int eeprom_index; - - // Encode each TLV type into the format to be stored in the EERPOM - switch (tcode) { - case TLV_CODE_PRODUCT_NAME: - case TLV_CODE_PART_NUMBER: - case TLV_CODE_SERIAL_NUMBER: - case TLV_CODE_LABEL_REVISION: - case TLV_CODE_PLATFORM_NAME: - case TLV_CODE_ONIE_VERSION: - case TLV_CODE_MANUF_NAME: - case TLV_CODE_MANUF_COUNTRY: - case TLV_CODE_VENDOR_NAME: - case TLV_CODE_DIAG_VERSION: - case TLV_CODE_SERVICE_TAG: - strncpy(data, strval, MAX_TLV_VALUE_LEN); - new_tlv_len = min_t(size_t, MAX_TLV_VALUE_LEN, strlen(strval)); - break; - case TLV_CODE_DEVICE_VERSION: - value = simple_strtoul(strval, NULL, 0); - if (value >= 256) { - printf("ERROR: Device version must be 255 or less. Value supplied: %u", - value); - return false; - } - data[0] = value & 0xFF; - new_tlv_len = 1; - break; - case TLV_CODE_MAC_SIZE: - value = simple_strtoul(strval, NULL, 0); - if (value >= 65536) { - printf("ERROR: MAC Size must be 65535 or less. Value supplied: %u", - value); - return false; - } - data[0] = (value >> 8) & 0xFF; - data[1] = value & 0xFF; - new_tlv_len = 2; - break; - case TLV_CODE_MANUF_DATE: - if (set_date(data, strval) != 0) - return false; - new_tlv_len = 19; - break; - case TLV_CODE_MAC_BASE: - if (set_mac(data, strval) != 0) - return false; - new_tlv_len = 6; - break; - case TLV_CODE_CRC_32: - printf("WARNING: The CRC TLV is set automatically and cannot be set manually.\n"); - return false; - case TLV_CODE_VENDOR_EXT: - default: - if (set_bytes(data, strval, &new_tlv_len) != 0) - return false; - break; - } - - // Is there room for this TLV? - if ((be16_to_cpu(eeprom_hdr->totallen) + TLV_INFO_ENTRY_SIZE + new_tlv_len) > - TLV_TOTAL_LEN_MAX) { - printf("ERROR: There is not enough room in the EERPOM to save data.\n"); - return false; - } - - // Add TLV at the end, overwriting CRC TLV if it exists - if (tlvinfo_find_tlv(eeprom, TLV_CODE_CRC_32, &eeprom_index)) - eeprom_hdr->totallen = - cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - - TLV_INFO_ENTRY_SIZE - 4); - else - eeprom_index = TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen); - eeprom_tlv = to_entry(&eeprom[eeprom_index]); - eeprom_tlv->type = tcode; - eeprom_tlv->length = new_tlv_len; - memcpy(eeprom_tlv->value, data, new_tlv_len); - - // Update the total length and calculate (add) a new CRC-32 TLV - eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + - TLV_INFO_ENTRY_SIZE + new_tlv_len); - tlvinfo_update_crc(eeprom); - - return true; -} - -/** - * set_mac - * - * Converts a string MAC address into a binary buffer. - * - * This function takes a pointer to a MAC address string - * (i.e."XX:XX:XX:XX:XX:XX", where "XX" is a two-digit hex number). - * The string format is verified and then converted to binary and - * stored in a buffer. - */ -static int set_mac(char *buf, const char *string) -{ - char *p = (char *)string; - int i; - int err = 0; - char *end; - - if (!p) { - printf("ERROR: NULL mac addr string passed in.\n"); - return -1; - } - - if (strlen(p) != 17) { - printf("ERROR: MAC address strlen() != 17 -- %zu\n", strlen(p)); - printf("ERROR: Bad MAC address format: %s\n", string); - return -1; - } - - for (i = 0; i < 17; i++) { - if ((i % 3) == 2) { - if (p[i] != ':') { - err++; - printf("ERROR: mac: p[%i] != :, found: `%c'\n", - i, p[i]); - break; - } - continue; - } else if (!is_hex(p[i])) { - err++; - printf("ERROR: mac: p[%i] != hex digit, found: `%c'\n", - i, p[i]); - break; - } - } - - if (err != 0) { - printf("ERROR: Bad MAC address format: %s\n", string); - return -1; - } - - /* Convert string to binary */ - for (i = 0, p = (char *)string; i < 6; i++) { - buf[i] = p ? hextoul(p, &end) : 0; - if (p) - p = (*end) ? end + 1 : end; - } - - if (!is_valid_ethaddr((u8 *)buf)) { - printf("ERROR: MAC address must not be 00:00:00:00:00:00, a multicast address or FF:FF:FF:FF:FF:FF.\n"); - printf("ERROR: Bad MAC address format: %s\n", string); - return -1; - } - - return 0; -} - -/** - * set_date - * - * Validates the format of the data string - * - * This function takes a pointer to a date string (i.e. MM/DD/YYYY hh:mm:ss) - * and validates that the format is correct. If so the string is copied - * to the supplied buffer. - */ -static int set_date(char *buf, const char *string) -{ - int i; - - if (!string) { - printf("ERROR: NULL date string passed in.\n"); - return -1; - } - - if (strlen(string) != 19) { - printf("ERROR: Date strlen() != 19 -- %zu\n", strlen(string)); - printf("ERROR: Bad date format (MM/DD/YYYY hh:mm:ss): %s\n", - string); - return -1; - } - - for (i = 0; string[i] != 0; i++) { - switch (i) { - case 2: - case 5: - if (string[i] != '/') { - printf("ERROR: Bad date format (MM/DD/YYYY hh:mm:ss): %s\n", - string); - return -1; - } - break; - case 10: - if (string[i] != ' ') { - printf("ERROR: Bad date format (MM/DD/YYYY hh:mm:ss): %s\n", - string); - return -1; - } - break; - case 13: - case 16: - if (string[i] != ':') { - printf("ERROR: Bad date format (MM/DD/YYYY hh:mm:ss): %s\n", - string); - return -1; - } - break; - default: - if (!is_digit(string[i])) { - printf("ERROR: Bad date format (MM/DD/YYYY hh:mm:ss): %s\n", - string); - return -1; - } - break; - } - } - - strcpy(buf, string); - return 0; -} - -/** - * set_bytes - * - * Converts a space-separated string of decimal numbers into a - * buffer of bytes. - * - * This function takes a pointer to a space-separated string of decimal - * numbers (i.e. "128 0x55 0321") with "C" standard radix specifiers - * and converts them to an array of bytes. - */ -static int set_bytes(char *buf, const char *string, int *converted_accum) -{ - char *p = (char *)string; - int i; - uint byte; - - if (!p) { - printf("ERROR: NULL string passed in.\n"); - return -1; - } - - /* Convert string to bytes */ - for (i = 0, p = (char *)string; (i < TLV_VALUE_MAX_LEN) && (*p != 0); - i++) { - while ((*p == ' ') || (*p == '\t') || (*p == ',') || - (*p == ';')) { - p++; - } - if (*p != 0) { - if (!is_digit(*p)) { - printf("ERROR: Non-digit found in byte string: (%s)\n", - string); - return -1; - } - byte = simple_strtoul(p, &p, 0); - if (byte >= 256) { - printf("ERROR: The value specified is greater than 255: (%u) in string: %s\n", - byte, string); - return -1; - } - buf[i] = byte & 0xFF; - } - } - - if (i == TLV_VALUE_MAX_LEN && (*p != 0)) { - printf("ERROR: Trying to assign too many bytes (max: %d) in string: %s\n", - TLV_VALUE_MAX_LEN, string); - return -1; - } - - *converted_accum = i; - return 0; -} - static void show_tlv_devices(int current_dev) { unsigned int dev; @@ -877,218 +370,3 @@ static void show_tlv_devices(int current_dev) printf("TLV: %u%s\n", dev, (dev == current_dev) ? " (*)" : ""); } - -static int find_tlv_devices(struct udevice **tlv_devices_p) -{ - int ret; - int count_dev = 0; - struct udevice *dev; - - for (ret = uclass_first_device_check(UCLASS_I2C_EEPROM, &dev); - dev; - ret = uclass_next_device_check(&dev)) { - if (ret == 0) - tlv_devices_p[count_dev++] = dev; - if (count_dev >= TLV_MAX_DEVICES) - break; - } - - return (count_dev == 0) ? -ENODEV : 0; -} - -static struct udevice *find_tlv_device_by_index(int dev_num) -{ - struct udevice *local_tlv_devices[TLV_MAX_DEVICES] = {}; - struct udevice **tlv_devices_p; - int ret; - - if (gd->flags & (GD_FLG_RELOC | GD_FLG_SPL_INIT)) { - /* Assume BSS is initialized; use static data */ - if (tlv_devices[dev_num]) - return tlv_devices[dev_num]; - tlv_devices_p = tlv_devices; - } else { - tlv_devices_p = local_tlv_devices; - } - - ret = find_tlv_devices(tlv_devices_p); - if (ret == 0 && tlv_devices_p[dev_num]) - return tlv_devices_p[dev_num]; - - return NULL; -} - -/** - * read_tlv_eeprom - read the hwinfo from i2c EEPROM - */ -int read_tlv_eeprom(void *eeprom, int offset, int len, int dev_num) -{ - struct udevice *dev; - - if (dev_num >= TLV_MAX_DEVICES) - return -EINVAL; - - dev = find_tlv_device_by_index(dev_num); - if (!dev) - return -ENODEV; - - return i2c_eeprom_read(dev, offset, eeprom, len); -} - -/** - * write_tlv_eeprom - write the hwinfo to i2c EEPROM - */ -int write_tlv_eeprom(void *eeprom, int len, int dev) -{ - if (!(gd->flags & GD_FLG_RELOC)) - return -ENODEV; - if (!tlv_devices[dev]) - return -ENODEV; - - return i2c_eeprom_write(tlv_devices[dev], 0, eeprom, len); -} - -int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, - struct tlvinfo_tlv **first_entry, int dev_num) -{ - int ret; - struct tlvinfo_header *tlv_hdr; - struct tlvinfo_tlv *tlv_ent; - - /* Read TLV header */ - ret = read_tlv_eeprom(eeprom, 0, TLV_INFO_HEADER_SIZE, dev_num); - if (ret < 0) - return ret; - - tlv_hdr = eeprom; - if (!is_valid_tlvinfo_header(tlv_hdr)) - return -EINVAL; - - /* Read TLV entries */ - tlv_ent = to_entry(&tlv_hdr[1]); - ret = read_tlv_eeprom(tlv_ent, TLV_INFO_HEADER_SIZE, - be16_to_cpu(tlv_hdr->totallen), dev_num); - if (ret < 0) - return ret; - if (!tlvinfo_check_crc(eeprom)) - return -EINVAL; - - *hdr = tlv_hdr; - *first_entry = tlv_ent; - - return 0; -} - -/** - * mac_read_from_eeprom - * - * Read the MAC addresses from EEPROM - * - * This function reads the MAC addresses from EEPROM and sets the - * appropriate environment variables for each one read. - * - * The environment variables are only set if they haven't been set already. - * This ensures that any user-saved variables are never overwritten. - * - * This function must be called after relocation. - */ -int mac_read_from_eeprom(void) -{ - unsigned int i; - int eeprom_index; - struct tlvinfo_tlv *eeprom_tlv; - 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(devnum, eeprom)) { - printf("Read failed.\n"); - return -1; - } - - maccount = 1; - if (tlvinfo_find_tlv(eeprom, TLV_CODE_MAC_SIZE, &eeprom_index)) { - eeprom_tlv = to_entry(&eeprom[eeprom_index]); - maccount = (eeprom_tlv->value[0] << 8) | eeprom_tlv->value[1]; - } - - memcpy(macbase, "\0\0\0\0\0\0", 6); - if (tlvinfo_find_tlv(eeprom, TLV_CODE_MAC_BASE, &eeprom_index)) { - eeprom_tlv = to_entry(&eeprom[eeprom_index]); - memcpy(macbase, eeprom_tlv->value, 6); - } - - for (i = 0; i < maccount; i++) { - if (is_valid_ethaddr(macbase)) { - char ethaddr[18]; - char enetvar[11]; - - sprintf(ethaddr, "%02X:%02X:%02X:%02X:%02X:%02X", - macbase[0], macbase[1], macbase[2], - macbase[3], macbase[4], macbase[5]); - sprintf(enetvar, i ? "eth%daddr" : "ethaddr", i); - /* Only initialize environment variables that are blank - * (i.e. have not yet been set) - */ - if (!env_get(enetvar)) - env_set(enetvar, ethaddr); - - macbase[5]++; - if (macbase[5] == 0) { - macbase[4]++; - if (macbase[4] == 0) { - macbase[3]++; - if (macbase[3] == 0) { - macbase[0] = 0; - macbase[1] = 0; - macbase[2] = 0; - } - } - } - } - } - - printf("%s v%u len=%u\n", eeprom_hdr->signature, eeprom_hdr->version, - be16_to_cpu(eeprom_hdr->totallen)); - - return 0; -} - -/** - * populate_serial_number - read the serial number from EEPROM - * - * This function reads the serial number from the EEPROM and sets the - * appropriate environment variable. - * - * The environment variable is only set if it has not been set - * already. This ensures that any user-saved variables are never - * overwritten. - * - * This function must be called after relocation. - */ -int populate_serial_number(int devnum) -{ - char serialstr[257]; - int eeprom_index; - struct tlvinfo_tlv *eeprom_tlv; - - if (env_get("serial#")) - return 0; - - if (read_eeprom(devnum, eeprom)) { - printf("Read failed.\n"); - return -1; - } - - if (tlvinfo_find_tlv(eeprom, TLV_CODE_SERIAL_NUMBER, &eeprom_index)) { - eeprom_tlv = to_entry(&eeprom[eeprom_index]); - memcpy(serialstr, eeprom_tlv->value, eeprom_tlv->length); - serialstr[eeprom_tlv->length] = 0; - env_set("serial#", serialstr); - } - - return 0; -} diff --git a/lib/Kconfig b/lib/Kconfig index 858be14f09..4497efbbf4 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -909,3 +909,5 @@ config PHANDLE_CHECK_SEQ phandles in fdtdec_get_alias_seq() function.
endmenu + +source lib/tlv/Kconfig diff --git a/lib/Makefile b/lib/Makefile index d9b1811f75..1bc6dad8ac 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -91,6 +91,8 @@ obj-$(CONFIG_LIBAVB) += libavb/ obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT) += libfdt/ obj-$(CONFIG_$(SPL_TPL_)OF_REAL) += fdtdec_common.o fdtdec.o
+obj-$(CONFIG_$(SPL_)EEPROM_TLV_LIB) += tlv/ + ifdef CONFIG_SPL_BUILD obj-$(CONFIG_SPL_YMODEM_SUPPORT) += crc16-ccitt.o obj-$(CONFIG_$(SPL_TPL_)HASH) += crc16-ccitt.o diff --git a/lib/tlv/Kconfig b/lib/tlv/Kconfig new file mode 100644 index 0000000000..b3912ada78 --- /dev/null +++ b/lib/tlv/Kconfig @@ -0,0 +1,15 @@ +config EEPROM_TLV_LIB + bool "Enable EEPROM TLV library" + depends on I2C_EEPROM + help + Selecting this option will enable the shared EEPROM TLV library code. + It provides functions for reading, writing and parsing of + TLV-encoded data from EEPROMs. + +config SPL_EEPROM_TLV_LIB + bool "Enable EEPROM TLV library for SPL" + depends on SPL_I2C_EEPROM + help + Selecting this option will enable the shared EEPROM TLV library code. + It provides functions for reading, writing and parsing of + TLV-encoded data from EEPROMs. diff --git a/lib/tlv/Makefile b/lib/tlv/Makefile new file mode 100644 index 0000000000..8e96752e75 --- /dev/null +++ b/lib/tlv/Makefile @@ -0,0 +1,5 @@ +# SPDX-License-Identifier: GPL-2.0+ +# +# (C) Copyright 2017 Linaro + +obj-$(CONFIG_EEPROM_TLV_LIB) += tlv_eeprom.o diff --git a/lib/tlv/tlv_eeprom.c b/lib/tlv/tlv_eeprom.c new file mode 100644 index 0000000000..464f0aa1fa --- /dev/null +++ b/lib/tlv/tlv_eeprom.c @@ -0,0 +1,750 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * See file CREDITS for list of people who contributed to this + * project. + * + * Copyright (C) 2013 Curt Brune curt@cumulusnetworks.com + * Copyright (C) 2014 Srideep srideep_devireddy@dell.com + * Copyright (C) 2013 Miles Tseng miles_tseng@accton.com + * Copyright (C) 2014,2016 david_yang david_yang@accton.com + * Copyright (C) 2022 Josua Mayer josua@solid-run.com + */ + +#include <common.h> +#include <command.h> +#include <dm.h> +#include <i2c.h> +#include <i2c_eeprom.h> +#include <env.h> +#include <init.h> +#include <net.h> +#include <asm/global_data.h> +#include <linux/ctype.h> +#include <u-boot/crc.h> + +#include "tlv_eeprom.h" + +DECLARE_GLOBAL_DATA_PTR; + +/* File scope function prototypes */ +static int read_eeprom(int devnum, u8 *eeprom); +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); + +/* The EERPOM contents after being read into memory */ +static u8 eeprom[TLV_INFO_MAX_LEN]; + +static struct udevice *tlv_devices[TLV_MAX_DEVICES]; + +#define to_header(p) ((struct tlvinfo_header *)p) +#define to_entry(p) ((struct tlvinfo_tlv *)p) + +/** + * Check whether eeprom device exists. + */ +bool exists_tlv_eeprom(int dev) +{ + return dev < TLV_MAX_DEVICES && tlv_devices[dev] != 0; +} + +static inline bool is_digit(char c) +{ + return (c >= '0' && c <= '9'); +} + +/** + * is_hex + * + * Tests if character is an ASCII hex digit + */ +static inline u8 is_hex(char p) +{ + return (((p >= '0') && (p <= '9')) || + ((p >= 'A') && (p <= 'F')) || + ((p >= 'a') && (p <= 'f'))); +} + +/** + * Validate the checksum in the provided TlvInfo EEPROM data. First, + * verify that the TlvInfo header is valid, then make sure the last + * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data + * and compare it to the value stored in the EEPROM CRC-32 TLV. + */ +bool tlvinfo_check_crc(u8 *eeprom) +{ + struct tlvinfo_header *eeprom_hdr = to_header(eeprom); + struct tlvinfo_tlv *eeprom_crc; + unsigned int calc_crc; + unsigned int stored_crc; + + // Is the eeprom header valid? + if (!is_valid_tlvinfo_header(eeprom_hdr)) + return false; + + // Is the last TLV a CRC? + eeprom_crc = to_entry(&eeprom[TLV_INFO_HEADER_SIZE + + be16_to_cpu(eeprom_hdr->totallen) - (TLV_INFO_ENTRY_SIZE + 4)]); + if (eeprom_crc->type != TLV_CODE_CRC_32 || eeprom_crc->length != 4) + return false; + + // Calculate the checksum + calc_crc = crc32(0, (void *)eeprom, + TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen) - 4); + stored_crc = (eeprom_crc->value[0] << 24) | + (eeprom_crc->value[1] << 16) | + (eeprom_crc->value[2] << 8) | + eeprom_crc->value[3]; + return calc_crc == stored_crc; +} + +/** + * read_eeprom + * + * Read the EEPROM into memory, if it hasn't already been read. + */ +static int read_eeprom(int devnum, u8 *eeprom) +{ + int ret; + struct tlvinfo_header *eeprom_hdr = to_header(eeprom); + struct tlvinfo_tlv *eeprom_tlv = to_entry(&eeprom[TLV_INFO_HEADER_SIZE]); + + /* Read the header */ + ret = read_tlv_eeprom((void *)eeprom_hdr, 0, TLV_INFO_HEADER_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, TLV_INFO_HEADER_SIZE, + be16_to_cpu(eeprom_hdr->totallen), devnum); + + // If the contents are invalid, start over with default contents + if (!is_valid_tlvinfo_header(eeprom_hdr) || + !tlvinfo_check_crc(eeprom)) { + strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); + eeprom_hdr->version = TLV_INFO_VERSION; + eeprom_hdr->totallen = cpu_to_be16(0); + tlvinfo_update_crc(eeprom); + } + +#ifdef DEBUG + show_eeprom(devnum, eeprom); +#endif + + return ret; +} + +/** + * tlvinfo_update_crc + * + * This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then + * one is added. This function should be called after each update to the + * EEPROM structure, to make sure the CRC is always correct. + */ +void tlvinfo_update_crc(u8 *eeprom) +{ + struct tlvinfo_header *eeprom_hdr = to_header(eeprom); + struct tlvinfo_tlv *eeprom_crc; + unsigned int calc_crc; + int eeprom_index; + + // Discover the CRC TLV + if (!tlvinfo_find_tlv(eeprom, TLV_CODE_CRC_32, &eeprom_index)) { + unsigned int totallen = be16_to_cpu(eeprom_hdr->totallen); + + if ((totallen + TLV_INFO_ENTRY_SIZE + 4) > TLV_TOTAL_LEN_MAX) + return; + eeprom_index = TLV_INFO_HEADER_SIZE + totallen; + eeprom_hdr->totallen = cpu_to_be16(totallen + TLV_INFO_ENTRY_SIZE + 4); + } + eeprom_crc = to_entry(&eeprom[eeprom_index]); + eeprom_crc->type = TLV_CODE_CRC_32; + eeprom_crc->length = 4; + + // Calculate the checksum + calc_crc = crc32(0, (void *)eeprom, + TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen) - 4); + eeprom_crc->value[0] = (calc_crc >> 24) & 0xFF; + eeprom_crc->value[1] = (calc_crc >> 16) & 0xFF; + eeprom_crc->value[2] = (calc_crc >> 8) & 0xFF; + eeprom_crc->value[3] = (calc_crc >> 0) & 0xFF; +} + +/** + * write_tlvinfo_tlv_eeprom + * + * Write the TLV data from CPU memory to the hardware. + */ +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev) +{ + int ret = 0; + struct tlvinfo_header *eeprom_hdr = to_header(eeprom); + int eeprom_len; + + tlvinfo_update_crc(eeprom); + + eeprom_len = TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen); + ret = write_tlv_eeprom(eeprom, eeprom_len, dev); + if (ret) { + printf("Programming failed.\n"); + return -1; + } + + printf("Programming passed.\n"); + return 0; +} + +/** + * tlvinfo_find_tlv + * + * This function finds the TLV with the supplied code in the EERPOM. + * An offset from the beginning of the EEPROM is returned in the + * eeprom_index parameter if the TLV is found. + */ +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) +{ + struct tlvinfo_header *eeprom_hdr = to_header(eeprom); + struct tlvinfo_tlv *eeprom_tlv; + int eeprom_end; + + // Search through the TLVs, looking for the first one which matches the + // supplied type code. + *eeprom_index = TLV_INFO_HEADER_SIZE; + eeprom_end = TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen); + while (*eeprom_index < eeprom_end) { + eeprom_tlv = to_entry(&eeprom[*eeprom_index]); + if (!is_valid_tlvinfo_entry(eeprom_tlv)) + return false; + if (eeprom_tlv->type == tcode) + return true; + *eeprom_index += TLV_INFO_ENTRY_SIZE + eeprom_tlv->length; + } + return(false); +} + +/** + * tlvinfo_delete_tlv + * + * This function deletes the TLV with the specified type code from the + * EEPROM. + */ +bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) +{ + int eeprom_index; + int tlength; + struct tlvinfo_header *eeprom_hdr = to_header(eeprom); + struct tlvinfo_tlv *eeprom_tlv; + + // Find the TLV and then move all following TLVs "forward" + if (tlvinfo_find_tlv(eeprom, code, &eeprom_index)) { + eeprom_tlv = to_entry(&eeprom[eeprom_index]); + tlength = TLV_INFO_ENTRY_SIZE + eeprom_tlv->length; + memcpy(&eeprom[eeprom_index], &eeprom[eeprom_index + tlength], + TLV_INFO_HEADER_SIZE + + be16_to_cpu(eeprom_hdr->totallen) - eeprom_index - + tlength); + eeprom_hdr->totallen = + cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - + tlength); + tlvinfo_update_crc(eeprom); + return true; + } + return false; +} + +/** + * tlvinfo_add_tlv + * + * This function adds a TLV to the EEPROM, converting the value (a string) to + * the format in which it will be stored in the EEPROM. + */ +#define MAX_TLV_VALUE_LEN 256 +bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) +{ + struct tlvinfo_header *eeprom_hdr = to_header(eeprom); + struct tlvinfo_tlv *eeprom_tlv; + int new_tlv_len = 0; + u32 value; + char data[MAX_TLV_VALUE_LEN]; + int eeprom_index; + + // Encode each TLV type into the format to be stored in the EERPOM + switch (tcode) { + case TLV_CODE_PRODUCT_NAME: + case TLV_CODE_PART_NUMBER: + case TLV_CODE_SERIAL_NUMBER: + case TLV_CODE_LABEL_REVISION: + case TLV_CODE_PLATFORM_NAME: + case TLV_CODE_ONIE_VERSION: + case TLV_CODE_MANUF_NAME: + case TLV_CODE_MANUF_COUNTRY: + case TLV_CODE_VENDOR_NAME: + case TLV_CODE_DIAG_VERSION: + case TLV_CODE_SERVICE_TAG: + strncpy(data, strval, MAX_TLV_VALUE_LEN); + new_tlv_len = min_t(size_t, MAX_TLV_VALUE_LEN, strlen(strval)); + break; + case TLV_CODE_DEVICE_VERSION: + value = simple_strtoul(strval, NULL, 0); + if (value >= 256) { + printf("ERROR: Device version must be 255 or less. Value supplied: %u", + value); + return false; + } + data[0] = value & 0xFF; + new_tlv_len = 1; + break; + case TLV_CODE_MAC_SIZE: + value = simple_strtoul(strval, NULL, 0); + if (value >= 65536) { + printf("ERROR: MAC Size must be 65535 or less. Value supplied: %u", + value); + return false; + } + data[0] = (value >> 8) & 0xFF; + data[1] = value & 0xFF; + new_tlv_len = 2; + break; + case TLV_CODE_MANUF_DATE: + if (set_date(data, strval) != 0) + return false; + new_tlv_len = 19; + break; + case TLV_CODE_MAC_BASE: + if (set_mac(data, strval) != 0) + return false; + new_tlv_len = 6; + break; + case TLV_CODE_CRC_32: + printf("WARNING: The CRC TLV is set automatically and cannot be set manually.\n"); + return false; + case TLV_CODE_VENDOR_EXT: + default: + if (set_bytes(data, strval, &new_tlv_len) != 0) + return false; + break; + } + + // Is there room for this TLV? + if ((be16_to_cpu(eeprom_hdr->totallen) + TLV_INFO_ENTRY_SIZE + new_tlv_len) > + TLV_TOTAL_LEN_MAX) { + printf("ERROR: There is not enough room in the EERPOM to save data.\n"); + return false; + } + + // Add TLV at the end, overwriting CRC TLV if it exists + if (tlvinfo_find_tlv(eeprom, TLV_CODE_CRC_32, &eeprom_index)) + eeprom_hdr->totallen = + cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - + TLV_INFO_ENTRY_SIZE - 4); + else + eeprom_index = TLV_INFO_HEADER_SIZE + be16_to_cpu(eeprom_hdr->totallen); + eeprom_tlv = to_entry(&eeprom[eeprom_index]); + eeprom_tlv->type = tcode; + eeprom_tlv->length = new_tlv_len; + memcpy(eeprom_tlv->value, data, new_tlv_len); + + // Update the total length and calculate (add) a new CRC-32 TLV + eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + + TLV_INFO_ENTRY_SIZE + new_tlv_len); + tlvinfo_update_crc(eeprom); + + return true; +} + +/** + * set_mac + * + * Converts a string MAC address into a binary buffer. + * + * This function takes a pointer to a MAC address string + * (i.e."XX:XX:XX:XX:XX:XX", where "XX" is a two-digit hex number). + * The string format is verified and then converted to binary and + * stored in a buffer. + */ +static int set_mac(char *buf, const char *string) +{ + char *p = (char *)string; + int i; + int err = 0; + char *end; + + if (!p) { + printf("ERROR: NULL mac addr string passed in.\n"); + return -1; + } + + if (strlen(p) != 17) { + printf("ERROR: MAC address strlen() != 17 -- %zu\n", strlen(p)); + printf("ERROR: Bad MAC address format: %s\n", string); + return -1; + } + + for (i = 0; i < 17; i++) { + if ((i % 3) == 2) { + if (p[i] != ':') { + err++; + printf("ERROR: mac: p[%i] != :, found: `%c'\n", + i, p[i]); + break; + } + continue; + } else if (!is_hex(p[i])) { + err++; + printf("ERROR: mac: p[%i] != hex digit, found: `%c'\n", + i, p[i]); + break; + } + } + + if (err != 0) { + printf("ERROR: Bad MAC address format: %s\n", string); + return -1; + } + + /* Convert string to binary */ + for (i = 0, p = (char *)string; i < 6; i++) { + buf[i] = p ? hextoul(p, &end) : 0; + if (p) + p = (*end) ? end + 1 : end; + } + + if (!is_valid_ethaddr((u8 *)buf)) { + printf("ERROR: MAC address must not be 00:00:00:00:00:00, a multicast address or FF:FF:FF:FF:FF:FF.\n"); + printf("ERROR: Bad MAC address format: %s\n", string); + return -1; + } + + return 0; +} + +/** + * set_date + * + * Validates the format of the data string + * + * This function takes a pointer to a date string (i.e. MM/DD/YYYY hh:mm:ss) + * and validates that the format is correct. If so the string is copied + * to the supplied buffer. + */ +static int set_date(char *buf, const char *string) +{ + int i; + + if (!string) { + printf("ERROR: NULL date string passed in.\n"); + return -1; + } + + if (strlen(string) != 19) { + printf("ERROR: Date strlen() != 19 -- %zu\n", strlen(string)); + printf("ERROR: Bad date format (MM/DD/YYYY hh:mm:ss): %s\n", + string); + return -1; + } + + for (i = 0; string[i] != 0; i++) { + switch (i) { + case 2: + case 5: + if (string[i] != '/') { + printf("ERROR: Bad date format (MM/DD/YYYY hh:mm:ss): %s\n", + string); + return -1; + } + break; + case 10: + if (string[i] != ' ') { + printf("ERROR: Bad date format (MM/DD/YYYY hh:mm:ss): %s\n", + string); + return -1; + } + break; + case 13: + case 16: + if (string[i] != ':') { + printf("ERROR: Bad date format (MM/DD/YYYY hh:mm:ss): %s\n", + string); + return -1; + } + break; + default: + if (!is_digit(string[i])) { + printf("ERROR: Bad date format (MM/DD/YYYY hh:mm:ss): %s\n", + string); + return -1; + } + break; + } + } + + strcpy(buf, string); + return 0; +} + +/** + * set_bytes + * + * Converts a space-separated string of decimal numbers into a + * buffer of bytes. + * + * This function takes a pointer to a space-separated string of decimal + * numbers (i.e. "128 0x55 0321") with "C" standard radix specifiers + * and converts them to an array of bytes. + */ +static int set_bytes(char *buf, const char *string, int *converted_accum) +{ + char *p = (char *)string; + int i; + uint byte; + + if (!p) { + printf("ERROR: NULL string passed in.\n"); + return -1; + } + + /* Convert string to bytes */ + for (i = 0, p = (char *)string; (i < TLV_VALUE_MAX_LEN) && (*p != 0); + i++) { + while ((*p == ' ') || (*p == '\t') || (*p == ',') || + (*p == ';')) { + p++; + } + if (*p != 0) { + if (!is_digit(*p)) { + printf("ERROR: Non-digit found in byte string: (%s)\n", + string); + return -1; + } + byte = simple_strtoul(p, &p, 0); + if (byte >= 256) { + printf("ERROR: The value specified is greater than 255: (%u) in string: %s\n", + byte, string); + return -1; + } + buf[i] = byte & 0xFF; + } + } + + if (i == TLV_VALUE_MAX_LEN && (*p != 0)) { + printf("ERROR: Trying to assign too many bytes (max: %d) in string: %s\n", + TLV_VALUE_MAX_LEN, string); + return -1; + } + + *converted_accum = i; + return 0; +} + +static int find_tlv_devices(struct udevice **tlv_devices_p) +{ + int ret; + int count_dev = 0; + struct udevice *dev; + + for (ret = uclass_first_device_check(UCLASS_I2C_EEPROM, &dev); + dev; + ret = uclass_next_device_check(&dev)) { + if (ret == 0) + tlv_devices_p[count_dev++] = dev; + if (count_dev >= TLV_MAX_DEVICES) + break; + } + + return (count_dev == 0) ? -ENODEV : 0; +} + +static struct udevice *find_tlv_device_by_index(int dev_num) +{ + struct udevice *local_tlv_devices[TLV_MAX_DEVICES] = {}; + struct udevice **tlv_devices_p; + int ret; + + if (gd->flags & (GD_FLG_RELOC | GD_FLG_SPL_INIT)) { + /* Assume BSS is initialized; use static data */ + if (tlv_devices[dev_num]) + return tlv_devices[dev_num]; + tlv_devices_p = tlv_devices; + } else { + tlv_devices_p = local_tlv_devices; + } + + ret = find_tlv_devices(tlv_devices_p); + if (ret == 0 && tlv_devices_p[dev_num]) + return tlv_devices_p[dev_num]; + + return NULL; +} + +/** + * read_tlv_eeprom - read the hwinfo from i2c EEPROM + */ +int read_tlv_eeprom(void *eeprom, int offset, int len, int dev_num) +{ + struct udevice *dev; + + if (dev_num >= TLV_MAX_DEVICES) + return -EINVAL; + + dev = find_tlv_device_by_index(dev_num); + if (!dev) + return -ENODEV; + + return i2c_eeprom_read(dev, offset, eeprom, len); +} + +/** + * write_tlv_eeprom - write the hwinfo to i2c EEPROM + */ +int write_tlv_eeprom(void *eeprom, int len, int dev) +{ + if (!(gd->flags & GD_FLG_RELOC)) + return -ENODEV; + if (!tlv_devices[dev]) + return -ENODEV; + + return i2c_eeprom_write(tlv_devices[dev], 0, eeprom, len); +} + +int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, + struct tlvinfo_tlv **first_entry, int dev_num) +{ + int ret; + struct tlvinfo_header *tlv_hdr; + struct tlvinfo_tlv *tlv_ent; + + /* Read TLV header */ + ret = read_tlv_eeprom(eeprom, 0, TLV_INFO_HEADER_SIZE, dev_num); + if (ret < 0) + return ret; + + tlv_hdr = eeprom; + if (!is_valid_tlvinfo_header(tlv_hdr)) + return -EINVAL; + + /* Read TLV entries */ + tlv_ent = to_entry(&tlv_hdr[1]); + ret = read_tlv_eeprom(tlv_ent, TLV_INFO_HEADER_SIZE, + be16_to_cpu(tlv_hdr->totallen), dev_num); + if (ret < 0) + return ret; + if (!tlvinfo_check_crc(eeprom)) + return -EINVAL; + + *hdr = tlv_hdr; + *first_entry = tlv_ent; + + return 0; +} + +/** + * mac_read_from_eeprom + * + * Read the MAC addresses from EEPROM + * + * This function reads the MAC addresses from EEPROM and sets the + * appropriate environment variables for each one read. + * + * The environment variables are only set if they haven't been set already. + * This ensures that any user-saved variables are never overwritten. + * + * This function must be called after relocation. + */ +int mac_read_from_eeprom(void) +{ + unsigned int i; + int eeprom_index; + struct tlvinfo_tlv *eeprom_tlv; + 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(devnum, eeprom)) { + printf("Read failed.\n"); + return -1; + } + + maccount = 1; + if (tlvinfo_find_tlv(eeprom, TLV_CODE_MAC_SIZE, &eeprom_index)) { + eeprom_tlv = to_entry(&eeprom[eeprom_index]); + maccount = (eeprom_tlv->value[0] << 8) | eeprom_tlv->value[1]; + } + + memcpy(macbase, "\0\0\0\0\0\0", 6); + if (tlvinfo_find_tlv(eeprom, TLV_CODE_MAC_BASE, &eeprom_index)) { + eeprom_tlv = to_entry(&eeprom[eeprom_index]); + memcpy(macbase, eeprom_tlv->value, 6); + } + + for (i = 0; i < maccount; i++) { + if (is_valid_ethaddr(macbase)) { + char ethaddr[18]; + char enetvar[11]; + + sprintf(ethaddr, "%02X:%02X:%02X:%02X:%02X:%02X", + macbase[0], macbase[1], macbase[2], + macbase[3], macbase[4], macbase[5]); + sprintf(enetvar, i ? "eth%daddr" : "ethaddr", i); + /* Only initialize environment variables that are blank + * (i.e. have not yet been set) + */ + if (!env_get(enetvar)) + env_set(enetvar, ethaddr); + + macbase[5]++; + if (macbase[5] == 0) { + macbase[4]++; + if (macbase[4] == 0) { + macbase[3]++; + if (macbase[3] == 0) { + macbase[0] = 0; + macbase[1] = 0; + macbase[2] = 0; + } + } + } + } + } + + printf("%s v%u len=%u\n", eeprom_hdr->signature, eeprom_hdr->version, + be16_to_cpu(eeprom_hdr->totallen)); + + return 0; +} + +/** + * populate_serial_number - read the serial number from EEPROM + * + * This function reads the serial number from the EEPROM and sets the + * appropriate environment variable. + * + * The environment variable is only set if it has not been set + * already. This ensures that any user-saved variables are never + * overwritten. + * + * This function must be called after relocation. + */ +int populate_serial_number(int devnum) +{ + char serialstr[257]; + int eeprom_index; + struct tlvinfo_tlv *eeprom_tlv; + + if (env_get("serial#")) + return 0; + + if (read_eeprom(devnum, eeprom)) { + printf("Read failed.\n"); + return -1; + } + + if (tlvinfo_find_tlv(eeprom, TLV_CODE_SERIAL_NUMBER, &eeprom_index)) { + eeprom_tlv = to_entry(&eeprom[eeprom_index]); + memcpy(serialstr, eeprom_tlv->value, eeprom_tlv->length); + serialstr[eeprom_tlv->length] = 0; + env_set("serial#", serialstr); + } + + return 0; +}
The board file used CONFIG_SPL_CMD_TLV_EEPROM as a library to facilitate reading tlv data with the memory size from eeprom. Since the tlv library has been split off, only CONFIG_SPL_EEPROM_TLV_LIB is required now.
Signed-off-by: Josua Mayer josua@solid-run.com --- configs/clearfog_defconfig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/configs/clearfog_defconfig b/configs/clearfog_defconfig index 880f16a6e0..87ba6f29a7 100644 --- a/configs/clearfog_defconfig +++ b/configs/clearfog_defconfig @@ -12,6 +12,7 @@ CONFIG_DM_GPIO=y CONFIG_DEFAULT_DEVICE_TREE="armada-388-clearfog" CONFIG_SPL_TEXT_BASE=0x40000030 CONFIG_SPL_SERIAL=y +CONFIG_SPL_DRIVERS_MISC=y CONFIG_SPL=y CONFIG_DEBUG_UART_BASE=0xd0012000 CONFIG_DEBUG_UART_CLOCK=250000000 @@ -26,7 +27,6 @@ CONFIG_SYS_CONSOLE_INFO_QUIET=y CONFIG_DISPLAY_BOARDINFO_LATE=y CONFIG_SPL_I2C=y CONFIG_CMD_TLV_EEPROM=y -CONFIG_SPL_CMD_TLV_EEPROM=y # CONFIG_CMD_FLASH is not set CONFIG_CMD_GPIO=y CONFIG_CMD_I2C=y @@ -71,3 +71,4 @@ CONFIG_SYS_NS16550=y CONFIG_KIRKWOOD_SPI=y CONFIG_USB=y CONFIG_USB_XHCI_HCD=y +CONFIG_SPL_EEPROM_TLV_LIB=y
This solves the potentially common problem of getting a specific tlv entry from an eeprom in board-files, without having to introduce several variables, error handling, memcpy and 0-terminating the string.
Signed-off-by: Josua Mayer josua@solid-run.com --- include/tlv_eeprom.h | 12 ++++++++++++ lib/tlv/tlv_eeprom.c | 25 +++++++++++++++++++++++++ 2 files changed, 37 insertions(+)
diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h index c81c58837d..5989c611f5 100644 --- a/include/tlv_eeprom.h +++ b/include/tlv_eeprom.h @@ -166,6 +166,18 @@ bool tlvinfo_add_tlv(u8 *eeprom, int code, char *strval); */ bool tlvinfo_delete_tlv(u8 *eeprom, u8 code);
+/** + * Read the TLV entry with specified code to a buffer as terminated C string. + * @eeprom: Pointer to buffer holding the TLV EEPROM binary data. + * @code: The TLV Code of the entry to read. + * @buffer: Pointer to buffer where the value will be stored. Must have capacity + * for the string representation of the data including null terminator. + * @length: size of the buffer where the value will be stored. + * + * Return length of string on success, -1 on error. + */ +ssize_t tlvinfo_read_tlv(u8 *eeprom, u8 code, u8 *buffer, size_t length); + /** * tlvinfo_update_crc * diff --git a/lib/tlv/tlv_eeprom.c b/lib/tlv/tlv_eeprom.c index 464f0aa1fa..205960e8f2 100644 --- a/lib/tlv/tlv_eeprom.c +++ b/lib/tlv/tlv_eeprom.c @@ -350,6 +350,31 @@ bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) return true; }
+/** + * Read the TLV entry with specified code to a buffer as terminated C string. + */ +ssize_t tlvinfo_read_tlv(u8 *eeprom, u8 code, u8 *buffer, size_t length) +{ + int index; + struct tlvinfo_tlv *tlv; + + // read sku from part-number field + if (tlvinfo_find_tlv(eeprom, code, &index)) { + tlv = (struct tlvinfo_tlv *)&eeprom[index]; + if (tlv->length > length) { + pr_err("%s: tlv value (%d) larger than buffer (%zu)!\n", + __func__, tlv->length + 1, length); + return -1; + } + memcpy(buffer, tlv->value, tlv->length); + buffer[tlv->length] = 0; + + return tlv->length; + } + + return -1; +} + /** * set_mac *
participants (2)
-
Josua Mayer -
Stefan Roese