by Maxime Ripard

[permalink] [raw]

Subject: Re: [linux-sunxi] [PATCH v5 09/20] arm64: dts: allwinner: h6: Add DAI node and soundcard for HDMI

On Fri, Oct 02, 2020 at 06:01:21PM +0200, Cl?ment P?ron wrote:
> Hi Chen-Yu,
>
> On Mon, 28 Sep 2020 at 07:42, Chen-Yu Tsai <[email protected]> wrote:
> >
> > On Mon, Sep 28, 2020 at 1:32 PM Chen-Yu Tsai <[email protected]> wrote:
> > >
> > > On Mon, Sep 28, 2020 at 3:29 AM Cl?ment P?ron <[email protected]> wrote:
> > > >
> > > > From: Jernej Skrabec <[email protected]>
> > > >
> > > > Add the I2S node used by the HDMI and a simple-soundcard to
> > > > link audio between HDMI and I2S.
> > > >
> > > > Note that the HDMI codec requires an inverted frame clock and
> > > > a fixed I2S width. As there is no such option for I2S we use
> > > > TDM property of the simple-soundcard to do that.
> > > >
> > > > Signed-off-by: Jernej Skrabec <[email protected]>
> > > > Signed-off-by: Marcus Cooper <[email protected]>
> > > > Signed-off-by: Cl?ment P?ron <[email protected]>
> > > > ---
> > > > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 33 ++++++++++++++++++++
> > > > 1 file changed, 33 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > > index 28c77d6872f6..a8853ee7885a 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > > @@ -67,6 +67,25 @@ de: display-engine {
> > > > status = "disabled";
> > > > };
> > > >
> > > > + hdmi_sound: hdmi-sound {
> > > > + compatible = "simple-audio-card";
> > > > + simple-audio-card,format = "i2s";
> > > > + simple-audio-card,name = "sun50i-h6-hdmi";
> > > > + simple-audio-card,mclk-fs = <128>;
> > > > + simple-audio-card,frame-inversion;
> > > > + status = "disabled";
> > > > +
> > > > + simple-audio-card,codec {
> > > > + sound-dai = <&hdmi>;
> > > > + };
> > > > +
> > > > + simple-audio-card,cpu {
> > > > + sound-dai = <&i2s1>;
> > > > + dai-tdm-slot-num = <2>;
> > >
> > > Doesn't this end up limiting the number of audio channels HDMI can carry?
> > > AFAICT the TDM properties are all optional, so just leave it out.
> > >
> > > Same goes for the other two patches.
> > >
> > > > + dai-tdm-slot-width = <32>;
> > > > + };
> > > > + };
> > > > +
> > > > osc24M: osc24M_clk {
> > > > #clock-cells = <0>;
> > > > compatible = "fixed-clock";
> > > > @@ -609,6 +628,19 @@ mdio: mdio {
> > > > };
> > > > };
> > > >
> > > > + i2s1: i2s@5091000 {
> > > > + #sound-dai-cells = <0>;
> > > > + compatible = "allwinner,sun50i-h6-i2s";
> > > > + reg = <0x05091000 0x1000>;
> > > > + interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
> > > > + clocks = <&ccu CLK_BUS_I2S1>, <&ccu CLK_I2S1>;
> > > > + clock-names = "apb", "mod";
> > > > + dmas = <&dma 4>, <&dma 4>;
> > > > + resets = <&ccu RST_BUS_I2S1>;
> > > > + dma-names = "rx", "tx";
> >
> > Sorry, missed this one.
> >
> > Given that usage for this interface is transmit only, and there is no
> > RX DRQ number assigned to it, you should drop the RX DMA number and name.
>
> Indeed if there is no DRQ number assigned we shouldn't have it in the
> device-tree
>
> but Samuel told me that the `make dtbs_check` reports:
>
> i2s@1c22800: dma-names:0: 'rx' was expected
> i2s@1c22800: dma-names: ['tx'] is too short
> i2s@1c22800: dmas: [[28, 27]] is too short
>
> Should I fix the YAML so?

Yep :)

Maxime

Attachments:

(No filename) (3.85 kB)
signature.asc (235.00 B)
Download all attachments

2020-10-03 09:34:35

by Clément Péron

[permalink] [raw]

Subject: Re: [linux-sunxi] [PATCH v5 09/20] arm64: dts: allwinner: h6: Add DAI node and soundcard for HDMI

Hi,

On Fri, 2 Oct 2020 at 18:24, Maxime Ripard <[email protected]> wrote:
>
> On Fri, Oct 02, 2020 at 06:01:21PM +0200, Clément Péron wrote:
> > Hi Chen-Yu,
> >
> > On Mon, 28 Sep 2020 at 07:42, Chen-Yu Tsai <[email protected]> wrote:
> > >
> > > On Mon, Sep 28, 2020 at 1:32 PM Chen-Yu Tsai <[email protected]> wrote:
> > > >
> > > > On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <[email protected]> wrote:
> > > > >
> > > > > From: Jernej Skrabec <[email protected]>
> > > > >
> > > > > Add the I2S node used by the HDMI and a simple-soundcard to
> > > > > link audio between HDMI and I2S.
> > > > >
> > > > > Note that the HDMI codec requires an inverted frame clock and
> > > > > a fixed I2S width. As there is no such option for I2S we use
> > > > > TDM property of the simple-soundcard to do that.
> > > > >
> > > > > Signed-off-by: Jernej Skrabec <[email protected]>
> > > > > Signed-off-by: Marcus Cooper <[email protected]>
> > > > > Signed-off-by: Clément Péron <[email protected]>
> > > > > ---
> > > > > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 33 ++++++++++++++++++++
> > > > > 1 file changed, 33 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > > > index 28c77d6872f6..a8853ee7885a 100644
> > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > > > @@ -67,6 +67,25 @@ de: display-engine {
> > > > > status = "disabled";
> > > > > };
> > > > >
> > > > > + hdmi_sound: hdmi-sound {
> > > > > + compatible = "simple-audio-card";
> > > > > + simple-audio-card,format = "i2s";
> > > > > + simple-audio-card,name = "sun50i-h6-hdmi";
> > > > > + simple-audio-card,mclk-fs = <128>;
> > > > > + simple-audio-card,frame-inversion;
> > > > > + status = "disabled";
> > > > > +
> > > > > + simple-audio-card,codec {
> > > > > + sound-dai = <&hdmi>;
> > > > > + };
> > > > > +
> > > > > + simple-audio-card,cpu {
> > > > > + sound-dai = <&i2s1>;
> > > > > + dai-tdm-slot-num = <2>;
> > > >
> > > > Doesn't this end up limiting the number of audio channels HDMI can carry?
> > > > AFAICT the TDM properties are all optional, so just leave it out.
> > > >
> > > > Same goes for the other two patches.
> > > >
> > > > > + dai-tdm-slot-width = <32>;
> > > > > + };
> > > > > + };
> > > > > +
> > > > > osc24M: osc24M_clk {
> > > > > #clock-cells = <0>;
> > > > > compatible = "fixed-clock";
> > > > > @@ -609,6 +628,19 @@ mdio: mdio {
> > > > > };
> > > > > };
> > > > >
> > > > > + i2s1: i2s@5091000 {
> > > > > + #sound-dai-cells = <0>;
> > > > > + compatible = "allwinner,sun50i-h6-i2s";
> > > > > + reg = <0x05091000 0x1000>;
> > > > > + interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
> > > > > + clocks = <&ccu CLK_BUS_I2S1>, <&ccu CLK_I2S1>;
> > > > > + clock-names = "apb", "mod";
> > > > > + dmas = <&dma 4>, <&dma 4>;
> > > > > + resets = <&ccu RST_BUS_I2S1>;
> > > > > + dma-names = "rx", "tx";
> > >
> > > Sorry, missed this one.
> > >
> > > Given that usage for this interface is transmt only, and there is no
> > > RX DRQ number assigned to it, you should drop the RX DMA number and name.
> >
> > Indeed if there is no DRQ number assigned we shouldn't have it in the
> > device-tree

After reviewing the H6 User Manual it seems to be assigned :
page 269 3.10.3.3 DRQ TYPE
port 4 => I2S/PCM1-RX / I2S/PCM1-TX
Same for A64
But indeed it's not mapped for H5.

I will fix the yaml for H3/H5 but not for A64/H6.

Regards,
Clement

> >
> > but Samuel told me that the `make dtbs_check` reports:
> >
> > i2s@1c22800: dma-names:0: 'rx' was expected
> > i2s@1c22800: dma-names: ['tx'] is too short
> > i2s@1c22800: dmas: [[28, 27]] is too short
> >
> > Should I fix the YAML so?
>
> Yep :)
>
> Maxime