From mboxrd@z Thu Jan  1 00:00:00 1970
From: Troy Kisky <troy.kisky@boundarydevices.com>
Date: Mon, 04 Jun 2012 13:43:07 -0700
Subject: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode
 command
In-Reply-To: <20120604194634.BC3A5204AE4@gemini.denx.de>
References: <1338066111-5835-1-git-send-email-troy.kisky@boundarydevices.com>
	<20120604194634.BC3A5204AE4@gemini.denx.de>
Message-ID: <4FCD1DDB.1010201@boundarydevices.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 6/4/2012 12:46 PM, Wolfgang Denk wrote:
> Dear Troy Kisky,
>
> In message<1338066111-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 Kisky<troy.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;
>> +		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
>