| Message ID | 1310810810-5322-4-git-send-email-hs@denx.de |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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
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
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
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
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
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
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
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(-)