Patchwork [1/3] ARM: imx: Add common imx cpuidle init functionality.

login
register
mail settings
Submitter Shawn Guo
Date April 23, 2012, 7:10 a.m.
Message ID <20120423071013.GS26306@S2101-09.ap.freescale.net>
Download mbox | patch
Permalink /patch/154329/
State New
Headers show

Comments

Shawn Guo - April 23, 2012, 7:10 a.m.
On Mon, Apr 23, 2012 at 08:56:23AM +0200, Sascha Hauer wrote:
> On Mon, Apr 23, 2012 at 02:53:01PM +0800, Shawn Guo wrote:
> > On Mon, Apr 23, 2012 at 08:27:39AM +0200, Sascha Hauer wrote:
> > > On Mon, Apr 23, 2012 at 01:18:21PM +0800, Shawn Guo wrote:
> > > > On Sun, Apr 22, 2012 at 11:44:39PM -0500, Rob Lee wrote:
...
> > > > > Since device_initcall isn't platform specific, it seems I would still
> > > > > need a cpu_is_imx6q() function or similiar functionality from a device
> > > > > tree call.  Or do you have something else in mind that I'm not seeing?
> > > > > 
> > > > I guess Sascha is asking for something like the following.
> > > > 
> > > > static int __init imx_device_init(void)
> > > > {
> > > > 	imx5_device_init();
> > > > 	imx6_device_init();
> > > > }
> > > > device_initcall(imx_device_init)
> > > > 
> > > > static int __init imx6_device_init(void)
> > > > {
> > > > 	/*
> > > > 	 * do whatever needs to get done in device_initcall time
> > > > 	 */
> > > > }
> > > 
> > > The problem is more how we can detect that we are actually running a
> > > i.MX6 SoC. We could directly ask the devicetree in an initcall or we
> > > could introduce a cpu_is_mx6() just like we have a macro for all other
> > > i.MX SoCs.
> > > 
> > Oops, my reply was a brain-dead one.  Hmm, then it seems that we have
> > to introduce cpu_is_mx6() which I tried hard to avoid.  I do not have
> > a preference between defining a macro and asking device tree.
> 
> Since we already have a place in early setup code in which we know that
> we are running on an i.MX6 I suggest for the sake of the symmetry of the
> universe that we introduce a cpu_is_mx6.
> 
Let me try last time.  What about having a late_initcall hook in
machine_desc?

Regards,
Shawn

8<---
Sascha Hauer - April 23, 2012, 7:48 a.m.
On Mon, Apr 23, 2012 at 03:10:15PM +0800, Shawn Guo wrote:
> On Mon, Apr 23, 2012 at 08:56:23AM +0200, Sascha Hauer wrote:
> > On Mon, Apr 23, 2012 at 02:53:01PM +0800, Shawn Guo wrote:
> > > On Mon, Apr 23, 2012 at 08:27:39AM +0200, Sascha Hauer wrote:
> > > > On Mon, Apr 23, 2012 at 01:18:21PM +0800, Shawn Guo wrote:
> > > > > On Sun, Apr 22, 2012 at 11:44:39PM -0500, Rob Lee wrote:
> ...
> > > > i.MX6 SoC. We could directly ask the devicetree in an initcall or we
> > > > could introduce a cpu_is_mx6() just like we have a macro for all other
> > > > i.MX SoCs.
> > > > 
> > > Oops, my reply was a brain-dead one.  Hmm, then it seems that we have
> > > to introduce cpu_is_mx6() which I tried hard to avoid.  I do not have
> > > a preference between defining a macro and asking device tree.
> > 
> > Since we already have a place in early setup code in which we know that
> > we are running on an i.MX6 I suggest for the sake of the symmetry of the
> > universe that we introduce a cpu_is_mx6.
> > 
> Let me try last time.  What about having a late_initcall hook in
> machine_desc?

Also fine with me.

> 
> Regards,
> Shawn
> 
> 8<---
> 
> diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
> index d7692ca..0b1c94b 100644
> --- a/arch/arm/include/asm/mach/arch.h
> +++ b/arch/arm/include/asm/mach/arch.h
> @@ -43,6 +43,7 @@ struct machine_desc {
>         void                    (*init_irq)(void);
>         struct sys_timer        *timer;         /* system tick timer    */
>         void                    (*init_machine)(void);
> +       void                    (*init_late)(void);
>  #ifdef CONFIG_MULTI_IRQ_HANDLER
>         void                    (*handle_irq)(struct pt_regs *);
>  #endif
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index ebfac78..549f036 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -800,6 +800,14 @@ static int __init customize_machine(void)
>  }
>  arch_initcall(customize_machine);
> 
> +static int __init init_machine_late(void)
> +{
> +       if (machine_desc->init_late)
> +               machine_desc->init_late();
> +       return 0;
> +}
> +late_initcall(init_machine_late);
> +
>  #ifdef CONFIG_KEXEC
>  static inline unsigned long long get_total_mem(void)
>  {
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index da6c1d9..0e3640f 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -142,6 +142,7 @@ DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad (Device Tree)")
>         .handle_irq     = imx6q_handle_irq,
>         .timer          = &imx6q_timer,
>         .init_machine   = imx6q_init_machine,
> +       .init_late      = imx6q_init_late,
>         .dt_compat      = imx6q_dt_compat,
>         .restart        = imx6q_restart,
>  MACHINE_END
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Robert Lee - April 23, 2012, 3:45 p.m.
>> Let me try last time.  What about having a late_initcall hook in
>> machine_desc?
>
> Also fine with me.
>

Shall I add Shawn's patch to my imx cpuidle patchset or should the
arch/arm/kernel/setup.c and arch.h changes be submitted separately?
If separately, Shawn, did you want to submit this patch or should I?

Thanks,
Rob

>>
>> Regards,
>> Shawn
>>
>> 8<---
>>
>> diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
>> index d7692ca..0b1c94b 100644
>> --- a/arch/arm/include/asm/mach/arch.h
>> +++ b/arch/arm/include/asm/mach/arch.h
>> @@ -43,6 +43,7 @@ struct machine_desc {
>>         void                    (*init_irq)(void);
>>         struct sys_timer        *timer;         /* system tick timer    */
>>         void                    (*init_machine)(void);
>> +       void                    (*init_late)(void);
>>  #ifdef CONFIG_MULTI_IRQ_HANDLER
>>         void                    (*handle_irq)(struct pt_regs *);
>>  #endif
>> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
>> index ebfac78..549f036 100644
>> --- a/arch/arm/kernel/setup.c
>> +++ b/arch/arm/kernel/setup.c
>> @@ -800,6 +800,14 @@ static int __init customize_machine(void)
>>  }
>>  arch_initcall(customize_machine);
>>
>> +static int __init init_machine_late(void)
>> +{
>> +       if (machine_desc->init_late)
>> +               machine_desc->init_late();
>> +       return 0;
>> +}
>> +late_initcall(init_machine_late);
>> +
>>  #ifdef CONFIG_KEXEC
>>  static inline unsigned long long get_total_mem(void)
>>  {
>> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
>> index da6c1d9..0e3640f 100644
>> --- a/arch/arm/mach-imx/mach-imx6q.c
>> +++ b/arch/arm/mach-imx/mach-imx6q.c
>> @@ -142,6 +142,7 @@ DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad (Device Tree)")
>>         .handle_irq     = imx6q_handle_irq,
>>         .timer          = &imx6q_timer,
>>         .init_machine   = imx6q_init_machine,
>> +       .init_late      = imx6q_init_late,
>>         .dt_compat      = imx6q_dt_compat,
>>         .restart        = imx6q_restart,
>>  MACHINE_END
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
Shawn Guo - April 24, 2012, 1:38 a.m.
On Mon, Apr 23, 2012 at 10:45:02AM -0500, Rob Lee wrote:
> >> Let me try last time.  What about having a late_initcall hook in
> >> machine_desc?
> >
> > Also fine with me.
> >
> 
> Shall I add Shawn's patch to my imx cpuidle patchset or should the
> arch/arm/kernel/setup.c and arch.h changes be submitted separately?
> If separately, Shawn, did you want to submit this patch or should I?
> 
Strange.  Russell is not in the Cc list.  I remember I added Russell
into Cc when I propose the idea.  Added him again.

Rob,

I suggest you have changes on arch/arm/kernel/setup.c and arch.h be
a separate patch, but you can still have it in the series to show why
we need the changes.  Cc Russell when posting the series, and see if
Russell is fine with the patch.  If he is, we can ask his preference
how the patch should go in, submitting it to his patch tracker or we
can have it go though arm-soc along with the series to save the
dependency.

Regards,
Shawn

> >> 8<---
> >>
> >> diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
> >> index d7692ca..0b1c94b 100644
> >> --- a/arch/arm/include/asm/mach/arch.h
> >> +++ b/arch/arm/include/asm/mach/arch.h
> >> @@ -43,6 +43,7 @@ struct machine_desc {
> >>         void                    (*init_irq)(void);
> >>         struct sys_timer        *timer;         /* system tick timer    */
> >>         void                    (*init_machine)(void);
> >> +       void                    (*init_late)(void);
> >>  #ifdef CONFIG_MULTI_IRQ_HANDLER
> >>         void                    (*handle_irq)(struct pt_regs *);
> >>  #endif
> >> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> >> index ebfac78..549f036 100644
> >> --- a/arch/arm/kernel/setup.c
> >> +++ b/arch/arm/kernel/setup.c
> >> @@ -800,6 +800,14 @@ static int __init customize_machine(void)
> >>  }
> >>  arch_initcall(customize_machine);
> >>
> >> +static int __init init_machine_late(void)
> >> +{
> >> +       if (machine_desc->init_late)
> >> +               machine_desc->init_late();
> >> +       return 0;
> >> +}
> >> +late_initcall(init_machine_late);
> >> +
> >>  #ifdef CONFIG_KEXEC
> >>  static inline unsigned long long get_total_mem(void)
> >>  {
> >> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> >> index da6c1d9..0e3640f 100644
> >> --- a/arch/arm/mach-imx/mach-imx6q.c
> >> +++ b/arch/arm/mach-imx/mach-imx6q.c
> >> @@ -142,6 +142,7 @@ DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad (Device Tree)")
> >>         .handle_irq     = imx6q_handle_irq,
> >>         .timer          = &imx6q_timer,
> >>         .init_machine   = imx6q_init_machine,
> >> +       .init_late      = imx6q_init_late,
> >>         .dt_compat      = imx6q_dt_compat,
> >>         .restart        = imx6q_restart,
> >>  MACHINE_END
Russell King - ARM Linux - April 24, 2012, 7:54 a.m.
On Tue, Apr 24, 2012 at 09:38:43AM +0800, Shawn Guo wrote:
> On Mon, Apr 23, 2012 at 10:45:02AM -0500, Rob Lee wrote:
> > >> Let me try last time.  What about having a late_initcall hook in
> > >> machine_desc?
> > >
> > > Also fine with me.
> > >
> > 
> > Shall I add Shawn's patch to my imx cpuidle patchset or should the
> > arch/arm/kernel/setup.c and arch.h changes be submitted separately?
> > If separately, Shawn, did you want to submit this patch or should I?
> > 
> Strange.  Russell is not in the Cc list.  I remember I added Russell
> into Cc when I propose the idea.  Added him again.

I didn't see any message in this thread cc'd to me, but that's not to
say I hadn't already read this patch.  I don't have any comment against
it, but I do wonder how often this hook would be used.

We do seem to have quite a number of late_initcall()s in arch/arm/mach-*,
so it seems to be a good idea - provided someone's willing to convert all
those users of late_initcall()s.
Shawn Guo - April 24, 2012, 8:36 a.m.
On Tue, Apr 24, 2012 at 08:54:26AM +0100, Russell King - ARM Linux wrote:
> On Tue, Apr 24, 2012 at 09:38:43AM +0800, Shawn Guo wrote:
> > On Mon, Apr 23, 2012 at 10:45:02AM -0500, Rob Lee wrote:
> > > >> Let me try last time.  What about having a late_initcall hook in
> > > >> machine_desc?
> > > >
> > > > Also fine with me.
> > > >
> > > 
> > > Shall I add Shawn's patch to my imx cpuidle patchset or should the
> > > arch/arm/kernel/setup.c and arch.h changes be submitted separately?
> > > If separately, Shawn, did you want to submit this patch or should I?
> > > 
> > Strange.  Russell is not in the Cc list.  I remember I added Russell
> > into Cc when I propose the idea.  Added him again.
> 
> I didn't see any message in this thread cc'd to me, but that's not to
> say I hadn't already read this patch.  I don't have any comment against
> it, but I do wonder how often this hook would be used.
> 
I guess mach-* that use common cpuidle will likely need this hook.

> We do seem to have quite a number of late_initcall()s in arch/arm/mach-*,
> so it seems to be a good idea - provided someone's willing to convert all
> those users of late_initcall()s.

Agreed.  The late_initcall()s in arch/arm/mach-* will not scale for
long time, since we are moving toward single build.
Robert Lee - April 24, 2012, 3:40 p.m.
On Tue, Apr 24, 2012 at 3:36 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Tue, Apr 24, 2012 at 08:54:26AM +0100, Russell King - ARM Linux wrote:
>> On Tue, Apr 24, 2012 at 09:38:43AM +0800, Shawn Guo wrote:
>> > On Mon, Apr 23, 2012 at 10:45:02AM -0500, Rob Lee wrote:
>> > > >> Let me try last time.  What about having a late_initcall hook in
>> > > >> machine_desc?
>> > > >
>> > > > Also fine with me.
>> > > >
>> > >
>> > > Shall I add Shawn's patch to my imx cpuidle patchset or should the
>> > > arch/arm/kernel/setup.c and arch.h changes be submitted separately?
>> > > If separately, Shawn, did you want to submit this patch or should I?
>> > >
>> > Strange.  Russell is not in the Cc list.  I remember I added Russell
>> > into Cc when I propose the idea.  Added him again.
>>
>> I didn't see any message in this thread cc'd to me, but that's not to
>> say I hadn't already read this patch.  I don't have any comment against
>> it, but I do wonder how often this hook would be used.
>>
> I guess mach-* that use common cpuidle will likely need this hook.
>
>> We do seem to have quite a number of late_initcall()s in arch/arm/mach-*,
>> so it seems to be a good idea - provided someone's willing to convert all
>> those users of late_initcall()s.
>
> Agreed.  The late_initcall()s in arch/arm/mach-* will not scale for
> long time, since we are moving toward single build.
>

Thanks for the attention on this.  From what I've understood, I will
send another submission that includes the imx cpuidle patchset and
Shawn's device tree late initcall patchset and I'll provide
explanation of the two separate patchsets in the cover sheet.

> --
> Regards,
> Shawn
Russell King - ARM Linux - April 24, 2012, 7:51 p.m.
On Tue, Apr 24, 2012 at 10:40:35AM -0500, Rob Lee wrote:
> Thanks for the attention on this.  From what I've understood, I will
> send another submission that includes the imx cpuidle patchset and
> Shawn's device tree late initcall patchset and I'll provide
> explanation of the two separate patchsets in the cover sheet.

What I also want to see _along with_ the addition of the init_late
callback is a patch set from the _same_ people fixing up the rest
of arch/arm/mach-* to use it - you know, like what I do when I
introduce these kinds of things.

Otherwise I'm going to NAK the change.  We can't have new stuff
introduced in this way and the older platforms ignored.  Forward
progress across the board please.
Shawn Guo - April 25, 2012, 2:06 a.m.
On 25 April 2012 03:51, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Tue, Apr 24, 2012 at 10:40:35AM -0500, Rob Lee wrote:
>> Thanks for the attention on this.  From what I've understood, I will
>> send another submission that includes the imx cpuidle patchset and
>> Shawn's device tree late initcall patchset and I'll provide
>> explanation of the two separate patchsets in the cover sheet.
>
> What I also want to see _along with_ the addition of the init_late
> callback is a patch set from the _same_ people fixing up the rest
> of arch/arm/mach-* to use it - you know, like what I do when I
> introduce these kinds of things.
>
> Otherwise I'm going to NAK the change.  We can't have new stuff
> introduced in this way and the older platforms ignored.  Forward
> progress across the board please.

I'm working on a series to clean them up.

Regards,
Shawn

Patch

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index d7692ca..0b1c94b 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -43,6 +43,7 @@  struct machine_desc {
        void                    (*init_irq)(void);
        struct sys_timer        *timer;         /* system tick timer    */
        void                    (*init_machine)(void);
+       void                    (*init_late)(void);
 #ifdef CONFIG_MULTI_IRQ_HANDLER
        void                    (*handle_irq)(struct pt_regs *);
 #endif
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index ebfac78..549f036 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -800,6 +800,14 @@  static int __init customize_machine(void)
 }
 arch_initcall(customize_machine);

+static int __init init_machine_late(void)
+{
+       if (machine_desc->init_late)
+               machine_desc->init_late();
+       return 0;
+}
+late_initcall(init_machine_late);
+
 #ifdef CONFIG_KEXEC
 static inline unsigned long long get_total_mem(void)
 {
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index da6c1d9..0e3640f 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -142,6 +142,7 @@  DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad (Device Tree)")
        .handle_irq     = imx6q_handle_irq,
        .timer          = &imx6q_timer,
        .init_machine   = imx6q_init_machine,
+       .init_late      = imx6q_init_late,
        .dt_compat      = imx6q_dt_compat,
        .restart        = imx6q_restart,
 MACHINE_END