Re: [PATCH V4 07/11] mmc: mmc: Enable CQE's

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

On 07/08/17 17:51, Ulf Hansson wrote:
> On 21 July 2017 at 11:49, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Enable or disable CQE when a card is added or removed respectively.
> 
> As a standalone change, this is hard to understand.
> 
> Perhaps if you squash this with some the patche calling ->cqe_off()
> and the one actually setting ext_csd.cmdq_en, I can get a better
> picture.

No they are not related.

> 
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/core/bus.c |  7 +++++++
>>  drivers/mmc/core/mmc.c | 13 +++++++++++++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
>> index 301246513a37..a4b49e25fe96 100644
>> --- a/drivers/mmc/core/bus.c
>> +++ b/drivers/mmc/core/bus.c
>> @@ -369,10 +369,17 @@ int mmc_add_card(struct mmc_card *card)
>>   */
>>  void mmc_remove_card(struct mmc_card *card)
>>  {
>> +       struct mmc_host *host = card->host;
>> +
>>  #ifdef CONFIG_DEBUG_FS
>>         mmc_remove_card_debugfs(card);
>>  #endif
>>
>> +       if (host->cqe_enabled) {
>> +               host->cqe_ops->cqe_disable(host);
>> +               host->cqe_enabled = false;
>> +       }
>> +
> 
> This doesn't feel like the correct place to disable cqe.
> 
> Primarily because I don't think you enable cqe in mmc_add_card(), so
> this isn't consistent.

mmc_add_card() and mmc_remove_card() are not paired functions. Instead
mmc_remove_card() is on the error path of the ..._init_card() functions and
consequently this is the best place for cqe->disable().

> 
>>         if (mmc_card_present(card)) {
>>                 if (mmc_host_is_spi(card->host)) {
>>                         pr_info("%s: SPI card removed\n",
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 2ff0caf92bc8..92c6167d64e0 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1804,6 +1804,19 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>>          */
>>         card->reenable_cmdq = card->ext_csd.cmdq_en;
>>
>> +       if (card->ext_csd.cmdq_en && (host->caps2 & MMC_CAP2_CQE) &&
>> +           !host->cqe_enabled) {
> 
> I assume card->ext_csd.cmdq_en can't be set, unless the MMC_CAP2_CQE
> bit also is set. So there should be no reason to check it here again?

Yes, it is a left-over from software command queue support.  I will remove it.

> 
> Also, can ever host->cqe_enabled be true in this path? If so, isn't
> that wrong by itself?

No, it can be set on the reset path.

> 
>> +               err = host->cqe_ops->cqe_enable(host, card);
>> +               if (err) {
>> +                       pr_err("%s: Failed to enable CQE, error %d\n",
>> +                               mmc_hostname(host), err);
>> +               } else {
>> +                       host->cqe_enabled = true;
>> +                       pr_info("%s: Command Queue Engine enabled\n",
>> +                               mmc_hostname(host));
>> +               }
>> +       }
>> +
>>         /*
>>          * The mandatory minimum values are defined for packed command.
>>          * read: 5, write: 3
>> --
>> 1.9.1
>>
> 
> Kind regards
> Uffe
>