Re: [PATCH RFC net-next 22/23] net: dsa: add driver for MaxLinear GSW1xx switch family

[Daniel Golle <daniel@makrotopia.org>]

On Thu, Aug 21, 2025 at 06:53:22PM +0000, Sverdlin, Alexander wrote:
> Hi Daniel,
> 
> On Sat, 2025-08-16 at 20:57 +0100, Daniel Golle wrote:
> > Add driver for the MaxLinear GSW1xx family of Ethernet switch ICs which
> > are based on the same IP as the Lantiq/Intel GSWIP found in the Lantiq VR9
> > and Intel GRX MIPS router SoCs. The main difference is that instead of
> > using memory-mapped I/O to communicate with the host CPU these ICs are
> > connected via MDIO (or SPI, which isn't supported by this driver).
> > Implement the regmap API to access the switch registers over MDIO to allow
> > reusing lantiq_gswip_common for all core functionality.
> > 
> > The GSW1xx also comes with a SerDes port capable of 1000Base-X, SGMII and
> > 2500Base-X, which can either be used to connect an external PHY or SFP
> > cage, or as the CPU port. Support for the SerDes interface is implemented
> > in this driver using the phylink_pcs interface.
> 
> ...
> 
> > --- /dev/null
> > +++ b/drivers/net/dsa/mxl-gsw1xx.c
> 
> ...
> 
> > static int gsw1xx_sgmii_pcs_config(struct phylink_pcs *pcs,
> > +				   unsigned int neg_mode,
> > +				   phy_interface_t interface,
> > +				   const unsigned long *advertising,
> > +				   bool permit_pause_to_mac)
> > +{
> > +	struct gsw1xx_priv *priv = sgmii_pcs_to_gsw1xx(pcs);
> > +	bool sgmii_mac_mode = dsa_is_user_port(priv->gswip.ds, GSW1XX_SGMII_PORT);
> > +	u16 txaneg, anegctl, val, nco_ctrl;
> > +	int ret;
> > +
> > +	/* Assert and deassert SGMII shell reset */
> > +	ret = regmap_set_bits(priv->shell, GSW1XX_SHELL_RST_REQ,
> > +			      GSW1XX_RST_REQ_SGMII_SHELL);
> 
> Can this be moved into gsw1xx_probe() maybe?
> 
> The thing is, if the switch is bootstrapped in
> "Self-start Mode: Managed Switch Sub-Mode", SGMII will be already
> brought out of reset (by bootloader?) (GSWIP_CFG register), refer
> to "Table 12 Registers Configuration for Self-start Mode: Managed Switch Sub-Mode"
> in datasheet. And nobody would disable SGMII if it's unused otherwise.

What you say is true if the SGMII interface is used as the CPU port or
to connect a (1000M/100M/10M) PHY. However, it can also be used to connect
SFP modules, which can be hot-plugged. Or a 2500M/1000M/100M/10M PHY which
requires switching to 2500Base-X mode in case of a 2500M link on the UTP
interface comes up, but uses SGMII for all lower speeds.

We can probably do this similar to drivers/net/pcs/pcs-mtk-lynxi.c and
only do a full reconf including reset if there are major changes which
actually require that, but as the impact is minimal and the vendor
implementation also carries out a reset as the first thing when
configuring the SGMII interface, I'd just keep it like that for now.
Optimization can come later if actually required.

Another good thing would probably be to implement pcs_enable() and
pcs_disable() ops which put the whole SGMII port into a low-power state
(stopping clocks, maybe asserting reset as you suggest...) and bring it
back up. However, also this can be done after initial support has been
merged and verified to work in all cases (I only got the MxL8611x
evaluation board on which GSW145 is connected to MxL86111, so I can't
really do anything too fancy with the SGMII interface other than making
sure it works with that PHY with both, enabled and disabled SGMII in-band
negotiation).

> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = regmap_clear_bits(priv->shell, GSW1XX_SHELL_RST_REQ,
> > +				GSW1XX_RST_REQ_SGMII_SHELL);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Hardware Bringup FSM Enable  */
> > +	ret = regmap_write(priv->sgmii, GSW1XX_SGMII_PHY_HWBU_CTRL,
> > +			   GSW1XX_SGMII_PHY_HWBU_CTRL_EN_HWBU_FSM |
> > +			   GSW1XX_SGMII_PHY_HWBU_CTRL_HW_FSM_EN);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Configure SGMII PHY Receiver */
> > +	val = FIELD_PREP(GSW1XX_SGMII_PHY_RX0_CFG2_EQ,
> > +			 GSW1XX_SGMII_PHY_RX0_CFG2_EQ_DEF) |
> > +	      GSW1XX_SGMII_PHY_RX0_CFG2_LOS_EN |
> > +	      GSW1XX_SGMII_PHY_RX0_CFG2_TERM_EN |
> > +	      FIELD_PREP(GSW1XX_SGMII_PHY_RX0_CFG2_FILT_CNT,
> > +			 GSW1XX_SGMII_PHY_RX0_CFG2_FILT_CNT_DEF);
> > +
> > +	// if (!priv->dts.sgmii_rx_invert)
>         ^^
> There is still a room for some cleanup ;-)

Ooops... I forgot about that, it should become a vendor DT property.