[PATCH v2 0/4] of_net: Add NVMEM support to of_get_mac_address

[PATCH v2 0/4] of_net: Add NVMEM support to of_get_mac_address

[Petr Štetiar]

Hi,

this patch series is a continuation of my previous attempt[1], where I've
tried to wire MTD layer into of_get_mac_address, so it would be possible to
load MAC addresses from various NVMEMs as EEPROMs etc.

Predecessor of this patch which used directly MTD layer has originated in
OpenWrt some time ago and supports already about 497 use cases in 357
device tree files.

During the review process of my 1st attempt I was told, that I shouldn't be
using MTD directly, but I should rather use new NVMEM subsystem, so this
patch series tries to accommodate that.

First patch is wiring NVMEM support directly into of_get_mac_address as
it's obvious, that adding support for NVMEM into every other driver would
mean adding a lot of repetitive code. This patch allows us to configure MAC
addresses in various devices like ethernet and wireless adapters directly
from of_get_mac_address, which is used by quite a lot of drivers in the
tree already.

Second patch is simply updating documentation with NVMEM bits, also adding
some missing bits like mac-address and local-mac-address properties, which
are currently supported by of_get_mac_address.

Third and fourth patches are simply removing duplicate NVMEM code which is
no longer needed as the first patch has added NVMEM support directly into
of_get_mac_address.

Just for a better picture, this patch series and one simple patch[2] on top
of it, allows me to configure 8Devices Carambola2 board's MAC addresses
with following DTS (simplified):

 &spi {
 	flash@0 {
 		partitions {
			art: partition@ff0000 {
				label = "art";
				reg = <0xff0000 0x010000>;
				read-only;

				nvmem-cells {
					compatible = "nvmem-cells";
					#address-cells = <1>;
					#size-cells = <1>;

					eth0_addr: eth-mac-addr@0 {
						reg = <0x0 0x6>;
					};

					eth1_addr: eth-mac-addr@6 {
						reg = <0x6 0x6>;
					};

					wmac_addr: wifi-mac-addr@1002 {
						reg = <0x1002 0x6>;
					};
				};
			};
		};
	};
 };

 &eth0 {
	nvmem-cells = <&eth0_addr>;
	nvmem-cell-names = "mac-address";
 };

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

 &wmac {
	nvmem-cells = <&wmac_addr>;
	nvmem-cell-names = "mac-address";
 };


1. https://patchwork.ozlabs.org/patch/1086628/
2. https://patchwork.ozlabs.org/patch/890738/

-- ynezz

Petr Štetiar (4):
  of_net: Add NVMEM support to of_get_mac_address
  dt-bindings: doc: Reflect new NVMEM of_get_mac_address behaviour
  net: macb: Drop nvmem_get_mac_address usage
  net: davinci_emac: Drop nvmem_get_mac_address usage

 .../devicetree/bindings/net/altera_tse.txt         |  5 ++-
 Documentation/devicetree/bindings/net/amd-xgbe.txt |  5 +--
 .../devicetree/bindings/net/brcm,amac.txt          |  4 +--
 Documentation/devicetree/bindings/net/cpsw.txt     |  5 +--
 .../devicetree/bindings/net/davinci_emac.txt       |  5 +--
 Documentation/devicetree/bindings/net/dsa/dsa.txt  | 13 ++-----
 Documentation/devicetree/bindings/net/ethernet.txt |  6 ++--
 .../devicetree/bindings/net/hisilicon-femac.txt    |  6 ++--
 .../bindings/net/hisilicon-hix5hd2-gmac.txt        |  7 ++--
 .../devicetree/bindings/net/keystone-netcp.txt     |  8 ++---
 Documentation/devicetree/bindings/net/macb.txt     |  5 ++-
 .../devicetree/bindings/net/marvell-pxa168.txt     |  5 +--
 .../devicetree/bindings/net/microchip,enc28j60.txt |  3 +-
 .../devicetree/bindings/net/microchip,lan78xx.txt  |  5 ++-
 .../devicetree/bindings/net/qca,qca7000.txt        |  4 ++-
 .../devicetree/bindings/net/samsung-sxgbe.txt      |  6 ++--
 .../bindings/net/snps,dwc-qos-ethernet.txt         |  6 ++--
 .../bindings/net/socionext,uniphier-ave4.txt       |  4 +--
 .../devicetree/bindings/net/socionext-netsec.txt   |  7 ++--
 .../bindings/net/wireless/mediatek,mt76.txt        |  5 +--
 .../devicetree/bindings/net/wireless/qca,ath9k.txt |  4 +--
 drivers/net/ethernet/cadence/macb_main.c           | 12 ++-----
 drivers/net/ethernet/ti/davinci_emac.c             | 14 +++-----
 drivers/of/of_net.c                                | 42 ++++++++++++++++++++--
 24 files changed, 102 insertions(+), 84 deletions(-)

-- 
1.9.1

[PATCH v2 1/4] of_net: Add NVMEM support to of_get_mac_address

[Petr Štetiar]

Many embedded devices have information such as MAC addresses stored
inside NVMEMs like EEPROMs and so on. Currently there are only two
drivers in the tree which benefit from NVMEM bindings.

Adding support for NVMEM into every other driver would mean adding a lot
of repetitive code. This patch allows us to configure MAC addresses in
various devices like ethernet and wireless adapters directly from
of_get_mac_address, which is already used by almost every driver in the
tree.

Predecessor of this patch which used directly MTD layer has originated
in OpenWrt some time ago and supports already about 497 use cases in 357
device tree files.

Cc: Alban Bedel <albeu@free.fr>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
Signed-off-by: John Crispin <john@phrozen.org>
Signed-off-by: Petr Štetiar <ynezz@true.cz>
---

 Changes since v1:

  * moved handling of nvmem after mac-address and local-mac-address properties

 drivers/of/of_net.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index d820f3e..8ce4f47 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -8,6 +8,7 @@
 #include <linux/etherdevice.h>
 #include <linux/kernel.h>
 #include <linux/of_net.h>
+#include <linux/of_platform.h>
 #include <linux/phy.h>
 #include <linux/export.h>
 
@@ -47,12 +48,45 @@ static const void *of_get_mac_addr(struct device_node *np, const char *name)
 	return NULL;
 }
 
+static const void *of_get_mac_addr_nvmem(struct device_node *np)
+{
+	int r;
+	u8 mac[ETH_ALEN];
+	struct property *pp;
+	struct platform_device *pdev = of_find_device_by_node(np);
+
+	if (!pdev)
+		return NULL;
+
+	r = nvmem_get_mac_address(&pdev->dev, &mac);
+	if (r < 0)
+		return NULL;
+
+	pp = kzalloc(sizeof(*pp), GFP_KERNEL);
+	if (!pp)
+		return NULL;
+
+	pp->name = "nvmem-mac-address";
+	pp->length = ETH_ALEN;
+	pp->value = kmemdup(mac, ETH_ALEN, GFP_KERNEL);
+	if (!pp->value || of_add_property(np, pp))
+		goto free;
+
+	return pp->value;
+free:
+	kfree(pp->value);
+	kfree(pp);
+
+	return NULL;
+}
+
 /**
  * Search the device tree for the best MAC address to use.  'mac-address' is
  * checked first, because that is supposed to contain to "most recent" MAC
  * address. If that isn't set, then 'local-mac-address' is checked next,
- * because that is the default address.  If that isn't set, then the obsolete
- * 'address' is checked, just in case we're using an old device tree.
+ * because that is the default address.  If that isn't set, try to get MAC
+ * address from nvmem cell named 'mac-address'. If that isn't set, then the
+ * obsolete 'address' is checked, just in case we're using an old device tree.
  *
  * Note that the 'address' property is supposed to contain a virtual address of
  * the register set, but some DTS files have redefined that property to be the
@@ -77,6 +111,10 @@ const void *of_get_mac_address(struct device_node *np)
 	if (addr)
 		return addr;
 
+	addr = of_get_mac_addr_nvmem(np);
+	if (addr)
+		return addr;
+
 	return of_get_mac_addr(np, "address");
 }
 EXPORT_SYMBOL(of_get_mac_address);
-- 
1.9.1

Re: [PATCH v2 1/4] of_net: Add NVMEM support to of_get_mac_address

[Rob Herring]

On Sun, Apr 28, 2019 at 02:53:19PM +0200, Petr Štetiar wrote:
> Many embedded devices have information such as MAC addresses stored
> inside NVMEMs like EEPROMs and so on. Currently there are only two
> drivers in the tree which benefit from NVMEM bindings.
> 
> Adding support for NVMEM into every other driver would mean adding a lot
> of repetitive code. This patch allows us to configure MAC addresses in
> various devices like ethernet and wireless adapters directly from
> of_get_mac_address, which is already used by almost every driver in the
> tree.
> 
> Predecessor of this patch which used directly MTD layer has originated
> in OpenWrt some time ago and supports already about 497 use cases in 357
> device tree files.
> 
> Cc: Alban Bedel <albeu@free.fr>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> Signed-off-by: John Crispin <john@phrozen.org>
> Signed-off-by: Petr Štetiar <ynezz@true.cz>
> ---
> 
>  Changes since v1:
> 
>   * moved handling of nvmem after mac-address and local-mac-address properties
> 
>  drivers/of/of_net.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> index d820f3e..8ce4f47 100644
> --- a/drivers/of/of_net.c
> +++ b/drivers/of/of_net.c
> @@ -8,6 +8,7 @@
>  #include <linux/etherdevice.h>
>  #include <linux/kernel.h>
>  #include <linux/of_net.h>
> +#include <linux/of_platform.h>
>  #include <linux/phy.h>
>  #include <linux/export.h>
>  
> @@ -47,12 +48,45 @@ static const void *of_get_mac_addr(struct device_node *np, const char *name)
>  	return NULL;
>  }
>  
> +static const void *of_get_mac_addr_nvmem(struct device_node *np)
> +{
> +	int r;
> +	u8 mac[ETH_ALEN];
> +	struct property *pp;
> +	struct platform_device *pdev = of_find_device_by_node(np);
> +
> +	if (!pdev)
> +		return NULL;
> +
> +	r = nvmem_get_mac_address(&pdev->dev, &mac);
> +	if (r < 0)
> +		return NULL;
> +
> +	pp = kzalloc(sizeof(*pp), GFP_KERNEL);
> +	if (!pp)
> +		return NULL;
> +
> +	pp->name = "nvmem-mac-address";
> +	pp->length = ETH_ALEN;
> +	pp->value = kmemdup(mac, ETH_ALEN, GFP_KERNEL);
> +	if (!pp->value || of_add_property(np, pp))
> +		goto free;

Why add this to the DT? You have the struct device ptr, so just use 
devm_kzalloc() if you need an allocation.

> +
> +	return pp->value;
> +free:
> +	kfree(pp->value);
> +	kfree(pp);
> +
> +	return NULL;
> +}
> +
>  /**
>   * Search the device tree for the best MAC address to use.  'mac-address' is
>   * checked first, because that is supposed to contain to "most recent" MAC
>   * address. If that isn't set, then 'local-mac-address' is checked next,
> - * because that is the default address.  If that isn't set, then the obsolete
> - * 'address' is checked, just in case we're using an old device tree.
> + * because that is the default address.  If that isn't set, try to get MAC
> + * address from nvmem cell named 'mac-address'. If that isn't set, then the
> + * obsolete 'address' is checked, just in case we're using an old device tree.
>   *
>   * Note that the 'address' property is supposed to contain a virtual address of
>   * the register set, but some DTS files have redefined that property to be the
> @@ -77,6 +111,10 @@ const void *of_get_mac_address(struct device_node *np)
>  	if (addr)
>  		return addr;
>  
> +	addr = of_get_mac_addr_nvmem(np);
> +	if (addr)
> +		return addr;
> +
>  	return of_get_mac_addr(np, "address");
>  }
>  EXPORT_SYMBOL(of_get_mac_address);
> -- 
> 1.9.1
> 

Re: [PATCH v2 1/4] of_net: Add NVMEM support to of_get_mac_address

[Petr Štetiar]

Rob Herring <robh@kernel.org> [2019-05-01 15:19:25]:

Hi Rob,

> > +	struct property *pp;

...

> > +	pp = kzalloc(sizeof(*pp), GFP_KERNEL);
> > +	if (!pp)
> > +		return NULL;
> > +
> > +	pp->name = "nvmem-mac-address";
> > +	pp->length = ETH_ALEN;
> > +	pp->value = kmemdup(mac, ETH_ALEN, GFP_KERNEL);
> > +	if (!pp->value || of_add_property(np, pp))
> > +		goto free;
> 
> Why add this to the DT?

I've just carried it over from v1 ("of_net: add mtd-mac-address support to
of_get_mac_address()")[1] as nobody objected about this so far. 

Honestly I don't know if it's necessary to have it, but so far address,
mac-address and local-mac-address properties provide this DT nodes, so I've
simply thought, that it would be good to have it for MAC address from NVMEM as
well in order to stay consistent.

Just FYI, my testing ar9331_8dev_carambola2.dts[2] currently produces
following runtime DT content:

 root@OpenWrt:/# find /sys/firmware/devicetree/ -name *nvmem* -o -name *addr@*
 /sys/firmware/devicetree/base/ahb/spi@1f000000/flash@0/partitions/partition@ff0000/nvmem-cells
 /sys/firmware/devicetree/base/ahb/spi@1f000000/flash@0/partitions/partition@ff0000/nvmem-cells/eth-mac-addr@0
 /sys/firmware/devicetree/base/ahb/spi@1f000000/flash@0/partitions/partition@ff0000/nvmem-cells/eth-mac-addr@6
 /sys/firmware/devicetree/base/ahb/spi@1f000000/flash@0/partitions/partition@ff0000/nvmem-cells/wifi-mac-addr@1002
 /sys/firmware/devicetree/base/ahb/wmac@18100000/nvmem-cells
 /sys/firmware/devicetree/base/ahb/wmac@18100000/nvmem-mac-address
 /sys/firmware/devicetree/base/ahb/wmac@18100000/nvmem-cell-names
 /sys/firmware/devicetree/base/ahb/eth@1a000000/nvmem-cells
 /sys/firmware/devicetree/base/ahb/eth@1a000000/nvmem-mac-address
 /sys/firmware/devicetree/base/ahb/eth@1a000000/nvmem-cell-names
 /sys/firmware/devicetree/base/ahb/eth@19000000/nvmem-cells
 /sys/firmware/devicetree/base/ahb/eth@19000000/nvmem-mac-address
 /sys/firmware/devicetree/base/ahb/eth@19000000/nvmem-cell-names

 root@OpenWrt:/# hexdump -C /sys/firmware/devicetree/base/ahb/wmac@18100000/nvmem-mac-address
 00000000  00 03 7f 11 52 da                                 |....R.|
 00000006

 root@OpenWrt:/# ip addr show wlan0
 4: wlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN qlen 1000
    link/ether 00:03:7f:11:52:da brd ff:ff:ff:ff:ff:ff

1. https://patchwork.ozlabs.org/patch/1086628/
2. https://git.openwrt.org/?p=openwrt/staging/ynezz.git;a=blob;f=target/linux/ath79/dts/ar9331_8dev_carambola2.dts;h=349c91e760ca5a56d65c587c949fed5fb6ea980e;hb=349c91e760ca5a56d65c587c949fed5fb6ea980e

> You have the struct device ptr, so just use devm_kzalloc() if you need an
> allocation.

I'll address this in v3, thanks.

-- ynezz

Re: [PATCH v2 1/4] of_net: Add NVMEM support to of_get_mac_address

[Rob Herring]

On Thu, May 2, 2019 at 4:05 AM Petr Štetiar <ynezz@true.cz> wrote:
>
> Rob Herring <robh@kernel.org> [2019-05-01 15:19:25]:
>
> Hi Rob,
>
> > > +   struct property *pp;
>
> ...
>
> > > +   pp = kzalloc(sizeof(*pp), GFP_KERNEL);
> > > +   if (!pp)
> > > +           return NULL;
> > > +
> > > +   pp->name = "nvmem-mac-address";
> > > +   pp->length = ETH_ALEN;
> > > +   pp->value = kmemdup(mac, ETH_ALEN, GFP_KERNEL);
> > > +   if (!pp->value || of_add_property(np, pp))
> > > +           goto free;
> >
> > Why add this to the DT?
>
> I've just carried it over from v1 ("of_net: add mtd-mac-address support to
> of_get_mac_address()")[1] as nobody objected about this so far.

That's not really a reason...

> Honestly I don't know if it's necessary to have it, but so far address,
> mac-address and local-mac-address properties provide this DT nodes, so I've
> simply thought, that it would be good to have it for MAC address from NVMEM as
> well in order to stay consistent.

If you want to be consistent, then fill in 'local-mac-address' with
the value from nvmem. We don't need the same thing with a new name
added to DT. (TBC, I'm not suggesting you do that here.)

But really, my point with using devm_kzalloc() is just return the
data, not store in DT and free it when the driver unbinds. Allocating
it with devm_kzalloc AND adding it to DT as you've done in v4 leads to
2 entities refcounting the allocation. If the driver unbinds, the
buffer is freed, but DT code is still referencing that memory.

Also, what happens the 2 time a driver binds? The property would
already be in the DT.

>
> Just FYI, my testing ar9331_8dev_carambola2.dts[2] currently produces
> following runtime DT content:
>
>  root@OpenWrt:/# find /sys/firmware/devicetree/ -name *nvmem* -o -name *addr@*
>  /sys/firmware/devicetree/base/ahb/spi@1f000000/flash@0/partitions/partition@ff0000/nvmem-cells
>  /sys/firmware/devicetree/base/ahb/spi@1f000000/flash@0/partitions/partition@ff0000/nvmem-cells/eth-mac-addr@0
>  /sys/firmware/devicetree/base/ahb/spi@1f000000/flash@0/partitions/partition@ff0000/nvmem-cells/eth-mac-addr@6
>  /sys/firmware/devicetree/base/ahb/spi@1f000000/flash@0/partitions/partition@ff0000/nvmem-cells/wifi-mac-addr@1002
>  /sys/firmware/devicetree/base/ahb/wmac@18100000/nvmem-cells
>  /sys/firmware/devicetree/base/ahb/wmac@18100000/nvmem-mac-address
>  /sys/firmware/devicetree/base/ahb/wmac@18100000/nvmem-cell-names
>  /sys/firmware/devicetree/base/ahb/eth@1a000000/nvmem-cells
>  /sys/firmware/devicetree/base/ahb/eth@1a000000/nvmem-mac-address
>  /sys/firmware/devicetree/base/ahb/eth@1a000000/nvmem-cell-names
>  /sys/firmware/devicetree/base/ahb/eth@19000000/nvmem-cells
>  /sys/firmware/devicetree/base/ahb/eth@19000000/nvmem-mac-address
>  /sys/firmware/devicetree/base/ahb/eth@19000000/nvmem-cell-names

'nvmem-mac-address' is not a documented property. That would need to
be documented before using upstream. Though, for reasons above, I
don't think it should be.

Rob

Re: [PATCH v2 1/4] of_net: Add NVMEM support to of_get_mac_address

[Petr Štetiar]

Rob Herring <robh@kernel.org> [2019-05-07 11:06:43]:

Hi,

> > Honestly I don't know if it's necessary to have it, but so far address,
> > mac-address and local-mac-address properties provide this DT nodes, so I've
> > simply thought, that it would be good to have it for MAC address from NVMEM as
> > well in order to stay consistent.
> 
> If you want to be consistent, then fill in 'local-mac-address' with
> the value from nvmem. We don't need the same thing with a new name
> added to DT. (TBC, I'm not suggesting you do that here.)

Ok, got it.

> But really, my point with using devm_kzalloc() is just return the
> data, not store in DT and free it when the driver unbinds. 

Ok, I've simply misunderstood your point, sorry, I'll handle it in the follow
up fix series, along with the DT documentation update.

> Allocating it with devm_kzalloc AND adding it to DT as you've done in v4
> leads to 2 entities refcounting the allocation. If the driver unbinds, the
> buffer is freed, but DT code is still referencing that memory.

Indeed, I did it wrong, will fix that.

> 'nvmem-mac-address' is not a documented property. That would need to
> be documented before using upstream. Though, for reasons above, I
> don't think it should be.

Ok, it makes sense now. Thanks for the detailed explanation.

-- ynezz

[PATCH v2 2/4] dt-bindings: doc: Reflect new NVMEM of_get_mac_address behaviour

[Petr Štetiar]

As of_get_mac_address now supports NVMEM under the hood, we need to update
the bindings documentation with the new nvmem-cell* properties, which would
mean copy&pasting a lot of redundant information to every binding
documentation currently referencing some of the MAC address properties.

So I've just removed all the references to the optional MAC address
properties and replaced them with the reference to the net/ethernet.txt
file.  While at it, I've also removed other optional Ethernet properties.

Signed-off-by: Petr Štetiar <ynezz@true.cz>
---

 Changes since v1:

 * instead of updating all bindings documentation with nvmem properties,
   I've just updated those docs which were already referencing MAC address
   properties and replaced them with reference to common net/ethernet.txt

 Documentation/devicetree/bindings/net/altera_tse.txt        |  5 ++---
 Documentation/devicetree/bindings/net/amd-xgbe.txt          |  5 +++--
 Documentation/devicetree/bindings/net/brcm,amac.txt         |  4 ++--
 Documentation/devicetree/bindings/net/cpsw.txt              |  5 +++--
 Documentation/devicetree/bindings/net/davinci_emac.txt      |  5 +++--
 Documentation/devicetree/bindings/net/dsa/dsa.txt           | 13 ++-----------
 Documentation/devicetree/bindings/net/ethernet.txt          |  6 ++++--
 Documentation/devicetree/bindings/net/hisilicon-femac.txt   |  6 ++----
 .../devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt      |  7 +++----
 Documentation/devicetree/bindings/net/keystone-netcp.txt    |  8 +++-----
 Documentation/devicetree/bindings/net/macb.txt              |  5 ++---
 Documentation/devicetree/bindings/net/marvell-pxa168.txt    |  5 +++--
 .../devicetree/bindings/net/microchip,enc28j60.txt          |  3 ++-
 Documentation/devicetree/bindings/net/microchip,lan78xx.txt |  5 ++---
 Documentation/devicetree/bindings/net/qca,qca7000.txt       |  4 +++-
 Documentation/devicetree/bindings/net/samsung-sxgbe.txt     |  6 ++----
 .../devicetree/bindings/net/snps,dwc-qos-ethernet.txt       |  6 +++---
 .../devicetree/bindings/net/socionext,uniphier-ave4.txt     |  4 ++--
 Documentation/devicetree/bindings/net/socionext-netsec.txt  |  7 +++----
 .../devicetree/bindings/net/wireless/mediatek,mt76.txt      |  5 +++--
 .../devicetree/bindings/net/wireless/qca,ath9k.txt          |  4 ++--
 21 files changed, 54 insertions(+), 64 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/altera_tse.txt b/Documentation/devicetree/bindings/net/altera_tse.txt
index 0e21df9..c85a589 100644
--- a/Documentation/devicetree/bindings/net/altera_tse.txt
+++ b/Documentation/devicetree/bindings/net/altera_tse.txt
@@ -46,9 +46,8 @@ Required properties:
 	- reg: phy id used to communicate to phy.
 	- device_type: Must be "ethernet-phy".
 
-Optional properties:
-- local-mac-address: See ethernet.txt in the same directory.
-- max-frame-size: See ethernet.txt in the same directory.
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
 
 Example:
 
diff --git a/Documentation/devicetree/bindings/net/amd-xgbe.txt b/Documentation/devicetree/bindings/net/amd-xgbe.txt
index 93dcb79..b607765 100644
--- a/Documentation/devicetree/bindings/net/amd-xgbe.txt
+++ b/Documentation/devicetree/bindings/net/amd-xgbe.txt
@@ -24,8 +24,6 @@ Required properties:
 - phy-mode: See ethernet.txt file in the same directory
 
 Optional properties:
-- mac-address: mac address to be assigned to the device. Can be overridden
-  by UEFI.
 - dma-coherent: Present if dma operations are coherent
 - amd,per-channel-interrupt: Indicates that Rx and Tx complete will generate
   a unique interrupt for each DMA channel - this requires an additional
@@ -48,6 +46,9 @@ property is used.
 - amd,serdes-dfe-tap-config: DFE taps available to run
 - amd,serdes-dfe-tap-enable: DFE taps to enable
 
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
+
 Example:
 	xgbe@e0700000 {
 		compatible = "amd,xgbe-seattle-v1a";
diff --git a/Documentation/devicetree/bindings/net/brcm,amac.txt b/Documentation/devicetree/bindings/net/brcm,amac.txt
index 0bfad65..5e0ba27 100644
--- a/Documentation/devicetree/bindings/net/brcm,amac.txt
+++ b/Documentation/devicetree/bindings/net/brcm,amac.txt
@@ -16,8 +16,8 @@ Required properties:
 				registers (required for Northstar2)
  - interrupts:	Interrupt number
 
-Optional properties:
-- mac-address:	See ethernet.txt file in the same directory
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
 
 Examples:
 
diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index 3264e19..370161c9 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -49,9 +49,10 @@ Required properties:
 
 Optional properties:
 - dual_emac_res_vlan	: Specifies VID to be used to segregate the ports
-- mac-address		: See ethernet.txt file in the same directory
 - phy_id		: Specifies slave phy id (deprecated, use phy-handle)
-- phy-handle		: See ethernet.txt file in the same directory
+
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
 
 Slave sub-nodes:
 - fixed-link		: See fixed-link.txt file in the same directory
diff --git a/Documentation/devicetree/bindings/net/davinci_emac.txt b/Documentation/devicetree/bindings/net/davinci_emac.txt
index ca83dcc..d250c8e 100644
--- a/Documentation/devicetree/bindings/net/davinci_emac.txt
+++ b/Documentation/devicetree/bindings/net/davinci_emac.txt
@@ -20,11 +20,12 @@ Required properties:
 Optional properties:
 - phy-handle: See ethernet.txt file in the same directory.
               If absent, davinci_emac driver defaults to 100/FULL.
-- 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
 - ti,davinci-rmii-en: 1 byte, 1 means use RMII
 - ti,davinci-no-bd-ram: boolean, does EMAC have BD RAM?
 
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
+
 Example (enbw_cmc board):
 	eth0: emac@1e20000 {
 		compatible = "ti,davinci-dm6467-emac";
diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.txt b/Documentation/devicetree/bindings/net/dsa/dsa.txt
index d66a529..6c8da44 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.txt
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.txt
@@ -58,22 +58,13 @@ A user port has the following optional property:
 Port child nodes may also contain the following optional standardised
 properties, described in binding documents:
 
-- phy-handle		: Phandle to a PHY on an MDIO bus. See
-			  Documentation/devicetree/bindings/net/ethernet.txt
-			  for details.
-
-- phy-mode		: See
-			  Documentation/devicetree/bindings/net/ethernet.txt
-			  for details.
-
 - fixed-link		: Fixed-link subnode describing a link to a non-MDIO
 			  managed entity. See
 			  Documentation/devicetree/bindings/net/fixed-link.txt
 			  for details.
 
-- local-mac-address	: See
-			  Documentation/devicetree/bindings/net/ethernet.txt
-			  for details.
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
 
 Example
 
diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
index 2974e63..e6e01e0 100644
--- a/Documentation/devicetree/bindings/net/ethernet.txt
+++ b/Documentation/devicetree/bindings/net/ethernet.txt
@@ -4,12 +4,14 @@ NOTE: All 'phy*' properties documented below are Ethernet specific. For the
 generic PHY 'phys' property, see
 Documentation/devicetree/bindings/phy/phy-bindings.txt.
 
-- local-mac-address: array of 6 bytes, specifies the MAC address that was
-  assigned to the network device;
 - mac-address: array of 6 bytes, specifies the MAC address that was last used by
   the boot program; should be used in cases where the MAC address assigned to
   the device by the boot program is different from the "local-mac-address"
   property;
+- local-mac-address: array of 6 bytes, specifies the MAC address that was
+  assigned to the network device;
+- 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
 - 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
diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac.txt b/Documentation/devicetree/bindings/net/hisilicon-femac.txt
index d11af5e..99ad4d5 100644
--- a/Documentation/devicetree/bindings/net/hisilicon-femac.txt
+++ b/Documentation/devicetree/bindings/net/hisilicon-femac.txt
@@ -14,15 +14,13 @@ Required properties:
 	the PHY reset signal(optional).
 - reset-names: should contain the reset signal name "mac"(required)
 	and "phy"(optional).
-- mac-address: see ethernet.txt [1].
-- phy-mode: see ethernet.txt [1].
-- phy-handle: see ethernet.txt [1].
 - hisilicon,phy-reset-delays-us: triplet of delays if PHY reset signal given.
 	The 1st cell is reset pre-delay in micro seconds.
 	The 2nd cell is reset pulse in micro seconds.
 	The 3rd cell is reset post-delay in micro seconds.
 
-[1] Documentation/devicetree/bindings/net/ethernet.txt
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
 
 Example:
 	hisi_femac: ethernet@10090000 {
diff --git a/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt b/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
index eea73ad..f4aad2a 100644
--- a/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
+++ b/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
@@ -16,9 +16,6 @@ Required properties:
 - interrupts: should contain the MAC interrupt.
 - #address-cells: must be <1>.
 - #size-cells: must be <0>.
-- phy-mode: see ethernet.txt [1].
-- phy-handle: see ethernet.txt [1].
-- mac-address: see ethernet.txt [1].
 - clocks: clock phandle and specifier pair.
 - clock-names: contain the clock name "mac_core"(required) and "mac_ifc"(optional).
 - resets: should contain the phandle to the MAC core reset signal(optional),
@@ -31,9 +28,11 @@ Required properties:
 	The 2nd cell is reset pulse in micro seconds.
 	The 3rd cell is reset post-delay in micro seconds.
 
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
+
 - PHY subnode: inherits from phy binding [2]
 
-[1] Documentation/devicetree/bindings/net/ethernet.txt
 [2] Documentation/devicetree/bindings/net/phy.txt
 
 Example:
diff --git a/Documentation/devicetree/bindings/net/keystone-netcp.txt b/Documentation/devicetree/bindings/net/keystone-netcp.txt
index 04ba1dc..2c9e8ac 100644
--- a/Documentation/devicetree/bindings/net/keystone-netcp.txt
+++ b/Documentation/devicetree/bindings/net/keystone-netcp.txt
@@ -135,14 +135,12 @@ Optional properties:
 		are swapped.  The netcp driver will swap the two DWORDs
 		back to the proper order when this property is set to 2
 		when it obtains the mac address from efuse.
-- local-mac-address:	the driver is designed to use the of_get_mac_address api
-			only if efuse-mac is 0. When efuse-mac is 0, the MAC
-			address is obtained from local-mac-address. If this
-			attribute is not present, then the driver will use a
-			random MAC address.
 - "netcp-device label":	phandle to the device specification for each of NetCP
 			sub-module attached to this interface.
 
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
+
 Example binding:
 
 netcp: netcp@2000000 {
diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
index 8b80515..58923f3 100644
--- a/Documentation/devicetree/bindings/net/macb.txt
+++ b/Documentation/devicetree/bindings/net/macb.txt
@@ -26,9 +26,8 @@ Required properties:
 	Optional elements: 'tsu_clk'
 - clocks: Phandles to input clocks.
 
-Optional properties:
-- 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
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
 
 Optional properties for PHY child node:
 - reset-gpios : Should specify the gpio for phy reset
diff --git a/Documentation/devicetree/bindings/net/marvell-pxa168.txt b/Documentation/devicetree/bindings/net/marvell-pxa168.txt
index 845a148..0d36fcb 100644
--- a/Documentation/devicetree/bindings/net/marvell-pxa168.txt
+++ b/Documentation/devicetree/bindings/net/marvell-pxa168.txt
@@ -10,8 +10,9 @@ Optional properties:
 - port-id: Ethernet port number. Should be '0','1' or '2'.
 - #address-cells: must be 1 when using sub-nodes.
 - #size-cells: must be 0 when using sub-nodes.
-- phy-handle: see ethernet.txt file in the same directory.
-- local-mac-address: see ethernet.txt file in the same directory.
+
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
 
 Sub-nodes:
 Each PHY can be represented as a sub-node. This is not mandatory.
diff --git a/Documentation/devicetree/bindings/net/microchip,enc28j60.txt b/Documentation/devicetree/bindings/net/microchip,enc28j60.txt
index 24626e0..dc15eba 100644
--- a/Documentation/devicetree/bindings/net/microchip,enc28j60.txt
+++ b/Documentation/devicetree/bindings/net/microchip,enc28j60.txt
@@ -21,8 +21,9 @@ Optional properties:
 - spi-max-frequency: Maximum frequency of the SPI bus when accessing the ENC28J60.
   According to the ENC28J80 datasheet, the chip allows a maximum of 20 MHz, however,
   board designs may need to limit this value.
-- local-mac-address: See ethernet.txt in the same directory.
 
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
 
 Example (for NXP i.MX28 with pin control stuff for GPIO irq):
 
diff --git a/Documentation/devicetree/bindings/net/microchip,lan78xx.txt b/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
index 76786a0..7824581 100644
--- a/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
+++ b/Documentation/devicetree/bindings/net/microchip,lan78xx.txt
@@ -7,9 +7,8 @@ The Device Tree properties, if present, override the OTP and EEPROM.
 Required properties:
 - compatible: Should be one of "usb424,7800", "usb424,7801" or "usb424,7850".
 
-Optional properties:
-- local-mac-address:   see ethernet.txt
-- mac-address:         see ethernet.txt
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
 
 Optional properties of the embedded PHY:
 - microchip,led-modes: a 0..4 element vector, with each element configuring
diff --git a/Documentation/devicetree/bindings/net/qca,qca7000.txt b/Documentation/devicetree/bindings/net/qca,qca7000.txt
index e4a8a51..26cce1c 100644
--- a/Documentation/devicetree/bindings/net/qca,qca7000.txt
+++ b/Documentation/devicetree/bindings/net/qca,qca7000.txt
@@ -23,7 +23,6 @@ Optional properties:
 		      Numbers smaller than 1000000 or greater than 16000000
 		      are invalid. Missing the property will set the SPI
 		      frequency to 8000000 Hertz.
-- local-mac-address : see ./ethernet.txt
 - qca,legacy-mode   : Set the SPI data transfer of the QCA7000 to legacy mode.
 		      In this mode the SPI master must toggle the chip select
 		      between each data word. In burst mode these gaps aren't
@@ -31,6 +30,9 @@ Optional properties:
 		      the QCA7000 is setup via GPIO pin strapping. If the
 		      property is missing the driver defaults to burst mode.
 
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
+
 SPI Example:
 
 /* Freescale i.MX28 SPI master*/
diff --git a/Documentation/devicetree/bindings/net/samsung-sxgbe.txt b/Documentation/devicetree/bindings/net/samsung-sxgbe.txt
index 46e5911..09bd850 100644
--- a/Documentation/devicetree/bindings/net/samsung-sxgbe.txt
+++ b/Documentation/devicetree/bindings/net/samsung-sxgbe.txt
@@ -20,10 +20,8 @@ Required properties:
   When fixed length is needed for burst mode, it can be set within allowable
   range.
 
-Optional properties:
-- mac-address: 6 bytes, mac address
-- max-frame-size: Maximum Transfer Unit (IEEE defined MTU), rather
-		  than the maximum frame size.
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
 
 Example:
 
diff --git a/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt b/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt
index 36f1aef..db4c698 100644
--- a/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt
+++ b/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt
@@ -103,8 +103,6 @@ Required properties:
 
 Optional properties:
 - dma-coherent: Present if dma operations are coherent
-- mac-address: See ethernet.txt in the same directory
-- local-mac-address: See ethernet.txt in the same directory
 - phy-reset-gpios: Phandle and specifier for any GPIO used to reset the PHY.
   See ../gpio/gpio.txt.
 - snps,en-lpi: If present it enables use of the AXI low-power interface
@@ -118,7 +116,6 @@ Optional properties:
 - snps,rxpbl: DMA Programmable burst length for the RX DMA
 - snps,en-tx-lpi-clockgating: Enable gating of the MAC TX clock during
   TX low-power mode.
-- phy-handle: See ethernet.txt file in the same directory
 - mdio device tree subnode: When the GMAC has a phy connected to its local
     mdio, there must be device tree subnode with the following
     required properties:
@@ -133,6 +130,9 @@ Optional properties:
     - device_type: Must be "ethernet-phy".
     - fixed-mode device tree subnode: see fixed-link.txt in the same directory
 
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
+
 Examples:
 ethernet2@40010000 {
 	clock-names = "phy_ref_clk", "apb_pclk";
diff --git a/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt b/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
index fc8f017..d90d27e 100644
--- a/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
+++ b/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
@@ -31,8 +31,8 @@ Required properties:
  - socionext,syscon-phy-mode: A phandle to syscon with one argument
 	that configures phy mode. The argument is the ID of MAC instance.
 
-Optional properties:
- - local-mac-address: See ethernet.txt in the same directory.
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
 
 Required subnode:
  - mdio: A container for child nodes representing phy nodes.
diff --git a/Documentation/devicetree/bindings/net/socionext-netsec.txt b/Documentation/devicetree/bindings/net/socionext-netsec.txt
index 0cff94f..3073c16 100644
--- a/Documentation/devicetree/bindings/net/socionext-netsec.txt
+++ b/Documentation/devicetree/bindings/net/socionext-netsec.txt
@@ -26,10 +26,9 @@ Required properties:
 Optional properties: (See ethernet.txt file in the same directory)
 - dma-coherent: Boolean property, must only be present if memory
 	accesses performed by the device are cache coherent.
-- local-mac-address: See ethernet.txt in the same directory.
-- mac-address: See ethernet.txt in the same directory.
-- max-speed: See ethernet.txt in the same directory.
-- max-frame-size: See ethernet.txt in the same directory.
+
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
 
 Example:
 	eth0: ethernet@522d0000 {
diff --git a/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
index 7b9a776..bcccbb9 100644
--- a/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
+++ b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
@@ -13,11 +13,12 @@ properties:
 
 Optional properties:
 
-- mac-address: See ethernet.txt in the parent directory
-- local-mac-address: See ethernet.txt in the parent directory
 - ieee80211-freq-limit: See ieee80211.txt
 - mediatek,mtd-eeprom: Specify a MTD partition + offset containing EEPROM data
 
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
+
 Optional nodes:
 - led: Properties for a connected LED
   Optional properties:
diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
index b7396c8..d07542f 100644
--- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
+++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
@@ -34,9 +34,9 @@ Optional properties:
 			ath9k wireless chip (in this case the calibration /
 			EEPROM data will be loaded from userspace using the
 			kernel firmware loader).
-- mac-address: See ethernet.txt in the parent directory
-- local-mac-address: See ethernet.txt in the parent directory
 
+For all other optional Ethernet properties, please refer to
+Documentation/devicetree/bindings/net/ethernet.txt.
 
 In this example, the node is defined as child node of the PCI controller:
 &pci0 {
-- 
1.9.1

Re: [PATCH v2 2/4] dt-bindings: doc: Reflect new NVMEM of_get_mac_address behaviour

[Andrew Lunn]

On Sun, Apr 28, 2019 at 02:53:20PM +0200, Petr Štetiar wrote:
> As of_get_mac_address now supports NVMEM under the hood, we need to update
> the bindings documentation with the new nvmem-cell* properties, which would
> mean copy&pasting a lot of redundant information to every binding
> documentation currently referencing some of the MAC address properties.
> 
> So I've just removed all the references to the optional MAC address
> properties and replaced them with the reference to the net/ethernet.txt
> file.  While at it, I've also removed other optional Ethernet properties.

Hi Petr

I think each individual binding needs to give a hint if
of_get_mac_address() is used, and hence if these optional properties
are respected. The same is true for other optional properties. I don't
want to have to look at the driver to know which optional properties
are implemented, the binding should tell me. What the optional
properties mean, and which order they are used in can then be defined
in ethernet.txt.

So i would suggests something like:

The MAC address will be determined using the optional properties
defined in ethernet.txt.

And leave all the other optional parameters in the bindings.

	Andrew

Re: [PATCH v2 2/4] dt-bindings: doc: Reflect new NVMEM of_get_mac_address behaviour

[Rob Herring]

On Sun, Apr 28, 2019 at 06:53:26PM +0200, Andrew Lunn wrote:
> On Sun, Apr 28, 2019 at 02:53:20PM +0200, Petr Štetiar wrote:
> > As of_get_mac_address now supports NVMEM under the hood, we need to update
> > the bindings documentation with the new nvmem-cell* properties, which would
> > mean copy&pasting a lot of redundant information to every binding
> > documentation currently referencing some of the MAC address properties.
> > 
> > So I've just removed all the references to the optional MAC address
> > properties and replaced them with the reference to the net/ethernet.txt
> > file.  While at it, I've also removed other optional Ethernet properties.
> 
> Hi Petr
> 
> I think each individual binding needs to give a hint if
> of_get_mac_address() is used, and hence if these optional properties
> are respected. The same is true for other optional properties. I don't
> want to have to look at the driver to know which optional properties
> are implemented, the binding should tell me. What the optional
> properties mean, and which order they are used in can then be defined
> in ethernet.txt.
> 
> So i would suggests something like:
> 
> The MAC address will be determined using the optional properties
> defined in ethernet.txt.
> 
> And leave all the other optional parameters in the bindings.

Yes. Generally we need to know which properties from a common pool of 
properties apply to a specific binding. Also there are typically 
additional constraints for a specific binding.

Rob

[PATCH v2 3/4] net: macb: Drop nvmem_get_mac_address usage

[Petr Štetiar]

of_get_mac_address now uses NVMEM under the hood, so it's not necessary
to call it manually anymore.

Signed-off-by: Petr Štetiar <ynezz@true.cz>
---
 drivers/net/ethernet/cadence/macb_main.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 3da2795..1b98bc8 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4172,16 +4172,10 @@ static int macb_probe(struct platform_device *pdev)
 		bp->rx_intr_mask |= MACB_BIT(RXUBR);
 
 	mac = of_get_mac_address(np);
-	if (mac) {
+	if (mac)
 		ether_addr_copy(bp->dev->dev_addr, mac);
-	} else {
-		err = nvmem_get_mac_address(&pdev->dev, bp->dev->dev_addr);
-		if (err) {
-			if (err == -EPROBE_DEFER)
-				goto err_out_free_netdev;
-			macb_get_hwaddr(bp);
-		}
-	}
+	else
+		macb_get_hwaddr(bp);
 
 	err = of_get_phy_mode(np);
 	if (err < 0) {
-- 
1.9.1

Re: [PATCH v2 3/4] net: macb: Drop nvmem_get_mac_address usage

[Andrew Lunn]

On Sun, Apr 28, 2019 at 02:53:21PM +0200, Petr Štetiar wrote:
> of_get_mac_address now uses NVMEM under the hood, so it's not necessary
> to call it manually anymore.
> 
> Signed-off-by: Petr Štetiar <ynezz@true.cz>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 3da2795..1b98bc8 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4172,16 +4172,10 @@ static int macb_probe(struct platform_device *pdev)
>  		bp->rx_intr_mask |= MACB_BIT(RXUBR);
>  
>  	mac = of_get_mac_address(np);
> -	if (mac) {
> +	if (mac)
>  		ether_addr_copy(bp->dev->dev_addr, mac);
> -	} else {
> -		err = nvmem_get_mac_address(&pdev->dev, bp->dev->dev_addr);
> -		if (err) {
> -			if (err == -EPROBE_DEFER)

The EPRODE_DEFER is interesting here. your change to
of_get_mac_address() does not seem to handle that case. It probably
should. We don't want it to fail and end up with a random MAC
addresses etc, because the NVMEM has not probed yet.

	  Andrew

Re: [PATCH v2 3/4] net: macb: Drop nvmem_get_mac_address usage

[Petr Štetiar]

Andrew Lunn <andrew@lunn.ch> [2019-04-28 18:56:37]:

Hi Andrew,

> On Sun, Apr 28, 2019 at 02:53:21PM +0200, Petr Štetiar wrote:
> > of_get_mac_address now uses NVMEM under the hood, so it's not necessary
> > to call it manually anymore.
> > 
> > Signed-off-by: Petr Štetiar <ynezz@true.cz>
> > ---
> >  drivers/net/ethernet/cadence/macb_main.c | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 3da2795..1b98bc8 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -4172,16 +4172,10 @@ static int macb_probe(struct platform_device *pdev)
> >  		bp->rx_intr_mask |= MACB_BIT(RXUBR);
> >  
> >  	mac = of_get_mac_address(np);
> > -	if (mac) {
> > +	if (mac)
> >  		ether_addr_copy(bp->dev->dev_addr, mac);
> > -	} else {
> > -		err = nvmem_get_mac_address(&pdev->dev, bp->dev->dev_addr);
> > -		if (err) {
> > -			if (err == -EPROBE_DEFER)
>
> The EPRODE_DEFER is interesting here. your change to
> of_get_mac_address() does not seem to handle that case. It probably
> should. We don't want it to fail and end up with a random MAC
> addresses etc, because the NVMEM has not probed yet.

so if I understand this correctly, it probably means, that this approach with
modified of_get_mac_address is dead end as current of_get_mac_address users
don't expect and handle possible -EPROBE_DEFER error, so I would need to
change all the current users, which is nonsense.

-- ynezz

Re: [PATCH v2 3/4] net: macb: Drop nvmem_get_mac_address usage

[Andrew Lunn]

> so if I understand this correctly, it probably means, that this approach with
> modified of_get_mac_address is dead end as current of_get_mac_address users
> don't expect and handle possible -EPROBE_DEFER error, so I would need to
> change all the current users, which is nonsense.

Hi Petr

I would not say it is dead, it just needs a bit more work.

The current users should always be checking for a NULL pointer.  You
just need to change that to !IS_ERR(). You can then return
ERR_PTR(-PROBE_DEFER) from the NVMEM operation.

And i agree, wrapping this all up inside of_get_mac_address() is the
correct way to do this.

     Andrew

Re: [PATCH v2 3/4] net: macb: Drop nvmem_get_mac_address usage

[Petr Štetiar]

Andrew Lunn <andrew@lunn.ch> [2019-04-28 23:36:40]:

Hi Andrew,

> > so if I understand this correctly, it probably means, that this approach with
> > modified of_get_mac_address is dead end as current of_get_mac_address users
> > don't expect and handle possible -EPROBE_DEFER error, so I would need to
> > change all the current users, which is nonsense.
> 
> I would not say it is dead, it just needs a bit more work.

ok, that's good news, I've probably just misunderstood your concern about the
random MAC address in case when platform/nvmem subsystem returns -EPROBE_DEFER.

> The current users should always be checking for a NULL pointer.  You
> just need to change that to !IS_ERR(). You can then return
> ERR_PTR(-PROBE_DEFER) from the NVMEM operation.

I'm more then happy to address this in v3, but I'm still curious, what is it
going to change in the current state of the tree. 

My understanding of -PROBE_DEFER is, that it needs to be propagated back from
the driver's probe callback/hook to the upper device/driver subsystem in order
to be moved to the list of pending drivers and considered for probe later
again. This is not going to happen in any of the current drivers, thus it will
probably still always result in random MAC address in case of -EPROBE_DEFER
error from the nvmem subsystem.

-- ynezz

Re: [PATCH v2 3/4] net: macb: Drop nvmem_get_mac_address usage

[Andrew Lunn]

On Mon, Apr 29, 2019 at 09:55:14AM +0200, Petr Štetiar wrote:
> Andrew Lunn <andrew@lunn.ch> [2019-04-28 23:36:40]:
> 
> Hi Andrew,
> 
> > > so if I understand this correctly, it probably means, that this approach with
> > > modified of_get_mac_address is dead end as current of_get_mac_address users
> > > don't expect and handle possible -EPROBE_DEFER error, so I would need to
> > > change all the current users, which is nonsense.
> > 
> > I would not say it is dead, it just needs a bit more work.
> 
> ok, that's good news, I've probably just misunderstood your concern about the
> random MAC address in case when platform/nvmem subsystem returns -EPROBE_DEFER.
> 
> > The current users should always be checking for a NULL pointer.  You
> > just need to change that to !IS_ERR(). You can then return
> > ERR_PTR(-PROBE_DEFER) from the NVMEM operation.
> 
> I'm more then happy to address this in v3, but I'm still curious, what is it
> going to change in the current state of the tree. 
> 
> My understanding of -PROBE_DEFER is, that it needs to be propagated back from
> the driver's probe callback/hook to the upper device/driver subsystem in order
> to be moved to the list of pending drivers and considered for probe later
> again. This is not going to happen in any of the current drivers, thus it will
> probably still always result in random MAC address in case of -EPROBE_DEFER
> error from the nvmem subsystem.

Hi Petr

All current drivers which don't look in NVMEM don't expect
EPROBE_DEFER. So not returning it as the result of the probe is fine.
The one driver which does expect EPROBE_DEFER already has the code to
handle it.

What you have to be careful of, is the return value from your new code
looking in NVMEM. It should only return EPROBE_DEFER, or another error
if there really is expected to be a value in NVMEM, or getting it from
NVMEM resulted in an error.

I've not looked at the details of nvmem_get_mac_address(), but it
should be a two stage process. The first is to look in device tree to
find the properties. Device tree is always accessible. So performing a
lookup will never return EPROBE_DEFER. If there are no properties, it
probably return -ENODEV. You need to consider that as not being a real
error, since these are optional properties. of_get_mac_address() needs
to try the next source of the MAC address. The second stage is to look
into the NVMEM. That could return -EPROBE_DEFER and you should return
that error, or any other error at this stage. The MAC address should
exist in NVMEM so we want to know about the error.

      Andrew

Handling of EPROBE_DEFER in of_get_mac_address [Was: Re: [PATCH v2 3/4] net: macb: Drop nvmem_get_mac_address usage]

[Petr Štetiar]

Andrew Lunn <andrew@lunn.ch> [2019-04-29 15:02:48]:

Hi Andrew,

> > My understanding of -PROBE_DEFER is, that it needs to be propagated back from
> > the driver's probe callback/hook to the upper device/driver subsystem in order
> > to be moved to the list of pending drivers and considered for probe later
> > again. This is not going to happen in any of the current drivers, thus it will
> > probably still always result in random MAC address in case of -EPROBE_DEFER
> > error from the nvmem subsystem.
> 
> All current drivers which don't look in NVMEM don't expect
> EPROBE_DEFER. 

once there's NVMEM wired in of_get_mac_address, one can simply use it, nothing
is going to stop the potential user of doing so and if EPROBE_DEFER isn't
propagated from the driver back to the upper device driver subsytem, it's
probably going to end with random MAC address in some (very rare?) cases.

So if we don't want to properly convert all current of_get_mac_address users,
and just selectively upgrade drivers which could support NVMEM (and it makes
sense for those drivers) with proper EPROBE_DEFER handling, we should probably
just extend of_get_mac_address and convert those drivers to NVMEM one by one,
as needed:

 enum of_mac_addr {
	OF_MAC_ADDR_DT = 0,
	OF_MAC_ADDR_DT_NVMEM
 };

 const void *of_get_mac_address(struct device_node *np, enum of_mac_addr type);

  (just my first rough idea, feel free to suggest something more acceptable)

> The one driver which does expect EPROBE_DEFER already has the code to
> handle it.

I've read the code in that macb driver several times and I don't see any code
which is specificaly handling the EPROBE_DEFER, so I'm probably blind or I miss
something fundamental ;-) This driver is simply forwarding that EPROBE_DEFER
error to the upper layer, nothing specific. The other one, davinci_emac simply
doesn't care about the possible EPROBE_DEFER and uses random MAC address in
case of any NVMEM error.

> What you have to be careful of, is the return value from your new code
> looking in NVMEM. It should only return EPROBE_DEFER, or another error
> if there really is expected to be a value in NVMEM, or getting it from
> NVMEM resulted in an error.

Thanks for the hint, I've created of_has_nvmem_mac_addr helper for that and it
works fine.

> I've not looked at the details of nvmem_get_mac_address(), but it
> should be a two stage process. The first is to look in device tree to
> find the properties. Device tree is always accessible. So performing a
> lookup will never return EPROBE_DEFER. If there are no properties, it
> probably return -ENODEV. You need to consider that as not being a real
> error, since these are optional properties. of_get_mac_address() needs
> to try the next source of the MAC address. The second stage is to look
> into the NVMEM. That could return -EPROBE_DEFER and you should return
> that error, or any other error at this stage. The MAC address should
> exist in NVMEM so we want to know about the error.

While looking at all current of_get_mac_address users, I've simply found out,
that it would look inconsistent and confusing to propagate back EPROBE_DEFER
just in some places, so I've bitten the bullet and converted all current
of_get_mac_address users to return EPROBE_DEFER properly (where applicable),
see bellow.

It has resulted in a bunch of commits, and as I'm not sure how it's going to be
received and to avoid sending more nonsense to a lot of lists and people, I've
made it available in my GitHub repository (just tell me that it's OK and I'll
send it as v3):

 The following changes since commit 37624b58542fb9f2d9a70e6ea006ef8a5f66c30b:

   Linux 5.1-rc7 (2019-04-28 17:04:13 -0700)

 are available in the git repository at:

   https://github.com/ynezz/linux.git upstream/nvmem-mac-address

 for you to fetch changes up to 5eb1e8131d3f97a488b74563dd8fefa068218e05:

   powerpc: tsi108: adjust for of_get_mac_address ERR_PTR encoded error value (2019-04-30 13:02:22 +0200)

 ----------------------------------------------------------------
 Petr Štetiar (20):
       of_net: add NVMEM support to of_get_mac_address
       dt-bindings: doc: reflect new NVMEM of_get_mac_address behaviour
       net: macb: drop nvmem_get_mac_address usage
       net: davinci_emac: drop nvmem_get_mac_address usage
       net: ethernet: make eth_platform_get_mac_address probe defer aware
       net: ethernet: make of_get_mac_address probe defer aware
       net: usb: make eth_platform_get_mac_address probe defer aware
       net: usb: make of_get_mac_address probe defer aware
       net: sh_eth: make of_get_mac_address probe defer aware
       net: fec: make of_get_mac_address probe defer aware
       net: fec_mpc52xx: make of_get_mac_address probe defer aware
       net: hisi_femac: make of_get_mac_address probe defer aware
       net: sky2: make of_get_mac_address probe defer aware
       net: ks8851: make of_get_mac_address probe defer aware
       wireless: ath9k: make of_get_mac_address probe defer aware
       wireless: mt76: make of_get_mac_address probe defer aware
       wireless: ralink: make of_get_mac_address probe defer aware
       staging: octeon-ethernet: make of_get_mac_address probe defer aware
       ARM: Kirkwood: adjust for of_get_mac_address ERR_PTR encoded error value
       powerpc: tsi108: adjust for of_get_mac_address ERR_PTR encoded error value

  .../devicetree/bindings/net/altera_tse.txt         |  5 +-
  Documentation/devicetree/bindings/net/amd-xgbe.txt |  5 +-
  .../devicetree/bindings/net/brcm,amac.txt          |  4 +-
  Documentation/devicetree/bindings/net/cpsw.txt     |  4 +-
  .../devicetree/bindings/net/davinci_emac.txt       |  5 +-
  Documentation/devicetree/bindings/net/dsa/dsa.txt  |  5 +-
  Documentation/devicetree/bindings/net/ethernet.txt |  6 +-
  .../devicetree/bindings/net/hisilicon-femac.txt    |  4 +-
  .../bindings/net/hisilicon-hix5hd2-gmac.txt        |  4 +-
  .../devicetree/bindings/net/keystone-netcp.txt     | 10 ++--
  Documentation/devicetree/bindings/net/macb.txt     |  5 +-
  .../devicetree/bindings/net/marvell-pxa168.txt     |  4 +-
  .../devicetree/bindings/net/microchip,enc28j60.txt |  3 +-
  .../devicetree/bindings/net/microchip,lan78xx.txt  |  5 +-
  .../devicetree/bindings/net/qca,qca7000.txt        |  4 +-
  .../devicetree/bindings/net/samsung-sxgbe.txt      |  4 +-
  .../bindings/net/snps,dwc-qos-ethernet.txt         |  5 +-
  .../bindings/net/socionext,uniphier-ave4.txt       |  4 +-
  .../devicetree/bindings/net/socionext-netsec.txt   |  5 +-
  .../bindings/net/wireless/mediatek,mt76.txt        |  5 +-
  .../devicetree/bindings/net/wireless/qca,ath9k.txt |  4 +-
  arch/arm/mach-mvebu/kirkwood.c                     |  3 +-
  arch/powerpc/sysdev/tsi108_dev.c                   |  2 +-
  drivers/net/ethernet/aeroflex/greth.c              |  5 +-
  drivers/net/ethernet/allwinner/sun4i-emac.c        |  9 +--
  drivers/net/ethernet/altera/altera_tse_main.c      |  8 ++-
  drivers/net/ethernet/arc/emac_main.c               |  9 ++-
  drivers/net/ethernet/aurora/nb8800.c               |  9 ++-
  drivers/net/ethernet/broadcom/bcmsysport.c         |  5 +-
  drivers/net/ethernet/broadcom/bgmac-bcma.c         |  9 ++-
  drivers/net/ethernet/broadcom/bgmac-platform.c     |  4 +-
  drivers/net/ethernet/broadcom/genet/bcmgenet.c     |  7 ++-
  drivers/net/ethernet/broadcom/tg3.c                | 10 +++-
  drivers/net/ethernet/cadence/macb_main.c           | 12 ++--
  drivers/net/ethernet/cavium/octeon/octeon_mgmt.c   |  9 ++-
  drivers/net/ethernet/cavium/thunder/thunder_bgx.c  |  4 +-
  drivers/net/ethernet/davicom/dm9000.c              |  4 +-
  drivers/net/ethernet/ethoc.c                       |  6 +-
  drivers/net/ethernet/ezchip/nps_enet.c             |  8 ++-
  drivers/net/ethernet/freescale/fec_main.c          | 17 ++++--
  drivers/net/ethernet/freescale/fec_mpc52xx.c       | 25 +++++----
  drivers/net/ethernet/freescale/fman/mac.c          |  5 +-
  .../net/ethernet/freescale/fs_enet/fs_enet-main.c  |  6 +-
  drivers/net/ethernet/freescale/gianfar.c           |  7 ++-
  drivers/net/ethernet/freescale/ucc_geth.c          |  6 +-
  drivers/net/ethernet/hisilicon/hisi_femac.c        | 21 ++++---
  drivers/net/ethernet/hisilicon/hix5hd2_gmac.c      |  7 ++-
  drivers/net/ethernet/intel/i40e/i40e_main.c        | 15 ++++-
  drivers/net/ethernet/intel/igb/igb_main.c          |  5 +-
  drivers/net/ethernet/intel/igc/igc_main.c          |  5 +-
  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c      |  6 +-
  drivers/net/ethernet/lantiq_xrx200.c               |  4 +-
  drivers/net/ethernet/marvell/mv643xx_eth.c         |  4 +-
  drivers/net/ethernet/marvell/mvneta.c              |  5 +-
  drivers/net/ethernet/marvell/pxa168_eth.c          |  9 ++-
  drivers/net/ethernet/marvell/sky2.c                | 14 +++--
  drivers/net/ethernet/mediatek/mtk_eth_soc.c        |  8 +--
  drivers/net/ethernet/micrel/ks8851.c               | 15 +++--
  drivers/net/ethernet/micrel/ks8851_mll.c           |  6 +-
  drivers/net/ethernet/microchip/enc28j60.c          |  8 ++-
  drivers/net/ethernet/nxp/lpc_eth.c                 | 10 +++-
  drivers/net/ethernet/qualcomm/qca_spi.c            |  9 +--
  drivers/net/ethernet/qualcomm/qca_uart.c           |  9 +--
  drivers/net/ethernet/realtek/r8169.c               |  4 +-
  drivers/net/ethernet/renesas/ravb_main.c           | 11 +++-
  drivers/net/ethernet/renesas/sh_eth.c              | 15 ++---
  .../net/ethernet/samsung/sxgbe/sxgbe_platform.c    |  7 ++-
  drivers/net/ethernet/socionext/sni_ave.c           |  9 +--
  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  2 +-
  .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  3 +
  drivers/net/ethernet/ti/cpsw.c                     |  4 +-
  drivers/net/ethernet/ti/davinci_emac.c             | 25 ++++-----
  drivers/net/ethernet/ti/netcp_core.c               |  8 ++-
  drivers/net/ethernet/wiznet/w5100-spi.c            |  6 +-
  drivers/net/ethernet/wiznet/w5100.c                |  2 +-
  drivers/net/ethernet/xilinx/ll_temac_main.c        |  5 +-
  drivers/net/ethernet/xilinx/xilinx_axienet_main.c  |  4 +-
  drivers/net/ethernet/xilinx/xilinx_emaclite.c      |  7 ++-
  drivers/net/usb/asix_devices.c                     |  7 ++-
  drivers/net/usb/lan78xx.c                          | 13 ++++-
  drivers/net/usb/smsc75xx.c                         | 16 ++++--
  drivers/net/usb/smsc95xx.c                         | 16 ++++--
  drivers/net/wireless/ath/ath9k/init.c              |  4 +-
  drivers/net/wireless/mediatek/mt76/eeprom.c        | 10 +++-
  drivers/net/wireless/mediatek/mt76/mt76.h          |  2 +-
  drivers/net/wireless/mediatek/mt76/mt7603/eeprom.c |  4 +-
  drivers/net/wireless/mediatek/mt76/mt76x2/eeprom.c |  6 +-
  drivers/net/wireless/ralink/rt2x00/rt2400pci.c     |  5 +-
  drivers/net/wireless/ralink/rt2x00/rt2500pci.c     |  5 +-
  drivers/net/wireless/ralink/rt2x00/rt2500usb.c     |  5 +-
  drivers/net/wireless/ralink/rt2x00/rt2800lib.c     |  4 +-
  drivers/net/wireless/ralink/rt2x00/rt2x00.h        |  3 +-
  drivers/net/wireless/ralink/rt2x00/rt2x00dev.c     | 16 ++++--
  drivers/net/wireless/ralink/rt2x00/rt61pci.c       |  5 +-
  drivers/net/wireless/ralink/rt2x00/rt73usb.c       |  5 +-
  drivers/of/of_net.c                                | 64 +++++++++++++++++++++-
  drivers/staging/octeon/ethernet.c                  |  7 ++-
  net/ethernet/eth.c                                 |  8 ++-
  98 files changed, 527 insertions(+), 239 deletions(-)

So I'm wondering, is this (probably) proper handling of EPROBE_DEFER overkill?

Or should I really just convert all current consumers of of_get_mac_address to
IS_ERR_OR_NULL() macro check (as you've suggested) and call it a day? 

Or should I simply change of_get_mac_address so it would allow for progressive
conversion of drivers using of_get_mac_address to NVMEM?

Thanks!

-- ynezz

Re: Handling of EPROBE_DEFER in of_get_mac_address [Was: Re: [PATCH v2 3/4] net: macb: Drop nvmem_get_mac_address usage]

[Andrew Lunn]

On Tue, Apr 30, 2019 at 04:13:35PM +0200, Petr Štetiar wrote:
> Andrew Lunn <andrew@lunn.ch> [2019-04-29 15:02:48]:
> 
> Hi Andrew,
> 
> > > My understanding of -PROBE_DEFER is, that it needs to be propagated back from
> > > the driver's probe callback/hook to the upper device/driver subsystem in order
> > > to be moved to the list of pending drivers and considered for probe later
> > > again. This is not going to happen in any of the current drivers, thus it will
> > > probably still always result in random MAC address in case of -EPROBE_DEFER
> > > error from the nvmem subsystem.
> > 
> > All current drivers which don't look in NVMEM don't expect
> > EPROBE_DEFER. 
> 
> once there's NVMEM wired in of_get_mac_address, one can simply use it, nothing
> is going to stop the potential user of doing so and if EPROBE_DEFER isn't
> propagated from the driver back to the upper device driver subsytem, it's
> probably going to end with random MAC address in some (very rare?) cases.

Hi Petr

There is no simple answer here. If we add EPROBE_DEFER support to all
the current drivers using of_get_mac_address(), we are likely to break
something. Regressions are bad. If somebody does add NVMEM properties
to a device which does not currently have them, and it fails, that it
just bad testing, not a regressions.

So i would keep it KISS. Allow of_get_mac_address() to return an
error, but don't modify current drivers to look for it.

       Andrew

[PATCH v2 4/4] net: davinci_emac: Drop nvmem_get_mac_address usage

[Petr Štetiar]

of_get_mac_address now uses NVMEM under the hood, so it's not necessary
to call it manually anymore.

Signed-off-by: Petr Štetiar <ynezz@true.cz>
---
 drivers/net/ethernet/ti/davinci_emac.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index 57450b1..c1a5526 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1912,15 +1912,11 @@ static int davinci_emac_probe(struct platform_device *pdev)
 		ether_addr_copy(ndev->dev_addr, priv->mac_addr);
 
 	if (!is_valid_ether_addr(priv->mac_addr)) {
-		/* Try nvmem if MAC wasn't passed over pdata or DT. */
-		rc = nvmem_get_mac_address(&pdev->dev, priv->mac_addr);
-		if (rc) {
-			/* Use random MAC if still none obtained. */
-			eth_hw_addr_random(ndev);
-			memcpy(priv->mac_addr, ndev->dev_addr, ndev->addr_len);
-			dev_warn(&pdev->dev, "using random MAC addr: %pM\n",
-				 priv->mac_addr);
-		}
+		/* Use random MAC if still none obtained. */
+		eth_hw_addr_random(ndev);
+		memcpy(priv->mac_addr, ndev->dev_addr, ndev->addr_len);
+		dev_warn(&pdev->dev, "using random MAC addr: %pM\n",
+			 priv->mac_addr);
 	}
 
 	ndev->netdev_ops = &emac_netdev_ops;
-- 
1.9.1

Re: [PATCH v2 4/4] net: davinci_emac: Drop nvmem_get_mac_address usage

[Andrew Lunn]

On Sun, Apr 28, 2019 at 02:53:22PM +0200, Petr Štetiar wrote:
> of_get_mac_address now uses NVMEM under the hood, so it's not necessary
> to call it manually anymore.
> 
> Signed-off-by: Petr Štetiar <ynezz@true.cz>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew