Hi Marek, On Tue, Jul 2, 2024 at 10:45 PM Marek Vasut <marex@xxxxxxx> wrote: > On 7/2/24 10:38 AM, Geert Uytterhoeven wrote: > > On Sun, Jun 30, 2024 at 5:47 AM Marek Vasut <marex@xxxxxxx> 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@xxxxxxxxx> > >> Closes: https://lore.kernel.org/oe-kbuild-all/202406290316.YvZdvLxu-lkp@xxxxxxxxx/ > >> Signed-off-by: Marek Vasut <marex@xxxxxxx> > > > > 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 -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx 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
