[U-Boot] [PATCH v1 1/1] mpc512x: adjust and improve AC14xx board support

[U-Boot] [PATCH v1 1/1] mpc512x: adjust and improve AC14xx board support

[Gerhard Sittig]

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

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 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


diff --git a/board/ifm/ac14xx/ac14xx.c b/board/ifm/ac14xx/ac14xx.c
index 7442591..e47f63d 100644
--- a/board/ifm/ac14xx/ac14xx.c
+++ b/board/ifm/ac14xx/ac14xx.c
@@ -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
diff --git a/include/configs/ac14xx.h b/include/configs/ac14xx.h
index 7cb10fb..d24d014 100644
--- a/include/configs/ac14xx.h
+++ b/include/configs/ac14xx.h
@@ -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
-- 
1.7.10.4

[U-Boot] [PATCH v1 1/1] mpc512x: adjust and improve AC14xx board support

[Wolfgang Denk]

Dear Gerhard,

In message <1370257851-27583-1-git-send-email-gsi@denx.de> you wrote:
>
> +		if (mac_diag)
> +			printf("DIAG: MAC [%s]\n", getenv("ethaddr"));

What happens if "ethaddr" is not defined in the environment?

...
> +	"rootpath=/opt/eldk/ppc_6xx\0"					\

You are really still using ELDK 4.x ?

Best regards,

Wolfgang Denk

-- 
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.

[U-Boot] [PATCH v1 1/1] mpc512x: adjust and improve AC14xx board support

[Gerhard Sittig]

On Mon, Jun 03, 2013 at 15:07 +0200, Wolfgang Denk wrote:
> 
> In message <1370257851-27583-1-git-send-email-gsi@denx.de> you wrote:
> >
> > +		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.

> ...
> > +	"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
-- 
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

[U-Boot] [PATCH v1 1/1] mpc512x: adjust and improve AC14xx board support

[Wolfgang Denk]

Dear Gerhard Sittig,

In message <20130603135004.GH2571@book.gsilab.sittig.org> you wrote:
>
> > > +		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.

...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.

> > > +	"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

-- 
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.

[U-Boot] [PATCH v1 1/1] mpc512x: adjust and improve AC14xx board support

[Stefano Babic]

Hi Gerhard,

On 03/06/2013 13:10, Gerhard Sittig wrote:

> -	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");

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.

> -	"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

-- 
=====================================================================
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
=====================================================================

[U-Boot] [PATCH v1 1/1] mpc512x: adjust and improve AC14xx board support

[Gerhard Sittig]

On Mon, Jun 03, 2013 at 15:31 +0200, Stefano Babic wrote:
> 
> On 03/06/2013 13:10, Gerhard Sittig wrote:
> 
> > -	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");
> 
> 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.

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?


> > -	"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
-- 
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

[U-Boot] [PATCH v2 0/7] cleanup in the 'ifm AC14xx' board support

[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


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(-)

-- 
1.7.10.4

[U-Boot] [PATCH v2 1/7] ac14xx: fix a potential NULL deref in diagnostics

[Gerhard Sittig]

getenv() immediately after setenv() may perfectly legally return NULL, so
make sure to not deference an invalid pointer when creating diagnostic output

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 board/ifm/ac14xx/ac14xx.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/board/ifm/ac14xx/ac14xx.c b/board/ifm/ac14xx/ac14xx.c
index 7442591..f200b45 100644
--- a/board/ifm/ac14xx/ac14xx.c
+++ b/board/ifm/ac14xx/ac14xx.c
@@ -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;
-- 
1.7.10.4

[U-Boot] [PATCH v2 1/7] ac14xx: fix a potential NULL deref in diagnostics

[Wolfgang Denk]

Dear Gerhard Sittig,

In message <1370436671-12422-2-git-send-email-gsi@denx.de> you wrote:
> getenv() immediately after setenv() may perfectly legally return NULL, so
> make sure to not deference an invalid pointer when creating diagnostic output
> 
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
>  board/ifm/ac14xx/ac14xx.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Applied to u-boot-mpc5xxx, thanks.

Best regards,

Wolfgang Denk

-- 
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

[U-Boot] [PATCH v2 2/7] ac14xx: cleanup comments in the board support

[Gerhard Sittig]

fix typos, minor rephrasing, remove obsolete notes and TODO items

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 board/ifm/ac14xx/ac14xx.c |   10 ++--------
 include/configs/ac14xx.h  |    5 ++---
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/board/ifm/ac14xx/ac14xx.c b/board/ifm/ac14xx/ac14xx.c
index f200b45..d45b7b2 100644
--- a/board/ifm/ac14xx/ac14xx.c
+++ b/board/ifm/ac14xx/ac14xx.c
@@ -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");
diff --git a/include/configs/ac14xx.h b/include/configs/ac14xx.h
index 7cb10fb..5dbcba2 100644
--- a/include/configs/ac14xx.h
+++ b/include/configs/ac14xx.h
@@ -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"
-- 
1.7.10.4

[U-Boot] [PATCH v2 2/7] ac14xx: cleanup comments in the board support

[Wolfgang Denk]

Dear Gerhard Sittig,

In message <1370436671-12422-3-git-send-email-gsi@denx.de> you wrote:
> fix typos, minor rephrasing, remove obsolete notes and TODO items
> 
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
>  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

-- 
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.

[U-Boot] [PATCH v2 3/7] ac14xx: minor improvement in diagnostics

[Gerhard Sittig]

- minor rewording of diagnostics output
- make diagnostics optional and off by default

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 board/ifm/ac14xx/ac14xx.c |   34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/board/ifm/ac14xx/ac14xx.c b/board/ifm/ac14xx/ac14xx.c
index d45b7b2..c8e88cc 100644
--- a/board/ifm/ac14xx/ac14xx.c
+++ b/board/ifm/ac14xx/ac14xx.c
@@ -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
-- 
1.7.10.4

[U-Boot] [PATCH v2 3/7] ac14xx: minor improvement in diagnostics

[Wolfgang Denk]

Dear Gerhard Sittig,

In message <1370436671-12422-4-git-send-email-gsi@denx.de> you wrote:
> - minor rewording of diagnostics output
> - make diagnostics optional and off by default
> 
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
>  board/ifm/ac14xx/ac14xx.c |   34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)

Applied to u-boot-mpc5xxx, thanks.


Best regards,

Wolfgang Denk

-- 
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. 2

[U-Boot] [PATCH v2 4/7] ac14xx: re-order the recovery condition checks

[Gerhard Sittig]

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 <gsi@denx.de>
---
 board/ifm/ac14xx/ac14xx.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/board/ifm/ac14xx/ac14xx.c b/board/ifm/ac14xx/ac14xx.c
index c8e88cc..dc2aff0 100644
--- a/board/ifm/ac14xx/ac14xx.c
+++ b/board/ifm/ac14xx/ac14xx.c
@@ -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");
-- 
1.7.10.4

[U-Boot] [PATCH v2 4/7] ac14xx: re-order the recovery condition checks

[Wolfgang Denk]

Dear Gerhard Sittig,

In message <1370436671-12422-5-git-send-email-gsi@denx.de> 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 <gsi@denx.de>
> ---
>  board/ifm/ac14xx/ac14xx.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Applied to u-boot-mpc5xxx, thanks.


Best regards,

Wolfgang Denk

-- 
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

[U-Boot] [PATCH v2 5/7] ac14xx: remove obsolete board config items

[Gerhard Sittig]

- use the default baudrate table for serial communication
- remove hostname/boofile/rootpath defines which were not referenced elsewhere

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 include/configs/ac14xx.h |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/include/configs/ac14xx.h b/include/configs/ac14xx.h
index 5dbcba2..e06e9ea 100644
--- a/include/configs/ac14xx.h
+++ b/include/configs/ac14xx.h
@@ -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
 
-- 
1.7.10.4

[U-Boot] [PATCH v2 5/7] ac14xx: remove obsolete board config items

[Wolfgang Denk]

Dear Gerhard Sittig,

In message <1370436671-12422-6-git-send-email-gsi@denx.de> you wrote:
> - use the default baudrate table for serial communication
> - remove hostname/boofile/rootpath defines which were not referenced elsewhere
> 
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
>  include/configs/ac14xx.h |    6 ------
>  1 file changed, 6 deletions(-)

Applied to u-boot-mpc5xxx, thanks.


Best regards,

Wolfgang Denk

-- 
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

[U-Boot] [PATCH v2 6/7] ac14xx: use the official product name everywhere

[Gerhard Sittig]

remove remaining "k6" code names, switch to the official 'ac14xx' name

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 include/configs/ac14xx.h |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/configs/ac14xx.h b/include/configs/ac14xx.h
index e06e9ea..fcb56f4 100644
--- a/include/configs/ac14xx.h
+++ b/include/configs/ac14xx.h
@@ -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"				\
-- 
1.7.10.4

[U-Boot] [PATCH v2 6/7] ac14xx: use the official product name everywhere

[Wolfgang Denk]

Dear Gerhard Sittig,

In message <1370436671-12422-7-git-send-email-gsi@denx.de> you wrote:
> remove remaining "k6" code names, switch to the official 'ac14xx' name
> 
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
>  include/configs/ac14xx.h |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Applied to u-boot-mpc5xxx, thanks.


Best regards,

Wolfgang Denk

-- 
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

[U-Boot] [PATCH v2 7/7] ac14xx: rephrase network boot config for development

[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

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 include/configs/ac14xx.h |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/configs/ac14xx.h b/include/configs/ac14xx.h
index fcb56f4..381bcdd 100644
--- a/include/configs/ac14xx.h
+++ b/include/configs/ac14xx.h
@@ -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
-- 
1.7.10.4

[U-Boot] [PATCH v2 7/7] ac14xx: rephrase network boot config for development

[Wolfgang Denk]

Dear Gerhard Sittig,

In message <1370436671-12422-8-git-send-email-gsi@denx.de> 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 boot
> 
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
>  include/configs/ac14xx.h |   17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)

Applied to u-boot-mpc5xxx, thanks.

Best regards,

Wolfgang Denk

-- 
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_

[U-Boot] [PATCH v2 0/7] cleanup in the 'ifm AC14xx' board support

[Gerhard Sittig]

On Wed, Jun 05, 2013 at 14:51 +0200, Gerhard Sittig wrote:
> 
> 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
> 
> 
> 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
-- 
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