Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <40e525300cd656dd17ffc89e1fcbc9a47ea90caf.camel@gmail.com>
Subject: Re: [PATCH v1 2/2] mmc: sdhci: Use the SW timer when the HW timer
 cannot meet the timeout value required by the device
From:   Bean Huo <huobean@gmail.com>
To:     Adrian Hunter <adrian.hunter@intel.com>,
        Ulf Hansson <ulf.hansson@linaro.org>
Cc:     Bean Huo <beanhuo@micron.com>, linux-mmc@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Fri, 24 Sep 2021 13:45:54 +0200
In-Reply-To: <93292ef4-8548-d2ba-d803-d3b40b7e6c1d@intel.com>
References: <20210917172727.26834-1-huobean@gmail.com>
         <20210917172727.26834-3-huobean@gmail.com>
         <fc14d8e1-9438-d4b0-80f4-ccf9055ab7d3@intel.com>
         <beda2d5ecc3c15e9bf9aa18383c22c2a90d31dab.camel@gmail.com>
         <93292ef4-8548-d2ba-d803-d3b40b7e6c1d@intel.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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