diff mbox

drivers: net: cpsw: fix RMII/RGMII mode when used with fixed-link PHY

Message ID 20151209223115.6c5a400f.drivshin.allworx@gmail.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

David Rivshin (Allworx) Dec. 10, 2015, 3:31 a.m. UTC
Commit 1f71e8c96fc654724723ce987e0a8b2aeb81746d ("drivers: net: cpsw: Add
support for fixed-link PHY") used a 'goto no_phy_slave' to skip over the
processing of the mutually-exclusive "phy_id" property. Unfortunately that
also skipped the "phy-mode" property processing, leaving slave_data->phy_if
with its default of PHY_INTERFACE_MODE_NA(0). This later gets passed to
phy_connect() in cpsw_slave_open(), and eventually to cpsw_phy_sel() where
it hits a default case that configures the MAC for MII mode.

The user visible symptom is that while kernel log messages seem to indicate
that the interface is set up, there is no network communication. Eventually
a watchdog error occurs:
    NETDEV WATCHDOG: eth0 (cpsw): transmit queue 0 timed out

Signed-off-by: David Rivshin (Allworx) <drivshin.allworx@gmail.com>
Fixes: 1f71e8c96fc6 ("drivers: net: cpsw: Add support for fixed-link PHY")
---

This patch was originally developed in parallel with 1f71e8c96fc6 to
accomplish the same goal. When I replaced this patch with 1f71e8c96fc6,
I found that it did not work on my hardware (which uses RGMII), so I
am submitting this patch now as a bugfix. I have rebased this to the
current head of the net tree, but because of the different order of the
logic (see explanation below) it ends up replacing 1f71e8c96fc6's changes
in cpsw.c. If a minimal patch is preferred, I can do that instead.

Besides fixing the bug mentioned in the commit log, there are a few other
differences to note:
 * If both "phy_id" and a "fixed-link" subnode are present this patch will
   use the "phy_id" property. This should preserve current behavior with
   existing devicetrees that might incorrectly have both. This motivates
   the biggest difference in code organization from 1f71e8c96fc6.
 * Some error messages have been tweaked to reflect the slightly changed
   meanings of the checks.
 * of_node_get() is called on slave_node before passing the result to
   of_phy_find_device(). This increments the reference count, which I
   believe is correct for this situation, but I'm not certain of that.
 * Uses the phy_device's 'addr' instead of 'phy_id' field when constructing
   slave_data->phy_id. Pascal Speck separately came to the same conclusion
   in another patch [1].
 * [2] pointed out that the 'lenp' sanity check was wrong, and since this
   patch re-arranged that check anyways I incorporated that fix as well.
Although the last three items could be fixed separately, it seemed like that
would be unnecessary churn since those same lines were already touched in
this patch. Let me know if its preferred to fix them separately.

This patch is also very similar to one Daniel Trautmann submitted [3], with
the biggest difference being that Daniel's patch uses the new priv->phy_node
field and of_node_get(). While this seems like a better path overall it
does not work if more than once CPSW slave is used, due to struct cpsw_priv
being shared with all the slaves. I am under the impression that phy_node
should really be in struct cpsw_slave instead, but I will leave that for
another patch.

Checkpatch does flag 3 issues I've decided to leave, but I'd be happy to
resolve them if desired:
WARNING: line over 80 characters (cpsw.c:2046):
+                               dev_err(&pdev->dev, "Invalid slave[%d] phy_id property\n", i);
WARNING: line over 80 characters (cpsw.c:2077):
+                       dev_err(&pdev->dev, "No slave[%d] phy_id or fixed-link property\n", i);
    Both modifications of the same pre-existing messages that was already
    over 80 characters. Seemed more readable to leave them as 1-liners.
CHECK: spaces preferred around that '+' (ctx:VxV) (cpsw.c:2051):
+                       phyid = be32_to_cpup(parp+1);
    ALso pre-existing, and far enough from the purpose of this patch that it
    seemed gratuitous to change now.

[1] http://www.spinics.net/lists/netdev/msg355254.html
[2] http://www.spinics.net/lists/netdev/msg351703.html
[3] http://www.spinics.net/lists/netdev/msg351674.html

(Side note: This is my first patch submission, and any feedback on things 
to do differently in the future would be appreciated.)

 drivers/net/ethernet/ti/cpsw.c | 65 +++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 26 deletions(-)

Comments

Markus Brunner Dec. 12, 2015, 3:44 p.m. UTC | #1
On Wednesday 09 December 2015 22:31:15 David Rivshin wrote:
...
> This patch was originally developed in parallel with 1f71e8c96fc6 to
> accomplish the same goal. When I replaced this patch with 1f71e8c96fc6,
> I found that it did not work on my hardware (which uses RGMII), so I
> am submitting this patch now as a bugfix. 

Your patch works fine on my board, which uses MII and dual_emac with a fixed_phy and a real one. 

> Besides fixing the bug mentioned in the commit log, there are a few other
> differences to note:
>  * If both "phy_id" and a "fixed-link" subnode are present this patch will
>    use the "phy_id" property. This should preserve current behavior with
>    existing devicetrees that might incorrectly have both. This motivates
>    the biggest difference in code organization from 1f71e8c96fc6.
>  * Some error messages have been tweaked to reflect the slightly changed
>    meanings of the checks.

I wanted to keep changes small and didn't spend too much thinking about already broken devicetrees. 
Since my patch is quite new, I don't see any problems with subtle changes like that. However you should update the documentation as well. 

>  * of_node_get() is called on slave_node before passing the result to
>    of_phy_find_device(). This increments the reference count, which I
>    believe is correct for this situation, but I'm not certain of that.
I think you are right. At least most other drivers calling of_phy_register_fixed_link(), call of_node_get afterwards. 
I can't remember where I got my "inspiration" for the patch. I made it almost a year ago and now 3 other guys tinker with the same feature? What a coincidence ;-)

>  * Uses the phy_device's 'addr' instead of 'phy_id' field when constructing
>    slave_data->phy_id. Pascal Speck separately came to the same conclusion
>    in another patch [1].
Clearly an improvement.
>  * [2] pointed out that the 'lenp' sanity check was wrong, and since this
>    patch re-arranged that check anyways I incorporated that fix as well.
> Although the last three items could be fixed separately, it seemed like that
> would be unnecessary churn since those same lines were already touched in
> this patch. Let me know if its preferred to fix them separately.
> 
> This patch is also very similar to one Daniel Trautmann submitted [3], with
> the biggest difference being that Daniel's patch uses the new priv->phy_node
> field and of_node_get(). While this seems like a better path overall it
> does not work if more than once CPSW slave is used, due to struct cpsw_priv
> being shared with all the slaves. I am under the impression that phy_node
> should really be in struct cpsw_slave instead, but I will leave that for
> another patch.
A lot of people will need both slaves.
...
> (Side note: This is my first patch submission, and any feedback on things
> to do differently in the future would be appreciated.)

> 
>  drivers/net/ethernet/ti/cpsw.c | 65
> +++++++++++++++++++++++++----------------- 1 file changed, 39
> insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 48b92c9..ba8d086 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -26,6 +26,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/net_tstamp.h>
>  #include <linux/phy.h>
> +#include <linux/phy_fixed.h>

The prototypes for of_*_fixed_link are in linux/of_mdio.h, which is already included.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Rivshin (Allworx) Dec. 14, 2015, 6:04 p.m. UTC | #2
On Sat, 12 Dec 2015 16:44:19 +0100
Markus Brunner <systemprogrammierung.brunner@gmail.com> wrote:

> On Wednesday 09 December 2015 22:31:15 David Rivshin wrote:
> ...
> > This patch was originally developed in parallel with 1f71e8c96fc6 to
> > accomplish the same goal. When I replaced this patch with
> > 1f71e8c96fc6, I found that it did not work on my hardware (which
> > uses RGMII), so I am submitting this patch now as a bugfix. 
> 
> Your patch works fine on my board, which uses MII and dual_emac with
> a fixed_phy and a real one. 

Thanks for checking. The only dual_emac board I have available is the 
EVMSK, which has two real PHYs. I'm not sure of the usual etiquette 
(and Google was  unhelpful), should I add a Tested-by on the next 
version?

> > Besides fixing the bug mentioned in the commit log, there are a few
> > other differences to note:
> >  * If both "phy_id" and a "fixed-link" subnode are present this
> > patch will use the "phy_id" property. This should preserve current
> > behavior with existing devicetrees that might incorrectly have
> > both. This motivates the biggest difference in code organization
> > from 1f71e8c96fc6.
> >  * Some error messages have been tweaked to reflect the slightly
> > changed meanings of the checks.
> 
> I wanted to keep changes small and didn't spend too much thinking
> about already broken devicetrees. Since my patch is quite new, I

I'm honestly not sure it's an important consideration myself. Most
patches I've seen in this area for this or other drivers do not take 
such behavior into account (e.g. the phy-handle parsing that went in 
to cpsw in 4.3).
I would generally feel more comfortable with such a behavior tweak
(minor as it is) before 4.4 is released, to avoid ping-ponging the
behavior. But given how far along the cycle is, I'm not sure about 
the chances of that.

> don't see any problems with subtle changes like that. However you
> should update the documentation as well. 

Your patch already updated .../bindings/net/cpsw.txt, which this
patch left alone. Are you referring to some other documentation, 
or do you think I should update the binding documentation to state
that phy_id takes precedence over fixed-link? I figured that such
devicetrees were still officially malformed, so I thought the 
existing text was appropriate.

> >  * of_node_get() is called on slave_node before passing the result
> > to of_phy_find_device(). This increments the reference count, which
> > I believe is correct for this situation, but I'm not certain of
> > that.
> I think you are right. At least most other drivers calling
> of_phy_register_fixed_link(), call of_node_get afterwards. I can't
> remember where I got my "inspiration" for the patch. I made it almost
> a year ago and now 3 other guys tinker with the same feature? What a
> coincidence ;-)

Perhaps not a complete coincidence. I first wrote this patch a few 
months ago against 4.1. While I had always intended on submitting when 
time allowed, testing your patch gave me an extra push.

[...]

> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/netdevice.h>
> >  #include <linux/net_tstamp.h>
> >  #include <linux/phy.h>
> > +#include <linux/phy_fixed.h>
> 
> The prototypes for of_*_fixed_link are in linux/of_mdio.h, which is
> already included.

You are correct; I forget why I had originally included that. I will 
remove it for v2.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Markus Brunner Dec. 16, 2015, 6:39 a.m. UTC | #3
On Monday 14 December 2015 13:04:46 David Rivshin wrote:
> On Sat, 12 Dec 2015 16:44:19 +0100
...
> > Your patch works fine on my board, which uses MII and dual_emac with
> > a fixed_phy and a real one.
> 
> Thanks for checking. The only dual_emac board I have available is the
> EVMSK, which has two real PHYs. I'm not sure of the usual etiquette
> (and Google was  unhelpful), should I add a Tested-by on the next
> version?
> 
Yes you can. Documentation/SubmittingPatches has some notes about it.

> > > Besides fixing the bug mentioned in the commit log, there are a few
> > > 
> > > other differences to note:
> > >  * If both "phy_id" and a "fixed-link" subnode are present this
> > > 
> > > patch will use the "phy_id" property. This should preserve current
> > > behavior with existing devicetrees that might incorrectly have
> > > both. This motivates the biggest difference in code organization
> > > from 1f71e8c96fc6.
> > > 
> > >  * Some error messages have been tweaked to reflect the slightly
> > > 
> > > changed meanings of the checks.
> > 
> > I wanted to keep changes small and didn't spend too much thinking
> > about already broken devicetrees. Since my patch is quite new, I
> 
> I'm honestly not sure it's an important consideration myself. Most
> patches I've seen in this area for this or other drivers do not take
> such behavior into account (e.g. the phy-handle parsing that went in
> to cpsw in 4.3).
> I would generally feel more comfortable with such a behavior tweak
> (minor as it is) before 4.4 is released, to avoid ping-ponging the
> behavior. But given how far along the cycle is, I'm not sure about
> the chances of that.
> 

Well I don't think compatibility for flawed DTs is such a high priority, especially if it is that unlikely that there are some affected.
Keep the focus on the other _real_ problems you have encountered and fix those like you see fit. 

> > don't see any problems with subtle changes like that. However you
> > should update the documentation as well.
> 
> Your patch already updated .../bindings/net/cpsw.txt, which this
> patch left alone. Are you referring to some other documentation,
> or do you think I should update the binding documentation to state
> that phy_id takes precedence over fixed-link? I figured that such
> devicetrees were still officially malformed, so I thought the
> existing text was appropriate.

"Either the properties phy_id and phy-mode, or the sub-node fixed-link can be specified"
One flaw of my patch was to ignore the phy-mode for a fixed link. Do not mention the precedence of the phy_id, because it is an undefined behavior.
Your patch should change it to:
"Either the property phy_id, or the sub-node fixed-link can be specified"


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Rivshin (Allworx) Dec. 17, 2015, 4:02 a.m. UTC | #4
Commit 1f71e8c96fc654724723ce987e0a8b2aeb81746d ("drivers: net: cpsw:
Add support for fixed-link PHY") added initial fixed-link PHY support
for CPSW, but missed a few considerations.

This series is based on the tip of the net tree. The first two patches
fix user-visible errors in different hardware configurations. The third
patch is for an internal reference counting issue. They are logically
independent changes, but in the same function, so must be applied in
order to apply cleanly.

The first patch was originally submitted by Pascal Speck on December 4,
but was not picked up by patchwork. I suspect that is because the patch
was mangled by the mailer. I fixed the mangling and am including it in
this series, as I believe it is the correct change.

I have tested on the following hardware configurations:
 - (EVMSK) dual emac with two real MDIO-connected phys using RGMII-TXID
 - single emac with fixed-link using RGMII
Testing of other CPSW emac configurations that folks may have would
be appreciated.


Changes from v1 [1]:
 - Split into 3 smaller patches.
 - Maintain 1f71e8c96fc6's preference for fixed-link over phy_id if
   they are both (incorrectly) specified in the slave node.
 - Update binding documentation to no longer say that phy_mode is also
   mutually exclusive with fixed-link.
 - Dropped unnecessary include of phy_fixed.h.

[1] https://patchwork.ozlabs.org/patch/554989/

David Rivshin (2):
  drivers: net: cpsw: fix RMII/RGMII mode when used with fixed-link PHY
  drivers: net: cpsw: increment reference count on fixed-link PHY node

Pascal Speck (Iktek) (1):
  ethernet:ti:cpsw: fix phy identification with multiple slaves on
    fixed-phy

 Documentation/devicetree/bindings/net/cpsw.txt |  6 +--
 drivers/net/ethernet/ti/cpsw.c                 | 53 +++++++++++++++-----------
 2 files changed, 34 insertions(+), 25 deletions(-)

--
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Rivshin (Allworx) Dec. 17, 2015, 5:04 a.m. UTC | #5
On Wed, 16 Dec 2015 07:39:16 +0100
Markus Brunner <systemprogrammierung.brunner@gmail.com> wrote:

> On Monday 14 December 2015 13:04:46 David Rivshin wrote:
> > On Sat, 12 Dec 2015 16:44:19 +0100
> ...
> > > Your patch works fine on my board, which uses MII and dual_emac
> > > with a fixed_phy and a real one.
> > 
> > Thanks for checking. The only dual_emac board I have available is
> > the EVMSK, which has two real PHYs. I'm not sure of the usual
> > etiquette (and Google was  unhelpful), should I add a Tested-by on
> > the next version?
> > 
> Yes you can. Documentation/SubmittingPatches has some notes about it.

Thanks, I didn't want to throw it on without permission. Although due 
to the non-trivial change I mention below, I figured that the previous 
testing wasn't totally valid anymore anyways, so I left it off the v2 
emails.

> > > I wanted to keep changes small and didn't spend too much thinking
> > > about already broken devicetrees. Since my patch is quite new, I
> > 
> > I'm honestly not sure it's an important consideration myself. Most
> > patches I've seen in this area for this or other drivers do not take
> > such behavior into account (e.g. the phy-handle parsing that went in
> > to cpsw in 4.3).
> > I would generally feel more comfortable with such a behavior tweak
> > (minor as it is) before 4.4 is released, to avoid ping-ponging the
> > behavior. But given how far along the cycle is, I'm not sure about
> > the chances of that.
> 
> Well I don't think compatibility for flawed DTs is such a high
> priority, especially if it is that unlikely that there are some
> affected. Keep the focus on the other _real_ problems you have
> encountered and fix those like you see fit. 

Since there's been no indication from anyone that being nice to such
broken DTs is desired, I decided to drop that aspect of the patch and 
leave the current 4.4-rc1..5 behavior. This also made it much more 
reasonable to chop up the patch into smaller pieces, which I think will 
be easier to review.

> > > don't see any problems with subtle changes like that. However you
> > > should update the documentation as well.
> > 
> > Your patch already updated .../bindings/net/cpsw.txt, which this
> > patch left alone. Are you referring to some other documentation,
> > or do you think I should update the binding documentation to state
> > that phy_id takes precedence over fixed-link? I figured that such
> > devicetrees were still officially malformed, so I thought the
> > existing text was appropriate.
> 
> "Either the properties phy_id and phy-mode, or the sub-node
> fixed-link can be specified" One flaw of my patch was to ignore the
> phy-mode for a fixed link. Do not mention the precedence of the
> phy_id, because it is an undefined behavior. Your patch should change
> it to: "Either the property phy_id, or the sub-node fixed-link can be
> specified"

Thanks for pointing that out. For some reason my brain skipped over the 
"and phy-mode" part. Fixed in v2.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Dec. 17, 2015, 8:45 p.m. UTC | #6
From: "David Rivshin (Allworx)" <drivshin.allworx@gmail.com>
Date: Wed, 16 Dec 2015 23:02:08 -0500

> I have tested on the following hardware configurations:
>  - (EVMSK) dual emac with two real MDIO-connected phys using RGMII-TXID
>  - single emac with fixed-link using RGMII
> Testing of other CPSW emac configurations that folks may have would
> be appreciated.

I'm going to wait until some others give some feedback and testing
results on this one, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Trautmann Dec. 18, 2015, 10:20 a.m. UTC | #7
On Thu, Dec 17, 2015 at 03:45:08PM -0500, David Miller wrote:
> From: "David Rivshin (Allworx)" <drivshin.allworx@gmail.com>
> Date: Wed, 16 Dec 2015 23:02:08 -0500
> 
> > I have tested on the following hardware configurations:
> >  - (EVMSK) dual emac with two real MDIO-connected phys using RGMII-TXID
> >  - single emac with fixed-link using RGMII
> > Testing of other CPSW emac configurations that folks may have would
> > be appreciated.
> 
> I'm going to wait until some others give some feedback and testing
> results on this one, thanks.

I can confirm that this patch is working on the following hardware setup:
 - single emac using MII connected with fixed-link to Micrel KSZ8895 Switch
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Dec. 18, 2015, 7:46 p.m. UTC | #8
From: "David Rivshin (Allworx)" <drivshin.allworx@gmail.com>
Date: Wed, 16 Dec 2015 23:02:08 -0500

> Commit 1f71e8c96fc654724723ce987e0a8b2aeb81746d ("drivers: net: cpsw:
> Add support for fixed-link PHY") added initial fixed-link PHY support
> for CPSW, but missed a few considerations.
> 
> This series is based on the tip of the net tree. The first two patches
> fix user-visible errors in different hardware configurations. The third
> patch is for an internal reference counting issue. They are logically
> independent changes, but in the same function, so must be applied in
> order to apply cleanly.
> 
> The first patch was originally submitted by Pascal Speck on December 4,
> but was not picked up by patchwork. I suspect that is because the patch
> was mangled by the mailer. I fixed the mangling and am including it in
> this series, as I believe it is the correct change.
> 
> I have tested on the following hardware configurations:
>  - (EVMSK) dual emac with two real MDIO-connected phys using RGMII-TXID
>  - single emac with fixed-link using RGMII
> Testing of other CPSW emac configurations that folks may have would
> be appreciated.

Series applied, thanks David.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Rivshin (Allworx) Dec. 18, 2015, 10:06 p.m. UTC | #9
On Fri, 18 Dec 2015 11:20:21 +0100
Daniel Trautmann <dtrautmann@ibhsoftec.com> wrote:

> On Thu, Dec 17, 2015 at 03:45:08PM -0500, David Miller wrote:
> > From: "David Rivshin (Allworx)" <drivshin.allworx@gmail.com>
> > Date: Wed, 16 Dec 2015 23:02:08 -0500
> > 
> > > I have tested on the following hardware configurations:
> > >  - (EVMSK) dual emac with two real MDIO-connected phys using
> > > RGMII-TXID
> > >  - single emac with fixed-link using RGMII
> > > Testing of other CPSW emac configurations that folks may have
> > > would be appreciated.
> > 
> > I'm going to wait until some others give some feedback and testing
> > results on this one, thanks.
> 
> I can confirm that this patch is working on the following hardware
> setup:
>  - single emac using MII connected with fixed-link to Micrel KSZ8895
> Switch

Thanks for testing.

I see that Dave applied the series, but just for good measure I'll 
report that I also tested on:
 - (BeagleBone-Black) single emac, MII, real 100Mbit PHY

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 48b92c9..ba8d086 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -26,6 +26,7 @@ 
 #include <linux/netdevice.h>
 #include <linux/net_tstamp.h>
 #include <linux/phy.h>
+#include <linux/phy_fixed.h>
 #include <linux/workqueue.h>
 #include <linux/delay.h>
 #include <linux/pm_runtime.h>
@@ -2026,45 +2027,57 @@  static int cpsw_probe_dt(struct cpsw_priv *priv,
 	for_each_child_of_node(node, slave_node) {
 		struct cpsw_slave_data *slave_data = data->slave_data + i;
 		const void *mac_addr = NULL;
-		u32 phyid;
 		int lenp;
 		const __be32 *parp;
-		struct device_node *mdio_node;
-		struct platform_device *mdio;
 
 		/* This is no slave child node, continue */
 		if (strcmp(slave_node->name, "slave"))
 			continue;
 
 		priv->phy_node = of_parse_phandle(slave_node, "phy-handle", 0);
-		if (of_phy_is_fixed_link(slave_node)) {
-			struct phy_device *pd;
+		parp = of_get_property(slave_node, "phy_id", &lenp);
+
+		if (parp) {
+			u32 phyid;
+			struct device_node *mdio_node;
+			struct platform_device *mdio;
 
+			if (lenp != (sizeof(__be32) * 2)) {
+				dev_err(&pdev->dev, "Invalid slave[%d] phy_id property\n", i);
+				goto no_phy_slave;
+			}
+
+			mdio_node = of_find_node_by_phandle(be32_to_cpup(parp));
+			phyid = be32_to_cpup(parp+1);
+			mdio = of_find_device_by_node(mdio_node);
+			of_node_put(mdio_node);
+			if (!mdio) {
+				dev_err(&pdev->dev, "Missing mdio platform device\n");
+				return -EINVAL;
+			}
+			snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
+				 PHY_ID_FMT, mdio->name, phyid);
+		} else if (of_phy_is_fixed_link(slave_node)) {
+			struct device_node *phy_node;
+			struct phy_device *phy_dev;
+
+			/* In the case of a fixed PHY, the DT node associated
+			 * to the PHY is the Ethernet MAC DT node.
+			 */
 			ret = of_phy_register_fixed_link(slave_node);
-			if (ret)
-				return ret;
-			pd = of_phy_find_device(slave_node);
-			if (!pd)
-				return -ENODEV;
+			if (ret) {
+				dev_err(&pdev->dev, "Cannot register fixed PHY\n");
+				goto no_phy_slave;
+			}
+			phy_node = of_node_get(slave_node);
+			phy_dev = of_phy_find_device(phy_node);
 			snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
-				 PHY_ID_FMT, pd->bus->id, pd->phy_id);
-			goto no_phy_slave;
-		}
-		parp = of_get_property(slave_node, "phy_id", &lenp);
-		if ((parp == NULL) || (lenp != (sizeof(void *) * 2))) {
-			dev_err(&pdev->dev, "Missing slave[%d] phy_id property\n", i);
+				PHY_ID_FMT, phy_dev->bus->id, phy_dev->addr);
+		} else {
+			dev_err(&pdev->dev, "No slave[%d] phy_id or fixed-link property\n", i);
 			goto no_phy_slave;
 		}
-		mdio_node = of_find_node_by_phandle(be32_to_cpup(parp));
-		phyid = be32_to_cpup(parp+1);
-		mdio = of_find_device_by_node(mdio_node);
-		of_node_put(mdio_node);
-		if (!mdio) {
-			dev_err(&pdev->dev, "Missing mdio platform device\n");
-			return -EINVAL;
-		}
-		snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
-			 PHY_ID_FMT, mdio->name, phyid);
+
 		slave_data->phy_if = of_get_phy_mode(slave_node);
 		if (slave_data->phy_if < 0) {
 			dev_err(&pdev->dev, "Missing or malformed slave[%d] phy-mode property\n",