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

[Lee Jones]

On Tue, 25 Apr 2017, åälinux wrote:

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

[PATCH 4] did not hit my inbox.

Please resend it.

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

> begin:vcard
> fn;quoted-printable:=E5=86=AF=E4=BC=9F
> n;quoted-printable:;=E5=86=AF=E4=BC=9F
> email;internet:steven_feng@xxxxxxxxxxxxxx
> tel;cell:18168990403
> version:2.1
> end:vcard
> 


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