[v3,1/3] arm: mvebu: mvebu_armada-37xx: Implement the mac command (Marvell hw_info)

The mac 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.

Currently the mac 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:
 => mac read
 => saveenv

Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr>
Cc: Luka Perkov <luka.perkov@sartura.hr>
Cc: Robert Marko <robert.marko@sartura.hr>
---
 arch/arm/mach-mvebu/Kconfig                   |   1 +
 board/Marvell/mvebu_armada-37xx/Kconfig       |  29 ++
 board/Marvell/mvebu_armada-37xx/Makefile      |   3 +-
 board/Marvell/mvebu_armada-37xx/mac/Makefile  |   5 +
 board/Marvell/mvebu_armada-37xx/mac/hw_info.c | 391 ++++++++++++++++++
 include/configs/mvebu_armada-37xx.h           |   7 +
 lib/hashtable.c                               |   2 +-
 7 files changed, 436 insertions(+), 2 deletions(-)
 create mode 100644 board/Marvell/mvebu_armada-37xx/Kconfig
 create mode 100644 board/Marvell/mvebu_armada-37xx/mac/Makefile
 create mode 100644 board/Marvell/mvebu_armada-37xx/mac/hw_info.c

On Fri, 13 Aug 2021 01:39:36 +0200
Luka Kovacic <luka.kovacic@sartura.hr> wrote:

> The mac 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.
> 
> Currently the mac 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:
>  => mac read
>  => saveenv  

I think the mac/fuse commands should be killed and nvmem API should be
implemented as is in Linux, with a corresponding nvmem command.

Marek

On Friday 13 August 2021 01:39:36 Luka Kovacic wrote:
> The mac 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.
> 
> Currently the mac 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:
>  => mac read
>  => saveenv

Hello Luka!

You are just using above commands for accessing following variables,
right?

pcb_slm
pcb_rev
eco_rev
pcb_sn
ethaddr
eth1addr
eth2addr
eth3addr
eth4addr
eth5addr
eth6addr
eth7addr
eth8addr
eth9addr

So why is additional command required? Espressobin boards can already
load and use correct eth*addr variables, which is implemented in board
file.

So what about implementing this import for all above variables for ultra
variant in board file, like it is already for all other variants? I have
already suggested it in the past. As I think this approach is just one
giant and complicated hack...
https://lore.kernel.org/u-boot/20210301151721.xt62rdun5dnlevqk@pali/

I guess that it should be enough to import variables from SPI to env in
board_late_init() function.

> Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr>
> Cc: Luka Perkov <luka.perkov@sartura.hr>
> Cc: Robert Marko <robert.marko@sartura.hr>
> ---
>  arch/arm/mach-mvebu/Kconfig                   |   1 +
>  board/Marvell/mvebu_armada-37xx/Kconfig       |  29 ++
>  board/Marvell/mvebu_armada-37xx/Makefile      |   3 +-
>  board/Marvell/mvebu_armada-37xx/mac/Makefile  |   5 +
>  board/Marvell/mvebu_armada-37xx/mac/hw_info.c | 391 ++++++++++++++++++
>  include/configs/mvebu_armada-37xx.h           |   7 +
>  lib/hashtable.c                               |   2 +-
>  7 files changed, 436 insertions(+), 2 deletions(-)
>  create mode 100644 board/Marvell/mvebu_armada-37xx/Kconfig
>  create mode 100644 board/Marvell/mvebu_armada-37xx/mac/Makefile
>  create mode 100644 board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> 
> diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> index 89737a37ad..dff9f05b28 100644
> --- a/arch/arm/mach-mvebu/Kconfig
> +++ b/arch/arm/mach-mvebu/Kconfig
> @@ -312,6 +312,7 @@ config SECURED_MODE_CSK_INDEX
>  	default 0
>  	depends on SECURED_MODE_IMAGE
>  
> +source "board/Marvell/mvebu_armada-37xx/Kconfig"
>  source "board/solidrun/clearfog/Kconfig"
>  source "board/kobol/helios4/Kconfig"
>  
> diff --git a/board/Marvell/mvebu_armada-37xx/Kconfig b/board/Marvell/mvebu_armada-37xx/Kconfig
> new file mode 100644
> index 0000000000..b84dd20023
> --- /dev/null
> +++ b/board/Marvell/mvebu_armada-37xx/Kconfig
> @@ -0,0 +1,29 @@
> +menu "Marvell Armada 37xx configuration"
> +depends on TARGET_MVEBU_ARMADA_37XX
> +
> +config MVEBU_MAC_HW_INFO
> +	bool "Marvell hw_info (mac) support"
> +	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.
> +	  After enabled, these parameters are managed from the common
> +	  U-Boot mac command.
> +
> +config MVEBU_MAC_HW_INFO_OFFSET
> +	hex "Marvell hw_info (mac) SPI flash offset"
> +	depends on MVEBU_MAC_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.
> +
> +endmenu
> diff --git a/board/Marvell/mvebu_armada-37xx/Makefile b/board/Marvell/mvebu_armada-37xx/Makefile
> index 27221557c7..6f6ac6b193 100644
> --- a/board/Marvell/mvebu_armada-37xx/Makefile
> +++ b/board/Marvell/mvebu_armada-37xx/Makefile
> @@ -2,4 +2,5 @@
>  #
>  # Copyright (C) 2016 Stefan Roese <sr@denx.de>
>  
> -obj-y	:= board.o
> +obj-y += board.o
> +obj-y += mac/
> diff --git a/board/Marvell/mvebu_armada-37xx/mac/Makefile b/board/Marvell/mvebu_armada-37xx/mac/Makefile
> new file mode 100644
> index 0000000000..7c30fe6194
> --- /dev/null
> +++ b/board/Marvell/mvebu_armada-37xx/mac/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# Copyright (c) 2021 Sartura Ltd.
> +
> +obj-$(CONFIG_ID_EEPROM) += hw_info.o
> diff --git a/board/Marvell/mvebu_armada-37xx/mac/hw_info.c b/board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> new file mode 100644
> index 0000000000..91e5cd8dcf
> --- /dev/null
> +++ b/board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> @@ -0,0 +1,391 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Marvell hw_info (mac) command implementation
> + * 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@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_MVEBU_MAC_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
> +
> +#define HW_INFO_MERGED_VARIABLE		"read_board_hw_info"
> +
> +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;
> +}
> +
> +/**
> + * read_spi_flash_offset() - Read data from the SPI flash
> + * @buf: Buffer to write in
> + * @offset: Offset from the flash start
> + *
> + * Read SPI flash data into the buffer from offset to HW_INFO_MAX_ENV_SIZE.
> + */
> +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;
> +}
> +
> +/**
> + * write_spi_flash_offset() - Write a buffer to SPI flash
> + * @buf: Buffer to write to SPI flash
> + * @offset: Offset from the flash start
> + * @size: Size of the buffer content
> + *
> + * This function probes the SPI flash and updates the specified flash location
> + * with new data from the buffer.
> + */
> +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;
> +}
> +
> +/**
> + * cmd_hw_info_dump() - Dump the hw_info parameters
> + *
> + * This function prints all Marvell hw_info parameters, which are stored in
> + * the SPI flash.
> + */
> +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;
> +}
> +
> +/**
> + * cmd_hw_info_read() - Import the hw_info parameters into U-Boot env
> + * @print_env: Print U-Boot environment after new parameters are imported
> + *
> + * This function reads the Marvell hw_info parameters from SPI flash and
> + * imports them into the U-Boot env.
> + */
> +static int cmd_hw_info_read(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;
> +}
> +
> +/**
> + * cmd_hw_info_save() - Save a parameter from U-Boot env to hw_info parameters
> + * @name: Name of the U-Boot env parameter to save
> + *
> + * This function finds the specified parameter by name in the U-Boot env
> + * and then updates the Marvell hw_info parameters with the new value.
> + */
> +static int cmd_hw_info_save(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;
> +}
> +
> +/**
> + * mac_read_from_eeprom() - Read the parameters from SPI flash.
> + *
> + * This function reads the content of the Marvell hw_info parameters from the
> + * SPI flash and imports them into the U-Boot environment.
> + * This includes MAC addresses and board serial numbers.
> + *
> + * The import is performed only once.
> + *
> + * This function is a part of the U-Boot mac command and must be executed
> + * after SPI flash initialization.
> + */
> +int mac_read_from_eeprom(void)
> +{
> +	if (env_get_ulong(HW_INFO_MERGED_VARIABLE, 10, 0) == 0) {
> +		if (env_set_ulong(HW_INFO_MERGED_VARIABLE, 1))
> +			return -ENOENT;
> +		return cmd_hw_info_read(false);
> +	}
> +	return 0;
> +}
> +
> +/**
> + * print_platform_help() - Print the platform specific help
> + *
> + * Extend the existing U-Boot mac command description by also printing
> + * platform specific help text.
> + */
> +static void print_platform_help(void)
> +{
> +	printf("\nNote: arguments mac [id|num|errata|date|ports|port_number]\n"
> +	       "are unavailable on Marvell Armada A37xx platforms.\n"
> +	       "Use mac [read|save {parameter}] instead.\n"
> +	       "Available parameters:\n"
> +	       "pcb_slm\tPCB SLM number\n"
> +	       "pcb_rev\tPCB revision number\n"
> +	       "eco_rev\tECO revision number\n"
> +	       "pcb_sn\tPCB SN\n"
> +	       "ethaddr\tfirst MAC address\n"
> +	       "eth[1-9]addr\tsecond-ninth MAC address\n");
> +}
> +
> +/**
> + * do_mac() - Standard U-Boot mac command implementation
> + * @cmdtp: U-Boot command table
> + * @flag: Execution flags
> + * @argc: Count of arguments
> + * @argv: Arguments
> + *
> + * This function implements the standard U-Boot mac command in a mostly
> + * compatible way.
> + * To conform to the general command structure as much as possible, the
> + * command description from cmd/mac.c is followed.
> + * Where not possible, convenient or sensible additional comments for the user
> + * are added.
> + */
> +int do_mac(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> +{
> +	const char *cmd = argv[1];
> +	int ret = 0;
> +
> +	if (argc == 1) {
> +		ret = cmd_hw_info_dump();
> +		if (ret)
> +			return -EINVAL;
> +		return CMD_RET_SUCCESS;
> +	}
> +
> +	if (!strcmp(cmd, "read")) {
> +		if (cmd_hw_info_read(true))
> +			return -EINVAL;
> +	} else if (!strcmp(cmd, "save")) {
> +		if (argc != 3) {
> +			printf("Please pass an additional argument to specify, "
> +			       "which env parameter to save.\n");
> +			return -EINVAL;
> +		}
> +		if (cmd_hw_info_save(argv[2]))
> +			return -EINVAL;
> +	} else {
> +		ret = cmd_usage(cmdtp);
> +		print_platform_help();
> +		return ret;
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}
> diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h
> index c8c34d7d92..8e8bcfa018 100644
> --- a/include/configs/mvebu_armada-37xx.h
> +++ b/include/configs/mvebu_armada-37xx.h
> @@ -62,6 +62,13 @@
>   */
>  #define DEFAULT_ENV_IS_RW		/* required for configuring default fdtfile= */
>  
> +/*
> + * Platform identification (Marvell hw_info parameters)
> + */
> +#ifdef CONFIG_MVEBU_MAC_HW_INFO
> +#define CONFIG_ID_EEPROM /* U-Boot mac command */
> +#endif
> +
>  /*
>   * Ethernet Driver configuration
>   */
> 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,
> -- 
> 2.31.1
>

Hello Marek and Pali,

On Fri, Aug 13, 2021 at 10:23 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Friday 13 August 2021 01:39:36 Luka Kovacic wrote:
> > The mac 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.
> >
> > Currently the mac 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:
> >  => mac read
> >  => saveenv
>
> Hello Luka!
>
> You are just using above commands for accessing following variables,
> right?
>
> pcb_slm
> pcb_rev
> eco_rev
> pcb_sn
> ethaddr
> eth1addr
> eth2addr
> eth3addr
> eth4addr
> eth5addr
> eth6addr
> eth7addr
> eth8addr
> eth9addr
>

Yes, I am accessing these variables.

> So why is additional command required? Espressobin boards can already
> load and use correct eth*addr variables, which is implemented in board
> file.

The current implementation in the board file preserves currently stored MAC
addresses, which are in the U-Boot environment.

My implementation is suitable for the ESPRESSOBin-Ultra, as the MACs
are stored in the hw_info area of the SPI flash and the tool imports them
into the normal U-Boot env.
They can also be modified using the tool - we won't be able to get this
functionality in the board file.

>
> So what about implementing this import for all above variables for ultra
> variant in board file, like it is already for all other variants? I have
> already suggested it in the past. As I think this approach is just one
> giant and complicated hack...
> https://lore.kernel.org/u-boot/20210301151721.xt62rdun5dnlevqk@pali/
>
> I guess that it should be enough to import variables from SPI to env in
> board_late_init() function.

Currently, the MACs are automatically imported from the
common/board_r.c file at boot, I think this is more standard and cleaner.

Nevertheless, I believe this a sufficient solution for now,
as this has been suggested as a solution in the previous patchset
comments too.

>
> > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr>
> > Cc: Luka Perkov <luka.perkov@sartura.hr>
> > Cc: Robert Marko <robert.marko@sartura.hr>
> > ---
> >  arch/arm/mach-mvebu/Kconfig                   |   1 +
> >  board/Marvell/mvebu_armada-37xx/Kconfig       |  29 ++
> >  board/Marvell/mvebu_armada-37xx/Makefile      |   3 +-
> >  board/Marvell/mvebu_armada-37xx/mac/Makefile  |   5 +
> >  board/Marvell/mvebu_armada-37xx/mac/hw_info.c | 391 ++++++++++++++++++
> >  include/configs/mvebu_armada-37xx.h           |   7 +
> >  lib/hashtable.c                               |   2 +-
> >  7 files changed, 436 insertions(+), 2 deletions(-)
> >  create mode 100644 board/Marvell/mvebu_armada-37xx/Kconfig
> >  create mode 100644 board/Marvell/mvebu_armada-37xx/mac/Makefile
> >  create mode 100644 board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> >
> > diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> > index 89737a37ad..dff9f05b28 100644
> > --- a/arch/arm/mach-mvebu/Kconfig
> > +++ b/arch/arm/mach-mvebu/Kconfig
> > @@ -312,6 +312,7 @@ config SECURED_MODE_CSK_INDEX
> >       default 0
> >       depends on SECURED_MODE_IMAGE
> >
> > +source "board/Marvell/mvebu_armada-37xx/Kconfig"
> >  source "board/solidrun/clearfog/Kconfig"
> >  source "board/kobol/helios4/Kconfig"
> >
> > diff --git a/board/Marvell/mvebu_armada-37xx/Kconfig b/board/Marvell/mvebu_armada-37xx/Kconfig
> > new file mode 100644
> > index 0000000000..b84dd20023
> > --- /dev/null
> > +++ b/board/Marvell/mvebu_armada-37xx/Kconfig
> > @@ -0,0 +1,29 @@
> > +menu "Marvell Armada 37xx configuration"
> > +depends on TARGET_MVEBU_ARMADA_37XX
> > +
> > +config MVEBU_MAC_HW_INFO
> > +     bool "Marvell hw_info (mac) support"
> > +     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.
> > +       After enabled, these parameters are managed from the common
> > +       U-Boot mac command.
> > +
> > +config MVEBU_MAC_HW_INFO_OFFSET
> > +     hex "Marvell hw_info (mac) SPI flash offset"
> > +     depends on MVEBU_MAC_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.
> > +
> > +endmenu
> > diff --git a/board/Marvell/mvebu_armada-37xx/Makefile b/board/Marvell/mvebu_armada-37xx/Makefile
> > index 27221557c7..6f6ac6b193 100644
> > --- a/board/Marvell/mvebu_armada-37xx/Makefile
> > +++ b/board/Marvell/mvebu_armada-37xx/Makefile
> > @@ -2,4 +2,5 @@
> >  #
> >  # Copyright (C) 2016 Stefan Roese <sr@denx.de>
> >
> > -obj-y        := board.o
> > +obj-y += board.o
> > +obj-y += mac/
> > diff --git a/board/Marvell/mvebu_armada-37xx/mac/Makefile b/board/Marvell/mvebu_armada-37xx/mac/Makefile
> > new file mode 100644
> > index 0000000000..7c30fe6194
> > --- /dev/null
> > +++ b/board/Marvell/mvebu_armada-37xx/mac/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +#
> > +# Copyright (c) 2021 Sartura Ltd.
> > +
> > +obj-$(CONFIG_ID_EEPROM) += hw_info.o
> > diff --git a/board/Marvell/mvebu_armada-37xx/mac/hw_info.c b/board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> > new file mode 100644
> > index 0000000000..91e5cd8dcf
> > --- /dev/null
> > +++ b/board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> > @@ -0,0 +1,391 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Marvell hw_info (mac) command implementation
> > + * 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@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_MVEBU_MAC_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
> > +
> > +#define HW_INFO_MERGED_VARIABLE              "read_board_hw_info"
> > +
> > +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;
> > +}
> > +
> > +/**
> > + * read_spi_flash_offset() - Read data from the SPI flash
> > + * @buf: Buffer to write in
> > + * @offset: Offset from the flash start
> > + *
> > + * Read SPI flash data into the buffer from offset to HW_INFO_MAX_ENV_SIZE.
> > + */
> > +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;
> > +}
> > +
> > +/**
> > + * write_spi_flash_offset() - Write a buffer to SPI flash
> > + * @buf: Buffer to write to SPI flash
> > + * @offset: Offset from the flash start
> > + * @size: Size of the buffer content
> > + *
> > + * This function probes the SPI flash and updates the specified flash location
> > + * with new data from the buffer.
> > + */
> > +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;
> > +}
> > +
> > +/**
> > + * cmd_hw_info_dump() - Dump the hw_info parameters
> > + *
> > + * This function prints all Marvell hw_info parameters, which are stored in
> > + * the SPI flash.
> > + */
> > +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;
> > +}
> > +
> > +/**
> > + * cmd_hw_info_read() - Import the hw_info parameters into U-Boot env
> > + * @print_env: Print U-Boot environment after new parameters are imported
> > + *
> > + * This function reads the Marvell hw_info parameters from SPI flash and
> > + * imports them into the U-Boot env.
> > + */
> > +static int cmd_hw_info_read(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;
> > +}
> > +
> > +/**
> > + * cmd_hw_info_save() - Save a parameter from U-Boot env to hw_info parameters
> > + * @name: Name of the U-Boot env parameter to save
> > + *
> > + * This function finds the specified parameter by name in the U-Boot env
> > + * and then updates the Marvell hw_info parameters with the new value.
> > + */
> > +static int cmd_hw_info_save(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;
> > +}
> > +
> > +/**
> > + * mac_read_from_eeprom() - Read the parameters from SPI flash.
> > + *
> > + * This function reads the content of the Marvell hw_info parameters from the
> > + * SPI flash and imports them into the U-Boot environment.
> > + * This includes MAC addresses and board serial numbers.
> > + *
> > + * The import is performed only once.
> > + *
> > + * This function is a part of the U-Boot mac command and must be executed
> > + * after SPI flash initialization.
> > + */
> > +int mac_read_from_eeprom(void)
> > +{
> > +     if (env_get_ulong(HW_INFO_MERGED_VARIABLE, 10, 0) == 0) {
> > +             if (env_set_ulong(HW_INFO_MERGED_VARIABLE, 1))
> > +                     return -ENOENT;
> > +             return cmd_hw_info_read(false);
> > +     }
> > +     return 0;
> > +}
> > +
> > +/**
> > + * print_platform_help() - Print the platform specific help
> > + *
> > + * Extend the existing U-Boot mac command description by also printing
> > + * platform specific help text.
> > + */
> > +static void print_platform_help(void)
> > +{
> > +     printf("\nNote: arguments mac [id|num|errata|date|ports|port_number]\n"
> > +            "are unavailable on Marvell Armada A37xx platforms.\n"
> > +            "Use mac [read|save {parameter}] instead.\n"
> > +            "Available parameters:\n"
> > +            "pcb_slm\tPCB SLM number\n"
> > +            "pcb_rev\tPCB revision number\n"
> > +            "eco_rev\tECO revision number\n"
> > +            "pcb_sn\tPCB SN\n"
> > +            "ethaddr\tfirst MAC address\n"
> > +            "eth[1-9]addr\tsecond-ninth MAC address\n");
> > +}
> > +
> > +/**
> > + * do_mac() - Standard U-Boot mac command implementation
> > + * @cmdtp: U-Boot command table
> > + * @flag: Execution flags
> > + * @argc: Count of arguments
> > + * @argv: Arguments
> > + *
> > + * This function implements the standard U-Boot mac command in a mostly
> > + * compatible way.
> > + * To conform to the general command structure as much as possible, the
> > + * command description from cmd/mac.c is followed.
> > + * Where not possible, convenient or sensible additional comments for the user
> > + * are added.
> > + */
> > +int do_mac(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> > +{
> > +     const char *cmd = argv[1];
> > +     int ret = 0;
> > +
> > +     if (argc == 1) {
> > +             ret = cmd_hw_info_dump();
> > +             if (ret)
> > +                     return -EINVAL;
> > +             return CMD_RET_SUCCESS;
> > +     }
> > +
> > +     if (!strcmp(cmd, "read")) {
> > +             if (cmd_hw_info_read(true))
> > +                     return -EINVAL;
> > +     } else if (!strcmp(cmd, "save")) {
> > +             if (argc != 3) {
> > +                     printf("Please pass an additional argument to specify, "
> > +                            "which env parameter to save.\n");
> > +                     return -EINVAL;
> > +             }
> > +             if (cmd_hw_info_save(argv[2]))
> > +                     return -EINVAL;
> > +     } else {
> > +             ret = cmd_usage(cmdtp);
> > +             print_platform_help();
> > +             return ret;
> > +     }
> > +
> > +     return CMD_RET_SUCCESS;
> > +}
> > diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h
> > index c8c34d7d92..8e8bcfa018 100644
> > --- a/include/configs/mvebu_armada-37xx.h
> > +++ b/include/configs/mvebu_armada-37xx.h
> > @@ -62,6 +62,13 @@
> >   */
> >  #define DEFAULT_ENV_IS_RW            /* required for configuring default fdtfile= */
> >
> > +/*
> > + * Platform identification (Marvell hw_info parameters)
> > + */
> > +#ifdef CONFIG_MVEBU_MAC_HW_INFO
> > +#define CONFIG_ID_EEPROM /* U-Boot mac command */
> > +#endif
> > +
> >  /*
> >   * Ethernet Driver configuration
> >   */
> > 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,
> > --
> > 2.31.1
> >

Kind regards,
Luka

On Friday 13 August 2021 11:23:15 Luka Kovacic wrote:
> Hello Marek and Pali,
> 
> On Fri, Aug 13, 2021 at 10:23 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Friday 13 August 2021 01:39:36 Luka Kovacic wrote:
> > > The mac 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.
> > >
> > > Currently the mac 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:
> > >  => mac read
> > >  => saveenv
> >
> > Hello Luka!
> >
> > You are just using above commands for accessing following variables,
> > right?
> >
> > pcb_slm
> > pcb_rev
> > eco_rev
> > pcb_sn
> > ethaddr
> > eth1addr
> > eth2addr
> > eth3addr
> > eth4addr
> > eth5addr
> > eth6addr
> > eth7addr
> > eth8addr
> > eth9addr
> >
> 
> Yes, I am accessing these variables.
> 
> > So why is additional command required? Espressobin boards can already
> > load and use correct eth*addr variables, which is implemented in board
> > file.
> 
> The current implementation in the board file preserves currently stored MAC
> addresses, which are in the U-Boot environment.

Yes, this is for v5/v7 variants. But it can be easily modified for ultra
to read mac addresses from this custom storage.

> My implementation is suitable for the ESPRESSOBin-Ultra, as the MACs
> are stored in the hw_info area of the SPI flash and the tool imports them
> into the normal U-Boot env.
> They can also be modified using the tool - we won't be able to get this
> functionality in the board file.

Modification by env variables is also possible from the board file. Look
at U_BOOT_ENV_CALLBACK() functionality, which allows to call custom code
when trying to change/delete particular env variable.

> > So what about implementing this import for all above variables for ultra
> > variant in board file, like it is already for all other variants? I have
> > already suggested it in the past. As I think this approach is just one
> > giant and complicated hack...
> > https://lore.kernel.org/u-boot/20210301151721.xt62rdun5dnlevqk@pali/
> >
> > I guess that it should be enough to import variables from SPI to env in
> > board_late_init() function.
> 
> Currently, the MACs are automatically imported from the
> common/board_r.c file at boot, I think this is more standard and cleaner.

Hm... then I somehow misunderstood how it works.

Because I do not see any change in common/board_r.c which could read or
import mac addresses via calling this new "mac read" command.

> Nevertheless, I believe this a sufficient solution for now,
> as this has been suggested as a solution in the previous patchset
> comments too.
> 
> >
> > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr>
> > > Cc: Luka Perkov <luka.perkov@sartura.hr>
> > > Cc: Robert Marko <robert.marko@sartura.hr>
> > > ---
> > >  arch/arm/mach-mvebu/Kconfig                   |   1 +
> > >  board/Marvell/mvebu_armada-37xx/Kconfig       |  29 ++
> > >  board/Marvell/mvebu_armada-37xx/Makefile      |   3 +-
> > >  board/Marvell/mvebu_armada-37xx/mac/Makefile  |   5 +
> > >  board/Marvell/mvebu_armada-37xx/mac/hw_info.c | 391 ++++++++++++++++++
> > >  include/configs/mvebu_armada-37xx.h           |   7 +
> > >  lib/hashtable.c                               |   2 +-
> > >  7 files changed, 436 insertions(+), 2 deletions(-)
> > >  create mode 100644 board/Marvell/mvebu_armada-37xx/Kconfig
> > >  create mode 100644 board/Marvell/mvebu_armada-37xx/mac/Makefile
> > >  create mode 100644 board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> > >
> > > diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> > > index 89737a37ad..dff9f05b28 100644
> > > --- a/arch/arm/mach-mvebu/Kconfig
> > > +++ b/arch/arm/mach-mvebu/Kconfig
> > > @@ -312,6 +312,7 @@ config SECURED_MODE_CSK_INDEX
> > >       default 0
> > >       depends on SECURED_MODE_IMAGE
> > >
> > > +source "board/Marvell/mvebu_armada-37xx/Kconfig"
> > >  source "board/solidrun/clearfog/Kconfig"
> > >  source "board/kobol/helios4/Kconfig"
> > >
> > > diff --git a/board/Marvell/mvebu_armada-37xx/Kconfig b/board/Marvell/mvebu_armada-37xx/Kconfig
> > > new file mode 100644
> > > index 0000000000..b84dd20023
> > > --- /dev/null
> > > +++ b/board/Marvell/mvebu_armada-37xx/Kconfig
> > > @@ -0,0 +1,29 @@
> > > +menu "Marvell Armada 37xx configuration"
> > > +depends on TARGET_MVEBU_ARMADA_37XX
> > > +
> > > +config MVEBU_MAC_HW_INFO
> > > +     bool "Marvell hw_info (mac) support"
> > > +     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.
> > > +       After enabled, these parameters are managed from the common
> > > +       U-Boot mac command.
> > > +
> > > +config MVEBU_MAC_HW_INFO_OFFSET
> > > +     hex "Marvell hw_info (mac) SPI flash offset"
> > > +     depends on MVEBU_MAC_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.
> > > +
> > > +endmenu
> > > diff --git a/board/Marvell/mvebu_armada-37xx/Makefile b/board/Marvell/mvebu_armada-37xx/Makefile
> > > index 27221557c7..6f6ac6b193 100644
> > > --- a/board/Marvell/mvebu_armada-37xx/Makefile
> > > +++ b/board/Marvell/mvebu_armada-37xx/Makefile
> > > @@ -2,4 +2,5 @@
> > >  #
> > >  # Copyright (C) 2016 Stefan Roese <sr@denx.de>
> > >
> > > -obj-y        := board.o
> > > +obj-y += board.o
> > > +obj-y += mac/
> > > diff --git a/board/Marvell/mvebu_armada-37xx/mac/Makefile b/board/Marvell/mvebu_armada-37xx/mac/Makefile
> > > new file mode 100644
> > > index 0000000000..7c30fe6194
> > > --- /dev/null
> > > +++ b/board/Marvell/mvebu_armada-37xx/mac/Makefile
> > > @@ -0,0 +1,5 @@
> > > +# SPDX-License-Identifier: GPL-2.0+
> > > +#
> > > +# Copyright (c) 2021 Sartura Ltd.
> > > +
> > > +obj-$(CONFIG_ID_EEPROM) += hw_info.o
> > > diff --git a/board/Marvell/mvebu_armada-37xx/mac/hw_info.c b/board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> > > new file mode 100644
> > > index 0000000000..91e5cd8dcf
> > > --- /dev/null
> > > +++ b/board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> > > @@ -0,0 +1,391 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Marvell hw_info (mac) command implementation
> > > + * 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@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_MVEBU_MAC_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
> > > +
> > > +#define HW_INFO_MERGED_VARIABLE              "read_board_hw_info"
> > > +
> > > +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;
> > > +}
> > > +
> > > +/**
> > > + * read_spi_flash_offset() - Read data from the SPI flash
> > > + * @buf: Buffer to write in
> > > + * @offset: Offset from the flash start
> > > + *
> > > + * Read SPI flash data into the buffer from offset to HW_INFO_MAX_ENV_SIZE.
> > > + */
> > > +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;
> > > +}
> > > +
> > > +/**
> > > + * write_spi_flash_offset() - Write a buffer to SPI flash
> > > + * @buf: Buffer to write to SPI flash
> > > + * @offset: Offset from the flash start
> > > + * @size: Size of the buffer content
> > > + *
> > > + * This function probes the SPI flash and updates the specified flash location
> > > + * with new data from the buffer.
> > > + */
> > > +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;
> > > +}
> > > +
> > > +/**
> > > + * cmd_hw_info_dump() - Dump the hw_info parameters
> > > + *
> > > + * This function prints all Marvell hw_info parameters, which are stored in
> > > + * the SPI flash.
> > > + */
> > > +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;
> > > +}
> > > +
> > > +/**
> > > + * cmd_hw_info_read() - Import the hw_info parameters into U-Boot env
> > > + * @print_env: Print U-Boot environment after new parameters are imported
> > > + *
> > > + * This function reads the Marvell hw_info parameters from SPI flash and
> > > + * imports them into the U-Boot env.
> > > + */
> > > +static int cmd_hw_info_read(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;
> > > +}
> > > +
> > > +/**
> > > + * cmd_hw_info_save() - Save a parameter from U-Boot env to hw_info parameters
> > > + * @name: Name of the U-Boot env parameter to save
> > > + *
> > > + * This function finds the specified parameter by name in the U-Boot env
> > > + * and then updates the Marvell hw_info parameters with the new value.
> > > + */
> > > +static int cmd_hw_info_save(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;
> > > +}
> > > +
> > > +/**
> > > + * mac_read_from_eeprom() - Read the parameters from SPI flash.
> > > + *
> > > + * This function reads the content of the Marvell hw_info parameters from the
> > > + * SPI flash and imports them into the U-Boot environment.
> > > + * This includes MAC addresses and board serial numbers.
> > > + *
> > > + * The import is performed only once.
> > > + *
> > > + * This function is a part of the U-Boot mac command and must be executed
> > > + * after SPI flash initialization.
> > > + */
> > > +int mac_read_from_eeprom(void)
> > > +{
> > > +     if (env_get_ulong(HW_INFO_MERGED_VARIABLE, 10, 0) == 0) {
> > > +             if (env_set_ulong(HW_INFO_MERGED_VARIABLE, 1))
> > > +                     return -ENOENT;
> > > +             return cmd_hw_info_read(false);
> > > +     }
> > > +     return 0;
> > > +}
> > > +
> > > +/**
> > > + * print_platform_help() - Print the platform specific help
> > > + *
> > > + * Extend the existing U-Boot mac command description by also printing
> > > + * platform specific help text.
> > > + */
> > > +static void print_platform_help(void)
> > > +{
> > > +     printf("\nNote: arguments mac [id|num|errata|date|ports|port_number]\n"
> > > +            "are unavailable on Marvell Armada A37xx platforms.\n"
> > > +            "Use mac [read|save {parameter}] instead.\n"
> > > +            "Available parameters:\n"
> > > +            "pcb_slm\tPCB SLM number\n"
> > > +            "pcb_rev\tPCB revision number\n"
> > > +            "eco_rev\tECO revision number\n"
> > > +            "pcb_sn\tPCB SN\n"
> > > +            "ethaddr\tfirst MAC address\n"
> > > +            "eth[1-9]addr\tsecond-ninth MAC address\n");
> > > +}
> > > +
> > > +/**
> > > + * do_mac() - Standard U-Boot mac command implementation
> > > + * @cmdtp: U-Boot command table
> > > + * @flag: Execution flags
> > > + * @argc: Count of arguments
> > > + * @argv: Arguments
> > > + *
> > > + * This function implements the standard U-Boot mac command in a mostly
> > > + * compatible way.
> > > + * To conform to the general command structure as much as possible, the
> > > + * command description from cmd/mac.c is followed.
> > > + * Where not possible, convenient or sensible additional comments for the user
> > > + * are added.
> > > + */
> > > +int do_mac(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> > > +{
> > > +     const char *cmd = argv[1];
> > > +     int ret = 0;
> > > +
> > > +     if (argc == 1) {
> > > +             ret = cmd_hw_info_dump();
> > > +             if (ret)
> > > +                     return -EINVAL;
> > > +             return CMD_RET_SUCCESS;
> > > +     }
> > > +
> > > +     if (!strcmp(cmd, "read")) {
> > > +             if (cmd_hw_info_read(true))
> > > +                     return -EINVAL;
> > > +     } else if (!strcmp(cmd, "save")) {
> > > +             if (argc != 3) {
> > > +                     printf("Please pass an additional argument to specify, "
> > > +                            "which env parameter to save.\n");
> > > +                     return -EINVAL;
> > > +             }
> > > +             if (cmd_hw_info_save(argv[2]))
> > > +                     return -EINVAL;
> > > +     } else {
> > > +             ret = cmd_usage(cmdtp);
> > > +             print_platform_help();
> > > +             return ret;
> > > +     }
> > > +
> > > +     return CMD_RET_SUCCESS;
> > > +}
> > > diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h
> > > index c8c34d7d92..8e8bcfa018 100644
> > > --- a/include/configs/mvebu_armada-37xx.h
> > > +++ b/include/configs/mvebu_armada-37xx.h
> > > @@ -62,6 +62,13 @@
> > >   */
> > >  #define DEFAULT_ENV_IS_RW            /* required for configuring default fdtfile= */
> > >
> > > +/*
> > > + * Platform identification (Marvell hw_info parameters)
> > > + */
> > > +#ifdef CONFIG_MVEBU_MAC_HW_INFO
> > > +#define CONFIG_ID_EEPROM /* U-Boot mac command */
> > > +#endif
> > > +
> > >  /*
> > >   * Ethernet Driver configuration
> > >   */
> > > 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,
> > > --
> > > 2.31.1
> > >
> 
> Kind regards,
> Luka

Hello Pali,

On Fri, Aug 13, 2021 at 11:41 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Friday 13 August 2021 11:23:15 Luka Kovacic wrote:
> > Hello Marek and Pali,
> >
> > On Fri, Aug 13, 2021 at 10:23 AM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Friday 13 August 2021 01:39:36 Luka Kovacic wrote:
> > > > The mac 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.
> > > >
> > > > Currently the mac 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:
> > > >  => mac read
> > > >  => saveenv
> > >
> > > Hello Luka!
> > >
> > > You are just using above commands for accessing following variables,
> > > right?
> > >
> > > pcb_slm
> > > pcb_rev
> > > eco_rev
> > > pcb_sn
> > > ethaddr
> > > eth1addr
> > > eth2addr
> > > eth3addr
> > > eth4addr
> > > eth5addr
> > > eth6addr
> > > eth7addr
> > > eth8addr
> > > eth9addr
> > >
> >
> > Yes, I am accessing these variables.
> >
> > > So why is additional command required? Espressobin boards can already
> > > load and use correct eth*addr variables, which is implemented in board
> > > file.
> >
> > The current implementation in the board file preserves currently stored MAC
> > addresses, which are in the U-Boot environment.
>
> Yes, this is for v5/v7 variants. But it can be easily modified for ultra
> to read mac addresses from this custom storage.
>
> > My implementation is suitable for the ESPRESSOBin-Ultra, as the MACs
> > are stored in the hw_info area of the SPI flash and the tool imports them
> > into the normal U-Boot env.
> > They can also be modified using the tool - we won't be able to get this
> > functionality in the board file.
>
> Modification by env variables is also possible from the board file. Look
> at U_BOOT_ENV_CALLBACK() functionality, which allows to call custom code
> when trying to change/delete particular env variable.
>
> > > So what about implementing this import for all above variables for ultra
> > > variant in board file, like it is already for all other variants? I have
> > > already suggested it in the past. As I think this approach is just one
> > > giant and complicated hack...
> > > https://lore.kernel.org/u-boot/20210301151721.xt62rdun5dnlevqk@pali/
> > >
> > > I guess that it should be enough to import variables from SPI to env in
> > > board_late_init() function.
> >
> > Currently, the MACs are automatically imported from the
> > common/board_r.c file at boot, I think this is more standard and cleaner.
>
> Hm... then I somehow misunderstood how it works.
>
> Because I do not see any change in common/board_r.c which could read or
> import mac addresses via calling this new "mac read" command.

The function mac_read_from_eeprom() is called, if CONFIG_ID_EEPROM is
enabled. This has also been tested and a mechanism was added to avoid
duplicate imports (if the user wants to modify the MAC address, so it then
isn't overwritten).

https://source.denx.de/u-boot/u-boot/-/blob/master/common/board_r.c#L724

>
> > Nevertheless, I believe this a sufficient solution for now,
> > as this has been suggested as a solution in the previous patchset
> > comments too.
> >
> > >
> > > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr>
> > > > Cc: Luka Perkov <luka.perkov@sartura.hr>
> > > > Cc: Robert Marko <robert.marko@sartura.hr>
> > > > ---
> > > >  arch/arm/mach-mvebu/Kconfig                   |   1 +
> > > >  board/Marvell/mvebu_armada-37xx/Kconfig       |  29 ++
> > > >  board/Marvell/mvebu_armada-37xx/Makefile      |   3 +-
> > > >  board/Marvell/mvebu_armada-37xx/mac/Makefile  |   5 +
> > > >  board/Marvell/mvebu_armada-37xx/mac/hw_info.c | 391 ++++++++++++++++++
> > > >  include/configs/mvebu_armada-37xx.h           |   7 +
> > > >  lib/hashtable.c                               |   2 +-
> > > >  7 files changed, 436 insertions(+), 2 deletions(-)
> > > >  create mode 100644 board/Marvell/mvebu_armada-37xx/Kconfig
> > > >  create mode 100644 board/Marvell/mvebu_armada-37xx/mac/Makefile
> > > >  create mode 100644 board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> > > >
> > > > diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> > > > index 89737a37ad..dff9f05b28 100644
> > > > --- a/arch/arm/mach-mvebu/Kconfig
> > > > +++ b/arch/arm/mach-mvebu/Kconfig
> > > > @@ -312,6 +312,7 @@ config SECURED_MODE_CSK_INDEX
> > > >       default 0
> > > >       depends on SECURED_MODE_IMAGE
> > > >
> > > > +source "board/Marvell/mvebu_armada-37xx/Kconfig"
> > > >  source "board/solidrun/clearfog/Kconfig"
> > > >  source "board/kobol/helios4/Kconfig"
> > > >
> > > > diff --git a/board/Marvell/mvebu_armada-37xx/Kconfig b/board/Marvell/mvebu_armada-37xx/Kconfig
> > > > new file mode 100644
> > > > index 0000000000..b84dd20023
> > > > --- /dev/null
> > > > +++ b/board/Marvell/mvebu_armada-37xx/Kconfig
> > > > @@ -0,0 +1,29 @@
> > > > +menu "Marvell Armada 37xx configuration"
> > > > +depends on TARGET_MVEBU_ARMADA_37XX
> > > > +
> > > > +config MVEBU_MAC_HW_INFO
> > > > +     bool "Marvell hw_info (mac) support"
> > > > +     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.
> > > > +       After enabled, these parameters are managed from the common
> > > > +       U-Boot mac command.
> > > > +
> > > > +config MVEBU_MAC_HW_INFO_OFFSET
> > > > +     hex "Marvell hw_info (mac) SPI flash offset"
> > > > +     depends on MVEBU_MAC_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.
> > > > +
> > > > +endmenu
> > > > diff --git a/board/Marvell/mvebu_armada-37xx/Makefile b/board/Marvell/mvebu_armada-37xx/Makefile
> > > > index 27221557c7..6f6ac6b193 100644
> > > > --- a/board/Marvell/mvebu_armada-37xx/Makefile
> > > > +++ b/board/Marvell/mvebu_armada-37xx/Makefile
> > > > @@ -2,4 +2,5 @@
> > > >  #
> > > >  # Copyright (C) 2016 Stefan Roese <sr@denx.de>
> > > >
> > > > -obj-y        := board.o
> > > > +obj-y += board.o
> > > > +obj-y += mac/
> > > > diff --git a/board/Marvell/mvebu_armada-37xx/mac/Makefile b/board/Marvell/mvebu_armada-37xx/mac/Makefile
> > > > new file mode 100644
> > > > index 0000000000..7c30fe6194
> > > > --- /dev/null
> > > > +++ b/board/Marvell/mvebu_armada-37xx/mac/Makefile
> > > > @@ -0,0 +1,5 @@
> > > > +# SPDX-License-Identifier: GPL-2.0+
> > > > +#
> > > > +# Copyright (c) 2021 Sartura Ltd.
> > > > +
> > > > +obj-$(CONFIG_ID_EEPROM) += hw_info.o
> > > > diff --git a/board/Marvell/mvebu_armada-37xx/mac/hw_info.c b/board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> > > > new file mode 100644
> > > > index 0000000000..91e5cd8dcf
> > > > --- /dev/null
> > > > +++ b/board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> > > > @@ -0,0 +1,391 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * Marvell hw_info (mac) command implementation
> > > > + * 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@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_MVEBU_MAC_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
> > > > +
> > > > +#define HW_INFO_MERGED_VARIABLE              "read_board_hw_info"
> > > > +
> > > > +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;
> > > > +}
> > > > +
> > > > +/**
> > > > + * read_spi_flash_offset() - Read data from the SPI flash
> > > > + * @buf: Buffer to write in
> > > > + * @offset: Offset from the flash start
> > > > + *
> > > > + * Read SPI flash data into the buffer from offset to HW_INFO_MAX_ENV_SIZE.
> > > > + */
> > > > +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;
> > > > +}
> > > > +
> > > > +/**
> > > > + * write_spi_flash_offset() - Write a buffer to SPI flash
> > > > + * @buf: Buffer to write to SPI flash
> > > > + * @offset: Offset from the flash start
> > > > + * @size: Size of the buffer content
> > > > + *
> > > > + * This function probes the SPI flash and updates the specified flash location
> > > > + * with new data from the buffer.
> > > > + */
> > > > +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;
> > > > +}
> > > > +
> > > > +/**
> > > > + * cmd_hw_info_dump() - Dump the hw_info parameters
> > > > + *
> > > > + * This function prints all Marvell hw_info parameters, which are stored in
> > > > + * the SPI flash.
> > > > + */
> > > > +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;
> > > > +}
> > > > +
> > > > +/**
> > > > + * cmd_hw_info_read() - Import the hw_info parameters into U-Boot env
> > > > + * @print_env: Print U-Boot environment after new parameters are imported
> > > > + *
> > > > + * This function reads the Marvell hw_info parameters from SPI flash and
> > > > + * imports them into the U-Boot env.
> > > > + */
> > > > +static int cmd_hw_info_read(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;
> > > > +}
> > > > +
> > > > +/**
> > > > + * cmd_hw_info_save() - Save a parameter from U-Boot env to hw_info parameters
> > > > + * @name: Name of the U-Boot env parameter to save
> > > > + *
> > > > + * This function finds the specified parameter by name in the U-Boot env
> > > > + * and then updates the Marvell hw_info parameters with the new value.
> > > > + */
> > > > +static int cmd_hw_info_save(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;
> > > > +}
> > > > +
> > > > +/**
> > > > + * mac_read_from_eeprom() - Read the parameters from SPI flash.
> > > > + *
> > > > + * This function reads the content of the Marvell hw_info parameters from the
> > > > + * SPI flash and imports them into the U-Boot environment.
> > > > + * This includes MAC addresses and board serial numbers.
> > > > + *
> > > > + * The import is performed only once.
> > > > + *
> > > > + * This function is a part of the U-Boot mac command and must be executed
> > > > + * after SPI flash initialization.
> > > > + */
> > > > +int mac_read_from_eeprom(void)
> > > > +{
> > > > +     if (env_get_ulong(HW_INFO_MERGED_VARIABLE, 10, 0) == 0) {
> > > > +             if (env_set_ulong(HW_INFO_MERGED_VARIABLE, 1))
> > > > +                     return -ENOENT;
> > > > +             return cmd_hw_info_read(false);
> > > > +     }
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * print_platform_help() - Print the platform specific help
> > > > + *
> > > > + * Extend the existing U-Boot mac command description by also printing
> > > > + * platform specific help text.
> > > > + */
> > > > +static void print_platform_help(void)
> > > > +{
> > > > +     printf("\nNote: arguments mac [id|num|errata|date|ports|port_number]\n"
> > > > +            "are unavailable on Marvell Armada A37xx platforms.\n"
> > > > +            "Use mac [read|save {parameter}] instead.\n"
> > > > +            "Available parameters:\n"
> > > > +            "pcb_slm\tPCB SLM number\n"
> > > > +            "pcb_rev\tPCB revision number\n"
> > > > +            "eco_rev\tECO revision number\n"
> > > > +            "pcb_sn\tPCB SN\n"
> > > > +            "ethaddr\tfirst MAC address\n"
> > > > +            "eth[1-9]addr\tsecond-ninth MAC address\n");
> > > > +}
> > > > +
> > > > +/**
> > > > + * do_mac() - Standard U-Boot mac command implementation
> > > > + * @cmdtp: U-Boot command table
> > > > + * @flag: Execution flags
> > > > + * @argc: Count of arguments
> > > > + * @argv: Arguments
> > > > + *
> > > > + * This function implements the standard U-Boot mac command in a mostly
> > > > + * compatible way.
> > > > + * To conform to the general command structure as much as possible, the
> > > > + * command description from cmd/mac.c is followed.
> > > > + * Where not possible, convenient or sensible additional comments for the user
> > > > + * are added.
> > > > + */
> > > > +int do_mac(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> > > > +{
> > > > +     const char *cmd = argv[1];
> > > > +     int ret = 0;
> > > > +
> > > > +     if (argc == 1) {
> > > > +             ret = cmd_hw_info_dump();
> > > > +             if (ret)
> > > > +                     return -EINVAL;
> > > > +             return CMD_RET_SUCCESS;
> > > > +     }
> > > > +
> > > > +     if (!strcmp(cmd, "read")) {
> > > > +             if (cmd_hw_info_read(true))
> > > > +                     return -EINVAL;
> > > > +     } else if (!strcmp(cmd, "save")) {
> > > > +             if (argc != 3) {
> > > > +                     printf("Please pass an additional argument to specify, "
> > > > +                            "which env parameter to save.\n");
> > > > +                     return -EINVAL;
> > > > +             }
> > > > +             if (cmd_hw_info_save(argv[2]))
> > > > +                     return -EINVAL;
> > > > +     } else {
> > > > +             ret = cmd_usage(cmdtp);
> > > > +             print_platform_help();
> > > > +             return ret;
> > > > +     }
> > > > +
> > > > +     return CMD_RET_SUCCESS;
> > > > +}
> > > > diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h
> > > > index c8c34d7d92..8e8bcfa018 100644
> > > > --- a/include/configs/mvebu_armada-37xx.h
> > > > +++ b/include/configs/mvebu_armada-37xx.h
> > > > @@ -62,6 +62,13 @@
> > > >   */
> > > >  #define DEFAULT_ENV_IS_RW            /* required for configuring default fdtfile= */
> > > >
> > > > +/*
> > > > + * Platform identification (Marvell hw_info parameters)
> > > > + */
> > > > +#ifdef CONFIG_MVEBU_MAC_HW_INFO
> > > > +#define CONFIG_ID_EEPROM /* U-Boot mac command */
> > > > +#endif
> > > > +
> > > >  /*
> > > >   * Ethernet Driver configuration
> > > >   */
> > > > 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,
> > > > --
> > > > 2.31.1
> > > >
> >
> > Kind regards,
> > Luka

Kind regards,
Luka

On Friday 13 August 2021 11:51:02 Luka Kovacic wrote:
> Hello Pali,
> 
> On Fri, Aug 13, 2021 at 11:41 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Friday 13 August 2021 11:23:15 Luka Kovacic wrote:
> > > Hello Marek and Pali,
> > >
> > > On Fri, Aug 13, 2021 at 10:23 AM Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Friday 13 August 2021 01:39:36 Luka Kovacic wrote:
> > > > > The mac 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.
> > > > >
> > > > > Currently the mac 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:
> > > > >  => mac read
> > > > >  => saveenv
> > > >
> > > > Hello Luka!
> > > >
> > > > You are just using above commands for accessing following variables,
> > > > right?
> > > >
> > > > pcb_slm
> > > > pcb_rev
> > > > eco_rev
> > > > pcb_sn
> > > > ethaddr
> > > > eth1addr
> > > > eth2addr
> > > > eth3addr
> > > > eth4addr
> > > > eth5addr
> > > > eth6addr
> > > > eth7addr
> > > > eth8addr
> > > > eth9addr
> > > >
> > >
> > > Yes, I am accessing these variables.
> > >
> > > > So why is additional command required? Espressobin boards can already
> > > > load and use correct eth*addr variables, which is implemented in board
> > > > file.
> > >
> > > The current implementation in the board file preserves currently stored MAC
> > > addresses, which are in the U-Boot environment.
> >
> > Yes, this is for v5/v7 variants. But it can be easily modified for ultra
> > to read mac addresses from this custom storage.
> >
> > > My implementation is suitable for the ESPRESSOBin-Ultra, as the MACs
> > > are stored in the hw_info area of the SPI flash and the tool imports them
> > > into the normal U-Boot env.
> > > They can also be modified using the tool - we won't be able to get this
> > > functionality in the board file.
> >
> > Modification by env variables is also possible from the board file. Look
> > at U_BOOT_ENV_CALLBACK() functionality, which allows to call custom code
> > when trying to change/delete particular env variable.
> >
> > > > So what about implementing this import for all above variables for ultra
> > > > variant in board file, like it is already for all other variants? I have
> > > > already suggested it in the past. As I think this approach is just one
> > > > giant and complicated hack...
> > > > https://lore.kernel.org/u-boot/20210301151721.xt62rdun5dnlevqk@pali/
> > > >
> > > > I guess that it should be enough to import variables from SPI to env in
> > > > board_late_init() function.
> > >
> > > Currently, the MACs are automatically imported from the
> > > common/board_r.c file at boot, I think this is more standard and cleaner.
> >
> > Hm... then I somehow misunderstood how it works.
> >
> > Because I do not see any change in common/board_r.c which could read or
> > import mac addresses via calling this new "mac read" command.
> 
> The function mac_read_from_eeprom() is called, if CONFIG_ID_EEPROM is
> enabled. This has also been tested and a mechanism was added to avoid
> duplicate imports (if the user wants to modify the MAC address, so it then
> isn't overwritten).
> 
> https://source.denx.de/u-boot/u-boot/-/blob/master/common/board_r.c#L724

Ok, thank you. Now I see how it works. From commit message and from the
first look at the code it was not clear for me.

So in this patch you are implementing:

1) mac_read_from_eeprom() function for CONFIG_ID_EEPROM

2) do_mac() function for CONFIG_ID_EEPROM

which looks fine now.

> >
> > > Nevertheless, I believe this a sufficient solution for now,
> > > as this has been suggested as a solution in the previous patchset
> > > comments too.

Ok, this seems to be sufficient.

> > > >
> > > > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr>
> > > > > Cc: Luka Perkov <luka.perkov@sartura.hr>
> > > > > Cc: Robert Marko <robert.marko@sartura.hr>
> > > > > ---
> > > > >  arch/arm/mach-mvebu/Kconfig                   |   1 +
> > > > >  board/Marvell/mvebu_armada-37xx/Kconfig       |  29 ++
> > > > >  board/Marvell/mvebu_armada-37xx/Makefile      |   3 +-
> > > > >  board/Marvell/mvebu_armada-37xx/mac/Makefile  |   5 +
> > > > >  board/Marvell/mvebu_armada-37xx/mac/hw_info.c | 391 ++++++++++++++++++

What about renaming this file just to?

board/Marvell/mvebu_armada-37xx/mac.c

I do not know if we need separate directory just for one hw_info.c file.

> > > > >  include/configs/mvebu_armada-37xx.h           |   7 +
> > > > >  lib/hashtable.c                               |   2 +-
> > > > >  7 files changed, 436 insertions(+), 2 deletions(-)
> > > > >  create mode 100644 board/Marvell/mvebu_armada-37xx/Kconfig
> > > > >  create mode 100644 board/Marvell/mvebu_armada-37xx/mac/Makefile
> > > > >  create mode 100644 board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> > > > >
> > > > > diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> > > > > index 89737a37ad..dff9f05b28 100644
> > > > > --- a/arch/arm/mach-mvebu/Kconfig
> > > > > +++ b/arch/arm/mach-mvebu/Kconfig
> > > > > @@ -312,6 +312,7 @@ config SECURED_MODE_CSK_INDEX
> > > > >       default 0
> > > > >       depends on SECURED_MODE_IMAGE
> > > > >
> > > > > +source "board/Marvell/mvebu_armada-37xx/Kconfig"
> > > > >  source "board/solidrun/clearfog/Kconfig"
> > > > >  source "board/kobol/helios4/Kconfig"
> > > > >
> > > > > diff --git a/board/Marvell/mvebu_armada-37xx/Kconfig b/board/Marvell/mvebu_armada-37xx/Kconfig
> > > > > new file mode 100644
> > > > > index 0000000000..b84dd20023
> > > > > --- /dev/null
> > > > > +++ b/board/Marvell/mvebu_armada-37xx/Kconfig
> > > > > @@ -0,0 +1,29 @@
> > > > > +menu "Marvell Armada 37xx configuration"
> > > > > +depends on TARGET_MVEBU_ARMADA_37XX
> > > > > +
> > > > > +config MVEBU_MAC_HW_INFO
> > > > > +     bool "Marvell hw_info (mac) support"
> > > > > +     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.
> > > > > +       After enabled, these parameters are managed from the common
> > > > > +       U-Boot mac command.
> > > > > +
> > > > > +config MVEBU_MAC_HW_INFO_OFFSET
> > > > > +     hex "Marvell hw_info (mac) SPI flash offset"
> > > > > +     depends on MVEBU_MAC_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.
> > > > > +
> > > > > +endmenu
> > > > > diff --git a/board/Marvell/mvebu_armada-37xx/Makefile b/board/Marvell/mvebu_armada-37xx/Makefile
> > > > > index 27221557c7..6f6ac6b193 100644
> > > > > --- a/board/Marvell/mvebu_armada-37xx/Makefile
> > > > > +++ b/board/Marvell/mvebu_armada-37xx/Makefile
> > > > > @@ -2,4 +2,5 @@
> > > > >  #
> > > > >  # Copyright (C) 2016 Stefan Roese <sr@denx.de>
> > > > >
> > > > > -obj-y        := board.o
> > > > > +obj-y += board.o
> > > > > +obj-y += mac/
> > > > > diff --git a/board/Marvell/mvebu_armada-37xx/mac/Makefile b/board/Marvell/mvebu_armada-37xx/mac/Makefile
> > > > > new file mode 100644
> > > > > index 0000000000..7c30fe6194
> > > > > --- /dev/null
> > > > > +++ b/board/Marvell/mvebu_armada-37xx/mac/Makefile
> > > > > @@ -0,0 +1,5 @@
> > > > > +# SPDX-License-Identifier: GPL-2.0+
> > > > > +#
> > > > > +# Copyright (c) 2021 Sartura Ltd.
> > > > > +
> > > > > +obj-$(CONFIG_ID_EEPROM) += hw_info.o
> > > > > diff --git a/board/Marvell/mvebu_armada-37xx/mac/hw_info.c b/board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> > > > > new file mode 100644
> > > > > index 0000000000..91e5cd8dcf
> > > > > --- /dev/null
> > > > > +++ b/board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> > > > > @@ -0,0 +1,391 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > +/*
> > > > > + * Marvell hw_info (mac) command implementation
> > > > > + * 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@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_MVEBU_MAC_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
> > > > > +
> > > > > +#define HW_INFO_MERGED_VARIABLE              "read_board_hw_info"
> > > > > +
> > > > > +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;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * read_spi_flash_offset() - Read data from the SPI flash
> > > > > + * @buf: Buffer to write in
> > > > > + * @offset: Offset from the flash start
> > > > > + *
> > > > > + * Read SPI flash data into the buffer from offset to HW_INFO_MAX_ENV_SIZE.
> > > > > + */
> > > > > +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;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * write_spi_flash_offset() - Write a buffer to SPI flash
> > > > > + * @buf: Buffer to write to SPI flash
> > > > > + * @offset: Offset from the flash start
> > > > > + * @size: Size of the buffer content
> > > > > + *
> > > > > + * This function probes the SPI flash and updates the specified flash location
> > > > > + * with new data from the buffer.
> > > > > + */
> > > > > +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;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * cmd_hw_info_dump() - Dump the hw_info parameters
> > > > > + *
> > > > > + * This function prints all Marvell hw_info parameters, which are stored in
> > > > > + * the SPI flash.
> > > > > + */
> > > > > +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;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * cmd_hw_info_read() - Import the hw_info parameters into U-Boot env
> > > > > + * @print_env: Print U-Boot environment after new parameters are imported
> > > > > + *
> > > > > + * This function reads the Marvell hw_info parameters from SPI flash and
> > > > > + * imports them into the U-Boot env.
> > > > > + */
> > > > > +static int cmd_hw_info_read(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;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * cmd_hw_info_save() - Save a parameter from U-Boot env to hw_info parameters
> > > > > + * @name: Name of the U-Boot env parameter to save
> > > > > + *
> > > > > + * This function finds the specified parameter by name in the U-Boot env
> > > > > + * and then updates the Marvell hw_info parameters with the new value.
> > > > > + */
> > > > > +static int cmd_hw_info_save(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;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * mac_read_from_eeprom() - Read the parameters from SPI flash.
> > > > > + *
> > > > > + * This function reads the content of the Marvell hw_info parameters from the
> > > > > + * SPI flash and imports them into the U-Boot environment.
> > > > > + * This includes MAC addresses and board serial numbers.
> > > > > + *
> > > > > + * The import is performed only once.
> > > > > + *
> > > > > + * This function is a part of the U-Boot mac command and must be executed
> > > > > + * after SPI flash initialization.
> > > > > + */
> > > > > +int mac_read_from_eeprom(void)
> > > > > +{
> > > > > +     if (env_get_ulong(HW_INFO_MERGED_VARIABLE, 10, 0) == 0) {
> > > > > +             if (env_set_ulong(HW_INFO_MERGED_VARIABLE, 1))
> > > > > +                     return -ENOENT;
> > > > > +             return cmd_hw_info_read(false);
> > > > > +     }
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * print_platform_help() - Print the platform specific help
> > > > > + *
> > > > > + * Extend the existing U-Boot mac command description by also printing
> > > > > + * platform specific help text.
> > > > > + */
> > > > > +static void print_platform_help(void)
> > > > > +{
> > > > > +     printf("\nNote: arguments mac [id|num|errata|date|ports|port_number]\n"
> > > > > +            "are unavailable on Marvell Armada A37xx platforms.\n"
> > > > > +            "Use mac [read|save {parameter}] instead.\n"
> > > > > +            "Available parameters:\n"
> > > > > +            "pcb_slm\tPCB SLM number\n"
> > > > > +            "pcb_rev\tPCB revision number\n"
> > > > > +            "eco_rev\tECO revision number\n"
> > > > > +            "pcb_sn\tPCB SN\n"
> > > > > +            "ethaddr\tfirst MAC address\n"
> > > > > +            "eth[1-9]addr\tsecond-ninth MAC address\n");
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * do_mac() - Standard U-Boot mac command implementation
> > > > > + * @cmdtp: U-Boot command table
> > > > > + * @flag: Execution flags
> > > > > + * @argc: Count of arguments
> > > > > + * @argv: Arguments
> > > > > + *
> > > > > + * This function implements the standard U-Boot mac command in a mostly
> > > > > + * compatible way.
> > > > > + * To conform to the general command structure as much as possible, the
> > > > > + * command description from cmd/mac.c is followed.
> > > > > + * Where not possible, convenient or sensible additional comments for the user
> > > > > + * are added.
> > > > > + */
> > > > > +int do_mac(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> > > > > +{
> > > > > +     const char *cmd = argv[1];
> > > > > +     int ret = 0;
> > > > > +
> > > > > +     if (argc == 1) {
> > > > > +             ret = cmd_hw_info_dump();
> > > > > +             if (ret)
> > > > > +                     return -EINVAL;
> > > > > +             return CMD_RET_SUCCESS;
> > > > > +     }
> > > > > +
> > > > > +     if (!strcmp(cmd, "read")) {
> > > > > +             if (cmd_hw_info_read(true))
> > > > > +                     return -EINVAL;
> > > > > +     } else if (!strcmp(cmd, "save")) {
> > > > > +             if (argc != 3) {
> > > > > +                     printf("Please pass an additional argument to specify, "
> > > > > +                            "which env parameter to save.\n");
> > > > > +                     return -EINVAL;
> > > > > +             }
> > > > > +             if (cmd_hw_info_save(argv[2]))
> > > > > +                     return -EINVAL;
> > > > > +     } else {
> > > > > +             ret = cmd_usage(cmdtp);
> > > > > +             print_platform_help();
> > > > > +             return ret;
> > > > > +     }
> > > > > +
> > > > > +     return CMD_RET_SUCCESS;
> > > > > +}
> > > > > diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h
> > > > > index c8c34d7d92..8e8bcfa018 100644
> > > > > --- a/include/configs/mvebu_armada-37xx.h
> > > > > +++ b/include/configs/mvebu_armada-37xx.h
> > > > > @@ -62,6 +62,13 @@
> > > > >   */
> > > > >  #define DEFAULT_ENV_IS_RW            /* required for configuring default fdtfile= */
> > > > >
> > > > > +/*
> > > > > + * Platform identification (Marvell hw_info parameters)
> > > > > + */
> > > > > +#ifdef CONFIG_MVEBU_MAC_HW_INFO
> > > > > +#define CONFIG_ID_EEPROM /* U-Boot mac command */
> > > > > +#endif
> > > > > +
> > > > >  /*
> > > > >   * Ethernet Driver configuration
> > > > >   */
> > > > > 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,
> > > > > --
> > > > > 2.31.1
> > > > >
> > >
> > > Kind regards,
> > > Luka
> 
> Kind regards,
> Luka

On Fri, Aug 13, 2021 at 12:09 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Friday 13 August 2021 11:51:02 Luka Kovacic wrote:
> > Hello Pali,
> >
> > On Fri, Aug 13, 2021 at 11:41 AM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Friday 13 August 2021 11:23:15 Luka Kovacic wrote:
> > > > Hello Marek and Pali,
> > > >
> > > > On Fri, Aug 13, 2021 at 10:23 AM Pali Rohár <pali@kernel.org> wrote:
> > > > >
> > > > > On Friday 13 August 2021 01:39:36 Luka Kovacic wrote:
> > > > > > The mac 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.
> > > > > >
> > > > > > Currently the mac 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:
> > > > > >  => mac read
> > > > > >  => saveenv
> > > > >
> > > > > Hello Luka!
> > > > >
> > > > > You are just using above commands for accessing following variables,
> > > > > right?
> > > > >
> > > > > pcb_slm
> > > > > pcb_rev
> > > > > eco_rev
> > > > > pcb_sn
> > > > > ethaddr
> > > > > eth1addr
> > > > > eth2addr
> > > > > eth3addr
> > > > > eth4addr
> > > > > eth5addr
> > > > > eth6addr
> > > > > eth7addr
> > > > > eth8addr
> > > > > eth9addr
> > > > >
> > > >
> > > > Yes, I am accessing these variables.
> > > >
> > > > > So why is additional command required? Espressobin boards can already
> > > > > load and use correct eth*addr variables, which is implemented in board
> > > > > file.
> > > >
> > > > The current implementation in the board file preserves currently stored MAC
> > > > addresses, which are in the U-Boot environment.
> > >
> > > Yes, this is for v5/v7 variants. But it can be easily modified for ultra
> > > to read mac addresses from this custom storage.
> > >
> > > > My implementation is suitable for the ESPRESSOBin-Ultra, as the MACs
> > > > are stored in the hw_info area of the SPI flash and the tool imports them
> > > > into the normal U-Boot env.
> > > > They can also be modified using the tool - we won't be able to get this
> > > > functionality in the board file.
> > >
> > > Modification by env variables is also possible from the board file. Look
> > > at U_BOOT_ENV_CALLBACK() functionality, which allows to call custom code
> > > when trying to change/delete particular env variable.
> > >
> > > > > So what about implementing this import for all above variables for ultra
> > > > > variant in board file, like it is already for all other variants? I have
> > > > > already suggested it in the past. As I think this approach is just one
> > > > > giant and complicated hack...
> > > > > https://lore.kernel.org/u-boot/20210301151721.xt62rdun5dnlevqk@pali/
> > > > >
> > > > > I guess that it should be enough to import variables from SPI to env in
> > > > > board_late_init() function.
> > > >
> > > > Currently, the MACs are automatically imported from the
> > > > common/board_r.c file at boot, I think this is more standard and cleaner.
> > >
> > > Hm... then I somehow misunderstood how it works.
> > >
> > > Because I do not see any change in common/board_r.c which could read or
> > > import mac addresses via calling this new "mac read" command.
> >
> > The function mac_read_from_eeprom() is called, if CONFIG_ID_EEPROM is
> > enabled. This has also been tested and a mechanism was added to avoid
> > duplicate imports (if the user wants to modify the MAC address, so it then
> > isn't overwritten).
> >
> > https://source.denx.de/u-boot/u-boot/-/blob/master/common/board_r.c#L724
>
> Ok, thank you. Now I see how it works. From commit message and from the
> first look at the code it was not clear for me.
>
> So in this patch you are implementing:
>
> 1) mac_read_from_eeprom() function for CONFIG_ID_EEPROM
>
> 2) do_mac() function for CONFIG_ID_EEPROM
>
> which looks fine now.
>
> > >
> > > > Nevertheless, I believe this a sufficient solution for now,
> > > > as this has been suggested as a solution in the previous patchset
> > > > comments too.
>
> Ok, this seems to be sufficient.
>
> > > > >
> > > > > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr>
> > > > > > Cc: Luka Perkov <luka.perkov@sartura.hr>
> > > > > > Cc: Robert Marko <robert.marko@sartura.hr>
> > > > > > ---
> > > > > >  arch/arm/mach-mvebu/Kconfig                   |   1 +
> > > > > >  board/Marvell/mvebu_armada-37xx/Kconfig       |  29 ++
> > > > > >  board/Marvell/mvebu_armada-37xx/Makefile      |   3 +-
> > > > > >  board/Marvell/mvebu_armada-37xx/mac/Makefile  |   5 +
> > > > > >  board/Marvell/mvebu_armada-37xx/mac/hw_info.c | 391 ++++++++++++++++++
>
> What about renaming this file just to?
>
> board/Marvell/mvebu_armada-37xx/mac.c
>
> I do not know if we need separate directory just for one hw_info.c file.

Sure, I can rename this.

>
> > > > > >  include/configs/mvebu_armada-37xx.h           |   7 +
> > > > > >  lib/hashtable.c                               |   2 +-
> > > > > >  7 files changed, 436 insertions(+), 2 deletions(-)
> > > > > >  create mode 100644 board/Marvell/mvebu_armada-37xx/Kconfig
> > > > > >  create mode 100644 board/Marvell/mvebu_armada-37xx/mac/Makefile
> > > > > >  create mode 100644 board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> > > > > >
> > > > > > diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> > > > > > index 89737a37ad..dff9f05b28 100644
> > > > > > --- a/arch/arm/mach-mvebu/Kconfig
> > > > > > +++ b/arch/arm/mach-mvebu/Kconfig
> > > > > > @@ -312,6 +312,7 @@ config SECURED_MODE_CSK_INDEX
> > > > > >       default 0
> > > > > >       depends on SECURED_MODE_IMAGE
> > > > > >
> > > > > > +source "board/Marvell/mvebu_armada-37xx/Kconfig"
> > > > > >  source "board/solidrun/clearfog/Kconfig"
> > > > > >  source "board/kobol/helios4/Kconfig"
> > > > > >
> > > > > > diff --git a/board/Marvell/mvebu_armada-37xx/Kconfig b/board/Marvell/mvebu_armada-37xx/Kconfig
> > > > > > new file mode 100644
> > > > > > index 0000000000..b84dd20023
> > > > > > --- /dev/null
> > > > > > +++ b/board/Marvell/mvebu_armada-37xx/Kconfig
> > > > > > @@ -0,0 +1,29 @@
> > > > > > +menu "Marvell Armada 37xx configuration"
> > > > > > +depends on TARGET_MVEBU_ARMADA_37XX
> > > > > > +
> > > > > > +config MVEBU_MAC_HW_INFO
> > > > > > +     bool "Marvell hw_info (mac) support"
> > > > > > +     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.
> > > > > > +       After enabled, these parameters are managed from the common
> > > > > > +       U-Boot mac command.
> > > > > > +
> > > > > > +config MVEBU_MAC_HW_INFO_OFFSET
> > > > > > +     hex "Marvell hw_info (mac) SPI flash offset"
> > > > > > +     depends on MVEBU_MAC_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.
> > > > > > +
> > > > > > +endmenu
> > > > > > diff --git a/board/Marvell/mvebu_armada-37xx/Makefile b/board/Marvell/mvebu_armada-37xx/Makefile
> > > > > > index 27221557c7..6f6ac6b193 100644
> > > > > > --- a/board/Marvell/mvebu_armada-37xx/Makefile
> > > > > > +++ b/board/Marvell/mvebu_armada-37xx/Makefile
> > > > > > @@ -2,4 +2,5 @@
> > > > > >  #
> > > > > >  # Copyright (C) 2016 Stefan Roese <sr@denx.de>
> > > > > >
> > > > > > -obj-y        := board.o
> > > > > > +obj-y += board.o
> > > > > > +obj-y += mac/
> > > > > > diff --git a/board/Marvell/mvebu_armada-37xx/mac/Makefile b/board/Marvell/mvebu_armada-37xx/mac/Makefile
> > > > > > new file mode 100644
> > > > > > index 0000000000..7c30fe6194
> > > > > > --- /dev/null
> > > > > > +++ b/board/Marvell/mvebu_armada-37xx/mac/Makefile
> > > > > > @@ -0,0 +1,5 @@
> > > > > > +# SPDX-License-Identifier: GPL-2.0+
> > > > > > +#
> > > > > > +# Copyright (c) 2021 Sartura Ltd.
> > > > > > +
> > > > > > +obj-$(CONFIG_ID_EEPROM) += hw_info.o
> > > > > > diff --git a/board/Marvell/mvebu_armada-37xx/mac/hw_info.c b/board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> > > > > > new file mode 100644
> > > > > > index 0000000000..91e5cd8dcf
> > > > > > --- /dev/null
> > > > > > +++ b/board/Marvell/mvebu_armada-37xx/mac/hw_info.c
> > > > > > @@ -0,0 +1,391 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > +/*
> > > > > > + * Marvell hw_info (mac) command implementation
> > > > > > + * 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@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_MVEBU_MAC_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
> > > > > > +
> > > > > > +#define HW_INFO_MERGED_VARIABLE              "read_board_hw_info"
> > > > > > +
> > > > > > +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;
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * read_spi_flash_offset() - Read data from the SPI flash
> > > > > > + * @buf: Buffer to write in
> > > > > > + * @offset: Offset from the flash start
> > > > > > + *
> > > > > > + * Read SPI flash data into the buffer from offset to HW_INFO_MAX_ENV_SIZE.
> > > > > > + */
> > > > > > +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;
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * write_spi_flash_offset() - Write a buffer to SPI flash
> > > > > > + * @buf: Buffer to write to SPI flash
> > > > > > + * @offset: Offset from the flash start
> > > > > > + * @size: Size of the buffer content
> > > > > > + *
> > > > > > + * This function probes the SPI flash and updates the specified flash location
> > > > > > + * with new data from the buffer.
> > > > > > + */
> > > > > > +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;
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * cmd_hw_info_dump() - Dump the hw_info parameters
> > > > > > + *
> > > > > > + * This function prints all Marvell hw_info parameters, which are stored in
> > > > > > + * the SPI flash.
> > > > > > + */
> > > > > > +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;
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * cmd_hw_info_read() - Import the hw_info parameters into U-Boot env
> > > > > > + * @print_env: Print U-Boot environment after new parameters are imported
> > > > > > + *
> > > > > > + * This function reads the Marvell hw_info parameters from SPI flash and
> > > > > > + * imports them into the U-Boot env.
> > > > > > + */
> > > > > > +static int cmd_hw_info_read(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;
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * cmd_hw_info_save() - Save a parameter from U-Boot env to hw_info parameters
> > > > > > + * @name: Name of the U-Boot env parameter to save
> > > > > > + *
> > > > > > + * This function finds the specified parameter by name in the U-Boot env
> > > > > > + * and then updates the Marvell hw_info parameters with the new value.
> > > > > > + */
> > > > > > +static int cmd_hw_info_save(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;
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * mac_read_from_eeprom() - Read the parameters from SPI flash.
> > > > > > + *
> > > > > > + * This function reads the content of the Marvell hw_info parameters from the
> > > > > > + * SPI flash and imports them into the U-Boot environment.
> > > > > > + * This includes MAC addresses and board serial numbers.
> > > > > > + *
> > > > > > + * The import is performed only once.
> > > > > > + *
> > > > > > + * This function is a part of the U-Boot mac command and must be executed
> > > > > > + * after SPI flash initialization.
> > > > > > + */
> > > > > > +int mac_read_from_eeprom(void)
> > > > > > +{
> > > > > > +     if (env_get_ulong(HW_INFO_MERGED_VARIABLE, 10, 0) == 0) {
> > > > > > +             if (env_set_ulong(HW_INFO_MERGED_VARIABLE, 1))
> > > > > > +                     return -ENOENT;
> > > > > > +             return cmd_hw_info_read(false);
> > > > > > +     }
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * print_platform_help() - Print the platform specific help
> > > > > > + *
> > > > > > + * Extend the existing U-Boot mac command description by also printing
> > > > > > + * platform specific help text.
> > > > > > + */
> > > > > > +static void print_platform_help(void)
> > > > > > +{
> > > > > > +     printf("\nNote: arguments mac [id|num|errata|date|ports|port_number]\n"
> > > > > > +            "are unavailable on Marvell Armada A37xx platforms.\n"
> > > > > > +            "Use mac [read|save {parameter}] instead.\n"
> > > > > > +            "Available parameters:\n"
> > > > > > +            "pcb_slm\tPCB SLM number\n"
> > > > > > +            "pcb_rev\tPCB revision number\n"
> > > > > > +            "eco_rev\tECO revision number\n"
> > > > > > +            "pcb_sn\tPCB SN\n"
> > > > > > +            "ethaddr\tfirst MAC address\n"
> > > > > > +            "eth[1-9]addr\tsecond-ninth MAC address\n");
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * do_mac() - Standard U-Boot mac command implementation
> > > > > > + * @cmdtp: U-Boot command table
> > > > > > + * @flag: Execution flags
> > > > > > + * @argc: Count of arguments
> > > > > > + * @argv: Arguments
> > > > > > + *
> > > > > > + * This function implements the standard U-Boot mac command in a mostly
> > > > > > + * compatible way.
> > > > > > + * To conform to the general command structure as much as possible, the
> > > > > > + * command description from cmd/mac.c is followed.
> > > > > > + * Where not possible, convenient or sensible additional comments for the user
> > > > > > + * are added.
> > > > > > + */
> > > > > > +int do_mac(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> > > > > > +{
> > > > > > +     const char *cmd = argv[1];
> > > > > > +     int ret = 0;
> > > > > > +
> > > > > > +     if (argc == 1) {
> > > > > > +             ret = cmd_hw_info_dump();
> > > > > > +             if (ret)
> > > > > > +                     return -EINVAL;
> > > > > > +             return CMD_RET_SUCCESS;
> > > > > > +     }
> > > > > > +
> > > > > > +     if (!strcmp(cmd, "read")) {
> > > > > > +             if (cmd_hw_info_read(true))
> > > > > > +                     return -EINVAL;
> > > > > > +     } else if (!strcmp(cmd, "save")) {
> > > > > > +             if (argc != 3) {
> > > > > > +                     printf("Please pass an additional argument to specify, "
> > > > > > +                            "which env parameter to save.\n");
> > > > > > +                     return -EINVAL;
> > > > > > +             }
> > > > > > +             if (cmd_hw_info_save(argv[2]))
> > > > > > +                     return -EINVAL;
> > > > > > +     } else {
> > > > > > +             ret = cmd_usage(cmdtp);
> > > > > > +             print_platform_help();
> > > > > > +             return ret;
> > > > > > +     }
> > > > > > +
> > > > > > +     return CMD_RET_SUCCESS;
> > > > > > +}
> > > > > > diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h
> > > > > > index c8c34d7d92..8e8bcfa018 100644
> > > > > > --- a/include/configs/mvebu_armada-37xx.h
> > > > > > +++ b/include/configs/mvebu_armada-37xx.h
> > > > > > @@ -62,6 +62,13 @@
> > > > > >   */
> > > > > >  #define DEFAULT_ENV_IS_RW            /* required for configuring default fdtfile= */
> > > > > >
> > > > > > +/*
> > > > > > + * Platform identification (Marvell hw_info parameters)
> > > > > > + */
> > > > > > +#ifdef CONFIG_MVEBU_MAC_HW_INFO
> > > > > > +#define CONFIG_ID_EEPROM /* U-Boot mac command */
> > > > > > +#endif
> > > > > > +
> > > > > >  /*
> > > > > >   * Ethernet Driver configuration
> > > > > >   */
> > > > > > 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,
> > > > > > --
> > > > > > 2.31.1
> > > > > >
> > > >
> > > > Kind regards,
> > > > Luka
> >
> > Kind regards,
> > Luka

Kind regards,
Luka

On Friday 13 August 2021 01:39:36 Luka Kovacic wrote:
> diff --git a/board/Marvell/mvebu_armada-37xx/Kconfig b/board/Marvell/mvebu_armada-37xx/Kconfig
> new file mode 100644
> index 0000000000..b84dd20023
> --- /dev/null
> +++ b/board/Marvell/mvebu_armada-37xx/Kconfig
> @@ -0,0 +1,29 @@
> +menu "Marvell Armada 37xx configuration"
> +depends on TARGET_MVEBU_ARMADA_37XX
> +
> +config MVEBU_MAC_HW_INFO
> +	bool "Marvell hw_info (mac) support"
> +	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.
> +	  After enabled, these parameters are managed from the common
> +	  U-Boot mac command.
> +
> +config MVEBU_MAC_HW_INFO_OFFSET
> +	hex "Marvell hw_info (mac) SPI flash offset"
> +	depends on MVEBU_MAC_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.

Just a question, cannot we load this offset from DTS? In DTS are already
specified SPI partitions, so this could eliminate need for defining this
offset at two places. But I really do not know at which time is this
code called, if DTB is available at this time or not.

> +endmenu

On Fri, Aug 13, 2021 at 12:29 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Friday 13 August 2021 01:39:36 Luka Kovacic wrote:
> > diff --git a/board/Marvell/mvebu_armada-37xx/Kconfig b/board/Marvell/mvebu_armada-37xx/Kconfig
> > new file mode 100644
> > index 0000000000..b84dd20023
> > --- /dev/null
> > +++ b/board/Marvell/mvebu_armada-37xx/Kconfig
> > @@ -0,0 +1,29 @@
> > +menu "Marvell Armada 37xx configuration"
> > +depends on TARGET_MVEBU_ARMADA_37XX
> > +
> > +config MVEBU_MAC_HW_INFO
> > +     bool "Marvell hw_info (mac) support"
> > +     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.
> > +       After enabled, these parameters are managed from the common
> > +       U-Boot mac command.
> > +
> > +config MVEBU_MAC_HW_INFO_OFFSET
> > +     hex "Marvell hw_info (mac) SPI flash offset"
> > +     depends on MVEBU_MAC_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.
>
> Just a question, cannot we load this offset from DTS? In DTS are already
> specified SPI partitions, so this could eliminate need for defining this
> offset at two places. But I really do not know at which time is this
> code called, if DTB is available at this time or not.

The code is called right after cpu_secondary_init_r.
I'm not sure, there also some other values, which are hard-coded and
so far I didn't really see any other possible offset.

Are you aware of any other relevant board with the Marvell hw_info
parameters?

>
> > +endmenu

Kind regards,
Luka

On Friday 13 August 2021 12:43:47 Luka Kovacic wrote:
> On Fri, Aug 13, 2021 at 12:29 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Friday 13 August 2021 01:39:36 Luka Kovacic wrote:
> > > diff --git a/board/Marvell/mvebu_armada-37xx/Kconfig b/board/Marvell/mvebu_armada-37xx/Kconfig
> > > new file mode 100644
> > > index 0000000000..b84dd20023
> > > --- /dev/null
> > > +++ b/board/Marvell/mvebu_armada-37xx/Kconfig
> > > @@ -0,0 +1,29 @@
> > > +menu "Marvell Armada 37xx configuration"
> > > +depends on TARGET_MVEBU_ARMADA_37XX
> > > +
> > > +config MVEBU_MAC_HW_INFO
> > > +     bool "Marvell hw_info (mac) support"
> > > +     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.
> > > +       After enabled, these parameters are managed from the common
> > > +       U-Boot mac command.
> > > +
> > > +config MVEBU_MAC_HW_INFO_OFFSET
> > > +     hex "Marvell hw_info (mac) SPI flash offset"
> > > +     depends on MVEBU_MAC_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.
> >
> > Just a question, cannot we load this offset from DTS? In DTS are already
> > specified SPI partitions, so this could eliminate need for defining this
> > offset at two places. But I really do not know at which time is this
> > code called, if DTB is available at this time or not.
> 
> The code is called right after cpu_secondary_init_r.
> I'm not sure, there also some other values, which are hard-coded and
> so far I didn't really see any other possible offset.
> 
> Are you aware of any other relevant board with the Marvell hw_info
> parameters?

I do not know any other board which uses this hw_info. And because you
adding config option for address, I though that there are more boards
with different addresses... And so I was thinking if it cannot be loaded
fro DTS.

I can check espressobin v5 if there is not some "hidden" hw_info stuff
somewhere...

> >
> > > +endmenu
> 
> Kind regards,
> Luka