diff mbox

[U-Boot] tools: fw_printenv supports mmc device

Message ID 1292462546-32516-1-git-send-email-dh09.lee@samsung.com
State Changes Requested
Headers show

Commit Message

Donghwa Lee Dec. 16, 2010, 1:22 a.m. UTC 
I modified fw_printenv tools to use /dev/mmcblk0 node. Original fw_printenv tool
can be access MTD devices, but, in some cases, environment variables can be stored
other memory devices, for example, mmc devices.
So, I modified a few code to use /dev/mmcblk0.

Signed-off-by: Donghwa Lee <dh09.lee@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

---
 tools/env/fw_env.c      |   13 ++++++++++---
 tools/env/fw_env.config |    7 +++++--
 2 files changed, 15 insertions(+), 5 deletions(-)

Comments

Stefano Babic Dec. 16, 2010, 6:42 a.m. UTC | #1 
On 12/16/2010 02:22 AM, Donghwa Lee wrote:
> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
> index 8ff7052..5a707f6 100644
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -855,6 +855,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
>  		}
>  
>  		erase.start = blockstart;
> +#ifndef CONFIG_ENV_IS_IN_MMC

Can we get rid of this #ifdef statement ? This does not allow to include
the binary in a distro, because it is decided at compile time where the
environment is stored.

We already check if we are using a flash device in fw_env.c, when we
call ioctl (fd, MEMGETINFO, &mtdinfo). At the moment, we set the
mtd_type field in the envdevices structure only with MTD_NORFLASH or
MTD_NANDFLASH. I think it should be better to improve the check to find
on which medium we store the environment and set the mtd_type according
to its result if a SD/MMC is found (with MTD_MMC, maybe ?).

>  # Notice, that the "Number of sectors" is ignored on NOR.
>  
>  # MTD device name	Device offset	Env. size	Flash sector size	Number of sectors
> -/dev/mtd1		0x0000		0x4000		0x4000
> -/dev/mtd2		0x0000		0x4000		0x4000
> +#/dev/mtd1		0x0000		0x4000		0x4000
> +#/dev/mtd2		0x0000		0x4000		0x4000

I do not see any change in these lines..

>  
>  # NAND example
>  #/dev/mtd0		0x4000		0x4000		0x20000			2
> +
> +# MMC device name	Device offset	Env. size	Flash sector size	Number of sectors
> +/dev/mmcblk0		0x7000		0x1000		0x1000

Do we need Flash sector size for MMC ?

Best regards,
Stefano Babic
Steve Sakoman Dec. 16, 2010, 6:45 p.m. UTC | #2 
On Wed, Dec 15, 2010 at 5:22 PM, Donghwa Lee <dh09.lee@samsung.com> wrote:
> I modified fw_printenv tools to use /dev/mmcblk0 node. Original fw_printenv tool
> can be access MTD devices, but, in some cases, environment variables can be stored
> other memory devices, for example, mmc devices.
> So, I modified a few code to use /dev/mmcblk0.

I submitted a similar patch series from Loïc Minier a couple of weeks ago:

http://old.nabble.com/-U-Boot---RFC-0-3--Enhance-env-tools-td30373328.html

I've been working a a v2 to address the comments received.

Do you think a variant of that patch series will work for you?

Perhaps we could combine efforts for a patch series to solve the mmc
and normal file write targets?

Steve
Donghwa Lee Dec. 17, 2010, 2:34 a.m. UTC | #3 
On 2010-12-16 오후 3:42 , Stefano Babic wrote:
> On 12/16/2010 02:22 AM, Donghwa Lee wrote:
>> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
>> index 8ff7052..5a707f6 100644
>> --- a/tools/env/fw_env.c
>> +++ b/tools/env/fw_env.c
>> @@ -855,6 +855,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
>>  		}
>>  
>>  		erase.start = blockstart;
>> +#ifndef CONFIG_ENV_IS_IN_MMC
> Can we get rid of this #ifdef statement ? This does not allow to include
> the binary in a distro, because it is decided at compile time where the
> environment is stored.
>
> We already check if we are using a flash device in fw_env.c, when we
> call ioctl (fd, MEMGETINFO, &mtdinfo). At the moment, we set the
> mtd_type field in the envdevices structure only with MTD_NORFLASH or
> MTD_NANDFLASH. I think it should be better to improve the check to find
> on which medium we store the environment and set the mtd_type according
> to its result if a SD/MMC is found (with MTD_MMC, maybe ?).
>
I think SD/MMC may not use MEMGETINFO ioctl. If mmc device doesn't use MTD, use only one partition, it can not use MEMGETINFO ioctl?
Actually, environment variables are very small area in SD/MMC, it doesn't need to divide own partition.

But, I think my patch that uses #ifndef statement was wrong, it needs to change another way.
>>  # Notice, that the "Number of sectors" is ignored on NOR.
>>  
>>  # MTD device name	Device offset	Env. size	Flash sector size	Number of sectors
>> -/dev/mtd1		0x0000		0x4000		0x4000
>> -/dev/mtd2		0x0000		0x4000		0x4000
>> +#/dev/mtd1		0x0000		0x4000		0x4000
>> +#/dev/mtd2		0x0000		0x4000		0x4000
> I do not see any change in these lines..
>
yes, there is no change, i added avobe to tell that case of not using /dev/mtd# node.

>>  
>>  # NAND example
>>  #/dev/mtd0		0x4000		0x4000		0x20000			2
>> +
>> +# MMC device name	Device offset	Env. size	Flash sector size	Number of sectors
>> +/dev/mmcblk0		0x7000		0x1000		0x1000
> Do we need Flash sector size for MMC ?
MMC doesn't need to flash sector size. MMC can overwrite.
> Best regards,
> Stefano Babic
>
Donghwa Lee Dec. 17, 2010, 2:58 a.m. UTC | #4 
On 2010-12-17 오전 3:45 , Steve Sakoman wrote:
> On Wed, Dec 15, 2010 at 5:22 PM, Donghwa Lee <dh09.lee@samsung.com> wrote:
>> I modified fw_printenv tools to use /dev/mmcblk0 node. Original fw_printenv tool
>> can be access MTD devices, but, in some cases, environment variables can be stored
>> other memory devices, for example, mmc devices.
>> So, I modified a few code to use /dev/mmcblk0.
> I submitted a similar patch series from Loïc Minier a couple of weeks ago:
>
> http://old.nabble.com/-U-Boot---RFC-0-3--Enhance-env-tools-td30373328.html
>
> I've been working a a v2 to address the comments received.
>
> Do you think a variant of that patch series will work for you?
>
> Perhaps we could combine efforts for a patch series to solve the mmc
> and normal file write targets?
>
> Steve
>

I think your ideas are quite similar to me. I tested your patch to my target.

In my kernel your patch can not recognize file type, but, when 'is_mtd' is 0, it work well.


Best regards,
Donghwa lee
diff mbox

Patch

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 8ff7052..5a707f6 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -855,6 +855,7 @@  static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 		}
 
 		erase.start = blockstart;
+#ifndef CONFIG_ENV_IS_IN_MMC
 		ioctl (fd, MEMUNLOCK, &erase);
 
 		if (ioctl (fd, MEMERASE, &erase) != 0) {
@@ -863,7 +864,7 @@  static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 				 strerror (errno));
 			return -1;
 		}
-
+#endif
 		if (lseek (fd, blockstart, SEEK_SET) == -1) {
 			fprintf (stderr,
 				 "Seek error on %s: %s\n",
@@ -880,8 +881,9 @@  static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 			return -1;
 		}
 
+#ifndef CONFIG_ENV_IS_IN_MMC
 		ioctl (fd, MEMLOCK, &erase);
-
+#endif
 		processed  += blocklen;
 		block_seek = 0;
 		blockstart += blocklen;
@@ -964,9 +966,10 @@  static int flash_write (int fd_current, int fd_target, int dev_target)
 
 static int flash_read (int fd)
 {
-	struct mtd_info_user mtdinfo;
 	int rc;
 
+#ifndef CONFIG_ENV_IS_IN_MMC
+	struct mtd_info_user mtdinfo;
 	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
 	if (rc < 0) {
 		perror ("Cannot get MTD information");
@@ -982,6 +985,10 @@  static int flash_read (int fd)
 
 	rc = flash_read_buf (dev_current, fd, environment.image, CONFIG_ENV_SIZE,
 			     DEVOFFSET (dev_current), mtdinfo.type);
+#else
+	rc = flash_read_buf (dev_current, fd, environment.image, CONFIG_ENV_SIZE,
+			     DEVOFFSET (dev_current), 0);
+#endif
 
 	return (rc != CONFIG_ENV_SIZE) ? -1 : 0;
 }
diff --git a/tools/env/fw_env.config b/tools/env/fw_env.config
index c8f12cf..228ded8 100644
--- a/tools/env/fw_env.config
+++ b/tools/env/fw_env.config
@@ -4,8 +4,11 @@ 
 # Notice, that the "Number of sectors" is ignored on NOR.
 
 # MTD device name	Device offset	Env. size	Flash sector size	Number of sectors
-/dev/mtd1		0x0000		0x4000		0x4000
-/dev/mtd2		0x0000		0x4000		0x4000
+#/dev/mtd1		0x0000		0x4000		0x4000
+#/dev/mtd2		0x0000		0x4000		0x4000
 
 # NAND example
 #/dev/mtd0		0x4000		0x4000		0x20000			2
+
+# MMC device name	Device offset	Env. size	Flash sector size	Number of sectors
+/dev/mmcblk0		0x7000		0x1000		0x1000