[PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order

[PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order

[Andy Duan]

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.

The patch set is to add property "nvmem_macaddr_swap" to swap
macaddr bytes order. If MAC address read from nvmem cell and
it is valid mac address, .of_get_mac_addr_nvmem() add new property
"nvmem-mac-address" in ethernet node. Later user call
.of_get_mac_address() to get MAC address again, it can read
valid MAC address from device tree in directly.

Update these two properties for binding documentation.


Fugang Duan (3):
  net: ethernet: add property "nvmem_macaddr_swap" to swap macaddr bytes
    order
  of_net: add property "nvmem-mac-address" for of_get_mac_addr()
  dt-bindings: doc: add new properties for of_get_mac_address from nvmem

 Documentation/devicetree/bindings/net/ethernet.txt |  3 +++
 drivers/of/of_net.c                                |  4 ++++
 net/ethernet/eth.c                                 | 25 +++++++++++++++++-----
 3 files changed, 27 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH net 1/3] net: ethernet: add property "nvmem_macaddr_swap" to swap macaddr bytes order

[Andy Duan]

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.

For example, read MAC address from nvmem:
of_get_mac_address()
	of_get_mac_addr_nvmem()
		nvmem_get_mac_address()

i.MX6x/7D/8MQ/8MM platforms ethernet MAC address read from
nvmem ocotp eFuses, but it requires to swap the six bytes
order.

The patch add optional property "nvmem_macaddr_swap" to swap
macaddr bytes order.

Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
---
 net/ethernet/eth.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 4b2b222..0a0986b 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -583,8 +583,10 @@ EXPORT_SYMBOL(eth_platform_get_mac_address);
 int nvmem_get_mac_address(struct device *dev, void *addrbuf)
 {
 	struct nvmem_cell *cell;
-	const void *mac;
+	const unsigned char *mac;
+	unsigned char macaddr[ETH_ALEN];
 	size_t len;
+	int i = 0;
 
 	cell = nvmem_cell_get(dev, "mac-address");
 	if (IS_ERR(cell))
@@ -596,14 +598,27 @@ int nvmem_get_mac_address(struct device *dev, void *addrbuf)
 	if (IS_ERR(mac))
 		return PTR_ERR(mac);
 
-	if (len != ETH_ALEN || !is_valid_ether_addr(mac)) {
-		kfree(mac);
-		return -EINVAL;
+	if (len != ETH_ALEN)
+		goto invalid_addr;
+
+	if (dev->of_node &&
+	    of_property_read_bool(dev->of_node, "nvmem_macaddr_swap")) {
+		for (i = 0; i < ETH_ALEN; i++)
+			macaddr[i] = mac[ETH_ALEN - i - 1];
+	} else {
+		ether_addr_copy(macaddr, mac);
 	}
 
-	ether_addr_copy(addrbuf, mac);
+	if (!is_valid_ether_addr(macaddr))
+		goto invalid_addr;
+
+	ether_addr_copy(addrbuf, macaddr);
 	kfree(mac);
 
 	return 0;
+
+invalid_addr:
+	kfree(mac);
+	return -EINVAL;
 }
 EXPORT_SYMBOL(nvmem_get_mac_address);
-- 
2.7.4

[PATCH net 2/3] of_net: add property "nvmem-mac-address" for of_get_mac_addr()

[Andy Duan]

If MAC address read from nvmem cell and it is valid mac address,
.of_get_mac_addr_nvmem() add new property "nvmem-mac-address" in
ethernet node. Once user call .of_get_mac_address() to get MAC
address again, it can read valid MAC address from device tree in
directly.

Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
---
 drivers/of/of_net.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index 9649cd5..5071241 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -125,6 +125,10 @@ const void *of_get_mac_address(struct device_node *np)
 	if (addr)
 		return addr;
 
+	addr = of_get_mac_addr(np, "nvmem-mac-address");
+	if (addr)
+		return addr;
+
 	return of_get_mac_addr_nvmem(np);
 }
 EXPORT_SYMBOL(of_get_mac_address);
-- 
2.7.4

[PATCH net 3/3] dt-bindings: doc: add new properties for of_get_mac_address from nvmem

[Andy Duan]

Currently, of_get_mac_address supports NVMEM, some platforms
MAC address that read from NVMEM efuse requires to swap bytes
order, so add new property "nvmem_macaddr_swap" to specify the
behavior. If the MAC address is valid from NVMEM, add new property
"nvmem-mac-address" in ethernet node.

Update these two properties in the binding documentation.

Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
---
 Documentation/devicetree/bindings/net/ethernet.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
index e88c364..921364a 100644
--- a/Documentation/devicetree/bindings/net/ethernet.txt
+++ b/Documentation/devicetree/bindings/net/ethernet.txt
@@ -10,8 +10,11 @@ Documentation/devicetree/bindings/phy/phy-bindings.txt.
   property;
 - local-mac-address: array of 6 bytes, specifies the MAC address that was
   assigned to the network device;
+- nvmem-mac-address: array of 6 bytes, specifies the MAC address that was
+  read from nvmem-cells and dynamically add the property in device node;
 - 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_macaddr_swap: swap bytes order for the 6 bytes of MAC address
 - max-speed: number, specifies maximum speed in Mbit/s supported by the device;
 - max-frame-size: number, maximum transfer unit (IEEE defined MTU), rather than
   the maximum frame size (there's contradiction in the Devicetree
-- 
2.7.4

Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order

[Petr Štetiar]

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):

 &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?

Cheers,

Petr

Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4711 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):
>
>  &eth0 {
> 	nvmem-cells = <&eth0_addr>;
> 	nvmem-cell-names = "mac-address";
> 	nvmem_macaddr_swap;
>  };
>
> I'm not sure about the `nvmem_macaddr_swap` property name, as currently there
> are no other properties with underscores, so it should be probably named as
> `nvmem-macaddr-swap`. DT specs permits use of the underscores, but the
> estabilished convention is probably prefered.
>
> In order to cover all above mentioned use cases, it would make more sense
> to add a description of the MAC address layout to the DT and use this
> information to properly postprocess the NVMEM content into usable MAC
> address?
>
> Something like this?
>
>  - nvmem-cells: phandle, reference to an nvmem node for the MAC address
>  - nvmem-cell-names: string, should be "mac-address" if nvmem is to be used
>  - nvmem-mac-address-layout: string, specifies MAC address storage layout.
>    Supported values are: "binary", "binary-swapped", "ascii", "ascii-delimited".
>    "binary" is the default.
>
> Or perhaps something like this?
>
>  - nvmem-cells: phandle, reference to an nvmem node for the MAC address
>  - nvmem-cell-names: string, should be any of the supported values.
>    Supported values are: "mac-address", "mac-address-swapped",
>    "mac-address-ascii", "mac-address-ascii-delimited".
>
> I'm more inclined towards the first proposed solution, as I would like to
> propose MAC address octet incrementation feature in the future, so it would
> become:
>
>  - nvmem-cells: phandle, reference to an nvmem node for the MAC address
>  - nvmem-cell-names: string, should be "mac-address" if nvmem is to be used
>  - nvmem-mac-address-layout: string, specifies MAC address storage layout.
>    Supported values are: "binary", "binary-swapped", "ascii", "ascii-delimited".
>    "binary" is the default.
>  - nvmem-mac-address-increment: number, value by which should be
>    incremented MAC address octet, could be negative value as well.
>  - nvmem-mac-address-increment-octet: number, valid values 0-5, default is 5.
>    Specifies MAC address octet used for `nvmem-mac-address-increment`.
>
> What do you think?

It looks to me that it should be abstracted away by the nvmem
interface and done at the provider level, not the customer.

Maxime

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order

[Maxime Ripard]

[-- 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):
>
>  &eth0 {
> 	nvmem-cells = <&eth0_addr>;
> 	nvmem-cell-names = "mac-address";
> 	nvmem_macaddr_swap;
>  };
>
> I'm not sure about the `nvmem_macaddr_swap` property name, as currently there
> are no other properties with underscores, so it should be probably named as
> `nvmem-macaddr-swap`. DT specs permits use of the underscores, but the
> estabilished convention is probably prefered.
>
> In order to cover all above mentioned use cases, it would make more sense
> to add a description of the MAC address layout to the DT and use this
> information to properly postprocess the NVMEM content into usable MAC
> address?
>
> Something like this?
>
>  - nvmem-cells: phandle, reference to an nvmem node for the MAC address
>  - nvmem-cell-names: string, should be "mac-address" if nvmem is to be used
>  - nvmem-mac-address-layout: string, specifies MAC address storage layout.
>    Supported values are: "binary", "binary-swapped", "ascii", "ascii-delimited".
>    "binary" is the default.
>
> Or perhaps something like this?
>
>  - nvmem-cells: phandle, reference to an nvmem node for the MAC address
>  - nvmem-cell-names: string, should be any of the supported values.
>    Supported values are: "mac-address", "mac-address-swapped",
>    "mac-address-ascii", "mac-address-ascii-delimited".
>
> I'm more inclined towards the first proposed solution, as I would like to
> propose MAC address octet incrementation feature in the future, so it would
> become:
>
>  - nvmem-cells: phandle, reference to an nvmem node for the MAC address
>  - nvmem-cell-names: string, should be "mac-address" if nvmem is to be used
>  - nvmem-mac-address-layout: string, specifies MAC address storage layout.
>    Supported values are: "binary", "binary-swapped", "ascii", "ascii-delimited".
>    "binary" is the default.
>  - nvmem-mac-address-increment: number, value by which should be
>    incremented MAC address octet, could be negative value as well.
>  - nvmem-mac-address-increment-octet: number, valid values 0-5, default is 5.
>    Specifies MAC address octet used for `nvmem-mac-address-increment`.
>
> What do you think?

It looks to me that it should be abstracted away by the nvmem
interface and done at the provider level, not the customer.

Maxime

Re: [PATCH net 1/3] net: ethernet: add property "nvmem_macaddr_swap" to swap macaddr bytes order

[Andrew Lunn]

On Fri, May 10, 2019 at 08:24:00AM +0000, Andy Duan wrote:
> 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.
> 
> For example, read MAC address from nvmem:
> of_get_mac_address()
> 	of_get_mac_addr_nvmem()
> 		nvmem_get_mac_address()
> 
> i.MX6x/7D/8MQ/8MM platforms ethernet MAC address read from
> nvmem ocotp eFuses, but it requires to swap the six bytes
> order.
> 
> The patch add optional property "nvmem_macaddr_swap" to swap

Please use nvmem-macaddr-swap. It is very unusal to use _ in property
names.

	Andrew

Re: [PATCH net 2/3] of_net: add property "nvmem-mac-address" for of_get_mac_addr()

[Andrew Lunn]

On Fri, May 10, 2019 at 08:24:03AM +0000, Andy Duan wrote:
> If MAC address read from nvmem cell and it is valid mac address,
> .of_get_mac_addr_nvmem() add new property "nvmem-mac-address" in
> ethernet node. Once user call .of_get_mac_address() to get MAC
> address again, it can read valid MAC address from device tree in
> directly.

I suspect putting the MAC address into OF will go away in a follow up
patch. It is a bad idea.

       Andrew

NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]

[Petr Štetiar]

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?

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.

Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]

[Maxime Ripard]

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

Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]

[Maxime Ripard]

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

RE: [EXT] Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order

[Andy Duan]

From: Petr Štetiar <ynezz@true.cz> Sent: Friday, May 10, 2019 7:28 PM
> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> kernel.org%2Fdoc%2Fhtml%2Flatest%2Fnetworking%2Fnetdev-FAQ.html&am
> p;data=02%7C01%7Cfugang.duan%40nxp.com%7Cdc1bcd43f3bd41701eed08
> d6d53a9dee%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63693
> 0845065526608&amp;sdata=QQItI08aTcR%2Bl4k%2FCqPCEPwT9o4GfzpZSM
> gf37ollWc%3D&amp;reserved=0

Thanks for your reminder.  ^_^
> 
> > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Fopenwrt%2Fopenwrt%2Fpull%2F1448%23issuecomment-442476695
> &amp;data=02%7C01%7Cfugang.duan%40nxp.com%7Cdc1bcd43f3bd41701e
> ed08d6d53a9dee%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> 36930845065526608&amp;sdata=VGzzDGMRrt6f%2FHZE%2BX4%2FieOkMQ
> EBC%2BiKNgKpu9Loltk%3D&amp;reserved=0)
> 
>  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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Fopenwrt%2Fopenwrt%2Fpull%2F1906%23issuecomment-483881911
> &amp;data=02%7C01%7Cfugang.duan%40nxp.com%7Cdc1bcd43f3bd41701e
> ed08d6d53a9dee%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> 36930845065526608&amp;sdata=y5%2F4e6tuEub%2Fj9fqOQXM3as%2BbKA
> vw6O3VY9oPE1qinU%3D&amp;reserved=0)
> 
> > 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.
> 
Yes, `nvmem-macaddr-swap` like is better.
It just to let i.MX series platform nvmem work for of_get_mac_address.

Not consider others' use cases like blew your mentioned since I am not familiar with
others platforms. Your consider a more comprehensive cases, it is great.

> 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?
The last one is better.

> 
> Cheers,
> 
> Petr

RE: [EXT] Re: [PATCH net 1/3] net: ethernet: add property "nvmem_macaddr_swap" to swap macaddr bytes order

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Saturday, May 11, 2019 2:17 AM
> On Fri, May 10, 2019 at 08:24:00AM +0000, Andy Duan wrote:
> > 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.
> >
> > For example, read MAC address from nvmem:
> > of_get_mac_address()
> >       of_get_mac_addr_nvmem()
> >               nvmem_get_mac_address()
> >
> > i.MX6x/7D/8MQ/8MM platforms ethernet MAC address read from nvmem
> ocotp
> > eFuses, but it requires to swap the six bytes order.
> >
> > The patch add optional property "nvmem_macaddr_swap" to swap
> 
> Please use nvmem-macaddr-swap. It is very unusal to use _ in property
> names.
> 
>         Andrew

Thanks, "nvmem-macaddr-swap" string is better. 
I will change "_" to "-" in next version.

RE: [EXT] Re: [PATCH net 2/3] of_net: add property "nvmem-mac-address" for of_get_mac_addr()

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Saturday, May 11, 2019 2:18 AM
> On Fri, May 10, 2019 at 08:24:03AM +0000, Andy Duan wrote:
> > If MAC address read from nvmem cell and it is valid mac address,
> > .of_get_mac_addr_nvmem() add new property "nvmem-mac-address" in
> > ethernet node. Once user call .of_get_mac_address() to get MAC address
> > again, it can read valid MAC address from device tree in directly.
> 
> I suspect putting the MAC address into OF will go away in a follow up patch. It
> is a bad idea.
> 
>        Andrew

I don't know the history why does of_get_mac_addr_nvmem() add the new property
"nvmem-mac-address" into OF. But if it already did that, so the patch add the property
check in . of_get_mac_address() to avoid multiple read nvmem cells in driver.

Andy

RE: [EXT] Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order

[Andy Duan]

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):
> >
> >  &eth0 {
> > 	nvmem-cells = <&eth0_addr>;
> > 	nvmem-cell-names = "mac-address";
> > 	nvmem_macaddr_swap;
> >  };
> >
> > I'm not sure about the `nvmem_macaddr_swap` property name, as
> > currently there are no other properties with underscores, so it should
> > be probably named as `nvmem-macaddr-swap`. DT specs permits use of the
> > underscores, but the estabilished convention is probably prefered.
> >
> > In order to cover all above mentioned use cases, it would make more
> > sense to add a description of the MAC address layout to the DT and use
> > this information to properly postprocess the NVMEM content into usable
> > MAC address?
> >
> > Something like this?
> >
> >  - nvmem-cells: phandle, reference to an nvmem node for the MAC
> > address
> >  - nvmem-cell-names: string, should be "mac-address" if nvmem is to be
> > used
> >  - nvmem-mac-address-layout: string, specifies MAC address storage
> layout.
> >    Supported values are: "binary", "binary-swapped", "ascii",
> "ascii-delimited".
> >    "binary" is the default.
> >
> > Or perhaps something like this?
> >
> >  - nvmem-cells: phandle, reference to an nvmem node for the MAC
> > address
> >  - nvmem-cell-names: string, should be any of the supported values.
> >    Supported values are: "mac-address", "mac-address-swapped",
> >    "mac-address-ascii", "mac-address-ascii-delimited".
> >
> > I'm more inclined towards the first proposed solution, as I would like
> > to propose MAC address octet incrementation feature in the future, so
> > it would
> > become:
> >
> >  - nvmem-cells: phandle, reference to an nvmem node for the MAC
> > address
> >  - nvmem-cell-names: string, should be "mac-address" if nvmem is to be
> > used
> >  - nvmem-mac-address-layout: string, specifies MAC address storage
> layout.
> >    Supported values are: "binary", "binary-swapped", "ascii",
> "ascii-delimited".
> >    "binary" is the default.
> >  - nvmem-mac-address-increment: number, value by which should be
> >    incremented MAC address octet, could be negative value as well.
> >  - nvmem-mac-address-increment-octet: number, valid values 0-5, default
> is 5.
> >    Specifies MAC address octet used for
> `nvmem-mac-address-increment`.
> >
> > What do you think?
> 
> It looks to me that it should be abstracted away by the nvmem interface and
> done at the provider level, not the customer.
> 
> Maxime
> 
If to implement add above features like Petr Štetiar described, it should be abstracted
In nvmem core driver.

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

Re: [EXT] Re: [PATCH net 2/3] of_net: add property "nvmem-mac-address" for of_get_mac_addr()

[Petr Štetiar]

Andy Duan <fugang.duan@nxp.com> [2019-05-13 03:31:59]:

> From: Andrew Lunn <andrew@lunn.ch> Sent: Saturday, May 11, 2019 2:18 AM
> > On Fri, May 10, 2019 at 08:24:03AM +0000, Andy Duan wrote:
> > > If MAC address read from nvmem cell and it is valid mac address,
> > > .of_get_mac_addr_nvmem() add new property "nvmem-mac-address" in
> > > ethernet node. Once user call .of_get_mac_address() to get MAC address
> > > again, it can read valid MAC address from device tree in directly.
> > 
> > I suspect putting the MAC address into OF will go away in a follow up patch. It
> > is a bad idea.
> > 
> >        Andrew
> 
> I don't know the history why does of_get_mac_addr_nvmem() add the new property
> "nvmem-mac-address" into OF. But if it already did that, so the patch add the property
> check in . of_get_mac_address() to avoid multiple read nvmem cells in driver.

it was removed[1] already, more details[2].

1. https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=265749861a2483263e6cd4c5e305640e4651c110
2. https://patchwork.ozlabs.org/patch/1092248/#2167609

-- ynnezz

Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]

[Srinivas Kandagatla]

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

RE: [EXT] Re: [PATCH net 2/3] of_net: add property "nvmem-mac-address" for of_get_mac_addr()

[Andy Duan]

From: Petr Štetiar <ynezz@true.cz> Sent: Monday, May 13, 2019 4:00 PM
> Andy Duan <fugang.duan@nxp.com> [2019-05-13 03:31:59]:
> 
> > From: Andrew Lunn <andrew@lunn.ch> Sent: Saturday, May 11, 2019 2:18
> > AM
> > > On Fri, May 10, 2019 at 08:24:03AM +0000, Andy Duan wrote:
> > > > If MAC address read from nvmem cell and it is valid mac address,
> > > > .of_get_mac_addr_nvmem() add new property "nvmem-mac-address"
> in
> > > > ethernet node. Once user call .of_get_mac_address() to get MAC
> > > > address again, it can read valid MAC address from device tree in directly.
> > >
> > > I suspect putting the MAC address into OF will go away in a follow
> > > up patch. It is a bad idea.
> > >
> > >        Andrew
> >
> > I don't know the history why does of_get_mac_addr_nvmem() add the new
> > property "nvmem-mac-address" into OF. But if it already did that, so
> > the patch add the property check in . of_get_mac_address() to avoid
> multiple read nvmem cells in driver.
> 
> it was removed[1] already, more details[2].
> 
> 1.
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ker
> nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fdavem%2Fnet.git%2Fco
> mmit%2F%3Fid%3D265749861a2483263e6cd4c5e305640e4651c110&amp;d
> ata=02%7C01%7Cfugang.duan%40nxp.com%7Ca9620569af74418a6a1d08d6
> d779076b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6369333
> 12147303481&amp;sdata=b2BB49WSZDqUfMhCeHobjI%2FLcXLed5sBkXhcR
> uM3mg4%3D&amp;reserved=0
> 2.
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.ozlabs.org%2Fpatch%2F1092248%2F%232167609&amp;data=02%7C01
> %7Cfugang.duan%40nxp.com%7Ca9620569af74418a6a1d08d6d779076b%7C
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636933312147313486
> &amp;sdata=Xu7CJyxmOC476k2RiMu2lm6h9juvjEflbztzp1wryuk%3D&amp;re
> served=0
> 
> -- ynnezz

Got it, thanks for your information.
So the patch also can be ignored.

Thanks, Petr Štetiar.

Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]

[Petr Štetiar]

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

Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]

[Petr Štetiar]

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

Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order

[ynezz]

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

Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]

[Srinivas Kandagatla]

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
> 

Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]

[Petr Štetiar]

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

Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]

[Srinivas Kandagatla]

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
> 

Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]

[Petr Štetiar]

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

Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]

[Srinivas Kandagatla]

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

Re: NVMEM address DT post processing [Was: Re: [PATCH net 0/3] add property "nvmem_macaddr_swap" to swap macaddr bytes order]

[Rob Herring]

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