[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

[PATCH] pio: add arch specific gpio_is_valid() function

[Nicolas Ferre]

Add a simple gpio_is_valid() function to replace
the standard one. It introduces the __HAVE_ARCH_GPIO_IS_VALID
macro to overload the generic code.
As an implementation example, it takes into account the AT91
pio numbering to check if a proper value is used.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
Hi all,

I come back on this thread as I would like to implement the gpio_is_valid()
function for AT91. I have based this piece of code on comments from Ben and
Haavard and chose the  __HAVE_ARCH_* macro definition as it seems wide spread
in kernel code.
Please make comments and if it is ok for you, eventually accept for merging...


 arch/arm/mach-at91/include/mach/gpio.h |    9 +++++++++
 include/asm-generic/gpio.h             |    4 ++++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-at91/include/mach/gpio.h b/arch/arm/mach-at91/include/mach/gpio.h
index 5cce2ed..103486d 100644
--- a/arch/arm/mach-at91/include/mach/gpio.h
+++ b/arch/arm/mach-at91/include/mach/gpio.h
@@ -188,6 +188,15 @@
 #define	AT91_PIN_PE31	(PIN_BASE + 0x80 + 31)
 
 #ifndef __ASSEMBLY__
+static inline int gpio_is_valid(int number)
+{
+	if (number >= PIN_BASE)
+		return 1;
+	return 0;
+}
+#define __HAVE_ARCH_GPIO_IS_VALID 1
+
+
 /* setup setup routines, called from board init or driver probe() */
 extern int __init_or_module at91_set_GPIO_periph(unsigned pin, int use_pullup);
 extern int __init_or_module at91_set_A_periph(unsigned pin, int use_pullup);
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 4f3d75e..2840f35 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -22,11 +22,13 @@
 #define ARCH_NR_GPIOS		256
 #endif
 
+#ifndef __HAVE_ARCH_GPIO_IS_VALID
 static inline int gpio_is_valid(int number)
 {
 	/* only some non-negative numbers are valid */
 	return ((unsigned)number) < ARCH_NR_GPIOS;
 }
+#endif
 
 struct device;
 struct seq_file;
@@ -185,11 +187,13 @@ extern void gpio_unexport(unsigned gpio);
 
 #else	/* !CONFIG_HAVE_GPIO_LIB */
 
+#ifndef __HAVE_ARCH_GPIO_IS_VALID
 static inline int gpio_is_valid(int number)
 {
 	/* only non-negative numbers are valid */
 	return number >= 0;
 }
+#endif
 
 /* platforms that don't directly support access to GPIOs through I2C, SPI,
  * or other blocking infrastructure can use these wrappers.
-- 
1.5.6.5

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[David Brownell]

--- On Mon, 8/23/10, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:

> From: Nicolas Ferre <nicolas.ferre@atmel.com>
> Subject: [PATCH] pio: add arch specific gpio_is_valid() function

What's the rationale?  It's valid or not.  And there's already a
function whose job it is to report that status ... which is set up
for arch customization.  Which ISTR worked fine for AT91 (among other
platforms) ...

So my first reacion is "NAK, pointless".

> 

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[Nicolas Ferre]

Le 23/08/2010 18:36, David Brownell :
> 
> 
> --- On Mon, 8/23/10, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> 
>> From: Nicolas Ferre <nicolas.ferre@atmel.com>
>> Subject: [PATCH] pio: add arch specific gpio_is_valid() function
> 
> What's the rationale?  It's valid or not.  And there's already a
> function whose job it is to report that status ... which is set up
> for arch customization.

How do you customize it? I would like to keep the benefit of gpiolib
implementation.
In arch/arm/mach-at91/include/mach/gpio.h we include the
asm-generic/gpio.h. That is why I protect the generic code with
__HAVE_ARCH_GPIO_IS_VALID.

With this, we can keep the benefit of having an inline function.

>  Which ISTR worked fine for AT91 (among other
> platforms) ...

Well the standard function:
return ((unsigned)number) < ARCH_NR_GPIOS;
is not suitable for AT91 as said in the thread.

Best regards,
-- 
Nicolas Ferre

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[Jean-Christophe PLAGNIOL-VILLARD]

On 17:01 Mon 23 Aug     , Nicolas Ferre wrote:
> Add a simple gpio_is_valid() function to replace
> the standard one. It introduces the __HAVE_ARCH_GPIO_IS_VALID
> macro to overload the generic code.
> As an implementation example, it takes into account the AT91
> pio numbering to check if a proper value is used.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> Hi all,
> 
> I come back on this thread as I would like to implement the gpio_is_valid()
> function for AT91. I have based this piece of code on comments from Ben and
> Haavard and chose the  __HAVE_ARCH_* macro definition as it seems wide spread
> in kernel code.
> Please make comments and if it is ok for you, eventually accept for merging...
> 
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

specially when the drivers are share between different arch such as here avr32
and arm
but also for renesas between sh and arm and ST between SH and ST200

the representation of an invalid gpio could differ and invalade it dito

Best Regards,
J.

[PATCH v2] AT91: pio: add gpio_is_valid() function

[Nicolas Ferre]

Add a simple gpio_is_valid() function to overload
the standard one. It takes into account the AT91
pio numbering to check if a proper value is used.
The upper limit keeps room for IO expanders above
regular on-chip GPIO numbers.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
Changes since v1:
	define ARCH_NR_GPIOS for AT91 keeping room for IO expanders
	now use an upper limit to conform with gpiolib

 arch/arm/mach-at91/include/mach/gpio.h |   14 ++++++++++++++
 include/asm-generic/gpio.h             |    4 ++++
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-at91/include/mach/gpio.h b/arch/arm/mach-at91/include/mach/gpio.h
index bfdd8ab..55828ad 100644
--- a/arch/arm/mach-at91/include/mach/gpio.h
+++ b/arch/arm/mach-at91/include/mach/gpio.h
@@ -21,6 +21,10 @@
 #define MAX_GPIO_BANKS		5
 #define NR_BUILTIN_GPIO		(PIN_BASE + (MAX_GPIO_BANKS * 32))
 
+/* keep room for a couple of GPIO expanders */
+#define NR_EXTRA_GPIO		64
+#define ARCH_NR_GPIOS		(NR_BUILTIN_GPIO + NR_EXTRA_GPIO)
+
 /* these pin numbers double as IRQ numbers, like AT91xxx_ID_* values */
 
 #define	AT91_PIN_PA0	(PIN_BASE + 0x00 + 0)
@@ -189,6 +193,16 @@
 #define	AT91_PIN_PE31	(PIN_BASE + 0x80 + 31)
 
 #ifndef __ASSEMBLY__
+static inline int gpio_is_valid(int number)
+{
+	if (number >= PIN_BASE &&
+	    number <= ARCH_NR_GPIOS)
+		return 1;
+	return 0;
+}
+#define __HAVE_ARCH_GPIO_IS_VALID 1
+
+
 /* setup setup routines, called from board init or driver probe() */
 extern int __init_or_module at91_set_GPIO_periph(unsigned pin, int use_pullup);
 extern int __init_or_module at91_set_A_periph(unsigned pin, int use_pullup);
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index c7376bf..264e701 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -22,11 +22,13 @@
 #define ARCH_NR_GPIOS		256
 #endif
 
+#ifndef __HAVE_ARCH_GPIO_IS_VALID
 static inline int gpio_is_valid(int number)
 {
 	/* only some non-negative numbers are valid */
 	return ((unsigned)number) < ARCH_NR_GPIOS;
 }
+#endif
 
 struct device;
 struct seq_file;
@@ -200,11 +202,13 @@ extern void gpio_unexport(unsigned gpio);
 
 #else	/* !CONFIG_HAVE_GPIO_LIB */
 
+#ifndef __HAVE_ARCH_GPIO_IS_VALID
 static inline int gpio_is_valid(int number)
 {
 	/* only non-negative numbers are valid */
 	return number >= 0;
 }
+#endif
 
 /* platforms that don't directly support access to GPIOs through I2C, SPI,
  * or other blocking infrastructure can use these wrappers.
-- 
1.5.6.5

Re: [PATCH v2] AT91: pio: add gpio_is_valid() function

[David Brownell]

--- On Mon, 9/6/10, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:

> From: Nicolas Ferre <nicolas.ferre@atmel.com>
> Subject: [PATCH v2] AT91: pio: add gpio_is_valid() function


Of course there already *IS* a gpio_is_valid(),
with arch/platform hooks


> --- a/arch/arm/mach-at91/include/mach/gpio.h
> +++ b/arch/arm/mach-at91/include/mach/gpio.h

>  
> +/* keep room for a couple of GPIO expanders */
> +#define NR_EXTRA_GPIO       64
> +#define ARCH_NR_GPIOS       
> (NR_BUILTIN_GPIO + NR_EXTRA_GPIO)

ISTR contemplating something like NR_EXTRA_GPIO
once too, but deciding against it.  Doing it this
way (per-platform) seems OK.  ISTR, matches OMAP;
might be worth generalizing...)


>  #ifndef __ASSEMBLY__
> +static inline int gpio_is_valid(int number)
> +{
> +    if (number >= PIN_BASE &&

I suppose that clause is the entire reason to
not like the standard gpio_is_valid() ??  Since
on AT91 the IRQ and GPIO numbers share the same
space, but 0..(PIN_BASE-1) are IRQs not GPIOs.
  Yes?

Worth re-thinking your approach to handling that.
Most of the numbers in that range are valid GPIO
numbers -- on non-AT91 platforms.  Maybe AT91
scould grow to_gpio(N) and to_irq(N) macros.  It
was handy sharing the spaces when implementing
GPIO IRQ support, but in retrospect maybe that was
not the best idea.


> +        number <=
> ARCH_NR_GPIOS)


More conventional, FWIW -- just return the
boolean xpression's value ...
> +        return 1;
> +    return 0;

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[David Brownell]

Still not liking or accepting this proposed
change to the GPIO framework.

For the AT91 case (where integers 0..N are
IRQs, but N..max are GPIOs)

A simpler solution is just to use a bit in
the integer to indicate IRQ vs GPIO.  Like
maybe the sign bit.. which is never set on
valid GPIO numbers, but platforms could let
be set on IRQs.

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[Ryan Mallon]

On 09/07/2010 02:23 PM, David Brownell wrote:
> Still not liking or accepting this proposed
> change to the GPIO framework.
>
> For the AT91 case (where integers 0..N are
> IRQs, but N..max are GPIOs)
>
> A simpler solution is just to use a bit in
> the integer to indicate IRQ vs GPIO.  Like
> maybe the sign bit.. which is never set on
> valid GPIO numbers, but platforms could let
> be set on IRQs.
>   
How about this approach instead?

----
On some architectures gpio numbering does not start from zero. Allow for
correct behaviour of gpio_is_valid on values below the first gpio by
adding the architecture overrideable ARCH_FIRST_GPIO.

Signed-off-by: Ryan Mallon <ryan@bluewatersys.com>
----

diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index c7376bf..01aab1f 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -22,10 +22,15 @@
 #define ARCH_NR_GPIOS		256
 #endif
 
+#ifndef ARCH_FIRST_GPIO
+#define ARCH_FIRST_GPIO		0
+#endif
+
 static inline int gpio_is_valid(int number)
 {
 	/* only some non-negative numbers are valid */
-	return ((unsigned)number) < ARCH_NR_GPIOS;
+	return (number >= ARCH_FIRST_GPIO &&
+		(unsigned)number < ARCH_NR_GPIOS;
 }
 
 struct device;

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[Eric Miao]

On Tue, Sep 7, 2010 at 10:44 AM, Ryan Mallon <ryan@bluewatersys.com> wrote:
> On 09/07/2010 02:23 PM, David Brownell wrote:
>> Still not liking or accepting this proposed
>> change to the GPIO framework.
>>
>> For the AT91 case (where integers 0..N are
>> IRQs, but N..max are GPIOs)
>>
>> A simpler solution is just to use a bit in
>> the integer to indicate IRQ vs GPIO.  Like
>> maybe the sign bit.. which is never set on
>> valid GPIO numbers, but platforms could let
>> be set on IRQs.
>>
> How about this approach instead?
>

This doesn't solve the problem with more complicated settings, e.g.
some GPIOs within are not valid, not just the begining ones.

So the real question here is the semantics of gpio_is_valid(). I'd
personally incline it reads as if a GPIO _number_ is valid generally,
(e.g. like -1 is not a valid GPIO number), instead of that specific
GPIO is valid on that specific platform. The latter can be judged
with gpio_request().

> ----
> On some architectures gpio numbering does not start from zero. Allow for
> correct behaviour of gpio_is_valid on values below the first gpio by
> adding the architecture overrideable ARCH_FIRST_GPIO.
>
> Signed-off-by: Ryan Mallon <ryan@bluewatersys.com>
> ----
>
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index c7376bf..01aab1f 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -22,10 +22,15 @@
>  #define ARCH_NR_GPIOS          256
>  #endif
>
> +#ifndef ARCH_FIRST_GPIO
> +#define ARCH_FIRST_GPIO                0
> +#endif
> +
>  static inline int gpio_is_valid(int number)
>  {
>        /* only some non-negative numbers are valid */
> -       return ((unsigned)number) < ARCH_NR_GPIOS;
> +       return (number >= ARCH_FIRST_GPIO &&
> +               (unsigned)number < ARCH_NR_GPIOS;
>  }
>
>  struct device;
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[Ryan Mallon]

On 09/07/2010 03:54 PM, Eric Miao wrote:
> On Tue, Sep 7, 2010 at 10:44 AM, Ryan Mallon <ryan@bluewatersys.com> wrote:
>> On 09/07/2010 02:23 PM, David Brownell wrote:
>>> Still not liking or accepting this proposed
>>> change to the GPIO framework.
>>>
>>> For the AT91 case (where integers 0..N are
>>> IRQs, but N..max are GPIOs)
>>>
>>> A simpler solution is just to use a bit in
>>> the integer to indicate IRQ vs GPIO.  Like
>>> maybe the sign bit.. which is never set on
>>> valid GPIO numbers, but platforms could let
>>> be set on IRQs.
>>>
>> How about this approach instead?
>>
> 
> This doesn't solve the problem with more complicated settings, e.g.
> some GPIOs within are not valid, not just the begining ones.

Agreed, but this does solve the immediate problem for AT91 in a simple
way. Are there boards in the kernel which have holes in the gpio layout?

Another possible solution is to loop through all the gpio_chips to see
if the number maps to a valid gpio. The obvious downside to this
approach is that the complexity of gpio_is_valid becomes reasonably high
for something which should be a very simple test and, as you say below,
we probably just don't need that fine-grained information.

> So the real question here is the semantics of gpio_is_valid(). I'd
> personally incline it reads as if a GPIO _number_ is valid generally,
> (e.g. like -1 is not a valid GPIO number), instead of that specific
> GPIO is valid on that specific platform. The latter can be judged
> with gpio_request().

Some drivers in the kernel appear to be using this behaviour to have
optional gpios, ie setting the foo_gpio = -1 in the platform data for
some driver. The documentation also suggests that this is what
gpio_is_valid is intended for. However, the documentation also says we
may want gpio_is_valid to return invalid on some other numbers.

~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] pio: add arch specific gpio_is_valid() function

[Eric Miao]

On Tue, Sep 7, 2010 at 12:07 PM, Ryan Mallon <ryan@bluewatersys.com> wrote:
> On 09/07/2010 03:54 PM, Eric Miao wrote:
>> On Tue, Sep 7, 2010 at 10:44 AM, Ryan Mallon <ryan@bluewatersys.com> wrote:
>>> On 09/07/2010 02:23 PM, David Brownell wrote:
>>>> Still not liking or accepting this proposed
>>>> change to the GPIO framework.
>>>>
>>>> For the AT91 case (where integers 0..N are
>>>> IRQs, but N..max are GPIOs)
>>>>
>>>> A simpler solution is just to use a bit in
>>>> the integer to indicate IRQ vs GPIO.  Like
>>>> maybe the sign bit.. which is never set on
>>>> valid GPIO numbers, but platforms could let
>>>> be set on IRQs.
>>>>
>>> How about this approach instead?
>>>
>>
>> This doesn't solve the problem with more complicated settings, e.g.
>> some GPIOs within are not valid, not just the begining ones.
>
> Agreed, but this does solve the immediate problem for AT91 in a simple
> way. Are there boards in the kernel which have holes in the gpio layout?
>
> Another possible solution is to loop through all the gpio_chips to see
> if the number maps to a valid gpio. The obvious downside to this
> approach is that the complexity of gpio_is_valid becomes reasonably high
> for something which should be a very simple test and, as you say below,
> we probably just don't need that fine-grained information.
>
>> So the real question here is the semantics of gpio_is_valid(). I'd
>> personally incline it reads as if a GPIO _number_ is valid generally,
>> (e.g. like -1 is not a valid GPIO number), instead of that specific
>> GPIO is valid on that specific platform. The latter can be judged
>> with gpio_request().
>
> Some drivers in the kernel appear to be using this behaviour to have
> optional gpios, ie setting the foo_gpio = -1 in the platform data for
> some driver. The documentation also suggests that this is what
> gpio_is_valid is intended for. However, the documentation also says we
> may want gpio_is_valid to return invalid on some other numbers.

Right. So I'd really like David to make the semantics clear. My intention
is to keep gpio_is_valid() as simple as checking a general range to
rule out most invalid cases. And just use gpio_request() to handle the
platform specific cases.

>
> ~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] pio: add arch specific gpio_is_valid() function

[Ryan Mallon]

On 09/07/2010 04:19 PM, Eric Miao wrote:
> On Tue, Sep 7, 2010 at 12:07 PM, Ryan Mallon <ryan@bluewatersys.com> wrote:
>> On 09/07/2010 03:54 PM, Eric Miao wrote:
>>> On Tue, Sep 7, 2010 at 10:44 AM, Ryan Mallon <ryan@bluewatersys.com> wrote:
>>>> On 09/07/2010 02:23 PM, David Brownell wrote:
>>>>> Still not liking or accepting this proposed
>>>>> change to the GPIO framework.
>>>>>
>>>>> For the AT91 case (where integers 0..N are
>>>>> IRQs, but N..max are GPIOs)
>>>>>
>>>>> A simpler solution is just to use a bit in
>>>>> the integer to indicate IRQ vs GPIO.  Like
>>>>> maybe the sign bit.. which is never set on
>>>>> valid GPIO numbers, but platforms could let
>>>>> be set on IRQs.
>>>>>
>>>> How about this approach instead?
>>>>
>>>
>>> This doesn't solve the problem with more complicated settings, e.g.
>>> some GPIOs within are not valid, not just the begining ones.
>>
>> Agreed, but this does solve the immediate problem for AT91 in a simple
>> way. Are there boards in the kernel which have holes in the gpio layout?
>>
>> Another possible solution is to loop through all the gpio_chips to see
>> if the number maps to a valid gpio. The obvious downside to this
>> approach is that the complexity of gpio_is_valid becomes reasonably high
>> for something which should be a very simple test and, as you say below,
>> we probably just don't need that fine-grained information.
>>
>>> So the real question here is the semantics of gpio_is_valid(). I'd
>>> personally incline it reads as if a GPIO _number_ is valid generally,
>>> (e.g. like -1 is not a valid GPIO number), instead of that specific
>>> GPIO is valid on that specific platform. The latter can be judged
>>> with gpio_request().
>>
>> Some drivers in the kernel appear to be using this behaviour to have
>> optional gpios, ie setting the foo_gpio = -1 in the platform data for
>> some driver. The documentation also suggests that this is what
>> gpio_is_valid is intended for. However, the documentation also says we
>> may want gpio_is_valid to return invalid on some other numbers.
> 
> Right. So I'd really like David to make the semantics clear. My intention
> is to keep gpio_is_valid() as simple as checking a general range to
> rule out most invalid cases. And just use gpio_request() to handle the
> platform specific cases.

Agreed. The intent of my patch was to keep gpio_is_valid simple, but add
a simple check for architectures where the base gpio is not zero. Will
wait and see what David has to say.

~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] pio: add arch specific gpio_is_valid() function

[David Brownell]

--- On Mon, 9/6/10, Ryan Mallon <ryan@bluewatersys.com> wrote:


> How about this approach instead?

Still don't like it, sorry.  gpio_is_valid()
is not intended as a fine-grained call, there is
a call which is fine grained; use that instead.

> ----
> On some architectures gpio numbering does not start from zero.

But on all of them, zero is a valid GPIO number.
It could get dynamically allocated someday...

 Allow for
> correct behaviour of gpio_is_valid

I'd say it's already correct ... what's not
correct is expecting to validate the *active* set
of GPIOs (some dynamically allocated) through
that, instead of one of the GPIO setup calls
like gpio_request, which have explicit guarantees
of reporting errors for GPIO numbers which are
not usable on the target board.


 on values below the
> first gpio by
> adding the architecture overrideable ARCH_FIRST_GPIO.

What are you (?) doing that it even matters
to a driver  which GPIOs are built into the SOC
versus external?  Caring about arch-specific
stuff at this level is a big thought-bug...

And  I'd ask why you're ignoring or bypassing
the error reporting from gpio_request() ...

That is, just what are you doing that makes
you want gpio_is_valid() to include error checks
you are supposed to get via gpio_request as part
of GPIO configuration?

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[Ben Nizette]

On 07/09/2010, at 4:33 PM, David Brownell wrote:

> 
> 
> --- On Mon, 9/6/10, Ryan Mallon <ryan@bluewatersys.com> wrote:
> 
> 
>> How about this approach instead?
> 
> Still don't like it, sorry.  gpio_is_valid()
> is not intended as a fine-grained call, there is
> a call which is fine grained; use that instead.
> 

I've been sitting on the sidelines but it seems like the below might
help :-)

---8K---

A recent discussion on LKML [1] highlighted some confusion regarding the
semantics of the gpio validity checks versus the gpio request mechanism.

gpio_is_valid determines whether a gpio /can/ be attached to a
controller, gpio_request determines whether that gpio currently /is/
attached to a controller.  Part of the confusion seems to come at least
in part from the overlooking of facility for dynamically added gpio
numbers.

Fix the documentation to clarify these points.

[1] http://lkml.org/lkml/2010/8/23/187

Signed-off-by: Ben Nizette <bn@niasdigital.com>
---
diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
index d96a6db..adc0db2 100644
--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -113,12 +113,13 @@ test if a number could reference a GPIO, you may use this predicate:

	int gpio_is_valid(int number);

-A number that's not valid will be rejected by calls which may request
-or free GPIOs (see below).  Other numbers may also be rejected; for
-example, a number might be valid but unused on a given board.
+This will return true for any number which may represent a valid gpio
+regardless of whether or not that identifier is currently active.  To
+determine whether an identifier is linked to a controller and therefore
+able to be used the request/free mechanism should be used (see below).

-Whether a platform supports multiple GPIO controllers is currently a
-platform-specific implementation issue.
+Whether a platform supports multiple and/or dynamically added GPIO
+controllers is currently a platform-specific implementation issue.


Using GPIOs

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[David Brownell]

> gpio_is_valid determines whether a gpio /can/ be attached to acontroller,

Or more simply:  whether its numeric prameter is
a valid argument to gpio_request() and friends.


 gpio_request determines whether that gpio
> currently /is/
> attached to a controller.

Of course, "controller" is out of sight of folk
just using GPIOs.  Otherwise, that's a fair take
on one set of gpio_request() error reports.


  Part of the confusion seems
> to come at least
> in part from the overlooking of facility for dynamically added gpio numbers.

Maybe, but I think most of it came from folk who
expected "valid" to mean (your terminology) the
GPIO number is now connected to a controller.
(Otherwise $SUBJECT would be meaningless since
there'd be no arch-specific issue.
> 
> Fix the documentation to clarify these points.

Patch is forthcoming.

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[David Brownell]

--- On Mon, 9/6/10, Ryan Mallon <ryan@bluewatersys.com> wrote:

> >  The intent of my patch was to keep gpio_is_valid
> simple, but add
> a simple check for architectures where the base gpio is not
> zero. Will
> wait and see what David has to say.

NAK still.  You're trying to abuse gpio_is_valid(),
which I see no need to support.


In terms of GPIO framework architecture, zero is
the first GPIO in all cases, and is always
a valid GPIO number, even if it's not
requestable/swritable/readable on a given board.

Whether it's usable on a given platform depends
on whether a GPIO controller is registered which
claims numbers 0..N ... (assuming gpiolib in use).

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[avictor.za]

hi,

> NAK still.  You're trying to abuse gpio_is_valid(),
> which I see no need to support.
>
> In terms of GPIO framework architecture, zero is
> the first GPIO in all cases, and is always
> a valid GPIO number, even if it's not
> requestable/swritable/readable on a given board.
>
> Whether it's usable on a given platform depends
> on whether a GPIO controller is registered which
> claims numbers 0..N ... (assuming gpiolib in use).

How should the following be done in a driver then?

   if (gpio_is_valid(device->output_pin)) {
         if (gpio_request(device->output_pin, "driverX") != 0)
             goto error_handling;

         /* continue with gpio setup */
    }
    else {
        /* there is no vcc_pin, so don't do any gpio setup */
    }

    ....

   if (gpio_is_valid(device->output_pin)) {
       /* set value high */
   }


Regards,
  Andrew Victor

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[Ryan Mallon]

avictor.za@gmail.com wrote:
> hi,
> 
>> NAK still.  You're trying to abuse gpio_is_valid(),
>> which I see no need to support.
>>
>> In terms of GPIO framework architecture, zero is
>> the first GPIO in all cases, and is always
>> a valid GPIO number, even if it's not
>> requestable/swritable/readable on a given board.
>>
>> Whether it's usable on a given platform depends
>> on whether a GPIO controller is registered which
>> claims numbers 0..N ... (assuming gpiolib in use).
> 
> How should the following be done in a driver then?
> 
>    if (gpio_is_valid(device->output_pin)) {
>          if (gpio_request(device->output_pin, "driverX") != 0)
>              goto error_handling;
> 
>          /* continue with gpio setup */
>     }
>     else {
>         /* there is no vcc_pin, so don't do any gpio setup */

Adding:

	 device->output_pin = -EINVAL;

Will force the gpio to be invalid here, so that subsequent uses of
gpio_is_valid will behave as expected in the case where
device->output_pin >= 0, but doesn't map to a useable gpio.

>     }
> 
>     ....
> 
>    if (gpio_is_valid(device->output_pin)) {
>        /* set value high */
>    }

~Ryan

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[Alan Cox]

> Adding:
> 
> 	 device->output_pin = -EINVAL;

gpio numbers are unsigned in the rest of the API

It's unfortunate that 0 was used for a GPIO - I take it this is too late
to get fixed as we had to fix IRQ numbering and the like ?

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[David Brownell]

--- On Tue, 9/7/10, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > 
> >      device->output_pin = -EINVAL;
> 
> gpio numbers are unsigned in the rest of the API

Minor goof; should have been "int" everywhere,
as implied by the references to negative numbers
in the docs... negatives not used internally, but
preserved for external use (as above).


> 
> It's unfortunate that 0 was used for a GPIO

Intentional:  board schematics and chip docs can
easily match up to Linux this way.  What would be
unfortunate is needing to map hardware docs into
software ones all the time -- error prone.

When the API is used correctly, it's not an issue.

Re: [PATCH] pio: add arch specific gpio_is_valid() function

[Alan Cox]

On Tue, 7 Sep 2010 16:44:55 -0700 (PDT)
David Brownell <david-b@pacbell.net> wrote:

> 
> 
> --- On Tue, 9/7/10, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > > 
> > >      device->output_pin = -EINVAL;
> > 
> > gpio numbers are unsigned in the rest of the API
> 
> Minor goof; should have been "int" everywhere,
> as implied by the references to negative numbers
> in the docs... negatives not used internally, but
> preserved for external use (as above).

Ok I need to patch a couple of the intel drivers to use -1 then.

Alan