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

[claudiu beznea <claudiu.beznea@tuxon.dev>]

Hi, Tommaso,

On 3/12/26 14:26, Tommaso Merciai wrote:
> Hi Claudiu,
> Thanks for your patch.
> 
> On Mon, Jan 26, 2026 at 12:31:53PM +0200, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The Renesas RZ/G3S SoC supports a power saving mode in which power to most
>> 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 and resume, the driver restores additional
>> registers for these channels during the 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
>> ongoing transfers to complete before allowing suspend-to-RAM to proceed.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---

[ ... ]

>> +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;
>> +}
> 
> Testing suspend/resume support on RZ/G3E (DMAC + RSPI) I'm seeing the
> following when suspending:
> 
> rz_dmac_suspend()
> 
>      [   50.657802] rz-dmac 11400000.dma-controller: PM: device_prepare(): genpd_prepare returns -11
>      [   50.667577] rz-dmac 11400000.dma-controller: PM: device_prepare(): genpd_prepare returns -11
>      [   50.675984] rz-dmac 11400000.dma-controller: PM: device_prepare(): genpd_prepare returns -11
>      [   50.684394] rz-dmac 11400000.dma-controller: PM: device_prepare(): genpd_prepare returns -11
>      [   50.692804] rz-dmac 11400000.dma-controller: PM: device_prepare(): genpd_prepare returns -11
>      [   50.701221] rz-dmac 11400000.dma-controller: PM: device_prepare(): genpd_prepare returns -11
>      [   50.709642] rz-dmac 11400000.dma-controller: PM: device_prepare(): genpd_prepare returns -11
>      [   50.718062] rz-dmac 11400000.dma-controller: PM: device_prepare(): genpd_prepare returns -11
>      [   50.726480] rz-dmac 11400000.dma-controller: PM: device_prepare(): genpd_prepare returns -11
>      [   50.734900] rz-dmac 11400000.dma-controller: PM: device_prepare(): genpd_prepare returns -11
> 
> I found out that this issue can be solved by:
> 
> When the IRQ handler thread completes a non-cyclic transfer and there
> are no more descriptors queued (ld_queue is empty), invalidate all
> link-mode descriptor headers and clear the RZ_DMAC_CHAN_STATUS_ENABLED
> bit into the rz_dmac_irq_handler_thread():
> 
> static irqreturn_t rz_dmac_irq_handler_thread(int irq, void *dev_id)
> {
> 	struct rz_dmac_chan *channel = dev_id;
> 	struct rz_dmac_desc *desc = NULL;
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&channel->vc.lock, flags);
> 
> 	if (list_empty(&channel->ld_active)) {
> 		/* Someone might have called terminate all */
> 		goto out;
> 	}
> 
> 	desc = list_first_entry(&channel->ld_active, struct rz_dmac_desc, node);
> 
> 	if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC)) {
> 		desc = channel->desc;
> 		vchan_cyclic_callback(&desc->vd);
> 		goto out;
> 	} else {
> 		vchan_cookie_complete(&desc->vd);
> 	}
> 
> 	list_move_tail(channel->ld_active.next, &channel->ld_free);
> 	if (!list_empty(&channel->ld_queue)) {
> 		desc = list_first_entry(&channel->ld_queue, struct rz_dmac_desc,
> 					node);
> 		channel->desc = desc;
> 		if (rz_dmac_xfer_desc(channel) == 0)
> 			list_move_tail(channel->ld_queue.next, &channel->ld_active);
> +	} else {
> +		rz_dmac_invalidate_lmdesc(channel);
> +		channel->status &= ~BIT(RZ_DMAC_CHAN_STATUS_ENABLED);
> 	}
> out:
> 	spin_unlock_irqrestore(&channel->vc.lock, flags);
> 
> 	return IRQ_HANDLED;
> }
> 
> 
> 
>> +
>> +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);
>> +
>> +		rz_dmac_set_dma_req_no(dmac, channel->index, channel->mid_rid);
>> +
>> +		if (!(channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC))) {
>> +			rz_dmac_ch_writel(&dmac->channels[i], CHCTRL_DEFAULT, CHCTRL, 1);
>> +			continue;
>> +		}
>> +
>> +		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;
>> +}
> 
> Then on resume I'm seeing the following when testing DMAC + RSPI:
> 
> [   52.831840] spi-nor spi0.0: SPI transfer failed: -110
> [   52.836950] spi_master spi0: failed to transfer one message from queue
> [   52.843474] spi_master spi0: noqueue transfer failed
> 
> Which I found out that can be solved by moving:
> 
> 	rz_dmac_set_dma_req_no(dmac, channel->index, channel->mid_rid);
> 
> after the cyclic check:
> 
> 	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);
> 
> In this way
> 	
> 	rz_dmac_set_dma_req_no()
> 
> Is only called for cyclic channels
> 
> What do you think?

Thank you for trying this series and looking to provide fixes. Unfortunatelly, I 
currently don't have at hand a platform with SPI enabled to check it. I'm going 
to investigate the fixes you provided and integrate them in the next version.

Thank you,
Claudiu