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 ?
Won't it be worth to add a new WFI_SLEEP state to the cpuidle driver ?
Thanks for the review.
-- 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
