Skip to contentSkip to conversation

ARM: omap_hwmod: Add a new state to handle hwmods left enabled at init

PATCH New Rajendra Nayak 4 participants 8 messages Nov 17, 2011 – Nov 18, 2011

Subsystems (4)
Arm (32-bit) Arm sub-architectures OMAP2+ support OMAP hwmod support
lore
Share & tools
lore

Rajendra Nayak

author
#0 lore raw

PATCH 1/1 ARM: omap_hwmod: Add a new state to handle hwmods left enabled at init

A hwmod with a 'HWMOD_INIT_NO_IDLE' flag set, is left in enabled state by the hwmod framework post the initial setup. Once a real user of the device (a driver) tries to enable it at a later point, the hmwod framework throws a WARN() about the device being already in enabled state.

Fix this by introducing a new state '_HWMOD_STATE_ENABLED_AT_INIT' to identify such devices/hwmods, so nothing but just a state change to '_HWMOD_STATE_ENABLED' can be done when the device/hwmod is requested to be enabled (the first time) by its driver/user.

A good example of a such a device is an UART used as debug console. The UART module needs to be kept enabled through the boot, until the UART driver takes control of it, for debug prints to appear on the console.

Signed-off-by Rajendra Nayak <[email protected]>
---
 arch/arm/mach-omap2/omap_hwmod.c             |   16 ++++++++++++++--
 arch/arm/plat-omap/include/plat/omap_hwmod.h |    1 +
 2 files changed, 15 insertions(+), 2 deletions(-)
arch/arm/mach-omap2/omap_hwmod.c +14 -2
index d7f4623..7d94cc3 100644
@@ -1441,6 +1441,17 @@ static int _enable(struct omap_hwmod *oh)
 
 	pr_debug("omap_hwmod: %s: enabling\n", oh->name);
 
+	/*
+	 * hwmods' with HWMOD_INIT_NO_IDLE flag set, are left
+	 * in enabled state at init.
+	 * Now that someone is really trying to enable them,
+	 * just update the state.
+	 */
+	if (oh->_state == _HWMOD_STATE_ENABLED_AT_INIT) {
+		oh->_state = _HWMOD_STATE_ENABLED;
+		return 0;
+	}
+
 	if (oh->_state != _HWMOD_STATE_INITIALIZED &&
 	    oh->_state != _HWMOD_STATE_IDLE &&
 	    oh->_state != _HWMOD_STATE_DISABLED) {

@@ -1449,7 +1460,6 @@ static int _enable(struct omap_hwmod *oh)
 		return -EINVAL;
 	}
 
-
 	/*
 	 * If an IP contains only one HW reset line, then de-assert it in order
 	 * to allow the module state transition. Otherwise the PRCM will return

@@ -1744,8 +1754,10 @@ static int _setup(struct omap_hwmod *oh, void *data)
 	 * it should be set by the core code as a runtime flag during startup
 	 */
 	if ((oh->flags & HWMOD_INIT_NO_IDLE) &&
-	    (postsetup_state == _HWMOD_STATE_IDLE))
+	    (postsetup_state == _HWMOD_STATE_IDLE)) {
+		oh->_state = _HWMOD_STATE_ENABLED_AT_INIT;
 		postsetup_state = _HWMOD_STATE_ENABLED;
+	}
 
 	if (postsetup_state == _HWMOD_STATE_IDLE)
 		_idle(oh);
arch/arm/plat-omap/include/plat/omap_hwmod.h +1 -0
index 1b81dfb..cd49d60 100644
@@ -436,6 +436,7 @@ struct omap_hwmod_omap4_prcm {
 #define _HWMOD_STATE_ENABLED			4
 #define _HWMOD_STATE_IDLE			5
 #define _HWMOD_STATE_DISABLED			6
+#define _HWMOD_STATE_ENABLED_AT_INIT		7
 
 /**
  * struct omap_hwmod_class - the type of an IP block
Signature · 1 line
1.7.1
#1 lore raw

PATCH 1/1 ARM: omap_hwmod: Add a new state to handle hwmods left enabled at init

+ Paul

Quoted 4 lines · Rajendra Nayak

A hwmod with a 'HWMOD_INIT_NO_IDLE' flag set, is left in enabled state by the hwmod framework post the initial setup. Once a real user of the device (a driver) tries to enable it at a later point, the hmwod framework throws a WARN() about

typo: -------------------^

Quoted 23 lines

the device being already in enabled state.

Fix this by introducing a new state '_HWMOD_STATE_ENABLED_AT_INIT' to identify such devices/hwmods, so nothing but just a state change to '_HWMOD_STATE_ENABLED' can be done when the device/hwmod is requested to be enabled (the first time) by its driver/user.

A good example of a such a device is an UART used as debug console. The UART module needs to be kept enabled through the boot, until the UART driver takes control of it, for debug prints to appear on the console.

Signed-off-by Rajendra Nayak<[email protected]>
---
  arch/arm/mach-omap2/omap_hwmod.c             |   16 ++++++++++++++--
  arch/arm/plat-omap/include/plat/omap_hwmod.h |    1 +
  2 files changed, 15 insertions(+), 2 deletions(-)
arch/arm/mach-omap2/omap_hwmod.c +0 -0
index d7f4623..7d94cc3 100644

arch/arm/mach-omap2/omap_hwmod.c static int _enable(struct omap_hwmod *oh) · line 1445

Quoted 5 lines
arch/arm/mach-omap2/omap_hwmod.c +2 -0
@@ -1441,6 +1441,17 @@ static int _enable(struct omap_hwmod *oh)
 
  	pr_debug("omap_hwmod: %s: enabling\n", oh->name);
 
+	/*
+	 * hwmods' with HWMOD_INIT_NO_IDLE flag set, are left

Is the ' needed here? But I'm not an expert in English grammar...

Quoted 13 lines
arch/arm/mach-omap2/omap_hwmod.c +9 -0
+	 * in enabled state at init.
+	 * Now that someone is really trying to enable them,
+	 * just update the state.
+	 */
+	if (oh->_state == _HWMOD_STATE_ENABLED_AT_INIT) {
+		oh->_state = _HWMOD_STATE_ENABLED;
+		return 0;
+	}
+
  	if (oh->_state != _HWMOD_STATE_INITIALIZED&&
  	oh->_state != _HWMOD_STATE_IDLE&&
  	oh->_state != _HWMOD_STATE_DISABLED) {

arch/arm/mach-omap2/omap_hwmod.c static int _enable(struct omap_hwmod *oh) · line 1452

Quoted 4 lines
arch/arm/mach-omap2/omap_hwmod.c +0 -1
@@ -1449,7 +1460,6 @@ static int _enable(struct omap_hwmod *oh)
  		return -EINVAL;
  	}
 
-

Not related to the patch changelog but that make senses to do it, so just update the changelog.

Quoted 20 lines
arch/arm/mach-omap2/omap_hwmod.c +3 -1
  	/*
  	 * If an IP contains only one HW reset line, then de-assert it in order
  	 * to allow the module state transition. Otherwise the PRCM will return

@@ -1744,8 +1754,10 @@ static int _setup(struct omap_hwmod *oh, void *data)
  	 * it should be set by the core code as a runtime flag during startup
  	 */
  	if ((oh->flags&  HWMOD_INIT_NO_IDLE)&&
-	    (postsetup_state == _HWMOD_STATE_IDLE))
+	    (postsetup_state == _HWMOD_STATE_IDLE)) {
+		oh->_state = _HWMOD_STATE_ENABLED_AT_INIT;
  		postsetup_state = _HWMOD_STATE_ENABLED;
+	}
 
  	if (postsetup_state == _HWMOD_STATE_IDLE)
  		_idle(oh);
arch/arm/plat-omap/include/plat/omap_hwmod.h +0 -0
index 1b81dfb..cd49d60 100644

arch/arm/plat-omap/include/plat/omap_hwmod.h struct omap_hwmod_omap4_prcm { · line 439

Quoted 4 lines
arch/arm/plat-omap/include/plat/omap_hwmod.h +1 -0
@@ -436,6 +436,7 @@ struct omap_hwmod_omap4_prcm {
  #define _HWMOD_STATE_ENABLED			4
  #define _HWMOD_STATE_IDLE			5
  #define _HWMOD_STATE_DISABLED			6
+#define _HWMOD_STATE_ENABLED_AT_INIT		7

You should update the comment before... yeah I know, the other states are not documented, but they are much more understandable than this new one.

arch/arm/plat-omap/include/plat/omap_hwmod.h struct omap_hwmod_omap4_prcm { · line 442

Quoted 2 lines
arch/arm/plat-omap/include/plat/omap_hwmod.h +0 -0
  /**
   * struct omap_hwmod_class - the type of an IP block

+1 Looks good to me but someone else must approve... Oops, sorry its not Gerrit :-)

Acked-by Benoit Cousson <[email protected]>

Thanks,
Benoit

author
#2 lore raw

PATCH 1/1 ARM: omap_hwmod: Add a new state to handle hwmods left enabled at init

Quoted 35 lines · Benoit

+ Paul

Quoted 4 lines · Rajendra Nayak

A hwmod with a 'HWMOD_INIT_NO_IDLE' flag set, is left in enabled state by the hwmod framework post the initial setup. Once a real user of the device (a driver) tries to enable it at a later point, the hmwod framework throws a WARN() about

typo: -------------------^

Quoted 25 lines

the device being already in enabled state.

Fix this by introducing a new state '_HWMOD_STATE_ENABLED_AT_INIT' to identify such devices/hwmods, so nothing but just a state change to '_HWMOD_STATE_ENABLED' can be done when the device/hwmod is requested to be enabled (the first time) by its driver/user.

A good example of a such a device is an UART used as debug console. The UART module needs to be kept enabled through the boot, until the UART driver takes control of it, for debug prints to appear on the console.

Signed-off-by Rajendra Nayak<[email protected]>

---
arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++++++++-- arch/arm/plat-omap/include/plat/omap_hwmod.h | 1 +

2 files changed, 15 insertions(+), 2 deletions(-)
arch/arm/mach-omap2/omap_hwmod.c +0 -0
index d7f4623..7d94cc3 100644
@@ -1441,6 +1441,17 @@ static int _enable(struct omap_hwmod *oh)
Quoted 6 lines

arch/arm/mach-omap2/omap_hwmod.c static int _enable(struct omap_hwmod *oh) · line 1445

Quoted 4 lines
arch/arm/mach-omap2/omap_hwmod.c +2 -0
 pr_debug("omap_hwmod: %s: enabling\n", oh->name);
 
+ /*
+ * hwmods' with HWMOD_INIT_NO_IDLE flag set, are left

Is the ' needed here? But I'm not an expert in English grammar...

Me neither, lets ask Paul :)

Quoted 17 lines
Quoted 17 lines
arch/arm/mach-omap2/omap_hwmod.c +9 -0
+ * in enabled state at init.
+ * Now that someone is really trying to enable them,
+ * just update the state.
+ */
+ if (oh->_state == _HWMOD_STATE_ENABLED_AT_INIT) {
+ oh->_state = _HWMOD_STATE_ENABLED;
+ return 0;
+ }
+

if (oh->_state != _HWMOD_STATE_INITIALIZED&&
oh->_state != _HWMOD_STATE_IDLE&&
oh->_state != _HWMOD_STATE_DISABLED) {

arch/arm/mach-omap2/omap_hwmod.c +0 -0
@@ -1449,7 +1460,6 @@ static int _enable(struct omap_hwmod *oh)
 return -EINVAL;
 }
Quoted 4 lines

arch/arm/mach-omap2/omap_hwmod.c static int _enable(struct omap_hwmod *oh) · line 1452

Quoted 1 line
arch/arm/mach-omap2/omap_hwmod.c +0 -1
-

Not related to the patch changelog but that make senses to do it, so just update the changelog.

No, its just a stay change. I'll get rid of it.

Quoted 15 lines
Quoted 15 lines
arch/arm/mach-omap2/omap_hwmod.c +0 -0
 /*
 * If an IP contains only one HW reset line, then de-assert it in order
 * to allow the module state transition. Otherwise the PRCM will return

@@ -1744,8 +1754,10 @@ static int _setup(struct omap_hwmod *oh, void
 *data)
 * it should be set by the core code as a runtime flag during startup
 */

if ((oh->flags& HWMOD_INIT_NO_IDLE)&&
- (postsetup_state == _HWMOD_STATE_IDLE))
+ (postsetup_state == _HWMOD_STATE_IDLE)) {
+ oh->_state = _HWMOD_STATE_ENABLED_AT_INIT;
postsetup_state = _HWMOD_STATE_ENABLED;
+ }

Quoted 16 lines

arch/arm/plat-omap/include/plat/omap_hwmod.h struct omap_hwmod_omap4_prcm { · line 439

Quoted 12 lines

if (postsetup_state == _HWMOD_STATE_IDLE)
_idle(oh);

arch/arm/plat-omap/include/plat/omap_hwmod.h +1 -0
index 1b81dfb..cd49d60 100644
@@ -436,6 +436,7 @@ struct omap_hwmod_omap4_prcm {
 #define _HWMOD_STATE_ENABLED 4
 #define _HWMOD_STATE_IDLE 5
 #define _HWMOD_STATE_DISABLED 6
+#define _HWMOD_STATE_ENABLED_AT_INIT 7

You should update the comment before... yeah I know, the other states are not documented, but they are much more understandable than this new one.

Yeah, makes sense. Will add comments.

Quoted 11 lines

arch/arm/plat-omap/include/plat/omap_hwmod.h struct omap_hwmod_omap4_prcm { · line 442

Quoted 3 lines
arch/arm/plat-omap/include/plat/omap_hwmod.h +0 -0
 /**
 * struct omap_hwmod_class - the type of an IP block

+1 Looks good to me but someone else must approve... Oops, sorry its not Gerrit :-)

Acked-by Benoit Cousson <[email protected]>

Thanks,
Benoit

#3 lore raw

PATCH 1/1 ARM: omap_hwmod: Add a new state to handle hwmods left enabled at init

In reply to Rajendra Nayak
Quoted 11 lines · Rajendra Nayak

A hwmod with a 'HWMOD_INIT_NO_IDLE' flag set, is left in enabled state by the hwmod framework post the initial setup. Once a real user of the device (a driver) tries to enable it at a later point, the hmwod framework throws a WARN() about the device being already in enabled state.

Fix this by introducing a new state '_HWMOD_STATE_ENABLED_AT_INIT' to identify such devices/hwmods, so nothing but just a state change to '_HWMOD_STATE_ENABLED' can be done when the device/hwmod is requested to be enabled (the first time) by its driver/user.

Quoted 6 lines

A good example of a such a device is an UART used as debug console. The UART module needs to be kept enabled through the boot, until the UART driver takes control of it, for debug prints to appear on the console.

Signed-off-by Rajendra Nayak <[email protected]>

tested with uart runtime v8 patch series works fine.
helps to simply the serial.c code without using custom console activate funcs. thanks for handling this within the framework.

FWIW,

Tested-by Govindraj.R <[email protected]>

--
Thanks,
Govindraj.R

Quoted 67 lines

---
?arch/arm/mach-omap2/omap_hwmod.c ? ? ? ? ? ? | ? 16 ++++++++++++++-- ?arch/arm/plat-omap/include/plat/omap_hwmod.h | ? ?1 +
?2 files changed, 15 insertions(+), 2 deletions(-)

arch/arm/mach-omap2/omap_hwmod.c +11 -0
index d7f4623..7d94cc3 100644
@@ -1441,6 +1441,17 @@ static int _enable(struct omap_hwmod *oh)
 
 ? ? ? ?pr_debug("omap_hwmod: %s: enabling\n", oh->name);
 
+ ? ? ? /*
+ ? ? ? ?* hwmods' with HWMOD_INIT_NO_IDLE flag set, are left
+ ? ? ? ?* in enabled state at init.
+ ? ? ? ?* Now that someone is really trying to enable them,
+ ? ? ? ?* just update the state.
+ ? ? ? ?*/
+ ? ? ? if (oh->_state == _HWMOD_STATE_ENABLED_AT_INIT) {
+ ? ? ? ? ? ? ? oh->_state = _HWMOD_STATE_ENABLED;
+ ? ? ? ? ? ? ? return 0;
+ ? ? ? }
+

? ? ? ?if (oh->_state != _HWMOD_STATE_INITIALIZED && ? ? ? ? ? ?oh->_state != _HWMOD_STATE_IDLE && ? ? ? ? ? ?oh->_state != _HWMOD_STATE_DISABLED) {

arch/arm/mach-omap2/omap_hwmod.c +0 -1
@@ -1449,7 +1460,6 @@ static int _enable(struct omap_hwmod *oh)
 ? ? ? ? ? ? ? ?return -EINVAL;
 ? ? ? ?}
 
-

? ? ? ?/*
? ? ? ? * If an IP contains only one HW reset line, then de-assert it in order ? ? ? ? * to allow the module state transition. Otherwise the PRCM will return

arch/arm/mach-omap2/omap_hwmod.c +0 -0
@@ -1744,8 +1754,10 @@ static int _setup(struct omap_hwmod *oh, void *data)

? ? ? ? * it should be set by the core code as a runtime flag during startup ? ? ? ? */
? ? ? ?if ((oh->flags & HWMOD_INIT_NO_IDLE) &&
- ? ? ? ? ? (postsetup_state == _HWMOD_STATE_IDLE))
+ ? ? ? ? ? (postsetup_state == _HWMOD_STATE_IDLE)) {
+ ? ? ? ? ? ? ? oh->_state = _HWMOD_STATE_ENABLED_AT_INIT;
? ? ? ? ? ? ? ?postsetup_state = _HWMOD_STATE_ENABLED;
+ ? ? ? }

? ? ? ?if (postsetup_state == _HWMOD_STATE_IDLE)
? ? ? ? ? ? ? ?_idle(oh);

arch/arm/plat-omap/include/plat/omap_hwmod.h +0 -0
index 1b81dfb..cd49d60 100644
@@ -436,6 +436,7 @@ struct omap_hwmod_omap4_prcm {

?#define _HWMOD_STATE_ENABLED ? ? ? ? ? ? ? ? ? 4
?#define _HWMOD_STATE_IDLE ? ? ? ? ? ? ? ? ? ? ?5
?#define _HWMOD_STATE_DISABLED ? ? ? ? ? ? ? ? ?6
+#define _HWMOD_STATE_ENABLED_AT_INIT ? ? ? ? ? 7

?/**
?* struct omap_hwmod_class - the type of an IP block --
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html

#4 lore raw

PATCH 1/1 ARM: omap_hwmod: Add a new state to handle hwmods left enabled at init

In reply to Rajendra Nayak
Quoted 11 lines · Rajendra Nayak

A hwmod with a 'HWMOD_INIT_NO_IDLE' flag set, is left in enabled state by the hwmod framework post the initial setup. Once a real user of the device (a driver) tries to enable it at a later point, the hmwod framework throws a WARN() about the device being already in enabled state.

Fix this by introducing a new state '_HWMOD_STATE_ENABLED_AT_INIT' to identify such devices/hwmods, so nothing but just a state change to '_HWMOD_STATE_ENABLED' can be done when the device/hwmod is requested to be enabled (the first time) by its driver/user.

Quoted 4 lines

A good example of a such a device is an UART used as debug console. The UART module needs to be kept enabled through the boot, until the UART driver takes control of it, for debug prints to appear on the console.

Nice. This is indeed much cleaner than what we're doing in the UART code. However...

Quoted 11 lines
Signed-off-by Rajendra Nayak <[email protected]>
---
 arch/arm/mach-omap2/omap_hwmod.c             |   16 ++++++++++++++--
 arch/arm/plat-omap/include/plat/omap_hwmod.h |    1 +
 2 files changed, 15 insertions(+), 2 deletions(-)
arch/arm/mach-omap2/omap_hwmod.c +0 -0
index d7f4623..7d94cc3 100644

arch/arm/mach-omap2/omap_hwmod.c static int _enable(struct omap_hwmod *oh) · line 1445

Quoted 5 lines
arch/arm/mach-omap2/omap_hwmod.c +2 -0
@@ -1441,6 +1441,17 @@ static int _enable(struct omap_hwmod *oh)
 
 	pr_debug("omap_hwmod: %s: enabling\n", oh->name);
 
+	/*
+	 * hwmods' with HWMOD_INIT_NO_IDLE flag set, are left

(no the ' isn't necessary)

arch/arm/mach-omap2/omap_hwmod.c static int _enable(struct omap_hwmod *oh) · line 1453

Quoted 8 lines
arch/arm/mach-omap2/omap_hwmod.c +8 -0
+	 * in enabled state at init.
+	 * Now that someone is really trying to enable them,
+	 * just update the state.
+	 */
+	if (oh->_state == _HWMOD_STATE_ENABLED_AT_INIT) {
+		oh->_state = _HWMOD_STATE_ENABLED;
+		return 0;
+	}

...this subtly changes the behavior, at least compared to how the UART code handles this today.

One thing that this doesn't do that the current UART code does is ensure that the IP is actually in a state that can properly idle after this is done.

For example, if the bootloader is dumb (most are) and has configured the UARTs in mode that prevents idle (e.g. no-idle mode), then these UARTs will never allow the SoC to hit low power states.

So, what's really needed is not just a return here, but an _idle() and then continue so we know that the HW is in a state that we know can idle from here on out.

Kevin

author
#5 lore raw

PATCH 1/1 ARM: omap_hwmod: Add a new state to handle hwmods left enabled at init

Hi Kevin,

Quoted 56 lines · Kevin Hilman
Quoted 11 lines · Rajendra Nayak

A hwmod with a 'HWMOD_INIT_NO_IDLE' flag set, is left in enabled state by the hwmod framework post the initial setup. Once a real user of the device (a driver) tries to enable it at a later point, the hmwod framework throws a WARN() about the device being already in enabled state.

Fix this by introducing a new state '_HWMOD_STATE_ENABLED_AT_INIT' to identify such devices/hwmods, so nothing but just a state change to '_HWMOD_STATE_ENABLED' can be done when the device/hwmod is requested to be enabled (the first time) by its driver/user.

Quoted 4 lines

A good example of a such a device is an UART used as debug console. The UART module needs to be kept enabled through the boot, until the UART driver takes control of it, for debug prints to appear on the console.

Nice. This is indeed much cleaner than what we're doing in the UART code. However...

Quoted 11 lines
Signed-off-by Rajendra Nayak<[email protected]>
---
  arch/arm/mach-omap2/omap_hwmod.c             |   16 ++++++++++++++--
  arch/arm/plat-omap/include/plat/omap_hwmod.h |    1 +
  2 files changed, 15 insertions(+), 2 deletions(-)
arch/arm/mach-omap2/omap_hwmod.c +0 -0
index d7f4623..7d94cc3 100644

arch/arm/mach-omap2/omap_hwmod.c static int _enable(struct omap_hwmod *oh) · line 1445

Quoted 5 lines
arch/arm/mach-omap2/omap_hwmod.c +2 -0
@@ -1441,6 +1441,17 @@ static int _enable(struct omap_hwmod *oh)
 
  	pr_debug("omap_hwmod: %s: enabling\n", oh->name);
 
+	/*
+	 * hwmods' with HWMOD_INIT_NO_IDLE flag set, are left

(no the ' isn't necessary)

arch/arm/mach-omap2/omap_hwmod.c static int _enable(struct omap_hwmod *oh) · line 1453

Quoted 8 lines
arch/arm/mach-omap2/omap_hwmod.c +8 -0
+	 * in enabled state at init.
+	 * Now that someone is really trying to enable them,
+	 * just update the state.
+	 */
+	if (oh->_state == _HWMOD_STATE_ENABLED_AT_INIT) {
+		oh->_state = _HWMOD_STATE_ENABLED;
+		return 0;
+	}

...this subtly changes the behavior, at least compared to how the UART code handles this today.

One thing that this doesn't do that the current UART code does is ensure that the IP is actually in a state that can properly idle after this is done.

Quoted 7 lines

For example, if the bootloader is dumb (most are) and has configured the UARTs in mode that prevents idle (e.g. no-idle mode), then these UARTs will never allow the SoC to hit low power states.

So, what's really needed is not just a return here, but an _idle() and then continue so we know that the HW is in a state that we know can idle from here on out.

I don't know if I completely understand what you are saying. The above change is in the _enable() call which would be triggered as part of runtime call from the serial driver to enable the module. (Since the module is already enabled, you just change state to suggest its now enabled by its user, rather than 'left enabled' at init) There would be another call from the serial driver to idle it when a corresponding hwmod _idle() call would idle the device.
I don't seem to understand why would a call to _idle() be needed in an _enable() call.

regards,
Rajendra

Quoted 2 lines

Kevin

#6 lore raw

PATCH 1/1 ARM: omap_hwmod: Add a new state to handle hwmods left enabled at init

Hi Kevin and Rajendra,

Quoted 63 lines · Rajendra Nayak

Hi Kevin,

Quoted 60 lines · Kevin Hilman
Quoted 11 lines · Rajendra Nayak

A hwmod with a 'HWMOD_INIT_NO_IDLE' flag set, is left in enabled state by the hwmod framework post the initial setup. Once a real user of the device (a driver) tries to enable it at a later point, the hmwod framework throws a WARN() about the device being already in enabled state.

Fix this by introducing a new state '_HWMOD_STATE_ENABLED_AT_INIT' to identify such devices/hwmods, so nothing but just a state change to '_HWMOD_STATE_ENABLED' can be done when the device/hwmod is requested to be enabled (the first time) by its driver/user.

Quoted 4 lines

A good example of a such a device is an UART used as debug console. The UART module needs to be kept enabled through the boot, until the UART driver takes control of it, for debug prints to appear on the console.

Nice. This is indeed much cleaner than what we're doing in the UART code. However...

Quoted 12 lines
Signed-off-by Rajendra Nayak<[email protected]>

---
arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++++++++-- arch/arm/plat-omap/include/plat/omap_hwmod.h | 1 +

2 files changed, 15 insertions(+), 2 deletions(-)
arch/arm/mach-omap2/omap_hwmod.c +0 -0
index d7f4623..7d94cc3 100644

arch/arm/mach-omap2/omap_hwmod.c static int _enable(struct omap_hwmod *oh) · line 1445

Quoted 5 lines
arch/arm/mach-omap2/omap_hwmod.c +2 -0
@@ -1441,6 +1441,17 @@ static int _enable(struct omap_hwmod *oh)
 
 pr_debug("omap_hwmod: %s: enabling\n", oh->name);
 
+ /*
+ * hwmods' with HWMOD_INIT_NO_IDLE flag set, are left

(no the ' isn't necessary)

arch/arm/mach-omap2/omap_hwmod.c static int _enable(struct omap_hwmod *oh) · line 1453

Quoted 8 lines
arch/arm/mach-omap2/omap_hwmod.c +8 -0
+ * in enabled state at init.
+ * Now that someone is really trying to enable them,
+ * just update the state.
+ */
+ if (oh->_state == _HWMOD_STATE_ENABLED_AT_INIT) {
+ oh->_state = _HWMOD_STATE_ENABLED;
+ return 0;
+ }

...this subtly changes the behavior, at least compared to how the UART code handles this today.

One thing that this doesn't do that the current UART code does is ensure that the IP is actually in a state that can properly idle after this is done.

For example, if the bootloader is dumb (most are) and has configured the UARTs in mode that prevents idle (e.g. no-idle mode), then these UARTs will never allow the SoC to hit low power states.

That will not happen because we already did the complete enable sequence during setup phase that will set the sysconfig properly.
So the device is properly initialized, we just skipped the idle phase because we wanted to keep the device operational for the console.

Quoted 3 lines
Quoted 3 lines

So, what's really needed is not just a return here, but an _idle() and then continue so we know that the HW is in a state that we know can idle from here on out.

We are in the enable call, so the driver / omap_device does expect the IP to be operational at the moment, we cannot put it into idle.

Quoted 9 lines

I don't know if I completely understand what you are saying. The above change is in the _enable() call which would be triggered as part of runtime call from the serial driver to enable the module. (Since the module is already enabled, you just change state to suggest its now enabled by its user, rather than 'left enabled' at init) There would be another call from the serial driver to idle it when a corresponding hwmod _idle() call would idle the device.
I don't seem to understand why would a call to _idle() be needed in an _enable() call.

Yep, I agree. I think Kevin's point is due to the previous sequence used by the uart:

+ omap_hwmod_idle(od->hwmods[0]);
+ omap_hwmod_enable(od->hwmods[0]);

But that sequence is not needed for my point of view. The device is already properly enabled. I don't think it was even needed in the previous code. The hwmod was already enabled properly, only the omap_device was disabled. So the original omap_console_hwmod_enable should just do nothing.

Regards,
Benoit

#7 lore raw

PATCH 1/1 ARM: omap_hwmod: Add a new state to handle hwmods left enabled at init

Quoted 63 lines · Benoit

Hi Kevin and Rajendra,

Quoted 60 lines · Rajendra Nayak

Hi Kevin,

Quoted 57 lines · Kevin Hilman
Quoted 11 lines · Rajendra Nayak

A hwmod with a 'HWMOD_INIT_NO_IDLE' flag set, is left in enabled state by the hwmod framework post the initial setup. Once a real user of the device (a driver) tries to enable it at a later point, the hmwod framework throws a WARN() about the device being already in enabled state.

Fix this by introducing a new state '_HWMOD_STATE_ENABLED_AT_INIT' to identify such devices/hwmods, so nothing but just a state change to '_HWMOD_STATE_ENABLED' can be done when the device/hwmod is requested to be enabled (the first time) by its driver/user.

Quoted 4 lines

A good example of a such a device is an UART used as debug console. The UART module needs to be kept enabled through the boot, until the UART driver takes control of it, for debug prints to appear on the console.

Nice. This is indeed much cleaner than what we're doing in the UART code. However...

Quoted 12 lines
Signed-off-by Rajendra Nayak<[email protected]>

---
arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++++++++-- arch/arm/plat-omap/include/plat/omap_hwmod.h | 1 +

2 files changed, 15 insertions(+), 2 deletions(-)
arch/arm/mach-omap2/omap_hwmod.c +0 -0
index d7f4623..7d94cc3 100644

arch/arm/mach-omap2/omap_hwmod.c static int _enable(struct omap_hwmod *oh) · line 1445

Quoted 5 lines
arch/arm/mach-omap2/omap_hwmod.c +2 -0
@@ -1441,6 +1441,17 @@ static int _enable(struct omap_hwmod *oh)
 
 pr_debug("omap_hwmod: %s: enabling\n", oh->name);
 
+ /*
+ * hwmods' with HWMOD_INIT_NO_IDLE flag set, are left

(no the ' isn't necessary)

arch/arm/mach-omap2/omap_hwmod.c static int _enable(struct omap_hwmod *oh) · line 1453

Quoted 8 lines
arch/arm/mach-omap2/omap_hwmod.c +8 -0
+ * in enabled state at init.
+ * Now that someone is really trying to enable them,
+ * just update the state.
+ */
+ if (oh->_state == _HWMOD_STATE_ENABLED_AT_INIT) {
+ oh->_state = _HWMOD_STATE_ENABLED;
+ return 0;
+ }

...this subtly changes the behavior, at least compared to how the UART code handles this today.

One thing that this doesn't do that the current UART code does is ensure that the IP is actually in a state that can properly idle after this is done.

Quoted 8 lines
Quoted 3 lines
Quoted 3 lines

For example, if the bootloader is dumb (most are) and has configured the UARTs in mode that prevents idle (e.g. no-idle mode), then these UARTs will never allow the SoC to hit low power states.

That will not happen because we already did the complete enable sequence during setup phase that will set the sysconfig properly. So the device is properly initialized, we just skipped the idle phase because we wanted to keep the device operational for the console.

Ah, perfect.

Quoted 23 lines
Quoted 3 lines
Quoted 3 lines

So, what's really needed is not just a return here, but an _idle() and then continue so we know that the HW is in a state that we know can idle from here on out.

We are in the enable call, so the driver / omap_device does expect the IP to be operational at the moment, we cannot put it into idle.

Quoted 9 lines

I don't know if I completely understand what you are saying. The above change is in the _enable() call which would be triggered as part of runtime call from the serial driver to enable the module. (Since the module is already enabled, you just change state to suggest its now enabled by its user, rather than 'left enabled' at init) There would be another call from the serial driver to idle it when a corresponding hwmod _idle() call would idle the device.
I don't seem to understand why would a call to _idle() be needed in an _enable() call.

Yep, I agree. I think Kevin's point is due to the previous sequence used by the uart:

(quoted diff) +2 -0
+	omap_hwmod_idle(od->hwmods[0]);
+	omap_hwmod_enable(od->hwmods[0]);
Quoted 5 lines

But that sequence is not needed for my point of view. The device is already properly enabled. I don't think it was even needed in the previous code. The hwmod was already enabled properly, only the omap_device was disabled. So the original omap_console_hwmod_enable should just do nothing.

OK, thanks for the clarification.

In that case, this patch

Acked-by Kevin Hilman <[email protected]>

Kevin