Re: [PATCH V7 10/25] mmc: mmc: Add functions to enable / disable the Command Queue

[Ritesh Harjani <riteshh@codeaurora.org>]

On 11/28/2016 6:53 PM, Adrian Hunter wrote:
> On 28/11/16 06:36, Ritesh Harjani wrote:
>>
>>
>> On 11/25/2016 3:37 PM, Adrian Hunter wrote:
>>> Add helper functions to enable or disable the Command Queue.
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>> ---
>>>  Documentation/mmc/mmc-dev-attrs.txt |  1 +
>>>  drivers/mmc/core/mmc.c              |  2 ++
>>>  drivers/mmc/core/mmc_ops.c          | 27 +++++++++++++++++++++++++++
>>>  include/linux/mmc/card.h            |  1 +
>>>  include/linux/mmc/core.h            |  2 ++
>>>  5 files changed, 33 insertions(+)
>>>
>>> diff --git a/Documentation/mmc/mmc-dev-attrs.txt
>>> b/Documentation/mmc/mmc-dev-attrs.txt
>>> index 404a0e9e92b0..dcd1252877fb 100644
>>> --- a/Documentation/mmc/mmc-dev-attrs.txt
>>> +++ b/Documentation/mmc/mmc-dev-attrs.txt
>>> @@ -30,6 +30,7 @@ All attributes are read-only.
>>>      rel_sectors        Reliable write sector count
>>>      ocr             Operation Conditions Register
>>>      dsr            Driver Stage Register
>>> +    cmdq_en            Command Queue enabled: 1 => enabled, 0 => not enabled
>>>
>>>  Note on Erase Size and Preferred Erase Size:
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 6e9830997eef..d6a30bbd399d 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -770,6 +770,7 @@ static int mmc_compare_ext_csds(struct mmc_card *card,
>>> unsigned bus_width)
>>>  MMC_DEV_ATTR(raw_rpmb_size_mult, "%#x\n", card->ext_csd.raw_rpmb_size_mult);
>>>  MMC_DEV_ATTR(rel_sectors, "%#x\n", card->ext_csd.rel_sectors);
>>>  MMC_DEV_ATTR(ocr, "%08x\n", card->ocr);
>>> +MMC_DEV_ATTR(cmdq_en, "%d\n", card->ext_csd.cmdq_en);
>>>
>>>  static ssize_t mmc_fwrev_show(struct device *dev,
>>>                    struct device_attribute *attr,
>>> @@ -823,6 +824,7 @@ static ssize_t mmc_dsr_show(struct device *dev,
>>>      &dev_attr_rel_sectors.attr,
>>>      &dev_attr_ocr.attr,
>>>      &dev_attr_dsr.attr,
>>> +    &dev_attr_cmdq_en.attr,
>>>      NULL,
>>>  };
>>>  ATTRIBUTE_GROUPS(mmc_std);
>>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>>> index 9b2617cfff67..92a1de9b4981 100644
>>> --- a/drivers/mmc/core/mmc_ops.c
>>> +++ b/drivers/mmc/core/mmc_ops.c
>>> @@ -824,3 +824,30 @@ int mmc_can_ext_csd(struct mmc_card *card)
>>>  {
>>>      return (card && card->csd.mmca_vsn > CSD_SPEC_VER_3);
>>>  }
>>> +
>>> +int mmc_cmdq_switch(struct mmc_card *card, int enable)
>>> +{
>>> +    int err;
>>> +
>>> +    if (!card->ext_csd.cmdq_support)
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_CMDQ_MODE_EN,
>>> +             enable, card->ext_csd.generic_cmd6_time);
>>> +    if (!err)
>>> +        card->ext_csd.cmdq_en = enable;
>>> +
>>> +    return err;
>>> +}
>>> +
>>> +int mmc_cmdq_enable(struct mmc_card *card)
>>> +{
>>> +    return mmc_cmdq_switch(card, EXT_CSD_CMDQ_MODE_ENABLED);
>>
>> EXT_CSD_CMDQ_MODE_ENABLED is defined as a BIT(0), but getting used here as a
>> value of 1. EXT_CSD_CMDQ_MODE_ENABLED seems redundant anyways.
>> Do you think we can remove it and pass 1 directly?
>
> How about:
>
> static int mmc_cmdq_switch(struct mmc_card *card, bool enable)
> {
> 	u8 val = enable ? EXT_CSD_CMDQ_MODE_ENABLED : 0;
> 	int err;
>
> 	if (!card->ext_csd.cmdq_support)
> 		return -EOPNOTSUPP;
>
> 	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_CMDQ_MODE_EN,
> 			 val, card->ext_csd.generic_cmd6_time);
> 	if (!err)
> 		card->ext_csd.cmdq_en = enable;
>
> 	return err;
> }
>
> int mmc_cmdq_enable(struct mmc_card *card)
> {
> 	return mmc_cmdq_switch(card, true);
> }
> EXPORT_SYMBOL_GPL(mmc_cmdq_enable);
>
> int mmc_cmdq_disable(struct mmc_card *card)
> {
> 	return mmc_cmdq_switch(card, false);
> }
> EXPORT_SYMBOL_GPL(mmc_cmdq_disable);
>
yes. This looks good.


>>
>>> +}
>>> +EXPORT_SYMBOL_GPL(mmc_cmdq_enable);
>>> +
>>> +int mmc_cmdq_disable(struct mmc_card *card)
>>> +{
>>> +    return mmc_cmdq_switch(card, 0);
>>> +}
>>> +EXPORT_SYMBOL_GPL(mmc_cmdq_disable);
>>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>>> index 95d69d498296..2d9c24f4e88e 100644
>>> --- a/include/linux/mmc/card.h
>>> +++ b/include/linux/mmc/card.h
>>> @@ -89,6 +89,7 @@ struct mmc_ext_csd {
>>>      unsigned int        boot_ro_lock;        /* ro lock support */
>>>      bool            boot_ro_lockable;
>>>      bool            ffu_capable;    /* Firmware upgrade support */
>>> +    bool            cmdq_en;    /* Command Queue enabled */
>>>      bool            cmdq_support;    /* Command Queue supported */
>>>      unsigned int        cmdq_depth;    /* Command Queue depth */
>>>  #define MMC_FIRMWARE_LEN 8
>>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>>> index 0ce928b3ce90..d045b06fc7ea 100644
>>> --- a/include/linux/mmc/core.h
>>> +++ b/include/linux/mmc/core.h
>>> @@ -177,6 +177,8 @@ extern int mmc_wait_for_app_cmd(struct mmc_host *,
>>> struct mmc_card *,
>>>  extern int mmc_switch(struct mmc_card *, u8, u8, u8, unsigned int);
>>>  extern int mmc_send_tuning(struct mmc_host *host, u32 opcode, int
>>> *cmd_error);
>>>  extern int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
>>> +extern int mmc_cmdq_enable(struct mmc_card *card);
>>> +extern int mmc_cmdq_disable(struct mmc_card *card);
>>>
>>>  #define MMC_ERASE_ARG        0x00000000
>>>  #define MMC_SECURE_ERASE_ARG    0x80000000
>>>
>>
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project