Hi Lee Jones:
I send this email mainly for the fllowing two things;
1.Is there anything unclear about the patch "mfd:rtsx: do retry when
dma transfer error"
2.Whether the pach I submitted in email "[PATCH v4] mfd:rtsx: do
retry when DMA transfer error"
will be merged?
steven feng
Realsil Microelectronics CO. LTD.
Mobile：181-6899-0403 Ext：57594
On 2017年04月11日 11:39, 冯伟linux wrote:
>> This errno need to be -EILSEQ.
>> You need to explain why.
>>
> When DMA transfer error with -EILSEQ, the request will retry some times,
> but when with errno -EINVAL, the request will be aborted directly.
> At the same time the DMA transfer error truely beacuse of the Illegal
> byte sequence,
> so -EILSEQ is used to instead of -EINVAL.
>
>
>>>>>> + if (card_clock == UHS_SDR104_MAX_DTR &&
>>>>>> + pcr->dma_error_count &&
>>>>>> + PCI_PID(pcr) == RTS5227_DEVICE_ID)
>>>>>> + card_clock = (UHS_SDR104_MAX_DTR -
>>>>>> + pcr->dma_error_count * 20000000);
>>>> ... but won't this only reduce the clock frequency just once?
>>>>
>>>> There is no point bracketing the whole statement.
>>>>
>>>> But you do need to bracket one (the second) section of it.
>>>>
>>> The times of DMA transfer error occurrs recorded in dma_error_count,
>>> When DMA transfer error occurrs, the card_clock is reduced by 20MHz.
>> I think you'll find this logic will only reduce the clock frequency by
>> 20MHz once and only once.
>>
>> After the first:
>>
>> card_clock = (UHS_SDR104_MAX_DTR - pcr->dma_error_count * 20000000)
>>
>> ... happens, the first comparison:
>>
>> card_clock == UHS_SDR104_MAX_DTR
>>
>> ... will fail on subsequent attempts and will not allow it to be
>> reduced any further. Did you test it?
>>
> When the request is resent, the card_clock will be still set to
> UHS_SDR104_MAX_DTR,
> so card_clock == UHS_SDR104_MAX_DTR will be always true.
> The times of DMA transfer error occurrs recorded in dma_error_count,
> and the card_clock will be changed to UHS_SDR104_MAX_DTR -
> dma_error_count * 20000000.
> I have tested the code, the finally clock will be reduced step by step
> with the increase of dma_error_count.
>
> steven feng
> Realsil Microelectronics CO. LTD.
> Mobile：181-6899-0403 Ext：57594
>
> On 2017年04月10日 23:00, Lee Jones wrote:
>> On Mon, 10 Apr 2017, 冯伟linux wrote:
>>
>>>> --- a/drivers/mfd/rtsx_pcr.c
>>>> +++ b/drivers/mfd/rtsx_pcr.c
>>>> @@ -30,6 +30,7 @@
>>>> #include <linux/platform_device.h>
>>>> #include <linux/mfd/core.h>
>>>> #include <linux/mfd/rtsx_pci.h>
>>>> +#include <linux/mmc/card.h>
>>>> Why is this required?
>>>>
>>> The UHS_SER104_MAX_DTR which is in "card_clock = UHS_SER104_MAX_DTR
>>> - (pcr->dma_error_count *20000000)" is defined in linux/mmc/card.h, so
>>> it is required.
>> Okay.
>>
>>>> spin_lock_irqsave(&pcr->lock, flags);
>>>> - if (pcr->trans_result == TRANS_RESULT_FAIL)
>>>> - err = -EINVAL;
>>>> + if (pcr->trans_result == TRANS_RESULT_FAIL) {
>>>> + err = -EILSEQ;
>>>> "Illegal byte sequence", really?
>>>>
>>> This errno need to be -EILSEQ.
>> You need to explain why.
>>
>>>>>> + if (card_clock == UHS_SDR104_MAX_DTR &&
>>>>>> + pcr->dma_error_count &&
>>>>>> + PCI_PID(pcr) == RTS5227_DEVICE_ID)
>>>>>> + card_clock = (UHS_SDR104_MAX_DTR -
>>>>>> + pcr->dma_error_count * 20000000);
>>>> ... but won't this only reduce the clock frequency just once?
>>>>
>>>> There is no point bracketing the whole statement.
>>>>
>>>> But you do need to bracket one (the second) section of it.
>>>>
>>> The times of DMA transfer error occurrs recorded in dma_error_count,
>>> When DMA transfer error occurrs, the card_clock is reduced by 20MHz.
>> I think you'll find this logic will only reduce the clock frequency by
>> 20MHz once and only once.
>>
>> After the first:
>>
>> card_clock = (UHS_SDR104_MAX_DTR - pcr->dma_error_count * 20000000)
>>
>> ... happens, the first comparison:
>>
>> card_clock == UHS_SDR104_MAX_DTR
>>
>> ... will fail on subsequent attempts and will not allow it to be
>> reduced any further. Did you test it?
>>
<<attachment: steven_feng.vcf>>
