RE: [PATCH 01/17] PM: fix suspend control for IVA2
On Thu, 2009-10-22 at 16:21 -0500, Woodruff, Richard wrote: > > From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- > > ow...@vger.kernel.org] On Behalf Of Paul Walmsley > > Sent: Thursday, October 22, 2009 5:24 AM > > > > IVA2 controls its target power state individually, thus suspend should not > > > touch IVA2. Without this patch DSP suspend always fails. > > > > We don't allow other device drivers to touch PRCM bits, so we should > > probably should remove all PRCM register accesses from the DSPBridge code, > > so all power control should go through the ARM. > > > > Is there a reason why the ARM code can't handle the DSP powerdomain? > > Sharing with DSP is something which probably could use some improvement. > > Today DSP self-manages its domain. Its (bios) micro-kernel makes decisions > to optimize its domain. The ARM can't really micro-manage the DSP as he > doesn't even want to know at the detail level what the DSP is up to at every > instant. > > - During idle time cpuidle should just be checking dsp status to see if its > current state gets in the way of a low c-state. > > - bridge does register with suspend frame work so he should do the right > thing when in the system. > > * problem is when bridge isn't there what to do. This is especially after an > unload of the bridge. Just a side note reminder: bridge does not exist in the real world. [dedek...@sauron linux-omap-2.6]\$ ls drivers/dsp/bridge ls: cannot access drivers/dsp/bridge: No such file or directory -- Best Regards, Artem Bityutskiy (Артём Битюцкий) -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 01/17] PM: fix suspend control for IVA2
> From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- > ow...@vger.kernel.org] On Behalf Of Paul Walmsley > Sent: Thursday, October 22, 2009 5:24 AM > > IVA2 controls its target power state individually, thus suspend should not > > touch IVA2. Without this patch DSP suspend always fails. > > We don't allow other device drivers to touch PRCM bits, so we should > probably should remove all PRCM register accesses from the DSPBridge code, > so all power control should go through the ARM. > > Is there a reason why the ARM code can't handle the DSP powerdomain? Sharing with DSP is something which probably could use some improvement. Today DSP self-manages its domain. Its (bios) micro-kernel makes decisions to optimize its domain. The ARM can't really micro-manage the DSP as he doesn't even want to know at the detail level what the DSP is up to at every instant. - During idle time cpuidle should just be checking dsp status to see if its current state gets in the way of a low c-state. - bridge does register with suspend frame work so he should do the right thing when in the system. * problem is when bridge isn't there what to do. This is especially after an unload of the bridge. What has been proposed in the past is like what Kevin inputted in related thread about having maintenance hand off. But for some reason it never quite to the top of the list. - bridge does request thought clock frame work clocks especially of those which are public peripherals. - bridge could request everything but it was not projected as power efficient waking up the big arm core for something the dsp could do itself and has all control over. This is especially true if you have dsp doing the rendering for something like mp3. it gets the wake ups and streaming and only every great while wakes the arm to give it a pile more data. Waking the arm every time it runs its equivalent of cpuidle was discourged. - main other sharing conflict was with irq routing between arm and bridge. Right now its kind of init mode setup. I guess this is ok for todays usage. .. so after context current code is not requesting through pm code purposefully. The hardware has been evolving from omap1,2,3,4 to make for more of a distributed model. After all the details/constraints are understood with silicon there is some time to re-evaluate if its paying back or not. Regards, Richard W. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/17] PM: fix suspend control for IVA2
Hi Tero, Ameya, Girish, On Fri, 16 Oct 2009, Tero Kristo wrote: > From: Tero Kristo > > IVA2 controls its target power state individually, thus suspend should not > touch IVA2. Without this patch DSP suspend always fails. We don't allow other device drivers to touch PRCM bits, so we should probably should remove all PRCM register accesses from the DSPBridge code, so all power control should go through the ARM. Is there a reason why the ARM code can't handle the DSP powerdomain? - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/17] PM: fix suspend control for IVA2
writes:
>>-Original Message-
>>From: ext Girish S G [mailto:giris...@ti.com]
>>Sent: 16 October, 2009 20:16
>>To: Kristo Tero (Nokia-D/Tampere); linux-omap@vger.kernel.org
>>Subject: RE: [PATCH 01/17] PM: fix suspend control for IVA2
>>
>>
>>
>>> -Original Message-
>>> From: linux-omap-ow...@vger.kernel.org
>>[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Tero
>>> Kristo
>>>
>>> From: Tero Kristo
>>>
>>> IVA2 controls its target power state individually, thus
>>suspend should not
>>> touch IVA2. Without this patch DSP suspend always fails.
>>>
>>> Signed-off-by: Tero Kristo
>>> Acked-by: Ameya Palande
>>> ---
>>> arch/arm/mach-omap2/pm34xx.c |9 -
>>> 1 files changed, 8 insertions(+), 1 deletions(-)
>>>
>>> static struct prm_setup_vc prm_setup = {
>>> .clksetup = 0xff,
>>> @@ -676,6 +676,12 @@ static int omap3_pm_suspend(void)
>>> pwrst->saved_state =
>>pwrdm_read_next_pwrst(pwrst->pwrdm);
>>> /* Set ones wanted by suspend */
>>> list_for_each_entry(pwrst, &pwrst_list, node) {
>>> + /* Special handling for IVA2, just use current
>>sleep state */
>>> + if (pwrst->pwrdm == iva2_pwrdm) {
>>> + state = pwrdm_read_pwrst(pwrst->pwrdm);
>>> + if (state < PWRDM_POWER_ON)
>>> + pwrst->next_state = state;
>>> + }
>>
>>Agree, IVA2 pwrdm is handled autonomously by bridge. I think
>>this needs some additional change to remove all the redundant
>>configuration of iva pwdm. There are some inconsistencies like,
>> - Say enable_off_mode is disabled. Before doing system
>>wide suspend if DSP hibernates then IVA2 will be put to OFF. In that
>>case we have IVA2 going to OFF and other domains in RET. This
>>might not be an issue, but it's bad from sytem PM framework integrity
>>perspective.
>
> This is an issue with bridge driver, and I am not sure how this should be
> fixed. Currently bridge driver does not care whether off mode is enabled or
> not.
>
>> - enable_off_mode->omap3_pm_off_mode_enable will also
>>touch IVA2 power domain next state. This we don't want to do if dsp
>>bridge is already taking care of IVA2.
>>
>>IMO, we need to have some mechanism wherein if bridge PM takes
>>care of IVA then PM framework should not configure the IVA
>>powerstate. It should only do if bridge PM is disabled.
>
> Should we have a Kconfig option for this? Like CONFIG_OMAP3_BRIDGE_PM, and
> disable all iva2 controls from pm34xx.c if it is enabled? Otherwise control
> IVA2 as currently done.
>
Rather than Kconfig, this should be run-time.
There should be some sort of flag for indpendent IVA2 control and a
register function that bridge (or any other DSP controller) could
register for and take over control of IVA2.
This way, if no bridge is loaded, IVA2 can still properly be suspended.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 01/17] PM: fix suspend control for IVA2
> -Original Message- > From: tero.kri...@nokia.com [mailto:tero.kri...@nokia.com] > Sent: Monday, October 19, 2009 4:23 AM > To: giris...@ti.com; linux-omap@vger.kernel.org > Subject: RE: [PATCH 01/17] PM: fix suspend control for IVA2 > > > >Agree, IVA2 pwrdm is handled autonomously by bridge. I think > >this needs some additional change to remove all the redundant > >configuration of iva pwdm. There are some inconsistencies like, > > - Say enable_off_mode is disabled. Before doing system > >wide suspend if DSP hibernates then IVA2 will be put to OFF. In that > >case we have IVA2 going to OFF and other domains in RET. This > >might not be an issue, but it's bad from sytem PM framework integrity > >perspective. > > This is an issue with bridge driver, and I am not sure how this should be > fixed. Currently bridge > driver does not care whether off mode is enabled or not. I have seen bridge considering enable_off_mode flag in suspend/resume path. But while hibernation (idle timeout) it goes to OFF, irrespective of the OFF flag. > > > - enable_off_mode->omap3_pm_off_mode_enable will also > >touch IVA2 power domain next state. This we don't want to do if dsp > >bridge is already taking care of IVA2. > > > >IMO, we need to have some mechanism wherein if bridge PM takes > >care of IVA then PM framework should not configure the IVA > >powerstate. It should only do if bridge PM is disabled. > > Should we have a Kconfig option for this? Like CONFIG_OMAP3_BRIDGE_PM, and > disable all iva2 controls > from pm34xx.c if it is enabled? Otherwise control IVA2 as currently done. Yes, this looks ok to me. -Girish -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 01/17] PM: fix suspend control for IVA2
>-Original Message-
>From: ext Girish S G [mailto:giris...@ti.com]
>Sent: 16 October, 2009 20:16
>To: Kristo Tero (Nokia-D/Tampere); linux-omap@vger.kernel.org
>Subject: RE: [PATCH 01/17] PM: fix suspend control for IVA2
>
>
>
>> -Original Message-
>> From: linux-omap-ow...@vger.kernel.org
>[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Tero
>> Kristo
>>
>> From: Tero Kristo
>>
>> IVA2 controls its target power state individually, thus
>suspend should not
>> touch IVA2. Without this patch DSP suspend always fails.
>>
>> Signed-off-by: Tero Kristo
>> Acked-by: Ameya Palande
>> ---
>> arch/arm/mach-omap2/pm34xx.c |9 -
>> 1 files changed, 8 insertions(+), 1 deletions(-)
>>
>> static struct prm_setup_vc prm_setup = {
>> .clksetup = 0xff,
>> @@ -676,6 +676,12 @@ static int omap3_pm_suspend(void)
>> pwrst->saved_state =
>pwrdm_read_next_pwrst(pwrst->pwrdm);
>> /* Set ones wanted by suspend */
>> list_for_each_entry(pwrst, &pwrst_list, node) {
>> +/* Special handling for IVA2, just use current
>sleep state */
>> +if (pwrst->pwrdm == iva2_pwrdm) {
>> +state = pwrdm_read_pwrst(pwrst->pwrdm);
>> +if (state < PWRDM_POWER_ON)
>> +pwrst->next_state = state;
>> +}
>
>Agree, IVA2 pwrdm is handled autonomously by bridge. I think
>this needs some additional change to remove all the redundant
>configuration of iva pwdm. There are some inconsistencies like,
> - Say enable_off_mode is disabled. Before doing system
>wide suspend if DSP hibernates then IVA2 will be put to OFF. In that
>case we have IVA2 going to OFF and other domains in RET. This
>might not be an issue, but it's bad from sytem PM framework integrity
>perspective.
This is an issue with bridge driver, and I am not sure how this should be
fixed. Currently bridge driver does not care whether off mode is enabled or not.
> - enable_off_mode->omap3_pm_off_mode_enable will also
>touch IVA2 power domain next state. This we don't want to do if dsp
>bridge is already taking care of IVA2.
>
>IMO, we need to have some mechanism wherein if bridge PM takes
>care of IVA then PM framework should not configure the IVA
>powerstate. It should only do if bridge PM is disabled.
Should we have a Kconfig option for this? Like CONFIG_OMAP3_BRIDGE_PM, and
disable all iva2 controls from pm34xx.c if it is enabled? Otherwise control
IVA2 as currently done.
-Tero
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 01/17] PM: fix suspend control for IVA2
> -Original Message-
> From: linux-omap-ow...@vger.kernel.org
> [mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Tero
> Kristo
>
> From: Tero Kristo
>
> IVA2 controls its target power state individually, thus suspend should not
> touch IVA2. Without this patch DSP suspend always fails.
>
> Signed-off-by: Tero Kristo
> Acked-by: Ameya Palande
> ---
> arch/arm/mach-omap2/pm34xx.c |9 -
> 1 files changed, 8 insertions(+), 1 deletions(-)
>
> static struct prm_setup_vc prm_setup = {
> .clksetup = 0xff,
> @@ -676,6 +676,12 @@ static int omap3_pm_suspend(void)
> pwrst->saved_state = pwrdm_read_next_pwrst(pwrst->pwrdm);
> /* Set ones wanted by suspend */
> list_for_each_entry(pwrst, &pwrst_list, node) {
> + /* Special handling for IVA2, just use current sleep state */
> + if (pwrst->pwrdm == iva2_pwrdm) {
> + state = pwrdm_read_pwrst(pwrst->pwrdm);
> + if (state < PWRDM_POWER_ON)
> + pwrst->next_state = state;
> + }
Agree, IVA2 pwrdm is handled autonomously by bridge. I think this needs some
additional change to remove all the redundant
configuration of iva pwdm. There are some inconsistencies like,
- Say enable_off_mode is disabled. Before doing system wide suspend if
DSP hibernates then IVA2 will be put to OFF. In that
case we have IVA2 going to OFF and other domains in RET. This might not be an
issue, but it's bad from sytem PM framework integrity
perspective.
- enable_off_mode->omap3_pm_off_mode_enable will also touch IVA2 power
domain next state. This we don't want to do if dsp
bridge is already taking care of IVA2.
IMO, we need to have some mechanism wherein if bridge PM takes care of IVA then
PM framework should not configure the IVA
powerstate. It should only do if bridge PM is disabled.
-Girish
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/17] PM: fix suspend control for IVA2
From: Tero Kristo
IVA2 controls its target power state individually, thus suspend should not
touch IVA2. Without this patch DSP suspend always fails.
Signed-off-by: Tero Kristo
Acked-by: Ameya Palande
---
arch/arm/mach-omap2/pm34xx.c |9 -
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index a3e3729..5e2ef63 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -81,7 +81,7 @@ static int (*_omap_save_secure_sram)(u32 *addr);
static struct powerdomain *mpu_pwrdm, *neon_pwrdm;
static struct powerdomain *core_pwrdm, *per_pwrdm;
-static struct powerdomain *cam_pwrdm;
+static struct powerdomain *cam_pwrdm, *iva2_pwrdm;
static struct prm_setup_vc prm_setup = {
.clksetup = 0xff,
@@ -676,6 +676,12 @@ static int omap3_pm_suspend(void)
pwrst->saved_state = pwrdm_read_next_pwrst(pwrst->pwrdm);
/* Set ones wanted by suspend */
list_for_each_entry(pwrst, &pwrst_list, node) {
+ /* Special handling for IVA2, just use current sleep state */
+ if (pwrst->pwrdm == iva2_pwrdm) {
+ state = pwrdm_read_pwrst(pwrst->pwrdm);
+ if (state < PWRDM_POWER_ON)
+ pwrst->next_state = state;
+ }
if (set_pwrdm_state(pwrst->pwrdm, pwrst->next_state))
goto restore;
if (pwrdm_clear_all_prev_pwrst(pwrst->pwrdm))
@@ -1183,6 +1189,7 @@ static int __init omap3_pm_init(void)
per_pwrdm = pwrdm_lookup("per_pwrdm");
core_pwrdm = pwrdm_lookup("core_pwrdm");
cam_pwrdm = pwrdm_lookup("cam_pwrdm");
+ iva2_pwrdm = pwrdm_lookup("iva2_pwrdm");
omap_push_sram_idle();
#ifdef CONFIG_SUSPEND
--
1.5.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
