Message-ID: <aMGebFhZYrFmHnH7@lizhi-Precision-Tower-5810>
Date: Wed, 10 Sep 2025 11:51:08 -0400
From: Frank Li <Frank.li@....com>
To: Marco Felsch <m.felsch@...gutronix.de>
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 Wed, Sep 10, 2025 at 11:34:28AM +0200, Marco Felsch wrote:
> 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.

Not sure, may be some historical reason. I have not enough confidence just
because I don't know why optional = true here.

So I think you can post this patch seperated with good cover letter, may
someone jump into discussion.

Frank
>
> > > > >  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