[PATCH 0/2] mmc: atmel-mci: introduce MCI2 support on at91

[PATCH 0/2] mmc: atmel-mci: introduce MCI2 support on at91

[Nicolas Ferre]

This patchset introduces the support of a new revision of the MCI IP on AT91
SOCs. The use of an alternate DMA engine on those platforms introduces the need
of a generic way of handling dma slave interface.

Note: those patches goes on top of the following patch that is already in
mmotm:
        atmel-mci: Unified Atmel MCI drivers (AVR32 & AT91)

Nicolas Ferre (2):
      atmel-mci: change use of dma slave interface
      mmc: atmel-mci: New MCI2 module support in atmel-mci driver

 arch/avr32/mach-at32ap/at32ap700x.c |    6 +-
 drivers/mmc/host/atmel-mci.c        |  167 ++++++++++++++++++++++++++++-------
 include/linux/atmel-mci.h           |    3 +-
 3 files changed, 143 insertions(+), 33 deletions(-)

[PATCH 1/2] atmel-mci: change use of dma slave interface

[Nicolas Ferre]

Allow the use of another DMA controller driver in atmel-mci sd/mmc driver. This
adds a generic dma_slave pointer to the mci platform structure where we can
store DMA controller information. In atmel-mci we use information provided by
this structure to initialize the driver (with new helper functions that are
architecture dependant).
This also adds at32/avr32 chip modifications to cope with this new access
method.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 arch/avr32/mach-at32ap/at32ap700x.c |    6 ++-
 drivers/mmc/host/atmel-mci.c        |   82 ++++++++++++++++++++++++++---------
 include/linux/atmel-mci.h           |    3 +-
 3 files changed, 68 insertions(+), 23 deletions(-)

diff --git a/arch/avr32/mach-at32ap/at32ap700x.c b/arch/avr32/mach-at32ap/at32ap700x.c
index eb9d4dc..d1fe145 100644
--- a/arch/avr32/mach-at32ap/at32ap700x.c
+++ b/arch/avr32/mach-at32ap/at32ap700x.c
@@ -1320,7 +1320,7 @@ struct platform_device *__init
 at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
 {
 	struct platform_device		*pdev;
-	struct dw_dma_slave		*dws = &data->dma_slave;
+	struct dw_dma_slave		*dws;
 	u32				pioa_mask;
 	u32				piob_mask;
 
@@ -1339,6 +1339,8 @@ at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
 				ARRAY_SIZE(atmel_mci0_resource)))
 		goto fail;
 
+	dws = kzalloc(sizeof(struct dw_dma_slave), GFP_KERNEL);
+
 	dws->dma_dev = &dw_dmac0_device.dev;
 	dws->reg_width = DW_DMA_SLAVE_WIDTH_32BIT;
 	dws->cfg_hi = (DWC_CFGH_SRC_PER(0)
@@ -1346,6 +1348,8 @@ at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
 	dws->cfg_lo &= ~(DWC_CFGL_HS_DST_POL
 				| DWC_CFGL_HS_SRC_POL);
 
+	data->dma_slave = dws;
+
 	if (platform_device_add_data(pdev, data,
 				sizeof(struct mci_platform_data)))
 		goto fail;
diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
index 065fa81..1689396 100644
--- a/drivers/mmc/host/atmel-mci.c
+++ b/drivers/mmc/host/atmel-mci.c
@@ -1575,16 +1575,71 @@ static void __exit atmci_cleanup_slot(struct atmel_mci_slot *slot,
 }
 
 #ifdef CONFIG_MMC_ATMELMCI_DMA
-static bool filter(struct dma_chan *chan, void *slave)
+static struct device *find_slave_dev(void *slave)
+{
+	if (!slave)
+		return NULL;
+
+	if (cpu_is_at32ap7000())
+		return ((struct dw_dma_slave *)slave)->dma_dev;
+	else
+		return ((struct at_dma_slave *)slave)->dma_dev;
+}
+
+static void setup_dma_addr(struct mci_platform_data *pdata,
+			dma_addr_t tx_addr, dma_addr_t rx_addr)
 {
-	struct dw_dma_slave *dws = slave;
+	if (!pdata)
+		return;
+
+	if (cpu_is_at32ap7000()) {
+		struct dw_dma_slave *dws = pdata->dma_slave;
+
+		dws->tx_reg = tx_addr;
+		dws->rx_reg = rx_addr;
+	} else {
+		struct at_dma_slave *ats = pdata->dma_slave;
 
-	if (dws->dma_dev == chan->device->dev) {
-		chan->private = dws;
+		ats->tx_reg = tx_addr;
+		ats->rx_reg = rx_addr;
+	}
+}
+
+static bool filter(struct dma_chan *chan, void *slave)
+{
+	if (find_slave_dev(slave) == chan->device->dev) {
+		chan->private = slave;
 		return true;
-	} else
+	} else {
 		return false;
+	}
+}
+
+static void atmci_configure_dma(struct atmel_mci *host)
+{
+	struct mci_platform_data	*pdata;
+
+	if (host == NULL)
+		return;
+
+	pdata = host->pdev->dev.platform_data;
+
+	if (pdata && find_slave_dev(pdata->dma_slave)) {
+		dma_cap_mask_t mask;
+
+		setup_dma_addr(pdata, host->mapbase + MCI_TDR,
+				      host->mapbase + MCI_RDR);
+
+		/* Try to grab a DMA channel */
+		dma_cap_zero(mask);
+		dma_cap_set(DMA_SLAVE, mask);
+		host->dma.chan = dma_request_channel(mask, filter, pdata->dma_slave);
+	}
+	if (!host->dma.chan)
+		dev_notice(&host->pdev->dev, "DMA not available, using PIO\n");
 }
+#else
+static void atmci_configure_dma(struct atmel_mci *host) {}
 #endif
 
 static int __init atmci_probe(struct platform_device *pdev)
@@ -1638,22 +1693,7 @@ static int __init atmci_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_request_irq;
 
-#ifdef CONFIG_MMC_ATMELMCI_DMA
-	if (pdata->dma_slave.dma_dev) {
-		struct dw_dma_slave *dws = &pdata->dma_slave;
-		dma_cap_mask_t mask;
-
-		dws->tx_reg = regs->start + MCI_TDR;
-		dws->rx_reg = regs->start + MCI_RDR;
-
-		/* Try to grab a DMA channel */
-		dma_cap_zero(mask);
-		dma_cap_set(DMA_SLAVE, mask);
-		host->dma.chan = dma_request_channel(mask, filter, dws);
-	}
-	if (!host->dma.chan)
-		dev_notice(&pdev->dev, "DMA not available, using PIO\n");
-#endif /* CONFIG_MMC_ATMELMCI_DMA */
+	atmci_configure_dma(host);
 
 	platform_set_drvdata(pdev, host);
 
diff --git a/include/linux/atmel-mci.h b/include/linux/atmel-mci.h
index 57b1846..e26b7de 100644
--- a/include/linux/atmel-mci.h
+++ b/include/linux/atmel-mci.h
@@ -4,6 +4,7 @@
 #define ATMEL_MCI_MAX_NR_SLOTS	2
 
 #include <linux/dw_dmac.h>
+#include <mach/at_hdmac.h>
 
 /**
  * struct mci_slot_pdata - board-specific per-slot configuration
@@ -34,7 +35,7 @@ struct mci_slot_pdata {
  * @slot: Per-slot configuration data.
  */
 struct mci_platform_data {
-	struct dw_dma_slave	dma_slave;
+	void			*dma_slave;
 	struct mci_slot_pdata	slot[ATMEL_MCI_MAX_NR_SLOTS];
 };
 
-- 
1.5.6.5

[PATCH 2/2] mmc: atmel-mci: New MCI2 module support in atmel-mci driver

[Nicolas Ferre]

This new revision of the IP adds some improvements to the MCI already present
in several Atmel SOC.
Some new registers are added and a particular way of handling DMA interaction
lead to a new sequence in function call which is backward compatible: On MCI2,
we must set the DMAEN bit to enable the DMA handshaking interface. This must
happen before the data transfer command is sent.

A new function is able to differentiate MCI2 code and is based on knowledge of
processor id (cpu_is_xxx()).

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
---
 drivers/mmc/host/atmel-mci.c |   85 +++++++++++++++++++++++++++++++++++++-----
 1 files changed, 75 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
index 1689396..d9a1a8e 100644
--- a/drivers/mmc/host/atmel-mci.c
+++ b/drivers/mmc/host/atmel-mci.c
@@ -92,6 +92,7 @@ struct atmel_mci_dma {
  * @need_clock_update: Update the clock rate before the next request.
  * @need_reset: Reset controller before next request.
  * @mode_reg: Value of the MR register.
+ * @cfg_reg: Value of the CFG register.
  * @bus_hz: The rate of @mck in Hz. This forms the basis for MMC bus
  *	rate and timeout calculations.
  * @mapbase: Physical address of the MMIO registers.
@@ -155,6 +156,7 @@ struct atmel_mci {
 	bool			need_clock_update;
 	bool			need_reset;
 	u32			mode_reg;
+	u32			cfg_reg;
 	unsigned long		bus_hz;
 	unsigned long		mapbase;
 	struct clk		*mck;
@@ -223,6 +225,19 @@ static bool mci_has_rwproof(void)
 }
 
 /*
+ * The new MCI2 module isn't 100% compatible with the old MCI module,
+ * and it has a few nice features which we want to use...
+ */
+static inline bool atmci_is_mci2(void)
+{
+	if (cpu_is_at91sam9g45())
+		return true;
+
+	return false;
+}
+
+
+/*
  * The debugfs stuff below is mostly optimized away when
  * CONFIG_DEBUG_FS is not set.
  */
@@ -357,12 +372,33 @@ static int atmci_regs_show(struct seq_file *s, void *v)
 			buf[MCI_BLKR / 4],
 			buf[MCI_BLKR / 4] & 0xffff,
 			(buf[MCI_BLKR / 4] >> 16) & 0xffff);
+	if (atmci_is_mci2())
+		seq_printf(s, "CSTOR:\t0x%08x\n", buf[MCI_CSTOR / 4]);
 
 	/* Don't read RSPR and RDR; it will consume the data there */
 
 	atmci_show_status_reg(s, "SR", buf[MCI_SR / 4]);
 	atmci_show_status_reg(s, "IMR", buf[MCI_IMR / 4]);
 
+	if (atmci_is_mci2()) {
+		u32 val;
+
+		val = buf[MCI_DMA / 4];
+		seq_printf(s, "DMA:\t0x%08x OFFSET=%u CHKSIZE=%u%s\n",
+				val, val & 3,
+				((val >> 4) & 3) ?
+					1 << (((val >> 4) & 3) + 1) : 1,
+				val & MCI_DMAEN ? " DMAEN" : "");
+
+		val = buf[MCI_CFG / 4];
+		seq_printf(s, "CFG:\t0x%08x%s%s%s%s\n",
+				val,
+				val & MCI_CFG_FIFOMODE_1DATA ? " FIFOMODE_ONE_DATA" : "",
+				val & MCI_CFG_FERRCTRL_COR ? " FERRCTRL_CLEAR_ON_READ" : "",
+				val & MCI_CFG_HSMODE ? " HSMODE" : "",
+				val & MCI_CFG_LSYNC ? " LSYNC" : "");
+	}
+
 	kfree(buf);
 
 	return 0;
@@ -557,6 +593,10 @@ static void atmci_dma_complete(void *arg)
 
 	dev_vdbg(&host->pdev->dev, "DMA complete\n");
 
+	if (atmci_is_mci2())
+		/* Disable DMA hardware handshaking on MCI */
+		mci_writel(host, DMA, mci_readl(host, DMA) & ~MCI_DMAEN);
+
 	atmci_dma_cleanup(host);
 
 	/*
@@ -592,7 +632,7 @@ static void atmci_dma_complete(void *arg)
 }
 
 static int
-atmci_submit_data_dma(struct atmel_mci *host, struct mmc_data *data)
+atmci_prepare_data_dma(struct atmel_mci *host, struct mmc_data *data)
 {
 	struct dma_chan			*chan;
 	struct dma_async_tx_descriptor	*desc;
@@ -623,6 +663,9 @@ atmci_submit_data_dma(struct atmel_mci *host, struct mmc_data *data)
 	if (!chan)
 		return -ENODEV;
 
+	if (atmci_is_mci2())
+		mci_writel(host, DMA, MCI_DMA_CHKSIZE(3) | MCI_DMAEN);
+
 	if (data->flags & MMC_DATA_READ)
 		direction = DMA_FROM_DEVICE;
 	else
@@ -637,21 +680,30 @@ atmci_submit_data_dma(struct atmel_mci *host, struct mmc_data *data)
 	host->dma.data_desc = desc;
 	desc->callback = atmci_dma_complete;
 	desc->callback_param = host;
-	desc->tx_submit(desc);
-
-	/* Go! */
-	chan->device->device_issue_pending(chan);
 
 	return 0;
 }
 
+static void atmci_submit_data(struct atmel_mci *host)
+{
+	struct dma_chan			*chan = host->data_chan;
+	struct dma_async_tx_descriptor	*desc = host->dma.data_desc;
+
+	if (chan) {
+		desc->tx_submit(desc);
+		chan->device->device_issue_pending(chan);
+	}
+}
+
 #else /* CONFIG_MMC_ATMELMCI_DMA */
 
-static int atmci_submit_data_dma(struct atmel_mci *host, struct mmc_data *data)
+static int atmci_prepare_data_dma(struct atmel_mci *host, struct mmc_data *data)
 {
 	return -ENOSYS;
 }
 
+static void atmci_submit_data(struct atmel_mci *host) {}
+
 static void atmci_stop_dma(struct atmel_mci *host)
 {
 	/* Data transfer was stopped by the interrupt handler */
@@ -665,7 +717,7 @@ static void atmci_stop_dma(struct atmel_mci *host)
  * Returns a mask of interrupt flags to be enabled after the whole
  * request has been prepared.
  */
-static u32 atmci_submit_data(struct atmel_mci *host, struct mmc_data *data)
+static u32 atmci_prepare_data(struct atmel_mci *host, struct mmc_data *data)
 {
 	u32 iflags;
 
@@ -676,7 +728,7 @@ static u32 atmci_submit_data(struct atmel_mci *host, struct mmc_data *data)
 	host->data = data;
 
 	iflags = ATMCI_DATA_ERROR_FLAGS;
-	if (atmci_submit_data_dma(host, data)) {
+	if (atmci_prepare_data_dma(host, data)) {
 		host->data_chan = NULL;
 
 		/*
@@ -722,6 +774,8 @@ static void atmci_start_request(struct atmel_mci *host,
 		mci_writel(host, CR, MCI_CR_SWRST);
 		mci_writel(host, CR, MCI_CR_MCIEN);
 		mci_writel(host, MR, host->mode_reg);
+		if (atmci_is_mci2())
+			mci_writel(host, CFG, host->cfg_reg);
 		host->need_reset = false;
 	}
 	mci_writel(host, SDCR, slot->sdc_reg);
@@ -737,6 +791,7 @@ static void atmci_start_request(struct atmel_mci *host,
 		while (!(mci_readl(host, SR) & MCI_CMDRDY))
 			cpu_relax();
 	}
+	iflags = 0;
 	data = mrq->data;
 	if (data) {
 		atmci_set_timeout(host, slot, data);
@@ -746,15 +801,17 @@ static void atmci_start_request(struct atmel_mci *host,
 				| MCI_BLKLEN(data->blksz));
 		dev_vdbg(&slot->mmc->class_dev, "BLKR=0x%08x\n",
 			MCI_BCNT(data->blocks) | MCI_BLKLEN(data->blksz));
+
+		iflags |= atmci_prepare_data(host, data);
 	}
 
-	iflags = MCI_CMDRDY;
+	iflags |= MCI_CMDRDY;
 	cmd = mrq->cmd;
 	cmdflags = atmci_prepare_command(slot->mmc, cmd);
 	atmci_start_command(host, cmd, cmdflags);
 
 	if (data)
-		iflags |= atmci_submit_data(host, data);
+		atmci_submit_data(host);
 
 	if (mrq->stop) {
 		host->stop_cmdr = atmci_prepare_command(slot->mmc, mrq->stop);
@@ -850,6 +907,8 @@ static void atmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			clk_enable(host->mck);
 			mci_writel(host, CR, MCI_CR_SWRST);
 			mci_writel(host, CR, MCI_CR_MCIEN);
+			if (atmci_is_mci2())
+				mci_writel(host, CFG, host->cfg_reg);
 		}
 
 		/*
@@ -1088,6 +1147,8 @@ static void atmci_detect_change(unsigned long data)
 				mci_writel(host, CR, MCI_CR_SWRST);
 				mci_writel(host, CR, MCI_CR_MCIEN);
 				mci_writel(host, MR, host->mode_reg);
+				if (atmci_is_mci2())
+					mci_writel(host, CFG, host->cfg_reg);
 
 				host->data = NULL;
 				host->cmd = NULL;
@@ -1637,6 +1698,10 @@ static void atmci_configure_dma(struct atmel_mci *host)
 	}
 	if (!host->dma.chan)
 		dev_notice(&host->pdev->dev, "DMA not available, using PIO\n");
+	else
+		dev_info(&host->pdev->dev,
+					"Using %s for DMA transfers\n",
+					dma_chan_name(host->dma.chan));
 }
 #else
 static void atmci_configure_dma(struct atmel_mci *host) {}
-- 
1.5.6.5

Re: [PATCH 1/2] atmel-mci: change use of dma slave interface

[Andrew Morton]

On Thu, 17 Sep 2009 18:29:26 +0200
Nicolas Ferre <nicolas.ferre@atmel.com> wrote:

> Allow the use of another DMA controller driver in atmel-mci sd/mmc driver. This
> adds a generic dma_slave pointer to the mci platform structure where we can
> store DMA controller information. In atmel-mci we use information provided by
> this structure to initialize the driver (with new helper functions that are
> architecture dependant).
> This also adds at32/avr32 chip modifications to cope with this new access
> method.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
>  arch/avr32/mach-at32ap/at32ap700x.c |    6 ++-
>  drivers/mmc/host/atmel-mci.c        |   82 ++++++++++++++++++++++++++---------
>  include/linux/atmel-mci.h           |    3 +-
>  3 files changed, 68 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/avr32/mach-at32ap/at32ap700x.c b/arch/avr32/mach-at32ap/at32ap700x.c
> index eb9d4dc..d1fe145 100644
> --- a/arch/avr32/mach-at32ap/at32ap700x.c
> +++ b/arch/avr32/mach-at32ap/at32ap700x.c
> @@ -1320,7 +1320,7 @@ struct platform_device *__init
>  at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
>  {
>  	struct platform_device		*pdev;
> -	struct dw_dma_slave		*dws = &data->dma_slave;
> +	struct dw_dma_slave		*dws;
>  	u32				pioa_mask;
>  	u32				piob_mask;
>  
> @@ -1339,6 +1339,8 @@ at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
>  				ARRAY_SIZE(atmel_mci0_resource)))
>  		goto fail;
>  
> +	dws = kzalloc(sizeof(struct dw_dma_slave), GFP_KERNEL);

I don't see anywhere where this gets freed again?

>  	dws->dma_dev = &dw_dmac0_device.dev;
>  	dws->reg_width = DW_DMA_SLAVE_WIDTH_32BIT;
>  	dws->cfg_hi = (DWC_CFGH_SRC_PER(0)
> @@ -1346,6 +1348,8 @@ at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
>  	dws->cfg_lo &= ~(DWC_CFGL_HS_DST_POL
>  				| DWC_CFGL_HS_SRC_POL);
>  
> +	data->dma_slave = dws;
> +
>  	if (platform_device_add_data(pdev, data,
>  				sizeof(struct mci_platform_data)))
>  		goto fail;
> diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
> index 065fa81..1689396 100644
> --- a/drivers/mmc/host/atmel-mci.c
> +++ b/drivers/mmc/host/atmel-mci.c
> @@ -1575,16 +1575,71 @@ static void __exit atmci_cleanup_slot(struct atmel_mci_slot *slot,
>  }
>  
>  #ifdef CONFIG_MMC_ATMELMCI_DMA
> -static bool filter(struct dma_chan *chan, void *slave)
> +static struct device *find_slave_dev(void *slave)
> +{
> +	if (!slave)
> +		return NULL;
> +
> +	if (cpu_is_at32ap7000())
> +		return ((struct dw_dma_slave *)slave)->dma_dev;
> +	else
> +		return ((struct at_dma_slave *)slave)->dma_dev;
> +}

Quite a few unsafeish typecasts here.

>  struct mci_platform_data {
> -	struct dw_dma_slave	dma_slave;
> +	void			*dma_slave;
>  	struct mci_slot_pdata	slot[ATMEL_MCI_MAX_NR_SLOTS];
>  };

I think the code would come out better if this has type dw_dma_slave*.

You'll still need typecasts to support the dma_request_channel()
callback, but the code will be safer and cleaner, I expect.

Re: [PATCH 1/2] atmel-mci: change use of dma slave interface

[Nicolas Ferre]

Andrew Morton :
> On Thu, 17 Sep 2009 18:29:26 +0200
> Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> 
>> Allow the use of another DMA controller driver in atmel-mci sd/mmc driver. This
>> adds a generic dma_slave pointer to the mci platform structure where we can
>> store DMA controller information. In atmel-mci we use information provided by
>> this structure to initialize the driver (with new helper functions that are
>> architecture dependant).
>> This also adds at32/avr32 chip modifications to cope with this new access
>> method.
>>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>>  arch/avr32/mach-at32ap/at32ap700x.c |    6 ++-
>>  drivers/mmc/host/atmel-mci.c        |   82 ++++++++++++++++++++++++++---------
>>  include/linux/atmel-mci.h           |    3 +-
>>  3 files changed, 68 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/avr32/mach-at32ap/at32ap700x.c b/arch/avr32/mach-at32ap/at32ap700x.c
>> index eb9d4dc..d1fe145 100644
>> --- a/arch/avr32/mach-at32ap/at32ap700x.c
>> +++ b/arch/avr32/mach-at32ap/at32ap700x.c
>> @@ -1320,7 +1320,7 @@ struct platform_device *__init
>>  at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
>>  {
>>  	struct platform_device		*pdev;
>> -	struct dw_dma_slave		*dws = &data->dma_slave;
>> +	struct dw_dma_slave		*dws;
>>  	u32				pioa_mask;
>>  	u32				piob_mask;
>>  
>> @@ -1339,6 +1339,8 @@ at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
>>  				ARRAY_SIZE(atmel_mci0_resource)))
>>  		goto fail;
>>  
>> +	dws = kzalloc(sizeof(struct dw_dma_slave), GFP_KERNEL);
> 
> I don't see anywhere where this gets freed again?

Well, in fact those are platform initialization functions that have no
"exit" equivalent. Is this the proper way of managing this ?

Anyway, I have forgotten to free memory in case of a "fail" error case
that is present in this function. I will correct this in my v2 patch.

> 
>>  	dws->dma_dev = &dw_dmac0_device.dev;
>>  	dws->reg_width = DW_DMA_SLAVE_WIDTH_32BIT;
>>  	dws->cfg_hi = (DWC_CFGH_SRC_PER(0)
>> @@ -1346,6 +1348,8 @@ at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
>>  	dws->cfg_lo &= ~(DWC_CFGL_HS_DST_POL
>>  				| DWC_CFGL_HS_SRC_POL);
>>  
>> +	data->dma_slave = dws;
>> +
>>  	if (platform_device_add_data(pdev, data,
>>  				sizeof(struct mci_platform_data)))
>>  		goto fail;
>> diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
>> index 065fa81..1689396 100644
>> --- a/drivers/mmc/host/atmel-mci.c
>> +++ b/drivers/mmc/host/atmel-mci.c
>> @@ -1575,16 +1575,71 @@ static void __exit atmci_cleanup_slot(struct atmel_mci_slot *slot,
>>  }
>>  
>>  #ifdef CONFIG_MMC_ATMELMCI_DMA
>> -static bool filter(struct dma_chan *chan, void *slave)
>> +static struct device *find_slave_dev(void *slave)
>> +{
>> +	if (!slave)
>> +		return NULL;
>> +
>> +	if (cpu_is_at32ap7000())
>> +		return ((struct dw_dma_slave *)slave)->dma_dev;
>> +	else
>> +		return ((struct at_dma_slave *)slave)->dma_dev;
>> +}
> 
> Quite a few unsafeish typecasts here.

I am afraid, yes.

>>  struct mci_platform_data {
>> -	struct dw_dma_slave	dma_slave;
>> +	void			*dma_slave;
>>  	struct mci_slot_pdata	slot[ATMEL_MCI_MAX_NR_SLOTS];
>>  };
> 
> I think the code would come out better if this has type dw_dma_slave*.

Do you mean that I would leave dw_dma_slave* in mci_platform_data and
use this field for struct at_dma_slave content where I need it ? I
thought it was more confusing...

> You'll still need typecasts to support the dma_request_channel()
> callback, but the code will be safer and cleaner, I expect.

My concern are:
1/ allow the use of either dmaengine driver
2/ avoid too much modification to dw_dma_slave as it
is also used for audio stuff on avr32 platforms...
3/ not introduce heavy weigh solution like the use of an union

Best regards,
-- 
Nicolas Ferre

Re: [PATCH 1/2] atmel-mci: change use of dma slave interface

[Haavard Skinnemoen]

Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> > You'll still need typecasts to support the dma_request_channel()
> > callback, but the code will be safer and cleaner, I expect.  
> 
> My concern are:
> 1/ allow the use of either dmaengine driver
> 2/ avoid too much modification to dw_dma_slave as it
> is also used for audio stuff on avr32 platforms...
> 3/ not introduce heavy weigh solution like the use of an union

We used to have a struct dma_slave in linux/dmaengine.h which took care
of all this, but it got removed at some point.

How about introducing a new mach/atmel-mci.h file with a struct
mci_dma_data encapsulating either a struct dw_dma_slave or a struct
at_dma_slave, as well as any other DMA-related definitions needed by
the atmel-mci driver?

Then we just need to make sure that we either
  1) use the same name on all fields in struct {dw,at}_dma_slave which
     are used by the atmel-mci driver, or
  2) add accessor functions or macros for those fields.

I think I would prefer the latter, but both should work equally well.

Haavard

[PATCH 0/2 v2]mmc: atmel-mci: introduce MCI2 support on at91

[Nicolas Ferre]

This patchset introduces the support of a new revision of the MCI IP on AT91
SOCs. The use of an alternate DMA engine on those platforms introduces the need
of a generic way of handling dma slave interface.

Note: those patches goes on top of the following patch that is already in
mainline:
	atmel-mci: unified Atmel MCI drivers (AVR32 & AT91)

Nicolas Ferre (3):
      atmel-mci: change use of dma slave interface
      mmc: atmel-mci: New MCI2 module support in atmel-mci driver
      at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

 arch/arm/mach-at91/at91sam9g45_devices.c        |  164 +++++++++++++++++++++++
 arch/arm/mach-at91/board-sam9m10g45ek.c         |   24 ++++
 arch/arm/mach-at91/include/mach/atmel-mci.h     |   24 ++++
 arch/avr32/mach-at32ap/at32ap700x.c             |   18 ++-
 arch/avr32/mach-at32ap/include/mach/atmel-mci.h |   24 ++++
 drivers/mmc/host/Kconfig                        |    2 +-
 drivers/mmc/host/atmel-mci.c                    |  141 +++++++++++++++----
 include/linux/atmel-mci.h                       |    4 +-
 8 files changed, 362 insertions(+), 39 deletions(-)
 create mode 100644 arch/arm/mach-at91/include/mach/atmel-mci.h
 create mode 100644 arch/avr32/mach-at32ap/include/mach/atmel-mci.h

[PATCH 1/3 v2] atmel-mci: change use of dma slave interface

[Nicolas Ferre]

Allow the use of another DMA controller driver in atmel-mci sd/mmc driver.
This adds a generic dma_slave pointer to the mci platform structure where
we can store DMA controller information.  In atmel-mci we use information
provided by this structure to initialize the driver (with new helper
functions that are architecture dependant).

This also adds at32/avr32 chip modifications to cope with this new access
method.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 arch/arm/mach-at91/include/mach/atmel-mci.h     |   24 ++++++++++
 arch/avr32/mach-at32ap/at32ap700x.c             |   18 +++++--
 arch/avr32/mach-at32ap/include/mach/atmel-mci.h |   24 ++++++++++
 drivers/mmc/host/atmel-mci.c                    |   56 +++++++++++++++--------
 include/linux/atmel-mci.h                       |    4 +-
 5 files changed, 98 insertions(+), 28 deletions(-)
 create mode 100644 arch/arm/mach-at91/include/mach/atmel-mci.h
 create mode 100644 arch/avr32/mach-at32ap/include/mach/atmel-mci.h

diff --git a/arch/arm/mach-at91/include/mach/atmel-mci.h b/arch/arm/mach-at91/include/mach/atmel-mci.h
new file mode 100644
index 0000000..998cb0c
--- /dev/null
+++ b/arch/arm/mach-at91/include/mach/atmel-mci.h
@@ -0,0 +1,24 @@
+#ifndef __MACH_ATMEL_MCI_H
+#define __MACH_ATMEL_MCI_H
+
+#include <mach/at_hdmac.h>
+
+/**
+ * struct mci_dma_data - DMA data for MCI interface
+ */
+struct mci_dma_data {
+	struct at_dma_slave	sdata;
+};
+
+/* accessor macros */
+#define	slave_data_ptr(s)	(&(s)->sdata)
+#define find_slave_dev(s)	((s)->sdata.dma_dev)
+
+#define	setup_dma_addr(s, t, r)	do {		\
+	if (s) {				\
+		(s)->sdata.tx_reg = (t);	\
+		(s)->sdata.rx_reg = (r);	\
+	}					\
+} while (0)
+
+#endif /* __MACH_ATMEL_MCI_H */
diff --git a/arch/avr32/mach-at32ap/at32ap700x.c b/arch/avr32/mach-at32ap/at32ap700x.c
index eb9d4dc..b40ff39 100644
--- a/arch/avr32/mach-at32ap/at32ap700x.c
+++ b/arch/avr32/mach-at32ap/at32ap700x.c
@@ -15,6 +15,8 @@
 #include <linux/gpio.h>
 #include <linux/spi/spi.h>
 #include <linux/usb/atmel_usba_udc.h>
+
+#include <mach/atmel-mci.h>
 #include <linux/atmel-mci.h>
 
 #include <asm/io.h>
@@ -1320,7 +1322,7 @@ struct platform_device *__init
 at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
 {
 	struct platform_device		*pdev;
-	struct dw_dma_slave		*dws = &data->dma_slave;
+	struct mci_dma_slave		*slave;
 	u32				pioa_mask;
 	u32				piob_mask;
 
@@ -1339,13 +1341,17 @@ at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
 				ARRAY_SIZE(atmel_mci0_resource)))
 		goto fail;
 
-	dws->dma_dev = &dw_dmac0_device.dev;
-	dws->reg_width = DW_DMA_SLAVE_WIDTH_32BIT;
-	dws->cfg_hi = (DWC_CFGH_SRC_PER(0)
+	slave = kzalloc(sizeof(struct mci_dma_slave), GFP_KERNEL);
+
+	slave->sdata.dma_dev = &dw_dmac0_device.dev;
+	slave->sdata.reg_width = DW_DMA_SLAVE_WIDTH_32BIT;
+	slave->sdata.cfg_hi = (DWC_CFGH_SRC_PER(0)
 				| DWC_CFGH_DST_PER(1));
-	dws->cfg_lo &= ~(DWC_CFGL_HS_DST_POL
+	slave->sdata.cfg_lo &= ~(DWC_CFGL_HS_DST_POL
 				| DWC_CFGL_HS_SRC_POL);
 
+	data->dma_slave = slave;
+
 	if (platform_device_add_data(pdev, data,
 				sizeof(struct mci_platform_data)))
 		goto fail;
@@ -1411,6 +1417,8 @@ at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
 	return pdev;
 
 fail:
+	data->dma_slave = NULL;
+	kfree(slave);
 	platform_device_put(pdev);
 	return NULL;
 }
diff --git a/arch/avr32/mach-at32ap/include/mach/atmel-mci.h b/arch/avr32/mach-at32ap/include/mach/atmel-mci.h
new file mode 100644
index 0000000..a9b3896
--- /dev/null
+++ b/arch/avr32/mach-at32ap/include/mach/atmel-mci.h
@@ -0,0 +1,24 @@
+#ifndef __MACH_ATMEL_MCI_H
+#define __MACH_ATMEL_MCI_H
+
+#include <linux/dw_dmac.h>
+
+/**
+ * struct mci_dma_data - DMA data for MCI interface
+ */
+struct mci_dma_data {
+	struct dw_dma_slave	sdata;
+};
+
+/* accessor macros */
+#define	slave_data_ptr(s)	(&(s)->sdata)
+#define find_slave_dev(s)	((s)->sdata.dma_dev)
+
+#define	setup_dma_addr(s, t, r)	do {		\
+	if (s) {				\
+		(s)->sdata.tx_reg = (t);	\
+		(s)->sdata.rx_reg = (r);	\
+	}					\
+} while (0)
+
+#endif /* __MACH_ATMEL_MCI_H */
diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
index fc25586..ba8b219 100644
--- a/drivers/mmc/host/atmel-mci.c
+++ b/drivers/mmc/host/atmel-mci.c
@@ -25,6 +25,8 @@
 #include <linux/stat.h>
 
 #include <linux/mmc/host.h>
+
+#include <mach/atmel-mci.h>
 #include <linux/atmel-mci.h>
 
 #include <asm/io.h>
@@ -1584,14 +1586,43 @@ static void __exit atmci_cleanup_slot(struct atmel_mci_slot *slot,
 #ifdef CONFIG_MMC_ATMELMCI_DMA
 static bool filter(struct dma_chan *chan, void *slave)
 {
-	struct dw_dma_slave *dws = slave;
+	struct mci_dma_data	*sl = slave;
 
-	if (dws->dma_dev == chan->device->dev) {
-		chan->private = dws;
+	if (sl && find_slave_dev(sl) == chan->device->dev) {
+		chan->private = slave_data_ptr(sl);
 		return true;
-	} else
+	} else {
 		return false;
+	}
 }
+
+static void atmci_configure_dma(struct atmel_mci *host)
+{
+	struct mci_platform_data	*pdata;
+
+	if (host == NULL)
+		return;
+
+	pdata = host->pdev->dev.platform_data;
+
+	if (pdata && find_slave_dev(pdata->dma_slave)) {
+		dma_cap_mask_t mask;
+
+		setup_dma_addr(pdata->dma_slave,
+			       host->mapbase + MCI_TDR,
+			       host->mapbase + MCI_RDR);
+
+		/* Try to grab a DMA channel */
+		dma_cap_zero(mask);
+		dma_cap_set(DMA_SLAVE, mask);
+		host->dma.chan =
+			dma_request_channel(mask, filter, pdata->dma_slave);
+	}
+	if (!host->dma.chan)
+		dev_notice(&host->pdev->dev, "DMA not available, using PIO\n");
+}
+#else
+static void atmci_configure_dma(struct atmel_mci *host) {}
 #endif
 
 static int __init atmci_probe(struct platform_device *pdev)
@@ -1645,22 +1676,7 @@ static int __init atmci_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_request_irq;
 
-#ifdef CONFIG_MMC_ATMELMCI_DMA
-	if (pdata->dma_slave.dma_dev) {
-		struct dw_dma_slave *dws = &pdata->dma_slave;
-		dma_cap_mask_t mask;
-
-		dws->tx_reg = regs->start + MCI_TDR;
-		dws->rx_reg = regs->start + MCI_RDR;
-
-		/* Try to grab a DMA channel */
-		dma_cap_zero(mask);
-		dma_cap_set(DMA_SLAVE, mask);
-		host->dma.chan = dma_request_channel(mask, filter, dws);
-	}
-	if (!host->dma.chan)
-		dev_notice(&pdev->dev, "DMA not available, using PIO\n");
-#endif /* CONFIG_MMC_ATMELMCI_DMA */
+	atmci_configure_dma(host);
 
 	platform_set_drvdata(pdev, host);
 
diff --git a/include/linux/atmel-mci.h b/include/linux/atmel-mci.h
index 57b1846..3e09b34 100644
--- a/include/linux/atmel-mci.h
+++ b/include/linux/atmel-mci.h
@@ -3,8 +3,6 @@
 
 #define ATMEL_MCI_MAX_NR_SLOTS	2
 
-#include <linux/dw_dmac.h>
-
 /**
  * struct mci_slot_pdata - board-specific per-slot configuration
  * @bus_width: Number of data lines wired up the slot
@@ -34,7 +32,7 @@ struct mci_slot_pdata {
  * @slot: Per-slot configuration data.
  */
 struct mci_platform_data {
-	struct dw_dma_slave	dma_slave;
+	struct mci_dma_data	*dma_slave;
 	struct mci_slot_pdata	slot[ATMEL_MCI_MAX_NR_SLOTS];
 };
 
-- 
1.5.6.5

[PATCH 2/3 v2] mmc: atmel-mci: New MCI2 module support in atmel-mci driver

[Nicolas Ferre]

This new revision of the IP adds some improvements to the MCI already
present in several Atmel SOC.

Some new registers are added and a particular way of handling DMA
interaction lead to a new sequence in function call which is backward
compatible: On MCI2, we must set the DMAEN bit to enable the DMA
handshaking interface.  This must happen before the data transfer command
is sent.

A new function is able to differentiate MCI2 code and is based on
knowledge of processor id (cpu_is_xxx()).

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
---
 drivers/mmc/host/atmel-mci.c |   85 +++++++++++++++++++++++++++++++++++++-----
 1 files changed, 75 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
index ba8b219..8072128 100644
--- a/drivers/mmc/host/atmel-mci.c
+++ b/drivers/mmc/host/atmel-mci.c
@@ -94,6 +94,7 @@ struct atmel_mci_dma {
  * @need_clock_update: Update the clock rate before the next request.
  * @need_reset: Reset controller before next request.
  * @mode_reg: Value of the MR register.
+ * @cfg_reg: Value of the CFG register.
  * @bus_hz: The rate of @mck in Hz. This forms the basis for MMC bus
  *	rate and timeout calculations.
  * @mapbase: Physical address of the MMIO registers.
@@ -157,6 +158,7 @@ struct atmel_mci {
 	bool			need_clock_update;
 	bool			need_reset;
 	u32			mode_reg;
+	u32			cfg_reg;
 	unsigned long		bus_hz;
 	unsigned long		mapbase;
 	struct clk		*mck;
@@ -225,6 +227,19 @@ static bool mci_has_rwproof(void)
 }
 
 /*
+ * The new MCI2 module isn't 100% compatible with the old MCI module,
+ * and it has a few nice features which we want to use...
+ */
+static inline bool atmci_is_mci2(void)
+{
+	if (cpu_is_at91sam9g45())
+		return true;
+
+	return false;
+}
+
+
+/*
  * The debugfs stuff below is mostly optimized away when
  * CONFIG_DEBUG_FS is not set.
  */
@@ -359,12 +374,33 @@ static int atmci_regs_show(struct seq_file *s, void *v)
 			buf[MCI_BLKR / 4],
 			buf[MCI_BLKR / 4] & 0xffff,
 			(buf[MCI_BLKR / 4] >> 16) & 0xffff);
+	if (atmci_is_mci2())
+		seq_printf(s, "CSTOR:\t0x%08x\n", buf[MCI_CSTOR / 4]);
 
 	/* Don't read RSPR and RDR; it will consume the data there */
 
 	atmci_show_status_reg(s, "SR", buf[MCI_SR / 4]);
 	atmci_show_status_reg(s, "IMR", buf[MCI_IMR / 4]);
 
+	if (atmci_is_mci2()) {
+		u32 val;
+
+		val = buf[MCI_DMA / 4];
+		seq_printf(s, "DMA:\t0x%08x OFFSET=%u CHKSIZE=%u%s\n",
+				val, val & 3,
+				((val >> 4) & 3) ?
+					1 << (((val >> 4) & 3) + 1) : 1,
+				val & MCI_DMAEN ? " DMAEN" : "");
+
+		val = buf[MCI_CFG / 4];
+		seq_printf(s, "CFG:\t0x%08x%s%s%s%s\n",
+				val,
+				val & MCI_CFG_FIFOMODE_1DATA ? " FIFOMODE_ONE_DATA" : "",
+				val & MCI_CFG_FERRCTRL_COR ? " FERRCTRL_CLEAR_ON_READ" : "",
+				val & MCI_CFG_HSMODE ? " HSMODE" : "",
+				val & MCI_CFG_LSYNC ? " LSYNC" : "");
+	}
+
 	kfree(buf);
 
 	return 0;
@@ -559,6 +595,10 @@ static void atmci_dma_complete(void *arg)
 
 	dev_vdbg(&host->pdev->dev, "DMA complete\n");
 
+	if (atmci_is_mci2())
+		/* Disable DMA hardware handshaking on MCI */
+		mci_writel(host, DMA, mci_readl(host, DMA) & ~MCI_DMAEN);
+
 	atmci_dma_cleanup(host);
 
 	/*
@@ -594,7 +634,7 @@ static void atmci_dma_complete(void *arg)
 }
 
 static int
-atmci_submit_data_dma(struct atmel_mci *host, struct mmc_data *data)
+atmci_prepare_data_dma(struct atmel_mci *host, struct mmc_data *data)
 {
 	struct dma_chan			*chan;
 	struct dma_async_tx_descriptor	*desc;
@@ -626,6 +666,9 @@ atmci_submit_data_dma(struct atmel_mci *host, struct mmc_data *data)
 	if (!chan)
 		return -ENODEV;
 
+	if (atmci_is_mci2())
+		mci_writel(host, DMA, MCI_DMA_CHKSIZE(3) | MCI_DMAEN);
+
 	if (data->flags & MMC_DATA_READ)
 		direction = DMA_FROM_DEVICE;
 	else
@@ -643,10 +686,6 @@ atmci_submit_data_dma(struct atmel_mci *host, struct mmc_data *data)
 	host->dma.data_desc = desc;
 	desc->callback = atmci_dma_complete;
 	desc->callback_param = host;
-	desc->tx_submit(desc);
-
-	/* Go! */
-	chan->device->device_issue_pending(chan);
 
 	return 0;
 unmap_exit:
@@ -654,13 +693,26 @@ unmap_exit:
 	return -ENOMEM;
 }
 
+static void atmci_submit_data(struct atmel_mci *host)
+{
+	struct dma_chan			*chan = host->data_chan;
+	struct dma_async_tx_descriptor	*desc = host->dma.data_desc;
+
+	if (chan) {
+		desc->tx_submit(desc);
+		chan->device->device_issue_pending(chan);
+	}
+}
+
 #else /* CONFIG_MMC_ATMELMCI_DMA */
 
-static int atmci_submit_data_dma(struct atmel_mci *host, struct mmc_data *data)
+static int atmci_prepare_data_dma(struct atmel_mci *host, struct mmc_data *data)
 {
 	return -ENOSYS;
 }
 
+static void atmci_submit_data(struct atmel_mci *host) {}
+
 static void atmci_stop_dma(struct atmel_mci *host)
 {
 	/* Data transfer was stopped by the interrupt handler */
@@ -674,7 +726,7 @@ static void atmci_stop_dma(struct atmel_mci *host)
  * Returns a mask of interrupt flags to be enabled after the whole
  * request has been prepared.
  */
-static u32 atmci_submit_data(struct atmel_mci *host, struct mmc_data *data)
+static u32 atmci_prepare_data(struct atmel_mci *host, struct mmc_data *data)
 {
 	u32 iflags;
 
@@ -685,7 +737,7 @@ static u32 atmci_submit_data(struct atmel_mci *host, struct mmc_data *data)
 	host->data = data;
 
 	iflags = ATMCI_DATA_ERROR_FLAGS;
-	if (atmci_submit_data_dma(host, data)) {
+	if (atmci_prepare_data_dma(host, data)) {
 		host->data_chan = NULL;
 
 		/*
@@ -731,6 +783,8 @@ static void atmci_start_request(struct atmel_mci *host,
 		mci_writel(host, CR, MCI_CR_SWRST);
 		mci_writel(host, CR, MCI_CR_MCIEN);
 		mci_writel(host, MR, host->mode_reg);
+		if (atmci_is_mci2())
+			mci_writel(host, CFG, host->cfg_reg);
 		host->need_reset = false;
 	}
 	mci_writel(host, SDCR, slot->sdc_reg);
@@ -746,6 +800,7 @@ static void atmci_start_request(struct atmel_mci *host,
 		while (!(mci_readl(host, SR) & MCI_CMDRDY))
 			cpu_relax();
 	}
+	iflags = 0;
 	data = mrq->data;
 	if (data) {
 		atmci_set_timeout(host, slot, data);
@@ -755,15 +810,17 @@ static void atmci_start_request(struct atmel_mci *host,
 				| MCI_BLKLEN(data->blksz));
 		dev_vdbg(&slot->mmc->class_dev, "BLKR=0x%08x\n",
 			MCI_BCNT(data->blocks) | MCI_BLKLEN(data->blksz));
+
+		iflags |= atmci_prepare_data(host, data);
 	}
 
-	iflags = MCI_CMDRDY;
+	iflags |= MCI_CMDRDY;
 	cmd = mrq->cmd;
 	cmdflags = atmci_prepare_command(slot->mmc, cmd);
 	atmci_start_command(host, cmd, cmdflags);
 
 	if (data)
-		iflags |= atmci_submit_data(host, data);
+		atmci_submit_data(host);
 
 	if (mrq->stop) {
 		host->stop_cmdr = atmci_prepare_command(slot->mmc, mrq->stop);
@@ -859,6 +916,8 @@ static void atmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			clk_enable(host->mck);
 			mci_writel(host, CR, MCI_CR_SWRST);
 			mci_writel(host, CR, MCI_CR_MCIEN);
+			if (atmci_is_mci2())
+				mci_writel(host, CFG, host->cfg_reg);
 		}
 
 		/*
@@ -1097,6 +1156,8 @@ static void atmci_detect_change(unsigned long data)
 				mci_writel(host, CR, MCI_CR_SWRST);
 				mci_writel(host, CR, MCI_CR_MCIEN);
 				mci_writel(host, MR, host->mode_reg);
+				if (atmci_is_mci2())
+					mci_writel(host, CFG, host->cfg_reg);
 
 				host->data = NULL;
 				host->cmd = NULL;
@@ -1620,6 +1681,10 @@ static void atmci_configure_dma(struct atmel_mci *host)
 	}
 	if (!host->dma.chan)
 		dev_notice(&host->pdev->dev, "DMA not available, using PIO\n");
+	else
+		dev_info(&host->pdev->dev,
+					"Using %s for DMA transfers\n",
+					dma_chan_name(host->dma.chan));
 }
 #else
 static void atmci_configure_dma(struct atmel_mci *host) {}
-- 
1.5.6.5

[PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Nicolas Ferre]

This adds the support of atmel-mci sd/mmc driver in at91sam9g45 devices and
board files. This also configures the DMA controller slave interface for
at_hdmac dmaengine driver.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 arch/arm/mach-at91/at91sam9g45_devices.c |  164 ++++++++++++++++++++++++++++++
 arch/arm/mach-at91/board-sam9m10g45ek.c  |   24 +++++
 drivers/mmc/host/Kconfig                 |    2 +-
 3 files changed, 189 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
index d581cff..f341b7e 100644
--- a/arch/arm/mach-at91/at91sam9g45_devices.c
+++ b/arch/arm/mach-at91/at91sam9g45_devices.c
@@ -24,7 +24,10 @@
 #include <mach/at91sam9g45.h>
 #include <mach/at91sam9g45_matrix.h>
 #include <mach/at91sam9_smc.h>
+
 #include <mach/at_hdmac.h>
+#include <mach/atmel-mci.h>
+#include <linux/atmel-mci.h>
 
 #include "generic.h"
 
@@ -294,6 +297,167 @@ void __init at91_add_device_eth(struct at91_eth_data *data) {}
 
 
 /* --------------------------------------------------------------------
+ *  MMC / SD
+ * -------------------------------------------------------------------- */
+
+#if defined(CONFIG_MMC_ATMELMCI) || defined(CONFIG_MMC_ATMELMCI_MODULE)
+static u64 mmc_dmamask = DMA_BIT_MASK(32);
+static struct mci_platform_data mmc0_data, mmc1_data;
+
+static struct resource mmc0_resources[] = {
+	[0] = {
+		.start	= AT91SAM9G45_BASE_MCI0,
+		.end	= AT91SAM9G45_BASE_MCI0 + SZ_16K - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= AT91SAM9G45_ID_MCI0,
+		.end	= AT91SAM9G45_ID_MCI0,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device at91sam9g45_mmc0_device = {
+	.name		= "atmel_mci",
+	.id		= 0,
+	.dev		= {
+				.dma_mask		= &mmc_dmamask,
+				.coherent_dma_mask	= DMA_BIT_MASK(32),
+				.platform_data		= &mmc0_data,
+	},
+	.resource	= mmc0_resources,
+	.num_resources	= ARRAY_SIZE(mmc0_resources),
+};
+
+static struct resource mmc1_resources[] = {
+	[0] = {
+		.start	= AT91SAM9G45_BASE_MCI1,
+		.end	= AT91SAM9G45_BASE_MCI1 + SZ_16K - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= AT91SAM9G45_ID_MCI1,
+		.end	= AT91SAM9G45_ID_MCI1,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device at91sam9g45_mmc1_device = {
+	.name		= "atmel_mci",
+	.id		= 1,
+	.dev		= {
+				.dma_mask		= &mmc_dmamask,
+				.coherent_dma_mask	= DMA_BIT_MASK(32),
+				.platform_data		= &mmc1_data,
+	},
+	.resource	= mmc1_resources,
+	.num_resources	= ARRAY_SIZE(mmc1_resources),
+};
+
+/* Consider only one slot : slot 0 */
+void __init at91_add_device_mci(short mmc_id, struct mci_platform_data *data)
+{
+
+	if (!data)
+		return;
+
+	/* Must have at least one usable slot */
+	if (!data->slot[0].bus_width)
+		return;
+
+#if defined(CONFIG_MMC_ATMELMCI_DMA)
+	{
+	struct mci_dma_data	*slave;
+
+	slave = kzalloc(sizeof(struct mci_dma_data), GFP_KERNEL);
+
+	/* DMA slave channel configuration */
+	slave->sdata.dma_dev = &at_hdmac_device.dev;
+	slave->sdata.reg_width = DW_DMA_SLAVE_WIDTH_32BIT;
+	slave->sdata.cfg = ATC_FIFOCFG_HALFFIFO
+			 | ATC_SRC_H2SEL_HW | ATC_DST_H2SEL_HW;
+	slave->sdata.ctrla = ATC_SCSIZE_16 | ATC_DCSIZE_16;
+	if (mmc_id == 0)	/* MCI0 */
+		slave->sdata.cfg |= ATC_SRC_PER(AT_DMA_ID_MCI0)
+				 | ATC_DST_PER(AT_DMA_ID_MCI0);
+
+	else			/* MCI1 */
+		slave->sdata.cfg |= ATC_SRC_PER(AT_DMA_ID_MCI1)
+				 | ATC_DST_PER(AT_DMA_ID_MCI1);
+
+	data->dma_slave = slave;
+	}
+#endif
+
+
+	/* input/irq */
+	if (data->slot[0].detect_pin) {
+		at91_set_gpio_input(data->slot[0].detect_pin, 1);
+		at91_set_deglitch(data->slot[0].detect_pin, 1);
+	}
+	if (data->slot[0].wp_pin)
+		at91_set_gpio_input(data->slot[0].wp_pin, 1);
+
+	if (mmc_id == 0) {		/* MCI0 */
+
+		/* CLK */
+		at91_set_A_periph(AT91_PIN_PA0, 0);
+
+		/* CMD */
+		at91_set_A_periph(AT91_PIN_PA1, 1);
+
+		/* DAT0, maybe DAT1..DAT3 and maybe DAT4..DAT7 */
+		at91_set_A_periph(AT91_PIN_PA2, 1);
+		if (data->slot[0].bus_width == 4) {
+			at91_set_A_periph(AT91_PIN_PA3, 1);
+			at91_set_A_periph(AT91_PIN_PA4, 1);
+			at91_set_A_periph(AT91_PIN_PA5, 1);
+			if (data->slot[0].bus_width == 8) {
+				at91_set_A_periph(AT91_PIN_PA6, 1);
+				at91_set_A_periph(AT91_PIN_PA7, 1);
+				at91_set_A_periph(AT91_PIN_PA8, 1);
+				at91_set_A_periph(AT91_PIN_PA9, 1);
+			}
+		}
+
+		mmc0_data = *data;
+		at91_clock_associate("mci0_clk", &at91sam9g45_mmc0_device.dev, "mci_clk");
+		platform_device_register(&at91sam9g45_mmc0_device);
+
+	} else {			/* MCI1 */
+
+		/* CLK */
+		at91_set_A_periph(AT91_PIN_PA31, 0);
+
+		/* CMD */
+		at91_set_A_periph(AT91_PIN_PA22, 1);
+
+		/* DAT0, maybe DAT1..DAT3 and maybe DAT4..DAT7 */
+		at91_set_A_periph(AT91_PIN_PA23, 1);
+		if (data->slot[0].bus_width == 4) {
+			at91_set_A_periph(AT91_PIN_PA24, 1);
+			at91_set_A_periph(AT91_PIN_PA25, 1);
+			at91_set_A_periph(AT91_PIN_PA26, 1);
+			if (data->slot[0].bus_width == 8) {
+				at91_set_A_periph(AT91_PIN_PA27, 1);
+				at91_set_A_periph(AT91_PIN_PA28, 1);
+				at91_set_A_periph(AT91_PIN_PA29, 1);
+				at91_set_A_periph(AT91_PIN_PA30, 1);
+			}
+		}
+
+		mmc1_data = *data;
+		at91_clock_associate("mci1_clk", &at91sam9g45_mmc1_device.dev, "mci_clk");
+		platform_device_register(&at91sam9g45_mmc1_device);
+
+	}
+}
+#else
+void __init at91_add_device_mci(short mmc_id, struct mci_platform_data *data) {}
+#endif
+
+
+/* --------------------------------------------------------------------
  *  NAND / SmartMedia
  * -------------------------------------------------------------------- */
 
diff --git a/arch/arm/mach-at91/board-sam9m10g45ek.c b/arch/arm/mach-at91/board-sam9m10g45ek.c
index 64c3843..1cce010 100644
--- a/arch/arm/mach-at91/board-sam9m10g45ek.c
+++ b/arch/arm/mach-at91/board-sam9m10g45ek.c
@@ -24,6 +24,7 @@
 #include <linux/input.h>
 #include <linux/leds.h>
 #include <linux/clk.h>
+#include <linux/atmel-mci.h>
 
 #include <mach/hardware.h>
 #include <video/atmel_lcdc.h>
@@ -99,6 +100,26 @@ static struct spi_board_info ek_spi_devices[] = {
 
 
 /*
+ * MCI (SD/MMC)
+ */
+static struct mci_platform_data __initdata mci0_data = {
+	.slot[0] = {
+		.bus_width	= 4,
+		.detect_pin	= AT91_PIN_PD10,
+		.wp_pin		= -1,
+	},
+};
+
+static struct mci_platform_data __initdata mci1_data = {
+	.slot[0] = {
+		.bus_width	= 4,
+		.detect_pin	= AT91_PIN_PD11,
+		.wp_pin		= AT91_PIN_PD29,
+	},
+};
+
+
+/*
  * MACB Ethernet device
  */
 static struct at91_eth_data __initdata ek_macb_data = {
@@ -370,6 +391,9 @@ static void __init ek_board_init(void)
 	at91_add_device_usba(&ek_usba_udc_data);
 	/* SPI */
 	at91_add_device_spi(ek_spi_devices, ARRAY_SIZE(ek_spi_devices));
+	/* MMC */
+	at91_add_device_mci(0, &mci0_data);
+	at91_add_device_mci(1, &mci1_data);
 	/* Ethernet */
 	at91_add_device_eth(&ek_macb_data);
 	/* NAND */
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 432ae83..b4aeb9d 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -188,7 +188,7 @@ endchoice
 
 config MMC_ATMELMCI_DMA
 	bool "Atmel MCI DMA support (EXPERIMENTAL)"
-	depends on MMC_ATMELMCI && AVR32 && DMA_ENGINE && EXPERIMENTAL
+	depends on MMC_ATMELMCI && (AVR32 || ARCH_AT91SAM9G45) && DMA_ENGINE && EXPERIMENTAL
 	help
 	  Say Y here to have the Atmel MCI driver use a DMA engine to
 	  do data transfers and thus increase the throughput and
-- 
1.5.6.5

Re: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Yegor Yefremov]

Nicolas Ferre wrote:
> This adds the support of atmel-mci sd/mmc driver in at91sam9g45 devices and
> board files. This also configures the DMA controller slave interface for
> at_hdmac dmaengine driver.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
>  arch/arm/mach-at91/at91sam9g45_devices.c |  164 ++++++++++++++++++++++++++++++
>  arch/arm/mach-at91/board-sam9m10g45ek.c  |   24 +++++
>  drivers/mmc/host/Kconfig                 |    2 +-
>  3 files changed, 189 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
> index d581cff..f341b7e 100644
> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
> @@ -24,7 +24,10 @@
>  #include <mach/at91sam9g45.h>
>  #include <mach/at91sam9g45_matrix.h>
>  #include <mach/at91sam9_smc.h>
> +
>  #include <mach/at_hdmac.h>
> +#include <mach/atmel-mci.h>
> +#include <linux/atmel-mci.h>
>  
>  #include "generic.h"
>  
> @@ -294,6 +297,167 @@ void __init at91_add_device_eth(struct at91_eth_data *data) {}
>  
>  
>  /* --------------------------------------------------------------------
> + *  MMC / SD
> + * -------------------------------------------------------------------- */
> +
> +#if defined(CONFIG_MMC_ATMELMCI) || defined(CONFIG_MMC_ATMELMCI_MODULE)
> +static u64 mmc_dmamask = DMA_BIT_MASK(32);
> +static struct mci_platform_data mmc0_data, mmc1_data;
> +
> +static struct resource mmc0_resources[] = {
> +	[0] = {
> +		.start	= AT91SAM9G45_BASE_MCI0,
> +		.end	= AT91SAM9G45_BASE_MCI0 + SZ_16K - 1,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +	[1] = {
> +		.start	= AT91SAM9G45_ID_MCI0,
> +		.end	= AT91SAM9G45_ID_MCI0,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct platform_device at91sam9g45_mmc0_device = {
> +	.name		= "atmel_mci",
> +	.id		= 0,
> +	.dev		= {
> +				.dma_mask		= &mmc_dmamask,
> +				.coherent_dma_mask	= DMA_BIT_MASK(32),
> +				.platform_data		= &mmc0_data,
> +	},
> +	.resource	= mmc0_resources,
> +	.num_resources	= ARRAY_SIZE(mmc0_resources),
> +};
> +
> +static struct resource mmc1_resources[] = {
> +	[0] = {
> +		.start	= AT91SAM9G45_BASE_MCI1,
> +		.end	= AT91SAM9G45_BASE_MCI1 + SZ_16K - 1,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +	[1] = {
> +		.start	= AT91SAM9G45_ID_MCI1,
> +		.end	= AT91SAM9G45_ID_MCI1,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct platform_device at91sam9g45_mmc1_device = {
> +	.name		= "atmel_mci",
> +	.id		= 1,
> +	.dev		= {
> +				.dma_mask		= &mmc_dmamask,
> +				.coherent_dma_mask	= DMA_BIT_MASK(32),
> +				.platform_data		= &mmc1_data,
> +	},
> +	.resource	= mmc1_resources,
> +	.num_resources	= ARRAY_SIZE(mmc1_resources),
> +};
> +
> +/* Consider only one slot : slot 0 */
> +void __init at91_add_device_mci(short mmc_id, struct mci_platform_data *data)
> +{
> +
> +	if (!data)
> +		return;
> +
> +	/* Must have at least one usable slot */
> +	if (!data->slot[0].bus_width)
> +		return;
> +
> +#if defined(CONFIG_MMC_ATMELMCI_DMA)
> +	{
> +	struct mci_dma_data	*slave;
> +
> +	slave = kzalloc(sizeof(struct mci_dma_data), GFP_KERNEL);
> +
> +	/* DMA slave channel configuration */
> +	slave->sdata.dma_dev = &at_hdmac_device.dev;
> +	slave->sdata.reg_width = DW_DMA_SLAVE_WIDTH_32BIT;

slave->sdata.reg_width = AT_DMA_SLAVE_WIDTH_32BIT;

> +	slave->sdata.cfg = ATC_FIFOCFG_HALFFIFO
> +			 | ATC_SRC_H2SEL_HW | ATC_DST_H2SEL_HW;
> +	slave->sdata.ctrla = ATC_SCSIZE_16 | ATC_DCSIZE_16;
> +	if (mmc_id == 0)	/* MCI0 */
> +		slave->sdata.cfg |= ATC_SRC_PER(AT_DMA_ID_MCI0)
> +				 | ATC_DST_PER(AT_DMA_ID_MCI0);
> +
> +	else			/* MCI1 */
> +		slave->sdata.cfg |= ATC_SRC_PER(AT_DMA_ID_MCI1)
> +				 | ATC_DST_PER(AT_DMA_ID_MCI1);
> +
> +	data->dma_slave = slave;
> +	}
> +#endif
> +
> +
> +	/* input/irq */
> +	if (data->slot[0].detect_pin) {
> +		at91_set_gpio_input(data->slot[0].detect_pin, 1);
> +		at91_set_deglitch(data->slot[0].detect_pin, 1);
> +	}
> +	if (data->slot[0].wp_pin)
> +		at91_set_gpio_input(data->slot[0].wp_pin, 1);
> +
> +	if (mmc_id == 0) {		/* MCI0 */
> +
> +		/* CLK */
> +		at91_set_A_periph(AT91_PIN_PA0, 0);
> +
> +		/* CMD */
> +		at91_set_A_periph(AT91_PIN_PA1, 1);
> +
> +		/* DAT0, maybe DAT1..DAT3 and maybe DAT4..DAT7 */
> +		at91_set_A_periph(AT91_PIN_PA2, 1);
> +		if (data->slot[0].bus_width == 4) {
> +			at91_set_A_periph(AT91_PIN_PA3, 1);
> +			at91_set_A_periph(AT91_PIN_PA4, 1);
> +			at91_set_A_periph(AT91_PIN_PA5, 1);
> +			if (data->slot[0].bus_width == 8) {
> +				at91_set_A_periph(AT91_PIN_PA6, 1);
> +				at91_set_A_periph(AT91_PIN_PA7, 1);
> +				at91_set_A_periph(AT91_PIN_PA8, 1);
> +				at91_set_A_periph(AT91_PIN_PA9, 1);
> +			}
> +		}
> +
> +		mmc0_data = *data;
> +		at91_clock_associate("mci0_clk", &at91sam9g45_mmc0_device.dev, "mci_clk");
> +		platform_device_register(&at91sam9g45_mmc0_device);
> +
> +	} else {			/* MCI1 */
> +
> +		/* CLK */
> +		at91_set_A_periph(AT91_PIN_PA31, 0);
> +
> +		/* CMD */
> +		at91_set_A_periph(AT91_PIN_PA22, 1);
> +
> +		/* DAT0, maybe DAT1..DAT3 and maybe DAT4..DAT7 */
> +		at91_set_A_periph(AT91_PIN_PA23, 1);
> +		if (data->slot[0].bus_width == 4) {
> +			at91_set_A_periph(AT91_PIN_PA24, 1);
> +			at91_set_A_periph(AT91_PIN_PA25, 1);
> +			at91_set_A_periph(AT91_PIN_PA26, 1);
> +			if (data->slot[0].bus_width == 8) {
> +				at91_set_A_periph(AT91_PIN_PA27, 1);
> +				at91_set_A_periph(AT91_PIN_PA28, 1);
> +				at91_set_A_periph(AT91_PIN_PA29, 1);
> +				at91_set_A_periph(AT91_PIN_PA30, 1);
> +			}
> +		}
> +
> +		mmc1_data = *data;
> +		at91_clock_associate("mci1_clk", &at91sam9g45_mmc1_device.dev, "mci_clk");
> +		platform_device_register(&at91sam9g45_mmc1_device);
> +
> +	}
> +}
> +#else
> +void __init at91_add_device_mci(short mmc_id, struct mci_platform_data *data) {}
> +#endif
> +
> +
> +/* --------------------------------------------------------------------
>   *  NAND / SmartMedia
>   * -------------------------------------------------------------------- */
>  
> diff --git a/arch/arm/mach-at91/board-sam9m10g45ek.c b/arch/arm/mach-at91/board-sam9m10g45ek.c
> index 64c3843..1cce010 100644
> --- a/arch/arm/mach-at91/board-sam9m10g45ek.c
> +++ b/arch/arm/mach-at91/board-sam9m10g45ek.c
> @@ -24,6 +24,7 @@
>  #include <linux/input.h>
>  #include <linux/leds.h>
>  #include <linux/clk.h>
> +#include <linux/atmel-mci.h>
>  
>  #include <mach/hardware.h>
>  #include <video/atmel_lcdc.h>
> @@ -99,6 +100,26 @@ static struct spi_board_info ek_spi_devices[] = {
>  
>  
>  /*
> + * MCI (SD/MMC)
> + */
> +static struct mci_platform_data __initdata mci0_data = {
> +	.slot[0] = {
> +		.bus_width	= 4,
> +		.detect_pin	= AT91_PIN_PD10,
> +		.wp_pin		= -1,
> +	},
> +};
> +
> +static struct mci_platform_data __initdata mci1_data = {
> +	.slot[0] = {
> +		.bus_width	= 4,
> +		.detect_pin	= AT91_PIN_PD11,
> +		.wp_pin		= AT91_PIN_PD29,
> +	},
> +};
> +
> +
> +/*
>   * MACB Ethernet device
>   */
>  static struct at91_eth_data __initdata ek_macb_data = {
> @@ -370,6 +391,9 @@ static void __init ek_board_init(void)
>  	at91_add_device_usba(&ek_usba_udc_data);
>  	/* SPI */
>  	at91_add_device_spi(ek_spi_devices, ARRAY_SIZE(ek_spi_devices));
> +	/* MMC */
> +	at91_add_device_mci(0, &mci0_data);
> +	at91_add_device_mci(1, &mci1_data);
>  	/* Ethernet */
>  	at91_add_device_eth(&ek_macb_data);
>  	/* NAND */
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 432ae83..b4aeb9d 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -188,7 +188,7 @@ endchoice
>  
>  config MMC_ATMELMCI_DMA
>  	bool "Atmel MCI DMA support (EXPERIMENTAL)"
> -	depends on MMC_ATMELMCI && AVR32 && DMA_ENGINE && EXPERIMENTAL
> +	depends on MMC_ATMELMCI && (AVR32 || ARCH_AT91SAM9G45) && DMA_ENGINE && EXPERIMENTAL
>  	help
>  	  Say Y here to have the Atmel MCI driver use a DMA engine to
>  	  do data transfers and thus increase the throughput and

Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>

Re: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Andrew Victor]

hi Nicolas,

> +       if (data->slot[0].wp_pin)
> +               at91_set_gpio_input(data->slot[0].wp_pin, 1);

> +static struct mci_platform_data __initdata mci0_data = {
> +       .slot[0] = {
> +               .bus_width      = 4,
> +               .detect_pin     = AT91_PIN_PD10,
> +               .wp_pin         = -1,
> +       },

Causes at91_set_gpio_input() to be called for pin -1.  Which shouldn't be valid.
AT91 platforms use 0 to indicate an un-connected GPIO pin, so the
assignment of "wp_pin" should probably just be removed.


Regards,
  Andrew Victor

Re: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Haavard Skinnemoen]

Andrew Victor <avictor.za@gmail.com> wrote:
> > +static struct mci_platform_data __initdata mci0_data = {
> > +       .slot[0] = {
> > +               .bus_width      = 4,
> > +               .detect_pin     = AT91_PIN_PD10,
> > +               .wp_pin         = -1,
> > +       },  
> 
> Causes at91_set_gpio_input() to be called for pin -1.  Which shouldn't be valid.
> AT91 platforms use 0 to indicate an un-connected GPIO pin, so the
> assignment of "wp_pin" should probably just be removed.

The mci driver expects non-existent pins to have a negative value, as
do all other drivers which use gpio_is_valid().

Haavard

Re: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Thiago A. Corrêa]

On Tue, Oct 27, 2009 at 10:35 PM, Haavard Skinnemoen
<haavard.skinnemoen@atmel.com> wrote:
> Andrew Victor <avictor.za@gmail.com> wrote:
>> > +static struct mci_platform_data __initdata mci0_data = {
>> > +       .slot[0] = {
>> > +               .bus_width      = 4,
>> > +               .detect_pin     = AT91_PIN_PD10,
>> > +               .wp_pin         = -1,
>> > +       },
>>
>> Causes at91_set_gpio_input() to be called for pin -1.  Which shouldn't be valid.
>> AT91 platforms use 0 to indicate an un-connected GPIO pin, so the
>> assignment of "wp_pin" should probably just be removed.
>
> The mci driver expects non-existent pins to have a negative value, as
> do all other drivers which use gpio_is_valid().
>

Then I think it would be best to use GPIO_PIN_NONE. Makes it clear
what is expected and avoids confusion on what should be the proper
value.
I hope I'm not saying non-sense, but even if I am, I guess you can see
that I'm advocating against the magic numbers :)


Kind Regards,
   Thiago A. Correa

Re: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Haavard Skinnemoen]

Thiago A. Corrêa <thiago.correa@gmail.com> wrote:
> >> Causes at91_set_gpio_input() to be called for pin -1.  Which shouldn't be valid.
> >> AT91 platforms use 0 to indicate an un-connected GPIO pin, so the
> >> assignment of "wp_pin" should probably just be removed.  
> >
> > The mci driver expects non-existent pins to have a negative value, as
> > do all other drivers which use gpio_is_valid().
> >  
> 
> Then I think it would be best to use GPIO_PIN_NONE. Makes it clear
> what is expected and avoids confusion on what should be the proper
> value.

Unfortunately, GPIO_PIN_NONE only exists on AVR32.

> I hope I'm not saying non-sense, but even if I am, I guess you can see
> that I'm advocating against the magic numbers :)

IIRC, the correct way to specify a non-existent pin is to use -ENODEV.

Haavard

Re: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Andrew Victor]

hi,

> Then I think it would be best to use GPIO_PIN_NONE. Makes it clear
> what is expected and avoids confusion on what should be the proper
> value.
> I hope I'm not saying non-sense, but even if I am, I guess you can see
> that I'm advocating against the magic numbers :)

What magic numbers ?

If you have a "wp_pin" on the board, you declare the struct as:
    static struct mci_platform_data __initdata mci0_data = {
          .slot[0] = {
                 .bus_width      = 4,
                 .detect_pin     = AT91_PIN_PD10,
        }
    }

and if you do have a "wp_pin" on your board, you declare the struct as:
    static struct mci_platform_data __initdata mci0_data = {
          .slot[0] = {
                 .bus_width      = 4,
                 .detect_pin     = AT91_PIN_PD10,
                 .wp_pin          = AT91_PIN_PD11,
        }
    }

And it's more future-proof.  If the next version of the
driver/peripheral has a "toggle_pin" GPIO option, you don't need to go
update all board files with a ".toggle_pin  = GPIO_PIN_NONE"   or
".toggle_pin = -ENODEV".


Regards,
  Andrew Victor

Re: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Ben Nizette]

On Wed, 2009-10-28 at 21:53 +0200, Andrew Victor wrote:
> hi,
> 
> > Then I think it would be best to use GPIO_PIN_NONE. Makes it clear
> > what is expected and avoids confusion on what should be the proper
> > value.
> > I hope I'm not saying non-sense, but even if I am, I guess you can see
> > that I'm advocating against the magic numbers :)
> 
> What magic numbers ?

I think Thiago was referring to the "-1" in the original patch as the
magic number.

Leaving the field blank to be initialised to 0 is certainly the
cleanest, I agree, but it doesn't actually /work/.  On many archs 0 is a
valid gpio number; the gpio_is_valid check used throughout the kernel
(including atmel-mci.c) looks like

static inline int gpio_is_valid(int number)
{
	/* only some non-negative numbers are valid */
	return ((unsigned)number) < ARCH_NR_GPIOS;
}

	--Ben.

Re: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Nicolas Ferre]

Ben Nizette :
> On Wed, 2009-10-28 at 21:53 +0200, Andrew Victor wrote:
>> hi,
>>
>>> Then I think it would be best to use GPIO_PIN_NONE. Makes it clear
>>> what is expected and avoids confusion on what should be the proper
>>> value.
>>> I hope I'm not saying non-sense, but even if I am, I guess you can see
>>> that I'm advocating against the magic numbers :)
>> What magic numbers ?
> 
> I think Thiago was referring to the "-1" in the original patch as the
> magic number.
> 
> Leaving the field blank to be initialised to 0 is certainly the
> cleanest, I agree, but it doesn't actually /work/.  On many archs 0 is a
> valid gpio number; the gpio_is_valid check used throughout the kernel
> (including atmel-mci.c) looks like
> 
> static inline int gpio_is_valid(int number)
> {
> 	/* only some non-negative numbers are valid */
> 	return ((unsigned)number) < ARCH_NR_GPIOS;
> }

I understand that the better way to solve this issue is to:
- keep the AT91 way of specifying not connected pins (= 0)
- code the gpio_is_valid() function for at91 that tests this way of
handling not connected gpio

I see that in arch/arm/mach-at91/include/mach/gpio.h
we include the asm-generic/gpio.h file... must I implement the full set
of gpiolib ?

Best regards,
-- 
Nicolas Ferre

Re: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Nicolas Ferre]

Yegor Yefremov :
> Nicolas Ferre wrote:
>> This adds the support of atmel-mci sd/mmc driver in at91sam9g45 devices and
>> board files. This also configures the DMA controller slave interface for
>> at_hdmac dmaengine driver.
>>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>

[..]

>> +	/* DMA slave channel configuration */
>> +	slave->sdata.dma_dev = &at_hdmac_device.dev;
>> +	slave->sdata.reg_width = DW_DMA_SLAVE_WIDTH_32BIT;
> 
> slave->sdata.reg_width = AT_DMA_SLAVE_WIDTH_32BIT;
> 
>> +	slave->sdata.cfg = ATC_FIFOCFG_HALFFIFO
>> +			 | ATC_SRC_H2SEL_HW | ATC_DST_H2SEL_HW;

Queued for v3 of this patch (I will dissociate if from the patch series).


> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>

Ok, I will add.

Thanks a lot.

Best regards,
-- 
Nicolas Ferre

Re: [PATCH 2/3 v2] mmc: atmel-mci: New MCI2 module support in atmel-mci driver

[Nicolas Ferre]

Hi,

What do you think about the rework of this atmel-mci DMA interface ?
patches:
[PATCH 1/3 v2] atmel-mci: change use of dma slave interface
[PATCH 2/3 v2] mmc: atmel-mci: New MCI2 module support in atmel-mci driver

We may consider them in replacement of the previous ones and dissociate 
this series from the third one which will be reworked and may goes through 
using another path.
(for the record: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc 
driver in at91sam9g45 chip and board).

Thanks,

Bye,
-- 
Nicolas Ferre

Re: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Ben Nizette]

On Mon, 2009-11-02 at 18:11 +0100, Nicolas Ferre wrote:
> Ben Nizette :
> > 
> > static inline int gpio_is_valid(int number)
> > {
> > 	/* only some non-negative numbers are valid */
> > 	return ((unsigned)number) < ARCH_NR_GPIOS;
> > }
> 
> I understand that the better way to solve this issue is to:
> - keep the AT91 way of specifying not connected pins (= 0)
> - code the gpio_is_valid() function for at91 that tests this way of
> handling not connected gpio
> 


> I see that in arch/arm/mach-at91/include/mach/gpio.h
> we include the asm-generic/gpio.h file... must I implement the full set
> of gpiolib ?



> 
> Best regards,

Re: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Ben Nizette]

[apologies for the MTA fart causing that weird, rouge blank email :-) ]

On Mon, 2009-11-02 at 18:11 +0100, Nicolas Ferre wrote:
> Ben Nizette :
> > 
> > static inline int gpio_is_valid(int number)
> > {
> > 	/* only some non-negative numbers are valid */
> > 	return ((unsigned)number) < ARCH_NR_GPIOS;
> > }
> 
> I understand that the better way to solve this issue is to:
> - keep the AT91 way of specifying not connected pins (= 0)
> - code the gpio_is_valid() function for at91 that tests this way of
> handling not connected gpio
> 

I'm not sure I'd break cross-arch compat here, but whatever.  I guess it
depends how deeply the =0 stuff is ingrained in the at91 codebase

> I see that in arch/arm/mach-at91/include/mach/gpio.h
> we include the asm-generic/gpio.h file... must I implement the full set
> of gpiolib ?

I'd suggest creating an ARCH_HAVE_GPIO_VALID (darn, long name) or
something; define it before #include <asm-generic/gpio.h> and #ifdef
protect the offending declaration in that header.

	--Ben.

> 
> Best regards,

Re: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Ryan Mallon]

Nicolas Ferre wrote:
> Ben Nizette :
>   
>> On Wed, 2009-10-28 at 21:53 +0200, Andrew Victor wrote:
>>     
>>> hi,
>>>
>>>       
>>>> Then I think it would be best to use GPIO_PIN_NONE. Makes it clear
>>>> what is expected and avoids confusion on what should be the proper
>>>> value.
>>>> I hope I'm not saying non-sense, but even if I am, I guess you can see
>>>> that I'm advocating against the magic numbers :)
>>>>         
>>> What magic numbers ?
>>>       
>> I think Thiago was referring to the "-1" in the original patch as the
>> magic number.
>>
>> Leaving the field blank to be initialised to 0 is certainly the
>> cleanest, I agree, but it doesn't actually /work/.  On many archs 0 is a
>> valid gpio number; the gpio_is_valid check used throughout the kernel
>> (including atmel-mci.c) looks like
>>
>> static inline int gpio_is_valid(int number)
>> {
>> 	/* only some non-negative numbers are valid */
>> 	return ((unsigned)number) < ARCH_NR_GPIOS;
>> }
>>     
>
> I understand that the better way to solve this issue is to:
> - keep the AT91 way of specifying not connected pins (= 0)
> - code the gpio_is_valid() function for at91 that tests this way of
> handling not connected gpio
>   
It doesn't appear that the gpio_is_valid function can be overridden by a
platform specific version. However, as you point out, on AT91 it appears
broken since anything less than AT91_PIN_PA0 (32) is not a valid gpio.

IIRC, we can't mark static inline functions as weak, and we don't want
to turn gpio_is_valid into an actual function call. We could do some
preprocessor magic, but that gets a bit messy.

CC'ed David Brownell, who does most of the gpiolib stuff. Any ideas?

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751 
Fax:   +64 3 3779135			  USA 1800 261 2934

Re: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Ben Nizette]

On Tue, 2009-11-03 at 15:30 +1300, Ryan Mallon wrote:
> > 
> IIRC, we can't mark static inline functions as weak, and we don't want
> to turn gpio_is_valid into an actual function call. We could do some
> preprocessor magic, but that gets a bit messy.

Messy but generally accepted.

Have a grep for, eg, __HAVE_ARCH_<string functions> and
HAVE_ARCH_BUG{_ON}

	--Ben.

Re: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc driver in at91sam9g45 chip and board

[Haavard Skinnemoen]

Ben Nizette <bn@niasdigital.com> wrote:
> On Tue, 2009-11-03 at 15:30 +1300, Ryan Mallon wrote:
> > >   
> > IIRC, we can't mark static inline functions as weak, and we don't want
> > to turn gpio_is_valid into an actual function call. We could do some
> > preprocessor magic, but that gets a bit messy.  
> 
> Messy but generally accepted.

IIRC the most generally accepted way is to do

static inline bool gpio_is_valid(int pin)
{
	/* whatever */
}
#define gpio_is_valid gpio_is_valid

in the platform code and

#ifndef gpio_is_valid
/* provide definition of gpio_is_valid */
#endif

in the generic code. This way, there's only one symbol to grep for.

Haavard

Re: [PATCH 2/3 v2] mmc: atmel-mci: New MCI2 module support in atmel-mci driver

[Nicolas Ferre]

Nicolas Ferre :
> Hi,
> 
> What do you think about the rework of this atmel-mci DMA interface ?
> patches:
> [PATCH 1/3 v2] atmel-mci: change use of dma slave interface
> [PATCH 2/3 v2] mmc: atmel-mci: New MCI2 module support in atmel-mci driver
> 
> We may consider them in replacement of the previous ones and dissociate 
> this series from the third one which will be reworked and may goes through 
> using another path.
> (for the record: [PATCH 3/3 v2] at91/atmel-mci: inclusion of sd/mmc 
> driver in at91sam9g45 chip and board).

Andrew, Haavard,

Ping ?

-- 
Nicolas Ferre