This patchset adds TDM audio driver for the StarFive JH7110 SoC. The
first patch adds device tree binding for TDM module. The second patch
adds the item for JH7110 audio board to the dt-binding of StarFive
SoC-based boards. The third patch adds tdm driver support for JH7110
SoC. The last patch adds device node of tdm and sound card to JH7110 dts.
The series has been tested on the VisionFive 2 board by plugging the
raspberry pie audio board.
For more information of raspberry pie audio board, you can take a look
at the following webpage:
https://wiki.seeedstudio.com/ReSpeaker_2_Mics_Pi_HAT/
The last patch should be applied after the following patchset:
https://lore.kernel.org/all/[email protected]/
Changes since v1:
- Rebased on Linux 6.3-rc4.
- Added the dts file dedicated to describe audio card.
- Added the item for JH7110 audio board to the dt-binding of StarFive
SoC-based boards.
---
v1: https://lore.kernel.org/all/[email protected]/
Walker Chen (4):
dt-bindings: sound: Add TDM for StarFive JH7110
dt-bindings: riscv: Add item for StarFive JH7110 audio board
ASoC: starfive: Add JH7110 TDM driver
riscv: dts: starfive: add tdm node and sound card
.../devicetree/bindings/riscv/starfive.yaml | 1 +
.../bindings/sound/starfive,jh7110-tdm.yaml | 98 +++
MAINTAINERS | 6 +
arch/riscv/boot/dts/starfive/Makefile | 1 +
.../starfive/jh7110-starfive-audio-card.dts | 67 ++
.../jh7110-starfive-visionfive-2.dtsi | 40 ++
arch/riscv/boot/dts/starfive/jh7110.dtsi | 21 +
sound/soc/Kconfig | 1 +
sound/soc/Makefile | 1 +
sound/soc/starfive/Kconfig | 15 +
sound/soc/starfive/Makefile | 2 +
sound/soc/starfive/jh7110_tdm.c | 579 ++++++++++++++++++
sound/soc/starfive/jh7110_tdm.h | 155 +++++
13 files changed, 987 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml
create mode 100644 arch/riscv/boot/dts/starfive/jh7110-starfive-audio-card.dts
create mode 100644 sound/soc/starfive/Kconfig
create mode 100644 sound/soc/starfive/Makefile
create mode 100644 sound/soc/starfive/jh7110_tdm.c
create mode 100644 sound/soc/starfive/jh7110_tdm.h
base-commit: 197b6b60ae7bc51dd0814953c562833143b292aa
prerequisite-patch-id: 30bec4dba6f250a6edd0c2cbab2ce09442e50e8a
prerequisite-patch-id: bb939c0c7c26b08addfccd890f9d3974b6eaec53
prerequisite-patch-id: 8a6f135bcabdad4a4bfb21f0c6a0ffd2bb57efe7
prerequisite-patch-id: c2366f993a9d85e28c06d8d09f064dd5e8b29a61
prerequisite-patch-id: 50d53a21f91f4087fc80b6f1f72864adfb0002b9
prerequisite-patch-id: 0df3703af91c30f1ca2c47f5609012f2d7200028
prerequisite-patch-id: 89f049f951e5acf75aab92541992f816fd0acc0d
prerequisite-patch-id: 551fae54377090044c3612fca9740a9b359abdd2
prerequisite-patch-id: c7fdf904f398d478f0ed6d57eb878982bc73329d
prerequisite-patch-id: 1b2d0982b18da060c82134f05bf3ce16425bac8d
prerequisite-patch-id: 090ba4b78d47bc19204916e76fdbc70021785388
prerequisite-patch-id: a5d9e0f7d4f8163f566678894cf693015119f2d9
prerequisite-patch-id: 4637a8fa2334a45fa6b64351f4e9e28d3e2d60d3
prerequisite-patch-id: 32647ec60a3b614e1c59ec8e54cb511ae832c22f
prerequisite-patch-id: aa06658ecf89c92d0dfdd6a4ba6d9e6e67532971
prerequisite-patch-id: 1387a7e87b446329dfc21f3e575ceae7ebcf954c
prerequisite-patch-id: 258ea5f9b8bf41b6981345dcc81795f25865d38f
prerequisite-patch-id: 8b6f2c9660c0ac0ee4e73e4c21aca8e6b75e81b9
prerequisite-patch-id: dbb0c0151b8bdf093e6ce79fd2fe3f60791a6e0b
prerequisite-patch-id: 9007c8610fdcd387592475949864edde874c20a2
prerequisite-patch-id: d57e95d31686772abc4c4d5aa1cadc344dc293cd
prerequisite-patch-id: 9f911969d0a550648493952c99096d26e05d4d83
prerequisite-patch-id: 2ddada18ab6ea5cd1da14212aaf59632f5203d40
prerequisite-patch-id: 80042661ff6156ce577a72e9eb8c0b218b624829
prerequisite-patch-id: 398744c61913c76a35754de867c4f820ca7a8d99
prerequisite-patch-id: f59269382164b5d642a5e10443ca447f5caa595c
prerequisite-patch-id: 1babe83d6bf999bad17584dc595480f9070a5369
prerequisite-patch-id: 77be3d122d66df813f13088141ce27b21107a341
prerequisite-patch-id: 9fbb7ad1dd258bb8ff5946c4a0e59de4bfd82a04
prerequisite-patch-id: 6f6984916dffd0cc66aa733c9b6bd3a55495a50c
prerequisite-patch-id: 39e1be2a3d1593577ab997f55f59367cba665aa7
prerequisite-patch-id: 584c256c9acb52ee2773d0c81c3f4977fc18155a
prerequisite-patch-id: b37ac15032973e1fcd918f157c82a0606775c9e9
prerequisite-patch-id: 32deea16304859842af5c2151bc41d91cf6dfc9b
prerequisite-patch-id: 20ac2450fb93b3f69f83fc720fd4800a95e618a6
prerequisite-patch-id: 6abf359fa445f4104432ddee27044dfbfb128417
--
2.17.1
Add the tdm controller node and sound card for the StarFive JH7110 SoC.
Reviewed-by: Hal Feng <[email protected]>
Signed-off-by: Walker Chen <[email protected]>
---
arch/riscv/boot/dts/starfive/Makefile | 1 +
.../starfive/jh7110-starfive-audio-card.dts | 67 +++++++++++++++++++
.../jh7110-starfive-visionfive-2.dtsi | 40 +++++++++++
arch/riscv/boot/dts/starfive/jh7110.dtsi | 21 ++++++
4 files changed, 129 insertions(+)
create mode 100644 arch/riscv/boot/dts/starfive/jh7110-starfive-audio-card.dts
diff --git a/arch/riscv/boot/dts/starfive/Makefile b/arch/riscv/boot/dts/starfive/Makefile
index 170956846d49..cb22cb7f66b0 100644
--- a/arch/riscv/boot/dts/starfive/Makefile
+++ b/arch/riscv/boot/dts/starfive/Makefile
@@ -4,3 +4,4 @@ dtb-$(CONFIG_ARCH_STARFIVE) += jh7100-starfive-visionfive-v1.dtb
dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-starfive-visionfive-2-v1.2a.dtb
dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-starfive-visionfive-2-v1.3b.dtb
+dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-starfive-audio-card.dtb
diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-audio-card.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-audio-card.dts
new file mode 100644
index 000000000000..967d9aa7d1e1
--- /dev/null
+++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-audio-card.dts
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * Device Tree for JH7110 + Simple Audio Card
+ *
+ * Copyright (C) 2023 StarFive Technology Co., Ltd.
+ */
+
+/dts-v1/;
+#include "jh7110-starfive-visionfive-2-v1.3b.dts"
+
+/ {
+ model = "StarFive VisionFive 2 Audio Board";
+ compatible = "starfive,visionfive-2-audio", "starfive,jh7110";
+
+ wm8960_mclk: wm8960-mclk {
+ compatible = "fixed-clock";
+ clock-output-names = "wm8960_mclk";
+ #clock-cells = <0>;
+ clock-frequency = <24576000>;
+ };
+
+ sound {
+ compatible = "simple-audio-card";
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ simple-audio-card,name = "Starfive-TDM-Sound-Card";
+ simple-audio-card,widgets = "Microphone", "Mic Jack",
+ "Line", "Line In",
+ "Line", "Line Out",
+ "Speaker", "Speaker",
+ "Headphone", "Headphone Jack";
+ simple-audio-card,routing = "Headphone Jack", "HP_L",
+ "Headphone Jack", "HP_R",
+ "Speaker", "SPK_LP",
+ "Speaker", "SPK_LN",
+ "LINPUT1", "Mic Jack",
+ "LINPUT3", "Mic Jack",
+ "RINPUT1", "Mic Jack",
+ "RINPUT2", "Mic Jack";
+
+ simple-audio-card,dai-link@0 {
+ reg = <0>;
+ format = "dsp_a";
+ bitclock-master = <&dailink_master>;
+ frame-master = <&dailink_master>;
+
+ cpu {
+ sound-dai = <&tdm>;
+ };
+ dailink_master: codec {
+ sound-dai = <&wm8960>;
+ clocks = <&wm8960_mclk>;
+ };
+ };
+ };
+};
+
+&i2c0 {
+ wm8960: codec@1a {
+ compatible = "wlf,wm8960";
+ reg = <0x1a>;
+ wlf,shared-lrclk;
+ #sound-dai-cells = <0>;
+ };
+};
diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
index 1155b97b593d..19b5954ee72d 100644
--- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
@@ -214,6 +214,40 @@
slew-rate = <0>;
};
};
+
+ tdm0_pins: tdm0-pins {
+ tdm0-pins-tx {
+ pinmux = <GPIOMUX(44, GPOUT_SYS_TDM_TXD,
+ GPOEN_ENABLE,
+ GPI_NONE)>;
+ bias-pull-up;
+ drive-strength = <2>;
+ input-disable;
+ input-schmitt-disable;
+ slew-rate = <0>;
+ };
+
+ tdm0-pins-rx {
+ pinmux = <GPIOMUX(61, GPOUT_HIGH,
+ GPOEN_DISABLE,
+ GPI_SYS_TDM_RXD)>;
+ input-enable;
+ };
+
+ tdm0-pins-sync {
+ pinmux = <GPIOMUX(63, GPOUT_HIGH,
+ GPOEN_DISABLE,
+ GPI_SYS_TDM_SYNC)>;
+ input-enable;
+ };
+
+ tdm0-pins-pcmclk {
+ pinmux = <GPIOMUX(38, GPOUT_HIGH,
+ GPOEN_DISABLE,
+ GPI_SYS_TDM_CLK)>;
+ input-enable;
+ };
+ };
};
&uart0 {
@@ -221,3 +255,9 @@
pinctrl-0 = <&uart0_pins>;
status = "okay";
};
+
+&tdm {
+ pinctrl-names = "default";
+ pinctrl-0 = <&tdm0_pins>;
+ status = "okay";
+};
diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index 866313570a7e..cfda6fb0d91b 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -366,6 +366,27 @@
status = "disabled";
};
+ tdm: tdm@10090000 {
+ compatible = "starfive,jh7110-tdm";
+ reg = <0x0 0x10090000 0x0 0x1000>;
+ clocks = <&syscrg JH7110_SYSCLK_TDM_AHB>,
+ <&syscrg JH7110_SYSCLK_TDM_APB>,
+ <&syscrg JH7110_SYSCLK_TDM_INTERNAL>,
+ <&syscrg JH7110_SYSCLK_TDM_TDM>,
+ <&syscrg JH7110_SYSCLK_MCLK_INNER>,
+ <&tdm_ext>;
+ clock-names = "tdm_ahb", "tdm_apb",
+ "tdm_internal", "tdm",
+ "mclk_inner", "tdm_ext";
+ resets = <&syscrg JH7110_SYSRST_TDM_AHB>,
+ <&syscrg JH7110_SYSRST_TDM_APB>,
+ <&syscrg JH7110_SYSRST_TDM_CORE>;
+ dmas = <&dma 20>, <&dma 21>;
+ dma-names = "rx","tx";
+ #sound-dai-cells = <0>;
+ status = "disabled";
+ };
+
stgcrg: clock-controller@10230000 {
compatible = "starfive,jh7110-stgcrg";
reg = <0x0 0x10230000 0x0 0x10000>;
--
2.17.1
Add bindings to describe the TDM driver for the StarFive JH7110 SoC.
Signed-off-by: Walker Chen <[email protected]>
---
.../bindings/sound/starfive,jh7110-tdm.yaml | 98 +++++++++++++++++++
1 file changed, 98 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml
diff --git a/Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml b/Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml
new file mode 100644
index 000000000000..abb373fbfa26
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml
@@ -0,0 +1,98 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/starfive,jh7110-tdm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH7110 TDM Controller
+
+description: |
+ The TDM Controller is a Time Division Multiplexed audio interface
+ integrated in StarFive JH7110 SoC, allowing up to 8 channels of
+ audio over a serial interface. The TDM controller can operate both
+ in master and slave mode.
+
+maintainers:
+ - Walker Chen <[email protected]>
+
+allOf:
+ - $ref: dai-common.yaml#
+
+properties:
+ compatible:
+ enum:
+ - starfive,jh7110-tdm
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: TDM AHB Clock
+ - description: TDM APB Clock
+ - description: TDM Internal Clock
+ - description: TDM Clock
+ - description: Inner MCLK
+ - description: TDM External Clock
+
+ clock-names:
+ items:
+ - const: tdm_ahb
+ - const: tdm_apb
+ - const: tdm_internal
+ - const: tdm
+ - const: mclk_inner
+ - const: tdm_ext
+
+ resets:
+ items:
+ - description: tdm ahb reset line
+ - description: tdm apb reset line
+ - description: tdm core reset line
+
+ dmas:
+ items:
+ - description: RX DMA Channel
+ - description: TX DMA Channel
+
+ dma-names:
+ items:
+ - const: rx
+ - const: tx
+
+ "#sound-dai-cells":
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - resets
+ - dmas
+ - dma-names
+ - "#sound-dai-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ tdm@10090000 {
+ compatible = "starfive,jh7110-tdm";
+ reg = <0x10090000 0x1000>;
+ clocks = <&syscrg 184>,
+ <&syscrg 185>,
+ <&syscrg 186>,
+ <&syscrg 187>,
+ <&syscrg 17>,
+ <&tdm_ext>;
+ clock-names = "tdm_ahb", "tdm_apb",
+ "tdm_internal", "tdm",
+ "mclk_inner", "tdm_ext";
+ resets = <&syscrg 105>,
+ <&syscrg 107>,
+ <&syscrg 106>;
+ dmas = <&dma 20>, <&dma 21>;
+ dma-names = "rx","tx";
+ #sound-dai-cells = <0>;
+ };
--
2.17.1
Add tdm driver support for the StarFive JH7110 SoC.
Signed-off-by: Walker Chen <[email protected]>
---
MAINTAINERS | 6 +
sound/soc/Kconfig | 1 +
sound/soc/Makefile | 1 +
sound/soc/starfive/Kconfig | 15 +
sound/soc/starfive/Makefile | 2 +
sound/soc/starfive/jh7110_tdm.c | 579 ++++++++++++++++++++++++++++++++
sound/soc/starfive/jh7110_tdm.h | 155 +++++++++
7 files changed, 759 insertions(+)
create mode 100644 sound/soc/starfive/Kconfig
create mode 100644 sound/soc/starfive/Makefile
create mode 100644 sound/soc/starfive/jh7110_tdm.c
create mode 100644 sound/soc/starfive/jh7110_tdm.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 5f9c544bc189..add89615d327 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19945,6 +19945,12 @@ F: Documentation/devicetree/bindings/power/starfive*
F: drivers/soc/starfive/jh71xx_pmu.c
F: include/dt-bindings/power/starfive,jh7110-pmu.h
+STARFIVE JH7110 TDM DRIVERS
+M: Walker Chen <[email protected]>
+S: Maintained
+F: Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml
+F: sound/soc/starfive/jh7110-tdm.*
+
STARFIVE SOC DRIVERS
M: Conor Dooley <[email protected]>
S: Maintained
diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index 848fbae26c3b..8d1d9401ecf2 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -91,6 +91,7 @@ source "sound/soc/sh/Kconfig"
source "sound/soc/sof/Kconfig"
source "sound/soc/spear/Kconfig"
source "sound/soc/sprd/Kconfig"
+source "sound/soc/starfive/Kconfig"
source "sound/soc/sti/Kconfig"
source "sound/soc/stm/Kconfig"
source "sound/soc/sunxi/Kconfig"
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index 507eaed1d6a1..65aeb4ef4068 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_SND_SOC) += sh/
obj-$(CONFIG_SND_SOC) += sof/
obj-$(CONFIG_SND_SOC) += spear/
obj-$(CONFIG_SND_SOC) += sprd/
+obj-$(CONFIG_SND_SOC) += starfive/
obj-$(CONFIG_SND_SOC) += sti/
obj-$(CONFIG_SND_SOC) += stm/
obj-$(CONFIG_SND_SOC) += sunxi/
diff --git a/sound/soc/starfive/Kconfig b/sound/soc/starfive/Kconfig
new file mode 100644
index 000000000000..737c956f7b93
--- /dev/null
+++ b/sound/soc/starfive/Kconfig
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config SND_SOC_STARFIVE
+ tristate "Audio support for StarFive SoC"
+ depends on COMPILE_TEST || SOC_STARFIVE
+ help
+ Say Y or M if you want to add support for codecs attached to
+ the Starfive SoCs' Audio interfaces. You will also need to
+ select the audio interfaces to support below.
+
+config SND_SOC_JH7110_TDM
+ tristate "JH7110 TDM device driver"
+ depends on HAVE_CLK && SND_SOC_STARFIVE
+ select SND_SOC_GENERIC_DMAENGINE_PCM
+ help
+ Say Y or M if you want to add support for StarFive TDM driver.
diff --git a/sound/soc/starfive/Makefile b/sound/soc/starfive/Makefile
new file mode 100644
index 000000000000..f7d960211d72
--- /dev/null
+++ b/sound/soc/starfive/Makefile
@@ -0,0 +1,2 @@
+# StarFive Platform Support
+obj-$(CONFIG_SND_SOC_JH7110_TDM) += jh7110_tdm.o
diff --git a/sound/soc/starfive/jh7110_tdm.c b/sound/soc/starfive/jh7110_tdm.c
new file mode 100644
index 000000000000..ec374db2e133
--- /dev/null
+++ b/sound/soc/starfive/jh7110_tdm.c
@@ -0,0 +1,579 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TDM driver for the StarFive JH7110 SoC
+ *
+ * Copyright (C) 2023 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <sound/initval.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/soc-dai.h>
+#include "jh7110_tdm.h"
+
+static inline u32 jh7110_tdm_readl(struct jh7110_tdm_dev *tdm, u16 reg)
+{
+ return readl_relaxed(tdm->tdm_base + reg);
+}
+
+static inline void jh7110_tdm_writel(struct jh7110_tdm_dev *tdm, u16 reg, u32 val)
+{
+ writel_relaxed(val, tdm->tdm_base + reg);
+}
+
+static void jh7110_tdm_save_context(struct jh7110_tdm_dev *tdm,
+ struct snd_pcm_substream *substream)
+{
+ tdm->saved_pcmgbcr = jh7110_tdm_readl(tdm, TDM_PCMGBCR);
+ tdm->saved_pcmdiv = jh7110_tdm_readl(tdm, TDM_PCMDIV);
+
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+ tdm->saved_pcmtxcr = jh7110_tdm_readl(tdm, TDM_PCMTXCR);
+ else
+ tdm->saved_pcmrxcr = jh7110_tdm_readl(tdm, TDM_PCMRXCR);
+}
+
+static void jh7110_tdm_start(struct jh7110_tdm_dev *tdm, struct snd_pcm_substream *substream)
+{
+ u32 data;
+ unsigned int val;
+
+ data = jh7110_tdm_readl(tdm, TDM_PCMGBCR);
+ jh7110_tdm_writel(tdm, TDM_PCMGBCR, data | PCMGBCR_ENABLE);
+
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+ val = jh7110_tdm_readl(tdm, TDM_PCMTXCR);
+ jh7110_tdm_writel(tdm, TDM_PCMTXCR, val | PCMTXCR_TXEN);
+ } else {
+ val = jh7110_tdm_readl(tdm, TDM_PCMRXCR);
+ jh7110_tdm_writel(tdm, TDM_PCMRXCR, val | PCMRXCR_RXEN);
+ }
+}
+
+static void jh7110_tdm_stop(struct jh7110_tdm_dev *tdm, struct snd_pcm_substream *substream)
+{
+ unsigned int val;
+
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+ val = jh7110_tdm_readl(tdm, TDM_PCMTXCR);
+ val &= ~PCMTXCR_TXEN;
+ jh7110_tdm_writel(tdm, TDM_PCMTXCR, val);
+ } else {
+ val = jh7110_tdm_readl(tdm, TDM_PCMRXCR);
+ val &= ~PCMRXCR_RXEN;
+ jh7110_tdm_writel(tdm, TDM_PCMRXCR, val);
+ }
+}
+
+static int jh7110_tdm_syncdiv(struct jh7110_tdm_dev *tdm)
+{
+ u32 sl, sscale, syncdiv;
+
+ sl = (tdm->rx.sl >= tdm->tx.sl) ? tdm->rx.sl : tdm->tx.sl;
+ sscale = (tdm->rx.sscale >= tdm->tx.sscale) ? tdm->rx.sscale : tdm->tx.sscale;
+ syncdiv = tdm->pcmclk / tdm->samplerate - 1;
+
+ if ((syncdiv + 1) < (sl * sscale)) {
+ dev_err(tdm->dev, "Failed to set syncdiv!\n");
+ return -EINVAL;
+ }
+
+ if (tdm->syncm == TDM_SYNCM_LONG &&
+ (tdm->rx.sscale <= 1 || tdm->tx.sscale <= 1)) {
+ if ((syncdiv + 1) <= sl) {
+ dev_err(tdm->dev, "Wrong syncdiv! It must be (syncdiv+1) > max[tx.sl, rx.sl]\n");
+ return -EINVAL;
+ }
+ }
+
+ jh7110_tdm_writel(tdm, TDM_PCMDIV, syncdiv);
+ return 0;
+}
+
+static void jh7110_tdm_control(struct jh7110_tdm_dev *tdm)
+{
+ u32 data;
+
+ data = (tdm->clkpolity << CLKPOL_BIT) |
+ (tdm->elm << ELM_BIT) |
+ (tdm->syncm << SYNCM_BIT) |
+ (tdm->ms_mode << MS_BIT);
+ jh7110_tdm_writel(tdm, TDM_PCMGBCR, data);
+}
+
+static void jh7110_tdm_config(struct jh7110_tdm_dev *tdm,
+ struct snd_pcm_substream *substream)
+{
+ u32 datarx, datatx;
+
+ jh7110_tdm_control(tdm);
+ jh7110_tdm_syncdiv(tdm);
+
+ datarx = (tdm->rx.ifl << IFL_BIT) |
+ (tdm->rx.wl << WL_BIT) |
+ (tdm->rx.sscale << SSCALE_BIT) |
+ (tdm->rx.sl << SL_BIT) |
+ (tdm->rx.lrj << LRJ_BIT);
+
+ datatx = (tdm->tx.ifl << IFL_BIT) |
+ (tdm->tx.wl << WL_BIT) |
+ (tdm->tx.sscale << SSCALE_BIT) |
+ (tdm->tx.sl << SL_BIT) |
+ (tdm->tx.lrj << LRJ_BIT);
+
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+ jh7110_tdm_writel(tdm, TDM_PCMTXCR, datatx);
+ else
+ jh7110_tdm_writel(tdm, TDM_PCMRXCR, datarx);
+}
+
+static void jh7110_tdm_clk_disable(struct jh7110_tdm_dev *tdm)
+{
+ clk_disable_unprepare(tdm->clk_tdm);
+ clk_disable_unprepare(tdm->clk_tdm_ext);
+ clk_disable_unprepare(tdm->clk_tdm_internal);
+ clk_disable_unprepare(tdm->clk_tdm_apb);
+ clk_disable_unprepare(tdm->clk_tdm_ahb);
+ clk_disable_unprepare(tdm->clk_mclk_inner);
+}
+
+static int jh7110_tdm_clk_enable(struct jh7110_tdm_dev *tdm)
+{
+ int ret;
+
+ ret = clk_prepare_enable(tdm->clk_mclk_inner);
+ if (ret) {
+ dev_err(tdm->dev, "failed to prepare enable clk_mclk_inner\n");
+ return ret;
+ }
+
+ ret = clk_prepare_enable(tdm->clk_tdm_ahb);
+ if (ret) {
+ dev_err(tdm->dev, "Failed to prepare enable clk_tdm_ahb\n");
+ goto dis_mclk_inner_clk;
+ }
+
+ ret = clk_prepare_enable(tdm->clk_tdm_apb);
+ if (ret) {
+ dev_err(tdm->dev, "Failed to prepare enable clk_tdm_apb\n");
+ goto dis_tdm_ahb_clk;
+ }
+
+ ret = clk_prepare_enable(tdm->clk_tdm_internal);
+ if (ret) {
+ dev_err(tdm->dev, "Failed to prepare enable clk_tdm_intl\n");
+ goto dis_tdm_apb_clk;
+ }
+
+ ret = clk_prepare_enable(tdm->clk_tdm_ext);
+ if (ret) {
+ dev_err(tdm->dev, "Failed to prepare enable clk_tdm_ext\n");
+ goto dis_tdm_internal_clk;
+ }
+
+ ret = clk_prepare_enable(tdm->clk_tdm);
+ if (ret) {
+ dev_err(tdm->dev, "Failed to prepare enable clk_tdm\n");
+ goto dis_tdm_ext_clk;
+ }
+
+ ret = reset_control_deassert(tdm->resets);
+ if (ret) {
+ dev_err(tdm->dev, "%s: failed to deassert tdm resets\n", __func__);
+ goto dis_tdm_clk;
+ }
+
+ ret = clk_set_parent(tdm->clk_tdm, tdm->clk_tdm_ext);
+ if (ret) {
+ dev_err(tdm->dev, "Can't set clock source for clk_tdm: %d\n", ret);
+ goto dis_tdm_clk;
+ }
+ return 0;
+
+dis_tdm_clk:
+ clk_disable_unprepare(tdm->clk_tdm);
+dis_tdm_ext_clk:
+ clk_disable_unprepare(tdm->clk_tdm_ext);
+dis_tdm_internal_clk:
+ clk_disable_unprepare(tdm->clk_tdm_internal);
+dis_tdm_apb_clk:
+ clk_disable_unprepare(tdm->clk_tdm_apb);
+dis_tdm_ahb_clk:
+ clk_disable_unprepare(tdm->clk_tdm_ahb);
+dis_mclk_inner_clk:
+ clk_disable_unprepare(tdm->clk_mclk_inner);
+
+ return ret;
+}
+
+#ifdef CONFIG_PM
+static int jh7110_tdm_runtime_suspend(struct device *dev)
+{
+ struct jh7110_tdm_dev *tdm = dev_get_drvdata(dev);
+
+ jh7110_tdm_clk_disable(tdm);
+
+ return 0;
+}
+
+static int jh7110_tdm_runtime_resume(struct device *dev)
+{
+ struct jh7110_tdm_dev *tdm = dev_get_drvdata(dev);
+
+ return jh7110_tdm_clk_enable(tdm);
+}
+#endif
+
+#ifdef CONFIG_PM_SLEEP
+static int jh7110_tdm_suspend(struct snd_soc_component *component)
+{
+ return pm_runtime_force_suspend(component->dev);
+}
+
+static int jh7110_tdm_resume(struct snd_soc_component *component)
+{
+ struct jh7110_tdm_dev *tdm = snd_soc_component_get_drvdata(component);
+
+ /* restore context */
+ jh7110_tdm_writel(tdm, TDM_PCMGBCR, tdm->saved_pcmgbcr);
+ jh7110_tdm_writel(tdm, TDM_PCMDIV, tdm->saved_pcmdiv);
+
+ return pm_runtime_force_resume(component->dev);
+}
+
+#else
+#define jh7110_tdm_suspend NULL
+#define jh7110_tdm_resume NULL
+#endif
+
+static const struct snd_soc_component_driver jh7110_tdm_component = {
+ .name = "jh7110-tdm",
+ .suspend = jh7110_tdm_suspend,
+ .resume = jh7110_tdm_resume,
+};
+
+static int jh7110_tdm_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ struct jh7110_tdm_dev *tdm = snd_soc_dai_get_drvdata(dai);
+ int chan_wl, chan_sl, chan_nr;
+ unsigned int data_width;
+ unsigned int dma_bus_width;
+ struct snd_dmaengine_dai_dma_data *dma_data = NULL;
+ struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+ struct snd_soc_dai_link *dai_link = rtd->dai_link;
+
+ dai_link->stop_dma_first = 1;
+
+ data_width = params_width(params);
+
+ tdm->samplerate = params_rate(params);
+ tdm->pcmclk = params_channels(params) * tdm->samplerate * data_width;
+
+ switch (params_format(params)) {
+ case SNDRV_PCM_FORMAT_S16_LE:
+ chan_wl = TDM_16BIT_WORD_LEN;
+ chan_sl = TDM_16BIT_SLOT_LEN;
+ dma_bus_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+ break;
+
+ case SNDRV_PCM_FORMAT_S32_LE:
+ chan_wl = TDM_32BIT_WORD_LEN;
+ chan_sl = TDM_32BIT_SLOT_LEN;
+ dma_bus_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+ break;
+
+ default:
+ dev_err(tdm->dev, "tdm: unsupported PCM fmt");
+ return -EINVAL;
+ }
+
+ chan_nr = params_channels(params);
+ switch (chan_nr) {
+ case ONE_CHANNEL_SUPPORT:
+ case TWO_CHANNEL_SUPPORT:
+ case FOUR_CHANNEL_SUPPORT:
+ case SIX_CHANNEL_SUPPORT:
+ case EIGHT_CHANNEL_SUPPORT:
+ break;
+ default:
+ dev_err(tdm->dev, "channel not supported\n");
+ return -EINVAL;
+ }
+
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+ tdm->tx.wl = chan_wl;
+ tdm->tx.sl = chan_sl;
+ tdm->tx.sscale = chan_nr;
+ tdm->play_dma_data.addr_width = dma_bus_width;
+ dma_data = &tdm->play_dma_data;
+ } else {
+ tdm->rx.wl = chan_wl;
+ tdm->rx.sl = chan_sl;
+ tdm->rx.sscale = chan_nr;
+ tdm->capture_dma_data.addr_width = dma_bus_width;
+ dma_data = &tdm->capture_dma_data;
+ }
+
+ snd_soc_dai_set_dma_data(dai, substream, dma_data);
+
+ jh7110_tdm_config(tdm, substream);
+ jh7110_tdm_save_context(tdm, substream);
+
+ return 0;
+}
+
+static int jh7110_tdm_trigger(struct snd_pcm_substream *substream,
+ int cmd, struct snd_soc_dai *dai)
+{
+ struct jh7110_tdm_dev *tdm = snd_soc_dai_get_drvdata(dai);
+ int ret = 0;
+
+ switch (cmd) {
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_RESUME:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ /* restore context */
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+ jh7110_tdm_writel(tdm, TDM_PCMTXCR, tdm->saved_pcmtxcr);
+ else
+ jh7110_tdm_writel(tdm, TDM_PCMRXCR, tdm->saved_pcmrxcr);
+
+ jh7110_tdm_start(tdm, substream);
+ break;
+
+ case SNDRV_PCM_TRIGGER_STOP:
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ jh7110_tdm_stop(tdm, substream);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ return ret;
+}
+
+static const struct snd_soc_dai_ops jh7110_tdm_dai_ops = {
+ .hw_params = jh7110_tdm_hw_params,
+ .trigger = jh7110_tdm_trigger,
+};
+
+static int jh7110_tdm_dai_probe(struct snd_soc_dai *dai)
+{
+ struct jh7110_tdm_dev *tdm = snd_soc_dai_get_drvdata(dai);
+
+ snd_soc_dai_init_dma_data(dai, &tdm->play_dma_data, &tdm->capture_dma_data);
+ snd_soc_dai_set_drvdata(dai, tdm);
+ return 0;
+}
+
+#define JH7110_TDM_RATES SNDRV_PCM_RATE_8000_48000
+
+#define JH7110_TDM_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
+ SNDRV_PCM_FMTBIT_S32_LE)
+
+static struct snd_soc_dai_driver jh7110_tdm_dai = {
+ .name = "sf_tdm",
+ .id = 0,
+ .playback = {
+ .stream_name = "Playback",
+ .channels_min = 1,
+ .channels_max = 8,
+ .rates = JH7110_TDM_RATES,
+ .formats = JH7110_TDM_FORMATS,
+ },
+ .capture = {
+ .stream_name = "Capture",
+ .channels_min = 1,
+ .channels_max = 8,
+ .rates = JH7110_TDM_RATES,
+ .formats = JH7110_TDM_FORMATS,
+ },
+ .ops = &jh7110_tdm_dai_ops,
+ .probe = jh7110_tdm_dai_probe,
+ .symmetric_rate = 1,
+};
+
+static const struct snd_pcm_hardware jh7110_pcm_hardware = {
+ .info = (SNDRV_PCM_INFO_MMAP |
+ SNDRV_PCM_INFO_MMAP_VALID |
+ SNDRV_PCM_INFO_PAUSE |
+ SNDRV_PCM_INFO_RESUME |
+ SNDRV_PCM_INFO_INTERLEAVED |
+ SNDRV_PCM_INFO_BLOCK_TRANSFER),
+ .buffer_bytes_max = 192512,
+ .period_bytes_min = 4096,
+ .period_bytes_max = 32768,
+ .periods_min = 1,
+ .periods_max = 48,
+ .fifo_size = 16,
+};
+
+static const struct snd_dmaengine_pcm_config jh7110_dmaengine_pcm_config = {
+ .pcm_hardware = &jh7110_pcm_hardware,
+ .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
+ .prealloc_buffer_size = 192512,
+};
+
+static void jh7110_tdm_init_params(struct jh7110_tdm_dev *tdm)
+{
+ tdm->clkpolity = TDM_TX_RASING_RX_FALLING;
+ if (tdm->frame_mode == SHORT_LATER) {
+ tdm->elm = TDM_ELM_LATE;
+ tdm->syncm = TDM_SYNCM_SHORT;
+ } else if (tdm->frame_mode == SHORT_EARLY) {
+ tdm->elm = TDM_ELM_EARLY;
+ tdm->syncm = TDM_SYNCM_SHORT;
+ } else {
+ tdm->elm = TDM_ELM_EARLY;
+ tdm->syncm = TDM_SYNCM_LONG;
+ }
+
+ tdm->ms_mode = TDM_AS_SLAVE;
+ tdm->rx.ifl = TDM_FIFO_HALF;
+ tdm->tx.ifl = TDM_FIFO_HALF;
+ tdm->rx.wl = TDM_16BIT_WORD_LEN;
+ tdm->tx.wl = TDM_16BIT_WORD_LEN;
+ tdm->rx.sscale = 2;
+ tdm->tx.sscale = 2;
+ tdm->rx.lrj = TDM_LEFT_JUSTIFT;
+ tdm->tx.lrj = TDM_LEFT_JUSTIFT;
+
+ tdm->play_dma_data.addr = TDM_FIFO;
+ tdm->play_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+ tdm->play_dma_data.fifo_size = TDM_FIFO_DEPTH / 2;
+ tdm->play_dma_data.maxburst = 16;
+
+ tdm->capture_dma_data.addr = TDM_FIFO;
+ tdm->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+ tdm->capture_dma_data.fifo_size = TDM_FIFO_DEPTH / 2;
+ tdm->capture_dma_data.maxburst = 8;
+}
+
+static int jh7110_tdm_clk_reset_init(struct platform_device *pdev,
+ struct jh7110_tdm_dev *tdm)
+{
+ int ret;
+
+ static struct clk_bulk_data clks[] = {
+ { .id = "tdm_ahb" },
+ { .id = "tdm_apb" },
+ { .id = "tdm_internal" },
+ { .id = "tdm" },
+ { .id = "mclk_inner" },
+ { .id = "tdm_ext" },
+ };
+
+ ret = devm_clk_bulk_get(&pdev->dev, ARRAY_SIZE(clks), clks);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to get tdm clocks\n");
+ return ret;
+ }
+
+ tdm->clk_tdm_ahb = clks[0].clk;
+ tdm->clk_tdm_apb = clks[1].clk;
+ tdm->clk_tdm_internal = clks[2].clk;
+ tdm->clk_tdm = clks[3].clk;
+ tdm->clk_mclk_inner = clks[4].clk;
+ tdm->clk_tdm_ext = clks[5].clk;
+
+ tdm->resets = devm_reset_control_array_get_exclusive(&pdev->dev);
+ if (IS_ERR(tdm->resets)) {
+ ret = PTR_ERR(tdm->resets);
+ dev_err(&pdev->dev, "Failed to get tdm resets");
+ return ret;
+ }
+
+ return jh7110_tdm_clk_enable(tdm);
+}
+
+static int jh7110_tdm_probe(struct platform_device *pdev)
+{
+ struct jh7110_tdm_dev *tdm;
+ int ret;
+
+ tdm = devm_kzalloc(&pdev->dev, sizeof(*tdm), GFP_KERNEL);
+ if (!tdm)
+ return -ENOMEM;
+
+ tdm->tdm_base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(tdm->tdm_base))
+ return PTR_ERR(tdm->tdm_base);
+
+ tdm->dev = &pdev->dev;
+
+ ret = jh7110_tdm_clk_reset_init(pdev, tdm);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to enable audio-tdm clock\n");
+ return ret;
+ }
+
+ tdm->frame_mode = SHORT_LATER;
+ jh7110_tdm_init_params(tdm);
+
+ dev_set_drvdata(&pdev->dev, tdm);
+ ret = devm_snd_soc_register_component(&pdev->dev, &jh7110_tdm_component,
+ &jh7110_tdm_dai, 1);
+ if (ret != 0) {
+ dev_err(&pdev->dev, "Failed to register dai\n");
+ return ret;
+ }
+
+ ret = devm_snd_dmaengine_pcm_register(&pdev->dev,
+ &jh7110_dmaengine_pcm_config,
+ SND_DMAENGINE_PCM_FLAG_COMPAT);
+ if (ret) {
+ dev_err(&pdev->dev, "Could not register pcm: %d\n", ret);
+ return ret;
+ }
+
+ pm_runtime_enable(&pdev->dev);
+#ifdef CONFIG_PM
+ jh7110_tdm_clk_disable(tdm);
+#endif
+
+ return 0;
+}
+
+static int jh7110_tdm_dev_remove(struct platform_device *pdev)
+{
+ pm_runtime_disable(&pdev->dev);
+ return 0;
+}
+
+static const struct of_device_id jh7110_tdm_of_match[] = {
+ {.compatible = "starfive,jh7110-tdm",},
+ {}
+};
+
+MODULE_DEVICE_TABLE(of, jh7110_tdm_of_match);
+
+static const struct dev_pm_ops jh7110_tdm_pm_ops = {
+ SET_RUNTIME_PM_OPS(jh7110_tdm_runtime_suspend,
+ jh7110_tdm_runtime_resume, NULL)
+};
+
+static struct platform_driver jh7110_tdm_driver = {
+ .driver = {
+ .name = "jh7110-tdm",
+ .of_match_table = jh7110_tdm_of_match,
+ .pm = &jh7110_tdm_pm_ops,
+ },
+ .probe = jh7110_tdm_probe,
+ .remove = jh7110_tdm_dev_remove,
+};
+module_platform_driver(jh7110_tdm_driver);
+
+MODULE_AUTHOR("Walker Chen <[email protected]>");
+MODULE_DESCRIPTION("StarFive JH7110 TDM ASoC Driver");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/starfive/jh7110_tdm.h b/sound/soc/starfive/jh7110_tdm.h
new file mode 100644
index 000000000000..aa4ab3624319
--- /dev/null
+++ b/sound/soc/starfive/jh7110_tdm.h
@@ -0,0 +1,155 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * TDM driver for the StarFive JH7110 SoC
+ *
+ * Copyright (C) 2023 StarFive Technology Co., Ltd.
+ */
+#ifndef __SND_SOC_STARFIVE_TDM_H
+#define __SND_SOC_STARFIVE_TDM_H
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/types.h>
+#include <sound/dmaengine_pcm.h>
+#include <sound/pcm.h>
+#include <linux/dmaengine.h>
+#include <linux/types.h>
+
+#define TDM_PCMGBCR 0x00
+ #define PCMGBCR_MASK 0x1e
+ #define PCMGBCR_ENABLE BIT(0)
+ #define PCMGBCR_TRITXEN BIT(4)
+ #define CLKPOL_BIT 5
+ #define TRITXEN_BIT 4
+ #define ELM_BIT 3
+ #define SYNCM_BIT 2
+ #define MS_BIT 1
+#define TDM_PCMTXCR 0x04
+ #define PCMTXCR_TXEN BIT(0)
+ #define IFL_BIT 11
+ #define WL_BIT 8
+ #define SSCALE_BIT 4
+ #define SL_BIT 2
+ #define LRJ_BIT 1
+#define TDM_PCMRXCR 0x08
+ #define PCMRXCR_RXEN BIT(0)
+ #define PCMRXCR_RXSL_MASK 0xc
+ #define PCMRXCR_RXSL_16BIT 0x4
+ #define PCMRXCR_RXSL_32BIT 0x8
+ #define PCMRXCR_SCALE_MASK 0xf0
+ #define PCMRXCR_SCALE_1CH 0x10
+#define TDM_PCMDIV 0x0c
+
+/* DMA registers */
+#define TDM_FIFO 0x170c0000
+#define TDM_FIFO_DEPTH 32
+
+#define ONE_CHANNEL_SUPPORT 1
+#define TWO_CHANNEL_SUPPORT 2
+#define FOUR_CHANNEL_SUPPORT 4
+#define SIX_CHANNEL_SUPPORT 6
+#define EIGHT_CHANNEL_SUPPORT 8
+
+enum TDM_MASTER_SLAVE_MODE {
+ TDM_AS_MASTER = 0,
+ TDM_AS_SLAVE,
+};
+
+enum TDM_CLKPOL {
+ /* tx raising and rx falling */
+ TDM_TX_RASING_RX_FALLING = 0,
+ /* tx falling and rx raising */
+ TDM_TX_FALLING_RX_RASING,
+};
+
+enum TDM_FRAME_MODE {
+ SHORT_EARLY = 0,
+ SHORT_LATER,
+ LONG,
+};
+
+enum TDM_ELM {
+ /* only work while SYNCM=0 */
+ TDM_ELM_LATE = 0,
+ TDM_ELM_EARLY,
+};
+
+enum TDM_SYNCM {
+ /* short frame sync */
+ TDM_SYNCM_SHORT = 0,
+ /* long frame sync */
+ TDM_SYNCM_LONG,
+};
+
+enum TDM_IFL {
+ /* FIFO to send or received : half-1/2, Quarter-1/4 */
+ TDM_FIFO_HALF = 0,
+ TDM_FIFO_QUARTER,
+};
+
+enum TDM_WL {
+ /* send or received word length */
+ TDM_8BIT_WORD_LEN = 0,
+ TDM_16BIT_WORD_LEN,
+ TDM_20BIT_WORD_LEN,
+ TDM_24BIT_WORD_LEN,
+ TDM_32BIT_WORD_LEN,
+};
+
+enum TDM_SL {
+ /* send or received slot length */
+ TDM_8BIT_SLOT_LEN = 0,
+ TDM_16BIT_SLOT_LEN,
+ TDM_32BIT_SLOT_LEN,
+};
+
+enum TDM_LRJ {
+ /* left-justify or right-justify */
+ TDM_RIGHT_JUSTIFY = 0,
+ TDM_LEFT_JUSTIFT,
+};
+
+struct tdm_chan_cfg {
+ enum TDM_IFL ifl;
+ enum TDM_WL wl;
+ unsigned char sscale;
+ enum TDM_SL sl;
+ enum TDM_LRJ lrj;
+ unsigned char enable;
+};
+
+struct jh7110_tdm_dev {
+ void __iomem *tdm_base;
+ struct device *dev;
+ struct clk *clk_tdm_ahb;
+ struct clk *clk_tdm_apb;
+ struct clk *clk_tdm_internal;
+ struct clk *clk_tdm;
+ struct clk *clk_mclk_inner;
+ struct clk *clk_tdm_ext;
+ struct reset_control *resets;
+
+ enum TDM_CLKPOL clkpolity;
+ enum TDM_ELM elm;
+ enum TDM_SYNCM syncm;
+ enum TDM_MASTER_SLAVE_MODE ms_mode;
+ enum TDM_FRAME_MODE frame_mode;
+
+ struct tdm_chan_cfg tx;
+ struct tdm_chan_cfg rx;
+
+ u16 syncdiv;
+ u32 samplerate;
+ u32 pcmclk;
+
+ /* data related to DMA transfers b/w tdm and DMAC */
+ struct snd_dmaengine_dai_dma_data play_dma_data;
+ struct snd_dmaengine_dai_dma_data capture_dma_data;
+ u32 saved_pcmgbcr;
+ u32 saved_pcmtxcr;
+ u32 saved_pcmrxcr;
+ u32 saved_pcmdiv;
+
+};
+
+#endif /* __SND_SOC_STARFIVE_TDM_H */
--
2.17.1
Add bindings to describe the audio board that works in conjunction with
the VisionFive2 board.
Signed-off-by: Walker Chen <[email protected]>
---
Documentation/devicetree/bindings/riscv/starfive.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/riscv/starfive.yaml b/Documentation/devicetree/bindings/riscv/starfive.yaml
index cc4d92f0a1bf..581d07718fde 100644
--- a/Documentation/devicetree/bindings/riscv/starfive.yaml
+++ b/Documentation/devicetree/bindings/riscv/starfive.yaml
@@ -28,6 +28,7 @@ properties:
- enum:
- starfive,visionfive-2-v1.2a
- starfive,visionfive-2-v1.3b
+ - starfive,visionfive-2-audio
- const: starfive,jh7110
additionalProperties: true
--
2.17.1
On Thu, Apr 20, 2023 at 10:41:17AM +0800, Walker Chen wrote:
> Add tdm driver support for the StarFive JH7110 SoC.
This is mostly fine, though the code all feels a bit messy somehow.
A lot of this is just coding style, I've highlighted a bunch of things
below. There's also a couple of more substantial issues.
> @@ -0,0 +1,579 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * TDM driver for the StarFive JH7110 SoC
> + *
> + * Copyright (C) 2023 StarFive Technology Co., Ltd.
Please make the entire comment a C++ one so things look more
intentional.
> +static int jh7110_tdm_syncdiv(struct jh7110_tdm_dev *tdm)
> +{
> + u32 sl, sscale, syncdiv;
> +
> + sl = (tdm->rx.sl >= tdm->tx.sl) ? tdm->rx.sl : tdm->tx.sl;
> + sscale = (tdm->rx.sscale >= tdm->tx.sscale) ? tdm->rx.sscale : tdm->tx.sscale;
Please write normal conditional statements to improve legibility.
> +static int jh7110_tdm_clk_enable(struct jh7110_tdm_dev *tdm)
> +{
> + ret = clk_set_parent(tdm->clk_tdm, tdm->clk_tdm_ext);
> + if (ret) {
> + dev_err(tdm->dev, "Can't set clock source for clk_tdm: %d\n",
> +ret);
> + goto dis_tdm_clk;
> + }
It's a bit weird to enable clocks and then reparent afterwards, that
seems more likely to run into issues with glitches doing something bad
than reparenting with the clock disabled.
This parenting looks like a system specific configuration (what if
the SoC is driving the audio bus?), and might be better done by using
the clock bindings. It's also strange that the driver is reparenting
every single time it enables the clocks rather than doing that once on
init.
> +static int jh7110_tdm_suspend(struct snd_soc_component *component)
> +{
> + return pm_runtime_force_suspend(component->dev);
> +}
> +
> +static int jh7110_tdm_resume(struct snd_soc_component *component)
> +{
> + struct jh7110_tdm_dev *tdm = snd_soc_component_get_drvdata(component);
> +
> + /* restore context */
> + jh7110_tdm_writel(tdm, TDM_PCMGBCR, tdm->saved_pcmgbcr);
> + jh7110_tdm_writel(tdm, TDM_PCMDIV, tdm->saved_pcmdiv);
> +
> + return pm_runtime_force_resume(component->dev);
> +}
It is weird that we restore context that we don't save on suspend, the
code *works* but it looks off.
> +static int jh7110_tdm_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{
> + struct jh7110_tdm_dev *tdm = snd_soc_dai_get_drvdata(dai);
> + int chan_wl, chan_sl, chan_nr;
> + unsigned int data_width;
> + unsigned int dma_bus_width;
> + struct snd_dmaengine_dai_dma_data *dma_data = NULL;
> + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> + struct snd_soc_dai_link *dai_link = rtd->dai_link;
> +
> + dai_link->stop_dma_first = 1;
A driver shouldn't be forcing dai_link settings, and hw_params is
claerly the wrong place to be configuring something like this which
never varies at runtime - it should be done on init(). If the DAI
really needs this you should extend the core so there's a flag in the
dai_driver which gets checked.
> + switch (chan_nr) {
> + case ONE_CHANNEL_SUPPORT:
> + case TWO_CHANNEL_SUPPORT:
> + case FOUR_CHANNEL_SUPPORT:
> + case SIX_CHANNEL_SUPPORT:
> + case EIGHT_CHANNEL_SUPPORT:
I am having a *really* hard time finding these definitions (which aren't
namespaced...) helpful. Just write the numbers directly.
> +static int jh7110_tdm_trigger(struct snd_pcm_substream *substream,
> + int cmd, struct snd_soc_dai *dai)
> +{
> + struct jh7110_tdm_dev *tdm = snd_soc_dai_get_drvdata(dai);
> + int ret = 0;
> +
> + switch (cmd) {
> + case SNDRV_PCM_TRIGGER_START:
> + case SNDRV_PCM_TRIGGER_RESUME:
> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> + /* restore context */
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + jh7110_tdm_writel(tdm, TDM_PCMTXCR, tdm->saved_pcmtxcr);
> + else
> + jh7110_tdm_writel(tdm, TDM_PCMRXCR, tdm->saved_pcmrxcr);
> +
> + jh7110_tdm_start(tdm, substream);
Why is the write to CR not part of start()?
> +static void jh7110_tdm_init_params(struct jh7110_tdm_dev *tdm)
> +{
> + tdm->clkpolity = TDM_TX_RASING_RX_FALLING;
> + if (tdm->frame_mode == SHORT_LATER) {
> + tdm->elm = TDM_ELM_LATE;
> + tdm->syncm = TDM_SYNCM_SHORT;
> + } else if (tdm->frame_mode == SHORT_EARLY) {
> + tdm->elm = TDM_ELM_EARLY;
> + tdm->syncm = TDM_SYNCM_SHORT;
> + } else {
> + tdm->elm = TDM_ELM_EARLY;
> + tdm->syncm = TDM_SYNCM_LONG;
> + }
This looks like it should be a switch statement, and the defintiions
namespaced. I can't see anyhwere where this ever gets configured to
anything other than SHORT_LATER ever being used so might be better to
just delete.
> + tdm->ms_mode = TDM_AS_SLAVE;
Please use the modern provider/consumer terminology for clocking.
> + tdm->clk_tdm_ahb = clks[0].clk;
> + tdm->clk_tdm_apb = clks[1].clk;
> + tdm->clk_tdm_internal = clks[2].clk;
> + tdm->clk_tdm = clks[3].clk;
> + tdm->clk_mclk_inner = clks[4].clk;
> + tdm->clk_tdm_ext = clks[5].clk;
Given that the driver only ever interacts with the clocks en masse is
there any point in having all the specific named variables, that'd mean
that the enable/disable could just use loops.
> +/* DMA registers */
> +#define TDM_FIFO 0x170c0000
> +#define TDM_FIFO_DEPTH 32
None of the defines in the header are namespaced and some of them (like
the above) seem generic enough to be likely to result in conflicts.
On 20/04/2023 04:41, Walker Chen wrote:
> Add bindings to describe the TDM driver for the StarFive JH7110 SoC.
>
> Signed-off-by: Walker Chen <[email protected]>
> ---
> .../bindings/sound/starfive,jh7110-tdm.yaml | 98 +++++++++++++++++++
> 1 file changed, 98 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml
>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 20/04/2023 04:41, Walker Chen wrote:
> Add bindings to describe the audio board that works in conjunction with
> the VisionFive2 board.
>
> Signed-off-by: Walker Chen <[email protected]>
> ---
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
Hey Walker,
On Thu, Apr 20, 2023 at 10:41:18AM +0800, Walker Chen wrote:
> Add the tdm controller node and sound card for the StarFive JH7110 SoC.
Is this one of these waveshare things + a visionfive 2?
https://www.waveshare.com/wm8960-audio-hat.htm
I'm a bit lost as to why this needs a whole new board, should it not
just be an overlay that you can apply to the existing dts?
Taking this to an extreme, should I expect to see a new devicetree for
everything RPi hat that you decide to use with a VisionFive 2?
Also, it'd be nice to provide a Link: to where someone can find more
info on this combination of items. Google for "wm8960 visionfive 2"
gives me nothing, nor does "starfive audio card" etc.
Thanks,
Conor.
> Reviewed-by: Hal Feng <[email protected]>
> Signed-off-by: Walker Chen <[email protected]>
> ---
> arch/riscv/boot/dts/starfive/Makefile | 1 +
> .../starfive/jh7110-starfive-audio-card.dts | 67 +++++++++++++++++++
> .../jh7110-starfive-visionfive-2.dtsi | 40 +++++++++++
> arch/riscv/boot/dts/starfive/jh7110.dtsi | 21 ++++++
> 4 files changed, 129 insertions(+)
> create mode 100644 arch/riscv/boot/dts/starfive/jh7110-starfive-audio-card.dts
>
> diff --git a/arch/riscv/boot/dts/starfive/Makefile b/arch/riscv/boot/dts/starfive/Makefile
> index 170956846d49..cb22cb7f66b0 100644
> --- a/arch/riscv/boot/dts/starfive/Makefile
> +++ b/arch/riscv/boot/dts/starfive/Makefile
> @@ -4,3 +4,4 @@ dtb-$(CONFIG_ARCH_STARFIVE) += jh7100-starfive-visionfive-v1.dtb
>
> dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-starfive-visionfive-2-v1.2a.dtb
> dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-starfive-visionfive-2-v1.3b.dtb
> +dtb-$(CONFIG_ARCH_STARFIVE) += jh7110-starfive-audio-card.dtb
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-audio-card.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-audio-card.dts
> new file mode 100644
> index 000000000000..967d9aa7d1e1
> --- /dev/null
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-audio-card.dts
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Device Tree for JH7110 + Simple Audio Card
> + *
> + * Copyright (C) 2023 StarFive Technology Co., Ltd.
> + */
> +
> +/dts-v1/;
> +#include "jh7110-starfive-visionfive-2-v1.3b.dts"
> +
> +/ {
> + model = "StarFive VisionFive 2 Audio Board";
> + compatible = "starfive,visionfive-2-audio", "starfive,jh7110";
> +
> + wm8960_mclk: wm8960-mclk {
> + compatible = "fixed-clock";
> + clock-output-names = "wm8960_mclk";
> + #clock-cells = <0>;
> + clock-frequency = <24576000>;
> + };
> +
> + sound {
> + compatible = "simple-audio-card";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + simple-audio-card,name = "Starfive-TDM-Sound-Card";
> + simple-audio-card,widgets = "Microphone", "Mic Jack",
> + "Line", "Line In",
> + "Line", "Line Out",
> + "Speaker", "Speaker",
> + "Headphone", "Headphone Jack";
> + simple-audio-card,routing = "Headphone Jack", "HP_L",
> + "Headphone Jack", "HP_R",
> + "Speaker", "SPK_LP",
> + "Speaker", "SPK_LN",
> + "LINPUT1", "Mic Jack",
> + "LINPUT3", "Mic Jack",
> + "RINPUT1", "Mic Jack",
> + "RINPUT2", "Mic Jack";
> +
> + simple-audio-card,dai-link@0 {
> + reg = <0>;
> + format = "dsp_a";
> + bitclock-master = <&dailink_master>;
> + frame-master = <&dailink_master>;
> +
> + cpu {
> + sound-dai = <&tdm>;
> + };
> + dailink_master: codec {
> + sound-dai = <&wm8960>;
> + clocks = <&wm8960_mclk>;
> + };
> + };
> + };
> +};
> +
> +&i2c0 {
> + wm8960: codec@1a {
> + compatible = "wlf,wm8960";
> + reg = <0x1a>;
> + wlf,shared-lrclk;
> + #sound-dai-cells = <0>;
> + };
> +};
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> index 1155b97b593d..19b5954ee72d 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> @@ -214,6 +214,40 @@
> slew-rate = <0>;
> };
> };
> +
> + tdm0_pins: tdm0-pins {
> + tdm0-pins-tx {
> + pinmux = <GPIOMUX(44, GPOUT_SYS_TDM_TXD,
> + GPOEN_ENABLE,
> + GPI_NONE)>;
> + bias-pull-up;
> + drive-strength = <2>;
> + input-disable;
> + input-schmitt-disable;
> + slew-rate = <0>;
> + };
> +
> + tdm0-pins-rx {
> + pinmux = <GPIOMUX(61, GPOUT_HIGH,
> + GPOEN_DISABLE,
> + GPI_SYS_TDM_RXD)>;
> + input-enable;
> + };
> +
> + tdm0-pins-sync {
> + pinmux = <GPIOMUX(63, GPOUT_HIGH,
> + GPOEN_DISABLE,
> + GPI_SYS_TDM_SYNC)>;
> + input-enable;
> + };
> +
> + tdm0-pins-pcmclk {
> + pinmux = <GPIOMUX(38, GPOUT_HIGH,
> + GPOEN_DISABLE,
> + GPI_SYS_TDM_CLK)>;
> + input-enable;
> + };
> + };
> };
>
> &uart0 {
> @@ -221,3 +255,9 @@
> pinctrl-0 = <&uart0_pins>;
> status = "okay";
> };
> +
> +&tdm {
> + pinctrl-names = "default";
> + pinctrl-0 = <&tdm0_pins>;
> + status = "okay";
> +};
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index 866313570a7e..cfda6fb0d91b 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -366,6 +366,27 @@
> status = "disabled";
> };
>
> + tdm: tdm@10090000 {
> + compatible = "starfive,jh7110-tdm";
> + reg = <0x0 0x10090000 0x0 0x1000>;
> + clocks = <&syscrg JH7110_SYSCLK_TDM_AHB>,
> + <&syscrg JH7110_SYSCLK_TDM_APB>,
> + <&syscrg JH7110_SYSCLK_TDM_INTERNAL>,
> + <&syscrg JH7110_SYSCLK_TDM_TDM>,
> + <&syscrg JH7110_SYSCLK_MCLK_INNER>,
> + <&tdm_ext>;
> + clock-names = "tdm_ahb", "tdm_apb",
> + "tdm_internal", "tdm",
> + "mclk_inner", "tdm_ext";
> + resets = <&syscrg JH7110_SYSRST_TDM_AHB>,
> + <&syscrg JH7110_SYSRST_TDM_APB>,
> + <&syscrg JH7110_SYSRST_TDM_CORE>;
> + dmas = <&dma 20>, <&dma 21>;
> + dma-names = "rx","tx";
> + #sound-dai-cells = <0>;
> + status = "disabled";
> + };
> +
> stgcrg: clock-controller@10230000 {
> compatible = "starfive,jh7110-stgcrg";
> reg = <0x0 0x10230000 0x0 0x10000>;
> --
> 2.17.1
>
On 2023/4/22 0:50, Conor Dooley wrote:
> Hey Walker,
>
> On Thu, Apr 20, 2023 at 10:41:18AM +0800, Walker Chen wrote:
>> Add the tdm controller node and sound card for the StarFive JH7110 SoC.
>
> Is this one of these waveshare things + a visionfive 2?
> https://www.waveshare.com/wm8960-audio-hat.htm
Hey Conor,
I'm glad to receive your comments.
Now we are using this board + VisionFive2 :
https://wiki.seeedstudio.com/ReSpeaker_2_Mics_Pi_HAT/
>
> I'm a bit lost as to why this needs a whole new board, should it not
> just be an overlay that you can apply to the existing dts?
>
> Taking this to an extreme, should I expect to see a new devicetree for
> everything RPi hat that you decide to use with a VisionFive 2?
For your response, I did some thinking. Because wm8960 codec is not integrated
on the VisionFive2 board, perhaps using overlay is a better way.
>
> Also, it'd be nice to provide a Link: to where someone can find more
> info on this combination of items. Google for "wm8960 visionfive 2"
> gives me nothing, nor does "starfive audio card" etc.
>
> Thanks,
> Conor.
>
>> Reviewed-by: Hal Feng <[email protected]>
>> Signed-off-by: Walker Chen <[email protected]>
>> ---
Best regards,
Walker
On Mon, Apr 24, 2023 at 11:21:11AM +0800, Walker Chen wrote:
>
>
> On 2023/4/22 0:50, Conor Dooley wrote:
> > Hey Walker,
> >
> > On Thu, Apr 20, 2023 at 10:41:18AM +0800, Walker Chen wrote:
> >> Add the tdm controller node and sound card for the StarFive JH7110 SoC.
> >
> > Is this one of these waveshare things + a visionfive 2?
> > https://www.waveshare.com/wm8960-audio-hat.htm
>
> Hey Conor,
> I'm glad to receive your comments.
>
> Now we are using this board + VisionFive2 :
> https://wiki.seeedstudio.com/ReSpeaker_2_Mics_Pi_HAT/
>
> >
> > I'm a bit lost as to why this needs a whole new board, should it not
> > just be an overlay that you can apply to the existing dts?
> >
> > Taking this to an extreme, should I expect to see a new devicetree for
> > everything RPi hat that you decide to use with a VisionFive 2?
>
> For your response, I did some thinking. Because wm8960 codec is not integrated
> on the VisionFive2 board, perhaps using overlay is a better way.
Aye. I think so too. From my PoV, if this particular seeed audio board
is something you're bundling with VisionFive 2 boards on your storefront
etc, then I'm fine with taking it as an in-tree overlay.
If it is just a "random" RPi hat (that happens to be exactly what you
need for testing the audio drivers), then I don't know where to draw a
line for what is & what is not acceptable for inclusion.
In both cases, it's Emil's call.
Cheers,
Conor.
> > Also, it'd be nice to provide a Link: to where someone can find more
> > info on this combination of items. Google for "wm8960 visionfive 2"
> > gives me nothing, nor does "starfive audio card" etc.
On Mon, 24 Apr 2023 at 18:27, Conor Dooley <[email protected]> wrote:
> On Mon, Apr 24, 2023 at 11:21:11AM +0800, Walker Chen wrote:
> >
> >
> > On 2023/4/22 0:50, Conor Dooley wrote:
> > > Hey Walker,
> > >
> > > On Thu, Apr 20, 2023 at 10:41:18AM +0800, Walker Chen wrote:
> > >> Add the tdm controller node and sound card for the StarFive JH7110 SoC.
> > >
> > > Is this one of these waveshare things + a visionfive 2?
> > > https://www.waveshare.com/wm8960-audio-hat.htm
> >
> > Hey Conor,
> > I'm glad to receive your comments.
> >
> > Now we are using this board + VisionFive2 :
> > https://wiki.seeedstudio.com/ReSpeaker_2_Mics_Pi_HAT/
> >
> > >
> > > I'm a bit lost as to why this needs a whole new board, should it not
> > > just be an overlay that you can apply to the existing dts?
> > >
> > > Taking this to an extreme, should I expect to see a new devicetree for
> > > everything RPi hat that you decide to use with a VisionFive 2?
> >
> > For your response, I did some thinking. Because wm8960 codec is not integrated
> > on the VisionFive2 board, perhaps using overlay is a better way.
>
> Aye. I think so too. From my PoV, if this particular seeed audio board
> is something you're bundling with VisionFive 2 boards on your storefront
> etc, then I'm fine with taking it as an in-tree overlay.
> If it is just a "random" RPi hat (that happens to be exactly what you
> need for testing the audio drivers), then I don't know where to draw a
> line for what is & what is not acceptable for inclusion.
>
> In both cases, it's Emil's call.
I'm not aware of any shop bundling the VF2 with an audio board. I
agree: please don't add device trees for combinations of VF2s and
hats. That should be an overlay.
/Emil
> > > Also, it'd be nice to provide a Link: to where someone can find more
> > > info on this combination of items. Google for "wm8960 visionfive 2"
> > > gives me nothing, nor does "starfive audio card" etc.
>
On 2023/4/26 4:57, Emil Renner Berthing wrote:
> On Mon, 24 Apr 2023 at 18:27, Conor Dooley <[email protected]> wrote:
>> On Mon, Apr 24, 2023 at 11:21:11AM +0800, Walker Chen wrote:
>> >
>> >
>> > On 2023/4/22 0:50, Conor Dooley wrote:
>> > > Hey Walker,
>> > >
>> > > On Thu, Apr 20, 2023 at 10:41:18AM +0800, Walker Chen wrote:
>> > >> Add the tdm controller node and sound card for the StarFive JH7110 SoC.
>> > >
>> > > Is this one of these waveshare things + a visionfive 2?
>> > > https://www.waveshare.com/wm8960-audio-hat.htm
>> >
>> > Hey Conor,
>> > I'm glad to receive your comments.
>> >
>> > Now we are using this board + VisionFive2 :
>> > https://wiki.seeedstudio.com/ReSpeaker_2_Mics_Pi_HAT/
>> >
>> > >
>> > > I'm a bit lost as to why this needs a whole new board, should it not
>> > > just be an overlay that you can apply to the existing dts?
>> > >
>> > > Taking this to an extreme, should I expect to see a new devicetree for
>> > > everything RPi hat that you decide to use with a VisionFive 2?
>> >
>> > For your response, I did some thinking. Because wm8960 codec is not integrated
>> > on the VisionFive2 board, perhaps using overlay is a better way.
>>
>> Aye. I think so too. From my PoV, if this particular seeed audio board
>> is something you're bundling with VisionFive 2 boards on your storefront
>> etc, then I'm fine with taking it as an in-tree overlay.
>> If it is just a "random" RPi hat (that happens to be exactly what you
>> need for testing the audio drivers), then I don't know where to draw a
>> line for what is & what is not acceptable for inclusion.
>>
>> In both cases, it's Emil's call.
>
> I'm not aware of any shop bundling the VF2 with an audio board. I
> agree: please don't add device trees for combinations of VF2s and
> hats. That should be an overlay.
>
> /Emil
Since you guys agree, so let's do it this way.
Thanks.
Best regards,
Walker
On 2023/4/20 22:30, Mark Brown wrote:
> On Thu, Apr 20, 2023 at 10:41:17AM +0800, Walker Chen wrote:
>> Add tdm driver support for the StarFive JH7110 SoC.
>
> This is mostly fine, though the code all feels a bit messy somehow.
> A lot of this is just coding style, I've highlighted a bunch of things
> below. There's also a couple of more substantial issues.
Hey Mark,
Firstly thanks for your patient review and detailed comments.
>
>> @@ -0,0 +1,579 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * TDM driver for the StarFive JH7110 SoC
>> + *
>> + * Copyright (C) 2023 StarFive Technology Co., Ltd.
>
> Please make the entire comment a C++ one so things look more
> intentional.
OK, can reference to other platform's format.
>
>> +static int jh7110_tdm_syncdiv(struct jh7110_tdm_dev *tdm)
>> +{
>> + u32 sl, sscale, syncdiv;
>> +
>> + sl = (tdm->rx.sl >= tdm->tx.sl) ? tdm->rx.sl : tdm->tx.sl;
>> + sscale = (tdm->rx.sscale >= tdm->tx.sscale) ? tdm->rx.sscale : tdm->tx.sscale;
>
> Please write normal conditional statements to improve legibility.
Will be modified.
>
>> +static int jh7110_tdm_clk_enable(struct jh7110_tdm_dev *tdm)
>> +{
>
>> + ret = clk_set_parent(tdm->clk_tdm, tdm->clk_tdm_ext);
>> + if (ret) {
>> + dev_err(tdm->dev, "Can't set clock source for clk_tdm: %d\n",
>> +ret);
>> + goto dis_tdm_clk;
>> + }
>
> It's a bit weird to enable clocks and then reparent afterwards, that
> seems more likely to run into issues with glitches doing something bad
> than reparenting with the clock disabled.
This TDM module ultimately uses an external clock. It firstly must uses internal clock
before being enabled, and then is switched to external clock, otherwise failed to reset.
This limitation is determined by the chip.
>
> This parenting looks like a system specific configuration (what if
> the SoC is driving the audio bus?), and might be better done by using
> the clock bindings. It's also strange that the driver is reparenting
> every single time it enables the clocks rather than doing that once on
> init.
To save power consumption, need to disable clock in suspend() and enable clock in
resume(). As mentioned above, the internal clock must be selected before enabling
clock every time, and then switch to external clock.
>
>> +static int jh7110_tdm_suspend(struct snd_soc_component *component)
>> +{
>> + return pm_runtime_force_suspend(component->dev);
>> +}
>> +
>> +static int jh7110_tdm_resume(struct snd_soc_component *component)
>> +{
>> + struct jh7110_tdm_dev *tdm = snd_soc_component_get_drvdata(component);
>> +
>> + /* restore context */
>> + jh7110_tdm_writel(tdm, TDM_PCMGBCR, tdm->saved_pcmgbcr);
>> + jh7110_tdm_writel(tdm, TDM_PCMDIV, tdm->saved_pcmdiv);
>> +
>> + return pm_runtime_force_resume(component->dev);
>> +}
>
> It is weird that we restore context that we don't save on suspend, the
> code *works* but it looks off.
Should be pairing operation in suspend() and resume().
>
>> +static int jh7110_tdm_hw_params(struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params,
>> + struct snd_soc_dai *dai)
>> +{
>> + struct jh7110_tdm_dev *tdm = snd_soc_dai_get_drvdata(dai);
>> + int chan_wl, chan_sl, chan_nr;
>> + unsigned int data_width;
>> + unsigned int dma_bus_width;
>> + struct snd_dmaengine_dai_dma_data *dma_data = NULL;
>> + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
>> + struct snd_soc_dai_link *dai_link = rtd->dai_link;
>> +
>> + dai_link->stop_dma_first = 1;
>
> A driver shouldn't be forcing dai_link settings, and hw_params is
> claerly the wrong place to be configuring something like this which
> never varies at runtime - it should be done on init(). If the DAI
> really needs this you should extend the core so there's a flag in the
> dai_driver which gets checked.
Yes, should be done at startup of dai_driver, doing that once on initialize stage.
>
>> + switch (chan_nr) {
>> + case ONE_CHANNEL_SUPPORT:
>> + case TWO_CHANNEL_SUPPORT:
>> + case FOUR_CHANNEL_SUPPORT:
>> + case SIX_CHANNEL_SUPPORT:
>> + case EIGHT_CHANNEL_SUPPORT:
>
> I am having a *really* hard time finding these definitions (which aren't
> namespaced...) helpful. Just write the numbers directly.
OK, will be changed.
>
>> +static int jh7110_tdm_trigger(struct snd_pcm_substream *substream,
>> + int cmd, struct snd_soc_dai *dai)
>> +{
>> + struct jh7110_tdm_dev *tdm = snd_soc_dai_get_drvdata(dai);
>> + int ret = 0;
>> +
>> + switch (cmd) {
>> + case SNDRV_PCM_TRIGGER_START:
>> + case SNDRV_PCM_TRIGGER_RESUME:
>> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> + /* restore context */
>> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> + jh7110_tdm_writel(tdm, TDM_PCMTXCR, tdm->saved_pcmtxcr);
>> + else
>> + jh7110_tdm_writel(tdm, TDM_PCMRXCR, tdm->saved_pcmrxcr);
>> +
>> + jh7110_tdm_start(tdm, substream);
>
> Why is the write to CR not part of start()?
OK, will be changed.
>
>> +static void jh7110_tdm_init_params(struct jh7110_tdm_dev *tdm)
>> +{
>> + tdm->clkpolity = TDM_TX_RASING_RX_FALLING;
>> + if (tdm->frame_mode == SHORT_LATER) {
>> + tdm->elm = TDM_ELM_LATE;
>> + tdm->syncm = TDM_SYNCM_SHORT;
>> + } else if (tdm->frame_mode == SHORT_EARLY) {
>> + tdm->elm = TDM_ELM_EARLY;
>> + tdm->syncm = TDM_SYNCM_SHORT;
>> + } else {
>> + tdm->elm = TDM_ELM_EARLY;
>> + tdm->syncm = TDM_SYNCM_LONG;
>> + }
>
> This looks like it should be a switch statement, and the defintiions
> namespaced. I can't see anyhwere where this ever gets configured to
> anything other than SHORT_LATER ever being used so might be better to
> just delete.
Will be modified according to your suggestion.
>
>> + tdm->ms_mode = TDM_AS_SLAVE;
>
> Please use the modern provider/consumer terminology for clocking.
>
>> + tdm->clk_tdm_ahb = clks[0].clk;
>> + tdm->clk_tdm_apb = clks[1].clk;
>> + tdm->clk_tdm_internal = clks[2].clk;
>> + tdm->clk_tdm = clks[3].clk;
>> + tdm->clk_mclk_inner = clks[4].clk;
>> + tdm->clk_tdm_ext = clks[5].clk;
>
> Given that the driver only ever interacts with the clocks en masse is
> there any point in having all the specific named variables, that'd mean
> that the enable/disable could just use loops.
Will be changed.
>
>> +/* DMA registers */
>> +#define TDM_FIFO 0x170c0000
>> +#define TDM_FIFO_DEPTH 32
>
> None of the defines in the header are namespaced and some of them (like
> the above) seem generic enough to be likely to result in conflicts.
Will add unified JH7110_TDM_ prefix.
Thank you very much for your suggestion.
Best regards,
Walker