Message ID | 20200920180758.592217-1-peron.clem@gmail.com |
---|---|
Headers | show |
Series | Add Allwinner H3/H5/H6/A64 HDMI audio | expand |
On 9/20/20 1:07 PM, 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 | 218 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 218 insertions(+) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index f23ff29e7c1d..348057464bed 100644 > --- a/sound/soc/sunxi/sun4i-i2s.c > +++ b/sound/soc/sunxi/sun4i-i2s.c ... > @@ -699,6 +770,102 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > return 0; > } > > +static int sun50i_h6_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > + unsigned int fmt) > +{ > + u32 mode, val; > + u8 offset; > + > + /* DAI clock polarity */ > + switch (fmt & SND_SOC_DAIFMT_INV_MASK) { > + case SND_SOC_DAIFMT_IB_IF: > + /* Invert both clocks */ > + val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED | > + SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED; > + break; > + case SND_SOC_DAIFMT_IB_NF: > + /* Invert bit clock */ > + val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED; > + break; > + case SND_SOC_DAIFMT_NB_IF: > + /* Invert frame clock */ > + val = SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED; > + break; > + case SND_SOC_DAIFMT_NB_NF: > + val = 0; > + break; > + default: > + return -EINVAL; > + } Maxime's testing that showed LRCK inversion was necessary was done on the H6. So in addition to dropping the patch that removed the LRCK inversion for other sun8i variants, you need to re-add it to this patch for the H6 variant. Cheers, Samuel > + > + 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); ...
On 9/20/20 1:07 PM, Clément Péron wrote: > From: Marcus Cooper <codekipper@gmail.com> > > Extend the functionality of the driver to include support of 20 and > 24 bits per sample. > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > Signed-off-by: Clément Péron <peron.clem@gmail.com> > Acked-by: Maxime Ripard <mripard@kernel.org> > --- > sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > As I have mentioned before, if you want to support a 32-bit slot width on sun4i variants (which patch 2 does via TDM and this patch does via PCM format), you need to fix sun4i_i2s_get_wss() to return "3", not "4", for a 32-bit input. Cheers, Samuel
On 9/20/20 1:07 PM, Clément Péron wrote: > The FIFO TX reg is volatile and sun8i i2s register > mapping is different from sun4i. > > Even if in this case it's doesn't create an issue, > Avoid setting some regs that are undefined in sun8i. > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > Acked-by: Maxime Ripard <mripard@kernel.org> > --- > sound/soc/sunxi/sun4i-i2s.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index ce4913f0ffe4..a35be0e2baf5 100644 > --- a/sound/soc/sunxi/sun4i-i2s.c > +++ b/sound/soc/sunxi/sun4i-i2s.c > @@ -1126,12 +1126,19 @@ static bool sun8i_i2s_rd_reg(struct device *dev, unsigned int reg) > > static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg) > { > - if (reg == SUN8I_I2S_INT_STA_REG) > + switch (reg) { > + case SUN4I_I2S_FIFO_CTRL_REG: Please check if this breaks audio recording with runtime PM enabled. I noticed this with an older revision of the series that also changed sun4i_i2s_volatile_reg. Marking SUN4I_I2S_FIFO_CTRL_REG as volatile broke setting of SUN4I_I2S_FIFO_CTRL_TX_MODE/RX_MODE, because the set_fmt() callback is not run with a runtime PM reference held, and volatile registers are not written by regcache_sync() during sun4i_i2s_runtime_resume(). As a workaround, I moved the TX_MODE/RX_MODE initialization to hw_params(), but I am not sure it is the right thing to do: https://github.com/smaeul/linux/commit/5e40ac610986.patch Cheers, Samuel > + case SUN4I_I2S_FIFO_RX_REG: > + case SUN4I_I2S_FIFO_STA_REG: > + case SUN4I_I2S_RX_CNT_REG: > + case SUN4I_I2S_TX_CNT_REG: > + case SUN8I_I2S_FIFO_TX_REG: > + case SUN8I_I2S_INT_STA_REG: > return true; > - if (reg == SUN8I_I2S_FIFO_TX_REG) > - return false; > > - return sun4i_i2s_volatile_reg(dev, reg); > + default: > + return false; > + } > } > > static const struct reg_default sun4i_i2s_reg_defaults[] = { >
Hi Samuel, On Sun, 20 Sep 2020 at 20:39, Samuel Holland <samuel@sholland.org> wrote: > > On 9/20/20 1:07 PM, 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 | 218 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 218 insertions(+) > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > > index f23ff29e7c1d..348057464bed 100644 > > --- a/sound/soc/sunxi/sun4i-i2s.c > > +++ b/sound/soc/sunxi/sun4i-i2s.c > ... > > @@ -699,6 +770,102 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > > return 0; > > } > > > > +static int sun50i_h6_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > > + unsigned int fmt) > > +{ > > + u32 mode, val; > > + u8 offset; > > + > > + /* DAI clock polarity */ > > + switch (fmt & SND_SOC_DAIFMT_INV_MASK) { > > + case SND_SOC_DAIFMT_IB_IF: > > + /* Invert both clocks */ > > + val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED | > > + SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED; > > + break; > > + case SND_SOC_DAIFMT_IB_NF: > > + /* Invert bit clock */ > > + val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED; > > + break; > > + case SND_SOC_DAIFMT_NB_IF: > > + /* Invert frame clock */ > > + val = SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED; > > + break; > > + case SND_SOC_DAIFMT_NB_NF: > > + val = 0; > > + break; > > + default: > > + return -EINVAL; > > + } > > Maxime's testing that showed LRCK inversion was necessary was done on the H6. So > in addition to dropping the patch that removed the LRCK inversion for other > sun8i variants, you need to re-add it to this patch for the H6 variant. Thanks, you're right! Clement > > Cheers, > Samuel > > > + > > + 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); > ...
Hi Samuel, On Sun, 20 Sep 2020 at 20:52, Samuel Holland <samuel@sholland.org> wrote: > > On 9/20/20 1:07 PM, Clément Péron wrote: > > The FIFO TX reg is volatile and sun8i i2s register > > mapping is different from sun4i. > > > > Even if in this case it's doesn't create an issue, > > Avoid setting some regs that are undefined in sun8i. > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > Acked-by: Maxime Ripard <mripard@kernel.org> > > --- > > sound/soc/sunxi/sun4i-i2s.c | 15 +++++++++++---- > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > > index ce4913f0ffe4..a35be0e2baf5 100644 > > --- a/sound/soc/sunxi/sun4i-i2s.c > > +++ b/sound/soc/sunxi/sun4i-i2s.c > > @@ -1126,12 +1126,19 @@ static bool sun8i_i2s_rd_reg(struct device *dev, unsigned int reg) > > > > static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg) > > { > > - if (reg == SUN8I_I2S_INT_STA_REG) > > + switch (reg) { > > + case SUN4I_I2S_FIFO_CTRL_REG: > > Please check if this breaks audio recording with runtime PM enabled. I noticed > this with an older revision of the series that also changed > sun4i_i2s_volatile_reg. Marking SUN4I_I2S_FIFO_CTRL_REG as volatile broke > setting of SUN4I_I2S_FIFO_CTRL_TX_MODE/RX_MODE, because the set_fmt() callback > is not run with a runtime PM reference held, and volatile registers are not > written by regcache_sync() during sun4i_i2s_runtime_resume(). > > As a workaround, I moved the TX_MODE/RX_MODE initialization to hw_params(), but > I am not sure it is the right thing to do: Thanks for the catch, I never tried to suspend/resume my board actually. But your explanation and the fix seems legit to me. I don't think it's a workaround as settings the fifo size is not related to set_fmt and could also be set in hw_params. I will add your fix in the next version. Regards, Clement > > https://github.com/smaeul/linux/commit/5e40ac610986.patch > > Cheers, > Samuel > > > + case SUN4I_I2S_FIFO_RX_REG: > > + case SUN4I_I2S_FIFO_STA_REG: > > + case SUN4I_I2S_RX_CNT_REG: > > + case SUN4I_I2S_TX_CNT_REG: > > + case SUN8I_I2S_FIFO_TX_REG: > > + case SUN8I_I2S_INT_STA_REG: > > return true; > > - if (reg == SUN8I_I2S_FIFO_TX_REG) > > - return false; > > > > - return sun4i_i2s_volatile_reg(dev, reg); > > + default: > > + return false; > > + } > > } > > > > static const struct reg_default sun4i_i2s_reg_defaults[] = { > > >
Hi Samuel, On Sun, 20 Sep 2020 at 20:45, Samuel Holland <samuel@sholland.org> wrote: > > On 9/20/20 1:07 PM, Clément Péron wrote: > > From: Marcus Cooper <codekipper@gmail.com> > > > > Extend the functionality of the driver to include support of 20 and > > 24 bits per sample. > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > Acked-by: Maxime Ripard <mripard@kernel.org> > > --- > > sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > As I have mentioned before, if you want to support a 32-bit slot width on sun4i > variants (which patch 2 does via TDM and this patch does via PCM format), you > need to fix sun4i_i2s_get_wss() to return "3", not "4", for a 32-bit input. Sorry I didn't get it the first time. Is using a switch case is a correct solution? static s8 sun4i_i2s_get_wss(const struct sun4i_i2s *i2s, int width) { switch (width) { case 16: return 0x0; case 20: return 0x1; case 24: return 0x2; case 32: return 0x3; } return -EINVAL; } Clement > > Cheers, > Samuel