| Message ID | Z1b9J-FihzJ4A6aQ@shell.armlinux.org.uk (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: add phylink managed EEE support | expand |
On Mon, Dec 09, 2024 at 02:22:31PM +0000, Russell King (Oracle) wrote: > Hi, > > Adding managed EEE support to phylink has been on the cards ever since > the idea in phylib was mooted. This overly large series attempts to do > so. I've included all the patches as it's important to get the driver > patches out there. > > Patch 1 adds a definition for the clock stop capable bit in the PCS > MMD status register. > > Patch 2 adds a phylib API to query whether the PHY allows the transmit > xMII clock to be stopped while in LPI mode. This capability is for MAC > drivers to save power when LPI is active, to allow them to stop their > transmit clock. > > Patch 3 adds another phylib API to configure whether the receive xMII > clock may be disabled by the PHY. We do have an existing API, > phy_init_eee(), but... it only allows the control bit to be set which > is weird - what if a boot firmware or previous kernel has set this bit > and we want it clear? > > Patch 4 starts on the phylink parts of this, extracting from > phylink_resolve() the detection of link-up. (Yes, okay, I could've > dropped this patch, but with 23 patches, it's not going to make that > much difference.) > > Patch 5 adds phylink managed EEE support. Two new MAC APIs are added, > to enable and disable LPI. The enable method is passed the LPI timer > setting which it is expected to program into the hardware, and also a > flag ehther the transmit clock should be stopped. > > *** There are open questions here. Eagle eyed reviewers will notice > pl->config->lpi_interfaces. There are MACs out there which only > support LPI signalling on a subset of their interface types. Phylib > doesn't understand this. I'm handling this at the moment by simply > not activating LPI at the MAC, but that leads to ethtool --show-eee > suggesting that EEE is active when it isn't. > *** Should we pass the phy_interface_t to these functions? Maybe only to validate? > *** Should mac_enable_tx_lpi() be allowed to fail if the MAC doesn't > support the interface mode? I'm a bit confused by this... Following principle with other OPs shouldn't this never happen? Supported interface are validated by capabilities hence mac_enable_tx_lpi() should never be reached (if not supported). Or I'm missing something by this idea? > > The above questions remain unanswered from the RFC posting of this > series. > > A change that has been included over the RFC version is the addition > of the mac_validate_tx_lpi() method, which allows MAC drivers to > validate the parameters to the ethtool set_eee() method. Implementations > of this are in mvneta and mvpp2. > > An example of a MAC that this is the case are the Marvell ones - both > NETA and PP2 only support LPI signalling when connected via SGMII, > which makes being connected to a PHY which changes its link mode > problematical. > > The remainder of the patches address the driver sides, which are > necessary to actually test phylink managed EEE. > > drivers/net/ethernet/marvell/mvneta.c | 127 +++++++++++-------- > drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 5 + > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 98 +++++++++++++++ > drivers/net/ethernet/microchip/lan743x_ethtool.c | 21 ---- > drivers/net/ethernet/microchip/lan743x_main.c | 39 ++++-- > drivers/net/ethernet/microchip/lan743x_main.h | 1 - > drivers/net/phy/phy.c | 47 ++++++- > drivers/net/phy/phylink.c | 150 +++++++++++++++++++++-- > include/linux/phy.h | 2 + > include/linux/phylink.h | 59 +++++++++ > include/uapi/linux/mdio.h | 1 + > 11 files changed, 458 insertions(+), 92 deletions(-) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Mon, Dec 09, 2024 at 07:35:11PM +0100, Christian Marangi wrote: > On Mon, Dec 09, 2024 at 02:22:31PM +0000, Russell King (Oracle) wrote: > > Hi, > > > > Adding managed EEE support to phylink has been on the cards ever since > > the idea in phylib was mooted. This overly large series attempts to do > > so. I've included all the patches as it's important to get the driver > > patches out there. > > > > Patch 1 adds a definition for the clock stop capable bit in the PCS > > MMD status register. > > > > Patch 2 adds a phylib API to query whether the PHY allows the transmit > > xMII clock to be stopped while in LPI mode. This capability is for MAC > > drivers to save power when LPI is active, to allow them to stop their > > transmit clock. > > > > Patch 3 adds another phylib API to configure whether the receive xMII > > clock may be disabled by the PHY. We do have an existing API, > > phy_init_eee(), but... it only allows the control bit to be set which > > is weird - what if a boot firmware or previous kernel has set this bit > > and we want it clear? > > > > Patch 4 starts on the phylink parts of this, extracting from > > phylink_resolve() the detection of link-up. (Yes, okay, I could've > > dropped this patch, but with 23 patches, it's not going to make that > > much difference.) > > > > Patch 5 adds phylink managed EEE support. Two new MAC APIs are added, > > to enable and disable LPI. The enable method is passed the LPI timer > > setting which it is expected to program into the hardware, and also a > > flag ehther the transmit clock should be stopped. > > > > *** There are open questions here. Eagle eyed reviewers will notice > > pl->config->lpi_interfaces. There are MACs out there which only > > support LPI signalling on a subset of their interface types. Phylib > > doesn't understand this. I'm handling this at the moment by simply > > not activating LPI at the MAC, but that leads to ethtool --show-eee > > suggesting that EEE is active when it isn't. > > *** Should we pass the phy_interface_t to these functions? > > Maybe only to validate? validate doesn't know what interface will be used - at set_eee() time we can't know that. > > *** Should mac_enable_tx_lpi() be allowed to fail if the MAC doesn't > > support the interface mode? > > I'm a bit confused by this... Following principle with other OPs > shouldn't this never happen? Supported interface are validated by > capabilities hence mac_enable_tx_lpi() should never be reached (if not > supported). Or I'm missing something by this idea? This question was asked in the RFC, and before I added lpi_interfaces which now prevents calling this unless the interface bit is set in lpi_interfaces. However, does it still make sense to pass the interface in case e.g. the MAC block that needs to be configured is dependent on it? Some network interfaces are made up of multiple MACs for different speeds, and the MAC is selected by interface. E.g. mvpp2.
On Mon, Dec 09, 2024 at 02:22:31PM +0000, Russell King (Oracle) wrote: > Patch 5 adds phylink managed EEE support. Two new MAC APIs are added, > to enable and disable LPI. The enable method is passed the LPI timer > setting which it is expected to program into the hardware, and also a > flag ehther the transmit clock should be stopped. > > *** There are open questions here. Eagle eyed reviewers will notice > pl->config->lpi_interfaces. There are MACs out there which only > support LPI signalling on a subset of their interface types. Phylib > doesn't understand this. I'm handling this at the moment by simply > not activating LPI at the MAC, but that leads to ethtool --show-eee > suggesting that EEE is active when it isn't. > *** Should we pass the phy_interface_t to these functions? > *** Should mac_enable_tx_lpi() be allowed to fail if the MAC doesn't > support the interface mode? > > The above questions remain unanswered from the RFC posting of this > series. Given the open questions that still remain and the lack of engagement, I don't think we're at the point of being able to apply this patch set (I don't want to rework the method functions in lots of drivers.) So, I suggest we put this on the back burner until there's a clear way forward.