[PATCH 0/4] sdhci-of: Some fixes for high-speed and 4-bit SD cards

[PATCH 0/4] sdhci-of: Some fixes for high-speed and 4-bit SD cards

[Anton Vorontsov]

Hi all,

Finally I've got a bunch of SD cards to test eSDHC in various ways,
and more importantly now I have a lot of SDHS cards. ;-)

So, here are few fixes that make eSDHC work flawlessly on MPC83xx
SOCs with all SD and MMC cards that I have.

On MPC85xx (namely MPC8536 and MPC8569) SOCs there is one issue:
the cards can be detected and read just fine, but writing doesn't
work (no interrupts received). I'm currently investigating this.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 1/4] sdhci-of: Fix SD clock calculation

[Anton Vorontsov]

Linear divisor's values in a register start at 0 (zero means
"divide by 1"). Before this patch the code didn't account that
fact, so SD cards were running underclocked.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci-of.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of.c b/drivers/mmc/host/sdhci-of.c
index 9088443..92b5667 100644
--- a/drivers/mmc/host/sdhci-of.c
+++ b/drivers/mmc/host/sdhci-of.c
@@ -136,6 +136,7 @@ static void esdhc_set_clock(struct sdhci_host *host, unsigned int clock)
 	}
 
 	pre_div >>= 1;
+	div--;
 
 	setbits32(host->ioaddr + ESDHC_SYSTEM_CONTROL, ESDHC_CLOCK_IPGEN |
 		  ESDHC_CLOCK_HCKEN | ESDHC_CLOCK_PEREN |
-- 
1.6.3.3

[PATCH 2/4] sdhci-of: Avoid writing reserved bits into host control register

[Anton Vorontsov]

SDHCI core tries to write HISPD bit into the host control register,
but the eSDHC controllers don't have that bit, and that causes
all sorts of misbehaviour when using 4-bit mode capable SD cards.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci-of.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of.c b/drivers/mmc/host/sdhci-of.c
index 92b5667..8440fd9 100644
--- a/drivers/mmc/host/sdhci-of.c
+++ b/drivers/mmc/host/sdhci-of.c
@@ -48,6 +48,8 @@ struct sdhci_of_host {
 #define ESDHC_CLOCK_HCKEN	0x00000002
 #define ESDHC_CLOCK_IPGEN	0x00000001
 
+#define ESDHC_HOST_CONTROL_RES	0x05
+
 static u32 esdhc_readl(struct sdhci_host *host, int reg)
 {
 	return in_be32(host->ioaddr + reg);
@@ -109,6 +111,10 @@ static void esdhc_writeb(struct sdhci_host *host, u8 val, int reg)
 	int base = reg & ~0x3;
 	int shift = (reg & 0x3) * 8;
 
+	/* Prevent SDHCI core from writing reserved bits (e.g. HISPD). */
+	if (reg == SDHCI_HOST_CONTROL)
+		val &= ~ESDHC_HOST_CONTROL_RES;
+
 	clrsetbits_be32(host->ioaddr + base , 0xff << shift, val << shift);
 }
 
-- 
1.6.3.3

[PATCH 3/4] sdhci-of: Fix high-speed cards recognition

[Anton Vorontsov]

eSDHC fails to recognize some SDHS cards, throwing timeout errors:

  mmc0: error -110 whilst initialising SD card

That's because we calculate timeout value in a wrong way: on eSDHC
hosts the timeout clock is derivied from the SD clock, which is set
dynamically.

This patch fixes the issue by introducing and implementing
DYNAMIC_TIMEOUT_CLOCK quirk for sdhci-of driver.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci-of.c |    5 ++---
 drivers/mmc/host/sdhci.c    |    4 ++++
 drivers/mmc/host/sdhci.h    |    2 ++
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of.c b/drivers/mmc/host/sdhci-of.c
index 8440fd9..b6ff2e8 100644
--- a/drivers/mmc/host/sdhci-of.c
+++ b/drivers/mmc/host/sdhci-of.c
@@ -174,9 +174,7 @@ static unsigned int esdhc_get_min_clock(struct sdhci_host *host)
 
 static unsigned int esdhc_get_timeout_clock(struct sdhci_host *host)
 {
-	struct sdhci_of_host *of_host = sdhci_priv(host);
-
-	return of_host->clock / 1000;
+	return host->clock / 1000;
 }
 
 static struct sdhci_of_data sdhci_esdhc = {
@@ -185,6 +183,7 @@ static struct sdhci_of_data sdhci_esdhc = {
 		  SDHCI_QUIRK_INVERTED_WRITE_PROTECT |
 		  SDHCI_QUIRK_NO_BUSY_IRQ |
 		  SDHCI_QUIRK_NONSTANDARD_CLOCK |
+		  SDHCI_QUIRK_DYNAMIC_TIMEOUT_CLOCK |
 		  SDHCI_QUIRK_PIO_NEEDS_DELAY |
 		  SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET |
 		  SDHCI_QUIRK_NO_CARD_NO_RESET,
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index fc96f8c..0f273fe 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -591,6 +591,10 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_data *data)
 	target_timeout = data->timeout_ns / 1000 +
 		data->timeout_clks / host->clock;
 
+	if (host->quirks & SDHCI_QUIRK_DYNAMIC_TIMEOUT_CLOCK &&
+			host->ops->get_timeout_clock)
+		host->timeout_clk = host->ops->get_timeout_clock(host);
+
 	/*
 	 * Figure out needed cycles.
 	 * We do this in steps in order to fit inside a 32 bit int.
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index c77e9ff..44b1dcc 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -232,6 +232,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_FORCE_1_BIT_DATA			(1<<22)
 /* Controller needs 10ms delay between applying power and clock */
 #define SDHCI_QUIRK_DELAY_AFTER_POWER			(1<<23)
+/* Controller has dynamic timeout clock management */
+#define SDHCI_QUIRK_DYNAMIC_TIMEOUT_CLOCK		(1<<24)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
-- 
1.6.3.3

[PATCH 4/4] sdhci-of: Cleanup eSDHC's set_clock() a little bit

[Anton Vorontsov]

- Get rid of incomprehensible "if { for { if } }" construction for the
  exponential divisor calculation. The first if statement isn't correct
  at all, since it should check for "host->max_clk / pre_div / 16 >
  clock". The error doesn't cause any bugs because the check in the for
  loop does the right thing, and so the outer check becomes useless;

- For the linear divisor do the same: a single while statement is more
  readable than for + if construction;

- Add dev_dbg() that prints desired and actual clock frequency.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/sdhci-of.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of.c b/drivers/mmc/host/sdhci-of.c
index b6ff2e8..bbdd468 100644
--- a/drivers/mmc/host/sdhci-of.c
+++ b/drivers/mmc/host/sdhci-of.c
@@ -120,8 +120,8 @@ static void esdhc_writeb(struct sdhci_host *host, u8 val, int reg)
 
 static void esdhc_set_clock(struct sdhci_host *host, unsigned int clock)
 {
-	int div;
 	int pre_div = 2;
+	int div = 1;
 
 	clrbits32(host->ioaddr + ESDHC_SYSTEM_CONTROL, ESDHC_CLOCK_IPGEN |
 		  ESDHC_CLOCK_HCKEN | ESDHC_CLOCK_PEREN | ESDHC_CLOCK_MASK);
@@ -129,17 +129,14 @@ static void esdhc_set_clock(struct sdhci_host *host, unsigned int clock)
 	if (clock == 0)
 		goto out;
 
-	if (host->max_clk / 16 > clock) {
-		for (; pre_div < 256; pre_div *= 2) {
-			if (host->max_clk / pre_div < clock * 16)
-				break;
-		}
-	}
+	while (host->max_clk / pre_div / 16 > clock && pre_div < 256)
+		pre_div *= 2;
 
-	for (div = 1; div <= 16; div++) {
-		if (host->max_clk / (div * pre_div) <= clock)
-			break;
-	}
+	while (host->max_clk / pre_div / div > clock && div < 16)
+		div++;
+
+	dev_dbg(mmc_dev(host->mmc), "desired SD clock: %d, actual: %d\n",
+		clock, host->max_clk / pre_div / div);
 
 	pre_div >>= 1;
 	div--;
-- 
1.6.3.3

Re: [PATCH 3/4] sdhci-of: Fix high-speed cards recognition

[David Vrabel]

Anton Vorontsov wrote:
> eSDHC fails to recognize some SDHS cards, throwing timeout errors:
> 
>   mmc0: error -110 whilst initialising SD card
> 
> That's because we calculate timeout value in a wrong way: on eSDHC
> hosts the timeout clock is derivied from the SD clock, which is set
> dynamically.

I've seen an reference design for an SDHC controller do this also.

> +/* Controller has dynamic timeout clock management */
> +#define SDHCI_QUIRK_DYNAMIC_TIMEOUT_CLOCK		(1<<24)

This comment and define would be better if it matched terms used in the
spec.  Suggest:

/* Controller uses SDCLK instead of TMCLK for data timeouts. */
#define SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK  (1 << 24)

David
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/


'member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom'

Re: [PATCH 0/4] sdhci-of: Some fixes for high-speed and 4-bit SD cards

[Anton Vorontsov]

On Fri, Aug 07, 2009 at 08:39:40PM +0400, Anton Vorontsov wrote:
> Hi all,
> 
> Finally I've got a bunch of SD cards to test eSDHC in various ways,
> and more importantly now I have a lot of SDHS cards. ;-)
> 
> So, here are few fixes that make eSDHC work flawlessly on MPC83xx
> SOCs with all SD and MMC cards that I have.
> 
> On MPC85xx (namely MPC8536 and MPC8569) SOCs there is one issue:
> the cards can be detected and read just fine, but writing doesn't
> work (no interrupts received). I'm currently investigating this.

Solved. It appears that eSDHC on MPC85xx has a normal write-protect
reporting. And eSDHC actually checks the WP pin, thus doesn't let
anybody to do any writes... I'll make some additional patches and
will send v2 soon.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 3/4] sdhci-of: Fix high-speed cards recognition

[Anton Vorontsov]

On Fri, Aug 07, 2009 at 06:08:59PM +0100, David Vrabel wrote:
> Anton Vorontsov wrote:
> > eSDHC fails to recognize some SDHS cards, throwing timeout errors:
> > 
> >   mmc0: error -110 whilst initialising SD card
> > 
> > That's because we calculate timeout value in a wrong way: on eSDHC
> > hosts the timeout clock is derivied from the SD clock, which is set
> > dynamically.
> 
> I've seen an reference design for an SDHC controller do this also.

Thanks for the information!

> > +/* Controller has dynamic timeout clock management */
> > +#define SDHCI_QUIRK_DYNAMIC_TIMEOUT_CLOCK		(1<<24)
> 
> This comment and define would be better if it matched terms used in the
> spec.  Suggest:
> 
> /* Controller uses SDCLK instead of TMCLK for data timeouts. */
> #define SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK  (1 << 24)

Yeah, if it's somewhat common scheme, then it makes sense to name the
quirk that way.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2