[lkml]   [2011]   [Oct]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRE: [RFC PATCH 1/2] RapidIO: Add DMA Engine support for RIO data transfers
    Dan J Williams wrote:
    > On Mon, Oct 3, 2011 at 9:52 AM, Bounine, Alexandre
    > <> wrote:
    > >
    > > My concern here is that other subsystems may use/request DMA_SLAVE
    channel(s) as well
    > > and wrongfully acquire one that belongs to RapidIO. In this case
    separation with another
    > > flag may have a sense - it is possible to have a system that uses
    > > and other "traditional" DMA slave channel.
    > >
    > > This is why I put that proposed interface for discussion instead of
    keeping everything
    > > inside of RapidIO.
    > > If you think that situation above will not happen I will be happy to
    > > that subsystem knowledge from dmaengine files.
    > I don't think that situation will happen, even on the same arch I
    > don't think DMA_SLAVE is ready to be enabled generically. So you're
    > probably safe here.

    Thank you for confirmation. I will rely on DMA_SLAVE only as in my most

    > >
    > > I agree, this is not a case of "pure" slave transfers but existing
    > > interface fits well into the RapidIO operations.
    > >
    > > First, we have only one memory mapped location on the host side. We
    > > data to/from location that is not mapped into memory on the same
    > >
    > > Second, having ability to pass private target information allows me
    to pass
    > > information about remote target device on per-transfer basis.
    > ...but there is no expectation that these engines will be generically
    > useful to other subsytems. To be clear you are just using dmaengine
    > as a match making service for your dma providers to clients, right?

    Not only that. As an example I am offering other defined DMA slave mode
    in tsi721_dma driver. I think that RapidIO specific DMA channels should
    unified DMA engine interface. I am expecting that other DMA defined
    to be used directly without RapidIO specifics. E.g. tx_submit, tx_status
    issue_pending are implemented in tsi721_dma driver. Similar approach may
    applied to fsl_dma driver which is capable to RapidIO data transfers on
    with RapidIO interface.

    > > ... skip ...
    > > RapidIO network usually has more than one device attached to it and
    > > single DMA channel may service data transfers to/from several
    > > In this case device information should be passed on per-transfer
    > >
    > You could maybe do what async_tx does and just apply the extra context
    > after the ->prep(), but before ->submit(), but it looks like that
    > context is critical to setting up the operation.

    Yes, it is possible to do but does not look as quite safe and effective
    as passing all related parameters during single call. This would require
    RIO DMA drivers to hold a "half cooked" descriptor chain until next
    of information arrives. This requires to have prep and post-configure
    coupled together by locks. In this situation prep with private data
    safer and more effective.

    > This looks pretty dangerous without knowing the other details. What
    > prevents another thread from changing dchan->private before the the
    > prep routine reads it?

    Yes, locking is needed in rio_dma_prep_slave_sg() around a call for prep
    routine. After all internal descriptors are set dchan can be submitted
    with new private content.

    > DMA_SLAVE assumes a static relationship between dma device and
    > slave-device, instead this rapid-io case is a per-operation slave
    > context. It sounds like you really do want a new dma operation type
    > that is just an extra-parameter version of the current
    > ->device_prep_slave_sg. But now we're getting into to
    > dma_transaction_type proliferation again. This is probably more fuel
    > for the fire of creating a structure transfer template that defines
    > multiple possible operation types and clients just fill in the fields
    > that they need, rather than adding new operation types for every
    > possible permutation of copy operation (DMA_SLAVE, DMA_MEMCPY,
    > DMA_CYCLIC, DMA_SG, DMA_INTERLEAVE, DMA_RAPIDIO), it's getting to be a
    > bit much.

    Exactly. I need an ability of passing private parameter to prep
    I chose to adopt DMA engine interface instead of adding one completely
    internal to RapidIO because having one common API has much more sense.
    Passing user defined data structure when building individual request
    make not only my life easier but probably will help some other drivers
    as well.

    > As a starting point, since this the first driver proposal to have
    > per-operation slave context and there are other rapid-io specific
    > considerations, maybe it's ok to have a rio_dma_prep_slave_sg() that
    > does something like:
    > struct tsi721_bdma_chan *bdma_chan = to_tsi721_chan(dchan);
    > bdma_chan->prep_sg(rext, sgl, sg_len, direction, flags);
    > Thoughts?

    This brings HW dependence into generic RapidIO layer.
    I would like to see it as a generic interface that is available
    to other RIO-capable devices (e.g Freescale's 85xx and QorIQ).

    I would probably make ->prep_sg callback part of rio_mport structure
    (which holds dma_device). In this case HW will be well abstracted.
    The only problem will be with registering with DMA engine but this can
    be solved by providing a pointer to a stub routine.

    Second option may be keeping rio_dma_prep_slave_sg() "as is" but with an
    locking as I mentioned above (maybe remove "slave" from its name to
    reduce confusion).

    Thank you,


     \ /
      Last update: 2011-10-07 18:15    [W:0.029 / U:15.332 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site