diff mbox series

[net-next,2/2] net: stmmac: dwmac-qcom-ethqos: Enable support for 2500BASEX

Message ID 20240524130653.30666-3-quic_snehshah@quicinc.com (mailing list archive)
State Not Applicable
Headers show
Series net: stmmac: Add 2500BASEX support for integrated PCS | expand

Commit Message

Sneh Shah May 24, 2024, 1:06 p.m. UTC
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(+)

Comments

Andrew Lunn May 24, 2024, 4:07 p.m. UTC | #1
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
Russell King (Oracle) May 24, 2024, 5:13 p.m. UTC | #2
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.)
Russell King (Oracle) May 24, 2024, 5:16 p.m. UTC | #3
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 mbox series

Patch

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;