[PATCH v6 00/11] ASoC: renesas: fsi: Fix system hang by adding SPU clock

[PATCH v6 00/11] ASoC: renesas: fsi: Fix system hang by adding SPU clock

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Hi all,

The FSI on r8a7740 requires the SPU clock to be enabled before accessing
its internal registers. Without it, register accesses may hang the system
even when the FSI functional clock is enabled.

Previously, the SPU clock remained enabled because it was left running by
the bootloader. After adding the SPU clock to the device tree, it is
automatically disabled once system initialization completes.

This series adds the missing clocks and aligns their names with those used
by the driver.

Following feedback from Morimoto-san, the driver is also refactored to
improve stability. Clock initialization is moved from the runtime path to
the probe function to simplify the flow and avoid redundant setup.
Additionally, the shutdown sequence is reordered to ensure the stream is
stopped before the hardware is shut down.

The driver currently uses clk_enable()/clk_disable() without matching
clk_prepare()/clk_unprepare() handling. This series adds the missing
prepare/unprepare operations and moves them into startup/shutdown paths,
since clk_prepare() may sleep and therefore must not be called from 
atomic contexts.

The series also fixes a race where in-flight IRQ handlers may continue
accessing registers after the SPU clock has been disabled during shutdown.


Changes in v6:
 - Add Acked-by tag from Kuninori Morimoto.
 - DT binding updates (drop uniqueItems, commit message).
 - Improve probe and clock error handling.
 - Small improvements in clock handling paths.

Changes in v5:
 - Drop spu_count and rely on the clk core for clock reference counting.
 - Reorder the patch series as suggested by Morimoto-san.
 - Update the probe error handling path to ensure proper cleanup on
   failure.

Changes in v4:
 - use fsi_stream_is_working() for Fixed a race where in-flight IRQ 
   handlers following Morimoto-san's suggestions
 - Handle the return value of fsi_clk_init() to properly support deferred 
   probe, as suggested by Mark.
 - Split the clock refactoring into a devm cleanup patch and a refactor 
   patch, as suggested by Morimoto-san.
 - Update dt-bindings based on feedback from Krzysztof, Rob, and Geert.

Changes in v3:
 - Reordered the patches following Morimoto-san's suggestions
 - Updated the DT bindings based on Geert's feedback and renamed the
   "own" clock to "fck"
 - Added fsi_clk_prepare()/fsi_clk_unprepare() and moved them into
   dai_startup()/dai_shutdown()
 - Fixed a race where in-flight IRQ handlers could continue accessing
   registers after the SPU clock had been disabled

Changes in v2:
 - DT Bindings:
   Define "own" clock and add "spu", "icka/b", "diva/b", "xcka/b" to the 
   clock tree.
   Use YAML anchors and "if" rules to enforce clock-names and r8a7740 
   requirements.
   Relocate allOf block and update example with full 8-clock configuration.

 - DTS:
   Rename "fsi" clock to "own" to match driver implementation.
   Add missing clock names: "icka", "ickb", "diva", "divb", "xcka", "xckb".
 
 - In the driver:
   Refactor clock initialization.
   Reorder shutdown: stop stream before hardware shutdown.
   Move SPU clock enable/disable handling to fsi_hw_startup/shutdown.

v5 links:
   https://lore.kernel.org/all/20260609013107.5995-1-phucduc.bui@gmail.com/
v4 links:
   https://lore.kernel.org/all/20260605121955.105661-1-phucduc.bui@gmail.com/
v3 links:
   https://lore.kernel.org/all/20260510084303.122426-1-phucduc.bui@gmail.com/
v2 links: 
   https://lore.kernel.org/all/20260413100700.30995-1-phucduc.bui@gmail.com/
v1 links : 
   https://lore.kernel.org/all/20260403112655.167593-1-phucduc.bui@gmail.com/

Testing:
  - Verified on r8a7740 (Armadillo-800EVA): FSI slave / Codec master mode.
    The system no longer hangs. aplay works correctly, while arecord has 
    some noise in the recorded file (this likely needs further tuning, but
    it is not part of this patch series). 
  - FSI master mode is currently compile-tested only. Full verification
    requires a dedicated HDMI driver (FSIB) or hardware modifications 
    (resoldering board resistors) (FSIA).
  - Youtube video link of the test process (from v3 verification):
    https://youtu.be/w3H4v5djr7M

Best regards,
Phuc


bui duc phuc (11):
  ASoC: dt-bindings: renesas,fsi: add support multiple clocks
  ARM: dts: renesas: r8a7740: Add clocks for FSI
  ASoC: renesas: fsi: Fix trigger stop ordering
  ASoC: renesas: fsi: Move fsi_stream_is_working()
  ASoC: renesas: fsi: Fix register access from in-flight IRQ after
    shutdown
  ASoC: renesas: fsi: Move fsi_clk_init()
  ASoC: renesas: fsi: Use devm_clk_get_optional() for optional clocks
  ASoC: renesas: fsi: refactor clock initialization
  ASoC: renesas: fsi: Add SPU clock support
  ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()
  ASoC: renesas: fsi: Add SPU clock control in hw_startup/shutdown

 .../bindings/sound/renesas,fsi.yaml           |  60 +++-
 arch/arm/boot/dts/renesas/r8a7740.dtsi        |  12 +-
 sound/soc/renesas/fsi.c                       | 267 ++++++++++++------
 3 files changed, 244 insertions(+), 95 deletions(-)

-- 
2.43.0

[PATCH v6 01/11] ASoC: dt-bindings: renesas,fsi: add support multiple clocks

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

The FSI on r8a7740 requires the SPU bus/bridge clock to be enabled before
accessing its registers. Without this clock, any register access leads to
a system hang as the FSI block sits behind the SPU bus.
Update the binding to support multiple clocks to properly describe the
hardware clock tree, including:
  - SPU bus/bridge clock (spu) for register access.
  - CPG DIV6 clocks (icka/b) as functional clock.
  - FSI dividers (diva/b) for audio clock generation.
  - External clock inputs (xcka/b) provided by the board.
The hardware supports several valid clock configurations. For example,
when both FSIA and FSIB operate as slaves, only the fck and spu clocks
are required. When a port operates as a master, it can use either an
internal clock source (ickx + divx) or an external clock source
(ickx + xckx). Therefore, while fck and spu are mandatory on r8a7740,
the remaining clocks (icka/b, diva/b and xcka/b) are optional and depend
on the selected master/slave configuration and clock source.
Both sh73a0 and r8a7740 define the SPU DIV6 clock control register at
0xe6150084. The binding therefore documents the clocks supported by the
FSI driver for these variants.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v6:
 - DT binding updates (drop uniqueItems, commit message) based on 
   Krzysztof's feedback.
Changes in v4:
 - Update dt-bindings based on feedback from Krzysztof, Rob, and Geert.


 .../bindings/sound/renesas,fsi.yaml           | 60 +++++++++++++++++--
 1 file changed, 55 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
index df91991699a7..803945b7f82f 100644
--- a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
+++ b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
@@ -9,9 +9,6 @@ title: Renesas FIFO-buffered Serial Interface (FSI)
 maintainers:
   - Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
 
-allOf:
-  - $ref: dai-common.yaml#
-
 properties:
   $nodename:
     pattern: "^sound@.*"
@@ -38,7 +35,32 @@ properties:
     maxItems: 1
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: Main FSI module clock
+      - description: |
+          SPU bus/bridge clock. On R8A7740, this clock must be enabled to allow
+          register access as the FSI block is connected behind the SPU bus.
+      - description: CPG DIV6 functional clocks for FSI port A
+      - description: CPG DIV6 functional clocks for FSI port B
+      - description: FSI dividers for port A used for audio clock generation
+      - description: FSI dividers for port B used for audio clock generation
+      - description: External clock inputs for FSI port A provided by the board
+      - description: External clock inputs for FSI port B provided by the board
+
+  clock-names:
+    minItems: 1
+    maxItems: 8
+    items:
+      enum:
+        - fck  # Main FSI module clock
+        - spu  # optional SPU bus/bridge clock
+        - icka # optional CPG DIV6 functional clocks for FSI port A
+        - ickb # optional CPG DIV6 functional clocks for FSI port B
+        - diva # optional FSI dividers for port A used for audio clock generation
+        - divb # optional FSI dividers for port B used for audio clock generation
+        - xcka # optional External clock inputs for FSI port A provided by the board
+        - xckb # optional External clock inputs for FSI port B provided by the board
 
   power-domains:
     maxItems: 1
@@ -69,6 +91,30 @@ required:
 
 unevaluatedProperties: false
 
+allOf:
+  - $ref: dai-common.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,fsi2-r8a7740
+    then:
+      required:
+        - clock-names
+
+      properties:
+        clock-names:
+          minItems: 2
+          items:
+            - const: fck
+            - const: spu
+            - enum: [icka, ickb, diva, divb, xcka, xckb]
+            - enum: [icka, ickb, diva, divb, xcka, xckb]
+            - enum: [icka, ickb, diva, divb, xcka, xckb]
+            - enum: [icka, ickb, diva, divb, xcka, xckb]
+            - enum: [icka, ickb, diva, divb, xcka, xckb]
+            - enum: [icka, ickb, diva, divb, xcka, xckb]
+
 examples:
   - |
     #include <dt-bindings/clock/r8a7740-clock.h>
@@ -77,7 +123,11 @@ examples:
             compatible = "renesas,fsi2-r8a7740", "renesas,sh_fsi2";
             reg = <0xfe1f0000 0x400>;
             interrupts = <GIC_SPI 9 0x4>;
-            clocks = <&mstp3_clks R8A7740_CLK_FSI>;
+            clocks = <&mstp3_clks R8A7740_CLK_FSI>, <&spu_clk>,
+                    <&fsia_clk>, <&fsiack_clk>, <&fsidiva_clk>,
+                    <&fsib_clk>, <&fsibck_clk>, <&fsidivb_clk>;
+            clock-names = "fck", "spu", "icka", "xcka", "diva",
+                         "ickb", "xckb", "divb";
             power-domains = <&pd_a4mp>;
 
             #sound-dai-cells = <1>;
-- 
2.43.0

[PATCH v6 02/11] ARM: dts: renesas: r8a7740: Add clocks for FSI

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Add the SPU bus clock, icka/b functional clocks, and xcka/b external
clock inputs to the FSI device node.
This prepares for subsequent driver changes that explicitly manage the
SPU clock required for FSI register access on the r8a7740.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 arch/arm/boot/dts/renesas/r8a7740.dtsi | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/renesas/r8a7740.dtsi b/arch/arm/boot/dts/renesas/r8a7740.dtsi
index d13ab86c3ab4..6f9d9bbfd159 100644
--- a/arch/arm/boot/dts/renesas/r8a7740.dtsi
+++ b/arch/arm/boot/dts/renesas/r8a7740.dtsi
@@ -393,7 +393,11 @@ sh_fsi2: sound@fe1f0000 {
 		compatible = "renesas,fsi2-r8a7740", "renesas,sh_fsi2";
 		reg = <0xfe1f0000 0x400>;
 		interrupts = <GIC_SPI 9 0x4>;
-		clocks = <&mstp3_clks R8A7740_CLK_FSI>;
+		clocks = <&mstp3_clks R8A7740_CLK_FSI>, <&spu_clk>,
+			<&fsia_clk>, <&fsib_clk>, <&fsiack_clk>,
+			<&fsibck_clk>;
+		clock-names = "fck", "spu", "icka", "ickb", "xcka",
+				"xckb";
 		power-domains = <&pd_a4mp>;
 		status = "disabled";
 	};
@@ -614,6 +618,12 @@ vou_clk: vou@e6150088 {
 				 <0>;
 			#clock-cells = <0>;
 		};
+		fsib_clk: fsib@e6150090 {
+			compatible = "renesas,r8a7740-div6-clock", "renesas,cpg-div6-clock";
+			reg = <0xe6150090 4>;
+			clocks = <&pllc1_div2_clk>, <&fsibck_clk>, <0>, <0>;
+			#clock-cells = <0>;
+		};
 		stpro_clk: stpro@e615009c {
 			compatible = "renesas,r8a7740-div6-clock", "renesas,cpg-div6-clock";
 			reg = <0xe615009c 4>;
-- 
2.43.0

[PATCH v6 03/11] ASoC: renesas: fsi: Fix trigger stop ordering

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Call fsi_stream_stop() before fsi_hw_shutdown(). This matches the existing
order in the suspend path.
This change ensures all register accesses during stream shutdown are fully
completed before disabling the clocks.

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v6:
 - Add Acked-by tag from Kuninori Morimoto.
Changes in v5:
 - Move fsi_hw_shutdown() after fsi_stream_quit() to prepare for
   subsequent patches that use fsi_stream_is_working() to handle
   in-flight IRQ handlers.
Changes in v4:
 - update commit messages 

 sound/soc/renesas/fsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 8cbd7acc26f4..800ac40f9680 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -1586,10 +1586,10 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 			ret = fsi_stream_transfer(io);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
-		if (!ret)
-			ret = fsi_hw_shutdown(fsi, dai->dev);
 		fsi_stream_stop(fsi, io);
 		fsi_stream_quit(fsi, io);
+		if (!ret)
+			ret = fsi_hw_shutdown(fsi, dai->dev);
 		break;
 	}
 
-- 
2.43.0

[PATCH v6 04/11] ASoC: renesas: fsi: Move fsi_stream_is_working()

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Move fsi_stream_is_working() before fsi_count_fifo_err().
This prepares for a subsequent patch that needs to check stream status
when handling in-flight IRQ handlers. No functional changwqes intended.

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v6:
 - Add Acked-by tag from Kuninori Morimoto.

 sound/soc/renesas/fsi.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 800ac40f9680..0bd0e0c8c5a3 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -442,6 +442,16 @@ static int fsi_sample2frame(struct fsi_priv *fsi, int samples)
 	return samples / fsi->chan_num;
 }
 
+static int fsi_stream_is_working(struct fsi_priv *fsi,
+				 struct fsi_stream *io)
+{
+	struct fsi_master *master = fsi_get_master(fsi);
+
+	guard(spinlock_irqsave)(&master->lock);
+
+	return !!(io->substream && io->substream->runtime);
+}
+
 static int fsi_get_current_fifo_samples(struct fsi_priv *fsi,
 					struct fsi_stream *io)
 {
@@ -488,16 +498,6 @@ static inline struct fsi_stream *fsi_stream_get(struct fsi_priv *fsi,
 	return fsi_is_play(substream) ? &fsi->playback : &fsi->capture;
 }
 
-static int fsi_stream_is_working(struct fsi_priv *fsi,
-				 struct fsi_stream *io)
-{
-	struct fsi_master *master = fsi_get_master(fsi);
-
-	guard(spinlock_irqsave)(&master->lock);
-
-	return !!(io->substream && io->substream->runtime);
-}
-
 static struct fsi_priv *fsi_stream_to_priv(struct fsi_stream *io)
 {
 	return io->priv;
-- 
2.43.0

[PATCH v6 05/11] ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

In-flight IRQs may still be running when the SPU clock is disabled,
leading to register access after shutdown and causing system hangs.

Fix this to use fsi_stream_is_working() when handling in-flight IRQ
handlers. If no streams are active, the handler now returns immediately
to prevent hardware access.

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v6:
 - Add Acked-by tag from Kuninori Morimoto.
Changes in v4:
 - use fsi_stream_is_working instead of running_streams.

 sound/soc/renesas/fsi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 0bd0e0c8c5a3..3e3c6fd7c56b 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -470,6 +470,10 @@ static int fsi_get_current_fifo_samples(struct fsi_priv *fsi,
 
 static void fsi_count_fifo_err(struct fsi_priv *fsi)
 {
+	if (!fsi_stream_is_working(fsi, &fsi->playback) &&
+	    !fsi_stream_is_working(fsi, &fsi->capture))
+		return;
+
 	u32 ostatus = fsi_reg_read(fsi, DOFF_ST);
 	u32 istatus = fsi_reg_read(fsi, DIFF_ST);
 
@@ -681,6 +685,10 @@ static void fsi_irq_clear_status(struct fsi_priv *fsi)
 	u32 data = 0;
 	struct fsi_master *master = fsi_get_master(fsi);
 
+	if (!fsi_stream_is_working(fsi, &fsi->playback) &&
+	    !fsi_stream_is_working(fsi, &fsi->capture))
+		return;
+
 	data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->playback));
 	data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->capture));
 
-- 
2.43.0

[PATCH v6 06/11] ASoC: renesas: fsi: Move fsi_clk_init()

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Move fsi_clk_init() after set_rate() functions to prepare for subsequent
refactoring.

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v6:
 - Add Acked-by tag from Kuninori Morimoto.

 sound/soc/renesas/fsi.c | 128 ++++++++++++++++++++--------------------
 1 file changed, 64 insertions(+), 64 deletions(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 3e3c6fd7c56b..1df7dc014363 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -717,70 +717,6 @@ static void fsi_spdif_clk_ctrl(struct fsi_priv *fsi, int enable)
 /*
  *		clock function
  */
-static int fsi_clk_init(struct device *dev,
-			struct fsi_priv *fsi,
-			int xck,
-			int ick,
-			int div,
-			int (*set_rate)(struct device *dev,
-					struct fsi_priv *fsi))
-{
-	struct fsi_clk *clock = &fsi->clock;
-	int is_porta = fsi_is_port_a(fsi);
-
-	clock->xck	= NULL;
-	clock->ick	= NULL;
-	clock->div	= NULL;
-	clock->rate	= 0;
-	clock->count	= 0;
-	clock->set_rate	= set_rate;
-
-	clock->own = devm_clk_get(dev, NULL);
-	if (IS_ERR(clock->own))
-		return -EINVAL;
-
-	/* external clock */
-	if (xck) {
-		clock->xck = devm_clk_get(dev, is_porta ? "xcka" : "xckb");
-		if (IS_ERR(clock->xck)) {
-			dev_err(dev, "can't get xck clock\n");
-			return -EINVAL;
-		}
-		if (clock->xck == clock->own) {
-			dev_err(dev, "cpu doesn't support xck clock\n");
-			return -EINVAL;
-		}
-	}
-
-	/* FSIACLK/FSIBCLK */
-	if (ick) {
-		clock->ick = devm_clk_get(dev,  is_porta ? "icka" : "ickb");
-		if (IS_ERR(clock->ick)) {
-			dev_err(dev, "can't get ick clock\n");
-			return -EINVAL;
-		}
-		if (clock->ick == clock->own) {
-			dev_err(dev, "cpu doesn't support ick clock\n");
-			return -EINVAL;
-		}
-	}
-
-	/* FSI-DIV */
-	if (div) {
-		clock->div = devm_clk_get(dev,  is_porta ? "diva" : "divb");
-		if (IS_ERR(clock->div)) {
-			dev_err(dev, "can't get div clock\n");
-			return -EINVAL;
-		}
-		if (clock->div == clock->own) {
-			dev_err(dev, "cpu doesn't support div clock\n");
-			return -EINVAL;
-		}
-	}
-
-	return 0;
-}
-
 #define fsi_clk_invalid(fsi) fsi_clk_valid(fsi, 0)
 static void fsi_clk_valid(struct fsi_priv *fsi, unsigned long rate)
 {
@@ -1034,6 +970,70 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
 	return ret;
 }
 
+static int fsi_clk_init(struct device *dev,
+			struct fsi_priv *fsi,
+			int xck,
+			int ick,
+			int div,
+			int (*set_rate)(struct device *dev,
+					struct fsi_priv *fsi))
+{
+	struct fsi_clk *clock = &fsi->clock;
+	int is_porta = fsi_is_port_a(fsi);
+
+	clock->xck	= NULL;
+	clock->ick	= NULL;
+	clock->div	= NULL;
+	clock->rate	= 0;
+	clock->count	= 0;
+	clock->set_rate	= set_rate;
+
+	clock->own = devm_clk_get(dev, NULL);
+	if (IS_ERR(clock->own))
+		return -EINVAL;
+
+	/* external clock */
+	if (xck) {
+		clock->xck = devm_clk_get(dev, is_porta ? "xcka" : "xckb");
+		if (IS_ERR(clock->xck)) {
+			dev_err(dev, "can't get xck clock\n");
+			return -EINVAL;
+		}
+		if (clock->xck == clock->own) {
+			dev_err(dev, "cpu doesn't support xck clock\n");
+			return -EINVAL;
+		}
+	}
+
+	/* FSIACLK/FSIBCLK */
+	if (ick) {
+		clock->ick = devm_clk_get(dev,  is_porta ? "icka" : "ickb");
+		if (IS_ERR(clock->ick)) {
+			dev_err(dev, "can't get ick clock\n");
+			return -EINVAL;
+		}
+		if (clock->ick == clock->own) {
+			dev_err(dev, "cpu doesn't support ick clock\n");
+			return -EINVAL;
+		}
+	}
+
+	/* FSI-DIV */
+	if (div) {
+		clock->div = devm_clk_get(dev,  is_porta ? "diva" : "divb");
+		if (IS_ERR(clock->div)) {
+			dev_err(dev, "can't get div clock\n");
+			return -EINVAL;
+		}
+		if (clock->div == clock->own) {
+			dev_err(dev, "cpu doesn't support div clock\n");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 static void fsi_pointer_update(struct fsi_stream *io, int size)
 {
 	io->buff_sample_pos += size;
-- 
2.43.0

[PATCH v6 07/11] ASoC: renesas: fsi: Use devm_clk_get_optional() for optional clocks

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

The xck, ick, and div clocks are optional. Switch from devm_clk_get()
to devm_clk_get_optional() to correctly handle cases where these clocks
are missing.

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v6:
 - Add Acked-by tag from Kuninori Morimoto.

 sound/soc/renesas/fsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 1df7dc014363..e29be2bcf952 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -994,7 +994,7 @@ static int fsi_clk_init(struct device *dev,
 
 	/* external clock */
 	if (xck) {
-		clock->xck = devm_clk_get(dev, is_porta ? "xcka" : "xckb");
+		clock->xck = devm_clk_get_optional(dev, is_porta ? "xcka" : "xckb");
 		if (IS_ERR(clock->xck)) {
 			dev_err(dev, "can't get xck clock\n");
 			return -EINVAL;
@@ -1007,7 +1007,7 @@ static int fsi_clk_init(struct device *dev,
 
 	/* FSIACLK/FSIBCLK */
 	if (ick) {
-		clock->ick = devm_clk_get(dev,  is_porta ? "icka" : "ickb");
+		clock->ick = devm_clk_get_optional(dev,  is_porta ? "icka" : "ickb");
 		if (IS_ERR(clock->ick)) {
 			dev_err(dev, "can't get ick clock\n");
 			return -EINVAL;
@@ -1020,7 +1020,7 @@ static int fsi_clk_init(struct device *dev,
 
 	/* FSI-DIV */
 	if (div) {
-		clock->div = devm_clk_get(dev,  is_porta ? "diva" : "divb");
+		clock->div = devm_clk_get_optional(dev,  is_porta ? "diva" : "divb");
 		if (IS_ERR(clock->div)) {
 			dev_err(dev, "can't get div clock\n");
 			return -EINVAL;
-- 
2.43.0

[PATCH v6 08/11] ASoC: renesas: fsi: refactor clock initialization

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Move fsi_clk_init() from set_fmt() to the probe path.
This ensures that clock resources are acquired only once during device
initialization, instead of being looked up repeatedly whenever set_fmt()
is called.
Together with the previous conversion to devm_clk_get_optional(), the
driver can now probe successfully even when optional clocks are absent.
The set_rate() callbacks continue to validate that all required clocks
are available before applying hardware-specific configuration.

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v6:
 - Add Acked-by tag from Kuninori Morimoto.
 - Improve probe and clock error handling.
Changes in v5:
 - Reorder the patches 
 - update the error handling path to ensure proper cleanup by Sashiko 
Changes in v4:
 - Handle the return value of fsi_clk_init() to properly support deferred 
   probe, as suggested by Mark.

 sound/soc/renesas/fsi.c | 67 +++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 30 deletions(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index e29be2bcf952..43bc77ebcca3 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -862,6 +862,11 @@ static int fsi_clk_set_rate_external(struct device *dev,
 	int ackmd, bpfmd;
 	int ret = 0;
 
+	if (!xck || !ick) {
+		dev_err(dev, "xck clock or ick clock is missing\n");
+		return -EINVAL;
+	}
+
 	/* check clock rate */
 	xrate = clk_get_rate(xck);
 	if (xrate % rate) {
@@ -898,6 +903,11 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
 	int ackmd, bpfmd;
 	int ret = -EINVAL;
 
+	if (!ick || !div) {
+		dev_err(dev, "ick clock or div clock is missing\n");
+		return -EINVAL;
+	}
+
 	if (!(12288000 % rate))
 		target = 12288000;
 	if (!(11289600 % rate))
@@ -970,35 +980,35 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
 	return ret;
 }
 
-static int fsi_clk_init(struct device *dev,
-			struct fsi_priv *fsi,
-			int xck,
-			int ick,
-			int div,
-			int (*set_rate)(struct device *dev,
-					struct fsi_priv *fsi))
+static int fsi_clk_init(struct device *dev, struct fsi_priv *fsi)
 {
 	struct fsi_clk *clock = &fsi->clock;
 	int is_porta = fsi_is_port_a(fsi);
+	int xck, ick, div;
+
+	if (fsi->clk_cpg) {
+		xck = 0; ick = 1; div = 1;
+		clock->set_rate = fsi_clk_set_rate_cpg;
+	} else {
+		xck = 1; ick = 1; div = 0;
+		clock->set_rate = fsi_clk_set_rate_external;
+	}
 
 	clock->xck	= NULL;
 	clock->ick	= NULL;
 	clock->div	= NULL;
 	clock->rate	= 0;
 	clock->count	= 0;
-	clock->set_rate	= set_rate;
 
 	clock->own = devm_clk_get(dev, NULL);
 	if (IS_ERR(clock->own))
-		return -EINVAL;
+		return dev_err_probe(dev, PTR_ERR(clock->own), "Can't get fck clock\n");
 
 	/* external clock */
 	if (xck) {
 		clock->xck = devm_clk_get_optional(dev, is_porta ? "xcka" : "xckb");
-		if (IS_ERR(clock->xck)) {
-			dev_err(dev, "can't get xck clock\n");
-			return -EINVAL;
-		}
+		if (IS_ERR(clock->xck))
+			return dev_err_probe(dev, PTR_ERR(clock->xck), "Can't get xck clock\n");
 		if (clock->xck == clock->own) {
 			dev_err(dev, "cpu doesn't support xck clock\n");
 			return -EINVAL;
@@ -1008,10 +1018,8 @@ static int fsi_clk_init(struct device *dev,
 	/* FSIACLK/FSIBCLK */
 	if (ick) {
 		clock->ick = devm_clk_get_optional(dev,  is_porta ? "icka" : "ickb");
-		if (IS_ERR(clock->ick)) {
-			dev_err(dev, "can't get ick clock\n");
-			return -EINVAL;
-		}
+		if (IS_ERR(clock->ick))
+			return dev_err_probe(dev, PTR_ERR(clock->ick), "Can't get ick clock\n");
 		if (clock->ick == clock->own) {
 			dev_err(dev, "cpu doesn't support ick clock\n");
 			return -EINVAL;
@@ -1021,10 +1029,8 @@ static int fsi_clk_init(struct device *dev,
 	/* FSI-DIV */
 	if (div) {
 		clock->div = devm_clk_get_optional(dev,  is_porta ? "diva" : "divb");
-		if (IS_ERR(clock->div)) {
-			dev_err(dev, "can't get div clock\n");
-			return -EINVAL;
-		}
+		if (IS_ERR(clock->div))
+			return dev_err_probe(dev, PTR_ERR(clock->div), "Can't get div clock\n");
 		if (clock->div == clock->own) {
 			dev_err(dev, "cpu doesn't support div clock\n");
 			return -EINVAL;
@@ -1672,15 +1678,6 @@ static int fsi_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		break;
 	}
 
-	if (fsi_is_clk_master(fsi)) {
-		if (fsi->clk_cpg)
-			fsi_clk_init(dai->dev, fsi, 0, 1, 1,
-				     fsi_clk_set_rate_cpg);
-		else
-			fsi_clk_init(dai->dev, fsi, 1, 1, 0,
-				     fsi_clk_set_rate_external);
-	}
-
 	/* set format */
 	if (fsi_is_spdif(fsi))
 		ret = fsi_set_fmt_spdif(fsi);
@@ -1978,6 +1975,11 @@ static int fsi_probe(struct platform_device *pdev)
 	fsi->master	= master;
 	fsi_port_info_init(fsi, &info.port_a);
 	fsi_handler_init(fsi, &info.port_a);
+	ret = fsi_clk_init(&pdev->dev, fsi);
+	if (ret) {
+		dev_err(&pdev->dev, "FSIA clk init failed\n");
+		return ret;
+	}
 	ret = fsi_stream_probe(fsi, &pdev->dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "FSIA stream probe failed\n");
@@ -1991,6 +1993,11 @@ static int fsi_probe(struct platform_device *pdev)
 	fsi->master	= master;
 	fsi_port_info_init(fsi, &info.port_b);
 	fsi_handler_init(fsi, &info.port_b);
+	ret = fsi_clk_init(&pdev->dev, fsi);
+	if (ret) {
+		dev_err(&pdev->dev, "FSIB clk init failed\n");
+		goto exit_fsia;
+	}
 	ret = fsi_stream_probe(fsi, &pdev->dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "FSIB stream probe failed\n");
-- 
2.43.0

[PATCH v6 09/11] ASoC: renesas: fsi: Add SPU clock support

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

FSI register accesses on the r8a7740 require the SPU bus clock to be
enabled. Add support for acquiring and managing the SPU clock via the
device tree to ensure proper register access.

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v6:
 - Add Acked-by tag from Kuninori Morimoto.

 sound/soc/renesas/fsi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 43bc77ebcca3..716ecf0401fe 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -292,6 +292,7 @@ struct fsi_master {
 	void __iomem *base;
 	struct fsi_priv fsia;
 	struct fsi_priv fsib;
+	struct clk *clk_spu;
 	const struct fsi_core *core;
 	spinlock_t lock;
 };
@@ -983,6 +984,7 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
 static int fsi_clk_init(struct device *dev, struct fsi_priv *fsi)
 {
 	struct fsi_clk *clock = &fsi->clock;
+	struct fsi_master *master = fsi->master;
 	int is_porta = fsi_is_port_a(fsi);
 	int xck, ick, div;
 
@@ -1004,6 +1006,13 @@ static int fsi_clk_init(struct device *dev, struct fsi_priv *fsi)
 	if (IS_ERR(clock->own))
 		return dev_err_probe(dev, PTR_ERR(clock->own), "Can't get fck clock\n");
 
+	if (!master->clk_spu) {
+		master->clk_spu = devm_clk_get_optional(dev, "spu");
+		if (IS_ERR(master->clk_spu))
+			return dev_err_probe(dev, PTR_ERR(master->clk_spu),
+					"Can't get spu clock\n");
+	}
+
 	/* external clock */
 	if (xck) {
 		clock->xck = devm_clk_get_optional(dev, is_porta ? "xcka" : "xckb");
-- 
2.43.0

[PATCH v6 10/11] ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Add fsi_clk_prepare() and fsi_clk_unprepare() helpers and call them
from fsi_dai_startup() and fsi_dai_shutdown().
This ensures clk_prepare() and clk_unprepare() are executed from
sleepable contexts and keeps clocks prepared only while audio streams
are active.

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
Changes in v6:
 - Add Acked-by tag from Kuninori Morimoto.
Changes in v5:
 - Drop count & spu_count and rely on the clk core for clock reference
   counting.
Changes in v4:
 - Move clock->count early return check to the beginning of 
   fsi_clk_[un]prepare() to simplify the code.

 sound/soc/renesas/fsi.c | 51 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 716ecf0401fe..e26f39dfe059 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -730,6 +730,54 @@ static int fsi_clk_is_valid(struct fsi_priv *fsi)
 		fsi->clock.rate;
 }
 
+static int fsi_clk_prepare(struct fsi_priv *fsi)
+{
+	struct fsi_clk *clock = &fsi->clock;
+	struct clk *spu = fsi->master->clk_spu;
+	struct clk *xck = clock->xck;
+	struct clk *ick = clock->ick;
+	struct clk *div = clock->div;
+	int ret;
+
+	ret = clk_prepare(spu);
+	if (ret)
+		return ret;
+	ret = clk_prepare(xck);
+	if (ret)
+		goto err_spu;
+	ret = clk_prepare(ick);
+	if (ret)
+		goto err_xck;
+	ret = clk_prepare(div);
+	if (ret)
+		goto err_ick;
+
+	return 0;
+
+err_ick:
+	clk_unprepare(ick);
+err_xck:
+	clk_unprepare(xck);
+err_spu:
+	clk_unprepare(spu);
+
+	return ret;
+}
+
+static void fsi_clk_unprepare(struct fsi_priv *fsi)
+{
+	struct fsi_clk *clock = &fsi->clock;
+	struct clk *spu = fsi->master->clk_spu;
+	struct clk *xck = clock->xck;
+	struct clk *ick = clock->ick;
+	struct clk *div = clock->div;
+
+	clk_unprepare(div);
+	clk_unprepare(ick);
+	clk_unprepare(xck);
+	clk_unprepare(spu);
+}
+
 static int fsi_clk_enable(struct device *dev,
 			  struct fsi_priv *fsi)
 {
@@ -1580,7 +1628,7 @@ static int fsi_dai_startup(struct snd_pcm_substream *substream,
 
 	fsi_clk_invalid(fsi);
 
-	return 0;
+	return fsi_clk_prepare(fsi);
 }
 
 static void fsi_dai_shutdown(struct snd_pcm_substream *substream,
@@ -1588,6 +1636,7 @@ static void fsi_dai_shutdown(struct snd_pcm_substream *substream,
 {
 	struct fsi_priv *fsi = fsi_get_priv(substream);
 
+	fsi_clk_unprepare(fsi);
 	fsi_clk_invalid(fsi);
 }
 
-- 
2.43.0

[PATCH v6 11/11] ASoC: renesas: fsi: Add SPU clock control in hw_startup/shutdown

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Enable and disable the SPU clock in fsi_hw_startup() and
fsi_hw_shutdown() to ensure the clock is active while the
driver accesses hardware registers.

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
Changes in v6:
 - Add Acked-by tag from Kuninori Morimoto.
 - Minor refactor in clock enable/disable paths.
Changes in v5:
 - Drop spu_count and rely on the clk core for clock reference
   counting.
 sound/soc/renesas/fsi.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index e26f39dfe059..b72396b5de7d 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -1560,6 +1560,11 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
 			  struct device *dev)
 {
 	u32 data = 0;
+	int ret;
+	/* enable spu bus bridge clock */
+	ret = clk_enable(fsi->master->clk_spu);
+	if (ret)
+		return ret;
 
 	/* clock setting */
 	if (fsi_is_clk_master(fsi))
@@ -1605,8 +1610,13 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
 	fsi_fifo_init(fsi, io, dev);
 
 	/* start master clock */
-	if (fsi_is_clk_master(fsi))
-		return fsi_clk_enable(dev, fsi);
+	if (fsi_is_clk_master(fsi)) {
+		ret = fsi_clk_enable(dev, fsi);
+		if (ret) {
+			clk_disable(fsi->master->clk_spu);
+			return ret;
+		}
+	}
 
 	return 0;
 }
@@ -1614,9 +1624,15 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
 static int fsi_hw_shutdown(struct fsi_priv *fsi,
 			    struct device *dev)
 {
+	int ret;
 	/* stop master clock */
-	if (fsi_is_clk_master(fsi))
-		return fsi_clk_disable(dev, fsi);
+	if (fsi_is_clk_master(fsi)) {
+		ret = fsi_clk_disable(dev, fsi);
+		if (ret)
+			return ret;
+	}
+	/* stop spu bus bridge clock */
+	clk_disable(fsi->master->clk_spu);
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH v6 01/11] ASoC: dt-bindings: renesas,fsi: add support multiple clocks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing schema constraints for `renesas,fsi2-sh73a0`.
- [Medium] Device tree schema mismatch between positional `clocks` descriptions and flexible `clock-names`.
--

commit f87e156c63c67f6faedd026ab0b5578543349e89
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: dt-bindings: renesas,fsi: add support multiple clocks

This commit updates the FSI binding to support multiple clocks to properly
describe the hardware clock tree, including the SPU bus/bridge clock and
various CPG DIV6 functional clocks. It documents the clocks supported by
the FSI driver for the sh73a0 and r8a7740 variants.

> diff --git a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> index df91991699a72..803945b7f82f0 100644
> --- a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> +++ b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> @@ -9,9 +9,6 @@ title: Renesas FIFO-buffered Serial Interface (FSI)
>  maintainers:
>    - Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>  
> -allOf:
> -  - $ref: dai-common.yaml#
> -
>  properties:
>    $nodename:
>      pattern: "^sound@.*"

[ ... ]

> @@ -38,7 +35,32 @@ properties:
>      maxItems: 1
>  
>    clocks:
> -    maxItems: 1
> +    minItems: 1
> +    items:
> +      - description: Main FSI module clock
> +      - description: |
> +          SPU bus/bridge clock. On R8A7740, this clock must be enabled to allow
> +          register access as the FSI block is connected behind the SPU bus.
> +      - description: CPG DIV6 functional clocks for FSI port A
> +      - description: CPG DIV6 functional clocks for FSI port B
> +      - description: FSI dividers for port A used for audio clock generation
> +      - description: FSI dividers for port B used for audio clock generation
> +      - description: External clock inputs for FSI port A provided by the board
> +      - description: External clock inputs for FSI port B provided by the board
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 8
> +    items:
> +      enum:
> +        - fck  # Main FSI module clock
> +        - spu  # optional SPU bus/bridge clock
> +        - icka # optional CPG DIV6 functional clocks for FSI port A
> +        - ickb # optional CPG DIV6 functional clocks for FSI port B
> +        - diva # optional FSI dividers for port A used for audio clock generation
> +        - divb # optional FSI dividers for port B used for audio clock generation
> +        - xcka # optional External clock inputs for FSI port A provided by the board
> +        - xckb # optional External clock inputs for FSI port B provided by the board

[ ... ]

> @@ -69,6 +91,30 @@ required:
>  
>  unevaluatedProperties: false
>  
> +allOf:
> +  - $ref: dai-common.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: renesas,fsi2-r8a7740

[Severity: Medium]
The commit message states that both the sh73a0 and r8a7740 variants define
the SPU DIV6 clock control register, but is the renesas,fsi2-sh73a0
compatible string missing from this conditional block?

If the condition restricts the clock validation rules entirely to
renesas,fsi2-r8a7740, the sh73a0 variant would be left without any
enforcement of the required fck and spu clocks, allowing improperly
configured device trees to silently pass schema validation but fail
at runtime.

> +    then:
> +      required:
> +        - clock-names
> +
> +      properties:
> +        clock-names:
> +          minItems: 2
> +          items:
> +            - const: fck
> +            - const: spu
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]
> +            - enum: [icka, ickb, diva, divb, xcka, xckb]

[Severity: Medium]
Does the use of an unordered enum list for clock-names create a structural
mismatch with the strictly positional descriptions defined in the clocks
array above?

Because dt-schema correlates array properties by index, a valid unordered
list of optional clocks might be mismatched with incorrect descriptions.
Additionally, defining sequential items with identical enum lists permits
invalid configurations with duplicate clock entries to pass validation.

> +
>  examples:
>    - |
>      #include <dt-bindings/clock/r8a7740-clock.h>
> @@ -77,7 +123,11 @@ examples:
>              compatible = "renesas,fsi2-r8a7740", "renesas,sh_fsi2";
>              reg = <0xfe1f0000 0x400>;
>              interrupts = <GIC_SPI 9 0x4>;
> -            clocks = <&mstp3_clks R8A7740_CLK_FSI>;
> +            clocks = <&mstp3_clks R8A7740_CLK_FSI>, <&spu_clk>,
> +                    <&fsia_clk>, <&fsiack_clk>, <&fsidiva_clk>,
> +                    <&fsib_clk>, <&fsibck_clk>, <&fsidivb_clk>;
> +            clock-names = "fck", "spu", "icka", "xcka", "diva",
> +                         "ickb", "xckb", "divb";

For instance, this example places xcka at index 3. Would this violate the
strict positional schema since index 3 in the clocks array is explicitly
described as "CPG DIV6 functional clocks for FSI port B"?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609113836.45079-1-phucduc.bui@gmail.com?part=1

Re: [PATCH v6 04/11] ASoC: renesas: fsi: Move fsi_stream_is_working()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Time-of-check to time-of-use (TOCTOU) race condition between fsi_stream_is_working() and subsequent lockless pointer access in fsi_pio_get_area(), leading to a potential NULL pointer dereference.
--

commit 8fa524754ea1b8f1ac9ac2d6e00b052f212f3a41
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: Move fsi_stream_is_working()
    
Move fsi_stream_is_working() before fsi_count_fifo_err(). This prepares for
a subsequent patch that needs to check stream status when handling in-flight
IRQ handlers.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 800ac40f9680d..0bd0e0c8c5a30 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -442,6 +442,16 @@ static int fsi_sample2frame(struct fsi_priv *fsi, int samples)
>  	return samples / fsi->chan_num;
>  }
>  
> +static int fsi_stream_is_working(struct fsi_priv *fsi,
> +				 struct fsi_stream *io)
> +{
> +	struct fsi_master *master = fsi_get_master(fsi);
> +
> +	guard(spinlock_irqsave)(&master->lock);
> +
> +	return !!(io->substream && io->substream->runtime);
> +}

[Severity: High]
This is a pre-existing issue, but does fsi_stream_is_working() leave callers
vulnerable to a time-of-check to time-of-use (TOCTOU) race condition?

The lock is acquired and then immediately released, protecting only the
condition check but not the caller's subsequent pointer usage. For example,
in fsi_pio_transfer(), the stream status is checked securely but the lock is
dropped, followed immediately by a lockless pointer access:

	if (!fsi_stream_is_working(fsi, io))
		return -EINVAL;

	buf = fsi_pio_get_area(fsi, io);

Concurrently, an ALSA stream stop triggered from userspace could call
fsi_stream_quit() on another CPU, which acquires the lock and sets
io->substream to NULL:

	guard(spinlock_irqsave)(&master->lock);
	...
	io->substream = NULL;

If fsi_stream_quit() acquires the lock immediately after
fsi_stream_is_working() releases it, the pointer would be nullified just
before the IRQ handler continues execution.

The IRQ handler would then proceed to call fsi_pio_get_area(), which
locklessly dereferences io->substream->runtime:

static u8 *fsi_pio_get_area(struct fsi_priv *fsi, struct fsi_stream *io)
{
	struct snd_pcm_runtime *runtime = io->substream->runtime;

Can this result in a NULL pointer dereference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609113836.45079-1-phucduc.bui@gmail.com?part=4

Re: [PATCH v6 06/11] ASoC: renesas: fsi: Move fsi_clk_init()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver releases DMA channels in `fsi_remove` before the devm-managed ALSA component is unregistered, leading to a NULL pointer dereference.
- [High] TOCTOU race condition in `fsi_pio_transfer` leads to a NULL pointer dereference on `io->substream->runtime`.
- [High] Concurrency bug and logic error in `fsi_clk_enable` breaks full-duplex audio and corrupts the shared clock refcount.
- [High] Unintended clearing of shared interrupt status causes dropped interrupts and audio dropouts for concurrent streams.
- [High] Missing cleanup on hardware startup failure leaves a dangling substream pointer, leading to a Use-After-Free.
- [High] Recursive fallback to PIO mode in `fsi_dma_probe` causes a permanent DMA channel leak.
- [Medium] Data race on hardware error registers and error counters causes dropped interrupt events.
- [Medium] PIO transfer functions perform raw 16-bit memory accesses without endianness conversion, corrupting audio on Big-Endian systems.
--

commit 143abdd60b62ddb2bbcd91ac88b1dcc0e574a4ad
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: Move fsi_clk_init()

Move fsi_clk_init() after set_rate() functions to prepare for subsequent
refactoring.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 3e3c6fd7c56b8..1df7dc0143630 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the cleanup
sequence in fsi_remove().

It appears fsi_remove() calls fsi_stream_remove(), which eventually
calls fsi_dma_remove() and sets io->chan to NULL:

fsi_remove()
   fsi_stream_remove()
      fsi_dma_remove()
         io->chan = NULL;

Since the ALSA component is registered with devres, it is unregistered
after fsi_remove() completes. During this devres cleanup, ALSA stops active
streams by invoking fsi_dai_trigger() with SNDRV_PCM_TRIGGER_STOP, which
calls into fsi_dma_push_start_stop():

fsi_dma_push_start_stop() {
        ...
        dmaengine_terminate_all(io->chan);
        ...
}

Does this cause a NULL pointer dereference on unbind if a stream is running?

[Severity: High]
This is a pre-existing issue, but does fsi_pio_transfer() have a race
condition during stream stop?

In fsi_pio_transfer(), the code briefly takes a lock inside
fsi_stream_is_working() to ensure io->substream is valid, but then
locklessly calls fsi_pio_get_area():

fsi_pio_transfer() {
        if (!fsi_stream_is_working(fsi, io))
                return -EINVAL;

        buf = fsi_pio_get_area(fsi, io);
}

fsi_pio_get_area() dereferences the substream:

fsi_pio_get_area() {
        struct snd_pcm_runtime *runtime = io->substream->runtime;
        ...
}

If the ALSA core concurrently triggers SNDRV_PCM_TRIGGER_STOP on another CPU,
it calls fsi_stream_quit() which sets io->substream = NULL. Could this
cause a NULL pointer dereference in fsi_pio_get_area()?

[Severity: High]
This is a pre-existing issue, but I have a question about the logic
in fsi_clk_enable().

fsi_clk_enable() {
        int ret = -EINVAL;
        ...
        if (0 == clock->count) {
                ...
                clock->count++;
        }

        return ret;
}

If one stream is already running (so clock->count is 1) and the opposite
stream starts, this skips the initialization block and returns the
uninitialized -EINVAL. Does this break full-duplex audio by failing the
startup of the second stream?

Additionally, fsi_clk_enable() and fsi_clk_disable() increment and
decrement clock->count locklessly. Since playback and capture can be
triggered concurrently, could this corrupt the shared clock refcount?

[Severity: Medium]
This is a pre-existing issue, but is it safe to read and modify these
hardware registers locklessly?

In fsi_count_fifo_err():

fsi_count_fifo_err() {
        u32 ostatus = fsi_reg_read(fsi, DOFF_ST);
        ...
        fsi->playback.oerr_num++;
        ...
        fsi_reg_write(fsi, DOFF_ST, 0);
}

Since this function can be invoked locklessly from both fsi_interrupt()
and fsi_dma_complete(), could concurrent execution cause lost hardware error
bits or corrupt the error counters?

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the interrupt
clearing logic during stream startup.

During stream initialization in fsi_hw_startup(), fsi_irq_clear_status()
is unconditionally called. It constructs a mask that includes both playback
and capture ports:

fsi_irq_clear_status() {
        data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->playback));
        data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->capture));

        /* clear interrupt factor */
        fsi_core_mask_set(master, int_st, data, 0);
}

If one stream is already actively running and the second stream is started,
will this unconditionally clear any pending interrupts for the active
stream, potentially causing dropped frames?

[Severity: High]
This is a pre-existing issue, but does fsi_dai_trigger() leave a dangling
pointer if hardware startup fails?

In fsi_dai_trigger():

fsi_dai_trigger() {
        ...
        fsi_stream_init(fsi, io, substream);
        if (!ret)
                ret = fsi_hw_startup(fsi, io, dai->dev);
        if (!ret)
                ret = fsi_stream_start(fsi, io);
        ...
}

fsi_stream_init() unconditionally assigns io->substream = substream.
If fsi_hw_startup() fails, it returns an error but does not call
fsi_stream_quit() to clear io->substream.

Since the ALSA core will destroy the failed substream runtime, could a
subsequent interrupt trigger fsi_count_fifo_err(), which invokes
fsi_stream_is_working() and evaluates the dangling io->substream->runtime?

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the fallback
logic in fsi_dma_probe().

If the DMA probe fails for capture, it switches the handler to PIO and
calls fsi_stream_probe() recursively:

fsi_dma_probe() {
        io->chan = dma_request_chan(dev, is_play ? "tx" : "rx");
        ...
        /* fallback to PIO */
        ...
        return fsi_stream_probe(fsi, dev);
}

The recursive call to fsi_stream_probe() will loop over both streams and
probe playback again. Since playback's handler is still DMA, it executes
fsi_dma_probe() a second time.

Will this unconditionally call dma_request_chan() for playback and
overwrite the existing io->chan without freeing it, causing a DMA
channel leak?

[Severity: Medium]
This is a pre-existing issue, but does fsi_pio_push16() handle endianness
correctly on big-endian architectures?

ALSA formats S16_LE and S24_LE define audio data as little-endian.
However, the function casts the buffer directly to a native u16 pointer
and reads it:

fsi_pio_push16() {
        u16 *buf = (u16 *)_buf;

        for (i = 0; i < samples; i++)
                fsi_reg_write(fsi, DODT, ((u32)*(buf + i) << 8));
}

Similarly, fsi_pio_pop16() directly casts and writes into the buffer:

fsi_pio_pop16() {
        u16 *buf = (u16 *)_buf;
        ...
        *(buf + i) = (u16)(fsi_reg_read(fsi, DIDT) >> 8);
}

On big-endian systems, wouldn't this yield byte-swapped data before
writing to the hardware registers or into the ALSA buffer? Should it
use little-endian conversion helpers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609113836.45079-1-phucduc.bui@gmail.com?part=6

Re: [PATCH v6 07/11] ASoC: renesas: fsi: Use devm_clk_get_optional() for optional clocks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] fsi_clk_enable() returns -EINVAL when called on an already enabled clock, breaking full-duplex operation.
--

commit e751270870bfce8cae5b9b1175c1bc9e265d605e
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: Use devm_clk_get_optional() for optional clocks

The xck, ick, and div clocks are optional. Switch from devm_clk_get()
to devm_clk_get_optional() to correctly handle cases where these clocks
are missing.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 1df7dc0143630..e29be2bcf952c 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -994,7 +994,7 @@ static int fsi_clk_init(struct device *dev,
>  
>  	/* external clock */
>  	if (xck) {
> -		clock->xck = devm_clk_get(dev, is_porta ? "xcka" : "xckb");
> +		clock->xck = devm_clk_get_optional(dev, is_porta ? "xcka" : "xckb");
>  		if (IS_ERR(clock->xck)) {
>  			dev_err(dev, "can't get xck clock\n");
>  			return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing these
clock changes, I noticed a pre-existing issue in fsi_clk_enable().

sound/soc/renesas/fsi.c:fsi_clk_enable() {
	struct fsi_clk *clock = &fsi->clock;
	int ret = -EINVAL;

	if (!fsi_clk_is_valid(fsi))
		return ret;

	if (0 == clock->count) {
		...
		clock->count++;
	}

	return ret;
}

When a second stream starts on the same port (for example, capture starting
while playback is already running), count is 1. The code skips the
(0 == clock->count) block and returns the initial ret value of -EINVAL.

Does this break full-duplex operation by causing fsi_hw_startup() to fail
when starting the second stream?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609113836.45079-1-phucduc.bui@gmail.com?part=7

Re: [PATCH v6 05/11] ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The newly added `fsi_stream_is_working()` checks introduce a Time-of-Check to Time-of-Use (TOCTOU) race condition with the clock disablement path.

Pre-existing issues:
- [High] The patch fails to prevent system hangs during in-flight IRQs because it misses the `fsi_stream_transfer()` path, which still performs unprotected hardware accesses and NULL pointer dereferences.
--

commit 6fe54dc43154d802e5ce8471d592fd6f73f622bd
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

This commit attempts to fix system hangs caused by in-flight IRQs accessing
registers when the SPU clock is disabled. It introduces fsi_stream_is_working()
checks in fsi_count_fifo_err() and fsi_irq_clear_status() to abort early if no
streams are active.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 0bd0e0c8c5a30..3e3c6fd7c56b8 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -470,6 +470,10 @@ static int fsi_get_current_fifo_samples(struct fsi_priv *fsi,
>  
>  static void fsi_count_fifo_err(struct fsi_priv *fsi)
>  {
> +	if (!fsi_stream_is_working(fsi, &fsi->playback) &&
> +	    !fsi_stream_is_working(fsi, &fsi->capture))
> +		return;
> +

[Severity: High]
Does this introduce a time-of-check to time-of-use race condition?

The fsi_stream_is_working() function acquires master->lock, checks the
substream, and immediately drops the lock. Because the lock is released before
the hardware access below:

	u32 ostatus = fsi_reg_read(fsi, DOFF_ST);
	u32 istatus = fsi_reg_read(fsi, DIFF_ST);

Could a concurrent ALSA shutdown sequence on another CPU execute
fsi_stream_quit() and fsi_hw_shutdown() to disable the SPU clock exactly in
this window, leading to the same system hang?

[ ... ]

> @@ -681,6 +685,10 @@ static void fsi_irq_clear_status(struct fsi_priv *fsi)
>  	u32 data = 0;
>  	struct fsi_master *master = fsi_get_master(fsi);
>  
> +	if (!fsi_stream_is_working(fsi, &fsi->playback) &&
> +	    !fsi_stream_is_working(fsi, &fsi->capture))
> +		return;
> +
>  	data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->playback));
>  	data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->capture));
>  

[Severity: High]
This isn't a bug introduced by this patch, but does the in-flight IRQ still
trigger hardware accesses and NULL pointer dereferences through other paths?

If fsi_interrupt() runs during shutdown, it calls fsi_stream_transfer()
unconditionally without checking fsi_stream_is_working() first:

sound/soc/renesas/fsi.c:fsi_interrupt() {
	if (int_st & AB_IO(1, AO_SHIFT))
		fsi_stream_transfer(&master->fsia.playback);
	...
}

In PIO mode, this leads to fsi_pio_push() which reads the hardware registers
before verifying the stream state:

	sample_space = io->fifo_sample_capa -
		       fsi_get_current_fifo_samples(fsi, io);

In DMA mode, it leads to fsi_dma_transfer() which dereferences io->substream
directly:

	struct snd_soc_dai *dai = fsi_get_dai(io->substream);
	struct snd_pcm_substream *substream = io->substream;

Can this still cause a system hang or a NULL pointer dereference if the
stream clock is already disabled and io->substream is set to NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609113836.45079-1-phucduc.bui@gmail.com?part=5

Re: [PATCH v6 03/11] ASoC: renesas: fsi: Fix trigger stop ordering

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Pre-existing data race in fsi_clk_enable() leads to premature clock disabling (bus hang) and breaks full-duplex operation.
- [High] Missing IRQ and DMA synchronization during stream shutdown leads to TOCTOU NULL pointer dereferences and hardware bus hangs.
--

commit b345d93585828cc07f1537860417394b58877fa1
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: Fix trigger stop ordering

This patch changes the trigger stop ordering by calling fsi_stream_stop()
before fsi_hw_shutdown(). This ensures that the sequence matches the
existing order in the suspend path.

> This change ensures all register accesses during stream shutdown are fully
> completed before disabling the clocks.

[Severity: High]
This isn't a bug introduced by this patch, but is this statement accurate
without synchronization?

Without synchronize_irq() and dmaengine_terminate_sync() in the shutdown
sequence, an in-flight fsi_interrupt() or fsi_dma_complete() could run
concurrently.

If an in-flight interrupt handler accesses FSI registers after
fsi_hw_shutdown() disables the hardware clocks, could this trigger an
asynchronous bus hang?

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 8cbd7acc26f49..800ac40f9680d 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1586,10 +1586,10 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>  			ret = fsi_stream_transfer(io);
>  		break;
>  	case SNDRV_PCM_TRIGGER_STOP:
> -		if (!ret)
> -			ret = fsi_hw_shutdown(fsi, dai->dev);
>  		fsi_stream_stop(fsi, io);
>  		fsi_stream_quit(fsi, io);

[Severity: High]
This is a pre-existing issue, but does fsi_stream_quit() introduce a race
with in-flight interrupts?

Because fsi_stream_quit() sets io->substream = NULL, could concurrent
execution cause NULL pointer dereferences in fsi_pio_get_area() or
fsi_pointer_update(), which dereference io->substream without validation?

> +		if (!ret)
> +			ret = fsi_hw_shutdown(fsi, dai->dev);

[Severity: High]
This is also a pre-existing issue, but does fsi_hw_shutdown() have a race
condition when managing the clocks?

fsi_hw_shutdown() could run concurrently with fsi_hw_startup() since ALSA
permits concurrent SNDRV_PCM_TRIGGER_START calls for playback and capture
on the same DAI.

Looking at fsi_clk_enable() and fsi_clk_disable() which are called from
these paths:

sound/soc/renesas/fsi.c:fsi_clk_enable() {
    ...
    int ret = -EINVAL;
    ...
    if (0 == clock->count) {
        // ...
        clock->count++;
    }
    return ret;
}

sound/soc/renesas/fsi.c:fsi_clk_disable() {
    ...
    if (1 == clock->count--) {
    ...
}

Could a lost update on the lockless clock->count variable lead to
prematurely disabling the clock when the first stream stops, turning off
FSI clocks while the second stream is still actively accessing registers?

Additionally, fsi_clk_enable() initializes ret to -EINVAL. If a second
stream tries to start (clock->count != 0), doesn't it unconditionally
return -EINVAL, breaking full-duplex operation?

>  		break;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609113836.45079-1-phucduc.bui@gmail.com?part=3

Re: [PATCH v6 08/11] ASoC: renesas: fsi: refactor clock initialization

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unconditional clock initialization during probe breaks the driver on slave-only platforms due to clkdev fallback.

Pre-existing issues:
- [High] Logic error in fsi_clk_enable() prevents concurrent playback and capture streams.
--

commit 83020abc14f73339100e18d0f7451918e1591b37
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: refactor clock initialization

Move fsi_clk_init() from set_fmt() to the probe path to ensure that clock
resources are acquired only once during device initialization.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index e29be2bcf952c..43bc77ebcca39 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
[ ... ]
> @@ -970,35 +980,35 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
>  	return ret;
>  }
>  
> -static int fsi_clk_init(struct device *dev,
> -			struct fsi_priv *fsi,
> -			int xck,
> -			int ick,
> -			int div,
> -			int (*set_rate)(struct device *dev,
> -					struct fsi_priv *fsi))
> +static int fsi_clk_init(struct device *dev, struct fsi_priv *fsi)
>  {
[ ... ]
>  	/* external clock */
>  	if (xck) {
>  		clock->xck = devm_clk_get_optional(dev, is_porta ? "xcka" : "xckb");
> -		if (IS_ERR(clock->xck)) {
> -			dev_err(dev, "can't get xck clock\n");
> -			return -EINVAL;
> -		}
> +		if (IS_ERR(clock->xck))
> +			return dev_err_probe(dev, PTR_ERR(clock->xck), "Can't get xck clock\n");
>  		if (clock->xck == clock->own) {
>  			dev_err(dev, "cpu doesn't support xck clock\n");
>  			return -EINVAL;
>  		}
[ ... ]
> @@ -1672,15 +1678,6 @@ static int fsi_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>  		break;
>  	}
>  
> -	if (fsi_is_clk_master(fsi)) {
> -		if (fsi->clk_cpg)
> -			fsi_clk_init(dai->dev, fsi, 0, 1, 1,
> -				     fsi_clk_set_rate_cpg);
> -		else
> -			fsi_clk_init(dai->dev, fsi, 1, 1, 0,
> -				     fsi_clk_set_rate_external);
> -	}
> -
>  	/* set format */
>  	if (fsi_is_spdif(fsi))
>  		ret = fsi_set_fmt_spdif(fsi);
[ ... ]
> @@ -1978,6 +1975,11 @@ static int fsi_probe(struct platform_device *pdev)
>  	fsi->master	= master;
>  	fsi_port_info_init(fsi, &info.port_a);
>  	fsi_handler_init(fsi, &info.port_a);
> +	ret = fsi_clk_init(&pdev->dev, fsi);
> +	if (ret) {
> +		dev_err(&pdev->dev, "FSIA clk init failed\n");
> +		return ret;
> +	}

[Severity: High]
By removing the fsi_is_clk_master() check from fsi_dai_set_fmt() and calling
fsi_clk_init() unconditionally here during probe(), does this break the
driver on slave-only platforms?

On older platforms using clkdev, looking up missing optional clocks like
xckb or ickb can trigger a fuzzy-match fallback to the device's main
functional clock. When devm_clk_get_optional() returns this fallback clock
instead of NULL, the check in fsi_clk_init() for (clock->xck == clock->own)
evaluates to true and returns -EINVAL.

Since fsi_clk_init() is now executed unconditionally for all ports, will this
cause legitimately configured slave ports (which lack explicit master clocks)
to fail probe entirely?

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the clock
handling, I noticed a pre-existing issue in fsi_clk_enable() that prevents
concurrent streams:

sound/soc/renesas/fsi.c:fsi_clk_enable() {
        int ret = -EINVAL;

        if (!fsi_clk_is_valid(fsi))
                return ret;

        if (0 == clock->count) {
                ...
        }

        ...
        return ret;
}

When a second concurrent stream starts (for example, capture during
playback), the function skips the if (0 == clock->count) block. Because
ret is initialized to -EINVAL, the function incorrectly returns -EINVAL
instead of 0.

Does this cause fsi_hw_startup() to fail and abort the stream trigger,
permanently preventing full-duplex operation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609113836.45079-1-phucduc.bui@gmail.com?part=8

Re: [PATCH v6 10/11] ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but fsi_clk_enable() incorrectly handles reference counting and returns -EINVAL for subsequent streams, breaking concurrent playback and capture (full-duplex).
- [High] This problem wasn't introduced by this patch, but fsi_clk_enable() triggers sleepable mutex acquisitions (clk_set_rate) from inside an atomic trigger context, potentially causing a deadlock or kernel panic.
- [High] This problem wasn't introduced by this patch, but opening or closing an audio stream unconditionally invalidates the shared clock state, breaking any other active stream on the same DAI.
--

commit 2ca0d4af70026409e881ce4021390b9ef2e6dfa9
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()

This commit adds fsi_clk_prepare() and fsi_clk_unprepare() helpers and calls
them from the startup and shutdown routines. This ensures clocks are prepared
from sleepable contexts.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 716ecf0401fe3..e26f39dfe0598 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
[ ... ]
> @@ -730,6 +730,54 @@ static int fsi_clk_is_valid(struct fsi_priv *fsi)
>  		fsi->clock.rate;
>  }
>  
> +static int fsi_clk_prepare(struct fsi_priv *fsi)
> +{
[ ... ]
> +static void fsi_clk_unprepare(struct fsi_priv *fsi)
> +{
[ ... ]
> +	clk_unprepare(spu);
> +}

[Severity: High]
This isn't a bug introduced by this patch, but does fsi_clk_enable() correctly
handle reference counting for subsequent streams?

Looking at fsi_clk_enable() in the same file:

static int fsi_clk_enable(struct device *dev, struct fsi_priv *fsi)
{
        struct fsi_clk *clock = &fsi->clock;
        int ret = -EINVAL;
        ...
        if (0 == clock->count) {
                ...
                clock->count++;
        }

        return ret;
}

If clock->count is greater than 0 (such as during a second stream start for
concurrent playback and capture), the if block is skipped and the function
directly returns the initialized ret value of -EINVAL. Does this break
full-duplex audio by failing the ALSA trigger operation for the second stream?

Additionally, if the second stream fails to start but later triggers an
unbalanced shutdown during cleanup, fsi_clk_disable() underflows clock->count
via the post-decrement:

static int fsi_clk_disable(struct device *dev, struct fsi_priv *fsi)
{
        ...
        if (1 == clock->count--) {
                clk_disable(clock->xck);
                ...
        }
        ...
}

Could this underflow cause further clock enablement to fail?

[Severity: High]
This is also a pre-existing issue, but does fsi_dai_trigger() call sleepable
functions while in an atomic context?

The ALSA ASoC trigger callback runs in an atomic context under a spinlock
since the DAI and PCM do not have the nonatomic flag set. However, the
call chain leads to clk_set_rate() which acquires the global CCF prepare_lock
mutex and can sleep:

fsi_dai_trigger()
  fsi_hw_startup()
    fsi_clk_enable()
      clock->set_rate()
        clk_set_rate()

Could this attempt to acquire a mutex while holding a spinlock result in a
scheduling while atomic panic or deadlock?

>  static int fsi_clk_enable(struct device *dev,
>  			  struct fsi_priv *fsi)
>  {
[ ... ]
> @@ -1580,7 +1628,7 @@ static int fsi_dai_startup(struct snd_pcm_substream *substream,
>  
>  	fsi_clk_invalid(fsi);
>  
> -	return 0;
> +	return fsi_clk_prepare(fsi);
>  }
>  
>  static void fsi_dai_shutdown(struct snd_pcm_substream *substream,
> @@ -1588,6 +1636,7 @@ static void fsi_dai_shutdown(struct snd_pcm_substream *substream,
>  {
>  	struct fsi_priv *fsi = fsi_get_priv(substream);
>  
> +	fsi_clk_unprepare(fsi);
>  	fsi_clk_invalid(fsi);

[Severity: High]
This is a pre-existing issue, but does calling fsi_clk_invalid()
unconditionally here and in fsi_dai_startup() break other active streams
on the same DAI?

Because playback and capture streams share a single fsi_clk structure,
if one stream is already running and a second stream is opened or closed,
fsi_clk_invalid() zeroes out fsi->clock.rate.

Later, when the running stream is closed via fsi_dai_trigger(),
fsi_hw_shutdown() will call fsi_clk_disable(). Because the rate was wiped,
fsi_clk_is_valid() evaluates to false:

static int fsi_clk_disable(struct device *dev, struct fsi_priv *fsi)
{
        if (!fsi_clk_is_valid(fsi))
                return -EINVAL;
        ...
}

This causes fsi_clk_disable() to return early without decrementing
clock->count or disabling the hardware clocks. Does this leak the clocks
indefinitely and break the reference count for future stream startups?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609113836.45079-1-phucduc.bui@gmail.com?part=10

Re: [PATCH v6 11/11] ASoC: renesas: fsi: Add SPU clock control in hw_startup/shutdown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Double disable of the SPU clock on the error path.

Pre-existing issues:
- [High] `fsi_clk_enable()` unconditionally fails subsequent streams (breaks full-duplex).
- [High] Sleeping in atomic context during trigger.
- [High] Concurrent triggers race on shared hardware registers.
--

commit f5cfbced3eb2033737642bc08abc86f7381ec9a6
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: Add SPU clock control in hw_startup/shutdown

This commit enables and disables the SPU clock in fsi_hw_startup() and
fsi_hw_shutdown(). This ensures the clock is active while the driver
accesses hardware registers.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index e26f39dfe0598..b72396b5de7dc 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1560,6 +1560,11 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
>  			  struct device *dev)
>  {
>  	u32 data = 0;
> +	int ret;
> +	/* enable spu bus bridge clock */
> +	ret = clk_enable(fsi->master->clk_spu);
> +	if (ret)
> +		return ret;

[Severity: High]
This is a pre-existing issue, but does this code have a race condition when
triggered concurrently?

Playback and capture streams use the same fsi_priv instance and share
hardware registers like CKG1. Since they are separate substreams with
separate ALSA locks, starting or stopping them concurrently on different
CPUs could cause fsi_hw_startup() and fsi_hw_shutdown() to run in parallel.

This could lead to unprotected read-modify-write operations on shared
registers:

    fsi_reg_mask_set(fsi, CKG1, (DIMD | DOMD), data);

and on the shared clock count:

    fsi_clk_enable() {
        ...
        clock->count++;
    }

Could this corrupt the clock reference count and hardware register states?

> [ ... ]
> @@ -1605,8 +1610,13 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
>  	fsi_fifo_init(fsi, io, dev);
>  
>  	/* start master clock */
> -	if (fsi_is_clk_master(fsi))
> -		return fsi_clk_enable(dev, fsi);
> +	if (fsi_is_clk_master(fsi)) {
> +		ret = fsi_clk_enable(dev, fsi);

[Severity: High]
This is a pre-existing issue, but does fsi_clk_enable() break full-duplex
operation?

If a stream is already running, clock->count is 1. When a second stream
starts, the condition if (0 == clock->count) is false, but the function
does not set ret = 0.

    fsi_clk_enable() {
        int ret = -EINVAL;
        if (!fsi_clk_is_valid(fsi))
            return ret;
            
        if (0 == clock->count) {
            ...
        }
        return ret;
    }

Won't this unconditionally return -EINVAL for the second stream, causing
fsi_hw_startup() to fail and preventing simultaneous playback and capture?

[Severity: High]
This is also a pre-existing issue, but could calling fsi_clk_enable() here
cause a sleep in atomic context?

The ALSA trigger callback runs in atomic context with the PCM substream
spinlock held. fsi_clk_enable() calls clock->set_rate(), which routes to
fsi_clk_set_rate_cpg():

    fsi_clk_set_rate_cpg() {
        ...
        ret = clk_set_rate(ick, best_cout);
    }

Since the Common Clock Framework's clk_set_rate() acquires a mutex and
sleeps, won't this cause a scheduling while atomic panic?

> +		if (ret) {
> +			clk_disable(fsi->master->clk_spu);
> +			return ret;
> +		}
> +	}
>  
>  	return 0;
>  }
> @@ -1614,9 +1624,15 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
>  static int fsi_hw_shutdown(struct fsi_priv *fsi,
>  			    struct device *dev)
>  {
> +	int ret;
>  	/* stop master clock */
> -	if (fsi_is_clk_master(fsi))
> -		return fsi_clk_disable(dev, fsi);
> +	if (fsi_is_clk_master(fsi)) {
> +		ret = fsi_clk_disable(dev, fsi);
> +		if (ret)
> +			return ret;
> +	}
> +	/* stop spu bus bridge clock */
> +	clk_disable(fsi->master->clk_spu);
>  
>  	return 0;
>  }

[Severity: High]
Is there a risk of double-disabling the SPU clock if fsi_clk_enable() fails?

If fsi_clk_enable() fails during startup, it disables the SPU clock before
returning the error:

    fsi_hw_startup() {
        ...
        if (ret) {
            clk_disable(fsi->master->clk_spu);
            return ret;
        }
    }

ALSA intercepts this error in snd_pcm_action_single() and invokes
snd_pcm_undo_start(), which calls the driver's trigger callback with
SNDRV_PCM_TRIGGER_STOP.

This triggers fsi_hw_shutdown(), which unconditionally calls clk_disable()
on the SPU clock again, since fsi_clk_disable() returns 0 for a zero clock
count.

Could this result in an unbalanced clock disable, potentially triggering
warnings and preventing the clock from being re-enabled later?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609113836.45079-1-phucduc.bui@gmail.com?part=11