Skip to contentSkip to conversation

nixge: fixed-link support

PATCH In review Moritz Fischer 5 participants 13 messages Aug 30, 2018

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Moritz Fischer

author
#0 lore raw

cover nixge: fixed-link support

Hi,

this series adds support for using nixge with fixed-link nodes, as well as an early attempt to support nixge as a subdevice of another device.

This series goes on top of: https://lkml.org/lkml/2018/8/28/1011

Patch 1: Adds of based fixed-link support, and hopefully isn't too
controversial barring grave mistakes.

Patch 2: Is an attempt at making sure that nixge works as a subdevice
of another (pci) device without a PHY. Note that same as Patch 3 the actual platform data might still change since the parent device driver is still under development. So this is more of an RFC, too.

Patch 3: Is more of an RFC at this point since the PCI parent device is
still under development, but required to run with IOMMU support. Feedback on this one would be appreciated.

Thanks for your input,

Moritz

Moritz Fischer (3):
net: nixge: Add support for fixed-link subnodes
net: nixge: Add support for having nixge as subdevice
net: nixge: Use sysdev instead of ndev->dev.parent for DMA

 drivers/net/ethernet/ni/nixge.c     | 187 ++++++++++++++++++++++------
 include/linux/platform_data/nixge.h |  19 +++
 2 files changed, 165 insertions(+), 41 deletions(-)
 create mode 100644 include/linux/platform_data/nixge.h
Signature · 1 line
2.18.0
author
#1 lore raw

PATCH 1/3 net: nixge: Add support for fixed-link subnodes

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 <[email protected]>
---
 drivers/net/ethernet/ni/nixge.c | 72 ++++++++++++++++++++++++---------
 1 file changed, 53 insertions(+), 19 deletions(-)
drivers/net/ethernet/ni/nixge.c +53 -19
index 74cf52e3fb09..670249313ff0 100644
@@ -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);
Signature · 1 line
2.18.0
author
#7 lore raw

PATCH 2/3 net: nixge: Add support for having nixge as subdevice

In reply to Moritz Fischer

Add support for instantiating nixge as subdevice using fixed-link and platform data to configure it.

Signed-off-by Moritz Fischer <[email protected]>

---

Hi,

not this patch is still in the early stages, and as the rest of this series goes on top of [1].

The actual platform data might still change since the parent device driver is still under development.

Thanks for your time,

Moritz

[1] https://lkml.org/lkml/2018/8/28/1011

---
 drivers/net/ethernet/ni/nixge.c     | 71 ++++++++++++++++++++++++++---
 include/linux/platform_data/nixge.h | 19 ++++++++
 2 files changed, 83 insertions(+), 7 deletions(-)
 create mode 100644 include/linux/platform_data/nixge.h
drivers/net/ethernet/ni/nixge.c +64 -7
index 670249313ff0..fd8e5b02c459 100644
@@ -7,6 +7,8 @@
 #include <linux/etherdevice.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
+#include <linux/phy_fixed.h>
+#include <linux/platform_data/nixge.h>
 #include <linux/of_address.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>

@@ -167,6 +169,7 @@ struct nixge_priv {
 	/* Connection to PHY device */
 	struct device_node *phy_node;
 	phy_interface_t		phy_mode;
+	struct phy_device *phydev;
 
 	int link;
 	unsigned int speed;

@@ -859,15 +862,25 @@ static void nixge_dma_err_handler(unsigned long data)
 static int nixge_open(struct net_device *ndev)
 {
 	struct nixge_priv *priv = netdev_priv(ndev);
-	struct phy_device *phy;
+	struct phy_device *phy = NULL;
 	int ret;
 
 	nixge_device_reset(ndev);
 
-	phy = of_phy_connect(ndev, priv->phy_node,
-			     &nixge_handle_link_change, 0, priv->phy_mode);
-	if (!phy)
-		return -ENODEV;
+	if (priv->dev->of_node) {
+		phy = of_phy_connect(ndev, priv->phy_node,
+				     &nixge_handle_link_change, 0,
+				     priv->phy_mode);
+		if (!phy)
+			return -ENODEV;
+	} else if (priv->phydev) {
+		ret = phy_connect_direct(ndev, priv->phydev,
+					 &nixge_handle_link_change,
+					 priv->phy_mode);
+		if (ret)
+			return ret;
+		phy = priv->phydev;
+	}
 
 	phy_start(phy);
 

@@ -1215,10 +1228,41 @@ static int nixge_of_get_phy(struct nixge_priv *priv, struct device_node *np)
 	return 0;
 }
 
+static int nixge_pdata_get_phy(struct nixge_priv *priv,
+			       struct nixge_platform_data *pdata)
+{
+	struct phy_device *phy = NULL;
+
+	if (!pdata)
+		return -EINVAL;
+
+	if (pdata && pdata->phy_interface == PHY_INTERFACE_MODE_NA) {
+		struct fixed_phy_status fphy_status = {
+			.link = 1,
+			.duplex = pdata->phy_duplex,
+			.speed = pdata->phy_speed,
+			.pause = 0,
+			.asym_pause = 0,
+		};
+
+		/* TODO: Pull out GPIO from pdata */
+		phy = fixed_phy_register(PHY_POLL, &fphy_status, -1,
+					 NULL);
+		if (IS_ERR_OR_NULL(phy)) {
+			dev_err(priv->dev,
+				"failed to register fixed PHY device\n");
+			return -ENODEV;
+		}
+	}
+	priv->phy_mode = pdata->phy_interface;
+	priv->phydev = phy;
+
+	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)

@@ -1254,6 +1298,7 @@ static void *nixge_get_nvmem_address(struct device *dev)
 
 static int nixge_probe(struct platform_device *pdev)
 {
+	struct nixge_platform_data *pdata = NULL;
 	struct nixge_priv *priv;
 	struct net_device *ndev;
 	struct resource *dmares;

@@ -1320,10 +1365,16 @@ static int nixge_probe(struct platform_device *pdev)
 		err = nixge_of_get_phy(priv, np);
 		if (err)
 			goto free_netdev;
+	} else {
+		pdata = dev_get_platdata(&pdev->dev);
+		err = nixge_pdata_get_phy(priv, pdata);
+		if (err)
+			goto free_netdev;
 	}
 
 	/* 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)) {
+	if ((priv->phy_node && !of_phy_is_fixed_link(np)) ||
+	    (pdata && pdata->phy_interface != PHY_INTERFACE_MODE_NA)) {
 		err = nixge_mdio_setup(priv, np);
 		if (err) {
 			dev_err(&pdev->dev, "error registering mdio bus");

@@ -1347,6 +1398,9 @@ static int nixge_probe(struct platform_device *pdev)
 		of_phy_deregister_fixed_link(np);
 		of_node_put(np);
 	}
+
+	if (priv->phydev && phy_is_pseudo_fixed_link(priv->phydev))
+		fixed_phy_unregister(priv->phydev);
 free_netdev:
 	free_netdev(ndev);
 

@@ -1357,6 +1411,7 @@ static int nixge_remove(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct nixge_priv *priv = netdev_priv(ndev);
+	struct device_node *np = pdev->dev.of_node;
 
 	unregister_netdev(ndev);
 

@@ -1365,6 +1420,8 @@ static int nixge_remove(struct platform_device *pdev)
 
 	if (np && of_phy_is_fixed_link(np))
 		of_phy_deregister_fixed_link(np);
+	else if (priv->phydev && phy_is_pseudo_fixed_link(priv->phydev))
+		fixed_phy_unregister(priv->phydev);
 
 	free_netdev(ndev);
include/linux/platform_data/nixge.h new file +19 -0
new file mode 100644
index 000000000000..aa5dd5760074
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2018 National Instruments Corp.
+ *
+ * Author: Moritz Fischer <[email protected]>
+ */
+
+#ifndef __NIXGE_PDATA_H__
+#define __NIXGE_PDATA_H__
+
+#include <linux/phy.h>
+
+struct nixge_platform_data {
+	phy_interface_t phy_interface;
+	int phy_speed;
+	int phy_duplex;
+};
+
+#endif /* __NIXGE_PDATA_H__ */
+
Signature · 1 line
2.18.0
author
#12 lore raw

PATCH 3/3 net: nixge: Use sysdev instead of ndev->dev.parent for DMA

In reply to Moritz Fischer

Use sysdev instead of ndev->dev.parent for DMA in order to enable usecases where an IOMMU might get in the way otherwise. In the PCIe wrapper case on x86 the IOMMU group is not inherited by the platform device that is created as a subdevice of the PCI wrapper driver.

Signed-off-by Moritz Fischer <[email protected]>

---

Hi,

not this patch is still in the early stages, and as the rest of this series goes on top of [1].

The actual parent device might still change since the parent device driver is still under development.

If there are clever suggestions on how to generalize the assignment of sysdev, I'm all ears.

Thanks for your time,

Moritz

[1] https://lkml.org/lkml/2018/8/28/1011

---
 drivers/net/ethernet/ni/nixge.c | 50 +++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 18 deletions(-)
drivers/net/ethernet/ni/nixge.c +32 -18
index fd8e5b02c459..25fb1642d558 100644
@@ -16,6 +16,7 @@
 #include <linux/of_irq.h>
 #include <linux/skbuff.h>
 #include <linux/phy.h>
+#include <linux/pci.h>
 #include <linux/mii.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/ethtool.h>

@@ -165,6 +166,7 @@ struct nixge_priv {
 	struct net_device *ndev;
 	struct napi_struct napi;
 	struct device *dev;
+	struct device *sysdev;
 
 	/* Connection to PHY device */
 	struct device_node *phy_node;

@@ -250,7 +252,7 @@ static void nixge_hw_dma_bd_release(struct net_device *ndev)
 		phys_addr = nixge_hw_dma_bd_get_addr(&priv->rx_bd_v[i],
 						     phys);
 
-		dma_unmap_single(ndev->dev.parent, phys_addr,
+		dma_unmap_single(priv->sysdev, phys_addr,
 				 NIXGE_MAX_JUMBO_FRAME_SIZE,
 				 DMA_FROM_DEVICE);
 

@@ -261,16 +263,16 @@ static void nixge_hw_dma_bd_release(struct net_device *ndev)
 	}
 
 	if (priv->rx_bd_v)
-		dma_free_coherent(ndev->dev.parent,
+		dma_free_coherent(priv->sysdev,
 				  sizeof(*priv->rx_bd_v) * RX_BD_NUM,
 				  priv->rx_bd_v,
 				  priv->rx_bd_p);
 
 	if (priv->tx_skb)
-		devm_kfree(ndev->dev.parent, priv->tx_skb);
+		devm_kfree(priv->sysdev, priv->tx_skb);
 
 	if (priv->tx_bd_v)
-		dma_free_coherent(ndev->dev.parent,
+		dma_free_coherent(priv->sysdev,
 				  sizeof(*priv->tx_bd_v) * TX_BD_NUM,
 				  priv->tx_bd_v,
 				  priv->tx_bd_p);

@@ -290,19 +292,19 @@ static int nixge_hw_dma_bd_init(struct net_device *ndev)
 	priv->rx_bd_ci = 0;
 
 	/* Allocate the Tx and Rx buffer descriptors. */
-	priv->tx_bd_v = dma_zalloc_coherent(ndev->dev.parent,
+	priv->tx_bd_v = dma_zalloc_coherent(priv->sysdev,
 					    sizeof(*priv->tx_bd_v) * TX_BD_NUM,
 					    &priv->tx_bd_p, GFP_KERNEL);
 	if (!priv->tx_bd_v)
 		goto out;
 
-	priv->tx_skb = devm_kcalloc(ndev->dev.parent,
+	priv->tx_skb = devm_kcalloc(priv->sysdev,
 				    TX_BD_NUM, sizeof(*priv->tx_skb),
 				    GFP_KERNEL);
 	if (!priv->tx_skb)
 		goto out;
 
-	priv->rx_bd_v = dma_zalloc_coherent(ndev->dev.parent,
+	priv->rx_bd_v = dma_zalloc_coherent(priv->sysdev,
 					    sizeof(*priv->rx_bd_v) * RX_BD_NUM,
 					    &priv->rx_bd_p, GFP_KERNEL);
 	if (!priv->rx_bd_v)

@@ -327,7 +329,7 @@ static int nixge_hw_dma_bd_init(struct net_device *ndev)
 			goto out;
 
 		nixge_hw_dma_bd_set_offset(&priv->rx_bd_v[i], skb);
-		phys = dma_map_single(ndev->dev.parent, skb->data,
+		phys = dma_map_single(priv->sysdev, skb->data,
 				      NIXGE_MAX_JUMBO_FRAME_SIZE,
 				      DMA_FROM_DEVICE);
 

@@ -438,10 +440,10 @@ static void nixge_tx_skb_unmap(struct nixge_priv *priv,
 {
 	if (tx_skb->mapping) {
 		if (tx_skb->mapped_as_page)
-			dma_unmap_page(priv->ndev->dev.parent, tx_skb->mapping,
+			dma_unmap_page(priv->sysdev, tx_skb->mapping,
 				       tx_skb->size, DMA_TO_DEVICE);
 		else
-			dma_unmap_single(priv->ndev->dev.parent,
+			dma_unmap_single(priv->sysdev,
 					 tx_skb->mapping,
 					 tx_skb->size, DMA_TO_DEVICE);
 		tx_skb->mapping = 0;

@@ -519,9 +521,9 @@ static int nixge_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		return NETDEV_TX_OK;
 	}
 
-	cur_phys = dma_map_single(ndev->dev.parent, skb->data,
+	cur_phys = dma_map_single(priv->sysdev, skb->data,
 				  skb_headlen(skb), DMA_TO_DEVICE);
-	if (dma_mapping_error(ndev->dev.parent, cur_phys))
+	if (dma_mapping_error(priv->sysdev, cur_phys))
 		goto drop;
 	nixge_hw_dma_bd_set_phys(cur_p, cur_phys);
 

@@ -539,10 +541,10 @@ static int nixge_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		tx_skb = &priv->tx_skb[priv->tx_bd_tail];
 		frag = &skb_shinfo(skb)->frags[ii];
 
-		cur_phys = skb_frag_dma_map(ndev->dev.parent, frag, 0,
+		cur_phys = skb_frag_dma_map(priv->sysdev, frag, 0,
 					    skb_frag_size(frag),
 					    DMA_TO_DEVICE);
-		if (dma_mapping_error(ndev->dev.parent, cur_phys))
+		if (dma_mapping_error(priv->sysdev, cur_phys))
 			goto frag_err;
 		nixge_hw_dma_bd_set_phys(cur_p, cur_phys);
 

@@ -579,7 +581,7 @@ static int nixge_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		cur_p = &priv->tx_bd_v[priv->tx_bd_tail];
 		cur_p->status = 0;
 	}
-	dma_unmap_single(priv->ndev->dev.parent,
+	dma_unmap_single(priv->sysdev,
 			 tx_skb->mapping,
 			 tx_skb->size, DMA_TO_DEVICE);
 drop:

@@ -611,7 +613,7 @@ static int nixge_recv(struct net_device *ndev, int budget)
 		if (length > NIXGE_MAX_JUMBO_FRAME_SIZE)
 			length = NIXGE_MAX_JUMBO_FRAME_SIZE;
 
-		dma_unmap_single(ndev->dev.parent,
+		dma_unmap_single(priv->sysdev,
 				 nixge_hw_dma_bd_get_addr(cur_p, phys),
 				 NIXGE_MAX_JUMBO_FRAME_SIZE,
 				 DMA_FROM_DEVICE);

@@ -636,10 +638,10 @@ static int nixge_recv(struct net_device *ndev, int budget)
 		if (!new_skb)
 			return packets;
 
-		cur_phys = dma_map_single(ndev->dev.parent, new_skb->data,
+		cur_phys = dma_map_single(priv->sysdev, new_skb->data,
 					  NIXGE_MAX_JUMBO_FRAME_SIZE,
 					  DMA_FROM_DEVICE);
-		if (dma_mapping_error(ndev->dev.parent, cur_phys)) {
+		if (dma_mapping_error(priv->sysdev, cur_phys)) {
 			/* FIXME: bail out and clean up */
 			netdev_err(ndev, "Failed to map ...\n");
 		}

@@ -1303,6 +1305,7 @@ static int nixge_probe(struct platform_device *pdev)
 	struct net_device *ndev;
 	struct resource *dmares;
 	struct device_node *np;
+	struct device *sysdev;
 	const u8 *mac_addr;
 	int err;
 

@@ -1331,9 +1334,20 @@ static int nixge_probe(struct platform_device *pdev)
 		eth_hw_addr_random(ndev);
 	}
 
+	sysdev = &pdev->dev;
+#ifdef CONFIG_PCI
+	/* if this is a subdevice of a PCI device we'll have to
+	 * make sure we'll use parent instead of our own platform
+	 * device to make sure  the DMA API if we use an IOMMU
+	 */
+	if (sysdev->parent && sysdev->parent->bus == &pci_bus_type)
+		sysdev = sysdev->parent;
+#endif
+
 	priv = netdev_priv(ndev);
 	priv->ndev = ndev;
 	priv->dev = &pdev->dev;
+	priv->sysdev = sysdev;
 
 	netif_napi_add(ndev, &priv->napi, nixge_poll, NAPI_POLL_WEIGHT);
Signature · 1 line
2.18.0
#8 lore raw

Re: [PATCH net-next 2/3] net: nixge: Add support for having nixge as subdevice

In reply to Moritz Fischer
Quoted 136 lines · Moritz Fischer

Add support for instantiating nixge as subdevice using fixed-link and platform data to configure it.

Signed-off-by Moritz Fischer <[email protected]>

---

Hi,

not this patch is still in the early stages, and as the rest of this series goes on top of [1].

The actual platform data might still change since the parent device driver is still under development.

Thanks for your time,

Moritz

[1] https://lkml.org/lkml/2018/8/28/1011

---
 drivers/net/ethernet/ni/nixge.c     | 71 ++++++++++++++++++++++++++---
 include/linux/platform_data/nixge.h | 19 ++++++++
 2 files changed, 83 insertions(+), 7 deletions(-)
 create mode 100644 include/linux/platform_data/nixge.h
drivers/net/ethernet/ni/nixge.c +52 -6
index 670249313ff0..fd8e5b02c459 100644
@@ -7,6 +7,8 @@
 #include <linux/etherdevice.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
+#include <linux/phy_fixed.h>
+#include <linux/platform_data/nixge.h>
 #include <linux/of_address.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>

@@ -167,6 +169,7 @@ struct nixge_priv {
        /* Connection to PHY device */
        struct device_node *phy_node;
        phy_interface_t         phy_mode;
+       struct phy_device *phydev;
 
        int link;
        unsigned int speed;

@@ -859,15 +862,25 @@ static void nixge_dma_err_handler(unsigned long data)
 static int nixge_open(struct net_device *ndev)
 {
        struct nixge_priv *priv = netdev_priv(ndev);
-       struct phy_device *phy;
+       struct phy_device *phy = NULL;
        int ret;
 
        nixge_device_reset(ndev);
 
-       phy = of_phy_connect(ndev, priv->phy_node,
-                            &nixge_handle_link_change, 0, priv->phy_mode);
-       if (!phy)
-               return -ENODEV;
+       if (priv->dev->of_node) {
+               phy = of_phy_connect(ndev, priv->phy_node,
+                                    &nixge_handle_link_change, 0,
+                                    priv->phy_mode);
+               if (!phy)
+                       return -ENODEV;
+       } else if (priv->phydev) {
+               ret = phy_connect_direct(ndev, priv->phydev,
+                                        &nixge_handle_link_change,
+                                        priv->phy_mode);
+               if (ret)
+                       return ret;
+               phy = priv->phydev;
+       }
 
        phy_start(phy);
 

@@ -1215,10 +1228,41 @@ static int nixge_of_get_phy(struct nixge_priv *priv, struct device_node *np)
        return 0;
 }
 
+static int nixge_pdata_get_phy(struct nixge_priv *priv,
+                              struct nixge_platform_data *pdata)
+{
+       struct phy_device *phy = NULL;
+
+       if (!pdata)
+               return -EINVAL;
+
+       if (pdata && pdata->phy_interface == PHY_INTERFACE_MODE_NA) {
+               struct fixed_phy_status fphy_status = {
+                       .link = 1,
+                       .duplex = pdata->phy_duplex,
+                       .speed = pdata->phy_speed,
+                       .pause = 0,
+                       .asym_pause = 0,
+               };
+
+               /* TODO: Pull out GPIO from pdata */
+               phy = fixed_phy_register(PHY_POLL, &fphy_status, -1,
+                                        NULL);
+               if (IS_ERR_OR_NULL(phy)) {
+                       dev_err(priv->dev,
+                               "failed to register fixed PHY device\n");
+                       return -ENODEV;
+               }
+       }
+       priv->phy_mode = pdata->phy_interface;
+       priv->phydev = phy;
+
+       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)

@@ -1254,6 +1298,7 @@ static void *nixge_get_nvmem_address(struct device *dev)
 
 static int nixge_probe(struct platform_device *pdev)
 {
+       struct nixge_platform_data *pdata = NULL;
        struct nixge_priv *priv;
        struct net_device *ndev;
        struct resource *dmares;

@@ -1320,10 +1365,16 @@ static int nixge_probe(struct platform_device *pdev)
                err = nixge_of_get_phy(priv, np);
                if (err)
                        goto free_netdev;
+       } else {

drivers/net/ethernet/ni/nixge.c static int nixge_probe(struct platform_device *pdev) · line 1377

Quoted 10 lines
drivers/net/ethernet/ni/nixge.c +6 -1
+               pdata = dev_get_platdata(&pdev->dev);
+               err = nixge_pdata_get_phy(priv, pdata);
+               if (err)
+                       goto free_netdev;
        }
 
        /* 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)) {
+       if ((priv->phy_node && !of_phy_is_fixed_link(np)) ||
+           (pdata && pdata->phy_interface != PHY_INTERFACE_MODE_NA)) {

Must've messed up the rebase. Missing a parents. I'll resubmit this one. Sorry for the noise.

Quoted 56 lines
drivers/net/ethernet/ni/nixge.c +6 -0
                err = nixge_mdio_setup(priv, np);
                if (err) {
                        dev_err(&pdev->dev, "error registering mdio bus");

@@ -1347,6 +1398,9 @@ static int nixge_probe(struct platform_device *pdev)
                of_phy_deregister_fixed_link(np);
                of_node_put(np);
        }
+
+       if (priv->phydev && phy_is_pseudo_fixed_link(priv->phydev))
+               fixed_phy_unregister(priv->phydev);
 free_netdev:
        free_netdev(ndev);
 

@@ -1357,6 +1411,7 @@ static int nixge_remove(struct platform_device *pdev)
 {
        struct net_device *ndev = platform_get_drvdata(pdev);
        struct nixge_priv *priv = netdev_priv(ndev);
+       struct device_node *np = pdev->dev.of_node;
 
        unregister_netdev(ndev);
 

@@ -1365,6 +1420,8 @@ static int nixge_remove(struct platform_device *pdev)
 
        if (np && of_phy_is_fixed_link(np))
                of_phy_deregister_fixed_link(np);
+       else if (priv->phydev && phy_is_pseudo_fixed_link(priv->phydev))
+               fixed_phy_unregister(priv->phydev);
 
        free_netdev(ndev);
include/linux/platform_data/nixge.h new file +19 -1
new file mode 100644
index 000000000000..aa5dd5760074
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2018 National Instruments Corp.
+ *
+ * Author: Moritz Fischer <[email protected]>
+ */
+
+#ifndef __NIXGE_PDATA_H__
+#define __NIXGE_PDATA_H__
+
+#include <linux/phy.h>
+
+struct nixge_platform_data {
+       phy_interface_t phy_interface;
+       int phy_speed;
+       int phy_duplex;
+};
+
+#endif /* __NIXGE_PDATA_H__ */
+
--
Quoted 2 lines

2.18.0

Thanks,
Moritz

#2 lore raw

Re: [PATCH net-next 1/3] net: nixge: Add support for fixed-link subnodes

In reply to Moritz Fischer
Quoted 4 lines · Moritz Fischer

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

#9 lore raw

Re: [PATCH net-next 2/3] net: nixge: Add support for having nixge as subdevice

In reply to Moritz Fischer
Quoted 6 lines · Moritz Fischer

Add support for instantiating nixge as subdevice using fixed-link and platform data to configure it.

Signed-off-by Moritz Fischer <[email protected]>

---

Quoted 7 lines

Hi,

not this patch is still in the early stages, and as the rest of this series goes on top of [1].

The actual platform data might still change since the parent device driver is still under development.

Hi Moritz

Could you tell us more about the parent device. I'm guessing PCIe. Is it x86 so no device tree? Are there cases where it does not have a PHY connected? What is connected instead? SFP? A switch? Can there be multiple PHYs on the MDIO bus?

Answering these questions will help decide how best to structure this.

Andrew

#10 lore raw

Re: [PATCH net-next 2/3] net: nixge: Add support for having nixge as subdevice

Hi Andrew,

Quoted 4 lines · Andrew Lunn

Could you tell us more about the parent device. I'm guessing PCIe. Is it x86 so no device tree? Are there cases where it does not have a PHY connected? What is connected instead? SFP? A switch? Can there be multiple PHYs on the MDIO bus?

The device is part of a larger FPGA design. One use case that I was trying to support with this patch is PCIe with x86 (hopefully on it's own PF...) Since the whole design isn't completely done, these are the use cases I see upcoming and current:

ARM(64):
a) DT: PHY over MDIO (current use case), fixed-link with GPIO (coming)
b) DT: SFP (potentially coming)

x86:
a) no PHY (coming)-> fixed-link with GPIO
b) SFP (potentially), PHY over MDIO (potentially)

Thanks for your help,

Moritz

#3 lore raw

Re: [PATCH net-next 1/3] net: nixge: Add support for fixed-link subnodes

In reply to Andrew Lunn

Hi Andrew,

Quoted 11 lines · Andrew Lunn
Quoted 4 lines · Moritz Fischer

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 27 lines

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>;
Quoted 3 lines

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?

Quoted 5 lines

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.

Quoted 7 lines

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

#11 lore raw

Re: [PATCH net-next 2/3] net: nixge: Add support for having nixge as subdevice

In reply to Moritz Fischer
Quoted 14 lines · Moritz Fischer

Hi Andrew,

Quoted 4 lines · Andrew Lunn

Could you tell us more about the parent device. I'm guessing PCIe. Is it x86 so no device tree? Are there cases where it does not have a PHY connected? What is connected instead? SFP? A switch? Can there be multiple PHYs on the MDIO bus?

The device is part of a larger FPGA design. One use case that I was trying to support with this patch is PCIe with x86 (hopefully on it's own PF...) Since the whole design isn't completely done, these are the use cases I see upcoming and current:

Quoted 7 lines

ARM(64):
a) DT: PHY over MDIO (current use case), fixed-link with GPIO (coming)
b) DT: SFP (potentially coming)

x86:
a) no PHY (coming)-> fixed-link with GPIO
b) SFP (potentially), PHY over MDIO (potentially)

Hi Mortiz

For SFP, you need to convert this driver to use phylink. That will also help you with fixed-link, since phylink will handle probing that for you.

But this brings its own problems. phylink and sfp currently has no support for platform devices. The SFP driver needs to know which i2c bus to use, and which GPIOs are connected to the SFP. phylink parses the device tree to find out if there is an SFP device, or a fixed link, etc. I don't know of any conceptual reason why platform data would not work, it just needs implementing.

There also does not appear to be any in kernel users of the device tree binding. That gives you some flexibility in that you could think about making non-backwards compatible changes in the binding. I would definitely want to move PHYs into an mdio subnode.

I'm not aware of any x86 drivers using fixed link. What they generally do is register the mdio bus using mdiobus_register() and then use phy_find_first() to get a PHY. This works O.K. for PCs, laptops, and PCIe cards where there is only one PHY on the bus. What you might be able to do is always register the MDIO bus, and if you fail to find a PHY, instantiate a fixed-link and use that instead.

Reality is, all the core work in this area has been pushed by the embedded world, which is ARM and device tree. For Intel and Server style networking, drivers tend to either ignore the Linux core code and write there own PHY and MDIO bus drivers, or it is all done in firmware.

Andrew

build bot
#6 lore raw

Re: [PATCH net-next 1/3] net: nixge: Add support for fixed-link subnodes

In reply to Moritz Fischer
I love your patch! Yet something to improve:

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

#4 lore raw

Re: [PATCH net-next 1/3] net: nixge: Add support for fixed-link subnodes

In reply to Moritz Fischer
Quoted 5 lines
Quoted 2 lines

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?

Quoted 5 lines
Quoted 3 lines

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.

Quoted 2 lines

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

#5 lore raw

Re: [PATCH net-next 1/3] net: nixge: Add support for fixed-link subnodes

Andrew,

Quoted 8 lines · Andrew Lunn
Quoted 5 lines
Quoted 2 lines

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

Quoted 9 lines
Quoted 6 lines
Quoted 4 lines

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.

Quoted 9 lines
Quoted 3 lines

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