Dear Troy Kisky,
In message <1338066111-5835-1-git-send-email-troy.ki...@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.ki...@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: w...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
