[PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[Addy Ke]

From: Addy <addy.ke@rock-chips.com>

This patch add a new quirk to notify the driver to teminate
current transfer and report a data timeout to the core,
if data over interrupt does NOT come within the given time.

dw_mmc call mmc_request_done func to finish transfer depends on
data over interrupt. If data over 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. This is the cause that
card does NOT send data to host.

According to synopsys designware databook, the timeout counter is
started only after the card clock is stopped.

So if card clock is always on, data read timeout interrupt will NOT come,
and if data lines are always holded high level, all data-related
interrupt such as start-bit error, data crc error, data over interrupt,
end-bit error, and so on, will NOT come too.

So driver can't get the current state, it can do nothing but wait for.

This patch is based on https://patchwork.kernel.org/patch/5227941/

Signed-off-by: Addy <addy.ke@rock-chips.com>
---
 drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/mmc/dw_mmc.h |  5 +++++
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index b4c3044..3960fc3 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
 	return data->error;
 }
 
+static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
+{
+	unsigned int data_tmout_clks;
+	unsigned int data_tmout_ms;
+
+	data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
+	data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
+
+	mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
+}
+
 static void dw_mci_tasklet_func(unsigned long priv)
 {
 	struct dw_mci *host = (struct dw_mci *)priv;
@@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
+					dw_mci_dto_start_monitor(host);
 				break;
+			}
 
 			set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
 
@@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
 		}
 
 		if (pending & SDMMC_INT_DATA_OVER) {
+			if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
+				del_timer(&host->dto_timer);
+
 			mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
 			if (!host->data_status)
 				host->data_status = pending;
@@ -2502,6 +2519,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 data over interrupt does NOT come in sending data state,
+		* we should notify the driver to teminate 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 +2552,9 @@ static struct dw_mci_of_quirks {
 	}, {
 		.quirk	= "disable-wp",
 		.id	= DW_MCI_QUIRK_NO_WRITE_PROTECT,
+	}, {
+		.quirk	= "dto-timer",
+		.id	= DW_MCI_QUIRK_DTO_TIMER,
 	},
 };
 
@@ -2654,6 +2696,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_DTO_TIMER)
+		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..2477813 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 data over interrupt timeout.
  *
  * 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 data over interrupt timeout */
+#define DW_MCI_QUIRK_DTO_TIMER			BIT(5)
 
 /* Slot level quirks */
 /* This slot has no write protect */
-- 
1.8.3.2

[PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[Addy Ke]

From: Addy <addy.ke@rock-chips.com>

This patch add a new quirk to notify the driver to teminate
current transfer and report a data timeout to the core,
if data over interrupt does NOT come within the given time.

dw_mmc call mmc_request_done func to finish transfer depends on
data over interrupt. If data over 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. This is the cause that
card does NOT send data to host.

According to synopsys designware databook, the timeout counter is
started only after the card clock is stopped.

So if card clock is always on, data read timeout interrupt will NOT come,
and if data lines are always holded high level, all data-related
interrupt such as start-bit error, data crc error, data over interrupt,
end-bit error, and so on, will NOT come too.

So driver can't get the current state, it can do nothing but wait for.

This patch is based on https://patchwork.kernel.org/patch/5227941/

Signed-off-by: Addy <addy.ke@rock-chips.com>
---
 drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/mmc/dw_mmc.h |  5 +++++
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index b4c3044..3960fc3 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
 	return data->error;
 }
 
+static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
+{
+	unsigned int data_tmout_clks;
+	unsigned int data_tmout_ms;
+
+	data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
+	data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
+
+	mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
+}
+
 static void dw_mci_tasklet_func(unsigned long priv)
 {
 	struct dw_mci *host = (struct dw_mci *)priv;
@@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
+					dw_mci_dto_start_monitor(host);
 				break;
+			}
 
 			set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
 
@@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
 		}
 
 		if (pending & SDMMC_INT_DATA_OVER) {
+			if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
+				del_timer(&host->dto_timer);
+
 			mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
 			if (!host->data_status)
 				host->data_status = pending;
@@ -2502,6 +2519,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 data over interrupt does NOT come in sending data state,
+		* we should notify the driver to teminate 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 +2552,9 @@ static struct dw_mci_of_quirks {
 	}, {
 		.quirk	= "disable-wp",
 		.id	= DW_MCI_QUIRK_NO_WRITE_PROTECT,
+	}, {
+		.quirk	= "dto-timer",
+		.id	= DW_MCI_QUIRK_DTO_TIMER,
 	},
 };
 
@@ -2654,6 +2696,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_DTO_TIMER)
+		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..2477813 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 data over interrupt timeout.
  *
  * 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 data over interrupt timeout */
+#define DW_MCI_QUIRK_DTO_TIMER			BIT(5)
 
 /* Slot level quirks */
 /* This slot has no write protect */
-- 
1.8.3.2

Re: [PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[Jaehoon Chung]

Hi, Addy.

Did you use the DW_MCI_QUIRK_IDMAC_DTO?
I'm not sure, but i wonder if you get what result when you use above quirk.

And i will check more this patch at next week.

Thanks for your efforts.

Best Regards,
Jaehoon Chung

On 11/14/2014 10:05 PM, Addy Ke wrote:
> From: Addy <addy.ke@rock-chips.com>
> 
> This patch add a new quirk to notify the driver to teminate
> current transfer and report a data timeout to the core,
> if data over interrupt does NOT come within the given time.
> 
> dw_mmc call mmc_request_done func to finish transfer depends on
> data over interrupt. If data over 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. This is the cause that
> card does NOT send data to host.
> 
> According to synopsys designware databook, the timeout counter is
> started only after the card clock is stopped.
> 
> So if card clock is always on, data read timeout interrupt will NOT come,
> and if data lines are always holded high level, all data-related
> interrupt such as start-bit error, data crc error, data over interrupt,
> end-bit error, and so on, will NOT come too.
> 
> So driver can't get the current state, it can do nothing but wait for.
> 
> This patch is based on https://patchwork.kernel.org/patch/5227941/
> 
> Signed-off-by: Addy <addy.ke@rock-chips.com>
> ---
>  drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/mmc/dw_mmc.h |  5 +++++
>  2 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index b4c3044..3960fc3 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>  	return data->error;
>  }
>  
> +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
> +{
> +	unsigned int data_tmout_clks;
> +	unsigned int data_tmout_ms;
> +
> +	data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
> +	data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
> +
> +	mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
> +}
> +
>  static void dw_mci_tasklet_func(unsigned long priv)
>  {
>  	struct dw_mci *host = (struct dw_mci *)priv;
> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
> +					dw_mci_dto_start_monitor(host);
>  				break;
> +			}
>  
>  			set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>  
> @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>  		}
>  
>  		if (pending & SDMMC_INT_DATA_OVER) {
> +			if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
> +				del_timer(&host->dto_timer);
> +
>  			mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>  			if (!host->data_status)
>  				host->data_status = pending;
> @@ -2502,6 +2519,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 data over interrupt does NOT come in sending data state,
> +		* we should notify the driver to teminate 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 +2552,9 @@ static struct dw_mci_of_quirks {
>  	}, {
>  		.quirk	= "disable-wp",
>  		.id	= DW_MCI_QUIRK_NO_WRITE_PROTECT,
> +	}, {
> +		.quirk	= "dto-timer",
> +		.id	= DW_MCI_QUIRK_DTO_TIMER,
>  	},
>  };
>  
> @@ -2654,6 +2696,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_DTO_TIMER)
> +		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..2477813 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 data over interrupt timeout.
>   *
>   * 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 data over interrupt timeout */
> +#define DW_MCI_QUIRK_DTO_TIMER			BIT(5)
>  
>  /* Slot level quirks */
>  /* This slot has no write protect */
> 

[PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[Jaehoon Chung]

Hi, Addy.

Did you use the DW_MCI_QUIRK_IDMAC_DTO?
I'm not sure, but i wonder if you get what result when you use above quirk.

And i will check more this patch at next week.

Thanks for your efforts.

Best Regards,
Jaehoon Chung

On 11/14/2014 10:05 PM, Addy Ke wrote:
> From: Addy <addy.ke@rock-chips.com>
> 
> This patch add a new quirk to notify the driver to teminate
> current transfer and report a data timeout to the core,
> if data over interrupt does NOT come within the given time.
> 
> dw_mmc call mmc_request_done func to finish transfer depends on
> data over interrupt. If data over 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. This is the cause that
> card does NOT send data to host.
> 
> According to synopsys designware databook, the timeout counter is
> started only after the card clock is stopped.
> 
> So if card clock is always on, data read timeout interrupt will NOT come,
> and if data lines are always holded high level, all data-related
> interrupt such as start-bit error, data crc error, data over interrupt,
> end-bit error, and so on, will NOT come too.
> 
> So driver can't get the current state, it can do nothing but wait for.
> 
> This patch is based on https://patchwork.kernel.org/patch/5227941/
> 
> Signed-off-by: Addy <addy.ke@rock-chips.com>
> ---
>  drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/mmc/dw_mmc.h |  5 +++++
>  2 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index b4c3044..3960fc3 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>  	return data->error;
>  }
>  
> +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
> +{
> +	unsigned int data_tmout_clks;
> +	unsigned int data_tmout_ms;
> +
> +	data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
> +	data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
> +
> +	mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
> +}
> +
>  static void dw_mci_tasklet_func(unsigned long priv)
>  {
>  	struct dw_mci *host = (struct dw_mci *)priv;
> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
> +					dw_mci_dto_start_monitor(host);
>  				break;
> +			}
>  
>  			set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>  
> @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>  		}
>  
>  		if (pending & SDMMC_INT_DATA_OVER) {
> +			if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
> +				del_timer(&host->dto_timer);
> +
>  			mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>  			if (!host->data_status)
>  				host->data_status = pending;
> @@ -2502,6 +2519,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 data over interrupt does NOT come in sending data state,
> +		* we should notify the driver to teminate 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 +2552,9 @@ static struct dw_mci_of_quirks {
>  	}, {
>  		.quirk	= "disable-wp",
>  		.id	= DW_MCI_QUIRK_NO_WRITE_PROTECT,
> +	}, {
> +		.quirk	= "dto-timer",
> +		.id	= DW_MCI_QUIRK_DTO_TIMER,
>  	},
>  };
>  
> @@ -2654,6 +2696,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_DTO_TIMER)
> +		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..2477813 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 data over interrupt timeout.
>   *
>   * 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 data over interrupt timeout */
> +#define DW_MCI_QUIRK_DTO_TIMER			BIT(5)
>  
>  /* Slot level quirks */
>  /* This slot has no write protect */
> 

Re: [PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[Addy]

On 2014年11月14日 21:18, Jaehoon Chung wrote:
> Hi, Addy.
>
> Did you use the DW_MCI_QUIRK_IDMAC_DTO?
> I'm not sure, but i wonder if you get what result when you use above quirk.

DW_MCI_QUIRK_IDMAC_DTO is only for version2.0 or below.
         /*
          * DTO fix - version 2.10a and below, and only if internal DMA
          * is configured.
          */
         if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) {
                 if (!pending &&
                     ((mci_readl(host, STATUS) >> 17) & 0x1fff))
                         pending |= SDMMC_INT_DATA_OVER;
         }

It meams that if interrupt comes, but pending = 0 && FIFO_COUNT(bit17-29) !=0,
then force to set SDMMC_INT_DATA_OVER.
But in our case, FIFO_COUNT = 0 (STATUS register value is 0xad06). This is
because that the card does not send data to host. So there is no interrupts come,
and interrupt handle function(dw_mci_interrupt) will not be called. So we need a
timer to handle this case.

So I think  SDMMC_INT_DATA_OVER is not suitable for this case, and we need a new
quirk.

>
> And i will check more this patch at next week.
>
> Thanks for your efforts.
>
> Best Regards,
> Jaehoon Chung
>
> On 11/14/2014 10:05 PM, Addy Ke wrote:
>> From: Addy <addy.ke@rock-chips.com>
>>
>> This patch add a new quirk to notify the driver to teminate
>> current transfer and report a data timeout to the core,
>> if data over interrupt does NOT come within the given time.
>>
>> dw_mmc call mmc_request_done func to finish transfer depends on
>> data over interrupt. If data over 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. This is the cause that
>> card does NOT send data to host.
>>
>> According to synopsys designware databook, the timeout counter is
>> started only after the card clock is stopped.
>>
>> So if card clock is always on, data read timeout interrupt will NOT come,
>> and if data lines are always holded high level, all data-related
>> interrupt such as start-bit error, data crc error, data over interrupt,
>> end-bit error, and so on, will NOT come too.
>>
>> So driver can't get the current state, it can do nothing but wait for.
>>
>> This patch is based on https://patchwork.kernel.org/patch/5227941/
>>
>> Signed-off-by: Addy <addy.ke@rock-chips.com>
>> ---
>>   drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>>   include/linux/mmc/dw_mmc.h |  5 +++++
>>   2 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index b4c3044..3960fc3 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>>   	return data->error;
>>   }
>>   
>> +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
>> +{
>> +	unsigned int data_tmout_clks;
>> +	unsigned int data_tmout_ms;
>> +
>> +	data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>> +	data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
>> +
>> +	mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
>> +}
>> +
>>   static void dw_mci_tasklet_func(unsigned long priv)
>>   {
>>   	struct dw_mci *host = (struct dw_mci *)priv;
>> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
>> +					dw_mci_dto_start_monitor(host);
>>   				break;
>> +			}
>>   
>>   			set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>>   
>> @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>   		}
>>   
>>   		if (pending & SDMMC_INT_DATA_OVER) {
>> +			if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
>> +				del_timer(&host->dto_timer);
>> +
>>   			mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>>   			if (!host->data_status)
>>   				host->data_status = pending;
>> @@ -2502,6 +2519,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 data over interrupt does NOT come in sending data state,
>> +		* we should notify the driver to teminate 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 +2552,9 @@ static struct dw_mci_of_quirks {
>>   	}, {
>>   		.quirk	= "disable-wp",
>>   		.id	= DW_MCI_QUIRK_NO_WRITE_PROTECT,
>> +	}, {
>> +		.quirk	= "dto-timer",
>> +		.id	= DW_MCI_QUIRK_DTO_TIMER,
>>   	},
>>   };
>>   
>> @@ -2654,6 +2696,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_DTO_TIMER)
>> +		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..2477813 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 data over interrupt timeout.
>>    *
>>    * 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 data over interrupt timeout */
>> +#define DW_MCI_QUIRK_DTO_TIMER			BIT(5)
>>   
>>   /* Slot level quirks */
>>   /* This slot has no write protect */
>>
>
>
>

[PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[Addy]

On 2014?11?14? 21:18, Jaehoon Chung wrote:
> Hi, Addy.
>
> Did you use the DW_MCI_QUIRK_IDMAC_DTO?
> I'm not sure, but i wonder if you get what result when you use above quirk.

DW_MCI_QUIRK_IDMAC_DTO is only for version2.0 or below.
         /*
          * DTO fix - version 2.10a and below, and only if internal DMA
          * is configured.
          */
         if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) {
                 if (!pending &&
                     ((mci_readl(host, STATUS) >> 17) & 0x1fff))
                         pending |= SDMMC_INT_DATA_OVER;
         }

It meams that if interrupt comes, but pending = 0 && FIFO_COUNT(bit17-29) !=0,
then force to set SDMMC_INT_DATA_OVER.
But in our case, FIFO_COUNT = 0 (STATUS register value is 0xad06). This is
because that the card does not send data to host. So there is no interrupts come,
and interrupt handle function(dw_mci_interrupt) will not be called. So we need a
timer to handle this case.

So I think  SDMMC_INT_DATA_OVER is not suitable for this case, and we need a new
quirk.

>
> And i will check more this patch at next week.
>
> Thanks for your efforts.
>
> Best Regards,
> Jaehoon Chung
>
> On 11/14/2014 10:05 PM, Addy Ke wrote:
>> From: Addy <addy.ke@rock-chips.com>
>>
>> This patch add a new quirk to notify the driver to teminate
>> current transfer and report a data timeout to the core,
>> if data over interrupt does NOT come within the given time.
>>
>> dw_mmc call mmc_request_done func to finish transfer depends on
>> data over interrupt. If data over 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. This is the cause that
>> card does NOT send data to host.
>>
>> According to synopsys designware databook, the timeout counter is
>> started only after the card clock is stopped.
>>
>> So if card clock is always on, data read timeout interrupt will NOT come,
>> and if data lines are always holded high level, all data-related
>> interrupt such as start-bit error, data crc error, data over interrupt,
>> end-bit error, and so on, will NOT come too.
>>
>> So driver can't get the current state, it can do nothing but wait for.
>>
>> This patch is based on https://patchwork.kernel.org/patch/5227941/
>>
>> Signed-off-by: Addy <addy.ke@rock-chips.com>
>> ---
>>   drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>>   include/linux/mmc/dw_mmc.h |  5 +++++
>>   2 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index b4c3044..3960fc3 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>>   	return data->error;
>>   }
>>   
>> +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
>> +{
>> +	unsigned int data_tmout_clks;
>> +	unsigned int data_tmout_ms;
>> +
>> +	data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>> +	data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
>> +
>> +	mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
>> +}
>> +
>>   static void dw_mci_tasklet_func(unsigned long priv)
>>   {
>>   	struct dw_mci *host = (struct dw_mci *)priv;
>> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
>> +					dw_mci_dto_start_monitor(host);
>>   				break;
>> +			}
>>   
>>   			set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>>   
>> @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>   		}
>>   
>>   		if (pending & SDMMC_INT_DATA_OVER) {
>> +			if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
>> +				del_timer(&host->dto_timer);
>> +
>>   			mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>>   			if (!host->data_status)
>>   				host->data_status = pending;
>> @@ -2502,6 +2519,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 data over interrupt does NOT come in sending data state,
>> +		* we should notify the driver to teminate 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 +2552,9 @@ static struct dw_mci_of_quirks {
>>   	}, {
>>   		.quirk	= "disable-wp",
>>   		.id	= DW_MCI_QUIRK_NO_WRITE_PROTECT,
>> +	}, {
>> +		.quirk	= "dto-timer",
>> +		.id	= DW_MCI_QUIRK_DTO_TIMER,
>>   	},
>>   };
>>   
>> @@ -2654,6 +2696,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_DTO_TIMER)
>> +		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..2477813 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 data over interrupt timeout.
>>    *
>>    * 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 data over interrupt timeout */
>> +#define DW_MCI_QUIRK_DTO_TIMER			BIT(5)
>>   
>>   /* Slot level quirks */
>>   /* This slot has no write protect */
>>
>
>
>

Re: [PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[Jaehoon Chung]

Hi, Addy.

On 11/18/2014 09:32 AM, Addy wrote:
> 
> On 2014年11月14日 21:18, Jaehoon Chung wrote:
>> Hi, Addy.
>>
>> Did you use the DW_MCI_QUIRK_IDMAC_DTO?
>> I'm not sure, but i wonder if you get what result when you use above quirk.
> 
> DW_MCI_QUIRK_IDMAC_DTO is only for version2.0 or below.
>         /*
>          * DTO fix - version 2.10a and below, and only if internal DMA
>          * is configured.
>          */
>         if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) {
>                 if (!pending &&
>                     ((mci_readl(host, STATUS) >> 17) & 0x1fff))
>                         pending |= SDMMC_INT_DATA_OVER;
>         }
> 
> It meams that if interrupt comes, but pending = 0 && FIFO_COUNT(bit17-29) !=0,
> then force to set SDMMC_INT_DATA_OVER.
> But in our case, FIFO_COUNT = 0 (STATUS register value is 0xad06). This is
> because that the card does not send data to host. So there is no interrupts come,
> and interrupt handle function(dw_mci_interrupt) will not be called. So we need a
> timer to handle this case.
> 
> So I think  SDMMC_INT_DATA_OVER is not suitable for this case, and we need a new
> quirk.
> 
>>
>> And i will check more this patch at next week.
>>
>> Thanks for your efforts.
>>
>> Best Regards,
>> Jaehoon Chung
>>
>> On 11/14/2014 10:05 PM, Addy Ke wrote:
>>> From: Addy <addy.ke@rock-chips.com>
>>>
>>> This patch add a new quirk to notify the driver to teminate
>>> current transfer and report a data timeout to the core,
>>> if data over interrupt does NOT come within the given time.
>>>
>>> dw_mmc call mmc_request_done func to finish transfer depends on
>>> data over interrupt. If data over 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. This is the cause that
>>> card does NOT send data to host.
>>>
>>> According to synopsys designware databook, the timeout counter is
>>> started only after the card clock is stopped.
>>>
>>> So if card clock is always on, data read timeout interrupt will NOT come,
>>> and if data lines are always holded high level, all data-related
>>> interrupt such as start-bit error, data crc error, data over interrupt,
>>> end-bit error, and so on, will NOT come too.
>>>
>>> So driver can't get the current state, it can do nothing but wait for.
>>>
>>> This patch is based on https://patchwork.kernel.org/patch/5227941/
>>>
>>> Signed-off-by: Addy <addy.ke@rock-chips.com>
>>> ---
>>>   drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>>>   include/linux/mmc/dw_mmc.h |  5 +++++
>>>   2 files changed, 51 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index b4c3044..3960fc3 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>>>       return data->error;
>>>   }
>>>   +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
>>> +{
>>> +    unsigned int data_tmout_clks;
>>> +    unsigned int data_tmout_ms;
>>> +
>>> +    data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>>> +    data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;

What's 250? And how about using the DIV_ROUND_UP? 

>>> +
>>> +    mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
>>> +}
>>> +
>>>   static void dw_mci_tasklet_func(unsigned long priv)
>>>   {
>>>       struct dw_mci *host = (struct dw_mci *)priv;
>>> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
>>> +                    dw_mci_dto_start_monitor(host);

if timer is starting at only here, dw_mci_dto_start_monitor() doesn't need.

>>>                   break;
>>> +            }
>>>                 set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>>>   @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>>           }
>>>             if (pending & SDMMC_INT_DATA_OVER) {
>>> +            if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
>>> +                del_timer(&host->dto_timer);
>>> +
>>>               mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>>>               if (!host->data_status)
>>>                   host->data_status = pending;
>>> @@ -2502,6 +2519,28 @@ ciu_out:
>>>       return ret;
>>>   }
>>>   +static void dw_mci_dto_timer(unsigned long arg)
>>> +{
>>> +    struct dw_mci *host = (struct dw_mci *)arg;

I prefer to use the "data" instead of "arg"

>>> +
>>> +    switch (host->state) {
>>> +    case STATE_SENDING_DATA:
>>> +    case STATE_DATA_BUSY:
>>> +        /*
>>> +        * If data over interrupt does NOT come in sending data state,
>>> +        * we should notify the driver to teminate current transfer
teminate/terminate?

>>> +        * 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);

Dose it need to set EVENT_DATA_COMPLETE?

>>> +        tasklet_schedule(&host->tasklet);
>>> +        break;
>>> +    default:
>>> +        break;
>>> +    }
>>> +}
>>> +
>>>   #ifdef CONFIG_OF
>>>   static struct dw_mci_of_quirks {
>>>       char *quirk;
>>> @@ -2513,6 +2552,9 @@ static struct dw_mci_of_quirks {
>>>       }, {
>>>           .quirk    = "disable-wp",
>>>           .id    = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>>> +    }, {
>>> +        .quirk    = "dto-timer",
>>> +        .id    = DW_MCI_QUIRK_DTO_TIMER,
>>>       },

Well, this is s/w timer, so i'm not sure this can be merged into dt-file.
If this is generic solution, we can add s/w timer by default. how about?

Best Regards,
Jaehoon Chung

>>>   };
>>>   @@ -2654,6 +2696,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_DTO_TIMER)
>>> +        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..2477813 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 data over interrupt timeout.
>>>    *
>>>    * 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 data over interrupt timeout */
>>> +#define DW_MCI_QUIRK_DTO_TIMER            BIT(5)
>>>     /* Slot level quirks */
>>>   /* This slot has no write protect */
>>>
>>
>>
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[Jaehoon Chung]

Hi, Addy.

On 11/18/2014 09:32 AM, Addy wrote:
> 
> On 2014?11?14? 21:18, Jaehoon Chung wrote:
>> Hi, Addy.
>>
>> Did you use the DW_MCI_QUIRK_IDMAC_DTO?
>> I'm not sure, but i wonder if you get what result when you use above quirk.
> 
> DW_MCI_QUIRK_IDMAC_DTO is only for version2.0 or below.
>         /*
>          * DTO fix - version 2.10a and below, and only if internal DMA
>          * is configured.
>          */
>         if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) {
>                 if (!pending &&
>                     ((mci_readl(host, STATUS) >> 17) & 0x1fff))
>                         pending |= SDMMC_INT_DATA_OVER;
>         }
> 
> It meams that if interrupt comes, but pending = 0 && FIFO_COUNT(bit17-29) !=0,
> then force to set SDMMC_INT_DATA_OVER.
> But in our case, FIFO_COUNT = 0 (STATUS register value is 0xad06). This is
> because that the card does not send data to host. So there is no interrupts come,
> and interrupt handle function(dw_mci_interrupt) will not be called. So we need a
> timer to handle this case.
> 
> So I think  SDMMC_INT_DATA_OVER is not suitable for this case, and we need a new
> quirk.
> 
>>
>> And i will check more this patch at next week.
>>
>> Thanks for your efforts.
>>
>> Best Regards,
>> Jaehoon Chung
>>
>> On 11/14/2014 10:05 PM, Addy Ke wrote:
>>> From: Addy <addy.ke@rock-chips.com>
>>>
>>> This patch add a new quirk to notify the driver to teminate
>>> current transfer and report a data timeout to the core,
>>> if data over interrupt does NOT come within the given time.
>>>
>>> dw_mmc call mmc_request_done func to finish transfer depends on
>>> data over interrupt. If data over 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. This is the cause that
>>> card does NOT send data to host.
>>>
>>> According to synopsys designware databook, the timeout counter is
>>> started only after the card clock is stopped.
>>>
>>> So if card clock is always on, data read timeout interrupt will NOT come,
>>> and if data lines are always holded high level, all data-related
>>> interrupt such as start-bit error, data crc error, data over interrupt,
>>> end-bit error, and so on, will NOT come too.
>>>
>>> So driver can't get the current state, it can do nothing but wait for.
>>>
>>> This patch is based on https://patchwork.kernel.org/patch/5227941/
>>>
>>> Signed-off-by: Addy <addy.ke@rock-chips.com>
>>> ---
>>>   drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>>>   include/linux/mmc/dw_mmc.h |  5 +++++
>>>   2 files changed, 51 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index b4c3044..3960fc3 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>>>       return data->error;
>>>   }
>>>   +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
>>> +{
>>> +    unsigned int data_tmout_clks;
>>> +    unsigned int data_tmout_ms;
>>> +
>>> +    data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>>> +    data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;

What's 250? And how about using the DIV_ROUND_UP? 

>>> +
>>> +    mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
>>> +}
>>> +
>>>   static void dw_mci_tasklet_func(unsigned long priv)
>>>   {
>>>       struct dw_mci *host = (struct dw_mci *)priv;
>>> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
>>> +                    dw_mci_dto_start_monitor(host);

if timer is starting at only here, dw_mci_dto_start_monitor() doesn't need.

>>>                   break;
>>> +            }
>>>                 set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>>>   @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>>           }
>>>             if (pending & SDMMC_INT_DATA_OVER) {
>>> +            if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
>>> +                del_timer(&host->dto_timer);
>>> +
>>>               mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>>>               if (!host->data_status)
>>>                   host->data_status = pending;
>>> @@ -2502,6 +2519,28 @@ ciu_out:
>>>       return ret;
>>>   }
>>>   +static void dw_mci_dto_timer(unsigned long arg)
>>> +{
>>> +    struct dw_mci *host = (struct dw_mci *)arg;

I prefer to use the "data" instead of "arg"

>>> +
>>> +    switch (host->state) {
>>> +    case STATE_SENDING_DATA:
>>> +    case STATE_DATA_BUSY:
>>> +        /*
>>> +        * If data over interrupt does NOT come in sending data state,
>>> +        * we should notify the driver to teminate current transfer
teminate/terminate?

>>> +        * 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);

Dose it need to set EVENT_DATA_COMPLETE?

>>> +        tasklet_schedule(&host->tasklet);
>>> +        break;
>>> +    default:
>>> +        break;
>>> +    }
>>> +}
>>> +
>>>   #ifdef CONFIG_OF
>>>   static struct dw_mci_of_quirks {
>>>       char *quirk;
>>> @@ -2513,6 +2552,9 @@ static struct dw_mci_of_quirks {
>>>       }, {
>>>           .quirk    = "disable-wp",
>>>           .id    = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>>> +    }, {
>>> +        .quirk    = "dto-timer",
>>> +        .id    = DW_MCI_QUIRK_DTO_TIMER,
>>>       },

Well, this is s/w timer, so i'm not sure this can be merged into dt-file.
If this is generic solution, we can add s/w timer by default. how about?

Best Regards,
Jaehoon Chung

>>>   };
>>>   @@ -2654,6 +2696,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_DTO_TIMER)
>>> +        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..2477813 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 data over interrupt timeout.
>>>    *
>>>    * 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 data over interrupt timeout */
>>> +#define DW_MCI_QUIRK_DTO_TIMER            BIT(5)
>>>     /* Slot level quirks */
>>>   /* This slot has no write protect */
>>>
>>
>>
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[addy ke]

Hi Jaehoon

On 2014/11/19 09:22, Jaehoon Chung Wrote:
> Hi, Addy.
> 
> On 11/18/2014 09:32 AM, Addy wrote:
>>
>> On 2014年11月14日 21:18, Jaehoon Chung wrote:
>>> Hi, Addy.
>>>
>>> Did you use the DW_MCI_QUIRK_IDMAC_DTO?
>>> I'm not sure, but i wonder if you get what result when you use above quirk.
>>
>> DW_MCI_QUIRK_IDMAC_DTO is only for version2.0 or below.
>>         /*
>>          * DTO fix - version 2.10a and below, and only if internal DMA
>>          * is configured.
>>          */
>>         if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) {
>>                 if (!pending &&
>>                     ((mci_readl(host, STATUS) >> 17) & 0x1fff))
>>                         pending |= SDMMC_INT_DATA_OVER;
>>         }
>>
>> It meams that if interrupt comes, but pending = 0 && FIFO_COUNT(bit17-29) !=0,
>> then force to set SDMMC_INT_DATA_OVER.
>> But in our case, FIFO_COUNT = 0 (STATUS register value is 0xad06). This is
>> because that the card does not send data to host. So there is no interrupts come,
>> and interrupt handle function(dw_mci_interrupt) will not be called. So we need a
>> timer to handle this case.
>>
>> So I think  SDMMC_INT_DATA_OVER is not suitable for this case, and we need a new
>> quirk.
>>
>>>
>>> And i will check more this patch at next week.
>>>
>>> Thanks for your efforts.
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>> On 11/14/2014 10:05 PM, Addy Ke wrote:
>>>> From: Addy <addy.ke@rock-chips.com>
>>>>
>>>> This patch add a new quirk to notify the driver to teminate
>>>> current transfer and report a data timeout to the core,
>>>> if data over interrupt does NOT come within the given time.
>>>>
>>>> dw_mmc call mmc_request_done func to finish transfer depends on
>>>> data over interrupt. If data over 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. This is the cause that
>>>> card does NOT send data to host.
>>>>
>>>> According to synopsys designware databook, the timeout counter is
>>>> started only after the card clock is stopped.
>>>>
>>>> So if card clock is always on, data read timeout interrupt will NOT come,
>>>> and if data lines are always holded high level, all data-related
>>>> interrupt such as start-bit error, data crc error, data over interrupt,
>>>> end-bit error, and so on, will NOT come too.
>>>>
>>>> So driver can't get the current state, it can do nothing but wait for.
>>>>
>>>> This patch is based on https://patchwork.kernel.org/patch/5227941/
>>>>
>>>> Signed-off-by: Addy <addy.ke@rock-chips.com>
>>>> ---
>>>>   drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>   include/linux/mmc/dw_mmc.h |  5 +++++
>>>>   2 files changed, 51 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>> index b4c3044..3960fc3 100644
>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>>>>       return data->error;
>>>>   }
>>>>   +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
>>>> +{
>>>> +    unsigned int data_tmout_clks;
>>>> +    unsigned int data_tmout_ms;
>>>> +
>>>> +    data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>>>> +    data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
> 
> What's 250? And how about using the DIV_ROUND_UP? 
> 
250ms is only for more timeout.
maybe data timeout read from TMOUT register is enough.
So, I will remove 250.
new code:
data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
data_tmout_ms = DIV_ROUND_UP(data_tmout_clks * 100, host->bus_hz);
Is right?

>>>> +
>>>> +    mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
>>>> +}
>>>> +
>>>>   static void dw_mci_tasklet_func(unsigned long priv)
>>>>   {
>>>>       struct dw_mci *host = (struct dw_mci *)priv;
>>>> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
>>>> +                    dw_mci_dto_start_monitor(host);
> 
> if timer is starting at only here, dw_mci_dto_start_monitor() doesn't need.
> 
Ok, I will change it in the next patch.
>>>>                   break;
>>>> +            }
>>>>                 set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>>>>   @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>>>           }
>>>>             if (pending & SDMMC_INT_DATA_OVER) {
>>>> +            if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
>>>> +                del_timer(&host->dto_timer);
>>>> +
>>>>               mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>>>>               if (!host->data_status)
>>>>                   host->data_status = pending;
>>>> @@ -2502,6 +2519,28 @@ ciu_out:
>>>>       return ret;
>>>>   }
>>>>   +static void dw_mci_dto_timer(unsigned long arg)
>>>> +{
>>>> +    struct dw_mci *host = (struct dw_mci *)arg;
> 
> I prefer to use the "data" instead of "arg"
> 
Ok, I will change it in the next patch.
>>>> +
>>>> +    switch (host->state) {
>>>> +    case STATE_SENDING_DATA:
>>>> +    case STATE_DATA_BUSY:
>>>> +        /*
>>>> +        * If data over interrupt does NOT come in sending data state,
>>>> +        * we should notify the driver to teminate current transfer
> teminate/terminate?
> 
Am, I will change it in the next patch.
>>>> +        * 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);
> 
> Dose it need to set EVENT_DATA_COMPLETE?
> 
Yes, it is nessarry!
If not, dw_mci_data_complete function will not be called in my test.
Analysis as follows:
After host recevied command response, driver call tasklet_schedule to
set EVENT_CMD_COMPLETE, change state to STATE_SENDING_DATA, and call
mod_timer. Because there is no any interrupts come in this case,
tasklet_schedule function will not be called until dw_mci_timer is called.

dw_mci_timer-->
tasklet_schedule-->
dw_mci_tasklet_func-->
state == STATE_SENDING_DATA  and EVENT_DATA_ERROR-->
dw_mci_stop_dma, set EVENT_XFER_COMPLETE, send_stop_abort, state = STATE_DATA_ERROR, and then break;-->
check state again -->
state == STATE_DATA_ERROR, if it NOT set EVENT_DATA_COMPLETE in dw_mci_timer goto 1), else goto 2) -->


1) in case STATE_DATA_BUSY, there does nothing but break, and dw_mci_data_complete and dw_mci_request_end
   will not be called. then mmc blocks.

2) in case STATE_DATA_BUSY, becase EVENT_DATA_COMPLETE is set, dw_mci_data_complete and dw_mci_request_end
   will be called to report error to the core.



>>>> +        tasklet_schedule(&host->tasklet);
>>>> +        break;
>>>> +    default:
>>>> +        break;
>>>> +    }
>>>> +}
>>>> +
>>>>   #ifdef CONFIG_OF
>>>>   static struct dw_mci_of_quirks {
>>>>       char *quirk;
>>>> @@ -2513,6 +2552,9 @@ static struct dw_mci_of_quirks {
>>>>       }, {
>>>>           .quirk    = "disable-wp",
>>>>           .id    = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>>>> +    }, {
>>>> +        .quirk    = "dto-timer",
>>>> +        .id    = DW_MCI_QUIRK_DTO_TIMER,
>>>>       },
> 
> Well, this is s/w timer, so i'm not sure this can be merged into dt-file.
> If this is generic solution, we can add s/w timer by default. how about?
ok, I will change it in the next patch.

And is there somewhere need to call del_timer?
> 
> Best Regards,
> Jaehoon Chung
> 
>>>>   };
>>>>   @@ -2654,6 +2696,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_DTO_TIMER)
>>>> +        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..2477813 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 data over interrupt timeout.
>>>>    *
>>>>    * 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 data over interrupt timeout */
>>>> +#define DW_MCI_QUIRK_DTO_TIMER            BIT(5)
>>>>     /* Slot level quirks */
>>>>   /* This slot has no write protect */
>>>>
>>>
>>>
>>>
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> 
> 

Re: [PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[addy ke]

Hi Jaehoon

On 2014/11/19 09:22, Jaehoon Chung Wrote:
> Hi, Addy.
> 
> On 11/18/2014 09:32 AM, Addy wrote:
>>
>> On 2014年11月14日 21:18, Jaehoon Chung wrote:
>>> Hi, Addy.
>>>
>>> Did you use the DW_MCI_QUIRK_IDMAC_DTO?
>>> I'm not sure, but i wonder if you get what result when you use above quirk.
>>
>> DW_MCI_QUIRK_IDMAC_DTO is only for version2.0 or below.
>>         /*
>>          * DTO fix - version 2.10a and below, and only if internal DMA
>>          * is configured.
>>          */
>>         if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) {
>>                 if (!pending &&
>>                     ((mci_readl(host, STATUS) >> 17) & 0x1fff))
>>                         pending |= SDMMC_INT_DATA_OVER;
>>         }
>>
>> It meams that if interrupt comes, but pending = 0 && FIFO_COUNT(bit17-29) !=0,
>> then force to set SDMMC_INT_DATA_OVER.
>> But in our case, FIFO_COUNT = 0 (STATUS register value is 0xad06). This is
>> because that the card does not send data to host. So there is no interrupts come,
>> and interrupt handle function(dw_mci_interrupt) will not be called. So we need a
>> timer to handle this case.
>>
>> So I think  SDMMC_INT_DATA_OVER is not suitable for this case, and we need a new
>> quirk.
>>
>>>
>>> And i will check more this patch at next week.
>>>
>>> Thanks for your efforts.
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>> On 11/14/2014 10:05 PM, Addy Ke wrote:
>>>> From: Addy <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>>
>>>> This patch add a new quirk to notify the driver to teminate
>>>> current transfer and report a data timeout to the core,
>>>> if data over interrupt does NOT come within the given time.
>>>>
>>>> dw_mmc call mmc_request_done func to finish transfer depends on
>>>> data over interrupt. If data over 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. This is the cause that
>>>> card does NOT send data to host.
>>>>
>>>> According to synopsys designware databook, the timeout counter is
>>>> started only after the card clock is stopped.
>>>>
>>>> So if card clock is always on, data read timeout interrupt will NOT come,
>>>> and if data lines are always holded high level, all data-related
>>>> interrupt such as start-bit error, data crc error, data over interrupt,
>>>> end-bit error, and so on, will NOT come too.
>>>>
>>>> So driver can't get the current state, it can do nothing but wait for.
>>>>
>>>> This patch is based on https://patchwork.kernel.org/patch/5227941/
>>>>
>>>> Signed-off-by: Addy <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>> ---
>>>>   drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>   include/linux/mmc/dw_mmc.h |  5 +++++
>>>>   2 files changed, 51 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>> index b4c3044..3960fc3 100644
>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>>>>       return data->error;
>>>>   }
>>>>   +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
>>>> +{
>>>> +    unsigned int data_tmout_clks;
>>>> +    unsigned int data_tmout_ms;
>>>> +
>>>> +    data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>>>> +    data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
> 
> What's 250? And how about using the DIV_ROUND_UP? 
> 
250ms is only for more timeout.
maybe data timeout read from TMOUT register is enough.
So, I will remove 250.
new code:
data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
data_tmout_ms = DIV_ROUND_UP(data_tmout_clks * 100, host->bus_hz);
Is right?

>>>> +
>>>> +    mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
>>>> +}
>>>> +
>>>>   static void dw_mci_tasklet_func(unsigned long priv)
>>>>   {
>>>>       struct dw_mci *host = (struct dw_mci *)priv;
>>>> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
>>>> +                    dw_mci_dto_start_monitor(host);
> 
> if timer is starting at only here, dw_mci_dto_start_monitor() doesn't need.
> 
Ok, I will change it in the next patch.
>>>>                   break;
>>>> +            }
>>>>                 set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>>>>   @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>>>           }
>>>>             if (pending & SDMMC_INT_DATA_OVER) {
>>>> +            if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
>>>> +                del_timer(&host->dto_timer);
>>>> +
>>>>               mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>>>>               if (!host->data_status)
>>>>                   host->data_status = pending;
>>>> @@ -2502,6 +2519,28 @@ ciu_out:
>>>>       return ret;
>>>>   }
>>>>   +static void dw_mci_dto_timer(unsigned long arg)
>>>> +{
>>>> +    struct dw_mci *host = (struct dw_mci *)arg;
> 
> I prefer to use the "data" instead of "arg"
> 
Ok, I will change it in the next patch.
>>>> +
>>>> +    switch (host->state) {
>>>> +    case STATE_SENDING_DATA:
>>>> +    case STATE_DATA_BUSY:
>>>> +        /*
>>>> +        * If data over interrupt does NOT come in sending data state,
>>>> +        * we should notify the driver to teminate current transfer
> teminate/terminate?
> 
Am, I will change it in the next patch.
>>>> +        * 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);
> 
> Dose it need to set EVENT_DATA_COMPLETE?
> 
Yes, it is nessarry!
If not, dw_mci_data_complete function will not be called in my test.
Analysis as follows:
After host recevied command response, driver call tasklet_schedule to
set EVENT_CMD_COMPLETE, change state to STATE_SENDING_DATA, and call
mod_timer. Because there is no any interrupts come in this case,
tasklet_schedule function will not be called until dw_mci_timer is called.

dw_mci_timer-->
tasklet_schedule-->
dw_mci_tasklet_func-->
state == STATE_SENDING_DATA  and EVENT_DATA_ERROR-->
dw_mci_stop_dma, set EVENT_XFER_COMPLETE, send_stop_abort, state = STATE_DATA_ERROR, and then break;-->
check state again -->
state == STATE_DATA_ERROR, if it NOT set EVENT_DATA_COMPLETE in dw_mci_timer goto 1), else goto 2) -->


1) in case STATE_DATA_BUSY, there does nothing but break, and dw_mci_data_complete and dw_mci_request_end
   will not be called. then mmc blocks.

2) in case STATE_DATA_BUSY, becase EVENT_DATA_COMPLETE is set, dw_mci_data_complete and dw_mci_request_end
   will be called to report error to the core.



>>>> +        tasklet_schedule(&host->tasklet);
>>>> +        break;
>>>> +    default:
>>>> +        break;
>>>> +    }
>>>> +}
>>>> +
>>>>   #ifdef CONFIG_OF
>>>>   static struct dw_mci_of_quirks {
>>>>       char *quirk;
>>>> @@ -2513,6 +2552,9 @@ static struct dw_mci_of_quirks {
>>>>       }, {
>>>>           .quirk    = "disable-wp",
>>>>           .id    = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>>>> +    }, {
>>>> +        .quirk    = "dto-timer",
>>>> +        .id    = DW_MCI_QUIRK_DTO_TIMER,
>>>>       },
> 
> Well, this is s/w timer, so i'm not sure this can be merged into dt-file.
> If this is generic solution, we can add s/w timer by default. how about?
ok, I will change it in the next patch.

And is there somewhere need to call del_timer?
> 
> Best Regards,
> Jaehoon Chung
> 
>>>>   };
>>>>   @@ -2654,6 +2696,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_DTO_TIMER)
>>>> +        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..2477813 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 data over interrupt timeout.
>>>>    *
>>>>    * 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 data over interrupt timeout */
>>>> +#define DW_MCI_QUIRK_DTO_TIMER            BIT(5)
>>>>     /* Slot level quirks */
>>>>   /* This slot has no write protect */
>>>>
>>>
>>>
>>>
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[addy ke]

Hi Jaehoon

On 2014/11/19 09:22, Jaehoon Chung Wrote:
> Hi, Addy.
> 
> On 11/18/2014 09:32 AM, Addy wrote:
>>
>> On 2014?11?14? 21:18, Jaehoon Chung wrote:
>>> Hi, Addy.
>>>
>>> Did you use the DW_MCI_QUIRK_IDMAC_DTO?
>>> I'm not sure, but i wonder if you get what result when you use above quirk.
>>
>> DW_MCI_QUIRK_IDMAC_DTO is only for version2.0 or below.
>>         /*
>>          * DTO fix - version 2.10a and below, and only if internal DMA
>>          * is configured.
>>          */
>>         if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) {
>>                 if (!pending &&
>>                     ((mci_readl(host, STATUS) >> 17) & 0x1fff))
>>                         pending |= SDMMC_INT_DATA_OVER;
>>         }
>>
>> It meams that if interrupt comes, but pending = 0 && FIFO_COUNT(bit17-29) !=0,
>> then force to set SDMMC_INT_DATA_OVER.
>> But in our case, FIFO_COUNT = 0 (STATUS register value is 0xad06). This is
>> because that the card does not send data to host. So there is no interrupts come,
>> and interrupt handle function(dw_mci_interrupt) will not be called. So we need a
>> timer to handle this case.
>>
>> So I think  SDMMC_INT_DATA_OVER is not suitable for this case, and we need a new
>> quirk.
>>
>>>
>>> And i will check more this patch at next week.
>>>
>>> Thanks for your efforts.
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>> On 11/14/2014 10:05 PM, Addy Ke wrote:
>>>> From: Addy <addy.ke@rock-chips.com>
>>>>
>>>> This patch add a new quirk to notify the driver to teminate
>>>> current transfer and report a data timeout to the core,
>>>> if data over interrupt does NOT come within the given time.
>>>>
>>>> dw_mmc call mmc_request_done func to finish transfer depends on
>>>> data over interrupt. If data over 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. This is the cause that
>>>> card does NOT send data to host.
>>>>
>>>> According to synopsys designware databook, the timeout counter is
>>>> started only after the card clock is stopped.
>>>>
>>>> So if card clock is always on, data read timeout interrupt will NOT come,
>>>> and if data lines are always holded high level, all data-related
>>>> interrupt such as start-bit error, data crc error, data over interrupt,
>>>> end-bit error, and so on, will NOT come too.
>>>>
>>>> So driver can't get the current state, it can do nothing but wait for.
>>>>
>>>> This patch is based on https://patchwork.kernel.org/patch/5227941/
>>>>
>>>> Signed-off-by: Addy <addy.ke@rock-chips.com>
>>>> ---
>>>>   drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>   include/linux/mmc/dw_mmc.h |  5 +++++
>>>>   2 files changed, 51 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>> index b4c3044..3960fc3 100644
>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>>>>       return data->error;
>>>>   }
>>>>   +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
>>>> +{
>>>> +    unsigned int data_tmout_clks;
>>>> +    unsigned int data_tmout_ms;
>>>> +
>>>> +    data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>>>> +    data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
> 
> What's 250? And how about using the DIV_ROUND_UP? 
> 
250ms is only for more timeout.
maybe data timeout read from TMOUT register is enough.
So, I will remove 250.
new code:
data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
data_tmout_ms = DIV_ROUND_UP(data_tmout_clks * 100, host->bus_hz);
Is right?

>>>> +
>>>> +    mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
>>>> +}
>>>> +
>>>>   static void dw_mci_tasklet_func(unsigned long priv)
>>>>   {
>>>>       struct dw_mci *host = (struct dw_mci *)priv;
>>>> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
>>>> +                    dw_mci_dto_start_monitor(host);
> 
> if timer is starting at only here, dw_mci_dto_start_monitor() doesn't need.
> 
Ok, I will change it in the next patch.
>>>>                   break;
>>>> +            }
>>>>                 set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>>>>   @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>>>           }
>>>>             if (pending & SDMMC_INT_DATA_OVER) {
>>>> +            if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
>>>> +                del_timer(&host->dto_timer);
>>>> +
>>>>               mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>>>>               if (!host->data_status)
>>>>                   host->data_status = pending;
>>>> @@ -2502,6 +2519,28 @@ ciu_out:
>>>>       return ret;
>>>>   }
>>>>   +static void dw_mci_dto_timer(unsigned long arg)
>>>> +{
>>>> +    struct dw_mci *host = (struct dw_mci *)arg;
> 
> I prefer to use the "data" instead of "arg"
> 
Ok, I will change it in the next patch.
>>>> +
>>>> +    switch (host->state) {
>>>> +    case STATE_SENDING_DATA:
>>>> +    case STATE_DATA_BUSY:
>>>> +        /*
>>>> +        * If data over interrupt does NOT come in sending data state,
>>>> +        * we should notify the driver to teminate current transfer
> teminate/terminate?
> 
Am, I will change it in the next patch.
>>>> +        * 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);
> 
> Dose it need to set EVENT_DATA_COMPLETE?
> 
Yes, it is nessarry!
If not, dw_mci_data_complete function will not be called in my test.
Analysis as follows:
After host recevied command response, driver call tasklet_schedule to
set EVENT_CMD_COMPLETE, change state to STATE_SENDING_DATA, and call
mod_timer. Because there is no any interrupts come in this case,
tasklet_schedule function will not be called until dw_mci_timer is called.

dw_mci_timer-->
tasklet_schedule-->
dw_mci_tasklet_func-->
state == STATE_SENDING_DATA  and EVENT_DATA_ERROR-->
dw_mci_stop_dma, set EVENT_XFER_COMPLETE, send_stop_abort, state = STATE_DATA_ERROR, and then break;-->
check state again -->
state == STATE_DATA_ERROR, if it NOT set EVENT_DATA_COMPLETE in dw_mci_timer goto 1), else goto 2) -->


1) in case STATE_DATA_BUSY, there does nothing but break, and dw_mci_data_complete and dw_mci_request_end
   will not be called. then mmc blocks.

2) in case STATE_DATA_BUSY, becase EVENT_DATA_COMPLETE is set, dw_mci_data_complete and dw_mci_request_end
   will be called to report error to the core.



>>>> +        tasklet_schedule(&host->tasklet);
>>>> +        break;
>>>> +    default:
>>>> +        break;
>>>> +    }
>>>> +}
>>>> +
>>>>   #ifdef CONFIG_OF
>>>>   static struct dw_mci_of_quirks {
>>>>       char *quirk;
>>>> @@ -2513,6 +2552,9 @@ static struct dw_mci_of_quirks {
>>>>       }, {
>>>>           .quirk    = "disable-wp",
>>>>           .id    = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>>>> +    }, {
>>>> +        .quirk    = "dto-timer",
>>>> +        .id    = DW_MCI_QUIRK_DTO_TIMER,
>>>>       },
> 
> Well, this is s/w timer, so i'm not sure this can be merged into dt-file.
> If this is generic solution, we can add s/w timer by default. how about?
ok, I will change it in the next patch.

And is there somewhere need to call del_timer?
> 
> Best Regards,
> Jaehoon Chung
> 
>>>>   };
>>>>   @@ -2654,6 +2696,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_DTO_TIMER)
>>>> +        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..2477813 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 data over interrupt timeout.
>>>>    *
>>>>    * 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 data over interrupt timeout */
>>>> +#define DW_MCI_QUIRK_DTO_TIMER            BIT(5)
>>>>     /* Slot level quirks */
>>>>   /* This slot has no write protect */
>>>>
>>>
>>>
>>>
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> 
> 

Re: [PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[addy ke]

Hi, Jaehoon

On 2014/11/19 13:56, addy ke wrote:
> Hi Jaehoon
> 
> On 2014/11/19 09:22, Jaehoon Chung Wrote:
>> Hi, Addy.
>>
>> On 11/18/2014 09:32 AM, Addy wrote:
>>>
>>> On 2014年11月14日 21:18, Jaehoon Chung wrote:
>>>> Hi, Addy.
>>>>
>>>> Did you use the DW_MCI_QUIRK_IDMAC_DTO?
>>>> I'm not sure, but i wonder if you get what result when you use above quirk.
>>>
>>> DW_MCI_QUIRK_IDMAC_DTO is only for version2.0 or below.
>>>         /*
>>>          * DTO fix - version 2.10a and below, and only if internal DMA
>>>          * is configured.
>>>          */
>>>         if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) {
>>>                 if (!pending &&
>>>                     ((mci_readl(host, STATUS) >> 17) & 0x1fff))
>>>                         pending |= SDMMC_INT_DATA_OVER;
>>>         }
>>>
>>> It meams that if interrupt comes, but pending = 0 && FIFO_COUNT(bit17-29) !=0,
>>> then force to set SDMMC_INT_DATA_OVER.
>>> But in our case, FIFO_COUNT = 0 (STATUS register value is 0xad06). This is
>>> because that the card does not send data to host. So there is no interrupts come,
>>> and interrupt handle function(dw_mci_interrupt) will not be called. So we need a
>>> timer to handle this case.
>>>
>>> So I think  SDMMC_INT_DATA_OVER is not suitable for this case, and we need a new
>>> quirk.
>>>
>>>>
>>>> And i will check more this patch at next week.
>>>>
>>>> Thanks for your efforts.
>>>>
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>> On 11/14/2014 10:05 PM, Addy Ke wrote:
>>>>> From: Addy <addy.ke@rock-chips.com>
>>>>>
>>>>> This patch add a new quirk to notify the driver to teminate
>>>>> current transfer and report a data timeout to the core,
>>>>> if data over interrupt does NOT come within the given time.
>>>>>
>>>>> dw_mmc call mmc_request_done func to finish transfer depends on
>>>>> data over interrupt. If data over 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. This is the cause that
>>>>> card does NOT send data to host.
>>>>>
>>>>> According to synopsys designware databook, the timeout counter is
>>>>> started only after the card clock is stopped.
>>>>>
>>>>> So if card clock is always on, data read timeout interrupt will NOT come,
>>>>> and if data lines are always holded high level, all data-related
>>>>> interrupt such as start-bit error, data crc error, data over interrupt,
>>>>> end-bit error, and so on, will NOT come too.
>>>>>
>>>>> So driver can't get the current state, it can do nothing but wait for.
>>>>>
>>>>> This patch is based on https://patchwork.kernel.org/patch/5227941/
>>>>>
>>>>> Signed-off-by: Addy <addy.ke@rock-chips.com>
>>>>> ---
>>>>>   drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>>   include/linux/mmc/dw_mmc.h |  5 +++++
>>>>>   2 files changed, 51 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>> index b4c3044..3960fc3 100644
>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>>>>>       return data->error;
>>>>>   }
>>>>>   +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
>>>>> +{
>>>>> +    unsigned int data_tmout_clks;
>>>>> +    unsigned int data_tmout_ms;
>>>>> +
>>>>> +    data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>>>>> +    data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
>>
>> What's 250? And how about using the DIV_ROUND_UP? 
>>
> 250ms is only for more timeout.
> maybe data timeout read from TMOUT register is enough.
> So, I will remove 250.
> new code:
> data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
> data_tmout_ms = DIV_ROUND_UP(data_tmout_clks * 100, host->bus_hz);
> Is right?
> 
>>>>> +
>>>>> +    mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
>>>>> +}
>>>>> +
>>>>>   static void dw_mci_tasklet_func(unsigned long priv)
>>>>>   {
>>>>>       struct dw_mci *host = (struct dw_mci *)priv;
>>>>> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
>>>>> +                    dw_mci_dto_start_monitor(host);
>>
>> if timer is starting at only here, dw_mci_dto_start_monitor() doesn't need.
>>
> Ok, I will change it in the next patch.
>>>>>                   break;
>>>>> +            }
>>>>>                 set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>>>>>   @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>>>>           }
>>>>>             if (pending & SDMMC_INT_DATA_OVER) {
>>>>> +            if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
>>>>> +                del_timer(&host->dto_timer);
>>>>> +
>>>>>               mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>>>>>               if (!host->data_status)
>>>>>                   host->data_status = pending;
>>>>> @@ -2502,6 +2519,28 @@ ciu_out:
>>>>>       return ret;
>>>>>   }
>>>>>   +static void dw_mci_dto_timer(unsigned long arg)
>>>>> +{
>>>>> +    struct dw_mci *host = (struct dw_mci *)arg;
>>
>> I prefer to use the "data" instead of "arg"
>>
> Ok, I will change it in the next patch.
>>>>> +
>>>>> +    switch (host->state) {
>>>>> +    case STATE_SENDING_DATA:
>>>>> +    case STATE_DATA_BUSY:
>>>>> +        /*
>>>>> +        * If data over interrupt does NOT come in sending data state,
>>>>> +        * we should notify the driver to teminate current transfer
>> teminate/terminate?
>>
> Am, I will change it in the next patch.
>>>>> +        * 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);
>>
>> Dose it need to set EVENT_DATA_COMPLETE?
>>
> Yes, it is nessarry!
> If not, dw_mci_data_complete function will not be called in my test.
> Analysis as follows:
> After host recevied command response, driver call tasklet_schedule to
> set EVENT_CMD_COMPLETE, change state to STATE_SENDING_DATA, and call
> mod_timer. Because there is no any interrupts come in this case,
> tasklet_schedule function will not be called until dw_mci_timer is called.
> 
> dw_mci_timer-->
> tasklet_schedule-->
> dw_mci_tasklet_func-->
> state == STATE_SENDING_DATA  and EVENT_DATA_ERROR-->
> dw_mci_stop_dma, set EVENT_XFER_COMPLETE, send_stop_abort, state = STATE_DATA_ERROR, and then break;-->
> check state again -->
> state == STATE_DATA_ERROR, if it NOT set EVENT_DATA_COMPLETE in dw_mci_timer goto 1), else goto 2) -->
> 
> 
> 1) in case STATE_DATA_BUSY, there does nothing but break, and dw_mci_data_complete and dw_mci_request_end
>    will not be called. then mmc blocks.
> 
> 2) in case STATE_DATA_BUSY, becase EVENT_DATA_COMPLETE is set, dw_mci_data_complete and dw_mci_request_end
>    will be called to report error to the core.
> 
> 
> 
>>>>> +        tasklet_schedule(&host->tasklet);
>>>>> +        break;
>>>>> +    default:
>>>>> +        break;
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>   #ifdef CONFIG_OF
>>>>>   static struct dw_mci_of_quirks {
>>>>>       char *quirk;
>>>>> @@ -2513,6 +2552,9 @@ static struct dw_mci_of_quirks {
>>>>>       }, {
>>>>>           .quirk    = "disable-wp",
>>>>>           .id    = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>>>>> +    }, {
>>>>> +        .quirk    = "dto-timer",
>>>>> +        .id    = DW_MCI_QUIRK_DTO_TIMER,
>>>>>       },
>>
>> Well, this is s/w timer, so i'm not sure this can be merged into dt-file.
>> If this is generic solution, we can add s/w timer by default. how about?
> ok, I will change it in the next patch.
> 
We got the reply from synopsys today:
 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.

It seems that if card does not send data to host, DRTO interrupt will come.
But I really have NOT gotten DRTO interrupt and DTO interrupt in RK3X soc.
Is there other SOC which have the same problem?

If not, I think we need a quirk for it.


> And is there somewhere need to call del_timer?
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>>>   };
>>>>>   @@ -2654,6 +2696,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_DTO_TIMER)
>>>>> +        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..2477813 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 data over interrupt timeout.
>>>>>    *
>>>>>    * 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 data over interrupt timeout */
>>>>> +#define DW_MCI_QUIRK_DTO_TIMER            BIT(5)
>>>>>     /* Slot level quirks */
>>>>>   /* This slot has no write protect */
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>>
>>
>>

Re: [PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[addy ke]

Hi, Jaehoon

On 2014/11/19 13:56, addy ke wrote:
> Hi Jaehoon
> 
> On 2014/11/19 09:22, Jaehoon Chung Wrote:
>> Hi, Addy.
>>
>> On 11/18/2014 09:32 AM, Addy wrote:
>>>
>>> On 2014年11月14日 21:18, Jaehoon Chung wrote:
>>>> Hi, Addy.
>>>>
>>>> Did you use the DW_MCI_QUIRK_IDMAC_DTO?
>>>> I'm not sure, but i wonder if you get what result when you use above quirk.
>>>
>>> DW_MCI_QUIRK_IDMAC_DTO is only for version2.0 or below.
>>>         /*
>>>          * DTO fix - version 2.10a and below, and only if internal DMA
>>>          * is configured.
>>>          */
>>>         if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) {
>>>                 if (!pending &&
>>>                     ((mci_readl(host, STATUS) >> 17) & 0x1fff))
>>>                         pending |= SDMMC_INT_DATA_OVER;
>>>         }
>>>
>>> It meams that if interrupt comes, but pending = 0 && FIFO_COUNT(bit17-29) !=0,
>>> then force to set SDMMC_INT_DATA_OVER.
>>> But in our case, FIFO_COUNT = 0 (STATUS register value is 0xad06). This is
>>> because that the card does not send data to host. So there is no interrupts come,
>>> and interrupt handle function(dw_mci_interrupt) will not be called. So we need a
>>> timer to handle this case.
>>>
>>> So I think  SDMMC_INT_DATA_OVER is not suitable for this case, and we need a new
>>> quirk.
>>>
>>>>
>>>> And i will check more this patch at next week.
>>>>
>>>> Thanks for your efforts.
>>>>
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>> On 11/14/2014 10:05 PM, Addy Ke wrote:
>>>>> From: Addy <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>>>
>>>>> This patch add a new quirk to notify the driver to teminate
>>>>> current transfer and report a data timeout to the core,
>>>>> if data over interrupt does NOT come within the given time.
>>>>>
>>>>> dw_mmc call mmc_request_done func to finish transfer depends on
>>>>> data over interrupt. If data over 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. This is the cause that
>>>>> card does NOT send data to host.
>>>>>
>>>>> According to synopsys designware databook, the timeout counter is
>>>>> started only after the card clock is stopped.
>>>>>
>>>>> So if card clock is always on, data read timeout interrupt will NOT come,
>>>>> and if data lines are always holded high level, all data-related
>>>>> interrupt such as start-bit error, data crc error, data over interrupt,
>>>>> end-bit error, and so on, will NOT come too.
>>>>>
>>>>> So driver can't get the current state, it can do nothing but wait for.
>>>>>
>>>>> This patch is based on https://patchwork.kernel.org/patch/5227941/
>>>>>
>>>>> Signed-off-by: Addy <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>>> ---
>>>>>   drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>>   include/linux/mmc/dw_mmc.h |  5 +++++
>>>>>   2 files changed, 51 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>> index b4c3044..3960fc3 100644
>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>>>>>       return data->error;
>>>>>   }
>>>>>   +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
>>>>> +{
>>>>> +    unsigned int data_tmout_clks;
>>>>> +    unsigned int data_tmout_ms;
>>>>> +
>>>>> +    data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>>>>> +    data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
>>
>> What's 250? And how about using the DIV_ROUND_UP? 
>>
> 250ms is only for more timeout.
> maybe data timeout read from TMOUT register is enough.
> So, I will remove 250.
> new code:
> data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
> data_tmout_ms = DIV_ROUND_UP(data_tmout_clks * 100, host->bus_hz);
> Is right?
> 
>>>>> +
>>>>> +    mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
>>>>> +}
>>>>> +
>>>>>   static void dw_mci_tasklet_func(unsigned long priv)
>>>>>   {
>>>>>       struct dw_mci *host = (struct dw_mci *)priv;
>>>>> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
>>>>> +                    dw_mci_dto_start_monitor(host);
>>
>> if timer is starting at only here, dw_mci_dto_start_monitor() doesn't need.
>>
> Ok, I will change it in the next patch.
>>>>>                   break;
>>>>> +            }
>>>>>                 set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>>>>>   @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>>>>           }
>>>>>             if (pending & SDMMC_INT_DATA_OVER) {
>>>>> +            if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
>>>>> +                del_timer(&host->dto_timer);
>>>>> +
>>>>>               mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>>>>>               if (!host->data_status)
>>>>>                   host->data_status = pending;
>>>>> @@ -2502,6 +2519,28 @@ ciu_out:
>>>>>       return ret;
>>>>>   }
>>>>>   +static void dw_mci_dto_timer(unsigned long arg)
>>>>> +{
>>>>> +    struct dw_mci *host = (struct dw_mci *)arg;
>>
>> I prefer to use the "data" instead of "arg"
>>
> Ok, I will change it in the next patch.
>>>>> +
>>>>> +    switch (host->state) {
>>>>> +    case STATE_SENDING_DATA:
>>>>> +    case STATE_DATA_BUSY:
>>>>> +        /*
>>>>> +        * If data over interrupt does NOT come in sending data state,
>>>>> +        * we should notify the driver to teminate current transfer
>> teminate/terminate?
>>
> Am, I will change it in the next patch.
>>>>> +        * 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);
>>
>> Dose it need to set EVENT_DATA_COMPLETE?
>>
> Yes, it is nessarry!
> If not, dw_mci_data_complete function will not be called in my test.
> Analysis as follows:
> After host recevied command response, driver call tasklet_schedule to
> set EVENT_CMD_COMPLETE, change state to STATE_SENDING_DATA, and call
> mod_timer. Because there is no any interrupts come in this case,
> tasklet_schedule function will not be called until dw_mci_timer is called.
> 
> dw_mci_timer-->
> tasklet_schedule-->
> dw_mci_tasklet_func-->
> state == STATE_SENDING_DATA  and EVENT_DATA_ERROR-->
> dw_mci_stop_dma, set EVENT_XFER_COMPLETE, send_stop_abort, state = STATE_DATA_ERROR, and then break;-->
> check state again -->
> state == STATE_DATA_ERROR, if it NOT set EVENT_DATA_COMPLETE in dw_mci_timer goto 1), else goto 2) -->
> 
> 
> 1) in case STATE_DATA_BUSY, there does nothing but break, and dw_mci_data_complete and dw_mci_request_end
>    will not be called. then mmc blocks.
> 
> 2) in case STATE_DATA_BUSY, becase EVENT_DATA_COMPLETE is set, dw_mci_data_complete and dw_mci_request_end
>    will be called to report error to the core.
> 
> 
> 
>>>>> +        tasklet_schedule(&host->tasklet);
>>>>> +        break;
>>>>> +    default:
>>>>> +        break;
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>   #ifdef CONFIG_OF
>>>>>   static struct dw_mci_of_quirks {
>>>>>       char *quirk;
>>>>> @@ -2513,6 +2552,9 @@ static struct dw_mci_of_quirks {
>>>>>       }, {
>>>>>           .quirk    = "disable-wp",
>>>>>           .id    = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>>>>> +    }, {
>>>>> +        .quirk    = "dto-timer",
>>>>> +        .id    = DW_MCI_QUIRK_DTO_TIMER,
>>>>>       },
>>
>> Well, this is s/w timer, so i'm not sure this can be merged into dt-file.
>> If this is generic solution, we can add s/w timer by default. how about?
> ok, I will change it in the next patch.
> 
We got the reply from synopsys today:
 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.

It seems that if card does not send data to host, DRTO interrupt will come.
But I really have NOT gotten DRTO interrupt and DTO interrupt in RK3X soc.
Is there other SOC which have the same problem?

If not, I think we need a quirk for it.


> And is there somewhere need to call del_timer?
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>>>   };
>>>>>   @@ -2654,6 +2696,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_DTO_TIMER)
>>>>> +        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..2477813 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 data over interrupt timeout.
>>>>>    *
>>>>>    * 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 data over interrupt timeout */
>>>>> +#define DW_MCI_QUIRK_DTO_TIMER            BIT(5)
>>>>>     /* Slot level quirks */
>>>>>   /* This slot has no write protect */
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>>
>>
>>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[addy ke]

Hi, Jaehoon

On 2014/11/19 13:56, addy ke wrote:
> Hi Jaehoon
> 
> On 2014/11/19 09:22, Jaehoon Chung Wrote:
>> Hi, Addy.
>>
>> On 11/18/2014 09:32 AM, Addy wrote:
>>>
>>> On 2014?11?14? 21:18, Jaehoon Chung wrote:
>>>> Hi, Addy.
>>>>
>>>> Did you use the DW_MCI_QUIRK_IDMAC_DTO?
>>>> I'm not sure, but i wonder if you get what result when you use above quirk.
>>>
>>> DW_MCI_QUIRK_IDMAC_DTO is only for version2.0 or below.
>>>         /*
>>>          * DTO fix - version 2.10a and below, and only if internal DMA
>>>          * is configured.
>>>          */
>>>         if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) {
>>>                 if (!pending &&
>>>                     ((mci_readl(host, STATUS) >> 17) & 0x1fff))
>>>                         pending |= SDMMC_INT_DATA_OVER;
>>>         }
>>>
>>> It meams that if interrupt comes, but pending = 0 && FIFO_COUNT(bit17-29) !=0,
>>> then force to set SDMMC_INT_DATA_OVER.
>>> But in our case, FIFO_COUNT = 0 (STATUS register value is 0xad06). This is
>>> because that the card does not send data to host. So there is no interrupts come,
>>> and interrupt handle function(dw_mci_interrupt) will not be called. So we need a
>>> timer to handle this case.
>>>
>>> So I think  SDMMC_INT_DATA_OVER is not suitable for this case, and we need a new
>>> quirk.
>>>
>>>>
>>>> And i will check more this patch at next week.
>>>>
>>>> Thanks for your efforts.
>>>>
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>> On 11/14/2014 10:05 PM, Addy Ke wrote:
>>>>> From: Addy <addy.ke@rock-chips.com>
>>>>>
>>>>> This patch add a new quirk to notify the driver to teminate
>>>>> current transfer and report a data timeout to the core,
>>>>> if data over interrupt does NOT come within the given time.
>>>>>
>>>>> dw_mmc call mmc_request_done func to finish transfer depends on
>>>>> data over interrupt. If data over 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. This is the cause that
>>>>> card does NOT send data to host.
>>>>>
>>>>> According to synopsys designware databook, the timeout counter is
>>>>> started only after the card clock is stopped.
>>>>>
>>>>> So if card clock is always on, data read timeout interrupt will NOT come,
>>>>> and if data lines are always holded high level, all data-related
>>>>> interrupt such as start-bit error, data crc error, data over interrupt,
>>>>> end-bit error, and so on, will NOT come too.
>>>>>
>>>>> So driver can't get the current state, it can do nothing but wait for.
>>>>>
>>>>> This patch is based on https://patchwork.kernel.org/patch/5227941/
>>>>>
>>>>> Signed-off-by: Addy <addy.ke@rock-chips.com>
>>>>> ---
>>>>>   drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>>   include/linux/mmc/dw_mmc.h |  5 +++++
>>>>>   2 files changed, 51 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>> index b4c3044..3960fc3 100644
>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>>>>>       return data->error;
>>>>>   }
>>>>>   +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
>>>>> +{
>>>>> +    unsigned int data_tmout_clks;
>>>>> +    unsigned int data_tmout_ms;
>>>>> +
>>>>> +    data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>>>>> +    data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
>>
>> What's 250? And how about using the DIV_ROUND_UP? 
>>
> 250ms is only for more timeout.
> maybe data timeout read from TMOUT register is enough.
> So, I will remove 250.
> new code:
> data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
> data_tmout_ms = DIV_ROUND_UP(data_tmout_clks * 100, host->bus_hz);
> Is right?
> 
>>>>> +
>>>>> +    mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
>>>>> +}
>>>>> +
>>>>>   static void dw_mci_tasklet_func(unsigned long priv)
>>>>>   {
>>>>>       struct dw_mci *host = (struct dw_mci *)priv;
>>>>> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
>>>>> +                    dw_mci_dto_start_monitor(host);
>>
>> if timer is starting at only here, dw_mci_dto_start_monitor() doesn't need.
>>
> Ok, I will change it in the next patch.
>>>>>                   break;
>>>>> +            }
>>>>>                 set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>>>>>   @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>>>>           }
>>>>>             if (pending & SDMMC_INT_DATA_OVER) {
>>>>> +            if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
>>>>> +                del_timer(&host->dto_timer);
>>>>> +
>>>>>               mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>>>>>               if (!host->data_status)
>>>>>                   host->data_status = pending;
>>>>> @@ -2502,6 +2519,28 @@ ciu_out:
>>>>>       return ret;
>>>>>   }
>>>>>   +static void dw_mci_dto_timer(unsigned long arg)
>>>>> +{
>>>>> +    struct dw_mci *host = (struct dw_mci *)arg;
>>
>> I prefer to use the "data" instead of "arg"
>>
> Ok, I will change it in the next patch.
>>>>> +
>>>>> +    switch (host->state) {
>>>>> +    case STATE_SENDING_DATA:
>>>>> +    case STATE_DATA_BUSY:
>>>>> +        /*
>>>>> +        * If data over interrupt does NOT come in sending data state,
>>>>> +        * we should notify the driver to teminate current transfer
>> teminate/terminate?
>>
> Am, I will change it in the next patch.
>>>>> +        * 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);
>>
>> Dose it need to set EVENT_DATA_COMPLETE?
>>
> Yes, it is nessarry!
> If not, dw_mci_data_complete function will not be called in my test.
> Analysis as follows:
> After host recevied command response, driver call tasklet_schedule to
> set EVENT_CMD_COMPLETE, change state to STATE_SENDING_DATA, and call
> mod_timer. Because there is no any interrupts come in this case,
> tasklet_schedule function will not be called until dw_mci_timer is called.
> 
> dw_mci_timer-->
> tasklet_schedule-->
> dw_mci_tasklet_func-->
> state == STATE_SENDING_DATA  and EVENT_DATA_ERROR-->
> dw_mci_stop_dma, set EVENT_XFER_COMPLETE, send_stop_abort, state = STATE_DATA_ERROR, and then break;-->
> check state again -->
> state == STATE_DATA_ERROR, if it NOT set EVENT_DATA_COMPLETE in dw_mci_timer goto 1), else goto 2) -->
> 
> 
> 1) in case STATE_DATA_BUSY, there does nothing but break, and dw_mci_data_complete and dw_mci_request_end
>    will not be called. then mmc blocks.
> 
> 2) in case STATE_DATA_BUSY, becase EVENT_DATA_COMPLETE is set, dw_mci_data_complete and dw_mci_request_end
>    will be called to report error to the core.
> 
> 
> 
>>>>> +        tasklet_schedule(&host->tasklet);
>>>>> +        break;
>>>>> +    default:
>>>>> +        break;
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>   #ifdef CONFIG_OF
>>>>>   static struct dw_mci_of_quirks {
>>>>>       char *quirk;
>>>>> @@ -2513,6 +2552,9 @@ static struct dw_mci_of_quirks {
>>>>>       }, {
>>>>>           .quirk    = "disable-wp",
>>>>>           .id    = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>>>>> +    }, {
>>>>> +        .quirk    = "dto-timer",
>>>>> +        .id    = DW_MCI_QUIRK_DTO_TIMER,
>>>>>       },
>>
>> Well, this is s/w timer, so i'm not sure this can be merged into dt-file.
>> If this is generic solution, we can add s/w timer by default. how about?
> ok, I will change it in the next patch.
> 
We got the reply from synopsys today:
 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.

It seems that if card does not send data to host, DRTO interrupt will come.
But I really have NOT gotten DRTO interrupt and DTO interrupt in RK3X soc.
Is there other SOC which have the same problem?

If not, I think we need a quirk for it.


> And is there somewhere need to call del_timer?
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>>>   };
>>>>>   @@ -2654,6 +2696,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_DTO_TIMER)
>>>>> +        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..2477813 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 data over interrupt timeout.
>>>>>    *
>>>>>    * 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 data over interrupt timeout */
>>>>> +#define DW_MCI_QUIRK_DTO_TIMER            BIT(5)
>>>>>     /* Slot level quirks */
>>>>>   /* This slot has no write protect */
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>>
>>
>>

Re: [PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[Jaehoon Chung]

Hi, Addy.

On 11/20/2014 06:33 PM, addy ke wrote:
> Hi, Jaehoon
> 
> On 2014/11/19 13:56, addy ke wrote:
>> Hi Jaehoon
>>
>> On 2014/11/19 09:22, Jaehoon Chung Wrote:
>>> Hi, Addy.
>>>
>>> On 11/18/2014 09:32 AM, Addy wrote:
>>>>
>>>> On 2014年11月14日 21:18, Jaehoon Chung wrote:
>>>>> Hi, Addy.
>>>>>
>>>>> Did you use the DW_MCI_QUIRK_IDMAC_DTO?
>>>>> I'm not sure, but i wonder if you get what result when you use above quirk.
>>>>
>>>> DW_MCI_QUIRK_IDMAC_DTO is only for version2.0 or below.
>>>>         /*
>>>>          * DTO fix - version 2.10a and below, and only if internal DMA
>>>>          * is configured.
>>>>          */
>>>>         if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) {
>>>>                 if (!pending &&
>>>>                     ((mci_readl(host, STATUS) >> 17) & 0x1fff))
>>>>                         pending |= SDMMC_INT_DATA_OVER;
>>>>         }
>>>>
>>>> It meams that if interrupt comes, but pending = 0 && FIFO_COUNT(bit17-29) !=0,
>>>> then force to set SDMMC_INT_DATA_OVER.
>>>> But in our case, FIFO_COUNT = 0 (STATUS register value is 0xad06). This is
>>>> because that the card does not send data to host. So there is no interrupts come,
>>>> and interrupt handle function(dw_mci_interrupt) will not be called. So we need a
>>>> timer to handle this case.
>>>>
>>>> So I think  SDMMC_INT_DATA_OVER is not suitable for this case, and we need a new
>>>> quirk.
>>>>
>>>>>
>>>>> And i will check more this patch at next week.
>>>>>
>>>>> Thanks for your efforts.
>>>>>
>>>>> Best Regards,
>>>>> Jaehoon Chung
>>>>>
>>>>> On 11/14/2014 10:05 PM, Addy Ke wrote:
>>>>>> From: Addy <addy.ke@rock-chips.com>
>>>>>>
>>>>>> This patch add a new quirk to notify the driver to teminate
>>>>>> current transfer and report a data timeout to the core,
>>>>>> if data over interrupt does NOT come within the given time.
>>>>>>
>>>>>> dw_mmc call mmc_request_done func to finish transfer depends on
>>>>>> data over interrupt. If data over 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. This is the cause that
>>>>>> card does NOT send data to host.
>>>>>>
>>>>>> According to synopsys designware databook, the timeout counter is
>>>>>> started only after the card clock is stopped.
>>>>>>
>>>>>> So if card clock is always on, data read timeout interrupt will NOT come,
>>>>>> and if data lines are always holded high level, all data-related
>>>>>> interrupt such as start-bit error, data crc error, data over interrupt,
>>>>>> end-bit error, and so on, will NOT come too.
>>>>>>
>>>>>> So driver can't get the current state, it can do nothing but wait for.
>>>>>>
>>>>>> This patch is based on https://patchwork.kernel.org/patch/5227941/
>>>>>>
>>>>>> Signed-off-by: Addy <addy.ke@rock-chips.com>
>>>>>> ---
>>>>>>   drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>>>   include/linux/mmc/dw_mmc.h |  5 +++++
>>>>>>   2 files changed, 51 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>>> index b4c3044..3960fc3 100644
>>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>>> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>>>>>>       return data->error;
>>>>>>   }
>>>>>>   +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
>>>>>> +{
>>>>>> +    unsigned int data_tmout_clks;
>>>>>> +    unsigned int data_tmout_ms;
>>>>>> +
>>>>>> +    data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>>>>>> +    data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
>>>
>>> What's 250? And how about using the DIV_ROUND_UP? 
>>>
>> 250ms is only for more timeout.
>> maybe data timeout read from TMOUT register is enough.
>> So, I will remove 250.
>> new code:
>> data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>> data_tmout_ms = DIV_ROUND_UP(data_tmout_clks * 100, host->bus_hz);
>> Is right?
>>
>>>>>> +
>>>>>> +    mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
>>>>>> +}
>>>>>> +
>>>>>>   static void dw_mci_tasklet_func(unsigned long priv)
>>>>>>   {
>>>>>>       struct dw_mci *host = (struct dw_mci *)priv;
>>>>>> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
>>>>>> +                    dw_mci_dto_start_monitor(host);
>>>
>>> if timer is starting at only here, dw_mci_dto_start_monitor() doesn't need.
>>>
>> Ok, I will change it in the next patch.
>>>>>>                   break;
>>>>>> +            }
>>>>>>                 set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>>>>>>   @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>>>>>           }
>>>>>>             if (pending & SDMMC_INT_DATA_OVER) {
>>>>>> +            if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
>>>>>> +                del_timer(&host->dto_timer);
>>>>>> +
>>>>>>               mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>>>>>>               if (!host->data_status)
>>>>>>                   host->data_status = pending;
>>>>>> @@ -2502,6 +2519,28 @@ ciu_out:
>>>>>>       return ret;
>>>>>>   }
>>>>>>   +static void dw_mci_dto_timer(unsigned long arg)
>>>>>> +{
>>>>>> +    struct dw_mci *host = (struct dw_mci *)arg;
>>>
>>> I prefer to use the "data" instead of "arg"
>>>
>> Ok, I will change it in the next patch.
>>>>>> +
>>>>>> +    switch (host->state) {
>>>>>> +    case STATE_SENDING_DATA:
>>>>>> +    case STATE_DATA_BUSY:
>>>>>> +        /*
>>>>>> +        * If data over interrupt does NOT come in sending data state,
>>>>>> +        * we should notify the driver to teminate current transfer
>>> teminate/terminate?
>>>
>> Am, I will change it in the next patch.
>>>>>> +        * 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);
>>>
>>> Dose it need to set EVENT_DATA_COMPLETE?
>>>
>> Yes, it is nessarry!
>> If not, dw_mci_data_complete function will not be called in my test.
>> Analysis as follows:
>> After host recevied command response, driver call tasklet_schedule to
>> set EVENT_CMD_COMPLETE, change state to STATE_SENDING_DATA, and call
>> mod_timer. Because there is no any interrupts come in this case,
>> tasklet_schedule function will not be called until dw_mci_timer is called.
>>
>> dw_mci_timer-->
>> tasklet_schedule-->
>> dw_mci_tasklet_func-->
>> state == STATE_SENDING_DATA  and EVENT_DATA_ERROR-->
>> dw_mci_stop_dma, set EVENT_XFER_COMPLETE, send_stop_abort, state = STATE_DATA_ERROR, and then break;-->
>> check state again -->
>> state == STATE_DATA_ERROR, if it NOT set EVENT_DATA_COMPLETE in dw_mci_timer goto 1), else goto 2) -->
>>
>>
>> 1) in case STATE_DATA_BUSY, there does nothing but break, and dw_mci_data_complete and dw_mci_request_end
>>    will not be called. then mmc blocks.
>>
>> 2) in case STATE_DATA_BUSY, becase EVENT_DATA_COMPLETE is set, dw_mci_data_complete and dw_mci_request_end
>>    will be called to report error to the core.
>>
>>
>>
>>>>>> +        tasklet_schedule(&host->tasklet);
>>>>>> +        break;
>>>>>> +    default:
>>>>>> +        break;
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>>   #ifdef CONFIG_OF
>>>>>>   static struct dw_mci_of_quirks {
>>>>>>       char *quirk;
>>>>>> @@ -2513,6 +2552,9 @@ static struct dw_mci_of_quirks {
>>>>>>       }, {
>>>>>>           .quirk    = "disable-wp",
>>>>>>           .id    = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>>>>>> +    }, {
>>>>>> +        .quirk    = "dto-timer",
>>>>>> +        .id    = DW_MCI_QUIRK_DTO_TIMER,
>>>>>>       },
>>>
>>> Well, this is s/w timer, so i'm not sure this can be merged into dt-file.
>>> If this is generic solution, we can add s/w timer by default. how about?
>> ok, I will change it in the next patch.
>>
> We got the reply from synopsys today:
>  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.

Then it doesn't need to add s/w timer. if it's working well, it should get DRTO, right?
And Did you try to disable "low-power control"?

> 
> It seems that if card does not send data to host, DRTO interrupt will come.
> But I really have NOT gotten DRTO interrupt and DTO interrupt in RK3X soc.
> Is there other SOC which have the same problem?

Did you get this problem at Only RK3X soc?
Actually, i didn't have see similar problem before.
If you have the error log, could you share it?

> 
> If not, I think we need a quirk for it.

if you need to add this quirks, how about using "broken-dto"?
It means that RK3X soc has "broken Data transfer over scheme"

I will check with my board.

Best Regards,
Jaehoon Chung

> 
> 
>> And is there somewhere need to call del_timer?
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>>>>   };
>>>>>>   @@ -2654,6 +2696,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_DTO_TIMER)
>>>>>> +        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..2477813 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 data over interrupt timeout.
>>>>>>    *
>>>>>>    * 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 data over interrupt timeout */
>>>>>> +#define DW_MCI_QUIRK_DTO_TIMER            BIT(5)
>>>>>>     /* Slot level quirks */
>>>>>>   /* This slot has no write protect */
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>>
>>>
>>>
> 
> 

[PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[Jaehoon Chung]

Hi, Addy.

On 11/20/2014 06:33 PM, addy ke wrote:
> Hi, Jaehoon
> 
> On 2014/11/19 13:56, addy ke wrote:
>> Hi Jaehoon
>>
>> On 2014/11/19 09:22, Jaehoon Chung Wrote:
>>> Hi, Addy.
>>>
>>> On 11/18/2014 09:32 AM, Addy wrote:
>>>>
>>>> On 2014?11?14? 21:18, Jaehoon Chung wrote:
>>>>> Hi, Addy.
>>>>>
>>>>> Did you use the DW_MCI_QUIRK_IDMAC_DTO?
>>>>> I'm not sure, but i wonder if you get what result when you use above quirk.
>>>>
>>>> DW_MCI_QUIRK_IDMAC_DTO is only for version2.0 or below.
>>>>         /*
>>>>          * DTO fix - version 2.10a and below, and only if internal DMA
>>>>          * is configured.
>>>>          */
>>>>         if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) {
>>>>                 if (!pending &&
>>>>                     ((mci_readl(host, STATUS) >> 17) & 0x1fff))
>>>>                         pending |= SDMMC_INT_DATA_OVER;
>>>>         }
>>>>
>>>> It meams that if interrupt comes, but pending = 0 && FIFO_COUNT(bit17-29) !=0,
>>>> then force to set SDMMC_INT_DATA_OVER.
>>>> But in our case, FIFO_COUNT = 0 (STATUS register value is 0xad06). This is
>>>> because that the card does not send data to host. So there is no interrupts come,
>>>> and interrupt handle function(dw_mci_interrupt) will not be called. So we need a
>>>> timer to handle this case.
>>>>
>>>> So I think  SDMMC_INT_DATA_OVER is not suitable for this case, and we need a new
>>>> quirk.
>>>>
>>>>>
>>>>> And i will check more this patch at next week.
>>>>>
>>>>> Thanks for your efforts.
>>>>>
>>>>> Best Regards,
>>>>> Jaehoon Chung
>>>>>
>>>>> On 11/14/2014 10:05 PM, Addy Ke wrote:
>>>>>> From: Addy <addy.ke@rock-chips.com>
>>>>>>
>>>>>> This patch add a new quirk to notify the driver to teminate
>>>>>> current transfer and report a data timeout to the core,
>>>>>> if data over interrupt does NOT come within the given time.
>>>>>>
>>>>>> dw_mmc call mmc_request_done func to finish transfer depends on
>>>>>> data over interrupt. If data over 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. This is the cause that
>>>>>> card does NOT send data to host.
>>>>>>
>>>>>> According to synopsys designware databook, the timeout counter is
>>>>>> started only after the card clock is stopped.
>>>>>>
>>>>>> So if card clock is always on, data read timeout interrupt will NOT come,
>>>>>> and if data lines are always holded high level, all data-related
>>>>>> interrupt such as start-bit error, data crc error, data over interrupt,
>>>>>> end-bit error, and so on, will NOT come too.
>>>>>>
>>>>>> So driver can't get the current state, it can do nothing but wait for.
>>>>>>
>>>>>> This patch is based on https://patchwork.kernel.org/patch/5227941/
>>>>>>
>>>>>> Signed-off-by: Addy <addy.ke@rock-chips.com>
>>>>>> ---
>>>>>>   drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>>>   include/linux/mmc/dw_mmc.h |  5 +++++
>>>>>>   2 files changed, 51 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>>> index b4c3044..3960fc3 100644
>>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>>> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>>>>>>       return data->error;
>>>>>>   }
>>>>>>   +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
>>>>>> +{
>>>>>> +    unsigned int data_tmout_clks;
>>>>>> +    unsigned int data_tmout_ms;
>>>>>> +
>>>>>> +    data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>>>>>> +    data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
>>>
>>> What's 250? And how about using the DIV_ROUND_UP? 
>>>
>> 250ms is only for more timeout.
>> maybe data timeout read from TMOUT register is enough.
>> So, I will remove 250.
>> new code:
>> data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>> data_tmout_ms = DIV_ROUND_UP(data_tmout_clks * 100, host->bus_hz);
>> Is right?
>>
>>>>>> +
>>>>>> +    mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
>>>>>> +}
>>>>>> +
>>>>>>   static void dw_mci_tasklet_func(unsigned long priv)
>>>>>>   {
>>>>>>       struct dw_mci *host = (struct dw_mci *)priv;
>>>>>> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
>>>>>> +                    dw_mci_dto_start_monitor(host);
>>>
>>> if timer is starting at only here, dw_mci_dto_start_monitor() doesn't need.
>>>
>> Ok, I will change it in the next patch.
>>>>>>                   break;
>>>>>> +            }
>>>>>>                 set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>>>>>>   @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>>>>>           }
>>>>>>             if (pending & SDMMC_INT_DATA_OVER) {
>>>>>> +            if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
>>>>>> +                del_timer(&host->dto_timer);
>>>>>> +
>>>>>>               mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>>>>>>               if (!host->data_status)
>>>>>>                   host->data_status = pending;
>>>>>> @@ -2502,6 +2519,28 @@ ciu_out:
>>>>>>       return ret;
>>>>>>   }
>>>>>>   +static void dw_mci_dto_timer(unsigned long arg)
>>>>>> +{
>>>>>> +    struct dw_mci *host = (struct dw_mci *)arg;
>>>
>>> I prefer to use the "data" instead of "arg"
>>>
>> Ok, I will change it in the next patch.
>>>>>> +
>>>>>> +    switch (host->state) {
>>>>>> +    case STATE_SENDING_DATA:
>>>>>> +    case STATE_DATA_BUSY:
>>>>>> +        /*
>>>>>> +        * If data over interrupt does NOT come in sending data state,
>>>>>> +        * we should notify the driver to teminate current transfer
>>> teminate/terminate?
>>>
>> Am, I will change it in the next patch.
>>>>>> +        * 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);
>>>
>>> Dose it need to set EVENT_DATA_COMPLETE?
>>>
>> Yes, it is nessarry!
>> If not, dw_mci_data_complete function will not be called in my test.
>> Analysis as follows:
>> After host recevied command response, driver call tasklet_schedule to
>> set EVENT_CMD_COMPLETE, change state to STATE_SENDING_DATA, and call
>> mod_timer. Because there is no any interrupts come in this case,
>> tasklet_schedule function will not be called until dw_mci_timer is called.
>>
>> dw_mci_timer-->
>> tasklet_schedule-->
>> dw_mci_tasklet_func-->
>> state == STATE_SENDING_DATA  and EVENT_DATA_ERROR-->
>> dw_mci_stop_dma, set EVENT_XFER_COMPLETE, send_stop_abort, state = STATE_DATA_ERROR, and then break;-->
>> check state again -->
>> state == STATE_DATA_ERROR, if it NOT set EVENT_DATA_COMPLETE in dw_mci_timer goto 1), else goto 2) -->
>>
>>
>> 1) in case STATE_DATA_BUSY, there does nothing but break, and dw_mci_data_complete and dw_mci_request_end
>>    will not be called. then mmc blocks.
>>
>> 2) in case STATE_DATA_BUSY, becase EVENT_DATA_COMPLETE is set, dw_mci_data_complete and dw_mci_request_end
>>    will be called to report error to the core.
>>
>>
>>
>>>>>> +        tasklet_schedule(&host->tasklet);
>>>>>> +        break;
>>>>>> +    default:
>>>>>> +        break;
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>>   #ifdef CONFIG_OF
>>>>>>   static struct dw_mci_of_quirks {
>>>>>>       char *quirk;
>>>>>> @@ -2513,6 +2552,9 @@ static struct dw_mci_of_quirks {
>>>>>>       }, {
>>>>>>           .quirk    = "disable-wp",
>>>>>>           .id    = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>>>>>> +    }, {
>>>>>> +        .quirk    = "dto-timer",
>>>>>> +        .id    = DW_MCI_QUIRK_DTO_TIMER,
>>>>>>       },
>>>
>>> Well, this is s/w timer, so i'm not sure this can be merged into dt-file.
>>> If this is generic solution, we can add s/w timer by default. how about?
>> ok, I will change it in the next patch.
>>
> We got the reply from synopsys today:
>  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.

Then it doesn't need to add s/w timer. if it's working well, it should get DRTO, right?
And Did you try to disable "low-power control"?

> 
> It seems that if card does not send data to host, DRTO interrupt will come.
> But I really have NOT gotten DRTO interrupt and DTO interrupt in RK3X soc.
> Is there other SOC which have the same problem?

Did you get this problem at Only RK3X soc?
Actually, i didn't have see similar problem before.
If you have the error log, could you share it?

> 
> If not, I think we need a quirk for it.

if you need to add this quirks, how about using "broken-dto"?
It means that RK3X soc has "broken Data transfer over scheme"

I will check with my board.

Best Regards,
Jaehoon Chung

> 
> 
>> And is there somewhere need to call del_timer?
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>>>>   };
>>>>>>   @@ -2654,6 +2696,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_DTO_TIMER)
>>>>>> +        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..2477813 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 data over interrupt timeout.
>>>>>>    *
>>>>>>    * 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 data over interrupt timeout */
>>>>>> +#define DW_MCI_QUIRK_DTO_TIMER            BIT(5)
>>>>>>     /* Slot level quirks */
>>>>>>   /* This slot has no write protect */
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel at lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>>
>>>
>>>
> 
> 

Re: [PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[Addy]

Hi, Jaehoon

On 2014/11/20 18:01, Jaehoon Chung wrote:
> Hi, Addy.
>
> On 11/20/2014 06:33 PM, addy ke wrote:
>> Hi, Jaehoon
>>
>> On 2014/11/19 13:56, addy ke wrote:
>>> Hi Jaehoon
>>>
>>> On 2014/11/19 09:22, Jaehoon Chung Wrote:
>>>> Hi, Addy.
>>>>
>>>> On 11/18/2014 09:32 AM, Addy wrote:
>>>>> On 2014年11月14日 21:18, Jaehoon Chung wrote:
>>>>>> Hi, Addy.
>>>>>>
>>>>>> Did you use the DW_MCI_QUIRK_IDMAC_DTO?
>>>>>> I'm not sure, but i wonder if you get what result when you use above quirk.
>>>>> DW_MCI_QUIRK_IDMAC_DTO is only for version2.0 or below.
>>>>>          /*
>>>>>           * DTO fix - version 2.10a and below, and only if internal DMA
>>>>>           * is configured.
>>>>>           */
>>>>>          if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) {
>>>>>                  if (!pending &&
>>>>>                      ((mci_readl(host, STATUS) >> 17) & 0x1fff))
>>>>>                          pending |= SDMMC_INT_DATA_OVER;
>>>>>          }
>>>>>
>>>>> It meams that if interrupt comes, but pending = 0 && FIFO_COUNT(bit17-29) !=0,
>>>>> then force to set SDMMC_INT_DATA_OVER.
>>>>> But in our case, FIFO_COUNT = 0 (STATUS register value is 0xad06). This is
>>>>> because that the card does not send data to host. So there is no interrupts come,
>>>>> and interrupt handle function(dw_mci_interrupt) will not be called. So we need a
>>>>> timer to handle this case.
>>>>>
>>>>> So I think  SDMMC_INT_DATA_OVER is not suitable for this case, and we need a new
>>>>> quirk.
>>>>>
>>>>>> And i will check more this patch at next week.
>>>>>>
>>>>>> Thanks for your efforts.
>>>>>>
>>>>>> Best Regards,
>>>>>> Jaehoon Chung
>>>>>>
>>>>>> On 11/14/2014 10:05 PM, Addy Ke wrote:
>>>>>>> From: Addy <addy.ke@rock-chips.com>
>>>>>>>
>>>>>>> This patch add a new quirk to notify the driver to teminate
>>>>>>> current transfer and report a data timeout to the core,
>>>>>>> if data over interrupt does NOT come within the given time.
>>>>>>>
>>>>>>> dw_mmc call mmc_request_done func to finish transfer depends on
>>>>>>> data over interrupt. If data over 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. This is the cause that
>>>>>>> card does NOT send data to host.
>>>>>>>
>>>>>>> According to synopsys designware databook, the timeout counter is
>>>>>>> started only after the card clock is stopped.
>>>>>>>
>>>>>>> So if card clock is always on, data read timeout interrupt will NOT come,
>>>>>>> and if data lines are always holded high level, all data-related
>>>>>>> interrupt such as start-bit error, data crc error, data over interrupt,
>>>>>>> end-bit error, and so on, will NOT come too.
>>>>>>>
>>>>>>> So driver can't get the current state, it can do nothing but wait for.
>>>>>>>
>>>>>>> This patch is based on https://patchwork.kernel.org/patch/5227941/
>>>>>>>
>>>>>>> Signed-off-by: Addy <addy.ke@rock-chips.com>
>>>>>>> ---
>>>>>>>    drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>>>>    include/linux/mmc/dw_mmc.h |  5 +++++
>>>>>>>    2 files changed, 51 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>>>> index b4c3044..3960fc3 100644
>>>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>>>> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>>>>>>>        return data->error;
>>>>>>>    }
>>>>>>>    +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
>>>>>>> +{
>>>>>>> +    unsigned int data_tmout_clks;
>>>>>>> +    unsigned int data_tmout_ms;
>>>>>>> +
>>>>>>> +    data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>>>>>>> +    data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
>>>> What's 250? And how about using the DIV_ROUND_UP?
>>>>
>>> 250ms is only for more timeout.
>>> maybe data timeout read from TMOUT register is enough.
>>> So, I will remove 250.
>>> new code:
>>> data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>>> data_tmout_ms = DIV_ROUND_UP(data_tmout_clks * 100, host->bus_hz);
>>> Is right?
>>>
>>>>>>> +
>>>>>>> +    mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
>>>>>>> +}
>>>>>>> +
>>>>>>>    static void dw_mci_tasklet_func(unsigned long priv)
>>>>>>>    {
>>>>>>>        struct dw_mci *host = (struct dw_mci *)priv;
>>>>>>> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
>>>>>>> +                    dw_mci_dto_start_monitor(host);
>>>> if timer is starting at only here, dw_mci_dto_start_monitor() doesn't need.
>>>>
>>> Ok, I will change it in the next patch.
>>>>>>>                    break;
>>>>>>> +            }
>>>>>>>                  set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>>>>>>>    @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>>>>>>            }
>>>>>>>              if (pending & SDMMC_INT_DATA_OVER) {
>>>>>>> +            if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
>>>>>>> +                del_timer(&host->dto_timer);
>>>>>>> +
>>>>>>>                mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>>>>>>>                if (!host->data_status)
>>>>>>>                    host->data_status = pending;
>>>>>>> @@ -2502,6 +2519,28 @@ ciu_out:
>>>>>>>        return ret;
>>>>>>>    }
>>>>>>>    +static void dw_mci_dto_timer(unsigned long arg)
>>>>>>> +{
>>>>>>> +    struct dw_mci *host = (struct dw_mci *)arg;
>>>> I prefer to use the "data" instead of "arg"
>>>>
>>> Ok, I will change it in the next patch.
>>>>>>> +
>>>>>>> +    switch (host->state) {
>>>>>>> +    case STATE_SENDING_DATA:
>>>>>>> +    case STATE_DATA_BUSY:
>>>>>>> +        /*
>>>>>>> +        * If data over interrupt does NOT come in sending data state,
>>>>>>> +        * we should notify the driver to teminate current transfer
>>>> teminate/terminate?
>>>>
>>> Am, I will change it in the next patch.
>>>>>>> +        * 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);
>>>> Dose it need to set EVENT_DATA_COMPLETE?
>>>>
>>> Yes, it is nessarry!
>>> If not, dw_mci_data_complete function will not be called in my test.
>>> Analysis as follows:
>>> After host recevied command response, driver call tasklet_schedule to
>>> set EVENT_CMD_COMPLETE, change state to STATE_SENDING_DATA, and call
>>> mod_timer. Because there is no any interrupts come in this case,
>>> tasklet_schedule function will not be called until dw_mci_timer is called.
>>>
>>> dw_mci_timer-->
>>> tasklet_schedule-->
>>> dw_mci_tasklet_func-->
>>> state == STATE_SENDING_DATA  and EVENT_DATA_ERROR-->
>>> dw_mci_stop_dma, set EVENT_XFER_COMPLETE, send_stop_abort, state = STATE_DATA_ERROR, and then break;-->
>>> check state again -->
>>> state == STATE_DATA_ERROR, if it NOT set EVENT_DATA_COMPLETE in dw_mci_timer goto 1), else goto 2) -->
>>>
>>>
>>> 1) in case STATE_DATA_BUSY, there does nothing but break, and dw_mci_data_complete and dw_mci_request_end
>>>     will not be called. then mmc blocks.
>>>
>>> 2) in case STATE_DATA_BUSY, becase EVENT_DATA_COMPLETE is set, dw_mci_data_complete and dw_mci_request_end
>>>     will be called to report error to the core.
>>>
>>>
>>>
>>>>>>> +        tasklet_schedule(&host->tasklet);
>>>>>>> +        break;
>>>>>>> +    default:
>>>>>>> +        break;
>>>>>>> +    }
>>>>>>> +}
>>>>>>> +
>>>>>>>    #ifdef CONFIG_OF
>>>>>>>    static struct dw_mci_of_quirks {
>>>>>>>        char *quirk;
>>>>>>> @@ -2513,6 +2552,9 @@ static struct dw_mci_of_quirks {
>>>>>>>        }, {
>>>>>>>            .quirk    = "disable-wp",
>>>>>>>            .id    = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>>>>>>> +    }, {
>>>>>>> +        .quirk    = "dto-timer",
>>>>>>> +        .id    = DW_MCI_QUIRK_DTO_TIMER,
>>>>>>>        },
>>>> Well, this is s/w timer, so i'm not sure this can be merged into dt-file.
>>>> If this is generic solution, we can add s/w timer by default. how about?
>>> ok, I will change it in the next patch.
>>>
>> We got the reply from synopsys today:
>>   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.
> Then it doesn't need to add s/w timer. if it's working well, it should get DRTO, right?
> And Did you try to disable "low-power control"?
Sure, I think
It should get DRTO and DTO according to spec.
I have tried to disable "low-power control" , but dto still didn't come.
>> It seems that if card does not send data to host, DRTO interrupt will come.
>> But I really have NOT gotten DRTO interrupt and DTO interrupt in RK3X soc.
>> Is there other SOC which have the same problem?
> Did you get this problem at Only RK3X soc?
> Actually, i didn't have see similar problem before.
> If you have the error log, could you share it?
Yes, I only got this problem on rk3x.
And there is not any error log, after host receives command response and 
enter "sending data state".
>> If not, I think we need a quirk for it.
> if you need to add this quirks, how about using "broken-dto"?
> It means that RK3X soc has "broken Data transfer over scheme"
Am, OK
I will use "broken-dto" for quirk, and DW_MCI_QUIRK_BROKEN_DTO for id in 
the next patch.
> I will check with my board.
>
> Best Regards,
> Jaehoon Chung
>
>>
>>> And is there somewhere need to call del_timer?
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>>>>>    };
>>>>>>>    @@ -2654,6 +2696,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_DTO_TIMER)
>>>>>>> +        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..2477813 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 data over interrupt timeout.
>>>>>>>     *
>>>>>>>     * 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 data over interrupt timeout */
>>>>>>> +#define DW_MCI_QUIRK_DTO_TIMER            BIT(5)
>>>>>>>      /* Slot level quirks */
>>>>>>>    /* This slot has no write protect */
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> linux-arm-kernel mailing list
>>>>> linux-arm-kernel@lists.infradead.org
>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>
>>>>
>>>>
>>
>
>
>

[PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout

[Addy]

Hi, Jaehoon

On 2014/11/20 18:01, Jaehoon Chung wrote:
> Hi, Addy.
>
> On 11/20/2014 06:33 PM, addy ke wrote:
>> Hi, Jaehoon
>>
>> On 2014/11/19 13:56, addy ke wrote:
>>> Hi Jaehoon
>>>
>>> On 2014/11/19 09:22, Jaehoon Chung Wrote:
>>>> Hi, Addy.
>>>>
>>>> On 11/18/2014 09:32 AM, Addy wrote:
>>>>> On 2014?11?14? 21:18, Jaehoon Chung wrote:
>>>>>> Hi, Addy.
>>>>>>
>>>>>> Did you use the DW_MCI_QUIRK_IDMAC_DTO?
>>>>>> I'm not sure, but i wonder if you get what result when you use above quirk.
>>>>> DW_MCI_QUIRK_IDMAC_DTO is only for version2.0 or below.
>>>>>          /*
>>>>>           * DTO fix - version 2.10a and below, and only if internal DMA
>>>>>           * is configured.
>>>>>           */
>>>>>          if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) {
>>>>>                  if (!pending &&
>>>>>                      ((mci_readl(host, STATUS) >> 17) & 0x1fff))
>>>>>                          pending |= SDMMC_INT_DATA_OVER;
>>>>>          }
>>>>>
>>>>> It meams that if interrupt comes, but pending = 0 && FIFO_COUNT(bit17-29) !=0,
>>>>> then force to set SDMMC_INT_DATA_OVER.
>>>>> But in our case, FIFO_COUNT = 0 (STATUS register value is 0xad06). This is
>>>>> because that the card does not send data to host. So there is no interrupts come,
>>>>> and interrupt handle function(dw_mci_interrupt) will not be called. So we need a
>>>>> timer to handle this case.
>>>>>
>>>>> So I think  SDMMC_INT_DATA_OVER is not suitable for this case, and we need a new
>>>>> quirk.
>>>>>
>>>>>> And i will check more this patch at next week.
>>>>>>
>>>>>> Thanks for your efforts.
>>>>>>
>>>>>> Best Regards,
>>>>>> Jaehoon Chung
>>>>>>
>>>>>> On 11/14/2014 10:05 PM, Addy Ke wrote:
>>>>>>> From: Addy <addy.ke@rock-chips.com>
>>>>>>>
>>>>>>> This patch add a new quirk to notify the driver to teminate
>>>>>>> current transfer and report a data timeout to the core,
>>>>>>> if data over interrupt does NOT come within the given time.
>>>>>>>
>>>>>>> dw_mmc call mmc_request_done func to finish transfer depends on
>>>>>>> data over interrupt. If data over 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. This is the cause that
>>>>>>> card does NOT send data to host.
>>>>>>>
>>>>>>> According to synopsys designware databook, the timeout counter is
>>>>>>> started only after the card clock is stopped.
>>>>>>>
>>>>>>> So if card clock is always on, data read timeout interrupt will NOT come,
>>>>>>> and if data lines are always holded high level, all data-related
>>>>>>> interrupt such as start-bit error, data crc error, data over interrupt,
>>>>>>> end-bit error, and so on, will NOT come too.
>>>>>>>
>>>>>>> So driver can't get the current state, it can do nothing but wait for.
>>>>>>>
>>>>>>> This patch is based on https://patchwork.kernel.org/patch/5227941/
>>>>>>>
>>>>>>> Signed-off-by: Addy <addy.ke@rock-chips.com>
>>>>>>> ---
>>>>>>>    drivers/mmc/host/dw_mmc.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>>>>    include/linux/mmc/dw_mmc.h |  5 +++++
>>>>>>>    2 files changed, 51 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>>>> index b4c3044..3960fc3 100644
>>>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>>>> @@ -1448,6 +1448,17 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>>>>>>>        return data->error;
>>>>>>>    }
>>>>>>>    +static inline void dw_mci_dto_start_monitor(struct dw_mci *host)
>>>>>>> +{
>>>>>>> +    unsigned int data_tmout_clks;
>>>>>>> +    unsigned int data_tmout_ms;
>>>>>>> +
>>>>>>> +    data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>>>>>>> +    data_tmout_ms = (data_tmout_clks * 1000 / host->bus_hz) + 250;
>>>> What's 250? And how about using the DIV_ROUND_UP?
>>>>
>>> 250ms is only for more timeout.
>>> maybe data timeout read from TMOUT register is enough.
>>> So, I will remove 250.
>>> new code:
>>> data_tmout_clks = (mci_readl(host, TMOUT) >> 8);
>>> data_tmout_ms = DIV_ROUND_UP(data_tmout_clks * 100, host->bus_hz);
>>> Is right?
>>>
>>>>>>> +
>>>>>>> +    mod_timer(&host->dto_timer, jiffies + msecs_to_jiffies(data_tmout_ms));
>>>>>>> +}
>>>>>>> +
>>>>>>>    static void dw_mci_tasklet_func(unsigned long priv)
>>>>>>>    {
>>>>>>>        struct dw_mci *host = (struct dw_mci *)priv;
>>>>>>> @@ -1522,8 +1533,11 @@ 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_DTO_TIMER)
>>>>>>> +                    dw_mci_dto_start_monitor(host);
>>>> if timer is starting at only here, dw_mci_dto_start_monitor() doesn't need.
>>>>
>>> Ok, I will change it in the next patch.
>>>>>>>                    break;
>>>>>>> +            }
>>>>>>>                  set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>>>>>>>    @@ -2115,6 +2129,9 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>>>>>>            }
>>>>>>>              if (pending & SDMMC_INT_DATA_OVER) {
>>>>>>> +            if (host->quirks & DW_MCI_QUIRK_DTO_TIMER)
>>>>>>> +                del_timer(&host->dto_timer);
>>>>>>> +
>>>>>>>                mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>>>>>>>                if (!host->data_status)
>>>>>>>                    host->data_status = pending;
>>>>>>> @@ -2502,6 +2519,28 @@ ciu_out:
>>>>>>>        return ret;
>>>>>>>    }
>>>>>>>    +static void dw_mci_dto_timer(unsigned long arg)
>>>>>>> +{
>>>>>>> +    struct dw_mci *host = (struct dw_mci *)arg;
>>>> I prefer to use the "data" instead of "arg"
>>>>
>>> Ok, I will change it in the next patch.
>>>>>>> +
>>>>>>> +    switch (host->state) {
>>>>>>> +    case STATE_SENDING_DATA:
>>>>>>> +    case STATE_DATA_BUSY:
>>>>>>> +        /*
>>>>>>> +        * If data over interrupt does NOT come in sending data state,
>>>>>>> +        * we should notify the driver to teminate current transfer
>>>> teminate/terminate?
>>>>
>>> Am, I will change it in the next patch.
>>>>>>> +        * 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);
>>>> Dose it need to set EVENT_DATA_COMPLETE?
>>>>
>>> Yes, it is nessarry!
>>> If not, dw_mci_data_complete function will not be called in my test.
>>> Analysis as follows:
>>> After host recevied command response, driver call tasklet_schedule to
>>> set EVENT_CMD_COMPLETE, change state to STATE_SENDING_DATA, and call
>>> mod_timer. Because there is no any interrupts come in this case,
>>> tasklet_schedule function will not be called until dw_mci_timer is called.
>>>
>>> dw_mci_timer-->
>>> tasklet_schedule-->
>>> dw_mci_tasklet_func-->
>>> state == STATE_SENDING_DATA  and EVENT_DATA_ERROR-->
>>> dw_mci_stop_dma, set EVENT_XFER_COMPLETE, send_stop_abort, state = STATE_DATA_ERROR, and then break;-->
>>> check state again -->
>>> state == STATE_DATA_ERROR, if it NOT set EVENT_DATA_COMPLETE in dw_mci_timer goto 1), else goto 2) -->
>>>
>>>
>>> 1) in case STATE_DATA_BUSY, there does nothing but break, and dw_mci_data_complete and dw_mci_request_end
>>>     will not be called. then mmc blocks.
>>>
>>> 2) in case STATE_DATA_BUSY, becase EVENT_DATA_COMPLETE is set, dw_mci_data_complete and dw_mci_request_end
>>>     will be called to report error to the core.
>>>
>>>
>>>
>>>>>>> +        tasklet_schedule(&host->tasklet);
>>>>>>> +        break;
>>>>>>> +    default:
>>>>>>> +        break;
>>>>>>> +    }
>>>>>>> +}
>>>>>>> +
>>>>>>>    #ifdef CONFIG_OF
>>>>>>>    static struct dw_mci_of_quirks {
>>>>>>>        char *quirk;
>>>>>>> @@ -2513,6 +2552,9 @@ static struct dw_mci_of_quirks {
>>>>>>>        }, {
>>>>>>>            .quirk    = "disable-wp",
>>>>>>>            .id    = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>>>>>>> +    }, {
>>>>>>> +        .quirk    = "dto-timer",
>>>>>>> +        .id    = DW_MCI_QUIRK_DTO_TIMER,
>>>>>>>        },
>>>> Well, this is s/w timer, so i'm not sure this can be merged into dt-file.
>>>> If this is generic solution, we can add s/w timer by default. how about?
>>> ok, I will change it in the next patch.
>>>
>> We got the reply from synopsys today:
>>   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.
> Then it doesn't need to add s/w timer. if it's working well, it should get DRTO, right?
> And Did you try to disable "low-power control"?
Sure, I think
It should get DRTO and DTO according to spec.
I have tried to disable "low-power control" , but dto still didn't come.
>> It seems that if card does not send data to host, DRTO interrupt will come.
>> But I really have NOT gotten DRTO interrupt and DTO interrupt in RK3X soc.
>> Is there other SOC which have the same problem?
> Did you get this problem at Only RK3X soc?
> Actually, i didn't have see similar problem before.
> If you have the error log, could you share it?
Yes, I only got this problem on rk3x.
And there is not any error log, after host receives command response and 
enter "sending data state".
>> If not, I think we need a quirk for it.
> if you need to add this quirks, how about using "broken-dto"?
> It means that RK3X soc has "broken Data transfer over scheme"
Am, OK
I will use "broken-dto" for quirk, and DW_MCI_QUIRK_BROKEN_DTO for id in 
the next patch.
> I will check with my board.
>
> Best Regards,
> Jaehoon Chung
>
>>
>>> And is there somewhere need to call del_timer?
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>>>>>    };
>>>>>>>    @@ -2654,6 +2696,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_DTO_TIMER)
>>>>>>> +        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..2477813 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 data over interrupt timeout.
>>>>>>>     *
>>>>>>>     * 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 data over interrupt timeout */
>>>>>>> +#define DW_MCI_QUIRK_DTO_TIMER            BIT(5)
>>>>>>>      /* Slot level quirks */
>>>>>>>    /* This slot has no write protect */
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> linux-arm-kernel mailing list
>>>>> linux-arm-kernel at lists.infradead.org
>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>
>>>>
>>>>
>>
>
>
>

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

[Addy Ke]

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(-)

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 */
-- 
1.8.3.2

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

[Addy Ke]

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(-)

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 */
-- 
1.8.3.2

Re: [PATCH v2] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Doug Anderson]

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

Re: [PATCH v2] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Doug Anderson]

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

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

[Doug Anderson]

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

Re: [PATCH v2] mmc: dw_mmc: add quirk for broken data transfer over scheme

[addy ke]

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?

> 
> 
> 

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

[addy ke]

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?

> 
> 
> 

Re: [PATCH v2] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Doug Anderson]

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?

Re: [PATCH v2] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Doug Anderson]

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 v2] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Doug Anderson]

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 v3] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Addy Ke]

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.

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

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
Changes in v2:
- 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.

Changes in v3:
- Remove dts for broken-dto, just add this quirk in dw_mci_rockchip_init

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

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 5650ac4..ba92ebd 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -73,6 +73,9 @@ static int dw_mci_rockchip_init(struct dw_mci *host)
 	/* It is slot 8 on Rockchip SoCs */
 	host->sdio_id0 = 8;
 
+	/* It needs this quirk on all Rockchip SoCs */
+	host->pdata->quirks |= DW_MCI_QUIRK_BROKEN_DTO;
+
 	return 0;
 }
 
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 67c0451..e222122 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1464,6 +1464,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);
 
@@ -1529,8 +1531,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);
 
@@ -2122,6 +2133,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;
@@ -2509,6 +2523,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;
@@ -2661,6 +2697,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 */
-- 
1.8.3.2

[PATCH v3] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Addy Ke]

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.

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

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
Changes in v2:
- 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.

Changes in v3:
- Remove dts for broken-dto, just add this quirk in dw_mci_rockchip_init

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

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 5650ac4..ba92ebd 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -73,6 +73,9 @@ static int dw_mci_rockchip_init(struct dw_mci *host)
 	/* It is slot 8 on Rockchip SoCs */
 	host->sdio_id0 = 8;
 
+	/* It needs this quirk on all Rockchip SoCs */
+	host->pdata->quirks |= DW_MCI_QUIRK_BROKEN_DTO;
+
 	return 0;
 }
 
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 67c0451..e222122 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1464,6 +1464,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);
 
@@ -1529,8 +1531,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);
 
@@ -2122,6 +2133,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;
@@ -2509,6 +2523,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;
@@ -2661,6 +2697,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 */
-- 
1.8.3.2

Re: [PATCH v3] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Doug Anderson]

Addy,

On Tue, Dec 2, 2014 at 7:16 PM, 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.
>
> We don't know why we have this problem, but we need it to fix this problem now.
> And I will post a follow up change when we find the root cause.
>
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> ---
> Changes in v2:
> - 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.
>
> Changes in v3:
> - Remove dts for broken-dto, just add this quirk in dw_mci_rockchip_init
>
>  drivers/mmc/host/dw_mmc-rockchip.c |  3 +++
>  drivers/mmc/host/dw_mmc.c          | 41 +++++++++++++++++++++++++++++++++++++-
>  include/linux/mmc/dw_mmc.h         |  5 +++++
>  3 files changed, 48 insertions(+), 1 deletion(-)

This seems reasonable to me.  I do hope that you can get to a root
cause, but I don't think this is a terrible bit of code to carry.
Obviously it's up to the folks who maintain dw_mmc and the mmc
subsystem, but:

Reviewed-by: Doug Anderson <dianders@chromium.org>

Re: [PATCH v3] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Doug Anderson]

Addy,

On Tue, Dec 2, 2014 at 7:16 PM, 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.
>
> We don't know why we have this problem, but we need it to fix this problem now.
> And I will post a follow up change when we find the root cause.
>
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> ---
> Changes in v2:
> - 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.
>
> Changes in v3:
> - Remove dts for broken-dto, just add this quirk in dw_mci_rockchip_init
>
>  drivers/mmc/host/dw_mmc-rockchip.c |  3 +++
>  drivers/mmc/host/dw_mmc.c          | 41 +++++++++++++++++++++++++++++++++++++-
>  include/linux/mmc/dw_mmc.h         |  5 +++++
>  3 files changed, 48 insertions(+), 1 deletion(-)

This seems reasonable to me.  I do hope that you can get to a root
cause, but I don't think this is a terrible bit of code to carry.
Obviously it's up to the folks who maintain dw_mmc and the mmc
subsystem, but:

Reviewed-by: Doug Anderson <dianders@chromium.org>

[PATCH v3] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Doug Anderson]

Addy,

On Tue, Dec 2, 2014 at 7:16 PM, 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.
>
> We don't know why we have this problem, but we need it to fix this problem now.
> And I will post a follow up change when we find the root cause.
>
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> ---
> Changes in v2:
> - 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.
>
> Changes in v3:
> - Remove dts for broken-dto, just add this quirk in dw_mci_rockchip_init
>
>  drivers/mmc/host/dw_mmc-rockchip.c |  3 +++
>  drivers/mmc/host/dw_mmc.c          | 41 +++++++++++++++++++++++++++++++++++++-
>  include/linux/mmc/dw_mmc.h         |  5 +++++
>  3 files changed, 48 insertions(+), 1 deletion(-)

This seems reasonable to me.  I do hope that you can get to a root
cause, but I don't think this is a terrible bit of code to carry.
Obviously it's up to the folks who maintain dw_mmc and the mmc
subsystem, but:

Reviewed-by: Doug Anderson <dianders@chromium.org>

Re: [PATCH v3] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Alexandru Stan]

Seems like the BROKEN_DTO also affects EVENT_DATA_COMPLETE, sometimes we don't
see that (but we do see EVENT_XFER_COMPLETE), leave the state machine and never
come back to it to timeout. I have this happening on emmc specifically, haven't
seen it on sdmmc yet.

The fix is to also start the timer when we don't see a EVENT_DATA_COMPLETE.

More details for how I debugged this can be found at:
https://chromium-review.googlesource.com/#/c/233691/

In ~100 reboot tests I haven't seen any hangs, whereas before it was
hanging about 20% of the time.

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 50865e7..c678206 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1685,8 +1685,17 @@ static void dw_mci_tasklet_func(unsigned long priv)

                case STATE_DATA_BUSY:
                        if (!test_and_clear_bit(EVENT_DATA_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;
+                       }

                        host->data = NULL;
                        set_bit(EVENT_DATA_COMPLETE, &host->completed_events);
Alexandru Stan (amstan)


On Tue, Dec 2, 2014 at 9:08 PM, Doug Anderson <dianders@chromium.org> wrote:
> Addy,
>
> On Tue, Dec 2, 2014 at 7:16 PM, 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.
>>
>> We don't know why we have this problem, but we need it to fix this problem now.
>> And I will post a follow up change when we find the root cause.
>>
>> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
>> ---
>> Changes in v2:
>> - 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.
>>
>> Changes in v3:
>> - Remove dts for broken-dto, just add this quirk in dw_mci_rockchip_init
>>
>>  drivers/mmc/host/dw_mmc-rockchip.c |  3 +++
>>  drivers/mmc/host/dw_mmc.c          | 41 +++++++++++++++++++++++++++++++++++++-
>>  include/linux/mmc/dw_mmc.h         |  5 +++++
>>  3 files changed, 48 insertions(+), 1 deletion(-)
>
> This seems reasonable to me.  I do hope that you can get to a root
> cause, but I don't think this is a terrible bit of code to carry.
> Obviously it's up to the folks who maintain dw_mmc and the mmc
> subsystem, but:
>
> Reviewed-by: Doug Anderson <dianders@chromium.org>

Re: [PATCH v3] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Alexandru Stan]

Seems like the BROKEN_DTO also affects EVENT_DATA_COMPLETE, sometimes we don't
see that (but we do see EVENT_XFER_COMPLETE), leave the state machine and never
come back to it to timeout. I have this happening on emmc specifically, haven't
seen it on sdmmc yet.

The fix is to also start the timer when we don't see a EVENT_DATA_COMPLETE.

More details for how I debugged this can be found at:
https://chromium-review.googlesource.com/#/c/233691/

In ~100 reboot tests I haven't seen any hangs, whereas before it was
hanging about 20% of the time.

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 50865e7..c678206 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1685,8 +1685,17 @@ static void dw_mci_tasklet_func(unsigned long priv)

                case STATE_DATA_BUSY:
                        if (!test_and_clear_bit(EVENT_DATA_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;
+                       }

                        host->data = NULL;
                        set_bit(EVENT_DATA_COMPLETE, &host->completed_events);
Alexandru Stan (amstan)


On Tue, Dec 2, 2014 at 9:08 PM, Doug Anderson <dianders@chromium.org> wrote:
> Addy,
>
> On Tue, Dec 2, 2014 at 7:16 PM, 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.
>>
>> We don't know why we have this problem, but we need it to fix this problem now.
>> And I will post a follow up change when we find the root cause.
>>
>> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
>> ---
>> Changes in v2:
>> - 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.
>>
>> Changes in v3:
>> - Remove dts for broken-dto, just add this quirk in dw_mci_rockchip_init
>>
>>  drivers/mmc/host/dw_mmc-rockchip.c |  3 +++
>>  drivers/mmc/host/dw_mmc.c          | 41 +++++++++++++++++++++++++++++++++++++-
>>  include/linux/mmc/dw_mmc.h         |  5 +++++
>>  3 files changed, 48 insertions(+), 1 deletion(-)
>
> This seems reasonable to me.  I do hope that you can get to a root
> cause, but I don't think this is a terrible bit of code to carry.
> Obviously it's up to the folks who maintain dw_mmc and the mmc
> subsystem, but:
>
> Reviewed-by: Doug Anderson <dianders@chromium.org>

[PATCH v3] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Alexandru Stan]

Seems like the BROKEN_DTO also affects EVENT_DATA_COMPLETE, sometimes we don't
see that (but we do see EVENT_XFER_COMPLETE), leave the state machine and never
come back to it to timeout. I have this happening on emmc specifically, haven't
seen it on sdmmc yet.

The fix is to also start the timer when we don't see a EVENT_DATA_COMPLETE.

More details for how I debugged this can be found at:
https://chromium-review.googlesource.com/#/c/233691/

In ~100 reboot tests I haven't seen any hangs, whereas before it was
hanging about 20% of the time.

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 50865e7..c678206 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1685,8 +1685,17 @@ static void dw_mci_tasklet_func(unsigned long priv)

                case STATE_DATA_BUSY:
                        if (!test_and_clear_bit(EVENT_DATA_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;
+                       }

                        host->data = NULL;
                        set_bit(EVENT_DATA_COMPLETE, &host->completed_events);
Alexandru Stan (amstan)


On Tue, Dec 2, 2014 at 9:08 PM, Doug Anderson <dianders@chromium.org> wrote:
> Addy,
>
> On Tue, Dec 2, 2014 at 7:16 PM, 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.
>>
>> We don't know why we have this problem, but we need it to fix this problem now.
>> And I will post a follow up change when we find the root cause.
>>
>> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
>> ---
>> Changes in v2:
>> - 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.
>>
>> Changes in v3:
>> - Remove dts for broken-dto, just add this quirk in dw_mci_rockchip_init
>>
>>  drivers/mmc/host/dw_mmc-rockchip.c |  3 +++
>>  drivers/mmc/host/dw_mmc.c          | 41 +++++++++++++++++++++++++++++++++++++-
>>  include/linux/mmc/dw_mmc.h         |  5 +++++
>>  3 files changed, 48 insertions(+), 1 deletion(-)
>
> This seems reasonable to me.  I do hope that you can get to a root
> cause, but I don't think this is a terrible bit of code to carry.
> Obviously it's up to the folks who maintain dw_mmc and the mmc
> subsystem, but:
>
> Reviewed-by: Doug Anderson <dianders@chromium.org>

[PATCH v4] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Addy Ke]

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.

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 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.

There are two possibility that data over interrupt doesn't come in
reading data state on RK3X SoCs:
- get command done interrupt, but doesn't get any data-related interrupt.
- get data error interrupt, but doesn't get data over interrupt.

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

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
Changes in v2:
- 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.

Changes in v3:
- Remove dts for broken-dto, just add this quirk in dw_mci_rockchip_init

Changes in v4:
- fix bug that may cause 32 bit overflow by (drto_clks * 1000).
- doesn't call mod_timer in writing data state, becase TMOUT register only
  for reading data.

 drivers/mmc/host/dw_mmc-rockchip.c |  3 ++
 drivers/mmc/host/dw_mmc.c          | 64 ++++++++++++++++++++++++++++++++++++--
 include/linux/mmc/dw_mmc.h         |  5 +++
 3 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 5650ac4..ba92ebd 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -73,6 +73,9 @@ static int dw_mci_rockchip_init(struct dw_mci *host)
 	/* It is slot 8 on Rockchip SoCs */
 	host->sdio_id0 = 8;
 
+	/* It needs this quirk on all Rockchip SoCs */
+	host->pdata->quirks |= DW_MCI_QUIRK_BROKEN_DTO;
+
 	return 0;
 }
 
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 6e4d864..385dc0f 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1477,6 +1477,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);
 
@@ -1542,8 +1544,22 @@ static void dw_mci_tasklet_func(unsigned long priv)
 			}
 
 			if (!test_and_clear_bit(EVENT_XFER_COMPLETE,
-						&host->pending_events))
+						&host->pending_events)) {
+				drto_clks = mci_readl(host, TMOUT) >> 8;
+				drto_ms = DIV_ROUND_UP(drto_clks,
+						       host->bus_hz / 1000);
+
+				/*
+				 * If all data-related interrupts don't come
+				 * within the given time in reading data state.
+				 * */
+				if ((host->quirks & DW_MCI_QUIRK_BROKEN_DTO) &&
+				    (host->dir_status == DW_MCI_RECV_STATUS)) {
+					mod_timer(&host->dto_timer, jiffies +
+						  msecs_to_jiffies(drto_ms));
+				}
 				break;
+			}
 
 			set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
 
@@ -1573,8 +1589,22 @@ static void dw_mci_tasklet_func(unsigned long priv)
 
 		case STATE_DATA_BUSY:
 			if (!test_and_clear_bit(EVENT_DATA_COMPLETE,
-						&host->pending_events))
+						&host->pending_events)) {
+				drto_clks = mci_readl(host, TMOUT) >> 8;
+				drto_ms = DIV_ROUND_UP(drto_clks,
+						       host->bus_hz / 1000);
+				/*
+				 * If data error interrupt comes but data over
+				 * interrupt doesn't come within the given time.
+				 * in reading data state.
+				 * */
+				if ((host->quirks & DW_MCI_QUIRK_BROKEN_DTO) &&
+				    (host->dir_status == DW_MCI_RECV_STATUS)) {
+					mod_timer(&host->dto_timer, jiffies +
+						  msecs_to_jiffies(drto_ms));
+				}
 				break;
+			}
 
 			host->data = NULL;
 			set_bit(EVENT_DATA_COMPLETE, &host->completed_events);
@@ -2135,6 +2165,10 @@ 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) &&
+			    (host->dir_status == DW_MCI_RECV_STATUS))
+				del_timer(&host->dto_timer);
+
 			mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
 			if (!host->data_status)
 				host->data_status = pending;
@@ -2522,6 +2556,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;
@@ -2670,6 +2726,10 @@ int dw_mci_probe(struct dw_mci *host)
 
 	host->quirks = host->pdata->quirks;
 
+	if (host->quirks & DW_MCI_QUIRK_BROKEN_DTO)
+		setup_timer(&host->dto_timer,
+			    dw_mci_dto_timer, (unsigned long)host);
+
 	spin_lock_init(&host->lock);
 	spin_lock_init(&host->irq_lock);
 	INIT_LIST_HEAD(&host->queue);
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 471fb31..2ece0dd 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
  * =======
@@ -202,6 +203,8 @@ struct dw_mci {
 	int			irq;
 
 	int			sdio_id0;
+
+	struct timer_list       dto_timer;
 };
 
 /* DMA ops for Internal/External DMAC interface */
@@ -226,6 +229,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 */
-- 
1.8.3.2

[PATCH v5] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Addy Ke]

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.

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 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.

There are two possibility that data over interrupt doesn't come in
reading data state on RK3X SoCs:
- get command done interrupt, but doesn't get any data-related interrupt.
- get data error interrupt, but doesn't get data over interrupt.

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

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
Changes in v2:
- 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.

Changes in v3:
- Remove dts for broken-dto, just add this quirk in dw_mci_rockchip_init

Changes in v4:
- fix bug that may cause 32 bit overflow by (drto_clks * 1000).
- doesn't call mod_timer in writing data state, becase TMOUT register only
  for reading data.

Changes in v5:
- fix some typo.
- add a buffer for drto_ms.
- move drto_ms related code to a helper function.

 drivers/mmc/host/dw_mmc-rockchip.c |  3 ++
 drivers/mmc/host/dw_mmc.c          | 71 ++++++++++++++++++++++++++++++++++++--
 include/linux/mmc/dw_mmc.h         |  5 +++
 3 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 5650ac4..ba92ebd 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -73,6 +73,9 @@ static int dw_mci_rockchip_init(struct dw_mci *host)
 	/* It is slot 8 on Rockchip SoCs */
 	host->sdio_id0 = 8;
 
+	/* It needs this quirk on all Rockchip SoCs */
+	host->pdata->quirks |= DW_MCI_QUIRK_BROKEN_DTO;
+
 	return 0;
 }
 
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 6e4d864..ace4b40 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1468,6 +1468,20 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
 	return data->error;
 }
 
+static unsigned int dw_mci_get_drto_ms(struct dw_mci *host)
+{
+	unsigned int drto_clks;
+	unsigned int drto_ms;
+
+	drto_clks = mci_readl(host, TMOUT) >> 8;
+	drto_ms = DIV_ROUND_UP(drto_clks, host->bus_hz / 1000);
+
+	/* add a buffer */
+	drto_ms += 10;
+
+	return drto_ms;
+}
+
 static void dw_mci_tasklet_func(unsigned long priv)
 {
 	struct dw_mci *host = (struct dw_mci *)priv;
@@ -1477,6 +1491,7 @@ 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_ms;
 
 	spin_lock(&host->lock);
 
@@ -1542,8 +1557,19 @@ static void dw_mci_tasklet_func(unsigned long priv)
 			}
 
 			if (!test_and_clear_bit(EVENT_XFER_COMPLETE,
-						&host->pending_events))
+						&host->pending_events)) {
+				/*
+				 * If all data-related interrupts don't come
+				 * within the given time in reading data state.
+				 */
+				if ((host->quirks & DW_MCI_QUIRK_BROKEN_DTO) &&
+				    (host->dir_status == DW_MCI_RECV_STATUS)) {
+					drto_ms = dw_mci_get_drto_ms(host);
+					mod_timer(&host->dto_timer, jiffies +
+						  msecs_to_jiffies(drto_ms));
+				}
 				break;
+			}
 
 			set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
 
@@ -1573,8 +1599,20 @@ static void dw_mci_tasklet_func(unsigned long priv)
 
 		case STATE_DATA_BUSY:
 			if (!test_and_clear_bit(EVENT_DATA_COMPLETE,
-						&host->pending_events))
+						&host->pending_events)) {
+				/*
+				 * If data error interrupt comes but data over
+				 * interrupt doesn't come within the given time.
+				 * in reading data state.
+				 */
+				if ((host->quirks & DW_MCI_QUIRK_BROKEN_DTO) &&
+				    (host->dir_status == DW_MCI_RECV_STATUS)) {
+					drto_ms = dw_mci_get_drto_ms(host);
+					mod_timer(&host->dto_timer, jiffies +
+						  msecs_to_jiffies(drto_ms));
+				}
 				break;
+			}
 
 			host->data = NULL;
 			set_bit(EVENT_DATA_COMPLETE, &host->completed_events);
@@ -2135,6 +2173,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;
@@ -2522,6 +2563,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;
@@ -2670,6 +2733,10 @@ int dw_mci_probe(struct dw_mci *host)
 
 	host->quirks = host->pdata->quirks;
 
+	if (host->quirks & DW_MCI_QUIRK_BROKEN_DTO)
+		setup_timer(&host->dto_timer,
+			    dw_mci_dto_timer, (unsigned long)host);
+
 	spin_lock_init(&host->lock);
 	spin_lock_init(&host->irq_lock);
 	INIT_LIST_HEAD(&host->queue);
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 471fb31..2ece0dd 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
  * =======
@@ -202,6 +203,8 @@ struct dw_mci {
 	int			irq;
 
 	int			sdio_id0;
+
+	struct timer_list       dto_timer;
 };
 
 /* DMA ops for Internal/External DMAC interface */
@@ -226,6 +229,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 */
-- 
1.8.3.2

Re: [PATCH v5] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Jaehoon Chung]

Hi, Addy.

On 01/05/2015 05:21 PM, Addy Ke 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.
> 
> 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 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.
> 
> There are two possibility that data over interrupt doesn't come in
> reading data state on RK3X SoCs:
> - get command done interrupt, but doesn't get any data-related interrupt.
> - get data error interrupt, but doesn't get data over interrupt.
> 
> We don't know why we have this problem, but we need it to fix this problem now.
> And I will post a follow up change when we find the root cause.
> 
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> ---
> Changes in v2:
> - 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.
> 
> Changes in v3:
> - Remove dts for broken-dto, just add this quirk in dw_mci_rockchip_init
> 
> Changes in v4:
> - fix bug that may cause 32 bit overflow by (drto_clks * 1000).
> - doesn't call mod_timer in writing data state, becase TMOUT register only
>   for reading data.
> 
> Changes in v5:
> - fix some typo.
> - add a buffer for drto_ms.
> - move drto_ms related code to a helper function.
> 
>  drivers/mmc/host/dw_mmc-rockchip.c |  3 ++
>  drivers/mmc/host/dw_mmc.c          | 71 ++++++++++++++++++++++++++++++++++++--
>  include/linux/mmc/dw_mmc.h         |  5 +++
>  3 files changed, 77 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> index 5650ac4..ba92ebd 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -73,6 +73,9 @@ static int dw_mci_rockchip_init(struct dw_mci *host)
>  	/* It is slot 8 on Rockchip SoCs */
>  	host->sdio_id0 = 8;
>  
> +	/* It needs this quirk on all Rockchip SoCs */
> +	host->pdata->quirks |= DW_MCI_QUIRK_BROKEN_DTO;
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 6e4d864..ace4b40 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1468,6 +1468,20 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>  	return data->error;
>  }
>  
> +static unsigned int dw_mci_get_drto_ms(struct dw_mci *host)
> +{
> +	unsigned int drto_clks;
> +	unsigned int drto_ms;
> +
> +	drto_clks = mci_readl(host, TMOUT) >> 8;
> +	drto_ms = DIV_ROUND_UP(drto_clks, host->bus_hz / 1000);
> +
> +	/* add a buffer */
> +	drto_ms += 10;

Add a buffer? What means?

> +
> +	return drto_ms;
> +}

dw_mci_set_drto() instead of dw_mci_get_drto_ms().

Then you can include the codes relevant to broken-dto.

static unsigned int (or void) dw_mci_set_drto(struct dw_mci *host) {

	if (!(host->quirks & DW_MCI_QUIRK_BROKEN_DTO))
		return;

	... 
	add codes relevant to broken-dtop (mod_timer) at here.
}

If you need to start timer, you have only to call dw_mci_set_drto().
how about?

Best Regards,
Jaehoon Chung

> +
>  static void dw_mci_tasklet_func(unsigned long priv)
>  {
>  	struct dw_mci *host = (struct dw_mci *)priv;
> @@ -1477,6 +1491,7 @@ 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_ms;
>  
>  	spin_lock(&host->lock);
>  
> @@ -1542,8 +1557,19 @@ static void dw_mci_tasklet_func(unsigned long priv)
>  			}
>  
>  			if (!test_and_clear_bit(EVENT_XFER_COMPLETE,
> -						&host->pending_events))
> +						&host->pending_events)) {
> +				/*
> +				 * If all data-related interrupts don't come
> +				 * within the given time in reading data state.
> +				 */
> +				if ((host->quirks & DW_MCI_QUIRK_BROKEN_DTO) &&
> +				    (host->dir_status == DW_MCI_RECV_STATUS)) {
> +					drto_ms = dw_mci_get_drto_ms(host);
> +					mod_timer(&host->dto_timer, jiffies +
> +						  msecs_to_jiffies(drto_ms));
> +				}
>  				break;
> +			}
>  
>  			set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>  
> @@ -1573,8 +1599,20 @@ static void dw_mci_tasklet_func(unsigned long priv)
>  
>  		case STATE_DATA_BUSY:
>  			if (!test_and_clear_bit(EVENT_DATA_COMPLETE,
> -						&host->pending_events))
> +						&host->pending_events)) {
> +				/*
> +				 * If data error interrupt comes but data over
> +				 * interrupt doesn't come within the given time.
> +				 * in reading data state.
> +				 */
> +				if ((host->quirks & DW_MCI_QUIRK_BROKEN_DTO) &&
> +				    (host->dir_status == DW_MCI_RECV_STATUS)) {
> +					drto_ms = dw_mci_get_drto_ms(host);
> +					mod_timer(&host->dto_timer, jiffies +
> +						  msecs_to_jiffies(drto_ms));
> +				}
>  				break;
> +			}
>  
>  			host->data = NULL;
>  			set_bit(EVENT_DATA_COMPLETE, &host->completed_events);
> @@ -2135,6 +2173,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;
> @@ -2522,6 +2563,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;
> @@ -2670,6 +2733,10 @@ int dw_mci_probe(struct dw_mci *host)
>  
>  	host->quirks = host->pdata->quirks;
>  
> +	if (host->quirks & DW_MCI_QUIRK_BROKEN_DTO)
> +		setup_timer(&host->dto_timer,
> +			    dw_mci_dto_timer, (unsigned long)host);
> +
>  	spin_lock_init(&host->lock);
>  	spin_lock_init(&host->irq_lock);
>  	INIT_LIST_HEAD(&host->queue);
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index 471fb31..2ece0dd 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
>   * =======
> @@ -202,6 +203,8 @@ struct dw_mci {
>  	int			irq;
>  
>  	int			sdio_id0;
> +
> +	struct timer_list       dto_timer;
>  };
>  
>  /* DMA ops for Internal/External DMAC interface */
> @@ -226,6 +229,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 */
> 

[PATCH v5] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Jaehoon Chung]

Hi, Addy.

On 01/05/2015 05:21 PM, Addy Ke 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.
> 
> 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 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.
> 
> There are two possibility that data over interrupt doesn't come in
> reading data state on RK3X SoCs:
> - get command done interrupt, but doesn't get any data-related interrupt.
> - get data error interrupt, but doesn't get data over interrupt.
> 
> We don't know why we have this problem, but we need it to fix this problem now.
> And I will post a follow up change when we find the root cause.
> 
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> ---
> Changes in v2:
> - 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.
> 
> Changes in v3:
> - Remove dts for broken-dto, just add this quirk in dw_mci_rockchip_init
> 
> Changes in v4:
> - fix bug that may cause 32 bit overflow by (drto_clks * 1000).
> - doesn't call mod_timer in writing data state, becase TMOUT register only
>   for reading data.
> 
> Changes in v5:
> - fix some typo.
> - add a buffer for drto_ms.
> - move drto_ms related code to a helper function.
> 
>  drivers/mmc/host/dw_mmc-rockchip.c |  3 ++
>  drivers/mmc/host/dw_mmc.c          | 71 ++++++++++++++++++++++++++++++++++++--
>  include/linux/mmc/dw_mmc.h         |  5 +++
>  3 files changed, 77 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> index 5650ac4..ba92ebd 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -73,6 +73,9 @@ static int dw_mci_rockchip_init(struct dw_mci *host)
>  	/* It is slot 8 on Rockchip SoCs */
>  	host->sdio_id0 = 8;
>  
> +	/* It needs this quirk on all Rockchip SoCs */
> +	host->pdata->quirks |= DW_MCI_QUIRK_BROKEN_DTO;
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 6e4d864..ace4b40 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1468,6 +1468,20 @@ static int dw_mci_data_complete(struct dw_mci *host, struct mmc_data *data)
>  	return data->error;
>  }
>  
> +static unsigned int dw_mci_get_drto_ms(struct dw_mci *host)
> +{
> +	unsigned int drto_clks;
> +	unsigned int drto_ms;
> +
> +	drto_clks = mci_readl(host, TMOUT) >> 8;
> +	drto_ms = DIV_ROUND_UP(drto_clks, host->bus_hz / 1000);
> +
> +	/* add a buffer */
> +	drto_ms += 10;

Add a buffer? What means?

> +
> +	return drto_ms;
> +}

dw_mci_set_drto() instead of dw_mci_get_drto_ms().

Then you can include the codes relevant to broken-dto.

static unsigned int (or void) dw_mci_set_drto(struct dw_mci *host) {

	if (!(host->quirks & DW_MCI_QUIRK_BROKEN_DTO))
		return;

	... 
	add codes relevant to broken-dtop (mod_timer) at here.
}

If you need to start timer, you have only to call dw_mci_set_drto().
how about?

Best Regards,
Jaehoon Chung

> +
>  static void dw_mci_tasklet_func(unsigned long priv)
>  {
>  	struct dw_mci *host = (struct dw_mci *)priv;
> @@ -1477,6 +1491,7 @@ 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_ms;
>  
>  	spin_lock(&host->lock);
>  
> @@ -1542,8 +1557,19 @@ static void dw_mci_tasklet_func(unsigned long priv)
>  			}
>  
>  			if (!test_and_clear_bit(EVENT_XFER_COMPLETE,
> -						&host->pending_events))
> +						&host->pending_events)) {
> +				/*
> +				 * If all data-related interrupts don't come
> +				 * within the given time in reading data state.
> +				 */
> +				if ((host->quirks & DW_MCI_QUIRK_BROKEN_DTO) &&
> +				    (host->dir_status == DW_MCI_RECV_STATUS)) {
> +					drto_ms = dw_mci_get_drto_ms(host);
> +					mod_timer(&host->dto_timer, jiffies +
> +						  msecs_to_jiffies(drto_ms));
> +				}
>  				break;
> +			}
>  
>  			set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
>  
> @@ -1573,8 +1599,20 @@ static void dw_mci_tasklet_func(unsigned long priv)
>  
>  		case STATE_DATA_BUSY:
>  			if (!test_and_clear_bit(EVENT_DATA_COMPLETE,
> -						&host->pending_events))
> +						&host->pending_events)) {
> +				/*
> +				 * If data error interrupt comes but data over
> +				 * interrupt doesn't come within the given time.
> +				 * in reading data state.
> +				 */
> +				if ((host->quirks & DW_MCI_QUIRK_BROKEN_DTO) &&
> +				    (host->dir_status == DW_MCI_RECV_STATUS)) {
> +					drto_ms = dw_mci_get_drto_ms(host);
> +					mod_timer(&host->dto_timer, jiffies +
> +						  msecs_to_jiffies(drto_ms));
> +				}
>  				break;
> +			}
>  
>  			host->data = NULL;
>  			set_bit(EVENT_DATA_COMPLETE, &host->completed_events);
> @@ -2135,6 +2173,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;
> @@ -2522,6 +2563,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;
> @@ -2670,6 +2733,10 @@ int dw_mci_probe(struct dw_mci *host)
>  
>  	host->quirks = host->pdata->quirks;
>  
> +	if (host->quirks & DW_MCI_QUIRK_BROKEN_DTO)
> +		setup_timer(&host->dto_timer,
> +			    dw_mci_dto_timer, (unsigned long)host);
> +
>  	spin_lock_init(&host->lock);
>  	spin_lock_init(&host->irq_lock);
>  	INIT_LIST_HEAD(&host->queue);
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index 471fb31..2ece0dd 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
>   * =======
> @@ -202,6 +203,8 @@ struct dw_mci {
>  	int			irq;
>  
>  	int			sdio_id0;
> +
> +	struct timer_list       dto_timer;
>  };
>  
>  /* DMA ops for Internal/External DMAC interface */
> @@ -226,6 +229,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 */
>