by Geert Uytterhoeven

[permalink] [raw]

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

Hi Adam,

On Mon, Dec 28, 2020 at 3:39 PM Adam Ford <[email protected]> wrote:
> On Mon, Dec 28, 2020 at 6:33 AM Geert Uytterhoeven <[email protected]> wrote:
> > On Thu, Dec 24, 2020 at 2:53 PM Adam Ford <[email protected]> wrote:
> > > On Tue, Dec 22, 2020 at 2:03 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > On Tue, Dec 22, 2020 at 2:39 AM Adam Ford <[email protected]> wrote:
> > > > > On Fri, Dec 18, 2020 at 7:16 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > > > On Thu, Dec 17, 2020 at 12:52 PM Adam Ford <[email protected]> wrote:
> > > > > > > On Thu, Dec 17, 2020 at 2:16 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > > > > > On Wed, Dec 16, 2020 at 6:03 PM Adam Ford <[email protected]> wrote:
> > > > > > > > > On Wed, Dec 16, 2020 at 8:55 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > > > > > > > On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <[email protected]> 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@mail.gmail.com/
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > 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?
> > >
> > > 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.
> >
> > Perhaps you need to use renesas,rcar-usb2-clock-sel?
> > Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.yaml
>
> Thanks for the pointer. I didn't know this existed. It looks like the
> right thing to do. With that node, it appears to enable the
> versaclock and USB works.
> I'll submit a V3 at some point with this node added to each of the
> kit-level files since they use slightly different power-domains.
>
> Do I need to add updates to the bindings for
> renesas,r8a774a1-rcar-usb2-clock-sel; r8a774b1-rcar-usb2-clock-sel;
> and renesas,r8a774e1-rcar-usb2-clock-sel; or I can I just use the
> generic reference to renesas,rcar-gen3-usb2-clock-sel?

Please update the bindings to add support for RZ/G1[MNH].
Note that without doing so, checkpatch will complain when adding the
device nodes to the .dtsi files.

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2021-05-05 12:12:42

by Adam Ford

[permalink] [raw]

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

On Mon, Dec 28, 2020 at 6:33 AM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Adam,
>
> On Thu, Dec 24, 2020 at 2:53 PM Adam Ford <[email protected]> wrote:
> > On Tue, Dec 22, 2020 at 2:03 AM Geert Uytterhoeven <[email protected]> wrote:
> > > On Tue, Dec 22, 2020 at 2:39 AM Adam Ford <[email protected]> wrote:
> > > > On Fri, Dec 18, 2020 at 7:16 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > > On Thu, Dec 17, 2020 at 12:52 PM Adam Ford <[email protected]> wrote:
> > > > > > On Thu, Dec 17, 2020 at 2:16 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > > > > On Wed, Dec 16, 2020 at 6:03 PM Adam Ford <[email protected]> wrote:
> > > > > > > > On Wed, Dec 16, 2020 at 8:55 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > > > > > > On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <[email protected]> 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@mail.gmail.com/
> > > > > > > > >
> > > > > > > >
> > > > > > > > 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?
> >
> > 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.
>
> Perhaps you need to use renesas,rcar-usb2-clock-sel?
> Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.yaml
>

Geert,

Sorry to resurrect an old thread, but I've been working with a
colleague on this, but we've had a lot of interruptions, and we're
just now getting back to this.

Based on our previous conversations, you didn’t want me to add a
reference clock to the EHCI node, because you wanted us to use the
rcar-usb2-clock-sel driver.
If I just add the node for the rcar-usb2-clock-sel that references
the versaclock, the clock tree shows it’s present, but neither the
rcar-usb2-clock-sel nor the versaclock are actually enabled. From
what we’re seeing, the rcar-usb2-clock-sel driver needs a consumer in
order for it to activate.

It seems like it makes sense to optionally associate the
rcar-usb2-clock-sel to all USB nodes, including the USB3 node. The
EHCI controller section in the UG calls out USB_XTAL/USB_EXTAL as
external pins as well as the USBHS module calling out the same pins in
its overview section. The USB3 Phy section mentions
USB_XTAL/USB_EXTAL, but for some reason the USB3 controller overview
does not mention them as “external pins”

I’d like to propose that we add an optional reference clock for the
USB3 which can point to the rcar-usb2-clock-sel.
On the USB EHCI nodes where I previously wanted to reference the
versaclock, I’d like to reference the rcar-usb2-clock-sel.

The clock tree would look something like:
Versaclock-> rcar-usb2-clock-sel->USB

The EHCI clocks would look like:
clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>, <&usb2_clksel>

If we do it this way, we’d need to modify the rcar-usb2-clock-sel to
enable the external versaclock and keep it enabled. Currently, it
enables the clock, reads the clock speed and shuts down after
determining the clock speed.

An alternative to modifying the rcar-usb2-clock-sel code would be to
add both usb2_clksel and the versaclock reference to the EHCI nodes,
but I know you were not completely satisfied with that idea, but it
would likely not require any code changes.

Versaclock-> rcar-usb2-clock-sel->USB<-versaclock

The ECHI clocks would like:
clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>, <&versaclock5 3>,
<&usb2_clksel>

Before I move forward on writing this code, I'd like to make sure
you're OK with one of those options , since there are a few ways to do
it. If you have another suggestion, I'm willing to do that instead.

adam
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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