[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Lei Wen]

For emmc, it may has upto 7 partitions: two boot partitions, one
user partition, one RPMB partition and four general purpose partitions.
(Refer to JESD84-A44.pdf/page 154)

As bootloader may need to read out or reflashing images on those
different partitions, it is better to enable the partition switch with
console command support.

Signed-off-by: Lei Wen <leiwen@marvell.com>
---
 common/cmd_mmc.c  |   24 +++++++++++++++++++++++-
 drivers/mmc/mmc.c |   16 ++++++++++++++++
 include/mmc.h     |    5 +++++
 3 files changed, 44 insertions(+), 1 deletions(-)

diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
index 4323f76..2181e04 100644
--- a/common/cmd_mmc.c
+++ b/common/cmd_mmc.c
@@ -177,7 +177,6 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	case 0:
 	case 1:
-	case 4:
 		return cmd_usage(cmdtp);
 
 	case 2:
@@ -233,6 +232,28 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			printf("%d blocks written: %s\n",
 				n, (n == cnt) ? "OK" : "ERROR");
 			return (n == cnt) ? 0 : 1;
+		} else if (strcmp(argv[1], "sw_part") == 0) {
+			int dev = simple_strtoul(argv[2], NULL, 10);
+			struct mmc *mmc = find_mmc_device(dev);
+			int num = simple_strtoul(argv[3], NULL, 10);
+			int ret;
+
+			if (num > PART_ACCESS_MASK) {
+				printf("#part_num shouldn't be larger than %d\n",
+					PART_ACCESS_MASK);
+				return 1;
+			}
+
+			mmc_init(mmc);
+			if (mmc->part_config == MMCPART_NOAVAILABLE) {
+				printf("Card doesn't support part_switch\n");
+				return 1;
+			}
+
+			ret = mmc_switch_part(dev, num);
+			printf("switch to partions #%d, %s\n",
+					num, (!ret) ? "OK" : "ERROR");
+			return (!ret) ? 0 : 1;
 		} else
 			rc = cmd_usage(cmdtp);
 
@@ -247,5 +268,6 @@ U_BOOT_CMD(
 	"mmc write <device num> addr blk# cnt\n"
 	"mmc rescan <device num>\n"
 	"mmc part <device num> - lists available partition on mmc\n"
+	"mmc sw_part <device num> <part_num> - switch part support for emmc\n"
 	"mmc list - lists available devices");
 #endif
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 6805b33..f42b3fb 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -428,6 +428,18 @@ int mmc_change_freq(struct mmc *mmc)
 	return 0;
 }
 
+int mmc_switch_part(int dev_num, unsigned int part_num)
+{
+	struct mmc *mmc = find_mmc_device(dev_num);
+
+	if (!mmc)
+		return -1;
+
+	return mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF,
+			  (mmc->part_config & ~PART_ACCESS_MASK)
+			  | (part_num & PART_ACCESS_MASK));
+}
+
 int sd_switch(struct mmc *mmc, int mode, int group, u8 value, u8 *resp)
 {
 	struct mmc_cmd cmd;
@@ -725,6 +737,7 @@ int mmc_startup(struct mmc *mmc)
 	if (err)
 		return err;
 
+	mmc->part_config = MMCPART_NOAVAILABLE;
 	if (!IS_SD(mmc) && (mmc->version >= MMC_VERSION_4)) {
 		/* check  ext_csd version and capacity */
 		err = mmc_send_ext_csd(mmc, ext_csd);
@@ -733,6 +746,9 @@ int mmc_startup(struct mmc *mmc)
 					ext_csd[214] << 16 | ext_csd[215] << 24;
 			mmc->capacity *= 512;
 		}
+		/* store the partition info of emmc */
+		if (ext_csd[160] & 0x1)
+			mmc->part_config = ext_csd[179];
 	}
 
 	if (IS_SD(mmc))
diff --git a/include/mmc.h b/include/mmc.h
index 74c0b1d..10dfafd 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -128,6 +128,7 @@
  * EXT_CSD fields
  */
 
+#define EXT_CSD_PART_CONF	179	/* R/W */
 #define EXT_CSD_BUS_WIDTH	183	/* R/W */
 #define EXT_CSD_HS_TIMING	185	/* R/W */
 #define EXT_CSD_CARD_TYPE	196	/* RO */
@@ -169,6 +170,8 @@
 #define MMC_RSP_R6      (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
 #define MMC_RSP_R7      (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
 
+#define MMCPART_NOAVAILABLE	(0xff)
+#define PART_ACCESS_MASK	(0x7)
 
 struct mmc_cid {
 	unsigned long psn;
@@ -265,6 +268,7 @@ struct mmc {
 	uint csd[4];
 	uint cid[4];
 	ushort rca;
+	char part_config;
 	uint tran_speed;
 	uint read_bl_len;
 	uint write_bl_len;
@@ -285,6 +289,7 @@ struct mmc *find_mmc_device(int dev_num);
 int mmc_set_dev(int dev_num);
 void print_mmc_devices(char separator);
 int board_mmc_getcd(u8 *cd, struct mmc *mmc);
+int mmc_switch_part(int dev_num, unsigned int part_num);
 
 #ifdef CONFIG_GENERIC_MMC
 int atmel_mci_init(void *regs);
-- 
1.7.0.4

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Wolfgang Denk]

Dear Lei Wen,

In message <AANLkTimacphrMbu1RTGjWJKYU9FaC8+Xo2sgnjfW4LZQ@mail.gmail.com> you wrote:
>
> Does this patch could get the chance to be included in the 2011.03 release?

This is not a bug fix, and it was submitted after the merge window
was closed.

> I just see it haven't gotten any response after eight days...

Will reply ASAP.

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
Why is an average signature file longer than an average Perl script??

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Wolfgang Denk]

Dear Lei Wen,

In message <1296463665-26551-1-git-send-email-leiwen@marvell.com> you wrote:
> For emmc, it may has upto 7 partitions: two boot partitions, one
> user partition, one RPMB partition and four general purpose partitions.
> (Refer to JESD84-A44.pdf/page 154)
> 
> As bootloader may need to read out or reflashing images on those
> different partitions, it is better to enable the partition switch with
> console command support.

Other devices - say, USB mass storage devices, IDE disks, etc. - have
partitions, too, and do not need any such special command.

> @@ -247,5 +268,6 @@ U_BOOT_CMD(
>  	"mmc write <device num> addr blk# cnt\n"
>  	"mmc rescan <device num>\n"
>  	"mmc part <device num> - lists available partition on mmc\n"
> +	"mmc sw_part <device num> <part_num> - switch part support for emmc\n"
>  	"mmc list - lists available devices");

Why would we need a separate command here?


Please rework this so it uses the same mechanism we use on all other
storage devices, without an extra command here.

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 following statement is not true.  The previous statement is true.

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Lei Wen]

Hi Wolfgang,

On Tue, Feb 8, 2011 at 10:27 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <1296463665-26551-1-git-send-email-leiwen@marvell.com> you wrote:
>> For emmc, it may has upto 7 partitions: two boot partitions, one
>> user partition, one RPMB partition and four general purpose partitions.
>> (Refer to JESD84-A44.pdf/page 154)
>>
>> As bootloader may need to read out or reflashing images on those
>> different partitions, it is better to enable the partition switch with
>> console command support.
>
> Other devices - say, USB mass storage devices, IDE disks, etc. - have
> partitions, too, and do not need any such special command.

EMMC is sightly different witht the other device with partition.
For IDE as example, we could copy file between part A and B without problem.
It is for the different partition start at different offset in the
physical media.
Just adding the offset with the read address would make this works, so it is
some kind of software implementation.

But for EMMC, the boot partition is different with the normal partition.
We need to send a special command to do the switch, not with the
different offset.
If we need to copy file between those two partition, we need to send
switch command
before copy operation.
It is a hardware one. IDE or usb masstorage has no such ability...

>
>> @@ -247,5 +268,6 @@ U_BOOT_CMD(
>> ? ? ? "mmc write <device num> addr blk# cnt\n"
>> ? ? ? "mmc rescan <device num>\n"
>> ? ? ? "mmc part <device num> - lists available partition on mmc\n"
>> + ? ? "mmc sw_part <device num> <part_num> - switch part support for emmc\n"
>> ? ? ? "mmc list - lists available devices");
>
> Why would we need a separate command here?
>
>
> Please rework this so it uses the same mechanism we use on all other
> storage devices, without an extra command here.
>

The problem here is that the need to switch this hardware partition
only make sense
to the emmc. If make it generic to the other mmc command, it seems a
bit of weird for the
sd/mmc...

Also add additional command here is the minimual change. If insert
this into already existed
mmc command would make the fat command hard to work.
For mmc may change the hardware partition, but fat command cannot...
It is because
it need to be compatiable with other devices that have no hardware partition...

Best regards,
Lei

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Wolfgang Denk]

Dear Lei Wen,

In message <AANLkTikdUN37v6tvYpy0hyJrB9OJ4XK-LyNPSida7Dyf@mail.gmail.com> you wrote:
> 
> EMMC is sightly different witht the other device with partition. 

Maybe.  But does this really require a different iunterface in the
U-Boot context?   I doubt that.

> For IDE as example, we could copy file between part A and B without problem.

No, we cannot. U-Boot can only read _or_ write form mass storage
operations, so a copy operation would always require a "read from
device to RAM buffer", "write to device from RAM buffer" sequence.
The same can obviously be done with EMMC as well.

> But for EMMC, the boot partition is different with the normal partition.
> We need to send a special command to do the switch, not with the
> different offset.

Thisis an internal implementation detail.  As a user, I do not want to
know about it. I just want to read or write data to some partition.
The rest is driver internals.

> If we need to copy file between those two partition, we need to send
> switch command
> before copy operation.

So where is the problem?  See above. We always have TWO separate
operations, and each of them will select a device/partition.

> It is a hardware one. IDE or usb masstorage has no such ability...

I see no difference from the user point of view.  The internal
implementation may be different, but that doesn't matter.

> For mmc may change the hardware partition, but fat command cannot...
> It is because
> it need to be compatiable with other devices that have no hardware partition...

I don;t understand what you want to tell me here.  My complaint is
that your new code is _not_ compatible with other divices, and I see
no reason for such an incompatibility.

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
Computers are not intelligent. They only think they are.

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Lei Wen]

Hi Wolfgang,

On Tue, Feb 8, 2011 at 11:13 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <AANLkTikdUN37v6tvYpy0hyJrB9OJ4XK-LyNPSida7Dyf@mail.gmail.com> you wrote:
>>
>> EMMC is sightly different witht the other device with partition.
>
> Maybe. ?But does this really require a different iunterface in the
> U-Boot context? ? I doubt that.
>
>> For IDE as example, we could copy file between part A and B without problem.
>
> No, we cannot. U-Boot can only read _or_ write form mass storage
> operations, so a copy operation would always require a "read from
> device to RAM buffer", "write to device from RAM buffer" sequence.
> The same can obviously be done with EMMC as well.
>
>> But for EMMC, the boot partition is different with the normal partition.
>> We need to send a special command to do the switch, not with the
>> different offset.
>
> Thisis an internal implementation detail. ?As a user, I do not want to
> know about it. I just want to read or write data to some partition.
> The rest is driver internals.
>
>> If we need to copy file between those two partition, we need to send
>> switch command
>> before copy operation.
>
> So where is the problem? ?See above. We always have TWO separate
> operations, and each of them will select a device/partition.
>
>> It is a hardware one. IDE or usb masstorage has no such ability...
>
> I see no difference from the user point of view. ?The internal
> implementation may be different, but that doesn't matter.
>
>> For mmc may change the hardware partition, but fat command cannot...
>> It is because
>> it need to be compatiable with other devices that have no hardware partition...
>
> I don;t understand what you want to tell me here. ?My complaint is
> that your new code is _not_ compatible with other divices, and I see
> no reason for such an incompatibility.
>

I am thinking now approach for this.
How about adding addtitional member in the mmc structure, like part.
During mmc probe, the driver could know whether the device support partition
switch or not, if it support, we could register three other "faked" mmc device
corresponding to two boot partition, and RPMB partition.

Then we could use the current command to access the different hardware
partition in the emmc. Like if we are accessing one hardware partition, we need
to send switch command first. If the part member in the mmc is a invalid value,
we then don't send that switch partition command.

Could this approach be acceptable?

Best regards,
Lei

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Lei Wen]

>
> I am thinking now approach for this.
> How about adding addtitional member in the mmc structure, like part.
> During mmc probe, the driver could know whether the device support partition
> switch or not, if it support, we could register three other "faked" mmc device
> corresponding to two boot partition, and RPMB partition.
>
> Then we could use the current command to access the different hardware
> partition in the emmc. Like if we are accessing one hardware partition, we need
> to send switch command first. If the part member in the mmc is a invalid value,
> we then don't send that switch partition command.
>
> Could this approach be acceptable?
>

Further refine this idea, don't need to call mmc_register multi-times.
For the purpose is to switch partition when needed, we really do want to one
real device. So I propose a new solution as:

Assume there is two sd/mmc controller on the board, named as mmc0&mmc1.
The mmc0 attach with a sd card, and mmc1 attach with a emmc that has partition
supported. Then we would have mmc(2, 3, 4, 5, 6, 7, 8) faked device map to mmc0
, mmc(9, 10, 11, 12, 13, 14, 15) map to mmc1.

Give the command as "mmc read 9 0x1000000 0 0x10", uboot would get the
real device
num by 9/7, while hardware part num is (9-1-7*real_dev_num) that is 1.
So we need
to make mmc1 send the switch part 1 command to the emmc, then perform the read
operation.

If we want to run "mmc read 5 0x10000 0x10 0x5", the sd attach on mmc0 has no
hardware partition suppport, it would simply refuse to execute that command.
Or if we want to be more gracefully handled, we could let the read 5
perform like the
read 0 command.

Best regards,
Lei

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Lei Wen]

Hi,

On Fri, Feb 11, 2011 at 11:05 PM, Lei Wen <adrian.wenl@gmail.com> wrote:
>>
>> I am thinking now approach for this.
>> How about adding addtitional member in the mmc structure, like part.
>> During mmc probe, the driver could know whether the device support partition
>> switch or not, if it support, we could register three other "faked" mmc device
>> corresponding to two boot partition, and RPMB partition.
>>
>> Then we could use the current command to access the different hardware
>> partition in the emmc. Like if we are accessing one hardware partition, we need
>> to send switch command first. If the part member in the mmc is a invalid value,
>> we then don't send that switch partition command.
>>
>> Could this approach be acceptable?
>>
>
> Further refine this idea, don't need to call mmc_register multi-times.
> For the purpose is to switch partition when needed, we really do want to one
> real device. So I propose a new solution as:
>
> Assume there is two sd/mmc controller on the board, named as mmc0&mmc1.
> The mmc0 attach with a sd card, and mmc1 attach with a emmc that has partition
> supported. Then we would have mmc(2, 3, 4, 5, 6, 7, 8) faked device map to mmc0
> , mmc(9, 10, 11, 12, 13, 14, 15) map to mmc1.
>
> Give the command as "mmc read 9 0x1000000 0 0x10", uboot would get the
> real device
> num by 9/7, while hardware part num is (9-1-7*real_dev_num) that is 1.
> So we need
> to make mmc1 send the switch part 1 command to the emmc, then perform the read
> operation.
>
> If we want to run "mmc read 5 0x10000 0x10 0x5", the sd attach on mmc0 has no
> hardware partition suppport, it would simply refuse to execute that command.
> Or if we want to be more gracefully handled, we could let the read 5
> perform like the
> read 0 command.
>

Any feedback for this?...

Thanks,
Lei

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Wolfgang Denk]

Dear Lei Wen,

In message <AANLkTikH_5rXgFsWbNRnBQtB6WFjT=2K8pd8CBXaKcCQ@mail.gmail.com> you wrote:
>
> Further refine this idea, don't need to call mmc_register multi-times.
> For the purpose is to switch partition when needed, we really do want to one
> real device. So I propose a new solution as:
> 
> Assume there is two sd/mmc controller on the board, named as mmc0&mmc1.
> The mmc0 attach with a sd card, and mmc1 attach with a emmc that has partition
> supported. Then we would have mmc(2, 3, 4, 5, 6, 7, 8) faked device map to mmc0
> , mmc(9, 10, 11, 12, 13, 14, 15) map to mmc1.
> 
> Give the command as "mmc read 9 0x1000000 0 0x10", uboot would get the
> real device
> num by 9/7, while hardware part num is (9-1-7*real_dev_num) that is 1.
> So we need
> to make mmc1 send the switch part 1 command to the emmc, then perform the read
> operation.
> 
> If we want to run "mmc read 5 0x10000 0x10 0x5", the sd attach on mmc0 has no
> hardware partition suppport, it would simply refuse to execute that command.
> Or if we want to be more gracefully handled, we could let the read 5
> perform like the
> read 0 command.

This is ugly and error prone.

Why don't you follow the existing examples from other storage devices,
where we have a "dev" subcommand that allows to select one of the
existing devices as "active" device?  Similarly, you could here select
an active device/partition pair.

This makes the access commands easier, and the user has much less
problems to figure out which number he has to use to specify the -
wrong - partition.

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 journaling file system can recover your data if the disk dies.
                                 - Steve Rago in <D4Cw1p.L9E@plc.com>

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Lei Wen]

Hi Wolfgang,

On Wed, Apr 13, 2011 at 3:31 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <AANLkTikH_5rXgFsWbNRnBQtB6WFjT=2K8pd8CBXaKcCQ@mail.gmail.com> you wrote:
>>
>> Further refine this idea, don't need to call mmc_register multi-times.
>> For the purpose is to switch partition when needed, we really do want to one
>> real device. So I propose a new solution as:
>>
>> Assume there is two sd/mmc controller on the board, named as mmc0&mmc1.
>> The mmc0 attach with a sd card, and mmc1 attach with a emmc that has partition
>> supported. Then we would have mmc(2, 3, 4, 5, 6, 7, 8) faked device map to mmc0
>> , mmc(9, 10, 11, 12, 13, 14, 15) map to mmc1.
>>
>> Give the command as "mmc read 9 0x1000000 0 0x10", uboot would get the
>> real device
>> num by 9/7, while hardware part num is (9-1-7*real_dev_num) that is 1.
>> So we need
>> to make mmc1 send the switch part 1 command to the emmc, then perform the read
>> operation.
>>
>> If we want to run "mmc read 5 0x10000 0x10 0x5", the sd attach on mmc0 has no
>> hardware partition suppport, it would simply refuse to execute that command.
>> Or if we want to be more gracefully handled, we could let the read 5
>> perform like the
>> read 0 command.
>
> This is ugly and error prone.
>
> Why don't you follow the existing examples from other storage devices,
> where we have a "dev" subcommand that allows to select one of the
> existing devices as "active" device? ?Similarly, you could here select
> an active device/partition pair.

dev subcommand to act as "sw_part" in my first patch?

>
> This makes the access commands easier, and the user has much less
> problems to figure out which number he has to use to specify the -
> wrong - partition.
>

How about this solution?
Like if want to access the dev 0 and hardware partition 1, we could use the
command as "mmc read 0:1 ***".
If not pass the :[part] parameter to the dev number, like "mmc read 0 ***",
that would means the partition is 0 by default. For this case, if that device
has no capabilty of multi hardware partition, it would act like normal, or
switch to the default 0 before any further action.

Best regads,
Lei

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Wolfgang Denk]

Dear Lei Wen,

In message <BANLkTikWwbbuNyHppQQygZ3JDh4aXUg3NA@mail.gmail.com> you wrote:
> 
> > Why don't you follow the existing examples from other storage devices,
> > where we have a "dev" subcommand that allows to select one of the
> > existing devices as "active" device? =A0Similarly, you could here select
> > an active device/partition pair.
> 
> dev subcommand to act as "sw_part" in my first patch?

I'm not really sure I understand what this command was supposed to do.
What I want to see is consistency with other drivers, i. e. you should
be able to use "mmc dev" to select a device and optionally a partition
on it as current device, so the remaining "mmc" commands do not need
any device arguments any more.

> How about this solution?
> Like if want to access the dev 0 and hardware partition 1, we could use the
> command as "mmc read 0:1 ***".
> If not pass the :[part] parameter to the dev number, like "mmc read 0 ***",
> that would means the partition is 0 by default. For this case, if that device
> has no capabilty of multi hardware partition, it would act like normal, or
> switch to the default 0 before any further action.

I want to get rid of the device argument in all these calls.

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
Der Horizont vieler Menschen ist ein Kreis mit Radius Null --
und das nennen sie ihren Standpunkt.

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Lei Wen]

Hi Wolfgang,

On Wed, Apr 13, 2011 at 1:35 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <BANLkTikWwbbuNyHppQQygZ3JDh4aXUg3NA@mail.gmail.com> you wrote:
>>
>> > Why don't you follow the existing examples from other storage devices,
>> > where we have a "dev" subcommand that allows to select one of the
>> > existing devices as "active" device? =A0Similarly, you could here select
>> > an active device/partition pair.
>>
>> dev subcommand to act as "sw_part" in my first patch?
>
> I'm not really sure I understand what this command was supposed to do.
> What I want to see is consistency with other drivers, i. e. you should
> be able to use "mmc dev" to select a device and optionally a partition
> on it as current device, so the remaining "mmc" commands do not need
> any device arguments any more.

Like this mmc dev[:part], could it be acceptable?

>
>> How about this solution?
>> Like if want to access the dev 0 and hardware partition 1, we could use the
>> command as "mmc read 0:1 ***".
>> If not pass the :[part] parameter to the dev number, like "mmc read 0 ***",
>> that would means the partition is 0 by default. For this case, if that device
>> has no capabilty of multi hardware partition, it would act like normal, or
>> switch to the default 0 before any further action.
>
> I want to get rid of the device argument in all these calls.
>

So the the mmc read would becomes "mmc read [ram address] [mmc sector
start] [sector num]"?

Best regards,
Lei

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Wolfgang Denk]

Dear Lei Wen,

In message <BANLkTinFVYDiAt9D+EPadat2f_0qTQaS8w@mail.gmail.com> you wrote:
> 
> > I'm not really sure I understand what this command was supposed to do.
> > What I want to see is consistency with other drivers, i. e. you should
> > be able to use "mmc dev" to select a device and optionally a partition
> > on it as current device, so the remaining "mmc" commands do not need
> > any device arguments any more.
> 
> Like this mmc dev[:part], could it be acceptable?

The command should work similar to what we see with other storage
devices, i. e.

	mmc dev 3

will select device "3" as current device, so that 

	mmc dev

will print something like "mmc device 3: Model ... Type ... Capacity
..." etc., and that

	mmc read 80800000 0 100

would read the first 256 blocks of device 3 into RAM starting at
address 80800000.

Similar after

	mmc dev 2:3

but now for device 2, partition 3.

For example, here is how it works on IDE:

=> help ide
ide - IDE sub-system

Usage:
ide reset - reset IDE controller
ide info  - show available IDE devices
ide device [dev] - show or set current device
ide part [dev] - print partition table of one or all IDE devices
ide read  addr blk# cnt
ide write addr blk# cnt - read/write `cnt' blocks starting at block `blk#'
    to/from memory address `addr'
=> ide dev

IDE device 0: Model: ST920813AM Firm: 3.02 Ser#: 5QA015N2
            Type: Hard Disk
            Capacity: 19077.1 MB = 18.6 GB (39070080 x 512)
=> ide dev 1

IDE device 1: not available


> > I want to get rid of the device argument in all these calls.
> 
> So the the mmc read would becomes "mmc read [ram address] [mmc sector
> start] [sector num]"?

Right.  That's what we use on other storage devices, too.

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
You can only live once, but if you do it right, once is enough.

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Lei Wen]

Hi Wolfgang,

On Wed, Apr 13, 2011 at 1:49 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <BANLkTinFVYDiAt9D+EPadat2f_0qTQaS8w@mail.gmail.com> you wrote:
>>
>> > I'm not really sure I understand what this command was supposed to do.
>> > What I want to see is consistency with other drivers, i. e. you should
>> > be able to use "mmc dev" to select a device and optionally a partition
>> > on it as current device, so the remaining "mmc" commands do not need
>> > any device arguments any more.
>>
>> Like this mmc dev[:part], could it be acceptable?
>
> The command should work similar to what we see with other storage
> devices, i. e.
>
> ? ? ? ?mmc dev 3
>
> will select device "3" as current device, so that
>
> ? ? ? ?mmc dev
>
> will print something like "mmc device 3: Model ... Type ... Capacity
> ..." etc., and that
>
> ? ? ? ?mmc read 80800000 0 100
>
> would read the first 256 blocks of device 3 into RAM starting at
> address 80800000.
>
> Similar after
>
> ? ? ? ?mmc dev 2:3
>
> but now for device 2, partition 3.
>
> For example, here is how it works on IDE:
>
> => help ide
> ide - IDE sub-system
>
> Usage:
> ide reset - reset IDE controller
> ide info ?- show available IDE devices
> ide device [dev] - show or set current device
> ide part [dev] - print partition table of one or all IDE devices
> ide read ?addr blk# cnt
> ide write addr blk# cnt - read/write `cnt' blocks starting at block `blk#'
> ? ?to/from memory address `addr'
> => ide dev
>
> IDE device 0: Model: ST920813AM Firm: 3.02 Ser#: 5QA015N2
> ? ? ? ? ? ?Type: Hard Disk
> ? ? ? ? ? ?Capacity: 19077.1 MB = 18.6 GB (39070080 x 512)
> => ide dev 1
>
> IDE device 1: not available
>
>
>> > I want to get rid of the device argument in all these calls.
>>
>> So the the mmc read would becomes "mmc read [ram address] [mmc sector
>> start] [sector num]"?
>
> Right. ?That's what we use on other storage devices, too.
>

Understand, I would reformat the original patch according to this change.

Thanks,
Lei

[U-Boot] [U-BOOT] [PATCH] mmc: enable switch partition function

[Andy Fleming]

>
>> > I want to get rid of the device argument in all these calls.
>>
>> So the the mmc read would becomes "mmc read [ram address] [mmc sector
>> start] [sector num]"?
>
> Right. ?That's what we use on other storage devices, too.

Right now, getting rid of all of those is a bit of a chore.
Personally, I'd prefer if we did it in such a way that we still *can*
provide the argument on the command line. In my personal experience, I
far more often think verb, object, rather than the other way around.
The way I did the mdio command for phylib, one could do either one:

mii device foo
mdio read 0 0

or

mdio read foo 0 0

(and, as a side bonus, from then on, mdio read 0 0 will work, too).

Though I can't for the life of me remember why I left the device
identifier as a simple integer.

My 2 cents,
Andy

[U-Boot] [PATCH V2 0/2] mmc: add partition support

[Lei Wen]

V2:
use the "mmc_cur_dev" to replace explicitly specify the dev num in mmc command.
change the switch parittion command per Wolfgang's suggestion

Lei Wen (2):
  cmd_mmc: eliminate device num in the mmc command
  mmc: enable partition switch fucntion for emmc

 common/cmd_mmc.c  |  198 +++++++++++++++++++++++++++++++----------------------
 drivers/mmc/mmc.c |   46 +++++++++++--
 include/mmc.h     |   11 +++
 3 files changed, 169 insertions(+), 86 deletions(-)

[U-Boot] [PATCH V2 1/2] cmd_mmc: eliminate device num in the mmc command

[Lei Wen]

[U-Boot] [PATCH V2 2/2] mmc: enable partition switch fucntion for emmc

[Lei Wen]

For emmc, it may has upto 7 partitions: two boot partitions, one
user partition, one RPMB partition and four general purpose partitions.
(Refer to JESD84-A44.pdf/page 154)

As bootloader may need to read out or reflashing images on those
different partitions, it is better to enable the partition switch with
console command support.

Also for partition would be restore to user partition(part 0) when CMD0
is used, so change mmc_init routine to perform normal initialization
only once for each slot, unless use the rescan command to force init
again.

Signed-off-by: Lei Wen <leiwen@marvell.com>
---
Changelog:
V2:
change the switch parittion command per Wolfgang's suggestion

 common/cmd_mmc.c  |   35 ++++++++++++++++++++++++++++++++---
 drivers/mmc/mmc.c |   30 +++++++++++++++++++++++++++++-
 include/mmc.h     |    8 ++++++++
 3 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
index dd4ee23..333a1ff 100644
--- a/common/cmd_mmc.c
+++ b/common/cmd_mmc.c
@@ -147,6 +147,7 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (!mmc)
 			return 1;
 
+		mmc->has_init = 0;
 		mmc_init(mmc);
 
 		return 0;
@@ -172,7 +173,7 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		print_mmc_devices('\n');
 		return 0;
 	} else if (strcmp(argv[1], "dev") == 0) {
-		int dev = mmc_cur_dev;
+		int dev = mmc_cur_dev, part = -1;
 		struct mmc *mmc;
 
 		if (argc == 2) {
@@ -183,6 +184,14 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		} else if (argc == 3) {
 			dev = simple_strtoul(argv[2], NULL, 10);
 
+		} else if (argc == 4) {
+			dev = (int)simple_strtoul(argv[2], NULL, 10);
+			part = (int)simple_strtoul(argv[3], NULL, 10);
+			if (part > PART_ACCESS_MASK) {
+				printf("#part_num shouldn't be larger"
+					" than %d\n", PART_ACCESS_MASK);
+				return 1;
+			}
 		} else {
 			return cmd_usage(cmdtp);
 		}
@@ -193,8 +202,28 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			return 1;
 		}
 
+		mmc_init(mmc);
+		if (part != -1) {
+			int ret;
+			if (mmc->part_config == MMCPART_NOAVAILABLE) {
+				printf("Card doesn't support part_switch\n");
+				return 1;
+			}
+
+			if (part != mmc->part_num) {
+				ret = mmc_switch_part(dev, part);
+				if (!ret)
+					mmc->part_num = part;
+			}
+			printf("switch to partions #%d, %s\n",
+					part, (!ret) ? "OK" : "ERROR");
+		}
 		mmc_cur_dev = dev;
-		printf("mmc%d is current device\n", mmc_cur_dev);
+		if (mmc->part_config == MMCPART_NOAVAILABLE)
+			printf("mmc%d is current device\n", mmc_cur_dev);
+		else
+			printf("mmc%d(part %d) is current device\n",
+				mmc_cur_dev, mmc->part_num);
 
 		return 0;
 	} else if (strcmp(argv[1], "read") == 0) {
@@ -253,6 +282,6 @@ U_BOOT_CMD(
 	"mmc write addr blk# cnt\n"
 	"mmc rescan\n"
 	"mmc part - lists available partition on current mmc device\n"
-	"mmc dev [dev] - show or set current mmc device\n"
+	"mmc dev [dev] [part] - show or set current mmc device [partition]\n"
 	"mmc list - lists available devices");
 #endif
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 377f13a..ae520a1 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -428,6 +428,18 @@ int mmc_change_freq(struct mmc *mmc)
 	return 0;
 }
 
+int mmc_switch_part(int dev_num, unsigned int part_num)
+{
+	struct mmc *mmc = find_mmc_device(dev_num);
+
+	if (!mmc)
+		return -1;
+
+	return mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF,
+			  (mmc->part_config & ~PART_ACCESS_MASK)
+			  | (part_num & PART_ACCESS_MASK));
+}
+
 int sd_switch(struct mmc *mmc, int mode, int group, u8 value, u8 *resp)
 {
 	struct mmc_cmd cmd;
@@ -725,6 +737,7 @@ int mmc_startup(struct mmc *mmc)
 	if (err)
 		return err;
 
+	mmc->part_config = MMCPART_NOAVAILABLE;
 	if (!IS_SD(mmc) && (mmc->version >= MMC_VERSION_4)) {
 		/* check  ext_csd version and capacity */
 		err = mmc_send_ext_csd(mmc, ext_csd);
@@ -733,6 +746,10 @@ int mmc_startup(struct mmc *mmc)
 					ext_csd[214] << 16 | ext_csd[215] << 24;
 			mmc->capacity *= 512;
 		}
+
+		/* store the partition info of emmc */
+		if (ext_csd[160] & PART_SUPPORT)
+			mmc->part_config = ext_csd[179];
 	}
 
 	if (IS_SD(mmc))
@@ -872,6 +889,9 @@ int mmc_init(struct mmc *mmc)
 {
 	int err;
 
+	if (mmc->has_init)
+		return 0;
+
 	err = mmc->init(mmc);
 
 	if (err)
@@ -886,6 +906,9 @@ int mmc_init(struct mmc *mmc)
 	if (err)
 		return err;
 
+	/* The internal partition reset to user partition(0) at every CMD0*/
+	mmc->part_num = 0;
+
 	/* Test for SD version 2 */
 	err = mmc_send_if_cond(mmc);
 
@@ -902,7 +925,12 @@ int mmc_init(struct mmc *mmc)
 		}
 	}
 
-	return mmc_startup(mmc);
+	err = mmc_startup(mmc);
+	if (err)
+		mmc->has_init = 0;
+	else
+		mmc->has_init = 1;
+	return err;
 }
 
 /*
diff --git a/include/mmc.h b/include/mmc.h
index dd9bf15..44435ac 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -128,6 +128,7 @@
  * EXT_CSD fields
  */
 
+#define EXT_CSD_PART_CONF	179	/* R/W */
 #define EXT_CSD_BUS_WIDTH	183	/* R/W */
 #define EXT_CSD_HS_TIMING	185	/* R/W */
 #define EXT_CSD_CARD_TYPE	196	/* RO */
@@ -169,6 +170,9 @@
 #define MMC_RSP_R6      (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
 #define MMC_RSP_R7      (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
 
+#define MMCPART_NOAVAILABLE	(0xff)
+#define PART_ACCESS_MASK	(0x7)
+#define PART_SUPPORT		(0x1)
 
 struct mmc_cid {
 	unsigned long psn;
@@ -253,6 +257,7 @@ struct mmc {
 	void *priv;
 	uint voltages;
 	uint version;
+	uint has_init;
 	uint f_min;
 	uint f_max;
 	int high_capacity;
@@ -265,6 +270,8 @@ struct mmc {
 	uint csd[4];
 	uint cid[4];
 	ushort rca;
+	char part_config;
+	char part_num;
 	uint tran_speed;
 	uint read_bl_len;
 	uint write_bl_len;
@@ -288,6 +295,7 @@ struct mmc *find_mmc_device(int dev_num);
 int mmc_set_dev(int dev_num);
 void print_mmc_devices(char separator);
 int board_mmc_getcd(u8 *cd, struct mmc *mmc);
+int mmc_switch_part(int dev_num, unsigned int part_num);
 
 #ifdef CONFIG_GENERIC_MMC
 int atmel_mci_init(void *regs);
-- 
1.7.0.4

[U-Boot] [PATCH V2 1/2] cmd_mmc: eliminate device num in the mmc command

[Andy Fleming]

On Fri, Apr 15, 2011 at 9:17 PM, Lei Wen <leiwen@marvell.com> wrote:
> From now on, follow the general rule "mmc dev [dev]" to change the
> mmc command applied device, like ide and usb...
>
> Signed-off-by: Lei Wen <leiwen@marvell.com>
> ---
> Changelog:
> V2: use the "mmc_cur_dev" to replace explicitly specify the dev num
> ? ?in mmc command.


I'd really prefer if there were still the option to specify the device
in the read/write commands. I appreciate the convenience of not having
to specify it every time, but I feel that by doing things this way, we
create an artificial separation between a transaction, and the target
of the transaction. It tends to encourage a notion that a transaction
can only be done to the global-current device, which is fine for
command-line interfaces, but can result in broken programming
interfaces



> ?int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> ?{
> - ? ? ? int rc = 0;
> + ? ? ? if (argc < 2)
> + ? ? ? ? ? ? ? return cmd_usage(cmdtp);
>
> - ? ? ? switch (argc) {
> - ? ? ? case 3:
> - ? ? ? ? ? ? ? if (strcmp(argv[1], "rescan") == 0) {
> - ? ? ? ? ? ? ? ? ? ? ? int dev = simple_strtoul(argv[2], NULL, 10);
> - ? ? ? ? ? ? ? ? ? ? ? struct mmc *mmc = find_mmc_device(dev);
> + ? ? ? if (strcmp(argv[1], "rescan") == 0) {
> + ? ? ? ? ? ? ? struct mmc *mmc = find_mmc_device(mmc_cur_dev);
>
> - ? ? ? ? ? ? ? ? ? ? ? if (!mmc)
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? return 1;
> + ? ? ? ? ? ? ? if (!mmc)
> + ? ? ? ? ? ? ? ? ? ? ? return 1;
> +
> + ? ? ? ? ? ? ? mmc_init(mmc);
>
> - ? ? ? ? ? ? ? ? ? ? ? mmc_init(mmc);
> + ? ? ? ? ? ? ? return 0;
> + ? ? ? } else if (strncmp(argv[1], "part", 4) == 0) {
> + ? ? ? ? ? ? ? block_dev_desc_t *mmc_dev;
> + ? ? ? ? ? ? ? struct mmc *mmc = find_mmc_device(mmc_cur_dev);
>
> + ? ? ? ? ? ? ? if (!mmc) {
> + ? ? ? ? ? ? ? ? ? ? ? puts("no mmc devices available\n");

Technically, this just means that no device is selected. I'm sure that
would only happen if no devices were available, but that's an
assumption carried outside of the vicinity of this code. Someone might
change that assumption (for instance, if something could cause an mmc
device to be removed)

> + ? ? ? ? ? ? ? ? ? ? ? return 1;
> + ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? mmc_init(mmc);
> + ? ? ? ? ? ? ? mmc_dev = mmc_get_dev(mmc_cur_dev);
> + ? ? ? ? ? ? ? if (mmc_dev != NULL &&
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? mmc_dev->type != DEV_TYPE_UNKNOWN) {
> + ? ? ? ? ? ? ? ? ? ? ? print_part(mmc_dev);

[...]

> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 6805b33..377f13a 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -34,7 +34,7 @@
> ?#include <div64.h>
>
> ?static struct list_head mmc_devices;
> -static int cur_dev_num = -1;


Please don't remove this, or conflate this value with the global
"current" pointer. This is used to assign the block dev num.
mmc_cur_dev is a runtime value that will change.  If, for some reason,
someone needs to interact with an mmc device before all of them have
come up, we will utterly break.  This is the sort of reason that I'm
not big on this sort of interaction paradigm.


> +int mmc_cur_dev = -1;


You should make this a pointer to the mmc device, not an integer.
Also, you should keep it in the cmd_mmc file.  The *only* place this
should be used is to declare what the current device for user
interaction is. Board code that wants to interact with MMC devices
should feel free to do so, irrespective of the user (ie, the user's
desired card should not affect which card the board code accesses, and
the board code should not cause the user's selection to change).


>
> ?int __board_mmc_getcd(u8 *cd, struct mmc *mmc) {
> ? ? ? ?return -1;
> @@ -849,7 +849,7 @@ int mmc_register(struct mmc *mmc)
> ?{
> ? ? ? ?/* Setup the universal parts of the block interface just once */
> ? ? ? ?mmc->block_dev.if_type = IF_TYPE_MMC;
> - ? ? ? mmc->block_dev.dev = cur_dev_num++;
> + ? ? ? mmc->block_dev.dev = mmc_cur_dev++;

Make sure to switch this back

> ? ? ? ?mmc->block_dev.removable = 1;
> ? ? ? ?mmc->block_dev.block_read = mmc_bread;
> ? ? ? ?mmc->block_dev.block_write = mmc_bwrite;
> @@ -936,12 +936,20 @@ void print_mmc_devices(char separator)
>
> ?int mmc_initialize(bd_t *bis)
> ?{
> + ? ? ? int ret = -1;
> ? ? ? ?INIT_LIST_HEAD (&mmc_devices);
> - ? ? ? cur_dev_num = 0;
> + ? ? ? mmc_cur_dev = 0;
>
> ? ? ? ?if (board_mmc_init(bis) < 0)
> - ? ? ? ? ? ? ? cpu_mmc_init(bis);
> + ? ? ? ? ? ? ? ret = cpu_mmc_init(bis);
> + ? ? ? else
> + ? ? ? ? ? ? ? ret = 0;
>
> + ? ? ? /* Put the device 0 as the default */
> + ? ? ? if (ret < 0)
> + ? ? ? ? ? ? ? mmc_cur_dev = -1;
> + ? ? ? else
> + ? ? ? ? ? ? ? mmc_cur_dev = 0;

These seems needlessly awkward. I suggest that we can initialize it
either to zero, or to the last register device, inside mmc_register().
You don't need "ret" at all, then. Because this should be in cmd_mmc,
we would add a function to cmd_mmc, like: mmc_set_current_dev(struct
mmc *)


> ? ? ? ?print_mmc_devices(',');
>
> ? ? ? ?return 0;
> diff --git a/include/mmc.h b/include/mmc.h
> index fcd0fd1..dd9bf15 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -295,4 +295,7 @@ int atmel_mci_init(void *regs);
> ?int mmc_legacy_init(int verbose);
> ?#endif
>
> +#ifdef CONFIG_GENERIC_MMC
> +extern int mmc_cur_dev;
> +#endif

And then this isn't necessary, but you can declare mmc_set_current_dev(), here

Andy

[U-Boot] [PATCH V2 1/2] cmd_mmc: eliminate device num in the mmc command

[Wolfgang Denk]

Dear Andy Fleming,

In message <BANLkTimmtV9L9P75A-PLez5rhVy4hYg=gA@mail.gmail.com> you wrote:
>
> > From now on, follow the general rule "mmc dev [dev]" to change the
> > mmc command applied device, like ide and usb...
...
> I'd really prefer if there were still the option to specify the device
> in the read/write commands. I appreciate the convenience of not having
> to specify it every time, but I feel that by doing things this way, we
> create an artificial separation between a transaction, and the target
> of the transaction. It tends to encourage a notion that a transaction
> can only be done to the global-current device, which is fine for
> command-line interfaces, but can result in broken programming
> interfaces

PLease see the preceeding thread for the reasons for this interface
(make it the same as what we have with USB, IDE, etc.).

Yes, it is not a nice one.  We really want a beter device model.

[I agree with the rest of your comments, though.]

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
Yes, it's a technical challenge, and  you  have  to  kind  of  admire
people  who go to the lengths of actually implementing it, but at the
same time you wonder about their IQ...
         --  Linus Torvalds in <5phda5$ml6$1@palladium.transmeta.com>

[U-Boot] [PATCH V2 1/2] cmd_mmc: eliminate device num in the mmc command

[Lei Wen]

Hi Andy,

On Thu, Apr 28, 2011 at 1:00 AM, Andy Fleming <afleming@gmail.com> wrote:
> On Fri, Apr 15, 2011 at 9:17 PM, Lei Wen <leiwen@marvell.com> wrote:
>> From now on, follow the general rule "mmc dev [dev]" to change the
>> mmc command applied device, like ide and usb...
>>
>> Signed-off-by: Lei Wen <leiwen@marvell.com>
>> ---
>> Changelog:
>> V2: use the "mmc_cur_dev" to replace explicitly specify the dev num
>> ? ?in mmc command.
>
>
> I'd really prefer if there were still the option to specify the device
> in the read/write commands. I appreciate the convenience of not having
> to specify it every time, but I feel that by doing things this way, we
> create an artificial separation between a transaction, and the target
> of the transaction. It tends to encourage a notion that a transaction
> can only be done to the global-current device, which is fine for
> command-line interfaces, but can result in broken programming
> interfaces
>
>
>
>> ?int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> ?{
>> - ? ? ? int rc = 0;
>> + ? ? ? if (argc < 2)
>> + ? ? ? ? ? ? ? return cmd_usage(cmdtp);
>>
>> - ? ? ? switch (argc) {
>> - ? ? ? case 3:
>> - ? ? ? ? ? ? ? if (strcmp(argv[1], "rescan") == 0) {
>> - ? ? ? ? ? ? ? ? ? ? ? int dev = simple_strtoul(argv[2], NULL, 10);
>> - ? ? ? ? ? ? ? ? ? ? ? struct mmc *mmc = find_mmc_device(dev);
>> + ? ? ? if (strcmp(argv[1], "rescan") == 0) {
>> + ? ? ? ? ? ? ? struct mmc *mmc = find_mmc_device(mmc_cur_dev);
>>
>> - ? ? ? ? ? ? ? ? ? ? ? if (!mmc)
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? return 1;
>> + ? ? ? ? ? ? ? if (!mmc)
>> + ? ? ? ? ? ? ? ? ? ? ? return 1;
>> +
>> + ? ? ? ? ? ? ? mmc_init(mmc);
>>
>> - ? ? ? ? ? ? ? ? ? ? ? mmc_init(mmc);
>> + ? ? ? ? ? ? ? return 0;
>> + ? ? ? } else if (strncmp(argv[1], "part", 4) == 0) {
>> + ? ? ? ? ? ? ? block_dev_desc_t *mmc_dev;
>> + ? ? ? ? ? ? ? struct mmc *mmc = find_mmc_device(mmc_cur_dev);
>>
>> + ? ? ? ? ? ? ? if (!mmc) {
>> + ? ? ? ? ? ? ? ? ? ? ? puts("no mmc devices available\n");
>
> Technically, this just means that no device is selected. I'm sure that
> would only happen if no devices were available, but that's an
> assumption carried outside of the vicinity of this code. Someone might
> change that assumption (for instance, if something could cause an mmc
> device to be removed)

If that device be removed, I think the following mmc_init could check it out,
wouldn't it? For this check, my understanding is to see whether that slot has
been register or not, then following init would double check device whether
function.

>
>> + ? ? ? ? ? ? ? ? ? ? ? return 1;
>> + ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? ? mmc_init(mmc);
>> + ? ? ? ? ? ? ? mmc_dev = mmc_get_dev(mmc_cur_dev);
>> + ? ? ? ? ? ? ? if (mmc_dev != NULL &&
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? mmc_dev->type != DEV_TYPE_UNKNOWN) {
>> + ? ? ? ? ? ? ? ? ? ? ? print_part(mmc_dev);
>
> [...]
>
>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>> index 6805b33..377f13a 100644
>> --- a/drivers/mmc/mmc.c
>> +++ b/drivers/mmc/mmc.c
>> @@ -34,7 +34,7 @@
>> ?#include <div64.h>
>>
>> ?static struct list_head mmc_devices;
>> -static int cur_dev_num = -1;
>
>
> Please don't remove this, or conflate this value with the global
> "current" pointer. This is used to assign the block dev num.
> mmc_cur_dev is a runtime value that will change. ?If, for some reason,
> someone needs to interact with an mmc device before all of them have
> come up, we will utterly break. ?This is the sort of reason that I'm
> not big on this sort of interaction paradigm.
>
>
>> +int mmc_cur_dev = -1;
>
>
> You should make this a pointer to the mmc device, not an integer.
> Also, you should keep it in the cmd_mmc file. ?The *only* place this
> should be used is to declare what the current device for user
> interaction is. Board code that wants to interact with MMC devices
> should feel free to do so, irrespective of the user (ie, the user's
> desired card should not affect which card the board code accesses, and
> the board code should not cause the user's selection to change).

I agree with your concern.
The main reason I mmc_cur_dev global is for cmd_mmc cannot get the info
of how many slots has been enabled mmc.c

I think make a function in mmc.c like mmc_get_num() which would also fulfill
my need.

>
>
>>
>> ?int __board_mmc_getcd(u8 *cd, struct mmc *mmc) {
>> ? ? ? ?return -1;
>> @@ -849,7 +849,7 @@ int mmc_register(struct mmc *mmc)
>> ?{
>> ? ? ? ?/* Setup the universal parts of the block interface just once */
>> ? ? ? ?mmc->block_dev.if_type = IF_TYPE_MMC;
>> - ? ? ? mmc->block_dev.dev = cur_dev_num++;
>> + ? ? ? mmc->block_dev.dev = mmc_cur_dev++;
>
> Make sure to switch this back
>
>> ? ? ? ?mmc->block_dev.removable = 1;
>> ? ? ? ?mmc->block_dev.block_read = mmc_bread;
>> ? ? ? ?mmc->block_dev.block_write = mmc_bwrite;
>> @@ -936,12 +936,20 @@ void print_mmc_devices(char separator)
>>
>> ?int mmc_initialize(bd_t *bis)
>> ?{
>> + ? ? ? int ret = -1;
>> ? ? ? ?INIT_LIST_HEAD (&mmc_devices);
>> - ? ? ? cur_dev_num = 0;
>> + ? ? ? mmc_cur_dev = 0;
>>
>> ? ? ? ?if (board_mmc_init(bis) < 0)
>> - ? ? ? ? ? ? ? cpu_mmc_init(bis);
>> + ? ? ? ? ? ? ? ret = cpu_mmc_init(bis);
>> + ? ? ? else
>> + ? ? ? ? ? ? ? ret = 0;
>>
>> + ? ? ? /* Put the device 0 as the default */
>> + ? ? ? if (ret < 0)
>> + ? ? ? ? ? ? ? mmc_cur_dev = -1;
>> + ? ? ? else
>> + ? ? ? ? ? ? ? mmc_cur_dev = 0;
>
> These seems needlessly awkward. I suggest that we can initialize it
> either to zero, or to the last register device, inside mmc_register().
> You don't need "ret" at all, then. Because this should be in cmd_mmc,
> we would add a function to cmd_mmc, like: mmc_set_current_dev(struct
> mmc *)

I agree. mmc_set_current_dev may not need, as cmd_mmc still pass the dev
num when it call the read/write function to the mmc.c.

>
>
>> ? ? ? ?print_mmc_devices(',');
>>
>> ? ? ? ?return 0;
>> diff --git a/include/mmc.h b/include/mmc.h
>> index fcd0fd1..dd9bf15 100644
>> --- a/include/mmc.h
>> +++ b/include/mmc.h
>> @@ -295,4 +295,7 @@ int atmel_mci_init(void *regs);
>> ?int mmc_legacy_init(int verbose);
>> ?#endif
>>
>> +#ifdef CONFIG_GENERIC_MMC
>> +extern int mmc_cur_dev;
>> +#endif
>
> And then this isn't necessary, but you can declare mmc_set_current_dev(), here
>
Best regards,
Lei

[U-Boot] [PATCH V2 1/2] cmd_mmc: eliminate device num in the mmc command

[Andy Fleming]

On Wed, Apr 27, 2011 at 2:12 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Andy Fleming,
>
> In message <BANLkTimmtV9L9P75A-PLez5rhVy4hYg=gA@mail.gmail.com> you wrote:
>>
>> > From now on, follow the general rule "mmc dev [dev]" to change the
>> > mmc command applied device, like ide and usb...
> ...
>> I'd really prefer if there were still the option to specify the device
>> in the read/write commands. I appreciate the convenience of not having
>> to specify it every time, but I feel that by doing things this way, we
>> create an artificial separation between a transaction, and the target
>> of the transaction. It tends to encourage a notion that a transaction
>> can only be done to the global-current device, which is fine for
>> command-line interfaces, but can result in broken programming
>> interfaces
>
> PLease see the preceeding thread for the reasons for this interface
> (make it the same as what we have with USB, IDE, etc.).
>
> Yes, it is not a nice one. ?We really want a beter device model.


I'm fine with supporting a consistent interface, but I think what I'm
suggesting doesn't break that.  After all, if we follow my suggestion,
one can quite happily pretend that the command line interface
requires:

mmc dev x:y
mmc read <addr> <blk> <cnt>

It's just that the other way would also be possible. I re-read the
thread where this was discussed, and while I definitely agree that the
originally proposed solution was highly error-prone, I think
*allowing* users to do this is fine:

mmc read <dev>:<part> <addr> <blk> <cnt>

Is there a reason that this wouldn't work with the current model?

Andy

[U-Boot] [PATCH V2 1/2] cmd_mmc: eliminate device num in the mmc command

[Andy Fleming]

On Wed, Apr 27, 2011 at 10:44 PM, Lei Wen <adrian.wenl@gmail.com> wrote:

>>> - ? ? ? ? ? ? ? ? ? ? ? mmc_init(mmc);
>>> + ? ? ? ? ? ? ? return 0;
>>> + ? ? ? } else if (strncmp(argv[1], "part", 4) == 0) {
>>> + ? ? ? ? ? ? ? block_dev_desc_t *mmc_dev;
>>> + ? ? ? ? ? ? ? struct mmc *mmc = find_mmc_device(mmc_cur_dev);
>>>
>>> + ? ? ? ? ? ? ? if (!mmc) {
>>> + ? ? ? ? ? ? ? ? ? ? ? puts("no mmc devices available\n");
>>
>> Technically, this just means that no device is selected. I'm sure that
>> would only happen if no devices were available, but that's an
>> assumption carried outside of the vicinity of this code. Someone might
>> change that assumption (for instance, if something could cause an mmc
>> device to be removed)
>
> If that device be removed, I think the following mmc_init could check it out,
> wouldn't it? For this check, my understanding is to see whether that slot has
> been register or not, then following init would double check device whether
> function.

Well, here I'm talking about some piece of code *programmatically*
removing a registered device. Right now, we don't support that, and I
don't suspect we will anytime soon, but my point was that the error
message should state what the actual problem was.  At best, this code
can *guess* that no mmc devices are available, but it doesn't know
that.  It only knows that it didn't find mmc_cur_dev. Also, by telling
the user that you didn't find mmc_cur_dev, the user can try to figure
out whether mmc_cur_dev got corrupted, or if he accidentally set it to
the wrong value.


>
>>
>>> + ? ? ? ? ? ? ? ? ? ? ? return 1;
>>> + ? ? ? ? ? ? ? }
>>> + ? ? ? ? ? ? ? mmc_init(mmc);
>>> + ? ? ? ? ? ? ? mmc_dev = mmc_get_dev(mmc_cur_dev);
>>> + ? ? ? ? ? ? ? if (mmc_dev != NULL &&
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? mmc_dev->type != DEV_TYPE_UNKNOWN) {
>>> + ? ? ? ? ? ? ? ? ? ? ? print_part(mmc_dev);
>>
>> [...]
>>
>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>>> index 6805b33..377f13a 100644
>>> --- a/drivers/mmc/mmc.c
>>> +++ b/drivers/mmc/mmc.c
>>> @@ -34,7 +34,7 @@
>>> ?#include <div64.h>
>>>
>>> ?static struct list_head mmc_devices;
>>> -static int cur_dev_num = -1;
>>
>>
>> Please don't remove this, or conflate this value with the global
>> "current" pointer. This is used to assign the block dev num.
>> mmc_cur_dev is a runtime value that will change. ?If, for some reason,
>> someone needs to interact with an mmc device before all of them have
>> come up, we will utterly break. ?This is the sort of reason that I'm
>> not big on this sort of interaction paradigm.
>>
>>
>>> +int mmc_cur_dev = -1;
>>
>>
>> You should make this a pointer to the mmc device, not an integer.
>> Also, you should keep it in the cmd_mmc file. ?The *only* place this
>> should be used is to declare what the current device for user
>> interaction is. Board code that wants to interact with MMC devices
>> should feel free to do so, irrespective of the user (ie, the user's
>> desired card should not affect which card the board code accesses, and
>> the board code should not cause the user's selection to change).
>
> I agree with your concern.
> The main reason I mmc_cur_dev global is for cmd_mmc cannot get the info
> of how many slots has been enabled mmc.c
>
> I think make a function in mmc.c like mmc_get_num() which would also fulfill
> my need.


I'm not that concerned about where the variable sits. If it's in
mmc.c, that's fine.  But it must not be used for any purpose other
than to record what device was last selected for use by the mmc
commands. Other clients (such as early boot code) should have no sense
of that state.


Andy

[U-Boot] [PATCH V2 1/2] cmd_mmc: eliminate device num in the mmc command

[Wolfgang Denk]

Dear Andy Fleming,

In message <BANLkTi=+tGpet6BDHiY_7Xx2dyiDbcu9DQ@mail.gmail.com> you wrote:
>
> It's just that the other way would also be possible. I re-read the
> thread where this was discussed, and while I definitely agree that the
> originally proposed solution was highly error-prone, I think
> *allowing* users to do this is fine:
> 
> mmc read <dev>:<part> <addr> <blk> <cnt>
> 
> Is there a reason that this wouldn't work with the current model?

Of course it works, but I don't like it:

- I think different user interfaces for the same thing are usually
  confusing.
- Defining a policy will be necessay, and even then it may cause side
  effects. For example, does "mmc read <dev>:<part> ..." change the
  "current" device settings, or are these only temporarily active
  while the command is running? etc.

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
Every living thing wants to survive.
	-- Spock, "The Ultimate Computer", stardate 4731.3

[U-Boot] [PATCH V2 1/2] cmd_mmc: eliminate device num in the mmc command

[Mike Frysinger]

On Friday, April 29, 2011 06:20:36 Wolfgang Denk wrote:
> Andy Fleming wrote:
> > It's just that the other way would also be possible. I re-read the
> > thread where this was discussed, and while I definitely agree that the
> > originally proposed solution was highly error-prone, I think
> > *allowing* users to do this is fine:
> > 
> > mmc read <dev>:<part> <addr> <blk> <cnt>
> > 
> > Is there a reason that this wouldn't work with the current model?
> 
> Of course it works, but I don't like it:
> 
> - I think different user interfaces for the same thing are usually
>   confusing.

and bloatful
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110430/277a1511/attachment.pgp 

[U-Boot] [PATCH V3 0/2] mmc: add partition support

[Lei Wen]

V2:
use the "mmc_cur_dev" to replace explicitly specify the dev num in mmc command.
change the switch parittion command per Wolfgang's suggestion

V3:
reuse "curr_device" in hte cmd_mmc.c
Modify notification message

Lei Wen (2):
  cmd_mmc: eliminate device num in the mmc command
  mmc: enable partition switch fucntion for emmc

 common/cmd_mmc.c  |  224 ++++++++++++++++++++++++++++++++--------------------
 drivers/mmc/mmc.c |   35 ++++++++-
 include/mmc.h     |    9 ++
 3 files changed, 181 insertions(+), 87 deletions(-)

[U-Boot] [PATCH V3 1/2] cmd_mmc: eliminate device num in the mmc command

[Lei Wen]

[U-Boot] [PATCH V3 2/2] mmc: enable partition switch fucntion for emmc

[Lei Wen]

For emmc, it may has upto 7 partitions: two boot partitions, one
user partition, one RPMB partition and four general purpose partitions.
(Refer to JESD84-A44.pdf/page 154)

As bootloader may need to read out or reflashing images on those
different partitions, it is better to enable the partition switch with
console command support.

Also for partition would be restore to user partition(part 0) when CMD0
is used, so change mmc_init routine to perform normal initialization
only once for each slot, unless use the rescan command to force init
again.

Signed-off-by: Lei Wen <leiwen@marvell.com>
---
Changelog:
V2:
change the switch parittion command per Wolfgang's suggestion

V3:
No Change.

 common/cmd_mmc.c  |   38 ++++++++++++++++++++++++++++++++++----
 drivers/mmc/mmc.c |   30 +++++++++++++++++++++++++++++-
 include/mmc.h     |    8 ++++++++
 3 files changed, 71 insertions(+), 5 deletions(-)

diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
index e266d4f..176646d 100644
--- a/common/cmd_mmc.c
+++ b/common/cmd_mmc.c
@@ -164,6 +164,7 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			return 1;
 		}
 
+		mmc->has_init = 0;
 		mmc_init(mmc);
 
 		return 0;
@@ -189,14 +190,22 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		print_mmc_devices('\n');
 		return 0;
 	} else if (strcmp(argv[1], "dev") == 0) {
-		int dev;
+		int dev, part = -1;
 		struct mmc *mmc;
 
 		if (argc == 2)
 			dev = curr_device;
 		else if (argc == 3)
 			dev = simple_strtoul(argv[2], NULL, 10);
-		else
+		else if (argc == 4) {
+			dev = (int)simple_strtoul(argv[2], NULL, 10);
+			part = (int)simple_strtoul(argv[3], NULL, 10);
+			if (part > PART_ACCESS_MASK) {
+				printf("#part_num shouldn't be larger"
+					" than %d\n", PART_ACCESS_MASK);
+				return 1;
+			}
+		} else
 			return cmd_usage(cmdtp);
 
 		mmc = find_mmc_device(dev);
@@ -205,8 +214,29 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			return 1;
 		}
 
+		mmc_init(mmc);
+		if (part != -1) {
+			int ret;
+			if (mmc->part_config == MMCPART_NOAVAILABLE) {
+				printf("Card doesn't support part_switch\n");
+				return 1;
+			}
+
+			if (part != mmc->part_num) {
+				ret = mmc_switch_part(dev, part);
+				if (!ret)
+					mmc->part_num = part;
+
+				printf("switch to partions #%d, %s\n",
+						part, (!ret) ? "OK" : "ERROR");
+			}
+		}
 		curr_device = dev;
-		printf("mmc%d is current device\n", curr_device);
+		if (mmc->part_config == MMCPART_NOAVAILABLE)
+			printf("mmc%d is current device\n", curr_device);
+		else
+			printf("mmc%d(part %d) is current device\n",
+				curr_device, mmc->part_num);
 
 		return 0;
 	} else if (strcmp(argv[1], "read") == 0) {
@@ -269,6 +299,6 @@ U_BOOT_CMD(
 	"mmc write addr blk# cnt\n"
 	"mmc rescan\n"
 	"mmc part - lists available partition on current mmc device\n"
-	"mmc dev [dev] - show or set current mmc device\n"
+	"mmc dev [dev] [part] - show or set current mmc device [partition]\n"
 	"mmc list - lists available devices");
 #endif
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index cdf2713..1d089a7 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -577,6 +577,18 @@ int mmc_change_freq(struct mmc *mmc)
 	return 0;
 }
 
+int mmc_switch_part(int dev_num, unsigned int part_num)
+{
+	struct mmc *mmc = find_mmc_device(dev_num);
+
+	if (!mmc)
+		return -1;
+
+	return mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF,
+			  (mmc->part_config & ~PART_ACCESS_MASK)
+			  | (part_num & PART_ACCESS_MASK));
+}
+
 int sd_switch(struct mmc *mmc, int mode, int group, u8 value, u8 *resp)
 {
 	struct mmc_cmd cmd;
@@ -899,6 +911,7 @@ int mmc_startup(struct mmc *mmc)
 			return err;
 	}
 
+	mmc->part_config = MMCPART_NOAVAILABLE;
 	if (!IS_SD(mmc) && (mmc->version >= MMC_VERSION_4)) {
 		/* check  ext_csd version and capacity */
 		err = mmc_send_ext_csd(mmc, ext_csd);
@@ -907,6 +920,10 @@ int mmc_startup(struct mmc *mmc)
 					ext_csd[214] << 16 | ext_csd[215] << 24;
 			mmc->capacity *= 512;
 		}
+
+		/* store the partition info of emmc */
+		if (ext_csd[160] & PART_SUPPORT)
+			mmc->part_config = ext_csd[179];
 	}
 
 	if (IS_SD(mmc))
@@ -1048,6 +1065,9 @@ int mmc_init(struct mmc *mmc)
 {
 	int err;
 
+	if (mmc->has_init)
+		return 0;
+
 	err = mmc->init(mmc);
 
 	if (err)
@@ -1062,6 +1082,9 @@ int mmc_init(struct mmc *mmc)
 	if (err)
 		return err;
 
+	/* The internal partition reset to user partition(0) at every CMD0*/
+	mmc->part_num = 0;
+
 	/* Test for SD version 2 */
 	err = mmc_send_if_cond(mmc);
 
@@ -1078,7 +1101,12 @@ int mmc_init(struct mmc *mmc)
 		}
 	}
 
-	return mmc_startup(mmc);
+	err = mmc_startup(mmc);
+	if (err)
+		mmc->has_init = 0;
+	else
+		mmc->has_init = 1;
+	return err;
 }
 
 /*
diff --git a/include/mmc.h b/include/mmc.h
index b1ad086..4d6881c 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -137,6 +137,7 @@
  * EXT_CSD fields
  */
 
+#define EXT_CSD_PART_CONF	179	/* R/W */
 #define EXT_CSD_BUS_WIDTH	183	/* R/W */
 #define EXT_CSD_HS_TIMING	185	/* R/W */
 #define EXT_CSD_CARD_TYPE	196	/* RO */
@@ -178,6 +179,9 @@
 #define MMC_RSP_R6	(MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
 #define MMC_RSP_R7	(MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
 
+#define MMCPART_NOAVAILABLE	(0xff)
+#define PART_ACCESS_MASK	(0x7)
+#define PART_SUPPORT		(0x1)
 
 struct mmc_cid {
 	unsigned long psn;
@@ -262,6 +266,7 @@ struct mmc {
 	void *priv;
 	uint voltages;
 	uint version;
+	uint has_init;
 	uint f_min;
 	uint f_max;
 	int high_capacity;
@@ -274,6 +279,8 @@ struct mmc {
 	uint csd[4];
 	uint cid[4];
 	ushort rca;
+	char part_config;
+	char part_num;
 	uint tran_speed;
 	uint read_bl_len;
 	uint write_bl_len;
@@ -296,6 +303,7 @@ int mmc_set_dev(int dev_num);
 void print_mmc_devices(char separator);
 int get_mmc_num(void);
 int board_mmc_getcd(u8 *cd, struct mmc *mmc);
+int mmc_switch_part(int dev_num, unsigned int part_num);
 
 #ifdef CONFIG_GENERIC_MMC
 int atmel_mci_init(void *regs);
-- 
1.7.0.4

[U-Boot] [PATCH V2 1/2] cmd_mmc: eliminate device num in the mmc command

[Lei Wen]

Hi Andy,

On Fri, Apr 29, 2011 at 3:57 PM, Andy Fleming <afleming@gmail.com> wrote:
> On Wed, Apr 27, 2011 at 10:44 PM, Lei Wen <adrian.wenl@gmail.com> wrote:
>
>>>> - ? ? ? ? ? ? ? ? ? ? ? mmc_init(mmc);
>>>> + ? ? ? ? ? ? ? return 0;
>>>> + ? ? ? } else if (strncmp(argv[1], "part", 4) == 0) {
>>>> + ? ? ? ? ? ? ? block_dev_desc_t *mmc_dev;
>>>> + ? ? ? ? ? ? ? struct mmc *mmc = find_mmc_device(mmc_cur_dev);
>>>>
>>>> + ? ? ? ? ? ? ? if (!mmc) {
>>>> + ? ? ? ? ? ? ? ? ? ? ? puts("no mmc devices available\n");
>>>
>>> Technically, this just means that no device is selected. I'm sure that
>>> would only happen if no devices were available, but that's an
>>> assumption carried outside of the vicinity of this code. Someone might
>>> change that assumption (for instance, if something could cause an mmc
>>> device to be removed)
>>
>> If that device be removed, I think the following mmc_init could check it out,
>> wouldn't it? For this check, my understanding is to see whether that slot has
>> been register or not, then following init would double check device whether
>> function.
>
> Well, here I'm talking about some piece of code *programmatically*
> removing a registered device. Right now, we don't support that, and I
> don't suspect we will anytime soon, but my point was that the error
> message should state what the actual problem was. ?At best, this code
> can *guess* that no mmc devices are available, but it doesn't know
> that. ?It only knows that it didn't find mmc_cur_dev. Also, by telling
> the user that you didn't find mmc_cur_dev, the user can try to figure
> out whether mmc_cur_dev got corrupted, or if he accidentally set it to
> the wrong value.
>
>
>>
>>>
>>>> + ? ? ? ? ? ? ? ? ? ? ? return 1;
>>>> + ? ? ? ? ? ? ? }
>>>> + ? ? ? ? ? ? ? mmc_init(mmc);
>>>> + ? ? ? ? ? ? ? mmc_dev = mmc_get_dev(mmc_cur_dev);
>>>> + ? ? ? ? ? ? ? if (mmc_dev != NULL &&
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? mmc_dev->type != DEV_TYPE_UNKNOWN) {
>>>> + ? ? ? ? ? ? ? ? ? ? ? print_part(mmc_dev);
>>>
>>> [...]
>>>
>>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>>>> index 6805b33..377f13a 100644
>>>> --- a/drivers/mmc/mmc.c
>>>> +++ b/drivers/mmc/mmc.c
>>>> @@ -34,7 +34,7 @@
>>>> ?#include <div64.h>
>>>>
>>>> ?static struct list_head mmc_devices;
>>>> -static int cur_dev_num = -1;
>>>
>>>
>>> Please don't remove this, or conflate this value with the global
>>> "current" pointer. This is used to assign the block dev num.
>>> mmc_cur_dev is a runtime value that will change. ?If, for some reason,
>>> someone needs to interact with an mmc device before all of them have
>>> come up, we will utterly break. ?This is the sort of reason that I'm
>>> not big on this sort of interaction paradigm.
>>>
>>>
>>>> +int mmc_cur_dev = -1;
>>>
>>>
>>> You should make this a pointer to the mmc device, not an integer.
>>> Also, you should keep it in the cmd_mmc file. ?The *only* place this
>>> should be used is to declare what the current device for user
>>> interaction is. Board code that wants to interact with MMC devices
>>> should feel free to do so, irrespective of the user (ie, the user's
>>> desired card should not affect which card the board code accesses, and
>>> the board code should not cause the user's selection to change).
>>
>> I agree with your concern.
>> The main reason I mmc_cur_dev global is for cmd_mmc cannot get the info
>> of how many slots has been enabled mmc.c
>>
>> I think make a function in mmc.c like mmc_get_num() which would also fulfill
>> my need.
>
>
> I'm not that concerned about where the variable sits. If it's in
> mmc.c, that's fine. ?But it must not be used for any purpose other
> than to record what device was last selected for use by the mmc
> commands. Other clients (such as early boot code) should have no sense
> of that state.

I have submit another round of patch, please review it whether it meet
your expectation.

Thanks,
Lei

[U-Boot] [PATCH V2 1/2] cmd_mmc: eliminate device num in the mmc command

[Andy Fleming]

On Mon, May 2, 2011 at 9:27 PM, Lei Wen <adrian.wenl@gmail.com> wrote:

>
> I have submit another round of patch, please review it whether it meet
> your expectation.

This is just a quick note to say I haven't forgotten about these or
other patches in my queue.  I'm just buried in other work at the
moment. I intend to get these looked at this week.

Andy

[U-Boot] [PATCH V2 1/2] cmd_mmc: eliminate device num in the mmc command

[Andy Fleming]

>
> I have submit another round of patch, please review it whether it meet
> your expectation.

Applied, thanks!