On Sun, Sep 26, 2010 at 6:56 PM, zhangfei gao <zhangfei....@gmail.com> wrote: > Assume the sdhci-pltfm call back are accepted > > From f7e1cc9975cbed2d993d4209eae36f8f300fb18c Mon Sep 17 00:00:00 2001 > From: Zhangfei Gao <zhangfei....@marvell.com> > Date: Sun, 26 Sep 2010 17:13:43 -0400 > Subject: [PATCH 2/2] mmc: add support of sdhci-pxa driver > > Support Marvell PXA168/PXA910/MMP2 SD Host Controller > > Signed-off-by: Zhangfei Gao <zhangfei....@marvell.com>
Zhangfei, Patch looks good to me, except for a few minor issues as I commented below. Please have a look again. > --- > arch/arm/plat-pxa/include/plat/sdhci.h | 32 +++++++ > drivers/mmc/host/Kconfig | 11 +++ > drivers/mmc/host/Makefile | 1 + > drivers/mmc/host/sdhci-pxa.c | 143 > ++++++++++++++++++++++++++++++++ I prefer we rename it to something like sdhci-mmp.c and the 'pxa' in the source code to 'mmp' to cause less confusion with pxamci.c? > 4 files changed, 187 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..3d5f5ef > --- /dev/null > +++ b/arch/arm/plat-pxa/include/plat/sdhci.h > @@ -0,0 +1,32 @@ > +/* arch/arm/plat-pxa/include/plat/sdhci.h > + * > + * Copyright 2010 Marvell > + * Zhangfei Gao <zhangfei....@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 __FILE__ typo > + > +/* pxa specific quirks */ > +/* Card alwayes wired to host, like emmc */ > +#define PXA_QUIRK_BROKEN_CARD_DETECTION (1<<0) > +/* Require clock free running */ > +#define PXA_QUIRK_DISABLE_CLOCK_GATING (1<<1) tabs/space > + > +/** > + * struct pxa_sdhci_platdata() - Platform device data for PXA SDHCI > + * @max_speed: The maximum speed supported. > + * @pxa_quirks: specific quirk of pxa > +*/ > +struct sdhci_pxa_platdata { > + unsigned int max_speed; > + unsigned int pxa_quirk; > +}; 'unsigned int quirk' should be enough here, no need for a pxa_ prefix. > + > +#endif /* __PLAT_PXA_SDHCI_H */ > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig > index 6f12d5d..9510976 100644 > --- a/drivers/mmc/host/Kconfig > +++ b/drivers/mmc/host/Kconfig > @@ -158,6 +158,17 @@ 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 Only on ARCH_MMP or is this also used in PXA950+? > + depends on MMC_SDHCI_PLTFM > + 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 ef32c32..d166493 100644 > --- a/drivers/mmc/host/Makefile > +++ b/drivers/mmc/host/Makefile > @@ -38,6 +38,7 @@ obj-$(CONFIG_MMC_USHC) += ushc.o > 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_PXA) += sdhci-pxa.o > > obj-$(CONFIG_MMC_SDHCI_OF) += sdhci-of.o > sdhci-of-y := sdhci-of-core.o > diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c > new file mode 100644 > index 0000000..12ede18 > --- /dev/null > +++ b/drivers/mmc/host/sdhci-pxa.c > @@ -0,0 +1,143 @@ > +/* linux/drivers/mmc/host/sdhci-pxa.c > + * > + * Copyright 2010 Marvell > + * Zhangfei Gao <zhangfei....@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 > + * > + * Based on sdhci-pltfm.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 <linux/sdhci-pltfm.h> > +#include "sdhci.h" > +#include "sdhci-pltfm.h" > + > +#define SD_FIFO_PARAM 0x104 > +#define DIS_PAD_SD_CLK_GATE 0x400 > + > +struct sdhci_pxa { > + struct sdhci_pxa_platdata *pdata; > + struct clk *clk; > + > + u32 quirks; > + u8 clk_enable; > +}; > + > +/*****************************************************************************\ > + * > * > + * SDHCI core callbacks > * > + * > * > +\*****************************************************************************/ > +static void sdhci_pxa_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->pxa_quirk Better copy this 'pxa_quirk' over to pxa->quirks at initialization? > + & PXA_QUIRK_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 = sdhci_pxa_set_clock, > +}; > + > +/*****************************************************************************\ > + * > * > + * sdhci-pltfm callbacks > * > + * > * > +\*****************************************************************************/ > +static int sdhci_pxa_init(struct sdhci_host *host, struct > sdhci_pltfm_data *pdata, void* priv_pdata) > +{ > + struct sdhci_pxa *pxa = sdhci_priv(host); > + struct device *dev = mmc_dev(host->mmc); > + > + pxa->pdata = priv_pdata; > + pxa->clk_enable = 0; > + > + pxa->clk = clk_get(dev, "PXA-SDHCLK"); > + if (IS_ERR(pxa->clk)) { > + dev_err(dev, "clk err\n"); > + return -ENODEV; > + } > + clk_enable(pxa->clk); > + dev_dbg(dev, "SDHC clock:%lu\n", clk_get_rate(pxa->clk)); > + > + pxa->clk_enable = 0; Shouldn't this be 1 as you've called clk_enable()? And if the .set_clock logic is all right, we can simply leave the clock enabling/disabling to that function. Or if the power consumption difference doesn't differ that much, I'd rather to see simpler solution to this: clk_enable() at probe, and clk_disable() at remove? > + pxa->quirks = pdata->quirks; > + > + if (pxa->pdata->pxa_quirk & PXA_QUIRK_BROKEN_CARD_DETECTION) > + pxa->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION; Why not just use SDHCI_QUIRK_BROKEN_CARD_DETECTION in pdata->pxa_quirk? > + > + return 0; > +} > + > +static unsigned int sdhci_pxa_get_quirk(struct sdhci_host *host) > +{ > + struct sdhci_pxa *pxa = sdhci_priv(host); > + > + return pxa->quirks; > +} > + > +static void sdhci_pxa_set_max_speed(struct sdhci_host *host) > +{ > + struct sdhci_pxa *pxa = sdhci_priv(host); > + > + if (pxa->pdata->max_speed) > + host->mmc->f_max = pxa->pdata->max_speed; > +} > + > +static void sdhci_pxa_exit(struct sdhci_host *host) > +{ > + struct sdhci_pxa *pxa = sdhci_priv(host); > + > + if (pxa->clk_enable) > + clk_disable(pxa->clk); > + clk_put(pxa->clk); > +} > + > +static struct sdhci_host *sdhci_pxa_alloc_host(struct device *dev) > +{ > + return sdhci_alloc_host(dev, sizeof(struct sdhci_pxa)); > +} > + > +struct sdhci_pltfm_data sdhci_pxa_pdata = { > + .ops = &sdhci_pxa_ops, > + .quirks = SDHCI_QUIRK_BROKEN_ADMA | SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, > + .init = sdhci_pxa_init, > + .exit = sdhci_pxa_exit, > + .get_quirk = sdhci_pxa_get_quirk, > + .set_max_speed = sdhci_pxa_set_max_speed, > + .alloc_host = sdhci_pxa_alloc_host, > +}; > + > +MODULE_DESCRIPTION("SDH controller driver for PXA168/PXA910/MMP2"); > +MODULE_AUTHOR("Zhangfei Gao <zhangfei....@marvell.com>"); > +MODULE_LICENSE("GPL v2"); > -- > 1.7.0.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html