| Message ID | 1310527588-13022-1-git-send-email-santosh.shilimkar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 13 Jul 2011, Santosh Shilimkar wrote: > From: Rajendra Nayak <rnayak@ti.com> > > Program all powerdomain target state as ON; This is to > prevent domains from hitting low power states (if bootloader > has target states set to something other than ON) and potentially > even losing context while PM is not fully initilized. > The PM late init code can then program the desired target > state for all the power domains. > > Signed-off-by: Rajendra Nayak <rnayak@ti.com> Thanks, dropped the second hunk of the patch and queued for 3.1-rc fixes at git://git.pwsan.com/linux-2.6 in the 'pwrdm_clkdm_fixes_3.1rc' branch. Santosh, looks like this is missing a Signed-off-by: or similar from you. Do you want me to add it? - Paul
Hi, On Wed, Jul 13, 2011 at 08:56:27AM +0530, Santosh Shilimkar wrote: > From: Rajendra Nayak <rnayak@ti.com> > > Program all powerdomain target state as ON; This is to > prevent domains from hitting low power states (if bootloader > has target states set to something other than ON) and potentially > even losing context while PM is not fully initilized. > The PM late init code can then program the desired target > state for all the power domains. > > Signed-off-by: Rajendra Nayak <rnayak@ti.com> > --- > arch/arm/mach-omap2/powerdomain.c | 12 +++++++++++- > 1 files changed, 11 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c > index e0490bc..e61866c 100644 > --- a/arch/arm/mach-omap2/powerdomain.c > +++ b/arch/arm/mach-omap2/powerdomain.c > @@ -109,6 +109,16 @@ static int _pwrdm_register(struct powerdomain *pwrdm) > > list_add(&pwrdm->node, &pwrdm_list); > > + /* > + * Program all powerdomain target state as ON; This is to > + * prevent domains from hitting low power states (if bootloader > + * has target states set to something other than ON) and potentially > + * even losing context while PM is not fully initilized. > + * The PM late init code can then program the desired target > + * state for all the power domains. > + */ > + pwrdm_set_next_pwrst(pwrdm, PWRDM_POWER_ON); Just out of curiosity, I was wondering if it really makes sense to power up all power domains during boot just to avoid loosing context. Doesn't hwmod/omap_device soft-reset all IPs during initialization ? If that's really the case, shouldn't we then choose which powerdomains are strictly necessary for boot and only power those up ? Sorry if this is a non-sensical question, but I was curious about it ;-)
Hi, On Fri, 15 Jul 2011, Felipe Balbi wrote: > > diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c > > index e0490bc..e61866c 100644 > > --- a/arch/arm/mach-omap2/powerdomain.c > > +++ b/arch/arm/mach-omap2/powerdomain.c > > @@ -109,6 +109,16 @@ static int _pwrdm_register(struct powerdomain *pwrdm) > > > > list_add(&pwrdm->node, &pwrdm_list); > > > > + /* > > + * Program all powerdomain target state as ON; This is to > > + * prevent domains from hitting low power states (if bootloader > > + * has target states set to something other than ON) and potentially > > + * even losing context while PM is not fully initilized. > > + * The PM late init code can then program the desired target > > + * state for all the power domains. > > + */ > > + pwrdm_set_next_pwrst(pwrdm, PWRDM_POWER_ON); > > Just out of curiosity, I was wondering if it really makes sense to power > up all power domains during boot just to avoid loosing context. Doesn't > hwmod/omap_device soft-reset all IPs during initialization ? If that's > really the case, shouldn't we then choose which powerdomains are > strictly necessary for boot and only power those up ? > > Sorry if this is a non-sensical question, but I was curious about it > ;-) This patch only sets the powerdomain's next power state to ON. It doesn't affect the current power state of the powerdomain. Let's say that the bootloader, previous OS (in the kexec case), or ROM code programs the next power state of some powerdomains to OFF. Let's also say that the kernel that is booted has PM disabled. The moment a powerdomain's clockdomains go inactive, the powerdomain will then switch off and all devices in that powerdomain will lose context. On a non-PM-enabled kernel, that will be unexpected and will probably cause the system to crash. This patch prevents that from happening. - Paul
Hi, On Fri, Jul 15, 2011 at 02:10:46AM -0600, Paul Walmsley wrote: > > > diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c > > > index e0490bc..e61866c 100644 > > > --- a/arch/arm/mach-omap2/powerdomain.c > > > +++ b/arch/arm/mach-omap2/powerdomain.c > > > @@ -109,6 +109,16 @@ static int _pwrdm_register(struct powerdomain *pwrdm) > > > > > > list_add(&pwrdm->node, &pwrdm_list); > > > > > > + /* > > > + * Program all powerdomain target state as ON; This is to > > > + * prevent domains from hitting low power states (if bootloader > > > + * has target states set to something other than ON) and potentially > > > + * even losing context while PM is not fully initilized. > > > + * The PM late init code can then program the desired target > > > + * state for all the power domains. > > > + */ > > > + pwrdm_set_next_pwrst(pwrdm, PWRDM_POWER_ON); > > > > Just out of curiosity, I was wondering if it really makes sense to power > > up all power domains during boot just to avoid loosing context. Doesn't > > hwmod/omap_device soft-reset all IPs during initialization ? If that's > > really the case, shouldn't we then choose which powerdomains are > > strictly necessary for boot and only power those up ? > > > > Sorry if this is a non-sensical question, but I was curious about it > > ;-) > > This patch only sets the powerdomain's next power state to ON. It doesn't > affect the current power state of the powerdomain. > > Let's say that the bootloader, previous OS (in the kexec case), or ROM > code programs the next power state of some powerdomains to OFF. Let's > also say that the kernel that is booted has PM disabled. The moment a > powerdomain's clockdomains go inactive, the powerdomain will then switch > off and all devices in that powerdomain will lose context. On a > non-PM-enabled kernel, that will be unexpected and will probably cause the > system to crash. This patch prevents that from happening. I see... thanks for clarifying. The comment above the code wasn't really clear about it to me.
Hi Felipe, On Fri, 15 Jul 2011, Felipe Balbi wrote: > I see... thanks for clarifying. The comment above the code wasn't really > clear about it to me. No worries. If you propose a clarification, I'll stick that into the patch. - Paul
Hi, On Fri, Jul 15, 2011 at 02:23:16AM -0600, Paul Walmsley wrote: > Hi Felipe, > > On Fri, 15 Jul 2011, Felipe Balbi wrote: > > > I see... thanks for clarifying. The comment above the code wasn't really > > clear about it to me. > > No worries. If you propose a clarification, I'll stick that into the > patch. How about something like: /* * Program target state of all power domains to ON. This is to prevent * power domains from hitting low power states during boot up and * potentially causing accesses to the address space of an IP while it * is disabled. * * PM late init code will make sure of disabling all unused IPs later. */ not sure if it's a lot better though.
On 7/15/2011 12:54 AM, Paul Walmsley wrote: > On Wed, 13 Jul 2011, Santosh Shilimkar wrote: > >> From: Rajendra Nayak<rnayak@ti.com> >> >> Program all powerdomain target state as ON; This is to >> prevent domains from hitting low power states (if bootloader >> has target states set to something other than ON) and potentially >> even losing context while PM is not fully initilized. >> The PM late init code can then program the desired target >> state for all the power domains. >> >> Signed-off-by: Rajendra Nayak<rnayak@ti.com> > > Thanks, dropped the second hunk of the patch and queued for 3.1-rc fixes > at git://git.pwsan.com/linux-2.6 in the 'pwrdm_clkdm_fixes_3.1rc' branch. > > Santosh, looks like this is missing a Signed-off-by: or similar from you. > Do you want me to add it? > Sure Paul. I missed to add it somehow while posting it. Regards Santosh
Hi, On Fri, 15 Jul 2011, Felipe Balbi wrote: > How about something like: > > /* > * Program target state of all power domains to ON. This is to prevent > * power domains from hitting low power states during boot up and > * potentially causing accesses to the address space of an IP while it > * is disabled. > * > * PM late init code will make sure of disabling all unused IPs later. > */ > > not sure if it's a lot better though. It's not directly related to accesses to the IP block's address space. The problem happens when the driver (or the hwmod code) for a particular device causes the last clock in a clockdomain to be disabled. This allows the clockdomain to go INACTIVE. At that point, if all of the clockdomains in the powerdomain are INACTIVE, the powerdomain itself will switch to its "next power state". If the bootloader had programmed the next power state to be OFF, for example, then all devices in that powerdomain will lose context. If this happens before the PM code initializes, or if the PM code is not compiled in, then this will cause some strange behavior or crashes, since the kernel won't be expecting devices to lose their context at that point. - Paul
diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c index e0490bc..e61866c 100644 --- a/arch/arm/mach-omap2/powerdomain.c +++ b/arch/arm/mach-omap2/powerdomain.c @@ -109,6 +109,16 @@ static int _pwrdm_register(struct powerdomain *pwrdm) list_add(&pwrdm->node, &pwrdm_list); + /* + * Program all powerdomain target state as ON; This is to + * prevent domains from hitting low power states (if bootloader + * has target states set to something other than ON) and potentially + * even losing context while PM is not fully initilized. + * The PM late init code can then program the desired target + * state for all the power domains. + */ + pwrdm_set_next_pwrst(pwrdm, PWRDM_POWER_ON); + /* Initialize the powerdomain's state counter */ for (i = 0; i < PWRDM_MAX_PWRSTS; i++) pwrdm->state_counter[i] = 0; @@ -218,7 +228,7 @@ static int _pwrdm_post_transition_cb(struct powerdomain *pwrdm, void *unused) /** * pwrdm_init - set up the powerdomain layer * @pwrdm_list: array of struct powerdomain pointers to register - * @custom_funcs: func pointers for arch specific implementations + * @custom_funcs: func pointers for arch specfic implementations * * Loop through the array of powerdomains @pwrdm_list, registering all * that are available on the current CPU. If pwrdm_list is supplied