diff mbox

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

Message ID 20180205125029.21570-10-kishon@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kishon Vijay Abraham I Feb. 5, 2018, 12:50 p.m. UTC
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@ti.com>
---
 drivers/mmc/host/sdhci.c | 12 ++++++++++++
 drivers/mmc/host/sdhci.h |  7 +++++++
 2 files changed, 19 insertions(+)

Comments

Adrian Hunter Feb. 19, 2018, 8:51 a.m. UTC | #1
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@ti.com>
> ---
>  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)
>  
>  	int irq;		/* Device IRQ */
>  	void __iomem *ioaddr;	/* Mapped address */
> @@ -522,6 +527,8 @@ struct sdhci_host {
>  	unsigned int            ocr_avail_mmc;
>  	u32 ocr_mask;		/* available voltages */
>  
> +	bool			hw_timeout_disabled;
> +
>  	unsigned		timing;		/* Current timing */
>  
>  	u32			thread_isr;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kishon Vijay Abraham I March 5, 2018, 9:30 a.m. UTC | #2
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@ti.com>
>> ---
>>  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.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Hunter March 5, 2018, 9:38 a.m. UTC | #3
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@ti.com>
>>> ---
>>>  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 linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kishon Vijay Abraham I March 14, 2018, 1:25 p.m. UTC | #4
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@ti.com>
>>>> ---
>>>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
+		}
 	}
 
 	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)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
@@ -522,6 +527,8 @@  struct sdhci_host {
 	unsigned int            ocr_avail_mmc;
 	u32 ocr_mask;		/* available voltages */
 
+	bool			hw_timeout_disabled;
+
 	unsigned		timing;		/* Current timing */
 
 	u32			thread_isr;