* Re: [PATCH] dmaengine: qcom: bam_dma: keep remotely controlled units on during boot
2025-05-05 12:17 ` [PATCH] dmaengine: qcom: bam_dma: keep remotely controlled units on during boot Dmitry Baryshkov
@ 2025-05-05 13:06 ` Stephan Gerhold
0 siblings, 0 replies; 2+ messages in thread
From: Stephan Gerhold @ 2025-05-05 13:06 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Greg Kroah-Hartman, Stephen Boyd, Michael Turquette, Vinod Koul,
Caleb Connolly, linux-arm-msm, dmaengine, linux-kernel, linux-clk,
linux-serial
On Mon, May 05, 2025 at 03:17:00PM +0300, Dmitry Baryshkov wrote:
> On Mon, May 05, 2025 at 10:56:56AM +0200, Stephan Gerhold wrote:
> > 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?
>
> My understanding is that for locally controlled BAMs we can disable the
> clock at the probe time as all the users of the BAM will be probed
> before accessing the BAM. In case of a remotely controlled BAM there can
> be a user (e.g. UART) which is running, but didn't request DMA channel
> yet.
>
> Please correct me if I'm wrong here.
>
Yes, I think there is a misunderstanding:
- UART doesn't use DMA for earlycon (and I'm not aware of any boot
firmware using DMA for UART either). The bam_dma driver really works
as intended here, it votes for the clock during probe to perform the
register access, then it drops the vote when going to runtime
suspend. We don't really need .sync_state() for the BAM, because
there are no active DMA consumers during boot.
- All of the BLSP peripherals (BAM DMA, UART, I2C, SPI) need the
shared GCC_BLSP1_AHB_CLK for register access. What happens is:
1. GCC_BLSP1_AHB_CLK is left running by the bootloader.
2. earlycon makes use the UART registers (without DMA), but doesn't
vote for the clock (also: the clock driver isn't probed yet).
3. GCC driver probes, leaves GCC_BLSP1_AHB_CLK running since unused
clock cleanup happens later.
4. bam_dma probes, requests GCC_BLSP1_AHB_CLK, votes for enable to
access registers during probe.
5. bam_dma goes to runtime suspend. The last (known) consumer of
GCC_BLSP1_AHB_CLK is gone, so the clock is disabled.
6. earlycon continues accessing the UART registers.
[Device reset]
7. The full UART driver probes and votes for GCC_BLSP1_AHB_CLK.
Conceptionally, we don't need .sync_state for the BAM, we need some kind
of .sync_state for the GCC_BLSP1_AHB_CLK. Or we need earlycon to vote
for the UART clocks as soon as the clock driver probes. Which is roughly
what drivers/clk/imx/clk.c imx_register_uart_clocks() does, except that
releasing the vote on late_initcall_sync() could still be too early.
(It's a stupid configuration, but what if UART needs some modules to
probe successfully?)
> > 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, ...).
>
> This probably means that the definition of the flag needs to be
> clarified and maybe some of those platforms should use it.
>
Yeah, probably. It would require testing and/or checking downstream
sources though. Here is a slightly clearer definition for the flags:
- Controlled-remotely = reset+initialized outside of Linux, skip reset
and initialization of global BAM registers.
- Powered-remotely = powered on outside of Linux, but global BAM
registers must be reset and initialized inside Linux.
And for the clock:
- Clock specified: We can vote for the BAM to power on by turning on
the clock. After that, we can access registers during probe.
- Clock missing: BAM power management is integrated into the consumer
interface (e.g. SLIMbus, BAM DMUX). We cannot access registers until
the consumer requests the DMA channel.
I think the BLSP DMA (for UART/I2C/SPI) is often "controlled-remotely",
because there is the option to use some of the instances inside one of
the remoteprocs (RPM/Modem/WCNSS/ADSP/etc). It's not our job to keep the
BAM on though, the GCC_BLSP1_AHB_CLK is a clock that every remoteproc
can vote for independently.
> > 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.
>
> Well... It is a commit which broke earlycon on on db410c.
>
FWIW, earlycon works for me on 6.15-rc4 on DB410c, I've only run into
this issue after adding the MPM definition, which changes probe order
enough to trigger this issue. I'm not sure how that commit changes
anything, will try reverting it for my setup later to see what happens.
> I started to describe here the usecase of the remotely-controlled DMA
> controller being used by the BLSP and then I understood, that I myself
> don't completely understand if the issue is because DMA block is
> controlled remotely (and we should not be disabling it because the BLSP
> still attempts to use it) or if it's a simple case of the clock being
> shared between several consumers and one of the consumers shutting it
> down before other running consumers had a chance to vote on it.
>
The latter, as explained above. The DMA block doesn't play any role
here, it's not used during boot.
> > 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.
>
> Please share your patch.
>
It's more or less equivalent to yours with some if checks removed.
I would be in favor of leaving the bam_dma driver as-is and solving this
on the clock/earlycon level.
> >
> > 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 ...?
>
> The spi-qup commit looks like another stop-gap workaround. Let's add CCF
> and serial maintainers to the discussion with the hope of finding some
> generic solution.
>
> Most likely the easiest solution for Qualcomm platforms is to add
> additional vote on earlycon clocks and then to try to generalise that.
>
Yeah, that would be the best option I think.
Thanks,
Stephan
^ permalink raw reply [flat|nested] 2+ messages in thread