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

[Marek Vasut]

Dear Stefano Babic,

> Hi Hector,
> 
> On 24/07/2013 18:08, Hector Palacios wrote:
> >> 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.
> 
> Right, I see the same. The comment hides some information, because we do
> not know which is the issue and in any case code does the opposite,
> setting the bit in any case. I think it is better to drop it.

Ah, I just looked at the code again, you're both right. I would expand the 
comment once we know what the problem is on EVK-A.

This is the original code from imx-bootlets power-prep/power-prep.c :

 304 /* clear RTC ALARM wakeup or AUTORESTART bits here. */
 305 void PowerPrep_ClearAutoRestart( void )
 306 {
 307         HW_RTC_CTRL_CLR( BM_ICOLL_CTRL_SFTRST );
 308         while( HW_RTC_CTRL.B.SFTRST == 1 );
 309         HW_RTC_CTRL_CLR( BM_ICOLL_CTRL_CLKGATE );
 310         while( HW_RTC_CTRL.B.CLKGATE == 1 );
 311         /* Due to the hardware design bug of mx28 EVK-A
 312         * we need to set the AUTO_RESTART bit.
 313         */
 314         if(HW_RTC_PERSISTENT0.B.AUTO_RESTART==0)
 315         {
 316                 while(BF_RD( RTC_STAT, NEW_REGS));
 317                 HW_RTC_PERSISTENT0.B.AUTO_RESTART = 1;
 318                 BF_SET(RTC_CTRL, FORCE_UPDATE);
 319                 BF_CLR(RTC_CTRL, FORCE_UPDATE);
 320                 while(BF_RD( RTC_STAT, NEW_REGS));
 321                 while(BF_RD( RTC_STAT, STALE_REGS));
 322         }
 323 }

> >>> -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.
> 
> I see also no changes in behavior, it is only a rename.

I must have been fully blind, please feel free to laugh at me, this calls for a 
facepalm. Sorry.

Best regards,
Marek Vasut