RE: [PATCH 01/11] dmaengine: add context parameter toprep_slave_sg and prep_dma_cyclic

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

On Fri, 2012-02-03 at 07:31 -0800, Bounine, Alexandre wrote:
> On Thu, Feb 02, 2012 at 4:44 PM, Russell King wrote:
> > 
> > On Thu, Feb 02, 2012 at 04:32:11PM -0500, Alexandre Bounine wrote:
> > > Add context parameter to device_prep_slave_sg() and
> device_prep_dma_cyclic()
> > > interfaces to allow passing client/target specific information
> associated
> > > with the data transfer.
> > 
> > I'm slightly concerned about having this as a vague 'void *' pointer.
> > What that means is that the data being passed through it is entirely
> > unspecified, and quite specific to the DMA engine.
> > 
> > That's rather worrying when we (on ARM) are moving towards a model
> > where the same peripheral IP can be backed by multiple different DMA
> > engines.  We already have peripheral drivers (clients of DMA engine)
> > which can be attached to completely different DMA engine drivers.
Russell,
Agreed but the problem with new models like RIO, MSM is that they seem
to have some different parameters they depend on, apart from the usual
ones we have. And these are per transfer dependent rather than channel
dependent as we have. So we need some way to pass arch specfic
parameters... so a agenric mechanism this was proposed...
Agreed, this has its own shortcoming. If you have a better way, please
let us know :)

> > 
> > If DMA engines continue to operate conventionally with this parameter
> > NULL then that's fine for current stuff.  I would suggest that it's
> > made to be something a little better defined though, as the 'void *'
> > approach encourages each driver writer to invent their own way to
> > specify, eg, an interleaved transfer.  We'll then end up with N
> > different ways to specify the same thing.
> > 
> > Not only that, but peripheral drivers won't know what kind of data to
> > pass there (they would have to have additional knowledge of the DMA
> > engine which they're attached to.)
> > 
> > Basically, allowing a DMA engine specific blob of data to be passed
> > destroys the idea of having a consistent API for peripheral drivers
> > to use, because they can then no longer be independent of their DMA
> > engine.
> > 
> I agree with your concern about "void *" being quite wide-open
> definition.
> But it may be practically impossible to come up with better definition
> that may be equally suitable for different systems or HW combinations
> (e.g. definition that is good for ARM peripherals may not fit into the
> RapidIO model).
> 
> Some level of protection should be expected from filter callbacks
> invoked
> by dma_request_channel(). This way, for example, RapidIO client will not
> acquire DMA channel which does not support RapidIO data transfers. 
>  
> After saying all above to defend the changes, I have to admit that your
> comment made me to look harder for an alternative way to pass
> per-transfer
> parameter. As a result, I am considering a new idea when per-transfer
> configuration data will be passed as the first element of the scatter
> list.
> Something like a packet header data but it will be used to setup
> buffer descriptors instead of being part of data stream.
Alexandre,
dmac gets physical address, so you would need to remap that again to be
able to parse these buffers... Sorry I dont like this idea at all.
> 
> This new approach will preserve existing DMA engine interface and be
> easier
> for backporting of RapidIO drivers. Yes, there is a system dependency in
> scatter
> list format but it is completely protected by channel filtering
> mechanism.   
> 
> Because of that it may be a good idea to consider this set of patches
> being "on hold" unless there are other parties interested in proposed
> interface changes.


-- 
~Vinod