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

From	Lee Jones <lee.jones@linaro.org>
Newsgroups	linux.kernel
Subject	Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error
Date	2017-04-25 14:30 +0200
Message-ID	<tA7Ls-4EF-31@gated-at.bofh.it> (permalink)
References	(2 earlier) <ttA5P-6lG-7@gated-at.bofh.it> <tuDO1-7gt-1@gated-at.bofh.it> <tuJ74-2kr-33@gated-at.bofh.it> <tuUYy-1CP-5@gated-at.bofh.it> <tA3Rx-28G-31@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

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@realsil.com.cn
> 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

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

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
    Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error 冯伟linux <steven_feng@realsil.com.cn> - 2017-04-25 10:20 +0200
      Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error Lee Jones <lee.jones@linaro.org> - 2017-04-25 14:30 +0200

csiph-web