[U-Boot] [PATCH] ARM: mxs: rename function that sets AUTO_RESTART flag

[Hector Palacios]

Dear Marek,

On 07/24/2013 05:54 PM, Marek Vasut wrote:
> Dear Hector Palacios,
>
>> The AUTO_RESTART flag of HW_RTC_PERSISTENT0 register will
>> power up the chip automatically 180ms after power down.
>> This bit must be enabled by the boot loader to ensure the
>> target can start upon hardware reset or watchdog reset
>> even when powered from a battery.
>>
>> Currently the function named 'mxs_power_clear_auto_restart()'
>> is setting this flag although the 'clear' in its name suggest
>> the opposite.
>>
>> This patch renames the function to 'mxs_power_set_auto_restart()'
>> and removes the comment about EVK revision A which was confusing
>> because the function indeed was setting the flag.
>
> Why? The comment seems fully valid to me.

The comment is confusing for a number of reasons:
- It says for revision A of the EVK the flag must be enabled but the code is common 
for all platforms and there is no distinction about the platform where it runs. The 
flag must be enabled for any platform.
- The flag is set by the function in any case (except if it is already set) so the 
comment superfluous.
- The comment doesn't even say what the problem is on EVK_A or why the setting of the 
bit helps.

Unless someone from Freescale elaborates the comment, I would remove it.
At minimum I would move it somewhere else, like to the top of the function, not where 
it is now. Maybe Fabio can help.

>> Signed-off-by: Hector Palacios <hector.palacios at digi.com>
>> ---
>>   arch/arm/cpu/arm926ejs/mxs/spl_power_init.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
>> b/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c index 5ee2d88..285ac79
>> 100644
>> --- a/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
>> +++ b/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c
>> @@ -52,7 +52,7 @@ static void mxs_power_clock2pll(void)
>>   			CLKCTRL_CLKSEQ_BYPASS_CPU);
>>   }
>>
>> -static void mxs_power_clear_auto_restart(void)
>> +static void mxs_power_set_auto_restart(void)
>>   {
>>   	struct mxs_rtc_regs *rtc_regs =
>>   		(struct mxs_rtc_regs *)MXS_RTC_BASE;
>> @@ -65,10 +65,7 @@ static void mxs_power_clear_auto_restart(void)
>>   	while (readl(&rtc_regs->hw_rtc_ctrl) & RTC_CTRL_CLKGATE)
>>   		;
>>
>> -	/*
>> -	 * Due to the hardware design bug of mx28 EVK-A
>> -	 * we need to set the AUTO_RESTART bit.
>> -	 */
>> +	/* Do nothing if flag already set */
>
> You're changing the behavior here and it's not documented. I see no point in
> this change while at it.

I'm not changing the behavior. I just renamed the function to reflect what the 
function does, which is set the flag, not clear it. Apart from the renaming, I didn't 
touch a line of real code.

>>   	if (readl(&rtc_regs->hw_rtc_persistent0) & RTC_PERSISTENT0_AUTO_RESTART)
>>   		return;
>>
>> @@ -903,7 +900,7 @@ void mxs_power_init(void)
>>   		(struct mxs_power_regs *)MXS_POWER_BASE;
>>
>>   	mxs_power_clock2xtal();
>> -	mxs_power_clear_auto_restart();
>> +	mxs_power_set_auto_restart();
>>   	mxs_power_set_linreg();
>>   	mxs_power_setup_5v_detect();
>
> Best regards,
> Marek Vasut
>


Best regards,
--
Hector Palacios