diff mbox series

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

Message ID 20180830004046.9417-2-mdf@kernel.org
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series nixge: fixed-link support | expand

Commit Message

Moritz Fischer Aug. 30, 2018, 12:40 a.m. UTC 
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(-)

Comments

Andrew Lunn Aug. 30, 2018, 3:04 a.m. UTC | #1 
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
Moritz Fischer Aug. 30, 2018, 5:21 p.m. UTC | #2 
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
kernel test robot Aug. 30, 2018, 5:44 p.m. UTC | #3 
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
Andrew Lunn Aug. 30, 2018, 5:54 p.m. UTC | #4 
> > 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
Moritz Fischer Aug. 30, 2018, 9:09 p.m. UTC | #5 
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 mbox series

Patch

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