Pending dmaengine patches

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

From: Matt Porter <[email protected]>

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 <[email protected]>: Changes to allocate limits structure in client and fill up in DMAEngine implementation.]

---
 include/linux/dmaengine.h |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

index 96d3e4a..2985878 100644
@@ -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

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

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

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.

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

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

Thanks,

-Joel

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.

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?

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.

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

Thanks,

-Joel

MXS MMC driver uses this: drivers/mmc/host/mxs-mmc.c And dma engine driver for this is mxs-dma.c

~Vinod

From: Matt Porter <[email protected]>

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 <[email protected]>: Allocate sg_limits structure in client driver, and have the dmaengine implementation fill it up]

---
 drivers/mmc/host/omap_hsmmc.c |    9 +++++++++
 1 file changed, 9 insertions(+)

index eccedc7..b723095 100644
@@ -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

From: Matt Porter <[email protected]>

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 <[email protected]>: 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]

---
 drivers/dma/edma.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

index 5f3e532..964de26 100644
@@ -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