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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
