Messages in this thread | | | Date | Tue, 25 Apr 2017 13:26:10 +0100 | From | Lee Jones <> | Subject | Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error |
| |
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
| |