ethernet-phy-ieee802.3-c22 binding and reset-gpios

ethernet-phy-ieee802.3-c22 binding and reset-gpios

[Sascha Hauer]

Hi All,

I am using the ethernet phy binding here that looks like:

ethphy1: ethernet-phy@1 {
	compatible = "ethernet-phy-ieee802.3-c22";
	reg = <1>;
	eee-broken-1000t;
	reset-gpios = <&gpio4 2 GPIO_ACTIVE_LOW>;
};

It seems the "reset-gpios" is inherently broken in Linux. With the above
in one way or the other we end up in of_mdiobus_register_phy() where we
have:

        if (!is_c45 && !of_get_phy_id(child, &phy_id))
                phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
        else
                phy = get_phy_device(mdio, addr, is_c45);

get_phy_device() tries to read the phy_id from the MDIO bus.
Unfortunately the "reset-gpios" property is only handled later in
phy_device_register(), so it can only work when the bootloader has left
the reset GPIO deasserted.  Note it works when of_get_phy_id() returns
the phy_id, which is the case when the phy_id is explicitly given with
"ethernet-phy-idxxxx.yyyy", but giving the exact phy_id shouldn't be
mandatory, right?

I think the phy_id is unknown at phy_device_create() time, so I looked
into removing the phy_id argument there and specifying it elsewhere,
maybe by adding a phy_device_set_id() function. Another point is that
phy_device_create() currently calls phy_request_driver_module() to get
the driver modules for a given phy_id. That would have to be called
later as well. Is this the path to go or are there any other ideas how
to solve this issue?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: ethernet-phy-ieee802.3-c22 binding and reset-gpios

[Andrew Lunn]

On Tue, Aug 25, 2020 at 11:09:33AM +0200, Sascha Hauer wrote:
> Hi All,
> 
> I am using the ethernet phy binding here that looks like:
> 
> ethphy1: ethernet-phy@1 {
> 	compatible = "ethernet-phy-ieee802.3-c22";
> 	reg = <1>;
> 	eee-broken-1000t;
> 	reset-gpios = <&gpio4 2 GPIO_ACTIVE_LOW>;
> };
> 
> It seems the "reset-gpios" is inherently broken in Linux.

Hi Sascha

I think it would be better to say, it does not do what people expect,
rather than broken.

This code was developed for a PHY which needed to be reset after
enumeration. That PHY did enumerate, either because it was not held in
reset, or would still answer ID requests while held in reset.

It does however not work for PHYs which are held in reset during probe
and won't enumerate. This is a known issues, but could be better
documented.

> Is this the path to go or are there any other ideas how to solve
> this issue?

There is two different reset gpios in DT. There is a per PHY reset,
which you are trying to use. And a per MDIO bus reset, which should
apply to all PHYs on the bus. This per bus reset works more as
expected. If this works for you, you could use that.

Otherwise, you need to modify of_mdiobus_register() to look in device
tree while it is performing the scan and see if there is a reset
property for each address on the bus. If so, take the device out of
reset before reading the ID registers.

	Andrew

Re: ethernet-phy-ieee802.3-c22 binding and reset-gpios

[Sascha Hauer]

Hi Andrew,

On Tue, Aug 25, 2020 at 03:14:00PM +0200, Andrew Lunn wrote:
> On Tue, Aug 25, 2020 at 11:09:33AM +0200, Sascha Hauer wrote:
> > Hi All,
> > 
> > I am using the ethernet phy binding here that looks like:
> > 
> > ethphy1: ethernet-phy@1 {
> > 	compatible = "ethernet-phy-ieee802.3-c22";
> > 	reg = <1>;
> > 	eee-broken-1000t;
> > 	reset-gpios = <&gpio4 2 GPIO_ACTIVE_LOW>;
> > };
> > 
> > It seems the "reset-gpios" is inherently broken in Linux.
> 
> Hi Sascha
> 
> I think it would be better to say, it does not do what people expect,
> rather than broken.
> 
> This code was developed for a PHY which needed to be reset after
> enumeration. That PHY did enumerate, either because it was not held in
> reset, or would still answer ID requests while held in reset.
> 
> It does however not work for PHYs which are held in reset during probe
> and won't enumerate. This is a known issues, but could be better
> documented.

I think the behaviour should rather be improved than documented.

> 
> > Is this the path to go or are there any other ideas how to solve
> > this issue?
> 
> There is two different reset gpios in DT. There is a per PHY reset,
> which you are trying to use. And a per MDIO bus reset, which should
> apply to all PHYs on the bus. This per bus reset works more as
> expected. If this works for you, you could use that.

Well there is only one phy connected to the bus, so it makes no
difference if I say the reset GPIO is for the whole bus or for a single
phy only. The per bus reset should work, but currently it doesn't. First
reason I found out that mdiobus_register() doesn't handle -EPROBE_DEFER
returned by the devm_gpiod_get_optional() properly, patch follows.
Second reason is that the phy is not detected (id read returns 0xffff)
when the reset is attached to the bus. So far I haven't found the reason
for that.

> 
> Otherwise, you need to modify of_mdiobus_register() to look in device
> tree while it is performing the scan and see if there is a reset
> property for each address on the bus. If so, take the device out of
> reset before reading the ID registers.

Ok.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: ethernet-phy-ieee802.3-c22 binding and reset-gpios

[Andrew Lunn]

On Wed, Aug 26, 2020 at 10:58:57AM +0200, Sascha Hauer wrote:
> Hi Andrew,
> 
> Well there is only one phy connected to the bus, so it makes no
> difference if I say the reset GPIO is for the whole bus or for a single
> phy only. The per bus reset should work, but currently it doesn't. First
> reason I found out that mdiobus_register() doesn't handle -EPROBE_DEFER
> returned by the devm_gpiod_get_optional() properly, patch follows.

Thanks

> Second reason is that the phy is not detected (id read returns 0xffff)
> when the reset is attached to the bus. So far I haven't found the reason
> for that.

Try giving the PHY a bit longer to recover from the reset before
probing it.

And what SoC are you using? Is it FEC ethernet driver? That has MDIO
issues at the moment.

     Andrew