| Message ID | 20210412174718.17382-3-michael@walle.cc (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f10843e04a075202dbb39dfcee047e3a2fdf5a8d |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | of: net: support non-platform devices in of_get_mac_address() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 51 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
On Mon, Apr 12, 2021 at 07:47:18PM +0200, Michael Walle wrote: > of_get_mac_address() already supports fetching the MAC address by an > nvmem provider. But until now, it was just working for platform devices. > Esp. it was not working for DSA ports and PCI devices. It gets more > common that PCI devices have a device tree binding since SoCs contain > integrated root complexes. > > Use the nvmem of_* binding to fetch the nvmem cells by a struct > device_node. We still have to try to read the cell by device first > because there might be a nvmem_cell_lookup associated with that device. > > Signed-off-by: Michael Walle <michael@walle.cc> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Mon, 2021-04-12 at 19:47 +0200, Michael Walle wrote: > > /** > * of_get_phy_mode - Get phy mode for given device_node > @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr) > static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) > { > struct platform_device *pdev = of_find_device_by_node(np); > + struct nvmem_cell *cell; > + const void *mac; > + size_t len; > int ret; > > - if (!pdev) > - return -ENODEV; > + /* Try lookup by device first, there might be a nvmem_cell_lookup > + * associated with a given device. > + */ > + if (pdev) { > + ret = nvmem_get_mac_address(&pdev->dev, addr); > + put_device(&pdev->dev); > + return ret; > + } > + This smells like the wrong band aid :) Any struct device can contain an OF node pointer these days. This seems all backwards. I think we are dealing with bad evolution. We need to do a lookup for the device because we get passed an of_node. We should just get passed a device here... or rather stop calling of_get_mac_addr() from all those drivers and instead call eth_platform_get_mac_address() which in turns calls of_get_mac_addr(). Then the nvmem stuff gets put in eth_platform_get_mac_address(). of_get_mac_addr() becomes a low-level thingy that most drivers don't care about. Cheers, Ben.
Am 2021-04-16 05:24, schrieb Benjamin Herrenschmidt: > On Mon, 2021-04-12 at 19:47 +0200, Michael Walle wrote: >> >> /** >> * of_get_phy_mode - Get phy mode for given device_node >> @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np, >> const char *name, u8 *addr) >> static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) >> { >> struct platform_device *pdev = of_find_device_by_node(np); >> + struct nvmem_cell *cell; >> + const void *mac; >> + size_t len; >> int ret; >> >> - if (!pdev) >> - return -ENODEV; >> + /* Try lookup by device first, there might be a >> nvmem_cell_lookup >> + * associated with a given device. >> + */ >> + if (pdev) { >> + ret = nvmem_get_mac_address(&pdev->dev, addr); >> + put_device(&pdev->dev); >> + return ret; >> + } >> + > > This smells like the wrong band aid :) > > Any struct device can contain an OF node pointer these days. But not all nodes might have an associated device, see DSA for example. And as the name suggests of_get_mac_address() operates on a node. So if a driver calls of_get_mac_address() it should work on the node. What is wrong IMHO, is that the ethernet drivers where the corresponding board has a nvmem_cell_lookup registered is calling of_get_mac_address(node). It should rather call eth_get_mac_address(dev) in the first place. One would need to figure out if there is an actual device (with an assiciated of_node), then call eth_get_mac_address(dev) and if there isn't a device call of_get_mac_address(node). But I don't know if that is easy to figure out. Well, one could start with just the device where nvmem_cell_lookup is used. Then we could drop the workaround above. > This seems all backwards. I think we are dealing with bad evolution. > > We need to do a lookup for the device because we get passed an of_node. > We should just get passed a device here... or rather stop calling > of_get_mac_addr() from all those drivers and instead call > eth_platform_get_mac_address() which in turns calls of_get_mac_addr(). > > Then the nvmem stuff gets put in eth_platform_get_mac_address(). > > of_get_mac_addr() becomes a low-level thingy that most drivers don't > care about. The NVMEM thing is just another (optional) way how the MAC address is fetched from the device tree. Thus, if the drivers have the of_get_mac_address() call they should automatically get the NVMEM method, too. -michael
On Fri, Apr 16, 2021 at 2:30 AM Michael Walle <michael@walle.cc> wrote: > > Am 2021-04-16 05:24, schrieb Benjamin Herrenschmidt: > > On Mon, 2021-04-12 at 19:47 +0200, Michael Walle wrote: > >> > >> /** > >> * of_get_phy_mode - Get phy mode for given device_node > >> @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np, > >> const char *name, u8 *addr) > >> static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) > >> { > >> struct platform_device *pdev = of_find_device_by_node(np); > >> + struct nvmem_cell *cell; > >> + const void *mac; > >> + size_t len; > >> int ret; > >> > >> - if (!pdev) > >> - return -ENODEV; > >> + /* Try lookup by device first, there might be a > >> nvmem_cell_lookup > >> + * associated with a given device. > >> + */ > >> + if (pdev) { > >> + ret = nvmem_get_mac_address(&pdev->dev, addr); > >> + put_device(&pdev->dev); > >> + return ret; > >> + } > >> + > > > > This smells like the wrong band aid :) > > > > Any struct device can contain an OF node pointer these days. > > But not all nodes might have an associated device, see DSA for example. I believe what Ben is saying and what I said earlier is going from dev -> OF node is right and OF node -> dev is wrong. If you only have an OF node, then use an of_* function. > And as the name suggests of_get_mac_address() operates on a node. So > if a driver calls of_get_mac_address() it should work on the node. What > is wrong IMHO, is that the ethernet drivers where the corresponding > board > has a nvmem_cell_lookup registered is calling of_get_mac_address(node). > It should rather call eth_get_mac_address(dev) in the first place. > > One would need to figure out if there is an actual device (with an > assiciated of_node), then call eth_get_mac_address(dev) and if there > isn't a device call of_get_mac_address(node). Yes, I think we're all in agreement. > But I don't know if that is easy to figure out. Well, one could start > with just the device where nvmem_cell_lookup is used. Then we could > drop the workaround above. Start with the ones just passing dev.of_node directly: $ git grep 'of_get_mac_address(.*of_node)' drivers/net/ethernet/aeroflex/greth.c: addr = of_get_mac_address(ofdev->dev.of_node); drivers/net/ethernet/altera/altera_tse_main.c: macaddr = of_get_mac_address(pdev->dev.of_node); drivers/net/ethernet/arc/emac_main.c: mac_addr = of_get_mac_address(dev->of_node); drivers/net/ethernet/broadcom/bgmac-bcma.c: mac = of_get_mac_address(bgmac->dev->of_node); drivers/net/ethernet/cavium/octeon/octeon_mgmt.c: mac = of_get_mac_address(pdev->dev.of_node); drivers/net/ethernet/ethoc.c: mac = of_get_mac_address(pdev->dev.of_node); drivers/net/ethernet/ezchip/nps_enet.c: mac_addr = of_get_mac_address(dev->of_node); drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c: mac_addr = of_get_mac_address(ofdev->dev.of_node); drivers/net/ethernet/marvell/pxa168_eth.c: mac_addr = of_get_mac_address(pdev->dev.of_node); drivers/net/ethernet/marvell/sky2.c: iap = of_get_mac_address(hw->pdev->dev.of_node); drivers/net/ethernet/mediatek/mtk_eth_soc.c: mac_addr = of_get_mac_address(mac->of_node); drivers/net/ethernet/microchip/lan743x_main.c: mac_addr = of_get_mac_address(pdev->dev.of_node); drivers/net/ethernet/qualcomm/qca_spi.c: mac = of_get_mac_address(spi->dev.of_node); drivers/net/ethernet/qualcomm/qca_uart.c: mac = of_get_mac_address(serdev->dev.of_node); drivers/net/ethernet/wiznet/w5100-spi.c: const void *mac = of_get_mac_address(spi->dev.of_node); drivers/net/ethernet/xilinx/xilinx_axienet_main.c: mac_addr = of_get_mac_address(pdev->dev.of_node); drivers/net/ethernet/xilinx/xilinx_emaclite.c: mac_address = of_get_mac_address(ofdev->dev.of_node); drivers/net/wireless/ralink/rt2x00/rt2x00dev.c: mac_addr = of_get_mac_address(rt2x00dev->dev->of_node); drivers/staging/octeon/ethernet.c: mac = of_get_mac_address(priv->of_node); drivers/staging/wfx/main.c: macaddr = of_get_mac_address(wdev->dev->of_node); net/ethernet/eth.c: addr = of_get_mac_address(dev->of_node); Then this will find most of the rest: git grep -W 'of_get_mac_address([a-z]*)'| grep -E '(node|np)' Rob
Am 2021-04-16 17:19, schrieb Rob Herring: > On Fri, Apr 16, 2021 at 2:30 AM Michael Walle <michael@walle.cc> wrote: >> >> Am 2021-04-16 05:24, schrieb Benjamin Herrenschmidt: >> > On Mon, 2021-04-12 at 19:47 +0200, Michael Walle wrote: >> >> >> >> /** >> >> * of_get_phy_mode - Get phy mode for given device_node >> >> @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np, >> >> const char *name, u8 *addr) >> >> static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) >> >> { >> >> struct platform_device *pdev = of_find_device_by_node(np); >> >> + struct nvmem_cell *cell; >> >> + const void *mac; >> >> + size_t len; >> >> int ret; >> >> >> >> - if (!pdev) >> >> - return -ENODEV; >> >> + /* Try lookup by device first, there might be a >> >> nvmem_cell_lookup >> >> + * associated with a given device. >> >> + */ >> >> + if (pdev) { >> >> + ret = nvmem_get_mac_address(&pdev->dev, addr); >> >> + put_device(&pdev->dev); >> >> + return ret; >> >> + } >> >> + >> > >> > This smells like the wrong band aid :) >> > >> > Any struct device can contain an OF node pointer these days. >> >> But not all nodes might have an associated device, see DSA for >> example. > > I believe what Ben is saying and what I said earlier is going from dev > -> OF node is right and OF node -> dev is wrong. If you only have an > OF node, then use an of_* function. > >> And as the name suggests of_get_mac_address() operates on a node. So >> if a driver calls of_get_mac_address() it should work on the node. >> What >> is wrong IMHO, is that the ethernet drivers where the corresponding >> board >> has a nvmem_cell_lookup registered is calling >> of_get_mac_address(node). >> It should rather call eth_get_mac_address(dev) in the first place. >> >> One would need to figure out if there is an actual device (with an >> assiciated of_node), then call eth_get_mac_address(dev) and if there >> isn't a device call of_get_mac_address(node). > > Yes, I think we're all in agreement. > >> But I don't know if that is easy to figure out. Well, one could start >> with just the device where nvmem_cell_lookup is used. Then we could >> drop the workaround above. > > Start with the ones just passing dev.of_node directly: > > $ git grep 'of_get_mac_address(.*of_node)' [..] Before I'll try to come up with a patch for this, I'd like to get your opinion on it. (1) replacing of_get_mac_address(node) with eth_get_mac_address(dev) might sometimes lead to confusing comments like in drivers/net/ethernet/allwinner/sun4i-emac.c: /* Read MAC-address from DT */ ret = of_get_mac_address(np, ndev->dev_addr); Do we live with that or should the new name somehow reflect that it is taken from the device tree. (2) What do you think of eth_get_mac_address(ndev). That is, the second argument is missing and ndev->dev_addr is used. I'm unsure about it. We'd still need a second function for drivers which don't write ndev->dev_addr directly, but have some custom logic in between. OTOH it would be like eth_hw_addr_random(ndev). -michael
On Mon, 2021-04-26 at 12:54 +0200, Michael Walle wrote: > Before I'll try to come up with a patch for this, I'd like to get > your opinion on it. > > (1) replacing of_get_mac_address(node) with eth_get_mac_address(dev) > might sometimes lead to confusing comments like in > drivers/net/ethernet/allwinner/sun4i-emac.c: > > /* Read MAC-address from DT */ > ret = of_get_mac_address(np, ndev->dev_addr); You could leave it or turn it into "from platform", doesn't matter... > (2) What do you think of eth_get_mac_address(ndev). That is, the Not sure what you mean, eth_platform_get_mac_address() takes the address as an argument. I think what you want is a consolidated nvmem_get_mac_address + eth_platform_get_mac_address that takes a device, which would have no requirement of the bus_type at all. Cheers, Ben.
Am 2021-04-27 01:44, schrieb Benjamin Herrenschmidt: > On Mon, 2021-04-26 at 12:54 +0200, Michael Walle wrote: >> (2) What do you think of eth_get_mac_address(ndev). That is, the > > Not sure what you mean, eth_platform_get_mac_address() takes the > address as an argument. I think what you want is a consolidated > nvmem_get_mac_address + eth_platform_get_mac_address that takes a > device, which would have no requirement of the bus_type at all. Sure. What I meant was the following: eth_get_mac_address(struct net_device *ndev) vs. eth_get_mac_address(struct device *dev, u8 *mac_buf) The first would assume the destination is ndev->dev_addr (which is true for most of the calls, but not all). -michael
diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c index cb77b774bf76..dbac3a172a11 100644 --- a/drivers/of/of_net.c +++ b/drivers/of/of_net.c @@ -11,6 +11,7 @@ #include <linux/phy.h> #include <linux/export.h> #include <linux/device.h> +#include <linux/nvmem-consumer.h> /** * of_get_phy_mode - Get phy mode for given device_node @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr) static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) { struct platform_device *pdev = of_find_device_by_node(np); + struct nvmem_cell *cell; + const void *mac; + size_t len; int ret; - if (!pdev) - return -ENODEV; + /* Try lookup by device first, there might be a nvmem_cell_lookup + * associated with a given device. + */ + if (pdev) { + ret = nvmem_get_mac_address(&pdev->dev, addr); + put_device(&pdev->dev); + return 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)) { + kfree(mac); + return -EINVAL; + } - ret = nvmem_get_mac_address(&pdev->dev, addr); - put_device(&pdev->dev); + memcpy(addr, mac, ETH_ALEN); + kfree(mac); - return ret; + return 0; } /**
of_get_mac_address() already supports fetching the MAC address by an nvmem provider. But until now, it was just working for platform devices. Esp. it was not working for DSA ports and PCI devices. It gets more common that PCI devices have a device tree binding since SoCs contain integrated root complexes. Use the nvmem of_* binding to fetch the nvmem cells by a struct device_node. We still have to try to read the cell by device first because there might be a nvmem_cell_lookup associated with that device. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/of/of_net.c | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-)