[PATCH v3] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Marek Vasut]

On 12/8/22 22:00, Xavier Drudis Ferran wrote:
> El Thu, Dec 08, 2022 at 09:12:08PM +0100, Marek Vasut deia:
>> On 12/8/22 17:53, Xavier Drudis Ferran wrote:
>>
>> [...]
>>
>>> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
>>> @@ -7,7 +7,7 @@
>>>     */
>>>    #include <common.h>
>>> -#include <clk.h>
>>> +#include <clk-uclass.h>
>>>    #include <dm.h>
>>>    #include <asm/global_data.h>
>>>    #include <dm/device_compat.h>
>>> @@ -168,6 +168,9 @@ static struct phy_ops rockchip_usb2phy_ops = {
>>>    	.of_xlate = rockchip_usb2phy_of_xlate,
>>>    };
>>> +static struct clk_ops rockchip_usb2phy_clk_ops = {
>>> +};
>>
>> Is this empty structure needed ? Why ?
>>
>> Either it shouldn't be here, or it should implement some callbacks, like
>> clock enable/disable ?
>>
> 
> I tried without it but it gave me a runtime error.  I think I have the
> log somewhere if you want to see it.  It looked like a null pointer
> dereference at first sight.  I just added it and it got fixed. I
> didn't research what the failing functions were trying to do.
> 
> Or is this a common case and there's some null_clk_ops() function or
> macro magic or something somewhere ?

It seems Linux kernel drivers/phy/rockchip/phy-rockchip-inno-usb2.c does 
implement clock operations for this 480 MHz clock .

> The thing is nobody is using this clk in u-boot. It's just its phandle
> in a clock phandle list that ehci-generic.c happens to use in bulk. So
> the default implementation seems to be enough to allocate, enable and
> release it in bulk with its clk set. Or at least to call the functions
> without error.
> 
> As I have left it, it might not work if ever someone wants to use it.
> But should I try to implement it so that it is usable ? How should I
> test it ?  Shouldn't we wait until someone has some real use for it ?
> Linux spent sometime without it in the phandle list until William wu
> discovered it was needed for suspend/resume, so u-boot may never need
> it.
> 
> Or should I add a comment in the code ?

I would say, model the clock tree the same way Linux does model it.
I know little about this hardware though.