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

2013-08-31 Thread Stefano Babic 
On 24/07/2013 16:01, Hector Palacios wrote:
 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.
 
 Signed-off-by: Hector Palacios hector.palac...@digi.com
 ---

Applied to u-boot-imx, thanks.

Best regards,
Stefano Babic


-- 
=
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-07-26 Thread 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.

 -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.

Best regards,
Stefano Babic

-- 
=
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

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

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

2013-07-24 Thread 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.

Signed-off-by: Hector Palacios hector.palac...@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 */
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();
 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-07-24 Thread Marek Vasut 
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.

 Signed-off-by: Hector Palacios hector.palac...@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.

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

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

2013-07-24 Thread 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.palac...@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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot