[U-Boot] [Question]AT91: MMC read and multiple read failed

[U-Boot] [Question]AT91: MMC read and multiple read failed

[Song, Elen]

Hello Reinhard:

I've tried your patch([PATCH] AT91: enable MMC on at91sam(9260/9g20/0xe)ek) on uboot-atmel branch on at91sam9260.The sdcard is correctly initialized,but when I test "mmc read" command, I found two issues.

1.while run command "mmc read 22000000 0 1" to read the first block,it will cause "gen_atmel_mci: CMDR 00051051 (17) ARGR 00000000 (SR: 0c400025) Data Transfer Failed".
Both read and write will fail.Sd1.0 and 2.0 cards face the same problem.
2.cmd 18(multiple read) failed,that means can not run "mmc read 22000000 0 2".

Do you have faced the same problem?

To issues1: "Data Transfer Failed" is caused by transfer data lost(not read),if MCCK too high the host may not receive the data.so I try to reduce the MCCK frequency,it works.
To issues2: While multiple reading ,it did not set block count . so multiple read failed because it don't know how many blocks to read.I set reg MCI_BLKR BCNT bit.It is add in gen_atmel_mci.c.

static int mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
{
...
	cmdr = mci_encode_cmd(cmd, data, &error_flags);

	/* Send the command */
	
	+if ((cmd->cmdidx == MMC_CMD_READ_MULTIPLE_BLOCK) ||(cmd->cmdidx == MMC_CMD_WRITE_MULTIPLE_BLOCK))
	+{
	+	writel(data->blocks|mmc->read_bl_len<<16,&mci->blkr);
	+}

	writel(cmd->cmdarg, &mci->argr);
	writel(cmdr, &mci->cmdr);
...

MCI_BLKR should be added in mmc.h:
typedef struct atmel_mci {
	/*	reg	Offset */
	u32	cr;	/* 0x00 */
	u32	mr;	/* 0x04 */
	u32	dtor;	/* 0x08 */
	u32	sdcr;	/* 0x0c */
	u32	argr;	/* 0x10 */
	u32	cmdr;	/* 0x14 */
   - u32	_18;	/* 0x18 */
   + u32	blkr;	/* 0x18 */
...

Do you think it is correct?
In that case,I will deliver a patch.

Best regards
elen.song

[U-Boot] [Question]AT91: MMC read and multiple read failed

[Reinhard Meyer]

Hello Song, Elen,
> Hello Reinhard:
>
> I've tried your patch([PATCH] AT91: enable MMC on at91sam(9260/9g20/0xe)ek) on uboot-atmel branch on at91sam9260.The sdcard is correctly initialized,but when I test "mmc read" command, I found two issues.
>
> 1.while run command "mmc read 22000000 0 1" to read the first block,it will cause "gen_atmel_mci: CMDR 00051051 (17) ARGR 00000000 (SR: 0c400025) Data Transfer Failed".
> Both read and write will fail.Sd1.0 and 2.0 cards face the same problem.
> 2.cmd 18(multiple read) failed,that means can not run "mmc read 22000000 0 2".
>
> Do you have faced the same problem?

All worked fine when I tested the patch late last year.
Since then some changes to common MMC code have been done, perhaps
those affect the Atmel implementation?

>
> To issues1: "Data Transfer Failed" is caused by transfer data lost(not read),if MCCK too high the host may not receive the data.so I try to reduce the MCCK frequency,it works.

No idea, the clock should be computed and set correctly in the code as is.
Have you investigated why it might be set too high for the card in question?
You are testing on a standard at91sam9260-ek? I can only test on a 9xe-ek,
but that should be the same.

> To issues2: While multiple reading ,it did not set block count . so multiple read failed because it don't know how many blocks to read.I set reg MCI_BLKR BCNT bit.It is add in gen_atmel_mci.c.

I think that multiple reading was added to common code.

>
> static int mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
> {
> ...
> 	cmdr = mci_encode_cmd(cmd, data,&error_flags);
>
> 	/* Send the command */
> 	
> 	+if ((cmd->cmdidx == MMC_CMD_READ_MULTIPLE_BLOCK) ||(cmd->cmdidx == MMC_CMD_WRITE_MULTIPLE_BLOCK))
> 	+{
> 	+	writel(data->blocks|mmc->read_bl_len<<16,&mci->blkr);
> 	+}

can't we write that register for all commands (remove the if)?

>
> 	writel(cmd->cmdarg,&mci->argr);
> 	writel(cmdr,&mci->cmdr);
> ...
>
> MCI_BLKR should be added in mmc.h:

atmel_mci.h, I hope...

> typedef struct atmel_mci {
> 	/*	reg	Offset */
> 	u32	cr;	/* 0x00 */
> 	u32	mr;	/* 0x04 */
> 	u32	dtor;	/* 0x08 */
> 	u32	sdcr;	/* 0x0c */
> 	u32	argr;	/* 0x10 */
> 	u32	cmdr;	/* 0x14 */
>     - u32	_18;	/* 0x18 */
>     + u32	blkr;	/* 0x18 */
> ...
>
> Do you think it is correct?

Looks correct by the data sheet. Please verify function in both read and write
modes, make sure the number of blocks read or write is correct.

> In that case,I will deliver a patch.

How does the patch for problem 1. look like?

And please run the patch through <linux>/scripts/checkpatch.pl, I see issues
in the samples you provided above.

Best Regards,
Reinhard

[U-Boot] [Question]AT91: MMC read and multiple read failed

[Song, Elen]

-----Original Message-----
From: Reinhard Meyer [mailto:u-boot at emk-elektronik.de] 
Sent: 2011?7?11? 17:06
To: Song, Elen
Cc: u-boot at lists.denx.de
Subject: Re: [U-Boot] [Question]AT91: MMC read and multiple read failed

>Hello Song, Elen,
>> Hello Reinhard:
>>
>> I've tried your patch([PATCH] AT91: enable MMC on at91sam(9260/9g20/0xe)ek) on uboot-atmel branch on at91sam9260.The sdcard is correctly initialized,but when I test "mmc read" command, I found two issues.
>>
>> 1.while run command "mmc read 22000000 0 1" to read the first block,it will cause "gen_atmel_mci: CMDR 00051051 (17) ARGR 00000000 (SR: 0c400025) Data Transfer Failed".
>> Both read and write will fail.Sd1.0 and 2.0 cards face the same problem.
>> 2.cmd 18(multiple read) failed,that means can not run "mmc read 22000000 0 2".
>>
>> Do you have faced the same problem?

>All worked fine when I tested the patch late last year.
>Since then some changes to common MMC code have been done, perhaps
>those affect the Atmel implementation?
The " Data Transfer Failed" only happens in HC card, normal card does not exist the problem.I test the HC card with "mmc read" several times, fortunately read data is equal to raw data.but I'm not sure if there's any potential problems.
>>
>> To issues1: "Data Transfer Failed" is caused by transfer data lost(not read),if MCCK too high the host may not receive the data.so I try to reduce the MCCK frequency,it works.

>No idea, the clock should be computed and set correctly in the code as is.
>Have you investigated why it might be set too high for the card in question?
>You are testing on a standard at91sam9260-ek? I can only test on a 9xe-ek,
>but that should be the same.
I just found in Atmel_mci.c MCCK is just 5000000(5M),but in mmc.c is 25000000(25M).

>> To issues2: While multiple reading ,it did not set block count . so multiple read failed because it don't know how many blocks to read.I set reg MCI_BLKR >BCNT bit.It is add in gen_atmel_mci.c.

>I think that multiple reading was added to common code.

>>
>> static int mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
>> {
>> ...
>> 	cmdr = mci_encode_cmd(cmd, data,&error_flags);
>>
>> 	/* Send the command */
>> 	
>> 	+if ((cmd->cmdidx == MMC_CMD_READ_MULTIPLE_BLOCK) ||(cmd->cmdidx == MMC_CMD_WRITE_MULTIPLE_BLOCK))
>> 	+{
>> 	+	writel(data->blocks|mmc->read_bl_len<<16,&mci->blkr);
>> 	+}

>can't we write that register for all commands (remove the if)?
I guess no. Other place ,like "sd_switch" function use it ,but it is not a real read or write.so do not need to set block count.I've tested,the code will stuck.
>>
>> 	writel(cmd->cmdarg,&mci->argr);
>> 	writel(cmdr,&mci->cmdr);
>> ...
>>
>> MCI_BLKR should be added in mmc.h:

>atmel_mci.h, I hope...
Yep...

>> typedef struct atmel_mci {
>> 	/*	reg	Offset */
>> 	u32	cr;	/* 0x00 */
>> 	u32	mr;	/* 0x04 */
>> 	u32	dtor;	/* 0x08 */
>> 	u32	sdcr;	/* 0x0c */
>> 	u32	argr;	/* 0x10 */
>> 	u32	cmdr;	/* 0x14 */
>>     - u32	_18;	/* 0x18 */
>>     + u32	blkr;	/* 0x18 */
>> ...
>>
>> Do you think it is correct?

>Looks correct by the data sheet. Please verify function in both read and write
>modes, make sure the number of blocks read or write is correct.
Tried ,it's ok.
> In that case,I will deliver a patch.

>How does the patch for problem 1. look like?
Just:
-mmc_set_clock(mmc, 25000000); 
+mmc_set_clock(mmc, 20000000);
Seems it is a TBD problem ,I think no need now.
>And please run the patch through <linux>/scripts/checkpatch.pl, I see issues
in the samples you provided above.
Ok!

Best Regards,
Elen.song

[U-Boot] [Question]AT91: MMC read and multiple read failed

[Reinhard Meyer]

Dear Song, Elen,
> [snip]
>>> static int mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
>>> {
>>> ...
>>> 	cmdr = mci_encode_cmd(cmd, data,&error_flags);
>>>
>>> 	/* Send the command */
>>> 	
>>> 	+if ((cmd->cmdidx == MMC_CMD_READ_MULTIPLE_BLOCK) ||(cmd->cmdidx == MMC_CMD_WRITE_MULTIPLE_BLOCK))
>>> 	+{
>>> 	+	writel(data->blocks|mmc->read_bl_len<<16,&mci->blkr);
>>> 	+}
> 
>> can't we write that register for all commands (remove the if)?
> I guess no. Other place ,like "sd_switch" function use it ,but it is not a real read or write.so do not need to set block count.I've tested,the code will stuck.
>>>
>>> 	writel(cmd->cmdarg,&mci->argr);
>>> 	writel(cmdr,&mci->cmdr);

I do not think that the code gets stuck while writing a parameter register
with any value, since a command has not been issued to the chip.

It might be that the command gets stuck when an unsuitable value is in one of the
parameter registers.

That itself would require to write another value to BLKR in case of such commands
and not let the value be undefined or residual from a previous command.

>>> [snip]

Another observation: it must be ensured that data->blocks is less than 65536!

Best Regards,
Reinhard

[U-Boot] [Question]AT91: MMC read and multiple read failed

[Song, Elen]

-----Original Message-----
From: Reinhard Meyer [mailto:u-boot at emk-elektronik.de] 
Sent: 2011?7?11? 18:27
To: Song, Elen
Cc: u-boot at lists.denx.de
Subject: Re: [U-Boot] [Question]AT91: MMC read and multiple read failed

Dear Reinhard,
> [snip]
>>> static int mci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
>>> {
>>> ...
>>> 	cmdr = mci_encode_cmd(cmd, data,&error_flags);
>>>
>>> 	/* Send the command */
>>> 	
>>> 	+if ((cmd->cmdidx == MMC_CMD_READ_MULTIPLE_BLOCK) ||(cmd->cmdidx == MMC_CMD_WRITE_MULTIPLE_BLOCK))
>>> 	+{
>>> 	+	writel(data->blocks|mmc->read_bl_len<<16,&mci->blkr);
>>> 	+}
> 
>> can't we write that register for all commands (remove the if)?
> I guess no. Other place ,like "sd_switch" function use it ,but it is not a real read or write.so do not need to set block count.I've tested,the code will stuck.
>>>
>>> 	writel(cmd->cmdarg,&mci->argr);
>>> 	writel(cmdr,&mci->cmdr);

>I do not think that the code gets stuck while writing a parameter register
>with any value, since a command has not been issued to the chip.

>It might be that the command gets stuck when an unsuitable value is in one of >the
>parameter registers.

>That itself would require to write another value to BLKR in case of such commands
>and not let the value be undefined or residual from a previous command.
I think you are right,I printf "data->blocks",that's an unsuitable value. Anyway,I will try to fix it!
>>> [snip]

>Another observation: it must be ensured that data->blocks is less than 65536!

Best Regards,
Elen.song