[v1,2/2] mmc: sdhci: Use the SW timer when the HW timer cannot meet the timeout value required by the device

Commit Message

Bean Huo Sept. 17, 2021, 5:27 p.m. UTC

From: Bean Huo <[e-mail]>

If the data transmission timeout value required by the device exceeds
the maximum timeout value of the host HW timer, we still use the HW
timer with the maximum timeout value of the HW timer. This setting is
suitable for most R/W situations. But sometimes, the device will complete
the R/W task within its required timeout value (greater than the HW timer).
In this case, the HW timer for data transmission will time out.

Currently, in this condition, we  disable the HW timer and use the SW
timer only when the SDHCI_QUIRK2_DISABLE_HW_TIMEOUT quirk is set by the
host driver. The patch is to remove this if statement restriction and
allow data transmission to use the SW timer when the hardware timer cannot
meet the required timeout value.

Signed-off-by: Bean Huo <[e-mail]>
---
 drivers/mmc/host/sdhci.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Adrian Hunter Sept. 24, 2021, 5:29 a.m. UTC | #1

On 17/09/21 8:27 pm, Bean Huo wrote:
> From: Bean Huo <[e-mail]>
> 
> If the data transmission timeout value required by the device exceeds
> the maximum timeout value of the host HW timer, we still use the HW
> timer with the maximum timeout value of the HW timer. This setting is
> suitable for most R/W situations. But sometimes, the device will complete
> the R/W task within its required timeout value (greater than the HW timer).
> In this case, the HW timer for data transmission will time out.
> 
> Currently, in this condition, we  disable the HW timer and use the SW
> timer only when the SDHCI_QUIRK2_DISABLE_HW_TIMEOUT quirk is set by the
> host driver. The patch is to remove this if statement restriction and
> allow data transmission to use the SW timer when the hardware timer cannot
> meet the required timeout value.

The reason it is a quirk is because it does not work for all hardware.
For some controllers the timeout cannot really be disabled, only the
interrupt is disabled, and then the controller never indicates completion
if the timeout is exceeded.

> 
> Signed-off-by: Bean Huo <[e-mail]>
> ---
>  drivers/mmc/host/sdhci.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 357b365bf0ec..463517fd9886 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -969,9 +969,6 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd,
>  		count++;
>  		current_timeout <<= 1;
>  		if (count > host->max_timeout_count) {
> -			if (!(host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT))
> -				DBG("Too large timeout 0x%x requested for CMD%d!\n",
> -				    count, cmd->opcode);
>  			count = host->max_timeout_count;
>  			*too_big = true;
>  			break;
> @@ -1016,8 +1013,7 @@ void __sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>  	bool too_big = false;
>  	u8 count = sdhci_calc_timeout(host, cmd, &too_big);
>  
> -	if (too_big &&
> -	    host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) {
> +	if (too_big) {
>  		sdhci_calc_sw_timeout(host, cmd);
>  		sdhci_set_data_timeout_irq(host, false);
>  	} else if (!(host->ier & SDHCI_INT_DATA_TIMEOUT)) {
>

Bean Huo Sept. 24, 2021, 9:17 a.m. UTC | #2

On Fri, 2021-09-24 at 08:29 +0300, Adrian Hunter wrote:
> > If the data transmission timeout value required by the device
> > exceeds
> > the maximum timeout value of the host HW timer, we still use the HW
> > timer with the maximum timeout value of the HW timer. This setting
> > is
> > suitable for most R/W situations. But sometimes, the device will
> > complete
> > the R/W task within its required timeout value (greater than the HW
> > timer).
> > In this case, the HW timer for data transmission will time out.
> > Currently, in this condition, we  disable the HW timer and use the
> > SW
> > timer only when the SDHCI_QUIRK2_DISABLE_HW_TIMEOUT quirk is set by
> > the
> > host driver. The patch is to remove this if statement restriction
> > and
> > allow data transmission to use the SW timer when the hardware timer
> > cannot
> > meet the required timeout value.
> 
> 
> The reason it is a quirk is because it does not work for all
> hardware.
> 
> For some controllers the timeout cannot really be disabled, only the
> 
> interrupt is disabled, and then the controller never indicates
> completion
> 
> if the timeout is exceeded.

Hi Adrian,
Thanks for your review.

Yes, you are right. But this quirk prevents disabling the hardware timeoutIRQ. The purpose of this patch is to disable the hardware timeout IRQ and
select the software timeout.

void __sdhci_set_timeout(struct sdhci_host *host, struct mmc_command
*cmd)
{
        bool too_big = false;
        u8 count = sdhci_calc_timeout(host, cmd, &too_big);

        if (too_big) {
                sdhci_calc_sw_timeout(host, cmd);
                sdhci_set_data_timeout_irq(host, false); // disable IRQ
        } else if (!(host->ier & SDHCI_INT_DATA_TIMEOUT)) {
                sdhci_set_data_timeout_irq(host, true);
        }

        sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
}


The driver has detected that the hardware timer cannot meet the timeout
requirements of the device, but we still use the hardware timer, which will
allow potential timeout issuea . Rather than allowing a potential
problem to exist, why can’t software timing be used to avoid this
problem?


Kind regards,
Bean

Adrian Hunter Sept. 24, 2021, 10:07 a.m. UTC | #3

On 24/09/21 12:17 pm, Bean Huo wrote:
> On Fri, 2021-09-24 at 08:29 +0300, Adrian Hunter wrote:
>>> If the data transmission timeout value required by the device
>>> exceeds
>>> the maximum timeout value of the host HW timer, we still use the HW
>>> timer with the maximum timeout value of the HW timer. This setting
>>> is
>>> suitable for most R/W situations. But sometimes, the device will
>>> complete
>>> the R/W task within its required timeout value (greater than the HW
>>> timer).
>>> In this case, the HW timer for data transmission will time out.
>>> Currently, in this condition, we  disable the HW timer and use the
>>> SW
>>> timer only when the SDHCI_QUIRK2_DISABLE_HW_TIMEOUT quirk is set by
>>> the
>>> host driver. The patch is to remove this if statement restriction
>>> and
>>> allow data transmission to use the SW timer when the hardware timer
>>> cannot
>>> meet the required timeout value.
>>
>>
>> The reason it is a quirk is because it does not work for all
>> hardware.
>>
>> For some controllers the timeout cannot really be disabled, only the
>>
>> interrupt is disabled, and then the controller never indicates
>> completion
>>
>> if the timeout is exceeded.
> 
> Hi Adrian,
> Thanks for your review.
> 
> Yes, you are right. But this quirk prevents disabling the hardware timeoutIRQ. The purpose of this patch is to disable the hardware timeout IRQ and
> select the software timeout.
> 
> void __sdhci_set_timeout(struct sdhci_host *host, struct mmc_command
> *cmd)
> {
>         bool too_big = false;
>         u8 count = sdhci_calc_timeout(host, cmd, &too_big);
> 
>         if (too_big) {
>                 sdhci_calc_sw_timeout(host, cmd);
>                 sdhci_set_data_timeout_irq(host, false); // disable IRQ
>         } else if (!(host->ier & SDHCI_INT_DATA_TIMEOUT)) {
>                 sdhci_set_data_timeout_irq(host, true);
>         }
> 
>         sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
> }
> 
> 
> The driver has detected that the hardware timer cannot meet the timeout
> requirements of the device, but we still use the hardware timer, which will
> allow potential timeout issuea . Rather than allowing a potential
> problem to exist, why can’t software timing be used to avoid this
> problem?

Timeouts aren't that accurate.  The maximum is assumed still to work.
mmc->max_busy_timeout is used to tell the core what the maximum is.

Bean Huo Sept. 24, 2021, 11:45 a.m. UTC | #4

On Fri, 2021-09-24 at 13:07 +0300, Adrian Hunter wrote:
> On 24/09/21 12:17 pm, Bean Huo wrote:
> > On Fri, 2021-09-24 at 08:29 +0300, Adrian Hunter wrote:
> > > > If the data transmission timeout value required by the device
> > > > exceeds
> > > > the maximum timeout value of the host HW timer, we still use
> > > > the HW
> > > > timer with the maximum timeout value of the HW timer. This
> > > > setting
> > > > is
> > > > suitable for most R/W situations. But sometimes, the device
> > > > will
> > > > complete
> > > > the R/W task within its required timeout value (greater than
> > > > the HW
> > > > timer).
> > > > In this case, the HW timer for data transmission will time out.
> > > > Currently, in this condition, we  disable the HW timer and use
> > > > the
> > > > SW
> > > > timer only when the SDHCI_QUIRK2_DISABLE_HW_TIMEOUT quirk is
> > > > set by
> > > > the
> > > > host driver. The patch is to remove this if statement
> > > > restriction
> > > > and
> > > > allow data transmission to use the SW timer when the hardware
> > > > timer
> > > > cannot
> > > > meet the required timeout value.
> > > 
> > > The reason it is a quirk is because it does not work for all
> > > hardware.
> > > 
> > > For some controllers the timeout cannot really be disabled, only
> > > the
> > > 
> > > interrupt is disabled, and then the controller never indicates
> > > completion
> > > 
> > > if the timeout is exceeded.
> > 
> > Hi Adrian,
> > Thanks for your review.
> > 
> > Yes, you are right. But this quirk prevents disabling the hardware
> > timeoutIRQ. The purpose of this patch is to disable the hardware
> > timeout IRQ and
> > select the software timeout.
> > 
> > void __sdhci_set_timeout(struct sdhci_host *host, struct
> > mmc_command
> > *cmd)
> > {
> >         bool too_big = false;
> >         u8 count = sdhci_calc_timeout(host, cmd, &too_big);
> > 
> >         if (too_big) {
> >                 sdhci_calc_sw_timeout(host, cmd);
> >                 sdhci_set_data_timeout_irq(host, false); // disable
> > IRQ
> >         } else if (!(host->ier & SDHCI_INT_DATA_TIMEOUT)) {
> >                 sdhci_set_data_timeout_irq(host, true);
> >         }
> > 
> >         sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
> > }
> > 
> > 
> > The driver has detected that the hardware timer cannot meet the
> > timeout
> > requirements of the device, but we still use the hardware timer,
> > which will
> > allow potential timeout issuea . Rather than allowing a potential
> > problem to exist, why can’t software timing be used to avoid this
> > problem?
> 
> Timeouts aren't that accurate.  The maximum is assumed still to work.
> mmc->max_busy_timeout is used to tell the core what the maximum is.

> 



mmc->max_busy_timeout is still a representation of Host HW timer
maximum timeout count, isn't it? 

Bean

Adrian Hunter Sept. 24, 2021, 12:17 p.m. UTC | #5

On 24/09/21 2:45 pm, Bean Huo wrote:
> On Fri, 2021-09-24 at 13:07 +0300, Adrian Hunter wrote:
>> On 24/09/21 12:17 pm, Bean Huo wrote:
>>> On Fri, 2021-09-24 at 08:29 +0300, Adrian Hunter wrote:
>>>>> If the data transmission timeout value required by the device
>>>>> exceeds
>>>>> the maximum timeout value of the host HW timer, we still use
>>>>> the HW
>>>>> timer with the maximum timeout value of the HW timer. This
>>>>> setting
>>>>> is
>>>>> suitable for most R/W situations. But sometimes, the device
>>>>> will
>>>>> complete
>>>>> the R/W task within its required timeout value (greater than
>>>>> the HW
>>>>> timer).
>>>>> In this case, the HW timer for data transmission will time out.
>>>>> Currently, in this condition, we  disable the HW timer and use
>>>>> the
>>>>> SW
>>>>> timer only when the SDHCI_QUIRK2_DISABLE_HW_TIMEOUT quirk is
>>>>> set by
>>>>> the
>>>>> host driver. The patch is to remove this if statement
>>>>> restriction
>>>>> and
>>>>> allow data transmission to use the SW timer when the hardware
>>>>> timer
>>>>> cannot
>>>>> meet the required timeout value.
>>>>
>>>> The reason it is a quirk is because it does not work for all
>>>> hardware.
>>>>
>>>> For some controllers the timeout cannot really be disabled, only
>>>> the
>>>>
>>>> interrupt is disabled, and then the controller never indicates
>>>> completion
>>>>
>>>> if the timeout is exceeded.
>>>
>>> Hi Adrian,
>>> Thanks for your review.
>>>
>>> Yes, you are right. But this quirk prevents disabling the hardware
>>> timeoutIRQ. The purpose of this patch is to disable the hardware
>>> timeout IRQ and
>>> select the software timeout.
>>>
>>> void __sdhci_set_timeout(struct sdhci_host *host, struct
>>> mmc_command
>>> *cmd)
>>> {
>>>         bool too_big = false;
>>>         u8 count = sdhci_calc_timeout(host, cmd, &too_big);
>>>
>>>         if (too_big) {
>>>                 sdhci_calc_sw_timeout(host, cmd);
>>>                 sdhci_set_data_timeout_irq(host, false); // disable
>>> IRQ
>>>         } else if (!(host->ier & SDHCI_INT_DATA_TIMEOUT)) {
>>>                 sdhci_set_data_timeout_irq(host, true);
>>>         }
>>>
>>>         sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
>>> }
>>>
>>>
>>> The driver has detected that the hardware timer cannot meet the
>>> timeout
>>> requirements of the device, but we still use the hardware timer,
>>> which will
>>> allow potential timeout issuea . Rather than allowing a potential
>>> problem to exist, why can’t software timing be used to avoid this
>>> problem?
>>
>> Timeouts aren't that accurate.  The maximum is assumed still to work.
>> mmc->max_busy_timeout is used to tell the core what the maximum is.
> 
>>
> 
> 
> 
> mmc->max_busy_timeout is still a representation of Host HW timer
> maximum timeout count, isn't it? 

Not necessarily.  For SDHCI_QUIRK2_DISABLE_HW_TIMEOUT it would be
set to zero to indicate no maximum.

Bean Huo Sept. 24, 2021, 1:08 p.m. UTC | #6

On Fri, 2021-09-24 at 15:17 +0300, Adrian Hunter wrote:
> > > >          sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
> > > > }
> > > > The driver has detected that the hardware timer cannot meet the
> > > > timeout
> > > > requirements of the device, but we still use the hardware
> > > > timer,
> > > > which will
> > > > allow potential timeout issuea . Rather than allowing a
> > > > potential
> > > > problem to exist, why can’t software timing be used to avoid
> > > > this
> > > > problem?
> > > Timeouts aren't that accurate.  The maximum is assumed still to
> > > work.
> > > mmc->max_busy_timeout is used to tell the core what the maximum
> > > is.
> > mmc->max_busy_timeout is still a representation of Host HW timer
> > maximum timeout count, isn't it? 
> 
> 
> Not necessarily.  For SDHCI_QUIRK2_DISABLE_HW_TIMEOUT it would be
> 
> set to zero to indicate no maximum.

yes, this is the purpose of the patch, for the host controller without
quirk SDHCI_QUIRK2_DISABLE_HW_TIMEOUT, if the timeout count required by
device is beyond the HW timer max count, we choose SW timer to avoid the HW timer timeout IRQ.

I don't know if I get it correctly.

Bean

Adrian Hunter Sept. 24, 2021, 1:26 p.m. UTC | #7

On 24/09/21 4:08 pm, Bean Huo wrote:
> On Fri, 2021-09-24 at 15:17 +0300, Adrian Hunter wrote:
>>>>>          sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
>>>>> }
>>>>> The driver has detected that the hardware timer cannot meet the
>>>>> timeout
>>>>> requirements of the device, but we still use the hardware
>>>>> timer,
>>>>> which will
>>>>> allow potential timeout issuea . Rather than allowing a
>>>>> potential
>>>>> problem to exist, why can’t software timing be used to avoid
>>>>> this
>>>>> problem?
>>>> Timeouts aren't that accurate.  The maximum is assumed still to
>>>> work.
>>>> mmc->max_busy_timeout is used to tell the core what the maximum
>>>> is.
>>> mmc->max_busy_timeout is still a representation of Host HW timer
>>> maximum timeout count, isn't it? 
>>
>>
>> Not necessarily.  For SDHCI_QUIRK2_DISABLE_HW_TIMEOUT it would be
>>
>> set to zero to indicate no maximum.
> 
> yes, this is the purpose of the patch, for the host controller without
> quirk SDHCI_QUIRK2_DISABLE_HW_TIMEOUT, if the timeout count required by
> device is beyond the HW timer max count, we choose SW timer to avoid the HW timer timeout IRQ.
> 
> I don't know if I get it correctly.

Why can't drivers that want the behaviour just set the quirk?

Drivers that do not work with the quirk, do not have to set it.

Bean Huo Sept. 24, 2021, 9:33 p.m. UTC | #8

On Fri, 2021-09-24 at 16:26 +0300, Adrian Hunter wrote:
> On 24/09/21 4:08 pm, Bean Huo wrote:
> > On Fri, 2021-09-24 at 15:17 +0300, Adrian Hunter wrote:
> > > > > >          sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
> > > > > > }
> > > > > > The driver has detected that the hardware timer cannot meet
> > > > > > the
> > > > > > timeout
> > > > > > requirements of the device, but we still use the hardware
> > > > > > timer,
> > > > > > which will
> > > > > > allow potential timeout issuea . Rather than allowing a
> > > > > > potential
> > > > > > problem to exist, why can’t software timing be used to
> > > > > > avoid
> > > > > > this
> > > > > > problem?
> > > > > Timeouts aren't that accurate.  The maximum is assumed still
> > > > > to
> > > > > work.
> > > > > mmc->max_busy_timeout is used to tell the core what the
> > > > > maximum
> > > > > is.
> > > > mmc->max_busy_timeout is still a representation of Host HW
> > > > timer
> > > > maximum timeout count, isn't it? 
> > > 
> > > Not necessarily.  For SDHCI_QUIRK2_DISABLE_HW_TIMEOUT it would be
> > > 
> > > set to zero to indicate no maximum.
> > 
> > yes, this is the purpose of the patch, for the host controller
> > without
> > quirk SDHCI_QUIRK2_DISABLE_HW_TIMEOUT, if the timeout count
> > required by
> > device is beyond the HW timer max count, we choose SW timer to
> > avoid the HW timer timeout IRQ.
> > 
> > I don't know if I get it correctly.
> 
> Why can't drivers that want the behaviour just set the quirk?
> 
> Drivers that do not work with the quirk, do not have to set it.


Adrian,

We cannot add this quirk to every host driver. This is the difference
on the device side. In addition, I don't know what the maximum hardware
timer budget for each host is. Even if you use the same SOC, the
maximum time budget on different platforms may be different.

Assume that the maximum timeout time supported by the hardware timer is
100 milliseconds, but the maximum data transmission time required by
the device is 150 milliseconds. In most cases, data transfer will not
take so long. 150 is the maximum time under extreme conditions. This
means that the device just needs to complete a data transfer within
>100ms and keep the data line busy. If we still use the HW timer, it
will trigger a DATA LINE timeout interrupt.

This patch does not affect scenarios where the hardware timer meets the
max data transmission time requirements of the device. It will still
use the hardware timer. Only when the device changes, will it switch to
using the SW timer.

Bean

Bean Huo Sept. 28, 2021, 9:39 a.m. UTC | #9

On Fri, 2021-09-24 at 23:33 +0200, Bean Huo wrote:
> On Fri, 2021-09-24 at 16:26 +0300, Adrian Hunter wrote:
> > On 24/09/21 4:08 pm, Bean Huo wrote:
> > > On Fri, 2021-09-24 at 15:17 +0300, Adrian Hunter wrote:
> > > > > > >          sdhci_writeb(host, count,
> > > > > > > SDHCI_TIMEOUT_CONTROL);
> > > > > > > }
> > > > > > > The driver has detected that the hardware timer cannot
> > > > > > > meet
> > > > > > > the
> > > > > > > timeout
> > > > > > > requirements of the device, but we still use the hardware
> > > > > > > timer,
> > > > > > > which will
> > > > > > > allow potential timeout issuea . Rather than allowing a
> > > > > > > potential
> > > > > > > problem to exist, why can’t software timing be used to
> > > > > > > avoid
> > > > > > > this
> > > > > > > problem?
> > > > > > Timeouts aren't that accurate.  The maximum is assumed
> > > > > > still
> > > > > > to
> > > > > > work.
> > > > > > mmc->max_busy_timeout is used to tell the core what the
> > > > > > maximum
> > > > > > is.
> > > > > mmc->max_busy_timeout is still a representation of Host HW
> > > > > timer
> > > > > maximum timeout count, isn't it? 
> > > > 
> > > > Not necessarily.  For SDHCI_QUIRK2_DISABLE_HW_TIMEOUT it would
> > > > be
> > > > 
> > > > set to zero to indicate no maximum.
> > > 
> > > yes, this is the purpose of the patch, for the host controller
> > > without
> > > quirk SDHCI_QUIRK2_DISABLE_HW_TIMEOUT, if the timeout count
> > > required by
> > > device is beyond the HW timer max count, we choose SW timer to
> > > avoid the HW timer timeout IRQ.
> > > 
> > > I don't know if I get it correctly.
> > 
> > Why can't drivers that want the behaviour just set the quirk?
> > 
> > Drivers that do not work with the quirk, do not have to set it.
> 
> Adrian,
> 
> We cannot add this quirk to every host driver. This is the difference
> on the device side. In addition, I don't know what the maximum
> hardware
> timer budget for each host is. Even if you use the same SOC, the
> maximum time budget on different platforms may be different.
> 
> Assume that the maximum timeout time supported by the hardware timer
> is
> 100 milliseconds, but the maximum data transmission time required by
> the device is 150 milliseconds. In most cases, data transfer will not
> take so long. 150 is the maximum time under extreme conditions. This
> means that the device just needs to complete a data transfer within
> > 100ms and keep the data line busy. If we still use the HW timer, it
> will trigger a DATA LINE timeout interrupt.
> 
> This patch does not affect scenarios where the hardware timer meets
> the
> max data transmission time requirements of the device. It will still
> use the hardware timer. Only when the device changes, will it switch
> to
> using the SW timer.
> 
> Bean
> 

Hi Adrian,

Would you please review this patch again and is it possible to be
accepted?

Bean

Adrian Hunter Sept. 28, 2021, 10:18 a.m. UTC | #10

On 25/09/2021 00:33, Bean Huo wrote:
> On Fri, 2021-09-24 at 16:26 +0300, Adrian Hunter wrote:
>> On 24/09/21 4:08 pm, Bean Huo wrote:
>>> On Fri, 2021-09-24 at 15:17 +0300, Adrian Hunter wrote:
>>>>>>>          sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
>>>>>>> }
>>>>>>> The driver has detected that the hardware timer cannot meet
>>>>>>> the
>>>>>>> timeout
>>>>>>> requirements of the device, but we still use the hardware
>>>>>>> timer,
>>>>>>> which will
>>>>>>> allow potential timeout issuea . Rather than allowing a
>>>>>>> potential
>>>>>>> problem to exist, why can’t software timing be used to
>>>>>>> avoid
>>>>>>> this
>>>>>>> problem?
>>>>>> Timeouts aren't that accurate.  The maximum is assumed still
>>>>>> to
>>>>>> work.
>>>>>> mmc->max_busy_timeout is used to tell the core what the
>>>>>> maximum
>>>>>> is.
>>>>> mmc->max_busy_timeout is still a representation of Host HW
>>>>> timer
>>>>> maximum timeout count, isn't it? 
>>>>
>>>> Not necessarily.  For SDHCI_QUIRK2_DISABLE_HW_TIMEOUT it would be
>>>>
>>>> set to zero to indicate no maximum.
>>>
>>> yes, this is the purpose of the patch, for the host controller
>>> without
>>> quirk SDHCI_QUIRK2_DISABLE_HW_TIMEOUT, if the timeout count
>>> required by
>>> device is beyond the HW timer max count, we choose SW timer to
>>> avoid the HW timer timeout IRQ.
>>>
>>> I don't know if I get it correctly.
>>
>> Why can't drivers that want the behaviour just set the quirk?
>>
>> Drivers that do not work with the quirk, do not have to set it.
> 
> 
> Adrian,
> 
> We cannot add this quirk to every host driver.

I was suggesting only the ones for which it works.

>  This is the difference
> on the device side.

It is the host controller that has the problem, not the device.
Hence the quirk.

> In addition, I don't know what the maximum hardware
> timer budget for each host is.

mmc->max_busy_timeout is calculated by sdhci.c, or the driver can
override the maximum count via ->get_max_timeout_count() or the driver
can override mmc->max_busy_timeout.

With the quirk, sdhci.c will usually set mmc->max_busy_timeout to zero.
That allows timeouts greater than the hardware can support, and then,
with the quirk, the driver will switch to a software timeout when needed.

However, that won't work for every host controller, because some do not
provide a completion interrupt after the timeout, even if the timeout
interrupt is disabled.  That means they should set mmc->max_busy_timeout
to the hardware value.  Hence the quirk is needed to tell the difference.

> Even if you use the same SOC, the
> maximum time budget on different platforms may be different.

The mmc core calculates timeouts based on the relevant standards and
values provided by the device itself.

> Assume that the maximum timeout time supported by the hardware timer is
> 100 milliseconds

I realise it is an example, but 100 milliseconds is a bit low. Legacy
host controllers have always had to deal with standard SD card and
MMC card timeouts.  SD card write timeout of 500 milliseconds for instance.

> but the maximum data transmission time required by
> the device is 150 milliseconds. In most cases, data transfer will not
> take so long. 150 is the maximum time under extreme conditions. This
> means that the device just needs to complete a data transfer within
>> 100ms and keep the data line busy. If we still use the HW timer, it
> will trigger a DATA LINE timeout interrupt.
> 
> This patch does not affect scenarios where the hardware timer meets the
> max data transmission time requirements of the device. It will still
> use the hardware timer. Only when the device changes, will it switch to
> using the SW timer.

Which is what the quirk does.  So I am very confused why the quirk is
no good?

Bean Huo Sept. 29, 2021, 10:49 a.m. UTC | #11

Hi Adrian,

On Tue, 2021-09-28 at 13:18 +0300, Adrian Hunter wrote:
> On 25/09/2021 00:33, Bean Huo wrote:
> > On Fri, 2021-09-24 at 16:26 +0300, Adrian Hunter wrote:
> > > On 24/09/21 4:08 pm, Bean Huo wrote:
> > > > On Fri, 2021-09-24 at 15:17 +0300, Adrian Hunter wrote:
> > > > > > > >          sdhci_writeb(host, count,
> > > > > > > > SDHCI_TIMEOUT_CONTROL);
> > > > > > > > }
> > > > > > > > The driver has detected that the hardware timer cannot
> > > > > > > > meet
> > > > > > > > the
> > > > > > > > timeout
> > > > > > > > requirements of the device, but we still use the
> > > > > > > > hardware
> > > > > > > > timer,
> > > > > > > > which will
> > > > > > > > allow potential timeout issuea . Rather than allowing a
> > > > > > > > potential
> > > > > > > > problem to exist, why can’t software timing be used to
> > > > > > > > avoid
> > > > > > > > this
> > > > > > > > problem?
> > > > > > > Timeouts aren't that accurate.  The maximum is assumed
> > > > > > > still
> > > > > > > to
> > > > > > > work.
> > > > > > > mmc->max_busy_timeout is used to tell the core what the
> > > > > > > maximum
> > > > > > > is.
> > > > > > mmc->max_busy_timeout is still a representation of Host HW
> > > > > > timer
> > > > > > maximum timeout count, isn't it? 
> > > > > 
> > > > > Not necessarily.  For SDHCI_QUIRK2_DISABLE_HW_TIMEOUT it
> > > > > would be
> > > > > 
> > > > > set to zero to indicate no maximum.
> > > > 
> > > > yes, this is the purpose of the patch, for the host controller
> > > > without
> > > > quirk SDHCI_QUIRK2_DISABLE_HW_TIMEOUT, if the timeout count
> > > > required by
> > > > device is beyond the HW timer max count, we choose SW timer to
> > > > avoid the HW timer timeout IRQ.
> > > > 
> > > > I don't know if I get it correctly.
> > > 
> > > Why can't drivers that want the behaviour just set the quirk?
> > > 
> > > Drivers that do not work with the quirk, do not have to set it.
> > 
> > Adrian,
> > 
> > We cannot add this quirk to every host driver.
> 
> I was suggesting only the ones for which it works.
> 
> >  This is the difference
> > on the device side.
> 
> It is the host controller that has the problem, not the device.
> Hence the quirk.
> 
> > In addition, I don't know what the maximum hardware
> > timer budget for each host is.
> 
> mmc->max_busy_timeout is calculated by sdhci.c, or the driver can
> override the maximum count via ->get_max_timeout_count() or the
> driver
> can override mmc->max_busy_timeout.
> 
> With the quirk, sdhci.c will usually set mmc->max_busy_timeout to
> zero.
> That allows timeouts greater than the hardware can support, and then,
> with the quirk, the driver will switch to a software timeout when
> needed.
> 


According to your above statement, do you mean that the eMMC host
controller does not support the scenario where the data transmission
timeout value required by the eMMC device is greater than the capacity
of the eMMC host hardware data transmission timer? Unless the eMMC host
vendor accepts their eMMC host accepts the use of SW timers in this
case (adding quirks)?


> However, that won't work for every host controller, because some do
> not
> provide a completion interrupt after the timeout, even if the timeout
> interrupt is disabled.  

Do you mean that if we disable the hardware timer/timeout interrupt and
use the software timer, the eMMC host controller will not trigger a
completion interrupt? Even before the SW timer expires, the data
transfer between the host and the eMMC device is complete? Is this what
you mean?


> That means they should set mmc->max_busy_timeout
> to the hardware value.  Hence the quirk is needed to tell the
> difference.
> 

This means this quirk is for eMMC host can privode the completion
interrupt in case HW timer is disabled?  


> > Even if you use the same SOC, the
> > maximum time budget on different platforms may be different.
> 
> The mmc core calculates timeouts based on the relevant standards and
> values provided by the device itself.


Yes, but the eMMC standard does not define the maximum timeout value.
Different eMMC will have different timeout values.

> 
> > Assume that the maximum timeout time supported by the hardware
> > timer is
> > 100 milliseconds
> 
> I realise it is an example, but 100 milliseconds is a bit low. Legacy
> host controllers have always had to deal with standard SD card and
> MMC card timeouts.  SD card write timeout of 500 milliseconds for
> instance.


Yes, this is just an example. I have several platforms, they are TI,
NXP, Intel and Qcom. The timeout time of the hardware timer is
different, the greatest one is 1.3s, some are less than 500ms.

> 
> > but the maximum data transmission time required by
> > the device is 150 milliseconds. In most cases, data transfer will
> > not
> > take so long. 150 is the maximum time under extreme conditions.
> > This
> > means that the device just needs to complete a data transfer within
> > > 100ms and keep the data line busy. If we still use the HW timer,
> > > it
> > will trigger a DATA LINE timeout interrupt.
> > 
> > This patch does not affect scenarios where the hardware timer meets
> > the
> > max data transmission time requirements of the device. It will
> > still
> > use the hardware timer. Only when the device changes, will it
> > switch to
> > using the SW timer.
> 
> Which is what the quirk does.  So I am very confused why the quirk is
> no goo

Because I don't know what the maximum volume of the real hardware timer
of each host controller is. And different hosts have different timer
capacities. Meanwhile, the eMMC devices have different data
transmission timeout between the different density/series as well.


Would you please confirm your three points above? If they are true, I
agree we cannot disable hardware timers and use SW tiner on some
platforms.

Thanks in advance.

Bean

Adrian Hunter Sept. 29, 2021, 12:38 p.m. UTC | #12

On 29/09/2021 13:49, Bean Huo wrote:
> Hi Adrian,
> 
> On Tue, 2021-09-28 at 13:18 +0300, Adrian Hunter wrote:
>> On 25/09/2021 00:33, Bean Huo wrote:
>>> On Fri, 2021-09-24 at 16:26 +0300, Adrian Hunter wrote:
>>>> On 24/09/21 4:08 pm, Bean Huo wrote:
>>>>> On Fri, 2021-09-24 at 15:17 +0300, Adrian Hunter wrote:
>>>>>>>>>          sdhci_writeb(host, count,
>>>>>>>>> SDHCI_TIMEOUT_CONTROL);
>>>>>>>>> }
>>>>>>>>> The driver has detected that the hardware timer cannot
>>>>>>>>> meet
>>>>>>>>> the
>>>>>>>>> timeout
>>>>>>>>> requirements of the device, but we still use the
>>>>>>>>> hardware
>>>>>>>>> timer,
>>>>>>>>> which will
>>>>>>>>> allow potential timeout issuea . Rather than allowing a
>>>>>>>>> potential
>>>>>>>>> problem to exist, why can’t software timing be used to
>>>>>>>>> avoid
>>>>>>>>> this
>>>>>>>>> problem?
>>>>>>>> Timeouts aren't that accurate.  The maximum is assumed
>>>>>>>> still
>>>>>>>> to
>>>>>>>> work.
>>>>>>>> mmc->max_busy_timeout is used to tell the core what the
>>>>>>>> maximum
>>>>>>>> is.
>>>>>>> mmc->max_busy_timeout is still a representation of Host HW
>>>>>>> timer
>>>>>>> maximum timeout count, isn't it? 
>>>>>>
>>>>>> Not necessarily.  For SDHCI_QUIRK2_DISABLE_HW_TIMEOUT it
>>>>>> would be
>>>>>>
>>>>>> set to zero to indicate no maximum.
>>>>>
>>>>> yes, this is the purpose of the patch, for the host controller
>>>>> without
>>>>> quirk SDHCI_QUIRK2_DISABLE_HW_TIMEOUT, if the timeout count
>>>>> required by
>>>>> device is beyond the HW timer max count, we choose SW timer to
>>>>> avoid the HW timer timeout IRQ.
>>>>>
>>>>> I don't know if I get it correctly.
>>>>
>>>> Why can't drivers that want the behaviour just set the quirk?
>>>>
>>>> Drivers that do not work with the quirk, do not have to set it.
>>>
>>> Adrian,
>>>
>>> We cannot add this quirk to every host driver.
>>
>> I was suggesting only the ones for which it works.
>>
>>>  This is the difference
>>> on the device side.
>>
>> It is the host controller that has the problem, not the device.
>> Hence the quirk.
>>
>>> In addition, I don't know what the maximum hardware
>>> timer budget for each host is.
>>
>> mmc->max_busy_timeout is calculated by sdhci.c, or the driver can
>> override the maximum count via ->get_max_timeout_count() or the
>> driver
>> can override mmc->max_busy_timeout.
>>
>> With the quirk, sdhci.c will usually set mmc->max_busy_timeout to
>> zero.
>> That allows timeouts greater than the hardware can support, and then,
>> with the quirk, the driver will switch to a software timeout when
>> needed.
>>
> 
> 
> According to your above statement, do you mean that the eMMC host
> controller does not support the scenario where the data transmission
> timeout value required by the eMMC device is greater than the capacity
> of the eMMC host hardware data transmission timer? Unless the eMMC host
> vendor accepts their eMMC host accepts the use of SW timers in this
> case (adding quirks)?

Yes

>> However, that won't work for every host controller, because some do
>> not
>> provide a completion interrupt after the timeout, even if the timeout
>> interrupt is disabled.  
> 
> Do you mean that if we disable the hardware timer/timeout interrupt and
> use the software timer, the eMMC host controller will not trigger a
> completion interrupt?

Yes.  On some hardware, if the timeout interrupt is disabled, and the
operation takes longer than the hardware timeout value, then there is
no completion interrupt.  It is as if the timeout "happens" even if the
interrupt is disabled.

> Even before the SW timer expires, the data
> transfer between the host and the eMMC device is complete? Is this what
> you mean?

I'm not 100% sure about what happens if the operation completes
before the hardware timeout value, but I think it gets a completion
interrupt same as normal.

> 
> 
>> That means they should set mmc->max_busy_timeout
>> to the hardware value.  Hence the quirk is needed to tell the
>> difference.
>>
> 
> This means this quirk is for eMMC host can privode the completion
> interrupt in case HW timer is disabled?  

Yes

>>> Even if you use the same SOC, the
>>> maximum time budget on different platforms may be different.
>>
>> The mmc core calculates timeouts based on the relevant standards and
>> values provided by the device itself.
> 
> 
> Yes, but the eMMC standard does not define the maximum timeout value.
> Different eMMC will have different timeout values.
> 
>>
>>> Assume that the maximum timeout time supported by the hardware
>>> timer is
>>> 100 milliseconds
>>
>> I realise it is an example, but 100 milliseconds is a bit low. Legacy
>> host controllers have always had to deal with standard SD card and
>> MMC card timeouts.  SD card write timeout of 500 milliseconds for
>> instance.
> 
> 
> Yes, this is just an example. I have several platforms, they are TI,
> NXP, Intel and Qcom. The timeout time of the hardware timer is
> different, the greatest one is 1.3s, some are less than 500ms.
> 
>>
>>> but the maximum data transmission time required by
>>> the device is 150 milliseconds. In most cases, data transfer will
>>> not
>>> take so long. 150 is the maximum time under extreme conditions.
>>> This
>>> means that the device just needs to complete a data transfer within
>>>> 100ms and keep the data line busy. If we still use the HW timer,
>>>> it
>>> will trigger a DATA LINE timeout interrupt.
>>>
>>> This patch does not affect scenarios where the hardware timer meets
>>> the
>>> max data transmission time requirements of the device. It will
>>> still
>>> use the hardware timer. Only when the device changes, will it
>>> switch to
>>> using the SW timer.
>>
>> Which is what the quirk does.  So I am very confused why the quirk is
>> no goo
> 
> Because I don't know what the maximum volume of the real hardware timer
> of each host controller is.> And different hosts have different timer
> capacities.
> Meanwhile, the eMMC devices have different data
> transmission timeout between the different density/series as well.
> 
> Would you please confirm your three points above? If they are true, I
> agree we cannot disable hardware timers and use SW tiner on some
> platforms.

Yes, I expect the quirk will do what you need.  Have you tried it?

Bean Huo Sept. 30, 2021, 8:34 a.m. UTC | #13

Hi Adrian,


Thanks.
I want to give a short conclusion  for our discussion:

Based on your information, these sounds disable of HW timer timeout
interrupt will make eMMC host controller malfunction, in another word,
the disable of timeout interrupt will make the eMMC host cannot
correctly provide the completion interrupt. And unless only when the
SOC vendor signals that their SOC supports that the host side SW can
disable this HW timeout interrupt, as TI does.

I studied the SDHCI Spec, and tried to see if there is this kind of
support statement, but not been found yet. I will check with other SOC
vendors.

I have one more question, if you know, please give me your information.

I did testing on HW timer bahevior in case CQE is on.  Currently, we
always set the HW timer with the maximum timeout value if CQE is on.
Based on my testing, the HW timer will never timeout when we enable
CQE. I changed the HW timer value to be lower, it is the same result.
Do you know that the HW timer will be inactivated in case CQE is
on?  but its timeout interrupt is still enabled.

Kind regards,
Bean

Adrian Hunter Sept. 30, 2021, 8:59 a.m. UTC | #14

On 30/09/2021 11:34, Bean Huo wrote:
> Hi Adrian,
> 
> 
> Thanks.
> I want to give a short conclusion  for our discussion:
> 
> Based on your information, these sounds disable of HW timer timeout
> interrupt will make eMMC host controller malfunction, in another word,
> the disable of timeout interrupt will make the eMMC host cannot
> correctly provide the completion interrupt. And unless only when the
> SOC vendor signals that their SOC supports that the host side SW can
> disable this HW timeout interrupt, as TI does.
> 
> I studied the SDHCI Spec, and tried to see if there is this kind of
> support statement, but not been found yet. I will check with other SOC
> vendors.
> 
> I have one more question, if you know, please give me your information.
> 
> I did testing on HW timer bahevior in case CQE is on.  Currently, we
> always set the HW timer with the maximum timeout value if CQE is on.
> Based on my testing, the HW timer will never timeout when we enable
> CQE. I changed the HW timer value to be lower, it is the same result.
> Do you know that the HW timer will be inactivated in case CQE is
> on?  but its timeout interrupt is still enabled.

No I don't know how different CQE handle timeouts.

> 
> Kind regards,
> Bean
>

Bean Huo Sept. 30, 2021, 9:02 a.m. UTC | #15

On Thu, 2021-09-30 at 11:59 +0300, Adrian Hunter wrote:
> On 30/09/2021 11:34, Bean Huo wrote:
> > Hi Adrian,
> > 
> > 
> > Thanks.
> > I want to give a short conclusion  for our discussion:
> > 
> > Based on your information, these sounds disable of HW timer timeout
> > interrupt will make eMMC host controller malfunction, in another
> > word,
> > the disable of timeout interrupt will make the eMMC host cannot
> > correctly provide the completion interrupt. And unless only when
> > the
> > SOC vendor signals that their SOC supports that the host side SW
> > can
> > disable this HW timeout interrupt, as TI does.
> > 
> > I studied the SDHCI Spec, and tried to see if there is this kind of
> > support statement, but not been found yet. I will check with other
> > SOC
> > vendors.
> > 
> > I have one more question, if you know, please give me your
> > information.
> > 
> > I did testing on HW timer bahevior in case CQE is on.  Currently,
> > we
> > always set the HW timer with the maximum timeout value if CQE is
> > on.
> > Based on my testing, the HW timer will never timeout when we enable
> > CQE. I changed the HW timer value to be lower, it is the same
> > result.
> > Do you know that the HW timer will be inactivated in case CQE is
> > on?  but its timeout interrupt is still enabled.
> 
> No I don't know how different CQE handle timeouts.

Thanks anyway.

Bean

> 
> > Kind regards,
> > Bean
> >

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 357b365bf0ec..463517fd9886 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -969,9 +969,6 @@  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd,
 		count++;
 		current_timeout <<= 1;
 		if (count > host->max_timeout_count) {
-			if (!(host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT))
-				DBG("Too large timeout 0x%x requested for CMD%d!\n",
-				    count, cmd->opcode);
 			count = host->max_timeout_count;
 			*too_big = true;
 			break;
@@ -1016,8 +1013,7 @@  void __sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 	bool too_big = false;
 	u8 count = sdhci_calc_timeout(host, cmd, &too_big);
 
-	if (too_big &&
-	    host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) {
+	if (too_big) {
 		sdhci_calc_sw_timeout(host, cmd);
 		sdhci_set_data_timeout_irq(host, false);
 	} else if (!(host->ier & SDHCI_INT_DATA_TIMEOUT)) {