| Message ID | 1371774924-9224-5-git-send-email-tomasz.figa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 21/06/13 01:35, Tomasz Figa wrote: Hi Tomasz, > Most ARM platforms have parts that should be initialized as early as > possible, which usually means as soon as memory management (kmalloc, > ioremap) starts to work, > > However, currently there is no appropriate callback in machine_desc > struct to use for such initialization and platforms tend to stuff things > up .init_irq() and .init_time() callbacks. > > Since all the DT-based platforms are going towards generic IRQ and time > initialization (using irqchip_init and clocksource_of_init) and current > code assumes that if custom callbacks are not provided in machine_desc > then generic ones should be used, this problem has become a bit more > inconvenient. > > This patch tries to solve this issue by introducing new callback called > .init_platform(), where any custom low level initialization of platform > can be done safely. > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> > --- > arch/arm/include/asm/mach/arch.h | 1 + > arch/arm/kernel/irq.c | 3 +++ > 2 files changed, 4 insertions(+) > > diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h > index 308ad7d..b2f4d11 100644 > --- a/arch/arm/include/asm/mach/arch.h > +++ b/arch/arm/include/asm/mach/arch.h > @@ -46,6 +46,7 @@ struct machine_desc { > void (*reserve)(void);/* reserve mem blocks */ > void (*map_io)(void);/* IO mapping function */ > void (*init_early)(void); > + void (*init_platform)(void); > void (*init_irq)(void); > void (*init_time)(void); > void (*init_machine)(void); > diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c > index 9723d17..61e2000 100644 > --- a/arch/arm/kernel/irq.c > +++ b/arch/arm/kernel/irq.c > @@ -115,6 +115,9 @@ EXPORT_SYMBOL_GPL(set_irq_flags); > > void __init init_IRQ(void) > { > + if (machine_desc->init_platform) > + machine_desc->init_platform(); > + > if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq) > irqchip_init(); > else To me, this new hook is strictly equivalent to init_irq. What do we gain exactly? I didn't think init_irq was going away... I know init_irq is not pretty, and we tend to overload it with other stuff, but I don't really see the point of adding a new callback that has the exact same properties. M.
Hi Marc, On Friday 21 of June 2013 11:24:52 Marc Zyngier wrote: > On 21/06/13 01:35, Tomasz Figa wrote: > > Hi Tomasz, > > > Most ARM platforms have parts that should be initialized as early as > > possible, which usually means as soon as memory management (kmalloc, > > ioremap) starts to work, > > > > However, currently there is no appropriate callback in machine_desc > > struct to use for such initialization and platforms tend to stuff > > things > > up .init_irq() and .init_time() callbacks. > > > > Since all the DT-based platforms are going towards generic IRQ and time > > initialization (using irqchip_init and clocksource_of_init) and current > > code assumes that if custom callbacks are not provided in machine_desc > > then generic ones should be used, this problem has become a bit more > > inconvenient. > > > > This patch tries to solve this issue by introducing new callback called > > .init_platform(), where any custom low level initialization of platform > > can be done safely. > > > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> > > --- > > > > arch/arm/include/asm/mach/arch.h | 1 + > > arch/arm/kernel/irq.c | 3 +++ > > 2 files changed, 4 insertions(+) > > > > diff --git a/arch/arm/include/asm/mach/arch.h > > b/arch/arm/include/asm/mach/arch.h index 308ad7d..b2f4d11 100644 > > --- a/arch/arm/include/asm/mach/arch.h > > +++ b/arch/arm/include/asm/mach/arch.h > > @@ -46,6 +46,7 @@ struct machine_desc { > > > > void (*reserve)(void);/* reserve mem blocks */ > > void (*map_io)(void);/* IO mapping function */ > > void (*init_early)(void); > > > > + void (*init_platform)(void); > > > > void (*init_irq)(void); > > void (*init_time)(void); > > void (*init_machine)(void); > > > > diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c > > index 9723d17..61e2000 100644 > > --- a/arch/arm/kernel/irq.c > > +++ b/arch/arm/kernel/irq.c > > @@ -115,6 +115,9 @@ EXPORT_SYMBOL_GPL(set_irq_flags); > > > > void __init init_IRQ(void) > > { > > > > + if (machine_desc->init_platform) > > + machine_desc->init_platform(); > > + > > > > if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq) > > > > irqchip_init(); > > > > else > > To me, this new hook is strictly equivalent to init_irq. What do we gain > exactly? I didn't think init_irq was going away... > > I know init_irq is not pretty, and we tend to overload it with other > stuff, but I don't really see the point of adding a new callback that > has the exact same properties. Well, it doesn't really give us any functional benefits. However in my opinion it looks much saner in case of DT-only platforms that don't need any specific IRQ initialization, but need to call some platform specific initialization routines, after memory management, but before anything else is initialized. This way irqchip_init() doesn't have to be explicitly called in platform code. Anyway, I don't have any strong opinion on this. If it is perfectly fine to abuse irqchip_init() for anything that must be done at this stage of boot, then I'm fine with this either and will modify the board file from further patch from this series to not rely on this change any more. Best regards, Tomasz > > M.
On Friday 21 June 2013, Tomasz Figa wrote: > > To me, this new hook is strictly equivalent to init_irq. What do we gain > > exactly? I didn't think init_irq was going away... > > > > I know init_irq is not pretty, and we tend to overload it with other > > stuff, but I don't really see the point of adding a new callback that > > has the exact same properties. > > Well, it doesn't really give us any functional benefits. > > However in my opinion it looks much saner in case of DT-only platforms that > don't need any specific IRQ initialization, but need to call some platform > specific initialization routines, after memory management, but before > anything else is initialized. > > This way irqchip_init() doesn't have to be explicitly called in platform > code. > > Anyway, I don't have any strong opinion on this. If it is perfectly fine to > abuse irqchip_init() for anything that must be done at this stage of boot, > then I'm fine with this either and will modify the board file from further > patch from this series to not rely on this change any more. Your init_platform only has these two calls in it: + of_clk_init(NULL); + samsung_wdt_reset_of_init(); Presumably you need of_clk_init() for the watchdog to work. But do you actually need to initialize the reset logic this early? Why not turn samsung_wdt_reset_of_init into a standalone driver, or call it from init_machine? I would actually like to call of_clk_init from common code at some point between init_irq and init_time, although I'm not sure if some platforms need it to be called before init_irq. Arnd
On Friday 21 of June 2013 16:12:15 Arnd Bergmann wrote: > On Friday 21 June 2013, Tomasz Figa wrote: > > > To me, this new hook is strictly equivalent to init_irq. What do we > > > gain exactly? I didn't think init_irq was going away... > > > > > > I know init_irq is not pretty, and we tend to overload it with other > > > stuff, but I don't really see the point of adding a new callback > > > that > > > has the exact same properties. > > > > Well, it doesn't really give us any functional benefits. > > > > However in my opinion it looks much saner in case of DT-only platforms > > that don't need any specific IRQ initialization, but need to call > > some platform specific initialization routines, after memory > > management, but before anything else is initialized. > > > > This way irqchip_init() doesn't have to be explicitly called in > > platform code. > > > > Anyway, I don't have any strong opinion on this. If it is perfectly > > fine to abuse irqchip_init() for anything that must be done at this > > stage of boot, then I'm fine with this either and will modify the > > board file from further patch from this series to not rely on this > > change any more. > > Your init_platform only has these two calls in it: > > + of_clk_init(NULL); > + samsung_wdt_reset_of_init(); > > Presumably you need of_clk_init() for the watchdog to work. Clock initialization is also required for timekeeping to work, so if we had to defer it, it must happen before (or inside) init_time(). Putting this platform aside, there might be other platforms which require clock initialization before IRQ initialization, e.g. to enable clock of an interrupt controller. > But do you > actually need to initialize the reset logic this early? Why not turn > samsung_wdt_reset_of_init into a standalone driver, or call it from > init_machine? This is debatable. One might want to have reset support working as early as possible to have panic timeout working, but I'm not sure if there is any point of rebooting the machine if the kernel fails so early. Personally I'd prefer this to be a separate driver, in drivers/power/reset/ or wherever appropriate, but I didn't want to change existing behavior too much, which was the reset working already after clock initialization. > I would actually like to call of_clk_init from common code at some point > between init_irq and init_time, although I'm not sure if some platforms > need it to be called before init_irq. Now as I think of it, some platforms might need it earlier, some later, so my init_platform(), which is not flexible at all, would be useless for some of them, that need such things done after init_irq(). I'm going to drop this patch from this series (and simply abuse one of existing callbacks instead), but we should think about a better solution for this issue. Best regards, Tomasz > Arnd > -- > 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
Hi Kukjin, On Friday 21 of June 2013 02:35:20 Tomasz Figa wrote: > Most ARM platforms have parts that should be initialized as early as > possible, which usually means as soon as memory management (kmalloc, > ioremap) starts to work, > > However, currently there is no appropriate callback in machine_desc > struct to use for such initialization and platforms tend to stuff things > up .init_irq() and .init_time() callbacks. > > Since all the DT-based platforms are going towards generic IRQ and time > initialization (using irqchip_init and clocksource_of_init) and current > code assumes that if custom callbacks are not provided in machine_desc > then generic ones should be used, this problem has become a bit more > inconvenient. > > This patch tries to solve this issue by introducing new callback called > .init_platform(), where any custom low level initialization of platform > can be done safely. > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> > --- > arch/arm/include/asm/mach/arch.h | 1 + > arch/arm/kernel/irq.c | 3 +++ > 2 files changed, 4 insertions(+) Please disregard this patch when applying the series (if that happens). Best regards, Tomasz
diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h index 308ad7d..b2f4d11 100644 --- a/arch/arm/include/asm/mach/arch.h +++ b/arch/arm/include/asm/mach/arch.h @@ -46,6 +46,7 @@ struct machine_desc { void (*reserve)(void);/* reserve mem blocks */ void (*map_io)(void);/* IO mapping function */ void (*init_early)(void); + void (*init_platform)(void); void (*init_irq)(void); void (*init_time)(void); void (*init_machine)(void); diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c index 9723d17..61e2000 100644 --- a/arch/arm/kernel/irq.c +++ b/arch/arm/kernel/irq.c @@ -115,6 +115,9 @@ EXPORT_SYMBOL_GPL(set_irq_flags); void __init init_IRQ(void) { + if (machine_desc->init_platform) + machine_desc->init_platform(); + if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq) irqchip_init(); else
Most ARM platforms have parts that should be initialized as early as possible, which usually means as soon as memory management (kmalloc, ioremap) starts to work, However, currently there is no appropriate callback in machine_desc struct to use for such initialization and platforms tend to stuff things up .init_irq() and .init_time() callbacks. Since all the DT-based platforms are going towards generic IRQ and time initialization (using irqchip_init and clocksource_of_init) and current code assumes that if custom callbacks are not provided in machine_desc then generic ones should be used, this problem has become a bit more inconvenient. This patch tries to solve this issue by introducing new callback called .init_platform(), where any custom low level initialization of platform can be done safely. Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> --- arch/arm/include/asm/mach/arch.h | 1 + arch/arm/kernel/irq.c | 3 +++ 2 files changed, 4 insertions(+)