| Message ID | 1395257399-359545-61-git-send-email-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 19, 2014 at 08:29:57PM +0100, Arnd Bergmann wrote: > When phylib is set to be built as a module, the lager and koelsch > boards fail to build: > > arch/arm/mach-shmobile/built-in.o: In function `lager_ksz8041_fixup': > :(.text+0x738): undefined reference to `mdiobus_read' > :(.text+0x73c): undefined reference to `mdiobus_write' > arch/arm/mach-shmobile/built-in.o: In function `koelsch_ksz8041_fixup': > :(.text+0x7e8): undefined reference to `mdiobus_read' > :(.text+0x7ec): undefined reference to `mdiobus_write' > > To work around that problem, this changes the code to check for > IS_BUILTIN rather than IS_ENABLED, turning the error into a runtime > problem. It's now possible to build random configurations, but the > phy may be set up incorrectly in this case. I wonder if Kconfig for koelsch should be tightened up somehow to ensure that PHYLIB is either unselected or builtin. Also, a minor nit, I would prefer changes for different boards in different patches. But I can split the patch myself if its not going to be changed otherwise. > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Cc: Simon Horman <horms@verge.net.au> > Cc: Magnus Damm <magnus.damm@gmail.com> > Cc: linux-sh@vger.kernel.org > --- > arch/arm/mach-shmobile/board-koelsch.c | 2 +- > arch/arm/mach-shmobile/board-lager.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-shmobile/board-koelsch.c b/arch/arm/mach-shmobile/board-koelsch.c > index 5a034ff..b724f33 100644 > --- a/arch/arm/mach-shmobile/board-koelsch.c > +++ b/arch/arm/mach-shmobile/board-koelsch.c > @@ -510,7 +510,7 @@ static void __init koelsch_init(void) > > irq_set_irq_type(irq_pin(0), IRQ_TYPE_LEVEL_LOW); > > - if (IS_ENABLED(CONFIG_PHYLIB)) > + if (IS_BUILTIN(CONFIG_PHYLIB)) > phy_register_fixup_for_id("r8a7791-ether-ff:01", > koelsch_ksz8041_fixup); > } > diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c > index f0104bf..67b1069 100644 > --- a/arch/arm/mach-shmobile/board-lager.c > +++ b/arch/arm/mach-shmobile/board-lager.c > @@ -869,7 +869,7 @@ static void __init lager_init(void) > > irq_set_irq_type(irq_pin(0), IRQ_TYPE_LEVEL_LOW); > > - if (IS_ENABLED(CONFIG_PHYLIB)) > + if (IS_BUILTIN(CONFIG_PHYLIB)) > phy_register_fixup_for_id("r8a7790-ether-ff:01", > lager_ksz8041_fixup); > } > -- > 1.8.3.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-sh" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Thursday 20 March 2014, Simon Horman wrote: > On Wed, Mar 19, 2014 at 08:29:57PM +0100, Arnd Bergmann wrote: > > When phylib is set to be built as a module, the lager and koelsch > > boards fail to build: > > > > arch/arm/mach-shmobile/built-in.o: In function `lager_ksz8041_fixup': > > :(.text+0x738): undefined reference to `mdiobus_read' > > :(.text+0x73c): undefined reference to `mdiobus_write' > > arch/arm/mach-shmobile/built-in.o: In function `koelsch_ksz8041_fixup': > > :(.text+0x7e8): undefined reference to `mdiobus_read' > > :(.text+0x7ec): undefined reference to `mdiobus_write' > > > > To work around that problem, this changes the code to check for > > IS_BUILTIN rather than IS_ENABLED, turning the error into a runtime > > problem. It's now possible to build random configurations, but the > > phy may be set up incorrectly in this case. > > I wonder if Kconfig for koelsch should be tightened up somehow to > ensure that PHYLIB is either unselected or builtin. > > Also, a minor nit, I would prefer changes for different boards > in different patches. But I can split the patch myself if its > not going to be changed otherwise. I would prefer to take the entire series directly into arm-soc this time, if you don't mind. Arnd
On Fri, Mar 21, 2014 at 04:43:24PM +0100, Arnd Bergmann wrote: > On Thursday 20 March 2014, Simon Horman wrote: > > On Wed, Mar 19, 2014 at 08:29:57PM +0100, Arnd Bergmann wrote: > > > When phylib is set to be built as a module, the lager and koelsch > > > boards fail to build: > > > > > > arch/arm/mach-shmobile/built-in.o: In function `lager_ksz8041_fixup': > > > :(.text+0x738): undefined reference to `mdiobus_read' > > > :(.text+0x73c): undefined reference to `mdiobus_write' > > > arch/arm/mach-shmobile/built-in.o: In function `koelsch_ksz8041_fixup': > > > :(.text+0x7e8): undefined reference to `mdiobus_read' > > > :(.text+0x7ec): undefined reference to `mdiobus_write' > > > > > > To work around that problem, this changes the code to check for > > > IS_BUILTIN rather than IS_ENABLED, turning the error into a runtime > > > problem. It's now possible to build random configurations, but the > > > phy may be set up incorrectly in this case. > > > > I wonder if Kconfig for koelsch should be tightened up somehow to > > ensure that PHYLIB is either unselected or builtin. > > > > Also, a minor nit, I would prefer changes for different boards > > in different patches. But I can split the patch myself if its > > not going to be changed otherwise. > > I would prefer to take the entire series directly into arm-soc > this time, if you don't mind. That I'm happy to go along with though I don't understand the motivation for it. And regardless of how the patches go in I'd prefer if they were split as I suggested above. What I'm not so happy about is us potentially moving a problem from being a compile time error, which is easy to observe, to a run-time behavioural problem, which may be much less obvious to the beholder.
On Monday 24 March 2014 10:35:56 Simon Horman wrote: > On Fri, Mar 21, 2014 at 04:43:24PM +0100, Arnd Bergmann wrote: > > On Thursday 20 March 2014, Simon Horman wrote: > > > I wonder if Kconfig for koelsch should be tightened up somehow to > > > ensure that PHYLIB is either unselected or builtin. > > > > > > Also, a minor nit, I would prefer changes for different boards > > > in different patches. But I can split the patch myself if its > > > not going to be changed otherwise. > > > > I would prefer to take the entire series directly into arm-soc > > this time, if you don't mind. > > That I'm happy to go along with though I don't understand the motivation > for it. And regardless of how the patches go in I'd prefer if they were > split as I suggested above. Sorry, I misunderstood you at first. I thought you meant you only split them up when you apply them yourself. It's no problem for me to split up this patch as well and then apply it here. > What I'm not so happy about is us potentially moving a problem from being a > compile time error, which is easy to observe, to a run-time behavioural > problem, which may be much less obvious to the beholder. I agree that it's not nice, but I couldn't come with a better approach, and we are doing the same thing on other platforms as well. The problems are: * leaving the code as it is today prevents me from running all 'make randconfig' kernels successfully. I use this as an important test case for verifying stuff we pull into the arm-soc tree. At the moment, I have a 160-patch series that I want to get merged upstream over time. * Using 'select PHYLIB' from the platform is nasty because we want to be able to turn off whole subsystems (BLOCK, NET, I2C, ...) in Kconfig independent of the board selection. PHYLIB requires the network code. * Using 'depends on' to disable the board option if PHYLIB is a module is counterintuitive. * Makeing PHYLIB always built-in when NETDEVICE is would be a bit wasteful. I don't have any other ideas for solving this. Arnd
On Mon, Mar 24, 2014 at 01:04:57PM +0100, Arnd Bergmann wrote: > On Monday 24 March 2014 10:35:56 Simon Horman wrote: > > On Fri, Mar 21, 2014 at 04:43:24PM +0100, Arnd Bergmann wrote: > > > On Thursday 20 March 2014, Simon Horman wrote: > > > > I wonder if Kconfig for koelsch should be tightened up somehow to > > > > ensure that PHYLIB is either unselected or builtin. > > > > > > > > Also, a minor nit, I would prefer changes for different boards > > > > in different patches. But I can split the patch myself if its > > > > not going to be changed otherwise. > > > > > > I would prefer to take the entire series directly into arm-soc > > > this time, if you don't mind. > > > > That I'm happy to go along with though I don't understand the motivation > > for it. And regardless of how the patches go in I'd prefer if they were > > split as I suggested above. > > Sorry, I misunderstood you at first. I thought you meant you only split > them up when you apply them yourself. It's no problem for me to split > up this patch as well and then apply it here. > > > What I'm not so happy about is us potentially moving a problem from being a > > compile time error, which is easy to observe, to a run-time behavioural > > problem, which may be much less obvious to the beholder. > > I agree that it's not nice, but I couldn't come with a better approach, > and we are doing the same thing on other platforms as well. > > The problems are: > > * leaving the code as it is today prevents me from running all > 'make randconfig' kernels successfully. I use this as an important > test case for verifying stuff we pull into the arm-soc tree. > At the moment, I have a 160-patch series that I want to get merged > upstream over time. > > * Using 'select PHYLIB' from the platform is nasty because we want > to be able to turn off whole subsystems (BLOCK, NET, I2C, ...) > in Kconfig independent of the board selection. PHYLIB requires > the network code. > > * Using 'depends on' to disable the board option if PHYLIB is > a module is counterintuitive. > > * Makeing PHYLIB always built-in when NETDEVICE is would be a > bit wasteful. maybe let the board select PHYLIB if NETDEVICE is a nice compromise? Best regards Uwe
diff --git a/arch/arm/mach-shmobile/board-koelsch.c b/arch/arm/mach-shmobile/board-koelsch.c index 5a034ff..b724f33 100644 --- a/arch/arm/mach-shmobile/board-koelsch.c +++ b/arch/arm/mach-shmobile/board-koelsch.c @@ -510,7 +510,7 @@ static void __init koelsch_init(void) irq_set_irq_type(irq_pin(0), IRQ_TYPE_LEVEL_LOW); - if (IS_ENABLED(CONFIG_PHYLIB)) + if (IS_BUILTIN(CONFIG_PHYLIB)) phy_register_fixup_for_id("r8a7791-ether-ff:01", koelsch_ksz8041_fixup); } diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c index f0104bf..67b1069 100644 --- a/arch/arm/mach-shmobile/board-lager.c +++ b/arch/arm/mach-shmobile/board-lager.c @@ -869,7 +869,7 @@ static void __init lager_init(void) irq_set_irq_type(irq_pin(0), IRQ_TYPE_LEVEL_LOW); - if (IS_ENABLED(CONFIG_PHYLIB)) + if (IS_BUILTIN(CONFIG_PHYLIB)) phy_register_fixup_for_id("r8a7790-ether-ff:01", lager_ksz8041_fixup); }
When phylib is set to be built as a module, the lager and koelsch boards fail to build: arch/arm/mach-shmobile/built-in.o: In function `lager_ksz8041_fixup': :(.text+0x738): undefined reference to `mdiobus_read' :(.text+0x73c): undefined reference to `mdiobus_write' arch/arm/mach-shmobile/built-in.o: In function `koelsch_ksz8041_fixup': :(.text+0x7e8): undefined reference to `mdiobus_read' :(.text+0x7ec): undefined reference to `mdiobus_write' To work around that problem, this changes the code to check for IS_BUILTIN rather than IS_ENABLED, turning the error into a runtime problem. It's now possible to build random configurations, but the phy may be set up incorrectly in this case. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Cc: Simon Horman <horms@verge.net.au> Cc: Magnus Damm <magnus.damm@gmail.com> Cc: linux-sh@vger.kernel.org --- arch/arm/mach-shmobile/board-koelsch.c | 2 +- arch/arm/mach-shmobile/board-lager.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)