[PATCH v2] ASoC: fsl_ssi: Fix bclk calculation for mono channel

[PATCH v2] ASoC: fsl_ssi: Fix bclk calculation for mono channel

[Shengjiu Wang]

For mono channel, SSI will switch to Normal mode.

In Normal mode and Network mode, the Word Length Control bits
control the word length divider in clock generator, which is
different with I2S Master mode (the word length is fixed to
32bit), it should be the value of params_width(hw_params).

The condition "slots == 2" is not good for I2S Master mode,
because for Network mode and Normal mode, the slots can also
be 2. Then we need to use (ssi->i2s_net & SSI_SCR_I2S_MODE_MASK)
to check if it is I2S Master mode.

So we refine the famula for mono channel, otherwise there
will be sound issue for S24_LE.

Fixes: b0a7043d5c2c ("ASoC: fsl_ssi: Caculate bit clock rate using slot number and width")
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
changes in v2
- refine patch for Network mode and Normal mode.

 sound/soc/fsl/fsl_ssi.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index bad89b0d129e..cbf67d132fda 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -678,7 +678,8 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
 	struct regmap *regs = ssi->regs;
 	u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
 	unsigned long clkrate, baudrate, tmprate;
-	unsigned int slots = params_channels(hw_params);
+	unsigned int channels = params_channels(hw_params);
+	unsigned int slots;
 	unsigned int slot_width = 32;
 	u64 sub, savesub = 100000;
 	unsigned int freq;
@@ -688,9 +689,15 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
 	/* Override slots and slot_width if being specifically set... */
 	if (ssi->slots)
 		slots = ssi->slots;
-	/* ...but keep 32 bits if slots is 2 -- I2S Master mode */
-	if (ssi->slot_width && slots != 2)
-		slot_width = ssi->slot_width;
+	else
+		/* Apply two slots for mono channel, because DC = 2 */
+		slots = (channels == 1) ? 2 : channels;
+
+	/* ...but keep 32 bits if I2S Master mode */
+	if ((ssi->i2s_net & SSI_SCR_I2S_MODE_MASK) != SSI_SCR_I2S_MODE_MASTER ||
+	    channels == 1)
+		slot_width = ssi->slot_width ? ssi->slot_width :
+			     params_width(hw_params);
 
 	/* Generate bit clock based on the slot number and slot width */
 	freq = slots * slot_width * params_rate(hw_params);
-- 
2.21.0

Re: [PATCH v2] ASoC: fsl_ssi: Fix bclk calculation for mono channel

[Nicolin Chen]

On Mon, Jun 15, 2020 at 01:56:18PM +0800, Shengjiu Wang wrote:
> For mono channel, SSI will switch to Normal mode.
> 
> In Normal mode and Network mode, the Word Length Control bits
> control the word length divider in clock generator, which is
> different with I2S Master mode (the word length is fixed to
> 32bit), it should be the value of params_width(hw_params).
> 
> The condition "slots == 2" is not good for I2S Master mode,
> because for Network mode and Normal mode, the slots can also
> be 2. Then we need to use (ssi->i2s_net & SSI_SCR_I2S_MODE_MASK)
> to check if it is I2S Master mode.

The fsl_ssi_set_bclk is only called when fsl_ssi_is_i2s_master,
though I agree that that line of comments sounds confusing now.

> So we refine the famula for mono channel, otherwise there

famula => formula?

> will be sound issue for S24_LE.
> 
> Fixes: b0a7043d5c2c ("ASoC: fsl_ssi: Caculate bit clock rate using slot number and width")
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
> changes in v2
> - refine patch for Network mode and Normal mode.
> 
>  sound/soc/fsl/fsl_ssi.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index bad89b0d129e..cbf67d132fda 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -678,7 +678,8 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
>  	struct regmap *regs = ssi->regs;
>  	u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
>  	unsigned long clkrate, baudrate, tmprate;
> -	unsigned int slots = params_channels(hw_params);
> +	unsigned int channels = params_channels(hw_params);
> +	unsigned int slots;
>  	unsigned int slot_width = 32;
>  	u64 sub, savesub = 100000;
>  	unsigned int freq;
> @@ -688,9 +689,15 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
>  	/* Override slots and slot_width if being specifically set... */
>  	if (ssi->slots)
>  		slots = ssi->slots;
> -	/* ...but keep 32 bits if slots is 2 -- I2S Master mode */
> -	if (ssi->slot_width && slots != 2)
> -		slot_width = ssi->slot_width;
> +	else
> +		/* Apply two slots for mono channel, because DC = 2 */
> +		slots = (channels == 1) ? 2 : channels;
> +
> +	/* ...but keep 32 bits if I2S Master mode */
> +	if ((ssi->i2s_net & SSI_SCR_I2S_MODE_MASK) != SSI_SCR_I2S_MODE_MASTER ||
> +	    channels == 1)
> +		slot_width = ssi->slot_width ? ssi->slot_width :

This looks very complicated...can you review and try mine?
(Basically, take 32-bit out of default but force it later)

@@ -678,8 +678,9 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
        struct regmap *regs = ssi->regs;
        u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
        unsigned long clkrate, baudrate, tmprate;
-       unsigned int slots = params_channels(hw_params);
-       unsigned int slot_width = 32;
+       unsigned int channels = params_channels(hw_params);
+       unsigned int slot_width = params_width(hw_params);
+       unsigned int slots = 2;
        u64 sub, savesub = 100000;
        unsigned int freq;
        bool baudclk_is_used;
@@ -688,10 +689,16 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
        /* Override slots and slot_width if being specifically set... */
        if (ssi->slots)
                slots = ssi->slots;
-       /* ...but keep 32 bits if slots is 2 -- I2S Master mode */
-       if (ssi->slot_width && slots != 2)
+       if (ssi->slot_width)
                slot_width = ssi->slot_width;
 
+       /*
+        * ...but force 32 bits for stereo audio. Note that mono audio is also
+        * sent in 2 slots via NORMAL mode, so check both slots and channels.
+        */
+       if (slots == 2 && channels == 2)
+               slot_width = 32;
+
        /* Generate bit clock based on the slot number and slot width */
        freq = slots * slot_width * params_rate(hw_params);
 

Re: [PATCH v2] ASoC: fsl_ssi: Fix bclk calculation for mono channel

[Shengjiu Wang]

On Tue, Jun 16, 2020 at 7:11 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Mon, Jun 15, 2020 at 01:56:18PM +0800, Shengjiu Wang wrote:
> > For mono channel, SSI will switch to Normal mode.
> >
> > In Normal mode and Network mode, the Word Length Control bits
> > control the word length divider in clock generator, which is
> > different with I2S Master mode (the word length is fixed to
> > 32bit), it should be the value of params_width(hw_params).
> >
> > The condition "slots == 2" is not good for I2S Master mode,
> > because for Network mode and Normal mode, the slots can also
> > be 2. Then we need to use (ssi->i2s_net & SSI_SCR_I2S_MODE_MASK)
> > to check if it is I2S Master mode.
>
> The fsl_ssi_set_bclk is only called when fsl_ssi_is_i2s_master,
> though I agree that that line of comments sounds confusing now.

Actually I think fsl_ssi_is_i2s_master is not accurate, it just checks
the Master mode,  but didn't check the I2S mode.

>
> > So we refine the famula for mono channel, otherwise there
>
> famula => formula?
>
> > will be sound issue for S24_LE.
> >
> > Fixes: b0a7043d5c2c ("ASoC: fsl_ssi: Caculate bit clock rate using slot number and width")
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > changes in v2
> > - refine patch for Network mode and Normal mode.
> >
> >  sound/soc/fsl/fsl_ssi.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > index bad89b0d129e..cbf67d132fda 100644
> > --- a/sound/soc/fsl/fsl_ssi.c
> > +++ b/sound/soc/fsl/fsl_ssi.c
> > @@ -678,7 +678,8 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
> >       struct regmap *regs = ssi->regs;
> >       u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
> >       unsigned long clkrate, baudrate, tmprate;
> > -     unsigned int slots = params_channels(hw_params);
> > +     unsigned int channels = params_channels(hw_params);
> > +     unsigned int slots;
> >       unsigned int slot_width = 32;
> >       u64 sub, savesub = 100000;
> >       unsigned int freq;
> > @@ -688,9 +689,15 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
> >       /* Override slots and slot_width if being specifically set... */
> >       if (ssi->slots)
> >               slots = ssi->slots;
> > -     /* ...but keep 32 bits if slots is 2 -- I2S Master mode */
> > -     if (ssi->slot_width && slots != 2)
> > -             slot_width = ssi->slot_width;
> > +     else
> > +             /* Apply two slots for mono channel, because DC = 2 */
> > +             slots = (channels == 1) ? 2 : channels;
> > +
> > +     /* ...but keep 32 bits if I2S Master mode */
> > +     if ((ssi->i2s_net & SSI_SCR_I2S_MODE_MASK) != SSI_SCR_I2S_MODE_MASTER ||
> > +         channels == 1)
> > +             slot_width = ssi->slot_width ? ssi->slot_width :
>
> This looks very complicated...can you review and try mine?
> (Basically, take 32-bit out of default but force it later)
>
> @@ -678,8 +678,9 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
>         struct regmap *regs = ssi->regs;
>         u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
>         unsigned long clkrate, baudrate, tmprate;
> -       unsigned int slots = params_channels(hw_params);
> -       unsigned int slot_width = 32;
> +       unsigned int channels = params_channels(hw_params);
> +       unsigned int slot_width = params_width(hw_params);
> +       unsigned int slots = 2;
>         u64 sub, savesub = 100000;
>         unsigned int freq;
>         bool baudclk_is_used;
> @@ -688,10 +689,16 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
>         /* Override slots and slot_width if being specifically set... */
>         if (ssi->slots)
>                 slots = ssi->slots;
> -       /* ...but keep 32 bits if slots is 2 -- I2S Master mode */
> -       if (ssi->slot_width && slots != 2)
> +       if (ssi->slot_width)
>                 slot_width = ssi->slot_width;
>
> +       /*
> +        * ...but force 32 bits for stereo audio. Note that mono audio is also
> +        * sent in 2 slots via NORMAL mode, so check both slots and channels.
> +        */
> +       if (slots == 2 && channels == 2)
> +               slot_width = 32;

slots ==2 && channels ==2 does not mean the I2S Master mode.
For LEFT_J, it is also slots =2 & channels = 2, then the slot_width
should be params_width(hw_params).
and DSP_A/B also supports stereo.

Re: [PATCH v2] ASoC: fsl_ssi: Fix bclk calculation for mono channel

[Nicolin Chen]

On Tue, Jun 16, 2020 at 09:48:39AM +0800, Shengjiu Wang wrote:
> On Tue, Jun 16, 2020 at 7:11 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> >
> > On Mon, Jun 15, 2020 at 01:56:18PM +0800, Shengjiu Wang wrote:
> > > For mono channel, SSI will switch to Normal mode.
> > >
> > > In Normal mode and Network mode, the Word Length Control bits
> > > control the word length divider in clock generator, which is
> > > different with I2S Master mode (the word length is fixed to
> > > 32bit), it should be the value of params_width(hw_params).
> > >
> > > The condition "slots == 2" is not good for I2S Master mode,
> > > because for Network mode and Normal mode, the slots can also
> > > be 2. Then we need to use (ssi->i2s_net & SSI_SCR_I2S_MODE_MASK)
> > > to check if it is I2S Master mode.
> >
> > The fsl_ssi_set_bclk is only called when fsl_ssi_is_i2s_master,
> > though I agree that that line of comments sounds confusing now.
> 
> Actually I think fsl_ssi_is_i2s_master is not accurate, it just checks
> the Master mode,  but didn't check the I2S mode.
> 
> >
> > > So we refine the famula for mono channel, otherwise there
> >
> > famula => formula?
> >
> > > will be sound issue for S24_LE.
> > >
> > > Fixes: b0a7043d5c2c ("ASoC: fsl_ssi: Caculate bit clock rate using slot number and width")
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > ---
> > > changes in v2
> > > - refine patch for Network mode and Normal mode.
> > >
> > >  sound/soc/fsl/fsl_ssi.c | 15 +++++++++++----
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > > index bad89b0d129e..cbf67d132fda 100644
> > > --- a/sound/soc/fsl/fsl_ssi.c
> > > +++ b/sound/soc/fsl/fsl_ssi.c
> > > @@ -678,7 +678,8 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
> > >       struct regmap *regs = ssi->regs;
> > >       u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
> > >       unsigned long clkrate, baudrate, tmprate;
> > > -     unsigned int slots = params_channels(hw_params);
> > > +     unsigned int channels = params_channels(hw_params);
> > > +     unsigned int slots;
> > >       unsigned int slot_width = 32;
> > >       u64 sub, savesub = 100000;
> > >       unsigned int freq;
> > > @@ -688,9 +689,15 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
> > >       /* Override slots and slot_width if being specifically set... */
> > >       if (ssi->slots)
> > >               slots = ssi->slots;
> > > -     /* ...but keep 32 bits if slots is 2 -- I2S Master mode */
> > > -     if (ssi->slot_width && slots != 2)
> > > -             slot_width = ssi->slot_width;
> > > +     else
> > > +             /* Apply two slots for mono channel, because DC = 2 */
> > > +             slots = (channels == 1) ? 2 : channels;
> > > +
> > > +     /* ...but keep 32 bits if I2S Master mode */
> > > +     if ((ssi->i2s_net & SSI_SCR_I2S_MODE_MASK) != SSI_SCR_I2S_MODE_MASTER ||
> > > +         channels == 1)
> > > +             slot_width = ssi->slot_width ? ssi->slot_width :
> >
> > This looks very complicated...can you review and try mine?
> > (Basically, take 32-bit out of default but force it later)
> >
> > @@ -678,8 +678,9 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
> >         struct regmap *regs = ssi->regs;
> >         u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
> >         unsigned long clkrate, baudrate, tmprate;
> > -       unsigned int slots = params_channels(hw_params);
> > -       unsigned int slot_width = 32;
> > +       unsigned int channels = params_channels(hw_params);
> > +       unsigned int slot_width = params_width(hw_params);
> > +       unsigned int slots = 2;
> >         u64 sub, savesub = 100000;
> >         unsigned int freq;
> >         bool baudclk_is_used;
> > @@ -688,10 +689,16 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
> >         /* Override slots and slot_width if being specifically set... */
> >         if (ssi->slots)
> >                 slots = ssi->slots;
> > -       /* ...but keep 32 bits if slots is 2 -- I2S Master mode */
> > -       if (ssi->slot_width && slots != 2)
> > +       if (ssi->slot_width)
> >                 slot_width = ssi->slot_width;
> >
> > +       /*
> > +        * ...but force 32 bits for stereo audio. Note that mono audio is also
> > +        * sent in 2 slots via NORMAL mode, so check both slots and channels.
> > +        */
> > +       if (slots == 2 && channels == 2)
> > +               slot_width = 32;
> 
> slots ==2 && channels ==2 does not mean the I2S Master mode.
> For LEFT_J, it is also slots =2 & channels = 2, then the slot_width
> should be params_width(hw_params).
> and DSP_A/B also supports stereo.

I think you have a point. Then would this condition work?

+       /* ...but force 32 bits for stereo audio using I2S Master Mode */
+	if (channels == 2 &&
+	    ssi->i2s_net & SSI_SCR_I2S_MODE_MASK == SSI_SCR_I2S_MODE_MASTER)

Similar to yours but the code above it could look straightforward.

Re: [PATCH v2] ASoC: fsl_ssi: Fix bclk calculation for mono channel

[Shengjiu Wang]

On Tue, Jun 16, 2020 at 9:59 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Tue, Jun 16, 2020 at 09:48:39AM +0800, Shengjiu Wang wrote:
> > On Tue, Jun 16, 2020 at 7:11 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> > >
> > > On Mon, Jun 15, 2020 at 01:56:18PM +0800, Shengjiu Wang wrote:
> > > > For mono channel, SSI will switch to Normal mode.
> > > >
> > > > In Normal mode and Network mode, the Word Length Control bits
> > > > control the word length divider in clock generator, which is
> > > > different with I2S Master mode (the word length is fixed to
> > > > 32bit), it should be the value of params_width(hw_params).
> > > >
> > > > The condition "slots == 2" is not good for I2S Master mode,
> > > > because for Network mode and Normal mode, the slots can also
> > > > be 2. Then we need to use (ssi->i2s_net & SSI_SCR_I2S_MODE_MASK)
> > > > to check if it is I2S Master mode.
> > >
> > > The fsl_ssi_set_bclk is only called when fsl_ssi_is_i2s_master,
> > > though I agree that that line of comments sounds confusing now.
> >
> > Actually I think fsl_ssi_is_i2s_master is not accurate, it just checks
> > the Master mode,  but didn't check the I2S mode.
> >
> > >
> > > > So we refine the famula for mono channel, otherwise there
> > >
> > > famula => formula?
> > >
> > > > will be sound issue for S24_LE.
> > > >
> > > > Fixes: b0a7043d5c2c ("ASoC: fsl_ssi: Caculate bit clock rate using slot number and width")
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > > ---
> > > > changes in v2
> > > > - refine patch for Network mode and Normal mode.
> > > >
> > > >  sound/soc/fsl/fsl_ssi.c | 15 +++++++++++----
> > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > > > index bad89b0d129e..cbf67d132fda 100644
> > > > --- a/sound/soc/fsl/fsl_ssi.c
> > > > +++ b/sound/soc/fsl/fsl_ssi.c
> > > > @@ -678,7 +678,8 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
> > > >       struct regmap *regs = ssi->regs;
> > > >       u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
> > > >       unsigned long clkrate, baudrate, tmprate;
> > > > -     unsigned int slots = params_channels(hw_params);
> > > > +     unsigned int channels = params_channels(hw_params);
> > > > +     unsigned int slots;
> > > >       unsigned int slot_width = 32;
> > > >       u64 sub, savesub = 100000;
> > > >       unsigned int freq;
> > > > @@ -688,9 +689,15 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
> > > >       /* Override slots and slot_width if being specifically set... */
> > > >       if (ssi->slots)
> > > >               slots = ssi->slots;
> > > > -     /* ...but keep 32 bits if slots is 2 -- I2S Master mode */
> > > > -     if (ssi->slot_width && slots != 2)
> > > > -             slot_width = ssi->slot_width;
> > > > +     else
> > > > +             /* Apply two slots for mono channel, because DC = 2 */
> > > > +             slots = (channels == 1) ? 2 : channels;
> > > > +
> > > > +     /* ...but keep 32 bits if I2S Master mode */
> > > > +     if ((ssi->i2s_net & SSI_SCR_I2S_MODE_MASK) != SSI_SCR_I2S_MODE_MASTER ||
> > > > +         channels == 1)
> > > > +             slot_width = ssi->slot_width ? ssi->slot_width :
> > >
> > > This looks very complicated...can you review and try mine?
> > > (Basically, take 32-bit out of default but force it later)
> > >
> > > @@ -678,8 +678,9 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
> > >         struct regmap *regs = ssi->regs;
> > >         u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
> > >         unsigned long clkrate, baudrate, tmprate;
> > > -       unsigned int slots = params_channels(hw_params);
> > > -       unsigned int slot_width = 32;
> > > +       unsigned int channels = params_channels(hw_params);
> > > +       unsigned int slot_width = params_width(hw_params);
> > > +       unsigned int slots = 2;
> > >         u64 sub, savesub = 100000;
> > >         unsigned int freq;
> > >         bool baudclk_is_used;
> > > @@ -688,10 +689,16 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream *substream,
> > >         /* Override slots and slot_width if being specifically set... */
> > >         if (ssi->slots)
> > >                 slots = ssi->slots;
> > > -       /* ...but keep 32 bits if slots is 2 -- I2S Master mode */
> > > -       if (ssi->slot_width && slots != 2)
> > > +       if (ssi->slot_width)
> > >                 slot_width = ssi->slot_width;
> > >
> > > +       /*
> > > +        * ...but force 32 bits for stereo audio. Note that mono audio is also
> > > +        * sent in 2 slots via NORMAL mode, so check both slots and channels.
> > > +        */
> > > +       if (slots == 2 && channels == 2)
> > > +               slot_width = 32;
> >
> > slots ==2 && channels ==2 does not mean the I2S Master mode.
> > For LEFT_J, it is also slots =2 & channels = 2, then the slot_width
> > should be params_width(hw_params).
> > and DSP_A/B also supports stereo.
>
> I think you have a point. Then would this condition work?
>
> +       /* ...but force 32 bits for stereo audio using I2S Master Mode */
> +       if (channels == 2 &&
> +           ssi->i2s_net & SSI_SCR_I2S_MODE_MASK == SSI_SCR_I2S_MODE_MASTER)
>
> Similar to yours but the code above it could look straightforward.

I think it should work.