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

[Bean Huo]

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