The mdio_bus reset code first de-asserted the reset by allocating with
GPIOD_OUT_LOW, then asserted and de-asserted again. In other words, if
the reset signal defaulted to asserted, there'd be a short "spike"
before the reset.
Instead, directly assert the reset signal using GPIOD_OUT_HIGH, this
removes the spike and also removes a line of code since the signal
is already high.
Signed-off-by: Mike Looijmans <[email protected]>
---
drivers/net/phy/mdio_bus.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 2b42e46066b4..34e98ae75110 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -543,8 +543,8 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
mutex_init(&bus->mdio_lock);
mutex_init(&bus->shared_lock);
- /* de-assert bus level PHY GPIO reset */
- gpiod = devm_gpiod_get_optional(&bus->dev, "reset", GPIOD_OUT_LOW);
+ /* assert bus level PHY GPIO reset */
+ gpiod = devm_gpiod_get_optional(&bus->dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(gpiod)) {
err = dev_err_probe(&bus->dev, PTR_ERR(gpiod),
"mii_bus %s couldn't get reset GPIO\n",
@@ -553,8 +553,6 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
return err;
} else if (gpiod) {
bus->reset_gpiod = gpiod;
-
- gpiod_set_value_cansleep(gpiod, 1);
fsleep(bus->reset_delay_us);
gpiod_set_value_cansleep(gpiod, 0);
if (bus->reset_post_delay_us > 0)
--
2.17.1
On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
> The mdio_bus reset code first de-asserted the reset by allocating with
> GPIOD_OUT_LOW, then asserted and de-asserted again. In other words, if
> the reset signal defaulted to asserted, there'd be a short "spike"
> before the reset.
>
> Instead, directly assert the reset signal using GPIOD_OUT_HIGH, this
> removes the spike and also removes a line of code since the signal
> is already high.
Hi Mike
Did you look at the per PHY reset? mdiobus_register_gpiod() gets the
GPIO with GPIOD_OUT_LOW. mdiobus_register_device() then immediately
sets it high.
So it looks like it suffers from the same problem.
Andrew
Hi Andrew,
Response below...
Met vriendelijke groet / kind regards,
Mike Looijmans
System Expert
TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands
T: +31 (0) 499 33 69 69
E: [email protected]
W: http://www.topicproducts.com
Please consider the environment before printing this e-mail
On 28-01-2021 02:56, Andrew Lunn wrote:
> On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
>> The mdio_bus reset code first de-asserted the reset by allocating with
>> GPIOD_OUT_LOW, then asserted and de-asserted again. In other words, if
>> the reset signal defaulted to asserted, there'd be a short "spike"
>> before the reset.
>>
>> Instead, directly assert the reset signal using GPIOD_OUT_HIGH, this
>> removes the spike and also removes a line of code since the signal
>> is already high.
>
> Hi Mike
>
> Did you look at the per PHY reset? mdiobus_register_gpiod() gets the
> GPIO with GPIOD_OUT_LOW. mdiobus_register_device() then immediately
> sets it high.
>
> So it looks like it suffers from the same problem.
Well, now that I have your attention...
The per PHY reset was more broken, it first probes the MDIO bus to see if the
PHY is there, and only after that it controls the reset line. So if the reset
defaults to "asserted", the PHY will not work because it didn't respond when
the MDIO went looking for it. I haven't quite figured out how this was
supposed to work, but at least for the case of one MDIO bus, one PHY
configured through devicetree it didn't work as one would expect. I added a
few printk statements to reveal that this was indeed the case.
This issue also makes the PHY hardware reset useless - if the PHY is in some
non-responsive state, the MDIO won't get a response and report the PHY as
missing before even attempting to assert/de-assert the reset line.
This was with a 5.4 kernel, but as far as I could see this hasn't changed
since then.
My solution to that was to move to the MDIO bus reset, since that at least
happened before interrogating the devices on the bus. This revealed the issue
with the extra "spike" while evaluating that, which is something that I could
easily solve and upstream.
Probably these issues were never dicovered because usually there's a pull-up
of some kind on the (active-low) reset signal of the PHYs. That hides the
spike and also hides the fact that the per-phy reset doesn't actually work. We
only discovered the issue when we changed that to a pull-down and suddenly the
phy failed to probe.
The way that the MDIO bus is being populated seemed rather complex to me, so
chances of breaking things are quite high there...
On Thu, Jan 28, 2021 at 09:45:41AM +0100, Mike Looijmans wrote:
> Hi Andrew,
>
> Response below...
Hi Mike
Everybody here knows that top posting is evil, we don't do it. We
expect the replay to be inline.
> > Hi Mike
> >
> > Did you look at the per PHY reset? mdiobus_register_gpiod() gets the
> > GPIO with GPIOD_OUT_LOW. mdiobus_register_device() then immediately
> > sets it high.
> >
> > So it looks like it suffers from the same problem.
>
> Well, now that I have your attention...
>
> The per PHY reset was more broken
It has history. It was designed to be used for PHYs which needed a
reset after the clock was changed. It assumed the PHY would probe,
which some do when held in reset.
But the GPIO is not the only problem. Some PHYs need a regulator
enabled, some need a clock enabled. The core has no idea what order to
do this in. It should be the PHY driver that does this, since it
should have knowledge of the PHY, and can do things in the correct
order. But if the PHY does not respond, it is not discovered, and so
the driver does not load. If that case, you can put the PHY ID into
the compatible string, and the core will load the correct driver and
probe it, allow it to turn on whatever it needs.
This has been discussed a few times and this is what we decided on.
Maybe we need to improve the documentation.
Andrew