Message-ID: <CAMdYzYoraXsZK3Uwe3QBVjt0cHn_GyOvR=57+qD-UrrwRE2TzA@mail.gmail.com>
Date: Fri, 20 Dec 2024 14:52:09 -0500
From: Peter Geis <pgwipeout@...il.com>
To: Alicja Michalska <alicja.michalska@...ements.com>
Cc: Jonas Karlman <jonas@...boo.se>, heiko@...ech.de, linux-rockchip@...ts.infradead.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64: dts: rockchip: ROCK3B: Correct clock rates for
 Ethernet PHYs

Good Afternoon,

On Fri, Dec 20, 2024 at 12:54 PM Alicja Michalska
<alicja.michalska@...ements.com> wrote:
>
> Hello Jonas,
>
>
> On 20/12/2024 18:09, Jonas Karlman wrote:
> > Have you used mainline or vendor u-boot when testing?
> >
> > There is an existing issue with RTL8211F PHYs that they must be reset
> > before Linux can identify them, see [1] for more details on the issue.
> >
> > Mainline U-Boot will reset the Ethernet PHYs on this board and that
> > should allow for Linux to identify and use the Ethernet PHYs.
> >
> > [1] https://lore.kernel.org/r/47d55aca-bee6-810f-379f-9431649fefa6@kwiboo.se/
>
> Thank you for the link. I've tested with vendor-provided U-Boot build:
>
> U-Boot latest-2023.10-11-cc60ff40-gcc60ff40 (Aug 14 2024 - 03:56:35 +0000)
>
> I can test with upstream in upcoming days.
>
> > The snps,reset props is deprecated and reset- props should be used in
> > the PHY node.
> >
> > This also changes to use 20/100 ms delay instead of current 20/50ms
> > delay. If anything it could be changed to 20/80ms, I have only seen
> > datasheets for rtl8211f mention minimum 10 ms and minimum 30-76 ms.
> >
> >> +
> >> +    tx_delay = <0x36>;
> >> +    rx_delay = <0x2d>;
> > Are you having issue with use of rgmii-id on your board?
>
> Interesting insight, thank you!
>
> I have to admit that I haven't read the current documentation, just seen that other RK356X boards in the tree used it. My apologies.
>
> I've ran into issues with rgmii-id, but that's with vendor U-Boot (as mentioned above), will try with upstream next and see a follow-up if I will still experience issues.

rgmii-id sets the phy to handle the tx and rx delays. All of the
various rgmii settings require the board to be built to specific
tolerances for the data lines. Rockchip based boards seem to forgo
this design phase, instead using rgmii (no built in delays) and set
the delays in the Rockchip gmac driver instead, using the tx_delay and
rx_delay values in the device tree. The tx_delay and rx_delay value
must be tuned correctly for your hardware. If you wish to use them
you'll need to set rgmii, then find the lowest and highest values that
work for all variations of hardware supported by the device tree then
take the middle value. If you wish to use rgmii-id, these values
should be omitted or set to zero as they are additive to the existing
delays.

>
> >> +
> >>      status = "okay";
> >>  };
> >>
> >>  &gmac1 {
> >>      assigned-clocks = <&cru SCLK_GMAC1_RX_TX>, <&cru SCLK_GMAC1>;
> >>      assigned-clock-parents = <&cru SCLK_GMAC1_RGMII_SPEED>, <&cru CLK_MAC1_2TOP>;
> >> -    clock_in_out = "input";
> >> -    phy-handle = <&rgmii_phy1>;
> >> -    phy-mode = "rgmii-id";
> >> +    assigned-clock-rates = <0>, <125000000>;
> >> +    clock_in_out = "output";

Instead, I suspect the clocks here are your issue. Looking at the
schematic at [1] your board is designed to feed the phy clock into the
SoC. This isn't the current clock configuration in your device tree,
you had it set to input, but the clock parents are wrong and will
default to using the internal SoC clock source for the mac. I suspect
setting the assigned clock rate is what fixed your issue.

I wrote the rk3566-quartz64-a.dts to be an example for how the new soc
behaves. I recommend looking at it for an example for how the clocks
are supposed to be set up for proper input [2].

> >>      phy-supply = <&vcc_3v3>;
> >> +    phy-mode = "rgmii";
> >> +    phy-handle = <&rgmii_phy1>;
> >>      pinctrl-names = "default";
> >>      pinctrl-0 = <&gmac1m1_miim
> >>                   &gmac1m1_tx_bus2
> >>                   &gmac1m1_rx_bus2
> >>                   &gmac1m1_rgmii_clk
> >> -                 &gmac1m1_rgmii_bus
> >> -                 &gmac1m1_clkinout>;
> >> +                 &gmac1m1_rgmii_bus>;
> >> +    snps,reset-gpio = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>;
> >> +    snps,reset-active-low;
> >> +    snps,reset-delays-us = <0 50000 150000>;
> > Same here, but now this is 50/150 ms?
> I wasn't sure about those delays, as I've taken them from vendor's sources. They did seem awfully high, though.
> >> +
> >> +    tx_delay = <0x47>;
> >> +    rx_delay = <0x28>;
> > Same here, is use of rgmii-id causing issues on your boards?
> Answer above :)
> >> +
> >>      status = "okay";
> >>  };
> >>
> >> +&mdio0 {
> >> +    rgmii_phy0: ethernet-phy@0 {
> >> +            compatible = "ethernet-phy-ieee802.3-c22";
> >> +            reg = <0>;
> > The phy addr used for these boards is 0x1.
> >
> >> +    };
> >> +};
> >> +
> >> +&mdio1 {
> >> +    rgmii_phy1: ethernet-phy@0 {
> >> +            compatible = "ethernet-phy-ieee802.3-c22";
> >> +            reg = <0>;
> > Same here.
> Makes sense, other boards in the tree had it configured the same way.
> >> +    };
> >> +};
> > There should be no need to move these nodes, they should already be in
> > alphabetical order?
> There's no reason to, just thought moving them closer to PHYs would look cleaner and make a bit more sense. It's just a nitpick though.
> >> +
> >>  &gpu {
> >>      mali-supply = <&vdd_gpu>;
> >>      status = "okay";
> >> @@ -512,26 +540,6 @@ &i2s1m0_sdi0
> >>      status = "okay";
> >>  };
> >>
> >> -&mdio0 {
> >> -    rgmii_phy0: ethernet-phy@1 {
> >> -            compatible = "ethernet-phy-ieee802.3-c22";
> > If you want to use vendor u-boot you can probably change this to
> > "ethernet-phy-id001c.c916".
> >
> >> -            reg = <1>;
> >> -            reset-assert-us = <20000>;
> >> -            reset-deassert-us = <50000>;
> >> -            reset-gpios = <&gpio3 RK_PB7 GPIO_ACTIVE_LOW>;
> >> -    };
> >> -};
> >> -
> >> -&mdio1 {
> >> -    rgmii_phy1: ethernet-phy@1 {
> >> -            compatible = "ethernet-phy-ieee802.3-c22";
> > Same here.
>
> True, but in this case it likely wouldn't be accepted in upstream, right?
>
> I really would prefer devicetrees to be passed to the OS by bootloader, just like ACPI does on x86 and some (i.e Ampere) platforms.
>
> This way we wouldn't need to maintain per-board DTs in the tree, and would keep functionality between kernel versions/BSDs... but one can dream, huh? :)

The device-tree is supposed to describe hardware without regards to a
specific operating system. If written correctly, it will work no
matter what software is running on top of it. While there are some
specific tweaks to make the operating systems behave correctly around
broken hardware, a lot of the deviations from this standard are due to
legacy trees and are being cleaned up over time.

>
> Thank you,
>
> Alicja

Very Respectfully,
Peter Geis

[1] https://dl.radxa.com/rock3/docs/hw/3b/Radxa_ROCK_3B_V1.51_SCH.pdf
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts?h=v6.13-rc3#n267

>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Powered by blists - more mailing lists