From: Paul Walmsley <paul@pwsan.com> To: "Cousson, Benoit" <b-cousson@ti.com> Cc: linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Tony Lindgren <tony@atomide.com>, Tero Kristo <t-kristo@ti.com>, Kevin Hilman <khilman@ti.com>, Vaibhav Hiremath <hvaibhav@ti.com> Subject: Re: [PATCHv2 02/12] ARM: OMAP2+: hwmod code/data: fix 32K sync timer Date: Thu, 14 Jun 2012 18:18:37 -0600 (MDT) [thread overview] Message-ID: <alpine.DEB.2.00.1206141706420.25628@utopia.booyaka.com> (raw) In-Reply-To: <4FDA45F5.9090300@ti.com> On Thu, 14 Jun 2012, Cousson, Benoit wrote: > On 6/14/2012 8:04 PM, Paul Walmsley wrote: > > On Thu, 14 Jun 2012, Cousson, Benoit wrote: (attribution lost) > > > > 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. > > 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. > > 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. 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. > > 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. 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? - Paul
WARNING: multiple messages have this Message-ID (diff)
From: paul@pwsan.com (Paul Walmsley) To: linux-arm-kernel@lists.infradead.org Subject: [PATCHv2 02/12] ARM: OMAP2+: hwmod code/data: fix 32K sync timer Date: Thu, 14 Jun 2012 18:18:37 -0600 (MDT) [thread overview] Message-ID: <alpine.DEB.2.00.1206141706420.25628@utopia.booyaka.com> (raw) In-Reply-To: <4FDA45F5.9090300@ti.com> On Thu, 14 Jun 2012, Cousson, Benoit wrote: > On 6/14/2012 8:04 PM, Paul Walmsley wrote: > > On Thu, 14 Jun 2012, Cousson, Benoit wrote: (attribution lost) > > > > 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. > > 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. > > 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. 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. > > 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. 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? - Paul
next prev parent reply other threads:[~2012-06-15 0:18 UTC|newest] Thread overview: 106+ messages / expand[flat|nested] mbox.gz Atom feed top 2012-06-11 0:45 [PATCHv2 00/12] ARM: OMAP: core/hwmod: first set of fixes for 3.5-rc Paul Walmsley 2012-06-11 0:45 ` Paul Walmsley 2012-06-11 0:45 ` [PATCHv2 01/12] ARM: OMAP: PM: Lock clocks list while generating summary Paul Walmsley 2012-06-11 0:45 ` Paul Walmsley 2012-06-11 0:45 ` [PATCHv2 02/12] ARM: OMAP2+: hwmod code/data: fix 32K sync timer Paul Walmsley 2012-06-11 0:45 ` Paul Walmsley 2012-06-14 16:47 ` Cousson, Benoit 2012-06-14 16:47 ` Cousson, Benoit 2012-06-14 18:04 ` Paul Walmsley 2012-06-14 18:04 ` Paul Walmsley 2012-06-14 20:13 ` Cousson, Benoit 2012-06-14 20:13 ` Cousson, Benoit 2012-06-15 0:18 ` Paul Walmsley [this message] 2012-06-15 0:18 ` Paul Walmsley 2012-06-15 13:28 ` Cousson, Benoit 2012-06-15 13:28 ` Cousson, Benoit 2012-07-04 12:48 ` Paul Walmsley 2012-07-04 12:48 ` Paul Walmsley 2012-07-04 12:53 ` Paul Walmsley 2012-07-04 12:53 ` Paul Walmsley 2012-07-04 14:27 ` Kevin Hilman 2012-07-04 14:27 ` Kevin Hilman 2012-07-04 14:53 ` Paul Walmsley 2012-07-04 14:53 ` Paul Walmsley 2012-07-04 16:14 ` Benoit Cousson 2012-07-04 16:14 ` Benoit Cousson 2012-07-04 16:41 ` Tero Kristo 2012-07-04 16:41 ` Tero Kristo 2012-07-04 16:43 ` Benoit Cousson 2012-07-04 16:43 ` Benoit Cousson 2012-07-04 19:02 ` Paul Walmsley 2012-07-04 19:02 ` Paul Walmsley 2012-07-04 16:57 ` Benoit Cousson 2012-07-04 16:57 ` Benoit Cousson 2012-07-04 18:59 ` Paul Walmsley 2012-07-04 18:59 ` Paul Walmsley 2012-07-05 22:06 ` Kevin Hilman 2012-07-05 22:06 ` Kevin Hilman 2012-07-04 16:01 ` Benoit Cousson 2012-07-04 16:01 ` Benoit Cousson 2012-07-04 19:05 ` Paul Walmsley 2012-07-04 19:05 ` Paul Walmsley 2012-06-11 0:46 ` [PATCHv2 03/12] ARM: OMAP4+: hwmod: fix issue causing IPs not going back to Smart-Standby Paul Walmsley 2012-06-11 0:46 ` Paul Walmsley 2012-06-11 0:46 ` [PATCHv2 04/12] ARM: OMAP4: hwmod data: fix 32k sync timer idle modes Paul Walmsley 2012-06-11 0:46 ` Paul Walmsley 2012-06-11 9:31 ` Cousson, Benoit 2012-06-11 9:31 ` Cousson, Benoit 2012-06-13 17:22 ` Paul Walmsley 2012-06-13 17:22 ` Paul Walmsley 2012-06-11 0:46 ` [PATCHv2 05/12] ARM: OMAP2+: hwmod: add setup_preprogram hook Paul Walmsley 2012-06-11 0:46 ` Paul Walmsley 2012-06-11 0:46 ` [PATCHv2 06/12] ARM: OMAP2+: hwmod: add flag to prevent hwmod code from touching IP block during init Paul Walmsley 2012-06-11 0:46 ` Paul Walmsley 2012-06-11 0:46 ` [PATCHv2 07/12] ARM: OMAP4+: AESS: enable internal auto-gating during initial setup Paul Walmsley 2012-06-11 0:46 ` Paul Walmsley 2012-06-11 6:29 ` Tony Lindgren 2012-06-11 6:29 ` Tony Lindgren 2012-06-14 9:49 ` Paul Walmsley 2012-06-14 9:49 ` Paul Walmsley 2012-06-14 9:59 ` Tony Lindgren 2012-06-14 9:59 ` Tony Lindgren 2012-06-11 0:46 ` [PATCHv2 08/12] ARM: OMAP4: hwmod data: add SL2IF hardreset line Paul Walmsley 2012-06-11 0:46 ` Paul Walmsley 2012-06-14 12:55 ` Cousson, Benoit 2012-06-14 12:55 ` Cousson, Benoit 2012-06-14 17:09 ` Paul Walmsley 2012-06-14 17:09 ` Paul Walmsley 2012-06-14 21:07 ` Cousson, Benoit 2012-06-14 21:07 ` Cousson, Benoit 2012-06-14 23:02 ` Paul Walmsley 2012-06-14 23:02 ` Paul Walmsley 2012-06-15 9:11 ` Cousson, Benoit 2012-06-15 9:11 ` Cousson, Benoit 2012-06-11 0:46 ` [PATCHv2 09/12] ARM: OMAP2+: usb_host_fs: add custom setup_preprogram for usb_host_fs (fsusb) Paul Walmsley 2012-06-11 0:46 ` Paul Walmsley 2012-06-11 6:34 ` Tony Lindgren 2012-06-11 6:34 ` Tony Lindgren 2012-06-11 7:13 ` Felipe Balbi 2012-06-11 7:13 ` Felipe Balbi 2012-06-11 7:41 ` Tony Lindgren 2012-06-11 7:41 ` Tony Lindgren 2012-06-11 7:48 ` Felipe Balbi 2012-06-11 7:48 ` Felipe Balbi 2012-06-11 8:03 ` Tony Lindgren 2012-06-11 8:03 ` Tony Lindgren 2012-06-11 9:12 ` Felipe Balbi 2012-06-11 9:12 ` Felipe Balbi 2012-06-11 0:46 ` [PATCHv2 10/12] ARM: OMAP2+: CM: increase the module disable timeout Paul Walmsley 2012-06-11 0:46 ` Paul Walmsley 2012-06-11 0:46 ` [PATCHv2 11/12] ARM: OMAP4: clock data: add clockdomains for clocks used as main clocks Paul Walmsley 2012-06-11 0:46 ` Paul Walmsley 2012-06-11 16:28 ` Cousson, Benoit 2012-06-11 16:28 ` Cousson, Benoit 2012-06-11 16:59 ` Paul Walmsley 2012-06-11 16:59 ` Paul Walmsley 2012-06-11 20:15 ` Cousson, Benoit 2012-06-11 20:15 ` Cousson, Benoit 2012-06-11 0:46 ` [PATCHv2 12/12] ARM: OMAP4: hwmod data: do not enable or reset the McPDM during kernel init Paul Walmsley 2012-06-11 0:46 ` Paul Walmsley 2012-06-11 9:54 ` Cousson, Benoit 2012-06-11 9:54 ` Cousson, Benoit 2012-10-30 4:05 ` Paul Walmsley 2012-10-30 4:05 ` Paul Walmsley 2012-10-30 7:41 ` Péter Ujfalusi 2012-10-30 7:41 ` Péter Ujfalusi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=alpine.DEB.2.00.1206141706420.25628@utopia.booyaka.com \ --to=paul@pwsan.com \ --cc=b-cousson@ti.com \ --cc=hvaibhav@ti.com \ --cc=khilman@ti.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-omap@vger.kernel.org \ --cc=t-kristo@ti.com \ --cc=tony@atomide.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.