[RFC PATCH v2 0/6] Flexible codec clock configuration

[RFC PATCH v2 0/6] Flexible codec clock configuration

[Sameer Pujar]

Typically the codec drivers require setting up of Sysclk. Sometimes
presence of internal PLL can provide more options of Sysclk configuration.
Presently ASoC provides callbacks set_sysclk() and set_pll() in such
cases. However it comes with following limitations considering generic
machine drivers (simple-card or audio-graph-card):

 1. The Sysclk source needs to be passed to set_sysclk() callback.
    Presently simple-card or audio-graph-card card rely on default
    source value (which is 0). If any other source needs to be used,
    it is currently not possible.

 2. The same would be true for codec PLL configuration as well, though
    simple-card or audio-graph-card don't have support yet for the PLL
    configuration.


Earlier attempt[0] to address above was not felt suitable. The suggestion
was to use standard clock based bindings instead.

This RFC series takes RT5659 as a reference and exposes clock relationships
via DT. **This is not in the final shape yet**, but I wanted to get some
valuable feedback to understand if the idea is right. If this appears fine,
this can be extended to other codecs (wherever necessary).

This does not completely remove the need of set_sysclk() callback because
the clock requirement (MCLK * fs) would come from the machine driver. But
machine driver need not worry about Sysclk source. It would be internally
managed by Codec via DT clock relationships.


[0] https://patchwork.kernel.org/project/alsa-devel/list/?series=438531&archive=both&state=*


Changelog:
==========

  v1 -> v2:
  ---------
    * New patch added to convert rt5659 binding doc to YAML format
    * New patch added to document audio-graph-port binding for rt5659
    * New patch added to document clock binding enhancement for rt5659

Sameer Pujar (6):
  ASoC: dt-bindings: Convert rt5659 bindings to YAML schema
  ASoC: dt-bindings: Add audio-graph-port bindings to rt5659
  ASoC: dt-bindings: Extend clock bindings of rt5659
  ASoC: soc-pcm: tweak DPCM BE hw_param() call order
  ASoC: rt5659: Expose internal clock relationships
  ASoC: tegra: Get clock rate in consumer mode

 .../devicetree/bindings/sound/realtek,rt5659.yaml  | 179 +++++++++++++++
 Documentation/devicetree/bindings/sound/rt5659.txt |  89 --------
 sound/soc/codecs/rt5659.c                          | 248 ++++++++++++++++++++-
 sound/soc/codecs/rt5659.h                          |   9 +
 sound/soc/soc-pcm.c                                |  60 ++++-
 sound/soc/tegra/tegra210_i2s.c                     |  25 ++-
 6 files changed, 503 insertions(+), 107 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/realtek,rt5659.yaml
 delete mode 100644 Documentation/devicetree/bindings/sound/rt5659.txt

-- 
2.7.4

[RFC PATCH v2 1/6] ASoC: dt-bindings: Convert rt5659 bindings to YAML schema

[Sameer Pujar]

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
+    maxItems: 1
+
+  interrupts:
+    description: The CODEC's interrupt output
+    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.
+    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.
+    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.
+
+  realtek,reset-gpios:
+    description: The GPIO that controls the CODEC's RESET pin.
+
+  sound-name-prefix: true
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+examples:
+  - |
+    #include<dt-bindings/gpio/tegra194-gpio.h>
+    #include<dt-bindings/clock/tegra194-clock.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        audio-codec@1a {
+            compatible = "realtek,rt5658";
+            reg = <0x1a>;
+            interrupt-parent = <&gpio>;
+            interrupts = <TEGRA194_MAIN_GPIO(S, 5) GPIO_ACTIVE_HIGH>;
+            clocks = <&bpmp TEGRA194_CLK_AUD_MCLK>;
+            clock-names = "mclk";
+            realtek,jd-src = <2>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/sound/rt5659.txt b/Documentation/devicetree/bindings/sound/rt5659.txt
deleted file mode 100644
index 013f534..0000000
--- a/Documentation/devicetree/bindings/sound/rt5659.txt
+++ /dev/null
@@ -1,89 +0,0 @@
-RT5659/RT5658 audio CODEC
-
-This device supports I2C only.
-
-Required properties:
-
-- compatible : One of "realtek,rt5659" or "realtek,rt5658".
-
-- reg : The I2C address of the device.
-
-- interrupts : The CODEC's interrupt output.
-
-Optional properties:
-
-- clocks: The phandle of the master clock to the CODEC
-- clock-names: Should be "mclk"
-
-- realtek,in1-differential
-- realtek,in3-differential
-- realtek,in4-differential
-  Boolean. Indicate MIC1/3/4 input are differential, rather than single-ended.
-
-- realtek,dmic1-data-pin
-  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
-  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
-  0: No JD is used
-  1: using JD3 as JD source
-  2: JD source for Intel HDA header
-
-- realtek,ldo1-en-gpios : The GPIO that controls the CODEC's LDO1_EN pin.
-- realtek,reset-gpios : The GPIO that controls the CODEC's RESET pin.
-
-- sound-name-prefix: Please refer to name-prefix.yaml
-
-- ports: A Codec may have a single or multiple I2S interfaces. These
-  interfaces on Codec side can be described under 'ports' or 'port'.
-  When the SoC or host device is connected to multiple interfaces of
-  the Codec, the connectivity can be described using 'ports' property.
-  If a single interface is used, then 'port' can be used. The usage
-  depends on the platform or board design.
-  Please refer to Documentation/devicetree/bindings/graph.txt
-
-Pins on the device (for linking into audio routes) for RT5659/RT5658:
-
-  * DMIC L1
-  * DMIC R1
-  * DMIC L2
-  * DMIC R2
-  * IN1P
-  * IN1N
-  * IN2P
-  * IN2N
-  * IN3P
-  * IN3N
-  * IN4P
-  * IN4N
-  * HPOL
-  * HPOR
-  * SPOL
-  * SPOR
-  * LOUTL
-  * LOUTR
-  * MONOOUT
-  * PDML
-  * PDMR
-  * SPDIF
-
-Example:
-
-rt5659 {
-	compatible = "realtek,rt5659";
-	reg = <0x1b>;
-	interrupt-parent = <&gpio>;
-	interrupts = <TEGRA_GPIO(W, 3) IRQ_TYPE_LEVEL_HIGH>;
-	realtek,ldo1-en-gpios =
-		<&gpio TEGRA_GPIO(V, 3) GPIO_ACTIVE_HIGH>;
-};
-- 
2.7.4

Re: [RFC PATCH v2 1/6] ASoC: dt-bindings: Convert rt5659 bindings to YAML schema

[Krzysztof Kozlowski]

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

Re: [RFC PATCH v2 1/6] ASoC: dt-bindings: Convert rt5659 bindings to YAML schema

[Rob Herring]

On Mon, 28 Mar 2022 11:44:05 +0530, 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
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/patch/1610026


audio-codec@1a: 'port' does not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dt.yaml

Re: [RFC PATCH v2 1/6] ASoC: dt-bindings: Convert rt5659 bindings to YAML schema

[Sameer Pujar]

On 28-03-2022 18:21, Rob Herring wrote:
> On Mon, 28 Mar 2022 11:44:05 +0530, 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
>>
> Running 'make dtbs_check' with the schema in this patch gives the
> following warnings. Consider if they are expected or the schema is
> incorrect. These may not be new warnings.
>
> Note that it is not yet a requirement to have 0 warnings for dtbs_check.
> This will change in the future.
>
> Full log is available here: https://patchwork.ozlabs.org/patch/1610026
>
>
> audio-codec@1a: 'port' does not match any of the regexes: 'pinctrl-[0-9]+'
>          arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dt.yaml

The port/ports binding is not added in 'patch v2 1/6'. I will fix this 
in next revision by squashing 'patch v2 1/6' and 'patch v2 2/6'. Thanks.

[RFC PATCH v2 2/6] ASoC: dt-bindings: Add audio-graph-port bindings to rt5659

[Sameer Pujar]

To use rt5658 or rt5659 audio CODEC with generic audio-graph-card machine
driver, the CODEC requires "port" bindings. Thus add "audio-graph-port"
reference to the rt5659 binding.

Signed-off-by: Sameer Pujar <spujar@nvidia.com>
Cc: Oder Chiou <oder_chiou@realtek.com>
---
 Documentation/devicetree/bindings/sound/realtek,rt5659.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml
index 3bd9b6f..b0485b8 100644
--- a/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml
+++ b/Documentation/devicetree/bindings/sound/realtek,rt5659.yaml
@@ -84,6 +84,10 @@ properties:
 
   sound-name-prefix: true
 
+  port:
+    $ref: audio-graph-port.yaml#
+    unevaluatedProperties: false
+
 additionalProperties: false
 
 required:
-- 
2.7.4

Re: [RFC PATCH v2 2/6] ASoC: dt-bindings: Add audio-graph-port bindings to rt5659

[Krzysztof Kozlowski]

On 28/03/2022 08:14, Sameer Pujar wrote:
> To use rt5658 or rt5659 audio CODEC with generic audio-graph-card machine
> driver, the CODEC requires "port" bindings. Thus add "audio-graph-port"
> reference to the rt5659 binding.
> 
> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> Cc: Oder Chiou <oder_chiou@realtek.com>
> ---
>  Documentation/devicetree/bindings/sound/realtek,rt5659.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 

This should be squashed with your previous patch, otherwise that one is
not a complete conversion.

Best regards,
Krzysztof

Re: [RFC PATCH v2 2/6] ASoC: dt-bindings: Add audio-graph-port bindings to rt5659

[Sameer Pujar]

On 28-03-2022 12:33, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
>
>
> On 28/03/2022 08:14, Sameer Pujar wrote:
>> To use rt5658 or rt5659 audio CODEC with generic audio-graph-card machine
>> driver, the CODEC requires "port" bindings. Thus add "audio-graph-port"
>> reference to the rt5659 binding.
>>
>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>> Cc: Oder Chiou <oder_chiou@realtek.com>
>> ---
>>   Documentation/devicetree/bindings/sound/realtek,rt5659.yaml | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
> This should be squashed with your previous patch, otherwise that one is
> not a complete conversion.
>
OK. I will squash this. Thanks.

[RFC PATCH v2 3/6] ASoC: dt-bindings: Extend clock bindings of rt5659

[Sameer Pujar]

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
+
+  "#clock-cells":
+    const: 1
+
+  clock-output-names:
+    description: PLL output clock
+    $ref: /schemas/types.yaml#/definitions/string
+    const: rt5659_pll_out
 
   realtek,in1-differential:
     description: MIC1 input is differntial and not single-ended.
@@ -97,6 +113,7 @@ required:
 
 examples:
   - |
+    /* SYSCLK derived from MCLK */
     #include<dt-bindings/gpio/tegra194-gpio.h>
     #include<dt-bindings/clock/tegra194-clock.h>
 
@@ -114,3 +131,31 @@ examples:
             realtek,jd-src = <2>;
         };
     };
+
+  - |
+    /*
+     * SYSCLK is derived from CODEC internal PLL and PLL uses I2S1 BCLK
+     * as the clock reference.
+     */
+    #include<dt-bindings/gpio/tegra194-gpio.h>
+    #include<dt-bindings/clock/tegra194-clock.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        rt5658: audio-codec@1a {
+            compatible = "realtek,rt5658";
+            reg = <0x1a>;
+            interrupt-parent = <&gpio>;
+            interrupts = <TEGRA194_MAIN_GPIO(S, 5) GPIO_ACTIVE_HIGH>;
+            clocks = <&bpmp TEGRA194_CLK_AUD_MCLK>,
+                     <&bpmp TEGRA194_CLK_I2S1>,
+                     <&bpmp TEGRA194_CLK_I2S1>,
+                     <&rt5658 0>;
+            clock-names = "mclk", "bclk1", "pll_ref", "sysclk";
+            #clock-cells = <1>;
+            clock-output-names = "rt5659_pll_out";
+            realtek,jd-src = <2>;
+        };
+    };
-- 
2.7.4

Re: [RFC PATCH v2 3/6] ASoC: dt-bindings: Extend clock bindings of rt5659

[Krzysztof Kozlowski]

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.

Best regards,
Krzysztof

Re: [RFC PATCH v2 3/6] ASoC: dt-bindings: Extend clock bindings of rt5659

[Sameer Pujar]

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?).

Re: [RFC PATCH v2 3/6] ASoC: dt-bindings: Extend clock bindings of rt5659

[Krzysztof Kozlowski]

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. How to achieve this depends on number of your combinations. IOW,
how many clocks are physically optional. 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).


Best regards,
Krzysztof

Re: [RFC PATCH v2 3/6] ASoC: dt-bindings: Extend clock bindings of rt5659

[Sameer Pujar]

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

Re: [RFC PATCH v2 3/6] ASoC: dt-bindings: Extend clock bindings of rt5659

[Krzysztof Kozlowski]

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 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?

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


Best regards,
Krzysztof

Re: [RFC PATCH v2 3/6] ASoC: dt-bindings: Extend clock bindings of rt5659

[Sameer Pujar]

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 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?
> Yes, this would work. Relaxing such constraints is possible.

Thanks. I will update this in next revision.

[RFC PATCH v2 4/6] ASoC: soc-pcm: tweak DPCM BE hw_param() call order

[Sameer Pujar]

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;
+
+	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) {
+				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.
+	 */
+	for (i = 0; i < count; i++) {
+
+		be = prov[i];
+
 		/* newly connected FE and BE */
 		err = dpcm_be_connect(fe, be, stream);
 		if (err < 0) {
-- 
2.7.4

Re: [RFC PATCH v2 4/6] ASoC: soc-pcm: tweak DPCM BE hw_param() call order

[Amadeusz Sławiński]

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) {

Re: [RFC PATCH v2 4/6] ASoC: soc-pcm: tweak DPCM BE hw_param() call order

[Sameer Pujar]

On 28-03-2022 20:41, Amadeusz Sławiński wrote:
> 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?
>
This patch makes provider calls to happen first.

>>
>> 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.

The dpcm_be_connect() call adds the new connection to a list using 
list_add() which would be a stack. When dpcm_be_connect() is deferred 
for provider BEs, these occupy top of the stack. When operating on this 
list during hw_params() stage, the provider call happen first. Is this 
part clear now? I can rephrase the comments/commit message for more clarity.

>
>
> Overall I don't exactly understand correct order of events after reading
> commit message and patch...
>
Consider there are two BEs (BE1 and BE2) and "BE1<==>BE2" can be an I2S 
interface for example. I am trying to get following sequence.

1. When BE1 is provider and BE2 is consumer, the call sequence expected 
is : hw_params(BE1) -> hw_params(BE2).

2. When BE2 is provider and BE1 is consumer, the call sequence expected 
is : hw_params(BE2) -> hw_params(BE1).

Idea is to make use of standard clock functions for rate info. Provider 
can use clk_set_rate() and consumer can clk_get_rate().

>> 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;
> }
>
Thanks for the suggestion. I will check if loop copy can be avoided.


>> +
>>   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.

Sorry about this. I will fix it.

Re: [RFC PATCH v2 4/6] ASoC: soc-pcm: tweak DPCM BE hw_param() call order

[Ranjani Sridharan]

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

Re: [RFC PATCH v2 4/6] ASoC: soc-pcm: tweak DPCM BE hw_param() call order

[Sameer Pujar]

On 28-03-2022 20:59, Ranjani Sridharan wrote:
> 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?

By default the link has "SND_SOC_DAIFMT_CBC_CFC". When no format 
(I2S/RIGHT_J etc.,) is specified, the links are mostly internal and the 
normal order can be followed.

>> +
>> +     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?

There is no specific reason as why it cannot be more than 10. I mostly 
thought it would be a fair assumption to have these many clock providers 
for audio paths. I will check if such limitation can be avoided. I 
cannot rely on "rtd->num_cpus", since in my case there are two different 
rtds one acting as provider and other as consumer.

[RFC PATCH v2 5/6] ASoC: rt5659: Expose internal clock relationships

[Sameer Pujar]

The RT5658 or RT5659 codecs have multiple options to derive Sysclk:

  * Sysclk sourced from MCLK clock supplied by SoC
  * Sysclk sourced from codec internal PLL. The PLL again can take
    reference from I2S BCLKs and MCLK.
  * Sysclk sourced from RCCLK.

The clock relationship for codec is as following:

             |\
             | \                                        |\
  BCLK1 ---->|  \                        RCCLK          | \
             |   \                         |----------->|  \
  BCLK2 ---->| M  \       ____________                  |   \
             | U  |      |            |  PLL output     | M  \
  BCLK3 ---->| X  |----->| Codec PLL  |---------------->| U  |
             |    |      |____________|                 | X  |----> Sysclk
  BCLK4 ---->|   /                               |----->|    |
             |  /                                |      |   /
  MCLK  ---->| /                                 |      |  /
         |   |/                                  |      | /
         |                               MCLK    |      |/
         |_______________________________________|

Presently 'snd_soc_component_driver' and 'snd_soc_dai_driver' expose
callbacks, set_sysclk() for Sysclk and set_pll() for PLL configurations,
which are implemented on codec driver side. The generic machine drivers
(simple-card or audio-graph-card) depend on default values for Sysclk
source or PLL reference. Specific clock relationships are not supported.

The simpler solution would be to expose new DT binding properties to
convey the PLL and Sysclk source. This attempt was made before with [0],
but was not encouraged because it tries to do the same thing what
standard clock bindings already provide

This patch uses standard clock bindings to establish the codec clock
relationships. Specific configurations can be applied by DT bindings
from codec device node. The codec driver registers PLL and MUX clocks
to provide this flexibility.

[0] https://patchwork.kernel.org/project/alsa-devel/list/?series=438531&archive=both&state=*

Signed-off-by: Sameer Pujar <spujar@nvidia.com>
Cc: Oder Chiou <oder_chiou@realtek.com>
---
 Note: If such model is OK, other codec drivers will require similar
 handling. Objective is to drive clock relationships from DT using
 standard clock bindings. With this machine driver need not know
 the details for configuring codec PLL or other clocks and thus can
 be more generic.

 sound/soc/codecs/rt5659.c | 248 ++++++++++++++++++++++++++++++++++++++++++++--
 sound/soc/codecs/rt5659.h |   9 ++
 2 files changed, 249 insertions(+), 8 deletions(-)

diff --git a/sound/soc/codecs/rt5659.c b/sound/soc/codecs/rt5659.c
index e1503c2..3bf9680 100644
--- a/sound/soc/codecs/rt5659.c
+++ b/sound/soc/codecs/rt5659.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/init.h>
@@ -18,6 +19,8 @@
 #include <linux/acpi.h>
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -3527,6 +3530,9 @@ static int rt5659_set_component_pll(struct snd_soc_component *component, int pll
 	rt5659->pll_out = freq_out;
 	rt5659->pll_src = source;
 
+	dev_dbg(component->dev, "pll_in = %u Hz, pll_out = %u Hz, pll_src = %d\n",
+		freq_in, freq_out, source);
+
 	return 0;
 }
 
@@ -3843,6 +3849,237 @@ static int rt5659_parse_dt(struct rt5659_priv *rt5659, struct device *dev)
 	return 0;
 }
 
+static unsigned long rt5659_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	struct rt5659_priv *rt5659 =
+		container_of(hw, struct rt5659_priv, clk_pll_out);
+
+	return rt5659->pll_out;
+}
+
+static long rt5659_pll_round_rate(struct clk_hw *hw, unsigned long rate,
+			   unsigned long *parent_rate)
+{
+	return rate;
+}
+
+static int rt5659_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+			   unsigned long parent_rate)
+{
+	struct rt5659_priv *rt5659 =
+		container_of(hw, struct rt5659_priv, clk_pll_out);
+
+	rt5659->pll_out = rate;
+
+	return 0;
+}
+
+static const struct clk_ops rt5659_pll_out_ops = {
+	.recalc_rate = &rt5659_pll_recalc_rate,
+	.round_rate = &rt5659_pll_round_rate,
+	.set_rate = &rt5659_pll_set_rate,
+};
+
+static int rt5659_setup_clk(struct snd_soc_dai *dai,
+			    struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_component *component = dai->component;
+	struct rt5659_priv *rt5659 = snd_soc_component_get_drvdata(component);
+	int ret, sysclk_src;
+
+	/*
+	 * Update the clock rate if Codec is driving it. The consumers
+	 * can use clk_get_rate() function to get the rate.
+	 */
+	if (rt5659->master[dai->id] && rt5659->clk_bclk[dai->id]) {
+		unsigned int bclk_rate = params_rate(params) *
+					 params_width(params) *
+					 params_channels(params);
+
+		clk_set_rate(rt5659->clk_bclk[dai->id], bclk_rate);
+	}
+
+	if (rt5659->clk_sysclk_src) {
+		sysclk_src = clk_hw_get_parent_index(rt5659->clk_sysclk_src);
+
+		ret = rt5659_set_component_sysclk(component, sysclk_src, 0,
+						  rt5659->sysclk, 0);
+		if (ret)
+			return ret;
+	}
+
+	if (rt5659->clk_pll_src && (sysclk_src == RT5659_SCLK_S_PLL1)) {
+		unsigned int pll_src =
+			clk_hw_get_parent_index(rt5659->clk_pll_src);
+		unsigned int freq_in = clk_get_rate(rt5659->clk_pll_src->clk);
+
+		ret = rt5659_set_component_pll(component, 0, pll_src,
+					       freq_in, rt5659->sysclk);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int rt5659_register_clks(struct device *dev, struct rt5659_priv *rt5659)
+{
+	const struct clk_hw *sysclk_clk_hw[RT5659_NUM_SCLK_SRC_CLKS] = { NULL };
+	const char *pnames_sysclk[RT5659_NUM_SCLK_SRC_CLKS] = { NULL };
+	const char *pnames_pll[RT5659_NUM_PLL1_SRC_CLKS] = { NULL };
+	struct clk_init_data init = { };
+	static void __iomem *clk_base;
+	const char *clk_name;
+	int ret, i, count_pll_src = 0, count_sysclk_src = 0;
+
+	/* Check if MCLK provided */
+	rt5659->mclk = devm_clk_get(dev, "mclk");
+	if (IS_ERR(rt5659->mclk)) {
+		if (PTR_ERR(rt5659->mclk) != -ENOENT)
+			return PTR_ERR(rt5659->mclk);
+		/* Otherwise mark the mclk pointer to NULL */
+		rt5659->mclk = NULL;
+	}
+
+	if (!of_find_property(dev->of_node, "#clock-cells", NULL))
+		return 0;
+
+	/* Get PLL source */
+	rt5659->pll_ref = devm_clk_get(dev, "pll_ref");
+	if (IS_ERR(rt5659->pll_ref)) {
+		if (PTR_ERR(rt5659->pll_ref) != -ENOENT)
+			return PTR_ERR(rt5659->pll_ref);
+
+		rt5659->pll_ref = NULL;
+	}
+
+	/* Possible parents for PLL */
+	if (rt5659->mclk) {
+		pnames_pll[count_pll_src] = __clk_get_name(rt5659->mclk);
+		count_pll_src++;
+	}
+
+	for (i = 0; i < RT5659_AIFS; i++) {
+		char name[50];
+
+		memset(name, '\0', sizeof(name));
+		snprintf(name, sizeof(name), "%s%d", "bclk", i + 1);
+
+		rt5659->clk_bclk[i] = devm_clk_get(dev, name);
+		if (IS_ERR(rt5659->clk_bclk[i])) {
+			if (PTR_ERR(rt5659->clk_bclk[i]) != -ENOENT)
+				return PTR_ERR(rt5659->clk_bclk[i]);
+
+			rt5659->clk_bclk[i] = NULL;
+			continue;
+		}
+
+		pnames_pll[count_pll_src] = __clk_get_name(rt5659->clk_bclk[i]);
+		count_pll_src++;
+	}
+
+	clk_base = devm_kzalloc(dev, sizeof(char) * 4, GFP_KERNEL);
+
+	/* Register MUX for PLL source */
+	rt5659->clk_pll_src = clk_hw_register_mux(dev, "rt5659_pll_ref",
+						  pnames_pll, count_pll_src,
+						  CLK_SET_RATE_PARENT,
+						  clk_base, 0, 1, 0, NULL);
+
+	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get,
+				     rt5659->clk_pll_src);
+	if (ret) {
+		dev_err(dev, "failed to register clk hw\n");
+		return ret;
+	}
+
+	if (rt5659->pll_ref) {
+		ret = clk_set_parent(rt5659->clk_pll_src->clk, rt5659->pll_ref);
+		if (ret) {
+			dev_err(dev, "failaed to set parent for clk %s\n",
+				__clk_get_name(rt5659->clk_pll_src->clk));
+			return ret;
+		}
+	}
+
+	/* Register PLL out clock */
+	if (of_property_read_string(dev->of_node, "clock-output-names",
+	    (const char **) &clk_name))
+		clk_name = "rt5659_pll_out";
+
+	init.name = clk_name;
+	init.ops = &rt5659_pll_out_ops;
+	init.flags = CLK_GET_RATE_NOCACHE | CLK_SET_RATE_GATE;
+	init.parent_hws = (const struct clk_hw **) &rt5659->clk_pll_src;
+	init.num_parents = 1;
+
+	rt5659->clk_pll_out.init = &init;
+
+	ret = devm_clk_hw_register(dev, &rt5659->clk_pll_out);
+	if (ret) {
+		dev_err(dev, "failed to register PLL clock HW\n");
+		return ret;
+	}
+
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					  &rt5659->clk_pll_out);
+	if (ret) {
+		dev_err(dev, "failed to add PLL clock provider\n");
+		return ret;
+	}
+
+	/* Get sysclk source */
+	rt5659->sysclk_ref = devm_clk_get(dev, "sysclk");
+	if (IS_ERR(rt5659->sysclk_ref)) {
+		if (PTR_ERR(rt5659->sysclk_ref) != -ENOENT)
+			return PTR_ERR(rt5659->sysclk_ref);
+
+		rt5659->sysclk_ref = NULL;
+	}
+
+	/* Possible parents for Sysclk */
+	if (rt5659->mclk) {
+		/* For sysclk */
+		pnames_sysclk[count_sysclk_src] = __clk_get_name(rt5659->mclk);
+		sysclk_clk_hw[count_sysclk_src] = __clk_get_hw(rt5659->mclk);
+		count_sysclk_src++;
+	}
+
+	if (rt5659->clk_pll_out.clk) {
+		pnames_sysclk[count_sysclk_src] = __clk_get_name(rt5659->clk_pll_out.clk);
+		sysclk_clk_hw[count_sysclk_src] = __clk_get_hw(rt5659->clk_pll_out.clk);
+		count_sysclk_src++;
+	}
+
+	/* Register MUX for sysclk source */
+	rt5659->clk_sysclk_src = __clk_hw_register_mux(dev, dev->of_node,
+						       "rt5659_sysclk",
+						       count_sysclk_src,
+						       pnames_sysclk,
+						       sysclk_clk_hw, NULL,
+						       CLK_SET_RATE_PARENT,
+						       clk_base, 0, 1, 0,
+						       NULL, NULL);
+
+	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get,
+				     rt5659->clk_sysclk_src);
+	if (ret) {
+		dev_err(dev, "failed to register clk hw\n");
+		return ret;
+	}
+
+	if (rt5659->sysclk_ref) {
+		ret = clk_set_parent(rt5659->clk_sysclk_src->clk, rt5659->sysclk_ref);
+		if (ret) {
+			dev_err(dev, "failed to set parent for clk %s\n",
+				__clk_get_name(rt5659->clk_sysclk_src->clk));
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static void rt5659_calibrate(struct rt5659_priv *rt5659)
 {
 	int value, count;
@@ -4142,14 +4379,9 @@ static int rt5659_i2c_probe(struct i2c_client *i2c,
 
 	regmap_write(rt5659->regmap, RT5659_RESET, 0);
 
-	/* Check if MCLK provided */
-	rt5659->mclk = devm_clk_get(&i2c->dev, "mclk");
-	if (IS_ERR(rt5659->mclk)) {
-		if (PTR_ERR(rt5659->mclk) != -ENOENT)
-			return PTR_ERR(rt5659->mclk);
-		/* Otherwise mark the mclk pointer to NULL */
-		rt5659->mclk = NULL;
-	}
+	ret = rt5659_register_clks(&i2c->dev, rt5659);
+	if (ret)
+		return ret;
 
 	rt5659_calibrate(rt5659);
 
diff --git a/sound/soc/codecs/rt5659.h b/sound/soc/codecs/rt5659.h
index b49fd8b..d46d39f 100644
--- a/sound/soc/codecs/rt5659.h
+++ b/sound/soc/codecs/rt5659.h
@@ -1763,6 +1763,7 @@ enum {
 	RT5659_SCLK_S_MCLK,
 	RT5659_SCLK_S_PLL1,
 	RT5659_SCLK_S_RCCLK,
+	RT5659_NUM_SCLK_SRC_CLKS,
 };
 
 /* PLL1 Source */
@@ -1772,6 +1773,7 @@ enum {
 	RT5659_PLL1_S_BCLK2,
 	RT5659_PLL1_S_BCLK3,
 	RT5659_PLL1_S_BCLK4,
+	RT5659_NUM_PLL1_SRC_CLKS,
 };
 
 enum {
@@ -1797,6 +1799,13 @@ struct rt5659_priv {
 	struct gpio_desc *gpiod_reset;
 	struct snd_soc_jack *hs_jack;
 	struct delayed_work jack_detect_work;
+
+	struct clk_hw *clk_sysclk_src;
+	struct clk_hw *clk_pll_src;
+	struct clk_hw clk_pll_out;
+	struct clk *clk_bclk[RT5659_AIFS];
+	struct clk *sysclk_ref;
+	struct clk *pll_ref;
 	struct clk *mclk;
 
 	int sysclk;
-- 
2.7.4

[RFC PATCH v2 6/6] ASoC: tegra: Get clock rate in consumer mode

[Sameer Pujar]

When Tegra I2S is consumer the clock is driven by the external codec.
In such cases, ideally the bit clock (BCLK) rate needs to be updated by
provider. Consumer can use standard clock function to get the rate.

On Tegra HW it is possible to use I2S BCLK clock as reference to the
I/O (other I2S or DMIC or DSPK) interfaces. This input clock is called
as SYNC input clock and it can act as a parent clock to any of the
remaining I/O interfaces. Thus it is important to set the clock rate
in Tegra I2S consumer mode as well.

With this patch SYNC input clock rate is updated and any I/O interface
relying on this can derive required rate.

Signed-off-by: Sameer Pujar <spujar@nvidia.com>
---
 Following are the DT binding cases I tried on Jetson AGX Xavier platform.

   1. Sysclk derived from MCLK : This is currently being used. No DT
      binding change would be necessary.

      Clock tree dump snippet in this case with proposed series:

          ...

          pll_a
            |
            |-- plla_out0
                    |
                    |-- ahub
                    |
                    |-- aud_mclk
                    |      |
                    |      |-- rt5659_sysclk
                    |
                    |-- i2s1

          ...
      

   2. Sysclk is derived from codec internal PLL and this PLL uses I2S
      bit clock (BCLK) as reference.

      rt5658: audio-codec@1a {
         ...

         clocks = <&bpmp TEGRA194_CLK_AUD_MCLK>,
		  <&bpmp TEGRA194_CLK_I2S1>,
		  <&bpmp TEGRA194_CLK_I2S1>,
                  <&rt5658 0>;
         clock-names = "mclk", "bclk1", "pll_ref", "sysclk";

         #clock-cells = <1>;
         clock-output-names = "rt5659_pll_out";

         ...
      };

      Clock tree dump snippet in this case with proposed series:

          ...

          pll_a
            |
            |-- plla_out0
                    |
                    |-- ahub
                    |
                    |-- aud_mclk
                    |
                    |-- i2s1
                         |
                         |-- rt5659_pll_ref
                                   |
                                   |-- rt5659_pll_out
                                             |
                                             |-- rt5659_sysclk

          ...


 sound/soc/tegra/tegra210_i2s.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/sound/soc/tegra/tegra210_i2s.c b/sound/soc/tegra/tegra210_i2s.c
index 9552bbb..53e5307 100644
--- a/sound/soc/tegra/tegra210_i2s.c
+++ b/sound/soc/tegra/tegra210_i2s.c
@@ -53,17 +53,24 @@ static int tegra210_i2s_set_clock_rate(struct device *dev,
 
 	regmap_read(i2s->regmap, TEGRA210_I2S_CTRL, &val);
 
-	/* No need to set rates if I2S is being operated in slave */
-	if (!(val & I2S_CTRL_MASTER_EN))
-		return 0;
-
-	err = clk_set_rate(i2s->clk_i2s, clock_rate);
-	if (err) {
-		dev_err(dev, "can't set I2S bit clock rate %u, err: %d\n",
-			clock_rate, err);
-		return err;
+	/*
+	 * If I2S is consumer, then the clock rate is expected to be
+	 * set by the respective provider and thus just read the rate
+	 * in such case. If I2S is provider, then set the clock rate.
+	 */
+	if (!(val & I2S_CTRL_MASTER_EN)) {
+		clock_rate = clk_get_rate(i2s->clk_i2s);
+	} else {
+		err = clk_set_rate(i2s->clk_i2s, clock_rate);
+		if (err) {
+			dev_err(dev, "can't set I2S bit clock rate %u, err: %d\n",
+				clock_rate, err);
+			return err;
+		}
 	}
 
+	dev_dbg(dev, "bit clock (BCLK) rate is %u\n", clock_rate);
+
 	if (!IS_ERR(i2s->clk_sync_input)) {
 		/*
 		 * Other I/O modules in AHUB can use i2s bclk as reference
-- 
2.7.4