On 28/03/2022 08:14, Sameer Pujar wrote:

Convert rt5659.txt DT binding to YAML schema. This binding is applicable to rt5658 and rt5659 audio CODECs.

Signed-off-by: Sameer Pujar spujar@nvidia.com Cc: Oder Chiou oder_chiou@realtek.com

---

.../devicetree/bindings/sound/realtek,rt5659.yaml | 112 +++++++++++++++++++++ Documentation/devicetree/bindings/sound/rt5659.txt | 89 ---------------- 2 files changed, 112 insertions(+), 89 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/realtek,rt5659.yaml delete mode 100644 Documentation/devicetree/bindings/sound/rt5659.txt

diff --git a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml new file mode 100644 index 0000000..3bd9b6f --- /dev/null +++ b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml @@ -0,0 +1,112 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/realtek,rt5659.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: RT5658 and RT5659 audio CODECs

+description: This device supports I2C only.

+maintainers:

• • Oder Chiou oder_chiou@realtek.com

+allOf:

• • $ref: name-prefix.yaml#

+properties:

• compatible:
• enum:

•  - realtek,rt5658
  

•  - realtek,rt5659
  

• reg:
• description: The I2C address of the device

Skip the description, it's obvious.

• maxItems: 1
• interrupts:
• description: The CODEC's interrupt output

Ditto.

• maxItems: 1
• clocks:
• items:

•  - description: Master clock (MCLK) to the CODEC
  

• clock-names:
• items:

•  - const: mclk
  

• realtek,in1-differential:
• description: MIC1 input is differntial and not single-ended.

typo (differential)

• type: boolean
• realtek,in3-differential:
• description: MIC3 input is differntial and not single-ended.
• type: boolean
• realtek,in4-differential:
• description: MIC3 input is differntial and not single-ended.

MIC4?

• type: boolean
• realtek,dmic1-data-pin:
• description: DMIC1 data pin usage
• $ref: /schemas/types.yaml#/definitions/uint32
• enum:

•  - 0 # dmic1 is not used
  

•  - 1 # using IN2N pin as dmic1 data pin
  

•  - 2 # using GPIO5 pin as dmic1 data pin
  

•  - 3 # using GPIO9 pin as dmic1 data pin
  

•  - 4 # using GPIO11 pin as dmic1 data pin
  

• realtek,dmic2-data-pin:
• description: DMIC2 data pin usage
• $ref: /schemas/types.yaml#/definitions/uint32
• enum:

•  - 0 # dmic2 is not used
  

•  - 1 # using IN2P pin as dmic2 data pin
  

•  - 2 # using GPIO6 pin as dmic2 data pin
  

•  - 3 # using GPIO10 pin as dmic2 data pin
  

•  - 4 # using GPIO12 pin as dmic2 data pin
  

• realtek,jd-src:
• description: Jack detect source
• $ref: /schemas/types.yaml#/definitions/uint32
• enum:

•  - 0 # No JD is used
  

•  - 1 # using JD3 as JD source
  

•  - 2 # JD source for Intel HDA header
  

• realtek,ldo1-en-gpios:
• description: The GPIO that controls the CODEC's LDO1_EN pin.

maxItems

• realtek,reset-gpios:
• description: The GPIO that controls the CODEC's RESET pin.

maxItems

What about the ports node?

Best regards, Krzysztof

On 28-03-2022 12:36, Krzysztof Kozlowski wrote:

External email: Use caution opening links or attachments

On 28/03/2022 08:14, Sameer Pujar wrote:

...

The rt5658 or rt5659 CODEC system clock (SYSCLK) can be derived from various clock sources. For example it can be derived either from master clock (MCLK) or by internal PLL. The internal PLL again can take input clock references from bit clocks (BCLKs) and MCLK. To enable a flexible clocking configuration the DT binding is extended here.

It makes use of standard clock bindings and sets up the clock relation via DT.

Signed-off-by: Sameer Pujar spujar@nvidia.com Cc: Oder Chiou oder_chiou@realtek.com

---

.../devicetree/bindings/sound/realtek,rt5659.yaml | 53 ++++++++++++++++++++-- 1 file changed, 49 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml index b0485b8..0c2f3cb 100644 --- a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml +++ b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml @@ -29,12 +29,28 @@ properties: maxItems: 1

clocks:

• items:

•  - description: Master clock (MCLK) to the CODEC
  

• description: |

•  CODEC can receive multiple clock inputs like Master
  

•  clock (MCLK), I2S bit clocks (BCLK1, BCLK2, BCLK3,
  

•  BCLK4). The CODEC SYSCLK can be generated from MCLK
  

•  or internal PLL. In turn PLL can reference from MCLK
  

•  and BCLKs.
  
  

  clock-names:

• items:

•  - const: mclk
  

• description: |

•  The clock names can be combination of following:
  

•    "mclk"        : Master clock
  

•    "pll_ref"     : Reference to CODEC PLL clock
  

•    "sysclk"      : CODEC SYSCLK
  

•    "^bclk[1-4]$" : Bit clocks to CODEC
  

No, that does not look correct. You allow anything as clock input (even 20 clocks, different names, any order). That's not how DT schema should work and that's not how hardware looks like.

Usually the clock inputs are always there which also you mentioned in description - "multiple clock inputs". All these clocks should be expected, unless really the wires (physical wires) can be left disconnected.

The CODEC can receive multiple clocks but all the input clocks need not be present or connected always. If a specific configuration is needed and platform supports such an input, then all these inputs can be added. I don't know how to define this detail in the schema. If I make all of them expected, then binding check throws errors. If I were to list all the possible combinations, the list is going to be big (not sure if this would be OK?).

On 28-03-2022 13:37, Krzysztof Kozlowski wrote:

On 28/03/2022 09:58, Sameer Pujar wrote:

...

On 28-03-2022 12:36, Krzysztof Kozlowski wrote:

...

External email: Use caution opening links or attachments

On 28/03/2022 08:14, Sameer Pujar wrote:

...

The rt5658 or rt5659 CODEC system clock (SYSCLK) can be derived from various clock sources. For example it can be derived either from master clock (MCLK) or by internal PLL. The internal PLL again can take input clock references from bit clocks (BCLKs) and MCLK. To enable a flexible clocking configuration the DT binding is extended here.

It makes use of standard clock bindings and sets up the clock relation via DT.

Signed-off-by: Sameer Pujar spujar@nvidia.com Cc: Oder Chiou oder_chiou@realtek.com

---

.../devicetree/bindings/sound/realtek,rt5659.yaml | 53 ++++++++++++++++++++-- 1 file changed, 49 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml index b0485b8..0c2f3cb 100644 --- a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml +++ b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml @@ -29,12 +29,28 @@ properties: maxItems: 1

 clocks:

• items:

•  - description: Master clock (MCLK) to the CODEC
  

• description: |

•  CODEC can receive multiple clock inputs like Master
  

•  clock (MCLK), I2S bit clocks (BCLK1, BCLK2, BCLK3,
  

•  BCLK4). The CODEC SYSCLK can be generated from MCLK
  

•  or internal PLL. In turn PLL can reference from MCLK
  

•  and BCLKs.
  
  

  clock-names:

• items:

•  - const: mclk
  

• description: |

•  The clock names can be combination of following:
  

•    "mclk"        : Master clock
  

•    "pll_ref"     : Reference to CODEC PLL clock
  

•    "sysclk"      : CODEC SYSCLK
  

•    "^bclk[1-4]$" : Bit clocks to CODEC
  

No, that does not look correct. You allow anything as clock input (even 20 clocks, different names, any order). That's not how DT schema should work and that's not how hardware looks like. Usually the clock inputs are always there which also you mentioned in description - "multiple clock inputs". All these clocks should be expected, unless really the wires (physical wires) can be left disconnected.

The CODEC can receive multiple clocks but all the input clocks need not be present or connected always. If a specific configuration is needed and platform supports such an input, then all these inputs can be added. I don't know how to define this detail in the schema. If I make all of them expected, then binding check throws errors. If I were to list all the possible combinations, the list is going to be big (not sure if this would be OK?).

Thanks for explanation. Please differentiate between these two:

1. clock inputs connected, but unused (not needed for driver or for

particular use case), 2. clock inputs really not connected.

For the 1. above, such clock inputs should still be listed in the bindings and DTS. For the 2. above, such clocks should actually not be there.

Thank you for the suggestion.

How to achieve this depends on number of your combinations. IOW, how many clocks are physically optional.

From CODEC point of view all these clock inputs are possible and a platform may choose to connect a subset of it depending on the application. The binding is expected to support all such cases. To support all possibilities, the total combinations can be very big (100+).

For some small number of variations this can be: oneOf:

• const: mclk
• items:
  • const: mclk
  • enum:
    • bclk1
    • bclk2
    • bclk3
    • bclk4
• items:
  • const: mclk
  • const: pll_ref
  • enum:
    • bclk1
    • bclk2
    • bclk3
    • bclk4

For a total flexibility that any clock input can be disconnected, this should be a list of enums I guess (with minItems). However please find the clocks always connected and include them if possible in a fixed way (like this oneOf above).

May be I can list the most commonly required combinations like below and extend it whenever there is a need for specific combination?

clock-names:     oneOf:       - const: mclk       - pattern: '^bclk[1-4]$'       - items:           - const: mclk           - pattern: '^bclk[1-4]$'       - items:           - const: mclk           - const: sysclk       - items:           - const: mclk           - const: pll_ref           - const: sysclk       - items:           - pattern: '^bclk[1-4]$'           - const: pll_ref           - const: sysclk       - items:           - const: mclk           - pattern: '^bclk[1-4]$'           - const: pll_ref           - const: sysclk

On 28-03-2022 18:58, Krzysztof Kozlowski wrote:

On 28/03/2022 15:19, Sameer Pujar wrote:

...

On 28-03-2022 13:37, Krzysztof Kozlowski wrote:

...

On 28/03/2022 09:58, Sameer Pujar wrote:

...

On 28-03-2022 12:36, Krzysztof Kozlowski wrote:

...

External email: Use caution opening links or attachments

On 28/03/2022 08:14, Sameer Pujar wrote:

...

The rt5658 or rt5659 CODEC system clock (SYSCLK) can be derived from various clock sources. For example it can be derived either from master clock (MCLK) or by internal PLL. The internal PLL again can take input clock references from bit clocks (BCLKs) and MCLK. To enable a flexible clocking configuration the DT binding is extended here.

It makes use of standard clock bindings and sets up the clock relation via DT.

Signed-off-by: Sameer Pujarspujar@nvidia.com Cc: Oder Chiouoder_chiou@realtek.com

---

.../devicetree/bindings/sound/realtek,rt5659.yaml  | 53 ++++++++++++++++++++--
1 file changed, 49 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml index b0485b8..0c2f3cb 100644 --- a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml +++ b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml @@ -29,12 +29,28 @@ properties: maxItems: 1

  clocks:

• items:

•  - description: Master clock (MCLK) to the CODEC
  

• description: |

•  CODEC can receive multiple clock inputs like Master
  

•  clock (MCLK), I2S bit clocks (BCLK1, BCLK2, BCLK3,
  

•  BCLK4). The CODEC SYSCLK can be generated from MCLK
  

•  or internal PLL. In turn PLL can reference from MCLK
  

•  and BCLKs.
  
  clock-names:
  

• items:

•  - const: mclk
  

• description: |

•  The clock names can be combination of following:
  

•    "mclk"        : Master clock
  

•    "pll_ref"     : Reference to CODEC PLL clock
  

•    "sysclk"      : CODEC SYSCLK
  

•    "^bclk[1-4]$" : Bit clocks to CODEC
  

No, that does not look correct. You allow anything as clock input (even 20 clocks, different names, any order). That's not how DT schema should work and that's not how hardware looks like. Usually the clock inputs are always there which also you mentioned in description - "multiple clock inputs". All these clocks should be expected, unless really the wires (physical wires) can be left disconnected.

The CODEC can receive multiple clocks but all the input clocks need not be present or connected always. If a specific configuration is needed and platform supports such an input, then all these inputs can be added. I don't know how to define this detail in the schema. If I make all of them expected, then binding check throws errors. If I were to list all the possible combinations, the list is going to be big (not sure if this would be OK?).

Thanks for explanation. Please differentiate between these two:

1. clock inputs connected, but unused (not needed for driver or for

particular use case), 2. clock inputs really not connected.

For the 1. above, such clock inputs should still be listed in the bindings and DTS. For the 2. above, such clocks should actually not be there.

Thank you for the suggestion.

...

How to achieve this depends on number of your combinations. IOW, how many clocks are physically optional.

From CODEC point of view all these clock inputs are possible and a platform may choose to connect a subset of it depending on the application. The binding is expected to support all such cases. To support all possibilities, the total combinations can be very big (100+).

...

For some small number of variations this can be: oneOf:

• const: mclk
• items:
  • const: mclk
  • enum:
    • bclk1
    • bclk2
    • bclk3
    • bclk4
• items:
  • const: mclk
  • const: pll_ref
  • enum:
    • bclk1
    • bclk2
    • bclk3
    • bclk4

For a total flexibility that any clock input can be disconnected, this should be a list of enums I guess (with minItems). However please find the clocks always connected and include them if possible in a fixed way (like this oneOf above).

May be I can list the most commonly required combinations like below and extend it whenever there is a need for specific combination?

Yes, this would work. Relaxing such constraints is possible.

Thanks. I will update this in next revision.

On 3/28/2022 8:14 AM, Sameer Pujar wrote:

For DPCM links, the order of hw_param() call depends on the sequence of BE connection to FE. It is possible that one BE link can provide clock to another BE link. In such cases consumer BE DAI, to get the rate set by provider BE DAI, can use the standard clock functions only if provider has already set the appropriate rate during its hw_param() stage.

Above sentence seems to suggest that consumer can set clock only after provider has started, but code in this patch seems to do it the other way around?

Presently the order is fixed and does not depend on the provider and consumer relationships. So the clock rates need to be known ahead of hw_param() stage.

This patch tweaks the hw_param() order by connecting the provider BEs late to a FE. With this hw_param() calls for provider BEs happen first and then followed by consumer BEs. The consumers can use the standard clk_get_rate() function to get the rate of the clock they depend on.

I'm bit confused by " With this hw_param() calls for provider BEs happen first and then followed by consumer BEs. "

Aren't consumers started first and provider second? Code and previous sentence "connecting the provider BEs late to a FE" confuse me.

Overall I don't exactly understand correct order of events after reading commit message and patch...

Signed-off-by: Sameer Pujar spujar@nvidia.com

TODO:

• The FE link is not considered in this. For Tegra it is fine to call hw_params() for FE at the end. But systems, which want to apply this tweak for FE as well, have to extend this tweak to FE.
• Also only DPCM is considered here. If normal links require such tweak, it needs to be extended.

sound/soc/soc-pcm.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 9a95468..5829514 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1442,6 +1442,29 @@ static int dpcm_prune_paths(struct snd_soc_pcm_runtime *fe, int stream, return prune; }

+static bool defer_dpcm_be_connect(struct snd_soc_pcm_runtime *rtd) +{

• struct snd_soc_dai *dai;
• int i;
• if (!(rtd->dai_link->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK))

• return false;
  

• if ((rtd->dai_link->dai_fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) ==

•    SND_SOC_DAIFMT_CBC_CFC) {
  

• for_each_rtd_cpu_dais(rtd, i, dai) {
  

• 	if (!snd_soc_dai_is_dummy(dai))
  

• 		return true;
  

• }
  

• }
• return false;

+}

+#define MAX_CLK_PROVIDER_BE 10

Not sure about this define, it adds unnecessary limitation on max clock number, can't you just run same loop twice while checking defer_dpcm_be_connect() first time and !defer_dpcm_be_connect() second time? defer_dpcm_be_connect() function name may need a bit of adjustment (rtd_is_clock_consumer() maybe?), but it gets rid of the limit.

or do something like following instead of copy pasting loop twice:

rename original dpcm_add_paths() to _dpcm_add_paths() and add additional argument and check somewhere inline: static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, struct snd_soc_dapm_widget_list **list_, bool clock_consumer) { ...

// with renamed defer_dpcm_be_connect if (clock_consumer ^ !rtd_is_clock_consumer()) continue;

... }

static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, struct snd_soc_dapm_widget_list **list_) { int ret;

/* start clock consumer BEs */ ret = _dpcm_add_paths(*fe, stream, **list_, true); if (ret) return ret;

/* start clock provider BEs */ ret = _dpcm_add_paths(*fe, stream, **list_, false);

return ret; }

• static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, struct snd_soc_dapm_widget_list **list_) {

@@ -1449,7 +1472,8 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, struct snd_soc_dapm_widget_list *list = *list_; struct snd_soc_pcm_runtime *be; struct snd_soc_dapm_widget *widget;

• int i, new = 0, err;

• struct snd_soc_pcm_runtime *prov[MAX_CLK_PROVIDER_BE];

• int i, new = 0, err, count = 0;

  /* Create any new FE <--> BE connections */ for_each_dapm_widgets(list, i, widget) {

@@ -1489,6 +1513,40 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, (be->dpcm[stream].state != SND_SOC_DPCM_STATE_CLOSE)) continue;

• /* Connect clock provider BEs at the end */
  

• if (defer_dpcm_be_connect(be)) {
  

• 	if (count >= MAX_CLK_PROVIDER_BE) {
  

• 		dev_err(fe->dev, "ASoC: too many clock provider BEs\n");
  

• 		return -EINVAL;
  

• 	}
  

• 	prov[count++] = be;
  

• 	continue;
  

• }
  

• /* newly connected FE and BE */
  

• err = dpcm_be_connect(fe, be, stream);
  

• if (err < 0) {
  

• 	dev_err(fe->dev, "ASoC: can't connect %s\n",
  

• 		widget->name);
  

• 	break;
  

• } else if (err == 0) /* already connected */
  

• 	continue;
  

• /* new */
  

• dpcm_set_be_update_state(be, stream, SND_SOC_DPCM_UPDATE_BE);
  

• new++;
  

• }
• /*

• * Now connect clock provider BEs. A late connection means,
  

• * these BE links appear first in the list maintained by FE
  

• * and hw_param() call for these happen first.
  

Let's stick to ALSA terminology, hw_params() please, same in commit message.

• */
  

• for (i = 0; i < count; i++) {

• be = prov[i];
  

• /* newly connected FE and BE */ err = dpcm_be_connect(fe, be, stream); if (err < 0) {

On Mon, 2022-03-28 at 11:44 +0530, Sameer Pujar wrote:

For DPCM links, the order of hw_param() call depends on the sequence of BE connection to FE. It is possible that one BE link can provide clock to another BE link. In such cases consumer BE DAI, to get the rate set by provider BE DAI, can use the standard clock functions only if provider has already set the appropriate rate during its hw_param() stage.

Presently the order is fixed and does not depend on the provider and consumer relationships. So the clock rates need to be known ahead of hw_param() stage.

This patch tweaks the hw_param() order by connecting the provider BEs late to a FE. With this hw_param() calls for provider BEs happen first and then followed by consumer BEs. The consumers can use the standard clk_get_rate() function to get the rate of the clock they depend on.

Signed-off-by: Sameer Pujar spujar@nvidia.com

TODO:

• The FE link is not considered in this. For Tegra it is fine to call hw_params() for FE at the end. But systems, which want to

apply this tweak for FE as well, have to extend this tweak to FE.

• Also only DPCM is considered here. If normal links require such tweak, it needs to be extended.

sound/soc/soc-pcm.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 9a95468..5829514 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1442,6 +1442,29 @@ static int dpcm_prune_paths(struct snd_soc_pcm_runtime *fe, int stream, return prune; }

+static bool defer_dpcm_be_connect(struct snd_soc_pcm_runtime *rtd) +{

• struct snd_soc_dai *dai;
• int i;
• if (!(rtd->dai_link->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK))

• return false;
  

Is this check necessary?

• if ((rtd->dai_link->dai_fmt &

SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) ==

•    SND_SOC_DAIFMT_CBC_CFC) {
  

• for_each_rtd_cpu_dais(rtd, i, dai) {
  

• 	if (!snd_soc_dai_is_dummy(dai))
  

• 		return true;
  

• }
  

• }
• return false;

+}

+#define MAX_CLK_PROVIDER_BE 10

static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, struct snd_soc_dapm_widget_list **list_) { @@ -1449,7 +1472,8 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, struct snd_soc_dapm_widget_list *list = *list_; struct snd_soc_pcm_runtime *be; struct snd_soc_dapm_widget *widget;

• int i, new = 0, err;

• struct snd_soc_pcm_runtime *prov[MAX_CLK_PROVIDER_BE];

• int i, new = 0, err, count = 0;

  /* Create any new FE <--> BE connections */ for_each_dapm_widgets(list, i, widget) {

@@ -1489,6 +1513,40 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, (be->dpcm[stream].state != SND_SOC_DPCM_STATE_CLOSE)) continue;

• /* Connect clock provider BEs at the end */
  

• if (defer_dpcm_be_connect(be)) {
  

• 	if (count >= MAX_CLK_PROVIDER_BE) {
  

What determines MAX_CLK_PROVIDER_BE? why 10? Can you use rtd->num_cpus instead? Thanks, Ranjani