Re: [PATCH v1 2/2] net: emac: add support for device-tree based PHY discovery and setup

[Florian Fainelli]

On 02/19/2017 01:22 PM, Christian Lamparter wrote:
> This patch adds glue-code that allows the EMAC driver to interface
> with the existing dt-supported PHYs in drivers/net/phy.
> 
> Because currently, the emac driver maintains a small library of
> supported phys for in a private phy.c file located in the drivers
> directory.
> 
> The support is limited to mostly single ethernet transceiver like the:
> CIS8201, BCM5248, ET1011C, Marvell 88E and 88E1112, AR8035.
> 
> However, routers like the Netgear WNDR4700 and Cisco Meraki MX60(W)
> have a 5-port switch (AR8327N) attached to the EMAC. The switch chip
> is supported by the qca8k mdio driver, which uses the generic phy
> library. Another reason is that PHYLIB also supports the BCM54610,
> which was used for the Western Digital My Book Live.
> 
> This will now also make EMAC select PHYLIB.

This looks mostly good, except one thing below:

> 
> Signed-off-by: Christian Lamparter 
> ---

> +static int emac_dt_phy_connect(struct emac_instance *dev,
> +struct device_node *phy_handle)
> +{
> + u32 phy_flags = 0;
> + int res;
> +
> + res = of_property_read_u32(phy_handle, "phy-flags", _flags);
> + if (res < 0 && res != -EINVAL)
> + return res;

phy-flags is not documented in the binding document, but looking at how
you pass it down, I don't think you should do this.

> +
> + dev->phy.def = devm_kzalloc(>ofdev->dev, sizeof(*dev->phy.def),
> + GFP_KERNEL);
> + if (!dev->phy.def)
> + return -ENOMEM;
> +
> + dev->phy_dev = of_phy_connect(dev->ndev, phy_handle,
> +   _adjust_link, phy_flags,
> +   dev->phy_mode);

phy_flags is meant to allow the MAC driver to pass information down to
the PHY driver, e.g: lane swap needed, a revision that could not be read
by the PHY itself, board-level workarounds etc.

> + if (!dev->phy_dev) {
> + dev_err(>ofdev->dev, "failed to connect to PHY.\n");
> + return -ENODEV;
> + }
-- 
Florian

[PATCH v1 2/2] net: emac: add support for device-tree based PHY discovery and setup

[Christian Lamparter]

This patch adds glue-code that allows the EMAC driver to interface
with the existing dt-supported PHYs in drivers/net/phy.

Because currently, the emac driver maintains a small library of
supported phys for in a private phy.c file located in the drivers
directory.

The support is limited to mostly single ethernet transceiver like the:
CIS8201, BCM5248, ET1011C, Marvell 88E and 88E1112, AR8035.

However, routers like the Netgear WNDR4700 and Cisco Meraki MX60(W)
have a 5-port switch (AR8327N) attached to the EMAC. The switch chip
is supported by the qca8k mdio driver, which uses the generic phy
library. Another reason is that PHYLIB also supports the BCM54610,
which was used for the Western Digital My Book Live.

This will now also make EMAC select PHYLIB.

Signed-off-by: Christian Lamparter 
---
RFC->v1:
- added PHYLIB (fixes kbuild-bot error)
- used the correct version (the working one from LEDE).
- added fixed-link DT support. (This fixes a XXX)
- Added WA for the MX60(W): -EREMOTEIO/-ETIMEDOUT/...
  errors from mdio_read are converted to 0x.
---
 drivers/net/ethernet/ibm/emac/Kconfig |   1 +
 drivers/net/ethernet/ibm/emac/core.c  | 260 +-
 drivers/net/ethernet/ibm/emac/core.h  |   4 +
 3 files changed, 258 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/Kconfig 
b/drivers/net/ethernet/ibm/emac/Kconfig
index 3f44a30e0615..90d49191beb3 100644
--- a/drivers/net/ethernet/ibm/emac/Kconfig
+++ b/drivers/net/ethernet/ibm/emac/Kconfig
@@ -2,6 +2,7 @@ config IBM_EMAC
tristate "IBM EMAC Ethernet support"
depends on PPC_DCR
select CRC32
+   select PHYLIB
help
  This driver supports the IBM EMAC family of Ethernet controllers
  typically found on 4xx embedded PowerPC chips, but also on the
diff --git a/drivers/net/ethernet/ibm/emac/core.c 
b/drivers/net/ethernet/ibm/emac/core.c
index 5909615c27f7..11d97971fb28 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -42,6 +42,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -2410,6 +2411,225 @@ static int emac_read_uint_prop(struct device_node *np, 
const char *name,
return 0;
 }
 
+static void emac_adjust_link(struct net_device *ndev)
+{
+   struct emac_instance *dev = netdev_priv(ndev);
+   struct phy_device *phy = dev->phy_dev;
+
+   dev->phy.autoneg = phy->autoneg;
+   dev->phy.speed = phy->speed;
+   dev->phy.duplex = phy->duplex;
+   dev->phy.pause = phy->pause;
+   dev->phy.asym_pause = phy->asym_pause;
+   dev->phy.advertising = phy->advertising;
+}
+
+static int emac_mii_bus_read(struct mii_bus *bus, int addr, int regnum)
+{
+   int ret = emac_mdio_read(bus->priv, addr, regnum);
+   /* This is a workaround for powered down ports/phys.
+* In the wild, this was seen on the Cisco Meraki MX60(W).
+* This hardware disables ports as part of the handoff
+* procedure. Accessing the ports will lead to errors
+* (-ETIMEDOUT, -EREMOTEIO) that do more harm than good.
+*/
+   return ret < 0 ? 0x : ret;
+}
+
+static int emac_mii_bus_write(struct mii_bus *bus, int addr,
+ int regnum, u16 val)
+{
+   emac_mdio_write(bus->priv, addr, regnum, val);
+   return 0;
+}
+
+static int emac_mii_bus_reset(struct mii_bus *bus)
+{
+   struct emac_instance *dev = netdev_priv(bus->priv);
+
+   return emac_reset(dev);
+}
+
+static int emac_mdio_setup_aneg(struct mii_phy *phy, u32 advertise)
+{
+   struct net_device *ndev = phy->dev;
+   struct emac_instance *dev = netdev_priv(ndev);
+
+   dev->phy.autoneg = AUTONEG_ENABLE;
+   dev->phy.speed = SPEED_1000;
+   dev->phy.duplex = DUPLEX_FULL;
+   dev->phy.advertising = advertise;
+   phy->autoneg = AUTONEG_ENABLE;
+   phy->speed = dev->phy.speed;
+   phy->duplex = dev->phy.duplex;
+   phy->advertising = advertise;
+   return phy_start_aneg(dev->phy_dev);
+}
+
+static int emac_mdio_setup_forced(struct mii_phy *phy, int speed, int fd)
+{
+   struct net_device *ndev = phy->dev;
+   struct emac_instance *dev = netdev_priv(ndev);
+
+   dev->phy.autoneg =  AUTONEG_DISABLE;
+   dev->phy.speed = speed;
+   dev->phy.duplex = fd;
+   phy->autoneg = AUTONEG_DISABLE;
+   phy->speed = speed;
+   phy->duplex = fd;
+   return phy_start_aneg(dev->phy_dev);
+}
+
+static int emac_mdio_poll_link(struct mii_phy *phy)
+{
+   struct net_device *ndev = phy->dev;
+   struct emac_instance *dev = netdev_priv(ndev);
+   int res;
+
+   res = phy_read_status(dev->phy_dev);
+   if (res) {
+   dev_err(>ofdev->dev, "link update failed (%d).", res);
+   return ethtool_op_get_link(ndev);
+   }
+
+   return dev->phy_dev->link;
+}
+
+static int