diff mbox series

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

Message ID 20201213183759.223246-2-aford173@gmail.com
State New
Headers show
Series arm64: dts: renesas: Cleanup Beacon Kit and support more SoC's | expand

Commit Message

Adam Ford Dec. 13, 2020, 6:37 p.m. UTC
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
files at the time they should have been, so do it now.

Signed-off-by: Adam Ford <aford173@gmail.com>
---
 .../dts/renesas/beacon-renesom-baseboard.dtsi | 35 +++++++++++++++++--
 .../boot/dts/renesas/beacon-renesom-som.dtsi  | 26 ++++++++++++++
 2 files changed, 58 insertions(+), 3 deletions(-)

Comments

Geert Uytterhoeven Dec. 16, 2020, 2:55 p.m. UTC | #1
Hi Adam,

On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> 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


Unfortunately

> files at the time they should have been, so do it now.

>

> Signed-off-by: Adam Ford <aford173@gmail.com>


Thanks for your patch!

> --- 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/

BTW, something I missed in the earlier review: is there an override
needed at all?

>  };

>

>  &ehci1 {

>         status = "okay";

> -       clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>;

> +       clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>, <&versaclock5 3>;


Same here.

BTW, something I missed in the earlier review: why did you override

    clocks = <&cpg CPG_MOD 702>;

by

    clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>;

?

>  };

>

>  &hdmi0 {

> @@ -373,12 +374,40 @@ versaclock6_bb: clock-controller@6a {

>                 #clock-cells = <1>;

>                 clocks = <&x304_clk>;

>                 clock-names = "xin";

> -               /* CSI0_MCLK, CSI1_MCLK, AUDIO_CLKIN, USB_HUB_MCLK_BB */

> +               clock-output-names = "versaclock6_bb.out0_sel_i2cb",

> +                                     "versaclock6_bb.out1",

> +                                     "versaclock6_bb.out2",

> +                                     "versaclock6_bb.out3",

> +                                     "versaclock6_bb.out4";


Why? IIUIC, the driver doesn't parse clock-output-names
(and it shouldn't).

>                 assigned-clocks = <&versaclock6_bb 1>,

>                                    <&versaclock6_bb 2>,

>                                    <&versaclock6_bb 3>,

>                                    <&versaclock6_bb 4>;

>                 assigned-clock-rates =  <24000000>, <24000000>, <24000000>, <24576000>;

> +

> +               OUT1 {

> +                       idt,mode = <VC5_CMOS>;

> +                       idt,voltage-microvolts = <1800000>;


Oops. The DT bindings say "idt,voltage-microvolt", the example in the DT
bindings says "idt,voltage-microvolts", and the driver parses
"idt,voltage-microvolts".

According to Documentation/devicetree/bindings/property-units.txt, the
property name should end in "microvolt".

Patch sent.
https://lore.kernel.org/linux-clk/20201216145231.1344317-1-geert+renesas@glider.be/

> +                       idt,slew-percent = <100>;

> +               };


Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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
Adam Ford Dec. 16, 2020, 5:03 p.m. UTC | #2
On Wed, Dec 16, 2020 at 8:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>

> Hi Adam,

>

> On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> 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

>

> Unfortunately


Sorry, I can fix that.

>

> > files at the time they should have been, so do it now.

> >

> > Signed-off-by: Adam Ford <aford173@gmail.com>

>

> Thanks for your patch!

>

> > --- 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.

> BTW, something I missed in the earlier review: is there an override

> needed at all?


We need the versaclock for sure.  I'll do some more testing and try to
clean this up in the next revision.

>

> >  };

> >

> >  &ehci1 {

> >         status = "okay";

> > -       clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>;

> > +       clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>, <&versaclock5 3>;

>

> Same here.

>

> BTW, something I missed in the earlier review: why did you override

>

>     clocks = <&cpg CPG_MOD 702>;

>

> by

>

>     clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>;


Might be an accidental copy-paste error.  I need to review all three
SoC's and adjust the device trees accordingly.

>

> ?

>

> >  };

> >

> >  &hdmi0 {

> > @@ -373,12 +374,40 @@ versaclock6_bb: clock-controller@6a {

> >                 #clock-cells = <1>;

> >                 clocks = <&x304_clk>;

> >                 clock-names = "xin";

> > -               /* CSI0_MCLK, CSI1_MCLK, AUDIO_CLKIN, USB_HUB_MCLK_BB */

> > +               clock-output-names = "versaclock6_bb.out0_sel_i2cb",

> > +                                     "versaclock6_bb.out1",

> > +                                     "versaclock6_bb.out2",

> > +                                     "versaclock6_bb.out3",

> > +                                     "versaclock6_bb.out4";

>

> Why? IIUIC, the driver doesn't parse clock-output-names

> (and it shouldn't).


This was probably copy-paste from an internal repo we have using an
older, customized kernel due to clashing of names with more than one
versaclock was available.  I'll remove it during the next revision.

>

> >                 assigned-clocks = <&versaclock6_bb 1>,

> >                                    <&versaclock6_bb 2>,

> >                                    <&versaclock6_bb 3>,

> >                                    <&versaclock6_bb 4>;

> >                 assigned-clock-rates =  <24000000>, <24000000>, <24000000>, <24576000>;

> > +

> > +               OUT1 {

> > +                       idt,mode = <VC5_CMOS>;

> > +                       idt,voltage-microvolts = <1800000>;

>

> Oops. The DT bindings say "idt,voltage-microvolt", the example in the DT

> bindings says "idt,voltage-microvolts", and the driver parses

> "idt,voltage-microvolts".

>

> According to Documentation/devicetree/bindings/property-units.txt, the

> property name should end in "microvolt".

>

> Patch sent.

> https://lore.kernel.org/linux-clk/20201216145231.1344317-1-geert+renesas@glider.be/

>


Thanks for that.  I'll submit an update based on the patch you sent.

adam
> > +                       idt,slew-percent = <100>;

> > +               };

>


Thank you for the review.  Is that the only patch in the series with
concerns?  I probably won't get to V2 until this weekend.

adam
> Gr{oetje,eeting}s,

>

>                         Geert

>

> --

> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

>

> 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


>

> > files at the time they should have been, so do it now.

> >

> > Signed-off-by: Adam Ford <aford173@gmail.com>

>

> Thanks for your patch!

>

> > --- 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/

>

> BTW, something I missed in the earlier review: is there an override

> needed at all?

>

> >  };

> >

> >  &ehci1 {

> >         status = "okay";

> > -       clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>;

> > +       clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>, <&versaclock5 3>;

>

> Same here.

>

> BTW, something I missed in the earlier review: why did you override

>

>     clocks = <&cpg CPG_MOD 702>;

>

> by

>

>     clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>;

>

> ?

>

> >  };

> >

> >  &hdmi0 {

> > @@ -373,12 +374,40 @@ versaclock6_bb: clock-controller@6a {

> >                 #clock-cells = <1>;

> >                 clocks = <&x304_clk>;

> >                 clock-names = "xin";

> > -               /* CSI0_MCLK, CSI1_MCLK, AUDIO_CLKIN, USB_HUB_MCLK_BB */

> > +               clock-output-names = "versaclock6_bb.out0_sel_i2cb",

> > +                                     "versaclock6_bb.out1",

> > +                                     "versaclock6_bb.out2",

> > +                                     "versaclock6_bb.out3",

> > +                                     "versaclock6_bb.out4";

>

> Why? IIUIC, the driver doesn't parse clock-output-names

> (and it shouldn't).

>

> >                 assigned-clocks = <&versaclock6_bb 1>,

> >                                    <&versaclock6_bb 2>,

> >                                    <&versaclock6_bb 3>,

> >                                    <&versaclock6_bb 4>;

> >                 assigned-clock-rates =  <24000000>, <24000000>, <24000000>, <24576000>;

> > +

> > +               OUT1 {

> > +                       idt,mode = <VC5_CMOS>;

> > +                       idt,voltage-microvolts = <1800000>;

>

> Oops. The DT bindings say "idt,voltage-microvolt", the example in the DT

> bindings says "idt,voltage-microvolts", and the driver parses

> "idt,voltage-microvolts".

>

> According to Documentation/devicetree/bindings/property-units.txt, the

> property name should end in "microvolt".

>

> Patch sent.

> https://lore.kernel.org/linux-clk/20201216145231.1344317-1-geert+renesas@glider.be/

>

> > +                       idt,slew-percent = <100>;

> > +               };

>

> Gr{oetje,eeting}s,

>

>                         Geert

>

> --

> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

>

> 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
Geert Uytterhoeven Dec. 17, 2020, 8:16 a.m. UTC | #3
Hi Adam,

On Wed, Dec 16, 2020 at 6:03 PM Adam Ford <aford173@gmail.com> wrote:
> On Wed, Dec 16, 2020 at 8:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> 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?
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.

> Thank you for the review.  Is that the only patch in the series with

> concerns?  I probably won't get to V2 until this weekend.


Sorry, I still have to review the other patches in your series.
Anyway, we have time until the end of January to queue DT patches for
v5.12...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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
Adam Ford Dec. 17, 2020, 11:52 a.m. UTC | #4
On Thu, Dec 17, 2020 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>

> Hi Adam,

>

> On Wed, Dec 16, 2020 at 6:03 PM Adam Ford <aford173@gmail.com> wrote:

> > On Wed, Dec 16, 2020 at 8:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> 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.

The DU clocks are also expecting an array, so I added the versaclock
to that array as well.

It's similar to the rationale that I'm trying to add the option clock
for the AVB TXC_Ref clock on the other path.  We're using the
versaclock there as well.  The difference is that in the case of the
AVB_TXCRefClk, the driver isn't expecting an array of clocks, it's
only expecting a single clock.  In order to enable the additional
clock,  I started the patch to accept the optional clock for the
TXCRefClk in order to get the clock system to enable the clock.

Because the Versaclock isn't programmed to automatically start, they
need the consumers of the clock to request and enable them.

I admit that I'll probably need to update the bindings to add the
extra clocks as optional, so if you want, I can submit additional
patches to add these optional clocks to their respective bindings.

>

> > Thank you for the review.  Is that the only patch in the series with

> > concerns?  I probably won't get to V2 until this weekend.

>

> Sorry, I still have to review the other patches in your series.

> Anyway, we have time until the end of January to queue DT patches for

> v5.12...


Great.  Thank you,

adam
>

> Gr{oetje,eeting}s,

>

>                         Geert

>

> --

> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

>

> 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
Geert Uytterhoeven Dec. 18, 2020, 1:16 p.m. UTC | #5
Hi Adam,

CC Shimoda-san

On Thu, Dec 17, 2020 at 12:52 PM Adam Ford <aford173@gmail.com> wrote:
> On Thu, Dec 17, 2020 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > On Wed, Dec 16, 2020 at 6:03 PM Adam Ford <aford173@gmail.com> wrote:

> > > On Wed, Dec 16, 2020 at 8:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> 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?

Still, it means we need to extend the bindings/driver for
renesas,rcar-gen3-xhci to handle USB_EXTAL.

> The DU clocks are also expecting an array, so I added the versaclock

> to that array as well.


For DU, the clock inputs are clearly defined in the bindings.

> It's similar to the rationale that I'm trying to add the option clock

> for the AVB TXC_Ref clock on the other path.  We're using the

> versaclock there as well.  The difference is that in the case of the

> AVB_TXCRefClk, the driver isn't expecting an array of clocks, it's

> only expecting a single clock.  In order to enable the additional

> clock,  I started the patch to accept the optional clock for the

> TXCRefClk in order to get the clock system to enable the clock.


Sure.

> Because the Versaclock isn't programmed to automatically start, they

> need the consumers of the clock to request and enable them.

>

> I admit that I'll probably need to update the bindings to add the

> extra clocks as optional, so if you want, I can submit additional

> patches to add these optional clocks to their respective bindings.


Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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
Adam Ford Dec. 22, 2020, 1:39 a.m. UTC | #6
On Fri, Dec 18, 2020 at 7:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>

> Hi Adam,

>

> CC Shimoda-san

>

> On Thu, Dec 17, 2020 at 12:52 PM Adam Ford <aford173@gmail.com> wrote:

> > On Thu, Dec 17, 2020 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > On Wed, Dec 16, 2020 at 6:03 PM Adam Ford <aford173@gmail.com> wrote:

> > > > On Wed, Dec 16, 2020 at 8:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> 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?

>

> 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?

>

> > The DU clocks are also expecting an array, so I added the versaclock

> > to that array as well.

>

> For DU, the clock inputs are clearly defined in the bindings.

>

> > It's similar to the rationale that I'm trying to add the option clock

> > for the AVB TXC_Ref clock on the other path.  We're using the

> > versaclock there as well.  The difference is that in the case of the

> > AVB_TXCRefClk, the driver isn't expecting an array of clocks, it's

> > only expecting a single clock.  In order to enable the additional

> > clock,  I started the patch to accept the optional clock for the

> > TXCRefClk in order to get the clock system to enable the clock.

>

> Sure.

>

> > Because the Versaclock isn't programmed to automatically start, they

> > need the consumers of the clock to request and enable them.

> >

> > I admit that I'll probably need to update the bindings to add the

> > extra clocks as optional, so if you want, I can submit additional

> > patches to add these optional clocks to their respective bindings.

>

> Thanks!

>

> Gr{oetje,eeting}s,

>

>                         Geert

>

> --

> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

>

> 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
Geert Uytterhoeven Dec. 22, 2020, 8:03 a.m. UTC | #7
Hi Adam,

On Tue, Dec 22, 2020 at 2:39 AM Adam Ford <aford173@gmail.com> wrote:
> On Fri, Dec 18, 2020 at 7:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > On Thu, Dec 17, 2020 at 12:52 PM Adam Ford <aford173@gmail.com> wrote:

> > > On Thu, Dec 17, 2020 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > On Wed, Dec 16, 2020 at 6:03 PM Adam Ford <aford173@gmail.com> wrote:

> > > > > On Wed, Dec 16, 2020 at 8:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > > On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> 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?

> >

> > 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.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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
Adam Ford Dec. 24, 2020, 1:52 p.m. UTC | #8
On Tue, Dec 22, 2020 at 2:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>

> Hi Adam,

>

> On Tue, Dec 22, 2020 at 2:39 AM Adam Ford <aford173@gmail.com> wrote:

> > On Fri, Dec 18, 2020 at 7:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > On Thu, Dec 17, 2020 at 12:52 PM Adam Ford <aford173@gmail.com> wrote:

> > > > On Thu, Dec 17, 2020 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > On Wed, Dec 16, 2020 at 6:03 PM Adam Ford <aford173@gmail.com> wrote:

> > > > > > On Wed, Dec 16, 2020 at 8:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > > > On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> 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?


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@linux-m68k.org

>

> 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
Geert Uytterhoeven Dec. 28, 2020, 12:33 p.m. UTC | #9
Hi Adam,

On Thu, Dec 24, 2020 at 2:53 PM Adam Ford <aford173@gmail.com> wrote:
> On Tue, Dec 22, 2020 at 2:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > On Tue, Dec 22, 2020 at 2:39 AM Adam Ford <aford173@gmail.com> wrote:

> > > On Fri, Dec 18, 2020 at 7:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > On Thu, Dec 17, 2020 at 12:52 PM Adam Ford <aford173@gmail.com> wrote:

> > > > > On Thu, Dec 17, 2020 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > > On Wed, Dec 16, 2020 at 6:03 PM Adam Ford <aford173@gmail.com> wrote:

> > > > > > > On Wed, Dec 16, 2020 at 8:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > > > > On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> 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

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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
Adam Ford Dec. 28, 2020, 2:38 p.m. UTC | #10
On Mon, Dec 28, 2020 at 6:33 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>

> Hi Adam,

>

> On Thu, Dec 24, 2020 at 2:53 PM Adam Ford <aford173@gmail.com> wrote:

> > On Tue, Dec 22, 2020 at 2:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > On Tue, Dec 22, 2020 at 2:39 AM Adam Ford <aford173@gmail.com> wrote:

> > > > On Fri, Dec 18, 2020 at 7:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > On Thu, Dec 17, 2020 at 12:52 PM Adam Ford <aford173@gmail.com> wrote:

> > > > > > On Thu, Dec 17, 2020 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > > > On Wed, Dec 16, 2020 at 6:03 PM Adam Ford <aford173@gmail.com> wrote:

> > > > > > > > On Wed, Dec 16, 2020 at 8:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > > > > > On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> 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?

adam
>

> Gr{oetje,eeting}s,

>

>                         Geert

>

> --

> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

>

> 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
Geert Uytterhoeven Dec. 28, 2020, 2:52 p.m. UTC | #11
Hi Adam,

On Mon, Dec 28, 2020 at 3:39 PM Adam Ford <aford173@gmail.com> wrote:
> On Mon, Dec 28, 2020 at 6:33 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > On Thu, Dec 24, 2020 at 2:53 PM Adam Ford <aford173@gmail.com> wrote:

> > > On Tue, Dec 22, 2020 at 2:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > On Tue, Dec 22, 2020 at 2:39 AM Adam Ford <aford173@gmail.com> wrote:

> > > > > On Fri, Dec 18, 2020 at 7:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > > On Thu, Dec 17, 2020 at 12:52 PM Adam Ford <aford173@gmail.com> wrote:

> > > > > > > On Thu, Dec 17, 2020 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > > > > On Wed, Dec 16, 2020 at 6:03 PM Adam Ford <aford173@gmail.com> wrote:

> > > > > > > > > On Wed, Dec 16, 2020 at 8:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > > > > > > On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> 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 -- geert@linux-m68k.org

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
Adam Ford May 5, 2021, 12:07 p.m. UTC | #12
On Mon, Dec 28, 2020 at 6:33 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>

> Hi Adam,

>

> On Thu, Dec 24, 2020 at 2:53 PM Adam Ford <aford173@gmail.com> wrote:

> > On Tue, Dec 22, 2020 at 2:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > On Tue, Dec 22, 2020 at 2:39 AM Adam Ford <aford173@gmail.com> wrote:

> > > > On Fri, Dec 18, 2020 at 7:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > On Thu, Dec 17, 2020 at 12:52 PM Adam Ford <aford173@gmail.com> wrote:

> > > > > > On Thu, Dec 17, 2020 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > > > On Wed, Dec 16, 2020 at 6:03 PM Adam Ford <aford173@gmail.com> wrote:

> > > > > > > > On Wed, Dec 16, 2020 at 8:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > > > > > > > On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@gmail.com> 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 -- geert@linux-m68k.org

>

> 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
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi b/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi
index e66b5b36e489..3c84e060c69f 100644
--- 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>;
 };
 
 &ehci1 {
 	status = "okay";
-	clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>;
+	clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>, <&versaclock5 3>;
 };
 
 &hdmi0 {
@@ -373,12 +374,40 @@  versaclock6_bb: clock-controller@6a {
 		#clock-cells = <1>;
 		clocks = <&x304_clk>;
 		clock-names = "xin";
-		/* CSI0_MCLK, CSI1_MCLK, AUDIO_CLKIN, USB_HUB_MCLK_BB */
+		clock-output-names = "versaclock6_bb.out0_sel_i2cb",
+				      "versaclock6_bb.out1",
+				      "versaclock6_bb.out2",
+				      "versaclock6_bb.out3",
+				      "versaclock6_bb.out4";
 		assigned-clocks = <&versaclock6_bb 1>,
 				   <&versaclock6_bb 2>,
 				   <&versaclock6_bb 3>,
 				   <&versaclock6_bb 4>;
 		assigned-clock-rates =	<24000000>, <24000000>, <24000000>, <24576000>;
+
+		OUT1 {
+			idt,mode = <VC5_CMOS>;
+			idt,voltage-microvolts = <1800000>;
+			idt,slew-percent = <100>;
+		};
+
+		OUT2 {
+			idt,mode = <VC5_CMOS>;
+			idt,voltage-microvolts = <1800000>;
+			idt,slew-percent = <100>;
+		};
+
+		OUT3 {
+			idt,mode = <VC5_CMOS>;
+			idt,voltage-microvolts = <3300000>;
+			idt,slew-percent = <100>;
+		};
+
+		OUT4 {
+			idt,mode = <VC5_CMOS>;
+			idt,voltage-microvolts = <3300000>;
+			idt,slew-percent = <100>;
+		};
 	};
 };
 
diff --git a/arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi b/arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi
index 8ac167aa18f0..449ff5937fc6 100644
--- a/arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi
+++ b/arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi
@@ -4,6 +4,7 @@ 
  */
 
 #include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/clk/versaclock.h>
 
 / {
 	memory@48000000 {
@@ -170,7 +171,32 @@  versaclock5: versaclock_som@6a {
 				   <&versaclock5 2>,
 				   <&versaclock5 3>,
 				   <&versaclock5 4>;
+
 		assigned-clock-rates = <33333333>, <33333333>, <50000000>, <125000000>;
+
+		OUT1 {
+			idt,mode = <VC5_CMOS>;
+			idt,voltage-microvolts = <1800000>;
+			idt,slew-percent = <100>;
+		};
+
+		OUT2 {
+			idt,mode = <VC5_CMOS>;
+			idt,voltage-microvolts = <1800000>;
+			idt,slew-percent = <100>;
+		};
+
+		OUT3 {
+			idt,mode = <VC5_CMOS>;
+			idt,voltage-microvolts = <1800000>;
+			idt,slew-percent = <100>;
+		};
+
+		OUT4 {
+			idt,mode = <VC5_CMOS>;
+			idt,voltage-microvolts = <3300000>;
+			idt,slew-percent = <100>;
+		};
 	};
 };