[PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[Mike Looijmans]

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 <mike.looijmans@topic.nl>

---

 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

Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[Andrew Lunn]

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

This however appears to remove the reset pulse, if the reset line was
already low to start with. Notice you left

fsleep(bus->reset_delay_us);

without any action before it? What are we now waiting for?  Most data
sheets talk of a reset pulse. Take the reset line high, wait for some
time, take the reset low, wait for some time, and then start talking
to the PHY. I think with this patch, we have lost the guarantee of a
low to high transition.

Is this spike, followed by a pulse actually causing you problems? If
so, i would actually suggest adding another delay, to stretch the
spike. We have no control over the initial state of the reset line, it
is how the bootloader left it, we have to handle both states.

   Andrew

Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[Russell King - ARM Linux admin]

On Tue, Jan 26, 2021 at 02:14:40PM +0100, 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
> 
> This however appears to remove the reset pulse, if the reset line was
> already low to start with. Notice you left
> 
> fsleep(bus->reset_delay_us);
> 
> without any action before it? What are we now waiting for?  Most data
> sheets talk of a reset pulse. Take the reset line high, wait for some
> time, take the reset low, wait for some time, and then start talking
> to the PHY. I think with this patch, we have lost the guarantee of a
> low to high transition.
> 
> Is this spike, followed by a pulse actually causing you problems? If
> so, i would actually suggest adding another delay, to stretch the
> spike. We have no control over the initial state of the reset line, it
> is how the bootloader left it, we have to handle both states.

Andrew, I don't get what you're saying.

Here is what happens depending on the pre-existing state of the
reset signal:

Reset (previously asserted):   ~~~|_|~~~~|_______
Reset (previously deasserted): _____|~~~~|_______
                                  ^ ^    ^
                                  A B    C

At point A, the low going transition is because the reset line is
requested using GPIOD_OUT_LOW. If the line is successfully requested,
the first thing we do is set it high _without_ any delay. This is
point B. So, a glitch occurs between A and B.

We then fsleep() and finally set the GPIO low at point C.

Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
transitions. Instead we get:

Reset (previously asserted)  : ~~~~~~~~~~|______
Reset (previously deasserted): ____|~~~~~|______
                                   ^     ^
                                   A     C

Where A and C are the points described above in the code. Point B
has been eliminated.

Therefore, to me the patch looks entirely reasonable and correct.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[Mike Looijmans]

See 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: mike.looijmans@topicproducts.com
W: www.topicproducts.com

Please consider the environment before printing this e-mail
On 26-01-2021 14:49, Russell King - ARM Linux admin wrote:
> On Tue, Jan 26, 2021 at 02:14:40PM +0100, 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
>>
>> This however appears to remove the reset pulse, if the reset line was
>> already low to start with. Notice you left
>>
>> fsleep(bus->reset_delay_us);
>>
>> without any action before it? What are we now waiting for?  Most data
>> sheets talk of a reset pulse. Take the reset line high, wait for some
>> time, take the reset low, wait for some time, and then start talking
>> to the PHY. I think with this patch, we have lost the guarantee of a
>> low to high transition.
>>
>> Is this spike, followed by a pulse actually causing you problems? If
>> so, i would actually suggest adding another delay, to stretch the
>> spike. We have no control over the initial state of the reset line, it
>> is how the bootloader left it, we have to handle both states.
> Andrew, I don't get what you're saying.
>
> Here is what happens depending on the pre-existing state of the
> reset signal:
>
> Reset (previously asserted):   ~~~|_|~~~~|_______
> Reset (previously deasserted): _____|~~~~|_______
>                                    ^ ^    ^
>                                    A B    C
>
> At point A, the low going transition is because the reset line is
> requested using GPIOD_OUT_LOW. If the line is successfully requested,
> the first thing we do is set it high _without_ any delay. This is
> point B. So, a glitch occurs between A and B.
>
> We then fsleep() and finally set the GPIO low at point C.
>
> Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
> transitions. Instead we get:
>
> Reset (previously asserted)  : ~~~~~~~~~~|______
> Reset (previously deasserted): ____|~~~~~|______
>                                     ^     ^
>                                     A     C
>
> Where A and C are the points described above in the code. Point B
> has been eliminated.
>
> Therefore, to me the patch looks entirely reasonable and correct.
>
Thanks, excellent explanation.

As a bit of background, we were using a Marvell PHY where the datasheet 
states that thou shallt not release the reset within 50 ms of power-up. 
A pull-down on the active-low reset was thus added. Looking at the reset 
signal with a scope revealed a short spike, visible only because it was 
being controlled by an I2C GPIO expander. So it's indeed point "B" that 
we wanted to eliminate.


-- 
Mike Looijmans

Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[Jakub Kicinski]

On Wed, 27 Jan 2021 08:08:29 +0100 Mike Looijmans wrote:
> > Andrew, I don't get what you're saying.
> >
> > Here is what happens depending on the pre-existing state of the
> > reset signal:
> >
> > Reset (previously asserted):   ~~~|_|~~~~|_______
> > Reset (previously deasserted): _____|~~~~|_______
> >                                    ^ ^    ^
> >                                    A B    C
> >
> > At point A, the low going transition is because the reset line is
> > requested using GPIOD_OUT_LOW. If the line is successfully requested,
> > the first thing we do is set it high _without_ any delay. This is
> > point B. So, a glitch occurs between A and B.
> >
> > We then fsleep() and finally set the GPIO low at point C.
> >
> > Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
> > transitions. Instead we get:
> >
> > Reset (previously asserted)  : ~~~~~~~~~~|______
> > Reset (previously deasserted): ____|~~~~~|______
> >                                     ^     ^
> >                                     A     C
> >
> > Where A and C are the points described above in the code. Point B
> > has been eliminated.
> >
> > Therefore, to me the patch looks entirely reasonable and correct.
> >  
> Thanks, excellent explanation.
> 
> As a bit of background, we were using a Marvell PHY where the datasheet 
> states that thou shallt not release the reset within 50 ms of power-up. 
> A pull-down on the active-low reset was thus added. Looking at the reset 
> signal with a scope revealed a short spike, visible only because it was 
> being controlled by an I2C GPIO expander. So it's indeed point "B" that 
> we wanted to eliminate.

This is all useful information - can we roll more of it into the commit
message? I'd think that calling out the part and the 50ms value could
make things more "concrete" for a reader down the line?

Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[Andrew Lunn]

On Tue, Jan 26, 2021 at 01:49:38PM +0000, Russell King - ARM Linux admin wrote:
> On Tue, Jan 26, 2021 at 02:14:40PM +0100, 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
> > 
> > This however appears to remove the reset pulse, if the reset line was
> > already low to start with. Notice you left
> > 
> > fsleep(bus->reset_delay_us);
> > 
> > without any action before it? What are we now waiting for?  Most data
> > sheets talk of a reset pulse. Take the reset line high, wait for some
> > time, take the reset low, wait for some time, and then start talking
> > to the PHY. I think with this patch, we have lost the guarantee of a
> > low to high transition.
> > 
> > Is this spike, followed by a pulse actually causing you problems? If
> > so, i would actually suggest adding another delay, to stretch the
> > spike. We have no control over the initial state of the reset line, it
> > is how the bootloader left it, we have to handle both states.
> 
> Andrew, I don't get what you're saying.
> 
> Here is what happens depending on the pre-existing state of the
> reset signal:
> 
> Reset (previously asserted):   ~~~|_|~~~~|_______
> Reset (previously deasserted): _____|~~~~|_______
>                                   ^ ^    ^
>                                   A B    C
> 
> At point A, the low going transition is because the reset line is
> requested using GPIOD_OUT_LOW. If the line is successfully requested,
> the first thing we do is set it high _without_ any delay. This is
> point B. So, a glitch occurs between A and B.
> 
> We then fsleep() and finally set the GPIO low at point C.
> 
> Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
> transitions. Instead we get:
> 
> Reset (previously asserted)  : ~~~~~~~~~~|______
> Reset (previously deasserted): ____|~~~~~|______
>                                    ^     ^
>                                    A     C
> 
> Where A and C are the points described above in the code. Point B
> has been eliminated.
> 
> Therefore, to me the patch looks entirely reasonable and correct.

I wonder if there are any PHYs which actually need a pulse? Would it
be better to have:

 Reset (previously asserted):   ~~~|____|~~~~|_______
 Reset (previously deasserted): ________|~~~~|_______
                                   ^    ^    ^    ^
                                   A    B    C    D

Point D is where we actually start talking to the PHY. C-D is
reset-post-delay-us, and defaults to 0, but can be set via DT.  B-C is
reset-delay-us, and defaults to 10us, but can be set via DT.
Currently A-B is '0', so we get the glitch. But should we make A-B the
same as B-C, so we get a real pulse?

     Andrew

Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[Russell King - ARM Linux admin]

On Thu, Jan 28, 2021 at 01:00:57AM +0100, Andrew Lunn wrote:
> On Tue, Jan 26, 2021 at 01:49:38PM +0000, Russell King - ARM Linux admin wrote:
> > On Tue, Jan 26, 2021 at 02:14:40PM +0100, 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
> > > 
> > > This however appears to remove the reset pulse, if the reset line was
> > > already low to start with. Notice you left
> > > 
> > > fsleep(bus->reset_delay_us);
> > > 
> > > without any action before it? What are we now waiting for?  Most data
> > > sheets talk of a reset pulse. Take the reset line high, wait for some
> > > time, take the reset low, wait for some time, and then start talking
> > > to the PHY. I think with this patch, we have lost the guarantee of a
> > > low to high transition.
> > > 
> > > Is this spike, followed by a pulse actually causing you problems? If
> > > so, i would actually suggest adding another delay, to stretch the
> > > spike. We have no control over the initial state of the reset line, it
> > > is how the bootloader left it, we have to handle both states.
> > 
> > Andrew, I don't get what you're saying.
> > 
> > Here is what happens depending on the pre-existing state of the
> > reset signal:
> > 
> > Reset (previously asserted):   ~~~|_|~~~~|_______
> > Reset (previously deasserted): _____|~~~~|_______
> >                                   ^ ^    ^
> >                                   A B    C
> > 
> > At point A, the low going transition is because the reset line is
> > requested using GPIOD_OUT_LOW. If the line is successfully requested,
> > the first thing we do is set it high _without_ any delay. This is
> > point B. So, a glitch occurs between A and B.
> > 
> > We then fsleep() and finally set the GPIO low at point C.
> > 
> > Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
> > transitions. Instead we get:
> > 
> > Reset (previously asserted)  : ~~~~~~~~~~|______
> > Reset (previously deasserted): ____|~~~~~|______
> >                                    ^     ^
> >                                    A     C
> > 
> > Where A and C are the points described above in the code. Point B
> > has been eliminated.
> > 
> > Therefore, to me the patch looks entirely reasonable and correct.
> 
> I wonder if there are any PHYs which actually need a pulse? Would it
> be better to have:
> 
>  Reset (previously asserted):   ~~~|____|~~~~|_______
>  Reset (previously deasserted): ________|~~~~|_______
>                                    ^    ^    ^    ^
>                                    A    B    C    D
> 
> Point D is where we actually start talking to the PHY. C-D is
> reset-post-delay-us, and defaults to 0, but can be set via DT.  B-C is
> reset-delay-us, and defaults to 10us, but can be set via DT.
> Currently A-B is '0', so we get the glitch. But should we make A-B the
> same as B-C, so we get a real pulse?

I do not see any need for A-B - what is the reason for it? You will
find most datasheets talk about a clock must be active for some number
of clock cycles prior to the reset signal being released, or minimum
delay after power up before reset is released, or talking about a
minimum pulse width.

Note that looking at a few of the Marvell PHY datasheets, they require
a minimum reset pulse width of 10ms and between 5ms and 50ms before
the first access. AR8035 also talks about 10ms.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[Andrew Lunn]

On Thu, Jan 28, 2021 at 12:25:55AM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Jan 28, 2021 at 01:00:57AM +0100, Andrew Lunn wrote:
> > On Tue, Jan 26, 2021 at 01:49:38PM +0000, Russell King - ARM Linux admin wrote:
> > > On Tue, Jan 26, 2021 at 02:14:40PM +0100, 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
> > > > 
> > > > This however appears to remove the reset pulse, if the reset line was
> > > > already low to start with. Notice you left
> > > > 
> > > > fsleep(bus->reset_delay_us);
> > > > 
> > > > without any action before it? What are we now waiting for?  Most data
> > > > sheets talk of a reset pulse. Take the reset line high, wait for some
> > > > time, take the reset low, wait for some time, and then start talking
> > > > to the PHY. I think with this patch, we have lost the guarantee of a
> > > > low to high transition.
> > > > 
> > > > Is this spike, followed by a pulse actually causing you problems? If
> > > > so, i would actually suggest adding another delay, to stretch the
> > > > spike. We have no control over the initial state of the reset line, it
> > > > is how the bootloader left it, we have to handle both states.
> > > 
> > > Andrew, I don't get what you're saying.
> > > 
> > > Here is what happens depending on the pre-existing state of the
> > > reset signal:
> > > 
> > > Reset (previously asserted):   ~~~|_|~~~~|_______
> > > Reset (previously deasserted): _____|~~~~|_______
> > >                                   ^ ^    ^
> > >                                   A B    C
> > > 
> > > At point A, the low going transition is because the reset line is
> > > requested using GPIOD_OUT_LOW. If the line is successfully requested,
> > > the first thing we do is set it high _without_ any delay. This is
> > > point B. So, a glitch occurs between A and B.
> > > 
> > > We then fsleep() and finally set the GPIO low at point C.
> > > 
> > > Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
> > > transitions. Instead we get:
> > > 
> > > Reset (previously asserted)  : ~~~~~~~~~~|______
> > > Reset (previously deasserted): ____|~~~~~|______
> > >                                    ^     ^
> > >                                    A     C
> > > 
> > > Where A and C are the points described above in the code. Point B
> > > has been eliminated.
> > > 
> > > Therefore, to me the patch looks entirely reasonable and correct.
> > 
> > I wonder if there are any PHYs which actually need a pulse? Would it
> > be better to have:
> > 
> >  Reset (previously asserted):   ~~~|____|~~~~|_______
> >  Reset (previously deasserted): ________|~~~~|_______
> >                                    ^    ^    ^    ^
> >                                    A    B    C    D
> > 
> > Point D is where we actually start talking to the PHY. C-D is
> > reset-post-delay-us, and defaults to 0, but can be set via DT.  B-C is
> > reset-delay-us, and defaults to 10us, but can be set via DT.
> > Currently A-B is '0', so we get the glitch. But should we make A-B the
> > same as B-C, so we get a real pulse?
> 
> I do not see any need for A-B - what is the reason for it?

If level is all that matters, then it is not needed. If a PHY needs an
actual pulse, both a raising and a falling edge, we potentially don't
get the rising edge now.

But the datasheets you have looked at all seem to talk about level,
not pulse. So lets go with this.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[Mike Looijmans]

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: mike.looijmans@topicproducts.com
W: www.topicproducts.com

Please consider the environment before printing this e-mail
On 28-01-2021 02:12, Andrew Lunn wrote:
> On Thu, Jan 28, 2021 at 12:25:55AM +0000, Russell King - ARM Linux admin wrote:
>> On Thu, Jan 28, 2021 at 01:00:57AM +0100, Andrew Lunn wrote:
>>> On Tue, Jan 26, 2021 at 01:49:38PM +0000, Russell King - ARM Linux admin wrote:
>>>> On Tue, Jan 26, 2021 at 02:14:40PM +0100, 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
>>>>>
>>>>> This however appears to remove the reset pulse, if the reset line was
>>>>> already low to start with. Notice you left
>>>>>
>>>>> fsleep(bus->reset_delay_us);
>>>>>
>>>>> without any action before it? What are we now waiting for?  Most data
>>>>> sheets talk of a reset pulse. Take the reset line high, wait for some
>>>>> time, take the reset low, wait for some time, and then start talking
>>>>> to the PHY. I think with this patch, we have lost the guarantee of a
>>>>> low to high transition.
>>>>>
>>>>> Is this spike, followed by a pulse actually causing you problems? If
>>>>> so, i would actually suggest adding another delay, to stretch the
>>>>> spike. We have no control over the initial state of the reset line, it
>>>>> is how the bootloader left it, we have to handle both states.
>>>> Andrew, I don't get what you're saying.
>>>>
>>>> Here is what happens depending on the pre-existing state of the
>>>> reset signal:
>>>>
>>>> Reset (previously asserted):   ~~~|_|~~~~|_______
>>>> Reset (previously deasserted): _____|~~~~|_______
>>>>                                    ^ ^    ^
>>>>                                    A B    C
>>>>
>>>> At point A, the low going transition is because the reset line is
>>>> requested using GPIOD_OUT_LOW. If the line is successfully requested,
>>>> the first thing we do is set it high _without_ any delay. This is
>>>> point B. So, a glitch occurs between A and B.
>>>>
>>>> We then fsleep() and finally set the GPIO low at point C.
>>>>
>>>> Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
>>>> transitions. Instead we get:
>>>>
>>>> Reset (previously asserted)  : ~~~~~~~~~~|______
>>>> Reset (previously deasserted): ____|~~~~~|______
>>>>                                     ^     ^
>>>>                                     A     C
>>>>
>>>> Where A and C are the points described above in the code. Point B
>>>> has been eliminated.
>>>>
>>>> Therefore, to me the patch looks entirely reasonable and correct.
>>> I wonder if there are any PHYs which actually need a pulse? Would it
>>> be better to have:
>>>
>>>   Reset (previously asserted):   ~~~|____|~~~~|_______
>>>   Reset (previously deasserted): ________|~~~~|_______
>>>                                     ^    ^    ^    ^
>>>                                     A    B    C    D
>>>
>>> Point D is where we actually start talking to the PHY. C-D is
>>> reset-post-delay-us, and defaults to 0, but can be set via DT.  B-C is
>>> reset-delay-us, and defaults to 10us, but can be set via DT.
>>> Currently A-B is '0', so we get the glitch. But should we make A-B the
>>> same as B-C, so we get a real pulse?
>> I do not see any need for A-B - what is the reason for it?
> If level is all that matters, then it is not needed. If a PHY needs an
> actual pulse, both a raising and a falling edge, we potentially don't
> get the rising edge now.

We only caught the "spike" because the reset GPIO was controlled by a 
GPIO expander, so it took about a millisecond to toggle it. With a 
"local" GPIO controller, the pulse duration would be below the 
microsecond range and most PHYs would never see it.

> But the datasheets you have looked at all seem to talk about level,
> not pulse. So lets go with this.
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>
>      Andrew

Just wondering, now, a v2 patch isn't needed? Or should I amend the 
commit text?


-- 
Mike Looijmans

Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[Andrew Lunn]

> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > 
> >      Andrew
> 
> Just wondering, now, a v2 patch isn't needed? Or should I amend the commit
> text?

Hi Mike

Take a look in patchwork.kernel.org. It has been flaky the last few
days. If the patch is not there, you definitively need to repost. If
you do find it, check what state it is in. That will tell you if it
needs reposting.

      Andrew

Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[Andrew Lunn]

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

Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[Mike Looijmans]

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: mike.looijmans@topicproducts.com
W: 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...

Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[Andrew Lunn]

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