| Message ID | 20210412174718.17382-1-michael@walle.cc |
|---|---|
| Headers | show |
| Series | of: net: support non-platform devices in of_get_mac_address() | expand |
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
Hello: This series was applied to netdev/net-next.git (refs/heads/master): On Mon, 12 Apr 2021 19:47:16 +0200 you wrote: > of_get_mac_address() is commonly used to fetch the MAC address > from the device tree. It also supports reading it from a NVMEM > provider. But the latter is only possible for platform devices, > because only platform devices are searched for a matching device > node. > > Add a second method to fetch the NVMEM cell by a device tree node > instead of a "struct device". > > [...] Here is the summary with links: - [net-next,v4,1/2] of: net: pass the dst buffer to of_get_mac_address() https://git.kernel.org/netdev/net-next/c/83216e3988cd - [net-next,v4,2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices https://git.kernel.org/netdev/net-next/c/f10843e04a07 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
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
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.