Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1620012 > unrolled thread

Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error Lee Jones <lee.jones@linaro.org> - 2017-04-10 17:10 +0200
    Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error 冯伟linux <steven_feng@realsil.com.cn> - 2017-04-11 05:50 +0200

#1620012 — Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error

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?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [next] | [standalone]

#1620876

[Multipart message — attachments visible in raw view] — view raw

> 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?
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web