Patchwork [U-Boot,3/9] nand: add a hw specific subcommand to the nand command.

login
register
mail settings
Submitter Heiko Schocher
Date July 16, 2011, 10:06 a.m.
Message ID <1310810810-5322-4-git-send-email-hs@denx.de>
Download mbox | patch
Permalink /patch/104944/
State Changes Requested
Headers show

Comments

Heiko Schocher - July 16, 2011, 10:06 a.m.
Actually this is needed for coming up davinci dm368 cam_enc_4xx
board support. There we need two different nand read/write
functions, because the RBL uses different nand read/write
functions than u-boot.

Another place where such a command is used is:
arch/arm/cpu/armv7/omap3/board.c: do_switch_ecc()

Maybe this should be fixed?

Signed-off-by: Heiko Schocher <hs@denx.de>
cc: Scott Wood <scottwood@freescale.com>
---
 README            |    2 ++
 common/cmd_nand.c |    8 ++++++++
 include/nand.h    |    3 +++
 3 files changed, 13 insertions(+), 0 deletions(-)
Scott Wood - Aug. 2, 2011, 8:19 p.m.
Sorry for the delay, didn't see this the first time around -- just
noticed it in patchwork.

On Sat, Jul 16, 2011 at 12:06:44AM -0000, Heiko Schocher wrote:
> @@ -739,6 +743,10 @@ U_BOOT_CMD(
>  	"nand env.oob set off|partition - set enviromnent offset\n"
>  	"nand env.oob get - get environment offset"
>  #endif
> +#ifdef CONFIG_CMD_NAND_HWFUNC
> +	"\n"
> +	"nand hwfunc " CONFIG_CMD_NAND_HWFUNC "\n"
> +#endif

Why not just define hardware-specific commands at whatever scope they
makes sense (board code, controller driver, etc.), rather than add a
generic hook here?

-Scott
Heiko Schocher - Aug. 3, 2011, 5:14 a.m.
Hello Scott,

Scott Wood wrote:
> Sorry for the delay, didn't see this the first time around -- just
> noticed it in patchwork.

No problem. Thanks for the review!

> On Sat, Jul 16, 2011 at 12:06:44AM -0000, Heiko Schocher wrote:
>> @@ -739,6 +743,10 @@ U_BOOT_CMD(
>>  	"nand env.oob set off|partition - set enviromnent offset\n"
>>  	"nand env.oob get - get environment offset"
>>  #endif
>> +#ifdef CONFIG_CMD_NAND_HWFUNC
>> +	"\n"
>> +	"nand hwfunc " CONFIG_CMD_NAND_HWFUNC "\n"
>> +#endif
> 
> Why not just define hardware-specific commands at whatever scope they
> makes sense (board code, controller driver, etc.), rather than add a
> generic hook here?

Hmm... because if I define a hw specific command for example at
soc scope, it is not a nand subcommand ... and I like to have all
nand commands (I use it for switching between different read/write
functions) accessable under "nand ..." from the u-boot shell ...
I don;t like to have all over the code randomised some nand commands ...

bye,
Heiko
Scott Wood - Aug. 3, 2011, 3:56 p.m.
On Wed, 3 Aug 2011 07:14:51 +0200
Heiko Schocher <hs@denx.de> wrote:

> Hello Scott,
> 
> Scott Wood wrote:
> > Sorry for the delay, didn't see this the first time around -- just
> > noticed it in patchwork.
> 
> No problem. Thanks for the review!
> 
> > On Sat, Jul 16, 2011 at 12:06:44AM -0000, Heiko Schocher wrote:
> >> @@ -739,6 +743,10 @@ U_BOOT_CMD(
> >>  	"nand env.oob set off|partition - set enviromnent offset\n"
> >>  	"nand env.oob get - get environment offset"
> >>  #endif
> >> +#ifdef CONFIG_CMD_NAND_HWFUNC
> >> +	"\n"
> >> +	"nand hwfunc " CONFIG_CMD_NAND_HWFUNC "\n"
> >> +#endif
> > 
> > Why not just define hardware-specific commands at whatever scope they
> > makes sense (board code, controller driver, etc.), rather than add a
> > generic hook here?
> 
> Hmm... because if I define a hw specific command for example at
> soc scope, it is not a nand subcommand ... and I like to have all
> nand commands (I use it for switching between different read/write
> functions) accessable under "nand ..." from the u-boot shell ...
> I don;t like to have all over the code randomised some nand commands ...

What if there are multiple such commands?  You'd end up with "nand hwfunc
foo <args>", which is getting a bit unwieldy.  Having only one might
actually be worse: what does a plain "nand hwfunc" do?

What if there are multiple commands, but which belong in different places
(e.g. one defined by the board, one defined by the controller driver)?

-Scott
Heiko Schocher - Aug. 4, 2011, 5:40 a.m.
Hello Scott,

Scott Wood wrote:
> On Wed, 3 Aug 2011 07:14:51 +0200
> Heiko Schocher <hs@denx.de> wrote:
> 
>> Hello Scott,
>>
>> Scott Wood wrote:
>>> Sorry for the delay, didn't see this the first time around -- just
>>> noticed it in patchwork.
>> No problem. Thanks for the review!
>>
>>> On Sat, Jul 16, 2011 at 12:06:44AM -0000, Heiko Schocher wrote:
>>>> @@ -739,6 +743,10 @@ U_BOOT_CMD(
>>>>  	"nand env.oob set off|partition - set enviromnent offset\n"
>>>>  	"nand env.oob get - get environment offset"
>>>>  #endif
>>>> +#ifdef CONFIG_CMD_NAND_HWFUNC
>>>> +	"\n"
>>>> +	"nand hwfunc " CONFIG_CMD_NAND_HWFUNC "\n"
>>>> +#endif
>>> Why not just define hardware-specific commands at whatever scope they
>>> makes sense (board code, controller driver, etc.), rather than add a
>>> generic hook here?
>> Hmm... because if I define a hw specific command for example at
>> soc scope, it is not a nand subcommand ... and I like to have all
>> nand commands (I use it for switching between different read/write
>> functions) accessable under "nand ..." from the u-boot shell ...
>> I don;t like to have all over the code randomised some nand commands ...
> 
> What if there are multiple such commands?  You'd end up with "nand hwfunc

Then we would have a (as hwfunc is a nand subcommand) hwfunc
subcommand(s) ...

> foo <args>", which is getting a bit unwieldy.  Having only one might

Why? (I don;t know, if we get really more then one hwfunc...), but
if we define all over the code some (board, soc, arch,...) specific
nand commands, we lose track ... and with the "nand hwfunc" we have
one place where we have to look ...

> actually be worse: what does a plain "nand hwfunc" do?

A plain "nand hwfunc" should print the help message (if we have more
subcommands under "nand hwfunc"), if we have only one, it returns
as usual in uboot, actual state ... for example on the cam_enc_4xx board:

cam_enc_4xx> nand
nand - NAND sub-system

Usage:
nand info - show available NAND devices
nand device [dev] - show or set current device
nand read - addr off|partition size
nand write - addr off|partition size
    read/write 'size' bytes starting at offset 'off'
    to/from memory address 'addr', skipping bad blocks.
nand erase[.spread] [clean] off size - erase 'size' bytes from offset 'off'
    With '.spread', erase enough for given file size, otherwise,
    'size' includes skipped bad blocks.
nand erase.part [clean] partition - erase entire mtd partition'
nand erase.chip [clean] - erase entire chip'
nand bad - show bad blocks
nand dump[.oob] off - dump page
nand scrub off size | scrub.part partition | scrub.chip
    really clean NAND erasing bad blocks (UNSAFE)
nand markbad off [...] - mark bad block(s) at offset (UNSAFE)
nand biterr off - make a bit error at offset (UNSAFE)
nand hwfunc [rbl/uboot]
         switch between rbl/uboot nand read/write functions

cam_enc_4xx> nand hwfunc
uboot
cam_enc_4xx>

> What if there are multiple commands, but which belong in different places
> (e.g. one defined by the board, one defined by the controller driver)?

Maybe something like this?

nand hwfunc arch ...
nand hwfunc board ...
nand hwfunc ctrl ...
nand hwfunc soc ...
[...]

each hwfunc subcommand added through an define ...

bye,
Heiko
Scott Wood - Aug. 4, 2011, 7:42 p.m.
On 08/04/2011 12:40 AM, Heiko Schocher wrote:
> Scott Wood wrote:
>> What if there are multiple such commands?  You'd end up with "nand hwfunc
>
> Then we would have a (as hwfunc is a nand subcommand) hwfunc
> subcommand(s) ...

The question was rhetorical, as the next sentence indicates -- unless 
you had a different answer, of course.

>> foo<args>", which is getting a bit unwieldy.  Having only one might
>
> Why?

The "hwfunc" doesn't really tell the user anything, it's an internal 
implementation detail of where in the code the command is implemented. 
It's verbosity without benefit.

> (I don;t know, if we get really more then one hwfunc...)

You later say a plain "nand hwfunc" would print a help message, so that 
plus one real command makes two...

>, but if we define all over the code some (board, soc, arch,...) specific
> nand commands, we lose track ...
> and with the "nand hwfunc" we have one place where we have to look ...

More like one thing to grep for, and you'll still need to limit the 
files grepped so you don't get irrelevant results from other boards or 
controllers.  Why not just search those files for U_BOOT_CMD?

>> actually be worse: what does a plain "nand hwfunc" do?
>
> A plain "nand hwfunc" should print the help message (if we have more
> subcommands under "nand hwfunc"), if we have only one, it returns
> as usual in uboot, actual state ...

"actual state" of what?  We haven't gotten to the point of actually 
defining a command yet.

> nand hwfunc [rbl/uboot]
>           switch between rbl/uboot nand read/write functions
>
> cam_enc_4xx>  nand hwfunc
> uboot

Again, I think "nand hwfunc" conveys little information about what it's 
actually going to do, versus something like:

=> nandrbl
off

At least "nand hwfunc type" or "nand hwfunc rbl" would be more 
informative, but I question the benefit that the verbosity buys us.

Why do you need alternative implementations of read/write functions, BTW?

> cam_enc_4xx>
>
>> What if there are multiple commands, but which belong in different places
>> (e.g. one defined by the board, one defined by the controller driver)?
>
> Maybe something like this?
>
> nand hwfunc arch ...
> nand hwfunc board ...
> nand hwfunc ctrl ...
> nand hwfunc soc ...
> [...]
>
> each hwfunc subcommand added through an define ...

So now it's "nand hwfunc ctrl type rbl"?  Just to avoid grepping for 
U_BOOT_CMD?

Or by "one place to look" are you talking about as a user in the help 
output?  If you insist on the command appearing as a proper "nand" 
subcommand, how about dropping "hwfunc" and letting other pieces of code 
register on a chain of handlers?  Anything that isn't recognized gets 
passed on to the next link in the chain.

-Scott
Wolfgang Denk - Aug. 4, 2011, 8:14 p.m.
Dear Scott Wood,

In message <4E3AF62B.8080605@freescale.com> you wrote:
>
> Again, I think "nand hwfunc" conveys little information about what it's 
> actually going to do, versus something like:
> 
> => nandrbl
> off

I agree with Heiko that NAND related commands should be implemented as
subcommands of the "nand" command.

Havong "nand <subcommand>" and "nandrbl" at the same time is not
acceptable to me - that should be changed into "nand rbl".

> At least "nand hwfunc type" or "nand hwfunc rbl" would be more 
> informative, but I question the benefit that the verbosity buys us.

I agree that "hwfunc" is an unlucky name.

> So now it's "nand hwfunc ctrl type rbl"?  Just to avoid grepping for 
> U_BOOT_CMD?

No. I would not like this either.


Best regards,

Wolfgang Denk
Scott Wood - Aug. 4, 2011, 8:28 p.m.
On 08/04/2011 03:14 PM, Wolfgang Denk wrote:
> Dear Scott Wood,
>
> In message<4E3AF62B.8080605@freescale.com>  you wrote:
>>
>> Again, I think "nand hwfunc" conveys little information about what it's
>> actually going to do, versus something like:
>>
>> =>  nandrbl
>> off
>
> I agree with Heiko that NAND related commands should be implemented as
> subcommands of the "nand" command.
>
> Havong "nand<subcommand>" and "nandrbl" at the same time is not
> acceptable to me - that should be changed into "nand rbl".
>
>> At least "nand hwfunc type" or "nand hwfunc rbl" would be more
>> informative, but I question the benefit that the verbosity buys us.
>
> I agree that "hwfunc" is an unlucky name.
>
>> So now it's "nand hwfunc ctrl type rbl"?  Just to avoid grepping for
>> U_BOOT_CMD?
>
> No. I would not like this either.

How about some way of board/controller/etc. code plugging in commands to 
"nand" without "hwfunc"?  Could be a chained handler, or copying entries 
into a command table, or some way of generalizing the stuff in 
common/command.c to operate on multiple command lists.  Though without 
some change to how linker scripts are managed, to be practical that last 
option would need to avoid introducing a new section per subtable (maybe 
just filter out others when iterating).  Would be nice to get tab 
completion on subcommands.

-Scott
Wolfgang Denk - Aug. 4, 2011, 8:46 p.m.
Dear Scott Wood,

In message <4E3B0109.5070201@freescale.com> you wrote:
>
> How about some way of board/controller/etc. code plugging in commands to 
> "nand" without "hwfunc"?  Could be a chained handler, or copying entries 
> into a command table, or some way of generalizing the stuff in 
> common/command.c to operate on multiple command lists.  Though without 

I think a chained handler would be best.

> some change to how linker scripts are managed, to be practical that last 
> option would need to avoid introducing a new section per subtable (maybe 

I didn't have time to actually think this to an end how to implement
it etc.  Proposals / patches welcome :-)

> just filter out others when iterating).  Would be nice to get tab 
> completion on subcommands.

Indeed :-)

Best regards,

Wolfgang Denk

Patch

diff --git a/README b/README
index 1e2d4d3..647e10b 100644
--- a/README
+++ b/README
@@ -705,6 +705,8 @@  The following options need to be configured:
 		CONFIG_CMD_MII		* MII utility commands
 		CONFIG_CMD_MTDPARTS	* MTD partition support
 		CONFIG_CMD_NAND		* NAND support
+		CONFIG_CMD_NAND_HWFUNC	  add to the nand commando a hw specific
+					  subcommand.
 		CONFIG_CMD_NET		  bootp, tftpboot, rarpboot
 		CONFIG_CMD_PCA953X	* PCA953x I2C gpio commands
 		CONFIG_CMD_PCA953X_INFO	* PCA953x I2C gpio info command
diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 8c56802..416d2c7 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -689,6 +689,10 @@  int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 		return 0;
 	}
 #endif
+#ifdef CONFIG_CMD_NAND_HWFUNC
+	if (strcmp(cmd, "hwfunc") == 0)
+		return nand_hwfunc(argc - 2, argv + 2);
+#endif
 
 usage:
 	return cmd_usage(cmdtp);
@@ -739,6 +743,10 @@  U_BOOT_CMD(
 	"nand env.oob set off|partition - set enviromnent offset\n"
 	"nand env.oob get - get environment offset"
 #endif
+#ifdef CONFIG_CMD_NAND_HWFUNC
+	"\n"
+	"nand hwfunc " CONFIG_CMD_NAND_HWFUNC "\n"
+#endif
 );
 
 static int nand_load_image(cmd_tbl_t *cmdtp, nand_info_t *nand,
diff --git a/include/nand.h b/include/nand.h
index 8d94b5c..06b86f8 100644
--- a/include/nand.h
+++ b/include/nand.h
@@ -138,6 +138,9 @@  void board_nand_select_device(struct nand_chip *nand, int chip);
 
 __attribute__((noreturn)) void nand_boot(void);
 
+#ifdef CONFIG_CMD_NAND_HWFUNC
+int nand_hwfunc(int argc, char * const argv[]);
+#endif
 #endif
 
 #ifdef CONFIG_ENV_OFFSET_OOB