ARM: OMAP: core/hwmod: first set of fixes for 3.5-rc

Kuvin discovered that commit c8d82ff68fb6873691536cf33021977efbf5593c

("ARM: OMAP2/3: hwmod data: Add 32k-sync timer data to hwmod database") broke CORE idle on OMAP3. This prevents device low power states.

The root cause is that the 32K sync timer IP block does not support smart-idle mode[1], and so the hwmod code keeps the IP block in no-idle mode while it is active. This in turn prevents the WKUP clockdomain from transitioning to idle. There is a hardcoded sleep dependency that prevents the CORE_L3 and CORE_CM clockdomains from transitioning to idle when the WKUP clockdomain is active[2], so the chip cannot enter any device low power states.

It turns out that there is no need to take the 32k sync timer out of idle.
The IP block itself pbobably does not have any native idle

handling at all, due to its simplicity.

Furthermore, the PRCM will
never request target idle for this IP block while the kernel is running, due to the sleep dependency that prevents the WKUP clockdomain from idling while the CORE_L3 clockdomain is active. So we can safely leave the 32k sync timer in target-no-idle mode, even while we continue to access it.

This workaround is implemented by defining a new hwmod flag, HWMOD_ALWAYS_FORCE_SIDLE, that places the IP block into target force-idle mode even when enabled. The 32k sync timer hwmods are marked with this flag for OMAP3 and OMAP4 SoCs. (On OMAP2xxx, no OCP header existed on the 32k sync timer.)

Another theoretically clean fix for this problem would be to implement PM runtime-based control for 32k sync timer accesses. These PM runtime calls would need to located in a custom clocksource, since the 32k sync timer is currently used as an MMIO clocksource. But in practice, there would be little benefit to doing so; and there would be some cost, due to the addition of unnecessary lines of code and the additional CPU overhead of the PM runtime and hwmod code - unnecessary in this case.

Another possible fix would have been to modify the pm34xx.c code to force the IP block idle before entering WFI. But this would not have been an acceptable approach: we are trying to remove this type of centralized IP block idle control from the PM code.

This patch is effectively a workaround for a hardware problem. A better hardware approach would have been to implement a smart-idle target idle mode for this IP block. The smart-idle mode in this case would behave identically to the force-idle mode. We consider the force-idle and no-idle target idle mode settings to be intended for debugging and automatic idle management bug workarounds only[4].

This patch is a collaboration between Kevin Hilman <[email protected]> and Paul Walmsley <[email protected]>.

Thanks to Vaibhav Hiremath <[email protected]> for providing comments on an earlier version of this patch. Thanks to Tero Kristo <[email protected]> for identifying a bug in an earlier version of this patch. Thanks to Beno?t Cousson <[email protected]> for identifying a bug in an earlier version of this patch and for implementation comments.

References:

1. Table 16-96 "REG_32KSYNCNT_SYSCONFIG" of the OMAP34xx TRM Rev. ZU

    (SWPU223U), available from:
    http://www.ti.com/pdfs/wtbu/OMAP34x_ES3.1.x_PUBLIC_TRM_vzU.zip

2. Table 4-72 "Sleep Dependencies" of the OMAP34xx TRM Rev. ZU

    (SWPU223U)

3. ibid.

4. Section 3.1.1.1.2 "Module-Level Clock Management" of the OMAP4430

    TRM Rev. vAA (SWPU231AA).

Cc Tony Lindgren <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

Cc Beno?t Cousson <[email protected]>

Cc Tero Kristo <[email protected]>

Tested-by Kevin Hilman <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

Signed-off-by Paul Walmsley <[email protected]>

---
  arch/arm/mach-omap2/omap_hwmod.c             |   17 +++++++++++------
  arch/arm/mach-omap2/omap_hwmod_3xxx_data.c   |    2 +-
  arch/arm/mach-omap2/omap_hwmod_44xx_data.c   |    2 +-
  arch/arm/plat-omap/include/plat/omap_hwmod.h |    9 +++++++++
  4 files changed, 22 insertions(+), 8 deletions(-)

arch/arm/mach-omap2/omap_hwmod.c +11 -6

index bf86f7e..096474c 100644
@@ -1124,10 +1124,12 @@ static struct omap_hwmod_addr_space * __init _find_mpu_rt_addr_space(struct omap
   * _enable_sysc - try to bring a module out of idle via OCP_SYSCONFIG
   * @oh: struct omap_hwmod *
   *
- * If module is marked as SWSUP_SIDLE, force the module out of slave
- * idle; otherwise, configure it for smart-idle.  If module is marked
- * as SWSUP_MSUSPEND, force the module out of master standby;
- * otherwise, configure it for smart-standby.  No return value.
+ * Ensure that the OCP_SYSCONFIG register for the IP block represented
+ * by @oh is set to indicate to the PRCM that the IP block is active.
+ * Usually this means placing the module into smart-idle mode and
+ * smart-standby, but if there is a bug in the automatic idle handling
+ * for the IP block, it may need to be placed into the force-idle or
+ * no-idle variants of these modes.  No return value.
   */
  static void _enable_sysc(struct omap_hwmod *oh)
  {

@@ -1141,8 +1143,11 @@ static void _enable_sysc(struct omap_hwmod *oh)
  	sf = oh->class->sysc->sysc_flags;
 
  	if (sf & SYSC_HAS_SIDLEMODE) {
-		idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
-			HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
+		if (oh->flags & HWMOD_ALWAYS_FORCE_SIDLE)
+			idlemode = HWMOD_IDLEMODE_FORCE;
+		else
+			idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
+				HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
  		_set_slave_idlemode(oh, idlemode, &v);
  	}
 

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

index b26d3c9..f8ac9e7 100644

Quoted 1 line · Paul Walmsley

Kuvin discovered that commit c8d82ff68fb6873691536cf33021977efbf5593c

I guess you meant Kevin?

Quoted 1 line

The IP block itself pbobably does not have any native idle

typo

Quoted 1 line

handling at all, due to its simplicity.

I don't think this description is really accurate, due to the usual confusing definition of IDLE for the PRCM standpoint :-)
The counter_32k will follow PRCM requirement toward SIdleReq/SIdleAck. It has to be "idle" for PRCM standpoint to allow the transition of the L4_WKUP to inactive (SIdleAck=IDLE). And it will be functional as soon as the L4_WKUP domain is active.

The fact that the smartidle mode is missing does not change anything in the way the PRCM handle the protocol. It is just an issue at IP level.

In that case force-idle = smart-idle, just because this module does not have anything to do to delay the SIdleAck upon SIdleReq request. The IP is probably connecting the SIdleAck to the SIdleReq signal.
Please note that there are a bunch of IPs that are doing that even if they do support the smartidle mode.

Quoted 5 lines

Furthermore, the PRCM will never request target idle for this IP block while the kernel is running, due to the sleep dependency that prevents the WKUP clockdomain from idling while the CORE_L3 clockdomain is active. So we can safely leave the 32k sync timer in target-no-idle mode, even while we continue to access it.

Do you mean force-idle? Because accessing a module in no-idle is always possible.

Quoted 5 lines

This workaround is implemented by defining a new hwmod flag, HWMOD_ALWAYS_FORCE_SIDLE, that places the IP block into target force-idle mode even when enabled. The 32k sync timer hwmods are marked with this flag for OMAP3 and OMAP4 SoCs. (On OMAP2xxx, no OCP header existed on the 32k sync timer.)

I still don't see the need for this flag. It looks to me that we are adding a redundant information and thus make things more complex than it should.

.idlemodes = (SIDLE_FORCE | SIDLE_NO),

It the real root cause of the problem. There is no need to re-encode that using an extra flag. Especially at hwmod level where the issue is at sysconfig level.

I did not really get the reason why you changed your mind on that point.

As soon as there is no SIDLE_SMART mode, what choice do we have but using the SIDLE_FORCE?

BTW, I even think the HWMOD_SWSUP_SIDLE hwmod flag is useless now. It was needed before probably because the idlemodes were wrongly populated.

In fact the hwmod flags is really there to *flag* some real HW bug we cannot figure out otherwise, but in that case, the sysc.idlemodes already contains all the information we need to set the proper mode during enable/disable.

Duplicating the information is always a source of confusion and might lead to nasty bugs due to the increase of complexity.

Quoted 8 lines

Another theoretically clean fix for this problem would be to implement PM runtime-based control for 32k sync timer accesses. These PM runtime calls would need to located in a custom clocksource, since the 32k sync timer is currently used as an MMIO clocksource. But in practice, there would be little benefit to doing so; and there would be some cost, due to the addition of unnecessary lines of code and the additional CPU overhead of the PM runtime and hwmod code - unnecessary in this case.

I don't think that part is really relevant anymore.

Quoted 4 lines

Another possible fix would have been to modify the pm34xx.c code to force the IP block idle before entering WFI. But this would not have been an acceptable approach: we are trying to remove this type of centralized IP block idle control from the PM code.

Neither that one I guess. As soon as we consider force-idle to be equivalent to smart-idle, nothing more is needed.

Hi

Quoted 4 lines · Benoit

Quoted 1 line · Paul Walmsley

Kuvin discovered that commit c8d82ff68fb6873691536cf33021977efbf5593c

I guess you meant Kevin?

...

Quoted 2 lines

Quoted 1 line

The IP block itself pbobably does not have any native idle

Quoted 1 line

typo

Thanks for noticing these, but they are not in what was sent out by the list server:

http://www.spinics.net/lists/linux-omap/msg71503.html

Some software/hardware issue on your end, maybe?

Quoted 10 lines

Quoted 2 lines

handling at all, due to its simplicity.

I don't think this description is really accurate, due to the usual confusing definition of IDLE for the PRCM standpoint :-)
The counter_32k will follow PRCM requirement toward SIdleReq/SIdleAck. It has to be "idle" for PRCM standpoint to allow the transition of the L4_WKUP to inactive (SIdleAck=IDLE). And it will be functional as soon as the L4_WKUP domain is active.

Quoted 8 lines

The fact that the smartidle mode is missing does not change anything in the way the PRCM handle the protocol. It is just an issue at IP level.

In that case force-idle = smart-idle, just because this module does not have anything to do to delay the SIdleAck upon SIdleReq request. The IP is probably connecting the SIdleAck to the SIdleReq signal.
Please note that there are a bunch of IPs that are doing that even if they do support the smartidle mode.

Right, that's exactly my point -- perhaps made in an unclear way.

My point was that the 32k counter IP block doesn't do anything with the incoming IdleReq signal to determine whether or not to assert IdleAck back to the PRCM. But rather than implementing smart-idle mode to handle this, like the other IP blocks do, the only two modes implemented were the debugging modes, force-idle and no-idle.

So depending on one's point of view, this patch is either:

1. a hardware bug workaround, because the hardware should have a smart-idle mode that acts the same way as the force-idle mode, just like the other IP blocks do; or

2. a software workaround, because we don't have a 32k counter device driver that implements runtime PM around counter reads.

Of course #2 would be rather pointless and the patch description tries to convey this too.

Quoted 6 lines

Quoted 5 lines

Furthermore, the PRCM will never request target idle for this IP block while the kernel is running, due to the sleep dependency that prevents the WKUP clockdomain from idling while the CORE_L3 clockdomain is active. So we can safely leave the 32k sync timer in target-no-idle mode, even while we continue to access it.

Quoted 2 lines

Do you mean force-idle? Because accessing a module in no-idle is always possible.

Thanks, that's indeed a description bug.

Quoted 11 lines

Quoted 5 lines

This workaround is implemented by defining a new hwmod flag, HWMOD_ALWAYS_FORCE_SIDLE, that places the IP block into target force-idle mode even when enabled. The 32k sync timer hwmods are marked with this flag for OMAP3 and OMAP4 SoCs. (On OMAP2xxx, no OCP header existed on the 32k sync timer.)

I still don't see the need for this flag. It looks to me that we are adding a redundant information and thus make things more complex than it should.

.idlemodes = (SIDLE_FORCE | SIDLE_NO),

Quoted 8 lines

It the real root cause of the problem. There is no need to re-encode that using an extra flag. Especially at hwmod level where the issue is at sysconfig level.

I did not really get the reason why you changed your mind on that point.

As soon as there is no SIDLE_SMART mode, what choice do we have but using the SIDLE_FORCE?

SIDLE_NO is the option that makes more sense to me.

Consider an IP block with SIDLE_NO and SIDLE_FORCE but without SIDLE_SMART. When an initiator will access it, the default setting should be SIDLE_NO, for the reason that you identified above: "because accessing a module in no-idle is always possible." On the other hand, we don't know when it's safe to access a module in SIDLE_FORCE unless we have additional information as to how the IP block handles the SIdleReq signal internally, and the characteristics of the clock domain in which it's integrated.

For
example, as mentioned earlier in the discussion on this patch, the AM335x documentation states "By definition, initiator may generate read/write transaction as long as it is out of IDLE state."

Quoted 7 lines

BTW, I even think the HWMOD_SWSUP_SIDLE hwmod flag is useless now. It was needed before probably because the idlemodes were wrongly populated.

In fact the hwmod flags is really there to *flag* some real HW bug we cannot figure out otherwise, but in that case, the sysc.idlemodes already contains all the information we need to set the proper mode during enable/disable.

Quoted 2 lines

Duplicating the information is always a source of confusion and might lead to nasty bugs due to the increase of complexity.

You may be misunderstanding the meaning of the HWMOD_SWSUP_SIDLE flag. It was added to work around IP blocks that had broken smart-idle. These modules definitely do exist; see for example the OMAP3 wdt2, usbhsotg, and usb_host_hs hwmods as some examples.

It might be reasonable to remove the HWMOD_SWSUP_SIDLE flag from the 32k counter and just test again for HWMOD_ALWAYS_FORCE_SIDLE in the module disable. I'll take a look at this.

Quoted 9 lines

Quoted 8 lines

Another theoretically clean fix for this problem would be to implement PM runtime-based control for 32k sync timer accesses. These PM runtime calls would need to located in a custom clocksource, since the 32k sync timer is currently used as an MMIO clocksource. But in practice, there would be little benefit to doing so; and there would be some cost, due to the addition of unnecessary lines of code and the additional CPU overhead of the PM runtime and hwmod code - unnecessary in this case.

Quoted 1 line

I don't think that part is really relevant anymore.

Why?

Quoted 7 lines

Quoted 4 lines

Another possible fix would have been to modify the pm34xx.c code to force the IP block idle before entering WFI. But this would not have been an acceptable approach: we are trying to remove this type of centralized IP block idle control from the PM code.

Neither that one I guess. As soon as we consider force-idle to be equivalent to smart-idle, nothing more is needed.

But they are definitely not equivalent.

Quoted 1 line · Paul Walmsley

On Thu, 14 Jun 2012, Cousson, Benoit wrote:

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Furthermore, the PRCM will never request target idle for this IP block while the kernel is running, due to the sleep dependency that prevents the WKUP clockdomain from idling while the CORE_L3 clockdomain is active. So we can safely leave the 32k sync timer in target-no-idle mode, even while we continue to access it.

Quoted 4 lines

Quoted 2 lines

Do you mean force-idle? Because accessing a module in no-idle is always possible.

Thanks, that's indeed a description bug.

I'm not sure to follow you? My point was it should be: "So we can safely leave the 32k sync timer in force-idle mode, even while we continue to access it." This is what the WA is doing.

Quoted 9 lines

SIDLE_NO is the option that makes more sense to me.

Consider an IP block with SIDLE_NO and SIDLE_FORCE but without SIDLE_SMART. When an initiator will access it, the default setting should be SIDLE_NO, for the reason that you identified above: "because accessing a module in no-idle is always possible." On the other hand, we don't know when it's safe to access a module in SIDLE_FORCE unless we have additional information as to how the IP block handles the SIdleReq signal internally, and the characteristics of the clock domain in which it's integrated.

This is the modulemode and clkdm static dependency / dynamic on OMAP3/4 that will guaranty that the OCP clock will be enabled upon any access to this L4_WKUP clock domain IPs. This has nothing to do with the SIDLE mode.

Quoted 3 lines

It might be reasonable to remove the HWMOD_SWSUP_SIDLE flag from the 32k counter and just test again for HWMOD_ALWAYS_FORCE_SIDLE in the module disable. I'll take a look at this.

Both should be removed as explained before. There is clearly no need for HWMOD_ALWAYS_FORCE_SIDLE.

We are already explicitly listing the limitation through the idlemodes attribute. Only SIDLE_FORCE and SIDLE_NO are supported, so we already know that SIDLE_FORCE is the proper way to fix that limitation for the current OMAPs. Since there is no other IP with such limitation, we know as well that there will be no side effect. If, in the future, some more IPs will have that limitation and will not work as expected, it will mean that some other HW bugs will be there, and only these ones will have to be flagged.

But my point is let's keep it simple and not try to anticipate future bugs. This flag is not require today, let's not add it.

Quoted 2 lines · Benoit

Quoted 1 line · Paul Walmsley

On Thu, 14 Jun 2012, Cousson, Benoit wrote:

(attribution lost)

Quoted 11 lines

Quoted 10 lines

Quoted 8 lines

Quoted 5 lines

Furthermore, the PRCM will never request target idle for this IP block while the kernel is running, due to the sleep dependency that prevents the WKUP clockdomain from idling while the CORE_L3 clockdomain is active. So we can safely leave the 32k sync timer in target-no-idle mode, even while we continue to access it.

Do you mean force-idle? Because accessing a module in no-idle is always possible.

Thanks, that's indeed a description bug.

Quoted 3 lines

I'm not sure to follow you? My point was it should be: "So we can safely leave the 32k sync timer in force-idle mode, even while we continue to access it." This is what the WA is doing.

I am expressing appreciation to you for pointing out something incorrect in the patch description, which has been fixed in the current version of the patch.

Quoted 8 lines

Quoted 8 lines

SIDLE_NO is the option that makes more sense to me.>>> Consider an IP block with SIDLE_NO and SIDLE_FORCE but without SIDLE_SMART. When an initiator will access it, the default setting should be SIDLE_NO, for the reason that you identified above: "because accessing a module in no-idle is always possible." On the other hand, we don't know when it's safe to access a module in SIDLE_FORCE unless we have additional information as to how the IP block handles the SIdleReq signal internally, and the characteristics of the clock domain in which it's integrated.

...

Quoted 3 lines

This is the modulemode and clkdm static dependency / dynamic on OMAP3/4 that will guaranty that the OCP clock will be enabled upon any access to this L4_WKUP clock domain IPs. This has nothing to do with the SIDLE mode.

I want to implement a behavior that does not implicitly assume that an IP block without smart-idle will only exist inside clockdomains which are guaranteed to be active when the kernel is running. That might be true for current WBU chips, but it seems unwise to make that assumption in general.

Quoted 16 lines

Quoted 3 lines

It might be reasonable to remove the HWMOD_SWSUP_SIDLE flag from the 32k counter and just test again for HWMOD_ALWAYS_FORCE_SIDLE in the module disable. I'll take a look at this.

Both should be removed as explained before. There is clearly no need for HWMOD_ALWAYS_FORCE_SIDLE.

We are already explicitly listing the limitation through the idlemodes attribute. Only SIDLE_FORCE and SIDLE_NO are supported, so we already know that SIDLE_FORCE is the proper way to fix that limitation for the current OMAPs. Since there is no other IP with such limitation, we know as well that there will be no side effect. If, in the future, some more IPs will have that limitation and will not work as expected, it will mean that some other HW bugs will be there, and only these ones will have to be flagged.

Quoted 2 lines

But my point is let's keep it simple and not try to anticipate future bugs. This flag is not require today, let's not add it.

Which of these two behaviors do you feel is more "future-proof," in general:

1. Implementing a target idle policy that could break if a hardware designer were to skip adding a target smart-idle mode to a module in a clockdomain that might go idle while the kernel was active?

2. Implementing a target idle policy that is guaranteed to allow initiator accesses to succeed by definition?

considering that the implementation cost of either approach is identical?

My point is simple, we should not add any new custom flag when all the information to detect such exception are already there in the current data and represent accurately what the HW is doing.

So the updated patch below uses a clockdomain data flag for this instead.

Quoted 2 lines · Paul Walmsley

So the updated patch below uses a clockdomain data flag for this instead.

Here's a version that's a little cleaner. No functional changes.

- Paul

From: Paul Walmsley <[email protected]>
Date: Wed, 4 Jul 2012 05:22:53 -0600
Subject: [PATCH] ARM: OMAP2+: hwmod code/clockdomain data: fix 32K sync timer

Kevin discovered that commit c8d82ff68fb6873691536cf33021977efbf5593c ("ARM: OMAP2/3: hwmod data: Add 32k-sync timer data to hwmod database") broke CORE idle on OMAP3. This prevents device low power states.

The root cause is that the 32K sync timer IP block does not support smart-idle mode[1], and so the hwmod code keeps the IP block in no-idle mode while it is active. This in turn prevents the WKUP clockdomain from transitioning to idle. There is a hardcoded sleep dependency that prevents the CORE_L3 and CORE_CM clockdomains from transitioning to idle when the WKUP clockdomain is active[2], so the chip cannot enter any device low power states.

It turns out that there is no need to take the 32k sync timer out of idle. The IP block itself probably does not have any native idle handling at all, due to its simplicity. Furthermore, the PRCM will never request target idle for this IP block while the kernel is running, due to the sleep dependency that prevents the WKUP clockdomain from idling while the CORE_L3 clockdomain is active. So we can safely leave the 32k sync timer in target-force-idle mode, even while we continue to access it.

This workaround is implemented by defining a new clockdomain flag, CLKDM_ACTIVE_WITH_MPU, that indicates that the clockdomain is guaranteed to be active whenever the MPU is inactive. If an IP block's main functional clock exists inside this clockdomain, and the IP block does not support smart-idle modes, then the hwmod code will place the IP block into target force-idle mode even when enabled. The WKUP clockdomains on OMAP3/4 are marked with this flag. (On OMAP2xxx,
no OCP header existed on the 32k sync timer.) Other clockdomains also
should be marked with this flag, but those changes are deferred until a later merge window, to create a minimal fix.

Another theoretically clean fix for this problem would be to implement PM runtime-based control for 32k sync timer accesses. These PM runtime calls would need to located in a custom clocksource, since the 32k sync timer is currently used as an MMIO clocksource. But in practice, there would be little benefit to doing so; and there would be some cost, due to the addition of unnecessary lines of code and the additional CPU overhead of the PM runtime and hwmod code - unnecessary in this case.

Another possible fix would have been to modify the pm34xx.c code to force the IP block idle before entering WFI. But this would not have been an acceptable approach: we are trying to remove this type of centralized IP block idle control from the PM code.

This patch is a collaboration between Kevin Hilman <[email protected]> and Paul Walmsley <[email protected]>.

Thanks to Vaibhav Hiremath <[email protected]> for providing comments on an earlier version of this patch. Thanks to Tero Kristo <[email protected]> for identifying a bug in an earlier version of this patch. Thanks to Benoît Cousson <[email protected]> for identifying some bugs in an earlier version of this patch and for implementation comments.

References:

1. Table 16-96 "REG_32KSYNCNT_SYSCONFIG" of the OMAP34xx TRM Rev. ZU (SWPU223U), available from:

   http://www.ti.com/pdfs/wtbu/OMAP34x_ES3.1.x_PUBLIC_TRM_vzU.zip

2. Table 4-72 "Sleep Dependencies" of the OMAP34xx TRM Rev. ZU (SWPU223U)

3. ibid.

Cc Tony Lindgren <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

Cc Benoît Cousson <[email protected]>

Cc Tero Kristo <[email protected]>

Cc Kevin Hilman <[email protected]>

Signed-off-by Paul Walmsley <[email protected]>

Tested-by Kevin Hilman <[email protected]>

I confirm this version is now allowing CORE to hit retention during suspend.

Tested-by Kevin Hilman <[email protected]>

I confirm this version is now allowing CORE to hit retention during suspend.

Benoit, I hope this is OK with you. We need a fix for this in v3.5 since this is last core bug preventing CORE retention in v3.5.

Hi Kevin,

On 07/04/2012 04:27 PM, Kevin Hilman wrote:

[...]

Quoted 5 lines

Tested-by Kevin Hilman <[email protected]>

I confirm this version is now allowing CORE to hit retention during suspend.

Quoted 2 lines

Benoit, I hope this is OK with you. We need a fix for this in v3.5 since this is last core bug preventing CORE retention in v3.5.

On OMAP4 as well? I've just tried with the hwmod fix for AESS and USB and still cannot reach RET.

Am I missing some other patches for OMAP4?

Quoted 14 lines · Benoit Cousson

Hi Kevin,

On 07/04/2012 04:27 PM, Kevin Hilman wrote:

[...]

Quoted 7 lines

Tested-by Kevin Hilman <[email protected]>

I confirm this version is now allowing CORE to hit retention during suspend.

Benoit, I hope this is OK with you. We need a fix for this in v3.5 since this is last core bug preventing CORE retention in v3.5.

Quoted 4 lines

On OMAP4 as well? I've just tried with the hwmod fix for AESS and USB and still cannot reach RET.

Am I missing some other patches for OMAP4?

You need also fixes for sl2if and mcpdm in addition to above.

Quoted 8 lines · Tero Kristo

Quoted 5 lines · Benoit Cousson

On OMAP4 as well? I've just tried with the hwmod fix for AESS and USB and still cannot reach RET.

Am I missing some other patches for OMAP4?

You need also fixes for sl2if and mcpdm in addition to above.

Are they in some lo branch already?

Quoted 2 lines · Paul Walmsley

So the updated patch below uses a clockdomain data flag for this instead.

Here's a version that's a little cleaner. No functional changes.

arch/arm/mach-omap2/omap_hwmod.c +7 -0

+			      oh->_clk->clkdm->flags & CLKDM_ACTIVE_WITH_MPU));
+		if (clkdm_act && !(oh->class->sysc->idlemodes &
+				   (SIDLE_SMART | SIDLE_SMART_WKUP)))
+			idlemode = HWMOD_IDLEMODE_FORCE;
+		else
+			idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
+				HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
 		_set_slave_idlemode(oh, idlemode, &v);
 	}
 

arch/arm/mach-omap2/omap_hwmod.c static void _enable_sysc(struct omap_hwmod *oh) · line 1149

Quoted 9 lines

arch/arm/mach-omap2/omap_hwmod.c +3 -2

@@ -1141,8 +1144,16 @@ static void _enable_sysc(struct omap_hwmod *oh)
 	sf = oh->class->sysc->sysc_flags;
 
 	if (sf & SYSC_HAS_SIDLEMODE) {
-		idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
-			HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
+		clkdm_act = ((oh->clkdm &&
+			      oh->clkdm->flags & CLKDM_ACTIVE_WITH_MPU) ||
+			     (oh->_clk->clkdm &&

This is crashing on OMAP4 due to a NULL oh->_clk that can happen on some hwmod.

(quoted diff) +1 -0

+			     (oh->_clk && oh->_clk->clkdm &&

Is fixing the issue.

Hi Benoît

Quoted 13 lines · Benoit Cousson

arch/arm/mach-omap2/omap_hwmod.c static void _enable_sysc(struct omap_hwmod *oh) · line 1149

Quoted 9 lines

arch/arm/mach-omap2/omap_hwmod.c +3 -2

@@ -1141,8 +1144,16 @@ static void _enable_sysc(struct omap_hwmod *oh)
 	sf = oh->class->sysc->sysc_flags;
 
 	if (sf & SYSC_HAS_SIDLEMODE) {
-		idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
-			HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
+		clkdm_act = ((oh->clkdm &&
+			      oh->clkdm->flags & CLKDM_ACTIVE_WITH_MPU) ||
+			     (oh->_clk->clkdm &&

This is crashing on OMAP4 due to a NULL oh->_clk that can happen on some hwmod.

Quoted 3 lines

(quoted diff) +1 -0

+			     (oh->_clk && oh->_clk->clkdm &&

Is fixing the issue.

Thanks, just made the change and pushed the patch up to
git://git.pwsan.com/linux-2.6 in the branch 'omap_fixes_c_3.5rc'

Hi Beno?t,

On Fri, 15 Jun 2012, Cousson, Benoit wrote:

Quoted 3 lines

My point is simple, we should not add any new custom flag when all the information to detect such exception are already there in the current data and represent accurately what the HW is doing.

This seems like the crux of the issue. We don't have sufficient information to detect this exception in our current data, in my view. The software additionally must know when it can rely on a clockdomain remaining active when the MPU is active.

Ideally the hardcoded clockdomain sleep dependencies would be encoded, and we could test the intersection of those and the software-programmable clockdomain sleep dependencies. But such a change seems too complex for the -rc cycle. So the updated patch below uses a clockdomain data flag for this instead.

From: Paul Walmsley <[email protected]>
Date: Wed, 4 Jul 2012 05:22:53 -0600
Subject: [PATCH] ARM: OMAP2+: hwmod code/clockdomain data: fix 32K sync timer

Quoted 3 lines

From: Paul Walmsley <[email protected]>
Date: Wed, 4 Jul 2012 05:22:53 -0600
Subject: [PATCH] ARM: OMAP2+: hwmod code/clockdomain data: fix 32K sync timer

[...]

Quoted 3 lines

(quoted diff) +0 -0

@@ -1208,8 +1219,13 @@ static void _idle_sysc(struct omap_hwmod *oh)
 	sf = oh->class->sysc->sysc_flags;
 

arch/arm/mach-omap2/omap_hwmod.c static void _idle_sysc(struct omap_hwmod *oh) · line 1222

Quoted 4 lines

arch/arm/mach-omap2/omap_hwmod.c +1 -2

 	if (sf & SYSC_HAS_SIDLEMODE) {
-		idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
-			HWMOD_IDLEMODE_FORCE : HWMOD_IDLEMODE_SMART;
+		/* XXX What about HWMOD_IDLEMODE_SMART_WKUP? */

What do you mean here?

The 32k sync timer IP block target idle modes are incorrect in the hwmod data are incorrect.

The IP block does not support any
smart-idle modes. Update the data to reflect the correct modes.

This problem was initially identified and a diff fragment posted to the lists by Beno?t Cousson <[email protected]>.

Signed-off-by Paul Walmsley <[email protected]>

Cc Beno?t Cousson <[email protected]>

Cc Tero Kristo <[email protected]>

---
  arch/arm/mach-omap2/omap_hwmod_44xx_data.c |    3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

arch/arm/mach-omap2/omap_hwmod_44xx_data.c +1 -2

index 4aaaa84..6b0aedc 100644
@@ -393,8 +393,7 @@ static struct omap_hwmod_class_sysconfig omap44xx_counter_sysc = {
  	.rev_offs	= 0x0000,
  	.sysc_offs	= 0x0004,
  	.sysc_flags	= SYSC_HAS_SIDLEMODE,
-	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART |
-			   SIDLE_SMART_WKUP),
+	.idlemodes	= (SIDLE_FORCE | SIDLE_NO),
  	.sysc_fields	= &omap_hwmod_sysc_type1,
  };
 

Quoted 2 lines · Paul Walmsley

The 32k sync timer IP block target idle modes are incorrect in the hwmod data are incorrect.

Nit: Is there too many "incorrect" in this sentence?

Enable the AESS auto-gating control bit during AESS hwmod setup. This fixes the following boot warning on OMAP4:

include/linux/platform_data/aess.h new file +5 -0

+/**
+ * aess_enable_autogating - enable AESS internal autogating
+ * @oh: struct omap_hwmod *
+ *
+ * Enable internal autogating on the AESS.  This allows the AESS to

include/linux/platform_data/aess.h new file +11 -0

+/**
+ * hwmod_aess_preprogram - enable AESS internal autogating (called by hwmod)
+ * @oh: struct omap_hwmod *
+ *
+ * The AESS will not IdleAck to the PRCM until its internal autogating
+ * is enabled.  Since internal autogating is disabled by default after
+ * AESS reset, we must enable autogating after the hwmod code resets
+ * the AESS.  Returns 0.
+ */
+static int __maybe_unused hwmod_aess_preprogram(struct omap_hwmod *oh)
+{

include/linux/platform_data/aess.h

Quoted 2 lines · Paul Walmsley

include/linux/platform_data/aess.h new file +0 -0

This should be include/linux/platform_data/omap-aess.h or similar as there are other aess controllers too.

Quoted 4 lines

+/*
+ * AESS_AUTO_GATING_ENABLE_OFFSET: offset in bytes of the AESS IP
+ * block's AESS_AUTO_GATING_ENABLE__1 register from the IP block's
+ * base address

include/linux/platform_data/aess.h

Quoted 5 lines

+ */

include/linux/platform_data/aess.h new file +4 -0

+#define AESS_AUTO_GATING_ENABLE_OFFSET			0x07c
+
+/* Register bitfields in the AESS_AUTO_GATING_ENABLE__1 register */
+#define AESS_AUTO_GATING_ENABLE_SHIFT			0

Also these should be OMAP_AESS_AUTOGATING etc, or even better, XYZ_AESS_AUTOGATING where XYZ is the type of the AESS controller.

This should be static inline function as it's in the header, and again something like omap_aess_enable_autogating.

Quoted 18 lines

include/linux/platform_data/aess.h new file +18 -0

+/**
+ * hwmod_aess_preprogram - enable AESS internal autogating (called by hwmod)
+ * @oh: struct omap_hwmod *
+ *
+ * The AESS will not IdleAck to the PRCM until its internal autogating
+ * is enabled.  Since internal autogating is disabled by default after
+ * AESS reset, we must enable autogating after the hwmod code resets
+ * the AESS.  Returns 0.
+ */
+static int __maybe_unused hwmod_aess_preprogram(struct omap_hwmod *oh)
+{
+	void __iomem *va;
+
+	va = omap_hwmod_get_mpu_rt_va(oh);
+	if (!va)
+		return -EINVAL;
+
+	aess_enable_autogating(va);

include/linux/platform_data/aess.h

Quoted 3 lines

include/linux/platform_data/aess.h new file +3 -0

+
+	return 0;
+}

Then this function should not be in this header, instead it should be a static function somewhere in the omap hwmod code in some .c file. That's because this header should only have omap aess specific driver code, no hwmod code should be needed here.

Hi

(revised patch below)

Quoted 7 lines · Tony Lindgren

include/linux/platform_data/aess.h

Quoted 2 lines · Paul Walmsley

include/linux/platform_data/aess.h new file +0 -0

This should be include/linux/platform_data/omap-aess.h or similar as there are other aess controllers too.

Hmm, I guess there could be other SoCs that include this IP block; probably there is nothing OMAP-specific about it. So I moved this file to include/sound/aess.h. What do you think?

Quoted 4 lines

Quoted 4 lines

+/*
+ * AESS_AUTO_GATING_ENABLE_OFFSET: offset in bytes of the AESS IP
+ * block's AESS_AUTO_GATING_ENABLE__1 register from the IP block's
+ * base address

Quoted 8 lines

include/linux/platform_data/aess.h

Quoted 5 lines

+ */

include/linux/platform_data/aess.h new file +4 -0

+#define AESS_AUTO_GATING_ENABLE_OFFSET			0x07c
+
+/* Register bitfields in the AESS_AUTO_GATING_ENABLE__1 register */
+#define AESS_AUTO_GATING_ENABLE_SHIFT			0

Also these should be OMAP_AESS_AUTOGATING etc, or even better, XYZ_AESS_AUTOGATING where XYZ is the type of the AESS controller.

Hmm, do you think this is really needed? The other files in include/sound don't have SOUND_ or AUDIO_ prefixe.

Quoted 2 lines

This should be static inline function as it's in the header, and again something like omap_aess_enable_autogating.

I made it static inline, but didn't rename this one by the same rationale above.

Quoted 18 lines

Quoted 18 lines

include/linux/platform_data/aess.h new file +18 -0

+/**
+ * hwmod_aess_preprogram - enable AESS internal autogating (called by hwmod)
+ * @oh: struct omap_hwmod *
+ *
+ * The AESS will not IdleAck to the PRCM until its internal autogating
+ * is enabled.  Since internal autogating is disabled by default after
+ * AESS reset, we must enable autogating after the hwmod code resets
+ * the AESS.  Returns 0.
+ */
+static int __maybe_unused hwmod_aess_preprogram(struct omap_hwmod *oh)
+{
+	void __iomem *va;
+
+	va = omap_hwmod_get_mpu_rt_va(oh);
+	if (!va)
+		return -EINVAL;
+
+	aess_enable_autogating(va);

Quoted 8 lines

include/linux/platform_data/aess.h

Quoted 3 lines

include/linux/platform_data/aess.h new file +3 -0

+
+	return 0;
+}

Then this function should not be in this header, instead it should be a static function somewhere in the omap hwmod code in some .c file. That's because this header should only have omap aess specific driver code, no hwmod code should be needed here.

Moved this to arch/arm/mach-omap2/omap_hwmod_reset.c and prefixed it with "omap_".

Are you okay with this approach?

- Paul

From: Paul Walmsley <[email protected]>
Date: Thu, 14 Jun 2012 03:35:13 -0600
Subject: [PATCH] ARM: OMAP4+: AESS: enable internal auto-gating during
initial setup

Enable the AESS auto-gating control bit during AESS hwmod setup. This fixes the following boot warning on OMAP4:

omap_hwmod: aess: _wait_target_disable failed

Without this patch, the AESS IP block does not indicate to the PRCM that it is idle after it is reset. This prevents some types of SoC power management until something sets the auto-gating control bit.

This second version of this patch moves the control functions to include/linux/platform_data/aess.h at Tony's request:

    http://www.spinics.net/lists/arm-kernel/msg178329.html

Signed-off-by Paul Walmsley <[email protected]>

Cc Beno?t Cousson <[email protected]>

Cc P?ter Ujfalusi <[email protected]>

Cc Tony Lindgren <[email protected]>

---
 arch/arm/mach-omap2/Makefile                 |    2 +-
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c   |    1 +
 arch/arm/mach-omap2/omap_hwmod_reset.c       |   52 +++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/omap_hwmod.h |    5 +++
 include/sound/aess.h                         |   53 ++++++++++++++++++++++++++
 5 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-omap2/omap_hwmod_reset.c
 create mode 100644 include/sound/aess.h

arch/arm/mach-omap2/Makefile +1 -1

index fa742f3..4381e04 100644
@@ -7,7 +7,7 @@ obj-y := id.o io.o control.o mux.o devices.o serial.o gpmc.o timer.o pm.o \
 	 common.o gpio.o dma.o wd_timer.o display.o i2c.o hdq1w.o
 
 omap-2-3-common				= irq.o sdrc.o
-hwmod-common				= omap_hwmod.o \
+hwmod-common				= omap_hwmod.o omap_hwmod_reset.o \
 					  omap_hwmod_common_data.o
 clock-common				= clock.o clock_common_data.o \
 					  clkt_dpll.o clkt_clksel.o

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

index 6b0aedc..d1c4f3c 100644
@@ -313,6 +313,7 @@ static struct omap_hwmod_class_sysconfig omap44xx_aess_sysc = {
 static struct omap_hwmod_class omap44xx_aess_hwmod_class = {
 	.name	= "aess",
 	.sysc	= &omap44xx_aess_sysc,
+	.setup_preprogram = omap_hwmod_aess_preprogram,
 };
 
 /* aess */

arch/arm/mach-omap2/omap_hwmod_reset.c new file +52 -0

new file mode 100644
index 0000000..f810ba3
@@ -0,0 +1,52 @@
+/*
+ * OMAP IP block custom reset and preprogramming stubs
+ *
+ * Copyright (C) 2012 Texas Instruments, Inc.
+ * Paul Walmsley
+ *
+ * A small number of IP blocks need custom reset and preprogramming
+ * functions.  The stubs in this file provide a standard way for the
+ * hwmod code to call these functions, which are to be located under
+ * drivers/.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+#include <linux/kernel.h>
+
+#include <plat/omap_hwmod.h>
+
+#include <sound/aess.h>
+
+/**
+ * omap_hwmod_aess_preprogram - enable AESS internal autogating
+ * @oh: struct omap_hwmod *
+ *
+ * The AESS will not IdleAck to the PRCM until its internal autogating
+ * is enabled.  Since internal autogating is disabled by default after
+ * AESS reset, we must enable autogating after the hwmod code resets
+ * the AESS.  Returns 0.
+ */
+int omap_hwmod_aess_preprogram(struct omap_hwmod *oh)
+{
+	void __iomem *va;
+
+	va = omap_hwmod_get_mpu_rt_va(oh);
+	if (!va)
+		return -EINVAL;
+
+	aess_enable_autogating(va);
+
+	return 0;
+}

arch/arm/plat-omap/include/plat/omap_hwmod.h +5 -0

index b5f3efc..76fea1f 100644
@@ -662,6 +662,11 @@ extern int omap2430_hwmod_init(void);
 extern int omap3xxx_hwmod_init(void);
 extern int omap44xx_hwmod_init(void);
 
+/* Custom reset & preprogram stub prototypes - from omap_hwmod_reset.c */
+
+extern int omap_hwmod_aess_preprogram(struct omap_hwmod *oh);
+
+
 extern int __init omap_hwmod_register_links(struct omap_hwmod_ocp_if **ois);
 
 #endif

include/sound/aess.h new file +53 -0

new file mode 100644
index 0000000..cee0d09
@@ -0,0 +1,53 @@
+/*
+ * AESS IP block reset
+ *
+ * Copyright (C) 2012 Texas Instruments, Inc.
+ * Paul Walmsley
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+#ifndef __SOUND_AESS_H__
+#define __SOUND_AESS_H__
+
+#include <linux/kernel.h>
+#include <linux/io.h>
+
+/*
+ * AESS_AUTO_GATING_ENABLE_OFFSET: offset in bytes of the AESS IP
+ *     block's AESS_AUTO_GATING_ENABLE__1 register from the IP block's
+ *     base address
+ */
+#define AESS_AUTO_GATING_ENABLE_OFFSET			0x07c
+
+/* Register bitfields in the AESS_AUTO_GATING_ENABLE__1 register */
+#define AESS_AUTO_GATING_ENABLE_SHIFT			0
+
+/**
+ * aess_enable_autogating - enable AESS internal autogating
+ * @oh: struct omap_hwmod *
+ *
+ * Enable internal autogating on the AESS.  This allows the AESS to
+ * indicate that it is idle to the OMAP PRCM.  Returns 0.
+ */
+static inline void aess_enable_autogating(void __iomem *base)
+{
+	u32 v;
+
+	/* Set AESS_AUTO_GATING_ENABLE__1.ENABLE to allow idle entry */
+	v = 1 << AESS_AUTO_GATING_ENABLE_SHIFT;
+	writel(v, base + AESS_AUTO_GATING_ENABLE_OFFSET);
+}
+
+#endif /* __SOUND_AESS_H__ */

Signature · 1 line

1.7.10

On boot, the sl2if module can't be enabled. The following message is logged:

omap_hwmod: sl2if: cannot be enabled for reset (3)

This is probably because the SL2IF is still being held in hardreset. The SL2IF's hardreset line is shared with one of the IVAHD's hardreset lines; see for example Table 3-536 "RM_IVAHD_RSTCTRL" in the OMAP4430 TRM Rev. AA (SWPU231AA). To work around this, add the SL2IF's hardreset line to the hwmod data. This is correct from a hardware perspective and also will prevent the hwmod from attempting to enable the SL2IF during reset. The driver for this IP block will need to handle its integration until the appropriate way to handle the IVAHD integration can be elucidated.

Signed-off-by Paul Walmsley <[email protected]>

Cc Tero Kristo <[email protected]>

---
  arch/arm/mach-omap2/omap_hwmod_44xx_data.c |    7 +++++++
  1 file changed, 7 insertions(+)

arch/arm/mach-omap2/omap_hwmod_44xx_data.c +7 -1

index 7700d6d..037424f 100644
@@ -2597,15 +2597,22 @@ static struct omap_hwmod_class omap44xx_sl2if_hwmod_class = {
  	.name	= "sl2if",
  };
 
+static struct omap_hwmod_rst_info omap44xx_sl2if_resets[] = {
+	{ .name = "logic", .rst_shift = 2 },
+};
+
  /* sl2if */
  static struct omap_hwmod omap44xx_sl2if_hwmod = {
  	.name		= "sl2if",
  	.class		= &omap44xx_sl2if_hwmod_class,
  	.clkdm_name	= "ivahd_clkdm",
+	.rst_lines	= omap44xx_sl2if_resets,
+	.rst_lines_cnt	= ARRAY_SIZE(omap44xx_sl2if_resets),
  	.prcm = {
  		.omap4 = {
  			.clkctrl_offs = OMAP4_CM_IVAHD_SL2_CLKCTRL_OFFSET,
  			.context_offs = OMAP4_RM_IVAHD_SL2_CONTEXT_OFFSET,
+			.rstctrl_offs = OMAP4_RM_IVAHD_RSTCTRL_OFFSET,
  			.modulemode   = MODULEMODE_HWCTRL,
  		},
  	},
 
 
--

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

I don't think we should allow that at hwmod level. This line is already handled by the IVAHD hwmod. We will then have some potential issue since nothing will prevent the concurrent access.
Moreover that IP is only used by the IVAHD and thus should be handled by the same driver. The SL2 being inside the IVA subsystem, it should be reset when the IVA will be reset and thus does not require an individual control.

I guess, we should just mark the dependency with some external power resource to avoid the fmwk to try to enable that automatically.

It is similar to DSS sub IPs and to some extend McPDM for my point of view.

Maybe we should rename HWMOD_EXT_OPT_MAIN_CLK with something more generic like HWMOD_EXT_POWER_DEP to highlight the dependency with an external resource. It can be a external clock or another module.

Hi

Quoted 6 lines · Benoit

I don't think we should allow that at hwmod level. This line is already handled by the IVAHD hwmod. We will then have some potential issue since nothing will prevent the concurrent access.
Moreover that IP is only used by the IVAHD and thus should be handled by the same driver. The SL2 being inside the IVA subsystem, it should be reset when the IVA will be reset and thus does not require an individual control.

If a hardreset line will affect an IP block, then the line should be listed as one of the IP block's hardreset lines.

Otherwise, the hwmod
data will not accurately reflect the hardware -- the ultimate goal.

Quoted 2 lines

I guess, we should just mark the dependency with some external power resource to avoid the fmwk to try to enable that automatically.

Right now, the presence of a hardreset line will effectively cause the hwmod code to ignore the IP block from a reset and enable point of view. So the IP block won't be enabled automatically right now anyway, since we still don't have a clear sense of how to handle those.

Quoted 1 line

It is similar to DSS sub IPs and to some extend McPDM for my point of view.

These seem like different cases to me.

The DSS has no hardreset and the sub-IPs have their own softreset bits. As far as I know, the main reason why we can't have individual reset functions for the DSS sub-IPs is because we don't have hierarchical IP block enable/disable implemented in the hwmod code, not any real hardware constraint.

McPDM has a functional clock dependency on an external (non-SoC) device that's only controllable via I2C. And, the McPDM can block the ABE clockdomain from going idle. That's just wacky.

So those cases seem quite different to me. One is a software issue. And the other is an off-chip (non-OMAP) dependency, unlike the SL2IF/IVAHD issue in which all the needed resources are still on-chip.

Quoted 3 lines

Maybe we should rename HWMOD_EXT_OPT_MAIN_CLK with something more generic like HWMOD_EXT_POWER_DEP to highlight the dependency with an external resource. It can be a external clock or another module.

Yes, I agree that it would be convenient to have some flag, either on the hwmod or the hardreset line, to indicate that some of the reset lines are shared.

Yep, but for that I'd rather add a flag than a information that is a duplication of the parent data.

Quoted 2 lines · Benoit

Yep, but for that I'd rather add a flag than a information that is a duplication of the parent data.

Great, send a patch.

+#include <plat/omap_hwmod.h>

include/linux/platform_data/fsusb.h new file +10 -0

+static int fsusb_reset_host_controller(const char *name, void __iomem *base)
+{
+	int i;
+
+	writel(HCCOMMANDSTATUS_HCR_MASK, base + HCCOMMANDSTATUS);
+
+	for (i = 0; i < MAX_FSUSB_HCR_TIME; i++) {
+		if (!(readl(base + HCCOMMANDSTATUS) & HCCOMMANDSTATUS_HCR_MASK))
+			break;
+		udelay(1);

Hi,

Similar comments to the asess patch on this one below.

include/linux/platform_data/fsusb.h

Quoted 2 lines · Paul Walmsley

include/linux/platform_data/fsusb.h new file +0 -0

This would be better as include/linux/platform_data/omap-usb.h.

Quoted 1 line

+#include <plat/omap_hwmod.h>

This include should not be needed here if the hwmod function is a static function in the some hwmod*.c file.

include/linux/platform_data/fsusb.h line 46

Quoted 5 lines

include/linux/platform_data/fsusb.h new file +5 -0

+/* HCCOMMANDSTATUS: the register offset of the HCCOMMANDSTATUS register */
+#define HCCOMMANDSTATUS			0x0008
+
+/* HCCOMMANDSTATUS_HCR: the bitmask of the host controller reset flag */
+#define HCCOMMANDSTATUS_HCR_MASK	(1 << 0)

I think these already have standard defines in some OHCI header? Felipe may be able to comment more on this?

Quoted 15 lines

include/linux/platform_data/fsusb.h new file +15 -0

+static int fsusb_reset_host_controller(const char *name, void __iomem *base)
+{
+	int i;
+
+	writel(HCCOMMANDSTATUS_HCR_MASK, base + HCCOMMANDSTATUS);
+
+	for (i = 0; i < MAX_FSUSB_HCR_TIME; i++) {
+		if (!(readl(base + HCCOMMANDSTATUS) & HCCOMMANDSTATUS_HCR_MASK))
+			break;
+		udelay(1);
+	}
+
+	if (i == MAX_FSUSB_HCR_TIME) {
+		pr_warn("%s: %s: host controller reset failed (waited %d usec)\n",
+			__func__, name, MAX_FSUSB_HCR_TIME);

include/linux/platform_data/fsusb.h line 79

Quoted 5 lines

include/linux/platform_data/fsusb.h new file +5 -0

+		return -EBUSY;
+	}
+
+	return 0;
+}

This should be "static inline int fsusb_reset_host_controller" as it's in a header.

Quoted 15 lines · Tony Lindgren

Hi,

Similar comments to the asess patch on this one below.

include/linux/platform_data/fsusb.h

Quoted 2 lines · Paul Walmsley

include/linux/platform_data/fsusb.h new file +0 -0

This would be better as include/linux/platform_data/omap-usb.h.

Quoted 1 line

+#include <plat/omap_hwmod.h>

This include should not be needed here if the hwmod function is a static function in the some hwmod*.c file.

Quoted 8 lines

include/linux/platform_data/fsusb.h line 46

Quoted 5 lines

include/linux/platform_data/fsusb.h new file +5 -0

+/* HCCOMMANDSTATUS: the register offset of the HCCOMMANDSTATUS register */
+#define HCCOMMANDSTATUS			0x0008
+
+/* HCCOMMANDSTATUS_HCR: the bitmask of the host controller reset flag */
+#define HCCOMMANDSTATUS_HCR_MASK	(1 << 0)

I think these already have standard defines in some OHCI header? Felipe may be able to comment more on this?

Well, yeah... but it's defined on drivers/usb/host/ohci.h and it's actually a structure:

Quoted 2 lines

This should be "static inline int fsusb_reset_host_controller" as it's in a header.

why is it even in a header ? Only hwmod_fsusb_preprogram() will use it anyway.

Quoted 5 lines

Quoted 2 lines

This should be "static inline int fsusb_reset_host_controller" as it's in a header.

why is it even in a header ? Only hwmod_fsusb_preprogram() will use it anyway.

Well ideally it would be something that any OHCI driver can use for it's reset, and whatever bus code can also call to reset and idle for the unused devices that don't have the driver compiled in. Got any better suggestions where to place it? I could be a generic ohci_reset function in some header?

I don't want to have driver specific code under arch/arm/*omap*/* as the bus level code should not need to know anything about the driver specific registers.

Hi,

Quoted 11 lines · Tony Lindgren

Quoted 5 lines

Quoted 2 lines

This should be "static inline int fsusb_reset_host_controller" as it's in a header.

why is it even in a header ? Only hwmod_fsusb_preprogram() will use it anyway.

Well ideally it would be something that any OHCI driver can use for it's reset, and whatever bus code can also call to reset and idle for the unused devices that don't have the driver compiled in. Got any better suggestions where to place it? I could be a generic ohci_reset function in some header?

maybe create <linux/usb/ohci.h> ? Then, while doing that, move register definitions to this new header too... Something like:

$ mv drivers/usb/host/ohci.h include/linux/usb/
$ sed -i 's/\"ohci.h\"/<linux/usb/ohci.h>/g' $(git ls-files)

then add your ohci_reset() function...

Quoted 3 lines

I don't want to have driver specific code under arch/arm/*omap*/* as the bus level code should not need to know anything about the driver specific registers.

I see.

Quoted 18 lines · Felipe Balbi

Hi,

Quoted 11 lines · Tony Lindgren

Quoted 5 lines

Quoted 2 lines

This should be "static inline int fsusb_reset_host_controller" as it's in a header.

why is it even in a header ? Only hwmod_fsusb_preprogram() will use it anyway.

Well ideally it would be something that any OHCI driver can use for it's reset, and whatever bus code can also call to reset and idle for the unused devices that don't have the driver compiled in. Got any better suggestions where to place it? I could be a generic ohci_reset function in some header?

maybe create <linux/usb/ohci.h> ? Then, while doing that, move register definitions to this new header too... Something like:

Quoted 4 lines

$ mv drivers/usb/host/ohci.h include/linux/usb/
$ sed -i 's/\"ohci.h\"/<linux/usb/ohci.h>/g' $(git ls-files)

then add your ohci_reset() function...

Hmm but then again it's pointless to export the all the ohci registers as that will lead to misuse outside drivers/usb.. Sounds like just defining the reset register is safest option here.

Until the OMAP4 code is converted to disable the use of the clock framework-based clockdomain enable/disable sequence, any clock used as a hwmod main_clk must have a clockdomain associated with it.

This
patch populates some clock structure clockdomain names to resolve the following warnings during kernel init:

omap_hwmod: dpll_mpu_m2_ck: missing clockdomain for dpll_mpu_m2_ck.
omap_hwmod: trace_clk_div_ck: missing clockdomain for trace_clk_div_ck.
omap_hwmod: l3_div_ck: missing clockdomain for l3_div_ck.
omap_hwmod: ddrphy_ck: missing clockdomain for ddrphy_ck.

Signed-off-by Paul Walmsley <[email protected]>

Cc Rajendra Nayak <[email protected]>

Cc Beno?t Cousson <[email protected]>

---
  arch/arm/mach-omap2/clock44xx_data.c |    5 +++++
  1 file changed, 5 insertions(+)

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

index 2172f66..e2b701e 100644

In fact, neither prm_clkdm not cm_clkdm are valid clock domain on OMAP4 :-(.

I've just realized that you introduced that for 3.5, but this is wrong. We should not start adding some fake clock domains just because the fmwk is not smart enough to allow a NULL clock domain.

In a period of data size reduction, adding some fake information does not seems to be the right approach. Don't you think so?

Quoted 7 lines · Benoit

In fact, neither prm_clkdm not cm_clkdm are valid clock domain on OMAP4 :-(.

I've just realized that you introduced that for 3.5, but this is wrong. We should not start adding some fake clock domains just because the fmwk is not smart enough to allow a NULL clock domain.

...

Quoted 2 lines

In a period of data size reduction, adding some fake information does not seems to be the right approach. Don't you think so?

No, I do not.

These clockdomains are clearly documented in both the OMAP4 TRM[1] and the NDA OMAP4 PRCM functional specifications.

I continue to be baffled as to why you assert that they are fake, given how clearly they are documented.

Resolve this kernel boot message:

omap_hwmod: mcpdm: cannot be enabled for reset (3)

It appears that the McPDM on OMAP4 can only receive its functional clock from an off-chip source. This source is not guaranteed to be present on the board, and when present, it is controlled by I2C. This would introduce a board dependency to the early hwmod code which it was not designed to handle. Also, neither the driver for this off-chip clock provider nor the I2C code is available early in boot when the hwmod code is attempting to enable and reset IP blocks. This effectively makes it impossible to enable and reset this device during hwmod init.

At its core, this patch is a workaround for an OMAP hardware problem. It should be possible to configure the OMAP to provide any IP block's functional clock from an on-chip source. (This is true for almost every IP block on the chip. As far as I know, McPDM is the only exception.) If the kernel cannot reset and configure IP blocks, it cannot guarantee a sane SoC state. Relying on an optional off-chip clock also creates a board dependency which is beyond the scope of the early hwmod code.

This patch works around the issue by marking the McPDM hwmod record with the HWMOD_EXT_OPT_MAIN_CLK flag. This prevents the hwmod code from touching the device early during boot.

Signed-off-by Paul Walmsley <[email protected]>

Cc P?ter Ujfalusi <[email protected]>

Cc Beno?t Cousson <[email protected]>

---
  arch/arm/mach-omap2/omap_hwmod_44xx_data.c |   12 ++++++++++++
  1 file changed, 12 insertions(+)

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

index 3613054..86fc513 100644

arch/arm/mach-omap2/omap_hwmod_44xx_data.c static struct omap_hwmod omap44xx_mcpdm_hwmod = { · line 2096

Quoted 12 lines · Paul Walmsley

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

index 3613054..86fc513 100644
@@ -2091,6 +2091,18 @@ static struct omap_hwmod omap44xx_mcpdm_hwmod = {
  	.name		= "mcpdm",
  	.class		= &omap44xx_mcpdm_hwmod_class,
  	.clkdm_name	= "abe_clkdm",
+	/*
+	 * It's suspected that the McPDM requires an off-chip main
+	 * functional clock, controlled via I2C.

Nit: Is it not suspected, it is a fact. The clock tree clearly indicate that the mcpdm fclk is generated from the pad_clks.
The IP is a custom link for the Audio IC control / data. using the Audio IC as a source clock is standard. Since that IP is done only for that purpose, there is no optional clock path from the PRCM like it is the case for the McASP / McBSP.

arch/arm/mach-omap2/omap_hwmod_44xx_data.c static struct omap_hwmod omap44xx_mcpdm_hwmod = { · line 2104

Quoted 8 lines

arch/arm/mach-omap2/omap_hwmod_44xx_data.c +7 -0

 This IP block is
+	 * currently reset very early during boot, before I2C is
+	 * available, so it doesn't seem that we have any choice in
+	 * the kernel other than to avoid resetting it.  XXX This is
+	 * really a hardware issue workaround: every IP block should
+	 * be able to source its main functional clock from either
+	 * on-chip or off-chip sources.  McPDM seems to be the only
+	 * current exception.

I do not think this is the right place to put some complaint about the HW :-). I guess we should just describe the facts.

arch/arm/mach-omap2/omap_hwmod_44xx_data.c static struct omap_hwmod omap44xx_mcpdm_hwmod = { · line 2106

Quoted 2 lines

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

+	 */
+	.flags		= HWMOD_EXT_OPT_MAIN_CLK,

Could you please update the hints for this IP in the autogen scripts? I added a comment attribute to add a custom comment on top of the flags entry. The latest branch is "omap5430_for_3.6".

omap_hwmod: mcpdm: cannot be enabled for reset (3)

The McPDM on OMAP4 can only receive its functional clock from an off-chip source. This source is not guaranteed to be present on the board, and when present, it is controlled by I2C. This would introduce a board dependency to the early hwmod code which it was not designed to handle. Also, neither the driver for this off-chip clock provider nor the I2C code is available early in boot when the hwmod code is attempting to enable and reset IP blocks. This effectively makes it impossible to enable and reset this device during hwmod init.

At its core, this patch is a workaround for an OMAP hardware problem. It should be possible to configure the OMAP to provide any IP block's functional clock from an on-chip source. (This is true for almost every IP block on the chip. As far as I know, McPDM is the only exception.) If the kernel cannot reset and configure IP blocks, it cannot guarantee a sane SoC state. Relying on an optional off-chip clock also creates a board dependency which is beyond the scope of the early hwmod code.

This patch works around the issue by marking the McPDM hwmod record with the HWMOD_EXT_OPT_MAIN_CLK flag. This prevents the hwmod code from touching the device early during boot.

Signed-off-by Paul Walmsley <[email protected]>

Cc P?ter Ujfalusi <[email protected]>

Cc Beno?t Cousson <[email protected]>

---
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |    8 ++++++++
 1 file changed, 8 insertions(+)

arch/arm/mach-omap2/omap_hwmod_44xx_data.c +8 -0

index 652d028..7bddfa5 100644
@@ -2125,6 +2125,14 @@ static struct omap_hwmod omap44xx_mcpdm_hwmod = {
 	.name		= "mcpdm",
 	.class		= &omap44xx_mcpdm_hwmod_class,
 	.clkdm_name	= "abe_clkdm",
+	/*
+	 * It's suspected that the McPDM requires an off-chip main
+	 * functional clock, controlled via I2C.  This IP block is
+	 * currently reset very early during boot, before I2C is
+	 * available, so it doesn't seem that we have any choice in
+	 * the kernel other than to avoid resetting it.
+	 */
+	.flags		= HWMOD_EXT_OPT_MAIN_CLK,
 	.mpu_irqs	= omap44xx_mcpdm_irqs,
 	.sdma_reqs	= omap44xx_mcpdm_sdma_reqs,
 	.main_clk	= "mcpdm_fck",