diff mbox

[1/3] dmaengine: add dma_get_slave_sg_limits()

Message ID 1374166001-31340-2-git-send-email-joelf@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Joel Fernandes July 18, 2013, 4:46 p.m. UTC 
From: Matt Porter <mporter@ti.com>

Add a dmaengine API to retrieve slave SG transfer limits.

The API is optionally implemented by dmaengine drivers and when
unimplemented will return a NULL pointer. A client driver using
this API provides the required dma channel, address width, and
burst size of the transfer. dma_get_slave_sg_limits() returns an
SG limits structure with the maximum number and size of SG segments
that the given channel can handle.

[Joel Fernandes <joelf@ti.com>: Changes to allocate limits
structure in client and fill up in DMAEngine implementation.]
Signed-off-by: Matt Porter <mporter@ti.com>
Signed-off-by: Joel Fernandes <joelf@ti.com>
Cc: Mark Jackson <mpfj-list@newflow.co.uk>
---
 include/linux/dmaengine.h |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

Comments

Vinod Koul July 18, 2013, 4:16 p.m. UTC | #1 
On Thu, Jul 18, 2013 at 11:46:39AM -0500, Joel Fernandes wrote:
> From: Matt Porter <mporter@ti.com>
> 
> Add a dmaengine API to retrieve slave SG transfer limits.
> 
> The API is optionally implemented by dmaengine drivers and when
> unimplemented will return a NULL pointer. A client driver using
> this API provides the required dma channel, address width, and
> burst size of the transfer. dma_get_slave_sg_limits() returns an
> SG limits structure with the maximum number and size of SG segments
> that the given channel can handle.
Hi Joel,

I have already resurrected this and generalized the API to get the slave
capablities. 
https://lkml.org/lkml/2013/7/15/147

Please start using this API

~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux July 18, 2013, 5:08 p.m. UTC | #2 
On Thu, Jul 18, 2013 at 11:46:39AM -0500, Joel Fernandes wrote:
> The API is optionally implemented by dmaengine drivers and when
> unimplemented will return a NULL pointer. A client driver using
> this API provides the required dma channel, address width, and
> burst size of the transfer. dma_get_slave_sg_limits() returns an
> SG limits structure with the maximum number and size of SG segments
> that the given channel can handle.

Please look at what's already in struct device:

struct device {
...
        struct device_dma_parameters *dma_parms;
...
};

This provides:

struct device_dma_parameters {
        /*
         * a low level driver may set these to teach IOMMU code about
         * sg limitations.
         */
        unsigned int max_segment_size;
        unsigned long segment_boundary_mask;
};

Now, these are helpfully accessed via:

dma_get_max_seg_size(dev)
dma_set_max_seg_size(dev)
dma_get_seg_boundary(dev)
dma_set_seg_boundary(dev, mask)

Drivers already use these to work out how to construct the scatterlist
before passing it to the DMA API, which means that they should also be
used when creating a scatterlist for the DMA engine (think about it -
you have to use the DMA API to map the buffers for the DMA engine too.)

So, we already have two properties defined on a per-device basis: the
maximum size of a scatterlist segment, and the boundary over which any
segment must not cross.

The former ties up with your max_seg_len() property, though arguably it
may depend on the DMA engine access size.  The problem with implementing
this new API though is that the subsystems (such as SCSI) which already
use dma_get_max_seg_size() will be at odds with what is possible via the
DMA engine.

I strongly suggest using the infrastructure at device level and not
implementing some private DMA engine API to convey this information.

As for the maximum number of scatterlist entries, really that's a bug in
the DMA engine implementations if they can't accept arbitary lengths.
I've created DMA engine drivers for implementations where you have to
program each segment individually, ones which can have the current and
next segments, as well as those which can walk a list.  Provided you get
informed of a transfer being completed, there really is no reason for a
DMA engine driver to limit the number of scatterlist entries that it
will accept.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joel Fernandes July 18, 2013, 6:57 p.m. UTC | #3 
On 07/18/2013 12:08 PM, Russell King - ARM Linux wrote:
> On Thu, Jul 18, 2013 at 11:46:39AM -0500, Joel Fernandes wrote:
>> The API is optionally implemented by dmaengine drivers and when
>> unimplemented will return a NULL pointer. A client driver using
>> this API provides the required dma channel, address width, and
>> burst size of the transfer. dma_get_slave_sg_limits() returns an
>> SG limits structure with the maximum number and size of SG segments
>> that the given channel can handle.
> 
> Please look at what's already in struct device:
> 
> struct device {
> ...
>         struct device_dma_parameters *dma_parms;
> ...
> };
> 
> This provides:
> 
> struct device_dma_parameters {
>         /*
>          * a low level driver may set these to teach IOMMU code about
>          * sg limitations.
>          */
>         unsigned int max_segment_size;
>         unsigned long segment_boundary_mask;
> };
> 
> Now, these are helpfully accessed via:
> 
> dma_get_max_seg_size(dev)
> dma_set_max_seg_size(dev)
> dma_get_seg_boundary(dev)
> dma_set_seg_boundary(dev, mask)
> Drivers already use these to work out how to construct the scatterlist
> before passing it to the DMA API, which means that they should also be
> used when creating a scatterlist for the DMA engine (think about it -
> you have to use the DMA API to map the buffers for the DMA engine too.)
> 
> So, we already have two properties defined on a per-device basis: the
> maximum size of a scatterlist segment, and the boundary over which any
> segment must not cross.
> 
> The former ties up with your max_seg_len() property, though arguably it
> may depend on the DMA engine access size.  The problem with implementing
> this new API though is that the subsystems (such as SCSI) which already
> use dma_get_max_seg_size() will be at odds with what is possible via the
> DMA engine.

Not very clear for this particular case, are you saying the DMAEngine
driver implementation should set the max_seg_size of its own struct dev,
and then the drivers retrieve it from the channel they are allocated?

> I strongly suggest using the infrastructure at device level and not
> implementing some private DMA engine API to convey this information.

Certainly see the value. OK with either approach. Can Vinod add to the
discussion here, and we can decide a way forward? Is it ok to use the
new CAPS API added for now so that we can keep AM33xx MMC alive?
seg_size atleast is a real regression, the number of slots limit however
is related more to MMC grabbing a lot of slots. Atleast for -rc cycle
the seg_size and MMC fixes should go in.

> As for the maximum number of scatterlist entries, really that's a bug in
> the DMA engine implementations if they can't accept arbitary lengths.
> I've created DMA engine drivers for implementations where you have to
> program each segment individually, ones which can have the current and
> next segments, as well as those which can walk a list.  Provided you get
> informed of a transfer being completed, there really is no reason for a
> DMA engine driver to limit the number of scatterlist entries that it
> will accept.

Sure, that makes sense. Can you point to such a typical example
implementation to get some ideas?

Thanks,

-Joel
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joel Fernandes July 22, 2013, 9:45 p.m. UTC | #4 
On 07/18/2013 11:16 AM, Vinod Koul wrote:> On Thu, Jul 18, 2013 at
11:46:39AM -0500, Joel Fernandes wrote:
>> From: Matt Porter <mporter@ti.com>
>>
>> Add a dmaengine API to retrieve slave SG transfer limits.
>>
>> The API is optionally implemented by dmaengine drivers and when
>> unimplemented will return a NULL pointer. A client driver using
>> this API provides the required dma channel, address width, and
>> burst size of the transfer. dma_get_slave_sg_limits() returns an
>> SG limits structure with the maximum number and size of SG segments
>> that the given channel can handle.
> Hi Joel,
>
> I have already resurrected this and generalized the API to get the slave
> capablities.
> https://lkml.org/lkml/2013/7/15/147

Hi Vinod,

get_caps and get_sg_limits are 2 different things, looks like this was
already discussed earlier, and this patch series is a separate API that
adds support for SG limits.

Infact, you can already see here that he changed the name of the
function from caps to dma_get_slave_sg_limits:
http://linux.davincidsp.com/pipermail/davinci-linux-open-source/2013-March/026601.html

Considering this, what is the way forward? Can this patch series be
merged as it is a different API as discussed above?

Summarizing:
* get_caps API cannot be used for this same purpose, as get_caps is done
_before_ the DMA channel can be configured from what it looks like:
* get_sg_limits, on the other hand is supposed to already have the
parameters required for configuring the DMA channel before hand.

Are there any other changes to the get_sg_limits series you would like
before it can be applied? Any other suggestions?

Thanks,

-Joel

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vinod Koul July 29, 2013, 6:44 a.m. UTC | #5 
On Thu, Jul 18, 2013 at 01:57:33PM -0500, Joel Fernandes wrote:
> On 07/18/2013 12:08 PM, Russell King - ARM Linux wrote:
> > As for the maximum number of scatterlist entries, really that's a bug in
> > the DMA engine implementations if they can't accept arbitary lengths.
> > I've created DMA engine drivers for implementations where you have to
> > program each segment individually, ones which can have the current and
> > next segments, as well as those which can walk a list.  Provided you get
> > informed of a transfer being completed, there really is no reason for a
> > DMA engine driver to limit the number of scatterlist entries that it
> > will accept.
> 
> Sure, that makes sense. Can you point to such a typical example
> implementation to get some ideas?
MXS MMC driver uses this: drivers/mmc/host/mxs-mmc.c
And dma engine driver for this is mxs-dma.c

~Vinod
diff mbox

Patch

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 96d3e4a..2985878 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -371,6 +371,18 @@  struct dma_slave_config {
 	unsigned int slave_id;
 };
 
+/* struct dma_slave_sg_limits - expose SG transfer limits of a channel
+ *
+ * @max_seg_nr: maximum number of SG segments supported on a SG/SLAVE
+ *	    channel (0 for no maximum or not a SG/SLAVE channel)
+ * @max_seg_len: maximum length of SG segments supported on a SG/SLAVE
+ *	     channel (0 for no maximum or not a SG/SLAVE channel)
+ */
+struct dma_slave_sg_limits {
+	u32 max_seg_nr;
+	u32 max_seg_len;
+};
+
 static inline const char *dma_chan_name(struct dma_chan *chan)
 {
 	return dev_name(&chan->dev->device);
@@ -534,6 +546,7 @@  struct dma_tx_state {
  *	struct with auxiliary transfer status information, otherwise the call
  *	will just return a simple status code
  * @device_issue_pending: push pending transactions to hardware
+ * @device_slave_sg_limits: return the slave SG capabilities
  */
 struct dma_device {
 
@@ -602,6 +615,10 @@  struct dma_device {
 					    dma_cookie_t cookie,
 					    struct dma_tx_state *txstate);
 	void (*device_issue_pending)(struct dma_chan *chan);
+	int (*device_slave_sg_limits)(struct dma_chan *chan,
+		enum dma_slave_buswidth addr_width,
+		u32 maxburst,
+		struct dma_slave_sg_limits *sg_limits);
 };
 
 static inline int dmaengine_device_control(struct dma_chan *chan,
@@ -963,6 +980,34 @@  dma_set_tx_state(struct dma_tx_state *st, dma_cookie_t last, dma_cookie_t used,
 	}
 }
 
+/**
+ * dma_get_slave_sg_limits - get DMAC SG transfer capabilities
+ * @chan: target DMA channel
+ * @addr_width: address width of the DMA transfer
+ * @maxburst: maximum DMA transfer burst size
+ * @sg_limits: point to sg_limits struct to populate with limit info
+ *
+ * Get SG transfer capabilities for a specified channel. If the dmaengine
+ * driver does not implement SG transfer capabilities then NULL is
+ * returned.
+ */
+static inline int
+dma_get_slave_sg_limits(struct dma_chan *chan,
+		       enum dma_slave_buswidth addr_width,
+		       u32 maxburst,
+		       struct dma_slave_sg_limits *sg_limits)
+
+{
+
+	if (chan->device->device_slave_sg_limits && sg_limits)
+		return chan->device->device_slave_sg_limits(chan,
+							  addr_width,
+							  maxburst,
+							  sg_limits);
+
+	return -EINVAL;
+}
+
 enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie);
 #ifdef CONFIG_DMA_ENGINE
 enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx);