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

[Matthias Reichl]

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.

so long,

Hias

> >
> > 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
> >
> >