[U-Boot] [PATCH 1/3] mmc: dw_mmc: change the read/write order under fifo mode

[Jaehoon Chung]

On 09/08/2016 12:43 PM, Ziyuan Xu wrote:
> 
> 
> On 2016年09月07日 14:50, Jaehoon Chung wrote:
>> On 09/07/2016 03:14 PM, Ziyuan Xu wrote:
>>> hi Jaehoon,
>>>
>>>
>>> On 2016年08月30日 17:54, Jaehoon Chung wrote:
>>>> Hi Jacob,
>>>>
>>>> On 08/30/2016 02:26 AM, Jacob Chen wrote:
>>>>> From: "jacob2.chen" <jacob2.chen at rock-chips.com>
>>>>>
>>>>> The former implement have a bug.
>>>>> It will cause wrong data reading sometimes.
>>>> Could you explain what bug is there?
>>>>> Signed-off-by: jacob2.chen <jacob2.chen at rock-chips.com>
>>>> Could you change from jacob2.chen to your name?
>>>>
>>>>> ---
>>>>>
>>>>>    drivers/mmc/dw_mmc.c | 32 +++++++++++++++++---------------
>>>>>    1 file changed, 17 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
>>>>> index afc674d..f072739 100644
>>>>> --- a/drivers/mmc/dw_mmc.c
>>>>> +++ b/drivers/mmc/dw_mmc.c
>>>>> @@ -120,35 +120,37 @@ static int dwmci_data_transfer(struct dwmci_host *host, struct mmc_data *data)
>>>>>              if (host->fifo_mode && size) {
>>>>>                len = 0;
>>>>> -            if (data->flags == MMC_DATA_READ) {
>>>>> -                if ((dwmci_readl(host, DWMCI_RINTSTS) &
>>>>> -                     DWMCI_INTMSK_RXDR)) {
>>>>> +            if (data->flags == MMC_DATA_READ &&
>>>>> +                (mask & DWMCI_INTMSK_RXDR)) {
>>>>> +                while (size) {
>>>>>                        len = dwmci_readl(host, DWMCI_STATUS);
>>>>>                        len = (len >> DWMCI_FIFO_SHIFT) &
>>>>> -                            DWMCI_FIFO_MASK;
>>>>> +                        DWMCI_FIFO_MASK;
>>>> this changing is related with bug?
>>> I just hit this bug on rk3036 board.
>> This changing is just an indentation, isn't?
>> It looks like unnecessary modifying.
>>
>>> When DTO interrupt occurred, there are any remaining data still in FIFO due to RX FIFO threshold is larger than remaining data. It also causes that dwmmc didn't trigger RXDR interrupt.
>>> It's responsibility of driver to read remaining bytes on seeing DTO interrupt. So we need rework dwmci_data_transfer w/ pio mode.
>> Sure, your bug is possible to occur..but my mean is that modified DWMCI_FIFO_MASK isn't related with your entire bug.
> 
> Okay I see. If I understand correctly, you recommend that set fifo-depth to 1, so that host can trigger RXDR interrupt as soon as 1 bytes data come in.
> I think it's inefficient.

Well, my meaning is " len = (len >> DWMCI_FIFO_SHIFT) & DWMCI_FIFO_MASK;"

>>>>> -                            DWMCI_FIFO_MASK;
>>>>> +                        DWMCI_FIFO_MASK;

It doesn't need to modify. There is no reason to change.

> 
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>>>                        len = min(size, len);
>>>>>                        for (i = 0; i < len; i++)
>>>>>                            *buf++ =
>>>>> -                        dwmci_readl(host, DWMCI_DATA);
>>>>> -                    dwmci_writel(host, DWMCI_RINTSTS,
>>>>> -                             DWMCI_INTMSK_RXDR);
>>>>> +                            dwmci_readl(host,
>>>>> +                                DWMCI_DATA);
>>>>> +                    size = size > len ? (size - len) : 0;
>>>>>                    }
>>>>> -            } else {
>>>>> -                if ((dwmci_readl(host, DWMCI_RINTSTS) &
>>>>> -                     DWMCI_INTMSK_TXDR)) {
>>>>> +                dwmci_writel(host, DWMCI_RINTSTS,
>>>>> +                         DWMCI_INTMSK_RXDR);
>>>>> +            } else if (data->flags == MMC_DATA_WRITE &&
>>>>> +                   (mask & DWMCI_INTMSK_TXDR)) {
>>>> data->flags == MMC_DATA_WRITE doesn't need..flags are only two..
>>>> one is MMC_DATA_READ, otherwise it's MMC_DATA_WRITE.
>>>>
>>>>> +                while (size) {
>>>>>                        len = dwmci_readl(host, DWMCI_STATUS);
>>>>>                        len = fifo_depth - ((len >>
>>>>> -                           DWMCI_FIFO_SHIFT) &
>>>>> -                           DWMCI_FIFO_MASK);
>>>>> +                                 DWMCI_FIFO_SHIFT) &
>>>>> +                                DWMCI_FIFO_MASK);
>>>> ditto.


>>>>> -                           DWMCI_FIFO_SHIFT) &
>>>>> -                           DWMCI_FIFO_MASK);
>>>>> +                                 DWMCI_FIFO_SHIFT) &
>>>>> +                                DWMCI_FIFO_MASK);

Ditto.

Best Regards,
Jaehoon Chung

>>>>
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>>>                        len = min(size, len);
>>>>>                        for (i = 0; i < len; i++)
>>>>>                            dwmci_writel(host, DWMCI_DATA,
>>>>>                                     *buf++);
>>>>> -                    dwmci_writel(host, DWMCI_RINTSTS,
>>>>> -                             DWMCI_INTMSK_TXDR);
>>>>> +                    size = size > len ? (size - len) : 0;
>>>>>                    }
>>>>> +                dwmci_writel(host, DWMCI_RINTSTS,
>>>>> +                         DWMCI_INTMSK_TXDR);
>>>>>                }
>>>>> -            size = size > len ? (size - len) : 0;
>>>>>            }
>>>>>              /* Data arrived correctly. */
>>>>>
>>>> _______________________________________________
>>>> U-Boot mailing list
>>>> U-Boot at lists.denx.de
>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>>
> 
> 
> 
> 
>