Message ID | 20200529193003.3717-1-rberg@berg-solutions.de |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | lan743x: Added fixed link and RGMII support | expand |
On Fri, May 29, 2020 at 09:30:02PM +0200, Roelof Berg wrote: > Microchip lan7431 is frequently connected to a phy. However, it > can also be directly connected to a MII remote peer without > any phy in between. For supporting such a phyless hardware setup > in Linux we utilized phylib, which supports a fixed-link > configuration via the device tree. And we added support for > defining the connection type R/GMII in the device tree. > > New behavior: > ------------- > . The automatic speed and duplex detection of the lan743x silicon > between mac and phy is disabled. Instead phylib is used like in > other typical Linux drivers. The usage of phylib allows to > specify fixed-link parameters in the device tree. > > . The device tree entry phy-connection-type is supported now with > the modes RGMII or (G)MII (default). > > Development state: > ------------------ > . Tested with fixed-phy configurations. Not yet tested in normal > configurations with phy. Microchip kindly offered testing > as soon as the Corona measures allow this. > > . All review findings of Andrew Lunn are included > > Example: > -------- > &pcie { > status = "okay"; > > host@0 { > reg = <0 0 0 0 0>; > > #address-cells = <3>; > #size-cells = <2>; > > ethernet@0 { > compatible = "weyland-yutani,noscom1", "microchip,lan743x"; > status = "okay"; > reg = <0 0 0 0 0>; > phy-connection-type = "rgmii"; > > fixed-link { > speed = <100>; > full-duplex; > }; > }; > }; > }; > > Signed-off-by: Roelof Berg <rberg@berg-solutions.de> Hi Roelof It looks like you took my suggestion as a basis. So i should give you: Signed-off-by: Andrew Lunn <andrew@lunn.ch> since i did not include one in my posting. I'm happy to see it helped, and fixed some other issues you were seeing. Andrew
From: Roelof Berg <rberg@berg-solutions.de> Date: Fri, 29 May 2020 21:30:02 +0200 > Microchip lan7431 is frequently connected to a phy. However, it > can also be directly connected to a MII remote peer without > any phy in between. For supporting such a phyless hardware setup > in Linux we utilized phylib, which supports a fixed-link > configuration via the device tree. And we added support for > defining the connection type R/GMII in the device tree. ... Applied, thank you.
TEST REPORT: BROKEN PATCH Thanks to everyone for working on the fixed link feature of lan743x eth driver. I received more test hardware today, and one piece of hardware (EVBlan7430) becomes incompatible by the patch. We need to roll back for now. Sorry. I’ll discuss about options of how to proceed in a second e-mail. Thank you and best regards, Roelof > David Miller <davem@davemloft.net>: > >> Microchip lan7431 is frequently connected to a phy. However, it >> can also be directly connected to a MII remote peer without >> any phy in between. For supporting such a phyless hardware setup >> in Linux we utilized phylib, which supports a fixed-link >> configuration via the device tree. And we added support for >> defining the connection type R/GMII in the device tree. > ... > > Applied, thank you. >
Hi, we need to decide how to continue with the lan743x patch for fixed phy support. Thanks everyone for the valuable time. Summary of the development steps so far: The patch initially had low influence on the installed base, but lacked some compatibility to Linux best practices. During the review process we found improvement potential. We decided to make the patch more similar to the majority of Linux drivers, by avoiding a special autodetection silicon feature that is unique. However, now, with more test hardware available, this road becomes more difficult than we initially assumed. Would it be ok to split the topics into two distinct patches ? Topic a: New feature: Add fixed link and RGMII support in a minimal invasive way. (Including fixing a reboot-issue that the last patch fixed.) Topic b: Refactor old driver: Understand, and if possible remove, the silicon’s Mac-Phy auto-negotiation feature. Maybe splitting distinct features into distinct patches is the most defensive approach, now that device testing disconfirmed our promising approach ? Thanks everyone for cooperating on this feature, Roelof > Roelof Berg <rberg@berg-solutions.de>: > > Testing note [...] > - Different from prior approaches it affects the installed base. So we need some more testing (with hardware I don’t have available yet) and I’m in contact with Microchip already. > > Thanks for guiding us to a proper solution, > Roelof Berg > >> . The automatic speed and duplex detection of the lan743x silicon >> between mac and phy is disabled.
On Wed, Jun 03, 2020 at 04:52:32PM +0200, Roelof Berg wrote: > TEST REPORT: BROKEN PATCH > > Thanks to everyone for working on the fixed link feature of lan743x eth driver. > > I received more test hardware today, and one piece of hardware > (EVBlan7430) becomes incompatible by the patch. We need to roll back > for now. Sorry. Hi Roelof We have a bit of time to fix this, before it becomes too critical. So lets try to fix it. How did it break? Thanks Andrew
Ok, let's proceed :) The code runs well, dmesg looks good, ip addr shows me a link up, speed/duplex looks ok. But it does not transfer any data. Debugging steps (A/B versions): - Check clocks with oscilloscope (10/100/1000) - Dump actual register settings - Trace Phy-Phy autonegotiation and ensure that our patch catches up the result Research topic: I will also look if in my test hardware uses distinct MDIO traces or in-band MDIO (MDIO via MII). Would inband MDIO need the autosense feature to talk to the phy before phy-phy auto negotiation is complete ? E.g. for triggering phy-phy autoneg ? And is this (inband mdio) maybe the reason why the may-phy autosense feature exists in the silicon ? Thanks, Roelof > Am 03.06.2020 um 17:59 schrieb Andrew Lunn <andrew@lunn.ch>: > > On Wed, Jun 03, 2020 at 04:52:32PM +0200, Roelof Berg wrote: >> TEST REPORT: BROKEN PATCH >> >> Thanks to everyone for working on the fixed link feature of lan743x eth driver. >> > >> I received more test hardware today, and one piece of hardware >> (EVBlan7430) becomes incompatible by the patch. We need to roll back >> for now. Sorry. > > Hi Roelof > > We have a bit of time to fix this, before it becomes too critical. > So lets try to fix it. > > How did it break? > > Thanks > Andrew >
If I find a fix, would I need to submit a delta patch (to our last one) or a full patch ? Thanks. > So lets try to fix it. > > Thanks > Andrew >
On Wed, Jun 03, 2020 at 06:36:28PM +0200, Roelof Berg wrote:
> If I find a fix, would I need to submit a delta patch (to our last one) or a full patch ?
A delta.
Andrew
On Wed, Jun 03, 2020 at 06:33:28PM +0200, Roelof Berg wrote: > Ok, let's proceed :) The code runs well, dmesg looks good, ip addr shows me a link up, speed/duplex looks ok. But it does not transfer any data. > > Debugging steps (A/B versions): > - Check clocks with oscilloscope (10/100/1000) > - Dump actual register settings > - Trace Phy-Phy autonegotiation and ensure that our patch catches up the result Adding #define DEBUG to the top of drivers/net/phy/phy.c will get you some debug info. What PHY is being used? Is it using RGMII? If it looks like the PHY and the MAC are happy, just that frames are not being transferred between them, it could be RGMII delays. Andrew
I’m testing with Microchip Lan7430, which is an integrated circuit that contains MAC and PHY in one package. With the release kernel the hardware works fine, so the overall configuration is ok (jumpers). I will verify wether the effective RGMII and delay settings, you mention, are equal in both driver versions. >> Ok, let's proceed :) > What PHY is being used? > Is it using RGMII? > > If it looks like the PHY and the MAC are happy, just that frames are > not being transferred between them, it could be RGMII delays. > > Andrew >
Thanks, I found the cause, the delta patch is submitted. Lan7430 runs now in different speeds, also ‚hot swap‘ between different speeds works well. Normal mode and fixed-phy mode coexist properly. So, I guess we’re done :) However, I’ll spend some more time testing to ensure we’re really safe. (I’m sending this e-mail through a Microchip Lan7430 by the way :) Roelof Berg > > Adding #define DEBUG to the top of drivers/net/phy/phy.c will get you > some debug info. > > […] > Andrew >
diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c index 3a0b289d9771..c533d06fbe3a 100644 --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c @@ -2,11 +2,11 @@ /* Copyright (C) 2018 Microchip Technology Inc. */ #include <linux/netdevice.h> -#include "lan743x_main.h" -#include "lan743x_ethtool.h" #include <linux/net_tstamp.h> #include <linux/pci.h> #include <linux/phy.h> +#include "lan743x_main.h" +#include "lan743x_ethtool.h" /* eeprom */ #define LAN743X_EEPROM_MAGIC (0x74A5) diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c index a43140f7b5eb..36624e3c633b 100644 --- a/drivers/net/ethernet/microchip/lan743x_main.c +++ b/drivers/net/ethernet/microchip/lan743x_main.c @@ -8,7 +8,10 @@ #include <linux/crc32.h> #include <linux/microchipphy.h> #include <linux/net_tstamp.h> +#include <linux/of_mdio.h> +#include <linux/of_net.h> #include <linux/phy.h> +#include <linux/phy_fixed.h> #include <linux/rtnetlink.h> #include <linux/iopoll.h> #include <linux/crc16.h> @@ -798,9 +801,9 @@ static int lan743x_mac_init(struct lan743x_adapter *adapter) netdev = adapter->netdev; - /* setup auto duplex, and speed detection */ + /* disable auto duplex, and speed detection. Phylib does that */ data = lan743x_csr_read(adapter, MAC_CR); - data |= MAC_CR_ADD_ | MAC_CR_ASD_; + data &= ~(MAC_CR_ADD_ | MAC_CR_ASD_); data |= MAC_CR_CNTR_RST_; lan743x_csr_write(adapter, MAC_CR, data); @@ -946,6 +949,7 @@ static void lan743x_phy_link_status_change(struct net_device *netdev) { struct lan743x_adapter *adapter = netdev_priv(netdev); struct phy_device *phydev = netdev->phydev; + u32 data; phy_print_status(phydev); if (phydev->state == PHY_RUNNING) { @@ -953,6 +957,39 @@ static void lan743x_phy_link_status_change(struct net_device *netdev) int remote_advertisement = 0; int local_advertisement = 0; + data = lan743x_csr_read(adapter, MAC_CR); + + /* set interface mode */ + if (phy_interface_mode_is_rgmii(adapter->phy_mode)) + /* RGMII */ + data &= ~MAC_CR_MII_EN_; + else + /* GMII */ + data |= MAC_CR_MII_EN_; + + /* set duplex mode */ + if (phydev->duplex) + data |= MAC_CR_DPX_; + else + data &= ~MAC_CR_DPX_; + + /* set bus speed */ + switch (phydev->speed) { + case SPEED_10: + data &= ~MAC_CR_CFG_H_; + data &= ~MAC_CR_CFG_L_; + break; + case SPEED_100: + data &= ~MAC_CR_CFG_H_; + data |= MAC_CR_CFG_L_; + break; + case SPEED_1000: + data |= MAC_CR_CFG_H_; + data |= MAC_CR_CFG_L_; + break; + } + lan743x_csr_write(adapter, MAC_CR, data); + memset(&ksettings, 0, sizeof(ksettings)); phy_ethtool_get_link_ksettings(netdev, &ksettings); local_advertisement = @@ -980,20 +1017,44 @@ static void lan743x_phy_close(struct lan743x_adapter *adapter) static int lan743x_phy_open(struct lan743x_adapter *adapter) { struct lan743x_phy *phy = &adapter->phy; + struct device_node *phynode; struct phy_device *phydev; struct net_device *netdev; int ret = -EIO; netdev = adapter->netdev; - phydev = phy_find_first(adapter->mdiobus); - if (!phydev) - goto return_error; + 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 { + phydev = phy_find_first(adapter->mdiobus); + if (!phydev) + goto return_error; - ret = phy_connect_direct(netdev, phydev, - lan743x_phy_link_status_change, - PHY_INTERFACE_MODE_GMII); - if (ret) - goto return_error; + ret = phy_connect_direct(netdev, phydev, + lan743x_phy_link_status_change, + adapter->phy_mode); + if (ret) + goto return_error; + } /* MAC doesn't support 1000T Half */ phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT); diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h index 3b02eeae5f45..c61a40411317 100644 --- a/drivers/net/ethernet/microchip/lan743x_main.h +++ b/drivers/net/ethernet/microchip/lan743x_main.h @@ -4,6 +4,7 @@ #ifndef _LAN743X_H #define _LAN743X_H +#include <linux/phy.h> #include "lan743x_ptp.h" #define DRIVER_AUTHOR "Bryan Whitehead <Bryan.Whitehead@microchip.com>" @@ -104,10 +105,14 @@ ((value << 0) & FCT_FLOW_CTL_ON_THRESHOLD_) #define MAC_CR (0x100) +#define MAC_CR_MII_EN_ BIT(19) #define MAC_CR_EEE_EN_ BIT(17) #define MAC_CR_ADD_ BIT(12) #define MAC_CR_ASD_ BIT(11) #define MAC_CR_CNTR_RST_ BIT(5) +#define MAC_CR_DPX_ BIT(3) +#define MAC_CR_CFG_H_ BIT(2) +#define MAC_CR_CFG_L_ BIT(1) #define MAC_CR_RST_ BIT(0) #define MAC_RX (0x104) @@ -698,6 +703,7 @@ 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; diff --git a/drivers/net/ethernet/microchip/lan743x_ptp.c b/drivers/net/ethernet/microchip/lan743x_ptp.c index 9399f6a98748..ab6d719d40f0 100644 --- a/drivers/net/ethernet/microchip/lan743x_ptp.c +++ b/drivers/net/ethernet/microchip/lan743x_ptp.c @@ -2,12 +2,12 @@ /* Copyright (C) 2018 Microchip Technology Inc. */ #include <linux/netdevice.h> -#include "lan743x_main.h" #include <linux/ptp_clock_kernel.h> #include <linux/module.h> #include <linux/pci.h> #include <linux/net_tstamp.h> +#include "lan743x_main.h" #include "lan743x_ptp.h"
Microchip lan7431 is frequently connected to a phy. However, it can also be directly connected to a MII remote peer without any phy in between. For supporting such a phyless hardware setup in Linux we utilized phylib, which supports a fixed-link configuration via the device tree. And we added support for defining the connection type R/GMII in the device tree. New behavior: ------------- . The automatic speed and duplex detection of the lan743x silicon between mac and phy is disabled. Instead phylib is used like in other typical Linux drivers. The usage of phylib allows to specify fixed-link parameters in the device tree. . The device tree entry phy-connection-type is supported now with the modes RGMII or (G)MII (default). Development state: ------------------ . Tested with fixed-phy configurations. Not yet tested in normal configurations with phy. Microchip kindly offered testing as soon as the Corona measures allow this. . All review findings of Andrew Lunn are included Example: -------- &pcie { status = "okay"; host@0 { reg = <0 0 0 0 0>; #address-cells = <3>; #size-cells = <2>; ethernet@0 { compatible = "weyland-yutani,noscom1", "microchip,lan743x"; status = "okay"; reg = <0 0 0 0 0>; phy-connection-type = "rgmii"; fixed-link { speed = <100>; full-duplex; }; }; }; }; Signed-off-by: Roelof Berg <rberg@berg-solutions.de> --- .../net/ethernet/microchip/lan743x_ethtool.c | 4 +- drivers/net/ethernet/microchip/lan743x_main.c | 81 ++++++++++++++++--- drivers/net/ethernet/microchip/lan743x_main.h | 6 ++ drivers/net/ethernet/microchip/lan743x_ptp.c | 2 +- 4 files changed, 80 insertions(+), 13 deletions(-)