2011-03-02 17:18:30

by Dinh.Nguyen

[permalink] [raw]
Subject: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

From: Dinh Nguyen <[email protected]>

Adds initial low power suspend functionality to MX51.
Supports "mem" and "standby" modes.

Signed-off-by: Dinh Nguyen <[email protected]>
---
arch/arm/mach-mx5/Makefile | 1 +
arch/arm/mach-mx5/pm.c | 75 ++++++++++++++++++++++++++++++++++
arch/arm/plat-mxc/include/mach/mxc.h | 1 +
3 files changed, 77 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-mx5/pm.c

diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
index 4f63048..7f9435d 100644
--- a/arch/arm/mach-mx5/Makefile
+++ b/arch/arm/mach-mx5/Makefile
@@ -6,6 +6,7 @@
obj-y := cpu.o mm.o clock-mx51-mx53.o devices.o ehci.o
obj-$(CONFIG_SOC_IMX50) += mm-mx50.o

+obj-$(CONFIG_PM) += pm.o
obj-$(CONFIG_CPU_FREQ_IMX) += cpu_op-mx51.o
obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o
obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o
diff --git a/arch/arm/mach-mx5/pm.c b/arch/arm/mach-mx5/pm.c
new file mode 100644
index 0000000..137d204
--- /dev/null
+++ b/arch/arm/mach-mx5/pm.c
@@ -0,0 +1,75 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+#include <linux/suspend.h>
+#include <asm/mach/map.h>
+#include <mach/system.h>
+#include "crm_regs.h"
+
+static int mx5_suspend_enter(suspend_state_t state)
+{
+ u32 plat_lpc, arm_srpgcr, ccm_clpcr;
+
+ /* always allow platform to issue a deep sleep mode request */
+ plat_lpc = __raw_readl(MXC_CORTEXA8_PLAT_LPC) &
+ ~(MXC_CORTEXA8_PLAT_LPC_DSM);
+ ccm_clpcr = __raw_readl(MXC_CCM_CLPCR) & ~(MXC_CCM_CLPCR_LPM_MASK);
+ arm_srpgcr = __raw_readl(MXC_SRPG_ARM_SRPGCR) & ~(MXC_SRPGCR_PCR);
+
+ switch (state) {
+ case PM_SUSPEND_MEM:
+ ccm_clpcr |= (0x2 << MXC_CCM_CLPCR_LPM_OFFSET);
+ ccm_clpcr |= (0x3 << MXC_CCM_CLPCR_STBY_COUNT_OFFSET);
+ ccm_clpcr |= MXC_CCM_CLPCR_VSTBY;
+ ccm_clpcr |= MXC_CCM_CLPCR_SBYOS;
+ break;
+ case PM_SUSPEND_STANDBY:
+ ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);
+ ccm_clpcr &= ~MXC_CCM_CLPCR_VSTBY;
+ ccm_clpcr &= ~MXC_CCM_CLPCR_SBYOS;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ plat_lpc |= MXC_CORTEXA8_PLAT_LPC_DSM
+ | MXC_CORTEXA8_PLAT_LPC_DBG_DSM;
+ arm_srpgcr |= MXC_SRPGCR_PCR;
+
+ __raw_writel(plat_lpc, MXC_CORTEXA8_PLAT_LPC);
+ __raw_writel(ccm_clpcr, MXC_CCM_CLPCR);
+ __raw_writel(arm_srpgcr, MXC_SRPG_ARM_SRPGCR);
+ __raw_writel(arm_srpgcr, MXC_SRPG_NEON_SRPGCR);
+
+ if (tzic_enable_wake(0) != 0)
+ return -EAGAIN;
+
+ cpu_do_idle();
+ return 0;
+}
+
+static int mx5_pm_valid(suspend_state_t state)
+{
+ return (state > PM_SUSPEND_ON && state <= PM_SUSPEND_MAX);
+}
+
+static const struct platform_suspend_ops mx5_suspend_ops = {
+ .valid = mx5_pm_valid,
+ .enter = mx5_suspend_enter,
+};
+
+static int __init mx5_pm_init(void)
+{
+ if (cpu_is_mx51())
+ suspend_set_ops(&mx5_suspend_ops);
+
+ return 0;
+}
+device_initcall(mx5_pm_init);
diff --git a/arch/arm/plat-mxc/include/mach/mxc.h b/arch/arm/plat-mxc/include/mach/mxc.h
index 04c7a26..a72839f 100644
--- a/arch/arm/plat-mxc/include/mach/mxc.h
+++ b/arch/arm/plat-mxc/include/mach/mxc.h
@@ -53,6 +53,7 @@

#ifndef __ASSEMBLY__
extern unsigned int __mxc_cpu_type;
+int tzic_enable_wake(int is_idle);
#endif

#ifdef CONFIG_ARCH_MX1
--
1.6.0.4


2011-03-02 19:41:54

by Nguyen Dinh-R00091

[permalink] [raw]
Subject: RE: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

Hi Arnaud,


>-----Original Message-----
>From: [email protected] [mailto:linux-arm-kernel-
>[email protected]] On Behalf Of Arnaud Patard
>Sent: Wednesday, March 02, 2011 1:35 PM
>To: Nguyen Dinh-R00091
>Cc: [email protected]; [email protected]; [email protected]; u.kleine-
>[email protected]; Zhang Lily-R58066; [email protected]
>Subject: Re: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.
>
><[email protected]> writes:
>
>Hi,
>
>> From: Dinh Nguyen <[email protected]>
>>
>> Adds initial low power suspend functionality to MX51.
>> Supports "mem" and "standby" modes.
>
>I've very quickly tried suspend to mem on my system and despite having set
>some irq as wakeup source, it doesn't wake up. Is there some extra step
>to be done in order to test your patch ?
>
>Moreover, the code I've seen for suspend is very different. It's seems
>to handle some cpufreq stuff and seems to be copying some code into some
>special ram. Is all this stuff unneeded ?

I am able to wakeup using gpio-key on my mx51-babbage system if I comment out the most of the drivers except for the gpio-keys driver. Yes, there are additional cpufreq and code that needs to get run in IRAM for mx51, which I will add on to this patch. This is just the initial patch to put the SoC in to suspend and have other developers add to it.

Thanks,

Dinh

>
>Arnaud
>
>_______________________________________________
>linux-arm-kernel mailing list
>[email protected]
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2011-03-02 19:42:44

by Arnaud Patard

[permalink] [raw]
Subject: Re: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

<[email protected]> writes:

Hi,

> From: Dinh Nguyen <[email protected]>
>
> Adds initial low power suspend functionality to MX51.
> Supports "mem" and "standby" modes.

I've very quickly tried suspend to mem on my system and despite having set
some irq as wakeup source, it doesn't wake up. Is there some extra step
to be done in order to test your patch ?

Moreover, the code I've seen for suspend is very different. It's seems
to handle some cpufreq stuff and seems to be copying some code into some
special ram. Is all this stuff unneeded ?

Arnaud

2011-03-02 21:52:40

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

On Wed, Mar 02, 2011 at 11:17:58AM -0600, [email protected] wrote:
> From: Dinh Nguyen <[email protected]>
>
> Adds initial low power suspend functionality to MX51.
> Supports "mem" and "standby" modes.
>
> Signed-off-by: Dinh Nguyen <[email protected]>
> ---
> arch/arm/mach-mx5/Makefile | 1 +
> arch/arm/mach-mx5/pm.c | 75 ++++++++++++++++++++++++++++++++++
> arch/arm/plat-mxc/include/mach/mxc.h | 1 +
> 3 files changed, 77 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/mach-mx5/pm.c
>
> diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
> index 4f63048..7f9435d 100644
> --- a/arch/arm/mach-mx5/Makefile
> +++ b/arch/arm/mach-mx5/Makefile
> @@ -6,6 +6,7 @@
> obj-y := cpu.o mm.o clock-mx51-mx53.o devices.o ehci.o
> obj-$(CONFIG_SOC_IMX50) += mm-mx50.o
>
> +obj-$(CONFIG_PM) += pm.o
> obj-$(CONFIG_CPU_FREQ_IMX) += cpu_op-mx51.o
> obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o
> obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o
> diff --git a/arch/arm/mach-mx5/pm.c b/arch/arm/mach-mx5/pm.c
> new file mode 100644
> index 0000000..137d204
> --- /dev/null
> +++ b/arch/arm/mach-mx5/pm.c
I'd like to have that called pm-imx51.c

> @@ -0,0 +1,75 @@
> +/*
> + * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +#include <linux/suspend.h>
> +#include <asm/mach/map.h>
> +#include <mach/system.h>
> +#include "crm_regs.h"
> +
> +static int mx5_suspend_enter(suspend_state_t state)
> +{
> + u32 plat_lpc, arm_srpgcr, ccm_clpcr;
> +
> + /* always allow platform to issue a deep sleep mode request */
> + plat_lpc = __raw_readl(MXC_CORTEXA8_PLAT_LPC) &
> + ~(MXC_CORTEXA8_PLAT_LPC_DSM);
> + ccm_clpcr = __raw_readl(MXC_CCM_CLPCR) & ~(MXC_CCM_CLPCR_LPM_MASK);
> + arm_srpgcr = __raw_readl(MXC_SRPG_ARM_SRPGCR) & ~(MXC_SRPGCR_PCR);
> +
> + switch (state) {
> + case PM_SUSPEND_MEM:
> + ccm_clpcr |= (0x2 << MXC_CCM_CLPCR_LPM_OFFSET);
> + ccm_clpcr |= (0x3 << MXC_CCM_CLPCR_STBY_COUNT_OFFSET);
the parentheses aren't needed here

> + ccm_clpcr |= MXC_CCM_CLPCR_VSTBY;
> + ccm_clpcr |= MXC_CCM_CLPCR_SBYOS;
> + break;
> + case PM_SUSPEND_STANDBY:
> + ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);
ditto

> + ccm_clpcr &= ~MXC_CCM_CLPCR_VSTBY;
> + ccm_clpcr &= ~MXC_CCM_CLPCR_SBYOS;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + plat_lpc |= MXC_CORTEXA8_PLAT_LPC_DSM
> + | MXC_CORTEXA8_PLAT_LPC_DBG_DSM;
> + arm_srpgcr |= MXC_SRPGCR_PCR;
> +
> + __raw_writel(plat_lpc, MXC_CORTEXA8_PLAT_LPC);
> + __raw_writel(ccm_clpcr, MXC_CCM_CLPCR);
> + __raw_writel(arm_srpgcr, MXC_SRPG_ARM_SRPGCR);
> + __raw_writel(arm_srpgcr, MXC_SRPG_NEON_SRPGCR);
> +
> + if (tzic_enable_wake(0) != 0)
> + return -EAGAIN;
> +
> + cpu_do_idle();
> + return 0;
> +}
> +
> +static int mx5_pm_valid(suspend_state_t state)
> +{
> + return (state > PM_SUSPEND_ON && state <= PM_SUSPEND_MAX);
> +}
> +
> +static const struct platform_suspend_ops mx5_suspend_ops = {
> + .valid = mx5_pm_valid,
> + .enter = mx5_suspend_enter,
> +};
> +
> +static int __init mx5_pm_init(void)
> +{
> + if (cpu_is_mx51())
> + suspend_set_ops(&mx5_suspend_ops);
I'd prefer to have that called by imx51_init_early.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |

2011-03-02 23:51:50

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

Uwe,

On Wed, 2 Mar 2011, Uwe Kleine-K?nig wrote:
> On Wed, Mar 02, 2011 at 11:17:58AM -0600, [email protected] wrote:
> > From: Dinh Nguyen <[email protected]>

> > --- /dev/null
> > +++ b/arch/arm/mach-mx5/pm.c
> I'd like to have that called pm-imx51.c

And I'd like to have a pony.

> > + ccm_clpcr |= (0x3 << MXC_CCM_CLPCR_STBY_COUNT_OFFSET);
> the parentheses aren't needed here

Could you finally provide a patch to checkpatch.pl or git commit which
resolves that issue once and forever ?

Not to mention the fact, that those parentheses are not disturbing the
readability of that code at all.

> > + ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);
> ditto

Ditto.

> > +static int __init mx5_pm_init(void)
> I'd prefer to have that called by imx51_init_early.

And the reason is?

1) your personal preference
2) there is some useful technical reason

If #1, then this comment was just waste of electrons
If #2, you failed to provide some reasonable explanation

Again, I'd like to have a pony.

Seriously, while all of us admire your invaluable skills of running
scripts over patches and kernel code, that kind of review you are
trying to provide is utterly useless.

1) The patch itself has been questioned about its correctness hours
before you added the output of your secret script. It was already
reported to be non functional. So what's the value of adding
scriptable review to it?

2) As long as you do not see the most obvious functional problems with
a patch please spare your script computing power and the bandwidth
you are consuming by your futile attempts to gain a profile as a
patch reviewer.

Just for the record. The obvious bug with this code is this:

> > + plat_lpc |= MXC_CORTEXA8_PLAT_LPC_DSM
> > + | MXC_CORTEXA8_PLAT_LPC_DBG_DSM;
> > + arm_srpgcr |= MXC_SRPGCR_PCR;
> > +
> > + __raw_writel(plat_lpc, MXC_CORTEXA8_PLAT_LPC);
> > + __raw_writel(ccm_clpcr, MXC_CCM_CLPCR);
> > + __raw_writel(arm_srpgcr, MXC_SRPG_ARM_SRPGCR);
> > + __raw_writel(arm_srpgcr, MXC_SRPG_NEON_SRPGCR);
> > +
> > + if (tzic_enable_wake(0) != 0)
> > + return -EAGAIN;

It happily returns here with -EAGAIN w/o undoing the already committed
changes which are preparatory to the tzic_enable_wake() check.

I might be wrong as usual, but if this is cleaned up by the calling
code magically then the "return -EINVAL"; lacks a big fat
comment. While such an omission is not a triggerable bug it documents
the lack of tought and taste by creating asymetric mechanisms w/o the
courtesy to document them properly. Even if documented, asymetric
interfaces are crap most of the time.

> > + cpu_do_idle();
> > + return 0;
> > +}

Thanks,

tglx

2011-03-03 09:35:44

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

Guten Morgen Thomas,

On Thu, Mar 03, 2011 at 12:51:32AM +0100, Thomas Gleixner wrote:
> Uwe,
>
> On Wed, 2 Mar 2011, Uwe Kleine-K?nig wrote:
> > On Wed, Mar 02, 2011 at 11:17:58AM -0600, [email protected] wrote:
> > > From: Dinh Nguyen <[email protected]>
>
> > > --- /dev/null
> > > +++ b/arch/arm/mach-mx5/pm.c
> > I'd like to have that called pm-imx51.c
>
> And I'd like to have a pony.
http://www.carl-russ-schule.de/files/Pony.jpg

>
> > > + ccm_clpcr |= (0x3 << MXC_CCM_CLPCR_STBY_COUNT_OFFSET);
> > the parentheses aren't needed here
>
> Could you finally provide a patch to checkpatch.pl or git commit which
> resolves that issue once and forever ?
>
> Not to mention the fact, that those parentheses are not disturbing the
> readability of that code at all.
So it seems we're different.

>
> > > + ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);
> > ditto
>
> Ditto.
>
> > > +static int __init mx5_pm_init(void)
> > I'd prefer to have that called by imx51_init_early.
>
> And the reason is?
>
> 1) your personal preference
> 2) there is some useful technical reason
>
> If #1, then this comment was just waste of electrons
> If #2, you failed to provide some reasonable explanation
Actually it's #2, and to quote a different review[1]:

Reviewers hint to a correct solution and you are supposed to
lookup what that solution means and act accordingly. If you do
not understand the hint or its implications please ask [...]

> Again, I'd like to have a pony.
http://h-6.abload.de/img/pony01_pixelquelle_dal8701.jpg

> Seriously, while all of us admire your invaluable skills of running
> scripts over patches and kernel code, that kind of review you are
> trying to provide is utterly useless.
actually it's not a script, but I guess that doesn't matter much.

> 1) The patch itself has been questioned about its correctness hours
> before you added the output of your secret script. It was already
> reported to be non functional. So what's the value of adding
> scriptable review to it?
It might save the patch sender from a third iteration.

> 2) As long as you do not see the most obvious functional problems with
> a patch please spare your script computing power and the bandwidth
> you are consuming by your futile attempts to gain a profile as a
> patch reviewer.
The functionallity of the patch has been questioned before so I didn't
see a value in repeating that :-) To be honest, I didn't check for
functional problems, but IMHO it's OK to provide feedback about a part
of the problems if you don't see all of them.

Best regards
Uwe

[1] http://article.gmane.org/gmane.linux.kernel/1107265

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |

2011-03-03 11:02:33

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

On Thu, 3 Mar 2011, Uwe Kleine-K?nig wrote:
> On Thu, Mar 03, 2011 at 12:51:32AM +0100, Thomas Gleixner wrote:
> > > > +static int __init mx5_pm_init(void)
> > > I'd prefer to have that called by imx51_init_early.
> >
> > And the reason is?
> >
> > 1) your personal preference
> > 2) there is some useful technical reason
> >
> > If #1, then this comment was just waste of electrons
> > If #2, you failed to provide some reasonable explanation
> Actually it's #2, and to quote a different review[1]:
>
> Reviewers hint to a correct solution and you are supposed to
> lookup what that solution means and act accordingly. If you do
> not understand the hint or its implications please ask [...]

I said the above when I hinted to use DEFINE_SPINLOCK(lock) instead of
static spinlock_t lock. And that requires to lookup what
DEFINE_SPINLOCK() actually does, which is a reasonable request.

How is the author of that code supposed to figure out what the merit
of s/mx5_pm_init/imx51_init_early/ is? By looking up your preferences
in google or what?

Using random quotes and failing to see why they don't apply is just
another proof of my assumption #1

Thanks,

tglx

2011-03-03 11:49:54

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

Hello Thomas,

On Thu, Mar 03, 2011 at 12:02:15PM +0100, Thomas Gleixner wrote:
> On Thu, 3 Mar 2011, Uwe Kleine-K?nig wrote:
> > On Thu, Mar 03, 2011 at 12:51:32AM +0100, Thomas Gleixner wrote:
> > > > > +static int __init mx5_pm_init(void)
> > > > I'd prefer to have that called by imx51_init_early.
> > >
> > > And the reason is?
> > >
> > > 1) your personal preference
> > > 2) there is some useful technical reason
> > >
> > > If #1, then this comment was just waste of electrons
> > > If #2, you failed to provide some reasonable explanation
> > Actually it's #2, and to quote a different review[1]:
> >
> > Reviewers hint to a correct solution and you are supposed to
> > lookup what that solution means and act accordingly. If you do
> > not understand the hint or its implications please ask [...]
>
> I said the above when I hinted to use DEFINE_SPINLOCK(lock) instead of
> static spinlock_t lock. And that requires to lookup what
> DEFINE_SPINLOCK() actually does, which is a reasonable request.
>
> How is the author of that code supposed to figure out what the merit
> of s/mx5_pm_init/imx51_init_early/ is? By looking up your preferences
> in google or what?
Note I didn't suggest to change the function name.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |

2011-03-03 11:52:54

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

On Wed, Mar 02, 2011 at 10:52:38PM +0100, Uwe Kleine-K?nig wrote:
> > +static int __init mx5_pm_init(void)
> > +{
> > + if (cpu_is_mx51())
> > + suspend_set_ops(&mx5_suspend_ops);
> I'd prefer to have that called by imx51_init_early.

This function name looks fine. As we now have an init_early in the
arch hooks, let's keep things called foo_init_early() to that use
and not start using 'early' for stuff used from initcalls.

Renaming this is a recipe for causing confusion and having grep hit
false positives. Please leave it as is.

2011-03-03 12:47:01

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

On Thu, Mar 03, 2011 at 11:52:42AM +0000, Russell King - ARM Linux wrote:
> On Wed, Mar 02, 2011 at 10:52:38PM +0100, Uwe Kleine-K?nig wrote:
> > > +static int __init mx5_pm_init(void)
> > > +{
> > > + if (cpu_is_mx51())
> > > + suspend_set_ops(&mx5_suspend_ops);
> > I'd prefer to have that called by imx51_init_early.
>
> This function name looks fine. As we now have an init_early in the
> arch hooks, let's keep things called foo_init_early() to that use
> and not start using 'early' for stuff used from initcalls.
>
> Renaming this is a recipe for causing confusion and having grep hit
> false positives. Please leave it as is.
It seems you and Thomas both didn't notice the "by" in my sentence.
Or maybe it's not proper English? The thing I wanted to express is that
instead of introducing another initcall I prefer that imx51_init_early
calls mx5_pm_init instead. The name mx5_pm_init is fine for me, though
imx51_pm_init would still be better.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |

2011-03-03 13:46:32

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

On Thu, Mar 03, 2011 at 01:46:58PM +0100, Uwe Kleine-K?nig wrote:
> On Thu, Mar 03, 2011 at 11:52:42AM +0000, Russell King - ARM Linux wrote:
> > On Wed, Mar 02, 2011 at 10:52:38PM +0100, Uwe Kleine-K?nig wrote:
> > > > +static int __init mx5_pm_init(void)
> > > > +{
> > > > + if (cpu_is_mx51())
> > > > + suspend_set_ops(&mx5_suspend_ops);
> > > I'd prefer to have that called by imx51_init_early.
> >
> > This function name looks fine. As we now have an init_early in the
> > arch hooks, let's keep things called foo_init_early() to that use
> > and not start using 'early' for stuff used from initcalls.
> >
> > Renaming this is a recipe for causing confusion and having grep hit
> > false positives. Please leave it as is.
> It seems you and Thomas both didn't notice the "by" in my sentence.
> Or maybe it's not proper English? The thing I wanted to express is that
> instead of introducing another initcall I prefer that imx51_init_early
> calls mx5_pm_init instead. The name mx5_pm_init is fine for me, though
> imx51_pm_init would still be better.

Is there a reason to set this really really early? What's that reason
exactly?

You can't suspend the system until the scheduler is up and running.
Neither can you use cpuidle. So it seems there isn't a pressing reason
to place this stuff really early in the fragile part of kernel
initialization.

2011-03-03 20:15:17

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

On Thu, Mar 03, 2011 at 01:45:51PM +0000, Russell King - ARM Linux wrote:
> On Thu, Mar 03, 2011 at 01:46:58PM +0100, Uwe Kleine-K?nig wrote:
> > On Thu, Mar 03, 2011 at 11:52:42AM +0000, Russell King - ARM Linux wrote:
> > > On Wed, Mar 02, 2011 at 10:52:38PM +0100, Uwe Kleine-K?nig wrote:
> > > > > +static int __init mx5_pm_init(void)
> > > > > +{
> > > > > + if (cpu_is_mx51())
> > > > > + suspend_set_ops(&mx5_suspend_ops);
> > > > I'd prefer to have that called by imx51_init_early.
> > >
> > > This function name looks fine. As we now have an init_early in the
> > > arch hooks, let's keep things called foo_init_early() to that use
> > > and not start using 'early' for stuff used from initcalls.
> > >
> > > Renaming this is a recipe for causing confusion and having grep hit
> > > false positives. Please leave it as is.
> > It seems you and Thomas both didn't notice the "by" in my sentence.
> > Or maybe it's not proper English? The thing I wanted to express is that
> > instead of introducing another initcall I prefer that imx51_init_early
> > calls mx5_pm_init instead. The name mx5_pm_init is fine for me, though
> > imx51_pm_init would still be better.
>
> Is there a reason to set this really really early? What's that reason
> exactly?
No there is no reason. If there were a imx51_init this would be the
right place. Maybe it's time to implement it.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |