[U-Boot] [PATCH 1/3] mmc: limit block count for multi-block write commands

[U-Boot] [PATCH 1/3] mmc: limit block count for multi-block write commands

[Lei Wen]

Signed-off-by: Lei Wen <leiwen@marvell.com>
---
 drivers/mmc/mmc.c |   59 ++++++++++++++++++++++++++++++++++------------------
 1 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index cf4ea16..5cc1904 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -77,29 +77,14 @@ struct mmc *find_mmc_device(int dev_num)
 	return NULL;
 }
 
-static ulong
-mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
+int mmc_write_block(struct mmc *mmc, void *src, ulong start, uint blocknum)
 {
-	struct mmc_cmd cmd;
-	struct mmc_data data;
-	int err;
-	int stoperr = 0;
-	struct mmc *mmc = find_mmc_device(dev_num);
 	int blklen;
-
-	if (!mmc)
-		return -1;
-
 	blklen = mmc->write_bl_len;
 
-	err = mmc_set_blocklen(mmc, mmc->write_bl_len);
-
-	if (err) {
-		printf("set write bl len failed\n\r");
-		return err;
-	}
-
-	if (blkcnt > 1)
+	BUG_ON(blklen * blocknum > 524288);
+	BUG_ON(blocknum > 65535);
+	if (blocknum > 1)
 		cmd.cmdidx = MMC_CMD_WRITE_MULTIPLE_BLOCK;
 	else
 		cmd.cmdidx = MMC_CMD_WRITE_SINGLE_BLOCK;
@@ -113,7 +98,7 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
 	cmd.flags = 0;
 
 	data.src = src;
-	data.blocks = blkcnt;
+	data.blocks = blocknum;
 	data.blocksize = blklen;
 	data.flags = MMC_DATA_WRITE;
 
@@ -124,7 +109,7 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
 		return err;
 	}
 
-	if (blkcnt > 1) {
+	if (blocknum > 1) {
 		cmd.cmdidx = MMC_CMD_STOP_TRANSMISSION;
 		cmd.cmdarg = 0;
 		cmd.resp_type = MMC_RSP_R1b;
@@ -132,6 +117,38 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
 		stoperr = mmc_send_cmd(mmc, &cmd, NULL);
 	}
 
+	return blocknum;
+}
+
+static ulong
+mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
+{
+	struct mmc_cmd cmd;
+	struct mmc_data data;
+	int err;
+	int stoperr = 0;
+	struct mmc *mmc = find_mmc_device(dev_num);
+	unsigned int max;
+	lbaint_t tmp = blkcnt, cur;
+
+	if (!mmc)
+		return -1;
+
+	err = mmc_set_blocklen(mmc, mmc->write_bl_len);
+	if (err) {
+		printf("set write bl len failed\n\r");
+		return err;
+	}
+
+	max = 524288 / mmc->write_bl_len;
+	do {
+		cur = (tmp > max) ? max : tmp;
+		if(mmc_write_block(mmc, src, start, cur) != cur)
+			return -1;
+		tmp -= cur;
+		start += cur;
+		src += cur * mmc->write_bl_len;
+	} while (tmp > 0);
 	return blkcnt;
 }
 
-- 
1.7.0.4

[U-Boot] [PATCH 2/3] mmc: add boundary check for mmc operation

[Lei Wen]

Signed-off-by: Lei Wen <leiwen@marvell.com>
---
 drivers/mmc/mmc.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 5cc1904..9a50b2f 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -134,6 +134,10 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
 	if (!mmc)
 		return -1;
 
+	if (start > mmc->block_dev.lba || (start + blkcnt) > mmc->block_dev.lba) {
+		printf("\noperation excceed mmc boudary..\n");
+		return 0;
+	}
 	err = mmc_set_blocklen(mmc, mmc->write_bl_len);
 	if (err) {
 		printf("set write bl len failed\n\r");
@@ -236,6 +240,10 @@ static ulong mmc_bread(int dev_num, ulong start, lbaint_t blkcnt, void *dst)
 	if (!mmc)
 		return 0;
 
+	if (start > mmc->block_dev.lba || (start + blkcnt) > mmc->block_dev.lba) {
+		printf("\noperation excceed mmc boudary..\n");
+		return 0;
+	}
 	/* We always do full block reads from the card */
 	err = mmc_set_blocklen(mmc, mmc->read_bl_len);
 
-- 
1.7.0.4

[U-Boot] [PATCH 3/3] MMC: print out avaible partition table

[Lei Wen]

Signed-off-by: Lei Wen <leiwen@marvell.com>
---
 common/cmd_mmc.c |   20 ++++++++++++++++++++
 disk/part.c      |    3 +++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
index c0b30d8..af82984 100644
--- a/common/cmd_mmc.c
+++ b/common/cmd_mmc.c
@@ -154,6 +154,25 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			mmc_init(mmc);
 
 			return 0;
+		} else if (strncmp(argv[1], "part", 4) == 0) {
+			int dev = simple_strtoul(argv[2], NULL, 10);
+			block_dev_desc_t *mmc_dev;
+			struct mmc *mmc = find_mmc_device(dev);
+
+			if (!mmc)
+				return 1;
+			mmc_init(mmc);
+			mmc_dev = mmc_get_dev(dev);
+			if (mmc_dev != NULL &&
+					mmc_dev->type != DEV_TYPE_UNKNOWN) {
+				rc = 1;
+				print_part(mmc_dev);
+			}
+			if (!rc) {
+				printf("\nno mmc devices available\n");
+				return 1;
+			}
+			return 0;
 		}
 
 	case 0:
@@ -230,5 +249,6 @@ U_BOOT_CMD(
 	"read <device num> addr blk# cnt\n"
 	"mmc write <device num> addr blk# cnt\n"
 	"mmc rescan <device num>\n"
+	"mmc part <device num>- lists avaiable partition on mmc\n"
 	"mmc list - lists available devices");
 #endif
diff --git a/disk/part.c b/disk/part.c
index 3ba88c7..1806fe6 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -364,6 +364,9 @@ static void print_part_header (const char *type, block_dev_desc_t * dev_desc)
 	case IF_TYPE_DOC:
 		puts ("DOC");
 		break;
+	case IF_TYPE_MMC:
+		puts ("MMC");
+		break;
 	default:
 		puts ("UNKNOWN");
 		break;
-- 
1.7.0.4

[U-Boot] [PATCH 2/3] mmc: add boundary check for mmc operation

[Wolfgang Denk]

Dear Lei Wen,

In message <1283862729-17045-2-git-send-email-leiwen@marvell.com> you wrote:
> Signed-off-by: Lei Wen <leiwen@marvell.com>
> ---
>  drivers/mmc/mmc.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 5cc1904..9a50b2f 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -134,6 +134,10 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
>  	if (!mmc)
>  		return -1;
>  
> +	if (start > mmc->block_dev.lba || (start + blkcnt) > mmc->block_dev.lba) {
> +		printf("\noperation excceed mmc boudary..\n");

No initial newline, please.

Please fix typos: exceed, boundary.

Use puts() instead of printf() for messages that do not need
formatting.

Hm... Maybe you could change the message so it is more informatiove,
i. e. tell the user which boundary was exceeded, and what caused this
condition - is this a user error or a problem in the code?

>  	err = mmc_set_blocklen(mmc, mmc->write_bl_len);
>  	if (err) {
>  		printf("set write bl len failed\n\r");

While you are at it: please fix this message, too. Remove the '\r',
and replace the "bl len" with something the user can understand.

> @@ -236,6 +240,10 @@ static ulong mmc_bread(int dev_num, ulong start, lbaint_t blkcnt, void *dst)
>  	if (!mmc)
>  		return 0;
>  
> +	if (start > mmc->block_dev.lba || (start + blkcnt) > mmc->block_dev.lba) {
> +		printf("\noperation excceed mmc boudary..\n");

See above.

Hm... This is repeated code. Factor it out?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There's an old proverb that says just about whatever you want it to.

[U-Boot] [PATCH 3/3] MMC: print out avaible partition table

[Wolfgang Denk]

Dear Lei Wen,

In message <1283862729-17045-3-git-send-email-leiwen@marvell.com> you wrote:
> Signed-off-by: Lei Wen <leiwen@marvell.com>
> ---
>  common/cmd_mmc.c |   20 ++++++++++++++++++++
>  disk/part.c      |    3 +++
>  2 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
> index c0b30d8..af82984 100644
> --- a/common/cmd_mmc.c
> +++ b/common/cmd_mmc.c
> @@ -154,6 +154,25 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  			mmc_init(mmc);
>  
>  			return 0;
> +		} else if (strncmp(argv[1], "part", 4) == 0) {
> +			int dev = simple_strtoul(argv[2], NULL, 10);
> +			block_dev_desc_t *mmc_dev;
> +			struct mmc *mmc = find_mmc_device(dev);
> +
> +			if (!mmc)
> +				return 1;

Please print error message in case of errors.

> +			mmc_init(mmc);
> +			mmc_dev = mmc_get_dev(dev);
> +			if (mmc_dev != NULL &&
> +					mmc_dev->type != DEV_TYPE_UNKNOWN) {
> +				rc = 1;
> +				print_part(mmc_dev);

Do we need to set rc at all? Just do a "return 0" here.

> +			}
> +			if (!rc) {
> +				printf("\nno mmc devices available\n");
> +				return 1;

No leading newline, please. And use puts() for strings that do not
need formatting.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Youth doesn't excuse everything.
	-- Dr. Janice Lester (in Kirk's body), "Turnabout Intruder",
	   stardate 5928.5.

[U-Boot] [PATCH 1/3] mmc: limit block count for multi-block write commands

[Wolfgang Denk]

Dear Lei Wen,

In message <1283862729-17045-1-git-send-email-leiwen@marvell.com> you wrote:
> Signed-off-by: Lei Wen <leiwen@marvell.com>
> ---
>  drivers/mmc/mmc.c |   59 ++++++++++++++++++++++++++++++++++------------------
>  1 files changed, 38 insertions(+), 21 deletions(-)

> +	BUG_ON(blklen * blocknum > 524288);
> +	BUG_ON(blocknum > 65535);

Please add a comment that explains where these magic numbers are
coming from.

And are you sure that these are conditions where the system should
hang? I think this is not an approrpiate way to handle such errors.
Just failing the command should be enough.

> +	if (blocknum > 1)

I already complained that I consider "blocknum" to be inappropriately
named. It is not a block number (= number of a block), but a block
count.

> -	data.blocks = blkcnt;
> +	data.blocks = blocknum;

The previously used name was way better!

>  	data.blocksize = blklen;
>  	data.flags = MMC_DATA_WRITE;
>  
> @@ -124,7 +109,7 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
>  		return err;
>  	}
>  
> -	if (blkcnt > 1) {
> +	if (blocknum > 1) {
>  		cmd.cmdidx = MMC_CMD_STOP_TRANSMISSION;
>  		cmd.cmdarg = 0;
>  		cmd.resp_type = MMC_RSP_R1b;
> @@ -132,6 +117,38 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
>  		stoperr = mmc_send_cmd(mmc, &cmd, NULL);
>  	}
> +	max = 524288 / mmc->write_bl_len;

Please explain where these magic number is coming from. Eventually use
#defines.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Commitment, n.:      Commitment can be illustrated by a breakfast
of ham and eggs. The chicken was involved, the pig was committed.

[U-Boot] [PATCH 1/3] mmc: limit block count for multi-block write commands

[Reinhard Meyer]

On 07.09.2010 15:57, Wolfgang Denk wrote:
> Dear Lei Wen,
>
> In message<1283862729-17045-1-git-send-email-leiwen@marvell.com>  you wrote:
>> Signed-off-by: Lei Wen<leiwen@marvell.com>
>> ---
>>   drivers/mmc/mmc.c |   59 ++++++++++++++++++++++++++++++++++------------------
>>   1 files changed, 38 insertions(+), 21 deletions(-)
>
>> +	BUG_ON(blklen * blocknum>  524288);
>> +	BUG_ON(blocknum>  65535);
>
> Please add a comment that explains where these magic numbers are
> coming from.
>
> And are you sure that these are conditions where the system should
> hang? I think this is not an approrpiate way to handle such errors.
> Just failing the command should be enough.
>
>> +	if (blocknum>  1)
>
> I already complained that I consider "blocknum" to be inappropriately
> named. It is not a block number (= number of a block), but a block
> count.
>
>> -	data.blocks = blkcnt;
>> +	data.blocks = blocknum;
>
> The previously used name was way better!
>
>>   	data.blocksize = blklen;
>>   	data.flags = MMC_DATA_WRITE;
>>
>> @@ -124,7 +109,7 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
>>   		return err;
>>   	}
>>
>> -	if (blkcnt>  1) {
>> +	if (blocknum>  1) {
>>   		cmd.cmdidx = MMC_CMD_STOP_TRANSMISSION;
>>   		cmd.cmdarg = 0;
>>   		cmd.resp_type = MMC_RSP_R1b;
>> @@ -132,6 +117,38 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
>>   		stoperr = mmc_send_cmd(mmc,&cmd, NULL);
>>   	}
>> +	max = 524288 / mmc->write_bl_len;
>
> Please explain where these magic number is coming from. Eventually use
> #defines.

Besides that my remarks to yesterdays patch of yours are still valid:
Those "magic numbers" are due to a specific hardware controller/limitation
and not any SD/MMC card limitation.

And which hardware driver are you using this with?
Maybe some context would help.
Maybe the splitting could be done in the hardware driver and not in this
generic part?

Reinhard

[U-Boot] [PATCH 1/3] mmc: limit block count for multi-block write commands

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4C865E75.5020409@emk-elektronik.de> you wrote:
>
> Besides that my remarks to yesterdays patch of yours are still valid:
> Those "magic numbers" are due to a specific hardware controller/limitation
> and not any SD/MMC card limitation.

Correct.


> And which hardware driver are you using this with?
> Maybe some context would help.
> Maybe the splitting could be done in the hardware driver and not in this
> generic part?

That would indeed be a better choice.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Evil does seek to maintain power by suppressing the truth."
"Or by misleading the innocent."
	-- Spock and McCoy, "And The Children Shall Lead",
	   stardate 5029.5.

[U-Boot] [PATCH 1/3] mmc: limit block count for multi-block write commands

[Lei Wen]

Hi Reinhard,


> Besides that my remarks to yesterdays patch of yours are still valid:
> Those "magic numbers" are due to a specific hardware controller/limitation
> and not any SD/MMC card limitation.
>
> And which hardware driver are you using this with?
> Maybe some context would help.
> Maybe the splitting could be done in the hardware driver and not in this
> generic part?
>
Certainly those limitation could not be put at the generic part. But
for the common limitation from the spec,
I believe put there is a better choice. I mention for the sd host
controller standard spec has such limitation for a maximum blocks at
one go.
You could refer to the pdf doc at:
http://www.sdcard.org/developers/tech/host_controller/simple_spec/Simplified_SD_Host_Controller_Spec.pdf
The register table at page 20 shows the block count register is only
16 bit width.

For the 524288 magic number, 512k, I just follow the Linux way, that
is don't let the driver to bother to handle the dma boundary if it use
sdma instead PIO.

So it is not which hardware's question.

Best regards,
Lei

[U-Boot] [PATCH 2/3] mmc: add boundary check for mmc operation

[Lei Wen]

On Tue, Sep 7, 2010 at 9:49 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <1283862729-17045-2-git-send-email-leiwen@marvell.com> you wrote:
>> Signed-off-by: Lei Wen <leiwen@marvell.com>
>> ---
>> ?drivers/mmc/mmc.c | ? ?8 ++++++++
>> ?1 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>> index 5cc1904..9a50b2f 100644
>> --- a/drivers/mmc/mmc.c
>> +++ b/drivers/mmc/mmc.c
>> @@ -134,6 +134,10 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
>> ? ? ? if (!mmc)
>> ? ? ? ? ? ? ? return -1;
>>
>> + ? ? if (start > mmc->block_dev.lba || (start + blkcnt) > mmc->block_dev.lba) {
>> + ? ? ? ? ? ? printf("\noperation excceed mmc boudary..\n");
>
> No initial newline, please.

You mean add additional line or just reduce a line here?...
>
> Please fix typos: exceed, boundary.

Ok... Sorry for that...
>
> Use puts() instead of printf() for messages that do not need
> formatting.

Em, ok.
>
> Hm... Maybe you could change the message so it is more informatiove,
> i. e. tell the user which boundary was exceeded, and what caused this
> condition - is this a user error or a problem in the code?

Give error message for each judgment of the two?
>
>> ? ? ? err = mmc_set_blocklen(mmc, mmc->write_bl_len);
>> ? ? ? if (err) {
>> ? ? ? ? ? ? ? printf("set write bl len failed\n\r");
>
> While you are at it: please fix this message, too. Remove the '\r',
> and replace the "bl len" with something the user can understand.
>
>> @@ -236,6 +240,10 @@ static ulong mmc_bread(int dev_num, ulong start, lbaint_t blkcnt, void *dst)
>> ? ? ? if (!mmc)
>> ? ? ? ? ? ? ? return 0;
>>
>> + ? ? if (start > mmc->block_dev.lba || (start + blkcnt) > mmc->block_dev.lba) {
>> + ? ? ? ? ? ? printf("\noperation excceed mmc boudary..\n");
>
> See above.
>
> Hm... This is repeated code. Factor it out?

It is so hard to factor... There is no common up layer calling, or
down layer calling to add this check...
>
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> There's an old proverb that says just about whatever you want it to.
>

[U-Boot] [PATCH 2/3] mmc: add boundary check for mmc operation

[Wolfgang Denk]

Dear Lei Wen,

In message <AANLkTi=m3P2w33-a7-ChpVPwU=XA0WTTL2gHV1dTJOWY@mail.gmail.com> you wrote:
> >>
> >> +       if (start > mmc->block_dev.lba || (start + blkcnt) mmc->block_dev.lba) {
> >> +                   printf("\noperation excceed mmc boudary..\n");
> >
> > No initial newline, please.
>
> You mean add additional line or just reduce a line here?...

I mean change

	printf("\noperation ...");
into
	printf("operation ...");

> > Hm... Maybe you could change the message so it is more informatiove,
> > i. e. tell the user which boundary was exceeded, and what caused this
> > condition - is this a user error or a problem in the code?
>
> Give error message for each judgment of the two?

Not really needed, but you might print the legal range and the current
value in the error message.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Time is a drug. Too much of it kills you.
                                      - Terry Pratchett, _Small Gods_

[U-Boot] [PATCH 1/3] mmc: limit block count for multi-block write commands

[Reinhard Meyer]

Dear Lei Wen,
> Hi Reinhard,
> 
> 
>> Besides that my remarks to yesterdays patch of yours are still valid:
>> Those "magic numbers" are due to a specific hardware controller/limitation
>> and not any SD/MMC card limitation.
>>
>> And which hardware driver are you using this with?
>> Maybe some context would help.
>> Maybe the splitting could be done in the hardware driver and not in this
>> generic part?
>>
> Certainly those limitation could not be put at the generic part. But
> for the common limitation from the spec,
> I believe put there is a better choice. I mention for the sd host
> controller standard spec has such limitation for a maximum blocks at
> one go.
> You could refer to the pdf doc at:
> http://www.sdcard.org/developers/tech/host_controller/simple_spec/Simplified_SD_Host_Controller_Spec.pdf
> The register table at page 20 shows the block count register is only
> 16 bit width.

This is now going in circles...
You keep repeating the same Tantra over and over. You should by now have
realized that this spec gives an EXAMPLE of how a host controller might
be realized. For obvious reasons most designers will NOT follow that
SD-Card specification and might even call their controller a multi-media-card
interface.

So I repeat again: I am sure that many embedded systems
do NOT have such a "Simplified_SD_Host_Controller" that has a
funny 16 Bit Register in its hardware.

> For the 524288 magic number, 512k, I just follow the Linux way, that
> is don't let the driver to bother to handle the dma boundary if it use
> sdma instead PIO.

And again: I am sure most hardware does not have a funny 512k boundary for
DMA operations either. And if, would that boundary not be at a divisible
by 512k memory address (page size maybe) and not at a length of 512k?

> 
> So it is not which hardware's question.

It IS hardware related.

However if agreement exists that the commom MMC code shall limit its
requests to drivers to the most limiting hardware design... be it.

Reinhard

[U-Boot] [PATCH 1/3] mmc: limit block count for multi-block write commands

[Lei Wen]

Hi Reinhard,

On Thu, Sep 9, 2010 at 11:07 PM, Reinhard Meyer
<u-boot@emk-elektronik.de> wrote:
> Dear Lei Wen,
>> Hi Reinhard,
>>
>>
>>> Besides that my remarks to yesterdays patch of yours are still valid:
>>> Those "magic numbers" are due to a specific hardware controller/limitation
>>> and not any SD/MMC card limitation.
>>>
>>> And which hardware driver are you using this with?
>>> Maybe some context would help.
>>> Maybe the splitting could be done in the hardware driver and not in this
>>> generic part?
>>>
>> Certainly those limitation could not be put at the generic part. But
>> for the common limitation from the spec,
>> I believe put there is a better choice. I mention for the sd host
>> controller standard spec has such limitation for a maximum blocks at
>> one go.
>> You could refer to the pdf doc at:
>> http://www.sdcard.org/developers/tech/host_controller/simple_spec/Simplified_SD_Host_Controller_Spec.pdf
>> The register table at page 20 shows the block count register is only
>> 16 bit width.
>
> This is now going in circles...
> You keep repeating the same Tantra over and over. You should by now have
> realized that this spec gives an EXAMPLE of how a host controller might
> be realized. For obvious reasons most designers will NOT follow that
> SD-Card specification and might even call their controller a multi-media-card
> interface.
>
> So I repeat again: I am sure that many embedded systems
> do NOT have such a "Simplified_SD_Host_Controller" that has a
> funny 16 Bit Register in its hardware.

I also agree this discussion should not going into a endless circle...
You says that the sd spec is just example, and mention the mmc.
But you should not forget mmc&sd card are often designed together, so
designer would
choose the sd register layout to get the sd&mmc both works.

If designer don't follow the spec, it is obvious allowable, but this
one just should be a special case.
I believe most others would like to follow the spec directly.

Maybe we could let somebody else to show their choice...


>
>> For the 524288 magic number, 512k, I just follow the Linux way, that
>> is don't let the driver to bother to handle the dma boundary if it use
>> sdma instead PIO.
>
> And again: I am sure most hardware does not have a funny 512k boundary for
> DMA operations either. And if, would that boundary not be at a divisible
> by 512k memory address (page size maybe) and not at a length of 512k?
>
>>
>> So it is not which hardware's question.
>
> It IS hardware related.

Yes, it is hardware related, but I mean not a special hardware...
>
> However if agreement exists that the commom MMC code shall limit its
> requests to drivers to the most limiting hardware design... be it.
>
> Reinhard
>

Best regards,
Lei
>

[U-Boot] [PATCH 1/3] mmc: limit block count for multi-block write commands

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4C88F81E.80604@emk-elektronik.de> you wrote:
>
> > So it is not which hardware's question.
> 
> It IS hardware related.
> 
> However if agreement exists that the commom MMC code shall limit its
> requests to drivers to the most limiting hardware design... be it.

No such agreement exists.

Your arguments are well-heard, and from what I've seen so far I am not
really happy with adding this patch either.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
No one wants war.
	-- Kirk, "Errand of Mercy", stardate 3201.7

[U-Boot] [PATCH 1/3] mmc: limit block count for multi-block write commands

[Wolfgang Denk]

Dear Lei Wen,

In message <AANLkTimNiu6U4XQJU=s71A15oQXOt071O5wdfXx-ehtg@mail.gmail.com> you wrote:
> 
> You says that the sd spec is just example, and mention the mmc.
> But you should not forget mmc&sd card are often designed together, so
> designer would
> choose the sd register layout to get the sd&mmc both works.
> 
> If designer don't follow the spec, it is obvious allowable, but this
> one just should be a special case.
> I believe most others would like to follow the spec directly.

As Reinhard pointed out, the document you quoted is the SD Host
Controller Simplified Specification, but we are talking about MMC code
here, which is a bit more generic. SD Host Controller limitations
should be handled in the SD Host Controller driver code, not in the
MMC generic code.


> >> For the 524288 magic number, 512k, I just follow the Linux way, that
> >> is don't let the driver to bother to handle the dma boundary if it use
> >> sdma instead PIO.
> >
> > And again: I am sure most hardware does not have a funny 512k boundary for
> > DMA operations either. And if, would that boundary not be at a divisible
> > by 512k memory address (page size maybe) and not at a length of 512k?

You did not reply to this argument. Are you really sure the
limitation is on the transfer size?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Any sufficiently advanced  technology  is  indistinguishable  from  a
rigged demo.                              - Andy Finkel, computer guy

[U-Boot] [PATCH 2/3] mmc: add boundary check for mmc operation

[Lei Wen]

Fix it in new version of patch set.

Thanks,
Lei

On Thu, Sep 9, 2010 at 10:51 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <AANLkTi=m3P2w33-a7-ChpVPwU=XA0WTTL2gHV1dTJOWY@mail.gmail.com> you wrote:
>> >>
>> >> + ? ? ? if (start > mmc->block_dev.lba || (start + blkcnt) mmc->block_dev.lba) {
>> >> + ? ? ? ? ? ? ? ? ? printf("\noperation excceed mmc boudary..\n");
>> >
>> > No initial newline, please.
>>
>> You mean add additional line or just reduce a line here?...
>
> I mean change
>
> ? ? ? ?printf("\noperation ...");
> into
> ? ? ? ?printf("operation ...");
>
>> > Hm... Maybe you could change the message so it is more informatiove,
>> > i. e. tell the user which boundary was exceeded, and what caused this
>> > condition - is this a user error or a problem in the code?
>>
>> Give error message for each judgment of the two?
>
> Not really needed, but you might print the legal range and the current
> value in the error message.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> Time is a drug. Too much of it kills you.
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?- Terry Pratchett, _Small Gods_
>