Rajendra Nayak [off-list ref] writes:
On Thursday 26 July 2012 04:13 AM, Kevin Hilman wrote:
quoted
Tero Kristo[off-list ref] writes:
quoted
On Fri, 2012-07-20 at 13:38 +0530, Rajendra Nayak wrote:
quoted
On Friday 20 July 2012 12:55 PM, Shilimkar, Santosh wrote:
quoted
On Fri, Jul 20, 2012 at 11:34 AM, Rajendra Nayak[off-list ref] wrote:
quoted
pwrdm_pre_transition()/pwrdm_post_transition() have always been high latency
operations done within cpuidle to do Powerdomain level book-keeping to know
what state transitions for different Powerdomains have been triggered.
This is also useful to do a restore-on-demand in some cases when we know
the context for the given Powerdomain was lost etc.
Now that we have definitive entry/exit points (thanks to the Powerdomain
level usecounting) for Powerdomain transitions, these book-keeping functions
can very well be moved from within CPUidle into pwrdm_clkdm_enable()/pwrdm_
clkdm_disable() functions.
Also rename _pwrdm_pre/post_transition_cb() to pwrdm_pre/post_transition()
and get rid of the original ones which iterate over all powerdomains.
Signed-off-by: Rajendra Nayak<redacted>
This is excellent! Thanks for working on this.
However, it needs a rebase against mainline though because I merged a
set of optimizations[1] to this code already that only calls pre/post
per-pwrdm.
Hi Kevin,
I thought some more on this patch, and I think this way of collecting
stats and knowing what state transitions the powerdomains been through
will not work on OMAP3, mainly because of the autodeps. Might work on
OMAP4 and beyond which do not need any autodeps.
Here why I think so,
Lets assume a Powerdomain X with a last module Y active, once Y disables
the last clock (lets assume no hardware controlled clocks for
simplicity), we clear the last power state register for X. However
due to autodeps X does not transition to a target state immediately.
It only does so when the MPU (and IVA) go down, and because
of the wakeup dependency (autodeps set a sleep and a wakeup dep with
both MPU and IVA) is also woken up every time MPU or IVA are up.
So its quite possible that X transitions in and out of sleep multiple
times before Y decides to re-enable its clocks, which is when we end up
looking for the last power state entered.
Lets say X entered OFF 3 times in between Y disabling and re-enabling
its clocks. Though we end up updating the counter only once (instead of
3) we still know and can tell Y that it lost context.
The problem however arises if for some reason X entered OFF
twice and happened to stay ON the third time the dependencies were met.
When Y re-enables its clocks, we end up telling it that it *did not*
lose context because we see the previous power state was ON.
Yeah, this is definitely a problem.
As long as we have autodeps, everything is centralized around CPU
transitions anyways, so it makes sense to keep the accounting
centralized too.
I think as long as we have autodeps, the only way on OMAP3 to accurately
do this is to do it for all dependent domains in CPUIdle :(
Or, to get rid of autodeps. ;)
Kevin
On Thu, 2012-07-26 at 10:44 -0700, Kevin Hilman wrote:
Rajendra Nayak [off-list ref] writes:
quoted
On Thursday 26 July 2012 04:13 AM, Kevin Hilman wrote:
quoted
Tero Kristo[off-list ref] writes:
quoted
On Fri, 2012-07-20 at 13:38 +0530, Rajendra Nayak wrote:
quoted
On Friday 20 July 2012 12:55 PM, Shilimkar, Santosh wrote:
quoted
On Fri, Jul 20, 2012 at 11:34 AM, Rajendra Nayak[off-list ref] wrote:
quoted
pwrdm_pre_transition()/pwrdm_post_transition() have always been high latency
operations done within cpuidle to do Powerdomain level book-keeping to know
what state transitions for different Powerdomains have been triggered.
This is also useful to do a restore-on-demand in some cases when we know
the context for the given Powerdomain was lost etc.
Now that we have definitive entry/exit points (thanks to the Powerdomain
level usecounting) for Powerdomain transitions, these book-keeping functions
can very well be moved from within CPUidle into pwrdm_clkdm_enable()/pwrdm_
clkdm_disable() functions.
Also rename _pwrdm_pre/post_transition_cb() to pwrdm_pre/post_transition()
and get rid of the original ones which iterate over all powerdomains.
Signed-off-by: Rajendra Nayak<redacted>
This is excellent! Thanks for working on this.
However, it needs a rebase against mainline though because I merged a
set of optimizations[1] to this code already that only calls pre/post
per-pwrdm.
Hi Kevin,
I thought some more on this patch, and I think this way of collecting
stats and knowing what state transitions the powerdomains been through
will not work on OMAP3, mainly because of the autodeps. Might work on
OMAP4 and beyond which do not need any autodeps.
Here why I think so,
Lets assume a Powerdomain X with a last module Y active, once Y disables
the last clock (lets assume no hardware controlled clocks for
simplicity), we clear the last power state register for X. However
due to autodeps X does not transition to a target state immediately.
It only does so when the MPU (and IVA) go down, and because
of the wakeup dependency (autodeps set a sleep and a wakeup dep with
both MPU and IVA) is also woken up every time MPU or IVA are up.
So its quite possible that X transitions in and out of sleep multiple
times before Y decides to re-enable its clocks, which is when we end up
looking for the last power state entered.
Lets say X entered OFF 3 times in between Y disabling and re-enabling
its clocks. Though we end up updating the counter only once (instead of
3) we still know and can tell Y that it lost context.
The problem however arises if for some reason X entered OFF
twice and happened to stay ON the third time the dependencies were met.
When Y re-enables its clocks, we end up telling it that it *did not*
lose context because we see the previous power state was ON.
Yeah, this is definitely a problem.
As long as we have autodeps, everything is centralized around CPU
transitions anyways, so it makes sense to keep the accounting
centralized too.
quoted
I think as long as we have autodeps, the only way on OMAP3 to accurately
do this is to do it for all dependent domains in CPUIdle :(
Or, to get rid of autodeps. ;)
Whats the reason for having them anyway? Some of the wakedeps make sense
(per domain due to hw bugs) but sleepdeps...?
-Tero
On Thu, 26 Jul 2012, Tero Kristo wrote:
On Thu, 2012-07-26 at 10:44 -0700, Kevin Hilman wrote:
quoted
Or, to get rid of autodeps. ;)
Whats the reason for having them anyway?
No one has yet posted tested patches to convert OMAP2/3 to use the IP
block-based clockdomain enable sequence. A few folks posted some patches
for this in 2010 but the patches didn't boot.
It should be a little easier to try this now since we have more data
defined now than we did in the past.
Some of the wakedeps make sense (per domain due to hw bugs) but
sleepdeps...?
That sounds like a different issue.
- Paul
Hi Tero,
On Thursday 26 July 2012 11:57 PM, Tero Kristo wrote:
quoted
Yeah, this is definitely a problem.
quoted
As long as we have autodeps, everything is centralized around CPU
transitions anyways, so it makes sense to keep the accounting
centralized too.
quoted
> I think as long as we have autodeps, the only way on OMAP3 to accurately
> do this is to do it for all dependent domains in CPUIdle:(
Or, to get rid of autodeps.;)
Whats the reason for having them anyway? Some of the wakedeps make sense
(per domain due to hw bugs) but sleepdeps...?
FWIK, the main problem is with modules with clocks under hardware
control. Once a slave module isn't functional and there are no
outstanding OCP accesses pending, the module when sent an IDLEREQ
by PRCM responds with an IDLEACK causing the clkdm to transition
to INACTIVE. A driver which is active can still in the meantime
cause a register access to the module, but the register access
does not act as an event which makes PRCM de-assert IDLEREQ causing
the module to unidle. This causes problems. So as long as there is
a possibility of some code doing a register access on the module
we need to keep it from idling. IIRC the issues we saw on OMAP3
were mostly around PER domain and I think with GPIO in particular.
The problem might be limited to modules with _only_ hardware controlled
clocks (iclks) like GPIO. For others which have an iclk and fclk, we can
always keep the autodeps while fclks are requested and get rid of them
when fclks are disabled. This is exactly what clkdm_clk_enable/disable
functions do, but given in the current mainline even iclks account
for usecounting, a clkdms usecount would never hit 0 causing
clkdm_clk_disable never to be called. (On clkdms which have atleast one
iclk under hardware control).
hwmod keeps all iclks always enabled and under hardware control unless
the OCP interface has a 'OCPIF_SWSUP_IDLE' flag set.
But now with your series, which does not account iclks for usecounting,
I would expect this to be fixed. I was expecting for modules with both
fclk and iclk, as long as the driver has done a pm_runtime_get_sync()
or equivalent the autodeps would be set, and once the driver does a
pm_runtime_put_sync() the autodeps would be removed. This would also
be the same time we clear the Powerdomains previous power state register
and the Powerdomain should ideally immediately transition on not go in
and out with every MPU transition.
So this kind of rules out the problems that my theory above was
suggesting we would have with the $subject patch.
We still have to deal with the iclk only modules like GPIO though.
However a quick test with just your latest usecounting series (without
any of my RFC patches) seems to make me think I am still missing
something.
If you see the counts below for usbhost and dss, they both seem to
go in and out of RET with every MPU transition. Which means the
dependencies are still set.
# cat /debug/pm_debug/count
usbhost_pwrdm
(ON),OFF:0,RET:138,INA:0,ON:139,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
sgx_pwrdm (OFF),OFF:1,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
core_pwrdm
(ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0
per_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
dss_pwrdm (ON),OFF:0,RET:138,INA:0,ON:139,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
cam_pwrdm (RET),OFF:0,RET:1,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
neon_pwrdm (ON),OFF:0,RET:132,INA:6,ON:139,RET-LOGIC-OFF:0
mpu_pwrdm (ON),OFF:0,RET:132,INA:6,ON:139,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
iva2_pwrdm
(RET),OFF:0,RET:1,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0,RET-MEMBANK3-OFF:0,RET-MEMBANK4-OFF:0
However if I look at the dss registers, I don;t see any fclks are
enabled.
# ./readmem 0x48004E00
0x00000000 <- All FCLK disabled.
# ./readmem 0x48004E10
0x00000001 <- ICLK enabled
# ./readmem 0x48004E44
0x00000006 <- dependencies are set with MPU and IVA
# ./readmem 0x48004E48
0x00000003 <- clkdm is under HWSUP.
Any idea why this could be happening?
regards,
Rajendra
Hi Tero,
On Friday 27 July 2012 12:16 PM, Rajendra Nayak wrote:
However a quick test with just your latest usecounting series (without
any of my RFC patches) seems to make me think I am still missing
something.
If you see the counts below for usbhost and dss, they both seem to
go in and out of RET with every MPU transition. Which means the
dependencies are still set.
# cat /debug/pm_debug/count
usbhost_pwrdm
(ON),OFF:0,RET:138,INA:0,ON:139,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
sgx_pwrdm (OFF),OFF:1,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
core_pwrdm
(ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0
per_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
dss_pwrdm
(ON),OFF:0,RET:138,INA:0,ON:139,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
cam_pwrdm (RET),OFF:0,RET:1,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
neon_pwrdm (ON),OFF:0,RET:132,INA:6,ON:139,RET-LOGIC-OFF:0
mpu_pwrdm
(ON),OFF:0,RET:132,INA:6,ON:139,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
iva2_pwrdm
(RET),OFF:0,RET:1,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0,RET-MEMBANK3-OFF:0,RET-MEMBANK4-OFF:0
However if I look at the dss registers, I don;t see any fclks are
enabled.
# ./readmem 0x48004E00
0x00000000 <- All FCLK disabled.
# ./readmem 0x48004E10
0x00000001 <- ICLK enabled
# ./readmem 0x48004E44
0x00000006 <- dependencies are set with MPU and IVA
# ./readmem 0x48004E48
0x00000003 <- clkdm is under HWSUP.
Any idea why this could be happening?
I think I know what the problem is now.
omap3_pm_init calls clkdm_allow_idle for all clkdms, while autoidle on
all iclks is still disabled. This causes autodeps to be set as the iclks
are accounted for in the usecount.
(hwmod would have done a clk_enable() on the iclks and left them enabled
as OCPIF_SWSUP_IDLE isn't set)
static void omap3_clkdm_allow_idle(struct clockdomain *clkdm)
{
if (atomic_read(&clkdm->usecount) > 0)
_clkdm_add_autodeps(clkdm);
omap3xxx_cm_clkdm_enable_hwsup(clkdm->pwrdm.ptr->prcm_offs,
clkdm->clktrctrl_mask);
}
A little later after omap3_pm_init, we enable autoidle for all iclks.
omap2_clkt_iclk_allow_idle decrements the usecount, but leaves the
autodeps still set.
This seems to be causing the dss and usb to also transition along with
MPU.
We will need some way to also clear and set autodeps in
omap2_clkt_iclk_allow_idle/deny_idle.
regards,
Rajendra