NVMEM address DT post processing [Was: Re: [PATCH net 0/3] addproperty "nvmem_macaddr_swap" to swap macaddr bytes order]
diff mbox series

Message ID 20190511144444.GU81826@meh.true.cz
State Changes Requested
Headers show
Series
  • NVMEM address DT post processing [Was: Re: [PATCH net 0/3] addproperty "nvmem_macaddr_swap" to swap macaddr bytes order]
Related show

Checks

Context Check Description
robh/checkpatch warning "total: 1 errors, 3 warnings, 70 lines checked"

Commit Message

Petr Štetiar May 11, 2019, 2:44 p.m. UTC
Maxime Ripard <maxime.ripard@bootlin.com> [2019-05-10 13:31:55]:

Hi,

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

So something like this?

Comments

Maxime Ripard May 12, 2019, 12:19 p.m. UTC | #1
On Sat, May 11, 2019 at 04:44:44PM +0200, Petr Štetiar wrote:
> So something like this?
>
> diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> index fd06c09b822b..d781e47b049d 100644
> --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt
> +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> @@ -1,12 +1,14 @@
>  = NVMEM(Non Volatile Memory) Data Device Tree Bindings =
>
>  This binding is intended to represent the location of hardware
> -configuration data stored in NVMEMs like eeprom, efuses and so on.
> +configuration data stored in NVMEMs like eeprom, efuses and so on. This
> +binding provides some basic transformation of the stored data as well.
>
>  On a significant proportion of boards, the manufacturer has stored
>  some data on NVMEM, for the OS to be able to retrieve these information
>  and act upon it. Obviously, the OS has to know about where to retrieve
> -these data from, and where they are stored on the storage device.
> +these data from, where they are stored on the storage device and how to
> +postprocess them.
>
>  This document is here to document this.
>
> @@ -29,6 +31,19 @@ Optional properties:
>  bits:  Is pair of bit location and number of bits, which specifies offset
>         in bit and number of bits within the address range specified by reg property.
>         Offset takes values from 0-7.
> +byte-indices: array, encoded as an arbitrary number of (offset, length) pairs,
> +            within the address range specified by reg property. Each pair is
> +            then processed with byte-transform in order to produce single u8
> +            sized byte.
> +byte-transform: string, specifies the transformation which should be applied
> +              to every byte-indices pair in order to produce usable u8 sized byte,
> +              possible values are "none", "ascii" and "bcd". Default is "none".
> +byte-adjust: number, value by which should be adjusted resulting output byte at
> +           byte-adjust-at offset.
> +byte-adjust-at: number, specifies offset of resulting output byte which should be
> +              adjusted by byte-adjust value, default is 0.
> +byte-result-swap: boolean, specifies if the resulting output bytes should be
> +                swapped prior to return
>
>  For example:
>
> @@ -59,6 +74,36 @@ For example:
>                 ...
>         };
>
> +Another example where we've MAC address for eth1 stored in the NOR EEPROM as
> +following sequence of bytes (output of hexdump -C /dev/mtdX):
> +
> + 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|
> +
> +Which means, that MAC address is stored in EEPROM as D4:EE:07:33:6C:20, so
> +ASCII delimited by colons, but we can't use this MAC address directly as
> +there's only one MAC address stored in the EEPROM and we need to increment last
> +octet/byte in this address in order to get usable MAC address for eth1 device.
> +
> + eth1_addr: eth-mac-addr@18a {
> +     reg = <0x18a 0x11>;
> +     byte-indices = < 0 2
> +                      3 2
> +                      6 2
> +                      9 2
> +                     12 2
> +                     15 2>;
> +     byte-transform = "ascii";
> +     byte-increment = <1>;
> +     byte-increment-at = <5>;
> +     byte-result-swap;
> + };
> +
> + &eth1 {
> +     nvmem-cells = <&eth1_addr>;
> +     nvmem-cell-names = "mac-address";
> + };
> +

Something along those lines yes. I'm not sure why in your example the
cell doesn't start at the mac address itself, instead of starting at
the key + having to specify an offset though. The reg property is the
offset already.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Srinivas Kandagatla May 13, 2019, 8:25 a.m. UTC | #2
On 11/05/2019 15:44, Petr Štetiar wrote:
>          };
>   
> +Another example where we've MAC address for eth1 stored in the NOR EEPROM as
> +following sequence of bytes (output of hexdump -C /dev/mtdX):
> +
> + 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|
> +
> +Which means, that MAC address is stored in EEPROM as D4:EE:07:33:6C:20, so
> +ASCII delimited by colons, but we can't use this MAC address directly as
> +there's only one MAC address stored in the EEPROM and we need to increment last
> +octet/byte in this address in order to get usable MAC address for eth1 device.
> +
> + eth1_addr: eth-mac-addr@18a {
> +     reg = <0x18a 0x11>;
> +     byte-indices = < 0 2
> +                      3 2
> +                      6 2
> +                      9 2
> +                     12 2
> +                     15 2>;
> +     byte-transform = "ascii";
> +     byte-increment = <1>;
> +     byte-increment-at = <5>;
> +     byte-result-swap;


> + };
> +
> + &eth1 {
> +     nvmem-cells = <&eth1_addr>;
> +     nvmem-cell-names = "mac-address";
> + };
> +
>   = Data consumers =
>   Are device nodes which consume nvmem data cells/providers.

TBH, I have not see the full thread as I don't seem to be added to the 
original thread for some reason!

Looking at the comments from last few emails, I think the overall idea 
of moving the transformations in to nvmem core looks fine with me. I 
remember we discuss this while nvmem was first added and the plan was to 
add some kinda nvmem plugin/transformation interface at cell/provider 
level. And this looks like correct time to introduce this.

My initial idea was to add compatible strings to the cell so that most 
of the encoding information can be derived from it. For example if the 
encoding representing in your example is pretty standard or vendor 
specific we could just do with a simple compatible like below:

eth1_addr: eth-mac-addr@18a {
	compatible = "xxx,nvmem-mac-address";
	reg = <0x18a 0x11>;	
};

&eth1 {
	nvmem-cells = <&eth1_addr>;
	nvmem-cell-names = "mac-address";
}

thanks,
srini
Petr Štetiar May 13, 2019, 9:07 a.m. UTC | #3
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 09:25:55]:

Hi,

> My initial idea was to add compatible strings to the cell so that most of
> the encoding information can be derived from it. For example if the encoding
> representing in your example is pretty standard or vendor specific we could
> just do with a simple compatible like below:

that vendor/compatible list would be quite long[1], there are hundreds of
devices in current OpenWrt tree (using currently custom patch) and probably
dozens currently unsupported (ASCII encoded MAC address in NVMEM). So my goal
is to add some DT functionality which would cover all of these.

> eth1_addr: eth-mac-addr@18a {
> 	compatible = "xxx,nvmem-mac-address";
> 	reg = <0x18a 0x11>;	
> };

while sketching the possible DT use cases I came to the this option as well, it
was very compeling as it would kill two birds with one stone (fix outstanding
MTD/NVMEM OF clash as well[2]), but I think, that it makes more sense to add
this functionality to nvmem core so it could be reused by other consumers, not
just by network layer.

1. https://git.openwrt.org/?p=openwrt%2Fopenwrt.git&a=search&h=HEAD&st=grep&s=mtd-mac-address
2. https://lore.kernel.org/netdev/20190418133646.GA94236@meh.true.cz

-- ynezz
Petr Štetiar May 13, 2019, 9:28 a.m. UTC | #4
Maxime Ripard <maxime.ripard@bootlin.com> [2019-05-12 14:19:10]:

> > @@ -29,6 +31,19 @@ Optional properties:
> >  bits:  Is pair of bit location and number of bits, which specifies offset
> >         in bit and number of bits within the address range specified by reg property.
> >         Offset takes values from 0-7.
> > +byte-indices: array, encoded as an arbitrary number of (offset, length) pairs,
> > +            within the address range specified by reg property. Each pair is
> > +            then processed with byte-transform in order to produce single u8
> > +            sized byte.
> > +byte-transform: string, specifies the transformation which should be applied
> > +              to every byte-indices pair in order to produce usable u8 sized byte,
> > +              possible values are "none", "ascii" and "bcd". Default is "none".
> > +byte-adjust: number, value by which should be adjusted resulting output byte at
> > +           byte-adjust-at offset.
> > +byte-adjust-at: number, specifies offset of resulting output byte which should be
> > +              adjusted by byte-adjust value, default is 0.
> > +byte-result-swap: boolean, specifies if the resulting output bytes should be
> > +                swapped prior to return
> >
> >  For example:
> >
> > @@ -59,6 +74,36 @@ For example:
> >                 ...
> >         };
> >
> > +Another example where we've MAC address for eth1 stored in the NOR EEPROM as
> > +following sequence of bytes (output of hexdump -C /dev/mtdX):
> > +
> > + 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|
> > +
> > +Which means, that MAC address is stored in EEPROM as D4:EE:07:33:6C:20, so
> > +ASCII delimited by colons, but we can't use this MAC address directly as
> > +there's only one MAC address stored in the EEPROM and we need to increment last
> > +octet/byte in this address in order to get usable MAC address for eth1 device.
> > +
> > + eth1_addr: eth-mac-addr@18a {
> > +     reg = <0x18a 0x11>;
> > +     byte-indices = < 0 2
> > +                      3 2
> > +                      6 2
> > +                      9 2
> > +                     12 2
> > +                     15 2>;
> > +     byte-transform = "ascii";
> > +     byte-increment = <1>;
> > +     byte-increment-at = <5>;
> > +     byte-result-swap;
> > + };
> > +
> > + &eth1 {
> > +     nvmem-cells = <&eth1_addr>;
> > +     nvmem-cell-names = "mac-address";
> > + };
> > +
> 
> Something along those lines yes. I'm not sure why in your example the
> cell doesn't start at the mac address itself, instead of starting at
> the key + having to specify an offset though. The reg property is the
> offset already.

The cell starts at the MAC address itself, 0x180 is offset within the EEPROM
and 0xa is byte within the offset (off-by-one, correct should be 0x9 though).

  EEPROM                 byte within EEPROM offset
  offset    1  2  3  4  5  5  6  7   8  9  a  b  c  d  e  f
 ------------------------------------------------------------|-----------------
 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|

So this would produce following:

 eth1_addr: eth-mac-addr@189 {
    reg = <0x189 0x11>;         /* 0x44 0x34 0x3a 0x45 0x45 0x3a 0x30 0x37
                                 * 0x3a 0x33 0x33 0x3a 0x36 0x43 0x3a 0x32 0x30 */
    byte-indices = < 0 2        /* 0x44 0x34 */
                     3 2        /* 0x45 0x45 */
                     6 2        /* 0x30 0x37 */
                     9 2        /* 0x33 0x33 */
                    12 2        /* 0x36 0x43 */
                    15 2>;      /* 0x32 0x30 */
    byte-transform = "ascii";   /* 0xd4 0xee 0x7 0x33 0x6c 0x20 */
    byte-increment = <1>;
    byte-increment-at = <5>;    /* 0xd4 0xee 0x7 0x33 0x6c 0x21 */
    byte-result-swap;           /* 0x21 0x6c 0x33 0x7 0xee 0xd4 */
 };

-- ynezz
Srinivas Kandagatla May 13, 2019, 10:06 a.m. UTC | #5
On 13/05/2019 10:07, Petr Štetiar wrote:
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 09:25:55]:
> 
> Hi,
> 
>> My initial idea was to add compatible strings to the cell so that most of
>> the encoding information can be derived from it. For example if the encoding
>> representing in your example is pretty standard or vendor specific we could
>> just do with a simple compatible like below:
> 
> that vendor/compatible list would be quite long[1], there are hundreds of

You are right just vendor list could be very long, but I was hoping that 
the post-processing would fall in some categories which can be used in 
compatible string.

Irrespective of which we need to have some sort of compatible string to 
enable nvmem core to know that there is some form of post processing to 
be done on the cells!. Without which there is a danger of continuing to 
adding new properties to the cell bindings which have no relation to 
each other.


> devices in current OpenWrt tree (using currently custom patch) and probably
> dozens currently unsupported (ASCII encoded MAC address in NVMEM). So my goal
> is to add some DT functionality which would cover all of these.

> 
>> eth1_addr: eth-mac-addr@18a {
>> 	compatible = "xxx,nvmem-mac-address";
>> 	reg = <0x18a 0x11>;	
>> };
> 
> while sketching the possible DT use cases I came to the this option as well, it
> was very compeling as it would kill two birds with one stone (fix outstanding
> MTD/NVMEM OF clash as well[2]), but I think, that it makes more sense to add
> this functionality to nvmem core so it could be reused by other consumers, not
> just by network layer.

Changes to nvmem dt bindings have been already rejected, for this more 
discussion at: https://lore.kernel.org/patchwork/patch/936312/

--srini

> 
> 1. https://git.openwrt.org/?p=openwrt%2Fopenwrt.git&a=search&h=HEAD&st=grep&s=mtd-mac-address
> 2. https://lore.kernel.org/netdev/20190418133646.GA94236@meh.true.cz
> 
> -- ynezz
>
Petr Štetiar May 13, 2019, 11:16 a.m. UTC | #6
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 11:06:48]:

> On 13/05/2019 10:07, Petr Štetiar wrote:
> > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 09:25:55]:
> > 
> > > My initial idea was to add compatible strings to the cell so that most of
> > > the encoding information can be derived from it. For example if the encoding
> > > representing in your example is pretty standard or vendor specific we could
> > > just do with a simple compatible like below:
> > 
> > that vendor/compatible list would be quite long[1], there are hundreds of
> 
> You are right just vendor list could be very long, but I was hoping that the
> post-processing would fall in some categories which can be used in
> compatible string.
> 
> Irrespective of which we need to have some sort of compatible string to
> enable nvmem core to know that there is some form of post processing to be
> done on the cells!. Without which there is a danger of continuing to adding
> new properties to the cell bindings which have no relation to each other.

makes sense, so something like this would be acceptable?

 eth1_addr: eth-mac-addr@18a {
     /* or rather linux,nvmem-post-process ? */
     compatible = "openwrt,nvmem-post-process";
     reg = <0x189 0x11>;
     indices = < 0 2
                 3 2
                 6 2
                 9 2
                12 2
                15 2>;
     transform = "ascii";
     increment = <1>;
     increment-at = <5>;
     result-swap;
 };

 &eth1 {
     nvmem-cells = <&eth1_addr>;
     nvmem-cell-names = "mac-address";
 };

> > was very compeling as it would kill two birds with one stone (fix outstanding
> > MTD/NVMEM OF clash as well[2]),
> 
> Changes to nvmem dt bindings have been already rejected, for this more
> discussion at: https://lore.kernel.org/patchwork/patch/936312/

I know, I've re-read this thread several times, but it's still unclear to me,
how this should be approached in order to be accepted by you and by DT
maintainers as well.

Anyway, to sum it up, from your point of view, following is fine?

 flash@0 {
    partitions {
        art: partition@ff0000 {
            nvmem_dev: nvmem-cells {
                compatible = "nvmem-cells";
                eth1_addr: eth-mac-addr@189 {
                    compatible = "openwrt,nvmem-post-process";
                    reg = <0x189 0x6>;
                    increment = <1>;
                    increment-at = <5>;
                    result-swap;
                };
            };
        };
    };
 };

 &eth1 {
    nvmem = <&nvmem_dev>;
    nvmem-cells = <&eth1_addr>;
    nvmem-cell-names = "mac-address";
 };

-- ynezz
Srinivas Kandagatla May 14, 2019, 3:13 p.m. UTC | #7
On 13/05/2019 12:16, Petr Štetiar wrote:
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 11:06:48]:
> 
>> On 13/05/2019 10:07, Petr Štetiar wrote:
>>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 09:25:55]:
>>>
>>>> My initial idea was to add compatible strings to the cell so that most of
>>>> the encoding information can be derived from it. For example if the encoding
>>>> representing in your example is pretty standard or vendor specific we could
>>>> just do with a simple compatible like below:
>>>
>>> that vendor/compatible list would be quite long[1], there are hundreds of
>>
>> You are right just vendor list could be very long, but I was hoping that the
>> post-processing would fall in some categories which can be used in
>> compatible string.
>>
>> Irrespective of which we need to have some sort of compatible string to
>> enable nvmem core to know that there is some form of post processing to be
>> done on the cells!. Without which there is a danger of continuing to adding
>> new properties to the cell bindings which have no relation to each other.
> 
> makes sense, so something like this would be acceptable?
> 
>   eth1_addr: eth-mac-addr@18a {
>       /* or rather linux,nvmem-post-process ? */
>       compatible = "openwrt,nvmem-post-process";

I don't think this would be a correct compatible string to use here.
Before we decide on naming, I would like to understand bit more on what 
are the other possible forms of storing mac address,
Here is what I found,

Type 1: Octets in ASCII without delimiters. (Swapped/non-Swapped)
Type 2: Octets in ASCII with delimiters like (":", ",", ".", "-"... so 
on) (Swapped/non-Swapped)
Type 3: Is the one which stores mac address in Type1/2 but this has to 
be incremented to be used on other instances of eth.

Did I miss anything?
My suggestion for type1 and type2 would be something like this, as long 
as its okay with DT maintainers

eth1_addr: eth-mac-addr@18a {
	compatible = "ascii-mac-address";
	reg = <0x18a 2>, <0x192 2>, <0x196 2>, <0x200 2>, <0x304 2>, <0x306 2>;
	swap-mac-address;
	delimiter = ":";
};

For type 3:

This sounds like very much vendor specific optimization thing which am 
not 100% sure atm.
If dt maintainers are okay, may be we can add an increment in the 
"ascii-mac-address" binding itself.

Do you think "increment-at " would ever change?

>       reg = <0x189 0x11>;
>       indices = < 0 2
>                   3 2
>                   6 2
>                   9 2
>                  12 2
>                  15 2>;
>       transform = "ascii";
>       increment = <1>;
>       increment-at = <5>;
>       result-swap;

>   };
> 
>   &eth1 {
>       nvmem-cells = <&eth1_addr>;
>       nvmem-cell-names = "mac-address";
>   };
> 
>>> was very compeling as it would kill two birds with one stone (fix outstanding
>>> MTD/NVMEM OF clash as well[2]),
>>
>> Changes to nvmem dt bindings have been already rejected, for this more
>> discussion at: https://lore.kernel.org/patchwork/patch/936312/
> 
> I know, I've re-read this thread several times, but it's still unclear to me,
> how this should be approached in order to be accepted by you and by DT
> maintainers as well.
> 
> Anyway, to sum it up, from your point of view, following is fine?
> 
currently mtd already has support for nvmem but without dt support.

>   flash@0 {
>      partitions {
>          art: partition@ff0000 {
>              nvmem_dev: nvmem-cells {
>                  compatible = "nvmem-cells";
>                  eth1_addr: eth-mac-addr@189 {
>                      compatible = "openwrt,nvmem-post-process";
>                      reg = <0x189 0x6>;
>                      increment = <1>;
>                      increment-at = <5>;
>                      result-swap;
>                  };
>              };
>          };
>      };
>   };

This [1] is what I had suggested at the end, where in its possible to 
add provider node with its own custom bindings. In above example 
nvmem_dev would be a proper nvmem provider.

Thanks,
srini
[1] https://lkml.org/lkml/2018/6/7/972
> 
>   &eth1 {
>      nvmem = <&nvmem_dev>;
>      nvmem-cells = <&eth1_addr>;
>      nvmem-cell-names = "mac-address";
>   };
> 
> -- ynezz
>
Petr Štetiar May 14, 2019, 5:44 p.m. UTC | #8
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-14 16:13:22]:

> On 13/05/2019 12:16, Petr Štetiar wrote:
> > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 11:06:48]:
> > 
> > > On 13/05/2019 10:07, Petr Štetiar wrote:
> > > > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 09:25:55]:
> > > > 
> > > > > My initial idea was to add compatible strings to the cell so that most of
> > > > > the encoding information can be derived from it. For example if the encoding
> > > > > representing in your example is pretty standard or vendor specific we could
> > > > > just do with a simple compatible like below:
> > > > 
> > > > that vendor/compatible list would be quite long[1], there are hundreds of
> > > 
> > > You are right just vendor list could be very long, but I was hoping that the
> > > post-processing would fall in some categories which can be used in
> > > compatible string.
> > > 
> > > Irrespective of which we need to have some sort of compatible string to
> > > enable nvmem core to know that there is some form of post processing to be
> > > done on the cells!. Without which there is a danger of continuing to adding
> > > new properties to the cell bindings which have no relation to each other.
> > 
> > makes sense, so something like this would be acceptable?
> > 
> >   eth1_addr: eth-mac-addr@18a {
> >       /* or rather linux,nvmem-post-process ? */
> >       compatible = "openwrt,nvmem-post-process";
> 
> I don't think this would be a correct compatible string to use here.
> Before we decide on naming, I would like to understand bit more on what are
> the other possible forms of storing mac address,
> Here is what I found,
> 
> Type 1: Octets in ASCII without delimiters. (Swapped/non-Swapped)
> Type 2: Octets in ASCII with delimiters like (":", ",", ".", "-"... so on)
> (Swapped/non-Swapped)
> Type 3: Is the one which stores mac address in Type1/2 but this has to be
> incremented to be used on other instances of eth.
> 
> Did I miss anything?

Type 4: Octets as bytes/u8, swapped/non-swapped

Currently just type4-non-swapped is supported. Support for type4-swapped was
goal of this patch series.

I've simply tried to avoid using mac-address for the compatible as this
provider could be reused by other potential nvmem consumers. The question is,
how much abstracted it should be then.

> My suggestion for type1 and type2 would be something like this, as long as
> its okay with DT maintainers
> 
> eth1_addr: eth-mac-addr@18a {
> 	compatible = "ascii-mac-address";
> 	reg = <0x18a 2>, <0x192 2>, <0x196 2>, <0x200 2>, <0x304 2>, <0x306 2>;
> 	swap-mac-address;
> 	delimiter = ":";
> };

with this reg array, you don't need the delimiter property anymore, do you?

> For type 3:
>
> This sounds like very much vendor specific optimization thing which am not
> 100% sure atm.  If dt maintainers are okay, may be we can add an increment
> in the "ascii-mac-address" binding itself.
> 
> Do you think "increment-at " would ever change?

Currently there's just one such real world use case in OpenWrt tree[1].
Probably some vendor decided to increment 4th octet.

> This [1] is what I had suggested at the end, where in its possible to add
> provider node with its own custom bindings. In above example nvmem_dev would
> be a proper nvmem provider.

Ok, thanks.

1. https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/ath79/dts/ar9331_embeddedwireless_dorin.dts;h=43bec35fa2860fe4d52880ad24ff7c56f5060a0a;hb=HEAD#l109

-- ynezz
Srinivas Kandagatla May 15, 2019, 5:12 p.m. UTC | #9
On 14/05/2019 18:44, Petr Štetiar wrote:
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-14 16:13:22]:
> 
>> On 13/05/2019 12:16, Petr Štetiar wrote:
>>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 11:06:48]:
>>>
>>>> On 13/05/2019 10:07, Petr Štetiar wrote:
>>>>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 09:25:55]:
>>>>>
>>>>>> My initial idea was to add compatible strings to the cell so that most of
>>>>>> the encoding information can be derived from it. For example if the encoding
>>>>>> representing in your example is pretty standard or vendor specific we could
>>>>>> just do with a simple compatible like below:
>>>>>
>>>>> that vendor/compatible list would be quite long[1], there are hundreds of
>>>>
>>>> You are right just vendor list could be very long, but I was hoping that the
>>>> post-processing would fall in some categories which can be used in
>>>> compatible string.
>>>>
>>>> Irrespective of which we need to have some sort of compatible string to
>>>> enable nvmem core to know that there is some form of post processing to be
>>>> done on the cells!. Without which there is a danger of continuing to adding
>>>> new properties to the cell bindings which have no relation to each other.
>>>
>>> makes sense, so something like this would be acceptable?
>>>
>>>    eth1_addr: eth-mac-addr@18a {
>>>        /* or rather linux,nvmem-post-process ? */
>>>        compatible = "openwrt,nvmem-post-process";
>>
>> I don't think this would be a correct compatible string to use here.
>> Before we decide on naming, I would like to understand bit more on what are
>> the other possible forms of storing mac address,
>> Here is what I found,
>>
>> Type 1: Octets in ASCII without delimiters. (Swapped/non-Swapped)
>> Type 2: Octets in ASCII with delimiters like (":", ",", ".", "-"... so on)
>> (Swapped/non-Swapped)
>> Type 3: Is the one which stores mac address in Type1/2 but this has to be
>> incremented to be used on other instances of eth.
>>
>> Did I miss anything?
> 
> Type 4: Octets as bytes/u8, swapped/non-swapped
> 
> Currently just type4-non-swapped is supported. Support for type4-swapped was
> goal of this patch series.
> 

Can we just get away with swapped/non-swapped by using order of reg dt 
property?
If that works for you then we do not need a special compatible string too.

Note that current nvmem core only supports single reg value pair which 
needs to be extended to support multiple reg value.

> I've simply tried to avoid using mac-address for the compatible as this
> provider could be reused by other potential nvmem consumers. The question is,
> how much abstracted it should be then.
> 
>> My suggestion for type1 and type2 would be something like this, as long as
>> its okay with DT maintainers
>>
>> eth1_addr: eth-mac-addr@18a {
>> 	compatible = "ascii-mac-address";
>> 	reg = <0x18a 2>, <0x192 2>, <0x196 2>, <0x200 2>, <0x304 2>, <0x306 2>;
>> 	swap-mac-address;
>> 	delimiter = ":";
>> };
> 
> with this reg array, you don't need the delimiter property anymore, do you?
> 
You are right we do not need it.

>> For type 3:
>>
>> This sounds like very much vendor specific optimization thing which am not
>> 100% sure atm.  If dt maintainers are okay, may be we can add an increment
>> in the "ascii-mac-address" binding itself.
>>
>> Do you think "increment-at " would ever change?
> 
> Currently there's just one such real world use case in OpenWrt tree[1].

If that is the case then we definitely need bindings prefixed with 
vendor name or something on those lines for this.


> Probably some vendor decided to increment 4th octet.
Incrementing 4th octet does not really make sense!

Thanks,
srini
Rob Herring May 20, 2019, 2:28 p.m. UTC | #10
On Mon, May 13, 2019 at 6:16 AM Petr Štetiar <ynezz@true.cz> wrote:
>
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 11:06:48]:
>
> > On 13/05/2019 10:07, Petr Štetiar wrote:
> > > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 09:25:55]:
> > >
> > > > My initial idea was to add compatible strings to the cell so that most of
> > > > the encoding information can be derived from it. For example if the encoding
> > > > representing in your example is pretty standard or vendor specific we could
> > > > just do with a simple compatible like below:
> > >
> > > that vendor/compatible list would be quite long[1], there are hundreds of
> >
> > You are right just vendor list could be very long, but I was hoping that the
> > post-processing would fall in some categories which can be used in
> > compatible string.
> >
> > Irrespective of which we need to have some sort of compatible string to
> > enable nvmem core to know that there is some form of post processing to be
> > done on the cells!. Without which there is a danger of continuing to adding
> > new properties to the cell bindings which have no relation to each other.
>
> makes sense, so something like this would be acceptable?

No. Don't try to put a data processing language into DT.

>
>  eth1_addr: eth-mac-addr@18a {
>      /* or rather linux,nvmem-post-process ? */
>      compatible = "openwrt,nvmem-post-process";
>      reg = <0x189 0x11>;
>      indices = < 0 2
>                  3 2
>                  6 2
>                  9 2
>                 12 2
>                 15 2>;
>      transform = "ascii";
>      increment = <1>;
>      increment-at = <5>;
>      result-swap;
>  };
>
>  &eth1 {
>      nvmem-cells = <&eth1_addr>;
>      nvmem-cell-names = "mac-address";
>  };
>
> > > was very compeling as it would kill two birds with one stone (fix outstanding
> > > MTD/NVMEM OF clash as well[2]),
> >
> > Changes to nvmem dt bindings have been already rejected, for this more
> > discussion at: https://lore.kernel.org/patchwork/patch/936312/
>
> I know, I've re-read this thread several times, but it's still unclear to me,
> how this should be approached in order to be accepted by you and by DT
> maintainers as well.
>
> Anyway, to sum it up, from your point of view, following is fine?
>
>  flash@0 {
>     partitions {
>         art: partition@ff0000 {
>             nvmem_dev: nvmem-cells {
>                 compatible = "nvmem-cells";

My suggestion would be to add a specific compatible here and that can
provide a driver or hooks to read and transform the data.

Rob

>                 eth1_addr: eth-mac-addr@189 {
>                     compatible = "openwrt,nvmem-post-process";
>                     reg = <0x189 0x6>;
>                     increment = <1>;
>                     increment-at = <5>;
>                     result-swap;
>                 };
>             };
>         };
>     };
>  };
>
>  &eth1 {
>     nvmem = <&nvmem_dev>;
>     nvmem-cells = <&eth1_addr>;
>     nvmem-cell-names = "mac-address";
>  };
>
> -- ynezz

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt
index fd06c09b822b..d781e47b049d 100644
--- a/Documentation/devicetree/bindings/nvmem/nvmem.txt
+++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
@@ -1,12 +1,14 @@ 
 = NVMEM(Non Volatile Memory) Data Device Tree Bindings =
 
 This binding is intended to represent the location of hardware
-configuration data stored in NVMEMs like eeprom, efuses and so on.
+configuration data stored in NVMEMs like eeprom, efuses and so on. This
+binding provides some basic transformation of the stored data as well.
 
 On a significant proportion of boards, the manufacturer has stored
 some data on NVMEM, for the OS to be able to retrieve these information
 and act upon it. Obviously, the OS has to know about where to retrieve
-these data from, and where they are stored on the storage device.
+these data from, where they are stored on the storage device and how to
+postprocess them.
 
 This document is here to document this.
 
@@ -29,6 +31,19 @@  Optional properties:
 bits:  Is pair of bit location and number of bits, which specifies offset
        in bit and number of bits within the address range specified by reg property.
        Offset takes values from 0-7.
+byte-indices: array, encoded as an arbitrary number of (offset, length) pairs,
+            within the address range specified by reg property. Each pair is
+            then processed with byte-transform in order to produce single u8
+            sized byte.
+byte-transform: string, specifies the transformation which should be applied
+              to every byte-indices pair in order to produce usable u8 sized byte,
+              possible values are "none", "ascii" and "bcd". Default is "none".
+byte-adjust: number, value by which should be adjusted resulting output byte at
+           byte-adjust-at offset.
+byte-adjust-at: number, specifies offset of resulting output byte which should be
+              adjusted by byte-adjust value, default is 0.
+byte-result-swap: boolean, specifies if the resulting output bytes should be
+                swapped prior to return
 
 For example:
 
@@ -59,6 +74,36 @@  For example:
                ...
        };
 
+Another example where we've MAC address for eth1 stored in the NOR EEPROM as
+following sequence of bytes (output of hexdump -C /dev/mtdX):
+
+ 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|
+
+Which means, that MAC address is stored in EEPROM as D4:EE:07:33:6C:20, so
+ASCII delimited by colons, but we can't use this MAC address directly as
+there's only one MAC address stored in the EEPROM and we need to increment last
+octet/byte in this address in order to get usable MAC address for eth1 device.
+
+ eth1_addr: eth-mac-addr@18a {
+     reg = <0x18a 0x11>;
+     byte-indices = < 0 2
+                      3 2
+                      6 2
+                      9 2
+                     12 2
+                     15 2>;
+     byte-transform = "ascii";
+     byte-increment = <1>;
+     byte-increment-at = <5>;
+     byte-result-swap;
+ };
+
+ &eth1 {
+     nvmem-cells = <&eth1_addr>;
+     nvmem-cell-names = "mac-address";
+ };
+
 = Data consumers =
 Are device nodes which consume nvmem data cells/providers.