diff mbox series

[linux-next-v2,1/5] arm64: dts: rockchip: Fix gmac phy mode to rgmii on Rock 3A SBC.

Message ID 20221116200150.4657-2-linux.amoon@gmail.com
State New
Headers
Series [linux-next-v2,1/5] arm64: dts: rockchip: Fix gmac phy mode to rgmii on Rock 3A SBC. |

Commit Message

Anand Moon Nov. 16, 2022, 8:01 p.m. UTC 
  On rk356x ethernet phy support reduced media independent interface (RMII)
and reduced gigabit media independent interface (RGMII).
So set the phy mode to rgmii to support clock delay, also
add TX and RX delay for phy-mode.

Fix following warning

[    7.365215] rk_gmac-dwmac fe010000.ethernet: Can not read property: tx_delay.
[    7.365219] rk_gmac-dwmac fe010000.ethernet: set tx_delay to 0x30
[    7.365224] rk_gmac-dwmac fe010000.ethernet: Can not read property: rx_delay.
[    7.365228] rk_gmac-dwmac fe010000.ethernet: set rx_delay to 0x10

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
V2: Fix commit message and added the RX and TX clock delay.
---
 arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Peter Geis Nov. 16, 2022, 8:36 p.m. UTC | #1 
On Wed, Nov 16, 2022 at 3:02 PM Anand Moon <linux.amoon@gmail.com> wrote:
>
> On rk356x ethernet phy support reduced media independent interface (RMII)
> and reduced gigabit media independent interface (RGMII).
> So set the phy mode to rgmii to support clock delay, also
> add TX and RX delay for phy-mode.

Controller based clock delay, the various rgmii-id modes (rgmii-txid
and rgmii-rxid are also valid) apply the delays in the phy. They are
usually at a fixed amount, but some phys support variable delays.

You want your commit message to accurately describe the problem, such
as "In rgmii-id mode, the phy on the rock-3a is unreliable due to
incorrect delays. Switch to rgmii mode in order to handle the delays
in the controller."

>
> Fix following warning
>
> [    7.365215] rk_gmac-dwmac fe010000.ethernet: Can not read property: tx_delay.
> [    7.365219] rk_gmac-dwmac fe010000.ethernet: set tx_delay to 0x30
> [    7.365224] rk_gmac-dwmac fe010000.ethernet: Can not read property: rx_delay.
> [    7.365228] rk_gmac-dwmac fe010000.ethernet: set rx_delay to 0x10

I've been meaning to make this a dev_debug message, because in the
various rgmii-id modes it is feasible for these to be non-existent in
the device-tree. In rgmii-id mode these are disabled, no matter what
they are set to in the dt.

>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> V2: Fix commit message and added the RX and TX clock delay.
> ---
>  arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
> index ea74ba32fbbd..e1c75532dcee 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
> @@ -253,13 +253,16 @@ &gmac1 {
>         assigned-clock-rates = <0>, <125000000>;
>         clock_in_out = "output";
>         phy-handle = <&rgmii_phy1>;
> -       phy-mode = "rgmii-id";
> +       phy-mode = "rgmii";
>         pinctrl-names = "default";
>         pinctrl-0 = <&gmac1m1_miim
>                      &gmac1m1_tx_bus2
>                      &gmac1m1_rx_bus2
>                      &gmac1m1_rgmii_clk
>                      &gmac1m1_rgmii_bus>;
> +
> +       tx_delay = <0x4f>;
> +       rx_delay = <0x26>;

These are pretty far off from the default, have you verified the upper
and lower bounds for the rock-3a? These should be roughly in the
middle of that range.

>         status = "okay";
>  };
>
> --
> 2.38.1
>
Anand Moon Nov. 17, 2022, 2:44 p.m. UTC | #2 
Hi Peter

On Thu, 17 Nov 2022 at 02:06, Peter Geis <pgwipeout@gmail.com> wrote:
>
> On Wed, Nov 16, 2022 at 3:02 PM Anand Moon <linux.amoon@gmail.com> wrote:
> >
> > On rk356x ethernet phy support reduced media independent interface (RMII)
> > and reduced gigabit media independent interface (RGMII).
> > So set the phy mode to rgmii to support clock delay, also
> > add TX and RX delay for phy-mode.
>
> Controller based clock delay, the various rgmii-id modes (rgmii-txid
> and rgmii-rxid are also valid) apply the delays in the phy. They are
> usually at a fixed amount, but some phys support variable delays.
>
> You want your commit message to accurately describe the problem, such
> as "In rgmii-id mode, the phy on the rock-3a is unreliable due to
> incorrect delays. Switch to rgmii mode in order to handle the delays
> in the controller."
>
> >
> > Fix following warning
> >
> > [    7.365215] rk_gmac-dwmac fe010000.ethernet: Can not read property: tx_delay.
> > [    7.365219] rk_gmac-dwmac fe010000.ethernet: set tx_delay to 0x30
> > [    7.365224] rk_gmac-dwmac fe010000.ethernet: Can not read property: rx_delay.
> > [    7.365228] rk_gmac-dwmac fe010000.ethernet: set rx_delay to 0x10
>
> I've been meaning to make this a dev_debug message, because in the
> various rgmii-id modes it is feasible for these to be non-existent in
> the device-tree. In rgmii-id mode these are disabled, no matter what
> they are set to in the dt.

As per the public datasheet share by Radxa below
[0] https://dl.radxa.com/rock3/docs/hw/datasheet/RTL8211F-CG-Datasheet.pdf

This Ethernet controller supports RGMII mode.

>
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > V2: Fix commit message and added the RX and TX clock delay.
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
> > index ea74ba32fbbd..e1c75532dcee 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
> > @@ -253,13 +253,16 @@ &gmac1 {
> >         assigned-clock-rates = <0>, <125000000>;
> >         clock_in_out = "output";
> >         phy-handle = <&rgmii_phy1>;
> > -       phy-mode = "rgmii-id";
> > +       phy-mode = "rgmii";
> >         pinctrl-names = "default";
> >         pinctrl-0 = <&gmac1m1_miim
> >                      &gmac1m1_tx_bus2
> >                      &gmac1m1_rx_bus2
> >                      &gmac1m1_rgmii_clk
> >                      &gmac1m1_rgmii_bus>;
> > +
> > +       tx_delay = <0x4f>;
> > +       rx_delay = <0x26>;
>
> These are pretty far off from the default, have you verified the upper
> and lower bounds for the rock-3a? These should be roughly in the
> middle of that range.
>
No I have not tested this way but as per the datasheet, it supports

TXDLY   Add 2ns delay to RXC for RXD latching (via 4.7k-ohm to DVDD_RG)
RXDLY   Add 2ns delay to RXC for RXD latching (via 4.7k-ohm to DVDD_RG)

I feel I should ignore the above warning and add it below.

rx-internal-delay-ps = <2000>;
tx-internal-delay-ps = <2000>;

Thanks
-Anand
> >         status = "okay";
> >  };
> >
> > --
> > 2.38.1
> >
Michael Riesch Nov. 18, 2022, 7:03 a.m. UTC | #3 
Hi Anand,

On 11/16/22 21:01, Anand Moon wrote:
> On rk356x ethernet phy support reduced media independent interface (RMII)
> and reduced gigabit media independent interface (RGMII).
> So set the phy mode to rgmii to support clock delay, also
> add TX and RX delay for phy-mode.

Based on this commit message I still don't understand what you are
actually trying to fix here. If you encounter network problems/stability
issues, please let me know what test triggers the faulty behavior.
Please describe the problem you are facing in detail here or in the
cover letter.

> Fix following warning
> 
> [    7.365215] rk_gmac-dwmac fe010000.ethernet: Can not read property: tx_delay.
> [    7.365219] rk_gmac-dwmac fe010000.ethernet: set tx_delay to 0x30
> [    7.365224] rk_gmac-dwmac fe010000.ethernet: Can not read property: rx_delay.
> [    7.365228] rk_gmac-dwmac fe010000.ethernet: set rx_delay to 0x10

If the only purpose of this patch is to get rid of this warnings, it may
make sense to set them to dev_dbg as Peter pointed out.

Best regards,
Michael

> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> V2: Fix commit message and added the RX and TX clock delay.
> ---
>  arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
> index ea74ba32fbbd..e1c75532dcee 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
> @@ -253,13 +253,16 @@ &gmac1 {
>  	assigned-clock-rates = <0>, <125000000>;
>  	clock_in_out = "output";
>  	phy-handle = <&rgmii_phy1>;
> -	phy-mode = "rgmii-id";
> +	phy-mode = "rgmii";
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&gmac1m1_miim
>  		     &gmac1m1_tx_bus2
>  		     &gmac1m1_rx_bus2
>  		     &gmac1m1_rgmii_clk
>  		     &gmac1m1_rgmii_bus>;
> +
> +	tx_delay = <0x4f>;
> +	rx_delay = <0x26>;
>  	status = "okay";
>  };
>
Anand Moon Nov. 18, 2022, 9:34 a.m. UTC | #4 
Hi Michael,

On Fri, 18 Nov 2022 at 12:33, Michael Riesch
<michael.riesch@wolfvision.net> wrote:
>
> Hi Anand,
>
> On 11/16/22 21:01, Anand Moon wrote:
> > On rk356x ethernet phy support reduced media independent interface (RMII)
> > and reduced gigabit media independent interface (RGMII).
> > So set the phy mode to rgmii to support clock delay, also
> > add TX and RX delay for phy-mode.
>
> Based on this commit message I still don't understand what you are
> actually trying to fix here. If you encounter network problems/stability
> issues, please let me know what test triggers the faulty behavior.
> Please describe the problem you are facing in detail here or in the
> cover letter.
>

Ok, Ethernet does not work on my Radxa 3A see boot logs.

[0] https://gist.github.com/moonlinux/bb56c787031226fbb9f69121564e76a2

Please find this updated commit message.

As per the schematic and datasheet PHY mode is RGMII
Use 2ns clock delay to RXC for RXD and TXC for TXD latching.

> > Fix the following warning
> >
> > [    7.365215] rk_gmac-dwmac fe010000.ethernet: Can not read property: tx_delay.
> > [    7.365219] rk_gmac-dwmac fe010000.ethernet: set tx_delay to 0x30
> > [    7.365224] rk_gmac-dwmac fe010000.ethernet: Can not read property: rx_delay.
> > [    7.365228] rk_gmac-dwmac fe010000.ethernet: set rx_delay to 0x10
>
> If the only purpose of this patch is to get rid of this warnings, it may

No, the intent is to fix the PHY mode to RGMII and fix the delay.
[ 7.066357] rk_gmac-dwmac fe010000.ethernet: init for RGMII_ID

> make sense to set them to dev_dbg as Peter pointed out.
>
Ok, will update this in the next version.

> Best regards,
> Michael
>
Thanks
-Anand
Peter Geis Nov. 18, 2022, 6:13 p.m. UTC | #5 
On Fri, Nov 18, 2022 at 4:35 AM Anand Moon <linux.amoon@gmail.com> wrote:
>
> Hi Michael,
>
> On Fri, 18 Nov 2022 at 12:33, Michael Riesch
> <michael.riesch@wolfvision.net> wrote:
> >
> > Hi Anand,
> >
> > On 11/16/22 21:01, Anand Moon wrote:
> > > On rk356x ethernet phy support reduced media independent interface (RMII)
> > > and reduced gigabit media independent interface (RGMII).
> > > So set the phy mode to rgmii to support clock delay, also
> > > add TX and RX delay for phy-mode.
> >
> > Based on this commit message I still don't understand what you are
> > actually trying to fix here. If you encounter network problems/stability
> > issues, please let me know what test triggers the faulty behavior.
> > Please describe the problem you are facing in detail here or in the
> > cover letter.
> >
>
> Ok, Ethernet does not work on my Radxa 3A see boot logs.
>
> [0] https://gist.github.com/moonlinux/bb56c787031226fbb9f69121564e76a2
>
> Please find this updated commit message.
>
> As per the schematic and datasheet PHY mode is RGMII
> Use 2ns clock delay to RXC for RXD and TXC for TXD latching.

rgmii-id mode does exactly this in the phy (your realtek chip). By
setting the mode to rgmii, you're telling the phy that delays are set
elsewhere, either in hardware or in the controller. You're then
handling them in the controller. While the delays aren't documented in
the TRM, I've long suspected that the defaults of 0x30 and 0x10 equate
to the standard 2ns delay. So you're setting the delays much higher
than the default means you need to add *more* than the standard 2ns
delay for your device to work.

This is why I've been asking if you have tested these. You need to set
each value and find the lowest and highest possible values that work,
then take the median value between those two.

>
> > > Fix the following warning
> > >
> > > [    7.365215] rk_gmac-dwmac fe010000.ethernet: Can not read property: tx_delay.
> > > [    7.365219] rk_gmac-dwmac fe010000.ethernet: set tx_delay to 0x30
> > > [    7.365224] rk_gmac-dwmac fe010000.ethernet: Can not read property: rx_delay.
> > > [    7.365228] rk_gmac-dwmac fe010000.ethernet: set rx_delay to 0x10
> >
> > If the only purpose of this patch is to get rid of this warnings, it may
>
> No, the intent is to fix the PHY mode to RGMII and fix the delay.
> [ 7.066357] rk_gmac-dwmac fe010000.ethernet: init for RGMII_ID
>
> > make sense to set them to dev_dbg as Peter pointed out.
> >
> Ok, will update this in the next version.
>
> > Best regards,
> > Michael
> >
> Thanks
> -Anand
Anand Moon Nov. 22, 2022, 2:19 p.m. UTC | #6 
Hi Peter / Michael

On Fri, 18 Nov 2022 at 23:43, Peter Geis <pgwipeout@gmail.com> wrote:
>
> On Fri, Nov 18, 2022 at 4:35 AM Anand Moon <linux.amoon@gmail.com> wrote:
> >
> > Hi Michael,
> >
> > On Fri, 18 Nov 2022 at 12:33, Michael Riesch
> > <michael.riesch@wolfvision.net> wrote:
> > >
> > > Hi Anand,
> > >
> > > On 11/16/22 21:01, Anand Moon wrote:
> > > > On rk356x ethernet phy support reduced media independent interface (RMII)
> > > > and reduced gigabit media independent interface (RGMII).
> > > > So set the phy mode to rgmii to support clock delay, also
> > > > add TX and RX delay for phy-mode.
> > >
> > > Based on this commit message I still don't understand what you are
> > > actually trying to fix here. If you encounter network problems/stability
> > > issues, please let me know what test triggers the faulty behavior.
> > > Please describe the problem you are facing in detail here or in the
> > > cover letter.
> > >
> >
> > Ok, Ethernet does not work on my Radxa 3A see boot logs.
> >
> > [0] https://gist.github.com/moonlinux/bb56c787031226fbb9f69121564e76a2
> >
> > Please find this updated commit message.
> >
> > As per the schematic and datasheet PHY mode is RGMII
> > Use 2ns clock delay to RXC for RXD and TXC for TXD latching.
>
> rgmii-id mode does exactly this in the phy (your realtek chip). By
> setting the mode to rgmii, you're telling the phy that delays are set
> elsewhere, either in hardware or in the controller. You're then
> handling them in the controller. While the delays aren't documented in
> the TRM, I've long suspected that the defaults of 0x30 and 0x10 equate
> to the standard 2ns delay. So you're setting the delays much higher
> than the default means you need to add *more* than the standard 2ns
> delay for your device to work.
>

TX and RX clock delay might have been updated for
     ID: 0x30, Synopsys ID: 0x51  DWMAC4/5

As per the schematic [0]
https://dl.radxa.com/rock3/docs/hw/3a/rock3a_v1.3_sch.pdf
   Pull-up for additional 2ns delay to RXC for data latching
   Pull-up for additional 2ns delay to TXC for data latching

As per the datasheet [1]
https://dl.radxa.com/rock3/docs/hw/datasheet/RTL8211F-CG-Datasheet.pdf
TXDLY   RGMII            Transmit Clock Timing Control.
    1: Add 2ns delay to RXC for RXD latching (via 4.7k-ohm to DVDD_RG)
    0: No delay (via 4.7k-ohm to GND)
RXDLY RGMII Receive Clock Timing Control.
    1: Add 2ns delay to RXC for RXD latching (via 4.7k-ohm to DVDD_RG)
    0: No delay (via 4.7k-ohm to GND)

tx_delay and rx_delay might be suitable for old DWMAC
hence it does not apply to this new Ethernet controller.
I have tested with remove these from the dts and
adding the following as this is required for rgmii mode.
       rx-internal-delay-ps = <2000>;
       tx-internal-delay-ps = <2000>;

I don't mind setting the defaults of 0x30 and 0x10 to equate
to the standard 2ns delay. but it does not have any effect
on the current ethernet controller.

> This is why I've been asking if you have tested these. You need to set
> each value and find the lowest and highest possible values that work,
> then take the median value between those two.
>

Yes, I have tested with and without those values.

Thanks
-Anand
> >
> > > > Fix the following warning
> > > >
> > > > [    7.365215] rk_gmac-dwmac fe010000.ethernet: Can not read property: tx_delay.
> > > > [    7.365219] rk_gmac-dwmac fe010000.ethernet: set tx_delay to 0x30
> > > > [    7.365224] rk_gmac-dwmac fe010000.ethernet: Can not read property: rx_delay.
> > > > [    7.365228] rk_gmac-dwmac fe010000.ethernet: set rx_delay to 0x10
> > >
> > > If the only purpose of this patch is to get rid of this warnings, it may
> >
> > No, the intent is to fix the PHY mode to RGMII and fix the delay.
> > [ 7.066357] rk_gmac-dwmac fe010000.ethernet: init for RGMII_ID
> >
> > > make sense to set them to dev_dbg as Peter pointed out.
> > >
> > Ok, will update this in the next version.
> >
> > > Best regards,
> > > Michael
> > >
> > Thanks
> > -Anand
Heiko Stübner Nov. 22, 2022, 10:34 p.m. UTC | #7 
On Wed, 16 Nov 2022 20:01:43 +0000, Anand Moon wrote:
> On rk356x ethernet phy support reduced media independent interface (RMII)
> and reduced gigabit media independent interface (RGMII).
> So set the phy mode to rgmii to support clock delay, also
> add TX and RX delay for phy-mode.
> 
> Fix following warning
> 
> [...]

Applied, thanks!

[2/5] arm64: dts: rockchip: Add support of external clock to ethernet node on Rock 3A SBC
      commit: ef9f4b4a50206bedd931f45dd9fd57fd4c1714a6
[3/5] arm64: dts: rockchip: Add support of regulator for ethernet node on Rock 3A SBC
      commit: 79aa02ddc682558edb9bd56522ad841759c99201

Best regards,
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
index ea74ba32fbbd..e1c75532dcee 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
@@ -253,13 +253,16 @@  &gmac1 {
 	assigned-clock-rates = <0>, <125000000>;
 	clock_in_out = "output";
 	phy-handle = <&rgmii_phy1>;
-	phy-mode = "rgmii-id";
+	phy-mode = "rgmii";
 	pinctrl-names = "default";
 	pinctrl-0 = <&gmac1m1_miim
 		     &gmac1m1_tx_bus2
 		     &gmac1m1_rx_bus2
 		     &gmac1m1_rgmii_clk
 		     &gmac1m1_rgmii_bus>;
+
+	tx_delay = <0x4f>;
+	rx_delay = <0x26>;
 	status = "okay";
 };