Re: [PATCH 01/18] arm64: dts: renesas: beacon kit: Configure programmable clocks

[Adam Ford]

On Tue, Dec 22, 2020 at 2:03 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Adam,
>
> On Tue, Dec 22, 2020 at 2:39 AM Adam Ford <aford173@xxxxxxxxx> wrote:
> > On Fri, Dec 18, 2020 at 7:16 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > On Thu, Dec 17, 2020 at 12:52 PM Adam Ford <aford173@xxxxxxxxx> wrote:
> > > > On Thu, Dec 17, 2020 at 2:16 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > > > On Wed, Dec 16, 2020 at 6:03 PM Adam Ford <aford173@xxxxxxxxx> wrote:
> > > > > > On Wed, Dec 16, 2020 at 8:55 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > > > > > On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@xxxxxxxxx> wrote:
> > > > > > > > When the board was added, clock drivers were being updated done at
> > > > > > > > the same time to allow the versaclock driver to properly configure
> > > > > > > > the modes.  Unforutnately, the updates were not applied to the board
> > > > >
> > > > > > > > --- a/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi
> > > > > > > > +++ b/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi
> > > > > > > > @@ -5,6 +5,7 @@
> > > > > > > >
> > > > > > > >  #include <dt-bindings/gpio/gpio.h>
> > > > > > > >  #include <dt-bindings/input/input.h>
> > > > > > > > +#include <dt-bindings/clk/versaclock.h>
> > > > > > > >
> > > > > > > >  / {
> > > > > > > >         backlight_lvds: backlight-lvds {
> > > > > > > > @@ -294,12 +295,12 @@ &du_out_rgb {
> > > > > > > >  &ehci0 {
> > > > > > > >         dr_mode = "otg";
> > > > > > > >         status = "okay";
> > > > > > > > -       clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>;
> > > > > > > > +       clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>, <&versaclock5 3>;
> > > > > > >
> > > > > > > Why this change? You said before you don't need this
> > > > > > > https://lore.kernel.org/linux-renesas-soc/CAHCN7xJWbP16SA-Ok-5syNnqOZAt8OFJo2_rtg5VrNVsN2-eiQ@xxxxxxxxxxxxxx/
> > > > > > >
> > > > > >
> > > > > > I had talked with the hardware guys about buy pre-programmed
> > > > > > versaclock chips which would have been pre-configured and pre-enabled.
> > > > > > I thought it was going to happen, but it didn't, so we need the
> > > > > > versaclock driver to enable the reference clock for the USB
> > > > > > controllers, ethernet controller and audio clocks.  Previously we were
> > > > > > manually configuring it or it was coincidentally working. Ideally,
> > > > > > we'd have the clock system intentionally enable/disable the clocks
> > > > > > when drivers are loaded/unloaded for for power management reasons.
> > > > >
> > > > > Can you tell me how exactly the Versaclock outputs are wired?
> > > >
> > > > The SoC is expecting a fixed external 50 MHz clock connected to
> > > > USB_EXTAL.  Instead of a fixed clock, we're using the Versaclock.
> > > > We're also using the Versaclock to drive the AVB TXCRefClk,
> > > > du_dotclkiun0 and du_dotclkin2 (also also called du_dotclkin3 on
> > > > RZ/G2N) instead of fixed clocks.
> > > >
> > > > > E.g. for USB, the bindings don't say anything about a third clock input,
> > > > > so I'd like to know where that clock is fed into USB.
> > > >
> > > > The way the driver is crafted, it can take in multiple clocks and it
> > > > goes through a list to enable them all, so I added the versaclock to
> > > > the array.  Without the versaclock reference, the clock doesn't get
> > > > turned on and the USB fails to operate.
> > >
> > > According to the Hardware User's Manual, USBL_EXTAL is used for USB3.0,
> > > while you added the clock references to the EHCI nodes.
> > > Are you sure EHCI is failing without this?

Geert,

I talked to a colleague about the USB_EXTAL.  He pointed me to table
60.1 of the RZ/2 Series, 2nd Generate reference manual
(R01UH0808EJ0100 Rev.1.00),  which shows the USB EHCI needing the
50MHz.  When I clear out the references from ehci0 and echi1, the USB
stops working, so it does appear that using the versaclock as the 3rd
clock is needed for operating.  The device tree bindings for the
generic-ehci provide for up to 4 clocks, so it seems like referencing
clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>, <&versaclock5 3> are
not a violation of the bindings.

I can add a better description when I do the V2 update for this if you like.


> > > Still, it means we need to extend the bindings/driver for
> > > renesas,rcar-gen3-xhci to handle USB_EXTAL.
> >
> > After investigating this, it looks like the USB_EXTAL is already
> > referenced from the device tree and it's referenced by the USB3 Phy.
> > The SoC calls it usb_extal_clk.  Since the phy driver is calling
> > devm_clk_get() it looks like i could just redefine the clocks of
> > usb3_phy0 to point to the versaclock instead of usb_extal_clk.
> >
> > The other option is to use a similar method I proposed for the audio
> > reference clock and redefine the usb_extal_clk as a fixed
> > fixed-factor-clock.
> >
> > Do you have a preference as to which direction I go?
>
> I'd go for the classical solution: override the clocks property of the
> usb3_phy0 node.

I dug into the USB3_phy and it enables and immediately disables the
clocks for the simple purpose of determining which clock reference to
use between usb3s0_clk or usb_extal_clk.  I was hoping that simply
referencing the versaclock here would be sufficient, but the Beacon
board has a usb3s0_clk at 100MHz, and the driver appears to use it
instead of the versaclock so adding the versaclock reference here
isn't sufficient to make it work for the ehci, nor do I think it's
appropriate.

It seems like the driver shouldn't immediately disable the clocks, but
they're expecting external fixed clocks.  Since we meet that criteria
with the usb3s0_clk, the USB3 works without the versaclock reference.

adam
>
> Thanks!
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds