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

[PATCH v3 00/10] 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 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.


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). 
    The test video link is provided below.
  - FSI master mode is currently compile-tested only. Full verification
    requires a dedicated HDMI driver (FSIB) or hardware modifications 
    (resoldering board resistors) (FSIA).
  - Kernel config gist link : 
    https://gist.github.com/BuiDucPhuc/fc9e5c3ab7a14766eecb4ce568f1cf17
  - Youtube video link of the test process
    https://youtu.be/w3H4v5djr7M

Best regards,
Phuc

bui duc phuc (10):
  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: Fix register access from in-flight IRQ after
    shutdown
  ASoC: renesas: fsi: Move fsi_clk_init()
  ASoC: renesas: fsi: Add shared SPU clock support
  ASoC: renesas: fsi: refactor clock initialization
  ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()
  ASoC: renesas: fsi: Use clock prepare handling in startup/shutdown
  ASoC: renesas: fsi: Add SPU clock control in hw_startup/shutdown

 .../bindings/sound/renesas,fsi.yaml           |  27 +-
 arch/arm/boot/dts/renesas/r8a7740.dtsi        |  12 +-
 sound/soc/renesas/fsi.c                       | 268 +++++++++++++-----
 3 files changed, 235 insertions(+), 72 deletions(-)

-- 
2.43.0

[PATCH v3 01/10] 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 parents.
  - FSI internal dividers (diva/b) for audio clock generation.
  - External clock inputs (xcka/b) provided by the board.

Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 .../bindings/sound/renesas,fsi.yaml           | 27 ++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
index df91991699a7..c50e7115b21a 100644
--- a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
+++ b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
@@ -38,7 +38,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: Internal FSI dividers for port A used for audio clock generation
+      - description: Internal 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 Internal FSI dividers for port A used for audio clock generation
+        - divb # optional Internal 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
-- 
2.43.0

[PATCH v3 02/10] arm: dts: renesas: r8a7740: Add clocks for FSI

[phucduc.bui]

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

Add the SPU clock to the FSI node to ensure it is enabled before register
access, preventing potential system hangs.
Add missing clocks for FSI (icka/b, xcka/b).

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 v3 03/10] ASoC: renesas: fsi: Fix trigger stop ordering

[phucduc.bui]

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

Reorder calls to execute fsi_stream_stop() before fsi_hw_shutdown().
This ensures that all register accesses are completed before the clock is
disabled, preventing the system hang observed on r8a7740.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/renesas/fsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

[PATCH v3 04/10] ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

[phucduc.bui]

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

Ensure that in-flight IRQ handlers do not access hardware registers
after SNDRV_PCM_TRIGGER_STOP and fsi_hw_shutdown().

An IRQ handler may still be running when the SPU clock is disabled,
leading to register access after clock shutdown and causing system
hangs.

Prevent register access in the IRQ path once the hardware has been
shut down.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/renesas/fsi.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 94ab2e490810..fc8428cb1ba7 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -211,6 +211,7 @@ struct fsi_stream {
 	int sample_width;	/* sample width */
 	int uerr_num;
 	int oerr_num;
+	bool running;
 
 	/*
 	 * bus options
@@ -255,6 +256,8 @@ struct fsi_priv {
 
 	u32 fmt;
 
+	int running_streams;
+
 	int chan_num:16;
 	unsigned int clk_master:1;
 	unsigned int clk_cpg:1;
@@ -460,6 +463,9 @@ static int fsi_get_current_fifo_samples(struct fsi_priv *fsi,
 
 static void fsi_count_fifo_err(struct fsi_priv *fsi)
 {
+	if (fsi->running_streams == 0)
+		return;
+
 	u32 ostatus = fsi_reg_read(fsi, DOFF_ST);
 	u32 istatus = fsi_reg_read(fsi, DIFF_ST);
 
@@ -681,6 +687,9 @@ static void fsi_irq_clear_status(struct fsi_priv *fsi)
 	u32 data = 0;
 	struct fsi_master *master = fsi_get_master(fsi);
 
+	if (fsi->running_streams == 0)
+		return;
+
 	data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->playback));
 	data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->capture));
 
@@ -1573,10 +1582,17 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 {
 	struct fsi_priv *fsi = fsi_get_priv(substream);
 	struct fsi_stream *io = fsi_stream_get(fsi, substream);
+	bool need_shutdown = false;
 	int ret = 0;
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
+		scoped_guard(spinlock_irqsave, &fsi->master->lock) {
+			if (!io->running) {
+				io->running = true;
+				fsi->running_streams++;
+			}
+		}
 		fsi_stream_init(fsi, io, substream);
 		if (!ret)
 			ret = fsi_hw_startup(fsi, io, dai->dev);
@@ -1586,8 +1602,15 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 			ret = fsi_stream_transfer(io);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
+		scoped_guard(spinlock_irqsave, &fsi->master->lock) {
+			if (io->running) {
+				io->running = false;
+				if (--fsi->running_streams == 0)
+					need_shutdown = true;
+			}
+		}
 		fsi_stream_stop(fsi, io);
-		if (!ret)
+		if (!ret && need_shutdown)
 			ret = fsi_hw_shutdown(fsi, dai->dev);
 		fsi_stream_quit(fsi, io);
 		break;
@@ -1968,6 +1991,7 @@ static int fsi_probe(struct platform_device *pdev)
 	fsi->base	= master->base;
 	fsi->phys	= res->start;
 	fsi->master	= master;
+	fsi->running_streams = 0;
 	fsi_port_info_init(fsi, &info.port_a);
 	fsi_handler_init(fsi, &info.port_a);
 	ret = fsi_stream_probe(fsi, &pdev->dev);
@@ -1981,6 +2005,7 @@ static int fsi_probe(struct platform_device *pdev)
 	fsi->base	= master->base + 0x40;
 	fsi->phys	= res->start + 0x40;
 	fsi->master	= master;
+	fsi->running_streams = 0;
 	fsi_port_info_init(fsi, &info.port_b);
 	fsi_handler_init(fsi, &info.port_b);
 	ret = fsi_stream_probe(fsi, &pdev->dev);
-- 
2.43.0

[PATCH v3 05/10] 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.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 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 fc8428cb1ba7..06ec5709216b 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -718,70 +718,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)
 {
@@ -1035,6 +971,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 v3 06/10] ASoC: renesas: fsi: Add shared SPU clock support

[phucduc.bui]

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

Add SPU clock pointer and reference count for shared FSIA/FSIB usage,
and initialize it in fsi_probe().

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/renesas/fsi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 06ec5709216b..55a11f1fe8aa 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -295,7 +295,9 @@ struct fsi_master {
 	void __iomem *base;
 	struct fsi_priv fsia;
 	struct fsi_priv fsib;
+	struct clk *clk_spu;
 	const struct fsi_core *core;
+	int spu_count;
 	spinlock_t lock;
 };
 
@@ -1984,6 +1986,7 @@ static int fsi_probe(struct platform_device *pdev)
 
 	/* master setting */
 	master->core		= core;
+	master->spu_count	= 0;
 	spin_lock_init(&master->lock);
 
 	/* FSI A setting */
-- 
2.43.0

[PATCH v3 07/10] ASoC: renesas: fsi: refactor clock initialization

[phucduc.bui]

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

Move fsi_clk_init() to probe and use devm_clk_get_optional() for optional
clocks. This allows probe to succeed even when some optional clocks are
missing, while set_rate() performs strict validation to ensure all required
clocks are available for hardware configuration during audio setup.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/renesas/fsi.c | 70 ++++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 32 deletions(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 55a11f1fe8aa..8c46d6806958 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -865,6 +865,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) {
@@ -901,6 +906,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))
@@ -973,35 +983,42 @@ 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;
+	struct fsi_master *master = fsi->master;
 	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;
 
+	if (!master->clk_spu) {
+		master->clk_spu = devm_clk_get_optional(dev, "spu");
+		if (IS_ERR(master->clk_spu))
+			return PTR_ERR(master->clk_spu);
+	}
+
 	/* 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;
-		}
+		clock->xck = devm_clk_get_optional(dev, is_porta ? "xcka" : "xckb");
+		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;
@@ -1010,11 +1027,9 @@ static int fsi_clk_init(struct device *dev,
 
 	/* 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;
-		}
+		clock->ick = devm_clk_get_optional(dev,  is_porta ? "icka" : "ickb");
+		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;
@@ -1023,11 +1038,9 @@ static int fsi_clk_init(struct device *dev,
 
 	/* 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;
-		}
+		clock->div = devm_clk_get_optional(dev,  is_porta ? "diva" : "divb");
+		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;
@@ -1689,15 +1702,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);
@@ -1997,6 +2001,7 @@ static int fsi_probe(struct platform_device *pdev)
 	fsi->running_streams = 0;
 	fsi_port_info_init(fsi, &info.port_a);
 	fsi_handler_init(fsi, &info.port_a);
+	fsi_clk_init(&pdev->dev, fsi);
 	ret = fsi_stream_probe(fsi, &pdev->dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "FSIA stream probe failed\n");
@@ -2011,6 +2016,7 @@ static int fsi_probe(struct platform_device *pdev)
 	fsi->running_streams = 0;
 	fsi_port_info_init(fsi, &info.port_b);
 	fsi_handler_init(fsi, &info.port_b);
+	fsi_clk_init(&pdev->dev, fsi);
 	ret = fsi_stream_probe(fsi, &pdev->dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "FSIB stream probe failed\n");
-- 
2.43.0

[PATCH v3 08/10] ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()

[phucduc.bui]

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

Add helper functions for preparing and unpreparing FSI clocks.

These helpers centralize clock prepare/unprepare handling and
will be used by subsequent patches to move clock management
into sleepable contexts.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/renesas/fsi.c | 66 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 8c46d6806958..b93809b5da15 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -732,6 +732,72 @@ 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;
+
+	if (!IS_ERR_OR_NULL(spu) && fsi->master->spu_count == 0) {
+		ret = clk_prepare(spu);
+		if (ret)
+			return ret;
+	}
+
+	if (!IS_ERR_OR_NULL(xck)) {
+		ret = clk_prepare(xck);
+		if (ret)
+			goto err_spu;
+	}
+
+	if (!IS_ERR_OR_NULL(ick)) {
+		ret = clk_prepare(ick);
+		if (ret)
+			goto err_xck;
+	}
+
+	if (!IS_ERR_OR_NULL(div)) {
+		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;
+
+	if (!IS_ERR_OR_NULL(div))
+		clk_unprepare(div);
+
+	if (!IS_ERR_OR_NULL(ick))
+		clk_unprepare(ick);
+
+	if (!IS_ERR_OR_NULL(xck))
+		clk_unprepare(xck);
+
+	if (!IS_ERR_OR_NULL(spu) && fsi->master->spu_count == 0)
+		clk_unprepare(spu);
+}
+
 static int fsi_clk_enable(struct device *dev,
 			  struct fsi_priv *fsi)
 {
-- 
2.43.0

[PATCH v3 09/10] ASoC: renesas: fsi: Use clock prepare handling in startup/shutdown

[phucduc.bui]

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

Use fsi_clk_prepare() and fsi_clk_unprepare() in dai_startup()
and dai_shutdown().

Prepare clocks only during active audio streams to reduce
unnecessary power usage, and ensure clk_prepare() and
related operations run in non-atomic contexts.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 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 b93809b5da15..c3ac8b30f342 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -1644,9 +1644,16 @@ static int fsi_dai_startup(struct snd_pcm_substream *substream,
 			   struct snd_soc_dai *dai)
 {
 	struct fsi_priv *fsi = fsi_get_priv(substream);
+	int ret;
 
 	fsi_clk_invalid(fsi);
 
+	if (fsi->clock.count == 0) {
+		ret = fsi_clk_prepare(fsi);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -1655,6 +1662,8 @@ static void fsi_dai_shutdown(struct snd_pcm_substream *substream,
 {
 	struct fsi_priv *fsi = fsi_get_priv(substream);
 
+	if (fsi->clock.count == 0)
+		fsi_clk_unprepare(fsi);
 	fsi_clk_invalid(fsi);
 }
 
-- 
2.43.0

[PATCH v3 10/10] 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.

Previously, the SPU clock was enabled by the bootloader and
remained active during operation. However, after adding the
SPU clock to the device tree, it is disabled automatically
once the system initialization completes.

As a result, register accesses may occur while the clock is
disabled, which can lead to system hangs. Manage the SPU
clock explicitly in the driver to ensure correct behavior.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/renesas/fsi.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index c3ac8b30f342..5307085c0615 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -1579,6 +1579,19 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
 			  struct device *dev)
 {
 	u32 data = 0;
+	int ret;
+
+	/* enable spu clock */
+	if (fsi->master->clk_spu) {
+		scoped_guard(spinlock_irqsave, &fsi->master->lock) {
+			if (fsi->master->spu_count == 0) {
+				ret = clk_enable(fsi->master->clk_spu);
+				if (ret < 0)
+					return ret;
+			}
+				fsi->master->spu_count++;
+		}
+	}
 
 	/* clock setting */
 	if (fsi_is_clk_master(fsi))
@@ -1636,6 +1649,12 @@ static int fsi_hw_shutdown(struct fsi_priv *fsi,
 	/* stop master clock */
 	if (fsi_is_clk_master(fsi))
 		return fsi_clk_disable(dev, fsi);
+	if (fsi->master->clk_spu) {
+		scoped_guard(spinlock_irqsave, &fsi->master->lock) {
+			if (--fsi->master->spu_count == 0)
+				clk_disable(fsi->master->clk_spu);
+		}
+	}
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH v3 07/10] ASoC: renesas: fsi: refactor clock initialization

[Mark Brown]

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

On Sun, May 10, 2026 at 03:43:00PM +0700, phucduc.bui@gmail.com wrote:

> Move fsi_clk_init() to probe and use devm_clk_get_optional() for optional
> clocks. This allows probe to succeed even when some optional clocks are
> missing, while set_rate() performs strict validation to ensure all required
> clocks are available for hardware configuration during audio setup.

> +static int fsi_clk_init(struct device *dev, struct fsi_priv *fsi)

> +	if (!master->clk_spu) {
> +		master->clk_spu = devm_clk_get_optional(dev, "spu");
> +		if (IS_ERR(master->clk_spu))
> +			return PTR_ERR(master->clk_spu);
> +	}
> +

> @@ -1997,6 +2001,7 @@ static int fsi_probe(struct platform_device *pdev)
>  	fsi->running_streams = 0;
>  	fsi_port_info_init(fsi, &info.port_a);
>  	fsi_handler_init(fsi, &info.port_a);
> +	fsi_clk_init(&pdev->dev, fsi);

We ignore the return value from fsi_clk_init() which means deferred
probe won't work (as well as fatal errors).

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

Re: [PATCH v3 04/10] ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

[Kuninori Morimoto]

Hi

Sorry for my late response. It was long holiday in Japan.

> Ensure that in-flight IRQ handlers do not access hardware registers
> after SNDRV_PCM_TRIGGER_STOP and fsi_hw_shutdown().
> 
> An IRQ handler may still be running when the SPU clock is disabled,
> leading to register access after clock shutdown and causing system
> hangs.
> 
> Prevent register access in the IRQ path once the hardware has been
> shut down.
> 
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---

fsi_stream_is_working() can handle that ?

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v3 06/10] ASoC: renesas: fsi: Add shared SPU clock support

[Kuninori Morimoto]

Hi

> Add SPU clock pointer and reference count for shared FSIA/FSIB usage,
> and initialize it in fsi_probe().
> 
> Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---

Am I suggested this ?
clk_spu   should be merged into [07/10], and
spu_count should be merged into [08/10]

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v3 07/10] ASoC: renesas: fsi: refactor clock initialization

[Kuninori Morimoto]

Hi

> Move fsi_clk_init() to probe and use devm_clk_get_optional() for optional
> clocks. This allows probe to succeed even when some optional clocks are
> missing, while set_rate() performs strict validation to ensure all required
> clocks are available for hardware configuration during audio setup.
> 
> Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---

Below parts are clock->xxx cleanup code, not refactor clocks initialization ?
Please separate it into cleanup part and refactor part.

> 	/* 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;
> -		}
> +		clock->xck = devm_clk_get_optional(dev, is_porta ? "xcka" : "xckb");
> +		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;
> @@ -1010,11 +1027,9 @@ static int fsi_clk_init(struct device *dev,
>  
>  	/* 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;
> -		}
> +		clock->ick = devm_clk_get_optional(dev,  is_porta ? "icka" : "ickb");
> +		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;
> @@ -1023,11 +1038,9 @@ static int fsi_clk_init(struct device *dev,
>  
>  	/* 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;
> -		}
> +		clock->div = devm_clk_get_optional(dev,  is_porta ? "diva" : "divb");
> +		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;

Re: [PATCH v3 08/10] ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()

[Kuninori Morimoto]

Hi

> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> Add helper functions for preparing and unpreparing FSI clocks.
> 
> These helpers centralize clock prepare/unprepare handling and
> will be used by subsequent patches to move clock management
> into sleepable contexts.
> 
> Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---

[08/10] and [09/10] should be merged.

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v3 09/10] ASoC: renesas: fsi: Use clock prepare handling in startup/shutdown

[Kuninori Morimoto]

Hi

> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> Use fsi_clk_prepare() and fsi_clk_unprepare() in dai_startup()
> and dai_shutdown().
> 
> Prepare clocks only during active audio streams to reduce
> unnecessary power usage, and ensure clk_prepare() and
> related operations run in non-atomic contexts.
> 
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
(snip)
> +	if (fsi->clock.count == 0) {
> +		ret = fsi_clk_prepare(fsi);
> +		if (ret)
> +			return ret;
(snip)
> +	if (fsi->clock.count == 0)
> +		fsi_clk_unprepare(fsi);

fsi->clock.count check can be done on fsi_clk_[un]prepare() ?

Thank you for your help !!

Best regards
---
Kuninori Morimoto

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

[Geert Uytterhoeven]

Hi Phuc,

Thanks for the update!

On Sun, 10 May 2026 at 10:43, <phucduc.bui@gmail.com> wrote:
> 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 parents.
>   - FSI internal dividers (diva/b) for audio clock generation.
>   - External clock inputs (xcka/b) provided by the board.
>
> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>

Please drop this tag: it is intended for giving credit to the person
who suggested the creation of this (full) patch, and not for crediting
review comments on a previous version.

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

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 07/10] ASoC: renesas: fsi: refactor clock initialization

[Bui Duc Phuc]

Hi Mark, Morimoto-san,

Thank you both for the review.

>We ignore the return value from fsi_clk_init() which means deferred
>probe won't work (as well as fatal errors).

I will update the fsi_clk_init() side as follows:

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");
}

The probe function will also be updated as follows:

ret = fsi_clk_init();
if (ret)
    return ret;

Do you think this approach looks okay?

>Below parts are clock->xxx cleanup code, not refactor clocks initialization ?
>Please separate it into cleanup part and refactor part.

Understood. I will split the current patch into two separate patches:

cleanup patch: includes the changes converting to devm_clk_get_optional()

refactor patch: includes the remaining changes such as updating
function arguments,
moving the function call locations, and adding handling in the
set_rate() functions, etc.

Best regards,
Phuc

Re: [PATCH v3 09/10] ASoC: renesas: fsi: Use clock prepare handling in startup/shutdown

[Bui Duc Phuc]

Hi,

> fsi->clock.count check can be done on fsi_clk_[un]prepare() ?

Thank you. I will move:

  if (clock->count != 0)
      return 0;

to the beginning of the fsi_clk_[un]prepare() functions to make the
code cleaner and more compact.

Best regards,
Phuc

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

[Bui Duc Phuc]

Hi Geert,

On Mon, May 11, 2026 at 2:31 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> > Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> Please drop this tag: it is intended for giving credit to the person
> who suggested the creation of this (full) patch, and not for crediting
> review comments on a previous version.

Thanks for the feedback. I understand the proper usage of the
Suggested-by tag now and will remove it in the v4 patch set.

Best regards,
Phuc

Re: [PATCH v3 01/10] 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:
- [High] The schema fails to enforce the mandatory SPU clock for the r8a7740 hardware variant, and the example provides a functionally broken configuration.
- [Medium] Mismatch between strictly positional `clocks` descriptions and unordered `clock-names`, combined with missing `clock-names` enforcement.
--

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

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

This commit updates the FSI bindings to support multiple clocks to properly
describe the hardware clock tree. It adds the SPU bus/bridge clock required
for the r8a7740 SoC, as well as definitions for functional clocks, internal
dividers, and external clock inputs.

> diff --git a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> index df91991699a72..c50e7115b21a0 100644
> --- a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> +++ b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> @@ -38,7 +38,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.

Should we add an allOf block with an if-then condition to enforce the
presence of the SPU clock for the renesas,fsi2-r8a7740 hardware variant?

Without it, incomplete device trees might silently pass schema validation but
fail catastrophically at runtime, as the commit message notes that missing
this clock causes a system hang.

The examples section of this file demonstrates this:

    sh_fsi2: sound@fe1f0000 {
            compatible = "renesas,fsi2-r8a7740", "renesas,sh_fsi2";
            ...
            clocks = <&mstp3_clks R8A7740_CLK_FSI>;

This example only supplies a single clock and omits both the required SPU
clock and the clock-names property, which contradicts the commit message.

> +      - description: CPG DIV6 functional clocks for FSI port A
> +      - description: CPG DIV6 functional clocks for FSI port B
> +      - description: Internal FSI dividers for port A used for audio clock generation
> +      - description: Internal 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

Does defining clock-names with an enum correctly map sparse clocks?

Because clocks defines an items array with eight fixed descriptions, it
acts as a strict positional tuple. If a board provides the Main clock and
the icka clock but omits the optional spu clock, icka shifts to index
1. The dt-schema tooling will then incorrectly map it to the SPU description.

Additionally, using an enum here allows arbitrary ordering and does not
enforce uniqueItems: true.

Could we add clock-names to the required list when multiple clocks are
provided? Since it is not required, a device tree could specify multiple clocks
without names, leaving the driver unable to reliably map the phandles to their
respective internal functions.

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

Re: [PATCH v3 02/10] arm: dts: renesas: r8a7740: Add clocks for FSI

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] System hang regression due to bisectability breakage (clock disabled by framework).
- [Low] DT Binding Schema Violation due to skipped positional items in the `clocks` array.
--

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

arm: dts: renesas: r8a7740: Add clocks for FSI

Add the SPU clock to the FSI node to ensure it is enabled before register
access, preventing potential system hangs.
Add missing clocks for FSI (icka/b, xcka/b).

> diff --git a/arch/arm/boot/dts/renesas/r8a7740.dtsi b/arch/arm/boot/dts/renesas/r8a7740.dtsi
> index d13ab86c3ab47..6f9d9bbfd159a 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>;

Will adding the spu clock to the device tree without the corresponding driver
support cause the common clock framework to automatically disable it during
system initialization?

If the bootloader leaves the spu clock enabled, and the kernel disables it
because the driver hasn't claimed it yet, could this cause a regression where
the system hangs on subsequent register accesses by the fsi driver?

To maintain bisectability, should the driver be updated to manage the spu
clock before or in the same commit as this device tree update? I see this
regression was addressed in a later commit adding spu clock control in
hw_startup/shutdown, but it may break bisectability here.

> +		clock-names = "fck", "spu", "icka", "ickb", "xcka",
> +				"xckb";

Does appending the external clocks (xcka and xckb) here without the internal
dividers (diva and divb) violate the strict positional order defined in the
device tree binding schema?

The dt-binding schema for renesas,sh_fsi2 defines the clocks property as an
ordered list where the 5th and 6th items are the internal dividers, and the
7th and 8th are the external clocks.

Since dt-validate evaluates array items positionally, will it evaluate the
external clocks against the internal divider descriptions? Should the array
be padded with dummy elements (like <0>) to maintain the strict positional
order?

>  		power-domains = <&pd_a4mp>;
>  		status = "disabled";
>  	};

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

Re: [PATCH v3 03/10] ASoC: renesas: fsi: Fix trigger stop ordering

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] TOCTOU race and lack of synchronization in stream stopping leads to NULL pointer dereference.
- [High] Teardown asymmetry in `fsi_remove` leads to use-after-free and NULL pointer dereference.
- [High] Lockless read-modify-write on shared FSI port registers causes hardware corruption.
- [High] Lockless modifications to shared `clock->count` cause reference count corruption.
--

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

ASoC: renesas: fsi: Fix trigger stop ordering

This commit reorders calls in the stop trigger to execute fsi_stream_stop()
before fsi_hw_shutdown(). It ensures register accesses complete before the
clock is disabled to prevent a system hang.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 8cbd7acc26f49..94ab2e4908108 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1586,9 +1586,9 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>  			ret = fsi_stream_transfer(io);
>  		break;
>  	case SNDRV_PCM_TRIGGER_STOP:

Playback and capture substreams on the same FSI port share port-level
configuration registers like CKG1, CKG2, and OUT_SEL.

Since the ALSA core serializes triggers at the substream level using
the self_group.lock, playback and capture triggers for the same DAI
can execute concurrently on different CPUs.

Does fsi_hw_startup() perform lockless read-modify-write operations on these
shared registers, and can this lack of exclusion lead to lost writes and
corrupted hardware state?

> +		fsi_stream_stop(fsi, io);

During a stream stop, fsi_stream_stop() disables DMA via
dmaengine_terminate_all() and masks device interrupts, but it does not appear
to wait for in-flight DMA callbacks or IRQ handlers to finish.

Concurrently, an executing IRQ handler calling fsi_pio_transfer() will
temporarily take master->lock in fsi_stream_is_working(), drop it, and then
locklessly access io->substream->runtime inside fsi_pio_get_area().

Similarly, an asynchronous fsi_dma_complete() callback unconditionally
dereferences io->substream in fsi_pointer_update().

Since there is no dmaengine_terminate_sync() or synchronize_irq(), could
fsi_stream_quit() race with these callbacks and clear io->substream,
resulting in a NULL pointer dereference?

The driver uses managed resources during fsi_probe(), meaning the IRQ handler
and ALSA component are unregistered after fsi_remove() returns. However,
fsi_remove() explicitly tears down the DMA streams via fsi_stream_remove(),
which calls dma_release_channel(io->chan) and sets io->chan to NULL.

Because the ALSA component is still fully registered and active while
fsi_remove() completes, userspace can still trigger ALSA operations.

If a STOP trigger occurs during this window, would calling
dmaengine_terminate_all(io->chan) here cause a NULL pointer dereference?

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

The shared clock state for an FSI port is tracked using clock->count inside
fsi_priv.

Since playback and capture triggers for the same port can execute
concurrently on different CPUs, do fsi_clk_enable() and fsi_clk_disable()
manually modify this counter without holding any lock?

Could concurrent updates to clock->count corrupt the counter, potentially
leaving the peripheral clocks enabled permanently or disabling them while a
stream is still active?

> -		fsi_stream_stop(fsi, io);
>  		fsi_stream_quit(fsi, io);
>  		break;
>  	}

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

Re: [PATCH v3 04/10] ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Incomplete Fix / Unprotected Hardware Access in fsi_stream_transfer()
- [Critical] Uncleared Interrupt Status Causes Guaranteed Hang on Next IRQ
- [Critical] Pre-existing TOCTOU Race Leading to NULL Pointer Dereference in IRQ Handler
- [High] Data Race / TOCTOU on fsi->running_streams
- [High] State Tracking Variables Leak on START Failure
--

commit a5f51696dc0fd84e1f16198b25e33b2b5da818dc
Author: bui duc phuc <phucduc.bui@gmail.com>
Subject: ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

This commit attempts to fix a system hang caused by in-flight interrupt
handlers accessing hardware registers after the clocks are shut down. It
tracks running streams and adds early returns in the interrupt path when no
streams are active.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 94ab2e4908108..fc8428cb1ba73 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -460,6 +463,9 @@ static int fsi_get_current_fifo_samples(struct fsi_priv *fsi,
>  
>  static void fsi_count_fifo_err(struct fsi_priv *fsi)
>  {
> +	if (fsi->running_streams == 0)
> +		return;
> +

Does this check completely protect against hardware access during shutdown?

The interrupt handler calls fsi_stream_transfer() before reaching
fsi_count_fifo_err() and fsi_irq_clear_status(). The fsi_stream_transfer()
function calls handlers that directly read hardware registers, such as
DOFF_ST inside fsi_get_current_fifo_samples(). Because this hardware access
remains completely unprotected by the new running_streams check, would an
in-flight interrupt handler still access the shut-down hardware and cause
the exact system hang this patch is intended to fix?

Additionally, is there a time-of-check to time-of-use data race when
evaluating fsi->running_streams here?

The value is read locklessly in the interrupt handler, while it is updated
under fsi->master->lock in the process context during fsi_dai_trigger().
An in-flight interrupt on one processor could evaluate running_streams > 0
locklessly and proceed. Concurrently, another processor could execute
fsi_dai_trigger(SNDRV_PCM_TRIGGER_STOP), acquire the lock, decrement the
counter to 0, and disable the hardware clocks. When the interrupt handler
resumes, it would execute hardware accesses on the disabled clock.

As a side note, there appears to be a pre-existing time-of-check to
time-of-use issue leading to a potential NULL pointer dereference in the
interrupt handler. In fsi_pio_transfer(), the handler calls
fsi_stream_is_working(), which temporarily takes master->lock to verify
io->substream != NULL. It then drops the lock and unconditionally
dereferences io->substream->runtime. Could a concurrent trigger stop on
another processor execute fsi_stream_quit() and set io->substream = NULL
between the check and the use, causing a kernel panic when the interrupt
handler resumes?

[ ... ]
> @@ -681,6 +687,9 @@ static void fsi_irq_clear_status(struct fsi_priv *fsi)
>  	u32 data = 0;
>  	struct fsi_master *master = fsi_get_master(fsi);
>  
> +	if (fsi->running_streams == 0)
> +		return;
> +

Can returning early here without clearing the interrupt status cause a
guaranteed hang on the next interrupt?

If an in-flight or delayed interrupt fires as the stream is shut down, the
handler skips clearing its port's interrupt bits in the shared master int_st
register. Because this register is shared between port A and port B, the
uncleared bit remains permanently asserted. When the other active port
subsequently generates an interrupt, fsi_interrupt() will read int_st, see
the stale bit for the shut-down port, and unconditionally call
fsi_stream_transfer() for it. This forces a hardware register access on the
shut-down port while its clocks are disabled.

[ ... ]
> @@ -1573,10 +1582,17 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>  {
>  	struct fsi_priv *fsi = fsi_get_priv(substream);
>  	struct fsi_stream *io = fsi_stream_get(fsi, substream);
> +	bool need_shutdown = false;
>  	int ret = 0;
>  
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
> +		scoped_guard(spinlock_irqsave, &fsi->master->lock) {
> +			if (!io->running) {
> +				io->running = true;
> +				fsi->running_streams++;
> +			}
> +		}
>  		fsi_stream_init(fsi, io, substream);
>  		if (!ret)
>  			ret = fsi_hw_startup(fsi, io, dai->dev);

Does this leak the state tracking variables if the start sequence fails?

If fsi_hw_startup() or fsi_stream_start() fails and returns an error,
the framework aborts the start sequence and does not invoke
SNDRV_PCM_TRIGGER_STOP. Because the error handling path does not revert
io->running and decrement fsi->running_streams, these variables could
permanently leak. This would prevent running_streams from reaching 0 on
subsequent valid stops, permanently bypassing fsi_hw_shutdown() and leaving
the hardware clocks powered on indefinitely.

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

Re: [PATCH v3 08/10] ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unbalanced clk_unprepare() for the shared SPU clock in the error path of fsi_clk_prepare().
--

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

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

This commit introduces fsi_clk_prepare() and fsi_clk_unprepare() helper
functions for managing FSI clocks. These centralize the prepare and unprepare
logic to facilitate moving clock management into sleepable contexts in future
patches.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 8c46d68069581..b93809b5da15e 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c

[ ... ]

> @@ -732,6 +732,72 @@ 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;
> +
> +	if (!IS_ERR_OR_NULL(spu) && fsi->master->spu_count == 0) {
> +		ret = clk_prepare(spu);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!IS_ERR_OR_NULL(xck)) {
> +		ret = clk_prepare(xck);
> +		if (ret)
> +			goto err_spu;
> +	}

[ ... ]

> +	return 0;
> +
> +err_ick:
> +	clk_unprepare(ick);
> +err_xck:
> +	clk_unprepare(xck);
> +err_spu:
> +	clk_unprepare(spu);

Will this result in an unbalanced clk_unprepare() for the spu clock?

When fsi->master->spu_count is greater than 0, clk_prepare(spu) is skipped
at the beginning of the function. If a subsequent clock preparation like
clk_prepare(xck) fails, the error path jumps to err_spu and unconditionally
calls clk_unprepare(spu).

Since clk_prepare(spu) was bypassed, does the err_spu label need to mirror
the preparation logic and check if spu_count is 0 to avoid decrementing the
clock framework's internal prepare count prematurely?

> +
> +	return ret;
> +}

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

Re: [PATCH v3 07/10] ASoC: renesas: fsi: refactor clock initialization

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The return value of `fsi_clk_init()` is ignored in `fsi_probe()`, which breaks probe deferral and leads to a kernel panic from dereferencing error pointers.
--

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

ASoC: renesas: fsi: refactor clock initialization

This commit moves fsi_clk_init() to probe and uses devm_clk_get_optional()
for optional clocks. It allows probe to succeed when optional clocks are
missing, while set_rate() validates clock availability.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 55a11f1fe8aa1..8c46d68069581 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
[ ... ]
> @@ -973,35 +983,42 @@ 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)
>  {
[ ... ]
>  	clock->own = devm_clk_get(dev, NULL);
>  	if (IS_ERR(clock->own))
>  		return -EINVAL;

Does returning -EINVAL here mask probe deferral? If devm_clk_get() returns
-EPROBE_DEFER, returning -EINVAL will permanently fail the driver probe
rather than deferring it.

[ ... ]
> @@ -1997,6 +2001,7 @@ static int fsi_probe(struct platform_device *pdev)
>  	fsi->running_streams = 0;
>  	fsi_port_info_init(fsi, &info.port_a);
>  	fsi_handler_init(fsi, &info.port_a);
> +	fsi_clk_init(&pdev->dev, fsi);

Is it safe to ignore the return value of fsi_clk_init() here?

If devm_clk_get_optional() fails and returns an error pointer like
-EPROBE_DEFER, those error pointers will be stored in the clock fields
instead of NULL.

Later, when fsi_clk_set_rate_external() checks if the clocks are missing:

    if (!xck || !ick) {

This check will pass because the error pointers are not NULL. The error
pointers will then be passed to common clock framework APIs like
clk_get_rate(), which will dereference them and cause a crash.

Should the return value of fsi_clk_init() be checked and propagated?

>  	ret = fsi_stream_probe(fsi, &pdev->dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "FSIA stream probe failed\n");
> @@ -2011,6 +2016,7 @@ static int fsi_probe(struct platform_device *pdev)
>  	fsi->running_streams = 0;
>  	fsi_port_info_init(fsi, &info.port_b);
>  	fsi_handler_init(fsi, &info.port_b);
> +	fsi_clk_init(&pdev->dev, fsi);

Would the same return value check be needed here as well?

>  	ret = fsi_stream_probe(fsi, &pdev->dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "FSIB stream probe failed\n");

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Unreachable SPU clock disable logic for clock masters leads to resource leak.
- [High] Shared use of `spu_count` for both clock preparation and enablement causes API violations and clock state corruption.
- [High] Missing rollback of SPU clock enablement on error path causes resource leak.
- [High] Unhandled error in `__fsi_resume` leads to system hang if clock enable fails.
- [Medium] Unprotected read of `spu_count` causes a data race.
- [Low] Misleading indentation in `fsi_hw_startup()`.
--

commit f4667d95bec52234e1ed11e120e67be3aae15efd
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() to prevent system hangs caused by hardware
register accesses while the clock is disabled.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index c3ac8b30f342b..5307085c06158 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1579,6 +1579,19 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
>  			  struct device *dev)
>  {
>  	u32 data = 0;
> +	int ret;
> +
> +	/* enable spu clock */
> +	if (fsi->master->clk_spu) {
> +		scoped_guard(spinlock_irqsave, &fsi->master->lock) {
> +			if (fsi->master->spu_count == 0) {
> +				ret = clk_enable(fsi->master->clk_spu);
> +				if (ret < 0)
> +					return ret;

If clk_enable() fails and returns an error here, how does __fsi_resume()
handle it? Looking at __fsi_resume(), it ignores the return value of
fsi_hw_startup() and unconditionally calls fsi_stream_start().

Could this attempt to access hardware registers and trigger a system hang if
the SPU clock failed to enable?

> +			}
> +				fsi->master->spu_count++;

This isn't a bug, but the indentation of fsi->master->spu_count++ makes it
appear as though it is conditionally executed inside the if block.

Does reusing fsi->master->spu_count here conflict with its use in
fsi_clk_prepare() and fsi_clk_unprepare()?

Since fsi_clk_prepare() checks spu_count without the fsi->master->lock,
could concurrent stream operations cause a data race?

Also, consider this sequence with concurrent streams:

Stream 1 opens: spu_count is 0, calls clk_prepare()
Stream 1 starts: spu_count is 0, calls clk_enable(), spu_count becomes 1
Stream 2 opens: spu_count is 1, skips clk_prepare()
Stream 1 stops: spu_count drops to 0, calls clk_disable()
Stream 1 closes: spu_count is 0, calls clk_unprepare()
Stream 2 starts: spu_count is 0, calls clk_enable()

Will Stream 2 try to call clk_enable() on an unprepared clock? Could a
separate counter be used for enablement to avoid breaking the clock state
machine?

> +		}
> +	}

[ ... ]

>  	/* start master clock */
>  	if (fsi_is_clk_master(fsi))
>  		return fsi_clk_enable(dev, fsi);

If fsi_clk_enable() fails here, does this code leak the SPU clock enablement?

Since the function returns the error directly, is there a need to call
clk_disable(fsi->master->clk_spu) and decrement fsi->master->spu_count
before returning?

>  
>  	return 0;
>  }
> @@ -1636,6 +1649,12 @@ static int fsi_hw_shutdown(struct fsi_priv *fsi,
>  	/* stop master clock */
>  	if (fsi_is_clk_master(fsi))
>  		return fsi_clk_disable(dev, fsi);
> +	if (fsi->master->clk_spu) {
> +		scoped_guard(spinlock_irqsave, &fsi->master->lock) {
> +			if (--fsi->master->spu_count == 0)
> +				clk_disable(fsi->master->clk_spu);
> +		}
> +	}
>  
>  	return 0;
>  }

Does this bypass the SPU clock disable logic entirely when the FSI device
operates as a clock master?

Since fsi_clk_disable() returns directly, it looks like fsi->master->clk_spu
is never disabled and fsi->master->spu_count remains unbalanced.

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

Re: [PATCH v3 09/10] ASoC: renesas: fsi: Use clock prepare handling in startup/shutdown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] State mismatch between stream open/close (startup/shutdown) and stream start/stop (trigger) causes unbalanced clk_prepare() and clk_unprepare() calls.
- [High] Unbalanced `clk_unprepare()` in the error cleanup path of `fsi_clk_prepare()`.
--

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

ASoC: renesas: fsi: Use clock prepare handling in startup/shutdown

This commit updates the FSI driver to use fsi_clk_prepare() and
fsi_clk_unprepare() during the DAI startup and shutdown phases to
reduce power usage and ensure clock operations run in non-atomic contexts.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index b93809b5da15e..c3ac8b30f342b 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1644,9 +1644,16 @@ static int fsi_dai_startup(struct snd_pcm_substream *substream,
>  			   struct snd_soc_dai *dai)
>  {
>  	struct fsi_priv *fsi = fsi_get_priv(substream);
> +	int ret;
>  
>  	fsi_clk_invalid(fsi);
>  
> +	if (fsi->clock.count == 0) {
> +		ret = fsi_clk_prepare(fsi);

Is it safe to use clock.count to guard the prepare and unprepare operations
in startup and shutdown?

Since clock.count is incremented and decremented during trigger (stream
start/stop) rather than startup/shutdown (stream open/close), this can lead
to state mismatches in multi-stream or full-duplex operations.

For instance, if Playback is opened and started (count=1), and then Capture
is opened, Capture will see count=1 and skip preparing the clock. If Playback
is then stopped and closed, it will unprepare the clock.

If Capture later starts, it will attempt to enable an unprepared clock,
violating the Common Clock Framework and potentially causing hardware failures.

Similarly, this can cause resource leaks if multiple streams are opened before
starting, or if trigger fails and underflows clock.count to UINT_MAX.

Also, looking at the error path for fsi_clk_prepare() which is called here:

sound/soc/renesas/fsi.c:fsi_clk_prepare() {
	...
	if (!IS_ERR_OR_NULL(spu) && fsi->master->spu_count == 0) {
		ret = clk_prepare(spu);
		if (ret)
			return ret;
	}
	...
err_spu:
	clk_unprepare(spu);

	return ret;
}

If fsi->master->spu_count is non-zero when entering the function,
clk_prepare(spu) is correctly skipped.

However, if a subsequent clock preparation fails, the error path unconditionally
calls clk_unprepare(spu). Does this prematurely decrement the shared SPU
clock's reference count, potentially disabling it while other instances are
still actively using it?

> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }

[ ... ]

> @@ -1655,6 +1662,8 @@ static void fsi_dai_shutdown(struct snd_pcm_substream *substream,
>  {
>  	struct fsi_priv *fsi = fsi_get_priv(substream);
>  
> +	if (fsi->clock.count == 0)
> +		fsi_clk_unprepare(fsi);
>  	fsi_clk_invalid(fsi);
>  }

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

Re: [PATCH v3 06/10] ASoC: renesas: fsi: Add shared SPU clock support

[Bui Duc Phuc]

Hi

> Am I suggested this ?
> clk_spu   should be merged into [07/10], and
> spu_count should be merged into [08/10]

Thank you for the suggestion!

I will move clk_spu into [07/10] and
spu_count into [08/10] in the next revision.

Best regards,
Phuc

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

[Bui Duc Phuc]

Hi all,

Based on the Sashiko AI review, I am thinking of adding the following
constraints specifically for renesas,fsi2-r8a7740 to address the
reported issues.

I think this may also help balance both Geert's and Krzysztof's
comments from the previous v2 review.

Does this approach look reasonable to you?

---------------------------------------------

-allOf:
-  - $ref: dai-common.yaml#
-
 properties:
   $nodename:
     pattern: "^sound@.*"
@@ -94,6 +91,78 @@ required:

 unevaluatedProperties: false

+allOf:
+  - $ref: dai-common.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,fsi2-r8a7740
+    then:
+      properties:
+        clock-names:
+          oneOf:
+            - items: # FSIA & FSIB is slave
+                - const: fck
+                - const: spu
+            - items: # FSIA slave & FSIB master use internal clock
+                - const: fck
+                - const: spu
+                - const: ickb
+                - const: divb
+            - items: # FSIA slave & FSIB master use external clock
+                - const: fck
+                - const: spu
+                - const: ickb
+                - const: xckb
+            - items: # FSIB slave & FSIA master use internal clock
+                - const: fck
+                - const: spu
+                - const: icka
+                - const: diva
+            - items: # FSIB slave & FSIA master use external clock
+                - const: fck
+                - const: spu
+                - const: icka
+                - const: xcka
+            - items: # FSIA master ex-clk  & FSIB master ex-clk
+                - const: fck
+                - const: spu
+                - const: icka
+                - const: xcka
+                - const: ickb
+                - const: xckb
+            - items: # FSIA master in-xlk  & FSIB master in-clk
+                - const: fck
+                - const: spu
+                - const: icka
+                - const: diva
+                - const: ickb
+                - const: divb
+            - items: # FSIA master in-clk  & FSIB master ex-clk
+                - const: fck
+                - const: spu
+                - const: icka
+                - const: diva
+                - const: ickb
+                - const: xckb
+            - items: # FSIA master ex-clk  & FSIB master in-clk
+                - const: fck
+                - const: spu
+                - const: icka
+                - const: xcka
+                - const: ickb
+                - const: divb
+            - items: # FSIA & FSIB  full clock support
+                - const: fck
+                - const: spu
+                - const: icka
+                - const: xcka
+                - const: diva
+                - const: ickb
+                - const: xckb
+                - const: divb
+
 examples:
   - |
     #include <dt-bindings/clock/r8a7740-clock.h>
@@ -102,7 +171,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>;

-----------------------------------------------------------------------------------------


Best regard,
Phuc




On Tue, May 12, 2026 at 3:45 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The schema fails to enforce the mandatory SPU clock for the r8a7740 hardware variant, and the example provides a functionally broken configuration.
> - [Medium] Mismatch between strictly positional `clocks` descriptions and unordered `clock-names`, combined with missing `clock-names` enforcement.
> --
>
> commit 3adcf4902a5e62b71566ef5381f002fd5338a399
> Author: bui duc phuc <phucduc.bui@gmail.com>
>
>     ASoC: dt-bindings: renesas,fsi: add support multiple clocks
>
> This commit updates the FSI bindings to support multiple clocks to properly
> describe the hardware clock tree. It adds the SPU bus/bridge clock required
> for the r8a7740 SoC, as well as definitions for functional clocks, internal
> dividers, and external clock inputs.
>
> > diff --git a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> > index df91991699a72..c50e7115b21a0 100644
> > --- a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> > +++ b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> > @@ -38,7 +38,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.
>
> Should we add an allOf block with an if-then condition to enforce the
> presence of the SPU clock for the renesas,fsi2-r8a7740 hardware variant?
>
> Without it, incomplete device trees might silently pass schema validation but
> fail catastrophically at runtime, as the commit message notes that missing
> this clock causes a system hang.
>
> The examples section of this file demonstrates this:
>
>     sh_fsi2: sound@fe1f0000 {
>             compatible = "renesas,fsi2-r8a7740", "renesas,sh_fsi2";
>             ...
>             clocks = <&mstp3_clks R8A7740_CLK_FSI>;
>
> This example only supplies a single clock and omits both the required SPU
> clock and the clock-names property, which contradicts the commit message.
>
> > +      - description: CPG DIV6 functional clocks for FSI port A
> > +      - description: CPG DIV6 functional clocks for FSI port B
> > +      - description: Internal FSI dividers for port A used for audio clock generation
> > +      - description: Internal 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
>
> Does defining clock-names with an enum correctly map sparse clocks?
>
> Because clocks defines an items array with eight fixed descriptions, it
> acts as a strict positional tuple. If a board provides the Main clock and
> the icka clock but omits the optional spu clock, icka shifts to index
> 1. The dt-schema tooling will then incorrectly map it to the SPU description.
>
> Additionally, using an enum here allows arbitrary ordering and does not
> enforce uniqueItems: true.
>
> Could we add clock-names to the required list when multiple clocks are
> provided? Since it is not required, a device tree could specify multiple clocks
> without names, leaving the driver unable to reliably map the phandles to their
> respective internal functions.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260510084303.122426-1-phucduc.bui@gmail.com?part=1