[PATCH 0/3 v2] Add SDHCI driver for Tegra

[PATCH 0/3 v2] Add SDHCI driver for Tegra

[Olof Johansson]

This is the second version of the SDHCI driver for tegra. I moved it
over to a sdhci-pltfm variant, which turned out to be feasible -- at
least as long as the platform_data is never required by sdhci-pltfm and
can be used by the sub-driver.

I also did away with wone of the quirks, since based on current testing
it seems to no longer be required. If needed it can be reintroduced in
some way or other later on instead.

The one remaining quirk is of the kind that can be moved entirely into
the driver, and I will look at doing so when I get around to cleaning
up other quirks together with it. Until then, at least we have one more
bit left to allocate. :-)


Thanks,

-Olof

[PATCH 1/3] 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.1

[PATCH 2/3] 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 c0094c1..562aaea 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.1

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

[Chris Ball]

Hi Olof,

On Wed, Dec 22, 2010 at 02:01:10AM -0600, Olof Johansson wrote:
> 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>

Could we have some more detail here, please?  What are the symptoms of
running without this patch?  Should we apply it to the stable tree too?

> ---
>  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 c0094c1..562aaea 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;
>  	}
>  

Thanks,

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

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

[Olof Johansson]

Hi,

On Wed, Dec 22, 2010 at 12:08 AM, Chris Ball <cjb@laptop.org> wrote:
> Hi Olof,
>
> On Wed, Dec 22, 2010 at 02:01:10AM -0600, Olof Johansson wrote:
>> 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>
>
> Could we have some more detail here, please?  What are the symptoms of
> running without this patch?  Should we apply it to the stable tree too?

Hmm, good question. I hadn't gotten to the bottom of why this specific
change was needed in the past -- it's something the original driver
did that I just ended up inheriting behavior from. Thanks for asking.
:-)

From some further investigation, it seems like the underlying problem
is that the controller reports spurious timeout and crc errors, in
spite of the transactions completing successfully.

My main worry with this fix as it was is that it would disable
behavior on controllers where the error bits are set and handled
correctly. So, what I think I will do instead is that I will simply
mask those two errors from generating interrupts (i.e. masking the two
bits when writing the SDHCI_SIGNAL_ENABLE register). That seems to
result in the equivalent behavior, and I no longer see the spurious
interrupts and controller resets. In case of real timeouts and/or CRC
errors, the existing software timer should take care of eventually
unfreezing the controller if need be.

I'll repost the series with these changes after letting this version
sit out for review for a bit longer.


-Olof

[PATCH 3/3] 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 plugs in as a new variant of sdhci-pltfm, using the platform
data structure passed in to specify the GPIOs to use for card detect,
write protect and card power enablement.

Original driver (of which only the header file is left):
Signed-off-by: Yvonne Yip <y@palm.com>

The rest, which has been rewritten by now:
Signed-off-by: Olof Johansson <olof@lixom.net>
Cc: Wolfram Sang <w.sang@pengutronix.de>
Cc: Mike Rapoport <mike@compulab.co.il>

---

 arch/arm/mach-tegra/include/mach/sdhci.h |   28 ++++
 drivers/mmc/host/Kconfig                 |   10 ++
 drivers/mmc/host/Makefile                |    1 +
 drivers/mmc/host/sdhci-pltfm.c           |    3 +
 drivers/mmc/host/sdhci-pltfm.h           |    1 +
 drivers/mmc/host/sdhci-tegra.c           |  219 ++++++++++++++++++++++++++++++
 6 files changed, 262 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..02f6ef27
--- /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..25c6a2a 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -140,6 +140,16 @@ config MMC_SDHCI_ESDHC_IMX
 
 	  If unsure, say N.
 
+config MMC_SDHCI_TEGRA
+	tristate "SDHCI platform support for the Tegra SD/MMC Controller"
+	depends on MMC_SDHCI_PLTFM && ARCH_TEGRA
+	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_SDHCI_S3C
 	tristate "SDHCI support on Samsung S3C SoC"
 	depends on MMC_SDHCI && PLAT_SAMSUNG
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 7b645ff..fc8f8f0 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_MMC_SDHCI_PLTFM)			+= sdhci-platform.o
 sdhci-platform-y				:= sdhci-pltfm.o
 sdhci-platform-$(CONFIG_MMC_SDHCI_CNS3XXX)	+= sdhci-cns3xxx.o
 sdhci-platform-$(CONFIG_MMC_SDHCI_ESDHC_IMX)	+= sdhci-esdhc-imx.o
+sdhci-platform-$(CONFIG_MMC_SDHCI_TEGRA)	+= sdhci-tegra.o
 
 obj-$(CONFIG_MMC_SDHCI_OF)	+= sdhci-of.o
 sdhci-of-y				:= sdhci-of-core.o
diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 0502f89..d9e6e88 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -170,6 +170,9 @@ static const struct platform_device_id sdhci_pltfm_ids[] = {
 #ifdef CONFIG_MMC_SDHCI_ESDHC_IMX
 	{ "sdhci-esdhc-imx", (kernel_ulong_t)&sdhci_esdhc_imx_pdata },
 #endif
+#ifdef CONFIG_MMC_SDHCI_TEGRA
+	{ "sdhci-tegra", (kernel_ulong_t)&sdhci_tegra_pdata },
+#endif
 	{ },
 };
 MODULE_DEVICE_TABLE(platform, sdhci_pltfm_ids);
diff --git a/drivers/mmc/host/sdhci-pltfm.h b/drivers/mmc/host/sdhci-pltfm.h
index c1bfe48..6f631e3 100644
--- a/drivers/mmc/host/sdhci-pltfm.h
+++ b/drivers/mmc/host/sdhci-pltfm.h
@@ -22,5 +22,6 @@ struct sdhci_pltfm_host {
 
 extern struct sdhci_pltfm_data sdhci_cns3xxx_pdata;
 extern struct sdhci_pltfm_data sdhci_esdhc_imx_pdata;
+extern struct sdhci_pltfm_data sdhci_tegra_pdata;
 
 #endif /* _DRIVERS_MMC_SDHCI_PLTFM_H */
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
new file mode 100644
index 0000000..1b79663
--- /dev/null
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -0,0 +1,219 @@
+/*
+ * Copyright (C) 2010 The Chromium OS Authors <chromium-os-dev@chromium.org>
+ *
+ * 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/platform_device.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/mmc/card.h>
+
+#include <mach/gpio.h>
+#include <mach/sdhci.h>
+
+#include "sdhci.h"
+#include "sdhci-pltfm.h"
+
+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 platform_device *pdev = to_platform_device(mmc_dev(sdhci->mmc));
+	struct tegra_sdhci_platform_data *plat;
+
+	plat = pdev->dev.platform_data;
+
+	if (gpio_is_valid(plat->wp_gpio))
+		return gpio_get_value(plat->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_pltfm_init(struct sdhci_host *host, 
+				  struct sdhci_pltfm_data *pdata)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
+	struct tegra_sdhci_platform_data *plat;
+	struct clk *clk;
+	int rc;
+
+	plat = pdev->dev.platform_data;
+	if (plat == NULL) {
+		dev_err(mmc_dev(host->mmc), "missing platform data\n");
+		return -ENXIO;
+	}
+
+	if (gpio_is_valid(plat->power_gpio)) {
+		rc = gpio_request(plat->power_gpio, "sdhci_power");
+		if (rc) {
+			dev_err(mmc_dev(host->mmc), 
+				"failed to allocate power gpio\n");
+			goto out;
+		}
+		tegra_gpio_enable(plat->power_gpio);
+		gpio_direction_output(plat->power_gpio, 1);
+	}
+
+	if (gpio_is_valid(plat->cd_gpio)) {
+		rc = gpio_request(plat->cd_gpio, "sdhci_cd");
+		if (rc) {
+			dev_err(mmc_dev(host->mmc), 
+				"failed to allocate cd gpio\n");
+			goto out_power;
+		}
+		tegra_gpio_enable(plat->cd_gpio);
+
+		rc = request_irq(gpio_to_irq(plat->cd_gpio), carddetect_irq,
+				 IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+				 mmc_hostname(host->mmc), host);
+
+		if (rc)	{
+			dev_err(mmc_dev(host->mmc), "request irq error\n");
+			goto out_cd;
+		}
+
+	}
+
+	if (gpio_is_valid(plat->wp_gpio)) {
+		rc = gpio_request(plat->wp_gpio, "sdhci_wp");
+		if (rc) {
+			dev_err(mmc_dev(host->mmc), 
+				"failed to allocate wp gpio\n");
+			goto out_cd;
+		}
+		tegra_gpio_enable(plat->wp_gpio);
+	}
+
+	clk = clk_get(mmc_dev(host->mmc), NULL);
+	if (IS_ERR(clk)) {
+		dev_err(mmc_dev(host->mmc), "clk err\n");
+		rc = PTR_ERR(clk);
+		goto out_wp;
+	}
+	clk_enable(clk);
+	pltfm_host->clk = clk;
+
+	return 0;
+
+out_wp:
+	if (gpio_is_valid(plat->wp_gpio)) {
+		tegra_gpio_disable(plat->wp_gpio);
+		gpio_free(plat->wp_gpio);
+	}
+
+out_cd:
+	if (gpio_is_valid(plat->cd_gpio)) {
+		tegra_gpio_disable(plat->cd_gpio);
+		gpio_free(plat->cd_gpio);
+	}
+
+out_power:
+	if (gpio_is_valid(plat->power_gpio)) {
+		tegra_gpio_disable(plat->power_gpio);
+		gpio_free(plat->power_gpio);
+	}
+
+out:
+	return rc;
+}
+
+static void tegra_sdhci_pltfm_exit(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
+	struct tegra_sdhci_platform_data *plat;
+
+	plat = pdev->dev.platform_data;
+
+	if (gpio_is_valid(plat->wp_gpio)) {
+		tegra_gpio_disable(plat->wp_gpio);
+		gpio_free(plat->wp_gpio);
+	}
+
+	if (gpio_is_valid(plat->cd_gpio)) {
+		tegra_gpio_disable(plat->cd_gpio);
+		gpio_free(plat->cd_gpio);
+	}
+
+	if (gpio_is_valid(plat->power_gpio)) {
+		tegra_gpio_disable(plat->power_gpio);
+		gpio_free(plat->power_gpio);
+	}
+
+	clk_disable(pltfm_host->clk);
+	clk_put(pltfm_host->clk);
+}
+
+static struct sdhci_ops tegra_sdhci_ops = {
+	.get_ro     = tegra_sdhci_get_ro,
+	.read_l     = tegra_sdhci_readl,
+	.read_w     = tegra_sdhci_readw,
+	.write_l    = tegra_sdhci_writel,
+};
+
+struct sdhci_pltfm_data sdhci_tegra_pdata = {
+	.quirks = SDHCI_QUIRK_BROKEN_TIMEOUT_VAL |
+		  SDHCI_QUIRK_SINGLE_POWER_WRITE |
+		  SDHCI_QUIRK_NO_HISPD_BIT |
+		  SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC,
+	.ops  = &tegra_sdhci_ops,
+	.init = tegra_sdhci_pltfm_init,
+	.exit = tegra_sdhci_pltfm_exit,
+};
-- 
1.7.3.1

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

[Wolfram Sang]

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

Hi Olof,

looks good to me. Just some minor comments, but in general

Reviewed-by: Wolfram Sang <w.sang@pengutronix.de>

On Wed, Dec 22, 2010 at 02:01:11AM -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.

I'd think this can go below the "---" line?

...
> This driver plugs in as a new variant of sdhci-pltfm, using the platform
> data structure passed in to specify the GPIOs to use for card detect,
> write protect and card power enablement.
> 
> Original driver (of which only the header file is left):
> Signed-off-by: Yvonne Yip <y@palm.com>
> 
> The rest, which has been rewritten by now:
> Signed-off-by: Olof Johansson <olof@lixom.net>
> Cc: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Mike Rapoport <mike@compulab.co.il>
> +static unsigned int tegra_sdhci_get_ro(struct sdhci_host *sdhci)
> +{
> +	struct platform_device *pdev = to_platform_device(mmc_dev(sdhci->mmc));
> +	struct tegra_sdhci_platform_data *plat;
> +
> +	plat = pdev->dev.platform_data;
> +
> +	if (gpio_is_valid(plat->wp_gpio))
> +		return gpio_get_value(plat->wp_gpio);
> +
> +	return -1;

So, read_only is the default case? I would have gone for returning 0.
Well, safety vs. convenience...

...
> +		if (rc) {
> +			dev_err(mmc_dev(host->mmc), 

Trailing white space

> +				"failed to allocate power gpio\n");
> +			goto out;
> +		}
> +		tegra_gpio_enable(plat->power_gpio);
> +		gpio_direction_output(plat->power_gpio, 1);
> +	}
> +
> +	if (gpio_is_valid(plat->cd_gpio)) {
> +		rc = gpio_request(plat->cd_gpio, "sdhci_cd");
> +		if (rc) {
> +			dev_err(mmc_dev(host->mmc), 

ditto

> +				"failed to allocate cd gpio\n");
> +			goto out_power;
> +		}
> +		tegra_gpio_enable(plat->cd_gpio);
> +
> +		rc = request_irq(gpio_to_irq(plat->cd_gpio), carddetect_irq,
> +				 IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> +				 mmc_hostname(host->mmc), host);
> +
> +		if (rc)	{
> +			dev_err(mmc_dev(host->mmc), "request irq error\n");
> +			goto out_cd;
> +		}
> +
> +	}
> +
> +	if (gpio_is_valid(plat->wp_gpio)) {
> +		rc = gpio_request(plat->wp_gpio, "sdhci_wp");
> +		if (rc) {
> +			dev_err(mmc_dev(host->mmc), 

ditto

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 3/3] mmc: add sdhci-tegra driver for Tegra SoCs

[Olof Johansson]

Hi,

Thanks for the review. All good points (and I forgot to do a last run
through checkpatch, my bad).

All have been addressed in the next version (not yet posted).

-Olof

On Wed, Dec 22, 2010 at 8:22 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Hi Olof,
>
> looks good to me. Just some minor comments, but in general
>
> Reviewed-by: Wolfram Sang <w.sang@pengutronix.de>
>
> On Wed, Dec 22, 2010 at 02:01:11AM -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.
>
> I'd think this can go below the "---" line?
>
> ...
>> This driver plugs in as a new variant of sdhci-pltfm, using the platform
>> data structure passed in to specify the GPIOs to use for card detect,
>> write protect and card power enablement.
>>
>> Original driver (of which only the header file is left):
>> Signed-off-by: Yvonne Yip <y@palm.com>
>>
>> The rest, which has been rewritten by now:
>> Signed-off-by: Olof Johansson <olof@lixom.net>
>> Cc: Wolfram Sang <w.sang@pengutronix.de>
>> Cc: Mike Rapoport <mike@compulab.co.il>
>> +static unsigned int tegra_sdhci_get_ro(struct sdhci_host *sdhci)
>> +{
>> +     struct platform_device *pdev = to_platform_device(mmc_dev(sdhci->mmc));
>> +     struct tegra_sdhci_platform_data *plat;
>> +
>> +     plat = pdev->dev.platform_data;
>> +
>> +     if (gpio_is_valid(plat->wp_gpio))
>> +             return gpio_get_value(plat->wp_gpio);
>> +
>> +     return -1;
>
> So, read_only is the default case? I would have gone for returning 0.
> Well, safety vs. convenience...
>
> ...
>> +             if (rc) {
>> +                     dev_err(mmc_dev(host->mmc),
>
> Trailing white space
>
>> +                             "failed to allocate power gpio\n");
>> +                     goto out;
>> +             }
>> +             tegra_gpio_enable(plat->power_gpio);
>> +             gpio_direction_output(plat->power_gpio, 1);
>> +     }
>> +
>> +     if (gpio_is_valid(plat->cd_gpio)) {
>> +             rc = gpio_request(plat->cd_gpio, "sdhci_cd");
>> +             if (rc) {
>> +                     dev_err(mmc_dev(host->mmc),
>
> ditto
>
>> +                             "failed to allocate cd gpio\n");
>> +                     goto out_power;
>> +             }
>> +             tegra_gpio_enable(plat->cd_gpio);
>> +
>> +             rc = request_irq(gpio_to_irq(plat->cd_gpio), carddetect_irq,
>> +                              IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
>> +                              mmc_hostname(host->mmc), host);
>> +
>> +             if (rc) {
>> +                     dev_err(mmc_dev(host->mmc), "request irq error\n");
>> +                     goto out_cd;
>> +             }
>> +
>> +     }
>> +
>> +     if (gpio_is_valid(plat->wp_gpio)) {
>> +             rc = gpio_request(plat->wp_gpio, "sdhci_wp");
>> +             if (rc) {
>> +                     dev_err(mmc_dev(host->mmc),
>
> ditto
>
> Kind regards,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAk0SJccACgkQD27XaX1/VRtnbgCdH8+R7hjsq4YjJIjvQgtkwOrF
> GHwAoJBRrdEv6+5d2sxZ5J/AqWsTXLb4
> =RLqI
> -----END PGP SIGNATURE-----
>
>

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

[Mike Rapoport]

On 12/22/10 10:01, 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 plugs in as a new variant of sdhci-pltfm, using the platform
> data structure passed in to specify the GPIOs to use for card detect,
> write protect and card power enablement.
> 
> Original driver (of which only the header file is left):
> Signed-off-by: Yvonne Yip <y@palm.com>
> 
> The rest, which has been rewritten by now:
> Signed-off-by: Olof Johansson <olof@lixom.net>
> Cc: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Mike Rapoport <mike@compulab.co.il>

Only one comment below, otherwise feel free to add
Acked-by: Mike Rapoport <mike@compulab.co.il>

> ---
> 
>  arch/arm/mach-tegra/include/mach/sdhci.h |   28 ++++
>  drivers/mmc/host/Kconfig                 |   10 ++
>  drivers/mmc/host/Makefile                |    1 +
>  drivers/mmc/host/sdhci-pltfm.c           |    3 +
>  drivers/mmc/host/sdhci-pltfm.h           |    1 +
>  drivers/mmc/host/sdhci-tegra.c           |  219 ++++++++++++++++++++++++++++++
>  6 files changed, 262 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-tegra/include/mach/sdhci.h
>  create mode 100644 drivers/mmc/host/sdhci-tegra.c
> 

[ snip ]

> +	if (gpio_is_valid(plat->wp_gpio)) {
> +		rc = gpio_request(plat->wp_gpio, "sdhci_wp");
> +		if (rc) {
> +			dev_err(mmc_dev(host->mmc), 
> +				"failed to allocate wp gpio\n");
> +			goto out_cd;
> +		}
> +		tegra_gpio_enable(plat->wp_gpio);

gpio_direction_input?

> +	}
> +
> +	clk = clk_get(mmc_dev(host->mmc), NULL);
> +	if (IS_ERR(clk)) {
> +		dev_err(mmc_dev(host->mmc), "clk err\n");
> +		rc = PTR_ERR(clk);
> +		goto out_wp;
> +	}
> +	clk_enable(clk);
> +	pltfm_host->clk = clk;
> +
> +	return 0;
> +
> +out_wp:
> +	if (gpio_is_valid(plat->wp_gpio)) {
> +		tegra_gpio_disable(plat->wp_gpio);
> +		gpio_free(plat->wp_gpio);
> +	}
> +
> +out_cd:
> +	if (gpio_is_valid(plat->cd_gpio)) {
> +		tegra_gpio_disable(plat->cd_gpio);
> +		gpio_free(plat->cd_gpio);
> +	}
> +
> +out_power:
> +	if (gpio_is_valid(plat->power_gpio)) {
> +		tegra_gpio_disable(plat->power_gpio);
> +		gpio_free(plat->power_gpio);
> +	}
> +
> +out:
> +	return rc;
> +}
> +
> +static void tegra_sdhci_pltfm_exit(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
> +	struct tegra_sdhci_platform_data *plat;
> +
> +	plat = pdev->dev.platform_data;
> +
> +	if (gpio_is_valid(plat->wp_gpio)) {
> +		tegra_gpio_disable(plat->wp_gpio);
> +		gpio_free(plat->wp_gpio);
> +	}
> +
> +	if (gpio_is_valid(plat->cd_gpio)) {
> +		tegra_gpio_disable(plat->cd_gpio);
> +		gpio_free(plat->cd_gpio);
> +	}
> +
> +	if (gpio_is_valid(plat->power_gpio)) {
> +		tegra_gpio_disable(plat->power_gpio);
> +		gpio_free(plat->power_gpio);
> +	}
> +
> +	clk_disable(pltfm_host->clk);
> +	clk_put(pltfm_host->clk);
> +}
> +
> +static struct sdhci_ops tegra_sdhci_ops = {
> +	.get_ro     = tegra_sdhci_get_ro,
> +	.read_l     = tegra_sdhci_readl,
> +	.read_w     = tegra_sdhci_readw,
> +	.write_l    = tegra_sdhci_writel,
> +};
> +
> +struct sdhci_pltfm_data sdhci_tegra_pdata = {
> +	.quirks = SDHCI_QUIRK_BROKEN_TIMEOUT_VAL |
> +		  SDHCI_QUIRK_SINGLE_POWER_WRITE |
> +		  SDHCI_QUIRK_NO_HISPD_BIT |
> +		  SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC,
> +	.ops  = &tegra_sdhci_ops,
> +	.init = tegra_sdhci_pltfm_init,
> +	.exit = tegra_sdhci_pltfm_exit,
> +};


-- 
Sincerely yours,
Mike.

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

[Olof Johansson]

Hi,

On Wed, Dec 22, 2010 at 10:20 PM, Mike Rapoport <mike@compulab.co.il> wrote:

> Only one comment below, otherwise feel free to add
> Acked-by: Mike Rapoport <mike@compulab.co.il>

Thanks!

>> +     if (gpio_is_valid(plat->wp_gpio)) {
>> +             rc = gpio_request(plat->wp_gpio, "sdhci_wp");
>> +             if (rc) {
>> +                     dev_err(mmc_dev(host->mmc),
>> +                             "failed to allocate wp gpio\n");
>> +                     goto out_cd;
>> +             }
>> +             tegra_gpio_enable(plat->wp_gpio);
>
> gpio_direction_input?

Yep, for cd_gpio too.


-Olof

Re: [PATCH 0/3 v2] Add SDHCI driver for Tegra

[Chris Ball]

Hi Olof,

On Wed, Dec 22, 2010 at 02:01:08AM -0600, Olof Johansson wrote:
> This is the second version of the SDHCI driver for tegra. I moved it
> over to a sdhci-pltfm variant, which turned out to be feasible -- at
> least as long as the platform_data is never required by sdhci-pltfm and
> can be used by the sub-driver.
> 
> I also did away with wone of the quirks, since based on current testing
> it seems to no longer be required. If needed it can be reintroduced in
> some way or other later on instead.

At first glance, this looks *much* better as a -pltfm driver.  Thanks
very much for redoing it!

> The one remaining quirk is of the kind that can be moved entirely into
> the driver, and I will look at doing so when I get around to cleaning
> up other quirks together with it.

Yes, that'd be great.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 0/3 v2] Add SDHCI driver for Tegra

[Wolfram Sang]

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

Hi Olof,

> This is the second version of the SDHCI driver for tegra. I moved it
> over to a sdhci-pltfm variant, which turned out to be feasible -- at

Great.

> The one remaining quirk is of the kind that can be moved entirely into
> the driver, and I will look at doing so when I get around to cleaning
> up other quirks together with it. Until then, at least we have one more
> bit left to allocate. :-)

[BAZAAR MODE]

So, when are going to remove them? And how many? And what happens if
not? ;)

[/BAZAAR MODE]

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 0/3 v2] Add SDHCI driver for Tegra

[Olof Johansson]

Hi,

On Wed, Dec 22, 2010 at 8:05 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:

>> The one remaining quirk is of the kind that can be moved entirely into
>> the driver, and I will look at doing so when I get around to cleaning
>> up other quirks together with it. Until then, at least we have one more
>> bit left to allocate. :-)
>
> [BAZAAR MODE]
>
> So, when are going to remove them? And how many? And what happens if
> not? ;)
>
> [/BAZAAR MODE]


I'll probably take a go at a few of them over the holiday if I get bored.

If it doesn't happen, we'll run out of bits and someone will be forced
to clean up, or increase the data type. We already had that
discussion. :)


-Olof