[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Lei Wen]

As mmc host limitation, the max number of block in one go
should be limited to 65535, and the max buffer size should
not excceed 512k bytes.

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

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 94d1ac2..ea398a5 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -77,29 +77,16 @@ 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, ulong *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;
-
+	int blklen, err;
 	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 +100,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,14 +111,41 @@ 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;
 		cmd.flags = 0;
-		stoperr = mmc_send_cmd(mmc, &cmd, NULL);
+		mmc_send_cmd(mmc, &cmd, NULL);
 	}
 
+	return blocknum;
+}
+
+static ulong
+mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
+{
+	int err;
+	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;
+		tmp -= cur;
+		if(mmc_write_block(mmc, src, start, cur) != cur)
+			return -1;
+	} while (tmp > 0);
 	return blkcnt;
 }
 
-- 
1.7.0.4

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Wolfgang Denk]

Dear Lei Wen,

In message <1283757567-15161-1-git-send-email-leiwen@marvell.com> you wrote:
> As mmc host limitation, the max number of block in one go
> should be limited to 65535, and the max buffer size should
> not excceed 512k bytes.

There is a typo in the Subject ("separate"), and actually I think the
Subject is a bit misleading. How about "limit block count for
multi-block write commands" or so?


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

Be careful with your variable names. Most people will read "blocknum"
as "block number" (and interpret it as an index, i. e. as the logical
block address on the device), while you probably mean "number of
block" - the old name "blkcnt" was more appropriate.


Why is mmc_write_block() not static?


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
SW engineering is a race between programmers trying  to  make  better
idiot-proof programs and the universe producing greater idiots.

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Lei Wen]

On Mon, Sep 6, 2010 at 4:28 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <1283757567-15161-1-git-send-email-leiwen@marvell.com> you wrote:
>> As mmc host limitation, the max number of block in one go
>> should be limited to 65535, and the max buffer size should
>> not excceed 512k bytes.
>
> There is a typo in the Subject ("separate"), and actually I think the
> Subject is a bit misleading. How about "limit block count for
> multi-block write commands" or so?

I agree with that. :-)
>
>
>> - ? ? data.blocks = blkcnt;
>> + ? ? data.blocks = blocknum;
>
> Be careful with your variable names. Most people will read "blocknum"
> as "block number" (and interpret it as an index, i. e. as the logical
> block address on the device), while you probably mean "number of
> block" - the old name "blkcnt" was more appropriate.
>
>
> Why is mmc_write_block() not static?

For mmc.c provide a mmc read api as:
int mmc_read_block(struct mmc *mmc, void *dst, uint blocknum)

So I think it is reasonable to also provide a write api like that,
should it be like?...
Variable blocknum is allow following mmc_read_block style.

>
>
> 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
> SW engineering is a race between programmers trying ?to ?make ?better
> idiot-proof programs and the universe producing greater idiots.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Reinhard Meyer]

Dear Lei Wen,
> As mmc host limitation, the max number of block in one go
> should be limited to 65535, and the max buffer size should
> not excceed 512k bytes.

Where does this limitation supposedly come from?

And, 65535 blocks (of 512) are already near 32 MB.

TOP9000> mmci
mci: setting clock 194000 Hz, block size 512
mci: setting clock 24832000 Hz, block size 512
mci: setting clock 194000 Hz, block size 512
mci: setting clock 194000 Hz, block size 512
mci: setting clock 24832000 Hz, block size 512
Device: mci
Manufacturer ID: 3
OEM: 5344
Name: SD32G
Tran Speed: 25000000
Rd Block Len: 512
SD version 2.0
High Capacity: Yes
Capacity: 31914983424
Bus Width: 4-bit
TOP9000> mmc write 0 20000000 1000 18000

MMC write: dev # 0, block # 4096, count 98304 ... mci: setting clock 194000 Hz,
block size 512
mci: setting clock 24832000 Hz, block size 512
mci: setting clock 194000 Hz, block size 512
mci: setting clock 194000 Hz, block size 512
mci: setting clock 24832000 Hz, block size 512
98304 blocks written: OK
TOP9000>

Reinhard

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Lei Wen]

On Mon, Sep 6, 2010 at 5:23 PM, Reinhard Meyer <u-boot@emk-elektronik.de> wrote:
> Dear Lei Wen,
>> As mmc host limitation, the max number of block in one go
>> should be limited to 65535, and the max buffer size should
>> not excceed 512k bytes.
>
> Where does this limitation supposedly come from?

This limitation comes from the SD/MMC sepc. You could find one and
check the 0x6 offset register(BLOCK COUNT REGISTER).
You could get that register is only 16bit width and only cound contain
the 65535 block.

This register is only validate for multi-operation, like multi-read or
multi-write command.

>
> And, 65535 blocks (of 512) are already near 32 MB.
>
> TOP9000> mmci
> mci: setting clock 194000 Hz, block size 512
> mci: setting clock 24832000 Hz, block size 512
> mci: setting clock 194000 Hz, block size 512
> mci: setting clock 194000 Hz, block size 512
> mci: setting clock 24832000 Hz, block size 512
> Device: mci
> Manufacturer ID: 3
> OEM: 5344
> Name: SD32G
> Tran Speed: 25000000
> Rd Block Len: 512
> SD version 2.0
> High Capacity: Yes
> Capacity: 31914983424
> Bus Width: 4-bit
> TOP9000> mmc write 0 20000000 1000 18000
>
> MMC write: dev # 0, block # 4096, count 98304 ... mci: setting clock 194000 Hz,
> block size 512
> mci: setting clock 24832000 Hz, block size 512
> mci: setting clock 194000 Hz, block size 512
> mci: setting clock 194000 Hz, block size 512
> mci: setting clock 24832000 Hz, block size 512
> 98304 blocks written: OK
> TOP9000>
>
> Reinhard
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Reinhard Meyer]

Lei Wen schrieb:
> On Mon, Sep 6, 2010 at 5:23 PM, Reinhard Meyer <u-boot@emk-elektronik.de> wrote:
>> Dear Lei Wen,
>>> As mmc host limitation, the max number of block in one go

You already write it's a HOST limitation.

>>> should be limited to 65535, and the max buffer size should
>>> not excceed 512k bytes.

Which would limit the #blocks to 1024 (assuming a 512 byte blocks)

>> Where does this limitation supposedly come from?
> 
> This limitation comes from the SD/MMC sepc. You could find one and
> check the 0x6 offset register(BLOCK COUNT REGISTER).

This might refer to certain HOST controllers, but not to Cards!

> You could get that register is only 16bit width and only cound contain
> the 65535 block.
> 
> This register is only validate for multi-operation, like multi-read or
> multi-write command.

Hmm, CMD25 states: "Continuously writes blocks of data until a
STOP_TRANSMISSION follows."

So the Card itself does not limit the number of blocks written in one go.

I do not mind the transfer to be split into chunks if required by some hosts,
but the "number of blocks in a go" should be configurable.

And you still do not explain why the buffer size shall be limited to 512KB?

Best Regards,
Reinhard

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Lei Wen]

On Mon, Sep 6, 2010 at 6:16 PM, Reinhard Meyer <u-boot@emk-elektronik.de> wrote:
> Lei Wen schrieb:
>> On Mon, Sep 6, 2010 at 5:23 PM, Reinhard Meyer <u-boot@emk-elektronik.de> wrote:
>>> Dear Lei Wen,
>>>> As mmc host limitation, the max number of block in one go
>
> You already write it's a HOST limitation.
>
>>>> should be limited to 65535, and the max buffer size should
>>>> not excceed 512k bytes.
>
> Which would limit the #blocks to 1024 (assuming a 512 byte blocks)
>
>>> Where does this limitation supposedly come from?
>>
>> This limitation comes from the SD/MMC sepc. You could find one and
>> check the 0x6 offset register(BLOCK COUNT REGISTER).
>
> This might refer to certain HOST controllers, but not to Cards!

You are right, this comes from HOST, not card. But since the host spec
define so, then is there any host don't follow the sepc? ...

>
>> You could get that register is only 16bit width and only cound contain
>> the 65535 block.
>>
>> This register is only validate for multi-operation, like multi-read or
>> multi-write command.
>
> Hmm, CMD25 states: "Continuously writes blocks of data until a
> STOP_TRANSMISSION follows."
>
> So the Card itself does not limit the number of blocks written in one go.
>
> I do not mind the transfer to be split into chunks if required by some hosts,
> but the "number of blocks in a go" should be configurable.
>
> And you still do not explain why the buffer size shall be limited to 512KB?

The 512 KB comes from the SDMA boundary, and this value is also adopt by Linux.
You could refer to drivers/mmc/host/sdhci.c in Linux code.

This way makes UBOOT don't need to check the boundary interrupt and
make the process more smooth.

>
> Best Regards,
> Reinhard
>
>
Best regards,
Lei

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Reinhard Meyer]

Lei Wen schrieb:
>>>> Where does this limitation supposedly come from?
>>> This limitation comes from the SD/MMC sepc. You could find one and
>>> check the 0x6 offset register(BLOCK COUNT REGISTER).
>> This might refer to certain HOST controllers, but not to Cards!
> 
> You are right, this comes from HOST, not card. But since the host spec
> define so, then is there any host don't follow the sepc? ...

Any that use bit banging, or SPI, and at least: ATMELs MCI.
Probably any number of others that do not copy Intel/PC centered
register structures...

If you had read my first mail on this, there was an example of a 98304
blocks transfer...

> 
>> And you still do not explain why the buffer size shall be limited to 512KB?
> 
> The 512 KB comes from the SDMA boundary, and this value is also adopt by Linux.
> You could refer to drivers/mmc/host/sdhci.c in Linux code.

What hardware, again? sdhci.c? SDMA?

You are trying to change a GENERIC function of U-Boot here to suit a
particular hardware, not a particular hardware driver.

As such, changes should be generic/configurable and not suited to a specific
hardware.

If a specific hardware needs the generic part to obey some limitations, those
limitations should be configurable, like maybe:

#define CONFIG_SYS_MMC_BLOCKLIMIT 65535
#define CONFIG_SYS_MMC_BUFFERLIMIT (512<<10)

Or maybe the splitting/chunking could be resolved in the hardware driver?

Also a clarification of those reasons in the patch comment would be appropriate.

That's my 2 cents for this. Let the community decide if there shall be a general
limitation be put into the generic part...

Best Regards,
Reinhard

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Lei Wen]

Hi Reinhard,

On Mon, Sep 6, 2010 at 7:50 PM, Reinhard Meyer <u-boot@emk-elektronik.de> wrote:
> Lei Wen schrieb:
>>>>> Where does this limitation supposedly come from?
>>>> This limitation comes from the SD/MMC sepc. You could find one and
>>>> check the 0x6 offset register(BLOCK COUNT REGISTER).
>>> This might refer to certain HOST controllers, but not to Cards!
>>
>> You are right, this comes from HOST, not card. But since the host spec
>> define so, then is there any host don't follow the sepc? ...
>
> Any that use bit banging, or SPI, and at least: ATMELs MCI.
> Probably any number of others that do not copy Intel/PC centered
> register structures...
>
> If you had read my first mail on this, there was an example of a 98304
> blocks transfer...

I know that maybe the mmc specific hardware may have no such limitation, but
for our hardware it is sd&mmc cocompatible one, which should follow both spec
and their limitation.

>
>>
>>> And you still do not explain why the buffer size shall be limited to 512KB?
>>
>> The 512 KB comes from the SDMA boundary, and this value is also adopt by Linux.
>> You could refer to drivers/mmc/host/sdhci.c in Linux code.
>
> What hardware, again? sdhci.c? SDMA?

My hardware is pxa168, which has sdhci controller for control both mmc and sd.

>
> You are trying to change a GENERIC function of U-Boot here to suit a
> particular hardware, not a particular hardware driver.
>
> As such, changes should be generic/configurable and not suited to a specific
> hardware.
>

Although the common code is named as mmc.c, I think a lot of people use it
to work for sd too, since they are compatible in most case.

I also try to put this limitation in the driver level, but no lucky,
without the generic change,
the driver cannot do nothing unless a mess change.
For multi-write, it should first send multi-write command, then follow
stop transmission command.
If I implement the seperation in driver, I need to send the stop
transmission command by self...
Then the "generic" one would make no sense...

> If a specific hardware needs the generic part to obey some limitations, those
> limitations should be configurable, like maybe:
>
> #define CONFIG_SYS_MMC_BLOCKLIMIT 65535
> #define CONFIG_SYS_MMC_BUFFERLIMIT (512<<10)
>

I am ok with set a macro for this.

Best regards,
Lei

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Wolfgang Denk]

Dear Lei Wen,

In message <AANLkTi=qBk7N+SdW_6K1=ywKM1ZKo5F-518Q7qceNXws@mail.gmail.com> you wrote:
> 
> Although the common code is named as mmc.c, I think a lot of people use it
> to work for sd too, since they are compatible in most case.

Right. And where SD drivers differ, such differeng code should be
provided by the SD drivers, and not pulled into the common code parts.

> the driver cannot do nothing unless a mess change.

Please explain why that shouldnot be possible. It's the driver that
accesses your hardware, so it has full control over everything going
on.


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
Dealing with failure is easy: work hard to improve. Success  is  also
easy  to  handle:  you've  solved  the  wrong  problem.  Work hard to
improve.

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Lei Wen]

Hi Wolfgang,

On Sun, Oct 10, 2010 at 2:48 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <AANLkTi=qBk7N+SdW_6K1=ywKM1ZKo5F-518Q7qceNXws@mail.gmail.com> you wrote:
>>
>> Although the common code is named as mmc.c, I think a lot of people use it
>> to work for sd too, since they are compatible in most case.
>
> Right. And where SD drivers differ, such differeng code should be
> provided by the SD drivers, and not pulled into the common code parts.
>
>> the driver cannot do nothing unless a mess change.
>
> Please explain why that shouldnot be possible. It's the driver that
> accesses your hardware, so it has full control over everything going
> on.
>
You are right, certainly driver cuold make this, but this would bring
a lot of complexity for it...
As I mentioned in the last mail, the multi-write command semantic
consist by multi-write command + stop transmission command.

Send what kind of command is the job of platform, and how to send the
command is the job of driver.
If I implement this seperation in the driver level, the driver must
send another stop transmission command during each internal
seperation,
which is so ugly and error prone..

So this is why I intend to put the code in the platform level...

Best regards,
Lei

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Wolfgang Denk]

Dear Lei Wen,

In message <AANLkTiny6iZRXoh=Aff1fVPz__B4Sfba1+m+=H97V7ox@mail.gmail.com> you wrote:
> 
> > Please explain why that shouldnot be possible. It's the driver that
> > accesses your hardware, so it has full control over everything going
> > on.
> >
> You are right, certainly driver cuold make this, but this would bring
> a lot of complexity for it...
> As I mentioned in the last mail, the multi-write command semantic
> consist by multi-write command + stop transmission command.
> 
> Send what kind of command is the job of platform, and how to send the
> command is the job of driver.
> If I implement this seperation in the driver level, the driver must
> send another stop transmission command during each internal
> seperation,
> which is so ugly and error prone..
> 
> So this is why I intend to put the code in the platform level...

But my (limited!) understanding is that it's not a platform problem
you are solving, but one of this special kind of controller, which
nobody else would ever run into.

Is this understanding correct?


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
Real computer scientists despise the idea of actual  hardware.  Hard-
ware has limitations, software doesn't. It's a real shame that Turing
machines are so poor at I/O.

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Lei Wen]

Hi Wolfgang,

On Sun, Oct 10, 2010 at 5:22 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <AANLkTiny6iZRXoh=Aff1fVPz__B4Sfba1+m+=H97V7ox@mail.gmail.com> you wrote:
>>
>> > Please explain why that shouldnot be possible. It's the driver that
>> > accesses your hardware, so it has full control over everything going
>> > on.
>> >
>> You are right, certainly driver cuold make this, but this would bring
>> a lot of complexity for it...
>> As I mentioned in the last mail, the multi-write command semantic
>> consist by multi-write command + stop transmission command.
>>
>> Send what kind of command is the job of platform, and how to send the
>> command is the job of driver.
>> If I implement this seperation in the driver level, the driver must
>> send another stop transmission command during each internal
>> seperation,
>> which is so ugly and error prone..
>>
>> So this is why I intend to put the code in the platform level...
>
> But my (limited!) understanding is that it's not a platform problem
> you are solving, but one of this special kind of controller, which
> nobody else would ever run into.
>
> Is this understanding correct?
>

I think it is a generic problem, and I am also curios why other people
don't complain for it...
In Linux code, the multi-write write semantic also has such limitation
in platform code.
So if UBOOT also could have this, that would be so nice...

As you also says that the mmc also server greatly for SD, while the
published SD host controller
spec has such limitation, I think It is general, right? ...

Best regards,
Lei

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Reinhard Meyer]

Dear all concerned,
> But my (limited!) understanding is that it's not a platform problem
> you are solving, but one of this special kind of controller, which
> nobody else would ever run into.

I would vouch to extend

struct mmc {
	struct list_head link;
	char name[32];
	void *priv;
	uint voltages;
	uint version;
	uint f_min;
	uint f_max;
	int high_capacity;
	uint bus_width;
	uint clock;
	uint card_caps;
	uint host_caps;
+	uint host_maxblk;
+	uint host_maxdmalen;
	uint ocr;
	uint scr[2];
	uint csd[4];
	uint cid[4];
	ushort rca;
	uint tran_speed;
	uint read_bl_len;
	uint write_bl_len;
	u64 capacity;
	block_dev_desc_t block_dev;
	int (*send_cmd)(struct mmc *mmc,
			struct mmc_cmd *cmd, struct mmc_data *data);
	void (*set_ios)(struct mmc *mmc);
	int (*init)(struct mmc *mmc);
};

have the driver fill those values and the common mmc part act upon them,
provided they are non-zero. Zero means no limitation on the host side.

This should not break existing code.

I fail, however, to understand the 512 KiB limit on that hardware. It
would allow for only 1024 blocks, despite the 65535 allowed by the register.
Also I assume that limit is about a 512k boundary, not a 512k length???

Reinhard

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Reinhard Meyer]

Dear Lei Wen,
> I think it is a generic problem, and I am also curios why other people
> don't complain for it...

Because it is NOT a generic problem.

> In Linux code, the multi-write write semantic also has such limitation
> in platform code.
> So if UBOOT also could have this, that would be so nice...

In PLATFORM code? Not in GENERIC code?

>
> As you also says that the mmc also server greatly for SD, while the
> published SD host controller
> spec has such limitation, I think It is general, right? ...

You REPEATEDLY state this since weeks, but that "specification" is just
an example of how a minimal controller could be.

Reinhard

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Lei Wen]

Hi Reinhard,

On Sun, Oct 10, 2010 at 5:33 PM, Reinhard Meyer
<u-boot@emk-elektronik.de> wrote:
> Dear all concerned,
>>
>> But my (limited!) understanding is that it's not a platform problem
>> you are solving, but one of this special kind of controller, which
>> nobody else would ever run into.
>
> I would vouch to extend
>
> struct mmc {
> ? ? ? ?struct list_head link;
> ? ? ? ?char name[32];
> ? ? ? ?void *priv;
> ? ? ? ?uint voltages;
> ? ? ? ?uint version;
> ? ? ? ?uint f_min;
> ? ? ? ?uint f_max;
> ? ? ? ?int high_capacity;
> ? ? ? ?uint bus_width;
> ? ? ? ?uint clock;
> ? ? ? ?uint card_caps;
> ? ? ? ?uint host_caps;
> + ? ? ? uint host_maxblk;
> + ? ? ? uint host_maxdmalen;
> ? ? ? ?uint ocr;
> ? ? ? ?uint scr[2];
> ? ? ? ?uint csd[4];
> ? ? ? ?uint cid[4];
> ? ? ? ?ushort rca;
> ? ? ? ?uint tran_speed;
> ? ? ? ?uint read_bl_len;
> ? ? ? ?uint write_bl_len;
> ? ? ? ?u64 capacity;
> ? ? ? ?block_dev_desc_t block_dev;
> ? ? ? ?int (*send_cmd)(struct mmc *mmc,
> ? ? ? ? ? ? ? ? ? ? ? ?struct mmc_cmd *cmd, struct mmc_data *data);
> ? ? ? ?void (*set_ios)(struct mmc *mmc);
> ? ? ? ?int (*init)(struct mmc *mmc);
> };
>
> have the driver fill those values and the common mmc part act upon them,
> provided they are non-zero. Zero means no limitation on the host side.

I like this modification.
>
> This should not break existing code.
>
> I fail, however, to understand the 512 KiB limit on that hardware. It
> would allow for only 1024 blocks, despite the 65535 allowed by the register.
> Also I assume that limit is about a 512k boundary, not a 512k length???
>
Yes, the 512KiB is intend to be a dma boudary.
The 512 KiB is used by Linux, for it didn't want to handle the DMA
interrupt, I guess so.

Best regards,
Lei

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Lei Wen]

On Sun, Oct 10, 2010 at 5:39 PM, Reinhard Meyer
<u-boot@emk-elektronik.de> wrote:
> Dear Lei Wen,
>>
>> I think it is a generic problem, and I am also curios why other people
>> don't complain for it...
>
> Because it is NOT a generic problem.
>
>> In Linux code, the multi-write write semantic also has such limitation
>> in platform code.
>> So if UBOOT also could have this, that would be so nice...
>
> In PLATFORM code? Not in GENERIC code?
>
>>
>> As you also says that the mmc also server greatly for SD, while the
>> published SD host controller
>> spec has such limitation, I think It is general, right? ...
>
> You REPEATEDLY state this since weeks, but that "specification" is just
> an example of how a minimal controller could be.


Sorry for repeating it again and again, what I intend to do is to let
the issue solved
gracefully.

So should I base on your suggestion add two additional member in struct mmc to
get a final solution?

Thanks,
Lei

[U-Boot] [PATCH V2] mmc: seperate block number into small parts for multi-write cmd

[Lei Wen]

As suggested by Reinhard, I add two additional member in mmc structure,
so that we could specify its value in each driver. If that value is 0, then
the behavior would be the same as original, as no seperation.

Signed-off-by: Lei Wen <leiwen@marvell.com>
Cc: Reinhard Meyer <u-boot@emk-elektronik.de>
---
V2: add additional members in mmc structure

 drivers/mmc/bfin_sdh.c      |    1 +
 drivers/mmc/fsl_esdhc.c     |    1 +
 drivers/mmc/gen_atmel_mci.c |    1 +
 drivers/mmc/mmc.c           |   62 +++++++++++++++++++++++++++----------------
 drivers/mmc/mxcmmc.c        |    1 +
 include/mmc.h               |    2 +
 6 files changed, 45 insertions(+), 23 deletions(-)

diff --git a/drivers/mmc/bfin_sdh.c b/drivers/mmc/bfin_sdh.c
index 27d9bf6..6803322 100644
--- a/drivers/mmc/bfin_sdh.c
+++ b/drivers/mmc/bfin_sdh.c
@@ -246,6 +246,7 @@ int bfin_mmc_init(bd_t *bis)
 	if (!mmc)
 		return -ENOMEM;
 	sprintf(mmc->name, "Blackfin SDH");
+	mmc->host_maxblk = mmc->host_maxdmalen = 0;
 	mmc->send_cmd = bfin_sdh_request;
 	mmc->set_ios = bfin_sdh_set_ios;
 	mmc->init = bfin_sdh_init;
diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index a368fe6..ba98de0 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -457,6 +457,7 @@ int fsl_esdhc_initialize(bd_t *bis, struct fsl_esdhc_cfg *cfg)
 	/* First reset the eSDHC controller */
 	esdhc_reset(regs);
 
+	mmc->host_maxblk = mmc->host_maxdmalen = 0;
 	mmc->priv = cfg;
 	mmc->send_cmd = esdhc_send_cmd;
 	mmc->set_ios = esdhc_set_ios;
diff --git a/drivers/mmc/gen_atmel_mci.c b/drivers/mmc/gen_atmel_mci.c
index fa4df99..054f62d 100644
--- a/drivers/mmc/gen_atmel_mci.c
+++ b/drivers/mmc/gen_atmel_mci.c
@@ -332,6 +332,7 @@ int atmel_mci_init(void *regs)
 	if (!mmc)
 		return -1;
 	strcpy(mmc->name, "mci");
+	mmc->host_maxblk = mmc->host_maxdmalen = 0;
 	mmc->priv = regs;
 	mmc->send_cmd = mci_send_cmd;
 	mmc->set_ios = mci_set_ios;
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 80cd9bf..b7bd2f9 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -77,33 +77,13 @@ 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, ulong *src, ulong start, uint blkcnt)
 {
 	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;
+	int blklen, err;
 
 	blklen = mmc->write_bl_len;
-
-	if ((start + blkcnt) > mmc->block_dev.lba) {
-		printf("MMC: block number 0x%lx exceeds max(0x%lx)",
-			start + blkcnt, mmc->block_dev.lba);
-		return 0;
-	}
-	err = mmc_set_blocklen(mmc, mmc->write_bl_len);
-
-	if (err) {
-		printf("set write bl len failed\n\r");
-		return err;
-	}
-
 	if (blkcnt > 1)
 		cmd.cmdidx = MMC_CMD_WRITE_MULTIPLE_BLOCK;
 	else
@@ -134,12 +114,48 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
 		cmd.cmdarg = 0;
 		cmd.resp_type = MMC_RSP_R1b;
 		cmd.flags = 0;
-		stoperr = mmc_send_cmd(mmc, &cmd, NULL);
+		mmc_send_cmd(mmc, &cmd, NULL);
 	}
 
 	return blkcnt;
 }
 
+static ulong
+mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
+{
+	int err;
+	struct mmc *mmc = find_mmc_device(dev_num);
+	unsigned int max;
+	lbaint_t tmp = blkcnt, cur;
+
+	if (!mmc)
+		return -1;
+
+	if ((start + blkcnt ) > mmc->block_dev.lba) {
+		printf("MMC: block number 0x%lx exceeds max(0x%lx)",
+			start + blkcnt, mmc->block_dev.lba);
+		return 0;
+	}
+	err = mmc_set_blocklen(mmc, mmc->write_bl_len);
+	if (err) {
+		printf("set write bl len failed\n\r");
+		return err;
+	}
+
+	max = ((mmc->host_maxdmalen / mmc->write_bl_len) > mmc->host_maxblk)
+	      ? mmc->host_maxblk :(mmc->host_maxdmalen / mmc->write_bl_len);
+	max = (!max) ? tmp : max;
+	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;
+}
+
 int mmc_read_block(struct mmc *mmc, void *dst, uint blocknum)
 {
 	struct mmc_cmd cmd;
diff --git a/drivers/mmc/mxcmmc.c b/drivers/mmc/mxcmmc.c
index 5963953..76501ed 100644
--- a/drivers/mmc/mxcmmc.c
+++ b/drivers/mmc/mxcmmc.c
@@ -497,6 +497,7 @@ static int mxcmci_initialize(bd_t *bis)
 		return -ENOMEM;
 
 	sprintf(mmc->name, "MXC MCI");
+	mmc->host_maxblk = mmc->host_maxdmalen = 0;
 	mmc->send_cmd = mxcmci_request;
 	mmc->set_ios = mxcmci_set_ios;
 	mmc->init = mxcmci_init;
diff --git a/include/mmc.h b/include/mmc.h
index 9f94f42..ad28ad2 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -260,6 +260,8 @@ struct mmc {
 	uint clock;
 	uint card_caps;
 	uint host_caps;
+	uint host_maxblk;
+	uint host_maxdmalen;
 	uint ocr;
 	uint scr[2];
 	uint csd[4];
-- 
1.7.0.4

[U-Boot] [PATCH V2] mmc: seperate block number into small parts for multi-write cmd

[Reinhard Meyer]

Dear Lei Wen,
> As suggested by Reinhard, I add two additional member in mmc structure,
> so that we could specify its value in each driver. If that value is 0, then
> the behavior would be the same as original, as no seperation.

After thinking alot about this:
Preface: (for understanding of the issue)

The high level "driver/part" mmc.c prepares a command structure, a data
structure and passes them to the hardware dependant low level driver:
	err = mmc_send_cmd(mmc, &cmd, &data);
I agree that it would be improper for the low level driver to split that
command into several commands on its own when the data length
cannot be handled by the hardware. That would require repeating some of the
logic that already exists in the high level part. The low level driver should
be and stay as little aware of the command details as possible.

Also some hardware really has only a 16 bit wide block counter. That
includes ATMEL's MCI, but since the data transfer is programmed in a loop,
that register is not used.

I see two possible solutions for that problem here:
1. generally limit the number of blocks requested to 65535. The performance
penalty for that is insignificant. (65535 blocks are about 32 MiB)
2. limit it on a case by case basis by passing such limit like a host
capability to the high level part.

Here I would (after much deliberation) favour version 1.

The second and more serious problem Lei Wen seems to have with his hardware
is that DMA seems problematic (how so?) for more than 512 KiB.
I really don't see that we should pull such (unusual?) limitations into the
high level part. I think the low level driver could issue several DMA
transfers in that case.

We need concensus here first before we can issue and comment patches.

With Best Regards to All,

Reinhard

[U-Boot] [PATCH V2] mmc: seperate block number into small parts for multi-write cmd

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4CB2D225.6020702@emk-elektronik.de> you wrote:
>
> After thinking alot about this:
> Preface: (for understanding of the issue)

Thanks!!

> I see two possible solutions for that problem here:
> 1. generally limit the number of blocks requested to 65535. The performance
> penalty for that is insignificant. (65535 blocks are about 32 MiB)
> 2. limit it on a case by case basis by passing such limit like a host
> capability to the high level part.
> 
> Here I would (after much deliberation) favour version 1.

I agree. Implementing 2) is not worth the effort or the code. 32 MiB
should be enough in most practical cases, and even for really big
transfers it is a more than appropriate chunk size.

> The second and more serious problem Lei Wen seems to have with his hardware
> is that DMA seems problematic (how so?) for more than 512 KiB.

Indeed. So far it is totally unclear to me what sort of restriction
this actually is.  If my interpretation is correct, than such DMA
transfers should not cross 512 KiB memory boundaries, so the maximum
transfer size is 512 KiB, but only when you start on a 512 KiB
aligned address.  Also my interpretation is that this is not actually
a hard restriction, but only a convenience to avoid handling some DMA
interrupt properly - in Linux.  I wonder if this really applied to
U-Boot at all, where we use polling instead.

> I really don't see that we should pull such (unusual?) limitations into the
> high level part. I think the low level driver could issue several DMA
> transfers in that case.

Again I agree. thi is a low-level, controller specific detail, which
should not affect any upper-layer code. It should be completely hidden
in the controller specific driver code.

> We need concensus here first before we can issue and comment patches.

I think both of us agree. We just need to convince Lei Wen...

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
For every problem there is one solution which is  simple,  neat,  and
wrong.                                                - H. L. Mencken

[U-Boot] [PATCH V2] mmc: seperate block number into small parts for multi-write cmd

[Lei Wen]

On Mon, Oct 11, 2010 at 5:22 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Reinhard Meyer,
>
> In message <4CB2D225.6020702@emk-elektronik.de> you wrote:
>>
>> After thinking alot about this:
>> Preface: (for understanding of the issue)
>
> Thanks!!
>
>> I see two possible solutions for that problem here:
>> 1. generally limit the number of blocks requested to 65535. The performance
>> penalty for that is insignificant. (65535 blocks are about 32 MiB)
>> 2. limit it on a case by case basis by passing such limit like a host
>> capability to the high level part.
>>
>> Here I would (after much deliberation) favour version 1.
>
> I agree. Implementing 2) is not worth the effort or the code. 32 MiB
> should be enough in most practical cases, and even for really big
> transfers it is a more than appropriate chunk size.
>
>> The second and more serious problem Lei Wen seems to have with his hardware
>> is that DMA seems problematic (how so?) for more than 512 KiB.
>
> Indeed. So far it is totally unclear to me what sort of restriction
> this actually is. ?If my interpretation is correct, than such DMA
> transfers should not cross 512 KiB memory boundaries, so the maximum
> transfer size is 512 KiB, but only when you start on a 512 KiB
> aligned address. ?Also my interpretation is that this is not actually
> a hard restriction, but only a convenience to avoid handling some DMA
> interrupt properly - in Linux. ?I wonder if this really applied to
> U-Boot at all, where we use polling instead.
>
>> I really don't see that we should pull such (unusual?) limitations into the
>> high level part. I think the low level driver could issue several DMA
>> transfers in that case.
>
> Again I agree. thi is a low-level, controller specific detail, which
> should not affect any upper-layer code. It should be completely hidden
> in the controller specific driver code.
>
>> We need concensus here first before we can issue and comment patches.
>
> I think both of us agree. We just need to convince Lei Wen...

Ok, I am also fine with not include the 512KiB restriction.
So we comes to a conclusion that we still use v1 patch, but cut the
512KiB limitation?

Best regards,
Lei

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Reinhard Meyer]

Dear Lei Wen,

> Ok, I am also fine with not include the 512KiB restriction.
> So we comes to a conclusion that we still use v1 patch, but cut the
> 512KiB limitation?

Considering the comments that were given to that one, yes.

> As mmc host limitation, the max number of block in one go
> should be limited to 65535, and the max buffer size should
> not excceed 512k bytes.

Better reads somehow like this:
Since some hardware has a 16 bit block counter, split the multiple block
write into a maximum of (2**16 - 1) blocks to be written at one go.

> 
> Signed-off-by: Lei Wen <leiwen@marvell.com>
> ---
>  drivers/mmc/mmc.c |   56 +++++++++++++++++++++++++++++++++-------------------
>  1 files changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 94d1ac2..ea398a5 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -77,29 +77,16 @@ 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, ulong *src, ulong start, uint blocknum)
should be:
static ulong mmc_write_blocks(struct_mmc *mmc, ulong start, lbaint_t blkcnt, const void*src)
(keep parameter types and attributes (const) intact and similar to mmc_bwrite)

>  {
>  	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;
> -
> +	int blklen, err;
>  	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);

remove the BUG_ONs

> +	if (blocknum > 1)
>  		cmd.cmdidx = MMC_CMD_WRITE_MULTIPLE_BLOCK;
>  	else
>  		cmd.cmdidx = MMC_CMD_WRITE_SINGLE_BLOCK;
> @@ -113,7 +100,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;

that change not necessary when above parameters are kept.

>  	data.blocksize = blklen;
>  	data.flags = MMC_DATA_WRITE;
>  
> @@ -124,14 +111,41 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
>  		return err;
>  	}
>  
> -	if (blkcnt > 1) {
> +	if (blocknum > 1) {

same here.

>  		cmd.cmdidx = MMC_CMD_STOP_TRANSMISSION;
>  		cmd.cmdarg = 0;
>  		cmd.resp_type = MMC_RSP_R1b;
>  		cmd.flags = 0;
> -		stoperr = mmc_send_cmd(mmc, &cmd, NULL);
> +		mmc_send_cmd(mmc, &cmd, NULL);

should be not keep checking the result for errors?

>  	}
>  
> +	return blocknum;
> +}
> +



> +static ulong
> +mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
> +{
> +	int err;
> +	struct mmc *mmc = find_mmc_device(dev_num);
> +	unsigned int max;

keep related variables the same type: lbainit_t

> +	lbaint_t tmp = blkcnt, cur;

tmp is better named: blocks_todo
put cur into the loop

> +
> +	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;
remove
> +	do {
> +		cur = (tmp > max) ? max : tmp;
	lbaint_t chunk = blocks_todo;
	if (chunk > 65535)
		chunk = 65535;
> +		tmp -= cur;
do that at end of loop
> +		if(mmc_write_block(mmc, src, start, cur) != cur)
		if (mmc_write_blocks(mmc, start, chunk, src) != chunk)
> +			return -1;

I think src, start should be adjusted, too...
	src += chunk * mmc->write_bl_len;
	start += chunk;
	blocks_todo -= chunk;
> +	} while (blocks_todo > 0);
>  	return blkcnt;
>  }
>  

could the functions be reordered such that diff does not mess up so much?
It would be easier to read if the really new function would not be diffed
with the existing function and the modified existing function not seen as
a new one.

I had similar issues with one of my patches and reordered the functions so
diff did "see" it the way it should be... even if that requires a forward
declaration.

Best Regards
Reinhard

[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Wolfgang Denk]

Dear Reinhard Meyer, dear Lei Wen,

In message <4CB2EA80.2090804@emk-elektronik.de> you wrote:
>
> > Ok, I am also fine with not include the 512KiB restriction.
> > So we comes to a conclusion that we still use v1 patch, but cut the
> > 512KiB limitation?
> 
> Considering the comments that were given to that one, yes.

I would like to point out that, from the previous discussion, I
actually think that a 512 KiB limit would be wrong. If the limitation
is really in the alignment, i. e. when crossing a 512 KiB boundary,
then the allowable transfer size depends on the start address, and
512 KiB is actually only the maximum possible value.

> > As mmc host limitation, the max number of block in one go
> > should be limited to 65535, and the max buffer size should
> > not excceed 512k bytes.
> 
> Better reads somehow like this:
> Since some hardware has a 16 bit block counter, split the multiple block
> write into a maximum of (2**16 - 1) blocks to be written at one go.

Agreed.

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
Einstein argued that there must be simplified explanations of nature,
because God is not capricious or arbitrary. No  such  faith  comforts
the software engineer.                             - Fred Brooks, Jr.

[U-Boot] [PATCH V3] mmc: seperate block number into small parts for multi-write cmd

[Lei Wen]

Constraint the mmc framework to only send no more than 65535
blocks in one go during the multi-write command. This constraint
comes due to the limitation of 16bit width block counter register
at some hardware.

Signed-off-by: Lei Wen <leiwen@marvell.com>
Cc: Reinhard Meyer <u-boot@emk-elektronik.de>
---
V3: return to the V1 solution that keep the 65535 constraint,
    but cut the 512KiB dma limitation
V2: add additional members in mmc structure

 drivers/mmc/mmc.c |   52 ++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 80cd9bf..18980e6 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -78,17 +78,11 @@ struct mmc *find_mmc_device(int dev_num)
 }
 
 static ulong
-mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
+mmc_write_blocks(struct mmc *mmc, 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);
-	int blklen;
-
-	if (!mmc)
-		return -1;
+	int blklen, err;
 
 	blklen = mmc->write_bl_len;
 
@@ -97,12 +91,6 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
 			start + blkcnt, mmc->block_dev.lba);
 		return 0;
 	}
-	err = mmc_set_blocklen(mmc, mmc->write_bl_len);
-
-	if (err) {
-		printf("set write bl len failed\n\r");
-		return err;
-	}
 
 	if (blkcnt > 1)
 		cmd.cmdidx = MMC_CMD_WRITE_MULTIPLE_BLOCK;
@@ -123,7 +111,6 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
 	data.flags = MMC_DATA_WRITE;
 
 	err = mmc_send_cmd(mmc, &cmd, &data);
-
 	if (err) {
 		printf("mmc write failed\n\r");
 		return err;
@@ -134,9 +121,42 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
 		cmd.cmdarg = 0;
 		cmd.resp_type = MMC_RSP_R1b;
 		cmd.flags = 0;
-		stoperr = mmc_send_cmd(mmc, &cmd, NULL);
+		err = mmc_send_cmd(mmc, &cmd, NULL);
+		if (err) {
+			printf("mmc fail to send stop cmd\n\r");
+			return err;
+		}
+	}
+
+	return blkcnt;
+}
+
+static ulong
+mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
+{
+	int err;
+	struct mmc *mmc = find_mmc_device(dev_num);
+	lbaint_t cur, blocks_todo = blkcnt;
+
+	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;
 	}
 
+	do {
+		/* The 65535 constraint comes from some hardware has
+		 * only 16 bit width block number counter */
+		cur = (blocks_todo > 65535) ? 65535 : blocks_todo;
+		if(mmc_write_blocks(mmc, start, cur, src) != cur)
+			return -1;
+		blocks_todo -= cur;
+		start += cur;
+		src += cur * mmc->write_bl_len;
+	} while (blocks_todo > 0);
 	return blkcnt;
 }
 
-- 
1.7.0.4

[U-Boot] [PATCH V3] mmc: seperate block number into small parts for multi-write cmd

[Wolfgang Denk]

Dear Lei Wen,

In message <1286811544-9312-1-git-send-email-leiwen@marvell.com> you wrote:
> Constraint the mmc framework to only send no more than 65535
> blocks in one go during the multi-write command. This constraint
> comes due to the limitation of 16bit width block counter register
> at some hardware.
...

> @@ -123,7 +111,6 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
>  	data.flags = MMC_DATA_WRITE;
>  
>  	err = mmc_send_cmd(mmc, &cmd, &data);
> -
>  	if (err) {
>  		printf("mmc write failed\n\r");
>  		return err;

Please don;t drop this empty line.

> +	do {
> +		/* The 65535 constraint comes from some hardware has
> +		 * only 16 bit width block number counter */

Incorrect multi-line comment style.

> +		cur = (blocks_todo > 65535) ? 65535 : blocks_todo;
> +		if(mmc_write_blocks(mmc, start, cur, src) != cur)
> +			return -1;
> +		blocks_todo -= cur;
> +		start += cur;
> +		src += cur * mmc->write_bl_len;
> +	} while (blocks_todo > 0);
>  	return blkcnt;

Please add a blank line before the "return".

Thanks.

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
The games have always  strengthened  us.  Death  becomes  a  familiar
pattern. We don't fear it as you do.
	-- Proconsul Marcus Claudius, "Bread and Circuses",
	   stardate 4041.2

[U-Boot] [PATCH V4] mmc: seperate block number into small parts for multi-write cmd

[Lei Wen]

Constraint the mmc framework to only send no more than 65535
blocks in one go during the multi-write command. This constraint
comes due to the limitation of 16bit width block counter register
at some hardware.

Signed-off-by: Lei Wen <leiwen@marvell.com>
Cc: Reinhard Meyer <u-boot@emk-elektronik.de>
---
V4: clean patch
V3: return to the V1 solution that keep the 65535 constraint,
    but cut the 512KiB dma limitation
V2: add additional members in mmc structure

 drivers/mmc/mmc.c |   54 ++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 80cd9bf..4567e36 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -78,17 +78,11 @@ struct mmc *find_mmc_device(int dev_num)
 }
 
 static ulong
-mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
+mmc_write_blocks(struct mmc *mmc, 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);
-	int blklen;
-
-	if (!mmc)
-		return -1;
+	int blklen, err;
 
 	blklen = mmc->write_bl_len;
 
@@ -97,12 +91,6 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
 			start + blkcnt, mmc->block_dev.lba);
 		return 0;
 	}
-	err = mmc_set_blocklen(mmc, mmc->write_bl_len);
-
-	if (err) {
-		printf("set write bl len failed\n\r");
-		return err;
-	}
 
 	if (blkcnt > 1)
 		cmd.cmdidx = MMC_CMD_WRITE_MULTIPLE_BLOCK;
@@ -134,9 +122,45 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
 		cmd.cmdarg = 0;
 		cmd.resp_type = MMC_RSP_R1b;
 		cmd.flags = 0;
-		stoperr = mmc_send_cmd(mmc, &cmd, NULL);
+		err = mmc_send_cmd(mmc, &cmd, NULL);
+		if (err) {
+			printf("mmc fail to send stop cmd\n\r");
+			return err;
+		}
+	}
+
+	return blkcnt;
+}
+
+static ulong
+mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
+{
+	int err;
+	struct mmc *mmc = find_mmc_device(dev_num);
+	lbaint_t cur, blocks_todo = blkcnt;
+
+	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;
 	}
 
+	do {
+		/*
+		 * The 65535 constraint comes from some hardware has
+		 * only 16 bit width block number counter
+		 */
+		cur = (blocks_todo > 65535) ? 65535 : blocks_todo;
+		if(mmc_write_blocks(mmc, start, cur, src) != cur)
+			return -1;
+		blocks_todo -= cur;
+		start += cur;
+		src += cur * mmc->write_bl_len;
+	} while (blocks_todo > 0);
+
 	return blkcnt;
 }
 
-- 
1.7.0.4

[U-Boot] [PATCH V4] mmc: seperate block number into small parts for multi-write cmd

[Steve Sakoman]

On Wed, Oct 13, 2010 at 10:38 PM, Lei Wen <leiwen@marvell.com> wrote:
> Constraint the mmc framework to only send no more than 65535
> blocks in one go during the multi-write command. This constraint
> comes due to the limitation of 16bit width block counter register
> at some hardware.
>
> Signed-off-by: Lei Wen <leiwen@marvell.com>
> Cc: Reinhard Meyer <u-boot@emk-elektronik.de>
> ---
> V4: clean patch
> V3: return to the V1 solution that keep the 65535 constraint,
> ? ?but cut the 512KiB dma limitation
> V2: add additional members in mmc structure

What is the status of this patch?

I would like to submit a multi-block read patch and need to know
whether to assume that this patch will be pushed.

Regards,

Steve

[U-Boot] [PATCH V3] mmc: seperate block number into small parts for multi-write cmd

[Lei Wen]

Hi Wolfgang,

How about merging this patch? I have the same concern with Steve Sakoman on
the point of improving mmc operation performance in UBOOT.

Best regards,
Lei

On Thu, Oct 14, 2010 at 3:57 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <1286811544-9312-1-git-send-email-leiwen@marvell.com> you wrote:
>> Constraint the mmc framework to only send no more than 65535
>> blocks in one go during the multi-write command. This constraint
>> comes due to the limitation of 16bit width block counter register
>> at some hardware.
> ...
>
>> @@ -123,7 +111,6 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
>> ? ? ? data.flags = MMC_DATA_WRITE;
>>
>> ? ? ? err = mmc_send_cmd(mmc, &cmd, &data);
>> -
>> ? ? ? if (err) {
>> ? ? ? ? ? ? ? printf("mmc write failed\n\r");
>> ? ? ? ? ? ? ? return err;
>
> Please don;t drop this empty line.
>
>> + ? ? do {
>> + ? ? ? ? ? ? /* The 65535 constraint comes from some hardware has
>> + ? ? ? ? ? ? ?* only 16 bit width block number counter */
>
> Incorrect multi-line comment style.
>
>> + ? ? ? ? ? ? cur = (blocks_todo > 65535) ? 65535 : blocks_todo;
>> + ? ? ? ? ? ? if(mmc_write_blocks(mmc, start, cur, src) != cur)
>> + ? ? ? ? ? ? ? ? ? ? return -1;
>> + ? ? ? ? ? ? blocks_todo -= cur;
>> + ? ? ? ? ? ? start += cur;
>> + ? ? ? ? ? ? src += cur * mmc->write_bl_len;
>> + ? ? } while (blocks_todo > 0);
>> ? ? ? return blkcnt;
>
> Please add a blank line before the "return".
>
> Thanks.
>
> 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
> The games have always ?strengthened ?us. ?Death ?becomes ?a ?familiar
> pattern. We don't fear it as you do.
> ? ? ? ?-- Proconsul Marcus Claudius, "Bread and Circuses",
> ? ? ? ? ? stardate 4041.2
>

[U-Boot] [PATCH V4] mmc: seperate block number into small parts for multi-write cmd

[Wolfgang Denk]

Dear Lei Wen,

In message <1287034691-26278-1-git-send-email-leiwen@marvell.com> you wrote:
> Constraint the mmc framework to only send no more than 65535
> blocks in one go during the multi-write command. This constraint
> comes due to the limitation of 16bit width block counter register
> at some hardware.
> 
> Signed-off-by: Lei Wen <leiwen@marvell.com>
> Cc: Reinhard Meyer <u-boot@emk-elektronik.de>
> ---
> V4: clean patch
> V3: return to the V1 solution that keep the 65535 constraint,
>     but cut the 512KiB dma limitation
> V2: add additional members in mmc structure
> 
>  drivers/mmc/mmc.c |   54 ++++++++++++++++++++++++++++++++++++++--------------
>  1 files changed, 39 insertions(+), 15 deletions(-)

Applied, thanks.

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
I have made mistakes, but have never made the mistake of  claiming  I
never made one.                                     - James G. Bennet

[U-Boot] [PATCH V4] mmc: seperate block number into small parts for multi-write cmd

[Steve Sakoman]

On Wed, Oct 27, 2010 at 11:39 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <1287034691-26278-1-git-send-email-leiwen@marvell.com> you wrote:
>> Constraint the mmc framework to only send no more than 65535
>> blocks in one go during the multi-write command. This constraint
>> comes due to the limitation of 16bit width block counter register
>> at some hardware.
>>
>> Signed-off-by: Lei Wen <leiwen@marvell.com>
>> Cc: Reinhard Meyer <u-boot@emk-elektronik.de>
>> ---
>> V4: clean patch
>> V3: return to the V1 solution that keep the 65535 constraint,
>> ? ? but cut the 512KiB dma limitation
>> V2: add additional members in mmc structure
>>
>> ?drivers/mmc/mmc.c | ? 54 ++++++++++++++++++++++++++++++++++++++--------------
>> ?1 files changed, 39 insertions(+), 15 deletions(-)
>
> Applied, thanks.

Thanks Wolfgang!

I'll get to work on a revised multi-block read implementation.  A 4x
speedup in large MMC reads is worth the effort!

Steve