[U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command
Troy Kisky
troy.kisky at boundarydevices.com
Mon Jun 4 22:43:07 CEST 2012
On 6/4/2012 12:46 PM, Wolfgang Denk wrote:
> 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.
Thanks for the feedback. I'll try building this string at run-time to
keep consistent.
>
> Best regards,
>
> Wolfgang Denk
>
More information about the U-Boot
mailing list