ASoC: fsl_sai: Address several issues in the driver

From: bui duc phuc <[email protected]>

Hi all,

I've continued reviewing the FSL SAI driver and referred to the i.MX RT1180 Reference Manual when looking into its register configurations and behavior:

https://www.nxp.com/docs/en/reference-manual/IMXRT1180RM.pdf

Link v1:
https://lore.kernel.org/all/[email protected]/
Change in v2:
- Add five patches covering disable handling, documentation, ISR error handling, BCLK ratio validation,
and preventing incorrect CHMOD writes to the other direction's xCR4.

Compile-tested only.

Best regards,
Phuc

bui duc phuc (10):
ASoC: fsl_sai: Fix PM QoS cleanup on resume failure
ASoC: fsl_sai: Use optional clock getter for MCLKs
ASoC: fsl_sai: Remove redundant error messages
ASoC: fsl_sai: Propagate device tree property read errors
ASoC: fsl_sai: Drop redundant dataline mask AND
ASoC: fsl_sai: Warn if TE/RE does not clear on disable
ASoC: fsl_sai: Clarify fsl_sai_dir_is_synced() documentation
ASoC: fsl_sai: Check regmap_read() return value in ISR
ASoC: fsl_sai: Reject bclk ratio smaller than slot width
ASoC: fsl_sai: Don't write CHMOD to the other direction's xCR4

 sound/soc/fsl/fsl_sai.c | 89 +++++++++++++++++++++++------------------
 1 file changed, 49 insertions(+), 40 deletions(-)

2.43.0

From: bui duc phuc <[email protected]>

If regcache_sync() fails during runtime resume, the CPU latency QoS request remains active even though the resume operation failed.

Switch the regmap back to cache-only mode and remove the PM QoS request before disabling the clocks, ensuring that the resources enabled earlier in the resume path are properly cleaned up.

---
 sound/soc/fsl/fsl_sai.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

index d232c8f53061..042885ba415e 100644
@@ -1981,7 +1981,7 @@ static int fsl_sai_runtime_resume(struct device *dev)
 
 	ret = regcache_sync(sai->regmap);
 	if (ret)
-		goto disable_rx_clk;
+		goto remove_qos;
 
 	if (sai->soc_data->mclk_with_tere && sai->mclk_direction_output)
 		regmap_update_bits(sai->regmap, FSL_SAI_TCSR(ofs),

@@ -1989,7 +1989,12 @@ static int fsl_sai_runtime_resume(struct device *dev)
 
 	return 0;
 
-disable_rx_clk:
+remove_qos:
+	regcache_cache_only(sai->regmap, true);
+
+	if (sai->soc_data->flags & PMQOS_CPU_LATENCY)
+		cpu_latency_qos_remove_request(&sai->pm_qos_req);
+
 	if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_CAPTURE))
 		clk_disable_unprepare(sai->mclk_clk[sai->mclk_id[0]]);
 disable_tx_clk:

2.43.0

From: bui duc phuc <[email protected]>

The current code uses devm_clk_get() and sets the clock pointer to NULL when it returns an error. However, this silently ignores errors such as -EPROBE_DEFER, allowing the driver to continue probing without the requested clock.

Use devm_clk_get_optional() to handle missing optional clocks automatically, and dev_err_probe() to properly propagate other errors, including -EPROBE_DEFER.

---
 sound/soc/fsl/fsl_sai.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

index 042885ba415e..44a34f48a0cf 100644
@@ -1589,12 +1589,10 @@ static int fsl_sai_probe(struct platform_device *pdev)
 
 	for (i = 1; i < FSL_SAI_MCLK_MAX; i++) {
 		sprintf(tmp, "mclk%d", i);
-		sai->mclk_clk[i] = devm_clk_get(dev, tmp);
-		if (IS_ERR(sai->mclk_clk[i])) {
-			dev_err(dev, "failed to get mclk%d clock: %ld\n",
-					i, PTR_ERR(sai->mclk_clk[i]));
-			sai->mclk_clk[i] = NULL;
-		}
+		sai->mclk_clk[i] = devm_clk_get_optional(dev, tmp);
+		if (IS_ERR(sai->mclk_clk[i]))
+			return dev_err_probe(dev, PTR_ERR(sai->mclk_clk[i]),
+					     "failed to get mclk%d clock\n", i);
 	}
 
 	if (sai->soc_data->mclk0_is_mclk1)

2.43.0

From: bui duc phuc <[email protected]>

The errors are either reported directly by these functions or already reported deeper in the call chain, so additional dev_err() and dev_warn() calls are unnecessary.

---
 sound/soc/fsl/fsl_sai.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

index 44a34f48a0cf..5532fab80be0 100644
@@ -1615,10 +1615,8 @@ static int fsl_sai_probe(struct platform_device *pdev)
 
 	/* read dataline mask for rx and tx*/
 	ret = fsl_sai_read_dlcfg(sai);
-	if (ret < 0) {
-		dev_err(dev, "failed to read dlcfg %d\n", ret);
+	if (ret < 0)
 		return ret;
-	}
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)

@@ -1698,9 +1696,7 @@ static int fsl_sai_probe(struct platform_device *pdev)
 	if (ret < 0)
 		dev_warn(dev, "Error reading SAI version: %d\n", ret);
 
-	ret = fsl_sai_reset_hw(dev);
-	if (ret < 0)
-		dev_warn(dev, "Failed to reset hardware: %d\n", ret);
+	fsl_sai_reset_hw(dev);
 
 	/* Select MCLK direction */
 	if (sai->mclk_direction_output &&

@@ -1715,10 +1711,8 @@ static int fsl_sai_probe(struct platform_device *pdev)
 
 	ret = devm_request_irq(dev, irq, fsl_sai_isr, IRQF_SHARED,
 			       np->name, sai);
-	if (ret) {
-		dev_err(dev, "failed to claim irq %u\n", irq);
+	if (ret)
 		goto err_pm_get_sync;
-	}
 
 	if (of_device_is_compatible(np, "fsl,imx952-sai") &&
 	    !of_property_read_string(np, "fsl,sai-amix-mode", &str)) {

2.43.0

Hi Mark,

Thank you for your review.

You're right.
I'll change fsl_sai_reset_hw() to return void.
Since this series has already been applied,
I'll send this as a separate patch.

From what I can see in the code, devm_request_irq() -> devm_request_threaded_irq() reports every failure via dev_err_probe() in devm_request_result(), so -EBUSY should be printed too.

Could you please point me to the path that fails silently? I may have missed it.

Or did you mean fsl_sai_read_dlcfg()? It does have some silent -EINVAL returns.
I'll restore that dev_err() together with the fsl_sai_reset_hw() change I mentioned earlier.

Best regards,
Phuc

Ah, that's changed since I last looked.

That too.

From: bui duc phuc <[email protected]>

Return the original error codes from of_property_read_u32_index() instead of replacing them with -EINVAL.

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

index 5532fab80be0..cb07e7d13132 100644
@@ -1495,15 +1495,15 @@ static int fsl_sai_read_dlcfg(struct fsl_sai *sai)
 		 */
 		ret = of_property_read_u32_index(np, propname, index++, &type);
 		if (ret)
-			return -EINVAL;
+			return ret;
 
 		ret = of_property_read_u32_index(np, propname, index++, &rx);
 		if (ret)
-			return -EINVAL;
+			return ret;
 
 		ret = of_property_read_u32_index(np, propname, index++, &tx);
 		if (ret)
-			return -EINVAL;
+			return ret;
 
 		if ((rx & ~soc_dl) || (tx & ~soc_dl)) {
 			dev_err(dev, "dataline cfg[%d] setting error, mask is 0x%x\n", i, soc_dl);

2.43.0

From: bui duc phuc <[email protected]>

The rx/tx masks with bits outside soc_dl are already rejected just above, so masking them with soc_dl again is no-op. Remove it.

---
 sound/soc/fsl/fsl_sai.c | 3 ---
 1 file changed, 3 deletions(-)

index cb07e7d13132..8315a0d8893b 100644
@@ -1510,9 +1510,6 @@ static int fsl_sai_read_dlcfg(struct fsl_sai *sai)
 			return -EINVAL;
 		}
 
-		rx = rx & soc_dl;
-		tx = tx & soc_dl;
-
 		cfg[i].type = type;
 		cfg[i].pins[0] = hweight8(rx);
 		cfg[i].mask[0] = rx;

2.43.0

From: bui duc phuc <[email protected]>

fsl_sai_config_disable() polls TE/RE with an open-coded loop and continues silently if the bit does not clear within 1ms. Also, if regmap_read() fails, the loop condition evaluates xcsr uninitialized.

Use regmap_read_poll_timeout_atomic() and warn on timeout.

---
 sound/soc/fsl/fsl_sai.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

index 8315a0d8893b..f75a1d61d7b7 100644
@@ -849,7 +849,8 @@ static void fsl_sai_config_disable(struct fsl_sai *sai, int dir)
 {
 	unsigned int ofs = sai->soc_data->reg_offset;
 	bool tx = dir == TX;
-	u32 xcsr, count = 100, mask;
+	u32 xcsr, mask;
+	int ret;
 
 	if (sai->soc_data->mclk_with_tere && sai->mclk_direction_output)
 		mask = FSL_SAI_CSR_TERE;

@@ -860,10 +861,15 @@ static void fsl_sai_config_disable(struct fsl_sai *sai, int dir)
 			   mask, 0);
 
 	/* TERE will remain set till the end of current frame */
-	do {
-		udelay(10);
-		regmap_read(sai->regmap, FSL_SAI_xCSR(tx, ofs), &xcsr);
-	} while (--count && xcsr & FSL_SAI_CSR_TERE);
+	ret = regmap_read_poll_timeout_atomic(sai->regmap, FSL_SAI_xCSR(tx, ofs),
+					      xcsr, !(xcsr & FSL_SAI_CSR_TERE),
+					      10, 1000);
+	if (ret == -ETIMEDOUT)
+		dev_warn(&sai->pdev->dev, "%cx not disabled at frame end\n",
+			 tx ? 'T' : 'R');
+	else if (ret)
+		dev_err(&sai->pdev->dev, "failed to read %cSCR : %d\n",
+			tx ? 'T' : 'R', ret);
 
 	regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
 			   FSL_SAI_CSR_FR, FSL_SAI_CSR_FR);

2.43.0

Hi Mark,

Thank you for your reviews.

That's correct. Since this function returns void, previous failures and abnormal hardware conditions were silently ignored. I added a warning for timeouts and an error message for regmap read failures to make these issues visible and easier to diagnose.

Best regards,
Phuc

From: bui duc phuc <[email protected]>

The summary line of fsl_sai_dir_is_synced() reads as if @dir follows the clocks of the opposite stream, while the function returns true when @dir provides the clocks and the opposite stream is synchronous.

Reword the description to match the behavior and document the return value. No functional change.

---
 sound/soc/fsl/fsl_sai.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

index f75a1d61d7b7..25243b6859d6 100644
@@ -88,14 +88,16 @@ static const struct snd_kcontrol_new fsl_sai_timestamp_ctrls[] = {
 };
 
 /**
- * fsl_sai_dir_is_synced - Check if stream is synced by the opposite stream
- *
- * SAI supports synchronous mode using bit/frame clocks of either Transmitter's
- * or Receiver's for both streams. This function is used to check if clocks of
- * the stream's are synced by the opposite stream.
- *
+ * fsl_sai_dir_is_synced - Check if the opposite stream is synced to @dir
  * @sai: SAI context
- * @dir: stream direction
+ * @dir: stream direction, TX or RX
+ *
+ * SAI supports synchronous mode, where one direction uses the bit clock and
+ * frame sync of the other. This function checks whether @dir provides the
+ * clocks, i.e. @dir runs in asynchronous mode while the opposite direction
+ * runs in synchronous mode.
+ *
+ * Return: true if the opposite direction uses the clocks of @dir.
  */
 static inline bool fsl_sai_dir_is_synced(struct fsl_sai *sai, int dir)
 {

2.43.0

From: bui duc phuc <[email protected]>

The IRQ is shared, so fsl_sai_isr() may run while the SAI is runtime suspended. The regmap is then in cache-only mode, so regmap_read() fails and xcsr is used uninitialized.

Check the return value of regmap_read() for both TCSR and RCSR, and skip the handling of the corresponding direction on failure.

---
 sound/soc/fsl/fsl_sai.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 25243b6859d6..5c50ed7c2258 100644
@@ -148,7 +148,8 @@ static irqreturn_t fsl_sai_isr(int irq, void *devid)
 	mask = (FSL_SAI_FLAGS >> FSL_SAI_CSR_xIE_SHIFT) << FSL_SAI_CSR_xF_SHIFT;
 
 	/* Tx IRQ */
-	regmap_read(sai->regmap, FSL_SAI_TCSR(ofs), &xcsr);
+	if (regmap_read(sai->regmap, FSL_SAI_TCSR(ofs), &xcsr))
+		return IRQ_NONE;
 	flags = xcsr & mask;
 
 	if (flags)

@@ -179,7 +180,8 @@ static irqreturn_t fsl_sai_isr(int irq, void *devid)
 
 irq_rx:
 	/* Rx IRQ */
-	regmap_read(sai->regmap, FSL_SAI_RCSR(ofs), &xcsr);
+	if (regmap_read(sai->regmap, FSL_SAI_RCSR(ofs), &xcsr))
+		goto out;
 	flags = xcsr & mask;
 
 	if (flags)

2.43.0

From: bui duc phuc <[email protected]>

In hw_params(), when a bclk ratio is set, the number of slots is computed as bclk_ratio / slot_width. If the ratio is smaller than the slot width, slots becomes zero and the following DIV_ROUND_UP(channels, slots) divides by zero.

Return -EINVAL in this case.

---
 sound/soc/fsl/fsl_sai.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

index 5c50ed7c2258..c61703ddb6f8 100644
@@ -632,8 +632,14 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 
 	if (sai->slots[tx])
 		slots = sai->slots[tx];
-	else if (sai->bclk_ratio)
+	else if (sai->bclk_ratio) {
+		if (sai->bclk_ratio < slot_width) {
+			dev_err(cpu_dai->dev, "bclk ratio %u < slot width %u\n",
+				sai->bclk_ratio, slot_width);
+			return -EINVAL;
+		}
 		slots = sai->bclk_ratio / slot_width;
+	}
 
 	pins = DIV_ROUND_UP(channels, slots);
 

2.43.0

From: bui duc phuc <[email protected]>

When the current direction is synced to the other one, hw_params() also writes the frame configuration to the other direction's xCR4. The mask includes CHMOD, whose value is set by the current direction (Tx only) but written to the other one, so it is wrong:
- TCR4[CHMOD] is cleared, while Tx needs it set (output mode).
- RCR4 bit 5 is set, while it is reserved and must be written as 0.

CHMOD is not part of the frame configuration, so drop it from the mask.

---
 sound/soc/fsl/fsl_sai.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index c61703ddb6f8..430e896d064e 100644
@@ -720,8 +720,7 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 
 	if (fsl_sai_dir_is_synced(sai, adir)) {
 		regmap_update_bits(sai->regmap, FSL_SAI_xCR4(!tx, ofs),
-				   FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK |
-				   FSL_SAI_CR4_CHMOD_MASK,
+				   FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK,
 				   val_cr4);
 		regmap_update_bits(sai->regmap, FSL_SAI_xCR5(!tx, ofs),
 				   FSL_SAI_CR5_WNW_MASK | FSL_SAI_CR5_W0W_MASK |

2.43.0

The series looks good to me.

Best regards
Shengjiu Wang

Hi Shengjiu Wang

Thanks for reviewing the series!

I'll continue looking into other parts of the FSL audio drivers, such as ASRC, PDM, and SPDIF, next week.

Best regards,
Phuc

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-7.5

Thanks!

[01/10] ASoC: fsl_sai: Fix PM QoS cleanup on resume failure

        https://git.kernel.org/broonie/sound/c/f804ad135529
[02/10] ASoC: fsl_sai: Use optional clock getter for MCLKs
        https://git.kernel.org/broonie/sound/c/ee95db533522
[03/10] ASoC: fsl_sai: Remove redundant error messages
        https://git.kernel.org/broonie/sound/c/b73124cd7b91
[04/10] ASoC: fsl_sai: Propagate device tree property read errors
        https://git.kernel.org/broonie/sound/c/702a9978d580
[05/10] ASoC: fsl_sai: Drop redundant dataline mask AND
        https://git.kernel.org/broonie/sound/c/a11ee78a0b25
[06/10] ASoC: fsl_sai: Warn if TE/RE does not clear on disable
        https://git.kernel.org/broonie/sound/c/db36f0b28b11
[07/10] ASoC: fsl_sai: Clarify fsl_sai_dir_is_synced() documentation
        https://git.kernel.org/broonie/sound/c/be10761cad31
[08/10] ASoC: fsl_sai: Check regmap_read() return value in ISR
        https://git.kernel.org/broonie/sound/c/b485bfcb12a8
[09/10] ASoC: fsl_sai: Reject bclk ratio smaller than slot width
        https://git.kernel.org/broonie/sound/c/279ea69e6df5
[10/10] ASoC: fsl_sai: Don't write CHMOD to the other direction's xCR4
        https://git.kernel.org/broonie/sound/c/9580e6ecce63

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark