Date:   Mon, 13 May 2019 03:38:32 +0000
From:   Andy Duan <fugang.duan@....com>
To:     Maxime Ripard <maxime.ripard@...tlin.com>,
        Petr Štetiar <ynezz@...e.cz>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "john@...ozen.org" <john@...ozen.org>,
        "bgolaszewski@...libre.com" <bgolaszewski@...libre.com>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Mark Rutland <mark.rutland@....com>,
        Alban Bedel <albeu@...e.fr>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [EXT] Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to
 swap macaddr bytes order

From: Maxime Ripard <maxime.ripard@...tlin.com> Sent: Friday, May 10, 2019 7:32 PM
> On Fri, May 10, 2019 at 01:28:22PM +0200, Petr Štetiar wrote:
> > Andy Duan <fugang.duan@....com> [2019-05-10 08:23:58]:
> >
> > Hi Andy,
> >
> > you've probably forget to Cc some maintainers and mailing lists, so
> > I'm adding them now to the Cc loop. This patch series should be posted
> > against net-next tree as per netdev FAQ[0], but you've to wait little
> > bit as net-next is currently closed for the new submissions and you
> > would need to resend it anyway.
> >
> > 0. https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html
> >
> > > ethernet controller driver call .of_get_mac_address() to get the mac
> > > address from devictree tree, if these properties are not present,
> > > then try to read from nvmem. i.MX6x/7D/8MQ/8MM platforms ethernet
> > > MAC address read from nvmem ocotp eFuses, but it requires to swap
> > > the six bytes order.
> >
> > Thanks for bringing up this topic, as I would like to extend the
> > functionality as well, but I'm still unsure how to tackle this and
> > where, so I'll (ab)use this opportunity to bring other use cases I
> > would like to cover in the future, so we could better understand the needs.
> >
> > This reverse byte order format/layout is one of a few other storage
> > formats currently used by vendors, some other (creative) vendors are
> > currently providing MAC addresses in NVMEMs as ASCII text in following
> > two formats (hexdump -C /dev/mtdX):
> >
> >  a) 0090FEC9CBE5 - MAC address stored as ASCII without colon between
> > octets
> >
> >   00000090  57 2e 4c 41 4e 2e 4d 41  43 2e 41 64 64 72 65 73
> |W.LAN.MAC.Addres|
> >   000000a0  73 3d 30 30 39 30 46 45  43 39 43 42 45 35 00 48
> |s=0090FEC9CBE5.H|
> >   000000b0  57 2e 4c 41 4e 2e 32 47  2e 30 2e 4d 41 43 2e 41
> > |W.LAN.2G.0.MAC.A|
> >
> >   (From
> >
> https://github.com/openwrt/openwrt/pull/1448#issuecomment-442476695)
> >
> >  b) D4:EE:07:33:6C:20 - MAC address stored as ASCII with colon between
> > octets
> >
> >   00000180  66 61 63 5f 6d 61 63 20  3d 20 44 34 3a 45 45 3a
> |fac_mac = D4:EE:|
> >   00000190  30 37 3a 33 33 3a 36 43  3a 32 30 0a 42 44 49 4e
> > |07:33:6C:20.BDIN|
> >
> >   (From
> >
> https://github.com/openwrt/openwrt/pull/1906#issuecomment-483881911)
> >
> > > The patch set is to add property "nvmem_macaddr_swap" to swap
> > > macaddr bytes order.
> >
> > so it would allow following DT construct (simplified):
> >
> >  &eth0 {
> > 	nvmem-cells = <&eth0_addr>;
> > 	nvmem-cell-names = "mac-address";
> > 	nvmem_macaddr_swap;
> >  };
> >
> > I'm not sure about the `nvmem_macaddr_swap` property name, as
> > currently there are no other properties with underscores, so it should
> > be probably named as `nvmem-macaddr-swap`. DT specs permits use of the
> > underscores, but the estabilished convention is probably prefered.
> >
> > In order to cover all above mentioned use cases, it would make more
> > sense to add a description of the MAC address layout to the DT and use
> > this information to properly postprocess the NVMEM content into usable
> > MAC address?
> >
> > Something like this?
> >
> >  - nvmem-cells: phandle, reference to an nvmem node for the MAC
> > address
> >  - nvmem-cell-names: string, should be "mac-address" if nvmem is to be
> > used
> >  - nvmem-mac-address-layout: string, specifies MAC address storage
> layout.
> >    Supported values are: "binary", "binary-swapped", "ascii",
> "ascii-delimited".
> >    "binary" is the default.
> >
> > Or perhaps something like this?
> >
> >  - nvmem-cells: phandle, reference to an nvmem node for the MAC
> > address
> >  - nvmem-cell-names: string, should be any of the supported values.
> >    Supported values are: "mac-address", "mac-address-swapped",
> >    "mac-address-ascii", "mac-address-ascii-delimited".
> >
> > I'm more inclined towards the first proposed solution, as I would like
> > to propose MAC address octet incrementation feature in the future, so
> > it would
> > become:
> >
> >  - nvmem-cells: phandle, reference to an nvmem node for the MAC
> > address
> >  - nvmem-cell-names: string, should be "mac-address" if nvmem is to be
> > used
> >  - nvmem-mac-address-layout: string, specifies MAC address storage
> layout.
> >    Supported values are: "binary", "binary-swapped", "ascii",
> "ascii-delimited".
> >    "binary" is the default.
> >  - nvmem-mac-address-increment: number, value by which should be
> >    incremented MAC address octet, could be negative value as well.
> >  - nvmem-mac-address-increment-octet: number, valid values 0-5, default
> is 5.
> >    Specifies MAC address octet used for
> `nvmem-mac-address-increment`.
> >
> > What do you think?
> 
> It looks to me that it should be abstracted away by the nvmem interface and
> done at the provider level, not the customer.
> 
> Maxime
> 
If to implement add above features like Petr Štetiar described, it should be abstracted
In nvmem core driver.

> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Powered by blists - more mailing lists