| Message ID | 20230804084527.2082302-1-yong.liang.choong@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | TSN auto negotiation between 1G and 2.5G | expand |
On Fri, Aug 04, 2023 at 04:45:22PM +0800, Choong Yong Liang wrote: > Intel platforms’ integrated Gigabit Ethernet controllers support > 2.5Gbps mode statically using BIOS programming. In the current > implementation, the BIOS menu provides an option to select between > 10/100/1000Mbps and 2.5Gbps modes. Based on the selection, the BIOS > programs the Phase Lock Loop (PLL) registers. The BIOS also read the > TSN lane registers from Flexible I/O Adapter (FIA) block and provided > 10/100/1000Mbps/2.5Gbps information to the stmmac driver. But > auto-negotiation between 10/100/1000Mbps and 2.5Gbps is not allowed. > The new proposal is to support auto-negotiation between 10/100/1000Mbps > and 2.5Gbps . Auto-negotiation between 10, 100, 1000Mbps will use > in-band auto negotiation. Auto-negotiation between 10/100/1000Mbps and > 2.5Gbps will work as the following proposed flow, the stmmac driver reads > the PHY link status registers then identifies the negotiated speed. > Based on the speed stmmac driver will identify TSN lane registers from > FIA then send IPC command to the Power Management controller (PMC) > through PMC driver/API. PMC will act as a proxy to programs the > PLL registers. Have you considered using out of band for all link modes? You might end up with a cleaner architecture, and not need any phylink/phylib hacks. Andrew
On 4/8/2023 8:04 pm, Andrew Lunn wrote: > On Fri, Aug 04, 2023 at 04:45:22PM +0800, Choong Yong Liang wrote: >> Intel platforms’ integrated Gigabit Ethernet controllers support >> 2.5Gbps mode statically using BIOS programming. In the current >> implementation, the BIOS menu provides an option to select between >> 10/100/1000Mbps and 2.5Gbps modes. Based on the selection, the BIOS >> programs the Phase Lock Loop (PLL) registers. The BIOS also read the >> TSN lane registers from Flexible I/O Adapter (FIA) block and provided >> 10/100/1000Mbps/2.5Gbps information to the stmmac driver. But >> auto-negotiation between 10/100/1000Mbps and 2.5Gbps is not allowed. >> The new proposal is to support auto-negotiation between 10/100/1000Mbps >> and 2.5Gbps . Auto-negotiation between 10, 100, 1000Mbps will use >> in-band auto negotiation. Auto-negotiation between 10/100/1000Mbps and >> 2.5Gbps will work as the following proposed flow, the stmmac driver reads >> the PHY link status registers then identifies the negotiated speed. >> Based on the speed stmmac driver will identify TSN lane registers from >> FIA then send IPC command to the Power Management controller (PMC) >> through PMC driver/API. PMC will act as a proxy to programs the >> PLL registers. > > Have you considered using out of band for all link modes? You might > end up with a cleaner architecture, and not need any phylink/phylib > hacks. > > Andrew Hi Andrew, Thank you for your feedback. I will study the feasibility of the out-of-band (OOB) approach.
On 4/8/2023 8:04 pm, Andrew Lunn wrote: > On Fri, Aug 04, 2023 at 04:45:22PM +0800, Choong Yong Liang wrote: >> Intel platforms’ integrated Gigabit Ethernet controllers support >> 2.5Gbps mode statically using BIOS programming. In the current >> implementation, the BIOS menu provides an option to select between >> 10/100/1000Mbps and 2.5Gbps modes. Based on the selection, the BIOS >> programs the Phase Lock Loop (PLL) registers. The BIOS also read the >> TSN lane registers from Flexible I/O Adapter (FIA) block and provided >> 10/100/1000Mbps/2.5Gbps information to the stmmac driver. But >> auto-negotiation between 10/100/1000Mbps and 2.5Gbps is not allowed. >> The new proposal is to support auto-negotiation between 10/100/1000Mbps >> and 2.5Gbps . Auto-negotiation between 10, 100, 1000Mbps will use >> in-band auto negotiation. Auto-negotiation between 10/100/1000Mbps and >> 2.5Gbps will work as the following proposed flow, the stmmac driver reads >> the PHY link status registers then identifies the negotiated speed. >> Based on the speed stmmac driver will identify TSN lane registers from >> FIA then send IPC command to the Power Management controller (PMC) >> through PMC driver/API. PMC will act as a proxy to programs the >> PLL registers. > > Have you considered using out of band for all link modes? You might > end up with a cleaner architecture, and not need any phylink/phylib > hacks. > > Andrew Hi Andrew, After conducting a comprehensive study, it seems that implementing out-of-band for all link modes might not be feasible. I may have missed some key aspects during my analysis. Would you be open to sharing a high-level idea of how we could potentially make this feasible? Your insights would be greatly appreciated. By the way, I've submitted a new design that not exposing phylink's AN mode into phylib. Please help review it to determine if it is acceptable.
> Hi Andrew, > > After conducting a comprehensive study, it seems that implementing > out-of-band for all link modes might not be feasible. I may have missed some > key aspects during my analysis. > > Would you be open to sharing a high-level idea of how we could potentially > make this feasible? Your insights would be greatly appreciated. stmmac_mac_link_up() gets passed interface, speed and duplex. That tells you what the PHY has negotiated. Is there anything else you need to know? Andrew
On Thu, Sep 21, 2023 at 08:25:05PM +0800, Choong Yong Liang wrote: > > > On 4/8/2023 8:04 pm, Andrew Lunn wrote: > > On Fri, Aug 04, 2023 at 04:45:22PM +0800, Choong Yong Liang wrote: > > > Intel platforms’ integrated Gigabit Ethernet controllers support > > > 2.5Gbps mode statically using BIOS programming. In the current > > > implementation, the BIOS menu provides an option to select between > > > 10/100/1000Mbps and 2.5Gbps modes. Based on the selection, the BIOS > > > programs the Phase Lock Loop (PLL) registers. The BIOS also read the > > > TSN lane registers from Flexible I/O Adapter (FIA) block and provided > > > 10/100/1000Mbps/2.5Gbps information to the stmmac driver. But > > > auto-negotiation between 10/100/1000Mbps and 2.5Gbps is not allowed. > > > The new proposal is to support auto-negotiation between 10/100/1000Mbps > > > and 2.5Gbps . Auto-negotiation between 10, 100, 1000Mbps will use > > > in-band auto negotiation. Auto-negotiation between 10/100/1000Mbps and > > > 2.5Gbps will work as the following proposed flow, the stmmac driver reads > > > the PHY link status registers then identifies the negotiated speed. > > > Based on the speed stmmac driver will identify TSN lane registers from > > > FIA then send IPC command to the Power Management controller (PMC) > > > through PMC driver/API. PMC will act as a proxy to programs the > > > PLL registers. > > > > Have you considered using out of band for all link modes? You might > > end up with a cleaner architecture, and not need any phylink/phylib > > hacks. > > > > Andrew > Hi Andrew, > > After conducting a comprehensive study, it seems that implementing > out-of-band for all link modes might not be feasible. I may have missed some > key aspects during my analysis. You need to provide details of why you think it's not feasible, because you're making those reading your message have to guess. We _do_ have cases where this is already supported. The DM7052 SFP module for example has a BCM84881 PHY on board that has no in-band support, so always has to use out-of-band. This module supports 10G, 5G, 2.5G, 1G, 100M and 10M speeds. It switches its interface between 10G, 2500base-X and SGMII mode. It's been supported in Linux for a while with MAC/PCS that implement phylink _correctly_. I wouldn't call stmmac a proper phylink implementation, especially when it comes to switching between different interfaces. My attempt at starting to clean up the stmmac code was thwarted by niggly review comments (over whether %u or %d should be used to print a _signed integer_ that stmmac stupidly implicitly casts to an unsigned integer. That lead me to decide that stmmac was beyond being cleaned up, so I junked the large patch set of improvements that I had - along with multiple issues that I had found in the driver. Someone else needs to sort stmmac out, and I suspect that may be a pre-requisit for your changes so that stmmac operates _correctly_ with phylink.
On Thu, Sep 21, 2023 at 03:21:00PM +0200, Andrew Lunn wrote: > > Hi Andrew, > > > > After conducting a comprehensive study, it seems that implementing > > out-of-band for all link modes might not be feasible. I may have missed some > > key aspects during my analysis. > > > > Would you be open to sharing a high-level idea of how we could potentially > > make this feasible? Your insights would be greatly appreciated. > > stmmac_mac_link_up() gets passed interface, speed and duplex. That > tells you what the PHY has negotiated. Is there anything else you need > to know? The problem is... the stmmac driver is utter bollocks - that information is *not* passed to the BSP. Instead, stmmac parse and store information such as the PHY interface mode at initialisation time. BSPs also re- parse and store e.g. the PHY interface mode at initialisation time. The driver ignores what it gets from phylink. The driver is basically utter crap. That's an area I _had_ patches to clean up. I no longer do. stmmac is crap crap crap and will stay crap until they become more receptive to patches to fix it, even if the patches are not 100% to their liking but are in fact correct. Maybe if I ever decide to touch that driver in the future. Which I doubt given my recent experience.
On Thu, Sep 21, 2023 at 03:12:19PM +0100, Russell King (Oracle) wrote: > On Thu, Sep 21, 2023 at 03:21:00PM +0200, Andrew Lunn wrote: > > > Hi Andrew, > > > > > > After conducting a comprehensive study, it seems that implementing > > > out-of-band for all link modes might not be feasible. I may have missed some > > > key aspects during my analysis. > > > > > > Would you be open to sharing a high-level idea of how we could potentially > > > make this feasible? Your insights would be greatly appreciated. > > > > stmmac_mac_link_up() gets passed interface, speed and duplex. That > > tells you what the PHY has negotiated. Is there anything else you need > > to know? > > The problem is... the stmmac driver is utter bollocks - that information > is *not* passed to the BSP. Instead, stmmac parse and store information > such as the PHY interface mode at initialisation time. BSPs also re- > parse and store e.g. the PHY interface mode at initialisation time. > The driver ignores what it gets from phylink. > > The driver is basically utter crap. That's an area I _had_ patches to > clean up. I no longer do. stmmac is crap crap crap and will stay crap > until they become more receptive to patches to fix it, even if the > patches are not 100% to their liking but are in fact correct. Maybe > if I ever decide to touch that driver in the future. Which I doubt > given my recent experience. Hi Russell You pointed out the current proposal will break stuff. Do you see a way forward for this patchset which does not first involve actually cleaning up of this driver? Andrew
On Thu, Sep 21, 2023 at 04:41:20PM +0200, Andrew Lunn wrote: > On Thu, Sep 21, 2023 at 03:12:19PM +0100, Russell King (Oracle) wrote: > > On Thu, Sep 21, 2023 at 03:21:00PM +0200, Andrew Lunn wrote: > > > > Hi Andrew, > > > > > > > > After conducting a comprehensive study, it seems that implementing > > > > out-of-band for all link modes might not be feasible. I may have missed some > > > > key aspects during my analysis. > > > > > > > > Would you be open to sharing a high-level idea of how we could potentially > > > > make this feasible? Your insights would be greatly appreciated. > > > > > > stmmac_mac_link_up() gets passed interface, speed and duplex. That > > > tells you what the PHY has negotiated. Is there anything else you need > > > to know? > > > > The problem is... the stmmac driver is utter bollocks - that information > > is *not* passed to the BSP. Instead, stmmac parse and store information > > such as the PHY interface mode at initialisation time. BSPs also re- > > parse and store e.g. the PHY interface mode at initialisation time. > > The driver ignores what it gets from phylink. > > > > The driver is basically utter crap. That's an area I _had_ patches to > > clean up. I no longer do. stmmac is crap crap crap and will stay crap > > until they become more receptive to patches to fix it, even if the > > patches are not 100% to their liking but are in fact correct. Maybe > > if I ever decide to touch that driver in the future. Which I doubt > > given my recent experience. > > Hi Russell > > You pointed out the current proposal will break stuff. Do you see a > way forward for this patchset which does not first involve actually > cleaning up of this driver? As I said in one of my replies, it would really help if the author can provide a table showing what is attempting to be achieved here. With that, we should be able to work out exactly what is required, what needs to change in stmmac, etc.
On 21/9/2023 10:55 pm, Russell King (Oracle) wrote: > On Thu, Sep 21, 2023 at 04:41:20PM +0200, Andrew Lunn wrote: >> On Thu, Sep 21, 2023 at 03:12:19PM +0100, Russell King (Oracle) wrote: >>> On Thu, Sep 21, 2023 at 03:21:00PM +0200, Andrew Lunn wrote: >>>>> Hi Andrew, >>>>> >>>>> After conducting a comprehensive study, it seems that implementing >>>>> out-of-band for all link modes might not be feasible. I may have missed some >>>>> key aspects during my analysis. >>>>> >>>>> Would you be open to sharing a high-level idea of how we could potentially >>>>> make this feasible? Your insights would be greatly appreciated. >>>> >>>> stmmac_mac_link_up() gets passed interface, speed and duplex. That >>>> tells you what the PHY has negotiated. Is there anything else you need >>>> to know? >>> >>> The problem is... the stmmac driver is utter bollocks - that information >>> is *not* passed to the BSP. Instead, stmmac parse and store information >>> such as the PHY interface mode at initialisation time. BSPs also re- >>> parse and store e.g. the PHY interface mode at initialisation time. >>> The driver ignores what it gets from phylink. >>> >>> The driver is basically utter crap. That's an area I _had_ patches to >>> clean up. I no longer do. stmmac is crap crap crap and will stay crap >>> until they become more receptive to patches to fix it, even if the >>> patches are not 100% to their liking but are in fact correct. Maybe >>> if I ever decide to touch that driver in the future. Which I doubt >>> given my recent experience. >> >> Hi Russell >> >> You pointed out the current proposal will break stuff. Do you see a >> way forward for this patchset which does not first involve actually >> cleaning up of this driver? > > As I said in one of my replies, it would really help if the author can > provide a table showing what is attempting to be achieved here. With > that, we should be able to work out exactly what is required, what > needs to change in stmmac, etc. > Thank you, Russell and Andrew for the comments. What I'm trying to achieve here is to enable interface mode switching between 2500basex and SGMII interfaces for Intel platforms. I did based on the DM7052 SFP module and BCM84881 PHY to make the necessary changes in the new version of my patch series. In the new patch series, the 'allow_switch_interface' flag was introduced, based on the 'allow_switch_interface' flag, the interface mode is configured to PHY_INTERFACE_MODE_NA within the 'phylink_validate_phy' function. This setting allows all ethtool link modes that are supported and advertised will be published. Then interface mode switching occurs based on the selection of different link modes. During the interface mode switching, the code will go through the `phylink_major_config` function and based on the interface mode to select PCS and PCS negotiation mode to configure PCS. Then, the MAC driver will perform SerDes configuration according to the interface mode. I did rewrite the description for the new patch series, hoping that it is clear to describe the whole intention of the changes.
Intel platforms’ integrated Gigabit Ethernet controllers support 2.5Gbps mode statically using BIOS programming. In the current implementation, the BIOS menu provides an option to select between 10/100/1000Mbps and 2.5Gbps modes. Based on the selection, the BIOS programs the Phase Lock Loop (PLL) registers. The BIOS also read the TSN lane registers from Flexible I/O Adapter (FIA) block and provided 10/100/1000Mbps/2.5Gbps information to the stmmac driver. But auto-negotiation between 10/100/1000Mbps and 2.5Gbps is not allowed. The new proposal is to support auto-negotiation between 10/100/1000Mbps and 2.5Gbps . Auto-negotiation between 10, 100, 1000Mbps will use in-band auto negotiation. Auto-negotiation between 10/100/1000Mbps and 2.5Gbps will work as the following proposed flow, the stmmac driver reads the PHY link status registers then identifies the negotiated speed. Based on the speed stmmac driver will identify TSN lane registers from FIA then send IPC command to the Power Management controller (PMC) through PMC driver/API. PMC will act as a proxy to programs the PLL registers. changelog: v1 -> v2: - Created intel_pmc_core.h in include/linux/platform_data/x86/ and export the desired functionality. - Add cur_link_an_mode to the kernel doc - Update cfg_link_an_mode value during phy driver changed - Combine 2 commits i.e. "stmmac: intel: Separate driver_data of ADL-N from TGL" and "net: stmmac: Add 1G/2.5G auto-negotiation support for ADL-N" into 1 commit. v1 -> v2: - Add static to pmc_lpm_modes declaration - Add cur_link_an_mode to the kernel doc - Combine 2 commits i.e. "stmmac: intel: Separate driver_data of ADL-N from TGL" and "net: stmmac: Add 1G/2.5G auto-negotiation support for ADL-N" into 1 commit. --- Choong Yong Liang (1): stmmac: intel: Add 1G/2.5G auto-negotiation support for ADL-N David E. Box (1): platform/x86: intel_pmc_core: Add IPC mailbox accessor function and add SoC register access Tan, Tee Min (3): net: pcs: xpcs: combine C37 SGMII AN and 2500BASEX for Intel mGbE controller net: phy: update in-band AN mode when changing interface by PHY driver net: stmmac: enable Intel mGbE 1G/2.5G auto-negotiation support MAINTAINERS | 1 + drivers/net/ethernet/stmicro/stmmac/Kconfig | 1 + .../net/ethernet/stmicro/stmmac/dwmac-intel.c | 183 +++++++++++++++++- .../net/ethernet/stmicro/stmmac/dwmac-intel.h | 81 ++++++++ .../net/ethernet/stmicro/stmmac/stmmac_main.c | 20 ++ drivers/net/pcs/pcs-xpcs.c | 72 +++++-- drivers/net/phy/marvell10g.c | 6 + drivers/net/phy/phylink.c | 4 + drivers/platform/x86/intel/pmc/core.c | 60 ++++++ include/linux/pcs/pcs-xpcs.h | 1 + include/linux/phy.h | 3 + .../linux/platform_data/x86/intel_pmc_core.h | 41 ++++ include/linux/stmmac.h | 1 + 13 files changed, 458 insertions(+), 16 deletions(-) create mode 100644 include/linux/platform_data/x86/intel_pmc_core.h