[PATCHv1,5/5] ASoC: da7213: add default clock handling
diff mbox series

Message ID 20191108174843.11227-6-sebastian.reichel@collabora.com
State New
Headers show
Series
  • ASoC: da7213: support for usage with simple-card
Related show

Commit Message

Sebastian Reichel Nov. 8, 2019, 5:48 p.m. UTC
This adds default clock/PLL configuration to the driver
for usage with generic drivers like simple-card for usage
with a fixed rate clock.

Upstreaming this requires a good way to disable the automatic
clock handling for systems doing it manually to avoid breaking
existing setups.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 sound/soc/codecs/da7213.c | 34 +++++++++++++++++++++++++++++++++-
 sound/soc/codecs/da7213.h |  1 +
 2 files changed, 34 insertions(+), 1 deletion(-)

Comments

Adam Thomson Nov. 11, 2019, 2:20 p.m. UTC | #1
On 08 November 2019 17:49, Sebastian Reichel wrote:

> This adds default clock/PLL configuration to the driver
> for usage with generic drivers like simple-card for usage
> with a fixed rate clock.
> 
> Upstreaming this requires a good way to disable the automatic
> clock handling for systems doing it manually to avoid breaking
> existing setups.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  sound/soc/codecs/da7213.c | 34 +++++++++++++++++++++++++++++++++-
>  sound/soc/codecs/da7213.h |  1 +
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> index 197609691525..a4ed382ddfc7 100644
> --- a/sound/soc/codecs/da7213.c
> +++ b/sound/soc/codecs/da7213.c
> @@ -1163,6 +1163,8 @@ static int da7213_hw_params(struct
> snd_pcm_substream *substream,
>  			    struct snd_soc_dai *dai)
>  {
>  	struct snd_soc_component *component = dai->component;
> +	struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> +	int freq = 0;
>  	u8 dai_ctrl = 0;
>  	u8 fs;
> 
> @@ -1188,38 +1190,54 @@ static int da7213_hw_params(struct
> snd_pcm_substream *substream,
>  	switch (params_rate(params)) {
>  	case 8000:
>  		fs = DA7213_SR_8000;
> +		freq = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 11025:
>  		fs = DA7213_SR_11025;
> +		freq = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 12000:
>  		fs = DA7213_SR_12000;
> +		freq = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 16000:
>  		fs = DA7213_SR_16000;
> +		freq = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 22050:
>  		fs = DA7213_SR_22050;
> +		freq = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 32000:
>  		fs = DA7213_SR_32000;
> +		freq = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 44100:
>  		fs = DA7213_SR_44100;
> +		freq = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 48000:
>  		fs = DA7213_SR_48000;
> +		freq = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 88200:
>  		fs = DA7213_SR_88200;
> +		freq = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 96000:
>  		fs = DA7213_SR_96000;
> +		freq = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
> 
> +	/* setup PLL */
> +	if (da7213->fixed_clk_auto) {
> +		snd_soc_component_set_pll(component, 0,
> DA7213_SYSCLK_PLL,
> +					  da7213->mclk_rate, freq);
> +	}
> +

Are we happy with the PLL being always enabled? Seems like a power drain,
especially if you're using an MCLK which is a natural harmonic for the SR in 
question in which case the PLL can be bypassed. Also the bias level function in
this driver will enable and disable the MCLK, if it has been provided, so it's a
bit strange to have the PLL enabled but it's clock source taken away.

>  	snd_soc_component_update_bits(component, DA7213_DAI_CTRL,
> DA7213_DAI_WORD_LENGTH_MASK,
>  			    dai_ctrl);
>  	snd_soc_component_write(component, DA7213_SR, fs);
> @@ -1700,10 +1718,10 @@ static struct da7213_platform_data
>  	return pdata;
>  }
> 
> -
>  static int da7213_probe(struct snd_soc_component *component)
>  {
>  	struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> +	int ret;
> 
>  	pm_runtime_get_sync(component->dev);
> 
> @@ -1836,6 +1854,20 @@ static int da7213_probe(struct snd_soc_component
> *component)
>  			return PTR_ERR(da7213->mclk);
>  		else
>  			da7213->mclk = NULL;
> +	} else {
> +		/* Store clock rate for fixed clocks for automatic PLL setup */
> +		ret = clk_prepare_enable(da7213->mclk);
> +		if (ret) {
> +			dev_err(component->dev, "Failed to enable mclk\n");
> +			return ret;
> +		}

I've not gone through the clk framework code but surprised to see the need to
enable a clock to retrieve it's rate.

> +
> +		da7213->mclk_rate = clk_get_rate(da7213->mclk);
> +
> +		clk_disable_unprepare(da7213->mclk);
> +
> +		/* assume fixed clock until set_sysclk() is being called */
> +		da7213->fixed_clk_auto = true;

I don't see any code where 'fixed_clk_auto' is set to false so it seems that
previous operational usage is being broken here.

>  	}
> 
>  	return 0;
> diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> index 97a250ea39e6..00aca0126cdb 100644
> --- a/sound/soc/codecs/da7213.h
> +++ b/sound/soc/codecs/da7213.h
> @@ -532,6 +532,7 @@ struct da7213_priv {
>  	bool master;
>  	bool alc_calib_auto;
>  	bool alc_en;
> +	bool fixed_clk_auto;
>  	struct da7213_platform_data *pdata;
>  };
> 
> --
> 2.24.0.rc1
Sebastian Reichel Nov. 12, 2019, 4:29 p.m. UTC | #2
Hi,

On Mon, Nov 11, 2019 at 02:20:07PM +0000, Adam Thomson wrote:
> On 08 November 2019 17:49, Sebastian Reichel wrote:
> 
> > This adds default clock/PLL configuration to the driver
> > for usage with generic drivers like simple-card for usage
> > with a fixed rate clock.
> > 
> > Upstreaming this requires a good way to disable the automatic
> > clock handling for systems doing it manually to avoid breaking
> > existing setups.
> > 
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  sound/soc/codecs/da7213.c | 34 +++++++++++++++++++++++++++++++++-
> >  sound/soc/codecs/da7213.h |  1 +
> >  2 files changed, 34 insertions(+), 1 deletion(-)
> > 
> > diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> > index 197609691525..a4ed382ddfc7 100644
> > --- a/sound/soc/codecs/da7213.c
> > +++ b/sound/soc/codecs/da7213.c
> > @@ -1163,6 +1163,8 @@ static int da7213_hw_params(struct
> > snd_pcm_substream *substream,
> >  			    struct snd_soc_dai *dai)
> >  {
> >  	struct snd_soc_component *component = dai->component;
> > +	struct da7213_priv *da7213 =
> > snd_soc_component_get_drvdata(component);
> > +	int freq = 0;
> >  	u8 dai_ctrl = 0;
> >  	u8 fs;
> > 
> > @@ -1188,38 +1190,54 @@ static int da7213_hw_params(struct
> > snd_pcm_substream *substream,
> >  	switch (params_rate(params)) {
> >  	case 8000:
> >  		fs = DA7213_SR_8000;
> > +		freq = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 11025:
> >  		fs = DA7213_SR_11025;
> > +		freq = DA7213_PLL_FREQ_OUT_90316800;
> >  		break;
> >  	case 12000:
> >  		fs = DA7213_SR_12000;
> > +		freq = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 16000:
> >  		fs = DA7213_SR_16000;
> > +		freq = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 22050:
> >  		fs = DA7213_SR_22050;
> > +		freq = DA7213_PLL_FREQ_OUT_90316800;
> >  		break;
> >  	case 32000:
> >  		fs = DA7213_SR_32000;
> > +		freq = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 44100:
> >  		fs = DA7213_SR_44100;
> > +		freq = DA7213_PLL_FREQ_OUT_90316800;
> >  		break;
> >  	case 48000:
> >  		fs = DA7213_SR_48000;
> > +		freq = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 88200:
> >  		fs = DA7213_SR_88200;
> > +		freq = DA7213_PLL_FREQ_OUT_90316800;
> >  		break;
> >  	case 96000:
> >  		fs = DA7213_SR_96000;
> > +		freq = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	default:
> >  		return -EINVAL;
> >  	}
> > 
> > +	/* setup PLL */
> > +	if (da7213->fixed_clk_auto) {
> > +		snd_soc_component_set_pll(component, 0,
> > DA7213_SYSCLK_PLL,
> > +					  da7213->mclk_rate, freq);
> > +	}
> > +
> 
> Are we happy with the PLL being always enabled? Seems like a power drain,
> especially if you're using an MCLK which is a natural harmonic for the SR in 
> question in which case the PLL can be bypassed. Also the bias level function in
> this driver will enable and disable the MCLK, if it has been provided, so it's a
> bit strange to have the PLL enabled but it's clock source taken away.

So you are suggesting adding something like this to
da7213_set_bias_level()?

if (freq % da7213->mclk_rate == 0)
    source = DA7213_SYSCLK_MCLK
else
    source = DA7213_SYSCLK_PLL
snd_soc_component_set_pll(component, 0, source, da7213->mclk_rate, freq);

> >  	snd_soc_component_update_bits(component, DA7213_DAI_CTRL,
> > DA7213_DAI_WORD_LENGTH_MASK,
> >  			    dai_ctrl);
> >  	snd_soc_component_write(component, DA7213_SR, fs);
> > @@ -1700,10 +1718,10 @@ static struct da7213_platform_data
> >  	return pdata;
> >  }
> > 
> > -
> >  static int da7213_probe(struct snd_soc_component *component)
> >  {
> >  	struct da7213_priv *da7213 =
> > snd_soc_component_get_drvdata(component);
> > +	int ret;
> > 
> >  	pm_runtime_get_sync(component->dev);
> > 
> > @@ -1836,6 +1854,20 @@ static int da7213_probe(struct snd_soc_component
> > *component)
> >  			return PTR_ERR(da7213->mclk);
> >  		else
> >  			da7213->mclk = NULL;
> > +	} else {
> > +		/* Store clock rate for fixed clocks for automatic PLL setup */
> > +		ret = clk_prepare_enable(da7213->mclk);
> > +		if (ret) {
> > +			dev_err(component->dev, "Failed to enable mclk\n");
> > +			return ret;
> > +		}
> 
> I've not gone through the clk framework code but surprised to see the need to
> enable a clock to retrieve it's rate.

/**
 * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
 *                This is only valid once the clock source has been enabled.
 * @clk: clock source
 */
unsigned long clk_get_rate(struct clk *clk);

Which makes sense for a non-fixed clock, which uses the same API.

> > +		da7213->mclk_rate = clk_get_rate(da7213->mclk);
> > +
> > +		clk_disable_unprepare(da7213->mclk);
> > +
> > +		/* assume fixed clock until set_sysclk() is being called */
> > +		da7213->fixed_clk_auto = true;
> 
> I don't see any code where 'fixed_clk_auto' is set to false so it seems that
> previous operational usage is being broken here.

oops.

> >  	}
> > 
> >  	return 0;
> > diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> > index 97a250ea39e6..00aca0126cdb 100644
> > --- a/sound/soc/codecs/da7213.h
> > +++ b/sound/soc/codecs/da7213.h
> > @@ -532,6 +532,7 @@ struct da7213_priv {
> >  	bool master;
> >  	bool alc_calib_auto;
> >  	bool alc_en;
> > +	bool fixed_clk_auto;
> >  	struct da7213_platform_data *pdata;
> >  };
> > 
> > --
> > 2.24.0.rc1
> 

-- Sebastian
Adam Thomson Nov. 13, 2019, 11:25 a.m. UTC | #3
On 12 November 2019 16:30, Sebastian Reichel wrote:

> > > @@ -1188,38 +1190,54 @@ static int da7213_hw_params(struct
> > > snd_pcm_substream *substream,
> > >  	switch (params_rate(params)) {
> > >  	case 8000:
> > >  		fs = DA7213_SR_8000;
> > > +		freq = DA7213_PLL_FREQ_OUT_98304000;
> > >  		break;
> > >  	case 11025:
> > >  		fs = DA7213_SR_11025;
> > > +		freq = DA7213_PLL_FREQ_OUT_90316800;
> > >  		break;
> > >  	case 12000:
> > >  		fs = DA7213_SR_12000;
> > > +		freq = DA7213_PLL_FREQ_OUT_98304000;
> > >  		break;
> > >  	case 16000:
> > >  		fs = DA7213_SR_16000;
> > > +		freq = DA7213_PLL_FREQ_OUT_98304000;
> > >  		break;
> > >  	case 22050:
> > >  		fs = DA7213_SR_22050;
> > > +		freq = DA7213_PLL_FREQ_OUT_90316800;
> > >  		break;
> > >  	case 32000:
> > >  		fs = DA7213_SR_32000;
> > > +		freq = DA7213_PLL_FREQ_OUT_98304000;
> > >  		break;
> > >  	case 44100:
> > >  		fs = DA7213_SR_44100;
> > > +		freq = DA7213_PLL_FREQ_OUT_90316800;
> > >  		break;
> > >  	case 48000:
> > >  		fs = DA7213_SR_48000;
> > > +		freq = DA7213_PLL_FREQ_OUT_98304000;
> > >  		break;
> > >  	case 88200:
> > >  		fs = DA7213_SR_88200;
> > > +		freq = DA7213_PLL_FREQ_OUT_90316800;
> > >  		break;
> > >  	case 96000:
> > >  		fs = DA7213_SR_96000;
> > > +		freq = DA7213_PLL_FREQ_OUT_98304000;
> > >  		break;
> > >  	default:
> > >  		return -EINVAL;
> > >  	}
> > >
> > > +	/* setup PLL */
> > > +	if (da7213->fixed_clk_auto) {
> > > +		snd_soc_component_set_pll(component, 0,
> > > DA7213_SYSCLK_PLL,
> > > +					  da7213->mclk_rate, freq);
> > > +	}
> > > +
> >
> > Are we happy with the PLL being always enabled? Seems like a power drain,
> > especially if you're using an MCLK which is a natural harmonic for the SR in
> > question in which case the PLL can be bypassed. Also the bias level function in
> > this driver will enable and disable the MCLK, if it has been provided, so it's a
> > bit strange to have the PLL enabled but it's clock source taken away.
> 
> So you are suggesting adding something like this to
> da7213_set_bias_level()?
> 
> if (freq % da7213->mclk_rate == 0)
>     source = DA7213_SYSCLK_MCLK
> else
>     source = DA7213_SYSCLK_PLL
> snd_soc_component_set_pll(component, 0, source, da7213->mclk_rate, freq);

Yes it would make more sense to control the PLL there as for MCLK. Also for the
transition back to SND_SOC_BIAS_STANDBY you would want to configure the PLL as
DA7213_SYSCLK_MCLK so it's in bypass mode (i.e. disabled).

Selecting bypass mode for natural harmonic MCLK frequencies (11.2896/12.288,
22.5792/24.576 and 45.1584/49.152 as stated in the datasheet) would be ideal. I
think the check you suggest above though might not be enough as it will pick out
valid MCLK rates of 5.6448/6.144 as allowing PLL bypass but the datasheet
doesn't state those as valid options for bypass.

> > > @@ -1836,6 +1854,20 @@ static int da7213_probe(struct
> snd_soc_component
> > > *component)
> > >  			return PTR_ERR(da7213->mclk);
> > >  		else
> > >  			da7213->mclk = NULL;
> > > +	} else {
> > > +		/* Store clock rate for fixed clocks for automatic PLL setup */
> > > +		ret = clk_prepare_enable(da7213->mclk);
> > > +		if (ret) {
> > > +			dev_err(component->dev, "Failed to enable mclk\n");
> > > +			return ret;
> > > +		}
> >
> > I've not gone through the clk framework code but surprised to see the need to
> > enable a clock to retrieve it's rate.
> 
> /**
>  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>  *                This is only valid once the clock source has been enabled.
>  * @clk: clock source
>  */
> unsigned long clk_get_rate(struct clk *clk);
> 
> Which makes sense for a non-fixed clock, which uses the same API.

Hmmm. Fair enough. Just seems odd to me that you would need to enable a clock
to retrieve it's configured rate. Surely it would have been configured prior to
enabling. Reading into that code, the clk rate value is always taken from cache
so until it's enabled then the cache isn't updated. Actually with the suggestion
to move PLL control to the bias_level() function, you could just get the clk
rate there instead.

> 
> > > +		da7213->mclk_rate = clk_get_rate(da7213->mclk);
> > > +
> > > +		clk_disable_unprepare(da7213->mclk);
> > > +
> > > +		/* assume fixed clock until set_sysclk() is being called */
> > > +		da7213->fixed_clk_auto = true;
> >
> > I don't see any code where 'fixed_clk_auto' is set to false so it seems that
> > previous operational usage is being broken here.
> 
> oops.

:)

Patch
diff mbox series

diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
index 197609691525..a4ed382ddfc7 100644
--- a/sound/soc/codecs/da7213.c
+++ b/sound/soc/codecs/da7213.c
@@ -1163,6 +1163,8 @@  static int da7213_hw_params(struct snd_pcm_substream *substream,
 			    struct snd_soc_dai *dai)
 {
 	struct snd_soc_component *component = dai->component;
+	struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
+	int freq = 0;
 	u8 dai_ctrl = 0;
 	u8 fs;
 
@@ -1188,38 +1190,54 @@  static int da7213_hw_params(struct snd_pcm_substream *substream,
 	switch (params_rate(params)) {
 	case 8000:
 		fs = DA7213_SR_8000;
+		freq = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 11025:
 		fs = DA7213_SR_11025;
+		freq = DA7213_PLL_FREQ_OUT_90316800;
 		break;
 	case 12000:
 		fs = DA7213_SR_12000;
+		freq = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 16000:
 		fs = DA7213_SR_16000;
+		freq = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 22050:
 		fs = DA7213_SR_22050;
+		freq = DA7213_PLL_FREQ_OUT_90316800;
 		break;
 	case 32000:
 		fs = DA7213_SR_32000;
+		freq = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 44100:
 		fs = DA7213_SR_44100;
+		freq = DA7213_PLL_FREQ_OUT_90316800;
 		break;
 	case 48000:
 		fs = DA7213_SR_48000;
+		freq = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 88200:
 		fs = DA7213_SR_88200;
+		freq = DA7213_PLL_FREQ_OUT_90316800;
 		break;
 	case 96000:
 		fs = DA7213_SR_96000;
+		freq = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	default:
 		return -EINVAL;
 	}
 
+	/* setup PLL */
+	if (da7213->fixed_clk_auto) {
+		snd_soc_component_set_pll(component, 0, DA7213_SYSCLK_PLL,
+					  da7213->mclk_rate, freq);
+	}
+
 	snd_soc_component_update_bits(component, DA7213_DAI_CTRL, DA7213_DAI_WORD_LENGTH_MASK,
 			    dai_ctrl);
 	snd_soc_component_write(component, DA7213_SR, fs);
@@ -1700,10 +1718,10 @@  static struct da7213_platform_data
 	return pdata;
 }
 
-
 static int da7213_probe(struct snd_soc_component *component)
 {
 	struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
+	int ret;
 
 	pm_runtime_get_sync(component->dev);
 
@@ -1836,6 +1854,20 @@  static int da7213_probe(struct snd_soc_component *component)
 			return PTR_ERR(da7213->mclk);
 		else
 			da7213->mclk = NULL;
+	} else {
+		/* Store clock rate for fixed clocks for automatic PLL setup */
+		ret = clk_prepare_enable(da7213->mclk);
+		if (ret) {
+			dev_err(component->dev, "Failed to enable mclk\n");
+			return ret;
+		}
+
+		da7213->mclk_rate = clk_get_rate(da7213->mclk);
+
+		clk_disable_unprepare(da7213->mclk);
+
+		/* assume fixed clock until set_sysclk() is being called */
+		da7213->fixed_clk_auto = true;
 	}
 
 	return 0;
diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
index 97a250ea39e6..00aca0126cdb 100644
--- a/sound/soc/codecs/da7213.h
+++ b/sound/soc/codecs/da7213.h
@@ -532,6 +532,7 @@  struct da7213_priv {
 	bool master;
 	bool alc_calib_auto;
 	bool alc_en;
+	bool fixed_clk_auto;
 	struct da7213_platform_data *pdata;
 };