[PATCH 0/3] mmc: host: omap_hsmmc: miscellaneous fixes

[PATCH 0/3] mmc: host: omap_hsmmc: miscellaneous fixes

[Ravikumar Kattekola]

A few issues have been identified during mmc testing and following
set of patches fixes a couple of them related to
*timeout handling and ceb error handling*

Tested on DRA75X PG 2.0 Rev H EVM

Kishon Vijay Abraham I (1):
  mmc: host: omap_hsmmc: use generic_cmd6_time to program timeout value
    for CMD6

Ravikumar Kattekola (2):
  mmc: host: omap_hsmmc: reset cmd line on ceb error
  mmc: host: omap_hsmmc: avoid possible overflow of timeout value

 drivers/mmc/host/omap_hsmmc.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH 1/3] mmc: host: omap_hsmmc: reset cmd line on ceb error

[Ravikumar Kattekola]

When CEB (command end bit error) occurs
reset CMD line to avoid system ending up in
erroneous state.
While command line is reset for CTO and CCRC errors,
it's not done for CEB error. Fix it here.

Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Ravikumar Kattekola <rk@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 drivers/mmc/host/omap_hsmmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index ad11c4c..0ee5650 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1162,7 +1162,7 @@ static void omap_hsmmc_do_irq(struct omap_hsmmc_host *host, int status)
 	if (status & ERR_EN) {
 		omap_hsmmc_dbg_report_irq(host, status);
 
-		if (status & (CTO_EN | CCRC_EN))
+		if (status & (CTO_EN | CCRC_EN | CEB_EN))
 			end_cmd = 1;
 		if (host->data || host->response_busy) {
 			end_trans = !end_cmd;
-- 
1.9.1

[PATCH 2/3] mmc: host: omap_hsmmc: use generic_cmd6_time to program timeout value for CMD6

[Ravikumar Kattekola]

From: Kishon Vijay Abraham I <kishon@ti.com>

commit e2bf08d643a244ccb ("omap_hsmmc: set a large data timeout for
commands with busy signal") sets an arbitrary timeout value (100ms) for
commands like CMD6 (MMC SWITCH). However extended CSD register defined
in the eMMC standard has a field for GENERIC_CMD6_TIME which indicates
the default maximum timeout for a SWITCH command.
Use busy_timeout of cmd structure (populated with GENERIC_CMD6_TIME
in the case of SWITCH command) to program the data timeout value in
omap_hsmmc driver.
SWITCH command to turn the cache on took more than 100ms to complete
with MICRON eMMC card present in AM572x IDK REV 1.3A resulting in
timeout and failed enumeration. It is fixed here by programming the
timeout with the value advertised in GENERIC_CMD6_TIME.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
Signed-off-by: Ravikumar Kattekola <rk@ti.com>
---
 drivers/mmc/host/omap_hsmmc.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 0ee5650..51ca3d7 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1527,16 +1527,24 @@ static void omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host)
 omap_hsmmc_prepare_data(struct omap_hsmmc_host *host, struct mmc_request *req)
 {
 	int ret;
+	unsigned int timeout;
+
 	host->data = req->data;
 
 	if (req->data == NULL) {
 		OMAP_HSMMC_WRITE(host->base, BLK, 0);
-		/*
-		 * Set an arbitrary 100ms data timeout for commands with
-		 * busy signal.
-		 */
-		if (req->cmd->flags & MMC_RSP_BUSY)
-			set_data_timeout(host, 100000000U, 0);
+		if (req->cmd->flags & MMC_RSP_BUSY) {
+			timeout = req->cmd->busy_timeout * NSEC_PER_MSEC;
+
+			/*
+			 * Set an arbitrary 100ms data timeout for commands with
+			 * busy signal and no indication of busy_timeout.
+			 */
+			if (!timeout)
+				timeout = 100000000U;
+
+			set_data_timeout(host, timeout, 0);
+		}
 		return 0;
 	}
 
-- 
1.9.1

Re: [PATCH 2/3] mmc: host: omap_hsmmc: use generic_cmd6_time to program timeout value for CMD6

[Ulf Hansson]

On 30 January 2017 at 11:11, Ravikumar Kattekola <rk@ti.com> wrote:
> From: Kishon Vijay Abraham I <kishon@ti.com>
>
> commit e2bf08d643a244ccb ("omap_hsmmc: set a large data timeout for
> commands with busy signal") sets an arbitrary timeout value (100ms) for
> commands like CMD6 (MMC SWITCH). However extended CSD register defined
> in the eMMC standard has a field for GENERIC_CMD6_TIME which indicates
> the default maximum timeout for a SWITCH command.
> Use busy_timeout of cmd structure (populated with GENERIC_CMD6_TIME
> in the case of SWITCH command) to program the data timeout value in
> omap_hsmmc driver.
> SWITCH command to turn the cache on took more than 100ms to complete
> with MICRON eMMC card present in AM572x IDK REV 1.3A resulting in
> timeout and failed enumeration. It is fixed here by programming the
> timeout with the value advertised in GENERIC_CMD6_TIME.
>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> Signed-off-by: Ravikumar Kattekola <rk@ti.com>
> ---
>  drivers/mmc/host/omap_hsmmc.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 0ee5650..51ca3d7 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -1527,16 +1527,24 @@ static void omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host)
>  omap_hsmmc_prepare_data(struct omap_hsmmc_host *host, struct mmc_request *req)
>  {
>         int ret;
> +       unsigned int timeout;
> +
>         host->data = req->data;
>
>         if (req->data == NULL) {
>                 OMAP_HSMMC_WRITE(host->base, BLK, 0);
> -               /*
> -                * Set an arbitrary 100ms data timeout for commands with
> -                * busy signal.
> -                */
> -               if (req->cmd->flags & MMC_RSP_BUSY)
> -                       set_data_timeout(host, 100000000U, 0);
> +               if (req->cmd->flags & MMC_RSP_BUSY) {
> +                       timeout = req->cmd->busy_timeout * NSEC_PER_MSEC;
> +
> +                       /*
> +                        * Set an arbitrary 100ms data timeout for commands with
> +                        * busy signal and no indication of busy_timeout.
> +                        */
> +                       if (!timeout)

This is a bug in the mmc core if this ever happen.

Therefore I am particularly interested to find out if this is really
needed or it's just playing safe?

> +                               timeout = 100000000U;
> +
> +                       set_data_timeout(host, timeout, 0);
> +               }
>                 return 0;
>         }
>
> --
> 1.9.1
>

Kind regards
Uffe

Re: [PATCH 2/3] mmc: host: omap_hsmmc: use generic_cmd6_time to program timeout value for CMD6

[Ravikumar]

On Tuesday 31 January 2017 03:42 PM, Ulf Hansson wrote:
> On 30 January 2017 at 11:11, Ravikumar Kattekola <rk@ti.com> wrote:
>> From: Kishon Vijay Abraham I <kishon@ti.com>
>>
>> commit e2bf08d643a244ccb ("omap_hsmmc: set a large data timeout for
>> commands with busy signal") sets an arbitrary timeout value (100ms) for
>> commands like CMD6 (MMC SWITCH). However extended CSD register defined
>> in the eMMC standard has a field for GENERIC_CMD6_TIME which indicates
>> the default maximum timeout for a SWITCH command.
>> Use busy_timeout of cmd structure (populated with GENERIC_CMD6_TIME
>> in the case of SWITCH command) to program the data timeout value in
>> omap_hsmmc driver.
>> SWITCH command to turn the cache on took more than 100ms to complete
>> with MICRON eMMC card present in AM572x IDK REV 1.3A resulting in
>> timeout and failed enumeration. It is fixed here by programming the
>> timeout with the value advertised in GENERIC_CMD6_TIME.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>> Signed-off-by: Ravikumar Kattekola <rk@ti.com>
>> ---
>>   drivers/mmc/host/omap_hsmmc.c | 20 ++++++++++++++------
>>   1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> index 0ee5650..51ca3d7 100644
>> --- a/drivers/mmc/host/omap_hsmmc.c
>> +++ b/drivers/mmc/host/omap_hsmmc.c
>> @@ -1527,16 +1527,24 @@ static void omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host)
>>   omap_hsmmc_prepare_data(struct omap_hsmmc_host *host, struct mmc_request *req)
>>   {
>>          int ret;
>> +       unsigned int timeout;
>> +
>>          host->data = req->data;
>>
>>          if (req->data == NULL) {
>>                  OMAP_HSMMC_WRITE(host->base, BLK, 0);
>> -               /*
>> -                * Set an arbitrary 100ms data timeout for commands with
>> -                * busy signal.
>> -                */
>> -               if (req->cmd->flags & MMC_RSP_BUSY)
>> -                       set_data_timeout(host, 100000000U, 0);
>> +               if (req->cmd->flags & MMC_RSP_BUSY) {
>> +                       timeout = req->cmd->busy_timeout * NSEC_PER_MSEC;
>> +
>> +                       /*
>> +                        * Set an arbitrary 100ms data timeout for commands with
>> +                        * busy signal and no indication of busy_timeout.
>> +                        */
>> +                       if (!timeout)
> This is a bug in the mmc core if this ever happen.
>
> Therefore I am particularly interested to find out if this is really
> needed or it's just playing safe?
You could call it playing safe.
We haven't hit any case where it was set to zero but per mmc_switch() 
description you are allowed to set it to zero to let the host decide 
what it wants to use.

>> +                               timeout = 100000000U;
>> +
>> +                       set_data_timeout(host, timeout, 0);
>> +               }
>>                  return 0;
>>          }
>>
>> --
>> 1.9.1
>>
> Kind regards
> Uffe
Regards,
RK

Re: [PATCH 2/3] mmc: host: omap_hsmmc: use generic_cmd6_time to program timeout value for CMD6

[Ulf Hansson]

[...]

>>> +                       /*
>>> +                        * Set an arbitrary 100ms data timeout for
>>> commands with
>>> +                        * busy signal and no indication of busy_timeout.
>>> +                        */
>>> +                       if (!timeout)
>>
>> This is a bug in the mmc core if this ever happen.
>>
>> Therefore I am particularly interested to find out if this is really
>> needed or it's just playing safe?
>
> You could call it playing safe.
> We haven't hit any case where it was set to zero but per mmc_switch()
> description you are allowed to set it to zero to let the host decide what it
> wants to use.

I check the code in the core. Apparently there are some cases when
INAND_CMD38_ARG* is used, but also some cases where I think the
timeout value becomes picked from the EXT_CSD without validating its
value.

Let's keep $subject patch as is, then allow me to submit a few changes
for core to deal with this properly.

>
>>> +                               timeout = 100000000U;
>>> +
>>> +                       set_data_timeout(host, timeout, 0);
>>> +               }
>>>                  return 0;
>>>          }

Kind regards
Uffe

[PATCH 3/3] mmc: host: omap_hsmmc: avoid possible overflow of timeout value

[Ravikumar Kattekola]

Fixes: a45c6cb81647 ("[ARM] 5369/1: omap mmc: Add new omap
hsmmc controller for 2430 and 34xx, v3")

when using really large timeout (up to 4*60*1000 ms for bkops)
there is a possibility of data overflow using
unsigned int so use 64 bit unsigned long long.

Signed-off-by: Ravikumar Kattekola <rk@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 drivers/mmc/host/omap_hsmmc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 51ca3d7..a58bd65 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1469,10 +1469,11 @@ static int omap_hsmmc_setup_dma_transfer(struct omap_hsmmc_host *host,
 }
 
 static void set_data_timeout(struct omap_hsmmc_host *host,
-			     unsigned int timeout_ns,
+			     unsigned long long timeout_ns,
 			     unsigned int timeout_clks)
 {
-	unsigned int timeout, cycle_ns;
+	unsigned long long timeout = timeout_ns;
+	unsigned int cycle_ns;
 	uint32_t reg, clkd, dto = 0;
 
 	reg = OMAP_HSMMC_READ(host->base, SYSCTL);
@@ -1481,7 +1482,7 @@ static void set_data_timeout(struct omap_hsmmc_host *host,
 		clkd = 1;
 
 	cycle_ns = 1000000000 / (host->clk_rate / clkd);
-	timeout = timeout_ns / cycle_ns;
+	do_div(timeout, cycle_ns);
 	timeout += timeout_clks;
 	if (timeout) {
 		while ((timeout & 0x80000000) == 0) {
@@ -1527,7 +1528,7 @@ static void omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host)
 omap_hsmmc_prepare_data(struct omap_hsmmc_host *host, struct mmc_request *req)
 {
 	int ret;
-	unsigned int timeout;
+	unsigned long long timeout;
 
 	host->data = req->data;
 
-- 
1.9.1

Re: [PATCH 0/3] mmc: host: omap_hsmmc: miscellaneous fixes

[Ulf Hansson]

On 30 January 2017 at 11:11, Ravikumar Kattekola <rk@ti.com> wrote:
> A few issues have been identified during mmc testing and following
> set of patches fixes a couple of them related to
> *timeout handling and ceb error handling*
>
> Tested on DRA75X PG 2.0 Rev H EVM
>
> Kishon Vijay Abraham I (1):
>   mmc: host: omap_hsmmc: use generic_cmd6_time to program timeout value
>     for CMD6
>
> Ravikumar Kattekola (2):
>   mmc: host: omap_hsmmc: reset cmd line on ceb error
>   mmc: host: omap_hsmmc: avoid possible overflow of timeout value
>
>  drivers/mmc/host/omap_hsmmc.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
>
> --
> 1.9.1
>


Assuming that you target 4.10 rcs for this and you want me to add a stable tag?

Kind regards
Uffe

Re: [PATCH 0/3] mmc: host: omap_hsmmc: miscellaneous fixes

[Ravikumar]

On Tuesday 31 January 2017 03:45 PM, Ulf Hansson wrote:
> On 30 January 2017 at 11:11, Ravikumar Kattekola <rk@ti.com> wrote:
>> A few issues have been identified during mmc testing and following
>> set of patches fixes a couple of them related to
>> *timeout handling and ceb error handling*
>>
>> Tested on DRA75X PG 2.0 Rev H EVM
>>
>> Kishon Vijay Abraham I (1):
>>    mmc: host: omap_hsmmc: use generic_cmd6_time to program timeout value
>>      for CMD6
>>
>> Ravikumar Kattekola (2):
>>    mmc: host: omap_hsmmc: reset cmd line on ceb error
>>    mmc: host: omap_hsmmc: avoid possible overflow of timeout value
>>
>>   drivers/mmc/host/omap_hsmmc.c | 29 +++++++++++++++++++----------
>>   1 file changed, 19 insertions(+), 10 deletions(-)
>>
>> --
>> 1.9.1
>>
>
> Assuming that you target 4.10 rcs for this and you want me to add a stable tag?
These are not critical fixes so can be queued for next , go to 4.11.
> Kind regards
> Uffe
Thanks and Regards,
RK

Re: [PATCH 0/3] mmc: host: omap_hsmmc: miscellaneous fixes

[Ulf Hansson]

On 31 January 2017 at 11:34, Ravikumar <rk@ti.com> wrote:
>
>
> On Tuesday 31 January 2017 03:45 PM, Ulf Hansson wrote:
>>
>> On 30 January 2017 at 11:11, Ravikumar Kattekola <rk@ti.com> wrote:
>>>
>>> A few issues have been identified during mmc testing and following
>>> set of patches fixes a couple of them related to
>>> *timeout handling and ceb error handling*
>>>
>>> Tested on DRA75X PG 2.0 Rev H EVM
>>>
>>> Kishon Vijay Abraham I (1):
>>>    mmc: host: omap_hsmmc: use generic_cmd6_time to program timeout value
>>>      for CMD6
>>>
>>> Ravikumar Kattekola (2):
>>>    mmc: host: omap_hsmmc: reset cmd line on ceb error
>>>    mmc: host: omap_hsmmc: avoid possible overflow of timeout value
>>>
>>>   drivers/mmc/host/omap_hsmmc.c | 29 +++++++++++++++++++----------
>>>   1 file changed, 19 insertions(+), 10 deletions(-)
>>>
>>> --
>>> 1.9.1
>>>
>>
>> Assuming that you target 4.10 rcs for this and you want me to add a stable
>> tag?
>
> These are not critical fixes so can be queued for next , go to 4.11.

Thanks, applied for next!

Kind regards
Uffe