[PATCH v1 1/2] ASoC: simple-card: Support custom DAI system clock IDs
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(-)
diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h index a0b827f0c2f6..9f9a72299637 100644 --- a/include/sound/simple_card_utils.h +++ b/include/sound/simple_card_utils.h @@ -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; diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c index bef16833c487..d4d898e06e76 100644 --- a/sound/soc/generic/simple-card-utils.c +++ b/sound/soc/generic/simple-card-utils.c @@ -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) {
This is a new per-DAI property used to specify the clock ID argument to snd_soc_dai_set_sysclk().
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- Documentation/devicetree/bindings/sound/simple-card.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml b/Documentation/devicetree/bindings/sound/simple-card.yaml index ed19899bc94b..cb7774e235d0 100644 --- a/Documentation/devicetree/bindings/sound/simple-card.yaml +++ b/Documentation/devicetree/bindings/sound/simple-card.yaml @@ -57,6 +57,12 @@ definitions: single fixed sampling rate. $ref: /schemas/types.yaml#/definitions/flag
+ system-clock-id: + description: | + Specify the clock ID used for setting the DAI system clock. + Defaults to 0 if unspecified. + $ref: /schemas/types.yaml#/definitions/uint32 + mclk-fs: description: | Multiplication factor between stream rate and codec mclk. @@ -145,6 +151,8 @@ definitions: $ref: "#/definitions/system-clock-direction-out" system-clock-fixed: $ref: "#/definitions/system-clock-fixed" + system-clock-id: + $ref: "#/definitions/system-clock-id" required: - sound-dai
On 22/10/2022 12:27, Aidan MacDonald wrote:
This is a new per-DAI property used to specify the clock ID argument to snd_soc_dai_set_sysclk().
You did no show the use of this property and here you refer to some specific Linux driver implementation, so in total this does no look like a hardware property.
You also did not explain why do you need it (the most important piece of commit msg).
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com
Documentation/devicetree/bindings/sound/simple-card.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml b/Documentation/devicetree/bindings/sound/simple-card.yaml index ed19899bc94b..cb7774e235d0 100644 --- a/Documentation/devicetree/bindings/sound/simple-card.yaml +++ b/Documentation/devicetree/bindings/sound/simple-card.yaml @@ -57,6 +57,12 @@ definitions: single fixed sampling rate. $ref: /schemas/types.yaml#/definitions/flag
- system-clock-id:
- description: |
Specify the clock ID used for setting the DAI system clock.
With lack of explanation above, I would say - use common clock framework to choose a clock...
Best regards, Krzysztof
Krzysztof Kozlowski krzysztof.kozlowski@linaro.org writes:
On 22/10/2022 12:27, Aidan MacDonald wrote:
This is a new per-DAI property used to specify the clock ID argument to snd_soc_dai_set_sysclk().
You did no show the use of this property and here you refer to some specific Linux driver implementation, so in total this does no look like a hardware property.
You also did not explain why do you need it (the most important piece of commit msg).
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com
Documentation/devicetree/bindings/sound/simple-card.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml b/Documentation/devicetree/bindings/sound/simple-card.yaml index ed19899bc94b..cb7774e235d0 100644 --- a/Documentation/devicetree/bindings/sound/simple-card.yaml +++ b/Documentation/devicetree/bindings/sound/simple-card.yaml @@ -57,6 +57,12 @@ definitions: single fixed sampling rate. $ref: /schemas/types.yaml#/definitions/flag
- system-clock-id:
- description: |
Specify the clock ID used for setting the DAI system clock.With lack of explanation above, I would say - use common clock framework to choose a clock...
Best regards, Krzysztof
Sorry, I didn't explain things very well. The system clock ID is indeed a property of the DAI hardware. The ID is not specific to Linux in any way, and really it's an enumeration that requires a dt-binding.
A DAI may support multiple system clock inputs or outputs identified by the clock ID. In the case of outputs, these could be distinct clocks that have their own I/O pins, or the clock ID could select the internal source clock used for a clock generator. For inputs, the system clock ID may inform the DAI how or where the system clock is being provided so hardware registers can be configured appropriately.
Really the details do not matter, except that in a particular DAI link configuration a specific clock ID must be used. This is determined by the actual hardware connection between the DAIs; if the wrong clock is used, the DAI may not function correctly.
Currently the device tree is ambiguous as to which system clock should be used when the DAI supports more than one, because there is no way to specify which clock was intended. Linux just treats the ID as zero, but that's currently a Linux-specific numbering so there's guarantee that another OS would choose the same clock as Linux.
The system-clock-id property is therefore necessary to fully describe the hardware connection between DAIs in a DAI link when a DAI offers more than one choice of system clock.
I will resend the patch with the above in the commit message.
Regards, Aidan
On 23/10/2022 09:47, Aidan MacDonald wrote:
Krzysztof Kozlowski krzysztof.kozlowski@linaro.org writes:
On 22/10/2022 12:27, Aidan MacDonald wrote:
This is a new per-DAI property used to specify the clock ID argument to snd_soc_dai_set_sysclk().
You did no show the use of this property and here you refer to some specific Linux driver implementation, so in total this does no look like a hardware property.
You also did not explain why do you need it (the most important piece of commit msg).
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com
Documentation/devicetree/bindings/sound/simple-card.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml b/Documentation/devicetree/bindings/sound/simple-card.yaml index ed19899bc94b..cb7774e235d0 100644 --- a/Documentation/devicetree/bindings/sound/simple-card.yaml +++ b/Documentation/devicetree/bindings/sound/simple-card.yaml @@ -57,6 +57,12 @@ definitions: single fixed sampling rate. $ref: /schemas/types.yaml#/definitions/flag
- system-clock-id:
- description: |
Specify the clock ID used for setting the DAI system clock.With lack of explanation above, I would say - use common clock framework to choose a clock...
Best regards, Krzysztof
Sorry, I didn't explain things very well. The system clock ID is indeed a property of the DAI hardware. The ID is not specific to Linux in any way, and really it's an enumeration that requires a dt-binding.
A DAI may support multiple system clock inputs or outputs identified by the clock ID. In the case of outputs, these could be distinct clocks that have their own I/O pins, or the clock ID could select the internal source clock used for a clock generator. For inputs, the system clock ID may inform the DAI how or where the system clock is being provided so hardware registers can be configured appropriately.
Really the details do not matter, except that in a particular DAI link configuration a specific clock ID must be used. This is determined by the actual hardware connection between the DAIs; if the wrong clock is used, the DAI may not function correctly.
Currently the device tree is ambiguous as to which system clock should be used when the DAI supports more than one, because there is no way to specify which clock was intended. Linux just treats the ID as zero, but that's currently a Linux-specific numbering so there's guarantee that another OS would choose the same clock as Linux.
The system-clock-id property is therefore necessary to fully describe the hardware connection between DAIs in a DAI link when a DAI offers more than one choice of system clock.
I will resend the patch with the above in the commit message.
For example if you want to define which input pin to use (so you have internal mux), it's quite unspecific to give them some indexes. What is 0? What is 1? Number of pin? Number of pin counting from where?
Since this is unanswered, the IDs are also driver and implementation dependent, thus you still have the same problem - another OS can choose different clock. That's not then a hardware description, but software configuration.
Best regards, Krzysztof
Krzysztof Kozlowski krzysztof.kozlowski@linaro.org writes:
On 23/10/2022 09:47, Aidan MacDonald wrote:
Krzysztof Kozlowski krzysztof.kozlowski@linaro.org writes:
On 22/10/2022 12:27, Aidan MacDonald wrote:
This is a new per-DAI property used to specify the clock ID argument to snd_soc_dai_set_sysclk().
You did no show the use of this property and here you refer to some specific Linux driver implementation, so in total this does no look like a hardware property.
You also did not explain why do you need it (the most important piece of commit msg).
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com
Documentation/devicetree/bindings/sound/simple-card.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml b/Documentation/devicetree/bindings/sound/simple-card.yaml index ed19899bc94b..cb7774e235d0 100644 --- a/Documentation/devicetree/bindings/sound/simple-card.yaml +++ b/Documentation/devicetree/bindings/sound/simple-card.yaml @@ -57,6 +57,12 @@ definitions: single fixed sampling rate. $ref: /schemas/types.yaml#/definitions/flag
- system-clock-id:
- description: |
Specify the clock ID used for setting the DAI system clock.With lack of explanation above, I would say - use common clock framework to choose a clock...
Best regards, Krzysztof
Sorry, I didn't explain things very well. The system clock ID is indeed a property of the DAI hardware. The ID is not specific to Linux in any way, and really it's an enumeration that requires a dt-binding.
A DAI may support multiple system clock inputs or outputs identified by the clock ID. In the case of outputs, these could be distinct clocks that have their own I/O pins, or the clock ID could select the internal source clock used for a clock generator. For inputs, the system clock ID may inform the DAI how or where the system clock is being provided so hardware registers can be configured appropriately.
Really the details do not matter, except that in a particular DAI link configuration a specific clock ID must be used. This is determined by the actual hardware connection between the DAIs; if the wrong clock is used, the DAI may not function correctly.
Currently the device tree is ambiguous as to which system clock should be used when the DAI supports more than one, because there is no way to specify which clock was intended. Linux just treats the ID as zero, but that's currently a Linux-specific numbering so there's guarantee that another OS would choose the same clock as Linux.
The system-clock-id property is therefore necessary to fully describe the hardware connection between DAIs in a DAI link when a DAI offers more than one choice of system clock.
I will resend the patch with the above in the commit message.
For example if you want to define which input pin to use (so you have internal mux), it's quite unspecific to give them some indexes. What is 0? What is 1? Number of pin? Number of pin counting from where?
Since this is unanswered, the IDs are also driver and implementation dependent, thus you still have the same problem - another OS can choose different clock. That's not then a hardware description, but software configuration.
Best regards, Krzysztof
I answered this already. The enumeration is arbitrary. Create some dt-bindings and voila, it becomes standardized and OS-independent.
Regards, Aidan
On 24/10/2022 19:38, Aidan MacDonald wrote:
Krzysztof Kozlowski krzysztof.kozlowski@linaro.org writes:
On 23/10/2022 09:47, Aidan MacDonald wrote:
Krzysztof Kozlowski krzysztof.kozlowski@linaro.org writes:
On 22/10/2022 12:27, Aidan MacDonald wrote:
This is a new per-DAI property used to specify the clock ID argument to snd_soc_dai_set_sysclk().
You did no show the use of this property and here you refer to some specific Linux driver implementation, so in total this does no look like a hardware property.
You also did not explain why do you need it (the most important piece of commit msg).
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com
Documentation/devicetree/bindings/sound/simple-card.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml b/Documentation/devicetree/bindings/sound/simple-card.yaml index ed19899bc94b..cb7774e235d0 100644 --- a/Documentation/devicetree/bindings/sound/simple-card.yaml +++ b/Documentation/devicetree/bindings/sound/simple-card.yaml @@ -57,6 +57,12 @@ definitions: single fixed sampling rate. $ref: /schemas/types.yaml#/definitions/flag
- system-clock-id:
- description: |
Specify the clock ID used for setting the DAI system clock.With lack of explanation above, I would say - use common clock framework to choose a clock...
Best regards, Krzysztof
Sorry, I didn't explain things very well. The system clock ID is indeed a property of the DAI hardware. The ID is not specific to Linux in any way, and really it's an enumeration that requires a dt-binding.
A DAI may support multiple system clock inputs or outputs identified by the clock ID. In the case of outputs, these could be distinct clocks that have their own I/O pins, or the clock ID could select the internal source clock used for a clock generator. For inputs, the system clock ID may inform the DAI how or where the system clock is being provided so hardware registers can be configured appropriately.
Really the details do not matter, except that in a particular DAI link configuration a specific clock ID must be used. This is determined by the actual hardware connection between the DAIs; if the wrong clock is used, the DAI may not function correctly.
Currently the device tree is ambiguous as to which system clock should be used when the DAI supports more than one, because there is no way to specify which clock was intended. Linux just treats the ID as zero, but that's currently a Linux-specific numbering so there's guarantee that another OS would choose the same clock as Linux.
The system-clock-id property is therefore necessary to fully describe the hardware connection between DAIs in a DAI link when a DAI offers more than one choice of system clock.
I will resend the patch with the above in the commit message.
For example if you want to define which input pin to use (so you have internal mux), it's quite unspecific to give them some indexes. What is 0? What is 1? Number of pin? Number of pin counting from where?
Since this is unanswered, the IDs are also driver and implementation dependent, thus you still have the same problem - another OS can choose different clock. That's not then a hardware description, but software configuration.
Best regards, Krzysztof
I answered this already. The enumeration is arbitrary. Create some dt-bindings and voila, it becomes standardized and OS-independent.
Hm, then I missed something. Can you point me to DTS and bindings (patches or in-tree) which show this standardized indices of clock inputs?
Best regards, Krzysztof
Krzysztof Kozlowski krzysztof.kozlowski@linaro.org writes:
On 24/10/2022 19:38, Aidan MacDonald wrote:
Krzysztof Kozlowski krzysztof.kozlowski@linaro.org writes:
On 23/10/2022 09:47, Aidan MacDonald wrote:
Krzysztof Kozlowski krzysztof.kozlowski@linaro.org writes:
On 22/10/2022 12:27, Aidan MacDonald wrote:
This is a new per-DAI property used to specify the clock ID argument to snd_soc_dai_set_sysclk().
You did no show the use of this property and here you refer to some specific Linux driver implementation, so in total this does no look like a hardware property.
You also did not explain why do you need it (the most important piece of commit msg).
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com
Documentation/devicetree/bindings/sound/simple-card.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml b/Documentation/devicetree/bindings/sound/simple-card.yaml index ed19899bc94b..cb7774e235d0 100644 --- a/Documentation/devicetree/bindings/sound/simple-card.yaml +++ b/Documentation/devicetree/bindings/sound/simple-card.yaml @@ -57,6 +57,12 @@ definitions: single fixed sampling rate. $ref: /schemas/types.yaml#/definitions/flag
- system-clock-id:
- description: |
Specify the clock ID used for setting the DAI system clock.With lack of explanation above, I would say - use common clock framework to choose a clock...
Best regards, Krzysztof
Sorry, I didn't explain things very well. The system clock ID is indeed a property of the DAI hardware. The ID is not specific to Linux in any way, and really it's an enumeration that requires a dt-binding.
A DAI may support multiple system clock inputs or outputs identified by the clock ID. In the case of outputs, these could be distinct clocks that have their own I/O pins, or the clock ID could select the internal source clock used for a clock generator. For inputs, the system clock ID may inform the DAI how or where the system clock is being provided so hardware registers can be configured appropriately.
Really the details do not matter, except that in a particular DAI link configuration a specific clock ID must be used. This is determined by the actual hardware connection between the DAIs; if the wrong clock is used, the DAI may not function correctly.
Currently the device tree is ambiguous as to which system clock should be used when the DAI supports more than one, because there is no way to specify which clock was intended. Linux just treats the ID as zero, but that's currently a Linux-specific numbering so there's guarantee that another OS would choose the same clock as Linux.
The system-clock-id property is therefore necessary to fully describe the hardware connection between DAIs in a DAI link when a DAI offers more than one choice of system clock.
I will resend the patch with the above in the commit message.
For example if you want to define which input pin to use (so you have internal mux), it's quite unspecific to give them some indexes. What is 0? What is 1? Number of pin? Number of pin counting from where?
Since this is unanswered, the IDs are also driver and implementation dependent, thus you still have the same problem - another OS can choose different clock. That's not then a hardware description, but software configuration.
Best regards, Krzysztof
I answered this already. The enumeration is arbitrary. Create some dt-bindings and voila, it becomes standardized and OS-independent.
Hm, then I missed something. Can you point me to DTS and bindings (patches or in-tree) which show this standardized indices of clock inputs?
Best regards, Krzysztof
Device trees already use standardized enumerations in other areas so it isn't a new idea. Look under include/dt-bindings/clock. Every header there contains an arbitrary enumeration of a device's clocks. In fact most of include/dt-bindings is exactly for this purpose, to define standard values that are not "just numbers" but an enum, a flag, etc, with a special meaning. It is not specific to clocks.
There is no dt-binding for system clock ID, because prior to this patch they were not exposed to DT in any way. But the enumerations themselves already exist, eg. the IDs for nau8821 codec:
/* System Clock Source */ enum { NAU8821_CLK_DIS, NAU8821_CLK_MCLK, NAU8821_CLK_INTERNAL, NAU8821_CLK_FLL_MCLK, NAU8821_CLK_FLL_BLK, NAU8821_CLK_FLL_FS, };
We would just be moving these into dt-bindings if somebody wants to use a codec with simple-card. Future drivers would add the enum into dt-bindings from the start because that's where it belongs.
Regards, Aidan
On 25/10/2022 05:14, Aidan MacDonald wrote:
Krzysztof
Device trees already use standardized enumerations in other areas so it isn't a new idea. Look under include/dt-bindings/clock. Every header there contains an arbitrary enumeration of a device's clocks. In fact most of include/dt-bindings is exactly for this purpose, to define standard values that are not "just numbers" but an enum, a flag, etc, with a special meaning. It is not specific to clocks.
There is no dt-binding for system clock ID, because prior to this patch they were not exposed to DT in any way. But the enumerations themselves already exist, eg. the IDs for nau8821 codec:
/* System Clock Source */ enum { NAU8821_CLK_DIS, NAU8821_CLK_MCLK, NAU8821_CLK_INTERNAL, NAU8821_CLK_FLL_MCLK, NAU8821_CLK_FLL_BLK, NAU8821_CLK_FLL_FS, };
OK, this looks good.
We would just be moving these into dt-bindings if somebody wants to use a codec with simple-card. Future drivers would add the enum into dt-bindings from the start because that's where it belongs.
And the remaining piece I don't get is that these are not bindings for codec, but for sound audio card. You want to set "system-clock-id" property for audio card, while putting clock from codec, which will be used to pass back to the codec... so it is a property of the codec, not of the audio card. IOW, NAU8821_CLK_* does not configure here the clock of the system, but only, only clock of the codec.
Best regards, Krzysztof
Krzysztof Kozlowski krzysztof.kozlowski@linaro.org writes:
And the remaining piece I don't get is that these are not bindings for codec, but for sound audio card. You want to set "system-clock-id" property for audio card, while putting clock from codec, which will be used to pass back to the codec... so it is a property of the codec, not of the audio card. IOW, NAU8821_CLK_* does not configure here the clock of the system, but only, only clock of the codec.
The system clock is controlled at the DAI level, it's specific to one DAI on one component. The simple-card device node has sub-nodes for the DAI links, and each DAI link node has sub-nodes for the DAIs within the link. "system-clock-id" is a property on the DAI nodes, so it's not a card-level property, just one part of the overall card definition.
Since the clock ID is something defined by the codec it would naturally be a value defined by the codec, but the *configuration* of the codec is part of the sound card because it depends on how everything is connected together. If you used the same codec in a different machine it would have a different configuration.
Regards, Aidan
On 26/10/2022 10:48, Aidan MacDonald wrote:
Krzysztof Kozlowski krzysztof.kozlowski@linaro.org writes:
And the remaining piece I don't get is that these are not bindings for codec, but for sound audio card. You want to set "system-clock-id" property for audio card, while putting clock from codec, which will be used to pass back to the codec... so it is a property of the codec, not of the audio card. IOW, NAU8821_CLK_* does not configure here the clock of the system, but only, only clock of the codec.
The system clock is controlled at the DAI level, it's specific to one DAI on one component. The simple-card device node has sub-nodes for the DAI links, and each DAI link node has sub-nodes for the DAIs within the link. "system-clock-id" is a property on the DAI nodes, so it's not a card-level property, just one part of the overall card definition.
Since the clock ID is something defined by the codec it would naturally be a value defined by the codec, but the *configuration* of the codec is part of the sound card because it depends on how everything is connected together. If you used the same codec in a different machine it would have a different configuration.
OK, that sounds reasonable. Thank you for explaining this. You still need to convince Mark :)
Best regards, Krzysztof
Krzysztof Kozlowski krzysztof.kozlowski@linaro.org writes:
On 26/10/2022 10:48, Aidan MacDonald wrote:
Krzysztof Kozlowski krzysztof.kozlowski@linaro.org writes:
And the remaining piece I don't get is that these are not bindings for codec, but for sound audio card. You want to set "system-clock-id" property for audio card, while putting clock from codec, which will be used to pass back to the codec... so it is a property of the codec, not of the audio card. IOW, NAU8821_CLK_* does not configure here the clock of the system, but only, only clock of the codec.
The system clock is controlled at the DAI level, it's specific to one DAI on one component. The simple-card device node has sub-nodes for the DAI links, and each DAI link node has sub-nodes for the DAIs within the link. "system-clock-id" is a property on the DAI nodes, so it's not a card-level property, just one part of the overall card definition.
Since the clock ID is something defined by the codec it would naturally be a value defined by the codec, but the *configuration* of the codec is part of the sound card because it depends on how everything is connected together. If you used the same codec in a different machine it would have a different configuration.
OK, that sounds reasonable. Thank you for explaining this. You still need to convince Mark :)
No problem, thanks for bearing with all my explanations! Mark raised some good points, and I have to agree with him. This could create too many future issues, and the problem might be better solved with the clock API -- but unfortunately that's not yet feasible.
Regards, Aidan
On 22/10/2022 12:27, Aidan MacDonald wrote:
This is a new per-DAI property used to specify the clock ID argument to snd_soc_dai_set_sysclk().
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com
Documentation/devicetree/bindings/sound/simple-card.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)
My concerns were addressed, so:
Acked-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
Best regards, Krzysztof
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
participants (4)
-
Aidan MacDonald -
Krzysztof Kozlowski -
Kuninori Morimoto -
Mark Brown