Re: [PATCH] Recognize CSD structure version 3

[Adrian Hunter <adrian.hunter@nokia.com>]

Kyungmin Park wrote:
> Hi
> 
> On Tue, Jun 1, 2010 at 6:51 PM, Adrian Hunter <adrian.hunter@nokia.com> wrote:
>> Kyungmin Park wrote:
>>> 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
> 
> Thank you for Info. I got the eMMC v4.4 spec yesterday.
> 
>>> To probe these chip properly and make it simple.
>>> it doesn't check CSD structure.
>>>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 89f7a25..9e42bc6 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -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.
> 
>>>         * 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?

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

> 
>> 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.

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.

> basically no problem to display csd version and ext_csd version.
> 
> Thank you,
> Kyungmin Park
>>
>>
>>
>>> +       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 majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>