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

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

[Marek Vasut]

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/arm/boot/dts/renesas/r7s9210-rza2mevb.dts | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/renesas/r7s9210-rza2mevb.dts b/arch/arm/boot/dts/renesas/r7s9210-rza2mevb.dts
index cd2324b8e8ffb..41b1dbcfba100 100644
--- a/arch/arm/boot/dts/renesas/r7s9210-rza2mevb.dts
+++ b/arch/arm/boot/dts/renesas/r7s9210-rza2mevb.dts
@@ -95,8 +95,7 @@ &ether1 {
 	renesas,no-ether-link;
 	phy-handle = <&phy1>;
 	phy1: ethernet-phy@1 {
-		compatible = "ethernet-phy-id001c.c816",
-			     "ethernet-phy-ieee802.3-c22";
+		compatible = "ethernet-phy-id001c.c816";
 		reg = <0>;
 	};
 };
-- 
2.43.0

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

[Marek Vasut]

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

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>;
 	};
 };
-- 
2.43.0

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

[Andrew Lunn]

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

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

[Geert Uytterhoeven]

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

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

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

[Marek Vasut]

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.

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

[Geert Uytterhoeven]

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

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

[Niklas Söderlund]

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.

-- 
Kind Regards,
Niklas Söderlund

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

[Marek Vasut]

On 7/3/24 11:36 AM, Niklas Söderlund wrote:
> 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.

My understanding of reset GPIO in the MDIO node is that it is used in 
case there might be multiple PHYs with shared reset GPIO on the same 
MDIO bus. Like on the NXP iMX28 .

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

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

[Niklas Söderlund]

On 2024-07-05 23:49:56 +0200, Marek Vasut wrote:
> On 7/3/24 11:36 AM, Niklas Söderlund wrote:
> > 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.
> 
> My understanding of reset GPIO in the MDIO node is that it is used in case
> there might be multiple PHYs with shared reset GPIO on the same MDIO bus.
> Like on the NXP iMX28 .

There is a use-case for a single PHY on the MDIO bus too, at least in 
Linux as I understand it. If the boot process leave the PHY in a bad 
state which prevents it from being probed. A GPIO reset in the MDIO node 
is used when the MDIO bus is registered thus resetting all (in this 
use-case the one) PHYs which later allows them to be probed. A GPIO 
reset on the PHY node is only used after a PHY have been probed, at 
least that is my understanding.

This is the use-case for adding a MDIO node to the AVB driver and 
WhiteHawk.

-- 
Kind Regards,
Niklas Söderlund

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

[Geert Uytterhoeven]

Hi Niklas,

On Sat, Jul 6, 2024 at 10:39 AM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> On 2024-07-05 23:49:56 +0200, Marek Vasut wrote:
> > On 7/3/24 11:36 AM, Niklas Söderlund wrote:
> > > 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.
> >
> > My understanding of reset GPIO in the MDIO node is that it is used in case
> > there might be multiple PHYs with shared reset GPIO on the same MDIO bus.
> > Like on the NXP iMX28 .
>
> There is a use-case for a single PHY on the MDIO bus too, at least in
> Linux as I understand it. If the boot process leave the PHY in a bad
> state which prevents it from being probed. A GPIO reset in the MDIO node
> is used when the MDIO bus is registered thus resetting all (in this
> use-case the one) PHYs which later allows them to be probed. A GPIO
> reset on the PHY node is only used after a PHY have been probed, at
> least that is my understanding.

The reset is also asserted on driver unbind.

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

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

[Marek Vasut]

On 7/3/24 10:24 AM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

[...]

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

That's a different use case, that commit uses generic 
"ethernet-phy-ieee802.3-c45" compatible string and the PHY type is 
determined by reading out the PHY ID registers after the reset is released.

This here uses specific compatible string, so the kernel can determine 
the PHY ID from the DT before the reset is released .

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

Aside from reset timing, which does not seem to be in use here, not that 
I am aware of.

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

What is the likelihood of such PHY exchange happening with these three 
specific boards ? I think close to none, as that would require a board 
redesign to swap in a different PHY.

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

Or maybe, it will work by sheer chance, the reset timing would be off 
for the new PHY, but nobody will notice until much later. At least with 
the specific compatible string, such a PHY exchange would be detected 
outright.

>         b. The new PHY does need specific reset sequencing: the
>            new PHY will not work.
> 
> Which case is preferable? Case 1 or 2?

I think 1 because that is the well defined option which always works or 
always fails.

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

[Geert Uytterhoeven]

Hi Marek,

On Fri, Jul 5, 2024 at 11:50 PM Marek Vasut <marex@denx.de> wrote:
> On 7/3/24 10:24 AM, Geert Uytterhoeven wrote:
> >>> 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.
>
> That's a different use case, that commit uses generic
> "ethernet-phy-ieee802.3-c45" compatible string and the PHY type is
> determined by reading out the PHY ID registers after the reset is released.
>
> This here uses specific compatible string, so the kernel can determine
> the PHY ID from the DT before the reset is released .

I am suggesting removing the specific compatible string here, too,
introducing an mdio subnode, so the kernel can read it from the PHY
ID registers after the reset is released?

> >> 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.
>
> What is the likelihood of such PHY exchange happening with these three
> specific boards ? I think close to none, as that would require a board
> redesign to swap in a different PHY.

I don't know about the likelihood for these boards.
It did happen before on other boards, e.g. commit a0d23b8645b2d577
("arm64: dts: renesas: beacon-renesom: Update Ethernet PHY ID").

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

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

[Marek Vasut]

On 7/8/24 9:09 AM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Fri, Jul 5, 2024 at 11:50 PM Marek Vasut <marex@denx.de> wrote:
>> On 7/3/24 10:24 AM, Geert Uytterhoeven wrote:
>>>>> 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.
>>
>> That's a different use case, that commit uses generic
>> "ethernet-phy-ieee802.3-c45" compatible string and the PHY type is
>> determined by reading out the PHY ID registers after the reset is released.
>>
>> This here uses specific compatible string, so the kernel can determine
>> the PHY ID from the DT before the reset is released .
> 
> I am suggesting removing the specific compatible string here, too,
> introducing an mdio subnode, so the kernel can read it from the PHY
> ID registers after the reset is released?

I wrote this to Niklas already, so let me expand on it:

My understanding of reset GPIO in the MDIO node is that it is used in 
case there might be multiple PHYs with shared reset GPIO on the same 
MDIO bus. Like on the NXP iMX28 .

In this case, the reset is connected to this single PHY, so the reset 
line connection is a property of the PHY and should be described in the 
PHY node.

You could argue that in this case, because there is only one PHY and 
only one reset line, it fits both categories, PHY reset and MDIO reset.

And then, there is the future-proofing aspect.

If the compatible string is retained, then if in the future there is 
some problem discovered related to the reset of this PHY, the PHY driver 
can match on the compatible string and apply a fix up. But it prevents 
future PHY replacement (which is unlikely in my opinion).

If the compatible string is removed and the reset is moved to MDIO node, 
then replacement of the PHY in the future is likely possible (assuming 
it does not have any special reset timing requirements), but if there is 
a problem related to the reset of the current PHY model, the PHY driver 
cannot fix it up because there is no compatible to match on.

I think that about sums the pros and cons up, right ?

I also think there is no good solution here, only two bad ones, with 
different issues each.

>>>> 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.
>>
>> What is the likelihood of such PHY exchange happening with these three
>> specific boards ? I think close to none, as that would require a board
>> redesign to swap in a different PHY.
> 
> I don't know about the likelihood for these boards.
> It did happen before on other boards, e.g. commit a0d23b8645b2d577
> ("arm64: dts: renesas: beacon-renesom: Update Ethernet PHY ID").

I had that happen too. The solution there was to upstream the newer PHY 
ID and apply backward compatibility DTO that rewrote the PHY ID for the 
few older boards. The DTO application decision was done in U-Boot scripting.

It was not possible to auto-detect the PHY after deasserting its reset 
in my case, I had to determine whether to apply DTO or not based on 
strap resistors on the board.

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

[Adam Ford]

On Mon, Jul 8, 2024 at 10:34 PM Marek Vasut <marex@denx.de> wrote:
>
> On 7/8/24 9:09 AM, Geert Uytterhoeven wrote:
> > Hi Marek,
>
> Hi,
>
> > On Fri, Jul 5, 2024 at 11:50 PM Marek Vasut <marex@denx.de> wrote:
> >> On 7/3/24 10:24 AM, Geert Uytterhoeven wrote:
> >>>>> 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.
> >>
> >> That's a different use case, that commit uses generic
> >> "ethernet-phy-ieee802.3-c45" compatible string and the PHY type is
> >> determined by reading out the PHY ID registers after the reset is released.
> >>
> >> This here uses specific compatible string, so the kernel can determine
> >> the PHY ID from the DT before the reset is released .
> >
> > I am suggesting removing the specific compatible string here, too,
> > introducing an mdio subnode, so the kernel can read it from the PHY
> > ID registers after the reset is released?
>
> I wrote this to Niklas already, so let me expand on it:
>
> My understanding of reset GPIO in the MDIO node is that it is used in
> case there might be multiple PHYs with shared reset GPIO on the same
> MDIO bus. Like on the NXP iMX28 .
>
> In this case, the reset is connected to this single PHY, so the reset
> line connection is a property of the PHY and should be described in the
> PHY node.
>
> You could argue that in this case, because there is only one PHY and
> only one reset line, it fits both categories, PHY reset and MDIO reset.
>
> And then, there is the future-proofing aspect.
>
> If the compatible string is retained, then if in the future there is
> some problem discovered related to the reset of this PHY, the PHY driver
> can match on the compatible string and apply a fix up. But it prevents
> future PHY replacement (which is unlikely in my opinion).
>
> If the compatible string is removed and the reset is moved to MDIO node,
> then replacement of the PHY in the future is likely possible (assuming
> it does not have any special reset timing requirements), but if there is
> a problem related to the reset of the current PHY model, the PHY driver
> cannot fix it up because there is no compatible to match on.
>
> I think that about sums the pros and cons up, right ?
>
> I also think there is no good solution here, only two bad ones, with
> different issues each.
>
> >>>> 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.
> >>
> >> What is the likelihood of such PHY exchange happening with these three
> >> specific boards ? I think close to none, as that would require a board
> >> redesign to swap in a different PHY.
> >
> > I don't know about the likelihood for these boards.
> > It did happen before on other boards, e.g. commit a0d23b8645b2d577
> > ("arm64: dts: renesas: beacon-renesom: Update Ethernet PHY ID").
>
> I had that happen too. The solution there was to upstream the newer PHY
> ID and apply backward compatibility DTO that rewrote the PHY ID for the
> few older boards. The DTO application decision was done in U-Boot scripting.
>
> It was not possible to auto-detect the PHY after deasserting its reset
> in my case, I had to determine whether to apply DTO or not based on
> strap resistors on the board.

Beacon had to swap the phy due to the great part shortage of 2021 and
having the hard-code ID's prevented backwards compatibility.  For the
Beacon downstream kernel, we removed the PHY ID and kept the generic
'ethernet-phy-ieee802.3-c22' because it could auto-detect what we
needed and both PHY's appear to come out of reset and register
properly.  I don't know if we could make a generic phy-ieee802.3-c22
handle the reset, wait a moment and then try to auto-detect, but it
would be nice to not have to jump through hoops if/when people need to
change PHY's.

adam

>

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

[Marek Vasut]

On 7/10/24 1:37 AM, Adam Ford wrote:
> On Mon, Jul 8, 2024 at 10:34 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 7/8/24 9:09 AM, Geert Uytterhoeven wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On Fri, Jul 5, 2024 at 11:50 PM Marek Vasut <marex@denx.de> wrote:
>>>> On 7/3/24 10:24 AM, Geert Uytterhoeven wrote:
>>>>>>> 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.
>>>>
>>>> That's a different use case, that commit uses generic
>>>> "ethernet-phy-ieee802.3-c45" compatible string and the PHY type is
>>>> determined by reading out the PHY ID registers after the reset is released.
>>>>
>>>> This here uses specific compatible string, so the kernel can determine
>>>> the PHY ID from the DT before the reset is released .
>>>
>>> I am suggesting removing the specific compatible string here, too,
>>> introducing an mdio subnode, so the kernel can read it from the PHY
>>> ID registers after the reset is released?
>>
>> I wrote this to Niklas already, so let me expand on it:
>>
>> My understanding of reset GPIO in the MDIO node is that it is used in
>> case there might be multiple PHYs with shared reset GPIO on the same
>> MDIO bus. Like on the NXP iMX28 .
>>
>> In this case, the reset is connected to this single PHY, so the reset
>> line connection is a property of the PHY and should be described in the
>> PHY node.
>>
>> You could argue that in this case, because there is only one PHY and
>> only one reset line, it fits both categories, PHY reset and MDIO reset.
>>
>> And then, there is the future-proofing aspect.
>>
>> If the compatible string is retained, then if in the future there is
>> some problem discovered related to the reset of this PHY, the PHY driver
>> can match on the compatible string and apply a fix up. But it prevents
>> future PHY replacement (which is unlikely in my opinion).
>>
>> If the compatible string is removed and the reset is moved to MDIO node,
>> then replacement of the PHY in the future is likely possible (assuming
>> it does not have any special reset timing requirements), but if there is
>> a problem related to the reset of the current PHY model, the PHY driver
>> cannot fix it up because there is no compatible to match on.
>>
>> I think that about sums the pros and cons up, right ?
>>
>> I also think there is no good solution here, only two bad ones, with
>> different issues each.
>>
>>>>>> 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.
>>>>
>>>> What is the likelihood of such PHY exchange happening with these three
>>>> specific boards ? I think close to none, as that would require a board
>>>> redesign to swap in a different PHY.
>>>
>>> I don't know about the likelihood for these boards.
>>> It did happen before on other boards, e.g. commit a0d23b8645b2d577
>>> ("arm64: dts: renesas: beacon-renesom: Update Ethernet PHY ID").
>>
>> I had that happen too. The solution there was to upstream the newer PHY
>> ID and apply backward compatibility DTO that rewrote the PHY ID for the
>> few older boards. The DTO application decision was done in U-Boot scripting.
>>
>> It was not possible to auto-detect the PHY after deasserting its reset
>> in my case, I had to determine whether to apply DTO or not based on
>> strap resistors on the board.
> 
> Beacon had to swap the phy due to the

Oh ...

> great part shortage of 2021

... I'll borrow this quote.

> and
> having the hard-code ID's prevented backwards compatibility.  For the
> Beacon downstream kernel, we removed the PHY ID and kept the generic
> 'ethernet-phy-ieee802.3-c22' because it could auto-detect what we
> needed and both PHY's appear to come out of reset and register
> properly.  I don't know if we could make a generic phy-ieee802.3-c22
> handle the reset, wait a moment and then try to auto-detect, but it
> would be nice to not have to jump through hoops if/when people need to
> change PHY's.

The reset handling and clock handling is the main issue here. Maybe we 
should cross that bridge when we come to it ? I.e. when someone needs to 
replace a PHY, let's deal with the requirement then ?

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

[Andrew Lunn]

On Sun, Jun 30, 2024 at 05:46:17AM +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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Geert Uytterhoeven]

Hi Marek,

Thanks for your patch!

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

Why not?

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

Ah, the self-fulfilling collection of patches ;-)

Let's move the discussion to the actual patch that causes this
https://lore.kernel.org/all/20240625184359.153423-1-marex@denx.de/

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

Hmm, nothing about r7s9210-rza2mevb in that report.

> 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

Please do not use the "Closes" tag if it does not fix everything in
the report. "Link" (pointing to the correct report!) is fine, though.

> --- a/arch/arm/boot/dts/renesas/r7s9210-rza2mevb.dts
> +++ b/arch/arm/boot/dts/renesas/r7s9210-rza2mevb.dts
> @@ -95,8 +95,7 @@ &ether1 {
>         renesas,no-ether-link;
>         phy-handle = <&phy1>;
>         phy1: ethernet-phy@1 {
> -               compatible = "ethernet-phy-id001c.c816",
> -                            "ethernet-phy-ieee802.3-c22";
> +               compatible = "ethernet-phy-id001c.c816";
>                 reg = <0>;
>         };
>  };

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