From: Jaehoon Chung <jh80.chung@samsung.com>
To: addy ke <addy.ke@rock-chips.com>,
robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com,
ijc+devicetree@hellion.org.uk, galak@codeaurora.org,
rdunlap@infradead.org, tgih.jun@samsung.com, chris@printf.net,
ulf.hansson@linaro.org, dinguyen@altera.com, heiko@sntech.de,
olof@lixom.net, dianders@chromium.org, sonnyrao@chromium.org,
amstan@chromium.org
Cc: huangtao@rock-chips.com, devicetree@vger.kernel.org,
hl@rock-chips.com, linux-doc@vger.kernel.org, yzq@rock-chips.com,
zyw@rock-chips.com, zhangqing@rock-chips.com,
linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
kever.yang@rock-chips.com, lintao@rock-chips.com,
linux-rockchip@lists.infradead.org, xjq@rock-chips.com,
zhenfu.fang@rock-chips.com, chenfen@rock-chips.com,
cf@rock-chips.com, hj@rock-chips.com,
linux-arm-kernel@lists.infradead.org, zyf@rock-chips.com
Subject: Re: [PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout
Date: Thu, 20 Nov 2014 19:01:05 +0900 [thread overview]
Message-ID: <546DBBE1.1050500@samsung.com> (raw)
In-Reply-To: <546DB55D.90503@rock-chips.com>
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
>>>
>>>
>>>
>>>
>
>
next prev parent reply other threads:[~2014-11-20 10:01 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-14 13:05 [PATCH] mmc: dw_mmc: add quirk for data over interrupt timeout Addy Ke
2014-11-14 13:18 ` Jaehoon Chung
2014-11-18 0:32 ` Addy
2014-11-19 1:22 ` Jaehoon Chung
2014-11-19 5:56 ` addy ke
2014-11-20 9:33 ` addy ke
2014-11-20 10:01 ` Jaehoon Chung [this message]
2014-11-25 6:30 ` Addy
2014-11-25 8:10 ` [PATCH v2] mmc: dw_mmc: add quirk for broken data transfer over scheme Addy Ke
2014-11-26 22:46 ` Doug Anderson
2014-12-02 7:50 ` addy ke
2014-12-02 17:47 ` Doug Anderson
2014-12-03 3:16 ` [PATCH v3] " Addy Ke
2014-12-03 5:08 ` Doug Anderson
2014-12-05 23:56 ` Alexandru Stan
2014-12-26 9:13 ` [PATCH v4] " Addy Ke
2015-01-05 8:21 ` [PATCH v5] " Addy Ke
2015-01-08 1:58 ` Jaehoon Chung
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=546DBBE1.1050500@samsung.com \
--to=jh80.chung@samsung.com \
--cc=addy.ke@rock-chips.com \
--cc=amstan@chromium.org \
--cc=cf@rock-chips.com \
--cc=chenfen@rock-chips.com \
--cc=chris@printf.net \
--cc=devicetree@vger.kernel.org \
--cc=dianders@chromium.org \
--cc=dinguyen@altera.com \
--cc=galak@codeaurora.org \
--cc=heiko@sntech.de \
--cc=hj@rock-chips.com \
--cc=hl@rock-chips.com \
--cc=huangtao@rock-chips.com \
--cc=ijc+devicetree@hellion.org.uk \
--cc=kever.yang@rock-chips.com \
--cc=lintao@rock-chips.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=mark.rutland@arm.com \
--cc=olof@lixom.net \
--cc=pawel.moll@arm.com \
--cc=rdunlap@infradead.org \
--cc=robh+dt@kernel.org \
--cc=sonnyrao@chromium.org \
--cc=tgih.jun@samsung.com \
--cc=ulf.hansson@linaro.org \
--cc=xjq@rock-chips.com \
--cc=yzq@rock-chips.com \
--cc=zhangqing@rock-chips.com \
--cc=zhenfu.fang@rock-chips.com \
--cc=zyf@rock-chips.com \
--cc=zyw@rock-chips.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).