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

[冯伟linux]

> This seems strange.  What is the reason for using this error?
>
>> +		if (pcr->dma_error_count < 8)
> Why 8?

dma_error_count record the number of dma transfer error,
card clock decreased by 20MHz *dma_error_count, when setting card clock
again.

while dma_error_count == 8, the clock had reduced from 208MHz to 40MHz
The frequency is low enough, if transfer error still occurs, card reset
is needed.


> What does 0x5227 mean?  No magic numbers.  Please define it.
>
>> +		card_clock = UHS_SDR104_MAX_DTR -
>> +			pcr->dma_error_count * 20000000;
> What does this do?

0x5227 is the rts5227's device ID, which is a pcie card reader.
card clock need to decreased by 20MHz * dma_error_count,
because of dma transfer error occured at current  frequency.

> Looks like you need a comment to describe what you're trying to
> achieve.

add it at patch v2

steven feng
Realsil Microelectronics CO. LTD.
Mobile：181-6899-0403  Ext：57594

在 2017年01月04日 01:13, Lee Jones 写道:
> On Fri, 09 Dec 2016, steven_feng@xxxxxxxxxxxxxx wrote:
>
>> From: steven_feng <steven_feng@xxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxx>
>> ---
>>  drivers/mfd/rtsx_pcr.c       | 15 +++++++++++++--
>>  include/linux/mfd/rtsx_pci.h |  2 ++
>>  2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
>> index 98029ee..1684e61 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;
> This seems strange.  What is the reason for using this error?
>
>> +		if (pcr->dma_error_count < 8)
> Why 8?
>
>> +			pcr->dma_error_count++;
>> +	}
>> +
>>  	else if (pcr->trans_result == TRANS_NO_DEVICE)
>>  		err = -ENODEV;
>>  	spin_unlock_irqrestore(&pcr->lock, flags);
>> @@ -659,6 +664,11 @@ int rtsx_pci_switch_clock(struct rtsx_pcr *pcr, unsigned int card_clock,
>>  	if (err < 0)
>>  		return err;
>>  
>> +	if (card_clock == UHS_SDR104_MAX_DTR &&
>> +			pcr->dma_error_count && PCI_PID(pcr) == 0x5227)
> Tabbing is off here.
>
> What does 0x5227 mean?  No magic numbers.  Please define it.
>
>> +		card_clock = UHS_SDR104_MAX_DTR -
>> +			pcr->dma_error_count * 20000000;
> What does this do?
>
> Looks like you need a comment to describe what you're trying to
> achieve.
>
>>  	card_clock /= 1000000;
>>  	pcr_dbg(pcr, "Switch card clock to %dMHz\n", card_clock);
>>  
>> @@ -894,6 +904,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..1d65f4e 100644
>> --- a/include/linux/mfd/rtsx_pci.h
>> +++ b/include/linux/mfd/rtsx_pci.h
>> @@ -957,6 +957,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@xxxxxxxxxxxxxx
tel;cell:18168990403
version:2.1
end:vcard