| Message ID | 20240524130653.30666-3-quic_snehshah@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: Add 2500BASEX support for integrated PCS | expand |
On Fri, May 24, 2024 at 06:36:53PM +0530, Sneh Shah wrote: > With integrated PCS qcom mac supports both SGMII and 2500BASEX mode. > Implement get_interfaces to add support for 2500BASEX. I don't know this driver very well..... but /* PCS defines */ #define STMMAC_PCS_RGMII (1 << 0) #define STMMAC_PCS_SGMII (1 << 1) #define STMMAC_PCS_TBI (1 << 2) #define STMMAC_PCS_RTBI (1 << 3) static int stmmac_ethtool_get_link_ksettings(struct net_device *dev, struct ethtool_link_ksettings *cmd) { struct stmmac_priv *priv = netdev_priv(dev); if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS) && (priv->hw->pcs & STMMAC_PCS_RGMII || priv->hw->pcs & STMMAC_PCS_SGMII)) { struct rgmii_adv adv; u32 supported, advertising, lp_advertising; if (!priv->xstats.pcs_link) { cmd->base.speed = SPEED_UNKNOWN; cmd->base.duplex = DUPLEX_UNKNOWN; return 0; } /** * stmmac_check_pcs_mode - verify if RGMII/SGMII is supported * @priv: driver private structure * Description: this is to verify if the HW supports the PCS. * Physical Coding Sublayer (PCS) interface that can be used when the MAC is * configured for the TBI, RTBI, or SGMII PHY interface. */ static void stmmac_check_pcs_mode(struct stmmac_priv *priv) { int interface = priv->plat->mac_interface; if (priv->dma_cap.pcs) { if ((interface == PHY_INTERFACE_MODE_RGMII) || (interface == PHY_INTERFACE_MODE_RGMII_ID) || (interface == PHY_INTERFACE_MODE_RGMII_RXID) || (interface == PHY_INTERFACE_MODE_RGMII_TXID)) { netdev_dbg(priv->dev, "PCS RGMII support enabled\n"); priv->hw->pcs = STMMAC_PCS_RGMII; } else if (interface == PHY_INTERFACE_MODE_SGMII) { netdev_dbg(priv->dev, "PCS SGMII support enabled\n"); priv->hw->pcs = STMMAC_PCS_SGMII; } } } I get the feeling this is a minimal hack, rather than a proper solution. Andrew --- pw-bot: cr
On Fri, May 24, 2024 at 06:07:15PM +0200, Andrew Lunn wrote: > On Fri, May 24, 2024 at 06:36:53PM +0530, Sneh Shah wrote: > > With integrated PCS qcom mac supports both SGMII and 2500BASEX mode. > > Implement get_interfaces to add support for 2500BASEX. > > I don't know this driver very well..... but > > /* PCS defines */ > #define STMMAC_PCS_RGMII (1 << 0) > #define STMMAC_PCS_SGMII (1 << 1) > #define STMMAC_PCS_TBI (1 << 2) > #define STMMAC_PCS_RTBI (1 << 3) > > > static int stmmac_ethtool_get_link_ksettings(struct net_device *dev, > struct ethtool_link_ksettings *cmd) > { > struct stmmac_priv *priv = netdev_priv(dev); > > if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS) && > (priv->hw->pcs & STMMAC_PCS_RGMII || > priv->hw->pcs & STMMAC_PCS_SGMII)) { > struct rgmii_adv adv; > u32 supported, advertising, lp_advertising; > > if (!priv->xstats.pcs_link) { > cmd->base.speed = SPEED_UNKNOWN; > cmd->base.duplex = DUPLEX_UNKNOWN; > return 0; > } Note that this checks for !STMMAC_FLAG_HAS_INTEGRATED_PCS, so this isn't going to be used by this code which is conditional on this flag being set. In any case, I posted a patch set 12 days ago, which has remained unreviewed and untested for 10 days from a promise to do so converting this ugly hack to a phylink PCS driver (not that I would have had time to deal with any feedback due to an urgent work issue, but that's sort of beside the point.) There's some vague handwaving by Serge that there are some issues in this series, but there hasn't been any feedback yet on what these issues may be. Also, from what I can tell, neither STMMAC_PCS_TBI nor STMMAC_PCS_RTBI are ever assigned to hw->pcs. So, in stmmac_eee_init(): if (priv->hw->pcs == STMMAC_PCS_TBI || priv->hw->pcs == STMMAC_PCS_RTBI) return false; is always false, and can be removed. In __stmmac_open(): if (priv->hw->pcs != STMMAC_PCS_TBI && priv->hw->pcs != STMMAC_PCS_RTBI && (!priv->hw->xpcs || xpcs_get_an_mode(priv->hw->xpcs, mode) != DW_AN_C73)) { can become: if (!priv->hw->xpcs || xpcs_get_an_mode(priv->hw->xpcs, mode) != DW_AN_C73)) { In stmmac_dvr_probe(): if (priv->hw->pcs != STMMAC_PCS_TBI && priv->hw->pcs != STMMAC_PCS_RTBI) { /* MDIO bus Registration */ ret = stmmac_mdio_register(ndev); This if() condition can be eliminated, and we always register the MDIO, and similarly in the cleanup and stmmac_dvr_remove() paths. > /** > * stmmac_check_pcs_mode - verify if RGMII/SGMII is supported > * @priv: driver private structure > * Description: this is to verify if the HW supports the PCS. > * Physical Coding Sublayer (PCS) interface that can be used when the MAC is > * configured for the TBI, RTBI, or SGMII PHY interface. > */ > static void stmmac_check_pcs_mode(struct stmmac_priv *priv) > { > int interface = priv->plat->mac_interface; > > if (priv->dma_cap.pcs) { > if ((interface == PHY_INTERFACE_MODE_RGMII) || > (interface == PHY_INTERFACE_MODE_RGMII_ID) || > (interface == PHY_INTERFACE_MODE_RGMII_RXID) || > (interface == PHY_INTERFACE_MODE_RGMII_TXID)) { > netdev_dbg(priv->dev, "PCS RGMII support enabled\n"); > priv->hw->pcs = STMMAC_PCS_RGMII; > } else if (interface == PHY_INTERFACE_MODE_SGMII) { > netdev_dbg(priv->dev, "PCS SGMII support enabled\n"); > priv->hw->pcs = STMMAC_PCS_SGMII; > } > } > } > > I get the feeling this is a minimal hack, rather than a proper > solution. I didn't remove that in my patch set because I don't understand fully the logic here - and I didn't want to add further dubious complication to my six patch series. I actually kept the logic and continued to use it explicitly to avoid changing the decision making: +static struct phylink_pcs * +dwmac4_phylink_select_pcs(struct stmmac_priv *priv, phy_interface_t interface) +{ + if (priv->hw->pcs & STMMAC_PCS_RGMII || + priv->hw->pcs & STMMAC_PCS_SGMII) + return &priv->hw->mac_pcs; + + return NULL; +} Ultimately, this _should_ check the "interface" here, but bear in mind that stmmac_check_pcs_mode() checks plat->mac_interface (which is the interface between the MAC and PCS) whereas the "interface" passed here is the interface between the PCS and PHY. This is why removing stmmac_check_pcs_mode() isn't a sane change to make until we have worked through the issues with removing the its-a-PCS-but-not-phylink_pcs hack. I _think_ a sensible next step would be to eliminate priv->hw->pcs, instead testing priv->plat->mac_interface in dwmac4_phylink_select_pcs() and dwmac1000_phylink_select_pcs(): phy_interface_t mac_interface = priv->plat->mac_interface; if (phy_interface_is_rgmii(mac_interface) || mac_interface == PHY_INTERFACE_MODE_SGMII) return &priv->hw->mac_pcs; and _then_ maybe as a separate patch switch this to test the PHY-side interface (because that would make more sense... but that would be a behavioural change to the driver.)
On Fri, May 24, 2024 at 06:13:17PM +0100, Russell King (Oracle) wrote: > Note that this checks for !STMMAC_FLAG_HAS_INTEGRATED_PCS, so this isn't > going to be used by this code which is conditional on this flag being > set. > > In any case, I posted a patch set 12 days ago, which has remained Oops, forgot the link to the patch series: https://lore.kernel.org/r/ZkDuJAx7atDXjf5m@shell.armlinux.org.uk
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c index e254b21fdb59..dad6e2448475 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c @@ -731,6 +731,13 @@ static void ethqos_clks_disable(void *data) ethqos_clks_config(data, false); } +static void ethqos_get_interfaces(struct stmmac_priv *priv) +{ + if (priv->plat->phy_interface == PHY_INTERFACE_MODE_SGMII) + __set_bit(PHY_INTERFACE_MODE_2500BASEX, + priv->phylink_config.supported_interfaces); +}; + static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv) { struct plat_stmmacenet_data *plat_dat = priv->plat; @@ -786,6 +793,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev) ethqos->configure_func = ethqos_configure_rgmii; break; case PHY_INTERFACE_MODE_SGMII: + case PHY_INTERFACE_MODE_2500BASEX: ethqos->configure_func = ethqos_configure_sgmii; break; default: @@ -851,6 +859,9 @@ static int qcom_ethqos_probe(struct platform_device *pdev) plat_dat->serdes_powerdown = qcom_ethqos_serdes_powerdown; } + if (plat_dat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS) + plat_dat->get_interfaces = ethqos_get_interfaces; + /* Enable TSO on queue0 and enable TBS on rest of the queues */ for (i = 1; i < plat_dat->tx_queues_to_use; i++) plat_dat->tx_queues_cfg[i].tbs_en = 1;
With integrated PCS qcom mac supports both SGMII and 2500BASEX mode. Implement get_interfaces to add support for 2500BASEX. Signed-off-by: Sneh Shah <quic_snehshah@quicinc.com> --- .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 11 +++++++++++ 1 file changed, 11 insertions(+)