From mboxrd@z Thu Jan 1 00:00:00 1970 From: Luka Kovacic Date: Sun, 28 Feb 2021 22:54:27 +0100 Subject: [PATCH v2 1/3] cmd: mvebu: Implement the Marvell hw_info command In-Reply-To: <20210227010921.e3g2digi4agjtskw@pali> References: <20210215195934.67034-1-luka.kovacic@sartura.hr> <20210215195934.67034-2-luka.kovacic@sartura.hr> <20210227010921.e3g2digi4agjtskw@pali> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Pali, On Sat, Feb 27, 2021 at 2:09 AM Pali Roh?r wrote: > > On Monday 15 February 2021 20:59:32 Luka Kovacic wrote: > > The hw_info command is implemented to enable parsing Marvell hw_info > > formatted environments. This format is often used on Marvell Armada A37XX > > based devices to store parameters like the board serial number, factory > > MAC addresses and some other information. > > These parameters are usually written to the flash in the factory. > > CCing Marek, as he does not like introduction of another vendor custom > command into mainline U-Boot. > > > Currently the command supports reading/writing parameters and dumping the > > current hw_info parameters. > > EEPROM config pattern and checksum aren't supported. > > > > This functionality has been tested on the GST ESPRESSOBin-Ultra board > > successfully, both reading the stock U-Boot parameters in mainline U-Boot > > and reading the parameters written by this command in the stock U-Boot. > > > > Usage example: > > => hw_info load > > => saveenv > > I'm looking at the code and it loads MAC addresses for espressobin > ultra variant. But for other espressobin variants there is already code > which sets correct eth*addr and fdtfile variables in board_late_init() > function. > > I think it should be relatively easy to implement reading these > variables from SPI to ENV in board_late_init() function. Which would > mean that it completely replaces this custom 'hw_info load' command. The board.c file looked really cluttered to me in the current state and adding more board-specific code wouldn't help it. Also there are some other potentially useful parameters in the hw_info SPI flash area, which might be of use (board serial number and some other values). > > Could you look at it? This would have a benefit that 'env default -a' > would always load correct mac addresses stored in SPI, so it would work > just via default u-boot commands without need to use something vendor > specific. I do agree that a vendor specific solution isn't too elegant. I might consider the mac command, which Marek has suggested. Kind regards, Luka > > > Signed-off-by: Luka Kovacic > > Cc: Luka Perkov > > Cc: Robert Marko > > Reviewed-by: Tom Rini > > --- > > cmd/mvebu/Kconfig | 23 ++++ > > cmd/mvebu/Makefile | 1 + > > cmd/mvebu/hw_info.c | 312 ++++++++++++++++++++++++++++++++++++++++++++ > > lib/hashtable.c | 2 +- > > 4 files changed, 337 insertions(+), 1 deletion(-) > > create mode 100644 cmd/mvebu/hw_info.c > > > > diff --git a/cmd/mvebu/Kconfig b/cmd/mvebu/Kconfig > > index ad10a572a3..a8e958e7c8 100644 > > --- a/cmd/mvebu/Kconfig > > +++ b/cmd/mvebu/Kconfig > > @@ -9,6 +9,29 @@ config CMD_MVEBU_BUBT > > For details about bubt command please see the documentation > > in doc/mvebu/cmd/bubt.txt > > > > +config CMD_MVEBU_HW_INFO > > + bool "hw_info" > > + depends on SPI_FLASH && ENV_IS_IN_SPI_FLASH && ARCH_MVEBU > > + default n > > + help > > + Enable loading of the Marvell hw_info parameters from the > > + SPI flash hw_info area. Parameters (usually the board serial > > + number and MAC addresses) are then imported into the > > + existing U-Boot environment. > > + Implementation of this command is compatible with the > > + original Marvell U-Boot command. Reading and writing is > > + supported. > > + EEPROM config pattern and checksum aren't supported. > > + > > +config CMD_MVEBU_HW_INFO_OFFSET > > + hex "Marvell hw_info SPI flash offset" > > + depends on CMD_MVEBU_HW_INFO > > + default 0x3E0000 > > + help > > + This option defines the SPI flash offset of the Marvell > > + hw_info area. This defaults to 0x3E0000 on most Armada > > + A3720 platforms. > > + > > choice > > prompt "Flash for image" > > default MVEBU_SPI_BOOT > > diff --git a/cmd/mvebu/Makefile b/cmd/mvebu/Makefile > > index 96829c48eb..2b5a8b37be 100644 > > --- a/cmd/mvebu/Makefile > > +++ b/cmd/mvebu/Makefile > > @@ -6,3 +6,4 @@ > > > > > > obj-$(CONFIG_CMD_MVEBU_BUBT) += bubt.o > > +obj-$(CONFIG_CMD_MVEBU_HW_INFO) += hw_info.o > > diff --git a/cmd/mvebu/hw_info.c b/cmd/mvebu/hw_info.c > > new file mode 100644 > > index 0000000000..1ef49d78d4 > > --- /dev/null > > +++ b/cmd/mvebu/hw_info.c > > @@ -0,0 +1,312 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Marvell hw_info command > > + * Helper command for interfacing with the Marvell hw_info parameters > > + * > > + * Copyright (c) 2021 Sartura Ltd. > > + * Copyright (c) 2018 Marvell International Ltd. > > + * > > + * Author: Luka Kovacic > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define HW_INFO_SPI_FLASH_OFFSET CONFIG_CMD_MVEBU_HW_INFO_OFFSET > > + > > +#define HW_INFO_MAX_ENV_SIZE 0x1F0 > > +#define HW_INFO_ENV_OFFSET 0xA > > +#define HW_INFO_ENV_SEP 0x20 > > + > > +#define HW_INFO_MAX_NAME_LEN 32 > > + > > +static char hw_info_allowed_parameters[][HW_INFO_MAX_NAME_LEN] = { > > + "pcb_slm", > > + "pcb_rev", > > + "eco_rev", > > + "pcb_sn", > > + "ethaddr", > > + "eth1addr", > > + "eth2addr", > > + "eth3addr", > > + "eth4addr", > > + "eth5addr", > > + "eth6addr", > > + "eth7addr", > > + "eth8addr", > > + "eth9addr", > > +}; > > + > > +static int hw_info_allowed_param_count = (sizeof(hw_info_allowed_parameters) / > > + sizeof(hw_info_allowed_parameters[0])); > > + > > +static int hw_info_check_parameter(char *name) > > +{ > > + int idx; > > + > > + for (idx = 0; idx < hw_info_allowed_param_count; idx++) { > > + if (strcmp(name, hw_info_allowed_parameters[idx]) == 0) > > + return 0; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int read_spi_flash_offset(char *buf, int offset) > > +{ > > + struct spi_flash *flash; > > + int ret; > > + > > + flash = spi_flash_probe(CONFIG_SF_DEFAULT_BUS, > > + CONFIG_SF_DEFAULT_CS, > > + CONFIG_SF_DEFAULT_SPEED, > > + CONFIG_SF_DEFAULT_MODE); > > + > > + if (!flash) { > > + printf("Error - unable to probe SPI flash.\n"); > > + return -EIO; > > + } > > + > > + ret = spi_flash_read(flash, offset, HW_INFO_MAX_ENV_SIZE, buf); > > + if (ret) { > > + printf("Error - unable to read hw_info environment from SPI flash.\n"); > > + return ret; > > + } > > + > > + return ret; > > +} > > + > > +static int write_spi_flash_offset(char *buf, int offset, ssize_t size) > > +{ > > + ssize_t safe_size, erase_size; > > + struct spi_flash *flash; > > + int ret; > > + > > + flash = spi_flash_probe(CONFIG_SF_DEFAULT_BUS, > > + CONFIG_SF_DEFAULT_CS, > > + CONFIG_SF_DEFAULT_SPEED, > > + CONFIG_SF_DEFAULT_MODE); > > + > > + if (!flash) { > > + printf("Error - unable to probe SPI flash.\n"); > > + return -EIO; > > + } > > + > > + safe_size = size > HW_INFO_MAX_ENV_SIZE ? HW_INFO_MAX_ENV_SIZE : size; > > + erase_size = safe_size + > > + (flash->erase_size - safe_size % flash->erase_size); > > + ret = spi_flash_erase(flash, HW_INFO_SPI_FLASH_OFFSET, erase_size); > > + if (ret) { > > + printf("Error - unable to erase the hw_info area on SPI flash.\n"); > > + return ret; > > + } > > + ret = spi_flash_write(flash, offset, safe_size, buf); > > + if (ret) { > > + printf("Error - unable to write hw_info parameters to SPI flash.\n"); > > + return ret; > > + } > > + > > + return ret; > > +} > > + > > +static int cmd_hw_info_dump(void) > > +{ > > + char buffer[HW_INFO_MAX_ENV_SIZE]; > > + struct hsearch_data htab; > > + char *res = NULL; > > + ssize_t len; > > + int ret = 0; > > + > > + ret = read_spi_flash_offset(buffer, HW_INFO_SPI_FLASH_OFFSET + > > + HW_INFO_ENV_OFFSET); > > + if (ret) > > + goto err; > > + memset(&htab, 0, sizeof(htab)); > > + if (!hcreate_r(HW_INFO_MAX_ENV_SIZE, &htab)) { > > + ret = -ENOMEM; > > + goto err; > > + } > > + if (!himport_r(&htab, buffer, HW_INFO_MAX_ENV_SIZE, > > + HW_INFO_ENV_SEP, H_NOCLEAR, 0, 0, NULL)) { > > + ret = -EFAULT; > > + goto err_htab; > > + } > > + > > + len = hexport_r(&htab, '\n', H_HIDE_DOT, &res, 0, 0, NULL); > > + if (len > 0) { > > + printf("Parameters (hw_info):\n"); > > + puts(res); > > + free(res); > > + ret = 0; > > + goto ret_htab; > > + } > > +ret_htab: > > + hdestroy_r(&htab); > > + return ret; > > +err_htab: > > + hdestroy_r(&htab); > > +err: > > + printf("## Error: cannot store hw_info parameters to SPI flash\n"); > > + return ret; > > +} > > + > > +static int cmd_hw_info_load(bool print_env) > > +{ > > + char buffer[HW_INFO_MAX_ENV_SIZE]; > > + char *res = NULL; > > + ssize_t len; > > + int ret = 0; > > + > > + ret = read_spi_flash_offset(buffer, HW_INFO_SPI_FLASH_OFFSET + > > + HW_INFO_ENV_OFFSET); > > + if (ret) > > + goto err; > > + if (!himport_r(&env_htab, buffer, HW_INFO_MAX_ENV_SIZE, > > + HW_INFO_ENV_SEP, H_NOCLEAR, 0, 0, NULL)) { > > + ret = -EFAULT; > > + goto err; > > + } > > + > > + printf("Successfully imported the Marvell hw_info parameters.\n"); > > + if (!print_env) > > + return 0; > > + > > + len = hexport_r(&env_htab, '\n', H_HIDE_DOT, &res, 0, 0, NULL); > > + if (len > 0) { > > + printf("Updated environment:\n"); > > + puts(res); > > + free(res); > > + return 0; > > + } > > +err: > > + printf("## Error: cannot import hw_info parameters\n"); > > + return ret; > > +} > > + > > +static int cmd_hw_info_store(char *name) > > +{ > > + char buffer[HW_INFO_MAX_ENV_SIZE]; > > + struct env_entry e, *ep, *rv; > > + struct hsearch_data htab; > > + char *res = NULL; > > + ssize_t len; > > + int ret = 0; > > + > > + ret = hw_info_check_parameter(name); > > + if (ret) { > > + printf("Invalid parameter %s, stopping.\n", name); > > + goto err; > > + } > > + > > + ret = read_spi_flash_offset(buffer, HW_INFO_SPI_FLASH_OFFSET + > > + HW_INFO_ENV_OFFSET); > > + if (ret) > > + goto err; > > + memset(&htab, 0, sizeof(htab)); > > + if (!hcreate_r(HW_INFO_MAX_ENV_SIZE, &htab)) { > > + ret = -ENOMEM; > > + goto err; > > + } > > + if (!himport_r(&htab, buffer, HW_INFO_MAX_ENV_SIZE, > > + HW_INFO_ENV_SEP, H_NOCLEAR, 0, 0, NULL)) { > > + ret = -EFAULT; > > + goto err_htab; > > + } > > + > > + e.key = name; > > + e.data = NULL; > > + if (!hsearch_r(e, ENV_FIND, &ep, &env_htab, H_HIDE_DOT)) { > > + ret = -ENOENT; > > + goto err_htab; > > + } > > + if (!ep) { > > + ret = -ENOENT; > > + goto err_htab; > > + } > > + > > + printf("Storing %s=%s to hw_info...\n", ep->key, ep->data); > > + > > + e.key = ep->key; > > + e.data = ep->data; > > + if (!hsearch_r(e, ENV_ENTER, &rv, &htab, H_HIDE_DOT)) { > > + ret = -EINVAL; > > + goto err_htab; > > + } > > + if (!rv) { > > + ret = -EINVAL; > > + goto err_htab; > > + } > > + len = hexport_r(&htab, HW_INFO_ENV_SEP, H_MATCH_KEY | H_MATCH_IDENT, > > + &res, 0, 0, NULL); > > + if (len <= 0) { > > + free(res); > > + goto ret_htab; > > + } > > + ret = write_spi_flash_offset(res, HW_INFO_SPI_FLASH_OFFSET + > > + HW_INFO_ENV_OFFSET, len); > > + free(res); > > + if (ret) > > + goto err_htab; > > + > > + printf("Successfully stored the Marvell hw_info parameters.\n"); > > + return 0; > > +ret_htab: > > + hdestroy_r(&htab); > > + return ret; > > +err_htab: > > + hdestroy_r(&htab); > > +err: > > + printf("## Error: cannot store hw_info parameters to SPI flash\n"); > > + return ret; > > +} > > + > > +static int do_hw_info(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > > +{ > > + const char *cmd = argv[1]; > > + > > + if (argc < 2) > > + return CMD_RET_USAGE; > > + > > + if (!strcmp(cmd, "dump")) { > > + if (cmd_hw_info_dump()) > > + return -EINVAL; > > + } else if (!strcmp(cmd, "load")) { > > + if (cmd_hw_info_load(true)) > > + return -EINVAL; > > + } else if (!strcmp(cmd, "store")) { > > + if (cmd_hw_info_store(argv[2])) > > + return -EINVAL; > > + } else { > > + return CMD_RET_USAGE; > > + } > > + > > + return CMD_RET_SUCCESS; > > +} > > + > > +U_BOOT_CMD( > > + hw_info, 3, 0, do_hw_info, > > + "Marvell hw_info utility", > > + "hw_info\n" > > + "\tdump - Dump all hw_info parameters from SPI flash\n" > > + "\tload - Load all hw_info parameters from hw_info to environment variables\n" > > + "\tstore - Store specific hw_info parameter from envirnoment variable to SPI flash\n" > > + "\nSupported hw_info parameters:\n" > > + "\tpcb_slm PCB SLM number\n" > > + "\tpcb_rev PCB revision number\n" > > + "\teco_rev ECO revision number\n" > > + "\tpcb_sn PCB SN\n" > > + "\tethaddr first MAC address\n" > > + "\teth1addr second MAC address\n" > > + "\teth2addr third MAC address\n" > > + "\teth3addr fourth MAC address\n" > > + "\teth4addr fifth MAC address\n" > > + "\teth5addr sixth MAC address\n" > > + "\teth6addr seventh MAC address\n" > > + "\teth7addr eighth MAC address\n" > > + "\teth8addr ninth MAC address\n" > > + "\teth9addr tenth MAC address\n" > > +); > > diff --git a/lib/hashtable.c b/lib/hashtable.c > > index ff5ff72639..06322e3304 100644 > > --- a/lib/hashtable.c > > +++ b/lib/hashtable.c > > @@ -794,7 +794,7 @@ static int drop_var_from_set(const char *name, int nvars, char * vars[]) > > * multi-line values. > > * > > * In theory, arbitrary separator characters can be used, but only > > - * '\0' and '\n' have really been tested. > > + * '\0', '\n' and 0x20 have been tested. > > */ > > > > int himport_r(struct hsearch_data *htab,