[PATCH] net: ethernet: cadence: Add fixed-link functionality

From: Moritz Fischer <mdf@kernel.org>

This allows 'fixed-link' direct MAC connections to be declared
in devicetree.

Signed-off-by: Moritz Fischer <mdf@kernel.org>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
---
 drivers/net/ethernet/cadence/macb.c | 61 ++++++++++++++++++++++++++++++++++---
 drivers/net/ethernet/cadence/macb.h |  1 +
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 30606b1..af443a8 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c

@@ -469,6 +469,34 @@ static int macb_mii_probe(struct net_device *dev)
 	return 0;
 }
 
+static int macb_fixed_init(struct macb *bp)
+{
+	struct phy_device *phydev;
+
+	phydev = of_phy_connect(bp->dev, bp->phy_node,
+				&macb_handle_link_change, 0,
+				bp->phy_interface);
+	if (!phydev)
+		return -ENODEV;
+
+	/* mask with MAC supported features */
+	if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
+		phydev->supported &= PHY_GBIT_FEATURES;
+	else
+		phydev->supported &= PHY_BASIC_FEATURES;
+
+	if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF)
+		phydev->supported &= ~SUPPORTED_1000baseT_Half;
+
+	phydev->advertising = phydev->supported;
+
+	bp->link = 0;
+	bp->speed = 0;
+	bp->duplex = -1;
+
+	return 0;
+}
+
 static int macb_mii_init(struct macb *bp)
 {
 	struct macb_platform_data *pdata;

@@ -3245,6 +3273,7 @@ static int macb_probe(struct platform_device *pdev)
 	const char *mac;
 	struct macb *bp;
 	int err;
+	bool fixed_link = false;
 
 	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	mem = devm_ioremap_resource(&pdev->dev, regs);

@@ -3342,8 +3371,18 @@ static int macb_probe(struct platform_device *pdev)
 		macb_get_hwaddr(bp);
 
 	/* Power up the PHY if there is a GPIO reset */
-	phy_node =  of_get_next_available_child(np, NULL);
-	if (phy_node) {
+	phy_node = of_parse_phandle(np, "phy-handle", 0);
+	if (!phy_node && of_phy_is_fixed_link(np)) {
+		err = of_phy_register_fixed_link(np);
+		if (err < 0) {
+			dev_err(&pdev->dev, "broken fixed-link specification");
+			goto failed_phy;
+		}
+		/* in case of a fixed PHY, the DT node is the ethernet MAC */
+		phy_node = of_node_get(np);
+		bp->phy_node = phy_node;
+		fixed_link = true;
+	} else {
 		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
 
 		if (gpio_is_valid(gpio)) {

@@ -3369,7 +3408,10 @@ static int macb_probe(struct platform_device *pdev)
 	if (err)
 		goto err_out_free_netdev;
 
-	err = macb_mii_init(bp);
+	if (!fixed_link)
+		err = macb_mii_init(bp);
+	else
+		err = macb_fixed_init(bp);
 	if (err)
 		goto err_out_free_netdev;
 

@@ -3400,6 +3442,9 @@ static int macb_probe(struct platform_device *pdev)
 	if (bp->reset_gpio)
 		gpiod_set_value(bp->reset_gpio, 0);
 
+failed_phy:
+	of_node_put(phy_node);
+
 err_out_free_netdev:
 	free_netdev(dev);
 

@@ -3423,9 +3468,14 @@ static int macb_remove(struct platform_device *pdev)
 		bp = netdev_priv(dev);
 		if (dev->phydev)
 			phy_disconnect(dev->phydev);
-		mdiobus_unregister(bp->mii_bus);
+
+		if (!bp->phy_node)
+			mdiobus_unregister(bp->mii_bus);
+
 		dev->phydev = NULL;
-		mdiobus_free(bp->mii_bus);
+
+		if (!bp->phy_node)
+			mdiobus_free(bp->mii_bus);
 
 		/* Shutdown the PHY if there is a GPIO reset */
 		if (bp->reset_gpio)

@@ -3435,6 +3485,7 @@ static int macb_remove(struct platform_device *pdev)
 		clk_disable_unprepare(bp->tx_clk);
 		clk_disable_unprepare(bp->hclk);
 		clk_disable_unprepare(bp->pclk);
+		of_node_put(bp->phy_node);
 		clk_disable_unprepare(bp->rx_clk);
 		free_netdev(dev);
 	}

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 234a49e..d9a9b6f 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h

@@ -931,6 +931,7 @@ struct macb {
 	struct macb_or_gem_ops	macbgem_ops;
 
 	struct mii_bus		*mii_bus;
+	struct device_node	*phy_node;
 	int 			link;
 	int 			speed;
 	int 			duplex;

-- 
2.7.4

On 02/15/2017 12:44 PM, mdf@kernel.org wrote:

This is nearly identical to macb_mii_probe(), can you try to re-use what
is done there and just extract the phy_find_first() part which is
different here?

Humm, I'd have to read the code a bit more, but conceptually, you could
declare child MDIO device nodes (e.g: Ethernet switch) and a fixed-link
that describes how you are connecting to that Ethernet switch. The MDIO
devices require the MDIO bus to be available, so I don't think you can
treat fixed-link as mutually exclusive with an absence of PHY nodes.
-- 
Florian

Hi Florian,

thanks for the quick reply.

On Wed, Feb 15, 2017 at 12:57 PM, Florian Fainelli [off-list ref] wrote:

On 02/15/2017 12:44 PM, mdf@kernel.org wrote:

quoted

From: Moritz Fischer <mdf@kernel.org>

This allows 'fixed-link' direct MAC connections to be declared
in devicetree.

Signed-off-by: Moritz Fischer <mdf@kernel.org>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
---
 drivers/net/ethernet/cadence/macb.c | 61 ++++++++++++++++++++++++++++++++++---
 drivers/net/ethernet/cadence/macb.h |  1 +
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 30606b1..af443a8 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c

@@ -469,6 +469,34 @@ static int macb_mii_probe(struct net_device *dev)
      return 0;
 }

+static int macb_fixed_init(struct macb *bp)
+{
+     struct phy_device *phydev;
+
+     phydev = of_phy_connect(bp->dev, bp->phy_node,
+                             &macb_handle_link_change, 0,
+                             bp->phy_interface);
+     if (!phydev)
+             return -ENODEV;
+
+     /* mask with MAC supported features */
+     if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
+             phydev->supported &= PHY_GBIT_FEATURES;
+     else
+             phydev->supported &= PHY_BASIC_FEATURES;
+
+     if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF)
+             phydev->supported &= ~SUPPORTED_1000baseT_Half;
+
+     phydev->advertising = phydev->supported;
+
+     bp->link = 0;
+     bp->speed = 0;
+     bp->duplex = -1;
+
+     return 0;
+}

This is nearly identical to macb_mii_probe(), can you try to re-use what
is done there and just extract the phy_find_first() part which is
different here?

Yeah. It probably still needs some work.

quoted

+
 static int macb_mii_init(struct macb *bp)
 {
      struct macb_platform_data *pdata;

@@ -3245,6 +3273,7 @@ static int macb_probe(struct platform_device *pdev)
      const char *mac;
      struct macb *bp;
      int err;
+     bool fixed_link = false;

      regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
      mem = devm_ioremap_resource(&pdev->dev, regs);

@@ -3342,8 +3371,18 @@ static int macb_probe(struct platform_device *pdev)
              macb_get_hwaddr(bp);

      /* Power up the PHY if there is a GPIO reset */
-     phy_node =  of_get_next_available_child(np, NULL);
-     if (phy_node) {
+     phy_node = of_parse_phandle(np, "phy-handle", 0);
+     if (!phy_node && of_phy_is_fixed_link(np)) {
+             err = of_phy_register_fixed_link(np);
+             if (err < 0) {
+                     dev_err(&pdev->dev, "broken fixed-link specification");
+                     goto failed_phy;
+             }
+             /* in case of a fixed PHY, the DT node is the ethernet MAC */
+             phy_node = of_node_get(np);
+             bp->phy_node = phy_node;
+             fixed_link = true;
+     } else {
              int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);

              if (gpio_is_valid(gpio)) {

@@ -3369,7 +3408,10 @@ static int macb_probe(struct platform_device *pdev)
      if (err)
              goto err_out_free_netdev;

-     err = macb_mii_init(bp);
+     if (!fixed_link)
+             err = macb_mii_init(bp);
+     else
+             err = macb_fixed_init(bp);
      if (err)
              goto err_out_free_netdev;

@@ -3400,6 +3442,9 @@ static int macb_probe(struct platform_device *pdev)
      if (bp->reset_gpio)
              gpiod_set_value(bp->reset_gpio, 0);

+failed_phy:
+     of_node_put(phy_node);
+
 err_out_free_netdev:
      free_netdev(dev);

@@ -3423,9 +3468,14 @@ static int macb_remove(struct platform_device *pdev)
              bp = netdev_priv(dev);
              if (dev->phydev)
                      phy_disconnect(dev->phydev);
-             mdiobus_unregister(bp->mii_bus);
+
+             if (!bp->phy_node)
+                     mdiobus_unregister(bp->mii_bus);
+
              dev->phydev = NULL;
-             mdiobus_free(bp->mii_bus);
+
+             if (!bp->phy_node)
+                     mdiobus_free(bp->mii_bus);

Humm, I'd have to read the code a bit more, but conceptually, you could
declare child MDIO device nodes (e.g: Ethernet switch) and a fixed-link
that describes how you are connecting to that Ethernet switch. The MDIO
devices require the MDIO bus to be available, so I don't think you can
treat fixed-link as mutually exclusive with an absence of PHY nodes.

Huh, yeah. In my case the switch configuration is done over SPI.
Probably someone
out there will have hardware that does MDIO, possible.
I'll do some more research on my end. In my understanding (which might
be wrong),
the fixed-link was the alternative to a PHY hooked up with MDIO.

If you find something in your research, feel free to keep me posted ;-)

Cheers,
Moritz

Hi Moritz

I don't see any calls to of_phy_deregister_fixed_link(), either in the
error path, or the remove code.

      Andrew

Andrew,

On Wed, Feb 15, 2017 at 2:12 PM, Andrew Lunn [off-list ref] wrote:

quoted

@@ -3342,8 +3371,18 @@ static int macb_probe(struct platform_device *pdev)
              macb_get_hwaddr(bp);

      /* Power up the PHY if there is a GPIO reset */
-     phy_node =  of_get_next_available_child(np, NULL);
-     if (phy_node) {
+     phy_node = of_parse_phandle(np, "phy-handle", 0);
+     if (!phy_node && of_phy_is_fixed_link(np)) {
+             err = of_phy_register_fixed_link(np);

Hi Moritz

I don't see any calls to of_phy_deregister_fixed_link(), either in the
error path, or the remove code.

Whoops, yeah I rebased it from like a year ago. Must've not survived my
merge conflict resolution.

I'll rework it.

Thanks,

Moritz