* [PATCH] mmc: dw_mmc: Support more time for data read timeouts
@ 2021-08-27 8:56 Mårten Lindahl
2021-09-06 16:36 ` Ulf Hansson
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Mårten Lindahl @ 2021-08-27 8:56 UTC (permalink / raw)
To: Jaehoon Chung, Ulf Hansson; +Cc: kernel, linux-mmc, Mårten Lindahl
For data read transfers a data transfer over timer (dto_timer) is
started to make sure the data command can be completed in cases the Data
Transfer Over (DTO) interrupt does not come. This timer was originally
introduced as a quirk in commit 57e104864bc48 ("mmc: dw_mmc: add quirk
for broken data transfer over scheme"), but is since a while back part
of the running code.
The dto timer picks the DATA_TIMEOUT value in the TMOUT register for its
timeout, which will give a max timeout of approximately 84 + (10 spare)
milliseconds on a 200MHz clock. But this register is not intended to be
used like that, since it is a counter for data read timeouts (DRTO) and
response timeouts (RTO), which will result in error interrupts in case
of data read and response delays.
The TMOUT register is always set with a full value for every transfer,
which according to the manual (and with 200MHz clock) will give a full
DRTO of:
((TMOUT[10:8] -1) * 0xFFFFFF + TMOUT[31:11] * 8) / 200000000 => ~587 ms
But as the same register is used for the dto_timer, the dto_timer will
always have a fixed timeout.
Instead of always setting a fixed value in TMOUT register, we can use
data->timeout_ns for the DRTO interrupts that actually matches what was
provided per requested command. Likewise we can also use timeout_ns for
the dto_timer, which will allow a max timeout of 587 ms, instead of the
fixed 94 ms. Furthermore, if a data error interrupt comes, it shouldn't
be necessary to wait for the dto_timer before we finish the command, but
instead we can handle it in the interrupt handler.
Lets fix this. In most cases data->timeout_ns values are given, but in
case it is not given, the maximum (default) timeout for the dto_timer,
and the DRTO, is set to approximately 587 ms.
Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
drivers/mmc/host/dw_mmc.c | 108 ++++++++++++++++++++++----------------
1 file changed, 63 insertions(+), 45 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index c3229d8c7041..0762d95293d4 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -52,6 +52,7 @@
#define DW_MCI_FREQ_MAX 200000000 /* unit: HZ */
#define DW_MCI_FREQ_MIN 100000 /* unit: HZ */
+#define DW_MCI_DATA_TMOUT_NS_MAX 587202490
#define IDMAC_INT_CLR (SDMMC_IDMAC_INT_AI | SDMMC_IDMAC_INT_NI | \
SDMMC_IDMAC_INT_CES | SDMMC_IDMAC_INT_DU | \
@@ -390,6 +391,23 @@ static inline void dw_mci_set_cto(struct dw_mci *host)
spin_unlock_irqrestore(&host->irq_lock, irqflags);
}
+static void dw_mci_set_dto(struct dw_mci *host, u32 timeout_ns)
+{
+ unsigned int dto_ns;
+ unsigned long irqflags;
+
+ if (!timeout_ns || timeout_ns > DW_MCI_DATA_TMOUT_NS_MAX)
+ dto_ns = DW_MCI_DATA_TMOUT_NS_MAX;
+ else
+ dto_ns = timeout_ns;
+
+ spin_lock_irqsave(&host->irq_lock, irqflags);
+ if (!test_bit(EVENT_DATA_COMPLETE, &host->pending_events))
+ mod_timer(&host->dto_timer,
+ jiffies + nsecs_to_jiffies(dto_ns));
+ spin_unlock_irqrestore(&host->irq_lock, irqflags);
+}
+
static void dw_mci_start_command(struct dw_mci *host,
struct mmc_command *cmd, u32 cmd_flags)
{
@@ -1144,9 +1162,10 @@ static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data)
host->sg = NULL;
host->data = data;
- if (data->flags & MMC_DATA_READ)
+ if (data->flags & MMC_DATA_READ) {
host->dir_status = DW_MCI_RECV_STATUS;
- else
+ dw_mci_set_dto(host, data->timeout_ns);
+ } else
host->dir_status = DW_MCI_SEND_STATUS;
dw_mci_ctrl_thld(host, data);
@@ -1277,6 +1296,36 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
mci_writel(host, CTYPE, (slot->ctype << slot->id));
}
+static void dw_mci_set_data_timeout(struct dw_mci *host, u32 timeout_ns)
+{
+ u32 timeout, freq_mhz, tmp, tmout;
+
+ if (!timeout_ns || timeout_ns > DW_MCI_DATA_TMOUT_NS_MAX) {
+ /* Timeout (maximum) */
+ mci_writel(host, TMOUT, 0xFFFFFFFF);
+ return;
+ }
+
+ timeout = timeout_ns / NSEC_PER_USEC;
+ freq_mhz = host->bus_hz / NSEC_PER_MSEC;
+
+ /* TMOUT[7:0] (RESPONSE_TIMEOUT) */
+ tmout = 0xFF;
+
+ /* TMOUT[10:8] (DATA_TIMEOUT) */
+ tmp = ((timeout * freq_mhz) / 0xFFFFFF) + 1;
+ tmout |= (tmp & 0x7) << 8;
+
+ /* TMOUT[31:11] (DATA_TIMEOUT) */
+ tmp = ((tmp - 1) * 0xFFFFFF) / freq_mhz;
+ tmp = (timeout - tmp) * freq_mhz / 8;
+ tmout |= (tmp & 0x1FFFFF) << 11;
+
+ mci_writel(host, TMOUT, tmout);
+ dev_dbg(host->dev, "timeout_ns: %u => TMOUT[31:8]: 0x%08x",
+ timeout_ns, tmout);
+}
+
static void __dw_mci_start_request(struct dw_mci *host,
struct dw_mci_slot *slot,
struct mmc_command *cmd)
@@ -1297,7 +1346,7 @@ static void __dw_mci_start_request(struct dw_mci *host,
data = cmd->data;
if (data) {
- mci_writel(host, TMOUT, 0xFFFFFFFF);
+ dw_mci_set_data_timeout(host, data->timeout_ns);
mci_writel(host, BYTCNT, data->blksz*data->blocks);
mci_writel(host, BLKSIZ, data->blksz);
}
@@ -1897,31 +1946,6 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
return data->error;
}
-static void dw_mci_set_drto(struct dw_mci *host)
-{
- unsigned int drto_clks;
- unsigned int drto_div;
- unsigned int drto_ms;
- unsigned long irqflags;
-
- drto_clks = mci_readl(host, TMOUT) >> 8;
- drto_div = (mci_readl(host, CLKDIV) & 0xff) * 2;
- if (drto_div == 0)
- drto_div = 1;
-
- drto_ms = DIV_ROUND_UP_ULL((u64)MSEC_PER_SEC * drto_clks * drto_div,
- host->bus_hz);
-
- /* add a bit spare time */
- drto_ms += 10;
-
- spin_lock_irqsave(&host->irq_lock, irqflags);
- if (!test_bit(EVENT_DATA_COMPLETE, &host->pending_events))
- mod_timer(&host->dto_timer,
- jiffies + msecs_to_jiffies(drto_ms));
- spin_unlock_irqrestore(&host->irq_lock, irqflags);
-}
-
static bool dw_mci_clear_pending_cmd_complete(struct dw_mci *host)
{
if (!test_bit(EVENT_CMD_COMPLETE, &host->pending_events))
@@ -2052,15 +2076,8 @@ static void dw_mci_tasklet_func(struct tasklet_struct *t)
}
if (!test_and_clear_bit(EVENT_XFER_COMPLETE,
- &host->pending_events)) {
- /*
- * If all data-related interrupts don't come
- * within the given time in reading data state.
- */
- if (host->dir_status == DW_MCI_RECV_STATUS)
- dw_mci_set_drto(host);
+ &host->pending_events))
break;
- }
set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
@@ -2091,16 +2108,8 @@ static void dw_mci_tasklet_func(struct tasklet_struct *t)
fallthrough;
case STATE_DATA_BUSY:
- if (!dw_mci_clear_pending_data_complete(host)) {
- /*
- * If data error interrupt comes but data over
- * interrupt doesn't come within the given time.
- * in reading data state.
- */
- if (host->dir_status == DW_MCI_RECV_STATUS)
- dw_mci_set_drto(host);
+ if (!dw_mci_clear_pending_data_complete(host))
break;
- }
host->data = NULL;
set_bit(EVENT_DATA_COMPLETE, &host->completed_events);
@@ -2649,12 +2658,21 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
}
if (pending & DW_MCI_DATA_ERROR_FLAGS) {
+ spin_lock(&host->irq_lock);
+
+ del_timer(&host->dto_timer);
+
/* if there is an error report DATA_ERROR */
mci_writel(host, RINTSTS, DW_MCI_DATA_ERROR_FLAGS);
host->data_status = pending;
smp_wmb(); /* drain writebuffer */
set_bit(EVENT_DATA_ERROR, &host->pending_events);
+
+ /* In case of error, we cannot expect a DTO */
+ set_bit(EVENT_DATA_COMPLETE, &host->pending_events);
tasklet_schedule(&host->tasklet);
+
+ spin_unlock(&host->irq_lock);
}
if (pending & SDMMC_INT_DATA_OVER) {
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: dw_mmc: Support more time for data read timeouts
2021-08-27 8:56 [PATCH] mmc: dw_mmc: Support more time for data read timeouts Mårten Lindahl
@ 2021-09-06 16:36 ` Ulf Hansson
2021-09-10 9:28 ` Vincent Whitchurch
2021-09-10 20:53 ` Doug Anderson
2021-09-17 7:13 ` Christian Löhle
2 siblings, 1 reply; 6+ messages in thread
From: Ulf Hansson @ 2021-09-06 16:36 UTC (permalink / raw)
To: Mårten Lindahl, Doug Anderson, Shawn Lin, Vincent Whitchurch
Cc: Jaehoon Chung, kernel, linux-mmc
+ Doug, Shawn and Vincent (maybe they can also help to review/test this change).
On Fri, 27 Aug 2021 at 10:56, Mårten Lindahl <marten.lindahl@axis.com> wrote:
>
> For data read transfers a data transfer over timer (dto_timer) is
> started to make sure the data command can be completed in cases the Data
> Transfer Over (DTO) interrupt does not come. This timer was originally
> introduced as a quirk in commit 57e104864bc48 ("mmc: dw_mmc: add quirk
> for broken data transfer over scheme"), but is since a while back part
> of the running code.
>
> The dto timer picks the DATA_TIMEOUT value in the TMOUT register for its
> timeout, which will give a max timeout of approximately 84 + (10 spare)
> milliseconds on a 200MHz clock. But this register is not intended to be
> used like that, since it is a counter for data read timeouts (DRTO) and
> response timeouts (RTO), which will result in error interrupts in case
> of data read and response delays.
>
> The TMOUT register is always set with a full value for every transfer,
> which according to the manual (and with 200MHz clock) will give a full
> DRTO of:
> ((TMOUT[10:8] -1) * 0xFFFFFF + TMOUT[31:11] * 8) / 200000000 => ~587 ms
>
> But as the same register is used for the dto_timer, the dto_timer will
> always have a fixed timeout.
>
> Instead of always setting a fixed value in TMOUT register, we can use
> data->timeout_ns for the DRTO interrupts that actually matches what was
> provided per requested command. Likewise we can also use timeout_ns for
> the dto_timer, which will allow a max timeout of 587 ms, instead of the
> fixed 94 ms. Furthermore, if a data error interrupt comes, it shouldn't
> be necessary to wait for the dto_timer before we finish the command, but
> instead we can handle it in the interrupt handler.
>
> Lets fix this. In most cases data->timeout_ns values are given, but in
> case it is not given, the maximum (default) timeout for the dto_timer,
> and the DRTO, is set to approximately 587 ms.
>
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
As this touches common dw_mmc code, I would appreciate some
tests/reviews to be done before I apply this. Therefore I looped in
some of the people who have been involved with dw_mmc lately.
Kind regards
Uffe
> ---
> drivers/mmc/host/dw_mmc.c | 108 ++++++++++++++++++++++----------------
> 1 file changed, 63 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index c3229d8c7041..0762d95293d4 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -52,6 +52,7 @@
>
> #define DW_MCI_FREQ_MAX 200000000 /* unit: HZ */
> #define DW_MCI_FREQ_MIN 100000 /* unit: HZ */
> +#define DW_MCI_DATA_TMOUT_NS_MAX 587202490
>
> #define IDMAC_INT_CLR (SDMMC_IDMAC_INT_AI | SDMMC_IDMAC_INT_NI | \
> SDMMC_IDMAC_INT_CES | SDMMC_IDMAC_INT_DU | \
> @@ -390,6 +391,23 @@ static inline void dw_mci_set_cto(struct dw_mci *host)
> spin_unlock_irqrestore(&host->irq_lock, irqflags);
> }
>
> +static void dw_mci_set_dto(struct dw_mci *host, u32 timeout_ns)
> +{
> + unsigned int dto_ns;
> + unsigned long irqflags;
> +
> + if (!timeout_ns || timeout_ns > DW_MCI_DATA_TMOUT_NS_MAX)
> + dto_ns = DW_MCI_DATA_TMOUT_NS_MAX;
> + else
> + dto_ns = timeout_ns;
> +
> + spin_lock_irqsave(&host->irq_lock, irqflags);
> + if (!test_bit(EVENT_DATA_COMPLETE, &host->pending_events))
> + mod_timer(&host->dto_timer,
> + jiffies + nsecs_to_jiffies(dto_ns));
> + spin_unlock_irqrestore(&host->irq_lock, irqflags);
> +}
> +
> static void dw_mci_start_command(struct dw_mci *host,
> struct mmc_command *cmd, u32 cmd_flags)
> {
> @@ -1144,9 +1162,10 @@ static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data)
> host->sg = NULL;
> host->data = data;
>
> - if (data->flags & MMC_DATA_READ)
> + if (data->flags & MMC_DATA_READ) {
> host->dir_status = DW_MCI_RECV_STATUS;
> - else
> + dw_mci_set_dto(host, data->timeout_ns);
> + } else
> host->dir_status = DW_MCI_SEND_STATUS;
>
> dw_mci_ctrl_thld(host, data);
> @@ -1277,6 +1296,36 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> mci_writel(host, CTYPE, (slot->ctype << slot->id));
> }
>
> +static void dw_mci_set_data_timeout(struct dw_mci *host, u32 timeout_ns)
> +{
> + u32 timeout, freq_mhz, tmp, tmout;
> +
> + if (!timeout_ns || timeout_ns > DW_MCI_DATA_TMOUT_NS_MAX) {
> + /* Timeout (maximum) */
> + mci_writel(host, TMOUT, 0xFFFFFFFF);
> + return;
> + }
> +
> + timeout = timeout_ns / NSEC_PER_USEC;
> + freq_mhz = host->bus_hz / NSEC_PER_MSEC;
> +
> + /* TMOUT[7:0] (RESPONSE_TIMEOUT) */
> + tmout = 0xFF;
> +
> + /* TMOUT[10:8] (DATA_TIMEOUT) */
> + tmp = ((timeout * freq_mhz) / 0xFFFFFF) + 1;
> + tmout |= (tmp & 0x7) << 8;
> +
> + /* TMOUT[31:11] (DATA_TIMEOUT) */
> + tmp = ((tmp - 1) * 0xFFFFFF) / freq_mhz;
> + tmp = (timeout - tmp) * freq_mhz / 8;
> + tmout |= (tmp & 0x1FFFFF) << 11;
> +
> + mci_writel(host, TMOUT, tmout);
> + dev_dbg(host->dev, "timeout_ns: %u => TMOUT[31:8]: 0x%08x",
> + timeout_ns, tmout);
> +}
> +
> static void __dw_mci_start_request(struct dw_mci *host,
> struct dw_mci_slot *slot,
> struct mmc_command *cmd)
> @@ -1297,7 +1346,7 @@ static void __dw_mci_start_request(struct dw_mci *host,
>
> data = cmd->data;
> if (data) {
> - mci_writel(host, TMOUT, 0xFFFFFFFF);
> + dw_mci_set_data_timeout(host, data->timeout_ns);
> mci_writel(host, BYTCNT, data->blksz*data->blocks);
> mci_writel(host, BLKSIZ, data->blksz);
> }
> @@ -1897,31 +1946,6 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
> return data->error;
> }
>
> -static void dw_mci_set_drto(struct dw_mci *host)
> -{
> - unsigned int drto_clks;
> - unsigned int drto_div;
> - unsigned int drto_ms;
> - unsigned long irqflags;
> -
> - drto_clks = mci_readl(host, TMOUT) >> 8;
> - drto_div = (mci_readl(host, CLKDIV) & 0xff) * 2;
> - if (drto_div == 0)
> - drto_div = 1;
> -
> - drto_ms = DIV_ROUND_UP_ULL((u64)MSEC_PER_SEC * drto_clks * drto_div,
> - host->bus_hz);
> -
> - /* add a bit spare time */
> - drto_ms += 10;
> -
> - spin_lock_irqsave(&host->irq_lock, irqflags);
> - if (!test_bit(EVENT_DATA_COMPLETE, &host->pending_events))
> - mod_timer(&host->dto_timer,
> - jiffies + msecs_to_jiffies(drto_ms));
> - spin_unlock_irqrestore(&host->irq_lock, irqflags);
> -}
> -
> static bool dw_mci_clear_pending_cmd_complete(struct dw_mci *host)
> {
> if (!test_bit(EVENT_CMD_COMPLETE, &host->pending_events))
> @@ -2052,15 +2076,8 @@ static void dw_mci_tasklet_func(struct tasklet_struct *t)
> }
>
> if (!test_and_clear_bit(EVENT_XFER_COMPLETE,
> - &host->pending_events)) {
> - /*
> - * If all data-related interrupts don't come
> - * within the given time in reading data state.
> - */
> - if (host->dir_status == DW_MCI_RECV_STATUS)
> - dw_mci_set_drto(host);
> + &host->pending_events))
> break;
> - }
>
> set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>
> @@ -2091,16 +2108,8 @@ static void dw_mci_tasklet_func(struct tasklet_struct *t)
> fallthrough;
>
> case STATE_DATA_BUSY:
> - if (!dw_mci_clear_pending_data_complete(host)) {
> - /*
> - * If data error interrupt comes but data over
> - * interrupt doesn't come within the given time.
> - * in reading data state.
> - */
> - if (host->dir_status == DW_MCI_RECV_STATUS)
> - dw_mci_set_drto(host);
> + if (!dw_mci_clear_pending_data_complete(host))
> break;
> - }
>
> host->data = NULL;
> set_bit(EVENT_DATA_COMPLETE, &host->completed_events);
> @@ -2649,12 +2658,21 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
> }
>
> if (pending & DW_MCI_DATA_ERROR_FLAGS) {
> + spin_lock(&host->irq_lock);
> +
> + del_timer(&host->dto_timer);
> +
> /* if there is an error report DATA_ERROR */
> mci_writel(host, RINTSTS, DW_MCI_DATA_ERROR_FLAGS);
> host->data_status = pending;
> smp_wmb(); /* drain writebuffer */
> set_bit(EVENT_DATA_ERROR, &host->pending_events);
> +
> + /* In case of error, we cannot expect a DTO */
> + set_bit(EVENT_DATA_COMPLETE, &host->pending_events);
> tasklet_schedule(&host->tasklet);
> +
> + spin_unlock(&host->irq_lock);
> }
>
> if (pending & SDMMC_INT_DATA_OVER) {
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: dw_mmc: Support more time for data read timeouts
2021-09-06 16:36 ` Ulf Hansson
@ 2021-09-10 9:28 ` Vincent Whitchurch
0 siblings, 0 replies; 6+ messages in thread
From: Vincent Whitchurch @ 2021-09-10 9:28 UTC (permalink / raw)
To: Ulf Hansson
Cc: Mårten Lindahl, Doug Anderson, Shawn Lin, Jaehoon Chung,
kernel, linux-mmc
On Mon, Sep 06, 2021 at 06:36:33PM +0200, Ulf Hansson wrote:
> On Fri, 27 Aug 2021 at 10:56, Mårten Lindahl <marten.lindahl@axis.com> wrote:
> > For data read transfers a data transfer over timer (dto_timer) is
> > started to make sure the data command can be completed in cases the Data
> > Transfer Over (DTO) interrupt does not come. This timer was originally
> > introduced as a quirk in commit 57e104864bc48 ("mmc: dw_mmc: add quirk
> > for broken data transfer over scheme"), but is since a while back part
> > of the running code.
> >
> > The dto timer picks the DATA_TIMEOUT value in the TMOUT register for its
> > timeout, which will give a max timeout of approximately 84 + (10 spare)
> > milliseconds on a 200MHz clock. But this register is not intended to be
> > used like that, since it is a counter for data read timeouts (DRTO) and
> > response timeouts (RTO), which will result in error interrupts in case
> > of data read and response delays.
> >
> > The TMOUT register is always set with a full value for every transfer,
> > which according to the manual (and with 200MHz clock) will give a full
> > DRTO of:
> > ((TMOUT[10:8] -1) * 0xFFFFFF + TMOUT[31:11] * 8) / 200000000 => ~587 ms
> >
> > But as the same register is used for the dto_timer, the dto_timer will
> > always have a fixed timeout.
> >
> > Instead of always setting a fixed value in TMOUT register, we can use
> > data->timeout_ns for the DRTO interrupts that actually matches what was
> > provided per requested command. Likewise we can also use timeout_ns for
> > the dto_timer, which will allow a max timeout of 587 ms, instead of the
> > fixed 94 ms. Furthermore, if a data error interrupt comes, it shouldn't
> > be necessary to wait for the dto_timer before we finish the command, but
> > instead we can handle it in the interrupt handler.
> >
> > Lets fix this. In most cases data->timeout_ns values are given, but in
> > case it is not given, the maximum (default) timeout for the dto_timer,
> > and the DRTO, is set to approximately 587 ms.
> >
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
>
> As this touches common dw_mmc code, I would appreciate some
> tests/reviews to be done before I apply this. Therefore I looped in
> some of the people who have been involved with dw_mmc lately.
>
> + Doug, Shawn and Vincent (maybe they can also help to review/test this change).
I have the same hardware as Mårten (ARTPEC-8) so I don't think my
testing would add much. We'd ideally need someone with a different
platform to help to test this.
As for the patch itself, it would perhaps be helpful if it were broken
down into three separate patches (not necessarily in the order listed
below) since it's doing three different things:
(1) Increasing the length of the software timeout.
This is the primary reason for the patch since the driver is clearly
using a wrong value for the software timeout for the entire transfer
which is lower than the timeouts required by eMMC chips' TAAC/NSAC and
the controller's timeout for (IIUC) one block.
We haven't seen any problems in practice (yet) though.
(As an aside, if I'm reading the eMMC specs correctly, the TAAC/NSAC
applies to each block of a multi-block transfer. If that's the case,
then the entire transfer could, at least in theory, take several
seconds.
Is the data->timeout_ns calculated by mmc_set_data_timeout() supposed
to be a per-block timeout or a timeout for the entire transfer?)
(2) Not waiting for DRTO on errors.
When attempting to increase the length of the software timeout as in (1)
or to remove it entirely, one realizes that the driver actually depends
on the software timeout to prevent hanging indefinitely when CRC errors
are received during tuning. It's unclear if this is something specific
to our version of the IP or if this is an old regression in the driver
(given that the driver apparently used to work fine without a software
timeout at some point of time on some platforms.)
So increasing the software timeout length will make each failing step in
tuning take half a second which would increase boot time significantly,
so that's why I think the early exit is needed.
(3) Reducing the length of the hardware timeout.
I *think* this is also to reduce initialization time if there are some
transfers which are meant to time out (without any other errors) which
would now take longer to time out after (1), but this part of the change
could use a bit more rationale.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: dw_mmc: Support more time for data read timeouts
2021-08-27 8:56 [PATCH] mmc: dw_mmc: Support more time for data read timeouts Mårten Lindahl
2021-09-06 16:36 ` Ulf Hansson
@ 2021-09-10 20:53 ` Doug Anderson
2021-09-16 10:45 ` Marten Lindahl
2021-09-17 7:13 ` Christian Löhle
2 siblings, 1 reply; 6+ messages in thread
From: Doug Anderson @ 2021-09-10 20:53 UTC (permalink / raw)
To: Mårten Lindahl
Cc: Jaehoon Chung, Ulf Hansson, kernel, Linux MMC List, Shawn Lin,
Vincent Whitchurch, Brian Norris, Chen-Yu Tsai
Hi,
My brain is pretty far away from dw_mmc these days. I touched it sorta
recently when uprevving the kernel on some rk3288 boards over a year
ago (so sorta recent?) but mostly it's paged out. Adding Brian and
Chen-Yu who might be able to test more since I think they're currently
working on an uprev of some rk3399 boards. Hopefully Shawn will be
able to chime in too.
On Fri, Aug 27, 2021 at 1:57 AM Mårten Lindahl <marten.lindahl@axis.com> wrote:
>
> For data read transfers a data transfer over timer (dto_timer) is
> started to make sure the data command can be completed in cases the Data
> Transfer Over (DTO) interrupt does not come. This timer was originally
> introduced as a quirk in commit 57e104864bc48 ("mmc: dw_mmc: add quirk
> for broken data transfer over scheme"), but is since a while back part
> of the running code.
>
> The dto timer picks the DATA_TIMEOUT value in the TMOUT register for its
> timeout, which will give a max timeout of approximately 84 + (10 spare)
> milliseconds on a 200MHz clock. But this register is not intended to be
> used like that, since it is a counter for data read timeouts (DRTO) and
> response timeouts (RTO), which will result in error interrupts in case
> of data read and response delays.
I'm not quite following your argument here. Are you just saying that
the math is wrong (because your version seems to have something
special for bits 10:8--see below) or are you saying that fundamentally
the DATA_TIMEOUT shouldn't be used for setting this timeout? As far
as I understand it, the original problem we were trying to solve was
that on Rockchip SoCs sometimes we simply weren't getting the data
timeout interrupt from hardware. As far as I understood, the data
timeout interrupt that was missing was the one controlled by the
"DATA_TIMEOUT" value. We added a software fallback for it.
> The TMOUT register is always set with a full value for every transfer,
Yeah, it's always been weird that TMOUT was just hardcoded to
0xffffffff. It was always in the back of my mind that it should be
fixed but I never saw any serious problems with it being 0xffffffff so
it never trickled to the top of the queue.
> which according to the manual (and with 200MHz clock) will give a full
> DRTO of:
> ((TMOUT[10:8] -1) * 0xFFFFFF + TMOUT[31:11] * 8) / 200000000 => ~587 ms
I wonder if perhaps you have a newer and/or customized version of this
controller. This doesn't match my definition of this register. I
searched for "rk3399 TRM" just now and found this:
https://www.t-firefly.com/download/Firefly-RK3399/docs/TRM/Rockchip%20RK3399TRM%20V1.3%20Part2.pdf
It's stamped full of "Rockchip Confidential" on it so maybe it'll go
away now that I've pointed it out, but at least you can see what I
mean. You can find "SDMMC_TMOUT" there defined as:
* 31:8 Value for Data Read Timeout and Data Starvation by Host
timeout. Value is in number of card output clocks (cclk_out) of
selected card.
* 7:0 Response timeout value. Also in card output clocks (cclk_out)
...so I don't see anything talking about 0xffffff nor about the bits
10:8 being something different.
I'll also mention that the DesignWare databook (2.80a) for dwmmc
matches what I see in the Rockchip TRM.
One last note is that the manual indicates that if we ever need a
value of > ~100 ms that we should disable the hardware timeout and
_only_ use the software timeout. Presumably this is because of the
fact that ~83 ms is the maximum value you can program here. ;-)
It seems like you'll almost certainly need a quirk or some type of
function override for your version of the driver instead of changing
this for everyone.
> But as the same register is used for the dto_timer, the dto_timer will
> always have a fixed timeout.
>
> Instead of always setting a fixed value in TMOUT register, we can use
> data->timeout_ns for the DRTO interrupts that actually matches what was
> provided per requested command. Likewise we can also use timeout_ns for
> the dto_timer, which will allow a max timeout of 587 ms, instead of the
> fixed 94 ms.
In general it feels like you should be breaking this up. First, a
patch to fix it so that TMOUT isn't always programmed ot 0xffffff.
Another patch to fix it so that it handles your special hardware and
how it handles bits 10:8. Once you do that then all of a sudden we
_won't_ have a fixed timeout for the dto_timer. Now the
dw_mci_set_drto() should read TMOUT and program a proper fallback
software delay. Done, right? If you want to add a feature to disable
the hardware timer and rely on the software timer for timeouts > ~84
ms (other dw_mmc) or > ~587 ms (your dw_mmc) then that should be yet
another separate patch.
> Furthermore, if a data error interrupt comes, it shouldn't
> be necessary to wait for the dto_timer before we finish the command, but
> instead we can handle it in the interrupt handler.
You'll need to be really careful here to make sure there aren't races.
I'll comment below.
> @@ -390,6 +391,23 @@ static inline void dw_mci_set_cto(struct dw_mci *host)
> spin_unlock_irqrestore(&host->irq_lock, irqflags);
> }
>
> +static void dw_mci_set_dto(struct dw_mci *host, u32 timeout_ns)
> +{
> + unsigned int dto_ns;
> + unsigned long irqflags;
> +
> + if (!timeout_ns || timeout_ns > DW_MCI_DATA_TMOUT_NS_MAX)
> + dto_ns = DW_MCI_DATA_TMOUT_NS_MAX;
> + else
> + dto_ns = timeout_ns;
> +
> + spin_lock_irqsave(&host->irq_lock, irqflags);
> + if (!test_bit(EVENT_DATA_COMPLETE, &host->pending_events))
> + mod_timer(&host->dto_timer,
> + jiffies + nsecs_to_jiffies(dto_ns));
> + spin_unlock_irqrestore(&host->irq_lock, irqflags);
> +}
You got rid of the extra 10 ms "slop". Assuming that the hardware
timer is "mostly reliable" like it was on Rockchip SoCs (it fired
properly at least 90% of the time from my memory) it pretty much means
that you're _always_ going to get both timeouts firing at the exact
same time, which seems like a bit of a waste. Having some extra slop
in there was nice because it gave the normal path a chance to run.
I would also say that I kinda liked the fact that the old code based
the software timeout on the value in the TMOUT register. The whole
point was that it was a fallback for the hardware timeout not firing.
Basing it on the same value the hardware timer was supposed to use
seemed cleaner. Then if there were rounding errors it would affect
both equally.
> @@ -1144,9 +1162,10 @@ static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data)
> host->sg = NULL;
> host->data = data;
>
> - if (data->flags & MMC_DATA_READ)
> + if (data->flags & MMC_DATA_READ) {
> host->dir_status = DW_MCI_RECV_STATUS;
> - else
> + dw_mci_set_dto(host, data->timeout_ns);
Are you sure it's safe to start this timeout right here? Shouldn't you
start the timeout _after_ you've started the hardware transfer?
Otherwise what happens if an interrupt storm occurs right after you
start the timeout but before you started the transfer? The timeout
could fire even before the transfer starts, right?
> @@ -1277,6 +1296,36 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> mci_writel(host, CTYPE, (slot->ctype << slot->id));
> }
>
> +static void dw_mci_set_data_timeout(struct dw_mci *host, u32 timeout_ns)
> +{
> + u32 timeout, freq_mhz, tmp, tmout;
> +
> + if (!timeout_ns || timeout_ns > DW_MCI_DATA_TMOUT_NS_MAX) {
> + /* Timeout (maximum) */
> + mci_writel(host, TMOUT, 0xFFFFFFFF);
> + return;
> + }
> +
> + timeout = timeout_ns / NSEC_PER_USEC;
> + freq_mhz = host->bus_hz / NSEC_PER_MSEC;
Double check rounding. Presumably you always want the resulting
timeout rounded _up_. So both of the above should probably be
"DIV_ROUND_UP", right?
> + /* TMOUT[7:0] (RESPONSE_TIMEOUT) */
> + tmout = 0xFF;
> +
> + /* TMOUT[10:8] (DATA_TIMEOUT) */
> + tmp = ((timeout * freq_mhz) / 0xFFFFFF) + 1;
> + tmout |= (tmp & 0x7) << 8;
> +
> + /* TMOUT[31:11] (DATA_TIMEOUT) */
> + tmp = ((tmp - 1) * 0xFFFFFF) / freq_mhz;
> + tmp = (timeout - tmp) * freq_mhz / 8;
Again, be careful of rounding and make sure you're rounding the
resulting timeout up. You probably want to calculate the [10:8] value
first and _then_ subtract it out from the timeout to be sure?
> @@ -2649,12 +2658,21 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
> }
>
> if (pending & DW_MCI_DATA_ERROR_FLAGS) {
> + spin_lock(&host->irq_lock);
> +
> + del_timer(&host->dto_timer);
> +
> /* if there is an error report DATA_ERROR */
> mci_writel(host, RINTSTS, DW_MCI_DATA_ERROR_FLAGS);
> host->data_status = pending;
> smp_wmb(); /* drain writebuffer */
> set_bit(EVENT_DATA_ERROR, &host->pending_events);
> +
> + /* In case of error, we cannot expect a DTO */
> + set_bit(EVENT_DATA_COMPLETE, &host->pending_events);
> tasklet_schedule(&host->tasklet);
> +
> + spin_unlock(&host->irq_lock);
I'm fairly certain that, at least on hardware I tested on, we _did_
get a DTO after errors. It's been a long time and my memory could be
fuzzy, but maybe this is just a quirk of your hardware or something
that changed in newer versions of dw_mmc?
-Doug
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: dw_mmc: Support more time for data read timeouts
2021-09-10 20:53 ` Doug Anderson
@ 2021-09-16 10:45 ` Marten Lindahl
0 siblings, 0 replies; 6+ messages in thread
From: Marten Lindahl @ 2021-09-16 10:45 UTC (permalink / raw)
To: Doug Anderson
Cc: Mårten Lindahl, Jaehoon Chung, Ulf Hansson, kernel,
Linux MMC List, Shawn Lin, Vincent Whitchurch, Brian Norris,
Chen-Yu Tsai
On Fri, Sep 10, 2021 at 10:53:43PM +0200, Doug Anderson wrote:
Hi Doug!
Thanks for your feedback.
> Hi,
>
> My brain is pretty far away from dw_mmc these days. I touched it sorta
> recently when uprevving the kernel on some rk3288 boards over a year
> ago (so sorta recent?) but mostly it's paged out. Adding Brian and
> Chen-Yu who might be able to test more since I think they're currently
> working on an uprev of some rk3399 boards. Hopefully Shawn will be
> able to chime in too.
>
> On Fri, Aug 27, 2021 at 1:57 AM Mårten Lindahl <marten.lindahl@axis.com> wrote:
> >
> > For data read transfers a data transfer over timer (dto_timer) is
> > started to make sure the data command can be completed in cases the Data
> > Transfer Over (DTO) interrupt does not come. This timer was originally
> > introduced as a quirk in commit 57e104864bc48 ("mmc: dw_mmc: add quirk
> > for broken data transfer over scheme"), but is since a while back part
> > of the running code.
> >
> > The dto timer picks the DATA_TIMEOUT value in the TMOUT register for its
> > timeout, which will give a max timeout of approximately 84 + (10 spare)
> > milliseconds on a 200MHz clock. But this register is not intended to be
> > used like that, since it is a counter for data read timeouts (DRTO) and
> > response timeouts (RTO), which will result in error interrupts in case
> > of data read and response delays.
>
> I'm not quite following your argument here. Are you just saying that
> the math is wrong (because your version seems to have something
> special for bits 10:8--see below) or are you saying that fundamentally
> the DATA_TIMEOUT shouldn't be used for setting this timeout? As far
> as I understand it, the original problem we were trying to solve was
> that on Rockchip SoCs sometimes we simply weren't getting the data
> timeout interrupt from hardware. As far as I understood, the data
> timeout interrupt that was missing was the one controlled by the
> "DATA_TIMEOUT" value. We added a software fallback for it.
>
I understand the reason for having the software timeout. And after reading
the documentation for the rk3399 TRM it came clear to me why the current
code looks the way it is. However, the Samsung manual (which is the one
I have) does not follow the (standard?) TMOUT:DATA_TIMEOUT field definition,
but instead it states:
For card data read timeout (DRTO):
[31:8]: ((TMOUT[10:8]-1)*0xffffff + TMOUT[31:11] * 8) cycle
For host starvation timeout (HTO): TMOUT[31:8] cycle
We will ask Samsung about this.
>
> > The TMOUT register is always set with a full value for every transfer,
>
> Yeah, it's always been weird that TMOUT was just hardcoded to
> 0xffffffff. It was always in the back of my mind that it should be
> fixed but I never saw any serious problems with it being 0xffffffff so
> it never trickled to the top of the queue.
>
>
> > which according to the manual (and with 200MHz clock) will give a full
> > DRTO of:
> > ((TMOUT[10:8] -1) * 0xFFFFFF + TMOUT[31:11] * 8) / 200000000 => ~587 ms
>
> I wonder if perhaps you have a newer and/or customized version of this
> controller. This doesn't match my definition of this register. I
> searched for "rk3399 TRM" just now and found this:
>
> https://www.t-firefly.com/download/Firefly-RK3399/docs/TRM/Rockchip%20RK3399TRM%20V1.3%20Part2.pdf
>
> It's stamped full of "Rockchip Confidential" on it so maybe it'll go
> away now that I've pointed it out, but at least you can see what I
> mean. You can find "SDMMC_TMOUT" there defined as:
>
> * 31:8 Value for Data Read Timeout and Data Starvation by Host
> timeout. Value is in number of card output clocks (cclk_out) of
> selected card.
>
> * 7:0 Response timeout value. Also in card output clocks (cclk_out)
>
> ...so I don't see anything talking about 0xffffff nor about the bits
> 10:8 being something different.
>
> I'll also mention that the DesignWare databook (2.80a) for dwmmc
> matches what I see in the Rockchip TRM.
>
> One last note is that the manual indicates that if we ever need a
> value of > ~100 ms that we should disable the hardware timeout and
> _only_ use the software timeout. Presumably this is because of the
> fact that ~83 ms is the maximum value you can program here. ;-)
>
Yes, that was one of my intentions with this patch, but clearly I
stumbled on the DATA_TIMEOUT bits, which (at least with the Samsung
version) I thought could give us much more time. Unfortunately I don't
have access to the original DesignWare dwmmc manual, which would of course
bring more light to this.
>
> It seems like you'll almost certainly need a quirk or some type of
> function override for your version of the driver instead of changing
> this for everyone.
>
Agree. But we will ask Samsung before uploading a patch for this.
>
> > But as the same register is used for the dto_timer, the dto_timer will
> > always have a fixed timeout.
> >
> > Instead of always setting a fixed value in TMOUT register, we can use
> > data->timeout_ns for the DRTO interrupts that actually matches what was
> > provided per requested command. Likewise we can also use timeout_ns for
> > the dto_timer, which will allow a max timeout of 587 ms, instead of the
> > fixed 94 ms.
>
> In general it feels like you should be breaking this up. First, a
> patch to fix it so that TMOUT isn't always programmed ot 0xffffff.
> Another patch to fix it so that it handles your special hardware and
> how it handles bits 10:8. Once you do that then all of a sudden we
> _won't_ have a fixed timeout for the dto_timer. Now the
> dw_mci_set_drto() should read TMOUT and program a proper fallback
> software delay. Done, right? If you want to add a feature to disable
> the hardware timer and rely on the software timer for timeouts > ~84
> ms (other dw_mmc) or > ~587 ms (your dw_mmc) then that should be yet
> another separate patch.
>
Agree. Good point. Vincent also suggested splitting it into three
different patches.
>
> > Furthermore, if a data error interrupt comes, it shouldn't
> > be necessary to wait for the dto_timer before we finish the command, but
> > instead we can handle it in the interrupt handler.
>
> You'll need to be really careful here to make sure there aren't races.
> I'll comment below.
>
>
> > @@ -390,6 +391,23 @@ static inline void dw_mci_set_cto(struct dw_mci *host)
> > spin_unlock_irqrestore(&host->irq_lock, irqflags);
> > }
> >
> > +static void dw_mci_set_dto(struct dw_mci *host, u32 timeout_ns)
> > +{
> > + unsigned int dto_ns;
> > + unsigned long irqflags;
> > +
> > + if (!timeout_ns || timeout_ns > DW_MCI_DATA_TMOUT_NS_MAX)
> > + dto_ns = DW_MCI_DATA_TMOUT_NS_MAX;
> > + else
> > + dto_ns = timeout_ns;
> > +
> > + spin_lock_irqsave(&host->irq_lock, irqflags);
> > + if (!test_bit(EVENT_DATA_COMPLETE, &host->pending_events))
> > + mod_timer(&host->dto_timer,
> > + jiffies + nsecs_to_jiffies(dto_ns));
> > + spin_unlock_irqrestore(&host->irq_lock, irqflags);
> > +}
>
> You got rid of the extra 10 ms "slop". Assuming that the hardware
> timer is "mostly reliable" like it was on Rockchip SoCs (it fired
> properly at least 90% of the time from my memory) it pretty much means
> that you're _always_ going to get both timeouts firing at the exact
> same time, which seems like a bit of a waste. Having some extra slop
> in there was nice because it gave the normal path a chance to run.
Understood.
>
> I would also say that I kinda liked the fact that the old code based
> the software timeout on the value in the TMOUT register. The whole
> point was that it was a fallback for the hardware timeout not firing.
> Basing it on the same value the hardware timer was supposed to use
> seemed cleaner. Then if there were rounding errors it would affect
> both equally.
>
>
> > @@ -1144,9 +1162,10 @@ static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data)
> > host->sg = NULL;
> > host->data = data;
> >
> > - if (data->flags & MMC_DATA_READ)
> > + if (data->flags & MMC_DATA_READ) {
> > host->dir_status = DW_MCI_RECV_STATUS;
> > - else
> > + dw_mci_set_dto(host, data->timeout_ns);
>
> Are you sure it's safe to start this timeout right here? Shouldn't you
> start the timeout _after_ you've started the hardware transfer?
> Otherwise what happens if an interrupt storm occurs right after you
> start the timeout but before you started the transfer? The timeout
> could fire even before the transfer starts, right?
Good point. Will look into that.
>
>
> > @@ -1277,6 +1296,36 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> > mci_writel(host, CTYPE, (slot->ctype << slot->id));
> > }
> >
> > +static void dw_mci_set_data_timeout(struct dw_mci *host, u32 timeout_ns)
> > +{
> > + u32 timeout, freq_mhz, tmp, tmout;
> > +
> > + if (!timeout_ns || timeout_ns > DW_MCI_DATA_TMOUT_NS_MAX) {
> > + /* Timeout (maximum) */
> > + mci_writel(host, TMOUT, 0xFFFFFFFF);
> > + return;
> > + }
> > +
> > + timeout = timeout_ns / NSEC_PER_USEC;
> > + freq_mhz = host->bus_hz / NSEC_PER_MSEC;
>
> Double check rounding. Presumably you always want the resulting
> timeout rounded _up_. So both of the above should probably be
> "DIV_ROUND_UP", right?
Will do.
>
>
> > + /* TMOUT[7:0] (RESPONSE_TIMEOUT) */
> > + tmout = 0xFF;
> > +
> > + /* TMOUT[10:8] (DATA_TIMEOUT) */
> > + tmp = ((timeout * freq_mhz) / 0xFFFFFF) + 1;
> > + tmout |= (tmp & 0x7) << 8;
> > +
> > + /* TMOUT[31:11] (DATA_TIMEOUT) */
> > + tmp = ((tmp - 1) * 0xFFFFFF) / freq_mhz;
> > + tmp = (timeout - tmp) * freq_mhz / 8;
>
> Again, be careful of rounding and make sure you're rounding the
> resulting timeout up. You probably want to calculate the [10:8] value
> first and _then_ subtract it out from the timeout to be sure?
>
>
> > @@ -2649,12 +2658,21 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
> > }
> >
> > if (pending & DW_MCI_DATA_ERROR_FLAGS) {
> > + spin_lock(&host->irq_lock);
> > +
> > + del_timer(&host->dto_timer);
> > +
> > /* if there is an error report DATA_ERROR */
> > mci_writel(host, RINTSTS, DW_MCI_DATA_ERROR_FLAGS);
> > host->data_status = pending;
> > smp_wmb(); /* drain writebuffer */
> > set_bit(EVENT_DATA_ERROR, &host->pending_events);
> > +
> > + /* In case of error, we cannot expect a DTO */
> > + set_bit(EVENT_DATA_COMPLETE, &host->pending_events);
> > tasklet_schedule(&host->tasklet);
> > +
> > + spin_unlock(&host->irq_lock);
>
> I'm fairly certain that, at least on hardware I tested on, we _did_
> get a DTO after errors. It's been a long time and my memory could be
> fuzzy, but maybe this is just a quirk of your hardware or something
> that changed in newer versions of dw_mmc?
I always wondered about this. On our hardware we don't always get the DTO
after errors which is why it (to us) seems the driver is relying on the
dto_timer to complete the state machine. So again, yes, it seems we have
a different version of this controller, and a quirk is probably needed
to make this efficient for us.
Kind regards
Mårten
>
> -Doug
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: dw_mmc: Support more time for data read timeouts
2021-08-27 8:56 [PATCH] mmc: dw_mmc: Support more time for data read timeouts Mårten Lindahl
2021-09-06 16:36 ` Ulf Hansson
2021-09-10 20:53 ` Doug Anderson
@ 2021-09-17 7:13 ` Christian Löhle
2 siblings, 0 replies; 6+ messages in thread
From: Christian Löhle @ 2021-09-17 7:13 UTC (permalink / raw)
To: Mårten Lindahl, Jaehoon Chung, Ulf Hansson; +Cc: kernel, linux-mmc
So I'm in favor in reworking the DRTO timeout calculation to something more meaningful.
(Maybe we should keep the R though as it currently only affects reads, but more on that later).
After playing around with dw_mmc (on the rk3399) Ive seen 3 issues with the current driver.
Lets look at the traces (format: timestamp,type(1=cmd,2=rsp48, 3=rsp136, 4=dataover),cmdopcode,arg)
-The worst of them Id say is this one:
32611.175228177,1,25,059f5000
32611.175247135,2,25,00000080
32611.175265218,0,RESP TO CMD 25 : 80 CONTAINED AN ERROR -84
--- hang --- tasklet never getting scheduled again
I send a patch for that yesterday.
-Then there's the case where the register is at reset value and the timeout is about 167 seconds
1547.207376888,1,55,00000000
1547.207986555,2,55,00000120
1547.208669638,2,41,80ff8000
1547.208064638,1,41,41040000
1547.208736055,1,02,00000000
1547.209899513,1,03,00000000
1547.209786721,3,02,00000000000000000000000000000001
1547.210519680,2,03,00010520
1547.210577638,1,09,00010000
1547.211636471,3,09,007f00321b5980f26ebb7fff1640009b
1547.211717763,1,07,00010000
1547.212334721,2,07,00000700
1547.212402305,1,55,00010000
1547.213022180,2,55,00000920
1547.213096471,1,51,00000000
1547.213718680,2,51,00000920
1547.214967596,4,51,0,8
1547.214971096,5,51,00a5000000000000
1547.215036930,1,55,00010000
1547.215660013,2,55,00000920
1547.215747721,1,13,00000000
1547.216369930,2,13,00000920
1631.102136318,0,Sending stop abort due data ERROR: data status: 208
Note the long gap between the last two lines, I can send a simple fix for that today.
-The last one I'm not sure why it occurs but I can also see something like this
77769.308526825,1,23,0000000c
77769.308550158,2,23,00000900
77769.308598575,1,25,08ac7800
77769.308614908,2,25,00000900
-- hang -- tasklet never getting scheduled again
To my understanding there should not be a state where we don't receive an interrupt
when a data write times out, but apparently this can happen.
This patch would fix the second issue.
Can we extend it to catch a dtwo timeout, too?
I'd just add a flag in dw_mci_set_dto for writes and give it some extra slack.
The case happened rarely enough for me that I'd say anything that is not a complete hang of the driver
is a good enough improvement.
Regards,
Christian
From: Mårten Lindahl <marten.lindahl@axis.com>
Sent: Friday, August 27, 2021 10:56 AM
To: Jaehoon Chung; Ulf Hansson
Cc: kernel@axis.com; linux-mmc@vger.kernel.org; Mårten Lindahl
Subject: [PATCH] mmc: dw_mmc: Support more time for data read timeouts
For data read transfers a data transfer over timer (dto_timer) is
started to make sure the data command can be completed in cases the Data
Transfer Over (DTO) interrupt does not come. This timer was originally
introduced as a quirk in commit 57e104864bc48 ("mmc: dw_mmc: add quirk
for broken data transfer over scheme"), but is since a while back part
of the running code.
The dto timer picks the DATA_TIMEOUT value in the TMOUT register for its
timeout, which will give a max timeout of approximately 84 + (10 spare)
milliseconds on a 200MHz clock. But this register is not intended to be
used like that, since it is a counter for data read timeouts (DRTO) and
response timeouts (RTO), which will result in error interrupts in case
of data read and response delays.
The TMOUT register is always set with a full value for every transfer,
which according to the manual (and with 200MHz clock) will give a full
DRTO of:
((TMOUT[10:8] -1) * 0xFFFFFF + TMOUT[31:11] * 8) / 200000000 => ~587 ms
But as the same register is used for the dto_timer, the dto_timer will
always have a fixed timeout.
Instead of always setting a fixed value in TMOUT register, we can use
data->timeout_ns for the DRTO interrupts that actually matches what was
provided per requested command. Likewise we can also use timeout_ns for
the dto_timer, which will allow a max timeout of 587 ms, instead of the
fixed 94 ms. Furthermore, if a data error interrupt comes, it shouldn't
be necessary to wait for the dto_timer before we finish the command, but
instead we can handle it in the interrupt handler.
Lets fix this. In most cases data->timeout_ns values are given, but in
case it is not given, the maximum (default) timeout for the dto_timer,
and the DRTO, is set to approximately 587 ms.
Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
drivers/mmc/host/dw_mmc.c | 108 ++++++++++++++++++++++----------------
1 file changed, 63 insertions(+), 45 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index c3229d8c7041..0762d95293d4 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -52,6 +52,7 @@
#define DW_MCI_FREQ_MAX 200000000 /* unit: HZ */
#define DW_MCI_FREQ_MIN 100000 /* unit: HZ */
+#define DW_MCI_DATA_TMOUT_NS_MAX 587202490
#define IDMAC_INT_CLR (SDMMC_IDMAC_INT_AI | SDMMC_IDMAC_INT_NI | \
SDMMC_IDMAC_INT_CES | SDMMC_IDMAC_INT_DU | \
@@ -390,6 +391,23 @@ static inline void dw_mci_set_cto(struct dw_mci *host)
spin_unlock_irqrestore(&host->irq_lock, irqflags);
}
+static void dw_mci_set_dto(struct dw_mci *host, u32 timeout_ns)
+{
+ unsigned int dto_ns;
+ unsigned long irqflags;
+
+ if (!timeout_ns || timeout_ns > DW_MCI_DATA_TMOUT_NS_MAX)
+ dto_ns = DW_MCI_DATA_TMOUT_NS_MAX;
+ else
+ dto_ns = timeout_ns;
+
+ spin_lock_irqsave(&host->irq_lock, irqflags);
+ if (!test_bit(EVENT_DATA_COMPLETE, &host->pending_events))
+ mod_timer(&host->dto_timer,
+ jiffies + nsecs_to_jiffies(dto_ns));
+ spin_unlock_irqrestore(&host->irq_lock, irqflags);
+}
+
static void dw_mci_start_command(struct dw_mci *host,
struct mmc_command *cmd, u32 cmd_flags)
{
@@ -1144,9 +1162,10 @@ static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data)
host->sg = NULL;
host->data = data;
- if (data->flags & MMC_DATA_READ)
+ if (data->flags & MMC_DATA_READ) {
host->dir_status = DW_MCI_RECV_STATUS;
- else
+ dw_mci_set_dto(host, data->timeout_ns);
+ } else
host->dir_status = DW_MCI_SEND_STATUS;
dw_mci_ctrl_thld(host, data);
@@ -1277,6 +1296,36 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
mci_writel(host, CTYPE, (slot->ctype << slot->id));
}
+static void dw_mci_set_data_timeout(struct dw_mci *host, u32 timeout_ns)
+{
+ u32 timeout, freq_mhz, tmp, tmout;
+
+ if (!timeout_ns || timeout_ns > DW_MCI_DATA_TMOUT_NS_MAX) {
+ /* Timeout (maximum) */
+ mci_writel(host, TMOUT, 0xFFFFFFFF);
+ return;
+ }
+
+ timeout = timeout_ns / NSEC_PER_USEC;
+ freq_mhz = host->bus_hz / NSEC_PER_MSEC;
+
+ /* TMOUT[7:0] (RESPONSE_TIMEOUT) */
+ tmout = 0xFF;
+
+ /* TMOUT[10:8] (DATA_TIMEOUT) */
+ tmp = ((timeout * freq_mhz) / 0xFFFFFF) + 1;
+ tmout |= (tmp & 0x7) << 8;
+
+ /* TMOUT[31:11] (DATA_TIMEOUT) */
+ tmp = ((tmp - 1) * 0xFFFFFF) / freq_mhz;
+ tmp = (timeout - tmp) * freq_mhz / 8;
+ tmout |= (tmp & 0x1FFFFF) << 11;
+
+ mci_writel(host, TMOUT, tmout);
+ dev_dbg(host->dev, "timeout_ns: %u => TMOUT[31:8]: 0x%08x",
+ timeout_ns, tmout);
+}
+
static void __dw_mci_start_request(struct dw_mci *host,
struct dw_mci_slot *slot,
struct mmc_command *cmd)
@@ -1297,7 +1346,7 @@ static void __dw_mci_start_request(struct dw_mci *host,
data = cmd->data;
if (data) {
- mci_writel(host, TMOUT, 0xFFFFFFFF);
+ dw_mci_set_data_timeout(host, data->timeout_ns);
mci_writel(host, BYTCNT, data->blksz*data->blocks);
mci_writel(host, BLKSIZ, data->blksz);
}
@@ -1897,31 +1946,6 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
return data->error;
}
-static void dw_mci_set_drto(struct dw_mci *host)
-{
- unsigned int drto_clks;
- unsigned int drto_div;
- unsigned int drto_ms;
- unsigned long irqflags;
-
- drto_clks = mci_readl(host, TMOUT) >> 8;
- drto_div = (mci_readl(host, CLKDIV) & 0xff) * 2;
- if (drto_div == 0)
- drto_div = 1;
-
- drto_ms = DIV_ROUND_UP_ULL((u64)MSEC_PER_SEC * drto_clks * drto_div,
- host->bus_hz);
-
- /* add a bit spare time */
- drto_ms += 10;
-
- spin_lock_irqsave(&host->irq_lock, irqflags);
- if (!test_bit(EVENT_DATA_COMPLETE, &host->pending_events))
- mod_timer(&host->dto_timer,
- jiffies + msecs_to_jiffies(drto_ms));
- spin_unlock_irqrestore(&host->irq_lock, irqflags);
-}
-
static bool dw_mci_clear_pending_cmd_complete(struct dw_mci *host)
{
if (!test_bit(EVENT_CMD_COMPLETE, &host->pending_events))
@@ -2052,15 +2076,8 @@ static void dw_mci_tasklet_func(struct tasklet_struct *t)
}
if (!test_and_clear_bit(EVENT_XFER_COMPLETE,
- &host->pending_events)) {
- /*
- * If all data-related interrupts don't come
- * within the given time in reading data state.
- */
- if (host->dir_status == DW_MCI_RECV_STATUS)
- dw_mci_set_drto(host);
+ &host->pending_events))
break;
- }
set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
@@ -2091,16 +2108,8 @@ static void dw_mci_tasklet_func(struct tasklet_struct *t)
fallthrough;
case STATE_DATA_BUSY:
- if (!dw_mci_clear_pending_data_complete(host)) {
- /*
- * If data error interrupt comes but data over
- * interrupt doesn't come within the given time.
- * in reading data state.
- */
- if (host->dir_status == DW_MCI_RECV_STATUS)
- dw_mci_set_drto(host);
+ if (!dw_mci_clear_pending_data_complete(host))
break;
- }
host->data = NULL;
set_bit(EVENT_DATA_COMPLETE, &host->completed_events);
@@ -2649,12 +2658,21 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
}
if (pending & DW_MCI_DATA_ERROR_FLAGS) {
+ spin_lock(&host->irq_lock);
+
+ del_timer(&host->dto_timer);
+
/* if there is an error report DATA_ERROR */
mci_writel(host, RINTSTS, DW_MCI_DATA_ERROR_FLAGS);
host->data_status = pending;
smp_wmb(); /* drain writebuffer */
set_bit(EVENT_DATA_ERROR, &host->pending_events);
+
+ /* In case of error, we cannot expect a DTO */
+ set_bit(EVENT_DATA_COMPLETE, &host->pending_events);
tasklet_schedule(&host->tasklet);
+
+ spin_unlock(&host->irq_lock);
}
if (pending & SDMMC_INT_DATA_OVER) {
--
2.20.1
=
Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-17 7:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-27 8:56 [PATCH] mmc: dw_mmc: Support more time for data read timeouts Mårten Lindahl
2021-09-06 16:36 ` Ulf Hansson
2021-09-10 9:28 ` Vincent Whitchurch
2021-09-10 20:53 ` Doug Anderson
2021-09-16 10:45 ` Marten Lindahl
2021-09-17 7:13 ` Christian Löhle
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).