* [PATCH v3] mfd:rtsx: do retry when dma transfer error @ 2017-01-05 9:01 steven_feng 2017-04-07 8:33 ` 冯伟linux 0 siblings, 1 reply; 8+ messages in thread From: steven_feng @ 2017-01-05 9:01 UTC (permalink / raw) To: lee.jones; +Cc: linux-kernel, steven_feng From: steven_feng <steven_feng@realsil.com.cn> the request should be reissued when dma transfer error. for rts5227, the clock freq need to step reduce when error occurred. Signed-off-by: steven_feng <steven_feng@realsil.com.cn> --- drivers/mfd/rtsx_pcr.c | 17 +++++++++++++++-- include/linux/mfd/rtsx_pci.h | 5 +++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c index 98029ee..3161c46 100644 --- 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> #include <asm/unaligned.h> #include "rtsx_pcr.h" @@ -452,8 +453,12 @@ int rtsx_pci_dma_transfer(struct rtsx_pcr *pcr, struct scatterlist *sglist, } spin_lock_irqsave(&pcr->lock, flags); - if (pcr->trans_result == TRANS_RESULT_FAIL) - err = -EINVAL; + if (pcr->trans_result == TRANS_RESULT_FAIL) { + err = -EILSEQ; + if (pcr->dma_error_count < RTS_MAX_TIMES_FREQ_REDUCTION) + pcr->dma_error_count++; + } + else if (pcr->trans_result == TRANS_NO_DEVICE) err = -ENODEV; spin_unlock_irqrestore(&pcr->lock, flags); @@ -659,6 +664,13 @@ int rtsx_pci_switch_clock(struct rtsx_pcr *pcr, unsigned int card_clock, if (err < 0) return err; + /* Each time dma transfer error occurs, card clock decreased by 20MHz */ + 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); + card_clock /= 1000000; pcr_dbg(pcr, "Switch card clock to %dMHz\n", card_clock); @@ -894,6 +906,7 @@ static irqreturn_t rtsx_pci_isr(int irq, void *dev_id) pcr->card_removed |= SD_EXIST; pcr->card_inserted &= ~SD_EXIST; } + pcr->dma_error_count = 0; } if (int_reg & MS_INT) { diff --git a/include/linux/mfd/rtsx_pci.h b/include/linux/mfd/rtsx_pci.h index 7eb7cba..116816f 100644 --- a/include/linux/mfd/rtsx_pci.h +++ b/include/linux/mfd/rtsx_pci.h @@ -850,6 +850,9 @@ #define rtsx_pci_init_cmd(pcr) ((pcr)->ci = 0) +#define RTS5227_DEVICE_ID 0x5227 +#define RTS_MAX_TIMES_FREQ_REDUCTION 8 + struct rtsx_pcr; struct pcr_handle { @@ -957,6 +960,8 @@ struct rtsx_pcr { int num_slots; struct rtsx_slot *slots; + + u8 dma_error_count; }; #define CHK_PCI_PID(pcr, pid) ((pcr)->pci->device == (pid)) -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error 2017-01-05 9:01 [PATCH v3] mfd:rtsx: do retry when dma transfer error steven_feng @ 2017-04-07 8:33 ` 冯伟linux 2017-04-07 11:13 ` Lee Jones 0 siblings, 1 reply; 8+ messages in thread From: 冯伟linux @ 2017-04-07 8:33 UTC (permalink / raw) To: lee.jones; +Cc: linux-kernel [-- Attachment #1: Type: text/plain, Size: 2907 bytes --] HI lee.jones I submitted my patch a few months ago, but the patch was not merged. I want to know what causes this, so that I can update the patch and make the patch merged. steven feng Realsil Microelectronics CO. LTD. Mobile:181-6899-0403 Ext:57594 On 2017年01月05日 17:01, 冯伟linux wrote: > From: steven_feng <steven_feng@realsil.com.cn> > > the request should be reissued when dma transfer error. > for rts5227, the clock freq need to step reduce when error occurred. > > Signed-off-by: steven_feng <steven_feng@realsil.com.cn> > --- > drivers/mfd/rtsx_pcr.c | 17 +++++++++++++++-- > include/linux/mfd/rtsx_pci.h | 5 +++++ > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c > index 98029ee..3161c46 100644 > --- 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> > #include <asm/unaligned.h> > > #include "rtsx_pcr.h" > @@ -452,8 +453,12 @@ int rtsx_pci_dma_transfer(struct rtsx_pcr *pcr, struct scatterlist *sglist, > } > > spin_lock_irqsave(&pcr->lock, flags); > - if (pcr->trans_result == TRANS_RESULT_FAIL) > - err = -EINVAL; > + if (pcr->trans_result == TRANS_RESULT_FAIL) { > + err = -EILSEQ; > + if (pcr->dma_error_count < RTS_MAX_TIMES_FREQ_REDUCTION) > + pcr->dma_error_count++; > + } > + > else if (pcr->trans_result == TRANS_NO_DEVICE) > err = -ENODEV; > spin_unlock_irqrestore(&pcr->lock, flags); > @@ -659,6 +664,13 @@ int rtsx_pci_switch_clock(struct rtsx_pcr *pcr, unsigned int card_clock, > if (err < 0) > return err; > > + /* Each time dma transfer error occurs, card clock decreased by 20MHz */ > + 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); > + > card_clock /= 1000000; > pcr_dbg(pcr, "Switch card clock to %dMHz\n", card_clock); > > @@ -894,6 +906,7 @@ static irqreturn_t rtsx_pci_isr(int irq, void *dev_id) > pcr->card_removed |= SD_EXIST; > pcr->card_inserted &= ~SD_EXIST; > } > + pcr->dma_error_count = 0; > } > > if (int_reg & MS_INT) { > diff --git a/include/linux/mfd/rtsx_pci.h b/include/linux/mfd/rtsx_pci.h > index 7eb7cba..116816f 100644 > --- a/include/linux/mfd/rtsx_pci.h > +++ b/include/linux/mfd/rtsx_pci.h > @@ -850,6 +850,9 @@ > > #define rtsx_pci_init_cmd(pcr) ((pcr)->ci = 0) > > +#define RTS5227_DEVICE_ID 0x5227 > +#define RTS_MAX_TIMES_FREQ_REDUCTION 8 > + > struct rtsx_pcr; > > struct pcr_handle { > @@ -957,6 +960,8 @@ struct rtsx_pcr { > > int num_slots; > struct rtsx_slot *slots; > + > + u8 dma_error_count; > }; > > #define CHK_PCI_PID(pcr, pid) ((pcr)->pci->device == (pid)) [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: steven_feng.vcf --] [-- Type: text/x-vcard; name="steven_feng.vcf", Size: 184 bytes --] 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error 2017-04-07 8:33 ` 冯伟linux @ 2017-04-07 11:13 ` Lee Jones 2017-04-10 9:28 ` 冯伟linux 0 siblings, 1 reply; 8+ messages in thread From: Lee Jones @ 2017-04-07 11:13 UTC (permalink / raw) To: 冯伟linux; +Cc: linux-kernel On Fri, 07 Apr 2017, 冯伟linux wrote: > HI lee.jones > > I submitted my patch a few months ago, but the patch was not merged. > I want to know what causes this, so that I can update the patch and make > the patch merged. The patch was missed. I do not know why. > steven feng > Realsil Microelectronics CO. LTD. > Mobile:181-6899-0403 Ext:57594 > > On 2017年01月05日 17:01, 冯伟linux wrote: > > From: steven_feng <steven_feng@realsil.com.cn> Should be "Steven Feng" > > the request should be reissued when dma transfer error. > > for rts5227, the clock freq need to step reduce when error occurred. Please use correct grammar, including full-stops and capital letters. > > Signed-off-by: steven_feng <steven_feng@realsil.com.cn> "Steven Feng" > > --- > > drivers/mfd/rtsx_pcr.c | 17 +++++++++++++++-- > > include/linux/mfd/rtsx_pci.h | 5 +++++ > > 2 files changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c > > index 98029ee..3161c46 100644 > > --- 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? > > #include <asm/unaligned.h> > > > > #include "rtsx_pcr.h" > > @@ -452,8 +453,12 @@ int rtsx_pci_dma_transfer(struct rtsx_pcr *pcr, struct scatterlist *sglist, > > } > > > > 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? > > + if (pcr->dma_error_count < RTS_MAX_TIMES_FREQ_REDUCTION) > > + pcr->dma_error_count++; > > + } > > + > > else if (pcr->trans_result == TRANS_NO_DEVICE) > > err = -ENODEV; > > spin_unlock_irqrestore(&pcr->lock, flags); > > @@ -659,6 +664,13 @@ int rtsx_pci_switch_clock(struct rtsx_pcr *pcr, unsigned int card_clock, > > if (err < 0) > > return err; > > > > + /* Each time dma transfer error occurs, card clock decreased by 20MHz */ s/dma/DMA/ I would reword as follows: "Reduce MMC card clock by 20MHz each time a DMA transfer error occurs" > > + 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. > > card_clock /= 1000000; > > pcr_dbg(pcr, "Switch card clock to %dMHz\n", card_clock); > > > > @@ -894,6 +906,7 @@ static irqreturn_t rtsx_pci_isr(int irq, void *dev_id) > > pcr->card_removed |= SD_EXIST; > > pcr->card_inserted &= ~SD_EXIST; > > } > > + pcr->dma_error_count = 0; > > } > > > > if (int_reg & MS_INT) { > > diff --git a/include/linux/mfd/rtsx_pci.h b/include/linux/mfd/rtsx_pci.h > > index 7eb7cba..116816f 100644 > > --- a/include/linux/mfd/rtsx_pci.h > > +++ b/include/linux/mfd/rtsx_pci.h > > @@ -850,6 +850,9 @@ > > > > #define rtsx_pci_init_cmd(pcr) ((pcr)->ci = 0) > > > > +#define RTS5227_DEVICE_ID 0x5227 > > +#define RTS_MAX_TIMES_FREQ_REDUCTION 8 > > + > > struct rtsx_pcr; > > > > struct pcr_handle { > > @@ -957,6 +960,8 @@ struct rtsx_pcr { > > > > int num_slots; > > struct rtsx_slot *slots; > > + > > + u8 dma_error_count; > > }; > > > > #define CHK_PCI_PID(pcr, pid) ((pcr)->pci->device == (pid)) > > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error 2017-04-07 11:13 ` Lee Jones @ 2017-04-10 9:28 ` 冯伟linux 2017-04-10 15:00 ` Lee Jones 0 siblings, 1 reply; 8+ messages in thread From: 冯伟linux @ 2017-04-10 9:28 UTC (permalink / raw) To: Lee Jones; +Cc: linux-kernel [-- Attachment #1: Type: text/plain, Size: 5193 bytes --] > --- 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. > 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. >>> + 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. Others are updated in PATCH v4 steven feng Realsil Microelectronics CO. LTD. Mobile:181-6899-0403 Ext:57594 On 2017年04月07日 19:13, Lee Jones wrote: > On Fri, 07 Apr 2017, 冯伟linux wrote: > >> HI lee.jones >> >> I submitted my patch a few months ago, but the patch was not merged. >> I want to know what causes this, so that I can update the patch and make >> the patch merged. > The patch was missed. I do not know why. > >> steven feng >> Realsil Microelectronics CO. LTD. >> Mobile:181-6899-0403 Ext:57594 >> >> On 2017年01月05日 17:01, 冯伟linux wrote: >>> From: steven_feng <steven_feng@realsil.com.cn> > Should be "Steven Feng" > >>> the request should be reissued when dma transfer error. >>> for rts5227, the clock freq need to step reduce when error occurred. > Please use correct grammar, including full-stops and capital letters. > >>> Signed-off-by: steven_feng <steven_feng@realsil.com.cn> > "Steven Feng" > >>> --- >>> drivers/mfd/rtsx_pcr.c | 17 +++++++++++++++-- >>> include/linux/mfd/rtsx_pci.h | 5 +++++ >>> 2 files changed, 20 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c >>> index 98029ee..3161c46 100644 >>> --- 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? > >>> #include <asm/unaligned.h> >>> >>> #include "rtsx_pcr.h" >>> @@ -452,8 +453,12 @@ int rtsx_pci_dma_transfer(struct rtsx_pcr *pcr, struct scatterlist *sglist, >>> } >>> >>> 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? > >>> + if (pcr->dma_error_count < RTS_MAX_TIMES_FREQ_REDUCTION) >>> + pcr->dma_error_count++; >>> + } >>> + >>> else if (pcr->trans_result == TRANS_NO_DEVICE) >>> err = -ENODEV; >>> spin_unlock_irqrestore(&pcr->lock, flags); >>> @@ -659,6 +664,13 @@ int rtsx_pci_switch_clock(struct rtsx_pcr *pcr, unsigned int card_clock, >>> if (err < 0) >>> return err; >>> >>> + /* Each time dma transfer error occurs, card clock decreased by 20MHz */ > s/dma/DMA/ > > I would reword as follows: > > "Reduce MMC card clock by 20MHz each time a DMA transfer error occurs" > >>> + 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. > >>> card_clock /= 1000000; >>> pcr_dbg(pcr, "Switch card clock to %dMHz\n", card_clock); >>> >>> @@ -894,6 +906,7 @@ static irqreturn_t rtsx_pci_isr(int irq, void *dev_id) >>> pcr->card_removed |= SD_EXIST; >>> pcr->card_inserted &= ~SD_EXIST; >>> } >>> + pcr->dma_error_count = 0; >>> } >>> >>> if (int_reg & MS_INT) { >>> diff --git a/include/linux/mfd/rtsx_pci.h b/include/linux/mfd/rtsx_pci.h >>> index 7eb7cba..116816f 100644 >>> --- a/include/linux/mfd/rtsx_pci.h >>> +++ b/include/linux/mfd/rtsx_pci.h >>> @@ -850,6 +850,9 @@ >>> >>> #define rtsx_pci_init_cmd(pcr) ((pcr)->ci = 0) >>> >>> +#define RTS5227_DEVICE_ID 0x5227 >>> +#define RTS_MAX_TIMES_FREQ_REDUCTION 8 >>> + >>> struct rtsx_pcr; >>> >>> struct pcr_handle { >>> @@ -957,6 +960,8 @@ struct rtsx_pcr { >>> >>> int num_slots; >>> struct rtsx_slot *slots; >>> + >>> + u8 dma_error_count; >>> }; >>> >>> #define CHK_PCI_PID(pcr, pid) ((pcr)->pci->device == (pid)) >> 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 >> > [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: steven_feng.vcf --] [-- Type: text/x-vcard; name="steven_feng.vcf", Size: 184 bytes --] 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error 2017-04-10 9:28 ` 冯伟linux @ 2017-04-10 15:00 ` Lee Jones 2017-04-11 3:39 ` 冯伟linux 0 siblings, 1 reply; 8+ messages in thread From: Lee Jones @ 2017-04-10 15:00 UTC (permalink / raw) To: 冯伟linux; +Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error 2017-04-10 15:00 ` Lee Jones @ 2017-04-11 3:39 ` 冯伟linux 2017-04-25 8:13 ` 冯伟linux 0 siblings, 1 reply; 8+ messages in thread From: 冯伟linux @ 2017-04-11 3:39 UTC (permalink / raw) To: Lee Jones; +Cc: linux-kernel [-- Attachment #1: Type: text/plain, Size: 3511 bytes --] > 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? > [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: steven_feng.vcf --] [-- Type: text/x-vcard; name="steven_feng.vcf", Size: 184 bytes --] 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error 2017-04-11 3:39 ` 冯伟linux @ 2017-04-25 8:13 ` 冯伟linux 2017-04-25 12:26 ` Lee Jones 0 siblings, 1 reply; 8+ messages in thread From: 冯伟linux @ 2017-04-25 8:13 UTC (permalink / raw) To: Lee Jones; +Cc: linux-kernel [-- Attachment #1: Type: text/plain, Size: 4044 bytes --] 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? 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? >> [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: steven_feng.vcf --] [-- Type: text/x-vcard; name="steven_feng.vcf", Size: 184 bytes --] 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error 2017-04-25 8:13 ` 冯伟linux @ 2017-04-25 12:26 ` Lee Jones 0 siblings, 0 replies; 8+ messages in thread From: Lee Jones @ 2017-04-25 12:26 UTC (permalink / raw) To: 冯伟linux; +Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-04-25 12:26 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-05 9:01 [PATCH v3] mfd:rtsx: do retry when dma transfer error steven_feng 2017-04-07 8:33 ` 冯伟linux 2017-04-07 11:13 ` Lee Jones 2017-04-10 9:28 ` 冯伟linux 2017-04-10 15:00 ` Lee Jones 2017-04-11 3:39 ` 冯伟linux 2017-04-25 8:13 ` 冯伟linux 2017-04-25 12:26 ` Lee Jones
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.