| Message ID | 1366910944-3033663-4-git-send-email-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 25, 2013 at 07:28:46PM +0200, Arnd Bergmann wrote: > The irqchip_init function is only available when building > with CONFIG_OF enabled, which causes this build failure for > bonito_defconfig: > > arch/arm/mach-shmobile/built-in.o: In function `r8a7740_init_irq_of': > :(.init.text+0x580): undefined reference to `irqchip_init' > > This makes both the OF and the ATAGS portion of the driver > conditional, which avoids the build error and also results > in smaller object code if not both are enabled, without the > need for an #ifdef. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Cc: Bastian Hecht <hechtb+renesas@gmail.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > arch/arm/mach-shmobile/intc-r8a7740.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-shmobile/intc-r8a7740.c b/arch/arm/mach-shmobile/intc-r8a7740.c > index 8871f77..5dc57f1 100644 > --- a/arch/arm/mach-shmobile/intc-r8a7740.c > +++ b/arch/arm/mach-shmobile/intc-r8a7740.c > @@ -53,14 +53,23 @@ static void __init r8a7740_init_irq_common(void) > > void __init r8a7740_init_irq_of(void) > { > + if (!IS_ENABLED(CONFIG_OF)) > + return; > + In other parts of the shmobile I believe that such code is guarded by #ifdef CONFIG_OF and I believe not guarding this code in some way was an oversight. The above change seems fine to me. > irqchip_init(); > r8a7740_init_irq_common(); > } > > void __init r8a7740_init_irq(void) > { > - void __iomem *gic_dist_base = ioremap_nocache(0xc2800000, 0x1000); > - void __iomem *gic_cpu_base = ioremap_nocache(0xc2000000, 0x1000); > + void __iomem *gic_dist_base; > + void __iomem *gic_cpu_base; > + > + if (!IS_ENABLED(CONFIG_ATAGS)) > + return; > + > + gic_dist_base = ioremap_nocache(0xc2800000, 0x1000); > + gic_cpu_base = ioremap_nocache(0xc2000000, 0x1000); > > /* initialize the Generic Interrupt Controller PL390 r0p0 */ > gic_init(0, 29, gic_dist_base, gic_cpu_base); This one seems broken as the armadillo800eva board currently uses it to initialise GIC even if CONFIG_ATAGS is not defined. I did test the above change on the armadillo800eva board with the above change and CONFIG_ATAGS disabled, the result was a boot failure. With the change reverted booting seems fine.
Hello Simon and Arnd, 2013/4/26 Simon Horman <horms@verge.net.au>: > On Thu, Apr 25, 2013 at 07:28:46PM +0200, Arnd Bergmann wrote: >> The irqchip_init function is only available when building >> with CONFIG_OF enabled, which causes this build failure for >> bonito_defconfig: >> >> arch/arm/mach-shmobile/built-in.o: In function `r8a7740_init_irq_of': >> :(.init.text+0x580): undefined reference to `irqchip_init' >> >> This makes both the OF and the ATAGS portion of the driver >> conditional, which avoids the build error and also results >> in smaller object code if not both are enabled, without the >> need for an #ifdef. >> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> Cc: Bastian Hecht <hechtb+renesas@gmail.com> >> Cc: Simon Horman <horms+renesas@verge.net.au> >> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >> --- >> arch/arm/mach-shmobile/intc-r8a7740.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/mach-shmobile/intc-r8a7740.c b/arch/arm/mach-shmobile/intc-r8a7740.c >> index 8871f77..5dc57f1 100644 >> --- a/arch/arm/mach-shmobile/intc-r8a7740.c >> +++ b/arch/arm/mach-shmobile/intc-r8a7740.c >> @@ -53,14 +53,23 @@ static void __init r8a7740_init_irq_common(void) >> >> void __init r8a7740_init_irq_of(void) >> { >> + if (!IS_ENABLED(CONFIG_OF)) >> + return; >> + > > In other parts of the shmobile I believe that such code is > guarded by #ifdef CONFIG_OF and I believe not guarding this code in > some way was an oversight. Yes, sorry, I haven't thought about the dependency of irqchip_init and CONFIG_OF. > The above change seems fine to me. > >> irqchip_init(); >> r8a7740_init_irq_common(); >> } >> >> void __init r8a7740_init_irq(void) >> { >> - void __iomem *gic_dist_base = ioremap_nocache(0xc2800000, 0x1000); >> - void __iomem *gic_cpu_base = ioremap_nocache(0xc2000000, 0x1000); >> + void __iomem *gic_dist_base; >> + void __iomem *gic_cpu_base; >> + >> + if (!IS_ENABLED(CONFIG_ATAGS)) >> + return; >> + >> + gic_dist_base = ioremap_nocache(0xc2800000, 0x1000); >> + gic_cpu_base = ioremap_nocache(0xc2000000, 0x1000); >> >> /* initialize the Generic Interrupt Controller PL390 r0p0 */ >> gic_init(0, 29, gic_dist_base, gic_cpu_base); > > This one seems broken as the armadillo800eva board currently uses > it to initialise GIC even if CONFIG_ATAGS is not defined. > > I did test the above change on the armadillo800eva board > with the above change and CONFIG_ATAGS disabled, the result was > a boot failure. With the change reverted booting seems fine. CONFIG_ATAGS and CONFIG_ARM_APPENDED_DTB are two ways to pass the device tree information, right? So we run into trouble when we use the later but disable the first. Is there a reason why you wanted to avoid using two times just "if (!IS_ENABLED(CONFIG_OF))", Arnd? Thanks, Bastian
On Friday 26 April 2013, Bastian Hecht wrote: > > This one seems broken as the armadillo800eva board currently uses > > it to initialise GIC even if CONFIG_ATAGS is not defined. > > > > I did test the above change on the armadillo800eva board > > with the above change and CONFIG_ATAGS disabled, the result was > > a boot failure. With the change reverted booting seems fine. > > CONFIG_ATAGS and CONFIG_ARM_APPENDED_DTB are two ways to pass the > device tree information, right? No, the two are methods alternative methods for booting from a legacy boot loader. CONFIG_ATAGS is the old boot method without a DT, using a board file and a MACHINE_START() macro. ARM_APPENDED_DTB means you have a DT linked into the kernel but pass ATAGS to the kernel which uses the passed data to modify the DT. > So we run into trouble when we use the later but disable the first. > Is there a reason why you wanted to avoid using two times just "if > (!IS_ENABLED(CONFIG_OF))", Arnd? I incorrectly assumed that the second one was only used without DT, since it uses hardcoded MMIO locations rather than using of_iomap(). Arnd
On Friday 26 April 2013, Simon Horman wrote: > > diff --git a/arch/arm/mach-shmobile/intc-r8a7740.c b/arch/arm/mach-shmobile/intc-r8a7740.c > > index 8871f77..5dc57f1 100644 > > --- a/arch/arm/mach-shmobile/intc-r8a7740.c > > +++ b/arch/arm/mach-shmobile/intc-r8a7740.c > > @@ -53,14 +53,23 @@ static void __init r8a7740_init_irq_common(void) > > > > void __init r8a7740_init_irq_of(void) > > { > > + if (!IS_ENABLED(CONFIG_OF)) > > + return; > > + > > In other parts of the shmobile I believe that such code is > guarded by #ifdef CONFIG_OF and I believe not guarding this code in > some way was an oversight. > > The above change seems fine to me. Ok. The change that broke the code is only present in linux-next from one of your trees, but is not in arm-soc. Could you include the change in your tree, either by folding into one of your patches or adapting my patch appropriately? > > + > > + gic_dist_base = ioremap_nocache(0xc2800000, 0x1000); > > + gic_cpu_base = ioremap_nocache(0xc2000000, 0x1000); > > > > /* initialize the Generic Interrupt Controller PL390 r0p0 */ > > gic_init(0, 29, gic_dist_base, gic_cpu_base); > > This one seems broken as the armadillo800eva board currently uses > it to initialise GIC even if CONFIG_ATAGS is not defined. > > I did test the above change on the armadillo800eva board > with the above change and CONFIG_ATAGS disabled, the result was > a boot failure. With the change reverted booting seems fine. Yes, I see my mistake now. The second change was clearly wrong as this function is also used for DT boards, not for ATAGS boards. Arnd
On Mon, Apr 29, 2013 at 04:49:07PM +0200, Arnd Bergmann wrote: > On Friday 26 April 2013, Simon Horman wrote: > > > diff --git a/arch/arm/mach-shmobile/intc-r8a7740.c b/arch/arm/mach-shmobile/intc-r8a7740.c > > > index 8871f77..5dc57f1 100644 > > > --- a/arch/arm/mach-shmobile/intc-r8a7740.c > > > +++ b/arch/arm/mach-shmobile/intc-r8a7740.c > > > @@ -53,14 +53,23 @@ static void __init r8a7740_init_irq_common(void) > > > > > > void __init r8a7740_init_irq_of(void) > > > { > > > + if (!IS_ENABLED(CONFIG_OF)) > > > + return; > > > + > > > > In other parts of the shmobile I believe that such code is > > guarded by #ifdef CONFIG_OF and I believe not guarding this code in > > some way was an oversight. > > > > The above change seems fine to me. > > Ok. The change that broke the code is only present in linux-next from > one of your trees, but is not in arm-soc. Could you include the change > in your tree, either by folding into one of your patches or adapting > my patch appropriately? Yes, or of course. I'll queue-up your patch with the second hunk (below) removed. > > > + > > > + gic_dist_base = ioremap_nocache(0xc2800000, 0x1000); > > > + gic_cpu_base = ioremap_nocache(0xc2000000, 0x1000); > > > > > > /* initialize the Generic Interrupt Controller PL390 r0p0 */ > > > gic_init(0, 29, gic_dist_base, gic_cpu_base); > > > > This one seems broken as the armadillo800eva board currently uses > > it to initialise GIC even if CONFIG_ATAGS is not defined. > > > > I did test the above change on the armadillo800eva board > > with the above change and CONFIG_ATAGS disabled, the result was > > a boot failure. With the change reverted booting seems fine. > > Yes, I see my mistake now. The second change was clearly wrong > as this function is also used for DT boards, not for ATAGS boards. > > Arnd >
On Thu, Apr 25, 2013 at 12:28 PM, Arnd Bergmann <arnd@arndb.de> wrote: > The irqchip_init function is only available when building > with CONFIG_OF enabled, which causes this build failure for > bonito_defconfig: > > arch/arm/mach-shmobile/built-in.o: In function `r8a7740_init_irq_of': > :(.init.text+0x580): undefined reference to `irqchip_init' > > This makes both the OF and the ATAGS portion of the driver > conditional, which avoids the build error and also results > in smaller object code if not both are enabled, without the > need for an #ifdef. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Cc: Bastian Hecht <hechtb+renesas@gmail.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > arch/arm/mach-shmobile/intc-r8a7740.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-shmobile/intc-r8a7740.c b/arch/arm/mach-shmobile/intc-r8a7740.c > index 8871f77..5dc57f1 100644 > --- a/arch/arm/mach-shmobile/intc-r8a7740.c > +++ b/arch/arm/mach-shmobile/intc-r8a7740.c > @@ -53,14 +53,23 @@ static void __init r8a7740_init_irq_common(void) > > void __init r8a7740_init_irq_of(void) > { > + if (!IS_ENABLED(CONFIG_OF)) > + return; > + > irqchip_init(); Why not have an empty irqchip_init? I'd guess we'll need this on other platforms and your default mach. Rob
On Wed, May 01, 2013 at 10:54:30AM -0500, Rob Herring wrote: > On Thu, Apr 25, 2013 at 12:28 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > The irqchip_init function is only available when building > > with CONFIG_OF enabled, which causes this build failure for > > bonito_defconfig: > > > > arch/arm/mach-shmobile/built-in.o: In function `r8a7740_init_irq_of': > > :(.init.text+0x580): undefined reference to `irqchip_init' > > > > This makes both the OF and the ATAGS portion of the driver > > conditional, which avoids the build error and also results > > in smaller object code if not both are enabled, without the > > need for an #ifdef. > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Cc: Bastian Hecht <hechtb+renesas@gmail.com> > > Cc: Simon Horman <horms+renesas@verge.net.au> > > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > --- > > arch/arm/mach-shmobile/intc-r8a7740.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/mach-shmobile/intc-r8a7740.c b/arch/arm/mach-shmobile/intc-r8a7740.c > > index 8871f77..5dc57f1 100644 > > --- a/arch/arm/mach-shmobile/intc-r8a7740.c > > +++ b/arch/arm/mach-shmobile/intc-r8a7740.c > > @@ -53,14 +53,23 @@ static void __init r8a7740_init_irq_common(void) > > > > void __init r8a7740_init_irq_of(void) > > { > > + if (!IS_ENABLED(CONFIG_OF)) > > + return; > > + > > irqchip_init(); > > Why not have an empty irqchip_init? I'd guess we'll need this on other > platforms and your default mach. Thanks, I think that could work. I will see about making it so.
On Thursday 02 May 2013, Simon Horman wrote: > > > diff --git a/arch/arm/mach-shmobile/intc-r8a7740.c b/arch/arm/mach-shmobile/intc-r8a7740.c > > > index 8871f77..5dc57f1 100644 > > > --- a/arch/arm/mach-shmobile/intc-r8a7740.c > > > +++ b/arch/arm/mach-shmobile/intc-r8a7740.c > > > @@ -53,14 +53,23 @@ static void __init r8a7740_init_irq_common(void) > > > > > > void __init r8a7740_init_irq_of(void) > > > { > > > + if (!IS_ENABLED(CONFIG_OF)) > > > + return; > > > + > > > irqchip_init(); > > > > Why not have an empty irqchip_init? I'd guess we'll need this on other > > platforms and your default mach. > > Thanks, I think that could work. > > I will see about making it so. Ping Linux-next is still broken for me. There is also anothe shmobile build bug, I'll send a separate patch for that, which also needs to go into your tree. Arnd
Hi, 2013/5/8 Arnd Bergmann <arnd@arndb.de>: > On Thursday 02 May 2013, Simon Horman wrote: >> > > diff --git a/arch/arm/mach-shmobile/intc-r8a7740.c b/arch/arm/mach-shmobile/intc-r8a7740.c >> > > index 8871f77..5dc57f1 100644 >> > > --- a/arch/arm/mach-shmobile/intc-r8a7740.c >> > > +++ b/arch/arm/mach-shmobile/intc-r8a7740.c >> > > @@ -53,14 +53,23 @@ static void __init r8a7740_init_irq_common(void) >> > > >> > > void __init r8a7740_init_irq_of(void) >> > > { >> > > + if (!IS_ENABLED(CONFIG_OF)) >> > > + return; >> > > + >> > > irqchip_init(); >> > >> > Why not have an empty irqchip_init? I'd guess we'll need this on other >> > platforms and your default mach. >> >> Thanks, I think that could work. >> >> I will see about making it so. > > Ping I have sent out a patch adding an empty irqchip_init() when CONFIG_IRQCHIP is not set with the subject [PATCH] irqchip: Add irqchip_init dummy function > Linux-next is still broken for me. There is also anothe shmobile build bug, > I'll send a separate patch for that, which also needs to go into your tree. Oh thanks Arnd for this fix! Cheers, Bastian
On Wednesday 08 May 2013, Bastian Hecht wrote: > > 2013/5/8 Arnd Bergmann <arnd@arndb.de>: > > I have sent out a patch adding an empty irqchip_init() when > CONFIG_IRQCHIP is not set with the subject > [PATCH] irqchip: Add irqchip_init dummy function Ah, I missed that one. Is it already in linux-next? Arnd
On Wed, May 08, 2013 at 03:22:22PM +0200, Bastian Hecht wrote: > Hi, > > 2013/5/8 Arnd Bergmann <arnd@arndb.de>: > > On Thursday 02 May 2013, Simon Horman wrote: > >> > > diff --git a/arch/arm/mach-shmobile/intc-r8a7740.c b/arch/arm/mach-shmobile/intc-r8a7740.c > >> > > index 8871f77..5dc57f1 100644 > >> > > --- a/arch/arm/mach-shmobile/intc-r8a7740.c > >> > > +++ b/arch/arm/mach-shmobile/intc-r8a7740.c > >> > > @@ -53,14 +53,23 @@ static void __init r8a7740_init_irq_common(void) > >> > > > >> > > void __init r8a7740_init_irq_of(void) > >> > > { > >> > > + if (!IS_ENABLED(CONFIG_OF)) > >> > > + return; > >> > > + > >> > > irqchip_init(); > >> > > >> > Why not have an empty irqchip_init? I'd guess we'll need this on other > >> > platforms and your default mach. > >> > >> Thanks, I think that could work. > >> > >> I will see about making it so. > > > > Ping > > I have sent out a patch adding an empty irqchip_init() when > CONFIG_IRQCHIP is not set with the subject > [PATCH] irqchip: Add irqchip_init dummy function Thanks. > > Linux-next is still broken for me. There is also anothe shmobile build bug, > > I'll send a separate patch for that, which also needs to go into your tree. Sorry about that. I though I had pushed your fix (the first hunk of the patch that started this thread), but it seems that I did not. > Oh thanks Arnd for this fix! > > Cheers, > > Bastian
On Thu, May 09, 2013 at 12:38:19PM +0900, Simon Horman wrote: > On Wed, May 08, 2013 at 03:22:22PM +0200, Bastian Hecht wrote: > > Hi, > > > > 2013/5/8 Arnd Bergmann <arnd@arndb.de>: > > > On Thursday 02 May 2013, Simon Horman wrote: > > >> > > diff --git a/arch/arm/mach-shmobile/intc-r8a7740.c b/arch/arm/mach-shmobile/intc-r8a7740.c > > >> > > index 8871f77..5dc57f1 100644 > > >> > > --- a/arch/arm/mach-shmobile/intc-r8a7740.c > > >> > > +++ b/arch/arm/mach-shmobile/intc-r8a7740.c > > >> > > @@ -53,14 +53,23 @@ static void __init r8a7740_init_irq_common(void) > > >> > > > > >> > > void __init r8a7740_init_irq_of(void) > > >> > > { > > >> > > + if (!IS_ENABLED(CONFIG_OF)) > > >> > > + return; > > >> > > + > > >> > > irqchip_init(); > > >> > > > >> > Why not have an empty irqchip_init? I'd guess we'll need this on other > > >> > platforms and your default mach. > > >> > > >> Thanks, I think that could work. > > >> > > >> I will see about making it so. > > > > > > Ping > > > > I have sent out a patch adding an empty irqchip_init() when > > CONFIG_IRQCHIP is not set with the subject > > [PATCH] irqchip: Add irqchip_init dummy function > > Thanks. > > > > Linux-next is still broken for me. There is also anothe shmobile build bug, > > > I'll send a separate patch for that, which also needs to go into your tree. > > Sorry about that. I though I had pushed your fix (the first hunk of the > patch that started this thread), but it seems that I did not. Sorry once again for the delay in addressing this. I have pushed a refreshed next branch which includes both Bastian's fix for irqchip_init() and the patch that Arnd posted relating to symbol renames. These changes should appear in linux-next in the not to distant future.
diff --git a/arch/arm/mach-shmobile/intc-r8a7740.c b/arch/arm/mach-shmobile/intc-r8a7740.c index 8871f77..5dc57f1 100644 --- a/arch/arm/mach-shmobile/intc-r8a7740.c +++ b/arch/arm/mach-shmobile/intc-r8a7740.c @@ -53,14 +53,23 @@ static void __init r8a7740_init_irq_common(void) void __init r8a7740_init_irq_of(void) { + if (!IS_ENABLED(CONFIG_OF)) + return; + irqchip_init(); r8a7740_init_irq_common(); } void __init r8a7740_init_irq(void) { - void __iomem *gic_dist_base = ioremap_nocache(0xc2800000, 0x1000); - void __iomem *gic_cpu_base = ioremap_nocache(0xc2000000, 0x1000); + void __iomem *gic_dist_base; + void __iomem *gic_cpu_base; + + if (!IS_ENABLED(CONFIG_ATAGS)) + return; + + gic_dist_base = ioremap_nocache(0xc2800000, 0x1000); + gic_cpu_base = ioremap_nocache(0xc2000000, 0x1000); /* initialize the Generic Interrupt Controller PL390 r0p0 */ gic_init(0, 29, gic_dist_base, gic_cpu_base);
The irqchip_init function is only available when building with CONFIG_OF enabled, which causes this build failure for bonito_defconfig: arch/arm/mach-shmobile/built-in.o: In function `r8a7740_init_irq_of': :(.init.text+0x580): undefined reference to `irqchip_init' This makes both the OF and the ATAGS portion of the driver conditional, which avoids the build error and also results in smaller object code if not both are enabled, without the need for an #ifdef. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Cc: Bastian Hecht <hechtb+renesas@gmail.com> Cc: Simon Horman <horms+renesas@verge.net.au> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> --- arch/arm/mach-shmobile/intc-r8a7740.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)