| Message ID | 20240730140619.80650-5-Raju.Lakkaraju@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support to PHYLINK for LAN743x/PCI11x1x chips | expand |
On Tue, Jul 30, 2024 at 07:36:19PM +0530, Raju Lakkaraju wrote: > diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c > index 3a63ec091413..a649ea7442a4 100644 > --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c > +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c > @@ -1058,61 +1058,48 @@ static int lan743x_ethtool_get_eee(struct net_device *netdev, > struct ethtool_keee *eee) > { > struct lan743x_adapter *adapter = netdev_priv(netdev); > - struct phy_device *phydev = netdev->phydev; > - u32 buf; > - int ret; > - > - if (!phydev) > - return -EIO; > - if (!phydev->drv) { > - netif_err(adapter, drv, adapter->netdev, > - "Missing PHY Driver\n"); > - return -EIO; > - } > > - ret = phy_ethtool_get_eee(phydev, eee); > - if (ret < 0) > - return ret; > - > - buf = lan743x_csr_read(adapter, MAC_CR); > - if (buf & MAC_CR_EEE_EN_) { > - /* EEE_TX_LPI_REQ_DLY & tx_lpi_timer are same uSec unit */ > - buf = lan743x_csr_read(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT); > - eee->tx_lpi_timer = buf; > - } else { > - eee->tx_lpi_timer = 0; > - } > + eee->tx_lpi_timer = lan743x_csr_read(adapter, > + MAC_EEE_TX_LPI_REQ_DLY_CNT); > + eee->eee_enabled = adapter->eee_enabled; > + eee->eee_active = adapter->eee_active; > + eee->tx_lpi_enabled = adapter->tx_lpi_enabled; You really need to start paying attention to the commits other people make as a result of development to other parts of the kernel. First, see: commit ef460a8986fa0dae1cdcb158a06127f7af27c92d Author: Andrew Lunn <andrew@lunn.ch> Date: Sat Apr 6 15:16:00 2024 -0500 net: lan743x: Fixup EEE and note that the assignment of eee->eee_enabled, eee->eee_active, and eee->tx_lpi_enabled were all removed. Next... > > - return 0; > + return phylink_ethtool_get_eee(adapter->phylink, eee); phylink_ethtool_get_eee() will call phy_ethtool_get_eee(), which in turn calls genphy_c45_ethtool_get_eee() and eeecfg_to_eee(). genphy_c45_ethtool_get_eee() will do this: data->eee_enabled = is_enabled; data->eee_active = ret; thus overwriting your assignment to eee->eee_enabled and eee->eee_active. eeecfg_to_eee() will overwrite eee->tx_lpi_enabled and eee->eee_enabled. Thus, writing to these fields and then calling phylink_ethtool_get_eee() results in these fields being overwritten. > static int lan743x_ethtool_set_eee(struct net_device *netdev, > struct ethtool_keee *eee) > { > - struct lan743x_adapter *adapter; > - struct phy_device *phydev; > - u32 buf = 0; > + struct lan743x_adapter *adapter = netdev_priv(netdev); > > - if (!netdev) > - return -EINVAL; > - adapter = netdev_priv(netdev); > - if (!adapter) > - return -EINVAL; > - phydev = netdev->phydev; > - if (!phydev) > - return -EIO; > - if (!phydev->drv) { > - netif_err(adapter, drv, adapter->netdev, > - "Missing PHY Driver\n"); > - return -EIO; > - } > + if (eee->tx_lpi_enabled) > + lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, > + eee->tx_lpi_timer); > + else > + lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, 0); > > - if (eee->eee_enabled) { > - buf = (u32)eee->tx_lpi_timer; > - lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, buf); > - } > + adapter->eee_enabled = eee->eee_enabled; > + adapter->tx_lpi_enabled = eee->tx_lpi_enabled; Given that phylib stores these and overwrites your copies in the get_eee method above, do you need to store these? > @@ -3013,7 +3025,12 @@ static void lan743x_phylink_mac_link_down(struct phylink_config *config, > unsigned int link_an_mode, > phy_interface_t interface) > { > + struct net_device *netdev = to_net_dev(config->dev); > + struct lan743x_adapter *adapter = netdev_priv(netdev); > + > netif_tx_stop_all_queues(to_net_dev(config->dev)); > + adapter->eee_active = false; phylib tracks this for you. > + lan743x_set_eee(adapter, false); > } > > static void lan743x_phylink_mac_link_up(struct phylink_config *config, > @@ -3055,6 +3072,14 @@ static void lan743x_phylink_mac_link_up(struct phylink_config *config, > cap & FLOW_CTRL_TX, > cap & FLOW_CTRL_RX); > > + if (phydev && adapter->eee_enabled) { > + bool enable; > + > + adapter->eee_active = phy_init_eee(phydev, false) >= 0; > + enable = adapter->eee_active && adapter->tx_lpi_enabled; No need. Your enable should be conditional on phydev->enable_tx_lpi here. See Andrew's commit and understand his changes, rather than just ignoring Andrew's work and continuing with your old pattern of EEE support. Things have moved forward. Thanks.
Hi Russell King, The 07/30/2024 16:11, Russell King (Oracle) wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Tue, Jul 30, 2024 at 07:36:19PM +0530, Raju Lakkaraju wrote: > > diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c > > index 3a63ec091413..a649ea7442a4 100644 > > --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c > > +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c > > @@ -1058,61 +1058,48 @@ static int lan743x_ethtool_get_eee(struct net_device *netdev, > > struct ethtool_keee *eee) > > { > > struct lan743x_adapter *adapter = netdev_priv(netdev); > > - struct phy_device *phydev = netdev->phydev; > > - u32 buf; > > - int ret; > > - > > - if (!phydev) > > - return -EIO; > > - if (!phydev->drv) { > > - netif_err(adapter, drv, adapter->netdev, > > - "Missing PHY Driver\n"); > > - return -EIO; > > - } > > > > - ret = phy_ethtool_get_eee(phydev, eee); > > - if (ret < 0) > > - return ret; > > - > > - buf = lan743x_csr_read(adapter, MAC_CR); > > - if (buf & MAC_CR_EEE_EN_) { > > - /* EEE_TX_LPI_REQ_DLY & tx_lpi_timer are same uSec unit */ > > - buf = lan743x_csr_read(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT); > > - eee->tx_lpi_timer = buf; > > - } else { > > - eee->tx_lpi_timer = 0; > > - } > > + eee->tx_lpi_timer = lan743x_csr_read(adapter, > > + MAC_EEE_TX_LPI_REQ_DLY_CNT); > > + eee->eee_enabled = adapter->eee_enabled; > > + eee->eee_active = adapter->eee_active; > > + eee->tx_lpi_enabled = adapter->tx_lpi_enabled; > > You really need to start paying attention to the commits other people > make as a result of development to other parts of the kernel. > > First, see: > > commit ef460a8986fa0dae1cdcb158a06127f7af27c92d > Author: Andrew Lunn <andrew@lunn.ch> > Date: Sat Apr 6 15:16:00 2024 -0500 > > net: lan743x: Fixup EEE > > and note that the assignment of eee->eee_enabled, eee->eee_active, and > eee->tx_lpi_enabled were all removed. > Accepted. I will fix > Next... > > > > > - return 0; > > + return phylink_ethtool_get_eee(adapter->phylink, eee); > > phylink_ethtool_get_eee() will call phy_ethtool_get_eee(), which > in turn calls genphy_c45_ethtool_get_eee() and eeecfg_to_eee(). > > genphy_c45_ethtool_get_eee() will do this: > > data->eee_enabled = is_enabled; > data->eee_active = ret; > > thus overwriting your assignment to eee->eee_enabled and > eee->eee_active. > > eeecfg_to_eee() will overwrite eee->tx_lpi_enabled and > eee->eee_enabled. > > Thus, writing to these fields and then calling > phylink_ethtool_get_eee() results in these fields being overwritten. > Ok. I will fix. > > static int lan743x_ethtool_set_eee(struct net_device *netdev, > > struct ethtool_keee *eee) > > { > > - struct lan743x_adapter *adapter; > > - struct phy_device *phydev; > > - u32 buf = 0; > > + struct lan743x_adapter *adapter = netdev_priv(netdev); > > > > - if (!netdev) > > - return -EINVAL; > > - adapter = netdev_priv(netdev); > > - if (!adapter) > > - return -EINVAL; > > - phydev = netdev->phydev; > > - if (!phydev) > > - return -EIO; > > - if (!phydev->drv) { > > - netif_err(adapter, drv, adapter->netdev, > > - "Missing PHY Driver\n"); > > - return -EIO; > > - } > > + if (eee->tx_lpi_enabled) > > + lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, > > + eee->tx_lpi_timer); > > + else > > + lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, 0); > > > > - if (eee->eee_enabled) { > > - buf = (u32)eee->tx_lpi_timer; > > - lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, buf); > > - } > > + adapter->eee_enabled = eee->eee_enabled; > > + adapter->tx_lpi_enabled = eee->tx_lpi_enabled; > > Given that phylib stores these and overwrites your copies in the get_eee > method above, do you need to store these? > OK > > @@ -3013,7 +3025,12 @@ static void lan743x_phylink_mac_link_down(struct phylink_config *config, > > unsigned int link_an_mode, > > phy_interface_t interface) > > { > > + struct net_device *netdev = to_net_dev(config->dev); > > + struct lan743x_adapter *adapter = netdev_priv(netdev); > > + > > netif_tx_stop_all_queues(to_net_dev(config->dev)); > > + adapter->eee_active = false; > > phylib tracks this for you. > Ok > > + lan743x_set_eee(adapter, false); > > } > > > > static void lan743x_phylink_mac_link_up(struct phylink_config *config, > > @@ -3055,6 +3072,14 @@ static void lan743x_phylink_mac_link_up(struct phylink_config *config, > > cap & FLOW_CTRL_TX, > > cap & FLOW_CTRL_RX); > > > > + if (phydev && adapter->eee_enabled) { > > + bool enable; > > + > > + adapter->eee_active = phy_init_eee(phydev, false) >= 0; > > + enable = adapter->eee_active && adapter->tx_lpi_enabled; > > No need. Your enable should be conditional on phydev->enable_tx_lpi > here. See Andrew's commit and understand his changes, rather than > just ignoring Andrew's work and continuing with your old pattern of > EEE support. Things have moved forward. Ok. I will fix > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c index 3a63ec091413..a649ea7442a4 100644 --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c @@ -1058,61 +1058,48 @@ static int lan743x_ethtool_get_eee(struct net_device *netdev, struct ethtool_keee *eee) { struct lan743x_adapter *adapter = netdev_priv(netdev); - struct phy_device *phydev = netdev->phydev; - u32 buf; - int ret; - - if (!phydev) - return -EIO; - if (!phydev->drv) { - netif_err(adapter, drv, adapter->netdev, - "Missing PHY Driver\n"); - return -EIO; - } - ret = phy_ethtool_get_eee(phydev, eee); - if (ret < 0) - return ret; - - buf = lan743x_csr_read(adapter, MAC_CR); - if (buf & MAC_CR_EEE_EN_) { - /* EEE_TX_LPI_REQ_DLY & tx_lpi_timer are same uSec unit */ - buf = lan743x_csr_read(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT); - eee->tx_lpi_timer = buf; - } else { - eee->tx_lpi_timer = 0; - } + eee->tx_lpi_timer = lan743x_csr_read(adapter, + MAC_EEE_TX_LPI_REQ_DLY_CNT); + eee->eee_enabled = adapter->eee_enabled; + eee->eee_active = adapter->eee_active; + eee->tx_lpi_enabled = adapter->tx_lpi_enabled; - return 0; + return phylink_ethtool_get_eee(adapter->phylink, eee); } static int lan743x_ethtool_set_eee(struct net_device *netdev, struct ethtool_keee *eee) { - struct lan743x_adapter *adapter; - struct phy_device *phydev; - u32 buf = 0; + struct lan743x_adapter *adapter = netdev_priv(netdev); - if (!netdev) - return -EINVAL; - adapter = netdev_priv(netdev); - if (!adapter) - return -EINVAL; - phydev = netdev->phydev; - if (!phydev) - return -EIO; - if (!phydev->drv) { - netif_err(adapter, drv, adapter->netdev, - "Missing PHY Driver\n"); - return -EIO; - } + if (eee->tx_lpi_enabled) + lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, + eee->tx_lpi_timer); + else + lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, 0); - if (eee->eee_enabled) { - buf = (u32)eee->tx_lpi_timer; - lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, buf); - } + adapter->eee_enabled = eee->eee_enabled; + adapter->tx_lpi_enabled = eee->tx_lpi_enabled; + lan743x_set_eee(adapter, eee->tx_lpi_enabled && eee->eee_enabled); - return phy_ethtool_set_eee(phydev, eee); + return phylink_ethtool_set_eee(adapter->phylink, eee); +} + +static int lan743x_ethtool_set_link_ksettings(struct net_device *netdev, + const struct ethtool_link_ksettings *cmd) +{ + struct lan743x_adapter *adapter = netdev_priv(netdev); + + return phylink_ethtool_ksettings_set(adapter->phylink, cmd); +} + +static int lan743x_ethtool_get_link_ksettings(struct net_device *netdev, + struct ethtool_link_ksettings *cmd) +{ + struct lan743x_adapter *adapter = netdev_priv(netdev); + + return phylink_ethtool_ksettings_get(adapter->phylink, cmd); } #ifdef CONFIG_PM @@ -1124,8 +1111,7 @@ static void lan743x_ethtool_get_wol(struct net_device *netdev, wol->supported = 0; wol->wolopts = 0; - if (netdev->phydev) - phy_ethtool_get_wol(netdev->phydev, wol); + phylink_ethtool_get_wol(adapter->phylink, wol); if (wol->supported != adapter->phy_wol_supported) netif_warn(adapter, drv, adapter->netdev, @@ -1166,7 +1152,7 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev, !(adapter->phy_wol_supported & WAKE_MAGICSECURE)) phy_wol.wolopts &= ~WAKE_MAGIC; - ret = phy_ethtool_set_wol(netdev->phydev, &phy_wol); + ret = phylink_ethtool_set_wol(adapter->phylink, wol); if (ret && (ret != -EOPNOTSUPP)) return ret; @@ -1355,44 +1341,16 @@ static void lan743x_get_pauseparam(struct net_device *dev, struct ethtool_pauseparam *pause) { struct lan743x_adapter *adapter = netdev_priv(dev); - struct lan743x_phy *phy = &adapter->phy; - if (phy->fc_request_control & FLOW_CTRL_TX) - pause->tx_pause = 1; - if (phy->fc_request_control & FLOW_CTRL_RX) - pause->rx_pause = 1; - pause->autoneg = phy->fc_autoneg; + phylink_ethtool_get_pauseparam(adapter->phylink, pause); } static int lan743x_set_pauseparam(struct net_device *dev, struct ethtool_pauseparam *pause) { struct lan743x_adapter *adapter = netdev_priv(dev); - struct phy_device *phydev = dev->phydev; - struct lan743x_phy *phy = &adapter->phy; - if (!phydev) - return -ENODEV; - - if (!phy_validate_pause(phydev, pause)) - return -EINVAL; - - phy->fc_request_control = 0; - if (pause->rx_pause) - phy->fc_request_control |= FLOW_CTRL_RX; - - if (pause->tx_pause) - phy->fc_request_control |= FLOW_CTRL_TX; - - phy->fc_autoneg = pause->autoneg; - - if (pause->autoneg == AUTONEG_DISABLE) - lan743x_mac_flow_ctrl_set_enables(adapter, pause->tx_pause, - pause->rx_pause); - else - phy_set_asym_pause(phydev, pause->rx_pause, pause->tx_pause); - - return 0; + return phylink_ethtool_set_pauseparam(adapter->phylink, pause); } const struct ethtool_ops lan743x_ethtool_ops = { @@ -1417,8 +1375,8 @@ const struct ethtool_ops lan743x_ethtool_ops = { .get_ts_info = lan743x_ethtool_get_ts_info, .get_eee = lan743x_ethtool_get_eee, .set_eee = lan743x_ethtool_set_eee, - .get_link_ksettings = phy_ethtool_get_link_ksettings, - .set_link_ksettings = phy_ethtool_set_link_ksettings, + .get_link_ksettings = lan743x_ethtool_get_link_ksettings, + .set_link_ksettings = lan743x_ethtool_set_link_ksettings, .get_regs_len = lan743x_get_regs_len, .get_regs = lan743x_get_regs, .get_pauseparam = lan743x_get_pauseparam, diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c index 845a48ca497d..dc46686529a2 100644 --- a/drivers/net/ethernet/microchip/lan743x_main.c +++ b/drivers/net/ethernet/microchip/lan743x_main.c @@ -2966,6 +2966,18 @@ static int lan743x_phylink_2500basex_config(struct lan743x_adapter *adapter) return lan743x_pcs_power_reset(adapter); } +void lan743x_set_eee(struct lan743x_adapter *adapter, bool enable) +{ + u32 lpi_ctl1; + + lpi_ctl1 = lan743x_csr_read(adapter, MAC_CR); + if (enable) + lpi_ctl1 |= MAC_CR_EEE_EN_; + else + lpi_ctl1 &= ~MAC_CR_EEE_EN_; + lan743x_csr_write(adapter, MAC_CR, lpi_ctl1); +} + static void lan743x_phylink_mac_config(struct phylink_config *config, unsigned int link_an_mode, const struct phylink_link_state *state) @@ -3013,7 +3025,12 @@ static void lan743x_phylink_mac_link_down(struct phylink_config *config, unsigned int link_an_mode, phy_interface_t interface) { + struct net_device *netdev = to_net_dev(config->dev); + struct lan743x_adapter *adapter = netdev_priv(netdev); + netif_tx_stop_all_queues(to_net_dev(config->dev)); + adapter->eee_active = false; + lan743x_set_eee(adapter, false); } static void lan743x_phylink_mac_link_up(struct phylink_config *config, @@ -3055,6 +3072,14 @@ static void lan743x_phylink_mac_link_up(struct phylink_config *config, cap & FLOW_CTRL_TX, cap & FLOW_CTRL_RX); + if (phydev && adapter->eee_enabled) { + bool enable; + + adapter->eee_active = phy_init_eee(phydev, false) >= 0; + enable = adapter->eee_active && adapter->tx_lpi_enabled; + lan743x_set_eee(adapter, enable); + } + netif_tx_wake_all_queues(netdev); } diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h index 7f73d66854be..79f21789eb32 100644 --- a/drivers/net/ethernet/microchip/lan743x_main.h +++ b/drivers/net/ethernet/microchip/lan743x_main.h @@ -1086,6 +1086,9 @@ struct lan743x_adapter { phy_interface_t phy_interface; struct phylink *phylink; struct phylink_config phylink_config; + bool eee_enabled; + bool eee_active; + bool tx_lpi_enabled; }; #define LAN743X_COMPONENT_FLAG_RX(channel) BIT(20 + (channel)) @@ -1206,5 +1209,6 @@ void lan743x_hs_syslock_release(struct lan743x_adapter *adapter); void lan743x_mac_flow_ctrl_set_enables(struct lan743x_adapter *adapter, bool tx_enable, bool rx_enable); int lan743x_sgmii_read(struct lan743x_adapter *adapter, u8 mmd, u16 addr); +void lan743x_set_eee(struct lan743x_adapter *adapter, bool enable); #endif /* _LAN743X_H */
Add support to ethtool phylink functions: - get/set settings like speed, duplex etc - get/set the wake-on-lan (WOL) - get/set the energy-efficient ethernet (EEE) - get/set the pause Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com> --- Change List: ============ V2 -> V3: - No change V1 -> V2: - Fix the phylink changes .../net/ethernet/microchip/lan743x_ethtool.c | 118 ++++++------------ drivers/net/ethernet/microchip/lan743x_main.c | 25 ++++ drivers/net/ethernet/microchip/lan743x_main.h | 4 + 3 files changed, 67 insertions(+), 80 deletions(-)