Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Russell King (Oracle)]

On Tue, May 28, 2024 at 03:13:32PM +0100, Russell King (Oracle) wrote:
> > Alternative solution could be to use the has_gmac/has_gmac4 flags
> > instead. That will emphasize that the embedded PCS is expected to be
> > specific for the DW GMAC and DW QoS Eth IP-cores:
> > 
> >        if (phy_interface_mode_is_rgmii(interface))
> >                priv->hw->pcs = STMMAC_PCS_RGMII;
> >        else if ((priv->plat.has_gmac || priv->plat.has_gmac4) &&
> > 		interface == PHY_INTERFACE_MODE_SGMII)
> >                priv->hw->pcs = STMMAC_PCS_SGMII;
> 
> which implies that gmac (dwgmac1000_core.c) and gmac4 (dwgmac4_core.c)
> will always have its internal PCS if we're using SGMII mode. Does this
> mean it is true that these cores will never be used with an external
> PCS?

Sorry to go off on a related tangent, but I've just been looking at
hw->ps which is related to this.

As I understand, hw->ps comes from the "snps,ps-speed" property in DT,
which is used for SGMII and MAC2MAC connections. Presumably for the
SGMII case, this is used where the port is made to look like the PHY
end of the SGMII link.

I'm guessing MAC2MAC refers to RGMII, or does that also refer to
SGMII-as-PHY?

I think it would've been nice to have picked SGMII-as-PHY up in the
driver earlier - we don't tend to use the "normal" PHY interface
mode names, instead we have the REVxxx modes, so I think this
_should_ have introduced PHY_INTERFACE_MODE_REVSGMII.

In any case, moving on... in stmmac_hw_setup(), we have:

        /* PS and related bits will be programmed according to the speed */
        if (priv->hw->pcs) {
                int speed = priv->plat->mac_port_sel_speed;

                if ((speed == SPEED_10) || (speed == SPEED_100) ||
                    (speed == SPEED_1000)) {
                        priv->hw->ps = speed;
                } else {
                        dev_warn(priv->device, "invalid port speed\n");
                        priv->hw->ps = 0;
                }
        }

Which means that if we're using the integrated PCS, then we basically
require the "snps,ps-speed" property otherwise we'll issue a warning
at this point... this seems to imply that reverse mode is the only
mode supported, which I'm fairly sure is false. So, maybe this
shouldn't be issuing the warning if mac_port_sel_speed was zero?

Moving on... hw->ps can only be 10M, 100M or 1G speeds and nothing else
- which is fine since RGMII and Cisco SGMII only support these speeds.

dwmac1000 tests for this against these speeds, so it is also fine.

dwmac4 is basically the same as dwmac1000, so is also fine.

The core code as it stands today passes this into the pcs_ctrl_ane
method's rsgmi_ral argument, which sets GMAC_AN_CTRL_SGMRAL. Presumably
this selects "reverse" mode for both SGMII and RGMII?

Persuing this a bit futher, qcom-ethqos always calls this with rsgmi_ral
clear. Presumably, qcom-ethqos never specifies "snps,ps-speed" in DT,
and thus always gets the warning above?

Finally, we get to the core issue, which is dwxgmac2_core.c.
dwxgmac2 tests this member against 10G, 2.5G and "any other non-zero
value". Out of all of these, the only possible path through that code
would be the one which results in:

	tx |= hw->link.speed1000;

Neither of the other two (2.5G and 10G) are possible because those
aren't legal values for hw->ps. Moreover, it doesn't appear to have
any kind of PCS, so I'm wondering whether any of this code gets used.


So, I suspect some of this is "not quite right" either, and I wonder
about the implications of changing how hw->pcs is set - whether we
first need to fix the code above dealing with priv->hw->ps ?

I'm also wondering what impact this has on my PCS conversion.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!