On Tue, Nov 9, 2010 at 10:17 PM, zhangfei gao <zhangfei.gao@xxxxxxxxx> wrote:
> Haojian,
>
> Would you help review, since arch/arm/plat-pxa/include/plat/sdhci.h is
> updated, thanks
>
> From 6619812b749aa12ebf0efbe057366c2f99051bfe Mon Sep 17 00:00:00 2001
> From: Zhangfei Gao <zhangfei.gao@xxxxxxxxxxx>
> Date: Tue, 2 Nov 2010 07:37:44 -0400
> Subject: [PATCH] sdhci-pxa: support tune_timming for various cards
>
> Â Â Â Â1. Add pdata check, in case pdata is NULL
> Â Â Â Â2. Add tune_timming to adjust read data/command timming without
> performance impact when crc error, as a result
> Â Â Â Â Â Â Â Âa, sd could work at 50M
> Â Â Â Â Â Â Â Âb, emmc could work at ddr50 mode
> Â Â Â Â3. Remove clock_enable checking, since clock gating is still on-going
> in core stack.
>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@xxxxxxxxxxx>
> ---
> Âarch/arm/plat-pxa/include/plat/sdhci.h | Â Â2 +
> Âdrivers/mmc/host/sdhci-pxa.c      |  51 +++++++++++++++++++++++--------
> Â2 files changed, 40 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h
> b/arch/arm/plat-pxa/include/plat/sdhci.h
> index fc5ceab..8c340b1 100644
> --- a/arch/arm/plat-pxa/include/plat/sdhci.h
> +++ b/arch/arm/plat-pxa/include/plat/sdhci.h
> @@ -24,11 +24,13 @@
> Â* @max_speed: the maximum speed supported
> Â* @quirks: quirks of specific device
> Â* @flags: flags for platform requirement
> + * @clk_delay: clk_delay vlaue to tunning various cards
> Â*/
> Âstruct sdhci_pxa_platdata {
>    Âunsigned int  Âmax_speed;
>    Âunsigned int  Âquirks;
>    Âunsigned int  Âflags;
> +    unsigned int  Âclk_delay;
> Â};
>
> Â#endif /* __PLAT_PXA_SDHCI_H */
> diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c
> index 8455c46..cf514f8 100644
> --- a/drivers/mmc/host/sdhci-pxa.c
> +++ b/drivers/mmc/host/sdhci-pxa.c
> @@ -32,6 +32,11 @@
> Â#define SD_FIFO_PARAM Â Â Â Â Â0x104
> Â#define DIS_PAD_SD_CLK_GATE Â Â0x400
>
> +#define SD_CLOCK_AND_BURST_SIZE_SETUP Â Â Â Â Â0x10A
> +#define SDCLK_SEL Â Â Â0x100
> +#define SDCLK_DELAY_SHIFT Â Â Â9
> +#define SDCLK_DELAY_MASK Â Â Â 0x1f
> +
> Âstruct sdhci_pxa {
>    Âstruct sdhci_host        *host;
>    Âstruct sdhci_pxa_platdata    *pdata;
> @@ -46,6 +51,26 @@ struct sdhci_pxa {
> Â* SDHCI core callbacks                           Â*
> Â* Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â *
> Â\*****************************************************************************/
> +static void tune_timming(struct sdhci_host *host)
> +{
> + Â Â Â struct sdhci_pxa *pxa = sdhci_priv(host);
> +
> + Â Â Â /*
> + Â Â Â Â* tune timming of read data/command when crc error happen
> + Â Â Â Â* No perfermance impact
> + Â Â Â Â*/
> + Â Â Â if (pxa->pdata && 0 != pxa->pdata->clk_delay) {
> + Â Â Â Â Â Â Â u16 tmp;
> +
> + Â Â Â Â Â Â Â tmp = readw(host->ioaddr + SD_CLOCK_AND_BURST_SIZE_SETUP);
> + Â Â Â Â Â Â Â tmp |= (pxa->pdata->clk_delay & SDCLK_DELAY_MASK)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â << SDCLK_DELAY_SHIFT;
delay of cycles or nanoseconds or microseconds? I think it would be more
readable to have something like:
.clk_delay_cycles or .clk_delay_ns or .clk_delay_ms and do a bit
translation here,
or you may want to add some comments to the unit of this delay.
And is this necessary to adjust the timing every time a set_clock is done? This
looks like a non-set-clock related action. Wondering whether it's
feasible to put
this in initialization.
> + Â Â Â Â Â Â Â tmp |= SDCLK_SEL;
> + Â Â Â Â Â Â Â writew(tmp, host->ioaddr + SD_CLOCK_AND_BURST_SIZE_SETUP);
> + Â Â Â }
> +
unnecessary new line here
> +}
> +
> Âstatic void set_clock(struct sdhci_host *host, unsigned int clock)
> Â{
> Â Â Â Âstruct sdhci_pxa *pxa = sdhci_priv(host);
> @@ -57,15 +82,16 @@ static void set_clock(struct sdhci_host *host,
> unsigned int clock)
> Â Â Â Â Â Â Â Â Â Â Â Â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;
> + Â Â Â Â Â Â Â tune_timming(host);
> + Â Â Â Â Â Â Â if (pxa->pdata &&
> + Â Â Â Â Â Â Â Â Â Â Â pxa->pdata->flags & PXA_FLAG_DISABLE_CLOCK_GATING) {
I guess instead of having this if () statement everywhere, you may
want to encode
the information from pdata to 'struct sdhci_pxa' instead. And you also need to
consider the case if pxa->pdata == NULL is allowed, and if it's allowed, what
is the default action.
> +
> + Â Â Â Â Â Â Â Â Â Â Â 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;
> Â Â Â Â}
> Â}
>
> @@ -138,13 +164,13 @@ static int __devinit sdhci_pxa_probe(struct
> platform_device *pdev)
> Â Â Â Âhost->irq = irq;
> Â Â Â Âhost->quirks = SDHCI_QUIRK_BROKEN_ADMA | SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>
> - Â Â Â if (pxa->pdata->flags & PXA_FLAG_CARD_PERMANENT) {
> + Â Â Â if (pdata && pdata->flags & PXA_FLAG_CARD_PERMANENT) {
> Â Â Â Â Â Â Â Â/* on-chip device */
> Â Â Â Â Â Â Â Âhost->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> Â Â Â Â Â Â Â Âhost->mmc->caps |= MMC_CAP_NONREMOVABLE;
> Â Â Â Â}
>
> - Â Â Â if (pdata->quirks)
> + Â Â Â if (pdata && pdata->quirks)
> Â Â Â Â Â Â Â Âhost->quirks |= pdata->quirks;
>
> Â Â Â Âret = sdhci_add_host(host);
> @@ -153,9 +179,8 @@ static int __devinit sdhci_pxa_probe(struct
> platform_device *pdev)
> Â Â Â Â Â Â Â Âgoto out;
> Â Â Â Â}
>
> - Â Â Â if (pxa->pdata->max_speed)
> - Â Â Â Â Â Â Â host->mmc->f_max = pxa->pdata->max_speed;
> -
> + Â Â Â if (pdata && pdata->max_speed)
> + Â Â Â Â Â Â Â host->mmc->f_max = pdata->max_speed;
> Â Â Â Âplatform_set_drvdata(pdev, host);
>
> Â Â Â Âreturn 0;
> --
> 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
