Message-ID: <20250910093428.qq5vskqhvumgjsow@pengutronix.de>
Date: Wed, 10 Sep 2025 11:34:28 +0200
From: Marco Felsch <m.felsch@...gutronix.de>
To: Frank Li <Frank.li@....com>
Cc: Vinod Koul <vkoul@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Jiada Wang <jiada_wang@...tor.com>, dmaengine@...r.kernel.org,
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 09/11] dmaengine: add support for device_link

On 25-09-09, Frank Li wrote:
> On Tue, Sep 09, 2025 at 02:03:09PM +0200, Marco Felsch wrote:
> > Hi Frank,
> >
> > On 25-09-03, Frank Li wrote:
> > > On Wed, Sep 03, 2025 at 03:06:17PM +0200, Marco Felsch wrote:
> > > > Add support to create device_links between dmaengine suppliers and the
> > > > dma consumers. This shifts the device dep-chain teardown/bringup logic
> > > > to the driver core.
> > > >
> > > > Moving this to the core allows the dmaengine drivers to simplify the
> > > > .remove() hooks and also to ensure that no dmaengine driver is ever
> > > > removed before the consumer is removed.
> > > >
> > > > Signed-off-by: Marco Felsch <m.felsch@...gutronix.de>
> > > > ---
> > >
> > > Thank you work for devlink between dmaengine and devices. I have similar
> > > idea.
> > >
> > > This patch should be first patch.
> >
> > I can shuffle it of course!
> >
> > > The below what planned commit message in my local tree.
> >
> > Okay, so you focused on runtime PM handling. Not quite sure if I can
> > test this feature with the SDMA engine. I also have limited time for
> > this feature.
> >
> > Is it okay for you and the DMA maintainers to add the runtime PM feature
> > as separate patch (provided by NXP/Frank)?
> 
> we can support runtime pm later.
> 
> >
> > > Implementing runtime PM for DMA channels is challenging. If a channel
> > > resumes at allocation and suspends at free, the DMA engine often remains on
> > > because most drivers request a channel at probe.
> > >
> > > Tracking the number of pending DMA descriptors is also problematic, as some
> > > consumers append new descriptors in atomic contexts, such as IRQ handlers,
> > > where runtime resume cannot be called.
> > >
> > > Using a device link simplifies this issue. If a consumer requires data
> > > transfer, it must be in a runtime-resumed state, ensuring that the DMA
> > > channel is also active by device link. This allows safe operations, like
> > > appending new descriptors. Conversely, when the consumer no longer requires
> > > data transfer, both it and the supplier (DMA channel) can enter a suspended
> > > state if no other consumer is using it.
> > >
> > > Introduce the `create_link` flag to enable this feature.
> > >
> > > also suggest add create_link flag to enable this feature in case some
> > > side impact to other dma-engine. After some time test, we can enable it
> > > default.
> >
> > What regressions do you have in mind? I wouldn't hide the feature behind
> > a flag because this may slow done the convert process, because no one is
> > interessted in, or has no time for testing, ...
> 
> Unlike other devices, like phys, regulator, mailbox..., which auto create
> devlink at probe. I am not clear why dma skip this one. So I think there
> should be some reason behind. Maybe other people, rob or Vinod Koul know
> the reason.
> 
> static const struct supplier_bindings of_supplier_bindings[] = {
>         ...
> 	{ .parse_prop = parse_dmas, .optional = true, },
> 
> If remove "optional = true", devlink will auto create. I am not sure why
> set true here.

I've seen this too. Could be because DMA controllers + users aren't OF
related and therefore should be handled within the framework itself.

> > > >  drivers/dma/dmaengine.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > > > index 758fcd0546d8bde8e8dddc6039848feeb1e24475..a50652bc70b8ce9d4edabfaa781b3432ee47d31e 100644
> > > > --- a/drivers/dma/dmaengine.c
> > > > +++ b/drivers/dma/dmaengine.c
> > > > @@ -817,6 +817,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > >  	struct dma_device *d, *_d;
> > > >  	struct dma_chan *chan = NULL;
> > > > +	struct device_link *dl;
> > > >
> > > >  	if (is_of_node(fwnode))
> > > >  		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
> > > > @@ -858,6 +859,13 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > >  	/* No functional issue if it fails, users are supposed to test before use */
> > > >  #endif
> > > >
> > > > +	dl = device_link_add(dev, chan->device->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> > >
> > > chan->device->dev is dmaengine devices. But some dmaengine's each channel
> > > have device, consumer should link to chan's device, not dmaengine device
> > > because some dmaengine support per channel clock\power management.
> >
> > I get your point. Can you give me some pointers please? To me it seems
> > like the dma_chan_dev is only used for sysfs purpose according the
> > dmaengine.h.
> 
> Not really, there are other dma engineer already reuse it for other purpose.
> So It needs update kernel doc for dma_chan_dev.

Okay.

> > > chan's device's parent devices is dmaengine devices. it should also work
> > > for sdma case
> >
> > I see, this must be tested of course.
> > > >         if (chan->device->create_devlink) {
> > >                 u32 flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
> >
> > According device_link.rst: using DL_FLAG_STATELESS and
> > DL_FLAG_AUTOREMOVE_CONSUMER is invalid.
> >
> > >                 if (pm_runtime_active(dev))
> > >                         flags |= DL_FLAG_RPM_ACTIVE;
> >
> > This is of course interessting, thanks for the hint.
> >
> > > When create device link (apply channel), consume may active.
> >
> > I have read it as: "resue the supplier and ensure that the supplier
> > follows the consumer runtime state".
> >
> > >                 dl = device_link_add(chan->slave, &chan->dev->device, flags);
> >
> > Huh.. you used the dmaengine device too?
> 
> /**
>  * struct dma_chan_dev - relate sysfs device node to backing channel device
>  * @chan: driver channel device
>  * @device: sysfs device
>  * @dev_id: parent dma_device dev_id
>  * @chan_dma_dev: The channel is using custom/different dma-mapping
>  * compared to the parent dma_device
>  */
> struct dma_chan_dev {
> 	struct dma_chan *chan;
> 	struct device device;
> 	int dev_id;
> 	bool chan_dma_dev;
> };
> 
> struct dma_chan {
> 	struct dma_device *device; /// this one should be dmaengine
> 	struct dma_chan_dev *dev; /// this one is pre-chan device.
> }

Argh.. mixed it within my head while writing the mail :/

Regards,
  Marco

Powered by blists - more mailing lists