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 <[email protected]> # lan7430
Signed-off-by: Sven Van Asbroeck <[email protected]>
---
Tree: v5.10-rc2
To: Bryan Whitehead <[email protected]>
To: "David S. Miller" <[email protected]>
To: Jakub Kicinski <[email protected]>
Cc: Microchip Linux Driver Support <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Roelof Berg <[email protected]>
Cc: [email protected]
Cc: [email protected]
drivers/net/ethernet/microchip/lan743x_main.c | 32 ++++++-------------
drivers/net/ethernet/microchip/lan743x_main.h | 1 -
2 files changed, 9 insertions(+), 24 deletions(-)
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;
--
2.17.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
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 <[email protected]> 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
Hi Andrew, many thanks for looking at this patch !
On Wed, Nov 4, 2020 at 11:27 AM Andrew Lunn <[email protected]> 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,
On Wed, Nov 4, 2020 at 11:55 AM Andrew Lunn <[email protected]> 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
On Wed, 4 Nov 2020 11:08:47 -0500 Sven Van Asbroeck wrote:
> Tested-by: Sven Van Asbroeck <[email protected]> # lan7430
> Signed-off-by: Sven Van Asbroeck <[email protected]>
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?
Hi Jakub,
On Wed, Nov 4, 2020 at 4:58 PM Jakub Kicinski <[email protected]> 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.
On Wed, Nov 4, 2020 at 11:55 AM Andrew Lunn <[email protected]> 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.