On Sun, Sep 26, 2010 at 8:39 PM, Eric Miao <eric.y.miao@xxxxxxxxx> wrote:
> 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?
pxamci.c is used for pxa25x/26x/27x, while sdhci-pxa.c is used for
pxa168/pxa910/mmp2.
>
>> 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.
To distinguish the pxa->quirk, which used for transfer to sdhci,c
>
>> +
>> +#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?
Some confusion here.
Since quirk is limited resource, we should not over-used, however, the
quirk method is good design and could be used to distinguish different
device for different usage.
pxa_quirk is used by our own driver, transfered from platform data,
for example, driver may understand free-running requirement from emmc.
pxa->quirks is used for transfer to sdhci.c, which contains not only
the init quirks, but also some other requirement, such as
SDHCI_QUIRK_BROKEN_CARD_DETECTION, which only effective to on-chip
non-removalbe device.
>
>> + & 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.
Thanks for finding, the clk_enable(pxa->clk) is planed to be removed
here, just use .set_clock to do clock issue, though one small issue is
mmc stack would never set_clock(host, 0) to disalbe clock, will submit
patch later.
>
> 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?
SDHCI_QUIRK_BROKEN_CARD_DETECTION is defined in sdhci.h, which in
driver/mmc/host instead of include, what's more, quirk is limited
resource, and we should not touch quirk as much as possible.
>
>> +
>> + 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
