Re: [PATCH 3/3] ASoC: renesas: fsi: Fix hang by enabling SPU clock

[Kuninori Morimoto]

Hi Bui

> Yes, enabling this clock is essential as it functions as a bus bridge clock.
> Currently, the SPU clock is still enabled by the bootloader. In legacy
> kernels (v4.2 and earlier) using the Armadillo board-file/defconfig, this
> clock remained active after boot, allowing the FSI to function correctly.
> However, after migrating to a full Device Tree (DTS) implementation,
> the kernel's unused clock cleanup mechanism disables the SPU clock
> because it isn't explicitly claimed. This leads to a system hang every
> time aplay is executed, as the FSI registers become inaccessible
> without this clock.

Thank you for clarify the situation.
I didn't notice about this.

> I previously attempted to manage the clock within fsi_hw_startup/
> shutdown, but the system would hang when stopping aplay
> (e.g., via Ctrl+C). This happens because certain cleanup operations,
> such as fsi_irq_disable(), are performed after fsi_hw_shutdown()
> finishes. These operations require register access, which triggers a
> system hang if the SPU clock has already been disabled. Therefore,
> I moved the clock management to fsi_dai_startup/shutdown to ensure
> the clock remains active throughout the entire lifecycle of the stream.

Hmm ?
fsi_irq_disable() is called after fsi_hw_shutdown() ??
Ah... is it because PIO transfer ?
I have 100% forgotten, but FSI doesn't support RX DMA...

Hmm... fsi_dai_trigger() seems strange.
It seems (A) stops clock, and (B) sets register after that.
Is this the reason why you get error ? I think (A) and (B) should be
reversed. The balance between SNDRV_PCM_TRIGGER_START, and with
__fsi_suspend() are also not good.
If so, can you use hw_start/stop() ?

	static int fsi_dai_trigger(...)
	{
		...
		switch (cmd) {
			...
		case SNDRV_PCM_TRIGGER_STOP:
			if (!ret)
(A)				ret = fsi_hw_shutdown(fsi, dev);
(B)			fsi_stream_stop(fsi, io);
(C)			fsi_stream_quit(fsi, io);
			break;
		}
		...
	}

> Furthermore, my testing shows that using dai_startup/shutdown
> eliminates the need for explicit Suspend/Resume handling for this clock.
> Since the ALSA framework typically invokes the hw_ callbacks during
> power management transitions rather than the dai_ ones, the SPU clock
> state remains stable, preventing any illegal register access during
> these transitions.

Basically, concept of this driver is that power/clock is enabled/disabled
when trigger() was called (except suspend/resume).
While your testing may be correct from an ALSA point of view, but setting
configuring it in multiple places will lead to confusion.

Thank you for your help !!

Best regards
---
Kuninori Morimoto