[PATCH] phy dp83867: depend on CONFIG_OF_MDIO

[PATCH] phy dp83867: depend on CONFIG_OF_MDIO

[Alexander Graf]

The DP83867 phy driver doesn't actually work when CONFIG_OF_MDIO isn't enabled.
It simply passes the device tree test, but leaves all internal configuration
initialized at 0. Then it configures the phy with those values and renders a
previously working configuration useless.

This patch makes sure that we only build the DP83867 phy code when
CONFIG_OF_MDIO is set, to not run into that problem.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 drivers/net/phy/Kconfig   | 1 +
 drivers/net/phy/dp83867.c | 7 -------
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 6dad9a9..4265ad5 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -148,6 +148,7 @@ config DP83848_PHY
 
 config DP83867_PHY
 	tristate "Drivers for Texas Instruments DP83867 Gigabit PHY"
+	depends on OF_MDIO
 	---help---
 	  Currently supports the DP83867 PHY.
 
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 2afa61b..ff867ba 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -99,7 +99,6 @@ static int dp83867_config_intr(struct phy_device *phydev)
 	return phy_write(phydev, MII_DP83867_MICR, micr_status);
 }
 
-#ifdef CONFIG_OF_MDIO
 static int dp83867_of_init(struct phy_device *phydev)
 {
 	struct dp83867_private *dp83867 = phydev->priv;
@@ -123,12 +122,6 @@ static int dp83867_of_init(struct phy_device *phydev)
 	return of_property_read_u32(of_node, "ti,fifo-depth",
 				   &dp83867->fifo_depth);
 }
-#else
-static int dp83867_of_init(struct phy_device *phydev)
-{
-	return 0;
-}
-#endif /* CONFIG_OF_MDIO */
 
 static int dp83867_config_init(struct phy_device *phydev)
 {
-- 
1.8.5.6

Re: [PATCH] phy dp83867: depend on CONFIG_OF_MDIO

[Andrew Lunn]

On Mon, May 16, 2016 at 01:28:15PM +0200, Alexander Graf wrote:
> The DP83867 phy driver doesn't actually work when CONFIG_OF_MDIO isn't enabled.
> It simply passes the device tree test, but leaves all internal configuration
> initialized at 0. Then it configures the phy with those values and renders a
> previously working configuration useless.
> 
> This patch makes sure that we only build the DP83867 phy code when
> CONFIG_OF_MDIO is set, to not run into that problem.

Hi Alexander

Looking at the code, the parameters read from device tree are needed
for RGMII mode. What about the case the PHY is used not in RGMII mode?
Could it be there are device trees which legitimately don't have these
properties since they are not needed, and are now going to get
-ENODEV?

	Andrew

Re: [PATCH] phy dp83867: depend on CONFIG_OF_MDIO

[Dan Murphy]

Alexander

On 05/16/2016 06:28 AM, Alexander Graf wrote:
> The DP83867 phy driver doesn't actually work when CONFIG_OF_MDIO isn't enabled.
> It simply passes the device tree test, but leaves all internal configuration
> initialized at 0. Then it configures the phy with those values and renders a
> previously working configuration useless.
>
> This patch makes sure that we only build the DP83867 phy code when
> CONFIG_OF_MDIO is set, to not run into that problem.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  drivers/net/phy/Kconfig   | 1 +
>  drivers/net/phy/dp83867.c | 7 -------
>  2 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 6dad9a9..4265ad5 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -148,6 +148,7 @@ config DP83848_PHY
>  
>  config DP83867_PHY
>  	tristate "Drivers for Texas Instruments DP83867 Gigabit PHY"
> +	depends on OF_MDIO
>  	---help---
>  	  Currently supports the DP83867 PHY.
>  
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 2afa61b..ff867ba 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -99,7 +99,6 @@ static int dp83867_config_intr(struct phy_device *phydev)
>  	return phy_write(phydev, MII_DP83867_MICR, micr_status);
>  }
>  
> -#ifdef CONFIG_OF_MDIO
>  static int dp83867_of_init(struct phy_device *phydev)
>  {
>  	struct dp83867_private *dp83867 = phydev->priv;
> @@ -123,12 +122,6 @@ static int dp83867_of_init(struct phy_device *phydev)
>  	return of_property_read_u32(of_node, "ti,fifo-depth",
>  				   &dp83867->fifo_depth);
>  }
> -#else
> -static int dp83867_of_init(struct phy_device *phydev)
> -{
> -	return 0;
> -}
> -#endif /* CONFIG_OF_MDIO */
>  
>  static int dp83867_config_init(struct phy_device *phydev)
>  {
I don't think we want this to depend solely on OF_MDIO.

The #else case should probably be coded to look at platform data, if
it exists.  I don't have any boards that still used platform data to test this
out so I did not feel comfortable adding code I could not test.

The platform data should contain the RGMII tx/rx delay and FIFO control.


Dan

-- 
------------------
Dan Murphy

Re: [PATCH] phy dp83867: depend on CONFIG_OF_MDIO

[Alexander Graf]

Hi Dan,

On 16.05.16 15:38, Dan Murphy wrote:
> Alexander
> 
> On 05/16/2016 06:28 AM, Alexander Graf wrote:
>> The DP83867 phy driver doesn't actually work when CONFIG_OF_MDIO isn't enabled.
>> It simply passes the device tree test, but leaves all internal configuration
>> initialized at 0. Then it configures the phy with those values and renders a
>> previously working configuration useless.
>>
>> This patch makes sure that we only build the DP83867 phy code when
>> CONFIG_OF_MDIO is set, to not run into that problem.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>  drivers/net/phy/Kconfig   | 1 +
>>  drivers/net/phy/dp83867.c | 7 -------
>>  2 files changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index 6dad9a9..4265ad5 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -148,6 +148,7 @@ config DP83848_PHY
>>  
>>  config DP83867_PHY
>>  	tristate "Drivers for Texas Instruments DP83867 Gigabit PHY"
>> +	depends on OF_MDIO
>>  	---help---
>>  	  Currently supports the DP83867 PHY.
>>  
>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>> index 2afa61b..ff867ba 100644
>> --- a/drivers/net/phy/dp83867.c
>> +++ b/drivers/net/phy/dp83867.c
>> @@ -99,7 +99,6 @@ static int dp83867_config_intr(struct phy_device *phydev)
>>  	return phy_write(phydev, MII_DP83867_MICR, micr_status);
>>  }
>>  
>> -#ifdef CONFIG_OF_MDIO
>>  static int dp83867_of_init(struct phy_device *phydev)
>>  {
>>  	struct dp83867_private *dp83867 = phydev->priv;
>> @@ -123,12 +122,6 @@ static int dp83867_of_init(struct phy_device *phydev)
>>  	return of_property_read_u32(of_node, "ti,fifo-depth",
>>  				   &dp83867->fifo_depth);
>>  }
>> -#else
>> -static int dp83867_of_init(struct phy_device *phydev)
>> -{
>> -	return 0;
>> -}
>> -#endif /* CONFIG_OF_MDIO */
>>  
>>  static int dp83867_config_init(struct phy_device *phydev)
>>  {
> I don't think we want this to depend solely on OF_MDIO.
> 
> The #else case should probably be coded to look at platform data, if
> it exists.  I don't have any boards that still used platform data to test this
> out so I did not feel comfortable adding code I could not test.

Since there was no code to look at platform data, those boards would be
broken just as well today, no? So at the end of the day, this change
should be no regression for them.


Alex

Re: [PATCH] phy dp83867: depend on CONFIG_OF_MDIO

[Alexander Graf]

On 16.05.16 14:44, Andrew Lunn wrote:
> On Mon, May 16, 2016 at 01:28:15PM +0200, Alexander Graf wrote:
>> The DP83867 phy driver doesn't actually work when CONFIG_OF_MDIO isn't enabled.
>> It simply passes the device tree test, but leaves all internal configuration
>> initialized at 0. Then it configures the phy with those values and renders a
>> previously working configuration useless.
>>
>> This patch makes sure that we only build the DP83867 phy code when
>> CONFIG_OF_MDIO is set, to not run into that problem.
> 
> Hi Alexander
> 
> Looking at the code, the parameters read from device tree are needed
> for RGMII mode. What about the case the PHY is used not in RGMII mode?
> Could it be there are device trees which legitimately don't have these
> properties since they are not needed, and are now going to get
> -ENODEV?

To be honest, I don't know. I only stumbled over this because we compile
all those phy drivers as modules and the driver simply rendered one
system I was working on unusable wrt network.

Dan?


Alex

Re: [PATCH] phy dp83867: depend on CONFIG_OF_MDIO

[Dan Murphy]

Alex

On 05/16/2016 12:57 PM, Alexander Graf wrote:
> Hi Dan,
>
> On 16.05.16 15:38, Dan Murphy wrote:
>> Alexander
>>
>> On 05/16/2016 06:28 AM, Alexander Graf wrote:
>>> The DP83867 phy driver doesn't actually work when CONFIG_OF_MDIO isn't enabled.
>>> It simply passes the device tree test, but leaves all internal configuration
>>> initialized at 0. Then it configures the phy with those values and renders a
>>> previously working configuration useless.
>>>
>>> This patch makes sure that we only build the DP83867 phy code when
>>> CONFIG_OF_MDIO is set, to not run into that problem.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>>  drivers/net/phy/Kconfig   | 1 +
>>>  drivers/net/phy/dp83867.c | 7 -------
>>>  2 files changed, 1 insertion(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>>> index 6dad9a9..4265ad5 100644
>>> --- a/drivers/net/phy/Kconfig
>>> +++ b/drivers/net/phy/Kconfig
>>> @@ -148,6 +148,7 @@ config DP83848_PHY
>>>  
>>>  config DP83867_PHY
>>>  	tristate "Drivers for Texas Instruments DP83867 Gigabit PHY"
>>> +	depends on OF_MDIO
>>>  	---help---
>>>  	  Currently supports the DP83867 PHY.
>>>  
>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>>> index 2afa61b..ff867ba 100644
>>> --- a/drivers/net/phy/dp83867.c
>>> +++ b/drivers/net/phy/dp83867.c
>>> @@ -99,7 +99,6 @@ static int dp83867_config_intr(struct phy_device *phydev)
>>>  	return phy_write(phydev, MII_DP83867_MICR, micr_status);
>>>  }
>>>  
>>> -#ifdef CONFIG_OF_MDIO
>>>  static int dp83867_of_init(struct phy_device *phydev)
>>>  {
>>>  	struct dp83867_private *dp83867 = phydev->priv;
>>> @@ -123,12 +122,6 @@ static int dp83867_of_init(struct phy_device *phydev)
>>>  	return of_property_read_u32(of_node, "ti,fifo-depth",
>>>  				   &dp83867->fifo_depth);
>>>  }
>>> -#else
>>> -static int dp83867_of_init(struct phy_device *phydev)
>>> -{
>>> -	return 0;
>>> -}
>>> -#endif /* CONFIG_OF_MDIO */
>>>  
>>>  static int dp83867_config_init(struct phy_device *phydev)
>>>  {
>> I don't think we want this to depend solely on OF_MDIO.
>>
>> The #else case should probably be coded to look at platform data, if
>> it exists.  I don't have any boards that still used platform data to test this
>> out so I did not feel comfortable adding code I could not test.
> Since there was no code to look at platform data, those boards would be
> broken just as well today, no? So at the end of the day, this change
> should be no regression for them.

As Andrew pointed out if you are not using RGMII you don't need internal delay or fifo_depth so making the driver dependent on OF_MDIO
does not make sense.

The DP83867 RGMII tx and rx delays and fifo should really be changed to optional parameters and only programmed if set.

Dan
>
> Alex


-- 
------------------
Dan Murphy