Patchwork [U-Boot,17/30] cramfs: make cramfs usable without a NOR flash

login
register
mail settings
Submitter Valentin Longchamp
Date April 8, 2011, 2:24 p.m.
Message ID <e0cad960c27371170bf2d2d4be3362d6665fbbfa.1302272395.git.valentin.longchamp@keymile.com>
Download mbox | patch
Permalink /patch/90367/
State Changes Requested
Headers show

Comments

Valentin Longchamp - April 8, 2011, 2:24 p.m.
From: Heiko Schocher <hs@denx.de>

Signed-off-by: Heiko Schocher <hs@denx.de>
cc: Wolfgang Denk <wd@denx.de>
cc: Detlev Zundel <dzu@denx.de>
cc: Valentin Longchamp <valentin.longchamp@keymile.com>
cc: Holger Brunck <holger.brunck@keymile.com>
Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>
---
 common/cmd_cramfs.c |   12 +++++++++++-
 fs/cramfs/cramfs.c  |    4 ++++
 2 files changed, 15 insertions(+), 1 deletions(-)
Wolfgang Denk - April 30, 2011, 8:20 a.m.
Dear Valentin Longchamp,

In message <e0cad960c27371170bf2d2d4be3362d6665fbbfa.1302272395.git.valentin.longchamp@keymile.com> you wrote:
> From: Heiko Schocher <hs@denx.de>
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> cc: Wolfgang Denk <wd@denx.de>
> cc: Detlev Zundel <dzu@denx.de>
> cc: Valentin Longchamp <valentin.longchamp@keymile.com>
> cc: Holger Brunck <holger.brunck@keymile.com>
> Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>
> ---
>  common/cmd_cramfs.c |   12 +++++++++++-
>  fs/cramfs/cramfs.c  |    4 ++++
>  2 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/common/cmd_cramfs.c b/common/cmd_cramfs.c
> index 8c86dc5..5e1487f 100644
> --- a/common/cmd_cramfs.c
> +++ b/common/cmd_cramfs.c
> @@ -43,7 +43,9 @@
>  #endif
>  
>  #ifdef CONFIG_CRAMFS_CMDLINE
> -flash_info_t flash_info[1];
> +#if !defined(CONFIG_SYS_NO_FLASH)
> +#include <flash.h>
> +#endif

Do we need the #ifndef here?  I don;t thik it hurts if we
unconditionally #include <flash.h> ?

But note: there was no "extern" in this declaration of flash_info[],
i. e. we _did_ allocate storage here. Is the new code really
equivalent? How extensively has it been tested?


>  #ifndef CONFIG_CMD_JFFS2
>  #include <linux/stat.h>
> @@ -119,7 +121,11 @@ int do_cramfs_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	dev.id = &id;
>  	part.dev = &dev;
>  	/* fake the address offset */
> +#if !defined(CONFIG_SYS_NO_FLASH)
>  	part.offset = addr - flash_info[id.num].start[0];
> +#else
> +	part.offset = addr;
> +#endif

Sequences like this repeat a number of times. How about 

#ifdef CONFIG_SYS_NO_FLASH
# define OFFSET_ADJUSTMENT(x)	0
#else
# define OFFSET_ADJUSTMENT(x)	(flash_info[id.num].start[x])
#endif
...
	dev.id = &id;
	part.dev = &dev;
	/* fake the address offset */
	part.offset = addr - OFFSET_ADJUSTMENT(0);

> +#if !defined(CONFIG_SYS_NO_FLASH)
>  	part.offset = addr - flash_info[id.num].start[0];
> +#else
> +	part.offset = addr;
> +#endif

	part.offset = addr - OFFSET_ADJUSTMENT(0);

>  extern flash_info_t flash_info[];
>  #define PART_OFFSET(x)	(x->offset + flash_info[x->dev->id->num].start[0])
> +#else
> +#define PART_OFFSET(x)	(x->offset)

#define PART_OFFSET(x)	(x->offset + OFFSET_ADJUSTMENT(0))


[If we always refer to start[0] only, we can even omit the argument.]

Best regards,

Wolfgang Denk
Heiko Schocher - May 3, 2011, 5:49 a.m.
Hello Wolfgang,

Wolfgang Denk wrote:
> Dear Valentin Longchamp,
> 
> In message <e0cad960c27371170bf2d2d4be3362d6665fbbfa.1302272395.git.valentin.longchamp@keymile.com> you wrote:
>> From: Heiko Schocher <hs@denx.de>
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> cc: Wolfgang Denk <wd@denx.de>
>> cc: Detlev Zundel <dzu@denx.de>
>> cc: Valentin Longchamp <valentin.longchamp@keymile.com>
>> cc: Holger Brunck <holger.brunck@keymile.com>
>> Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>
>> ---
>>  common/cmd_cramfs.c |   12 +++++++++++-
>>  fs/cramfs/cramfs.c  |    4 ++++
>>  2 files changed, 15 insertions(+), 1 deletions(-)
>>
>> diff --git a/common/cmd_cramfs.c b/common/cmd_cramfs.c
>> index 8c86dc5..5e1487f 100644
>> --- a/common/cmd_cramfs.c
>> +++ b/common/cmd_cramfs.c
>> @@ -43,7 +43,9 @@
>>  #endif
>>  
>>  #ifdef CONFIG_CRAMFS_CMDLINE
>> -flash_info_t flash_info[1];
>> +#if !defined(CONFIG_SYS_NO_FLASH)
>> +#include <flash.h>
>> +#endif
> 
> Do we need the #ifndef here?  I don;t thik it hurts if we
> unconditionally #include <flash.h> ?

Yep, you are right.

> But note: there was no "extern" in this declaration of flash_info[],
> i. e. we _did_ allocate storage here. Is the new code really
> equivalent? How extensively has it been tested?

flash_info is defined in the flash driver, so this is OK.
It is tested on the keymile boards, and a MAKEALL runs
clean.

>>  #ifndef CONFIG_CMD_JFFS2
>>  #include <linux/stat.h>
>> @@ -119,7 +121,11 @@ int do_cramfs_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>  	dev.id = &id;
>>  	part.dev = &dev;
>>  	/* fake the address offset */
>> +#if !defined(CONFIG_SYS_NO_FLASH)
>>  	part.offset = addr - flash_info[id.num].start[0];
>> +#else
>> +	part.offset = addr;
>> +#endif
> 
> Sequences like this repeat a number of times. How about 
> 
> #ifdef CONFIG_SYS_NO_FLASH
> # define OFFSET_ADJUSTMENT(x)	0
> #else
> # define OFFSET_ADJUSTMENT(x)	(flash_info[id.num].start[x])
> #endif
> ...
> 	dev.id = &id;
> 	part.dev = &dev;
> 	/* fake the address offset */
> 	part.offset = addr - OFFSET_ADJUSTMENT(0);

Yep, that looks better, I change this.

>> +#if !defined(CONFIG_SYS_NO_FLASH)
>>  	part.offset = addr - flash_info[id.num].start[0];
>> +#else
>> +	part.offset = addr;
>> +#endif
> 
> 	part.offset = addr - OFFSET_ADJUSTMENT(0);
> 
>>  extern flash_info_t flash_info[];
>>  #define PART_OFFSET(x)	(x->offset + flash_info[x->dev->id->num].start[0])
>> +#else
>> +#define PART_OFFSET(x)	(x->offset)
> 
> #define PART_OFFSET(x)	(x->offset + OFFSET_ADJUSTMENT(0))
> 
> 
> [If we always refer to start[0] only, we can even omit the argument.]

Yep.

Wolfgang? Is it OK, to send a v2 which is not in this patchseries,
as I think this is an independent patch?

bye,
Heiko
Wolfgang Denk - May 3, 2011, 11:48 a.m.
Dear Heiko Schocher,

In message <4DBF9760.5030607@denx.de> you wrote:
>
> >> --- a/common/cmd_cramfs.c
> >> +++ b/common/cmd_cramfs.c
> >> @@ -43,7 +43,9 @@
> >>  #endif
> >>  
> >>  #ifdef CONFIG_CRAMFS_CMDLINE
> >> -flash_info_t flash_info[1];
> >> +#if !defined(CONFIG_SYS_NO_FLASH)
> >> +#include <flash.h>
> >> +#endif
> > 
> > Do we need the #ifndef here?  I don;t thik it hurts if we
> > unconditionally #include <flash.h> ?
> 
> Yep, you are right.
> 
> > But note: there was no "extern" in this declaration of flash_info[],
> > i. e. we _did_ allocate storage here. Is the new code really
> > equivalent? How extensively has it been tested?
> 
> flash_info is defined in the flash driver, so this is OK.
> It is tested on the keymile boards, and a MAKEALL runs
> clean.

You are probably right that the code with your patch is correct, i. e.
that this one-line deletion fixes an actual bug in the existing code.
But you don't mention this in the commit message.

These are two unrelated changes, that belon into separate commits.

Best regards,

Wolfgang Denk

Patch

diff --git a/common/cmd_cramfs.c b/common/cmd_cramfs.c
index 8c86dc5..5e1487f 100644
--- a/common/cmd_cramfs.c
+++ b/common/cmd_cramfs.c
@@ -43,7 +43,9 @@ 
 #endif
 
 #ifdef CONFIG_CRAMFS_CMDLINE
-flash_info_t flash_info[1];
+#if !defined(CONFIG_SYS_NO_FLASH)
+#include <flash.h>
+#endif
 
 #ifndef CONFIG_CMD_JFFS2
 #include <linux/stat.h>
@@ -119,7 +121,11 @@  int do_cramfs_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	dev.id = &id;
 	part.dev = &dev;
 	/* fake the address offset */
+#if !defined(CONFIG_SYS_NO_FLASH)
 	part.offset = addr - flash_info[id.num].start[0];
+#else
+	part.offset = addr;
+#endif
 
 	/* pre-set Boot file name */
 	if ((filename = getenv("bootfile")) == NULL) {
@@ -182,7 +188,11 @@  int do_cramfs_ls(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	dev.id = &id;
 	part.dev = &dev;
 	/* fake the address offset */
+#if !defined(CONFIG_SYS_NO_FLASH)
 	part.offset = addr - flash_info[id.num].start[0];
+#else
+	part.offset = addr;
+#endif
 
 	if (argc == 2)
 		filename = argv[1];
diff --git a/fs/cramfs/cramfs.c b/fs/cramfs/cramfs.c
index 2956d39..910955d 100644
--- a/fs/cramfs/cramfs.c
+++ b/fs/cramfs/cramfs.c
@@ -41,8 +41,12 @@  struct cramfs_super super;
 
 /* CPU address space offset calculation macro, struct part_info offset is
  * device address space offset, so we need to shift it by a device start address. */
+#if !defined(CONFIG_SYS_NO_FLASH)
 extern flash_info_t flash_info[];
 #define PART_OFFSET(x)	(x->offset + flash_info[x->dev->id->num].start[0])
+#else
+#define PART_OFFSET(x)	(x->offset)
+#endif
 
 static int cramfs_read_super (struct part_info *info)
 {