On 10/25/2013 10:52 PM, Tomasz Figa wrote:
On Friday 25 of October 2013 21:13:35 Daniel Lezcano wrote:
On 10/25/2013 12:39 PM, Tomasz Figa wrote:
Hi Daniel,
[Sending again, without HTML part. Sorry for the noise.]
On Friday 25 of October 2013 09:11:13 Daniel Lezcano wrote:
The driver is tied with the pm low level code making difficult to
split
the driver into a more arch independent code. The platform driver
allows
to move the standby callback into the platform data field and use a
simple driver with no more dependency on the low level code.
The standby callback has a portion of code to set the standby method
and
the effective cpu_do_idle switching the cpu to the right mode. As this
code is redundant in the cpu suspend code, it has been factored out
when
implementing the standby methdod.
By this way, the driver is ready to be moved out to the
drivers/cpuidle.
The idea itself is quite good, but unfortunately I have to NAK this.
Please see details in comments below.
Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
---
arch/arm/mach-s3c64xx/cpuidle.c | 38
++++++++++++++++---------------------- arch/arm/mach-s3c64xx/pm.c
|
33 ++++++++++++++++++++++++++------- 2 files changed, 42
insertions(+), 29 deletions(-)
[ ... ]
-
- tmp = __raw_readl(S3C64XX_PWR_CFG);
- tmp &= ~S3C64XX_PWRCFG_CFG_WFI_MASK;
- tmp |= S3C64XX_PWRCFG_CFG_WFI_SLEEP;
Finally note the S3C64XX_PWRCFG_CFG_WFI_SLEEP flag here again.
Ouch ! I missed it. Thanks for spotting the problem.
I believe it should be visible now what's wrong with this patch. To
make
sure it is, let me explain how the system controller of S3C64xx handles
WFI requests.
When the CPU issues WFI request to the syscon, it takes an action
depending on how it is configured. A bit field is there in one of
syscon registers (S3C64XX_PWR_CFG) that selects what action to perform
in case of WFI request.
You can program the syscon to ignore the request, enter IDLE mode,
enter
STOP mode or enter SLEEP mode. As the names suggest, for cpuidle, it
needs to be programmed for IDLE mode and for system-wide sleep it
needs to be set to SLEEP mode. STOP mode is not very useful as it has
mostly the same effect that can be achieved by performing fine-grained
clock and power gating of peripherals manually, so it is unused by
Linux.
Yes, this is what I assumed but I missed the CFG_WFI_SLEEP flag, my eyes
read CFG_WFI_IDLE.
Now, my take on the issue you are trying to solve would be a bit
different. Since the S3C64xx does not have any interesting cpuidle
modes, just a normal, clock-gated WFI mode, it does not need to have a
cpuidle driver at all. All that is needed is simply setting up the
S3C64XX_PWRCFG_CFG_WFI field to S3C64XX_PWRCFG_CFG_WFI_IDLE early at
boot-up, then set it to S3C64XX_PWRCFG_CFG_WFI_SLEEP just before
entering the sleep mode and restore it back to
S3C64XX_PWRCFG_CFG_WFI_IDLE after waking up.
So you are suggesting to remove the cpuidle driver ?
Exactly.
I see.
Won't it be worth to add a new WFI_SLEEP state to the cpuidle driver ?
I don't think so. How a suspend-to-RAM specific thing like WFI_SLEEP could
be relevant to a cpuidle driver? (Unless there are some plans to
consolidate STR with cpuidle that I haven't heard about...)
I finally found a documentation for the s3c6410x and the description of
the different modes. Indeed, the sleep mode is not adequate for a
cpuidle state. What about the 'stop' and 'deep stop' state ?
What is 'STR' ?
Thanks
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
