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 08:13:24PM +0000, Sverdlin, Alexander wrote:
> Hello Daniel,
> 
> On Thu, 2025-08-21 at 20:18 +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.
> 
> I'm actually concerned about use-cases where SGMII is unused.
> In "Self-start Mode" SGMII block is being brought up and driver will never disable it.
> I'm not proposing to move the de-assertion of the reset, but either
> the assertion can be done unconditionally somewhere around probe
> or struct dsa_switch_ops::setup callback or the assertion can remain
> here and be duplicated somewhere around init.

Lets assert the SGMII in the probe() function and let .pcs_enable() and
.pcs_disable() handle deassertion and assertion at runtime. That's easy
and obvious, and makes sure the SGMII reset is always asserted if the
SGMII unit isn't used. We can later optmize more and also stop clocks
or do whatever MaxLinear folks are telling us would be good to further
reduce power consumption and potentially also EM noise.

> 
> > 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.
> 
> Sure, it goes a bit beyond basic support as it's a power consumption
> optimization, but I thought I'll bring this up now as the re-spin will happen
> anyway and if you agree on moving the reset assertion, then later patching
> will not be required.

Convinced me ;)