[RFC PATCH 0/3] Add DT support for fixed PHYs

[RFC PATCH 0/3] Add DT support for fixed PHYs

[Thomas Petazzoni]

Hello,

Following the discussion of last week with Florian Fainelli and Grant
Likely [1], this patch set introduces DT-based support for fixed PHYs
through an additional OF API, and uses it in the context of the
Marvell mvneta network driver.

Grant, Rob, your opinions as DT maintainers are welcome. Thanks!

Thomas

[1] http://www.spinics.net/lists/netdev/msg242766.html
    http://www.spinics.net/lists/netdev/msg243192.html

Thomas Petazzoni (3):
  of: provide a binding for the 'fixed-link' property
  net: phy: call mdiobus_scan() after adding a fixed PHY
  net: mvneta: add support for fixed links

 .../devicetree/bindings/net/fixed-link.txt         | 26 ++++++++++++++++
 .../bindings/net/marvell-armada-370-neta.txt       | 24 +++++++++++++--
 drivers/net/ethernet/marvell/mvneta.c              | 18 ++++++++---
 drivers/net/phy/fixed.c                            |  2 ++
 drivers/of/of_mdio.c                               | 36 +++++++++++++++++++---
 include/linux/of_mdio.h                            | 10 ++++++
 6 files changed, 104 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt

-- 
1.8.1.2

[RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[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", as a "fixed-link" property
of the Ethernet device Device Tree node.

This patch adds:

 * A documentation for the Device Tree property "fixed-link".

 * A of_phy_register_fixed_link() OF helper, which provided an OF node
   that contains a "fixed-link" property, registers the corresponding
   fixed PHY.

 * Removes the warning on the of_phy_connect_fixed_link() that says
   new drivers should not use it, since Grant Likely indicated that
   this "fixed-link" property is indeed the way to go.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 .../devicetree/bindings/net/fixed-link.txt         | 26 ++++++++++++++++
 drivers/of/of_mdio.c                               | 36 +++++++++++++++++++---
 include/linux/of_mdio.h                            | 10 ++++++
 3 files changed, 68 insertions(+), 4 deletions(-)
 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..25a009a
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/fixed-link.txt
@@ -0,0 +1,26 @@
+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 within an Ethernet device
+Device Tree node using a 'fixed-link' property, composed of 5
+elements:
+
+ 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in
+    the system.
+ 2. The duplex (1 for full-duplex, 0 for half-duplex)
+ 3. The speed (10, 100, 1000)
+ 4. The pause setting (1 for enabled, 0 for disabled)
+ 5. The asym pause setting (1 for enabled, 0 for disabled)
+
+Example:
+
+ethernet@0 {
+	...
+	fixed-link = <1 1 1000 0 0>;
+	...
+};
+
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index d5a57a9..66d5591 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>
@@ -215,10 +216,6 @@ EXPORT_SYMBOL(of_phy_connect);
  * @dev: pointer to net_device claiming the phy
  * @hndlr: Link state callback for the network device
  * @iface: PHY data interface type
- *
- * This function is a temporary stop-gap and will be removed soon.  It is
- * only to support the fs_enet, ucc_geth and gianfar Ethernet drivers.  Do
- * not call this function from new drivers.
  */
 struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
 					     void (*hndlr)(struct net_device *),
@@ -247,3 +244,34 @@ 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)
+/**
+ * of_phy_register_fixed_link - Parse fixed-link property and register a dummy phy
+ * @np: pointer to the OF device node that contains the "fixed-link"
+ * property for which a dummy phy should be registered.
+ */
+#define FIXED_LINK_PROPERTIES_COUNT 5
+int of_phy_register_fixed_link(struct device_node *np)
+{
+	struct fixed_phy_status status = {};
+	u32 fixed_link_props[FIXED_LINK_PROPERTIES_COUNT];
+	int ret;
+
+	ret = of_property_read_u32_array(np, "fixed-link",
+					 fixed_link_props,
+					 FIXED_LINK_PROPERTIES_COUNT);
+	if (ret < 0)
+		return ret;
+
+	status.link       = 1;
+	status.duplex     = fixed_link_props[1];
+	status.speed      = fixed_link_props[2];
+	status.pause      = fixed_link_props[3];
+	status.asym_pause = fixed_link_props[4];
+
+	return fixed_phy_add(PHY_POLL, fixed_link_props[0],
+			     &status);
+}
+EXPORT_SYMBOL(of_phy_register_fixed_link);
+#endif
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 8163107..bf6efea 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -57,4 +57,14 @@ 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);
+#else
+static inline int of_phy_register_fixed_link(struct device_node *np)
+{
+	return -ENOSYS;
+}
+#endif
+
+
 #endif /* __LINUX_OF_MDIO_H */
-- 
1.8.1.2

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Florian Fainelli]

Hello Thomas,

2013/7/15 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", as a "fixed-link" property
> of the Ethernet device Device Tree node.
>
> This patch adds:
>
>  * A documentation for the Device Tree property "fixed-link".
>
>  * A of_phy_register_fixed_link() OF helper, which provided an OF node
>    that contains a "fixed-link" property, registers the corresponding
>    fixed PHY.
>
>  * Removes the warning on the of_phy_connect_fixed_link() that says
>    new drivers should not use it, since Grant Likely indicated that
>    this "fixed-link" property is indeed the way to go.

Some minor nitpicks below. Other than that:

>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Reviewed-by: Florian Fainelli <florian@openwrt.org>

> ---
>  .../devicetree/bindings/net/fixed-link.txt         | 26 ++++++++++++++++
>  drivers/of/of_mdio.c                               | 36 +++++++++++++++++++---
>  include/linux/of_mdio.h                            | 10 ++++++
>  3 files changed, 68 insertions(+), 4 deletions(-)
>  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..25a009a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt
> @@ -0,0 +1,26 @@
> +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".

You could also add the following:

"this might be the case when using an Ethernet switch whose management
interface is not accessible using the MDIO bus, or for specific PHYs
where the control path of the PHY might be separated from its data
path".

> +
> +Such a fixed link situation is described within an Ethernet device
> +Device Tree node using a 'fixed-link' property, composed of 5
> +elements:
> +
> + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in
> +    the system.

You might want to detail that will be used to fake PHY identification
registers 2 & 3.

> + 2. The duplex (1 for full-duplex, 0 for half-duplex)
> + 3. The speed (10, 100, 1000)
> + 4. The pause setting (1 for enabled, 0 for disabled)
> + 5. The asym pause setting (1 for enabled, 0 for disabled)
> +
> +Example:
> +
> +ethernet@0 {
> +       ...
> +       fixed-link = <1 1 1000 0 0>;
> +       ...
> +};
> +
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index d5a57a9..66d5591 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>
> @@ -215,10 +216,6 @@ EXPORT_SYMBOL(of_phy_connect);
>   * @dev: pointer to net_device claiming the phy
>   * @hndlr: Link state callback for the network device
>   * @iface: PHY data interface type
> - *
> - * This function is a temporary stop-gap and will be removed soon.  It is
> - * only to support the fs_enet, ucc_geth and gianfar Ethernet drivers.  Do
> - * not call this function from new drivers.
>   */
>  struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
>                                              void (*hndlr)(struct net_device *),
> @@ -247,3 +244,34 @@ 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)
> +/**
> + * of_phy_register_fixed_link - Parse fixed-link property and register a dummy phy
> + * @np: pointer to the OF device node that contains the "fixed-link"
> + * property for which a dummy phy should be registered.
> + */
> +#define FIXED_LINK_PROPERTIES_COUNT 5
> +int of_phy_register_fixed_link(struct device_node *np)
> +{
> +       struct fixed_phy_status status = {};
> +       u32 fixed_link_props[FIXED_LINK_PROPERTIES_COUNT];
> +       int ret;
> +
> +       ret = of_property_read_u32_array(np, "fixed-link",
> +                                        fixed_link_props,
> +                                        FIXED_LINK_PROPERTIES_COUNT);
> +       if (ret < 0)
> +               return ret;
> +
> +       status.link       = 1;
> +       status.duplex     = fixed_link_props[1];
> +       status.speed      = fixed_link_props[2];
> +       status.pause      = fixed_link_props[3];
> +       status.asym_pause = fixed_link_props[4];
> +
> +       return fixed_phy_add(PHY_POLL, fixed_link_props[0],
> +                            &status);
> +}
> +EXPORT_SYMBOL(of_phy_register_fixed_link);
> +#endif
> diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
> index 8163107..bf6efea 100644
> --- a/include/linux/of_mdio.h
> +++ b/include/linux/of_mdio.h
> @@ -57,4 +57,14 @@ 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);
> +#else
> +static inline int of_phy_register_fixed_link(struct device_node *np)
> +{
> +       return -ENOSYS;
> +}
> +#endif
> +
> +
>  #endif /* __LINUX_OF_MDIO_H */
> --
> 1.8.1.2
>



--
Florian

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Mark Rutland]

Hi Thomas,

On Mon, Jul 15, 2013 at 04:34:08PM +0100, Thomas Petazzoni 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", as a "fixed-link" property
> of the Ethernet device Device Tree node.
> 
> This patch adds:
> 
>  * A documentation for the Device Tree property "fixed-link".
> 
>  * A of_phy_register_fixed_link() OF helper, which provided an OF node
>    that contains a "fixed-link" property, registers the corresponding
>    fixed PHY.
> 
>  * Removes the warning on the of_phy_connect_fixed_link() that says
>    new drivers should not use it, since Grant Likely indicated that
>    this "fixed-link" property is indeed the way to go.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  .../devicetree/bindings/net/fixed-link.txt         | 26 ++++++++++++++++
>  drivers/of/of_mdio.c                               | 36 +++++++++++++++++++---
>  include/linux/of_mdio.h                            | 10 ++++++
>  3 files changed, 68 insertions(+), 4 deletions(-)
>  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..25a009a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt
> @@ -0,0 +1,26 @@
> +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".

Are partictular MACs fixed link, or can some either be either fixed link
or wired to an MDIO-managed PHY? i.e. can we assume a given MAC is
fixed-link from its compatible string?

> +
> +Such a fixed link situation is described within an Ethernet device
> +Device Tree node using a 'fixed-link' property, composed of 5
> +elements:

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

> +
> + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in
> +    the system.

Is there any reason this couldn't be allocated dynamically within the
kernel as needed? I don't see why an arbitrary unique value should be a
dt binding requirement; it seems like a leak of Linux implementation
details.

> + 2. The duplex (1 for full-duplex, 0 for half-duplex)

Will this change for a given MAC?

Could we not have a boolean property for each of these, and require one
to be present?

Possibly fixed-link-full-duplex / fix-link-half-duplex?

> + 3. The speed (10, 100, 1000)

fixed-link-speed?

> + 4. The pause setting (1 for enabled, 0 for disabled)
> + 5. The asym pause setting (1 for enabled, 0 for disabled)

Boolean properties for both of these?

Thanks,
Mark.

> +
> +Example:
> +
> +ethernet@0 {
> +	...
> +	fixed-link = <1 1 1000 0 0>;
> +	...
> +};
> +
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index d5a57a9..66d5591 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>
> @@ -215,10 +216,6 @@ EXPORT_SYMBOL(of_phy_connect);
>   * @dev: pointer to net_device claiming the phy
>   * @hndlr: Link state callback for the network device
>   * @iface: PHY data interface type
> - *
> - * This function is a temporary stop-gap and will be removed soon.  It is
> - * only to support the fs_enet, ucc_geth and gianfar Ethernet drivers.  Do
> - * not call this function from new drivers.
>   */
>  struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
>  					     void (*hndlr)(struct net_device *),
> @@ -247,3 +244,34 @@ 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)
> +/**
> + * of_phy_register_fixed_link - Parse fixed-link property and register a dummy phy
> + * @np: pointer to the OF device node that contains the "fixed-link"
> + * property for which a dummy phy should be registered.
> + */
> +#define FIXED_LINK_PROPERTIES_COUNT 5
> +int of_phy_register_fixed_link(struct device_node *np)
> +{
> +	struct fixed_phy_status status = {};
> +	u32 fixed_link_props[FIXED_LINK_PROPERTIES_COUNT];
> +	int ret;
> +
> +	ret = of_property_read_u32_array(np, "fixed-link",
> +					 fixed_link_props,
> +					 FIXED_LINK_PROPERTIES_COUNT);
> +	if (ret < 0)
> +		return ret;
> +
> +	status.link       = 1;
> +	status.duplex     = fixed_link_props[1];
> +	status.speed      = fixed_link_props[2];
> +	status.pause      = fixed_link_props[3];
> +	status.asym_pause = fixed_link_props[4];
> +
> +	return fixed_phy_add(PHY_POLL, fixed_link_props[0],
> +			     &status);
> +}
> +EXPORT_SYMBOL(of_phy_register_fixed_link);
> +#endif
> diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
> index 8163107..bf6efea 100644
> --- a/include/linux/of_mdio.h
> +++ b/include/linux/of_mdio.h
> @@ -57,4 +57,14 @@ 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);
> +#else
> +static inline int of_phy_register_fixed_link(struct device_node *np)
> +{
> +	return -ENOSYS;
> +}
> +#endif
> +
> +
>  #endif /* __LINUX_OF_MDIO_H */
> -- 
> 1.8.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Florian Fainelli]

Hello,

2013/7/23 Mark Rutland <mark.rutland@arm.com>:
[snip]

>> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt
>> @@ -0,0 +1,26 @@
>> +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".
>
> Are partictular MACs fixed link, or can some either be either fixed link
> or wired to an MDIO-managed PHY? i.e. can we assume a given MAC is
> fixed-link from its compatible string?

There are different use-cases out there:

- you lack or do not want to have a proper switch/PHY library driver
for an Ethernet switch, but that specific switch may still be
accessible on the MDIO bus, yet you just want to expose a link UP from
the CPU port perspective, that specific use case should dissapear in
favor of a simplified PHY library driver which just eventually makes
the link appear as UP from the CPU port perspective

- you have absolutely no control over the PHY on the MDIO bus
        * for switches, it could be SPI, I2C, GPIO whatever, so to
avoid any dependency, you might just want to let the link appear as UP
        * or you could have a very different PHY whose data-path is
Ethernet, but the control-path is something else, e.g: MoCA (which is
more the kind of use-case I am interested in)

>
>> +
>> +Such a fixed link situation is described within an Ethernet device
>> +Device Tree node using a 'fixed-link' property, composed of 5
>> +elements:
>
> I'm not sure grouping these values together is the best way of handling
> this. It's rather opaque, and inflexible for future extension.

Well, I proposed making them look like a "standard" Ethernet PHY node
but this somehow got rejected, see at the very bottom.

>
>> +
>> + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in
>> +    the system.
>
> Is there any reason this couldn't be allocated dynamically within the
> kernel as needed? I don't see why an arbitrary unique value should be a
> dt binding requirement; it seems like a leak of Linux implementation
> details.

Honestly, I do not really think actually letting this be configurable
is a good idea because it would become some sort of user-space ABI. If
we want to expose some PHY identification registers, we could use some
ID2 and ID3 values which are not allocated to any vendor, or allocated
to the Linux foundation (e.g; like what USB does with Linux HUBs).

>
>> + 2. The duplex (1 for full-duplex, 0 for half-duplex)
>
> Will this change for a given MAC?
>
> Could we not have a boolean property for each of these, and require one
> to be present?
>
> Possibly fixed-link-full-duplex / fix-link-half-duplex?
>
>> + 3. The speed (10, 100, 1000)
>
> fixed-link-speed?
>
>> + 4. The pause setting (1 for enabled, 0 for disabled)
>> + 5. The asym pause setting (1 for enabled, 0 for disabled)
>
> Boolean properties for both of these?
>
> Thanks,
> Mark.
>
>> +
>> +Example:
>> +
>> +ethernet@0 {
>> +     ...
>> +     fixed-link = <1 1 1000 0 0>;
>> +     ...
>> +};
>> +

The initial proposal that I had suggested was this:

ethernet-phy@0 {
     reg = <0>;
     id = <0xdeadbeef>;
     speed = <1000>;
     full-duplex;
     pause;
     asym-pause;
};

which looks more or less what you seem to want as well but this
somehow makes it look like a real PHY device, so we could argue that
this has nowhere to be in the Device Tree. Arguably there are a
gazillions of other bindings which imho should also not be in a Device
Tree.
--
Florian

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Thomas Petazzoni]

Dear Mark Rutland,

On Tue, 23 Jul 2013 12:22:59 +0100, Mark Rutland 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".
> 
> Are partictular MACs fixed link, or can some either be either fixed link
> or wired to an MDIO-managed PHY? i.e. can we assume a given MAC is
> fixed-link from its compatible string?

No, you can't. The case that I have is that the mvneta Ethernet MAC
(of Marvell 370/XP SOCs) is sometimes used with a MDIO-managed PHY, and
sometimes used with a switch that isn't manageable at all and should be
considered as a fixed link.

So no, the compatible string of the Ethernet MAC cannot be used to
determine whether we're wired fixed link or to a classical MDIO-managed
PHY.

> > +Such a fixed link situation is described within an Ethernet device
> > +Device Tree node using a 'fixed-link' property, composed of 5
> > +elements:
> 
> I'm not sure grouping these values together is the best way of handling
> this. It's rather opaque, and inflexible for future extension.

That's the DT binding that has been used by PowerPC platforms since
several years, and I've simply re-used it. See 'git grep fixed-link
arch/powerpc/boot/dts'.

I have nothing against creating another DT binding, but for a start, I
thought using existing bindings would be the best idea.

> > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in
> > +    the system.
> 
> Is there any reason this couldn't be allocated dynamically within the
> kernel as needed? I don't see why an arbitrary unique value should be a
> dt binding requirement; it seems like a leak of Linux implementation
> details.

As I pointed out in my reply to Grant, this value is used both as the
PHY address on the fake fixed MDIO bus, and as the PHY identifier as
reported by MII registers PHYS_ID1 and PHYS_ID2. If we can get assigned
a proper PHY identifier that is used statically by the driver (it
doesn't have to be different per fixed PHY instance), then we can
allocate the PHY address dynamically. It requires a little bit of API
changes but that's certainly doable. See my reply to Grant for a
proposal about this.

> > + 2. The duplex (1 for full-duplex, 0 for half-duplex)
> 
> Will this change for a given MAC?
> 
> Could we not have a boolean property for each of these, and require one
> to be present?
> 
> Possibly fixed-link-full-duplex / fix-link-half-duplex?
> 
> > + 3. The speed (10, 100, 1000)
> 
> fixed-link-speed?
> 
> > + 4. The pause setting (1 for enabled, 0 for disabled)
> > + 5. The asym pause setting (1 for enabled, 0 for disabled)
> 
> Boolean properties for both of these?

As Florian already answered, he already proposed something like this in
the past, and it was rejected because a fixed PHY is not a piece of
hardware and should therefore not be represented in the Device Tree.
However, the fact that the MAC is not connected to a MDIO-manageable
PHY but to some fixed-link thing is a property of the MAC hardware
layout, and can therefore be expressed as a property of the MAC
hardware.

See the thread that starts at
http://article.gmane.org/gmane.linux.network/275771, and specifically
Grant answers to Florian suggestions of having DT nodes to represent
fixed PHY: http://article.gmane.org/gmane.linux.network/276208. Grant's
answer was:

"""
I think this discussion is going in the wrong direction. The concept
of a dummy phy is really a Linux kernel internal detail. Creating some
kind of dummy MDIO bus node does not describe the hardware. There is
already support in the kernel for Ethernet MACs connected directly to
a switch or other device. It is far better to describe how the MAC
needs to be configured than to invent a non-existent phy. Search for
"fixed-link" in the kernel tree to see how it is used.
"""

Best regards,

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

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Florian Fainelli]

Hello Thomas,

2013/7/30 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>:
> Dear Mark Rutland,
>
> On Tue, 23 Jul 2013 12:22:59 +0100, Mark Rutland 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".
>>
>> Are partictular MACs fixed link, or can some either be either fixed link
>> or wired to an MDIO-managed PHY? i.e. can we assume a given MAC is
>> fixed-link from its compatible string?
>
> No, you can't. The case that I have is that the mvneta Ethernet MAC
> (of Marvell 370/XP SOCs) is sometimes used with a MDIO-managed PHY, and
> sometimes used with a switch that isn't manageable at all and should be
> considered as a fixed link.
>
> So no, the compatible string of the Ethernet MAC cannot be used to
> determine whether we're wired fixed link or to a classical MDIO-managed
> PHY.
>
>> > +Such a fixed link situation is described within an Ethernet device
>> > +Device Tree node using a 'fixed-link' property, composed of 5
>> > +elements:
>>
>> I'm not sure grouping these values together is the best way of handling
>> this. It's rather opaque, and inflexible for future extension.
>
> That's the DT binding that has been used by PowerPC platforms since
> several years, and I've simply re-used it. See 'git grep fixed-link
> arch/powerpc/boot/dts'.
>
> I have nothing against creating another DT binding, but for a start, I
> thought using existing bindings would be the best idea.
>
>> > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in
>> > +    the system.
>>
>> Is there any reason this couldn't be allocated dynamically within the
>> kernel as needed? I don't see why an arbitrary unique value should be a
>> dt binding requirement; it seems like a leak of Linux implementation
>> details.
>
> As I pointed out in my reply to Grant, this value is used both as the
> PHY address on the fake fixed MDIO bus, and as the PHY identifier as
> reported by MII registers PHYS_ID1 and PHYS_ID2. If we can get assigned
> a proper PHY identifier that is used statically by the driver (it
> doesn't have to be different per fixed PHY instance), then we can
> allocate the PHY address dynamically. It requires a little bit of API
> changes but that's certainly doable. See my reply to Grant for a
> proposal about this.
>
>> > + 2. The duplex (1 for full-duplex, 0 for half-duplex)
>>
>> Will this change for a given MAC?
>>
>> Could we not have a boolean property for each of these, and require one
>> to be present?
>>
>> Possibly fixed-link-full-duplex / fix-link-half-duplex?
>>
>> > + 3. The speed (10, 100, 1000)
>>
>> fixed-link-speed?
>>
>> > + 4. The pause setting (1 for enabled, 0 for disabled)
>> > + 5. The asym pause setting (1 for enabled, 0 for disabled)
>>
>> Boolean properties for both of these?
>
> As Florian already answered, he already proposed something like this in
> the past, and it was rejected because a fixed PHY is not a piece of
> hardware and should therefore not be represented in the Device Tree.
> However, the fact that the MAC is not connected to a MDIO-manageable
> PHY but to some fixed-link thing is a property of the MAC hardware
> layout, and can therefore be expressed as a property of the MAC
> hardware.

True, what I *did* not like about this "fixed-link" 5 integer property
is that it does not present a consistent view of a PHY device and puts
some properties in the Ethernet MAC node, while some other are in the
Ethernet PHY node. From a hardware perspective, the Ethernet MAC and
the Ethernet PHY are two pieces of hardware, so both should get their
own node.

That said, since "fixed PHY" devices are not connected on the MDIO
bus, they cannot be represented as Ethernet PHY nodes as leafs of a
parent MDIO bus node, so maybe what we could do is having the fixed
PHY nodes child nodes of the Ethernet MAC but still make them look
like a "relatively" conventional Ethernet PHY node?

>
> See the thread that starts at
> http://article.gmane.org/gmane.linux.network/275771, and specifically
> Grant answers to Florian suggestions of having DT nodes to represent
> fixed PHY: http://article.gmane.org/gmane.linux.network/276208. Grant's
> answer was:
>
> """
> I think this discussion is going in the wrong direction. The concept
> of a dummy phy is really a Linux kernel internal detail. Creating some
> kind of dummy MDIO bus node does not describe the hardware. There is
> already support in the kernel for Ethernet MACs connected directly to
> a switch or other device. It is far better to describe how the MAC
> needs to be configured than to invent a non-existent phy. Search for
> "fixed-link" in the kernel tree to see how it is used.
> """
>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com



-- 
Florian

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Mark Rutland]

Hi Thomas,

On Tue, Jul 30, 2013 at 10:16:04AM +0100, Thomas Petazzoni wrote:
> Dear Mark Rutland,
> 
> On Tue, 23 Jul 2013 12:22:59 +0100, Mark Rutland 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".
> > 
> > Are partictular MACs fixed link, or can some either be either fixed link
> > or wired to an MDIO-managed PHY? i.e. can we assume a given MAC is
> > fixed-link from its compatible string?
> 
> No, you can't. The case that I have is that the mvneta Ethernet MAC
> (of Marvell 370/XP SOCs) is sometimes used with a MDIO-managed PHY, and
> sometimes used with a switch that isn't manageable at all and should be
> considered as a fixed link.
> 
> So no, the compatible string of the Ethernet MAC cannot be used to
> determine whether we're wired fixed link or to a classical MDIO-managed
> PHY.

Ok. Thanks for the info.

> 
> > > +Such a fixed link situation is described within an Ethernet device
> > > +Device Tree node using a 'fixed-link' property, composed of 5
> > > +elements:
> > 
> > I'm not sure grouping these values together is the best way of handling
> > this. It's rather opaque, and inflexible for future extension.
> 
> That's the DT binding that has been used by PowerPC platforms since
> several years, and I've simply re-used it. See 'git grep fixed-link
> arch/powerpc/boot/dts'.
> 
> I have nothing against creating another DT binding, but for a start, I
> thought using existing bindings would be the best idea.

Sorry, I was not aware that was the case. I agree that reusing the
existing binding is the right place to start.

> 
> > > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in
> > > +    the system.
> > 
> > Is there any reason this couldn't be allocated dynamically within the
> > kernel as needed? I don't see why an arbitrary unique value should be a
> > dt binding requirement; it seems like a leak of Linux implementation
> > details.
> 
> As I pointed out in my reply to Grant, this value is used both as the
> PHY address on the fake fixed MDIO bus, and as the PHY identifier as
> reported by MII registers PHYS_ID1 and PHYS_ID2. If we can get assigned
> a proper PHY identifier that is used statically by the driver (it
> doesn't have to be different per fixed PHY instance), then we can
> allocate the PHY address dynamically. It requires a little bit of API
> changes but that's certainly doable. See my reply to Grant for a
> proposal about this.

Ok. I must admit to not knowing enough about ethernet hardware to fully
understand the implications here for the MII registers, but otherwise
your proposal makes sense.

> 
> > > + 2. The duplex (1 for full-duplex, 0 for half-duplex)
> > 
> > Will this change for a given MAC?
> > 
> > Could we not have a boolean property for each of these, and require one
> > to be present?
> > 
> > Possibly fixed-link-full-duplex / fix-link-half-duplex?
> > 
> > > + 3. The speed (10, 100, 1000)
> > 
> > fixed-link-speed?
> > 
> > > + 4. The pause setting (1 for enabled, 0 for disabled)
> > > + 5. The asym pause setting (1 for enabled, 0 for disabled)
> > 
> > Boolean properties for both of these?
> 
> As Florian already answered, he already proposed something like this in
> the past, and it was rejected because a fixed PHY is not a piece of
> hardware and should therefore not be represented in the Device Tree.
> However, the fact that the MAC is not connected to a MDIO-manageable
> PHY but to some fixed-link thing is a property of the MAC hardware
> layout, and can therefore be expressed as a property of the MAC
> hardware.

I think it would certainly make sense to describe this as a property of
the MAC.

Thanks,
Mark.

> 
> See the thread that starts at
> http://article.gmane.org/gmane.linux.network/275771, and specifically
> Grant answers to Florian suggestions of having DT nodes to represent
> fixed PHY: http://article.gmane.org/gmane.linux.network/276208. Grant's
> answer was:
> 
> """
> I think this discussion is going in the wrong direction. The concept
> of a dummy phy is really a Linux kernel internal detail. Creating some
> kind of dummy MDIO bus node does not describe the hardware. There is
> already support in the kernel for Ethernet MACs connected directly to
> a switch or other device. It is far better to describe how the MAC
> needs to be configured than to invent a non-existent phy. Search for
> "fixed-link" in the kernel tree to see how it is used.
> """
> 
> Best regards,
> 
> Thomas
> -- 
> Thomas Petazzoni, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com
> 

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Grant Likely]

On Mon, Jul 15, 2013 at 4:34 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Some Ethernet MACs have a "fixed link", and are not connected to a
> normal MDIO-managed PHY device. For those situations, a Device Tree
> binding allows to describe a "fixed link", as a "fixed-link" property
> of the Ethernet device Device Tree node.
>
> This patch adds:
>
>  * A documentation for the Device Tree property "fixed-link".
>
>  * A of_phy_register_fixed_link() OF helper, which provided an OF node
>    that contains a "fixed-link" property, registers the corresponding
>    fixed PHY.
>
>  * Removes the warning on the of_phy_connect_fixed_link() that says
>    new drivers should not use it, since Grant Likely indicated that
>    this "fixed-link" property is indeed the way to go.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  .../devicetree/bindings/net/fixed-link.txt         | 26 ++++++++++++++++
>  drivers/of/of_mdio.c                               | 36 +++++++++++++++++++---
>  include/linux/of_mdio.h                            | 10 ++++++
>  3 files changed, 68 insertions(+), 4 deletions(-)
>  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..25a009a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt
> @@ -0,0 +1,26 @@
> +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 within an Ethernet device
> +Device Tree node using a 'fixed-link' property, composed of 5
> +elements:
> +
> + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in
> +    the system.

That's just loony!  :)  Regardless of existing code doing this, it is
absolutely ridiculous to have it in the driver. The kernel should
handle generating a phy id transparently. I'd rather mark this field
as reserved in the binding and change the code to not care about it
anymore.

g.

> + 2. The duplex (1 for full-duplex, 0 for half-duplex)
> + 3. The speed (10, 100, 1000)
> + 4. The pause setting (1 for enabled, 0 for disabled)
> + 5. The asym pause setting (1 for enabled, 0 for disabled)
> +
> +Example:
> +
> +ethernet@0 {
> +       ...
> +       fixed-link = <1 1 1000 0 0>;
> +       ...
> +};
> +
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index d5a57a9..66d5591 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>
> @@ -215,10 +216,6 @@ EXPORT_SYMBOL(of_phy_connect);
>   * @dev: pointer to net_device claiming the phy
>   * @hndlr: Link state callback for the network device
>   * @iface: PHY data interface type
> - *
> - * This function is a temporary stop-gap and will be removed soon.  It is
> - * only to support the fs_enet, ucc_geth and gianfar Ethernet drivers.  Do
> - * not call this function from new drivers.
>   */
>  struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
>                                              void (*hndlr)(struct net_device *),
> @@ -247,3 +244,34 @@ 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)
> +/**
> + * of_phy_register_fixed_link - Parse fixed-link property and register a dummy phy
> + * @np: pointer to the OF device node that contains the "fixed-link"
> + * property for which a dummy phy should be registered.
> + */
> +#define FIXED_LINK_PROPERTIES_COUNT 5
> +int of_phy_register_fixed_link(struct device_node *np)
> +{
> +       struct fixed_phy_status status = {};
> +       u32 fixed_link_props[FIXED_LINK_PROPERTIES_COUNT];
> +       int ret;
> +
> +       ret = of_property_read_u32_array(np, "fixed-link",
> +                                        fixed_link_props,
> +                                        FIXED_LINK_PROPERTIES_COUNT);
> +       if (ret < 0)
> +               return ret;
> +
> +       status.link       = 1;
> +       status.duplex     = fixed_link_props[1];
> +       status.speed      = fixed_link_props[2];
> +       status.pause      = fixed_link_props[3];
> +       status.asym_pause = fixed_link_props[4];
> +
> +       return fixed_phy_add(PHY_POLL, fixed_link_props[0],
> +                            &status);
> +}
> +EXPORT_SYMBOL(of_phy_register_fixed_link);
> +#endif
> diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
> index 8163107..bf6efea 100644
> --- a/include/linux/of_mdio.h
> +++ b/include/linux/of_mdio.h
> @@ -57,4 +57,14 @@ 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);
> +#else
> +static inline int of_phy_register_fixed_link(struct device_node *np)
> +{
> +       return -ENOSYS;
> +}
> +#endif
> +
> +
>  #endif /* __LINUX_OF_MDIO_H */
> --
> 1.8.1.2
>

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Thomas Petazzoni]

Dear Grant Likely,

On Tue, 23 Jul 2013 12:39:52 +0100, Grant Likely wrote:

> > +Such a fixed link situation is described within an Ethernet device
> > +Device Tree node using a 'fixed-link' property, composed of 5
> > +elements:
> > +
> > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in
> > +    the system.
> 
> That's just loony!  :)  Regardless of existing code doing this, it is
> absolutely ridiculous to have it in the driver. The kernel should
> handle generating a phy id transparently. I'd rather mark this field
> as reserved in the binding and change the code to not care about it
> anymore.

In fact, this value is used for two things:

 * As the PHY address on the fake "fixed" MDIO bus.

 * As the PHY identifier, as reported by the MII registers PHYS_ID1
   (0x2) and PHYS_ID2 (0x3).

I think this doesn't make sense, because the two things are completely
unrelated. Ideally, we'd like the PHY identifier for fixed PHYs to be
something fixed, identical for all fixed PHYs. The problem is finding
an OUI and device number that is available for that, but maybe we can
ask the OpenMoko people to allocate one (see
http://wiki.openmoko.org/wiki/OUI).

Then, the PHY address could be generated dynamically. This would
require:

 * Adding a fixed_phy_create() function that internally uses
   fixed_phy_add(), but before that creates an unique PHY address for
   this newly created PHY. Those unique addresses will be generated by
   incrementing a global number of fixed PHYs, up to PHY_MAX_ADDR,
   which is the maximum number of fixed PHYs that can anyway be
   registered on the fixed MDIO bus.

   fixed_phy_create() would return this PHY address (positive) on
   success, or a negative error code on failure.

 * Change of_phy_register_fixed_link() to call fixed_phy_create()
   instead of fixed_phy_add() and make it return the PHY address
   allocated by fixed_phy_create().

 * Add a of_phy_connect_fixed_link_direct() that is similar to
   of_phy_connect_fixed_link() but takes an additional PHY address as
   argument and uses that to generate the 'bus_id' used to find the
   phy_device.

Grant, Mark, Florian, do you have other proposals?

Thanks,

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

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Florian Fainelli]

Hello Thomas,

2013/7/30 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>:
> Dear Grant Likely,
>
> On Tue, 23 Jul 2013 12:39:52 +0100, Grant Likely wrote:
>
>> > +Such a fixed link situation is described within an Ethernet device
>> > +Device Tree node using a 'fixed-link' property, composed of 5
>> > +elements:
>> > +
>> > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in
>> > +    the system.
>>
>> That's just loony!  :)  Regardless of existing code doing this, it is
>> absolutely ridiculous to have it in the driver. The kernel should
>> handle generating a phy id transparently. I'd rather mark this field
>> as reserved in the binding and change the code to not care about it
>> anymore.
>
> In fact, this value is used for two things:
>
>  * As the PHY address on the fake "fixed" MDIO bus.
>
>  * As the PHY identifier, as reported by the MII registers PHYS_ID1
>    (0x2) and PHYS_ID2 (0x3).

Right, so I would start with disambiguating the two and just forget
about PHYS_ID1 and PHYS_ID2 for the moment since they probably do not
need to be per-PHY configurable.

>
> I think this doesn't make sense, because the two things are completely
> unrelated. Ideally, we'd like the PHY identifier for fixed PHYs to be
> something fixed, identical for all fixed PHYs. The problem is finding
> an OUI and device number that is available for that, but maybe we can
> ask the OpenMoko people to allocate one (see
> http://wiki.openmoko.org/wiki/OUI).

Well that would be ideal indeed, but I am wondering if we cannot just
go with some kind of magic value here anyway, regardless of
allocations since this is not a real hardware device. How about the
Linux Foundation? Is not that the same problem as with gadget USB
devices which should have some sort of real MAC address for instance?

>
> Then, the PHY address could be generated dynamically. This would
> require:
>
>  * Adding a fixed_phy_create() function that internally uses
>    fixed_phy_add(), but before that creates an unique PHY address for
>    this newly created PHY. Those unique addresses will be generated by
>    incrementing a global number of fixed PHYs, up to PHY_MAX_ADDR,
>    which is the maximum number of fixed PHYs that can anyway be
>    registered on the fixed MDIO bus.

Even though these are purely software PHY implementations, I believe
that some sort of predictability would be welcome, so I would just use
the phy_id argument passed to fixed_phy_add() as the address on the
fixed MDIO bus like it is today.

>
>    fixed_phy_create() would return this PHY address (positive) on
>    success, or a negative error code on failure.
>
>  * Change of_phy_register_fixed_link() to call fixed_phy_create()
>    instead of fixed_phy_add() and make it return the PHY address
>    allocated by fixed_phy_create().
>
>  * Add a of_phy_connect_fixed_link_direct() that is similar to
>    of_phy_connect_fixed_link() but takes an additional PHY address as
>    argument and uses that to generate the 'bus_id' used to find the
>    phy_device.
>
> Grant, Mark, Florian, do you have other proposals?

To sum up, let's just forget about the misuse of phy_id to fill in
PHYS_ID1 and PHYS_ID2 registers and keep the existing code with a
clear note that phy_id means the "PHY address on the fixed MDIO bus".
--
Florian

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Thomas Petazzoni]

Dear Florian Fainelli,

On Tue, 30 Jul 2013 11:05:04 +0100, Florian Fainelli wrote:

> > In fact, this value is used for two things:
> >
> >  * As the PHY address on the fake "fixed" MDIO bus.
> >
> >  * As the PHY identifier, as reported by the MII registers PHYS_ID1
> >    (0x2) and PHYS_ID2 (0x3).
> 
> Right, so I would start with disambiguating the two and just forget
> about PHYS_ID1 and PHYS_ID2 for the moment since they probably do not
> need to be per-PHY configurable.

Agreed.

> > I think this doesn't make sense, because the two things are completely
> > unrelated. Ideally, we'd like the PHY identifier for fixed PHYs to be
> > something fixed, identical for all fixed PHYs. The problem is finding
> > an OUI and device number that is available for that, but maybe we can
> > ask the OpenMoko people to allocate one (see
> > http://wiki.openmoko.org/wiki/OUI).
> 
> Well that would be ideal indeed, but I am wondering if we cannot just
> go with some kind of magic value here anyway, regardless of
> allocations since this is not a real hardware device.

Well, isn't that exactly what I'm proposing? Have a fixed value for
PHYS_ID1 and PHYS_ID2, that is used for /all/ fixed PHYs.

> How about the Linux Foundation?

I am concerned about the time it would take to get a new OUI
attributed. The OpenMoko OUI is said to be open for free and open
source projects, I think the Linux kernel qualifies :)

> Is not that the same problem as with gadget USB
> devices which should have some sort of real MAC address for instance?

They are either provided by the user or generated randomly, as far as I
can see.

> > Then, the PHY address could be generated dynamically. This would
> > require:
> >
> >  * Adding a fixed_phy_create() function that internally uses
> >    fixed_phy_add(), but before that creates an unique PHY address for
> >    this newly created PHY. Those unique addresses will be generated by
> >    incrementing a global number of fixed PHYs, up to PHY_MAX_ADDR,
> >    which is the maximum number of fixed PHYs that can anyway be
> >    registered on the fixed MDIO bus.
> 
> Even though these are purely software PHY implementations, I believe
> that some sort of predictability would be welcome, so I would just use
> the phy_id argument passed to fixed_phy_add() as the address on the
> fixed MDIO bus like it is today.

Well, the whole starting point of this discussion is precisely that
both Grant and Mark disliked this phy ID that had to be globally
unique, and they wanted it to be dynamically allocated by the kernel,
because the DT shouldn't have to care about such internal details.

> > Grant, Mark, Florian, do you have other proposals?
> 
> To sum up, let's just forget about the misuse of phy_id to fill in
> PHYS_ID1 and PHYS_ID2 registers and keep the existing code with a
> clear note that phy_id means the "PHY address on the fixed MDIO bus".

Except that Grant and Mark point was precisely that the "PHY address on
the fake MDIO bus" is not a description of the hardware, and therefore
shouldn't be mentioned in the Device Tree but instead taken care of by
the kernel itself.

Best regards,

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

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Florian Fainelli]

2013/7/30 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>:
> Dear Florian Fainelli,
>
> On Tue, 30 Jul 2013 11:05:04 +0100, Florian Fainelli wrote:
>
>> > In fact, this value is used for two things:
>> >
>> >  * As the PHY address on the fake "fixed" MDIO bus.
>> >
>> >  * As the PHY identifier, as reported by the MII registers PHYS_ID1
>> >    (0x2) and PHYS_ID2 (0x3).
>>
>> Right, so I would start with disambiguating the two and just forget
>> about PHYS_ID1 and PHYS_ID2 for the moment since they probably do not
>> need to be per-PHY configurable.
>
> Agreed.
>
>> > I think this doesn't make sense, because the two things are completely
>> > unrelated. Ideally, we'd like the PHY identifier for fixed PHYs to be
>> > something fixed, identical for all fixed PHYs. The problem is finding
>> > an OUI and device number that is available for that, but maybe we can
>> > ask the OpenMoko people to allocate one (see
>> > http://wiki.openmoko.org/wiki/OUI).
>>
>> Well that would be ideal indeed, but I am wondering if we cannot just
>> go with some kind of magic value here anyway, regardless of
>> allocations since this is not a real hardware device.
>
> Well, isn't that exactly what I'm proposing? Have a fixed value for
> PHYS_ID1 and PHYS_ID2, that is used for /all/ fixed PHYs.
>
>> How about the Linux Foundation?
>
> I am concerned about the time it would take to get a new OUI
> attributed. The OpenMoko OUI is said to be open for free and open
> source projects, I think the Linux kernel qualifies :)
>
>> Is not that the same problem as with gadget USB
>> devices which should have some sort of real MAC address for instance?
>
> They are either provided by the user or generated randomly, as far as I
> can see.
>
>> > Then, the PHY address could be generated dynamically. This would
>> > require:
>> >
>> >  * Adding a fixed_phy_create() function that internally uses
>> >    fixed_phy_add(), but before that creates an unique PHY address for
>> >    this newly created PHY. Those unique addresses will be generated by
>> >    incrementing a global number of fixed PHYs, up to PHY_MAX_ADDR,
>> >    which is the maximum number of fixed PHYs that can anyway be
>> >    registered on the fixed MDIO bus.
>>
>> Even though these are purely software PHY implementations, I believe
>> that some sort of predictability would be welcome, so I would just use
>> the phy_id argument passed to fixed_phy_add() as the address on the
>> fixed MDIO bus like it is today.
>
> Well, the whole starting point of this discussion is precisely that
> both Grant and Mark disliked this phy ID that had to be globally
> unique, and they wanted it to be dynamically allocated by the kernel,
> because the DT shouldn't have to care about such internal details.

Ok, well, thinking about this some more, we do not need to be able to
specify the fixed PHY fake MDIO address in the binding anyway. My
concern was first with how to ensure that this would be relatively
stable from an Ethernet driver and user-space perspective, but this
will be stable if we allocate unique IDs in say, parsing/probing order
anyway.

>
>> > Grant, Mark, Florian, do you have other proposals?
>>
>> To sum up, let's just forget about the misuse of phy_id to fill in
>> PHYS_ID1 and PHYS_ID2 registers and keep the existing code with a
>> clear note that phy_id means the "PHY address on the fixed MDIO bus".
>
> Except that Grant and Mark point was precisely that the "PHY address on
> the fake MDIO bus" is not a description of the hardware, and therefore
> shouldn't be mentioned in the Device Tree but instead taken care of by
> the kernel itself.

Right, that works for me now.
--
Florian

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Mark Rutland]

On Tue, Jul 30, 2013 at 10:07:05AM +0100, Thomas Petazzoni wrote:
> Dear Grant Likely,
> 
> On Tue, 23 Jul 2013 12:39:52 +0100, Grant Likely wrote:
> 
> > > +Such a fixed link situation is described within an Ethernet device
> > > +Device Tree node using a 'fixed-link' property, composed of 5
> > > +elements:
> > > +
> > > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in
> > > +    the system.
> > 
> > That's just loony!  :)  Regardless of existing code doing this, it is
> > absolutely ridiculous to have it in the driver. The kernel should
> > handle generating a phy id transparently. I'd rather mark this field
> > as reserved in the binding and change the code to not care about it
> > anymore.
> 
> In fact, this value is used for two things:
> 
>  * As the PHY address on the fake "fixed" MDIO bus.
> 
>  * As the PHY identifier, as reported by the MII registers PHYS_ID1
>    (0x2) and PHYS_ID2 (0x3).
> 
> I think this doesn't make sense, because the two things are completely
> unrelated. Ideally, we'd like the PHY identifier for fixed PHYs to be
> something fixed, identical for all fixed PHYs. The problem is finding
> an OUI and device number that is available for that, but maybe we can
> ask the OpenMoko people to allocate one (see
> http://wiki.openmoko.org/wiki/OUI).
> 
> Then, the PHY address could be generated dynamically. This would
> require:
> 
>  * Adding a fixed_phy_create() function that internally uses
>    fixed_phy_add(), but before that creates an unique PHY address for
>    this newly created PHY. Those unique addresses will be generated by
>    incrementing a global number of fixed PHYs, up to PHY_MAX_ADDR,
>    which is the maximum number of fixed PHYs that can anyway be
>    registered on the fixed MDIO bus.
> 
>    fixed_phy_create() would return this PHY address (positive) on
>    success, or a negative error code on failure.
> 
>  * Change of_phy_register_fixed_link() to call fixed_phy_create()
>    instead of fixed_phy_add() and make it return the PHY address
>    allocated by fixed_phy_create().
> 
>  * Add a of_phy_connect_fixed_link_direct() that is similar to
>    of_phy_connect_fixed_link() but takes an additional PHY address as
>    argument and uses that to generate the 'bus_id' used to find the
>    phy_device.
> 
> Grant, Mark, Florian, do you have other proposals?

The above sounds good to me.

Thanks,
Mark.

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Sascha Hauer]

On Mon, Jul 15, 2013 at 05:34:08PM +0200, Thomas Petazzoni 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", as a "fixed-link" property
> of the Ethernet device Device Tree node.
> 
> This patch adds:
> 
>  * A documentation for the Device Tree property "fixed-link".
> 
>  * A of_phy_register_fixed_link() OF helper, which provided an OF node
>    that contains a "fixed-link" property, registers the corresponding
>    fixed PHY.
> 
>  * Removes the warning on the of_phy_connect_fixed_link() that says
>    new drivers should not use it, since Grant Likely indicated that
>    this "fixed-link" property is indeed the way to go.
> 

Any progress with this series?

We have more and more boards here with exactly the same problem as
Thomas has. For reasons stated below I don't like this binding, but
still it would solve my problem.


> +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 within an Ethernet device
> +Device Tree node using a 'fixed-link' property, composed of 5
> +elements:
> +
> + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in
> +    the system.
> + 2. The duplex (1 for full-duplex, 0 for half-duplex)
> + 3. The speed (10, 100, 1000)
> + 4. The pause setting (1 for enabled, 0 for disabled)
> + 5. The asym pause setting (1 for enabled, 0 for disabled)
> +
> +Example:
> +
> +ethernet@0 {
> +	...
> +	fixed-link = <1 1 1000 0 0>;
> +	...
> +};

I must say I don't like this binding at all for two reasons.

First the positional arguments make it impossible to add optional
arguments to the link.

Second the other side of the link is most likely a switch. Once this
switch has its own node in the devicetree it seems like having a phandle
to the switch here would be better.

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 PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Thomas Petazzoni]

Dear Sascha Hauer,

On Mon, 12 Aug 2013 08:38:06 +0200, Sascha Hauer wrote:

> > This patch adds:
> > 
> >  * A documentation for the Device Tree property "fixed-link".
> > 
> >  * A of_phy_register_fixed_link() OF helper, which provided an OF node
> >    that contains a "fixed-link" property, registers the corresponding
> >    fixed PHY.
> > 
> >  * Removes the warning on the of_phy_connect_fixed_link() that says
> >    new drivers should not use it, since Grant Likely indicated that
> >    this "fixed-link" property is indeed the way to go.
> > 
> 
> Any progress with this series?

I am not sure there really was a consensus yet on what the DT binding
looks like. As soon as there is a consensus, I'm definitely willing to
make progress on this series.

> We have more and more boards here with exactly the same problem as
> Thomas has. For reasons stated below I don't like this binding, but
> still it would solve my problem.

Ok.

> > +Example:
> > +
> > +ethernet@0 {
> > +	...
> > +	fixed-link = <1 1 1000 0 0>;
> > +	...
> > +};
> 
> I must say I don't like this binding at all for two reasons.

As I explained, this binding was chosen for this RFC for two reasons:

 * It's the binding used on PowerPC platforms to represent fixed links.
 * It allows to encode all the informations into a single property,
   which avoids the need for a separate DT node for a "fake PHY", which
   isn't a representation of the hardware.

> First the positional arguments make it impossible to add optional
> arguments to the link.
> 
> Second the other side of the link is most likely a switch. Once this
> switch has its own node in the devicetree it seems like having a phandle
> to the switch here would be better.

So, in other words, what you're suggesting is something like:

	ethernet@0 {
		reg = <...>;
		interrupt = <...>;
		phy = <&phy0>;
		phy0: phy@0 {
			fixed-link;
			speed = <1000>;
			full-duplex;
			...
		};
	};

Or something else?

Best regards,

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

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Sascha Hauer]

On Mon, Aug 12, 2013 at 10:16:49AM +0200, Thomas Petazzoni wrote:
> Dear Sascha Hauer,
> 
> On Mon, 12 Aug 2013 08:38:06 +0200, Sascha Hauer wrote:
> 
> > > This patch adds:
> > > 
> > >  * A documentation for the Device Tree property "fixed-link".
> > > 
> > >  * A of_phy_register_fixed_link() OF helper, which provided an OF node
> > >    that contains a "fixed-link" property, registers the corresponding
> > >    fixed PHY.
> > > 
> > >  * Removes the warning on the of_phy_connect_fixed_link() that says
> > >    new drivers should not use it, since Grant Likely indicated that
> > >    this "fixed-link" property is indeed the way to go.
> > > 
> > 
> > Any progress with this series?
> 
> I am not sure there really was a consensus yet on what the DT binding
> looks like. As soon as there is a consensus, I'm definitely willing to
> make progress on this series.
> 
> > We have more and more boards here with exactly the same problem as
> > Thomas has. For reasons stated below I don't like this binding, but
> > still it would solve my problem.
> 
> Ok.
> 
> > > +Example:
> > > +
> > > +ethernet@0 {
> > > +	...
> > > +	fixed-link = <1 1 1000 0 0>;
> > > +	...
> > > +};
> > 
> > I must say I don't like this binding at all for two reasons.
> 
> As I explained, this binding was chosen for this RFC for two reasons:
> 
>  * It's the binding used on PowerPC platforms to represent fixed links.
>  * It allows to encode all the informations into a single property,
>    which avoids the need for a separate DT node for a "fake PHY", which
>    isn't a representation of the hardware.

The fake phy is avoided by making the other side of the link what it
really is: An ethernet switch. I'm currently not aware of a situation
where a fixed link is needed and the other side is not a switch. And I
can't think of a situation in which the other side of the other side of
the fixed link really is pure 'virtual', I mean there always must be
something connected, right?

> 
> > First the positional arguments make it impossible to add optional
> > arguments to the link.
> > 
> > Second the other side of the link is most likely a switch. Once this
> > switch has its own node in the devicetree it seems like having a phandle
> > to the switch here would be better.
> 
> So, in other words, what you're suggesting is something like:
> 
> 	ethernet@0 {
> 		reg = <...>;
> 		interrupt = <...>;
> 		phy = <&phy0>;
> 		phy0: phy@0 {
> 			fixed-link;
> 			speed = <1000>;
> 			full-duplex;
> 			...
> 		};
> 	};

Yes, this looks good. ePAPR suggests naming the phy property
"phy-handle" instead of just "phy", but that's just details. In case the
phy really is a switch the phandle could just point to a i2c device instead
of the ethernet node.

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 PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Christian Gmeiner]

--
Christian Gmeiner, MSc


2013/8/12 Sascha Hauer <s.hauer@pengutronix.de>:
> On Mon, Aug 12, 2013 at 10:16:49AM +0200, Thomas Petazzoni wrote:
>> Dear Sascha Hauer,
>>
>> On Mon, 12 Aug 2013 08:38:06 +0200, Sascha Hauer wrote:
>>
>> > > This patch adds:
>> > >
>> > >  * A documentation for the Device Tree property "fixed-link".
>> > >
>> > >  * A of_phy_register_fixed_link() OF helper, which provided an OF node
>> > >    that contains a "fixed-link" property, registers the corresponding
>> > >    fixed PHY.
>> > >
>> > >  * Removes the warning on the of_phy_connect_fixed_link() that says
>> > >    new drivers should not use it, since Grant Likely indicated that
>> > >    this "fixed-link" property is indeed the way to go.
>> > >
>> >
>> > Any progress with this series?
>>
>> I am not sure there really was a consensus yet on what the DT binding
>> looks like. As soon as there is a consensus, I'm definitely willing to
>> make progress on this series.
>>
>> > We have more and more boards here with exactly the same problem as
>> > Thomas has. For reasons stated below I don't like this binding, but
>> > still it would solve my problem.
>>
>> Ok.
>>
>> > > +Example:
>> > > +
>> > > +ethernet@0 {
>> > > + ...
>> > > + fixed-link = <1 1 1000 0 0>;
>> > > + ...
>> > > +};
>> >
>> > I must say I don't like this binding at all for two reasons.
>>
>> As I explained, this binding was chosen for this RFC for two reasons:
>>
>>  * It's the binding used on PowerPC platforms to represent fixed links.
>>  * It allows to encode all the informations into a single property,
>>    which avoids the need for a separate DT node for a "fake PHY", which
>>    isn't a representation of the hardware.
>
> The fake phy is avoided by making the other side of the link what it
> really is: An ethernet switch. I'm currently not aware of a situation
> where a fixed link is needed and the other side is not a switch. And I
> can't think of a situation in which the other side of the other side of
> the fixed link really is pure 'virtual', I mean there always must be
> something connected, right?
>
>>
>> > First the positional arguments make it impossible to add optional
>> > arguments to the link.
>> >
>> > Second the other side of the link is most likely a switch. Once this
>> > switch has its own node in the devicetree it seems like having a phandle
>> > to the switch here would be better.
>>
>> So, in other words, what you're suggesting is something like:
>>
>>       ethernet@0 {
>>               reg = <...>;
>>               interrupt = <...>;
>>               phy = <&phy0>;
>>               phy0: phy@0 {
>>                       fixed-link;
>>                       speed = <1000>;
>>                       full-duplex;
>>                       ...
>>               };
>>       };
>
> Yes, this looks good. ePAPR suggests naming the phy property
> "phy-handle" instead of just "phy", but that's just details. In case the
> phy really is a switch the phandle could just point to a i2c device instead
> of the ethernet node.
>

I have here a I.MX6 based board where I have the same issue that the
MAC is directly
connected to a switch. The switch has 5 phys (5 port 100mbt switch)
but the phy5 is not
physically connected to my FEC MAC as my hw guy has chosen to do it without it.
In general it is not a bad idea and I got networking working in u-boot
quite easily.

http://patchwork.ozlabs.org/patch/266558/

Now I am interested in a solution and I think that a fixed-phy is the wrong way.
I like the fixed-link solution more as it models the real world
better. In my example I
do not have any phy, but I have a fixed-link.

&fec {
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_enet_1>;
        fixed-link = <0 1 100 0 0>;
        status = "okay";
};

--
Christian Gmeiner, MSc

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Florian Fainelli]

2013/8/21 Christian Gmeiner <christian.gmeiner@gmail.com>:
> --
> Christian Gmeiner, MSc
>
>
> 2013/8/12 Sascha Hauer <s.hauer@pengutronix.de>:
>> On Mon, Aug 12, 2013 at 10:16:49AM +0200, Thomas Petazzoni wrote:
>>> Dear Sascha Hauer,
>>>
>>> On Mon, 12 Aug 2013 08:38:06 +0200, Sascha Hauer wrote:
>>>
>>> > > This patch adds:
>>> > >
>>> > >  * A documentation for the Device Tree property "fixed-link".
>>> > >
>>> > >  * A of_phy_register_fixed_link() OF helper, which provided an OF node
>>> > >    that contains a "fixed-link" property, registers the corresponding
>>> > >    fixed PHY.
>>> > >
>>> > >  * Removes the warning on the of_phy_connect_fixed_link() that says
>>> > >    new drivers should not use it, since Grant Likely indicated that
>>> > >    this "fixed-link" property is indeed the way to go.
>>> > >
>>> >
>>> > Any progress with this series?
>>>
>>> I am not sure there really was a consensus yet on what the DT binding
>>> looks like. As soon as there is a consensus, I'm definitely willing to
>>> make progress on this series.
>>>
>>> > We have more and more boards here with exactly the same problem as
>>> > Thomas has. For reasons stated below I don't like this binding, but
>>> > still it would solve my problem.
>>>
>>> Ok.
>>>
>>> > > +Example:
>>> > > +
>>> > > +ethernet@0 {
>>> > > + ...
>>> > > + fixed-link = <1 1 1000 0 0>;
>>> > > + ...
>>> > > +};
>>> >
>>> > I must say I don't like this binding at all for two reasons.
>>>
>>> As I explained, this binding was chosen for this RFC for two reasons:
>>>
>>>  * It's the binding used on PowerPC platforms to represent fixed links.
>>>  * It allows to encode all the informations into a single property,
>>>    which avoids the need for a separate DT node for a "fake PHY", which
>>>    isn't a representation of the hardware.
>>
>> The fake phy is avoided by making the other side of the link what it
>> really is: An ethernet switch. I'm currently not aware of a situation
>> where a fixed link is needed and the other side is not a switch. And I
>> can't think of a situation in which the other side of the other side of
>> the fixed link really is pure 'virtual', I mean there always must be
>> something connected, right?
>>
>>>
>>> > First the positional arguments make it impossible to add optional
>>> > arguments to the link.
>>> >
>>> > Second the other side of the link is most likely a switch. Once this
>>> > switch has its own node in the devicetree it seems like having a phandle
>>> > to the switch here would be better.
>>>
>>> So, in other words, what you're suggesting is something like:
>>>
>>>       ethernet@0 {
>>>               reg = <...>;
>>>               interrupt = <...>;
>>>               phy = <&phy0>;
>>>               phy0: phy@0 {
>>>                       fixed-link;
>>>                       speed = <1000>;
>>>                       full-duplex;
>>>                       ...
>>>               };
>>>       };
>>
>> Yes, this looks good. ePAPR suggests naming the phy property
>> "phy-handle" instead of just "phy", but that's just details. In case the
>> phy really is a switch the phandle could just point to a i2c device instead
>> of the ethernet node.
>>
>
> I have here a I.MX6 based board where I have the same issue that the
> MAC is directly
> connected to a switch. The switch has 5 phys (5 port 100mbt switch)
> but the phy5 is not
> physically connected to my FEC MAC as my hw guy has chosen to do it without it.
> In general it is not a bad idea and I got networking working in u-boot
> quite easily.

This is completely debatable and left to your own use case here.

>
> http://patchwork.ozlabs.org/patch/266558/
>
> Now I am interested in a solution and I think that a fixed-phy is the wrong way.
> I like the fixed-link solution more as it models the real world
> better. In my example I
> do not have any phy, but I have a fixed-link.
>
> &fec {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_enet_1>;
>         fixed-link = <0 1 100 0 0>;
>         status = "okay";
> };

As was already raised before, this representation has a couple of issues:
- not easily extendable
- not self-explanatory
- it transpires some internal OS concepts such as id/address to the binding

On the other side I prefer the other representation for all of the
opposite reasons above and it actually represents some kind of a PHY
device which is not discoverable via usual means such as MDIO, but
which still needs to be known by the Ethernet MAC to work correctly.

To move forward, we really need a better argumentation than "I do not like it".
-- 
Florian

Re: [RFC PATCH 1/3] of: provide a binding for the 'fixed-link' property

[Florian Fainelli]

2013/8/12 Sascha Hauer <s.hauer@pengutronix.de>:
> On Mon, Aug 12, 2013 at 10:16:49AM +0200, Thomas Petazzoni wrote:
>> Dear Sascha Hauer,
>>
>> On Mon, 12 Aug 2013 08:38:06 +0200, Sascha Hauer wrote:
>>
>> > > This patch adds:
>> > >
>> > >  * A documentation for the Device Tree property "fixed-link".
>> > >
>> > >  * A of_phy_register_fixed_link() OF helper, which provided an OF node
>> > >    that contains a "fixed-link" property, registers the corresponding
>> > >    fixed PHY.
>> > >
>> > >  * Removes the warning on the of_phy_connect_fixed_link() that says
>> > >    new drivers should not use it, since Grant Likely indicated that
>> > >    this "fixed-link" property is indeed the way to go.
>> > >
>> >
>> > Any progress with this series?
>>
>> I am not sure there really was a consensus yet on what the DT binding
>> looks like. As soon as there is a consensus, I'm definitely willing to
>> make progress on this series.
>>
>> > We have more and more boards here with exactly the same problem as
>> > Thomas has. For reasons stated below I don't like this binding, but
>> > still it would solve my problem.
>>
>> Ok.
>>
>> > > +Example:
>> > > +
>> > > +ethernet@0 {
>> > > + ...
>> > > + fixed-link = <1 1 1000 0 0>;
>> > > + ...
>> > > +};
>> >
>> > I must say I don't like this binding at all for two reasons.
>>
>> As I explained, this binding was chosen for this RFC for two reasons:
>>
>>  * It's the binding used on PowerPC platforms to represent fixed links.
>>  * It allows to encode all the informations into a single property,
>>    which avoids the need for a separate DT node for a "fake PHY", which
>>    isn't a representation of the hardware.
>
> The fake phy is avoided by making the other side of the link what it
> really is: An ethernet switch. I'm currently not aware of a situation
> where a fixed link is needed and the other side is not a switch.

There is such hardware out there, some platforms have a MoCA PHY which
is responsible for the signaling/control path while the data-path can
be connected to a slightly modified Ethernet MAC.

> And I
> can't think of a situation in which the other side of the other side of
> the fixed link really is pure 'virtual', I mean there always must be
> something connected, right?

I agree, there is something on the other end in every case.

>
>>
>> > First the positional arguments make it impossible to add optional
>> > arguments to the link.
>> >
>> > Second the other side of the link is most likely a switch. Once this
>> > switch has its own node in the devicetree it seems like having a phandle
>> > to the switch here would be better.
>>
>> So, in other words, what you're suggesting is something like:
>>
>>       ethernet@0 {
>>               reg = <...>;
>>               interrupt = <...>;
>>               phy = <&phy0>;
>>               phy0: phy@0 {
>>                       fixed-link;
>>                       speed = <1000>;
>>                       full-duplex;
>>                       ...
>>               };
>>       };
>
> Yes, this looks good. ePAPR suggests naming the phy property
> "phy-handle" instead of just "phy", but that's just details. In case the
> phy really is a switch the phandle could just point to a i2c device instead
> of the ethernet node.

I do like this representation better than the existing fixed-link property.
--
Florian

[RFC PATCH 2/3] net: phy: call mdiobus_scan() after adding a fixed PHY

[Thomas Petazzoni]

The fixed_phy_add() function allows to register a fixed PHY. However,
when this function gets called *after* fixed_mdio_bus_init() (which
gets called at the module_init stage), then the fixed PHY is not
registered into the phylib.

In order to address this, we add a call to mdiobus_scan() in
fixed_phy_add() to ensure that the PHY indeed gets registered into the
phylib, even if the fixed_phy_add() is called after
fixed_mdio_bus_init().

This is needed because until now, the only code that was calling the
fixed_add_phy() function was PowerPC-specific platform code, which
could ensure that such fixed PHYs get registered before
fixed_mdio_bus_init() is called.

However, with the new of_phy_register_fixed_link() function, device
drivers can parse their 'fixed-link' property and register a fixed PHY
at ->probe() time, which may happen after fixed_mdio_bus_init() is
called.

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

diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c
index ba55adf..bd1e67a 100644
--- a/drivers/net/phy/fixed.c
+++ b/drivers/net/phy/fixed.c
@@ -195,6 +195,8 @@ int fixed_phy_add(unsigned int irq, int phy_id,
 
 	list_add_tail(&fp->node, &fmb->phys);
 
+	mdiobus_scan(fmb->mii_bus, phy_id);
+
 	return 0;
 
 err_regs:
-- 
1.8.1.2

Re: [RFC PATCH 2/3] net: phy: call mdiobus_scan() after adding a fixed PHY

[Florian Fainelli]

Hello Thomas,

2013/7/15 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>:
> The fixed_phy_add() function allows to register a fixed PHY. However,
> when this function gets called *after* fixed_mdio_bus_init() (which
> gets called at the module_init stage), then the fixed PHY is not
> registered into the phylib.
>
> In order to address this, we add a call to mdiobus_scan() in
> fixed_phy_add() to ensure that the PHY indeed gets registered into the
> phylib, even if the fixed_phy_add() is called after
> fixed_mdio_bus_init().
>
> This is needed because until now, the only code that was calling the
> fixed_add_phy() function was PowerPC-specific platform code, which
> could ensure that such fixed PHYs get registered before
> fixed_mdio_bus_init() is called.

arch/mips/ar7/platform.c also calls fixed_phy_add() early enough as
well to ensure that we do not need to rescan the MDIO bus. There is
also another user: arch/m68k/platform/coldfire/m5272.c

>
> However, with the new of_phy_register_fixed_link() function, device
> drivers can parse their 'fixed-link' property and register a fixed PHY
> at ->probe() time, which may happen after fixed_mdio_bus_init() is
> called.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Reviewed-by: Florian Fainelli <florian@openwrt.org>

> ---
>  drivers/net/phy/fixed.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c
> index ba55adf..bd1e67a 100644
> --- a/drivers/net/phy/fixed.c
> +++ b/drivers/net/phy/fixed.c
> @@ -195,6 +195,8 @@ int fixed_phy_add(unsigned int irq, int phy_id,
>
>         list_add_tail(&fp->node, &fmb->phys);
>
> +       mdiobus_scan(fmb->mii_bus, phy_id);
> +
>         return 0;
>
>  err_regs:
> --
> 1.8.1.2
>



--
Florian

[RFC PATCH 3/3] 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>
---
 .../bindings/net/marvell-armada-370-neta.txt       | 24 +++++++++++++++++++---
 drivers/net/ethernet/marvell/mvneta.c              | 18 +++++++++++-----
 2 files changed, 34 insertions(+), 8 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..31e61c0 100644
--- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
+++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
@@ -4,13 +4,21 @@ 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-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.
 
-Example:
+Optional properties:
+
+- phy: A phandle to a phy node defining the PHY address (as the reg
+  property, a single integer). Note: if this property isn't present,
+  then fixed link is assumed, and the 'fixed-link' property is
+  mandatory.
+- fixed-link: A 5 elements array that describe a fixed link, see
+  fixed-link.txt for details. Note: if a 'phy' property is present,
+  this 'fixed-link' property is ignored.
+
+Examples:
 
 ethernet@d0070000 {
 	compatible = "marvell,armada-370-neta";
@@ -21,3 +29,13 @@ ethernet@d0070000 {
 	phy = <&phy0>;
 	phy-mode = "rgmii-id";
 };
+
+ethernet@d0070000 {
+	compatible = "marvell,armada-370-neta";
+	reg = <0xd0070000 0x2500>;
+	interrupts = <8>;
+	clocks = <&gate_clk 4>;
+	status = "okay";
+	fixed-link = <1 1 1000 0 0>;
+	phy-mode = "rgmii-id";
+};
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 712779f..0bd1590 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -2349,8 +2349,12 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
 {
 	struct phy_device *phy_dev;
 
-	phy_dev = of_phy_connect(pp->dev, pp->phy_node, mvneta_adjust_link, 0,
-				 pp->phy_interface);
+	if (pp->phy_node)
+		phy_dev = of_phy_connect(pp->dev, pp->phy_node, mvneta_adjust_link, 0,
+					 pp->phy_interface);
+	else
+		phy_dev = of_phy_connect_fixed_link(pp->dev, mvneta_adjust_link,
+						    pp->phy_interface);
 	if (!phy_dev) {
 		netdev_err(pp->dev, "could not find the PHY\n");
 		return -ENODEV;
@@ -2708,9 +2712,13 @@ 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;
+		/* No 'phy' found, see if we have a 'fixed-link'
+		 * property */
+		err = of_phy_register_fixed_link(dn);
+		if (err < 0) {
+			dev_err(&pdev->dev, "no 'phy' or 'fixed-link' properties\n");
+			goto err_free_irq;
+		}
 	}
 
 	phy_mode = of_get_phy_mode(dn);
-- 
1.8.1.2