| Message ID | 20240529140658.180966-3-matteomartelli3@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: sunxi: sun4i-i2s: fix LRCLK polarity in i2s mode | expand |
Hi, On Wed, May 29, 2024 at 04:00:15PM GMT, Matteo Martelli wrote: > This fixes the LRCLK polarity for sun8i-h3 and sun50i-h6 in i2s mode > which was wrongly inverted. > > The LRCLK was being set in reversed logic compared to the DAI format: > inverted LRCLK for SND_SOC_DAIFMT_IB_NF and SND_SOC_DAIFMT_NB_NF; normal > LRCLK for SND_SOC_DAIFMT_IB_IF and SND_SOC_DAIFMT_NB_IF. Such reversed > logic applies properly for DSP_A, DSP_B, LEFT_J and RIGHT_J modes but > not for I2S mode, for which the LRCLK signal results reversed to what > expected on the bus. The issue is due to a misinterpretation of the > LRCLK polarity bit of the H3 and H6 i2s controllers. Such bit in this > case does not mean "0 => normal" or "1 => inverted" according to the > expected bus operation, but it means "0 => frame starts on low edge" and > "1 => frame starts on high edge" (from the User Manuals). > > This commit fixes the LRCLK polarity by setting the LRCLK polarity bit > according to the selected bus mode and renames the LRCLK polarity bit > definition to avoid further confusion. > > Fixes: dd657eae8164 ("ASoC: sun4i-i2s: Fix the LRCK polarity") > Fixes: 73adf87b7a58 ("ASoC: sun4i-i2s: Add support for H6 I2S") > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> > --- > sound/soc/sunxi/sun4i-i2s.c | 143 ++++++++++++++++++------------------ > 1 file changed, 73 insertions(+), 70 deletions(-) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index 5f8d979585b6..a200ba41679a 100644 > --- a/sound/soc/sunxi/sun4i-i2s.c > +++ b/sound/soc/sunxi/sun4i-i2s.c > @@ -100,8 +100,8 @@ > #define SUN8I_I2S_CTRL_MODE_PCM (0 << 4) > > #define SUN8I_I2S_FMT0_LRCLK_POLARITY_MASK BIT(19) > -#define SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED (1 << 19) > -#define SUN8I_I2S_FMT0_LRCLK_POLARITY_NORMAL (0 << 19) > +#define SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH (1 << 19) > +#define SUN8I_I2S_FMT0_LRCLK_POLARITY_START_LOW (0 << 19) > #define SUN8I_I2S_FMT0_LRCK_PERIOD_MASK GENMASK(17, 8) > #define SUN8I_I2S_FMT0_LRCK_PERIOD(period) ((period - 1) << 8) > #define SUN8I_I2S_FMT0_BCLK_POLARITY_MASK BIT(7) > @@ -729,65 +729,37 @@ static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, > unsigned int fmt) > { > - u32 mode, val; > + u32 mode, lrclk_pol, bclk_pol, 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. > - */ I think we should keep that comment somewhere. Maxime
Maxime Ripard wrote: > > - /* > > - * 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. > > - */ > > I think we should keep that comment somewhere. I think that keeping that comment would be very misleading since the LRCLK setup would not contradict the datasheet anymore [1][2]. Also, do you recall any details about the mentioned scope test setup? Was i2s mode tested in that occasion? It would help clarify the situation. Could anyone verify this patch against H3/H6 SoCs? [1]: https://linux-sunxi.org/images/4/4b/Allwinner_H3_Datasheet_V1.2.pdf section 8.6.7.2 [2]: https://linux-sunxi.org/images/4/46/Allwinner_H6_V200_User_Manual_V1.1.pdf section 7.2.5.2 Thanks, Matteo Martelli
On Fri, Jun 07, 2024 at 10:04:43AM +0200, Matteo Martelli wrote: > Maxime Ripard wrote: > > > - /* > > > - * 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. > > > - */ > > > > I think we should keep that comment somewhere. > > I think that keeping that comment would be very misleading since the LRCLK > setup would not contradict the datasheet anymore [1][2]. > > Also, do you recall any details about the mentioned scope test setup? Was i2s > mode tested in that occasion? It would help clarify the situation. > > Could anyone verify this patch against H3/H6 SoCs? Any news on any of this?
Mark Brown wrote: > On Fri, Jun 07, 2024 at 10:04:43AM +0200, Matteo Martelli wrote: > > Maxime Ripard wrote: > > > > - /* > > > > - * 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. > > > > - */ > > > > > > I think we should keep that comment somewhere. > > > > I think that keeping that comment would be very misleading since the LRCLK > > setup would not contradict the datasheet anymore [1][2]. > > > > Also, do you recall any details about the mentioned scope test setup? Was i2s > > mode tested in that occasion? It would help clarify the situation. > > > > Could anyone verify this patch against H3/H6 SoCs? > > Any news on any of this? Not on my side, unfortunately I cannot test the fix against H3/H6 SoCs. Matteo Martelli
Hi, On Fri, Jun 07, 2024 at 10:04:43AM GMT, Matteo Martelli wrote: > Maxime Ripard wrote: > > > - /* > > > - * 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. > > > - */ > > > > I think we should keep that comment somewhere. > > I think that keeping that comment would be very misleading since the LRCLK > setup would not contradict the datasheet anymore [1][2]. > > Also, do you recall any details about the mentioned scope test setup? Was i2s > mode tested in that occasion? It would help clarify the situation. I can't remember if I tested i2s, I think I did though. But most of the work was done on either TDM or DSP modes, and I remember very clearly that the LRCK polarity was inverted compared to what Allwinner documents. So the doc was, at best, misleading for these formats and we should keep the comments. Maxime
Maxime Ripard wrote: > Hi, > > On Fri, Jun 07, 2024 at 10:04:43AM GMT, Matteo Martelli wrote: > > Maxime Ripard wrote: > > > > - /* > > > > - * 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. > > > > - */ > > > > > > I think we should keep that comment somewhere. > > > > I think that keeping that comment would be very misleading since the LRCLK > > setup would not contradict the datasheet anymore [1][2]. > > > > Also, do you recall any details about the mentioned scope test setup? Was i2s > > mode tested in that occasion? It would help clarify the situation. > > I can't remember if I tested i2s, I think I did though. But most of the > work was done on either TDM or DSP modes, and I remember very clearly > that the LRCK polarity was inverted compared to what Allwinner documents. > > So the doc was, at best, misleading for these formats and we should keep > the comments. > > Maxime Thanks for the reply Maxime, would you be able to point out the Allwinner document part that is (or was) misleading? The current datasheets (see links [1][2]) look correct, the current driver setup for TDM and DSP modes respects those datasheets and it's not "reversed compared to the expected polarity on the bus" as the comment states. Also I didn't find any related errata in their changelog. Could it be possible that during those mentioned tests you were still referring to the datasheets of other SoCs like A10 for instance? Or maybe that the misleading information was in another document rather than the main datasheets? If that's the case, would you still think that the comment should be kept as it is? [1]: https://linux-sunxi.org/images/4/4b/Allwinner_H3_Datasheet_V1.2.pdf section 8.6.7.2 [2]: https://linux-sunxi.org/images/4/46/Allwinner_H6_V200_User_Manual_V1.1.pdf section 7.2.5.2 Thanks, Matteo
On Tue, Jul 02, 2024 at 05:17:15PM GMT, Matteo Martelli wrote: > Maxime Ripard wrote: > > On Fri, Jun 07, 2024 at 10:04:43AM GMT, Matteo Martelli wrote: > > > Maxime Ripard wrote: > > > > > - /* > > > > > - * 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. > > > > > - */ > > > > > > > > I think we should keep that comment somewhere. > > > > > > I think that keeping that comment would be very misleading since the LRCLK > > > setup would not contradict the datasheet anymore [1][2]. > > > > > > Also, do you recall any details about the mentioned scope test setup? Was i2s > > > mode tested in that occasion? It would help clarify the situation. > > > > I can't remember if I tested i2s, I think I did though. But most of the > > work was done on either TDM or DSP modes, and I remember very clearly > > that the LRCK polarity was inverted compared to what Allwinner documents. > > > > So the doc was, at best, misleading for these formats and we should keep > > the comments. > > Thanks for the reply Maxime, would you be able to point out the Allwinner > document part that is (or was) misleading? The current datasheets (see links > [1][2]) look correct, the current driver setup for TDM and DSP modes respects > those datasheets and it's not "reversed compared to the expected polarity on > the bus" as the comment states. I clearly remember having to debug something there, but I don't remember much more, sorry. I guess if you have tested on the H3 I2S, TDM and DSP and it all works as expected with your changes, go ahead and ignore my comment then. > Also I didn't find any related errata in their changelog. Yeah... Allwinner doesn't do errata. > Could it be possible that during those mentioned tests you > were still referring to the datasheets of other SoCs like A10 for > instance? Or maybe that the misleading information was in another > document rather than the main datasheets? If that's the case, would > you still think that the comment should be kept as it is? Possibly, or an older version of the datasheet, I really can't remember. Maxime
Maxime Ripard wrote: > On Tue, Jul 02, 2024 at 05:17:15PM GMT, Matteo Martelli wrote: > > Maxime Ripard wrote: > > > On Fri, Jun 07, 2024 at 10:04:43AM GMT, Matteo Martelli wrote: > > > > Maxime Ripard wrote: > > > > > > - /* > > > > > > - * 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. > > > > > > - */ > > > > > > > > > > I think we should keep that comment somewhere. > > > > > > > > I think that keeping that comment would be very misleading since the LRCLK > > > > setup would not contradict the datasheet anymore [1][2]. > > > > > > > > Also, do you recall any details about the mentioned scope test setup? Was i2s > > > > mode tested in that occasion? It would help clarify the situation. > > > > > > I can't remember if I tested i2s, I think I did though. But most of the > > > work was done on either TDM or DSP modes, and I remember very clearly > > > that the LRCK polarity was inverted compared to what Allwinner documents. > > > > > > So the doc was, at best, misleading for these formats and we should keep > > > the comments. > > > > Thanks for the reply Maxime, would you be able to point out the Allwinner > > document part that is (or was) misleading? The current datasheets (see links > > [1][2]) look correct, the current driver setup for TDM and DSP modes respects > > those datasheets and it's not "reversed compared to the expected polarity on > > the bus" as the comment states. > > I clearly remember having to debug something there, but I don't remember > much more, sorry. > > I guess if you have tested on the H3 I2S, TDM and DSP and it all works > as expected with your changes, go ahead and ignore my comment then. I did test it on the A64 SoC only (all modes I2S, TDM and DSP working). Best reguards, Matteo
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 5f8d979585b6..a200ba41679a 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -100,8 +100,8 @@ #define SUN8I_I2S_CTRL_MODE_PCM (0 << 4) #define SUN8I_I2S_FMT0_LRCLK_POLARITY_MASK BIT(19) -#define SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED (1 << 19) -#define SUN8I_I2S_FMT0_LRCLK_POLARITY_NORMAL (0 << 19) +#define SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH (1 << 19) +#define SUN8I_I2S_FMT0_LRCLK_POLARITY_START_LOW (0 << 19) #define SUN8I_I2S_FMT0_LRCK_PERIOD_MASK GENMASK(17, 8) #define SUN8I_I2S_FMT0_LRCK_PERIOD(period) ((period - 1) << 8) #define SUN8I_I2S_FMT0_BCLK_POLARITY_MASK BIT(7) @@ -729,65 +729,37 @@ static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, unsigned int fmt) { - u32 mode, val; + u32 mode, lrclk_pol, bclk_pol, 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: + lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH; mode = SUN8I_I2S_CTRL_MODE_PCM; offset = 1; break; case SND_SOC_DAIFMT_DSP_B: + lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH; mode = SUN8I_I2S_CTRL_MODE_PCM; offset = 0; break; case SND_SOC_DAIFMT_I2S: + lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_LOW; mode = SUN8I_I2S_CTRL_MODE_LEFT; offset = 1; break; case SND_SOC_DAIFMT_LEFT_J: + lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH; mode = SUN8I_I2S_CTRL_MODE_LEFT; offset = 0; break; case SND_SOC_DAIFMT_RIGHT_J: + lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH; mode = SUN8I_I2S_CTRL_MODE_RIGHT; offset = 0; break; @@ -805,6 +777,35 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, SUN8I_I2S_TX_CHAN_OFFSET_MASK, SUN8I_I2S_TX_CHAN_OFFSET(offset)); + /* DAI polarity */ + bclk_pol = SUN8I_I2S_FMT0_BCLK_POLARITY_NORMAL; + + switch (fmt & SND_SOC_DAIFMT_INV_MASK) { + case SND_SOC_DAIFMT_IB_IF: + /* Invert both clocks */ + lrclk_pol ^= SUN8I_I2S_FMT0_BCLK_POLARITY_MASK; + bclk_pol = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED; + break; + case SND_SOC_DAIFMT_IB_NF: + /* Invert bit clock */ + bclk_pol = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED; + break; + case SND_SOC_DAIFMT_NB_IF: + /* Invert frame clock */ + lrclk_pol ^= SUN8I_I2S_FMT0_BCLK_POLARITY_MASK; + break; + case SND_SOC_DAIFMT_NB_NF: + /* No inversion */ + 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, + lrclk_pol | bclk_pol); + /* DAI clock master masks */ switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) { case SND_SOC_DAIFMT_BP_FP: @@ -836,65 +837,37 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, static int sun50i_h6_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, unsigned int fmt) { - u32 mode, val; + u32 mode, lrclk_pol, bclk_pol, 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: + lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH; mode = SUN8I_I2S_CTRL_MODE_PCM; offset = 1; break; case SND_SOC_DAIFMT_DSP_B: + lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH; mode = SUN8I_I2S_CTRL_MODE_PCM; offset = 0; break; case SND_SOC_DAIFMT_I2S: + lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_LOW; mode = SUN8I_I2S_CTRL_MODE_LEFT; offset = 1; break; case SND_SOC_DAIFMT_LEFT_J: + lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH; mode = SUN8I_I2S_CTRL_MODE_LEFT; offset = 0; break; case SND_SOC_DAIFMT_RIGHT_J: + lrclk_pol = SUN8I_I2S_FMT0_LRCLK_POLARITY_START_HIGH; mode = SUN8I_I2S_CTRL_MODE_RIGHT; offset = 0; break; @@ -912,6 +885,36 @@ static int sun50i_h6_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK, SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset)); + /* DAI clock polarity */ + bclk_pol = SUN8I_I2S_FMT0_BCLK_POLARITY_NORMAL; + + switch (fmt & SND_SOC_DAIFMT_INV_MASK) { + case SND_SOC_DAIFMT_IB_IF: + /* Invert both clocks */ + lrclk_pol ^= SUN8I_I2S_FMT0_BCLK_POLARITY_MASK; + bclk_pol = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED; + break; + case SND_SOC_DAIFMT_IB_NF: + /* Invert bit clock */ + bclk_pol = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED; + break; + case SND_SOC_DAIFMT_NB_IF: + /* Invert frame clock */ + lrclk_pol ^= SUN8I_I2S_FMT0_BCLK_POLARITY_MASK; + break; + case SND_SOC_DAIFMT_NB_NF: + /* No inversion */ + 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, + lrclk_pol | bclk_pol); + + /* DAI clock master masks */ switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) { case SND_SOC_DAIFMT_BP_FP:
This fixes the LRCLK polarity for sun8i-h3 and sun50i-h6 in i2s mode which was wrongly inverted. The LRCLK was being set in reversed logic compared to the DAI format: inverted LRCLK for SND_SOC_DAIFMT_IB_NF and SND_SOC_DAIFMT_NB_NF; normal LRCLK for SND_SOC_DAIFMT_IB_IF and SND_SOC_DAIFMT_NB_IF. Such reversed logic applies properly for DSP_A, DSP_B, LEFT_J and RIGHT_J modes but not for I2S mode, for which the LRCLK signal results reversed to what expected on the bus. The issue is due to a misinterpretation of the LRCLK polarity bit of the H3 and H6 i2s controllers. Such bit in this case does not mean "0 => normal" or "1 => inverted" according to the expected bus operation, but it means "0 => frame starts on low edge" and "1 => frame starts on high edge" (from the User Manuals). This commit fixes the LRCLK polarity by setting the LRCLK polarity bit according to the selected bus mode and renames the LRCLK polarity bit definition to avoid further confusion. Fixes: dd657eae8164 ("ASoC: sun4i-i2s: Fix the LRCK polarity") Fixes: 73adf87b7a58 ("ASoC: sun4i-i2s: Add support for H6 I2S") Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> --- sound/soc/sunxi/sun4i-i2s.c | 143 ++++++++++++++++++------------------ 1 file changed, 73 insertions(+), 70 deletions(-)