Patchwork [v2,2/3] dma: edma: add device_channel_caps() support

login
register
mail settings
Submitter Matt Porter
Date Jan. 10, 2013, 7:07 p.m.
Message ID <1357844826-30746-3-git-send-email-mporter@ti.com>
Download mbox | patch
Permalink /patch/1961591/
State New, archived
Headers show

Comments

Matt Porter - Jan. 10, 2013, 7:07 p.m.
Implement device_channel_caps().

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 SG segment is limited by the slave config
maxburst and addr_width for the channel in question. These values
are used from the current channel config to calculate and return
the max segment length cap.

Signed-off-by: Matt Porter <mporter@ti.com>
---
 drivers/dma/edma.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
Vinod Koul - Jan. 20, 2013, 1:03 p.m.
On Thu, Jan 10, 2013 at 02:07:05PM -0500, Matt Porter wrote:
> Implement device_channel_caps().
> 
> 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 SG segment is limited by the slave config
> maxburst and addr_width for the channel in question. These values
> are used from the current channel config to calculate and return
> the max segment length cap.
> 
> Signed-off-by: Matt Porter <mporter@ti.com>
> ---
>  drivers/dma/edma.c |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> index 82c8672..fc4b9db 100644
> --- a/drivers/dma/edma.c
> +++ b/drivers/dma/edma.c
> @@ -70,6 +70,7 @@ struct edma_chan {
>  	bool				alloced;
>  	int				slot[EDMA_MAX_SLOTS];
>  	struct dma_slave_config		cfg;
> +	struct dmaengine_chan_caps	caps;
>  };
>  
>  struct edma_cc {
> @@ -462,6 +463,28 @@ static void edma_issue_pending(struct dma_chan *chan)
>  	spin_unlock_irqrestore(&echan->vchan.lock, flags);
>  }
>  
> +static struct dmaengine_chan_caps
> +*edma_get_channel_caps(struct dma_chan *chan, enum dma_transfer_direction dir)
> +{
> +	struct edma_chan *echan;
> +	enum dma_slave_buswidth width = 0;
> +	u32 burst = 0;
> +
> +	if (chan) {
I think this check is redundant.
> +		echan = to_edma_chan(chan);
> +		if (dir == DMA_MEM_TO_DEV) {
> +			width = echan->cfg.dst_addr_width;
> +			burst = echan->cfg.dst_maxburst;
Per you API example burst and width is not set yet, so this doesn't make sense
> +		} else if (dir == DMA_DEV_TO_MEM) {
> +			width = echan->cfg.src_addr_width;
> +			burst = echan->cfg.src_maxburst;
> +		}
> +		echan->caps.seg_len = (SZ_64K - 1) * width * burst;
Also the defination of API is max, above computation doesn't make sense to me!

--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Matt Porter - Jan. 20, 2013, 4:51 p.m.
On Sun, Jan 20, 2013 at 01:03:21PM +0000, Vinod Koul wrote:
> On Thu, Jan 10, 2013 at 02:07:05PM -0500, Matt Porter wrote:
> > Implement device_channel_caps().
> > 
> > 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 SG segment is limited by the slave config
> > maxburst and addr_width for the channel in question. These values
> > are used from the current channel config to calculate and return
> > the max segment length cap.
> > 
> > Signed-off-by: Matt Porter <mporter@ti.com>
> > ---
> >  drivers/dma/edma.c |   27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> > index 82c8672..fc4b9db 100644
> > --- a/drivers/dma/edma.c
> > +++ b/drivers/dma/edma.c
> > @@ -70,6 +70,7 @@ struct edma_chan {
> >  	bool				alloced;
> >  	int				slot[EDMA_MAX_SLOTS];
> >  	struct dma_slave_config		cfg;
> > +	struct dmaengine_chan_caps	caps;
> >  };
> >  
> >  struct edma_cc {
> > @@ -462,6 +463,28 @@ static void edma_issue_pending(struct dma_chan *chan)
> >  	spin_unlock_irqrestore(&echan->vchan.lock, flags);
> >  }
> >  
> > +static struct dmaengine_chan_caps
> > +*edma_get_channel_caps(struct dma_chan *chan, enum dma_transfer_direction dir)
> > +{
> > +	struct edma_chan *echan;
> > +	enum dma_slave_buswidth width = 0;
> > +	u32 burst = 0;
> > +
> > +	if (chan) {
> I think this check is redundant.

Yes, will remove.

> > +		echan = to_edma_chan(chan);
> > +		if (dir == DMA_MEM_TO_DEV) {
> > +			width = echan->cfg.dst_addr_width;
> > +			burst = echan->cfg.dst_maxburst;
> Per you API example burst and width is not set yet, so this doesn't make sense

The explanation in the cover letter mentions that dmaengine_slave_config() is
required to be called prior to dmaengine_get_channel_caps(). If we
switch to the alternative API, then that would go away including the
dependency on direction.

> > +		} else if (dir == DMA_DEV_TO_MEM) {
> > +			width = echan->cfg.src_addr_width;
> > +			burst = echan->cfg.src_maxburst;
> > +		}
> > +		echan->caps.seg_len = (SZ_64K - 1) * width * burst;
> Also the defination of API is max, above computation doesn't make sense to me!

Ok, so in this case, the slave driver has informed the dmaengine driver
that the max burst for the channel is foo. That's in units of
addr_width. On the EDMA DMAC, we program burst transfers by setting ACNT
to our per-transfer width (FIFO width in the slave SG case we are
covering here) then BCNT gets the maxburst setting. We then have a CCNT
field for EDMA that has a limit of SZ_64K-1 transfers. Thus, if a slave
driver tells us the maxburst for the channel is foo and our width is
bar, the maximum size of an SG segment in that configuration is:
(SZ_64K - 1) * bar * foo. 

-Matt
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Vinod Koul - Jan. 21, 2013, 3:16 a.m.
On Sun, Jan 20, 2013 at 11:51:08AM -0500, Matt Porter wrote:
> The explanation in the cover letter mentions that dmaengine_slave_config() is
> required to be called prior to dmaengine_get_channel_caps(). If we
> switch to the alternative API, then that would go away including the
> dependency on direction.
Nope you got that wrong!

--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Matt Porter - Jan. 21, 2013, 6:29 p.m.
On Mon, Jan 21, 2013 at 03:16:32AM +0000, Vinod Koul wrote:
> On Sun, Jan 20, 2013 at 11:51:08AM -0500, Matt Porter wrote:
> > The explanation in the cover letter mentions that dmaengine_slave_config() is
> > required to be called prior to dmaengine_get_channel_caps(). If we
> > switch to the alternative API, then that would go away including the
> > dependency on direction.
> Nope you got that wrong!

:) Yes, dropped the ball there, should have been for the api to make
sense as implemented:

1. Allocate a DMA slave channel
2. Set slave and controller specific parameters
2a. [Optionally] Get channel capabilities
3. Get a descriptor for transaction
4. Submit the transaction
5. Issue pending requests and wait for callback notification

FWIW, the implementation example in the davinci mmc client driver shows
proper use as in the correct documentation above.

-Matt
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 82c8672..fc4b9db 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -70,6 +70,7 @@  struct edma_chan {
 	bool				alloced;
 	int				slot[EDMA_MAX_SLOTS];
 	struct dma_slave_config		cfg;
+	struct dmaengine_chan_caps	caps;
 };
 
 struct edma_cc {
@@ -462,6 +463,28 @@  static void edma_issue_pending(struct dma_chan *chan)
 	spin_unlock_irqrestore(&echan->vchan.lock, flags);
 }
 
+static struct dmaengine_chan_caps
+*edma_get_channel_caps(struct dma_chan *chan, enum dma_transfer_direction dir)
+{
+	struct edma_chan *echan;
+	enum dma_slave_buswidth width = 0;
+	u32 burst = 0;
+
+	if (chan) {
+		echan = to_edma_chan(chan);
+		if (dir == DMA_MEM_TO_DEV) {
+			width = echan->cfg.dst_addr_width;
+			burst = echan->cfg.dst_maxburst;
+		} else if (dir == DMA_DEV_TO_MEM) {
+			width = echan->cfg.src_addr_width;
+			burst = echan->cfg.src_maxburst;
+		}
+		echan->caps.seg_len = (SZ_64K - 1) * width * burst;
+		return &echan->caps;
+	}
+	return NULL;
+}
+
 static size_t edma_desc_size(struct edma_desc *edesc)
 {
 	int i;
@@ -521,6 +544,9 @@  static void __init edma_chan_init(struct edma_cc *ecc,
 		echan->ch_num = EDMA_CTLR_CHAN(ecc->ctlr, i);
 		echan->ecc = ecc;
 		echan->vchan.desc_free = edma_desc_free;
+		dma_cap_set(DMA_SLAVE, echan->caps.cap_mask);
+		dma_cap_set(DMA_SG, echan->caps.cap_mask);
+		echan->caps.seg_nr = MAX_NR_SG;
 
 		vchan_init(&echan->vchan, dma);
 
@@ -537,6 +563,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_channel_caps = edma_get_channel_caps;
 	dma->device_tx_status = edma_tx_status;
 	dma->device_control = edma_control;
 	dma->dev = dev;