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

[Robert Marko]

On Sat, Oct 9, 2021 at 11:42 AM Marek Behún <kabel at kernel.org> wrote:
>
> > > > > +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.
>
> This should be done in device tree. Device tree has bindings for such
> things. You should be able to do something like this:
>
> spi-flash at 1 {
>         partitions {
>                 compatible = "fixed-partitions";
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>
>                 board_info: board-info at 3E0000 {
>                         compatible = "nvmem-cells";
>                         label = "board-info";
>                         reg = <0x3E0000 0x1000>;
>                         read-only;
>                         #address-cells = <1>;
>                         #size-cells = <1>;
>
>                         mac_addr_eth0: mac-addr1 at 0 {
>                                 reg = <0x0 0x6>;
>                         };
>
>                         mac_addr_eth1: mac-addr2 at 1 {
>                                 reg = <0x6 0x6>;
>                         };
>                 };
>         };
> };
>
> ethernet at 30000 {
>         nvmem-cells = <&mac_addr_eth0>;
>         nvmem-cell-names = "mac-address";
> };

The thing is that the position of each value is not fixed, they can be
in whatever order or position inside of the partition and they are not
TLV
as there is no length argument stored.
They are really similar to the traditional U-boot env so you cant use
generic NVMEM cells and parse by position and length.
But I am sure that we can add a compatible for the hw_info parser and
then it can parse the reg property directly instead of using KConfig
for that.

I am sure Luka knows more about the format than me.

Regards,
Robert

>
> Look at
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mtd/partitions/nvmem-cells.yaml
>
> Also we need to add nvmem API into U-Boot and get rid of the ad-hoc
> efuse/mac/hw_mac nonsense.

This would be great for a lot of devices.
>
> Marek



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko at sartura.hr
Web: www.sartura.hr