RE: [PATCH v4 14/17] dmaengine: sh: rz-dmac: Add suspend to RAM support

[Biju Das]

Hi Claudiu,

> -----Original Message-----
> From: Claudiu <claudiu.beznea@xxxxxxxxx>
> Sent: 11 April 2026 12:43
> Subject: [PATCH v4 14/17] dmaengine: sh: rz-dmac: Add suspend to RAM support
> 
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> 
> The Renesas RZ/G3S SoC supports a power saving mode in which power to most of the SoC components is
> turned off, including the DMA IP. Add suspend to RAM support to save and restore the DMA IP registers.
> 
> Cyclic DMA channels require special handling. Since they can be paused and resumed during system
> suspend/resume, the driver restores additional registers for these channels during the system resume
> phase. If a channel was not explicitly paused during suspend, the driver ensures that it is paused and
> resumed as part of the system suspend/resume flow. This might be the case of a serial device being used
> with no_console_suspend.
> 
> For non-cyclic channels, the dev_pm_ops::prepare callback waits for all the ongoing transfers to
> complete before allowing suspend-to-RAM to proceed.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> ---
> 
> Changes in v4:
> - in rz_dmac_device_synchronize() kept the read_poll_timeout() as
>   this doesn't fail anymore with the proper status return from
>   ->device_tx_status() API in case the channel is paused; with it
>   the patch description was updated
> - keep the cleanup path in rz_dmac_suspend() simpler to avoid
>   confusion when using guard()
> - used SYSTEM_SLEEP_PM_OPS() as there is no need for having the
>   suspend/resume callbacks being called in NOIRQ phase
> 
> Changes in v3:
> - dropped RZ_DMAC_CHAN_STATUS_SYS_SUSPENDED
> - dropped read_poll_timeout() from rz_dmac_device_synchronze() as
>   with audio drivers this times out all the time on suspend because
>   the audio DMA is already paused when the rz_dmac_device_synchronize()
>   is called; updated the commit description to describe this change
> - call rz_dmac_device_pause_internal() only if RZ_DMAC_CHAN_STATUS_PAUSED
>   bit is not set or the device is enabled in HW
> - updated rz_dmac_device_resume_set() to have it simpler and cover
>   the cases when it is called with the channel enabled or paused;
>   updated the comment describing the covered use cases
> - call rz_dmac_device_resume_internal() only if
>   RZ_DMAC_CHAN_STATUS_PAUSED_INTERNAL bit is set
> - in rz_dmac_chan_is_enabled() return -EAGAIN only if the channel is
>   enabled in HW
> - in rz_dmac_suspend_recover() drop the update of
>   RZ_DMAC_CHAN_STATUS_SYS_SUSPENDED as this is not available anymore
> - in rz_dmac_suspend() call rz_dmac_device_pause_internal() unconditionally
>   as the logic is now handled inside the called function; also, do not
>   ignore anymore the failure of internal suspend and abort the suspend
>   instead
> - report channel internal resume failures in rz_dmac_resume()
> - use rz_dmac_disable_hw() instead of open coding it in rz_dmac_resume()
> - call rz_dmac_device_resume_internal() uncoditionally as the skip
>   logic is now handled in the function itself
> - use NOIRQ_SYSTEM_SLEEP_PM_OPS()
> - didn't collect Tommaso's Tb tag as the series was changed a lot since
>   v2
> 
> Changes in v2:
> - fixed typos in patch description
> - in rz_dmac_suspend_prepare(): return -EAGAIN based on the value returned
>   by vchan_issue_pending()
> - in rz_dmac_suspend_recover(): clear RZ_DMAC_CHAN_STATUS_SYS_SUSPENDED for
>   non cyclic channels
> - in rz_dmac_resume(): call rz_dmac_set_dma_req_no() only for cyclic channels
> 
>  drivers/dma/sh/rz-dmac.c | 188 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 183 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c index 9a10430109e5..00e18d8213ca
> 100644
> --- a/drivers/dma/sh/rz-dmac.c
> +++ b/drivers/dma/sh/rz-dmac.c
> @@ -69,10 +69,12 @@ struct rz_dmac_desc {
>   * enum rz_dmac_chan_status: RZ DMAC channel status
>   * @RZ_DMAC_CHAN_STATUS_PAUSED: Channel is paused though DMA engine callbacks
>   * @RZ_DMAC_CHAN_STATUS_CYCLIC: Channel is cyclic
> + * @RZ_DMAC_CHAN_STATUS_PAUSED_INTERNAL: Channel is paused through
> + driver internal logic
>   */
>  enum rz_dmac_chan_status {
>  	RZ_DMAC_CHAN_STATUS_PAUSED,
>  	RZ_DMAC_CHAN_STATUS_CYCLIC,
> +	RZ_DMAC_CHAN_STATUS_PAUSED_INTERNAL,
>  };
> 
>  struct rz_dmac_chan {
> @@ -92,6 +94,10 @@ struct rz_dmac_chan {
>  	u32 chctrl;
>  	int mid_rid;
> 
> +	struct {
> +		u32 nxla;
> +	} pm_state;
> +
>  	struct list_head ld_free;
> 
>  	struct {
> @@ -962,20 +968,57 @@ static int rz_dmac_device_pause(struct dma_chan *chan)
>  	return rz_dmac_device_pause_set(channel, BIT(RZ_DMAC_CHAN_STATUS_PAUSED));  }
> 
> +static int rz_dmac_device_pause_internal(struct rz_dmac_chan *channel)
> +{
> +	lockdep_assert_held(&channel->vc.lock);
> +
> +	/* Skip channels explicitly paused by consummers or disabled. */
> +	if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_PAUSED) ||
> +	    !rz_dmac_chan_is_enabled(channel))
> +		return 0;
> +
> +	return rz_dmac_device_pause_set(channel,
> +BIT(RZ_DMAC_CHAN_STATUS_PAUSED_INTERNAL));
> +}
> +
>  static int rz_dmac_device_resume_set(struct rz_dmac_chan *channel,
>  				     unsigned long clear_bitmask)
>  {
> -	int ret = 0;
>  	u32 val;
> +	int ret;
> 
>  	lockdep_assert_held(&channel->vc.lock);
> 
> -	/* Do not check CHSTAT_SUS but rely on HW capabilities. */
> +	/*
> +	 * We can be:
> +	 *
> +	 * 1/ after the channel was paused by a consummer and now it
> +	 *    needs to be resummed
> +	 * 2/ after the channel was paused internally (as a result of
> +	 *    a system suspend with power loss or not)
> +	 * 3/ after the channel was paused by a consummer, the system
> +	 *    went through a system suspend (with power loss or not)
> +	 *    and the consummer wants to resume the channel
> +	 *
> +	 * To cover all the above cases we set both CLRSUS and SETEN.
> +	 *
> +	 * In case 1/ setting SETEN while the channel is still enabled
> +	 * is harmless for the controller.
> +	 *
> +	 * In case 2/ the channel is disabled when calling this function
> +	 * and setting CLRSUS is harmless for the controller as the
> +	 * channel is disabled anyway.
> +	 *
> +	 * In case 3/ the channel is disabled/enabled if the system
> +	 * went though a suspend with power loss/or not and setting
> +	 * CLRSUS/SETEN is harmless for the controller as the channel
> +	 * is enabled/disabled anyway.
> +	 */
> +
> +	rz_dmac_ch_writel(channel, CHCTRL_CLRSUS | CHCTRL_SETEN, CHCTRL, 1);
> 
> -	rz_dmac_ch_writel(channel, CHCTRL_CLRSUS, CHCTRL, 1);
>  	ret = read_poll_timeout_atomic(rz_dmac_ch_readl, val,
> -				       !(val & CHSTAT_SUS), 1, 1024, false,
> -				       channel, CHSTAT, 1);
> +				       ((val & (CHSTAT_SUS | CHSTAT_EN)) == CHSTAT_EN),
> +				       1, 1024, false, channel, CHSTAT, 1);
> 
>  	channel->status &= ~clear_bitmask;
> 
> @@ -994,6 +1037,16 @@ static int rz_dmac_device_resume(struct dma_chan *chan)
>  	return rz_dmac_device_resume_set(channel, BIT(RZ_DMAC_CHAN_STATUS_PAUSED));  }
> 
> +static int rz_dmac_device_resume_internal(struct rz_dmac_chan *channel)
> +{
> +	lockdep_assert_held(&channel->vc.lock);
> +
> +	if (!(channel->status & BIT(RZ_DMAC_CHAN_STATUS_PAUSED_INTERNAL)))
> +		return 0;
> +
> +	return rz_dmac_device_resume_set(channel,
> +BIT(RZ_DMAC_CHAN_STATUS_PAUSED_INTERNAL));
> +}
> +
>  /*
>   * -----------------------------------------------------------------------------
>   * IRQ handling
> @@ -1354,6 +1407,130 @@ static void rz_dmac_remove(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  }
> 
> +static int rz_dmac_suspend_prepare(struct device *dev) {
> +	struct rz_dmac *dmac = dev_get_drvdata(dev);
> +
> +	for (unsigned int i = 0; i < dmac->n_channels; i++) {
> +		struct rz_dmac_chan *channel = &dmac->channels[i];
> +
> +		guard(spinlock_irqsave)(&channel->vc.lock);
> +
> +		/* Wait for transfer completion, except in cyclic case. */
> +		if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC))
> +			continue;
> +
> +		if (rz_dmac_chan_is_enabled(channel))
> +			return -EAGAIN;
> +	}
> +
> +	return 0;
> +}
> +
> +static void rz_dmac_suspend_recover(struct rz_dmac *dmac) {
> +	for (unsigned int i = 0; i < dmac->n_channels; i++) {
> +		struct rz_dmac_chan *channel = &dmac->channels[i];
> +
> +		guard(spinlock_irqsave)(&channel->vc.lock);
> +
> +		if (!(channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC)))
> +			continue;
> +
> +		rz_dmac_device_resume_internal(channel);
> +	}
> +}
> +
> +static int rz_dmac_suspend(struct device *dev) {
> +	struct rz_dmac *dmac = dev_get_drvdata(dev);
> +	int ret;
> +
> +	for (unsigned int i = 0; i < dmac->n_channels; i++) {
> +		struct rz_dmac_chan *channel = &dmac->channels[i];
> +
> +		guard(spinlock_irqsave)(&channel->vc.lock);
> +
> +		if (!(channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC)))
> +			continue;
> +
> +		ret = rz_dmac_device_pause_internal(channel);
> +		if (ret) {
> +			dev_err(dev, "Failed to suspend channel %s\n",
> +				dma_chan_name(&channel->vc.chan));
> +			break;
> +		}
> +
> +		channel->pm_state.nxla = rz_dmac_ch_readl(channel, NXLA, 1);
> +	}
> +
> +	if (ret) {
> +		rz_dmac_suspend_recover(dmac);
> +		return ret;
> +	}
> +
> +	pm_runtime_put_sync(dmac->dev);
> +
> +	ret = reset_control_assert(dmac->rstc);
> +	if (ret) {
> +		pm_runtime_resume_and_get(dmac->dev);
> +		rz_dmac_suspend_recover(dmac);
> +	}
> +
> +	return ret;
> +}
> +
> +static int rz_dmac_resume(struct device *dev) {
> +	struct rz_dmac *dmac = dev_get_drvdata(dev);
> +	int errors = 0, ret;
> +
> +	ret = reset_control_deassert(dmac->rstc);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_resume_and_get(dmac->dev);


If this fails for any reason, the next suspend still be called and it will decrement the counter, potentially undeflowing it.
Consider switching to pm_runtime_get_sync(), which suits better here.

Cheers,
Biju