arm: omap3: am35x: Powerdomain, EMIF4, etc. fixups

From: "Mark A. Greer" <[email protected]>

Currently in the OMAP3 code, cpu_idle() calls pm_idle(), which is a function pointer set to omap3_pm_idle()). omap3_pm_idle() calls omap_sram_idle() which eventually causes a 'wfi' instruction to be executed effectively putting the system to sleep. It is assumed that an I/O wake-up event will occur to wake the system up again. This doesn't work on systems that don't support I/O wake-ups (indicated by omap3_has_io_wakeup() returning false).

Leaving pm_idle() pointing to default_idle() won't work either because the cpu_<processor>_do_idle() routine which is eventually called may also execute a 'wfi' instruction (e.g., cpu_v7_do_idle()).

To handle this, leave pm_idle() pointing to omap3_pm_idle() but add a check in omap3_pm_idle() so it avoids calling omap_sram_idle() when I/O wake-ups aren't supported.

Signed-off-by Mark A. Greer <[email protected]>

From: "Mark A. Greer" <[email protected]>

Currently in the OMAP3 code, cpu_idle() calls pm_idle(), which is a function pointer set to omap3_pm_idle()). omap3_pm_idle() calls omap_sram_idle() which eventually causes a 'wfi' instruction to be executed effectively putting the system to sleep. It is assumed that an I/O wake-up event will occur to wake the system up again. This doesn't work on systems that don't support I/O wake-ups (indicated by omap3_has_io_wakeup() returning false).

Leaving pm_idle() pointing to default_idle() won't work either because the cpu_<processor>_do_idle() routine which is eventually called may also execute a 'wfi' instruction (e.g., cpu_v7_do_idle()).

Hi Mark,

Quoted 11 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

Currently in the OMAP3 code, cpu_idle() calls pm_idle(), which is a function pointer set to omap3_pm_idle()). omap3_pm_idle() calls omap_sram_idle() which eventually causes a 'wfi' instruction to be executed effectively putting the system to sleep. It is assumed that an I/O wake-up event will occur to wake the system up again. This doesn't work on systems that don't support I/O wake-ups (indicated by omap3_has_io_wakeup() returning false).

Quoted 4 lines

Leaving pm_idle() pointing to default_idle() won't work either because the cpu_<processor>_do_idle() routine which is eventually called may also execute a 'wfi' instruction (e.g., cpu_v7_do_idle()).

So with this change you will never execute wfi?

I would have thought a timer or peripheral interrupt would be able to bring you out of wfi.

IO wake-ups would only be needed for very low
power states. At least that is how it is on OMAP devices. I am not sure how the AMxxx devices differ.

From: "Mark A. Greer" <[email protected]>

Currently, the OMAP3 cpuidle driver calls omap3_enter_idle() which calls omap_sram_idle(). omap_sram_idle() eventually causes a 'wfi' instruction to be executed effectively putting the system to sleep. It is assumed that an I/O wake-up event will occur to wake the system up again. This doesn't work on systems that don't support I/O wake-ups (indicated by omap3_has_io_wakeup() returning false).

To handle this, follow the same path in omap3_enter_idle() that would be followed if an interrupt were pending.

Signed-off-by Mark A. Greer <[email protected]>

---
 arch/arm/mach-omap2/cpuidle34xx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/mach-omap2/cpuidle34xx.c +1 -1

index 5358664..12cf81d 100644
@@ -100,7 +100,7 @@ static int __omap3_enter_idle(struct cpuidle_device *dev,
 	pwrdm_set_next_pwrst(mpu_pd, mpu_state);
 	pwrdm_set_next_pwrst(core_pd, core_state);
 
-	if (omap_irq_pending() || need_resched())
+	if (omap_irq_pending() || need_resched() || !omap3_has_io_wakeup())
 		goto return_sleep_time;
 
 	/* Deny idle for C1 */

Signature · 1 line

1.7.9.4

Quoted 10 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

Currently, the OMAP3 cpuidle driver calls omap3_enter_idle() which calls omap_sram_idle(). omap_sram_idle() eventually causes a 'wfi' instruction to be executed effectively putting the system to sleep. It is assumed that an I/O wake-up event will occur to wake the system up again. This doesn't work on systems that don't support I/O wake-ups (indicated by omap3_has_io_wakeup() returning false).

Quoted 2 lines

To handle this, follow the same path in omap3_enter_idle() that would be followed if an interrupt were pending.

I don't quite understand this patch. Are you saying that AM3517/3505 can't wake from WFI? That would seem odd.

There are other sources of wakeup on the system other than I/O wakeup. I/O wakeup only applies to wakeups from the I/O pads when the chip is in RETENTION or OFF. And as I understand it, neither of those apply to AM3517/3505?

Even if I/O wakeups aren't supported, many of the IP blocks on the system should be able to cause the ARM to exit WFI by asserting their SWAKEUP lines and raising their interrupt lines.

So this change doesn't seem quite right to me...

More broadly, if AM3517/3505 only supports powerdomains ON, then you should probably use your own CPUIdle driver that doesn't touch the powerdomain states at all.

Quoted 2 lines · Paul Walmsley

I don't quite understand this patch. Are you saying that AM3517/3505 can't wake from WFI? That would seem odd.

No, I'm saying that I/O doesn't seem to wake it up from the WFI. I've learned to not trust the am35x TRM much so I'm pretty much feeling my way along in the dark.

I do know that without this patch, the system is extremely slow which I believe is from it only returning from the WFI because of a timer expiration or something like that.

Quoted 4 lines

There are other sources of wakeup on the system other than I/O wakeup. I/O wakeup only applies to wakeups from the I/O pads when the chip is in RETENTION or OFF. And as I understand it, neither of those apply to AM3517/3505?

Hmm, its true that RETENTION and OFF aren't supported. It does wake up occasionally (as mentioned above) it just seems that I/O doesn't wake it up. I may be mistaken. I'm only going from what I've seen since and the TRM seems to have lots of errors so I'm not sure what to trust in it.

Quoted 3 lines

Even if I/O wakeups aren't supported, many of the IP blocks on the system should be able to cause the ARM to exit WFI by asserting their SWAKEUP lines and raising their interrupt lines.

Okay, but that doesn't seem to be working. I'll look some more to see why they aren't working.

cc Govindraj

Quoted 8 lines · Mark A. Greer

Quoted 2 lines · Paul Walmsley

I don't quite understand this patch. Are you saying that AM3517/3505 can't wake from WFI? That would seem odd.

No, I'm saying that I/O doesn't seem to wake it up from the WFI. I've learned to not trust the am35x TRM much so I'm pretty much feeling my way along in the dark.

Hehe, no worries, just curious.

Quoted 3 lines

I do know that without this patch, the system is extremely slow which I believe is from it only returning from the WFI because of a timer expiration or something like that.

How are you trying to wake it up -- from an incoming character via the UART?

Does the system pause in the middle of UART transmits, or does it get the transmit buffers out cleanly and just not respond promptly to incoming characters?

Quoted 5 lines

Quoted 4 lines

There are other sources of wakeup on the system other than I/O wakeup. I/O wakeup only applies to wakeups from the I/O pads when the chip is in RETENTION or OFF. And as I understand it, neither of those apply to AM3517/3505?

Quoted 4 lines

Hmm, its true that RETENTION and OFF aren't supported. It does wake up occasionally (as mentioned above) it just seems that I/O doesn't wake it up. I may be mistaken. I'm only going from what I've seen since and the TRM seems to have lots of errors so I'm not sure what to trust in it.

OK no problem, just trying to understand what's going on. Sounds like you've gotten tossed into the deep end :-)

Quoted 6 lines

Quoted 3 lines

Even if I/O wakeups aren't supported, many of the IP blocks on the system should be able to cause the ARM to exit WFI by asserting their SWAKEUP lines and raising their interrupt lines.

Okay, but that doesn't seem to be working. I'll look some more to see why they aren't working.

There could be a hidden dependency on I/O wakeup being present in the UART driver. We've been going through some UART driver angst recently...

Hi

Quoted 11 lines

From: "Mark A. Greer" <[email protected]>

Currently, the OMAP3 cpuidle driver calls omap3_enter_idle() which calls omap_sram_idle(). omap_sram_idle() eventually causes a 'wfi' instruction to be executed effectively putting the system to sleep. It is assumed that an I/O wake-up event will occur to wake the system up again. This doesn't work on systems that don't support I/O wake-ups (indicated by omap3_has_io_wakeup() returning false).

Quoted 2 lines

To handle this, follow the same path in omap3_enter_idle() that would be followed if an interrupt were pending.

I don't quite understand this patch. Are you saying that AM3517/3505 can't wake from WFI? That would seem odd.

There are other sources of wakeup on the system other than I/O wakeup. I/O wakeup only applies to wakeups from the I/O pads when the chip is in RETENTION or OFF. And as I understand it, neither of those apply to AM3517/3505?

Even if I/O wakeups aren't supported, many of the IP blocks on the system should be able to cause the ARM to exit WFI by asserting their SWAKEUP lines and raising their interrupt lines.

Quoted 1 line · Paul Walmsley

Hi

Hi Paul, Kevin.

Quoted 18 lines · Mark A. Greer

Quoted 11 lines

From: "Mark A. Greer" <[email protected]>

Currently, the OMAP3 cpuidle driver calls omap3_enter_idle() which calls omap_sram_idle(). omap_sram_idle() eventually causes a 'wfi' instruction to be executed effectively putting the system to sleep. It is assumed that an I/O wake-up event will occur to wake the system up again. This doesn't work on systems that don't support I/O wake-ups (indicated by omap3_has_io_wakeup() returning false).

Quoted 2 lines

To handle this, follow the same path in omap3_enter_idle() that would be followed if an interrupt were pending.

I don't quite understand this patch. Are you saying that AM3517/3505 can't wake from WFI? That would seem odd.

Quoted 8 lines

There are other sources of wakeup on the system other than I/O wakeup. I/O wakeup only applies to wakeups from the I/O pads when the chip is in RETENTION or OFF. And as I understand it, neither of those apply to AM3517/3505?

Even if I/O wakeups aren't supported, many of the IP blocks on the system should be able to cause the ARM to exit WFI by asserting their SWAKEUP lines and raising their interrupt lines.

I have changed the code to keep everything in the ON state and use cpu_do_idle()--basically a wfi, cpu_v7_do_idle() in this case--and everything works well (except networking, see below) when using an mmc-based rootfs.

The issue is with the emac--it can't wake the system up unless there is an irq from the PHY to a wakeup-enabled GPIO (confirmed by h/w engr). So, when using networking or using an nfs-based rootfs, there are all kinds of timeouts, etc. Basically, doing a wfi (i.e., pm_idle or cpuidle) and expecting the emac to wake the system back up won't work.

How would you like me to proceed (to avoid the wfi in pm_idle & cpuidle)?

Quoted 36 lines · Mark A. Greer

Quoted 1 line · Paul Walmsley

Hi

Hi Paul, Kevin.

Quoted 18 lines · Mark A. Greer

Quoted 11 lines

From: "Mark A. Greer" <[email protected]>

Currently, the OMAP3 cpuidle driver calls omap3_enter_idle() which calls omap_sram_idle(). omap_sram_idle() eventually causes a 'wfi' instruction to be executed effectively putting the system to sleep. It is assumed that an I/O wake-up event will occur to wake the system up again. This doesn't work on systems that don't support I/O wake-ups (indicated by omap3_has_io_wakeup() returning false).

Quoted 2 lines

To handle this, follow the same path in omap3_enter_idle() that would be followed if an interrupt were pending.

I don't quite understand this patch. Are you saying that AM3517/3505 can't wake from WFI? That would seem odd.

Quoted 8 lines

There are other sources of wakeup on the system other than I/O wakeup. I/O wakeup only applies to wakeups from the I/O pads when the chip is in RETENTION or OFF. And as I understand it, neither of those apply to AM3517/3505?

Even if I/O wakeups aren't supported, many of the IP blocks on the system should be able to cause the ARM to exit WFI by asserting their SWAKEUP lines and raising their interrupt lines.

I have changed the code to keep everything in the ON state and use cpu_do_idle()--basically a wfi, cpu_v7_do_idle() in this case--and everything works well (except networking, see below) when using an mmc-based rootfs.

Quoted 7 lines

The issue is with the emac--it can't wake the system up unless there is an irq from the PHY to a wakeup-enabled GPIO (confirmed by h/w engr). So, when using networking or using an nfs-based rootfs, there are all kinds of timeouts, etc. Basically, doing a wfi (i.e., pm_idle or cpuidle) and expecting the emac to wake the system back up won't work.

How would you like me to proceed (to avoid the wfi in pm_idle & cpuidle)?

Just thinking out loud...

We could chose whether pm_idle & cpuidle issue a wfi based on whether there is a davinci-emac present. The issue with that is that someday there may be another SoC that has a davinci-emac that can wake up the system. I know cpu_is_xxx() is frowned upon but this does seem like a proper usage of it--the deciding factor really is whether its an am35x or not (and CONFIG_TI_DAVINCI_EMAC is enabled).

Quoted 9 lines · Mark A. Greer

On Tue, Apr 24, 2012 at 01:51:05PM -0700, Mark A. Greer wrote:

We could chose whether pm_idle & cpuidle issue a wfi based on whether there is a davinci-emac present. The issue with that is that someday there may be another SoC that has a davinci-emac that can wake up the system. I know cpu_is_xxx() is frowned upon but this does seem like a proper usage of it--the deciding factor really is whether its an am35x or not (and CONFIG_TI_DAVINCI_EMAC is enabled).

The presence of the davinci_emac is probably overkill, avoiding WFI should really only be done when the davinci_emac is *active*. e.g. using an am35x with an EMAC *present*, but not in use because there's an MMC rootfs for example.

If there's a good way to detect an in-use davinci_emac, then disable_hlt() can be used to avoid WFI. When the davinci_emac is not in use (e.g. module unloaded etc.) then enable_hlt() can be used to (re)allow WFI.

Quoted 11 lines · Mark A. Greer

On Tue, Apr 24, 2012 at 01:51:05PM -0700, Mark A. Greer wrote:

Just thinking out loud...

We could chose whether pm_idle & cpuidle issue a wfi based on whether there is a davinci-emac present. The issue with that is that someday there may be another SoC that has a davinci-emac that can wake up the system. I know cpu_is_xxx() is frowned upon but this does seem like a proper usage of it--the deciding factor really is whether its an am35x or not (and CONFIG_TI_DAVINCI_EMAC is enabled).

The presence of the davinci_emac is probably overkill, avoiding WFI should really only be done when the davinci_emac is *active*. e.g. using an am35x with an EMAC *present*, but not in use because there's an MMC rootfs for example.

If there's a good way to detect an in-use davinci_emac, then disable_hlt() can be used to avoid WFI. When the davinci_emac is not in use (e.g. module unloaded etc.) then enable_hlt() can be used to (re)allow WFI.

Quoted 19 lines · Kevin Hilman

Quoted 11 lines · Mark A. Greer

On Tue, Apr 24, 2012 at 01:51:05PM -0700, Mark A. Greer wrote:

Just thinking out loud...

We could chose whether pm_idle & cpuidle issue a wfi based on whether there is a davinci-emac present. The issue with that is that someday there may be another SoC that has a davinci-emac that can wake up the system. I know cpu_is_xxx() is frowned upon but this does seem like a proper usage of it--the deciding factor really is whether its an am35x or not (and CONFIG_TI_DAVINCI_EMAC is enabled).

The presence of the davinci_emac is probably overkill, avoiding WFI should really only be done when the davinci_emac is *active*. e.g. using an am35x with an EMAC *present*, but not in use because there's an MMC rootfs for example.

Quoted 4 lines

If there's a good way to detect an in-use davinci_emac, then disable_hlt() can be used to avoid WFI. When the davinci_emac is not in use (e.g. module unloaded etc.) then enable_hlt() can be used to (re)allow WFI.

This can work for pm_idle but what should be done for cpu_idle (as in, omap3_enter_idle[_bm])? We should have some sort of solution for that so users can safely enable CONFIG_CPU_IDLE and not have the am3517 fall apart.

Quoted 25 lines · Mark A. Greer

Quoted 19 lines · Kevin Hilman

Quoted 11 lines · Mark A. Greer

On Tue, Apr 24, 2012 at 01:51:05PM -0700, Mark A. Greer wrote:

Just thinking out loud...

We could chose whether pm_idle & cpuidle issue a wfi based on whether there is a davinci-emac present. The issue with that is that someday there may be another SoC that has a davinci-emac that can wake up the system. I know cpu_is_xxx() is frowned upon but this does seem like a proper usage of it--the deciding factor really is whether its an am35x or not (and CONFIG_TI_DAVINCI_EMAC is enabled).

The presence of the davinci_emac is probably overkill, avoiding WFI should really only be done when the davinci_emac is *active*. e.g. using an am35x with an EMAC *present*, but not in use because there's an MMC rootfs for example.

Quoted 4 lines

If there's a good way to detect an in-use davinci_emac, then disable_hlt() can be used to avoid WFI. When the davinci_emac is not in use (e.g. module unloaded etc.) then enable_hlt() can be used to (re)allow WFI.

Quoted 4 lines

This can work for pm_idle but what should be done for cpu_idle (as in, omap3_enter_idle[_bm])? We should have some sort of solution for that so users can safely enable CONFIG_CPU_IDLE and not have the am3517 fall apart.

disable_hlt() will prevent either the default idle *or* CPUidle from being called.

It will simply call cpu_relax() instead of calling any idle function. (c.f. cpu_idle() in arch/arm/kernel/process.c)

From: "Mark A. Greer" <[email protected]>

Typical OMAP3 SoCs have four power domain states: ON, INACTIVE, RETENTION, and OFF. The am35x family of SoCs has only two states: ON and INACTIVE. To distinguish which set of states the current device has, add the 'OMAP3_HAS_PWROFF' feature. When that feature/bit is set, the device supports the RETENTION and OFF states; otherwise, it doesn't.

Signed-off-by Mark A. Greer <[email protected]>

Hi Mark,

Quoted 10 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

Typical OMAP3 SoCs have four power domain states: ON, INACTIVE, RETENTION, and OFF. The am35x family of SoCs has only two states: ON and INACTIVE. To distinguish which set of states the current device has, add the 'OMAP3_HAS_PWROFF' feature. When that feature/bit is set, the device supports the RETENTION and OFF states; otherwise, it doesn't.

Signed-off-by Mark A. Greer <[email protected]>

Paul has mentioned this already, but the same applies here: We shouldn't
be using SoC-global feature flag for this. We already have per-pwrdm
flags that indicate what states a given powerdomain suports (see .pwrsts field.)

Wherever we have blind writes to next powerstates that assume support for RET/OFF is present, those should probably use a helper function from the powerdomain code that checks if that state is even supported.

Jean's work on functional powerstates will probably help here if you really need to support INACTIVE. However, Paul may be right in that you might just start with supporing ON only, and validate that module-level wakups acutally work.

Hi Mark,

Quoted 10 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

Typical OMAP3 SoCs have four power domain states: ON, INACTIVE, RETENTION, and OFF. The am35x family of SoCs has only two states: ON and INACTIVE. To distinguish which set of states the current device has, add the 'OMAP3_HAS_PWROFF' feature. When that feature/bit is set, the device supports the RETENTION and OFF states; otherwise, it doesn't.

Signed-off-by Mark A. Greer <[email protected]>

Paul has mentioned this already, but the same applies here: We shouldn't
be using SoC-global feature flag for this. We already have per-pwrdm
flags that indicate what states a given powerdomain suports (see .pwrsts field.)

Wherever we have blind writes to next powerstates that assume support for RET/OFF is present, those should probably use a helper function from the powerdomain code that checks if that state is even supported.

Jean's work on functional powerstates will probably help here if you really need to support INACTIVE. However, Paul may be right in that you might just start with supporing ON only, and validate that module-level wakups acutally work.

From 32c54adb15c76396aeec809d38de4dde936b1e66 Mon Sep 17 00:00:00 2001

Quoted 1 line · Kevin Hilman

Hi Mark,

Hi Kevin.

Quoted 18 lines

Quoted 10 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

Typical OMAP3 SoCs have four power domain states: ON, INACTIVE, RETENTION, and OFF. The am35x family of SoCs has only two states: ON and INACTIVE. To distinguish which set of states the current device has, add the 'OMAP3_HAS_PWROFF' feature. When that feature/bit is set, the device supports the RETENTION and OFF states; otherwise, it doesn't.

Signed-off-by Mark A. Greer <[email protected]>

Paul has mentioned this already, but the same applies here: We shouldn't
be using SoC-global feature flag for this. We already have per-pwrdm
flags that indicate what states a given powerdomain suports (see .pwrsts field.)

Quoted 3 lines

Wherever we have blind writes to next powerstates that assume support for RET/OFF is present, those should probably use a helper function from the powerdomain code that checks if that state is even supported.

How about something like the patch below?
Note: its not well tested; just RFC.

Quoted 4 lines

Jean's work on functional powerstates will probably help here if you really need to support INACTIVE. However, Paul may be right in that you might just start with supporing ON only, and validate that module-level wakups acutally work.

Yes, I think INACTIVE is a red herring so I'm going to stick with k.o master branch for now (IOW, not base on Jean's patches). If you still want me to base on his patches, just let me know.

Mark
--

From 32c54adb15c76396aeec809d38de4dde936b1e66 Mon Sep 17 00:00:00 2001
From: "Mark A. Greer" <[email protected]>
Date: Mon, 23 Apr 2012 17:48:06 -0700
Subject: [PATCH] arm: omap: Use only valid power domain states

Some parts of the OMAP code assume that all power
domains support certain states (e.g., RET & OFF).
This isn't always true (e.g., the am35x family of
SoC's) so those assumptions need to be removed.

Remove those assumptions by looking up the deepest state that a power domain can be in whereever its being blindly set. The 'max()' of the deepest state and what the code formerly wanted to set it to, is the correct state. If the code formerly wanted to set it to PWRDM_POWER_OFF, then the deepest possible state will be used (i.e., no need to perform the 'max()').

The code still assumes that ON is a valid state.

Signed-off-by Mark A. Greer <[email protected]>

---
 arch/arm/mach-omap2/cpuidle34xx.c |   14 ++++++++++----
 arch/arm/mach-omap2/pm34xx.c      |   15 +++++++++------
 arch/arm/mach-omap2/powerdomain.c |   25 +++++++++++++++++++++++++
 arch/arm/mach-omap2/powerdomain.h |    2 ++
 4 files changed, 46 insertions(+), 10 deletions(-)

arch/arm/mach-omap2/cpuidle34xx.c +10 -4

index 5358664..60aa0c3 100644
@@ -175,19 +175,25 @@ static int next_valid_state(struct cpuidle_device *dev,
 	struct cpuidle_state_usage *curr_usage = &dev->states_usage[index];
 	struct cpuidle_state *curr = &drv->states[index];
 	struct omap3_idle_statedata *cx = cpuidle_get_statedata(curr_usage);
-	u32 mpu_deepest_state = PWRDM_POWER_RET;
-	u32 core_deepest_state = PWRDM_POWER_RET;
+	u32 mpu_deepest_state, mpu_deepest_possible;
+	u32 core_deepest_state, core_deepest_possible;
 	int next_index = -1;
 
+	mpu_deepest_possible = pwrdm_get_deepest_state(mpu_pd);
+	mpu_deepest_state = max(mpu_deepest_possible, (u32)PWRDM_POWER_RET);
+
+	core_deepest_possible = pwrdm_get_deepest_state(core_pd);
+	core_deepest_state = max(core_deepest_possible, (u32)PWRDM_POWER_RET);
+
 	if (enable_off_mode) {
-		mpu_deepest_state = PWRDM_POWER_OFF;
+		mpu_deepest_state = mpu_deepest_possible;
 		/*
 		 * Erratum i583: valable for ES rev < Es1.2 on 3630.
 		 * CORE OFF mode is not supported in a stable form, restrict
 		 * instead the CORE state to RET.
 		 */
 		if (!IS_PM34XX_ERRATUM(PM_SDRC_WAKEUP_ERRATUM_i583))
-			core_deepest_state = PWRDM_POWER_OFF;
+			core_deepest_state = core_deepest_possible;
 	}
 
 	/* Check if current state is valid */

arch/arm/mach-omap2/pm34xx.c +9 -6

index ec92676..7737bfb 100644
@@ -614,12 +614,11 @@ void omap3_pm_off_mode_enable(int enable)
 	struct power_state *pwrst;
 	u32 state;
 
-	if (enable)
-		state = PWRDM_POWER_OFF;
-	else
-		state = PWRDM_POWER_RET;
-
 	list_for_each_entry(pwrst, &pwrst_list, node) {
+		state = pwrdm_get_deepest_state(pwrst->pwrdm);
+		if (!enable)
+			state = max(state, (u32)PWRDM_POWER_RET);
+
 		if (IS_PM34XX_ERRATUM(PM_SDRC_WAKEUP_ERRATUM_i583) &&
 				pwrst->pwrdm == core_pwrdm &&
 				state == PWRDM_POWER_OFF) {

@@ -660,6 +659,7 @@ int omap3_pm_set_suspend_state(struct powerdomain *pwrdm, int state)
 static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
 {
 	struct power_state *pwrst;
+	u32 state;
 
 	if (!pwrdm->pwrsts)
 		return 0;

@@ -668,7 +668,10 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
 	if (!pwrst)
 		return -ENOMEM;
 	pwrst->pwrdm = pwrdm;
-	pwrst->next_state = PWRDM_POWER_RET;
+
+	state = pwrdm_get_deepest_state(pwrdm);
+	pwrst->next_state = max(state, (u32)PWRDM_POWER_RET);
+
 	list_add(&pwrst->node, &pwrst_list);
 
 	if (pwrdm_has_hdwr_sar(pwrdm))

arch/arm/mach-omap2/powerdomain.c +25 -0

index 96ad3dbe..9c80c19 100644
@@ -1078,3 +1078,28 @@ bool pwrdm_can_ever_lose_context(struct powerdomain *pwrdm)
 
 	return 0;
 }
+
+/**
+ * pwrdm_get_deepest_state - Get deepest valid state domain can enter
+ * @pwrdm: struct powerdomain *
+ *
+ * Find and return the deepest valid state a power domain can be in.
+ * Returns the deepest state that @pwrdm can enter.
+ */
+u32 pwrdm_get_deepest_state(struct powerdomain *pwrdm)
+{
+	u32 valid_states, deepest_state;
+
+	valid_states = pwrdm->pwrsts;
+
+	if (valid_states & PWRSTS_OFF)
+		deepest_state = PWRDM_POWER_OFF;
+	else if (valid_states & PWRSTS_RET)
+		deepest_state = PWRDM_POWER_RET;
+	else if (valid_states & PWRSTS_INACTIVE)
+		deepest_state = PWRDM_POWER_INACTIVE;
+	else
+		deepest_state = PWRDM_POWER_ON;
+
+	return deepest_state;
+}

arch/arm/mach-omap2/powerdomain.h +2 -0

index 0d72a8a..9688ff1 100644
@@ -220,6 +220,8 @@ int pwrdm_set_lowpwrstchange(struct powerdomain *pwrdm);
 int pwrdm_get_context_loss_count(struct powerdomain *pwrdm);
 bool pwrdm_can_ever_lose_context(struct powerdomain *pwrdm);
 
+u32 pwrdm_get_deepest_state(struct powerdomain *pwrdm);
+
 extern void omap242x_powerdomains_init(void);
 extern void omap243x_powerdomains_init(void);
 extern void omap3xxx_powerdomains_init(void);

Quoted 23 lines · Mark A. Greer

Quoted 1 line · Kevin Hilman

Hi Mark,

Hi Kevin.

Quoted 18 lines

Quoted 10 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

Typical OMAP3 SoCs have four power domain states: ON, INACTIVE, RETENTION, and OFF. The am35x family of SoCs has only two states: ON and INACTIVE. To distinguish which set of states the current device has, add the 'OMAP3_HAS_PWROFF' feature. When that feature/bit is set, the device supports the RETENTION and OFF states; otherwise, it doesn't.

Signed-off-by Mark A. Greer <[email protected]>

Paul has mentioned this already, but the same applies here: We shouldn't
be using SoC-global feature flag for this. We already have per-pwrdm
flags that indicate what states a given powerdomain suports (see .pwrsts field.)

Quoted 6 lines

Quoted 3 lines

Wherever we have blind writes to next powerstates that assume support for RET/OFF is present, those should probably use a helper function from the powerdomain code that checks if that state is even supported.

How about something like the patch below?
Note: its not well tested; just RFC.

Yes, your proposed patch looks right to me.

I guess it's up to Paul & Jean to see if they'd rather see this build on top of the Jean's functional power state work, or take this as a standalone fix.

Kevin

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs only support PWRDM_POWER_ON and PWRDM_POWER_INACTIVE power states. This causes an issue in some areas of the OMAP3 power-related code because of assumptions that PWRDM_POWER_RET and/or PWRDM_POWER_OFF are always valid states.

To get am35x SoCs to work properly, add missing support for PWRDM_POWER_INACTIVE and remove assumptions that PWRDM_POWER_RET and PWRDM_POWER_OFF are always valid states.

Signed-off-by Mark A. Greer <[email protected]>

---

?arch/arm/mach-omap2/cpuidle34xx.c | ? 29 +++++++++++++++++++++++--
?arch/arm/mach-omap2/pm-debug.c ? ?| ? ?8 +++++--
?arch/arm/mach-omap2/pm34xx.c ? ? ?| ? 43 +++++++++++++++++++++++--------------

?3 files changed, 60 insertions(+), 20 deletions(-)

arch/arm/mach-omap2/cpuidle34xx.c +13 -2

index 12cf81d..187872a 100644
@@ -175,12 +175,23 @@ static int next_valid_state(struct cpuidle_device *dev,
 ? ? ? ?struct cpuidle_state_usage *curr_usage = &dev->states_usage[index];
 ? ? ? ?struct cpuidle_state *curr = &drv->states[index];
 ? ? ? ?struct omap3_idle_statedata *cx = cpuidle_get_statedata(curr_usage);
- ? ? ? u32 mpu_deepest_state = PWRDM_POWER_RET;
- ? ? ? u32 core_deepest_state = PWRDM_POWER_RET;
+ ? ? ? u32 mpu_deepest_state;
+ ? ? ? u32 core_deepest_state;
 ? ? ? ?int next_index = -1;
 
+ ? ? ? if (omap3_has_pwroff()) {
+ ? ? ? ? ? ? ? mpu_deepest_state = PWRDM_POWER_RET;
+ ? ? ? ? ? ? ? core_deepest_state = PWRDM_POWER_RET;
+ ? ? ? } else {
+ ? ? ? ? ? ? ? mpu_deepest_state = PWRDM_POWER_INACTIVE;
+ ? ? ? ? ? ? ? core_deepest_state = PWRDM_POWER_INACTIVE;
+ ? ? ? }
+
 ? ? ? ?if (enable_off_mode) {
+ ? ? ? ? ? ? ? WARN_ON(!omap3_has_pwroff());
+
 ? ? ? ? ? ? ? ?mpu_deepest_state = PWRDM_POWER_OFF;
+

? ? ? ? ? ? ? ?/*
? ? ? ? ? ? ? ? * Erratum i583: valable for ES rev < Es1.2 on 3630. ? ? ? ? ? ? ? ? * CORE OFF mode is not supported in a stable form, restrict

arch/arm/mach-omap2/cpuidle34xx.c +0 -0

@@ -311,6 +322,9 @@ void omap3_pm_init_cpuidle(struct cpuidle_params *cpuidle_board_params)
 ? ? ? ? ? ? ? ? ? ? ? ?cpuidle_board_params[i].exit_latency;

? ? ? ? ? ? ? ?cpuidle_params_table[i].target_residency = ? ? ? ? ? ? ? ? ? ? ? ?cpuidle_board_params[i].target_residency; +
+ ? ? ? ? ? ? ? WARN_ON(!omap3_has_pwroff() && (i >= 2) &&
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cpuidle_params_table[i].valid); ? ? ? ?}
? ? ? ?return;
?}

arch/arm/mach-omap2/cpuidle34xx.c +10 -0

@@ -349,6 +363,16 @@ static inline struct omap3_idle_statedata *_fill_cstate_usage(
 ? ? ? ?return cx;
 ?}
 
+/* Only C1 & C2 C states valid when RET & OFF states aren't supported */
+static void omap3_cstate_valid_fixup(void)
+{
+ ? ? ? int idx;
+
+ ? ? ? if (!omap3_has_pwroff())
+ ? ? ? ? ? ? ? for (idx = 2; idx < OMAP3_NUM_STATES; idx++)
+ ? ? ? ? ? ? ? ? ? ? ? cpuidle_params_table[idx].valid = 0;
+}
+

?/**
?* omap3_idle_init - Init routine for OMAP3 idle
?*

arch/arm/mach-omap2/cpuidle34xx.c +1 -0

@@ -366,6 +390,7 @@ int __init omap3_idle_init(void)
 ? ? ? ?per_pd = pwrdm_lookup("per_pwrdm");
 ? ? ? ?cam_pd = pwrdm_lookup("cam_pwrdm");
 
+ ? ? ? omap3_cstate_valid_fixup();
 
 ? ? ? ?drv->safe_state_index = -1;
 ? ? ? ?dev = &per_cpu(omap3_idle_dev, smp_processor_id());

arch/arm/mach-omap2/pm-debug.c +2 -1

index 814bcd9..339ed42 100644
@@ -239,19 +239,23 @@ static int option_get(void *data, u64 *val)
 ?static int option_set(void *data, u64 val)
 ?{
 ? ? ? ?u32 *option = data;
+ ? ? ? int ret = -EINVAL;
 
 ? ? ? ?*option = val;
 
- ? ? ? if (option == &enable_off_mode) {
+ ? ? ? if (omap3_has_pwroff() && (option == &enable_off_mode)) {
 ? ? ? ? ? ? ? ?if (val)
 ? ? ? ? ? ? ? ? ? ? ? ?omap_pm_enable_off_mode();

? ? ? ? ? ? ? ?else
? ? ? ? ? ? ? ? ? ? ? ?omap_pm_disable_off_mode(); +
? ? ? ? ? ? ? ?if (cpu_is_omap34xx())
? ? ? ? ? ? ? ? ? ? ? ?omap3_pm_off_mode_enable(val); +
+ ? ? ? ? ? ? ? ret = 0;
? ? ? ?}

- ? ? ? return 0;
+ ? ? ? return ret;
?}

?DEFINE_SIMPLE_ATTRIBUTE(pm_dbg_option_fops, option_get, option_set, "%llu\n");

arch/arm/mach-omap2/pm34xx.c +0 -0

index b7bbcee..00c4abe 100644
@@ -279,6 +279,7 @@ void omap_sram_idle(void)
 ? ? ? ?mpu_next_state = pwrdm_read_next_pwrst(mpu_pwrdm);
 ? ? ? ?switch (mpu_next_state) {

? ? ? ?case PWRDM_POWER_ON:
+ ? ? ? case PWRDM_POWER_INACTIVE:
? ? ? ?case PWRDM_POWER_RET:
? ? ? ? ? ? ? ?/* No need to save context */
? ? ? ? ? ? ? ?save_state = 0;

arch/arm/mach-omap2/pm34xx.c +2 -0

@@ -614,6 +615,8 @@ void omap3_pm_off_mode_enable(int enable)
 ? ? ? ?struct power_state *pwrst;
 ? ? ? ?u32 state;
 
+ ? ? ? WARN_ON(!omap3_has_pwroff());
+
 ? ? ? ?if (enable)
 ? ? ? ? ? ? ? ?state = PWRDM_POWER_OFF;

? ? ? ?else

arch/arm/mach-omap2/pm34xx.c +24 -16

@@ -668,7 +671,12 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
 ? ? ? ?if (!pwrst)
 ? ? ? ? ? ? ? ?return -ENOMEM;
 ? ? ? ?pwrst->pwrdm = pwrdm;
- ? ? ? pwrst->next_state = PWRDM_POWER_RET;
+
+ ? ? ? if (omap3_has_pwroff())
+ ? ? ? ? ? ? ? pwrst->next_state = PWRDM_POWER_RET;
+ ? ? ? else
+ ? ? ? ? ? ? ? pwrst->next_state = PWRDM_POWER_INACTIVE;
+
 ? ? ? ?list_add(&pwrst->node, &pwrst_list);
 
 ? ? ? ?if (pwrdm_has_hdwr_sar(pwrdm))

@@ -781,25 +789,28 @@ static int __init omap3_pm_init(void)
 ? ? ? ? ? ? ? ?omap3630_ctrl_disable_rta();
 
 ? ? ? ?clkdm_add_wkdep(neon_clkdm, mpu_clkdm);
- ? ? ? if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
- ? ? ? ? ? ? ? omap3_secure_ram_storage =
- ? ? ? ? ? ? ? ? ? ? ? kmalloc(0x803F, GFP_KERNEL);
- ? ? ? ? ? ? ? if (!omap3_secure_ram_storage)
- ? ? ? ? ? ? ? ? ? ? ? pr_err("Memory allocation failed when "
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"allocating for secure sram context\n");
 
- ? ? ? ? ? ? ? local_irq_disable();
- ? ? ? ? ? ? ? local_fiq_disable();
+ ? ? ? if (omap3_has_pwroff()) {
+ ? ? ? ? ? ? ? if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
+ ? ? ? ? ? ? ? ? ? ? ? omap3_secure_ram_storage =
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? kmalloc(0x803F, GFP_KERNEL);
+ ? ? ? ? ? ? ? ? ? ? ? if (!omap3_secure_ram_storage)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pr_err("Memory allocation failed when "
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"allocating for secure sram context\n");
 
- ? ? ? ? ? ? ? omap_dma_global_context_save();
- ? ? ? ? ? ? ? omap3_save_secure_ram_context();
- ? ? ? ? ? ? ? omap_dma_global_context_restore();
+ ? ? ? ? ? ? ? ? ? ? ? local_irq_disable();
+ ? ? ? ? ? ? ? ? ? ? ? local_fiq_disable();
 
- ? ? ? ? ? ? ? local_irq_enable();
- ? ? ? ? ? ? ? local_fiq_enable();
- ? ? ? }
+ ? ? ? ? ? ? ? ? ? ? ? omap_dma_global_context_save();
+ ? ? ? ? ? ? ? ? ? ? ? omap3_save_secure_ram_context();
+ ? ? ? ? ? ? ? ? ? ? ? omap_dma_global_context_restore();
+
+ ? ? ? ? ? ? ? ? ? ? ? local_irq_enable();
+ ? ? ? ? ? ? ? ? ? ? ? local_fiq_enable();
 
- ? ? ? omap3_save_scratchpad_contents();
+ ? ? ? ? ? ? ? ? ? ? ? omap3_save_scratchpad_contents();
+ ? ? ? ? ? ? ? }
+ ? ? ? }
 ? ? ? ?return ret;
 

?err3:
--
1.7.9.4

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

Quoted 8 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs only support PWRDM_POWER_ON and PWRDM_POWER_INACTIVE power states. This causes an issue in some areas of the OMAP3 power-related code because of assumptions that PWRDM_POWER_RET and/or PWRDM_POWER_OFF are always valid states.

Quoted 5 lines

To get am35x SoCs to work properly, add missing support for PWRDM_POWER_INACTIVE and remove assumptions that PWRDM_POWER_RET and PWRDM_POWER_OFF are always valid states.

Signed-off-by Mark A. Greer <[email protected]>

I just posted a patch set '[RFC/PATCH v2 0/6] ARM: OMAP2+: PM:
introduce the power domains functional states' [1] which I think should be used to base your patches on. This patch set introduces functional states for the power domains power ans logic states, and the conversion functions between the internal power and logic states and the functional states.

I would be glad to help on the AM35x support for the functional states.

Hi

Quoted 22 lines · Jean Pihet

Quoted 8 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs only support PWRDM_POWER_ON and PWRDM_POWER_INACTIVE power states. This causes an issue in some areas of the OMAP3 power-related code because of assumptions that PWRDM_POWER_RET and/or PWRDM_POWER_OFF are always valid states.

Quoted 5 lines

To get am35x SoCs to work properly, add missing support for PWRDM_POWER_INACTIVE and remove assumptions that PWRDM_POWER_RET and PWRDM_POWER_OFF are always valid states.

Signed-off-by Mark A. Greer <[email protected]>

I just posted a patch set '[RFC/PATCH v2 0/6] ARM: OMAP2+: PM:
introduce the power domains functional states' [1] which I think should be used to base your patches on. This patch set introduces functional states for the power domains power ans logic states, and the conversion functions between the internal power and logic states and the functional states.

Quoted 1 line

I would be glad to help on the AM35x support for the functional states.

Since it might take a while for the functional powerstate code to go upstream, if Mark wants to do this, I'd suggest that he split the powerdomain-specific patches off from the rest of his series. This should avoid blocking the rest of his patches on the infrastructure changes.

Quoted 28 lines · Paul Walmsley

Hi

Quoted 23 lines · Jean Pihet

Quoted 8 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs only support PWRDM_POWER_ON and PWRDM_POWER_INACTIVE power states. This causes an issue in some areas of the OMAP3 power-related code because of assumptions that PWRDM_POWER_RET and/or PWRDM_POWER_OFF are always valid states.

Quoted 5 lines

To get am35x SoCs to work properly, add missing support for PWRDM_POWER_INACTIVE and remove assumptions that PWRDM_POWER_RET and PWRDM_POWER_OFF are always valid states.

Signed-off-by Mark A. Greer <[email protected]>

I just posted a patch set '[RFC/PATCH v2 0/6] ARM: OMAP2+: PM:
introduce the power domains functional states' [1] which I think should be used to base your patches on. This patch set introduces functional states for the power domains power ans logic states, and the conversion functions between the internal power and logic states and the functional states.

I would be glad to help on the AM35x support for the functional states.

Quoted 4 lines

Since it might take a while for the functional powerstate code to go upstream, if Mark wants to do this, I'd suggest that he split the powerdomain-specific patches off from the rest of his series. ?This should avoid blocking the rest of his patches on the infrastructure changes.

Another good idea. ?I'll do the split in another day or so in case there are more comments I can address when I respin & separate the patches.

Mark
--

Hi Mark,

Quoted 8 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs only support PWRDM_POWER_ON and PWRDM_POWER_INACTIVE power states. This causes an issue in some areas of the OMAP3 power-related code because of assumptions that PWRDM_POWER_RET and/or PWRDM_POWER_OFF are always valid states.

Quoted 5 lines

To get am35x SoCs to work properly, add missing support for PWRDM_POWER_INACTIVE and remove assumptions that PWRDM_POWER_RET and PWRDM_POWER_OFF are always valid states.

Signed-off-by Mark A. Greer <[email protected]>

I just posted a patch set '[RFC/PATCH v2 0/6] ARM: OMAP2+: PM:
introduce the power domains functional states' [1] which I think should be used to base your patches on. This patch set introduces functional states for the power domains power ans logic states, and the conversion functions between the internal power and logic states and the functional states.

I would be glad to help on the AM35x support for the functional states.

What do you think?

[1] http://marc.info/?l=linux-omap&m=133417729712693&w=2

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs only support PWRDM_POWER_ON and PWRDM_POWER_INACTIVE power states. This causes an issue in some areas of the OMAP3 power-related code because of assumptions that PWRDM_POWER_RET and/or PWRDM_POWER_OFF are always valid states.

To get am35x SoCs to work properly, add missing support for PWRDM_POWER_INACTIVE and remove assumptions that PWRDM_POWER_RET and PWRDM_POWER_OFF are always valid states.

Signed-off-by Mark A. Greer <[email protected]>

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs only support PWRSTS_ON and PWRSTS_INACTIVE states so create a new set of powerdomain structures that ensure that only the ON and INACTIVE states are entered.

Signed-off-by Mark A. Greer <[email protected]>

---
 arch/arm/mach-omap2/powerdomain.h           |    1 +
 arch/arm/mach-omap2/powerdomains3xxx_data.c |  134 ++++++++++++++++++++++++---
 2 files changed, 123 insertions(+), 12 deletions(-)

arch/arm/mach-omap2/powerdomain.h +1 -0

index 0d72a8a..561cb27 100644
@@ -43,6 +43,7 @@
 #define PWRSTS_OFF_ON		(PWRSTS_OFF | PWRSTS_ON)
 #define PWRSTS_OFF_RET		(PWRSTS_OFF | PWRSTS_RET)
 #define PWRSTS_RET_ON		(PWRSTS_RET | PWRSTS_ON)
+#define PWRSTS_INACTIVE_ON	(PWRSTS_INACTIVE | PWRSTS_ON)
 #define PWRSTS_OFF_RET_ON	(PWRSTS_OFF_RET | PWRSTS_ON)
 
 

arch/arm/mach-omap2/powerdomains3xxx_data.c +122 -12

index b7ea468..fae6dea 100644
@@ -71,6 +71,22 @@ static struct powerdomain mpu_3xxx_pwrdm = {
 	.voltdm           = { .name = "mpu_iva" },
 };
 
+static struct powerdomain mpu_am35x_pwrdm = {
+	.name		  = "mpu_pwrdm",
+	.prcm_offs	  = MPU_MOD,
+	.pwrsts		  = PWRSTS_INACTIVE_ON,
+	.pwrsts_logic_ret = PWRSTS_INACTIVE,
+	.flags		  = PWRDM_HAS_MPU_QUIRK,
+	.banks		  = 1,
+	.pwrsts_mem_ret	  = {
+		[0] = PWRSTS_INACTIVE,
+	},
+	.pwrsts_mem_on	  = {
+		[0] = PWRSTS_INACTIVE_ON,
+	},
+	.voltdm           = { .name = "mpu_iva" },
+};
+
 /*
  * The USBTLL Save-and-Restore mechanism is broken on
  * 3430s up to ES3.0 and 3630ES1.0. Hence this feature

@@ -120,6 +136,23 @@ static struct powerdomain core_3xxx_es3_1_pwrdm = {
 	.voltdm           = { .name = "core" },
 };
 
+static struct powerdomain core_am35x_pwrdm = {
+	.name		  = "core_pwrdm",
+	.prcm_offs	  = CORE_MOD,
+	.pwrsts		  = PWRSTS_INACTIVE_ON,
+	.pwrsts_logic_ret = PWRSTS_INACTIVE,
+	.banks		  = 2,
+	.pwrsts_mem_ret	  = {
+		[0] = PWRSTS_INACTIVE,	 /* MEM1RETSTATE */
+		[1] = PWRSTS_INACTIVE,	 /* MEM2RETSTATE */
+	},
+	.pwrsts_mem_on	  = {
+		[0] = PWRSTS_INACTIVE_ON, /* MEM1ONSTATE */
+		[1] = PWRSTS_INACTIVE_ON, /* MEM2ONSTATE */
+	},
+	.voltdm           = { .name = "core" },
+};
+
 static struct powerdomain dss_pwrdm = {
 	.name		  = "dss_pwrdm",
 	.prcm_offs	  = OMAP3430_DSS_MOD,

@@ -135,6 +168,21 @@ static struct powerdomain dss_pwrdm = {
 	.voltdm           = { .name = "core" },
 };
 
+static struct powerdomain dss_am35x_pwrdm = {
+	.name		  = "dss_pwrdm",
+	.prcm_offs	  = OMAP3430_DSS_MOD,
+	.pwrsts		  = PWRSTS_INACTIVE_ON,
+	.pwrsts_logic_ret = PWRSTS_INACTIVE,
+	.banks		  = 1,
+	.pwrsts_mem_ret	  = {
+		[0] = PWRSTS_INACTIVE, /* MEMRETSTATE */
+	},
+	.pwrsts_mem_on	  = {
+		[0] = PWRSTS_ON,  /* MEMONSTATE */
+	},
+	.voltdm           = { .name = "core" },
+};
+
 /*
  * Although the 34XX TRM Rev K Table 4-371 notes that retention is a
  * possible SGX powerstate, the SGX device itself does not support

@@ -156,6 +204,21 @@ static struct powerdomain sgx_pwrdm = {
 	.voltdm           = { .name = "core" },
 };
 
+static struct powerdomain sgx_am35x_pwrdm = {
+	.name		  = "sgx_pwrdm",
+	.prcm_offs	  = OMAP3430ES2_SGX_MOD,
+	.pwrsts		  = PWRSTS_INACTIVE_ON,
+	.pwrsts_logic_ret = PWRSTS_INACTIVE,
+	.banks		  = 1,
+	.pwrsts_mem_ret	  = {
+		[0] = PWRSTS_INACTIVE, /* MEMRETSTATE */
+	},
+	.pwrsts_mem_on	  = {
+		[0] = PWRSTS_ON,  /* MEMONSTATE */
+	},
+	.voltdm           = { .name = "core" },
+};
+
 static struct powerdomain cam_pwrdm = {
 	.name		  = "cam_pwrdm",
 	.prcm_offs	  = OMAP3430_CAM_MOD,

@@ -186,6 +249,21 @@ static struct powerdomain per_pwrdm = {
 	.voltdm           = { .name = "core" },
 };
 
+static struct powerdomain per_am35x_pwrdm = {
+	.name		  = "per_pwrdm",
+	.prcm_offs	  = OMAP3430_PER_MOD,
+	.pwrsts		  = PWRSTS_INACTIVE_ON,
+	.pwrsts_logic_ret = PWRSTS_INACTIVE,
+	.banks		  = 1,
+	.pwrsts_mem_ret	  = {
+		[0] = PWRSTS_INACTIVE, /* MEMRETSTATE */
+	},
+	.pwrsts_mem_on	  = {
+		[0] = PWRSTS_ON,  /* MEMONSTATE */
+	},
+	.voltdm           = { .name = "core" },
+};
+
 static struct powerdomain emu_pwrdm = {
 	.name		= "emu_pwrdm",
 	.prcm_offs	= OMAP3430_EMU_MOD,

@@ -200,6 +278,14 @@ static struct powerdomain neon_pwrdm = {
 	.voltdm           = { .name = "mpu_iva" },
 };
 
+static struct powerdomain neon_am35x_pwrdm = {
+	.name		  = "neon_pwrdm",
+	.prcm_offs	  = OMAP3430_NEON_MOD,
+	.pwrsts		  = PWRSTS_INACTIVE_ON,
+	.pwrsts_logic_ret = PWRSTS_INACTIVE,
+	.voltdm           = { .name = "mpu_iva" },
+};
+
 static struct powerdomain usbhost_pwrdm = {
 	.name		  = "usbhost_pwrdm",
 	.prcm_offs	  = OMAP3430ES2_USBHOST_MOD,

@@ -293,6 +379,22 @@ static struct powerdomain *powerdomains_omap3430es3_1plus[] __initdata = {
 	NULL
 };
 
+static struct powerdomain *powerdomains_am35x[] __initdata = {
+	&wkup_omap2_pwrdm,
+	&mpu_am35x_pwrdm,
+	&neon_am35x_pwrdm,
+	&core_am35x_pwrdm,
+	&sgx_am35x_pwrdm,
+	&dss_am35x_pwrdm,
+	&per_am35x_pwrdm,
+	&emu_pwrdm,
+	&dpll1_pwrdm,
+	&dpll3_pwrdm,
+	&dpll4_pwrdm,
+	&dpll5_pwrdm,
+	NULL
+};
+
 void __init omap3xxx_powerdomains_init(void)
 {
 	unsigned int rev;

@@ -301,21 +403,29 @@ void __init omap3xxx_powerdomains_init(void)
 		return;
 
 	pwrdm_register_platform_funcs(&omap3_pwrdm_operations);
-	pwrdm_register_pwrdms(powerdomains_omap3430_common);
 
 	rev = omap_rev();
 
-	if (rev == OMAP3430_REV_ES1_0)
-		pwrdm_register_pwrdms(powerdomains_omap3430es1);
-	else if (rev == OMAP3430_REV_ES2_0 || rev == OMAP3430_REV_ES2_1 ||
-		 rev == OMAP3430_REV_ES3_0 || rev == OMAP3630_REV_ES1_0)
-		pwrdm_register_pwrdms(powerdomains_omap3430es2_es3_0);
-	else if (rev == OMAP3430_REV_ES3_1 || rev == OMAP3430_REV_ES3_1_2 ||
-		 rev == OMAP3517_REV_ES1_0 || rev == OMAP3517_REV_ES1_1 ||
-		 rev == OMAP3630_REV_ES1_1 || rev == OMAP3630_REV_ES1_2)
-		pwrdm_register_pwrdms(powerdomains_omap3430es3_1plus);
-	else
-		WARN(1, "OMAP3 powerdomain init: unknown chip type\n");
+	if (rev == OMAP3517_REV_ES1_0 || rev == OMAP3517_REV_ES1_1)
+		pwrdm_register_pwrdms(powerdomains_am35x);
+	else {
+		pwrdm_register_pwrdms(powerdomains_omap3430_common);
+
+		if (rev == OMAP3430_REV_ES1_0)
+			pwrdm_register_pwrdms(powerdomains_omap3430es1);
+		else if (rev == OMAP3430_REV_ES2_0 ||
+			 rev == OMAP3430_REV_ES2_1 ||
+			 rev == OMAP3430_REV_ES3_0 ||
+			 rev == OMAP3630_REV_ES1_0)
+			pwrdm_register_pwrdms(powerdomains_omap3430es2_es3_0);
+		else if (rev == OMAP3430_REV_ES3_1 ||
+			 rev == OMAP3430_REV_ES3_1_2 ||
+			 rev == OMAP3630_REV_ES1_1 ||
+			 rev == OMAP3630_REV_ES1_2)
+			pwrdm_register_pwrdms(powerdomains_omap3430es3_1plus);
+		else
+			WARN(1, "OMAP3 powerdomain init: unknown chip type\n");
+	}
 
 	pwrdm_complete_init();
 }

Signature · 1 line

1.7.9.4

Hi

Quoted 6 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs only support PWRSTS_ON and PWRSTS_INACTIVE states so create a new set of powerdomain structures that ensure that only the ON and INACTIVE states are entered.

I'm still unsure what we should do about this INACTIVE state. As far as I can tell, it does not actually represent a distinct powerdomain state on OMAP3. We can't write it to the PM_PWSTCTRL_*.POWERSTATE bits, since that value is marked as "reserved". And SPRUGR0B, the AM3517/3505 TRM, explicitly states "This register should not be programmed with reserved values of bit fields for proper functioning of power state control." Of course SPRUGR0B also claims that AM3517/3505 supports RETENTION, so that is relatively confusing.

OMAP4 PRCM actually does seem to support powerdomain INACTIVE state, although it does not actually seem to represent a separate state. As I understand it, it simply allows the clockdomains in the powerdomain to transitioning from the ACTIVE state into the INACTIVE state. In other words, it doesn't affect the powerdomain power state at all. And on OMAP4 it's referred to as the ON-INACTIVE state, to clarify that it is simply a variant of the ON state. I wish they had just added a separate bit for this; it would have avoided some frustration...

Anyway, my question is this: is there any point at all to defining the INACTIVE state for 3517/3505, given that it apparently cannot be read from, nor written to the PRCM hardware?

Quoted 11 lines · Paul Walmsley

Hi

Quoted 6 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs only support PWRSTS_ON and PWRSTS_INACTIVE states so create a new set of powerdomain structures that ensure that only the ON and INACTIVE states are entered.

Quoted 8 lines

I'm still unsure what we should do about this INACTIVE state. As far as I can tell, it does not actually represent a distinct powerdomain state on OMAP3. We can't write it to the PM_PWSTCTRL_*.POWERSTATE bits, since that value is marked as "reserved". And SPRUGR0B, the AM3517/3505 TRM, explicitly states "This register should not be programmed with reserved values of bit fields for proper functioning of power state control." Of course SPRUGR0B also claims that AM3517/3505 supports RETENTION, so that is relatively confusing.

Yep. You read one section which tells you the device doesn't support such-and-such then it goes on for pages describing how it works. I play with the hardware and the registers are there so it seems like it should work. Its been an "adventure". :)

Quoted 9 lines

OMAP4 PRCM actually does seem to support powerdomain INACTIVE state, although it does not actually seem to represent a separate state. As I understand it, it simply allows the clockdomains in the powerdomain to transitioning from the ACTIVE state into the INACTIVE state. In other words, it doesn't affect the powerdomain power state at all. And on OMAP4 it's referred to as the ON-INACTIVE state, to clarify that it is simply a variant of the ON state. I wish they had just added a separate bit for this; it would have avoided some frustration...

Quoted 3 lines

Anyway, my question is this: is there any point at all to defining the INACTIVE state for 3517/3505, given that it apparently cannot be read from, nor written to the PRCM hardware?

Great question and I really don't know the answer--its too hard to know what's for real and what isn't in the TRM. Maybe it is best to get rid of INACTIVE and just leave everything ON.

Quoted 16 lines · Mark A. Greer

Quoted 15 lines · Paul Walmsley

Hi

Quoted 1 line · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

OMAP4 PRCM actually does seem to support powerdomain INACTIVE state, although it does not actually seem to represent a separate state. As I understand it, it simply allows the clockdomains in the powerdomain to transitioning from the ACTIVE state into the INACTIVE state. In other words, it doesn't affect the powerdomain power state at all. And on OMAP4 it's referred to as the ON-INACTIVE state, to clarify that it is simply a variant of the ON state. I wish they had just added a separate bit for this; it would have avoided some frustration...

Quoted 7 lines

Quoted 3 lines

Anyway, my question is this: is there any point at all to defining the INACTIVE state for 3517/3505, given that it apparently cannot be read from, nor written to the PRCM hardware?

Great question and I really don't know the answer--its too hard to know what's for real and what isn't in the TRM. Maybe it is best to get rid of INACTIVE and just leave everything ON.

FWIW, for omap3 class device inactive is defined as ...

"Inactive: The same as power on but all clocks are cut. Logic is not working, because no clock is running."

There is nothing explicit to program as far as the power domain goes. For OMAP4 you can program the power domain to enter inactive and when all power domains in a voltage domain are inactive the voltage domain can enter the sleep state.

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs do not have an IVA so
a parallel set of clockdomain dependencies are
required that are simililar to OMAP3 but without
any IVA dependencies.

Signed-off-by Mark A. Greer <[email protected]>

---
 arch/arm/mach-omap2/clockdomains3xxx_data.c |  146 ++++++++++++++++++++++++++-
 1 file changed, 141 insertions(+), 5 deletions(-)

arch/arm/mach-omap2/clockdomains3xxx_data.c +0 -0

index b84e138..a05a8cb 100644

arch/arm/mach-omap2/clockdomains3xxx_data.c +111 -0

+
 /* 3430: PM_WKDEP_PER: CORE, IVA2, MPU, WKUP */
 static struct clkdm_dep per_wkdeps[] = {
 	{ .clkdm_name = "core_l3_clkdm" },

@@ -69,6 +75,14 @@ static struct clkdm_dep per_wkdeps[] = {
 	{ NULL },
 };
 
+static struct clkdm_dep per_am35x_wkdeps[] = {
+	{ .clkdm_name = "core_l3_clkdm" },
+	{ .clkdm_name = "core_l4_clkdm" },
+	{ .clkdm_name = "mpu_clkdm" },
+	{ .clkdm_name = "wkup_clkdm" },
+	{ NULL },
+};
+
 /* 3430ES2: PM_WKDEP_USBHOST: CORE, IVA2, MPU, WKUP */
 static struct clkdm_dep usbhost_wkdeps[] = {
 	{ .clkdm_name = "core_l3_clkdm" },

@@ -79,6 +93,14 @@ static struct clkdm_dep usbhost_wkdeps[] = {
 	{ NULL },
 };
 
+static struct clkdm_dep usbhost_am35x_wkdeps[] = {
+	{ .clkdm_name = "core_l3_clkdm" },
+	{ .clkdm_name = "core_l4_clkdm" },
+	{ .clkdm_name = "mpu_clkdm" },
+	{ .clkdm_name = "wkup_clkdm" },
+	{ NULL },
+};
+
 /* 3430 PM_WKDEP_MPU: CORE, IVA2, DSS, PER */
 static struct clkdm_dep mpu_3xxx_wkdeps[] = {
 	{ .clkdm_name = "core_l3_clkdm" },

@@ -89,6 +111,14 @@ static struct clkdm_dep mpu_3xxx_wkdeps[] = {
 	{ NULL },
 };
 
+static struct clkdm_dep mpu_am35x_wkdeps[] = {
+	{ .clkdm_name = "core_l3_clkdm" },
+	{ .clkdm_name = "core_l4_clkdm" },
+	{ .clkdm_name = "dss_clkdm" },
+	{ .clkdm_name = "per_clkdm" },
+	{ NULL },
+};
+
 /* 3430 PM_WKDEP_IVA2: CORE, MPU, WKUP, DSS, PER */
 static struct clkdm_dep iva2_wkdeps[] = {
 	{ .clkdm_name = "core_l3_clkdm" },

@@ -116,6 +146,12 @@ static struct clkdm_dep dss_wkdeps[] = {
 	{ NULL },
 };
 
+static struct clkdm_dep dss_am35x_wkdeps[] = {
+	{ .clkdm_name = "mpu_clkdm" },
+	{ .clkdm_name = "wkup_clkdm" },
+	{ NULL },
+};
+
 /* 3430: PM_WKDEP_NEON: MPU */
 static struct clkdm_dep neon_wkdeps[] = {
 	{ .clkdm_name = "mpu_clkdm" },

@@ -131,6 +167,11 @@ static struct clkdm_dep dss_sleepdeps[] = {
 	{ NULL },
 };
 
+static struct clkdm_dep dss_am35x_sleepdeps[] = {
+	{ .clkdm_name = "mpu_clkdm" },
+	{ NULL },
+};
+
 /* 3430: CM_SLEEPDEP_PER: MPU, IVA */
 static struct clkdm_dep per_sleepdeps[] = {
 	{ .clkdm_name = "mpu_clkdm" },

@@ -138,6 +179,11 @@ static struct clkdm_dep per_sleepdeps[] = {
 	{ NULL },
 };
 
+static struct clkdm_dep per_am35x_sleepdeps[] = {
+	{ .clkdm_name = "mpu_clkdm" },
+	{ NULL },
+};
+
 /* 3430ES2: CM_SLEEPDEP_USBHOST: MPU, IVA */
 static struct clkdm_dep usbhost_sleepdeps[] = {
 	{ .clkdm_name = "mpu_clkdm" },

@@ -145,6 +191,11 @@ static struct clkdm_dep usbhost_sleepdeps[] = {
 	{ NULL },
 };
 
+static struct clkdm_dep usbhost_am35x_sleepdeps[] = {
+	{ .clkdm_name = "mpu_clkdm" },
+	{ NULL },
+};
+
 /* 3430: CM_SLEEPDEP_CAM: MPU */
 static struct clkdm_dep cam_sleepdeps[] = {
 	{ .clkdm_name = "mpu_clkdm" },

@@ -175,6 +226,15 @@ static struct clockdomain mpu_3xxx_clkdm = {
 	.clktrctrl_mask = OMAP3430_CLKTRCTRL_MPU_MASK,
 };
 
+static struct clockdomain mpu_am35x_clkdm = {
+	.name		= "mpu_clkdm",
+	.pwrdm		= { .name = "mpu_pwrdm" },
+	.flags		= CLKDM_CAN_HWSUP | CLKDM_CAN_FORCE_WAKEUP,
+	.dep_bit	= OMAP3430_EN_MPU_SHIFT,
+	.wkdep_srcs	= mpu_am35x_wkdeps,
+	.clktrctrl_mask = OMAP3430_CLKTRCTRL_MPU_MASK,
+};
+
 static struct clockdomain neon_clkdm = {
 	.name		= "neon_clkdm",
 	.pwrdm		= { .name = "neon_pwrdm" },

@@ -210,6 +270,15 @@ static struct clockdomain sgx_clkdm = {
 	.clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_SGX_MASK,
 };
 
+static struct clockdomain sgx_am35x_clkdm = {
+	.name		= "sgx_clkdm",
+	.pwrdm		= { .name = "sgx_pwrdm" },
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,
+	.wkdep_srcs	= gfx_sgx_am35x_wkdeps,
+	.sleepdep_srcs	= gfx_sgx_sleepdeps,
+	.clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_SGX_MASK,
+};
+
 /*
  * The die-to-die clockdomain was documented in the 34xx ES1 TRM, but
  * then that information was removed from the 34xx ES2+ TRM.  It is

@@ -261,6 +330,16 @@ static struct clockdomain dss_3xxx_clkdm = {
 	.clktrctrl_mask = OMAP3430_CLKTRCTRL_DSS_MASK,
 };
 
+static struct clockdomain dss_am35x_clkdm = {
+	.name		= "dss_clkdm",
+	.pwrdm		= { .name = "dss_pwrdm" },
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,
+	.dep_bit	= OMAP3430_PM_WKDEP_MPU_EN_DSS_SHIFT,
+	.wkdep_srcs	= dss_am35x_wkdeps,
+	.sleepdep_srcs	= dss_am35x_sleepdeps,
+	.clktrctrl_mask = OMAP3430_CLKTRCTRL_DSS_MASK,
+};
+
 static struct clockdomain cam_clkdm = {
 	.name		= "cam_clkdm",
 	.pwrdm		= { .name = "cam_pwrdm" },

@@ -279,6 +358,15 @@ static struct clockdomain usbhost_clkdm = {
 	.clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_USBHOST_MASK,
 };
 
+static struct clockdomain usbhost_am35x_clkdm = {
+	.name		= "usbhost_clkdm",
+	.pwrdm		= { .name = "core_pwrdm" },
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,
+	.wkdep_srcs	= usbhost_am35x_wkdeps,
+	.sleepdep_srcs	= usbhost_am35x_sleepdeps,
+	.clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_USBHOST_MASK,
+};
+
 static struct clockdomain per_clkdm = {
 	.name		= "per_clkdm",
 	.pwrdm		= { .name = "per_pwrdm" },

@@ -289,6 +377,16 @@ static struct clockdomain per_clkdm = {
 	.clktrctrl_mask = OMAP3430_CLKTRCTRL_PER_MASK,
 };
 
+static struct clockdomain per_am35x_clkdm = {
+	.name		= "per_clkdm",
+	.pwrdm		= { .name = "per_pwrdm" },
+	.flags		= CLKDM_CAN_HWSUP_SWSUP,
+	.dep_bit	= OMAP3430_EN_PER_SHIFT,
+	.wkdep_srcs	= per_am35x_wkdeps,
+	.sleepdep_srcs	= per_am35x_sleepdeps,
+	.clktrctrl_mask = OMAP3430_CLKTRCTRL_PER_MASK,
+};
+
 /*
  * Disable hw supervised mode for emu_clkdm, because emu_pwrdm is
  * switched of even if sdti is in use

@@ -341,6 +439,15 @@ static struct clkdm_autodep clkdm_autodeps[] = {
 	}
 };
 
+static struct clkdm_autodep clkdm_am35x_autodeps[] = {
+	{
+		.clkdm = { .name = "mpu_clkdm" },
+	},
+	{
+		.clkdm = { .name = NULL },
+	}
+};
+
 /*
  *
  */

@@ -366,6 +473,26 @@ static struct clockdomain *clockdomains_omap3430_common[] __initdata = {
 	NULL
 };
 
+static struct clockdomain *clockdomains_am35x_common[] __initdata = {
+	&wkup_common_clkdm,
+	&cm_common_clkdm,
+	&prm_common_clkdm,
+	&mpu_am35x_clkdm,
+	&neon_clkdm,
+	&sgx_am35x_clkdm,
+	&core_l3_3xxx_clkdm,
+	&core_l4_3xxx_clkdm,

arch/arm/mach-omap2/clockdomains3xxx_data.c +15 -5

+
 static struct clockdomain *clockdomains_omap3430es1[] __initdata = {
 	&gfx_3430es1_clkdm,
 	NULL,

@@ -381,18 +508,27 @@ static struct clockdomain *clockdomains_omap3430es2plus[] __initdata = {
 void __init omap3xxx_clockdomains_init(void)
 {
 	struct clockdomain **sc;
+	unsigned int rev;
 
 	if (!cpu_is_omap34xx())
 		return;
 
 	clkdm_register_platform_funcs(&omap3_clkdm_operations);
-	clkdm_register_clkdms(clockdomains_omap3430_common);
 
-	sc = (omap_rev() == OMAP3430_REV_ES1_0) ? clockdomains_omap3430es1 :
-		clockdomains_omap3430es2plus;
+	rev = omap_rev();
 
-	clkdm_register_clkdms(sc);
+	if (rev == OMAP3517_REV_ES1_0 || rev == OMAP3517_REV_ES1_1) {
+		clkdm_register_clkdms(clockdomains_am35x_common);
+		clkdm_register_autodeps(clkdm_am35x_autodeps);
+	} else {
+		clkdm_register_clkdms(clockdomains_omap3430_common);
+
+		sc = (rev == OMAP3430_REV_ES1_0) ?
+			clockdomains_omap3430es1 : clockdomains_omap3430es2plus;
+
+		clkdm_register_clkdms(sc);
+		clkdm_register_autodeps(clkdm_autodeps);
+	}
 
-	clkdm_register_autodeps(clkdm_autodeps);
 	clkdm_complete_init();
 }

Signature · 1 line

1.7.9.4

Hi

Quoted 16 lines

From: "Mark A. Greer" <[email protected]>

The am35x family of SoCs do not have an IVA so
a parallel set of clockdomain dependencies are
required that are simililar to OMAP3 but without
any IVA dependencies.

Signed-off-by Mark A. Greer <[email protected]>

---

arch/arm/mach-omap2/clockdomains3xxx_data.c +0 -0

index b84e138..a05a8cb 100644

arch/arm/mach-omap2/clockdomains3xxx_data.c static struct clkdm_dep gfx_sgx_3xxx_wkdeps[] = { · line 66

Quoted 8 lines

arch/arm/mach-omap2/clockdomains3xxx_data.c +5 -0

@@ -59,6 +59,12 @@ static struct clkdm_dep gfx_sgx_3xxx_wkdeps[] = {
 	{ NULL },
 };
 
+static struct clkdm_dep gfx_sgx_am35x_wkdeps[] = {
+	{ .clkdm_name = "mpu_clkdm", },
+	{ .clkdm_name = "wkup_clkdm", },
+	{ NULL },
+};

Looks like there are an extra set of commas here, maybe just use:

static struct clkdm_dep gfx_sgx_am35x_wkdeps[] = {
	{ .clkdm_name = "mpu_clkdm" },
	{ .clkdm_name = "wkup_clkdm" },
	{ NULL },

Quoted 20 lines

arch/arm/mach-omap2/clockdomains3xxx_data.c +16 -0

@@ -366,6 +473,26 @@ static struct clockdomain *clockdomains_omap3430_common[] __initdata = {
 	NULL
 };
 
+static struct clockdomain *clockdomains_am35x_common[] __initdata = {
+	&wkup_common_clkdm,
+	&cm_common_clkdm,
+	&prm_common_clkdm,
+	&mpu_am35x_clkdm,
+	&neon_clkdm,
+	&sgx_am35x_clkdm,
+	&core_l3_3xxx_clkdm,
+	&core_l4_3xxx_clkdm,
+	&dss_am35x_clkdm,
+	&per_am35x_clkdm,
+	&emu_clkdm,
+	&usbhost_am35x_clkdm,
+	&dpll1_clkdm,
+	&dpll3_clkdm,
+	&dpll4_clkdm,

arch/arm/mach-omap2/clockdomains3xxx_data.c static struct clockdomain *clockdomains_omap3430_common[] __initdata = { · line 494

Quoted 3 lines

arch/arm/mach-omap2/clockdomains3xxx_data.c +3 -0

+	&dpll5_clkdm,
+	NULL
+};

Looks like many of these clockdomains are shared with the rest of OMAP3xxx. Maybe move the shared ones into a common array, remove them from clockdomains_omap3430_common, and in the init function below, first register the shared ones? That will avoid duplicate listings.

Quoted 6 lines · Paul Walmsley

Looks like there are an extra set of commas here, maybe just use:

static struct clkdm_dep gfx_sgx_am35x_wkdeps[] = {
	{ .clkdm_name = "mpu_clkdm" },
	{ .clkdm_name = "wkup_clkdm" },
	{ NULL },

Yes, there are extra commas but they come from the code I'm copying and removing iva2_clkdm from. I was keeping things consistent so I kept the commas. Given that, do you still want them removed? (I'm indifferent.)

Quoted 9 lines · Mark A. Greer

Quoted 6 lines · Paul Walmsley

Looks like there are an extra set of commas here, maybe just use:

static struct clkdm_dep gfx_sgx_am35x_wkdeps[] = {
	{ .clkdm_name = "mpu_clkdm" },
	{ .clkdm_name = "wkup_clkdm" },
	{ NULL },

Quoted 4 lines

Yes, there are extra commas but they come from the code I'm copying and removing iva2_clkdm from. I was keeping things consistent so I kept the commas. Given that, do you still want them removed? (I'm indifferent.)

Yes please. I realize it's a minor issue, but I'd like to keep the format of these files internally consistent.

Quoted 16 lines · Paul Walmsley

Quoted 13 lines · Mark A. Greer

Quoted 6 lines · Paul Walmsley

Looks like there are an extra set of commas here, maybe just use:

static struct clkdm_dep gfx_sgx_am35x_wkdeps[] = {
	{ .clkdm_name = "mpu_clkdm" },
	{ .clkdm_name = "wkup_clkdm" },
	{ NULL },

Yes, there are extra commas but they come from the code I'm copying and removing iva2_clkdm from. I was keeping things consistent so I kept the commas. Given that, do you still want them removed? (I'm indifferent.)

Quoted 2 lines

Yes please. I realize it's a minor issue, but I'd like to keep the format of these files internally consistent.

Hrm, did you meant "No thanks"? As in, leave the commas there since that's what the code that is already there is like?

Quoted 17 lines · Mark A. Greer

Quoted 16 lines · Paul Walmsley

Quoted 13 lines · Mark A. Greer

Quoted 6 lines · Paul Walmsley

Looks like there are an extra set of commas here, maybe just use:

static struct clkdm_dep gfx_sgx_am35x_wkdeps[] = {
	{ .clkdm_name = "mpu_clkdm" },
	{ .clkdm_name = "wkup_clkdm" },
	{ NULL },

Yes, there are extra commas but they come from the code I'm copying and removing iva2_clkdm from. I was keeping things consistent so I kept the commas. Given that, do you still want them removed? (I'm indifferent.)

Quoted 5 lines

Quoted 2 lines

Yes please. I realize it's a minor issue, but I'd like to keep the format of these files internally consistent.

Hrm, did you meant "No thanks"? As in, leave the commas there since that's what the code that is already there is like?

The current data is in this format:

/* 3430 PM_WKDEP_CAM: IVA2, MPU, WKUP */

static struct clkdm_dep cam_wkdeps[] = {
	{ .clkdm_name = "iva2_clkdm" },
	{ .clkdm_name = "mpu_clkdm" },
	{ .clkdm_name = "wkup_clkdm" },
	{ NULL },
};

So if you can match that, it would be ideal.

Okay, I'll get rid of the commas (fwiw, gfx_sgx_3xxx_wkdeps[] has commas in k.o, the others don't).

Quoted 2 lines · Mark A. Greer

Okay, I'll get rid of the commas (fwiw, gfx_sgx_3xxx_wkdeps[] has commas in k.o, the others don't).

Okay thanks, that's just an oversight in the mainline data. If you're so inclined, feel free to clean that up in your patch...

From: "Mark A. Greer" <[email protected]>

The typical SDRAM Controller Subsystem module (SDRC) on TI OMAP3 devices has two submodules: the SDRAM Memory Scheduler (SMS) submodule, and the SDRC submodule--the 'SDRC' acronym/term is overloaded. The am35x family of devices is different in that it has an EMIF4 submodule instead of an SDRC submodule. The SMS submodules are similar, though.

To allow code to determine whether the current device has an SDRC submodule or and EMIF4 submodule, add the 'OMAP3_HAS_SDRC_EMIF4' feature and set it when running on an am35x SoC.

So when:
- omap3_has_sdrc() returns true: the system has an SDRC module consisting of an SMS submodule and either an SDRC submodule or an EMIF4 submodule.
- omap3_has_sdrc_emif4() returns true: the system has an EMIF4 submodule. It is assumed that this feature will only be checked when there is an SDRC module present (i.e., when omap3_has_sdrc() would return true).

Signed-off-by Mark A. Greer <[email protected]>

---
 arch/arm/mach-omap2/id.c              |    5 ++++-
 arch/arm/plat-omap/include/plat/cpu.h |   12 +++++++-----
 2 files changed, 11 insertions(+), 6 deletions(-)

arch/arm/mach-omap2/id.c +4 -1

index b6508e5..abb5e51 100644
@@ -256,10 +256,13 @@ void __init omap3xxx_check_features(void)
 	 *   the incorrectly set feature bits.
 	 * - Indicate that am35x SoCs don't support the PWRDM_POWER_RET
 	 *   and PWRDM_POWER_OFF states by clearing OMAP3_HAS_PWROFF.
+	 * - Indicate that am35x SoCs have an EMIF4 SDRC submodule.
 	 */
-	if (cpu_is_omap3505() || cpu_is_omap3517())
+	if (cpu_is_omap3505() || cpu_is_omap3517()) {
 		omap_features &= ~(OMAP3_HAS_IVA | OMAP3_HAS_ISP |
 			OMAP3_HAS_PWROFF);
+		omap_features |= OMAP3_HAS_SDRC_EMIF4;
+	}
 
 	/*
 	 * TODO: Get additional info (where applicable)

arch/arm/plat-omap/include/plat/cpu.h +7 -5

index c3f1a42..207f21b 100644
@@ -476,11 +476,12 @@ extern u32 omap_features;
 #define OMAP3_HAS_192MHZ_CLK		BIT(5)
 #define OMAP3_HAS_IO_WAKEUP		BIT(6)
 #define OMAP3_HAS_SDRC			BIT(7)
-#define OMAP3_HAS_IO_CHAIN_CTRL		BIT(8)
-#define OMAP3_HAS_PWROFF		BIT(9)
-#define OMAP4_HAS_MPU_1GHZ		BIT(10)
-#define OMAP4_HAS_MPU_1_2GHZ		BIT(11)
-#define OMAP4_HAS_MPU_1_5GHZ		BIT(12)
+#define OMAP3_HAS_SDRC_EMIF4		BIT(8)
+#define OMAP3_HAS_IO_CHAIN_CTRL		BIT(9)
+#define OMAP3_HAS_PWROFF		BIT(10)
+#define OMAP4_HAS_MPU_1GHZ		BIT(11)
+#define OMAP4_HAS_MPU_1_2GHZ		BIT(12)
+#define OMAP4_HAS_MPU_1_5GHZ		BIT(13)
 
 
 #define OMAP3_HAS_FEATURE(feat,flag)			\

@@ -497,6 +498,7 @@ OMAP3_HAS_FEATURE(isp, ISP)
 OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
 OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
 OMAP3_HAS_FEATURE(sdrc, SDRC)
+OMAP3_HAS_FEATURE(sdrc_emif4, SDRC_EMIF4)
 OMAP3_HAS_FEATURE(io_chain_ctrl, IO_CHAIN_CTRL)
 OMAP3_HAS_FEATURE(pwroff, PWROFF)
 

Signature · 1 line

1.7.9.4

Hi

Quoted 23 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

The typical SDRAM Controller Subsystem module (SDRC) on TI OMAP3 devices has two submodules: the SDRAM Memory Scheduler (SMS) submodule, and the SDRC submodule--the 'SDRC' acronym/term is overloaded. The am35x family of devices is different in that it has an EMIF4 submodule instead of an SDRC submodule. The SMS submodules are similar, though.

To allow code to determine whether the current device has an SDRC submodule or and EMIF4 submodule, add the 'OMAP3_HAS_SDRC_EMIF4' feature and set it when running on an am35x SoC.

So when:
- omap3_has_sdrc() returns true: the system has an SDRC module consisting of an SMS submodule and either an SDRC submodule or an EMIF4 submodule.
- omap3_has_sdrc_emif4() returns true: the system has an EMIF4 submodule. It is assumed that this feature will only be checked when there is an SDRC module present (i.e., when omap3_has_sdrc() would return true).

Quoted 1 line

Signed-off-by Mark A. Greer <[email protected]>

We need to stop declaring these feature flags for IP blocks, and use the hwmod code instead. hwmod code provides omap_hwmod_lookup() which can be used to determine whether a given IP block exists on a particular chip, so I don't think there's any need for omap3_has_sdrc(), omap3_has_sdrc_emif4(), omap3_has_iva(), omap3_has_isp(), etc.

So it would be best if you could simply add an EMIF hwmod data entry in omap_hwmod_3xxx_data.c.

From: "Mark A. Greer" <[email protected]>

The typical SDRAM Controller Subsystem module (SDRC) on TI OMAP3 devices has two submodules: the SDRAM Memory Scheduler (SMS) submodule, and the SDRC submodule--the 'SDRC' acronym/term is overloaded. The am35x family of devices is different in that it has an EMIF4 submodule instead of an SDRC submodule. The SMS submodules are similar, though.

Quoted 9 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

The typical SDRAM Controller Subsystem module (SDRC) on TI OMAP3 devices has two submodules: the SDRAM Memory Scheduler (SMS) submodule, and the SDRC submodule--the 'SDRC' acronym/term is overloaded. The am35x family of devices is different in that it has an EMIF4 submodule instead of an SDRC submodule. The SMS submodules are similar, though.

By the way, thanks for the great patch descriptions; they are very helpful.

From: "Mark A. Greer" <[email protected]>

The typical SDRAM Controller Subsystem module (SDRC) on TI OMAP3 devices has two submodules: the SDRAM Memory Scheduler (SMS) submodule, and the SDRC submodule--the 'SDRC' acronym/term is overloaded. The am35x family of devices is different in that it has an EMIF4 submodule instead of an SDRC submodule. The SMS submodules are similar, though.

cc Aneesh

Quoted 9 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

The typical SDRAM Controller Subsystem module (SDRC) on TI OMAP3 devices has two submodules: the SDRAM Memory Scheduler (SMS) submodule, and the SDRC submodule--the 'SDRC' acronym/term is overloaded. The am35x family of devices is different in that it has an EMIF4 submodule instead of an SDRC submodule. The SMS submodules are similar, though.

The best thing to do is to add an EMIF driver stub that is initialized based on hwmod data, rather than hacking this into the existing SDRC.

I don't know what the current state of the EMIF driver is. Aneesh, do you know what the current status is?

If it's not going upstream for some reason, then I'd suggest adding arch/arm/mach-omap2/emif.c for now.

From: "Mark A. Greer" <[email protected]>

The typical SDRAM Controller Subsystem module (SDRC) on TI OMAP3 devices has two submodules: the SDRAM Memory Scheduler (SMS) submodule, and the SDRC submodule--the 'SDRC' acronym/term is overloaded. The am35x family of devices is different in that it has an EMIF4 submodule instead of an SDRC submodule. The SMS submodules are similar, though.

The difference in SDRC/EMIF4 submodules is important because omap34xx_cpu_suspend() will ultimately access the submodule's registers and the register sets are different. To support the EMIF4 submodule, add the omap3_emif4_do_wfi() routine which roughly does for the EMIF4 submodule what omap3_do_wfi() does for the SDRC submodule. This requires omap34xx_cpu_suspend() to use a pointer set up in omap_push_sram_idle() so that it jumps to the correct omap3*_do_wfi() routine in either SDRAM or SRAM.

Credits: arch/arm/mach-omap2/sleep3517.S in TI's am35x SDK (05.03.02.00) which appears to be authored by Ranjith Lohithakshan <[email protected]> was used as a reference.

CC Ranjith Lohithakshan <[email protected]>

Signed-off-by Mark A. Greer <[email protected]>

Quoted 20 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

The typical SDRAM Controller Subsystem module (SDRC) on TI OMAP3 devices has two submodules: the SDRAM Memory Scheduler (SMS) submodule, and the SDRC submodule--the 'SDRC' acronym/term is overloaded. The am35x family of devices is different in that it has an EMIF4 submodule instead of an SDRC submodule. The SMS submodules are similar, though.

The difference in SDRC/EMIF4 submodules is important because omap34xx_cpu_suspend() will ultimately access the submodule's registers and the register sets are different. To support the EMIF4 submodule, add the omap3_emif4_do_wfi() routine which roughly does for the EMIF4 submodule what omap3_do_wfi() does for the SDRC submodule. This requires omap34xx_cpu_suspend() to use a pointer set up in omap_push_sram_idle() so that it jumps to the correct omap3*_do_wfi() routine in either SDRAM or SRAM.

Quoted 6 lines

Credits: arch/arm/mach-omap2/sleep3517.S in TI's am35x SDK (05.03.02.00) which appears to be authored by Ranjith Lohithakshan <[email protected]> was used as a reference.

CC Ranjith Lohithakshan <[email protected]>

Signed-off-by Mark A. Greer <[email protected]>

Dumb Q: do you actually need to do the EMIF4 self-refresh control from SRAM?

The reason I ask is because on OMAP3, we tried going down the path of not running any of this from SRAM (run it from cache instead.) However, due to some errata (c.f. wait_sdrc*, wait_dll_* ...), we had to handle these errata from SRAM.

Looking at your omap3_emif4_do_wfi, it looks like that's very minimal, and can probably be prefetched/run from cache.

arch/arm/mach-omap2/emif4.h new file +24 -0

new file mode 100644
index 0000000..0126af3
@@ -0,0 +1,24 @@
+/*
+ * SDRC Module, EMIF4 Submodule Definitions
+ *
+ * Author: Mark A. Greer <[email protected]>
+ *
+ * Copyright (c) 2012 by Animal Creek Technologies, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef ____ASM_ARCH_EMIF4_H
+#define ____ASM_ARCH_EMIF4_H
+
+#ifdef __ASSEMBLER__
+
+#define OMAP34XX_EMIF4_REGADDR(reg) \
+		OMAP2_L3_IO_ADDRESS(OMAP343X_EMIF4_BASE + (reg))
+
+#endif
+
+#define EMIF4_PWR_MGMT_CTRL	0x38
+
+#endif

arch/arm/mach-omap2/pm.h +5 -2

index 36fa90b..ab1262b 100644
@@ -85,10 +85,13 @@ extern unsigned int omap24xx_cpu_suspend_sz;
 /* 3xxx */
 extern void omap34xx_cpu_suspend(int save_state);
 
-/* omap3_do_wfi function pointer and size, for copy to SRAM */
+/* omap3*_do_wfi function pointers and sizes, for copy to SRAM */
 extern void omap3_do_wfi(void);
 extern unsigned int omap3_do_wfi_sz;
-/* ... and its pointer from SRAM after copy */
+extern void omap3_emif4_do_wfi(void);
+extern unsigned int omap3_emif4_do_wfi_sz;
+/* ... and pointers when in SDRAM and in SRAM after copy */
+extern void (*omap3_do_wfi_sdram)(void);
 extern void (*omap3_do_wfi_sram)(void);
 
 /* save_secure_ram_context function pointer and size, for copy to SRAM */

arch/arm/mach-omap2/pm34xx.c +10 -1

index 00c4abe..c1dee25 100644
@@ -66,6 +66,7 @@ struct power_state {
 static LIST_HEAD(pwrst_list);
 
 static int (*_omap_save_secure_sram)(u32 *addr);
+void (*omap3_do_wfi_sdram)(void);
 void (*omap3_do_wfi_sram)(void);
 
 static struct powerdomain *mpu_pwrdm, *neon_pwrdm;

@@ -694,7 +695,15 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
  */
 void omap_push_sram_idle(void)
 {
-	omap3_do_wfi_sram = omap_sram_push(omap3_do_wfi, omap3_do_wfi_sz);
+	if (omap3_has_sdrc_emif4()) {
+		omap3_do_wfi_sdram = omap3_emif4_do_wfi;
+		omap3_do_wfi_sram = omap_sram_push(omap3_emif4_do_wfi,
+				omap3_emif4_do_wfi_sz);
+	} else {
+		omap3_do_wfi_sdram = omap3_do_wfi;
+		omap3_do_wfi_sram = omap_sram_push(omap3_do_wfi,
+				omap3_do_wfi_sz);
+	}
 
 	if (omap_type() != OMAP2_DEVICE_TYPE_GP)
 		_omap_save_secure_sram = omap_sram_push(save_secure_ram_context,

arch/arm/mach-omap2/sleep34xx.S +16 -5

index 1f62f23..22aac4c 100644
@@ -33,6 +33,7 @@
 #include "cm2xxx_3xxx.h"
 #include "prm2xxx_3xxx.h"
 #include "sdrc.h"
+#include "emif4.h"
 #include "control.h"
 
 /*

@@ -67,6 +68,8 @@
 #define SDRC_DLLA_STATUS_V	OMAP34XX_SDRC_REGADDR(SDRC_DLLA_STATUS)
 #define SDRC_DLLA_CTRL_V	OMAP34XX_SDRC_REGADDR(SDRC_DLLA_CTRL)
 
+#define PWR_MGMT_CTRL_V		OMAP34XX_EMIF4_REGADDR(EMIF4_PWR_MGMT_CTRL)
+
 /*
  * This file needs be built unconditionally as ARM to interoperate correctly
  * with non-Thumb-2-capable firmware.

@@ -163,8 +166,12 @@ ENTRY(omap34xx_cpu_suspend)
 	 */
 
 	/*
-	 * For OFF mode: save context and jump to WFI in SDRAM (omap3_do_wfi)
-	 * For non-OFF modes: jump to the WFI code in SRAM (omap3_do_wfi_sram)
+	 * For OFF mode: save context and jump to WFI in SDRAM
+	 * (omap3_do_wfi_sdram). For non-OFF modes: jump to the WFI code
+	 * in SRAM (omap3_do_wfi_sram).  Note that omap_push_sram_idle()
+	 * will have set omap3_do_wfi_sdram and omap3_do_wfi_sram to the
+	 * SDRAM and SRAM addresses of either omap3_do_wfi or
+	 * omap3_emif4_do_wfi depending on the type of SDRC submodule.
 	 */
 	ldr	r4, omap3_do_wfi_sram_addr
 	ldr	r5, [r4]

@@ -216,11 +223,15 @@ save_context_wfi:
  THUMB(	nop		)
 	.arm
 
-	b	omap3_do_wfi
+	ldr	r4, omap3_do_wfi_sdram_addr
+	ldr	r5, [r4]
+	bx	r5			@  jump to the WFI code in SRAM
 
 /*
  * Local variables
  */
+omap3_do_wfi_sdram_addr:
+	.word omap3_do_wfi_sdram
 omap3_do_wfi_sram_addr:
 	.word omap3_do_wfi_sram
 kernel_flush:

@@ -232,8 +243,7 @@ kernel_flush:
  */
 
 /*
- * Do WFI instruction
- * Includes the resume path for non-OFF modes
+ * Do WFI instruction (SDRC module has SDRC submodule)
  *
  * This code gets copied to internal SRAM and is accessible
  * from both SDRAM and SRAM:

@@ -391,6 +401,52 @@ wait_dll_lock_counter:
 ENTRY(omap3_do_wfi_sz)
 	.word	. - omap3_do_wfi
 
+/*

arch/arm/mach-omap2/sleep34xx.S +6 -0

+
+	dsb
+	dmb
+
+	wfi
+

arch/arm/mach-omap2/sleep34xx.S +16 -0

+	ldr     r4, pwr_mgmt_ctrl
+	ldr     r5, [r4]
+	bic     r5, r5, #0x200
+	str     r5, [r4]
+
+	ldmfd	sp!, {r4 - r11, pc}	@ restore regs and return
+
+pwr_mgmt_ctrl:
+	.word   PWR_MGMT_CTRL_V
+ENDPROC(omap3_emif4_do_wfi)
+ENTRY(omap3_emif4_do_wfi_sz)
+	.word	. - omap3_emif4_do_wfi
+
 
 /*
  * ==============================

@@ -564,6 +620,9 @@ l2dis_3630:
 /*
  * This function implements the erratum ID i443 WA, applies to 34xx >= ES3.0
  * Copied to and run from SRAM in order to reconfigure the SDRC parameters.
+ *
+ * Note: Never call this routine when running on an am35x device since it
+ *  has an EMIF4 submodule instead of the standard SDRC submodule.
  */
 	.text
 	.align	3

arch/arm/plat-omap/include/plat/omap34xx.h +1 -0

index 0d818ac..3a84520 100644
@@ -42,6 +42,7 @@
 #define OMAP3430_PRM_BASE	0x48306800
 #define OMAP343X_SMS_BASE	0x6C000000
 #define OMAP343X_SDRC_BASE	0x6D000000
+#define OMAP343X_EMIF4_BASE	0x6D000000
 #define OMAP34XX_GPMC_BASE	0x6E000000
 #define OMAP343X_SCM_BASE	0x48002000
 #define OMAP343X_CTRL_BASE	OMAP343X_SCM_BASE

Signature · 1 line

1.7.9.4

Hi

just a few comments based on a quick look

Quoted 10 lines · Mark A. Greer

arch/arm/mach-omap2/sleep34xx.S +1 -0

index 1f62f23..22aac4c 100644
@@ -391,6 +401,52 @@ wait_dll_lock_counter:
 ENTRY(omap3_do_wfi_sz)
 	.word	. - omap3_do_wfi
 
+/*

arch/arm/mach-omap2/sleep34xx.S wait_dll_lock_counter: · line 414

Quoted 10 lines

arch/arm/mach-omap2/sleep34xx.S +10 -0

+ * Do WFI instruction (SDRC module has EMIF4 submodule)
+ *
+ * This code gets copied to internal SRAM and is accessible
+ * from both SDRAM and SRAM:
+ * - executed from SRAM for non-off modes (omap3_emif4_do_wfi_sram),
+ * - executed from SDRAM for OFF mode (omap3_emif4_do_wfi).
+ */
+	.align	3
+ENTRY(omap3_emif4_do_wfi)
+	/* Put EMIF in self-refresh */

Hmm, I guess this comment isn't quite right; it just tells it to go to self-refresh when it's "idle" ? (see below)

arch/arm/mach-omap2/sleep34xx.S wait_dll_lock_counter: · line 417

Quoted 3 lines

arch/arm/mach-omap2/sleep34xx.S +3 -0

+	ldr     r4, pwr_mgmt_ctrl
+	ldr     r5, [r4]
+	orr     r5, r5, #0x200

Maybe use a macro here in place of the #0x200 to indicate what it's trying to set...

Do you happen to know what "idle" means in the context of SPRUGR0B Section 9.2.3.4.5.1 "SDRAM Self-Refresh Mode" ? Is it referring to target module-level idle, e.g. SIdleReq; or is it referring to an internal definition of idle, e.g., something like "no reads or writes from/to the SDRAM" ? Am wondering if this should just be set once during EMIF initialization.

arch/arm/mach-omap2/sleep34xx.S wait_dll_lock_counter: · line 418

Quoted 2 lines

arch/arm/mach-omap2/sleep34xx.S +1 -0

+	str     r5, [r4]

Might be good to do a readback after this to ensure that the store has made it to the EMIF.

Also, scanning SPRUGR0B, it looks like this setting is dependent partially on the REG_PM_TIM field in the same register. Might be good to set REG_PM_TIM during EMIF init to avoid any bootloader dependency.

Quoted 11 lines

arch/arm/mach-omap2/sleep34xx.S +11 -0

+
+	dsb
+	dmb
+
+	wfi
+
+	nop
+	nop
+	nop
+	nop
+	nop

arch/arm/mach-omap2/sleep34xx.S wait_dll_lock_counter: · line 429

Quoted 5 lines

arch/arm/mach-omap2/sleep34xx.S +5 -0

+	nop
+	nop
+	nop
+	nop
+	nop

Are these NOPs necessary? Is the number of NOPs dependent on CPU or bus speed or some ARM parameter?

arch/arm/mach-omap2/sleep34xx.S +16 -0

+ENTRY(omap3_emif4_do_wfi)
+	/* Put EMIF in self-refresh */
+	ldr     r4, pwr_mgmt_ctrl
+	ldr     r5, [r4]
+	orr     r5, r5, #0x200
+	str     r5, [r4]
+
+	dsb
+	dmb
+
+	wfi
+
+	nop
+	nop
+	nop
+	nop

From: "Mark A. Greer" <[email protected]>

Problem:
--------
When resuming from a Suspend-to-RAM event, the eth0 device (davinci emac/mdio) on am35xx boards wouldn't work and the following error message appeared on the console:

"net eth0: could not connect to phy davinci_mdio-0:00"

cc Ilya

Quoted 9 lines · Mark A. Greer

From: "Mark A. Greer" <[email protected]>

Problem:
--------
When resuming from a Suspend-to-RAM event, the eth0 device (davinci emac/mdio) on am35xx boards wouldn't work and the following error message appeared on the console:

"net eth0: could not connect to phy davinci_mdio-0:00"

...

Quoted 5 lines

Signed-off-by Mark A. Greer <[email protected]>

---
 arch/arm/mach-omap2/am35xx-emac.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

arch/arm/mach-omap2/am35xx-emac.c

Quoted 4 lines

arch/arm/mach-omap2/am35xx-emac.c +0 -0

index 1f97e74..d23bd6c 100644

This is a little off-topic from this patch, but we should get rid of this arch/arm/mach-omap2/am35xx-emac.c file. These on-chip devices should be registered via the hwmod code. Looking at arch/arm/mach-omap2/omap_hwmod_3xxx_data.c, I don't see any trace of the EMAC or MDIO in there. That's where all of these IPSS IP blocks should be declared...