| Message ID | 1374166001-31340-2-git-send-email-joelf@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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
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@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 > 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
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@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 >> >> > > 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 >
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 --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);