| Message ID | 20200927192912.46323-1-peron.clem@gmail.com |
|---|---|
| Headers | show |
| Series | Add Allwinner H3/H5/H6/A64 HDMI audio | expand |
On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@gmail.com> wrote: > > We are actually using a complex formula to just return a bunch of > simple values. Also this formula is wrong for sun4i when calling > get_wss() the function return 4 instead of 3. > > Replace this with a simpler switch case. > > Also drop the i2s params which is unused and return a simple int as > returning an error code could be out of range for an s8 and there is > no optim to return a s8 here. > > Signed-off-by: Clément Péron <peron.clem@gmail.com> Fixes: 619c15f7fac9 ("ASoC: sun4i-i2s: Change SR and WSS computation") Reviewed-by: Chen-Yu Tsai <wens@csie.org>
On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@gmail.com> wrote: > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > H6 I2S is very similar to that in H3, except it supports up to 16 > channels. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- > sound/soc/sunxi/sun4i-i2s.c | 224 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 224 insertions(+) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index f23ff29e7c1d..2baf6c276280 100644 > --- a/sound/soc/sunxi/sun4i-i2s.c > +++ b/sound/soc/sunxi/sun4i-i2s.c > @@ -124,6 +124,21 @@ > #define SUN8I_I2S_RX_CHAN_SEL_REG 0x54 > #define SUN8I_I2S_RX_CHAN_MAP_REG 0x58 > > +/* Defines required for sun50i-h6 support */ > +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK GENMASK(21, 20) > +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset) ((offset) << 20) > +#define SUN50I_H6_I2S_TX_CHAN_SEL_MASK GENMASK(19, 16) > +#define SUN50I_H6_I2S_TX_CHAN_SEL(chan) ((chan - 1) << 16) > +#define SUN50I_H6_I2S_TX_CHAN_EN_MASK GENMASK(15, 0) > +#define SUN50I_H6_I2S_TX_CHAN_EN(num_chan) (((1 << num_chan) - 1)) > + > +#define SUN50I_H6_I2S_TX_CHAN_MAP0_REG 0x44 > +#define SUN50I_H6_I2S_TX_CHAN_MAP1_REG 0x48 > + > +#define SUN50I_H6_I2S_RX_CHAN_SEL_REG 0x64 > +#define SUN50I_H6_I2S_RX_CHAN_MAP0_REG 0x68 > +#define SUN50I_H6_I2S_RX_CHAN_MAP1_REG 0x6C > + > struct sun4i_i2s; > > /** > @@ -474,6 +489,62 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > return 0; > } > > +static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > + const struct snd_pcm_hw_params *params) > +{ > + unsigned int channels = params_channels(params); > + unsigned int slots = channels; > + unsigned int lrck_period; > + > + if (i2s->slots) > + slots = i2s->slots; > + > + /* Map the channels for playback and capture */ > + regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210); > + regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 0x76543210); Nit, since it supports up to 16 channels, you might want to map all 16 of them now, instead of having to come back and fix it later. Code wise, this patch is Reviewed-by: Chen-Yu Tsai <wens@csie.org> I don't have a scope nor logic analyzer, so I wasn't able to participate in the LRCK discussion.
On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@gmail.com> wrote: > > As slots and slot_width can be overwritter in case set_tdm() is > called. Avoid to have this logic in set_chan_cfg(). It doesn't seem that set_tdm_slot() would get called concurrently with hw_params(), at least not for the simple-card family. If so then we'd have more problems like mismatched slots/slot-width due to no serialization when interacting with those values. > Instead pass the required values as params to set_chan_cfg(). > > This will also avoid a bug when we will enable 20/24bits support, > i2s->slot_width is not actually used in the lrck_period computation. > > Suggested-by: Samuel Holland <samuel@sholland.org> > Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- > sound/soc/sunxi/sun4i-i2s.c | 36 ++++++++++++++---------------------- > 1 file changed, 14 insertions(+), 22 deletions(-) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index 2baf6c276280..0633b9fba3d7 100644 > --- a/sound/soc/sunxi/sun4i-i2s.c > +++ b/sound/soc/sunxi/sun4i-i2s.c > @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks { > unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *); > s8 (*get_sr)(const struct sun4i_i2s *, int); > s8 (*get_wss)(const struct sun4i_i2s *, int); > - int (*set_chan_cfg)(const struct sun4i_i2s *, > - const struct snd_pcm_hw_params *); > + int (*set_chan_cfg)(const struct sun4i_i2s *i2s, > + unsigned int channels, unsigned int slots, > + unsigned int slot_width); > int (*set_fmt)(const struct sun4i_i2s *, unsigned int); > }; > > @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width) > } > > static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > - const struct snd_pcm_hw_params *params) > + unsigned int channels, unsigned int slots, > + unsigned int slot_width) > { > - unsigned int channels = params_channels(params); > - > /* Map the channels for playback and capture */ > regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210); > regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x00003210); > @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > } > > static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > - const struct snd_pcm_hw_params *params) > + unsigned int channels, unsigned int slots, > + unsigned int slot_width) > { > - unsigned int channels = params_channels(params); > - unsigned int slots = channels; > unsigned int lrck_period; > > - if (i2s->slots) > - slots = i2s->slots; > - Based on the bug you highlighted in your expanded commit log, the simpler fix would be to look at i2s->slot_width in addition to i2s->slots, and replace params_physical_width(params) accordingly. Also, I would put fixes (even preparatory ones) before patches that introduce support for new features and hardware. That makes them easier to backport if needed. ChenYu > /* Map the channels for playback and capture */ > regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210); > regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210); > @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > case SND_SOC_DAIFMT_DSP_B: > case SND_SOC_DAIFMT_LEFT_J: > case SND_SOC_DAIFMT_RIGHT_J: > - lrck_period = params_physical_width(params) * slots; > + lrck_period = slot_width * slots; > break; > > case SND_SOC_DAIFMT_I2S: > - lrck_period = params_physical_width(params); > + lrck_period = slot_width; > break; > > default:
On Mon, Sep 28, 2020 at 12:37 PM Chen-Yu Tsai <wens@csie.org> wrote: > > On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@gmail.com> wrote: > > > > We are actually using a complex formula to just return a bunch of > > simple values. Also this formula is wrong for sun4i when calling BTW, it is entirely possible that the compiler optimizes your switch-case back into the original complex formula you replaced. :) > > get_wss() the function return 4 instead of 3. > > > > Replace this with a simpler switch case. > > > > Also drop the i2s params which is unused and return a simple int as > > returning an error code could be out of range for an s8 and there is > > no optim to return a s8 here. > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > Fixes: 619c15f7fac9 ("ASoC: sun4i-i2s: Change SR and WSS computation") > > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@gmail.com> wrote: > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > 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 <jernej.skrabec@siol.net> > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- > 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"; > + status = "disabled"; > + }; > + > spdif: spdif@5093000 { > #sound-dai-cells = <0>; > compatible = "allwinner,sun50i-h6-spdif"; > @@ -739,6 +771,7 @@ ohci3: usb@5311400 { > }; > > hdmi: hdmi@6000000 { > + #sound-dai-cells = <0>; > compatible = "allwinner,sun50i-h6-dw-hdmi"; > reg = <0x06000000 0x10000>; > reg-io-width = <1>; The rest of the patch looks OK.
On Mon, Sep 28, 2020 at 1:32 PM Chen-Yu Tsai <wens@csie.org> wrote: > > On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@gmail.com> wrote: > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > 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 <jernej.skrabec@siol.net> > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > 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. > > + status = "disabled"; > > + }; > > + > > spdif: spdif@5093000 { > > #sound-dai-cells = <0>; > > compatible = "allwinner,sun50i-h6-spdif"; > > @@ -739,6 +771,7 @@ ohci3: usb@5311400 { > > }; > > > > hdmi: hdmi@6000000 { > > + #sound-dai-cells = <0>; > > compatible = "allwinner,sun50i-h6-dw-hdmi"; > > reg = <0x06000000 0x10000>; > > reg-io-width = <1>; > > The rest of the patch looks OK.
On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@gmail.com> wrote: > > From: Marcus Cooper <codekipper@gmail.com> > > On the newer SoCs such as the H3 and A64 this is set by default > to transfer a 0 after each sample in each slot. However the A10 > and A20 SoCs that this driver was developed on had a default > setting where it padded the audio gain with zeros. > > This isn't a problem while we have only support for 16bit audio > but with larger sample resolution rates in the pipeline then SEXT > bits should be cleared so that they also pad at the LSB. Without > this the audio gets distorted. > > Set sign extend sample for all the sunxi generations even if they > are not affected. This will keep consistency and avoid relying on > default. > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > Signed-off-by: Clément Péron <peron.clem@gmail.com> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@gmail.com> wrote: > > Hi, > > This is exactly the same as v4 but with more details in some commit log > and also device-tree soundcard and DAI node have been merged. > > Regards, > Clement > > Change since v4; > - add more comment on get_wss() and set_channel_cfg() patch > - merge soundcard and DAI HDMI patches > > Change since v3: > - add Samuel Holland patch to reconfigure FIFO_TX_REG when suspend is enabled > - readd inversion to H6 LRCK sun50i_h6_i2s_set_soc_fmt() > - Fix get_wss() for sun4i > - Add a commit to fix checkpatch warning > > Change since v2: > - rebase on next-20200918 > - drop revert LRCK polarity patch > - readd simple-audio-card,frame-inversion in dts > - Add patch for changing set_chan_cfg params > > Change since v1: > - rebase on next-20200828 > - add revert LRCK polarity > - remove all simple-audio-card,frame-inversion in dts > - add Ondrej patches for Orange Pi board > - Add arm64 defconfig patch > > Clément Péron (6): > ASoC: sun4i-i2s: Change set_chan_cfg() params > ASoC: sun4i-i2s: Change get_sr() and get_wss() to be more explicit > ASoC: sun4i-i2s: Fix sun8i volatile regs > arm64: dts: allwinner: h6: Enable HDMI sound for Beelink GS1 > arm64: defconfig: Enable Allwinner i2s driver > ASoC: sun4i-i2s: fix coding-style for callback definition > > Jernej Skrabec (3): > ASoC: sun4i-i2s: Add support for H6 I2S > dt-bindings: ASoC: sun4i-i2s: Add H6 compatible > arm64: dts: allwinner: h6: Add DAI node and soundcard for HDMI > > Marcus Cooper (7): > ASoC: sun4i-i2s: Set sign extend sample > ASoc: sun4i-i2s: Add 20 and 24 bit support > arm: dts: sunxi: h3/h5: Add DAI node and soundcard for HDMI > arm64: dts: allwinner: a64: Add DAI node and soundcard for HDMI > arm: sun8i: h3: Add HDMI audio to Orange Pi 2 > arm: sun8i: h3: Add HDMI audio to Beelink X2 > arm64: dts: allwinner: a64: Add HDMI audio to Pine64 > > Ondrej Jirman (3): > arm64: dts: allwinner: Enable HDMI audio on Orange Pi PC 2 > ARM: dts: sun8i-h3: Enable HDMI audio on Orange Pi PC/One > arm64: dts: sun50i-h6-orangepi-3: Enable HDMI audio Acked-by: Chen-Yu Tsai <wens@csie.org> for all the board DTS file changes.
Hi Chen-Yu, On Mon, 28 Sep 2020 at 06:40, Chen-Yu Tsai <wens@csie.org> wrote: > > On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@gmail.com> wrote: > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > H6 I2S is very similar to that in H3, except it supports up to 16 > > channels. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > sound/soc/sunxi/sun4i-i2s.c | 224 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 224 insertions(+) > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > > index f23ff29e7c1d..2baf6c276280 100644 > > --- a/sound/soc/sunxi/sun4i-i2s.c > > +++ b/sound/soc/sunxi/sun4i-i2s.c > > @@ -124,6 +124,21 @@ > > #define SUN8I_I2S_RX_CHAN_SEL_REG 0x54 > > #define SUN8I_I2S_RX_CHAN_MAP_REG 0x58 > > > > +/* Defines required for sun50i-h6 support */ > > +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK GENMASK(21, 20) > > +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset) ((offset) << 20) > > +#define SUN50I_H6_I2S_TX_CHAN_SEL_MASK GENMASK(19, 16) > > +#define SUN50I_H6_I2S_TX_CHAN_SEL(chan) ((chan - 1) << 16) > > +#define SUN50I_H6_I2S_TX_CHAN_EN_MASK GENMASK(15, 0) > > +#define SUN50I_H6_I2S_TX_CHAN_EN(num_chan) (((1 << num_chan) - 1)) > > + > > +#define SUN50I_H6_I2S_TX_CHAN_MAP0_REG 0x44 > > +#define SUN50I_H6_I2S_TX_CHAN_MAP1_REG 0x48 > > + > > +#define SUN50I_H6_I2S_RX_CHAN_SEL_REG 0x64 > > +#define SUN50I_H6_I2S_RX_CHAN_MAP0_REG 0x68 > > +#define SUN50I_H6_I2S_RX_CHAN_MAP1_REG 0x6C > > + > > struct sun4i_i2s; > > > > /** > > @@ -474,6 +489,62 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > return 0; > > } > > > > +static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > + const struct snd_pcm_hw_params *params) > > +{ > > + unsigned int channels = params_channels(params); > > + unsigned int slots = channels; > > + unsigned int lrck_period; > > + > > + if (i2s->slots) > > + slots = i2s->slots; > > + > > + /* Map the channels for playback and capture */ > > + regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210); > > + regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 0x76543210); > > Nit, since it supports up to 16 channels, you might want to map all 16 of them > now, instead of having to come back and fix it later. Thanks for the review. Do you mean there is missing MAP0 for RX/TX ? + regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0xFEDCBA98); regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210); + regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP0_REG, 0xFEDCBA98); regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 0x76543210); Regards, Clement > > Code wise, this patch is > > Reviewed-by: Chen-Yu Tsai <wens@csie.org> > > I don't have a scope nor logic analyzer, so I wasn't able to participate in the > LRCK discussion.
On Mon, Sep 28, 2020 at 10:37 PM Clément Péron <peron.clem@gmail.com> wrote: > > Hi Chen-Yu, > > On Mon, 28 Sep 2020 at 06:40, Chen-Yu Tsai <wens@csie.org> wrote: > > > > On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@gmail.com> wrote: > > > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > H6 I2S is very similar to that in H3, except it supports up to 16 > > > channels. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > --- > > > sound/soc/sunxi/sun4i-i2s.c | 224 ++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 224 insertions(+) > > > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > > > index f23ff29e7c1d..2baf6c276280 100644 > > > --- a/sound/soc/sunxi/sun4i-i2s.c > > > +++ b/sound/soc/sunxi/sun4i-i2s.c > > > @@ -124,6 +124,21 @@ > > > #define SUN8I_I2S_RX_CHAN_SEL_REG 0x54 > > > #define SUN8I_I2S_RX_CHAN_MAP_REG 0x58 > > > > > > +/* Defines required for sun50i-h6 support */ > > > +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK GENMASK(21, 20) > > > +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset) ((offset) << 20) > > > +#define SUN50I_H6_I2S_TX_CHAN_SEL_MASK GENMASK(19, 16) > > > +#define SUN50I_H6_I2S_TX_CHAN_SEL(chan) ((chan - 1) << 16) > > > +#define SUN50I_H6_I2S_TX_CHAN_EN_MASK GENMASK(15, 0) > > > +#define SUN50I_H6_I2S_TX_CHAN_EN(num_chan) (((1 << num_chan) - 1)) > > > + > > > +#define SUN50I_H6_I2S_TX_CHAN_MAP0_REG 0x44 > > > +#define SUN50I_H6_I2S_TX_CHAN_MAP1_REG 0x48 > > > + > > > +#define SUN50I_H6_I2S_RX_CHAN_SEL_REG 0x64 > > > +#define SUN50I_H6_I2S_RX_CHAN_MAP0_REG 0x68 > > > +#define SUN50I_H6_I2S_RX_CHAN_MAP1_REG 0x6C > > > + > > > struct sun4i_i2s; > > > > > > /** > > > @@ -474,6 +489,62 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > > return 0; > > > } > > > > > > +static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > > + const struct snd_pcm_hw_params *params) > > > +{ > > > + unsigned int channels = params_channels(params); > > > + unsigned int slots = channels; > > > + unsigned int lrck_period; > > > + > > > + if (i2s->slots) > > > + slots = i2s->slots; > > > + > > > + /* Map the channels for playback and capture */ > > > + regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210); > > > + regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 0x76543210); > > > > Nit, since it supports up to 16 channels, you might want to map all 16 of them > > now, instead of having to come back and fix it later. > > Thanks for the review. Do you mean there is missing MAP0 for RX/TX ? > > + regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0xFEDCBA98); > regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210); > + regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP0_REG, 0xFEDCBA98); > regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 0x76543210); Correct. ChenYu > Regards, > Clement > > > > > Code wise, this patch is > > > > Reviewed-by: Chen-Yu Tsai <wens@csie.org> > > > > I don't have a scope nor logic analyzer, so I wasn't able to participate in the > > LRCK discussion. > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/CAJiuCcevtzX_%2B02r54q6tH0%2BbOF%3DBM%3DknnaxN%2BG3QW035F8gZQ%40mail.gmail.com.
Hi Chen-Yu, On Mon, 28 Sep 2020 at 07:00, Chen-Yu Tsai <wens@csie.org> wrote: > > On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@gmail.com> wrote: > > > > As slots and slot_width can be overwritter in case set_tdm() is > > called. Avoid to have this logic in set_chan_cfg(). > > It doesn't seem that set_tdm_slot() would get called concurrently > with hw_params(), at least not for the simple-card family. If so > then we'd have more problems like mismatched slots/slot-width > due to no serialization when interacting with those values. Sorry maybe the commit log is not clear. I was not talking about a concurrent effect but more if the slot_width is setted then we don't properly use it later. > > > Instead pass the required values as params to set_chan_cfg(). > > > > This will also avoid a bug when we will enable 20/24bits support, > > i2s->slot_width is not actually used in the lrck_period computation. > > > > Suggested-by: Samuel Holland <samuel@sholland.org> > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > sound/soc/sunxi/sun4i-i2s.c | 36 ++++++++++++++---------------------- > > 1 file changed, 14 insertions(+), 22 deletions(-) > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > > index 2baf6c276280..0633b9fba3d7 100644 > > --- a/sound/soc/sunxi/sun4i-i2s.c > > +++ b/sound/soc/sunxi/sun4i-i2s.c > > @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks { > > unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *); > > s8 (*get_sr)(const struct sun4i_i2s *, int); > > s8 (*get_wss)(const struct sun4i_i2s *, int); > > - int (*set_chan_cfg)(const struct sun4i_i2s *, > > - const struct snd_pcm_hw_params *); > > + int (*set_chan_cfg)(const struct sun4i_i2s *i2s, > > + unsigned int channels, unsigned int slots, > > + unsigned int slot_width); > > int (*set_fmt)(const struct sun4i_i2s *, unsigned int); > > }; > > > > @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width) > > } > > > > static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > - const struct snd_pcm_hw_params *params) > > + unsigned int channels, unsigned int slots, > > + unsigned int slot_width) > > { > > - unsigned int channels = params_channels(params); > > - > > /* Map the channels for playback and capture */ > > regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210); > > regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x00003210); > > @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > } > > > > static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > - const struct snd_pcm_hw_params *params) > > + unsigned int channels, unsigned int slots, > > + unsigned int slot_width) > > { > > - unsigned int channels = params_channels(params); > > - unsigned int slots = channels; > > unsigned int lrck_period; > > > > - if (i2s->slots) > > - slots = i2s->slots; > > - > > Based on the bug you highlighted in your expanded commit log, the simpler fix > would be to look at i2s->slot_width in addition to i2s->slots, and replace > params_physical_width(params) accordingly. That's what I did in the first version but was pointed out by Samuel that the same logic was done several times. As I can avoid it by passing the correct params it's simpler. > > Also, I would put fixes (even preparatory ones) before patches that introduce > support for new features and hardware. That makes them easier to backport if > needed. I will wait for Maxime to comment on this. Regarding the doc fixes tag should only be used to fix a previous commit. This will make the fixes commit a bit more complicated for stable kernel team. Thanks for your review :) Regards Clement > > > ChenYu > > > > /* Map the channels for playback and capture */ > > regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210); > > regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210); > > @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > case SND_SOC_DAIFMT_DSP_B: > > case SND_SOC_DAIFMT_LEFT_J: > > case SND_SOC_DAIFMT_RIGHT_J: > > - lrck_period = params_physical_width(params) * slots; > > + lrck_period = slot_width * slots; > > break; > > > > case SND_SOC_DAIFMT_I2S: > > - lrck_period = params_physical_width(params); > > + lrck_period = slot_width; > > break; > > > > default:
Hi Chen-Yu, On Mon, 28 Sep 2020 at 07:42, Chen-Yu Tsai <wens@csie.org> wrote: > > On Mon, Sep 28, 2020 at 1:32 PM Chen-Yu Tsai <wens@csie.org> wrote: > > > > On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@gmail.com> wrote: > > > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > 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 <jernej.skrabec@siol.net> > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > --- > > > 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? Regards, Clement > > > > + status = "disabled"; > > > + }; > > > + > > > spdif: spdif@5093000 { > > > #sound-dai-cells = <0>; > > > compatible = "allwinner,sun50i-h6-spdif"; > > > @@ -739,6 +771,7 @@ ohci3: usb@5311400 { > > > }; > > > > > > hdmi: hdmi@6000000 { > > > + #sound-dai-cells = <0>; > > > compatible = "allwinner,sun50i-h6-dw-hdmi"; > > > reg = <0x06000000 0x10000>; > > > reg-io-width = <1>; > > > > The rest of the patch looks OK.
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 <wens@csie.org> wrote: > > > > On Mon, Sep 28, 2020 at 1:32 PM Chen-Yu Tsai <wens@csie.org> wrote: > > > > > > On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@gmail.com> wrote: > > > > > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > 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 <jernej.skrabec@siol.net> > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > > --- > > > > 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
Hi, On Fri, 2 Oct 2020 at 18:24, Maxime Ripard <maxime@cerno.tech> 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 <wens@csie.org> wrote: > > > > > > On Mon, Sep 28, 2020 at 1:32 PM Chen-Yu Tsai <wens@csie.org> wrote: > > > > > > > > On Mon, Sep 28, 2020 at 3:29 AM Clément Péron <peron.clem@gmail.com> wrote: > > > > > > > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > > > 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 <jernej.skrabec@siol.net> > > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > > > --- > > > > > 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