Subject: Re: [RFC PATCH 09/12] mmc: sdhci: Use software timer when timeout
 greater than hardware capablility
To: Kishon Vijay Abraham I <kishon@ti.com>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Tony Lindgren <tony@atomide.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
        Russell King <linux@armlinux.org.uk>,
        linux-mmc@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, nsekhar@ti.com
References: <20171214130941.26666-1-kishon@ti.com>
 <20171214130941.26666-10-kishon@ti.com>
 <74170f0a-329a-2c66-7ad4-a1fedb9747df@intel.com>
 <2da01441-84d0-0d96-610d-2f9a3055ccc7@ti.com>
From: Adrian Hunter <adrian.hunter@intel.com>
Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki,
 Business Identity Code: 0357606 - 4, Domiciled in Helsinki
Message-ID: <2a41ba31-07e5-027a-de30-495f69d71a88@intel.com>
Date: Thu, 11 Jan 2018 10:46:06 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <2da01441-84d0-0d96-610d-2f9a3055ccc7@ti.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org

On 04/01/18 14:59, Kishon Vijay Abraham I wrote:
> Hi Adrian,
> 
> On Wednesday 20 December 2017 07:41 PM, Adrian Hunter wrote:
>> On 14/12/17 15:09, Kishon Vijay Abraham I wrote:
>>> Errata i834 in AM572x Sitara Processors Silicon Revision 2.0, 1.1
>>> (SPRZ429K July 2014–Revised March 2017 [1]) mentions
>>> Under high speed HS200 and SDR104 modes, the functional clock for MMC
>>> modules will reach up to 192 MHz. At this frequency, the maximum obtainable
>>> timeout (DTO = 0xE) through MMC host controller is (1/192MHz)*2^27 = 700ms.
>>> Commands taking longer than 700ms may be affected by this small window
>>> frame. Workaround for this errata is use a software timer instead of
>>> hardware timer to provide the delay requested by the upper layer.
>>>
>>> While this errata is specific to AM572x, it is applicable to all sdhci
>>> based controllers when a particular request require timeout greater
>>> than hardware capability.
>>
>> It doesn't work for our controllers.  Even if the data timeout interrupt is
>> disabled, it seems like the timeout still "happens" in some fashion - after
>> which the host controller starts misbehaving.
> 
> even if the data timeout doesn't get disabled, count = 0xE is still present. So
> ideally this shouldn't break any existing platforms no?

I don't want to hide this kind of variation in the hardware behaviour.

>>
>> So you will need to add a quirk.
>>
>>>
>>> Re-use the software timer already implemented in sdhci to program the
>>> correct timeout value and also disable the hardware timeout when
>>> the required timeout is greater than hardware capabiltiy in order to
>>> avoid spurious timeout interrupts.
>>>
>>> This patch is based on the earlier patch implemented for omap_hsmmc [2]
>>>
>>> [1] -> http://www.ti.com/lit/er/sprz429k/sprz429k.pdf
>>> [2] -> https://patchwork.kernel.org/patch/9791449/
>>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>>  drivers/mmc/host/sdhci.c | 41 +++++++++++++++++++++++++++++++++++++++--
>>>  drivers/mmc/host/sdhci.h | 11 +++++++++++
>>>  2 files changed, 50 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index e9290a3439d5..d0655e1d2cc7 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -673,6 +673,27 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
>>>  	}
>>>  }
>>>  
>>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
>>> +				  struct mmc_command *cmd,
>>> +				  unsigned int target_timeout)
>>> +{
>>> +	struct mmc_host *mmc = host->mmc;
>>> +	struct mmc_ios *ios = &mmc->ios;
>>> +	struct mmc_data *data = cmd->data;
>>> +	unsigned long long transfer_time;
>>> +
>>> +	if (data) {
>>> +		transfer_time = MMC_BLOCK_TRANSFER_TIME_MS(data->blksz,
>>> +							   ios->bus_width,
>>> +							   ios->clock);
>>
>> If it has a value, actual_clock is better than ios->clock.
> 
> okay.
>>
>>> +		/* calculate timeout for the entire data */
>>> +		host->data_timeout = (data->blocks * (target_timeout +
>>> +						      transfer_time));
>>> +	} else if (cmd->flags & MMC_RSP_BUSY) {
>>> +		host->data_timeout = cmd->busy_timeout * MSEC_PER_SEC;
>>
>> Doesn't need MSEC_PER_SEC multiplier.
> 
> right.
>>
>>> +	}
>>> +}
>>> +
>>>  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>>>  {
>>>  	u8 count;
>>> @@ -732,8 +753,12 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>>>  	}
>>>  
>>>  	if (count >= 0xF) {
>>> -		DBG("Too large timeout 0x%x requested for CMD%d!\n",
>>> -		    count, cmd->opcode);
>>> +		DBG("Too large timeout.. using SW timeout for CMD%d!\n",
>>> +		    cmd->opcode);
>>> +		sdhci_calc_sw_timeout(host, cmd, target_timeout);
>>> +		host->ier &= ~SDHCI_INT_DATA_TIMEOUT;
>>> +		sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
>>> +		sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
>>>  		count = 0xE;
>>>  	}
>>>  
>>> @@ -1198,6 +1223,14 @@ static void sdhci_finish_command(struct sdhci_host *host)
>>>  {
>>>  	struct mmc_command *cmd = host->cmd;
>>>  
>>> +	if (host->data_timeout) {
>>> +		unsigned long timeout;
>>> +
>>> +		timeout = jiffies +
>>> +			  msecs_to_jiffies(host->data_timeout);
>>> +		sdhci_mod_timer(host, host->cmd->mrq, timeout);
>>
>> cmd could be the sbc or a stop cmd or a command during transfer, so this
>> needs more logic.
> 
> host->data_timeout gets set only for data commands or commands with busy
> timeout. But I guess for commands during data transfer, host->data_timeout
> might still be set?
> 
> Checking sdhci_data_line_cmd(mrq->cmd) in addition to host->data_timeout should
> take care of all cases right?

I suggest you make the timeout calculation allow for the commands as well
and then reorder sdhci_mod_timer() to be called after sdhci_prepare_data()
and make sdhci_mod_timer() do the right thing.

>>
>>> +	}
>>> +
>>>  	host->cmd = NULL;
>>>  
>>>  	if (cmd->flags & MMC_RSP_PRESENT) {
>>> @@ -2341,6 +2374,10 @@ static bool sdhci_request_done(struct sdhci_host *host)
>>>  		return true;
>>>  	}
>>>  
>>> +	host->data_timeout = 0;
>>> +	host->ier |= SDHCI_INT_DATA_TIMEOUT;
>>> +	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
>>> +	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
>>
>> sdhci can have 2 requests in progress to allow for commands to be sent while
>> a data transfer is in progress, so this is not necessarily the data transfer
>> request that is done.  Also we want to avoid unnecessary register writes.
>>
> 
> okay.. got it.
>>>  	sdhci_del_timer(host, mrq);
>>>  
>>>  	/*
>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>> index 54bc444c317f..e6e0278bea1a 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -332,6 +332,15 @@ struct sdhci_adma2_64_desc {
>>>  /* Allow for a a command request and a data request at the same time */
>>>  #define SDHCI_MAX_MRQS		2
>>>  
>>> +/*
>>> + * Time taken for transferring one block. It is multiplied by a constant
>>> + * factor '2' to account for any errors
>>> + */
>>> +#define MMC_BLOCK_TRANSFER_TIME_MS(blksz, bus_width, freq)		\
>>> +				   ((unsigned long long)		\
>>> +				   (2 * (((blksz) * MSEC_PER_SEC *	\
>>> +				   (8 / (bus_width))) / (freq))))
>>
>> I don't think the macro helps make the code more readable.  Might just as
>> well write a nice function to calculate the entire data request timeout.
> 
> okay.
>>
>>> +
>>>  enum sdhci_cookie {
>>>  	COOKIE_UNMAPPED,
>>>  	COOKIE_PRE_MAPPED,	/* mapped by sdhci_pre_req() */
>>> @@ -546,6 +555,8 @@ struct sdhci_host {
>>>  	/* Host SDMA buffer boundary. */
>>>  	u32			sdma_boundary;
>>>  
>>> +	unsigned long long	data_timeout;
>>
>> msecs_to_jiffies() will truncate to 'unsigned int' anyway, so this might as
>> well be 'unsigned int'.
>>
> 
> okay.
> 
> Thanks
> Kishon
>