| Message ID | 20200426104115.22630-4-peron.clem@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add H6 I2S support | expand |
Hi, On Sun, Apr 26, 2020 at 12:41:11PM +0200, Clément Péron 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 | 227 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 227 insertions(+) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index 4198a5410bf9..a23c9f2a3f8c 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; > > /** > @@ -469,6 +484,65 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > return 0; > } > > +static int sun50i_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); > + > + /* Configure the channels */ > + regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, > + SUN50I_H6_I2S_TX_CHAN_SEL_MASK, > + SUN50I_H6_I2S_TX_CHAN_SEL(channels)); > + regmap_update_bits(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_SEL_REG, > + SUN50I_H6_I2S_TX_CHAN_SEL_MASK, > + SUN50I_H6_I2S_TX_CHAN_SEL(channels)); > + > + regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, > + SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK, > + SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM(channels)); > + regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, > + SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM_MASK, > + SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM(channels)); > + > + switch (i2s->format & SND_SOC_DAIFMT_FORMAT_MASK) { > + case SND_SOC_DAIFMT_DSP_A: > + 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; > + break; > + > + case SND_SOC_DAIFMT_I2S: > + lrck_period = params_physical_width(params); > + break; > + > + default: > + return -EINVAL; > + } > + > + if (i2s->slot_width) > + lrck_period = i2s->slot_width; > + > + regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG, > + SUN8I_I2S_FMT0_LRCK_PERIOD_MASK, > + SUN8I_I2S_FMT0_LRCK_PERIOD(lrck_period)); > + > + regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, > + SUN50I_H6_I2S_TX_CHAN_EN_MASK, > + SUN50I_H6_I2S_TX_CHAN_EN(channels)); > + > + return 0; > +} > + > static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, > struct snd_pcm_hw_params *params, > struct snd_soc_dai *dai) > @@ -694,6 +768,108 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > return 0; > } > > +static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > + unsigned int fmt) The alignment is off here > +{ > + u32 mode, val; > + u8 offset; > + > + /* > + * DAI clock polarity > + * > + * The setup for LRCK contradicts the datasheet, but under a > + * scope it's clear that the LRCK polarity is reversed > + * compared to the expected polarity on the bus. > + */ Did you check this or has it been copy-pasted? Thanks! Maxime
Hi Maxime, On Tue, 28 Apr 2020 at 10:13, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi, > > On Sun, Apr 26, 2020 at 12:41:11PM +0200, Clément Péron 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 | 227 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 227 insertions(+) > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > > index 4198a5410bf9..a23c9f2a3f8c 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; > > > > /** > > @@ -469,6 +484,65 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, > > return 0; > > } > > > > +static int sun50i_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); > > + > > + /* Configure the channels */ > > + regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, > > + SUN50I_H6_I2S_TX_CHAN_SEL_MASK, > > + SUN50I_H6_I2S_TX_CHAN_SEL(channels)); > > + regmap_update_bits(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_SEL_REG, > > + SUN50I_H6_I2S_TX_CHAN_SEL_MASK, > > + SUN50I_H6_I2S_TX_CHAN_SEL(channels)); > > + > > + regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, > > + SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK, > > + SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM(channels)); > > + regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, > > + SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM_MASK, > > + SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM(channels)); > > + > > + switch (i2s->format & SND_SOC_DAIFMT_FORMAT_MASK) { > > + case SND_SOC_DAIFMT_DSP_A: > > + 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; > > + break; > > + > > + case SND_SOC_DAIFMT_I2S: > > + lrck_period = params_physical_width(params); > > + break; > > + > > + default: > > + return -EINVAL; > > + } > > + > > + if (i2s->slot_width) > > + lrck_period = i2s->slot_width; > > + > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG, > > + SUN8I_I2S_FMT0_LRCK_PERIOD_MASK, > > + SUN8I_I2S_FMT0_LRCK_PERIOD(lrck_period)); > > + > > + regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, > > + SUN50I_H6_I2S_TX_CHAN_EN_MASK, > > + SUN50I_H6_I2S_TX_CHAN_EN(channels)); > > + > > + return 0; > > +} > > + > > static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, > > struct snd_pcm_hw_params *params, > > struct snd_soc_dai *dai) > > @@ -694,6 +768,108 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > > return 0; > > } > > > > +static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > > + unsigned int fmt) > > The alignment is off here > > > +{ > > + u32 mode, val; > > + u8 offset; > > + > > + /* > > + * DAI clock polarity > > + * > > + * The setup for LRCK contradicts the datasheet, but under a > > + * scope it's clear that the LRCK polarity is reversed > > + * compared to the expected polarity on the bus. > > + */ > > Did you check this or has it been copy-pasted? copy-pasted, I will check this. Thanks, Clement > > Thanks! > Maxime
On Tue, Apr 28, 2020 at 10:55:47AM +0200, Clément Péron wrote: > > > +static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > > > + unsigned int fmt) > > > > The alignment is off here > > > > > +{ > > > + u32 mode, val; > > > + u8 offset; > > > + > > > + /* > > > + * DAI clock polarity > > > + * > > > + * The setup for LRCK contradicts the datasheet, but under a > > > + * scope it's clear that the LRCK polarity is reversed > > > + * compared to the expected polarity on the bus. > > > + */ > > > > Did you check this or has it been copy-pasted? > > copy-pasted, I will check this. It's not going to be easy to do this if you only have a board with HDMI. If you can't test that easily, just remove the comment (or make it explicit that you copy pasted it?), no comment is better than a wrong one. Maxime
Hi Maxime, On Wed, 29 Apr 2020 at 14:35, Maxime Ripard <maxime@cerno.tech> wrote: > > On Tue, Apr 28, 2020 at 10:55:47AM +0200, Clément Péron wrote: > > > > +static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > > > > + unsigned int fmt) > > > > > > The alignment is off here > > > > > > > +{ > > > > + u32 mode, val; > > > > + u8 offset; > > > > + > > > > + /* > > > > + * DAI clock polarity > > > > + * > > > > + * The setup for LRCK contradicts the datasheet, but under a > > > > + * scope it's clear that the LRCK polarity is reversed > > > > + * compared to the expected polarity on the bus. > > > > + */ > > > > > > Did you check this or has it been copy-pasted? > > > > copy-pasted, I will check this. > > It's not going to be easy to do this if you only have a board with HDMI. If you > can't test that easily, just remove the comment (or make it explicit that you > copy pasted it?), no comment is better than a wrong one. I have talked with Marcus Cooper it may be able to test this this week-end. Also this can explain why we need the " simple-audio-card,frame-inversion;" in the device-tree. If think this fix has been introduced by you, correct? Could you say on which SoC did you see this issue? Thanks Clement > > Maxime
Hi, On Wed, Apr 29, 2020 at 06:33:00PM +0200, Clément Péron wrote: > On Wed, 29 Apr 2020 at 14:35, Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Tue, Apr 28, 2020 at 10:55:47AM +0200, Clément Péron wrote: > > > > > +static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > > > > > + unsigned int fmt) > > > > > > > > The alignment is off here > > > > > > > > > +{ > > > > > + u32 mode, val; > > > > > + u8 offset; > > > > > + > > > > > + /* > > > > > + * DAI clock polarity > > > > > + * > > > > > + * The setup for LRCK contradicts the datasheet, but under a > > > > > + * scope it's clear that the LRCK polarity is reversed > > > > > + * compared to the expected polarity on the bus. > > > > > + */ > > > > > > > > Did you check this or has it been copy-pasted? > > > > > > copy-pasted, I will check this. > > > > It's not going to be easy to do this if you only have a board with HDMI. If you > > can't test that easily, just remove the comment (or make it explicit that you > > copy pasted it?), no comment is better than a wrong one. > > I have talked with Marcus Cooper it may be able to test this this week-end. > Also this can explain why we need the " > simple-audio-card,frame-inversion;" in the device-tree. > > If think this fix has been introduced by you, correct? Could you say > on which SoC did you see this issue? This was seen on an H3 Maxime
Hi Maxime, On Thu, 30 Apr 2020 at 10:46, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi, > > On Wed, Apr 29, 2020 at 06:33:00PM +0200, Clément Péron wrote: > > On Wed, 29 Apr 2020 at 14:35, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Tue, Apr 28, 2020 at 10:55:47AM +0200, Clément Péron wrote: > > > > > > +static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > > > > > > + unsigned int fmt) > > > > > > > > > > The alignment is off here > > > > > > > > > > > +{ > > > > > > + u32 mode, val; > > > > > > + u8 offset; > > > > > > + > > > > > > + /* > > > > > > + * DAI clock polarity > > > > > > + * > > > > > > + * The setup for LRCK contradicts the datasheet, but under a > > > > > > + * scope it's clear that the LRCK polarity is reversed > > > > > > + * compared to the expected polarity on the bus. > > > > > > + */ > > > > > > > > > > Did you check this or has it been copy-pasted? > > > > > > > > copy-pasted, I will check this. > > > > > > It's not going to be easy to do this if you only have a board with HDMI. If you > > > can't test that easily, just remove the comment (or make it explicit that you > > > copy pasted it?), no comment is better than a wrong one. > > > > I have talked with Marcus Cooper it may be able to test this this week-end. > > Also this can explain why we need the " > > simple-audio-card,frame-inversion;" in the device-tree. > > > > If think this fix has been introduced by you, correct? Could you say > > on which SoC did you see this issue? > > This was seen on an H3 Just two more questions: - Did you observe this issue on both TDM and I2S mode? - On which DAI node? Since recent change in sun4i-i2s.c, we had to introduce the "simple-audio-card,frame-inversion" in LibreElec tree. H3 boards are quite common in LibreElec community so I think: - This fix is only needed in TDM mode - Or this fix is not required for the HDMI DAI node (HDMI DAI is a little bit different compare to other DAI but I think the first guess is more likely) Regards, Clement > > Maxime
Hi Clement, On Thu, Apr 30, 2020 at 04:00:14PM +0200, Clément Péron wrote: > On Thu, 30 Apr 2020 at 10:46, Maxime Ripard <maxime@cerno.tech> wrote: > > On Wed, Apr 29, 2020 at 06:33:00PM +0200, Clément Péron wrote: > > > On Wed, 29 Apr 2020 at 14:35, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > On Tue, Apr 28, 2020 at 10:55:47AM +0200, Clément Péron wrote: > > > > > > > +static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > > > > > > > + unsigned int fmt) > > > > > > > > > > > > The alignment is off here > > > > > > > > > > > > > +{ > > > > > > > + u32 mode, val; > > > > > > > + u8 offset; > > > > > > > + > > > > > > > + /* > > > > > > > + * DAI clock polarity > > > > > > > + * > > > > > > > + * The setup for LRCK contradicts the datasheet, but under a > > > > > > > + * scope it's clear that the LRCK polarity is reversed > > > > > > > + * compared to the expected polarity on the bus. > > > > > > > + */ > > > > > > > > > > > > Did you check this or has it been copy-pasted? > > > > > > > > > > copy-pasted, I will check this. > > > > > > > > It's not going to be easy to do this if you only have a board with HDMI. If you > > > > can't test that easily, just remove the comment (or make it explicit that you > > > > copy pasted it?), no comment is better than a wrong one. > > > > > > I have talked with Marcus Cooper it may be able to test this this week-end. > > > Also this can explain why we need the " > > > simple-audio-card,frame-inversion;" in the device-tree. > > > > > > If think this fix has been introduced by you, correct? Could you say > > > on which SoC did you see this issue? > > > > This was seen on an H3 > > Just two more questions: > - Did you observe this issue on both TDM and I2S mode? > - On which DAI node? This is fairly fuzzy now and I don't remember if I tested it in I2S. Let's assume I didn't. And similarly, I'm not sure what the exact controller was, but it was one of the regular controllers (so not either connected to the codec or the HDMI, one that goes out of the SoC). We had pretty much the same issue on the A33 in I2S for the codec though: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/sound/soc/sunxi/sun8i-codec.c?id=18c1bf35c1c09bca05cf70bc984a4764e0b0372b I didn't think of it that way then, but it might very well have been the i2s controller suffering the same issue. > Since recent change in sun4i-i2s.c, we had to introduce the > "simple-audio-card,frame-inversion" in LibreElec tree. Do you have a link to that commit ? I couldn't find anything on libreelec's github repo. > H3 boards are quite common in LibreElec community so I think: > - This fix is only needed in TDM mode > - Or this fix is not required for the HDMI DAI node (HDMI DAI is a > little bit different compare to other DAI but I think the first guess > is more likely) Given what we know about the A33, I'd be inclined to say the latter. I'd don't have the tools to check anymore, but if you have even a cheap logical analyzer, i2s being pretty slow you can definitely see it. Maxime
Hi Maxime, On Mon, 4 May 2020 at 14:09, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi Clement, > > On Thu, Apr 30, 2020 at 04:00:14PM +0200, Clément Péron wrote: > > On Thu, 30 Apr 2020 at 10:46, Maxime Ripard <maxime@cerno.tech> wrote: > > > On Wed, Apr 29, 2020 at 06:33:00PM +0200, Clément Péron wrote: > > > > On Wed, 29 Apr 2020 at 14:35, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > On Tue, Apr 28, 2020 at 10:55:47AM +0200, Clément Péron wrote: > > > > > > > > +static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > > > > > > > > + unsigned int fmt) > > > > > > > > > > > > > > The alignment is off here > > > > > > > > > > > > > > > +{ > > > > > > > > + u32 mode, val; > > > > > > > > + u8 offset; > > > > > > > > + > > > > > > > > + /* > > > > > > > > + * DAI clock polarity > > > > > > > > + * > > > > > > > > + * The setup for LRCK contradicts the datasheet, but under a > > > > > > > > + * scope it's clear that the LRCK polarity is reversed > > > > > > > > + * compared to the expected polarity on the bus. > > > > > > > > + */ > > > > > > > > > > > > > > Did you check this or has it been copy-pasted? > > > > > > > > > > > > copy-pasted, I will check this. > > > > > > > > > > It's not going to be easy to do this if you only have a board with HDMI. If you > > > > > can't test that easily, just remove the comment (or make it explicit that you > > > > > copy pasted it?), no comment is better than a wrong one. > > > > > > > > I have talked with Marcus Cooper it may be able to test this this week-end. > > > > Also this can explain why we need the " > > > > simple-audio-card,frame-inversion;" in the device-tree. > > > > > > > > If think this fix has been introduced by you, correct? Could you say > > > > on which SoC did you see this issue? > > > > > > This was seen on an H3 > > > > Just two more questions: > > - Did you observe this issue on both TDM and I2S mode? > > - On which DAI node? > > This is fairly fuzzy now and I don't remember if I tested it in I2S. Let's > assume I didn't. And similarly, I'm not sure what the exact controller was, but > it was one of the regular controllers (so not either connected to the codec or > the HDMI, one that goes out of the SoC). > > We had pretty much the same issue on the A33 in I2S for the codec though: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/sound/soc/sunxi/sun8i-codec.c?id=18c1bf35c1c09bca05cf70bc984a4764e0b0372b > > I didn't think of it that way then, but it might very well have been the i2s > controller suffering the same issue. > > > Since recent change in sun4i-i2s.c, we had to introduce the > > "simple-audio-card,frame-inversion" in LibreElec tree. > > Do you have a link to that commit ? I couldn't find anything on libreelec's > github repo. These commits: https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Allwinner/devices/A64/patches/linux/04-Add-frame-inversion-to-correct-multi-channel-audio.patch https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Allwinner/devices/H3/patches/linux/17-Add-frame-inversion-to-correct-multi-channel-audio.patch https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Allwinner/devices/H6/patches/linux/16-Add-frame-inversion-to-correct-multi-channel-audio.patch > > > H3 boards are quite common in LibreElec community so I think: > > - This fix is only needed in TDM mode > > - Or this fix is not required for the HDMI DAI node (HDMI DAI is a > > little bit different compare to other DAI but I think the first guess > > is more likely) > > Given what we know about the A33, I'd be inclined to say the latter. I'd don't > have the tools to check anymore, but if you have even a cheap logical analyzer, > i2s being pretty slow you can definitely see it. Me neither but maybe Marcus will be able to check this. Thanks for all these informations. > > Maxime
On Wed, Jul 29, 2020 at 04:39:27PM +0200, Maxime Ripard wrote: > It really looks like the polarity of LRCK is fine though. The first word > is sent with LRCK low, and then high, so we have channel 0 and then > channel 1 which seems to be the proper ordering? Yes, that's normal.
Hi Maxime, On Wed, 29 Jul 2020 at 17:16, Mark Brown <broonie@kernel.org> wrote: > > On Wed, Jul 29, 2020 at 04:39:27PM +0200, Maxime Ripard wrote: > > > It really looks like the polarity of LRCK is fine though. The first word > > is sent with LRCK low, and then high, so we have channel 0 and then > > channel 1 which seems to be the proper ordering? > > Yes, that's normal. Thank you very much for this test. So I will revert the following commit: ASoC: sun4i-i2s: Fix the LRCK polarity https://github.com/clementperon/linux/commit/dd657eae8164f7e4bafe8b875031a7c6c50646a9 Regards, Clement
On Thu, Sep 03, 2020 at 10:02:31PM +0200, Clément Péron wrote: > Hi Maxime, > > On Wed, 29 Jul 2020 at 17:16, Mark Brown <broonie@kernel.org> wrote: > > > > On Wed, Jul 29, 2020 at 04:39:27PM +0200, Maxime Ripard wrote: > > > > > It really looks like the polarity of LRCK is fine though. The first word > > > is sent with LRCK low, and then high, so we have channel 0 and then > > > channel 1 which seems to be the proper ordering? > > > > Yes, that's normal. > > Thank you very much for this test. > > So I will revert the following commit: > > ASoC: sun4i-i2s: Fix the LRCK polarity > > https://github.com/clementperon/linux/commit/dd657eae8164f7e4bafe8b875031a7c6c50646a9 Like I said, the current code is working as expected with regard to the LRCK polarity. The issue is that the samples are delayed and start to be transmitted on the wrong phase of the signal. But the LRCK polarity is fine. Maxime
Maxime, On 9/3/20 3:58 PM, Maxime Ripard wrote: > On Thu, Sep 03, 2020 at 10:02:31PM +0200, Clément Péron wrote: >> Hi Maxime, >> >> On Wed, 29 Jul 2020 at 17:16, Mark Brown <broonie@kernel.org> wrote: >>> >>> On Wed, Jul 29, 2020 at 04:39:27PM +0200, Maxime Ripard wrote: >>> >>>> It really looks like the polarity of LRCK is fine though. The first word >>>> is sent with LRCK low, and then high, so we have channel 0 and then >>>> channel 1 which seems to be the proper ordering? Which image file is this in reference to? >>> Yes, that's normal. >> >> Thank you very much for this test. >> >> So I will revert the following commit: >> >> ASoC: sun4i-i2s: Fix the LRCK polarity >> >> https://github.com/clementperon/linux/commit/dd657eae8164f7e4bafe8b875031a7c6c50646a9 > > Like I said, the current code is working as expected with regard to the > LRCK polarity. The issue is that the samples are delayed and start to be > transmitted on the wrong phase of the signal. Since an I2S LRCK frame is radially symmetric, "wrong phase" and "inverted polarity" look the same. The only way to definitively distinguish them is by looking at the sample data. In "i2s-h6.png", the samples are all zeroes, so you're assuming that the first sample transmitted (that is, when the bit clock starts transitioning) was a "left" sample. However, in "h6-i2s-start-data.png", there are pairs of samples we can look at. I'm still assuming that similar samples are a left/right pair, but that's probably a safe assumption. Here we see the first sample in each pair is transmitted with LRCK *high*, and the second sample in the pair is transmitted with LRCK *low*. This is the opposite of your claim above. An ideal test would put left/right markers and frame numbers in the data channel. The Python script below can generate such a file. Then you would know how much startup delay there is, which channel the "first sample" came from, and how each channel maps to the LRCK level. It would also be helpful to test DSP_A mode, where the LRCK signal is asymmetric and an inversion would be obvious. > But the LRCK polarity is fine. > > Maxime > Samuel ----8<---- import wave from struct import Struct markers = (0x2, 0xe) rate = 8000 seconds = 10 struct = Struct('<' + 'H' * len(markers)) nframes = seconds * rate data = bytearray(nframes * struct.size) for i in range(nframes): frame = [(m << 12) + (i % 2**12) for m in markers] offset = i * struct.size struct.pack_into(data, offset, *frame) with wave.open('test.wav', 'wb') as wf: wf.setparams((len(markers), 2, rate, nframes, 'NONE', '')) wf.writeframes(data)
Maxime, On 9/10/20 9:33 AM, Maxime Ripard wrote: > On Thu, Sep 03, 2020 at 09:54:39PM -0500, Samuel Holland wrote: >> On 9/3/20 3:58 PM, Maxime Ripard wrote: >>> On Thu, Sep 03, 2020 at 10:02:31PM +0200, Clément Péron wrote: >>>> Hi Maxime, >>>> >>>> On Wed, 29 Jul 2020 at 17:16, Mark Brown <broonie@kernel.org> wrote: >>>>> >>>>> On Wed, Jul 29, 2020 at 04:39:27PM +0200, Maxime Ripard wrote: >>>>> >>>>>> It really looks like the polarity of LRCK is fine though. The first word >>>>>> is sent with LRCK low, and then high, so we have channel 0 and then >>>>>> channel 1 which seems to be the proper ordering? >> >> Which image file is this in reference to? >> >>>>> Yes, that's normal. >>>> >>>> Thank you very much for this test. >>>> >>>> So I will revert the following commit: >>>> >>>> ASoC: sun4i-i2s: Fix the LRCK polarity >>>> >>>> https://github.com/clementperon/linux/commit/dd657eae8164f7e4bafe8b875031a7c6c50646a9 >>> >>> Like I said, the current code is working as expected with regard to the >>> LRCK polarity. The issue is that the samples are delayed and start to be >>> transmitted on the wrong phase of the signal. >> >> Since an I2S LRCK frame is radially symmetric, "wrong phase" and "inverted >> polarity" look the same. The only way to definitively distinguish them is by >> looking at the sample data. >> >> In "i2s-h6.png", the samples are all zeroes, so you're assuming that the first >> sample transmitted (that is, when the bit clock starts transitioning) was a >> "left" sample. >> >> However, in "h6-i2s-start-data.png", there are pairs of samples we can look at. >> I'm still assuming that similar samples are a left/right pair, but that's >> probably a safe assumption. Here we see the first sample in each pair is >> transmitted with LRCK *high*, and the second sample in the pair is transmitted >> with LRCK *low*. This is the opposite of your claim above. >> >> An ideal test would put left/right markers and frame numbers in the data >> channel. The Python script below can generate such a file. Then you would know >> how much startup delay there is, which channel the "first sample" came from, and >> how each channel maps to the LRCK level. >> >> It would also be helpful to test DSP_A mode, where the LRCK signal is >> asymmetric and an inversion would be obvious. > > I had no idea that there was a wave module in Python, that's a great > suggestion, thanks! > > You'll find attached the screenshots for both the I2S and DSP_A formats. > I zoomed out a bit to be able to have the first valid samples, but it > should be readable. > > The code I used is there: > https://github.com/mripard/linux/tree/sunxi/h6-i2s-test > > It's basically the v3, plus the DT bits. > > As you can see, in the i2s case, LRCK starts low and then goes up, with > the first channel (0x2*** samples) transmitted first, so everything > looks right here. > > On the DSP_A screenshot, LRCK will be low with small bursts high, and > once again with the first channel being transmitted first, so it looks > right to me too. Indeed, for H6 i2s0 with LRCK inversion in software, everything looks correct on the wire. It's still concerning to me that the BSP has no evidence of this inversion, either for i2s0 or i2s1[1]. And the inversion seems not to be required for HDMI audio on mainline either (but there could be an inversion on the HDMI side or on the interconnect). Even so, your research is sufficient justification for me that the code is correct as-is (with the inversion). Thank you very much for collecting the data! Cheers, Samuel [1]: https://github.com/Allwinner-Homlet/H6-BSP4.9-linux/blob/e634a960316dddd1eb50f2a6cf237f2f1c6da3e6/sound/soc/sunxi/sunxi-daudio.c#L1062 where 1 == SND_SOC_DAIFMT_NB_NF, and there's no inversion in sunxi_daudio_init_fmt().
Hi, On Sat, Sep 12, 2020 at 03:29:55PM -0500, Samuel Holland wrote: > On 9/10/20 9:33 AM, Maxime Ripard wrote: > > On Thu, Sep 03, 2020 at 09:54:39PM -0500, Samuel Holland wrote: > >> On 9/3/20 3:58 PM, Maxime Ripard wrote: > >>> On Thu, Sep 03, 2020 at 10:02:31PM +0200, Clément Péron wrote: > >>>> Hi Maxime, > >>>> > >>>> On Wed, 29 Jul 2020 at 17:16, Mark Brown <broonie@kernel.org> wrote: > >>>>> > >>>>> On Wed, Jul 29, 2020 at 04:39:27PM +0200, Maxime Ripard wrote: > >>>>> > >>>>>> It really looks like the polarity of LRCK is fine though. The first word > >>>>>> is sent with LRCK low, and then high, so we have channel 0 and then > >>>>>> channel 1 which seems to be the proper ordering? > >> > >> Which image file is this in reference to? > >> > >>>>> Yes, that's normal. > >>>> > >>>> Thank you very much for this test. > >>>> > >>>> So I will revert the following commit: > >>>> > >>>> ASoC: sun4i-i2s: Fix the LRCK polarity > >>>> > >>>> https://github.com/clementperon/linux/commit/dd657eae8164f7e4bafe8b875031a7c6c50646a9 > >>> > >>> Like I said, the current code is working as expected with regard to the > >>> LRCK polarity. The issue is that the samples are delayed and start to be > >>> transmitted on the wrong phase of the signal. > >> > >> Since an I2S LRCK frame is radially symmetric, "wrong phase" and "inverted > >> polarity" look the same. The only way to definitively distinguish them is by > >> looking at the sample data. > >> > >> In "i2s-h6.png", the samples are all zeroes, so you're assuming that the first > >> sample transmitted (that is, when the bit clock starts transitioning) was a > >> "left" sample. > >> > >> However, in "h6-i2s-start-data.png", there are pairs of samples we can look at. > >> I'm still assuming that similar samples are a left/right pair, but that's > >> probably a safe assumption. Here we see the first sample in each pair is > >> transmitted with LRCK *high*, and the second sample in the pair is transmitted > >> with LRCK *low*. This is the opposite of your claim above. > >> > >> An ideal test would put left/right markers and frame numbers in the data > >> channel. The Python script below can generate such a file. Then you would know > >> how much startup delay there is, which channel the "first sample" came from, and > >> how each channel maps to the LRCK level. > >> > >> It would also be helpful to test DSP_A mode, where the LRCK signal is > >> asymmetric and an inversion would be obvious. > > > > I had no idea that there was a wave module in Python, that's a great > > suggestion, thanks! > > > > You'll find attached the screenshots for both the I2S and DSP_A formats. > > I zoomed out a bit to be able to have the first valid samples, but it > > should be readable. > > > > The code I used is there: > > https://github.com/mripard/linux/tree/sunxi/h6-i2s-test > > > > It's basically the v3, plus the DT bits. > > > > As you can see, in the i2s case, LRCK starts low and then goes up, with > > the first channel (0x2*** samples) transmitted first, so everything > > looks right here. > > > > On the DSP_A screenshot, LRCK will be low with small bursts high, and > > once again with the first channel being transmitted first, so it looks > > right to me too. > > Indeed, for H6 i2s0 with LRCK inversion in software, everything looks correct on > the wire. > > It's still concerning to me that the BSP has no evidence of this inversion, > either for i2s0 or i2s1[1]. And the inversion seems not to be required for HDMI > audio on mainline either (but there could be an inversion on the HDMI side or on > the interconnect). One can only guess here, but it's also quite easy to fix it at the card level (or maybe there's a similar inversion in the codecs, or whatever). > Even so, your research is sufficient justification for me that the code is > correct as-is (with the inversion). Thank you very much for collecting the data! You're welcome, thanks for that script :) maxime
Hi Maxime and Samuel, On Thu, 17 Sep 2020 at 15:21, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi, > > On Sat, Sep 12, 2020 at 03:29:55PM -0500, Samuel Holland wrote: > > On 9/10/20 9:33 AM, Maxime Ripard wrote: > > > On Thu, Sep 03, 2020 at 09:54:39PM -0500, Samuel Holland wrote: > > >> On 9/3/20 3:58 PM, Maxime Ripard wrote: > > >>> On Thu, Sep 03, 2020 at 10:02:31PM +0200, Clément Péron wrote: > > >>>> Hi Maxime, > > >>>> > > >>>> On Wed, 29 Jul 2020 at 17:16, Mark Brown <broonie@kernel.org> wrote: > > >>>>> > > >>>>> On Wed, Jul 29, 2020 at 04:39:27PM +0200, Maxime Ripard wrote: > > >>>>> > > >>>>>> It really looks like the polarity of LRCK is fine though. The first word > > >>>>>> is sent with LRCK low, and then high, so we have channel 0 and then > > >>>>>> channel 1 which seems to be the proper ordering? > > >> > > >> Which image file is this in reference to? > > >> > > >>>>> Yes, that's normal. > > >>>> > > >>>> Thank you very much for this test. > > >>>> > > >>>> So I will revert the following commit: > > >>>> > > >>>> ASoC: sun4i-i2s: Fix the LRCK polarity > > >>>> > > >>>> https://github.com/clementperon/linux/commit/dd657eae8164f7e4bafe8b875031a7c6c50646a9 > > >>> > > >>> Like I said, the current code is working as expected with regard to the > > >>> LRCK polarity. The issue is that the samples are delayed and start to be > > >>> transmitted on the wrong phase of the signal. > > >> > > >> Since an I2S LRCK frame is radially symmetric, "wrong phase" and "inverted > > >> polarity" look the same. The only way to definitively distinguish them is by > > >> looking at the sample data. > > >> > > >> In "i2s-h6.png", the samples are all zeroes, so you're assuming that the first > > >> sample transmitted (that is, when the bit clock starts transitioning) was a > > >> "left" sample. > > >> > > >> However, in "h6-i2s-start-data.png", there are pairs of samples we can look at. > > >> I'm still assuming that similar samples are a left/right pair, but that's > > >> probably a safe assumption. Here we see the first sample in each pair is > > >> transmitted with LRCK *high*, and the second sample in the pair is transmitted > > >> with LRCK *low*. This is the opposite of your claim above. > > >> > > >> An ideal test would put left/right markers and frame numbers in the data > > >> channel. The Python script below can generate such a file. Then you would know > > >> how much startup delay there is, which channel the "first sample" came from, and > > >> how each channel maps to the LRCK level. > > >> > > >> It would also be helpful to test DSP_A mode, where the LRCK signal is > > >> asymmetric and an inversion would be obvious. > > > > > > I had no idea that there was a wave module in Python, that's a great > > > suggestion, thanks! > > > > > > You'll find attached the screenshots for both the I2S and DSP_A formats. > > > I zoomed out a bit to be able to have the first valid samples, but it > > > should be readable. > > > > > > The code I used is there: > > > https://github.com/mripard/linux/tree/sunxi/h6-i2s-test > > > > > > It's basically the v3, plus the DT bits. > > > > > > As you can see, in the i2s case, LRCK starts low and then goes up, with > > > the first channel (0x2*** samples) transmitted first, so everything > > > looks right here. > > > > > > On the DSP_A screenshot, LRCK will be low with small bursts high, and > > > once again with the first channel being transmitted first, so it looks > > > right to me too. > > > > Indeed, for H6 i2s0 with LRCK inversion in software, everything looks correct on > > the wire. > > > > It's still concerning to me that the BSP has no evidence of this inversion, > > either for i2s0 or i2s1[1]. And the inversion seems not to be required for HDMI > > audio on mainline either (but there could be an inversion on the HDMI side or on > > the interconnect). > > One can only guess here, but it's also quite easy to fix it at the card > level (or maybe there's a similar inversion in the codecs, or whatever). Thanks for the test and the explanation. Quite disturbing that there is no evidence of the LRCK inversion in kernel vendor indeed... Could it be an issue with the mainline code? But still regarding the kernel vendor, it seems logical to have a frame-inversion in the device-tree for the HDMI I2S node. I will drop the revert patch and re-add the frame inversion in the next series. Regards, Clement > > > Even so, your research is sufficient justification for me that the code is > > correct as-is (with the inversion). Thank you very much for collecting the data! > > You're welcome, thanks for that script :) > > maxime
Hi Clement, On Thu, Sep 17, 2020 at 03:55:45PM +0200, Clément Péron wrote: > Hi Maxime and Samuel, > > On Thu, 17 Sep 2020 at 15:21, Maxime Ripard <maxime@cerno.tech> wrote: > > > > Hi, > > > > On Sat, Sep 12, 2020 at 03:29:55PM -0500, Samuel Holland wrote: > > > On 9/10/20 9:33 AM, Maxime Ripard wrote: > > > > On Thu, Sep 03, 2020 at 09:54:39PM -0500, Samuel Holland wrote: > > > >> On 9/3/20 3:58 PM, Maxime Ripard wrote: > > > >>> On Thu, Sep 03, 2020 at 10:02:31PM +0200, Clément Péron wrote: > > > >>>> Hi Maxime, > > > >>>> > > > >>>> On Wed, 29 Jul 2020 at 17:16, Mark Brown <broonie@kernel.org> wrote: > > > >>>>> > > > >>>>> On Wed, Jul 29, 2020 at 04:39:27PM +0200, Maxime Ripard wrote: > > > >>>>> > > > >>>>>> It really looks like the polarity of LRCK is fine though. The first word > > > >>>>>> is sent with LRCK low, and then high, so we have channel 0 and then > > > >>>>>> channel 1 which seems to be the proper ordering? > > > >> > > > >> Which image file is this in reference to? > > > >> > > > >>>>> Yes, that's normal. > > > >>>> > > > >>>> Thank you very much for this test. > > > >>>> > > > >>>> So I will revert the following commit: > > > >>>> > > > >>>> ASoC: sun4i-i2s: Fix the LRCK polarity > > > >>>> > > > >>>> https://github.com/clementperon/linux/commit/dd657eae8164f7e4bafe8b875031a7c6c50646a9 > > > >>> > > > >>> Like I said, the current code is working as expected with regard to the > > > >>> LRCK polarity. The issue is that the samples are delayed and start to be > > > >>> transmitted on the wrong phase of the signal. > > > >> > > > >> Since an I2S LRCK frame is radially symmetric, "wrong phase" and "inverted > > > >> polarity" look the same. The only way to definitively distinguish them is by > > > >> looking at the sample data. > > > >> > > > >> In "i2s-h6.png", the samples are all zeroes, so you're assuming that the first > > > >> sample transmitted (that is, when the bit clock starts transitioning) was a > > > >> "left" sample. > > > >> > > > >> However, in "h6-i2s-start-data.png", there are pairs of samples we can look at. > > > >> I'm still assuming that similar samples are a left/right pair, but that's > > > >> probably a safe assumption. Here we see the first sample in each pair is > > > >> transmitted with LRCK *high*, and the second sample in the pair is transmitted > > > >> with LRCK *low*. This is the opposite of your claim above. > > > >> > > > >> An ideal test would put left/right markers and frame numbers in the data > > > >> channel. The Python script below can generate such a file. Then you would know > > > >> how much startup delay there is, which channel the "first sample" came from, and > > > >> how each channel maps to the LRCK level. > > > >> > > > >> It would also be helpful to test DSP_A mode, where the LRCK signal is > > > >> asymmetric and an inversion would be obvious. > > > > > > > > I had no idea that there was a wave module in Python, that's a great > > > > suggestion, thanks! > > > > > > > > You'll find attached the screenshots for both the I2S and DSP_A formats. > > > > I zoomed out a bit to be able to have the first valid samples, but it > > > > should be readable. > > > > > > > > The code I used is there: > > > > https://github.com/mripard/linux/tree/sunxi/h6-i2s-test > > > > > > > > It's basically the v3, plus the DT bits. > > > > > > > > As you can see, in the i2s case, LRCK starts low and then goes up, with > > > > the first channel (0x2*** samples) transmitted first, so everything > > > > looks right here. > > > > > > > > On the DSP_A screenshot, LRCK will be low with small bursts high, and > > > > once again with the first channel being transmitted first, so it looks > > > > right to me too. > > > > > > Indeed, for H6 i2s0 with LRCK inversion in software, everything looks correct on > > > the wire. > > > > > > It's still concerning to me that the BSP has no evidence of this inversion, > > > either for i2s0 or i2s1[1]. And the inversion seems not to be required for HDMI > > > audio on mainline either (but there could be an inversion on the HDMI side or on > > > the interconnect). > > > > One can only guess here, but it's also quite easy to fix it at the card > > level (or maybe there's a similar inversion in the codecs, or whatever). > > Thanks for the test and the explanation. > > Quite disturbing that there is no evidence of the LRCK inversion in > kernel vendor indeed... > Could it be an issue with the mainline code? I'm not sure what you mean here, this was tested with mainline? Maxime
Hi Maxime, On Thu, 17 Sep 2020 at 16:06, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi Clement, > > On Thu, Sep 17, 2020 at 03:55:45PM +0200, Clément Péron wrote: > > Hi Maxime and Samuel, > > > > On Thu, 17 Sep 2020 at 15:21, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > Hi, > > > > > > On Sat, Sep 12, 2020 at 03:29:55PM -0500, Samuel Holland wrote: > > > > On 9/10/20 9:33 AM, Maxime Ripard wrote: > > > > > On Thu, Sep 03, 2020 at 09:54:39PM -0500, Samuel Holland wrote: > > > > >> On 9/3/20 3:58 PM, Maxime Ripard wrote: > > > > >>> On Thu, Sep 03, 2020 at 10:02:31PM +0200, Clément Péron wrote: > > > > >>>> Hi Maxime, > > > > >>>> > > > > >>>> On Wed, 29 Jul 2020 at 17:16, Mark Brown <broonie@kernel.org> wrote: > > > > >>>>> > > > > >>>>> On Wed, Jul 29, 2020 at 04:39:27PM +0200, Maxime Ripard wrote: > > > > >>>>> > > > > >>>>>> It really looks like the polarity of LRCK is fine though. The first word > > > > >>>>>> is sent with LRCK low, and then high, so we have channel 0 and then > > > > >>>>>> channel 1 which seems to be the proper ordering? > > > > >> > > > > >> Which image file is this in reference to? > > > > >> > > > > >>>>> Yes, that's normal. > > > > >>>> > > > > >>>> Thank you very much for this test. > > > > >>>> > > > > >>>> So I will revert the following commit: > > > > >>>> > > > > >>>> ASoC: sun4i-i2s: Fix the LRCK polarity > > > > >>>> > > > > >>>> https://github.com/clementperon/linux/commit/dd657eae8164f7e4bafe8b875031a7c6c50646a9 > > > > >>> > > > > >>> Like I said, the current code is working as expected with regard to the > > > > >>> LRCK polarity. The issue is that the samples are delayed and start to be > > > > >>> transmitted on the wrong phase of the signal. > > > > >> > > > > >> Since an I2S LRCK frame is radially symmetric, "wrong phase" and "inverted > > > > >> polarity" look the same. The only way to definitively distinguish them is by > > > > >> looking at the sample data. > > > > >> > > > > >> In "i2s-h6.png", the samples are all zeroes, so you're assuming that the first > > > > >> sample transmitted (that is, when the bit clock starts transitioning) was a > > > > >> "left" sample. > > > > >> > > > > >> However, in "h6-i2s-start-data.png", there are pairs of samples we can look at. > > > > >> I'm still assuming that similar samples are a left/right pair, but that's > > > > >> probably a safe assumption. Here we see the first sample in each pair is > > > > >> transmitted with LRCK *high*, and the second sample in the pair is transmitted > > > > >> with LRCK *low*. This is the opposite of your claim above. > > > > >> > > > > >> An ideal test would put left/right markers and frame numbers in the data > > > > >> channel. The Python script below can generate such a file. Then you would know > > > > >> how much startup delay there is, which channel the "first sample" came from, and > > > > >> how each channel maps to the LRCK level. > > > > >> > > > > >> It would also be helpful to test DSP_A mode, where the LRCK signal is > > > > >> asymmetric and an inversion would be obvious. > > > > > > > > > > I had no idea that there was a wave module in Python, that's a great > > > > > suggestion, thanks! > > > > > > > > > > You'll find attached the screenshots for both the I2S and DSP_A formats. > > > > > I zoomed out a bit to be able to have the first valid samples, but it > > > > > should be readable. > > > > > > > > > > The code I used is there: > > > > > https://github.com/mripard/linux/tree/sunxi/h6-i2s-test > > > > > > > > > > It's basically the v3, plus the DT bits. > > > > > > > > > > As you can see, in the i2s case, LRCK starts low and then goes up, with > > > > > the first channel (0x2*** samples) transmitted first, so everything > > > > > looks right here. > > > > > > > > > > On the DSP_A screenshot, LRCK will be low with small bursts high, and > > > > > once again with the first channel being transmitted first, so it looks > > > > > right to me too. > > > > > > > > Indeed, for H6 i2s0 with LRCK inversion in software, everything looks correct on > > > > the wire. > > > > > > > > It's still concerning to me that the BSP has no evidence of this inversion, > > > > either for i2s0 or i2s1[1]. And the inversion seems not to be required for HDMI > > > > audio on mainline either (but there could be an inversion on the HDMI side or on > > > > the interconnect). > > > > > > One can only guess here, but it's also quite easy to fix it at the card > > > level (or maybe there's a similar inversion in the codecs, or whatever). > > > > Thanks for the test and the explanation. > > > > Quite disturbing that there is no evidence of the LRCK inversion in > > kernel vendor indeed... > > Could it be an issue with the mainline code? > > I'm not sure what you mean here, this was tested with mainline? Sorry i was not clear, I meant either there is an issue in the vendor kernel that doesn't set properly the LRCK or maybe we did something or forgot to do it that set this inversion. But I just checked a device-tree used with a kernel vendor and indeed codecs are inverted but not hdmi so the vendor kernel has an issue here... E.g this is what is used for Tanix TX6 daudio@0x05091000 { compatible = "allwinner,sunxi-tdmhdmi"; reg = <0x00 0x5091000 0x00 0x74>; clocks = <0x04 0x4d>; status = "okay"; phandle = <0x63>; device_type = "audiohdmi"; }; daudio@0x05092000 { compatible = "allwinner,sunxi-daudio"; reg = <0x00 0x5092000 0x00 0x74>; clocks = <0x04 0x4e>; pinctrl-names = "default\0sleep"; pinctrl-0 = <0x4f>; pinctrl-1 = <0x50>; pcm_lrck_period = <0x40>; slot_width_select = <0x20>; daudio_master = <0x04>; audio_format = <0x04>; signal_inversion = <0x03>; tdm_config = <0x01>; frametype = <0x00>; tdm_num = <0x02>; mclk_div = <0x01>; status = "okay"; phandle = <0x65>; device_type = "daudio2"; }; daudio@0x0508f000 { compatible = "allwinner,sunxi-daudio"; reg = <0x00 0x508f000 0x00 0x74>; clocks = <0x04 0x51>; pinctrl-names = "default\0sleep"; pinctrl-0 = <0x52>; pinctrl-1 = <0x53>; pcm_lrck_period = <0x20>; slot_width_select = <0x20>; daudio_master = <0x04>; audio_format = <0x01>; signal_inversion = <0x01>; tdm_config = <0x01>; frametype = <0x00>; tdm_num = <0x03>; mclk_div = <0x01>; status = "okay"; phandle = <0x67>; device_type = "daudio3"; }; Clement > > Maxime
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 4198a5410bf9..a23c9f2a3f8c 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; /** @@ -469,6 +484,65 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, return 0; } +static int sun50i_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); + + /* Configure the channels */ + regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, + SUN50I_H6_I2S_TX_CHAN_SEL_MASK, + SUN50I_H6_I2S_TX_CHAN_SEL(channels)); + regmap_update_bits(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_SEL_REG, + SUN50I_H6_I2S_TX_CHAN_SEL_MASK, + SUN50I_H6_I2S_TX_CHAN_SEL(channels)); + + regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, + SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK, + SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM(channels)); + regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, + SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM_MASK, + SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM(channels)); + + switch (i2s->format & SND_SOC_DAIFMT_FORMAT_MASK) { + case SND_SOC_DAIFMT_DSP_A: + 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; + break; + + case SND_SOC_DAIFMT_I2S: + lrck_period = params_physical_width(params); + break; + + default: + return -EINVAL; + } + + if (i2s->slot_width) + lrck_period = i2s->slot_width; + + regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG, + SUN8I_I2S_FMT0_LRCK_PERIOD_MASK, + SUN8I_I2S_FMT0_LRCK_PERIOD(lrck_period)); + + regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, + SUN50I_H6_I2S_TX_CHAN_EN_MASK, + SUN50I_H6_I2S_TX_CHAN_EN(channels)); + + return 0; +} + static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) @@ -694,6 +768,108 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, return 0; } +static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, + unsigned int fmt) +{ + u32 mode, val; + u8 offset; + + /* + * DAI clock polarity + * + * The setup for LRCK contradicts the datasheet, but under a + * scope it's clear that the LRCK polarity is reversed + * compared to the expected polarity on the bus. + */ + switch (fmt & SND_SOC_DAIFMT_INV_MASK) { + case SND_SOC_DAIFMT_IB_IF: + /* Invert both clocks */ + val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED; + break; + case SND_SOC_DAIFMT_IB_NF: + /* Invert bit clock */ + val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED | + SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED; + break; + case SND_SOC_DAIFMT_NB_IF: + /* Invert frame clock */ + val = 0; + break; + case SND_SOC_DAIFMT_NB_NF: + val = SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED; + break; + default: + return -EINVAL; + } + + regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG, + SUN8I_I2S_FMT0_LRCLK_POLARITY_MASK | + SUN8I_I2S_FMT0_BCLK_POLARITY_MASK, + val); + + /* DAI Mode */ + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { + case SND_SOC_DAIFMT_DSP_A: + mode = SUN8I_I2S_CTRL_MODE_PCM; + offset = 1; + break; + + case SND_SOC_DAIFMT_DSP_B: + mode = SUN8I_I2S_CTRL_MODE_PCM; + offset = 0; + break; + + case SND_SOC_DAIFMT_I2S: + mode = SUN8I_I2S_CTRL_MODE_LEFT; + offset = 1; + break; + + case SND_SOC_DAIFMT_LEFT_J: + mode = SUN8I_I2S_CTRL_MODE_LEFT; + offset = 0; + break; + + case SND_SOC_DAIFMT_RIGHT_J: + mode = SUN8I_I2S_CTRL_MODE_RIGHT; + offset = 0; + break; + + default: + return -EINVAL; + } + + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, + SUN8I_I2S_CTRL_MODE_MASK, mode); + regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, + SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK, + SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset)); + regmap_update_bits(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_SEL_REG, + SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK, + SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset)); + + /* DAI clock master masks */ + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { + case SND_SOC_DAIFMT_CBS_CFS: + /* BCLK and LRCLK master */ + val = SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT; + break; + + case SND_SOC_DAIFMT_CBM_CFM: + /* BCLK and LRCLK slave */ + val = 0; + break; + + default: + return -EINVAL; + } + + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, + SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT, + val); + + return 0; +} + static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) { struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai); @@ -974,6 +1150,22 @@ static const struct reg_default sun8i_i2s_reg_defaults[] = { { SUN8I_I2S_RX_CHAN_MAP_REG, 0x00000000 }, }; +static const struct reg_default sun50i_i2s_reg_defaults[] = { + { SUN4I_I2S_CTRL_REG, 0x00060000 }, + { SUN4I_I2S_FMT0_REG, 0x00000033 }, + { SUN4I_I2S_FMT1_REG, 0x00000030 }, + { SUN4I_I2S_FIFO_CTRL_REG, 0x000400f0 }, + { SUN4I_I2S_DMA_INT_CTRL_REG, 0x00000000 }, + { SUN4I_I2S_CLK_DIV_REG, 0x00000000 }, + { SUN8I_I2S_CHAN_CFG_REG, 0x00000000 }, + { SUN8I_I2S_TX_CHAN_SEL_REG, 0x00000000 }, + { SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0x00000000 }, + { SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x00000000 }, + { SUN50I_H6_I2S_RX_CHAN_SEL_REG, 0x00000000 }, + { SUN50I_H6_I2S_RX_CHAN_MAP0_REG, 0x00000000 }, + { SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 0x00000000 }, +}; + static const struct regmap_config sun4i_i2s_regmap_config = { .reg_bits = 32, .reg_stride = 4, @@ -1001,6 +1193,19 @@ static const struct regmap_config sun8i_i2s_regmap_config = { .volatile_reg = sun8i_i2s_volatile_reg, }; +static const struct regmap_config sun50i_i2s_regmap_config = { + .reg_bits = 32, + .reg_stride = 4, + .val_bits = 32, + .max_register = SUN50I_H6_I2S_RX_CHAN_MAP1_REG, + .cache_type = REGCACHE_FLAT, + .reg_defaults = sun50i_i2s_reg_defaults, + .num_reg_defaults = ARRAY_SIZE(sun50i_i2s_reg_defaults), + .writeable_reg = sun4i_i2s_wr_reg, + .readable_reg = sun8i_i2s_rd_reg, + .volatile_reg = sun8i_i2s_volatile_reg, +}; + static int sun4i_i2s_runtime_resume(struct device *dev) { struct sun4i_i2s *i2s = dev_get_drvdata(dev); @@ -1159,6 +1364,24 @@ static const struct sun4i_i2s_quirks sun50i_a64_codec_i2s_quirks = { .set_fmt = sun4i_i2s_set_soc_fmt, }; +static const struct sun4i_i2s_quirks sun50i_h6_i2s_quirks = { + .has_reset = true, + .reg_offset_txdata = SUN8I_I2S_FIFO_TX_REG, + .sun4i_i2s_regmap = &sun50i_i2s_regmap_config, + .field_clkdiv_mclk_en = REG_FIELD(SUN4I_I2S_CLK_DIV_REG, 8, 8), + .field_fmt_wss = REG_FIELD(SUN4I_I2S_FMT0_REG, 0, 2), + .field_fmt_sr = REG_FIELD(SUN4I_I2S_FMT0_REG, 4, 6), + .bclk_dividers = sun8i_i2s_clk_div, + .num_bclk_dividers = ARRAY_SIZE(sun8i_i2s_clk_div), + .mclk_dividers = sun8i_i2s_clk_div, + .num_mclk_dividers = ARRAY_SIZE(sun8i_i2s_clk_div), + .get_bclk_parent_rate = sun8i_i2s_get_bclk_parent_rate, + .get_sr = sun8i_i2s_get_sr_wss, + .get_wss = sun8i_i2s_get_sr_wss, + .set_chan_cfg = sun50i_i2s_set_chan_cfg, + .set_fmt = sun50i_i2s_set_soc_fmt, +}; + static int sun4i_i2s_init_regmap_fields(struct device *dev, struct sun4i_i2s *i2s) { @@ -1328,6 +1551,10 @@ static const struct of_device_id sun4i_i2s_match[] = { .compatible = "allwinner,sun50i-a64-codec-i2s", .data = &sun50i_a64_codec_i2s_quirks, }, + { + .compatible = "allwinner,sun50i-h6-i2s", + .data = &sun50i_h6_i2s_quirks, + }, {} }; MODULE_DEVICE_TABLE(of, sun4i_i2s_match);