| Message ID | 20180601212643.20101-1-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 01, 2018 at 04:26:43PM -0500, Pierre-Louis Bossart wrote: > For codec master mode, the current pcm512x code relies on the clock API. > Simple boards such as Hifiberry DAC+ PRO rely on 2 local oscillators > to generate 44.1 or 48kHz multiples, with the clock selection being > handled by the machine driver toggling GPIOS. > The RaspberryPi kernel exposes a "hifiberry,dacpro-clk" clock driver > which does nothing but tell the codec driver what rate is set by the > machine driver [1][2] > This patch suggests an alternate fallback solution to let the codec > driver know what the sysclk is based on the existing .set_sysclk > callback. I'm not seeing a great advantage in this - we want to have more adoption of standard APIs for clocking rather than less. As that Pi board shows it's easy to create static clocks in the clock API so why not just do that?
On 6/4/18 5:40 AM, Mark Brown wrote: > On Fri, Jun 01, 2018 at 04:26:43PM -0500, Pierre-Louis Bossart wrote: >> For codec master mode, the current pcm512x code relies on the clock API. > >> Simple boards such as Hifiberry DAC+ PRO rely on 2 local oscillators >> to generate 44.1 or 48kHz multiples, with the clock selection being >> handled by the machine driver toggling GPIOS. > >> The RaspberryPi kernel exposes a "hifiberry,dacpro-clk" clock driver >> which does nothing but tell the codec driver what rate is set by the >> machine driver [1][2] > >> This patch suggests an alternate fallback solution to let the codec >> driver know what the sysclk is based on the existing .set_sysclk >> callback. > > I'm not seeing a great advantage in this - we want to have more adoption > of standard APIs for clocking rather than less. As that Pi board shows > it's easy to create static clocks in the clock API so why not just do > that? What I find odd is the selection between the two static clocks. I have nothing against the clock API, I just find that the Pi solution isn't terribly elegant. The machine driver selects the 44.1 or 48kHz oscillator by configuring GPIO using registers exposed by the codec driver, the clock API implementation does nothing related to hardware, so we have three entities involved to flip 2 GPIOs. And even if I use this solution, I still don't know how to tell the codec driver which sclk to use in an ACPI environment (this is to enable HiFiberry cards on Up/Up^2). At the moment the code does pcm512x->sclk = devm_clk_get(dev, NULL); and I don't have a moral equivalent for ACPI. I can add information for the codec driver to get from the DSDT, I'd need however a agreed solution based on _DSD or some property, and the thread with AMD on their 'mclk_name' topic for DA7219 didn't go anywhere, did it? Bottom line there is an 'easy' solution for this specific case and a more complicated one but potentially more generic based on the clock API - hence the RFC.
On Mon, Jun 04, 2018 at 08:48:40AM -0500, Pierre-Louis Bossart wrote: > What I find odd is the selection between the two static clocks. I have > nothing against the clock API, I just find that the Pi solution isn't > terribly elegant. The machine driver selects the 44.1 or 48kHz oscillator by > configuring GPIO using registers exposed by the codec driver, the clock API > implementation does nothing related to hardware, so we have three entities > involved to flip 2 GPIOs. That just sounds like a not great implementation decision in that platform code rather than a fundamental problem. > And even if I use this solution, I still don't know how to tell the codec > driver which sclk to use in an ACPI environment (this is to enable HiFiberry > cards on Up/Up^2). At the moment the code does > pcm512x->sclk = devm_clk_get(dev, NULL); > and I don't have a moral equivalent for ACPI. I can add information for the > codec driver to get from the DSDT, I'd need however a agreed solution based > on _DSD or some property, and the thread with AMD on their 'mclk_name' topic > for DA7219 didn't go anywhere, did it? For systems out in the wild now you're going to need to have a bit of platform glue code anyway so I'm not sure there's that big an overhead (other than the data tables, but that seems idiomatic for ACPI). It's a bit of work to get something set up but I'd not expect it to be that bad, but perhaps I'm missing something there. > Bottom line there is an 'easy' solution for this specific case and a more > complicated one but potentially more generic based on the clock API - hence > the RFC. As far as I can see the two aren't that far off - creating a fixed frequency clock based on DMI data shouldn't be that much harder than doing a set_sysclk().
On 06/04/2018 11:27 AM, Mark Brown wrote: > On Mon, Jun 04, 2018 at 08:48:40AM -0500, Pierre-Louis Bossart wrote: > >> What I find odd is the selection between the two static clocks. I have >> nothing against the clock API, I just find that the Pi solution isn't >> terribly elegant. The machine driver selects the 44.1 or 48kHz oscillator by >> configuring GPIO using registers exposed by the codec driver, the clock API >> implementation does nothing related to hardware, so we have three entities >> involved to flip 2 GPIOs. > That just sounds like a not great implementation decision in that > platform code rather than a fundamental problem. > >> And even if I use this solution, I still don't know how to tell the codec >> driver which sclk to use in an ACPI environment (this is to enable HiFiberry >> cards on Up/Up^2). At the moment the code does >> pcm512x->sclk = devm_clk_get(dev, NULL); >> and I don't have a moral equivalent for ACPI. I can add information for the >> codec driver to get from the DSDT, I'd need however a agreed solution based >> on _DSD or some property, and the thread with AMD on their 'mclk_name' topic >> for DA7219 didn't go anywhere, did it? > For systems out in the wild now you're going to need to have a bit of > platform glue code anyway so I'm not sure there's that big an overhead > (other than the data tables, but that seems idiomatic for ACPI). It's > a bit of work to get something set up but I'd not expect it to be that > bad, but perhaps I'm missing something there. > >> Bottom line there is an 'easy' solution for this specific case and a more >> complicated one but potentially more generic based on the clock API - hence >> the RFC. > As far as I can see the two aren't that far off - creating a fixed > frequency clock based on DMI data shouldn't be that much harder than > doing a set_sysclk(). Not sure how DMI could be used for devices with connectors? The information should come from the additional ASL code that we typically add to SSDT or initrd, e.g. [1]. This ASL code can add additional information clock as needed, e.g. with _DSD properties, but the link with the clock API isn't there atm. [1] https://github.com/plbossart/acpi-scripts/blob/master/Up2/PCM512X.asl
On Mon, Jun 04, 2018 at 01:14:10PM -0500, Pierre-Louis Bossart wrote: > On 06/04/2018 11:27 AM, Mark Brown wrote: > > On Mon, Jun 04, 2018 at 08:48:40AM -0500, Pierre-Louis Bossart wrote: > > > Bottom line there is an 'easy' solution for this specific case and a more > > > complicated one but potentially more generic based on the clock API - hence > > > the RFC. > > As far as I can see the two aren't that far off - creating a fixed > > frequency clock based on DMI data shouldn't be that much harder than > > doing a set_sysclk(). > Not sure how DMI could be used for devices with connectors? The information > should come from the additional ASL code that we typically add to SSDT or > initrd, e.g. [1]. This ASL code can add additional information clock as > needed, e.g. with _DSD properties, but the link with the clock API isn't > there atm. Presumably there's some way you can add IDs for quirking with these? This is the idiomatic way of handling this sort of stuff in ACPI as far as I can tell. Otherwise presumably whatever works for letting you call set_sysclk() would let you set up a clock instead? It really feels like some of the ACPI users need to step up here and do the work on handling clocks.
diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c index f0f2d4fd3769..8d6c173c1e25 100644 --- a/sound/soc/codecs/pcm512x.c +++ b/sound/soc/codecs/pcm512x.c @@ -40,6 +40,7 @@ static const char * const pcm512x_supply_names[PCM512x_NUM_SUPPLIES] = { struct pcm512x_priv { struct regmap *regmap; struct clk *sclk; + int sysclk; struct regulator_bulk_data supplies[PCM512x_NUM_SUPPLIES]; struct notifier_block supply_nb[PCM512x_NUM_SUPPLIES]; int fmt; @@ -519,6 +520,29 @@ static int pcm512x_hw_rule_rate(struct snd_pcm_hw_params *params, ARRAY_SIZE(ranges), ranges, 0); } +static int sclk_get_rate(struct pcm512x_priv *pcm512x) +{ + if (!IS_ERR(pcm512x->sclk)) + return clk_get_rate(pcm512x->sclk); + else + return pcm512x->sysclk; +} + +static int pcm512x_set_sysclk(struct snd_soc_dai *dai, + int clk_id, unsigned int freq, int dir) +{ + struct snd_soc_component *component = dai->component; + struct pcm512x_priv *pcm512x = snd_soc_component_get_drvdata(component); + + switch (freq) { + case 22579200: + case 24576000: + pcm512x->sysclk = freq; + return 0; + } + return -EINVAL; +} + static int pcm512x_dai_startup_master(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { @@ -528,7 +552,7 @@ static int pcm512x_dai_startup_master(struct snd_pcm_substream *substream, struct snd_pcm_hw_constraint_ratnums *constraints_no_pll; struct snd_ratnum *rats_no_pll; - if (IS_ERR(pcm512x->sclk)) { + if (IS_ERR(pcm512x->sclk) && !pcm512x->sysclk) { dev_err(dev, "Need SCLK for master mode: %ld\n", PTR_ERR(pcm512x->sclk)); return PTR_ERR(pcm512x->sclk); @@ -551,7 +575,7 @@ static int pcm512x_dai_startup_master(struct snd_pcm_substream *substream, if (!rats_no_pll) return -ENOMEM; constraints_no_pll->rats = rats_no_pll; - rats_no_pll->num = clk_get_rate(pcm512x->sclk) / 64; + rats_no_pll->num = sclk_get_rate(pcm512x) / 64; rats_no_pll->den_min = 1; rats_no_pll->den_max = 128; rats_no_pll->den_step = 1; @@ -858,7 +882,7 @@ static int pcm512x_set_dividers(struct snd_soc_dai *dai, } if (!pcm512x->pll_out) { - sck_rate = clk_get_rate(pcm512x->sclk); + sck_rate = sclk_get_rate(pcm512x); bclk_div = params->rate_den * 64 / lrclk_div; bclk_rate = DIV_ROUND_CLOSEST(sck_rate, bclk_div); @@ -875,7 +899,7 @@ static int pcm512x_set_dividers(struct snd_soc_dai *dai, } bclk_rate = ret; - pllin_rate = clk_get_rate(pcm512x->sclk); + pllin_rate = sclk_get_rate(pcm512x); sck_rate = pcm512x_find_sck(dai, bclk_rate); if (!sck_rate) @@ -1323,6 +1347,7 @@ static const struct snd_soc_dai_ops pcm512x_dai_ops = { .startup = pcm512x_dai_startup, .hw_params = pcm512x_hw_params, .set_fmt = pcm512x_set_fmt, + .set_sysclk = pcm512x_set_sysclk, }; static struct snd_soc_dai_driver pcm512x_dai = {
For codec master mode, the current pcm512x code relies on the clock API. Simple boards such as Hifiberry DAC+ PRO rely on 2 local oscillators to generate 44.1 or 48kHz multiples, with the clock selection being handled by the machine driver toggling GPIOS. The RaspberryPi kernel exposes a "hifiberry,dacpro-clk" clock driver which does nothing but tell the codec driver what rate is set by the machine driver [1][2] This patch suggests an alternate fallback solution to let the codec driver know what the sysclk is based on the existing .set_sysclk callback. [1] https://github.com/raspberrypi/linux/blob/rpi-4.14.y/sound/soc/bcm/hifiberry_dacplus.c [2] https://github.com/raspberrypi/linux/blob/rpi-4.14.y/drivers/clk/clk-hifiberry-dacpro.c Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- sound/soc/codecs/pcm512x.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)