[PATCHv2,6/6] ASoC: da7213: Add default clock handling
diff mbox series

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

Commit Message

Sebastian Reichel Nov. 20, 2019, 3:24 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.

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

Comments

Adam Thomson Nov. 21, 2019, 9:49 p.m. UTC | #1
On 20 November 2019 15:24, 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.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  sound/soc/codecs/da7213.c | 75
> ++++++++++++++++++++++++++++++++++++---
>  sound/soc/codecs/da7213.h |  2 ++
>  2 files changed, 73 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> index 3e6ad996741b..ff1a936240be 100644
> --- a/sound/soc/codecs/da7213.c
> +++ b/sound/soc/codecs/da7213.c
> @@ -1156,6 +1156,7 @@ 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);
>  	u8 dai_ctrl = 0;
>  	u8 fs;
> 
> @@ -1181,33 +1182,43 @@ static int da7213_hw_params(struct
> snd_pcm_substream *substream,
>  	switch (params_rate(params)) {
>  	case 8000:
>  		fs = DA7213_SR_8000;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 11025:
>  		fs = DA7213_SR_11025;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 12000:
>  		fs = DA7213_SR_12000;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 16000:
>  		fs = DA7213_SR_16000;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 22050:
>  		fs = DA7213_SR_22050;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 32000:
>  		fs = DA7213_SR_32000;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 44100:
>  		fs = DA7213_SR_44100;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 48000:
>  		fs = DA7213_SR_48000;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 88200:
>  		fs = DA7213_SR_88200;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 96000:
>  		fs = DA7213_SR_96000;
> +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -1392,9 +1403,9 @@ static int da7213_set_component_sysclk(struct
> snd_soc_component *component,
>  }
> 
>  /* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */
> -static int da7213_set_component_pll(struct snd_soc_component *component,
> -				    int pll_id, int source,
> -				    unsigned int fref, unsigned int fout)
> +static int _da7213_set_component_pll(struct snd_soc_component
> *component,
> +				     int pll_id, int source,
> +				     unsigned int fref, unsigned int fout)
>  {
>  	struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> 
> @@ -1503,6 +1514,16 @@ static int da7213_set_component_pll(struct
> snd_soc_component *component,
>  	return 0;
>  }
> 
> +static int da7213_set_component_pll(struct snd_soc_component *component,
> +				    int pll_id, int source,
> +				    unsigned int fref, unsigned int fout)
> +{
> +	struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> +	da7213->fixed_clk_auto_pll = false;
> +
> +	return _da7213_set_component_pll(component, pll_id, source, fref,
> fout);
> +}
> +
>  /* DAI operations */
>  static const struct snd_soc_dai_ops da7213_dai_ops = {
>  	.hw_params	= da7213_hw_params,
> @@ -1532,6 +1553,43 @@ static struct snd_soc_dai_driver da7213_dai = {
>  	.symmetric_rates = 1,
>  };
> 
> +static int da7213_set_auto_pll(struct snd_soc_component *component, bool
> enable)
> +{
> +	struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> +	int mode;
> +
> +	if (!da7213->fixed_clk_auto_pll)
> +		return 0;
> +
> +	da7213->mclk_rate = clk_get_rate(da7213->mclk);
> +
> +	if (enable)
> +		mode = DA7213_SYSCLK_PLL;
> +	else
> +		mode = DA7213_SYSCLK_MCLK;

If we're the clock slave, and we're using an MCLK that's not a harmonic then
SRM is required to synchronise the PLL to the incoming WCLK signal. I assume
simple sound card should allow for both master and slave modes? If so we'll
need to do something here to determine this as well.

> +
> +	switch (da7213->out_rate) {
> +	case DA7213_PLL_FREQ_OUT_90316800:
> +		if (da7213->mclk_rate == 11289600 ||
> +		    da7213->mclk_rate == 22579200 ||
> +		    da7213->mclk_rate == 45158400)
> +			mode = DA7213_SYSCLK_MCLK;
> +		break;
> +	case DA7213_PLL_FREQ_OUT_98304000:
> +		if (da7213->mclk_rate == 12288000 ||
> +		    da7213->mclk_rate == 24576000 ||
> +		    da7213->mclk_rate == 49152000)
> +			mode = DA7213_SYSCLK_MCLK;
> +
> +		break;
> +	default:
> +		return -1;
> +	}
> +
> +	return _da7213_set_component_pll(component, 0, mode,
> +					 da7213->mclk_rate, da7213->out_rate);
> +}
> +
>  static int da7213_set_bias_level(struct snd_soc_component *component,
>  				 enum snd_soc_bias_level level)
>  {
> @@ -1551,6 +1609,8 @@ static int da7213_set_bias_level(struct
> snd_soc_component *component,
>  						"Failed to enable mclk\n");
>  					return ret;
>  				}
> +
> +				da7213_set_auto_pll(component, true);

I've thought more about this and for the scenario where a machine driver
controls the PLL through a DAPM widget associated with this codec (like some
Intel boards do), then the PLL will be configured once here and then again
when the relevant widget is called. I don't think that will matter but I will
take a further look just in case this might cause some oddities. 

>  			}
>  		}
>  		break;
> @@ -1562,8 +1622,10 @@ static int da7213_set_bias_level(struct
> snd_soc_component *component,
>  					    DA7213_VMID_EN | DA7213_BIAS_EN);
>  		} else {
>  			/* Remove MCLK */
> -			if (da7213->mclk)
> +			if (da7213->mclk) {
> +				da7213_set_auto_pll(component, false);
>  				clk_disable_unprepare(da7213->mclk);
> +			}
>  		}
>  		break;
>  	case SND_SOC_BIAS_OFF:
> @@ -1829,6 +1891,11 @@ static int da7213_probe(struct snd_soc_component
> *component)
>  			return PTR_ERR(da7213->mclk);
>  		else
>  			da7213->mclk = NULL;
> +	} else {
> +		/* Do automatic PLL handling assuming fixed clock until
> +		 * set_pll() has been called. This makes the codec usable
> +		 * with the simple-audio-card driver. */
> +		da7213->fixed_clk_auto_pll = true;
>  	}
> 
>  	return 0;
> diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> index 3890829dfb6e..97ccf0ddd2be 100644
> --- a/sound/soc/codecs/da7213.h
> +++ b/sound/soc/codecs/da7213.h
> @@ -535,10 +535,12 @@ struct da7213_priv {
>  	struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES];
>  	struct clk *mclk;
>  	unsigned int mclk_rate;
> +	unsigned int out_rate;
>  	int clk_src;
>  	bool master;
>  	bool alc_calib_auto;
>  	bool alc_en;
> +	bool fixed_clk_auto_pll;
>  	struct da7213_platform_data *pdata;
>  };
> 
> --
> 2.24.0
Adam Thomson Nov. 26, 2019, 4:55 p.m. UTC | #2
On 21 November 2019 21:49, Adam Thomson wrote:

> On 20 November 2019 15:24, 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.
> >
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  sound/soc/codecs/da7213.c | 75
> > ++++++++++++++++++++++++++++++++++++---
> >  sound/soc/codecs/da7213.h |  2 ++
> >  2 files changed, 73 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> > index 3e6ad996741b..ff1a936240be 100644
> > --- a/sound/soc/codecs/da7213.c
> > +++ b/sound/soc/codecs/da7213.c
> > @@ -1156,6 +1156,7 @@ 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);
> >  	u8 dai_ctrl = 0;
> >  	u8 fs;
> >
> > @@ -1181,33 +1182,43 @@ static int da7213_hw_params(struct
> > snd_pcm_substream *substream,
> >  	switch (params_rate(params)) {
> >  	case 8000:
> >  		fs = DA7213_SR_8000;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 11025:
> >  		fs = DA7213_SR_11025;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> >  		break;
> >  	case 12000:
> >  		fs = DA7213_SR_12000;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 16000:
> >  		fs = DA7213_SR_16000;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 22050:
> >  		fs = DA7213_SR_22050;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> >  		break;
> >  	case 32000:
> >  		fs = DA7213_SR_32000;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 44100:
> >  		fs = DA7213_SR_44100;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> >  		break;
> >  	case 48000:
> >  		fs = DA7213_SR_48000;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	case 88200:
> >  		fs = DA7213_SR_88200;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
> >  		break;
> >  	case 96000:
> >  		fs = DA7213_SR_96000;
> > +		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
> >  		break;
> >  	default:
> >  		return -EINVAL;
> > @@ -1392,9 +1403,9 @@ static int da7213_set_component_sysclk(struct
> > snd_soc_component *component,
> >  }
> >
> >  /* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */
> > -static int da7213_set_component_pll(struct snd_soc_component
> *component,
> > -				    int pll_id, int source,
> > -				    unsigned int fref, unsigned int fout)
> > +static int _da7213_set_component_pll(struct snd_soc_component
> > *component,
> > +				     int pll_id, int source,
> > +				     unsigned int fref, unsigned int fout)
> >  {
> >  	struct da7213_priv *da7213 =
> > snd_soc_component_get_drvdata(component);
> >
> > @@ -1503,6 +1514,16 @@ static int da7213_set_component_pll(struct
> > snd_soc_component *component,
> >  	return 0;
> >  }
> >
> > +static int da7213_set_component_pll(struct snd_soc_component
> *component,
> > +				    int pll_id, int source,
> > +				    unsigned int fref, unsigned int fout)
> > +{
> > +	struct da7213_priv *da7213 =
> > snd_soc_component_get_drvdata(component);
> > +	da7213->fixed_clk_auto_pll = false;
> > +
> > +	return _da7213_set_component_pll(component, pll_id, source, fref,
> > fout);
> > +}
> > +
> >  /* DAI operations */
> >  static const struct snd_soc_dai_ops da7213_dai_ops = {
> >  	.hw_params	= da7213_hw_params,
> > @@ -1532,6 +1553,43 @@ static struct snd_soc_dai_driver da7213_dai = {
> >  	.symmetric_rates = 1,
> >  };
> >
> > +static int da7213_set_auto_pll(struct snd_soc_component *component, bool
> > enable)
> > +{
> > +	struct da7213_priv *da7213 =
> > snd_soc_component_get_drvdata(component);
> > +	int mode;
> > +
> > +	if (!da7213->fixed_clk_auto_pll)
> > +		return 0;
> > +
> > +	da7213->mclk_rate = clk_get_rate(da7213->mclk);
> > +
> > +	if (enable)
> > +		mode = DA7213_SYSCLK_PLL;
> > +	else
> > +		mode = DA7213_SYSCLK_MCLK;
> 
> If we're the clock slave, and we're using an MCLK that's not a harmonic then
> SRM is required to synchronise the PLL to the incoming WCLK signal. I assume
> simple sound card should allow for both master and slave modes? If so we'll
> need to do something here to determine this as well.
> 
> > +
> > +	switch (da7213->out_rate) {
> > +	case DA7213_PLL_FREQ_OUT_90316800:
> > +		if (da7213->mclk_rate == 11289600 ||
> > +		    da7213->mclk_rate == 22579200 ||
> > +		    da7213->mclk_rate == 45158400)
> > +			mode = DA7213_SYSCLK_MCLK;
> > +		break;
> > +	case DA7213_PLL_FREQ_OUT_98304000:
> > +		if (da7213->mclk_rate == 12288000 ||
> > +		    da7213->mclk_rate == 24576000 ||
> > +		    da7213->mclk_rate == 49152000)
> > +			mode = DA7213_SYSCLK_MCLK;
> > +
> > +		break;
> > +	default:
> > +		return -1;
> > +	}
> > +
> > +	return _da7213_set_component_pll(component, 0, mode,
> > +					 da7213->mclk_rate, da7213->out_rate);
> > +}
> > +
> >  static int da7213_set_bias_level(struct snd_soc_component *component,
> >  				 enum snd_soc_bias_level level)
> >  {
> > @@ -1551,6 +1609,8 @@ static int da7213_set_bias_level(struct
> > snd_soc_component *component,
> >  						"Failed to enable mclk\n");
> >  					return ret;
> >  				}
> > +
> > +				da7213_set_auto_pll(component, true);
> 
> I've thought more about this and for the scenario where a machine driver
> controls the PLL through a DAPM widget associated with this codec (like some
> Intel boards do), then the PLL will be configured once here and then again
> when the relevant widget is called. I don't think that will matter but I will
> take a further look just in case this might cause some oddities.

So I don't see any issues per say with the PLL function being called twice in
the example I mentioned. However it still feels a bit clunky; You either live
with it or you have something in the machine driver to call the codec's PLL
function early doors to prevent the bias_level() code of the codec controlling
the PLL automatically. Am wondering though if there would be some use in having
an indicator that simple-card is being used so we can avoid this? I guess we
could check the parent sound card instance's OF node to look at the compatible
string and see if it's a simple-card instantiation. Bit messy maybe.
Alternatively would it be worthwhile storing in the snd_soc_card instance that
this is a simple-card instantiation? We could then use that to determine
whether the codec driver should automatically deal with the PLL or not. This
would of course require an update to the snd_soc_card structure to add the new
flag and then some update to simple-card.c to flag this.

I think relying on the timing of the call to the codec's PLL configuration
function from a machine driver isn't ideal.

> 
> >  			}
> >  		}
> >  		break;
> > @@ -1562,8 +1622,10 @@ static int da7213_set_bias_level(struct
> > snd_soc_component *component,
> >  					    DA7213_VMID_EN | DA7213_BIAS_EN);
> >  		} else {
> >  			/* Remove MCLK */
> > -			if (da7213->mclk)
> > +			if (da7213->mclk) {
> > +				da7213_set_auto_pll(component, false);
> >  				clk_disable_unprepare(da7213->mclk);
> > +			}
> >  		}
> >  		break;
> >  	case SND_SOC_BIAS_OFF:
> > @@ -1829,6 +1891,11 @@ static int da7213_probe(struct snd_soc_component
> > *component)
> >  			return PTR_ERR(da7213->mclk);
> >  		else
> >  			da7213->mclk = NULL;
> > +	} else {
> > +		/* Do automatic PLL handling assuming fixed clock until
> > +		 * set_pll() has been called. This makes the codec usable
> > +		 * with the simple-audio-card driver. */
> > +		da7213->fixed_clk_auto_pll = true;
> >  	}
> >
> >  	return 0;
> > diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> > index 3890829dfb6e..97ccf0ddd2be 100644
> > --- a/sound/soc/codecs/da7213.h
> > +++ b/sound/soc/codecs/da7213.h
> > @@ -535,10 +535,12 @@ struct da7213_priv {
> >  	struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES];
> >  	struct clk *mclk;
> >  	unsigned int mclk_rate;
> > +	unsigned int out_rate;
> >  	int clk_src;
> >  	bool master;
> >  	bool alc_calib_auto;
> >  	bool alc_en;
> > +	bool fixed_clk_auto_pll;
> >  	struct da7213_platform_data *pdata;
> >  };
> >
> > --
> > 2.24.0
Mark Brown Nov. 26, 2019, 5:08 p.m. UTC | #3
On Tue, Nov 26, 2019 at 04:55:39PM +0000, Adam Thomson wrote:
> On 21 November 2019 21:49, Adam Thomson wrote:
> > On 20 November 2019 15:24, Sebastian Reichel wrote:

> > I've thought more about this and for the scenario where a machine driver
> > controls the PLL through a DAPM widget associated with this codec (like some
> > Intel boards do), then the PLL will be configured once here and then again
> > when the relevant widget is called. I don't think that will matter but I will
> > take a further look just in case this might cause some oddities.

> So I don't see any issues per say with the PLL function being called twice in
> the example I mentioned. However it still feels a bit clunky; You either live
> with it or you have something in the machine driver to call the codec's PLL
> function early doors to prevent the bias_level() code of the codec controlling
> the PLL automatically. Am wondering though if there would be some use in having
> an indicator that simple-card is being used so we can avoid this? I guess we

If we're special casing simple-card we're doing it wrong - there's
nothing stopping any other machine driver behaving in the same way.
Adam Thomson Nov. 26, 2019, 5:39 p.m. UTC | #4
On 26 November 2019 17:09, Mark Brown wrote:

> On Tue, Nov 26, 2019 at 04:55:39PM +0000, Adam Thomson wrote:
> > On 21 November 2019 21:49, Adam Thomson wrote:
> > > On 20 November 2019 15:24, Sebastian Reichel wrote:
> 
> > > I've thought more about this and for the scenario where a machine driver
> > > controls the PLL through a DAPM widget associated with this codec (like some
> > > Intel boards do), then the PLL will be configured once here and then again
> > > when the relevant widget is called. I don't think that will matter but I will
> > > take a further look just in case this might cause some oddities.
> 
> > So I don't see any issues per say with the PLL function being called twice in
> > the example I mentioned. However it still feels a bit clunky; You either live
> > with it or you have something in the machine driver to call the codec's PLL
> > function early doors to prevent the bias_level() code of the codec controlling
> > the PLL automatically. Am wondering though if there would be some use in
> having
> > an indicator that simple-card is being used so we can avoid this? I guess we
> 
> If we're special casing simple-card we're doing it wrong - there's
> nothing stopping any other machine driver behaving in the same way.

Ok, what's being proposed here is for the codec to automatically control the PLL
rather than leaving it to the machine driver as is the case right now. In the
possible scenario where this is done using a card level widget to enable/disable
the PLL we will conflict with that using the current suggested approach for the
da7213 driver, albeit with no real consequence other than configuring the PLL
twice the first time a stream is started. It's a case of how to determine who's
in control of the PLL here; machine driver or codec?
Mark Brown Nov. 26, 2019, 5:50 p.m. UTC | #5
On Tue, Nov 26, 2019 at 05:39:45PM +0000, Adam Thomson wrote:
> On 26 November 2019 17:09, Mark Brown wrote:

> > If we're special casing simple-card we're doing it wrong - there's
> > nothing stopping any other machine driver behaving in the same way.

> Ok, what's being proposed here is for the codec to automatically control the PLL
> rather than leaving it to the machine driver as is the case right now. In the
> possible scenario where this is done using a card level widget to enable/disable

Wasn't the proposal to add a new mode where the machine driver *could*
choose to delgate PLL selection to the CODEC driver rather than do so
unconditionally?  

> the PLL we will conflict with that using the current suggested approach for the
> da7213 driver, albeit with no real consequence other than configuring the PLL
> twice the first time a stream is started. It's a case of how to determine who's
> in control of the PLL here; machine driver or codec?

The patch looked like the idea was that as soon as the machine driver
configures the PLL the CODEC driver will stop touching it which looked
like a reasonable way of doing it, you could also do it with an explicit
SYSCLK value (which would have to be 0 for generic machine drivers to
pick it up) but this works just as well and may even be better.  Perhaps
I misread it though.
Adam Thomson Nov. 27, 2019, 11:32 a.m. UTC | #6
On 26 November 2019 17:51, Mark Brown wrote:

> On Tue, Nov 26, 2019 at 05:39:45PM +0000, Adam Thomson wrote:
> > On 26 November 2019 17:09, Mark Brown wrote:
>
> > > If we're special casing simple-card we're doing it wrong - there's
> > > nothing stopping any other machine driver behaving in the same way.
>
> > Ok, what's being proposed here is for the codec to automatically control the PLL
> > rather than leaving it to the machine driver as is the case right now. In the
> > possible scenario where this is done using a card level widget to enable/disable
>
> Wasn't the proposal to add a new mode where the machine driver *could*
> choose to delgate PLL selection to the CODEC driver rather than do so
> unconditionally?

Yes, but by default the assumption is the codec will do things automatically
until the machine driver calls the relevant PLL config code.....

>
> > the PLL we will conflict with that using the current suggested approach for the
> > da7213 driver, albeit with no real consequence other than configuring the PLL
> > twice the first time a stream is started. It's a case of how to determine who's
> > in control of the PLL here; machine driver or codec?
>
> The patch looked like the idea was that as soon as the machine driver
> configures the PLL the CODEC driver will stop touching it which looked
> like a reasonable way of doing it, you could also do it with an explicit
> SYSCLK value (which would have to be 0 for generic machine drivers to
> pick it up) but this works just as well and may even be better.  Perhaps
> I misread it though.

...so yes you're right in your comment here. However the bias_level code is
called prior to the DAPM paths being sequenced. If a dedicated machine driver
were to want to control the PLL via a DAPM widget at the card level (which is
done for other codecs for example on Intel platforms), the code in the
bias_level() function of the codec to auto configure the PLL would always be
called the very first time a path is enabled before the relevant DAPM widget for
the card is called, so you'd have two configurations of the PLL in succession. I
don't expect that would cause issues, but it's not ideal. The other approach
would be to have something in the machine driver like a dai_link init function
to default the PLL config using the codec's PLL function which then prevents
any chance of auto configuration subsequently. However that requires the person
implementing the machine driver to know about this behaviour.

As I said it's a small thing and requires a specific use-case to occur, but
having the PLL configured twice for the very first stream in that scenario
seems messy. Regarding the SYSCLK approach you mention, I'm not clear how that
would work so I'm obviously missing something. If we had some init stage
indication though that auto PLL was required then we're golden.
Mark Brown Nov. 27, 2019, 12:33 p.m. UTC | #7
On Wed, Nov 27, 2019 at 11:32:54AM +0000, Adam Thomson wrote:

> As I said it's a small thing and requires a specific use-case to occur, but
> having the PLL configured twice for the very first stream in that scenario
> seems messy. Regarding the SYSCLK approach you mention, I'm not clear how that
> would work so I'm obviously missing something. If we had some init stage
> indication though that auto PLL was required then we're golden.

There's a bunch of other drivers using the SYSCLK thing, when you call
set_sysclk() they provide a different SYSCLK number if they want to use
manual mode.  If there's a concern about drivers doing stuff on init you
could always ask them to set the PLL during init, even if only briefly.
Adam Thomson Nov. 27, 2019, 1:42 p.m. UTC | #8
On 27 November 2019 12:33, Mark Brown wrote:

> On Wed, Nov 27, 2019 at 11:32:54AM +0000, Adam Thomson wrote:
> 
> > As I said it's a small thing and requires a specific use-case to occur, but
> > having the PLL configured twice for the very first stream in that scenario
> > seems messy. Regarding the SYSCLK approach you mention, I'm not clear how
> that
> > would work so I'm obviously missing something. If we had some init stage
> > indication though that auto PLL was required then we're golden.
> 
> There's a bunch of other drivers using the SYSCLK thing, when you call
> set_sysclk() they provide a different SYSCLK number if they want to use
> manual mode.  If there's a concern about drivers doing stuff on init you
> could always ask them to set the PLL during init, even if only briefly.

Yeah OK I see what you mean; using the sysclk id. Still doesn't feel like the
nicest solution here though. I guess we're stuck with people having to manually
configure the PLL for bypass when a non-generic machine driver inits, to avoid
the initial double config, as I don't see other options unless you have
something to specify at init that it's auto or manual, and this doesn't feel
like a DT device specific property thing as it's more software than hardware.
At least Sebastian's patch has a good comment block to highlight this.
Mark Brown Nov. 27, 2019, 3:40 p.m. UTC | #9
On Wed, Nov 27, 2019 at 01:42:54PM +0000, Adam Thomson wrote:

> nicest solution here though. I guess we're stuck with people having to manually
> configure the PLL for bypass when a non-generic machine driver inits, to avoid
> the initial double config, as I don't see other options unless you have
> something to specify at init that it's auto or manual, and this doesn't feel
> like a DT device specific property thing as it's more software than hardware.
> At least Sebastian's patch has a good comment block to highlight this.

Not sure I follow here - if we're configuring the PLL explicitly then
I'd expect the PLL to be configured first, then the SYSCLK, so I'd
expect that the automatic PLL configuration wouldn't kick in.
Adam Thomson Nov. 27, 2019, 4:33 p.m. UTC | #10
On 27 November 2019 15:41, Mark Brown wrote:

> On Wed, Nov 27, 2019 at 01:42:54PM +0000, Adam Thomson wrote:
> 
> > nicest solution here though. I guess we're stuck with people having to manually
> > configure the PLL for bypass when a non-generic machine driver inits, to avoid
> > the initial double config, as I don't see other options unless you have
> > something to specify at init that it's auto or manual, and this doesn't feel
> > like a DT device specific property thing as it's more software than hardware.
> > At least Sebastian's patch has a good comment block to highlight this.
> 
> Not sure I follow here - if we're configuring the PLL explicitly then
> I'd expect the PLL to be configured first, then the SYSCLK, so I'd
> expect that the automatic PLL configuration wouldn't kick in.

The PLL in the codec relies on MCLK. The MCLK rate can be specified/configured
by a machine driver using the relevant codec sysclk function, as is done in a
number of drivers. Surely that has to happen first before we configure the PLL
as the PLL functions needs to know what rate is coming in so the correct
dividers can be applied for the required internal clocking to match up with the
desired sample rates. I guess I'm still missing something regarding your
discussion around SYSCLK?
Mark Brown Nov. 27, 2019, 4:41 p.m. UTC | #11
On Wed, Nov 27, 2019 at 04:33:12PM +0000, Adam Thomson wrote:
> On 27 November 2019 15:41, Mark Brown wrote:

> > Not sure I follow here - if we're configuring the PLL explicitly then
> > I'd expect the PLL to be configured first, then the SYSCLK, so I'd
> > expect that the automatic PLL configuration wouldn't kick in.

> The PLL in the codec relies on MCLK. The MCLK rate can be specified/configured
> by a machine driver using the relevant codec sysclk function, as is done in a
> number of drivers. Surely that has to happen first before we configure the PLL
> as the PLL functions needs to know what rate is coming in so the correct
> dividers can be applied for the required internal clocking to match up with the
> desired sample rates. I guess I'm still missing something regarding your
> discussion around SYSCLK?

The PLL configuration specifies both input and output clock rates (as
well as an input clock source) so if it's got to configure the MCLK I'd
expect the driver to figure that out without needing the caller to
separately set the MCLK rate.
Adam Thomson Nov. 27, 2019, 6:10 p.m. UTC | #12
On 27 November 2019 16:41, Mark Brown wrote:

> > The PLL in the codec relies on MCLK. The MCLK rate can be specified/configured
> > by a machine driver using the relevant codec sysclk function, as is done in a
> > number of drivers. Surely that has to happen first before we configure the PLL
> > as the PLL functions needs to know what rate is coming in so the correct
> > dividers can be applied for the required internal clocking to match up with the
> > desired sample rates. I guess I'm still missing something regarding your
> > discussion around SYSCLK?
>
> The PLL configuration specifies both input and output clock rates (as
> well as an input clock source) so if it's got to configure the MCLK I'd
> expect the driver to figure that out without needing the caller to
> separately set the MCLK rate.

Yes it does but the name of the function implies it's setting the codec's PLL,
not the system clock, whereas the other function implies setting the system
clock and not the PLL. Also generally you're only setting the sysclk once
whereas you may want to configure and enable/disable the PLL more dynamically,
at least for devices which do have a built-in PLL. Of course that could still be
handled through the single PLL function call.

Just as an informational, what's the future for these two functions if
essentially one is only really required and the other deemed redundant? I would
just like to be clear so I'm not falling over things like this in the future,
and wasting your time as well. Seems that the PLL call isn't part of simple
generic card code so would the be deemed surplus to requirements some point down
the line?
Mark Brown Nov. 28, 2019, 1:13 p.m. UTC | #13
On Wed, Nov 27, 2019 at 06:10:00PM +0000, Adam Thomson wrote:
> On 27 November 2019 16:41, Mark Brown wrote:

> > The PLL configuration specifies both input and output clock rates (as
> > well as an input clock source) so if it's got to configure the MCLK I'd
> > expect the driver to figure that out without needing the caller to
> > separately set the MCLK rate.

> Yes it does but the name of the function implies it's setting the codec's PLL,
> not the system clock, whereas the other function implies setting the system
> clock and not the PLL. Also generally you're only setting the sysclk once
> whereas you may want to configure and enable/disable the PLL more dynamically,
> at least for devices which do have a built-in PLL. Of course that could still be
> handled through the single PLL function call.

I wouldn't be so sure about only setting the SYSCLK once - if you've got
an input clock you can configure then you might for example want to
switch between 44.1kHz and 48kHz bases, and disable it or run it at very
low frequency when idle.  In some systems it's as dynamic as a PLL is.

> Just as an informational, what's the future for these two functions if
> essentially one is only really required and the other deemed redundant? I would
> just like to be clear so I'm not falling over things like this in the future,
> and wasting your time as well. Seems that the PLL call isn't part of simple
> generic card code so would the be deemed surplus to requirements some point down
> the line?

Things like simple-card are good for 90% of systems but I'm fairly sure
we'll never be able to handle more complex setups entirely
automatically.  What we *should* be doing over time is transitioning all
this clock stuff into the actual clock framework but that's a big, long
term thing which nobody is currently actually working on.

Patch
diff mbox series

diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
index 3e6ad996741b..ff1a936240be 100644
--- a/sound/soc/codecs/da7213.c
+++ b/sound/soc/codecs/da7213.c
@@ -1156,6 +1156,7 @@  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);
 	u8 dai_ctrl = 0;
 	u8 fs;
 
@@ -1181,33 +1182,43 @@  static int da7213_hw_params(struct snd_pcm_substream *substream,
 	switch (params_rate(params)) {
 	case 8000:
 		fs = DA7213_SR_8000;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 11025:
 		fs = DA7213_SR_11025;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
 		break;
 	case 12000:
 		fs = DA7213_SR_12000;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 16000:
 		fs = DA7213_SR_16000;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 22050:
 		fs = DA7213_SR_22050;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
 		break;
 	case 32000:
 		fs = DA7213_SR_32000;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 44100:
 		fs = DA7213_SR_44100;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
 		break;
 	case 48000:
 		fs = DA7213_SR_48000;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	case 88200:
 		fs = DA7213_SR_88200;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_90316800;
 		break;
 	case 96000:
 		fs = DA7213_SR_96000;
+		da7213->out_rate = DA7213_PLL_FREQ_OUT_98304000;
 		break;
 	default:
 		return -EINVAL;
@@ -1392,9 +1403,9 @@  static int da7213_set_component_sysclk(struct snd_soc_component *component,
 }
 
 /* Supported PLL input frequencies are 32KHz, 5MHz - 54MHz. */
-static int da7213_set_component_pll(struct snd_soc_component *component,
-				    int pll_id, int source,
-				    unsigned int fref, unsigned int fout)
+static int _da7213_set_component_pll(struct snd_soc_component *component,
+				     int pll_id, int source,
+				     unsigned int fref, unsigned int fout)
 {
 	struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
 
@@ -1503,6 +1514,16 @@  static int da7213_set_component_pll(struct snd_soc_component *component,
 	return 0;
 }
 
+static int da7213_set_component_pll(struct snd_soc_component *component,
+				    int pll_id, int source,
+				    unsigned int fref, unsigned int fout)
+{
+	struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
+	da7213->fixed_clk_auto_pll = false;
+
+	return _da7213_set_component_pll(component, pll_id, source, fref, fout);
+}
+
 /* DAI operations */
 static const struct snd_soc_dai_ops da7213_dai_ops = {
 	.hw_params	= da7213_hw_params,
@@ -1532,6 +1553,43 @@  static struct snd_soc_dai_driver da7213_dai = {
 	.symmetric_rates = 1,
 };
 
+static int da7213_set_auto_pll(struct snd_soc_component *component, bool enable)
+{
+	struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
+	int mode;
+
+	if (!da7213->fixed_clk_auto_pll)
+		return 0;
+
+	da7213->mclk_rate = clk_get_rate(da7213->mclk);
+
+	if (enable)
+		mode = DA7213_SYSCLK_PLL;
+	else
+		mode = DA7213_SYSCLK_MCLK;
+
+	switch (da7213->out_rate) {
+	case DA7213_PLL_FREQ_OUT_90316800:
+		if (da7213->mclk_rate == 11289600 ||
+		    da7213->mclk_rate == 22579200 ||
+		    da7213->mclk_rate == 45158400)
+			mode = DA7213_SYSCLK_MCLK;
+		break;
+	case DA7213_PLL_FREQ_OUT_98304000:
+		if (da7213->mclk_rate == 12288000 ||
+		    da7213->mclk_rate == 24576000 ||
+		    da7213->mclk_rate == 49152000)
+			mode = DA7213_SYSCLK_MCLK;
+
+		break;
+	default:
+		return -1;
+	}
+
+	return _da7213_set_component_pll(component, 0, mode,
+					 da7213->mclk_rate, da7213->out_rate);
+}
+
 static int da7213_set_bias_level(struct snd_soc_component *component,
 				 enum snd_soc_bias_level level)
 {
@@ -1551,6 +1609,8 @@  static int da7213_set_bias_level(struct snd_soc_component *component,
 						"Failed to enable mclk\n");
 					return ret;
 				}
+
+				da7213_set_auto_pll(component, true);
 			}
 		}
 		break;
@@ -1562,8 +1622,10 @@  static int da7213_set_bias_level(struct snd_soc_component *component,
 					    DA7213_VMID_EN | DA7213_BIAS_EN);
 		} else {
 			/* Remove MCLK */
-			if (da7213->mclk)
+			if (da7213->mclk) {
+				da7213_set_auto_pll(component, false);
 				clk_disable_unprepare(da7213->mclk);
+			}
 		}
 		break;
 	case SND_SOC_BIAS_OFF:
@@ -1829,6 +1891,11 @@  static int da7213_probe(struct snd_soc_component *component)
 			return PTR_ERR(da7213->mclk);
 		else
 			da7213->mclk = NULL;
+	} else {
+		/* Do automatic PLL handling assuming fixed clock until
+		 * set_pll() has been called. This makes the codec usable
+		 * with the simple-audio-card driver. */
+		da7213->fixed_clk_auto_pll = true;
 	}
 
 	return 0;
diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
index 3890829dfb6e..97ccf0ddd2be 100644
--- a/sound/soc/codecs/da7213.h
+++ b/sound/soc/codecs/da7213.h
@@ -535,10 +535,12 @@  struct da7213_priv {
 	struct regulator_bulk_data supplies[DA7213_NUM_SUPPLIES];
 	struct clk *mclk;
 	unsigned int mclk_rate;
+	unsigned int out_rate;
 	int clk_src;
 	bool master;
 	bool alc_calib_auto;
 	bool alc_en;
+	bool fixed_clk_auto_pll;
 	struct da7213_platform_data *pdata;
 };