Re: [PATCH 09/11] dmaengine: add support for device_link

[Frank Li <Frank.li@nxp.com>]

On Thu, Sep 11, 2025 at 01:50:56PM +0200, Marco Felsch wrote:
> On 25-09-10, Frank Li wrote:
> > On Wed, Sep 10, 2025 at 09:35:45PM +0200, Marco Felsch wrote:
> > > On 25-09-09, Frank Li wrote:
> > >
> > > ...
> > >
> > > > > > > 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.
> > >
> > > Can you please provide me some pointers? I checked the kernel code base
> > > for the struct::dma_chan_dev. I didn't found any references within the
> > > dmaengine drivers. The only usage I found was for the sysfs purpose.
> >
> > static void k3_configure_chan_coherency(struct dma_chan *chan, u32 asel)
> > {
> > 	struct device *chan_dev = &chan->dev->device;
> > 	...
> > }
> >
> > >
> > > > > > 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.
> > > > }
> > >
> > > I've tested your approach but it turns out that teh dma_chan_dev has no
> > > driver. Of course we could use the DL_FLAG_STATELESS flag but this is
> > > described as:
> > >
> > > | When driver presence on the supplier is irrelevant and only correct
> > > | suspend/resume and shutdown ordering is needed, the device link may
> > > | simply be set up with the ``DL_FLAG_STATELESS`` flag.  In other words,
> > > | enforcing driver presence on the supplier is optional.
> > >
> > > I want to enforce the driver presence, therefore I used the manged flags
> > > which excludes the DL_FLAG_STATELESS, if I get it right.
> > >
> > > Please see the below the debug output:
> > >
> > > ** use the dmaengine device as supplier **
> > >
> > > device_link_init_status: supplier.dev:30bd0000.dma-controller supplier.drv:imx-sdma supplier.status:0x2 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
> > > device_link_init_status: supplier.dev:30e10000.dma-controller supplier.drv:imx-sdma supplier.status:0x2 consumer:dev:30c20000.sai consumer.drv:fsl-sai consumer.status:0x1
> > >
> > >
> > > ** use the dma channel device as supplier **
> > >
> > > device_link_init_status: supplier.dev:dma0chan0 supplier.drv:no-driver supplier.status:0x0 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
> > > device_link_init_status: supplier.dev:dma0chan1 supplier.drv:no-driver supplier.status:0x0 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
> >
> > It should be similar with phy drivers, which phy_create() create individual
> > phy devices (like dma channel devices).
>
> Unfortunately phy drivers do use the DL_FLAG_STATELESS mechanism. My
> main goal was to have managed links to overcome the current situation:
> dmaengine drivers can be removed without removing the consumer drivers
> first.
>
> You have a valid point by making use dma-channel devices ( dma<X>cha<Y>)
> to manage suspend/resume, as well as runtime-PM for each channel.
>
> But I see this rather as an addition to my solution because these links
> must be stateless and stateless/unmanaged links don't guarantee the
> correct remove order (my main goal).

If that, phy should have simiar problems. It should be resolved at higher
layer to avoid fix this kinds of problem one by one.

>
> That beeing said, I'm not sure how you want to handle the clock/power
> enablement per channel-device. This would require additional work on the
> dma_devclass to add a proper .pm hook else the PM and runtime-PM calls
> are only forwarded to the parent dmaengine driver. On this level the
> dmaengine driver has no knowledge which channel is going to be
> enabled/disabled.

I have draft runtime pm patch for eDMA.

>
> In conclusion, I see my approach as valid to ensure the correct remove
> order. Your suggestion is valid and can be added later on too since this
> needs more work to have a proper per-channel runtime-PM.

We need pave a good road. This part is common dma-engine, which is worth to
do good solution.

Frank

>
> Regards,
>   Marco
>
> >
> > > ------------[ cut here ]------------
> > > WARNING: CPU: 0 PID: 51 at /drivers/base/core.c:1387 device_links_driver_bound+0x170/0x3a0
> > > ...
> > > ---[ end trace 0000000000000000 ]---
> > > ------------[ cut here ]------------
> > >
> > > As said, I get your point regarding the usage of the dma-channel device
> > > but I didn't found any reference to a driver which used the dma-channel
> > > device. Also since I want to have the supply driver to enforced by the
> > > devlink I don't want to use the DL_FLAG_STATELESS flag.
> >
> > Maybe add DL_FLAG, link to parent's device driver. Need some time to
> > investigate more. PHY driver should good example to refer to.
> >
> > >
> > > Regarding your point, that some DMA controllers may have seperate clocks
> > > for each channel: I think this can be handled by the dmaengine driver,
> > > e.g. via the device_alloc_chan_resources() hook.
> >
> > device_alloc_chan_resources() is not efficient enough, most driver allocate
> > channel at probe, so clk of this channel will be always on. ideally, only
> > when consumer devices is runtime resume state,  turn on dma channel clock.
> >
> > Frank
> > >
> > > @all
> > > I'm pleased about any input :)
> > >
> > > Regards,
> > >   Marco
> >