Date: Mon, 19 Dec 2016 11:52:10 -0600
From: Andy Gross <andy.gross@linaro.org>
To: Vinod Koul <vinod.koul@intel.com>
Cc: Abhishek Sahu <absahu@codeaurora.org>, dan.j.williams@intel.com,
        stanimir.varbanov@linaro.org, mcgrof@suse.com, okaya@codeaurora.org,
        pramod.gurav@linaro.org, arnd@arndb.de, linux-kernel@vger.kernel.org,
        dmaengine@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH 2/5] dmaengine: Add support for custom data mapping
Message-ID: <20161219175210.GB3047@hector.attlocal.net>
References: <1481795755-15302-1-git-send-email-absahu@codeaurora.org>
 <1481795755-15302-3-git-send-email-absahu@codeaurora.org>
 <20161218162602.GR25795@localhost>
 <20161219050642.GA3047@hector.attlocal.net>
 <20161219154923.GT25795@localhost>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20161219154923.GT25795@localhost>
User-Agent: Mutt/1.5.23.1 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5037
Lines: 103

On Mon, Dec 19, 2016 at 09:19:23PM +0530, Vinod Koul wrote:
> On Sun, Dec 18, 2016 at 11:06:42PM -0600, Andy Gross wrote:
> > On Sun, Dec 18, 2016 at 09:56:02PM +0530, Vinod Koul wrote:
> > > On Thu, Dec 15, 2016 at 03:25:52PM +0530, Abhishek Sahu wrote:
> > > > The current DMA APIs only support SGL or data in generic format.
> > > > The QCA BAM DMA engine data cannot be mapped with already
> > > > available APIs due to following reasons.
> > > > 
> > > > 1. The QCA BAM DMA engine uses custom flags which cannot be
> > > >    mapped with generic DMA engine flags.
> > > > 2. Some peripheral driver like QCA QPIC NAND/LCD requires to
> > > >    set specific flags (Like NWD, EOT) for some of the descriptors
> > > >    in scatter gather list. The already available mapping APIs take
> > > >    flags parameter in API itself and there is no support for
> > > >    passing DMA specific flags for each SGL entry.
> > > > 
> > > > Now this patch adds the support for making the DMA descriptor from
> > > > custom data with new DMA mapping function prep_dma_custom_mapping.
> > > > The peripheral driver will pass the custom data in this function and
> > > > DMA engine driver will form the descriptor according to its own
> > > > logic. In future, this API can be used by any other DMA engine
> > > > drivers also which are unable to do DMA mapping with already
> > > > available API’s.
> > > > 
> > > > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> > > > ---
> > > >  include/linux/dmaengine.h | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > > > index cc535a4..6324c1f 100644
> > > > --- a/include/linux/dmaengine.h
> > > > +++ b/include/linux/dmaengine.h
> > > > @@ -692,6 +692,8 @@ struct dma_filter {
> > > >   *	be called after period_len bytes have been transferred.
> > > >   * @device_prep_interleaved_dma: Transfer expression in a generic way.
> > > >   * @device_prep_dma_imm_data: DMA's 8 byte immediate data to the dst address
> > > > + * @device_prep_dma_custom_mapping: prepares a dma operation from dma driver
> > > > + *	specific custom data
> > > >   * @device_config: Pushes a new configuration to a channel, return 0 or an error
> > > >   *	code
> > > >   * @device_pause: Pauses any transfer happening on a channel. Returns
> > > > @@ -783,6 +785,9 @@ struct dma_device {
> > > >  	struct dma_async_tx_descriptor *(*device_prep_dma_imm_data)(
> > > >  		struct dma_chan *chan, dma_addr_t dst, u64 data,
> > > >  		unsigned long flags);
> > > > +	struct dma_async_tx_descriptor *(*device_prep_dma_custom_mapping)(
> > > > +		struct dma_chan *chan, void *data,
> > > > +		unsigned long flags);
> > > 
> > > This needs a discussion. Why do you need to add a new API for framework.
> > > 
> > > What are NWD and EOT flags, cna you details out the flags?
> > 
> > These are the notify when done and end of transaction flags.  I believe the last
> > time we talked about this, we (Vinod and I)  agreed to just expose a QCOM only interface to set
> > the special transaction flags.  You'd then have to have some other API to fixup
> > the descriptor with the right qcom flags.
> 
> Okay, do you have pointer on that one, will avoid asking the same questions
> again.

I'll see if I can find the correspondance and send to you directly.

> 
> > Ahbishek, correct me where i am wrong on the following:
> > So two main differences between a normal descriptor and a command descriptor:
> > 1) size of the descriptor
> > 2) the flag setting
> > 3) data sent in is a modified scatter gather that includes flags , vs a normal
> > scatter gather
> > 
> > So the CMD descriptors in a given sgl can all have varying flags set? I'd assume
> > they all have CMD flag set.  Do the current users of the command descriptors
> > coalesce all of their requests into a big list?
> > 
> > So a couple of thoughts on how to deal with this:
> > 
> > 1) Define a virtual channel for the command descriptors vs a normal DMA
> > transaction.  This lets you use the same hardware channel, but lets you discern
> > which descriptor format you need to use.  The only issue I see with this is the
> > required change in device tree binding to target the right type of channel (cmd
> > vs normal).
> 
> Or mark the descriptor is cmd and write accordingly...

The only issue i see with that is knowing how much to pre-allocate during the
prep call.  The flag set API would be called on the allocated tx descriptor.  So
you'd have to know up front and be able to specify it.

> 
> > 
> > 2) Provide an API to set flags for the descriptor on a whole descriptor basis.
> > 
> > 3) If you have a set of transactions described by an sgl that has disparate use
> > of flags, you split the list and use a separate transaction.  In other words, we
> > need to enforce that the flag set API will be applied to all descriptors
> > described by an sgl.  This means that the whole transaction may be comprised of
> > multiple async TX descriptors.

Regards,

Andy