of_net: add mtd-mac-address support to of_get_mac_address()
diff mbox series

Message ID 1555445100-30936-1-git-send-email-ynezz@true.cz
State Changes Requested
Headers show
Series
  • of_net: add mtd-mac-address support to of_get_mac_address()
Related show

Checks

Context Check Description
robh/checkpatch warning "total: 0 errors, 1 warnings, 122 lines checked"

Commit Message

Petr Štetiar April 16, 2019, 8:05 p.m. UTC
From: John Crispin <john@phrozen.org>

Many embedded devices have information such as MAC addresses stored
inside MTD devices. This patch allows us to add a property inside a node
describing a network interface. The new property points at a MTD
partition with an offset where the MAC address can be found.

This patch has originated in OpenWrt some time ago, so in order to
consider usefulness of this patch, here are some real-world numbers
which hopefully speak for themselves:

 * mtd-mac-address                used 497 times in 357 device tree files
 * mtd-mac-address-increment      used  74 times in  58 device tree files
 * mtd-mac-address-increment-byte used   1 time  in   1 device tree file

Signed-off-by: John Crispin <john@phrozen.org>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
[cleanup of the patch for upstream submission]
Signed-off-by: Petr Štetiar <ynezz@true.cz>
---
 Documentation/devicetree/bindings/net/ethernet.txt |  5 ++
 drivers/of/of_net.c                                | 82 +++++++++++++++++++++-
 2 files changed, 86 insertions(+), 1 deletion(-)

Comments

Florian Fainelli April 17, 2019, 12:29 a.m. UTC | #1
On 16/04/2019 13:05, Petr Štetiar wrote:
> From: John Crispin <john@phrozen.org>
> 
> Many embedded devices have information such as MAC addresses stored
> inside MTD devices. This patch allows us to add a property inside a node
> describing a network interface. The new property points at a MTD
> partition with an offset where the MAC address can be found.
> 
> This patch has originated in OpenWrt some time ago, so in order to
> consider usefulness of this patch, here are some real-world numbers
> which hopefully speak for themselves:
> 
>   * mtd-mac-address                used 497 times in 357 device tree files
>   * mtd-mac-address-increment      used  74 times in  58 device tree files
>   * mtd-mac-address-increment-byte used   1 time  in   1 device tree file
> 
> Signed-off-by: John Crispin <john@phrozen.org>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> [cleanup of the patch for upstream submission]
> Signed-off-by: Petr Štetiar <ynezz@true.cz>
> ---

[snip]

> +static const void *of_get_mac_address_mtd(struct device_node *np)
> +{
> +#ifdef CONFIG_MTD
> +	void *addr;
> +	size_t retlen;
> +	int size, ret;
> +	u8 mac[ETH_ALEN];
> +	phandle phandle;
> +	const char *part;
> +	const __be32 *list;
> +	struct mtd_info *mtd;
> +	struct property *prop;
> +	u32 mac_inc = 0;
> +	u32 inc_idx = ETH_ALEN-1;
> +	struct device_node *mtd_np = NULL;

Reverse christmas tree would look a bit nicer here.

> +
> +	list = of_get_property(np, "mtd-mac-address", &size);
> +	if (!list || (size != (2 * sizeof(*list))))
> +		return NULL;
> +
> +	phandle = be32_to_cpup(list++);
> +	if (phandle)
> +		mtd_np = of_find_node_by_phandle(phandle);
> +
> +	if (!mtd_np)
> +		return NULL;
> +
> +	part = of_get_property(mtd_np, "label", NULL);
> +	if (!part)
> +		part = mtd_np->name;
> +
> +	mtd = get_mtd_device_nm(part);
> +	if (IS_ERR(mtd))
> +		return NULL;
> +
> +	ret = mtd_read(mtd, be32_to_cpup(list), ETH_ALEN, &retlen, mac);
> +	put_mtd_device(mtd);
> +
> +	of_property_read_u32(np, "mtd-mac-address-increment-byte", &inc_idx);

of_property_read_u8() would probably be good here since this can't be 
bigger than 5 anyway.

> +	if (inc_idx > ETH_ALEN-1)
> +		return NULL; >
> +	if (!of_property_read_u32(np, "mtd-mac-address-increment", &mac_inc))
> +		mac[inc_idx] += mac_inc;

If I use a number greater than and included 128; this will cause a roll 
over, should this be range checked? Similarly, using 
of_property_read_u8() might be a better fit?

Other than those, LGTM
Frank Rowand April 17, 2019, 3:01 a.m. UTC | #2
Hi Rob,

On 4/16/19 5:29 PM, Florian Fainelli wrote:
> 
> 
> On 16/04/2019 13:05, Petr Štetiar wrote:
>> From: John Crispin <john@phrozen.org>
>>
>> Many embedded devices have information such as MAC addresses stored
>> inside MTD devices. This patch allows us to add a property inside a node
>> describing a network interface. The new property points at a MTD
>> partition with an offset where the MAC address can be found.
>>
>> This patch has originated in OpenWrt some time ago, so in order to
>> consider usefulness of this patch, here are some real-world numbers
>> which hopefully speak for themselves:
>>
>>   * mtd-mac-address                used 497 times in 357 device tree files
>>   * mtd-mac-address-increment      used  74 times in  58 device tree files
>>   * mtd-mac-address-increment-byte used   1 time  in   1 device tree file
>>
>> Signed-off-by: John Crispin <john@phrozen.org>
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> [cleanup of the patch for upstream submission]
>> Signed-off-by: Petr Štetiar <ynezz@true.cz>
>> ---
> 
> [snip]
> 
>> +static const void *of_get_mac_address_mtd(struct device_node *np)
>> +{
>> +#ifdef CONFIG_MTD
>> +    void *addr;
>> +    size_t retlen;
>> +    int size, ret;
>> +    u8 mac[ETH_ALEN];
>> +    phandle phandle;
>> +    const char *part;
>> +    const __be32 *list;
>> +    struct mtd_info *mtd;
>> +    struct property *prop;
>> +    u32 mac_inc = 0;
>> +    u32 inc_idx = ETH_ALEN-1;
>> +    struct device_node *mtd_np = NULL;
> 
> Reverse christmas tree would look a bit nicer here.

Do we a variable declaration format preference for drivers/of/*?

-Frank


> 
>> +
>> +    list = of_get_property(np, "mtd-mac-address", &size);
>> +    if (!list || (size != (2 * sizeof(*list))))
>> +        return NULL;
>> +
>> +    phandle = be32_to_cpup(list++);
>> +    if (phandle)
>> +        mtd_np = of_find_node_by_phandle(phandle);
>> +
>> +    if (!mtd_np)
>> +        return NULL;
>> +
>> +    part = of_get_property(mtd_np, "label", NULL);
>> +    if (!part)
>> +        part = mtd_np->name;
>> +
>> +    mtd = get_mtd_device_nm(part);
>> +    if (IS_ERR(mtd))
>> +        return NULL;
>> +
>> +    ret = mtd_read(mtd, be32_to_cpup(list), ETH_ALEN, &retlen, mac);
>> +    put_mtd_device(mtd);
>> +
>> +    of_property_read_u32(np, "mtd-mac-address-increment-byte", &inc_idx);
> 
> of_property_read_u8() would probably be good here since this can't be bigger than 5 anyway.
> 
>> +    if (inc_idx > ETH_ALEN-1)
>> +        return NULL; >
>> +    if (!of_property_read_u32(np, "mtd-mac-address-increment", &mac_inc))
>> +        mac[inc_idx] += mac_inc;
> 
> If I use a number greater than and included 128; this will cause a roll over, should this be range checked? Similarly, using of_property_read_u8() might be a better fit?
> 
> Other than those, LGTM
Heiner Kallweit April 17, 2019, 5 a.m. UTC | #3
On 16.04.2019 22:05, Petr Štetiar wrote:
> From: John Crispin <john@phrozen.org>
> 
> Many embedded devices have information such as MAC addresses stored
> inside MTD devices. This patch allows us to add a property inside a node
> describing a network interface. The new property points at a MTD
> partition with an offset where the MAC address can be found.
> 
> This patch has originated in OpenWrt some time ago, so in order to
> consider usefulness of this patch, here are some real-world numbers
> which hopefully speak for themselves:
> 
>  * mtd-mac-address                used 497 times in 357 device tree files
>  * mtd-mac-address-increment      used  74 times in  58 device tree files
>  * mtd-mac-address-increment-byte used   1 time  in   1 device tree file
> 
> Signed-off-by: John Crispin <john@phrozen.org>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> [cleanup of the patch for upstream submission]
> Signed-off-by: Petr Štetiar <ynezz@true.cz>
> ---
>  Documentation/devicetree/bindings/net/ethernet.txt |  5 ++
>  drivers/of/of_net.c                                | 82 +++++++++++++++++++++-
>  2 files changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
> index cfc376b..8a7891e 100644
> --- a/Documentation/devicetree/bindings/net/ethernet.txt
> +++ b/Documentation/devicetree/bindings/net/ethernet.txt
> @@ -10,6 +10,11 @@ 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;
> +- mtd-mac-address: specify a MTD partition + offset containing array of 6 bytes
> +- mtd-mac-address-increment: specify number by which we should increment the
> +  MAC address stored in the MTD partition
> +- mtd-mac-address-increment-byte: specify octet/byte(0-5) in the MAC address,
> +  where we should increment the value, defaults to octet 5 (the last one)
>  - 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;
> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> index 810ab0f..01b24d6 100644
> --- a/drivers/of/of_net.c
> +++ b/drivers/of/of_net.c
> @@ -11,6 +11,7 @@
>  #include <linux/of_net.h>
>  #include <linux/phy.h>
>  #include <linux/export.h>
> +#include <linux/mtd/mtd.h>
>  
>  /**
>   * of_get_phy_mode - Get phy mode for given device_node
> @@ -39,7 +40,7 @@ int of_get_phy_mode(struct device_node *np)
>  }
>  EXPORT_SYMBOL_GPL(of_get_phy_mode);
>  
> -static const void *of_get_mac_addr(struct device_node *np, const char *name)
> +static void *of_get_mac_addr(struct device_node *np, const char *name)
>  {
>  	struct property *pp = of_find_property(np, name, NULL);
>  
> @@ -48,6 +49,78 @@ static const void *of_get_mac_addr(struct device_node *np, const char *name)
>  	return NULL;
>  }
>  
> +static const void *of_get_mac_address_mtd(struct device_node *np)
> +{
> +#ifdef CONFIG_MTD
> +	void *addr;
> +	size_t retlen;
> +	int size, ret;
> +	u8 mac[ETH_ALEN];
> +	phandle phandle;
> +	const char *part;
> +	const __be32 *list;
> +	struct mtd_info *mtd;
> +	struct property *prop;
> +	u32 mac_inc = 0;
> +	u32 inc_idx = ETH_ALEN-1;
> +	struct device_node *mtd_np = NULL;
> +
> +	list = of_get_property(np, "mtd-mac-address", &size);
> +	if (!list || (size != (2 * sizeof(*list))))
> +		return NULL;
> +
> +	phandle = be32_to_cpup(list++);
> +	if (phandle)
> +		mtd_np = of_find_node_by_phandle(phandle);
> +
> +	if (!mtd_np)
> +		return NULL;
> +
> +	part = of_get_property(mtd_np, "label", NULL);
> +	if (!part)
> +		part = mtd_np->name;
> +
> +	mtd = get_mtd_device_nm(part);
> +	if (IS_ERR(mtd))
> +		return NULL;
> +
> +	ret = mtd_read(mtd, be32_to_cpup(list), ETH_ALEN, &retlen, mac);

ret doesn't seem to be checked. And checking retlen wouldn't be bad too.

> +	put_mtd_device(mtd);
> +
> +	of_property_read_u32(np, "mtd-mac-address-increment-byte", &inc_idx);
> +	if (inc_idx > ETH_ALEN-1)
> +		return NULL;
> +
> +	if (!of_property_read_u32(np, "mtd-mac-address-increment", &mac_inc))
> +		mac[inc_idx] += mac_inc;
> +
> +	if (!is_valid_ether_addr(mac))
> +		return NULL;
> +
> +	addr = of_get_mac_addr(np, "mac-address");
> +	if (addr) {
> +		memcpy(addr, mac, ETH_ALEN);
> +		return addr;
> +	}
> +
> +	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> +	if (!prop)
> +		return NULL;
> +
> +	prop->name = "mac-address";
> +	prop->length = ETH_ALEN;
> +	prop->value = kmemdup(mac, ETH_ALEN, GFP_KERNEL);
> +	if (!prop->value || of_add_property(np, prop))
> +		goto free;
> +
> +	return prop->value;
> +free:
> +	kfree(prop->value);
> +	kfree(prop);
> +#endif
> +	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
> @@ -65,11 +138,18 @@ 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.
> + *
> + * If a mtd-mac-address property exists, try to fetch the MAC address from the
> + * specified MTD device, and store it as a 'mac-address' property.
>  */
>  const void *of_get_mac_address(struct device_node *np)
>  {
>  	const void *addr;
>  
> +	addr = of_get_mac_address_mtd(np);
> +	if (addr)
> +		return addr;
> +
>  	addr = of_get_mac_addr(np, "mac-address");
>  	if (addr)
>  		return addr;
>
Maxime Ripard April 17, 2019, 8:06 a.m. UTC | #4
On Tue, Apr 16, 2019 at 10:05:00PM +0200, Petr Štetiar wrote:
> From: John Crispin <john@phrozen.org>
>
> Many embedded devices have information such as MAC addresses stored
> inside MTD devices. This patch allows us to add a property inside a node
> describing a network interface. The new property points at a MTD
> partition with an offset where the MAC address can be found.
>
> This patch has originated in OpenWrt some time ago, so in order to
> consider usefulness of this patch, here are some real-world numbers
> which hopefully speak for themselves:
>
>  * mtd-mac-address                used 497 times in 357 device tree files
>  * mtd-mac-address-increment      used  74 times in  58 device tree files
>  * mtd-mac-address-increment-byte used   1 time  in   1 device tree file
>
> Signed-off-by: John Crispin <john@phrozen.org>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> [cleanup of the patch for upstream submission]
> Signed-off-by: Petr Štetiar <ynezz@true.cz>

NVMEM is supported by of_net already and there's an MTD-to-nvmem
bridge already, so it doesn't look really necessary to create
additional properties that cover the same use case.

(or at least, you should explain why nvmem doesn't work there)

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Petr Štetiar April 17, 2019, 9:49 a.m. UTC | #5
Maxime Ripard <maxime.ripard@bootlin.com> [2019-04-17 10:06:14]:

Hi Maxime,

> On Tue, Apr 16, 2019 at 10:05:00PM +0200, Petr Štetiar wrote:
> > From: John Crispin <john@phrozen.org>
> >
> > Many embedded devices have information such as MAC addresses stored
> > inside MTD devices. This patch allows us to add a property inside a node
> > describing a network interface. The new property points at a MTD
> > partition with an offset where the MAC address can be found.
> >
> > This patch has originated in OpenWrt some time ago, so in order to
> > consider usefulness of this patch, here are some real-world numbers
> > which hopefully speak for themselves:
> >
> >  * mtd-mac-address                used 497 times in 357 device tree files
> >  * mtd-mac-address-increment      used  74 times in  58 device tree files
> >  * mtd-mac-address-increment-byte used   1 time  in   1 device tree file
> 
> NVMEM is supported by of_net already and there's an MTD-to-nvmem
> bridge already, so it doesn't look really necessary to create
> additional properties that cover the same use case.

if those use cases could be handled with NVMEM, then I'm all in. As I can't
find any example in some device tree file in the kernel tree yet and the
documentation isn't clear to me about this topic either, could you please help
me and provide me with some NVMEM based example for the following simplified
use case? Or what do I need to bend/patch in order to support this within
NVME.

&spi {
	flash@0 {
		compatible = "jedec,spi-nor";

		mtdparts: partitions {
			compatible = "fixed-partitions";

			art: partition@50000 {
				label = "factory";
				reg = <0x050000 0x010000>;
				read-only;
			};
		};
	};
};

&eth0 {
	mtd-mac-address = <&art 0x4>;
};

&eth1 {
	mtd-mac-address = <&art 0x04>;
	mtd-mac-address-increment = <1>;
};

&wmac {
	mtd-mac-address = <&art 0x04>;
	mtd-mac-address-increment = <2>;
};

I would like to point out, that this proposed patch is currently handling only
some of the use cases within OpenWrt tree as well, merely those where the
vendor of the device is providing MAC address in the NVMEM as an array of 6
bytes.

Unfortunately there are creative vendors which provide MAC addresses in the
NVMEM as ASCII text in following two formats (hexdump -C /dev/mtdX):

 00000090  57 2e 4c 41 4e 2e 4d 41  43 2e 41 64 64 72 65 73  |W.LAN.MAC.Addres|
 000000a0  73 3d 30 30 39 30 46 45  43 39 43 42 45 35 00 48  |s=0090FEC9CBE5.H|
 000000b0  57 2e 4c 41 4e 2e 32 47  2e 30 2e 4d 41 43 2e 41  |W.LAN.2G.0.MAC.A|
 000000c0  64 64 72 65 73 73 3d 30  30 39 30 46 45 43 39 43  |ddress=0090FEC9C|
 000000d0  42 45 36 00 48 57 2e 4c  41 4e 2e 35 47 2e 30 2e  |BE6.HW.LAN.5G.0.|
 000000e0  4d 41 43 2e 41 64 64 72  65 73 73 3d 30 30 39 30  |MAC.Address=0090|
 000000f0  46 45 43 39 43 42 45 37  00 48 57 2e 57 41 4e 2e  |FEC9CBE7.HW.WAN.|
 00000100  4d 41 43 2e 41 64 64 72  65 73 73 3d 30 30 39 30  |MAC.Address=0090|
 00000110  46 45 43 39 43 42 45 34  00 00 00 00 00 00 00 00  |FEC9CBE4........|

 (From https://github.com/openwrt/openwrt/pull/1448#issuecomment-442476695)

 00000170  39 7a 71 05 83 6b bd 9c  a7 45 fb 69 6e 27 a2 56  |9zq..k...E.in'.V|
 00000180  66 61 63 5f 6d 61 63 20  3d 20 44 34 3a 45 45 3a  |fac_mac = D4:EE:|
 00000190  30 37 3a 33 33 3a 36 43  3a 32 30 0a 42 44 49 4e  |07:33:6C:20.BDIN|
 000001a0  46 4f 5f 45 4e 44 0a 00  ff ff ff ff ff ff ff ff  |FO_END..........|

 (From https://github.com/openwrt/openwrt/pull/1906#issuecomment-483881911)

And those use cases are currently handled within OpenWrt by workarounds in the
user space[1] like this one for example:

 echo $(macaddr_add "$(mtd_get_mac_ascii u-boot-env ethaddr)" 1) > /sys${DEVPATH}/macaddress

So I'm wondering how this could be handled with NVMEM as well. I've simply
though, that I would try to fix this with some device tree based solution as I
think, that this information belongs to the device tree and shouldn't be
handled in the user space.

I was thinking about adding `mtd-mac-address-ascii` (or similar) device tree
based solution to handle the MAC addresses in 0090FEC9CBE4 and
00:90:FE:C9:CB:E4 ascii/text formats as well.

1.  https://github.com/openwrt/openwrt/blob/master/target/linux/ath79/base-files/etc/hotplug.d/ieee80211/10_fix_wifi_mac

-- ynezz
Maxime Ripard April 17, 2019, 10:15 a.m. UTC | #6
On Wed, Apr 17, 2019 at 11:49:21AM +0200, Petr Štetiar wrote:
> Maxime Ripard <maxime.ripard@bootlin.com> [2019-04-17 10:06:14]:
>
> Hi Maxime,
>
> > On Tue, Apr 16, 2019 at 10:05:00PM +0200, Petr Štetiar wrote:
> > > From: John Crispin <john@phrozen.org>
> > >
> > > Many embedded devices have information such as MAC addresses stored
> > > inside MTD devices. This patch allows us to add a property inside a node
> > > describing a network interface. The new property points at a MTD
> > > partition with an offset where the MAC address can be found.
> > >
> > > This patch has originated in OpenWrt some time ago, so in order to
> > > consider usefulness of this patch, here are some real-world numbers
> > > which hopefully speak for themselves:
> > >
> > >  * mtd-mac-address                used 497 times in 357 device tree files
> > >  * mtd-mac-address-increment      used  74 times in  58 device tree files
> > >  * mtd-mac-address-increment-byte used   1 time  in   1 device tree file
> >
> > NVMEM is supported by of_net already and there's an MTD-to-nvmem
> > bridge already, so it doesn't look really necessary to create
> > additional properties that cover the same use case.
>
> if those use cases could be handled with NVMEM, then I'm all in. As I can't
> find any example in some device tree file in the kernel tree yet and the
> documentation isn't clear to me about this topic either, could you please help
> me and provide me with some NVMEM based example for the following simplified
> use case? Or what do I need to bend/patch in order to support this within
> NVME.

https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/nvmem/nvmem.txt
and

https://elixir.bootlin.com/linux/latest/source/net/ethernet/eth.c#L564

Should get you started

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Petr Štetiar April 17, 2019, 12:10 p.m. UTC | #7
Maxime Ripard <maxime.ripard@bootlin.com> [2019-04-17 12:15:57]:

> https://elixir.bootlin.com/linux/latest/source/net/ethernet/eth.c#L564

Thanks, so if I parse those 2 use cases of nvmem_get_mac_address correctly:

 drivers/net/ethernet/cadence/macb_main.c:  err = nvmem_get_mac_address(&pdev->dev, bp->dev->dev_addr);
 drivers/net/ethernet/ti/davinci_emac.c:    rc = nvmem_get_mac_address(&pdev->dev, priv->mac_addr);

We would need to spill `nvmem_get_mac_address` into every single driver on
earth (as it's done now in those two drivers I've mentioned above) which would
like to use MAC address stored in NVME or wire `nvmem_get_mac_address` into
`of_get_mac_address`, right?

-- ynezz
Petr Štetiar April 17, 2019, 4:06 p.m. UTC | #8
Maxime Ripard <maxime.ripard@bootlin.com> [2019-04-17 10:06:14]:

> NVMEM is supported by of_net already

Well, not anymore:

 commit afa64a72b862a7a9d04f8d07fba632eaf06b23f8
 Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
 Date:   Fri Nov 30 09:20:59 2018 +0100

    of: net: kill of_get_nvmem_mac_address()

Now, I'm really confused.

Documentation/devicetree/bindings/net/ethernet.txt states following:

 - 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;

which is actually misleading and confusing. There are only two ethernet
drivers in the tree cadence and davinci which support this properties. Well
there's nixge, but this one is special, because it has renamed `mac-address`
to `address` with it's own implementation in `nixge_get_nvmem_address`.

All other ethernet drivers (and few others) simply use `of_get_mac_address`
which doesn't support NVMEM and the documented nvmem-cells* properties.

> so it doesn't look really necessary to create additional properties that
> cover the same use case.

NVMEM could be reused for this purpose and it seems like a way to go, probably
if we could wire `nvmem_get_mac_address` into `of_get_mac_address` and find a
way how to handle the remaining use cases currently not handled in NVMEM:

 * MAC address (octet/byte) incrementation (already handled by the proposed patch)
 * MAC address stored as ascii/text (0090FEC9CBE4 and 00:90:FE:C9:CB:E4
   formats) which is currently missing but would be nice to have

I can prepare patches for that, just don't want to waste more time then really
necessary, so it would really help me if someone could tell me how this should
be implemented in NVMEM and I'll simply do it in this acceptable way and call
it a day.

Thanks!

-- ynezz
Maxime Ripard April 17, 2019, 6:05 p.m. UTC | #9
On Wed, Apr 17, 2019 at 06:06:00PM +0200, Petr Štetiar wrote:
> Maxime Ripard <maxime.ripard@bootlin.com> [2019-04-17 10:06:14]:
>
> > NVMEM is supported by of_net already
>
> Well, not anymore:
>
>  commit afa64a72b862a7a9d04f8d07fba632eaf06b23f8
>  Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>  Date:   Fri Nov 30 09:20:59 2018 +0100
>
>     of: net: kill of_get_nvmem_mac_address()
>
> Now, I'm really confused.
>
> Documentation/devicetree/bindings/net/ethernet.txt states following:
>
>  - 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;
>
> which is actually misleading and confusing. There are only two ethernet
> drivers in the tree cadence and davinci which support this properties. Well
> there's nixge, but this one is special, because it has renamed `mac-address`
> to `address` with it's own implementation in `nixge_get_nvmem_address`.
>
> All other ethernet drivers (and few others) simply use `of_get_mac_address`
> which doesn't support NVMEM and the documented nvmem-cells* properties.
>
> > so it doesn't look really necessary to create additional properties that
> > cover the same use case.
>
> NVMEM could be reused for this purpose and it seems like a way to go, probably
> if we could wire `nvmem_get_mac_address` into `of_get_mac_address` and find a
> way how to handle the remaining use cases currently not handled in NVMEM:
>
>  * MAC address (octet/byte) incrementation (already handled by the proposed patch)
>  * MAC address stored as ascii/text (0090FEC9CBE4 and 00:90:FE:C9:CB:E4
>    formats) which is currently missing but would be nice to have
>
> I can prepare patches for that, just don't want to waste more time then really
> necessary, so it would really help me if someone could tell me how this should
> be implemented in NVMEM and I'll simply do it in this acceptable way and call
> it a day.

Just send whatever you have in mind with the nvmem developpers as
recepients. They are not in this thread, so I'm not sure we can point
you in the direction they want

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Rob Herring April 30, 2019, 12:48 a.m. UTC | #10
On Tue, Apr 16, 2019 at 08:01:56PM -0700, Frank Rowand wrote:
> Hi Rob,
> 
> On 4/16/19 5:29 PM, Florian Fainelli wrote:
> > 
> > 
> > On 16/04/2019 13:05, Petr Štetiar wrote:
> >> From: John Crispin <john@phrozen.org>
> >>
> >> Many embedded devices have information such as MAC addresses stored
> >> inside MTD devices. This patch allows us to add a property inside a node
> >> describing a network interface. The new property points at a MTD
> >> partition with an offset where the MAC address can be found.
> >>
> >> This patch has originated in OpenWrt some time ago, so in order to
> >> consider usefulness of this patch, here are some real-world numbers
> >> which hopefully speak for themselves:
> >>
> >>   * mtd-mac-address                used 497 times in 357 device tree files
> >>   * mtd-mac-address-increment      used  74 times in  58 device tree files
> >>   * mtd-mac-address-increment-byte used   1 time  in   1 device tree file
> >>
> >> Signed-off-by: John Crispin <john@phrozen.org>
> >> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> >> [cleanup of the patch for upstream submission]
> >> Signed-off-by: Petr Štetiar <ynezz@true.cz>
> >> ---
> > 
> > [snip]
> > 
> >> +static const void *of_get_mac_address_mtd(struct device_node *np)
> >> +{
> >> +#ifdef CONFIG_MTD
> >> +    void *addr;
> >> +    size_t retlen;
> >> +    int size, ret;
> >> +    u8 mac[ETH_ALEN];
> >> +    phandle phandle;
> >> +    const char *part;
> >> +    const __be32 *list;
> >> +    struct mtd_info *mtd;
> >> +    struct property *prop;
> >> +    u32 mac_inc = 0;
> >> +    u32 inc_idx = ETH_ALEN-1;
> >> +    struct device_node *mtd_np = NULL;
> > 
> > Reverse christmas tree would look a bit nicer here.
> 
> Do we a variable declaration format preference for drivers/of/*?

We'd better get one. It's all the rage.

How about fallen Christmas tree:

	int a;
	bool fallen;
	char christmas_tree;
	int for_our;
	int dt;

Rob
Frank Rowand April 30, 2019, 1:15 a.m. UTC | #11
On 4/29/19 5:48 PM, Rob Herring wrote:
> On Tue, Apr 16, 2019 at 08:01:56PM -0700, Frank Rowand wrote:
>> Hi Rob,
>>
>> On 4/16/19 5:29 PM, Florian Fainelli wrote:
>>>
>>>
>>> On 16/04/2019 13:05, Petr Štetiar wrote:
>>>> From: John Crispin <john@phrozen.org>
>>>>
>>>> Many embedded devices have information such as MAC addresses stored
>>>> inside MTD devices. This patch allows us to add a property inside a node
>>>> describing a network interface. The new property points at a MTD
>>>> partition with an offset where the MAC address can be found.
>>>>
>>>> This patch has originated in OpenWrt some time ago, so in order to
>>>> consider usefulness of this patch, here are some real-world numbers
>>>> which hopefully speak for themselves:
>>>>
>>>>   * mtd-mac-address                used 497 times in 357 device tree files
>>>>   * mtd-mac-address-increment      used  74 times in  58 device tree files
>>>>   * mtd-mac-address-increment-byte used   1 time  in   1 device tree file
>>>>
>>>> Signed-off-by: John Crispin <john@phrozen.org>
>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>>> [cleanup of the patch for upstream submission]
>>>> Signed-off-by: Petr Štetiar <ynezz@true.cz>
>>>> ---
>>>
>>> [snip]
>>>
>>>> +static const void *of_get_mac_address_mtd(struct device_node *np)
>>>> +{
>>>> +#ifdef CONFIG_MTD
>>>> +    void *addr;
>>>> +    size_t retlen;
>>>> +    int size, ret;
>>>> +    u8 mac[ETH_ALEN];
>>>> +    phandle phandle;
>>>> +    const char *part;
>>>> +    const __be32 *list;
>>>> +    struct mtd_info *mtd;
>>>> +    struct property *prop;
>>>> +    u32 mac_inc = 0;
>>>> +    u32 inc_idx = ETH_ALEN-1;
>>>> +    struct device_node *mtd_np = NULL;
>>>
>>> Reverse christmas tree would look a bit nicer here.
>>
>> Do we a variable declaration format preference for drivers/of/*?
> 
> We'd better get one. It's all the rage.
> 
> How about fallen Christmas tree:
> 
> 	int a;
> 	bool fallen;
> 	char christmas_tree;
> 	int for_our;
> 	int dt;

Nice!  That is actually the most aesthetically pleasing method I have
seen.  :-)

In the future I will tell people to ignore devicetree review comments
that espouse a declaration religion.  As long as the declarations are
within reason (and sort of follow whatever style is present elsewhere
in the same file).

> 
> Rob
>

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt
index cfc376b..8a7891e 100644
--- a/Documentation/devicetree/bindings/net/ethernet.txt
+++ b/Documentation/devicetree/bindings/net/ethernet.txt
@@ -10,6 +10,11 @@  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;
+- mtd-mac-address: specify a MTD partition + offset containing array of 6 bytes
+- mtd-mac-address-increment: specify number by which we should increment the
+  MAC address stored in the MTD partition
+- mtd-mac-address-increment-byte: specify octet/byte(0-5) in the MAC address,
+  where we should increment the value, defaults to octet 5 (the last one)
 - 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;
diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index 810ab0f..01b24d6 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -11,6 +11,7 @@ 
 #include <linux/of_net.h>
 #include <linux/phy.h>
 #include <linux/export.h>
+#include <linux/mtd/mtd.h>
 
 /**
  * of_get_phy_mode - Get phy mode for given device_node
@@ -39,7 +40,7 @@  int of_get_phy_mode(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(of_get_phy_mode);
 
-static const void *of_get_mac_addr(struct device_node *np, const char *name)
+static void *of_get_mac_addr(struct device_node *np, const char *name)
 {
 	struct property *pp = of_find_property(np, name, NULL);
 
@@ -48,6 +49,78 @@  static const void *of_get_mac_addr(struct device_node *np, const char *name)
 	return NULL;
 }
 
+static const void *of_get_mac_address_mtd(struct device_node *np)
+{
+#ifdef CONFIG_MTD
+	void *addr;
+	size_t retlen;
+	int size, ret;
+	u8 mac[ETH_ALEN];
+	phandle phandle;
+	const char *part;
+	const __be32 *list;
+	struct mtd_info *mtd;
+	struct property *prop;
+	u32 mac_inc = 0;
+	u32 inc_idx = ETH_ALEN-1;
+	struct device_node *mtd_np = NULL;
+
+	list = of_get_property(np, "mtd-mac-address", &size);
+	if (!list || (size != (2 * sizeof(*list))))
+		return NULL;
+
+	phandle = be32_to_cpup(list++);
+	if (phandle)
+		mtd_np = of_find_node_by_phandle(phandle);
+
+	if (!mtd_np)
+		return NULL;
+
+	part = of_get_property(mtd_np, "label", NULL);
+	if (!part)
+		part = mtd_np->name;
+
+	mtd = get_mtd_device_nm(part);
+	if (IS_ERR(mtd))
+		return NULL;
+
+	ret = mtd_read(mtd, be32_to_cpup(list), ETH_ALEN, &retlen, mac);
+	put_mtd_device(mtd);
+
+	of_property_read_u32(np, "mtd-mac-address-increment-byte", &inc_idx);
+	if (inc_idx > ETH_ALEN-1)
+		return NULL;
+
+	if (!of_property_read_u32(np, "mtd-mac-address-increment", &mac_inc))
+		mac[inc_idx] += mac_inc;
+
+	if (!is_valid_ether_addr(mac))
+		return NULL;
+
+	addr = of_get_mac_addr(np, "mac-address");
+	if (addr) {
+		memcpy(addr, mac, ETH_ALEN);
+		return addr;
+	}
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop)
+		return NULL;
+
+	prop->name = "mac-address";
+	prop->length = ETH_ALEN;
+	prop->value = kmemdup(mac, ETH_ALEN, GFP_KERNEL);
+	if (!prop->value || of_add_property(np, prop))
+		goto free;
+
+	return prop->value;
+free:
+	kfree(prop->value);
+	kfree(prop);
+#endif
+	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
@@ -65,11 +138,18 @@  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.
+ *
+ * If a mtd-mac-address property exists, try to fetch the MAC address from the
+ * specified MTD device, and store it as a 'mac-address' property.
 */
 const void *of_get_mac_address(struct device_node *np)
 {
 	const void *addr;
 
+	addr = of_get_mac_address_mtd(np);
+	if (addr)
+		return addr;
+
 	addr = of_get_mac_addr(np, "mac-address");
 	if (addr)
 		return addr;