[RFC 1/2] dt: emac: document device-tree based phy discovery and setup

[RFC 1/2] dt: emac: document device-tree based phy discovery and setup

[Christian Lamparter]

This patch adds documentation for a new "phy-handler" property
and "mdio" sub-node. These allows the enumeration of PHYs which
are supported by the phy library under drivers/net/phy.

The EMAC ethernet controller in IBM and AMCC 4xx chips is
currently stuck with a few privately defined phy
implementations. It has no support for PHYs which
are supported by the generic phylib.

Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
 .../devicetree/bindings/powerpc/4xx/emac.txt       | 60 +++++++++++++++++++++-
 1 file changed, 58 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/powerpc/4xx/emac.txt b/Documentation/devicetree/bindings/powerpc/4xx/emac.txt
index 712baf6c3e24..0572d053c35a 100644
--- a/Documentation/devicetree/bindings/powerpc/4xx/emac.txt
+++ b/Documentation/devicetree/bindings/powerpc/4xx/emac.txt
@@ -71,6 +71,8 @@
 			  For Axon it can be absent, though my current driver
 			  doesn't handle phy-address yet so for now, keep
 			  0x00ffffff in it.
+    - phy-handle	: See net/ethernet.txt file; used to describe
+			  configurations where a external PHY is used.
     - rx-fifo-size-gige : 1 cell, Rx fifo size in bytes for 1000 Mb/sec
 			  operations (if absent the value is the same as
 			  rx-fifo-size).  For Axon, either absent or 2048.
@@ -82,7 +84,18 @@
     - tah-channel       : 1 cell, optional. If appropriate, channel used on the
 			  TAH engine.
 
-    Example:
+    - mdio subnode	: When the EMAC has a phy connected to its local
+			  mdio, which us supported by the kernel's network
+			  PHY library in drivers/net/phy, there must be device
+			  tree subnode with the following required properties:
+				- #address-cells: Must be <1>.
+				- #size-cells: Must be <0>.
+
+			  For each phy on the mdio bus, there must be a node
+			  with the following fields:
+				- reg: phy id used to communicate to phy.
+				- device_type: Must be "ethernet-phy".
+    Examples:
 
 	EMAC0: ethernet@40000800 {
 		device_type = "network";
@@ -104,6 +117,50 @@
 		zmii-channel = <0>;
 	};
 
+	EMAC1: ethernet@ef600c00 {
+		device_type = "network";
+		compatible = "ibm,emac-apm821xx", "ibm,emac4sync";
+		interrupt-parent = <&EMAC1>;
+		interrupts = <0 1>;
+		#interrupt-cells = <1>;
+		#address-cells = <0>;
+		#size-cells = <0>;
+		interrupt-map = <0 &UIC2 0x10 IRQ_TYPE_LEVEL_HIGH /* Status */
+				 1 &UIC2 0x14 IRQ_TYPE_LEVEL_HIGH /* Wake */>;
+		reg = <0xef600c00 0x000000c4>;
+		local-mac-address = [000000000000]; /* Filled in by U-Boot */
+		mal-device = <&MAL0>;
+		mal-tx-channel = <0>;
+		mal-rx-channel = <0>;
+		cell-index = <0>;
+		max-frame-size = <9000>;
+		rx-fifo-size = <16384>;
+		tx-fifo-size = <2048>;
+		fifo-entry-size = <10>;
+		phy-mode = "rgmii";
+		phy-map = <0x00000000>;
+		phy-handle = <&phy0>;
+		rgmii-device = <&RGMII0>;
+		rgmii-channel = <0>;
+		tah-device = <&TAH0>;
+		tah-channel = <0>;
+		has-inverted-stacr-oc;
+		has-new-stacr-staopc;
+
+	        mdio {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			phy0: ethernet-phy@0 {
+				device_type = "ethernet-phy";
+				reg = <0>;
+
+				qca,ar8327-initvals = <
+					0x0010 0x40000000>;
+		};
+	};
+
+
       ii) McMAL node
 
     Required properties:
@@ -145,4 +202,3 @@
     - revision           : as provided by the RGMII new version register if
 			   available.
 			   For Axon: 0x0000012a
-
-- 
2.11.0

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

[Christian Lamparter]

From: Christian Lamparter <chunkeey-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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 88E1111 and 88E1112, AR8035.
However, routers like the Netgear WNDR4700 and Cisco Meraki MX60(W)
have a 5-port switch (QCA8327N) attached to the MDIO of the EMAC.
The switch chip has already a proper phy-driver (qca8k) that uses
the generic phy library.

Signed-off-by: Christian Lamparter <chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 drivers/net/ethernet/ibm/emac/core.c | 188 +++++++++++++++++++++++++++++++++++
 drivers/net/ethernet/ibm/emac/core.h |   4 +
 2 files changed, 192 insertions(+)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index 6ead2335a169..ea9234cdb227 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -42,6 +42,7 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_net.h>
+#include <linux/of_mdio.h>
 #include <linux/slab.h>
 
 #include <asm/processor.h>
@@ -2420,6 +2421,179 @@ 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;
+
+	mutex_lock(&dev->link_lock);
+	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;
+	mutex_unlock(&dev->link_lock);
+}
+
+static int emac_mii_bus_read(struct mii_bus *bus, int addr, int regnum)
+{
+	return emac_mdio_read(bus->priv, addr, regnum);
+}
+
+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);
+
+	emac_mii_reset_phy(&dev->phy);
+	return 0;
+}
+
+static int emac_mdio_probe(struct emac_instance *dev)
+{
+	struct device_node *mii_np;
+	struct mii_bus *bus;
+	int res;
+
+	bus = mdiobus_alloc();
+	if (!bus)
+		return -ENOMEM;
+
+	mii_np = of_get_child_by_name(dev->ofdev->dev.of_node, "mdio");
+	if (!mii_np) {
+		dev_err(&dev->ndev->dev, "no mdio definition found.");
+		return -ENODEV;
+	}
+
+	if (!of_device_is_available(mii_np))
+		return 0;
+
+	bus->priv = dev->ndev;
+	bus->parent = dev->ndev->dev.parent;
+	bus->name = "emac_mdio";
+	bus->read = &emac_mii_bus_read;
+	bus->write = &emac_mii_bus_write;
+	bus->reset = &emac_mii_bus_reset;
+
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", bus->name);
+
+	res = of_mdiobus_register(bus, mii_np);
+	if (res) {
+		dev_err(&dev->ndev->dev, "cannot register MDIO bus %s\n",
+			bus->name);
+		mdiobus_free(bus);
+	}
+
+	dev->mii_bus = bus;
+	return res;
+}
+
+static void emac_mdio_cleanup(struct emac_instance *dev)
+{
+	if (dev->mii_bus) {
+		if (dev->mii_bus->state == MDIOBUS_REGISTERED)
+			mdiobus_unregister(dev->mii_bus);
+		mdiobus_free(dev->mii_bus);
+		dev->mii_bus = NULL;
+		kfree(dev->phy.def);
+	}
+}
+
+static int stub_setup_aneg(struct mii_phy *phy, u32 advertise)
+{
+	return 0;
+}
+
+static int stub_setup_forced(struct mii_phy *phy, int speed, int fd)
+{
+	return 0;
+}
+
+static int stub_poll_link(struct mii_phy *phy)
+{
+	struct net_device *ndev = phy->dev;
+	struct emac_instance *dev = netdev_priv(ndev);
+
+	return dev->opened;
+}
+
+static int stub_read_link(struct mii_phy *phy)
+{
+	struct net_device *ndev = phy->dev;
+	struct emac_instance *dev = netdev_priv(ndev);
+
+	phy_start(dev->phy_dev);
+	return 0;
+}
+
+static const struct mii_phy_ops emac_stub_phy_ops = {
+	.setup_aneg	= stub_setup_aneg,
+	.setup_forced	= stub_setup_forced,
+	.poll_link	= stub_poll_link,
+	.read_link	= stub_read_link,
+};
+
+static int emac_probe_dt_phy(struct emac_instance *dev)
+{
+	struct device_node *np = dev->ofdev->dev.of_node;
+	struct device_node *phy_handle;
+	struct net_device *ndev = dev->ndev;
+	int res;
+
+	phy_handle = of_parse_phandle(np, "phy-handle", 0);
+
+	if (phy_handle) {
+		res = emac_mdio_probe(dev);
+		if (res)
+			goto err_cleanup;
+
+		dev->phy.def = kzalloc(sizeof(*dev->phy.def), GFP_KERNEL);
+		if (!dev->phy.def) {
+			res = -ENOMEM;
+			goto err_cleanup;
+		}
+
+		dev->phy_dev = of_phy_connect(ndev, phy_handle,
+					      &emac_adjust_link, 0,
+					      PHY_INTERFACE_MODE_RGMII);
+		if (!dev->phy_dev) {
+			res = -ENODEV;
+			goto err_cleanup;
+		}
+
+		of_node_put(phy_handle);
+		dev->phy.def->phy_id = dev->phy_dev->drv->phy_id;
+		dev->phy.def->phy_id_mask = dev->phy_dev->drv->phy_id_mask;
+		dev->phy.def->name = dev->phy_dev->drv->name;
+		dev->phy.def->ops = &emac_stub_phy_ops;
+		/* Disable any PHY features not supported by the platform */
+		dev->phy.def->features =  dev->phy_dev->drv->features &
+					  ~dev->phy_feat_exc;
+		dev->phy.features = dev->phy.def->features;
+		dev->phy.address = dev->phy_dev->mdio.addr;
+		dev->phy.mode = dev->phy_dev->interface;
+		return 0;
+	}
+
+	/* if the device tree didn't specifiy the the phy, then
+	 * we simply fallback to the old emac_phy.c probe code
+	 * for compatibility reasons.
+	 */
+	return 1;
+
+ err_cleanup:
+	of_node_put(phy_handle);
+	kfree(dev->phy.def);
+	return res;
+}
+
 static int emac_init_phy(struct emac_instance *dev)
 {
 	struct device_node *np = dev->ofdev->dev.of_node;
@@ -2490,6 +2664,13 @@ static int emac_init_phy(struct emac_instance *dev)
 
 	emac_configure(dev);
 
+	if (emac_has_feature(dev, EMAC_FTR_HAS_RGMII)) {
+		int res = emac_probe_dt_phy(dev);
+
+		if (res <= 0)
+			return res;
+	}
+
 	if (dev->phy_address != 0xffffffff)
 		phy_map = ~(1 << dev->phy_address);
 
@@ -2938,6 +3119,8 @@ static int emac_probe(struct platform_device *ofdev)
 	/* I have a bad feeling about this ... */
 
  err_detach_tah:
+	emac_mdio_cleanup(dev);
+
 	if (emac_has_feature(dev, EMAC_FTR_HAS_TAH))
 		tah_detach(dev->tah_dev, dev->tah_port);
  err_detach_rgmii:
@@ -2988,6 +3171,11 @@ static int emac_remove(struct platform_device *ofdev)
 	if (emac_has_feature(dev, EMAC_FTR_HAS_ZMII))
 		zmii_detach(dev->zmii_dev, dev->zmii_port);
 
+	if (dev->phy_dev)
+		phy_disconnect(dev->phy_dev);
+
+	emac_mdio_cleanup(dev);
+
 	busy_phy_map &= ~(1 << dev->phy.address);
 	DBG(dev, "busy_phy_map now %#x" NL, busy_phy_map);
 
diff --git a/drivers/net/ethernet/ibm/emac/core.h b/drivers/net/ethernet/ibm/emac/core.h
index 93ae11494810..0710a6685489 100644
--- a/drivers/net/ethernet/ibm/emac/core.h
+++ b/drivers/net/ethernet/ibm/emac/core.h
@@ -199,6 +199,10 @@ struct emac_instance {
 	struct emac_instance		*mdio_instance;
 	struct mutex			mdio_lock;
 
+	/* Device-tree based phy configuration */
+	struct mii_bus			*mii_bus;
+	struct phy_device		*phy_dev;
+
 	/* ZMII infos if any */
 	u32				zmii_ph;
 	u32				zmii_port;
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/2] dt: emac: document device-tree based phy discovery and setup

[Florian Fainelli]

Le 02/05/17 à 14:25, Christian Lamparter a écrit :
> This patch adds documentation for a new "phy-handler" property
> and "mdio" sub-node. These allows the enumeration of PHYs which
> are supported by the phy library under drivers/net/phy.
> 
> The EMAC ethernet controller in IBM and AMCC 4xx chips is
> currently stuck with a few privately defined phy
> implementations. It has no support for PHYs which
> are supported by the generic phylib.
> 
> Signed-off-by: Christian Lamparter <chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> ---
>  .../devicetree/bindings/powerpc/4xx/emac.txt       | 60 +++++++++++++++++++++-
>  1 file changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/powerpc/4xx/emac.txt b/Documentation/devicetree/bindings/powerpc/4xx/emac.txt
> index 712baf6c3e24..0572d053c35a 100644
> --- a/Documentation/devicetree/bindings/powerpc/4xx/emac.txt
> +++ b/Documentation/devicetree/bindings/powerpc/4xx/emac.txt
> @@ -71,6 +71,8 @@
>  			  For Axon it can be absent, though my current driver
>  			  doesn't handle phy-address yet so for now, keep
>  			  0x00ffffff in it.
> +    - phy-handle	: See net/ethernet.txt file; used to describe
> +			  configurations where a external PHY is used.
>      - rx-fifo-size-gige : 1 cell, Rx fifo size in bytes for 1000 Mb/sec
>  			  operations (if absent the value is the same as
>  			  rx-fifo-size).  For Axon, either absent or 2048.
> @@ -82,7 +84,18 @@
>      - tah-channel       : 1 cell, optional. If appropriate, channel used on the
>  			  TAH engine.
>  
> -    Example:
> +    - mdio subnode	: When the EMAC has a phy connected to its local
> +			  mdio, which us supported by the kernel's network
> +			  PHY library in drivers/net/phy, there must be device
> +			  tree subnode with the following required properties:
> +				- #address-cells: Must be <1>.
> +				- #size-cells: Must be <0>.
> +
> +			  For each phy on the mdio bus, there must be a node
> +			  with the following fields:
> +				- reg: phy id used to communicate to phy.
> +				- device_type: Must be "ethernet-phy".

Just provide a reference to
Documentation/devicetree/bindings/net/phy.txt and
Documentation/devicetree/bindings/net/ethernet.txt here. device_type is
not required.

> +    Examples:
>  
>  	EMAC0: ethernet@40000800 {
>  		device_type = "network";
> @@ -104,6 +117,50 @@
>  		zmii-channel = <0>;
>  	};
>  
> +	EMAC1: ethernet@ef600c00 {
> +		device_type = "network";
> +		compatible = "ibm,emac-apm821xx", "ibm,emac4sync";
> +		interrupt-parent = <&EMAC1>;
> +		interrupts = <0 1>;
> +		#interrupt-cells = <1>;
> +		#address-cells = <0>;
> +		#size-cells = <0>;
> +		interrupt-map = <0 &UIC2 0x10 IRQ_TYPE_LEVEL_HIGH /* Status */
> +				 1 &UIC2 0x14 IRQ_TYPE_LEVEL_HIGH /* Wake */>;
> +		reg = <0xef600c00 0x000000c4>;
> +		local-mac-address = [000000000000]; /* Filled in by U-Boot */
> +		mal-device = <&MAL0>;
> +		mal-tx-channel = <0>;
> +		mal-rx-channel = <0>;
> +		cell-index = <0>;
> +		max-frame-size = <9000>;
> +		rx-fifo-size = <16384>;
> +		tx-fifo-size = <2048>;
> +		fifo-entry-size = <10>;
> +		phy-mode = "rgmii";
> +		phy-map = <0x00000000>;

If you have a proper mdio subnode, this property becomes irrelevant and
should be unused.

> +		phy-handle = <&phy0>;
> +		rgmii-device = <&RGMII0>;
> +		rgmii-channel = <0>;
> +		tah-device = <&TAH0>;
> +		tah-channel = <0>;
> +		has-inverted-stacr-oc;
> +		has-new-stacr-staopc;
> +
> +	        mdio {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			phy0: ethernet-phy@0 {
> +				device_type = "ethernet-phy";
> +				reg = <0>;
> +
> +				qca,ar8327-initvals = <
> +					0x0010 0x40000000>;
> +		};
> +	};
> +
> +
>        ii) McMAL node
>  
>      Required properties:
> @@ -145,4 +202,3 @@
>      - revision           : as provided by the RGMII new version register if
>  			   available.
>  			   For Axon: 0x0000012a
> -
> 


-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Florian Fainelli]

Le 02/05/17 à 14:25, Christian Lamparter a écrit :
> From: Christian Lamparter <chunkeey-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> 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 88E1111 and 88E1112, AR8035.
> However, routers like the Netgear WNDR4700 and Cisco Meraki MX60(W)
> have a 5-port switch (QCA8327N) attached to the MDIO of the EMAC.
> The switch chip has already a proper phy-driver (qca8k) that uses
> the generic phy library.

Technically, it's a mdio_device in the upstream kernel that registers a
switch with DSA (and a PHY device in the OpenWrt/LEDE downstream
kernel). If your goal is to specifically support that device you should
consider making the EMAC interface with a fixed link PHY to properly
initialize the EMAC <=> CPU port of the switch link, and then declare
the qca8k device as a child MDIO device (not a PHY), similar to what is
done in arch/arm/boot/dts/vf610-zii-dev-rev-b.dts for instance.

> 
> Signed-off-by: Christian Lamparter <chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> ---
>  drivers/net/ethernet/ibm/emac/core.c | 188 +++++++++++++++++++++++++++++++++++
>  drivers/net/ethernet/ibm/emac/core.h |   4 +
>  2 files changed, 192 insertions(+)
> 
> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> index 6ead2335a169..ea9234cdb227 100644
> --- a/drivers/net/ethernet/ibm/emac/core.c
> +++ b/drivers/net/ethernet/ibm/emac/core.c
> @@ -42,6 +42,7 @@
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_net.h>
> +#include <linux/of_mdio.h>
>  #include <linux/slab.h>
>  
>  #include <asm/processor.h>
> @@ -2420,6 +2421,179 @@ 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;
> +
> +	mutex_lock(&dev->link_lock);
> +	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;
> +	mutex_unlock(&dev->link_lock);

PHYLIB already executes grabbing the phy device's mutex, is this really
needed here?

> +}
> +
> +static int emac_mii_bus_read(struct mii_bus *bus, int addr, int regnum)
> +{
> +	return emac_mdio_read(bus->priv, addr, regnum);
> +}
> +
> +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);
> +
> +	emac_mii_reset_phy(&dev->phy);

This seems wrong, emac_mii_reset_phy() does a BMCR software reset, which
PHYLIB is already going to do (phy_init_hw), yet you do this here at the
MDIO bus level towards a specify PHY, whereas this should be affecting
the MDIO bus itself (and/or *all* PHY child devices for quirks).

> +	return 0;
> +}
> +
> +static int emac_mdio_probe(struct emac_instance *dev)
> +{
> +	struct device_node *mii_np;
> +	struct mii_bus *bus;
> +	int res;
> +
> +	bus = mdiobus_alloc();
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	mii_np = of_get_child_by_name(dev->ofdev->dev.of_node, "mdio");
> +	if (!mii_np) {
> +		dev_err(&dev->ndev->dev, "no mdio definition found.");
> +		return -ENODEV;
> +	}
> +
> +	if (!of_device_is_available(mii_np))
> +		return 0;
> +
> +	bus->priv = dev->ndev;
> +	bus->parent = dev->ndev->dev.parent;
> +	bus->name = "emac_mdio";
> +	bus->read = &emac_mii_bus_read;
> +	bus->write = &emac_mii_bus_write;
> +	bus->reset = &emac_mii_bus_reset;
> +
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", bus->name);

You should pick a more unique name here, if you ever have a second
instance it would just clash with the previous one.

> +
> +	res = of_mdiobus_register(bus, mii_np);
> +	if (res) {
> +		dev_err(&dev->ndev->dev, "cannot register MDIO bus %s\n",
> +			bus->name);
> +		mdiobus_free(bus);
> +	}
> +
> +	dev->mii_bus = bus;
> +	return res;
> +}
> +
> +static void emac_mdio_cleanup(struct emac_instance *dev)
> +{
> +	if (dev->mii_bus) {
> +		if (dev->mii_bus->state == MDIOBUS_REGISTERED)
> +			mdiobus_unregister(dev->mii_bus);

If you need to make that kind of check, why not separate how the mdio
bus structure's lifecycle is managed? This seems to be avoiding to hit
the BUG_ON() in mdiobus_unregister..

> +		mdiobus_free(dev->mii_bus);
> +		dev->mii_bus = NULL;
> +		kfree(dev->phy.def);
> +	}
> +}
> +
> +static int stub_setup_aneg(struct mii_phy *phy, u32 advertise)
> +{
> +	return 0;
> +}
> +
> +static int stub_setup_forced(struct mii_phy *phy, int speed, int fd)
> +{
> +	return 0;
> +}
> +
> +static int stub_poll_link(struct mii_phy *phy)
> +{
> +	struct net_device *ndev = phy->dev;
> +	struct emac_instance *dev = netdev_priv(ndev);
> +
> +	return dev->opened;
> +}
> +
> +static int stub_read_link(struct mii_phy *phy)
> +{
> +	struct net_device *ndev = phy->dev;
> +	struct emac_instance *dev = netdev_priv(ndev);
> +
> +	phy_start(dev->phy_dev);

Are you sure the read_link function is supposed to start the PHY state
machine? Either the name is confusing, or it's not the right thing to do
here.

> +	return 0;
> +}
> +
> +static const struct mii_phy_ops emac_stub_phy_ops = {
> +	.setup_aneg	= stub_setup_aneg,
> +	.setup_forced	= stub_setup_forced,
> +	.poll_link	= stub_poll_link,
> +	.read_link	= stub_read_link,
> +};
> +
> +static int emac_probe_dt_phy(struct emac_instance *dev)
> +{
> +	struct device_node *np = dev->ofdev->dev.of_node;
> +	struct device_node *phy_handle;
> +	struct net_device *ndev = dev->ndev;
> +	int res;
> +
> +	phy_handle = of_parse_phandle(np, "phy-handle", 0);
> +
> +	if (phy_handle) {
> +		res = emac_mdio_probe(dev);
> +		if (res)
> +			goto err_cleanup;
> +
> +		dev->phy.def = kzalloc(sizeof(*dev->phy.def), GFP_KERNEL);
> +		if (!dev->phy.def) {
> +			res = -ENOMEM;
> +			goto err_cleanup;
> +		}
> +
> +		dev->phy_dev = of_phy_connect(ndev, phy_handle,
> +					      &emac_adjust_link, 0,
> +					      PHY_INTERFACE_MODE_RGMII);

You should call of_get_phy_mode() since there should be a proper
"phy-mode" or "phy-connection-type" property describing how it's
connected to the EMAC.

> +		if (!dev->phy_dev) {
> +			res = -ENODEV;
> +			goto err_cleanup;
> +		}
> +
> +		of_node_put(phy_handle);
> +		dev->phy.def->phy_id = dev->phy_dev->drv->phy_id;
> +		dev->phy.def->phy_id_mask = dev->phy_dev->drv->phy_id_mask;
> +		dev->phy.def->name = dev->phy_dev->drv->name;
> +		dev->phy.def->ops = &emac_stub_phy_ops;
> +		/* Disable any PHY features not supported by the platform */
> +		dev->phy.def->features =  dev->phy_dev->drv->features &
> +					  ~dev->phy_feat_exc;
> +		dev->phy.features = dev->phy.def->features;
> +		dev->phy.address = dev->phy_dev->mdio.addr;
> +		dev->phy.mode = dev->phy_dev->interface;
> +		return 0;
> +	}
> +
> +	/* if the device tree didn't specifiy the the phy, then
> +	 * we simply fallback to the old emac_phy.c probe code
> +	 * for compatibility reasons.
> +	 */
> +	return 1;
> +
> + err_cleanup:
> +	of_node_put(phy_handle);
> +	kfree(dev->phy.def);
> +	return res;
> +}
> +
>  static int emac_init_phy(struct emac_instance *dev)
>  {
>  	struct device_node *np = dev->ofdev->dev.of_node;
> @@ -2490,6 +2664,13 @@ static int emac_init_phy(struct emac_instance *dev)
>  
>  	emac_configure(dev);
>  
> +	if (emac_has_feature(dev, EMAC_FTR_HAS_RGMII)) {
> +		int res = emac_probe_dt_phy(dev);
> +
> +		if (res <= 0)
> +			return res;
> +	}

Why is this limited to EMAC_FTR_HAS_RGMII here?

> +
>  	if (dev->phy_address != 0xffffffff)
>  		phy_map = ~(1 << dev->phy_address);
>  
> @@ -2938,6 +3119,8 @@ static int emac_probe(struct platform_device *ofdev)
>  	/* I have a bad feeling about this ... */
>  
>   err_detach_tah:
> +	emac_mdio_cleanup(dev);
> +
>  	if (emac_has_feature(dev, EMAC_FTR_HAS_TAH))
>  		tah_detach(dev->tah_dev, dev->tah_port);
>   err_detach_rgmii:
> @@ -2988,6 +3171,11 @@ static int emac_remove(struct platform_device *ofdev)
>  	if (emac_has_feature(dev, EMAC_FTR_HAS_ZMII))
>  		zmii_detach(dev->zmii_dev, dev->zmii_port);
>  
> +	if (dev->phy_dev)
> +		phy_disconnect(dev->phy_dev);
> +
> +	emac_mdio_cleanup(dev);
> +
>  	busy_phy_map &= ~(1 << dev->phy.address);
>  	DBG(dev, "busy_phy_map now %#x" NL, busy_phy_map);
>  
> diff --git a/drivers/net/ethernet/ibm/emac/core.h b/drivers/net/ethernet/ibm/emac/core.h
> index 93ae11494810..0710a6685489 100644
> --- a/drivers/net/ethernet/ibm/emac/core.h
> +++ b/drivers/net/ethernet/ibm/emac/core.h
> @@ -199,6 +199,10 @@ struct emac_instance {
>  	struct emac_instance		*mdio_instance;
>  	struct mutex			mdio_lock;
>  
> +	/* Device-tree based phy configuration */
> +	struct mii_bus			*mii_bus;
> +	struct phy_device		*phy_dev;
> +
>  	/* ZMII infos if any */
>  	u32				zmii_ph;
>  	u32				zmii_port;
> 


-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/2] dt: emac: document device-tree based phy discovery and setup

[Rob Herring]

On Sun, Feb 05, 2017 at 11:25:05PM +0100, Christian Lamparter wrote:
> This patch adds documentation for a new "phy-handler" property

s/phy-handler/phy-handle/

> and "mdio" sub-node. These allows the enumeration of PHYs which
> are supported by the phy library under drivers/net/phy.
> 
> The EMAC ethernet controller in IBM and AMCC 4xx chips is
> currently stuck with a few privately defined phy
> implementations. It has no support for PHYs which
> are supported by the generic phylib.
> 
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> ---
>  .../devicetree/bindings/powerpc/4xx/emac.txt       | 60 +++++++++++++++++++++-
>  1 file changed, 58 insertions(+), 2 deletions(-)

Otherwise,

Acked-by: Rob Herring <robh@kernel.org>

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

[Christian Lamparter]

Hello,

I'm sorry for the delay.

On Sunday, February 5, 2017 2:44:54 PM CET Florian Fainelli wrote:
> Le 02/05/17 à 14:25, Christian Lamparter a écrit :
> > From: Christian Lamparter <chunkeey-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > 
> > 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 88E1111 and 88E1112, AR8035.
> > However, routers like the Netgear WNDR4700 and Cisco Meraki MX60(W)
> > have a 5-port switch (QCA8327N) attached to the MDIO of the EMAC.
> > The switch chip has already a proper phy-driver (qca8k) that uses
> > the generic phy library.
> 
> Technically, it's a mdio_device in the upstream kernel that registers a
> switch with DSA (and a PHY device in the OpenWrt/LEDE downstream
> kernel). If your goal is to specifically support that device you should
> consider making the EMAC interface with a fixed link PHY to properly
> initialize the EMAC <=> CPU port of the switch link, and then declare
> the qca8k device as a child MDIO device (not a PHY), similar to what is
> done in arch/arm/boot/dts/vf610-zii-dev-rev-b.dts for instance.

Ok. I looked what was going on here. As you explained: qca8k is indeed 
the wrong driver. We do use the ar8216 with swconfig interface.

As for this patch. Currently the apm821xx target in LEDE has two supported
routers, on AP and one NAS.

Both routers: The Netgear WNDR4700 and the Cisco MX60(W) use the AR8327N.

The AP: The Cisco Meraki MR24 has a AR8035 PHY. There's the at803x. driver,
but David Miller was nice enough to merge this patch [0]. This patch added
support for it in in emac's phy.c, however it also limits it to the MR24.

The NAS: Western Digital My Book Live (Uno and Duo) have a Broadcom PHY
BCM54610 (it is detected as a BCM50610 PHY with a better version of this
patch). There's a proper phy driver in the kernel for it too (broadcom.c).
However, emac is limited to its own generic phy driver for this device.

Before I can answer the comments, I would like to deal with 
the kbuild-test-robot. It discovered the following issues:

|   drivers/built-in.o: In function `emac_mdio_cleanup.isra.2':
|>> core.c:(.text+0x70464): undefined reference to `mdiobus_free'
|>> core.c:(.text+0x70494): undefined reference to `mdiobus_unregister'
|   core.c:(.text+0x704a0): undefined reference to `mdiobus_free'
|   drivers/built-in.o: In function `emac_remove':
|>> core.c:(.text+0x70500): undefined reference to `phy_disconnect'

All these symbols are defined in include/linux/phy.h though.
So, shouldn't there be some stubs for those functions in the
header in case CONFIG_PHYLIB is not defined.
Is this a simple oversight, or is there more to it?
(I can add them if necessary. Or is someone looking for "easy" work?)

> > Signed-off-by: Christian Lamparter <chunkeey-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> > ---
> >  drivers/net/ethernet/ibm/emac/core.c | 188 +++++++++++++++++++++++++++++++++++
> >  drivers/net/ethernet/ibm/emac/core.h |   4 +
> >  2 files changed, 192 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> > index 6ead2335a169..ea9234cdb227 100644
> > --- a/drivers/net/ethernet/ibm/emac/core.c
> > +++ b/drivers/net/ethernet/ibm/emac/core.c
> > @@ -42,6 +42,7 @@
> >  #include <linux/of_address.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/of_net.h>
> > +#include <linux/of_mdio.h>
> >  #include <linux/slab.h>
> >  
> >  #include <asm/processor.h>
> > @@ -2420,6 +2421,179 @@ 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;
> > +
> > +	mutex_lock(&dev->link_lock);
> > +	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;
> > +	mutex_unlock(&dev->link_lock);
> 
> PHYLIB already executes grabbing the phy device's mutex, is this really
> needed here?
Yes, this is a bug. I accidently sent a very old version.
(In fact, the LEDE patch had it already fixed[1].)

> > +}
> > +
> > +static int emac_mii_bus_read(struct mii_bus *bus, int addr, int regnum)
> > +{
> > +	return emac_mdio_read(bus->priv, addr, regnum);
> > +}
> > +
> > +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);
> > +
> > +	emac_mii_reset_phy(&dev->phy);
> 
> This seems wrong, emac_mii_reset_phy() does a BMCR software reset, which
> PHYLIB is already going to do (phy_init_hw), yet you do this here at the
> MDIO bus level towards a specify PHY, whereas this should be affecting
> the MDIO bus itself (and/or *all* PHY child devices for quirks).
Ah, this is a good point. The emac driver has a emac_reset() function
that does disable and enabled the phy clocks. That said, this is already
done by the emac driver during init too. So if I added it, the bus is
reset twice (since it doesn't hurt - I added it back).

The emac_mii_phy_reset() was added because of the Meraki MX60(W).
This is because Cisco's bootloader disables the switch port 
(probably to prevent WAN<->LAN leakage during boot)

[bootlog from the MX60(W)]
|Disabling port 0
|Disabling port 1
|Disabling port 2
|Disabling port 3
|ENET Speed is 1000 Mbps - FULL duplex connection (EMAC0)

Without emac_mii_reset_phy(), the mdiobus_scan() function, which
is called by mdiobus_register will fail with -ENODEV.
| /plb/opb/ethernet@ef600c00: failed to attach dt phy (-19).
This is because get_phy_id() will "mostly read mostly Fs" and abort.


With emac_mii_reset_phy() in place, it gets detected:
| switch0: Atheros AR8327 rev. 4 switch registered on emac_mdio

Furthermore, this is probably not the only device which need it.
Currently, emac's own phy.c code does call emac_mii_reset_phy() 
as well as part of its probe procedure.
<http://lxr.free-electrons.com/source/drivers/net/ethernet/ibm/emac/phy.c#L522>

Ideally, we would like to reset only the ports which are registered in the DT.
Do you know if there's a good way to do that? We measured that it takes ~5
seconds to reset all 31 phys.

|[    1.405249] /plb/opb/emac-rgmii@ef601500: input 0 in RGMII mode
|[    1.663307] (phy 0 reset)
|...
|[    6.264852] (phy 31 reset)
|[    6.270056] libphy: emac_mdio: probed

> > +	return 0;
> > +}
> > +
> > +static int emac_mdio_probe(struct emac_instance *dev)
> > +{
> > +	struct device_node *mii_np;
> > +	struct mii_bus *bus;
> > +	int res;
> > +
> > +	bus = mdiobus_alloc();
> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	mii_np = of_get_child_by_name(dev->ofdev->dev.of_node, "mdio");
> > +	if (!mii_np) {
> > +		dev_err(&dev->ndev->dev, "no mdio definition found.");
> > +		return -ENODEV;
> > +	}
> > +
> > +	if (!of_device_is_available(mii_np))
> > +		return 0;
> > +
> > +	bus->priv = dev->ndev;
> > +	bus->parent = dev->ndev->dev.parent;
> > +	bus->name = "emac_mdio";
> > +	bus->read = &emac_mii_bus_read;
> > +	bus->write = &emac_mii_bus_write;
> > +	bus->reset = &emac_mii_bus_reset;
> > +
> > +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", bus->name);
> 
> You should pick a more unique name here, if you ever have a second
> instance it would just clash with the previous one.
I looked around what other drivers do. From what I can tell DT drivers
just stick with the of->name.

> > +
> > +	res = of_mdiobus_register(bus, mii_np);
> > +	if (res) {
> > +		dev_err(&dev->ndev->dev, "cannot register MDIO bus %s\n",
> > +			bus->name);
> > +		mdiobus_free(bus);
> > +	}
> > +
> > +	dev->mii_bus = bus;
> > +	return res;
> > +}
> > +
> > +static void emac_mdio_cleanup(struct emac_instance *dev)
> > +{
> > +	if (dev->mii_bus) {
> > +		if (dev->mii_bus->state == MDIOBUS_REGISTERED)
> > +			mdiobus_unregister(dev->mii_bus);
> 
> If you need to make that kind of check, why not separate how the mdio
> bus structure's lifecycle is managed? This seems to be avoiding to hit
> the BUG_ON() in mdiobus_unregister..
Yes, I converted it all to devres. 

> > +		mdiobus_free(dev->mii_bus);
> > +		dev->mii_bus = NULL;
> > +		kfree(dev->phy.def);
> > +	}
> > +}
> > +
> > +static int stub_setup_aneg(struct mii_phy *phy, u32 advertise)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int stub_setup_forced(struct mii_phy *phy, int speed, int fd)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int stub_poll_link(struct mii_phy *phy)
> > +{
> > +	struct net_device *ndev = phy->dev;
> > +	struct emac_instance *dev = netdev_priv(ndev);
> > +
> > +	return dev->opened;
> > +}
> > +
> > +static int stub_read_link(struct mii_phy *phy)
> > +{
> > +	struct net_device *ndev = phy->dev;
> > +	struct emac_instance *dev = netdev_priv(ndev);
> > +
> > +	phy_start(dev->phy_dev);
> 
> Are you sure the read_link function is supposed to start the PHY state
> machine? Either the name is confusing, or it's not the right thing to do
> here.
This was already fixed too :).
> 
> > +	return 0;
> > +}
> > +
> > +static const struct mii_phy_ops emac_stub_phy_ops = {
> > +	.setup_aneg	= stub_setup_aneg,
> > +	.setup_forced	= stub_setup_forced,
> > +	.poll_link	= stub_poll_link,
> > +	.read_link	= stub_read_link,
> > +};
> > +
> > +static int emac_probe_dt_phy(struct emac_instance *dev)
> > +{
> > +	struct device_node *np = dev->ofdev->dev.of_node;
> > +	struct device_node *phy_handle;
> > +	struct net_device *ndev = dev->ndev;
> > +	int res;
> > +
> > +	phy_handle = of_parse_phandle(np, "phy-handle", 0);
> > +
> > +	if (phy_handle) {
> > +		res = emac_mdio_probe(dev);
> > +		if (res)
> > +			goto err_cleanup;
> > +
> > +		dev->phy.def = kzalloc(sizeof(*dev->phy.def), GFP_KERNEL);
> > +		if (!dev->phy.def) {
> > +			res = -ENOMEM;
> > +			goto err_cleanup;
> > +		}
> > +
> > +		dev->phy_dev = of_phy_connect(ndev, phy_handle,
> > +					      &emac_adjust_link, 0,
> > +					      PHY_INTERFACE_MODE_RGMII);
> 
> You should call of_get_phy_mode() since there should be a proper
> "phy-mode" or "phy-connection-type" property describing how it's
> connected to the EMAC.
of_get_phy_mode() is already called by emac.c as part of the 
emac_init_config() function. I changed it to dev->phy_mode. 

> > +		if (!dev->phy_dev) {
> > +			res = -ENODEV;
> > +			goto err_cleanup;
> > +		}
> > +
> > +		of_node_put(phy_handle);
> > +		dev->phy.def->phy_id = dev->phy_dev->drv->phy_id;
> > +		dev->phy.def->phy_id_mask = dev->phy_dev->drv->phy_id_mask;
> > +		dev->phy.def->name = dev->phy_dev->drv->name;
> > +		dev->phy.def->ops = &emac_stub_phy_ops;
> > +		/* Disable any PHY features not supported by the platform */
> > +		dev->phy.def->features =  dev->phy_dev->drv->features &
> > +					  ~dev->phy_feat_exc;
> > +		dev->phy.features = dev->phy.def->features;
> > +		dev->phy.address = dev->phy_dev->mdio.addr;
> > +		dev->phy.mode = dev->phy_dev->interface;
> > +		return 0;
> > +	}
> > +
> > +	/* if the device tree didn't specifiy the the phy, then
> > +	 * we simply fallback to the old emac_phy.c probe code
> > +	 * for compatibility reasons.
> > +	 */
> > +	return 1;
> > +
> > + err_cleanup:
> > +	of_node_put(phy_handle);
> > +	kfree(dev->phy.def);
> > +	return res;
> > +}
> > +
> >  static int emac_init_phy(struct emac_instance *dev)
> >  {
> >  	struct device_node *np = dev->ofdev->dev.of_node;
> > @@ -2490,6 +2664,13 @@ static int emac_init_phy(struct emac_instance *dev)
> >  
> >  	emac_configure(dev);
> >  
> > +	if (emac_has_feature(dev, EMAC_FTR_HAS_RGMII)) {
> > +		int res = emac_probe_dt_phy(dev);
> > +
> > +		if (res <= 0)
> > +			return res;
> > +	}
> 
> Why is this limited to EMAC_FTR_HAS_RGMII here?
This is because, this code is only tested with RGMII.
SGMII has a separate set of mii_read/write/reset functions and
without a device to test the functionality, I don't really want
to add it.

> > +
> >  	if (dev->phy_address != 0xffffffff)
> >  		phy_map = ~(1 << dev->phy_address);
> >  
> > @@ -2938,6 +3119,8 @@ static int emac_probe(struct platform_device *ofdev)
> >  	/* I have a bad feeling about this ... */
> >  
> >   err_detach_tah:
> > +	emac_mdio_cleanup(dev);
> > +
> >  	if (emac_has_feature(dev, EMAC_FTR_HAS_TAH))
> >  		tah_detach(dev->tah_dev, dev->tah_port);
> >   err_detach_rgmii:
> > @@ -2988,6 +3171,11 @@ static int emac_remove(struct platform_device *ofdev)
> >  	if (emac_has_feature(dev, EMAC_FTR_HAS_ZMII))
> >  		zmii_detach(dev->zmii_dev, dev->zmii_port);
> >  
> > +	if (dev->phy_dev)
> > +		phy_disconnect(dev->phy_dev);
> > +
> > +	emac_mdio_cleanup(dev);
> > +
> >  	busy_phy_map &= ~(1 << dev->phy.address);
> >  	DBG(dev, "busy_phy_map now %#x" NL, busy_phy_map);
> >  
> > diff --git a/drivers/net/ethernet/ibm/emac/core.h b/drivers/net/ethernet/ibm/emac/core.h
> > index 93ae11494810..0710a6685489 100644
> > --- a/drivers/net/ethernet/ibm/emac/core.h
> > +++ b/drivers/net/ethernet/ibm/emac/core.h
> > @@ -199,6 +199,10 @@ struct emac_instance {
> >  	struct emac_instance		*mdio_instance;
> >  	struct mutex			mdio_lock;
> >  
> > +	/* Device-tree based phy configuration */
> > +	struct mii_bus			*mii_bus;
> > +	struct phy_device		*phy_dev;
> > +
> >  	/* ZMII infos if any */
> >  	u32				zmii_ph;
> >  	u32				zmii_port;
> > 
>

[0] <https://patchwork.ozlabs.org/patch/617930/>
[1] <https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/apm821xx/patches-4.4/702-powerpc_ibm_phy_add_dt_parser.patch;h=c84e761ed02efe881a20adc0d275e4e4e74589a3;hb=6c6167621f3aba358742d68aeaed8dd360254ad6>
[2] <https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/apm821xx/patches-4.9/702-powerpc_ibm_phy_add_dt_parser.patch;h=c84e761ed02efe881a20adc0d275e4e4e74589a3;hb=6c6167621f3aba358742d68aeaed8dd360254ad6>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Florian Fainelli]

Le 02/11/17 à 14:45, Christian Lamparter a écrit :
> Hello,
> 
> I'm sorry for the delay.
> 
> On Sunday, February 5, 2017 2:44:54 PM CET Florian Fainelli wrote:
>> Le 02/05/17 à 14:25, Christian Lamparter a écrit :
>>> From: Christian Lamparter <chunkeey@gmail.com>
>>>
>>> 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 88E1111 and 88E1112, AR8035.
>>> However, routers like the Netgear WNDR4700 and Cisco Meraki MX60(W)
>>> have a 5-port switch (QCA8327N) attached to the MDIO of the EMAC.
>>> The switch chip has already a proper phy-driver (qca8k) that uses
>>> the generic phy library.
>>
>> Technically, it's a mdio_device in the upstream kernel that registers a
>> switch with DSA (and a PHY device in the OpenWrt/LEDE downstream
>> kernel). If your goal is to specifically support that device you should
>> consider making the EMAC interface with a fixed link PHY to properly
>> initialize the EMAC <=> CPU port of the switch link, and then declare
>> the qca8k device as a child MDIO device (not a PHY), similar to what is
>> done in arch/arm/boot/dts/vf610-zii-dev-rev-b.dts for instance.
> 
> Ok. I looked what was going on here. As you explained: qca8k is indeed 
> the wrong driver. We do use the ar8216 with swconfig interface.

Can you look into adding support for the 8216 into
drivers/net/dsa/qca8k.c? You don't necessarily need to use QCA tags
(using DSA_PROTO_NONE works too) and this would be a good way to know
what could be missing in that driver, you'd also get per-port network
devices, which could all be driving their built-in PHYs (so ethtool and
friends work as expected).

> 
> As for this patch. Currently the apm821xx target in LEDE has two supported
> routers, on AP and one NAS.
> 
> Both routers: The Netgear WNDR4700 and the Cisco MX60(W) use the AR8327N.
> 
> The AP: The Cisco Meraki MR24 has a AR8035 PHY. There's the at803x. driver,
> but David Miller was nice enough to merge this patch [0]. This patch added
> support for it in in emac's phy.c, however it also limits it to the MR24.
> 
> The NAS: Western Digital My Book Live (Uno and Duo) have a Broadcom PHY
> BCM54610 (it is detected as a BCM50610 PHY with a better version of this
> patch). There's a proper phy driver in the kernel for it too (broadcom.c).
> However, emac is limited to its own generic phy driver for this device.
> 
> Before I can answer the comments, I would like to deal with 
> the kbuild-test-robot. It discovered the following issues:
> 
> |   drivers/built-in.o: In function `emac_mdio_cleanup.isra.2':
> |>> core.c:(.text+0x70464): undefined reference to `mdiobus_free'
> |>> core.c:(.text+0x70494): undefined reference to `mdiobus_unregister'
> |   core.c:(.text+0x704a0): undefined reference to `mdiobus_free'
> |   drivers/built-in.o: In function `emac_remove':
> |>> core.c:(.text+0x70500): undefined reference to `phy_disconnect'
> 
> All these symbols are defined in include/linux/phy.h though.
> So, shouldn't there be some stubs for those functions in the
> header in case CONFIG_PHYLIB is not defined.
> Is this a simple oversight, or is there more to it?
> (I can add them if necessary. Or is someone looking for "easy" work?)

I am not clear how you ran into that build failure, don't you select
PHYLIB? You still need PHYLIB even if you implement a MDIO device driver
for the switch.

> 
>>> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
>>> ---
>>>  drivers/net/ethernet/ibm/emac/core.c | 188 +++++++++++++++++++++++++++++++++++
>>>  drivers/net/ethernet/ibm/emac/core.h |   4 +
>>>  2 files changed, 192 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
>>> index 6ead2335a169..ea9234cdb227 100644
>>> --- a/drivers/net/ethernet/ibm/emac/core.c
>>> +++ b/drivers/net/ethernet/ibm/emac/core.c
>>> @@ -42,6 +42,7 @@
>>>  #include <linux/of_address.h>
>>>  #include <linux/of_irq.h>
>>>  #include <linux/of_net.h>
>>> +#include <linux/of_mdio.h>
>>>  #include <linux/slab.h>
>>>  
>>>  #include <asm/processor.h>
>>> @@ -2420,6 +2421,179 @@ 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;
>>> +
>>> +	mutex_lock(&dev->link_lock);
>>> +	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;
>>> +	mutex_unlock(&dev->link_lock);
>>
>> PHYLIB already executes grabbing the phy device's mutex, is this really
>> needed here?
> Yes, this is a bug. I accidently sent a very old version.
> (In fact, the LEDE patch had it already fixed[1].)
> 
>>> +}
>>> +
>>> +static int emac_mii_bus_read(struct mii_bus *bus, int addr, int regnum)
>>> +{
>>> +	return emac_mdio_read(bus->priv, addr, regnum);
>>> +}
>>> +
>>> +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);
>>> +
>>> +	emac_mii_reset_phy(&dev->phy);
>>
>> This seems wrong, emac_mii_reset_phy() does a BMCR software reset, which
>> PHYLIB is already going to do (phy_init_hw), yet you do this here at the
>> MDIO bus level towards a specify PHY, whereas this should be affecting
>> the MDIO bus itself (and/or *all* PHY child devices for quirks).
> Ah, this is a good point. The emac driver has a emac_reset() function
> that does disable and enabled the phy clocks. That said, this is already
> done by the emac driver during init too. So if I added it, the bus is
> reset twice (since it doesn't hurt - I added it back).
> 
> The emac_mii_phy_reset() was added because of the Meraki MX60(W).
> This is because Cisco's bootloader disables the switch port 
> (probably to prevent WAN<->LAN leakage during boot)
> 
> [bootlog from the MX60(W)]
> |Disabling port 0
> |Disabling port 1
> |Disabling port 2
> |Disabling port 3
> |ENET Speed is 1000 Mbps - FULL duplex connection (EMAC0)
> 
> Without emac_mii_reset_phy(), the mdiobus_scan() function, which
> is called by mdiobus_register will fail with -ENODEV.
> | /plb/opb/ethernet@ef600c00: failed to attach dt phy (-19).
> This is because get_phy_id() will "mostly read mostly Fs" and abort.

Is the PHY just powered down by chance (BMCR_PWRDN set?) and resetting
it implicitly clears the power down that seems to be what is going on.

Keep in mind that MDIO address 16 is the switch's pseudo PHY address
here, so if you are telling PHYLIB to probe for that address and you
don't get the expected MII_PHYSID1/2 value in return, that usually means
that there was a PHY fixup registered to intercept these reads and make
us return the switch's unique identifier. Reading from the switch's
pseudo PHY at address 16 registers 2/3 (MII_PHYSID1/2) is not guaranteed
to return the switch's unique identifer.

With a MDIO device driver this won't happen because you will be probed
by address, and you can read any switch register you want to and from
there move on with the initialization.

> 
> 
> With emac_mii_reset_phy() in place, it gets detected:
> | switch0: Atheros AR8327 rev. 4 switch registered on emac_mdio
> 
> Furthermore, this is probably not the only device which need it.
> Currently, emac's own phy.c code does call emac_mii_reset_phy() 
> as well as part of its probe procedure.
> <http://lxr.free-electrons.com/source/drivers/net/ethernet/ibm/emac/phy.c#L522>
> 
> Ideally, we would like to reset only the ports which are registered in the DT.

Which you would get for free if you did extend qca8k to support the
8216, because qca8k does implicitly tell the DSA layer to register a
dsa_slave_mii_bus which will probe and attach to per-port built-in PHYs
and that happens only for the ports enabled on your specific board.

> Do you know if there's a good way to do that? We measured that it takes ~5
> seconds to reset all 31 phys.

AFAICT there is no good way (without becoming too complex) to reset a
vector of PHYs and then just come back every 50ms or to see which ones
are reset or not.

NB: on some top of the rack switches, MDIO address 0 acts as a broadcast
address and you can use that feature to write to many, that still poses
the question of the read though which needs to be done for all PHYs to
know if the reset has completed.

> 
> |[    1.405249] /plb/opb/emac-rgmii@ef601500: input 0 in RGMII mode
> |[    1.663307] (phy 0 reset)
> |...
> |[    6.264852] (phy 31 reset)
> |[    6.270056] libphy: emac_mdio: probed
> 
>>> +	return 0;
>>> +}
>>> +
>>> +static int emac_mdio_probe(struct emac_instance *dev)
>>> +{
>>> +	struct device_node *mii_np;
>>> +	struct mii_bus *bus;
>>> +	int res;
>>> +
>>> +	bus = mdiobus_alloc();
>>> +	if (!bus)
>>> +		return -ENOMEM;
>>> +
>>> +	mii_np = of_get_child_by_name(dev->ofdev->dev.of_node, "mdio");
>>> +	if (!mii_np) {
>>> +		dev_err(&dev->ndev->dev, "no mdio definition found.");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	if (!of_device_is_available(mii_np))
>>> +		return 0;
>>> +
>>> +	bus->priv = dev->ndev;
>>> +	bus->parent = dev->ndev->dev.parent;
>>> +	bus->name = "emac_mdio";
>>> +	bus->read = &emac_mii_bus_read;
>>> +	bus->write = &emac_mii_bus_write;
>>> +	bus->reset = &emac_mii_bus_reset;
>>> +
>>> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", bus->name);
>>
>> You should pick a more unique name here, if you ever have a second
>> instance it would just clash with the previous one.
> I looked around what other drivers do. From what I can tell DT drivers
> just stick with the of->name.

My comment still stands, if you have two instances of this bus in a
system, the second will clash with the first one. You can just use
np->full_name or just use a driver private static index + bus->name to
create an unique enough name.

>>> +		dev->phy_dev = of_phy_connect(ndev, phy_handle,
>>> +					      &emac_adjust_link, 0,
>>> +					      PHY_INTERFACE_MODE_RGMII);
>>
>> You should call of_get_phy_mode() since there should be a proper
>> "phy-mode" or "phy-connection-type" property describing how it's
>> connected to the EMAC.
> of_get_phy_mode() is already called by emac.c as part of the 
> emac_init_config() function. I changed it to dev->phy_mode. 

Great thanks!

> 
>>> +		if (!dev->phy_dev) {
>>> +			res = -ENODEV;
>>> +			goto err_cleanup;
>>> +		}
>>> +
>>> +		of_node_put(phy_handle);
>>> +		dev->phy.def->phy_id = dev->phy_dev->drv->phy_id;
>>> +		dev->phy.def->phy_id_mask = dev->phy_dev->drv->phy_id_mask;
>>> +		dev->phy.def->name = dev->phy_dev->drv->name;
>>> +		dev->phy.def->ops = &emac_stub_phy_ops;
>>> +		/* Disable any PHY features not supported by the platform */
>>> +		dev->phy.def->features =  dev->phy_dev->drv->features &
>>> +					  ~dev->phy_feat_exc;
>>> +		dev->phy.features = dev->phy.def->features;
>>> +		dev->phy.address = dev->phy_dev->mdio.addr;
>>> +		dev->phy.mode = dev->phy_dev->interface;
>>> +		return 0;
>>> +	}
>>> +
>>> +	/* if the device tree didn't specifiy the the phy, then
>>> +	 * we simply fallback to the old emac_phy.c probe code
>>> +	 * for compatibility reasons.
>>> +	 */
>>> +	return 1;
>>> +
>>> + err_cleanup:
>>> +	of_node_put(phy_handle);
>>> +	kfree(dev->phy.def);
>>> +	return res;
>>> +}
>>> +
>>>  static int emac_init_phy(struct emac_instance *dev)
>>>  {
>>>  	struct device_node *np = dev->ofdev->dev.of_node;
>>> @@ -2490,6 +2664,13 @@ static int emac_init_phy(struct emac_instance *dev)
>>>  
>>>  	emac_configure(dev);
>>>  
>>> +	if (emac_has_feature(dev, EMAC_FTR_HAS_RGMII)) {
>>> +		int res = emac_probe_dt_phy(dev);
>>> +
>>> +		if (res <= 0)
>>> +			return res;
>>> +	}
>>
>> Why is this limited to EMAC_FTR_HAS_RGMII here?
> This is because, this code is only tested with RGMII.
> SGMII has a separate set of mii_read/write/reset functions and
> without a device to test the functionality, I don't really want
> to add it.

OK fair enough and that makes sense.
-- 
Florian

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

[Christian Lamparter]

On Saturday, February 11, 2017 3:07:04 PM CET Florian Fainelli wrote:
> Le 02/11/17 à 14:45, Christian Lamparter a écrit :
> > On Sunday, February 5, 2017 2:44:54 PM CET Florian Fainelli wrote:
> >> Le 02/05/17 à 14:25, Christian Lamparter a écrit :
> >>> From: Christian Lamparter <chunkeey@gmail.com>
> >>>
> >>> 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 88E1111 and 88E1112, AR8035.
> >>> However, routers like the Netgear WNDR4700 and Cisco Meraki MX60(W)
> >>> have a 5-port switch (QCA8327N) attached to the MDIO of the EMAC.
> >>> The switch chip has already a proper phy-driver (qca8k) that uses
> >>> the generic phy library.
> >>
> >> Technically, it's a mdio_device in the upstream kernel that registers a
> >> switch with DSA (and a PHY device in the OpenWrt/LEDE downstream
> >> kernel). If your goal is to specifically support that device you should
> >> consider making the EMAC interface with a fixed link PHY to properly
> >> initialize the EMAC <=> CPU port of the switch link, and then declare
> >> the qca8k device as a child MDIO device (not a PHY), similar to what is
> >> done in arch/arm/boot/dts/vf610-zii-dev-rev-b.dts for instance.
> > 
> > Ok. I looked what was going on here. As you explained: qca8k is indeed 
> > the wrong driver. We do use the ar8216 with swconfig interface.
> 
> Can you look into adding support for the 8216 into
> drivers/net/dsa/qca8k.c? You don't necessarily need to use QCA tags
> (using DSA_PROTO_NONE works too) and this would be a good way to know
> what could be missing in that driver, you'd also get per-port network
> devices, which could all be driving their built-in PHYs (so ethtool and
> friends work as expected).
Oh, I don't have any devices with an AR8216. Both the Meraki MX60(W) and
the WNDR4700 have the AR8327. I only mentioned AR8216, because that's
the driver module in OpenWRT/LEDE which supports the AR8327 [0], [1].

As for emac and AR8327N: It will come right up, once the QCA8337-only guard 
is removed from qca8k.c [2]. [QCA8K_ID_QCA8337 is 0x13, AR8327N is 0x12]:
|954	if (id != QCA8K_ID_QCA8337)
|955			return -ENODEV;

[    4.250097] libphy: emac_mdio: probed
[    4.253789] mdio_bus 4ef600c00.ethern:10: mdio_device_register
[    4.346679] eth0: EMAC-0 /plb/opb/ethernet@ef600c00, MAC 10:0d:7f:4e:20:6e
[...]
[    4.425333] DSA: switch 0 0 parsed
[    4.428751] DSA: tree 0 parsed
[    4.496094] libphy: dsa slave smi: probed
[    4.513056] Generic PHY 4ef600c00.ethern:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=4ef600c00.ethern:00, irq=-1)
[    4.524916] Generic PHY dsa-0.0:01: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:01, irq=-1)
[    4.535219] Generic PHY dsa-0.0:02: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:02, irq=-1)
[    4.545504] Generic PHY dsa-0.0:03: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:03, irq=-1)
[    4.555815] Generic PHY dsa-0.0:04: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:04, irq=-1)

# ip link
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000
    link/ether 10:0d:7f:4e:20:6e brd ff:ff:ff:ff:ff:ff
3: lan4@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue switchid 00000000 state UP mode DEFAULT group default qlen 1000
    link/ether 10:0d:7f:4e:20:6e brd ff:ff:ff:ff:ff:ff
4: lan3@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop switchid 00000000 state DOWN mode DEFAULT group default qlen 1000
    link/ether 10:0d:7f:4e:20:6e brd ff:ff:ff:ff:ff:ff
5: lan2@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop switchid 00000000 state DOWN mode DEFAULT group default qlen 1000
    link/ether 10:0d:7f:4e:20:6e brd ff:ff:ff:ff:ff:ff
6: lan1@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop switchid 00000000 state DOWN mode DEFAULT group default qlen 1000
    link/ether 10:0d:7f:4e:20:6e brd ff:ff:ff:ff:ff:ff
7: wan@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop switchid 00000000 state DOWN mode DEFAULT group default qlen 1000
    link/ether 10:0d:7f:4e:20:6e brd ff:ff:ff:ff:ff:ff
 
> > As for this patch. Currently the apm821xx target in LEDE has two supported
> > routers, on AP and one NAS.
> > 
> > Both routers: The Netgear WNDR4700 and the Cisco MX60(W) use the AR8327N.
> > 
> > The AP: The Cisco Meraki MR24 has a AR8035 PHY. There's the at803x. driver,
> > but David Miller was nice enough to merge this patch [0]. This patch added
> > support for it in in emac's phy.c, however it also limits it to the MR24.
> > 
> > The NAS: Western Digital My Book Live (Uno and Duo) have a Broadcom PHY
> > BCM54610 (it is detected as a BCM50610 PHY with a better version of this
> > patch). There's a proper phy driver in the kernel for it too (broadcom.c).
> > However, emac is limited to its own generic phy driver for this device.
> > 
> > Before I can answer the comments, I would like to deal with 
> > the kbuild-test-robot. It discovered the following issues:
> > 
> > |   drivers/built-in.o: In function `emac_mdio_cleanup.isra.2':
> > |>> core.c:(.text+0x70464): undefined reference to `mdiobus_free'
> > |>> core.c:(.text+0x70494): undefined reference to `mdiobus_unregister'
> > |   core.c:(.text+0x704a0): undefined reference to `mdiobus_free'
> > |   drivers/built-in.o: In function `emac_remove':
> > |>> core.c:(.text+0x70500): undefined reference to `phy_disconnect'
> > 
> > All these symbols are defined in include/linux/phy.h though.
> > So, shouldn't there be some stubs for those functions in the
> > header in case CONFIG_PHYLIB is not defined.
> > Is this a simple oversight, or is there more to it?
> > (I can add them if necessary. Or is someone looking for "easy" work?)
> 
> I am not clear how you ran into that build failure, don't you select
> PHYLIB? You still need PHYLIB even if you implement a MDIO device driver
> for the switch.
No, I didn't add it, because technically the emac.c has its own private
implementation for just the listed PHYs and they won't need PHYLIB.
However, once you had selected a PHY/DSA driver, PHYLIB was pulled in
automatically. So, I never spotted this because I always had the
broadcom.c PHY driver selected. 

> >>> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> >>> ---
> >>>  drivers/net/ethernet/ibm/emac/core.c | 188 +++++++++++++++++++++++++++++++++++
> >>>  drivers/net/ethernet/ibm/emac/core.h |   4 +
> >>>  2 files changed, 192 insertions(+)
> >>>
> >>> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> >>> index 6ead2335a169..ea9234cdb227 100644
> >>> --- a/drivers/net/ethernet/ibm/emac/core.c
> >>> +++ b/drivers/net/ethernet/ibm/emac/core.c
> >>> @@ -2420,6 +2421,179 @@ static int emac_read_uint_prop(struct device_node *np, const char *name,
> >>> [...] 
> >>> +static int emac_mii_bus_reset(struct mii_bus *bus)
> >>> +{
> >>> +	struct emac_instance *dev = netdev_priv(bus->priv);
> >>> +
> >>> +	emac_mii_reset_phy(&dev->phy);
> >>
> >> This seems wrong, emac_mii_reset_phy() does a BMCR software reset, which
> >> PHYLIB is already going to do (phy_init_hw), yet you do this here at the
> >> MDIO bus level towards a specify PHY, whereas this should be affecting
> >> the MDIO bus itself (and/or *all* PHY child devices for quirks).
> > Ah, this is a good point. The emac driver has a emac_reset() function
> > that does disable and enabled the phy clocks. That said, this is already
> > done by the emac driver during init too. So if I added it, the bus is
> > reset twice (since it doesn't hurt - I added it back).
> > 
> > The emac_mii_phy_reset() was added because of the Meraki MX60(W).
> > This is because Cisco's bootloader disables the switch port 
> > (probably to prevent WAN<->LAN leakage during boot)
> > 
> > [bootlog from the MX60(W)]
> > |Disabling port 0
> > |Disabling port 1
> > |Disabling port 2
> > |Disabling port 3
> > |ENET Speed is 1000 Mbps - FULL duplex connection (EMAC0)
> > 
> > Without emac_mii_reset_phy(), the mdiobus_scan() function, which
> > is called by mdiobus_register will fail with -ENODEV.
> > | /plb/opb/ethernet@ef600c00: failed to attach dt phy (-19).
> > This is because get_phy_id() will "mostly read mostly Fs" and abort.
> 
> Is the PHY just powered down by chance (BMCR_PWRDN set?) and resetting
> it implicitly clears the power down that seems to be what is going on.

Yes, the PHY is just in the BMCR_PDOWN state. I can do the same
on the WNDR4700, by messing with u-boot:

| => mii write 0 0 0x0800
| => mii dump
| 0.     (ffff)                 -- PHY control register --
|  (8000:8000) 0.15    =     1    reset
|  (4000:4000) 0.14    =     1    loopback
|  (2040:2040) 0. 6,13 =   b11    speed selection = ??? Mbps
|  (1000:1000) 0.12    =     1    A/N enable
|  (0800:0800) 0.11    =     1    power-down
|  (0400:0400) 0.10    =     1    isolate
|  (0200:0200) 0. 9    =     1    restart A/N
|  (0100:0100) 0. 8    =     1    duplex = full
|  (0080:0080) 0. 7    =     1    collision test enable
|  (003f:003f) 0. 5- 0 =    63    (reserved)

On the Meraki, the port disabled by the bootloader. 
The reset is still needed.

> Keep in mind that MDIO address 16 is the switch's pseudo PHY address
> here, so if you are telling PHYLIB to probe for that address and you
> don't get the expected MII_PHYSID1/2 value in return, that usually means
> that there was a PHY fixup registered to intercept these reads and make
> us return the switch's unique identifier. Reading from the switch's
> pseudo PHY at address 16 registers 2/3 (MII_PHYSID1/2) is not guaranteed
> to return the switch's unique identifer.
> 
> With a MDIO device driver this won't happen because you will be probed
> by address, and you can read any switch register you want to and from
> there move on with the initialization.
> 
> > 
> > 
> > With emac_mii_reset_phy() in place, it gets detected:
> > | switch0: Atheros AR8327 rev. 4 switch registered on emac_mdio
> > 
> > Furthermore, this is probably not the only device which need it.
> > Currently, emac's own phy.c code does call emac_mii_reset_phy() 
> > as well as part of its probe procedure.
> > <http://lxr.free-electrons.com/source/drivers/net/ethernet/ibm/emac/phy.c#L522>
> > 
> > Ideally, we would like to reset only the ports which are registered in the DT.
> 
> Which you would get for free if you did extend qca8k to support the
> 8327, because qca8k does implicitly tell the DSA layer to register a
> dsa_slave_mii_bus which will probe and attach to per-port built-in PHYs
> and that happens only for the ports enabled on your specific board.
No, the Meraki and the modified WNDR4700 still refuse to work. Just >one<
of the phys at address 0-4 need to be powered down to get the following
error:
[    4.425618] DSA: switch 0 0 parsed
[    4.429034] DSA: tree 0 parsed
[    4.435416] qca8k: probe of 4ef600c00.ethern:10 failed with error -5

I'll report back, what exactly is causing the error in this case.
> > Do you know if there's a good way to do that? We measured that it takes ~5
> > seconds to reset all 31 phys.
> 
> AFAICT there is no good way (without becoming too complex) to reset a
> vector of PHYs and then just come back every 50ms or to see which ones
> are reset or not.
> 
> NB: on some top of the rack switches, MDIO address 0 acts as a broadcast
> address and you can use that feature to write to many, that still poses
> the question of the read though which needs to be done for all PHYs to
> know if the reset has completed.
That's good to know. On the AR8327, each of the five phys just map to one
of the four lan or wan ports.

> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int emac_mdio_probe(struct emac_instance *dev)
> >>> +{
> >>> +	struct device_node *mii_np;
> >>> +	struct mii_bus *bus;
> >>> +	int res;
> >>> +
> >>> +	bus = mdiobus_alloc();
> >>> +	if (!bus)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	mii_np = of_get_child_by_name(dev->ofdev->dev.of_node, "mdio");
> >>> +	if (!mii_np) {
> >>> +		dev_err(&dev->ndev->dev, "no mdio definition found.");
> >>> +		return -ENODEV;
> >>> +	}
> >>> +
> >>> +	if (!of_device_is_available(mii_np))
> >>> +		return 0;
> >>> +
> >>> +	bus->priv = dev->ndev;
> >>> +	bus->parent = dev->ndev->dev.parent;
> >>> +	bus->name = "emac_mdio";
> >>> +	bus->read = &emac_mii_bus_read;
> >>> +	bus->write = &emac_mii_bus_write;
> >>> +	bus->reset = &emac_mii_bus_reset;
> >>> +
> >>> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", bus->name);
> >>
> >> You should pick a more unique name here, if you ever have a second
> >> instance it would just clash with the previous one.
> > I looked around what other drivers do. From what I can tell DT drivers
> > just stick with the of->name.
> 
> My comment still stands, if you have two instances of this bus in a
> system, the second will clash with the first one. You can just use
> np->full_name or just use a driver private static index + bus->name to
> create an unique enough name.
Oh, I forgot to say that I changed it. It uses the dt node name, just
like everybody else. 
 
Regards,
Christian

[0] <https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/files/drivers/net/phy/ar8327.c;h=9b40cd7e4259e1ca8202a607a2d4701d3e903707;hb=d5221d5a419c14456bccba9f6825567839082fb0>
[1] <https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/files/drivers/net/phy/ar8216.c;h=f3c953b808ee924493a4de9204bba2fb7906b0bf;hb=d5221d5a419c14456bccba9f6825567839082fb0>
[2] <http://lxr.free-electrons.com/source/drivers/net/dsa/qca8k.c#L954>

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

[Christian Lamparter]

On Tuesday, February 14, 2017 12:38:42 AM CET Christian Lamparter wrote:
> > >>> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> > >>> --- a/drivers/net/ethernet/ibm/emac/core.c
> > >>> +++ b/drivers/net/ethernet/ibm/emac/core.c
> > >>> @@ -2420,6 +2421,179 @@ static int emac_read_uint_prop(struct device_node *np, const char *name,
> > >>> [...] 
> > >>> +static int emac_mii_bus_reset(struct mii_bus *bus)
> > >>> +{
> > >>> +	struct emac_instance *dev = netdev_priv(bus->priv);
> > >>> +
> > >>> +	emac_mii_reset_phy(&dev->phy);
> > >>
> > >> This seems wrong, emac_mii_reset_phy() does a BMCR software reset, which
> > >> PHYLIB is already going to do (phy_init_hw), yet you do this here at the
> > >> MDIO bus level towards a specify PHY, whereas this should be affecting
> > >> the MDIO bus itself (and/or *all* PHY child devices for quirks).
> > > Ah, this is a good point. The emac driver has a emac_reset() function
> > > that does disable and enabled the phy clocks. That said, this is already
> > > done by the emac driver during init too. So if I added it, the bus is
> > > reset twice (since it doesn't hurt - I added it back).
> > > 
> > > The emac_mii_phy_reset() was added because of the Meraki MX60(W).
> > > This is because Cisco's bootloader disables the switch port 
> > > (probably to prevent WAN<->LAN leakage during boot)
> > > 
> > > [bootlog from the MX60(W)]
> > > |Disabling port 0
> > > |Disabling port 1
> > > |Disabling port 2
> > > |Disabling port 3
> > > |ENET Speed is 1000 Mbps - FULL duplex connection (EMAC0)
> > > 
> > > Without emac_mii_reset_phy(), the mdiobus_scan() function, which
> > > is called by mdiobus_register will fail with -ENODEV.
> > > | /plb/opb/ethernet@ef600c00: failed to attach dt phy (-19).
> > > This is because get_phy_id() will "mostly read mostly Fs" and abort.
> > 
> > Is the PHY just powered down by chance (BMCR_PWRDN set?) and resetting
> > it implicitly clears the power down that seems to be what is going on.
> 
> Yes, the PHY is just in the BMCR_PDOWN state. I can do the same
> on the WNDR4700, by messing with u-boot:
> 
> | => mii write 0 0 0x0800
> | => mii dump
> | 0.     (ffff)                 -- PHY control register --
> |  (8000:8000) 0.15    =     1    reset
> |  (4000:4000) 0.14    =     1    loopback
> |  (2040:2040) 0. 6,13 =   b11    speed selection = ??? Mbps
> |  (1000:1000) 0.12    =     1    A/N enable
> |  (0800:0800) 0.11    =     1    power-down
> |  (0400:0400) 0.10    =     1    isolate
> |  (0200:0200) 0. 9    =     1    restart A/N
> |  (0100:0100) 0. 8    =     1    duplex = full
> |  (0080:0080) 0. 7    =     1    collision test enable
> |  (003f:003f) 0. 5- 0 =    63    (reserved)
> 
> On the Meraki, the port disabled by the bootloader. 
> The reset is still needed.
> 
> > Keep in mind that MDIO address 16 is the switch's pseudo PHY address
> > here, so if you are telling PHYLIB to probe for that address and you
> > don't get the expected MII_PHYSID1/2 value in return, that usually means
> > that there was a PHY fixup registered to intercept these reads and make
> > us return the switch's unique identifier. Reading from the switch's
> > pseudo PHY at address 16 registers 2/3 (MII_PHYSID1/2) is not guaranteed
> > to return the switch's unique identifer.
> > 
> > With a MDIO device driver this won't happen because you will be probed
> > by address, and you can read any switch register you want to and from
> > there move on with the initialization.
> > 
> > > 
> > > 
> > > With emac_mii_reset_phy() in place, it gets detected:
> > > | switch0: Atheros AR8327 rev. 4 switch registered on emac_mdio
> > > 
> > > Furthermore, this is probably not the only device which need it.
> > > Currently, emac's own phy.c code does call emac_mii_reset_phy() 
> > > as well as part of its probe procedure.
> > > <http://lxr.free-electrons.com/source/drivers/net/ethernet/ibm/emac/phy.c#L522>
> > > 
> > > Ideally, we would like to reset only the ports which are registered in the DT.
> > 
> > Which you would get for free if you did extend qca8k to support the
> > 8327, because qca8k does implicitly tell the DSA layer to register a
> > dsa_slave_mii_bus which will probe and attach to per-port built-in PHYs
> > and that happens only for the ports enabled on your specific board.
> No, the Meraki and the modified WNDR4700 still refuse to work. Just >one<
> of the phys at address 0-4 need to be powered down to get the following
> error:
> [    4.425618] DSA: switch 0 0 parsed
> [    4.429034] DSA: tree 0 parsed
> [    4.435416] qca8k: probe of 4ef600c00.ethern:10 failed with error -5
> 
> I'll report back, what exactly is causing the error in this case.
Ok, the -EIO error is coming from this line:
<http://lxr.free-electrons.com/source/drivers/net/phy/phy_device.c?v=4.9#L496>

get_phy_id() gets called as part of mdiobus_register() in dsa_ds_apply()
<http://lxr.free-electrons.com/source/net/dsa/dsa2.c?v=4.9#L322> which is
called as part of the dsa_switch_register().

So, is there a good place to reset the switch/ports? Sure,
doing it in qca8k has the advantage that we know it's 5 ports.
However, I'm wondering, if the dsa or phy driver the right
place for this?

Note: behind the mdiobus_read() at 
<http://lxr.free-electrons.com/source/drivers/net/phy/phy_device.c?v=4.9#L494>
is emac's emac_mdio_read does returning -EREMOTEIO. I don't see that hiding
the error code behind a return 0xffff will help much:
---
For example I disabled just phy2:
| Generic PHY 4ef600c00.ethern:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=4ef600c00.ethern:00, irq=-1)
| Generic PHY dsa-0.0:01: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:01, irq=-1)
| qca8k 4ef600c00.ethern:10 lan2: no phy at 2
| qca8k 4ef600c00.ethern:10 lan2: failed to connect to phy2: -19
| emac 4ef600c00.ethernet eth0: error -19 setting up slave phy
| qca8k 4ef600c00.ethern:10: Failed to create slave 3: -19
| Generic PHY dsa-0.0:03: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:03, irq=-1)
| Generic PHY dsa-0.0:04: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:04, irq=-1)

And as a result: the switch "lost" the port.

When I tried to unbind the "faulty" switch in order to reset it
and rebind:

root@LEDE:/sys/bus/mdio_bus/drivers/qca8k# echo 4ef600c00.ethern:10 > unbind 
[ 2435.970104] Unable to handle kernel paging request for data at address 0x00000050
[ 2435.977580] Faulting instruction address: 0xc0359ae8
[ 2435.982532] Oops: Kernel access of bad area, sig: 11 [#1]
[ 2435.987901] WNDR4700 Platform
[ 2436.116263] CPU: 0 PID: 663 Comm: ash Not tainted 4.9.8 #0
[ 2436.121729] task: cf974000 task.stack: ce42a000
[ 2436.126238] NIP: c0359ae8 LR: c036a534 CTR: c029b338
[ 2436.131180] REGS: ce42bd10 TRAP: 0300   Not tainted  (4.9.8)
[ 2436.136811] MSR: 00029000 <CE,EE,ME>[ 2436.140260]   CR: 24002288  XER: 00000000
[ 2436.144251] DEAR: 00000050 ESR: 00000000 
GPR00: c036a534 ce42bdc0 cf974000 cfb02800 c03f5553 00000000 cf9e19e0 00000001 
GPR08: cf9e1b00 00000040 00000000 00000001 00780100 00000000 00000000 00000000 
GPR16: 00000000 00000000 10060000 1005cf90 b79a2495 1005cf70 b7a354c4 00000000 
GPR24: cfaf2508 00000001 cfaf24fc cfffbca8 cfaf228c 00000003 cfaf2400 cfb02800 
NIP [c0359ae8] dsa_slave_destroy+0x28/0x78
[ 2436.180491] LR [c036a534] dsa_dst_unapply.part.7+0xa4/0xb70
[ 2436.186033] Call Trace:
[ 2436.188475] [ce42bdc0] [c0359c48] dsa_port_is_cpu+0x1c/0x50 (unreliable)
[ 2436.195161] [ce42bdd0] [c036a534] dsa_dst_unapply.part.7+0xa4/0xb70
[ 2436.201409] [ce42be00] [c0359d60] dsa_unregister_switch+0x3c/0x60
[ 2436.207485] [ce42be20] [c0238a18] mdio_remove+0x24/0x40
[ 2436.212701] [ce42be30] [c01c6544] __device_release_driver+0xa4/0x13c
[ 2436.219034] [ce42be40] [c01c6604] device_release_driver+0x28/0x40
[ 2436.225107] [ce42be50] [c01c4c94] unbind_store+0x6c/0xac
[ 2436.230417] [ce42be70] [c00f8838] kernfs_fop_write+0x128/0x1ac
[ 2436.236247] [ce42be90] [c00a8ce4] __vfs_write+0x28/0x134
[ 2436.241542] [ce42bef0] [c00a9a00] vfs_write+0xd0/0x17c
[ 2436.246665] [ce42bf10] [c00aa758] SyS_write+0x4c/0xa4
[ 2436.251705] [ce42bf40] [c000a848] ret_from_syscall+0x0/0x3c
[ 2436.257266] --- interrupt: c01 at 0xb7a0edc0
[ 2436.257266]     LR = 0xb7a00df0
[ 2436.264631] Instruction dump:
[ 2436.267597] 38210030 4e800020 7c0802a6 9421fff0 bfc10008 7c7f1b78 90010014 892304e8 
[ 2436.275397] 814304e4 39290004 55292036 7d2a4a14 <83c90010> 4bf46641 807f04ec 2f830000 
[ 2436.283367] ---[ end trace e5787cb2a55a0fbd ]---
[ 2436.289726] 
[ 2437.291295] Kernel panic - not syncing: Fatal exception

The panic is caused by dsa_slave_create() not clearing the dangling
ds->ports[port].netdev pointer. The function dsa_user_port_unapply()
relies on it being NULL in order to skip unregistered entries.
---
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 09fc3e9462c1..0dae29eb95d6 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1460,6 +1460,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 	ret = dsa_slave_phy_setup(p, slave_dev);
 	if (ret) {
 		netdev_err(master, "error %d setting up slave phy\n", ret);
+		ds->ports[port].netdev = NULL;
 		unregister_netdev(slave_dev);
 		free_netdev(slave_dev);
 		return ret;
---

Thanks,
Christian

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

[Florian Fainelli]

On 02/14/2017 04:16 PM, Christian Lamparter wrote:
> On Tuesday, February 14, 2017 12:38:42 AM CET Christian Lamparter wrote:
>>>>>> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
>>>>>> --- a/drivers/net/ethernet/ibm/emac/core.c
>>>>>> +++ b/drivers/net/ethernet/ibm/emac/core.c
>>>>>> @@ -2420,6 +2421,179 @@ static int emac_read_uint_prop(struct device_node *np, const char *name,
>>>>>> [...] 
>>>>>> +static int emac_mii_bus_reset(struct mii_bus *bus)
>>>>>> +{
>>>>>> +	struct emac_instance *dev = netdev_priv(bus->priv);
>>>>>> +
>>>>>> +	emac_mii_reset_phy(&dev->phy);
>>>>>
>>>>> This seems wrong, emac_mii_reset_phy() does a BMCR software reset, which
>>>>> PHYLIB is already going to do (phy_init_hw), yet you do this here at the
>>>>> MDIO bus level towards a specify PHY, whereas this should be affecting
>>>>> the MDIO bus itself (and/or *all* PHY child devices for quirks).
>>>> Ah, this is a good point. The emac driver has a emac_reset() function
>>>> that does disable and enabled the phy clocks. That said, this is already
>>>> done by the emac driver during init too. So if I added it, the bus is
>>>> reset twice (since it doesn't hurt - I added it back).
>>>>
>>>> The emac_mii_phy_reset() was added because of the Meraki MX60(W).
>>>> This is because Cisco's bootloader disables the switch port 
>>>> (probably to prevent WAN<->LAN leakage during boot)
>>>>
>>>> [bootlog from the MX60(W)]
>>>> |Disabling port 0
>>>> |Disabling port 1
>>>> |Disabling port 2
>>>> |Disabling port 3
>>>> |ENET Speed is 1000 Mbps - FULL duplex connection (EMAC0)
>>>>
>>>> Without emac_mii_reset_phy(), the mdiobus_scan() function, which
>>>> is called by mdiobus_register will fail with -ENODEV.
>>>> | /plb/opb/ethernet@ef600c00: failed to attach dt phy (-19).
>>>> This is because get_phy_id() will "mostly read mostly Fs" and abort.
>>>
>>> Is the PHY just powered down by chance (BMCR_PWRDN set?) and resetting
>>> it implicitly clears the power down that seems to be what is going on.
>>
>> Yes, the PHY is just in the BMCR_PDOWN state. I can do the same
>> on the WNDR4700, by messing with u-boot:
>>
>> | => mii write 0 0 0x0800
>> | => mii dump
>> | 0.     (ffff)                 -- PHY control register --
>> |  (8000:8000) 0.15    =     1    reset
>> |  (4000:4000) 0.14    =     1    loopback
>> |  (2040:2040) 0. 6,13 =   b11    speed selection = ??? Mbps
>> |  (1000:1000) 0.12    =     1    A/N enable
>> |  (0800:0800) 0.11    =     1    power-down
>> |  (0400:0400) 0.10    =     1    isolate
>> |  (0200:0200) 0. 9    =     1    restart A/N
>> |  (0100:0100) 0. 8    =     1    duplex = full
>> |  (0080:0080) 0. 7    =     1    collision test enable
>> |  (003f:003f) 0. 5- 0 =    63    (reserved)
>>
>> On the Meraki, the port disabled by the bootloader. 
>> The reset is still needed.
>>
>>> Keep in mind that MDIO address 16 is the switch's pseudo PHY address
>>> here, so if you are telling PHYLIB to probe for that address and you
>>> don't get the expected MII_PHYSID1/2 value in return, that usually means
>>> that there was a PHY fixup registered to intercept these reads and make
>>> us return the switch's unique identifier. Reading from the switch's
>>> pseudo PHY at address 16 registers 2/3 (MII_PHYSID1/2) is not guaranteed
>>> to return the switch's unique identifer.
>>>
>>> With a MDIO device driver this won't happen because you will be probed
>>> by address, and you can read any switch register you want to and from
>>> there move on with the initialization.
>>>
>>>>
>>>>
>>>> With emac_mii_reset_phy() in place, it gets detected:
>>>> | switch0: Atheros AR8327 rev. 4 switch registered on emac_mdio
>>>>
>>>> Furthermore, this is probably not the only device which need it.
>>>> Currently, emac's own phy.c code does call emac_mii_reset_phy() 
>>>> as well as part of its probe procedure.
>>>> <http://lxr.free-electrons.com/source/drivers/net/ethernet/ibm/emac/phy.c#L522>
>>>>
>>>> Ideally, we would like to reset only the ports which are registered in the DT.
>>>
>>> Which you would get for free if you did extend qca8k to support the
>>> 8327, because qca8k does implicitly tell the DSA layer to register a
>>> dsa_slave_mii_bus which will probe and attach to per-port built-in PHYs
>>> and that happens only for the ports enabled on your specific board.
>> No, the Meraki and the modified WNDR4700 still refuse to work. Just >one<
>> of the phys at address 0-4 need to be powered down to get the following
>> error:
>> [    4.425618] DSA: switch 0 0 parsed
>> [    4.429034] DSA: tree 0 parsed
>> [    4.435416] qca8k: probe of 4ef600c00.ethern:10 failed with error -5
>>
>> I'll report back, what exactly is causing the error in this case.
> Ok, the -EIO error is coming from this line:
> <http://lxr.free-electrons.com/source/drivers/net/phy/phy_device.c?v=4.9#L496>
> 
> get_phy_id() gets called as part of mdiobus_register() in dsa_ds_apply()
> <http://lxr.free-electrons.com/source/net/dsa/dsa2.c?v=4.9#L322> which is
> called as part of the dsa_switch_register().
> 
> So, is there a good place to reset the switch/ports? Sure,
> doing it in qca8k has the advantage that we know it's 5 ports.
> However, I'm wondering, if the dsa or phy driver the right
> place for this?
> 
> Note: behind the mdiobus_read() at 
> <http://lxr.free-electrons.com/source/drivers/net/phy/phy_device.c?v=4.9#L494>
> is emac's emac_mdio_read does returning -EREMOTEIO. I don't see that hiding
> the error code behind a return 0xffff will help much:
> ---
> For example I disabled just phy2:
> | Generic PHY 4ef600c00.ethern:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=4ef600c00.ethern:00, irq=-1)
> | Generic PHY dsa-0.0:01: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:01, irq=-1)
> | qca8k 4ef600c00.ethern:10 lan2: no phy at 2
> | qca8k 4ef600c00.ethern:10 lan2: failed to connect to phy2: -19
> | emac 4ef600c00.ethernet eth0: error -19 setting up slave phy
> | qca8k 4ef600c00.ethern:10: Failed to create slave 3: -19
> | Generic PHY dsa-0.0:03: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:03, irq=-1)
> | Generic PHY dsa-0.0:04: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:04, irq=-1)
> 
> And as a result: the switch "lost" the port.
> 
> When I tried to unbind the "faulty" switch in order to reset it
> and rebind:
> 
> root@LEDE:/sys/bus/mdio_bus/drivers/qca8k# echo 4ef600c00.ethern:10 > unbind 
> [ 2435.970104] Unable to handle kernel paging request for data at address 0x00000050
> [ 2435.977580] Faulting instruction address: 0xc0359ae8
> [ 2435.982532] Oops: Kernel access of bad area, sig: 11 [#1]
> [ 2435.987901] WNDR4700 Platform
> [ 2436.116263] CPU: 0 PID: 663 Comm: ash Not tainted 4.9.8 #0
> [ 2436.121729] task: cf974000 task.stack: ce42a000
> [ 2436.126238] NIP: c0359ae8 LR: c036a534 CTR: c029b338
> [ 2436.131180] REGS: ce42bd10 TRAP: 0300   Not tainted  (4.9.8)
> [ 2436.136811] MSR: 00029000 <CE,EE,ME>[ 2436.140260]   CR: 24002288  XER: 00000000
> [ 2436.144251] DEAR: 00000050 ESR: 00000000 
> GPR00: c036a534 ce42bdc0 cf974000 cfb02800 c03f5553 00000000 cf9e19e0 00000001 
> GPR08: cf9e1b00 00000040 00000000 00000001 00780100 00000000 00000000 00000000 
> GPR16: 00000000 00000000 10060000 1005cf90 b79a2495 1005cf70 b7a354c4 00000000 
> GPR24: cfaf2508 00000001 cfaf24fc cfffbca8 cfaf228c 00000003 cfaf2400 cfb02800 
> NIP [c0359ae8] dsa_slave_destroy+0x28/0x78
> [ 2436.180491] LR [c036a534] dsa_dst_unapply.part.7+0xa4/0xb70
> [ 2436.186033] Call Trace:
> [ 2436.188475] [ce42bdc0] [c0359c48] dsa_port_is_cpu+0x1c/0x50 (unreliable)
> [ 2436.195161] [ce42bdd0] [c036a534] dsa_dst_unapply.part.7+0xa4/0xb70
> [ 2436.201409] [ce42be00] [c0359d60] dsa_unregister_switch+0x3c/0x60
> [ 2436.207485] [ce42be20] [c0238a18] mdio_remove+0x24/0x40
> [ 2436.212701] [ce42be30] [c01c6544] __device_release_driver+0xa4/0x13c
> [ 2436.219034] [ce42be40] [c01c6604] device_release_driver+0x28/0x40
> [ 2436.225107] [ce42be50] [c01c4c94] unbind_store+0x6c/0xac
> [ 2436.230417] [ce42be70] [c00f8838] kernfs_fop_write+0x128/0x1ac
> [ 2436.236247] [ce42be90] [c00a8ce4] __vfs_write+0x28/0x134
> [ 2436.241542] [ce42bef0] [c00a9a00] vfs_write+0xd0/0x17c
> [ 2436.246665] [ce42bf10] [c00aa758] SyS_write+0x4c/0xa4
> [ 2436.251705] [ce42bf40] [c000a848] ret_from_syscall+0x0/0x3c
> [ 2436.257266] --- interrupt: c01 at 0xb7a0edc0
> [ 2436.257266]     LR = 0xb7a00df0
> [ 2436.264631] Instruction dump:
> [ 2436.267597] 38210030 4e800020 7c0802a6 9421fff0 bfc10008 7c7f1b78 90010014 892304e8 
> [ 2436.275397] 814304e4 39290004 55292036 7d2a4a14 <83c90010> 4bf46641 807f04ec 2f830000 
> [ 2436.283367] ---[ end trace e5787cb2a55a0fbd ]---
> [ 2436.289726] 
> [ 2437.291295] Kernel panic - not syncing: Fatal exception
> 
> The panic is caused by dsa_slave_create() not clearing the dangling
> ds->ports[port].netdev pointer. The function dsa_user_port_unapply()
> relies on it being NULL in order to skip unregistered entries.

Fixed that just recently:

https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=382e1eea2d983cd2343482c6a638f497bb44a636

and I am currently in the process of making the bind/unbind working
(without having the PHY detached from the network device), but that's a
stretch, and requires a ton of mucking around with driver references,
current state machine state, target state machine state etc...

Candidate patches for that are located here if you want:

https://github.com/ffainelli/linux/commit/e7e99a9ab998952b9f18e6e396779bb58626e9a5
https://github.com/ffainelli/linux/commit/e005be050cd876189f39c5a89fffa70ee8d6c512

> ---
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 09fc3e9462c1..0dae29eb95d6 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1460,6 +1460,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
>  	ret = dsa_slave_phy_setup(p, slave_dev);
>  	if (ret) {
>  		netdev_err(master, "error %d setting up slave phy\n", ret);
> +		ds->ports[port].netdev = NULL;
>  		unregister_netdev(slave_dev);
>  		free_netdev(slave_dev);
>  		return ret;
> ---
> 
> Thanks,
> Christian
> 


-- 
Florian

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

[Andrew Lunn]

> > > Is the PHY just powered down by chance (BMCR_PWRDN set?) and resetting
> > > it implicitly clears the power down that seems to be what is going on.
> > 
> > Yes, the PHY is just in the BMCR_PDOWN state. I can do the same
> > on the WNDR4700, by messing with u-boot:

Hi Christian

What happens if you list the PHYs in the device tree, with their PHY
ID. That should avoid it looking for the ID and getting 0xffff
back. It should just probe the correct PHY driver. If the first thing
the drivers probe function does it reset the power down bit, it might
work.

	Andrew

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

[Christian Lamparter]

On Wednesday, February 15, 2017 3:23:08 PM CET Andrew Lunn wrote:
> > > > Is the PHY just powered down by chance (BMCR_PWRDN set?) and resetting
> > > > it implicitly clears the power down that seems to be what is going on.
> > > 
> > > Yes, the PHY is just in the BMCR_PDOWN state. I can do the same
> > > on the WNDR4700, by messing with u-boot:
> 
> Hi Christian
> 
> What happens if you list the PHYs in the device tree, with their PHY
> ID. That should avoid it looking for the ID and getting 0xffff
> back. It should just probe the correct PHY driver. If the first thing
> the drivers probe function does it reset the power down bit, it might
> work.

I just tested it. And it didn't work (Same result/error).

It fails because the PHYs on the dsa slave-bus are not detected via the
device tree method. See <http://lxr.free-electrons.com/source/net/dsa/dsa2.c#L322>

|315         if (!ds->slave_mii_bus && ds->ops->phy_read) {
|316                 ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
|317                 if (!ds->slave_mii_bus)
|318                         return -ENOMEM;
|319 
|320                 dsa_slave_mii_bus_init(ds);
|321 
|322                 err = mdiobus_register(ds->slave_mii_bus);
|323                 if (err < 0)
|324                         return err;
|325         }

You see that dsa2 just calls mdiobus_register() which will do the
PHY discovery with mdiobus_scan() 
<http://lxr.free-electrons.com/source/drivers/net/phy/mdio_bus.c#L335>
which relys on the values from MII_PHYSID1 and MII_PHYSID2.

In order to get it work, this mdiobus_register would need to be
replaced with of_mdiobus_register().

---
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 737be6470c7f..5a90ec81040f 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -18,6 +18,7 @@
 #include <net/dsa.h>
 #include <linux/of.h>
 #include <linux/of_net.h>
+#include <linux/of_mdio.h>
 #include "dsa_priv.h"
 
 static LIST_HEAD(dsa_switch_trees);
@@ -288,7 +289,8 @@ static void dsa_user_port_unapply(struct dsa_port *port, u32 index,
 	}
 }
 
-static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
+static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds,
+			struct device_node *ports)
 {
 	struct dsa_port *port;
 	u32 index;
@@ -322,7 +324,10 @@ static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
 
 		dsa_slave_mii_bus_init(ds);
 
-		err = mdiobus_register(ds->slave_mii_bus);
+		if (!ports)
+			err = mdiobus_register(ds->slave_mii_bus);
+		else
+			err = of_mdiobus_register(ds->slave_mii_bus, ports);
 		if (err < 0)
 			return err;
 	}
@@ -383,7 +388,8 @@ static void dsa_ds_unapply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
 	dsa_switch_unregister_notifier(ds);
 }
 
-static int dsa_dst_apply(struct dsa_switch_tree *dst)
+static int dsa_dst_apply(struct dsa_switch_tree *dst,
+			 struct device_node *ports)
 {
 	struct dsa_switch *ds;
 	u32 index;
@@ -394,7 +400,7 @@ static int dsa_dst_apply(struct dsa_switch_tree *dst)
 		if (!ds)
 			continue;
 
-		err = dsa_ds_apply(dst, ds);
+		err = dsa_ds_apply(dst, ds, ports);
 		if (err)
 			return err;
 	}
@@ -649,7 +655,7 @@ static int _dsa_register_switch(struct dsa_switch *ds, struct device *dev)
 	struct dsa_chip_data *pdata = dev->platform_data;
 	struct device_node *np = dev->of_node;
 	struct dsa_switch_tree *dst;
-	struct device_node *ports;
+	struct device_node *ports = NULL;
 	u32 tree, index;
 	int i, err;
 
@@ -722,7 +728,7 @@ static int _dsa_register_switch(struct dsa_switch *ds, struct device *dev)
 		goto out_del_dst;
 	}
 
-	err = dsa_dst_apply(dst);
+	err = dsa_dst_apply(dst, ports);
 	if (err) {
 		dsa_dst_unapply(dst);
 		goto out_del_dst;
---

With this patch applied, the device discovery works as intended:


| [    4.514106] Generic PHY 4ef600c00.ethern:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=4ef600c00.ethern:00, irq=-1)
|[    4.525975] Generic PHY dsa-0.0:01: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:01, irq=-1)
|[    4.536269] Generic PHY dsa-0.0:02: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:02, irq=-1)
|[    4.546562] Generic PHY dsa-0.0:03: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:03, irq=-1)
|[    4.556887] Generic PHY dsa-0.0:04: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:04, irq=-1)

>From what I can tell, the PHY works. Although it will ping-pong for a bit:
|[  170.463823] qca8k 4ef600c00.ethern:10 lan1: Link is Down
|[  172.511860] qca8k 4ef600c00.ethern:10 lan1: Link is Up - 1Gbps/Full - flow control rx/tx
|[  174.559823] qca8k 4ef600c00.ethern:10 lan1: Link is Down
|[  175.583853] qca8k 4ef600c00.ethern:10 lan1: Link is Up - 1Gbps/Full - flow control rx/tx
|[  179.679816] qca8k 4ef600c00.ethern:10 lan1: Link is Down
|[  182.751846] qca8k 4ef600c00.ethern:10 lan1: Link is Up - 1Gbps/Full - flow control rx/tx
|[  186.847834] qca8k 4ef600c00.ethern:10 lan1: Link is Down
|[  188.895847] qca8k 4ef600c00.ethern:10 lan1: Link is Up - 1Gbps/Full - flow control rx/tx
(But it will stay up).


As for the patch. It's a bit cheesy because it forces you to specify the
compatible property on the "dsa_slave_bus":

| mdio {
|	#address-cells = <1>;
|	#size-cells = <0>;
|  ...
|  phy_port2: phy@1 { /* this phy is PDOWNed */
|		compatible = "ethernet-phy-id004d.d034", "ethernet-phy-ieee802.3-c22";
|		^^ this is ignored!
|		reg = <1>;
|	};
|  ...
|	switch0@16 {
|		compatible = "qca,qca8337";
|		#address-cells = <1>;
|		#size-cells = <0>;
|		reg = <16>;
|      ports {
|			#address-cells = <1>;
|			#size-cells = <0>;
|				...
|				port@2 {
|					compatible = "ethernet-phy-id004d.d034", "ethernet-phy-ieee802.3-c22";
|					// ^^ This compatible string is parsed. However of_mdiobus_register()
|					// should look up the ignored compatible string from the phy_handle
|					// property down below.
|					phy-handle = <&phy_port2>;
|					// ^^ since this is the real device on the mdiobus. And this is going
|					// to be tricky since the switch itself it there as well.
|					reg = <1>;
|					label = "lan3";
|				};
|		...
|		};

So, anyone: What would be a good solution for this?

Thanks,
Christian

Re: [RFC 1/2] dt: emac: document device-tree based phy discovery and setup

[Christian Lamparter]

On Sunday, February 5, 2017 2:33:44 PM CET Florian Fainelli wrote:
> Le 02/05/17 à 14:25, Christian Lamparter a écrit :
> > This patch adds documentation for a new "phy-handler" property
> > and "mdio" sub-node. These allows the enumeration of PHYs which
> > are supported by the phy library under drivers/net/phy.
> > 
> > The EMAC ethernet controller in IBM and AMCC 4xx chips is
> > currently stuck with a few privately defined phy
> > implementations. It has no support for PHYs which
> > are supported by the generic phylib.
> > 
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > ---
> >  .../devicetree/bindings/powerpc/4xx/emac.txt       | 60 +++++++++++++++++++++-
> >  1 file changed, 58 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/powerpc/4xx/emac.txt b/Documentation/devicetree/bindings/powerpc/4xx/emac.txt
> > index 712baf6c3e24..0572d053c35a 100644
> > --- a/Documentation/devicetree/bindings/powerpc/4xx/emac.txt
> > +++ b/Documentation/devicetree/bindings/powerpc/4xx/emac.txt
> > @@ -71,6 +71,8 @@
> >  			  For Axon it can be absent, though my current driver
> >  			  doesn't handle phy-address yet so for now, keep
> >  			  0x00ffffff in it.
> > +    - phy-handle	: See net/ethernet.txt file; used to describe
> > +			  configurations where a external PHY is used.
> >      - rx-fifo-size-gige : 1 cell, Rx fifo size in bytes for 1000 Mb/sec
> >  			  operations (if absent the value is the same as
> >  			  rx-fifo-size).  For Axon, either absent or 2048.
> > @@ -82,7 +84,18 @@
> >      - tah-channel       : 1 cell, optional. If appropriate, channel used on the
> >  			  TAH engine.
> >  
> > -    Example:
> > +    - mdio subnode	: When the EMAC has a phy connected to its local
> > +			  mdio, which us supported by the kernel's network
> > +			  PHY library in drivers/net/phy, there must be device
> > +			  tree subnode with the following required properties:
> > +				- #address-cells: Must be <1>.
> > +				- #size-cells: Must be <0>.
> > +
> > +			  For each phy on the mdio bus, there must be a node
> > +			  with the following fields:
> > +				- reg: phy id used to communicate to phy.
> > +				- device_type: Must be "ethernet-phy".
> 
> Just provide a reference to
> Documentation/devicetree/bindings/net/phy.txt and
> Documentation/devicetree/bindings/net/ethernet.txt here. device_type is
> not required.

Yes, I added a reference there.
> 
> > +    Examples:
> >  
> >  	EMAC0: ethernet@40000800 {
> >  		device_type = "network";
> > @@ -104,6 +117,50 @@
> >  		zmii-channel = <0>;
> >  	};
> >  
> > +	EMAC1: ethernet@ef600c00 {
> > +		device_type = "network";
> > +		compatible = "ibm,emac-apm821xx", "ibm,emac4sync";
> > +		interrupt-parent = <&EMAC1>;
> > +		interrupts = <0 1>;
> > +		#interrupt-cells = <1>;
> > +		#address-cells = <0>;
> > +		#size-cells = <0>;
> > +		interrupt-map = <0 &UIC2 0x10 IRQ_TYPE_LEVEL_HIGH /* Status */
> > +				 1 &UIC2 0x14 IRQ_TYPE_LEVEL_HIGH /* Wake */>;
> > +		reg = <0xef600c00 0x000000c4>;
> > +		local-mac-address = [000000000000]; /* Filled in by U-Boot */
> > +		mal-device = <&MAL0>;
> > +		mal-tx-channel = <0>;
> > +		mal-rx-channel = <0>;
> > +		cell-index = <0>;
> > +		max-frame-size = <9000>;
> > +		rx-fifo-size = <16384>;
> > +		tx-fifo-size = <2048>;
> > +		fifo-entry-size = <10>;
> > +		phy-mode = "rgmii";
> > +		phy-map = <0x00000000>;
> 
> If you have a proper mdio subnode, this property becomes irrelevant and
> should be unused.
This is emac.c doing. It defaults to 0xffff... if the property is absent.
<http://lxr.free-electrons.com/source/drivers/net/ethernet/ibm/emac/core.c#L2578>
> > +		phy-handle = <&phy0>;
> > +		rgmii-device = <&RGMII0>;
> > +		rgmii-channel = <0>;
> > +		tah-device = <&TAH0>;
> > +		tah-channel = <0>;
> > +		has-inverted-stacr-oc;
> > +		has-new-stacr-staopc;
> > +
> > +	        mdio {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			phy0: ethernet-phy@0 {
> > +				device_type = "ethernet-phy";
> > +				reg = <0>;
> > +
> > +				qca,ar8327-initvals = <
> > +					0x0010 0x40000000>;
> > +		};
> > +	};
> > +
> > +
> >        ii) McMAL node
> >  
> >      Required properties:
> > @@ -145,4 +202,3 @@
> >      - revision           : as provided by the RGMII new version register if
> >  			   available.
> >  			   For Axon: 0x0000012a
> > -
> > 
> 
> 
>