Message-ID: <0CE8B6BE3C4AD74AB97D9D29BD24E552028729F9@CORPEXCH1.na.ads.idt.com>
Date:	Mon, 6 Feb 2012 07:04:32 -0800
From:	"Bounine, Alexandre" <Alexandre.Bounine@....com>
To:	Vinod Koul <vinod.koul@...el.com>,
	Russell King <rmk@....linux.org.uk>
CC:	<dan.j.williams@...el.com>, <linux-kernel@...r.kernel.org>,
	<akpm@...ux-foundation.org>,
	Jassi Brar <jaswinder.singh@...aro.org>,
	"Kumar Gala" <galak@...nel.crashing.org>,
	Matt Porter <mporter@...nel.crashing.org>,
	Li Yang <leoli@...escale.com>
Subject: RE: [PATCH 01/11] dmaengine: add context parameter toprep_slave_sg and prep_dma_cyclic

On Mon, Feb 06, 2012 at 6:53 AM, Vinod Koul wrote:
> 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.

I was thinking about keeping the original scatterlist for dmac unchanged,
but allocating another scatterlist in rio_dma_prep_slave_sg() and chaining
two lists together. After it passed to device specific function, it parses
first section of the chain for additional transfer parameters and use
following scatterlist as intended for dmac.

But Russell's idea (See https://lkml.org/lkml/2012/2/3/269 ) seems to be
a better way without added complexity and I am ready to take that path and
make new patches if you and Dan agree with it.   

... skip ...

Alex.

Powered by blists - more mailing lists