[PATCH 0/4] Add SDHCI driver for Tegra

[PATCH 0/4] Add SDHCI driver for Tegra

[Olof Johansson]

Hi Chris,

This series introduces a SDHCI driver for the on-chip MMC/SDIO controller
on tegra. Some of the erratas weren't feasible to work around locally
in the driver, so it also adds a couple of new quirk bits.

Thanks,

-Olof

[PATCH 1/4] sdhci: add quirk for max len ADMA descriptors

[Olof Johansson]

Some controllers misparse segment length 0 as being 0, not 65536. Add
a quirk to deal with it.

Signed-off-by: Olof Johansson <olof@lixom.net>
---
 drivers/mmc/host/sdhci.c  |   10 +++++++---
 include/linux/mmc/sdhci.h |    2 ++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a25db426..c0094c1 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1928,10 +1928,14 @@ int sdhci_add_host(struct sdhci_host *host)
 	 * of bytes. When doing hardware scatter/gather, each entry cannot
 	 * be larger than 64 KiB though.
 	 */
-	if (host->flags & SDHCI_USE_ADMA)
-		mmc->max_seg_size = 65536;
-	else
+	if (host->flags & SDHCI_USE_ADMA) {
+		if (host->quirks & SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC)
+			mmc->max_seg_size = 65535;
+		else
+			mmc->max_seg_size = 65536;
+	} else {
 		mmc->max_seg_size = mmc->max_req_size;
+	}
 
 	/*
 	 * Maximum block size. This varies from controller to controller and
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 1fdc673..dfb2106 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -83,6 +83,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12		(1<<28)
 /* Controller doesn't have HISPD bit field in HI-SPEED SD card */
 #define SDHCI_QUIRK_NO_HISPD_BIT			(1<<29)
+/* Controller treats ADMA descriptors with length 0000h incorrectly */
+#define SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC		(1<<30)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
1.7.3.GIT

[PATCH 2/4] sdhci: add quirk for broken sdio irq

[Olof Johansson]

Some controllers can't handle SDIO IRQ properly. Give a way to
disable it.

Signed-off-by: Olof Johansson <olof@lixom.net>
---
 drivers/mmc/host/sdhci.c  |    6 +++++-
 include/linux/mmc/sdhci.h |    2 ++
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c0094c1..98f3d3d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1871,7 +1871,10 @@ int sdhci_add_host(struct sdhci_host *host)
 		mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
 
 	mmc->f_max = host->max_clk;
-	mmc->caps |= MMC_CAP_SDIO_IRQ;
+	mmc->caps = 0;
+
+	if (!(host->quirks & SDHCI_QUIRK_NO_SDIO_IRQ))
+		mmc->caps |= MMC_CAP_SDIO_IRQ;
 
 	/*
 	 * A controller may support 8-bit width, but the board itself
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index dfb2106..97d547e 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -85,6 +85,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_NO_HISPD_BIT			(1<<29)
 /* Controller treats ADMA descriptors with length 0000h incorrectly */
 #define SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC		(1<<30)
+/* Controller should not use SDIO IRQ */
+#define SDHCI_QUIRK_NO_SDIO_IRQ				(1<<31)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
1.7.3.GIT

Re: [PATCH 2/4] sdhci: add quirk for broken sdio irq

[zhangfei gao]

On Tue, Dec 14, 2010 at 11:49 PM, Olof Johansson <olof@lixom.net> wrote:
> Some controllers can't handle SDIO IRQ properly. Give a way to
> disable it.
>
> Signed-off-by: Olof Johansson <olof@lixom.net>
> ---
>  drivers/mmc/host/sdhci.c  |    6 +++++-
>  include/linux/mmc/sdhci.h |    2 ++
>  2 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c0094c1..98f3d3d 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1871,7 +1871,10 @@ int sdhci_add_host(struct sdhci_host *host)
>                mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
>
>        mmc->f_max = host->max_clk;
> -       mmc->caps |= MMC_CAP_SDIO_IRQ;
> +       mmc->caps = 0;
> +
> +       if (!(host->quirks & SDHCI_QUIRK_NO_SDIO_IRQ))
> +               mmc->caps |= MMC_CAP_SDIO_IRQ;
>
>        /*
>         * A controller may support 8-bit width, but the board itself
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index dfb2106..97d547e 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -85,6 +85,8 @@ struct sdhci_host {
>  #define SDHCI_QUIRK_NO_HISPD_BIT                       (1<<29)
>  /* Controller treats ADMA descriptors with length 0000h incorrectly */
>  #define SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC           (1<<30)
> +/* Controller should not use SDIO IRQ */
> +#define SDHCI_QUIRK_NO_SDIO_IRQ                                (1<<31)

Just wander can we use another way instead of using the last two
quirk, otherwise, no quirk can be used any more.
If this is only tegra specific issue, any possibility to modify vector
after add_host in sdhci-tegra.c.
for example
sdhci_add_host(host);
host->mmc->max_seg_size = 65535;
host->mmc->caps |= MMC_CAP_SDIO_IRQ;

Really appreciate if not using the valuable quirk resource, which is u32.

>
>        int irq;                /* Device IRQ */
>        void __iomem *ioaddr;   /* Mapped address */
> --
> 1.7.3.GIT
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/4] sdhci: add quirk for broken sdio irq

[Olof Johansson]

On Wed, Dec 15, 2010 at 05:42:14AM -0500, zhangfei gao wrote:
> On Tue, Dec 14, 2010 at 11:49 PM, Olof Johansson <olof@lixom.net> wrote:
> > Some controllers can't handle SDIO IRQ properly. Give a way to
> > disable it.
> >
> > Signed-off-by: Olof Johansson <olof@lixom.net>
> > ---
> >  drivers/mmc/host/sdhci.c  |    6 +++++-
> >  include/linux/mmc/sdhci.h |    2 ++
> >  2 files changed, 7 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index c0094c1..98f3d3d 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1871,7 +1871,10 @@ int sdhci_add_host(struct sdhci_host *host)
> >                mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
> >
> >        mmc->f_max = host->max_clk;
> > -       mmc->caps |= MMC_CAP_SDIO_IRQ;
> > +       mmc->caps = 0;
> > +
> > +       if (!(host->quirks & SDHCI_QUIRK_NO_SDIO_IRQ))
> > +               mmc->caps |= MMC_CAP_SDIO_IRQ;
> >
> >        /*
> >         * A controller may support 8-bit width, but the board itself
> > diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> > index dfb2106..97d547e 100644
> > --- a/include/linux/mmc/sdhci.h
> > +++ b/include/linux/mmc/sdhci.h
> > @@ -85,6 +85,8 @@ struct sdhci_host {
> >  #define SDHCI_QUIRK_NO_HISPD_BIT                       (1<<29)
> >  /* Controller treats ADMA descriptors with length 0000h incorrectly */
> >  #define SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC           (1<<30)
> > +/* Controller should not use SDIO IRQ */
> > +#define SDHCI_QUIRK_NO_SDIO_IRQ                                (1<<31)
> 
> Just wander can we use another way instead of using the last two
> quirk, otherwise, no quirk can be used any more.
> If this is only tegra specific issue, any possibility to modify vector
> after add_host in sdhci-tegra.c.
> for example
> sdhci_add_host(host);
> host->mmc->max_seg_size = 65535;
> host->mmc->caps |= MMC_CAP_SDIO_IRQ;

Yuck. That completely breaks any kind of abstraction and layering.

> Really appreciate if not using the valuable quirk resource, which is u32.

No big deal. Next person that needs a quirk bit gets to bump the data
type to u64.


-Olof

Re: [PATCH 2/4] sdhci: add quirk for broken sdio irq

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 532 bytes --]


> > Really appreciate if not using the valuable quirk resource, which is u32.
> 
> No big deal. Next person that needs a quirk bit gets to bump the data
> type to u64.

That won't scale for long, I fear (and will make sdhci.c an unreadable
mess). We need less quirks and more flexible means to handle flawed
controllers, I'd think.

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/4] sdhci: add quirk for broken sdio irq

[Olof Johansson]

On Wed, Dec 15, 2010 at 12:03:12PM +0100, Wolfram Sang wrote:
> 
> > > Really appreciate if not using the valuable quirk resource, which is u32.
> > 
> > No big deal. Next person that needs a quirk bit gets to bump the data
> > type to u64.
> 
> That won't scale for long, I fear (and will make sdhci.c an unreadable
> mess). We need less quirks and more flexible means to handle flawed
> controllers, I'd think.

It won't scale for long, but it'll give a buffer while a cleaned up
quirk structure can be hashed out and implemented.

There are already a handful of quirks in there that can be trivially
moved into their respective drivers by overriding the register read/write
functions (like I already have for a couple on sdhci-tegra). Some of
the others are not obvious to me how they can be easily abstracted out
without adding even more callbacks up and down the stack for adding hooks.


-Olof

Re: [PATCH 2/4] sdhci: add quirk for broken sdio irq

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1196 bytes --]


> > That won't scale for long, I fear (and will make sdhci.c an unreadable
> > mess). We need less quirks and more flexible means to handle flawed
> > controllers, I'd think.
> 
> It won't scale for long, but it'll give a buffer while a cleaned up
> quirk structure can be hashed out and implemented.

I am a bit pessimistic here; at the end of u64, the first suggestion
will probably be using two u64 ;)

> There are already a handful of quirks in there that can be trivially
> moved into their respective drivers by overriding the register read/write
> functions (like I already have for a couple on sdhci-tegra). Some of

I know. Despite being easy, it hardly resulted in patches so far.

> the others are not obvious to me how they can be easily abstracted out
> without adding even more callbacks up and down the stack for adding hooks.

Yes, it needs some architectural work. If we'd go for u64 now, we will
just earn more quirks and it will get harder and harder to abstract.

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/4] sdhci: add quirk for broken sdio irq

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

On Tue, Dec 14, 2010 at 10:49:34PM -0600, Olof Johansson wrote:
> Some controllers can't handle SDIO IRQ properly. Give a way to
> disable it.
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> ---
>  drivers/mmc/host/sdhci.c  |    6 +++++-
>  include/linux/mmc/sdhci.h |    2 ++
>  2 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c0094c1..98f3d3d 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1871,7 +1871,10 @@ int sdhci_add_host(struct sdhci_host *host)
>  		mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
>  
>  	mmc->f_max = host->max_clk;
> -	mmc->caps |= MMC_CAP_SDIO_IRQ;
> +	mmc->caps = 0;

That would clear flags already set in custom probe functions (check
sdhci-pxa) for example.

> +
> +	if (!(host->quirks & SDHCI_QUIRK_NO_SDIO_IRQ))
> +		mmc->caps |= MMC_CAP_SDIO_IRQ;

As we are running out of quirk-bits, can you imagine a solution without
a quirk (same for the other patch)?

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/4] sdhci: add quirk for broken sdio irq

[Olof Johansson]

On Wed, Dec 15, 2010 at 11:57:01AM +0100, Wolfram Sang wrote:
> On Tue, Dec 14, 2010 at 10:49:34PM -0600, Olof Johansson wrote:
> > Some controllers can't handle SDIO IRQ properly. Give a way to
> > disable it.
> > 
> > Signed-off-by: Olof Johansson <olof@lixom.net>
> > ---
> >  drivers/mmc/host/sdhci.c  |    6 +++++-
> >  include/linux/mmc/sdhci.h |    2 ++
> >  2 files changed, 7 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index c0094c1..98f3d3d 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1871,7 +1871,10 @@ int sdhci_add_host(struct sdhci_host *host)
> >  		mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
> >  
> >  	mmc->f_max = host->max_clk;
> > -	mmc->caps |= MMC_CAP_SDIO_IRQ;
> > +	mmc->caps = 0;
> 
> That would clear flags already set in custom probe functions (check
> sdhci-pxa) for example.

Good point, will fix.

> > +
> > +	if (!(host->quirks & SDHCI_QUIRK_NO_SDIO_IRQ))
> > +		mmc->caps |= MMC_CAP_SDIO_IRQ;
> 
> As we are running out of quirk-bits, can you imagine a solution without
> a quirk (same for the other patch)?

See other sub-thread. Not without violating the layering. If anyone else
has suggestions I'm all ears though.


-Olof

[PATCH 3/4] sdhci: don't finish commands in flight

[Olof Johansson]

Don't schedule the finish_tasklet unless the command complete bit is
set in the interrupt status register.

Signed-off-by: Olof Johansson <olof@lixom.net>
---
 drivers/mmc/host/sdhci.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 98f3d3d..4285cba 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1432,7 +1432,8 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask)
 		host->cmd->error = -EILSEQ;
 
 	if (host->cmd->error) {
-		tasklet_schedule(&host->finish_tasklet);
+		if (intmask & SDHCI_INT_RESPONSE)
+			tasklet_schedule(&host->finish_tasklet);
 		return;
 	}
 
-- 
1.7.3.GIT

[PATCH 4/4] mmc: add sdhci-tegra driver for Tegra SoCs

[Olof Johansson]

SDHCI driver for Tegra. Pretty straight forward, a few pieces of
functionality left to fill in but nothing that stops it from going
upstream. Board enablement submitted separately.

This driver is based on an original one from:

Signed-off-by: Yvonne Yip <y@palm.com>

Misc fixes and suspend/resume by:

Signed-off-by: Gary King <gking@nvidia.com>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
Signed-off-by: Dmitry Shmidt <dimitrysh@google.com>

GPIO write protect plumbing:

Signed-off-by: Rhyland Klein <rklein@nvidia.com>

Quirk rework and cleanups before submission:

Signed-off-by: Olof Johansson <olof@lixom.net>
---
 arch/arm/mach-tegra/include/mach/sdhci.h |   28 +++
 drivers/mmc/host/Kconfig                 |   10 +
 drivers/mmc/host/Makefile                |    1 +
 drivers/mmc/host/sdhci-tegra.c           |  267 ++++++++++++++++++++++++++++++
 4 files changed, 306 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-tegra/include/mach/sdhci.h
 create mode 100644 drivers/mmc/host/sdhci-tegra.c

diff --git a/arch/arm/mach-tegra/include/mach/sdhci.h b/arch/arm/mach-tegra/include/mach/sdhci.h
new file mode 100644
index 0000000..8ca9539
--- /dev/null
+++ b/arch/arm/mach-tegra/include/mach/sdhci.h
@@ -0,0 +1,28 @@
+/*
+ * include/asm-arm/arch-tegra/include/mach/sdhci.h
+ *
+ * Copyright (C) 2009 Palm, Inc.
+ * Author: Yvonne Yip <y@palm.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#ifndef __ASM_ARM_ARCH_TEGRA_SDHCI_H
+#define __ASM_ARM_ARCH_TEGRA_SDHCI_H
+
+#include <linux/mmc/host.h>
+
+struct tegra_sdhci_platform_data {
+	int cd_gpio;
+	int wp_gpio;
+	int power_gpio;
+};
+
+#endif
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index d618e86..bd69bf9 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -189,6 +189,16 @@ config MMC_SDHCI_S3C_DMA
 
 	  YMMV.
 
+config MMC_SDHCI_TEGRA
+	tristate "Tegra SD/MMC Controller Support"
+	depends on ARCH_TEGRA && MMC_SDHCI
+	select MMC_SDHCI_IO_ACCESSORS
+	help
+	  This selects the Tegra SD/MMC controller. If you have a Tegra
+	  platform with SD or MMC devices, say Y or M here.
+
+	  If unsure, say N.
+
 config MMC_OMAP
 	tristate "TI OMAP Multimedia Card Interface support"
 	depends on ARCH_OMAP
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 7b645ff..a096f7f 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_MMC_SDHCI_PCI)	+= sdhci-pci.o
 obj-$(CONFIG_MMC_SDHCI_PXA)	+= sdhci-pxa.o
 obj-$(CONFIG_MMC_SDHCI_S3C)	+= sdhci-s3c.o
 obj-$(CONFIG_MMC_SDHCI_SPEAR)	+= sdhci-spear.o
+obj-$(CONFIG_MMC_SDHCI_TEGRA)	+= sdhci-tegra.o
 obj-$(CONFIG_MMC_WBSD)		+= wbsd.o
 obj-$(CONFIG_MMC_AU1X)		+= au1xmmc.o
 obj-$(CONFIG_MMC_OMAP)		+= omap.o
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
new file mode 100644
index 0000000..eb6b952
--- /dev/null
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -0,0 +1,267 @@
+/*
+ * drivers/mmc/host/sdhci-tegra.c
+ *
+ * Copyright (C) 2009 Palm, Inc.
+ * Author: Yvonne Yip <y@palm.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/mmc/card.h>
+
+#include <mach/sdhci.h>
+
+#include "sdhci.h"
+
+#define DRIVER_NAME    "sdhci-tegra"
+
+struct tegra_sdhci_host {
+	struct sdhci_host *sdhci;
+	struct clk *clk;
+	int wp_gpio;
+};
+
+
+static u32 tegra_sdhci_readl(struct sdhci_host *host, int reg)
+{
+	u32 val;
+
+	if (unlikely(reg == SDHCI_PRESENT_STATE)) {
+		/* Use wp_gpio here instead? */
+		val = readl(host->ioaddr + reg);
+		return val | SDHCI_WRITE_PROTECT;
+	}
+
+	return readl(host->ioaddr + reg);
+}
+
+static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg)
+{
+	if (unlikely(reg == SDHCI_HOST_VERSION)) {
+		/* Erratum: Version register is invalid in HW. */
+		return SDHCI_SPEC_200;
+	}
+
+	return readw(host->ioaddr + reg);
+}
+
+static void tegra_sdhci_writel(struct sdhci_host *host, u32 val, int reg)
+{
+	writel(val, host->ioaddr + reg);
+	if (unlikely(reg == SDHCI_INT_ENABLE)) {
+		/* Erratum: Must enable block gap interrupt detection */
+		u8 gap_ctrl = readb(host->ioaddr + SDHCI_BLOCK_GAP_CONTROL);
+		if (val & SDHCI_INT_CARD_INT)
+			gap_ctrl |= 0x8;
+		else
+			gap_ctrl &= ~0x8;
+		writeb(gap_ctrl, host->ioaddr + SDHCI_BLOCK_GAP_CONTROL);
+	}
+}
+
+static unsigned int tegra_sdhci_get_ro(struct sdhci_host *sdhci)
+{
+	struct tegra_sdhci_host *host = sdhci_priv(sdhci);
+
+	if (host->wp_gpio != -1)
+		return gpio_get_value(host->wp_gpio);
+
+	return -1;
+}
+
+static irqreturn_t carddetect_irq(int irq, void *data)
+{
+	struct sdhci_host *sdhost = (struct sdhci_host *)data;
+
+	tasklet_schedule(&sdhost->card_tasklet);
+	return IRQ_HANDLED;
+};
+
+static int tegra_sdhci_enable_dma(struct sdhci_host *host)
+{
+	return 0;
+}
+
+static struct sdhci_ops tegra_sdhci_ops = {
+	.enable_dma = tegra_sdhci_enable_dma,
+	.get_ro     = tegra_sdhci_get_ro,
+	.read_l     = tegra_sdhci_readl,
+	.read_w     = tegra_sdhci_readw,
+	.write_l    = tegra_sdhci_writel,
+};
+
+static int __devinit tegra_sdhci_probe(struct platform_device *pdev)
+{
+	int rc;
+	struct tegra_sdhci_platform_data *plat;
+	struct sdhci_host *sdhci;
+	struct tegra_sdhci_host *host;
+	struct resource *res;
+	int irq;
+	void __iomem *ioaddr;
+
+	plat = pdev->dev.platform_data;
+	if (plat == NULL)
+		return -ENXIO;
+
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (res == NULL)
+		return -ENODEV;
+
+	irq = res->start;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res == NULL)
+		return -ENODEV;
+
+	ioaddr = ioremap(res->start, res->end - res->start);
+
+	sdhci = sdhci_alloc_host(&pdev->dev, sizeof(struct tegra_sdhci_host));
+	if (IS_ERR(sdhci)) {
+		rc = PTR_ERR(sdhci);
+		goto err_unmap;
+	}
+
+	host = sdhci_priv(sdhci);
+	host->sdhci = sdhci;
+
+	host->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(host->clk)) {
+		rc = PTR_ERR(host->clk);
+		goto err_free_host;
+	}
+
+	rc = clk_enable(host->clk);
+	if (rc != 0)
+		goto err_clkput;
+
+	host->wp_gpio = plat->wp_gpio;
+
+	sdhci->hw_name = "tegra";
+	sdhci->ops = &tegra_sdhci_ops;
+	sdhci->irq = irq;
+	sdhci->ioaddr = ioaddr;
+	sdhci->quirks = SDHCI_QUIRK_BROKEN_TIMEOUT_VAL |
+			SDHCI_QUIRK_SINGLE_POWER_WRITE |
+			SDHCI_QUIRK_NO_HISPD_BIT |
+			SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC |
+			SDHCI_QUIRK_NO_SDIO_IRQ;
+
+	rc = sdhci_add_host(sdhci);
+	if (rc)
+		goto err_clk_disable;
+
+	platform_set_drvdata(pdev, host);
+
+	if (plat->cd_gpio != -1) {
+		rc = request_irq(gpio_to_irq(plat->cd_gpio), carddetect_irq,
+			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			mmc_hostname(sdhci->mmc), sdhci);
+
+		if (rc)
+			goto err_remove_host;
+	}
+
+	printk(KERN_INFO "sdhci%d: initialized irq %d ioaddr %p\n", pdev->id,
+			sdhci->irq, sdhci->ioaddr);
+
+	return 0;
+
+err_remove_host:
+	sdhci_remove_host(sdhci, 1);
+err_clk_disable:
+	clk_disable(host->clk);
+err_clkput:
+	clk_put(host->clk);
+err_free_host:
+	if (sdhci)
+		sdhci_free_host(sdhci);
+err_unmap:
+	iounmap(sdhci->ioaddr);
+
+	return rc;
+}
+
+static int tegra_sdhci_remove(struct platform_device *pdev)
+{
+	struct tegra_sdhci_host *host = platform_get_drvdata(pdev);
+	if (host) {
+		struct tegra_sdhci_platform_data *plat;
+		plat = pdev->dev.platform_data;
+
+		sdhci_remove_host(host->sdhci, 0);
+		sdhci_free_host(host->sdhci);
+	}
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int tegra_sdhci_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct tegra_sdhci_host *host = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = sdhci_suspend_host(host->sdhci, state);
+	if (ret)
+		pr_err("%s: failed, error = %d\n", __func__, ret);
+
+	return ret;
+}
+
+static int tegra_sdhci_resume(struct platform_device *pdev)
+{
+	struct tegra_sdhci_host *host = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = sdhci_resume_host(host->sdhci);
+	if (ret)
+		pr_err("%s: failed, error = %d\n", __func__, ret);
+
+	return ret;
+}
+#else
+#define tegra_sdhci_suspend    NULL
+#define tegra_sdhci_resume     NULL
+#endif
+
+static struct platform_driver tegra_sdhci_driver = {
+	.probe = tegra_sdhci_probe,
+	.remove = tegra_sdhci_remove,
+	.suspend = tegra_sdhci_suspend,
+	.resume = tegra_sdhci_resume,
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init tegra_sdhci_init(void)
+{
+	return platform_driver_register(&tegra_sdhci_driver);
+}
+
+static void __exit tegra_sdhci_exit(void)
+{
+	platform_driver_unregister(&tegra_sdhci_driver);
+}
+
+module_init(tegra_sdhci_init);
+module_exit(tegra_sdhci_exit);
+
+MODULE_DESCRIPTION("Tegra SDHCI controller driver");
+MODULE_LICENSE("GPL");
-- 
1.7.3.GIT

Re: [PATCH 4/4] mmc: add sdhci-tegra driver for Tegra SoCs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 985 bytes --]

Hi Olof,

On Tue, Dec 14, 2010 at 10:49:36PM -0600, Olof Johansson wrote:
> SDHCI driver for Tegra. Pretty straight forward, a few pieces of
> functionality left to fill in but nothing that stops it from going
> upstream. Board enablement submitted separately.
> 
> This driver is based on an original one from:
> 
> Signed-off-by: Yvonne Yip <y@palm.com>
> 
> Misc fixes and suspend/resume by:
> 
> Signed-off-by: Gary King <gking@nvidia.com>
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> Signed-off-by: Dmitry Shmidt <dimitrysh@google.com>
> 
> GPIO write protect plumbing:
> 
> Signed-off-by: Rhyland Klein <rklein@nvidia.com>
> 
> Quirk rework and cleanups before submission:
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>

Can't you just use sdhci-pltfm?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 4/4] mmc: add sdhci-tegra driver for Tegra SoCs

[Olof Johansson]

Hi,

On Wed, Dec 15, 2010 at 09:35:46AM +0100, Wolfram Sang wrote:

> Can't you just use sdhci-pltfm?

Not if I want to hide some of the SoC-specific errata with driver-local
workarounds instead of polluting the global quirk namespace with them.

Otherwise, yeah, it'd be a good candidate for it.


-Olof

Re: [PATCH 4/4] mmc: add sdhci-tegra driver for Tegra SoCs

[Olof Johansson]

On Wed, Dec 15, 2010 at 02:43:07AM -0600, Olof Johansson wrote:
> Hi,
> 
> On Wed, Dec 15, 2010 at 09:35:46AM +0100, Wolfram Sang wrote:
> 
> > Can't you just use sdhci-pltfm?
> 
> Not if I want to hide some of the SoC-specific errata with driver-local
> workarounds instead of polluting the global quirk namespace with them.
> 
> Otherwise, yeah, it'd be a good candidate for it.

Hm, on second look, it seems as if the -pltfm can take the ops structure
from the platform data.

Still, I don't see the point -- the amount of code would be about the
same between wrapping it with a coat of workarounds and op structures,
gpio setup, etc, and just doing a separate simple driver. The code shared
is really just the resource allocation pieces.


-Olof

Re: [PATCH 4/4] mmc: add sdhci-tegra driver for Tegra SoCs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 744 bytes --]


> Still, I don't see the point -- the amount of code would be about the
> same between wrapping it with a coat of workarounds and op structures,
> gpio setup, etc, and just doing a separate simple driver. The code shared
> is really just the resource allocation pieces.

? Saving 150 out of 260 lines does not count? Having a central point for
bug-fixes in that part of the code?

If there is something yet missing in sdhci-pltfm which you need, it
probably is worth adding it there. Chances are good that other
pltfm-users might want that, too.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 4/4] mmc: add sdhci-tegra driver for Tegra SoCs

[Olof Johansson]

On Wed, Dec 15, 2010 at 11:37:30AM +0100, Wolfram Sang wrote:
> 
> > Still, I don't see the point -- the amount of code would be about the
> > same between wrapping it with a coat of workarounds and op structures,
> > gpio setup, etc, and just doing a separate simple driver. The code shared
> > is really just the resource allocation pieces.
> 
> ? Saving 150 out of 260 lines does not count? Having a central point for
> bug-fixes in that part of the code?

Between duplicating some platform resource allocation code in the separate driver,
a driver (or shim layer) that will be needed in either case, I don't consider that
a high price to pay.

> If there is something yet missing in sdhci-pltfm which you need, it
> probably is worth adding it there. Chances are good that other
> pltfm-users might want that, too.

Taking that to an extreme, any sdhci driver should plug into sdhci-pltfm and
just add the hooks needed. It will result in just one more abstraction layer that
makes following code flow harder.

I know there's times when it makes sense to do so. I just consider this one to be
over the line where it makes sense to do a separate driver.

Especially if you want some of the quirk code to be moved from sdhci.c
to the driver.


Thanks,

-Olof

Re: [PATCH 4/4] mmc: add sdhci-tegra driver for Tegra SoCs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]


> > If there is something yet missing in sdhci-pltfm which you need, it
> > probably is worth adding it there. Chances are good that other
> > pltfm-users might want that, too.
> 
> Taking that to an extreme, any sdhci driver should plug into sdhci-pltfm and
> just add the hooks needed. It will result in just one more abstraction layer that
> makes following code flow harder.

Yes, of course. SDHCI is pretty well defined, so a good core on a SOC
should just need sdhci-pltfm and some pltfm-data (and we got rid of a
complete driver that way recently). If the core has quirks (as most
sadly do), a bit of extension is needed. I'd prefer to have those
extensions in one place rather than hidden in various drivers. I have
the assumption they will be reusable, maybe this is a core point where
we disagree?

> Especially if you want some of the quirk code to be moved from sdhci.c
> to the driver.

Please elaborate. Why is sdhci-pltfm complicating things here?

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 4/4] mmc: add sdhci-tegra driver for Tegra SoCs

[Olof Johansson]

On Wed, Dec 15, 2010 at 12:59:11PM +0100, Wolfram Sang wrote:
> 
> > > If there is something yet missing in sdhci-pltfm which you need, it
> > > probably is worth adding it there. Chances are good that other
> > > pltfm-users might want that, too.
> > 
> > Taking that to an extreme, any sdhci driver should plug into sdhci-pltfm and
> > just add the hooks needed. It will result in just one more abstraction layer that
> > makes following code flow harder.
> 
> Yes, of course. SDHCI is pretty well defined, so a good core on a SOC
> should just need sdhci-pltfm and some pltfm-data (and we got rid of a
> complete driver that way recently). If the core has quirks (as most
> sadly do), a bit of extension is needed. I'd prefer to have those
> extensions in one place rather than hidden in various drivers. I have
> the assumption they will be reusable, maybe this is a core point where
> we disagree?

Actually, I was of the impression that you wanted to hide the per-platform
hooks away from drivers/mmc/host. I see now that other drivers still add it
there. That's acceptable to me.

I'll whip up the changes and see how invasive it gets as a -pltfm
driver. It shouldn't be too bad. I'll repost the series that way, but
I will still keep the quirks for now and do a pass reworking these as
well as others separately (see below).

> > Especially if you want some of the quirk code to be moved from sdhci.c
> > to the driver.
> 
> Please elaborate. Why is sdhci-pltfm complicating things here?

Well, doing post-host_add-modifications of host->mmc->caps would add yet
another hook for the various platforms. :(

Anyway, I wonder if it does makes some sense to add a sdhci_ops callback
for fixing up the mmc settings after setting the defaults in host_add,
and for not just -pltfm. A handful of quirks are already only used for
overriding defaults, and they could be freed up. There's a few more that
are only used by a single driver and are suitable for handling through
overloaded register read/write functions.


I'll do a first pass, but it might take me a week or two due to other
more pressing things.


-Olof

Re: [PATCH 4/4] mmc: add sdhci-tegra driver for Tegra SoCs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]


> I'll whip up the changes and see how invasive it gets as a -pltfm
> driver. It shouldn't be too bad. I'll repost the series that way, but
> I will still keep the quirks for now and do a pass reworking these as
> well as others separately (see below).

Sounds great!

> Anyway, I wonder if it does makes some sense to add a sdhci_ops callback
> for fixing up the mmc settings after setting the defaults in host_add,
> and for not just -pltfm. A handful of quirks are already only used for
> overriding defaults, and they could be freed up. There's a few more that
> are only used by a single driver and are suitable for handling through
> overloaded register read/write functions.

Yes, exactly. I'd think more than 50% of the quirks can go away.

> I'll do a first pass, but it might take me a week or two due to other
> more pressing things.

I perfectly understand. Thanks for looking into it.

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 4/4] mmc: add sdhci-tegra driver for Tegra SoCs

[Mike Rapoport]

Hi Olof,

Overall looks good, just some nitpicking comments.

On 12/15/10 06:49, Olof Johansson wrote:
> SDHCI driver for Tegra. Pretty straight forward, a few pieces of
> functionality left to fill in but nothing that stops it from going
> upstream. Board enablement submitted separately.
> 
> This driver is based on an original one from:
> 
> Signed-off-by: Yvonne Yip <y@palm.com>
> 
> Misc fixes and suspend/resume by:
> 
> Signed-off-by: Gary King <gking@nvidia.com>
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> Signed-off-by: Dmitry Shmidt <dimitrysh@google.com>
> 
> GPIO write protect plumbing:
> 
> Signed-off-by: Rhyland Klein <rklein@nvidia.com>
> 
> Quirk rework and cleanups before submission:
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> ---
>  arch/arm/mach-tegra/include/mach/sdhci.h |   28 +++
>  drivers/mmc/host/Kconfig                 |   10 +
>  drivers/mmc/host/Makefile                |    1 +
>  drivers/mmc/host/sdhci-tegra.c           |  267 ++++++++++++++++++++++++++++++
>  4 files changed, 306 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-tegra/include/mach/sdhci.h
>  create mode 100644 drivers/mmc/host/sdhci-tegra.c
> 
> diff --git a/arch/arm/mach-tegra/include/mach/sdhci.h b/arch/arm/mach-tegra/include/mach/sdhci.h
> new file mode 100644
> index 0000000..8ca9539
> --- /dev/null
> +++ b/arch/arm/mach-tegra/include/mach/sdhci.h
> @@ -0,0 +1,28 @@
> +/*
> + * include/asm-arm/arch-tegra/include/mach/sdhci.h
> + *
> + * Copyright (C) 2009 Palm, Inc.
> + * Author: Yvonne Yip <y@palm.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +#ifndef __ASM_ARM_ARCH_TEGRA_SDHCI_H
> +#define __ASM_ARM_ARCH_TEGRA_SDHCI_H
> +
> +#include <linux/mmc/host.h>
> +
> +struct tegra_sdhci_platform_data {
> +	int cd_gpio;
> +	int wp_gpio;
> +	int power_gpio;

The power_gpio seems to be unused...

> +};
> +
> +#endif
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index d618e86..bd69bf9 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -189,6 +189,16 @@ config MMC_SDHCI_S3C_DMA
>  
>  	  YMMV.
>  
> +config MMC_SDHCI_TEGRA
> +	tristate "Tegra SD/MMC Controller Support"
> +	depends on ARCH_TEGRA && MMC_SDHCI
> +	select MMC_SDHCI_IO_ACCESSORS
> +	help
> +	  This selects the Tegra SD/MMC controller. If you have a Tegra
> +	  platform with SD or MMC devices, say Y or M here.
> +
> +	  If unsure, say N.
> +
>  config MMC_OMAP
>  	tristate "TI OMAP Multimedia Card Interface support"
>  	depends on ARCH_OMAP
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 7b645ff..a096f7f 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_MMC_SDHCI_PCI)	+= sdhci-pci.o
>  obj-$(CONFIG_MMC_SDHCI_PXA)	+= sdhci-pxa.o
>  obj-$(CONFIG_MMC_SDHCI_S3C)	+= sdhci-s3c.o
>  obj-$(CONFIG_MMC_SDHCI_SPEAR)	+= sdhci-spear.o
> +obj-$(CONFIG_MMC_SDHCI_TEGRA)	+= sdhci-tegra.o
>  obj-$(CONFIG_MMC_WBSD)		+= wbsd.o
>  obj-$(CONFIG_MMC_AU1X)		+= au1xmmc.o
>  obj-$(CONFIG_MMC_OMAP)		+= omap.o
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> new file mode 100644
> index 0000000..eb6b952
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -0,0 +1,267 @@
> +/*
> + * drivers/mmc/host/sdhci-tegra.c
> + *
> + * Copyright (C) 2009 Palm, Inc.
> + * Author: Yvonne Yip <y@palm.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/gpio.h>
> +#include <linux/mmc/card.h>
> +
> +#include <mach/sdhci.h>
> +
> +#include "sdhci.h"
> +
> +#define DRIVER_NAME    "sdhci-tegra"
> +
> +struct tegra_sdhci_host {
> +	struct sdhci_host *sdhci;
> +	struct clk *clk;
> +	int wp_gpio;
> +};
> +
> +
> +static u32 tegra_sdhci_readl(struct sdhci_host *host, int reg)
> +{
> +	u32 val;
> +
> +	if (unlikely(reg == SDHCI_PRESENT_STATE)) {
> +		/* Use wp_gpio here instead? */
> +		val = readl(host->ioaddr + reg);
> +		return val | SDHCI_WRITE_PROTECT;
> +	}
> +
> +	return readl(host->ioaddr + reg);
> +}
> +
> +static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg)
> +{
> +	if (unlikely(reg == SDHCI_HOST_VERSION)) {
> +		/* Erratum: Version register is invalid in HW. */
> +		return SDHCI_SPEC_200;
> +	}
> +
> +	return readw(host->ioaddr + reg);
> +}
> +
> +static void tegra_sdhci_writel(struct sdhci_host *host, u32 val, int reg)
> +{
> +	writel(val, host->ioaddr + reg);
> +	if (unlikely(reg == SDHCI_INT_ENABLE)) {
> +		/* Erratum: Must enable block gap interrupt detection */
> +		u8 gap_ctrl = readb(host->ioaddr + SDHCI_BLOCK_GAP_CONTROL);
> +		if (val & SDHCI_INT_CARD_INT)
> +			gap_ctrl |= 0x8;
> +		else
> +			gap_ctrl &= ~0x8;
> +		writeb(gap_ctrl, host->ioaddr + SDHCI_BLOCK_GAP_CONTROL);
> +	}
> +}
> +
> +static unsigned int tegra_sdhci_get_ro(struct sdhci_host *sdhci)
> +{
> +	struct tegra_sdhci_host *host = sdhci_priv(sdhci);
> +
> +	if (host->wp_gpio != -1)

if (gpio_is_valid(host->wp_gpio)) whould be better IMO.

> +		return gpio_get_value(host->wp_gpio);
> +
> +	return -1;
> +}
> +
> +static irqreturn_t carddetect_irq(int irq, void *data)
> +{
> +	struct sdhci_host *sdhost = (struct sdhci_host *)data;
> +
> +	tasklet_schedule(&sdhost->card_tasklet);
> +	return IRQ_HANDLED;
> +};
> +
> +static int tegra_sdhci_enable_dma(struct sdhci_host *host)
> +{
> +	return 0;
> +}
> +
> +static struct sdhci_ops tegra_sdhci_ops = {
> +	.enable_dma = tegra_sdhci_enable_dma,
> +	.get_ro     = tegra_sdhci_get_ro,
> +	.read_l     = tegra_sdhci_readl,
> +	.read_w     = tegra_sdhci_readw,
> +	.write_l    = tegra_sdhci_writel,
> +};
> +
> +static int __devinit tegra_sdhci_probe(struct platform_device *pdev)
> +{
> +	int rc;
> +	struct tegra_sdhci_platform_data *plat;
> +	struct sdhci_host *sdhci;
> +	struct tegra_sdhci_host *host;
> +	struct resource *res;
> +	int irq;
> +	void __iomem *ioaddr;
> +
> +	plat = pdev->dev.platform_data;
> +	if (plat == NULL)
> +		return -ENXIO;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (res == NULL)
> +		return -ENODEV;
> +
> +	irq = res->start;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (res == NULL)
> +		return -ENODEV;
> +
> +	ioaddr = ioremap(res->start, res->end - res->start);
> +
> +	sdhci = sdhci_alloc_host(&pdev->dev, sizeof(struct tegra_sdhci_host));
> +	if (IS_ERR(sdhci)) {
> +		rc = PTR_ERR(sdhci);
> +		goto err_unmap;
> +	}
> +
> +	host = sdhci_priv(sdhci);
> +	host->sdhci = sdhci;
> +
> +	host->clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(host->clk)) {
> +		rc = PTR_ERR(host->clk);
> +		goto err_free_host;
> +	}
> +
> +	rc = clk_enable(host->clk);
> +	if (rc != 0)
> +		goto err_clkput;
> +
> +	host->wp_gpio = plat->wp_gpio;
> +
> +	sdhci->hw_name = "tegra";
> +	sdhci->ops = &tegra_sdhci_ops;
> +	sdhci->irq = irq;
> +	sdhci->ioaddr = ioaddr;
> +	sdhci->quirks = SDHCI_QUIRK_BROKEN_TIMEOUT_VAL |
> +			SDHCI_QUIRK_SINGLE_POWER_WRITE |
> +			SDHCI_QUIRK_NO_HISPD_BIT |
> +			SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC |
> +			SDHCI_QUIRK_NO_SDIO_IRQ;
> +
> +	rc = sdhci_add_host(sdhci);
> +	if (rc)
> +		goto err_clk_disable;
> +
> +	platform_set_drvdata(pdev, host);
> +
> +	if (plat->cd_gpio != -1) {

if (gpio_is_valid(host->wp_gpio)) whould be better IMO.

> +		rc = request_irq(gpio_to_irq(plat->cd_gpio), carddetect_irq,
> +			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> +			mmc_hostname(sdhci->mmc), sdhci);
> +
> +		if (rc)
> +			goto err_remove_host;
> +	}

It seems to me that the tegra_sdhci_probe should handle gpio initialization,
rather then keep gpio_request etc calls in the board files.

> +	printk(KERN_INFO "sdhci%d: initialized irq %d ioaddr %p\n", pdev->id,
> +			sdhci->irq, sdhci->ioaddr);
> +

dev_info?

> +	return 0;
> +
> +err_remove_host:
> +	sdhci_remove_host(sdhci, 1);
> +err_clk_disable:
> +	clk_disable(host->clk);
> +err_clkput:
> +	clk_put(host->clk);
> +err_free_host:
> +	if (sdhci)
> +		sdhci_free_host(sdhci);
> +err_unmap:
> +	iounmap(sdhci->ioaddr);
> +
> +	return rc;
> +}
> +
> +static int tegra_sdhci_remove(struct platform_device *pdev)
> +{
> +	struct tegra_sdhci_host *host = platform_get_drvdata(pdev);
> +	if (host) {
> +		struct tegra_sdhci_platform_data *plat;
> +		plat = pdev->dev.platform_data;
> +
> +		sdhci_remove_host(host->sdhci, 0);
> +		sdhci_free_host(host->sdhci);
> +	}
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int tegra_sdhci_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct tegra_sdhci_host *host = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = sdhci_suspend_host(host->sdhci, state);
> +	if (ret)
> +		pr_err("%s: failed, error = %d\n", __func__, ret);
> +
> +	return ret;
> +}
> +
> +static int tegra_sdhci_resume(struct platform_device *pdev)
> +{
> +	struct tegra_sdhci_host *host = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = sdhci_resume_host(host->sdhci);
> +	if (ret)
> +		pr_err("%s: failed, error = %d\n", __func__, ret);
> +
> +	return ret;
> +}
> +#else
> +#define tegra_sdhci_suspend    NULL
> +#define tegra_sdhci_resume     NULL
> +#endif
> +
> +static struct platform_driver tegra_sdhci_driver = {
> +	.probe = tegra_sdhci_probe,
> +	.remove = tegra_sdhci_remove,
> +	.suspend = tegra_sdhci_suspend,
> +	.resume = tegra_sdhci_resume,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +static int __init tegra_sdhci_init(void)
> +{
> +	return platform_driver_register(&tegra_sdhci_driver);
> +}
> +
> +static void __exit tegra_sdhci_exit(void)
> +{
> +	platform_driver_unregister(&tegra_sdhci_driver);
> +}
> +
> +module_init(tegra_sdhci_init);
> +module_exit(tegra_sdhci_exit);
> +
> +MODULE_DESCRIPTION("Tegra SDHCI controller driver");
> +MODULE_LICENSE("GPL");


-- 
Sincerely yours,
Mike.

Re: [PATCH 4/4] mmc: add sdhci-tegra driver for Tegra SoCs

[Olof Johansson]

Hi,

On Wed, Dec 15, 2010 at 01:23:58PM +0200, Mike Rapoport wrote:
> Hi Olof,
> 
> Overall looks good, just some nitpicking comments.
> 
[...]

> > +struct tegra_sdhci_platform_data {
> > +	int cd_gpio;
> > +	int wp_gpio;
> > +	int power_gpio;
> 
> The power_gpio seems to be unused...

Yep, will remove.

> > +static unsigned int tegra_sdhci_get_ro(struct sdhci_host *sdhci)
> > +{
> > +	struct tegra_sdhci_host *host = sdhci_priv(sdhci);
> > +
> > +	if (host->wp_gpio != -1)
> 
> if (gpio_is_valid(host->wp_gpio)) whould be better IMO.
[...]
> > +	if (plat->cd_gpio != -1) {
> 
> if (gpio_is_valid(host->wp_gpio)) whould be better IMO.
> 

Fair enough (x2).

> > +		rc = request_irq(gpio_to_irq(plat->cd_gpio), carddetect_irq,
> > +			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> > +			mmc_hostname(sdhci->mmc), sdhci);
> > +
> > +		if (rc)
> > +			goto err_remove_host;
> > +	}
> 
> It seems to me that the tegra_sdhci_probe should handle gpio initialization,
> rather then keep gpio_request etc calls in the board files.

Yeah, I had been planning on moving that over but not at this pass. I can do that
though.

> > +	printk(KERN_INFO "sdhci%d: initialized irq %d ioaddr %p\n", pdev->id,
> > +			sdhci->irq, sdhci->ioaddr);
> > +
> 
> dev_info?

Yep, will change.



Thanks!


-Olof

Re: [PATCH 4/4] mmc: add sdhci-tegra driver for Tegra SoCs

[Pavan Kondeti]

On 12/15/2010 10:19 AM, Olof Johansson wrote:
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (res == NULL)
> +		return -ENODEV;
> +
> +	ioaddr = ioremap(res->start, res->end - res->start);
> +

Return value is not checked.

> +	sdhci = sdhci_alloc_host(&pdev->dev, sizeof(struct tegra_sdhci_host));
> +	if (IS_ERR(sdhci)) {
> +		rc = PTR_ERR(sdhci);
> +		goto err_unmap;
> +	}


-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 4/4] mmc: add sdhci-tegra driver for Tegra SoCs

[Olof Johansson]

On Wed, Dec 15, 2010 at 05:03:44PM +0530, Pavan Kondeti wrote:
> On 12/15/2010 10:19 AM, Olof Johansson wrote:
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (res == NULL)
> > +		return -ENODEV;
> > +
> > +	ioaddr = ioremap(res->start, res->end - res->start);
> > +
> 
> Return value is not checked.

Good catch. Thanks.


-Olof

Re: [PATCH 4/4] mmc: add sdhci-tegra driver for Tegra SoCs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 257 bytes --]

> > Return value is not checked.
> 
> Good catch. Thanks.

It is checked in sdhci-pltfm.c ;)

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]