Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-04-04 Thread Adrian Hunter 
On 20/03/18 11:48, Kishon Vijay Abraham I wrote:
> Hi Adrian,
> 
> On Monday 19 March 2018 03:49 PM, Kishon Vijay Abraham I wrote:
>> Hi Adrian,
>>
>> On Monday 19 March 2018 03:30 PM, Adrian Hunter wrote:
>>> On 19/03/18 11:20, Kishon Vijay Abraham I wrote:
 Hi Adrian,

 On Friday 16 March 2018 07:51 PM, Adrian Hunter wrote:
> On 16/03/18 08:29, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote:
>>> On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
 sdhci has a 10 second timeout to catch devices that stop responding.
 Instead of programming 10 second arbitrary value, calculate the total 
 time
 it would take for the entire transfer to happen and program the timeout
 value accordingly.

 Signed-off-by: Kishon Vijay Abraham I 
 ---
  drivers/mmc/host/sdhci.c | 47 
 ---
  drivers/mmc/host/sdhci.h | 10 ++
  2 files changed, 50 insertions(+), 7 deletions(-)

 diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
 index 1dd117cbeb6e..baab67bfa39b 100644
 --- a/drivers/mmc/host/sdhci.c
 +++ b/drivers/mmc/host/sdhci.c
 @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host 
 *host)
return sg_dma_address(host->data->sg);
  }
  
 +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
 +struct mmc_command *cmd,
 +unsigned int target_timeout)
 +{
 +  struct mmc_data *data = cmd->data;
 +  struct mmc_host *mmc = host->mmc;
 +  u64 transfer_time;
 +  struct mmc_ios *ios = >ios;
 +  unsigned char bus_width = 1 << ios->bus_width;
 +  unsigned int blksz;
 +  unsigned int freq;
 +
 +  if (data) {
 +  blksz = data->blksz;
 +  freq = host->mmc->actual_clock ? : host->clock;
 +  transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / 
 bus_width);
 +  do_div(transfer_time, freq);
 +  /* multiply by '2' to account for any unknowns */
 +  transfer_time = transfer_time * 2;
 +  /* calculate timeout for the entire data */
 +  host->data_timeout = (data->blocks * ((target_timeout *
 + NSEC_PER_USEC) +
 + transfer_time));
>>>
>>> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
>>> for timeouts greater than about 4 seconds.
>>>
 +  } else {
 +  host->data_timeout = (u64)target_timeout * 
 NSEC_PER_USEC;
 +  }
 +
 +  host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>>
>>> Need to allow for target_timeout == 0 so:
>>>
>>> if (host->data_timeout)
>>> host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>>
 +}
 +
  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct 
 mmc_command *cmd)
  {
u8 count;
 @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host 
 *host, struct mmc_command *cmd)
if (count >= 0xF)
break;
}
 +  sdhci_calc_sw_timeout(host, cmd, target_timeout);
>>>
>>> If you make the changes I suggest for patch 6, then this would
>>> move sdhci_calc_sw_timeout() into sdhci_set_timeout().
>>>
>>> I suggest you factor out the target_timeout calculation e.g.
>>>
>>> static unsigned int sdhci_target_timeout(struct sdhci_host *host,
>>>  struct mmc_command *cmd,
>>>  struct mmc_data *data)
>>> {
>>> unsigned int target_timeout;
>>>
>>> /* timeout in us */
>>> if (!data)
>>> target_timeout = cmd->busy_timeout * 1000;
>>> else {
>>> target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
>>> if (host->clock && data->timeout_clks) {
>>> unsigned long long val;
>>>
>>> /*
>>>  * data->timeout_clks is in units of clock 
>>> cycles.
>>>  * host->clock is in Hz.  target_timeout is in 
>>> us.
>>>  * Hence, us = 100 * cycles / Hz.  Round up.
>>>  */
>>>

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-04-04 Thread Adrian Hunter 
On 20/03/18 11:48, Kishon Vijay Abraham I wrote:
> Hi Adrian,
> 
> On Monday 19 March 2018 03:49 PM, Kishon Vijay Abraham I wrote:
>> Hi Adrian,
>>
>> On Monday 19 March 2018 03:30 PM, Adrian Hunter wrote:
>>> On 19/03/18 11:20, Kishon Vijay Abraham I wrote:
 Hi Adrian,

 On Friday 16 March 2018 07:51 PM, Adrian Hunter wrote:
> On 16/03/18 08:29, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote:
>>> On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
 sdhci has a 10 second timeout to catch devices that stop responding.
 Instead of programming 10 second arbitrary value, calculate the total 
 time
 it would take for the entire transfer to happen and program the timeout
 value accordingly.

 Signed-off-by: Kishon Vijay Abraham I 
 ---
  drivers/mmc/host/sdhci.c | 47 
 ---
  drivers/mmc/host/sdhci.h | 10 ++
  2 files changed, 50 insertions(+), 7 deletions(-)

 diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
 index 1dd117cbeb6e..baab67bfa39b 100644
 --- a/drivers/mmc/host/sdhci.c
 +++ b/drivers/mmc/host/sdhci.c
 @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host 
 *host)
return sg_dma_address(host->data->sg);
  }
  
 +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
 +struct mmc_command *cmd,
 +unsigned int target_timeout)
 +{
 +  struct mmc_data *data = cmd->data;
 +  struct mmc_host *mmc = host->mmc;
 +  u64 transfer_time;
 +  struct mmc_ios *ios = >ios;
 +  unsigned char bus_width = 1 << ios->bus_width;
 +  unsigned int blksz;
 +  unsigned int freq;
 +
 +  if (data) {
 +  blksz = data->blksz;
 +  freq = host->mmc->actual_clock ? : host->clock;
 +  transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / 
 bus_width);
 +  do_div(transfer_time, freq);
 +  /* multiply by '2' to account for any unknowns */
 +  transfer_time = transfer_time * 2;
 +  /* calculate timeout for the entire data */
 +  host->data_timeout = (data->blocks * ((target_timeout *
 + NSEC_PER_USEC) +
 + transfer_time));
>>>
>>> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
>>> for timeouts greater than about 4 seconds.
>>>
 +  } else {
 +  host->data_timeout = (u64)target_timeout * 
 NSEC_PER_USEC;
 +  }
 +
 +  host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>>
>>> Need to allow for target_timeout == 0 so:
>>>
>>> if (host->data_timeout)
>>> host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>>
 +}
 +
  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct 
 mmc_command *cmd)
  {
u8 count;
 @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host 
 *host, struct mmc_command *cmd)
if (count >= 0xF)
break;
}
 +  sdhci_calc_sw_timeout(host, cmd, target_timeout);
>>>
>>> If you make the changes I suggest for patch 6, then this would
>>> move sdhci_calc_sw_timeout() into sdhci_set_timeout().
>>>
>>> I suggest you factor out the target_timeout calculation e.g.
>>>
>>> static unsigned int sdhci_target_timeout(struct sdhci_host *host,
>>>  struct mmc_command *cmd,
>>>  struct mmc_data *data)
>>> {
>>> unsigned int target_timeout;
>>>
>>> /* timeout in us */
>>> if (!data)
>>> target_timeout = cmd->busy_timeout * 1000;
>>> else {
>>> target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
>>> if (host->clock && data->timeout_clks) {
>>> unsigned long long val;
>>>
>>> /*
>>>  * data->timeout_clks is in units of clock 
>>> cycles.
>>>  * host->clock is in Hz.  target_timeout is in 
>>> us.
>>>  * Hence, us = 100 * cycles / Hz.  Round up.
>>>  */
>>> val =

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-20 Thread Kishon Vijay Abraham I 
Hi Adrian,

On Monday 19 March 2018 03:49 PM, Kishon Vijay Abraham I wrote:
> Hi Adrian,
> 
> On Monday 19 March 2018 03:30 PM, Adrian Hunter wrote:
>> On 19/03/18 11:20, Kishon Vijay Abraham I wrote:
>>> Hi Adrian,
>>>
>>> On Friday 16 March 2018 07:51 PM, Adrian Hunter wrote:
 On 16/03/18 08:29, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote:
>> On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
>>> sdhci has a 10 second timeout to catch devices that stop responding.
>>> Instead of programming 10 second arbitrary value, calculate the total 
>>> time
>>> it would take for the entire transfer to happen and program the timeout
>>> value accordingly.
>>>
>>> Signed-off-by: Kishon Vijay Abraham I 
>>> ---
>>>  drivers/mmc/host/sdhci.c | 47 
>>> ---
>>>  drivers/mmc/host/sdhci.h | 10 ++
>>>  2 files changed, 50 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 1dd117cbeb6e..baab67bfa39b 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host 
>>> *host)
>>> return sg_dma_address(host->data->sg);
>>>  }
>>>  
>>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
>>> + struct mmc_command *cmd,
>>> + unsigned int target_timeout)
>>> +{
>>> +   struct mmc_data *data = cmd->data;
>>> +   struct mmc_host *mmc = host->mmc;
>>> +   u64 transfer_time;
>>> +   struct mmc_ios *ios = >ios;
>>> +   unsigned char bus_width = 1 << ios->bus_width;
>>> +   unsigned int blksz;
>>> +   unsigned int freq;
>>> +
>>> +   if (data) {
>>> +   blksz = data->blksz;
>>> +   freq = host->mmc->actual_clock ? : host->clock;
>>> +   transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / 
>>> bus_width);
>>> +   do_div(transfer_time, freq);
>>> +   /* multiply by '2' to account for any unknowns */
>>> +   transfer_time = transfer_time * 2;
>>> +   /* calculate timeout for the entire data */
>>> +   host->data_timeout = (data->blocks * ((target_timeout *
>>> +  NSEC_PER_USEC) +
>>> +  transfer_time));
>>
>> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
>> for timeouts greater than about 4 seconds.
>>
>>> +   } else {
>>> +   host->data_timeout = (u64)target_timeout * 
>>> NSEC_PER_USEC;
>>> +   }
>>> +
>>> +   host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>
>> Need to allow for target_timeout == 0 so:
>>
>>  if (host->data_timeout)
>>  host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>
>>> +}
>>> +
>>>  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct 
>>> mmc_command *cmd)
>>>  {
>>> u8 count;
>>> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host 
>>> *host, struct mmc_command *cmd)
>>> if (count >= 0xF)
>>> break;
>>> }
>>> +   sdhci_calc_sw_timeout(host, cmd, target_timeout);
>>
>> If you make the changes I suggest for patch 6, then this would
>> move sdhci_calc_sw_timeout() into sdhci_set_timeout().
>>
>> I suggest you factor out the target_timeout calculation e.g.
>>
>> static unsigned int sdhci_target_timeout(struct sdhci_host *host,
>>   struct mmc_command *cmd,
>>   struct mmc_data *data)
>> {
>>  unsigned int target_timeout;
>>
>>  /* timeout in us */
>>  if (!data)
>>  target_timeout = cmd->busy_timeout * 1000;
>>  else {
>>  target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
>>  if (host->clock && data->timeout_clks) {
>>  unsigned long long val;
>>
>>  /*
>>   * data->timeout_clks is in units of clock cycles.
>>   * host->clock is in Hz.  target_timeout is in us.
>>   * Hence, us = 100 * cycles / Hz.  Round up.
>>   */
>>  val = 100ULL * data->timeout_clks;
>>  if (do_div(val, host->clock))
>>  target_timeout++;
>>  target_timeout += val;
>>  }
>>  }
>>
>>  return target_timeout;
>> }
>>
>>

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-20 Thread Kishon Vijay Abraham I 
Hi Adrian,

On Monday 19 March 2018 03:49 PM, Kishon Vijay Abraham I wrote:
> Hi Adrian,
> 
> On Monday 19 March 2018 03:30 PM, Adrian Hunter wrote:
>> On 19/03/18 11:20, Kishon Vijay Abraham I wrote:
>>> Hi Adrian,
>>>
>>> On Friday 16 March 2018 07:51 PM, Adrian Hunter wrote:
 On 16/03/18 08:29, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote:
>> On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
>>> sdhci has a 10 second timeout to catch devices that stop responding.
>>> Instead of programming 10 second arbitrary value, calculate the total 
>>> time
>>> it would take for the entire transfer to happen and program the timeout
>>> value accordingly.
>>>
>>> Signed-off-by: Kishon Vijay Abraham I 
>>> ---
>>>  drivers/mmc/host/sdhci.c | 47 
>>> ---
>>>  drivers/mmc/host/sdhci.h | 10 ++
>>>  2 files changed, 50 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 1dd117cbeb6e..baab67bfa39b 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host 
>>> *host)
>>> return sg_dma_address(host->data->sg);
>>>  }
>>>  
>>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
>>> + struct mmc_command *cmd,
>>> + unsigned int target_timeout)
>>> +{
>>> +   struct mmc_data *data = cmd->data;
>>> +   struct mmc_host *mmc = host->mmc;
>>> +   u64 transfer_time;
>>> +   struct mmc_ios *ios = >ios;
>>> +   unsigned char bus_width = 1 << ios->bus_width;
>>> +   unsigned int blksz;
>>> +   unsigned int freq;
>>> +
>>> +   if (data) {
>>> +   blksz = data->blksz;
>>> +   freq = host->mmc->actual_clock ? : host->clock;
>>> +   transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / 
>>> bus_width);
>>> +   do_div(transfer_time, freq);
>>> +   /* multiply by '2' to account for any unknowns */
>>> +   transfer_time = transfer_time * 2;
>>> +   /* calculate timeout for the entire data */
>>> +   host->data_timeout = (data->blocks * ((target_timeout *
>>> +  NSEC_PER_USEC) +
>>> +  transfer_time));
>>
>> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
>> for timeouts greater than about 4 seconds.
>>
>>> +   } else {
>>> +   host->data_timeout = (u64)target_timeout * 
>>> NSEC_PER_USEC;
>>> +   }
>>> +
>>> +   host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>
>> Need to allow for target_timeout == 0 so:
>>
>>  if (host->data_timeout)
>>  host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>
>>> +}
>>> +
>>>  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct 
>>> mmc_command *cmd)
>>>  {
>>> u8 count;
>>> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host 
>>> *host, struct mmc_command *cmd)
>>> if (count >= 0xF)
>>> break;
>>> }
>>> +   sdhci_calc_sw_timeout(host, cmd, target_timeout);
>>
>> If you make the changes I suggest for patch 6, then this would
>> move sdhci_calc_sw_timeout() into sdhci_set_timeout().
>>
>> I suggest you factor out the target_timeout calculation e.g.
>>
>> static unsigned int sdhci_target_timeout(struct sdhci_host *host,
>>   struct mmc_command *cmd,
>>   struct mmc_data *data)
>> {
>>  unsigned int target_timeout;
>>
>>  /* timeout in us */
>>  if (!data)
>>  target_timeout = cmd->busy_timeout * 1000;
>>  else {
>>  target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
>>  if (host->clock && data->timeout_clks) {
>>  unsigned long long val;
>>
>>  /*
>>   * data->timeout_clks is in units of clock cycles.
>>   * host->clock is in Hz.  target_timeout is in us.
>>   * Hence, us = 100 * cycles / Hz.  Round up.
>>   */
>>  val = 100ULL * data->timeout_clks;
>>  if (do_div(val, host->clock))
>>  target_timeout++;
>>  target_timeout += val;
>>  }
>>  }
>>
>>  return target_timeout;
>> }
>>
>> And call it

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-19 Thread Kishon Vijay Abraham I 
Hi Adrian,

On Monday 19 March 2018 03:30 PM, Adrian Hunter wrote:
> On 19/03/18 11:20, Kishon Vijay Abraham I wrote:
>> Hi Adrian,
>>
>> On Friday 16 March 2018 07:51 PM, Adrian Hunter wrote:
>>> On 16/03/18 08:29, Kishon Vijay Abraham I wrote:
 Hi,

 On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote:
> On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
>> sdhci has a 10 second timeout to catch devices that stop responding.
>> Instead of programming 10 second arbitrary value, calculate the total 
>> time
>> it would take for the entire transfer to happen and program the timeout
>> value accordingly.
>>
>> Signed-off-by: Kishon Vijay Abraham I 
>> ---
>>  drivers/mmc/host/sdhci.c | 47 
>> ---
>>  drivers/mmc/host/sdhci.h | 10 ++
>>  2 files changed, 50 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 1dd117cbeb6e..baab67bfa39b 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host 
>> *host)
>>  return sg_dma_address(host->data->sg);
>>  }
>>  
>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
>> +  struct mmc_command *cmd,
>> +  unsigned int target_timeout)
>> +{
>> +struct mmc_data *data = cmd->data;
>> +struct mmc_host *mmc = host->mmc;
>> +u64 transfer_time;
>> +struct mmc_ios *ios = >ios;
>> +unsigned char bus_width = 1 << ios->bus_width;
>> +unsigned int blksz;
>> +unsigned int freq;
>> +
>> +if (data) {
>> +blksz = data->blksz;
>> +freq = host->mmc->actual_clock ? : host->clock;
>> +transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / 
>> bus_width);
>> +do_div(transfer_time, freq);
>> +/* multiply by '2' to account for any unknowns */
>> +transfer_time = transfer_time * 2;
>> +/* calculate timeout for the entire data */
>> +host->data_timeout = (data->blocks * ((target_timeout *
>> +   NSEC_PER_USEC) +
>> +   transfer_time));
>
> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
> for timeouts greater than about 4 seconds.
>
>> +} else {
>> +host->data_timeout = (u64)target_timeout * 
>> NSEC_PER_USEC;
>> +}
>> +
>> +host->data_timeout += MMC_CMD_TRANSFER_TIME;
>
> Need to allow for target_timeout == 0 so:
>
>   if (host->data_timeout)
>   host->data_timeout += MMC_CMD_TRANSFER_TIME;
>
>> +}
>> +
>>  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct 
>> mmc_command *cmd)
>>  {
>>  u8 count;
>> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host 
>> *host, struct mmc_command *cmd)
>>  if (count >= 0xF)
>>  break;
>>  }
>> +sdhci_calc_sw_timeout(host, cmd, target_timeout);
>
> If you make the changes I suggest for patch 6, then this would
> move sdhci_calc_sw_timeout() into sdhci_set_timeout().
>
> I suggest you factor out the target_timeout calculation e.g.
>
> static unsigned int sdhci_target_timeout(struct sdhci_host *host,
>struct mmc_command *cmd,
>struct mmc_data *data)
> {
>   unsigned int target_timeout;
>
>   /* timeout in us */
>   if (!data)
>   target_timeout = cmd->busy_timeout * 1000;
>   else {
>   target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
>   if (host->clock && data->timeout_clks) {
>   unsigned long long val;
>
>   /*
>* data->timeout_clks is in units of clock cycles.
>* host->clock is in Hz.  target_timeout is in us.
>* Hence, us = 100 * cycles / Hz.  Round up.
>*/
>   val = 100ULL * data->timeout_clks;
>   if (do_div(val, host->clock))
>   target_timeout++;
>   target_timeout += val;
>   }
>   }
>
>   return target_timeout;
> }
>
> And call it from sdhci_calc_sw_timeout()
>
>>  
>>  return count;
>>  }
>> @@ -1175,13 +1206,6 @@ void

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-19 Thread Kishon Vijay Abraham I 
Hi Adrian,

On Monday 19 March 2018 03:30 PM, Adrian Hunter wrote:
> On 19/03/18 11:20, Kishon Vijay Abraham I wrote:
>> Hi Adrian,
>>
>> On Friday 16 March 2018 07:51 PM, Adrian Hunter wrote:
>>> On 16/03/18 08:29, Kishon Vijay Abraham I wrote:
 Hi,

 On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote:
> On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
>> sdhci has a 10 second timeout to catch devices that stop responding.
>> Instead of programming 10 second arbitrary value, calculate the total 
>> time
>> it would take for the entire transfer to happen and program the timeout
>> value accordingly.
>>
>> Signed-off-by: Kishon Vijay Abraham I 
>> ---
>>  drivers/mmc/host/sdhci.c | 47 
>> ---
>>  drivers/mmc/host/sdhci.h | 10 ++
>>  2 files changed, 50 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 1dd117cbeb6e..baab67bfa39b 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host 
>> *host)
>>  return sg_dma_address(host->data->sg);
>>  }
>>  
>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
>> +  struct mmc_command *cmd,
>> +  unsigned int target_timeout)
>> +{
>> +struct mmc_data *data = cmd->data;
>> +struct mmc_host *mmc = host->mmc;
>> +u64 transfer_time;
>> +struct mmc_ios *ios = >ios;
>> +unsigned char bus_width = 1 << ios->bus_width;
>> +unsigned int blksz;
>> +unsigned int freq;
>> +
>> +if (data) {
>> +blksz = data->blksz;
>> +freq = host->mmc->actual_clock ? : host->clock;
>> +transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / 
>> bus_width);
>> +do_div(transfer_time, freq);
>> +/* multiply by '2' to account for any unknowns */
>> +transfer_time = transfer_time * 2;
>> +/* calculate timeout for the entire data */
>> +host->data_timeout = (data->blocks * ((target_timeout *
>> +   NSEC_PER_USEC) +
>> +   transfer_time));
>
> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
> for timeouts greater than about 4 seconds.
>
>> +} else {
>> +host->data_timeout = (u64)target_timeout * 
>> NSEC_PER_USEC;
>> +}
>> +
>> +host->data_timeout += MMC_CMD_TRANSFER_TIME;
>
> Need to allow for target_timeout == 0 so:
>
>   if (host->data_timeout)
>   host->data_timeout += MMC_CMD_TRANSFER_TIME;
>
>> +}
>> +
>>  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct 
>> mmc_command *cmd)
>>  {
>>  u8 count;
>> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host 
>> *host, struct mmc_command *cmd)
>>  if (count >= 0xF)
>>  break;
>>  }
>> +sdhci_calc_sw_timeout(host, cmd, target_timeout);
>
> If you make the changes I suggest for patch 6, then this would
> move sdhci_calc_sw_timeout() into sdhci_set_timeout().
>
> I suggest you factor out the target_timeout calculation e.g.
>
> static unsigned int sdhci_target_timeout(struct sdhci_host *host,
>struct mmc_command *cmd,
>struct mmc_data *data)
> {
>   unsigned int target_timeout;
>
>   /* timeout in us */
>   if (!data)
>   target_timeout = cmd->busy_timeout * 1000;
>   else {
>   target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
>   if (host->clock && data->timeout_clks) {
>   unsigned long long val;
>
>   /*
>* data->timeout_clks is in units of clock cycles.
>* host->clock is in Hz.  target_timeout is in us.
>* Hence, us = 100 * cycles / Hz.  Round up.
>*/
>   val = 100ULL * data->timeout_clks;
>   if (do_div(val, host->clock))
>   target_timeout++;
>   target_timeout += val;
>   }
>   }
>
>   return target_timeout;
> }
>
> And call it from sdhci_calc_sw_timeout()
>
>>  
>>  return count;
>>  }
>> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-19 Thread Adrian Hunter 
On 19/03/18 11:20, Kishon Vijay Abraham I wrote:
> Hi Adrian,
> 
> On Friday 16 March 2018 07:51 PM, Adrian Hunter wrote:
>> On 16/03/18 08:29, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote:
 On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
> sdhci has a 10 second timeout to catch devices that stop responding.
> Instead of programming 10 second arbitrary value, calculate the total time
> it would take for the entire transfer to happen and program the timeout
> value accordingly.
>
> Signed-off-by: Kishon Vijay Abraham I 
> ---
>  drivers/mmc/host/sdhci.c | 47 
> ---
>  drivers/mmc/host/sdhci.h | 10 ++
>  2 files changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1dd117cbeb6e..baab67bfa39b 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host 
> *host)
>   return sg_dma_address(host->data->sg);
>  }
>  
> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
> +   struct mmc_command *cmd,
> +   unsigned int target_timeout)
> +{
> + struct mmc_data *data = cmd->data;
> + struct mmc_host *mmc = host->mmc;
> + u64 transfer_time;
> + struct mmc_ios *ios = >ios;
> + unsigned char bus_width = 1 << ios->bus_width;
> + unsigned int blksz;
> + unsigned int freq;
> +
> + if (data) {
> + blksz = data->blksz;
> + freq = host->mmc->actual_clock ? : host->clock;
> + transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
> + do_div(transfer_time, freq);
> + /* multiply by '2' to account for any unknowns */
> + transfer_time = transfer_time * 2;
> + /* calculate timeout for the entire data */
> + host->data_timeout = (data->blocks * ((target_timeout *
> +NSEC_PER_USEC) +
> +transfer_time));

 (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
 for timeouts greater than about 4 seconds.

> + } else {
> + host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
> + }
> +
> + host->data_timeout += MMC_CMD_TRANSFER_TIME;

 Need to allow for target_timeout == 0 so:

if (host->data_timeout)
host->data_timeout += MMC_CMD_TRANSFER_TIME;

> +}
> +
>  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command 
> *cmd)
>  {
>   u8 count;
> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
> struct mmc_command *cmd)
>   if (count >= 0xF)
>   break;
>   }
> + sdhci_calc_sw_timeout(host, cmd, target_timeout);

 If you make the changes I suggest for patch 6, then this would
 move sdhci_calc_sw_timeout() into sdhci_set_timeout().

 I suggest you factor out the target_timeout calculation e.g.

 static unsigned int sdhci_target_timeout(struct sdhci_host *host,
 struct mmc_command *cmd,
 struct mmc_data *data)
 {
unsigned int target_timeout;

/* timeout in us */
if (!data)
target_timeout = cmd->busy_timeout * 1000;
else {
target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
if (host->clock && data->timeout_clks) {
unsigned long long val;

/*
 * data->timeout_clks is in units of clock cycles.
 * host->clock is in Hz.  target_timeout is in us.
 * Hence, us = 100 * cycles / Hz.  Round up.
 */
val = 100ULL * data->timeout_clks;
if (do_div(val, host->clock))
target_timeout++;
target_timeout += val;
}
}

return target_timeout;
 }

 And call it from sdhci_calc_sw_timeout()

>  
>   return count;
>  }
> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, 
> struct mmc_command *cmd)
>   mdelay(1);
>   }
>  
> - timeout = jiffies;
> - if (!cmd->data && cmd->busy_timeout > 9000)
> - timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
> - else
> - timeout += 10 * HZ;
> - sdhci_mod_timer(host, cmd->mrq, timeout);
> -
>   host->cmd = cmd;
>   if

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-19 Thread Adrian Hunter 
On 19/03/18 11:20, Kishon Vijay Abraham I wrote:
> Hi Adrian,
> 
> On Friday 16 March 2018 07:51 PM, Adrian Hunter wrote:
>> On 16/03/18 08:29, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote:
 On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
> sdhci has a 10 second timeout to catch devices that stop responding.
> Instead of programming 10 second arbitrary value, calculate the total time
> it would take for the entire transfer to happen and program the timeout
> value accordingly.
>
> Signed-off-by: Kishon Vijay Abraham I 
> ---
>  drivers/mmc/host/sdhci.c | 47 
> ---
>  drivers/mmc/host/sdhci.h | 10 ++
>  2 files changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1dd117cbeb6e..baab67bfa39b 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host 
> *host)
>   return sg_dma_address(host->data->sg);
>  }
>  
> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
> +   struct mmc_command *cmd,
> +   unsigned int target_timeout)
> +{
> + struct mmc_data *data = cmd->data;
> + struct mmc_host *mmc = host->mmc;
> + u64 transfer_time;
> + struct mmc_ios *ios = >ios;
> + unsigned char bus_width = 1 << ios->bus_width;
> + unsigned int blksz;
> + unsigned int freq;
> +
> + if (data) {
> + blksz = data->blksz;
> + freq = host->mmc->actual_clock ? : host->clock;
> + transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
> + do_div(transfer_time, freq);
> + /* multiply by '2' to account for any unknowns */
> + transfer_time = transfer_time * 2;
> + /* calculate timeout for the entire data */
> + host->data_timeout = (data->blocks * ((target_timeout *
> +NSEC_PER_USEC) +
> +transfer_time));

 (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
 for timeouts greater than about 4 seconds.

> + } else {
> + host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
> + }
> +
> + host->data_timeout += MMC_CMD_TRANSFER_TIME;

 Need to allow for target_timeout == 0 so:

if (host->data_timeout)
host->data_timeout += MMC_CMD_TRANSFER_TIME;

> +}
> +
>  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command 
> *cmd)
>  {
>   u8 count;
> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
> struct mmc_command *cmd)
>   if (count >= 0xF)
>   break;
>   }
> + sdhci_calc_sw_timeout(host, cmd, target_timeout);

 If you make the changes I suggest for patch 6, then this would
 move sdhci_calc_sw_timeout() into sdhci_set_timeout().

 I suggest you factor out the target_timeout calculation e.g.

 static unsigned int sdhci_target_timeout(struct sdhci_host *host,
 struct mmc_command *cmd,
 struct mmc_data *data)
 {
unsigned int target_timeout;

/* timeout in us */
if (!data)
target_timeout = cmd->busy_timeout * 1000;
else {
target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
if (host->clock && data->timeout_clks) {
unsigned long long val;

/*
 * data->timeout_clks is in units of clock cycles.
 * host->clock is in Hz.  target_timeout is in us.
 * Hence, us = 100 * cycles / Hz.  Round up.
 */
val = 100ULL * data->timeout_clks;
if (do_div(val, host->clock))
target_timeout++;
target_timeout += val;
}
}

return target_timeout;
 }

 And call it from sdhci_calc_sw_timeout()

>  
>   return count;
>  }
> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, 
> struct mmc_command *cmd)
>   mdelay(1);
>   }
>  
> - timeout = jiffies;
> - if (!cmd->data && cmd->busy_timeout > 9000)
> - timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
> - else
> - timeout += 10 * HZ;
> - sdhci_mod_timer(host, cmd->mrq, timeout);
> -
>   host->cmd = cmd;
>   if

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-19 Thread Kishon Vijay Abraham I 
Hi Adrian,

On Friday 16 March 2018 07:51 PM, Adrian Hunter wrote:
> On 16/03/18 08:29, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote:
>>> On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
 sdhci has a 10 second timeout to catch devices that stop responding.
 Instead of programming 10 second arbitrary value, calculate the total time
 it would take for the entire transfer to happen and program the timeout
 value accordingly.

 Signed-off-by: Kishon Vijay Abraham I 
 ---
  drivers/mmc/host/sdhci.c | 47 
 ---
  drivers/mmc/host/sdhci.h | 10 ++
  2 files changed, 50 insertions(+), 7 deletions(-)

 diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
 index 1dd117cbeb6e..baab67bfa39b 100644
 --- a/drivers/mmc/host/sdhci.c
 +++ b/drivers/mmc/host/sdhci.c
 @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
return sg_dma_address(host->data->sg);
  }
  
 +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
 +struct mmc_command *cmd,
 +unsigned int target_timeout)
 +{
 +  struct mmc_data *data = cmd->data;
 +  struct mmc_host *mmc = host->mmc;
 +  u64 transfer_time;
 +  struct mmc_ios *ios = >ios;
 +  unsigned char bus_width = 1 << ios->bus_width;
 +  unsigned int blksz;
 +  unsigned int freq;
 +
 +  if (data) {
 +  blksz = data->blksz;
 +  freq = host->mmc->actual_clock ? : host->clock;
 +  transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
 +  do_div(transfer_time, freq);
 +  /* multiply by '2' to account for any unknowns */
 +  transfer_time = transfer_time * 2;
 +  /* calculate timeout for the entire data */
 +  host->data_timeout = (data->blocks * ((target_timeout *
 + NSEC_PER_USEC) +
 + transfer_time));
>>>
>>> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
>>> for timeouts greater than about 4 seconds.
>>>
 +  } else {
 +  host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
 +  }
 +
 +  host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>>
>>> Need to allow for target_timeout == 0 so:
>>>
>>> if (host->data_timeout)
>>> host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>>
 +}
 +
  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command 
 *cmd)
  {
u8 count;
 @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
 struct mmc_command *cmd)
if (count >= 0xF)
break;
}
 +  sdhci_calc_sw_timeout(host, cmd, target_timeout);
>>>
>>> If you make the changes I suggest for patch 6, then this would
>>> move sdhci_calc_sw_timeout() into sdhci_set_timeout().
>>>
>>> I suggest you factor out the target_timeout calculation e.g.
>>>
>>> static unsigned int sdhci_target_timeout(struct sdhci_host *host,
>>>  struct mmc_command *cmd,
>>>  struct mmc_data *data)
>>> {
>>> unsigned int target_timeout;
>>>
>>> /* timeout in us */
>>> if (!data)
>>> target_timeout = cmd->busy_timeout * 1000;
>>> else {
>>> target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
>>> if (host->clock && data->timeout_clks) {
>>> unsigned long long val;
>>>
>>> /*
>>>  * data->timeout_clks is in units of clock cycles.
>>>  * host->clock is in Hz.  target_timeout is in us.
>>>  * Hence, us = 100 * cycles / Hz.  Round up.
>>>  */
>>> val = 100ULL * data->timeout_clks;
>>> if (do_div(val, host->clock))
>>> target_timeout++;
>>> target_timeout += val;
>>> }
>>> }
>>>
>>> return target_timeout;
>>> }
>>>
>>> And call it from sdhci_calc_sw_timeout()
>>>
  
return count;
  }
 @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, 
 struct mmc_command *cmd)
mdelay(1);
}
  
 -  timeout = jiffies;
 -  if (!cmd->data && cmd->busy_timeout > 9000)
 -  timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
 -  else
 -  timeout += 10 * HZ;
 -  sdhci_mod_timer(host, cmd->mrq, timeout);
 -
host->cmd = cmd;
if (sdhci_data_line_cmd(cmd)) {
WARN_ON(host->data_cmd);
 @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-19 Thread Kishon Vijay Abraham I 
Hi Adrian,

On Friday 16 March 2018 07:51 PM, Adrian Hunter wrote:
> On 16/03/18 08:29, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote:
>>> On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
 sdhci has a 10 second timeout to catch devices that stop responding.
 Instead of programming 10 second arbitrary value, calculate the total time
 it would take for the entire transfer to happen and program the timeout
 value accordingly.

 Signed-off-by: Kishon Vijay Abraham I 
 ---
  drivers/mmc/host/sdhci.c | 47 
 ---
  drivers/mmc/host/sdhci.h | 10 ++
  2 files changed, 50 insertions(+), 7 deletions(-)

 diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
 index 1dd117cbeb6e..baab67bfa39b 100644
 --- a/drivers/mmc/host/sdhci.c
 +++ b/drivers/mmc/host/sdhci.c
 @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
return sg_dma_address(host->data->sg);
  }
  
 +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
 +struct mmc_command *cmd,
 +unsigned int target_timeout)
 +{
 +  struct mmc_data *data = cmd->data;
 +  struct mmc_host *mmc = host->mmc;
 +  u64 transfer_time;
 +  struct mmc_ios *ios = >ios;
 +  unsigned char bus_width = 1 << ios->bus_width;
 +  unsigned int blksz;
 +  unsigned int freq;
 +
 +  if (data) {
 +  blksz = data->blksz;
 +  freq = host->mmc->actual_clock ? : host->clock;
 +  transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
 +  do_div(transfer_time, freq);
 +  /* multiply by '2' to account for any unknowns */
 +  transfer_time = transfer_time * 2;
 +  /* calculate timeout for the entire data */
 +  host->data_timeout = (data->blocks * ((target_timeout *
 + NSEC_PER_USEC) +
 + transfer_time));
>>>
>>> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
>>> for timeouts greater than about 4 seconds.
>>>
 +  } else {
 +  host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
 +  }
 +
 +  host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>>
>>> Need to allow for target_timeout == 0 so:
>>>
>>> if (host->data_timeout)
>>> host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>>
 +}
 +
  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command 
 *cmd)
  {
u8 count;
 @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
 struct mmc_command *cmd)
if (count >= 0xF)
break;
}
 +  sdhci_calc_sw_timeout(host, cmd, target_timeout);
>>>
>>> If you make the changes I suggest for patch 6, then this would
>>> move sdhci_calc_sw_timeout() into sdhci_set_timeout().
>>>
>>> I suggest you factor out the target_timeout calculation e.g.
>>>
>>> static unsigned int sdhci_target_timeout(struct sdhci_host *host,
>>>  struct mmc_command *cmd,
>>>  struct mmc_data *data)
>>> {
>>> unsigned int target_timeout;
>>>
>>> /* timeout in us */
>>> if (!data)
>>> target_timeout = cmd->busy_timeout * 1000;
>>> else {
>>> target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
>>> if (host->clock && data->timeout_clks) {
>>> unsigned long long val;
>>>
>>> /*
>>>  * data->timeout_clks is in units of clock cycles.
>>>  * host->clock is in Hz.  target_timeout is in us.
>>>  * Hence, us = 100 * cycles / Hz.  Round up.
>>>  */
>>> val = 100ULL * data->timeout_clks;
>>> if (do_div(val, host->clock))
>>> target_timeout++;
>>> target_timeout += val;
>>> }
>>> }
>>>
>>> return target_timeout;
>>> }
>>>
>>> And call it from sdhci_calc_sw_timeout()
>>>
  
return count;
  }
 @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, 
 struct mmc_command *cmd)
mdelay(1);
}
  
 -  timeout = jiffies;
 -  if (!cmd->data && cmd->busy_timeout > 9000)
 -  timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
 -  else
 -  timeout += 10 * HZ;
 -  sdhci_mod_timer(host, cmd->mrq, timeout);
 -
host->cmd = cmd;
if (sdhci_data_line_cmd(cmd)) {
WARN_ON(host->data_cmd);
 @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-16 Thread Adrian Hunter 
On 16/03/18 08:29, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote:
>> On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
>>> sdhci has a 10 second timeout to catch devices that stop responding.
>>> Instead of programming 10 second arbitrary value, calculate the total time
>>> it would take for the entire transfer to happen and program the timeout
>>> value accordingly.
>>>
>>> Signed-off-by: Kishon Vijay Abraham I 
>>> ---
>>>  drivers/mmc/host/sdhci.c | 47 
>>> ---
>>>  drivers/mmc/host/sdhci.h | 10 ++
>>>  2 files changed, 50 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 1dd117cbeb6e..baab67bfa39b 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
>>> return sg_dma_address(host->data->sg);
>>>  }
>>>  
>>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
>>> + struct mmc_command *cmd,
>>> + unsigned int target_timeout)
>>> +{
>>> +   struct mmc_data *data = cmd->data;
>>> +   struct mmc_host *mmc = host->mmc;
>>> +   u64 transfer_time;
>>> +   struct mmc_ios *ios = >ios;
>>> +   unsigned char bus_width = 1 << ios->bus_width;
>>> +   unsigned int blksz;
>>> +   unsigned int freq;
>>> +
>>> +   if (data) {
>>> +   blksz = data->blksz;
>>> +   freq = host->mmc->actual_clock ? : host->clock;
>>> +   transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
>>> +   do_div(transfer_time, freq);
>>> +   /* multiply by '2' to account for any unknowns */
>>> +   transfer_time = transfer_time * 2;
>>> +   /* calculate timeout for the entire data */
>>> +   host->data_timeout = (data->blocks * ((target_timeout *
>>> +  NSEC_PER_USEC) +
>>> +  transfer_time));
>>
>> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
>> for timeouts greater than about 4 seconds.
>>
>>> +   } else {
>>> +   host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
>>> +   }
>>> +
>>> +   host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>
>> Need to allow for target_timeout == 0 so:
>>
>>  if (host->data_timeout)
>>  host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>
>>> +}
>>> +
>>>  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command 
>>> *cmd)
>>>  {
>>> u8 count;
>>> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
>>> struct mmc_command *cmd)
>>> if (count >= 0xF)
>>> break;
>>> }
>>> +   sdhci_calc_sw_timeout(host, cmd, target_timeout);
>>
>> If you make the changes I suggest for patch 6, then this would
>> move sdhci_calc_sw_timeout() into sdhci_set_timeout().
>>
>> I suggest you factor out the target_timeout calculation e.g.
>>
>> static unsigned int sdhci_target_timeout(struct sdhci_host *host,
>>   struct mmc_command *cmd,
>>   struct mmc_data *data)
>> {
>>  unsigned int target_timeout;
>>
>>  /* timeout in us */
>>  if (!data)
>>  target_timeout = cmd->busy_timeout * 1000;
>>  else {
>>  target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
>>  if (host->clock && data->timeout_clks) {
>>  unsigned long long val;
>>
>>  /*
>>   * data->timeout_clks is in units of clock cycles.
>>   * host->clock is in Hz.  target_timeout is in us.
>>   * Hence, us = 100 * cycles / Hz.  Round up.
>>   */
>>  val = 100ULL * data->timeout_clks;
>>  if (do_div(val, host->clock))
>>  target_timeout++;
>>  target_timeout += val;
>>  }
>>  }
>>
>>  return target_timeout;
>> }
>>
>> And call it from sdhci_calc_sw_timeout()
>>
>>>  
>>> return count;
>>>  }
>>> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, 
>>> struct mmc_command *cmd)
>>> mdelay(1);
>>> }
>>>  
>>> -   timeout = jiffies;
>>> -   if (!cmd->data && cmd->busy_timeout > 9000)
>>> -   timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
>>> -   else
>>> -   timeout += 10 * HZ;
>>> -   sdhci_mod_timer(host, cmd->mrq, timeout);
>>> -
>>> host->cmd = cmd;
>>> if (sdhci_data_line_cmd(cmd)) {
>>> WARN_ON(host->data_cmd);
>>> @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, 
>>> struct mmc_command *cmd)
>>> cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
>>> flags |=

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-16 Thread Adrian Hunter 
On 16/03/18 08:29, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote:
>> On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
>>> sdhci has a 10 second timeout to catch devices that stop responding.
>>> Instead of programming 10 second arbitrary value, calculate the total time
>>> it would take for the entire transfer to happen and program the timeout
>>> value accordingly.
>>>
>>> Signed-off-by: Kishon Vijay Abraham I 
>>> ---
>>>  drivers/mmc/host/sdhci.c | 47 
>>> ---
>>>  drivers/mmc/host/sdhci.h | 10 ++
>>>  2 files changed, 50 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 1dd117cbeb6e..baab67bfa39b 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
>>> return sg_dma_address(host->data->sg);
>>>  }
>>>  
>>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
>>> + struct mmc_command *cmd,
>>> + unsigned int target_timeout)
>>> +{
>>> +   struct mmc_data *data = cmd->data;
>>> +   struct mmc_host *mmc = host->mmc;
>>> +   u64 transfer_time;
>>> +   struct mmc_ios *ios = >ios;
>>> +   unsigned char bus_width = 1 << ios->bus_width;
>>> +   unsigned int blksz;
>>> +   unsigned int freq;
>>> +
>>> +   if (data) {
>>> +   blksz = data->blksz;
>>> +   freq = host->mmc->actual_clock ? : host->clock;
>>> +   transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
>>> +   do_div(transfer_time, freq);
>>> +   /* multiply by '2' to account for any unknowns */
>>> +   transfer_time = transfer_time * 2;
>>> +   /* calculate timeout for the entire data */
>>> +   host->data_timeout = (data->blocks * ((target_timeout *
>>> +  NSEC_PER_USEC) +
>>> +  transfer_time));
>>
>> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
>> for timeouts greater than about 4 seconds.
>>
>>> +   } else {
>>> +   host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
>>> +   }
>>> +
>>> +   host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>
>> Need to allow for target_timeout == 0 so:
>>
>>  if (host->data_timeout)
>>  host->data_timeout += MMC_CMD_TRANSFER_TIME;
>>
>>> +}
>>> +
>>>  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command 
>>> *cmd)
>>>  {
>>> u8 count;
>>> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
>>> struct mmc_command *cmd)
>>> if (count >= 0xF)
>>> break;
>>> }
>>> +   sdhci_calc_sw_timeout(host, cmd, target_timeout);
>>
>> If you make the changes I suggest for patch 6, then this would
>> move sdhci_calc_sw_timeout() into sdhci_set_timeout().
>>
>> I suggest you factor out the target_timeout calculation e.g.
>>
>> static unsigned int sdhci_target_timeout(struct sdhci_host *host,
>>   struct mmc_command *cmd,
>>   struct mmc_data *data)
>> {
>>  unsigned int target_timeout;
>>
>>  /* timeout in us */
>>  if (!data)
>>  target_timeout = cmd->busy_timeout * 1000;
>>  else {
>>  target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
>>  if (host->clock && data->timeout_clks) {
>>  unsigned long long val;
>>
>>  /*
>>   * data->timeout_clks is in units of clock cycles.
>>   * host->clock is in Hz.  target_timeout is in us.
>>   * Hence, us = 100 * cycles / Hz.  Round up.
>>   */
>>  val = 100ULL * data->timeout_clks;
>>  if (do_div(val, host->clock))
>>  target_timeout++;
>>  target_timeout += val;
>>  }
>>  }
>>
>>  return target_timeout;
>> }
>>
>> And call it from sdhci_calc_sw_timeout()
>>
>>>  
>>> return count;
>>>  }
>>> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, 
>>> struct mmc_command *cmd)
>>> mdelay(1);
>>> }
>>>  
>>> -   timeout = jiffies;
>>> -   if (!cmd->data && cmd->busy_timeout > 9000)
>>> -   timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
>>> -   else
>>> -   timeout += 10 * HZ;
>>> -   sdhci_mod_timer(host, cmd->mrq, timeout);
>>> -
>>> host->cmd = cmd;
>>> if (sdhci_data_line_cmd(cmd)) {
>>> WARN_ON(host->data_cmd);
>>> @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, 
>>> struct mmc_command *cmd)
>>> cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
>>> flags |= SDHCI_CMD_DATA;

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-16 Thread Kishon Vijay Abraham I 
Hi,

On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote:
> On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
>> sdhci has a 10 second timeout to catch devices that stop responding.
>> Instead of programming 10 second arbitrary value, calculate the total time
>> it would take for the entire transfer to happen and program the timeout
>> value accordingly.
>>
>> Signed-off-by: Kishon Vijay Abraham I 
>> ---
>>  drivers/mmc/host/sdhci.c | 47 
>> ---
>>  drivers/mmc/host/sdhci.h | 10 ++
>>  2 files changed, 50 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 1dd117cbeb6e..baab67bfa39b 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
>>  return sg_dma_address(host->data->sg);
>>  }
>>  
>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
>> +  struct mmc_command *cmd,
>> +  unsigned int target_timeout)
>> +{
>> +struct mmc_data *data = cmd->data;
>> +struct mmc_host *mmc = host->mmc;
>> +u64 transfer_time;
>> +struct mmc_ios *ios = >ios;
>> +unsigned char bus_width = 1 << ios->bus_width;
>> +unsigned int blksz;
>> +unsigned int freq;
>> +
>> +if (data) {
>> +blksz = data->blksz;
>> +freq = host->mmc->actual_clock ? : host->clock;
>> +transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
>> +do_div(transfer_time, freq);
>> +/* multiply by '2' to account for any unknowns */
>> +transfer_time = transfer_time * 2;
>> +/* calculate timeout for the entire data */
>> +host->data_timeout = (data->blocks * ((target_timeout *
>> +   NSEC_PER_USEC) +
>> +   transfer_time));
> 
> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
> for timeouts greater than about 4 seconds.
> 
>> +} else {
>> +host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
>> +}
>> +
>> +host->data_timeout += MMC_CMD_TRANSFER_TIME;
> 
> Need to allow for target_timeout == 0 so:
> 
>   if (host->data_timeout)
>   host->data_timeout += MMC_CMD_TRANSFER_TIME;
> 
>> +}
>> +
>>  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command 
>> *cmd)
>>  {
>>  u8 count;
>> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
>> struct mmc_command *cmd)
>>  if (count >= 0xF)
>>  break;
>>  }
>> +sdhci_calc_sw_timeout(host, cmd, target_timeout);
> 
> If you make the changes I suggest for patch 6, then this would
> move sdhci_calc_sw_timeout() into sdhci_set_timeout().
> 
> I suggest you factor out the target_timeout calculation e.g.
> 
> static unsigned int sdhci_target_timeout(struct sdhci_host *host,
>struct mmc_command *cmd,
>struct mmc_data *data)
> {
>   unsigned int target_timeout;
> 
>   /* timeout in us */
>   if (!data)
>   target_timeout = cmd->busy_timeout * 1000;
>   else {
>   target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
>   if (host->clock && data->timeout_clks) {
>   unsigned long long val;
> 
>   /*
>* data->timeout_clks is in units of clock cycles.
>* host->clock is in Hz.  target_timeout is in us.
>* Hence, us = 100 * cycles / Hz.  Round up.
>*/
>   val = 100ULL * data->timeout_clks;
>   if (do_div(val, host->clock))
>   target_timeout++;
>   target_timeout += val;
>   }
>   }
> 
>   return target_timeout;
> }
> 
> And call it from sdhci_calc_sw_timeout()
> 
>>  
>>  return count;
>>  }
>> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, 
>> struct mmc_command *cmd)
>>  mdelay(1);
>>  }
>>  
>> -timeout = jiffies;
>> -if (!cmd->data && cmd->busy_timeout > 9000)
>> -timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
>> -else
>> -timeout += 10 * HZ;
>> -sdhci_mod_timer(host, cmd->mrq, timeout);
>> -
>>  host->cmd = cmd;
>>  if (sdhci_data_line_cmd(cmd)) {
>>  WARN_ON(host->data_cmd);
>> @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, 
>> struct mmc_command *cmd)
>>  cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
>>  flags |= SDHCI_CMD_DATA;
>>  
>> +timeout = jiffies;
>> +if (host->data_timeout > 0) {
> 
> This can be

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-16 Thread Kishon Vijay Abraham I 
Hi,

On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote:
> On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
>> sdhci has a 10 second timeout to catch devices that stop responding.
>> Instead of programming 10 second arbitrary value, calculate the total time
>> it would take for the entire transfer to happen and program the timeout
>> value accordingly.
>>
>> Signed-off-by: Kishon Vijay Abraham I 
>> ---
>>  drivers/mmc/host/sdhci.c | 47 
>> ---
>>  drivers/mmc/host/sdhci.h | 10 ++
>>  2 files changed, 50 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 1dd117cbeb6e..baab67bfa39b 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
>>  return sg_dma_address(host->data->sg);
>>  }
>>  
>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
>> +  struct mmc_command *cmd,
>> +  unsigned int target_timeout)
>> +{
>> +struct mmc_data *data = cmd->data;
>> +struct mmc_host *mmc = host->mmc;
>> +u64 transfer_time;
>> +struct mmc_ios *ios = >ios;
>> +unsigned char bus_width = 1 << ios->bus_width;
>> +unsigned int blksz;
>> +unsigned int freq;
>> +
>> +if (data) {
>> +blksz = data->blksz;
>> +freq = host->mmc->actual_clock ? : host->clock;
>> +transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
>> +do_div(transfer_time, freq);
>> +/* multiply by '2' to account for any unknowns */
>> +transfer_time = transfer_time * 2;
>> +/* calculate timeout for the entire data */
>> +host->data_timeout = (data->blocks * ((target_timeout *
>> +   NSEC_PER_USEC) +
>> +   transfer_time));
> 
> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
> for timeouts greater than about 4 seconds.
> 
>> +} else {
>> +host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
>> +}
>> +
>> +host->data_timeout += MMC_CMD_TRANSFER_TIME;
> 
> Need to allow for target_timeout == 0 so:
> 
>   if (host->data_timeout)
>   host->data_timeout += MMC_CMD_TRANSFER_TIME;
> 
>> +}
>> +
>>  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command 
>> *cmd)
>>  {
>>  u8 count;
>> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
>> struct mmc_command *cmd)
>>  if (count >= 0xF)
>>  break;
>>  }
>> +sdhci_calc_sw_timeout(host, cmd, target_timeout);
> 
> If you make the changes I suggest for patch 6, then this would
> move sdhci_calc_sw_timeout() into sdhci_set_timeout().
> 
> I suggest you factor out the target_timeout calculation e.g.
> 
> static unsigned int sdhci_target_timeout(struct sdhci_host *host,
>struct mmc_command *cmd,
>struct mmc_data *data)
> {
>   unsigned int target_timeout;
> 
>   /* timeout in us */
>   if (!data)
>   target_timeout = cmd->busy_timeout * 1000;
>   else {
>   target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
>   if (host->clock && data->timeout_clks) {
>   unsigned long long val;
> 
>   /*
>* data->timeout_clks is in units of clock cycles.
>* host->clock is in Hz.  target_timeout is in us.
>* Hence, us = 100 * cycles / Hz.  Round up.
>*/
>   val = 100ULL * data->timeout_clks;
>   if (do_div(val, host->clock))
>   target_timeout++;
>   target_timeout += val;
>   }
>   }
> 
>   return target_timeout;
> }
> 
> And call it from sdhci_calc_sw_timeout()
> 
>>  
>>  return count;
>>  }
>> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, 
>> struct mmc_command *cmd)
>>  mdelay(1);
>>  }
>>  
>> -timeout = jiffies;
>> -if (!cmd->data && cmd->busy_timeout > 9000)
>> -timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
>> -else
>> -timeout += 10 * HZ;
>> -sdhci_mod_timer(host, cmd->mrq, timeout);
>> -
>>  host->cmd = cmd;
>>  if (sdhci_data_line_cmd(cmd)) {
>>  WARN_ON(host->data_cmd);
>> @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, 
>> struct mmc_command *cmd)
>>  cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
>>  flags |= SDHCI_CMD_DATA;
>>  
>> +timeout = jiffies;
>> +if (host->data_timeout > 0) {
> 
> This can be just:
> 
>

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-15 Thread Adrian Hunter 
On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
> sdhci has a 10 second timeout to catch devices that stop responding.
> Instead of programming 10 second arbitrary value, calculate the total time
> it would take for the entire transfer to happen and program the timeout
> value accordingly.
> 
> Signed-off-by: Kishon Vijay Abraham I 
> ---
>  drivers/mmc/host/sdhci.c | 47 ---
>  drivers/mmc/host/sdhci.h | 10 ++
>  2 files changed, 50 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1dd117cbeb6e..baab67bfa39b 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
>   return sg_dma_address(host->data->sg);
>  }
>  
> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
> +   struct mmc_command *cmd,
> +   unsigned int target_timeout)
> +{
> + struct mmc_data *data = cmd->data;
> + struct mmc_host *mmc = host->mmc;
> + u64 transfer_time;
> + struct mmc_ios *ios = >ios;
> + unsigned char bus_width = 1 << ios->bus_width;
> + unsigned int blksz;
> + unsigned int freq;
> +
> + if (data) {
> + blksz = data->blksz;
> + freq = host->mmc->actual_clock ? : host->clock;
> + transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
> + do_div(transfer_time, freq);
> + /* multiply by '2' to account for any unknowns */
> + transfer_time = transfer_time * 2;
> + /* calculate timeout for the entire data */
> + host->data_timeout = (data->blocks * ((target_timeout *
> +NSEC_PER_USEC) +
> +transfer_time));

(target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
for timeouts greater than about 4 seconds.

> + } else {
> + host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
> + }
> +
> + host->data_timeout += MMC_CMD_TRANSFER_TIME;

Need to allow for target_timeout == 0 so:

if (host->data_timeout)
host->data_timeout += MMC_CMD_TRANSFER_TIME;

> +}
> +
>  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command 
> *cmd)
>  {
>   u8 count;
> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
> struct mmc_command *cmd)
>   if (count >= 0xF)
>   break;
>   }
> + sdhci_calc_sw_timeout(host, cmd, target_timeout);

If you make the changes I suggest for patch 6, then this would
move sdhci_calc_sw_timeout() into sdhci_set_timeout().

I suggest you factor out the target_timeout calculation e.g.

static unsigned int sdhci_target_timeout(struct sdhci_host *host,
 struct mmc_command *cmd,
 struct mmc_data *data)
{
unsigned int target_timeout;

/* timeout in us */
if (!data)
target_timeout = cmd->busy_timeout * 1000;
else {
target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
if (host->clock && data->timeout_clks) {
unsigned long long val;

/*
 * data->timeout_clks is in units of clock cycles.
 * host->clock is in Hz.  target_timeout is in us.
 * Hence, us = 100 * cycles / Hz.  Round up.
 */
val = 100ULL * data->timeout_clks;
if (do_div(val, host->clock))
target_timeout++;
target_timeout += val;
}
}

return target_timeout;
}

And call it from sdhci_calc_sw_timeout()

>  
>   return count;
>  }
> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, 
> struct mmc_command *cmd)
>   mdelay(1);
>   }
>  
> - timeout = jiffies;
> - if (!cmd->data && cmd->busy_timeout > 9000)
> - timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
> - else
> - timeout += 10 * HZ;
> - sdhci_mod_timer(host, cmd->mrq, timeout);
> -
>   host->cmd = cmd;
>   if (sdhci_data_line_cmd(cmd)) {
>   WARN_ON(host->data_cmd);
> @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, 
> struct mmc_command *cmd)
>   cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
>   flags |= SDHCI_CMD_DATA;
>  
> + timeout = jiffies;
> + if (host->data_timeout > 0) {

This can be just:

if (host->data_timeout) {

> + timeout += nsecs_to_jiffies(host->data_timeout);
> + host->data_timeout = 0;

It would

Re: [PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-15 Thread Adrian Hunter 
On 07/03/18 15:20, Kishon Vijay Abraham I wrote:
> sdhci has a 10 second timeout to catch devices that stop responding.
> Instead of programming 10 second arbitrary value, calculate the total time
> it would take for the entire transfer to happen and program the timeout
> value accordingly.
> 
> Signed-off-by: Kishon Vijay Abraham I 
> ---
>  drivers/mmc/host/sdhci.c | 47 ---
>  drivers/mmc/host/sdhci.h | 10 ++
>  2 files changed, 50 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1dd117cbeb6e..baab67bfa39b 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
>   return sg_dma_address(host->data->sg);
>  }
>  
> +static void sdhci_calc_sw_timeout(struct sdhci_host *host,
> +   struct mmc_command *cmd,
> +   unsigned int target_timeout)
> +{
> + struct mmc_data *data = cmd->data;
> + struct mmc_host *mmc = host->mmc;
> + u64 transfer_time;
> + struct mmc_ios *ios = >ios;
> + unsigned char bus_width = 1 << ios->bus_width;
> + unsigned int blksz;
> + unsigned int freq;
> +
> + if (data) {
> + blksz = data->blksz;
> + freq = host->mmc->actual_clock ? : host->clock;
> + transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
> + do_div(transfer_time, freq);
> + /* multiply by '2' to account for any unknowns */
> + transfer_time = transfer_time * 2;
> + /* calculate timeout for the entire data */
> + host->data_timeout = (data->blocks * ((target_timeout *
> +NSEC_PER_USEC) +
> +transfer_time));

(target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow
for timeouts greater than about 4 seconds.

> + } else {
> + host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
> + }
> +
> + host->data_timeout += MMC_CMD_TRANSFER_TIME;

Need to allow for target_timeout == 0 so:

if (host->data_timeout)
host->data_timeout += MMC_CMD_TRANSFER_TIME;

> +}
> +
>  static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command 
> *cmd)
>  {
>   u8 count;
> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
> struct mmc_command *cmd)
>   if (count >= 0xF)
>   break;
>   }
> + sdhci_calc_sw_timeout(host, cmd, target_timeout);

If you make the changes I suggest for patch 6, then this would
move sdhci_calc_sw_timeout() into sdhci_set_timeout().

I suggest you factor out the target_timeout calculation e.g.

static unsigned int sdhci_target_timeout(struct sdhci_host *host,
 struct mmc_command *cmd,
 struct mmc_data *data)
{
unsigned int target_timeout;

/* timeout in us */
if (!data)
target_timeout = cmd->busy_timeout * 1000;
else {
target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
if (host->clock && data->timeout_clks) {
unsigned long long val;

/*
 * data->timeout_clks is in units of clock cycles.
 * host->clock is in Hz.  target_timeout is in us.
 * Hence, us = 100 * cycles / Hz.  Round up.
 */
val = 100ULL * data->timeout_clks;
if (do_div(val, host->clock))
target_timeout++;
target_timeout += val;
}
}

return target_timeout;
}

And call it from sdhci_calc_sw_timeout()

>  
>   return count;
>  }
> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, 
> struct mmc_command *cmd)
>   mdelay(1);
>   }
>  
> - timeout = jiffies;
> - if (!cmd->data && cmd->busy_timeout > 9000)
> - timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
> - else
> - timeout += 10 * HZ;
> - sdhci_mod_timer(host, cmd->mrq, timeout);
> -
>   host->cmd = cmd;
>   if (sdhci_data_line_cmd(cmd)) {
>   WARN_ON(host->data_cmd);
> @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, 
> struct mmc_command *cmd)
>   cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
>   flags |= SDHCI_CMD_DATA;
>  
> + timeout = jiffies;
> + if (host->data_timeout > 0) {

This can be just:

if (host->data_timeout) {

> + timeout += nsecs_to_jiffies(host->data_timeout);
> + host->data_timeout = 0;

It would be better to

[PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-07 Thread Kishon Vijay Abraham I 
sdhci has a 10 second timeout to catch devices that stop responding.
Instead of programming 10 second arbitrary value, calculate the total time
it would take for the entire transfer to happen and program the timeout
value accordingly.

Signed-off-by: Kishon Vijay Abraham I 
---
 drivers/mmc/host/sdhci.c | 47 ---
 drivers/mmc/host/sdhci.h | 10 ++
 2 files changed, 50 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1dd117cbeb6e..baab67bfa39b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
return sg_dma_address(host->data->sg);
 }
 
+static void sdhci_calc_sw_timeout(struct sdhci_host *host,
+ struct mmc_command *cmd,
+ unsigned int target_timeout)
+{
+   struct mmc_data *data = cmd->data;
+   struct mmc_host *mmc = host->mmc;
+   u64 transfer_time;
+   struct mmc_ios *ios = >ios;
+   unsigned char bus_width = 1 << ios->bus_width;
+   unsigned int blksz;
+   unsigned int freq;
+
+   if (data) {
+   blksz = data->blksz;
+   freq = host->mmc->actual_clock ? : host->clock;
+   transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
+   do_div(transfer_time, freq);
+   /* multiply by '2' to account for any unknowns */
+   transfer_time = transfer_time * 2;
+   /* calculate timeout for the entire data */
+   host->data_timeout = (data->blocks * ((target_timeout *
+  NSEC_PER_USEC) +
+  transfer_time));
+   } else {
+   host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
+   }
+
+   host->data_timeout += MMC_CMD_TRANSFER_TIME;
+}
+
 static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 {
u8 count;
@@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
struct mmc_command *cmd)
if (count >= 0xF)
break;
}
+   sdhci_calc_sw_timeout(host, cmd, target_timeout);
 
return count;
 }
@@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct 
mmc_command *cmd)
mdelay(1);
}
 
-   timeout = jiffies;
-   if (!cmd->data && cmd->busy_timeout > 9000)
-   timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
-   else
-   timeout += 10 * HZ;
-   sdhci_mod_timer(host, cmd->mrq, timeout);
-
host->cmd = cmd;
if (sdhci_data_line_cmd(cmd)) {
WARN_ON(host->data_cmd);
@@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct 
mmc_command *cmd)
cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
flags |= SDHCI_CMD_DATA;
 
+   timeout = jiffies;
+   if (host->data_timeout > 0) {
+   timeout += nsecs_to_jiffies(host->data_timeout);
+   host->data_timeout = 0;
+   } else {
+   timeout += 10 * HZ;
+   }
+   sdhci_mod_timer(host, cmd->mrq, timeout);
+
sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
 }
 EXPORT_SYMBOL_GPL(sdhci_send_command);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index ff283ee08854..29b242fd17de 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -332,6 +332,14 @@ struct sdhci_adma2_64_desc {
 /* Allow for a a command request and a data request at the same time */
 #define SDHCI_MAX_MRQS 2
 
+/*
+ * 48bit command and 136 bit response in 100KHz clock could take upto 2.48ms.
+ * However since the start time of the command, the time between
+ * command and response, and the time between response and start of data is
+ * not known, set the command transfer time to 10ms.
+ */
+#define MMC_CMD_TRANSFER_TIME  (10 * NSEC_PER_MSEC) /* max 10 ms */
+
 enum sdhci_cookie {
COOKIE_UNMAPPED,
COOKIE_PRE_MAPPED,  /* mapped by sdhci_pre_req() */
@@ -555,6 +563,8 @@ struct sdhci_host {
/* Host SDMA buffer boundary. */
u32 sdma_boundary;
 
+   u64 data_timeout;
+
unsigned long private[0] cacheline_aligned;
 };
 
-- 
2.11.0

[PATCH v3 07/11] mmc: sdhci: Program a relatively accurate SW timeout value

2018-03-07 Thread Kishon Vijay Abraham I 
sdhci has a 10 second timeout to catch devices that stop responding.
Instead of programming 10 second arbitrary value, calculate the total time
it would take for the entire transfer to happen and program the timeout
value accordingly.

Signed-off-by: Kishon Vijay Abraham I 
---
 drivers/mmc/host/sdhci.c | 47 ---
 drivers/mmc/host/sdhci.h | 10 ++
 2 files changed, 50 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1dd117cbeb6e..baab67bfa39b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host)
return sg_dma_address(host->data->sg);
 }
 
+static void sdhci_calc_sw_timeout(struct sdhci_host *host,
+ struct mmc_command *cmd,
+ unsigned int target_timeout)
+{
+   struct mmc_data *data = cmd->data;
+   struct mmc_host *mmc = host->mmc;
+   u64 transfer_time;
+   struct mmc_ios *ios = >ios;
+   unsigned char bus_width = 1 << ios->bus_width;
+   unsigned int blksz;
+   unsigned int freq;
+
+   if (data) {
+   blksz = data->blksz;
+   freq = host->mmc->actual_clock ? : host->clock;
+   transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width);
+   do_div(transfer_time, freq);
+   /* multiply by '2' to account for any unknowns */
+   transfer_time = transfer_time * 2;
+   /* calculate timeout for the entire data */
+   host->data_timeout = (data->blocks * ((target_timeout *
+  NSEC_PER_USEC) +
+  transfer_time));
+   } else {
+   host->data_timeout = (u64)target_timeout * NSEC_PER_USEC;
+   }
+
+   host->data_timeout += MMC_CMD_TRANSFER_TIME;
+}
+
 static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 {
u8 count;
@@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
struct mmc_command *cmd)
if (count >= 0xF)
break;
}
+   sdhci_calc_sw_timeout(host, cmd, target_timeout);
 
return count;
 }
@@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct 
mmc_command *cmd)
mdelay(1);
}
 
-   timeout = jiffies;
-   if (!cmd->data && cmd->busy_timeout > 9000)
-   timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ;
-   else
-   timeout += 10 * HZ;
-   sdhci_mod_timer(host, cmd->mrq, timeout);
-
host->cmd = cmd;
if (sdhci_data_line_cmd(cmd)) {
WARN_ON(host->data_cmd);
@@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct 
mmc_command *cmd)
cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
flags |= SDHCI_CMD_DATA;
 
+   timeout = jiffies;
+   if (host->data_timeout > 0) {
+   timeout += nsecs_to_jiffies(host->data_timeout);
+   host->data_timeout = 0;
+   } else {
+   timeout += 10 * HZ;
+   }
+   sdhci_mod_timer(host, cmd->mrq, timeout);
+
sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
 }
 EXPORT_SYMBOL_GPL(sdhci_send_command);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index ff283ee08854..29b242fd17de 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -332,6 +332,14 @@ struct sdhci_adma2_64_desc {
 /* Allow for a a command request and a data request at the same time */
 #define SDHCI_MAX_MRQS 2
 
+/*
+ * 48bit command and 136 bit response in 100KHz clock could take upto 2.48ms.
+ * However since the start time of the command, the time between
+ * command and response, and the time between response and start of data is
+ * not known, set the command transfer time to 10ms.
+ */
+#define MMC_CMD_TRANSFER_TIME  (10 * NSEC_PER_MSEC) /* max 10 ms */
+
 enum sdhci_cookie {
COOKIE_UNMAPPED,
COOKIE_PRE_MAPPED,  /* mapped by sdhci_pre_req() */
@@ -555,6 +563,8 @@ struct sdhci_host {
/* Host SDMA buffer boundary. */
u32 sdma_boundary;
 
+   u64 data_timeout;
+
unsigned long private[0] cacheline_aligned;
 };
 
-- 
2.11.0