diff mbox

[60/62] ARM: shmobile: work around CONFIG_PHYLIB=m

Message ID 1395257399-359545-61-git-send-email-arnd@arndb.de (mailing list archive)
State Deferred
Headers show

Commit Message

Arnd Bergmann March 19, 2014, 7:29 p.m. UTC
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(-)

Comments

Simon Horman March 20, 2014, 3:55 a.m. UTC | #1
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
> 
--
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
Arnd Bergmann March 21, 2014, 3:43 p.m. UTC | #2
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
--
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
Simon Horman March 24, 2014, 1:35 a.m. UTC | #3
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.
--
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
Arnd Bergmann March 24, 2014, 12:04 p.m. UTC | #4
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
--
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
Uwe Kleine-König March 25, 2014, 9:16 a.m. UTC | #5
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 mbox

Patch

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);
 }