* [PATCH] ASoC: bcm2835: Add enable/disable clock functions
@ 2020-10-10 19:26 Allen Martin
[not found] ` <CABfSH=oUfY=+eDDwccxMu724YAGOAf2kskZxvibxwOhYrd0_jQ@mail.gmail.com>
0 siblings, 1 reply; 3+ messages in thread
From: Allen Martin @ 2020-10-10 19:26 UTC (permalink / raw)
To: hias, phil; +Cc: linux-rpi-kernel, linux-kernel, Allen Martin
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.
Signed-off-by: Allen Martin <armartin@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ASoC: bcm2835: Add enable/disable clock functions
[not found] ` <CABfSH=oUfY=+eDDwccxMu724YAGOAf2kskZxvibxwOhYrd0_jQ@mail.gmail.com>
@ 2020-10-28 9:39 ` Matthias Reichl
2020-10-30 0:05 ` Allen Martin
0 siblings, 1 reply; 3+ messages in thread
From: Matthias Reichl @ 2020-10-28 9:39 UTC (permalink / raw)
To: Allen Martin; +Cc: phil, linux-rpi-kernel, linux-kernel
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@gmail.com> 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@gmail.com>
> > ---
> > 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
> >
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ASoC: bcm2835: Add enable/disable clock functions
2020-10-28 9:39 ` Matthias Reichl
@ 2020-10-30 0:05 ` Allen Martin
0 siblings, 0 replies; 3+ messages in thread
From: Allen Martin @ 2020-10-30 0:05 UTC (permalink / raw)
To: Matthias Reichl, phil, linux-rpi-kernel, linux-kernel
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@gmail.com> 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@gmail.com>
>> > ---
>> > 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
>> >
>> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-30 0:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-10 19:26 [PATCH] ASoC: bcm2835: Add enable/disable clock functions Allen Martin
[not found] ` <CABfSH=oUfY=+eDDwccxMu724YAGOAf2kskZxvibxwOhYrd0_jQ@mail.gmail.com>
2020-10-28 9:39 ` Matthias Reichl
2020-10-30 0:05 ` Allen Martin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).