| Message ID | 20191120152406.2744-7-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: da7213: support for usage with simple-card | expand |
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
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
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.
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?
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.
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.
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.
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.
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.
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?
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.
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?
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.
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; };
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(-)