Re: [PATCH v2 5/7] dmaengine: sh: rz-dmac: Add suspend to RAM support

[Tommaso Merciai]

Hi Claudiu,
Thanks for your patch!

On 3/20/26 12:28, Claudiu wrote:
> 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 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

I've tested this series on RZ/G3E.
Suspend/resume support DMAC + RSPI is now working fine.

Tested-by: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>


Kind Regards,
Tommaso

>   drivers/dma/sh/rz-dmac.c | 185 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 177 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> index ca8c0aa8ae59..6f83ccdf94c6 100644
> --- a/drivers/dma/sh/rz-dmac.c
> +++ b/drivers/dma/sh/rz-dmac.c
> @@ -69,11 +69,15 @@ struct rz_dmac_desc {
>    * enum rz_dmac_chan_status: RZ DMAC channel status
>    * @RZ_DMAC_CHAN_STATUS_ENABLED: Channel is enabled
>    * @RZ_DMAC_CHAN_STATUS_PAUSED: Channel is paused though DMA engine callbacks
> + * @RZ_DMAC_CHAN_STATUS_PAUSED_INTERNAL: Channel is paused through driver internal logic
> + * @RZ_DMAC_CHAN_STATUS_SYS_SUSPENDED: Channel was prepared for system suspend
>    * @RZ_DMAC_CHAN_STATUS_CYCLIC: Channel is cyclic
>    */
>   enum rz_dmac_chan_status {
>   	RZ_DMAC_CHAN_STATUS_ENABLED,
>   	RZ_DMAC_CHAN_STATUS_PAUSED,
> +	RZ_DMAC_CHAN_STATUS_PAUSED_INTERNAL,
> +	RZ_DMAC_CHAN_STATUS_SYS_SUSPENDED,
>   	RZ_DMAC_CHAN_STATUS_CYCLIC,
>   };
>   
> @@ -94,6 +98,10 @@ struct rz_dmac_chan {
>   	u32 chctrl;
>   	int mid_rid;
>   
> +	struct {
> +		u32 nxla;
> +	} pm_state;
> +
>   	struct list_head ld_free;
>   	struct list_head ld_queue;
>   	struct list_head ld_active;
> @@ -994,10 +1002,17 @@ static int rz_dmac_device_pause(struct dma_chan *chan)
>   	return rz_dmac_device_pause_set(channel, RZ_DMAC_CHAN_STATUS_PAUSED);
>   }
>   
> +static int rz_dmac_device_pause_internal(struct rz_dmac_chan *channel)
> +{
> +	lockdep_assert_held(&channel->vc.lock);
> +
> +	return rz_dmac_device_pause_set(channel, RZ_DMAC_CHAN_STATUS_PAUSED_INTERNAL);
> +}
> +
>   static int rz_dmac_device_resume_set(struct rz_dmac_chan *channel,
>   				     enum rz_dmac_chan_status status)
>   {
> -	u32 val;
> +	u32 val, chctrl;
>   	int ret;
>   
>   	lockdep_assert_held(&channel->vc.lock);
> @@ -1005,14 +1020,33 @@ static int rz_dmac_device_resume_set(struct rz_dmac_chan *channel,
>   	if (!(channel->status & BIT(RZ_DMAC_CHAN_STATUS_PAUSED)))
>   		return 0;
>   
> -	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);
> -	if (ret)
> -		return ret;
> +	if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_SYS_SUSPENDED)) {
> +		/*
> +		 * We can be after a sleep state with power loss. If power was
> +		 * lost, the CHSTAT_SUS bit is zero. In this case, we need to
> +		 * enable the channel directly. Otherwise, just set the CLRSUS
> +		 * bit.
> +		 */
> +		val = rz_dmac_ch_readl(channel, CHSTAT, 1);
> +		if (val & CHSTAT_SUS)
> +			chctrl = CHCTRL_CLRSUS;
> +		else
> +			chctrl = CHCTRL_SETEN;
> +	} else {
> +		chctrl = CHCTRL_CLRSUS;
> +	}
> +
> +	rz_dmac_ch_writel(channel, chctrl, CHCTRL, 1);
>   
> -	channel->status &= ~BIT(status);
> +	if (chctrl & CHCTRL_CLRSUS) {
> +		ret = read_poll_timeout_atomic(rz_dmac_ch_readl, val,
> +					       !(val & CHSTAT_SUS), 1, 1024, false,
> +					       channel, CHSTAT, 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	channel->status &= ~(BIT(status) | BIT(RZ_DMAC_CHAN_STATUS_SYS_SUSPENDED));
>   
>   	return 0;
>   }
> @@ -1026,6 +1060,13 @@ static int rz_dmac_device_resume(struct dma_chan *chan)
>   	return rz_dmac_device_resume_set(channel, RZ_DMAC_CHAN_STATUS_PAUSED);
>   }
>   
> +static int rz_dmac_device_resume_internal(struct rz_dmac_chan *channel)
> +{
> +	lockdep_assert_held(&channel->vc.lock);
> +
> +	return rz_dmac_device_resume_set(channel, RZ_DMAC_CHAN_STATUS_PAUSED_INTERNAL);
> +}
> +
>   /*
>    * -----------------------------------------------------------------------------
>    * IRQ handling
> @@ -1430,6 +1471,133 @@ 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 (vchan_issue_pending(&channel->vc) &&
> +		    !(channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC)))
> +			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;
> +
> +		if (!(channel->status & BIT(RZ_DMAC_CHAN_STATUS_PAUSED_INTERNAL))) {
> +			channel->status &= ~BIT(RZ_DMAC_CHAN_STATUS_SYS_SUSPENDED);
> +			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;
> +
> +		if (!(channel->status & BIT(RZ_DMAC_CHAN_STATUS_PAUSED))) {
> +			ret = rz_dmac_device_pause_internal(channel);
> +			if (ret) {
> +				dev_err(dev, "Failed to suspend channel %s\n",
> +					dma_chan_name(&channel->vc.chan));
> +				continue;
> +			}
> +		}
> +
> +		channel->pm_state.nxla = rz_dmac_ch_readl(channel, NXLA, 1);
> +		channel->status |= BIT(RZ_DMAC_CHAN_STATUS_SYS_SUSPENDED);
> +	}
> +
> +	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 ret;
> +
> +	ret = reset_control_deassert(dmac->rstc);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_resume_and_get(dmac->dev);
> +	if (ret) {
> +		reset_control_assert(dmac->rstc);
> +		return ret;
> +	}
> +
> +	rz_dmac_writel(dmac, DCTRL_DEFAULT, CHANNEL_0_7_COMMON_BASE + DCTRL);
> +	rz_dmac_writel(dmac, DCTRL_DEFAULT, CHANNEL_8_15_COMMON_BASE + DCTRL);
> +
> +	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))) {
> +			rz_dmac_ch_writel(&dmac->channels[i], CHCTRL_DEFAULT, CHCTRL, 1);
> +			continue;
> +		}
> +
> +		rz_dmac_set_dma_req_no(dmac, channel->index, channel->mid_rid);
> +
> +		rz_dmac_ch_writel(channel, channel->pm_state.nxla, NXLA, 1);
> +		rz_dmac_ch_writel(channel, channel->chcfg, CHCFG, 1);
> +		rz_dmac_ch_writel(channel, CHCTRL_SWRST, CHCTRL, 1);
> +		rz_dmac_ch_writel(channel, channel->chctrl, CHCTRL, 1);
> +
> +		if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_PAUSED_INTERNAL)) {
> +			ret = rz_dmac_device_resume_internal(channel);
> +			if (ret) {
> +				dev_err(dev, "Failed to resume channel %s\n",
> +					dma_chan_name(&channel->vc.chan));
> +				continue;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops rz_dmac_pm_ops = {
> +	.prepare = rz_dmac_suspend_prepare,
> +	SYSTEM_SLEEP_PM_OPS(rz_dmac_suspend, rz_dmac_resume)
> +};
> +
>   static const struct rz_dmac_info rz_dmac_v2h_info = {
>   	.icu_register_dma_req = rzv2h_icu_register_dma_req,
>   	.default_dma_req_no = RZV2H_ICU_DMAC_REQ_NO_DEFAULT,
> @@ -1456,6 +1624,7 @@ static struct platform_driver rz_dmac_driver = {
>   	.driver		= {
>   		.name	= "rz-dmac",
>   		.of_match_table = of_rz_dmac_match,
> +		.pm	= pm_sleep_ptr(&rz_dmac_pm_ops),
>   	},
>   	.probe		= rz_dmac_probe,
>   	.remove		= rz_dmac_remove,