[PATCH 0/3] tlv320aic3xx4 updates

[PATCH 0/3] tlv320aic3xx4 updates

[Miquel Raynal]

Hello,

While doing a kernel update on a sama5-based board I figured out the
sound system was not working anymore, the debug session led me to the
following commits. As I am not an audio expert at all, I am fully open
to comments and suggestions.

Thanks,
Miquèl

Miquel Raynal (3):
  ASoC: tlv320aic32x4: Ensure a minimum delay before clock stabilization
  ASoC: tlv320aic32x4: Fix bdiv clock rate derivation
  ASoC: tlv320aic32x4: Enable fast charge

 sound/soc/codecs/tlv320aic32x4-clk.c |  9 ++++++++-
 sound/soc/codecs/tlv320aic32x4.c     | 17 ++++++++++++++---
 sound/soc/codecs/tlv320aic32x4.h     |  7 +++++++
 3 files changed, 29 insertions(+), 4 deletions(-)

-- 
2.20.1

[PATCH 1/3] ASoC: tlv320aic32x4: Ensure a minimum delay before clock stabilization

[Miquel Raynal]

As indicated in the datasheet, a 10ms delay must be observed after
programming the divisors.

The lack of delay prevents the codec to work properly and the playback
appears extremely slow and totally un-audible on a custom sama5 based
board.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 sound/soc/codecs/tlv320aic32x4-clk.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/tlv320aic32x4-clk.c b/sound/soc/codecs/tlv320aic32x4-clk.c
index 156c153c12ab..2f78e6820c75 100644
--- a/sound/soc/codecs/tlv320aic32x4-clk.c
+++ b/sound/soc/codecs/tlv320aic32x4-clk.c
@@ -230,7 +230,14 @@ static int clk_aic32x4_pll_set_rate(struct clk_hw *hw,
 	if (ret < 0)
 		return -EINVAL;
 
-	return clk_aic32x4_pll_set_muldiv(pll, &settings);
+	ret = clk_aic32x4_pll_set_muldiv(pll, &settings);
+	if (ret)
+		return ret;
+
+	/* 10ms is the delay to wait before the clocks are stable */
+	msleep(10);
+
+	return 0;
 }
 
 static int clk_aic32x4_pll_set_parent(struct clk_hw *hw, u8 index)
-- 
2.20.1

[PATCH 2/3] ASoC: tlv320aic32x4: Fix bdiv clock rate derivation

[Miquel Raynal]

Current code expects a single channel to be always used. Fix this
situation by forwarding the number of channels used. Then fix the
derivation of the bdiv clock rate.

Fixes: 96c3bb00239d ("ASoC: tlv320aic32x4: Dynamically Determine Clocking")
Suggested-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 sound/soc/codecs/tlv320aic32x4.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic32x4.c b/sound/soc/codecs/tlv320aic32x4.c
index 68165de1c8de..7a1ffbaf48be 100644
--- a/sound/soc/codecs/tlv320aic32x4.c
+++ b/sound/soc/codecs/tlv320aic32x4.c
@@ -662,7 +662,7 @@ static int aic32x4_set_processing_blocks(struct snd_soc_component *component,
 }
 
 static int aic32x4_setup_clocks(struct snd_soc_component *component,
-				unsigned int sample_rate)
+				unsigned int sample_rate, unsigned int channels)
 {
 	u8 aosr;
 	u16 dosr;
@@ -750,7 +750,9 @@ static int aic32x4_setup_clocks(struct snd_soc_component *component,
 							dosr);
 
 						clk_set_rate(clocks[5].clk,
-							sample_rate * 32);
+							sample_rate * 32 *
+							channels);
+
 						return 0;
 					}
 				}
@@ -772,7 +774,8 @@ static int aic32x4_hw_params(struct snd_pcm_substream *substream,
 	u8 iface1_reg = 0;
 	u8 dacsetup_reg = 0;
 
-	aic32x4_setup_clocks(component, params_rate(params));
+	aic32x4_setup_clocks(component, params_rate(params),
+			     params_channels(params));
 
 	switch (params_width(params)) {
 	case 16:
-- 
2.20.1

[PATCH 3/3] ASoC: tlv320aic32x4: Enable fast charge

[Miquel Raynal]

At power-up the analog circuits may take up to one full second before
being charged with the default configuration. Using the analog blocks
before they are ready generates a *very* crappy sound.

Enable the fast charge feature, which will require a bit more power
than normal charge but will definitely speed up the starting operation
by shrinking this delay to up to 40 ms.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 sound/soc/codecs/tlv320aic32x4.c | 8 ++++++++
 sound/soc/codecs/tlv320aic32x4.h | 7 +++++++
 2 files changed, 15 insertions(+)

diff --git a/sound/soc/codecs/tlv320aic32x4.c b/sound/soc/codecs/tlv320aic32x4.c
index 7a1ffbaf48be..5fb8ba109bc9 100644
--- a/sound/soc/codecs/tlv320aic32x4.c
+++ b/sound/soc/codecs/tlv320aic32x4.c
@@ -1009,6 +1009,14 @@ static int aic32x4_component_probe(struct snd_soc_component *component)
 				AIC32X4_LADC_EN | AIC32X4_RADC_EN);
 	snd_soc_component_write(component, AIC32X4_ADCSETUP, tmp_reg);
 
+	/*
+	 * Enable the fast charging feature and ensure the needed 40ms ellapsed
+	 * before using the analog circuits.
+	 */
+	snd_soc_component_write(component, AIC32X4_REFPOWERUP,
+				AIC32X4_REFPOWERUP_40MS);
+	msleep(40);
+
 	return 0;
 }
 
diff --git a/sound/soc/codecs/tlv320aic32x4.h b/sound/soc/codecs/tlv320aic32x4.h
index 38f47704bb75..7550122e9f8a 100644
--- a/sound/soc/codecs/tlv320aic32x4.h
+++ b/sound/soc/codecs/tlv320aic32x4.h
@@ -96,6 +96,7 @@ int aic32x4_register_clocks(struct device *dev, const char *mclk_name);
 #define AIC32X4_FLOATINGINPUT	AIC32X4_REG(1, 58)
 #define AIC32X4_LMICPGAVOL	AIC32X4_REG(1, 59)
 #define AIC32X4_RMICPGAVOL	AIC32X4_REG(1, 60)
+#define AIC32X4_REFPOWERUP	AIC32X4_REG(1, 123)
 
 /* Bits, masks, and shifts */
 
@@ -205,6 +206,12 @@ int aic32x4_register_clocks(struct device *dev, const char *mclk_name);
 #define AIC32X4_RMICPGANIN_IN1L_10K	0x10
 #define AIC32X4_RMICPGANIN_CM1R_10K	0x40
 
+/* AIC32X4_REFPOWERUP */
+#define AIC32X4_REFPOWERUP_SLOW		0x04
+#define AIC32X4_REFPOWERUP_40MS		0x05
+#define AIC32X4_REFPOWERUP_80MS		0x06
+#define AIC32X4_REFPOWERUP_120MS	0x07
+
 /* Common mask and enable for all of the dividers */
 #define AIC32X4_DIVEN           BIT(7)
 #define AIC32X4_DIV_MASK        GENMASK(6, 0)
-- 
2.20.1

Re: [PATCH 3/3] ASoC: tlv320aic32x4: Enable fast charge

[Alexandre Belloni]

Hi,

On 11/09/2020 19:31:40+0200, Miquel Raynal wrote:
> At power-up the analog circuits may take up to one full second before
> being charged with the default configuration. Using the analog blocks
> before they are ready generates a *very* crappy sound.
> 
> Enable the fast charge feature, which will require a bit more power
> than normal charge but will definitely speed up the starting operation
> by shrinking this delay to up to 40 ms.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  sound/soc/codecs/tlv320aic32x4.c | 8 ++++++++
>  sound/soc/codecs/tlv320aic32x4.h | 7 +++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/sound/soc/codecs/tlv320aic32x4.c b/sound/soc/codecs/tlv320aic32x4.c
> index 7a1ffbaf48be..5fb8ba109bc9 100644
> --- a/sound/soc/codecs/tlv320aic32x4.c
> +++ b/sound/soc/codecs/tlv320aic32x4.c
> @@ -1009,6 +1009,14 @@ static int aic32x4_component_probe(struct snd_soc_component *component)
>  				AIC32X4_LADC_EN | AIC32X4_RADC_EN);
>  	snd_soc_component_write(component, AIC32X4_ADCSETUP, tmp_reg);
>  
> +	/*
> +	 * Enable the fast charging feature and ensure the needed 40ms ellapsed
> +	 * before using the analog circuits.
> +	 */
> +	snd_soc_component_write(component, AIC32X4_REFPOWERUP,
> +				AIC32X4_REFPOWERUP_40MS);
> +	msleep(40);
> +

Maybe the actual REFPOWERUP value could be exposed as a control so
userspace has a way to set the policy? 

I'm not sure it make sense to have the delay in probe because it is not
enable the analog part of the codec. The delay should probable be after
the clocks have been set up because the datasheet says that it is mdac
and madc that is starting the analog circuitry.

>  	return 0;
>  }
>  
> diff --git a/sound/soc/codecs/tlv320aic32x4.h b/sound/soc/codecs/tlv320aic32x4.h
> index 38f47704bb75..7550122e9f8a 100644
> --- a/sound/soc/codecs/tlv320aic32x4.h
> +++ b/sound/soc/codecs/tlv320aic32x4.h
> @@ -96,6 +96,7 @@ int aic32x4_register_clocks(struct device *dev, const char *mclk_name);
>  #define AIC32X4_FLOATINGINPUT	AIC32X4_REG(1, 58)
>  #define AIC32X4_LMICPGAVOL	AIC32X4_REG(1, 59)
>  #define AIC32X4_RMICPGAVOL	AIC32X4_REG(1, 60)
> +#define AIC32X4_REFPOWERUP	AIC32X4_REG(1, 123)
>  
>  /* Bits, masks, and shifts */
>  
> @@ -205,6 +206,12 @@ int aic32x4_register_clocks(struct device *dev, const char *mclk_name);
>  #define AIC32X4_RMICPGANIN_IN1L_10K	0x10
>  #define AIC32X4_RMICPGANIN_CM1R_10K	0x40
>  
> +/* AIC32X4_REFPOWERUP */
> +#define AIC32X4_REFPOWERUP_SLOW		0x04
> +#define AIC32X4_REFPOWERUP_40MS		0x05
> +#define AIC32X4_REFPOWERUP_80MS		0x06
> +#define AIC32X4_REFPOWERUP_120MS	0x07
> +
>  /* Common mask and enable for all of the dividers */
>  #define AIC32X4_DIVEN           BIT(7)
>  #define AIC32X4_DIV_MASK        GENMASK(6, 0)
> -- 
> 2.20.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 3/3] ASoC: tlv320aic32x4: Enable fast charge

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

On Tue, Sep 15, 2020 at 10:26:02AM +0200, Alexandre Belloni wrote:
> On 11/09/2020 19:31:40+0200, Miquel Raynal wrote:

> > +	/*
> > +	 * Enable the fast charging feature and ensure the needed 40ms ellapsed
> > +	 * before using the analog circuits.
> > +	 */
> > +	snd_soc_component_write(component, AIC32X4_REFPOWERUP,
> > +				AIC32X4_REFPOWERUP_40MS);
> > +	msleep(40);
> > +

> Maybe the actual REFPOWERUP value could be exposed as a control so
> userspace has a way to set the policy? 

We very rarely do this, there's not usially anything 

> I'm not sure it make sense to have the delay in probe because it is not
> enable the analog part of the codec. The delay should probable be after
> the clocks have been set up because the datasheet says that it is mdac
> and madc that is starting the analog circuitry.

Deferring the delay to a workqueue is the usual thing where there's
concerns about slowing down boot.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] ASoC: tlv320aic32x4: Enable fast charge

[Alexandre Belloni]

On 15/09/2020 12:50:34+0100, Mark Brown wrote:
> On Tue, Sep 15, 2020 at 10:26:02AM +0200, Alexandre Belloni wrote:
> > On 11/09/2020 19:31:40+0200, Miquel Raynal wrote:
> 
> > > +	/*
> > > +	 * Enable the fast charging feature and ensure the needed 40ms ellapsed
> > > +	 * before using the analog circuits.
> > > +	 */
> > > +	snd_soc_component_write(component, AIC32X4_REFPOWERUP,
> > > +				AIC32X4_REFPOWERUP_40MS);
> > > +	msleep(40);
> > > +
> 
> > Maybe the actual REFPOWERUP value could be exposed as a control so
> > userspace has a way to set the policy? 
> 
> We very rarely do this, there's not usially anything 
> 

Could you suggest something then? This mainly changes the power
codec power consumption. I guess people will want to trade latency
for less consumption.

> > I'm not sure it make sense to have the delay in probe because it is not
> > enable the analog part of the codec. The delay should probable be after
> > the clocks have been set up because the datasheet says that it is mdac
> > and madc that is starting the analog circuitry.
> 
> Deferring the delay to a workqueue is the usual thing where there's
> concerns about slowing down boot.

Well, that was not my concern. I didn't realize Miquel actually used the
Force power-up values and though the actual power up happened after
configuring the clocks (as is the case for the dafule values). In this
case, the delay is at the proper location.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 3/3] ASoC: tlv320aic32x4: Enable fast charge

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

On Tue, Sep 15, 2020 at 03:02:07PM +0200, Alexandre Belloni wrote:
> On 15/09/2020 12:50:34+0100, Mark Brown wrote:
> > On Tue, Sep 15, 2020 at 10:26:02AM +0200, Alexandre Belloni wrote:
> > > On 11/09/2020 19:31:40+0200, Miquel Raynal wrote:

> > > > +	/*
> > > > +	 * Enable the fast charging feature and ensure the needed 40ms ellapsed
> > > > +	 * before using the analog circuits.
> > > > +	 */
> > > > +	snd_soc_component_write(component, AIC32X4_REFPOWERUP,
> > > > +				AIC32X4_REFPOWERUP_40MS);
> > > > +	msleep(40);
> > > > +

> > > Maybe the actual REFPOWERUP value could be exposed as a control so
> > > userspace has a way to set the policy? 

> > We very rarely do this, there's not usially anything 

> Could you suggest something then? This mainly changes the power
> codec power consumption. I guess people will want to trade latency
> for less consumption.

Is it increasing steady state power consumption or is it just drawing
more power during the ramp (ie, peak current consumption is bigger)?
Usually this is trading off clean ramps for fast ramps rather than
affecting steady state.  If it's affecting steady state a control seems
sensible.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] ASoC: tlv320aic32x4: Enable fast charge

[Miquel Raynal]

Hi Mark,

Mark Brown <broonie@kernel.org> wrote on Tue, 15 Sep 2020 15:10:25
+0100:

> On Tue, Sep 15, 2020 at 03:02:07PM +0200, Alexandre Belloni wrote:
> > On 15/09/2020 12:50:34+0100, Mark Brown wrote:  
> > > On Tue, Sep 15, 2020 at 10:26:02AM +0200, Alexandre Belloni wrote:  
> > > > On 11/09/2020 19:31:40+0200, Miquel Raynal wrote:  
> 
> > > > > +	/*
> > > > > +	 * Enable the fast charging feature and ensure the needed 40ms ellapsed
> > > > > +	 * before using the analog circuits.
> > > > > +	 */
> > > > > +	snd_soc_component_write(component, AIC32X4_REFPOWERUP,
> > > > > +				AIC32X4_REFPOWERUP_40MS);
> > > > > +	msleep(40);
> > > > > +  
> 
> > > > Maybe the actual REFPOWERUP value could be exposed as a control so
> > > > userspace has a way to set the policy?   
> 
> > > We very rarely do this, there's not usially anything   
> 
> > Could you suggest something then? This mainly changes the power
> > codec power consumption. I guess people will want to trade latency
> > for less consumption.  
> 
> Is it increasing steady state power consumption or is it just drawing
> more power during the ramp (ie, peak current consumption is bigger)?
> Usually this is trading off clean ramps for fast ramps rather than
> affecting steady state.  If it's affecting steady state a control seems
> sensible.

Indeed, it is just affecting the ramp (peak current is bigger).

Thanks,
Miquèl

Re: [PATCH 3/3] ASoC: tlv320aic32x4: Enable fast charge

[Alexandre Belloni]

On 15/09/2020 16:14:01+0200, Miquel Raynal wrote:
> Mark Brown <broonie@kernel.org> wrote on Tue, 15 Sep 2020 15:10:25
> +0100:
> 
> > On Tue, Sep 15, 2020 at 03:02:07PM +0200, Alexandre Belloni wrote:
> > > On 15/09/2020 12:50:34+0100, Mark Brown wrote:  
> > > > On Tue, Sep 15, 2020 at 10:26:02AM +0200, Alexandre Belloni wrote:  
> > > > > On 11/09/2020 19:31:40+0200, Miquel Raynal wrote:  
> > 
> > > > > > +	/*
> > > > > > +	 * Enable the fast charging feature and ensure the needed 40ms ellapsed
> > > > > > +	 * before using the analog circuits.
> > > > > > +	 */
> > > > > > +	snd_soc_component_write(component, AIC32X4_REFPOWERUP,
> > > > > > +				AIC32X4_REFPOWERUP_40MS);
> > > > > > +	msleep(40);
> > > > > > +  
> > 
> > > > > Maybe the actual REFPOWERUP value could be exposed as a control so
> > > > > userspace has a way to set the policy?   
> > 
> > > > We very rarely do this, there's not usially anything   
> > 
> > > Could you suggest something then? This mainly changes the power
> > > codec power consumption. I guess people will want to trade latency
> > > for less consumption.  
> > 
> > Is it increasing steady state power consumption or is it just drawing
> > more power during the ramp (ie, peak current consumption is bigger)?
> > Usually this is trading off clean ramps for fast ramps rather than
> > affecting steady state.  If it's affecting steady state a control seems
> > sensible.
> 
> Indeed, it is just affecting the ramp (peak current is bigger).
> 

However, forcing powerup at probe time versus at play time means that
you consume power even when not playing audio.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 3/3] ASoC: tlv320aic32x4: Enable fast charge

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

On Tue, Sep 15, 2020 at 04:27:43PM +0200, Alexandre Belloni wrote:
> On 15/09/2020 16:14:01+0200, Miquel Raynal wrote:

> > Indeed, it is just affecting the ramp (peak current is bigger).

> However, forcing powerup at probe time versus at play time means that
> you consume power even when not playing audio.

With older VMID referenced devices like this one seems to be there's
usually not really any good tradeoffs.  You have to balance audible
noise on the outputs when starting audio, idle power consumption and the
time taken to get the device live either on starting a stream or boot.
Generally for a given device there's a fairly clear optimum and trying
to support multiple options makes things more complex and less robust.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] tlv320aic3xx4 updates

[Mark Brown]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

On Fri, 11 Sep 2020 19:31:37 +0200, Miquel Raynal wrote:
> While doing a kernel update on a sama5-based board I figured out the
> sound system was not working anymore, the debug session led me to the
> following commits. As I am not an audio expert at all, I am fully open
> to comments and suggestions.
> 
> Thanks,
> Miquèl
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/3] ASoC: tlv320aic32x4: Ensure a minimum delay before clock stabilization
      commit: 5b4458ebb4c8007dae7eaeb88cb52b2bb4879894
[2/3] ASoC: tlv320aic32x4: Fix bdiv clock rate derivation
      commit: 40b37136287ba6b34aa2f1f6123f3d6d205dc2f0
[3/3] ASoC: tlv320aic32x4: Enable fast charge
      commit: ec96690de82cee2cb028c07b1e72cb4a446ad03a

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark