[RFC PATCH net-next 0/2] DT MDIO bus of fixed phys

[RFC PATCH net-next 0/2] DT MDIO bus of fixed phys

[Andrew Lunn]

Currently, supporting a fixed-phy in a MAC driver is a bit messy. It
needs to be explicit supported, since a fixed phy is somewhat
different from a normal phy.

These two patches solve this by making fixed-phys appear as normal
PHYs within device tree, allowing them to be referenced by a phandle.
Any MAC driver that supports phy-handle can then automatically support
a fixed-phy without any code change.

Andrew Lunn (2):
  of: of_mdio: Factor out fixed-link parsing
  phy: fixed-phy: Allow DT description of an MDIO bus and PHYs.

 .../devicetree/bindings/net/fixed-link.txt         |  39 +++++++
 drivers/net/phy/fixed_phy.c                        | 122 ++++++++++++++++++++-
 drivers/of/of_mdio.c                               |  34 +++---
 include/linux/of_mdio.h                            |  11 +-
 4 files changed, 188 insertions(+), 18 deletions(-)

-- 
2.7.0

[RFC PATCH net-next 1/2] of: of_mdio: Factor out fixed-link parsing

[Andrew Lunn]

Turn the parsing of the fixed link properties into a helper function,
and export it for others to use.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/of/of_mdio.c    | 34 +++++++++++++++++++++-------------
 include/linux/of_mdio.h | 11 ++++++++++-
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 5e7838290998..e9ad328174ff 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -397,6 +397,23 @@ bool of_phy_is_fixed_link(struct device_node *np)
 }
 EXPORT_SYMBOL(of_phy_is_fixed_link);
 
+int of_phy_parse_fixed_link(struct device_node *np,
+			    struct fixed_phy_status *status,
+			    int *link_gpio)
+{
+	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");
+	*link_gpio = of_get_named_gpio_flags(np, "link-gpios", 0, NULL);
+	if (*link_gpio == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+	return 0;
+}
+EXPORT_SYMBOL(of_phy_parse_fixed_link);
+
 int of_phy_register_fixed_link(struct device_node *np)
 {
 	struct fixed_phy_status status = {};
@@ -419,20 +436,11 @@ int of_phy_register_fixed_link(struct device_node *np)
 	/* New binding */
 	fixed_link_node = of_get_child_by_name(np, "fixed-link");
 	if (fixed_link_node) {
-		status.link = 1;
-		status.duplex = of_property_read_bool(fixed_link_node,
-						      "full-duplex");
-		if (of_property_read_u32(fixed_link_node, "speed", &status.speed))
-			return -EINVAL;
-		status.pause = of_property_read_bool(fixed_link_node, "pause");
-		status.asym_pause = of_property_read_bool(fixed_link_node,
-							  "asym-pause");
-		link_gpio = of_get_named_gpio_flags(fixed_link_node,
-						    "link-gpios", 0, NULL);
+		err = of_phy_parse_fixed_link(fixed_link_node, &status,
+					      &link_gpio);
 		of_node_put(fixed_link_node);
-		if (link_gpio == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-
+		if (err)
+			return err;
 		phy = fixed_phy_register(PHY_POLL, &status, link_gpio, np);
 		return IS_ERR(phy) ? PTR_ERR(phy) : 0;
 	}
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 8f2237eb3485..1286a76dbcf0 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -10,6 +10,7 @@
 #define __LINUX_OF_MDIO_H
 
 #include <linux/phy.h>
+#include <linux/phy_fixed.h>
 #include <linux/of.h>
 
 #ifdef CONFIG_OF
@@ -25,7 +26,6 @@ struct phy_device *of_phy_attach(struct net_device *dev,
 
 extern struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np);
 extern int of_mdio_parse_addr(struct device *dev, const struct device_node *np);
-
 #else /* CONFIG_OF */
 static inline int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 {
@@ -72,6 +72,9 @@ static inline int of_mdio_parse_addr(struct device *dev,
 #if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY)
 extern int of_phy_register_fixed_link(struct device_node *np);
 extern bool of_phy_is_fixed_link(struct device_node *np);
+extern int of_phy_parse_fixed_link(struct device_node *np,
+				   struct fixed_phy_status *status,
+				   int *link_gpio);
 #else
 static inline int of_phy_register_fixed_link(struct device_node *np)
 {
@@ -81,6 +84,12 @@ static inline bool of_phy_is_fixed_link(struct device_node *np)
 {
 	return false;
 }
+static inline int of_phy_parse_fixed_link(struct device_node *np,
+				   struct fixed_phy_status *status,
+				   int *link_gpio)
+{
+	return -ENOSYS;
+}
 #endif
 
 
-- 
2.7.0

[RFC PATCH net-next 2/2] phy: fixed-phy: Allow DT description of an MDIO bus and PHYs.

[Andrew Lunn]

Not all MACs are connected to PHYs. They can for example be connected
to a switch. Using the fixed PHY properties with the MAC is possible,
but requires support in the MAC. It is however simpler to make use of
the phy-handle property to point to a PHY. To achieve this, the PHY
must be in the device tree.

Allow virtual MDIO busses to be represented in device tree, which
contains virtual fixed-phys.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 .../devicetree/bindings/net/fixed-link.txt         |  39 +++++++
 drivers/net/phy/fixed_phy.c                        | 122 ++++++++++++++++++++-
 2 files changed, 157 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt
index ec5d889fe3d8..2a22b92007fa 100644
--- a/Documentation/devicetree/bindings/net/fixed-link.txt
+++ b/Documentation/devicetree/bindings/net/fixed-link.txt
@@ -52,3 +52,42 @@ ethernet@1 {
 	};
 	...
 };
+
+Fixed link PHYs on an MDIO bus
+------------------------------
+
+An alternative to using the fixed link properties in the MAC is to
+define an MDIO bus with a number of fixed link phys on it.
+
+Required properties:
+- compatible = ""linux,mdio-fixed-phy";
+- #address-cells = <1>;
+- #size-cells = <0>;
+
+Child nodes represent PHYs on this mdio bus. Standard properties for
+fixed links, 'speed', 'full-duplex', 'pause', 'asym-pause',
+'link-gpios', as defined above are used. Additionally a 'reg' property
+is required for the address of the PHY on the bus. This should be of
+value 0 to 31.
+
+Example
+-------
+
+mdio {
+	compatible = "linux,mdio-fixed-phy";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	phy0: phy@0 {
+		reg = <0>;
+		speed = <1000>;
+		pause;
+		link-gpios = <&gpio0 12 GPIO_ACTIVE_HIGH>;
+	};
+
+	phy1: phy@8 {
+		reg = <8>;
+		speed = <100>;
+		full-duplex;
+	};
+};
diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index fc07a8866020..4df2fcdcee68 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -22,6 +22,8 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_mdio.h>
 #include <linux/gpio.h>
 
 #define MII_REGS_NUM 29
@@ -241,12 +243,12 @@ int fixed_phy_update_state(struct phy_device *phydev,
 }
 EXPORT_SYMBOL(fixed_phy_update_state);
 
-int fixed_phy_add(unsigned int irq, int phy_addr,
-		  struct fixed_phy_status *status,
-		  int link_gpio)
+int fixed_phy_add_fmb(struct fixed_mdio_bus *fmb,
+		      unsigned int irq, int phy_addr,
+		      struct fixed_phy_status *status,
+		      int link_gpio)
 {
 	int ret;
-	struct fixed_mdio_bus *fmb = &platform_fmb;
 	struct fixed_phy *fp;
 
 	fp = kzalloc(sizeof(*fp), GFP_KERNEL);
@@ -283,6 +285,14 @@ err_regs:
 	kfree(fp);
 	return ret;
 }
+
+int fixed_phy_add(unsigned int irq, int phy_addr,
+		  struct fixed_phy_status *status,
+		  int link_gpio)
+{
+	return fixed_phy_add_fmb(&platform_fmb, irq, phy_addr, status,
+				 link_gpio);
+}
 EXPORT_SYMBOL_GPL(fixed_phy_add);
 
 static void fixed_phy_del(int phy_addr)
@@ -378,6 +388,103 @@ void fixed_phy_unregister(struct phy_device *phy)
 }
 EXPORT_SYMBOL_GPL(fixed_phy_unregister);
 
+static int mdio_fixed_phy_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct fixed_phy_status status;
+	struct fixed_mdio_bus *fmb;
+	struct device_node *child;
+	struct mii_bus *bus;
+	int phy_addr, irq;
+	int link_gpio;
+
+	int ret;
+
+	if (!np)
+		return -EINVAL;
+
+	bus = mdiobus_alloc_size(sizeof(*fmb));
+	if (!bus)
+		return -ENOMEM;
+
+	if (strlen(pdev->name) >= MII_BUS_ID_SIZE)
+		return -EINVAL;
+
+	strncpy(bus->id, pdev->name, MII_BUS_ID_SIZE - 1);
+	bus->name = bus->id;
+	bus->parent = &pdev->dev;
+	bus->read = fixed_mdio_read;
+	bus->write = fixed_mdio_write;
+	fmb = bus->priv;
+	fmb->mii_bus = bus;
+	INIT_LIST_HEAD(&fmb->phys);
+
+	for_each_available_child_of_node(np, child) {
+		phy_addr = of_mdio_parse_addr(&pdev->dev, child);
+		if (phy_addr < 0) {
+			ret = phy_addr;
+			goto err_out_free_phys;
+		}
+		irq = irq_of_parse_and_map(child, 0);
+
+		ret = of_phy_parse_fixed_link(child, &status, &link_gpio);
+		if (ret < 0)
+			goto err_out_free_phys;
+
+		ret = fixed_phy_add_fmb(fmb, irq, phy_addr,
+					&status, link_gpio);
+		if (ret < 0)
+			goto err_out_free_phys;
+	}
+
+	ret = of_mdiobus_register(bus, np);
+	if (ret < 0)
+		goto err_out_free_mdiobus;
+
+	platform_set_drvdata(pdev, bus);
+
+	return 0;
+
+err_out_free_phys:
+	for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++)
+		fixed_phy_del(phy_addr);
+
+err_out_free_mdiobus:
+	mdiobus_free(bus);
+	return ret;
+}
+
+static int mdio_fixed_phy_remove(struct platform_device *pdev)
+{
+	struct mii_bus *bus = platform_get_drvdata(pdev);
+	int phy_addr;
+
+	mdiobus_unregister(bus);
+
+	for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++)
+		fixed_phy_del(phy_addr);
+
+	mdiobus_free(bus);
+
+	return 0;
+}
+
+static const struct of_device_id mdio_fixed_phy_dt_ids[] = {
+	{ .compatible = "linux,mdio-fixed-phy" },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, mdio_fixed_phy_dt_ids);
+
+static struct platform_driver mdio_fixed_phy_driver = {
+	.probe = mdio_fixed_phy_probe,
+	.remove = mdio_fixed_phy_remove,
+	.driver = {
+		.name = "mdio-fixed-phy",
+		.of_match_table = mdio_fixed_phy_dt_ids,
+	},
+};
+
 static int __init fixed_mdio_bus_init(void)
 {
 	struct fixed_mdio_bus *fmb = &platform_fmb;
@@ -406,8 +513,14 @@ static int __init fixed_mdio_bus_init(void)
 	if (ret)
 		goto err_mdiobus_alloc;
 
+	ret = platform_driver_register(&mdio_fixed_phy_driver);
+	if (ret)
+		goto err_mdiobus_register;
+
 	return 0;
 
+err_mdiobus_register:
+	mdiobus_unregister(fmb->mii_bus);
 err_mdiobus_alloc:
 	mdiobus_free(fmb->mii_bus);
 err_mdiobus_reg:
@@ -422,6 +535,7 @@ static void __exit fixed_mdio_bus_exit(void)
 	struct fixed_mdio_bus *fmb = &platform_fmb;
 	struct fixed_phy *fp, *tmp;
 
+	platform_driver_unregister(&mdio_fixed_phy_driver);
 	mdiobus_unregister(fmb->mii_bus);
 	mdiobus_free(fmb->mii_bus);
 	platform_device_unregister(pdev);
-- 
2.7.0

Re: [RFC PATCH net-next 0/2] DT MDIO bus of fixed phys

[Florian Fainelli]

On 11/03/16 15:08, Andrew Lunn wrote:
> Currently, supporting a fixed-phy in a MAC driver is a bit messy. It
> needs to be explicit supported, since a fixed phy is somewhat
> different from a normal phy.
> 
> These two patches solve this by making fixed-phys appear as normal
> PHYs within device tree, allowing them to be referenced by a phandle.
> Any MAC driver that supports phy-handle can then automatically support
> a fixed-phy without any code change.

Humm, if that's the problem we want to solve, we could introduce a
helper function which tries to locate the phy using a 'phy-handle'
property, and if it fails falls back to looking for a fixed-link
property and use that if desired. That behavior could either be
automatic, or controlled via boolean flag for instance which indicates
whether fallback is appropriate?

I am not sure the virtual MDIO bus is looking much better in that
regard, it does provide a good model for how the fixed PHYs are
implemented: as a virtual MDIO bus, but that seems quite a bit of a
stretch here...
-- 
Florian

Re: [RFC PATCH net-next 2/2] phy: fixed-phy: Allow DT description of an MDIO bus and PHYs.

[Florian Fainelli]

On 11/03/16 15:08, Andrew Lunn wrote:
> Not all MACs are connected to PHYs. They can for example be connected
> to a switch. Using the fixed PHY properties with the MAC is possible,
> but requires support in the MAC. It is however simpler to make use of
> the phy-handle property to point to a PHY. To achieve this, the PHY
> must be in the device tree.
> 
> Allow virtual MDIO busses to be represented in device tree, which
> contains virtual fixed-phys.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  .../devicetree/bindings/net/fixed-link.txt         |  39 +++++++
>  drivers/net/phy/fixed_phy.c                        | 122 ++++++++++++++++++++-
>  2 files changed, 157 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt
> index ec5d889fe3d8..2a22b92007fa 100644
> --- a/Documentation/devicetree/bindings/net/fixed-link.txt
> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt
> @@ -52,3 +52,42 @@ ethernet@1 {
>  	};
>  	...
>  };
> +
> +Fixed link PHYs on an MDIO bus
> +------------------------------
> +
> +An alternative to using the fixed link properties in the MAC is to
> +define an MDIO bus with a number of fixed link phys on it.
> +
> +Required properties:
> +- compatible = ""linux,mdio-fixed-phy";

One too many " here.

> +- #address-cells = <1>;
> +- #size-cells = <0>;
> +
> +Child nodes represent PHYs on this mdio bus. Standard properties for
> +fixed links, 'speed', 'full-duplex', 'pause', 'asym-pause',
> +'link-gpios', as defined above are used. Additionally a 'reg' property
> +is required for the address of the PHY on the bus. This should be of
> +value 0 to 31.

This is a virtual bus, the only limitation is because we re-use to the
maximum permission extent the real MDIO bus code, and this is putting a
SW constraint on something that does not have one here.

> +
> +Example
> +-------
> +
> +mdio {
> +	compatible = "linux,mdio-fixed-phy";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	phy0: phy@0 {
> +		reg = <0>;
> +		speed = <1000>;
> +		pause;
> +		link-gpios = <&gpio0 12 GPIO_ACTIVE_HIGH>;
> +	};
> +
> +	phy1: phy@8 {
> +		reg = <8>;
> +		speed = <100>;
> +		full-duplex;
> +	};

The 'fixed-link' property, although it suffers from one basic issue
which is that it is placed on the consumer of it (e.g: an Ethernet MAC
Device Tree node) while being a two-headed snake (you actually describe
a data-pipe with a fixed-link) is not quite perfect, but this does not
seem to look any better.

One thing that Thomas solved nicely was avoid the need for allocating an
address on the virtual MDIO bus, but this is coming back here, which
does not seem needed except for correctness wrt. how real MDIO buses.

-- 
Florian

Re: [RFC PATCH net-next 0/2] DT MDIO bus of fixed phys

[Andrew Lunn]

On Fri, Mar 11, 2016 at 03:26:42PM -0800, Florian Fainelli wrote:
> On 11/03/16 15:08, Andrew Lunn wrote:
> > Currently, supporting a fixed-phy in a MAC driver is a bit messy. It
> > needs to be explicit supported, since a fixed phy is somewhat
> > different from a normal phy.
> > 
> > These two patches solve this by making fixed-phys appear as normal
> > PHYs within device tree, allowing them to be referenced by a phandle.
> > Any MAC driver that supports phy-handle can then automatically support
> > a fixed-phy without any code change.
> 
> Humm, if that's the problem we want to solve, we could introduce a
> helper function which tries to locate the phy using a 'phy-handle'
> property

I don't follow you. Where do you get a phandle from to use with
phy-handle?

	Andrew

Re: [RFC PATCH net-next 0/2] DT MDIO bus of fixed phys

[Florian Fainelli]

On 11/03/16 15:36, Andrew Lunn wrote:
> On Fri, Mar 11, 2016 at 03:26:42PM -0800, Florian Fainelli wrote:
>> On 11/03/16 15:08, Andrew Lunn wrote:
>>> Currently, supporting a fixed-phy in a MAC driver is a bit messy. It
>>> needs to be explicit supported, since a fixed phy is somewhat
>>> different from a normal phy.
>>>
>>> These two patches solve this by making fixed-phys appear as normal
>>> PHYs within device tree, allowing them to be referenced by a phandle.
>>> Any MAC driver that supports phy-handle can then automatically support
>>> a fixed-phy without any code change.
>>
>> Humm, if that's the problem we want to solve, we could introduce a
>> helper function which tries to locate the phy using a 'phy-handle'
>> property
> 
> I don't follow you. Where do you get a phandle from to use with
> phy-handle?

>From the caller of the function: the consumer of that phy-handle and/or
fixed-link property which is either an Ethernet MAC driver or a DSA's
switch port node.
-- 
Florian

Re: [RFC PATCH net-next 2/2] phy: fixed-phy: Allow DT description of an MDIO bus and PHYs.

[Andrew Lunn]

> > +Child nodes represent PHYs on this mdio bus. Standard properties for
> > +fixed links, 'speed', 'full-duplex', 'pause', 'asym-pause',
> > +'link-gpios', as defined above are used. Additionally a 'reg' property
> > +is required for the address of the PHY on the bus. This should be of
> > +value 0 to 31.
> 
> This is a virtual bus, the only limitation is because we re-use to the
> maximum permission extent the real MDIO bus code, and this is putting a
> SW constraint on something that does not have one here.

True. I was too lazy to audit the phy and mdio code to see if it
actually allows phy_addr >= 32. And it is easy to instantiate another
bus if you need it.
 
> > +
> > +Example
> > +-------
> > +
> > +mdio {
> > +	compatible = "linux,mdio-fixed-phy";
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +
> > +	phy0: phy@0 {
> > +		reg = <0>;
> > +		speed = <1000>;
> > +		pause;
> > +		link-gpios = <&gpio0 12 GPIO_ACTIVE_HIGH>;
> > +	};
> > +
> > +	phy1: phy@8 {
> > +		reg = <8>;
> > +		speed = <100>;
> > +		full-duplex;
> > +	};
> 
> The 'fixed-link' property, although it suffers from one basic issue
> which is that it is placed on the consumer of it (e.g: an Ethernet MAC
> Device Tree node) while being a two-headed snake (you actually describe
> a data-pipe with a fixed-link) is not quite perfect, but this does not
> seem to look any better.

It is actually just the same. Not worse, not better.

I see the real benefit in the drivers. They get fixed-phy for
free. Cleanup on release just works, which most drivers don't even do
now, so leak a fixed phy every time they are unloaded. DSA has some
messy code, which has had a number of bugs, handling fixed phys. By
adding this extra code, which i hope is reasonably simple to
understand and review, we make the MAC drivers a lot simpler and less
error prone.
 
> One thing that Thomas solved nicely was avoid the need for allocating an
> address on the virtual MDIO bus, but this is coming back here, which
> does not seem needed except for correctness wrt. how real MDIO buses.

Actually Thomas's code is broken. Fixed phy addresses are allocated
incrementally. When a fixed phy is released, its address is not
re-used. So as soon as you have gone through 32 alloc/free cycles, all
further allocs fail. This is however fixable.

	Andrew

Re: [RFC PATCH net-next 0/2] DT MDIO bus of fixed phys

[Andrew Lunn]

> >> Humm, if that's the problem we want to solve, we could introduce a
> >> helper function which tries to locate the phy using a 'phy-handle'
> >> property
> > 
> > I don't follow you. Where do you get a phandle from to use with
> > phy-handle?
> 
> >From the caller of the function: the consumer of that phy-handle and/or
> fixed-link property which is either an Ethernet MAC driver or a DSA's
> switch port node.

I still don't get it. Lets take a real example. I currently have this
in one of my dts files:

&fec1 {
       phy-mode = "rmii";
       pinctrl-names = "default";
       pinctrl-0 = <&pinctrl_fec1>;
       status = "okay";

       fixed-link {
                  speed = <100>;
                  full-duplex;
       };
};

and we now want to use a phy-handle:

&fec1 {
       phy-mode = "rmii";
       pinctrl-names = "default";
       pinctrl-0 = <&pinctrl_fec1>;
       status = "okay";

       phy-handle = <XZY>;
};

What do i use as XZY, so that it somehow resolves to a fixed-phy?

Thanks
	Andrew

Re: [RFC PATCH net-next 2/2] phy: fixed-phy: Allow DT description of an MDIO bus and PHYs.

[Andrew Lunn]

> One too many " here.
> 
> > +- #address-cells = <1>;
> > +- #size-cells = <0>;
> > +
> > +Child nodes represent PHYs on this mdio bus. Standard properties for
> > +fixed links, 'speed', 'full-duplex', 'pause', 'asym-pause',
> > +'link-gpios', as defined above are used. Additionally a 'reg' property
> > +is required for the address of the PHY on the bus. This should be of
> > +value 0 to 31.
> 
> This is a virtual bus, the only limitation is because we re-use to the
> maximum permission extent the real MDIO bus code, and this is putting a
> SW constraint on something that does not have one here.

Hi Florian

I now took a look at the core code. PHY_MAX_ADDR is used in quite a
few places, e.g. phy_find_first(), of_mdiobus_link_phydev(),
__mdiobus_register(). In order to be able to use this virtual MDIO bus
just like any other MDIO bus, we need to enforce PHY_MAX_ADDR.
Otherwise it is not going to work.

	  Andrew

Re: [RFC PATCH net-next 0/2] DT MDIO bus of fixed phys

[Florian Fainelli]

On 11/03/16 16:12, Andrew Lunn wrote:
>>>> Humm, if that's the problem we want to solve, we could introduce a
>>>> helper function which tries to locate the phy using a 'phy-handle'
>>>> property
>>>
>>> I don't follow you. Where do you get a phandle from to use with
>>> phy-handle?
>>
>> >From the caller of the function: the consumer of that phy-handle and/or
>> fixed-link property which is either an Ethernet MAC driver or a DSA's
>> switch port node.
> 
> I still don't get it. Lets take a real example. I currently have this
> in one of my dts files:
> 
> &fec1 {
>        phy-mode = "rmii";
>        pinctrl-names = "default";
>        pinctrl-0 = <&pinctrl_fec1>;
>        status = "okay";
> 
>        fixed-link {
>                   speed = <100>;
>                   full-duplex;
>        };
> };

All drivers have this exact same structure:

&fec1 {
	phy-handle = <XYZ>;
	or
	fixed-link {
		speed = <100>;
		full-duplex;
	};
};

In both cases, the argument that this proposed helper function would
take is a struct device_node pointing to &fec1 here. You could therefore
imagine having something along these lines:

struct device_node *of_get_phy_by_phandle(struct device_node *dn, bool
try_fixed_link)
{
	struct device_node *phy_dn;
	int ret;

	phy_dn = of_parse_phandle(dn, "phy-handle", 0);
	if (!phy_dn && !try_fixed_link)
		return -ENODEV;

	if (of_phy_is_fixed_link(dn)) {
		ret  = of_phy_register_fixed_link(dn);
		if (ret)
			return PTR_ERR(-ret);

		phy_dn = of_node_get(dn);
	}

	return phy_dn;		
}

In fact, we could even remove the "try_fixed_link" argument and just see
if of_phy_is_fixed_link() returns true. Yes, this is not a proper
device_node pointing to the emulated PHY, but without introducing
binding changes, that is probably the best we can do.

I mistakenly used the term 'phandle' when I actually meant 'struct
device_node' reference.
-- 
Florian

Re: [RFC PATCH net-next 0/2] DT MDIO bus of fixed phys

[Andrew Lunn]

On Mon, Mar 14, 2016 at 09:51:47AM -0700, Florian Fainelli wrote:
> On 11/03/16 16:12, Andrew Lunn wrote:
> >>>> Humm, if that's the problem we want to solve, we could introduce a
> >>>> helper function which tries to locate the phy using a 'phy-handle'
> >>>> property
> >>>
> >>> I don't follow you. Where do you get a phandle from to use with
> >>> phy-handle?
> >>
> >> >From the caller of the function: the consumer of that phy-handle and/or
> >> fixed-link property which is either an Ethernet MAC driver or a DSA's
> >> switch port node.
> > 
> > I still don't get it. Lets take a real example. I currently have this
> > in one of my dts files:
> > 
> > &fec1 {
> >        phy-mode = "rmii";
> >        pinctrl-names = "default";
> >        pinctrl-0 = <&pinctrl_fec1>;
> >        status = "okay";
> > 
> >        fixed-link {
> >                   speed = <100>;
> >                   full-duplex;
> >        };
> > };
> 
> All drivers have this exact same structure:
> 
> &fec1 {
> 	phy-handle = <XYZ>;
> 	or
> 	fixed-link {
> 		speed = <100>;
> 		full-duplex;
> 	};
> };
> 
> In both cases, the argument that this proposed helper function would
> take is a struct device_node pointing to &fec1 here. You could therefore
> imagine having something along these lines:
> 
> struct device_node *of_get_phy_by_phandle(struct device_node *dn, bool
> try_fixed_link)

I don't particularly like this name. It suggests it is using the
phandle, when it might not.

> {
> 	struct device_node *phy_dn;
> 	int ret;
> 
> 	phy_dn = of_parse_phandle(dn, "phy-handle", 0);
> 	if (!phy_dn && !try_fixed_link)
> 		return -ENODEV;
> 
> 	if (of_phy_is_fixed_link(dn)) {
> 		ret  = of_phy_register_fixed_link(dn);
> 		if (ret)
> 			return PTR_ERR(-ret);
> 
> 		phy_dn = of_node_get(dn);
> 	}
> 
> 	return phy_dn;		
> }

To make release work, i think you need to hack something into
phy_disconnect() or phy_detach() so that the fixed_phy registered
above gets freed.

I don't particularly like these special cases. What i suggested does
not require any special cases, because they act just like phys on an
mdio bus.

     Andrew