2019-05-03 14:37:41

by Petr Štetiar

[permalink] [raw]
Subject: [PATCH v4 01/10] of_net: add NVMEM support to of_get_mac_address

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 <[email protected]>
Signed-off-by: Felix Fietkau <[email protected]>
Signed-off-by: John Crispin <[email protected]>
Signed-off-by: Petr Štetiar <[email protected]>
---

Changes since v1:

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

Changes since v2:

* moved of_get_mac_addr_nvmem after of_get_mac_addr(np, "address") call
* replaced kzalloc, kmemdup and kfree with it's devm variants
* introduced of_has_nvmem_mac_addr helper which checks if DT node has nvmem
cell with `mac-address`
* of_get_mac_address now returns ERR_PTR encoded error value

Changes since v3:

* removed of_has_nvmem_mac_addr helper as it's not needed now
* of_get_mac_address now returns only valid pointer or ERR_PTR encoded error value

drivers/of/of_net.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index d820f3e..9649cd5 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -8,8 +8,10 @@
#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>
+#include <linux/device.h>

/**
* of_get_phy_mode - Get phy mode for given device_node
@@ -47,12 +49,52 @@ 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 ret;
+ u8 mac[ETH_ALEN];
+ struct property *pp;
+ struct platform_device *pdev = of_find_device_by_node(np);
+
+ if (!pdev)
+ return ERR_PTR(-ENODEV);
+
+ ret = nvmem_get_mac_address(&pdev->dev, &mac);
+ if (ret)
+ return ERR_PTR(ret);
+
+ pp = devm_kzalloc(&pdev->dev, sizeof(*pp), GFP_KERNEL);
+ if (!pp)
+ return ERR_PTR(-ENOMEM);
+
+ pp->name = "nvmem-mac-address";
+ pp->length = ETH_ALEN;
+ pp->value = devm_kmemdup(&pdev->dev, mac, ETH_ALEN, GFP_KERNEL);
+ if (!pp->value) {
+ ret = -ENOMEM;
+ goto free;
+ }
+
+ ret = of_add_property(np, pp);
+ if (ret)
+ goto free;
+
+ return pp->value;
+free:
+ devm_kfree(&pdev->dev, pp->value);
+ devm_kfree(&pdev->dev, pp);
+
+ return ERR_PTR(ret);
+}
+
/**
* 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, then the obsolete
+ * 'address' is checked, just in case we're using an old device tree. If any
+ * of the above isn't set, then try to get MAC address from nvmem cell named
+ * 'mac-address'.
*
* 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
@@ -64,6 +106,8 @@ static const void *of_get_mac_addr(struct device_node *np, const char *name)
* addresses. Some older U-Boots only initialized 'local-mac-address'. In
* this case, the real MAC is in 'local-mac-address', and 'mac-address' exists
* but is all zeros.
+ *
+ * Return: Will be a valid pointer on success and ERR_PTR in case of error.
*/
const void *of_get_mac_address(struct device_node *np)
{
@@ -77,6 +121,10 @@ const void *of_get_mac_address(struct device_node *np)
if (addr)
return addr;

- return of_get_mac_addr(np, "address");
+ addr = of_get_mac_addr(np, "address");
+ if (addr)
+ return addr;
+
+ return of_get_mac_addr_nvmem(np);
}
EXPORT_SYMBOL(of_get_mac_address);
--
1.9.1


2019-05-03 14:39:57

by Maxime Ripard

[permalink] [raw]
Subject: Re: [PATCH v4 01/10] of_net: add NVMEM support to of_get_mac_address

On Fri, May 03, 2019 at 04:27:06PM +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 <[email protected]>
> Signed-off-by: Felix Fietkau <[email protected]>
> Signed-off-by: John Crispin <[email protected]>
> Signed-off-by: Petr Štetiar <[email protected]>
> ---
>
> Changes since v1:
>
> * moved handling of nvmem after mac-address and local-mac-address properties
>
> Changes since v2:
>
> * moved of_get_mac_addr_nvmem after of_get_mac_addr(np, "address") call
> * replaced kzalloc, kmemdup and kfree with it's devm variants
> * introduced of_has_nvmem_mac_addr helper which checks if DT node has nvmem
> cell with `mac-address`
> * of_get_mac_address now returns ERR_PTR encoded error value
>
> Changes since v3:
>
> * removed of_has_nvmem_mac_addr helper as it's not needed now
> * of_get_mac_address now returns only valid pointer or ERR_PTR encoded error value
>
> drivers/of/of_net.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> index d820f3e..9649cd5 100644
> --- a/drivers/of/of_net.c
> +++ b/drivers/of/of_net.c
> @@ -8,8 +8,10 @@
> #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>
> +#include <linux/device.h>
>
> /**
> * of_get_phy_mode - Get phy mode for given device_node
> @@ -47,12 +49,52 @@ 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 ret;
> + u8 mac[ETH_ALEN];
> + struct property *pp;
> + struct platform_device *pdev = of_find_device_by_node(np);
> +
> + if (!pdev)
> + return ERR_PTR(-ENODEV);
> +
> + ret = nvmem_get_mac_address(&pdev->dev, &mac);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + pp = devm_kzalloc(&pdev->dev, sizeof(*pp), GFP_KERNEL);
> + if (!pp)
> + return ERR_PTR(-ENOMEM);
> +
> + pp->name = "nvmem-mac-address";
> + pp->length = ETH_ALEN;
> + pp->value = devm_kmemdup(&pdev->dev, mac, ETH_ALEN, GFP_KERNEL);
> + if (!pp->value) {
> + ret = -ENOMEM;
> + goto free;
> + }
> +
> + ret = of_add_property(np, pp);
> + if (ret)
> + goto free;
> +
> + return pp->value;

I'm not sure why you need to do that allocation here, and why you need
to modify the DT?

can't you just return the mac address directly since it's what the
of_get_mac_address caller will expect anyway?

maxime

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


Attachments:
(No filename) (3.30 kB)
signature.asc (235.00 B)
Download all attachments

2019-05-03 16:07:44

by Petr Štetiar

[permalink] [raw]
Subject: Re: [PATCH v4 01/10] of_net: add NVMEM support to of_get_mac_address

Maxime Ripard <[email protected]> [2019-05-03 16:36:43]:

Hi,

...

> > + pp = devm_kzalloc(&pdev->dev, sizeof(*pp), GFP_KERNEL);
> > + if (!pp)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + pp->name = "nvmem-mac-address";
> > + pp->length = ETH_ALEN;
> > + pp->value = devm_kmemdup(&pdev->dev, mac, ETH_ALEN, GFP_KERNEL);
> > + if (!pp->value) {
> > + ret = -ENOMEM;
> > + goto free;
> > + }
> > +
> > + ret = of_add_property(np, pp);
> > + if (ret)
> > + goto free;
> > +
> > + return pp->value;
>
> I'm not sure why you need to do that allocation here, and why you need
> to modify the DT?

I was asked about that in v2[0] already, so just copy&pasting relevant part of
my response here:

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.

[...]

0. https://patchwork.ozlabs.org/patch/1092248/#2164089
1. https://patchwork.ozlabs.org/patch/1086628/

> can't you just return the mac address directly since it's what the
> of_get_mac_address caller will expect anyway?

I don't need this stuff, I can remove it, please just tell me what is
appropriate and I'm going to do that.

-- ynezz

2019-05-06 14:07:34

by Maxime Ripard

[permalink] [raw]
Subject: Re: [PATCH v4 01/10] of_net: add NVMEM support to of_get_mac_address

On Fri, May 03, 2019 at 05:44:07PM +0200, Petr Štetiar wrote:
> Maxime Ripard <[email protected]> [2019-05-03 16:36:43]:
>
> Hi,
>
> ...
>
> > > + pp = devm_kzalloc(&pdev->dev, sizeof(*pp), GFP_KERNEL);
> > > + if (!pp)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + pp->name = "nvmem-mac-address";
> > > + pp->length = ETH_ALEN;
> > > + pp->value = devm_kmemdup(&pdev->dev, mac, ETH_ALEN, GFP_KERNEL);
> > > + if (!pp->value) {
> > > + ret = -ENOMEM;
> > > + goto free;
> > > + }
> > > +
> > > + ret = of_add_property(np, pp);
> > > + if (ret)
> > > + goto free;
> > > +
> > > + return pp->value;
> >
> > I'm not sure why you need to do that allocation here, and why you need
> > to modify the DT?
>
> I was asked about that in v2[0] already, so just copy&pasting relevant part of
> my response here:
>
> 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.
>
> [...]
>
> 0. https://patchwork.ozlabs.org/patch/1092248/#2164089
> 1. https://patchwork.ozlabs.org/patch/1086628/
>
> > can't you just return the mac address directly since it's what the
> > of_get_mac_address caller will expect anyway?
>
> I don't need this stuff, I can remove it, please just tell me what is
> appropriate and I'm going to do that.

Well, if no ones need it, then there's no point in adding support for
it then?

Maxime

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


Attachments:
(No filename) (1.78 kB)
signature.asc (235.00 B)
Download all attachments