[PATCH 0/4] Rockchip I2S/TDM controller

[PATCH 0/4] Rockchip I2S/TDM controller

[Nicolas Frattaroli]

Hello,

I come bearing four patches for your consideration.

The first of these four patches adds a driver for the Rockchip
I2S/TDM controller, used in interfacing between the AHB bus and the
I2S bus on some Rockchip SoCs. This allows for audio playback with
a matching codec.

The controller has three different modes: I2S, I2S/TDM and PCM.
It is distinct from the earlier Rockchip I2S controller, and
therefore not just an extension of that driver.

The driver is based on the downstream version, though various
changes have been made to hopefully make it more palatable to
upstream. Some needless code duplication has been refactored, and
the probe function will no longer let wrong device tree values
write nonsense to hardware registers. Properties have been renamed
and had their semantics changed. I won't bore you with the details
of what downstream did, since that's not what I'm submitting, but
the changes are significant enough that I've added myself to the
list of authors.

The second patch adds the YAML device tree bindings for this, which
have been written from scratch by yours truly. Since I didn't like
having random integers mean things, I defined them as constants in
a header file for the bindings.

The third patch adds the i2s1 controller to the rk356x device tree.
I didn't add any of the other i2s controllers on that SoC for now as
I have no way of testing them; in particular, i2s0 is tied to HDMI,
so needs a functioning VOP2 driver to even have a chance of working.

The fourth patch makes use of the i2s1 controller to enable analog
audio output on the Quartz64 Model A through its RK817 codec. I've
tested this to work properly at both 44.1 kHz and 96 kHz, so both
mclk_root0 and mclk_root1 are definitely functioning.

This is my first kernel contribution, so I most likely did
something horribly wrong. That's why I'm more than happy to receive
any criticisms and concerns over how the driver is implemented,
because I've run out of ideas on how to make it clearly better
myself.

I'd also like to extend my thanks to Peter Geis, who has been
acting as somewhat of a mentor and gave me occasional feedback
and ideas during the writing of this patch series.

Regards,
Nicolas Frattaroli

Nicolas Frattaroli (4):
  ASoC: rockchip: add support for i2s-tdm controller
  dt-bindings: sound: add rockchip i2s-tdm binding
  arm64: dts: rockchip: add i2s1 on rk356x
  arm64: dts: rockchip: add analog audio on Quartz64

 .../bindings/sound/rockchip,i2s-tdm.yaml      |  221 ++
 .../boot/dts/rockchip/rk3566-quartz64-a.dts   |   36 +-
 arch/arm64/boot/dts/rockchip/rk356x.dtsi      |   26 +
 include/dt-bindings/sound/rockchip,i2s-tdm.h  |    9 +
 sound/soc/rockchip/Kconfig                    |   11 +
 sound/soc/rockchip/Makefile                   |    2 +
 sound/soc/rockchip/rockchip_i2s_tdm.c         | 1804 +++++++++++++++++
 sound/soc/rockchip/rockchip_i2s_tdm.h         |  400 ++++
 8 files changed, 2508 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml
 create mode 100644 include/dt-bindings/sound/rockchip,i2s-tdm.h
 create mode 100644 sound/soc/rockchip/rockchip_i2s_tdm.c
 create mode 100644 sound/soc/rockchip/rockchip_i2s_tdm.h

-- 
2.32.0

[PATCH 2/4] dt-bindings: sound: add rockchip i2s-tdm binding

[Nicolas Frattaroli]

This adds the YAML bindings for the Rockchip I2S/TDM audio driver.

Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
---
 .../bindings/sound/rockchip,i2s-tdm.yaml      | 221 ++++++++++++++++++
 include/dt-bindings/sound/rockchip,i2s-tdm.h  |   9 +
 2 files changed, 230 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml
 create mode 100644 include/dt-bindings/sound/rockchip,i2s-tdm.h

diff --git a/Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml b/Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml
new file mode 100644
index 000000000000..c3022620b47f
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml
@@ -0,0 +1,221 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/rockchip,i2s-tdm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip I2S/TDM Controller
+
+description:
+  The Rockchip I2S/TDM Controller is a Time Division Multiplexed
+  audio interface found in various Rockchip SoCs, allowing up
+  to 8 channels of audio over a serial interface.
+
+maintainers:
+  - Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
+
+properties:
+  compatible:
+    enum:
+      - rockchip,px30-i2s-tdm
+      - rockchip,rk1808-i2s-tdm
+      - rockchip,rk3308-i2s-tdm
+      - rockchip,rk3568-i2s-tdm
+      - rockchip,rv1126-i2s-tdm
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  dmas:
+    minItems: 1
+    maxItems: 2
+
+  dma-names:
+    oneOf:
+      - const: rx
+      - items:
+          - const: tx
+          - const: rx
+
+  clocks:
+    items:
+      - description: clock for TX
+      - description: clock for RX
+      - description: clock for I2S bus
+
+  clock-names:
+    items:
+      - const: mclk_tx
+      - const: mclk_rx
+      - const: hclk
+
+  rockchip,frame-width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 64
+    minimum: 32
+    maximum: 512
+    description:
+      Width of a frame, usually slot width multiplied by number of slots.
+      Must be even.
+
+  resets:
+    items:
+      - description: reset for TX
+      - description: reset for RX
+
+  reset-names:
+    items:
+      - const: tx-m
+      - const: rx-m
+
+  rockchip,cru:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      The phandle of the cru.
+      Required if both playback and capture are used, i.e. if rockchip,clk-trcm
+      is 0.
+
+  rockchip,grf:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      The phandle of the syscon node for the GRF register.
+
+  rockchip,mclk-calibrate:
+    description:
+      Enable mclk source calibration.
+    type: boolean
+
+  rockchip,trcm-sync:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Which lrck/bclk clocks each direction will sync to. You should use the
+      constants in <dt-bindings/sound/rockchip,i2s-tdm.h>
+    oneOf:
+      - const: 0
+        description:
+          RK_TRCM_TXRX. Use both the TX and the RX clock for TX and RX.
+      - const: 1
+        description:
+          RK_TRCM_TX. Use only the TX clock for TX and RX.
+      - const: 2
+        description:
+          RK_TRCM_RX. Use only the RX clock for TX and RX.
+
+  "#sound-dai-cells":
+    const: 0
+
+  rockchip,no-dmaengine:
+    description:
+      If present, driver will not register a pcm dmaengine, only the dai.
+      If the dai is part of multi-dais, the property should be present.
+    type: boolean
+
+  rockchip,playback-only:
+    description: Specify that the controller only has playback capability.
+    type: boolean
+
+  rockchip,capture-only:
+    description: Specify that the controller only has capture capability.
+    type: boolean
+
+  rockchip,i2s-rx-route:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Defines the mapping of I2S RX sdis to I2S data bus lines.
+      By default, they are mapped one-to-one.
+    items:
+      - description: which sdi to connect to data line 0
+      - description: which sdi to connect to data line 1
+      - description: which sdi to connect to data line 2
+      - description: which sdi to connect to data line 3
+
+  rockchip,i2s-tx-route:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Defines the mapping of I2S TX sdos to I2S data bus lines.
+      By default, they are mapped one-to-one.
+    items:
+      - description: which sdo to connect to data line 0
+      - description: which sdo to connect to data line 1
+      - description: which sdo to connect to data line 2
+      - description: which sdo to connect to data line 3
+
+  rockchip,tdm-fsync-half-frame:
+    description: Whether to use half frame fsync.
+    type: boolean
+
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - dmas
+  - dma-names
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - rockchip,grf
+  - "#sound-dai-cells"
+  - rockchip,trcm-sync
+
+allOf:
+  - if:
+      properties:
+        rockchip,clk-trcm:
+          contains:
+            enum: [0]
+    then:
+      required:
+        - rockchip,cru
+
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/rk3568-cru.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/pinctrl/rockchip.h>
+    #include <dt-bindings/sound/rockchip,i2s-tdm.h>
+
+
+    foo {
+        #address-cells = <2>;
+        #size-cells = <2>;
+        i2s@fe410000 {
+            compatible = "rockchip,rk3568-i2s-tdm";
+            reg = <0x0 0xfe410000 0x0 0x1000>;
+            interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&cru MCLK_I2S1_8CH_TX>, <&cru MCLK_I2S1_8CH_RX>,
+                     <&cru HCLK_I2S1_8CH>;
+            clock-names = "mclk_tx", "mclk_rx", "hclk";
+            dmas = <&dmac1 2>, <&dmac1 3>;
+            dma-names = "tx", "rx";
+            resets = <&cru SRST_M_I2S1_8CH_TX>, <&cru SRST_M_I2S1_8CH_RX>;
+            reset-names = "tx-m", "rx-m";
+            rockchip,trcm-sync = <RK_TRCM_TX>;
+            rockchip,cru = <&cru>;
+            rockchip,grf = <&grf>;
+            #sound-dai-cells = <0>;
+            pinctrl-names = "default";
+            pinctrl-0 =
+                <&i2s1m0_sclktx
+                &i2s1m0_sclkrx
+                &i2s1m0_lrcktx
+                &i2s1m0_lrckrx
+                &i2s1m0_sdi0
+                &i2s1m0_sdi1
+                &i2s1m0_sdi2
+                &i2s1m0_sdi3
+                &i2s1m0_sdo0
+                &i2s1m0_sdo1
+                &i2s1m0_sdo2
+                &i2s1m0_sdo3>;
+            status = "disabled";
+        };
+    };
diff --git a/include/dt-bindings/sound/rockchip,i2s-tdm.h b/include/dt-bindings/sound/rockchip,i2s-tdm.h
new file mode 100644
index 000000000000..32494d64cf33
--- /dev/null
+++ b/include/dt-bindings/sound/rockchip,i2s-tdm.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _DT_BINDINGS_ROCKCHIP_I2S_TDM_H
+#define _DT_BINDINGS_ROCKCHIP_I2S_TDM_H
+
+#define RK_TRCM_TXRX 0
+#define RK_TRCM_TX 1
+#define RK_TRCM_RX 2
+
+#endif /* _DT_BINDINGS_ROCKCHIP_I2S_TDM_H */
-- 
2.32.0

[PATCH 3/4] arm64: dts: rockchip: add i2s1 on rk356x

[Nicolas Frattaroli]

This adds the necessary device tree node on rk3566 and rk3568
to enable the I2S1 TDM audio controller.

I2S0 has not been added, as it is connected to HDMI and there is
no way to test that it's working without a functioning video
clock (read: VOP2 driver).

Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk356x.dtsi | 26 ++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index bef747fb1fe2..2cdb13f29bed 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -554,6 +554,32 @@ sdhci: mmc@fe310000 {
 		status = "disabled";
 	};
 
+	i2s1_8ch: i2s@fe410000 {
+		compatible = "rockchip,rk3568-i2s-tdm";
+		reg = <0x0 0xfe410000 0x0 0x1000>;
+		interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>;
+		assigned-clocks = <&cru CLK_I2S1_8CH_TX_SRC>, <&cru CLK_I2S1_8CH_RX_SRC>;
+		assigned-clock-rates = <1188000000>, <1188000000>;
+		clocks = <&cru MCLK_I2S1_8CH_TX>, <&cru MCLK_I2S1_8CH_RX>,
+		<&cru HCLK_I2S1_8CH>;
+		clock-names = "mclk_tx", "mclk_rx", "hclk";
+		dmas = <&dmac1 2>, <&dmac1 3>;
+		dma-names = "tx", "rx";
+		resets = <&cru SRST_M_I2S1_8CH_TX>, <&cru SRST_M_I2S1_8CH_RX>;
+		reset-names = "tx-m", "rx-m";
+		rockchip,cru = <&cru>;
+		rockchip,grf = <&grf>;
+		#sound-dai-cells = <0>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&i2s1m0_sclktx &i2s1m0_sclkrx
+			     &i2s1m0_lrcktx &i2s1m0_lrckrx
+			     &i2s1m0_sdi0   &i2s1m0_sdi1
+			     &i2s1m0_sdi2   &i2s1m0_sdi3
+			     &i2s1m0_sdo0   &i2s1m0_sdo1
+			     &i2s1m0_sdo2   &i2s1m0_sdo3>;
+		status = "disabled";
+	};
+
 	dmac0: dmac@fe530000 {
 		compatible = "arm,pl330", "arm,primecell";
 		reg = <0x0 0xfe530000 0x0 0x4000>;
-- 
2.32.0

[PATCH 4/4] arm64: dts: rockchip: add analog audio on Quartz64

[Nicolas Frattaroli]

On the Quartz64 Model A, the I2S1 TDM controller is connected
to the rk817 codec in I2S mode. Enabling it and adding the
necessary simple-sound-card and codec nodes allows for analog
audio output on the PINE64 Quartz64 Model A SBC.

Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
---
 .../boot/dts/rockchip/rk3566-quartz64-a.dts   | 36 ++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
index b239f314b38a..8da4d1f600c3 100644
--- a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
@@ -4,6 +4,7 @@
 
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/pinctrl/rockchip.h>
+#include <dt-bindings/sound/rockchip,i2s-tdm.h>
 #include "rk3566.dtsi"
 
 / {
@@ -50,6 +51,20 @@ led-diy {
 		};
 	};
 
+	rk817-sound {
+		compatible = "simple-audio-card";
+		simple-audio-card,format = "i2s";
+		simple-audio-card,name = "Analog RK817";
+		simple-audio-card,mclk-fs = <256>;
+
+		simple-audio-card,cpu {
+			sound-dai = <&i2s1_8ch>;
+		};
+		simple-audio-card,codec {
+			sound-dai = <&rk817>;
+		};
+	};
+
 	vcc12v_dcin: vcc12v_dcin {
 		compatible = "regulator-fixed";
 		regulator-name = "vcc12v_dcin";
@@ -174,8 +189,13 @@ rk817: pmic@20 {
 		interrupts = <RK_PA3 IRQ_TYPE_LEVEL_LOW>;
 		clock-output-names = "rk808-clkout1", "rk808-clkout2";
 
+		#sound-dai-cells = <0>;
+		clock-names = "mclk";
+		clocks = <&cru I2S1_MCLKOUT_TX>;
+		assigned-clocks = <&cru I2S1_MCLKOUT_TX>;
+		assigned-clock-parents = <&cru CLK_I2S1_8CH_TX>;
 		pinctrl-names = "default";
-		pinctrl-0 = <&pmic_int_l>;
+		pinctrl-0 = <&pmic_int_l>, <&i2s1m0_mclk>;
 		rockchip,system-power-controller;
 		wakeup-source;
 		#clock-cells = <1>;
@@ -364,9 +384,23 @@ regulator-state-mem {
 				};
 			};
 		};
+
+		rk817_codec: codec {
+		};
+
 	};
 };
 
+&i2s1_8ch {
+	status = "okay";
+	rockchip,trcm-sync = <RK_TRCM_TX>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2s1m0_sclktx
+		     &i2s1m0_lrcktx
+		     &i2s1m0_sdi0
+		     &i2s1m0_sdo0>;
+};
+
 &mdio1 {
 	rgmii_phy1: ethernet-phy@0 {
 		compatible = "ethernet-phy-ieee802.3-c22";
-- 
2.32.0

Re: [PATCH 2/4] dt-bindings: sound: add rockchip i2s-tdm binding

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5574 bytes --]

Hi Nicolas,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on rockchip/for-next]
[also build test ERROR on asoc/for-next sound/for-next v5.14-rc6 next-20210816]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nicolas-Frattaroli/Rockchip-I2S-TDM-controller/20210817-181921
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/3b075f992be5028cbbe3ab1fbdf95bb63bbdac0c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nicolas-Frattaroli/Rockchip-I2S-TDM-controller/20210817-181921
        git checkout 3b075f992be5028cbbe3ab1fbdf95bb63bbdac0c
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   sound/soc/rockchip/rockchip_i2s_tdm.c: In function 'rockchip_snd_xfer_reset_assert':
>> sound/soc/rockchip/rockchip_i2s_tdm.c:198:25: error: implicit declaration of function 'writeq'; did you mean 'writel'? [-Werror=implicit-function-declaration]
     198 |                         writeq(val, addr);
         |                         ^~~~~~
         |                         writel
   cc1: some warnings being treated as errors


vim +198 sound/soc/rockchip/rockchip_i2s_tdm.c

2a497b883a8aaf Nicolas Frattaroli 2021-08-17  167  
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  168  static void rockchip_snd_xfer_reset_assert(struct rk_i2s_tdm_dev *i2s_tdm,
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  169  					   int tx_bank, int tx_offset,
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  170  					   int rx_bank, int rx_offset)
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  171  {
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  172  	void __iomem *cru_reset, *addr;
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  173  	unsigned long flags;
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  174  	u64 val;
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  175  
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  176  	cru_reset = i2s_tdm->cru_base + i2s_tdm->soc_data->softrst_offset;
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  177  
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  178  	switch (abs(tx_bank - rx_bank)) {
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  179  	case 0:
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  180  		writel(BIT(tx_offset) | BIT(rx_offset) |
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  181  		       (BIT(tx_offset) << 16) | (BIT(rx_offset) << 16),
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  182  		       cru_reset + (tx_bank * 4));
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  183  		break;
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  184  	case 1:
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  185  		if (tx_bank < rx_bank) {
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  186  			val = BIT(rx_offset) | (BIT(rx_offset) << 16);
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  187  			val <<= 32;
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  188  			val |= BIT(tx_offset) | (BIT(tx_offset) << 16);
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  189  			addr = cru_reset + (tx_bank * 4);
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  190  		} else {
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  191  			val = BIT(tx_offset) | (BIT(tx_offset) << 16);
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  192  			val <<= 32;
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  193  			val |= BIT(rx_offset) | (BIT(rx_offset) << 16);
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  194  			addr = cru_reset + (rx_bank * 4);
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  195  		}
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  196  
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  197  		if (IS_ALIGNED((uintptr_t)addr, 8)) {
2a497b883a8aaf Nicolas Frattaroli 2021-08-17 @198  			writeq(val, addr);
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  199  			break;
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  200  		}
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  201  		fallthrough;
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  202  	default:
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  203  		local_irq_save(flags);
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  204  		writel(BIT(tx_offset) | (BIT(tx_offset) << 16),
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  205  		       cru_reset + (tx_bank * 4));
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  206  		writel(BIT(rx_offset) | (BIT(rx_offset) << 16),
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  207  		       cru_reset + (rx_bank * 4));
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  208  		local_irq_restore(flags);
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  209  		break;
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  210  	}
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  211  }
2a497b883a8aaf Nicolas Frattaroli 2021-08-17  212  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 60695 bytes --]

Re: [PATCH 2/4] dt-bindings: sound: add rockchip i2s-tdm binding

[Rob Herring]

On Tue, Aug 17, 2021 at 12:11:17PM +0200, Nicolas Frattaroli wrote:
> This adds the YAML bindings for the Rockchip I2S/TDM audio driver.
> 
> Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> ---
>  .../bindings/sound/rockchip,i2s-tdm.yaml      | 221 ++++++++++++++++++
>  include/dt-bindings/sound/rockchip,i2s-tdm.h  |   9 +
>  2 files changed, 230 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml
>  create mode 100644 include/dt-bindings/sound/rockchip,i2s-tdm.h
> 
> diff --git a/Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml b/Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml
> new file mode 100644
> index 000000000000..c3022620b47f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml
> @@ -0,0 +1,221 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/rockchip,i2s-tdm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip I2S/TDM Controller
> +
> +description:
> +  The Rockchip I2S/TDM Controller is a Time Division Multiplexed
> +  audio interface found in various Rockchip SoCs, allowing up
> +  to 8 channels of audio over a serial interface.
> +
> +maintainers:
> +  - Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - rockchip,px30-i2s-tdm
> +      - rockchip,rk1808-i2s-tdm
> +      - rockchip,rk3308-i2s-tdm
> +      - rockchip,rk3568-i2s-tdm
> +      - rockchip,rv1126-i2s-tdm
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  dmas:
> +    minItems: 1
> +    maxItems: 2
> +
> +  dma-names:
> +    oneOf:
> +      - const: rx
> +      - items:
> +          - const: tx
> +          - const: rx

If 'rx' is always present, then make it first:

minItems: 1
items:
  - const: rx
  - const: tx

> +
> +  clocks:
> +    items:
> +      - description: clock for TX
> +      - description: clock for RX
> +      - description: clock for I2S bus
> +
> +  clock-names:
> +    items:
> +      - const: mclk_tx
> +      - const: mclk_rx
> +      - const: hclk
> +
> +  rockchip,frame-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 64
> +    minimum: 32
> +    maximum: 512
> +    description:
> +      Width of a frame, usually slot width multiplied by number of slots.
> +      Must be even.
> +
> +  resets:
> +    items:
> +      - description: reset for TX
> +      - description: reset for RX
> +
> +  reset-names:
> +    items:
> +      - const: tx-m
> +      - const: rx-m
> +
> +  rockchip,cru:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      The phandle of the cru.
> +      Required if both playback and capture are used, i.e. if rockchip,clk-trcm
> +      is 0.
> +
> +  rockchip,grf:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      The phandle of the syscon node for the GRF register.
> +
> +  rockchip,mclk-calibrate:
> +    description:
> +      Enable mclk source calibration.
> +    type: boolean
> +
> +  rockchip,trcm-sync:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Which lrck/bclk clocks each direction will sync to. You should use the
> +      constants in <dt-bindings/sound/rockchip,i2s-tdm.h>
> +    oneOf:
> +      - const: 0
> +        description:
> +          RK_TRCM_TXRX. Use both the TX and the RX clock for TX and RX.
> +      - const: 1
> +        description:
> +          RK_TRCM_TX. Use only the TX clock for TX and RX.
> +      - const: 2
> +        description:
> +          RK_TRCM_RX. Use only the RX clock for TX and RX.
> +
> +  "#sound-dai-cells":
> +    const: 0
> +
> +  rockchip,no-dmaengine:
> +    description:
> +      If present, driver will not register a pcm dmaengine, only the dai.
> +      If the dai is part of multi-dais, the property should be present.
> +    type: boolean
> +
> +  rockchip,playback-only:
> +    description: Specify that the controller only has playback capability.
> +    type: boolean
> +
> +  rockchip,capture-only:
> +    description: Specify that the controller only has capture capability.
> +    type: boolean
> +
> +  rockchip,i2s-rx-route:
> +    $ref: /schemas/types.yaml#/definitions/uint32

A single uint32 or

> +    description:
> +      Defines the mapping of I2S RX sdis to I2S data bus lines.
> +      By default, they are mapped one-to-one.
> +    items:
> +      - description: which sdi to connect to data line 0
> +      - description: which sdi to connect to data line 1
> +      - description: which sdi to connect to data line 2
> +      - description: which sdi to connect to data line 3

An array of 4 uint32s?

> +
> +  rockchip,i2s-tx-route:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Defines the mapping of I2S TX sdos to I2S data bus lines.
> +      By default, they are mapped one-to-one.
> +    items:
> +      - description: which sdo to connect to data line 0
> +      - description: which sdo to connect to data line 1
> +      - description: which sdo to connect to data line 2
> +      - description: which sdo to connect to data line 3
> +
> +  rockchip,tdm-fsync-half-frame:
> +    description: Whether to use half frame fsync.
> +    type: boolean
> +
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - dmas
> +  - dma-names
> +  - clocks
> +  - clock-names
> +  - resets
> +  - reset-names
> +  - rockchip,grf
> +  - "#sound-dai-cells"
> +  - rockchip,trcm-sync
> +
> +allOf:
> +  - if:
> +      properties:
> +        rockchip,clk-trcm:

This property doesn't exist.

> +          contains:
> +            enum: [0]
> +    then:
> +      required:
> +        - rockchip,cru
> +
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/rk3568-cru.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/pinctrl/rockchip.h>
> +    #include <dt-bindings/sound/rockchip,i2s-tdm.h>
> +
> +
> +    foo {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +        i2s@fe410000 {
> +            compatible = "rockchip,rk3568-i2s-tdm";
> +            reg = <0x0 0xfe410000 0x0 0x1000>;
> +            interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>;
> +            clocks = <&cru MCLK_I2S1_8CH_TX>, <&cru MCLK_I2S1_8CH_RX>,
> +                     <&cru HCLK_I2S1_8CH>;
> +            clock-names = "mclk_tx", "mclk_rx", "hclk";
> +            dmas = <&dmac1 2>, <&dmac1 3>;
> +            dma-names = "tx", "rx";
> +            resets = <&cru SRST_M_I2S1_8CH_TX>, <&cru SRST_M_I2S1_8CH_RX>;
> +            reset-names = "tx-m", "rx-m";
> +            rockchip,trcm-sync = <RK_TRCM_TX>;
> +            rockchip,cru = <&cru>;
> +            rockchip,grf = <&grf>;
> +            #sound-dai-cells = <0>;
> +            pinctrl-names = "default";
> +            pinctrl-0 =
> +                <&i2s1m0_sclktx
> +                &i2s1m0_sclkrx
> +                &i2s1m0_lrcktx
> +                &i2s1m0_lrckrx
> +                &i2s1m0_sdi0
> +                &i2s1m0_sdi1
> +                &i2s1m0_sdi2
> +                &i2s1m0_sdi3
> +                &i2s1m0_sdo0
> +                &i2s1m0_sdo1
> +                &i2s1m0_sdo2
> +                &i2s1m0_sdo3>;
> +            status = "disabled";

Why is the example disabled?

> +        };
> +    };
> diff --git a/include/dt-bindings/sound/rockchip,i2s-tdm.h b/include/dt-bindings/sound/rockchip,i2s-tdm.h
> new file mode 100644
> index 000000000000..32494d64cf33
> --- /dev/null
> +++ b/include/dt-bindings/sound/rockchip,i2s-tdm.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _DT_BINDINGS_ROCKCHIP_I2S_TDM_H
> +#define _DT_BINDINGS_ROCKCHIP_I2S_TDM_H
> +
> +#define RK_TRCM_TXRX 0
> +#define RK_TRCM_TX 1
> +#define RK_TRCM_RX 2
> +
> +#endif /* _DT_BINDINGS_ROCKCHIP_I2S_TDM_H */
> -- 
> 2.32.0
> 
> 

Re: [PATCH 2/4] dt-bindings: sound: add rockchip i2s-tdm binding

[Robin Murphy]

On 2021-08-17 11:11, Nicolas Frattaroli wrote:
> This adds the YAML bindings for the Rockchip I2S/TDM audio driver.
> 
> Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> ---
>   .../bindings/sound/rockchip,i2s-tdm.yaml      | 221 ++++++++++++++++++
>   include/dt-bindings/sound/rockchip,i2s-tdm.h  |   9 +
>   2 files changed, 230 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml
>   create mode 100644 include/dt-bindings/sound/rockchip,i2s-tdm.h
> 
> diff --git a/Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml b/Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml
> new file mode 100644
> index 000000000000..c3022620b47f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/rockchip,i2s-tdm.yaml
> @@ -0,0 +1,221 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/rockchip,i2s-tdm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip I2S/TDM Controller
> +
> +description:
> +  The Rockchip I2S/TDM Controller is a Time Division Multiplexed
> +  audio interface found in various Rockchip SoCs, allowing up
> +  to 8 channels of audio over a serial interface.
> +
> +maintainers:
> +  - Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - rockchip,px30-i2s-tdm
> +      - rockchip,rk1808-i2s-tdm
> +      - rockchip,rk3308-i2s-tdm
> +      - rockchip,rk3568-i2s-tdm
> +      - rockchip,rv1126-i2s-tdm
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  dmas:
> +    minItems: 1
> +    maxItems: 2
> +
> +  dma-names:
> +    oneOf:
> +      - const: rx
> +      - items:
> +          - const: tx
> +          - const: rx

It seems a bit weird for Rx DMA to be mandatory when other things imply 
that some interfaces may only be used for Tx.

> +
> +  clocks:
> +    items:
> +      - description: clock for TX
> +      - description: clock for RX
> +      - description: clock for I2S bus

Nit: "hclk" normally means the AHB clock driving the bus interface which 
connects to the rest of the SoC (and often doubling up as the core clock 
for the IP block itself) - AIUI the Tx and Rx represent the I2S bus(es).

> +
> +  clock-names:
> +    items:
> +      - const: mclk_tx
> +      - const: mclk_rx
> +      - const: hclk
> +
> +  rockchip,frame-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 64
> +    minimum: 32
> +    maximum: 512
> +    description:
> +      Width of a frame, usually slot width multiplied by number of slots.
> +      Must be even.
> +
> +  resets:
> +    items:
> +      - description: reset for TX
> +      - description: reset for RX
> +
> +  reset-names:
> +    items:
> +      - const: tx-m
> +      - const: rx-m
> +
> +  rockchip,cru:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      The phandle of the cru.
> +      Required if both playback and capture are used, i.e. if rockchip,clk-trcm
> +      is 0.
> +
> +  rockchip,grf:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      The phandle of the syscon node for the GRF register.
> +
> +  rockchip,mclk-calibrate:
> +    description:
> +      Enable mclk source calibration.
> +    type: boolean
> +
> +  rockchip,trcm-sync:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Which lrck/bclk clocks each direction will sync to. You should use the
> +      constants in <dt-bindings/sound/rockchip,i2s-tdm.h>
> +    oneOf:
> +      - const: 0
> +        description:
> +          RK_TRCM_TXRX. Use both the TX and the RX clock for TX and RX.
> +      - const: 1
> +        description:
> +          RK_TRCM_TX. Use only the TX clock for TX and RX.
> +      - const: 2
> +        description:
> +          RK_TRCM_RX. Use only the RX clock for TX and RX.

I wonder if that might make sense to have boolean properties to describe 
the latter two cases (which would effectively be mutually-exclusive), 
rather than a magic number? Or possibly even just make the respective 
clocks optional, if this is something which would be done per-SoC rather 
than per-board?

> +
> +  "#sound-dai-cells":
> +    const: 0
> +
> +  rockchip,no-dmaengine:
> +    description:
> +      If present, driver will not register a pcm dmaengine, only the dai.
> +      If the dai is part of multi-dais, the property should be present.
> +    type: boolean

That sounds a lot more like a policy decision specific to the Linux 
driver implementation, than something which really belongs in DT as a 
description of the platform.

> +
> +  rockchip,playback-only:
> +    description: Specify that the controller only has playback capability.
> +    type: boolean
> +
> +  rockchip,capture-only:
> +    description: Specify that the controller only has capture capability.
> +    type: boolean

Could those be inferred from the compatible string, or are there cases 
where you have multiple instances of the IP block in different 
configurations within the same SoC? (Or if it's merely reflecting 
whether the respective interface is actually wired up externally, could 
that be inferred from the attached codec?)

Robin.

> +  rockchip,i2s-rx-route:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Defines the mapping of I2S RX sdis to I2S data bus lines.
> +      By default, they are mapped one-to-one.
> +    items:
> +      - description: which sdi to connect to data line 0
> +      - description: which sdi to connect to data line 1
> +      - description: which sdi to connect to data line 2
> +      - description: which sdi to connect to data line 3
> +
> +  rockchip,i2s-tx-route:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Defines the mapping of I2S TX sdos to I2S data bus lines.
> +      By default, they are mapped one-to-one.
> +    items:
> +      - description: which sdo to connect to data line 0
> +      - description: which sdo to connect to data line 1
> +      - description: which sdo to connect to data line 2
> +      - description: which sdo to connect to data line 3
> +
> +  rockchip,tdm-fsync-half-frame:
> +    description: Whether to use half frame fsync.
> +    type: boolean
> +
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - dmas
> +  - dma-names
> +  - clocks
> +  - clock-names
> +  - resets
> +  - reset-names
> +  - rockchip,grf
> +  - "#sound-dai-cells"
> +  - rockchip,trcm-sync
> +
> +allOf:
> +  - if:
> +      properties:
> +        rockchip,clk-trcm:
> +          contains:
> +            enum: [0]
> +    then:
> +      required:
> +        - rockchip,cru
> +
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/rk3568-cru.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/pinctrl/rockchip.h>
> +    #include <dt-bindings/sound/rockchip,i2s-tdm.h>
> +
> +
> +    foo {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +        i2s@fe410000 {
> +            compatible = "rockchip,rk3568-i2s-tdm";
> +            reg = <0x0 0xfe410000 0x0 0x1000>;
> +            interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>;
> +            clocks = <&cru MCLK_I2S1_8CH_TX>, <&cru MCLK_I2S1_8CH_RX>,
> +                     <&cru HCLK_I2S1_8CH>;
> +            clock-names = "mclk_tx", "mclk_rx", "hclk";
> +            dmas = <&dmac1 2>, <&dmac1 3>;
> +            dma-names = "tx", "rx";
> +            resets = <&cru SRST_M_I2S1_8CH_TX>, <&cru SRST_M_I2S1_8CH_RX>;
> +            reset-names = "tx-m", "rx-m";
> +            rockchip,trcm-sync = <RK_TRCM_TX>;
> +            rockchip,cru = <&cru>;
> +            rockchip,grf = <&grf>;
> +            #sound-dai-cells = <0>;
> +            pinctrl-names = "default";
> +            pinctrl-0 =
> +                <&i2s1m0_sclktx
> +                &i2s1m0_sclkrx
> +                &i2s1m0_lrcktx
> +                &i2s1m0_lrckrx
> +                &i2s1m0_sdi0
> +                &i2s1m0_sdi1
> +                &i2s1m0_sdi2
> +                &i2s1m0_sdi3
> +                &i2s1m0_sdo0
> +                &i2s1m0_sdo1
> +                &i2s1m0_sdo2
> +                &i2s1m0_sdo3>;
> +            status = "disabled";
> +        };
> +    };
> diff --git a/include/dt-bindings/sound/rockchip,i2s-tdm.h b/include/dt-bindings/sound/rockchip,i2s-tdm.h
> new file mode 100644
> index 000000000000..32494d64cf33
> --- /dev/null
> +++ b/include/dt-bindings/sound/rockchip,i2s-tdm.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _DT_BINDINGS_ROCKCHIP_I2S_TDM_H
> +#define _DT_BINDINGS_ROCKCHIP_I2S_TDM_H
> +
> +#define RK_TRCM_TXRX 0
> +#define RK_TRCM_TX 1
> +#define RK_TRCM_RX 2
> +
> +#endif /* _DT_BINDINGS_ROCKCHIP_I2S_TDM_H */
> 

Re: [PATCH 2/4] dt-bindings: sound: add rockchip i2s-tdm binding

[Nicolas Frattaroli]

On Donnerstag, 19. August 2021 14:08:36 CEST Robin Murphy wrote:
> On 2021-08-17 11:11, Nicolas Frattaroli wrote:
> > +  rockchip,trcm-sync:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Which lrck/bclk clocks each direction will sync to. You should use
> > the +      constants in <dt-bindings/sound/rockchip,i2s-tdm.h>
> > +    oneOf:
> > +      - const: 0
> > +        description:
> > +          RK_TRCM_TXRX. Use both the TX and the RX clock for TX and RX.
> > +      - const: 1
> > +        description:
> > +          RK_TRCM_TX. Use only the TX clock for TX and RX.
> > +      - const: 2
> > +        description:
> > +          RK_TRCM_RX. Use only the RX clock for TX and RX.
> 
> I wonder if that might make sense to have boolean properties to describe
> the latter two cases (which would effectively be mutually-exclusive),
> rather than a magic number? Or possibly even just make the respective
> clocks optional, if this is something which would be done per-SoC rather
> than per-board?
> 

From what I know from downstream vendor device trees, these are per
board, not for the SoC as a whole. There are I2S/TDM controllers on the
SoC which I think are hardwired to certain other IP blocks, such as I2S0
being connected to HDMI, but I2S1 can be routed outside of the SoC where
these come into play I believe.

As for making them boolean properties, I'd rather not. If I were to make it
two mutually exclusive booleans, this would result in 4 possible states
rather than 3, and require complexity to check it both in the schema and
in the probe function. Like this, I can get away with a switch case that
has a fallthrough, and a list of consts in the schema.

> > +
> > +  "#sound-dai-cells":
> > +    const: 0
> > +
> > +  rockchip,no-dmaengine:
> > +    description:
> > +      If present, driver will not register a pcm dmaengine, only the dai.
> > +      If the dai is part of multi-dais, the property should be present.
> > +    type: boolean
> 
> That sounds a lot more like a policy decision specific to the Linux
> driver implementation, than something which really belongs in DT as a
> description of the platform.

I agree. Should I be refactoring this into a module parameter or
something along those lines? I'm unsure of where this goes.

> 
> > +
> > +  rockchip,playback-only:
> > +    description: Specify that the controller only has playback
> > capability.
> > +    type: boolean
> > +
> > +  rockchip,capture-only:
> > +    description: Specify that the controller only has capture capability.
> > +    type: boolean
> 
> Could those be inferred from the compatible string, or are there cases
> where you have multiple instances of the IP block in different
> configurations within the same SoC? (Or if it's merely reflecting
> whether the respective interface is actually wired up externally, could
> that be inferred from the attached codec?)
> 
> Robin.
> 

They can't be inferred from the SoC because there are indeed multiple
instances of this IP block in different configurations on the same SoC.
The RK3566 and RK3568 have four in total, of two different categories,
each being able to be configured for a different format (though the
number of channels and available formats vary for the two categories,
one group only supports I2S and PCM with two channels)

The particular configuration may even vary per-board; an I2S/TDM
controller may be connected to an external codec which does not
support capture, whereas on another board it may be connected to
one that does.

As an example, if I understand it correctly, I2S3 on the RK3566 and
RK3568 can do 2 channels RX and TX in I2S mode, but only 2 channels
either RX or TX in PCM mode, but I'm unsure of the language in the
(still not public) documentation I have.

Regards,
Nicolas Frattaroli

Re: [PATCH 2/4] dt-bindings: sound: add rockchip i2s-tdm binding

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

On Thu, Aug 19, 2021 at 03:52:55PM +0200, Nicolas Frattaroli wrote:
> On Donnerstag, 19. August 2021 14:08:36 CEST Robin Murphy wrote:

> > > +  rockchip,no-dmaengine:
> > > +    description:
> > > +      If present, driver will not register a pcm dmaengine, only the dai.
> > > +      If the dai is part of multi-dais, the property should be present.
> > > +    type: boolean

> > That sounds a lot more like a policy decision specific to the Linux
> > driver implementation, than something which really belongs in DT as a
> > description of the platform.

> I agree. Should I be refactoring this into a module parameter or
> something along those lines? I'm unsure of where this goes.

Why is this even required?  What is "multi-dais" and why would
registering the DMA stuff cause a problem?

> The particular configuration may even vary per-board; an I2S/TDM
> controller may be connected to an external codec which does not
> support capture, whereas on another board it may be connected to
> one that does.

If the external device doesn't support both directions then why does the
driver for the I2S controller in the CPU care?  The constraint handling
code in the core will ensure that nothing tries to start something that
isn't supported

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/4] dt-bindings: sound: add rockchip i2s-tdm binding

[Nicolas Frattaroli]

On Donnerstag, 19. August 2021 16:16:17 CEST Mark Brown wrote:
> On Thu, Aug 19, 2021 at 03:52:55PM +0200, Nicolas Frattaroli wrote:
> > On Donnerstag, 19. August 2021 14:08:36 CEST Robin Murphy wrote:
> > > > +  rockchip,no-dmaengine:
> > > > +    description:
> > > > +      If present, driver will not register a pcm dmaengine, only the
> > > > dai.
> > > > +      If the dai is part of multi-dais, the property should be
> > > > present.
> > > > +    type: boolean
> > > 
> > > That sounds a lot more like a policy decision specific to the Linux
> > > driver implementation, than something which really belongs in DT as a
> > > description of the platform.
> > 
> > I agree. Should I be refactoring this into a module parameter or
> > something along those lines? I'm unsure of where this goes.
> 
> Why is this even required?  What is "multi-dais" and why would
> registering the DMA stuff cause a problem?

After some research, it appears that multi-dais is a special driver that
downstream uses to allow multiple sub-DAIs to be combined into one DAI
that has all the channels of the sub-DAIs. This does not seem like
something that should be done at that level to me, because it seems
like it's pushing a sound driver configuration into the realm of
hardware description.

In retrospect, I should have stripped this out before submitting it,
because I should not be submitting things I don't understand completely.
I apologise.

> > The particular configuration may even vary per-board; an I2S/TDM
> > controller may be connected to an external codec which does not
> > support capture, whereas on another board it may be connected to
> > one that does.
> 
> If the external device doesn't support both directions then why does the
> driver for the I2S controller in the CPU care?  The constraint handling
> code in the core will ensure that nothing tries to start something that
> isn't supported

I went over the downstream text binding description again and from that
it appears that the playback/capture-only capability is something
specific to the controller, not to any device connected to it.

The downstream device tree for the rk3568 specifies playback-only for
I2S0, a.k.a. the one connected to the HDMI that I can't test because
we currently don't have a video clock. Another downstream device tree,
specific to what appears to be a robot demo for the px30 SoC, uses this
property on i2s1, which tells me that this is not an actual description
of the controller hardware but just a description of the application.

While not relevant to the device tree schema, the driver reacts to these
properties by setting the opposite directions _minimum_ channel number
to 0 (from the default of 2.)

My conclusion from this is that this reeks of nonsense and I will look
into what happens when I simply remove these properties and lower the
channels_min to 0 in the driver. If it turns out that on some SoC for
some I2S/TDM controller instance there is a limitation where specifying
that the controller only handles either capture or playback does make
sense, we can always add it later.

Thank you for your comments,
Nicolas Frattaroli

Re: [PATCH 2/4] dt-bindings: sound: add rockchip i2s-tdm binding

[Robin Murphy]

On 2021-08-19 14:52, Nicolas Frattaroli wrote:
> On Donnerstag, 19. August 2021 14:08:36 CEST Robin Murphy wrote:
>> On 2021-08-17 11:11, Nicolas Frattaroli wrote:
>>> +  rockchip,trcm-sync:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      Which lrck/bclk clocks each direction will sync to. You should use
>>> the +      constants in <dt-bindings/sound/rockchip,i2s-tdm.h>
>>> +    oneOf:
>>> +      - const: 0
>>> +        description:
>>> +          RK_TRCM_TXRX. Use both the TX and the RX clock for TX and RX.
>>> +      - const: 1
>>> +        description:
>>> +          RK_TRCM_TX. Use only the TX clock for TX and RX.
>>> +      - const: 2
>>> +        description:
>>> +          RK_TRCM_RX. Use only the RX clock for TX and RX.
>>
>> I wonder if that might make sense to have boolean properties to describe
>> the latter two cases (which would effectively be mutually-exclusive),
>> rather than a magic number? Or possibly even just make the respective
>> clocks optional, if this is something which would be done per-SoC rather
>> than per-board?
>>
> 
>  From what I know from downstream vendor device trees, these are per
> board, not for the SoC as a whole. There are I2S/TDM controllers on the
> SoC which I think are hardwired to certain other IP blocks, such as I2S0
> being connected to HDMI, but I2S1 can be routed outside of the SoC where
> these come into play I believe.

That's fair enough. I know a lot more about DT bindings than I do about 
I2S, but I did guess it might be related to clocking requirements of the 
connected codec rather than a constraint of the I2S block itself.

> As for making them boolean properties, I'd rather not. If I were to make it
> two mutually exclusive booleans, this would result in 4 possible states
> rather than 3, and require complexity to check it both in the schema and
> in the probe function. Like this, I can get away with a switch case that
> has a fallthrough, and a list of consts in the schema.

Complexity?


	if (of_property_read_bool(node, "rockchip,trcm-sync-tx-only"))
		i2s_tdm->clk_trcm = RK_TRCM_TX;
	if (of_property_read_bool(node, "rockchip,trcm-sync-rx-only")) {
		if (i2s_td->clk_trcm) {
			dev_err(i2s_tdm->dev, "invalid trcm-sync configuration\n");
			return -EINVAL;
		}
		i2s_tdm->clk_trcm = RK_TRCM_RX;
	}
	if (i2s_td->clk_trcm)
		i2s_tdm_dai.symmetric_rate = 1;


If I'm counting correctly, that off-the-top-of-my-head example is a mere 
58% of the size of your switch statement ;)

The usual aim in designing bindings to robustly abstract the underlying 
features, not to be easy to implement. That's why the "put this magic 
value in this register" style of property is generally frowned upon.

As for the schema, it doesn't necessarily have to try to exhaustively 
catch every possible usage error - if a combination of properties is so 
obviously nonsensical that a driver shouldn't accept it anyway, I'd 
imagine it's unlikely to slip through testing.

>>> +
>>> +  "#sound-dai-cells":
>>> +    const: 0
>>> +
>>> +  rockchip,no-dmaengine:
>>> +    description:
>>> +      If present, driver will not register a pcm dmaengine, only the dai.
>>> +      If the dai is part of multi-dais, the property should be present.
>>> +    type: boolean
>>
>> That sounds a lot more like a policy decision specific to the Linux
>> driver implementation, than something which really belongs in DT as a
>> description of the platform.
> 
> I agree. Should I be refactoring this into a module parameter or
> something along those lines? I'm unsure of where this goes.

Depends on what it actually means, and whether that's something the 
driver can figure out for itself. I just see a DT property based around 
a particular Linux API call as a big red flag :)

>>> +
>>> +  rockchip,playback-only:
>>> +    description: Specify that the controller only has playback
>>> capability.
>>> +    type: boolean
>>> +
>>> +  rockchip,capture-only:
>>> +    description: Specify that the controller only has capture capability.
>>> +    type: boolean
>>
>> Could those be inferred from the compatible string, or are there cases
>> where you have multiple instances of the IP block in different
>> configurations within the same SoC? (Or if it's merely reflecting
>> whether the respective interface is actually wired up externally, could
>> that be inferred from the attached codec?)
>>
>> Robin.
>>
> 
> They can't be inferred from the SoC because there are indeed multiple
> instances of this IP block in different configurations on the same SoC.
> The RK3566 and RK3568 have four in total, of two different categories,
> each being able to be configured for a different format (though the
> number of channels and available formats vary for the two categories,
> one group only supports I2S and PCM with two channels)
> 
> The particular configuration may even vary per-board; an I2S/TDM
> controller may be connected to an external codec which does not
> support capture, whereas on another board it may be connected to
> one that does.

Fair enough again, but surely if the codec doesn't support capture then 
in the end no capture interface is going to be exposed anyway - does the 
low-level transport need to care?

> As an example, if I understand it correctly, I2S3 on the RK3566 and
> RK3568 can do 2 channels RX and TX in I2S mode, but only 2 channels
> either RX or TX in PCM mode, but I'm unsure of the language in the
> (still not public) documentation I have.

And that starts to sound like something the driver should probably be 
aware of anyway, but at very least only casts more doubt on these 
particular properties - even if an interface to a stereo PCM codec 
couldn't support simultaneous playback and recording, couldn't it still 
support doing either, separately?

Robin.

[PATCH 4/4] arm64: dts: rockchip: Add analog audio on Quartz64

[Nicolas Frattaroli]

On the Quartz64 Model A, the I2S1 TDM controller is connected
to the rk817 codec in I2S mode. Enabling it and adding the
necessary simple-sound-card and codec nodes allows for analog
audio output on the PINE64 Quartz64 Model A SBC.

Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
---
 .../boot/dts/rockchip/rk3566-quartz64-a.dts   | 31 ++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
index a244f7b87e38..f1261f25cb35 100644
--- a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
@@ -58,6 +58,20 @@ led-diy {
 		};
 	};
 
+	rk817-sound {
+		compatible = "simple-audio-card";
+		simple-audio-card,format = "i2s";
+		simple-audio-card,name = "Analog RK817";
+		simple-audio-card,mclk-fs = <256>;
+
+		simple-audio-card,cpu {
+			sound-dai = <&i2s1_8ch>;
+		};
+		simple-audio-card,codec {
+			sound-dai = <&rk817>;
+		};
+	};
+
 	vcc12v_dcin: vcc12v_dcin {
 		compatible = "regulator-fixed";
 		regulator-name = "vcc12v_dcin";
@@ -199,8 +213,13 @@ rk817: pmic@20 {
 		interrupts = <RK_PA3 IRQ_TYPE_LEVEL_LOW>;
 		clock-output-names = "rk808-clkout1", "rk808-clkout2";
 
+		#sound-dai-cells = <0>;
+		clock-names = "mclk";
+		clocks = <&cru I2S1_MCLKOUT_TX>;
+		assigned-clocks = <&cru I2S1_MCLKOUT_TX>;
+		assigned-clock-parents = <&cru CLK_I2S1_8CH_TX>;
 		pinctrl-names = "default";
-		pinctrl-0 = <&pmic_int_l>;
+		pinctrl-0 = <&pmic_int_l>, <&i2s1m0_mclk>;
 		rockchip,system-power-controller;
 		wakeup-source;
 		#clock-cells = <1>;
@@ -392,6 +411,16 @@ regulator-state-mem {
 	};
 };
 
+&i2s1_8ch {
+	status = "okay";
+	rockchip,trcm-sync-tx-only;
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2s1m0_sclktx
+		     &i2s1m0_lrcktx
+		     &i2s1m0_sdi0
+		     &i2s1m0_sdo0>;
+};
+
 &mdio1 {
 	rgmii_phy1: ethernet-phy@0 {
 		compatible = "ethernet-phy-ieee802.3-c22";
-- 
2.33.1