[PATCH v2 1/3] cmd: mvebu: Implement the Marvell hw_info command
Pali Rohár
pali at kernel.org
Mon Mar 1 16:17:21 CET 2021
On Sunday 28 February 2021 22:54:27 Luka Kovacic wrote:
> Hello Pali,
>
> On Sat, Feb 27, 2021 at 2:09 AM Pali Rohár <pali at kernel.org> 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.
Well, code which you adding in this patch series is currently specific
for one board = Espressobin Ultra. Therefore it should be part of board
code.
I agree with you that board code is not the best code on the world, but
it is still better to have board specific stuff at one place (in one
file or one directory). Currently you put some logic into defconfig file
(loading of variables from SPI) and some into board code / header files
and some into external command.
> 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).
These parameters can be useful. But cannot you export them to U-Boot env
variable storage from board code?
For me it looks like a giant hack if you need to "program" preboot
command which loads these information from SPI via external u-boot
command to u-boot variable storage.
See how is initialized 'fdtfile' variable for Espressobin based at
runtime based on hw on which is running.
I think in the same way you could be able to initialize these useful
parameters (like board serial number, etc...) into u-boot variable
storage.
> >
> > 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.
mac command looks like a good solution for MAC addresses. If it is
possible I would suggest to use it.
> Kind regards,
> Luka
>
> >
> > > Signed-off-by: Luka Kovacic <luka.kovacic at sartura.hr>
> > > Cc: Luka Perkov <luka.perkov at sartura.hr>
> > > Cc: Robert Marko <robert.marko at sartura.hr>
> > > Reviewed-by: Tom Rini <trini at konsulko.com>
> > > ---
> > > 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 <luka.kovacic at sartura.hr>
> > > + */
> > > +
> > > +#include <command.h>
> > > +#include <common.h>
> > > +#include <env.h>
> > > +#include <env_internal.h>
> > > +#include <spi.h>
> > > +#include <spi_flash.h>
> > > +
> > > +#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 <env_name> - 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,
More information about the U-Boot
mailing list