Re: [PATCH 2/2] arm64: dts: renesas: Drop ethernet-phy-ieee802.3-c22 from PHY compatible string on all RZ boards

[Geert Uytterhoeven <geert@linux-m68k.org>]

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

-- 
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