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

[Geert Uytterhoeven]

Hi Adam,

On Sun, Dec 13, 2020 at 7:38 PM Adam Ford <aford173@xxxxxxxxx> wrote:
> When the board was added, clock drivers were being updated done at
> the same time to allow the versaclock driver to properly configure
> the modes.  Unforutnately, the updates were not applied to the board

Unfortunately

> files at the time they should have been, so do it now.
>
> Signed-off-by: Adam Ford <aford173@xxxxxxxxx>

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

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

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

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds