[PATCHv3 0/4] Add DT support for fixed PHYs

[PATCHv3 0/4] Add DT support for fixed PHYs

[Thomas Petazzoni]

Hello,

Here is a third version of the patch set that adds a Device Tree
binding and the related code to support fixed PHYs.

Since the second version, the changes have been:

 * Rebased on top of v3.14-rc1, and re-tested on hardware.

 * Removed the RFC tag, since there seems to be some real interest in
   this feature, and the code has gone through several iterations
   already.

 * The error handling in fixed_phy_register() has been fixed.

Since the first version, the changes have been:

 * Instead of using a 'fixed-link' property inside the Ethernet device
   DT node, with a fairly cryptic succession of integer values, we now
   use a PHY subnode under the Ethernet device DT node, with explicit
   properties to configure the duplex, speed, pause and other PHY
   properties.

 * The PHY address is automatically allocated by the kernel and no
   longer visible in the Device Tree binding.

 * The PHY device is created directly when the network driver calls
   of_phy_connect_fixed_link(), and associated to the PHY DT node,
   which allows the existing of_phy_connect() function to work,
   without the need to use the deprecated of_phy_connect_fixed_link().

Posts of previous versions:

  RFCv1: http://www.spinics.net/lists/netdev/msg243253.html
  RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-September/196919.html

Thanks,

Thomas

Thomas Petazzoni (4):
  net: phy: decouple PHY id and PHY address in fixed PHY driver
  net: phy: extend fixed driver with fixed_phy_register()
  of: provide a binding for fixed link PHYs
  net: mvneta: add support for fixed links

 .../devicetree/bindings/net/fixed-link.txt         | 34 +++++++++
 .../bindings/net/marvell-armada-370-neta.txt       |  4 +-
 drivers/net/ethernet/marvell/mvneta.c              | 10 +--
 drivers/net/phy/fixed.c                            | 81 +++++++++++++++++++---
 drivers/of/of_mdio.c                               | 24 +++++++
 include/linux/of_mdio.h                            | 15 ++++
 include/linux/phy_fixed.h                          | 11 +++
 7 files changed, 163 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt

-- 
1.8.3.2

[PATCHv3 1/4] net: phy: decouple PHY id and PHY address in fixed PHY driver

[Thomas Petazzoni]

Until now, the fixed_phy_add() function was taking as argument
'phy_id', which was used both as the PHY address on the fake fixed
MDIO bus, and as the PHY id, as available in the MII_PHYSID1 and
MII_PHYSID2 registers. However, those two informations are completely
unrelated.

This patch decouples them. The PHY id of fixed PHYs is hardcoded to be
0xdeadbeef. Ideally, a really reserved value would be nicer, but there
doesn't seem to be an easy of making sure a dummy value can be
assigned to the Linux kernel for such usage.

The PHY address remains passed by the caller of phy_fixed_add().

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/net/phy/fixed.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c
index ba55adf..0f02403 100644
--- a/drivers/net/phy/fixed.c
+++ b/drivers/net/phy/fixed.c
@@ -31,7 +31,7 @@ struct fixed_mdio_bus {
 };
 
 struct fixed_phy {
-	int id;
+	int addr;
 	u16 regs[MII_REGS_NUM];
 	struct phy_device *phydev;
 	struct fixed_phy_status status;
@@ -104,8 +104,8 @@ static int fixed_phy_update_regs(struct fixed_phy *fp)
 	if (fp->status.asym_pause)
 		lpa |= LPA_PAUSE_ASYM;
 
-	fp->regs[MII_PHYSID1] = fp->id >> 16;
-	fp->regs[MII_PHYSID2] = fp->id;
+	fp->regs[MII_PHYSID1] = 0xdead;
+	fp->regs[MII_PHYSID2] = 0xbeef;
 
 	fp->regs[MII_BMSR] = bmsr;
 	fp->regs[MII_BMCR] = bmcr;
@@ -115,7 +115,7 @@ static int fixed_phy_update_regs(struct fixed_phy *fp)
 	return 0;
 }
 
-static int fixed_mdio_read(struct mii_bus *bus, int phy_id, int reg_num)
+static int fixed_mdio_read(struct mii_bus *bus, int phy_addr, int reg_num)
 {
 	struct fixed_mdio_bus *fmb = bus->priv;
 	struct fixed_phy *fp;
@@ -124,7 +124,7 @@ static int fixed_mdio_read(struct mii_bus *bus, int phy_id, int reg_num)
 		return -1;
 
 	list_for_each_entry(fp, &fmb->phys, node) {
-		if (fp->id == phy_id) {
+		if (fp->addr == phy_addr) {
 			/* Issue callback if user registered it. */
 			if (fp->link_update) {
 				fp->link_update(fp->phydev->attached_dev,
@@ -138,7 +138,7 @@ static int fixed_mdio_read(struct mii_bus *bus, int phy_id, int reg_num)
 	return 0xFFFF;
 }
 
-static int fixed_mdio_write(struct mii_bus *bus, int phy_id, int reg_num,
+static int fixed_mdio_write(struct mii_bus *bus, int phy_addr, int reg_num,
 			    u16 val)
 {
 	return 0;
@@ -160,7 +160,7 @@ int fixed_phy_set_link_update(struct phy_device *phydev,
 		return -EINVAL;
 
 	list_for_each_entry(fp, &fmb->phys, node) {
-		if (fp->id == phydev->phy_id) {
+		if (fp->addr == phydev->addr) {
 			fp->link_update = link_update;
 			fp->phydev = phydev;
 			return 0;
@@ -171,7 +171,7 @@ int fixed_phy_set_link_update(struct phy_device *phydev,
 }
 EXPORT_SYMBOL_GPL(fixed_phy_set_link_update);
 
-int fixed_phy_add(unsigned int irq, int phy_id,
+int fixed_phy_add(unsigned int irq, int phy_addr,
 		  struct fixed_phy_status *status)
 {
 	int ret;
@@ -184,9 +184,9 @@ int fixed_phy_add(unsigned int irq, int phy_id,
 
 	memset(fp->regs, 0xFF,  sizeof(fp->regs[0]) * MII_REGS_NUM);
 
-	fmb->irqs[phy_id] = irq;
+	fmb->irqs[phy_addr] = irq;
 
-	fp->id = phy_id;
+	fp->addr = phy_addr;
 	fp->status = *status;
 
 	ret = fixed_phy_update_regs(fp);
-- 
1.8.3.2

[PATCHv3 2/4] net: phy: extend fixed driver with fixed_phy_register()

[Thomas Petazzoni]

The existing fixed_phy_add() function has several drawbacks that
prevents it from being used as is for OF-based declaration of fixed
PHYs:

 * The address of the PHY on the fake bus needs to be passed, while a
   dynamic allocation is desired.

 * Since the phy_device instantiation is post-poned until the next
   mdiobus scan, there is no way to associate the fixed PHY with its
   OF node, which later prevents of_phy_connect() from finding this
   fixed PHY from a given OF node.

To solve this, this commit introduces fixed_phy_register(), which will
allocate an available PHY address, add the PHY using fixed_phy_add()
and instantiate the phy_device structure associated with the provided
OF node.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/net/phy/fixed.c   | 61 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/phy_fixed.h | 11 +++++++++
 2 files changed, 72 insertions(+)

diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c
index 0f02403..82095cc 100644
--- a/drivers/net/phy/fixed.c
+++ b/drivers/net/phy/fixed.c
@@ -21,6 +21,7 @@
 #include <linux/phy_fixed.h>
 #include <linux/err.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 
 #define MII_REGS_NUM 29
 
@@ -203,6 +204,66 @@ err_regs:
 }
 EXPORT_SYMBOL_GPL(fixed_phy_add);
 
+void fixed_phy_del(int phy_addr)
+{
+	struct fixed_mdio_bus *fmb = &platform_fmb;
+	struct fixed_phy *fp, *tmp;
+
+	list_for_each_entry_safe(fp, tmp, &fmb->phys, node) {
+		if (fp->addr == phy_addr) {
+			list_del(&fp->node);
+			kfree(fp);
+			return;
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(fixed_phy_del);
+
+static int phy_fixed_addr;
+static DEFINE_SPINLOCK(phy_fixed_addr_lock);
+
+int fixed_phy_register(unsigned int irq,
+		       struct fixed_phy_status *status,
+		       struct device_node *np)
+{
+	struct fixed_mdio_bus *fmb = &platform_fmb;
+	struct phy_device *phy;
+	int phy_addr;
+	int ret;
+
+	/* Get the next available PHY address, up to PHY_MAX_ADDR */
+	spin_lock(&phy_fixed_addr_lock);
+	if (phy_fixed_addr == PHY_MAX_ADDR) {
+		spin_unlock(&phy_fixed_addr_lock);
+		return -ENOSPC;
+	}
+	phy_addr = phy_fixed_addr++;
+	spin_unlock(&phy_fixed_addr_lock);
+
+	ret = fixed_phy_add(PHY_POLL, phy_addr, status);
+	if (ret < 0)
+		return ret;
+
+	phy = get_phy_device(fmb->mii_bus, phy_addr, false);
+	if (!phy || IS_ERR(phy)) {
+		fixed_phy_del(phy_addr);
+		return -EINVAL;
+	}
+
+	of_node_get(np);
+	phy->dev.of_node = np;
+
+	ret = phy_device_register(phy);
+	if (ret) {
+		phy_device_free(phy);
+		of_node_put(np);
+		fixed_phy_del(phy_addr);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int __init fixed_mdio_bus_init(void)
 {
 	struct fixed_mdio_bus *fmb = &platform_fmb;
diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
index 509d8f5..902e8a1 100644
--- a/include/linux/phy_fixed.h
+++ b/include/linux/phy_fixed.h
@@ -9,15 +9,26 @@ struct fixed_phy_status {
 	int asym_pause;
 };
 
+struct device_node;
+
 #ifdef CONFIG_FIXED_PHY
 extern int fixed_phy_add(unsigned int irq, int phy_id,
 			 struct fixed_phy_status *status);
+extern int fixed_phy_register(unsigned int irq,
+			      struct fixed_phy_status *status,
+			      struct device_node *np);
 #else
 static inline int fixed_phy_add(unsigned int irq, int phy_id,
 				struct fixed_phy_status *status)
 {
 	return -ENODEV;
 }
+extern int fixed_phy_register(unsigned int irq,
+			      struct fixed_phy_status *status,
+			      struct device_node *np)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_FIXED_PHY */
 
 /*
-- 
1.8.3.2

[PATCHv3 3/4] of: provide a binding for fixed link PHYs

[Thomas Petazzoni]

Some Ethernet MACs have a "fixed link", and are not connected to a
normal MDIO-managed PHY device. For those situations, a Device Tree
binding allows to describe a "fixed link" using a special PHY node.

This patch adds:

 * A documentation for the fixed PHY Device Tree binding.

 * An of_phy_is_fixed_link() function that an Ethernet driver can call
   on its PHY phandle to find out whether it's a fixed link PHY or
   not. It should typically be used to know if
   of_phy_register_fixed_link() should be called.

 * An of_phy_register_fixed_link() function that instantiates the
   fixed PHY into the PHY subsystem, so that when the driver calls
   of_phy_connect(), the PHY device associated to the OF node will be
   found.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 .../devicetree/bindings/net/fixed-link.txt         | 34 ++++++++++++++++++++++
 drivers/of/of_mdio.c                               | 24 +++++++++++++++
 include/linux/of_mdio.h                            | 15 ++++++++++
 3 files changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt

diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt
new file mode 100644
index 0000000..9f2a1a50
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/fixed-link.txt
@@ -0,0 +1,34 @@
+Fixed link Device Tree binding
+------------------------------
+
+Some Ethernet MACs have a "fixed link", and are not connected to a
+normal MDIO-managed PHY device. For those situations, a Device Tree
+binding allows to describe a "fixed link".
+
+Such a fixed link situation is described by creating a PHY node as a
+sub-node of an Ethernet device, with the following properties:
+
+* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a
+  fixed link PHY.
+* 'speed' (integer, mandatory), to indicate the link speed. Accepted
+  values are 10, 100 and 1000
+* 'full-duplex' (boolean, optional), to indicate that full duplex is
+  used. When absent, half duplex is assumed.
+* 'pause' (boolean, optional), to indicate that pause should be
+  enabled.
+* 'asym-pause' (boolean, optional), to indicate that asym_pause should
+  be enabled.
+
+Example:
+
+ethernet@0 {
+	...
+	phy = <&phy0>;
+	phy0: phy@0 {
+	      fixed-link;
+	      speed = <1000>;
+	      full-duplex;
+	};
+	...
+};
+
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 875b7b6..c645fb8 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -14,6 +14,7 @@
 #include <linux/netdevice.h>
 #include <linux/err.h>
 #include <linux/phy.h>
+#include <linux/phy_fixed.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/of_mdio.h>
@@ -274,3 +275,26 @@ struct phy_device *of_phy_attach(struct net_device *dev,
 	return phy_attach_direct(dev, phy, flags, iface) ? NULL : phy;
 }
 EXPORT_SYMBOL(of_phy_attach);
+
+#if defined(CONFIG_FIXED_PHY)
+bool of_phy_is_fixed_link(struct device_node *np)
+{
+	return of_property_read_bool(np, "fixed-link");
+}
+EXPORT_SYMBOL(of_phy_is_fixed_link);
+
+int of_phy_register_fixed_link(struct device_node *np)
+{
+	struct fixed_phy_status status = {};
+
+	status.link = 1;
+	status.duplex = of_property_read_bool(np, "full-duplex");
+	if (of_property_read_u32(np, "speed", &status.speed))
+		return -EINVAL;
+	status.pause = of_property_read_bool(np, "pause");
+	status.asym_pause = of_property_read_bool(np, "asym-pause");
+
+	return fixed_phy_register(PHY_POLL, &status, np);
+}
+EXPORT_SYMBOL(of_phy_register_fixed_link);
+#endif
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 6fe8464..77a6e32 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -67,4 +67,19 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np)
 }
 #endif /* CONFIG_OF */
 
+#if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY)
+extern int of_phy_register_fixed_link(struct device_node *np);
+extern bool of_phy_is_fixed_link(struct device_node *np);
+#else
+static inline int of_phy_register_fixed_link(struct device_node *np)
+{
+	return -ENOSYS;
+}
+static inline bool of_phy_is_fixed_link(struct device_node *np)
+{
+	return false;
+}
+#endif
+
+
 #endif /* __LINUX_OF_MDIO_H */
-- 
1.8.3.2

[PATCHv3 4/4] net: mvneta: add support for fixed links

[Thomas Petazzoni]

Following the introduction of of_phy_register_fixed_link(), this patch
introduces fixed link support in the mvneta driver, for Marvell Armada
370/XP SOCs.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 .../devicetree/bindings/net/marvell-armada-370-neta.txt        |  4 ++--
 drivers/net/ethernet/marvell/mvneta.c                          | 10 ++++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
index 859a6fa..4d07d4e 100644
--- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
+++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
@@ -4,8 +4,8 @@ Required properties:
 - compatible: should be "marvell,armada-370-neta".
 - reg: address and length of the register set for the device.
 - interrupts: interrupt for the device
-- phy: A phandle to a phy node defining the PHY address (as the reg
-  property, a single integer).
+- phy: A phandle to the PHY node describing the PHY to which this
+  Ethernet controller is connected to.
 - phy-mode: The interface between the SoC and the PHY (a string that
   of_get_phy_mode() can understand)
 - clocks: a pointer to the reference clock for this device.
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index f418f4f..053a650 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -2805,10 +2805,12 @@ static int mvneta_probe(struct platform_device *pdev)
 	}
 
 	phy_node = of_parse_phandle(dn, "phy", 0);
-	if (!phy_node) {
-		dev_err(&pdev->dev, "no associated PHY\n");
-		err = -ENODEV;
-		goto err_free_irq;
+	if (of_phy_is_fixed_link(phy_node)) {
+		err = of_phy_register_fixed_link(phy_node);
+		if (err < 0) {
+			dev_err(&pdev->dev, "cannot register fixed PHY\n");
+			goto err_free_irq;
+		}
 	}
 
 	phy_mode = of_get_phy_mode(dn);
-- 
1.8.3.2

Re: [PATCHv3 4/4] net: mvneta: add support for fixed links

[Thomas Petazzoni]

Hello all,

On Tue,  4 Mar 2014 11:58:24 +0100, Thomas Petazzoni wrote:

>  	phy_node = of_parse_phandle(dn, "phy", 0);
> -	if (!phy_node) {
> -		dev_err(&pdev->dev, "no associated PHY\n");
> -		err = -ENODEV;
> -		goto err_free_irq;
> +	if (of_phy_is_fixed_link(phy_node)) {
> +		err = of_phy_register_fixed_link(phy_node);
> +		if (err < 0) {
> +			dev_err(&pdev->dev, "cannot register fixed PHY\n");
> +			goto err_free_irq;
> +		}
>  	}
>  
>  	phy_mode = of_get_phy_mode(dn);

As noted by Sebastian Hesselbarth (talking with me on IRC), it would be
nicer if network driver didn't had to be modified to ensure that the
fixed PHY the network device is connected to gets registered into the
kernel.

So ideally, the fixed MDIO bus implemented in drivers/net/phy/fixed.c
should scan the Device Tree, and register at boot time the fixed PHYs.
The only problem is how to recognize in the DT which nodes are fixed
PHYs. Looking for the "fixed-link" property is very fragile as other
completely unrelated nodes may use this property for other purposes.
Using a compatible string? A device_type property? Or should we have a
fragment of the DT for the fixed MDIO bus itself?

I've done a quick prototype that is based on using the "fixed-eth-phy"
compatible string as the key to find fixed PHYs in the DT, just to see
how it would work. It works well, and allows to get rid of changes in
the network driver completely. See below for the prototype-level patch
(which applies on top of the proposed patch series, for now).

Suggestions?

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 053a650..8a68a07 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -2805,14 +2805,6 @@ static int mvneta_probe(struct platform_device *pdev)
 	}
 
 	phy_node = of_parse_phandle(dn, "phy", 0);
-	if (of_phy_is_fixed_link(phy_node)) {
-		err = of_phy_register_fixed_link(phy_node);
-		if (err < 0) {
-			dev_err(&pdev->dev, "cannot register fixed PHY\n");
-			goto err_free_irq;
-		}
-	}
-
 	phy_mode = of_get_phy_mode(dn);
 	if (phy_mode < 0) {
 		dev_err(&pdev->dev, "incorrect phy-mode\n");
diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c
index 82095cc..45a7d5a 100644
--- a/drivers/net/phy/fixed.c
+++ b/drivers/net/phy/fixed.c
@@ -22,6 +22,7 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_mdio.h>
 
 #define MII_REGS_NUM 29
 
@@ -293,6 +294,8 @@ static int __init fixed_mdio_bus_init(void)
 	if (ret)
 		goto err_mdiobus_alloc;
 
+	of_phy_register_fixed_phys();
+
 	return 0;
 
 err_mdiobus_alloc:
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index c645fb8..4e75a70 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -277,16 +277,12 @@ struct phy_device *of_phy_attach(struct net_device *dev,
 EXPORT_SYMBOL(of_phy_attach);
 
 #if defined(CONFIG_FIXED_PHY)
-bool of_phy_is_fixed_link(struct device_node *np)
-{
-	return of_property_read_bool(np, "fixed-link");
-}
-EXPORT_SYMBOL(of_phy_is_fixed_link);
-
-int of_phy_register_fixed_link(struct device_node *np)
+static int of_phy_register_fixed_link(struct device_node *np)
 {
 	struct fixed_phy_status status = {};
 
 	status.link = 1;
 	status.duplex = of_property_read_bool(np, "full-duplex");
 	if (of_property_read_u32(np, "speed", &status.speed))
@@ -296,5 +292,14 @@ int of_phy_register_fixed_link(struct device_node *np)
 
 	return fixed_phy_register(PHY_POLL, &status, np);
 }
-EXPORT_SYMBOL(of_phy_register_fixed_link);
+
+void of_phy_register_fixed_phys(void)
+{
+	struct device_node *np;
+
+	for_each_compatible_node(np, NULL, "fixed-eth-phy") {
+		of_phy_register_fixed_link(np);
+	}
+}
+
 #endif
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 77a6e32..c8d080b 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -68,17 +68,9 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np)
 #endif /* CONFIG_OF */
 
 #if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY)
-extern int of_phy_register_fixed_link(struct device_node *np);
-extern bool of_phy_is_fixed_link(struct device_node *np);
+extern void of_phy_register_fixed_phys(void);
 #else
-static inline int of_phy_register_fixed_link(struct device_node *np)
-{
-	return -ENOSYS;
-}
-static inline bool of_phy_is_fixed_link(struct device_node *np)
-{
-	return false;
-}
+static inline void of_phy_register_fixed_phys(void) {};
 #endif
 
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCHv3 0/4] Add DT support for fixed PHYs

[Sergei Shtylyov]

Hello.

On 03/04/2014 01:58 PM, Thomas Petazzoni wrote:

> Here is a third version of the patch set that adds a Device Tree
> binding and the related code to support fixed PHYs.

> Since the second version, the changes have been:

>   * Rebased on top of v3.14-rc1, and re-tested on hardware.

    This needs to be rebased atop of net-next.git repo on git.kernel.org.

WBR, Sergei

Re: [PATCHv3 1/4] net: phy: decouple PHY id and PHY address in fixed PHY driver

[Florian Fainelli]

2014-03-04 2:58 GMT-08:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Until now, the fixed_phy_add() function was taking as argument
> 'phy_id', which was used both as the PHY address on the fake fixed
> MDIO bus, and as the PHY id, as available in the MII_PHYSID1 and
> MII_PHYSID2 registers. However, those two informations are completely
> unrelated.
>
> This patch decouples them. The PHY id of fixed PHYs is hardcoded to be
> 0xdeadbeef. Ideally, a really reserved value would be nicer, but there
> doesn't seem to be an easy of making sure a dummy value can be
> assigned to the Linux kernel for such usage.
>
> The PHY address remains passed by the caller of phy_fixed_add().
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/net/phy/fixed.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c
> index ba55adf..0f02403 100644
> --- a/drivers/net/phy/fixed.c
> +++ b/drivers/net/phy/fixed.c
> @@ -31,7 +31,7 @@ struct fixed_mdio_bus {
>  };
>
>  struct fixed_phy {
> -       int id;
> +       int addr;
>         u16 regs[MII_REGS_NUM];
>         struct phy_device *phydev;
>         struct fixed_phy_status status;
> @@ -104,8 +104,8 @@ static int fixed_phy_update_regs(struct fixed_phy *fp)
>         if (fp->status.asym_pause)
>                 lpa |= LPA_PAUSE_ASYM;
>
> -       fp->regs[MII_PHYSID1] = fp->id >> 16;
> -       fp->regs[MII_PHYSID2] = fp->id;
> +       fp->regs[MII_PHYSID1] = 0xdead;
> +       fp->regs[MII_PHYSID2] = 0xbeef;

I am still scratching my head as to whether we want that change to be
in this particular version of changes, or if we want that to happen at
a later time when (if?) we can finally get some proper OUI number
allocation.

Technically we are presenting some sort of ABI to user-space, although
detecting a fixed PHY device by reading its MII_PHYSID1/2 and matching
it against its PHY address on the fixed MDIO bus would have been a
little "weak" (especially when you can check that the parent device in
sysfs is the fixed-0 bus).

>
>         fp->regs[MII_BMSR] = bmsr;
>         fp->regs[MII_BMCR] = bmcr;
> @@ -115,7 +115,7 @@ static int fixed_phy_update_regs(struct fixed_phy *fp)
>         return 0;
>  }
>
> -static int fixed_mdio_read(struct mii_bus *bus, int phy_id, int reg_num)
> +static int fixed_mdio_read(struct mii_bus *bus, int phy_addr, int reg_num)
>  {
>         struct fixed_mdio_bus *fmb = bus->priv;
>         struct fixed_phy *fp;
> @@ -124,7 +124,7 @@ static int fixed_mdio_read(struct mii_bus *bus, int phy_id, int reg_num)
>                 return -1;
>
>         list_for_each_entry(fp, &fmb->phys, node) {
> -               if (fp->id == phy_id) {
> +               if (fp->addr == phy_addr) {
>                         /* Issue callback if user registered it. */
>                         if (fp->link_update) {
>                                 fp->link_update(fp->phydev->attached_dev,
> @@ -138,7 +138,7 @@ static int fixed_mdio_read(struct mii_bus *bus, int phy_id, int reg_num)
>         return 0xFFFF;
>  }
>
> -static int fixed_mdio_write(struct mii_bus *bus, int phy_id, int reg_num,
> +static int fixed_mdio_write(struct mii_bus *bus, int phy_addr, int reg_num,
>                             u16 val)
>  {
>         return 0;
> @@ -160,7 +160,7 @@ int fixed_phy_set_link_update(struct phy_device *phydev,
>                 return -EINVAL;
>
>         list_for_each_entry(fp, &fmb->phys, node) {
> -               if (fp->id == phydev->phy_id) {
> +               if (fp->addr == phydev->addr) {
>                         fp->link_update = link_update;
>                         fp->phydev = phydev;
>                         return 0;
> @@ -171,7 +171,7 @@ int fixed_phy_set_link_update(struct phy_device *phydev,
>  }
>  EXPORT_SYMBOL_GPL(fixed_phy_set_link_update);
>
> -int fixed_phy_add(unsigned int irq, int phy_id,
> +int fixed_phy_add(unsigned int irq, int phy_addr,
>                   struct fixed_phy_status *status)
>  {
>         int ret;
> @@ -184,9 +184,9 @@ int fixed_phy_add(unsigned int irq, int phy_id,
>
>         memset(fp->regs, 0xFF,  sizeof(fp->regs[0]) * MII_REGS_NUM);
>
> -       fmb->irqs[phy_id] = irq;
> +       fmb->irqs[phy_addr] = irq;
>
> -       fp->id = phy_id;
> +       fp->addr = phy_addr;
>         fp->status = *status;
>
>         ret = fixed_phy_update_regs(fp);
> --
> 1.8.3.2



-- 
Florian

Re: [PATCHv3 2/4] net: phy: extend fixed driver with fixed_phy_register()

[Florian Fainelli]

2014-03-04 2:58 GMT-08:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> The existing fixed_phy_add() function has several drawbacks that
> prevents it from being used as is for OF-based declaration of fixed
> PHYs:
>
>  * The address of the PHY on the fake bus needs to be passed, while a
>    dynamic allocation is desired.
>
>  * Since the phy_device instantiation is post-poned until the next
>    mdiobus scan, there is no way to associate the fixed PHY with its
>    OF node, which later prevents of_phy_connect() from finding this
>    fixed PHY from a given OF node.
>
> To solve this, this commit introduces fixed_phy_register(), which will
> allocate an available PHY address, add the PHY using fixed_phy_add()
> and instantiate the phy_device structure associated with the provided
> OF node.

Besides the !CONFIG_FIXED_PHY hiccup (see below), this looks good to me:

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/net/phy/fixed.c   | 61 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/phy_fixed.h | 11 +++++++++
>  2 files changed, 72 insertions(+)
>
> diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c
> index 0f02403..82095cc 100644
> --- a/drivers/net/phy/fixed.c
> +++ b/drivers/net/phy/fixed.c
> @@ -21,6 +21,7 @@
>  #include <linux/phy_fixed.h>
>  #include <linux/err.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
>
>  #define MII_REGS_NUM 29
>
> @@ -203,6 +204,66 @@ err_regs:
>  }
>  EXPORT_SYMBOL_GPL(fixed_phy_add);
>
> +void fixed_phy_del(int phy_addr)
> +{
> +       struct fixed_mdio_bus *fmb = &platform_fmb;
> +       struct fixed_phy *fp, *tmp;
> +
> +       list_for_each_entry_safe(fp, tmp, &fmb->phys, node) {
> +               if (fp->addr == phy_addr) {
> +                       list_del(&fp->node);
> +                       kfree(fp);
> +                       return;
> +               }
> +       }
> +}
> +EXPORT_SYMBOL_GPL(fixed_phy_del);
> +
> +static int phy_fixed_addr;
> +static DEFINE_SPINLOCK(phy_fixed_addr_lock);
> +
> +int fixed_phy_register(unsigned int irq,
> +                      struct fixed_phy_status *status,
> +                      struct device_node *np)
> +{
> +       struct fixed_mdio_bus *fmb = &platform_fmb;
> +       struct phy_device *phy;
> +       int phy_addr;
> +       int ret;
> +
> +       /* Get the next available PHY address, up to PHY_MAX_ADDR */
> +       spin_lock(&phy_fixed_addr_lock);
> +       if (phy_fixed_addr == PHY_MAX_ADDR) {
> +               spin_unlock(&phy_fixed_addr_lock);
> +               return -ENOSPC;
> +       }
> +       phy_addr = phy_fixed_addr++;
> +       spin_unlock(&phy_fixed_addr_lock);
> +
> +       ret = fixed_phy_add(PHY_POLL, phy_addr, status);
> +       if (ret < 0)
> +               return ret;
> +
> +       phy = get_phy_device(fmb->mii_bus, phy_addr, false);
> +       if (!phy || IS_ERR(phy)) {
> +               fixed_phy_del(phy_addr);
> +               return -EINVAL;
> +       }
> +
> +       of_node_get(np);
> +       phy->dev.of_node = np;
> +
> +       ret = phy_device_register(phy);
> +       if (ret) {
> +               phy_device_free(phy);
> +               of_node_put(np);
> +               fixed_phy_del(phy_addr);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
>  static int __init fixed_mdio_bus_init(void)
>  {
>         struct fixed_mdio_bus *fmb = &platform_fmb;
> diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
> index 509d8f5..902e8a1 100644
> --- a/include/linux/phy_fixed.h
> +++ b/include/linux/phy_fixed.h
> @@ -9,15 +9,26 @@ struct fixed_phy_status {
>         int asym_pause;
>  };
>
> +struct device_node;
> +
>  #ifdef CONFIG_FIXED_PHY
>  extern int fixed_phy_add(unsigned int irq, int phy_id,
>                          struct fixed_phy_status *status);
> +extern int fixed_phy_register(unsigned int irq,
> +                             struct fixed_phy_status *status,
> +                             struct device_node *np);
>  #else
>  static inline int fixed_phy_add(unsigned int irq, int phy_id,
>                                 struct fixed_phy_status *status)
>  {
>         return -ENODEV;
>  }
> +extern int fixed_phy_register(unsigned int irq,
> +                             struct fixed_phy_status *status,
> +                             struct device_node *np)
> +{
> +       return -ENODEV;
> +}

s/extern/static/ otherwise I doubt this builds correctly.
-- 
Florian

Re: [PATCHv3 1/4] net: phy: decouple PHY id and PHY address in fixed PHY driver

[Thomas Petazzoni]

Dear Florian Fainelli,

On Tue, 4 Mar 2014 10:43:12 -0800, Florian Fainelli wrote:

> >  struct fixed_phy {
> > -       int id;
> > +       int addr;
> >         u16 regs[MII_REGS_NUM];
> >         struct phy_device *phydev;
> >         struct fixed_phy_status status;
> > @@ -104,8 +104,8 @@ static int fixed_phy_update_regs(struct fixed_phy *fp)
> >         if (fp->status.asym_pause)
> >                 lpa |= LPA_PAUSE_ASYM;
> >
> > -       fp->regs[MII_PHYSID1] = fp->id >> 16;
> > -       fp->regs[MII_PHYSID2] = fp->id;
> > +       fp->regs[MII_PHYSID1] = 0xdead;
> > +       fp->regs[MII_PHYSID2] = 0xbeef;
> 
> I am still scratching my head as to whether we want that change to be
> in this particular version of changes, or if we want that to happen at
> a later time when (if?) we can finally get some proper OUI number
> allocation.
> 
> Technically we are presenting some sort of ABI to user-space, although
> detecting a fixed PHY device by reading its MII_PHYSID1/2 and matching
> it against its PHY address on the fixed MDIO bus would have been a
> little "weak" (especially when you can check that the parent device in
> sysfs is the fixed-0 bus).

Well the problem is that fp->id really isn't an id, it's the fake
address of the PHY on the fake fixed MDIO bus. So it would mean that
the MII_PHYSID of the first fixed PHY would probably be 0x0, then the
second would have 0x1, then 0x2, and so on.

Probably not worse than what is done today, though.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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: [PATCHv3 3/4] of: provide a binding for fixed link PHYs

[Florian Fainelli]

Hi Thomas,

2014-03-04 2:58 GMT-08:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Some Ethernet MACs have a "fixed link", and are not connected to a
> normal MDIO-managed PHY device. For those situations, a Device Tree
> binding allows to describe a "fixed link" using a special PHY node.
>
> This patch adds:
>
>  * A documentation for the fixed PHY Device Tree binding.
>
>  * An of_phy_is_fixed_link() function that an Ethernet driver can call
>    on its PHY phandle to find out whether it's a fixed link PHY or
>    not. It should typically be used to know if
>    of_phy_register_fixed_link() should be called.
>
>  * An of_phy_register_fixed_link() function that instantiates the
>    fixed PHY into the PHY subsystem, so that when the driver calls
>    of_phy_connect(), the PHY device associated to the OF node will be
>    found.

I have a better understanding of why Grant objected last time, and I
just found yet another argument which should hopefully be in favor of
this new binding.

One of the problems mentioned earlier about the 'fixed-link' 5-digit
property is that it is inflexible. It turns out that it seems to
capture everything we need, as nobody needed to extend it with a 6th
type for instance. This new binding allows for more flexibility and is
more human readable, which is a net bonus, though not a compelling
reason (yet).

Another problem with that "old" 'fixed-link' property is that we are
not properly capturing and representing Ethernet switches/PHYs whose
data-path are isolated from the control path. For instance such
devices will traditionally expose their control path as a
MMIO/GPIO/I2C/SPI interface. Using the 5-digit 'fixed-link' property
we are not representing this, on one side the Ethernet MAC is just
told to hardcode the link parameters with some parameters, and on
other side, any MMIO/GPIO/I2C/SPI device is not equipped with the
correct properties to express the fact that is also has a data-path
connected to an Ethernet MAC.

What I like about this new binding is that we could place the
'fixed-link' related properties in e.g: a SPI slave node, and have the
Ethernet MAC be pointed at it by a phandle to tell it: look this is
your PHY, it might not be one you could address on a MDIO bus, so I
have been providing additional properties to help you with the link
configuration.

One thing that needs to be addressed in this patch is how to deal with
the existing 5-digit fixed-link, something that sounds fairly easy and
which would not require changing the callers of of_phy_connect_fixed()
is to do the following:

- of_phy_is_fixed_link() needs to look for *all* required compatible
properties of the new binding to give an accurate verdict on the
nature of the PHY (to avoid false positives as mentioned in PATCH 4),
and it also needs to look for the 5-digit fixed-link property and
ensure the property is 5-digits long if existing

- of_phy_register_fixed_link() needs to also parse the old 5-digit
fixed-link property, most likely just copy-pasting what
arch/powerpc/sysdev/fsl_soc.c::of_add_fixed_phys does with the
property endian-swapping (as this code is for PowerPC)

Then we can deal with how to make that semi-automatic for the new
binding users to make it smoother to use a regular or "fixed PHY"
device.

>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  .../devicetree/bindings/net/fixed-link.txt         | 34 ++++++++++++++++++++++
>  drivers/of/of_mdio.c                               | 24 +++++++++++++++
>  include/linux/of_mdio.h                            | 15 ++++++++++
>  3 files changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt
>
> diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt
> new file mode 100644
> index 0000000..9f2a1a50
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt
> @@ -0,0 +1,34 @@
> +Fixed link Device Tree binding
> +------------------------------
> +
> +Some Ethernet MACs have a "fixed link", and are not connected to a
> +normal MDIO-managed PHY device. For those situations, a Device Tree
> +binding allows to describe a "fixed link".
> +
> +Such a fixed link situation is described by creating a PHY node as a
> +sub-node of an Ethernet device, with the following properties:
> +
> +* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a
> +  fixed link PHY.
> +* 'speed' (integer, mandatory), to indicate the link speed. Accepted
> +  values are 10, 100 and 1000
> +* 'full-duplex' (boolean, optional), to indicate that full duplex is
> +  used. When absent, half duplex is assumed.
> +* 'pause' (boolean, optional), to indicate that pause should be
> +  enabled.
> +* 'asym-pause' (boolean, optional), to indicate that asym_pause should
> +  be enabled.
> +
> +Example:
> +
> +ethernet@0 {
> +       ...
> +       phy = <&phy0>;
> +       phy0: phy@0 {
> +             fixed-link;
> +             speed = <1000>;
> +             full-duplex;
> +       };
> +       ...
> +};
> +
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 875b7b6..c645fb8 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -14,6 +14,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/err.h>
>  #include <linux/phy.h>
> +#include <linux/phy_fixed.h>
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_mdio.h>
> @@ -274,3 +275,26 @@ struct phy_device *of_phy_attach(struct net_device *dev,
>         return phy_attach_direct(dev, phy, flags, iface) ? NULL : phy;
>  }
>  EXPORT_SYMBOL(of_phy_attach);
> +
> +#if defined(CONFIG_FIXED_PHY)
> +bool of_phy_is_fixed_link(struct device_node *np)
> +{
> +       return of_property_read_bool(np, "fixed-link");
> +}
> +EXPORT_SYMBOL(of_phy_is_fixed_link);
> +
> +int of_phy_register_fixed_link(struct device_node *np)
> +{
> +       struct fixed_phy_status status = {};
> +
> +       status.link = 1;
> +       status.duplex = of_property_read_bool(np, "full-duplex");
> +       if (of_property_read_u32(np, "speed", &status.speed))
> +               return -EINVAL;
> +       status.pause = of_property_read_bool(np, "pause");
> +       status.asym_pause = of_property_read_bool(np, "asym-pause");
> +
> +       return fixed_phy_register(PHY_POLL, &status, np);
> +}
> +EXPORT_SYMBOL(of_phy_register_fixed_link);
> +#endif
> diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
> index 6fe8464..77a6e32 100644
> --- a/include/linux/of_mdio.h
> +++ b/include/linux/of_mdio.h
> @@ -67,4 +67,19 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np)
>  }
>  #endif /* CONFIG_OF */
>
> +#if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY)
> +extern int of_phy_register_fixed_link(struct device_node *np);
> +extern bool of_phy_is_fixed_link(struct device_node *np);
> +#else
> +static inline int of_phy_register_fixed_link(struct device_node *np)
> +{
> +       return -ENOSYS;
> +}
> +static inline bool of_phy_is_fixed_link(struct device_node *np)
> +{
> +       return false;
> +}
> +#endif
> +
> +
>  #endif /* __LINUX_OF_MDIO_H */
> --
> 1.8.3.2



-- 
Florian

Re: [PATCHv3 3/4] of: provide a binding for fixed link PHYs

[Thomas Petazzoni]

Dear Florian Fainelli,

On Tue, 4 Mar 2014 12:58:41 -0800, Florian Fainelli wrote:


> What I like about this new binding is that we could place the
> 'fixed-link' related properties in e.g: a SPI slave node, and have the
> Ethernet MAC be pointed at it by a phandle to tell it: look this is
> your PHY, it might not be one you could address on a MDIO bus, so I
> have been providing additional properties to help you with the link
> configuration.
> 
> One thing that needs to be addressed in this patch is how to deal with
> the existing 5-digit fixed-link, something that sounds fairly easy and
> which would not require changing the callers of of_phy_connect_fixed()
> is to do the following:

I am not sure to understand "would not require changing the callers of
of_phy_connect_fixed()". This function is precisely introduced by the
patch set, so how would we need to "change the callers" ? Maybe you're
making a confusion with the existing of_phy_connect_fixed_link(), which
is used by network drivers to create a PHY using the old-style
fixed-link = <5 digits> binding ?

> - of_phy_is_fixed_link() needs to look for *all* required compatible
> properties of the new binding to give an accurate verdict on the
> nature of the PHY (to avoid false positives as mentioned in PATCH 4),

Hum?

The false positive problem only exists if you want to automatically
instantiate the fixed PHYs, as I proposed in a patch as a reply to my
series. And checking for *all* required properties does not make the
problem better: you could very well have other nodes in the tree that
have a "fixed-link" and a "speed" property, for example.

> and it also needs to look for the 5-digit fixed-link property and
> ensure the property is 5-digits long if existing

I don't understand how this could work. The of_phy_is_fixed_link()
function is meant to take as argument a Device Tree node that describes
a fixed PHY, using the new proposed DT binding for fixed PHYs.

The old 'fixed-link' binding has the fixed-link property as part of the
Ethernet node itself.

So I don't really see how a sane function could check both.

> - of_phy_register_fixed_link() needs to also parse the old 5-digit
> fixed-link property, most likely just copy-pasting what
> arch/powerpc/sysdev/fsl_soc.c::of_add_fixed_phys does with the
> property endian-swapping (as this code is for PowerPC)
> 
> Then we can deal with how to make that semi-automatic for the new
> binding users to make it smoother to use a regular or "fixed PHY"
> device.

I still don't understand. With the old binding, the "fixed-link"
property is within some random Ethernet node, and there is no way for
us to find out whether a given node having a "fixed-link" property
corresponds to a fixed PHY, or something completely unrelated.

So to conclude, I'm sorry, but I didn't understand at all what you
meant to say here, so I'm completely puzzled about what your
suggestions are.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCHv3 3/4] of: provide a binding for fixed link PHYs

[Florian Fainelli]

2014-03-05 1:24 GMT-08:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Dear Florian Fainelli,
>
> On Tue, 4 Mar 2014 12:58:41 -0800, Florian Fainelli wrote:
>
>
>> What I like about this new binding is that we could place the
>> 'fixed-link' related properties in e.g: a SPI slave node, and have the
>> Ethernet MAC be pointed at it by a phandle to tell it: look this is
>> your PHY, it might not be one you could address on a MDIO bus, so I
>> have been providing additional properties to help you with the link
>> configuration.
>>
>> One thing that needs to be addressed in this patch is how to deal with
>> the existing 5-digit fixed-link, something that sounds fairly easy and
>> which would not require changing the callers of of_phy_connect_fixed()
>> is to do the following:
>
> I am not sure to understand "would not require changing the callers of
> of_phy_connect_fixed()". This function is precisely introduced by the
> patch set, so how would we need to "change the callers" ? Maybe you're
> making a confusion with the existing of_phy_connect_fixed_link(), which
> is used by network drivers to create a PHY using the old-style
> fixed-link = <5 digits> binding ?

I meant to write of_phy_connect_fixed_link() here.

>
>> - of_phy_is_fixed_link() needs to look for *all* required compatible
>> properties of the new binding to give an accurate verdict on the
>> nature of the PHY (to avoid false positives as mentioned in PATCH 4),
>
> Hum?
>
> The false positive problem only exists if you want to automatically
> instantiate the fixed PHYs, as I proposed in a patch as a reply to my
> series. And checking for *all* required properties does not make the
> problem better: you could very well have other nodes in the tree that
> have a "fixed-link" and a "speed" property, for example.

Correct, but that means there is probably something missing here to
uniquely identify this type of fixed PHYs then. 'fixed-link' could
indeed be too generic, so we need to find a better name which is
guaranteed to be unique to our use-case here. You offered a compatible
string solution earlier, I am not sure that will fly very well too as
we would need to keep an ever-growing list of special devices.

>
>> and it also needs to look for the 5-digit fixed-link property and
>> ensure the property is 5-digits long if existing
>
> I don't understand how this could work. The of_phy_is_fixed_link()
> function is meant to take as argument a Device Tree node that describes
> a fixed PHY, using the new proposed DT binding for fixed PHYs.
>
> The old 'fixed-link' binding has the fixed-link property as part of the
> Ethernet node itself.

The drivers using the 5-digit 'fixed-link' property already have that
knowledge, so they can call of_phy_is_fixed_link() with a Ethernet
Device Tree node argument, whereas newer driver using the new binding
would call you with a phandle device tree node. This makes no
difference for your specific function which will return a verdict.
Once we know that we are dealing with such a fixed PHY, we can call
of_phy_register_fixed_link() which will do what
arch/powerpc/sysdev/fsl_soc.c::of_add_fixed_phys does today, except
that it will be for users beyond PowerPC (e.g: bcmgenet).

>
> So I don't really see how a sane function could check both.

Something like:

int sz;

of_property_read_bool(np, "fixed-link") || (of_get_property(np,
"fixed-link", &sz) && sz == sizeof(u32) * 5);

The key point is that drivers using the "old" 5-digit 'fixed-link'
already know that their 'fixed-link' property belongs to their
Ethernet Device Tree node.

>
>> - of_phy_register_fixed_link() needs to also parse the old 5-digit
>> fixed-link property, most likely just copy-pasting what
>> arch/powerpc/sysdev/fsl_soc.c::of_add_fixed_phys does with the
>> property endian-swapping (as this code is for PowerPC)
>>
>> Then we can deal with how to make that semi-automatic for the new
>> binding users to make it smoother to use a regular or "fixed PHY"
>> device.
>
> I still don't understand. With the old binding, the "fixed-link"
> property is within some random Ethernet node, and there is no way for
> us to find out whether a given node having a "fixed-link" property
> corresponds to a fixed PHY, or something completely unrelated.

By then, I meant, once we have sorted out that specific patchset, we
can try to work on a solution to make

>
> So to conclude, I'm sorry, but I didn't understand at all what you
> meant to say here, so I'm completely puzzled about what your
> suggestions are.
>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com



-- 
Florian

Re: [PATCHv3 1/4] net: phy: decouple PHY id and PHY address in fixed PHY driver

[Grant Likely]

On Tue, 4 Mar 2014 20:04:24 +0100, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:
> Dear Florian Fainelli,
> 
> On Tue, 4 Mar 2014 10:43:12 -0800, Florian Fainelli wrote:
> 
> > >  struct fixed_phy {
> > > -       int id;
> > > +       int addr;
> > >         u16 regs[MII_REGS_NUM];
> > >         struct phy_device *phydev;
> > >         struct fixed_phy_status status;
> > > @@ -104,8 +104,8 @@ static int fixed_phy_update_regs(struct fixed_phy *fp)
> > >         if (fp->status.asym_pause)
> > >                 lpa |= LPA_PAUSE_ASYM;
> > >
> > > -       fp->regs[MII_PHYSID1] = fp->id >> 16;
> > > -       fp->regs[MII_PHYSID2] = fp->id;
> > > +       fp->regs[MII_PHYSID1] = 0xdead;
> > > +       fp->regs[MII_PHYSID2] = 0xbeef;
> > 
> > I am still scratching my head as to whether we want that change to be
> > in this particular version of changes, or if we want that to happen at
> > a later time when (if?) we can finally get some proper OUI number
> > allocation.
> > 
> > Technically we are presenting some sort of ABI to user-space, although
> > detecting a fixed PHY device by reading its MII_PHYSID1/2 and matching
> > it against its PHY address on the fixed MDIO bus would have been a
> > little "weak" (especially when you can check that the parent device in
> > sysfs is the fixed-0 bus).
> 
> Well the problem is that fp->id really isn't an id, it's the fake
> address of the PHY on the fake fixed MDIO bus. So it would mean that
> the MII_PHYSID of the first fixed PHY would probably be 0x0, then the
> second would have 0x1, then 0x2, and so on.

I think the change is reasonable, but I'm not thrilled with the 0xdeadbeaf
constant. I'd rather PHYSID simply be left as 0.

g.

Re: [PATCHv3 2/4] net: phy: extend fixed driver with fixed_phy_register()

[Grant Likely]

On Tue,  4 Mar 2014 11:58:22 +0100, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:
> The existing fixed_phy_add() function has several drawbacks that
> prevents it from being used as is for OF-based declaration of fixed
> PHYs:
> 
>  * The address of the PHY on the fake bus needs to be passed, while a
>    dynamic allocation is desired.
> 
>  * Since the phy_device instantiation is post-poned until the next
>    mdiobus scan, there is no way to associate the fixed PHY with its
>    OF node, which later prevents of_phy_connect() from finding this
>    fixed PHY from a given OF node.
> 
> To solve this, this commit introduces fixed_phy_register(), which will
> allocate an available PHY address, add the PHY using fixed_phy_add()
> and instantiate the phy_device structure associated with the provided
> OF node.

The phy address assignment is a little naive, but that can be fixed in a
follow-up patch (see below for a suggestion)

Acked-by: Grant Likely <grant.likely@linaro.org>

> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/net/phy/fixed.c   | 61 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/phy_fixed.h | 11 +++++++++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c
> index 0f02403..82095cc 100644
> --- a/drivers/net/phy/fixed.c
> +++ b/drivers/net/phy/fixed.c
> @@ -21,6 +21,7 @@
>  #include <linux/phy_fixed.h>
>  #include <linux/err.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
>  
>  #define MII_REGS_NUM 29
>  
> @@ -203,6 +204,66 @@ err_regs:
>  }
>  EXPORT_SYMBOL_GPL(fixed_phy_add);
>  
> +void fixed_phy_del(int phy_addr)
> +{
> +	struct fixed_mdio_bus *fmb = &platform_fmb;
> +	struct fixed_phy *fp, *tmp;
> +
> +	list_for_each_entry_safe(fp, tmp, &fmb->phys, node) {
> +		if (fp->addr == phy_addr) {
> +			list_del(&fp->node);
> +			kfree(fp);
> +			return;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL_GPL(fixed_phy_del);
> +
> +static int phy_fixed_addr;
> +static DEFINE_SPINLOCK(phy_fixed_addr_lock);
> +
> +int fixed_phy_register(unsigned int irq,
> +		       struct fixed_phy_status *status,
> +		       struct device_node *np)
> +{
> +	struct fixed_mdio_bus *fmb = &platform_fmb;
> +	struct phy_device *phy;
> +	int phy_addr;
> +	int ret;
> +
> +	/* Get the next available PHY address, up to PHY_MAX_ADDR */
> +	spin_lock(&phy_fixed_addr_lock);
> +	if (phy_fixed_addr == PHY_MAX_ADDR) {
> +		spin_unlock(&phy_fixed_addr_lock);
> +		return -ENOSPC;
> +	}
> +	phy_addr = phy_fixed_addr++;

If this instead looked over the fixed mdio bus list then an unallocated
address could be assigned without the global static.

> +	spin_unlock(&phy_fixed_addr_lock);
> +
> +	ret = fixed_phy_add(PHY_POLL, phy_addr, status);
> +	if (ret < 0)
> +		return ret;
> +
> +	phy = get_phy_device(fmb->mii_bus, phy_addr, false);
> +	if (!phy || IS_ERR(phy)) {
> +		fixed_phy_del(phy_addr);
> +		return -EINVAL;
> +	}
> +
> +	of_node_get(np);
> +	phy->dev.of_node = np;
> +
> +	ret = phy_device_register(phy);
> +	if (ret) {
> +		phy_device_free(phy);
> +		of_node_put(np);
> +		fixed_phy_del(phy_addr);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int __init fixed_mdio_bus_init(void)
>  {
>  	struct fixed_mdio_bus *fmb = &platform_fmb;
> diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
> index 509d8f5..902e8a1 100644
> --- a/include/linux/phy_fixed.h
> +++ b/include/linux/phy_fixed.h
> @@ -9,15 +9,26 @@ struct fixed_phy_status {
>  	int asym_pause;
>  };
>  
> +struct device_node;
> +
>  #ifdef CONFIG_FIXED_PHY
>  extern int fixed_phy_add(unsigned int irq, int phy_id,
>  			 struct fixed_phy_status *status);
> +extern int fixed_phy_register(unsigned int irq,
> +			      struct fixed_phy_status *status,
> +			      struct device_node *np);
>  #else
>  static inline int fixed_phy_add(unsigned int irq, int phy_id,
>  				struct fixed_phy_status *status)
>  {
>  	return -ENODEV;
>  }
> +extern int fixed_phy_register(unsigned int irq,
> +			      struct fixed_phy_status *status,
> +			      struct device_node *np)
> +{
> +	return -ENODEV;
> +}
>  #endif /* CONFIG_FIXED_PHY */
>  
>  /*
> -- 
> 1.8.3.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv3 3/4] of: provide a binding for fixed link PHYs

[Grant Likely]

On Tue,  4 Mar 2014 11:58:23 +0100, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:
> Some Ethernet MACs have a "fixed link", and are not connected to a
> normal MDIO-managed PHY device. For those situations, a Device Tree
> binding allows to describe a "fixed link" using a special PHY node.
> 
> This patch adds:
> 
>  * A documentation for the fixed PHY Device Tree binding.
> 
>  * An of_phy_is_fixed_link() function that an Ethernet driver can call
>    on its PHY phandle to find out whether it's a fixed link PHY or
>    not. It should typically be used to know if
>    of_phy_register_fixed_link() should be called.
> 
>  * An of_phy_register_fixed_link() function that instantiates the
>    fixed PHY into the PHY subsystem, so that when the driver calls
>    of_phy_connect(), the PHY device associated to the OF node will be
>    found.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  .../devicetree/bindings/net/fixed-link.txt         | 34 ++++++++++++++++++++++
>  drivers/of/of_mdio.c                               | 24 +++++++++++++++
>  include/linux/of_mdio.h                            | 15 ++++++++++
>  3 files changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt
> new file mode 100644
> index 0000000..9f2a1a50
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt
> @@ -0,0 +1,34 @@
> +Fixed link Device Tree binding
> +------------------------------
> +
> +Some Ethernet MACs have a "fixed link", and are not connected to a
> +normal MDIO-managed PHY device. For those situations, a Device Tree
> +binding allows to describe a "fixed link".
> +
> +Such a fixed link situation is described by creating a PHY node as a
> +sub-node of an Ethernet device, with the following properties:
> +
> +* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a
> +  fixed link PHY.
> +* 'speed' (integer, mandatory), to indicate the link speed. Accepted
> +  values are 10, 100 and 1000
> +* 'full-duplex' (boolean, optional), to indicate that full duplex is
> +  used. When absent, half duplex is assumed.
> +* 'pause' (boolean, optional), to indicate that pause should be
> +  enabled.
> +* 'asym-pause' (boolean, optional), to indicate that asym_pause should
> +  be enabled.
> +
> +Example:
> +
> +ethernet@0 {
> +	...
> +	phy = <&phy0>;
> +	phy0: phy@0 {
> +	      fixed-link;
> +	      speed = <1000>;
> +	      full-duplex;
> +	};

The phy phandle to a child node is superfluous. A phandle to a fixed
child node doesn't make a whole lot of sense.

You've worn me down though. I'll ack the binding with a few tweaks in
usage. This binding provides a direct super-set of the old fixed-link
property binding. There should be a single function for parsing it. That
way we can allow the new behaviour on drivers using the old. The new
binding should be preferred over the old.

ie. a call to of_phy_is_fixed_link() should look for a fixed link node
first, followed by the older property version.

There should be a strong recommendation for the child node name. Make it
"fixed-link". The only reason a device should ever deviate from that is
if it has multiple links that all need to be processed.

If anyone complains that "fixed-link" is ambiguous, then perhaps
"mii-fixed-link" would be better. I don't see a problem though.

There should be no address portion in the node name. It isn't a child
device, the node is merely more configuration data for the parent.

Example:

ethernet@0 {
	...
	fixed-link {
	      speed = <1000>;
	      full-duplex;
	};

g.

Re: [PATCHv3 4/4] net: mvneta: add support for fixed links

[Grant Likely]

On Tue, 4 Mar 2014 12:30:38 +0100, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:
> Hello all,
> 
> On Tue,  4 Mar 2014 11:58:24 +0100, Thomas Petazzoni wrote:
> 
> >  	phy_node = of_parse_phandle(dn, "phy", 0);
> > -	if (!phy_node) {
> > -		dev_err(&pdev->dev, "no associated PHY\n");
> > -		err = -ENODEV;
> > -		goto err_free_irq;
> > +	if (of_phy_is_fixed_link(phy_node)) {
> > +		err = of_phy_register_fixed_link(phy_node);
> > +		if (err < 0) {
> > +			dev_err(&pdev->dev, "cannot register fixed PHY\n");
> > +			goto err_free_irq;
> > +		}
> >  	}
> >  
> >  	phy_mode = of_get_phy_mode(dn);
> 
> As noted by Sebastian Hesselbarth (talking with me on IRC), it would be
> nicer if network driver didn't had to be modified to ensure that the
> fixed PHY the network device is connected to gets registered into the
> kernel.
> 
> So ideally, the fixed MDIO bus implemented in drivers/net/phy/fixed.c
> should scan the Device Tree, and register at boot time the fixed PHYs.

That's a poor design. I don't like to see code parsing the entire tree
looking for something it recognizes. All bindings are ultimately under
control of the driver using it so that there is always the option to
work around crazy stuff.

Creating the fixed phy really does need to be triggered at probe time when
the ethernet device goes looking for a phy. We should be able to make
that more generic though. I suggest a single wrapper function that all
Eth drivers can use to hook up the PHY and will parse both real phys and
fixed links, but if a driver has a crazy binding, then it can override
it by open coding the individual decode steps.

> The only problem is how to recognize in the DT which nodes are fixed
> PHYs. Looking for the "fixed-link" property is very fragile as other
> completely unrelated nodes may use this property for other purposes.
> Using a compatible string? A device_type property? Or should we have a
> fragment of the DT for the fixed MDIO bus itself?
> 
> I've done a quick prototype that is based on using the "fixed-eth-phy"
> compatible string as the key to find fixed PHYs in the DT, just to see
> how it would work. It works well, and allows to get rid of changes in
> the network driver completely. See below for the prototype-level patch
> (which applies on top of the proposed patch series, for now).
> 
> Suggestions?
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 053a650..8a68a07 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -2805,14 +2805,6 @@ static int mvneta_probe(struct platform_device *pdev)
>  	}
>  
>  	phy_node = of_parse_phandle(dn, "phy", 0);
> -	if (of_phy_is_fixed_link(phy_node)) {
> -		err = of_phy_register_fixed_link(phy_node);
> -		if (err < 0) {
> -			dev_err(&pdev->dev, "cannot register fixed PHY\n");
> -			goto err_free_irq;
> -		}
> -	}
> -
>  	phy_mode = of_get_phy_mode(dn);
>  	if (phy_mode < 0) {
>  		dev_err(&pdev->dev, "incorrect phy-mode\n");
> diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c
> index 82095cc..45a7d5a 100644
> --- a/drivers/net/phy/fixed.c
> +++ b/drivers/net/phy/fixed.c
> @@ -22,6 +22,7 @@
>  #include <linux/err.h>
>  #include <linux/slab.h>
>  #include <linux/of.h>
> +#include <linux/of_mdio.h>
>  
>  #define MII_REGS_NUM 29
>  
> @@ -293,6 +294,8 @@ static int __init fixed_mdio_bus_init(void)
>  	if (ret)
>  		goto err_mdiobus_alloc;
>  
> +	of_phy_register_fixed_phys();
> +
>  	return 0;
>  
>  err_mdiobus_alloc:
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index c645fb8..4e75a70 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -277,16 +277,12 @@ struct phy_device *of_phy_attach(struct net_device *dev,
>  EXPORT_SYMBOL(of_phy_attach);
>  
>  #if defined(CONFIG_FIXED_PHY)
> -bool of_phy_is_fixed_link(struct device_node *np)
> -{
> -	return of_property_read_bool(np, "fixed-link");
> -}
> -EXPORT_SYMBOL(of_phy_is_fixed_link);
> -
> -int of_phy_register_fixed_link(struct device_node *np)
> +static int of_phy_register_fixed_link(struct device_node *np)
>  {
>  	struct fixed_phy_status status = {};
>  
>  	status.link = 1;
>  	status.duplex = of_property_read_bool(np, "full-duplex");
>  	if (of_property_read_u32(np, "speed", &status.speed))
> @@ -296,5 +292,14 @@ int of_phy_register_fixed_link(struct device_node *np)
>  
>  	return fixed_phy_register(PHY_POLL, &status, np);
>  }
> -EXPORT_SYMBOL(of_phy_register_fixed_link);
> +
> +void of_phy_register_fixed_phys(void)
> +{
> +	struct device_node *np;
> +
> +	for_each_compatible_node(np, NULL, "fixed-eth-phy") {
> +		of_phy_register_fixed_link(np);
> +	}
> +}
> +
>  #endif
> diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
> index 77a6e32..c8d080b 100644
> --- a/include/linux/of_mdio.h
> +++ b/include/linux/of_mdio.h
> @@ -68,17 +68,9 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np)
>  #endif /* CONFIG_OF */
>  
>  #if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY)
> -extern int of_phy_register_fixed_link(struct device_node *np);
> -extern bool of_phy_is_fixed_link(struct device_node *np);
> +extern void of_phy_register_fixed_phys(void);
>  #else
> -static inline int of_phy_register_fixed_link(struct device_node *np)
> -{
> -	return -ENOSYS;
> -}
> -static inline bool of_phy_is_fixed_link(struct device_node *np)
> -{
> -	return false;
> -}
> +static inline void of_phy_register_fixed_phys(void) {};
>  #endif
>  
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv3 3/4] of: provide a binding for fixed link PHYs

[Thomas Petazzoni]

Dear Grant Likely,

Sorry for bringing back this old topic, but I'm working on this again,
hopefully reaching an acceptable solution this time. One question for
you below.

On Sat, 08 Mar 2014 05:50:33 +0000, Grant Likely wrote:

> > +Example:
> > +
> > +ethernet@0 {
> > +	...
> > +	phy = <&phy0>;
> > +	phy0: phy@0 {
> > +	      fixed-link;
> > +	      speed = <1000>;
> > +	      full-duplex;
> > +	};
> 
> The phy phandle to a child node is superfluous. A phandle to a fixed
> child node doesn't make a whole lot of sense.

[...]

> There should be no address portion in the node name. It isn't a child
> device, the node is merely more configuration data for the parent.
> 
> Example:
> 
> ethernet@0 {
> 	...
> 	fixed-link {
> 	      speed = <1000>;
> 	      full-duplex;
> 	};

For my current use case, I'm personally fine with that. But that
doesn't work well with Florian Fainelli's which to potentially have the
"fixed-link" node as part of another node in the DT, in the case the
PHY is configurable through some separate SPI/I2C bus. See his comment
in http://article.gmane.org/gmane.linux.network/306789:

"""
Another problem with that "old" 'fixed-link' property is that we are
not properly capturing and representing Ethernet switches/PHYs whose
data-path are isolated from the control path. For instance such
devices will traditionally expose their control path as a
MMIO/GPIO/I2C/SPI interface. Using the 5-digit 'fixed-link' property
we are not representing this, on one side the Ethernet MAC is just
told to hardcode the link parameters with some parameters, and on
other side, any MMIO/GPIO/I2C/SPI device is not equipped with the
correct properties to express the fact that is also has a data-path
connected to an Ethernet MAC.

What I like about this new binding is that we could place the
'fixed-link' related properties in e.g: a SPI slave node, and have the
Ethernet MAC be pointed at it by a phandle to tell it: look this is
your PHY, it might not be one you could address on a MDIO bus, so I
have been providing additional properties to help you with the link
configuration.
"""

What is your opinion about this?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCHv3 3/4] of: provide a binding for fixed link PHYs

[Florian Fainelli]

Hi Thomas,

2014-05-15 6:39 GMT-07:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Dear Grant Likely,
>
> Sorry for bringing back this old topic, but I'm working on this again,
> hopefully reaching an acceptable solution this time. One question for
> you below.
>
> On Sat, 08 Mar 2014 05:50:33 +0000, Grant Likely wrote:
>
>> > +Example:
>> > +
>> > +ethernet@0 {
>> > +   ...
>> > +   phy = <&phy0>;
>> > +   phy0: phy@0 {
>> > +         fixed-link;
>> > +         speed = <1000>;
>> > +         full-duplex;
>> > +   };
>>
>> The phy phandle to a child node is superfluous. A phandle to a fixed
>> child node doesn't make a whole lot of sense.
>
> [...]
>
>> There should be no address portion in the node name. It isn't a child
>> device, the node is merely more configuration data for the parent.
>>
>> Example:
>>
>> ethernet@0 {
>>       ...
>>       fixed-link {
>>             speed = <1000>;
>>             full-duplex;
>>       };
>
> For my current use case, I'm personally fine with that. But that
> doesn't work well with Florian Fainelli's which to potentially have the
> "fixed-link" node as part of another node in the DT, in the case the
> PHY is configurable through some separate SPI/I2C bus. See his comment
> in http://article.gmane.org/gmane.linux.network/306789:

I just re-read Grant's comments, and I don't think we contradict with
each other here. Since the data-path is symetrical by nature, whether
you place the 'fixed-link' property on one end (Ethernet MAC) or the
other (SPI/I2C/GPIO node) does make a huge difference, except you want
it to be placed where it makes the most sense: in the Ethernet MAC
node. Properties like the speed, duplex, pause are definitively
Ethernet MAC only properties.

In case we need to specifically associate these two nodes with each
other, a phandle property to the Ethernet MAC node, or e.g: the SPI
node can be used.

That said, I think we should go with Grant's proposal of having the
following representation:

ethernet@0 {
      ...
       fixed-link {
             speed = <1000>;
             full-duplex;
       };

we might want to use 'max-speed' instead of speed, but that's debatable.

>
> """
> Another problem with that "old" 'fixed-link' property is that we are
> not properly capturing and representing Ethernet switches/PHYs whose
> data-path are isolated from the control path. For instance such
> devices will traditionally expose their control path as a
> MMIO/GPIO/I2C/SPI interface. Using the 5-digit 'fixed-link' property
> we are not representing this, on one side the Ethernet MAC is just
> told to hardcode the link parameters with some parameters, and on
> other side, any MMIO/GPIO/I2C/SPI device is not equipped with the
> correct properties to express the fact that is also has a data-path
> connected to an Ethernet MAC.
>
> What I like about this new binding is that we could place the
> 'fixed-link' related properties in e.g: a SPI slave node, and have the
> Ethernet MAC be pointed at it by a phandle to tell it: look this is
> your PHY, it might not be one you could address on a MDIO bus, so I
> have been providing additional properties to help you with the link
> configuration.
> """
>
> What is your opinion about this?
>
> Thanks,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
Florian