* PHY reset handling during DT parsing @ 2020-07-06 18:13 Maxime Ripard 2020-07-07 14:19 ` Andrew Lunn 0 siblings, 1 reply; 6+ messages in thread From: Maxime Ripard @ 2020-07-06 18:13 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Russell King Cc: Rob Herring, Frank Rowand, David S. Miller, Jakub Kicinski, netdev, devicetree, linux-kernel, Antoine Ténart [-- Attachment #1: Type: text/plain, Size: 1901 bytes --] Hi, I came across an issue today on an Allwinner board, but I believe it's a core issue. That board is using the stmac driver together with a phy that happens to have a reset GPIO, except that that GPIO will never be claimed, and the PHY will thus never work. You can find an example of such a board here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun6i-a31-hummingbird.dts#n195 It looks like when of_mdiobus_register() will parse the DT, it will then call of_mdiobus_register_phy() for each PHY it encounters [1]. of_mdiobus_register_phy() will then if the phy doesn't have an ethernet-phy-id* compatible call get_phy_device() [2], and will later on call phy_register_device [3]. get_phy_device() will then call get_phy_id() [4], that will try to access the PHY through the MDIO bus [5]. The code that deals with the PHY reset line / GPIO is however only done in mdiobus_device_register, called through phy_device_register. Since this is happening way after the call to get_phy_device, our PHY might still very well be in reset if the bootloader hasn't put it out of reset and left it there. I'm not entirely sure how to fix that though. I tried to fix it by splitting away the gpio / reset code away from mdiobus_device_register into a new function, and calling it before the first call to get_phy_id so that we can put our phy out of reset, but it looks like the device registration makes it more complicated than that. Any ideas? Thanks! Maxime 1: https://elixir.bootlin.com/linux/latest/source/drivers/of/of_mdio.c#L274 2: https://elixir.bootlin.com/linux/latest/source/drivers/of/of_mdio.c#L82 3: https://elixir.bootlin.com/linux/latest/source/drivers/of/of_mdio.c#L119 4: https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L830 5: https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L791 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PHY reset handling during DT parsing 2020-07-06 18:13 PHY reset handling during DT parsing Maxime Ripard @ 2020-07-07 14:19 ` Andrew Lunn 2020-07-07 14:54 ` Maxime Ripard 0 siblings, 1 reply; 6+ messages in thread From: Andrew Lunn @ 2020-07-07 14:19 UTC (permalink / raw) To: Maxime Ripard Cc: Florian Fainelli, Heiner Kallweit, Russell King, Rob Herring, Frank Rowand, David S. Miller, Jakub Kicinski, netdev, devicetree, linux-kernel, Antoine Ténart On Mon, Jul 06, 2020 at 08:13:31PM +0200, Maxime Ripard wrote: > Hi, > > I came across an issue today on an Allwinner board, but I believe it's a > core issue. > > That board is using the stmac driver together with a phy that happens to > have a reset GPIO, except that that GPIO will never be claimed, and the > PHY will thus never work. > > You can find an example of such a board here: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun6i-a31-hummingbird.dts#n195 > > It looks like when of_mdiobus_register() will parse the DT, it will then > call of_mdiobus_register_phy() for each PHY it encounters [1]. > of_mdiobus_register_phy() will then if the phy doesn't have an > ethernet-phy-id* compatible call get_phy_device() [2], and will later on > call phy_register_device [3]. > > get_phy_device() will then call get_phy_id() [4], that will try to > access the PHY through the MDIO bus [5]. > > The code that deals with the PHY reset line / GPIO is however only done > in mdiobus_device_register, called through phy_device_register. Since > this is happening way after the call to get_phy_device, our PHY might > still very well be in reset if the bootloader hasn't put it out of reset > and left it there. Hi Maxime If you look at the history of this code, commit bafbdd527d569c8200521f2f7579f65a044271be Author: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Date: Mon Dec 4 13:35:05 2017 +0100 phylib: Add device reset GPIO support you will see there is an assumption the PHY can be detected while in reset. The reset was originally handled inside the at803x PHY driver probe function, before it got moved into the core. What you are asking for it reasonable, but you have some history to deal with, changing some assumptions as to what the reset is all about. Andrew ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PHY reset handling during DT parsing 2020-07-07 14:19 ` Andrew Lunn @ 2020-07-07 14:54 ` Maxime Ripard 2020-07-07 16:39 ` Florian Fainelli 0 siblings, 1 reply; 6+ messages in thread From: Maxime Ripard @ 2020-07-07 14:54 UTC (permalink / raw) To: Andrew Lunn Cc: Florian Fainelli, Heiner Kallweit, Russell King, Rob Herring, Frank Rowand, David S. Miller, Jakub Kicinski, netdev, devicetree, linux-kernel, Antoine Ténart Hi Andrew, On Tue, Jul 07, 2020 at 04:19:18PM +0200, Andrew Lunn wrote: > On Mon, Jul 06, 2020 at 08:13:31PM +0200, Maxime Ripard wrote: > > I came across an issue today on an Allwinner board, but I believe it's a > > core issue. > > > > That board is using the stmac driver together with a phy that happens to > > have a reset GPIO, except that that GPIO will never be claimed, and the > > PHY will thus never work. > > > > You can find an example of such a board here: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun6i-a31-hummingbird.dts#n195 > > > > It looks like when of_mdiobus_register() will parse the DT, it will then > > call of_mdiobus_register_phy() for each PHY it encounters [1]. > > of_mdiobus_register_phy() will then if the phy doesn't have an > > ethernet-phy-id* compatible call get_phy_device() [2], and will later on > > call phy_register_device [3]. > > > > get_phy_device() will then call get_phy_id() [4], that will try to > > access the PHY through the MDIO bus [5]. > > > > The code that deals with the PHY reset line / GPIO is however only done > > in mdiobus_device_register, called through phy_device_register. Since > > this is happening way after the call to get_phy_device, our PHY might > > still very well be in reset if the bootloader hasn't put it out of reset > > and left it there. > > Hi Maxime > > If you look at the history of this code, > > commit bafbdd527d569c8200521f2f7579f65a044271be > Author: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > Date: Mon Dec 4 13:35:05 2017 +0100 > > phylib: Add device reset GPIO support > > you will see there is an assumption the PHY can be detected while in > reset. The reset was originally handled inside the at803x PHY driver > probe function, before it got moved into the core. > > What you are asking for it reasonable, but you have some history to > deal with, changing some assumptions as to what the reset is all > about. Thanks for the pointer. It looks to me from the commit log that the assumption was that a bootloader could leave the PHY into reset though? It starts with: > The PHY devices sometimes do have their reset signal (maybe even power > supply?) tied to some GPIO and sometimes it also does happen that a > boot loader does not leave it deasserted. This is exactly the case I was discussing. The bootloader hasn't used the PHY, and thus the PHY reset signal is still asserted? Maxime ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PHY reset handling during DT parsing 2020-07-07 14:54 ` Maxime Ripard @ 2020-07-07 16:39 ` Florian Fainelli 2020-07-07 17:18 ` Rob Herring 0 siblings, 1 reply; 6+ messages in thread From: Florian Fainelli @ 2020-07-07 16:39 UTC (permalink / raw) To: Maxime Ripard, Andrew Lunn Cc: Heiner Kallweit, Russell King, Rob Herring, Frank Rowand, David S. Miller, Jakub Kicinski, netdev, devicetree, linux-kernel, Antoine Ténart On 7/7/2020 7:54 AM, Maxime Ripard wrote: > Hi Andrew, > > On Tue, Jul 07, 2020 at 04:19:18PM +0200, Andrew Lunn wrote: >> On Mon, Jul 06, 2020 at 08:13:31PM +0200, Maxime Ripard wrote: >>> I came across an issue today on an Allwinner board, but I believe it's a >>> core issue. >>> >>> That board is using the stmac driver together with a phy that happens to >>> have a reset GPIO, except that that GPIO will never be claimed, and the >>> PHY will thus never work. >>> >>> You can find an example of such a board here: >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun6i-a31-hummingbird.dts#n195 >>> >>> It looks like when of_mdiobus_register() will parse the DT, it will then >>> call of_mdiobus_register_phy() for each PHY it encounters [1]. >>> of_mdiobus_register_phy() will then if the phy doesn't have an >>> ethernet-phy-id* compatible call get_phy_device() [2], and will later on >>> call phy_register_device [3]. >>> >>> get_phy_device() will then call get_phy_id() [4], that will try to >>> access the PHY through the MDIO bus [5]. >>> >>> The code that deals with the PHY reset line / GPIO is however only done >>> in mdiobus_device_register, called through phy_device_register. Since >>> this is happening way after the call to get_phy_device, our PHY might >>> still very well be in reset if the bootloader hasn't put it out of reset >>> and left it there. >> >> Hi Maxime >> >> If you look at the history of this code, >> >> commit bafbdd527d569c8200521f2f7579f65a044271be >> Author: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> Date: Mon Dec 4 13:35:05 2017 +0100 >> >> phylib: Add device reset GPIO support >> >> you will see there is an assumption the PHY can be detected while in >> reset. The reset was originally handled inside the at803x PHY driver >> probe function, before it got moved into the core. >> >> What you are asking for it reasonable, but you have some history to >> deal with, changing some assumptions as to what the reset is all >> about. > > Thanks for the pointer. > > It looks to me from the commit log that the assumption was that a > bootloader could leave the PHY into reset though? > > It starts with: > >> The PHY devices sometimes do have their reset signal (maybe even power >> supply?) tied to some GPIO and sometimes it also does happen that a >> boot loader does not leave it deasserted. > > This is exactly the case I was discussing. The bootloader hasn't used > the PHY, and thus the PHY reset signal is still asserted? The current solution to this problem would be to have a reset property specified for the MDIO bus controller node such that the reset would be de-asserted during mdiobus_register() and prior to scanning the MDIO bus. This has the nice property that for a 1:1 mapping between MDIO bus controller and device, it works (albeit maybe not being accurately describing hardware), but if you have multiple MDIO/PHY devices sitting on the bus, they might each have their own reset control and while we would attempt to manage that reset line from that call path: mdiobus_scan() -> phy_device_register() -> mdiobus_register_device() it would be too late because there is a preceding get_phy_device() which attempts to read the PHY device's OUI and it would fail if the PHY is still held in reset. We have had a similar discussion before with regulators: http://archive.lwn.net:8080/devicetree/20200622093744.13685-1-brgl@bgdev.pl/ and so far we do not really have a good solution to this problem either. For your specific use case with resets you could do a couple of things: - if there is only one PHY on the MDIO bus you can describe the reset line to be within the MDIO bus controller node (explained above), this is not great but it is not necessarily too far from reality either - if you know the PHY OUI, you can put it as a compatible string in the form: ethernet-phy-id%4x.%4x and that will avoid the MDIO bus layer to try to read from the PHY but instead match it with a driver and create a phy_device instance right away I think we are open to a "pre probe" model where it is possible to have a phy_device instance created that would allow reset controller or regulator (or clocks, too) to be usable with a struct device reference, yet make no HW access until all of these resources have been enabled. -- Florian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PHY reset handling during DT parsing 2020-07-07 16:39 ` Florian Fainelli @ 2020-07-07 17:18 ` Rob Herring 2020-07-07 21:15 ` Florian Fainelli 0 siblings, 1 reply; 6+ messages in thread From: Rob Herring @ 2020-07-07 17:18 UTC (permalink / raw) To: Florian Fainelli Cc: Maxime Ripard, Andrew Lunn, Heiner Kallweit, Russell King, Frank Rowand, David S. Miller, Jakub Kicinski, netdev, devicetree, linux-kernel, Antoine Ténart On Tue, Jul 7, 2020 at 10:39 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 7/7/2020 7:54 AM, Maxime Ripard wrote: > > Hi Andrew, > > > > On Tue, Jul 07, 2020 at 04:19:18PM +0200, Andrew Lunn wrote: > >> On Mon, Jul 06, 2020 at 08:13:31PM +0200, Maxime Ripard wrote: > >>> I came across an issue today on an Allwinner board, but I believe it's a > >>> core issue. > >>> > >>> That board is using the stmac driver together with a phy that happens to > >>> have a reset GPIO, except that that GPIO will never be claimed, and the > >>> PHY will thus never work. > >>> > >>> You can find an example of such a board here: > >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun6i-a31-hummingbird.dts#n195 > >>> > >>> It looks like when of_mdiobus_register() will parse the DT, it will then > >>> call of_mdiobus_register_phy() for each PHY it encounters [1]. > >>> of_mdiobus_register_phy() will then if the phy doesn't have an > >>> ethernet-phy-id* compatible call get_phy_device() [2], and will later on > >>> call phy_register_device [3]. > >>> > >>> get_phy_device() will then call get_phy_id() [4], that will try to > >>> access the PHY through the MDIO bus [5]. > >>> > >>> The code that deals with the PHY reset line / GPIO is however only done > >>> in mdiobus_device_register, called through phy_device_register. Since > >>> this is happening way after the call to get_phy_device, our PHY might > >>> still very well be in reset if the bootloader hasn't put it out of reset > >>> and left it there. > >> > >> Hi Maxime > >> > >> If you look at the history of this code, > >> > >> commit bafbdd527d569c8200521f2f7579f65a044271be > >> Author: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > >> Date: Mon Dec 4 13:35:05 2017 +0100 > >> > >> phylib: Add device reset GPIO support > >> > >> you will see there is an assumption the PHY can be detected while in > >> reset. The reset was originally handled inside the at803x PHY driver > >> probe function, before it got moved into the core. > >> > >> What you are asking for it reasonable, but you have some history to > >> deal with, changing some assumptions as to what the reset is all > >> about. > > > > Thanks for the pointer. > > > > It looks to me from the commit log that the assumption was that a > > bootloader could leave the PHY into reset though? > > > > It starts with: > > > >> The PHY devices sometimes do have their reset signal (maybe even power > >> supply?) tied to some GPIO and sometimes it also does happen that a > >> boot loader does not leave it deasserted. > > > > This is exactly the case I was discussing. The bootloader hasn't used > > the PHY, and thus the PHY reset signal is still asserted? > > The current solution to this problem would be to have a reset property > specified for the MDIO bus controller node such that the reset would be > de-asserted during mdiobus_register() and prior to scanning the MDIO bus. Unless the reset controls all the devices on the mdio bus, the controller node is not the right place. The core could look into the child nodes, but this is just one possible property. > This has the nice property that for a 1:1 mapping between MDIO bus > controller and device, it works (albeit maybe not being accurately > describing hardware), but if you have multiple MDIO/PHY devices sitting > on the bus, they might each have their own reset control and while we > would attempt to manage that reset line from that call path: > > mdiobus_scan() > -> phy_device_register() > -> mdiobus_register_device() > > it would be too late because there is a preceding get_phy_device() which > attempts to read the PHY device's OUI and it would fail if the PHY is > still held in reset. > > We have had a similar discussion before with regulators: > > http://archive.lwn.net:8080/devicetree/20200622093744.13685-1-brgl@bgdev.pl/ > > and so far we do not really have a good solution to this problem either. > > For your specific use case with resets you could do a couple of things: > > - if there is only one PHY on the MDIO bus you can describe the reset > line to be within the MDIO bus controller node (explained above), this > is not great but it is not necessarily too far from reality either > > - if you know the PHY OUI, you can put it as a compatible string in the > form: ethernet-phy-id%4x.%4x and that will avoid the MDIO bus layer to > try to read from the PHY but instead match it with a driver and create a > phy_device instance right away The h/w is simply not discoverable, so it needs a compatible string. > I think we are open to a "pre probe" model where it is possible to have > a phy_device instance created that would allow reset controller or > regulator (or clocks, too) to be usable with a struct device reference, > yet make no HW access until all of these resources have been enabled. I think this is needed for the kernel's driver model in general. It's not an MDIO specific problem. Rob ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PHY reset handling during DT parsing 2020-07-07 17:18 ` Rob Herring @ 2020-07-07 21:15 ` Florian Fainelli 0 siblings, 0 replies; 6+ messages in thread From: Florian Fainelli @ 2020-07-07 21:15 UTC (permalink / raw) To: Rob Herring Cc: Maxime Ripard, Andrew Lunn, Heiner Kallweit, Russell King, Frank Rowand, David S. Miller, Jakub Kicinski, netdev, devicetree, linux-kernel, Antoine Ténart On 7/7/2020 10:18 AM, Rob Herring wrote: > On Tue, Jul 7, 2020 at 10:39 AM Florian Fainelli <f.fainelli@gmail.com> wrote: >> >> >> >> On 7/7/2020 7:54 AM, Maxime Ripard wrote: >>> Hi Andrew, >>> >>> On Tue, Jul 07, 2020 at 04:19:18PM +0200, Andrew Lunn wrote: >>>> On Mon, Jul 06, 2020 at 08:13:31PM +0200, Maxime Ripard wrote: >>>>> I came across an issue today on an Allwinner board, but I believe it's a >>>>> core issue. >>>>> >>>>> That board is using the stmac driver together with a phy that happens to >>>>> have a reset GPIO, except that that GPIO will never be claimed, and the >>>>> PHY will thus never work. >>>>> >>>>> You can find an example of such a board here: >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun6i-a31-hummingbird.dts#n195 >>>>> >>>>> It looks like when of_mdiobus_register() will parse the DT, it will then >>>>> call of_mdiobus_register_phy() for each PHY it encounters [1]. >>>>> of_mdiobus_register_phy() will then if the phy doesn't have an >>>>> ethernet-phy-id* compatible call get_phy_device() [2], and will later on >>>>> call phy_register_device [3]. >>>>> >>>>> get_phy_device() will then call get_phy_id() [4], that will try to >>>>> access the PHY through the MDIO bus [5]. >>>>> >>>>> The code that deals with the PHY reset line / GPIO is however only done >>>>> in mdiobus_device_register, called through phy_device_register. Since >>>>> this is happening way after the call to get_phy_device, our PHY might >>>>> still very well be in reset if the bootloader hasn't put it out of reset >>>>> and left it there. >>>> >>>> Hi Maxime >>>> >>>> If you look at the history of this code, >>>> >>>> commit bafbdd527d569c8200521f2f7579f65a044271be >>>> Author: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>>> Date: Mon Dec 4 13:35:05 2017 +0100 >>>> >>>> phylib: Add device reset GPIO support >>>> >>>> you will see there is an assumption the PHY can be detected while in >>>> reset. The reset was originally handled inside the at803x PHY driver >>>> probe function, before it got moved into the core. >>>> >>>> What you are asking for it reasonable, but you have some history to >>>> deal with, changing some assumptions as to what the reset is all >>>> about. >>> >>> Thanks for the pointer. >>> >>> It looks to me from the commit log that the assumption was that a >>> bootloader could leave the PHY into reset though? >>> >>> It starts with: >>> >>>> The PHY devices sometimes do have their reset signal (maybe even power >>>> supply?) tied to some GPIO and sometimes it also does happen that a >>>> boot loader does not leave it deasserted. >>> >>> This is exactly the case I was discussing. The bootloader hasn't used >>> the PHY, and thus the PHY reset signal is still asserted? >> >> The current solution to this problem would be to have a reset property >> specified for the MDIO bus controller node such that the reset would be >> de-asserted during mdiobus_register() and prior to scanning the MDIO bus. > > Unless the reset controls all the devices on the mdio bus, the > controller node is not the right place. The core could look into the > child nodes, but this is just one possible property. Both are defined and supported (with the caveat mentioned below). > >> This has the nice property that for a 1:1 mapping between MDIO bus >> controller and device, it works (albeit maybe not being accurately >> describing hardware), but if you have multiple MDIO/PHY devices sitting >> on the bus, they might each have their own reset control and while we >> would attempt to manage that reset line from that call path: >> >> mdiobus_scan() >> -> phy_device_register() >> -> mdiobus_register_device() >> >> it would be too late because there is a preceding get_phy_device() which >> attempts to read the PHY device's OUI and it would fail if the PHY is >> still held in reset. >> >> We have had a similar discussion before with regulators: >> >> http://archive.lwn.net:8080/devicetree/20200622093744.13685-1-brgl@bgdev.pl/ >> >> and so far we do not really have a good solution to this problem either. >> >> For your specific use case with resets you could do a couple of things: >> >> - if there is only one PHY on the MDIO bus you can describe the reset >> line to be within the MDIO bus controller node (explained above), this >> is not great but it is not necessarily too far from reality either >> >> - if you know the PHY OUI, you can put it as a compatible string in the >> form: ethernet-phy-id%4x.%4x and that will avoid the MDIO bus layer to >> try to read from the PHY but instead match it with a driver and create a >> phy_device instance right away > > The h/w is simply not discoverable, so it needs a compatible string. It is discoverable if you have the various resources (regulator(s), clock(s) and reset(s) managed ahead of issuing the discovery which is really the crux of the problem here. > >> I think we are open to a "pre probe" model where it is possible to have >> a phy_device instance created that would allow reset controller or >> regulator (or clocks, too) to be usable with a struct device reference, >> yet make no HW access until all of these resources have been enabled. > > I think this is needed for the kernel's driver model in general. It's > not an MDIO specific problem. Oh absolutely not, PCI and USB have the same issue, it is not clear to me how to tackle this yet though. -- Florian ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-07 21:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-06 18:13 PHY reset handling during DT parsing Maxime Ripard 2020-07-07 14:19 ` Andrew Lunn 2020-07-07 14:54 ` Maxime Ripard 2020-07-07 16:39 ` Florian Fainelli 2020-07-07 17:18 ` Rob Herring 2020-07-07 21:15 ` Florian Fainelli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).