Re: [PATCH RFC 01/10] mmc: core: Add support to read command queue parameters

[Ritesh Harjani <riteshh@codeaurora.org>]

Hi Shawn,

On 6/16/2016 1:42 PM, Shawn Lin wrote:
> On 2016/6/15 21:01, Ritesh Harjani wrote:
>> From: Asutosh Das <asutoshd@codeaurora.org>
>>
>> eMMC cards with EXT_CSD version >= 7, optionally support command
will change it here to "8"

>> queuing feature as defined by JEDEC eMMC5.1. Add support for probing
>> command queue feature for such type of cards.
>>
>> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
>> [subhashj@codeaurora.org: fixed trivial merge conflicts]
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> ---
>>  drivers/mmc/core/mmc.c   | 18 ++++++++++++++++++
>>  include/linux/mmc/card.h |  2 ++
>>  include/linux/mmc/mmc.h  |  3 +++
>>  3 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 4dbe3df..ff560e9 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -578,6 +578,24 @@ static int mmc_decode_ext_csd(struct mmc_card
>> *card, u8 *ext_csd)
>>          card->ext_csd.data_sector_size = 512;
>>      }
>>
>> +    if (card->ext_csd.rev >= 7) {
>
> Do you ever find one device claiming to support it with
> ext_csd.rev to be 7?
No, will change it to 8 (for 5.1 revision).


>
> I believe it should be 8 as 7 is for 5.0 and 5.01 which don't support
> hw-cmdq.
Yes, thanks for pointing out.

>
>> +        card->ext_csd.cmdq_support = ext_csd[EXT_CSD_CMDQ_SUPPORT];
>> +        if (card->ext_csd.cmdq_support) {
>> +            /*
>> +             * Queue Depth = N + 1,
>> +             * see JEDEC JESD84-B51 section 7.4.19
>> +             */
>> +            card->ext_csd.cmdq_depth =
>> +                ext_csd[EXT_CSD_CMDQ_DEPTH] + 1;
>> +            pr_info("%s: CMDQ supported: depth: %d\n",
>> +                mmc_hostname(card->host),
>> +                card->ext_csd.cmdq_depth);
>
> pr_debug may be more appropriate?
Done.

>
>> +        }
>> +    } else {
>> +        card->ext_csd.cmdq_support = 0;
>> +        card->ext_csd.cmdq_depth = 0;
>> +    }
>
> if the code fallback to here, card->ext_csd.cmdq_support must be
> zero already, and the ext_csd is already set to zero which also
> means you not need to zero it again here.
since card->ext_csd is zero for uninitialized variables,
we can remove these. Sure.

>
>> +
>>      /* eMMC v5 or later */
>>      if (card->ext_csd.rev >= 7) {
>>          memcpy(card->ext_csd.fwrev, &ext_csd[EXT_CSD_FIRMWARE_VERSION],
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index eb0151b..f74db84 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -118,6 +118,8 @@ struct mmc_ext_csd {
>>      u8            raw_pwr_cl_ddr_200_360;    /* 253 */
>>      u8            raw_bkops_status;    /* 246 */
>>      u8            raw_sectors[4];        /* 212 - 4 bytes */
>> +    u8            cmdq_depth;        /* 307 */
>> +    u8            cmdq_support;        /* 308 */
>>
>>      unsigned int            feature_support;
>>  #define MMC_DISCARD_FEATURE    BIT(0)                  /* CMD38
>> feature */
>> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
>> index 15f2c4a..1c0ae75 100644
>> --- a/include/linux/mmc/mmc.h
>> +++ b/include/linux/mmc/mmc.h
>> @@ -330,6 +330,9 @@ struct _mmc_csd {
>>  #define EXT_CSD_CACHE_SIZE        249    /* RO, 4 bytes */
>>  #define EXT_CSD_PWR_CL_DDR_200_360    253    /* RO */
>>  #define EXT_CSD_FIRMWARE_VERSION    254    /* RO, 8 bytes */
>> +#define EXT_CSD_CMDQ_DEPTH        307    /* RO */
>> +#define EXT_CSD_CMDQ_SUPPORT        308    /* RO */
>> + #define EXT_CSD_TAG_UNIT_SIZE        498    /* RO */
>
> an invalid space here
Done.

>
>>  #define EXT_CSD_SUPPORTED_MODE        493    /* RO */
>>  #define EXT_CSD_TAG_UNIT_SIZE        498    /* RO */
>>  #define EXT_CSD_DATA_TAG_SUPPORT    499    /* RO */
>>
>
>

--
BR
Ritesh