Re: [PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout
From: Addy
Date: Tue Nov 25 2014 - 01:31:24 EST
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@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/