| Message ID | 20210126073337.20393-1-mike.looijmans@topic.nl (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mdiobus: Prevent spike on MDIO bus reset signal | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 18 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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.
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?
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
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.
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
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: 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...
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
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?
> > 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
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)
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(-)