[PATCH] net: fec: Add support for multiple phys on mdiobus

[PATCH] net: fec: Add support for multiple phys on mdiobus

[Sascha Hauer]

There may be multiple phys on an mdio bus. This series adds support
for this to the fec driver. I recently had a board which has a switch
connected to the fec's mdio bus, so I had to pick the correct phy.

This series should be in line with other network drivers supporting
this (Marvell Armada for example)

Sascha

----------------------------------------------------------------
Sascha Hauer (2):
      net: fec: refactor dt probing
      net: fec: Add support for phys from devicetree

 Documentation/devicetree/bindings/net/fsl-fec.txt |   20 ++++++
 drivers/net/ethernet/freescale/fec.c              |   77 ++++++++++++---------
 drivers/net/ethernet/freescale/fec.h              |    1 +
 3 files changed, 67 insertions(+), 31 deletions(-)

[PATCH 1/2] net: fec: refactor dt probing

[Sascha Hauer]

For devicetree parsing only the fec_get_phy_mode_dt() is
available. Rename it to fec_probe_dt() to be able to add more devicetree
parsing to it.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 0704bca..2f86557 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -1484,12 +1484,14 @@ static int fec_enet_init(struct net_device *ndev)
 }
 
 #ifdef CONFIG_OF
-static int fec_get_phy_mode_dt(struct platform_device *pdev)
+static int fec_probe_dt(struct fec_enet_private *fep)
 {
-	struct device_node *np = pdev->dev.of_node;
+	struct device_node *np = fep->pdev->dev.of_node;
 
-	if (np)
-		return of_get_phy_mode(np);
+	if (!np)
+		return -ENODEV;
+
+	fep->phy_interface = of_get_phy_mode(np);
 
 	return -ENODEV;
 }
@@ -1519,7 +1521,7 @@ static void fec_reset_phy(struct platform_device *pdev)
 	gpio_set_value(phy_reset, 1);
 }
 #else /* CONFIG_OF */
-static inline int fec_get_phy_mode_dt(struct platform_device *pdev)
+static inline int fec_probe_dt(struct fec_enet_private *fep)
 {
 	return -ENODEV;
 }
@@ -1581,15 +1583,13 @@ fec_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, ndev);
 
-	ret = fec_get_phy_mode_dt(pdev);
+	ret = fec_probe_dt(fep);
 	if (ret < 0) {
 		pdata = pdev->dev.platform_data;
 		if (pdata)
 			fep->phy_interface = pdata->phy;
 		else
 			fep->phy_interface = PHY_INTERFACE_MODE_MII;
-	} else {
-		fep->phy_interface = ret;
 	}
 
 	for (i = 0; i < FEC_IRQ_NUM; i++) {
-- 
1.7.10.4

Re: [PATCH 1/2] net: fec: refactor dt probing

[Baruch Siach]

Hi Sascha,

On Mon, Jan 21, 2013 at 09:37:54AM +0100, Sascha Hauer wrote:
> For devicetree parsing only the fec_get_phy_mode_dt() is
> available. Rename it to fec_probe_dt() to be able to add more devicetree
> parsing to it.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---

[...]

> -static int fec_get_phy_mode_dt(struct platform_device *pdev)
> +static int fec_probe_dt(struct fec_enet_private *fep)
>  {
> -	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *np = fep->pdev->dev.of_node;
>  
> -	if (np)
> -		return of_get_phy_mode(np);
> +	if (!np)
> +		return -ENODEV;
> +
> +	fep->phy_interface = of_get_phy_mode(np);
>  
>  	return -ENODEV;

Maybe return fep->phy_interface?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH 1/2] net: fec: refactor dt probing

[Sascha Hauer]

On Mon, Jan 21, 2013 at 07:48:26PM +0200, Baruch Siach wrote:
> Hi Sascha,
> 
> On Mon, Jan 21, 2013 at 09:37:54AM +0100, Sascha Hauer wrote:
> > For devicetree parsing only the fec_get_phy_mode_dt() is
> > available. Rename it to fec_probe_dt() to be able to add more devicetree
> > parsing to it.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> 
> [...]
> 
> > -static int fec_get_phy_mode_dt(struct platform_device *pdev)
> > +static int fec_probe_dt(struct fec_enet_private *fep)
> >  {
> > -	struct device_node *np = pdev->dev.of_node;
> > +	struct device_node *np = fep->pdev->dev.of_node;
> >  
> > -	if (np)
> > -		return of_get_phy_mode(np);
> > +	if (!np)
> > +		return -ENODEV;
> > +
> > +	fep->phy_interface = of_get_phy_mode(np);
> >  
> >  	return -ENODEV;
> 
> Maybe return fep->phy_interface?

No, but indeed this should return successfully. I will rework this after
it's clear how we want to proceed with this.

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 |

[PATCH 2/2] net: fec: Add support for phys from devicetree

[Sascha Hauer]

This adds support for specifying the phy for the fec driver through the
devicetree. Possible usecases are:

- The fec internal MDIO bus has multiple phys connected and a particular
  one has to be chosen which is physically connected to the (RG)MII interface.
- The phy is connected to an external MDIO bus.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 Documentation/devicetree/bindings/net/fsl-fec.txt |   20 +++++++
 drivers/net/ethernet/freescale/fec.c              |   61 +++++++++++++--------
 drivers/net/ethernet/freescale/fec.h              |    1 +
 3 files changed, 59 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
index d536392..ec7060b 100644
--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
@@ -15,6 +15,9 @@ Optional properties:
   only if property "phy-reset-gpios" is available.  Missing the property
   will have the duration be 1 millisecond.  Numbers greater than 1000 are
   invalid and 1 millisecond will be used instead.
+- phy : a phandle for the PHY device used for the fec. Used to specify an
+  external phy or to specify a particular address if the mdio bus has multiple
+  phys on it.
 
 Example:
 
@@ -26,3 +29,20 @@ ethernet@83fec000 {
 	phy-reset-gpios = <&gpio2 14 0>; /* GPIO2_14 */
 	local-mac-address = [00 04 9F 01 1B B9];
 };
+
+Example with specific phy address:
+
+ethernet@83fec000 {
+	compatible = "fsl,imx51-fec", "fsl,imx27-fec";
+	reg = <0x83fec000 0x4000>;
+	interrupts = <87>;
+	phy-mode = "mii";
+	phy-reset-gpios = <&gpio2 14 0>; /* GPIO2_14 */
+	local-mac-address = [00 04 9F 01 1B B9];
+	phy = &phy3;
+
+	phy3: ethernet-phy@3 {
+		reg = <3>;
+		device_type = "ethernet-phy";
+	};
+};
diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 2f86557..54a8506 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -48,6 +48,7 @@
 #include <linux/of_device.h>
 #include <linux/of_gpio.h>
 #include <linux/of_net.h>
+#include <linux/of_mdio.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/regulator/consumer.h>
 
@@ -950,31 +951,38 @@ static int fec_enet_mii_probe(struct net_device *ndev)
 
 	fep->phy_dev = NULL;
 
-	/* check for attached phy */
-	for (phy_id = 0; (phy_id < PHY_MAX_ADDR); phy_id++) {
-		if ((fep->mii_bus->phy_mask & (1 << phy_id)))
-			continue;
-		if (fep->mii_bus->phy_map[phy_id] == NULL)
-			continue;
-		if (fep->mii_bus->phy_map[phy_id]->phy_id == 0)
-			continue;
-		if (dev_id--)
-			continue;
-		strncpy(mdio_bus_id, fep->mii_bus->id, MII_BUS_ID_SIZE);
-		break;
-	}
+	if (fep->phy_node) {
+		phy_dev = of_phy_connect(ndev, fep->phy_node, &fec_enet_adjust_link, 0,
+			      fep->phy_interface);
+	} else {
+		/* check for attached phy */
+		for (phy_id = 0; (phy_id < PHY_MAX_ADDR); phy_id++) {
+			if ((fep->mii_bus->phy_mask & (1 << phy_id)))
+				continue;
+			if (fep->mii_bus->phy_map[phy_id] == NULL)
+				continue;
+			if (fep->mii_bus->phy_map[phy_id]->phy_id == 0)
+				continue;
+			if (dev_id--)
+				continue;
+			strncpy(mdio_bus_id, fep->mii_bus->id, MII_BUS_ID_SIZE);
+			break;
+		}
+
+		if (phy_id >= PHY_MAX_ADDR) {
+			printk(KERN_INFO
+				"%s: no PHY, assuming direct connection to switch\n",
+				ndev->name);
+			strncpy(mdio_bus_id, "fixed-0", MII_BUS_ID_SIZE);
+			phy_id = 0;
+		}
+
+		snprintf(phy_name, sizeof(phy_name), PHY_ID_FMT, mdio_bus_id, phy_id);
 
-	if (phy_id >= PHY_MAX_ADDR) {
-		printk(KERN_INFO
-			"%s: no PHY, assuming direct connection to switch\n",
-			ndev->name);
-		strncpy(mdio_bus_id, "fixed-0", MII_BUS_ID_SIZE);
-		phy_id = 0;
+		phy_dev = phy_connect(ndev, phy_name, &fec_enet_adjust_link, 0,
+				      fep->phy_interface);
 	}
 
-	snprintf(phy_name, sizeof(phy_name), PHY_ID_FMT, mdio_bus_id, phy_id);
-	phy_dev = phy_connect(ndev, phy_name, &fec_enet_adjust_link, 0,
-			      fep->phy_interface);
 	if (IS_ERR(phy_dev)) {
 		printk(KERN_ERR "%s: could not attach to PHY\n", ndev->name);
 		return PTR_ERR(phy_dev);
@@ -1076,7 +1084,12 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	for (i = 0; i < PHY_MAX_ADDR; i++)
 		fep->mii_bus->irq[i] = PHY_POLL;
 
-	if (mdiobus_register(fep->mii_bus))
+	if (fep->phy_node)
+		err = of_mdiobus_register(fep->mii_bus, pdev->dev.of_node);
+	else
+		err = mdiobus_register(fep->mii_bus);
+
+	if (err)
 		goto err_out_free_mdio_irq;
 
 	mii_cnt++;
@@ -1493,6 +1506,8 @@ static int fec_probe_dt(struct fec_enet_private *fep)
 
 	fep->phy_interface = of_get_phy_mode(np);
 
+	fep->phy_node = of_parse_phandle(np, "phy", 0);
+
 	return -ENODEV;
 }
 
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index c5a3bc1..0120ea6 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -236,6 +236,7 @@ struct fec_enet_private {
 	/* Phylib and MDIO interface */
 	struct	mii_bus *mii_bus;
 	struct	phy_device *phy_dev;
+	struct	device_node *phy_node;
 	int	mii_timeout;
 	uint	phy_speed;
 	phy_interface_t	phy_interface;
-- 
1.7.10.4

Re: [PATCH] net: fec: Add support for multiple phys on mdiobus

[Wolfgang Grandegger]

On 01/21/2013 09:37 AM, Sascha Hauer wrote:
> There may be multiple phys on an mdio bus. This series adds support
> for this to the fec driver. I recently had a board which has a switch
> connected to the fec's mdio bus, so I had to pick the correct phy.

Pick one PHY from a switch port? Well, does a PHY-less (or fixed-link)
configuration for a switch not make more sense? Various ARM Ethernet
contoller drivers do not support it. I recently needed a hack for an
AT91 board.

Wolfgang.

Re: [PATCH] net: fec: Add support for multiple phys on mdiobus

[Sascha Hauer]

On Mon, Jan 21, 2013 at 09:56:24AM +0100, Wolfgang Grandegger wrote:
> On 01/21/2013 09:37 AM, Sascha Hauer wrote:
> > There may be multiple phys on an mdio bus. This series adds support
> > for this to the fec driver. I recently had a board which has a switch
> > connected to the fec's mdio bus, so I had to pick the correct phy.
> 
> Pick one PHY from a switch port? Well, does a PHY-less (or fixed-link)
> configuration for a switch not make more sense?

Yes, you're probably right.

> Various ARM Ethernet
> contoller drivers do not support it. I recently needed a hack for an
> AT91 board.

I wonder how we want to proceed. Should there be a devicetree property
'fixed-link' like done for fs_enet (and not recommended for new code,
stated in the comment above of_phy_connect_fixed_link)?

Currently I have a property 'phy' in the fec binding which has a phandle
to a phy provided by the fec's mdio bus, but this could equally well
point to a fixed dummy phy:

	phy = &fixed-phy;

Currently there seems to be no common convention for the devicetree how
to handle such situations, or am I missing something?

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: [PATCH] net: fec: Add support for multiple phys on mdiobus

[Wolfgang Grandegger]

On 01/21/2013 11:07 AM, Sascha Hauer wrote:
> On Mon, Jan 21, 2013 at 09:56:24AM +0100, Wolfgang Grandegger wrote:
>> On 01/21/2013 09:37 AM, Sascha Hauer wrote:
>>> There may be multiple phys on an mdio bus. This series adds support
>>> for this to the fec driver. I recently had a board which has a switch
>>> connected to the fec's mdio bus, so I had to pick the correct phy.
>>
>> Pick one PHY from a switch port? Well, does a PHY-less (or fixed-link)
>> configuration for a switch not make more sense?
> 
> Yes, you're probably right.
> 
>> Various ARM Ethernet
>> contoller drivers do not support it. I recently needed a hack for an
>> AT91 board.
> 
> I wonder how we want to proceed. Should there be a devicetree property
> 'fixed-link' like done for fs_enet (and not recommended for new code,
> stated in the comment above of_phy_connect_fixed_link)?

Also the gianfar and ucc_geth drivers use this interface (via fixed
link phy). I tried to use it for the AT91 macb driver but stopped
quickly because the usage was not straight forward (too much code)...
even if the idea of using a fake fixed-link phy is not bad.

> Currently I have a property 'phy' in the fec binding which has a phandle
> to a phy provided by the fec's mdio bus, but this could equally well

But than the cable must be connected to the associated switch port.

> point to a fixed dummy phy:
> 
> 	phy = &fixed-phy;

The link speed, full/half duplex and maybe some mroe parameter should
be configurable via device tree.

> Currently there seems to be no common convention for the devicetree how
> to handle such situations, or am I missing something?

That's also may impression. There seem to be a few more related hacks:

$ find . -name '*.c'| xargs grep -i "phy-less" 
./ethernet/amd/au1000_eth.c:			netdev_info(dev, "using PHY-less setup\n");
./ethernet/amd/au1000_eth.c:	} else { /* PHY-less op, assume full-duplex */
./ethernet/ibm/emac/core.c:	/* PHY-less configuration.
./ethernet/ibm/emac/core.c:		/* PHY-less configuration.

I would prefer to handle the "fixed-link" property of the ethernet dt
node directly in the driver with a generic helper function.

Wolfgang.

Re: [PATCH] net: fec: Add support for multiple phys on mdiobus

[Florian Fainelli]

On 01/21/2013 12:07 PM, Wolfgang Grandegger wrote:
> On 01/21/2013 11:07 AM, Sascha Hauer wrote:
>> On Mon, Jan 21, 2013 at 09:56:24AM +0100, Wolfgang Grandegger wrote:
>>> On 01/21/2013 09:37 AM, Sascha Hauer wrote:
>>>> There may be multiple phys on an mdio bus. This series adds support
>>>> for this to the fec driver. I recently had a board which has a switch
>>>> connected to the fec's mdio bus, so I had to pick the correct phy.
>>>
>>> Pick one PHY from a switch port? Well, does a PHY-less (or fixed-link)
>>> configuration for a switch not make more sense?
>>
>> Yes, you're probably right.
>>
>>> Various ARM Ethernet
>>> contoller drivers do not support it. I recently needed a hack for an
>>> AT91 board.
>>
>> I wonder how we want to proceed. Should there be a devicetree property
>> 'fixed-link' like done for fs_enet (and not recommended for new code,
>> stated in the comment above of_phy_connect_fixed_link)?
>
> Also the gianfar and ucc_geth drivers use this interface (via fixed
> link phy). I tried to use it for the AT91 macb driver but stopped
> quickly because the usage was not straight forward (too much code)...
> even if the idea of using a fake fixed-link phy is not bad.
>
>> Currently I have a property 'phy' in the fec binding which has a phandle
>> to a phy provided by the fec's mdio bus, but this could equally well
>
> But than the cable must be connected to the associated switch port.
>
>> point to a fixed dummy phy:
>>
>> 	phy = &fixed-phy;
>
> The link speed, full/half duplex and maybe some mroe parameter should
> be configurable via device tree.
>
>> Currently there seems to be no common convention for the devicetree how
>> to handle such situations, or am I missing something?
>
> That's also may impression. There seem to be a few more related hacks:
>
> $ find . -name '*.c'| xargs grep -i "phy-less"
> ./ethernet/amd/au1000_eth.c:			netdev_info(dev, "using PHY-less setup\n");
> ./ethernet/amd/au1000_eth.c:	} else { /* PHY-less op, assume full-duplex */
> ./ethernet/ibm/emac/core.c:	/* PHY-less configuration.
> ./ethernet/ibm/emac/core.c:		/* PHY-less configuration.
>
> I would prefer to handle the "fixed-link" property of the ethernet dt
> node directly in the driver with a generic helper function.

Is not what of_phy_connect_fixed_link() offer? I am not sure there can 
be much done by an helper than that.
--
Florian

Re: [PATCH] net: fec: Add support for multiple phys on mdiobus

[Wolfgang Grandegger]

On 01/21/2013 12:12 PM, Florian Fainelli wrote:
> On 01/21/2013 12:07 PM, Wolfgang Grandegger wrote:
>> On 01/21/2013 11:07 AM, Sascha Hauer wrote:
>>> On Mon, Jan 21, 2013 at 09:56:24AM +0100, Wolfgang Grandegger wrote:
>>>> On 01/21/2013 09:37 AM, Sascha Hauer wrote:
>>>>> There may be multiple phys on an mdio bus. This series adds support
>>>>> for this to the fec driver. I recently had a board which has a switch
>>>>> connected to the fec's mdio bus, so I had to pick the correct phy.
>>>>
>>>> Pick one PHY from a switch port? Well, does a PHY-less (or fixed-link)
>>>> configuration for a switch not make more sense?
>>>
>>> Yes, you're probably right.
>>>
>>>> Various ARM Ethernet
>>>> contoller drivers do not support it. I recently needed a hack for an
>>>> AT91 board.
>>>
>>> I wonder how we want to proceed. Should there be a devicetree property
>>> 'fixed-link' like done for fs_enet (and not recommended for new code,
>>> stated in the comment above of_phy_connect_fixed_link)?
>>
>> Also the gianfar and ucc_geth drivers use this interface (via fixed
>> link phy). I tried to use it for the AT91 macb driver but stopped
>> quickly because the usage was not straight forward (too much code)...
>> even if the idea of using a fake fixed-link phy is not bad.
>>
>>> Currently I have a property 'phy' in the fec binding which has a phandle
>>> to a phy provided by the fec's mdio bus, but this could equally well
>>
>> But than the cable must be connected to the associated switch port.
>>
>>> point to a fixed dummy phy:
>>>
>>>     phy = &fixed-phy;
>>
>> The link speed, full/half duplex and maybe some mroe parameter should
>> be configurable via device tree.
>>
>>> Currently there seems to be no common convention for the devicetree how
>>> to handle such situations, or am I missing something?
>>
>> That's also may impression. There seem to be a few more related hacks:
>>
>> $ find . -name '*.c'| xargs grep -i "phy-less"
>> ./ethernet/amd/au1000_eth.c:            netdev_info(dev, "using
>> PHY-less setup\n");
>> ./ethernet/amd/au1000_eth.c:    } else { /* PHY-less op, assume
>> full-duplex */
>> ./ethernet/ibm/emac/core.c:    /* PHY-less configuration.
>> ./ethernet/ibm/emac/core.c:        /* PHY-less configuration.
>>
>> I would prefer to handle the "fixed-link" property of the ethernet dt
>> node directly in the driver with a generic helper function.
> 
> Is not what of_phy_connect_fixed_link() offer? I am not sure there can
> be much done by an helper than that.

Probably. I now remember that my primary problem with the AT91 board was
that it does not have yet device tree support.

Wolfgang.

Re: [PATCH] net: fec: Add support for multiple phys on mdiobus

[Sascha Hauer]

On Mon, Jan 21, 2013 at 12:12:52PM +0100, Florian Fainelli wrote:
> On 01/21/2013 12:07 PM, Wolfgang Grandegger wrote:
> >On 01/21/2013 11:07 AM, Sascha Hauer wrote:
> >>On Mon, Jan 21, 2013 at 09:56:24AM +0100, Wolfgang Grandegger wrote:
> >>>On 01/21/2013 09:37 AM, Sascha Hauer wrote:
> >>>>There may be multiple phys on an mdio bus. This series adds support
> >>>>for this to the fec driver. I recently had a board which has a switch
> >>>>connected to the fec's mdio bus, so I had to pick the correct phy.
> >>>
> >>>Pick one PHY from a switch port? Well, does a PHY-less (or fixed-link)
> >>>configuration for a switch not make more sense?
> >>
> >>Yes, you're probably right.
> >>
> >>>Various ARM Ethernet
> >>>contoller drivers do not support it. I recently needed a hack for an
> >>>AT91 board.
> >>
> >>I wonder how we want to proceed. Should there be a devicetree property
> >>'fixed-link' like done for fs_enet (and not recommended for new code,
> >>stated in the comment above of_phy_connect_fixed_link)?
> >
> >Also the gianfar and ucc_geth drivers use this interface (via fixed
> >link phy). I tried to use it for the AT91 macb driver but stopped
> >quickly because the usage was not straight forward (too much code)...
> >even if the idea of using a fake fixed-link phy is not bad.
> >
> >>Currently I have a property 'phy' in the fec binding which has a phandle
> >>to a phy provided by the fec's mdio bus, but this could equally well
> >
> >But than the cable must be connected to the associated switch port.
> >
> >>point to a fixed dummy phy:
> >>
> >>	phy = &fixed-phy;
> >
> >The link speed, full/half duplex and maybe some mroe parameter should
> >be configurable via device tree.
> >
> >>Currently there seems to be no common convention for the devicetree how
> >>to handle such situations, or am I missing something?
> >
> >That's also may impression. There seem to be a few more related hacks:
> >
> >$ find . -name '*.c'| xargs grep -i "phy-less"
> >./ethernet/amd/au1000_eth.c:			netdev_info(dev, "using PHY-less setup\n");
> >./ethernet/amd/au1000_eth.c:	} else { /* PHY-less op, assume full-duplex */
> >./ethernet/ibm/emac/core.c:	/* PHY-less configuration.
> >./ethernet/ibm/emac/core.c:		/* PHY-less configuration.
> >
> >I would prefer to handle the "fixed-link" property of the ethernet dt
> >node directly in the driver with a generic helper function.
> 
> Is not what of_phy_connect_fixed_link() offer? I am not sure there
> can be much done by an helper than that.

The comment above this function states:

/**
 * of_phy_connect_fixed_link - Parse fixed-link property and return a dummy phy
 * @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.
 */

And the commit introducing it has:

    Note: the dummy phy handling in arch/powerpc is a bit of a hack and
    needs to be reworked.  This function is being added now to solve the
    regression in the Ethernet drivers, but it should be considered a
    temporary measure until the fixed link handling can be reworked.


The 'temporary measure' exists for 3 1/2 years now ;)

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: [PATCH] net: fec: Add support for multiple phys on mdiobus

[Sascha Hauer]

On Mon, Jan 21, 2013 at 12:07:30PM +0100, Wolfgang Grandegger wrote:
> On 01/21/2013 11:07 AM, Sascha Hauer wrote:
> > On Mon, Jan 21, 2013 at 09:56:24AM +0100, Wolfgang Grandegger wrote:
> >> On 01/21/2013 09:37 AM, Sascha Hauer wrote:
> >>> There may be multiple phys on an mdio bus. This series adds support
> >>> for this to the fec driver. I recently had a board which has a switch
> >>> connected to the fec's mdio bus, so I had to pick the correct phy.
> >>
> >> Pick one PHY from a switch port? Well, does a PHY-less (or fixed-link)
> >> configuration for a switch not make more sense?
> > 
> > Yes, you're probably right.
> > 
> >> Various ARM Ethernet
> >> contoller drivers do not support it. I recently needed a hack for an
> >> AT91 board.
> > 
> > I wonder how we want to proceed. Should there be a devicetree property
> > 'fixed-link' like done for fs_enet (and not recommended for new code,
> > stated in the comment above of_phy_connect_fixed_link)?
> 
> Also the gianfar and ucc_geth drivers use this interface (via fixed
> link phy). I tried to use it for the AT91 macb driver but stopped
> quickly because the usage was not straight forward (too much code)...
> even if the idea of using a fake fixed-link phy is not bad.
> 
> > Currently I have a property 'phy' in the fec binding which has a phandle
> > to a phy provided by the fec's mdio bus, but this could equally well
> 
> But than the cable must be connected to the associated switch port.
> 
> > point to a fixed dummy phy:
> > 
> > 	phy = &fixed-phy;
> 
> The link speed, full/half duplex and maybe some mroe parameter should
> be configurable via device tree.

Well this could be done when the fixed phy driver could be registered
with the devicetree, maybe like this:

	fixed-phy: mdiophy {
		compatible = "mdio-fixed-phy";
		link = "100FD";
	};

The good thing about this would be that every ethernet driver could just
use such a fixed phy, any external mdio phy (like on Marvell Armada) or
just a phy connected to the internal mdio interface provided by the ethernet
core.

I probably should write a RFC to devicetree-discuss.

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: [PATCH] net: fec: Add support for multiple phys on mdiobus

[Wolfgang Grandegger]

On 01/21/2013 01:06 PM, Sascha Hauer wrote:
> On Mon, Jan 21, 2013 at 12:07:30PM +0100, Wolfgang Grandegger wrote:
>> On 01/21/2013 11:07 AM, Sascha Hauer wrote:
>>> On Mon, Jan 21, 2013 at 09:56:24AM +0100, Wolfgang Grandegger wrote:
>>>> On 01/21/2013 09:37 AM, Sascha Hauer wrote:
>>>>> There may be multiple phys on an mdio bus. This series adds support
>>>>> for this to the fec driver. I recently had a board which has a switch
>>>>> connected to the fec's mdio bus, so I had to pick the correct phy.
>>>>
>>>> Pick one PHY from a switch port? Well, does a PHY-less (or fixed-link)
>>>> configuration for a switch not make more sense?
>>>
>>> Yes, you're probably right.
>>>
>>>> Various ARM Ethernet
>>>> contoller drivers do not support it. I recently needed a hack for an
>>>> AT91 board.
>>>
>>> I wonder how we want to proceed. Should there be a devicetree property
>>> 'fixed-link' like done for fs_enet (and not recommended for new code,
>>> stated in the comment above of_phy_connect_fixed_link)?
>>
>> Also the gianfar and ucc_geth drivers use this interface (via fixed
>> link phy). I tried to use it for the AT91 macb driver but stopped
>> quickly because the usage was not straight forward (too much code)...
>> even if the idea of using a fake fixed-link phy is not bad.
>>
>>> Currently I have a property 'phy' in the fec binding which has a phandle
>>> to a phy provided by the fec's mdio bus, but this could equally well
>>
>> But than the cable must be connected to the associated switch port.
>>
>>> point to a fixed dummy phy:
>>>
>>> 	phy = &fixed-phy;
>>
>> The link speed, full/half duplex and maybe some mroe parameter should
>> be configurable via device tree.
> 
> Well this could be done when the fixed phy driver could be registered
> with the devicetree, maybe like this:
> 
> 	fixed-phy: mdiophy {
> 		compatible = "mdio-fixed-phy";
> 		link = "100FD";
> 	};

I find that confusing. There is *no* phy but just a fixed link to the
switch...

> The good thing about this would be that every ethernet driver could just
> use such a fixed phy, any external mdio phy (like on Marvell Armada) or
> just a phy connected to the internal mdio interface provided by the ethernet
> core.

What is wrong with the existing "fixed-link" property of the *ethernet*
node. The fixed-link handling should/could be done in the phy layer, and
not in the driver as it currently is implemented. Maybe that's the
reason why the current code is regarded as hack!

> I probably should write a RFC to devicetree-discuss.

Yep,

Wolfgang.

Re: [PATCH] net: fec: Add support for multiple phys on mdiobus

[Florian Fainelli]

On 01/22/2013 08:22 AM, Wolfgang Grandegger wrote:
>> Well this could be done when the fixed phy driver could be registered
>> with the devicetree, maybe like this:
>>
>> 	fixed-phy: mdiophy {
>> 		compatible = "mdio-fixed-phy";
>> 		link = "100FD";
>> 	};
>
> I find that confusing. There is *no* phy but just a fixed link to the
> switch...
>
>> The good thing about this would be that every ethernet driver could just
>> use such a fixed phy, any external mdio phy (like on Marvell Armada) or
>> just a phy connected to the internal mdio interface provided by the ethernet
>> core.
>
> What is wrong with the existing "fixed-link" property of the *ethernet*
> node. The fixed-link handling should/could be done in the phy layer, and
> not in the driver as it currently is implemented. Maybe that's the
> reason why the current code is regarded as hack!

As far as I have used it with the CPMAC driver, the fixed PHY is a 
specific PHY device and there is no specific handling to be done by the 
Ethernet MAC driver but eventually changing its MII bus id so that it is 
named "fixed-0" to allow the fixed PHY driver to bind. Put differently, 
using the fixed PHY driver is a kind of "last" resort thing to cope with 
situations like:

- switch not providing a consistent PHY-like interface on the MDC/MDIO bus
- switches not connected to the MDC/MDIO bus of the Ethernet MAC they 
forward to

What exactly do you mean by "as it currently is implemented"? that you 
do not like?
--
Florian

Re: [PATCH] net: fec: Add support for multiple phys on mdiobus

[Wolfgang Grandegger]

On 01/22/2013 03:47 PM, Florian Fainelli wrote:
> On 01/22/2013 08:22 AM, Wolfgang Grandegger wrote:
>>> Well this could be done when the fixed phy driver could be registered
>>> with the devicetree, maybe like this:
>>>
>>>     fixed-phy: mdiophy {
>>>         compatible = "mdio-fixed-phy";
>>>         link = "100FD";
>>>     };
>>
>> I find that confusing. There is *no* phy but just a fixed link to the
>> switch...
>>
>>> The good thing about this would be that every ethernet driver could just
>>> use such a fixed phy, any external mdio phy (like on Marvell Armada) or
>>> just a phy connected to the internal mdio interface provided by the
>>> ethernet
>>> core.
>>
>> What is wrong with the existing "fixed-link" property of the *ethernet*
>> node. The fixed-link handling should/could be done in the phy layer, and
>> not in the driver as it currently is implemented. Maybe that's the
>> reason why the current code is regarded as hack!
> 
> As far as I have used it with the CPMAC driver, the fixed PHY is a
> specific PHY device and there is no specific handling to be done by the
> Ethernet MAC driver but eventually changing its MII bus id so that it is
> named "fixed-0" to allow the fixed PHY driver to bind. Put differently,

There is special handling for the fixed link, e.g. here:

http://lxr.linux.no/#linux+v3.7.4/drivers/net/ethernet/freescale/gianfar.c#L1462

This could be hidden in the PHY layer allowing all ethernet drivers
using the "fixed-link" property.

> using the fixed PHY driver is a kind of "last" resort thing to cope with
> situations like:
> 
> - switch not providing a consistent PHY-like interface on the MDC/MDIO bus
> - switches not connected to the MDC/MDIO bus of the Ethernet MAC they
> forward to
> 
> What exactly do you mean by "as it currently is implemented"? that you
> do not like?

As Sascha pointed out, the implementation is labeled as "hack", which
should be fixed sooner than later. It's just not clear why ;).

Wolfgang.

Re: [PATCH] net: fec: Add support for multiple phys on mdiobus

[Florian Fainelli]

On 01/22/2013 04:03 PM, Wolfgang Grandegger wrote:
> On 01/22/2013 03:47 PM, Florian Fainelli wrote:
>> On 01/22/2013 08:22 AM, Wolfgang Grandegger wrote:
>>>> Well this could be done when the fixed phy driver could be registered
>>>> with the devicetree, maybe like this:
>>>>
>>>>      fixed-phy: mdiophy {
>>>>          compatible = "mdio-fixed-phy";
>>>>          link = "100FD";
>>>>      };
>>> I find that confusing. There is *no* phy but just a fixed link to the
>>> switch...
>>>
>>>> The good thing about this would be that every ethernet driver could just
>>>> use such a fixed phy, any external mdio phy (like on Marvell Armada) or
>>>> just a phy connected to the internal mdio interface provided by the
>>>> ethernet
>>>> core.
>>> What is wrong with the existing "fixed-link" property of the *ethernet*
>>> node. The fixed-link handling should/could be done in the phy layer, and
>>> not in the driver as it currently is implemented. Maybe that's the
>>> reason why the current code is regarded as hack!
>> As far as I have used it with the CPMAC driver, the fixed PHY is a
>> specific PHY device and there is no specific handling to be done by the
>> Ethernet MAC driver but eventually changing its MII bus id so that it is
>> named "fixed-0" to allow the fixed PHY driver to bind. Put differently,
> There is special handling for the fixed link, e.g. here:
>
> http://lxr.linux.no/#linux+v3.7.4/drivers/net/ethernet/freescale/gianfar.c#L1462
>
> This could be hidden in the PHY layer allowing all ethernet drivers
> using the "fixed-link" property.

Ok, so in the end you could have potentially an ethernet PHY node 
containing the following properties:
- fixed-link
- fixed-speed
- fixed-duplex

and treat it as a fixed-phy. If so, this would be pretty handy.

>
>> using the fixed PHY driver is a kind of "last" resort thing to cope with
>> situations like:
>>
>> - switch not providing a consistent PHY-like interface on the MDC/MDIO bus
>> - switches not connected to the MDC/MDIO bus of the Ethernet MAC they
>> forward to
>>
>> What exactly do you mean by "as it currently is implemented"? that you
>> do not like?
> As Sascha pointed out, the implementation is labeled as "hack", which
> should be fixed sooner than later. It's just not clear why ;).
>
> Wolfgang.
>
> --
> 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: [PATCH] net: fec: Add support for multiple phys on mdiobus

[Wolfgang Grandegger]

On 01/22/2013 04:19 PM, Florian Fainelli wrote:
> On 01/22/2013 04:03 PM, Wolfgang Grandegger wrote:
>> On 01/22/2013 03:47 PM, Florian Fainelli wrote:
>>> On 01/22/2013 08:22 AM, Wolfgang Grandegger wrote:
>>>>> Well this could be done when the fixed phy driver could be registered
>>>>> with the devicetree, maybe like this:
>>>>>
>>>>>      fixed-phy: mdiophy {
>>>>>          compatible = "mdio-fixed-phy";
>>>>>          link = "100FD";
>>>>>      };
>>>> I find that confusing. There is *no* phy but just a fixed link to the
>>>> switch...
>>>>
>>>>> The good thing about this would be that every ethernet driver could
>>>>> just
>>>>> use such a fixed phy, any external mdio phy (like on Marvell
>>>>> Armada) or
>>>>> just a phy connected to the internal mdio interface provided by the
>>>>> ethernet
>>>>> core.
>>>> What is wrong with the existing "fixed-link" property of the *ethernet*
>>>> node. The fixed-link handling should/could be done in the phy layer,
>>>> and
>>>> not in the driver as it currently is implemented. Maybe that's the
>>>> reason why the current code is regarded as hack!
>>> As far as I have used it with the CPMAC driver, the fixed PHY is a
>>> specific PHY device and there is no specific handling to be done by the
>>> Ethernet MAC driver but eventually changing its MII bus id so that it is
>>> named "fixed-0" to allow the fixed PHY driver to bind. Put differently,
>> There is special handling for the fixed link, e.g. here:
>>
>> http://lxr.linux.no/#linux+v3.7.4/drivers/net/ethernet/freescale/gianfar.c#L1462
>>
>>
>> This could be hidden in the PHY layer allowing all ethernet drivers
>> using the "fixed-link" property.
> 
> Ok, so in the end you could have potentially an ethernet PHY node
> containing the following properties:
> - fixed-link
> - fixed-speed
> - fixed-duplex
> 
> and treat it as a fixed-phy. If so, this would be pretty handy.

The "fixed-link" property already allows to specify speed, duplex, etc.:

http://lxr.linux.no/#linux+v3.7.4/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt#L48

Wolfgang.