ARM: mxs: rename function that sets AUTO_RESTART flag

PATCH New Hector Palacios 3 participants 6 messages Jul 24, 2013 – Aug 31, 2013

Subsystems (2)

Arm (32-bit) Arm i.MX and Layerscape

lore

Share & tools

lore

Hector Palacios

author

2013-07-24 14:01 UTC #0 lore raw

PATCH 1/1 ARM: mxs: rename function that sets AUTO_RESTART flag

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 <[email protected]>

---
 arch/arm/cpu/arm926ejs/mxs/spl_power_init.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

arch/arm/cpu/arm926ejs/mxs/spl_power_init.c +3 -6

index 5ee2d88..285ac79 100644
@@ -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();

Marek Vasut

2013-07-24 15:54 UTC #1 lore raw

PATCH 1/1 ARM: mxs: rename function that sets AUTO_RESTART flag

Dear Hector Palacios,

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 20 lines

Signed-off-by Hector Palacios <[email protected]>

---
 arch/arm/cpu/arm926ejs/mxs/spl_power_init.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c index 5ee2d88..285ac79 100644

arch/arm/cpu/arm926ejs/mxs/spl_power_init.c +1 -1

@@ -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;

arch/arm/cpu/arm926ejs/mxs/spl_power_init.c static void mxs_power_clear_auto_restart(void) · line 68

Quoted 8 lines

arch/arm/cpu/arm926ejs/mxs/spl_power_init.c +1 -4

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

arch/arm/cpu/arm926ejs/mxs/spl_power_init.c void mxs_power_init(void) · line 905

Quoted 11 lines

arch/arm/cpu/arm926ejs/mxs/spl_power_init.c +1 -1

 	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

Hector Palacios

author

2013-07-24 16:08 UTC #2 lore raw

PATCH 1/1 ARM: mxs: rename function that sets AUTO_RESTART flag

Dear Marek,

Quoted 12 lines · Marek Vasut

Dear Hector Palacios,
Quoted 10 lines
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.

Quoted 5 lines

Quoted 3 lines
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.

Quoted 23 lines

Signed-off-by Hector Palacios <[email protected]>

---
  arch/arm/cpu/arm926ejs/mxs/spl_power_init.c | 9 +++------
  1 file changed, 3 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/arch/arm/cpu/arm926ejs/mxs/spl_power_init.c index 5ee2d88..285ac79 100644

arch/arm/cpu/arm926ejs/mxs/spl_power_init.c +1 -1

@@ -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)
  		;

Quoted 8 lines

arch/arm/cpu/arm926ejs/mxs/spl_power_init.c static void mxs_power_clear_auto_restart(void) · line 68
Quoted 5 lines
arch/arm/cpu/arm926ejs/mxs/spl_power_init.c +1 -4
-	/*
-	 * 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.

Quoted 6 lines

arch/arm/cpu/arm926ejs/mxs/spl_power_init.c +0 -0

  	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;

Quoted 9 lines

arch/arm/cpu/arm926ejs/mxs/spl_power_init.c void mxs_power_init(void) · line 905
Quoted 5 lines
arch/arm/cpu/arm926ejs/mxs/spl_power_init.c +1 -1
  	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

Stefano Babic

2013-07-26 08:50 UTC #3 lore raw

PATCH 1/1 ARM: mxs: rename function that sets AUTO_RESTART flag

Hi Hector,

Quoted 10 lines · Hector Palacios

Quoted 1 line
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.

Quoted 19 lines

Quoted 9 lines
Quoted 9 lines
arch/arm/cpu/arm926ejs/mxs/spl_power_init.c +1 -1
-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)
          ;
 
Quoted 9 lines
arch/arm/cpu/arm926ejs/mxs/spl_power_init.c static void mxs_power_clear_auto_restart(void) · line 68
Quoted 5 lines
arch/arm/cpu/arm926ejs/mxs/spl_power_init.c +1 -4
-    /*
-     * 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.

Quoted 3 lines

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

Signature · 5 lines

=====================================================================
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: sbabic at denx.de
=====================================================================

Marek Vasut

2013-07-26 08:58 UTC #4 lore raw

PATCH 1/1 ARM: mxs: rename function that sets AUTO_RESTART flag

Dear Stefano Babic,

Quoted 14 lines

Hi Hector,
Quoted 10 lines · Hector Palacios
Quoted 1 line
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.

Quoted 3 lines

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 }

Quoted 25 lines

Quoted 25 lines
Quoted 24 lines
Quoted 10 lines
arch/arm/cpu/arm926ejs/mxs/spl_power_init.c +1 -1
-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;
arch/arm/cpu/arm926ejs/mxs/spl_power_init.c static void mxs_power_clear_auto_restart(void) · line 70
Quoted 10 lines
arch/arm/cpu/arm926ejs/mxs/spl_power_init.c +1 -4
@@ -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.

Quoted 5 lines

Quoted 3 lines
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

Stefano Babic

2013-08-31 15:51 UTC #5 lore raw

PATCH 1/1 ARM: mxs: rename function that sets AUTO_RESTART flag

In reply to Hector Palacios

Quoted 10 lines · 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.

Quoted 6 lines

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 <[email protected]>
---

Applied to u-boot-imx, thanks.

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
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: sbabic at denx.de
=====================================================================