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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
