[PATCH] phylib: Don't allow core of phylib to build as a module

[PATCH] phylib: Don't allow core of phylib to build as a module

[Kumar Gala]

The core portions of the phylib aren't capable of being used as
a module.  This isn't really any different than something like i2c
in that the bus driver and core need to be built into the kernel.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---

Jeff, please consider this for 2.6.26 as w/o it we get build issues
if phylib is config'd as a module on ppc.

 drivers/net/phy/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 6eb2d31..ab04cc7 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -3,7 +3,7 @@
 #

 menuconfig PHYLIB
-	tristate "PHY Device support and infrastructure"
+	bool "PHY Device support and infrastructure"
 	depends on !S390
 	depends on NET_ETHERNET
 	help
-- 
1.5.4.5

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Jeff Garzik]

Kumar Gala wrote:
> The core portions of the phylib aren't capable of being used as
> a module.  This isn't really any different than something like i2c
> in that the bus driver and core need to be built into the kernel.
> 
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
> 
> Jeff, please consider this for 2.6.26 as w/o it we get build issues
> if phylib is config'd as a module on ppc.
> 
>  drivers/net/phy/Kconfig |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 6eb2d31..ab04cc7 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -3,7 +3,7 @@
>  #
> 
>  menuconfig PHYLIB
> -	tristate "PHY Device support and infrastructure"
> +	bool "PHY Device support and infrastructure"
>  	depends on !S390
>  	depends on NET_ETHERNET

What are the issues?

The core _should_ be able to be built as a module.

	Jeff

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Kumar Gala]

On Jun 2, 2008, at 11:03 AM, Jeff Garzik wrote:

> Kumar Gala wrote:
>> The core portions of the phylib aren't capable of being used as
>> a module.  This isn't really any different than something like i2c
>> in that the bus driver and core need to be built into the kernel.
>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>> ---
>> Jeff, please consider this for 2.6.26 as w/o it we get build issues
>> if phylib is config'd as a module on ppc.
>> drivers/net/phy/Kconfig |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index 6eb2d31..ab04cc7 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -3,7 +3,7 @@
>> #
>> menuconfig PHYLIB
>> -	tristate "PHY Device support and infrastructure"
>> +	bool "PHY Device support and infrastructure"
>> 	depends on !S390
>> 	depends on NET_ETHERNET
>
> What are the issues?
>
> The core _should_ be able to be built as a module.

The core provides functions like phy_read/phy_write.  Andy has  
recently introduced board level workaround/fixups.  The problem is  
these workarounds tend to use phy_read/phy_write and the board/ 
platform code is not built as modules.

So we get errors like:

arch/powerpc/platforms/built-in.o: In function `mpc8568_mds_phy_fixups':
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:99: undefined reference to `phy_write'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:104: undefined reference to `phy_read'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:110: undefined reference to `phy_write'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:115: undefined reference to `phy_write'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:120: undefined reference to `phy_read'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:125: undefined reference to `phy_read'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:132: undefined reference to `phy_write'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:138: undefined reference to `phy_read'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:144: undefined reference to `phy_write'
arch/powerpc/platforms/built-in.o: In function  
`mpc8568_fixup_125_clock':
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:68: undefined reference to `phy_read'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:73: undefined reference to `phy_write'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:78: undefined reference to `phy_write'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:83: undefined reference to `phy_read'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:88: undefined reference to `phy_write'
arch/powerpc/platforms/built-in.o: In function `board_fixups':
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:246: undefined reference to `phy_register_fixup_for_id'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:247: undefined reference to `phy_register_fixup_for_id'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:251: undefined reference to `phy_register_fixup_for_id'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:246: undefined reference to `phy_register_fixup_for_id'
/home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
mpc85xx_mds.c:247: undefined reference to `phy_register_fixup_for_id'
arch/powerpc/platforms/built-in.o:/home/galak/git/master/powerpc/arch/ 
powerpc/platforms/85xx/mpc85xx_mds.c:251: more undefined references to  
`phy_register_fixup_for_id' follow
make: *** [.tmp_vmlinux1] Error 1


- k

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Scott Wood]

On Mon, Jun 02, 2008 at 11:25:14AM -0500, Kumar Gala wrote:
> The core provides functions like phy_read/phy_write.  Andy has recently 
> introduced board level workaround/fixups.  The problem is these 
> workarounds tend to use phy_read/phy_write and the board/platform code is 
> not built as modules.

So why not have those platforms select PHYLIB?

-Scott

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Jeff Garzik]

Kumar Gala wrote:
> 
> On Jun 2, 2008, at 11:03 AM, Jeff Garzik wrote:
> 
>> Kumar Gala wrote:
>>> The core portions of the phylib aren't capable of being used as
>>> a module.  This isn't really any different than something like i2c
>>> in that the bus driver and core need to be built into the kernel.
>>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>>> ---
>>> Jeff, please consider this for 2.6.26 as w/o it we get build issues
>>> if phylib is config'd as a module on ppc.
>>> drivers/net/phy/Kconfig |    2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>>> index 6eb2d31..ab04cc7 100644
>>> --- a/drivers/net/phy/Kconfig
>>> +++ b/drivers/net/phy/Kconfig
>>> @@ -3,7 +3,7 @@
>>> #
>>> menuconfig PHYLIB
>>> -    tristate "PHY Device support and infrastructure"
>>> +    bool "PHY Device support and infrastructure"
>>>     depends on !S390
>>>     depends on NET_ETHERNET
>>
>> What are the issues?
>>
>> The core _should_ be able to be built as a module.
> 
> The core provides functions like phy_read/phy_write.  Andy has recently 
> introduced board level workaround/fixups.  The problem is these 
> workarounds tend to use phy_read/phy_write and the board/platform code 
> is not built as modules.
> 
> So we get errors like:
> 
> arch/powerpc/platforms/built-in.o: In function `mpc8568_mds_phy_fixups':
> /home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/mpc85xx_mds.c:99: 
> undefined reference to `phy_write'
> /home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/mpc85xx_mds.c:104: 
> undefined reference to `phy_read'

The whole world isn't embedded ppc, we use this stuff elsewhere too.

You guys need to figure out something that doesn't require phylib be 
built-in on ALL platforms, but only the platforms that require it.

Or, update the platform to not require built-in -- convert the board 
code to function pointers, and execute them later on somehow, for example.

	Jeff

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Kumar Gala]

On Jun 2, 2008, at 11:39 AM, Jeff Garzik wrote:

> Kumar Gala wrote:
>> On Jun 2, 2008, at 11:03 AM, Jeff Garzik wrote:
>>> Kumar Gala wrote:
>>>> The core portions of the phylib aren't capable of being used as
>>>> a module.  This isn't really any different than something like i2c
>>>> in that the bus driver and core need to be built into the kernel.
>>>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>>>> ---
>>>> Jeff, please consider this for 2.6.26 as w/o it we get build issues
>>>> if phylib is config'd as a module on ppc.
>>>> drivers/net/phy/Kconfig |    2 +-
>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>>>> index 6eb2d31..ab04cc7 100644
>>>> --- a/drivers/net/phy/Kconfig
>>>> +++ b/drivers/net/phy/Kconfig
>>>> @@ -3,7 +3,7 @@
>>>> #
>>>> menuconfig PHYLIB
>>>> -    tristate "PHY Device support and infrastructure"
>>>> +    bool "PHY Device support and infrastructure"
>>>>    depends on !S390
>>>>    depends on NET_ETHERNET
>>>
>>> What are the issues?
>>>
>>> The core _should_ be able to be built as a module.
>> The core provides functions like phy_read/phy_write.  Andy has  
>> recently introduced board level workaround/fixups.  The problem is  
>> these workarounds tend to use phy_read/phy_write and the board/ 
>> platform code is not built as modules.
>> So we get errors like:
>> arch/powerpc/platforms/built-in.o: In function  
>> `mpc8568_mds_phy_fixups':
>> /home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
>> mpc85xx_mds.c:99: undefined reference to `phy_write'
>> /home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
>> mpc85xx_mds.c:104: undefined reference to `phy_read'
>
> The whole world isn't embedded ppc, we use this stuff elsewhere too.
>
> You guys need to figure out something that doesn't require phylib be  
> built-in on ALL platforms, but only the platforms that require it.

I wasn't suggesting we build it always, just not let it be built as a  
module.

> Or, update the platform to not require built-in -- convert the board  
> code to function pointers, and execute them later on somehow, for  
> example.

- k

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Jeff Garzik]

Kumar Gala wrote:
> 
> On Jun 2, 2008, at 11:39 AM, Jeff Garzik wrote:
> 
>> Kumar Gala wrote:
>>> On Jun 2, 2008, at 11:03 AM, Jeff Garzik wrote:
>>>> Kumar Gala wrote:
>>>>> The core portions of the phylib aren't capable of being used as
>>>>> a module.  This isn't really any different than something like i2c
>>>>> in that the bus driver and core need to be built into the kernel.
>>>>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>>>>> ---
>>>>> Jeff, please consider this for 2.6.26 as w/o it we get build issues
>>>>> if phylib is config'd as a module on ppc.
>>>>> drivers/net/phy/Kconfig |    2 +-
>>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>>>>> index 6eb2d31..ab04cc7 100644
>>>>> --- a/drivers/net/phy/Kconfig
>>>>> +++ b/drivers/net/phy/Kconfig
>>>>> @@ -3,7 +3,7 @@
>>>>> #
>>>>> menuconfig PHYLIB
>>>>> -    tristate "PHY Device support and infrastructure"
>>>>> +    bool "PHY Device support and infrastructure"
>>>>>    depends on !S390
>>>>>    depends on NET_ETHERNET
>>>>
>>>> What are the issues?
>>>>
>>>> The core _should_ be able to be built as a module.
>>> The core provides functions like phy_read/phy_write.  Andy has 
>>> recently introduced board level workaround/fixups.  The problem is 
>>> these workarounds tend to use phy_read/phy_write and the 
>>> board/platform code is not built as modules.
>>> So we get errors like:
>>> arch/powerpc/platforms/built-in.o: In function `mpc8568_mds_phy_fixups':
>>> /home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/mpc85xx_mds.c:99: 
>>> undefined reference to `phy_write'
>>> /home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/mpc85xx_mds.c:104: 
>>> undefined reference to `phy_read'
>>
>> The whole world isn't embedded ppc, we use this stuff elsewhere too.
>>
>> You guys need to figure out something that doesn't require phylib be 
>> built-in on ALL platforms, but only the platforms that require it.
> 
> I wasn't suggesting we build it always, just not let it be built as a 
> module.

I was saying, you are requiring everyone to bloat their kernel with 
phylib, if they enable phylib, because of your particular platform details.

That is not a path we want to follow -- limiting everyone else because 
of one case is not acceptable.

	Jeff

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Kumar Gala]

>>> The whole world isn't embedded ppc, we use this stuff elsewhere too.
>>>
>>> You guys need to figure out something that doesn't require phylib  
>>> be built-in on ALL platforms, but only the platforms that require  
>>> it.
>> I wasn't suggesting we build it always, just not let it be built as  
>> a module.
>
> I was saying, you are requiring everyone to bloat their kernel with  
> phylib, if they enable phylib, because of your particular platform  
> details.
>
> That is not a path we want to follow -- limiting everyone else  
> because of one case is not acceptable.

Are you saying that all the driver subsystems that require being built  
into the kernel should be changed to not require this? (I2C, SPI, DMA  
engine, etc.)

- k

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Kumar Gala]

On Jun 2, 2008, at 11:39 AM, Jeff Garzik wrote:

> Kumar Gala wrote:
>> On Jun 2, 2008, at 11:03 AM, Jeff Garzik wrote:
>>> Kumar Gala wrote:
>>>> The core portions of the phylib aren't capable of being used as
>>>> a module.  This isn't really any different than something like i2c
>>>> in that the bus driver and core need to be built into the kernel.
>>>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>>>> ---
>>>> Jeff, please consider this for 2.6.26 as w/o it we get build issues
>>>> if phylib is config'd as a module on ppc.
>>>> drivers/net/phy/Kconfig |    2 +-
>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>>>> index 6eb2d31..ab04cc7 100644
>>>> --- a/drivers/net/phy/Kconfig
>>>> +++ b/drivers/net/phy/Kconfig
>>>> @@ -3,7 +3,7 @@
>>>> #
>>>> menuconfig PHYLIB
>>>> -    tristate "PHY Device support and infrastructure"
>>>> +    bool "PHY Device support and infrastructure"
>>>>    depends on !S390
>>>>    depends on NET_ETHERNET
>>>
>>> What are the issues?
>>>
>>> The core _should_ be able to be built as a module.
>> The core provides functions like phy_read/phy_write.  Andy has  
>> recently introduced board level workaround/fixups.  The problem is  
>> these workarounds tend to use phy_read/phy_write and the board/ 
>> platform code is not built as modules.
>> So we get errors like:
>> arch/powerpc/platforms/built-in.o: In function  
>> `mpc8568_mds_phy_fixups':
>> /home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
>> mpc85xx_mds.c:99: undefined reference to `phy_write'
>> /home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
>> mpc85xx_mds.c:104: undefined reference to `phy_read'
>
> The whole world isn't embedded ppc, we use this stuff elsewhere too.
>
> You guys need to figure out something that doesn't require phylib be  
> built-in on ALL platforms, but only the platforms that require it.
>
> Or, update the platform to not require built-in -- convert the board  
> code to function pointers, and execute them later on somehow, for  
> example.

If you really think the core of the phylib should be able to be built  
as a module than we could possibly add function pointers to phy_dev to  
do the real phy_read()/phy_write() and change phy_read/_write to look  
like:

int phy_read(struct phy_device *phydev, u16 regnum) {
	return phydev->read(phydev, regnum);
}

int phy_write(struct phy_device *phydev, u16 regnum, u16 val) {
	return phydev->write(phydev, regnum, val);
}

- k

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Andy Fleming]

On Jun 2, 2008, at 14:30, Kumar Gala wrote:

>
> On Jun 2, 2008, at 11:39 AM, Jeff Garzik wrote:
>
> If you really think the core of the phylib should be able to be  
> built as a module than we could possibly add function pointers to  
> phy_dev to do the real phy_read()/phy_write() and change phy_read/ 
> _write to look like:
>
> int phy_read(struct phy_device *phydev, u16 regnum) {
> 	return phydev->read(phydev, regnum);
> }

That would be a bit silly, since this is the definition of phy_read():

int phy_read(struct phy_device *phydev, u16 regnum)
{
         int retval;
         struct mii_bus *bus = phydev->bus;

         BUG_ON(in_interrupt());

         mutex_lock(&bus->mdio_lock);
         retval = bus->read(bus, phydev->addr, regnum);
         mutex_unlock(&bus->mdio_lock);

         return retval;
}


We could, of course, move phy_read *out* of the phylib module.  And  
also phy_register_fixup and any other functions needed by board code.

I'm partial to the select-it-if-you-need-it paradigm.

Andy

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Jeff Garzik]

Andy Fleming wrote:
> I'm partial to the select-it-if-you-need-it paradigm.


AFAICS this can all be solved by the platform Kconfig ensuring that phylib=y

	Jeff

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Kumar Gala]

On Jun 2, 2008, at 3:30 PM, Jeff Garzik wrote:

> Andy Fleming wrote:
>> I'm partial to the select-it-if-you-need-it paradigm.
>
>
> AFAICS this can all be solved by the platform Kconfig ensuring that  
> phylib=y

I don't care for this as it means making sure each platform/board port  
gets it right.  I think we'd be better off with a small stub that is  
always built into the kernel for phy_read/phy_write, etc or the  
function pointer indirection mechanism.

- k

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Scott Wood]

Kumar Gala wrote:
> On Jun 2, 2008, at 3:30 PM, Jeff Garzik wrote:
> 
>> Andy Fleming wrote:
>>> I'm partial to the select-it-if-you-need-it paradigm.
>>
>>
>> AFAICS this can all be solved by the platform Kconfig ensuring that 
>> phylib=y
> 
> I don't care for this as it means making sure each platform/board port 
> gets it right.

How is this different from any other kconfig dependency?  It's not too 
hard to scan through your platform code and see what you call...

> I think we'd be better off with a small stub that is 
> always built into the kernel for phy_read/phy_write, etc or the function 
> pointer indirection mechanism.

And then instead of build failures, you'd get a silent runtime failure 
to apply the workaround if phylib is built as a module.

-Scott

[RFC] Make board force selection of PHYLIB

[Kumar Gala]

How is this as a fix.

- k

diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
index 7ff29d5..9e5c884 100644
--- a/arch/powerpc/platforms/85xx/Kconfig
+++ b/arch/powerpc/platforms/85xx/Kconfig
@@ -34,6 +34,7 @@ config MPC85xx_MDS
 	bool "Freescale MPC85xx MDS"
 	select DEFAULT_UIMAGE
 	select QUICC_ENGINE
+	select PHYLIB if GIANFAR=m
 	help
 	  This option enables support for the MPC85xx MDS board

Re: [RFC] Make board force selection of PHYLIB

[Scott Wood]

Kumar Gala wrote:
> How is this as a fix.
> 
> - k
> 
> diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
> index 7ff29d5..9e5c884 100644
> --- a/arch/powerpc/platforms/85xx/Kconfig
> +++ b/arch/powerpc/platforms/85xx/Kconfig
> @@ -34,6 +34,7 @@ config MPC85xx_MDS
>  	bool "Freescale MPC85xx MDS"
>  	select DEFAULT_UIMAGE
>  	select QUICC_ENGINE
> +	select PHYLIB if GIANFAR=m
>  	help
>  	  This option enables support for the MPC85xx MDS board

I assume you'll also ifdef-protect the phy calls in the platform code?

I'd rather avoid adding another case where the kernel needs to know what 
modules are being built, though, especially if the result of changing 
the .config and building modules is a mysterious runtime failure (due to 
a missing platform fixup) rather than compile- or insertion-time.

-Scott

Re: [RFC] Make board force selection of PHYLIB

[Kumar Gala]

On Jun 3, 2008, at 10:10 AM, Scott Wood wrote:

> Kumar Gala wrote:
>> How is this as a fix.
>> - k
>> diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/ 
>> platforms/85xx/Kconfig
>> index 7ff29d5..9e5c884 100644
>> --- a/arch/powerpc/platforms/85xx/Kconfig
>> +++ b/arch/powerpc/platforms/85xx/Kconfig
>> @@ -34,6 +34,7 @@ config MPC85xx_MDS
>> 	bool "Freescale MPC85xx MDS"
>> 	select DEFAULT_UIMAGE
>> 	select QUICC_ENGINE
>> +	select PHYLIB if GIANFAR=m
>> 	help
>> 	  This option enables support for the MPC85xx MDS board
>
> I assume you'll also ifdef-protect the phy calls in the platform code?

yes this needs to get done.

> I'd rather avoid adding another case where the kernel needs to know  
> what modules are being built, though, especially if the result of  
> changing the .config and building modules is a mysterious runtime  
> failure (due to a missing platform fixup) rather than compile- or  
> insertion-time.

I don't follow what you are getting at here.  Is this something more  
than #ifdef PHYLIB in the platform code?

- k

Re: [RFC] Make board force selection of PHYLIB

[Scott Wood]

Kumar Gala wrote:
> On Jun 3, 2008, at 10:10 AM, Scott Wood wrote:
>> I'd rather avoid adding another case where the kernel needs to know 
>> what modules are being built, though, especially if the result of 
>> changing the .config and building modules is a mysterious runtime 
>> failure (due to a missing platform fixup) rather than compile- or 
>> insertion-time.
> 
> I don't follow what you are getting at here.  Is this something more 
> than #ifdef PHYLIB in the platform code?

If you just #ifdef PHYLIB, then things will break if the user does this:
make config, GIANFAR=PHYLIB=n
make zImage
make config, GIANFAR=PHYLIB=m
make modules

And the cause of the failure will not be something that obviously points 
to a build problem, such as unresolved symbols.

I'd rather just unconditionally select PHYLIB on platforms that need to 
do fixups.

-Scott

Re: [RFC] Make board force selection of PHYLIB

[Kumar Gala]

On Jun 3, 2008, at 10:18 AM, Scott Wood wrote:

> Kumar Gala wrote:
>> On Jun 3, 2008, at 10:10 AM, Scott Wood wrote:
>>> I'd rather avoid adding another case where the kernel needs to  
>>> know what modules are being built, though, especially if the  
>>> result of changing the .config and building modules is a  
>>> mysterious runtime failure (due to a missing platform fixup)  
>>> rather than compile- or insertion-time.
>> I don't follow what you are getting at here.  Is this something  
>> more than #ifdef PHYLIB in the platform code?
>
> If you just #ifdef PHYLIB, then things will break if the user does  
> this:
> make config, GIANFAR=PHYLIB=n
> make zImage
> make config, GIANFAR=PHYLIB=m
> make modules
>
> And the cause of the failure will not be something that obviously  
> points to a build problem, such as unresolved symbols.

what you are suggesting will not break with my patch.

The second case will for PHYLIB=y w/the select.

> I'd rather just unconditionally select PHYLIB on platforms that need  
> to do fixups.

But you don't need fix ups for the phy if you don't have the enet  
driver that the phy is connected to in your system.

(But I do understand the desire to be generous, but I think we can get  
this right).

- k

Re: [RFC] Make board force selection of PHYLIB

[Scott Wood]

Kumar Gala wrote:
> On Jun 3, 2008, at 10:18 AM, Scott Wood wrote:
>> Kumar Gala wrote:
>>> On Jun 3, 2008, at 10:10 AM, Scott Wood wrote:
>>>> I'd rather avoid adding another case where the kernel needs to know 
>>>> what modules are being built, though, especially if the result of 
>>>> changing the .config and building modules is a mysterious runtime 
>>>> failure (due to a missing platform fixup) rather than compile- or 
>>>> insertion-time.
>>> I don't follow what you are getting at here.  Is this something more 
>>> than #ifdef PHYLIB in the platform code?
>>
>> If you just #ifdef PHYLIB, then things will break if the user does this:
>> make config, GIANFAR=PHYLIB=n
>> make zImage
>> make config, GIANFAR=PHYLIB=m
>> make modules
>>
>> And the cause of the failure will not be something that obviously 
>> points to a build problem, such as unresolved symbols.
> 
> what you are suggesting will not break with my patch.

Yes, it will -- note the absence of a "make zImage" after the second 
make config.

> The second case will for PHYLIB=y w/the select.

And that will only make a difference if you rebuild the kernel itself 
after enabling the module.

-Scott

Re: [RFC] Make board force selection of PHYLIB

[Kumar Gala]

On Jun 3, 2008, at 10:36 AM, Scott Wood wrote:

> Kumar Gala wrote:
>> On Jun 3, 2008, at 10:18 AM, Scott Wood wrote:
>>> Kumar Gala wrote:
>>>> On Jun 3, 2008, at 10:10 AM, Scott Wood wrote:
>>>>> I'd rather avoid adding another case where the kernel needs to  
>>>>> know what modules are being built, though, especially if the  
>>>>> result of changing the .config and building modules is a  
>>>>> mysterious runtime failure (due to a missing platform fixup)  
>>>>> rather than compile- or insertion-time.
>>>> I don't follow what you are getting at here.  Is this something  
>>>> more than #ifdef PHYLIB in the platform code?
>>>
>>> If you just #ifdef PHYLIB, then things will break if the user does  
>>> this:
>>> make config, GIANFAR=PHYLIB=n
>>> make zImage
>>> make config, GIANFAR=PHYLIB=m
>>> make modules
>>>
>>> And the cause of the failure will not be something that obviously  
>>> points to a build problem, such as unresolved symbols.
>> what you are suggesting will not break with my patch.
>
> Yes, it will -- note the absence of a "make zImage" after the second  
> make config.
>
>> The second case will for PHYLIB=y w/the select.
>
> And that will only make a difference if you rebuild the kernel  
> itself after enabling the module.

I see.  However, I don't like the idea I have to build in the PHYLIB  
if I don't need it at all.  It seems like the type of breakage you are  
talking about exists today all over the place.  I dont like it anymore  
than you do, but it seems to me the lesser of evils is to allow the  
user the ability to config things as they want.

- k

Re: [RFC] Make board force selection of PHYLIB

[Scott Wood]

Kumar Gala wrote:
> I see.  However, I don't like the idea I have to build in the PHYLIB
> if I don't need it at all.

Understood, but in practice I don't think the 10K or so matters that
much on a typical 85xx board, and if the user really needs to trim
things down, they always have the option of manually ripping the phy
stuff out.

> It seems like the type of breakage you are talking about exists today
> all over the place.

It exists in a few places, but it's not that common, and often it's 
obvious what went wrong when the module won't load because some kernel 
hook didn't get built.  It's not a huge deal, but I'd rather avoid 
introducing another instance.

-Scott

Re: [RFC] Make board force selection of PHYLIB

[Andy Fleming]

On Jun 3, 2008, at 10:31, Kumar Gala wrote:

>
> On Jun 3, 2008, at 10:18 AM, Scott Wood wrote:
>>
>> If you just #ifdef PHYLIB, then things will break if the user does  
>> this:
>> make config, GIANFAR=PHYLIB=n
>> make zImage
>> make config, GIANFAR=PHYLIB=m
>> make modules
>>
>> And the cause of the failure will not be something that obviously  
>> points to a build problem, such as unresolved symbols.
>
> what you are suggesting will not break with my patch.
>
> The second case will for PHYLIB=y w/the select.
>
>> I'd rather just unconditionally select PHYLIB on platforms that  
>> need to do fixups.
>
> But you don't need fix ups for the phy if you don't have the enet  
> driver that the phy is connected to in your system.
>
> (But I do understand the desire to be generous, but I think we can  
> get this right).


Nonono.  You can't make that code depend on GIANFAR.  Remember, the  
8568 has eTSECs *and* UCCs.  I'm in favor of selecting the PHYLIB  
unconditionally.  But if you really hate that, then at least select it  
based on *either* of those drivers being enabled.

Re: [RFC] Make board force selection of PHYLIB

[Adrian Bunk]

On Tue, Jun 03, 2008 at 09:47:19AM -0500, Kumar Gala wrote:
> How is this as a fix.
> 
> - k
> 
> diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
> index 7ff29d5..9e5c884 100644
> --- a/arch/powerpc/platforms/85xx/Kconfig
> +++ b/arch/powerpc/platforms/85xx/Kconfig
> @@ -34,6 +34,7 @@ config MPC85xx_MDS
>  	bool "Freescale MPC85xx MDS"
>  	select DEFAULT_UIMAGE
>  	select QUICC_ENGINE
> +	select PHYLIB if GIANFAR=m
>  	help
>  	  This option enables support for the MPC85xx MDS board

The .config you sent me as an example didn't have GIANFAR set, so it 
couldn't help there.

How early do the fixups have to run?

I see two possible solutions:
- let MPC85xx_MDS unconditionally select PHYLIB or
- move the fixups to the gianfar driver

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [RFC] Make board force selection of PHYLIB

[Andy Fleming]

On Jun 3, 2008, at 12:00, Adrian Bunk wrote:

> On Tue, Jun 03, 2008 at 09:47:19AM -0500, Kumar Gala wrote:
>> How is this as a fix.
>>
>> - k
>>
>> diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/ 
>> platforms/85xx/Kconfig
>> index 7ff29d5..9e5c884 100644
>> --- a/arch/powerpc/platforms/85xx/Kconfig
>> +++ b/arch/powerpc/platforms/85xx/Kconfig
>> @@ -34,6 +34,7 @@ config MPC85xx_MDS
>> 	bool "Freescale MPC85xx MDS"
>> 	select DEFAULT_UIMAGE
>> 	select QUICC_ENGINE
>> +	select PHYLIB if GIANFAR=m
>> 	help
>> 	  This option enables support for the MPC85xx MDS board
>
> The .config you sent me as an example didn't have GIANFAR set, so it
> couldn't help there.
>
> How early do the fixups have to run?
>
> I see two possible solutions:
> - let MPC85xx_MDS unconditionally select PHYLIB or
> - move the fixups to the gianfar driver

Just to reiterate the point:  The fixups are not only not gianfar- 
specific, they may not involve gianfar at all.  The 8568 has *two*  
types of built-in ethernet controllers, which use 4 PHYs (of one  
type).  The fixups are purely PHY-specific for this board.

Andy

Re: [RFC] Make board force selection of PHYLIB

[Kumar Gala]

On Jun 3, 2008, at 12:00 PM, Adrian Bunk wrote:

> On Tue, Jun 03, 2008 at 09:47:19AM -0500, Kumar Gala wrote:
>> How is this as a fix.
>>
>> - k
>>
>> diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/ 
>> platforms/85xx/Kconfig
>> index 7ff29d5..9e5c884 100644
>> --- a/arch/powerpc/platforms/85xx/Kconfig
>> +++ b/arch/powerpc/platforms/85xx/Kconfig
>> @@ -34,6 +34,7 @@ config MPC85xx_MDS
>> 	bool "Freescale MPC85xx MDS"
>> 	select DEFAULT_UIMAGE
>> 	select QUICC_ENGINE
>> +	select PHYLIB if GIANFAR=m
>> 	help
>> 	  This option enables support for the MPC85xx MDS board
>
> The .config you sent me as an example didn't have GIANFAR set, so it
> couldn't help there.
>
> How early do the fixups have to run?
>
> I see two possible solutions:
> - let MPC85xx_MDS unconditionally select PHYLIB or
> - move the fixups to the gianfar driver

I'm ok w/unconditionally selecting PHYLIB for MPC85xx_MDS as that  
seems to be the best solution at this point.

I'll workup a patch and send it via the powerpc.git tree.

- k

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Grant Likely]

On Mon, Jun 2, 2008 at 5:20 PM, Scott Wood <scottwood@freescale.com> wrote:
> Kumar Gala wrote:
>> I think we'd be better off with a small stub that is always built into the
>> kernel for phy_read/phy_write, etc or the function pointer indirection
>> mechanism.
>
> And then instead of build failures, you'd get a silent runtime failure to
> apply the workaround if phylib is built as a module.

Indeed; I vote for the build failure over the silent runtime failure.
If a platform needs it and does not select it, then the platform is
broken and it is a bug.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Adrian Bunk]

On Mon, Jun 02, 2008 at 11:25:14AM -0500, Kumar Gala wrote:
>
> On Jun 2, 2008, at 11:03 AM, Jeff Garzik wrote:
>
>> Kumar Gala wrote:
>>> The core portions of the phylib aren't capable of being used as
>>> a module.  This isn't really any different than something like i2c
>>> in that the bus driver and core need to be built into the kernel.
>>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>>> ---
>>> Jeff, please consider this for 2.6.26 as w/o it we get build issues
>>> if phylib is config'd as a module on ppc.
>>> drivers/net/phy/Kconfig |    2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>>> index 6eb2d31..ab04cc7 100644
>>> --- a/drivers/net/phy/Kconfig
>>> +++ b/drivers/net/phy/Kconfig
>>> @@ -3,7 +3,7 @@
>>> #
>>> menuconfig PHYLIB
>>> -	tristate "PHY Device support and infrastructure"
>>> +	bool "PHY Device support and infrastructure"
>>> 	depends on !S390
>>> 	depends on NET_ETHERNET
>>
>> What are the issues?
>>
>> The core _should_ be able to be built as a module.
>
> The core provides functions like phy_read/phy_write.  Andy has recently 
> introduced board level workaround/fixups.  The problem is these 
> workarounds tend to use phy_read/phy_write and the board/platform code is 
> not built as modules.
>
> So we get errors like:
>
> arch/powerpc/platforms/built-in.o: In function `mpc8568_mds_phy_fixups':
> /home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/ 
> mpc85xx_mds.c:99: undefined reference to `phy_write'
>...

At first glance PHYLIB=n might also cause similar problems.

Please send me the failing .config and I'll cook up a fix.

> - k

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] phylib: Don't allow core of phylib to build as a module

[Kumar Gala]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1992 bytes --]

On Mon, 2 Jun 2008, Adrian Bunk wrote:

> On Mon, Jun 02, 2008 at 11:25:14AM -0500, Kumar Gala wrote:
> >
> > On Jun 2, 2008, at 11:03 AM, Jeff Garzik wrote:
> >
> >> Kumar Gala wrote:
> >>> The core portions of the phylib aren't capable of being used as
> >>> a module.  This isn't really any different than something like i2c
> >>> in that the bus driver and core need to be built into the kernel.
> >>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> >>> ---
> >>> Jeff, please consider this for 2.6.26 as w/o it we get build issues
> >>> if phylib is config'd as a module on ppc.
> >>> drivers/net/phy/Kconfig |    2 +-
> >>> 1 files changed, 1 insertions(+), 1 deletions(-)
> >>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> >>> index 6eb2d31..ab04cc7 100644
> >>> --- a/drivers/net/phy/Kconfig
> >>> +++ b/drivers/net/phy/Kconfig
> >>> @@ -3,7 +3,7 @@
> >>> #
> >>> menuconfig PHYLIB
> >>> -	tristate "PHY Device support and infrastructure"
> >>> +	bool "PHY Device support and infrastructure"
> >>> 	depends on !S390
> >>> 	depends on NET_ETHERNET
> >>
> >> What are the issues?
> >>
> >> The core _should_ be able to be built as a module.
> >
> > The core provides functions like phy_read/phy_write.  Andy has recently
> > introduced board level workaround/fixups.  The problem is these
> > workarounds tend to use phy_read/phy_write and the board/platform code is
> > not built as modules.
> >
> > So we get errors like:
> >
> > arch/powerpc/platforms/built-in.o: In function `mpc8568_mds_phy_fixups':
> > /home/galak/git/master/powerpc/arch/powerpc/platforms/85xx/
> > mpc85xx_mds.c:99: undefined reference to `phy_write'
> >...
>
> At first glance PHYLIB=n might also cause similar problems.
>
> Please send me the failing .config and I'll cook up a fix.
>
> > - k
>
> cu
> Adrian

defconfig attached.  I think Scott might be right in that the simple fix
would be to select PHYLIB from the MPC8568_MDS config in
arch/powerpc/platforms/85xx/Kconfig.

- k

[-- Attachment #2: Type: APPLICATION/x-gzip, Size: 9536 bytes --]

[PATCH] [POWERPC] 85xx: MPC85xx MDS - Unconditionally select PHYLIB for board fixups

[Kumar Gala]

The MPC85xx MDS board requires some board level tweaks of the PHYs that
either the eTSEC (gianfar) or UCC ethernet controllers are connected to.

Its possible to build the phylib as a module, however this breaks the
board level fix ups because phy_read and phy_write are not available
if we build as a module.

So we unconditionally select PHYLIB to ensure its built into the kernel
if we are building in MPC85xx MDS support.  This was determined to be
the easiest soultion even though it prevents the user from removing
PHYLIB support if they decide they don't want it.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---

will go via the powerpc.git tree for 2.6.26.

- k

 arch/powerpc/platforms/85xx/Kconfig |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
index 7ff29d5..ecbe580 100644
--- a/arch/powerpc/platforms/85xx/Kconfig
+++ b/arch/powerpc/platforms/85xx/Kconfig
@@ -34,6 +34,7 @@ config MPC85xx_MDS
 	bool "Freescale MPC85xx MDS"
 	select DEFAULT_UIMAGE
 	select QUICC_ENGINE
+	select PHYLIB
 	help
 	  This option enables support for the MPC85xx MDS board

-- 
1.5.5.1

Re: [PATCH] [POWERPC] 85xx: MPC85xx MDS - Unconditionally select PHYLIB for board fixups

[Jeff Garzik]

Kumar Gala wrote:
> The MPC85xx MDS board requires some board level tweaks of the PHYs that
> either the eTSEC (gianfar) or UCC ethernet controllers are connected to.
> 
> Its possible to build the phylib as a module, however this breaks the
> board level fix ups because phy_read and phy_write are not available
> if we build as a module.
> 
> So we unconditionally select PHYLIB to ensure its built into the kernel
> if we are building in MPC85xx MDS support.  This was determined to be
> the easiest soultion even though it prevents the user from removing
> PHYLIB support if they decide they don't want it.
> 
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
> 
> will go via the powerpc.git tree for 2.6.26.
> 
> - k
> 
>  arch/powerpc/platforms/85xx/Kconfig |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
> index 7ff29d5..ecbe580 100644
> --- a/arch/powerpc/platforms/85xx/Kconfig
> +++ b/arch/powerpc/platforms/85xx/Kconfig
> @@ -34,6 +34,7 @@ config MPC85xx_MDS
>  	bool "Freescale MPC85xx MDS"
>  	select DEFAULT_UIMAGE
>  	select QUICC_ENGINE
> +	select PHYLIB
>  	help

ACK