Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1570561 > unrolled thread
| Started by | Ulf Hansson <ulf.hansson@linaro.org> |
|---|---|
| First post | 2017-01-31 11:20 +0100 |
| Last post | 2017-01-31 21:40 +0100 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 2/3] mmc: host: omap_hsmmc: use generic_cmd6_time to program timeout value for CMD6 Ulf Hansson <ulf.hansson@linaro.org> - 2017-01-31 11:20 +0100
Re: [PATCH 2/3] mmc: host: omap_hsmmc: use generic_cmd6_time to program timeout value for CMD6 Ulf Hansson <ulf.hansson@linaro.org> - 2017-01-31 12:00 +0100
Re: [PATCH 2/3] mmc: host: omap_hsmmc: use generic_cmd6_time to program timeout value for CMD6 Ravikumar <rk@ti.com> - 2017-01-31 21:40 +0100
| From | Ulf Hansson <ulf.hansson@linaro.org> |
|---|---|
| Date | 2017-01-31 11:20 +0100 |
| Subject | Re: [PATCH 2/3] mmc: host: omap_hsmmc: use generic_cmd6_time to program timeout value for CMD6 |
| Message-ID | <t5DHz-7V9-1@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Ulf Hansson <ulf.hansson@linaro.org> |
|---|---|
| Date | 2017-01-31 12:00 +0100 |
| Message-ID | <t5Ekh-87F-5@gated-at.bofh.it> |
| In reply to | #1570561 |
[...] >>> + /* >>> + * 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
[toc] | [prev] | [next] | [standalone]
| From | Ravikumar <rk@ti.com> |
|---|---|
| Date | 2017-01-31 21:40 +0100 |
| Message-ID | <t5Ekh-87F-7@gated-at.bofh.it> |
| In reply to | #1570561 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web