| Message ID | 20120423071013.GS26306@S2101-09.ap.freescale.net |
|---|---|
| State | New |
| Headers | show |
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 >
>> 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 |
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
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.
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.
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
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.
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
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