* Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order
@ 2019-05-10 11:31 ` Maxime Ripard
0 siblings, 0 replies; 28+ messages in thread
From: Maxime Ripard @ 2019-05-10 11:31 UTC (permalink / raw)
To: Petr Štetiar
Cc: Andy Duan, davem, netdev, john, bgolaszewski,
Srinivas Kandagatla, Andrew Lunn, Florian Fainelli,
Heiner Kallweit, Rob Herring, Frank Rowand, Mark Rutland,
Alban Bedel, devicetree
[-- Attachment #1: Type: text/plain, Size: 4619 bytes --]
On Fri, May 10, 2019 at 01:28:22PM +0200, Petr Štetiar wrote:
> Andy Duan <fugang.duan@nxp.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):
>
> ð0 {
> nvmem-cells = <ð0_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
^ permalink raw reply [flat|nested] 28+ messages in thread* NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]
2019-05-10 11:31 ` Maxime Ripard
(?)
@ 2019-05-11 14:44 ` Petr Štetiar
2019-05-12 12:19 ` Maxime Ripard
2019-05-13 8:25 ` Srinivas Kandagatla
-1 siblings, 2 replies; 28+ messages in thread
From: Petr Štetiar @ 2019-05-11 14:44 UTC (permalink / raw)
To: Maxime Ripard
Cc: Andy Duan, davem, netdev, john, bgolaszewski,
Srinivas Kandagatla, Andrew Lunn, Florian Fainelli,
Heiner Kallweit, Rob Herring, Frank Rowand, Mark Rutland,
Alban Bedel, devicetree
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):
> >
> > ð0 {
> > nvmem-cells = <ð0_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?
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;
+ };
+
+ ð1 {
+ nvmem-cells = <ð1_addr>;
+ nvmem-cell-names = "mac-address";
+ };
+
= Data consumers =
Are device nodes which consume nvmem data cells/providers.
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]
2019-05-11 14:44 ` NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order] Petr Štetiar
@ 2019-05-12 12:19 ` Maxime Ripard
2019-05-13 8:25 ` Srinivas Kandagatla
1 sibling, 0 replies; 28+ messages in thread
From: Maxime Ripard @ 2019-05-12 12:19 UTC (permalink / raw)
To: Petr Štetiar
Cc: Andy Duan, davem, netdev, john, bgolaszewski,
Srinivas Kandagatla, Andrew Lunn, Florian Fainelli,
Heiner Kallweit, Rob Herring, Frank Rowand, Mark Rutland,
Alban Bedel, devicetree
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;
> + };
> +
> + ð1 {
> + nvmem-cells = <ð1_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
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]
@ 2019-05-12 12:19 ` Maxime Ripard
0 siblings, 0 replies; 28+ messages in thread
From: Maxime Ripard @ 2019-05-12 12:19 UTC (permalink / raw)
To: Petr Štetiar
Cc: Andy Duan, davem, netdev, john, bgolaszewski,
Srinivas Kandagatla, Andrew Lunn, Florian Fainelli,
Heiner Kallweit, Rob Herring, Frank Rowand, Mark Rutland,
Alban Bedel, devicetree
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;
> + };
> +
> + ð1 {
> + nvmem-cells = <ð1_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
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]
2019-05-12 12:19 ` Maxime Ripard
(?)
@ 2019-05-13 9:28 ` Petr Štetiar
-1 siblings, 0 replies; 28+ messages in thread
From: Petr Štetiar @ 2019-05-13 9:28 UTC (permalink / raw)
To: Maxime Ripard
Cc: Andy Duan, davem, netdev, john, bgolaszewski,
Srinivas Kandagatla, Andrew Lunn, Florian Fainelli,
Heiner Kallweit, Rob Herring, Frank Rowand, Mark Rutland,
Alban Bedel, devicetree
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;
> > + };
> > +
> > + ð1 {
> > + nvmem-cells = <ð1_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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]
2019-05-11 14:44 ` NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order] Petr Štetiar
2019-05-12 12:19 ` Maxime Ripard
@ 2019-05-13 8:25 ` Srinivas Kandagatla
2019-05-13 9:07 ` Petr Štetiar
1 sibling, 1 reply; 28+ messages in thread
From: Srinivas Kandagatla @ 2019-05-13 8:25 UTC (permalink / raw)
To: Petr Štetiar, Maxime Ripard
Cc: Andy Duan, davem, netdev, john, bgolaszewski, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, Rob Herring, Frank Rowand,
Mark Rutland, Alban Bedel, devicetree
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;
> + };
> +
> + ð1 {
> + nvmem-cells = <ð1_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>;
};
ð1 {
nvmem-cells = <ð1_addr>;
nvmem-cell-names = "mac-address";
}
thanks,
srini
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]
2019-05-13 8:25 ` Srinivas Kandagatla
@ 2019-05-13 9:07 ` Petr Štetiar
2019-05-13 10:06 ` Srinivas Kandagatla
0 siblings, 1 reply; 28+ messages in thread
From: Petr Štetiar @ 2019-05-13 9:07 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Maxime Ripard, Andy Duan, davem, netdev, john, bgolaszewski,
Andrew Lunn, Florian Fainelli, Heiner Kallweit, Rob Herring,
Frank Rowand, Mark Rutland, Alban Bedel, devicetree
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
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]
2019-05-13 9:07 ` Petr Štetiar
@ 2019-05-13 10:06 ` Srinivas Kandagatla
2019-05-13 11:16 ` Petr Štetiar
0 siblings, 1 reply; 28+ messages in thread
From: Srinivas Kandagatla @ 2019-05-13 10:06 UTC (permalink / raw)
To: Petr Štetiar
Cc: Maxime Ripard, Andy Duan, davem, netdev, john, bgolaszewski,
Andrew Lunn, Florian Fainelli, Heiner Kallweit, Rob Herring,
Frank Rowand, Mark Rutland, Alban Bedel, devicetree
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
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]
2019-05-13 10:06 ` Srinivas Kandagatla
@ 2019-05-13 11:16 ` Petr Štetiar
2019-05-14 15:13 ` Srinivas Kandagatla
2019-05-20 14:28 ` Rob Herring
0 siblings, 2 replies; 28+ messages in thread
From: Petr Štetiar @ 2019-05-13 11:16 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Maxime Ripard, Andy Duan, davem, netdev, john, bgolaszewski,
Andrew Lunn, Florian Fainelli, Heiner Kallweit, Rob Herring,
Frank Rowand, Mark Rutland, Alban Bedel, devicetree
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;
};
ð1 {
nvmem-cells = <ð1_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;
};
};
};
};
};
ð1 {
nvmem = <&nvmem_dev>;
nvmem-cells = <ð1_addr>;
nvmem-cell-names = "mac-address";
};
-- ynezz
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]
2019-05-13 11:16 ` Petr Štetiar
@ 2019-05-14 15:13 ` Srinivas Kandagatla
2019-05-14 17:44 ` Petr Štetiar
2019-05-20 14:28 ` Rob Herring
1 sibling, 1 reply; 28+ messages in thread
From: Srinivas Kandagatla @ 2019-05-14 15:13 UTC (permalink / raw)
To: Petr Štetiar
Cc: Maxime Ripard, Andy Duan, davem, netdev, john, bgolaszewski,
Andrew Lunn, Florian Fainelli, Heiner Kallweit, Rob Herring,
Frank Rowand, Mark Rutland, Alban Bedel, devicetree
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;
> };
>
> ð1 {
> nvmem-cells = <ð1_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
>
> ð1 {
> nvmem = <&nvmem_dev>;
> nvmem-cells = <ð1_addr>;
> nvmem-cell-names = "mac-address";
> };
>
> -- ynezz
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]
2019-05-14 15:13 ` Srinivas Kandagatla
@ 2019-05-14 17:44 ` Petr Štetiar
2019-05-15 17:12 ` Srinivas Kandagatla
0 siblings, 1 reply; 28+ messages in thread
From: Petr Štetiar @ 2019-05-14 17:44 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Maxime Ripard, Andy Duan, davem, netdev, john, bgolaszewski,
Andrew Lunn, Florian Fainelli, Heiner Kallweit, Rob Herring,
Frank Rowand, Mark Rutland, Alban Bedel, devicetree
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
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]
2019-05-14 17:44 ` Petr Štetiar
@ 2019-05-15 17:12 ` Srinivas Kandagatla
0 siblings, 0 replies; 28+ messages in thread
From: Srinivas Kandagatla @ 2019-05-15 17:12 UTC (permalink / raw)
To: Petr Štetiar
Cc: Maxime Ripard, Andy Duan, davem, netdev, john, bgolaszewski,
Andrew Lunn, Florian Fainelli, Heiner Kallweit, Rob Herring,
Frank Rowand, Mark Rutland, Alban Bedel, devicetree
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]
2019-05-13 11:16 ` Petr Štetiar
2019-05-14 15:13 ` Srinivas Kandagatla
@ 2019-05-20 14:28 ` Rob Herring
1 sibling, 0 replies; 28+ messages in thread
From: Rob Herring @ 2019-05-20 14:28 UTC (permalink / raw)
To: Petr Štetiar
Cc: Srinivas Kandagatla, Maxime Ripard, Andy Duan, davem, netdev,
john, bgolaszewski, Andrew Lunn, Florian Fainelli,
Heiner Kallweit, Frank Rowand, Mark Rutland, Alban Bedel,
devicetree
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;
> };
>
> ð1 {
> nvmem-cells = <ð1_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;
> };
> };
> };
> };
> };
>
> ð1 {
> nvmem = <&nvmem_dev>;
> nvmem-cells = <ð1_addr>;
> nvmem-cell-names = "mac-address";
> };
>
> -- ynezz
^ permalink raw reply [flat|nested] 28+ messages in thread
* RE: [EXT] Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order
2019-05-10 11:31 ` Maxime Ripard
(?)
(?)
@ 2019-05-13 3:38 ` Andy Duan
2019-05-13 9:35 ` ynezz
-1 siblings, 1 reply; 28+ messages in thread
From: Andy Duan @ 2019-05-13 3:38 UTC (permalink / raw)
To: Maxime Ripard, Petr Štetiar
Cc: davem, netdev, john, bgolaszewski, Srinivas Kandagatla,
Andrew Lunn, Florian Fainelli, Heiner Kallweit, Rob Herring,
Frank Rowand, Mark Rutland, Alban Bedel, devicetree
From: Maxime Ripard <maxime.ripard@bootlin.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@nxp.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):
> >
> > ð0 {
> > nvmem-cells = <ð0_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
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order
2019-05-13 3:38 ` [EXT] Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order Andy Duan
@ 2019-05-13 9:35 ` ynezz
0 siblings, 0 replies; 28+ messages in thread
From: ynezz @ 2019-05-13 9:35 UTC (permalink / raw)
To: Andy Duan
Cc: Maxime Ripard, Petr Štetiar, davem, netdev, john,
bgolaszewski, Srinivas Kandagatla, Andrew Lunn, Florian Fainelli,
Heiner Kallweit, Rob Herring, Frank Rowand, Mark Rutland,
Alban Bedel, devicetree
Andy Duan <fugang.duan@nxp.com> [2019-05-13 03:38:32]:
> From: Maxime Ripard <maxime.ripard@bootlin.com> Sent: Friday, May 10, 2019 7:32 PM
> >
> > It looks to me that it should be abstracted away by the nvmem interface and
> > done at the provider level, not the customer.
> >
> If to implement add above features like Petr Štetiar described, it should be abstracted
> In nvmem core driver.
Maxime made it clear, that network layer as a consumer of the nvmem provider
doesn't need to know about this byte order swapping details, so this byte
order swapping should be implemented in nvmem as well, as a bonus it doesn't
matter if you're going to swap 3, 6 or whatever other amount of bytes
described by the reg property, so this functionality could be reused which is
always good.
-- ynezz
^ permalink raw reply [flat|nested] 28+ messages in thread