[PATCH 0/3] Pending dmaengine patches

[PATCH 0/3] Pending dmaengine patches

[Joel Fernandes]

I've reworked Matt's dmaegine sg_limits series addressing review comments at [1].

Currently MMC on AM33xx requires EDMA which was posted and accepted, following
it is a series that are fixes that have to go in for it to work correctly.
Without this series, DMA fails on MMC resulting in I/O errors.

A subsequent MMC DT patch for AM33xx is also on hold [2] for the same reason.

Tested on AM335x (Beaglebone Rev A5A).

[1] http://linux.davincidsp.com/pipermail/davinci-linux-open-source/2013-March/026658.html
[2] http://lkml.indiana.edu/hypermail/linux/kernel/1306.3/03766.html

Matt Porter (3):
  dmaengine: add dma_get_slave_sg_limits()
  mmc: omap_hsmmc: set max_segs based on dma engine limits
  dma: edma: add device_slave_sg_limits() support

 drivers/dma/edma.c            |   14 +++++++++++++
 drivers/mmc/host/omap_hsmmc.c |    9 +++++++++
 include/linux/dmaengine.h     |   45 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+)

-- 
1.7.9.5

[PATCH 1/3] dmaengine: add dma_get_slave_sg_limits()

[Joel Fernandes]

From: Matt Porter <mporter-l0cyMroinI0@public.gmane.org>

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-l0cyMroinI0@public.gmane.org>: Changes to allocate limits
structure in client and fill up in DMAEngine implementation.]
Signed-off-by: Matt Porter <mporter-l0cyMroinI0@public.gmane.org>
Signed-off-by: Joel Fernandes <joelf-l0cyMroinI0@public.gmane.org>
Cc: Mark Jackson <mpfj-list-2FZW7xY0fHgqdlJmJB21zg@public.gmane.org>
---
 include/linux/dmaengine.h |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

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);
-- 
1.7.9.5

Re: [PATCH 1/3] dmaengine: add dma_get_slave_sg_limits()

[Vinod Koul]

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

Re: [PATCH 1/3] dmaengine: add dma_get_slave_sg_limits()

[Joel Fernandes]

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-l0cyMroinI0@public.gmane.org>
>>
>> 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

Re: [PATCH 1/3] dmaengine: add dma_get_slave_sg_limits()

[Lars-Peter Clausen]

On 07/22/2013 11:45 PM, Joel Fernandes wrote:
> 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-l0cyMroinI0@public.gmane.org>
>>>
>>> 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
>

And it was changed back, since we need to provide more information than just 
the sg limits. So the name didn't fit anymore. The dma_get_slave_caps() API 
patch was based on the dma_get_slave_sg_limits() patch.

> 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?

If you need to calculate the limit based on a certain configuration I'd suggest 
to add a optional dma_slave_config parameter to the dma_slave_get_caps() function.

- Lars

Re: [PATCH 1/3] dmaengine: add dma_get_slave_sg_limits()

[Joel Fernandes]

On 07/23/2013 01:41 AM, Lars-Peter Clausen wrote:
> On 07/22/2013 11:45 PM, Joel Fernandes wrote:
>> 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-l0cyMroinI0@public.gmane.org>
>>>>
>>>> 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
>>
>>
> 
> And it was changed back, since we need to provide more information than
> just the sg limits. So the name didn't fit anymore. The
> dma_get_slave_caps() API patch was based on the
> dma_get_slave_sg_limits() patch.

Well including the functionality together with it doesn't seem to solve
the problem, but correct me if I'm wrong..

The only I have with get_slave_caps is due to the other additional
information it provides like "addr widths allowed" etc.. it seems that
it should called *before* the channel configuration. Is this not true?

If that's the case then we can't use it, because we want a caps API that
can be called either after a channel is configured or the available
address widths are already known..

So the flow would go something like:
1. Determine address widths, etc before hand (driver already knows..)
2. Configure the channel or store this info somewhere.
3. Pass this information to get_caps and the max segment size which is
calculated by the get_caps implementation based on the data passed in 1.

So 3. depends on 1.
Is that an acceptable use? If semantically we can use it as above then
I'm ok.. we will just ignore the extra "possible widths" and other info
stored in get_caps..

-Joel

> 
>> 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?
> 
> If you need to calculate the limit based on a certain configuration I'd
> suggest to add a optional dma_slave_config parameter to the
> dma_slave_get_caps() function.
> 
> - Lars
> 


-- 
Thanks,

-Joel

Re: [PATCH 1/3] dmaengine: add dma_get_slave_sg_limits()

[Russell King - ARM Linux]

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.

Re: [PATCH 1/3] dmaengine: add dma_get_slave_sg_limits()

[Joel Fernandes]

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

Re: [PATCH 1/3] dmaengine: add dma_get_slave_sg_limits()

[Vinod Koul]

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
-- 

[PATCH 2/3] mmc: omap_hsmmc: set max_segs based on dma engine limits

[Joel Fernandes]

From: Matt Porter <mporter-l0cyMroinI0@public.gmane.org>

The EDMA DMAC has a hardware limitation that prevents supporting
scatter gather lists with any number of segments. The DMA Engine
API reports the maximum number of segments a channel can support
via the optional dma_get_slave_sg_limits() API. If the max_nr_segs
limit is present, the value is used to configure mmc->max_segs
appropriately.

[Joel Fernandes <joelf-l0cyMroinI0@public.gmane.org>: Allocate sg_limits structure in
client driver, and have the dmaengine implementation fill it up]
Signed-off-by: Matt Porter <mporter-l0cyMroinI0@public.gmane.org>
Acked-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Joel Fernandes <joelf-l0cyMroinI0@public.gmane.org>
Cc: Mark Jackson <mpfj-list-2FZW7xY0fHgqdlJmJB21zg@public.gmane.org>
---
 drivers/mmc/host/omap_hsmmc.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index eccedc7..b723095 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1776,6 +1776,7 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
 	const struct of_device_id *match;
 	dma_cap_mask_t mask;
 	unsigned tx_req, rx_req;
+	struct dma_slave_sg_limits dma_sg_limits;
 	struct pinctrl *pinctrl;
 
 	match = of_match_device(of_match_ptr(omap_mmc_of_match), &pdev->dev);
@@ -1952,6 +1953,14 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
 		goto err_irq;
 	}
 
+	/* Some DMA Engines only handle a limited number of SG segments */
+	ret = dma_get_slave_sg_limits(host->rx_chan,
+				DMA_SLAVE_BUSWIDTH_4_BYTES,
+				mmc->max_blk_size / 4,
+				&dma_sg_limits);
+	if (!ret && dma_sg_limits.max_seg_nr)
+		mmc->max_segs = dma_sg_limits.max_seg_nr;
+
 	/* Request IRQ for MMC operations */
 	ret = request_irq(host->irq, omap_hsmmc_irq, 0,
 			mmc_hostname(mmc), host);
-- 
1.7.9.5

[PATCH 3/3] dma: edma: add device_slave_sg_limits() support

[Joel Fernandes]

From: Matt Porter <mporter-l0cyMroinI0@public.gmane.org>

Implement device_slave_sg_limits().

EDMA has a finite set of PaRAM slots available for linking a
multi-segment SG transfer. In order to prevent any one channel
from consuming all PaRAM slots to fulfill a large SG transfer,
the driver reports a static per-channel max number of SG segments
it will handle.

The maximum size of an SG segment is limited by the addr_width
and maxburst of a given transfer request. These values are
provided by the client driver and used to calculate and return
the maximum segment length.

[Joel Fernandes <joelf-l0cyMroinI0@public.gmane.org>: Changes for filling sg_limits
by DMAEngine implementation and allocating in client, channel
parameter is unused in this implementation as all channels
have the same capability]
Signed-off-by: Matt Porter <mporter-l0cyMroinI0@public.gmane.org>
Signed-off-by: Joel Fernandes <joelf-l0cyMroinI0@public.gmane.org>
Cc: Mark Jackson <mpfj-list-2FZW7xY0fHgqdlJmJB21zg@public.gmane.org>
---
 drivers/dma/edma.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 5f3e532..964de26 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -462,6 +462,19 @@ static void edma_issue_pending(struct dma_chan *chan)
 	spin_unlock_irqrestore(&echan->vchan.lock, flags);
 }
 
+static inline int edma_get_slave_sg_limits(struct dma_chan *chan,
+		       enum dma_slave_buswidth addr_width,
+		       u32 maxburst,
+		       struct dma_slave_sg_limits *sg_limits)
+{
+	if (!sg_limits)
+		return -EINVAL;
+	sg_limits->max_seg_nr = MAX_NR_SG;
+	sg_limits->max_seg_len =
+		(SZ_64K - 1) * addr_width * maxburst;
+	return 0;
+}
+
 static size_t edma_desc_size(struct edma_desc *edesc)
 {
 	int i;
@@ -537,6 +550,7 @@ static void edma_dma_init(struct edma_cc *ecc, struct dma_device *dma,
 	dma->device_alloc_chan_resources = edma_alloc_chan_resources;
 	dma->device_free_chan_resources = edma_free_chan_resources;
 	dma->device_issue_pending = edma_issue_pending;
+	dma->device_slave_sg_limits = edma_get_slave_sg_limits;
 	dma->device_tx_status = edma_tx_status;
 	dma->device_control = edma_control;
 	dma->dev = dev;
-- 
1.7.9.5