[PATCH] arm64: dts: rockchip: change eth phy mode to rgmii-id for orangepi r1 plus lts

[PATCH] arm64: dts: rockchip: change eth phy mode to rgmii-id for orangepi r1 plus lts

[Tianling Shen]

In general the delay should be added by the PHY instead of the MAC,
and this improves network stability on some boards which seem to
need different delay.

Fixes: 387b3bbac5ea ("arm64: dts: rockchip: Add Xunlong OrangePi R1 Plus LTS")
Cc: stable@vger.kernel.org # 6.6+
Signed-off-by: Tianling Shen <cnsztl@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts | 3 +--
 arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts     | 1 +
 arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi    | 1 -
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
index 67c246ad8b8c..ec2ce894da1f 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
@@ -17,8 +17,7 @@ / {
 
 &gmac2io {
 	phy-handle = <&yt8531c>;
-	tx_delay = <0x19>;
-	rx_delay = <0x05>;
+	phy-mode = "rgmii-id";
 	status = "okay";
 
 	mdio {
diff --git a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
index 324a8e951f7e..846b931e16d2 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
@@ -15,6 +15,7 @@ / {
 
 &gmac2io {
 	phy-handle = <&rtl8211e>;
+	phy-mode = "rgmii";
 	tx_delay = <0x24>;
 	rx_delay = <0x18>;
 	status = "okay";
diff --git a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
index 4f193704e5dc..09508e324a28 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
@@ -109,7 +109,6 @@ &gmac2io {
 	assigned-clocks = <&cru SCLK_MAC2IO>, <&cru SCLK_MAC2IO_EXT>;
 	assigned-clock-parents = <&gmac_clk>, <&gmac_clk>;
 	clock_in_out = "input";
-	phy-mode = "rgmii";
 	phy-supply = <&vcc_io>;
 	pinctrl-0 = <&rgmiim1_pins>;
 	pinctrl-names = "default";
-- 
2.48.1

Re: [PATCH] arm64: dts: rockchip: change eth phy mode to rgmii-id for orangepi r1 plus lts

[Dragan Simic]

Hello Tianling,

Thanks for the patch.  Please, see a comment below.

On 2025-01-19 10:11, Tianling Shen wrote:
> In general the delay should be added by the PHY instead of the MAC,
> and this improves network stability on some boards which seem to
> need different delay.
> 
> Fixes: 387b3bbac5ea ("arm64: dts: rockchip: Add Xunlong OrangePi R1 
> Plus LTS")
> Cc: stable@vger.kernel.org # 6.6+
> Signed-off-by: Tianling Shen <cnsztl@gmail.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts | 3 +--
>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts     | 1 +
>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi    | 1 -
>  3 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git
> a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
> index 67c246ad8b8c..ec2ce894da1f 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
> @@ -17,8 +17,7 @@ / {
> 
>  &gmac2io {
>  	phy-handle = <&yt8531c>;
> -	tx_delay = <0x19>;
> -	rx_delay = <0x05>;
> +	phy-mode = "rgmii-id";

Shouldn't the "tx_delay" and "rx_delay" DT parameters be converted
into the "tx-internal-delay-ps" and "rx-internal-delay-ps" parameters,
respectively, so the Motorcomm PHY driver can pick them up and
actually configure the internal PHY delays?

>  	status = "okay";
> 
>  	mdio {
> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
> index 324a8e951f7e..846b931e16d2 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
> @@ -15,6 +15,7 @@ / {
> 
>  &gmac2io {
>  	phy-handle = <&rtl8211e>;
> +	phy-mode = "rgmii";
>  	tx_delay = <0x24>;
>  	rx_delay = <0x18>;
>  	status = "okay";
> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
> index 4f193704e5dc..09508e324a28 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
> @@ -109,7 +109,6 @@ &gmac2io {
>  	assigned-clocks = <&cru SCLK_MAC2IO>, <&cru SCLK_MAC2IO_EXT>;
>  	assigned-clock-parents = <&gmac_clk>, <&gmac_clk>;
>  	clock_in_out = "input";
> -	phy-mode = "rgmii";
>  	phy-supply = <&vcc_io>;
>  	pinctrl-0 = <&rgmiim1_pins>;
>  	pinctrl-names = "default";

Re: [PATCH] arm64: dts: rockchip: change eth phy mode to rgmii-id for orangepi r1 plus lts

[Tianling Shen]

Hi Dragan,

On 2025/1/19 17:54, Dragan Simic wrote:
> Hello Tianling,
> 
> Thanks for the patch.  Please, see a comment below.
> 
> On 2025-01-19 10:11, Tianling Shen wrote:
>> In general the delay should be added by the PHY instead of the MAC,
>> and this improves network stability on some boards which seem to
>> need different delay.
>>
>> Fixes: 387b3bbac5ea ("arm64: dts: rockchip: Add Xunlong OrangePi R1 
>> Plus LTS")
>> Cc: stable@vger.kernel.org # 6.6+
>> Signed-off-by: Tianling Shen <cnsztl@gmail.com>
>> ---
>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts | 3 +--
>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts     | 1 +
>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi    | 1 -
>>  3 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git
>> a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>> index 67c246ad8b8c..ec2ce894da1f 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>> @@ -17,8 +17,7 @@ / {
>>
>>  &gmac2io {
>>      phy-handle = <&yt8531c>;
>> -    tx_delay = <0x19>;
>> -    rx_delay = <0x05>;
>> +    phy-mode = "rgmii-id";
> 
> Shouldn't the "tx_delay" and "rx_delay" DT parameters be converted
> into the "tx-internal-delay-ps" and "rx-internal-delay-ps" parameters,
> respectively, so the Motorcomm PHY driver can pick them up and
> actually configure the internal PHY delays?

The documentation[1] says "{t,r}x-internal-delay-ps" default to 1950 and 
that value already works fine on my board.

1. 
https://www.kernel.org/doc/Documentation/devicetree/bindings/net/motorcomm%2Cyt8xxx.yaml

Thanks,
Tianling.

> 
>>      status = "okay";
>>
>>      mdio {
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>> index 324a8e951f7e..846b931e16d2 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>> @@ -15,6 +15,7 @@ / {
>>
>>  &gmac2io {
>>      phy-handle = <&rtl8211e>;
>> +    phy-mode = "rgmii";
>>      tx_delay = <0x24>;
>>      rx_delay = <0x18>;
>>      status = "okay";
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>> index 4f193704e5dc..09508e324a28 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>> @@ -109,7 +109,6 @@ &gmac2io {
>>      assigned-clocks = <&cru SCLK_MAC2IO>, <&cru SCLK_MAC2IO_EXT>;
>>      assigned-clock-parents = <&gmac_clk>, <&gmac_clk>;
>>      clock_in_out = "input";
>> -    phy-mode = "rgmii";
>>      phy-supply = <&vcc_io>;
>>      pinctrl-0 = <&rgmiim1_pins>;
>>      pinctrl-names = "default";

Re: [PATCH] arm64: dts: rockchip: change eth phy mode to rgmii-id for orangepi r1 plus lts

[Dragan Simic]

On 2025-01-19 12:15, Tianling Shen wrote:
> On 2025/1/19 17:54, Dragan Simic wrote:
>> Thanks for the patch.  Please, see a comment below.
>> 
>> On 2025-01-19 10:11, Tianling Shen wrote:
>>> In general the delay should be added by the PHY instead of the MAC,
>>> and this improves network stability on some boards which seem to
>>> need different delay.
>>> 
>>> Fixes: 387b3bbac5ea ("arm64: dts: rockchip: Add Xunlong OrangePi R1 
>>> Plus LTS")
>>> Cc: stable@vger.kernel.org # 6.6+
>>> Signed-off-by: Tianling Shen <cnsztl@gmail.com>
>>> ---
>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts | 3 +--
>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts     | 1 +
>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi    | 1 -
>>>  3 files changed, 2 insertions(+), 3 deletions(-)
>>> 
>>> diff --git
>>> a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>> index 67c246ad8b8c..ec2ce894da1f 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>> @@ -17,8 +17,7 @@ / {
>>> 
>>>  &gmac2io {
>>>      phy-handle = <&yt8531c>;
>>> -    tx_delay = <0x19>;
>>> -    rx_delay = <0x05>;
>>> +    phy-mode = "rgmii-id";
>> 
>> Shouldn't the "tx_delay" and "rx_delay" DT parameters be converted
>> into the "tx-internal-delay-ps" and "rx-internal-delay-ps" parameters,
>> respectively, so the Motorcomm PHY driver can pick them up and
>> actually configure the internal PHY delays?
> 
> The documentation[1] says "{t,r}x-internal-delay-ps" default to 1950
> and that value already works fine on my board.
> 
> 1. 
> https://www.kernel.org/doc/Documentation/devicetree/bindings/net/motorcomm%2Cyt8xxx.yaml

I see, but those values differ from the values found in the
"tx_delay" and "rx_delay" DT parameters, so I think this patch
should be tested with at least one more Orange Pi R1 Plus LTS
board, to make sure it's all still fine.

>> 
>>>      status = "okay";
>>> 
>>>      mdio {
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>> index 324a8e951f7e..846b931e16d2 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>> @@ -15,6 +15,7 @@ / {
>>> 
>>>  &gmac2io {
>>>      phy-handle = <&rtl8211e>;
>>> +    phy-mode = "rgmii";
>>>      tx_delay = <0x24>;
>>>      rx_delay = <0x18>;
>>>      status = "okay";
>>> diff --git 
>>> a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>> index 4f193704e5dc..09508e324a28 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>> @@ -109,7 +109,6 @@ &gmac2io {
>>>      assigned-clocks = <&cru SCLK_MAC2IO>, <&cru SCLK_MAC2IO_EXT>;
>>>      assigned-clock-parents = <&gmac_clk>, <&gmac_clk>;
>>>      clock_in_out = "input";
>>> -    phy-mode = "rgmii";
>>>      phy-supply = <&vcc_io>;
>>>      pinctrl-0 = <&rgmiim1_pins>;
>>>      pinctrl-names = "default";

Re: [PATCH] arm64: dts: rockchip: change eth phy mode to rgmii-id for orangepi r1 plus lts

[Tianling Shen]

On 2025/1/19 19:36, Dragan Simic wrote:
> On 2025-01-19 12:15, Tianling Shen wrote:
>> On 2025/1/19 17:54, Dragan Simic wrote:
>>> Thanks for the patch.  Please, see a comment below.
>>>
>>> On 2025-01-19 10:11, Tianling Shen wrote:
>>>> In general the delay should be added by the PHY instead of the MAC,
>>>> and this improves network stability on some boards which seem to
>>>> need different delay.
>>>>
>>>> Fixes: 387b3bbac5ea ("arm64: dts: rockchip: Add Xunlong OrangePi R1 
>>>> Plus LTS")
>>>> Cc: stable@vger.kernel.org # 6.6+
>>>> Signed-off-by: Tianling Shen <cnsztl@gmail.com>
>>>> ---
>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts | 3 +--
>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts     | 1 +
>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi    | 1 -
>>>>  3 files changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git
>>>> a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>>> index 67c246ad8b8c..ec2ce894da1f 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>>> @@ -17,8 +17,7 @@ / {
>>>>
>>>>  &gmac2io {
>>>>      phy-handle = <&yt8531c>;
>>>> -    tx_delay = <0x19>;
>>>> -    rx_delay = <0x05>;
>>>> +    phy-mode = "rgmii-id";
>>>
>>> Shouldn't the "tx_delay" and "rx_delay" DT parameters be converted
>>> into the "tx-internal-delay-ps" and "rx-internal-delay-ps" parameters,
>>> respectively, so the Motorcomm PHY driver can pick them up and
>>> actually configure the internal PHY delays?
>>
>> The documentation[1] says "{t,r}x-internal-delay-ps" default to 1950
>> and that value already works fine on my board.
>>
>> 1. https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ 
>> motorcomm%2Cyt8xxx.yaml
> 
> I see, but those values differ from the values found in the
> "tx_delay" and "rx_delay" DT parameters, so I think this patch
> should be tested with at least one more Orange Pi R1 Plus LTS
> board, to make sure it's all still fine.

This patch has been tested on 2 boards, and we will do more tests in 
next week.

Thanks,
Tianling.

> 
>>>
>>>>      status = "okay";
>>>>
>>>>      mdio {
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>>> index 324a8e951f7e..846b931e16d2 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>>> @@ -15,6 +15,7 @@ / {
>>>>
>>>>  &gmac2io {
>>>>      phy-handle = <&rtl8211e>;
>>>> +    phy-mode = "rgmii";
>>>>      tx_delay = <0x24>;
>>>>      rx_delay = <0x18>;
>>>>      status = "okay";
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>>> index 4f193704e5dc..09508e324a28 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>>> @@ -109,7 +109,6 @@ &gmac2io {
>>>>      assigned-clocks = <&cru SCLK_MAC2IO>, <&cru SCLK_MAC2IO_EXT>;
>>>>      assigned-clock-parents = <&gmac_clk>, <&gmac_clk>;
>>>>      clock_in_out = "input";
>>>> -    phy-mode = "rgmii";
>>>>      phy-supply = <&vcc_io>;
>>>>      pinctrl-0 = <&rgmiim1_pins>;
>>>>      pinctrl-names = "default";

Re: [PATCH] arm64: dts: rockchip: change eth phy mode to rgmii-id for orangepi r1 plus lts

[Andrew Lunn]

> >  &gmac2io {
> >  	phy-handle = <&yt8531c>;
> > -	tx_delay = <0x19>;
> > -	rx_delay = <0x05>;
> > +	phy-mode = "rgmii-id";
> 
> Shouldn't the "tx_delay" and "rx_delay" DT parameters be converted
> into the "tx-internal-delay-ps" and "rx-internal-delay-ps" parameters,
> respectively, so the Motorcomm PHY driver can pick them up and
> actually configure the internal PHY delays?

Maybe.

One problem is that tx_delay and rx_delay are really bad DT
bindings. They are basically values to be poked into a registers, with
no explanation of what they mean. Maybe 0x19 means 2ns? If so, that is
exactly what the PHY will do with rgmii-id, and you don't need any
additional properties.

The fact testing suggests this works does suggest these delay values
are around 2ns, so there is probably no need for
{rt}x-internal-delay-ps.

In general, i always suggest the PHY does the delay, just so that we
try to make all boards act in the same way. The fact this also removes
the use of these terrible tx_delay and rx_delay makes this patch even
better, if it can be shown to be reliable.

	Andrew

Re: [PATCH] arm64: dts: rockchip: change eth phy mode to rgmii-id for orangepi r1 plus lts

[Tianling Shen]

On 2025/1/19 23:48, Tianling Shen wrote:
> On 2025/1/19 19:36, Dragan Simic wrote:
>> On 2025-01-19 12:15, Tianling Shen wrote:
>>> On 2025/1/19 17:54, Dragan Simic wrote:
>>>> Thanks for the patch.  Please, see a comment below.
>>>>
>>>> On 2025-01-19 10:11, Tianling Shen wrote:
>>>>> In general the delay should be added by the PHY instead of the MAC,
>>>>> and this improves network stability on some boards which seem to
>>>>> need different delay.
>>>>>
>>>>> Fixes: 387b3bbac5ea ("arm64: dts: rockchip: Add Xunlong OrangePi R1 
>>>>> Plus LTS")
>>>>> Cc: stable@vger.kernel.org # 6.6+
>>>>> Signed-off-by: Tianling Shen <cnsztl@gmail.com>
>>>>> ---
>>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts | 3 +--
>>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts     | 1 +
>>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi    | 1 -
>>>>>  3 files changed, 2 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git
>>>>> a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>>>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>>>> index 67c246ad8b8c..ec2ce894da1f 100644
>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>>>> @@ -17,8 +17,7 @@ / {
>>>>>
>>>>>  &gmac2io {
>>>>>      phy-handle = <&yt8531c>;
>>>>> -    tx_delay = <0x19>;
>>>>> -    rx_delay = <0x05>;
>>>>> +    phy-mode = "rgmii-id";
>>>>
>>>> Shouldn't the "tx_delay" and "rx_delay" DT parameters be converted
>>>> into the "tx-internal-delay-ps" and "rx-internal-delay-ps" parameters,
>>>> respectively, so the Motorcomm PHY driver can pick them up and
>>>> actually configure the internal PHY delays?
>>>
>>> The documentation[1] says "{t,r}x-internal-delay-ps" default to 1950
>>> and that value already works fine on my board.
>>>
>>> 1. https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ 
>>> motorcomm%2Cyt8xxx.yaml
>>
>> I see, but those values differ from the values found in the
>> "tx_delay" and "rx_delay" DT parameters, so I think this patch
>> should be tested with at least one more Orange Pi R1 Plus LTS
>> board, to make sure it's all still fine.
> 
> This patch has been tested on 2 boards, and we will do more tests in 
> next week.
> 

Managed to test on another board and looks so far so good.
(Working network connection, no packet drop)

Thanks,
Tianling.

> Thanks,
> Tianling.
> 
>>
>>>>
>>>>>      status = "okay";
>>>>>
>>>>>      mdio {
>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>>>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>>>> index 324a8e951f7e..846b931e16d2 100644
>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>>>> @@ -15,6 +15,7 @@ / {
>>>>>
>>>>>  &gmac2io {
>>>>>      phy-handle = <&rtl8211e>;
>>>>> +    phy-mode = "rgmii";
>>>>>      tx_delay = <0x24>;
>>>>>      rx_delay = <0x18>;
>>>>>      status = "okay";
>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>>>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>>>> index 4f193704e5dc..09508e324a28 100644
>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>>>> @@ -109,7 +109,6 @@ &gmac2io {
>>>>>      assigned-clocks = <&cru SCLK_MAC2IO>, <&cru SCLK_MAC2IO_EXT>;
>>>>>      assigned-clock-parents = <&gmac_clk>, <&gmac_clk>;
>>>>>      clock_in_out = "input";
>>>>> -    phy-mode = "rgmii";
>>>>>      phy-supply = <&vcc_io>;
>>>>>      pinctrl-0 = <&rgmiim1_pins>;
>>>>>      pinctrl-names = "default";
> 

Re: [PATCH] arm64: dts: rockchip: change eth phy mode to rgmii-id for orangepi r1 plus lts

[Dragan Simic]

Hello Tianling,

On 2025-01-24 07:28, Tianling Shen wrote:
> On 2025/1/19 23:48, Tianling Shen wrote:
>> On 2025/1/19 19:36, Dragan Simic wrote:
>>> On 2025-01-19 12:15, Tianling Shen wrote:
>>>> On 2025/1/19 17:54, Dragan Simic wrote:
>>>>> Thanks for the patch.  Please, see a comment below.
>>>>> 
>>>>> On 2025-01-19 10:11, Tianling Shen wrote:
>>>>>> In general the delay should be added by the PHY instead of the 
>>>>>> MAC,
>>>>>> and this improves network stability on some boards which seem to
>>>>>> need different delay.
>>>>>> 
>>>>>> Fixes: 387b3bbac5ea ("arm64: dts: rockchip: Add Xunlong OrangePi 
>>>>>> R1 Plus LTS")
>>>>>> Cc: stable@vger.kernel.org # 6.6+
>>>>>> Signed-off-by: Tianling Shen <cnsztl@gmail.com>
>>>>>> ---
>>>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts | 3 
>>>>>> +--
>>>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts     | 1 
>>>>>> +
>>>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi    | 1 
>>>>>> -
>>>>>>  3 files changed, 2 insertions(+), 3 deletions(-)
>>>>>> 
>>>>>> diff --git
>>>>>> a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>>>>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>>>>> index 67c246ad8b8c..ec2ce894da1f 100644
>>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>>>>>> @@ -17,8 +17,7 @@ / {
>>>>>> 
>>>>>>  &gmac2io {
>>>>>>      phy-handle = <&yt8531c>;
>>>>>> -    tx_delay = <0x19>;
>>>>>> -    rx_delay = <0x05>;
>>>>>> +    phy-mode = "rgmii-id";
>>>>> 
>>>>> Shouldn't the "tx_delay" and "rx_delay" DT parameters be converted
>>>>> into the "tx-internal-delay-ps" and "rx-internal-delay-ps" 
>>>>> parameters,
>>>>> respectively, so the Motorcomm PHY driver can pick them up and
>>>>> actually configure the internal PHY delays?
>>>> 
>>>> The documentation[1] says "{t,r}x-internal-delay-ps" default to 1950
>>>> and that value already works fine on my board.
>>>> 
>>>> 1. https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ 
>>>> motorcomm%2Cyt8xxx.yaml
>>> 
>>> I see, but those values differ from the values found in the
>>> "tx_delay" and "rx_delay" DT parameters, so I think this patch
>>> should be tested with at least one more Orange Pi R1 Plus LTS
>>> board, to make sure it's all still fine.
>> 
>> This patch has been tested on 2 boards, and we will do more tests in 
>> next week.
>> 
> 
> Managed to test on another board and looks so far so good.
> (Working network connection, no packet drop)

Sounds good to me, thanks for the additional testing.

>>>>>>      status = "okay";
>>>>>> 
>>>>>>      mdio {
>>>>>> diff --git 
>>>>>> a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>>>>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>>>>> index 324a8e951f7e..846b931e16d2 100644
>>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts
>>>>>> @@ -15,6 +15,7 @@ / {
>>>>>> 
>>>>>>  &gmac2io {
>>>>>>      phy-handle = <&rtl8211e>;
>>>>>> +    phy-mode = "rgmii";
>>>>>>      tx_delay = <0x24>;
>>>>>>      rx_delay = <0x18>;
>>>>>>      status = "okay";
>>>>>> diff --git 
>>>>>> a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>>>>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>>>>> index 4f193704e5dc..09508e324a28 100644
>>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
>>>>>> @@ -109,7 +109,6 @@ &gmac2io {
>>>>>>      assigned-clocks = <&cru SCLK_MAC2IO>, <&cru SCLK_MAC2IO_EXT>;
>>>>>>      assigned-clock-parents = <&gmac_clk>, <&gmac_clk>;
>>>>>>      clock_in_out = "input";
>>>>>> -    phy-mode = "rgmii";
>>>>>>      phy-supply = <&vcc_io>;
>>>>>>      pinctrl-0 = <&rgmiim1_pins>;
>>>>>>      pinctrl-names = "default";

Re: [PATCH] arm64: dts: rockchip: change eth phy mode to rgmii-id for orangepi r1 plus lts

[Heiko Stuebner]

Hey,

Am Freitag, 24. Januar 2025, 07:35:50 MEZ schrieb Dragan Simic:
> Hello Tianling,
> 
> On 2025-01-24 07:28, Tianling Shen wrote:
> > On 2025/1/19 23:48, Tianling Shen wrote:
> >> On 2025/1/19 19:36, Dragan Simic wrote:
> >>> On 2025-01-19 12:15, Tianling Shen wrote:
> >>>> On 2025/1/19 17:54, Dragan Simic wrote:
> >>>>> Thanks for the patch.  Please, see a comment below.
> >>>>> 
> >>>>> On 2025-01-19 10:11, Tianling Shen wrote:
> >>>>>> In general the delay should be added by the PHY instead of the 
> >>>>>> MAC,
> >>>>>> and this improves network stability on some boards which seem to
> >>>>>> need different delay.
> >>>>>> 
> >>>>>> Fixes: 387b3bbac5ea ("arm64: dts: rockchip: Add Xunlong OrangePi 
> >>>>>> R1 Plus LTS")
> >>>>>> Cc: stable@vger.kernel.org # 6.6+
> >>>>>> Signed-off-by: Tianling Shen <cnsztl@gmail.com>
> >>>>>> ---
> >>>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts | 3 
> >>>>>> +--
> >>>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts     | 1 
> >>>>>> +
> >>>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi    | 1 
> >>>>>> -
> >>>>>>  3 files changed, 2 insertions(+), 3 deletions(-)
> >>>>>> 
> >>>>>> diff --git
> >>>>>> a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
> >>>>>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
> >>>>>> index 67c246ad8b8c..ec2ce894da1f 100644
> >>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
> >>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
> >>>>>> @@ -17,8 +17,7 @@ / {
> >>>>>> 
> >>>>>>  &gmac2io {
> >>>>>>      phy-handle = <&yt8531c>;
> >>>>>> -    tx_delay = <0x19>;
> >>>>>> -    rx_delay = <0x05>;
> >>>>>> +    phy-mode = "rgmii-id";
> >>>>> 
> >>>>> Shouldn't the "tx_delay" and "rx_delay" DT parameters be converted
> >>>>> into the "tx-internal-delay-ps" and "rx-internal-delay-ps" 
> >>>>> parameters,
> >>>>> respectively, so the Motorcomm PHY driver can pick them up and
> >>>>> actually configure the internal PHY delays?
> >>>> 
> >>>> The documentation[1] says "{t,r}x-internal-delay-ps" default to 1950
> >>>> and that value already works fine on my board.
> >>>> 
> >>>> 1. https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ 
> >>>> motorcomm%2Cyt8xxx.yaml
> >>> 
> >>> I see, but those values differ from the values found in the
> >>> "tx_delay" and "rx_delay" DT parameters, so I think this patch
> >>> should be tested with at least one more Orange Pi R1 Plus LTS
> >>> board, to make sure it's all still fine.
> >> 
> >> This patch has been tested on 2 boards, and we will do more tests in 
> >> next week.
> >> 
> > 
> > Managed to test on another board and looks so far so good.
> > (Working network connection, no packet drop)
> 
> Sounds good to me, thanks for the additional testing.

I assume that means there are no more open issues, right?
At least I got that impression from glancing at the thread :-)

Heiko

Re: [PATCH] arm64: dts: rockchip: change eth phy mode to rgmii-id for orangepi r1 plus lts

[Dragan Simic]

Hello Heiko,

On 2025-01-31 10:01, Heiko Stuebner wrote:
> Am Freitag, 24. Januar 2025, 07:35:50 MEZ schrieb Dragan Simic:
>> On 2025-01-24 07:28, Tianling Shen wrote:
>> > On 2025/1/19 23:48, Tianling Shen wrote:
>> >> On 2025/1/19 19:36, Dragan Simic wrote:
>> >>> On 2025-01-19 12:15, Tianling Shen wrote:
>> >>>> On 2025/1/19 17:54, Dragan Simic wrote:
>> >>>>> Thanks for the patch.  Please, see a comment below.
>> >>>>>
>> >>>>> On 2025-01-19 10:11, Tianling Shen wrote:
>> >>>>>> In general the delay should be added by the PHY instead of the
>> >>>>>> MAC,
>> >>>>>> and this improves network stability on some boards which seem to
>> >>>>>> need different delay.
>> >>>>>>
>> >>>>>> Fixes: 387b3bbac5ea ("arm64: dts: rockchip: Add Xunlong OrangePi
>> >>>>>> R1 Plus LTS")
>> >>>>>> Cc: stable@vger.kernel.org # 6.6+
>> >>>>>> Signed-off-by: Tianling Shen <cnsztl@gmail.com>
>> >>>>>> ---
>> >>>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts | 3
>> >>>>>> +--
>> >>>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dts     | 1
>> >>>>>> +
>> >>>>>>  arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi    | 1
>> >>>>>> -
>> >>>>>>  3 files changed, 2 insertions(+), 3 deletions(-)
>> >>>>>>
>> >>>>>> diff --git
>> >>>>>> a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>> >>>>>> b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>> >>>>>> index 67c246ad8b8c..ec2ce894da1f 100644
>> >>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>> >>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus-lts.dts
>> >>>>>> @@ -17,8 +17,7 @@ / {
>> >>>>>>
>> >>>>>>  &gmac2io {
>> >>>>>>      phy-handle = <&yt8531c>;
>> >>>>>> -    tx_delay = <0x19>;
>> >>>>>> -    rx_delay = <0x05>;
>> >>>>>> +    phy-mode = "rgmii-id";
>> >>>>>
>> >>>>> Shouldn't the "tx_delay" and "rx_delay" DT parameters be converted
>> >>>>> into the "tx-internal-delay-ps" and "rx-internal-delay-ps"
>> >>>>> parameters,
>> >>>>> respectively, so the Motorcomm PHY driver can pick them up and
>> >>>>> actually configure the internal PHY delays?
>> >>>>
>> >>>> The documentation[1] says "{t,r}x-internal-delay-ps" default to 1950
>> >>>> and that value already works fine on my board.
>> >>>>
>> >>>> 1. https://www.kernel.org/doc/Documentation/devicetree/bindings/net/
>> >>>> motorcomm%2Cyt8xxx.yaml
>> >>>
>> >>> I see, but those values differ from the values found in the
>> >>> "tx_delay" and "rx_delay" DT parameters, so I think this patch
>> >>> should be tested with at least one more Orange Pi R1 Plus LTS
>> >>> board, to make sure it's all still fine.
>> >>
>> >> This patch has been tested on 2 boards, and we will do more tests in
>> >> next week.
>> >
>> > Managed to test on another board and looks so far so good.
>> > (Working network connection, no packet drop)
>> 
>> Sounds good to me, thanks for the additional testing.
> 
> I assume that means there are no more open issues, right?
> At least I got that impression from glancing at the thread :-)

Yes, Tianling performed the additional testing, and the
introduced changes worked fine on three boards in total.
To me, that sounds good. :)

Re: [PATCH] arm64: dts: rockchip: change eth phy mode to rgmii-id for orangepi r1 plus lts

[Heiko Stuebner]

On Sun, 19 Jan 2025 17:11:54 +0800, Tianling Shen wrote:
> In general the delay should be added by the PHY instead of the MAC,
> and this improves network stability on some boards which seem to
> need different delay.
> 
> 

Applied, thanks!

[1/1] arm64: dts: rockchip: change eth phy mode to rgmii-id for orangepi r1 plus lts
      commit: a6a7cba17c544fb95d5a29ab9d9ed4503029cb29

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>