Re: [PATCH] ASoC: bcm2835: Add enable/disable clock functions

[Allen Martin]

On Wed, Oct 28, 2020 at 10:39:12AM +0100, Matthias Reichl wrote:
> On Wed, Oct 28, 2020 at 01:18:33AM -0700, Allen Martin wrote:
> > Hi, just checking if you had a chance to review this patch.
> >
> > On Sat, Oct 10, 2020 at 12:26 PM Allen Martin <armartin@xxxxxxxxx> wrote:
> >
> > > Add functions to control enable/disable of BCLK output of bcm2835 I2S
> > > controller so that BCLK output only starts when dma starts.  This
> > > resolves issues of audio pop with DACs such as max98357 on rpi.  The
> > > LRCLK output of bcm2835 only starts when the frame size has been
> > > configured and there is data in the FIFO.  The max98357 dac makes a
> > > loud popping sound when BCLK is toggling but LRCLK is not.
>
> I'm afraid that changing the clocking in the way you proposed has a high
> potential of breaking existing setups which need bclk to be present
> after prepare(). And it complicates the already rather convoluted
> clock setup even more. So I don't think this patch should be applied.
>
> Since you mentioned max98357: have you properly connected and configured
> the sd-mode GPIO? This chip has very strict timing requirements and is
> known to "pop" without sd-mode wired up - see the datasheet and devicetree
> binding docs.

The board I'm testing on is this: https://www.adafruit.com/product/3346
which does not have SD_MODE wired to GPIO (schematic is here:
https://www.adafruit.com/product/3346).  I agree this should ideally
be wired to GPIO as described in the max98357 datasheet to enable the
click and pop suppression feature, however there are still problems
with the clock initialization this patch addresses:

1) In bcm2835_i2s_hw_params() BCLK is enabled before the FIFO is
cleared causing residual data in the FIFO to be transmitted when BCLK
is initialized.

2) Also in bcm2835_i2s_hw_params() BCLK is enabled before the frame
size is configured in MODE_A or DMA is initialized.  This causes the
i2s controller to transmit a data frame many thousands of bits long,
violating the 0.5*BCLK < t < 0.5*LRCLK requirement of the max98357
datasheet.

I think the driver should separate clock initialization from output
enable and only start trasmitting once everything is initialized and
there is data to transmit.

Do you have more details about what setups require BCLK output after
prepare()?  I have access to a PCM5101A DAC, but I have not tested it
yet.

-Allen


> > >
> > > Signed-off-by: Allen Martin <armartin@xxxxxxxxx>
> > > ---
> > >  sound/soc/bcm/bcm2835-i2s.c | 35 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > >
> > > diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c
> > > index e6a12e271b07..5c8649864c0d 100644
> > > --- a/sound/soc/bcm/bcm2835-i2s.c
> > > +++ b/sound/soc/bcm/bcm2835-i2s.c
> > > @@ -122,9 +122,27 @@ struct bcm2835_i2s_dev {
> > >         struct regmap                           *i2s_regmap;
> > >         struct clk                              *clk;
> > >         bool                                    clk_prepared;
> > > +       bool                                    clk_enabled;
> > >         int                                     clk_rate;
> > >  };
> > >
> > > +static void bcm2835_i2s_enable_clock(struct bcm2835_i2s_dev *dev)
> > > +{
> > > +       if (dev->clk_enabled)
> > > +               return;
> > > +
> > > +       regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_MODE_A_REG,
> > > BCM2835_I2S_CLKDIS, 0);
> > > +       dev->clk_enabled = true;
> > > +}
> > > +
> > > +static void bcm2835_i2s_disable_clock(struct bcm2835_i2s_dev *dev)
> > > +{
> > > +       if (dev->clk_enabled)
> > > +               regmap_update_bits(dev->i2s_regmap,
> > > BCM2835_I2S_MODE_A_REG, BCM2835_I2S_CLKDIS, BCM2835_I2S_CLKDIS);
> > > +
> > > +       dev->clk_enabled = false;
> > > +}
> > > +
> > >  static void bcm2835_i2s_start_clock(struct bcm2835_i2s_dev *dev)
> > >  {
> > >         unsigned int master = dev->fmt & SND_SOC_DAIFMT_MASTER_MASK;
> > > @@ -145,6 +163,7 @@ static void bcm2835_i2s_start_clock(struct
> > > bcm2835_i2s_dev *dev)
> > >
> > >  static void bcm2835_i2s_stop_clock(struct bcm2835_i2s_dev *dev)
> > >  {
> > > +       bcm2835_i2s_disable_clock(dev);
> > >         if (dev->clk_prepared)
> > >                 clk_disable_unprepare(dev->clk);
> > >         dev->clk_prepared = false;
> > > @@ -158,6 +177,7 @@ static void bcm2835_i2s_clear_fifos(struct
> > > bcm2835_i2s_dev *dev,
> > >         uint32_t csreg;
> > >         uint32_t i2s_active_state;
> > >         bool clk_was_prepared;
> > > +       bool clk_was_enabled;
> > >         uint32_t off;
> > >         uint32_t clr;
> > >
> > > @@ -176,6 +196,11 @@ static void bcm2835_i2s_clear_fifos(struct
> > > bcm2835_i2s_dev *dev,
> > >         if (!clk_was_prepared)
> > >                 bcm2835_i2s_start_clock(dev);
> > >
> > > +       /* Enable clock if not enabled */
> > > +       clk_was_enabled = dev->clk_enabled;
> > > +       if (!clk_was_enabled)
> > > +               bcm2835_i2s_enable_clock(dev);
> > > +
> > >         /* Stop I2S module */
> > >         regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, off, 0);
> > >
> > > @@ -207,6 +232,10 @@ static void bcm2835_i2s_clear_fifos(struct
> > > bcm2835_i2s_dev *dev,
> > >         if (!timeout)
> > >                 dev_err(dev->dev, "I2S SYNC error!\n");
> > >
> > > +       /* Disable clock if it was not enabled before */
> > > +       if (!clk_was_enabled)
> > > +               bcm2835_i2s_disable_clock(dev);
> > > +
> > >         /* Stop clock if it was not running before */
> > >         if (!clk_was_prepared)
> > >                 bcm2835_i2s_stop_clock(dev);
> > > @@ -414,6 +443,8 @@ static int bcm2835_i2s_hw_params(struct
> > > snd_pcm_substream *substream,
> > >         /* Clock should only be set up here if CPU is clock master */
> > >         if (bit_clock_master &&
> > >             (!dev->clk_prepared || dev->clk_rate != bclk_rate)) {
> > > +               if (dev->clk_enabled)
> > > +                       bcm2835_i2s_disable_clock(dev);
> > >                 if (dev->clk_prepared)
> > >                         bcm2835_i2s_stop_clock(dev);
> > >
> > > @@ -534,6 +565,8 @@ static int bcm2835_i2s_hw_params(struct
> > > snd_pcm_substream *substream,
> > >                 mode |= BCM2835_I2S_FTXP | BCM2835_I2S_FRXP;
> > >         }
> > >
> > > +       if (!dev->clk_enabled)
> > > +               mode |= BCM2835_I2S_CLKDIS;
> > >         mode |= BCM2835_I2S_FLEN(frame_length - 1);
> > >         mode |= BCM2835_I2S_FSLEN(framesync_length);
> > >
> > > @@ -668,6 +701,7 @@ static int bcm2835_i2s_trigger(struct
> > > snd_pcm_substream *substream, int cmd,
> > >         case SNDRV_PCM_TRIGGER_RESUME:
> > >         case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> > >                 bcm2835_i2s_start_clock(dev);
> > > +               bcm2835_i2s_enable_clock(dev);
> > >
> > >                 if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> > >                         mask = BCM2835_I2S_RXON;
> > > @@ -839,6 +873,7 @@ static int bcm2835_i2s_probe(struct platform_device
> > > *pdev)
> > >
> > >         /* get the clock */
> > >         dev->clk_prepared = false;
> > > +       dev->clk_enabled = false;
> > >         dev->clk = devm_clk_get(&pdev->dev, NULL);
> > >         if (IS_ERR(dev->clk)) {
> > >                 dev_err(&pdev->dev, "could not get clk: %ld\n",
> > > --
> > > 2.20.1
> > >
> > >