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. > > 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...) Best regards, Tomasz -- 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
 |