diff mbox series

[v1] lan743x: correctly handle chips with internal PHY

Message ID 20201104160847.30049-1-TheSven73@gmail.com
State New
Headers show
Series [v1] lan743x: correctly handle chips with internal PHY | expand

Commit Message

Sven Van Asbroeck Nov. 4, 2020, 4:08 p.m. UTC
Commit 6f197fb63850 ("lan743x: Added fixed link and RGMII support")
assumes that chips with an internal PHY will not have a devicetree
entry. This is incorrect: even for these chips, a devicetree entry
can be useful e.g. to pass the mac address from bootloader to chip:

    &pcie {
            status = "okay";

            host@0 {
                    reg = <0 0 0 0 0>;

                    #address-cells = <3>;
                    #size-cells = <2>;

                    lan7430: ethernet@0 {
                            /* LAN7430 with internal PHY */
                            compatible = "microchip,lan743x";
                            status = "okay";
                            reg = <0 0 0 0 0>;
                            /* filled in by bootloader */
                            local-mac-address = [00 00 00 00 00 00];
                    };
            };
    };

If a devicetree entry is present, the driver will not attach the chip
to its internal phy, and the chip will be non-operational.

Fix by tweaking the phy connection algorithm:
- first try to connect to a phy specified in the devicetree
  (could be 'real' phy, or just a 'fixed-link')
- if that doesn't succeed, try to connect to an internal phy, even
  if the chip has a devnode

This method no longer explicitly exposes the phy mode, but we can
get around that by querying the phy mode from the phydev. The
phy_mode member in the adapter private struct can then be removed.

Note that as a side-effect, the devicetree phy mode now no longer
has a default, and always needs to be specified explicitly (via
'phy-connection-type').

Tested on a LAN7430 with internal PHY. I cannot test a device using
fixed-link, as I do not have access to one.

Fixes: 6f197fb63850 ("lan743x: Added fixed link and RGMII support")
Tested-by: Sven Van Asbroeck <TheSven73@gmail.com> # lan7430
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---

Tree: v5.10-rc2

To: Bryan Whitehead <bryan.whitehead@microchip.com>
To: "David S. Miller" <davem@davemloft.net>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Roelof Berg <rberg@berg-solutions.de>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

 drivers/net/ethernet/microchip/lan743x_main.c | 32 ++++++-------------
 drivers/net/ethernet/microchip/lan743x_main.h |  1 -
 2 files changed, 9 insertions(+), 24 deletions(-)

Comments

Andrew Lunn Nov. 4, 2020, 4:27 p.m. UTC | #1
> Note that as a side-effect, the devicetree phy mode now no longer

> has a default, and always needs to be specified explicitly (via

> 'phy-connection-type').


That sounds like it could break systems. Why do you do this?

     Andrew
Sven Van Asbroeck Nov. 4, 2020, 4:39 p.m. UTC | #2
Hi Andrew, many thanks for looking at this patch !

On Wed, Nov 4, 2020 at 11:27 AM Andrew Lunn <andrew@lunn.ch> wrote:
>

> > Note that as a side-effect, the devicetree phy mode now no longer

> > has a default, and always needs to be specified explicitly (via

> > 'phy-connection-type').

>

> That sounds like it could break systems. Why do you do this?


Because the standard mdio library function (of_phy_get_and_connect())
does not appear to support a default value. The original driver
code duplicated that library function's code, with a slight
tweak - the default value.

The default value was introduced quite recently, in the commit which
this patch fixes:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/net/ethernet/microchip/lan743x_main.c?h=v5.9.3&id=6f197fb63850b26ef8f70f1bfe5900e377910a5a

I'm not sure if other devices that specify phys in devicetrees have a
default for 'phy-connection-type'. I'm wondering if any do?
Andrew Lunn Nov. 4, 2020, 4:55 p.m. UTC | #3
On Wed, Nov 04, 2020 at 11:39:47AM -0500, Sven Van Asbroeck wrote:
> Hi Andrew, many thanks for looking at this patch !

> 

> On Wed, Nov 4, 2020 at 11:27 AM Andrew Lunn <andrew@lunn.ch> wrote:

> >

> > > Note that as a side-effect, the devicetree phy mode now no longer

> > > has a default, and always needs to be specified explicitly (via

> > > 'phy-connection-type').

> >

> > That sounds like it could break systems. Why do you do this?

> 

> Because the standard mdio library function (of_phy_get_and_connect())

> does not appear to support a default value. The original driver

> code duplicated that library function's code, with a slight

> tweak - the default value.

> 

> The default value was introduced quite recently, in the commit which

> this patch fixes:

> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/net/ethernet/microchip/lan743x_main.c?h=v5.9.3&id=6f197fb63850b26ef8f70f1bfe5900e377910a5a


If you look at that patch, you see:

-       ret = phy_connect_direct(netdev, phydev,
-                                lan743x_phy_link_status_change,
-                                PHY_INTERFACE_MODE_GMII);
-       if (ret)
-               goto return_error;


That was added as part of the first commit for the lan743x
driver. Changing that now seems dangerous.

This is a fix you want backporting to stable. Such fixes should be
minimal, and obviously correct. So i suggest you keep with the open
coded version of of_phy_get_and_connect(), and make sure it keeps with
the default as PHY_INTERFACE_MODE_GMII. That can be committed to net
as a fix.

You can then consider a refactoring patch for net-next, and see about
modifying of_phy_get_and_connect() to do what you need.

	  Andrew
Sven Van Asbroeck Nov. 4, 2020, 5:10 p.m. UTC | #4
On Wed, Nov 4, 2020 at 11:55 AM Andrew Lunn <andrew@lunn.ch> wrote:
>

> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/net/ethernet/microchip/lan743x_main.c?h=v5.9.3&id=6f197fb63850b26ef8f70f1bfe5900e377910a5a

>

> If you look at that patch, you see:

>

> -       ret = phy_connect_direct(netdev, phydev,

> -                                lan743x_phy_link_status_change,

> -                                PHY_INTERFACE_MODE_GMII);

> -       if (ret)

> -               goto return_error;

>

>

> That was added as part of the first commit for the lan743x

> driver. Changing that now seems dangerous.


My knowledge of netdev/phy is extremely limited, so bear with me.

The code quoted above (the first commit for lan743x) has been reinstated
in my patch. It's literally identical - in the case the kernel can't find any
available/sensible devicetree phy description.

In the case of devicetree phys, which have been added recently,
the 'phy-connection-type' prop appeared to have been optional,
defaulting to (G)MII. My patch removes that devicetree default.

So I guess my question is this - is there really a need for a
devicetree default for 'phy-connection-type'? If not, no code
duplication or mdio refactoring is required.
Sven Van Asbroeck Nov. 4, 2020, 9:52 p.m. UTC | #5
Andrew,

On Wed, Nov 4, 2020 at 11:55 AM Andrew Lunn <andrew@lunn.ch> wrote:
> If you look at that patch, you see:

>

> -       ret = phy_connect_direct(netdev, phydev,

> -                                lan743x_phy_link_status_change,

> -                                PHY_INTERFACE_MODE_GMII);

> -       if (ret)

> -               goto return_error;

>

>

> That was added as part of the first commit for the lan743x

> driver. Changing that now seems dangerous.


I think there's a misunderstanding on my part, and it flows from the
following bit of the commit message in 6f197fb63850
("lan743x: Added fixed link and RGMII support"):

>   . The device tree entry phy-connection-type is supported now with

>     the modes RGMII or (G)MII (default).


I interpreted that to mean "if phy-connection-type is omitted, it will default
to G(MII)". However I now notice that the code in that patch does no such
thing: if that prop is omitted, the mode is actually silently set to
PHY_INTERFACE_MODE_NA, which is probably not great.

In summary, 6f197fb63850 behaves as follows:
1. if a devnode is present, attempts to configure the phy from devicetree,
   but silently breaks if phy-connection-type is omitted
2. if no devicetree node present, tries to connect to an internal phy using
  (G)MII (which silently breaks if an internal phy chip has a devicenode)

This proposed patch replaces this with:
1. attempts to configure the phy from devicetree, fails if no correct devicetree
   description present (phy-connection-type is required)
2. if (1) fails, tries to connect to an internal phy using (G)MII

As far as I can see, this doesn't appear to introduce any breaking changes?
It's of course quite possible that I've overlooked something, I am definitely
not a netdev/phy expert.

Sven
Jakub Kicinski Nov. 4, 2020, 9:58 p.m. UTC | #6
On Wed,  4 Nov 2020 11:08:47 -0500 Sven Van Asbroeck wrote:
> Tested-by: Sven Van Asbroeck <TheSven73@gmail.com> # lan7430

> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>


Not a big deal but if you have to change the patch could you make sure
your email address is spelled the same in the From line and other tags?
Sven Van Asbroeck Nov. 4, 2020, 10:35 p.m. UTC | #7
Hi Jakub,

On Wed, Nov 4, 2020 at 4:58 PM Jakub Kicinski <kuba@kernel.org> wrote:
>

> Not a big deal but if you have to change the patch could you make sure

> your email address is spelled the same in the From line and other tags?


Absolutely, thanks for letting me know about those case differences.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index f2d13e8d20f0..eb990e036611 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -957,7 +957,7 @@  static void lan743x_phy_link_status_change(struct net_device *netdev)
 		data = lan743x_csr_read(adapter, MAC_CR);
 
 		/* set interface mode */
-		if (phy_interface_mode_is_rgmii(adapter->phy_mode))
+		if (phy_interface_is_rgmii(phydev))
 			/* RGMII */
 			data &= ~MAC_CR_MII_EN_;
 		else
@@ -1021,34 +1021,19 @@  static int lan743x_phy_open(struct lan743x_adapter *adapter)
 
 	netdev = adapter->netdev;
 	phynode = of_node_get(adapter->pdev->dev.of_node);
-	adapter->phy_mode = PHY_INTERFACE_MODE_GMII;
-
-	if (phynode) {
-		of_get_phy_mode(phynode, &adapter->phy_mode);
-
-		if (of_phy_is_fixed_link(phynode)) {
-			ret = of_phy_register_fixed_link(phynode);
-			if (ret) {
-				netdev_err(netdev,
-					   "cannot register fixed PHY\n");
-				of_node_put(phynode);
-				goto return_error;
-			}
-		}
-		phydev = of_phy_connect(netdev, phynode,
-					lan743x_phy_link_status_change, 0,
-					adapter->phy_mode);
-		of_node_put(phynode);
-		if (!phydev)
-			goto return_error;
-	} else {
+
+	/* try devicetree phy, or fixed link */
+	phydev = of_phy_get_and_connect(netdev, phynode,
+					lan743x_phy_link_status_change);
+	if (!phydev) {
+		/* try internal PHY */
 		phydev = phy_find_first(adapter->mdiobus);
 		if (!phydev)
 			goto return_error;
 
 		ret = phy_connect_direct(netdev, phydev,
 					 lan743x_phy_link_status_change,
-					 adapter->phy_mode);
+					 PHY_INTERFACE_MODE_GMII);
 		if (ret)
 			goto return_error;
 	}
@@ -1063,6 +1048,7 @@  static int lan743x_phy_open(struct lan743x_adapter *adapter)
 
 	phy_start(phydev);
 	phy_start_aneg(phydev);
+	phy_attached_info(phydev);
 	return 0;
 
 return_error:
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index a536f4a4994d..3a0e70daa88f 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -703,7 +703,6 @@  struct lan743x_rx {
 struct lan743x_adapter {
 	struct net_device       *netdev;
 	struct mii_bus		*mdiobus;
-	phy_interface_t		phy_mode;
 	int                     msg_enable;
 #ifdef CONFIG_PM
 	u32			wolopts;