On Sun, Sep 26, 2010 at 6:56 PM, zhangfei gao <zhangfei.gao@xxxxxxxxx> wrote:
> Assume the sdhci-pltfm call back are accepted
>
> From f7e1cc9975cbed2d993d4209eae36f8f300fb18c Mon Sep 17 00:00:00 2001
> From: Zhangfei Gao <zhangfei.gao@xxxxxxxxxxx>
> 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.gao@xxxxxxxxxxx>
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.gao@xxxxxxxxxxx>
> + *
> + * 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.gao@xxxxxxxxxxx>
> + *
> + * 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.gao@xxxxxxxxxxx>");
> +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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
