[U-Boot] [PATCH 0/2] New abstraction layer for handling MMC and FAT commands

[U-Boot] [PATCH 0/2] New abstraction layer for handling MMC and FAT commands

[Lukasz Majewski]

New abstraction layer for performing MMC and FAT operations has been added.
It is necessary for using MMC and FAT operations from other subsystems without
the need to call "sprintf" and "run_command" afterwards.

MMC/FAT function call allows for better type checking during compilation time.
For FAT, common code has been encapsulated to a separate function.

Lukasz Majewski (2):
  mmc: New abstract function (do_mmcops_safe) for MMC subsystem
  fat: New abstract function (do_fat_safe) for FAT subsystem

 common/cmd_fat.c |   89 +++++++++++++++++++++++++++++++-----------------------
 common/cmd_mmc.c |   69 ++++++++++++++++++++++-------------------
 common/env_fat.c |   20 +-----------
 include/fat.h    |    7 ++++
 include/mmc.h    |    9 +++++
 5 files changed, 106 insertions(+), 88 deletions(-)

-- 
1.7.2.3

[U-Boot] [PATCH 1/2] mmc: New abstract function (do_mmcops_safe) for MMC subsystem

[Lukasz Majewski]

The new do_mmcops_safe function has been added to the mmc subsystem.
It is necessary to decouple the core mmc read/write operation from
code responsible for parsing user input.

The do_mmcops_safe function shall be used from other subsystems to
read/write data to MMC devices.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Andy Fleming <afleming@gmail.com>
---
 common/cmd_mmc.c |   69 +++++++++++++++++++++++++++++-------------------------
 include/mmc.h    |    9 +++++++
 2 files changed, 46 insertions(+), 32 deletions(-)

diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
index 750509d..60e6873 100644
--- a/common/cmd_mmc.c
+++ b/common/cmd_mmc.c
@@ -87,12 +87,6 @@ U_BOOT_CMD(
 );
 #else /* !CONFIG_GENERIC_MMC */
 
-enum mmc_state {
-	MMC_INVALID,
-	MMC_READ,
-	MMC_WRITE,
-	MMC_ERASE,
-};
 static void print_mmcinfo(struct mmc *mmc)
 {
 	printf("Device: %s\n", mmc->name);
@@ -148,7 +142,42 @@ U_BOOT_CMD(
 	""
 );
 
-int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+u32 do_mmcops_safe(enum mmc_state state, int curr_device, u32 blk,
+		   u32 cnt, void *addr)
+{
+	struct mmc *mmc = find_mmc_device(curr_device);
+	u32 n = 0;
+
+	if (!mmc) {
+		printf("no mmc device at slot %x\n", curr_device);
+		return 1;
+	}
+
+	mmc_init(mmc);
+
+	switch (state) {
+	case MMC_READ:
+		n = mmc->block_dev.block_read(curr_device, blk,
+					      cnt, addr);
+		/* flush cache after read */
+		flush_cache((ulong)addr, cnt * 512); /* FIXME */
+		break;
+	case MMC_WRITE:
+		n = mmc->block_dev.block_write(curr_device, blk,
+					       cnt, addr);
+		break;
+	case MMC_ERASE:
+		n = mmc->block_dev.block_erase(curr_device, blk, cnt);
+		break;
+	default:
+		BUG();
+	}
+
+	return n;
+}
+
+int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc,
+		       char * const argv[])
 {
 	enum mmc_state state;
 
@@ -261,7 +290,6 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		state = MMC_INVALID;
 
 	if (state != MMC_INVALID) {
-		struct mmc *mmc = find_mmc_device(curr_device);
 		int idx = 2;
 		u32 blk, cnt, n;
 		void *addr;
@@ -274,33 +302,10 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		blk = simple_strtoul(argv[idx], NULL, 16);
 		cnt = simple_strtoul(argv[idx + 1], NULL, 16);
 
-		if (!mmc) {
-			printf("no mmc device at slot %x\n", curr_device);
-			return 1;
-		}
-
 		printf("\nMMC %s: dev # %d, block # %d, count %d ... ",
 				argv[1], curr_device, blk, cnt);
 
-		mmc_init(mmc);
-
-		switch (state) {
-		case MMC_READ:
-			n = mmc->block_dev.block_read(curr_device, blk,
-						      cnt, addr);
-			/* flush cache after read */
-			flush_cache((ulong)addr, cnt * 512); /* FIXME */
-			break;
-		case MMC_WRITE:
-			n = mmc->block_dev.block_write(curr_device, blk,
-						      cnt, addr);
-			break;
-		case MMC_ERASE:
-			n = mmc->block_dev.block_erase(curr_device, blk, cnt);
-			break;
-		default:
-			BUG();
-		}
+		n = do_mmcops_safe(state, curr_device, blk, cnt, addr);
 
 		printf("%d blocks %s: %s\n",
 				n, argv[1], (n == cnt) ? "OK" : "ERROR");
diff --git a/include/mmc.h b/include/mmc.h
index 2305986..5686e1b 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -199,6 +199,13 @@
 #define PART_ACCESS_MASK	(0x7)
 #define PART_SUPPORT		(0x1)
 
+enum mmc_state {
+	MMC_INVALID,
+	MMC_READ,
+	MMC_WRITE,
+	MMC_ERASE,
+};
+
 struct mmc_cid {
 	unsigned long psn;
 	unsigned short oid;
@@ -274,6 +281,8 @@ int board_mmc_getcd(struct mmc *mmc);
 int mmc_switch_part(int dev_num, unsigned int part_num);
 int mmc_getcd(struct mmc *mmc);
 
+u32 do_mmcops_safe(enum mmc_state state, int curr_device, u32 blk,
+		   u32 cnt, void *addr);
 #ifdef CONFIG_GENERIC_MMC
 #define mmc_host_is_spi(mmc)	((mmc)->host_caps & MMC_MODE_SPI)
 struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode);
-- 
1.7.2.3

[U-Boot] [PATCH 2/2] fat: New abstract function (do_fat_safe) for FAT subsystem

[Lukasz Majewski]

The new do_fat_safe function has been added to the FAT handling command.
It is necessary to decouple the core fat command read/write operation from
code responsible for parsing user input.

The do_fat_safe function shall be used from other subsystems willing to
read/write data to FAT file system.

Due to code reorganization a common code has been removed from env_fat.c.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 common/cmd_fat.c |   89 +++++++++++++++++++++++++++++++-----------------------
 common/env_fat.c |   20 +-----------
 include/fat.h    |    7 ++++
 3 files changed, 60 insertions(+), 56 deletions(-)

diff --git a/common/cmd_fat.c b/common/cmd_fat.c
index 559a16d..a197393 100644
--- a/common/cmd_fat.c
+++ b/common/cmd_fat.c
@@ -32,6 +32,48 @@
 #include <part.h>
 #include <fat.h>
 
+long do_fat_safe(enum fat_op op, char *iname, char *file, int dev, int part,
+		       void *addr, unsigned long count)
+{
+	block_dev_desc_t *dev_desc = NULL;
+	long size = 0;
+
+	dev_desc = get_dev(iname, dev);
+	if (dev_desc == NULL) {
+		puts("\n** Invalid boot device **\n");
+		return -1;
+	}
+
+	if (fat_register_device(dev_desc, part) != 0) {
+		printf("\n** Unable to use %s %d:%d for fat%s **\n",
+		       iname, dev, part, op == FAT_READ ? "load" : "write");
+		return -1;
+	}
+
+	switch (op) {
+	case FAT_READ:
+		size = file_fat_read(file, addr, count);
+		break;
+#ifdef CONFIG_FAT_WRITE
+	case FAT_WRITE:
+		size = file_fat_write(file, addr, count);
+		break;
+#endif
+	default:
+		puts("Operation not supported!\n");
+		return -1;
+	}
+
+	if (size == -1) {
+		printf("\n** Unable to %s \"%s\" from %s %d:%d **\n",
+		       op == FAT_READ ? "read" : "write",
+		       file, iname, dev, part);
+		return -1;
+	}
+	printf("%ld bytes %s\n", size, op == FAT_READ ? "read" : "written");
+
+	return size;
+}
 
 int do_fat_fsload (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
@@ -39,7 +81,6 @@ int do_fat_fsload (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	unsigned long offset;
 	unsigned long count;
 	char buf [12];
-	block_dev_desc_t *dev_desc=NULL;
 	int dev=0;
 	int part=1;
 	char *ep;
@@ -51,11 +92,6 @@ int do_fat_fsload (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	}
 
 	dev = (int)simple_strtoul(argv[2], &ep, 16);
-	dev_desc = get_dev(argv[1],dev);
-	if (dev_desc == NULL) {
-		puts("\n** Invalid boot device **\n");
-		return 1;
-	}
 	if (*ep) {
 		if (*ep != ':') {
 			puts("\n** Invalid boot device, use `dev[:part]' **\n");
@@ -63,25 +99,17 @@ int do_fat_fsload (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		}
 		part = (int)simple_strtoul(++ep, NULL, 16);
 	}
-	if (fat_register_device(dev_desc,part)!=0) {
-		printf("\n** Unable to use %s %d:%d for fatload **\n",
-			argv[1], dev, part);
-		return 1;
-	}
 	offset = simple_strtoul(argv[3], NULL, 16);
 	if (argc == 6)
 		count = simple_strtoul(argv[5], NULL, 16);
 	else
 		count = 0;
-	size = file_fat_read(argv[4], (unsigned char *)offset, count);
 
-	if(size==-1) {
-		printf("\n** Unable to read \"%s\" from %s %d:%d **\n",
-			argv[4], argv[1], dev, part);
-		return 1;
-	}
+	size = do_fat_safe(FAT_READ, argv[1], argv[4], dev, part,
+			     (void *)offset, count);
 
-	printf("\n%ld bytes read\n", size);
+	if (size == -1)
+		return 1;
 
 	sprintf(buf, "%lX", size);
 	setenv("filesize", buf);
@@ -134,7 +162,7 @@ int do_fat_ls (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	else
 		ret = file_fat_ls(filename);
 
-	if(ret!=0)
+	if (ret != 0)
 		printf("No Fat FS detected\n");
 	return ret;
 }
@@ -189,10 +217,9 @@ U_BOOT_CMD(
 static int do_fat_fswrite(cmd_tbl_t *cmdtp, int flag,
 		int argc, char * const argv[])
 {
-	long size;
 	unsigned long addr;
 	unsigned long count;
-	block_dev_desc_t *dev_desc = NULL;
+
 	int dev = 0;
 	int part = 1;
 	char *ep;
@@ -201,11 +228,6 @@ static int do_fat_fswrite(cmd_tbl_t *cmdtp, int flag,
 		return cmd_usage(cmdtp);
 
 	dev = (int)simple_strtoul(argv[2], &ep, 16);
-	dev_desc = get_dev(argv[1], dev);
-	if (dev_desc == NULL) {
-		puts("\n** Invalid boot device **\n");
-		return 1;
-	}
 	if (*ep) {
 		if (*ep != ':') {
 			puts("\n** Invalid boot device, use `dev[:part]' **\n");
@@ -213,22 +235,13 @@ static int do_fat_fswrite(cmd_tbl_t *cmdtp, int flag,
 		}
 		part = (int)simple_strtoul(++ep, NULL, 16);
 	}
-	if (fat_register_device(dev_desc, part) != 0) {
-		printf("\n** Unable to use %s %d:%d for fatwrite **\n",
-			argv[1], dev, part);
-		return 1;
-	}
+
 	addr = simple_strtoul(argv[3], NULL, 16);
 	count = simple_strtoul(argv[5], NULL, 16);
 
-	size = file_fat_write(argv[4], (void *)addr, count);
-	if (size == -1) {
-		printf("\n** Unable to write \"%s\" from %s %d:%d **\n",
-			argv[4], argv[1], dev, part);
+	if (do_fat_safe(FAT_WRITE, argv[1], argv[4], dev, part,
+			      (void *)addr, count) == -1)
 		return 1;
-	}
-
-	printf("%ld bytes written\n", size);
 
 	return 0;
 }
diff --git a/common/env_fat.c b/common/env_fat.c
index bad92aa..35d8ac0 100644
--- a/common/env_fat.c
+++ b/common/env_fat.c
@@ -85,25 +85,9 @@ int saveenv(void)
 	}
 #endif /* CONFIG_MMC */
 
-	dev_desc = get_dev(FAT_ENV_INTERFACE, dev);
-	if (dev_desc == NULL) {
-		printf("Failed to find %s%d\n",
-			FAT_ENV_INTERFACE, dev);
-		return 1;
-	}
-	if (fat_register_device(dev_desc, part) != 0) {
-		printf("Failed to register %s%d:%d\n",
-			FAT_ENV_INTERFACE, dev, part);
-		return 1;
-	}
-
 	env_new.crc = crc32(0, env_new.data, ENV_SIZE);
-	if (file_fat_write(FAT_ENV_FILE, (void *)&env_new, sizeof(env_t)) == -1) {
-		printf("\n** Unable to write \"%s\" from %s%d:%d **\n",
-			FAT_ENV_FILE, FAT_ENV_INTERFACE, dev, part);
-		return 1;
-	}
-
+	do_fat_safe(FAT_WRITE, FAT_ENV_INTERFACE, FAT_ENV_FILE, dev, part,
+			  (void *)&env_new, sizeof(env_t));
 	puts("done\n");
 	return 0;
 }
diff --git a/include/fat.h b/include/fat.h
index f1b4a0d..20a0682 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -199,6 +199,11 @@ struct filesystem {
 	const char		name[12];
 };
 
+enum fat_op {
+	FAT_READ = 1,
+	FAT_WRITE
+};
+
 /* FAT tables */
 file_detectfs_func	file_fat_detectfs;
 file_ls_func		file_fat_ls;
@@ -213,4 +218,6 @@ const char *file_getfsname(int idx);
 int fat_register_device(block_dev_desc_t *dev_desc, int part_no);
 
 int file_fat_write(const char *filename, void *buffer, unsigned long maxsize);
+long do_fat_safe(enum fat_op op, char *iname, char *file, int dev, int part,
+		       void *addr, unsigned long count);
 #endif /* _FAT_H_ */
-- 
1.7.2.3

[U-Boot] [PATCH 0/2] New abstraction layer for handling MMC and FAT commands

[Wolfgang Denk]

Dear Lukasz Majewski,

In message <1343378116-5569-1-git-send-email-l.majewski@samsung.com> you wrote:
> New abstraction layer for performing MMC and FAT operations has been added.
> It is necessary for using MMC and FAT operations from other subsystems without
> the need to call "sprintf" and "run_command" afterwards.
> 
> MMC/FAT function call allows for better type checking during compilation time.
> For FAT, common code has been encapsulated to a separate function.
> 
> Lukasz Majewski (2):
>   mmc: New abstract function (do_mmcops_safe) for MMC subsystem
>   fat: New abstract function (do_fat_safe) for FAT subsystem

Sorry if I don't understand, but what exactly is special with MMC and
FAT here?

Why are the same changes as to the MMC code not also needed for
example for the IDE, SATA, SCSI or USB code?

Why are the same changes as for FAT not also needed for the other file
systems?


And what exactly are you changing anyway?  The commit messages in the
patches give neither sufficient information to understand why you make
a change at all, nor how the new code is supposed to be used, nor in
which way he is better ("more safe" ?) than the existing code?


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 ain't experienced..." "Well, nor are you." "That's true. But the
point is ... the point is ... the point is we've been not experienced
for a lot longer than you. We've got  a  lot  of  experience  of  not
having any experience."           - Terry Pratchett, _Witches Abroad_

[U-Boot] [PATCH 0/2] New abstraction layer for handling MMC and FAT commands

[Lukasz Majewski]

Dear Wolfgang Denk,

> Dear Lukasz Majewski,
> 
> In message <1343378116-5569-1-git-send-email-l.majewski@samsung.com>
> you wrote:
> > New abstraction layer for performing MMC and FAT operations has
> > been added. It is necessary for using MMC and FAT operations from
> > other subsystems without the need to call "sprintf" and
> > "run_command" afterwards.
> > 
> > MMC/FAT function call allows for better type checking during
> > compilation time. For FAT, common code has been encapsulated to a
> > separate function.
> > 
> > Lukasz Majewski (2):
> >   mmc: New abstract function (do_mmcops_safe) for MMC subsystem
> >   fat: New abstract function (do_fat_safe) for FAT subsystem
> 
> Sorry if I don't understand, but what exactly is special with MMC and
> FAT here?

Those patches are a follow up of a discussion about DFU support in
u-boot.
"[PATCH 4/7] dfu: MMC specific routines for DFU operation"

http://article.gmane.org/gmane.comp.boot-loaders.u-boot/134401/match=dfu+mmc+specific+routines+dfu+operation

In short - during this discussion it has been suggested, that
sprintf() + run_command() call shall be replaced with a "_safe" function
call, which decouples "real" e.g. MMC write from parsing user data
from prompt. 

> 
> Why are the same changes as to the MMC code not also needed for
> example for the IDE, SATA, SCSI or USB code?

I need the FAT and MMC for DFU patches, on which I work now. Those
patches (actually v2) have already been posted to ML and are under
review.


> 
> Why are the same changes as for FAT not also needed for the other file
> systems?

It is because DFU is not supporting other file systems for now. I only
changed the code which I use.

> 
> 
> And what exactly are you changing anyway?  The commit messages in the
> patches give neither sufficient information to understand why you make
> a change at all, nor how the new code is supposed to be used, nor in
> which way he is better ("more safe" ?) than the existing code?

The problem is as follows:
1. DFU patches requires access to FAT and MMC
2. I'm using sprintf() + run_command() approach to execute e.g. "mmc
write 0x44000000 60 100" to store data from DFU in a proper place.
3. The approach presented at 2. was under discussion on ML. Reviewers
suggested, that run_command() approach is unsafe and for compilation
time type checking I shall write mmc with using special "_safe"
function.

For more details, please visit link, which I've posted. 
In this discussion we are arguing about pros and cons of using sprintf
+ run_command approach.

-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH 0/2] New abstraction layer for handling MMC and FAT commands

[Wolfgang Denk]

Dear Lukasz Majewski,

In message <20120727123832.69195dcd@amdc308.digital.local> you wrote:
> 
> > Sorry if I don't understand, but what exactly is special with MMC and
> > FAT here?
> 
> Those patches are a follow up of a discussion about DFU support in
> u-boot.
> "[PATCH 4/7] dfu: MMC specific routines for DFU operation"

OK, I see.   Guess I will have to start reading these patches then
(which I didn't so far).

> In short - during this discussion it has been suggested, that
> sprintf() + run_command() call shall be replaced with a "_safe" function
> call, which decouples "real" e.g. MMC write from parsing user data
> from prompt. 
...
> I need the FAT and MMC for DFU patches, on which I work now. Those
> patches (actually v2) have already been posted to ML and are under
> review.

This makes no sense to me.  MMC is just one of the storagte devices we
support, and especially with the upcoming support of a clean device
model it makes no sense to handle it special in any way.

The same holds for FAT - it is just one out of a number of file
systems we support, and it makes no sense to add any code to FAT
support that makes it incompatible with other file systems.

Any such addition should be implemented in a generic way, that is
agnostic of the underlying storage device and the file system used on
top of it.

It is OK if you then test with one combination only, but the
implementation shall be generic.

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
Intel's new motto: United we stand. Divided we fall!

[U-Boot] [PATCH 0/2] New abstraction layer for handling MMC and FAT commands

[Marek Vasut]

Dear Wolfgang Denk,

> Dear Lukasz Majewski,
> 
> In message <20120727123832.69195dcd@amdc308.digital.local> you wrote:
> > > Sorry if I don't understand, but what exactly is special with MMC and
> > > FAT here?
> > 
> > Those patches are a follow up of a discussion about DFU support in
> > u-boot.
> > "[PATCH 4/7] dfu: MMC specific routines for DFU operation"
> 
> OK, I see.   Guess I will have to start reading these patches then
> (which I didn't so far).
> 
> > In short - during this discussion it has been suggested, that
> > sprintf() + run_command() call shall be replaced with a "_safe" function
> > call, which decouples "real" e.g. MMC write from parsing user data
> > from prompt.
> 
> ...
> 
> > I need the FAT and MMC for DFU patches, on which I work now. Those
> > patches (actually v2) have already been posted to ML and are under
> > review.
> 
> This makes no sense to me.  MMC is just one of the storagte devices we
> support, and especially with the upcoming support of a clean device
> model it makes no sense to handle it special in any way.

Sure, but that might still be a release or so away. So let's not hinder people 
from doing so, rather let's coordinate. Also, I consider there patches helpful, 
as we can use that abstraction during the DM work.

> The same holds for FAT - it is just one out of a number of file
> systems we support, and it makes no sense to add any code to FAT
> support that makes it incompatible with other file systems.

It's not incompatible, how?

> Any such addition should be implemented in a generic way, that is
> agnostic of the underlying storage device and the file system used on
> top of it.

Should, that's the proper word here. It isn't in most of FSs in uboot though and 
I think this is better than nothing.

> It is OK if you then test with one combination only, but the
> implementation shall be generic.

I still consider it better than nothing -- if we have to pick this up and tear 
it apart during the DM, it's also OK, because we will easily figure out what 
parts to put where. So I'm quite fine with these patches.

> Best regards,
> 
> Wolfgang Denk

Best regards,
Marek Vasut

[U-Boot] [PATCH 0/2] New abstraction layer for handling MMC and FAT commands

[Wolfgang Denk]

Dear Marek,

In message <201207271330.46967.marex@denx.de> you wrote:
> 
> > This makes no sense to me.  MMC is just one of the storagte devices we
> > support, and especially with the upcoming support of a clean device
> > model it makes no sense to handle it special in any way.
> 
> Sure, but that might still be a release or so away. So let's not hinder people 
> from doing so, rather let's coordinate. Also, I consider there patches helpful, 
> as we can use that abstraction during the DM work.

Fine.

But why adding special code to only one storage media, and one file
system type?

> > The same holds for FAT - it is just one out of a number of file
> > systems we support, and it makes no sense to add any code to FAT
> > support that makes it incompatible with other file systems.
> 
> It's not incompatible, how?

I cannot use - say - a USB storage device instead of MMC.

Why is this so?

I will reply to that in the context of the DFU thread, it makes more
sense there.

> > Any such addition should be implemented in a generic way, that is
> > agnostic of the underlying storage device and the file system used on
> > top of it.
> 
> Should, that's the proper word here. It isn't in most of FSs in uboot though and 
> I think this is better than nothing.

Wrong.  Adding special code to one file system which makes it
"special" compared to other file systems is actually really, really
bad.


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
Romulan women are not like Vulcan females. We are  not  dedicated  to
pure logic and the sterility of non-emotion.
	-- Romulan Commander, "The Enterprise Incident",
	   stardate 5027.3

[U-Boot] [PATCH 0/2] New abstraction layer for handling MMC and FAT commands

[Marek Vasut]

Dear Wolfgang Denk,

> Dear Marek,
> 
> In message <201207271330.46967.marex@denx.de> you wrote:
> > > This makes no sense to me.  MMC is just one of the storagte devices we
> > > support, and especially with the upcoming support of a clean device
> > > model it makes no sense to handle it special in any way.
> > 
> > Sure, but that might still be a release or so away. So let's not hinder
> > people from doing so, rather let's coordinate. Also, I consider there
> > patches helpful, as we can use that abstraction during the DM work.
> 
> Fine.
> 
> But why adding special code to only one storage media, and one file
> system type?

Well, that's other thing. But I hope Lukasz will eventually extend this work 
over other media as the DFU gains support for more stuff. Right, Lukasz?

> > > The same holds for FAT - it is just one out of a number of file
> > > systems we support, and it makes no sense to add any code to FAT
> > > support that makes it incompatible with other file systems.
> > 
> > It's not incompatible, how?
> 
> I cannot use - say - a USB storage device instead of MMC.

Not _yet_ .

> Why is this so?

Because it's not implemented yet.

> I will reply to that in the context of the DFU thread, it makes more
> sense there.
> 
> > > Any such addition should be implemented in a generic way, that is
> > > agnostic of the underlying storage device and the file system used on
> > > top of it.
> > 
> > Should, that's the proper word here. It isn't in most of FSs in uboot
> > though and I think this is better than nothing.
> 
> Wrong.  Adding special code to one file system which makes it
> "special" compared to other file systems is actually really, really
> bad.

I hope it'll not only be fat over the time.

> Best regards,
> 
> Wolfgang Denk

Best regards,
Marek Vasut

[U-Boot] [PATCH 0/2] New abstraction layer for handling MMC and FAT commands

[Lukasz Majewski]

Dear Marek Vasut,

> Dear Wolfgang Denk,
> 
> > Dear Lukasz Majewski,
> > 
> > In message <20120727123832.69195dcd@amdc308.digital.local> you
> > wrote:
> > > > Sorry if I don't understand, but what exactly is special with
> > > > MMC and FAT here?
> > > 
> > > Those patches are a follow up of a discussion about DFU support in
> > > u-boot.
> > > "[PATCH 4/7] dfu: MMC specific routines for DFU operation"
> > 
> > OK, I see.   Guess I will have to start reading these patches then
> > (which I didn't so far).
> > 
> > > In short - during this discussion it has been suggested, that
> > > sprintf() + run_command() call shall be replaced with a "_safe"
> > > function call, which decouples "real" e.g. MMC write from parsing
> > > user data from prompt.
> > 
> > ...
> > 
> > > I need the FAT and MMC for DFU patches, on which I work now. Those
> > > patches (actually v2) have already been posted to ML and are under
> > > review.
> > 
> > This makes no sense to me.  MMC is just one of the storagte devices
> > we support, and especially with the upcoming support of a clean
> > device model it makes no sense to handle it special in any way.
> 
> Sure, but that might still be a release or so away. So let's not
> hinder people from doing so, rather let's coordinate. Also, I
> consider there patches helpful, as we can use that abstraction during
> the DM work.
> 
> > The same holds for FAT - it is just one out of a number of file
> > systems we support, and it makes no sense to add any code to FAT
> > support that makes it incompatible with other file systems.
> 
> It's not incompatible, how?
> 
> > Any such addition should be implemented in a generic way, that is
> > agnostic of the underlying storage device and the file system used
> > on top of it.
> 
> Should, that's the proper word here. It isn't in most of FSs in uboot
> though and I think this is better than nothing.
> 
> > It is OK if you then test with one combination only, but the
> > implementation shall be generic.
> 
> I still consider it better than nothing -- if we have to pick this up
> and tear it apart during the DM, it's also OK, because we will easily
> figure out what parts to put where. So I'm quite fine with these
> patches.

Nice to hear. 
Anyway I think, that we shall wait for Wolfgang's opinion.


-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group