[PATCHv4 0/5] Add DT support for fixed PHYs

Subject: Add DT support for fixed PHYs

Hello,

Here is a fourth version of the patch set that adds a Device Tree
binding and the related code to support fixed PHYs. I'm hoping to get
this merged in 3.16.

Changes since v3:

 * Rebased on top of v3.15-rc5

 * In patch "net: phy: decouple PHY id and PHY address in fixed PHY
   driver", changed the PHY ID of fixed PHYs from 0xdeadbeef to 0x0,
   as suggested by Grant Likely.

 * Fixed the !CONFIG_PHY_FIXED case in patch "net: phy: extend fixed
   driver with fixed_phy_register()". Noticed by Florian Fainelli.

 * Added Acked-by from Grant Likely and Florian Fainelli on patch
   "net: phy: extend fixed driver with fixed_phy_register()".

 * Reworked the new fixed-link DT binding to be just a sub-node of the
   Ethernet MAC node, and not a node referenced by the 'phy'
   property. This was requested by Grant Likely.

 * Reworked the code implementing the new DT binding to also make it
   accept the old, single property based, DT binding.

 * Added a patch that actually uses the new fixed link DT binding for
   the Armada XP Matrix board.

Changes since v2:

 * 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.

Changes since v1:

 * 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
  PATCHv3: http://www.spinics.net/lists/netdev/msg273117.html

Thanks,

Thomas

Thomas Petazzoni (5):
  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
  ARM: mvebu: use the fixed-link PHY DT binding for the Armada XP Matrix
    board

 .../devicetree/bindings/net/fixed-link.txt         | 31 +++++++++
 arch/arm/boot/dts/armada-xp-matrix.dts             |  4 ++
 drivers/net/ethernet/marvell/mvneta.c              | 19 ++++-
 drivers/net/phy/fixed.c                            | 81 +++++++++++++++++++---
 drivers/of/of_mdio.c                               | 67 ++++++++++++++++++
 include/linux/of_mdio.h                            | 15 ++++
 include/linux/phy_fixed.h                          | 11 +++
 7 files changed, 215 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt

-- 
1.9.3

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

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
0x0. 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 <redacted>
---
 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..e41546d 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] = 0;
+	fp->regs[MII_PHYSID2] = 0;
 
 	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.9.3

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 <redacted>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Grant Likely <redacted>
---
 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 e41546d..d60d875 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..4f2478b 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;
 }
+static inline int fixed_phy_register(unsigned int irq,
+				     struct fixed_phy_status *status,
+				     struct device_node *np)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_FIXED_PHY */
 
 /*

-- 
1.9.3

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.

These two additional functions also support the old fixed-link Device
Tree binding used on PowerPC platforms, so that ultimately, the
network device drivers for those platforms could be converted to use
of_phy_is_fixed_link() and of_phy_register_fixed_link() instead of
of_phy_connect_fixed_link(), while keeping compatibility with their
respective Device Tree bindings.

Signed-off-by: Thomas Petazzoni <redacted>
---
 .../devicetree/bindings/net/fixed-link.txt         | 31 ++++++++++
 drivers/of/of_mdio.c                               | 67 ++++++++++++++++++++++
 include/linux/of_mdio.h                            | 15 +++++
 3 files changed, 113 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..377f775
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/fixed-link.txt

@@ -0,0 +1,31 @@
+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 'fixed-link'
+sub-node of the Ethernet MAC device node, with the following
+properties:
+
+* '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 {
+	...
+	fixed-link {
+	      speed = <1000>;
+	      full-duplex;
+	};
+	...
+};
+

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 9a95831..1def0bb 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>

@@ -301,3 +302,69 @@ 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)
+/*
+ * of_phy_is_fixed_link() and of_phy_register_fixed_link() must
+ * support two DT bindings:
+ * - the old DT binding, where 'fixed-link' was a property with 5
+ *   cells encoding various informations about the fixed PHY
+ * - the new DT binding, where 'fixed-link' is a sub-node of the
+ *   Ethernet device.
+ */
+bool of_phy_is_fixed_link(struct device_node *np)
+{
+	struct device_node *dn;
+	int len;
+
+	/* New binding */
+	dn = of_get_child_by_name(np, "fixed-link");
+	if (dn) {
+		of_node_put(dn);
+		return true;
+	}
+
+	/* Old binding */
+	if (of_get_property(np, "fixed-link", &len) &&
+	    len == (5 * sizeof(__be32)))
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL(of_phy_is_fixed_link);
+
+int of_phy_register_fixed_link(struct device_node *np)
+{
+	struct fixed_phy_status status = {};
+	struct device_node *fixed_link_node;
+	const __be32 *fixed_link_prop;
+	int len;
+
+	/* New binding */
+	fixed_link_node = of_get_child_by_name(np, "fixed-link");
+	if (fixed_link_node) {
+		status.link = 1;
+		status.duplex = of_property_read_bool(np, "full-duplex");
+		if (of_property_read_u32(fixed_link_node, "speed", &status.speed))
+			return -EINVAL;
+		status.pause = of_property_read_bool(np, "pause");
+		status.asym_pause = of_property_read_bool(np, "asym-pause");
+		of_node_put(fixed_link_node);
+		return fixed_phy_register(PHY_POLL, &status, np);
+	}
+
+	/* Old binding */
+	fixed_link_prop = of_get_property(np, "fixed-link", &len);
+	if (fixed_link_prop && len == (5 * sizeof(__be32))) {
+		status.link = 1;
+		status.duplex = be32_to_cpu(fixed_link_prop[1]);
+		status.speed = be32_to_cpu(fixed_link_prop[2]);
+		status.pause = be32_to_cpu(fixed_link_prop[3]);
+		status.asym_pause = be32_to_cpu(fixed_link_prop[4]);
+		return fixed_phy_register(PHY_POLL, &status, np);
+	}
+
+	return -ENODEV;
+}
+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.9.3

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 <redacted>
---
 drivers/net/ethernet/marvell/mvneta.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 14786c8..f232e3b 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c

@@ -2797,9 +2797,22 @@ 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(dn)) {
+			dev_err(&pdev->dev, "no PHY specified\n");
+			err = -ENODEV;
+			goto err_free_irq;
+		}
+
+		err = of_phy_register_fixed_link(dn);
+		if (err < 0) {
+			dev_err(&pdev->dev, "cannot register fixed PHY\n");
+			goto err_free_irq;
+		}
+
+		/* In the case of a fixed PHY, the DT node associated
+		 * to the PHY is the Ethernet MAC DT node.
+		 */
+		phy_node = dn;
 	}
 
 	phy_mode = of_get_phy_mode(dn);

-- 
1.9.3

The Armada XP Matrix board has an Ethernet PHY that isn't configurable
through the MDIO bus, so we use the newly introduced fixed-link PHY DT
binding to represent the PHY of this platform and get network working.

Signed-off-by: Thomas Petazzoni <redacted>
---
 arch/arm/boot/dts/armada-xp-matrix.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/armada-xp-matrix.dts b/arch/arm/boot/dts/armada-xp-matrix.dts
index c224274..3bb8c00 100644
--- a/arch/arm/boot/dts/armada-xp-matrix.dts
+++ b/arch/arm/boot/dts/armada-xp-matrix.dts

@@ -61,6 +61,10 @@
 			ethernet@30000 {
 				status = "okay";
 				phy-mode = "sgmii";
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+				};
 			};
 
 			pcie-controller {

-- 
1.9.3

Hi Thomas,

2014-05-16 7:14 GMT-07:00 Thomas Petazzoni
[off-list ref]:

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 <redacted>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Grant Likely <redacted>

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

-- 
Florian

2014-05-16 7:14 GMT-07:00 Thomas Petazzoni
[off-list ref]:

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
0x0. 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 <redacted>

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

-- 
Florian

Hello.

On 05/16/2014 06:14 PM, Thomas Petazzoni wrote:

[...]

[...]

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

    Was rummaging in the fixed_phy driver and a bug sprang right at me: 'phy' 
should have been passed here, not PHY_POLL. Luckily, all callers pass PHY_POLL 
anyway...

[...]

MBR, Sergei

On 03/09/15 12:20, Sergei Shtylyov wrote:

Hello.

On 05/16/2014 06:14 PM, Thomas Petazzoni wrote:

quoted

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 <redacted>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Grant Likely <redacted>
---
  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 e41546d..d60d875 100644
--- a/drivers/net/phy/fixed.c
+++ b/drivers/net/phy/fixed.c

[...]

quoted

@@ -203,6 +204,66 @@ err_regs:

[...]

quoted

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

   Was rummaging in the fixed_phy driver and a bug sprang right at me:
'phy' should have been passed here, not PHY_POLL. Luckily, all callers
pass PHY_POLL anyway...

Are we looking at the same header file for the prototype of fixed_phy_add()?

extern int fixed_phy_add(unsigned int irq, int phy_id,
                         struct fixed_phy_status *status,
                         int link_gpio);

First argument is correct here.. at any rate, if something needs fixing,
just go ahead and submit a patch.
--
Florian

Hello.

On 09/03/2015 10:23 PM, Florian Fainelli wrote:

quoted

quoted

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 <redacted>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Grant Likely <redacted>
---
   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 e41546d..d60d875 100644
--- a/drivers/net/phy/fixed.c
+++ b/drivers/net/phy/fixed.c

[...]

quoted

@@ -203,6 +204,66 @@ err_regs:

[...]

quoted

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

    Was rummaging in the fixed_phy driver and a bug sprang right at me:
'phy' should have been passed here, not PHY_POLL. Luckily, all callers
pass PHY_POLL anyway...

Are we looking at the same header file for the prototype of fixed_phy_add()?

    Probably not. I was looking at Linus' tree, yours is probably net-next. :-)

extern int fixed_phy_add(unsigned int irq, int phy_id,
                          struct fixed_phy_status *status,
                          int link_gpio);

First argument is correct here..

    No, fixed_phy_register() gets 'irq' passed to it and it should in its turn 
call fixed_phy_add() with this argument, not PHY_POLL; otherwise the 'irq' 
parameter gets completely ignored...

at any rate, if something needs fixing, just go ahead and submit a patch.

    OK.

--
Florian

MBR, Sergei

On 09/03/2015 10:20 PM, Sergei Shtylyov wrote:

quoted

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 <redacted>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Grant Likely <redacted>
---
  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 e41546d..d60d875 100644
--- a/drivers/net/phy/fixed.c
+++ b/drivers/net/phy/fixed.c

[...]

quoted

@@ -203,6 +204,66 @@ err_regs:

[...]

quoted

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

    Was rummaging in the fixed_phy driver and a bug sprang right at me: 'phy'

    Sorry, s/phy/irq/ of course. Just noticed. :-/

should have been passed here, not PHY_POLL. Luckily, all callers pass PHY_POLL
anyway...

[...]

MBR, Sergei

On 03/09/15 12:37, Sergei Shtylyov wrote:

On 09/03/2015 10:20 PM, Sergei Shtylyov wrote:

quoted

quoted

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 <redacted>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Grant Likely <redacted>
---
  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 e41546d..d60d875 100644
--- a/drivers/net/phy/fixed.c
+++ b/drivers/net/phy/fixed.c

[...]

quoted

@@ -203,6 +204,66 @@ err_regs:

[...]

quoted

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

    Was rummaging in the fixed_phy driver and a bug sprang right at
me: 'phy'

   Sorry, s/phy/irq/ of course. Just noticed. :-/

Ok, that makes sense then, and yes, this "irq" argument should have been
passed down to fixed_phy_add(). Might be worth adding a WARN_ON(irq !=
PHY_POLL) just to catch callers that expect something else.

Thanks!
--
Florian

On 09/03/2015 10:38 PM, Florian Fainelli wrote:

quoted

quoted

quoted

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 <redacted>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Grant Likely <redacted>
---
   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 e41546d..d60d875 100644
--- a/drivers/net/phy/fixed.c
+++ b/drivers/net/phy/fixed.c

[...]

quoted

@@ -203,6 +204,66 @@ err_regs:

[...]

quoted

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

     Was rummaging in the fixed_phy driver and a bug sprang right at
me: 'phy'

    Sorry, s/phy/irq/ of course. Just noticed. :-/

    I've reported the bug on #miplsinux, there I used the correct word. :-)

Ok, that makes sense then, and yes, this "irq" argument should have been
passed down to fixed_phy_add(). Might be worth adding a WARN_ON(irq !=
PHY_POLL) just to catch callers that expect something else.

    In-tree callers all seem to pass PHY_POLL to fixed_phy_register(). Do we 
care about out of tree stuff?

Thanks!
--
Florian

MBR, Sergei