[PATCH] arm64: dts: rockchip: rock-3a: make ethernet work

Sat Aug 5 14:40:46 PDT 2023

On 2023-08-05 18:22, Richard Kojedzinszky wrote:
> 2023-07-24 20:48 időpontban Heiko Stuebner ezt írta:
>> Am Montag, 24. Juli 2023, 07:09:48 CEST schrieb FUKAUMI Naoki:
>>> hi,
> 
> Hi
> 
>>>
>>> On 7/24/23 07:54, Jonas Karlman wrote:
>>>> On 2023-07-17 19:11, Jonas Karlman wrote:
>>>>> On 2023-07-17 09:40, Michael Riesch wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> In addition to what has been already said:
>>>>>>
>>>>>> On 7/16/23 15:50, Jonas Karlman wrote:
>>>>>>> On 2023-07-15 06:49, FUKAUMI Naoki wrote:
>>>>>>>> hi,
>>>>>>>>
>>>>>>>> On 7/15/23 01:24, Jonas Karlman wrote:
>>>>>>>>> On 2023-07-14 17:46, Heiko Stuebner wrote:
>>>>>>>>>> Am Freitag, 14. Juli 2023, 08:30:27 CEST schrieb FUKAUMI Naoki:
>>>>>>>>>>> ethernet on Radxa ROCK 3A is not working by following error:
>>>>>>>>>>>
>>>>>>>>>>>    rk_gmac-dwmac fe010000.ethernet eth0: no phy at addr -1
>>>>>>>>>>>    rk_gmac-dwmac fe010000.ethernet eth0: stmmac_open: Cannot attach to PHY (error: -19)
>>>>>>>>>>>
>>>>>>>>>>> to fix this problem, align related properties with vendor kernel
>>>>>>>>>>>    https://github.com/radxa/kernel/blob/linux-5.10-gen-rkr4.1/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
>>>>>>>>>>>
>>>>>>>>>>> Fixes: 22a442e6586c ("arm64: dts: rockchip: add basic dts for the radxa rock3 model a")
>>>>>>>>>>> Signed-off-by: FUKAUMI Naoki <naoki at radxa.com>
>>>>>>>>>>
>>>>>>>>>> There also is a second patch in the mix adding the gmac1_clkin
>>>>>>>>>> ef9f4b4a5020 ("arm64: dts: rockchip: Add support of external clock to ethernet node on Rock 3A SBC")
>>>>>>>>>>
>>>>>>>>>> And this patch does the exact opposite as the original nodes.
>>>>>>>>>> Can someone please mention board versions? Or did the gmac1 never
>>>>>>>>>> really work in the first place?
>>>>>>
>>>>>> As far as I know all schematics versions state that the PHY produces the
>>>>>> clock (see the most recent schematics here:
>>>>>> https://dl.radxa.com/rock3/docs/hw/3a/ROCK-3A-V1.3-SCH.pdf
>>>>>>
>>>>>> Although using the internal MAC clock works as well, using the
>>>>>> gmac1_clkin is most probably the correct way.
>>>>>>
>>>>>>>>> Ethernet have worked and probably still works booting with vendor U-Boot.
>>>>>>>>>
>>>>>>>>> However, when booting using mainline U-Boot the ethernet PHY is never
>>>>>>>>> initialized or reset due to lack of a ethernet gmac driver for rk35xx.
>>>>>>>>
>>>>>>>> surely I'm using mainline u-boot.
>>>>>>
>>>>>> Ah, this might explain why I never experienced this issue: I am using
>>>>>> mainline barebox with GMAC support.
>>>>>>
>>>>>>>>> With an early work-in-progress gmac driver the ethernet PHY is working
>>>>>>>>> same as with vendor U-Boot, and the ethernet PHY is identified.
>>>>>>>>>
>>>>>>>>> This revert from using reset-gpios to using the deprecated
>>>>>>>>> snps,reset-gpio is probably the wrong way forward.
>>>>>>>>>
>>>>>>>>> I suspect there is a bug in net/phy/mdio_device.c on how reset-gpios
>>>>>>>>> is asserted/deasserted, it works differently than how I would expect it
>>>>>>>>> to work, and also differs in how U-Boot handles reset-gpios.
>>>>>>>>>
>>>>>>>>> Some earlier findings regarding this reset issue:
>>>>>>>>> https://github.com/Kwiboo/linux-rockchip/compare/rk3568-eth-phy-reset~2...rk3568-eth-phy-reset
>>>>>>>>>
>>>>>>>>> Will try to get a proper patch/rfc out later this weekend or early next
>>>>>>>>> week after re-testing that on latest kernel.
>>>>>>>>
>>>>>>>> thank you so much for your awesome work!
>>>>>>>
>>>>>>> Thanks, made some progress on tracking down the root cause.
>>>>>>>  From what I have discovered so far there is a chicken-and-egg problem:
>>>>>>>
>>>>>>> - phy needs to be reset or phy_id read back as 0xffffffff on mdio bus
>>>>>>> - phy device is not created because a valid phy_id is not read back
>>>>>>> - phy device needs to be created before it can be reset
>>>>>>>
>>>>>>> Possible workarounds so far:
>>>>>>> - phy is reset in U-Boot
>>>>>>>    - very early work-in-progress at https://github.com/Kwiboo/u-boot-rockchip/commit/6ee80f9a895a32551cf30dd4252a4960ed80dfc9
>>>>>>> - phy is reset using mdio bus reset
>>>>>>>    - similar to old https://github.com/Kwiboo/linux-rockchip/commit/8597fcfa0c5c792dabb44a2db7b283c56c99ec6a
>>>>>>> - phy is reset using deprecated snps,reset-gpio
>>>>>>>    - similar to mdio bus reset
>>>>>>
>>>>>> There was a similar discussion here:
>>>>>> https://lore.kernel.org/all/CAMdYzYo_DGiO0UxJEb3xues7Um=X9AgPvz+Xp_YWb9pp9HaScg@mail.gmail.com/
>>>>>>
>>>>>> The approach that moves the reset to the MDIO bus has been mentioned
>>>>>> there as well. On the first glance this approach looks reasonable to me.
>>>>>
>>>>> It does not look like U-Boot support mdio bus reset-gpios, so changing
>>>>> to that would require adding even more code into U-Boot to get ethernet
>>>>> working in U-Boot.
>>>>>
>>>>> Moving to mdio bus would make it behave like with old snps,reset-gpio,
>>>>> however phy reset-gpios still describe the hw more accurately, a
>>>>> assert/deassert cycle of reset-gpios triggers a phy hard reset.
>>>>>
>>>>> Do not know if it is possible to have both mdio bus reset and phy reset.
>>>>>
>>>>> There also looks like there is a bug in dwmac-rk rk3568_set_to_rgmii,
>>>>> tx/rx delay is always enabled. It should be disabled in some phy modes.
>>>>> Can send a patch once I finish with the U-Boot ethernet driver.
>>>>>
>>>>> Will try to complete a U-Boot driver that supports both phy reset-gpios
>>>>> and the deprecated snps,reset-gpio as a first step.
>>>>
>>>> I have now created a small U-Boot dummy ethernet phy reset driver that
>>>> will assert/deassert reset-gpios to help make linux detect the PHY.
>>>>
>>>> With this my ROCK 3 Model B detects the RTL8211F PHY again without any
>>>> changes to linux device tree.
>>>>
>>>> See the U-Boot patch "HACK: net: Add dummy PHY reset-gpios driver"
>>>> at https://github.com/Kwiboo/u-boot-rockchip/commits/rk3568-2023.10-hacks
>>>
>>> your hack works fine on my ROCK 3A with mainline kernel.
>>
>> very nice. Jonas, thanks for working on that.
>>
>> So I'll disregard this dt patch.
>>
> 
> While this sounds good to me also, that we have a solution, would it not 
> be linux kernels sole task to initialize devices on its own? I mean, why 
> is this treated as an u-boot bug? This way the linux kernel will depend 
> on specific u-boot version, and perhaps specific u-boot features, 
> compile time options. I assume there are many other devices which u-boot 
> does not support, and linux can initialize it well.

I fully agree that this should also be fixed in linux, but not by
reverting to use deprecated DT properties as done in this patch.

Big progress has also been made on the U-Boot front and a GMAC driver
for RK356x and RK3588 will be posted any day now.

See https://github.com/Kwiboo/u-boot-rockchip/commits/rk3568-2023.10-gmac
for a fully working work-in-progress state of such series. There are
some dependencies to sort out first, like porting the IO-domain driver.

> 
> So, would not be there a way to fix this only in linux kernel?

It should be possible, not something I will be looking into.

Doing a proper gpio assert/deassert cycle before trying to read back the
phy_id should fix this issue and allow the PHY to be registered.

As I mentioned earlier there is a chicken-and-egg problem to be solved:
- phy needs to be reset or phy_id read back as 0xffffffff on mdio bus
- phy device is not created because a valid phy_id is not read back
- phy device needs to be created before it can be reset

A reset need to happen before/during this call chain:
stmmac_mdio_register()
-> of_mdiobus_register()
-> of_mdiobus_register_phy()
-> fwnode_mdiobus_register_phy()
-> get_phy_device()
-> get_phy_c22_id()
-> mdiobus_read(bus, addr, MII_PHYSID1)

Here mdiobus_read cannot read back the phy_id unless the PHY has been reset.

This is why the deprecated props work, the reset is issued before/during
stmmac_mdio_register(). Or at the mdiobus level, the reset is issued
before/during of_mdiobus_register(). With reset at phy level the reset
happens after device is created, after phy_id has been read, hence a
chicken-and-egg problem.

The following commit was able at least reset the PHY correctly some
linux versions ago. This worked because stmmac_init_phy() would somehow
end up calling mdio_device_reset() and a PHY could be probed after that.

https://github.com/Kwiboo/linux-rockchip/commit/c338ed260bfd87277c41aa0290f1f2aad8d629b1

I think the commit 8fbc10b995a5 ("net: stmmac: check fwnode for phy
device before scanning for phy") changed this behavior and now the phy
must be registered during the above call chain to later be picked up
by in the stmmac_init_phy() call.

Hope this can help if someone wants to dig more into this issue and
work on a proper fix for the linux side.

Regards,
Jonas

> 
> Thanks, regards,
> Richard
> 
>>
>> Heiko
>>
>>