Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <be967357-6ecb-5ecf-c860-5265e1527b2a@gmail.com>
Date:   Sat, 5 Nov 2022 20:49:49 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
To:     Georgi Vlaev <g-vlaev@ti.com>, Vinod Koul <vkoul@kernel.org>
Cc:     Nishanth Menon <nm@ti.com>, dmaengine@vger.kernel.org,
        linux-kernel@vger.kernel.org, Vignesh Raghavendra <vigneshr@ti.com>
References: <20221103203021.83929-1-g-vlaev@ti.com>
Content-Language: en-US
From:   =?UTF-8?Q?P=c3=a9ter_Ujfalusi?= <peter.ujfalusi@gmail.com>
Subject: Re: [PATCH v2] dmaengine: k3-udma: Add system suspend/resume support
In-Reply-To: <20221103203021.83929-1-g-vlaev@ti.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 03/11/2022 22:30, Georgi Vlaev wrote:
> From: Vignesh Raghavendra <vigneshr@ti.com>
> 
> The K3 platforms configure the DMA resources with the
> help of the TI's System Firmware's Device Manager(DM)
> over TISCI. The group of DMA related Resource Manager[1]
> TISCI messages includes: INTA, RINGACC, UDMAP, and PSI-L.
> This configuration however, does not persist in the DM
> after leaving from Suspend-to-RAM state. We have to restore
> the DMA channel configuration over TISCI for all configured
> channels when returning from suspend.
> 
> The TISCI resource management calls for each DMA type (UDMA,
> PKTDMA, BCDMA) happen in device_free_chan_resources() and
> device_alloc_chan_resources(). In pm_suspend() we store
> the current udma_chan_config for channels that still have
> attached clients and call device_free_chan_resources().
> In pm_resume() restore the udma_channel_config from backup
> and call device_alloc_chan_resources() for those channels.
> Drivers like CPSW can do their own DMA resource management,
> so use the late system suspend/resume hooks.

It is wrong to push the DMA context store/restore task to a client 
driver (cpsw or icss), it has to be done by the glue layer.

With this patch the DMAengine side of the UDMA/BCDMA/PKTDMA will support 
suspend/resume while the networking will remain broken, right?

I can not test this atm since my setup relies solely on NFS rootfs via 
cpsw, I might be able to check with a USB-ethernet dongle..

Please do a followup for the glue layer support.

Acked-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>

> [1] https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/index.html#resource-management-rm
> 
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> [g-vlaev@ti.com: Add patch description and config backup]
> [g-vlaev@ti.com: Supend only channels with clients]
> Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
> ---
> Changes:
> 
> v2:
> * Update the commit message
> * Use list_for_each_entry() to iterate over the channel list.
> 
>   drivers/dma/ti/k3-udma.c | 54 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 54 insertions(+)
> 
> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
> index ce8b80bb34d7..29844044132c 100644
> --- a/drivers/dma/ti/k3-udma.c
> +++ b/drivers/dma/ti/k3-udma.c
> @@ -304,6 +304,8 @@ struct udma_chan {
>   
>   	/* Channel configuration parameters */
>   	struct udma_chan_config config;
> +	/* Channel configuration parameters (backup) */
> +	struct udma_chan_config backup_config;
>   
>   	/* dmapool for packet mode descriptors */
>   	bool use_dma_pool;
> @@ -5491,11 +5493,63 @@ static int udma_probe(struct platform_device *pdev)
>   	return ret;
>   }
>   
> +static int udma_pm_suspend(struct device *dev)
> +{
> +	struct udma_dev *ud = dev_get_drvdata(dev);
> +	struct dma_device *dma_dev = &ud->ddev;
> +	struct dma_chan *chan;
> +	struct udma_chan *uc;
> +
> +	list_for_each_entry(chan, &dma_dev->channels, device_node) {
> +		if (chan->client_count) {
> +			uc = to_udma_chan(chan);
> +			/* backup the channel configuration */
> +			memcpy(&uc->backup_config, &uc->config,
> +			       sizeof(struct udma_chan_config));
> +			dev_dbg(dev, "Suspending channel %s\n",
> +				dma_chan_name(chan));
> +			ud->ddev.device_free_chan_resources(chan);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int udma_pm_resume(struct device *dev)
> +{
> +	struct udma_dev *ud = dev_get_drvdata(dev);
> +	struct dma_device *dma_dev = &ud->ddev;
> +	struct dma_chan *chan;
> +	struct udma_chan *uc;
> +	int ret;
> +
> +	list_for_each_entry(chan, &dma_dev->channels, device_node) {
> +		if (chan->client_count) {
> +			uc = to_udma_chan(chan);
> +			/* restore the channel configuration */
> +			memcpy(&uc->config, &uc->backup_config,
> +			       sizeof(struct udma_chan_config));
> +			dev_dbg(dev, "Resuming channel %s\n",
> +				dma_chan_name(chan));
> +			ret = ud->ddev.device_alloc_chan_resources(chan);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops udma_pm_ops = {
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(udma_pm_suspend, udma_pm_resume)
> +};
> +
>   static struct platform_driver udma_driver = {
>   	.driver = {
>   		.name	= "ti-udma",
>   		.of_match_table = udma_of_match,
>   		.suppress_bind_attrs = true,
> +		.pm = &udma_pm_ops,
>   	},
>   	.probe		= udma_probe,
>   };
> 
> base-commit: 81214a573d19ae2fa5b528286ba23cd1cb17feec

-- 
Péter