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

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Haavard Skinnemoen <[email protected]>
---
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

2009-09-29 19:29:35

by Andrew Morton

[permalink] [raw]

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

On Thu, 17 Sep 2009 18:29:26 +0200
Nicolas Ferre <[email protected]> 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 <[email protected]>
> ---
> 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.

2009-09-30 13:33:23

[permalink] [raw]

Subject: 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 <[email protected]> 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 <[email protected]>
>> ---
>> 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

2009-09-30 13:56:14

[permalink] [raw]

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

Nicolas Ferre <[email protected]> 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

2009-10-23 15:29:19

[permalink] [raw]

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

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

2009-10-23 15:29:51

[permalink] [raw]

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

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 <[email protected]>
---
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

2009-10-23 15:29:31

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Haavard Skinnemoen <[email protected]>
---
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

2009-10-23 15:29:38

[permalink] [raw]

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

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 <[email protected]>
---
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

2009-10-26 08:34:44

by Yegor Yefremov

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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 <[email protected]>

2009-10-27 19:43:25

by Andrew Victor

[permalink] [raw]

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

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

2009-10-28 00:36:18

[permalink] [raw]

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

Andrew Victor <[email protected]> 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

2009-10-28 00:53:24

by Thiago A. Corrêa

[permalink] [raw]

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

On Tue, Oct 27, 2009 at 10:35 PM, Haavard Skinnemoen
<[email protected]> wrote:
> Andrew Victor <[email protected]> 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

2009-10-28 01:45:28

[permalink] [raw]

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

Thiago A. Corrêa <[email protected]> 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

2009-10-28 19:53:17

by Andrew Victor

[permalink] [raw]

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

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

2009-10-28 20:50:38

[permalink] [raw]

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

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.

2009-11-02 17:12:52

[permalink] [raw]

Subject: 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;
> }

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

2009-11-02 17:15:58

[permalink] [raw]

Subject: 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 <[email protected]>

[..]

>> + /* 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 <[email protected]>

Ok, I will add.

Thanks a lot.

Best regards,
--
Nicolas Ferre

2009-11-02 17:19:06

[permalink] [raw]

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

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

2009-11-02 22:10:19

[permalink] [raw]

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

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,

2009-11-02 22:14:45

[permalink] [raw]

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

[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,

2009-11-03 02:34:17

by Ryan Mallon

[permalink] [raw]

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

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
[email protected] 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

2009-11-03 02:55:31

[permalink] [raw]

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

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.

2009-11-07 11:20:06

[permalink] [raw]

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

Ben Nizette <[email protected]> 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

2009-11-18 13:33:45

[permalink] [raw]

Subject: 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).

Andrew, Haavard,

Ping ?

--
Nicolas Ferre

2010-08-23 13:56:21

[permalink] [raw]

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

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 <[email protected]>
---
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

2010-08-23 16:36:37

by David Brownell

[permalink] [raw]

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

--- On Mon, 8/23/10, Nicolas Ferre <[email protected]> wrote:

> From: Nicolas Ferre <[email protected]>
> 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".

>

2010-08-24 08:19:53