[PATCH v4 2/2] arm: rk3399: usb2phy: phy-rockchip-inno-usb2.c: Implement operations for the 480MHz usb2phy clock in rk3399.

[Xavier Drudis Ferran]

El Sun, Dec 11, 2022 at 06:20:41AM +0100, Marek Vasut deia:
> On 12/9/22 16:47, Xavier Drudis Ferran wrote:
> > This clock has no users but appears in a phandle list used by
> > ehci-generic.c to bulk enable it. The phandle list comes from linux,
> > where it is needed for suspend/resume to work [1].
> > 
> > My tests give the same results with or without this patch, but Marek
> > Vasut found it weird to declare an empty clk_ops[2].
> > 
> > So I adapted the code from linux 6.1-rc8 so that it hopefully works
> > if it ever has some user. For now, without real users, it seems to
> > at least not give any errors.
> 
> You might want to squash 1/2 and 2/2 together, since it's one change (add
> clock operations to a phy driver).
>

I prefer to send them separate, but I don't mind if they are squashed
when applied. In my mind it is better if the second patch can be
reverted easily if it ever is found to give any trouble (or just
increase code size). But I'm not the most knowledgeable person in
this list by a huge margin, so if mantainers apply it squashed I'll be
happy with my 2 USB2 ports working.

In fact I don't mind if u-boot takes v4, v3, v2 or v1 of this, or any
other solution, I just want 2 USB2 ports in Rock Pi 4B to be usable in
u-boot.  As it is now only the USB3 ports work in u-boot, unless one
enables CONFIG_OCHI_GENERIC, and then the USB2 ports would work slowly.

> Since 1/2 works without any clock operations, who does enable these usb
> clock on this SoC ? Is there any driver or platform code for that ? If so,
> you can drop that platform code with this driver-side implementation in
> place (which is nice).
>

I'm sorry if I'm not being clear.

I can't drop that code, it needs to enable 2 clocks, but it is
enabling 3.  I can remove one clock from the list of enabled clocks (I
did so in v1) or I can leave it and ignore the error (I did so in
v2). But since that didn't seem to please the list, I tried v3 and v4.
Maybe I just added noise and should have waited further opinions.

ehci-generic.c bulk enables all clocks in the phandle list assigned to the
usb host (usb_host_0ehci).
The arch/arm/dts/rk3399.dtsi provides such a list.

But the last clock is the usb phy &u2phy0. The phy is otherwise
enabled, and the output clock associated with it seems not to serve
any purpose, since u-boot works when I remove &u2phy0 from the clock
phandle list (v1), and also when I leave it there but ignore the error
for the missing clock (v2). In master, v1 and v2 &u2phy0 is
UCLASS_PHY, but not UCLASS_CLK.

With version v3 the &u2phy0 is bound as UCLASS_CLK too, so the bulk
enable from ehci-generic succeeds, although it does nothing to the
hardware (the clock driver having no operations). This does not have
any bad effect as far as I can see. The simple fact that ehci-generic
doesn't fail makes the 2 USB2 ports usable in u-boot.

With v4 the &u2phy0 is bound as UCLASS_CLK, it is bulk enabled, and
that ends up calling an operation that does touch the hardware. This
still doesn't have any bad effect, but I can't tell that it works, because
it does the same as in v3 or v2 or v1.

So when I say there are no users, I don't mean the code isn't called.
It is, but the behaviour I can see is the same when it is and when it
isn't, so whatever the enabled clock does, it does not seem that
u-boot is using it.

Why is it in the phandle list then ? Because it apparently helps linux
resume or suspend. When the output clock is not enabled, something
seems to block linux resume. But I'm not testing linux resume, and it
doesn't matter, because in linux the output clock is enabled anyway,
because &u2phy0 provides a clk, and is in the clock phandle list of
usb_host0_ehci).

Since u-boot and linux share rk3399.dtsi the &u2phy0 is in the phandle list
in u-boot too, but since in master it isn't UCLASS_CLK, ehci-generic fails
and USB2 is not available in that port. 

Maybe there is some use for the clock in u-boot that I'm not seeing because
I don't test good enough or I don't know enough about USB, or lack USB testing
equipment, or something. 

> btw I am still hoping some of the rockchip people will have a look at this
> series.

Yes, I'm looking forward to it too. So far Kever Yang reacted in late August
to v1 by proposing v2 (as far as I understand).

Thanks for listening to my walls of text.

--
Xavi