mpc512x: adjust and improve AC14xx board support
PATCH 1/1 mpc512x: adjust and improve AC14xx board support
minor improvements for the 'ifm AC14xx' board setup
- adjust diagnostics (reworded, silent by default)
- re-order the list of "recovery conditions"
- update and improve comments
- adjust the board configuration
- use the builtin serial baudrate table
- use the official 'ac14xx' name everywhere (remove 'k6' remainders)
- fix a NUL termination issue in the rootpath spec
- rephrase the 'muster_nr' suffix for development network boot
- reduce the ARP timeout for faster network boot
--- board/ifm/ac14xx/ac14xx.c | 44 +++++++++++++++++++++----------------------- include/configs/ac14xx.h | 25 ++++++++++++------------- 2 files changed, 33 insertions(+), 36 deletions(-)
all of the modifications only affect the 'ifm AC14xx' board, the changes were tested and verified on the hardware, but a style related question remains:
shall I split this patch into a series of tiny patches each addressing a specific aspect of the whole set, or is the aggregation acceptable since the modifications are so small?
nothing was broken in the previous implementation given that an external environment image existed, so the patch is not a fix but just improves the existing board support -- except for the builtin 'rootpath' spec which was incorrectly terminated and shadowed the 'netdev' spec, while both variables only apply to network boot which isn't the default configuration and exclusively relates to development support
board/ifm/ac14xx/ac14xx.c +21 -23
@@ -23,6 +23,10 @@#include <i2c.h> #endif +static int eeprom_diag; +static int mac_diag; +static int gpio_diag; +DECLARE_GLOBAL_DATA_PTR ;static void gpio_configure (void )
@@ -37,7 +41,7 @@ static void gpio_configure(void)/* * out_be32(&gpioregs->gpdir, 0xC2293020); -* workaround for a hardware affect: configure direction in pieces, +* workaround for a hardware effect: configure direction in pieces, * setting all outputs at once drops the reset line too low and * makes us lose the MII connection (breaks ethernet for us) */
@@ -126,8 +130,6 @@ static u32 gpio_querykbd(void)/* excerpt from the recovery's hw_info.h */ -static int eeprom_diag =1 ; -struct __attribute__ ((__packed__ ))eeprom_layout {char magic[3 ];/** 'ifm' */ u8 len[2 ];/** content length without magic/len fields */
@@ -230,8 +232,8 @@ int mac_read_from_eeprom(void)if (mac &&is_valid_ether_addr (mac)) {eth_setenv_enetaddr ("ethaddr" , mac); -printf ("DIAG: %s() MAC value [%s]\n" , - __func__,getenv ("ethaddr" )); +if (mac_diag) +printf ("DIAG: MAC [%s]\n" ,getenv ("ethaddr" )); }return 0 ;
@@ -326,42 +328,38 @@ int misc_init_r(void)gpio_configure ();/* -* check the GPIO keyboard, -* enforced start of the recovery when +* enforce the start of the recovery system when * - the appropriate keys were pressed -* - a previous installation was aborted or has failed * - "some" external software told us to +* - a previous installation was aborted or has failed */ want_recovery =0 ; keys =gpio_querykbd (); -printf ("GPIO keyboard status [0x%08X]\n" , keys); -/* XXX insist in the _exact_ combination? */ +if (gpio_diag) +printf ("GPIO keyboard status [0x%02X]\n" , keys);if ((keys &GPIOKEY_BITS_RECOVERY ) ==GPIOKEY_BITS_RECOVERY ) { -printf ("GPIO keyboard requested RECOVERY\n" ); -/* XXX TODO -* refine the logic to detect the first keypress, and -* wait to recheck IF it was the recovery combination? -*/ +printf ("detected recovery request (keyboard)\n" ); want_recovery =1 ; } - s =getenv ("install_in_progress" ); + s =getenv ("want_recovery" );if ((s !=NULL ) && (*s !='\0' )) { -printf ("previous installation aborted, running RECOVERY\n" ); +printf ("detected recovery request (environment)\n" ); want_recovery =1 ; } - s =getenv ("install_failed" ); + s =getenv ("install_in_progress" );if ((s !=NULL ) && (*s !='\0' )) { -printf ("previous installation FAILED, running RECOVERY\n" ); +printf ("previous installation has not completed\n" ); want_recovery =1 ; } - s =getenv ("want_recovery" ); + s =getenv ("install_failed" );if ((s !=NULL ) && (*s !='\0' )) { -printf ("running RECOVERY according to the request\n" ); +printf ("previous installation has failed\n" ); want_recovery =1 ; } - -if (want_recovery) +if (want_recovery) { +printf ("enforced start of the recovery system\n" );setenv ("bootcmd" ,"run recovery" ); + }/* * boot the recovery system without waiting; boot the
include/configs/ac14xx.h +12 -13
@@ -72,7 +72,7 @@#define CONFIG_SYS_MAX_RAM_SIZE 0x20000000/* -* DDR Controller Configuration XXX TODO +* DDR Controller Configuration * * SYS_CFG: * [31:31] MDDRC Soft Reset: Diabled
@@ -265,7 +265,6 @@/* * CS related parameters -* TODO document these */ /* CS0 Flash */ #define CONFIG_SYS_CS0_CFG 0x00031110
@@ -331,8 +330,6 @@#endif #define CONFIG_BAUDRATE 115200/* ... at 115200 bps */ -#define CONFIG_SYS_BAUDRATE_TABLE \ - {300, 600, 1200, 2400, 4800, 9600, 19200, 38400, 115200}#define CONSOLE_FIFO_TX_SIZE FIFOC_PSC3_TX_SIZE#define CONSOLE_FIFO_TX_ADDR FIFOC_PSC3_TX_ADDR
@@ -506,21 +503,21 @@#define CONFIG_BOOTDELAY 2/* -1 disables auto-boot */ -/* XXX TODO need to specify the builtin environment */ +/* the builtin environment and standard greeting */ #define CONFIG_PREBOOT "echo;" \ "echo Type \\\"run flash_nfs\\\" to mount root filesystem over NFS;" \ "echo"#define CONFIG_EXTRA_ENV_SETTINGS_DEVEL \ - "muster_nr=00\0" \ + "muster_nr=-00\0" \ "fromram=run ramargs addip addtty; " \ - "tftp ${fdt_addr_r} k6m2/ac14xx.dtb-${muster_nr}; " \ - "tftp ${kernel_addr_r} k6m2/uImage-${muster_nr}; " \ - "tftp ${ramdisk_addr_r} k6m2/uFS-${muster_nr}; " \ + "tftp ${fdt_addr_r} ac14xx/ac14xx.dtb${muster_nr}; " \ + "tftp ${kernel_addr_r} ac14xx/uImage${muster_nr}; " \ + "tftp ${ramdisk_addr_r} ac14xx/uFS${muster_nr}; " \ "bootm ${kernel_addr_r} ${ramdisk_addr_r} ${fdt_addr_r}\0" \ "fromnfs=run nfsargs addip addtty; " \ - "tftp ${fdt_addr_r} k6m2/ac14xx.dtb-${muster_nr}; " \ - "tftp ${kernel_addr_r} k6m2/uImage-${muster_nr}; " \ + "tftp ${fdt_addr_r} ac14xx/ac14xx.dtb${muster_nr}; " \ + "tftp ${kernel_addr_r} ac14xx/uImage${muster_nr}; " \ "bootm ${kernel_addr_r} - ${fdt_addr_r}\0" \ "fromflash=run nfsargs addip addtty; " \ "bootm fc020000 - fc000000\0" \
@@ -548,12 +545,12 @@"u-boot=ac14xx/u-boot.bin\0" \"bootfile=ac14xx/uImage\0" \"fdtfile=ac14xx/ac14xx.dtb\0" \ -"rootpath=/opt/eldk/ppc_6xx\n" \ +"rootpath=/opt/eldk/ppc_6xx\0" \"netdev=eth0\0" \"consdev=ttyPSC0\0" \"hostname=ac14xx\0" \"nfsargs=setenv bootargs root=/dev/nfs rw " \ -"nfsroot=${serverip}:${rootpath}-${muster_nr}\0" \ +"nfsroot=${serverip}:${rootpath}${muster_nr}\0" \"ramargs=setenv bootargs root=/dev/ram rw\0" \"addip=setenv bootargs ${bootargs} " \"ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}" \
@@ -583,6 +580,8 @@#define CONFIG_BOOTCOMMAND "run production" +#define CONFIG_ARP_TIMEOUT 200UL +#define CONFIG_FIT 1#define CONFIG_OF_LIBFDT 1#define CONFIG_OF_BOARD_SETUP 1
Signature · 1 line
1.7.10.4
PATCH 1/1 mpc512x: adjust and improve AC14xx board support
Dear Gerhard,
board/ifm/ac14xx/ac14xx.c int mac_read_from_eeprom(void) · line 236
Quoted 3 lines · In message <[email protected]> you wrote:
board/ifm/ac14xx/ac14xx.c +2
-0+if (mac_diag) +printf ("DIAG: MAC [%s]\n" ,getenv ("ethaddr" ));
What happens if "ethaddr" is not defined in the environment?
...
include/configs/ac14xx.h line 548
Quoted 1 line
include/configs/ac14xx.h +1
-0+"rootpath=/opt/eldk/ppc_6xx\0" \
You are really still using ELDK 4.x ?
Best regards,
Wolfgang Denk
Signature · 7 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de Digital computers are themselves more complex than most things people build: They have very large numbers of states. This makes conceiving, describing, and testing them hard. Software systems have orders-of- magnitude more states than computers do. - Fred Brooks, Jr.
PATCH 1/1 mpc512x: adjust and improve AC14xx board support
Quoted 7 lines · Wolfgang Denk
board/ifm/ac14xx/ac14xx.c int mac_read_from_eeprom(void) · line 236
Quoted 3 lines · In message <[email protected]> you wrote:
board/ifm/ac14xx/ac14xx.c +2
-0+if (mac_diag) +printf ("DIAG: MAC [%s]\n" ,getenv ("ethaddr" ));What happens if "ethaddr" is not defined in the environment?
The context is the mac_read_from_eeprom() routine, and the getenv() call for the message is immediately preceeded by a setenv() call.
Right, the lookup may fail if the setup failed as well. That's a yet uncaught followup failure. Will include an improved check in v2 of the patch.
Quoted 4 lines
...
include/configs/ac14xx.h line 548
Quoted 1 line
include/configs/ac14xx.h +1
-0+"rootpath=/opt/eldk/ppc_6xx\0" \You are really still using ELDK 4.x ?
In this specific project? Yes! Although development setups used to run with both 4.2 and 5.3 here for some time, the evaluation hasn't comleted yet, so the final switch wasn't made so far.
I'll look into what else is involved in the ELDK-5 switch (using a mere symlink here from under /opt/eldk/ppc_6xx-sign to /opt/eldk-5.3/powerpc/rootfs-*), although /opt/eldk itself may perfectly legally be a symlink to /opt/eldk-5.3 for those setups which want to switch back and forth. Wasn't this the usual approach before ELDK-5? Somehow I'm reluctant to encode the ELDK version into the binary, hmm ...
virtually yours
Gerhard Sittig
Signature · 3 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de
PATCH 1/1 mpc512x: adjust and improve AC14xx board support
Dear Gerhard Sittig,
Quoted 9 lines · In message <[email protected]> you wrote:
Quoted 5 lines
board/ifm/ac14xx/ac14xx.c int mac_read_from_eeprom(void) · line 236
Quoted 3 lines
board/ifm/ac14xx/ac14xx.c +2
-0+if (mac_diag) +printf ("DIAG: MAC [%s]\n" ,getenv ("ethaddr" ));What happens if "ethaddr" is not defined in the environment?
The context is the
mac_read_from_eeprom()routine, and thegetenv()call for the message is immediately preceeded by asetenv()call.
...which, as everything, can fail for a number of reasons.
It is utterly wrong to rely on results of any previous actions and assume that nothing can go wrong. Murphy will teach you.
Quoted 7 lines
Quoted 3 lines
include/configs/ac14xx.h line 548
Quoted 1 line
include/configs/ac14xx.h +1
-0+"rootpath=/opt/eldk/ppc_6xx\0" \You are really still using ELDK 4.x ?
In this specific project? Yes! Although development setups used to run with both 4.2 and 5.3 here for some time, the evaluation hasn't comleted yet, so the final switch wasn't made so far.
...which makes clear that setting such variables in the default config is of questionable value.
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de By the way, ALL software projects are done by iterative prototyping. Some companies call their prototypes "releases", that's all.
PATCH 1/1 mpc512x: adjust and improve AC14xx board support
In reply to Gerhard SittigHi Gerhard,
Quoted 6 lines · Gerhard Sittig
board/ifm/ac14xx/ac14xx.c +2
-2- s =getenv ("install_in_progress" ); + s =getenv ("want_recovery" );if ((s !=NULL ) && (*s !='\0' )) { -printf ("previous installation aborted, running RECOVERY\n" ); +printf ("detected recovery request (environment)\n" ); want_recovery =1 ;
board/ifm/ac14xx/ac14xx.c int misc_init_r(void) · line 354
Quoted 10 lines
board/ifm/ac14xx/ac14xx.c +3
-3} - s =getenv ("install_failed" ); + s =getenv ("install_in_progress" );if ((s !=NULL ) && (*s !='\0' )) { -printf ("previous installation FAILED, running RECOVERY\n" ); +printf ("previous installation has not completed\n" ); want_recovery =1 ; } - s =getenv ("want_recovery" ); + s =getenv ("install_failed" );
I am asking myself if it is strictly required to have multiple variables to identify if the "recovery" script must be run or not. If a previous install was interrupted ("install_in_progress"
), or a request was initiated ("want recovery") or the last installation failed, is not so important. In all cases you set the want_recovery flag and starts the script. But using multiple variables it is not atomic, and could be, due for example to a system reset, that a variable is set without clearing the other one.
Why is not better to set a single variable, maybe with multiple values ? Value could be a simple integer (0=no recovery, 1=install_in_progress,,..) or still a string, if you prefer this solution.
include/configs/ac14xx.h line 548
Quoted 2 lines
include/configs/ac14xx.h +1
-1-"rootpath=/opt/eldk/ppc_6xx\n" \ +"rootpath=/opt/eldk/ppc_6xx\0" \
We do not set IP addresses or fix rootpath. Someone could have installed the rootfs on NFS on a different path. Also, IMHO rootpath should be simply removed.
Best regards,
Stefano
Signature · 5 lines
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de =====================================================================
PATCH 1/1 mpc512x: adjust and improve AC14xx board support
Quoted 29 lines · Stefano Babic
Quoted 6 lines · Gerhard Sittig
board/ifm/ac14xx/ac14xx.c +2
-2- s =getenv ("install_in_progress" ); + s =getenv ("want_recovery" );if ((s !=NULL ) && (*s !='\0' )) { -printf ("previous installation aborted, running RECOVERY\n" ); +printf ("detected recovery request (environment)\n" ); want_recovery =1 ;board/ifm/ac14xx/ac14xx.c int misc_init_r(void) · line 354
Quoted 10 lines
board/ifm/ac14xx/ac14xx.c +3
-3} - s =getenv ("install_failed" ); + s =getenv ("install_in_progress" );if ((s !=NULL ) && (*s !='\0' )) { -printf ("previous installation FAILED, running RECOVERY\n" ); +printf ("previous installation has not completed\n" ); want_recovery =1 ; } - s =getenv ("want_recovery" ); + s =getenv ("install_failed" );I am asking myself if it is strictly required to have multiple variables to identify if the "recovery" script must be run or not. If a previous install was interrupted ("
install_in_progress"
), or a request was initiated ("want recovery") or the last installation failed, is not so important. In all cases you set thewant_recoveryflag and starts the script. But using multiple variables it is not atomic, and could be, due for example to a system reset, that a variable is set without clearing the other one.
Quoted 3 lines
Why is not better to set a single variable, maybe with multiple values ? Value could be a simple integer (0=no recovery, 1=install_in_progress,,..) or still a string, if you prefer this solution.
Quick answer: holy compatibility with shipped products on one hand, and simplicity on the initiators' sides on the other hand. The full answer is more involved than one may think at first glance, as usual. :)
There are at least three (or four depending on how you count it) parties involved: The installer within the recovery system, the user in front of the product, and the regular application software including potential remote access (all of them are "initiators", and the bootloader (who is trying to help or to mediate between the former).
Some requests are carried out willingly (the user's pressing keys, the regular product software's or the remote access' wanting to enter maintenance mode), others are not so much of a willingly initiated request but instead mere consequence of failure (the installer's not being able to finish or to succeed). Some requests are done interactively in a moment (the user's pressing buttons during power on), others get queued and acted upon at any random later point in time (scheduling to enter maintenance mode but not before the next reboot). Notice also that the three parts (boot loader, recovery system, product software) may get developed and maintained by individual teams or under differing circumstances (regarding the targetted set of feature or supported complexity, differing schedules for releases or updates, maybe even locks or freezes after first shipping).
The current approach is appropriate because you don't have to coordinate the individual initiators, they don't have to agree on one common way of expressing their request, they don't even have to know about each other.
The evaluation of reasons in the boot loader is simple and straight forward and just errs to the safe side. The evaluation may look redundant yet the cost is acceptable and the complexity is rather low (totally cheap, mere OR, nothing else involved when you ignore the diagnostics).
Any pending reason will make the product enter maintenance mode, and not change any of the information which led there. Processing the interactive request upon key press or remote support intervention won't clobber the information about potentially incomplete or failed installation attempts. Error conditions will stick as long as they apply, without further logic needed. The product keeps nagging and enforces that a working status gets established (software gets installed, completely and successfully verified please), while additional manual intervention is possible as the need arises (one time access to the recovery system, not voiding any of the other information which may apply).
I feel that this is a good thing, simple to explain and to understand (once you know the motivation), and to test and to verify, and to maintain and support.
Introducing a uniform way of carrying the "start recovery mode, please" request interestingly increases complexity, you need to coordinate all the initiators. Having the one variable hold only one reason for the request opens a new area of unwanted interaction, having the variable hold multiple reasons introduces complex manipulation activities and may make you need to update all initiators when the logic changes or gets extended. Manipulating the variable instead of just setting or deleting it may introduce atomicity issues, unless you add complexity by adding locking (which all participants have to agree upon and respect).
So I see the desire to reduce the redundancy, but I'm afraid that this comes at the cost of increased complexity. Which may turn out to not be a benefit.
Are there other projects and experiences to learn from?
Quoted 6 lines
include/configs/ac14xx.h line 548
Quoted 2 lines
include/configs/ac14xx.h +1
-1-"rootpath=/opt/eldk/ppc_6xx\n" \ +"rootpath=/opt/eldk/ppc_6xx\0" \We do not set IP addresses or fix rootpath. Someone could have installed the rootfs on NFS on a different path. Also, IMHO rootpath should be simply removed.
Ah, removing the 'rootpath' spec (from the builtin set that is, still allowing for specs in user provided environments) eliminates my problem discussed in the other thread. Will have to ponder this for a moment.
virtually yours
Gerhard Sittig
Signature · 3 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de
v2 cover cleanup in the 'ifm AC14xx' board support
In reply to Gerhard Sittigthis change set adjusts the configuration of the 'ifm AC14xx' board to get closer to mainline philosophy and slightly improves builtin diagnostics and robustness after setup failure
changes in v2:
- address a potential NULL pointer dereference in the diagnostics code path as well (previously unhandled in mainline)
- split the previous convoluted v1 patch into a series of individual patches, each addressing one specific issue, to aid in the review process
Gerhard Sittig (7):
ac14xx: fix a potential NULL deref in diagnostics
ac14xx: cleanup comments in the board support
ac14xx: minor improvement in diagnostics
ac14xx: re-order the recovery condition checks
ac14xx: remove obsolete board config items
ac14xx: use the official product name everywhere
ac14xx: rephrase network boot config for development
board/ifm/ac14xx/ac14xx.c | 50 ++++++++++++++++++++++++--------------------- include/configs/ac14xx.h | 28 ++++++++++--------------- 2 files changed, 38 insertions(+), 40 deletions(-)
Signature · 1 line
1.7.10.4
PATCH v2 1/7 ac14xx: fix a potential NULL deref in diagnostics
getenv() immediately after setenv() may perfectly legally return NULL, so make sure to not deference an invalid pointer when creating diagnostic output
--- board/ifm/ac14xx/ac14xx.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
board/ifm/ac14xx/ac14xx.c +6 -2
@@ -209,6 +209,7 @@ static int read_eeprom(void)int mac_read_from_eeprom (void ) {const u8 *mac; +const char *mac_txt;if (read_eeprom ()) {printf ("I2C EEPROM read failed.\n" );
@@ -230,8 +231,11 @@ int mac_read_from_eeprom(void)if (mac &&is_valid_ether_addr (mac)) {eth_setenv_enetaddr ("ethaddr" , mac); -printf ("DIAG: %s() MAC value [%s]\n" , - __func__,getenv ("ethaddr" )); + mac_txt =getenv ("ethaddr" ); +if (mac_txt) +printf ("DIAG: MAC value [%s]\n" , mac_txt); +else +printf ("DIAG: failed to setup MAC env\n" ); }return 0 ;
Signature · 1 line
1.7.10.4
PATCH v2 1/7 ac14xx: fix a potential NULL deref in diagnostics
Dear Gerhard Sittig,
Quoted 7 lines · In message <[email protected]> you wrote:
getenv()immediately aftersetenv()may perfectly legally return NULL, so make sure to not deference an invalid pointer when creating diagnostic outputSigned-off-by Gerhard Sittig <[email protected]>--- board/ifm/ac14xx/ac14xx.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
Applied to u-boot-mpc5xxx, thanks.
Best regards,
Wolfgang Denk
Signature · 6 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de My brother sent me a postcard the other day with this big sattelite photo of the entire earth on it. On the back it said: "Wish you were here". - Steven Wright
PATCH v2 2/7 ac14xx: cleanup comments in the board support
In reply to Gerhard Sittigfix typos, minor rephrasing, remove obsolete notes and TODO items
--- board/ifm/ac14xx/ac14xx.c | 10 ++-------- include/configs/ac14xx.h | 5 ++--- 2 files changed, 4 insertions(+), 11 deletions(-)
board/ifm/ac14xx/ac14xx.c +2 -8
@@ -37,7 +37,7 @@ static void gpio_configure(void)/* * out_be32(&gpioregs->gpdir, 0xC2293020); -* workaround for a hardware affect: configure direction in pieces, +* workaround for a hardware effect: configure direction in pieces, * setting all outputs at once drops the reset line too low and * makes us lose the MII connection (breaks ethernet for us) */
@@ -330,8 +330,7 @@ int misc_init_r(void)gpio_configure ();/* -* check the GPIO keyboard, -* enforced start of the recovery when +* enforce the start of the recovery system when * - the appropriate keys were pressed * - a previous installation was aborted or has failed * - "some" external software told us to
@@ -339,13 +338,8 @@ int misc_init_r(void) want_recovery =0 ; keys =gpio_querykbd ();printf ("GPIO keyboard status [0x%08X]\n" , keys); -/* XXX insist in the _exact_ combination? */ if ((keys &GPIOKEY_BITS_RECOVERY ) ==GPIOKEY_BITS_RECOVERY ) {printf ("GPIO keyboard requested RECOVERY\n" ); -/* XXX TODO -* refine the logic to detect the first keypress, and -* wait to recheck IF it was the recovery combination? -*/ want_recovery =1 ; } s =getenv ("install_in_progress" );
include/configs/ac14xx.h +2 -3
@@ -72,7 +72,7 @@#define CONFIG_SYS_MAX_RAM_SIZE 0x20000000/* -* DDR Controller Configuration XXX TODO +* DDR Controller Configuration * * SYS_CFG: * [31:31] MDDRC Soft Reset: Diabled
@@ -265,7 +265,6 @@/* * CS related parameters -* TODO document these */ /* CS0 Flash */ #define CONFIG_SYS_CS0_CFG 0x00031110
@@ -506,7 +505,7 @@#define CONFIG_BOOTDELAY 2/* -1 disables auto-boot */ -/* XXX TODO need to specify the builtin environment */ +/* the builtin environment and standard greeting */ #define CONFIG_PREBOOT "echo;" \ "echo Type \\\"run flash_nfs\\\" to mount root filesystem over NFS;" \ "echo"
Signature · 1 line
1.7.10.4
PATCH v2 2/7 ac14xx: cleanup comments in the board support
Dear Gerhard Sittig,
Quoted 7 lines · In message <[email protected]> you wrote:
fix typos, minor rephrasing, remove obsolete notes and TODO items
Signed-off-by Gerhard Sittig <[email protected]>--- board/ifm/ac14xx/ac14xx.c | 10 ++-------- include/configs/ac14xx.h | 5 ++--- 2 files changed, 4 insertions(+), 11 deletions(-)
Applied to u-boot-mpc5xxx, thanks.
Best regards,
Wolfgang Denk
Signature · 4 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de I still miss my ex-wife, but my aim is getting better.
PATCH v2 3/7 ac14xx: minor improvement in diagnostics
In reply to Gerhard Sittig- minor rewording of diagnostics output
- make diagnostics optional and off by default
--- board/ifm/ac14xx/ac14xx.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)
board/ifm/ac14xx/ac14xx.c +20 -14
@@ -23,6 +23,10 @@#include <i2c.h> #endif +static int eeprom_diag; +static int mac_diag; +static int gpio_diag; +DECLARE_GLOBAL_DATA_PTR ;static void gpio_configure (void )
@@ -126,8 +130,6 @@ static u32 gpio_querykbd(void)/* excerpt from the recovery's hw_info.h */ -static int eeprom_diag =1 ; -struct __attribute__ ((__packed__ ))eeprom_layout {char magic[3 ];/** 'ifm' */ u8 len[2 ];/** content length without magic/len fields */
@@ -231,11 +233,13 @@ int mac_read_from_eeprom(void)if (mac &&is_valid_ether_addr (mac)) {eth_setenv_enetaddr ("ethaddr" , mac); - mac_txt =getenv ("ethaddr" ); -if (mac_txt) -printf ("DIAG: MAC value [%s]\n" , mac_txt); -else -printf ("DIAG: failed to setup MAC env\n" ); +if (mac_diag) { + mac_txt =getenv ("ethaddr" ); +if (mac_txt) +printf ("DIAG: MAC value [%s]\n" , mac_txt); +else +printf ("DIAG: failed to setup MAC env\n" ); + } }return 0 ;
@@ -337,29 +341,31 @@ int misc_init_r(void)*/ want_recovery =0 ; keys =gpio_querykbd (); -printf ("GPIO keyboard status [0x%08X]\n" , keys); +if (gpio_diag) +printf ("GPIO keyboard status [0x%02X]\n" , keys);if ((keys &GPIOKEY_BITS_RECOVERY ) ==GPIOKEY_BITS_RECOVERY ) { -printf ("GPIO keyboard requested RECOVERY\n" ); +printf ("detected recovery request (keyboard)\n" ); want_recovery =1 ; } s =getenv ("install_in_progress" );if ((s !=NULL ) && (*s !='\0' )) { -printf ("previous installation aborted, running RECOVERY\n" ); +printf ("previous installation has not completed\n" ); want_recovery =1 ; } s =getenv ("install_failed" );if ((s !=NULL ) && (*s !='\0' )) { -printf ("previous installation FAILED, running RECOVERY\n" ); +printf ("previous installation has failed\n" ); want_recovery =1 ; } s =getenv ("want_recovery" );if ((s !=NULL ) && (*s !='\0' )) { -printf ("running RECOVERY according to the request\n" ); +printf ("detected recovery request (environment)\n" ); want_recovery =1 ; } - -if (want_recovery) +if (want_recovery) { +printf ("enforced start of the recovery system\n" );setenv ("bootcmd" ,"run recovery" ); + }/* * boot the recovery system without waiting; boot the
Signature · 1 line
1.7.10.4
PATCH v2 3/7 ac14xx: minor improvement in diagnostics
Dear Gerhard Sittig,
Quoted 7 lines · In message <[email protected]> you wrote:
- minor rewording of diagnostics output
- make diagnostics optional and off by defaultSigned-off-by Gerhard Sittig <[email protected]>--- board/ifm/ac14xx/ac14xx.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)
Applied to u-boot-mpc5xxx, thanks.
Best regards,
Wolfgang Denk
Signature · 8 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I wrote my name at the top of the page. I wrote down the number of
the question ``1''. After much reflection I put a bracket round it
thus ``(1)''. But thereafter I could not think of anything connected
with it that was either relevant or true.
- Sir Winston Churchill _My Early Life_ ch. 2PATCH v2 4/7 ac14xx: re-order the recovery condition checks
In reply to Gerhard Sittigre-order the conditions which make the recovery system startup: combine those conditions which were explicitly initiated (key press, software request) and those which post-process error conditions (installer issues)
--- board/ifm/ac14xx/ac14xx.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
board/ifm/ac14xx/ac14xx.c +6 -6
@@ -336,8 +336,8 @@ int misc_init_r(void)/* * enforce the start of the recovery system when * - the appropriate keys were pressed -* - a previous installation was aborted or has failed * - "some" external software told us to +* - a previous installation was aborted or has failed */ want_recovery =0 ; keys =gpio_querykbd ();
@@ -347,6 +347,11 @@ int misc_init_r(void)printf ("detected recovery request (keyboard)\n" ); want_recovery =1 ; } + s =getenv ("want_recovery" ); +if ((s !=NULL ) && (*s !='\0' )) { +printf ("detected recovery request (environment)\n" ); + want_recovery =1 ; + } s =getenv ("install_in_progress" );if ((s !=NULL ) && (*s !='\0' )) {printf ("previous installation has not completed\n" );
@@ -357,11 +362,6 @@ int misc_init_r(void)printf ("previous installation has failed\n" ); want_recovery =1 ; } - s =getenv ("want_recovery" ); -if ((s !=NULL ) && (*s !='\0' )) { -printf ("detected recovery request (environment)\n" ); - want_recovery =1 ; - }if (want_recovery) {printf ("enforced start of the recovery system\n" );setenv ("bootcmd" ,"run recovery" );
Signature · 1 line
1.7.10.4
PATCH v2 4/7 ac14xx: re-order the recovery condition checks
Dear Gerhard Sittig,
Quoted 8 lines · In message <[email protected]> you wrote:
re-order the conditions which make the recovery system startup: combine those conditions which were explicitly initiated (key press, software request) and those which post-process error conditions (installer issues)
Signed-off-by Gerhard Sittig <[email protected]>--- board/ifm/ac14xx/ac14xx.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Applied to u-boot-mpc5xxx, thanks.
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de There are certain things men must do to remain men. -- Kirk, "The Ultimate Computer", stardate 4929.4
PATCH v2 5/7 ac14xx: remove obsolete board config items
In reply to Gerhard Sittig- use the default baudrate table for serial communication
- remove hostname/boofile/rootpath defines which were not referenced elsewhere
--- include/configs/ac14xx.h | 6 ------ 1 file changed, 6 deletions(-)
include/configs/ac14xx.h +0 -6
@@ -330,8 +330,6 @@#endif #define CONFIG_BAUDRATE 115200/* ... at 115200 bps */ -#define CONFIG_SYS_BAUDRATE_TABLE \ - {300, 600, 1200, 2400, 4800, 9600, 19200, 38400, 115200}#define CONSOLE_FIFO_TX_SIZE FIFOC_PSC3_TX_SIZE#define CONSOLE_FIFO_TX_ADDR FIFOC_PSC3_TX_ADDR
@@ -496,10 +494,6 @@#define CONFIG_ENV_OVERWRITE #define CONFIG_TIMESTAMP -#define CONFIG_HOSTNAME ac14xx -#define CONFIG_BOOTFILE "ac14xx/uImage" -#define CONFIG_ROOTPATH "/opt/eldk/ppc_6xx" -/* default load addr for tftp and bootm */ #define CONFIG_LOADADDR 400000
Signature · 1 line
1.7.10.4
PATCH v2 5/7 ac14xx: remove obsolete board config items
Dear Gerhard Sittig,
Quoted 7 lines · In message <[email protected]> you wrote:
- use the default baudrate table for serial communication
- remove hostname/boofile/rootpath defines which were not referenced elsewhereSigned-off-by Gerhard Sittig <[email protected]>--- include/configs/ac14xx.h | 6 ------ 1 file changed, 6 deletions(-)
Applied to u-boot-mpc5xxx, thanks.
Best regards,
Wolfgang Denk
Signature · 7 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de The further the spiritual evolution of mankind advances, the more certain it seems to me that the path to genuine religiosity does not lie through the fear of life, and the fear of death, and blind faith, but through striving after rational knowledge. - Albert Einstein
PATCH v2 6/7 ac14xx: use the official product name everywhere
In reply to Gerhard Sittigremove remaining "k6" code names, switch to the official 'ac14xx' name
--- include/configs/ac14xx.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
include/configs/ac14xx.h +5 -5
@@ -507,13 +507,13 @@#define CONFIG_EXTRA_ENV_SETTINGS_DEVEL \ "muster_nr=00\0" \ "fromram=run ramargs addip addtty; " \ - "tftp ${fdt_addr_r} k6m2/ac14xx.dtb-${muster_nr}; " \ - "tftp ${kernel_addr_r} k6m2/uImage-${muster_nr}; " \ - "tftp ${ramdisk_addr_r} k6m2/uFS-${muster_nr}; " \ + "tftp ${fdt_addr_r} ac14xx/ac14xx.dtb-${muster_nr}; " \ + "tftp ${kernel_addr_r} ac14xx/uImage-${muster_nr}; " \ + "tftp ${ramdisk_addr_r} ac14xx/uFS-${muster_nr}; " \ "bootm ${kernel_addr_r} ${ramdisk_addr_r} ${fdt_addr_r}\0" \ "fromnfs=run nfsargs addip addtty; " \ - "tftp ${fdt_addr_r} k6m2/ac14xx.dtb-${muster_nr}; " \ - "tftp ${kernel_addr_r} k6m2/uImage-${muster_nr}; " \ + "tftp ${fdt_addr_r} ac14xx/ac14xx.dtb-${muster_nr}; " \ + "tftp ${kernel_addr_r} ac14xx/uImage-${muster_nr}; " \ "bootm ${kernel_addr_r} - ${fdt_addr_r}\0" \ "fromflash=run nfsargs addip addtty; " \ "bootm fc020000 - fc000000\0" \
Signature · 1 line
1.7.10.4
PATCH v2 6/7 ac14xx: use the official product name everywhere
Dear Gerhard Sittig,
Quoted 6 lines · In message <[email protected]> you wrote:
remove remaining "k6" code names, switch to the official 'ac14xx' name
Signed-off-by Gerhard Sittig <[email protected]>--- include/configs/ac14xx.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Applied to u-boot-mpc5xxx, thanks.
Best regards,
Wolfgang Denk
Signature · 4 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de "Engineering without management is art." - Jeff Johnson
PATCH v2 7/7 ac14xx: rephrase network boot config for development
In reply to Gerhard Sittig- remove the builtin 'rootpath' spec (according to U-Boot project policy) and require user provided environments to contain these
- rephrase the evaluation of the 'muster_nr' approach which allows to quickly switch among several network boot setups (make the setting transparent when empty, resulting in default DULG behaviour)
- reduce the ARP timeout for faster network boot
--- include/configs/ac14xx.h | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
include/configs/ac14xx.h +9 -8
@@ -505,15 +505,15 @@ "echo"#define CONFIG_EXTRA_ENV_SETTINGS_DEVEL \ - "muster_nr=00\0" \ + "muster_nr=-00\0" \ "fromram=run ramargs addip addtty; " \ - "tftp ${fdt_addr_r} ac14xx/ac14xx.dtb-${muster_nr}; " \ - "tftp ${kernel_addr_r} ac14xx/uImage-${muster_nr}; " \ - "tftp ${ramdisk_addr_r} ac14xx/uFS-${muster_nr}; " \ + "tftp ${fdt_addr_r} ac14xx/ac14xx.dtb${muster_nr}; " \ + "tftp ${kernel_addr_r} ac14xx/uImage${muster_nr}; " \ + "tftp ${ramdisk_addr_r} ac14xx/uFS${muster_nr}; " \ "bootm ${kernel_addr_r} ${ramdisk_addr_r} ${fdt_addr_r}\0" \ "fromnfs=run nfsargs addip addtty; " \ - "tftp ${fdt_addr_r} ac14xx/ac14xx.dtb-${muster_nr}; " \ - "tftp ${kernel_addr_r} ac14xx/uImage-${muster_nr}; " \ + "tftp ${fdt_addr_r} ac14xx/ac14xx.dtb${muster_nr}; " \ + "tftp ${kernel_addr_r} ac14xx/uImage${muster_nr}; " \ "bootm ${kernel_addr_r} - ${fdt_addr_r}\0" \ "fromflash=run nfsargs addip addtty; " \ "bootm fc020000 - fc000000\0" \
@@ -541,12 +541,11 @@"u-boot=ac14xx/u-boot.bin\0" \"bootfile=ac14xx/uImage\0" \"fdtfile=ac14xx/ac14xx.dtb\0" \ -"rootpath=/opt/eldk/ppc_6xx\n" \"netdev=eth0\0" \"consdev=ttyPSC0\0" \"hostname=ac14xx\0" \"nfsargs=setenv bootargs root=/dev/nfs rw " \ -"nfsroot=${serverip}:${rootpath}-${muster_nr}\0" \ +"nfsroot=${serverip}:${rootpath}${muster_nr}\0" \"ramargs=setenv bootargs root=/dev/ram rw\0" \"addip=setenv bootargs ${bootargs} " \"ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}" \
@@ -576,6 +575,8 @@#define CONFIG_BOOTCOMMAND "run production" +#define CONFIG_ARP_TIMEOUT 200UL +#define CONFIG_FIT 1#define CONFIG_OF_LIBFDT 1#define CONFIG_OF_BOARD_SETUP 1
Signature · 1 line
1.7.10.4
PATCH v2 7/7 ac14xx: rephrase network boot config for development
Dear Gerhard Sittig,
Quoted 11 lines · In message <[email protected]> you wrote:
- remove the builtin 'rootpath' spec (according to U-Boot project policy) and require user provided environments to contain these
- rephrase the evaluation of the 'muster_nr' approach which allows to quickly switch among several network boot setups (make the setting transparent when empty, resulting in default DULG behaviour)
- reduce the ARP timeout for faster network bootSigned-off-by Gerhard Sittig <[email protected]>--- include/configs/ac14xx.h | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
Applied to u-boot-mpc5xxx, thanks.
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de "There was no difference between the behavior of a god and the operations of pure chance..." - Thomas Pynchon, _Gravity's Rainbow_
v2 cover cleanup in the 'ifm AC14xx' board support
In reply to Gerhard SittigQuoted 6 lines · Gerhard Sittig
this change set adjusts the configuration of the 'ifm AC14xx' board to get closer to mainline philosophy and slightly improves builtin diagnostics and robustness after setup failure
Quoted 6 lines
changes in v2:
- address a potential NULL pointer dereference in the diagnostics code path as well (previously unhandled in mainline)
- split the previous convoluted v1 patch into a series of individual patches, each addressing one specific issue, to aid in the review process
Are there any other concerns that I shall address? Any feedback beyond what was mentioned before?
virtually yours
Gerhard Sittig
Signature · 3 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de