linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mmc: enable UHS voltage switch for SDSC if supported
@ 2021-04-22 13:18 Christian Löhle
  2021-04-23  1:34 ` [PATCH] mmc: enable UHS voltage switch for SDSC if supported【请注意,邮件由linux-mmc-owner@vger.kernel.org代发】 Shawn Lin
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Christian Löhle @ 2021-04-22 13:18 UTC (permalink / raw)
  To: linux-kernel, linux-mmc, Christian Löhle, ulf.hansson
  Cc: pali, huyue2, tiantao6

Ignore the reported capacity if the card otherwise reports UHS support.

Currently SDSC cards reporting UHS support except for the CCS do not run
through the voltage switch.
While strictly speaking a SDSC card cannot support UHS in compliance
with the standard, there is no good reason to throttle them that way.
Especially for pSLCs in practice such cards benefit greatly by this patch,
as they can be new and UHS supporting, but must not lie about their CCS.
The behaviour of linux-mmc for SDSC is deviating from the standard anyway
in such a case, as the card is treated as UHS card not supporting the
voltage switch in general.
Such a card will come up as
mmc0: new ultra high speed SDR25 SD card at address 0001.
Thus the subsystem will support CMD23 and others to the card.
But if we deviate from the standard anyway, then we might as well
not throttle SDSC to 25MB/s.

Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
---
 drivers/mmc/core/sd.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 6fa51a6ed058..281ca2da8e0b 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -841,11 +841,10 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
 		return err;
 
 	/*
-	 * In case CCS and S18A in the response is set, start Signal Voltage
-	 * Switch procedure. SPI mode doesn't support CMD11.
+	 * In case S18A in the response is set, start Signal Voltage Switch
+	 * procedure. SPI mode doesn't support CMD11.
 	 */
-	if (!mmc_host_is_spi(host) && rocr &&
-	   ((*rocr & 0x41000000) == 0x41000000)) {
+	if (!mmc_host_is_spi(host) && rocr && (*rocr & 0x01000000)) {
 		err = mmc_set_uhs_voltage(host, pocr);
 		if (err == -EAGAIN) {
 			retries--;
-- 
2.31.1

Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported【请注意,邮件由linux-mmc-owner@vger.kernel.org代发】
  2021-04-22 13:18 [PATCH] mmc: enable UHS voltage switch for SDSC if supported Christian Löhle
@ 2021-04-23  1:34 ` Shawn Lin
  2021-04-23  6:38   ` Christian Löhle
  2021-04-30 10:45 ` [PATCH] mmc: enable UHS voltage switch for SDSC if supported Christian Löhle
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Shawn Lin @ 2021-04-23  1:34 UTC (permalink / raw)
  To: Christian Löhle
  Cc: shawn.lin, pali, huyue2, tiantao6, linux-mmc, linux-kernel, ulf.hansson

On 2021/4/22 21:18, Christian Löhle wrote:
> Ignore the reported capacity if the card otherwise reports UHS support.
> 
> Currently SDSC cards reporting UHS support except for the CCS do not run
> through the voltage switch.
> While strictly speaking a SDSC card cannot support UHS in compliance
> with the standard, there is no good reason to throttle them that way.
> Especially for pSLCs in practice such cards benefit greatly by this patch,

Cduld you provide some more infomation about the SDSC cards which ones 
are labeled as UHS, e.g brand, webpage....

> as they can be new and UHS supporting, but must not lie about their CCS.
> The behaviour of linux-mmc for SDSC is deviating from the standard anyway
> in such a case, as the card is treated as UHS card not supporting the
> voltage switch in general.
> Such a card will come up as
> mmc0: new ultra high speed SDR25 SD card at address 0001.
> Thus the subsystem will support CMD23 and others to the card.
> But if we deviate from the standard anyway, then we might as well
> not throttle SDSC to 25MB/s.
> 
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> ---
>   drivers/mmc/core/sd.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 6fa51a6ed058..281ca2da8e0b 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -841,11 +841,10 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
>   		return err;
>   
>   	/*
> -	 * In case CCS and S18A in the response is set, start Signal Voltage
> -	 * Switch procedure. SPI mode doesn't support CMD11.
> +	 * In case S18A in the response is set, start Signal Voltage Switch
> +	 * procedure. SPI mode doesn't support CMD11.
>   	 */
> -	if (!mmc_host_is_spi(host) && rocr &&
> -	   ((*rocr & 0x41000000) == 0x41000000)) {
> +	if (!mmc_host_is_spi(host) && rocr && (*rocr & 0x01000000)) {
>   		err = mmc_set_uhs_voltage(host, pocr);
>   		if (err == -EAGAIN) {
>   			retries--;
> 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported【请注意,邮件由linux-mmc-owner@vger.kernel.org代发】
  2021-04-23  1:34 ` [PATCH] mmc: enable UHS voltage switch for SDSC if supported【请注意,邮件由linux-mmc-owner@vger.kernel.org代发】 Shawn Lin
@ 2021-04-23  6:38   ` Christian Löhle
  2021-04-23  6:51     ` Shawn Lin
  0 siblings, 1 reply; 12+ messages in thread
From: Christian Löhle @ 2021-04-23  6:38 UTC (permalink / raw)
  To: Shawn Lin; +Cc: pali, huyue2, tiantao6, linux-mmc, linux-kernel, ulf.hansson

Claiming UHS-I support with SDSC is pretty rare, for obvious reasons (namely they are non-standard, so it's a gamble if a host makes use of it), but here you go:
https://de.rs-online.com/web/p/micro-sd-karten/8743910/
(sorry for the german, the site has a en. subpage, but it's certificate seem untrusted.)
The more likely case is that they do not advertise UHS-I to customers,
but the card will show support to the host.
Let's look at this product, just as an example.
https://www.tme.eu/html/EN/goodram-industrial-industrial-sd-sd-micro-memory-cards/ramka_15242_EN_pelny.html
They provide an MLC and pSLC version of each device, with half the capacity naturally.
The 4GB MLC claims UHS-I support, the 2GB pSLC is very likely to be the same controller,
it only claims Class 6.
So either they specifically changed the firmware to remove any UHS support indication,
or, more likely IMO, the card still reports UHS support to the host.
Such a card would benefit from this patch.

Disclaimer: I do not own the product and have not confirmed this device in any way,
but I have seen UHS support on other devices that do not advertise it for their
SDSC versions.
If the mailing list would be interested, I can try to acquire such devices, listed in
public open stores. Modern, potentially UHS supporting cards. are,
as mentioned in the initial Mail, often SLC or pSLC and often are made on
a customers request.


From: Shawn Lin <shawn.lin@rock-chips.com>
Sent: Friday, April 23, 2021 3:34 AM
To: Christian Löhle <CLoehle@hyperstone.com>
Cc: shawn.lin@rock-chips.com <shawn.lin@rock-chips.com>; pali@kernel.org <pali@kernel.org>; huyue2@yulong.com <huyue2@yulong.com>; tiantao6@hisilicon.com <tiantao6@hisilicon.com>; linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; ulf.hansson@linaro.org <ulf.hansson@linaro.org>
Subject: Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported【请注意,邮件由linux-mmc-owner@vger.kernel.org代发】 
 
On 2021/4/22 21:18, Christian Löhle wrote:
> Ignore the reported capacity if the card otherwise reports UHS support.
> 
> Currently SDSC cards reporting UHS support except for the CCS do not run
> through the voltage switch.
> While strictly speaking a SDSC card cannot support UHS in compliance
> with the standard, there is no good reason to throttle them that way.
> Especially for pSLCs in practice such cards benefit greatly by this patch,

Cduld you provide some more infomation about the SDSC cards which ones 
are labeled as UHS, e.g brand, webpage....

> as they can be new and UHS supporting, but must not lie about their CCS.
> The behaviour of linux-mmc for SDSC is deviating from the standard anyway
> in such a case, as the card is treated as UHS card not supporting the
> voltage switch in general.
> Such a card will come up as
> mmc0: new ultra high speed SDR25 SD card at address 0001.
> Thus the subsystem will support CMD23 and others to the card.
> But if we deviate from the standard anyway, then we might as well
> not throttle SDSC to 25MB/s.
> 
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> ---
>   drivers/mmc/core/sd.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 6fa51a6ed058..281ca2da8e0b 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -841,11 +841,10 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
>                return err;
>   
>        /*
> -      * In case CCS and S18A in the response is set, start Signal Voltage
> -      * Switch procedure. SPI mode doesn't support CMD11.
> +      * In case S18A in the response is set, start Signal Voltage Switch
> +      * procedure. SPI mode doesn't support CMD11.
>         */
> -     if (!mmc_host_is_spi(host) && rocr &&
> -        ((*rocr & 0x41000000) == 0x41000000)) {
> +     if (!mmc_host_is_spi(host) && rocr && (*rocr & 0x01000000)) {
>                err = mmc_set_uhs_voltage(host, pocr);
>                if (err == -EAGAIN) {
>                        retries--;
> 


Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported【请注意,邮件由linux-mmc-owner@vger.kernel.org代发】
  2021-04-23  6:38   ` Christian Löhle
@ 2021-04-23  6:51     ` Shawn Lin
  0 siblings, 0 replies; 12+ messages in thread
From: Shawn Lin @ 2021-04-23  6:51 UTC (permalink / raw)
  To: Christian Löhle
  Cc: shawn.lin, pali, huyue2, tiantao6, linux-mmc, linux-kernel, ulf.hansson

On 2021/4/23 14:38, Christian Löhle wrote:
> Claiming UHS-I support with SDSC is pretty rare, for obvious reasons (namely they are non-standard, so it's a gamble if a host makes use of it), but here you go:
> https://de.rs-online.com/web/p/micro-sd-karten/8743910/
> (sorry for the german, the site has a en. subpage, but it's certificate seem untrusted.)
> The more likely case is that they do not advertise UHS-I to customers,
> but the card will show support to the host.
> Let's look at this product, just as an example.
> https://www.tme.eu/html/EN/goodram-industrial-industrial-sd-sd-micro-memory-cards/ramka_15242_EN_pelny.html
> They provide an MLC and pSLC version of each device, with half the capacity naturally.
> The 4GB MLC claims UHS-I support, the 2GB pSLC is very likely to be the same controller,
> it only claims Class 6.
> So either they specifically changed the firmware to remove any UHS support indication,
> or, more likely IMO, the card still reports UHS support to the host.
> Such a card would benefit from this patch.
> 
> Disclaimer: I do not own the product and have not confirmed this device in any way,
> but I have seen UHS support on other devices that do not advertise it for their
> SDSC versions.
> If the mailing list would be interested, I can try to acquire such devices, listed in
> public open stores. Modern, potentially UHS supporting cards. are,
> as mentioned in the initial Mail, often SLC or pSLC and often are made on
> a customers request.
> 
> 

Thanks for sharing these, but from the datasheets, these SDSC cards
claims their throught up to 20MB/s. So, I guess highpseed mode is enough
for them? what do you think?

> From: Shawn Lin <shawn.lin@rock-chips.com>
> Sent: Friday, April 23, 2021 3:34 AM
> To: Christian Löhle <CLoehle@hyperstone.com>
> Cc: shawn.lin@rock-chips.com <shawn.lin@rock-chips.com>; pali@kernel.org <pali@kernel.org>; huyue2@yulong.com <huyue2@yulong.com>; tiantao6@hisilicon.com <tiantao6@hisilicon.com>; linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; ulf.hansson@linaro.org <ulf.hansson@linaro.org>
> Subject: Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported【请注意,邮件由linux-mmc-owner@vger.kernel.org代发】
>   
> On 2021/4/22 21:18, Christian Löhle wrote:
>> Ignore the reported capacity if the card otherwise reports UHS support.
>>
>> Currently SDSC cards reporting UHS support except for the CCS do not run
>> through the voltage switch.
>> While strictly speaking a SDSC card cannot support UHS in compliance
>> with the standard, there is no good reason to throttle them that way.
>> Especially for pSLCs in practice such cards benefit greatly by this patch,
> 
> Cduld you provide some more infomation about the SDSC cards which ones
> are labeled as UHS, e.g brand, webpage....
> 
>> as they can be new and UHS supporting, but must not lie about their CCS.
>> The behaviour of linux-mmc for SDSC is deviating from the standard anyway
>> in such a case, as the card is treated as UHS card not supporting the
>> voltage switch in general.
>> Such a card will come up as
>> mmc0: new ultra high speed SDR25 SD card at address 0001.
>> Thus the subsystem will support CMD23 and others to the card.
>> But if we deviate from the standard anyway, then we might as well
>> not throttle SDSC to 25MB/s.
>>
>> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
>> ---
>>     drivers/mmc/core/sd.c | 7 +++----
>>     1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>> index 6fa51a6ed058..281ca2da8e0b 100644
>> --- a/drivers/mmc/core/sd.c
>> +++ b/drivers/mmc/core/sd.c
>> @@ -841,11 +841,10 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
>>                  return err;
>>     
>>          /*
>> -      * In case CCS and S18A in the response is set, start Signal Voltage
>> -      * Switch procedure. SPI mode doesn't support CMD11.
>> +      * In case S18A in the response is set, start Signal Voltage Switch
>> +      * procedure. SPI mode doesn't support CMD11.
>>           */
>> -     if (!mmc_host_is_spi(host) && rocr &&
>> -        ((*rocr & 0x41000000) == 0x41000000)) {
>> +     if (!mmc_host_is_spi(host) && rocr && (*rocr & 0x01000000)) {
>>                  err = mmc_set_uhs_voltage(host, pocr);
>>                  if (err == -EAGAIN) {
>>                          retries--;
>>
> 
> 
> Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
> Managing Directors: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782
> 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported
  2021-04-22 13:18 [PATCH] mmc: enable UHS voltage switch for SDSC if supported Christian Löhle
  2021-04-23  1:34 ` [PATCH] mmc: enable UHS voltage switch for SDSC if supported【请注意,邮件由linux-mmc-owner@vger.kernel.org代发】 Shawn Lin
@ 2021-04-30 10:45 ` Christian Löhle
  2021-05-05 10:09 ` Ulf Hansson
  2021-05-12 16:03 ` [PATCH v2] " Christian Löhle
  3 siblings, 0 replies; 12+ messages in thread
From: Christian Löhle @ 2021-04-30 10:45 UTC (permalink / raw)
  To: linux-kernel, linux-mmc, ulf.hansson; +Cc: pali, huyue2, tiantao6

So are there any other concerns, comments or opinions about this?


From: Christian Löhle <CLoehle@hyperstone.com>
Sent: Thursday, April 22, 2021 3:18 PM
To: linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>; Christian Löhle <CLoehle@hyperstone.com>; ulf.hansson@linaro.org <ulf.hansson@linaro.org>
Cc: pali@kernel.org <pali@kernel.org>; huyue2@yulong.com <huyue2@yulong.com>; tiantao6@hisilicon.com <tiantao6@hisilicon.com>
Subject: [PATCH] mmc: enable UHS voltage switch for SDSC if supported 
 
Ignore the reported capacity if the card otherwise reports UHS support.

Currently SDSC cards reporting UHS support except for the CCS do not run
through the voltage switch.
While strictly speaking a SDSC card cannot support UHS in compliance
with the standard, there is no good reason to throttle them that way.
Especially for pSLCs in practice such cards benefit greatly by this patch,
as they can be new and UHS supporting, but must not lie about their CCS.
The behaviour of linux-mmc for SDSC is deviating from the standard anyway
in such a case, as the card is treated as UHS card not supporting the
voltage switch in general.
Such a card will come up as
mmc0: new ultra high speed SDR25 SD card at address 0001.
Thus the subsystem will support CMD23 and others to the card.
But if we deviate from the standard anyway, then we might as well
not throttle SDSC to 25MB/s.

Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
---
 drivers/mmc/core/sd.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 6fa51a6ed058..281ca2da8e0b 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -841,11 +841,10 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
                 return err;
 
         /*
-        * In case CCS and S18A in the response is set, start Signal Voltage
-        * Switch procedure. SPI mode doesn't support CMD11.
+        * In case S18A in the response is set, start Signal Voltage Switch
+        * procedure. SPI mode doesn't support CMD11.
          */
-       if (!mmc_host_is_spi(host) && rocr &&
-          ((*rocr & 0x41000000) == 0x41000000)) {
+       if (!mmc_host_is_spi(host) && rocr && (*rocr & 0x01000000)) {
                 err = mmc_set_uhs_voltage(host, pocr);
                 if (err == -EAGAIN) {
                         retries--;
-- 
2.31.1
Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported
  2021-04-22 13:18 [PATCH] mmc: enable UHS voltage switch for SDSC if supported Christian Löhle
  2021-04-23  1:34 ` [PATCH] mmc: enable UHS voltage switch for SDSC if supported【请注意,邮件由linux-mmc-owner@vger.kernel.org代发】 Shawn Lin
  2021-04-30 10:45 ` [PATCH] mmc: enable UHS voltage switch for SDSC if supported Christian Löhle
@ 2021-05-05 10:09 ` Ulf Hansson
  2021-05-05 12:12   ` Christian Löhle
  2021-05-12 16:03 ` [PATCH v2] " Christian Löhle
  3 siblings, 1 reply; 12+ messages in thread
From: Ulf Hansson @ 2021-05-05 10:09 UTC (permalink / raw)
  To: Christian Löhle; +Cc: linux-kernel, linux-mmc, pali, huyue2, tiantao6

On Thu, 22 Apr 2021 at 15:18, Christian Löhle <CLoehle@hyperstone.com> wrote:
>
> Ignore the reported capacity if the card otherwise reports UHS support.
>
> Currently SDSC cards reporting UHS support except for the CCS do not run
> through the voltage switch.
> While strictly speaking a SDSC card cannot support UHS in compliance
> with the standard, there is no good reason to throttle them that way.

Maybe not, but I think the code just tries to conform to the SD spec.

> Especially for pSLCs in practice such cards benefit greatly by this patch,
> as they can be new and UHS supporting, but must not lie about their CCS.

Can you provide some concrete examples of SDSC cards that support UHS-I?

> The behaviour of linux-mmc for SDSC is deviating from the standard anyway
> in such a case, as the card is treated as UHS card not supporting the
> voltage switch in general.
> Such a card will come up as
> mmc0: new ultra high speed SDR25 SD card at address 0001.
> Thus the subsystem will support CMD23 and others to the card.
> But if we deviate from the standard anyway, then we might as well
> not throttle SDSC to 25MB/s.

Not sure I understand this correctly? Can you elaborate a bit more, so
I understand how we deviate from the SD spec here?

As far as I understand the spec, SDSC cards shouldn't support UHS-I.
If we decide to deviate from the spec to support this anyway, at least
we need to know about a couple of cards that actually supports this.

Kind regards
Uffe

>
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> ---
>  drivers/mmc/core/sd.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 6fa51a6ed058..281ca2da8e0b 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -841,11 +841,10 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
>                 return err;
>
>         /*
> -        * In case CCS and S18A in the response is set, start Signal Voltage
> -        * Switch procedure. SPI mode doesn't support CMD11.
> +        * In case S18A in the response is set, start Signal Voltage Switch
> +        * procedure. SPI mode doesn't support CMD11.
>          */
> -       if (!mmc_host_is_spi(host) && rocr &&
> -          ((*rocr & 0x41000000) == 0x41000000)) {
> +       if (!mmc_host_is_spi(host) && rocr && (*rocr & 0x01000000)) {
>                 err = mmc_set_uhs_voltage(host, pocr);
>                 if (err == -EAGAIN) {
>                         retries--;
> --
> 2.31.1
>
> Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
> Managing Directors: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported
  2021-05-05 10:09 ` Ulf Hansson
@ 2021-05-05 12:12   ` Christian Löhle
  2021-05-05 14:20     ` Ulf Hansson
  0 siblings, 1 reply; 12+ messages in thread
From: Christian Löhle @ 2021-05-05 12:12 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-kernel, linux-mmc, pali, huyue2, tiantao6

So two cards that advertise UHS:
https://www.euric.de/de/flash-speicher/sd-microsd/sd/
Model SDI790
and
https://na.industrial.panasonic.com/products/storage-media/industrial-sd-cards/lineup/industrial-grade-2d-nand/series/71695
Hopefully that suffices as a couple, otherwise I would buy some
that do not advertise it and actually check if they support it anyway.
But 2GB SLC listings are kind of rare these days, so it's not as
easy to find many that are still sold and available.

My point about deviating from the standard was that these cards
are already initialized as UHS, they just don't do the voltage switch.
So anything reserved for UHS-I (e.g. CMD23 )is already being done
by the host with the current linux-mmc,
so the standard deviation is already there.
Just the voltage switch is missing.

Regards,
Christian


From: Ulf Hansson <ulf.hansson@linaro.org>
Sent: Wednesday, May 5, 2021 12:09 PM
To: Christian Löhle <CLoehle@hyperstone.com>
Subject: Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported 
 
On Thu, 22 Apr 2021 at 15:18, Christian Löhle <CLoehle@hyperstone.com> wrote:
>
> Ignore the reported capacity if the card otherwise reports UHS support.
>
> Currently SDSC cards reporting UHS support except for the CCS do not run
> through the voltage switch.
> While strictly speaking a SDSC card cannot support UHS in compliance
> with the standard, there is no good reason to throttle them that way.

Maybe not, but I think the code just tries to conform to the SD spec.

> Especially for pSLCs in practice such cards benefit greatly by this patch,
> as they can be new and UHS supporting, but must not lie about their CCS.

Can you provide some concrete examples of SDSC cards that support UHS-I?

> The behaviour of linux-mmc for SDSC is deviating from the standard anyway
> in such a case, as the card is treated as UHS card not supporting the
> voltage switch in general.
> Such a card will come up as
> mmc0: new ultra high speed SDR25 SD card at address 0001.
> Thus the subsystem will support CMD23 and others to the card.
> But if we deviate from the standard anyway, then we might as well
> not throttle SDSC to 25MB/s.

Not sure I understand this correctly? Can you elaborate a bit more, so
I understand how we deviate from the SD spec here?

As far as I understand the spec, SDSC cards shouldn't support UHS-I.
If we decide to deviate from the spec to support this anyway, at least
we need to know about a couple of cards that actually supports this.

Kind regards
Uffe

>
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> ---
>  drivers/mmc/core/sd.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 6fa51a6ed058..281ca2da8e0b 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -841,11 +841,10 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
>                 return err;
>
>         /*
> -        * In case CCS and S18A in the response is set, start Signal Voltage
> -        * Switch procedure. SPI mode doesn't support CMD11.
> +        * In case S18A in the response is set, start Signal Voltage Switch
> +        * procedure. SPI mode doesn't support CMD11.
>          */
> -       if (!mmc_host_is_spi(host) && rocr &&
> -          ((*rocr & 0x41000000) == 0x41000000)) {
> +       if (!mmc_host_is_spi(host) && rocr && (*rocr & 0x01000000)) {
>                 err = mmc_set_uhs_voltage(host, pocr);
>                 if (err == -EAGAIN) {
>                         retries--;
> --
> 2.31.1
>
> Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
> Managing Directors: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782
>

Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported
  2021-05-05 12:12   ` Christian Löhle
@ 2021-05-05 14:20     ` Ulf Hansson
  2021-05-06  8:34       ` Christian Löhle
  0 siblings, 1 reply; 12+ messages in thread
From: Ulf Hansson @ 2021-05-05 14:20 UTC (permalink / raw)
  To: Christian Löhle; +Cc: linux-kernel, linux-mmc, pali, huyue2, tiantao6

On Wed, 5 May 2021 at 14:13, Christian Löhle <CLoehle@hyperstone.com> wrote:
>
> So two cards that advertise UHS:
> https://www.euric.de/de/flash-speicher/sd-microsd/sd/
> Model SDI790
> and
> https://na.industrial.panasonic.com/products/storage-media/industrial-sd-cards/lineup/industrial-grade-2d-nand/series/71695

Thanks for the links!

Are these the ones that you have been using for testing?

> Hopefully that suffices as a couple, otherwise I would buy some
> that do not advertise it and actually check if they support it anyway.
> But 2GB SLC listings are kind of rare these days, so it's not as
> easy to find many that are still sold and available.

Right. I have a whole bunch of old SD cards that I can test. I will
let you know when I have got some time to test some of them.

The main worry (but I may be wrong) I have with the patch is that it
could cause problems for other older SDSC cards, which happens to
incorrectly set the S18A bit. I mean, before UHS-I was introduced in
the SD spec, this bit was just being reserved and potentially it could
have *any* value by the card manufacturer. As I said, let me do some
tests of my older SD cards.

>
> My point about deviating from the standard was that these cards
> are already initialized as UHS, they just don't do the voltage switch.
> So anything reserved for UHS-I (e.g. CMD23 )is already being done
> by the host with the current linux-mmc,
> so the standard deviation is already there.
> Just the voltage switch is missing.

Ah, yes you are right!

We call mmc_sd_init_uhs_card() no matter whether the CCS bit is set.
Certainly this is a bug. Both the card and host aren't designed to
work under these conditions/timings.

>
> Regards,
> Christian
>

Kind regards
Uffe

>
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Wednesday, May 5, 2021 12:09 PM
> To: Christian Löhle <CLoehle@hyperstone.com>
> Subject: Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported
>
> On Thu, 22 Apr 2021 at 15:18, Christian Löhle <CLoehle@hyperstone.com> wrote:
> >
> > Ignore the reported capacity if the card otherwise reports UHS support.
> >
> > Currently SDSC cards reporting UHS support except for the CCS do not run
> > through the voltage switch.
> > While strictly speaking a SDSC card cannot support UHS in compliance
> > with the standard, there is no good reason to throttle them that way.
>
> Maybe not, but I think the code just tries to conform to the SD spec.
>
> > Especially for pSLCs in practice such cards benefit greatly by this patch,
> > as they can be new and UHS supporting, but must not lie about their CCS.
>
> Can you provide some concrete examples of SDSC cards that support UHS-I?
>
> > The behaviour of linux-mmc for SDSC is deviating from the standard anyway
> > in such a case, as the card is treated as UHS card not supporting the
> > voltage switch in general.
> > Such a card will come up as
> > mmc0: new ultra high speed SDR25 SD card at address 0001.
> > Thus the subsystem will support CMD23 and others to the card.
> > But if we deviate from the standard anyway, then we might as well
> > not throttle SDSC to 25MB/s.
>
> Not sure I understand this correctly? Can you elaborate a bit more, so
> I understand how we deviate from the SD spec here?
>
> As far as I understand the spec, SDSC cards shouldn't support UHS-I.
> If we decide to deviate from the spec to support this anyway, at least
> we need to know about a couple of cards that actually supports this.
>
> Kind regards
> Uffe
>
> >
> > Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> > ---
> >  drivers/mmc/core/sd.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > index 6fa51a6ed058..281ca2da8e0b 100644
> > --- a/drivers/mmc/core/sd.c
> > +++ b/drivers/mmc/core/sd.c
> > @@ -841,11 +841,10 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
> >                 return err;
> >
> >         /*
> > -        * In case CCS and S18A in the response is set, start Signal Voltage
> > -        * Switch procedure. SPI mode doesn't support CMD11.
> > +        * In case S18A in the response is set, start Signal Voltage Switch
> > +        * procedure. SPI mode doesn't support CMD11.
> >          */
> > -       if (!mmc_host_is_spi(host) && rocr &&
> > -          ((*rocr & 0x41000000) == 0x41000000)) {
> > +       if (!mmc_host_is_spi(host) && rocr && (*rocr & 0x01000000)) {
> >                 err = mmc_set_uhs_voltage(host, pocr);
> >                 if (err == -EAGAIN) {
> >                         retries--;
> > --
> > 2.31.1
> >
> > Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
> > Managing Directors: Dr. Jan Peter Berns.
> > Commercial register of local courts: Freiburg HRB381782
> >
>
> Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
> Managing Directors: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported
  2021-05-05 14:20     ` Ulf Hansson
@ 2021-05-06  8:34       ` Christian Löhle
  2021-05-06 13:54         ` Ulf Hansson
  0 siblings, 1 reply; 12+ messages in thread
From: Christian Löhle @ 2021-05-06  8:34 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-kernel, linux-mmc, pali, huyue2, tiantao6

Well I am definitely for keeping this bug as a feature
and utilizing UHS with this patch ;)
I have tried all SDSC cards I could get my hands on and
have not seen any 'accidentally' reporting UHS support.

I tested the patch with own custom cards.
(Hyperstone, my employer, develops controllers for flash
storage.)

There you would have to make the same choice, disabling
UHS support reporting for all SDSC cards, but as some
hosts accept UHS (including voltage switch) without checking
CCS, this would throttle performance.
(One such host is e.g.
8564:4000 Transcend Information, Inc. microSD/SD/CF UHS-II Card Reader)

It would be nice to have this in linux-mmc as arguably the most
important SD native host.
I'm sure the other manufacturers either do not disable UHS
support for SDSC cards, or have at least been playing with the idea.

Regards,
Christian

From: Ulf Hansson <ulf.hansson@linaro.org>
Sent: Wednesday, May 5, 2021 4:20 PM
To: Christian Löhle <CLoehle@hyperstone.com>
Cc: linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>; pali@kernel.org <pali@kernel.org>; huyue2@yulong.com <huyue2@yulong.com>; tiantao6@hisilicon.com <tiantao6@hisilicon.com>
Subject: Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported 
 
On Wed, 5 May 2021 at 14:13, Christian Löhle <CLoehle@hyperstone.com> wrote:
>
> So two cards that advertise UHS:
> https://www.euric.de/de/flash-speicher/sd-microsd/sd/
> Model SDI790
> and
> https://na.industrial.panasonic.com/products/storage-media/industrial-sd-cards/lineup/industrial-grade-2d-nand/series/71695

Thanks for the links!

Are these the ones that you have been using for testing?

> Hopefully that suffices as a couple, otherwise I would buy some
> that do not advertise it and actually check if they support it anyway.
> But 2GB SLC listings are kind of rare these days, so it's not as
> easy to find many that are still sold and available.

Right. I have a whole bunch of old SD cards that I can test. I will
let you know when I have got some time to test some of them.

The main worry (but I may be wrong) I have with the patch is that it
could cause problems for other older SDSC cards, which happens to
incorrectly set the S18A bit. I mean, before UHS-I was introduced in
the SD spec, this bit was just being reserved and potentially it could
have *any* value by the card manufacturer. As I said, let me do some
tests of my older SD cards.

>
> My point about deviating from the standard was that these cards
> are already initialized as UHS, they just don't do the voltage switch.
> So anything reserved for UHS-I (e.g. CMD23 )is already being done
> by the host with the current linux-mmc,
> so the standard deviation is already there.
> Just the voltage switch is missing.

Ah, yes you are right!

We call mmc_sd_init_uhs_card() no matter whether the CCS bit is set.
Certainly this is a bug. Both the card and host aren't designed to
work under these conditions/timings.

>
> Regards,
> Christian
>

Kind regards
Uffe

>
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Wednesday, May 5, 2021 12:09 PM
> To: Christian Löhle <CLoehle@hyperstone.com>
> Subject: Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported
>
> On Thu, 22 Apr 2021 at 15:18, Christian Löhle <CLoehle@hyperstone.com> wrote:
> >
> > Ignore the reported capacity if the card otherwise reports UHS support.
> >
> > Currently SDSC cards reporting UHS support except for the CCS do not run
> > through the voltage switch.
> > While strictly speaking a SDSC card cannot support UHS in compliance
> > with the standard, there is no good reason to throttle them that way.
>
> Maybe not, but I think the code just tries to conform to the SD spec.
>
> > Especially for pSLCs in practice such cards benefit greatly by this patch,
> > as they can be new and UHS supporting, but must not lie about their CCS.
>
> Can you provide some concrete examples of SDSC cards that support UHS-I?
>
> > The behaviour of linux-mmc for SDSC is deviating from the standard anyway
> > in such a case, as the card is treated as UHS card not supporting the
> > voltage switch in general.
> > Such a card will come up as
> > mmc0: new ultra high speed SDR25 SD card at address 0001.
> > Thus the subsystem will support CMD23 and others to the card.
> > But if we deviate from the standard anyway, then we might as well
> > not throttle SDSC to 25MB/s.
>
> Not sure I understand this correctly? Can you elaborate a bit more, so
> I understand how we deviate from the SD spec here?
>
> As far as I understand the spec, SDSC cards shouldn't support UHS-I.
> If we decide to deviate from the spec to support this anyway, at least
> we need to know about a couple of cards that actually supports this.
>
> Kind regards
> Uffe
>
> >
> > Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> > ---
> >  drivers/mmc/core/sd.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > index 6fa51a6ed058..281ca2da8e0b 100644
> > --- a/drivers/mmc/core/sd.c
> > +++ b/drivers/mmc/core/sd.c
> > @@ -841,11 +841,10 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
> >                 return err;
> >
> >         /*
> > -        * In case CCS and S18A in the response is set, start Signal Voltage
> > -        * Switch procedure. SPI mode doesn't support CMD11.
> > +        * In case S18A in the response is set, start Signal Voltage Switch
> > +        * procedure. SPI mode doesn't support CMD11.
> >          */
> > -       if (!mmc_host_is_spi(host) && rocr &&
> > -          ((*rocr & 0x41000000) == 0x41000000)) {
> > +       if (!mmc_host_is_spi(host) && rocr && (*rocr & 0x01000000)) {
> >                 err = mmc_set_uhs_voltage(host, pocr);
> >                 if (err == -EAGAIN) {
> >                         retries--;
> > --
> > 2.31.1
> >
> > Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
> > Managing Directors: Dr. Jan Peter Berns.
> > Commercial register of local courts: Freiburg HRB381782
> >
>
> Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
> Managing Directors: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782
>

Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported
  2021-05-06  8:34       ` Christian Löhle
@ 2021-05-06 13:54         ` Ulf Hansson
  0 siblings, 0 replies; 12+ messages in thread
From: Ulf Hansson @ 2021-05-06 13:54 UTC (permalink / raw)
  To: Christian Löhle; +Cc: linux-kernel, linux-mmc, pali, huyue2, tiantao6

On Thu, 6 May 2021 at 10:34, Christian Löhle <CLoehle@hyperstone.com> wrote:
>
> Well I am definitely for keeping this bug as a feature
> and utilizing UHS with this patch ;)
> I have tried all SDSC cards I could get my hands on and
> have not seen any 'accidentally' reporting UHS support.

That sounds promising.

>
> I tested the patch with own custom cards.
> (Hyperstone, my employer, develops controllers for flash
> storage.)

I see, thanks for sharing!

>
> There you would have to make the same choice, disabling
> UHS support reporting for all SDSC cards, but as some
> hosts accept UHS (including voltage switch) without checking
> CCS, this would throttle performance.
> (One such host is e.g.
> 8564:4000 Transcend Information, Inc. microSD/SD/CF UHS-II Card Reader)
>
> It would be nice to have this in linux-mmc as arguably the most
> important SD native host.
> I'm sure the other manufacturers either do not disable UHS
> support for SDSC cards, or have at least been playing with the idea.

Assuming I don't find any issues when I am testing some of my older SD
cards, I am definitely open to change into what you propose in
$subject patch.

Although, I think it's important that we also state in the code with a
comment somewhere - that we deliberately have decided to deviate from
the SD spec for the reasons you have pointed out in our discussions.

Can you perhaps prepare and post a new version and possibly also work
a little bit on the commit message to make things a bit more clear?

In the meantime, I will test some SD cards at my side...

Kind regards
Uffe

>
> Regards,
> Christian
>
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Wednesday, May 5, 2021 4:20 PM
> To: Christian Löhle <CLoehle@hyperstone.com>
> Cc: linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-mmc@vger.kernel.org <linux-mmc@vger.kernel.org>; pali@kernel.org <pali@kernel.org>; huyue2@yulong.com <huyue2@yulong.com>; tiantao6@hisilicon.com <tiantao6@hisilicon.com>
> Subject: Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported
>
> On Wed, 5 May 2021 at 14:13, Christian Löhle <CLoehle@hyperstone.com> wrote:
> >
> > So two cards that advertise UHS:
> > https://www.euric.de/de/flash-speicher/sd-microsd/sd/
> > Model SDI790
> > and
> > https://na.industrial.panasonic.com/products/storage-media/industrial-sd-cards/lineup/industrial-grade-2d-nand/series/71695
>
> Thanks for the links!
>
> Are these the ones that you have been using for testing?
>
> > Hopefully that suffices as a couple, otherwise I would buy some
> > that do not advertise it and actually check if they support it anyway.
> > But 2GB SLC listings are kind of rare these days, so it's not as
> > easy to find many that are still sold and available.
>
> Right. I have a whole bunch of old SD cards that I can test. I will
> let you know when I have got some time to test some of them.
>
> The main worry (but I may be wrong) I have with the patch is that it
> could cause problems for other older SDSC cards, which happens to
> incorrectly set the S18A bit. I mean, before UHS-I was introduced in
> the SD spec, this bit was just being reserved and potentially it could
> have *any* value by the card manufacturer. As I said, let me do some
> tests of my older SD cards.
>
> >
> > My point about deviating from the standard was that these cards
> > are already initialized as UHS, they just don't do the voltage switch.
> > So anything reserved for UHS-I (e.g. CMD23 )is already being done
> > by the host with the current linux-mmc,
> > so the standard deviation is already there.
> > Just the voltage switch is missing.
>
> Ah, yes you are right!
>
> We call mmc_sd_init_uhs_card() no matter whether the CCS bit is set.
> Certainly this is a bug. Both the card and host aren't designed to
> work under these conditions/timings.
>
> >
> > Regards,
> > Christian
> >
>
> Kind regards
> Uffe
>
> >
> > From: Ulf Hansson <ulf.hansson@linaro.org>
> > Sent: Wednesday, May 5, 2021 12:09 PM
> > To: Christian Löhle <CLoehle@hyperstone.com>
> > Subject: Re: [PATCH] mmc: enable UHS voltage switch for SDSC if supported
> >
> > On Thu, 22 Apr 2021 at 15:18, Christian Löhle <CLoehle@hyperstone.com> wrote:
> > >
> > > Ignore the reported capacity if the card otherwise reports UHS support.
> > >
> > > Currently SDSC cards reporting UHS support except for the CCS do not run
> > > through the voltage switch.
> > > While strictly speaking a SDSC card cannot support UHS in compliance
> > > with the standard, there is no good reason to throttle them that way.
> >
> > Maybe not, but I think the code just tries to conform to the SD spec.
> >
> > > Especially for pSLCs in practice such cards benefit greatly by this patch,
> > > as they can be new and UHS supporting, but must not lie about their CCS.
> >
> > Can you provide some concrete examples of SDSC cards that support UHS-I?
> >
> > > The behaviour of linux-mmc for SDSC is deviating from the standard anyway
> > > in such a case, as the card is treated as UHS card not supporting the
> > > voltage switch in general.
> > > Such a card will come up as
> > > mmc0: new ultra high speed SDR25 SD card at address 0001.
> > > Thus the subsystem will support CMD23 and others to the card.
> > > But if we deviate from the standard anyway, then we might as well
> > > not throttle SDSC to 25MB/s.
> >
> > Not sure I understand this correctly? Can you elaborate a bit more, so
> > I understand how we deviate from the SD spec here?
> >
> > As far as I understand the spec, SDSC cards shouldn't support UHS-I.
> > If we decide to deviate from the spec to support this anyway, at least
> > we need to know about a couple of cards that actually supports this.
> >
> > Kind regards
> > Uffe
> >
> > >
> > > Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> > > ---
> > >  drivers/mmc/core/sd.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > > index 6fa51a6ed058..281ca2da8e0b 100644
> > > --- a/drivers/mmc/core/sd.c
> > > +++ b/drivers/mmc/core/sd.c
> > > @@ -841,11 +841,10 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
> > >                 return err;
> > >
> > >         /*
> > > -        * In case CCS and S18A in the response is set, start Signal Voltage
> > > -        * Switch procedure. SPI mode doesn't support CMD11.
> > > +        * In case S18A in the response is set, start Signal Voltage Switch
> > > +        * procedure. SPI mode doesn't support CMD11.
> > >          */
> > > -       if (!mmc_host_is_spi(host) && rocr &&
> > > -          ((*rocr & 0x41000000) == 0x41000000)) {
> > > +       if (!mmc_host_is_spi(host) && rocr && (*rocr & 0x01000000)) {
> > >                 err = mmc_set_uhs_voltage(host, pocr);
> > >                 if (err == -EAGAIN) {
> > >                         retries--;
> > > --
> > > 2.31.1
> > >
> > > Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
> > > Managing Directors: Dr. Jan Peter Berns.
> > > Commercial register of local courts: Freiburg HRB381782
> > >
> >
> > Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
> > Managing Directors: Dr. Jan Peter Berns.
> > Commercial register of local courts: Freiburg HRB381782
> >
>
> Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
> Managing Directors: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] mmc: enable UHS voltage switch for SDSC if supported
  2021-04-22 13:18 [PATCH] mmc: enable UHS voltage switch for SDSC if supported Christian Löhle
                   ` (2 preceding siblings ...)
  2021-05-05 10:09 ` Ulf Hansson
@ 2021-05-12 16:03 ` Christian Löhle
  2021-06-04 12:10   ` Ulf Hansson
  3 siblings, 1 reply; 12+ messages in thread
From: Christian Löhle @ 2021-05-12 16:03 UTC (permalink / raw)
  To: linux-kernel, linux-mmc, ulf.hansson; +Cc: pali, huyue2, tiantao6

Ignore the reported capacity if the card otherwise reports UHS support.

Let SDSC cards reporting UHS support (except for the CCS) attempt the
voltage switch.
Up until now such cards would be initialized as UHS,
supporting UHS features SDSC cards are otherwise barred from,
but skip the voltage switch.
While strictly speaking a SDSC card cannot support UHS in compliance
with the standard, there is no good reason to throttle them that way.
Especially for pSLCs in practice such cards benefit greatly,
as they can be new and UHS supporting, but must not lie about their CCS.

Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
---
 drivers/mmc/core/sd.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 2c48d6504101..62d02f1dc924 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -847,11 +847,17 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
 		return err;
 
 	/*
-	 * In case CCS and S18A in the response is set, start Signal Voltage
+	 * In case S18A in the response is set, start Signal Voltage
 	 * Switch procedure. SPI mode doesn't support CMD11.
+	 * Strictly speaking, S18A is not valid if CCS is not set (= not SDSC),
+	 * so one would have to OCR for 0x41000000.
+	 * We choose to ignore this as SDSC cards that report UHS voltage
+	 * support should not be throttled artificially by the standard this
+	 * way.
+	 * SDSC cards that 'accidentally' reporting UHS support by setting the
+	 * reserved bits don't seem to be an issue in practice.
 	 */
-	if (!mmc_host_is_spi(host) && rocr &&
-	   ((*rocr & 0x41000000) == 0x41000000)) {
+	if (!mmc_host_is_spi(host) && rocr && (*rocr & 0x01000000)) {
 		err = mmc_set_uhs_voltage(host, pocr);
 		if (err == -EAGAIN) {
 			retries--;
@@ -1109,7 +1115,14 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 		}
 	}
 
-	/* Initialization sequence for UHS-I cards */
+	/* Initialization sequence for UHS-I cards
+	 * Strictly speaking, S18A in the OCR is only valid if CCS is set, too.
+	 * So SDSC cards should be excluded. We choose to deviate from the
+	 * standard here to allow SDSC cards to utilize UHS if they report
+	 * supporting it.
+	 * Fortunately, SDSC cards reporting S18A and the related bus speed
+	 * modes on accident, by setting reserved bits, don't seem to exist.
+	 */
 	if (rocr & SD_ROCR_S18A && mmc_host_uhs(host)) {
 		err = mmc_sd_init_uhs_card(card);
 		if (err)
-- 
2.31.1

Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] mmc: enable UHS voltage switch for SDSC if supported
  2021-05-12 16:03 ` [PATCH v2] " Christian Löhle
@ 2021-06-04 12:10   ` Ulf Hansson
  0 siblings, 0 replies; 12+ messages in thread
From: Ulf Hansson @ 2021-06-04 12:10 UTC (permalink / raw)
  To: Christian Löhle; +Cc: linux-kernel, linux-mmc, pali, huyue2, tiantao6

On Wed, 12 May 2021 at 18:03, Christian Löhle <CLoehle@hyperstone.com> wrote:
>
> Ignore the reported capacity if the card otherwise reports UHS support.
>
> Let SDSC cards reporting UHS support (except for the CCS) attempt the
> voltage switch.
> Up until now such cards would be initialized as UHS,
> supporting UHS features SDSC cards are otherwise barred from,
> but skip the voltage switch.
> While strictly speaking a SDSC card cannot support UHS in compliance
> with the standard, there is no good reason to throttle them that way.
> Especially for pSLCs in practice such cards benefit greatly,
> as they can be new and UHS supporting, but must not lie about their CCS.
>
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>

Apologize for the delay. I have tested a bunch of my old (and new)
SD/uSD cards, this seems to work fine.

I have applied this for next, but I think this actually should be
considered as bug fix. I let it cook in linux-next for a while, then I
will probably add a stable tag for it.

Thanks and kind regards
Uffe

> ---
>  drivers/mmc/core/sd.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 2c48d6504101..62d02f1dc924 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -847,11 +847,17 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
>                 return err;
>
>         /*
> -        * In case CCS and S18A in the response is set, start Signal Voltage
> +        * In case S18A in the response is set, start Signal Voltage
>          * Switch procedure. SPI mode doesn't support CMD11.
> +        * Strictly speaking, S18A is not valid if CCS is not set (= not SDSC),
> +        * so one would have to OCR for 0x41000000.
> +        * We choose to ignore this as SDSC cards that report UHS voltage
> +        * support should not be throttled artificially by the standard this
> +        * way.
> +        * SDSC cards that 'accidentally' reporting UHS support by setting the
> +        * reserved bits don't seem to be an issue in practice.
>          */
> -       if (!mmc_host_is_spi(host) && rocr &&
> -          ((*rocr & 0x41000000) == 0x41000000)) {
> +       if (!mmc_host_is_spi(host) && rocr && (*rocr & 0x01000000)) {
>                 err = mmc_set_uhs_voltage(host, pocr);
>                 if (err == -EAGAIN) {
>                         retries--;
> @@ -1109,7 +1115,14 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
>                 }
>         }
>
> -       /* Initialization sequence for UHS-I cards */
> +       /* Initialization sequence for UHS-I cards
> +        * Strictly speaking, S18A in the OCR is only valid if CCS is set, too.
> +        * So SDSC cards should be excluded. We choose to deviate from the
> +        * standard here to allow SDSC cards to utilize UHS if they report
> +        * supporting it.
> +        * Fortunately, SDSC cards reporting S18A and the related bus speed
> +        * modes on accident, by setting reserved bits, don't seem to exist.
> +        */
>         if (rocr & SD_ROCR_S18A && mmc_host_uhs(host)) {
>                 err = mmc_sd_init_uhs_card(card);
>                 if (err)
> --
> 2.31.1
>
> Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
> Managing Directors: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-06-04 12:12 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-22 13:18 [PATCH] mmc: enable UHS voltage switch for SDSC if supported Christian Löhle
2021-04-23  1:34 ` [PATCH] mmc: enable UHS voltage switch for SDSC if supported【请注意,邮件由linux-mmc-owner@vger.kernel.org代发】 Shawn Lin
2021-04-23  6:38   ` Christian Löhle
2021-04-23  6:51     ` Shawn Lin
2021-04-30 10:45 ` [PATCH] mmc: enable UHS voltage switch for SDSC if supported Christian Löhle
2021-05-05 10:09 ` Ulf Hansson
2021-05-05 12:12   ` Christian Löhle
2021-05-05 14:20     ` Ulf Hansson
2021-05-06  8:34       ` Christian Löhle
2021-05-06 13:54         ` Ulf Hansson
2021-05-12 16:03 ` [PATCH v2] " Christian Löhle
2021-06-04 12:10   ` Ulf Hansson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).