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

[Pali Rohár]

On Saturday 09 October 2021 12:09:28 Robert Marko wrote:
> 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.

Something like this? compatible = "marvell,hw-info"

> 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