Re: [PATCH] dmaengine: qcom: bam_dma: keep remotely controlled units on during boot

[Stephan Gerhold <stephan.gerhold@linaro.org>]

On Sat, May 03, 2025 at 03:41:43AM +0300, Dmitry Baryshkov wrote:
> The commit 0ac9c3dd0d6f ("dmaengine: qcom: bam_dma: fix runtime PM
> underflow") made sure the BAM DMA device gets suspended, disabling the
> bam_clk. However for remotely controlled BAM DMA devices the clock might
> be disabled prematurely (e.g. in case of the earlycon this frequently
> happens before UART driver is able to probe), which causes device reset.
> 
> Use sync_state callback to ensure that bam_clk stays on until all users
> are probed (and are able to vote upon corresponding clocks).
> 
> Fixes: 0ac9c3dd0d6f ("dmaengine: qcom: bam_dma: fix runtime PM underflow")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

Thanks for the patch! I actually created almost the same patch on
Friday, after struggling with this issue on DB410c when trying to add
the MPM as wakeup-parent for GPIOs. :-)

How is this issue related to _remotely-controlled_ BAMs?

The BAM clock will get disabled for all types of BAM control, so I don't
think the type of BAM control plays any role here. The BLSP DMA instance
that would most likely interfere with UART earlycon is
controlled-remotely on some SoCs (e.g. MSM8916), but currently not all
of them (e.g. MSM8974, IPQ8074, IPQ9574, ...).

The fixes tag also doesn't look correct to me, since commit 0ac9c3dd0d6f
("dmaengine: qcom: bam_dma: fix runtime PM underflow") only changed the
behavior for BAMs with "if (!bdev->bamclk)". This applies to some/most
remotely-controlled BAMs, but the issue we have here occurs only because
we do have a clock and cause it to get disabled prematurely.

Checking for if (bdev->bamclk) would probably make more sense. In my
patch I did it just unconditionally, because runtime PM is currently
a no-op for BAMs without clock anyway.

I think it's also worth noting in the commit message that this is sort
of a stop-gap solution. The root problem is that the earlycon code
doesn't claim the clock while active. Any of the drivers that consume
this shared clock could trigger the issue, I had to fix a similar issue
in the spi-qup driver before in commit 0c331fd1dccf ("spi: qup: Request
DMA before enabling clocks"). On some SoCs (e.g. MSM8974), we have
"dmas" currently only on &blsp2_i2c5, so the UART controller wouldn't
even be considered as consumer to wait for before calling the bam_dma
.sync_state.

It may be more reliable to implement something like in
drivers/clk/imx/clk.c imx_register_uart_clocks(), which tries to claim
only the actually used UART clocks until late_initcall_sync(). That
would at least make it independent from individual drivers, but assumes
the UART driver can actually probe before late_initcall_sync() ...
Most of this code is generic though, so perhaps releasing the clocks
could be hooked up somewhere generic, when earlycon exits ...?

Thanks,
Stephan