[PATCH v2 0/6] ASoC: renesas: fsi: Fix system hang by adding SPU clock

[PATCH v2 0/6] 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 this clock, register access can lead to a
system hang, even when the FSI functional clock is properly enabled.
This series adds the missing clocks and aligns their names with those
used in the driver.
Following feedback from Morimoto-san, the driver is refactored to improve
stability. Clock initialization is moved from the runtime path to the probe
function to simplify the flow and avoid redundant setups. Additionally, the
shutdown sequence is reordered to ensure the stream is properly stopped 
before the hardware is shut down.

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", "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.
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.
  - FSI master mode is currently compile-tested only. Full verification
    requires a dedicated HDMI driver (FSIB) or hardware modifications 
    (resoldering board resistors) (FSIA).Full support for fsidiv requires 
    additional DT bindings and a corresponding driver.


bui duc phuc (6):
  ASoC: renesas: fsi: Add shared SPU clock support
  ASoC: renesas: fsi: Fix hang by enabling SPU clock
  ASoC: renesas: fsi: Fix trigger stop ordering
  ASoC: renesas: fsi: refactor clock initialization
  arm: dts: renesas: r8a7740: Add clocks for FSI
  ASoC: dt-bindings: renesas,fsi: add support for multiple clocks

 .../bindings/sound/renesas,fsi.yaml           |  61 +++++-
 arch/arm/boot/dts/renesas/r8a7740.dtsi        |  12 +-
 sound/soc/renesas/fsi.c                       | 181 ++++++++++--------
 3 files changed, 171 insertions(+), 83 deletions(-)

-- 
2.43.0

[PATCH v2 1/6] ASoC: renesas: fsi: Add shared SPU clock support

[phucduc.bui]

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

Add SPU clock pointer, reference counter, and locking in fsi_master for
shared FSIA/FSIB usage, and initialize them in fsi_probe().

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

Changes in v2:
 - Add spu_count to track active users of the SPU clock.
 - Add clk_lock mutex to prevent race conditions during SPU clock
   enable/disable operations.
 - Initialize spu_count and clk_lock during driver probe.

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

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 1491c2f2cc96..196ec7bac33d 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -292,8 +292,11 @@ 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;
+	struct mutex clk_lock;
 };
 
 static inline int fsi_stream_is_play(struct fsi_priv *fsi,
@@ -1961,7 +1964,9 @@ static int fsi_probe(struct platform_device *pdev)
 
 	/* master setting */
 	master->core		= core;
+	master->spu_count	= 0;
 	spin_lock_init(&master->lock);
+	mutex_init(&master->clk_lock);
 
 	/* FSI A setting */
 	fsi		= &master->fsia;
-- 
2.43.0

[PATCH v2 2/6] ASoC: renesas: fsi: Fix hang by enabling SPU clock

[phucduc.bui]

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

Enable/disable the shared SPU clock in hw startup/shutdown. Without this,
accessing FSI registers may hang the system.

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

Changes in v2:
 - Move SPU clock enable/disable handling from fsi_dai_startup/shutdown 
   to fsi_hw_startup/shutdown

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

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 196ec7bac33d..109e06b5f32d 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -1492,6 +1492,18 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
 			  struct device *dev)
 {
 	u32 data = 0;
+	int ret = 0;
+	/* enable spu clock */
+	mutex_lock(&fsi->master->clk_lock);
+	if (fsi->master->clk_spu && fsi->master->spu_count++ == 0) {
+		ret = clk_prepare_enable(fsi->master->clk_spu);
+		if (ret < 0) {
+			fsi->master->spu_count--;
+			mutex_unlock(&fsi->master->clk_lock);
+			return ret;
+		}
+	}
+	mutex_unlock(&fsi->master->clk_lock);
 
 	/* clock setting */
 	if (fsi_is_clk_master(fsi))
@@ -1549,6 +1561,11 @@ static int fsi_hw_shutdown(struct fsi_priv *fsi,
 	/* stop master clock */
 	if (fsi_is_clk_master(fsi))
 		return fsi_clk_disable(dev, fsi);
+	/* stop spu clock */
+	mutex_lock(&fsi->master->clk_lock);
+	if (fsi->master->clk_spu && --fsi->master->spu_count == 0)
+		clk_disable_unprepare(fsi->master->clk_spu);
+	mutex_unlock(&fsi->master->clk_lock);
 
 	return 0;
 }
-- 
2.43.0

[PATCH v2 3/6] 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 109e06b5f32d..9df3e91ac79c 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -1606,9 +1606,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 v2 4/6] ASoC: renesas: fsi: refactor clock initialization

[phucduc.bui]

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

Move fsi_clk_init() out of set_fmt() and handle clock master logic
internally. This simplifies the flow and aligns with probe-time
initialization.

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

Note: Due to hardware limitations, only slave mode has been verified.
Testing master mode requires resoldering board resistors or
developing an HDMI driver, so master mode logic is currently
compile-tested only. Full support for fsidiv requires additional
DT bindings and a corresponding driver.

 sound/soc/renesas/fsi.c | 157 +++++++++++++++++++++-------------------
 1 file changed, 81 insertions(+), 76 deletions(-)

diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
index 9df3e91ac79c..db4ddc30f44f 100644
--- a/sound/soc/renesas/fsi.c
+++ b/sound/soc/renesas/fsi.c
@@ -709,73 +709,6 @@ static void fsi_spdif_clk_ctrl(struct fsi_priv *fsi, int enable)
 		fsi_core_mask_set(master, b_mclk, mask, val);
 }
 
-/*
- *		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)
 {
@@ -921,6 +854,10 @@ static int fsi_clk_set_rate_external(struct device *dev,
 	int ackmd, bpfmd;
 	int ret = 0;
 
+	if (!xck || !ick) {
+		dev_err(dev, "External (xck) or Internal (ick) clock is missing\n");
+		return -EINVAL;
+	}
 	/* check clock rate */
 	xrate = clk_get_rate(xck);
 	if (xrate % rate) {
@@ -957,6 +894,11 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
 	int ackmd, bpfmd;
 	int ret = -EINVAL;
 
+	if (!ick || !div) {
+		dev_err(dev, "Internal (ick) or Divider (div) clock is missing\n");
+		return -EINVAL;
+	}
+
 	if (!(12288000 % rate))
 		target = 12288000;
 	if (!(11289600 % rate))
@@ -1029,6 +971,76 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
 	return ret;
 }
 
+/*
+ *		clock function
+ */
+static int fsi_clk_init(struct device *dev, struct fsi_priv *fsi, int is_cpg)
+{
+	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 (is_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->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_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;
+		}
+	}
+
+	/* FSIACLK/FSIBCLK */
+	if (ick) {
+		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;
+		}
+	}
+
+	/* FSI-DIV */
+	if (div) {
+		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;
+		}
+	}
+
+	return 0;
+}
+
 static void fsi_pointer_update(struct fsi_stream *io, int size)
 {
 	io->buff_sample_pos += size;
@@ -1684,15 +1696,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);
@@ -1992,6 +1995,7 @@ 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);
+	fsi_clk_init(&pdev->dev, fsi, !!(info.port_a.flags & SH_FSI_CLK_CPG));
 	ret = fsi_stream_probe(fsi, &pdev->dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "FSIA stream probe failed\n");
@@ -2005,6 +2009,7 @@ 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);
+	fsi_clk_init(&pdev->dev, fsi, !!(info.port_b.flags & SH_FSI_CLK_CPG));
 	ret = fsi_stream_probe(fsi, &pdev->dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "FSIB stream probe failed\n");
-- 
2.43.0

[PATCH v2 5/6] 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.
Also complete the FSI clock tree by adding:
  - CPG DIV6 clocks (icka/b) as functional parents
  - External clocks (xcka/b) from the board
Define fsib nodes to support the clock hierarchy.

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

Changes in v2:
 - Rename "fsi" clock to "own" to match driver implementation.
 - Add missing clock names: "icka", "ickb", "xcka", "xckb".

 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..b8d903b711be 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 = "own", "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 v2 6/6] ASoC: dt-bindings: renesas,fsi: add support for 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 a flexible positional clock list 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.

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

Changes in v2:
 - Rename FSI module clock to "own" to match driver.
 - Add "spu", "icka/b", "diva/b", "xcka/b" clock names.
 - Use YAML anchors to constrain clock-names properly.
 - Add "if" rule to require "spu" clock for r8a7740.
 - Update example with full clock configuration.
 - Clean up schema by moving allOf location.

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

diff --git a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
index df91991699a7..d0ae54f3d321 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,36 @@ properties:
     maxItems: 1
 
   clocks:
-    maxItems: 1
+    description: |
+      Clock driving the FSI Controller. The first clock must be
+      the module clock ("own").
+    minItems: 1
+    maxItems: 8
+
+  clock-names:
+    description: |
+      Names of clocks corresponding to entries in "clocks":
+      - "own": Main FSI module clock (must be first and always present)
+      - "spu": 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.
+      - "icka" / "ickb": CPG DIV6 functional clocks for FSI port A/B
+      - "diva"/"divb": Internal FSI dividers for port A/B used for
+        audio clock generation
+      - "xcka"/"xckb": External clock inputs for FSI port A/B
+        provided by the board
+    minItems: 1
+    items:
+      - const: own
+      - &fsi_all_clks
+        enum: [spu, icka, ickb, diva, divb, xcka, xckb]
+      - &fsi_no_spu_clks
+        enum: [icka, ickb, diva, divb, xcka, xckb]
+      - *fsi_no_spu_clks
+      - *fsi_no_spu_clks
+      - *fsi_no_spu_clks
+      - *fsi_no_spu_clks
+      - *fsi_no_spu_clks
 
   power-domains:
     maxItems: 1
@@ -69,6 +95,27 @@ required:
 
 unevaluatedProperties: false
 
+allOf:
+  - $ref: dai-common.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,fsi2-r8a7740
+    then:
+      properties:
+        clock-names:
+          minItems: 2
+          items:
+            - const: own
+            - const: spu
+            - *fsi_no_spu_clks
+            - *fsi_no_spu_clks
+            - *fsi_no_spu_clks
+            - *fsi_no_spu_clks
+            - *fsi_no_spu_clks
+            - *fsi_no_spu_clks
+
 examples:
   - |
     #include <dt-bindings/clock/r8a7740-clock.h>
@@ -77,7 +124,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>, <&fsib_clk>, <&fsidiva_clk>,
+                     <&fsidivb_clk>,<&fsiack_clk>,<&fsibck_clk>;
+            clock-names = "own", "spu", "icka", "ickb",
+                          "diva", "divb", "xcka", "xckb";
             power-domains = <&pd_a4mp>;
 
             #sound-dai-cells = <1>;
-- 
2.43.0

Re: [PATCH v2 1/6] ASoC: renesas: fsi: Add shared SPU clock support

[Kuninori Morimoto]

Hi

> Add SPU clock pointer, reference counter, and locking in fsi_master for
> shared FSIA/FSIB usage, and initialize them in fsi_probe().
> 
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
(snip)
> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 1491c2f2cc96..196ec7bac33d 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -292,8 +292,11 @@ 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;
> +	struct mutex clk_lock;
>  };

You added clk_spu in this patch, but not touched.
When I checked whole patch-set, you initialize it at [4/6], but [2/6] is
using it. Maybe it works, but is strange.

The total patch orders are opposite, I think.
I think it can be...

As prepare

	- Fix trigger stop ordering
	- move fsi_clk_init()
	  - this just moves the function, no change

As adding new feature

	- remains

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 2/6] ASoC: renesas: fsi: Fix hang by enabling SPU clock

[Kuninori Morimoto]

Hi

Hi

> Enable/disable the shared SPU clock in hw startup/shutdown. Without this,
> accessing FSI registers may hang the system.
> 
> Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
(snip)
> @@ -1492,6 +1492,18 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
>  			  struct device *dev)
>  {
>  	u32 data = 0;
> +	int ret = 0;
> +	/* enable spu clock */
> +	mutex_lock(&fsi->master->clk_lock);
> +	if (fsi->master->clk_spu && fsi->master->spu_count++ == 0) {
> +		ret = clk_prepare_enable(fsi->master->clk_spu);
> +		if (ret < 0) {
> +			fsi->master->spu_count--;
> +			mutex_unlock(&fsi->master->clk_lock);
> +			return ret;
> +		}
> +	}
> +	mutex_unlock(&fsi->master->clk_lock);

1st, please insert white line between "int ret = 0;" and "/* enable spu
clock */".

2nd, besically, FSI already has "lock", and using it for several protecting.
Please re-use it, and don't add random new-lock. It makes code confusable.
Then, please use guard().

3rd, I don't like above count inc/dec, and mutex_unlock() style, because
the code unnecessarily complicated. It can be...

	int ret = 0;

	if (master->clk_spu) {
		guard(spinlock_irqsave)(&master->lock);

		if (master->spu_count == 0)
			ret = clk_prepare_enable(master->clk_spu);

		master->spu_count++;
	}
	if (ret < 0)
		return ret;

I'm not 100% sure, but I guess you need to count up spu_count anyway
regardless of clk_prepare_enable() result ?

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 3/6] ASoC: renesas: fsi: Fix trigger stop ordering

[Kuninori Morimoto]

Hi

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

This patch should appearing much earlier.

>  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 109e06b5f32d..9df3e91ac79c 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1606,9 +1606,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
> 

Re: [PATCH v2 4/6] ASoC: renesas: fsi: refactor clock initialization

[Kuninori Morimoto]

Hi

> Move fsi_clk_init() out of set_fmt() and handle clock master logic
> internally. This simplifies the flow and aligns with probe-time
> initialization.
> 
> Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
(snip)
> -/*
> - *		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))

I have mentioned in previous mail to just move fsi_clk_init(), but why do
you need to move it ? It works without any issue without moving function,
I guess ?
This patch 1) moves fsi_clk_init() 2) and update it. So there are too many
diffs. Difficult to review.

Note is that the comment /* clock function */ is not only for fsi_clk_init()
but for all fsi_clk_xxx() functions. Here is that position.


> @@ -1684,15 +1696,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);
> -	}

You removes fsi_is_clk_master() check in new fsi_clk_init() ?

> @@ -1992,6 +1995,7 @@ 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);
> +	fsi_clk_init(&pdev->dev, fsi, !!(info.port_a.flags & SH_FSI_CLK_CPG));
>  	ret = fsi_stream_probe(fsi, &pdev->dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "FSIA stream probe failed\n");
> @@ -2005,6 +2009,7 @@ 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);
> +	fsi_clk_init(&pdev->dev, fsi, !!(info.port_b.flags & SH_FSI_CLK_CPG));
>  	ret = fsi_stream_probe(fsi, &pdev->dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "FSIB stream probe failed\n");

Why don't use fsi->clk_cpg ? And why you need to call fsi_clk_init() twice ?

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 6/6] ASoC: dt-bindings: renesas,fsi: add support for multiple clocks

[Krzysztof Kozlowski]

On Mon, Apr 13, 2026 at 05:07:00PM +0700, 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 a flexible positional clock list to properly

Flexible is not allowed. Provide reasons for exception.

> 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.
> 
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
> 
> Changes in v2:
>  - Rename FSI module clock to "own" to match driver.
>  - Add "spu", "icka/b", "diva/b", "xcka/b" clock names.
>  - Use YAML anchors to constrain clock-names properly.
>  - Add "if" rule to require "spu" clock for r8a7740.
>  - Update example with full clock configuration.
>  - Clean up schema by moving allOf location.
> 
>  .../bindings/sound/renesas,fsi.yaml           | 61 +++++++++++++++++--
>  1 file changed, 56 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml
> index df91991699a7..d0ae54f3d321 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,36 @@ properties:
>      maxItems: 1
>  
>    clocks:
> -    maxItems: 1
> +    description: |
> +      Clock driving the FSI Controller. The first clock must be
> +      the module clock ("own").
> +    minItems: 1
> +    maxItems: 8
> +
> +  clock-names:
> +    description: |
> +      Names of clocks corresponding to entries in "clocks":
> +      - "own": Main FSI module clock (must be first and always present)
> +      - "spu": 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.
> +      - "icka" / "ickb": CPG DIV6 functional clocks for FSI port A/B
> +      - "diva"/"divb": Internal FSI dividers for port A/B used for
> +        audio clock generation
> +      - "xcka"/"xckb": External clock inputs for FSI port A/B
> +        provided by the board

This goes to the "clocks:"

> +    minItems: 1
> +    items:
> +      - const: own
> +      - &fsi_all_clks

I don't understand this syntax.

Best regards,
Krzysztof

Re: [PATCH v2 6/6] ASoC: dt-bindings: renesas,fsi: add support for multiple clocks

[Bui Duc Phuc]

Hi Krzysztof,

Thank you for your detailed review and feedback.

> Flexible is not allowed. Provide reasons for exception.

I understand and will remove this approach and replace it with
explicit valid clock combinations.

> This goes to the "clocks:"

Understood, I will move the description to "clocks".

> > +    minItems: 1
> > +    items:
> > +      - const: own
> > +      - &fsi_all_clks
>
> I don't understand this syntax.

Understood, I will drop the YAML anchor and use explicit constraints instead.

I will update it to the following structure:

  clocks:
    description: |
      Clock driving the FSI Controller :
      - "own": Main FSI module clock (must be first and always present)
      - "spu": 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.
      - "icka" / "ickb": CPG DIV6 functional clocks for FSI port A/B
      - "diva"/"divb": Internal FSI dividers for port A/B used for
        audio clock generation
      - "xcka"/"xckb": External clock inputs for FSI port A/B
        provided by the board
    minItems: 1
    maxItems: 8

  clock-names:

    minItems: 1
    maxItems: 8

allOf:
  - $ref: dai-common.yaml#
  - if:
      properties:
        compatible:
          contains:
            const: renesas,fsi2-r8a7740
    then:
      properties:
        clock-names:
          oneOf:
            - items:
                - const: own
                - const: spu
            - items:
                - const: own
                - const: spu
                - const: ickb
                - const: divb

Best regards,
Phuc

Re: [PATCH v2 1/6] ASoC: renesas: fsi: Add shared SPU clock support

[Bui Duc Phuc]

Hi,

Thanks for the review and explanation.

> You added clk_spu in this patch, but not touched.
> When I checked whole patch-set, you initialize it at [4/6], but [2/6] is
> using it. Maybe it works, but is strange.

You are right, clk_spu is used before being initialized.
I was not careful with the patch ordering and only ensured the series
worked as a whole.
I understand now and will fix the ordering accordingly.

Best regards,
Phuc

Re: [PATCH v2 4/6] ASoC: renesas: fsi: refactor clock initialization

[Bui Duc Phuc]

Hi Morimoto-san,

Thank you for the review.

> I have mentioned in previous mail to just move fsi_clk_init(), but why do
> you need to move it ? It works without any issue without moving function,
> I guess ?

I moved fsi_clk_init() below the two functions fsi_clk_set_rate_cpg
and fsi_clk_set_rate_external because, inside fsi_clk_init(),
I assign these functions to clock->set_rate. Moving the function was
necessary to avoid compilation errors.

+       if (is_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;
+       }

Would you prefer that I use forward declarations instead of changing
the function order?

> Note is that the comment /* clock function */ is not only for fsi_clk_init()
> but for all fsi_clk_xxx() functions. Here is that position.

Understood, I will fix the comment placement accordingly.

> > -     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);
> > -     }
>
> You removes fsi_is_clk_master() check in new fsi_clk_init() ?

At the probe stage, the Master/Slave status has not yet been determined
because it depends on a subsequent set_fmt() call. Therefore, I am not using
the fsi_is_clk_master() function inside the new fsi_clk_init() during
the probe process.

Instead, the new fsi_clk_init() function acquires all resources
(including the mandatory SPU clock) upfront using
devm_clk_get_optional().
The actual fsi_is_clk_master() check remains strictly enforced in
fsi_hw_startup() before enabling any functional clocks.

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

> Why don't use fsi->clk_cpg ?

You're right, using fsi->clk_cpg is cleaner since it's already
initialized in fsi_port_info_init().
 I will use it in the next version.

> And why you need to call fsi_clk_init() twice ?
The FSI controller has two independent ports (Port A and Port B).
Each port requires its own clock resource initialization and configuration.

Best regards,
Phuc

Re: [PATCH v2 4/6] ASoC: renesas: fsi: refactor clock initialization

[Kuninori Morimoto]

Hi Bui

> > I have mentioned in previous mail to just move fsi_clk_init(), but why do
> > you need to move it ? It works without any issue without moving function,
> > I guess ?
> 
> I moved fsi_clk_init() below the two functions fsi_clk_set_rate_cpg
> and fsi_clk_set_rate_external because, inside fsi_clk_init(),
> I assign these functions to clock->set_rate. Moving the function was
> necessary to avoid compilation errors.

Ah, OK.
So the patch 1) moves fsi_clk_init() and 2) update it.
It is including many features in 1 patch. Please separate it.

One note here is that /* clock function */ is for all fsi_clk_xxx(),
so don't move it.

> > And why you need to call fsi_clk_init() twice ?
> The FSI controller has two independent ports (Port A and Port B).
> Each port requires its own clock resource initialization and configuration.

Ah, yes indeed.


Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 2/6] ASoC: renesas: fsi: Fix hang by enabling SPU clock

[Bui Duc Phuc]

Hi Morimoto-san,

Thank you for your detailed review and feedback.

> 1st, please insert white line between "int ret = 0;" and "/* enable spu
> clock */".
>
> 2nd, besically, FSI already has "lock", and using it for several protecting.
> Please re-use it, and don't add random new-lock. It makes code confusable.
> Then, please use guard().

I will fix the coding style and use
guard(spinlock_irqsave)(&master->lock) in v3.
It’s much better than adding a new lock.

> 3rd, I don't like above count inc/dec, and mutex_unlock() style, because
> the code unnecessarily complicated. It can be...
>
>       int ret = 0;
>
>         if (master->clk_spu) {
>                 guard(spinlock_irqsave)(&master->lock);
>
>                 if (master->spu_count == 0)
>                         ret = clk_prepare_enable(master->clk_spu);
>
>                 master->spu_count++;
>         }
>         if (ret < 0)
>                 return ret;
>
> I'm not 100% sure, but I guess you need to count up spu_count anyway
> regardless of clk_prepare_enable() result ?

Regarding spu_count, I’m not entirely sure, but if we increment it
even on failure,
the counter might become unbalanced and clk_prepare_enable() may not
be retried on the next call.
Would it be better to increment spu_count only on success to keep the
state consistent?

Also, I have a question about the context here.
Since fsi_hw_startup() and fsi_hw_shutdown() are called from fsi_dai_trigger(),
I think this runs in an atomic context, but please correct me if I'm wrong.
If so, is it safe to call clk_prepare_enable() under guard(spinlock_irqsave)?
Since clk_prepare() can sleep, I’m wondering if this could potentially
cause a "scheduling while atomic" issue.
Would it make more sense to move clk_prepare() to init time (in new
fsi_clk_init() ),
and only use clk_enable() / clk_disable() in the trigger path?

Best regards,
Phuc

On Tue, Apr 14, 2026 at 7:27 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>
>
> Hi
>
> Hi
>
> > Enable/disable the shared SPU clock in hw startup/shutdown. Without this,
> > accessing FSI registers may hang the system.
> >
> > Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> > ---
> (snip)
> > @@ -1492,6 +1492,18 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
> >                         struct device *dev)
> >  {
> >       u32 data = 0;
> > +     int ret = 0;
> > +     /* enable spu clock */
> > +     mutex_lock(&fsi->master->clk_lock);
> > +     if (fsi->master->clk_spu && fsi->master->spu_count++ == 0) {
> > +             ret = clk_prepare_enable(fsi->master->clk_spu);
> > +             if (ret < 0) {
> > +                     fsi->master->spu_count--;
> > +                     mutex_unlock(&fsi->master->clk_lock);
> > +                     return ret;
> > +             }
> > +     }
> > +     mutex_unlock(&fsi->master->clk_lock);
>
> 1st, please insert white line between "int ret = 0;" and "/* enable spu
> clock */".
>
> 2nd, besically, FSI already has "lock", and using it for several protecting.
> Please re-use it, and don't add random new-lock. It makes code confusable.
> Then, please use guard().
>
> 3rd, I don't like above count inc/dec, and mutex_unlock() style, because
> the code unnecessarily complicated. It can be...
>
>         int ret = 0;
>
>         if (master->clk_spu) {
>                 guard(spinlock_irqsave)(&master->lock);
>
>                 if (master->spu_count == 0)
>                         ret = clk_prepare_enable(master->clk_spu);
>
>                 master->spu_count++;
>         }
>         if (ret < 0)
>                 return ret;
>
> I'm not 100% sure, but I guess you need to count up spu_count anyway
> regardless of clk_prepare_enable() result ?
>
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto

Re: [PATCH v2 3/6] ASoC: renesas: fsi: Fix trigger stop ordering

[Bui Duc Phuc]

Hi Morimoto-san,

> This patch should appearing much earlier.
Thanks for your guidance.
I will reorder the patch series to place this fix earlier in the sequence.

Best regards,
Phuc

On Tue, Apr 14, 2026 at 7:28 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>
>
> Hi
>
> > 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>
> > ---
>
> This patch should appearing much earlier.
>
> >  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 109e06b5f32d..9df3e91ac79c 100644
> > --- a/sound/soc/renesas/fsi.c
> > +++ b/sound/soc/renesas/fsi.c
> > @@ -1606,9 +1606,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
> >

Re: [PATCH v2 4/6] ASoC: renesas: fsi: refactor clock initialization

[Bui Duc Phuc]

Hi Morimoto-san,

> Ah, OK.
> So the patch 1) moves fsi_clk_init() and 2) update it.
> It is including many features in 1 patch. Please separate it.
>
> One note here is that /* clock function */ is for all fsi_clk_xxx(),
> so don't move it.

Thank you for your guidance.
I will split this into two separate patches accordingly.

Best regards,
Phuc

On Wed, Apr 15, 2026 at 11:55 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>
>
> Hi Bui
>
> > > I have mentioned in previous mail to just move fsi_clk_init(), but why do
> > > you need to move it ? It works without any issue without moving function,
> > > I guess ?
> >
> > I moved fsi_clk_init() below the two functions fsi_clk_set_rate_cpg
> > and fsi_clk_set_rate_external because, inside fsi_clk_init(),
> > I assign these functions to clock->set_rate. Moving the function was
> > necessary to avoid compilation errors.
>
> Ah, OK.
> So the patch 1) moves fsi_clk_init() and 2) update it.
> It is including many features in 1 patch. Please separate it.
>
> One note here is that /* clock function */ is for all fsi_clk_xxx(),
> so don't move it.
>
> > > And why you need to call fsi_clk_init() twice ?
> > The FSI controller has two independent ports (Port A and Port B).
> > Each port requires its own clock resource initialization and configuration.
>
> Ah, yes indeed.
>
>
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto