Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Nov 04, 2022 at 08:40:56AM -0700, Florian Fainelli wrote:
> On 11/4/2022 4:35 AM, Russell King (Oracle) wrote:
> > diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> > index 18a3847bd82b..6676971128d1 100644
> > --- a/drivers/net/dsa/bcm_sf2.c
> > +++ b/drivers/net/dsa/bcm_sf2.c
> > @@ -727,6 +727,10 @@ static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
> >   		__set_bit(PHY_INTERFACE_MODE_MII, interfaces);
> >   		__set_bit(PHY_INTERFACE_MODE_REVMII, interfaces);
> >   		__set_bit(PHY_INTERFACE_MODE_GMII, interfaces);
> > +
> > +		/* FIXME 1: Are RGMII_RXID and RGMII_ID actually supported?
> > +		 * See FIXME 2 and FIXME 3 below.
> > +		 */
> 
> They are supported, just not tested and still don't have hardware to test, I
> suppose you can include both modes for simplicity if they end up being
> broken, a fix would be submitted.

Okay, that sounds like we can add them to the switch() in
bcm_sf2_sw_mac_config(). I assume id_mode_dis should be zero for
these other modes.

> > +static void bcm_sf2_sw_validate(struct dsa_switch *ds, int port,
> > +				unsigned long *supported,
> > +				struct phylink_link_state *state)
> > +{
> > +	u32 caps;
> > +
> > +	caps = dsa_to_port(ds, port)->pl_config.mac_capabilities;
> > +
> > +	/* Pause modes are only programmed for these modes - see FIXME 3.
> > +	 * So, as pause modes are not configured for other modes, disable
> > +	 * support for them. If FIXME 3 is updated to allow the other RGMII
> > +	 * modes, these should be included here as well.
> > +	 */
> > +	if (!(state->interface == PHY_INTERFACE_MODE_RGMII ||
> > +	      state->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
> > +	      state->interface == PHY_INTERFACE_MODE_MII ||
> > +	      state->interface == PHY_INTERFACE_MODE_REVMII))
> > +		caps &= ~(MAC_ASYM_PAUSE | MAC_SYM_PAUSE);
> 
> Can be programmed on all ports.

If I understand you correctly, I think you mean that this can be
programmed on all ports that support the RGMII control register:

        if (phy_interface_mode_is_rgmii(interface) ||
            interface == PHY_INTERFACE_MODE_MII ||
            interface == PHY_INTERFACE_MODE_REVMII) {
                reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, port);
                reg = reg_readl(priv, reg_rgmii_ctrl);
                reg &= ~(RX_PAUSE_EN | TX_PAUSE_EN);

                if (tx_pause)
                        reg |= TX_PAUSE_EN;
                if (rx_pause)
                        reg |= RX_PAUSE_EN;

                reg_writel(priv, reg, reg_rgmii_ctrl);
        }

We seem to have several places in the code that make this decision -
bcm_sf2_sw_mac_link_set(). I'm guessing, looking at
bcm_sf2_reg_rgmii_cntrl(), that we _could_ use the device ID and port
number in bcm_sf2_sw_get_caps() to narrow down whether the pause
modes are supported - as ports that do not have the RGMII control
register can't have pause modes programmed?

So for the BCM4908, it's just port 7, and for everything else it's
ports 0-2.

That would mean something like:

static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
                                struct phylink_config *config)
{
...
	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
...
	config->mac_capabilities = MAC_10 | MAC_100 | MAC_1000;

	if (priv->type == BCM4908_DEVICE_ID ? port == 7 :
	    (port >= 0 && port < 3))
		config->mac_capabilities |= MAC_ASYM_PAUSE | MAC_SYM_PAUSE;

may be sensible?

It brings up another question: if the port supports this register, but
we aren't using one of the rgmii, mii or revmii modes, should we be
clearing the pause bits in this register if we're telling the system
that pause isn't supported, or does the hardware not look at this RGMII
register unless its in one of those three modes?

Thanks.

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