| Message ID | 20201104160847.30049-1-TheSven73@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v1] lan743x: correctly handle chips with internal PHY | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Guessed tree name to be net-next |
| jkicinski/subject_prefix | warning | Target tree name not specified in the subject |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | warning | WARNING: From:/Signed-off-by: email address mismatch: 'From: Sven Van Asbroeck <thesven73@gmail.com>' != 'Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>' |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
> 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
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?
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
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.
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
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?
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 --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;
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(-)