Re: [PATCH net-next V3 4/4] net: lan743x: Add support to ethtool phylink get and set settings
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
Date: 2024-07-30 15:11:16
Also in:
lkml
On Tue, Jul 30, 2024 at 07:36:19PM +0530, Raju Lakkaraju wrote:
quoted hunk ↗ jump to hunk
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 [off-list ref]
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?
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
+ 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. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!