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

[Marco Felsch <m.felsch@pengutronix.de>]

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.

> > > 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
------------[ 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.

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.

@all
I'm pleased about any input :)

Regards,
  Marco