Messages in this thread Patch in this message | | | From | 冯伟linux <> | Subject | Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error | Date | Tue, 11 Apr 2017 03:39:51 +0000 |
| |
> 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
| |