| Message ID | 20200911173140.29984-4-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ec96690de82cee2cb028c07b1e72cb4a446ad03a |
| Headers | show |
| Series | tlv320aic3xx4 updates | expand |
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 >
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.
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.
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.
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
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.
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.
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)
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(+)