RE: [PATCHv4] DMAEngine: Define interleaved transfer request api

[Bounine, Alexandre]

On Fri, Oct 14, 2011 at 2:37 PM, Jassi Brar <jaswinder.singh@xxxxxxxxxx> wrote:
> 
> On 14 October 2011 23:20, Bounine, Alexandre
> <Alexandre.Bounine@xxxxxxx> wrote:
> >> On Fri, Oct 7, 2011 at 4:27 AM, Jassi Brar
> > <jaswinder.singh@xxxxxxxxxx>
> >> wrote:
> >> > On 7 October 2011 11:15, Vinod Koul <vinod.koul@xxxxxxxxx> wrote:
> >> >
> >> >> Thru this patch Jassi gave a very good try at merging DMA_SLAVE
> and
> >> >> memcpy, but more we debate this, I am still not convinced about
> >> merging
> >> >> memcpy and DMA_SLAVE yet.
> >> >>
> >> > Nobody is merging memcpy and DMA_SLAVE right away.
> >> > The api's primary purpose is to support interleave transfers.
> >> > Possibility to merge other prepares into this is a side-effect.
> >> >
> >> >> I would still argue that if we split this on same lines as
> current
> >> >> mechanism, we have clean way to convey all details for both
> cases.
> >> >>
> >> > Do you mean to have separate interleaved transfer apis for Slave
> >> > and Mem->Mem ? Please clarify.
> >> >
> >>
> >> This is a tangent, but it would be nice if this API extension also
> >> covered the needs of the incoming RapidIO case which wants to
> specify
> >> new device context information per operation (and not once at
> >> configuration time, like slave case).  Would it be enough if the
> >> transfer template included a (struct device *context) member at the
> >> end?  Most dma users could ignore it, but RapidIO could use it to do
> >> something like:
> >>
> >>    struct rio_dev *rdev = container_of(context, typeof(*rdev),
> > device);
> >>
> >> That might not be enough, but I'm concerned that making the context
> a
> >> (void *) is too flexible.  I'd rather have something like this than
> >> acquiring a lock in rio_dma_prep_slave_sg() and holding it over
> >> ->prep().  The alternative is to extend device_prep_slave_sg to take
> >> an extra parameter, but that impacts all other slave implementations
> >> with a dead parameter.
> >>
> >
> > Having context limited to the device structure will not be enough for
> > RapidIO because of 66-bit target address (dma_addr_t will not work
> > here).
> > Probably that range is out of practical use at this moment but it is
> > defined by RIO specification and I would prefer to deal with it now
> > instead of postponing it for future. Passing context using (void *)
> will
> > solve this.
> >
> OK so you need a void* to contain all info. Agreed.
> But doesn't the info, pointed to by this (void *), remain same for
> every
> transfer to a particular target/remote device ?
No. An address within the target may (and most likely will) be changed for
every transfer. Target destination ID will be the same for given virtual channel. 

> If so, couldn't you stick this (void *) to the virtual channel's
> 'private' ?  'private' :D

This is what I am trying to do for physical channel ;).
Virtual channel may bring the same challenge and I may need a channel locking
if more than one requester try to read/write data to the same target RIO device.

Currently, I am leaning towards adopting Dan's idea of having subsystem specific
prep_sg() routine which will be associated with rio_mport device that provides DMA
support but keep it registered as DMA_SLAVE. In this context I am happy to see
that your patch removes BUG_ON check for DMA_SLAVE.

This also gives RapidIO greater level of independence in dealing with
RIO transfer details.

I am sorry for my delayed replies - I was on vacation.

Alex.
   
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/