* [PATCH] phy dp83867: depend on CONFIG_OF_MDIO
@ 2016-05-16 11:28 Alexander Graf
2016-05-16 12:44 ` Andrew Lunn
2016-05-16 13:38 ` Dan Murphy
0 siblings, 2 replies; 6+ messages in thread
From: Alexander Graf @ 2016-05-16 11:28 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel, michal.simek, Dan Murphy
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] phy dp83867: depend on CONFIG_OF_MDIO
2016-05-16 11:28 [PATCH] phy dp83867: depend on CONFIG_OF_MDIO Alexander Graf
@ 2016-05-16 12:44 ` Andrew Lunn
2016-05-16 17:59 ` Alexander Graf
2016-05-16 13:38 ` Dan Murphy
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2016-05-16 12:44 UTC (permalink / raw)
To: Alexander Graf; +Cc: netdev, linux-kernel, michal.simek, Dan Murphy
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] phy dp83867: depend on CONFIG_OF_MDIO
2016-05-16 12:44 ` Andrew Lunn
@ 2016-05-16 17:59 ` Alexander Graf
0 siblings, 0 replies; 6+ messages in thread
From: Alexander Graf @ 2016-05-16 17:59 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev, linux-kernel, michal.simek, Dan Murphy
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] phy dp83867: depend on CONFIG_OF_MDIO
2016-05-16 11:28 [PATCH] phy dp83867: depend on CONFIG_OF_MDIO Alexander Graf
2016-05-16 12:44 ` Andrew Lunn
@ 2016-05-16 13:38 ` Dan Murphy
2016-05-16 17:57 ` Alexander Graf
1 sibling, 1 reply; 6+ messages in thread
From: Dan Murphy @ 2016-05-16 13:38 UTC (permalink / raw)
To: Alexander Graf, netdev; +Cc: linux-kernel, michal.simek
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] phy dp83867: depend on CONFIG_OF_MDIO
2016-05-16 13:38 ` Dan Murphy
@ 2016-05-16 17:57 ` Alexander Graf
2016-05-16 18:22 ` Dan Murphy
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Graf @ 2016-05-16 17:57 UTC (permalink / raw)
To: Dan Murphy, netdev; +Cc: linux-kernel, michal.simek
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] phy dp83867: depend on CONFIG_OF_MDIO
2016-05-16 17:57 ` Alexander Graf
@ 2016-05-16 18:22 ` Dan Murphy
0 siblings, 0 replies; 6+ messages in thread
From: Dan Murphy @ 2016-05-16 18:22 UTC (permalink / raw)
To: Alexander Graf, netdev; +Cc: linux-kernel, michal.simek
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-16 18:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-16 11:28 [PATCH] phy dp83867: depend on CONFIG_OF_MDIO Alexander Graf
2016-05-16 12:44 ` Andrew Lunn
2016-05-16 17:59 ` Alexander Graf
2016-05-16 13:38 ` Dan Murphy
2016-05-16 17:57 ` Alexander Graf
2016-05-16 18:22 ` Dan Murphy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).