Re: [PATCH 2/2] arm64: dts: rockchip: rock-pi-e: fix location of snps properties

[Jonas Karlman]

On 2024-01-17 09:15, Jonas Karlman wrote:
> On 2024-01-17 06:47, Trevor Woerner wrote:
>> On Wed 2024-01-17 @ 12:38:39 PM, Chen-Yu Tsai wrote:
>>> On Wed, Jan 17, 2024 at 4:41 AM Trevor Woerner <twoerner@xxxxxxxxx> wrote:
>>>>
>>>> A number of snps (Synopsys) properties are not in their correct location.
>>>
>>> Nope. If you read the snps,dwmac.yaml binding file, you'll see that these
>>> properties have been deprecated. They are properties pertaining to the PHY
>>> and should be described under the PHY node. Support for reset GPIOs on PHY
>>> devices in phylib has been there since v4.16. The snps prefixed properties
>>> were deprecated in v5.3.
>>
>> If that's the case, then the bindings and the drivers are out of sync in both
>> U-Boot and Linux. I discovered this issue while working with older and newer
>> revisions of the rock-pi-e board. The first three spins of the rock-pi-e have
>> the rtl8211e PHY but the last one (v1.21) has the rtl8211f PHY.
>>
>> With the existing layout nothing works in U-Boot and in Linux the rtl8211e
>> works but not the rtl8211f. With this patch both the rtl8211e and the rtl8211f
>> PHYs work using the exact same device trees on both older and newer rock-pi-e
>> boards in both U-Boot and Linux.
> 
> For linux this is probably related to the same chicken-and-egg reset
> issue outlined at [1]. The phy is not reset before it is probed and
> cannot be probed because it is not reset.
> 
> As for U-Boot the designware/gmac_rockchip ethernet driver may need some
> adjustments to properly integrate with eth-phy uclass to properly reset
> the phy described in a ethernet-phy node before it can be probed.
> 
> Following config options is disabled for rock-pi-e-rk3328 defconfig:
> 
> # CONFIG_DM_ETH_PHY is not set
> # CONFIG_PHY_REALTEK is not set
> 
> Also the driver needs to be updated to make use eth-phy uclass for it to
> work correctly. More similar to how the dwc_eth_qos driver works related
> to ethernet phy.

I have just sent out a U-Boot series that fix ethernet on the v1.21
revision of the ROCK Pi E board, see [2].

Would expect that ethernet also starts to work in linux with those
patches applied to U-Boot. I only tested and verified ethernet in U-Boot.

Hopefully someone can take a closer look at the issue on linux side
so that it does not need to depend on PHY being reset by the bootloader.

[2] https://lore.kernel.org/u-boot/20240118071949.927089-1-jonas@xxxxxxxxx/T/

Regards,
Jonas

> 
> [1] https://lore.kernel.org/linux-rockchip/47d55aca-bee6-810f-379f-9431649fefa6@xxxxxxxxx/
> 
> Regards,
> Jonas
> 
>>
>> Comparing the rock-pi-e's dts file with the one from the roc-rk3328-cc board,
>> which also uses the layout in this patch, and which also uses the rtl8211e
>> external PHY, is what led me in this direction.
>>
>>>
>>> ChenYu
>>>
>>>> Fixes: b918e81f2145 ("arm64: dts: rockchip: rk3328: Add Radxa ROCK Pi E")
>>>> Signed-off-by: Trevor Woerner <twoerner@xxxxxxxxx>
>>>> ---
>>>>  arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts | 10 +++++-----
>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
>>>> index 096cfa19036e..0739b8fec86e 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
>>>> @@ -150,8 +150,11 @@ &gmac2io {
>>>>         phy-mode = "rgmii";
>>>>         phy-supply = <&vcc_io>;
>>>>         pinctrl-names = "default";
>>>> -       pinctrl-0 = <&rgmiim1_pins>;
>>>> +       pinctrl-0 = <&rgmiim1_pins>, <&eth_phy_reset_pin>;
>>>>         snps,aal;
>>>> +       snps,reset-gpio = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>;
>>>> +       snps,reset-active-low;
>>>> +       snps,reset-delays-us = <0 10000 50000>;
>>>>         snps,rxpbl = <0x4>;
>>>>         snps,txpbl = <0x4>;
>>>>         tx_delay = <0x26>;
>>>> @@ -165,13 +168,10 @@ mdio {
>>>>
>>>>                 rtl8211: ethernet-phy@1 {
>>>>                         reg = <1>;
>>>> -                       pinctrl-0 = <&eth_phy_int_pin>, <&eth_phy_reset_pin>;
>>>> +                       pinctrl-0 = <&eth_phy_int_pin>;
>>>>                         pinctrl-names = "default";
>>>>                         interrupt-parent = <&gpio1>;
>>>>                         interrupts = <24 IRQ_TYPE_LEVEL_LOW>;
>>>> -                       reset-assert-us = <10000>;
>>>> -                       reset-deassert-us = <50000>;
>>>> -                       reset-gpios = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>;
>>>>                 };
>>>>         };
>>>>  };
>>>> --
>>>> 2.43.0.76.g1a87c842ece3
>>>>