RE: [PATCH 6/8] OMAP3 PM: Enable DPLL4 autoidle after system off.
Hi,
On Thu, 21 Jan 2010, Gopinath, Thara wrote:
> >>> But if this approach is not ok, I can modify the save restore APIs to
> >>> take power state as a parameter and do only dpll4 autoidle save and
> >>> restore in case of OSWR. Is this ok?
> >>
> >>Well, let me know what you think of the above...
>
> I am ok with this. Only thing is it will involve a clk_get("dpll4_clk")
> and then rest of the API calls as you have suggested. Considering this
> is in cpuidle path, will the latencies be high?
How about doing the clk_get() in advance in omap3_pm_init(), and store the
struct clk pointer in a static variable that can be referenced from the
cpuidle path?
> If we have a latency issue, we can still keep the logic same as you have
> suggested and instead of dpll api's directly use CM API's to implement
> the same. So use cm_read_mod_reg and cm_rmw_mod_reg_bits.
We could but let's try the DPLL API first. I doubt the difference between
using the DPLL API and using the cm_* functions will be measurable. If
you look at the code for those functions, there's not much to them.
- 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 6/8] OMAP3 PM: Enable DPLL4 autoidle after system off.
Hi Paul,
>>
>>That may change depending on device wakeup latency constraints. If a
>>device has a low wakeup latency, we may choose to keep a DPLL running.
>>Sometimes this is also changed to work around silicon bugs. So we should
>>avoid adding code that makes assumptions about any of those autoidle bits.
>>
>>> Why I had to make this unconditional is because later before core
>>> entering OSWR I explicitly disable this bit in the register. This is so
>>> as to match this register content with those of scratchpad during OSWR.
>>> Failing to do this rom code while exiting OSWR will take the path it
>>> takes while coming out of core off . This causes a crash in the system
>>> today.
>>
>>Fine, but can't you save the previous state of the DPLL autoidle with
>>omap3_dpll_autoidle_read() before you disable it prior to OSWR, and just
>>restore it afterwards?
>>
>>> Also another reason is omap3_prcm_save_context needs to be called
>>> only during core off today.
>>
>>Is this because the CM registers are part of the logic section of the CORE
>>powerdomain?
This is because CM module is built with Retention flip flops(RFF) and during
OSWR the voltage is still supplied to the logic built with RFF. Thus no content
loss.
>>
>>> If I move saving of this bit and restoring it back into
>>> omap3_prcm_save_context and omap3_prcm_restore_context I will have to
>>> call it during core OSWR also.
>>
>>If you're not calling those now for OSWR, you shouldn't have to call them
>>just to save and restore the DPLL4 autoidle state, right? Wouldn't it
>>work to simply read the current autoidle state with
>>omap3_dpll_autoidle_read() before entering OSWR, then restore it
>>afterwards with omap3_dpll_{allow,deny}_idle() ?
>>
>>> Wanted to avoid the extra complications.
>>
>>And the extra delay. Reads and writes from L4_WAKEUP-connected devices
>>are sllloow.
Agreed.
>>
>>> But if this approach is not ok, I can modify the save restore APIs to
>>> take power state as a parameter and do only dpll4 autoidle save and
>>> restore in case of OSWR. Is this ok?
>>
>>Well, let me know what you think of the above...
I am ok with this. Only thing is it will involve a clk_get("dpll4_clk") and
then rest of the API calls as you have suggested. Considering this is in
cpuidle path, will the latencies be high?
If we have a latency issue, we can still keep the logic same as you have
suggested and instead of dpll api's directly use CM API's to implement the
same. So use cm_read_mod_reg and cm_rmw_mod_reg_bits.
Regards
Thara
--
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 6/8] OMAP3 PM: Enable DPLL4 autoidle after system off.
Hello Thara,
On Thu, 21 Jan 2010, Gopinath, Thara wrote:
> >>-Original Message-
> >>From: Paul Walmsley [mailto:p...@pwsan.com]
> >>Sent: Wednesday, January 20, 2010 11:05 PM
> >>To: Gopinath, Thara
> >>Cc: linux-omap@vger.kernel.org
> >>Subject: Re: [PATCH 6/8] OMAP3 PM: Enable DPLL4 autoidle after system off.
> >>
> >>On Wed, 20 Jan 2010, Thara Gopinath wrote:
> >>
> >>> DPLL4 autoidle is controlled through the register CM_PLL_AUTOIDLE
> >>> which is to be restored by rom code from the scratchpad in case
> >>> of a core domain context loss. But enabling this bit in scratchpad
> >>> causes rom code to take an extra 20 ms delay in the restore path.
> >>> To avoid this delay this bit is not enabled in the scratchpad today.
> >>> This means after a core off happens DPLL4 autoidle is never again
> >>> enabled back.
> >>> This patch enables DPLL4 autoidle in case of core domain losing
> >>> context.
> >>
> >>Shouldn't this be contingent on whether DPLL4 autoidle was enabled before
> >>the CORE off transition?
>
> Hi Paul, Hmmm may be yes.. But you see today we enable all autoidle bits
> in the init.
That may change depending on device wakeup latency constraints. If a
device has a low wakeup latency, we may choose to keep a DPLL running.
Sometimes this is also changed to work around silicon bugs. So we should
avoid adding code that makes assumptions about any of those autoidle bits.
> Why I had to make this unconditional is because later before core
> entering OSWR I explicitly disable this bit in the register. This is so
> as to match this register content with those of scratchpad during OSWR.
> Failing to do this rom code while exiting OSWR will take the path it
> takes while coming out of core off . This causes a crash in the system
> today.
Fine, but can't you save the previous state of the DPLL autoidle with
omap3_dpll_autoidle_read() before you disable it prior to OSWR, and just
restore it afterwards?
> Also another reason is omap3_prcm_save_context needs to be called
> only during core off today.
Is this because the CM registers are part of the logic section of the CORE
powerdomain?
> If I move saving of this bit and restoring it back into
> omap3_prcm_save_context and omap3_prcm_restore_context I will have to
> call it during core OSWR also.
If you're not calling those now for OSWR, you shouldn't have to call them
just to save and restore the DPLL4 autoidle state, right? Wouldn't it
work to simply read the current autoidle state with
omap3_dpll_autoidle_read() before entering OSWR, then restore it
afterwards with omap3_dpll_{allow,deny}_idle() ?
> Wanted to avoid the extra complications.
And the extra delay. Reads and writes from L4_WAKEUP-connected devices
are sllloow.
> But if this approach is not ok, I can modify the save restore APIs to
> take power state as a parameter and do only dpll4 autoidle save and
> restore in case of OSWR. Is this ok?
Well, let me know what you think of the above...
> >>> Signed-off-by: Thara Gopinath
> >>> ---
> >>> arch/arm/mach-omap2/pm34xx.c |8
> >>> 1 files changed, 8 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> >>> index e4db1ea..6e6d954 100644
> >>> --- a/arch/arm/mach-omap2/pm34xx.c
> >>> +++ b/arch/arm/mach-omap2/pm34xx.c
> >>> @@ -520,6 +520,14 @@ void omap_sram_idle(void)
> >>>*/
> >>> if (cpu_is_omap3430())
> >>> usb_musb_disable_autoidle();
> >>> + /* We do not program the scratchpad to restore back
> >>> + * PER DPLL in autoidle due to 20 ms delay in
> >>> + * rom code restore path. So enable it explicitly
> >>> + * after core off
> >>> + */
> >>
> >>This multi-line comment needs to be fixed to conform to
> >>Documentation/CodingStyle.
>
> Oops.. My bad. Will fix and resend the patch. Don't know why checkpatch
> is not catching this. I will wait a couple of days for comments on other
> patches also and resend all of them after fixing the relevant comments.
>
> >>> + cm_rmw_mod_reg_bits(
> >>> + 0x0, (1 << OMAP3430_AUTO_PERIPH_DPLL_SHIFT),
> >>> + PLL_MOD, CM_AUTOIDLE);
One other comment. Any reason why this patch can't use the
omap3_dpll_allow_idle() and omap3_dpll_deny_idle() functions, rather than
writing to the register directly?
> >>> }
> >>> omap_uart_resume_idle(0);
> >>> omap_uart_resume_idle(1);
> >>> --
> >>> 1.5.6.3
- 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 6/8] OMAP3 PM: Enable DPLL4 autoidle after system off.
>>-Original Message- >>From: Paul Walmsley [mailto:p...@pwsan.com] >>Sent: Wednesday, January 20, 2010 11:05 PM >>To: Gopinath, Thara >>Cc: linux-omap@vger.kernel.org >>Subject: Re: [PATCH 6/8] OMAP3 PM: Enable DPLL4 autoidle after system off. >> >>Hi Thara, >> >>two comments: >> >>On Wed, 20 Jan 2010, Thara Gopinath wrote: >> >>> DPLL4 autoidle is controlled through the register CM_PLL_AUTOIDLE >>> which is to be restored by rom code from the scratchpad in case >>> of a core domain context loss. But enabling this bit in scratchpad >>> causes rom code to take an extra 20 ms delay in the restore path. >>> To avoid this delay this bit is not enabled in the scratchpad today. >>> This means after a core off happens DPLL4 autoidle is never again >>> enabled back. >>> This patch enables DPLL4 autoidle in case of core domain losing >>> context. >> >>Shouldn't this be contingent on whether DPLL4 autoidle was enabled before >>the CORE off transition? Hi Paul, Hmmm may be yes.. But you see today we enable all autoidle bits in the init. Why I had to make this unconditional is because later before core entering OSWR I explicitly disable this bit in the register. This is so as to match this register content with those of scratchpad during OSWR. Failing to do this rom code while exiting OSWR will take the path it takes while coming out of core off . This causes a crash in the system today. Also another reason is omap3_prcm_save_context needs to be called only during core off today. If I move saving of this bit and restoring it back into omap3_prcm_save_context and omap3_prcm_restore_context I will have to call it during core OSWR also. Wanted to avoid the extra complications. But if this approach is not ok, I can modify the save restore APIs to take power state as a parameter and do only dpll4 autoidle save and restore in case of OSWR. Is this ok? >> >>> Signed-off-by: Thara Gopinath >>> --- >>> arch/arm/mach-omap2/pm34xx.c |8 >>> 1 files changed, 8 insertions(+), 0 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c >>> index e4db1ea..6e6d954 100644 >>> --- a/arch/arm/mach-omap2/pm34xx.c >>> +++ b/arch/arm/mach-omap2/pm34xx.c >>> @@ -520,6 +520,14 @@ void omap_sram_idle(void) >>> */ >>> if (cpu_is_omap3430()) >>> usb_musb_disable_autoidle(); >>> + /* We do not program the scratchpad to restore back >>> +* PER DPLL in autoidle due to 20 ms delay in >>> +* rom code restore path. So enable it explicitly >>> +* after core off >>> +*/ >> >>This multi-line comment needs to be fixed to conform to >>Documentation/CodingStyle. Oops.. My bad. Will fix and resend the patch. Don't know why checkpatch is not catching this. I will wait a couple of days for comments on other patches also and resend all of them after fixing the relevant comments. >> >>> + cm_rmw_mod_reg_bits( >>> + 0x0, (1 << OMAP3430_AUTO_PERIPH_DPLL_SHIFT), >>> + PLL_MOD, CM_AUTOIDLE); >>> } >>> omap_uart_resume_idle(0); >>> omap_uart_resume_idle(1); >>> -- >>> 1.5.6.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 >>> >> >> >>- 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 6/8] OMAP3 PM: Enable DPLL4 autoidle after system off.
Hi Thara, two comments: On Wed, 20 Jan 2010, Thara Gopinath wrote: > DPLL4 autoidle is controlled through the register CM_PLL_AUTOIDLE > which is to be restored by rom code from the scratchpad in case > of a core domain context loss. But enabling this bit in scratchpad > causes rom code to take an extra 20 ms delay in the restore path. > To avoid this delay this bit is not enabled in the scratchpad today. > This means after a core off happens DPLL4 autoidle is never again > enabled back. > This patch enables DPLL4 autoidle in case of core domain losing > context. Shouldn't this be contingent on whether DPLL4 autoidle was enabled before the CORE off transition? > Signed-off-by: Thara Gopinath > --- > arch/arm/mach-omap2/pm34xx.c |8 > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c > index e4db1ea..6e6d954 100644 > --- a/arch/arm/mach-omap2/pm34xx.c > +++ b/arch/arm/mach-omap2/pm34xx.c > @@ -520,6 +520,14 @@ void omap_sram_idle(void) >*/ > if (cpu_is_omap3430()) > usb_musb_disable_autoidle(); > + /* We do not program the scratchpad to restore back > + * PER DPLL in autoidle due to 20 ms delay in > + * rom code restore path. So enable it explicitly > + * after core off > + */ This multi-line comment needs to be fixed to conform to Documentation/CodingStyle. > + cm_rmw_mod_reg_bits( > + 0x0, (1 << OMAP3430_AUTO_PERIPH_DPLL_SHIFT), > + PLL_MOD, CM_AUTOIDLE); > } > omap_uart_resume_idle(0); > omap_uart_resume_idle(1); > -- > 1.5.6.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 > - 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
[PATCH 6/8] OMAP3 PM: Enable DPLL4 autoidle after system off.
DPLL4 autoidle is controlled through the register CM_PLL_AUTOIDLE which is to be restored by rom code from the scratchpad in case of a core domain context loss. But enabling this bit in scratchpad causes rom code to take an extra 20 ms delay in the restore path. To avoid this delay this bit is not enabled in the scratchpad today. This means after a core off happens DPLL4 autoidle is never again enabled back. This patch enables DPLL4 autoidle in case of core domain losing context. Signed-off-by: Thara Gopinath --- arch/arm/mach-omap2/pm34xx.c |8 1 files changed, 8 insertions(+), 0 deletions(-) diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c index e4db1ea..6e6d954 100644 --- a/arch/arm/mach-omap2/pm34xx.c +++ b/arch/arm/mach-omap2/pm34xx.c @@ -520,6 +520,14 @@ void omap_sram_idle(void) */ if (cpu_is_omap3430()) usb_musb_disable_autoidle(); + /* We do not program the scratchpad to restore back +* PER DPLL in autoidle due to 20 ms delay in +* rom code restore path. So enable it explicitly +* after core off +*/ + cm_rmw_mod_reg_bits( + 0x0, (1 << OMAP3430_AUTO_PERIPH_DPLL_SHIFT), + PLL_MOD, CM_AUTOIDLE); } omap_uart_resume_idle(0); omap_uart_resume_idle(1); -- 1.5.6.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
