[v1,1/2] ASoC: simple-card: Support custom DAI system clock IDs
Commit Message
Some DAIs have multiple system clock sources, which can be chosen
using the "clk_id" argument to snd_soc_dai_set_sysclk(). Currently
this is hardcoded to 0 when using simple cards, but that choice is
not always suitable.
Add the "system-clock-id" property to allow selecting a different
clock ID on a per-DAI basis.
To simplify the logic on DPCM cards, add a dummy "asoc_simple_dai"
instance and use that for the dummy components on DPCM links. This
ensures that when we're iterating over DAIs in the PCM runtime there
is always a matching "asoc_simple_dai" we can dereference.
Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
include/sound/simple_card_utils.h | 2 ++
sound/soc/generic/simple-card-utils.c | 26 ++++++++++++++++++++------
2 files changed, 22 insertions(+), 6 deletions(-)
Comments
Hi Aidan
Thank you for your patch
> Some DAIs have multiple system clock sources, which can be chosen
> using the "clk_id" argument to snd_soc_dai_set_sysclk(). Currently
> this is hardcoded to 0 when using simple cards, but that choice is
> not always suitable.
>
> Add the "system-clock-id" property to allow selecting a different
> clock ID on a per-DAI basis.
>
> To simplify the logic on DPCM cards, add a dummy "asoc_simple_dai"
> instance and use that for the dummy components on DPCM links. This
> ensures that when we're iterating over DAIs in the PCM runtime there
> is always a matching "asoc_simple_dai" we can dereference.
>
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
I think adding "system-clock-id" and adding "dummy asoc_simple_dai" are
different topics. This patch should be separated into 2 patches.
And I couldn't understand the reason why we need to add dummy asoc_simple_dai.
In my understanding, we don't parse DT for dummy connection.
Which process are you talking about specifically here?
This ensures that when we're iterating over DAIs in the PCM runtime there
is always a matching "asoc_simple_dai" we can dereference.
-
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> writes:
> Hi Aidan
>
> Thank you for your patch
>
>> Some DAIs have multiple system clock sources, which can be chosen
>> using the "clk_id" argument to snd_soc_dai_set_sysclk(). Currently
>> this is hardcoded to 0 when using simple cards, but that choice is
>> not always suitable.
>>
>> Add the "system-clock-id" property to allow selecting a different
>> clock ID on a per-DAI basis.
>>
>> To simplify the logic on DPCM cards, add a dummy "asoc_simple_dai"
>> instance and use that for the dummy components on DPCM links. This
>> ensures that when we're iterating over DAIs in the PCM runtime there
>> is always a matching "asoc_simple_dai" we can dereference.
>>
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>> ---
>
> I think adding "system-clock-id" and adding "dummy asoc_simple_dai" are
> different topics. This patch should be separated into 2 patches.
Sounds good to me.
> And I couldn't understand the reason why we need to add dummy asoc_simple_dai.
> In my understanding, we don't parse DT for dummy connection.
> Which process are you talking about specifically here?
>
> This ensures that when we're iterating over DAIs in the PCM runtime there
> is always a matching "asoc_simple_dai" we can dereference.
> -
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto
DPCM DAI links have some real DAIs and one dummy DAI. Each real DAI has
an asoc_simple_dai associated with it to contain the information parsed
from the DT. The dummy DAI does not have an asoc_simple_dai. I'm adding
a dummy asoc_simple_dai for these dummy DAIs to make the mapping of
snd_soc_dai to asoc_simple_dai 1-to-1.
The non 1-to-1 mapping is problematic, because if I have a snd_soc_dai
and want to look up a simple-card property I would need to check if the
matching asoc_simple_dai exists first, and have a special case for DPCM
dummy DAIs. With a 1-to-1 mapping I can handle all DAIs the same way.
Regards,
Aidan
On Sat, Oct 22, 2022 at 05:27:41PM +0100, Aidan MacDonald wrote:
> Some DAIs have multiple system clock sources, which can be chosen
> using the "clk_id" argument to snd_soc_dai_set_sysclk(). Currently
> this is hardcoded to 0 when using simple cards, but that choice is
> not always suitable.
We already have clock bindings, if we need to configure clocks we should
be using those to configure there.
Mark Brown <broonie@kernel.org> writes:
> On Sat, Oct 22, 2022 at 05:27:41PM +0100, Aidan MacDonald wrote:
>
>> Some DAIs have multiple system clock sources, which can be chosen
>> using the "clk_id" argument to snd_soc_dai_set_sysclk(). Currently
>> this is hardcoded to 0 when using simple cards, but that choice is
>> not always suitable.
>
> We already have clock bindings, if we need to configure clocks we should
> be using those to configure there.
The existing clock bindings are only useful for setting rates, and
.set_sysclk() does more than that. See my reply to Krzysztof if you
want an explanation, check nau8821 or tas2552 codecs for an example
of the kind of thing I'm talking about.
I picked those codecs at random, but they are fairly representative:
often a codec will allow the system clock to be derived from another
I2S clock (eg. BCLK), or provided directly, or maybe generated from an
internal PLL. In cases like that you need to configure the codec with
.set_sysclk() to select the right input. Many card drivers need to do
this, it's just as important as .set_fmt() or .hw_params().
Normal DT clocks don't seem capable of doing the job of .set_sysclk()
even in principle.
Regards,
Aidan
On Tue, Oct 25, 2022 at 12:17:25AM +0100, Aidan MacDonald wrote:
> Mark Brown <broonie@kernel.org> writes:
> > We already have clock bindings, if we need to configure clocks we should
> > be using those to configure there.
> The existing clock bindings are only useful for setting rates, and
> .set_sysclk() does more than that. See my reply to Krzysztof if you
> want an explanation, check nau8821 or tas2552 codecs for an example
> of the kind of thing I'm talking about.
I thought there was stuff for muxes, but in any case if you are adding a
new binding here you could just as well add one to the clock bindings.
> I picked those codecs at random, but they are fairly representative:
> often a codec will allow the system clock to be derived from another
> I2S clock (eg. BCLK), or provided directly, or maybe generated from an
> internal PLL. In cases like that you need to configure the codec with
> .set_sysclk() to select the right input. Many card drivers need to do
> this, it's just as important as .set_fmt() or .hw_params().
There is a strong case for saying that all the clocking in CODECs might
fit into the clock API, especially given the whole DT thing.
Mark Brown <broonie@kernel.org> writes:
> On Tue, Oct 25, 2022 at 12:17:25AM +0100, Aidan MacDonald wrote:
>> Mark Brown <broonie@kernel.org> writes:
>
>> > We already have clock bindings, if we need to configure clocks we should
>> > be using those to configure there.
>
>> The existing clock bindings are only useful for setting rates, and
>> .set_sysclk() does more than that. See my reply to Krzysztof if you
>> want an explanation, check nau8821 or tas2552 codecs for an example
>> of the kind of thing I'm talking about.
>
> I thought there was stuff for muxes, but in any case if you are adding a
> new binding here you could just as well add one to the clock bindings.
>
>> I picked those codecs at random, but they are fairly representative:
>> often a codec will allow the system clock to be derived from another
>> I2S clock (eg. BCLK), or provided directly, or maybe generated from an
>> internal PLL. In cases like that you need to configure the codec with
>> .set_sysclk() to select the right input. Many card drivers need to do
>> this, it's just as important as .set_fmt() or .hw_params().
>
> There is a strong case for saying that all the clocking in CODECs might
> fit into the clock API, especially given the whole DT thing.
The ASoC APIs don't speak "struct clk", which seems (to me) like a
prerequisite before we can think about doing anything with clocks.
Even if ASoC began to use the clock API for codec clocking, it's not
clear how you maintain backward compatibility with the existing
simple-card bindings. You'd have to go over all DAIs and mimic the
effects of "snd_soc_dai_set_sysclk(dai, 0, freq, dir)" because there
could be a device tree relying on it somewhere.
So... given you're already stuck maintaining .set_sysclk() behavior
forever, is there much harm in exposing the sysclock ID to the DT?
On Wed, Oct 26, 2022 at 03:42:31PM +0100, Aidan MacDonald wrote:
> Mark Brown <broonie@kernel.org> writes:
> > There is a strong case for saying that all the clocking in CODECs might
> > fit into the clock API, especially given the whole DT thing.
> The ASoC APIs don't speak "struct clk", which seems (to me) like a
> prerequisite before we can think about doing anything with clocks.
Right, they probably should.
> Even if ASoC began to use the clock API for codec clocking, it's not
> clear how you maintain backward compatibility with the existing
> simple-card bindings. You'd have to go over all DAIs and mimic the
> effects of "snd_soc_dai_set_sysclk(dai, 0, freq, dir)" because there
> could be a device tree relying on it somewhere.
Of course, you'd need to define bindings for devices with multiple
clocks such that things continue to work out compatibly.
> So... given you're already stuck maintaining .set_sysclk() behavior
> forever, is there much harm in exposing the sysclock ID to the DT?
Yes, it's ABI and the more baked in this stuff gets the more issues we
have when trying to integrate with the wider clock tree in the system -
for example when devices are able to output their system clock to be
used as a master clock for a device which can use the clock API as an
input. It's fine in kernel but we should be trying to keep it out of
ABI.
Mark Brown <broonie@kernel.org> writes:
> On Wed, Oct 26, 2022 at 03:42:31PM +0100, Aidan MacDonald wrote:
>> Mark Brown <broonie@kernel.org> writes:
>>
>>> There is a strong case for saying that all the clocking in CODECs might
>>> fit into the clock API, especially given the whole DT thing.
>>>
>> The ASoC APIs don't speak "struct clk", which seems (to me) like a
>> prerequisite before we can think about doing anything with clocks.
>
> Right, they probably should.
Let me throw out an idea then... the clock API will probably need to
gain the ability to express constraints, eg. limit a clock to set of
frequencies or frequency ranges; ratio constraints to ensure a set of
clocks are in one of a specified set of ratios; and maybe require that
certain clocks be synchronous.
This'd go a long way toward making the clock API suitable for audio
clocking.
>> Even if ASoC began to use the clock API for codec clocking, it's not
>> clear how you maintain backward compatibility with the existing
>> simple-card bindings. You'd have to go over all DAIs and mimic the
>> effects of "snd_soc_dai_set_sysclk(dai, 0, freq, dir)" because there
>> could be a device tree relying on it somewhere.
>
> Of course, you'd need to define bindings for devices with multiple
> clocks such that things continue to work out compatibly.
>
>> So... given you're already stuck maintaining .set_sysclk() behavior
>> forever, is there much harm in exposing the sysclock ID to the DT?
>
> Yes, it's ABI and the more baked in this stuff gets the more issues we
> have when trying to integrate with the wider clock tree in the system -
> for example when devices are able to output their system clock to be
> used as a master clock for a device which can use the clock API as an
> input. It's fine in kernel but we should be trying to keep it out of
> ABI.
Fair enough. It's too bad this limits the usefulness of simple-card,
but for the time being I'm happy maintaining these patches out of tree.
Regards,
Aidan
@@ -26,6 +26,7 @@ struct asoc_simple_dai {
const char *name;
unsigned int sysclk;
int clk_direction;
+ int sysclk_id;
int slots;
int slot_width;
unsigned int tx_slot_mask;
@@ -67,6 +68,7 @@ struct asoc_simple_priv {
struct prop_nums num;
unsigned int mclk_fs;
} *dai_props;
+ struct asoc_simple_dai dummy_dai;
struct asoc_simple_jack hp_jack;
struct asoc_simple_jack mic_jack;
struct snd_soc_dai_link *dai_link;
@@ -262,6 +262,9 @@ int asoc_simple_parse_clk(struct device *dev,
if (of_property_read_bool(node, "system-clock-direction-out"))
simple_dai->clk_direction = SND_SOC_CLOCK_OUT;
+ if (!of_property_read_u32(node, "system-clock-id", &val))
+ simple_dai->sysclk_id = val;
+
return 0;
}
EXPORT_SYMBOL_GPL(asoc_simple_parse_clk);
@@ -355,7 +358,7 @@ void asoc_simple_shutdown(struct snd_pcm_substream *substream)
if (props->mclk_fs && !dai->clk_fixed && !snd_soc_dai_active(cpu_dai))
snd_soc_dai_set_sysclk(cpu_dai,
- 0, 0, SND_SOC_CLOCK_OUT);
+ dai->sysclk_id, 0, SND_SOC_CLOCK_OUT);
asoc_simple_clk_disable(dai);
}
@@ -364,7 +367,7 @@ void asoc_simple_shutdown(struct snd_pcm_substream *substream)
if (props->mclk_fs && !dai->clk_fixed && !snd_soc_dai_active(codec_dai))
snd_soc_dai_set_sysclk(codec_dai,
- 0, 0, SND_SOC_CLOCK_IN);
+ dai->sysclk_id, 0, SND_SOC_CLOCK_IN);
asoc_simple_clk_disable(dai);
}
@@ -439,7 +442,7 @@ int asoc_simple_hw_params(struct snd_pcm_substream *substream,
struct asoc_simple_priv *priv = snd_soc_card_get_drvdata(rtd->card);
struct simple_dai_props *props = simple_priv_to_props(priv, rtd->num);
unsigned int mclk, mclk_fs = 0;
- int i, ret;
+ int i, ret, sysclk_id;
if (props->mclk_fs)
mclk_fs = props->mclk_fs;
@@ -472,13 +475,21 @@ int asoc_simple_hw_params(struct snd_pcm_substream *substream,
}
for_each_rtd_codec_dais(rtd, i, sdai) {
- ret = snd_soc_dai_set_sysclk(sdai, 0, mclk, SND_SOC_CLOCK_IN);
+ pdai = simple_props_to_dai_codec(props, i);
+ sysclk_id = pdai->sysclk_id;
+
+ ret = snd_soc_dai_set_sysclk(sdai, sysclk_id, mclk,
+ SND_SOC_CLOCK_IN);
if (ret && ret != -ENOTSUPP)
return ret;
}
for_each_rtd_cpu_dais(rtd, i, sdai) {
- ret = snd_soc_dai_set_sysclk(sdai, 0, mclk, SND_SOC_CLOCK_OUT);
+ pdai = simple_props_to_dai_cpu(props, i);
+ sysclk_id = pdai->sysclk_id;
+
+ ret = snd_soc_dai_set_sysclk(sdai, pdai->sysclk_id, mclk,
+ SND_SOC_CLOCK_OUT);
if (ret && ret != -ENOTSUPP)
return ret;
}
@@ -523,7 +534,8 @@ static int asoc_simple_init_dai(struct snd_soc_dai *dai,
return 0;
if (simple_dai->sysclk) {
- ret = snd_soc_dai_set_sysclk(dai, 0, simple_dai->sysclk,
+ ret = snd_soc_dai_set_sysclk(dai, simple_dai->sysclk_id,
+ simple_dai->sysclk,
simple_dai->clk_direction);
if (ret && ret != -ENOTSUPP) {
dev_err(dai->dev, "simple-card: set_sysclk error\n");
@@ -858,6 +870,7 @@ int asoc_simple_init_priv(struct asoc_simple_priv *priv,
dai_link[i].cpus = &priv->dummy;
dai_props[i].num.cpus =
dai_link[i].num_cpus = 1;
+ dai_props[i].cpu_dai = &priv->dummy_dai;
}
if (li->num[i].codecs) {
@@ -882,6 +895,7 @@ int asoc_simple_init_priv(struct asoc_simple_priv *priv,
dai_link[i].codecs = &priv->dummy;
dai_props[i].num.codecs =
dai_link[i].num_codecs = 1;
+ dai_props[i].codec_dai = &priv->dummy_dai;
}
if (li->num[i].platforms) {