| Message ID | 1382685074-16502-1-git-send-email-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linaro.org> > --- > arch/arm/mach-s3c64xx/cpuidle.c | 38 > ++++++++++++++++---------------------- arch/arm/mach-s3c64xx/pm.c | > 33 ++++++++++++++++++++++++++------- 2 files changed, 42 > insertions(+), 29 deletions(-) > > diff --git a/arch/arm/mach-s3c64xx/cpuidle.c > b/arch/arm/mach-s3c64xx/cpuidle.c index 3c8ab07..8022f5f 100644 > --- a/arch/arm/mach-s3c64xx/cpuidle.c > +++ b/arch/arm/mach-s3c64xx/cpuidle.c > @@ -9,33 +9,16 @@ > * published by the Free Software Foundation. > */ [snip] > static int s3c64xx_enter_idle(struct cpuidle_device *dev, > struct cpuidle_driver *drv, > int index) > { > - unsigned long tmp; > - > - /* Setup PWRCFG to enter idle mode */ > - tmp = __raw_readl(S3C64XX_PWR_CFG); > - tmp &= ~S3C64XX_PWRCFG_CFG_WFI_MASK; > - tmp |= S3C64XX_PWRCFG_CFG_WFI_IDLE; Note the S3C64XX_PWRCFG_CFG_WFI_IDLE flag here. > - __raw_writel(tmp, S3C64XX_PWR_CFG); > - > - cpu_do_idle(); > + s3c64xx_standby(); > > return index; > } [snip] > +static void s3c64xx_set_standby(void) > +{ > + unsigned long tmp; > + > + tmp = __raw_readl(S3C64XX_PWR_CFG); > + tmp &= ~S3C64XX_PWRCFG_CFG_WFI_MASK; > + tmp |= S3C64XX_PWRCFG_CFG_WFI_SLEEP; Note the S3C64XX_PWRCFG_CFG_WFI_SLEEP flag here, which is different from the S3C64XX_PWRCFG_CFG_WFI_IDLE flag above. > + __raw_writel(tmp, S3C64XX_PWR_CFG); > +} > + > +static void s3c64xx_standby(void) > +{ > + s3c64xx_set_standby(); > + cpu_do_idle(); > +} > + > /* since both s3c6400 and s3c6410 share the same sleep pm calls, we > * put the per-cpu code in here until any new cpu comes along and > changes * this. > @@ -269,11 +286,7 @@ static int s3c64xx_cpu_suspend(unsigned long arg) > unsigned long tmp; > > /* set our standby method to sleep */ > - > - 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. 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. 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. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@linaro.org> >> --- >> 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
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@linaro.org> > >> --- > >> > >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@linaro.org> >>>> --- >>>> >>>> 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
On 10/25/2013 03:23 PM, Daniel Lezcano wrote: [ ... ] >>> 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 ? Hi Thomas, just a reminder about the question above, so I can go ahead: fix what you pointed out or remove the driver directly. You mentionned in the previous email the STOP is not usful because it can be controlled by manually outside of the cpuidle driver. But I see in the documentation, the stop states power gates the cpu and deep-stop stops the regulator. If these states have to been added later, still it worth to remove the driver ? Thanks! -- Daniel
Hi Daniel, On Wednesday 30 of October 2013 14:43:51 Daniel Lezcano wrote: > On 10/25/2013 03:23 PM, Daniel Lezcano wrote: > > [ ... ] > > >>> 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 ? > > Hi Thomas, > > just a reminder about the question above, so I can go ahead: fix what > you pointed out or remove the driver directly. > > You mentionned in the previous email the STOP is not usful because it > can be controlled by manually outside of the cpuidle driver. But I see > in the documentation, the stop states power gates the cpu and deep-stop > stops the regulator. STOP clock-gates the CPU and DEEP-STOP power-gates it by stopping the regulator. There are some interesting aspects of those modes, like memory self- refresh, PLL power-off and system-wide down clocking, but I believe they are too tricky to handle (coupling with device PM, stopped timers) and with too little possible power saving to justify the effort of adding support for them. In the end, I haven't seen support for them implemented even in strange vendor kernels used even on production devices, like Android phones. > > If these states have to been added later, still it worth to remove the > driver ? I don't think that anybody is even going to add support for them and by anybody I should probably mean myself, since I'm currently the only person actively adding new code for this platform, as a part of my hobby. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/30/2013 03:40 PM, Tomasz Figa wrote: > Hi Daniel, > > On Wednesday 30 of October 2013 14:43:51 Daniel Lezcano wrote: >> On 10/25/2013 03:23 PM, Daniel Lezcano wrote: >> >> [ ... ] >> >>>>> 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 ? >> >> Hi Thomas, >> >> just a reminder about the question above, so I can go ahead: fix what >> you pointed out or remove the driver directly. >> >> You mentionned in the previous email the STOP is not usful because it >> can be controlled by manually outside of the cpuidle driver. But I see >> in the documentation, the stop states power gates the cpu and deep-stop >> stops the regulator. > > STOP clock-gates the CPU and DEEP-STOP power-gates it by stopping the > regulator. > > There are some interesting aspects of those modes, like memory self- > refresh, PLL power-off and system-wide down clocking, but I believe they > are too tricky to handle (coupling with device PM, stopped timers) and > with too little possible power saving to justify the effort of adding > support for them. > > In the end, I haven't seen support for them implemented even in strange > vendor kernels used even on production devices, like Android phones. > >> >> If these states have to been added later, still it worth to remove the >> driver ? > > I don't think that anybody is even going to add support for them and by > anybody I should probably mean myself, since I'm currently the only person > actively adding new code for this platform, as a part of my hobby. Ok, I will kill the driver then. Thanks -- Daniel
diff --git a/arch/arm/mach-s3c64xx/cpuidle.c b/arch/arm/mach-s3c64xx/cpuidle.c index 3c8ab07..8022f5f 100644 --- a/arch/arm/mach-s3c64xx/cpuidle.c +++ b/arch/arm/mach-s3c64xx/cpuidle.c @@ -9,33 +9,16 @@ * published by the Free Software Foundation. */ -#include <linux/kernel.h> -#include <linux/init.h> #include <linux/cpuidle.h> -#include <linux/io.h> -#include <linux/export.h> -#include <linux/time.h> +#include <linux/platform_device.h> -#include <asm/proc-fns.h> - -#include <mach/map.h> - -#include "regs-sys.h" -#include "regs-syscon-power.h" +static void (*s3c64xx_standby)(void); static int s3c64xx_enter_idle(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { - unsigned long tmp; - - /* Setup PWRCFG to enter idle mode */ - tmp = __raw_readl(S3C64XX_PWR_CFG); - tmp &= ~S3C64XX_PWRCFG_CFG_WFI_MASK; - tmp |= S3C64XX_PWRCFG_CFG_WFI_IDLE; - __raw_writel(tmp, S3C64XX_PWR_CFG); - - cpu_do_idle(); + s3c64xx_standby(); return index; } @@ -56,8 +39,19 @@ static struct cpuidle_driver s3c64xx_cpuidle_driver = { .state_count = 1, }; -static int __init s3c64xx_init_cpuidle(void) +static int s3c64xx_cpuidle_probe(struct platform_device *dev) { + s3c64xx_standby = (void *)(dev->dev.platform_data); + return cpuidle_register(&s3c64xx_cpuidle_driver, NULL); } -device_initcall(s3c64xx_init_cpuidle); + +static struct platform_driver s3c64xx_driver_cpuidle = { + .driver = { + .name = "cpuidle-s3c64xx", + .owner = THIS_MODULE, + }, + .probe = s3c64xx_cpuidle_probe, +}; + +module_platform_driver(s3c64xx_driver_cpuidle); diff --git a/arch/arm/mach-s3c64xx/pm.c b/arch/arm/mach-s3c64xx/pm.c index 6a1f91f..534fb4e 100644 --- a/arch/arm/mach-s3c64xx/pm.c +++ b/arch/arm/mach-s3c64xx/pm.c @@ -18,6 +18,7 @@ #include <linux/io.h> #include <linux/gpio.h> #include <linux/pm_domain.h> +#include <linux/platform_device.h> #include <mach/map.h> #include <mach/irqs.h> @@ -259,6 +260,22 @@ void s3c_pm_save_core(void) s3c_pm_do_save(core_save, ARRAY_SIZE(core_save)); } +static void s3c64xx_set_standby(void) +{ + unsigned long tmp; + + tmp = __raw_readl(S3C64XX_PWR_CFG); + tmp &= ~S3C64XX_PWRCFG_CFG_WFI_MASK; + tmp |= S3C64XX_PWRCFG_CFG_WFI_SLEEP; + __raw_writel(tmp, S3C64XX_PWR_CFG); +} + +static void s3c64xx_standby(void) +{ + s3c64xx_set_standby(); + cpu_do_idle(); +} + /* since both s3c6400 and s3c6410 share the same sleep pm calls, we * put the per-cpu code in here until any new cpu comes along and changes * this. @@ -269,11 +286,7 @@ static int s3c64xx_cpu_suspend(unsigned long arg) unsigned long tmp; /* set our standby method to sleep */ - - tmp = __raw_readl(S3C64XX_PWR_CFG); - tmp &= ~S3C64XX_PWRCFG_CFG_WFI_MASK; - tmp |= S3C64XX_PWRCFG_CFG_WFI_SLEEP; - __raw_writel(tmp, S3C64XX_PWR_CFG); + s3c64xx_set_standby(); /* clear any old wakeup */ @@ -348,6 +361,13 @@ int __init s3c64xx_pm_init(void) return 0; } +static struct platform_device s3c64xx_cpuidle_device = { + .name = "cpuidle-s3c64xx", + .dev = { + .platform_data = s3c64xx_standby, + }, +}; + static __init int s3c64xx_pm_initcall(void) { pm_cpu_prep = s3c64xx_pm_prepare; @@ -364,8 +384,7 @@ static __init int s3c64xx_pm_initcall(void) gpio_direction_output(S3C64XX_GPN(14), 0); gpio_direction_output(S3C64XX_GPN(15), 0); #endif - - return 0; + return platform_device_register(&s3c64xx_cpuidle_device); } arch_initcall(s3c64xx_pm_initcall);
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. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- arch/arm/mach-s3c64xx/cpuidle.c | 38 ++++++++++++++++---------------------- arch/arm/mach-s3c64xx/pm.c | 33 ++++++++++++++++++++++++++------- 2 files changed, 42 insertions(+), 29 deletions(-)