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

["Sverdlin, Alexander" <alexander.sverdlin@siemens.com>]

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.

> +	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 ;-)

> +		val |= GSW1XX_SGMII_PHY_RX0_CFG2_INVERT;
> +
> +	ret = regmap_write(priv->sgmii, GSW1XX_SGMII_PHY_RX0_CFG2, val);
> +	if (ret < 0)
> +		return ret;
> +

...

> +static int gsw1xx_probe(struct mdio_device *mdiodev)
> +{
> +	struct device *dev = &mdiodev->dev;
> +	struct gsw1xx_priv *priv;
> +	struct dsa_switch *ds;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->mdio_dev = mdiodev;
> +	priv->smdio_badr = GSW1XX_SMDIO_BADR_UNKNOWN;
> +
> +	priv->gswip.dev = dev;
> +	priv->gswip.hw_info = of_device_get_match_data(dev);
> +	if (!priv->gswip.hw_info)
> +		return -EINVAL;
> +
> +	mutex_init(&priv->gswip.pce_table_lock);
> +
> +	priv->gswip.gswip = gsw1xx_regmap_init(priv, "switch",
> +					       GSW1XX_SWITCH_BASE, 0xfff);
> +	if (IS_ERR(priv->gswip.gswip))
> +		return PTR_ERR(priv->gswip.gswip);
> +
> +	priv->gswip.mdio = gsw1xx_regmap_init(priv, "mdio", GSW1XX_MMDIO_BASE,
> +					      0xff);
> +	if (IS_ERR(priv->gswip.mdio))
> +		return PTR_ERR(priv->gswip.mdio);
> +
> +	priv->gswip.mii = gsw1xx_regmap_init(priv, "mii", GSW1XX_RGMII_BASE,
> +					     0xff);
> +	if (IS_ERR(priv->gswip.mii))
> +		return PTR_ERR(priv->gswip.mii);
> +
> +	priv->sgmii = gsw1xx_regmap_init(priv, "sgmii", GSW1XX_SGMII_BASE,
> +					 0xfff);
> +	if (IS_ERR(priv->sgmii))
> +		return PTR_ERR(priv->sgmii);
> +
> +	priv->gpio = gsw1xx_regmap_init(priv, "gpio", GSW1XX_GPIO_BASE,
> +					0xff);
> +	if (IS_ERR(priv->gpio))
> +		return PTR_ERR(priv->gpio);
> +
> +	priv->clk = gsw1xx_regmap_init(priv, "clk", GSW1XX_CLK_BASE, 0xff);
> +	if (IS_ERR(priv->clk))
> +		return PTR_ERR(priv->clk);
> +
> +	priv->shell = gsw1xx_regmap_init(priv, "shell", GSW1XX_SHELL_BASE,
> +					 0xff);
> +	if (IS_ERR(priv->shell))
> +		return PTR_ERR(priv->shell);
> +
> +	priv->sgmii_pcs.ops = &gsw1xx_sgmii_pcs_ops;
> +	priv->sgmii_pcs.poll = 1;
> +	__set_bit(PHY_INTERFACE_MODE_SGMII,
> +		  priv->sgmii_pcs.supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +		  priv->sgmii_pcs.supported_interfaces);
> +	if (priv->gswip.hw_info->supports_2500m)
> +		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> +			  priv->sgmii_pcs.supported_interfaces);
> +
> +	ret = regmap_read(priv->gswip.gswip, GSWIP_VERSION, &priv->gswip.version);
> +	if (ret < 0)
> +		return ret;
> +
> +	ds = devm_kzalloc(dev, sizeof(*ds), GFP_KERNEL);
> +	if (!ds)
> +		return -ENOMEM;
> +
> +	priv->gswip.ds = ds;
> +
> +	ret = gswip_allocate_vlans(&priv->gswip);
> +	if (ret)
> +		return ret;
> +
> +	ds->dev = dev;
> +	ds->num_ports = GSW1XX_PORTS;
> +	ds->priv = &priv->gswip;
> +	ds->ops = &gswip_switch_ops;
> +	ds->phylink_mac_ops = &gswip_phylink_mac_ops;
> +	ds->fdb_isolation = true;
> +
> +	/* configure GPIO pin-mux for MMDIO in case of external PHY connected to
> +	 * SGMII or RGMII as slave interface
> +	 */
> +	regmap_set_bits(priv->gpio, GPIO_ALTSEL0, 3);
> +	regmap_set_bits(priv->gpio, GPIO_ALTSEL1, 3);
> +
> +	dev_set_drvdata(dev, ds);
> +
> +	ret = dsa_register_switch(ds);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "%s: Error %d register DSA switch\n",
> +				__func__, ret);
> +		return ret;
> +	}
> +
> +	ret = gswip_validate_cpu_port(ds);
> +	if (ret)
> +		goto disable_switch;
> +
> +	dev_info(dev, "probed GSWIP version %lx mod %lx\n",
> +		 (priv->gswip.version & GSWIP_VERSION_REV_MASK) >> GSWIP_VERSION_REV_SHIFT,
> +		 (priv->gswip.version & GSWIP_VERSION_MOD_MASK) >> GSWIP_VERSION_MOD_SHIFT);
> +
> +	return 0;
> +
> +disable_switch:
> +	gswip_disable_switch(&priv->gswip);
> +	dsa_unregister_switch(ds);
> +
> +	return ret;
> +}

...

> +static struct mdio_driver gsw1xx_driver = {
> +	.probe		= gsw1xx_probe,
> +	.remove		= gsw1xx_remove,
> +	.shutdown	= gsw1xx_shutdown,
> +	.mdiodrv.driver	= {
> +		.name = "mxl-gsw1xx",
> +		.of_match_table = gsw1xx_of_match,
> +	},
> +};

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com