[PATCH] atmel-mci: fix initialization of dma slave data

[PATCH] atmel-mci: fix initialization of dma slave data

[Dan Williams]

The conversion of atmel-mci to dma_request_channel missed the initialization
of the channel dma_slave information.  dma_request_channel, along with the
filter_fn, find the channel with the proper capabilities, then it is up to the
driver to update the dw_dma_chan with its slave data.

Reported-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Haavard,

I do not have any conflicting dmaengine patches so feel free to carry
this through the avr32 tree.

Regards,
Dan

 drivers/dma/dw_dmac_regs.h   |   24 ------------------------
 drivers/mmc/host/atmel-mci.c |    1 +
 include/linux/dw_dmac.h      |   33 +++++++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index 00fdd18..10ffa5c 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -126,24 +126,6 @@ struct dw_dma_regs {
 
 #define DW_REGLEN		0x400
 
-struct dw_dma_chan {
-	struct dma_chan		chan;
-	void __iomem		*ch_regs;
-	u8			mask;
-
-	spinlock_t		lock;
-
-	/* these other elements are all protected by lock */
-	dma_cookie_t		completed;
-	struct list_head	active_list;
-	struct list_head	queue;
-	struct list_head	free_list;
-
-	struct dw_dma_slave	*dws;
-
-	unsigned int		descs_allocated;
-};
-
 static inline struct dw_dma_chan_regs __iomem *
 __dwc_regs(struct dw_dma_chan *dwc)
 {
@@ -155,12 +137,6 @@ __dwc_regs(struct dw_dma_chan *dwc)
 #define channel_writel(dwc, name, val) \
 	__raw_writel((val), &(__dwc_regs(dwc)->name))
 
-static inline struct dw_dma_chan *to_dw_dma_chan(struct dma_chan *chan)
-{
-	return container_of(chan, struct dw_dma_chan, chan);
-}
-
-
 struct dw_dma {
 	struct dma_device	dma;
 	void __iomem		*regs;
diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
index 76bfe16..2a81d46 100644
--- a/drivers/mmc/host/atmel-mci.c
+++ b/drivers/mmc/host/atmel-mci.c
@@ -1618,6 +1618,7 @@ static int __init atmci_probe(struct platform_device *pdev)
 		dma_cap_zero(mask);
 		dma_cap_set(DMA_SLAVE, mask);
 		host->dma.chan = dma_request_channel(mask, filter, dws);
+		set_dw_dma_slave(host->dma.chan, dws);
 	}
 	if (!host->dma.chan)
 		dev_notice(&pdev->dev, "DMA not available, using PIO\n");
diff --git a/include/linux/dw_dmac.h b/include/linux/dw_dmac.h
index d797dde..799b6ef 100644
--- a/include/linux/dw_dmac.h
+++ b/include/linux/dw_dmac.h
@@ -54,6 +54,39 @@ struct dw_dma_slave {
 	u32			cfg_lo;
 };
 
+struct dw_dma_chan {
+	struct dma_chan		chan;
+	void __iomem		*ch_regs;
+	u8			mask;
+
+	spinlock_t		lock;
+
+	/* these other elements are all protected by lock */
+	dma_cookie_t		completed;
+	struct list_head	active_list;
+	struct list_head	queue;
+	struct list_head	free_list;
+
+	struct dw_dma_slave	*dws;
+
+	unsigned int		descs_allocated;
+};
+
+static inline struct dw_dma_chan *to_dw_dma_chan(struct dma_chan *chan)
+{
+	return container_of(chan, struct dw_dma_chan, chan);
+}
+
+static inline void set_dw_dma_slave(struct dma_chan *chan, struct dw_dma_slave *dws)
+{
+	if (chan) {
+		struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
+
+		dwc->dws = dws;
+	}
+}
+
+
 /* Platform-configurable bits in CFG_HI */
 #define DWC_CFGH_FCMODE		(1 << 0)
 #define DWC_CFGH_FIFO_MODE	(1 << 1)

Re: [PATCH] atmel-mci: fix initialization of dma slave data

[Atsushi Nemoto]

On Fri, 30 Jan 2009 16:05:57 -0700, Dan Williams <dan.j.williams@intel.com> wrote:
> The conversion of atmel-mci to dma_request_channel missed the initialization
> of the channel dma_slave information.  dma_request_channel, along with the
> filter_fn, find the channel with the proper capabilities, then it is up to the
> driver to update the dw_dma_chan with its slave data.
> 
> Reported-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Thanks.  This patch disclose struct dw_dma_chan to its client.  But
the struct seems too private for me.  Maybe we need some generic
method to pass slave information?  Any idea or plan?

---
Atsushi Nemoto

Re: [PATCH] atmel-mci: fix initialization of dma slave data

[Atsushi Nemoto]

On Sat, 31 Jan 2009 21:06:51 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> > The conversion of atmel-mci to dma_request_channel missed the initialization
> > of the channel dma_slave information.  dma_request_channel, along with the
> > filter_fn, find the channel with the proper capabilities, then it is up to the
> > driver to update the dw_dma_chan with its slave data.
> > 
> > Reported-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> Thanks.  This patch disclose struct dw_dma_chan to its client.  But
> the struct seems too private for me.  Maybe we need some generic
> method to pass slave information?  Any idea or plan?

Also, it seems too late to initialize dwc->dws after returning from
dma_request_channel().  The alloc_chan_resources routine of dw_dmac
driver needs dmc->dws and it is called from dma_request_channel() via
dma_chan_get().

---
Atsushi Nemoto

Re: [PATCH] atmel-mci: fix initialization of dma slave data

[Dan Williams]

Subject: atmel-mci: fix initialization of dma slave data

From: Dan Williams <dan.j.williams@intel.com>

The conversion of atmel-mci to dma_request_channel missed the
initialization of the channel dma_slave information.  The filter_fn
passed to dma_request_channel is responsible for initializing the
channel's private data.  This implementation has the additional benefit
of enabling a generic client-channel data passing mechanism.

Reviewed-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
On Sat, 2009-01-31 at 07:52 -0700, Atsushi Nemoto wrote: 
> > Thanks.  This patch disclose struct dw_dma_chan to its client.  But
> > the struct seems too private for me.  Maybe we need some generic
> > method to pass slave information?  Any idea or plan?
> 
> Also, it seems too late to initialize dwc->dws after returning from
> dma_request_channel().  The alloc_chan_resources routine of dw_dmac
> driver needs dmc->dws and it is called from dma_request_channel() via
> dma_chan_get().

Thanks for the review Atsushi, much appreciated.  I thought the
Reviewed-by tag was appropriate.

 drivers/dma/dmaengine.c      |    2 ++
 drivers/dma/dw_dmac.c        |    5 ++---
 drivers/dma/dw_dmac_regs.h   |    2 --
 drivers/mmc/host/atmel-mci.c |    5 +++--
 include/linux/dmaengine.h    |    2 ++
 5 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index a589930..280a9d2 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -518,6 +518,7 @@ struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, v
 				       dma_chan_name(chan), err);
 			else
 				break;
+			chan->private = NULL;
 			chan = NULL;
 		}
 	}
@@ -536,6 +537,7 @@ void dma_release_channel(struct dma_chan *chan)
 	WARN_ONCE(chan->client_count != 1,
 		  "chan reference count %d != 1\n", chan->client_count);
 	dma_chan_put(chan);
+	chan->private = NULL;
 	mutex_unlock(&dma_list_mutex);
 }
 EXPORT_SYMBOL_GPL(dma_release_channel);
diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 6b702cc..a97c07e 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -560,7 +560,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		unsigned long flags)
 {
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
-	struct dw_dma_slave	*dws = dwc->dws;
+	struct dw_dma_slave	*dws = chan->private;
 	struct dw_desc		*prev;
 	struct dw_desc		*first;
 	u32			ctllo;
@@ -790,7 +790,7 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 	cfghi = DWC_CFGH_FIFO_MODE;
 	cfglo = 0;
 
-	dws = dwc->dws;
+	dws = chan->private;
 	if (dws) {
 		/*
 		 * We need controller-specific data to set up slave
@@ -866,7 +866,6 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	spin_lock_bh(&dwc->lock);
 	list_splice_init(&dwc->free_list, &list);
 	dwc->descs_allocated = 0;
-	dwc->dws = NULL;
 
 	/* Disable interrupts */
 	channel_clear_bit(dw, MASK.XFER, dwc->mask);
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index 00fdd18..b252b20 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -139,8 +139,6 @@ struct dw_dma_chan {
 	struct list_head	queue;
 	struct list_head	free_list;
 
-	struct dw_dma_slave	*dws;
-
 	unsigned int		descs_allocated;
 };
 
diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
index 76bfe16..2b1196e 100644
--- a/drivers/mmc/host/atmel-mci.c
+++ b/drivers/mmc/host/atmel-mci.c
@@ -1548,9 +1548,10 @@ static bool filter(struct dma_chan *chan, void *slave)
 {
 	struct dw_dma_slave *dws = slave;
 
-	if (dws->dma_dev == chan->device->dev)
+	if (dws->dma_dev == chan->device->dev) {
+		chan->private = dws;
 		return true;
-	else
+	} else
 		return false;
 }
 #endif
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 3e0f64c..494aa0d 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -121,6 +121,7 @@ struct dma_chan_percpu {
  * @local: per-cpu pointer to a struct dma_chan_percpu
  * @client-count: how many clients are using this channel
  * @table_count: number of appearances in the mem-to-mem allocation table
+ * @private: private data for certain client-channel associations
  */
 struct dma_chan {
 	struct dma_device *device;
@@ -134,6 +135,7 @@ struct dma_chan {
 	struct dma_chan_percpu *local;
 	int client_count;
 	int table_count;
+	void *private;
 };
 
 /**

Re: [PATCH] atmel-mci: fix initialization of dma slave data

[Hans-Christian Egtvedt]

On Sat, 31 Jan 2009 09:26:43 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Subject: atmel-mci: fix initialization of dma slave data
> 
> From: Dan Williams <dan.j.williams@intel.com>
> 
> The conversion of atmel-mci to dma_request_channel missed the
> initialization of the channel dma_slave information.  The filter_fn
> passed to dma_request_channel is responsible for initializing the
> channel's private data.  This implementation has the additional
> benefit of enabling a generic client-channel data passing mechanism.
> 

FWIW I ran this through an ALSA sound driver I'm working on for AVR32,
uses the DW DMA controller and this was exactly what I needed to pass
on the information about the slave. Seems to behave fine on my setup,
I'm working against the 2.6.29-rc3 tag.

> Reviewed-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

<snipp patch which adds a private pointer to struct dma_chan>

-- 
Best regards,
Hans-Christian Egtvedt

Re: [PATCH] atmel-mci: fix initialization of dma slave data

[Atsushi Nemoto]

On Sat, 31 Jan 2009 09:26:43 -0700, Dan Williams <dan.j.williams@intel.com> wrote:
> Subject: atmel-mci: fix initialization of dma slave data
> 
> From: Dan Williams <dan.j.williams@intel.com>
> 
> The conversion of atmel-mci to dma_request_channel missed the
> initialization of the channel dma_slave information.  The filter_fn
> passed to dma_request_channel is responsible for initializing the
> channel's private data.  This implementation has the additional benefit
> of enabling a generic client-channel data passing mechanism.

Thank you.  It looks fine for me.

---
Atsushi Nemoto