[RFC PATCHv2 0/4] Add DT support for fixed PHYs

[RFC PATCHv2 0/4] Add DT support for fixed PHYs

[Thomas Petazzoni]

Hello,

Here is a second version of the patch set that adds a Device Tree
binding and the related code to support fixed PHYs. Marked as RFC,
this patch set is obviously not intended for merging in 3.12.

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().

The things I am not entirely happy with yet are:

 * The PHY ID is hardcoded to 0xdeadbeef. Ideally, it should be a
   properly reserved vendor/device identifier, but it isn't clear how
   to get one allocated for this purpose.

 * The fixed_phy_register() function in drivers/net/phy/fixed.c has
   some OF references. So ideally, I would have preferred to put this
   code in drivers/of/of_mdio.c, but to call get_phy_device(), we need
   a reference to the mii_bus structure that represents the fixed MDIO
   bus.

 * There is some error management missing in fixed_phy_register(), but
   it can certainly be added easily. This RFC is meant to sort out the
   general idea.

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                            | 63 ++++++++++++++++++----
 drivers/of/of_mdio.c                               | 24 +++++++++
 include/linux/of_mdio.h                            | 15 ++++++
 include/linux/phy_fixed.h                          | 11 ++++
 7 files changed, 145 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt

-- 
1.8.1.2

[RFC PATCHv2 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.1.2

[RFC PATCHv2 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   | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/phy_fixed.h | 11 +++++++++++
 2 files changed, 54 insertions(+)

diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c
index 0f02403..3f9412b 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,48 @@ err_regs:
 }
 EXPORT_SYMBOL_GPL(fixed_phy_add);
 
+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))
+		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);
+		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.1.2

[RFC PATCHv2 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 d5a57a9..0507f8f 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>
@@ -247,3 +248,26 @@ struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
 	return IS_ERR(phy) ? NULL : phy;
 }
 EXPORT_SYMBOL(of_phy_connect_fixed_link);
+
+#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 8163107..2f535ee 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -57,4 +57,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.1.2

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

[Grant Likely]

On Fri,  6 Sep 2013 17:18:20 +0200, Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Hi Thomas,

The implemenation in this series looks like it is in good shape, so I'll
restrict my comments to be binding...

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

I understand what you're trying to do here, but it causes a troublesome
leakage of implementation detail into the binding, making the whole
thing look very odd. This binding tries to make a fixed link look
exactly like a real PHY even to the point of including a phandle to the
phy. But having a phandle to a node which is *always* a direct child of
the MAC node is redundant and a rather looney. Yes, doing it that way
makes it easy for of_phy_find_device() to be transparent for fixed link,
but that should *not* drive bindings, especially when that makes the
binding really rather weird.

Second, this new binding doesn't provide anything over and above the
existing fixed-link binding. It may not be pretty, but it is
estabilshed.

That said, I do agree that the current Linux implementation is not good
because it cannot handle a fixed-link property transparently. That's a
deficiency in the Linux implementation and it should be fixed.
of_phy_connect() currently requires the phy phandle to be passed in.
Part of the reason it was done this way is that some drivers connect to
multiple 'phys'. A soulition could be to make the phy handle optional.
If it is empty then go looking for either a phy-device or fixed-link
property. Otherwise use the provided node.

g.

> +
> +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 d5a57a9..0507f8f 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>
> @@ -247,3 +248,26 @@ struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
>  	return IS_ERR(phy) ? NULL : phy;
>  }
>  EXPORT_SYMBOL(of_phy_connect_fixed_link);
> +
> +#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 8163107..2f535ee 100644
> --- a/include/linux/of_mdio.h
> +++ b/include/linux/of_mdio.h
> @@ -57,4 +57,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.1.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

[Florian Fainelli]

Hello,

2013/9/18 Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
> On Fri,  6 Sep 2013 17:18:20 +0200, Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>
> Hi Thomas,
>
> The implemenation in this series looks like it is in good shape, so I'll
> restrict my comments to be binding...
>
>> ---
>>  .../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.
>
> I understand what you're trying to do here, but it causes a troublesome
> leakage of implementation detail into the binding, making the whole
> thing look very odd. This binding tries to make a fixed link look
> exactly like a real PHY even to the point of including a phandle to the
> phy. But having a phandle to a node which is *always* a direct child of
> the MAC node is redundant and a rather looney. Yes, doing it that way
> makes it easy for of_phy_find_device() to be transparent for fixed link,
> but that should *not* drive bindings, especially when that makes the
> binding really rather weird.

This is not exactly true in the sense that the "new" binding just
re-shuffles the properties representation into something that is
clearer and more extendible but there is not much difference in the
semantics.

>
> Second, this new binding doesn't provide anything over and above the
> existing fixed-link binding. It may not be pretty, but it is
> estabilshed.

In fact it does, the old one is obscure and not easily extendable
because we rely on an integer array to represent the various
properties, so at least this new one makes it easy to extend the
binding to support a possibly new fixed-link property. Being able to
deprecate a fundamentaly badly designed binding should still be a
prerogative, software is flexible and can deal with both with little
cost.

>
> That said, I do agree that the current Linux implementation is not good
> because it cannot handle a fixed-link property transparently. That's a
> deficiency in the Linux implementation and it should be fixed.
> of_phy_connect() currently requires the phy phandle to be passed in.
> Part of the reason it was done this way is that some drivers connect to
> multiple 'phys'. A soulition could be to make the phy handle optional.
> If it is empty then go looking for either a phy-device or fixed-link
> property. Otherwise use the provided node.

I do not quite follow you on this one, and I fear we might be leaking
some Linux probing heuristic into Device Tree bindings by implicitely
saying "not including a PHY phandle means connecting to a fixed-PHY
link." This would also dramatically change the current behavior for
most drivers where they might refuse probing if no corresponding PHY
device node is present and they are not designed to connect to a fixed
PHY one.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Grant Likely]

On Wed, 18 Sep 2013 10:21:11 +0100, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hello,
> 
> 2013/9/18 Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
> > On Fri,  6 Sep 2013 17:18:20 +0200, Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> >
> > Hi Thomas,
> >
> > The implemenation in this series looks like it is in good shape, so I'll
> > restrict my comments to be binding...
> >
> >> ---
> >>  .../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.
> >
> > I understand what you're trying to do here, but it causes a troublesome
> > leakage of implementation detail into the binding, making the whole
> > thing look very odd. This binding tries to make a fixed link look
> > exactly like a real PHY even to the point of including a phandle to the
> > phy. But having a phandle to a node which is *always* a direct child of
> > the MAC node is redundant and a rather looney. Yes, doing it that way
> > makes it easy for of_phy_find_device() to be transparent for fixed link,
> > but that should *not* drive bindings, especially when that makes the
> > binding really rather weird.
> 
> This is not exactly true in the sense that the "new" binding just
> re-shuffles the properties representation into something that is
> clearer and more extendible but there is not much difference in the
> semantics.

That's not my point in the above paragraph. My point is a binding that
consists of a phandle to a node that is always a direct child is goofy
and wrong.

> > Second, this new binding doesn't provide anything over and above the
> > existing fixed-link binding. It may not be pretty, but it is
> > estabilshed.
> 
> In fact it does, the old one is obscure and not easily extendable
> because we rely on an integer array to represent the various
> properties, so at least this new one makes it easy to extend the
> binding to support a possibly new fixed-link property. Being able to
> deprecate a fundamentaly badly designed binding should still be a
> prerogative, software is flexible and can deal with both with little
> cost.

I understand that, but consistency is also important. I don't see a
proposal for a new feature for the binding in this patch. Without a
really compelling reason to change a binding that works (even if it is
opaque) I cannot agree to changing it.

> > That said, I do agree that the current Linux implementation is not good
> > because it cannot handle a fixed-link property transparently. That's a
> > deficiency in the Linux implementation and it should be fixed.
> > of_phy_connect() currently requires the phy phandle to be passed in.
> > Part of the reason it was done this way is that some drivers connect to
> > multiple 'phys'. A soulition could be to make the phy handle optional.
> > If it is empty then go looking for either a phy-device or fixed-link
> > property. Otherwise use the provided node.
> 
> I do not quite follow you on this one, and I fear we might be leaking
> some Linux probing heuristic into Device Tree bindings by implicitely
> saying "not including a PHY phandle means connecting to a fixed-PHY
> link." This would also dramatically change the current behavior for
> most drivers where they might refuse probing if no corresponding PHY
> device node is present and they are not designed to connect to a fixed
> PHY one.

Drivers we can change and fix. There aren't very may call sites
affected so I'm not overly worried about it. Also, I was making a
suggestion on how to fix it, but a suggestion is not a fully formed
patch. The issue you raise would of course need to be addressed.

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

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

[Sascha Hauer]

On Wed, Sep 18, 2013 at 10:00:31AM -0500, Grant Likely wrote:
> On Wed, 18 Sep 2013 10:21:11 +0100, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > >
> > > I understand what you're trying to do here, but it causes a troublesome
> > > leakage of implementation detail into the binding, making the whole
> > > thing look very odd. This binding tries to make a fixed link look
> > > exactly like a real PHY even to the point of including a phandle to the
> > > phy. But having a phandle to a node which is *always* a direct child of
> > > the MAC node is redundant and a rather looney. Yes, doing it that way
> > > makes it easy for of_phy_find_device() to be transparent for fixed link,
> > > but that should *not* drive bindings, especially when that makes the
> > > binding really rather weird.
> > 
> > This is not exactly true in the sense that the "new" binding just
> > re-shuffles the properties representation into something that is
> > clearer and more extendible but there is not much difference in the
> > semantics.
> 
> That's not my point in the above paragraph. My point is a binding that
> consists of a phandle to a node that is always a direct child is goofy
> and wrong.

It's not necessarily a direct child. Most of these fixed links are really
ethernet switches. These are (mostly) i2c devices which are under their
corresponding i2c bus node. Using a phandle from the ethernet MAC to
the port of a switch not only provides the link information, but also the
information to which port of the switch the MAC is connected.

Another situation is that some SoCs have a MDIO bus external to the MAC
and possibly shared for multiple ethernet MACs. This also requires a
phandle from the MAC to the MDIO bus.

So we have the situation that we need a phandle from the MAC to something
that provides a link. For consistency it would be good to always use a
phandle instead of having an inflexible 'fixed-link' property.

You're right, the binding doesn't provide anything new, but I think it
straightens things up for the future.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Thomas Petazzoni]

Dear Grant Likely,

On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote:

> I understand what you're trying to do here, but it causes a troublesome
> leakage of implementation detail into the binding, making the whole
> thing look very odd. This binding tries to make a fixed link look
> exactly like a real PHY even to the point of including a phandle to the
> phy. But having a phandle to a node which is *always* a direct child of
> the MAC node is redundant and a rather looney. Yes, doing it that way
> makes it easy for of_phy_find_device() to be transparent for fixed link,
> but that should *not* drive bindings, especially when that makes the
> binding really rather weird.
> 
> Second, this new binding doesn't provide anything over and above the
> existing fixed-link binding. It may not be pretty, but it is
> estabilshed.

Have you followed the past discussions about this patch set? Basically
the *only* feedback I received on RFCv1 is that the fixed-link property
sucks, and everybody (including the known Device Tree binding
maintainer Mark Rutland) suggested to not use the fixed-link mechanism.
See http://article.gmane.org/gmane.linux.network/276932, where Mark
said:

""
I'm not sure grouping these values together is the best way of handling
this. It's rather opaque, and inflexible for future extension.
""

So, please DT maintainers, tell me what you want. I honestly don't care
whether fixed-link or a separate node is chosen. However, I do care
about being dragged around between two solutions just because the
former DT maintainer and the new DT maintainers do not agree.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
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: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs

[Florian Fainelli]

Le mercredi 18 septembre 2013, 18:11:12 Thomas Petazzoni a écrit :
> Dear Grant Likely,
> 
> On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote:
> > I understand what you're trying to do here, but it causes a troublesome
> > leakage of implementation detail into the binding, making the whole
> > thing look very odd. This binding tries to make a fixed link look
> > exactly like a real PHY even to the point of including a phandle to the
> > phy. But having a phandle to a node which is *always* a direct child of
> > the MAC node is redundant and a rather looney. Yes, doing it that way
> > makes it easy for of_phy_find_device() to be transparent for fixed link,
> > but that should *not* drive bindings, especially when that makes the
> > binding really rather weird.
> > 
> > Second, this new binding doesn't provide anything over and above the
> > existing fixed-link binding. It may not be pretty, but it is
> > estabilshed.
> 
> Have you followed the past discussions about this patch set? Basically
> the *only* feedback I received on RFCv1 is that the fixed-link property
> sucks, and everybody (including the known Device Tree binding
> maintainer Mark Rutland) suggested to not use the fixed-link mechanism.
> See http://article.gmane.org/gmane.linux.network/276932, where Mark
> said:
> 
> ""
> I'm not sure grouping these values together is the best way of handling
> this. It's rather opaque, and inflexible for future extension.
> ""
> 
> So, please DT maintainers, tell me what you want. I honestly don't care
> whether fixed-link or a separate node is chosen. However, I do care
> about being dragged around between two solutions just because the
> former DT maintainer and the new DT maintainers do not agree.

Since I would like to move forward so I can one day use that binding in a 
real-life product, I am going to go for Mark's side which happens to be how I 
want the binding to look like.

Do we all agree that the new binding is just way better than the old one? In 
light of the recent unstable DT ABI discussion, we can still continue parsing 
the old one for the sake of compatibility.
-- 
Florian

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

[Grant Likely]

On Fri, 25 Oct 2013 05:40:57 +0100, Florian Fainelli <florian@openwrt.org> wrote:
> Le mercredi 18 septembre 2013, 18:11:12 Thomas Petazzoni a écrit :
> > Dear Grant Likely,
> > 
> > On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote:
> > > I understand what you're trying to do here, but it causes a troublesome
> > > leakage of implementation detail into the binding, making the whole
> > > thing look very odd. This binding tries to make a fixed link look
> > > exactly like a real PHY even to the point of including a phandle to the
> > > phy. But having a phandle to a node which is *always* a direct child of
> > > the MAC node is redundant and a rather looney. Yes, doing it that way
> > > makes it easy for of_phy_find_device() to be transparent for fixed link,
> > > but that should *not* drive bindings, especially when that makes the
> > > binding really rather weird.
> > > 
> > > Second, this new binding doesn't provide anything over and above the
> > > existing fixed-link binding. It may not be pretty, but it is
> > > estabilshed.
> > 
> > Have you followed the past discussions about this patch set? Basically
> > the *only* feedback I received on RFCv1 is that the fixed-link property
> > sucks, and everybody (including the known Device Tree binding
> > maintainer Mark Rutland) suggested to not use the fixed-link mechanism.
> > See http://article.gmane.org/gmane.linux.network/276932, where Mark
> > said:
> > 
> > ""
> > I'm not sure grouping these values together is the best way of handling
> > this. It's rather opaque, and inflexible for future extension.
> > ""
> > 
> > So, please DT maintainers, tell me what you want. I honestly don't care
> > whether fixed-link or a separate node is chosen. However, I do care
> > about being dragged around between two solutions just because the
> > former DT maintainer and the new DT maintainers do not agree.

I've been sleepy on this issue, which limits how much I can push. I'll
say one more thing on the issue (below) and then leave the decision up
to Mark. I trust him and he knows what he is doing.

> Since I would like to move forward so I can one day use that binding in a 
> real-life product, I am going to go for Mark's side which happens to be how I 
> want the binding to look like.
> 
> Do we all agree that the new binding is just way better than the old one? In 
> light of the recent unstable DT ABI discussion, we can still continue parsing 
> the old one for the sake of compatibility.

Regardless of what you want it to look like, does the old binding work
for your purposes? If yes then use it. The only valid reason for
creating a new binding is if the old one doesn't work for a specific
(not theoretical) use case.

g.

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

[Mark Rutland]

Hi Florian, Grant,

On Tue, Nov 12, 2013 at 12:37:46PM +0000, Grant Likely wrote:
> On Fri, 25 Oct 2013 05:40:57 +0100, Florian Fainelli <florian@openwrt.org> wrote:
> > Le mercredi 18 septembre 2013, 18:11:12 Thomas Petazzoni a écrit :
> > > Dear Grant Likely,
> > > 
> > > On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote:
> > > > I understand what you're trying to do here, but it causes a troublesome
> > > > leakage of implementation detail into the binding, making the whole
> > > > thing look very odd. This binding tries to make a fixed link look
> > > > exactly like a real PHY even to the point of including a phandle to the
> > > > phy. But having a phandle to a node which is *always* a direct child of
> > > > the MAC node is redundant and a rather looney. Yes, doing it that way
> > > > makes it easy for of_phy_find_device() to be transparent for fixed link,
> > > > but that should *not* drive bindings, especially when that makes the
> > > > binding really rather weird.
> > > > 
> > > > Second, this new binding doesn't provide anything over and above the
> > > > existing fixed-link binding. It may not be pretty, but it is
> > > > estabilshed.
> > > 
> > > Have you followed the past discussions about this patch set? Basically
> > > the *only* feedback I received on RFCv1 is that the fixed-link property
> > > sucks, and everybody (including the known Device Tree binding
> > > maintainer Mark Rutland) suggested to not use the fixed-link mechanism.
> > > See http://article.gmane.org/gmane.linux.network/276932, where Mark
> > > said:
> > > 
> > > ""
> > > I'm not sure grouping these values together is the best way of handling
> > > this. It's rather opaque, and inflexible for future extension.
> > > ""
> > > 
> > > So, please DT maintainers, tell me what you want. I honestly don't care
> > > whether fixed-link or a separate node is chosen. However, I do care
> > > about being dragged around between two solutions just because the
> > > former DT maintainer and the new DT maintainers do not agree.
> 
> I've been sleepy on this issue, which limits how much I can push. I'll
> say one more thing on the issue (below) and then leave the decision up
> to Mark. I trust him and he knows what he is doing.
> 
> > Since I would like to move forward so I can one day use that binding in a 
> > real-life product, I am going to go for Mark's side which happens to be how I 
> > want the binding to look like.
> > 
> > Do we all agree that the new binding is just way better than the old one? In 
> > light of the recent unstable DT ABI discussion, we can still continue parsing 
> > the old one for the sake of compatibility.
> 
> Regardless of what you want it to look like, does the old binding work
> for your purposes? If yes then use it. The only valid reason for
> creating a new binding is if the old one doesn't work for a specific
> (not theoretical) use case.

I think the issue here was that I am not versed in the history of all of
the existing bindings. While I'm not keen on the existing fixed-link
property and I think it should be done differently were it being created
from scratch today, as Grant has pointed out we're already supporting it
today, and adding a new binding is going to make the code handling it
more complex.

If fixed-link works for your use case today, then use fixed-link.

If we have a valid reason to create a new binding, we should. At the
moment I think the only egregious portion of the binding is the globally
unique fake PHY id, and if that causes issues we should be able to
assign IDs within Linux ignoring the values in the DT, or reorganise
things such that the arbitrary ID doesn't matter.

If there are configurations we need to support that the fixed-link
property cannot encode, then I think we should go ahead with the binding
style that you are proposing today. However, we don't need to go with it
right away, and we can continue to support fixed-link regardless.

I apologise for the lack of consistency here, and I'm sorry that I've
delayed this series for so long.

Thanks,
Mark.

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

[Florian Fainelli]

2013/11/12 Mark Rutland <mark.rutland@arm.com>:
> Hi Florian, Grant,
>
> On Tue, Nov 12, 2013 at 12:37:46PM +0000, Grant Likely wrote:
>> On Fri, 25 Oct 2013 05:40:57 +0100, Florian Fainelli <florian@openwrt.org> wrote:
>> > Le mercredi 18 septembre 2013, 18:11:12 Thomas Petazzoni a écrit :
>> > > Dear Grant Likely,
>> > >
>> > > On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote:
>> > > > I understand what you're trying to do here, but it causes a troublesome
>> > > > leakage of implementation detail into the binding, making the whole
>> > > > thing look very odd. This binding tries to make a fixed link look
>> > > > exactly like a real PHY even to the point of including a phandle to the
>> > > > phy. But having a phandle to a node which is *always* a direct child of
>> > > > the MAC node is redundant and a rather looney. Yes, doing it that way
>> > > > makes it easy for of_phy_find_device() to be transparent for fixed link,
>> > > > but that should *not* drive bindings, especially when that makes the
>> > > > binding really rather weird.
>> > > >
>> > > > Second, this new binding doesn't provide anything over and above the
>> > > > existing fixed-link binding. It may not be pretty, but it is
>> > > > estabilshed.
>> > >
>> > > Have you followed the past discussions about this patch set? Basically
>> > > the *only* feedback I received on RFCv1 is that the fixed-link property
>> > > sucks, and everybody (including the known Device Tree binding
>> > > maintainer Mark Rutland) suggested to not use the fixed-link mechanism.
>> > > See http://article.gmane.org/gmane.linux.network/276932, where Mark
>> > > said:
>> > >
>> > > ""
>> > > I'm not sure grouping these values together is the best way of handling
>> > > this. It's rather opaque, and inflexible for future extension.
>> > > ""
>> > >
>> > > So, please DT maintainers, tell me what you want. I honestly don't care
>> > > whether fixed-link or a separate node is chosen. However, I do care
>> > > about being dragged around between two solutions just because the
>> > > former DT maintainer and the new DT maintainers do not agree.
>>
>> I've been sleepy on this issue, which limits how much I can push. I'll
>> say one more thing on the issue (below) and then leave the decision up
>> to Mark. I trust him and he knows what he is doing.
>>
>> > Since I would like to move forward so I can one day use that binding in a
>> > real-life product, I am going to go for Mark's side which happens to be how I
>> > want the binding to look like.
>> >
>> > Do we all agree that the new binding is just way better than the old one? In
>> > light of the recent unstable DT ABI discussion, we can still continue parsing
>> > the old one for the sake of compatibility.
>>
>> Regardless of what you want it to look like, does the old binding work
>> for your purposes? If yes then use it. The only valid reason for
>> creating a new binding is if the old one doesn't work for a specific
>> (not theoretical) use case.
>
> I think the issue here was that I am not versed in the history of all of
> the existing bindings. While I'm not keen on the existing fixed-link
> property and I think it should be done differently were it being created
> from scratch today, as Grant has pointed out we're already supporting it
> today, and adding a new binding is going to make the code handling it
> more complex.
>
> If fixed-link works for your use case today, then use fixed-link.

Like I said in an earlier response to Thomas yesterday, it does not
cover everything, most importantly, the existing "fixed-link" property
does not allow the representation of direct MAC to MAC connections
where you still need to specify the MII connection type for it to work
properly. This is something that can be easily provided by the new
fixed PHY binding, but less easily by the existing "fixed-link"
property. We cannot extend "fixed-link" to contain a 6th integer
because "phy-mode" or "phy-connection-type"  is a string. My options
here are pretty simple:

- use the existing "fixed-link" property even though it is not nice,
it does the job
- add a supplemental "connection-type" property and parse it in my
driver, which is where it is going to be used
- go on with the new proposed Device Tree binding

>
> If we have a valid reason to create a new binding, we should. At the
> moment I think the only egregious portion of the binding is the globally
> unique fake PHY id, and if that causes issues we should be able to
> assign IDs within Linux ignoring the values in the DT, or reorganise
> things such that the arbitrary ID doesn't matter.

I think we all agreed that the PHY ID (the term should be clarified as
it tends to either mean PHY address or PHY OUI) had to be gone, and
this is what Thomas did in this serie. Whichever binding we choose, it
should be fairly easy to keep the "PHY ID" integer in the "fixed-link"
property but make the code ignore it (with possibly a big fat warning
for a transition period).

>
> If there are configurations we need to support that the fixed-link
> property cannot encode, then I think we should go ahead with the binding
> style that you are proposing today. However, we don't need to go with it
> right away, and we can continue to support fixed-link regardless.

Well, yes, the lack of "connection-type" representation is a blocker
for some hardware configurations where two Ethernet MACs are connected
one to each other. Whether this deserves a new incarnation of the
"fixed PHY" Device Tree binding is left for discussion.

>
> I apologise for the lack of consistency here, and I'm sorry that I've
> delayed this series for so long.
-- 
Florian

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

[Florian Fainelli]

Hello Thomas,

2013/9/6 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".
> +
> +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

'max-speed' might be better here to match ePAPR v1.1 (if we do care,
'speed') works for me too.

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

We also need to add a property: 'connection-type' which can be any of
'mii', 'rgmii' etc... When operating Ethernet devices with Ethernet
devices connected back to back, it might be required to configure the
Ethernet MAC with an appropriate connection type.

Note that I picked 'connection-type' here because this the ePAPR v1.1
terminology. Now the good thing is that it is a new "feature" wrt. the
old binding.

[RFC PATCHv2 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 b017818..6da1516 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -2711,10 +2711,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.1.2

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

[Florian Fainelli]

Hello Thomas,

Le vendredi 6 septembre 2013 17:18:17 Thomas Petazzoni a écrit :
> Hello,
> 
> Here is a second version of the patch set that adds a Device Tree
> binding and the related code to support fixed PHYs. Marked as RFC,
> this patch set is obviously not intended for merging in 3.12.

Thanks a lot for continuing on this work, I really like the state of it now.

> 
> 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().
> 
> The things I am not entirely happy with yet are:
> 
>  * The PHY ID is hardcoded to 0xdeadbeef. Ideally, it should be a
>    properly reserved vendor/device identifier, but it isn't clear how
>    to get one allocated for this purpose.

Right, we should try to get something better, but we obviously cannot use an 
already allocated OUI for this. Can we ask the Linux foundation or a Linux-
friendly company to allocate one maybe?

> 
>  * The fixed_phy_register() function in drivers/net/phy/fixed.c has
>    some OF references. So ideally, I would have preferred to put this
>    code in drivers/of/of_mdio.c, but to call get_phy_device(), we need
>    a reference to the mii_bus structure that represents the fixed MDIO
>    bus.

This is not a big deal, not everything in drivers/ is consistent with this, 
and making the fixed MDIO bus globally accessible does not sound too great.

> 
>  * There is some error management missing in fixed_phy_register(), but
>    it can certainly be added easily. This RFC is meant to sort out the
>    general idea.

Do you think you could add these to got beyond the RFC state? The patchset as 
it currently is fine with me if you can address these.
-- 
Florian

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

[Thomas Petazzoni]

Dear Florian Fainelli,

On Fri, 06 Sep 2013 21:42:42 +0100, Florian Fainelli wrote:

> > Here is a second version of the patch set that adds a Device Tree
> > binding and the related code to support fixed PHYs. Marked as RFC,
> > this patch set is obviously not intended for merging in 3.12.
> 
> Thanks a lot for continuing on this work, I really like the state of it now.

Thanks for your feedback.

> > 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().
> > 
> > The things I am not entirely happy with yet are:
> > 
> >  * The PHY ID is hardcoded to 0xdeadbeef. Ideally, it should be a
> >    properly reserved vendor/device identifier, but it isn't clear how
> >    to get one allocated for this purpose.
> 
> Right, we should try to get something better, but we obviously cannot use an 
> already allocated OUI for this. Can we ask the Linux foundation or a Linux-
> friendly company to allocate one maybe?

According to http://standards.ieee.org/develop/regauth/oui/oui.txt, the
Linux Foundation doesn't seem to own any OUI. Should we simply be
contacting some random companies in this list?

> >  * The fixed_phy_register() function in drivers/net/phy/fixed.c has
> >    some OF references. So ideally, I would have preferred to put this
> >    code in drivers/of/of_mdio.c, but to call get_phy_device(), we need
> >    a reference to the mii_bus structure that represents the fixed MDIO
> >    bus.
> 
> This is not a big deal, not everything in drivers/ is consistent with this, 
> and making the fixed MDIO bus globally accessible does not sound too great.

Indeed.

> >  * There is some error management missing in fixed_phy_register(), but
> >    it can certainly be added easily. This RFC is meant to sort out the
> >    general idea.
> 
> Do you think you could add these to got beyond the RFC state? The patchset as 
> it currently is fine with me if you can address these.

Sure, it shouldn't be too difficult.

In the mean time, I'm interested in hearing comments from other people,
especially from the Device Tree bindings maintainers: while the
internal implementation details can always be fixed later on, the DT
binding should obviously get an approval from the DT maintainers.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Sascha Hauer]

On Fri, Sep 06, 2013 at 05:18:17PM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> Here is a second version of the patch set that adds a Device Tree
> binding and the related code to support fixed PHYs. Marked as RFC,
> this patch set is obviously not intended for merging in 3.12.
> 
> 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().
> 
> The things I am not entirely happy with yet are:
> 
>  * The PHY ID is hardcoded to 0xdeadbeef. Ideally, it should be a
>    properly reserved vendor/device identifier, but it isn't clear how
>    to get one allocated for this purpose.
> 
>  * The fixed_phy_register() function in drivers/net/phy/fixed.c has
>    some OF references. So ideally, I would have preferred to put this
>    code in drivers/of/of_mdio.c, but to call get_phy_device(), we need
>    a reference to the mii_bus structure that represents the fixed MDIO
>    bus.
> 
>  * There is some error management missing in fixed_phy_register(), but
>    it can certainly be added easily. This RFC is meant to sort out the
>    general idea.

+1 for the general idea. This really looks good now. I've not much more
to say. Maybe someone from the devicetree corner has a few words for the
binding?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Christian Gmeiner]

Hi

>> Hello,
>>
>> Here is a second version of the patch set that adds a Device Tree
>> binding and the related code to support fixed PHYs. Marked as RFC,
>> this patch set is obviously not intended for merging in 3.12.
>>
>> 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().
>>
>> The things I am not entirely happy with yet are:
>>
>>  * The PHY ID is hardcoded to 0xdeadbeef. Ideally, it should be a
>>    properly reserved vendor/device identifier, but it isn't clear how
>>    to get one allocated for this purpose.
>>
>>  * The fixed_phy_register() function in drivers/net/phy/fixed.c has
>>    some OF references. So ideally, I would have preferred to put this
>>    code in drivers/of/of_mdio.c, but to call get_phy_device(), we need
>>    a reference to the mii_bus structure that represents the fixed MDIO
>>    bus.
>>
>>  * There is some error management missing in fixed_phy_register(), but
>>    it can certainly be added easily. This RFC is meant to sort out the
>>    general idea.
>
> +1 for the general idea. This really looks good now. I've not much more
> to say. Maybe someone from the devicetree corner has a few words for the
> binding?
>

I tested the whole series with an I.MX6D board with the FEC driver:
fec 2188000.ethernet eth0: Freescale FEC PHY driver [Generic PHY]
(mii_bus:phy_addr=fixed-0:00, irq=-1)

For me binding looks nice and I hope to see this patch series in 3.13.

Tested-by: Christian Gmeiner <christian.gmeiner-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
--
Christian Gmeiner, MSc
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Christian Gmeiner]

2013-09-25 9:12 GMT+02:00 Christian Gmeiner <christian.gmeiner-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> Hi
>
>>> Hello,
>>>
>>> Here is a second version of the patch set that adds a Device Tree
>>> binding and the related code to support fixed PHYs. Marked as RFC,
>>> this patch set is obviously not intended for merging in 3.12.
>>>
>>> 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().
>>>
>>> The things I am not entirely happy with yet are:
>>>
>>>  * The PHY ID is hardcoded to 0xdeadbeef. Ideally, it should be a
>>>    properly reserved vendor/device identifier, but it isn't clear how
>>>    to get one allocated for this purpose.
>>>
>>>  * The fixed_phy_register() function in drivers/net/phy/fixed.c has
>>>    some OF references. So ideally, I would have preferred to put this
>>>    code in drivers/of/of_mdio.c, but to call get_phy_device(), we need
>>>    a reference to the mii_bus structure that represents the fixed MDIO
>>>    bus.
>>>
>>>  * There is some error management missing in fixed_phy_register(), but
>>>    it can certainly be added easily. This RFC is meant to sort out the
>>>    general idea.
>>
>> +1 for the general idea. This really looks good now. I've not much more
>> to say. Maybe someone from the devicetree corner has a few words for the
>> binding?
>>
>
> I tested the whole series with an I.MX6D board with the FEC driver:
> fec 2188000.ethernet eth0: Freescale FEC PHY driver [Generic PHY]
> (mii_bus:phy_addr=fixed-0:00, irq=-1)
>
> For me binding looks nice and I hope to see this patch series in 3.13.
>
> Tested-by: Christian Gmeiner <christian.gmeiner-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Is there any update on this patch series?

--
Christian Gmeiner, MSc

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

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

[Thomas Petazzoni]

Dear Christian Gmeiner,

On Mon, 10 Feb 2014 11:30:30 +0100, Christian Gmeiner wrote:

> >> +1 for the general idea. This really looks good now. I've not much more
> >> to say. Maybe someone from the devicetree corner has a few words for the
> >> binding?
> >>
> >
> > I tested the whole series with an I.MX6D board with the FEC driver:
> > fec 2188000.ethernet eth0: Freescale FEC PHY driver [Generic PHY]
> > (mii_bus:phy_addr=fixed-0:00, irq=-1)
> >
> > For me binding looks nice and I hope to see this patch series in 3.13.
> >
> > Tested-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> 
> Is there any update on this patch series?

I'll try to send a new version in the next few days. It's still part of
my TODO list.

Thanks!

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