Add TDM audio on StarFive JH7110

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

Add bindings to describe the audio board that works in conjunction with the VisionFive2 board.

Signed-off-by Walker Chen <[email protected]>

---

Add tdm driver support for the StarFive JH7110 SoC.

sound/soc/starfive/jh7110_tdm.c new file +5 -0

+static int jh7110_tdm_suspend(struct snd_soc_component *component)
+{
+	return pm_runtime_force_suspend(component->dev);
+}
+

sound/soc/starfive/jh7110_tdm.c new file +7 -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) {

Quoted 1 line · Walker Chen

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.

sound/soc/starfive/jh7110_tdm.c line 5

Quoted 7 lines

sound/soc/starfive/jh7110_tdm.c new file +5 -0

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

sound/soc/starfive/jh7110_tdm.c line 81

Quoted 7 lines

sound/soc/starfive/jh7110_tdm.c new file +6 -0

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

sound/soc/starfive/jh7110_tdm.c line 198

Quoted 10 lines

sound/soc/starfive/jh7110_tdm.c new file +8 -0

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

Quoted 11 lines

sound/soc/starfive/jh7110_tdm.c new file +10 -0

+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 */

sound/soc/starfive/jh7110_tdm.c line 250

Quoted 5 lines

sound/soc/starfive/jh7110_tdm.c new file +5 -0

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

Quoted 12 lines

sound/soc/starfive/jh7110_tdm.c new file +11 -0

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

sound/soc/starfive/jh7110_tdm.c line 275

Quoted 2 lines

sound/soc/starfive/jh7110_tdm.c new file +2 -0

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

sound/soc/starfive/jh7110_tdm.c line 306

Quoted 7 lines

sound/soc/starfive/jh7110_tdm.c new file +6 -0

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

Quoted 12 lines

sound/soc/starfive/jh7110_tdm.c new file +11 -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 */

sound/soc/starfive/jh7110_tdm.c line 351

Quoted 6 lines

sound/soc/starfive/jh7110_tdm.c new file +6 -0

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

Quoted 11 lines

sound/soc/starfive/jh7110_tdm.c new file +10 -0

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

sound/soc/starfive/jh7110_tdm.c line 440

Quoted 3 lines

sound/soc/starfive/jh7110_tdm.c new file +3 -0

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

sound/soc/starfive/jh7110_tdm.c line 442

Quoted 2 lines

sound/soc/starfive/jh7110_tdm.c new file +1 -0

+	tdm->ms_mode = TDM_AS_SLAVE;

Please use the modern provider/consumer terminology for clocking.

sound/soc/starfive/jh7110_tdm.c line 488

Quoted 6 lines

sound/soc/starfive/jh7110_tdm.c new file +6 -0

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

sound/soc/starfive/jh7110_tdm.h line 45

Quoted 4 lines

sound/soc/starfive/jh7110_tdm.h new file +3 -0

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

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

arch/riscv/boot/dts/starfive/Makefile +1 -0

index 170956846d49..cb22cb7f66b0 100644
@@ -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

arch/riscv/boot/dts/starfive/jh7110-starfive-audio-card.dts new file +67 -0

new file mode 100644
index 000000000000..967d9aa7d1e1
@@ -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>;
+	};
+};

arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi +40 -0

index 1155b97b593d..19b5954ee72d 100644
@@ -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";
+};

arch/riscv/boot/dts/starfive/jh7110.dtsi +21 -0

index 866313570a7e..cfda6fb0d91b 100644
@@ -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>;

Signature · 1 line

2.17.1

Hey Walker,

Quoted 1 line · Walker Chen

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.

Quoted 3 lines

Reviewed-by Hal Feng <[email protected]>

Signed-off-by Walker Chen <[email protected]>

---

Quoted 7 lines · Conor Dooley

Hey Walker,

Quoted 1 line · Walker Chen

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/

Quoted 1 line

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 18 lines · Walker Chen

Quoted 7 lines · Conor Dooley

Hey Walker,

Quoted 1 line · Walker Chen

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/

Quoted 1 line

Quoted 8 lines

Quoted 5 lines

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.

Quoted 3 lines

Quoted 3 lines

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.

Quoted 35 lines · Conor Dooley

Quoted 18 lines · Walker Chen

Quoted 7 lines · Conor Dooley

Hey Walker,

Quoted 1 line · Walker Chen

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/

Quoted 1 line

Quoted 8 lines

Quoted 5 lines

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.

Quoted 1 line

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