[PATCH 1/2] arm: mvebu: Implement the mac command (Marvell hw_info)

[Pali Rohár]

Hello!

On Friday 08 October 2021 15:28:03 Luka Kovacic wrote:
> Hello Pali,
> 
> On Fri, Oct 8, 2021 at 2:53 PM Pali Rohár <pali at kernel.org> wrote:
> >
> > Hello!
> >
> > On Friday 08 October 2021 14:09:23 Robert Marko wrote:
> > > From: Luka Kovacic <luka.kovacic at sartura.hr>
> > >
> > > The mac command is implemented to enable parsing Marvell hw_info formatted
> > > environments. This format is often used on Marvell Armada 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.
> >
> > Is there any documentation how is checksum stored in this hw_info
> > structure?
> >
> 
> There probably isn't any public documentation.
> This implementation was written using the public Marvell U-Boot source code
> which is hosted on GitHub (MarvellEmbeddedProcessors/u-boot-marvell).
> 
> Anyway, this shouldn't be much of a problem for the initial version, as SPI
> flash is quite reliable and data written here can also be read by the official
> version and vice versa.

Are you planning to implement that checksum verification support? I
understand that purpose of checksum is there to ensure that only valid
data are processed and used.

> > > 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.
> > >
> > > Support for this command is added for Marvell Armada A37XX and 7K/8K
> > > devices.
> > >
> > > Usage example:
> > >  => mac read
> > >  => saveenv
> > >
> > > Signed-off-by: Luka Kovacic <luka.kovacic at sartura.hr>
> > > Cc: Luka Perkov <luka.perkov at sartura.hr>
> > > Cc: Robert Marko <robert.marko at sartura.hr>
> > > ---
> > >  arch/arm/mach-mvebu/Kconfig         |   1 +
> > >  board/Marvell/common/Kconfig        |  29 +++
> > >  board/Marvell/common/Makefile       |   5 +
> > >  board/Marvell/common/mac.c          | 391 ++++++++++++++++++++++++++++
> > >  include/configs/mvebu_armada-37xx.h |   7 +
> > >  include/configs/mvebu_armada-8k.h   |   7 +
> > >  lib/hashtable.c                     |   2 +-
> > >  7 files changed, 441 insertions(+), 1 deletion(-)
> > >  create mode 100644 board/Marvell/common/Kconfig
> > >  create mode 100644 board/Marvell/common/Makefile
> > >  create mode 100644 board/Marvell/common/mac.c
> > >
> > > diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> > > index 087643725e..d48de626ea 100644
> > > --- a/arch/arm/mach-mvebu/Kconfig
> > > +++ b/arch/arm/mach-mvebu/Kconfig
> > > @@ -336,6 +336,7 @@ config SECURED_MODE_CSK_INDEX
> > >       default 0
> > >       depends on SECURED_MODE_IMAGE
> > >
> > > +source "board/Marvell/common/Kconfig"
> > >  source "board/solidrun/clearfog/Kconfig"
> > >  source "board/kobol/helios4/Kconfig"
> > >
> > > diff --git a/board/Marvell/common/Kconfig b/board/Marvell/common/Kconfig
> > > new file mode 100644
> > > index 0000000000..473a83b05b
> > > --- /dev/null
> > > +++ b/board/Marvell/common/Kconfig
> > > @@ -0,0 +1,29 @@
> > > +menu "Marvell Armada common configuration"
> > > +depends on TARGET_MVEBU_ARMADA_37XX || TARGET_MVEBU_ARMADA_8K
> > > +
> > > +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.
> >
> > Have you tried to specify this offset directly into DTS file? Because
> > in DTS file is already specified this hw info partition and it seems
> > like that this kind of information belongs to DTS.
> 
> I haven't encountered a board, which has a different offset so far.
> This can be treated as a nicer way of defining this offset, rather
> than just hard-coding it in the source code itself.
> 
> In case there are more boards with this partition, a way of defining
> the offset in the DTS can be added later and this value can then
> be used as a default.

+Marek

My understanding is that all these definitions, like memory address
spaces, partitions, etc... belong to DTS file (or plat structures for
boards which do not use DT) and not into source code or config options
as they are describing hw layout.

There is ongoing process to move / convert SPI partitions from source
files and config defines into DTS files, so for me this looks like a
step backward...

But I would like to hear opinion also from other people.

> >
> > Otherwise, patch looks good now.
> >
> > Reviewed-by: Pali Rohár <pali at kernel.org>