[v2] mmc: dw_mmc: add quirk for broken data transfer over scheme
diff mbox

Message ID 1416903008-4160-1-git-send-email-addy.ke@rock-chips.com
State New, archived
Headers show

Commit Message

addy ke Nov. 25, 2014, 8:10 a.m. UTC
This patch add a new quirk to add a s/w timer to notify the driver
to terminate current transfer and report a data timeout to the core,
if DTO interrupt does NOT come within the given time.

dw_mmc call mmc_request_done func to finish transfer depends on
DTO interrupt. If DTO interrupt does not come in sending data state,
the current transfer will be blocked.

But this case really exists, when driver reads tuning data from
card on RK3288-pink2 board. I measured waveforms by oscilloscope
and found that card clock was always on and data lines were always
holded high level in sending data state.

We got the reply from synopsys:
There are two counters but both use the same value of [31:8] bits.
Data timeout counter doesn't wait for stop clock and you should get
DRTO even when the clock is not stopped.
Host Starvation timeout counter is triggered with stop clock condition.

This means that host should get DRTO and DTO interrupt.

But we really don't get any data-related interrupt in RK3X SoCs.
And driver can't get data transfer state, it can do nothing but wait for.

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
- fix some typo.
- remove extra timeout value (250ms).
- remove dw_mci_dto_start_monitor func.
- use "broken-dto" for new quirk and change Subject for it.

 drivers/mmc/host/dw_mmc.c  | 44 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/mmc/dw_mmc.h |  5 +++++
 2 files changed, 48 insertions(+), 1 deletion(-)

Comments

Doug Anderson Nov. 26, 2014, 10:46 p.m. UTC | #1
Hi,

On Tue, Nov 25, 2014 at 12:10 AM, Addy Ke <addy.ke@rock-chips.com> wrote:
> This patch add a new quirk to add a s/w timer to notify the driver
> to terminate current transfer and report a data timeout to the core,
> if DTO interrupt does NOT come within the given time.
>
> dw_mmc call mmc_request_done func to finish transfer depends on
> DTO interrupt. If DTO interrupt does not come in sending data state,
> the current transfer will be blocked.
>
> But this case really exists, when driver reads tuning data from
> card on RK3288-pink2 board. I measured waveforms by oscilloscope
> and found that card clock was always on and data lines were always
> holded high level in sending data state.
>
> We got the reply from synopsys:
> There are two counters but both use the same value of [31:8] bits.
> Data timeout counter doesn't wait for stop clock and you should get
> DRTO even when the clock is not stopped.
> Host Starvation timeout counter is triggered with stop clock condition.
>
> This means that host should get DRTO and DTO interrupt.
>
> But we really don't get any data-related interrupt in RK3X SoCs.
> And driver can't get data transfer state, it can do nothing but wait for.

Have you asked someone on your IC team to confirm this is an SoC
errata on your SoC?  ...or is there something else we could be doing
wrong (overclocking?  jitter in the clock?  bad dividers?) that could
be causing this problem?


>  #ifdef CONFIG_OF
>  static struct dw_mci_of_quirks {
>         char *quirk;
> @@ -2513,6 +2549,9 @@ static struct dw_mci_of_quirks {
>         }, {
>                 .quirk  = "disable-wp",
>                 .id     = DW_MCI_QUIRK_NO_WRITE_PROTECT,
> +       }, {
> +               .quirk  = "broken-dto",
> +               .id     = DW_MCI_QUIRK_BROKEN_DTO,

You're adding a device tree property without any binding.  If you need
to add this please send a patch before this one modifying the device
tree bindings.

...but that brings up the question: do you _really_ need to add a
property?  You already know that all rk3288 SoCs need this and you
already know that you're an rk3288 SoC.  Just add this quirk in the
rk3288 code always and be done with it.  ...and if this is also needed
on other Rockchip parts, add it there too.

-Doug
addy ke Dec. 2, 2014, 7:50 a.m. UTC | #2
Hi,
On 2014/11/27 06:46, Doug Anderson wrote:
> Hi,
> 
> On Tue, Nov 25, 2014 at 12:10 AM, Addy Ke <addy.ke@rock-chips.com> wrote:
>> This patch add a new quirk to add a s/w timer to notify the driver
>> to terminate current transfer and report a data timeout to the core,
>> if DTO interrupt does NOT come within the given time.
>>
>> dw_mmc call mmc_request_done func to finish transfer depends on
>> DTO interrupt. If DTO interrupt does not come in sending data state,
>> the current transfer will be blocked.
>>
>> But this case really exists, when driver reads tuning data from
>> card on RK3288-pink2 board. I measured waveforms by oscilloscope
>> and found that card clock was always on and data lines were always
>> holded high level in sending data state.
>>
>> We got the reply from synopsys:
>> There are two counters but both use the same value of [31:8] bits.
>> Data timeout counter doesn't wait for stop clock and you should get
>> DRTO even when the clock is not stopped.
>> Host Starvation timeout counter is triggered with stop clock condition.
>>
>> This means that host should get DRTO and DTO interrupt.
>>
>> But we really don't get any data-related interrupt in RK3X SoCs.
>> And driver can't get data transfer state, it can do nothing but wait for.
> 
> Have you asked someone on your IC team to confirm this is an SoC
> errata on your SoC?  ...or is there something else we could be doing
> wrong (overclocking?  jitter in the clock?  bad dividers?) that could
> be causing this problem?
> 
> 
>>  #ifdef CONFIG_OF
>>  static struct dw_mci_of_quirks {
>>         char *quirk;
>> @@ -2513,6 +2549,9 @@ static struct dw_mci_of_quirks {
>>         }, {
>>                 .quirk  = "disable-wp",
>>                 .id     = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>> +       }, {
>> +               .quirk  = "broken-dto",
>> +               .id     = DW_MCI_QUIRK_BROKEN_DTO,
> 
> You're adding a device tree property without any binding.  If you need
> to add this please send a patch before this one modifying the device
> tree bindings.
> 
> ...but that brings up the question: do you _really_ need to add a
> property?  You already know that all rk3288 SoCs need this and you
> already know that you're an rk3288 SoC.  Just add this quirk in the
> rk3288 code always and be done with it.  ...and if this is also needed
> on other Rockchip parts, add it there too.
> 
> -Doug

We don't know why we have this problem,
but this problem is really exist, and we need patch to fix this problem now.
I will post a follow up change when we find the root cause.

And there is a little probability of this problem on RK SoC, such as RK3188, RK3066,
when worse card inserted in.
Maybe the other SoCs have the similar problem.

So I will add this quirk in rockchip code(dw_mmc-rockchip.c) as follows:
static int dw_mci_rockchip_parse_dt(struct dw_mci *host)
{
	host->quirk |= DW_MCI_QUIRK_BROKEN_DTO;

	return 0;
}

......
	.parse_dt = dw_mci_rockchip_parse_dt,
......

is right?

> 
> 
>
Doug Anderson Dec. 2, 2014, 5:47 p.m. UTC | #3
Addy,

On Mon, Dec 1, 2014 at 11:50 PM, addy ke <addy.ke@rock-chips.com> wrote:
> We don't know why we have this problem,
> but this problem is really exist, and we need patch to fix this problem now.
> I will post a follow up change when we find the root cause.

To me that seems reasonable.  Certainly I would prefer to have this
patch while waiting for the root cause, but it's up to Ulf.


> And there is a little probability of this problem on RK SoC, such as RK3188, RK3066,
> when worse card inserted in.
> Maybe the other SoCs have the similar problem.
>
> So I will add this quirk in rockchip code(dw_mmc-rockchip.c) as follows:
> static int dw_mci_rockchip_parse_dt(struct dw_mci *host)
> {
>         host->quirk |= DW_MCI_QUIRK_BROKEN_DTO;
>
>         return 0;
> }
>
> ......
>         .parse_dt = dw_mci_rockchip_parse_dt,
> ......
>
> is right?

When you added "sdio_id0" you got feedback that you should use
dw_mci_rockchip_init().  Why not do the same thing here?

Patch
diff mbox

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index b4c3044..bc09f50 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1457,6 +1457,8 @@  static void dw_mci_tasklet_func(unsigned long priv)
 	enum dw_mci_state state;
 	enum dw_mci_state prev_state;
 	unsigned int err;
+	unsigned int drto_clks;
+	unsigned int drto_ms;
 
 	spin_lock(&host->lock);
 
@@ -1522,8 +1524,17 @@  static void dw_mci_tasklet_func(unsigned long priv)
 			}
 
 			if (!test_and_clear_bit(EVENT_XFER_COMPLETE,
-						&host->pending_events))
+						&host->pending_events)) {
+				if (host->quirks & DW_MCI_QUIRK_BROKEN_DTO) {
+					drto_clks = mci_readl(host, TMOUT) >> 8;
+					drto_ms = DIV_ROUND_UP(drto_clks * 1000,
+							       host->bus_hz);
+
+					mod_timer(&host->dto_timer, jiffies +
+						msecs_to_jiffies(drto_ms));
+				}
 				break;
+			}
 
 			set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
 
@@ -2115,6 +2126,9 @@  static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
 		}
 
 		if (pending & SDMMC_INT_DATA_OVER) {
+			if (host->quirks & DW_MCI_QUIRK_BROKEN_DTO)
+				del_timer(&host->dto_timer);
+
 			mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
 			if (!host->data_status)
 				host->data_status = pending;
@@ -2502,6 +2516,28 @@  ciu_out:
 	return ret;
 }
 
+static void dw_mci_dto_timer(unsigned long arg)
+{
+	struct dw_mci *host = (struct dw_mci *)arg;
+
+	switch (host->state) {
+	case STATE_SENDING_DATA:
+	case STATE_DATA_BUSY:
+		/*
+		* If DTO interrupt does NOT come in sending data state,
+		* we should notify the driver to terminate current transfer
+		* and report a data timeout to the core.
+		*/
+		host->data_status = SDMMC_INT_DRTO;
+		set_bit(EVENT_DATA_ERROR, &host->pending_events);
+		set_bit(EVENT_DATA_COMPLETE, &host->pending_events);
+		tasklet_schedule(&host->tasklet);
+		break;
+	default:
+		break;
+	}
+}
+
 #ifdef CONFIG_OF
 static struct dw_mci_of_quirks {
 	char *quirk;
@@ -2513,6 +2549,9 @@  static struct dw_mci_of_quirks {
 	}, {
 		.quirk	= "disable-wp",
 		.id	= DW_MCI_QUIRK_NO_WRITE_PROTECT,
+	}, {
+		.quirk	= "broken-dto",
+		.id	= DW_MCI_QUIRK_BROKEN_DTO,
 	},
 };
 
@@ -2654,6 +2693,9 @@  int dw_mci_probe(struct dw_mci *host)
 
 	spin_lock_init(&host->lock);
 	INIT_LIST_HEAD(&host->queue);
+	if (host->quirks & DW_MCI_QUIRK_BROKEN_DTO)
+		setup_timer(&host->dto_timer,
+			    dw_mci_dto_timer, (unsigned long)host);
 
 	/*
 	 * Get the host data width - this assumes that HCON has been set with
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 42b724e..ff9bd1e 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -98,6 +98,7 @@  struct mmc_data;
  * @irq_flags: The flags to be passed to request_irq.
  * @irq: The irq value to be passed to request_irq.
  * @sdio_id0: Number of slot0 in the SDIO interrupt registers.
+ * @dto_timer: Timer for broken data transfer over scheme.
  *
  * Locking
  * =======
@@ -196,6 +197,8 @@  struct dw_mci {
 	int			irq;
 
 	int			sdio_id0;
+
+	struct timer_list	dto_timer;
 };
 
 /* DMA ops for Internal/External DMAC interface */
@@ -220,6 +223,8 @@  struct dw_mci_dma_ops {
 #define DW_MCI_QUIRK_BROKEN_CARD_DETECTION	BIT(3)
 /* No write protect */
 #define DW_MCI_QUIRK_NO_WRITE_PROTECT		BIT(4)
+/* Timer for broken data transfer over scheme */
+#define DW_MCI_QUIRK_BROKEN_DTO			BIT(5)
 
 /* Slot level quirks */
 /* This slot has no write protect */