Re: [PATCH v2 09/16] mmc: sdhci: Add quirk to disable HW timeout

[Kishon Vijay Abraham I]

On Monday 05 March 2018 03:08 PM, Adrian Hunter wrote:
> On 05/03/18 11:30, Kishon Vijay Abraham I wrote:
>> Hi Adrian,
>>
>> On Monday 19 February 2018 02:21 PM, Adrian Hunter wrote:
>>> On 05/02/18 14:50, Kishon Vijay Abraham I wrote:
>>>> Add quirk to disable HW timeout if the requested timeout is more than
>>>> the maximum obtainable timeout.
>>>>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@xxxxxx>
>>>> ---
>>>>  drivers/mmc/host/sdhci.c | 12 ++++++++++++
>>>>  drivers/mmc/host/sdhci.h |  7 +++++++
>>>>  2 files changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index 070aff9c108f..1aa74b4682f3 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -735,6 +735,12 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>>>>  		DBG("Too large timeout 0x%x requested for CMD%d!\n",
>>>>  		    count, cmd->opcode);
>>>>  		count = 0xE;
>>>> +		if (host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) {
>>>> +			host->ier &= ~SDHCI_INT_DATA_TIMEOUT;
>>>> +			sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
>>>> +			sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
>>>> +			host->hw_timeout_disabled = true;
>>>> +		}
>>>
>>> sdhci_calc_timeout() is really for calculations so please do this in
>>> sdhci_set_timeout() instead (i.e. change sdhci_calc_timeout() to return
>>> whether the timeout is too big).  Note that you need to cater for the "/*
>>> Unspecified timeout, assume max */" case and the
>>> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL case.
>>>
>>> Also if SDHCI_INT_DATA_TIMEOUT is not being disabled, check here to
>>> re-enable it and leave sdhci_request_done() alone. i.e.
>>>
>>> 	else if (!(host->ier & SDHCI_INT_DATA_TIMEOUT)) {
>>> 		host->ier |= SDHCI_INT_DATA_TIMEOUT;
>>> 		sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
>>> 		sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
>>> 	}
>>>
>>> Maybe make a separate subroutine for the update:
>>>
>>> void sdhci_set_data_timeout_irq(struct sdhci_host *host, bool enable)
>>> {
>>> 	if (enable)
>>> 		host->ier |= SDHCI_INT_DATA_TIMEOUT;
>>> 	else
>>> 		host->ier &= ~SDHCI_INT_DATA_TIMEOUT;
>>> 	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
>>> 	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
>>> }
>>>
>>>
>>>>  	}
>>>>  
>>>>  	return count;
>>>> @@ -2349,6 +2355,12 @@ static bool sdhci_request_done(struct sdhci_host *host)
>>>>  	}
>>>>  
>>>>  	sdhci_del_timer(host, mrq);
>>>> +	if (sdhci_data_line_cmd(mrq->cmd) && host->hw_timeout_disabled) {
>>>> +		host->ier |= SDHCI_INT_DATA_TIMEOUT;
>>>> +		sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
>>>> +		sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
>>>> +		host->hw_timeout_disabled = false;
>>>> +	}
>>>>  
>>>>  	/*
>>>>  	 * Always unmap the data buffers if they were mapped by
>>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>>> index afab26fd70e6..3a967a56fcc3 100644
>>>> --- a/drivers/mmc/host/sdhci.h
>>>> +++ b/drivers/mmc/host/sdhci.h
>>>> @@ -437,6 +437,11 @@ struct sdhci_host {
>>>>  #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN		(1<<15)
>>>>  /* Controller has CRC in 136 bit Command Response */
>>>>  #define SDHCI_QUIRK2_RSP_136_HAS_CRC			(1<<16)
>>>> +/*
>>>> + * Disable HW timeout if the requested timeout is more than the maximum
>>>> + * obtainable timeout
>>>> + */
>>>> +#define SDHCI_QUIRK2_DISABLE_HW_TIMEOUT			(1<<17)
>>
>> Are you okay with the definition of this quirk? i.e this quirk is applied only
>> when the requested timeout is "more" than the maximum obtainable timeout.
>>
>> By this way platforms can continue to use hardware timeout for smaller timeout
>> value.
> 
> It is fine to me.
> 
Okay thanks. I've posted v3 sometime last week. I'm trying to get this in for
4.17? Can you review the new revision please?

Thanks
Kishon