On 6/4/2012 12:46 PM, Wolfgang Denk wrote:
Dear Troy Kisky,
In message1338066111-5835-1-git-send-email-troy.kisky@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 Kiskytroy.kisky@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;elsereg&= ~(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.
Thanks for the feedback. I'll try building this string at run-time to keep consistent.
Best regards,
Wolfgang Denk