Re: [PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout
From: Jaehoon Chung
Date: Thu Nov 20 2014 - 05:01:26 EST
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"?
>
> 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@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/