Recognize CSD structure version 3

PATCH Superseded Kyungmin Park 3 participants 6 messages May 31, 2010 – Jun 2, 2010 MMC, SD and SDIO

Share & tools

Kyungmin Park

author

2010-05-31 06:40 UTC #0 lore raw

PATCH 1/1 Recognize CSD structure version 3

The eMMC spec 4.4 and 4.3 + additional feature chips has CSD structure version 3 To probe these chip properly and make it simple.
it doesn't check CSD structure.

Signed-off-by Kyungmin Park <[email protected]>

---

drivers/mmc/core/mmc.c +2 -5

index 89f7a25..9e42bc6 100644
@@ -118,15 +118,12 @@ static int mmc_decode_csd(struct mmc_card *card)
 	u32 *resp = card->raw_csd;
 
 	/*
-	 * We only understand CSD structure v1.1 and v1.2.
+	 * We understand all CSD structure v1.1, v1.2 and v1.3.
 	 * v1.2 has extra information in bits 15, 11 and 10.
 	 */
 	csd_struct = UNSTUFF_BITS(resp, 126, 2);
-	if (csd_struct != 1 && csd_struct != 2) {
-		printk(KERN_ERR "%s: unrecognised CSD structure version %d\n",
+	printk(KERN_DEBUG "%s: recognised CSD structure version %d\n",
 			mmc_hostname(card->host), csd_struct);
-		return -EINVAL;
-	}
 
 	csd->mmca_vsn	 = UNSTUFF_BITS(resp, 122, 4);
 	m = UNSTUFF_BITS(resp, 115, 4);

Ben Dooks

2010-05-31 07:06 UTC #1 lore raw

Quoted 11 lines · Kyungmin Park

The eMMC spec 4.4 and 4.3 + additional feature chips has CSD structure version 3 To probe these chip properly and make it simple.
it doesn't check CSD structure.
Signed-off-by Kyungmin Park <[email protected]>
---
drivers/mmc/core/mmc.c +0 -0
index 89f7a25..9e42bc6 100644

drivers/mmc/core/mmc.c static int mmc_decode_csd(struct mmc_card *card) · line 125

Quoted 9 lines

drivers/mmc/core/mmc.c +1 -2

@@ -118,15 +118,12 @@ static int mmc_decode_csd(struct mmc_card *card)
 	u32 *resp = card->raw_csd;
 
 	/*
-	 * We only understand CSD structure v1.1 and v1.2.
+	 * We understand all CSD structure v1.1, v1.2 and v1.3.
 	 * v1.2 has extra information in bits 15, 11 and 10.
 	 */
 	csd_struct = UNSTUFF_BITS(resp, 126, 2);
-	if (csd_struct != 1 && csd_struct != 2) {

Hmm, it isn't easy to see what was going on here, this version field is 2bits?

I'd still check for 0 in case someone repurposes the 0 bit to mean some future version we don't know.

Quoted 12 lines

drivers/mmc/core/mmc.c +1 -4
-		printk(KERN_ERR "%s: unrecognised CSD structure version %d\n",
+	printk(KERN_DEBUG "%s: recognised CSD structure version %d\n",
 			mmc_hostname(card->host), csd_struct);
-		return -EINVAL;
-	}
 
 	csd->mmca_vsn	 = UNSTUFF_BITS(resp, 122, 4);
 	m = UNSTUFF_BITS(resp, 115, 4);
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 5 lines

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Kyungmin Park

author

2010-05-31 07:35 UTC #2 lore raw

Quoted 24 lines · Ben Dooks

Quoted 11 lines · Kyungmin Park
The eMMC spec 4.4 and 4.3 + additional feature chips has CSD structure version 3 To probe these chip properly and make it simple.
it doesn't check CSD structure.
Signed-off-by Kyungmin Park <[email protected]>
---
drivers/mmc/core/mmc.c +0 -0
index 89f7a25..9e42bc6 100644
drivers/mmc/core/mmc.c static int mmc_decode_csd(struct mmc_card *card) · line 125
Quoted 9 lines
drivers/mmc/core/mmc.c +1 -2
@@ -118,15 +118,12 @@ static int mmc_decode_csd(struct mmc_card *card)
       u32 *resp = card->raw_csd;
 
       /*
-      * We only understand CSD structure v1.1 and v1.2.
+      * We understand all CSD structure v1.1, v1.2 and v1.3.
        * v1.2 has extra information in bits 15, 11 and 10.
        */
       csd_struct = UNSTUFF_BITS(resp, 126, 2);
-     if (csd_struct != 1 && csd_struct != 2) {
Hmm, it isn't easy to see what was going on here, this version field is 2bits?

Quoted 2 lines

I'd still check for 0 in case someone repurposes the 0 bit to mean some future version we don't know.

I also consider it. but even though we don't check the 0 at here. later time it can't pass the EXT_CSD version check. so it will return error.

That's reason I remain it printk at here. and even though '0' csd version has some meaning at future. no need to modify for this.

I want to wait others comments.

Thank you,
Kyungmin Park

Quoted 26 lines

Quoted 10 lines
drivers/mmc/core/mmc.c +1 -4
-             printk(KERN_ERR "%s: unrecognised CSD structure version %d\n",
+     printk(KERN_DEBUG "%s: recognised CSD structure version %d\n",
                       mmc_hostname(card->host), csd_struct);
-             return -EINVAL;
-     }
 
       csd->mmca_vsn    = UNSTUFF_BITS(resp, 122, 4);
       m = UNSTUFF_BITS(resp, 115, 4);
--
Quoted 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Adrian Hunter

2010-06-01 09:52 UTC #3 lore raw

In reply to Kyungmin Park

Quoted 1 line · Kyungmin Park

The eMMC spec 4.4 and 4.3 + additional feature chips has CSD structure version 3

That is not exactly correct. There is no CSD structure version 1.3. Instead there are two fields for CSD structure version. One in CSD and one in Extended CSD.

In CSD there is:

CSD_STRUCTURE [127:126]
Describes the version of the CSD structure.
Table 44 — CSD register structure

CSD_STRUCTURE    CSD Structure Version    Valid for System Specification Version
0                CSD version No.          1.0 Allocated by MMCA
1                CSD version No.          1.1 Allocated by MMCA
2                CSD version No.          1.2 Version 4.1–4.2–4.3
3                Version is coded in the CSD_STRUCTURE byte in the EXT_CSD register

So the 3 does not mean version 1.3 it just means look at Extended CSD instead. But the Extended CSD field does not define CSD structure version 3 at all.

CSD_STRUCTURE [194]
This field is a continuation of the CSD_STRUCTURE field in the CSD register Table 78 — CSD register structure

CSD_STRUCTURE    CSD structure version    Valid for System Specification Version
0                CSD version No. 1.0      Allocated by MMCA
1                CSD version No. 1.1      Allocated by MMCA
2                CSD version No. 1.2      Version 4.1–4.2–4.3-4.4

3–255 Reserved for future use

Quoted 10 lines

To probe these chip properly and make it simple.
it doesn't check CSD structure.
Signed-off-by Kyungmin Park <[email protected]>
---
drivers/mmc/core/mmc.c +0 -0
index 89f7a25..9e42bc6 100644

drivers/mmc/core/mmc.c static int mmc_decode_csd(struct mmc_card *card) · line 121

Quoted 5 lines

drivers/mmc/core/mmc.c +1 -1

@@ -118,15 +118,12 @@ static int mmc_decode_csd(struct mmc_card *card)
 	u32 *resp = card->raw_csd;
 
 	/*
-	 * We only understand CSD structure v1.1 and v1.2.
+	 * We understand all CSD structure v1.1, v1.2 and v1.3.

This comment is not correct. v1.3 does not exist.

drivers/mmc/core/mmc.c static int mmc_decode_csd(struct mmc_card *card) · line 126

Quoted 5 lines

drivers/mmc/core/mmc.c +0 -2

 	 * v1.2 has extra information in bits 15, 11 and 10.
 	 */
 	csd_struct = UNSTUFF_BITS(resp, 126, 2);
-	if (csd_struct != 1 && csd_struct != 2) {
-		printk(KERN_ERR "%s: unrecognised CSD structure version %d\n",

I don't agree with removing this validation without a reason. Instead extend it to 3.

To support the '3' value properly, the Extended CSD CSD_STRUCTURE field needs to be validated also i.e. in mmc_read_ext_csd() do something like:

	u32 *resp = card->raw_csd;

	if (UNSTUFF_BITS(resp, 126, 2) == 3) {
		check ext_csd[194]
	}

Quoted 12 lines

drivers/mmc/core/mmc.c +1 -3
+	printk(KERN_DEBUG "%s: recognised CSD structure version %d\n",
 			mmc_hostname(card->host), csd_struct);
-		return -EINVAL;
-	}
 
 	csd->mmca_vsn	 = UNSTUFF_BITS(resp, 122, 4);
 	m = UNSTUFF_BITS(resp, 115, 4);
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kyungmin Park

author

2010-06-02 08:12 UTC #4 lore raw

Quoted 27 lines · Adrian Hunter

Quoted 3 lines · Kyungmin Park
The eMMC spec 4.4 and 4.3 + additional feature chips has CSD structure version 3
That is not exactly correct. There is no CSD structure version 1.3. Instead
there are two fields for CSD structure version. One in CSD and one in Extended CSD.
In CSD there is:
CSD_STRUCTURE [127:126]
Describes the version of the CSD structure.
Table 44 — CSD register structure
CSD_STRUCTURE CSD Structure Version Valid for System Specification Version
0 CSD version No. 1.0 Allocated by MMCA 1 CSD version No. 1.1 Allocated by MMCA 2 CSD version No. 1.2 Version 4.1–4.2–4.3 3 Version is coded in the CSD_STRUCTURE byte in the EXT_CSD register
So the 3 does not mean version 1.3 it just means look at Extended CSD instead.
But the Extended CSD field does not define CSD structure version 3 at all.

Quoted 9 lines

CSD_STRUCTURE [194]
This field is a continuation of the CSD_STRUCTURE field in the CSD register Table 78 — CSD register structure
CSD_STRUCTURE CSD structure version Valid for System Specification Version
0 CSD version No. 1.0 Allocated by MMCA
1 CSD version No. 1.1 Allocated by MMCA
2 CSD version No. 1.2 Version 4.1–4.2–4.3-4.4 3–255 Reserved for future use

Thank you for Info. I got the eMMC v4.4 spec yesterday.

Quoted 4 lines

Quoted 4 lines
To probe these chip properly and make it simple.
it doesn't check CSD structure.

Quoted 13 lines

drivers/mmc/core/mmc.c static int mmc_decode_csd(struct mmc_card *card) · line 120
Quoted 11 lines
Signed-off-by Kyungmin Park <[email protected]>
---
drivers/mmc/core/mmc.c +1 -1
index 89f7a25..9e42bc6 100644
@@ -118,15 +118,12 @@ static int mmc_decode_csd(struct mmc_card *card)
        u32 *resp = card->raw_csd;
        /*
-        * We only understand CSD structure v1.1 and v1.2.
+        * We understand all CSD structure v1.1, v1.2 and v1.3.
This comment is not correct. v1.3 does not exist.

I will fix it correctly.

Quoted 10 lines

drivers/mmc/core/mmc.c static int mmc_decode_csd(struct mmc_card *card)
Quoted 7 lines
drivers/mmc/core/mmc.c +0 -2
         * v1.2 has extra information in bits 15, 11 and 10.
         */
        csd_struct = UNSTUFF_BITS(resp, 126, 2);
-       if (csd_struct != 1 && csd_struct != 2) {
-               printk(KERN_ERR "%s: unrecognised CSD structure version
 %d\n",
I don't agree with removing this validation without a reason. Instead extend it to 3.

With above description. it has all meaning. even though we don't use the '0' value.
Do you want to check if csd_struct == 0 case then fail?

Quoted 11 lines

To support the '3' value properly, the Extended CSD CSD_STRUCTURE field needs to be validated also i.e. in mmc_read_ext_csd() do something like:
u32 *resp = card->raw_csd;
if (UNSTUFF_BITS(resp, 126, 2) == 3) {
check ext_csd[194]
}

I wonder are there any use case of csd version? it's just check routine for proper operation.
basically no problem to display csd version and ext_csd version.

Thank you,
Kyungmin Park

Quoted 17 lines

Quoted 15 lines
drivers/mmc/core/mmc.c +1 -3
+       printk(KERN_DEBUG "%s: recognised CSD structure version %d\n",
                        mmc_hostname(card->host), csd_struct);
-               return -EINVAL;
-       }
        csd->mmca_vsn    = UNSTUFF_BITS(resp, 122, 4);
        m = UNSTUFF_BITS(resp, 115, 4);
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Adrian Hunter

2010-06-02 19:31 UTC #5 lore raw

Quoted 64 lines · Kyungmin Park

Hi
Quoted 30 lines · Adrian Hunter
Quoted 2 lines · Kyungmin Park
The eMMC spec 4.4 and 4.3 + additional feature chips has CSD structure version 3
That is not exactly correct. There is no CSD structure version 1.3.
Instead
there are two fields for CSD structure version. One in CSD and one in Extended CSD.
In CSD there is:
CSD_STRUCTURE [127:126]
Describes the version of the CSD structure.
Table 44 — CSD register structure
CSD_STRUCTURE CSD Structure Version Valid for System Specification
Version
0                CSD version No.          1.0 Allocated by MMCA
1                CSD version No.          1.1 Allocated by MMCA
2                CSD version No.          1.2 Version 4.1–4.2–4.3
3                Version is coded in the CSD_STRUCTURE byte in the EXT_CSD
register
So the 3 does not mean version 1.3 it just means look at Extended CSD instead.
But the Extended CSD field does not define CSD structure version 3 at all.
CSD_STRUCTURE [194]
This field is a continuation of the CSD_STRUCTURE field in the CSD register Table 78 — CSD register structure
CSD_STRUCTURE CSD structure version Valid for System Specification
Version
Quoted 4 lines
0                CSD version No. 1.0      Allocated by MMCA
1                CSD version No. 1.1      Allocated by MMCA
2                CSD version No. 1.2      Version 4.1–4.2–4.3-4.4
3–255 Reserved for future use
Thank you for Info. I got the eMMC v4.4 spec yesterday.
Quoted 15 lines
Quoted 10 lines
To probe these chip properly and make it simple.
it doesn't check CSD structure.
Signed-off-by Kyungmin Park <[email protected]>
---
drivers/mmc/core/mmc.c +0 -0
index 89f7a25..9e42bc6 100644
drivers/mmc/core/mmc.c static int mmc_decode_csd(struct mmc_card *card) · line 120
Quoted 4 lines
drivers/mmc/core/mmc.c +1 -1
@@ -118,15 +118,12 @@ static int mmc_decode_csd(struct mmc_card *card)
       u32 *resp = card->raw_csd;
       /*
-        * We only understand CSD structure v1.1 and v1.2.
+        * We understand all CSD structure v1.1, v1.2 and v1.3.
This comment is not correct. v1.3 does not exist.
I will fix it correctly.
Quoted 6 lines
Quoted 6 lines
drivers/mmc/core/mmc.c +0 -2
        * v1.2 has extra information in bits 15, 11 and 10.
        */
       csd_struct = UNSTUFF_BITS(resp, 126, 2);
-       if (csd_struct != 1 && csd_struct != 2) {
-               printk(KERN_ERR "%s: unrecognised CSD structure version
 %d\n",

Quoted 6 lines

Quoted 2 lines
I don't agree with removing this validation without a reason. Instead extend it to 3.
With above description. it has all meaning. even though we don't use the '0' value.
Do you want to check if csd_struct == 0 case then fail?

Yes because that is how is presently works. We should not change that without a reason.

Quoted 6 lines

Quoted 6 lines
To support the '3' value properly, the Extended CSD CSD_STRUCTURE field needs to be validated also i.e. in mmc_read_ext_csd() do something like:

Quoted 8 lines

Quoted 5 lines
       u32 *resp = card->raw_csd;

       if (UNSTUFF_BITS(resp, 126, 2) == 3) {
               check ext_csd[194]
       }
I wonder are there any use case of csd version? it's just check routine for proper operation.

I presume it is being checked to prevent unsupported or broken cards from causing unpredictable behaviour. I think you should do the check I proposed above.

Quoted 20 lines

basically no problem to display csd version and ext_csd version.
Thank you,
Kyungmin Park
Quoted 15 lines
Quoted 14 lines
drivers/mmc/core/mmc.c +1 -3
+       printk(KERN_DEBUG "%s: recognised CSD structure version %d\n",
                       mmc_hostname(card->host), csd_struct);
-               return -EINVAL;
-       }
       csd->mmca_vsn    = UNSTUFF_BITS(resp, 122, 4);
       m = UNSTUFF_BITS(resp, 115, 4);
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html