[PATCH] SD/MMC: fix the issue of SDHC performance regression

[PATCH] SD/MMC: fix the issue of SDHC performance regression

[Qiang Liu]

Low performance of SDHC (half of before) due to its frequency was set to 25MHz,
but not 50MHz (involved by commit id 013909c4ffd16ded4895528b856fd8782df04dc6,
add support for query function modes for uhs cards according to Physical Layer
SPEC V3.01).

Set high speed max frequency according to response status of CMD6, but
not for SPEC Version. Response of switch command is first consideration
factor when set SDHC max working frequency. TRAN_SPEED in CSD register
will be seemed as the second factor.

Signed-off-by: Qiang Liu <qiang.liu@freescale.com>
---
 drivers/mmc/core/sd.c    |    6 +++---
 include/linux/mmc/card.h |    3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index b33537f..5cdab7c 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -306,6 +306,9 @@ static int mmc_read_switch(struct mmc_card *card)
 		goto out;
 	}

+	if (status[13] & SD_MODE_HIGH_SPEED)
+		card->sw_caps.hs_max_dtr = HIGH_SPEED_MAX_DTR;
+
 	if (card->scr.sda_spec3) {
 		card->sw_caps.sd3_bus_mode = status[13];

@@ -348,9 +351,6 @@ static int mmc_read_switch(struct mmc_card *card)
 		}

 		card->sw_caps.sd3_curr_limit = status[7];
-	} else {
-		if (status[13] & 0x02)
-			card->sw_caps.hs_max_dtr = 50000000;
 	}

 out:
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 6ad4355..499a268 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -99,6 +99,7 @@ struct sd_ssr {
 struct sd_switch_caps {
 	unsigned int		hs_max_dtr;
 	unsigned int		uhs_max_dtr;
+#define HIGH_SPEED_MAX_DTR	50000000
 #define UHS_SDR104_MAX_DTR	208000000
 #define UHS_SDR50_MAX_DTR	100000000
 #define UHS_DDR50_MAX_DTR	50000000
@@ -106,11 +107,13 @@ struct sd_switch_caps {
 #define UHS_SDR12_MAX_DTR	25000000
 	unsigned int		sd3_bus_mode;
 #define UHS_SDR12_BUS_SPEED	0
+#define HIGH_SPEED_BUS_SPEED	1
 #define UHS_SDR25_BUS_SPEED	1
 #define UHS_SDR50_BUS_SPEED	2
 #define UHS_SDR104_BUS_SPEED	3
 #define UHS_DDR50_BUS_SPEED	4

+#define SD_MODE_HIGH_SPEED	(1 << HIGH_SPEED_BUS_SPEED)
 #define SD_MODE_UHS_SDR12	(1 << UHS_SDR12_BUS_SPEED)
 #define SD_MODE_UHS_SDR25	(1 << UHS_SDR25_BUS_SPEED)
 #define SD_MODE_UHS_SDR50	(1 << UHS_SDR50_BUS_SPEED)
--
1.6.4

RE: [PATCH] SD/MMC: fix the issue of SDHC performance regression

[Li Yang-R58472]

>-----Original Message-----
>From: Liu Qiang-B32616
>Sent: Monday, November 07, 2011 5:36 PM
>To: linux-mmc@vger.kernel.org
>Cc: cjb@laptop.org; Li Yang-R58472; Gala Kumar-B11780; Liu Qiang-B32616
>Subject: [PATCH] SD/MMC: fix the issue of SDHC performance regression
>
>Low performance of SDHC (half of before) due to its frequency was set to
>25MHz,
>but not 50MHz (involved by commit id
>013909c4ffd16ded4895528b856fd8782df04dc6,
>add support for query function modes for uhs cards according to Physical
>Layer
>SPEC V3.01).
>
>Set high speed max frequency according to response status of CMD6, but
>not for SPEC Version. Response of switch command is first consideration
>factor when set SDHC max working frequency. TRAN_SPEED in CSD register
>will be seemed as the second factor.

I'm curious why many SD cards are not setting the clock frequency correctly in the CSD register.  Anyone has a clue?

- Leo

Re: [PATCH] SD/MMC: fix the issue of SDHC performance regression

[Chris Ball]

Hi,

On Mon, Nov 07 2011, Qiang Liu wrote:
> Low performance of SDHC (half of before) due to its frequency was set to 25MHz,
> but not 50MHz (involved by commit id 013909c4ffd16ded4895528b856fd8782df04dc6,
> add support for query function modes for uhs cards according to Physical Layer
> SPEC V3.01).
>
> Set high speed max frequency according to response status of CMD6, but
> not for SPEC Version. Response of switch command is first consideration
> factor when set SDHC max working frequency. TRAN_SPEED in CSD register
> will be seemed as the second factor.
>
> Signed-off-by: Qiang Liu <qiang.liu@freescale.com>

Have you seen:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=f2815f68dabbb373fd1c9f0fd4a609d486697c2b 
("mmc: sd: Handle SD3.0 cards not supporting UHS-I bus speed mode")

which is already in mainline?  I think your patch is identical.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

RE: [PATCH] SD/MMC: fix the issue of SDHC performance regression

[Liu Qiang-B32616]

-----Original Message-----
From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Chris Ball
Sent: Monday, November 07, 2011 9:20 PM
To: Liu Qiang-B32616
Cc: linux-mmc@vger.kernel.org; Li Yang-R58472; Gala Kumar-B11780
Subject: Re: [PATCH] SD/MMC: fix the issue of SDHC performance regression

Hi,

On Mon, Nov 07 2011, Qiang Liu wrote:
> Low performance of SDHC (half of before) due to its frequency was set 
> to 25MHz, but not 50MHz (involved by commit id 
> 013909c4ffd16ded4895528b856fd8782df04dc6,
> add support for query function modes for uhs cards according to 
> Physical Layer SPEC V3.01).
>
> Set high speed max frequency according to response status of CMD6, but 
> not for SPEC Version. Response of switch command is first 
> consideration factor when set SDHC max working frequency. TRAN_SPEED 
> in CSD register will be seemed as the second factor.
>
> Signed-off-by: Qiang Liu <qiang.liu@freescale.com>

Have you seen:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=f2815f68dabbb373fd1c9f0fd4a609d486697c2b
("mmc: sd: Handle SD3.0 cards not supporting UHS-I bus speed mode")

which is already in mainline?  I think your patch is identical.
[Liu Qiang] Yes, they are identical. Thanks a lot.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SD/MMC: fix the issue of SDHC performance regression

[Aaron Lu]

On Mon, Nov 07, 2011 at 08:20:10AM -0500, Chris Ball wrote:
> Have you seen:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=f2815f68dabbb373fd1c9f0fd4a609d486697c2b 
> ("mmc: sd: Handle SD3.0 cards not supporting UHS-I bus speed mode")
> 
> which is already in mainline?  I think your patch is identical.
Hi Chris,

I think the existing code is somewhat confusing, since SDR50 means
100MHZ frequency while high speed is 50MHZ.
The reason it is correct is UHS_SDR50_BUS_SPEED is defined as 2,
which happened to be the same value as (1 << UHS_SDR25_BUS_SPEED).

How about change it like this:

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index a230e7f..670fd7f 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -306,7 +306,7 @@ static int mmc_read_switch(struct mmc_card *card)
                goto out;
        }
 
-       if (status[13] & UHS_SDR50_BUS_SPEED)
+       if (status[13] & SD_MODE_UHS_SDR25)
                card->sw_caps.hs_max_dtr = 50000000;
 
        if (card->scr.sda_spec3) {

SDR25 is also 50MHZ, the same frequency as high speed.
Or we can add a new macro for high speed like qiang has done,
which one you prefer?

Thanks,
Aaron

Re: [PATCH] SD/MMC: fix the issue of SDHC performance regression

[Chris Ball]

Hi,

On Tue, Nov 08 2011, Aaron Lu wrote:
> SDR25 is also 50MHZ, the same frequency as high speed.
> Or we can add a new macro for high speed like qiang has done,
> which one you prefer?

The new macro makes most sense to me -- if no-one disagrees, I'll
convert Liu's patch into a rename patch and queue it as a cleanup
for 3.2.  Thanks!

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

RE: [PATCH] SD/MMC: fix the issue of SDHC performance regression

[Subhash Jadavani]

Chris, Aaron,

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c index
a230e7f..670fd7f 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -306,7 +306,7 @@ static int mmc_read_switch(struct mmc_card *card)
                goto out;
        }
 
-       if (status[13] & UHS_SDR50_BUS_SPEED)
+       if (status[13] & SD_MODE_UHS_SDR25)
                card->sw_caps.hs_max_dtr = 50000000;
 
Using SD_MODE_UHS_SDR25 macro name is not appropriate here. It's really not
the SDR25 mode (although clock rate is 50mhz), it's High speed mode only.
"Qiang Liu" patch had defined new marco "SD_MODE_HIGH_SPEED" which looks
most appropriate to use here.

@@ -99,6 +99,7 @@ struct sd_ssr {
 struct sd_switch_caps {
 	unsigned int		hs_max_dtr;
 	unsigned int		uhs_max_dtr;
+#define HIGH_SPEED_MAX_DTR	50000000
 #define UHS_SDR104_MAX_DTR	208000000
 #define UHS_SDR50_MAX_DTR	100000000
 #define UHS_DDR50_MAX_DTR	50000000
@@ -106,11 +107,13 @@ struct sd_switch_caps {
 #define UHS_SDR12_MAX_DTR	25000000
 	unsigned int		sd3_bus_mode;
 #define UHS_SDR12_BUS_SPEED	0
+#define HIGH_SPEED_BUS_SPEED	1
 #define UHS_SDR25_BUS_SPEED	1
 #define UHS_SDR50_BUS_SPEED	2
 #define UHS_SDR104_BUS_SPEED	3
 #define UHS_DDR50_BUS_SPEED	4

+#define SD_MODE_HIGH_SPEED	(1 << HIGH_SPEED_BUS_SPEED)
 #define SD_MODE_UHS_SDR12	(1 << UHS_SDR12_BUS_SPEED)
 #define SD_MODE_UHS_SDR25	(1 << UHS_SDR25_BUS_SPEED)
 #define SD_MODE_UHS_SDR50	(1 << UHS_SDR50_BUS_SPEED)
--

Regards,
Subhash


> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Chris Ball
> Sent: Tuesday, November 08, 2011 7:08 PM
> To: Aaron Lu
> Cc: Qiang Liu; linux-mmc@vger.kernel.org; leoli@freescale.com;
> kumar.gala@freescale.com; subhashj@codeaurora.org
> Subject: Re: [PATCH] SD/MMC: fix the issue of SDHC performance
> regression
> 
> Hi,
> 
> On Tue, Nov 08 2011, Aaron Lu wrote:
> > SDR25 is also 50MHZ, the same frequency as high speed.
> > Or we can add a new macro for high speed like qiang has done,
> > which one you prefer?
> 
> The new macro makes most sense to me -- if no-one disagrees, I'll
> convert Liu's patch into a rename patch and queue it as a cleanup
> for 3.2.  Thanks!
> 
> - Chris.
> --
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SD/MMC: fix the issue of SDHC performance regression

[Chris Ball]

Hi,

On Sun, Nov 13 2011, Subhash Jadavani wrote:
> Using SD_MODE_UHS_SDR25 macro name is not appropriate here. It's really not
> the SDR25 mode (although clock rate is 50mhz), it's High speed mode only.
> "Qiang Liu" patch had defined new marco "SD_MODE_HIGH_SPEED" which looks
> most appropriate to use here.

See the version in current mmc-next, it already uses Qiang Liu's macros.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

RE: [PATCH] SD/MMC: fix the issue of SDHC performance regression

[Subhash Jadavani]

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Chris Ball
> Sent: Sunday, November 13, 2011 10:15 PM
> To: Subhash Jadavani
> Cc: 'Aaron Lu'; 'Qiang Liu'; linux-mmc@vger.kernel.org;
> leoli@freescale.com; kumar.gala@freescale.com
> Subject: Re: [PATCH] SD/MMC: fix the issue of SDHC performance
> regression
> 
> Hi,
> 
> On Sun, Nov 13 2011, Subhash Jadavani wrote:
> > Using SD_MODE_UHS_SDR25 macro name is not appropriate here. It's
> really not
> > the SDR25 mode (although clock rate is 50mhz), it's High speed mode
> only.
> > "Qiang Liu" patch had defined new marco "SD_MODE_HIGH_SPEED" which
> looks
> > most appropriate to use here.
> 
> See the version in current mmc-next, it already uses Qiang Liu's
> macros.

Yes, Just checked now. 

Thanks,
Subhash

> 
> Thanks,
> 
> - Chris.
> --
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html