| Message ID | 20240704152610.1345709-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | arm64: dts: renesas: white-hawk-cpu: Move avb0 reset gpio to mdio node | expand |
Hi Marek, What is your stance on this? Thanks! On Thu, Jul 4, 2024 at 5:26 PM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote: > When creating a dedicated mdio node to describe the bus the gpio reset > property was erroneously left in the phy node. The reason for adding > mdio nodes on WhiteHawk was to ensure the PHYs where reset before they > were probed, keeping the property in the phy node prevented this. > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Fixes: 54bf0c27380b ("arm64: dts: renesas: r8a779g0: Use MDIO node for all AVB devices") > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > arch/arm64/boot/dts/renesas/white-hawk-cpu-common.dtsi | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/renesas/white-hawk-cpu-common.dtsi b/arch/arm64/boot/dts/renesas/white-hawk-cpu-common.dtsi > index 80496fb3d476..4f0230327868 100644 > --- a/arch/arm64/boot/dts/renesas/white-hawk-cpu-common.dtsi > +++ b/arch/arm64/boot/dts/renesas/white-hawk-cpu-common.dtsi > @@ -156,6 +156,8 @@ mdio { > #address-cells = <1>; > #size-cells = <0>; > > + reset-gpios = <&gpio7 10 GPIO_ACTIVE_LOW>; > + > avb0_phy: ethernet-phy@0 { > compatible = "ethernet-phy-id0022.1622", > "ethernet-phy-ieee802.3-c22"; > @@ -163,7 +165,6 @@ avb0_phy: ethernet-phy@0 { > reg = <0>; > interrupt-parent = <&gpio7>; > interrupts = <5 IRQ_TYPE_LEVEL_LOW>; > - reset-gpios = <&gpio7 10 GPIO_ACTIVE_LOW>; > }; > }; > }; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 8/2/24 10:33 AM, Geert Uytterhoeven wrote: > Hi Marek, Hi, > What is your stance on this? > Thanks! > > On Thu, Jul 4, 2024 at 5:26 PM Niklas Söderlund > <niklas.soderlund+renesas@ragnatech.se> wrote: >> When creating a dedicated mdio node to describe the bus the gpio reset >> property was erroneously left in the phy node. The reason for adding >> mdio nodes on WhiteHawk was to ensure the PHYs where reset before they >> were probed, keeping the property in the phy node prevented this. If the PHYs should be reset before they are probed, that is something the PHY driver should take care of, right ? The PHY driver can bind to the PHY via compatible string. Does the PHY driver not reset the PHYs ?
Hi Marek, On Fri, Aug 2, 2024 at 7:16 PM Marek Vasut <marex@denx.de> wrote: > On 8/2/24 10:33 AM, Geert Uytterhoeven wrote: > > What is your stance on this? > > On Thu, Jul 4, 2024 at 5:26 PM Niklas Söderlund > > <niklas.soderlund+renesas@ragnatech.se> wrote: > >> When creating a dedicated mdio node to describe the bus the gpio reset > >> property was erroneously left in the phy node. The reason for adding > >> mdio nodes on WhiteHawk was to ensure the PHYs where reset before they > >> were probed, keeping the property in the phy node prevented this. > > If the PHYs should be reset before they are probed, that is something > the PHY driver should take care of, right ? The PHY driver can bind to > the PHY via compatible string. Does the PHY driver not reset the PHYs ? AFAIK, there is no requirement to reset the PHY before it is probed. However, the reset signal may be in asserted state when the PHY is probed (e.g. after unbind from the Ethernet driver, or during kexec). Identifying the PHY by reading the ID register requires deasserting the reset first. Gr{oetje,eeting}s, Geert
Hello Geert and Marek, On 2024-08-22 15:56:44 +0200, Geert Uytterhoeven wrote: > Hi Marek, > > On Fri, Aug 2, 2024 at 7:16 PM Marek Vasut <marex@denx.de> wrote: > > On 8/2/24 10:33 AM, Geert Uytterhoeven wrote: > > > What is your stance on this? > > > > On Thu, Jul 4, 2024 at 5:26 PM Niklas Söderlund > > > <niklas.soderlund+renesas@ragnatech.se> wrote: > > >> When creating a dedicated mdio node to describe the bus the gpio reset > > >> property was erroneously left in the phy node. The reason for adding > > >> mdio nodes on WhiteHawk was to ensure the PHYs where reset before they > > >> were probed, keeping the property in the phy node prevented this. > > > > If the PHYs should be reset before they are probed, that is something > > the PHY driver should take care of, right ? The PHY driver can bind to > > the PHY via compatible string. Does the PHY driver not reset the PHYs ? > > AFAIK, there is no requirement to reset the PHY before it is probed. > However, the reset signal may be in asserted state when the PHY is > probed (e.g. after unbind from the Ethernet driver, or during kexec). > Identifying the PHY by reading the ID register requires deasserting > the reset first. Did we reach consensus on this? My primary motivation for this was to align it what is done for the other AVB instances on WhiteHawk, which do have the reset-gpios property in the mdio node and not the phy node. As having a mdio node at all is a new-ish thing for AVB as this was needed or the mv88q2110 PHYs connected to the other AVBs to function. If we are happy to have this here for AVB0 I will drop this patch.
On 8/22/24 3:56 PM, Geert Uytterhoeven wrote: > Hi Marek, Hi, sorry for the delay. > On Fri, Aug 2, 2024 at 7:16 PM Marek Vasut <marex@denx.de> wrote: >> On 8/2/24 10:33 AM, Geert Uytterhoeven wrote: >>> What is your stance on this? > >>> On Thu, Jul 4, 2024 at 5:26 PM Niklas Söderlund >>> <niklas.soderlund+renesas@ragnatech.se> wrote: >>>> When creating a dedicated mdio node to describe the bus the gpio reset >>>> property was erroneously left in the phy node. The reason for adding >>>> mdio nodes on WhiteHawk was to ensure the PHYs where reset before they >>>> were probed, keeping the property in the phy node prevented this. >> >> If the PHYs should be reset before they are probed, that is something >> the PHY driver should take care of, right ? The PHY driver can bind to >> the PHY via compatible string. Does the PHY driver not reset the PHYs ? > > AFAIK, there is no requirement to reset the PHY before it is probed. That would mean the PHY is in undefined state before it is probed, which is not good. > However, the reset signal may be in asserted state when the PHY is > probed (e.g. after unbind from the Ethernet driver, or during kexec). > Identifying the PHY by reading the ID register requires deasserting > the reset first. That may not be the entire precondition. For example the SMSC LAN87xx PHYs also require PHY clock to be enabled before the reset is toggled, but such information is available only to the specific PHY driver. The MDIO-level reset GPIO handling, as far as I understand it, applies in case there are more PHYs on the MDIO bus which share the same reset GPIO line. In this case there is only one PHY on the MDIO bus, so the only bit which applies is the potential PHY-specific reset requirement handling. If the PHY driver ever gets extended with such a thing in the future, then having the reset-gpios in the PHY node is beneficial over having it in MDIO node. It will always be a compromise between the above and best-effort PHY auto-detection though.
Hello, On 2024-09-06 20:09:19 +0200, Marek Vasut wrote: > On 8/22/24 3:56 PM, Geert Uytterhoeven wrote: > > Hi Marek, > > Hi, > > sorry for the delay. > > > On Fri, Aug 2, 2024 at 7:16 PM Marek Vasut <marex@denx.de> wrote: > > > On 8/2/24 10:33 AM, Geert Uytterhoeven wrote: > > > > What is your stance on this? > > > > > > On Thu, Jul 4, 2024 at 5:26 PM Niklas Söderlund > > > > <niklas.soderlund+renesas@ragnatech.se> wrote: > > > > > When creating a dedicated mdio node to describe the bus the gpio reset > > > > > property was erroneously left in the phy node. The reason for adding > > > > > mdio nodes on WhiteHawk was to ensure the PHYs where reset before they > > > > > were probed, keeping the property in the phy node prevented this. > > > > > > If the PHYs should be reset before they are probed, that is something > > > the PHY driver should take care of, right ? The PHY driver can bind to > > > the PHY via compatible string. Does the PHY driver not reset the PHYs ? > > > > AFAIK, there is no requirement to reset the PHY before it is probed. > > That would mean the PHY is in undefined state before it is probed, which is > not good. > > > However, the reset signal may be in asserted state when the PHY is > > probed (e.g. after unbind from the Ethernet driver, or during kexec). > > Identifying the PHY by reading the ID register requires deasserting > > the reset first. > That may not be the entire precondition. For example the SMSC LAN87xx PHYs > also require PHY clock to be enabled before the reset is toggled, but such > information is available only to the specific PHY driver. > > The MDIO-level reset GPIO handling, as far as I understand it, applies in > case there are more PHYs on the MDIO bus which share the same reset GPIO > line. > > In this case there is only one PHY on the MDIO bus, so the only bit which > applies is the potential PHY-specific reset requirement handling. If the PHY > driver ever gets extended with such a thing in the future, then having the > reset-gpios in the PHY node is beneficial over having it in MDIO node. > > It will always be a compromise between the above and best-effort PHY > auto-detection though. I agree this is not needed if the PHY is identified by the compatible string, but might be if it is not. In this case it works and the reason for this patch was just to align the style used here. I'm happy to drop this patch, or send a rebased version that applies since the context changed ;-) Marek, Geert what is your view? I'm happy with either option.
On 10/15/24 4:48 PM, Niklas Söderlund wrote: Hi, >>> However, the reset signal may be in asserted state when the PHY is >>> probed (e.g. after unbind from the Ethernet driver, or during kexec). >>> Identifying the PHY by reading the ID register requires deasserting >>> the reset first. >> That may not be the entire precondition. For example the SMSC LAN87xx PHYs >> also require PHY clock to be enabled before the reset is toggled, but such >> information is available only to the specific PHY driver. >> >> The MDIO-level reset GPIO handling, as far as I understand it, applies in >> case there are more PHYs on the MDIO bus which share the same reset GPIO >> line. >> >> In this case there is only one PHY on the MDIO bus, so the only bit which >> applies is the potential PHY-specific reset requirement handling. If the PHY >> driver ever gets extended with such a thing in the future, then having the >> reset-gpios in the PHY node is beneficial over having it in MDIO node. >> >> It will always be a compromise between the above and best-effort PHY >> auto-detection though. > > I agree this is not needed if the PHY is identified by the compatible > string, but might be if it is not. In this case it works and the reason > for this patch was just to align the style used here. > > I'm happy to drop this patch, or send a rebased version that applies > since the context changed ;-) Marek, Geert what is your view? I'm happy > with either option. I was hoping Geert would comment on this first, but seems like maybe no. I think, since the PHY node does have a compatible string AND the reset is connected to the PHY, I would keep the reset property in the PHY node. Sorry.
Hi Marek, On Mon, Oct 21, 2024 at 2:13 AM Marek Vasut <marex@denx.de> wrote: > On 10/15/24 4:48 PM, Niklas Söderlund wrote: > >>> However, the reset signal may be in asserted state when the PHY is > >>> probed (e.g. after unbind from the Ethernet driver, or during kexec). > >>> Identifying the PHY by reading the ID register requires deasserting > >>> the reset first. > >> That may not be the entire precondition. For example the SMSC LAN87xx PHYs > >> also require PHY clock to be enabled before the reset is toggled, but such > >> information is available only to the specific PHY driver. > >> > >> The MDIO-level reset GPIO handling, as far as I understand it, applies in > >> case there are more PHYs on the MDIO bus which share the same reset GPIO > >> line. > >> > >> In this case there is only one PHY on the MDIO bus, so the only bit which > >> applies is the potential PHY-specific reset requirement handling. If the PHY > >> driver ever gets extended with such a thing in the future, then having the > >> reset-gpios in the PHY node is beneficial over having it in MDIO node. > >> > >> It will always be a compromise between the above and best-effort PHY > >> auto-detection though. > > > > I agree this is not needed if the PHY is identified by the compatible > > string, but might be if it is not. In this case it works and the reason > > for this patch was just to align the style used here. > > > > I'm happy to drop this patch, or send a rebased version that applies > > since the context changed ;-) Marek, Geert what is your view? I'm happy > > with either option. > > I was hoping Geert would comment on this first, but seems like maybe no. > I think, since the PHY node does have a compatible string AND the reset > is connected to the PHY, I would keep the reset property in the PHY > node. Sorry. You are inverting the reasoning ;-) The compatible strings were added because otherwise the PHY core can not identify the PHY when the reset is asserted (e.g. after kexec). If possible, I'd rather remove the compatible strings again, as different PHYs may be mounted on different PHY revisions, causing a headache for DTB management. So, what would you suggest when the PHY nodes would not have compatible strings? Thanks! Gr{oetje,eeting}s, Geert
On 10/21/24 9:13 AM, Geert Uytterhoeven wrote: > Hi Marek, Hi, > On Mon, Oct 21, 2024 at 2:13 AM Marek Vasut <marex@denx.de> wrote: >> On 10/15/24 4:48 PM, Niklas Söderlund wrote: >>>>> However, the reset signal may be in asserted state when the PHY is >>>>> probed (e.g. after unbind from the Ethernet driver, or during kexec). >>>>> Identifying the PHY by reading the ID register requires deasserting >>>>> the reset first. >>>> That may not be the entire precondition. For example the SMSC LAN87xx PHYs >>>> also require PHY clock to be enabled before the reset is toggled, but such >>>> information is available only to the specific PHY driver. >>>> >>>> The MDIO-level reset GPIO handling, as far as I understand it, applies in >>>> case there are more PHYs on the MDIO bus which share the same reset GPIO >>>> line. >>>> >>>> In this case there is only one PHY on the MDIO bus, so the only bit which >>>> applies is the potential PHY-specific reset requirement handling. If the PHY >>>> driver ever gets extended with such a thing in the future, then having the >>>> reset-gpios in the PHY node is beneficial over having it in MDIO node. >>>> >>>> It will always be a compromise between the above and best-effort PHY >>>> auto-detection though. >>> >>> I agree this is not needed if the PHY is identified by the compatible >>> string, but might be if it is not. In this case it works and the reason >>> for this patch was just to align the style used here. >>> >>> I'm happy to drop this patch, or send a rebased version that applies >>> since the context changed ;-) Marek, Geert what is your view? I'm happy >>> with either option. >> >> I was hoping Geert would comment on this first, but seems like maybe no. >> I think, since the PHY node does have a compatible string AND the reset >> is connected to the PHY, I would keep the reset property in the PHY >> node. Sorry. > > You are inverting the reasoning ;-) The compatible strings were added > because otherwise the PHY core can not identify the PHY when the > reset is asserted (e.g. after kexec). ... or because the PHY requires some complex sequence to bring it up, it is not just reset. > If possible, I'd rather remove > the compatible strings again, as different PHYs may be mounted on > different PHY revisions, causing a headache for DTB management. Will that ever be the case with this hardware ? > So, what would you suggest when the PHY nodes would not have compatible > strings? I would suggest keep the PHY compatible strings, because that is the most accurate method to describe the hardware and fulfill the PHY bring up requirements. If the PHY changes on this hardware in some future revision, we can revisit this discussion ? Maybe bootloader-applied DTOs could work then ?
Hi Marek, On Tue, Oct 22, 2024 at 4:07 AM Marek Vasut <marex@denx.de> wrote: > On 10/21/24 9:13 AM, Geert Uytterhoeven wrote: > > On Mon, Oct 21, 2024 at 2:13 AM Marek Vasut <marex@denx.de> wrote: > >> On 10/15/24 4:48 PM, Niklas Söderlund wrote: > >>>>> However, the reset signal may be in asserted state when the PHY is > >>>>> probed (e.g. after unbind from the Ethernet driver, or during kexec). > >>>>> Identifying the PHY by reading the ID register requires deasserting > >>>>> the reset first. > >>>> That may not be the entire precondition. For example the SMSC LAN87xx PHYs > >>>> also require PHY clock to be enabled before the reset is toggled, but such > >>>> information is available only to the specific PHY driver. > >>>> > >>>> The MDIO-level reset GPIO handling, as far as I understand it, applies in > >>>> case there are more PHYs on the MDIO bus which share the same reset GPIO > >>>> line. > >>>> > >>>> In this case there is only one PHY on the MDIO bus, so the only bit which > >>>> applies is the potential PHY-specific reset requirement handling. If the PHY > >>>> driver ever gets extended with such a thing in the future, then having the > >>>> reset-gpios in the PHY node is beneficial over having it in MDIO node. > >>>> > >>>> It will always be a compromise between the above and best-effort PHY > >>>> auto-detection though. > >>> > >>> I agree this is not needed if the PHY is identified by the compatible > >>> string, but might be if it is not. In this case it works and the reason > >>> for this patch was just to align the style used here. > >>> > >>> I'm happy to drop this patch, or send a rebased version that applies > >>> since the context changed ;-) Marek, Geert what is your view? I'm happy > >>> with either option. > >> > >> I was hoping Geert would comment on this first, but seems like maybe no. > >> I think, since the PHY node does have a compatible string AND the reset > >> is connected to the PHY, I would keep the reset property in the PHY > >> node. Sorry. > > > > You are inverting the reasoning ;-) The compatible strings were added > > because otherwise the PHY core can not identify the PHY when the > > reset is asserted (e.g. after kexec). > > ... or because the PHY requires some complex sequence to bring it up, it > is not just reset. That is your hypothetical case, but not the reason behind commit 722d55f3a9bd810f ("arm64: dts: renesas: Add compatible properties to KSZ9031 Ethernet PHYs"). > > If possible, I'd rather remove > > the compatible strings again, as different PHYs may be mounted on > > different PHY revisions, causing a headache for DTB management. > > Will that ever be the case with this hardware ? Dunno. It did happen with the Beacon boards. > > So, what would you suggest when the PHY nodes would not have compatible > > strings? > I would suggest keep the PHY compatible strings, because that is the > most accurate method to describe the hardware and fulfill the PHY bring > up requirements. If the PHY changes on this hardware in some future That issue is moot for KSZ9031. > revision, we can revisit this discussion ? Maybe bootloader-applied DTOs > could work then ? So, what would you suggest when the PHY nodes would not have compatible strings? Gr{oetje,eeting}s, Geert
On 10/22/24 9:38 AM, Geert Uytterhoeven wrote: > Hi Marek, Hello Geert, >>>> I was hoping Geert would comment on this first, but seems like maybe no. >>>> I think, since the PHY node does have a compatible string AND the reset >>>> is connected to the PHY, I would keep the reset property in the PHY >>>> node. Sorry. >>> >>> You are inverting the reasoning ;-) The compatible strings were added >>> because otherwise the PHY core can not identify the PHY when the >>> reset is asserted (e.g. after kexec). >> >> ... or because the PHY requires some complex sequence to bring it up, it >> is not just reset. > > That is your hypothetical case, but not the reason behind commit > 722d55f3a9bd810f ("arm64: dts: renesas: Add compatible properties to > KSZ9031 Ethernet PHYs"). We can stick to the "reset line in unknown state" here for the sake of this argument, it makes no difference. >>> If possible, I'd rather remove >>> the compatible strings again, as different PHYs may be mounted on >>> different PHY revisions, causing a headache for DTB management. >> >> Will that ever be the case with this hardware ? > > Dunno. It did happen with the Beacon boards. Let's cross that bridge when we come to it ? >>> So, what would you suggest when the PHY nodes would not have compatible >>> strings? >> I would suggest keep the PHY compatible strings, because that is the >> most accurate method to describe the hardware and fulfill the PHY bring >> up requirements. If the PHY changes on this hardware in some future > > That issue is moot for KSZ9031. If the PHY won't change, then we can keep the compatible strings ? >> revision, we can revisit this discussion ? Maybe bootloader-applied DTOs >> could work then ? > > So, what would you suggest when the PHY nodes would not have compatible > strings? I hope I already answered that question before. [...]
Hi Marek, On Sun, Oct 27, 2024 at 5:09 PM Marek Vasut <marex@denx.de> wrote: > On 10/22/24 9:38 AM, Geert Uytterhoeven wrote: > >>>> I was hoping Geert would comment on this first, but seems like maybe no. > >>>> I think, since the PHY node does have a compatible string AND the reset > >>>> is connected to the PHY, I would keep the reset property in the PHY > >>>> node. Sorry. > >>> > >>> You are inverting the reasoning ;-) The compatible strings were added > >>> because otherwise the PHY core can not identify the PHY when the > >>> reset is asserted (e.g. after kexec). > >> > >> ... or because the PHY requires some complex sequence to bring it up, it > >> is not just reset. > > > > That is your hypothetical case, but not the reason behind commit > > 722d55f3a9bd810f ("arm64: dts: renesas: Add compatible properties to > > KSZ9031 Ethernet PHYs"). > > We can stick to the "reset line in unknown state" here for the sake of > this argument, it makes no difference. > > >>> If possible, I'd rather remove > >>> the compatible strings again, as different PHYs may be mounted on > >>> different PHY revisions, causing a headache for DTB management. > >> > >> Will that ever be the case with this hardware ? > > > > Dunno. It did happen with the Beacon boards. > > Let's cross that bridge when we come to it ? > > >>> So, what would you suggest when the PHY nodes would not have compatible > >>> strings? > >> I would suggest keep the PHY compatible strings, because that is the > >> most accurate method to describe the hardware and fulfill the PHY bring > >> up requirements. If the PHY changes on this hardware in some future > > > > That issue is moot for KSZ9031. > > If the PHY won't change, then we can keep the compatible strings ? Sorry for being unclear. I should have written "the PHY bring-up requirements are moot for KSZ9031". > >> revision, we can revisit this discussion ? Maybe bootloader-applied DTOs > >> could work then ? > > > > So, what would you suggest when the PHY nodes would not have compatible > > strings? > I hope I already answered that question before. Sorry, I may have missed that? I really prefer not having the PHY compatible strings, as DT should describe only what cannot be auto-detected. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 10/28/24 11:13 AM, Geert Uytterhoeven wrote: > Hi Marek, Hello Geert, >>>>> So, what would you suggest when the PHY nodes would not have compatible >>>>> strings? >>>> I would suggest keep the PHY compatible strings, because that is the >>>> most accurate method to describe the hardware and fulfill the PHY bring >>>> up requirements. If the PHY changes on this hardware in some future >>> >>> That issue is moot for KSZ9031. >> >> If the PHY won't change, then we can keep the compatible strings ? > > Sorry for being unclear. I should have written "the PHY bring-up > requirements are moot for KSZ9031". Perhaps, (*) but odd erratas do show up every once in a while, so unless you can surely say no such errata will show up for the KSZ9031, can you really dismiss the bring up requirements ? >>>> revision, we can revisit this discussion ? Maybe bootloader-applied DTOs >>>> could work then ? >>> >>> So, what would you suggest when the PHY nodes would not have compatible >>> strings? >> I hope I already answered that question before. > > Sorry, I may have missed that? > > I really prefer not having the PHY compatible strings, as DT should > describe only what cannot be auto-detected. See paragraph above (*). My take on this is the exact opposite, better describe the PHY in DT fully, including compatible strings, so that if the PHY driver needs to do some sort of bring up tweak/fix/errata workaround/... , it can do so by matching on the compatible string without trying to bring the PHY up in some generic and potentially problematic way. The MDIO bus is not discoverable the same way as PCIe or USB is, so I don't think the "DT should describe only what cannot be detected" is really applicable to MDIO bus the same way it applies to PCIe or USB.
On Mon, Oct 28, 2024 at 7:58 PM Marek Vasut <marex@denx.de> wrote: > On 10/28/24 11:13 AM, Geert Uytterhoeven wrote: > >>>>> So, what would you suggest when the PHY nodes would not have compatible > >>>>> strings? > >>>> I would suggest keep the PHY compatible strings, because that is the > >>>> most accurate method to describe the hardware and fulfill the PHY bring > >>>> up requirements. If the PHY changes on this hardware in some future > >>> > >>> That issue is moot for KSZ9031. > >> > >> If the PHY won't change, then we can keep the compatible strings ? > > > > Sorry for being unclear. I should have written "the PHY bring-up > > requirements are moot for KSZ9031". > > Perhaps, (*) but odd erratas do show up every once in a while, so unless > you can surely say no such errata will show up for the KSZ9031, can you > really dismiss the bring up requirements ? > > >>>> revision, we can revisit this discussion ? Maybe bootloader-applied DTOs > >>>> could work then ? > >>> > >>> So, what would you suggest when the PHY nodes would not have compatible > >>> strings? > >> I hope I already answered that question before. > > > > Sorry, I may have missed that? > > > > I really prefer not having the PHY compatible strings, as DT should > > describe only what cannot be auto-detected. > See paragraph above (*). My take on this is the exact opposite, better > describe the PHY in DT fully, including compatible strings, so that if > the PHY driver needs to do some sort of bring up tweak/fix/errata > workaround/... , it can do so by matching on the compatible string > without trying to bring the PHY up in some generic and potentially > problematic way. > > The MDIO bus is not discoverable the same way as PCIe or USB is, so I > don't think the "DT should describe only what cannot be detected" is > really applicable to MDIO bus the same way it applies to PCIe or USB. So you think this is similar to SPI NOR, where most FLASHes can be discovered with the JEDEC READ ID opcode? See commit 4b0cb4e7ab2f777c ("dt-bindings: mtd: spi-nor: clarify the need for spi-nor compatibles"), which clarified why no new compatible values are accepted. Any guidance/opinion from the DT people? Thanks! Gr{oetje,eeting}s, Geert
On 10/29/24 9:26 AM, Geert Uytterhoeven wrote: [...] >>>>>> revision, we can revisit this discussion ? Maybe bootloader-applied DTOs >>>>>> could work then ? >>>>> >>>>> So, what would you suggest when the PHY nodes would not have compatible >>>>> strings? >>>> I hope I already answered that question before. >>> >>> Sorry, I may have missed that? >>> >>> I really prefer not having the PHY compatible strings, as DT should >>> describe only what cannot be auto-detected. >> See paragraph above (*). My take on this is the exact opposite, better >> describe the PHY in DT fully, including compatible strings, so that if >> the PHY driver needs to do some sort of bring up tweak/fix/errata >> workaround/... , it can do so by matching on the compatible string >> without trying to bring the PHY up in some generic and potentially >> problematic way. >> >> The MDIO bus is not discoverable the same way as PCIe or USB is, so I >> don't think the "DT should describe only what cannot be detected" is >> really applicable to MDIO bus the same way it applies to PCIe or USB. > > So you think this is similar to SPI NOR, where most FLASHes can be > discovered with the JEDEC READ ID opcode? Possibly, if you take broken-flash-reset DT property into account somehow. Even SPI NOR does require a proper reset after all, else the READ ID opcode may not work. > See commit 4b0cb4e7ab2f777c > ("dt-bindings: mtd: spi-nor: clarify the need for spi-nor compatibles"), > which clarified why no new compatible values are accepted. This works as long as your SPI NOR reset works.
diff --git a/arch/arm64/boot/dts/renesas/white-hawk-cpu-common.dtsi b/arch/arm64/boot/dts/renesas/white-hawk-cpu-common.dtsi index 80496fb3d476..4f0230327868 100644 --- a/arch/arm64/boot/dts/renesas/white-hawk-cpu-common.dtsi +++ b/arch/arm64/boot/dts/renesas/white-hawk-cpu-common.dtsi @@ -156,6 +156,8 @@ mdio { #address-cells = <1>; #size-cells = <0>; + reset-gpios = <&gpio7 10 GPIO_ACTIVE_LOW>; + avb0_phy: ethernet-phy@0 { compatible = "ethernet-phy-id0022.1622", "ethernet-phy-ieee802.3-c22"; @@ -163,7 +165,6 @@ avb0_phy: ethernet-phy@0 { reg = <0>; interrupt-parent = <&gpio7>; interrupts = <5 IRQ_TYPE_LEVEL_LOW>; - reset-gpios = <&gpio7 10 GPIO_ACTIVE_LOW>; }; }; };
When creating a dedicated mdio node to describe the bus the gpio reset property was erroneously left in the phy node. The reason for adding mdio nodes on WhiteHawk was to ensure the PHYs where reset before they were probed, keeping the property in the phy node prevented this. Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Fixes: 54bf0c27380b ("arm64: dts: renesas: r8a779g0: Use MDIO node for all AVB devices") Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- arch/arm64/boot/dts/renesas/white-hawk-cpu-common.dtsi | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)