Re: [PATCH v7 2/4] dmaengine: Forward slave device pointer to of_xlate callback

[Vinod Koul <vinod.koul@intel.com>]

On Fri, Feb 03, 2017 at 02:01:27PM +0100, Marek Szyprowski wrote:
> Hi All,
> 
> On 2017-01-26 15:43, Marek Szyprowski wrote:
> >On 2017-01-25 14:12, Lars-Peter Clausen wrote:
> >>On 01/25/2017 11:28 AM, Marek Szyprowski wrote:
> >>>Add pointer to slave device to of_dma_xlate to let DMA engine driver
> >>>to know which slave device is using given DMA channel. This will be
> >>>later used to implement non-irq-safe runtime PM for DMA engine driver.
> >>of_dma_xlate() is used to translate from a OF phandle and a
> >>specifier to a
> >>DMA channel. On one hand this does not necessarily mean that the
> >>channel is
> >>actually going to be used by the slave that called the xlate function.
> >>Modifying the driver state when a lookup of the channel is done is a
> >>layering violation. And this approach is also missing a way to
> >>disassociate
> >>a slave from a DMA channel, e.g. when it is no longer used.
> >>
> >>On the other hand there are other mechanisms to translate
> >>between some kind
> >>of firmware handle to a DMA channel which are completely ignored here.
> >>
> >>So this approach does not work. This is something that needs to
> >>be done at
> >>the dmaengine level, not a the firmware resource translation
> >>level. And it
> >>needs a matching method that is called when the channel is disassociated
> >>from a device, when the device no longer uses the DMA channel.
> >
> >Frankly I agree that of_dma_xlate() should only return the
> >requested channel
> >to the dmaengine core and do not do any modification in the the
> >driver state.
> >
> >However the current dma engine design and implementation breaks
> >this rule.
> >Please check the drivers - how do they implement of_xlate callback. They
> >usually call dma_get_any_slave_channel, dma_get_slave_channel or
> >__dma_request_channel there, which in turn calls dma_chan_get, which then
> >calls back to device_alloc_chan_resources callback. Some of the
> >drivers also
> >do a hardware configuration or other resource allocation in of_xlate.
> >This is a bit messy design and leave no place in the core to set
> >slave device
> >before device_alloc_chan_resources callback, where one would
> >expect to have
> >it already set.
> >
> >The best place to add new calls to the dmaengine drivers to set
> >slave device
> >would be just before device_alloc_chan_resources(), what in turn
> >means that
> >the current dmaengine core should do in dma_chan_get(). This would
> >require to
> >forward the slave device pointer via even more layers including
> >the of_xlate
> >callback too. IMHO this is not worth the effort.
> >
> >DMA engine core and API definitely needs some cleanup. During such
> >cleanup
> >the slave device pointer might be moved out of xlate into separate
> >callback
> >when the core gets ready for such operation.
> >
> >I ignored other paths for other firmware handle to a DMA channel
> >translation
> >mechanism because for the current pl330 driver they are simply not
> >used. I
> >assume that if one needs to implement similar things for drivers
> >relying on
> >them, he will update the respective DMA engine core parts.
> >
> >Slave device assignments can be cleared in
> >device_chan_release_resources if
> >this is needed and that what existing DMA engine drivers do with
> >the resources
> >allocated in the of_xlate callback...
> 
> Vinod: could you comment this patchset? Is there a chance to get it
> merged or
> at least give it a try in -next? If not, could you provide some
> hints what should
> I do?

Nothing :) I am slowly clearning up my queue and should process this series
in couple of days or so...

-- 
~Vinod