| Message ID | 20200113141556.GI3606@pflmari (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | media: i2c: adv748x: add support for HDMI audio | expand |
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
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
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
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
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
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
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
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
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
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.
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
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 ? :-)
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
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
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
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
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.
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>;
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(-)