PATCH] Add 8641reset commmand for mpc8641hpcn board
From: Haiying Wang <[email protected]>
Date: Mon, 22 Jan 2007 12:37:30 -0600
Subject: [PATCH] Add 8641reset commmand for mpc8641hpcn board Also add brief introduction in README.mpc8641hpcn
---
Wolfgang,
Lo, these many moons ago, there was a bad patch to the 8641 tree that modified the reset command in an unaccepted way. Here is a rewrite of a new reset command that has the extended functionality needed for the 8641 HPCN board. It only messes with board-specific files in this form.
Does this look better/acceptable to you this way?
Thanks,
jdl
board/mpc8641hpcn/mpc8641hpcn.c | 44 +++++++++++++++++++++++--------------- cpu/mpc86xx/cpu.c | 8 +------ doc/README.mpc8641hpcn | 19 ++++++++++++++++ 3 files changed, 47 insertions(+), 24 deletions(-)
board/mpc8641hpcn/mpc8641hpcn.c +27 -17
@@ -282,11 +282,14 @@ mpc8641_reset_board(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])/* not reached */ } - cmd = argv[1 ][1 ]; + cmd = argv[1 ][0 ];switch (cmd) { -case 'f' :/* reset with frequency changed */ -if (argc <5 ) - goto my_usage; +case 'c' :/* reset with frequency changed */ +if (argc <5 ) { +puts ("\nUsage: 8641reset cf <SYSCLK freq> <COREPLL ratio> <MPXPLLratio>\n" ); +break ; + } +read_from_px_regs (0 ); val =set_px_sysclk (simple_strtoul (argv[2 ],NULL ,10 ));
@@ -304,8 +307,8 @@ mpc8641_reset_board(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])while (1 ) ;/* Not reached */ -case 'l' : -if (argv[2 ][1 ] =='f' ) { +case 'a' : +if (argv[2 ][0 ] =='c' ) {read_from_px_regs (0 );read_from_px_regs_altbank (0 );/* reset with frequency changed */
@@ -327,7 +330,7 @@ mpc8641_reset_board(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])while (1 ) ;/* Not reached */ - }else if (argv[2 ][1 ] =='d' ) { + }else if (argv[2 ][0 ] =='w' ) {/* * Reset from alternate bank without changing * frequencies but with watchdog timer enabled.
@@ -348,27 +351,34 @@ mpc8641_reset_board(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])*/ read_from_px_regs (0 );read_from_px_regs_altbank (0 ); -if (argc >2 ) - goto my_usage; +if (argc >2 ) { +puts (" \nUsage: 8641reset altbank\n" ); +break ; + }puts ("Setting registers VCFGNE1, VBOOT, and VCTL\n" );set_altbank ();read_from_px_regs_altbank (1 );puts ("Resetting board to boot from the other bank....\n" );set_px_go (); } - +case 'h' :/* help */ default : - goto my_usage; +my_usage: +puts ("\nUsage: 8641reset cf <SYSCLK freq> <COREPLL ratio> <MPXPLL ratio>\n" ); +puts (" 8641reset altbank [cf <SYSCLK freq> <COREPLL ratio> <MPXPLL ratio>]\n" ); +puts (" 8641reset altbank [wd]\n" ); +puts ("For example: 8641reset cf 40 2.5 10\n" ); +puts ("See MPC8641HPCN Design Workbook for valid values of command line parameters.\n" ); +break ; } -my_usage: -puts ("\nUsage: reset cf <SYSCLK freq> <COREPLL ratio> <MPXPLL ratio>\n" ); -puts (" reset altbank [cf <SYSCLK freq> <COREPLL ratio> <MPXPLL ratio>]\n" ); -puts (" reset altbank [wd]\n" ); -puts ("For example: reset cf 40 2.5 10\n" ); -puts ("See MPC8641HPCN Design Workbook for valid values of command line parameters.\n" ); } +U_BOOT_CMD ( + 8641reset, CFG_MAXARGS, 1, mpc8641_reset_board, +"8641reset - Reset the CPU to current bank or alternative bank\n" , +NULL +);/* * get_board_sys_clk
cpu/mpc86xx/cpu.c +1 -7
@@ -32,12 +32,6 @@#include <ft_build.h> #endif -#ifdef CONFIG_MPC8641HPCN -extern void mpc8641_reset_board (cmd_tbl_t *cmdtp,int flag, -int argc,char *argv[]); -#endif - -int checkcpu (void ) {
@@ -185,7 +179,7 @@ do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])#else /* CONFIG_MPC8641HPCN */ -mpc8641_reset_board (cmdtp, flag, argc, argv); +out8 (PIXIS_BASE +PIXIS_RST ,0 );#endif /* !CONFIG_MPC8641HPCN */
doc/README.mpc8641hpcn +19 -0
@@ -121,3 +121,22 @@ To Flash U-boot into the alternative bank (0xFF800000 - 0xFFBFFFFF): 0xe300_0000 0xe3ff_ffff PCI2/PEX2 IO 16M 0xfe00_0000 0xfeff_ffff Flash(alternate)16M 0xff00_0000 0xffff_ffff Flash(boot bank)16M + +5. 8641reset command +-------------------- +A new command "8641reset" is introduced for reset mpc8641hpcn board to +current bank or alternative bank, with/without wathdog timer enabled, +with/without frequency changed. There are some example usage: + /* reset to current bank, like "reset" command */ + 8641reset + + /* reset board to alternative bank */ + 8641reset altbank + + /* reset board to alternative bank with watchdog timer enabled*/ + 8641reset altbank wd + + /* reset board to alternative bank with frequency changed. + * 40 is SYSCLK, 2.5 is COREPLL ratio, 10 is MPXPLL ratio + */ + 8641reset altbank cf 40 2.5 10
Signature · 1 line
1.5.0.3
Dear Jon,
Quoted 2 lines · in message <1174588314.32390.41.camel@ld0161-tx32> you wrote:
Does this look better/acceptable to you this way?
The implementation still lacks a lot.
board/mpc8641hpcn/mpc8641hpcn.c mpc8641_reset_board(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) · line 285
Quoted 2 lines
board/mpc8641hpcn/mpc8641hpcn.c +1
-1- cmd = argv[1 ][1 ]; + cmd = argv[1 ][0 ];
Instead of switching on single characters, ignoring all the rest of the strings, please use a string compare like done everywhere else in U-Boot.
board/mpc8641hpcn/mpc8641hpcn.c mpc8641_reset_board(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) · line 289
Quoted 2 lines
board/mpc8641hpcn/mpc8641hpcn.c +2
-0+if (argc <5 ) { +puts ("\nUsage: 8641reset cf <SYSCLK freq> <COREPLL ratio> <MPXPLLratio>\n" );
How many different usage messages are there for this command? Please be consistent, and implement this just once.
board/mpc8641hpcn/mpc8641hpcn.c mpc8641_reset_board(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) · line 364
Quoted 1 line
board/mpc8641hpcn/mpc8641hpcn.c +1
-0+case 'h' :/* help */
Note the 'h' case.
board/mpc8641hpcn/mpc8641hpcn.c mpc8641_reset_board(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) · line 369
Quoted 4 lines
board/mpc8641hpcn/mpc8641hpcn.c +4
-0+my_usage: +puts ("\nUsage: 8641reset cf <SYSCLK freq> <COREPLL ratio> <MPXPLL ratio>\n" ); +puts (" 8641reset altbank [cf <SYSCLK freq> <COREPLL ratio> <MPXPLL ratio>]\n" ); +puts (" 8641reset altbank [wd]\n" );
The 'h' cxase is not documented.
board/mpc8641hpcn/mpc8641hpcn.c mpc8641_reset_board(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) · line 371
Quoted 2 lines
board/mpc8641hpcn/mpc8641hpcn.c +2
-0+puts ("For example: 8641reset cf 40 2.5 10\n" ); +puts ("See MPC8641HPCN Design Workbook for valid values of command line parameters.\n" );
Please don't provide examples in the usage message. These should go into the documentation, not the code.
board/mpc8641hpcn/mpc8641hpcn.c mpc8641_reset_board(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) · line 381
Quoted 5 lines
board/mpc8641hpcn/mpc8641hpcn.c +5
-0+U_BOOT_CMD ( + 8641reset, CFG_MAXARGS, 1, mpc8641_reset_board, +"8641reset - Reset the CPU to current bank or alternative bank\n" , +NULL +);
Now *this* is the real usage message. Please use this, and drop the "my_usage" code.
Also, I object against the command name. It should follow standard name rules (enven though these are not strictly enforced by the current command line parser), i. e. only consist of alphanumeric characters and underscores, and begin with an underscore or an alphabetic character (and I hereby discourage to use an underscore as the first character).
cpu/mpc86xx/cpu.c do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) · line 182
Quoted 2 lines
cpu/mpc86xx/cpu.c +1
-1-mpc8641_reset_board (cmdtp, flag, argc, argv); +out8 (PIXIS_BASE +PIXIS_RST ,0 );
Some comment seems like a good idea to me...
doc/README.mpc8641hpcn To Flash U-boot into the alternative bank (0xFF800000 - 0xFFBFFFFF): · line 128
Quoted 4 lines
+5. 8641reset command
+--------------------doc/README.mpc8641hpcn +2
-0+A new command "8641reset" is introduced for reset mpc8641hpcn board to +current bank or alternative bank, with/without wathdog timer enabled,
Typo: s/th/tch/
Best regards,
Wolfgang Denk
Signature · 4 lines
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de "I like your game but we have to change the rules."
Quoted 7 lines · Wolfgang Denk
Dear Jon,
Quoted 2 lines · in message <1174588314.32390.41.camel@ld0161-tx32> you wrote:
Does this look better/acceptable to you this way?
The implementation still lacks a lot.
But the overall approach? Is it better than the
previous version that hit common code?
Quoted 6 lines
board/mpc8641hpcn/mpc8641hpcn.c mpc8641_reset_board(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) · line 285
Quoted 2 lines
board/mpc8641hpcn/mpc8641hpcn.c +1
-1- cmd = argv[1 ][1 ]; + cmd = argv[1 ][0 ];Instead of switching on single characters, ignoring all the rest of the strings, please use a string compare like done everywhere else in U-Boot.
Will do.
Quoted 2 lines
How many different usage messages are there for this command? Please be consistent, and implement this just once.
[ ... ]
Quoted 6 lines
board/mpc8641hpcn/mpc8641hpcn.c mpc8641_reset_board(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) · line 371
Quoted 2 lines
board/mpc8641hpcn/mpc8641hpcn.c +2
-0+puts ("For example: 8641reset cf 40 2.5 10\n" ); +puts ("See MPC8641HPCN Design Workbook for valid values of command line parameters.\n" );Please don't provide examples in the usage message. These should go into the documentation, not the code.
Quoted 8 lines
board/mpc8641hpcn/mpc8641hpcn.c mpc8641_reset_board(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) · line 381
Quoted 5 lines
board/mpc8641hpcn/mpc8641hpcn.c +5
-0+U_BOOT_CMD ( + 8641reset, CFG_MAXARGS, 1, mpc8641_reset_board, +"8641reset - Reset the CPU to current bank or alternative bank\n" , +NULL +);Now *this* is the real usage message. Please use this, and drop the "
my_usage" code.
Got it. Will do.
Quoted 6 lines
Also, I object against the command name. It should follow standard name rules (enven though these are not strictly enforced by the current command line parser), i. e. only consist of alphanumeric characters and underscores, and begin with an underscore or an alphabetic character (and I hereby discourage to use an underscore as the first character).
OK. Will "reset8641" be acceptable?
Quoted 5 lines
cpu/mpc86xx/cpu.c do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) · line 182
Quoted 3 lines
cpu/mpc86xx/cpu.c +1
-1-mpc8641_reset_board (cmdtp, flag, argc, argv); +out8 (PIXIS_BASE +PIXIS_RST ,0 );Some comment seems like a good idea to me...
Hmmm. ok.
Quoted 6 lines
doc/README.mpc8641hpcn To Flash U-boot into the alternative bank (0xFF800000 - 0xFFBFFFFF): · line 128
Quoted 4 lines
+5. 8641reset command
+--------------------doc/README.mpc8641hpcn +2
-0+A new command "8641reset" is introduced for reset mpc8641hpcn board to +current bank or alternative bank, with/without wathdog timer enabled,Typo: s/th/tch/
Right.
Quoted 3 lines
Best regards,
Wolfgang Denk
Thanks for your review!
I'll respin the patch an check again before asking for a pull!
jdl
Dear Jon,
Quoted 3 lines · in message <1174661567.32390.63.camel@ld0161-tx32> you wrote:
But the overall approach? Is it better than the
previous version that hit common code?
Anything that stays local is better ... at least a bit.
Quoted 8 lines
Quoted 6 lines
Also, I object against the command name. It should follow standard name rules (enven though these are not strictly enforced by the current command line parser), i. e. only consist of alphanumeric characters and underscores, and begin with an underscore or an alphabetic character (and I hereby discourage to use an underscore as the first character).
OK. Will "reset8641" be acceptable?
Yes, as far as the name goes.
Conceptually I disagree. This command implements two different
functions: (1) it adjusts some system parameters (but this change will only take affect after a reboot), and (2) it performs a reboot. Following good old Unix design, you should split this command into it's logical parts. Then you will probably realize that the second part (perform a reset) already exists and can be reused instead of reimplemented.
Best regards,
Wolfgang Denk
Signature · 4 lines
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de I am more bored than you could ever possibly be. Go back to work.
Quoted 7 lines · Wolfgang Denk
Conceptually I disagree. This command implements two different
functions: (1) it adjusts some system parameters (but this change will only take affect after a reboot), and (2) it performs a reboot. Following good old Unix design, you should split this command into it's logical parts. Then you will probably realize that the second part (perform a reset) already exists and can be reused instead of reimplemented.
Wolfgang,
I went a little Search-And-Annoy mission here recently, and tracked down the board designer for the 8641 HPCN (and the MPC8544DS), and talked about the FPGA (called "PIXIS") implementation.
It turns out that the normal SW controlled board reset operation is distinctly different than the reset operation initiated through the FPGA (the "GO" bit). Specifically, the FPGA 'GO' bit drives a reset sequencer that allows for the additional state that has been programmed through PIXIS for the alternate speeds, flash bank selections, etc. That state is not used through the normal reset mechanism.
Effectively, there really are two reset operations, one which is explicitly tied to the FPGA reset operation as currently implemented in the patch as submitted.
Naturally, I am happy to clean up the other issues you pointed out in your review of the patch and resubmit it.
Thanks,
jdl
Dear Jon,
Quoted 4 lines · in message <1175186136.32390.284.camel@ld0161-tx32> you wrote:
Effectively, there really are two reset operations, one which is explicitly tied to the FPGA reset operation as currently implemented in the patch as submitted.
I see. OK, then your second command makes kind of sense.
Quoted 2 lines
Naturally, I am happy to clean up the other issues you pointed out in your review of the patch and resubmit it.
Please do, thanks.
Best regards,
Wolfgang Denk
Signature · 6 lines
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de After a time, you may find that "having" is not so pleasing a thing, after all, as "wanting." It is not logical, but it is often true. -- Spock, "Amok Time", stardate 3372.7