Message ID | 20180830004046.9417-2-mdf@kernel.org |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Series | nixge: fixed-link support | expand |
On Wed, Aug 29, 2018 at 05:40:44PM -0700, Moritz Fischer wrote: > Add support for fixed-link cases where no MDIO is > actually required to run the device. > In that case no MDIO bus is instantiated since the > actual registers are not available in hardware. Hi Moritz There are a few different use cases here: The hardware is missing MDIO - You need fixed-link. The hardware has MDIO, but you don't have a PHY connected on it, and use fixed link. The hardware has MDIO, and it is used e.g. for an Ethernet switch, or a PHY for another Ethernet interface. Plus you need fixed link. The binding typically looks like: &fec1 { phy-mode = "rmii"; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_fec1>; status = "okay"; fixed-link { speed = <100>; full-duplex; }; mdio1: mdio { #address-cells = <1>; #size-cells = <0>; status = "okay"; switch0: switch0@0 { compatible = "marvell,mv88e6085"; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_switch>; reg = <0>; eeprom-length = <512>; interrupt-parent = <&gpio3>; It is important you have the mdio subnode, with PHYs and switches as children. The driver currently gets this wrong, it uses pdev->dev.of_node. So the first patch should be to extend this behaviour. Look for a child node called mdio. If it exists, call nixge_mdio_setup() passing that child. Otherwise continue using pdev->dev.of_node, so you don't break backwards compatibility. Then a patch adding support for fixed-link. If the mdio child node exists, you still need to register the MDIO bus. If there is no child node, but there is a fixed-link, skip registering the mdio bus with pdev->dev.of_node. Andrew
Hi Andrew, On Wed, Aug 29, 2018 at 8:04 PM, Andrew Lunn <andrew@lunn.ch> wrote: > On Wed, Aug 29, 2018 at 05:40:44PM -0700, Moritz Fischer wrote: >> Add support for fixed-link cases where no MDIO is >> actually required to run the device. >> In that case no MDIO bus is instantiated since the >> actual registers are not available in hardware. > > Hi Moritz > > There are a few different use cases here: > > The hardware is missing MDIO - You need fixed-link. Agreed. > > The hardware has MDIO, but you don't have a PHY connected on it, and > use fixed link. Since it's an FPGA design in that case we'd probably build the hardware without MDIO to save resources. > The hardware has MDIO, and it is used e.g. for an Ethernet switch, or > a PHY for another Ethernet interface. Plus you need fixed link. We haven't had that yet but I can see that happen. > > The binding typically looks like: > > &fec1 { > phy-mode = "rmii"; > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_fec1>; > status = "okay"; > > fixed-link { > speed = <100>; > full-duplex; > }; > > mdio1: mdio { > #address-cells = <1>; > #size-cells = <0>; > status = "okay"; > > switch0: switch0@0 { > compatible = "marvell,mv88e6085"; > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_switch>; > reg = <0>; > eeprom-length = <512>; > interrupt-parent = <&gpio3>; > > It is important you have the mdio subnode, with PHYs and switches as > children. The driver currently gets this wrong, it uses > pdev->dev.of_node. Oh, whoops. Yeah I should look into that. Any good examples of drivers doing it right? Is the one going with the DT snippet above a good example? > > So the first patch should be to extend this behaviour. Look for a > child node called mdio. If it exists, call nixge_mdio_setup() passing > that child. Otherwise continue using pdev->dev.of_node, so you don't > break backwards compatibility. Ok will do. > > Then a patch adding support for fixed-link. If the mdio child node > exists, you still need to register the MDIO bus. If there is no child > node, but there is a fixed-link, skip registering the mdio bus with > pdev->dev.of_node. > > Andrew Thanks for your feedback, much appreciated! Moritz
Hi Moritz, I love your patch! Yet something to improve: [auto build test ERROR on net-next/master] url: https://github.com/0day-ci/linux/commits/Moritz-Fischer/nixge-fixed-link-support/20180830-150857 config: i386-allmodconfig (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 Note: the linux-review/Moritz-Fischer/nixge-fixed-link-support/20180830-150857 HEAD 300a42d41dc76f270bff67d414dc7fc127d3f17c builds fine. It only hurts bisectibility. All errors (new ones prefixed by >>): drivers/net/ethernet/ni/nixge.c: In function 'nixge_mdio_setup': drivers/net/ethernet/ni/nixge.c:1221:6: warning: unused variable 'err' [-Wunused-variable] int err; ^~~ drivers/net/ethernet/ni/nixge.c: In function 'nixge_remove': >> drivers/net/ethernet/ni/nixge.c:1366:6: error: 'np' undeclared (first use in this function); did you mean 'up'? if (np && of_phy_is_fixed_link(np)) ^~ up drivers/net/ethernet/ni/nixge.c:1366:6: note: each undeclared identifier is reported only once for each function it appears in vim +1366 drivers/net/ethernet/ni/nixge.c 1217 1218 static int nixge_mdio_setup(struct nixge_priv *priv, struct device_node *np) 1219 { 1220 struct mii_bus *bus; > 1221 int err; 1222 1223 bus = devm_mdiobus_alloc(priv->dev); 1224 if (!bus) 1225 return -ENOMEM; 1226 1227 snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(priv->dev)); 1228 bus->priv = priv; 1229 bus->name = "nixge_mii_bus"; 1230 bus->read = nixge_mdio_read; 1231 bus->write = nixge_mdio_write; 1232 bus->parent = priv->dev; 1233 1234 priv->mii_bus = bus; 1235 1236 return of_mdiobus_register(bus, np); 1237 } 1238 1239 static void *nixge_get_nvmem_address(struct device *dev) 1240 { 1241 struct nvmem_cell *cell; 1242 size_t cell_size; 1243 char *mac; 1244 1245 cell = nvmem_cell_get(dev, "address"); 1246 if (IS_ERR(cell)) 1247 return NULL; 1248 1249 mac = nvmem_cell_read(cell, &cell_size); 1250 nvmem_cell_put(cell); 1251 1252 return mac; 1253 } 1254 1255 static int nixge_probe(struct platform_device *pdev) 1256 { 1257 struct nixge_priv *priv; 1258 struct net_device *ndev; 1259 struct resource *dmares; 1260 struct device_node *np; 1261 const u8 *mac_addr; 1262 int err; 1263 1264 ndev = alloc_etherdev(sizeof(*priv)); 1265 if (!ndev) 1266 return -ENOMEM; 1267 1268 np = pdev->dev.of_node; 1269 1270 platform_set_drvdata(pdev, ndev); 1271 SET_NETDEV_DEV(ndev, &pdev->dev); 1272 1273 ndev->features = NETIF_F_SG; 1274 ndev->netdev_ops = &nixge_netdev_ops; 1275 ndev->ethtool_ops = &nixge_ethtool_ops; 1276 1277 /* MTU range: 64 - 9000 */ 1278 ndev->min_mtu = 64; 1279 ndev->max_mtu = NIXGE_JUMBO_MTU; 1280 1281 mac_addr = nixge_get_nvmem_address(&pdev->dev); 1282 if (mac_addr && is_valid_ether_addr(mac_addr)) { 1283 ether_addr_copy(ndev->dev_addr, mac_addr); 1284 kfree(mac_addr); 1285 } else { 1286 eth_hw_addr_random(ndev); 1287 } 1288 1289 priv = netdev_priv(ndev); 1290 priv->ndev = ndev; 1291 priv->dev = &pdev->dev; 1292 1293 netif_napi_add(ndev, &priv->napi, nixge_poll, NAPI_POLL_WEIGHT); 1294 1295 dmares = platform_get_resource(pdev, IORESOURCE_MEM, 0); 1296 priv->dma_regs = devm_ioremap_resource(&pdev->dev, dmares); 1297 if (IS_ERR(priv->dma_regs)) { 1298 netdev_err(ndev, "failed to map dma regs\n"); 1299 return PTR_ERR(priv->dma_regs); 1300 } 1301 priv->ctrl_regs = priv->dma_regs + NIXGE_REG_CTRL_OFFSET; 1302 __nixge_hw_set_mac_address(ndev); 1303 1304 priv->tx_irq = platform_get_irq_byname(pdev, "tx"); 1305 if (priv->tx_irq < 0) { 1306 netdev_err(ndev, "could not find 'tx' irq"); 1307 return priv->tx_irq; 1308 } 1309 1310 priv->rx_irq = platform_get_irq_byname(pdev, "rx"); 1311 if (priv->rx_irq < 0) { 1312 netdev_err(ndev, "could not find 'rx' irq"); 1313 return priv->rx_irq; 1314 } 1315 1316 priv->coalesce_count_rx = XAXIDMA_DFT_RX_THRESHOLD; 1317 priv->coalesce_count_tx = XAXIDMA_DFT_TX_THRESHOLD; 1318 1319 if (np) { 1320 err = nixge_of_get_phy(priv, np); 1321 if (err) 1322 goto free_netdev; 1323 } 1324 1325 /* only if it's not a fixed link, do we care about MDIO at all */ 1326 if (priv->phy_node && !of_phy_is_fixed_link(np)) { 1327 err = nixge_mdio_setup(priv, np); 1328 if (err) { 1329 dev_err(&pdev->dev, "error registering mdio bus"); 1330 goto free_phy; 1331 } 1332 } 1333 1334 err = register_netdev(priv->ndev); 1335 if (err) { 1336 netdev_err(ndev, "register_netdev() error (%i)\n", err); 1337 goto unregister_mdio; 1338 } 1339 1340 return 0; 1341 1342 unregister_mdio: 1343 if (priv->mii_bus) 1344 mdiobus_unregister(priv->mii_bus); 1345 free_phy: 1346 if (np && of_phy_is_fixed_link(np)) { 1347 of_phy_deregister_fixed_link(np); 1348 of_node_put(np); 1349 } 1350 free_netdev: 1351 free_netdev(ndev); 1352 1353 return err; 1354 } 1355 1356 static int nixge_remove(struct platform_device *pdev) 1357 { 1358 struct net_device *ndev = platform_get_drvdata(pdev); 1359 struct nixge_priv *priv = netdev_priv(ndev); 1360 1361 unregister_netdev(ndev); 1362 1363 if (priv->mii_bus) 1364 mdiobus_unregister(priv->mii_bus); 1365 > 1366 if (np && of_phy_is_fixed_link(np)) 1367 of_phy_deregister_fixed_link(np); 1368 1369 free_netdev(ndev); 1370 1371 return 0; 1372 } 1373 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
> > The hardware has MDIO, but you don't have a PHY connected on it, and > > use fixed link. > > Since it's an FPGA design in that case we'd probably build the hardware without > MDIO to save resources. You can save resources, but is it worth the complexity else where, like in the software? > > It is important you have the mdio subnode, with PHYs and switches as > > children. The driver currently gets this wrong, it uses > > pdev->dev.of_node. > > Oh, whoops. Yes, and i also missed it. I generally review all new network drivers and look at their MDIO and PHY code. > Any good examples of drivers doing it right? Is the one going with > the DT snippet above a good example? That comes from the Freescale fec_main.c. It only supports DT, and always uses of_mdiobus_register. You need to be a bit more flexible for when you don't have DT. I'm not sure there are good example of this, since they either don't need this flexibility, or they get it wrong :-( Andrew
Andrew, On Thu, Aug 30, 2018 at 10:54 AM, Andrew Lunn <andrew@lunn.ch> wrote: >> > The hardware has MDIO, but you don't have a PHY connected on it, and >> > use fixed link. >> >> Since it's an FPGA design in that case we'd probably build the hardware without >> MDIO to save resources. > > You can save resources, but is it worth the complexity else where, > like in the software? Depends. For now I definitely have build versions that don't have MDIO regs there. I might be able to chat with HW folks ... > >> > It is important you have the mdio subnode, with PHYs and switches as >> > children. The driver currently gets this wrong, it uses >> > pdev->dev.of_node. >> >> Oh, whoops. > > Yes, and i also missed it. I generally review all new network drivers > and look at their MDIO and PHY code. I had looked at macb as an example and there were a bunch of other cases where there was no 'mdio' subnode. > >> Any good examples of drivers doing it right? Is the one going with >> the DT snippet above a good example? > > That comes from the Freescale fec_main.c. It only supports DT, and > always uses of_mdiobus_register. You need to be a bit more flexible > for when you don't have DT. I'm not sure there are good example of > this, since they either don't need this flexibility, or they get it > wrong :-( Alright, no problem. I'll take a stab at it. And come back with a v2 ;-) Need to look at your response in the other patch. Cheers, Moritz
diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c index 74cf52e3fb09..670249313ff0 100644 --- a/drivers/net/ethernet/ni/nixge.c +++ b/drivers/net/ethernet/ni/nixge.c @@ -1189,9 +1189,36 @@ static int nixge_mdio_write(struct mii_bus *bus, int phy_id, int reg, u16 val) return err; } +static int nixge_of_get_phy(struct nixge_priv *priv, struct device_node *np) +{ + priv->phy_mode = of_get_phy_mode(np); + if (priv->phy_mode < 0) { + dev_err(priv->dev, "not find \"phy-mode\" property\n"); + return -EINVAL; + } + + if (of_phy_is_fixed_link(np)) { + if (of_phy_register_fixed_link(np) < 0) { + dev_err(priv->dev, "broken fixed link spec\n"); + return -EINVAL; + } + + priv->phy_node = of_node_get(np); + } else { + priv->phy_node = of_parse_phandle(np, "phy-handle", 0); + if (!priv->phy_node) { + dev_err(priv->dev, "not find \"phy-handle\" property\n"); + return -EINVAL; + } + } + + return 0; +} + static int nixge_mdio_setup(struct nixge_priv *priv, struct device_node *np) { struct mii_bus *bus; + int err; bus = devm_mdiobus_alloc(priv->dev); if (!bus) @@ -1230,6 +1257,7 @@ static int nixge_probe(struct platform_device *pdev) struct nixge_priv *priv; struct net_device *ndev; struct resource *dmares; + struct device_node *np; const u8 *mac_addr; int err; @@ -1237,6 +1265,8 @@ static int nixge_probe(struct platform_device *pdev) if (!ndev) return -ENOMEM; + np = pdev->dev.of_node; + platform_set_drvdata(pdev, ndev); SET_NETDEV_DEV(ndev, &pdev->dev); @@ -1286,24 +1316,19 @@ static int nixge_probe(struct platform_device *pdev) priv->coalesce_count_rx = XAXIDMA_DFT_RX_THRESHOLD; priv->coalesce_count_tx = XAXIDMA_DFT_TX_THRESHOLD; - err = nixge_mdio_setup(priv, pdev->dev.of_node); - if (err) { - netdev_err(ndev, "error registering mdio bus"); - goto free_netdev; - } - - priv->phy_mode = of_get_phy_mode(pdev->dev.of_node); - if (priv->phy_mode < 0) { - netdev_err(ndev, "not find \"phy-mode\" property\n"); - err = -EINVAL; - goto unregister_mdio; + if (np) { + err = nixge_of_get_phy(priv, np); + if (err) + goto free_netdev; } - priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0); - if (!priv->phy_node) { - netdev_err(ndev, "not find \"phy-handle\" property\n"); - err = -EINVAL; - goto unregister_mdio; + /* only if it's not a fixed link, do we care about MDIO at all */ + if (priv->phy_node && !of_phy_is_fixed_link(np)) { + err = nixge_mdio_setup(priv, np); + if (err) { + dev_err(&pdev->dev, "error registering mdio bus"); + goto free_phy; + } } err = register_netdev(priv->ndev); @@ -1315,8 +1340,13 @@ static int nixge_probe(struct platform_device *pdev) return 0; unregister_mdio: - mdiobus_unregister(priv->mii_bus); - + if (priv->mii_bus) + mdiobus_unregister(priv->mii_bus); +free_phy: + if (np && of_phy_is_fixed_link(np)) { + of_phy_deregister_fixed_link(np); + of_node_put(np); + } free_netdev: free_netdev(ndev); @@ -1330,7 +1360,11 @@ static int nixge_remove(struct platform_device *pdev) unregister_netdev(ndev); - mdiobus_unregister(priv->mii_bus); + if (priv->mii_bus) + mdiobus_unregister(priv->mii_bus); + + if (np && of_phy_is_fixed_link(np)) + of_phy_deregister_fixed_link(np); free_netdev(ndev);
Add support for fixed-link cases where no MDIO is actually required to run the device. In that case no MDIO bus is instantiated since the actual registers are not available in hardware. Signed-off-by: Moritz Fischer <mdf@kernel.org> --- drivers/net/ethernet/ni/nixge.c | 72 ++++++++++++++++++++++++--------- 1 file changed, 53 insertions(+), 19 deletions(-)