[PATCH V3 1/1]MMC: add support of sdhci-pxa driver

[PATCH V3 1/1]MMC: add support of sdhci-pxa driver

[Chris Ball]

Hi,

On Thu, Oct 21, 2010 at 03:13:02PM +0100, Chris Ball wrote:
> On Mon, Oct 18, 2010 at 08:32:46AM -0400, zhangfei gao wrote:
> > Update with comments from Matt and Eric.
> > Test with sd and emmc.
> > 
> > >From e5dd554ed4d3488a83d9a4888d68d1d85482f747 Mon Sep 17 00:00:00 2001
> > From: Zhangfei Gao <zhangfei.gao@marvell.com>
> > Date: Mon, 20 Sep 2010 10:51:28 -0400
> > Subject: [PATCH] mmc: add support of sdhci-pxa driver
> > 
> > 	Support Marvell PXA168/PXA910/MMP2 SD Host Controller
> > 
> > Signed-off-by: Zhangfei Gao <zhangfei.gao@marvell.com>
> > ---
> >  arch/arm/plat-pxa/include/plat/sdhci.h |   32 ++++
> >  drivers/mmc/host/Kconfig               |   12 ++
> >  drivers/mmc/host/Makefile              |    1 +
> >  drivers/mmc/host/sdhci-pxa.c           |  259 ++++++++++++++++++++++++++++++++
> >  4 files changed, 304 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/plat-pxa/include/plat/sdhci.h
> >  create mode 100644 drivers/mmc/host/sdhci-pxa.c
> 
> I'll take the drivers/mmc/ hunks, but you should send the plat-pxa/
> patch through the ARM tree.

Oh.  If we do that then the driver won't compile until everything's
together, because it #includes <plat/sdhci.h>.  Haojian/Eric, what do
you prefer here?  Should I take the arch/arm/ hunk via the MMC tree?
(If so, please provide ACKs.)

Thanks,

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

[PATCH V3 1/1]MMC: add support of sdhci-pxa driver

[Marek Vasut]

Dne ?t 21. ??jna 2010 16:52:49 Chris Ball napsal(a):
> Hi,
> 
> On Thu, Oct 21, 2010 at 03:13:02PM +0100, Chris Ball wrote:
> > On Mon, Oct 18, 2010 at 08:32:46AM -0400, zhangfei gao wrote:
> > > Update with comments from Matt and Eric.
> > > Test with sd and emmc.
> > > 
> > > >From e5dd554ed4d3488a83d9a4888d68d1d85482f747 Mon Sep 17 00:00:00 2001
> > > 
> > > From: Zhangfei Gao <zhangfei.gao@marvell.com>
> > > Date: Mon, 20 Sep 2010 10:51:28 -0400
> > > Subject: [PATCH] mmc: add support of sdhci-pxa driver
> > > 
> > > 	Support Marvell PXA168/PXA910/MMP2 SD Host Controller
> > > 
> > > Signed-off-by: Zhangfei Gao <zhangfei.gao@marvell.com>
> > > ---
> > > 
> > >  arch/arm/plat-pxa/include/plat/sdhci.h |   32 ++++
> > >  drivers/mmc/host/Kconfig               |   12 ++
> > >  drivers/mmc/host/Makefile              |    1 +
> > >  drivers/mmc/host/sdhci-pxa.c           |  259
> > >  ++++++++++++++++++++++++++++++++ 4 files changed, 304 insertions(+),
> > >  0 deletions(-)
> > >  create mode 100644 arch/arm/plat-pxa/include/plat/sdhci.h
> > >  create mode 100644 drivers/mmc/host/sdhci-pxa.c
> > 
> > I'll take the drivers/mmc/ hunks, but you should send the plat-pxa/
> > patch through the ARM tree.
> 
> Oh.  If we do that then the driver won't compile until everything's
> together, because it #includes <plat/sdhci.h>.  Haojian/Eric, what do
> you prefer here?  Should I take the arch/arm/ hunk via the MMC tree?
> (If so, please provide ACKs.)
> 
> Thanks,

Hi, where can I find the whole driver so I can take a look? It didn't crash into 
linux-arm-kernel for some reason.

Thanks

[PATCH V3 1/1]MMC: add support of sdhci-pxa driver

[Chris Ball]

Hi Marek,

On Fri, Oct 22, 2010 at 02:20:10AM +0200, Marek Vasut wrote:
> > > I'll take the drivers/mmc/ hunks, but you should send the plat-pxa/
> > > patch through the ARM tree.
> > 
> > Oh.  If we do that then the driver won't compile until everything's
> > together, because it #includes <plat/sdhci.h>.  Haojian/Eric, what do
> > you prefer here?  Should I take the arch/arm/ hunk via the MMC tree?
> > (If so, please provide ACKs.)
> 
> Hi, where can I find the whole driver so I can take a look? It didn't 
> crash into linux-arm-kernel for some reason.

Here it is:

http://thread.gmane.org/gmane.linux.kernel.mmc/4190

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

[PATCH V3 1/1]MMC: add support of sdhci-pxa driver

[Haojian Zhuang]

On Thu, Oct 21, 2010 at 10:52 PM, Chris Ball <cjb@laptop.org> wrote:
> Hi,
>
> On Thu, Oct 21, 2010 at 03:13:02PM +0100, Chris Ball wrote:
>> On Mon, Oct 18, 2010 at 08:32:46AM -0400, zhangfei gao wrote:
>> > Update with comments from Matt and Eric.
>> > Test with sd and emmc.
>> >
>> > >From e5dd554ed4d3488a83d9a4888d68d1d85482f747 Mon Sep 17 00:00:00 2001
>> > From: Zhangfei Gao <zhangfei.gao@marvell.com>
>> > Date: Mon, 20 Sep 2010 10:51:28 -0400
>> > Subject: [PATCH] mmc: add support of sdhci-pxa driver
>> >
>> > ? ? Support Marvell PXA168/PXA910/MMP2 SD Host Controller
>> >
>> > Signed-off-by: Zhangfei Gao <zhangfei.gao@marvell.com>
>> > ---
>> > ?arch/arm/plat-pxa/include/plat/sdhci.h | ? 32 ++++
>> > ?drivers/mmc/host/Kconfig ? ? ? ? ? ? ? | ? 12 ++
>> > ?drivers/mmc/host/Makefile ? ? ? ? ? ? ?| ? ?1 +
>> > ?drivers/mmc/host/sdhci-pxa.c ? ? ? ? ? | ?259 ++++++++++++++++++++++++++++++++
>> > ?4 files changed, 304 insertions(+), 0 deletions(-)
>> > ?create mode 100644 arch/arm/plat-pxa/include/plat/sdhci.h
>> > ?create mode 100644 drivers/mmc/host/sdhci-pxa.c
>>
>> I'll take the drivers/mmc/ hunks, but you should send the plat-pxa/
>> patch through the ARM tree.
>
> Oh. ?If we do that then the driver won't compile until everything's
> together, because it #includes <plat/sdhci.h>. ?Haojian/Eric, what do
> you prefer here? ?Should I take the arch/arm/ hunk via the MMC tree?
> (If so, please provide ACKs.)
>

Acked-by: Haojian Zhuang.

I'm fine on this. Please merge it via the MMC tree.


Thanks
Haojian

[PATCH V3 1/1]MMC: add support of sdhci-pxa driver

[Chris Ball]

Hi Russell,

On Fri, Oct 22, 2010 at 04:44:07PM +0800, Haojian Zhuang wrote:
> I'm fine on this. Please merge it via the MMC tree.

I'm planning on merging this patch via the MMC tree, please let me know
if you object.  Thanks.


From: Zhangfei Gao <zhangfei.gao@marvell.com>
Date: Mon, 20 Sep 2010 10:51:28 -0400
Subject: [PATCH] mmc: add new sdhci-pxa driver for Marvell SoCs

Support Marvell PXA168/PXA910/MMP2 SD Host Controller.

Signed-off-by: Zhangfei Gao <zhangfei.gao@marvell.com>
Acked-by: Haojian Zhuang <haojian.zhuang@marvell.com>
Signed-off-by: Chris Ball <cjb@laptop.org>
---
 arch/arm/plat-pxa/include/plat/sdhci.h |   32 ++++
 drivers/mmc/host/Kconfig               |   12 ++
 drivers/mmc/host/Makefile              |    1 +
 drivers/mmc/host/sdhci-pxa.c           |  258 ++++++++++++++++++++++++++++++++
 4 files changed, 303 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/plat-pxa/include/plat/sdhci.h
 create mode 100644 drivers/mmc/host/sdhci-pxa.c

diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h b/arch/arm/plat-pxa/include/plat/sdhci.h
new file mode 100644
index 0000000..3e3c728
--- /dev/null
+++ b/arch/arm/plat-pxa/include/plat/sdhci.h
@@ -0,0 +1,32 @@
+/* linux/arch/arm/plat-pxa/include/plat/sdhci.h
+ *
+ * Copyright 2010 Marvell
+ *	Zhangfei Gao <zhangfei.gao@marvell.com>
+ *
+ * PXA Platform - SDHCI platform data definitions
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#ifndef __PLAT_PXA_SDHCI_H
+#define __PLAT_PXA_SDHCI_H
+
+/* pxa specific flag */
+/* Require clock free running */
+#define PXA_FLAG_DISABLE_CLOCK_GATING (1<<0)
+
+/**
+ * struct pxa_sdhci_platdata() - Platform device data for PXA SDHCI
+ * @max_speed: the maximum speed supported
+ * @quirks: quirks of specific device
+ * @flags: flags for platform requirement
+*/
+struct sdhci_pxa_platdata {
+	unsigned int	max_speed;
+	unsigned int	quirks;
+	unsigned int	flags;
+};
+
+#endif /* __PLAT_PXA_SDHCI_H */
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index c9c2520..40b0fb9 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -155,6 +155,18 @@ config MMC_SDHCI_S3C
 
 	  If unsure, say N.
 
+config MMC_SDHCI_PXA
+	tristate "Marvell PXA168/PXA910/MMP2 SD Host Controller support"
+	depends on ARCH_PXA || ARCH_MMP
+	select MMC_SDHCI
+	select MMC_SDHCI_IO_ACCESSORS
+	help
+	  This selects the Marvell(R) PXA168/PXA910/MMP2 SD Host Controller.
+	  If you have a PXA168/PXA910/MMP2 platform with SD Host Controller
+	  and a card slot, say Y or M here.
+
+	  If unsure, say N.
+
 config MMC_SDHCI_SPEAR
 	tristate "SDHCI support on ST SPEAr platform"
 	depends on MMC_SDHCI && PLAT_SPEAR
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 6c4ac67..7b645ff 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_MMC_IMX)		+= imxmmc.o
 obj-$(CONFIG_MMC_MXC)		+= mxcmmc.o
 obj-$(CONFIG_MMC_SDHCI)		+= sdhci.o
 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_WBSD)		+= wbsd.o
diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c
new file mode 100644
index 0000000..aeba3e3
--- /dev/null
+++ b/drivers/mmc/host/sdhci-pxa.c
@@ -0,0 +1,258 @@
+/* linux/drivers/mmc/host/sdhci-pxa.c
+ *
+ * Copyright (C) 2010 Marvell International Ltd.
+ *		Zhangfei Gao <zhangfei.gao@marvell.com>
+ *		Kevin Wang <dwang4@marvell.com>
+ *		Mingwei Wang <mwwang@marvell.com>
+ *		Philip Rakity <prakity@marvell.com>
+ *		Mark Brown <markb@marvell.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/* Supports:
+ * SDHCI support for MMP2/PXA910/PXA168
+ *
+ * Refer to sdhci-s3c.c.
+ */
+
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/mmc/host.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <plat/sdhci.h>
+#include "sdhci.h"
+
+#define DRIVER_NAME	"sdhci-pxa"
+
+#define SD_FIFO_PARAM		0x104
+#define DIS_PAD_SD_CLK_GATE	0x400
+
+struct sdhci_pxa {
+	struct sdhci_host		*host;
+	struct sdhci_pxa_platdata	*pdata;
+	struct clk			*clk;
+	struct resource			*res;
+
+	u8 clk_enable;
+};
+
+/*****************************************************************************\
+ *                                                                           *
+ * SDHCI core callbacks                                                      *
+ *                                                                           *
+\*****************************************************************************/
+static void set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	struct sdhci_pxa *pxa = sdhci_priv(host);
+	u32 tmp = 0;
+
+	if (clock == 0) {
+		if (pxa->clk_enable) {
+			clk_disable(pxa->clk);
+			pxa->clk_enable = 0;
+		}
+	} else {
+		if (0 == pxa->clk_enable) {
+			if (pxa->pdata->flags & PXA_FLAG_DISABLE_CLOCK_GATING) {
+				tmp = readl(host->ioaddr + SD_FIFO_PARAM);
+				tmp |= DIS_PAD_SD_CLK_GATE;
+				writel(tmp, host->ioaddr + SD_FIFO_PARAM);
+			}
+			clk_enable(pxa->clk);
+			pxa->clk_enable = 1;
+		}
+	}
+}
+
+static struct sdhci_ops sdhci_pxa_ops = {
+	.set_clock = set_clock,
+};
+
+/*****************************************************************************\
+ *                                                                           *
+ * Device probing/removal                                                    *
+ *                                                                           *
+\*****************************************************************************/
+
+static int __devinit sdhci_pxa_probe(struct platform_device *pdev)
+{
+	struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
+	struct device *dev = &pdev->dev;
+	struct sdhci_host *host = NULL;
+	struct resource *iomem = NULL;
+	struct sdhci_pxa *pxa = NULL;
+	int ret, irq;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "no irq specified\n");
+		return irq;
+	}
+
+	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!iomem) {
+		dev_err(dev, "no memory specified\n");
+		return -ENOENT;
+	}
+
+	host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pxa));
+	if (IS_ERR(host)) {
+		dev_err(dev, "failed to alloc host\n");
+		return PTR_ERR(host);
+	}
+
+	pxa = sdhci_priv(host);
+	pxa->host = host;
+	pxa->pdata = pdata;
+	pxa->clk_enable = 0;
+
+	pxa->clk = clk_get(dev, "PXA-SDHCLK");
+	if (IS_ERR(pxa->clk)) {
+		dev_err(dev, "failed to get io clock\n");
+		ret = PTR_ERR(pxa->clk);
+		goto out;
+	}
+
+	pxa->res = request_mem_region(iomem->start, resource_size(iomem),
+				      mmc_hostname(host->mmc));
+	if (!pxa->res) {
+		dev_err(&pdev->dev, "cannot request region\n");
+		ret = -EBUSY;
+		goto out;
+	}
+
+	host->ioaddr = ioremap(iomem->start, resource_size(iomem));
+	if (!host->ioaddr) {
+		dev_err(&pdev->dev, "failed to remap registers\n");
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	host->hw_name = "MMC";
+	host->ops = &sdhci_pxa_ops;
+	host->irq = irq;
+	host->quirks = SDHCI_QUIRK_BROKEN_ADMA | SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
+
+	if (pdata->quirks)
+		host->quirks |= pdata->quirks;
+
+	ret = sdhci_add_host(host);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add host\n");
+		goto out;
+	}
+
+	if (pxa->pdata->max_speed)
+		host->mmc->f_max = pxa->pdata->max_speed;
+
+	platform_set_drvdata(pdev, host);
+
+	return 0;
+out:
+	if (host) {
+		clk_put(pxa->clk);
+		if (host->ioaddr)
+			iounmap(host->ioaddr);
+		if (pxa->res)
+			release_mem_region(pxa->res->start,
+					   resource_size(pxa->res));
+		sdhci_free_host(host);
+	}
+
+	return ret;
+}
+
+static int __devexit sdhci_pxa_remove(struct platform_device *pdev)
+{
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+	struct sdhci_pxa *pxa = sdhci_priv(host);
+	int dead = 0;
+	u32 scratch;
+
+	if (host) {
+		scratch = readl(host->ioaddr + SDHCI_INT_STATUS);
+		if (scratch == (u32)-1)
+			dead = 1;
+
+		sdhci_remove_host(host, dead);
+
+		if (host->ioaddr)
+			iounmap(host->ioaddr);
+		if (pxa->res)
+			release_mem_region(pxa->res->start,
+					   resource_size(pxa->res));
+		if (pxa->clk_enable) {
+			clk_disable(pxa->clk);
+			pxa->clk_enable = 0;
+		}
+		clk_put(pxa->clk);
+
+		sdhci_free_host(host);
+		platform_set_drvdata(pdev, NULL);
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int sdhci_pxa_suspend(struct platform_device *dev)
+{
+	struct sdhci_host *host = platform_get_drvdata(to_platform_device(dev));
+
+	return sdhci_suspend_host(host, state);
+}
+
+static int sdhci_pxa_resume(struct platform_device *dev)
+{
+	struct sdhci_host *host = platform_get_drvdata(to_platform_device(dev));
+
+	return sdhci_resume_host(host);
+}
+#else
+#define sdhci_pxa_suspend	NULL
+#define sdhci_pxa_resume	NULL
+#endif
+
+static const struct dev_pm_ops sdhci_pxa_pm_ops = {
+	.suspend	= sdhci_pxa_suspend,
+	.resume		= sdhci_pxa_resume,
+};
+
+
+static struct platform_driver sdhci_pxa_driver = {
+	.probe		= sdhci_pxa_probe,
+	.remove		= __devexit_p(sdhci_pxa_remove),
+	.driver		= {
+		.name	= DRIVER_NAME,
+		.owner	= THIS_MODULE,
+		.pm = &sdhci_pxa_pm_ops,
+	},
+};
+
+/*****************************************************************************\
+ *                                                                           *
+ * Driver init/exit                                                          *
+ *                                                                           *
+\*****************************************************************************/
+
+static int __init sdhci_pxa_init(void)
+{
+	return platform_driver_register(&sdhci_pxa_driver);
+}
+
+static void __exit sdhci_pxa_exit(void)
+{
+	platform_driver_unregister(&sdhci_pxa_driver);
+}
+
+module_init(sdhci_pxa_init);
+module_exit(sdhci_pxa_exit);
+
+MODULE_DESCRIPTION("SDH controller driver for PXA168/PXA910/MMP2");
+MODULE_AUTHOR("Zhangfei Gao <zhangfei.gao@marvell.com>");
+MODULE_LICENSE("GPL v2");
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH V3 1/1]MMC: add support of sdhci-pxa driver

[Chris Ball]

Hi,

On Fri, Oct 22, 2010 at 10:58:14AM +0100, Chris Ball wrote:
[...]
> +#ifdef CONFIG_PM
> +static int sdhci_pxa_suspend(struct platform_device *dev)
> +{
> +	struct sdhci_host *host = platform_get_drvdata(to_platform_device(dev));
> +
> +	return sdhci_suspend_host(host, state);
> +}
> +
> +static int sdhci_pxa_resume(struct platform_device *dev)
> +{

These prototypes are not correct, leading to:

  CC [M]  drivers/mmc/host/sdhci-pxa.o
drivers/mmc/host/sdhci-pxa.c: In function ?sdhci_pxa_suspend?:
drivers/mmc/host/sdhci-pxa.c:205: warning: initialization from incompatible pointer type
drivers/mmc/host/sdhci-pxa.c:207: error: ?state? undeclared (first use in this function)
drivers/mmc/host/sdhci-pxa.c:207: error: (Each undeclared identifier is reported only once
drivers/mmc/host/sdhci-pxa.c:207: error: for each function it appears in.)
drivers/mmc/host/sdhci-pxa.c: In function ?sdhci_pxa_resume?:
drivers/mmc/host/sdhci-pxa.c:212: warning: initialization from incompatible pointer type
drivers/mmc/host/sdhci-pxa.c: At top level:
drivers/mmc/host/sdhci-pxa.c:222: warning: initialization from incompatible pointer type
drivers/mmc/host/sdhci-pxa.c:223: warning: initialization from incompatible pointer type

when compiled with CONFIG_PM=y.

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

[PATCH V3 1/1]MMC: add support of sdhci-pxa driver

[zhangfei gao]

On Fri, Oct 22, 2010 at 7:04 AM, Chris Ball <cjb@laptop.org> wrote:
> Hi,
>
> On Fri, Oct 22, 2010 at 10:58:14AM +0100, Chris Ball wrote:
> [...]
>> +#ifdef CONFIG_PM
>> +static int sdhci_pxa_suspend(struct platform_device *dev)
>> +{
>> + ? ? struct sdhci_host *host = platform_get_drvdata(to_platform_device(dev));
>> +
>> + ? ? return sdhci_suspend_host(host, state);
>> +}
>> +
>> +static int sdhci_pxa_resume(struct platform_device *dev)
>> +{
>
> These prototypes are not correct, leading to:
>
> ?CC [M] ?drivers/mmc/host/sdhci-pxa.o
> drivers/mmc/host/sdhci-pxa.c: In function ?sdhci_pxa_suspend?:
> drivers/mmc/host/sdhci-pxa.c:205: warning: initialization from incompatible pointer type
> drivers/mmc/host/sdhci-pxa.c:207: error: ?state? undeclared (first use in this function)
> drivers/mmc/host/sdhci-pxa.c:207: error: (Each undeclared identifier is reported only once
> drivers/mmc/host/sdhci-pxa.c:207: error: for each function it appears in.)
> drivers/mmc/host/sdhci-pxa.c: In function ?sdhci_pxa_resume?:
> drivers/mmc/host/sdhci-pxa.c:212: warning: initialization from incompatible pointer type
> drivers/mmc/host/sdhci-pxa.c: At top level:
> drivers/mmc/host/sdhci-pxa.c:222: warning: initialization from incompatible pointer type
> drivers/mmc/host/sdhci-pxa.c:223: warning: initialization from incompatible pointer type
>
> when compiled with CONFIG_PM=y.
>
> --
> Chris Ball ? <cjb@laptop.org> ? <http://printf.net/>
> One Laptop Per Child
>

Sorry, forgot open CONFIG_PM.
Updated patch, thanks

[PATCH V3 1/1]MMC: add support of sdhci-pxa driver

[Marek Vasut]

Dne P? 22. ??jna 2010 16:09:03 zhangfei gao napsal(a):
> On Fri, Oct 22, 2010 at 7:04 AM, Chris Ball <cjb@laptop.org> wrote:
> > Hi,
> > 
> > On Fri, Oct 22, 2010 at 10:58:14AM +0100, Chris Ball wrote:
> > [...]
> > 
> >> +#ifdef CONFIG_PM
> >> +static int sdhci_pxa_suspend(struct platform_device *dev)
> >> +{
> >> +     struct sdhci_host *host =
> >> platform_get_drvdata(to_platform_device(dev)); +
> >> +     return sdhci_suspend_host(host, state);
> >> +}
> >> +
> >> +static int sdhci_pxa_resume(struct platform_device *dev)
> >> +{
> > 
> > These prototypes are not correct, leading to:
> > 
> >  CC [M]  drivers/mmc/host/sdhci-pxa.o
> > drivers/mmc/host/sdhci-pxa.c: In function ?sdhci_pxa_suspend?:
> > drivers/mmc/host/sdhci-pxa.c:205: warning: initialization from
> > incompatible pointer type drivers/mmc/host/sdhci-pxa.c:207: error:
> > ?state? undeclared (first use in this function)
> > drivers/mmc/host/sdhci-pxa.c:207: error: (Each undeclared identifier is
> > reported only once drivers/mmc/host/sdhci-pxa.c:207: error: for each
> > function it appears in.) drivers/mmc/host/sdhci-pxa.c: In function
> > ?sdhci_pxa_resume?:
> > drivers/mmc/host/sdhci-pxa.c:212: warning: initialization from
> > incompatible pointer type drivers/mmc/host/sdhci-pxa.c: At top level:
> > drivers/mmc/host/sdhci-pxa.c:222: warning: initialization from
> > incompatible pointer type drivers/mmc/host/sdhci-pxa.c:223: warning:
> > initialization from incompatible pointer type
> > 
> > when compiled with CONFIG_PM=y.
> > 
> > --
> > Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> > One Laptop Per Child
> 
> Sorry, forgot open CONFIG_PM.
> Updated patch, thanks
> 
> From 88e7f028433fe87b211bf3d75b54261979d0d176 Mon Sep 17 00:00:00 2001
> From: Zhangfei Gao <zhangfei.gao@marvell.com>
> Date: Mon, 20 Sep 2010 10:51:28 -0400
> Subject: [PATCH] mmc: add support of sdhci-pxa driver
> 
> 	Support Marvell PXA168/PXA910/MMP2 SD Host Controller
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@marvell.com>
> Acked-by: Haojian Zhuang <haojian.zhuang@marvell.com>
> ---
>  arch/arm/plat-pxa/include/plat/sdhci.h |   32 ++++
>  drivers/mmc/host/Kconfig               |   12 ++
>  drivers/mmc/host/Makefile              |    1 +
>  drivers/mmc/host/sdhci-pxa.c           |  254
> ++++++++++++++++++++++++++++++++ 4 files changed, 299 insertions(+), 0
> deletions(-)
>  create mode 100644 arch/arm/plat-pxa/include/plat/sdhci.h
>  create mode 100644 drivers/mmc/host/sdhci-pxa.c
> 
> diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h
> b/arch/arm/plat-pxa/include/plat/sdhci.h
> new file mode 100644
> index 0000000..38e86ad
> --- /dev/null
> +++ b/arch/arm/plat-pxa/include/plat/sdhci.h
> @@ -0,0 +1,32 @@
> +/* linux/arch/arm/plat-pxa/include/plat/sdhci.h
> + *
> + * Copyright 2010 Marvell
> + *	Zhangfei Gao <zhangfei.gao@marvell.com>
> + *
> + * PXA Platform - SDHCI platform data definitions
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#ifndef __PLAT_PXA_SDHCI_H
> +#define __PLAT_PXA_SDHCI_H
> +
> +/* pxa specific flag */
> +/* Require clock free running */
> +#define	PXA_FLAG_DISABLE_CLOCK_GATING (1<<0)
> +
> +/**
> + * struct pxa_sdhci_platdata() - Platform device data for PXA SDHCI
> + * @max_speed: The maximum speed supported.
> + * @quirks: quirks of specific device
> + * @flags: flags for platfrom requirement
> +*/
> +struct sdhci_pxa_platdata {
> +	unsigned int	max_speed;
> +	unsigned int	quirks;
> +	unsigned int	flags;
> +};
> +
> +#endif /* __PLAT_PXA_SDHCI_H */
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index c9c2520..c387402 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -155,6 +155,18 @@ config MMC_SDHCI_S3C
> 
>  	  If unsure, say N.
> 
> +config MMC_SDHCI_PXA
> +	tristate "Marvell PXA168/PXA910/MMP2 SD Host Controller support"
> +	depends on ARCH_PXA || ARCH_MMP
> +	select MMC_SDHCI
> +	select MMC_SDHCI_IO_ACCESSORS
> +	help
> +	  This selects the Marvell(R) PXA168/PXA910/MMP2 SD Host Controller.
> +	  If you have a PXA168/PXA910/MMP2 platform with SD Host Controller and a
> +	  card slot,say Y or M here.
> +
> +	  If unsure, say N.
> +
>  config MMC_SDHCI_SPEAR
>  	tristate "SDHCI support on ST SPEAr platform"
>  	depends on MMC_SDHCI && PLAT_SPEAR
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 6c4ac67..7b645ff 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_MMC_IMX)		+= imxmmc.o
>  obj-$(CONFIG_MMC_MXC)		+= mxcmmc.o
>  obj-$(CONFIG_MMC_SDHCI)		+= sdhci.o
>  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_WBSD)		+= wbsd.o
> diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c
> new file mode 100644
> index 0000000..abf208c
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-pxa.c
> @@ -0,0 +1,254 @@
> +/* linux/drivers/mmc/host/sdhci-pxa.c
> + *
> + * Copyright (C) 2010 Marvell International Ltd.
> + *		Zhangfei Gao <zhangfei.gao@marvell.com>
> + *		Kevin Wang <dwang4@marvell.com>
> + *		Mingwei Wang <mwwang@marvell.com>
> + *		Philip Rakity <prakity@marvell.com>
> + *		Mark Brown <markb@marvell.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/* Supports:
> + * SDHCI support for MMP2/PXA910/PXA168
> + *
> + * Refer sdhci-s3c.c
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/mmc/host.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <plat/sdhci.h>
> +#include "sdhci.h"
> +
> +#define DRIVER_NAME	"sdhci-pxa"
> +
> +#define SD_FIFO_PARAM		0x104
> +#define DIS_PAD_SD_CLK_GATE	0x400
> +
> +struct sdhci_pxa {
> +	struct sdhci_host		*host;
> +	struct sdhci_pxa_platdata	*pdata;
> +	struct clk			*clk;
> +	struct resource			*res;
> +
> +	u8 clk_enable;
> +};
> +
> +/*************************************************************************
> ****\ + *                                                                  
>         * + * SDHCI core callbacks                                        
>              * + *                                                        
>                   *
> +\************************************************************************
> *****/ +static void set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> +	struct sdhci_pxa *pxa = sdhci_priv(host);
> +	u32 tmp = 0;
> +
> +	if (clock == 0) {
> +		if (pxa->clk_enable) {
> +			clk_disable(pxa->clk);
> +			pxa->clk_enable = 0;
> +		}
> +	} else {
> +		if (0 == pxa->clk_enable) {
> +			if (pxa->pdata->flags
> +					& PXA_FLAG_DISABLE_CLOCK_GATING) {
> +				tmp = readl(host->ioaddr + SD_FIFO_PARAM);
> +				tmp |= DIS_PAD_SD_CLK_GATE;
> +				writel(tmp, host->ioaddr + SD_FIFO_PARAM);
> +			}
> +			clk_enable(pxa->clk);
> +			pxa->clk_enable = 1;
> +		}
> +	}
> +}

Doesn't the clock framework have something like clk_is_already_enabled() 
function ?
> +
> +static struct sdhci_ops sdhci_pxa_ops = {
> +	.set_clock = set_clock,
> +};
> +
> +/*************************************************************************
> ****\ + *                                                                  
>         * + * Device probing/removal                                      
>              * + *                                                        
>                   *
> +\************************************************************************
> *****/ +
> +static int __devinit sdhci_pxa_probe(struct platform_device *pdev)
> +{
> +	struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
> +	struct device *dev = &pdev->dev;
> +	struct sdhci_host *host = NULL;
> +	struct resource *iomem = NULL;
> +	struct sdhci_pxa *pxa = NULL;
> +	int ret, irq;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(dev, "no irq specified\n");
> +		return irq;
> +	}
> +
> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!iomem) {
> +		dev_err(dev, "no memory specified\n");
> +		return -ENOENT;
> +	}
> +
> +	host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pxa));
> +	if (IS_ERR(host)) {
> +		dev_err(dev, "failed to alloc host\n");
> +		return PTR_ERR(host);
> +	}
> +
> +	pxa = sdhci_priv(host);
> +	pxa->host = host;
> +	pxa->pdata = pdata;
> +	pxa->clk_enable = 0;
> +
> +	pxa->clk = clk_get(dev, "PXA-SDHCLK");
> +	if (IS_ERR(pxa->clk)) {
> +		dev_err(dev, "failed to get io clock\n");
> +		ret = PTR_ERR(pxa->clk);
> +		goto out;
> +	}
> +
> +	pxa->res = request_mem_region(iomem->start, resource_size(iomem),
> +		mmc_hostname(host->mmc));
> +	if (!pxa->res) {
> +		dev_err(&pdev->dev, "cannot request region\n");
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	host->ioaddr = ioremap(iomem->start, resource_size(iomem));
> +	if (!host->ioaddr) {
> +		dev_err(&pdev->dev, "failed to remap registers\n");
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	host->hw_name = "MMC";
> +	host->ops = &sdhci_pxa_ops;
> +	host->irq = irq;
> +	host->quirks = SDHCI_QUIRK_BROKEN_ADMA | SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
> +

Maybe check if these aren't already set in pdata and warn user ?

> +	if (pdata->quirks)
> +		host->quirks |= pdata->quirks;
> +
> +	ret = sdhci_add_host(host);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add host\n");
> +		goto out;
> +	}
> +
> +	if (pxa->pdata->max_speed)
> +		host->mmc->f_max = pxa->pdata->max_speed;

What happens otherwise ?

> +
> +	platform_set_drvdata(pdev, host);
> +
> +	return 0;
> +out:
> +	if (host) {
> +		clk_put(pxa->clk);
> +		if (host->ioaddr)
> +			iounmap(host->ioaddr);
> +		if (pxa->res)
> +			release_mem_region(pxa->res->start,
> +					resource_size(pxa->res));
> +		sdhci_free_host(host);
> +	}
> +
> +	return ret;
> +}
> +
> +static int __devexit sdhci_pxa_remove(struct platform_device *pdev)
> +{
> +	struct sdhci_host *host = platform_get_drvdata(pdev);
> +	struct sdhci_pxa *pxa = sdhci_priv(host);
> +	int dead = 0;
> +	u32 scratch;
> +
> +	if (host) {
> +		scratch = readl(host->ioaddr + SDHCI_INT_STATUS);
> +		if (scratch == (u32)-1)
> +			dead = 1;
> +
> +		sdhci_remove_host(host, dead);
> +
> +		if (host->ioaddr)
> +			iounmap(host->ioaddr);
> +		if (pxa->res)
> +			release_mem_region(pxa->res->start,
> +					resource_size(pxa->res));
> +		if (pxa->clk_enable) {
> +			clk_disable(pxa->clk);
> +			pxa->clk_enable = 0;
> +		}
> +		clk_put(pxa->clk);
> +
> +		sdhci_free_host(host);
> +		platform_set_drvdata(pdev, NULL);
> +	}
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int sdhci_pxa_suspend(struct platform_device *dev, pm_message_t
> state) +{
> +	struct sdhci_host *host = platform_get_drvdata(dev);
> +
> +	return sdhci_suspend_host(host, state);
> +}
> +
> +static int sdhci_pxa_resume(struct platform_device *dev)
> +{
> +	struct sdhci_host *host = platform_get_drvdata(dev);
> +
> +	return sdhci_resume_host(host);
> +}
> +#else
> +#define sdhci_pxa_suspend	NULL
> +#define sdhci_pxa_resume	NULL
> +#endif
> +
> +static struct platform_driver sdhci_pxa_driver = {
> +	.probe		= sdhci_pxa_probe,
> +	.remove		= __devexit_p(sdhci_pxa_remove),
> +	.suspend	= sdhci_pxa_suspend,
> +	.resume		= sdhci_pxa_resume,
> +	.driver		= {
> +		.name	= DRIVER_NAME,
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +/*************************************************************************
> ****\ + *                                                                  
>         * + * Driver init/exit                                            
>              * + *                                                        
>                   *
> +\************************************************************************
> *****/ +
> +static int __init sdhci_pxa_init(void)
> +{
> +	return platform_driver_register(&sdhci_pxa_driver);
> +}
> +
> +static void __exit sdhci_pxa_exit(void)
> +{
> +	platform_driver_unregister(&sdhci_pxa_driver);
> +}
> +
> +module_init(sdhci_pxa_init);
> +module_exit(sdhci_pxa_exit);
> +
> +MODULE_DESCRIPTION("SDH controller driver for PXA168/PXA910/MMP2");
> +MODULE_AUTHOR("Zhangfei Gao <zhangfei.gao@marvell.com>");
> +MODULE_LICENSE("GPL v2");

Thanks!

Cheers

[PATCH V3 1/1]MMC: add support of sdhci-pxa driver

[zhangfei gao]

On Sat, Oct 23, 2010 at 10:24 PM, Marek Vasut <marek.vasut@gmail.com> wrote:

>> +/*************************************************************************
>> ****\ + *
>> ? ? ? ? * + * Device probing/removal
>> ? ? ? ? ? ? ?* + *
>> ? ? ? ? ? ? ? ? ? *
>> +\************************************************************************
>> *****/ +
>> +static int __devinit sdhci_pxa_probe(struct platform_device *pdev)
>> +{
>> + ? ? struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
>> + ? ? struct device *dev = &pdev->dev;
>> + ? ? struct sdhci_host *host = NULL;
>> + ? ? struct resource *iomem = NULL;
>> + ? ? struct sdhci_pxa *pxa = NULL;
>> + ? ? int ret, irq;
>> +
>> + ? ? irq = platform_get_irq(pdev, 0);
>> + ? ? if (irq < 0) {
>> + ? ? ? ? ? ? dev_err(dev, "no irq specified\n");
>> + ? ? ? ? ? ? return irq;
>> + ? ? }
>> +
>> + ? ? iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + ? ? if (!iomem) {
>> + ? ? ? ? ? ? dev_err(dev, "no memory specified\n");
>> + ? ? ? ? ? ? return -ENOENT;
>> + ? ? }
>> +
>> + ? ? host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pxa));
>> + ? ? if (IS_ERR(host)) {
>> + ? ? ? ? ? ? dev_err(dev, "failed to alloc host\n");
>> + ? ? ? ? ? ? return PTR_ERR(host);
>> + ? ? }
>> +
>> + ? ? pxa = sdhci_priv(host);
>> + ? ? pxa->host = host;
>> + ? ? pxa->pdata = pdata;
>> + ? ? pxa->clk_enable = 0;
>> +
>> + ? ? pxa->clk = clk_get(dev, "PXA-SDHCLK");
>> + ? ? if (IS_ERR(pxa->clk)) {
>> + ? ? ? ? ? ? dev_err(dev, "failed to get io clock\n");
>> + ? ? ? ? ? ? ret = PTR_ERR(pxa->clk);
>> + ? ? ? ? ? ? goto out;
>> + ? ? }
>> +
>> + ? ? pxa->res = request_mem_region(iomem->start, resource_size(iomem),
>> + ? ? ? ? ? ? mmc_hostname(host->mmc));
>> + ? ? if (!pxa->res) {
>> + ? ? ? ? ? ? dev_err(&pdev->dev, "cannot request region\n");
>> + ? ? ? ? ? ? ret = -EBUSY;
>> + ? ? ? ? ? ? goto out;
>> + ? ? }
>> +
>> + ? ? host->ioaddr = ioremap(iomem->start, resource_size(iomem));
>> + ? ? if (!host->ioaddr) {
>> + ? ? ? ? ? ? dev_err(&pdev->dev, "failed to remap registers\n");
>> + ? ? ? ? ? ? ret = -ENOMEM;
>> + ? ? ? ? ? ? goto out;
>> + ? ? }
>> +
>> + ? ? host->hw_name = "MMC";
>> + ? ? host->ops = &sdhci_pxa_ops;
>> + ? ? host->irq = irq;
>> + ? ? host->quirks = SDHCI_QUIRK_BROKEN_ADMA | SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>> +
>
> Maybe check if these aren't already set in pdata and warn user ?
Here pdata->quirks only provide specific quirk like
SDHCI_QUIRK_BROKEN_CARD_DETECTION for on-chip device, the common quirk
is provided above.
In sdhci-s3c, host->quirks is modified here via pdata->cd_type, which
is also a good method.
Here we transfer SDHCI_QUIRK_BROKEN_CARD_DETECTION directly from pdata
since it already move to include folder.

>
>> + ? ? if (pdata->quirks)
>> + ? ? ? ? ? ? host->quirks |= pdata->quirks;
>> +
>> + ? ? ret = sdhci_add_host(host);
>> + ? ? if (ret) {
>> + ? ? ? ? ? ? dev_err(&pdev->dev, "failed to add host\n");
>> + ? ? ? ? ? ? goto out;
>> + ? ? }
>> +
>> + ? ? if (pxa->pdata->max_speed)
>> + ? ? ? ? ? ? host->mmc->f_max = pxa->pdata->max_speed;
>
> What happens otherwise ?
Otherwise, host->mmc->f_max = host->max_clk set in sdhci_add_host,
host->max_clk is 50M in 2.0 controller, and 200M in 3.0 controller, if
controller does not need to provide get_max_clock.
it's better add defalut value in else condition.

Thanks Marek for review.

>
>> +
>> + ? ? platform_set_drvdata(pdev, host);
>> +
>> + ? ? return 0;
>> +out:
>> + ? ? if (host) {
>> + ? ? ? ? ? ? clk_put(pxa->clk);
>> + ? ? ? ? ? ? if (host->ioaddr)
>> + ? ? ? ? ? ? ? ? ? ? iounmap(host->ioaddr);
>> + ? ? ? ? ? ? if (pxa->res)
>> + ? ? ? ? ? ? ? ? ? ? release_mem_region(pxa->res->start,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? resource_size(pxa->res));
>> + ? ? ? ? ? ? sdhci_free_host(host);
>> + ? ? }
>> +
>> + ? ? return ret;
>> +}
>> +

>
> Thanks!
>
> Cheers
>

[PATCH V3 1/1]MMC: add support of sdhci-pxa driver

[Marek Vasut]

Dne So 23. ??jna 2010 18:47:10 zhangfei gao napsal(a):
> On Sat, Oct 23, 2010 at 10:24 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> >> +/**********************************************************************
> >> *** ****\ + *
> >>         * + * Device probing/removal
> >>              * + *
> >>                   *
> >> +\**********************************************************************
> >> ** *****/ +
> >> +static int __devinit sdhci_pxa_probe(struct platform_device *pdev)
> >> +{
> >> +     struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
> >> +     struct device *dev = &pdev->dev;
> >> +     struct sdhci_host *host = NULL;
> >> +     struct resource *iomem = NULL;
> >> +     struct sdhci_pxa *pxa = NULL;
> >> +     int ret, irq;
> >> +
> >> +     irq = platform_get_irq(pdev, 0);
> >> +     if (irq < 0) {
> >> +             dev_err(dev, "no irq specified\n");
> >> +             return irq;
> >> +     }
> >> +
> >> +     iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +     if (!iomem) {
> >> +             dev_err(dev, "no memory specified\n");
> >> +             return -ENOENT;
> >> +     }
> >> +
> >> +     host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pxa));
> >> +     if (IS_ERR(host)) {
> >> +             dev_err(dev, "failed to alloc host\n");
> >> +             return PTR_ERR(host);
> >> +     }
> >> +
> >> +     pxa = sdhci_priv(host);
> >> +     pxa->host = host;
> >> +     pxa->pdata = pdata;
> >> +     pxa->clk_enable = 0;
> >> +
> >> +     pxa->clk = clk_get(dev, "PXA-SDHCLK");
> >> +     if (IS_ERR(pxa->clk)) {
> >> +             dev_err(dev, "failed to get io clock\n");
> >> +             ret = PTR_ERR(pxa->clk);
> >> +             goto out;
> >> +     }
> >> +
> >> +     pxa->res = request_mem_region(iomem->start, resource_size(iomem),
> >> +             mmc_hostname(host->mmc));
> >> +     if (!pxa->res) {
> >> +             dev_err(&pdev->dev, "cannot request region\n");
> >> +             ret = -EBUSY;
> >> +             goto out;
> >> +     }
> >> +
> >> +     host->ioaddr = ioremap(iomem->start, resource_size(iomem));
> >> +     if (!host->ioaddr) {
> >> +             dev_err(&pdev->dev, "failed to remap registers\n");
> >> +             ret = -ENOMEM;
> >> +             goto out;
> >> +     }
> >> +
> >> +     host->hw_name = "MMC";
> >> +     host->ops = &sdhci_pxa_ops;
> >> +     host->irq = irq;
> >> +     host->quirks = SDHCI_QUIRK_BROKEN_ADMA |
> >> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; +
> > 
> > Maybe check if these aren't already set in pdata and warn user ?
> 
> Here pdata->quirks only provide specific quirk like
> SDHCI_QUIRK_BROKEN_CARD_DETECTION for on-chip device, the common quirk
> is provided above.

Right, but what if the user also provides such a quirk? It's unnecessary ... and 
in case the quirk was removed from here later, it'd have to be removed from 
user's code too ...

> In sdhci-s3c, host->quirks is modified here via pdata->cd_type, which
> is also a good method.
> Here we transfer SDHCI_QUIRK_BROKEN_CARD_DETECTION directly from pdata
> since it already move to include folder.
> 
> >> +     if (pdata->quirks)
> >> +             host->quirks |= pdata->quirks;
> >> +
> >> +     ret = sdhci_add_host(host);
> >> +     if (ret) {
> >> +             dev_err(&pdev->dev, "failed to add host\n");
> >> +             goto out;
> >> +     }
> >> +
> >> +     if (pxa->pdata->max_speed)
> >> +             host->mmc->f_max = pxa->pdata->max_speed;
> > 
> > What happens otherwise ?
> 
> Otherwise, host->mmc->f_max = host->max_clk set in sdhci_add_host,
> host->max_clk is 50M in 2.0 controller, and 200M in 3.0 controller, if
> controller does not need to provide get_max_clock.
> it's better add defalut value in else condition.

Thanks for clearing this
> 
> Thanks Marek for review.
> 
> >> +
> >> +     platform_set_drvdata(pdev, host);
> >> +
> >> +     return 0;
> >> +out:
> >> +     if (host) {
> >> +             clk_put(pxa->clk);
> >> +             if (host->ioaddr)
> >> +                     iounmap(host->ioaddr);
> >> +             if (pxa->res)
> >> +                     release_mem_region(pxa->res->start,
> >> +                                     resource_size(pxa->res));
> >> +             sdhci_free_host(host);
> >> +     }
> >> +
> >> +     return ret;
> >> +}
> >> +
> > 
> > Thanks!
> > 
> > Cheers

[PATCH V3 1/1]MMC: add support of sdhci-pxa driver

[zhangfei gao]

On Sun, Oct 24, 2010 at 1:50 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Dne So 23. ??jna 2010 18:47:10 zhangfei gao napsal(a):
>> On Sat, Oct 23, 2010 at 10:24 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> >> +/**********************************************************************
>> >> *** ****\ + *
>> >> ? ? ? ? * + * Device probing/removal
>> >> ? ? ? ? ? ? ?* + *
>> >> ? ? ? ? ? ? ? ? ? *
>> >> +\**********************************************************************
>> >> ** *****/ +
>> >> +static int __devinit sdhci_pxa_probe(struct platform_device *pdev)
>> >> +{
>> >> + ? ? struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
>> >> + ? ? struct device *dev = &pdev->dev;
>> >> + ? ? struct sdhci_host *host = NULL;
>> >> + ? ? struct resource *iomem = NULL;
>> >> + ? ? struct sdhci_pxa *pxa = NULL;
>> >> + ? ? int ret, irq;
>> >> +
>> >> + ? ? irq = platform_get_irq(pdev, 0);
>> >> + ? ? if (irq < 0) {
>> >> + ? ? ? ? ? ? dev_err(dev, "no irq specified\n");
>> >> + ? ? ? ? ? ? return irq;
>> >> + ? ? }
>> >> +
>> >> + ? ? iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >> + ? ? if (!iomem) {
>> >> + ? ? ? ? ? ? dev_err(dev, "no memory specified\n");
>> >> + ? ? ? ? ? ? return -ENOENT;
>> >> + ? ? }
>> >> +
>> >> + ? ? host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pxa));
>> >> + ? ? if (IS_ERR(host)) {
>> >> + ? ? ? ? ? ? dev_err(dev, "failed to alloc host\n");
>> >> + ? ? ? ? ? ? return PTR_ERR(host);
>> >> + ? ? }
>> >> +
>> >> + ? ? pxa = sdhci_priv(host);
>> >> + ? ? pxa->host = host;
>> >> + ? ? pxa->pdata = pdata;
>> >> + ? ? pxa->clk_enable = 0;
>> >> +
>> >> + ? ? pxa->clk = clk_get(dev, "PXA-SDHCLK");
>> >> + ? ? if (IS_ERR(pxa->clk)) {
>> >> + ? ? ? ? ? ? dev_err(dev, "failed to get io clock\n");
>> >> + ? ? ? ? ? ? ret = PTR_ERR(pxa->clk);
>> >> + ? ? ? ? ? ? goto out;
>> >> + ? ? }
>> >> +
>> >> + ? ? pxa->res = request_mem_region(iomem->start, resource_size(iomem),
>> >> + ? ? ? ? ? ? mmc_hostname(host->mmc));
>> >> + ? ? if (!pxa->res) {
>> >> + ? ? ? ? ? ? dev_err(&pdev->dev, "cannot request region\n");
>> >> + ? ? ? ? ? ? ret = -EBUSY;
>> >> + ? ? ? ? ? ? goto out;
>> >> + ? ? }
>> >> +
>> >> + ? ? host->ioaddr = ioremap(iomem->start, resource_size(iomem));
>> >> + ? ? if (!host->ioaddr) {
>> >> + ? ? ? ? ? ? dev_err(&pdev->dev, "failed to remap registers\n");
>> >> + ? ? ? ? ? ? ret = -ENOMEM;
>> >> + ? ? ? ? ? ? goto out;
>> >> + ? ? }
>> >> +
>> >> + ? ? host->hw_name = "MMC";
>> >> + ? ? host->ops = &sdhci_pxa_ops;
>> >> + ? ? host->irq = irq;
>> >> + ? ? host->quirks = SDHCI_QUIRK_BROKEN_ADMA |
>> >> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; +
>> >
>> > Maybe check if these aren't already set in pdata and warn user ?
>>
>> Here pdata->quirks only provide specific quirk like
>> SDHCI_QUIRK_BROKEN_CARD_DETECTION for on-chip device, the common quirk
>> is provided above.
>
> Right, but what if the user also provides such a quirk? It's unnecessary ... and
> in case the quirk was removed from here later, it'd have to be removed from
> user's code too ...

What do you mean 'in case the quirk was removed from here later'.
SDHCI_QUIRK_* is just move from drivers/mmc/host/sdhci.h to
include/linux/mmc/sdhci.h, so code in arch/arm could access them
directly.

We also consider use one flag here, like sdhci-s3c.c.
Code like this,
if (pdata->flags & PXA_FLAG_CARD_PERMENT)
     host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;

If SDHCI_QUIRK_* not permitted to access, it would be better to use this method.

>
>> In sdhci-s3c, host->quirks is modified here via pdata->cd_type, which
>> is also a good method.
>> Here we transfer SDHCI_QUIRK_BROKEN_CARD_DETECTION directly from pdata
>> since it already move to include folder.
>>
>> >> + ? ? if (pdata->quirks)
>> >> + ? ? ? ? ? ? host->quirks |= pdata->quirks;
>> >> +
>> >> + ? ? ret = sdhci_add_host(host);
>> >> + ? ? if (ret) {
>> >> + ? ? ? ? ? ? dev_err(&pdev->dev, "failed to add host\n");
>> >> + ? ? ? ? ? ? goto out;
>> >> + ? ? }
>> >> +
>> >> + ? ? if (pxa->pdata->max_speed)
>> >> + ? ? ? ? ? ? host->mmc->f_max = pxa->pdata->max_speed;
>> >
>> > What happens otherwise ?
>>
>> Otherwise, host->mmc->f_max = host->max_clk set in sdhci_add_host,
>> host->max_clk is 50M in 2.0 controller, and 200M in 3.0 controller, if
>> controller does not need to provide get_max_clock.
>> it's better add defalut value in else condition.
>
> Thanks for clearing this
>>
>> Thanks Marek for review.
>>
>> >> +
>> >> + ? ? platform_set_drvdata(pdev, host);
>> >> +
>> >> + ? ? return 0;
>> >> +out:
>> >> + ? ? if (host) {
>> >> + ? ? ? ? ? ? clk_put(pxa->clk);
>> >> + ? ? ? ? ? ? if (host->ioaddr)
>> >> + ? ? ? ? ? ? ? ? ? ? iounmap(host->ioaddr);
>> >> + ? ? ? ? ? ? if (pxa->res)
>> >> + ? ? ? ? ? ? ? ? ? ? release_mem_region(pxa->res->start,
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? resource_size(pxa->res));
>> >> + ? ? ? ? ? ? sdhci_free_host(host);
>> >> + ? ? }
>> >> +
>> >> + ? ? return ret;
>> >> +}
>> >> +
>> >
>> > Thanks!
>> >
>> > Cheers
>

[PATCH V3 1/1]MMC: add support of sdhci-pxa driver

[Marek Vasut]

Dne Ne 24. ??jna 2010 05:26:38 zhangfei gao napsal(a):
> On Sun, Oct 24, 2010 at 1:50 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > Dne So 23. ??jna 2010 18:47:10 zhangfei gao napsal(a):
> >> On Sat, Oct 23, 2010 at 10:24 PM, Marek Vasut <marek.vasut@gmail.com> 
wrote:
> >> >> +/*******************************************************************
> >> >> *** *** ****\ + *
> >> >>         * + * Device probing/removal
> >> >>              * + *
> >> >>                   *
> >> >> +\*******************************************************************
> >> >> *** ** *****/ +
> >> >> +static int __devinit sdhci_pxa_probe(struct platform_device *pdev)
> >> >> +{
> >> >> +     struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
> >> >> +     struct device *dev = &pdev->dev;
> >> >> +     struct sdhci_host *host = NULL;
> >> >> +     struct resource *iomem = NULL;
> >> >> +     struct sdhci_pxa *pxa = NULL;
> >> >> +     int ret, irq;
> >> >> +
> >> >> +     irq = platform_get_irq(pdev, 0);
> >> >> +     if (irq < 0) {
> >> >> +             dev_err(dev, "no irq specified\n");
> >> >> +             return irq;
> >> >> +     }
> >> >> +
> >> >> +     iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> >> +     if (!iomem) {
> >> >> +             dev_err(dev, "no memory specified\n");
> >> >> +             return -ENOENT;
> >> >> +     }
> >> >> +
> >> >> +     host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pxa));
> >> >> +     if (IS_ERR(host)) {
> >> >> +             dev_err(dev, "failed to alloc host\n");
> >> >> +             return PTR_ERR(host);
> >> >> +     }
> >> >> +
> >> >> +     pxa = sdhci_priv(host);
> >> >> +     pxa->host = host;
> >> >> +     pxa->pdata = pdata;
> >> >> +     pxa->clk_enable = 0;
> >> >> +
> >> >> +     pxa->clk = clk_get(dev, "PXA-SDHCLK");
> >> >> +     if (IS_ERR(pxa->clk)) {
> >> >> +             dev_err(dev, "failed to get io clock\n");
> >> >> +             ret = PTR_ERR(pxa->clk);
> >> >> +             goto out;
> >> >> +     }
> >> >> +
> >> >> +     pxa->res = request_mem_region(iomem->start,
> >> >> resource_size(iomem), +             mmc_hostname(host->mmc));
> >> >> +     if (!pxa->res) {
> >> >> +             dev_err(&pdev->dev, "cannot request region\n");
> >> >> +             ret = -EBUSY;
> >> >> +             goto out;
> >> >> +     }
> >> >> +
> >> >> +     host->ioaddr = ioremap(iomem->start, resource_size(iomem));
> >> >> +     if (!host->ioaddr) {
> >> >> +             dev_err(&pdev->dev, "failed to remap registers\n");
> >> >> +             ret = -ENOMEM;
> >> >> +             goto out;
> >> >> +     }
> >> >> +
> >> >> +     host->hw_name = "MMC";
> >> >> +     host->ops = &sdhci_pxa_ops;
> >> >> +     host->irq = irq;
> >> >> +     host->quirks = SDHCI_QUIRK_BROKEN_ADMA |
> >> >> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; +
> >> > 
> >> > Maybe check if these aren't already set in pdata and warn user ?
> >> 
> >> Here pdata->quirks only provide specific quirk like
> >> SDHCI_QUIRK_BROKEN_CARD_DETECTION for on-chip device, the common quirk
> >> is provided above.
> > 
> > Right, but what if the user also provides such a quirk? It's unnecessary
> > ... and in case the quirk was removed from here later, it'd have to be
> > removed from user's code too ...
> 
> What do you mean 'in case the quirk was removed from here later'.

I mean ... if setting the quirk was removed from the driver source, it'd have to 
be removed from user code that uses the driver as well -- in case user also set 
such a quirk in pdata->quirks. So it might be a good idea to warn user that he's 
setting something that's getting set elsewhere (in the driver) anyway.

> SDHCI_QUIRK_* is just move from drivers/mmc/host/sdhci.h to
> include/linux/mmc/sdhci.h, so code in arch/arm could access them
> directly.
> 
> We also consider use one flag here, like sdhci-s3c.c.
> Code like this,
> if (pdata->flags & PXA_FLAG_CARD_PERMENT)
>      host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> 
> If SDHCI_QUIRK_* not permitted to access, it would be better to use this
> method.
> 
> >> In sdhci-s3c, host->quirks is modified here via pdata->cd_type, which
> >> is also a good method.
> >> Here we transfer SDHCI_QUIRK_BROKEN_CARD_DETECTION directly from pdata
> >> since it already move to include folder.
> >> 
> >> >> +     if (pdata->quirks)
> >> >> +             host->quirks |= pdata->quirks;
> >> >> +
> >> >> +     ret = sdhci_add_host(host);
> >> >> +     if (ret) {
> >> >> +             dev_err(&pdev->dev, "failed to add host\n");
> >> >> +             goto out;
> >> >> +     }
> >> >> +
> >> >> +     if (pxa->pdata->max_speed)
> >> >> +             host->mmc->f_max = pxa->pdata->max_speed;
> >> > 
> >> > What happens otherwise ?
> >> 
> >> Otherwise, host->mmc->f_max = host->max_clk set in sdhci_add_host,
> >> host->max_clk is 50M in 2.0 controller, and 200M in 3.0 controller, if
> >> controller does not need to provide get_max_clock.
> >> it's better add defalut value in else condition.
> > 
> > Thanks for clearing this
> > 
> >> Thanks Marek for review.
> >> 
> >> >> +
> >> >> +     platform_set_drvdata(pdev, host);
> >> >> +
> >> >> +     return 0;
> >> >> +out:
> >> >> +     if (host) {
> >> >> +             clk_put(pxa->clk);
> >> >> +             if (host->ioaddr)
> >> >> +                     iounmap(host->ioaddr);
> >> >> +             if (pxa->res)
> >> >> +                     release_mem_region(pxa->res->start,
> >> >> +                                     resource_size(pxa->res));
> >> >> +             sdhci_free_host(host);
> >> >> +     }
> >> >> +
> >> >> +     return ret;
> >> >> +}
> >> >> +
> >> > 
> >> > Thanks!
> >> > 
> >> > Cheers

[PATCH V3 1/1]MMC: add support of sdhci-pxa driver

[zhangfei gao]

2010/10/24 Marek Vasut <marek.vasut@gmail.com>:
> Dne Ne 24. ??jna 2010 05:26:38 zhangfei gao napsal(a):
>> On Sun, Oct 24, 2010 at 1:50 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> > Dne So 23. ??jna 2010 18:47:10 zhangfei gao napsal(a):
>> >> On Sat, Oct 23, 2010 at 10:24 PM, Marek Vasut <marek.vasut@gmail.com>
> wrote:
>> >> >> +/*******************************************************************
>> >> >> *** *** ****\ + *
>> >> >> ? ? ? ? * + * Device probing/removal
>> >> >> ? ? ? ? ? ? ?* + *
>> >> >> ? ? ? ? ? ? ? ? ? *
>> >> >> +\*******************************************************************
>> >> >> *** ** *****/ +
>> >> >> +static int __devinit sdhci_pxa_probe(struct platform_device *pdev)
>> >> >> +{
>> >> >> + ? ? struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
>> >> >> + ? ? struct device *dev = &pdev->dev;
>> >> >> + ? ? struct sdhci_host *host = NULL;
>> >> >> + ? ? struct resource *iomem = NULL;
>> >> >> + ? ? struct sdhci_pxa *pxa = NULL;
>> >> >> + ? ? int ret, irq;
>> >> >> +
>> >> >> + ? ? irq = platform_get_irq(pdev, 0);
>> >> >> + ? ? if (irq < 0) {
>> >> >> + ? ? ? ? ? ? dev_err(dev, "no irq specified\n");
>> >> >> + ? ? ? ? ? ? return irq;
>> >> >> + ? ? }
>> >> >> +
>> >> >> + ? ? iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >> >> + ? ? if (!iomem) {
>> >> >> + ? ? ? ? ? ? dev_err(dev, "no memory specified\n");
>> >> >> + ? ? ? ? ? ? return -ENOENT;
>> >> >> + ? ? }
>> >> >> +
>> >> >> + ? ? host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pxa));
>> >> >> + ? ? if (IS_ERR(host)) {
>> >> >> + ? ? ? ? ? ? dev_err(dev, "failed to alloc host\n");
>> >> >> + ? ? ? ? ? ? return PTR_ERR(host);
>> >> >> + ? ? }
>> >> >> +
>> >> >> + ? ? pxa = sdhci_priv(host);
>> >> >> + ? ? pxa->host = host;
>> >> >> + ? ? pxa->pdata = pdata;
>> >> >> + ? ? pxa->clk_enable = 0;
>> >> >> +
>> >> >> + ? ? pxa->clk = clk_get(dev, "PXA-SDHCLK");
>> >> >> + ? ? if (IS_ERR(pxa->clk)) {
>> >> >> + ? ? ? ? ? ? dev_err(dev, "failed to get io clock\n");
>> >> >> + ? ? ? ? ? ? ret = PTR_ERR(pxa->clk);
>> >> >> + ? ? ? ? ? ? goto out;
>> >> >> + ? ? }
>> >> >> +
>> >> >> + ? ? pxa->res = request_mem_region(iomem->start,
>> >> >> resource_size(iomem), + ? ? ? ? ? ? mmc_hostname(host->mmc));
>> >> >> + ? ? if (!pxa->res) {
>> >> >> + ? ? ? ? ? ? dev_err(&pdev->dev, "cannot request region\n");
>> >> >> + ? ? ? ? ? ? ret = -EBUSY;
>> >> >> + ? ? ? ? ? ? goto out;
>> >> >> + ? ? }
>> >> >> +
>> >> >> + ? ? host->ioaddr = ioremap(iomem->start, resource_size(iomem));
>> >> >> + ? ? if (!host->ioaddr) {
>> >> >> + ? ? ? ? ? ? dev_err(&pdev->dev, "failed to remap registers\n");
>> >> >> + ? ? ? ? ? ? ret = -ENOMEM;
>> >> >> + ? ? ? ? ? ? goto out;
>> >> >> + ? ? }
>> >> >> +
>> >> >> + ? ? host->hw_name = "MMC";
>> >> >> + ? ? host->ops = &sdhci_pxa_ops;
>> >> >> + ? ? host->irq = irq;
>> >> >> + ? ? host->quirks = SDHCI_QUIRK_BROKEN_ADMA |
>> >> >> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; +
>> >> >
>> >> > Maybe check if these aren't already set in pdata and warn user ?
>> >>
>> >> Here pdata->quirks only provide specific quirk like
>> >> SDHCI_QUIRK_BROKEN_CARD_DETECTION for on-chip device, the common quirk
>> >> is provided above.
>> >
>> > Right, but what if the user also provides such a quirk? It's unnecessary
>> > ... and in case the quirk was removed from here later, it'd have to be
>> > removed from user's code too ...
>>
>> What do you mean 'in case the quirk was removed from here later'.
>
> I mean ... if setting the quirk was removed from the driver source, it'd have to
> be removed from user code that uses the driver as well -- in case user also set
> such a quirk in pdata->quirks. So it might be a good idea to warn user that he's
> setting something that's getting set elsewhere (in the driver) anyway.

Thanks for suggestion, but I think it is OK here.
In our design, driver set common limitation quirk, and pdata set
specific quirk for some device.

>
>> SDHCI_QUIRK_* is just move from drivers/mmc/host/sdhci.h to
>> include/linux/mmc/sdhci.h, so code in arch/arm could access them
>> directly.
>>
>> We also consider use one flag here, like sdhci-s3c.c.
>> Code like this,
>> if (pdata->flags & PXA_FLAG_CARD_PERMENT)
>> ? ? ?host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
>>
>> If SDHCI_QUIRK_* not permitted to access, it would be better to use this
>> method.
>>
>> >> In sdhci-s3c, host->quirks is modified here via pdata->cd_type, which
>> >> is also a good method.
>> >> Here we transfer SDHCI_QUIRK_BROKEN_CARD_DETECTION directly from pdata
>> >> since it already move to include folder.
>> >>
>> >> >> + ? ? if (pdata->quirks)
>> >> >> + ? ? ? ? ? ? host->quirks |= pdata->quirks;
>> >> >> +
>> >> >> + ? ? ret = sdhci_add_host(host);
>> >> >> + ? ? if (ret) {
>> >> >> + ? ? ? ? ? ? dev_err(&pdev->dev, "failed to add host\n");
>> >> >> + ? ? ? ? ? ? goto out;
>> >> >> + ? ? }
>> >> >> +
>> >> >> + ? ? if (pxa->pdata->max_speed)
>> >> >> + ? ? ? ? ? ? host->mmc->f_max = pxa->pdata->max_speed;
>> >> >
>> >> > What happens otherwise ?
>> >>
>> >> Otherwise, host->mmc->f_max = host->max_clk set in sdhci_add_host,
>> >> host->max_clk is 50M in 2.0 controller, and 200M in 3.0 controller, if
>> >> controller does not need to provide get_max_clock.
>> >> it's better add defalut value in else condition.
>> >
>> > Thanks for clearing this
>> >
>> >> Thanks Marek for review.
>> >>
>> >> >> +
>> >> >> + ? ? platform_set_drvdata(pdev, host);
>> >> >> +
>> >> >> + ? ? return 0;
>> >> >> +out:
>> >> >> + ? ? if (host) {
>> >> >> + ? ? ? ? ? ? clk_put(pxa->clk);
>> >> >> + ? ? ? ? ? ? if (host->ioaddr)
>> >> >> + ? ? ? ? ? ? ? ? ? ? iounmap(host->ioaddr);
>> >> >> + ? ? ? ? ? ? if (pxa->res)
>> >> >> + ? ? ? ? ? ? ? ? ? ? release_mem_region(pxa->res->start,
>> >> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? resource_size(pxa->res));
>> >> >> + ? ? ? ? ? ? sdhci_free_host(host);
>> >> >> + ? ? }
>> >> >> +
>> >> >> + ? ? return ret;
>> >> >> +}
>> >> >> +
>> >> >
>> >> > Thanks!
>> >> >
>> >> > Cheers
>