| Message ID | 20240630034649.173229-2-marex@denx.de (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [1/2] ARM: dts: renesas: Drop ethernet-phy-ieee802.3-c22 from PHY compatible string on all RZ boards | expand |
On Sun, Jun 30, 2024 at 05:46:18AM +0200, Marek Vasut wrote: > The rtl82xx DT bindings do not require ethernet-phy-ieee802.3-c22 > as the fallback compatible string. There are fewer users of the > Realtek PHY compatible string with fallback compatible string than > there are users without fallback compatible string, so drop the > fallback compatible string from the few remaining users: > > $ git grep -ho ethernet-phy-id001c....... | sort | uniq -c > 1 ethernet-phy-id001c.c816", > 2 ethernet-phy-id001c.c915", > 2 ethernet-phy-id001c.c915"; > 5 ethernet-phy-id001c.c916", > 13 ethernet-phy-id001c.c916"; > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202406290316.YvZdvLxu-lkp@intel.com/ > Signed-off-by: Marek Vasut <marex@denx.de> "ethernet-phy-ieee802.3-c22" is pretty much pointless. I don't remember seeing a DT description which actually needs it. It is in the binding more for completion, since "ethernet-phy-ieee802.3-c45" is needed sometimes, and -c22 just completes the list. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
Hi Marek, On Sun, Jun 30, 2024 at 5:47 AM Marek Vasut <marex@denx.de> wrote: > The rtl82xx DT bindings do not require ethernet-phy-ieee802.3-c22 > as the fallback compatible string. There are fewer users of the > Realtek PHY compatible string with fallback compatible string than > there are users without fallback compatible string, so drop the > fallback compatible string from the few remaining users: > > $ git grep -ho ethernet-phy-id001c....... | sort | uniq -c > 1 ethernet-phy-id001c.c816", > 2 ethernet-phy-id001c.c915", > 2 ethernet-phy-id001c.c915"; > 5 ethernet-phy-id001c.c916", > 13 ethernet-phy-id001c.c916"; > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202406290316.YvZdvLxu-lkp@intel.com/ > Signed-off-by: Marek Vasut <marex@denx.de> Thanks for your patch! > Note: this closes only part of the report In that case you should use a Link: instead of a Closes: tag? > --- a/arch/arm64/boot/dts/renesas/cat875.dtsi > +++ b/arch/arm64/boot/dts/renesas/cat875.dtsi > @@ -22,8 +22,7 @@ &avb { > status = "okay"; > > phy0: ethernet-phy@0 { > - compatible = "ethernet-phy-id001c.c915", > - "ethernet-phy-ieee802.3-c22"; > + compatible = "ethernet-phy-id001c.c915"; > reg = <0>; > interrupt-parent = <&gpio2>; > interrupts = <21 IRQ_TYPE_LEVEL_LOW>; What about moving the PHYs inside an mdio subnode, and removing the compatible properties instead? That would protect against different board revisions using different PHYs or PHY revisions. According to Niklas[1], using an mdio subnode cancels the original reason (failure to identify the PHY in reset state after unbind/rebind or kexec) for adding the compatible values[2]. [1] https://lore.kernel.org/linux-renesas-soc/20240625171445.GF3655345@ragnatech.se [2] commit d45ba2a5f718346e ("arm64: dts: renesas: Add compatible properties to RTL8211E Ethernet PHYs"). Gr{oetje,eeting}s, Geert
On 7/2/24 10:38 AM, Geert Uytterhoeven wrote: > Hi Marek, Hi, > On Sun, Jun 30, 2024 at 5:47 AM Marek Vasut <marex@denx.de> wrote: >> The rtl82xx DT bindings do not require ethernet-phy-ieee802.3-c22 >> as the fallback compatible string. There are fewer users of the >> Realtek PHY compatible string with fallback compatible string than >> there are users without fallback compatible string, so drop the >> fallback compatible string from the few remaining users: >> >> $ git grep -ho ethernet-phy-id001c....... | sort | uniq -c >> 1 ethernet-phy-id001c.c816", >> 2 ethernet-phy-id001c.c915", >> 2 ethernet-phy-id001c.c915"; >> 5 ethernet-phy-id001c.c916", >> 13 ethernet-phy-id001c.c916"; >> >> Reported-by: kernel test robot <lkp@intel.com> >> Closes: https://lore.kernel.org/oe-kbuild-all/202406290316.YvZdvLxu-lkp@intel.com/ >> Signed-off-by: Marek Vasut <marex@denx.de> > > Thanks for your patch! > >> Note: this closes only part of the report > > In that case you should use a Link: instead of a Closes: tag? But which patch would be the one that Closes that report then ? >> --- a/arch/arm64/boot/dts/renesas/cat875.dtsi >> +++ b/arch/arm64/boot/dts/renesas/cat875.dtsi >> @@ -22,8 +22,7 @@ &avb { >> status = "okay"; >> >> phy0: ethernet-phy@0 { >> - compatible = "ethernet-phy-id001c.c915", >> - "ethernet-phy-ieee802.3-c22"; >> + compatible = "ethernet-phy-id001c.c915"; >> reg = <0>; >> interrupt-parent = <&gpio2>; >> interrupts = <21 IRQ_TYPE_LEVEL_LOW>; > > What about moving the PHYs inside an mdio subnode, and removing the > compatible properties instead? That would protect against different > board revisions using different PHYs or PHY revisions. > > According to Niklas[1], using an mdio subnode cancels the original > reason (failure to identify the PHY in reset state after unbind/rebind > or kexec) for adding the compatible values[2]. My understanding is that the compatible string is necessary if the PHY needs clock/reset sequencing of any kind. Without the compatible string, it is not possible to select the correct PHY driver which would handle that sequencing according to the PHY requirements. This board here does use reset-gpio property in the PHY node (it is not visible in this diff context), so I believe a compatible string should be present here. What would happen if this board got a revision with another PHY with different PHY reset sequencing requirements ? The MDIO node level reset handling might no longer be viable.
Hi Marek, On Tue, Jul 2, 2024 at 10:45 PM Marek Vasut <marex@denx.de> wrote: > On 7/2/24 10:38 AM, Geert Uytterhoeven wrote: > > On Sun, Jun 30, 2024 at 5:47 AM Marek Vasut <marex@denx.de> wrote: > >> The rtl82xx DT bindings do not require ethernet-phy-ieee802.3-c22 > >> as the fallback compatible string. There are fewer users of the > >> Realtek PHY compatible string with fallback compatible string than > >> there are users without fallback compatible string, so drop the > >> fallback compatible string from the few remaining users: > >> > >> $ git grep -ho ethernet-phy-id001c....... | sort | uniq -c > >> 1 ethernet-phy-id001c.c816", > >> 2 ethernet-phy-id001c.c915", > >> 2 ethernet-phy-id001c.c915"; > >> 5 ethernet-phy-id001c.c916", > >> 13 ethernet-phy-id001c.c916"; > >> > >> Reported-by: kernel test robot <lkp@intel.com> > >> Closes: https://lore.kernel.org/oe-kbuild-all/202406290316.YvZdvLxu-lkp@intel.com/ > >> Signed-off-by: Marek Vasut <marex@denx.de> > > > > Thanks for your patch! > > > >> Note: this closes only part of the report > > > > In that case you should use a Link: instead of a Closes: tag? > > But which patch would be the one that Closes that report then ? The "last" one that goes in (in parallel with the others)? Yes, this is not easy to automate... > >> --- a/arch/arm64/boot/dts/renesas/cat875.dtsi > >> +++ b/arch/arm64/boot/dts/renesas/cat875.dtsi > >> @@ -22,8 +22,7 @@ &avb { > >> status = "okay"; > >> > >> phy0: ethernet-phy@0 { > >> - compatible = "ethernet-phy-id001c.c915", > >> - "ethernet-phy-ieee802.3-c22"; > >> + compatible = "ethernet-phy-id001c.c915"; > >> reg = <0>; > >> interrupt-parent = <&gpio2>; > >> interrupts = <21 IRQ_TYPE_LEVEL_LOW>; > > > > What about moving the PHYs inside an mdio subnode, and removing the > > compatible properties instead? That would protect against different > > board revisions using different PHYs or PHY revisions. > > > > According to Niklas[1], using an mdio subnode cancels the original > > reason (failure to identify the PHY in reset state after unbind/rebind > > or kexec) for adding the compatible values[2]. > > My understanding is that the compatible string is necessary if the PHY > needs clock/reset sequencing of any kind. Without the compatible string, > it is not possible to select the correct PHY driver which would handle > that sequencing according to the PHY requirements. This board here does > use reset-gpio property in the PHY node (it is not visible in this diff > context), so I believe a compatible string should be present here. With the introduction of an mdio subnode, the reset-gpios would move from the PHY node to the mio subnode, cfr. commit b4944dc7b7935a02 ("arm64: dts: renesas: white-hawk: ethernet: Describe AVB1 and AVB2") in linux-next. Niklas: commit 54bf0c27380b95a2 ("arm64: dts: renesas: r8a779g0: Use MDIO node for all AVB devices") did keep the reset-gpios property in the PHY node. I guess it should be moved one level up? Does the rtl82xx PHY have special reset sequencing requirements? > What would happen if this board got a revision with another PHY with > different PHY reset sequencing requirements ? The MDIO node level reset > handling might no longer be viable. True. However, please consider these two cases, both assuming reset-gpios is in the MDIO node: 1. The PHY node has a compatible value, and a different PHY is mounted: the new PHY will not work, as the wrong PHY driver is used. 2. The PHY node does not have a compatible value, and a different PHY is mounted: a. The new PHY does not need specific reset sequencing, and the existing reset-gpios is fine: the new PHY will just work, as it is auto-detected. b. The new PHY does need specific reset sequencing: the new PHY will not work. Which case is preferable? Case 1 or 2? Gr{oetje,eeting}s, Geert
On 2024-07-03 10:24:26 +0200, Geert Uytterhoeven wrote: > Niklas: commit 54bf0c27380b95a2 ("arm64: dts: renesas: r8a779g0: Use > MDIO node for all AVB devices") did keep the reset-gpios property in > the PHY node. I guess it should be moved one level up? It's possible to have a rest-gpios property both in the mdio node and the phy node. The former resets the whole bus while the later a single PHY, at least that's my understanding. I think it would be more correct to move the reset-gpios up one level for r8a779g0. However as it already was in the PHY node and this functioned as it should I kept it there. The need for the mdio node was to avoid a device specific property added in the BSP to reset the whole bus. At the moment I can't recall the two different call sites for when the two resets are called. Maybe if we move it to the mdio node we can avoid setting the PHY id in the compatible string as we could then always probe the PHY correctly. I will look into it.
diff --git a/arch/arm64/boot/dts/renesas/cat875.dtsi b/arch/arm64/boot/dts/renesas/cat875.dtsi index 8c9da8b4bd60b..502764aef210b 100644 --- a/arch/arm64/boot/dts/renesas/cat875.dtsi +++ b/arch/arm64/boot/dts/renesas/cat875.dtsi @@ -22,8 +22,7 @@ &avb { status = "okay"; phy0: ethernet-phy@0 { - compatible = "ethernet-phy-id001c.c915", - "ethernet-phy-ieee802.3-c22"; + compatible = "ethernet-phy-id001c.c915"; reg = <0>; interrupt-parent = <&gpio2>; interrupts = <21 IRQ_TYPE_LEVEL_LOW>; diff --git a/arch/arm64/boot/dts/renesas/hihope-rzg2-ex.dtsi b/arch/arm64/boot/dts/renesas/hihope-rzg2-ex.dtsi index ad898c6db4e62..d7a8de2619263 100644 --- a/arch/arm64/boot/dts/renesas/hihope-rzg2-ex.dtsi +++ b/arch/arm64/boot/dts/renesas/hihope-rzg2-ex.dtsi @@ -24,8 +24,7 @@ &avb { status = "okay"; phy0: ethernet-phy@0 { - compatible = "ethernet-phy-id001c.c915", - "ethernet-phy-ieee802.3-c22"; + compatible = "ethernet-phy-id001c.c915"; reg = <0>; interrupt-parent = <&gpio2>; interrupts = <11 IRQ_TYPE_LEVEL_LOW>; diff --git a/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts b/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts index 39fe3f94991e3..07147743de93f 100644 --- a/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts +++ b/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts @@ -100,8 +100,7 @@ &avb { status = "okay"; phy0: ethernet-phy@0 { - compatible = "ethernet-phy-id001c.c916", - "ethernet-phy-ieee802.3-c22"; + compatible = "ethernet-phy-id001c.c916"; reg = <0>; }; };
The rtl82xx DT bindings do not require ethernet-phy-ieee802.3-c22 as the fallback compatible string. There are fewer users of the Realtek PHY compatible string with fallback compatible string than there are users without fallback compatible string, so drop the fallback compatible string from the few remaining users: $ git grep -ho ethernet-phy-id001c....... | sort | uniq -c 1 ethernet-phy-id001c.c816", 2 ethernet-phy-id001c.c915", 2 ethernet-phy-id001c.c915"; 5 ethernet-phy-id001c.c916", 13 ethernet-phy-id001c.c916"; Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202406290316.YvZdvLxu-lkp@intel.com/ Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Conor Dooley <conor+dt@kernel.org> Cc: Geert Uytterhoeven <geert+renesas@glider.be> Cc: Khuong Dinh <khuong@os.amperecomputing.com> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org> Cc: Magnus Damm <magnus.damm@gmail.com> Cc: Rob Herring <robh@kernel.org> Cc: devicetree@vger.kernel.org Cc: linux-renesas-soc@vger.kernel.org --- Note: this closes only part of the report --- arch/arm64/boot/dts/renesas/cat875.dtsi | 3 +-- arch/arm64/boot/dts/renesas/hihope-rzg2-ex.dtsi | 3 +-- arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-)