[PATCH] of_net: Implement of_get_nvmem_mac_address helper

[PATCH] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

It's common practice to store MAC addresses for network interfaces into
nvmem devices. However the code to actually do this in the kernel lacks,
so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
address from an nvmem cell provider.

This is particulary useful on devices where the ethernet interface cannot
be configured by the bootloader, for example because it's in an FPGA.

Tested by adapting the cadence macb driver to call this instead of
of_get_mac_address().

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 drivers/of/of_net.c    | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_net.h |  6 ++++++
 2 files changed, 54 insertions(+)

diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index d820f3e..316a537 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -7,6 +7,7 @@
  */
 #include <linux/etherdevice.h>
 #include <linux/kernel.h>
+#include <linux/nvmem-consumer.h>
 #include <linux/of_net.h>
 #include <linux/phy.h>
 #include <linux/export.h>
@@ -80,3 +81,50 @@ const void *of_get_mac_address(struct device_node *np)
 	return of_get_mac_addr(np, "address");
 }
 EXPORT_SYMBOL(of_get_mac_address);
+
+/**
+ * Search the device tree for a MAC address, by calling of_get_mac_address
+ * and if that doesn't provide an address, fetch it from an nvmem provider
+ * using the name 'mac-address'.
+ * On success, copies the new address is into memory pointed to by addr and
+ * returns 0. Returns a negative error code otherwise.
+ * @dev:	Pointer to the device containing the device_node
+ * @addr:	Pointer to receive the MAC address using ether_addr_copy()
+ */
+int of_get_nvmem_mac_address(struct device *dev, char *addr)
+{
+	const char *mac;
+	struct nvmem_cell *cell;
+	size_t len;
+	int ret;
+
+	mac = of_get_mac_address(dev->of_node);
+	if (mac) {
+		ether_addr_copy(addr, mac);
+		return 0;
+	}
+
+	cell = nvmem_cell_get(dev, "mac-address");
+	if (IS_ERR(cell))
+		return PTR_ERR(cell);
+
+	mac = (const char *)nvmem_cell_read(cell, &len);
+
+	nvmem_cell_put(cell);
+
+	if (IS_ERR(mac))
+		return PTR_ERR(mac);
+
+	if (len < 6 || !is_valid_ether_addr(mac)) {
+		dev_err(dev, "MAC address from NVMEM is invalid\n");
+		ret = -EINVAL;
+	} else {
+		ether_addr_copy(addr, mac);
+		ret = 0;
+	}
+
+	kfree(mac);
+
+	return ret;
+}
+EXPORT_SYMBOL(of_get_nvmem_mac_address);
diff --git a/include/linux/of_net.h b/include/linux/of_net.h
index 9cd72aa..0d52e1d 100644
--- a/include/linux/of_net.h
+++ b/include/linux/of_net.h
@@ -13,6 +13,7 @@
 struct net_device;
 extern int of_get_phy_mode(struct device_node *np);
 extern const void *of_get_mac_address(struct device_node *np);
+extern int of_get_nvmem_mac_address(struct device *dev, char *addr);
 extern struct net_device *of_find_net_device_by_node(struct device_node *np);
 #else
 static inline int of_get_phy_mode(struct device_node *np)
@@ -25,6 +26,11 @@ static inline const void *of_get_mac_address(struct device_node *np)
 	return NULL;
 }
 
+static inline int of_get_nvmem_mac_address(struct device *dev, char *addr)
+{
+	return -ENODEV;
+}
+
 static inline struct net_device *of_find_net_device_by_node(struct device_node *np)
 {
 	return NULL;
-- 
1.9.1

Re: [PATCH] of_net: Implement of_get_nvmem_mac_address helper

[Andrew Lunn]

On Fri, Mar 23, 2018 at 03:24:34PM +0100, Mike Looijmans wrote:
> It's common practice to store MAC addresses for network interfaces into
> nvmem devices. However the code to actually do this in the kernel lacks,
> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
> address from an nvmem cell provider.
> 
> This is particulary useful on devices where the ethernet interface cannot
> be configured by the bootloader, for example because it's in an FPGA.
> 
> Tested by adapting the cadence macb driver to call this instead of
> of_get_mac_address().

Hi Mike

Please can you document the device tree binding. I assume you are
adding a nvmen-cells and nvmem-cell-names to the Ethernet node in
device tree.

> +/**
> + * Search the device tree for a MAC address, by calling of_get_mac_address
> + * and if that doesn't provide an address, fetch it from an nvmem provider
> + * using the name 'mac-address'.
> + * On success, copies the new address is into memory pointed to by addr and
> + * returns 0. Returns a negative error code otherwise.
> + * @dev:	Pointer to the device containing the device_node
> + * @addr:	Pointer to receive the MAC address using ether_addr_copy()
> + */
> +int of_get_nvmem_mac_address(struct device *dev, char *addr)
> +{
> +	const char *mac;
> +	struct nvmem_cell *cell;
> +	size_t len;
> +	int ret;
> +
> +	mac = of_get_mac_address(dev->of_node);
> +	if (mac) {
> +		ether_addr_copy(addr, mac);
> +		return 0;
> +	}

Is there a need to add a new API? Could of_get_mac_address() be
extended to look in NVMEM? The MAC driver does not care. It is saying,
using OF get me a MAC address. One API seems sufficient, and would
mean you don't need to change the MAC drivers.

     Andrew

Re: [PATCH] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

On 23-3-2018 16:11, Andrew Lunn wrote:
> On Fri, Mar 23, 2018 at 03:24:34PM +0100, Mike Looijmans wrote:
>> It's common practice to store MAC addresses for network interfaces into
>> nvmem devices. However the code to actually do this in the kernel lacks,
>> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
>> address from an nvmem cell provider.
>>
>> This is particulary useful on devices where the ethernet interface cannot
>> be configured by the bootloader, for example because it's in an FPGA.
>>
>> Tested by adapting the cadence macb driver to call this instead of
>> of_get_mac_address().
>
> Hi Mike
>
> Please can you document the device tree binding. I assume you are
> adding a nvmen-cells and nvmem-cell-names to the Ethernet node in
> device tree.

Indeed. I'll add my settings as an example. Where should I put this 
documentation, in the commit comment or somewhere in 
Documents/devicetree/bindings?

>> +/**
>> + * Search the device tree for a MAC address, by calling of_get_mac_address
>> + * and if that doesn't provide an address, fetch it from an nvmem provider
>> + * using the name 'mac-address'.
>> + * On success, copies the new address is into memory pointed to by addr and
>> + * returns 0. Returns a negative error code otherwise.
>> + * @dev:	Pointer to the device containing the device_node
>> + * @addr:	Pointer to receive the MAC address using ether_addr_copy()
>> + */
>> +int of_get_nvmem_mac_address(struct device *dev, char *addr)
>> +{
>> +	const char *mac;
>> +	struct nvmem_cell *cell;
>> +	size_t len;
>> +	int ret;
>> +
>> +	mac = of_get_mac_address(dev->of_node);
>> +	if (mac) {
>> +		ether_addr_copy(addr, mac);
>> +		return 0;
>> +	}
>
> Is there a need to add a new API? Could of_get_mac_address() be
> extended to look in NVMEM? The MAC driver does not care. It is saying,
> using OF get me a MAC address. One API seems sufficient, and would
> mean you don't need to change the MAC drivers.

It's what I intended to do, but there were two problems with that:
- of_get_mac_address() returns a pointer to constant data in memory, but 
the nvmem functions return an allocated memory object that must be freed 
after use. This changes the way the call is to be made.
- The nvmem functions need the "struct device" pointer as well, while 
of_get_mac_address() only gets passed the DT node.

One approach would be to deprecate the of_get_mac_address() interface 
and migrate existing drivers to the of_get_nvmem_mac_address() interface.


-- 
Mike Looijmans

Re: [PATCH] of_net: Implement of_get_nvmem_mac_address helper

[Florian Fainelli]

On 03/23/2018 12:20 PM, Mike Looijmans wrote:
> On 23-3-2018 16:11, Andrew Lunn wrote:
>> On Fri, Mar 23, 2018 at 03:24:34PM +0100, Mike Looijmans wrote:
>>> It's common practice to store MAC addresses for network interfaces into
>>> nvmem devices. However the code to actually do this in the kernel lacks,
>>> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
>>> address from an nvmem cell provider.
>>>
>>> This is particulary useful on devices where the ethernet interface
>>> cannot
>>> be configured by the bootloader, for example because it's in an FPGA.
>>>
>>> Tested by adapting the cadence macb driver to call this instead of
>>> of_get_mac_address().
>>
>> Hi Mike
>>
>> Please can you document the device tree binding. I assume you are
>> adding a nvmen-cells and nvmem-cell-names to the Ethernet node in
>> device tree.
> 
> Indeed. I'll add my settings as an example. Where should I put this
> documentation, in the commit comment or somewhere in
> Documents/devicetree/bindings?
> 
>>> +/**
>>> + * Search the device tree for a MAC address, by calling
>>> of_get_mac_address
>>> + * and if that doesn't provide an address, fetch it from an nvmem
>>> provider
>>> + * using the name 'mac-address'.
>>> + * On success, copies the new address is into memory pointed to by
>>> addr and
>>> + * returns 0. Returns a negative error code otherwise.
>>> + * @dev:    Pointer to the device containing the device_node
>>> + * @addr:    Pointer to receive the MAC address using ether_addr_copy()
>>> + */
>>> +int of_get_nvmem_mac_address(struct device *dev, char *addr)
>>> +{
>>> +    const char *mac;
>>> +    struct nvmem_cell *cell;
>>> +    size_t len;
>>> +    int ret;
>>> +
>>> +    mac = of_get_mac_address(dev->of_node);
>>> +    if (mac) {
>>> +        ether_addr_copy(addr, mac);
>>> +        return 0;
>>> +    }
>>
>> Is there a need to add a new API? Could of_get_mac_address() be
>> extended to look in NVMEM? The MAC driver does not care. It is saying,
>> using OF get me a MAC address. One API seems sufficient, and would
>> mean you don't need to change the MAC drivers.
> 
> It's what I intended to do, but there were two problems with that:
> - of_get_mac_address() returns a pointer to constant data in memory, but
> the nvmem functions return an allocated memory object that must be freed
> after use. This changes the way the call is to be made.

Yeah...

> - The nvmem functions need the "struct device" pointer as well, while
> of_get_mac_address() only gets passed the DT node.

Bummer, you can't assume there is always a struct device associated with
a struct device_node. Also, bigger question is, how can we make this
work, for e.g: ACPI systems and therefore use an abstract fw_node handle?

> 
> One approach would be to deprecate the of_get_mac_address() interface
> and migrate existing drivers to the of_get_nvmem_mac_address() interface.

Humm maybe, but clearly making of_get_mac_address() look for a nvmem is
less error prone and does not require people to opt-in for the new
helper, that seems beneficial to me.
-- 
Florian

Re: [PATCH] of_net: Implement of_get_nvmem_mac_address helper

[Andrew Lunn]

> Indeed. I'll add my settings as an example. Where should I put this
> documentation, in the commit comment or somewhere in
> Documents/devicetree/bindings?

Documention/devicetree/bindings/net/ethernet.txt

> It's what I intended to do, but there were two problems with that:
> - of_get_mac_address() returns a pointer to constant data in memory, but the
> nvmem functions return an allocated memory object that must be freed after
> use. This changes the way the call is to be made.
> - The nvmem functions need the "struct device" pointer as well, while
> of_get_mac_address() only gets passed the DT node.

Does of_nvmem_cell_get() help?

      Andrew

Re: [PATCH] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

On 23-03-18 20:42, Andrew Lunn wrote:
>> Indeed. I'll add my settings as an example. Where should I put this
>> documentation, in the commit comment or somewhere in
>> Documents/devicetree/bindings?
> 
> Documention/devicetree/bindings/net/ethernet.txt

Ok

>> It's what I intended to do, but there were two problems with that:
>> - of_get_mac_address() returns a pointer to constant data in memory, but the
>> nvmem functions return an allocated memory object that must be freed after
>> use. This changes the way the call is to be made.
>> - The nvmem functions need the "struct device" pointer as well, while
>> of_get_mac_address() only gets passed the DT node.
> 
> Does of_nvmem_cell_get() help?

Yes, looking at the interface, it would.

-- 
Mike Looijmans

Re: [PATCH] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

On 23-03-18 20:33, Florian Fainelli wrote:
> On 03/23/2018 12:20 PM, Mike Looijmans wrote:
>> On 23-3-2018 16:11, Andrew Lunn wrote:
>>> On Fri, Mar 23, 2018 at 03:24:34PM +0100, Mike Looijmans wrote:
>>>> It's common practice to store MAC addresses for network interfaces into
>>>> nvmem devices. However the code to actually do this in the kernel lacks,
>>>> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
>>>> address from an nvmem cell provider.
>>>>
>>>> This is particulary useful on devices where the ethernet interface
>>>> cannot
>>>> be configured by the bootloader, for example because it's in an FPGA.
>>>>
>>>> Tested by adapting the cadence macb driver to call this instead of
>>>> of_get_mac_address().
>>>
>>> Hi Mike
>>>
>>> Please can you document the device tree binding. I assume you are
>>> adding a nvmen-cells and nvmem-cell-names to the Ethernet node in
>>> device tree.
>>
>> Indeed. I'll add my settings as an example. Where should I put this
>> documentation, in the commit comment or somewhere in
>> Documents/devicetree/bindings?
>>
>>>> +/**
>>>> + * Search the device tree for a MAC address, by calling
>>>> of_get_mac_address
>>>> + * and if that doesn't provide an address, fetch it from an nvmem
>>>> provider
>>>> + * using the name 'mac-address'.
>>>> + * On success, copies the new address is into memory pointed to by
>>>> addr and
>>>> + * returns 0. Returns a negative error code otherwise.
>>>> + * @dev:    Pointer to the device containing the device_node
>>>> + * @addr:    Pointer to receive the MAC address using ether_addr_copy()
>>>> + */
>>>> +int of_get_nvmem_mac_address(struct device *dev, char *addr)
>>>> +{
>>>> +    const char *mac;
>>>> +    struct nvmem_cell *cell;
>>>> +    size_t len;
>>>> +    int ret;
>>>> +
>>>> +    mac = of_get_mac_address(dev->of_node);
>>>> +    if (mac) {
>>>> +        ether_addr_copy(addr, mac);
>>>> +        return 0;
>>>> +    }
>>>
>>> Is there a need to add a new API? Could of_get_mac_address() be
>>> extended to look in NVMEM? The MAC driver does not care. It is saying,
>>> using OF get me a MAC address. One API seems sufficient, and would
>>> mean you don't need to change the MAC drivers.
>>
>> It's what I intended to do, but there were two problems with that:
>> - of_get_mac_address() returns a pointer to constant data in memory, but
>> the nvmem functions return an allocated memory object that must be freed
>> after use. This changes the way the call is to be made.
> 
> Yeah...
> 
>> - The nvmem functions need the "struct device" pointer as well, while
>> of_get_mac_address() only gets passed the DT node.
> 
> Bummer, you can't assume there is always a struct device associated with
> a struct device_node. Also, bigger question is, how can we make this
> work, for e.g: ACPI systems and therefore use an abstract fw_node handle?
> 

Andrew Lunn's suggestion of using "of_nvmem_cell_get()" should solve this.

>> One approach would be to deprecate the of_get_mac_address() interface
>> and migrate existing drivers to the of_get_nvmem_mac_address() interface.
> 
> Humm maybe, but clearly making of_get_mac_address() look for a nvmem is
> less error prone and does not require people to opt-in for the new
> helper, that seems beneficial to me.

Totally agree. But I can't think of a way that doesn't change the 
method's signature. At some point the allocated nvmem buffer must be freed.

A quick survey for the of_get_mac_address users learns that most of them 
do a memcpy (or similar) right after it, so for these drivers the 
"of_get_nvmem_mac_address" style signature that performs the memcpy (or 
better, ether_addr_copy) is a better fit, e.g.:

int of_get_mac_address(struct device_node *np, void *addr)


-- 
Mike Looijmans

Re: [PATCH] of_net: Implement of_get_nvmem_mac_address helper

[Andrew Lunn]

> A quick survey for the of_get_mac_address users learns that most of them do
> a memcpy (or similar) right after it, so for these drivers the
> "of_get_nvmem_mac_address" style signature that performs the memcpy (or
> better, ether_addr_copy) is a better fit, e.g.:
> 
> int of_get_mac_address(struct device_node *np, void *addr)

Hi Mike

This is a nicer solution, but it is quite a lot of work, there are a
lot of users. Maybe Coccinelle can help?

    Andrew

Re: [PATCH] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

On 24-03-18 19:53, Andrew Lunn wrote:
>> A quick survey for the of_get_mac_address users learns that most of them do
>> a memcpy (or similar) right after it, so for these drivers the
>> "of_get_nvmem_mac_address" style signature that performs the memcpy (or
>> better, ether_addr_copy) is a better fit, e.g.:
>>
>> int of_get_mac_address(struct device_node *np, void *addr)
> 
> Hi Mike
> 
> This is a nicer solution, but it is quite a lot of work, there are a
> lot of users. Maybe Coccinelle can help?

About 58 of them, yeah. And this looked like such a simple thing when I 
started it...
https://elixir.bootlin.com/linux/v4.16-rc6/ident/of_get_mac_address
I have no experience with Coccinelle though.



-- 
Mike Looijmans

Re: [PATCH] of_net: Implement of_get_nvmem_mac_address helper

[Andrew Lunn]

> I have no experience with Coccinelle though.

Hi Mike

I've very little either. But all the interactions i've had with
Coccinelle people have been very friendly and helpful. It could be, if
you can describe in words what you need help with, they can write the
script to do it.

       Andrew

[PATCH v2] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

It's common practice to store MAC addresses for network interfaces into
nvmem devices. However the code to actually do this in the kernel lacks,
so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
address from an nvmem cell provider.

This is particulary useful on devices where the ethernet interface cannot
be configured by the bootloader, for example because it's in an FPGA.

Tested by adapting the cadence macb driver to call this instead of
of_get_mac_address().

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
v2: Use of_nvmem_cell_get to avoid needing the assiciated device
    Use void* instead of char*
    Add devicetree binding doc
 Documentation/devicetree/bindings/net/ethernet.txt |  2 +
 drivers/of/of_net.c                                | 47 ++++++++++++++++++++++
 include/linux/of_net.h                             |  6 +++
 3 files changed, 55 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
index 2974e63..cfc376b 100644
--- a/Documentation/devicetree/bindings/net/ethernet.txt
+++ b/Documentation/devicetree/bindings/net/ethernet.txt
@@ -10,6 +10,8 @@ Documentation/devicetree/bindings/phy/phy-bindings.txt.
   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;
+- 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/drivers/of/of_net.c b/drivers/of/of_net.c
index d820f3e..8999745 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -7,6 +7,7 @@
  */
 #include <linux/etherdevice.h>
 #include <linux/kernel.h>
+#include <linux/nvmem-consumer.h>
 #include <linux/of_net.h>
 #include <linux/phy.h>
 #include <linux/export.h>
@@ -80,3 +81,49 @@ const void *of_get_mac_address(struct device_node *np)
 	return of_get_mac_addr(np, "address");
 }
 EXPORT_SYMBOL(of_get_mac_address);
+
+/**
+ * Search the device tree for a MAC address, by calling of_get_mac_address
+ * and if that doesn't provide an address, fetch it from an nvmem provider
+ * using the name 'mac-address'.
+ * On success, copies the new address is into memory pointed to by addr and
+ * returns 0. Returns a negative error code otherwise.
+ * @dev:	Pointer to the device containing the device_node
+ * @addr:	Pointer to receive the MAC address using ether_addr_copy()
+ */
+int of_get_nvmem_mac_address(struct device_node *np, void *addr)
+{
+	const void *mac;
+	struct nvmem_cell *cell;
+	size_t len;
+	int ret;
+
+	mac = of_get_mac_address(np);
+	if (mac) {
+		ether_addr_copy(addr, mac);
+		return 0;
+	}
+
+	cell = of_nvmem_cell_get(np, "mac-address");
+	if (IS_ERR(cell))
+		return PTR_ERR(cell);
+
+	mac = nvmem_cell_read(cell, &len);
+
+	nvmem_cell_put(cell);
+
+	if (IS_ERR(mac))
+		return PTR_ERR(mac);
+
+	if (len < 6 || !is_valid_ether_addr(mac)) {
+		ret = -EINVAL;
+	} else {
+		ether_addr_copy(addr, mac);
+		ret = 0;
+	}
+
+	kfree(mac);
+
+	return ret;
+}
+EXPORT_SYMBOL(of_get_nvmem_mac_address);
diff --git a/include/linux/of_net.h b/include/linux/of_net.h
index 9cd72aa..90d81ee 100644
--- a/include/linux/of_net.h
+++ b/include/linux/of_net.h
@@ -13,6 +13,7 @@
 struct net_device;
 extern int of_get_phy_mode(struct device_node *np);
 extern const void *of_get_mac_address(struct device_node *np);
+extern int of_get_nvmem_mac_address(struct device_node *np, void *addr);
 extern struct net_device *of_find_net_device_by_node(struct device_node *np);
 #else
 static inline int of_get_phy_mode(struct device_node *np)
@@ -25,6 +26,11 @@ static inline const void *of_get_mac_address(struct device_node *np)
 	return NULL;
 }
 
+static inline int of_get_nvmem_mac_address(struct device_node *np, void *addr)
+{
+	return -ENODEV;
+}
+
 static inline struct net_device *of_find_net_device_by_node(struct device_node *np)
 {
 	return NULL;
-- 
1.9.1

Re: [PATCH] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

On 25-03-18 10:17, Mike Looijmans wrote:
> On 24-03-18 19:53, Andrew Lunn wrote:
>>> A quick survey for the of_get_mac_address users learns that most of them do
>>> a memcpy (or similar) right after it, so for these drivers the
>>> "of_get_nvmem_mac_address" style signature that performs the memcpy (or
>>> better, ether_addr_copy) is a better fit, e.g.:
>>>
>>> int of_get_mac_address(struct device_node *np, void *addr)
>>
>> Hi Mike
>>
>> This is a nicer solution, but it is quite a lot of work, there are a
>> lot of users. Maybe Coccinelle can help?
> 
> About 58 of them, yeah. And this looked like such a simple thing when I 
> started it...
> https://elixir.bootlin.com/linux/v4.16-rc6/ident/of_get_mac_address
> I have no experience with Coccinelle though.
> 

I'll at least post a v2 patch with what we've discussed so far.

Another thing that just occurred to me is that the nvmem interface would also 
allow a MAC address to be written to NV storage. Haven't looked at that path, 
but it would be nice to be able to permanently program the MAC address using 
standard interfaces.


Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

Re: [PATCH v2] of_net: Implement of_get_nvmem_mac_address helper

[Andrew Lunn]

On Mon, Mar 26, 2018 at 08:41:29AM +0200, Mike Looijmans wrote:
> It's common practice to store MAC addresses for network interfaces into
> nvmem devices. However the code to actually do this in the kernel lacks,
> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
> address from an nvmem cell provider.
> 
> This is particulary useful on devices where the ethernet interface cannot
> be configured by the bootloader, for example because it's in an FPGA.
> 
> Tested by adapting the cadence macb driver to call this instead of
> of_get_mac_address().

Hi Mike

I can understand you not wanting to modify all the call sites for
of_get_mac_address().

However, the name of_get_nvmem_mac_address() suggests it gets the MAC
address from NVMEM. I think people are going to be surprised when they
find it first tries for a MAC address directly in device tree. I would
drop the call to of_get_mac_address(), and have the MAC driver call
both.

You could also maybe take a look at fwnode_get_mac_address(). It
should work for both OF and ACPI. It fits better because is passes a
char * for the address. You could make that do both, and call it from
the macb driver. dev_fwnode() probably does what you want.

    Andrew

Re: [PATCH v2] of_net: Implement of_get_nvmem_mac_address helper

[David Miller]

From: Mike Looijmans <mike.looijmans@topic.nl>
Date: Mon, 26 Mar 2018 08:41:29 +0200

> It's common practice to store MAC addresses for network interfaces into
> nvmem devices. However the code to actually do this in the kernel lacks,
> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
> address from an nvmem cell provider.
> 
> This is particulary useful on devices where the ethernet interface cannot
> be configured by the bootloader, for example because it's in an FPGA.
> 
> Tested by adapting the cadence macb driver to call this instead of
> of_get_mac_address().
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> v2: Use of_nvmem_cell_get to avoid needing the assiciated device
>     Use void* instead of char*
>     Add devicetree binding doc

Like Andrew, I think you should add a new interface for getting the MAC
address from nvmem.

And drivers can call both of them if they want OF device tree and
NVMEM probing for MAC addresses.

Later you can add a consolidated interface, if necessary, which does
both and also take a reference to the MAC address buffer of the driver
in order to deal with the resource allocation issues.

Thanks.

Re: [PATCH v2] of_net: Implement of_get_nvmem_mac_address helper

[Florian Fainelli]

On 03/26/2018 09:58 AM, David Miller wrote:
> From: Mike Looijmans <mike.looijmans@topic.nl>
> Date: Mon, 26 Mar 2018 08:41:29 +0200
> 
>> It's common practice to store MAC addresses for network interfaces into
>> nvmem devices. However the code to actually do this in the kernel lacks,
>> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
>> address from an nvmem cell provider.
>>
>> This is particulary useful on devices where the ethernet interface cannot
>> be configured by the bootloader, for example because it's in an FPGA.
>>
>> Tested by adapting the cadence macb driver to call this instead of
>> of_get_mac_address().
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>> ---
>> v2: Use of_nvmem_cell_get to avoid needing the assiciated device
>>     Use void* instead of char*
>>     Add devicetree binding doc
> 
> Like Andrew, I think you should add a new interface for getting the MAC
> address from nvmem.
> 
> And drivers can call both of them if they want OF device tree and
> NVMEM probing for MAC addresses.
> 
> Later you can add a consolidated interface, if necessary, which does
> both and also take a reference to the MAC address buffer of the driver
> in order to deal with the resource allocation issues.

Agreed, also, how does this fit with Alban's patch series here:

https://lkml.org/lkml/2018/3/24/312

do you depend on those changes at all?
-- 
Florian

Re: [PATCH v2] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

On 26-03-18 19:05, Florian Fainelli wrote:
> On 03/26/2018 09:58 AM, David Miller wrote:
>> From: Mike Looijmans <mike.looijmans@topic.nl>
>> Date: Mon, 26 Mar 2018 08:41:29 +0200
>>
>>> It's common practice to store MAC addresses for network interfaces into
>>> nvmem devices. However the code to actually do this in the kernel lacks,
>>> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
>>> address from an nvmem cell provider.
>>>
>>> This is particulary useful on devices where the ethernet interface cannot
>>> be configured by the bootloader, for example because it's in an FPGA.
>>>
>>> Tested by adapting the cadence macb driver to call this instead of
>>> of_get_mac_address().
>>>
>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>> ---
>>> v2: Use of_nvmem_cell_get to avoid needing the assiciated device
>>>      Use void* instead of char*
>>>      Add devicetree binding doc
>>
>> Like Andrew, I think you should add a new interface for getting the MAC
>> address from nvmem.
>>
>> And drivers can call both of them if they want OF device tree and
>> NVMEM probing for MAC addresses.
>>
>> Later you can add a consolidated interface, if necessary, which does
>> both and also take a reference to the MAC address buffer of the driver
>> in order to deal with the resource allocation issues.
> 
> Agreed, also, how does this fit with Alban's patch series here:

Ok, makes sense. I'll cook up a v3.

> 
> https://lkml.org/lkml/2018/3/24/312
> 
> do you depend on those changes at all?
> 

As far as I can tell, there's no dependency, I'm adding an nvmem 
"consumer" while Alban's patch adds a "provider". I'm mainly interested 
in storing the MAC address into an I2C EEPROM, which you can also buy 
with a pre-programmed MAC address in the first 6 bytes, so there's no 
production cost for managing that.


Alban's patch might some day collide with my idea of making hardware 
that has some unique ID into nvmem providers, like 1-wire chips, some 
NOR flash chips (there's the conflict) and various other devices. That 
would allow a board to obtain a random yet constant MAC address without 
any additional hardware. I'll cross that bridge when I find it.


-- 
Mike Looijmans

Re: [PATCH v2] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

On 26-03-18 17:50, Andrew Lunn wrote:
> On Mon, Mar 26, 2018 at 08:41:29AM +0200, Mike Looijmans wrote:
>> It's common practice to store MAC addresses for network interfaces into
>> nvmem devices. However the code to actually do this in the kernel lacks,
>> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
>> address from an nvmem cell provider.
>>
>> This is particulary useful on devices where the ethernet interface cannot
>> be configured by the bootloader, for example because it's in an FPGA.
>>
>> Tested by adapting the cadence macb driver to call this instead of
>> of_get_mac_address().
> 
> Hi Mike
> 
> I can understand you not wanting to modify all the call sites for
> of_get_mac_address().
> 
> However, the name of_get_nvmem_mac_address() suggests it gets the MAC
> address from NVMEM. I think people are going to be surprised when they
> find it first tries for a MAC address directly in device tree. I would
> drop the call to of_get_mac_address(), and have the MAC driver call
> both.
> 
> You could also maybe take a look at fwnode_get_mac_address(). It
> should work for both OF and ACPI. It fits better because is passes a
> char * for the address. You could make that do both, and call it from
> the macb driver. dev_fwnode() probably does what you want.

fwnode_get_mac_address looks really new, there's only one user so far. 
Is it the intention that all drivers eventually migrate to that?

(It also means I cannot backport it to the kernel I'm actually using, 
'cause I haven't got the Zynq to work with the mainline macb driver yet. 
But that's just my problem...)

-- 
Mike Looijmans

Re: [PATCH v2] of_net: Implement of_get_nvmem_mac_address helper

[Andrew Lunn]

> fwnode_get_mac_address looks really new, there's only one user so far. Is it
> the intention that all drivers eventually migrate to that?

Hi Mike

Probably not. But any driver which needs to work with both ACPI and OF
is likely to use this API. So server class ARM64 chips for example.

   Andrew

[PATCH v3 0/2] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

Posted this as a small set now, with an (optional) second patch that shows
how the changes work and what I've used to test the code on a Topic Miami board.

v3: Add patch that implements mac in nvmem for the Cadence MACB controller
    Remove the integrated of_get_mac_address call

v2: Use of_nvmem_cell_get to avoid needing the assiciated device
    Use void* instead of char*
    Add devicetree binding doc

Mike Looijmans (2):
  of_net: Implement of_get_nvmem_mac_address helper
  net: macb: Try to retrieve MAC addess from nvmem provider

 Documentation/devicetree/bindings/net/ethernet.txt |  2 ++
 drivers/net/ethernet/cadence/macb_main.c           | 12 +++++--
 drivers/of/of_net.c                                | 40 ++++++++++++++++++++++
 include/linux/of_net.h                             |  6 ++++
 4 files changed, 57 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH v3 1/2] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

It's common practice to store MAC addresses for network interfaces into
nvmem devices. However the code to actually do this in the kernel lacks,
so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
address from an nvmem cell provider.

This is particulary useful on devices where the ethernet interface cannot
be configured by the bootloader, for example because it's in an FPGA.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 Documentation/devicetree/bindings/net/ethernet.txt |  2 ++
 drivers/of/of_net.c                                | 40 ++++++++++++++++++++++
 include/linux/of_net.h                             |  6 ++++
 3 files changed, 48 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
index 2974e63..cfc376b 100644
--- a/Documentation/devicetree/bindings/net/ethernet.txt
+++ b/Documentation/devicetree/bindings/net/ethernet.txt
@@ -10,6 +10,8 @@ Documentation/devicetree/bindings/phy/phy-bindings.txt.
   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;
+- 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/drivers/of/of_net.c b/drivers/of/of_net.c
index d820f3e..1c5d372 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -7,6 +7,7 @@
  */
 #include <linux/etherdevice.h>
 #include <linux/kernel.h>
+#include <linux/nvmem-consumer.h>
 #include <linux/of_net.h>
 #include <linux/phy.h>
 #include <linux/export.h>
@@ -80,3 +81,42 @@ const void *of_get_mac_address(struct device_node *np)
 	return of_get_mac_addr(np, "address");
 }
 EXPORT_SYMBOL(of_get_mac_address);
+
+/**
+ * Obtain the MAC address from an nvmem provider named 'mac-address' through
+ * device tree.
+ * On success, copies the new address into memory pointed to by addr and
+ * returns 0. Returns a negative error code otherwise.
+ * @np:		Device tree node containing the nvmem-cells phandle
+ * @addr:	Pointer to receive the MAC address using ether_addr_copy()
+ */
+int of_get_nvmem_mac_address(struct device_node *np, void *addr)
+{
+	struct nvmem_cell *cell;
+	const void *mac;
+	size_t len;
+	int ret;
+
+	cell = of_nvmem_cell_get(np, "mac-address");
+	if (IS_ERR(cell))
+		return PTR_ERR(cell);
+
+	mac = nvmem_cell_read(cell, &len);
+
+	nvmem_cell_put(cell);
+
+	if (IS_ERR(mac))
+		return PTR_ERR(mac);
+
+	if (len < 6 || !is_valid_ether_addr(mac)) {
+		ret = -EINVAL;
+	} else {
+		ether_addr_copy(addr, mac);
+		ret = 0;
+	}
+
+	kfree(mac);
+
+	return ret;
+}
+EXPORT_SYMBOL(of_get_nvmem_mac_address);
diff --git a/include/linux/of_net.h b/include/linux/of_net.h
index 9cd72aa..90d81ee 100644
--- a/include/linux/of_net.h
+++ b/include/linux/of_net.h
@@ -13,6 +13,7 @@
 struct net_device;
 extern int of_get_phy_mode(struct device_node *np);
 extern const void *of_get_mac_address(struct device_node *np);
+extern int of_get_nvmem_mac_address(struct device_node *np, void *addr);
 extern struct net_device *of_find_net_device_by_node(struct device_node *np);
 #else
 static inline int of_get_phy_mode(struct device_node *np)
@@ -25,6 +26,11 @@ static inline const void *of_get_mac_address(struct device_node *np)
 	return NULL;
 }
 
+static inline int of_get_nvmem_mac_address(struct device_node *np, void *addr)
+{
+	return -ENODEV;
+}
+
 static inline struct net_device *of_find_net_device_by_node(struct device_node *np)
 {
 	return NULL;
-- 
1.9.1

[PATCH v3 2/2] net: macb: Try to retrieve MAC addess from nvmem provider

[Mike Looijmans]

Call of_get_nvmem_mac_address() to fetch the MAC address from an nvmem
cell, if one is provided in the device tree. This allows the address to
be stored in an I2C EEPROM device for example.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 drivers/net/ethernet/cadence/macb_main.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index e84afcf..eabe14f 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3950,10 +3950,16 @@ static int macb_probe(struct platform_device *pdev)
 		dev->max_mtu = ETH_DATA_LEN;
 
 	mac = of_get_mac_address(np);
-	if (mac)
+	if (mac) {
 		ether_addr_copy(bp->dev->dev_addr, mac);
-	else
-		macb_get_hwaddr(bp);
+	} else {
+		err = of_get_nvmem_mac_address(np, bp->dev->dev_addr);
+		if (err) {
+			if (err == -EPROBE_DEFER)
+				goto err_out_free_netdev;
+			macb_get_hwaddr(bp);
+		}
+	}
 
 	err = of_get_phy_mode(np);
 	if (err < 0) {
-- 
1.9.1

Re: [PATCH v3 0/2] of_net: Implement of_get_nvmem_mac_address helper

[Andrew Lunn]

On Tue, Mar 27, 2018 at 11:52:24AM +0200, Mike Looijmans wrote:
> Posted this as a small set now, with an (optional) second patch that shows
> how the changes work and what I've used to test the code on a Topic Miami board.
> 
> v3: Add patch that implements mac in nvmem for the Cadence MACB controller
>     Remove the integrated of_get_mac_address call
> 
> v2: Use of_nvmem_cell_get to avoid needing the assiciated device
>     Use void* instead of char*
>     Add devicetree binding doc

Hi Mike

This looks good now, thanks.

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

    Andrew

Re: [PATCH v3 1/2] of_net: Implement of_get_nvmem_mac_address helper

[Florian Fainelli]

On 03/27/2018 02:52 AM, Mike Looijmans wrote:
> It's common practice to store MAC addresses for network interfaces into
> nvmem devices. However the code to actually do this in the kernel lacks,
> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
> address from an nvmem cell provider.
> 
> This is particulary useful on devices where the ethernet interface cannot
> be configured by the bootloader, for example because it's in an FPGA.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
>  Documentation/devicetree/bindings/net/ethernet.txt |  2 ++
>  drivers/of/of_net.c                                | 40 ++++++++++++++++++++++
>  include/linux/of_net.h                             |  6 ++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
> index 2974e63..cfc376b 100644
> --- a/Documentation/devicetree/bindings/net/ethernet.txt
> +++ b/Documentation/devicetree/bindings/net/ethernet.txt
> @@ -10,6 +10,8 @@ Documentation/devicetree/bindings/phy/phy-bindings.txt.
>    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;
> +- 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/drivers/of/of_net.c b/drivers/of/of_net.c
> index d820f3e..1c5d372 100644
> --- a/drivers/of/of_net.c
> +++ b/drivers/of/of_net.c
> @@ -7,6 +7,7 @@
>   */
>  #include <linux/etherdevice.h>
>  #include <linux/kernel.h>
> +#include <linux/nvmem-consumer.h>
>  #include <linux/of_net.h>
>  #include <linux/phy.h>
>  #include <linux/export.h>
> @@ -80,3 +81,42 @@ const void *of_get_mac_address(struct device_node *np)
>  	return of_get_mac_addr(np, "address");
>  }
>  EXPORT_SYMBOL(of_get_mac_address);
> +
> +/**
> + * Obtain the MAC address from an nvmem provider named 'mac-address' through
> + * device tree.
> + * On success, copies the new address into memory pointed to by addr and
> + * returns 0. Returns a negative error code otherwise.
> + * @np:		Device tree node containing the nvmem-cells phandle
> + * @addr:	Pointer to receive the MAC address using ether_addr_copy()
> + */
> +int of_get_nvmem_mac_address(struct device_node *np, void *addr)
> +{
> +	struct nvmem_cell *cell;
> +	const void *mac;
> +	size_t len;
> +	int ret;
> +
> +	cell = of_nvmem_cell_get(np, "mac-address");
> +	if (IS_ERR(cell))
> +		return PTR_ERR(cell);
> +
> +	mac = nvmem_cell_read(cell, &len);
> +
> +	nvmem_cell_put(cell);
> +
> +	if (IS_ERR(mac))
> +		return PTR_ERR(mac);
> +
> +	if (len < 6 || !is_valid_ether_addr(mac)) {
> +		ret = -EINVAL;

Just one nit here, can you use ETH_ALEN instead of 6? With that fixed:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v3 2/2] net: macb: Try to retrieve MAC addess from nvmem provider

[Nicolas Ferre]

On 27/03/2018 at 11:52, Mike Looijmans wrote:
> Call of_get_nvmem_mac_address() to fetch the MAC address from an nvmem
> cell, if one is provided in the device tree. This allows the address to
> be stored in an I2C EEPROM device for example.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

For this part:
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

> ---
>   drivers/net/ethernet/cadence/macb_main.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index e84afcf..eabe14f 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3950,10 +3950,16 @@ static int macb_probe(struct platform_device *pdev)
>   		dev->max_mtu = ETH_DATA_LEN;
>   
>   	mac = of_get_mac_address(np);
> -	if (mac)
> +	if (mac) {
>   		ether_addr_copy(bp->dev->dev_addr, mac);
> -	else
> -		macb_get_hwaddr(bp);
> +	} else {
> +		err = of_get_nvmem_mac_address(np, bp->dev->dev_addr);
> +		if (err) {
> +			if (err == -EPROBE_DEFER)
> +				goto err_out_free_netdev;
> +			macb_get_hwaddr(bp);
> +		}
> +	}
>   
>   	err = of_get_phy_mode(np);
>   	if (err < 0) {
> 


-- 
Nicolas Ferre

[PATCH v4 0/2] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

Posted this as a small set now, with an (optional) second patch that shows
how the changes work and what I've used to test the code on a Topic Miami board.
I've taken the liberty to add appropriate "Acked" and "Review" tags.

v4: Replaced "6" with ETH_ALEN

v3: Add patch that implements mac in nvmem for the Cadence MACB controller
    Remove the integrated of_get_mac_address call

v2: Use of_nvmem_cell_get to avoid needing the assiciated device
    Use void* instead of char*
    Add devicetree binding doc


Mike Looijmans (2):
  of_net: Implement of_get_nvmem_mac_address helper
  net: macb: Try to retrieve MAC addess from nvmem provider

 Documentation/devicetree/bindings/net/ethernet.txt |  2 ++
 drivers/net/ethernet/cadence/macb_main.c           | 12 +++++--
 drivers/of/of_net.c                                | 40 ++++++++++++++++++++++
 include/linux/of_net.h                             |  6 ++++
 4 files changed, 57 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH v4 1/2] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

It's common practice to store MAC addresses for network interfaces into
nvmem devices. However the code to actually do this in the kernel lacks,
so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
address from an nvmem cell provider.

This is particulary useful on devices where the ethernet interface cannot
be configured by the bootloader, for example because it's in an FPGA.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 Documentation/devicetree/bindings/net/ethernet.txt |  2 ++
 drivers/of/of_net.c                                | 40 ++++++++++++++++++++++
 include/linux/of_net.h                             |  6 ++++
 3 files changed, 48 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
index 2974e63..cfc376b 100644
--- a/Documentation/devicetree/bindings/net/ethernet.txt
+++ b/Documentation/devicetree/bindings/net/ethernet.txt
@@ -10,6 +10,8 @@ Documentation/devicetree/bindings/phy/phy-bindings.txt.
   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;
+- 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/drivers/of/of_net.c b/drivers/of/of_net.c
index d820f3e..53189d4 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -7,6 +7,7 @@
  */
 #include <linux/etherdevice.h>
 #include <linux/kernel.h>
+#include <linux/nvmem-consumer.h>
 #include <linux/of_net.h>
 #include <linux/phy.h>
 #include <linux/export.h>
@@ -80,3 +81,42 @@ const void *of_get_mac_address(struct device_node *np)
 	return of_get_mac_addr(np, "address");
 }
 EXPORT_SYMBOL(of_get_mac_address);
+
+/**
+ * Obtain the MAC address from an nvmem provider named 'mac-address' through
+ * device tree.
+ * On success, copies the new address into memory pointed to by addr and
+ * returns 0. Returns a negative error code otherwise.
+ * @np:		Device tree node containing the nvmem-cells phandle
+ * @addr:	Pointer to receive the MAC address using ether_addr_copy()
+ */
+int of_get_nvmem_mac_address(struct device_node *np, void *addr)
+{
+	struct nvmem_cell *cell;
+	const void *mac;
+	size_t len;
+	int ret;
+
+	cell = of_nvmem_cell_get(np, "mac-address");
+	if (IS_ERR(cell))
+		return PTR_ERR(cell);
+
+	mac = nvmem_cell_read(cell, &len);
+
+	nvmem_cell_put(cell);
+
+	if (IS_ERR(mac))
+		return PTR_ERR(mac);
+
+	if (len < ETH_ALEN || !is_valid_ether_addr(mac)) {
+		ret = -EINVAL;
+	} else {
+		ether_addr_copy(addr, mac);
+		ret = 0;
+	}
+
+	kfree(mac);
+
+	return ret;
+}
+EXPORT_SYMBOL(of_get_nvmem_mac_address);
diff --git a/include/linux/of_net.h b/include/linux/of_net.h
index 9cd72aa..90d81ee 100644
--- a/include/linux/of_net.h
+++ b/include/linux/of_net.h
@@ -13,6 +13,7 @@
 struct net_device;
 extern int of_get_phy_mode(struct device_node *np);
 extern const void *of_get_mac_address(struct device_node *np);
+extern int of_get_nvmem_mac_address(struct device_node *np, void *addr);
 extern struct net_device *of_find_net_device_by_node(struct device_node *np);
 #else
 static inline int of_get_phy_mode(struct device_node *np)
@@ -25,6 +26,11 @@ static inline const void *of_get_mac_address(struct device_node *np)
 	return NULL;
 }
 
+static inline int of_get_nvmem_mac_address(struct device_node *np, void *addr)
+{
+	return -ENODEV;
+}
+
 static inline struct net_device *of_find_net_device_by_node(struct device_node *np)
 {
 	return NULL;
-- 
1.9.1

[PATCH v4 2/2] net: macb: Try to retrieve MAC addess from nvmem provider

[Mike Looijmans]

Call of_get_nvmem_mac_address() to fetch the MAC address from an nvmem
cell, if one is provided in the device tree. This allows the address to
be stored in an I2C EEPROM device for example.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index e84afcf..eabe14f 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3950,10 +3950,16 @@ static int macb_probe(struct platform_device *pdev)
 		dev->max_mtu = ETH_DATA_LEN;
 
 	mac = of_get_mac_address(np);
-	if (mac)
+	if (mac) {
 		ether_addr_copy(bp->dev->dev_addr, mac);
-	else
-		macb_get_hwaddr(bp);
+	} else {
+		err = of_get_nvmem_mac_address(np, bp->dev->dev_addr);
+		if (err) {
+			if (err == -EPROBE_DEFER)
+				goto err_out_free_netdev;
+			macb_get_hwaddr(bp);
+		}
+	}
 
 	err = of_get_phy_mode(np);
 	if (err < 0) {
-- 
1.9.1

Re: [PATCH v3 1/2] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

On 28-03-18 00:50, Florian Fainelli wrote:
> On 03/27/2018 02:52 AM, Mike Looijmans wrote:
>> It's common practice to store MAC addresses for network interfaces into
>> nvmem devices. However the code to actually do this in the kernel lacks,
>> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
>> address from an nvmem cell provider.
>>
>> This is particulary useful on devices where the ethernet interface cannot
>> be configured by the bootloader, for example because it's in an FPGA.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>> ---
>>   Documentation/devicetree/bindings/net/ethernet.txt |  2 ++
>>   drivers/of/of_net.c                                | 40 ++++++++++++++++++++++
>>   include/linux/of_net.h                             |  6 ++++
>>   3 files changed, 48 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
>> index 2974e63..cfc376b 100644
>> --- a/Documentation/devicetree/bindings/net/ethernet.txt
>> +++ b/Documentation/devicetree/bindings/net/ethernet.txt
>> @@ -10,6 +10,8 @@ Documentation/devicetree/bindings/phy/phy-bindings.txt.
>>     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;
>> +- 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/drivers/of/of_net.c b/drivers/of/of_net.c
>> index d820f3e..1c5d372 100644
>> --- a/drivers/of/of_net.c
>> +++ b/drivers/of/of_net.c
>> @@ -7,6 +7,7 @@
>>    */
>>   #include <linux/etherdevice.h>
>>   #include <linux/kernel.h>
>> +#include <linux/nvmem-consumer.h>
>>   #include <linux/of_net.h>
>>   #include <linux/phy.h>
>>   #include <linux/export.h>
>> @@ -80,3 +81,42 @@ const void *of_get_mac_address(struct device_node *np)
>>   	return of_get_mac_addr(np, "address");
>>   }
>>   EXPORT_SYMBOL(of_get_mac_address);
>> +
>> +/**
>> + * Obtain the MAC address from an nvmem provider named 'mac-address' through
>> + * device tree.
>> + * On success, copies the new address into memory pointed to by addr and
>> + * returns 0. Returns a negative error code otherwise.
>> + * @np:		Device tree node containing the nvmem-cells phandle
>> + * @addr:	Pointer to receive the MAC address using ether_addr_copy()
>> + */
>> +int of_get_nvmem_mac_address(struct device_node *np, void *addr)
>> +{
>> +	struct nvmem_cell *cell;
>> +	const void *mac;
>> +	size_t len;
>> +	int ret;
>> +
>> +	cell = of_nvmem_cell_get(np, "mac-address");
>> +	if (IS_ERR(cell))
>> +		return PTR_ERR(cell);
>> +
>> +	mac = nvmem_cell_read(cell, &len);
>> +
>> +	nvmem_cell_put(cell);
>> +
>> +	if (IS_ERR(mac))
>> +		return PTR_ERR(mac);
>> +
>> +	if (len < 6 || !is_valid_ether_addr(mac)) {
>> +		ret = -EINVAL;
> 
> Just one nit here, can you use ETH_ALEN instead of 6? With that fixed:
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 

Ok, implemented and tested it, and v4 is on the way with that change.


Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

Re: [PATCH v4 0/2] of_net: Implement of_get_nvmem_mac_address helper

[David Miller]

From: Mike Looijmans <mike.looijmans@topic.nl>
Date: Thu, 29 Mar 2018 07:29:47 +0200

> Posted this as a small set now, with an (optional) second patch that shows
> how the changes work and what I've used to test the code on a Topic Miami board.
> I've taken the liberty to add appropriate "Acked" and "Review" tags.
> 
> v4: Replaced "6" with ETH_ALEN
> 
> v3: Add patch that implements mac in nvmem for the Cadence MACB controller
>     Remove the integrated of_get_mac_address call
> 
> v2: Use of_nvmem_cell_get to avoid needing the assiciated device
>     Use void* instead of char*
>     Add devicetree binding doc

Series applied to net-next, thank you.

Re: [PATCH v4 0/2] of_net: Implement of_get_nvmem_mac_address helper

[Mike Looijmans]

On 30-03-18 16:40, David Miller wrote:
> From: Mike Looijmans <mike.looijmans@topic.nl>
> Date: Thu, 29 Mar 2018 07:29:47 +0200
> 
>> Posted this as a small set now, with an (optional) second patch that shows
>> how the changes work and what I've used to test the code on a Topic Miami board.
>> I've taken the liberty to add appropriate "Acked" and "Review" tags.
>>
>> v4: Replaced "6" with ETH_ALEN
>>
>> v3: Add patch that implements mac in nvmem for the Cadence MACB controller
>>      Remove the integrated of_get_mac_address call
>>
>> v2: Use of_nvmem_cell_get to avoid needing the assiciated device
>>      Use void* instead of char*
>>      Add devicetree binding doc
> 
> Series applied to net-next, thank you.
> 

Got a kbuild bot error message in the e-mail, with errors like: 
"undefined reference to `of_nvmem_cell_get'"

Should I add an "#if CONFIG_NVMEM" guard around the new code and make a 
v5 patch?

-- 
Mike Looijmans

Re: [PATCH v4 0/2] of_net: Implement of_get_nvmem_mac_address helper

[Andrew Lunn]

On Sat, Mar 31, 2018 at 07:20:30PM +0200, Mike Looijmans wrote:
> On 30-03-18 16:40, David Miller wrote:
> >From: Mike Looijmans <mike.looijmans@topic.nl>
> >Date: Thu, 29 Mar 2018 07:29:47 +0200
> >
> >>Posted this as a small set now, with an (optional) second patch that shows
> >>how the changes work and what I've used to test the code on a Topic Miami board.
> >>I've taken the liberty to add appropriate "Acked" and "Review" tags.
> >>
> >>v4: Replaced "6" with ETH_ALEN
> >>
> >>v3: Add patch that implements mac in nvmem for the Cadence MACB controller
> >>     Remove the integrated of_get_mac_address call
> >>
> >>v2: Use of_nvmem_cell_get to avoid needing the assiciated device
> >>     Use void* instead of char*
> >>     Add devicetree binding doc
> >
> >Series applied to net-next, thank you.
> >
> 
> Got a kbuild bot error message in the e-mail, with errors like: "undefined
> reference to `of_nvmem_cell_get'"
> 
> Should I add an "#if CONFIG_NVMEM" guard around the new code and make a v5
> patch?

Hi Mike

You need to understand what is actually causing the problem. If it no
NVMEM, or no OF? The kernel config was part of the email, so you
should be able to reproduce the failure.

We don't normally add #ifdef in code. Instead we implement stub
functions in the header file, which would in this case return
something like -ENODEV.

Dave has already take this patch. So you need to do an incremental
fix.

	  Andrew