From: Mike Looijmans <hidden> Date: 2021-01-26 17:32:14
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 <redacted>
---
drivers/net/phy/mdio_bus.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-01-26 13:19:37
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
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2021-01-26 13:53:52
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:
quoted
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!
From: Mike Looijmans <hidden> Date: 2021-01-27 07:16:36
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:
quoted
On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
quoted
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
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-01-27 22:56:16
On Wed, 27 Jan 2021 08:08:29 +0100 Mike Looijmans wrote:
quoted
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?
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-01-28 00:02:06
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:
quoted
On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
quoted
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
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2021-01-28 00:27:51
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:
quoted
On Tue, Jan 26, 2021 at 02:14:40PM +0100, Andrew Lunn wrote:
quoted
On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
quoted
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!
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-01-28 01:16:37
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:
quoted
On Tue, Jan 26, 2021 at 01:49:38PM +0000, Russell King - ARM Linux admin wrote:
quoted
On Tue, Jan 26, 2021 at 02:14:40PM +0100, Andrew Lunn wrote:
quoted
On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
quoted
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
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-01-28 01:57:58
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
From: Mike Looijmans <hidden> Date: 2021-01-28 08:50:10
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:
quoted
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...
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-01-29 20:27:01
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.
quoted
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
From: Mike Looijmans <hidden> Date: 2021-02-02 11:41:36
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:
quoted
On Thu, Jan 28, 2021 at 01:00:57AM +0100, Andrew Lunn wrote:
quoted
On Tue, Jan 26, 2021 at 01:49:38PM +0000, Russell King - ARM Linux admin wrote:
quoted
On Tue, Jan 26, 2021 at 02:14:40PM +0100, Andrew Lunn wrote:
quoted
On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
quoted
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
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-02-02 13:59:28
quoted
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