* [PATCH v2 1/1]sdhci-pxa: support tune_timming for various cards
@ 2010-11-11 13:19 zhangfei gao
2010-11-16 5:52 ` Haojian Zhuang
2010-12-05 3:30 ` Chris Ball
0 siblings, 2 replies; 3+ messages in thread
From: zhangfei gao @ 2010-11-11 13:19 UTC (permalink / raw)
To: linux-mmc; +Cc: Chris Ball, Eric Miao, Haojian Zhuang
[-- Attachment #1: Type: text/plain, Size: 4789 bytes --]
Hi, Chirs & Eric
Thanks for review, here is updated version.
1. After clk_gating is enabled, set_clock will transfer clock=0, so
clk_disable will be called, currently set_clock will never transfer
clock=0.
Later tune_timing only occurs once clock is started, currently it will
happen when clock is changed.
2. if pdata is NULL, on-chip device can not be detected, but SD card
is workable, though some card may have crc error at 50M.
>From 553b84abee56f36ee166564f7ce3c617950e194f Mon Sep 17 00:00:00 2001
From: Zhangfei Gao <zhangfei.gao@marvell.com>
Date: Tue, 2 Nov 2010 07:37:44 -0400
Subject: [PATCH] sdhci-pxa: support tune_timing for various cards
1. Add pdata check, in case pdata is NULL
2. Add tune_timing to adjust read data/command timing 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@marvell.com>
---
arch/arm/plat-pxa/include/plat/sdhci.h | 2 +
drivers/mmc/host/sdhci-pxa.c | 47 +++++++++++++++++++++++--------
2 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h
b/arch/arm/plat-pxa/include/plat/sdhci.h
index fc5ceab..f6f46db 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_cycles: 1 ~ 0x1f, each step is roughly 100ps, for tuning timing
*/
struct sdhci_pxa_platdata {
unsigned int max_speed;
unsigned int quirks;
unsigned int flags;
+ unsigned int clk_delay_cycles;
};
#endif /* __PLAT_PXA_SDHCI_H */
diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c
index 8455c46..2b665e4 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,9 +51,28 @@ struct sdhci_pxa {
* SDHCI core callbacks *
* *
\*****************************************************************************/
+static inline void tune_timing(struct sdhci_host *host,
+ struct sdhci_pxa_platdata *pdata)
+{
+ /*
+ * tune timing of read data/command when crc error happen
+ * no performance impact
+ */
+ if (pdata && 0 != pdata->clk_delay_cycles) {
+ u16 tmp;
+
+ tmp = readw(host->ioaddr + SD_CLOCK_AND_BURST_SIZE_SETUP);
+ tmp |= (pdata->clk_delay_cycles & SDCLK_DELAY_MASK)
+ << SDCLK_DELAY_SHIFT;
+ tmp |= SDCLK_SEL;
+ writew(tmp, host->ioaddr + SD_CLOCK_AND_BURST_SIZE_SETUP);
+ }
+}
+
static void set_clock(struct sdhci_host *host, unsigned int clock)
{
struct sdhci_pxa *pxa = sdhci_priv(host);
+ struct sdhci_pxa_platdata *pdata = pxa->pdata;
u32 tmp = 0;
if (clock == 0) {
@@ -57,15 +81,14 @@ 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_timing(host, pdata);
+ if (pdata && 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;
}
}
@@ -138,13 +161,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,8 +176,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);
--
1.7.0.4
[-- Attachment #2: 0001-sdhci-pxa-support-tune_timing-for-various-cards.patch --]
[-- Type: text/x-patch, Size: 4362 bytes --]
From 553b84abee56f36ee166564f7ce3c617950e194f Mon Sep 17 00:00:00 2001
From: Zhangfei Gao <zhangfei.gao@marvell.com>
Date: Tue, 2 Nov 2010 07:37:44 -0400
Subject: [PATCH] sdhci-pxa: support tune_timing for various cards
1. Add pdata check, in case pdata is NULL
2. Add tune_timing to adjust read data/command timing 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@marvell.com>
---
arch/arm/plat-pxa/include/plat/sdhci.h | 2 +
drivers/mmc/host/sdhci-pxa.c | 47 +++++++++++++++++++++++--------
2 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h b/arch/arm/plat-pxa/include/plat/sdhci.h
index fc5ceab..f6f46db 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_cycles: 1 ~ 0x1f, each step is roughly 100ps, for tuning timing
*/
struct sdhci_pxa_platdata {
unsigned int max_speed;
unsigned int quirks;
unsigned int flags;
+ unsigned int clk_delay_cycles;
};
#endif /* __PLAT_PXA_SDHCI_H */
diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c
index 8455c46..2b665e4 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,9 +51,28 @@ struct sdhci_pxa {
* SDHCI core callbacks *
* *
\*****************************************************************************/
+static inline void tune_timing(struct sdhci_host *host,
+ struct sdhci_pxa_platdata *pdata)
+{
+ /*
+ * tune timing of read data/command when crc error happen
+ * no performance impact
+ */
+ if (pdata && 0 != pdata->clk_delay_cycles) {
+ u16 tmp;
+
+ tmp = readw(host->ioaddr + SD_CLOCK_AND_BURST_SIZE_SETUP);
+ tmp |= (pdata->clk_delay_cycles & SDCLK_DELAY_MASK)
+ << SDCLK_DELAY_SHIFT;
+ tmp |= SDCLK_SEL;
+ writew(tmp, host->ioaddr + SD_CLOCK_AND_BURST_SIZE_SETUP);
+ }
+}
+
static void set_clock(struct sdhci_host *host, unsigned int clock)
{
struct sdhci_pxa *pxa = sdhci_priv(host);
+ struct sdhci_pxa_platdata *pdata = pxa->pdata;
u32 tmp = 0;
if (clock == 0) {
@@ -57,15 +81,14 @@ 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_timing(host, pdata);
+ if (pdata && 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;
}
}
@@ -138,13 +161,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,8 +176,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);
--
1.7.0.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/1]sdhci-pxa: support tune_timming for various cards
2010-11-11 13:19 [PATCH v2 1/1]sdhci-pxa: support tune_timming for various cards zhangfei gao
@ 2010-11-16 5:52 ` Haojian Zhuang
2010-12-05 3:30 ` Chris Ball
1 sibling, 0 replies; 3+ messages in thread
From: Haojian Zhuang @ 2010-11-16 5:52 UTC (permalink / raw)
To: zhangfei gao; +Cc: linux-mmc, Chris Ball, Eric Miao
On Thu, Nov 11, 2010 at 9:19 PM, zhangfei gao <zhangfei.gao@gmail.com> wrote:
> Hi, Chirs & Eric
>
> Thanks for review, here is updated version.
> 1. After clk_gating is enabled, set_clock will transfer clock=0, so
> clk_disable will be called, currently set_clock will never transfer
> clock=0.
> Later tune_timing only occurs once clock is started, currently it will
> happen when clock is changed.
>
> 2. if pdata is NULL, on-chip device can not be detected, but SD card
> is workable, though some card may have crc error at 50M.
>
>
> From 553b84abee56f36ee166564f7ce3c617950e194f Mon Sep 17 00:00:00 2001
> From: Zhangfei Gao <zhangfei.gao@marvell.com>
> Date: Tue, 2 Nov 2010 07:37:44 -0400
> Subject: [PATCH] sdhci-pxa: support tune_timing for various cards
>
> 1. Add pdata check, in case pdata is NULL
> 2. Add tune_timing to adjust read data/command timing 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@marvell.com>
> ---
> arch/arm/plat-pxa/include/plat/sdhci.h | 2 +
> drivers/mmc/host/sdhci-pxa.c | 47 +++++++++++++++++++++++--------
> 2 files changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/plat-pxa/include/plat/sdhci.h
> b/arch/arm/plat-pxa/include/plat/sdhci.h
> index fc5ceab..f6f46db 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_cycles: 1 ~ 0x1f, each step is roughly 100ps, for tuning timing
> */
> struct sdhci_pxa_platdata {
> unsigned int max_speed;
> unsigned int quirks;
> unsigned int flags;
> + unsigned int clk_delay_cycles;
> };
>
> #endif /* __PLAT_PXA_SDHCI_H */
> diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c
> index 8455c46..2b665e4 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,9 +51,28 @@ struct sdhci_pxa {
> * SDHCI core callbacks *
> * *
> \*****************************************************************************/
> +static inline void tune_timing(struct sdhci_host *host,
> + struct sdhci_pxa_platdata *pdata)
> +{
> + /*
> + * tune timing of read data/command when crc error happen
> + * no performance impact
> + */
> + if (pdata && 0 != pdata->clk_delay_cycles) {
> + u16 tmp;
> +
> + tmp = readw(host->ioaddr + SD_CLOCK_AND_BURST_SIZE_SETUP);
> + tmp |= (pdata->clk_delay_cycles & SDCLK_DELAY_MASK)
> + << SDCLK_DELAY_SHIFT;
> + tmp |= SDCLK_SEL;
> + writew(tmp, host->ioaddr + SD_CLOCK_AND_BURST_SIZE_SETUP);
> + }
> +}
> +
> static void set_clock(struct sdhci_host *host, unsigned int clock)
> {
> struct sdhci_pxa *pxa = sdhci_priv(host);
> + struct sdhci_pxa_platdata *pdata = pxa->pdata;
> u32 tmp = 0;
>
> if (clock == 0) {
> @@ -57,15 +81,14 @@ 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_timing(host, pdata);
> + if (pdata && 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;
> }
> }
>
> @@ -138,13 +161,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,8 +176,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);
>
> --
> 1.7.0.4
>
Acked.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/1]sdhci-pxa: support tune_timming for various cards
2010-11-11 13:19 [PATCH v2 1/1]sdhci-pxa: support tune_timming for various cards zhangfei gao
2010-11-16 5:52 ` Haojian Zhuang
@ 2010-12-05 3:30 ` Chris Ball
1 sibling, 0 replies; 3+ messages in thread
From: Chris Ball @ 2010-12-05 3:30 UTC (permalink / raw)
To: zhangfei gao; +Cc: linux-mmc, Eric Miao, Haojian Zhuang
Hi Zhangfei,
On Thu, Nov 11, 2010 at 08:19:12AM -0500, zhangfei gao wrote:
> Hi, Chirs & Eric
>
> Thanks for review, here is updated version.
> 1. After clk_gating is enabled, set_clock will transfer clock=0, so
> clk_disable will be called, currently set_clock will never transfer
> clock=0.
> Later tune_timing only occurs once clock is started, currently it will
> happen when clock is changed.
There are still some review comments that haven't been replied to yet:
* Eric asked whether you really need to tune on every clock change, or
if once at initialization time would be enough.
* I asked why we shouldn't just inline tune_timing() at its callsite,
since it's a void function that's only called from one place.
* Philip points out that SD_CLOCK_AND_BURST_SIZE_SETUP is an MMP2-only
register and should be marked as such, and I agree. Even *if*
sdhci-pxa wasn't going to support non-MMP2 SoCs¹, you should still
mark hardware-specific registers with the hardware they're used on.
Speaking generally, please always reply to review comments -- even if
it's just to explain why you considered a suggested change and aren't
going to make it.
Thanks,
- Chris.
¹: (.. which doesn't seem to be the case, since you've since posted a
patchset that generalizes the driver.)
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-12-05 3:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-11-11 13:19 [PATCH v2 1/1]sdhci-pxa: support tune_timming for various cards zhangfei gao
2010-11-16 5:52 ` Haojian Zhuang
2010-12-05 3:30 ` Chris Ball
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox