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: <21fb260b-df57-f48d-90bb-aa126fc564a5@gmail.com>
Date:   Wed, 2 Nov 2022 17:10:38 +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: <20221028163050.248074-1-g-vlaev@ti.com>
Content-Language: en-US
From:   =?UTF-8?Q?P=c3=a9ter_Ujfalusi?= <peter.ujfalusi@gmail.com>
Subject: Re: [PATCH] dmaengine: k3-udma: Add system suspend/resume support
In-Reply-To: <20221028163050.248074-1-g-vlaev@ti.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Georgi,

On 28/10/2022 19: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 entering suspend.

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.
> 
> [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>
> ---
>   drivers/dma/ti/k3-udma.c | 55 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
> 
> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
> index ce8b80bb34d7..efcc1c5ddb2d 100644
> --- a/drivers/dma/ti/k3-udma.c
> +++ b/drivers/dma/ti/k3-udma.c
> @@ -194,6 +194,7 @@ struct udma_dev {
>   	int rchan_cnt;
>   	int rflow_cnt;
>   	int tflow_cnt;
> +	int ch_count;

I think udev->ddev.chancnt already contains this information and there 
is a helper to iterate through the list:

list_for_each_entry(chan, &udev->ddev.channels, device_node) { }

Used in udma_dbg_summary_show() for example.

>   	unsigned long *bchan_map;
>   	unsigned long *tchan_map;
>   	unsigned long *rchan_map;
> @@ -304,6 +305,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;
> @@ -4999,6 +5002,7 @@ static int setup_resources(struct udma_dev *ud)
>   	if (!ch_count)
>   		return -ENODEV;
>   
> +	ud->ch_count = ch_count;
>   	ud->channels = devm_kcalloc(dev, ch_count, sizeof(*ud->channels),
>   				    GFP_KERNEL);
>   	if (!ud->channels)
> @@ -5491,11 +5495,62 @@ 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_chan *chan;
> +	int i;
> +
> +	for (i = 0; i < ud->ch_count; i++) {
> +		chan = &ud->channels[i].vc.chan;
> +		if (chan->client_count) {
> +			/* backup the channel configuration */
> +			memcpy(&ud->channels[i].backup_config,
> +			       &ud->channels[i].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_chan *chan;
> +	int ret, i;
> +
> +	for (i = 0; i < ud->ch_count; i++) {
> +		chan = &ud->channels[i].vc.chan;
> +		if (chan->client_count) {
> +			/* restore the channel configuration */
> +			memcpy(&ud->channels[i].config,
> +			       &ud->channels[i].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,
>   };

-- 
Péter