| Message ID | 20201213183759.223246-2-aford173@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | arm64: dts: renesas: Cleanup Beacon Kit and support more SoC's | expand |
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
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
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
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
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
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
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
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
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
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
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
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 --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>; + }; }; };
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(-)