[8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC
diff mbox series

Message ID 20200113141556.GI3606@pflmari
State Changes Requested
Delegated to: Geert Uytterhoeven
Headers show
Series
  • media: i2c: adv748x: add support for HDMI audio
Related show

Commit Message

Alex Riesen Jan. 13, 2020, 2:15 p.m. UTC
Not sure if all variants of the Salvator board have the HDMI decoder
chip (the ADV7482) connected to the SSI4 on R-Car SoC, as it is on
Salvator-X ES1, so the the ADV7482 endpoint and connection definitions
are placed in the board file.

I do assume though that all Salvator variants have the CLK_C clock line
hard-wired to the ADV7482 HDMI decoder, and remove it from the list of
clocks provided by the R-Car sound system.

The I2C wiring is also likely to persist across the variants (similar
to ak4613, connected to the same interface), so that is in the common
file.

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
---
 .../dts/renesas/r8a7795-es1-salvator-x.dts    | 24 ++++++++++++-
 .../boot/dts/renesas/salvator-common.dtsi     | 35 ++++++++++++++++---
 2 files changed, 53 insertions(+), 6 deletions(-)

Comments

Geert Uytterhoeven March 2, 2020, 12:28 p.m. UTC | #1
Hi Alex,

Thanks for your patch!

On Mon, Jan 13, 2020 at 3:24 PM Alex Riesen
<alexander.riesen@cetitec.com> wrote:
> Not sure if all variants of the Salvator board have the HDMI decoder
> chip (the ADV7482) connected to the SSI4 on R-Car SoC, as it is on
> Salvator-X ES1, so the the ADV7482 endpoint and connection definitions
> are placed in the board file.

Both Salvator-X and Salvator-XS have SSI4 wired to the ADV7482.

> I do assume though that all Salvator variants have the CLK_C clock line
> hard-wired to the ADV7482 HDMI decoder, and remove it from the list of
> clocks provided by the R-Car sound system.

Yes, both Salvator-X and Salvator-XS have it wired that way.  But please
see below.

> The I2C wiring is also likely to persist across the variants (similar
> to ak4613, connected to the same interface), so that is in the common
> file.
>
> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>

Below are my comments w.r.t. the board-specific wiring.
I'll defer to the multimedia people for commenting on the audio parts.

BTW, what is the status of the other patches in this series?

> --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> @@ -322,6 +322,10 @@
>         clock-frequency = <22579200>;
>  };
>
> +&audio_clk_c {
> +       clock-frequency = <12288000>;
> +};

Does the ADV7482 always generate a 12.288 MHz clock signal?
Or is this programmable?

> +
>  &avb {
>         pinctrl-0 = <&avb_pins>;
>         pinctrl-names = "default";
> @@ -471,12 +475,14 @@
>
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> +               #sound-dai-cells = <0>;
>
>                 interrupt-parent = <&gpio6>;
>                 interrupt-names = "intrq1", "intrq2";
>                 interrupts = <30 IRQ_TYPE_LEVEL_LOW>,
>                              <31 IRQ_TYPE_LEVEL_LOW>;
> -
> +               clocks = <&rcar_sound 3>, <&audio_clk_c>;
> +               clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";

The above declares the Audio CLK C to be a clock input of the ADV7482, while
it is an output.
Furthermore, the DT bindings do not document that clocks can be specified.

>                 port@7 {
>                         reg = <7>;
>
> @@ -512,6 +518,14 @@
>                                 remote-endpoint = <&csi20_in>;
>                         };
>                 };
> +
> +               port@c {
> +                       reg = <12>;
> +
> +                       adv7482_i2s: endpoint {
> +                               /* remote-endpoint defined in the board file */
> +                       };
> +               };
>         };
>
>         csa_vdd: adc@7c {
> @@ -686,7 +700,8 @@
>         };
>
>         sound_pins: sound {
> -               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
> +               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
> +                        "ssi4_data";

Missing "ss4_ctrl", for the SCK4 and WS4 pins.

>                 function = "ssi";
>         };
>
> @@ -735,8 +750,8 @@
>         pinctrl-0 = <&sound_pins &sound_clk_pins>;
>         pinctrl-names = "default";
>
> -       /* Single DAI */
> -       #sound-dai-cells = <0>;
> +       /* multi DAI */
> +       #sound-dai-cells = <1>;
>
>         /* audio_clkout0/1/2/3 */
>         #clock-cells = <1>;
> @@ -760,8 +775,18 @@
>                  <&cpg CPG_MOD 1020>, <&cpg CPG_MOD 1021>,
>                  <&cpg CPG_MOD 1019>, <&cpg CPG_MOD 1018>,
>                  <&audio_clk_a>, <&cs2000>,
> -                <&audio_clk_c>,

Why remove it? This is the list of clock inputs, not outputs.

>                  <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
> +       clock-names = "ssi-all",
> +                     "ssi.9", "ssi.8", "ssi.7", "ssi.6",
> +                     "ssi.5", "ssi.4", "ssi.3", "ssi.2",
> +                     "ssi.1", "ssi.0",
> +                     "src.9", "src.8", "src.7", "src.6",
> +                     "src.5", "src.4", "src.3", "src.2",
> +                     "src.1", "src.0",
> +                     "mix.1", "mix.0",
> +                     "ctu.1", "ctu.0",
> +                     "dvc.0", "dvc.1",
> +                     "clk_a", "clk_b", "clk_i";
>
>         ports {
>                 #address-cells = <1>;
> --
> 2.24.1.508.g91d2dafee0

Gr{oetje,eeting}s,

                        Geert
Alex Riesen March 2, 2020, 1:40 p.m. UTC | #2
Geert Uytterhoeven, Mon, Mar 02, 2020 13:28:13 +0100:
> Hi Alex,
> 
> Thanks for your patch!
> 
> On Mon, Jan 13, 2020 at 3:24 PM Alex Riesen
> <alexander.riesen@cetitec.com> wrote:
> > Not sure if all variants of the Salvator board have the HDMI decoder
> > chip (the ADV7482) connected to the SSI4 on R-Car SoC, as it is on
> > Salvator-X ES1, so the the ADV7482 endpoint and connection definitions
> > are placed in the board file.
> 
> Both Salvator-X and Salvator-XS have SSI4 wired to the ADV7482.
> 
> > I do assume though that all Salvator variants have the CLK_C clock line
> > hard-wired to the ADV7482 HDMI decoder, and remove it from the list of
> > clocks provided by the R-Car sound system.
> 
> Yes, both Salvator-X and Salvator-XS have it wired that way.

Ok, seems like I can move that part into the common file as well.
Integrations of ADV7482 and R-Car which use salvator-common.dts can still
redefine the endpoint settings in their board files, right?

> But please see below.

...

> > The I2C wiring is also likely to persist across the variants (similar
> > to ak4613, connected to the same interface), so that is in the common
> > file.
> >
> > Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> 
> Below are my comments w.r.t. the board-specific wiring.
> I'll defer to the multimedia people for commenting on the audio parts.
> 
> BTW, what is the status of the other patches in this series?

"Submitted", at the moment. Besides you and Rob Herring no one said anything
yet (either that or I missed the replies).

> > --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > @@ -322,6 +322,10 @@
> >         clock-frequency = <22579200>;
> >  };
> >
> > +&audio_clk_c {
> > +       clock-frequency = <12288000>;
> > +};
> 
> Does the ADV7482 always generate a 12.288 MHz clock signal?
> Or is this programmable?

Oops. It looks like it is and the value is derived from the sampling rate
(48kHz) and the master clock multiplier. Both hard-coded in the board file.

> > video-receiver@70 {
> > 	compatible = "adi,adv7482";
> > ...
> > +   clocks = <&rcar_sound 3>, <&audio_clk_c>;
> > +   clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";
> 
> The above declares the Audio CLK C to be a clock input of the ADV7482, while
> it is an output.

I would gladly give it right direction if I *really* understood what I was
doing...

> Furthermore, the DT bindings do not document that clocks can be specified.

Should the DT bindings document that the clock cannot be specified than?

> > @@ -686,7 +700,8 @@
> >         };
> >
> >         sound_pins: sound {
> > -               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
> > +               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
> > +                        "ssi4_data";
> 
> Missing "ss4_ctrl", for the SCK4 and WS4 pins.

I'll add them.
As the device seems to function even without thoes, does this mean the pins in
the group are used "on demand" by whatever needs them?

> > @@ -760,8 +775,18 @@
> >                  <&cpg CPG_MOD 1020>, <&cpg CPG_MOD 1021>,
> >                  <&cpg CPG_MOD 1019>, <&cpg CPG_MOD 1018>,
> >                  <&audio_clk_a>, <&cs2000>,
> > -                <&audio_clk_c>,
> 
> Why remove it? This is the list of clock inputs, not outputs.

...probably because I was thinking the specification was exactly the other way
around.

Does a "clocks = ..." statement always mean input clocks?

I shall correct that and re-test (might take a while, I don't have the
hardware anymore).

Thanks for looking!
Regards,
Alex
Geert Uytterhoeven March 2, 2020, 1:47 p.m. UTC | #3
Hi Alex,

On Mon, Mar 2, 2020 at 2:40 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> Geert Uytterhoeven, Mon, Mar 02, 2020 13:28:13 +0100:
> > On Mon, Jan 13, 2020 at 3:24 PM Alex Riesen
> > <alexander.riesen@cetitec.com> wrote:
> > > Not sure if all variants of the Salvator board have the HDMI decoder
> > > chip (the ADV7482) connected to the SSI4 on R-Car SoC, as it is on
> > > Salvator-X ES1, so the the ADV7482 endpoint and connection definitions
> > > are placed in the board file.
> >
> > Both Salvator-X and Salvator-XS have SSI4 wired to the ADV7482.
> >
> > > I do assume though that all Salvator variants have the CLK_C clock line
> > > hard-wired to the ADV7482 HDMI decoder, and remove it from the list of
> > > clocks provided by the R-Car sound system.
> >
> > Yes, both Salvator-X and Salvator-XS have it wired that way.
>
> Ok, seems like I can move that part into the common file as well.
> Integrations of ADV7482 and R-Car which use salvator-common.dts can still
> redefine the endpoint settings in their board files, right?

Indeed.

> > > --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > > +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > > @@ -322,6 +322,10 @@
> > >         clock-frequency = <22579200>;
> > >  };
> > >
> > > +&audio_clk_c {
> > > +       clock-frequency = <12288000>;
> > > +};
> >
> > Does the ADV7482 always generate a 12.288 MHz clock signal?
> > Or is this programmable?
>
> Oops. It looks like it is and the value is derived from the sampling rate
> (48kHz) and the master clock multiplier. Both hard-coded in the board file.

Where are these hardcoded in the board file?
Even if they are, technically this is a clock output of the ADC7482.

> > > video-receiver@70 {
> > >     compatible = "adi,adv7482";
> > > ...
> > > +   clocks = <&rcar_sound 3>, <&audio_clk_c>;
> > > +   clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";
> >
> > The above declares the Audio CLK C to be a clock input of the ADV7482, while
> > it is an output.
>
> I would gladly give it right direction if I *really* understood what I was
> doing...

:-)

> > Furthermore, the DT bindings do not document that clocks can be specified.
>
> Should the DT bindings document that the clock cannot be specified than?

It currently does say so, as it doesn't list "clocks" in its properties section.

> > > @@ -686,7 +700,8 @@
> > >         };
> > >
> > >         sound_pins: sound {
> > > -               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
> > > +               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
> > > +                        "ssi4_data";
> >
> > Missing "ss4_ctrl", for the SCK4 and WS4 pins.
>
> I'll add them.
> As the device seems to function even without thoes, does this mean the pins in
> the group are used "on demand" by whatever needs them?

Probably the SCK4/WS4 functions are the reset-state defaults.

> > > @@ -760,8 +775,18 @@
> > >                  <&cpg CPG_MOD 1020>, <&cpg CPG_MOD 1021>,
> > >                  <&cpg CPG_MOD 1019>, <&cpg CPG_MOD 1018>,
> > >                  <&audio_clk_a>, <&cs2000>,
> > > -                <&audio_clk_c>,
> >
> > Why remove it? This is the list of clock inputs, not outputs.
>
> ...probably because I was thinking the specification was exactly the other way
> around.
>
> Does a "clocks = ..." statement always mean input clocks?

Yes it does.
If a device has clock outputs and is thus a clock provider, it should
have a #clock-cells property, and this should be documented in the bindings.

A clock consumer will refer to clocks of a provider using the "clocks"
property, specifying a clock specifier (phandle and zero or more indices)
for each clock referenced.

> I shall correct that and re-test (might take a while, I don't have the
> hardware anymore).

Oops.

Gr{oetje,eeting}s,

                        Geert
Alex Riesen March 2, 2020, 3:07 p.m. UTC | #4
Geert Uytterhoeven, Mon, Mar 02, 2020 14:47:46 +0100:
> On Mon, Mar 2, 2020 at 2:40 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > > --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > > > +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > > > @@ -322,6 +322,10 @@
> > > >         clock-frequency = <22579200>;
> > > >  };
> > > >
> > > > +&audio_clk_c {
> > > > +       clock-frequency = <12288000>;
> > > > +};
> > >
> > > Does the ADV7482 always generate a 12.288 MHz clock signal?
> > > Or is this programmable?
> >
> > Oops. It looks like it is and the value is derived from the sampling rate
> > (48kHz) and the master clock multiplier. Both hard-coded in the board file.
> 
> Where are these hardcoded in the board file?

In the endpoint definition, arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dts

So the frequency can be set at the run-time, perhaps even derived from
endpoint connected to the output. In this case, rsnd_endpoint3,
which has the "mclk-fs" setting. Not sure if the sampling rate
can be set to something else for the HDMI, though.

> Even if they are, technically this is a clock output of the ADV7482.

... which I hope to correct as soon as I steal the hardware from whoever stole
it from me...

> > > > video-receiver@70 {
> > > >     compatible = "adi,adv7482";
> > > > ...
> > > > +   clocks = <&rcar_sound 3>, <&audio_clk_c>;
> > > > +   clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";
> > >
> > > The above declares the Audio CLK C to be a clock input of the ADV7482, while
> > > it is an output.
> >
> > I would gladly give it right direction if I *really* understood what I was
> > doing...
> 
> :-)
> 
> > > Furthermore, the DT bindings do not document that clocks can be specified.
> >
> > Should the DT bindings document that the clock cannot be specified than?
> 
> It currently does say so, as it doesn't list "clocks" in its properties section.

The bindings documentation file, which we're talking about here and which does
not list the specifiable input clocks in its properties, is it the

    Documentation/devicetree/bindings/media/i2c/adv748x.txt

?

And this absence of documentation also means that whatever clocks (both input
in "clocks=" and output in "#clock-cells") listed in a specific .dts are just
an integration detail?

Does this below makes more sense, than?

    video-receiver@70 {
        compatible = "adi,adv7482";
        clocks = <&rcar_sound 3>;
        clock-names = "clk-hdmi-video";
        adv748x_mclk: mclk {
            compatible = "fixed-clock";
            #clock-cells =  <0>;
            /* frequency hard-coded for illustration */
            clock-frequency = <12288000>;
            clock-output-names = "clk-hdmi-i2s-mclk";
        };
    };

Now I'm a bit hazy on how to declare that the MCLK output of the
video-receiver@70 is connected to the Audio Clock C of the SoC...
Probably remove use of "audio_clk_c" completely?

> > > > @@ -686,7 +700,8 @@
> > > >         };
> > > >
> > > >         sound_pins: sound {
> > > > -               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
> > > > +               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
> > > > +                        "ssi4_data";
> > >
> > > Missing "ss4_ctrl", for the SCK4 and WS4 pins.
> >
> > I'll add them.
> > As the device seems to function even without thoes, does this mean the
> > pins in the group are used "on demand" by whatever needs them?
> 
> Probably the SCK4/WS4 functions are the reset-state defaults.

That ... might require some trial and testing: when I add them to the group,
the reset defaults will be overridden by the platform initialization, which is
not necessarily the reset default. Will see.

> >
> > Does a "clocks = ..." statement always mean input clocks?
> 
> Yes it does.
> If a device has clock outputs and is thus a clock provider, it should
> have a #clock-cells property, and this should be documented in the bindings.
> 
> A clock consumer will refer to clocks of a provider using the "clocks"
> property, specifying a clock specifier (phandle and zero or more indices)
> for each clock referenced.

Something like this?

    &rcar_sound {
        clocks = ...,
                 <&adv748x_mclk>,
                 <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
        clock-names = ...,
                      "clk_c",
                      "clk_i";
    };

Regards,
Alex
Geert Uytterhoeven March 2, 2020, 3:32 p.m. UTC | #5
Hi Alex,

On Mon, Mar 2, 2020 at 4:07 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> Geert Uytterhoeven, Mon, Mar 02, 2020 14:47:46 +0100:
> > On Mon, Mar 2, 2020 at 2:40 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > > > --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > > > > +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > > > > @@ -322,6 +322,10 @@
> > > > >         clock-frequency = <22579200>;
> > > > >  };
> > > > >
> > > > > +&audio_clk_c {
> > > > > +       clock-frequency = <12288000>;
> > > > > +};
> > > >
> > > > Does the ADV7482 always generate a 12.288 MHz clock signal?
> > > > Or is this programmable?
> > >
> > > Oops. It looks like it is and the value is derived from the sampling rate
> > > (48kHz) and the master clock multiplier. Both hard-coded in the board file.
> >
> > Where are these hardcoded in the board file?
>
> In the endpoint definition, arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dts
>
> So the frequency can be set at the run-time, perhaps even derived from
> endpoint connected to the output. In this case, rsnd_endpoint3,
> which has the "mclk-fs" setting. Not sure if the sampling rate
> can be set to something else for the HDMI, though.
>
> > Even if they are, technically this is a clock output of the ADV7482.
>
> ... which I hope to correct as soon as I steal the hardware from whoever stole
> it from me...
>
> > > > > video-receiver@70 {
> > > > >     compatible = "adi,adv7482";
> > > > > ...
> > > > > +   clocks = <&rcar_sound 3>, <&audio_clk_c>;
> > > > > +   clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";
> > > >
> > > > The above declares the Audio CLK C to be a clock input of the ADV7482, while
> > > > it is an output.
> > >
> > > I would gladly give it right direction if I *really* understood what I was
> > > doing...
> >
> > :-)
> >
> > > > Furthermore, the DT bindings do not document that clocks can be specified.
> > >
> > > Should the DT bindings document that the clock cannot be specified than?
> >
> > It currently does say so, as it doesn't list "clocks" in its properties section.
>
> The bindings documentation file, which we're talking about here and which does
> not list the specifiable input clocks in its properties, is it the
>
>     Documentation/devicetree/bindings/media/i2c/adv748x.txt
>
> ?

Yes.

>
> And this absence of documentation also means that whatever clocks (both input
> in "clocks=" and output in "#clock-cells") listed in a specific .dts are just
> an integration detail?

No, the absence probably means that any clock-related properties in a .dts
file will just be ignored.

Looking at the driver source, it indeed has no support related to clocks at all.

> Does this below makes more sense, than?
>
>     video-receiver@70 {
>         compatible = "adi,adv7482";
>         clocks = <&rcar_sound 3>;
>         clock-names = "clk-hdmi-video";
>         adv748x_mclk: mclk {
>             compatible = "fixed-clock";
>             #clock-cells =  <0>;
>             /* frequency hard-coded for illustration */
>             clock-frequency = <12288000>;
>             clock-output-names = "clk-hdmi-i2s-mclk";
>         };
>     };

The #clock-cells should be in the main video-receiver node.
Probably there is more than one clock output, so #clock-cells may be 1?
There is no need for a fixed-clock compatible, nor for clock-frequency
and clock-output-names.

But most important: this should be documented in the adv748x DT bindings,
and implemented in the adv748x driver.

> Now I'm a bit hazy on how to declare that the MCLK output of the
> video-receiver@70 is connected to the Audio Clock C of the SoC...
> Probably remove use of "audio_clk_c" completely?

Yes, the current audio_clk_c definition in the DTS assumes a fixed
crystal.

> > > > > @@ -686,7 +700,8 @@
> > > > >         };
> > > > >
> > > > >         sound_pins: sound {
> > > > > -               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
> > > > > +               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
> > > > > +                        "ssi4_data";
> > > >
> > > > Missing "ss4_ctrl", for the SCK4 and WS4 pins.
> > >
> > > I'll add them.
> > > As the device seems to function even without thoes, does this mean the
> > > pins in the group are used "on demand" by whatever needs them?
> >
> > Probably the SCK4/WS4 functions are the reset-state defaults.
>
> That ... might require some trial and testing: when I add them to the group,
> the reset defaults will be overridden by the platform initialization, which is
> not necessarily the reset default. Will see.

Or by the boot loader.  Anyway, you need to specify these in the DTS.

> > > Does a "clocks = ..." statement always mean input clocks?
> >
> > Yes it does.
> > If a device has clock outputs and is thus a clock provider, it should
> > have a #clock-cells property, and this should be documented in the bindings.
> >
> > A clock consumer will refer to clocks of a provider using the "clocks"
> > property, specifying a clock specifier (phandle and zero or more indices)
> > for each clock referenced.
>
> Something like this?
>
>     &rcar_sound {
>         clocks = ...,
>                  <&adv748x_mclk>,
>                  <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
>         clock-names = ...,
>                       "clk_c",
>                       "clk_i";
>     };

More or less.

Might become

    find_a_better_label_choice: video-receiver@70 {
            ...
    };

    &rcar_sound {
            clock = ...,
                    <&find_a_better_label_choice 0>,
                    ...
    };

as there may be multiple clock outputs on the ADV7482.

Gr{oetje,eeting}s,

                        Geert
Alex Riesen March 2, 2020, 4:09 p.m. UTC | #6
Hi Geert,

Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:
> > And this absence of documentation also means that whatever clocks (both input
> > in "clocks=" and output in "#clock-cells") listed in a specific .dts are just
> > an integration detail?
> 
> No, the absence probably means that any clock-related properties in a .dts
> file will just be ignored.
> 
> Looking at the driver source, it indeed has no support related to clocks at all.

...

> > Does this below makes more sense, than?
> >
> >     video-receiver@70 {
> >         compatible = "adi,adv7482";
> >         clocks = <&rcar_sound 3>;
> >         clock-names = "clk-hdmi-video";
> >         adv748x_mclk: mclk {
> >             compatible = "fixed-clock";
> >             #clock-cells =  <0>;
> >             /* frequency hard-coded for illustration */
> >             clock-frequency = <12288000>;
> >             clock-output-names = "clk-hdmi-i2s-mclk";
> >         };
> >     };
> 
> The #clock-cells should be in the main video-receiver node.
> Probably there is more than one clock output, so #clock-cells may be 1?

AFAICS, the device can provide only this one clock line (audio master clock
for I2S output)... I shall re-check, just in case.

> There is no need for a fixed-clock compatible, nor for clock-frequency
> and clock-output-names.
> 
> But most important: this should be documented in the adv748x DT bindings,
> and implemented in the adv748x driver.

So if the driver is to export that clock for the kernel (like in this case),
it must implement its support?

> > > > Does a "clocks = ..." statement always mean input clocks?
> > >
> > > Yes it does.
> > > If a device has clock outputs and is thus a clock provider, it should
> > > have a #clock-cells property, and this should be documented in the bindings.
> > >
> > > A clock consumer will refer to clocks of a provider using the "clocks"
> > > property, specifying a clock specifier (phandle and zero or more indices)
> > > for each clock referenced.
> >
> > Something like this?
> >
> >     &rcar_sound {
> >         clocks = ...,
> >                  <&adv748x_mclk>,
> >                  <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
> >         clock-names = ...,
> >                       "clk_c",
> >                       "clk_i";
> >     };
> 
> More or less.
> 
> Might become
> 
>     find_a_better_label_choice: video-receiver@70 {
>             ...
>     };
> 
>     &rcar_sound {
>             clock = ...,
>                     <&find_a_better_label_choice 0>,
>                     ...
>     };
> 
> as there may be multiple clock outputs on the ADV7482.

I see. Working on it.

Thanks a lot!

Regards,
Alex
Geert Uytterhoeven March 2, 2020, 4:13 p.m. UTC | #7
Hi Alex,

On Mon, Mar 2, 2020 at 5:09 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:
> > > And this absence of documentation also means that whatever clocks (both input
> > > in "clocks=" and output in "#clock-cells") listed in a specific .dts are just
> > > an integration detail?
> >
> > No, the absence probably means that any clock-related properties in a .dts
> > file will just be ignored.
> >
> > Looking at the driver source, it indeed has no support related to clocks at all.
>
> ...
>
> > > Does this below makes more sense, than?
> > >
> > >     video-receiver@70 {
> > >         compatible = "adi,adv7482";
> > >         clocks = <&rcar_sound 3>;
> > >         clock-names = "clk-hdmi-video";
> > >         adv748x_mclk: mclk {
> > >             compatible = "fixed-clock";
> > >             #clock-cells =  <0>;
> > >             /* frequency hard-coded for illustration */
> > >             clock-frequency = <12288000>;
> > >             clock-output-names = "clk-hdmi-i2s-mclk";
> > >         };
> > >     };
> >
> > The #clock-cells should be in the main video-receiver node.
> > Probably there is more than one clock output, so #clock-cells may be 1?
>
> AFAICS, the device can provide only this one clock line (audio master clock
> for I2S output)... I shall re-check, just in case.
>
> > There is no need for a fixed-clock compatible, nor for clock-frequency
> > and clock-output-names.
> >
> > But most important: this should be documented in the adv748x DT bindings,
> > and implemented in the adv748x driver.
>
> So if the driver is to export that clock for the kernel (like in this case),
> it must implement its support?

Exactly.  Unless that pin is hardcoded to output a fixed clock, in which case
you can just override the existing audio_clk_c rate.

Gr{oetje,eeting}s,

                        Geert
Alex Riesen March 5, 2020, 2:36 p.m. UTC | #8
Hi Geert,

Geert Uytterhoeven, Mon, Mar 02, 2020 17:13:30 +0100:
> On Mon, Mar 2, 2020 at 5:09 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:
> > >
> > > The #clock-cells should be in the main video-receiver node.
> > > Probably there is more than one clock output, so #clock-cells may be 1?
> >
> > AFAICS, the device can provide only this one clock line (audio master clock
> > for I2S output)... I shall re-check, just in case.

And you're right, of course: the audio output formatting module of the ADV748x
devices provides a set of clock lines related to the I2S pins: the already
discussed master clock, left-right channel clock and the serial clock (bit
clock?).

> > > There is no need for a fixed-clock compatible, nor for clock-frequency
> > > and clock-output-names.
> > >
> > > But most important: this should be documented in the adv748x DT bindings,
> > > and implemented in the adv748x driver.
> >
> > So if the driver is to export that clock for the kernel (like in this case),
> > it must implement its support?
> 
> Exactly.  Unless that pin is hardcoded to output a fixed clock, in which case
> you can just override the existing audio_clk_c rate.

Just to try it out (I'll set #clock-cells to 1), I registered a fixed rate
clock in the driver, added a clock provider:

adv748x_probe:

    clk = clk_register_fixed_rate(state->dev,
				  "clk-hdmi-i2s-mclk",
				  NULL     /* parent_name */,
				  0        /* flags */,
				  12288000 /* rate */);
    of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get, clk);

And removed the audio_clk_c frequency setting. I also replaced the audio_clk_c
in the list of input clocks of the R-Car-side sound card with the phandle of
the adv7482 main node:

salvator-common.dtsi:

    &i2c4 {
	status = "okay";

	adv7482_hdmi_decoder: video-receiver@70 {
	    #clock-cells = <0>; // to be replaced with <1>
	};
    };

    &rcar_sound {
	clocks = ..., <&adv7482_hdmi_decoder>, ...;
    };

As everything continues to work as before, I assume that at least the clock
dependencies were resolved.

Is there a way to verify that the added input clock is actually used?
IOW, if its frequency is actually has been programmed into the ssi4 (R-Car
receiving hardware) registers, and not just a left-over from previuos attempts
or plain default setting?

As the ADV748x devices seem to provide also the clocks for video outputs, will
it make any sense to place the clock definition into the port node?
Or should all provided clocks be indexed in the main device node?

Regards,
Alex
Geert Uytterhoeven March 6, 2020, 12:21 p.m. UTC | #9
Hi Alex,

On Thu, Mar 5, 2020 at 3:36 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> Geert Uytterhoeven, Mon, Mar 02, 2020 17:13:30 +0100:
> > On Mon, Mar 2, 2020 at 5:09 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:
> > > >
> > > > The #clock-cells should be in the main video-receiver node.
> > > > Probably there is more than one clock output, so #clock-cells may be 1?
> > >
> > > AFAICS, the device can provide only this one clock line (audio master clock
> > > for I2S output)... I shall re-check, just in case.
>
> And you're right, of course: the audio output formatting module of the ADV748x
> devices provides a set of clock lines related to the I2S pins: the already
> discussed master clock, left-right channel clock and the serial clock (bit
> clock?).
>
> > > > There is no need for a fixed-clock compatible, nor for clock-frequency
> > > > and clock-output-names.
> > > >
> > > > But most important: this should be documented in the adv748x DT bindings,
> > > > and implemented in the adv748x driver.
> > >
> > > So if the driver is to export that clock for the kernel (like in this case),
> > > it must implement its support?
> >
> > Exactly.  Unless that pin is hardcoded to output a fixed clock, in which case
> > you can just override the existing audio_clk_c rate.
>
> Just to try it out (I'll set #clock-cells to 1), I registered a fixed rate
> clock in the driver, added a clock provider:
>
> adv748x_probe:
>
>     clk = clk_register_fixed_rate(state->dev,
>                                   "clk-hdmi-i2s-mclk",
>                                   NULL     /* parent_name */,
>                                   0        /* flags */,
>                                   12288000 /* rate */);
>     of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get, clk);
>
> And removed the audio_clk_c frequency setting. I also replaced the audio_clk_c
> in the list of input clocks of the R-Car-side sound card with the phandle of
> the adv7482 main node:
>
> salvator-common.dtsi:
>
>     &i2c4 {
>         status = "okay";
>
>         adv7482_hdmi_decoder: video-receiver@70 {
>             #clock-cells = <0>; // to be replaced with <1>
>         };
>     };
>
>     &rcar_sound {
>         clocks = ..., <&adv7482_hdmi_decoder>, ...;
>     };
>
> As everything continues to work as before, I assume that at least the clock
> dependencies were resolved.
>
> Is there a way to verify that the added input clock is actually used?
> IOW, if its frequency is actually has been programmed into the ssi4 (R-Car
> receiving hardware) registers, and not just a left-over from previuos attempts
> or plain default setting?

Have a look at /sys/kernel/debug/clk/clk_summary

> As the ADV748x devices seem to provide also the clocks for video outputs, will
> it make any sense to place the clock definition into the port node?
> Or should all provided clocks be indexed in the main device node?

Sorry, I don't know.

Gr{oetje,eeting}s,

                        Geert
Laurent Pinchart March 6, 2020, 1:16 p.m. UTC | #10
Hi Alex,

On Thu, Mar 05, 2020 at 03:36:28PM +0100, Alex Riesen wrote:
> Geert Uytterhoeven, Mon, Mar 02, 2020 17:13:30 +0100:
> > On Mon, Mar 2, 2020 at 5:09 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:
> > > >
> > > > The #clock-cells should be in the main video-receiver node.
> > > > Probably there is more than one clock output, so #clock-cells may be 1?
> > >
> > > AFAICS, the device can provide only this one clock line (audio master clock
> > > for I2S output)... I shall re-check, just in case.
> 
> And you're right, of course: the audio output formatting module of the ADV748x
> devices provides a set of clock lines related to the I2S pins: the already
> discussed master clock, left-right channel clock and the serial clock (bit
> clock?).

I don't think we need to model the last two clocks through CCF though,
they're part of the I2S protocol, not clock sources that need to be
explicitly controlled (or queried).

> > > > There is no need for a fixed-clock compatible, nor for clock-frequency
> > > > and clock-output-names.
> > > >
> > > > But most important: this should be documented in the adv748x DT bindings,
> > > > and implemented in the adv748x driver.
> > >
> > > So if the driver is to export that clock for the kernel (like in this case),
> > > it must implement its support?
> > 
> > Exactly.  Unless that pin is hardcoded to output a fixed clock, in which case
> > you can just override the existing audio_clk_c rate.
> 
> Just to try it out (I'll set #clock-cells to 1), I registered a fixed rate
> clock in the driver, added a clock provider:
> 
> adv748x_probe:
> 
>     clk = clk_register_fixed_rate(state->dev,
> 				  "clk-hdmi-i2s-mclk",
> 				  NULL     /* parent_name */,
> 				  0        /* flags */,
> 				  12288000 /* rate */);
>     of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get, clk);
> 
> And removed the audio_clk_c frequency setting. I also replaced the audio_clk_c
> in the list of input clocks of the R-Car-side sound card with the phandle of
> the adv7482 main node:
> 
> salvator-common.dtsi:
> 
>     &i2c4 {
> 	status = "okay";
> 
> 	adv7482_hdmi_decoder: video-receiver@70 {
> 	    #clock-cells = <0>; // to be replaced with <1>
> 	};
>     };
> 
>     &rcar_sound {
> 	clocks = ..., <&adv7482_hdmi_decoder>, ...;
>     };
> 
> As everything continues to work as before, I assume that at least the clock
> dependencies were resolved.

This looks good to me.

> Is there a way to verify that the added input clock is actually used?
> IOW, if its frequency is actually has been programmed into the ssi4 (R-Car
> receiving hardware) registers, and not just a left-over from previuos attempts
> or plain default setting?
> 
> As the ADV748x devices seem to provide also the clocks for video outputs, will
> it make any sense to place the clock definition into the port node?
> Or should all provided clocks be indexed in the main device node?

Those clocks are part of the CSI-2 protocol and also don't need to be
explicitly controlled. As far as I can tell from a quick check of the
ADV7482 documentation, only the I2S MCLK is a general-purpose clock that
needs to be exposed.
Alex Riesen March 6, 2020, 1:41 p.m. UTC | #11
Hi Laurent,

Laurent Pinchart, Fri, Mar 06, 2020 14:16:32 +0100:
> On Thu, Mar 05, 2020 at 03:36:28PM +0100, Alex Riesen wrote:
> > Geert Uytterhoeven, Mon, Mar 02, 2020 17:13:30 +0100:
> > > On Mon, Mar 2, 2020 at 5:09 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > > Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:
> > > > >
> > > > > The #clock-cells should be in the main video-receiver node.
> > > > > Probably there is more than one clock output, so #clock-cells may be 1?
> > > >
> > > > AFAICS, the device can provide only this one clock line (audio master clock
> > > > for I2S output)... I shall re-check, just in case.
> > 
> > And you're right, of course: the audio output formatting module of the ADV748x
> > devices provides a set of clock lines related to the I2S pins: the already
> > discussed master clock, left-right channel clock and the serial clock (bit
> > clock?).
> 
> I don't think we need to model the last two clocks through CCF though,
> they're part of the I2S protocol, not clock sources that need to be
> explicitly controlled (or queried).

That's good, because I'm right now having hard time finding out how to
calculate the frequencies!

> > Just to try it out (I'll set #clock-cells to 1), I registered a fixed rate
> > clock in the driver, added a clock provider:
> > 
> > adv748x_probe:
> > 
> >     clk = clk_register_fixed_rate(state->dev,
> > 				  "clk-hdmi-i2s-mclk",
> > 				  NULL     /* parent_name */,
> > 				  0        /* flags */,
> > 				  12288000 /* rate */);
> >     of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get, clk);
> > 
> > And removed the audio_clk_c frequency setting. I also replaced the audio_clk_c
> > in the list of input clocks of the R-Car-side sound card with the phandle of
> > the adv7482 main node:
> > 
> > salvator-common.dtsi:
> > 
> >     &i2c4 {
> > 	status = "okay";
> > 
> > 	adv7482_hdmi_decoder: video-receiver@70 {
> > 	    #clock-cells = <0>; // to be replaced with <1>
> > 	};
> >     };
> > 
> >     &rcar_sound {
> > 	clocks = ..., <&adv7482_hdmi_decoder>, ...;
> >     };
> > 
> > As everything continues to work as before, I assume that at least the clock
> > dependencies were resolved.
> 
> This looks good to me.

Ok, I settle on this than.

> > Is there a way to verify that the added input clock is actually used?
> > IOW, if its frequency is actually has been programmed into the ssi4 (R-Car
> > receiving hardware) registers, and not just a left-over from previuos attempts
> > or plain default setting?
> > 
> > As the ADV748x devices seem to provide also the clocks for video outputs, will
> > it make any sense to place the clock definition into the port node?
> > Or should all provided clocks be indexed in the main device node?
> 
> Those clocks are part of the CSI-2 protocol and also don't need to be
> explicitly controlled. As far as I can tell from a quick check of the
> ADV7482 documentation, only the I2S MCLK is a general-purpose clock that
> needs to be exposed.

Thanks, that's good to know!

Do you know, by chance, which of the snd_soc* callbacks should be used to
implement setting of the MCLK? The one in snd_soc_component_driver or
snd_soc_dai_driver->ops (snd_soc_dai_ops)?

Or how the userspace interface looks like? Or, if there is no userspace
interface for this, how the MCLK is supposed to be set? Through mclk-fs?

Regards,
Alex
Laurent Pinchart March 6, 2020, 1:45 p.m. UTC | #12
Hi Alex,

(CC'ing Morimoto-san)

On Fri, Mar 06, 2020 at 02:41:54PM +0100, Alex Riesen wrote:
> Laurent Pinchart, Fri, Mar 06, 2020 14:16:32 +0100:
> > On Thu, Mar 05, 2020 at 03:36:28PM +0100, Alex Riesen wrote:
> >> Geert Uytterhoeven, Mon, Mar 02, 2020 17:13:30 +0100:
> >>> On Mon, Mar 2, 2020 at 5:09 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> >>>> Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:
> >>>>>
> >>>>> The #clock-cells should be in the main video-receiver node.
> >>>>> Probably there is more than one clock output, so #clock-cells may be 1?
> >>>>
> >>>> AFAICS, the device can provide only this one clock line (audio master clock
> >>>> for I2S output)... I shall re-check, just in case.
> >> 
> >> And you're right, of course: the audio output formatting module of the ADV748x
> >> devices provides a set of clock lines related to the I2S pins: the already
> >> discussed master clock, left-right channel clock and the serial clock (bit
> >> clock?).
> > 
> > I don't think we need to model the last two clocks through CCF though,
> > they're part of the I2S protocol, not clock sources that need to be
> > explicitly controlled (or queried).
> 
> That's good, because I'm right now having hard time finding out how to
> calculate the frequencies!
> 
> >> Just to try it out (I'll set #clock-cells to 1), I registered a fixed rate
> >> clock in the driver, added a clock provider:
> >> 
> >> adv748x_probe:
> >> 
> >>     clk = clk_register_fixed_rate(state->dev,
> >> 				  "clk-hdmi-i2s-mclk",
> >> 				  NULL     /* parent_name */,
> >> 				  0        /* flags */,
> >> 				  12288000 /* rate */);
> >>     of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get, clk);
> >> 
> >> And removed the audio_clk_c frequency setting. I also replaced the audio_clk_c
> >> in the list of input clocks of the R-Car-side sound card with the phandle of
> >> the adv7482 main node:
> >> 
> >> salvator-common.dtsi:
> >> 
> >>     &i2c4 {
> >> 	status = "okay";
> >> 
> >> 	adv7482_hdmi_decoder: video-receiver@70 {
> >> 	    #clock-cells = <0>; // to be replaced with <1>
> >> 	};
> >>     };
> >> 
> >>     &rcar_sound {
> >> 	clocks = ..., <&adv7482_hdmi_decoder>, ...;
> >>     };
> >> 
> >> As everything continues to work as before, I assume that at least the clock
> >> dependencies were resolved.
> > 
> > This looks good to me.
> 
> Ok, I settle on this than.
> 
> >> Is there a way to verify that the added input clock is actually used?
> >> IOW, if its frequency is actually has been programmed into the ssi4 (R-Car
> >> receiving hardware) registers, and not just a left-over from previuos attempts
> >> or plain default setting?
> >> 
> >> As the ADV748x devices seem to provide also the clocks for video outputs, will
> >> it make any sense to place the clock definition into the port node?
> >> Or should all provided clocks be indexed in the main device node?
> > 
> > Those clocks are part of the CSI-2 protocol and also don't need to be
> > explicitly controlled. As far as I can tell from a quick check of the
> > ADV7482 documentation, only the I2S MCLK is a general-purpose clock that
> > needs to be exposed.
> 
> Thanks, that's good to know!
> 
> Do you know, by chance, which of the snd_soc* callbacks should be used to
> implement setting of the MCLK? The one in snd_soc_component_driver or
> snd_soc_dai_driver->ops (snd_soc_dai_ops)?
> 
> Or how the userspace interface looks like? Or, if there is no userspace
> interface for this, how the MCLK is supposed to be set? Through mclk-fs?

I'm afraid my knowledge of the sound subsystem is limited. Morimoto-san
is the main developer and maintainer of Renesas sound drivers.
Morimoto-sensei, would you have an answer to that question ? :-)
Kuninori Morimoto March 9, 2020, 1:31 a.m. UTC | #13
Hi

> > > Those clocks are part of the CSI-2 protocol and also don't need to be
> > > explicitly controlled. As far as I can tell from a quick check of the
> > > ADV7482 documentation, only the I2S MCLK is a general-purpose clock that
> > > needs to be exposed.
(snip)
> > Do you know, by chance, which of the snd_soc* callbacks should be used to
> > implement setting of the MCLK? The one in snd_soc_component_driver or
> > snd_soc_dai_driver->ops (snd_soc_dai_ops)?
> > 
> > Or how the userspace interface looks like? Or, if there is no userspace
> > interface for this, how the MCLK is supposed to be set? Through mclk-fs?
> 
> I'm afraid my knowledge of the sound subsystem is limited. Morimoto-san
> is the main developer and maintainer of Renesas sound drivers.
> Morimoto-sensei, would you have an answer to that question ? :-)

In my quick check, it goes to AUDIO_CLKC.
If so, you can update rcar_sound::clocks.

	&rcar_sound {
		...
-		/* update <audio_clk_b> to <cs2000> */
+		/* update <audio_clk_b> to <cs2000>,
+		 *        <audio_clk_c> to <adv748x> */
		clocks = <&cpg CPG_MOD 1005>,
			...
			 <&audio_clk_a>, <&cs2000>,
-			 <&audio_clk_c>,
+			 <&adv748x>,
			 <&cpg CPG_CORE CPG_AUDIO_CLK_I>;

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Alex Riesen March 9, 2020, 11:09 a.m. UTC | #14
Hi,

Kuninori Morimoto, Mon, Mar 09, 2020 02:31:01 +0100:
> > > > Those clocks are part of the CSI-2 protocol and also don't need to be
> > > > explicitly controlled. As far as I can tell from a quick check of the
> > > > ADV7482 documentation, only the I2S MCLK is a general-purpose clock that
> > > > needs to be exposed.
> (snip)
> > > Do you know, by chance, which of the snd_soc* callbacks should be used to
> > > implement setting of the MCLK? The one in snd_soc_component_driver or
> > > snd_soc_dai_driver->ops (snd_soc_dai_ops)?
> > > 
> > > Or how the userspace interface looks like? Or, if there is no userspace
> > > interface for this, how the MCLK is supposed to be set? Through mclk-fs?
> > 
> > I'm afraid my knowledge of the sound subsystem is limited. Morimoto-san
> > is the main developer and maintainer of Renesas sound drivers.
> > Morimoto-sensei, would you have an answer to that question ? :-)
> 
> In my quick check, it goes to AUDIO_CLKC.
> If so, you can update rcar_sound::clocks.
> 
> 	&rcar_sound {
> 		...
> -		/* update <audio_clk_b> to <cs2000> */
> +		/* update <audio_clk_b> to <cs2000>,
> +		 *        <audio_clk_c> to <adv748x> */
> 		clocks = <&cpg CPG_MOD 1005>,
> 			...
> 			 <&audio_clk_a>, <&cs2000>,
> -			 <&audio_clk_c>,
> +			 <&adv748x>,
> 			 <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
> 
> Thank you for your help !!

Thanks.

Should the adv748x driver also implement anything to configure the frequency
of MCLK clock? I mean something like .set_sysclk and .set_fmt callbacks of
snd_soc_dai_ops?

Or is the driver implementation, which depends on mclk-fs to be 256, the audio
stream format to be 8x S24_LE, and requires strictly 48kHz sampling rate on
the HDMI input, a totally acceptable first attempt at writing a DAI driver?

I'm a bit bothered by that, as the hardware is also capable of decoding
stereo, sampling rate 32-192kHz, a variety of PCM and compressed/encrypted
formats, 128-768fs MCLK multipliers, and a row of I2S options.

I just find it confusing to place the configuration interfaces.
For instance, the patches use the media ioctl for audio output selection to
select I2S protocol. While works, it does not feel right (shouldn't it be in
the device tree?)

Maybe you can point me at a driver doing something similar? I'm studying media
drivers now, but not many of them use ASoC interfaces for devices providing a
clock. Or maybe I should better look at sound/soc/...?

Thanks in advance,
Alex
Kuninori Morimoto March 10, 2020, 1:07 a.m. UTC | #15
Hi Alex

> Should the adv748x driver also implement anything to configure the frequency
> of MCLK clock? I mean something like .set_sysclk and .set_fmt callbacks of
> snd_soc_dai_ops?
> 
> Or is the driver implementation, which depends on mclk-fs to be 256, the audio
> stream format to be 8x S24_LE, and requires strictly 48kHz sampling rate on
> the HDMI input, a totally acceptable first attempt at writing a DAI driver?
> 
> I'm a bit bothered by that, as the hardware is also capable of decoding
> stereo, sampling rate 32-192kHz, a variety of PCM and compressed/encrypted
> formats, 128-768fs MCLK multipliers, and a row of I2S options.
> 
> I just find it confusing to place the configuration interfaces.
> For instance, the patches use the media ioctl for audio output selection to
> select I2S protocol. While works, it does not feel right (shouldn't it be in
> the device tree?)
> 
> Maybe you can point me at a driver doing something similar? I'm studying media
> drivers now, but not many of them use ASoC interfaces for devices providing a
> clock. Or maybe I should better look at sound/soc/...?

Setting Sound Clock for all cases/patterns are very complex and difficult actually.
(ADV7482 configuration) x (ADG divider / selector) x etc, etc...

Thus, Current R-Car sound is assuming that audio_clk_a/b/c/i are providing
route clock (= no configuration, fixed clock), and ADG divides it,
and provide best clock to each SSIx.

Current Salvator/ULCB already have 44.1/48kHz route clock (= CS2000 and Audio_CLK_A),
and we can reuse it for all SSIx. Thus, ADV7482 clock is not necessary, I guess ?
Or providing specific clock for some case is enough
(ADG will automatically select it if necessary).

If ADV7482 needs more detail clock settings combination,
then, there is no method to adjust to it.
We need to consider such system somehow.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Alex Riesen March 10, 2020, 8:17 a.m. UTC | #16
Hello Morimoto-san,

Kuninori Morimoto, Tue, Mar 10, 2020 02:07:23 +0100:
> > Should the adv748x driver also implement anything to configure the frequency
> > of MCLK clock? I mean something like .set_sysclk and .set_fmt callbacks of
> > snd_soc_dai_ops?
> > 
> > Or is the driver implementation, which depends on mclk-fs to be 256, the audio
> > stream format to be 8x S24_LE, and requires strictly 48kHz sampling rate on
> > the HDMI input, a totally acceptable first attempt at writing a DAI driver?
> > 
> > I'm a bit bothered by that, as the hardware is also capable of decoding
> > stereo, sampling rate 32-192kHz, a variety of PCM and compressed/encrypted
> > formats, 128-768fs MCLK multipliers, and a row of I2S options.
> > 
> > I just find it confusing to place the configuration interfaces.
> > For instance, the patches use the media ioctl for audio output selection to
> > select I2S protocol. While works, it does not feel right (shouldn't it be in
> > the device tree?)
> > 
> > Maybe you can point me at a driver doing something similar? I'm studying media
> > drivers now, but not many of them use ASoC interfaces for devices providing a
> > clock. Or maybe I should better look at sound/soc/...?
> 
> Setting Sound Clock for all cases/patterns are very complex and difficult actually.
> (ADV7482 configuration) x (ADG divider / selector) x etc, etc...
> 
> Thus, Current R-Car sound is assuming that audio_clk_a/b/c/i are providing
> route clock (= no configuration, fixed clock), and ADG divides it,
> and provide best clock to each SSIx.
> Current Salvator/ULCB already have 44.1/48kHz route clock (= CS2000 and Audio_CLK_A),
> and we can reuse it for all SSIx. Thus, ADV7482 clock is not necessary, I guess ?
> Or providing specific clock for some case is enough
> (ADG will automatically select it if necessary).

In this particular case, the ADV7482 *must* provide the clock, I believe: it
extracts the audio stream from the HDMI connection (in addition to everything
else) and serves the stream on I2S. Its MCLK line is physically connected to
the CLK_C line (which is an input) of the R-Car SoC. The I2S audio
transmission does not work if the ADV7482 clock is not programmed (or
programmed incorrectly).
Yes, I tried (I also tried programming it incorrectly, just because I didn't
know what I was doing).

> If ADV7482 needs more detail clock settings combination,
> then, there is no method to adjust to it.
> We need to consider such system somehow.

Not encouraging...

Maybe I should leave the clock fixed, with the frequency configuration in the
device tree, e.g. as adv7482 port node property "clock-frequency".
Which feels rather pathetic, but at least serves my purpose (48k, 8x24).

But let me describe the situation as I see it first.

As far as I understand, the SSI4 (Salvator-X board) should be programmed by
the snd-soc-rcar driver in the "slave receiver" mode for this use case, which
is HDMI input ADV7482 (I2S master, TDM) -> SSI4 (I2S slave)):

[   63.305990] asoc_simple_card_parse_clk: asoc-audio-graph-card sound: rsnd-dai.1 : sysclk = 66666664, direction 0
[   63.306028] asoc_simple_card_parse_clk: asoc-audio-graph-card sound: adv748x-i2s : sysclk = 12288000, direction 1

I am a bit bothered by the fact that sysclk of rsnd-dai.1 does not match that
sysclk of adv7482-i2s, but I think it's just DT node configuration.

[   63.306033] asoc_simple_card_set_dailink_name: asoc-audio-graph-card sound: name : rsnd-dai.1-adv748x-i2s
...
[   63.332641] asoc-audio-graph-card sound: adv748x.4-0070 <-> rsnd-dai.1 mapping ok
...
[   63.341317] dapm_connect_dai_link_widgets:  rsnd-dai.1-adv748x-i2s: connected DAI link adv748x.4-0070:Capture -> ec500000.sound:DAI1 Capture
...
[  128.961389] rsnd_write: rcar_sound ec500000.sound: w ssi[4] - SSICR ( 124) : 9ceb0100

Decoding this last line (9ceb0100) gives SSICR.TRMD (bit1) =0, SSICR.SCKD
(bit15) =0, SSICR.SWSD (bit14) =0. The combination is documented as "slave
receiver". Which, I assume, makes SSI4 use the external clock. Given the
received stream looks ok, something also must have set the dividers correctly.

From the above, I conclude, whatever the complexity of the audio system clock
configuration, it seems to be implemented for the case.

I only miss a more or less clear way to configure the I2S master (ADV7482, that is).

Regards,
Alex
Laurent Pinchart March 10, 2020, 10:39 a.m. UTC | #17
Hi Alex,

On Tue, Mar 10, 2020 at 09:17:14AM +0100, Alex Riesen wrote:
> Kuninori Morimoto, Tue, Mar 10, 2020 02:07:23 +0100:
> > > Should the adv748x driver also implement anything to configure the frequency
> > > of MCLK clock? I mean something like .set_sysclk and .set_fmt callbacks of
> > > snd_soc_dai_ops?
> > > 
> > > Or is the driver implementation, which depends on mclk-fs to be 256, the audio
> > > stream format to be 8x S24_LE, and requires strictly 48kHz sampling rate on
> > > the HDMI input, a totally acceptable first attempt at writing a DAI driver?
> > > 
> > > I'm a bit bothered by that, as the hardware is also capable of decoding
> > > stereo, sampling rate 32-192kHz, a variety of PCM and compressed/encrypted
> > > formats, 128-768fs MCLK multipliers, and a row of I2S options.
> > > 
> > > I just find it confusing to place the configuration interfaces.
> > > For instance, the patches use the media ioctl for audio output selection to
> > > select I2S protocol. While works, it does not feel right (shouldn't it be in
> > > the device tree?)
> > > 
> > > Maybe you can point me at a driver doing something similar? I'm studying media
> > > drivers now, but not many of them use ASoC interfaces for devices providing a
> > > clock. Or maybe I should better look at sound/soc/...?
> > 
> > Setting Sound Clock for all cases/patterns are very complex and difficult actually.
> > (ADV7482 configuration) x (ADG divider / selector) x etc, etc...
> > 
> > Thus, Current R-Car sound is assuming that audio_clk_a/b/c/i are providing
> > route clock (= no configuration, fixed clock), and ADG divides it,
> > and provide best clock to each SSIx.
> > Current Salvator/ULCB already have 44.1/48kHz route clock (= CS2000 and Audio_CLK_A),
> > and we can reuse it for all SSIx. Thus, ADV7482 clock is not necessary, I guess ?
> > Or providing specific clock for some case is enough
> > (ADG will automatically select it if necessary).
> 
> In this particular case, the ADV7482 *must* provide the clock, I believe: it
> extracts the audio stream from the HDMI connection (in addition to everything
> else) and serves the stream on I2S. Its MCLK line is physically connected to
> the CLK_C line (which is an input) of the R-Car SoC. The I2S audio
> transmission does not work if the ADV7482 clock is not programmed (or
> programmed incorrectly).
> Yes, I tried (I also tried programming it incorrectly, just because I didn't
> know what I was doing).
> 
> > If ADV7482 needs more detail clock settings combination,
> > then, there is no method to adjust to it.
> > We need to consider such system somehow.
> 
> Not encouraging...
> 
> Maybe I should leave the clock fixed, with the frequency configuration in the
> device tree, e.g. as adv7482 port node property "clock-frequency".
> Which feels rather pathetic, but at least serves my purpose (48k, 8x24).
> 
> But let me describe the situation as I see it first.
> 
> As far as I understand, the SSI4 (Salvator-X board) should be programmed by
> the snd-soc-rcar driver in the "slave receiver" mode for this use case, which
> is HDMI input ADV7482 (I2S master, TDM) -> SSI4 (I2S slave)):
> 
> [   63.305990] asoc_simple_card_parse_clk: asoc-audio-graph-card sound: rsnd-dai.1 : sysclk = 66666664, direction 0
> [   63.306028] asoc_simple_card_parse_clk: asoc-audio-graph-card sound: adv748x-i2s : sysclk = 12288000, direction 1
> 
> I am a bit bothered by the fact that sysclk of rsnd-dai.1 does not match that
> sysclk of adv7482-i2s, but I think it's just DT node configuration.
> 
> [   63.306033] asoc_simple_card_set_dailink_name: asoc-audio-graph-card sound: name : rsnd-dai.1-adv748x-i2s
> ...
> [   63.332641] asoc-audio-graph-card sound: adv748x.4-0070 <-> rsnd-dai.1 mapping ok
> ...
> [   63.341317] dapm_connect_dai_link_widgets:  rsnd-dai.1-adv748x-i2s: connected DAI link adv748x.4-0070:Capture -> ec500000.sound:DAI1 Capture
> ...
> [  128.961389] rsnd_write: rcar_sound ec500000.sound: w ssi[4] - SSICR ( 124) : 9ceb0100
> 
> Decoding this last line (9ceb0100) gives SSICR.TRMD (bit1) =0, SSICR.SCKD
> (bit15) =0, SSICR.SWSD (bit14) =0. The combination is documented as "slave
> receiver". Which, I assume, makes SSI4 use the external clock. Given the
> received stream looks ok, something also must have set the dividers correctly.
> 
> From the above, I conclude, whatever the complexity of the audio system clock
> configuration, it seems to be implemented for the case.
> 
> I only miss a more or less clear way to configure the I2S master (ADV7482, that is).

As a stop-gap measure, until the sound driver programs the clock, you
can set its frequency in DT with the assigned-clock-rates property.

Patch
diff mbox series

diff --git a/arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dts
index c72968623e94..10f74f7a0efe 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dts
+++ b/arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dts
@@ -136,9 +136,29 @@ 
 				playback = <&ssi3>;
 			};
 		};
+		rsnd_port3: port@3 {
+			reg = <3>;
+			rsnd_endpoint3: endpoint {
+				remote-endpoint = <&adv7482_i2s>;
+
+				dai-tdm-slot-num = <8>;
+				dai-tdm-slot-width = <32>;
+				dai-format = "left_j";
+				mclk-fs = <256>;
+				bitclock-master = <&adv7482_i2s>;
+				frame-master = <&adv7482_i2s>;
+				system-clock-direction-out;
+
+				capture = <&ssi4>;
+			};
+		};
 	};
 };
 
+&adv7482_i2s {
+	remote-endpoint = <&rsnd_endpoint3>;
+};
+
 &sata {
 	status = "okay";
 };
@@ -146,9 +166,11 @@ 
 &sound_card {
 	dais = <&rsnd_port0	/* ak4613 */
 		&rsnd_port1	/* HDMI0  */
-		&rsnd_port2>;	/* HDMI1  */
+		&rsnd_port2	/* HDMI1  */
+		&rsnd_port3>;	/* adv7482 hdmi-in  */
 };
 
+
 &usb2_phy2 {
 	pinctrl-0 = <&usb2_pins>;
 	pinctrl-names = "default";
diff --git a/arch/arm64/boot/dts/renesas/salvator-common.dtsi b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
index 21e01056e759..e887805b16fc 100644
--- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
+++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
@@ -322,6 +322,10 @@ 
 	clock-frequency = <22579200>;
 };
 
+&audio_clk_c {
+	clock-frequency = <12288000>;
+};
+
 &avb {
 	pinctrl-0 = <&avb_pins>;
 	pinctrl-names = "default";
@@ -471,12 +475,14 @@ 
 
 		#address-cells = <1>;
 		#size-cells = <0>;
+		#sound-dai-cells = <0>;
 
 		interrupt-parent = <&gpio6>;
 		interrupt-names = "intrq1", "intrq2";
 		interrupts = <30 IRQ_TYPE_LEVEL_LOW>,
 			     <31 IRQ_TYPE_LEVEL_LOW>;
-
+		clocks = <&rcar_sound 3>, <&audio_clk_c>;
+		clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";
 		port@7 {
 			reg = <7>;
 
@@ -512,6 +518,14 @@ 
 				remote-endpoint = <&csi20_in>;
 			};
 		};
+
+		port@c {
+			reg = <12>;
+
+			adv7482_i2s: endpoint {
+				/* remote-endpoint defined in the board file */
+			};
+		};
 	};
 
 	csa_vdd: adc@7c {
@@ -686,7 +700,8 @@ 
 	};
 
 	sound_pins: sound {
-		groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
+		groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
+			 "ssi4_data";
 		function = "ssi";
 	};
 
@@ -735,8 +750,8 @@ 
 	pinctrl-0 = <&sound_pins &sound_clk_pins>;
 	pinctrl-names = "default";
 
-	/* Single DAI */
-	#sound-dai-cells = <0>;
+	/* multi DAI */
+	#sound-dai-cells = <1>;
 
 	/* audio_clkout0/1/2/3 */
 	#clock-cells = <1>;
@@ -760,8 +775,18 @@ 
 		 <&cpg CPG_MOD 1020>, <&cpg CPG_MOD 1021>,
 		 <&cpg CPG_MOD 1019>, <&cpg CPG_MOD 1018>,
 		 <&audio_clk_a>, <&cs2000>,
-		 <&audio_clk_c>,
 		 <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
+	clock-names = "ssi-all",
+		      "ssi.9", "ssi.8", "ssi.7", "ssi.6",
+		      "ssi.5", "ssi.4", "ssi.3", "ssi.2",
+		      "ssi.1", "ssi.0",
+		      "src.9", "src.8", "src.7", "src.6",
+		      "src.5", "src.4", "src.3", "src.2",
+		      "src.1", "src.0",
+		      "mix.1", "mix.0",
+		      "ctu.1", "ctu.0",
+		      "dvc.0", "dvc.1",
+		      "clk_a", "clk_b", "clk_i";
 
 	ports {
 		#address-cells = <1>;