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

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

[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

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

[冯伟linux]

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

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

[Lee Jones]

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

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

[冯伟linux]

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

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

[Lee Jones]

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

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

[冯伟linux]

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

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

[冯伟linux]

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

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