[U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

[Wolfgang Denk]

Dear Troy Kisky,

In message <1338066111-5835-1-git-send-email-troy.kisky at boundarydevices.com> you wrote:
> This is useful for forcing the ROM's
> usb downloader to activate upon a watchdog reset.
> Or, you can boot from either SD Card.
> 
> Currently, support added for MX53 and MX6Q
> Signed-off-by: Troy Kisky <troy.kisky at boundarydevices.com>

This is highly architecture specific code.  I do not want to have this
in the common/ directory.

> --- /dev/null
> +++ b/common/cmd_rsmode.c
> @@ -0,0 +1,118 @@
...
> +#ifdef CONFIG_MX53
...
> +#ifdef CONFIG_MX6Q
...

Instead of starting a (probably growing) list of #ifdef's here, you
whould rather just include a header file which gets auto-selected for
the CPU in use by the usual config mechanism.

So please move allthese defines into appropriate header files.

> +#define SBMR_COPY_ADDR &((struct srtc_regs *)SRTC_BASE_ADDR)->lpgr

NAK.  This is ugly and does not scale.  Feel free to provide
architecturre specific (inline?) accessor functions to this register.

> +int do_rsmode(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	int i;
> +	if (argc < 2) {

Empty line between declarations and code, please.

> +options:
> +		printf("Options:\n");
> +		for (i = 0; i < ARRAY_SIZE(modes); i++)
> +			printf("%s\n", modes[i].name);
> +		return 0;

NAK.  The command should return proper error codes.  Please use the
regular usage() call here.

> +	if (i >= ARRAY_SIZE(modes))
> +		goto options;

Please get rid of this goto.

> +	writel(modes[i].cfg_val, SBMR_COPY_ADDR);
> +#ifdef SMBR_COPY_ENABLE_ADDR
> +	{
> +		unsigned reg = readl(SMBR_COPY_ENABLE_ADDR);
> +		if (i)
> +			reg |= 1 << 28;
> +		else
> +			reg &= ~(1 << 28);
> +		writel(reg, SMBR_COPY_ENABLE_ADDR);
> +	}
> +#endif

You mean, if SMBR_COPY_ENABLE_ADDR was not defined, the command would
silently turn into a NOP?  One more reson not to allow such code.

> +U_BOOT_CMD(
> +	rsmode, 2, 0, do_rsmode,
> +	"change reset mode to normal/usb/sata/ecspi1:1/esdhc1",

How do you intend to keep this doucmentation in sync with the code?
Either the above code (reading the entries to mode[]) is redundant, or
this is incorrect - or probably both.

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 question of whether a computer can think is no more  interesting
than the question of whether a submarine can swim"
                                                - Edsgar W.  Dijkstra