[RFC,net-next,2/2] net: dsa: Add driver for Maxlinear GSW1XX switch

  Add initial framework for Maxlinear's GSW1xx switch and
currently only GSW145 in MDIO managed mode is supported.

Signed-off-by: Camel Guo <camel.guo@axis.com>
---
 MAINTAINERS                   |   3 +
 drivers/net/dsa/Kconfig       |  16 +
 drivers/net/dsa/Makefile      |   2 +
 drivers/net/dsa/gsw1xx.h      |  27 ++
 drivers/net/dsa/gsw1xx_core.c | 823 ++++++++++++++++++++++++++++++++++
 drivers/net/dsa/gsw1xx_mdio.c | 128 ++++++
 6 files changed, 999 insertions(+)
 create mode 100644 drivers/net/dsa/gsw1xx.h
 create mode 100644 drivers/net/dsa/gsw1xx_core.c
 create mode 100644 drivers/net/dsa/gsw1xx_mdio.c
  

On 25/10/2022 09:52, Camel Guo wrote:
> Add initial framework for Maxlinear's GSW1xx switch and
> currently only GSW145 in MDIO managed mode is supported.
> 
> Signed-off-by: Camel Guo <camel.guo@axis.com>
> ---

(...)

> +	priv->ds->dev = dev;
> +	priv->ds->num_ports = priv->hw_info->max_ports;
> +	priv->ds->priv = priv;
> +	priv->ds->ops = &gsw1xx_switch_ops;
> +	priv->dev = dev;
> +	version = gsw1xx_switch_r(priv, GSW1XX_IP_VERSION);
> +
> +	np = dev->of_node;
> +	switch (version) {
> +	case GSW1XX_IP_VERSION_2_3:
> +		if (!of_device_is_compatible(np, "mxl,gsw145-mdio"))
> +			return -EINVAL;
> +		break;
> +	default:
> +		dev_err(dev, "unknown GSW1XX_IP version: 0x%x", version);
> +		return -ENOENT;
> +	}
> +
> +	/* bring up the mdio bus */
> +	mdio_np = of_get_child_by_name(np, "mdio");
> +	if (!mdio_np) {
> +		dev_err(dev, "missing child mdio node\n");
> +		return -EINVAL;
> +	}
> +
> +	err = gsw1xx_mdio(priv, mdio_np);
> +	if (err) {
> +		dev_err(dev, "mdio probe failed\n");

dev_err_probe()

> +		goto put_mdio_node;
> +	}
> +
> +	err = dsa_register_switch(priv->ds);
> +	if (err) {
> +		dev_err(dev, "dsa switch register failed: %i\n", err);


dev_err_probe()

> +		goto mdio_bus;
> +	}
> +	if (!dsa_is_cpu_port(priv->ds, priv->hw_info->cpu_port)) {
> +		dev_err(dev,
> +			"wrong CPU port defined, HW only supports port: %i",
> +			priv->hw_info->cpu_port);
> +		err = -EINVAL;
> +		goto disable_switch;
> +	}
> +
> +	dev_set_drvdata(dev, priv);
> +
> +	return 0;
> +
> +disable_switch:
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_GLOB_ENABLE, 0, GSW1XX_MDIO_GLOB);
> +	dsa_unregister_switch(priv->ds);
> +mdio_bus:
> +	if (mdio_np) {
> +		mdiobus_unregister(priv->ds->slave_mii_bus);
> +		mdiobus_free(priv->ds->slave_mii_bus);
> +	}
> +put_mdio_node:
> +	of_node_put(mdio_np);
> +	return err;
> +}
> +EXPORT_SYMBOL(gsw1xx_probe);
> +
> +void gsw1xx_remove(struct gsw1xx_priv *priv)
> +{
> +	if (!priv)
> +		return;
> +
> +	/* disable the switch */
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_GLOB_ENABLE, 0, GSW1XX_MDIO_GLOB);
> +
> +	dsa_unregister_switch(priv->ds);
> +
> +	if (priv->ds->slave_mii_bus) {
> +		mdiobus_unregister(priv->ds->slave_mii_bus);
> +		of_node_put(priv->ds->slave_mii_bus->dev.of_node);
> +		mdiobus_free(priv->ds->slave_mii_bus);
> +	}
> +
> +	dev_set_drvdata(priv->dev, NULL);
> +}
> +EXPORT_SYMBOL(gsw1xx_remove);
> +
> +void gsw1xx_shutdown(struct gsw1xx_priv *priv)
> +{
> +	if (!priv)
> +		return;
> +
> +	/* disable the switch */
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_GLOB_ENABLE, 0, GSW1XX_MDIO_GLOB);
> +
> +	dsa_switch_shutdown(priv->ds);
> +
> +	dev_set_drvdata(priv->dev, NULL);
> +}
> +EXPORT_SYMBOL(gsw1xx_shutdown);

1. EXPORT_SYMBOL_GPL
2. Why do you do it in the first place? It's one driver, no need for
building two modules. Same applies to other places.

> +
> +static const struct regmap_range gsw1xx_valid_regs[] = {
> +	/* GSWIP Core Registers */
> +	regmap_reg_range(GSW1XX_IP_BASE_ADDR,
> +			 GSW1XX_IP_BASE_ADDR + GSW1XX_IP_REG_LEN),
> +	/* Top Level PDI Registers, MDIO Master Reigsters */
> +	regmap_reg_range(GSW1XX_MDIO_BASE_ADDR,
> +			 GSW1XX_MDIO_BASE_ADDR + GSW1XX_MDIO_REG_LEN),
> +};
> +
> +const struct regmap_access_table gsw1xx_register_set = {
> +	.yes_ranges = gsw1xx_valid_regs,
> +	.n_yes_ranges = ARRAY_SIZE(gsw1xx_valid_regs),
> +};
> +EXPORT_SYMBOL(gsw1xx_register_set);
> +
> +MODULE_AUTHOR("Camel Guo <camel.guo@axis.com>");
> +MODULE_DESCRIPTION("Core Driver for MaxLinear GSM1XX ethernet switch");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/dsa/gsw1xx_mdio.c b/drivers/net/dsa/gsw1xx_mdio.c
> new file mode 100644
> index 000000000000..8328001041ed
> --- /dev/null
> +++ b/drivers/net/dsa/gsw1xx_mdio.c
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MaxLinear switch driver for GSW1XX in MDIO managed mode
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
> +#include <linux/of.h>
> +
> +#include "gsw1xx.h"
> +
> +#define GSW1XX_SMDIO_TARGET_BASE_ADDR_REG	0x1F
> +
> +static int gsw1xx_mdio_write(void *ctx, uint32_t reg, uint32_t val)
> +{
> +	struct mdio_device *mdiodev = (struct mdio_device *)ctx;
> +	int ret = 0;
> +
> +	mutex_lock_nested(&mdiodev->bus->mdio_lock, MDIO_MUTEX_NESTED);
> +
> +	ret = mdiodev->bus->write(mdiodev->bus, mdiodev->addr,
> +				  GSW1XX_SMDIO_TARGET_BASE_ADDR_REG, reg);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = mdiodev->bus->write(mdiodev->bus, mdiodev->addr, 0, val);
> +
> +out:
> +	mutex_unlock(&mdiodev->bus->mdio_lock);
> +
> +	return ret;
> +}
> +
> +static int gsw1xx_mdio_read(void *ctx, uint32_t reg, uint32_t *val)
> +{
> +	struct mdio_device *mdiodev = (struct mdio_device *)ctx;
> +	int ret = 0;
> +
> +	mutex_lock_nested(&mdiodev->bus->mdio_lock, MDIO_MUTEX_NESTED);
> +
> +	ret = mdiodev->bus->write(mdiodev->bus, mdiodev->addr,
> +				  GSW1XX_SMDIO_TARGET_BASE_ADDR_REG, reg);
> +	if (ret < 0)
> +		goto out;
> +
> +	*val = mdiodev->bus->read(mdiodev->bus, mdiodev->addr, 0);
> +
> +out:
> +	mutex_unlock(&mdiodev->bus->mdio_lock);
> +
> +	return ret;
> +}
> +
> +static const struct regmap_config gsw1xx_mdio_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 16,
> +	.reg_stride = 1,
> +
> +	.disable_locking = true,
> +
> +	.volatile_table = &gsw1xx_register_set,
> +	.wr_table = &gsw1xx_register_set,
> +	.rd_table = &gsw1xx_register_set,
> +
> +	.reg_read = gsw1xx_mdio_read,
> +	.reg_write = gsw1xx_mdio_write,
> +
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +static int gsw1xx_mdio_probe(struct mdio_device *mdiodev)
> +{
> +	struct gsw1xx_priv *priv;
> +	struct device *dev = &mdiodev->dev;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->regmap = devm_regmap_init(dev, NULL, mdiodev,
> +					&gsw1xx_mdio_regmap_config);
> +	if (IS_ERR(priv->regmap)) {
> +		ret = PTR_ERR(priv->regmap);
> +		dev_err(dev, "regmap init failed: %d\n", ret);
> +		return ret;

return dev_err_probe().

> +	}
> +
> +	return gsw1xx_probe(priv, dev);
> +}
> +
> +static void gsw1xx_mdio_remove(struct mdio_device *mdiodev)
> +{
> +	gsw1xx_remove(dev_get_drvdata(&mdiodev->dev));
> +}
> +
> +static void gsw1xx_mdio_shutdown(struct mdio_device *mdiodev)
> +{
> +	gsw1xx_shutdown(dev_get_drvdata(&mdiodev->dev));
> +}
> +
> +static const struct gsw1xx_hw_info gsw145_hw_info = {
> +	.max_ports = 6,
> +	.cpu_port = 5,
> +};
> +
> +static const struct of_device_id gsw1xx_mdio_of_match[] = {
> +	{ .compatible = "mxl,gsw145-mdio", .data = &gsw145_hw_info },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, gsw1xx_mdio_of_match);
> +
> +static struct mdio_driver gsw1xx_mdio_driver = {
> +	.probe  = gsw1xx_mdio_probe,
> +	.remove = gsw1xx_mdio_remove,
> +	.shutdown = gsw1xx_mdio_shutdown,
> +	.mdiodrv.driver = {
> +		.name = "GSW1XX_MDIO",
> +		.of_match_table = of_match_ptr(gsw1xx_mdio_of_match),

of_match_ptr requires maybe_unused. Or just drop it.

Best regards,
Krzysztof
  

> +++ b/drivers/net/dsa/Kconfig
> @@ -122,4 +122,20 @@ config NET_DSA_VITESSE_VSC73XX_PLATFORM
>  	  This enables support for the Vitesse VSC7385, VSC7388, VSC7395
>  	  and VSC7398 SparX integrated ethernet switches, connected over
>  	  a CPU-attached address bus and work in memory-mapped I/O mode.
> +
> +config NET_DSA_MXL_GSW1XX
> +	tristate
> +	select REGMAP
> +	help
> +	  This enables support for the Maxlinear GSW1XX integrated ethernet
> +	  switch chips.
> +
> +config NET_DSA_MXL_GSW1XX_MDIO
> +	tristate "MaxLinear GSW1XX ethernet switch in MDIO managed mode"

Please keep this file sorted on the tristate text.

In general, it is wrong to insert at the end. That causes the most
conflicts. By keeping lists like this sorted, inserts tend to be
separated, and so don't cause conflicts. Also, keeping it sorted helps
users actually find the configuration option they want.

> +	select NET_DSA_MXL_GSW1XX
> +	select FIXED_PHY
> +	help
> +	  This enables access functions if the MaxLinear GSW1XX is configured
> +	  for MDIO managed mode.
> +
>  endmenu
> diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
> index 16eb879e0cb4..022fc661107b 100644
> --- a/drivers/net/dsa/Makefile
> +++ b/drivers/net/dsa/Makefile
> @@ -15,6 +15,8 @@ obj-$(CONFIG_NET_DSA_SMSC_LAN9303_MDIO) += lan9303_mdio.o
>  obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX) += vitesse-vsc73xx-core.o
>  obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_PLATFORM) += vitesse-vsc73xx-platform.o
>  obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_SPI) += vitesse-vsc73xx-spi.o
> +obj-$(CONFIG_NET_DSA_MXL_GSW1XX) += gsw1xx_core.o
> +obj-$(CONFIG_NET_DSA_MXL_GSW1XX_MDIO) += gsw1xx_mdio.o

This file is sorted as well.

> diff --git a/drivers/net/dsa/gsw1xx.h b/drivers/net/dsa/gsw1xx.h

If you think there is going to be a gsw1xx_spi.c and gsw1xx_uart.c, i
would suggest you move into a subdirectory.

> +static u32 gsw1xx_switch_r(struct gsw1xx_priv *priv, u32 offset)
> +{
> +	int ret = 0;
> +	u32 val = 0;
> +
> +	ret = regmap_read(priv->regmap, GSW1XX_IP_BASE_ADDR + offset, &val);
> +
> +	return ret < 0 ? (u32)ret : val;

A negative error code becomes positive? So how do you then know it is
an error code?

The general pattern is you pass the error code and the register value
in two separate ways. Generally, the error code as the return value,
as an int, and the register value via a pointer. Just as regmap_read()
does.

> +}
> +
> +static void gsw1xx_switch_w(struct gsw1xx_priv *priv, u32 val, u32 offset)
> +{
> +	regmap_write(priv->regmap, GSW1XX_IP_BASE_ADDR + offset, val);
> +}

Return the error code from regmap_write().

In general, don't ignore errors. Return them up the call stack.

> +static u32 gsw1xx_switch_r_timeout(struct gsw1xx_priv *priv, u32 offset,
> +				   u32 cleared)
> +{
> +	u32 val;
> +
> +	return read_poll_timeout(gsw1xx_switch_r, val, (val & cleared) == 0, 20,
> +				 50000, true, priv, offset);
> +}
> +
> +static int gsw1xx_mdio_poll(struct gsw1xx_priv *priv)
> +{
> +	int cnt = 100;
> +
> +	while (likely(cnt--)) {
> +		u32 ctrl = gsw1xx_mdio_r(priv, GSW1XX_MDIO_CTRL);
> +
> +		if ((ctrl & GSW1XX_MDIO_CTRL_BUSY) == 0)
> +			return 0;
> +		usleep_range(20, 40);
> +	}
> +
> +	return -ETIMEDOUT;

It looks like this could be implemented using read_poll_timeout() as
well?

> +}
> +
> +static int gsw1xx_mdio_wr(struct mii_bus *bus, int addr, int reg, u16 val)
> +{
> +	struct gsw1xx_priv *priv = bus->priv;
> +	int err;

Please check for C45 and return -EOPNOTSUPP.

> +
> +	err = gsw1xx_mdio_poll(priv);
> +	if (err) {
> +		dev_err(&bus->dev, "timeout while waiting for MDIO bus\n");
> +		return err;
> +	}
> +
> +	gsw1xx_mdio_w(priv, val, GSW1XX_MDIO_WRITE);
> +	gsw1xx_mdio_w(priv,
> +		      GSW1XX_MDIO_CTRL_WR |
> +			      ((addr & GSW1XX_MDIO_CTRL_PHYAD_MASK)
> +			       << GSW1XX_MDIO_CTRL_PHYAD_SHIFT) |
> +			      (reg & GSW1XX_MDIO_CTRL_REGAD_MASK),
> +		      GSW1XX_MDIO_CTRL);
> +
> +	return 0;
> +}
> +
> +static int gsw1xx_mdio_rd(struct mii_bus *bus, int addr, int reg)
> +{
> +	struct gsw1xx_priv *priv = bus->priv;
> +	int err;

Same here.

> +static int gsw1xx_port_enable(struct dsa_switch *ds, int port,
> +			      struct phy_device *phydev)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +
> +	if (!dsa_is_user_port(ds, port))
> +		return 0;
> +
> +	/* RMON Counter Enable for port */
> +	gsw1xx_switch_w(priv, GSW1XX_IP_BM_PCFG_CNTEN,
> +			GSW1XX_IP_BM_PCFGp(port));
> +
> +	/* enable port fetch/store dma */
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_FDMA_PCTRL_EN,
> +			   GSW1XX_IP_FDMA_PCTRLp(port));
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_SDMA_PCTRL_EN,
> +			   GSW1XX_IP_SDMA_PCTRLp(port));
> +
> +	if (!dsa_is_cpu_port(ds, port)) {

How can this be true given the previous check for dsa_is_user_port()?


> +static int gsw1xx_setup(struct dsa_switch *ds)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +	unsigned int cpu_port = priv->hw_info->cpu_port;
> +	int i;
> +	int err;

Reverse christmass tree, which means you need to delay assigning
cpu_port into the body of the function.

> +int gsw1xx_probe(struct gsw1xx_priv *priv, struct device *dev)
> +{
> +	struct device_node *np, *mdio_np;
> +	int err;
> +	u32 version;

Reverse christmass tree.

> +
> +	if (!priv->regmap || IS_ERR(priv->regmap))
> +		return -EINVAL;
> +
> +	priv->hw_info = of_device_get_match_data(dev);
> +	if (!priv->hw_info)
> +		return -EINVAL;
> +
> +	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
> +	if (!priv->ds)
> +		return -ENOMEM;
> +
> +	priv->ds->dev = dev;
> +	priv->ds->num_ports = priv->hw_info->max_ports;
> +	priv->ds->priv = priv;
> +	priv->ds->ops = &gsw1xx_switch_ops;
> +	priv->dev = dev;
> +	version = gsw1xx_switch_r(priv, GSW1XX_IP_VERSION);
> +
> +	np = dev->of_node;
> +	switch (version) {
> +	case GSW1XX_IP_VERSION_2_3:
> +		if (!of_device_is_compatible(np, "mxl,gsw145-mdio"))
> +			return -EINVAL;
> +		break;
> +	default:
> +		dev_err(dev, "unknown GSW1XX_IP version: 0x%x", version);
> +		return -ENOENT;

I think ENODEV is more appropriate.

I noticed there is no tagging protocol defined. How are frames
direction out a specific port?

I've also not yet looked at the overlap with lantiq_gswip.c.

     Andrew
  

> > +EXPORT_SYMBOL(gsw1xx_shutdown);
> 
> 1. EXPORT_SYMBOL_GPL
> 2. Why do you do it in the first place? It's one driver, no need for
> building two modules. Same applies to other places.

At some point, there is likely to be SPI and UART support. The
communication with the chip and the core driver will then be in
separate modules. But i agree this is not needed at the moment when it
is all linked into one.

   Andrew
  

On 10/25/22 16:56, Andrew Lunn wrote:
>> > +EXPORT_SYMBOL(gsw1xx_shutdown);
>> 
>> 1. EXPORT_SYMBOL_GPL
>> 2. Why do you do it in the first place? It's one driver, no need for
>> building two modules. Same applies to other places.
> 
> At some point, there is likely to be SPI and UART support. The
> communication with the chip and the core driver will then be in
> separate modules. But i agree this is not needed at the moment when it
> is all linked into one.

Do you suggest that currently we put the content of gsw1xx_core.c and 
gsw1xx_mdio.c into one file and split them later at the time when 
another management mode (e,g: spi) is added?

Actually I kinda hope this piece of code (gsw1xx_core.c) can be reused 
in lantiq_gswip in short future.

I tried to use the logic in lantiq_gswip directly on the gsw145 chip. 
Unfortunately it did not work. It seems that the GSWIP part changes a lot.

> 
>     Andrew
  

On Thu, Oct 27, 2022 at 08:35:17AM +0200, Camel Guo wrote:
> On 10/25/22 16:56, Andrew Lunn wrote:
> > > > +EXPORT_SYMBOL(gsw1xx_shutdown);
> > > 
> > > 1. EXPORT_SYMBOL_GPL
> > > 2. Why do you do it in the first place? It's one driver, no need for
> > > building two modules. Same applies to other places.
> > 
> > At some point, there is likely to be SPI and UART support. The
> > communication with the chip and the core driver will then be in
> > separate modules. But i agree this is not needed at the moment when it
> > is all linked into one.
> 
> Do you suggest that currently we put the content of gsw1xx_core.c and
> gsw1xx_mdio.c into one file and split them later at the time when another
> management mode (e,g: spi) is added?

No, keep them separate. But you can remove the module exports, and
just link them together at compile time into one module. For forward
compatibility, call that module gsw1xx_mdio.ko. In the future,
somebody can then split it apart again, add gsw1xx_spi.ko, and module
dependencies should cause the gsw1xx_core.ko to be loaded first.

     Andrew
  

> >> +}
> >> +
> >> +static int gsw1xx_mdio_wr(struct mii_bus *bus, int addr, int reg, u16 val)
> >> +{
> >> +     struct gsw1xx_priv *priv = bus->priv;
> >> +     int err;
> >
> > Please check for C45 and return -EOPNOTSUPP.
> 
> Maybe not need. According to the datasheet of gsw145 "The interface uses the
> serial protocol defined by IEEE 802.3, clause 22.",  I think it is enough to
> add "ds->slave_mii_bus->probe_capabilities = MDIOBUS_C22" into gsw1xx_mdio. It
> probe_capabilities is static and never changes.

probe_capabilities only limits probing the bus when it is
registered. It does not prevent C45 transfers from being requested by
PHY drivers. And there are some funky PHY drivers which mix C22 and
C45. They can probe via C22 and then use C45. So it is much better to
check and return an error if requested to do something which the
hardware cannot do. Also, if you don't check, and convert a C45
request into a C22 request, you often end up with really odd accesses,
depending on the hardware, reads could become writes, etc.

> > I noticed there is no tagging protocol defined. How are frames
> > direction out a specific port?
> 
> Yes, this chip supports Special Tags which should be enabled, but unfortunately
> I have no make it work.

You need to make this work. You added support to set the port spanning
tree status. But that makes no sense if you cannot send/receive bridge
PDUs out specific ports, etc.

> The chip in my dev board works in self-start, managed switch mode. So far, it
> works fine on this board.
> 
> >
> > I've also not yet looked at the overlap with lantiq_gswip.c.
> 
> The version of GSWIP changes and also the management interface of
> it is memory-mapped io. I tried to use the same logic in my gsw145 chip
> (with mdio interface update), lots of parts (e.g: fdb, vlan) don't work
> at all.

There has been past attempts at a driver for this hardware and it was
argued that they are sufficiently different that a new driver was
needed. As i said, i've not compared the code yet, so i cannot comment
on that yet.

   Andrew
  

On 27/10/2022 02:35, Camel Guo wrote:
utdown(priv->ds);
>  >> +
>  >> +     dev_set_drvdata(priv->dev, NULL);
>  >> +}
>  >> +EXPORT_SYMBOL(gsw1xx_shutdown);
>  >
>  > 1. EXPORT_SYMBOL_GPL
> 
> Will update in v2
> 
>  > 2. Why do you do it in the first place? It's one driver, no need for
>  > building two modules. Same applies to other places.
> 
> All stuff in drivers/net/dsa/gsw1xx_core.c is supposed to be generic and
> totally independent of the actual management interface (mdio, spi, uart,
> maybe memory-mapped IO). This way, I think the gsw1xx_core.ko can be 
> reused in
> gsw1xx_spi.ko, gsw1xx_uart.ko and so on.
> 
> I don't how similar the chips that lantiq_gswip.c supports are due to
> no datasheet. If not too much, maybe someone the gsw1xx_core.ko can also
> be reused in lantiq_gswip as well.

Keep the files separate but there is no need to make two modules and
exprt this. Your patch should stand on its own, not prepare for some
imaginary future work which might or might not bring more modules.

Once these future modules appear, it will be easy to change existing
file to a module.

Best regards,
Krzysztof
  

Hi Camel,

I took a very superficial look. I'm only interested in the API
perspective for now. Some comments.

On Tue, Oct 25, 2022 at 03:52:41PM +0200, Camel Guo wrote:
> +static int gsw1xx_port_enable(struct dsa_switch *ds, int port,
> +			      struct phy_device *phydev)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +
> +	if (!dsa_is_user_port(ds, port))
> +		return 0;
> +
> +	/* RMON Counter Enable for port */
> +	gsw1xx_switch_w(priv, GSW1XX_IP_BM_PCFG_CNTEN,
> +			GSW1XX_IP_BM_PCFGp(port));
> +
> +	/* enable port fetch/store dma */
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_FDMA_PCTRL_EN,
> +			   GSW1XX_IP_FDMA_PCTRLp(port));
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_SDMA_PCTRL_EN,
> +			   GSW1XX_IP_SDMA_PCTRLp(port));
> +
> +	if (!dsa_is_cpu_port(ds, port)) {
> +		u32 mdio_phy = 0;
> +
> +		if (phydev)
> +			mdio_phy =
> +				phydev->mdio.addr & GSW1XX_MDIO_PHY_ADDR_MASK;
> +
> +		gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_ADDR_MASK, mdio_phy,
> +				 GSW1XX_MDIO_PHYp(port));
> +	}
> +
> +	return 0;
> +}
> +
> +static void gsw1xx_port_disable(struct dsa_switch *ds, int port)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +
> +	if (!dsa_is_user_port(ds, port))
> +		return;
> +
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_FDMA_PCTRL_EN, 0,
> +			   GSW1XX_IP_FDMA_PCTRLp(port));
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_SDMA_PCTRL_EN, 0,
> +			   GSW1XX_IP_SDMA_PCTRLp(port));
> +}
> +
> +static int gsw1xx_setup(struct dsa_switch *ds)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +	unsigned int cpu_port = priv->hw_info->cpu_port;
> +	int i;
> +	int err;
> +
> +	gsw1xx_switch_w(priv, GSW1XX_IP_SWRES_R0, GSW1XX_IP_SWRES);
> +	usleep_range(5000, 10000);
> +	gsw1xx_switch_w(priv, 0, GSW1XX_IP_SWRES);
> +
> +	/* disable port fetch/store dma on all ports */
> +	for (i = 0; i < priv->hw_info->max_ports; i++)
> +		gsw1xx_port_disable(ds, i);
> +
> +	/* enable Switch */
> +	gsw1xx_mdio_mask(priv, 0, GSW1XX_MDIO_GLOB_ENABLE, GSW1XX_MDIO_GLOB);
> +
> +	gsw1xx_switch_w(priv, 0x7F, GSW1XX_IP_PCE_PMAP2);
> +	gsw1xx_switch_w(priv, 0x7F, GSW1XX_IP_PCE_PMAP3);
> +
> +	/* Deactivate MDIO PHY auto polling since it affects mmd read/write.
> +	 */
> +	gsw1xx_mdio_w(priv, 0x0, GSW1XX_MDIO_MDC_CFG0);
> +
> +	gsw1xx_switch_mask(priv, 1, GSW1XX_IP_MAC_CTRL_2_MLEN,
> +			   GSW1XX_IP_MAC_CTRL_2p(cpu_port));
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_BM_QUEUE_GCTRL_GL_MOD,
> +			   GSW1XX_IP_BM_QUEUE_GCTRL);
> +
> +	/* Flush MAC Table */
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_PCE_GCTRL_0_MTFL,
> +			   GSW1XX_IP_PCE_GCTRL_0);
> +	err = gsw1xx_switch_r_timeout(priv, GSW1XX_IP_PCE_GCTRL_0,
> +				      GSW1XX_IP_PCE_GCTRL_0_MTFL);
> +	if (err) {
> +		dev_err(priv->dev, "MAC flushing didn't finish\n");
> +		return err;
> +	}
> +
> +	gsw1xx_port_enable(ds, cpu_port, NULL);

DSA automatically calls this on the CPU port. You have this code which
ignores the call:

	if (!dsa_is_user_port(ds, port)) // which the CPU port isn't
		return 0;

so why do it?!

> +
> +	return 0;
> +}
> +
> +static enum dsa_tag_protocol gsw1xx_get_tag_protocol(struct dsa_switch *ds,
> +						     int port,
> +						     enum dsa_tag_protocol mp)
> +{
> +	return DSA_TAG_PROTO_NONE;

Nope, this won't fly for new drivers, please write a protocol driver for
your switch, or use something based on tag_8021q.c if the switch doesn't
support something natively.

> +}
> +
> +static void gsw1xx_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +	u32 stp_state;
> +
> +	switch (state) {
> +	case BR_STATE_DISABLED:
> +		gsw1xx_switch_mask(priv, GSW1XX_IP_SDMA_PCTRL_EN, 0,
> +				   GSW1XX_IP_SDMA_PCTRLp(port));
> +		return;
> +	case BR_STATE_BLOCKING:
> +	case BR_STATE_LISTENING:
> +		stp_state = GSW1XX_IP_PCE_PCTRL_0_PSTATE_LISTEN;
> +		break;
> +	case BR_STATE_LEARNING:
> +		stp_state = GSW1XX_IP_PCE_PCTRL_0_PSTATE_LEARNING;
> +		break;
> +	case BR_STATE_FORWARDING:
> +		stp_state = GSW1XX_IP_PCE_PCTRL_0_PSTATE_FORWARDING;
> +		break;
> +	default:
> +		dev_err(priv->dev, "invalid STP state: %d\n", state);
> +		return;
> +	}
> +
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_SDMA_PCTRL_EN,
> +			   GSW1XX_IP_SDMA_PCTRLp(port));
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_PCE_PCTRL_0_PSTATE_MASK, stp_state,
> +			   GSW1XX_IP_PCE_PCTRL_0p(port));
> +}
> +
> +static const struct dsa_switch_ops gsw1xx_switch_ops = {
> +	.get_tag_protocol	= gsw1xx_get_tag_protocol,
> +	.setup			= gsw1xx_setup,
> +	.set_mac_eee		= gsw1xx_set_mac_eee,
> +	.get_mac_eee		= gsw1xx_get_mac_eee,
> +	.port_enable		= gsw1xx_port_enable,
> +	.port_disable		= gsw1xx_port_disable,
> +	.port_stp_state_set	= gsw1xx_port_stp_state_set,

No need to implement .port_stp_state_set() if .port_bridge_join() is
absent. First get the support for standalone port mode right (hint, need
to disable address learning and forwarding between ports for standalone mode).

Look inside tools/testing/selftests/drivers/net/dsa/, a bunch of tests
should pass even with software forwarding. First make sure that switch
ports can ping each other through a cable in loopback. Then there's
no_forwarding.sh, a must have. I think bridge_vlan_aware.sh and
bridge_vlan_unaware.sh should also pass.

As far as local_termination.sh, that also tests for some optional
optimizations. You can run it, and the goal should be to get "OK" for
all tests. "FAIL" is also ok, if the reason is "reception succeeded,
but should have failed". What is not ok is "FAIL" with the reason
"reception failed". Something like this would be ok:

    TEST: br0: Unicast IPv4 to primary MAC address                      [ OK ]
    TEST: br0: Unicast IPv4 to macvlan MAC address                      [ OK ]
    TEST: br0: Unicast IPv4 to unknown MAC address                      [FAIL]
            reception succeeded, but should have failed
    TEST: br0: Unicast IPv4 to unknown MAC address, promisc             [ OK ]
    TEST: br0: Unicast IPv4 to unknown MAC address, allmulti            [FAIL]
            reception succeeded, but should have failed
    TEST: br0: Multicast IPv4 to joined group                           [ OK ]
    TEST: br0: Multicast IPv4 to unknown group                          [FAIL]
            reception succeeded, but should have failed
    TEST: br0: Multicast IPv4 to unknown group, promisc                 [ OK ]
    TEST: br0: Multicast IPv4 to unknown group, allmulti                [ OK ]
    TEST: br0: Multicast IPv6 to joined group                           [ OK ]
    TEST: br0: Multicast IPv6 to unknown group                          [FAIL]
            reception succeeded, but should have failed
    TEST: br0: Multicast IPv6 to unknown group, promisc                 [ OK ]
    TEST: br0: Multicast IPv6 to unknown group, allmulti                [ OK ]

The selftest results for unoffloaded mode will stand as a future guide
for regression testing when you add later support for offloading various
features. So please try to spend some time running them now, ask
questions if needed.

> +	.phylink_mac_link_down	= gsw1xx_phylink_mac_link_down,
> +	.phylink_mac_link_up	= gsw1xx_phylink_mac_link_up,
> +	.get_strings		= gsw1xx_get_strings,
> +	.get_ethtool_stats	= gsw1xx_get_ethtool_stats,
> +	.get_sset_count		= gsw1xx_get_sset_count,

New DSA drivers should first add support for:

	.get_stats64
	.get_pause_stats
	.get_rmon_stats
	.get_eth_ctrl_stats
	.get_eth_mac_stats
	.get_eth_phy_stats

Only if there remains something uncovered do we start talking about
unstructured stats.

> +};
> +
> +void gsw1xx_remove(struct gsw1xx_priv *priv)
> +{
> +	if (!priv)
> +		return;
> +
> +	/* disable the switch */
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_GLOB_ENABLE, 0, GSW1XX_MDIO_GLOB);
> +
> +	dsa_unregister_switch(priv->ds);
> +
> +	if (priv->ds->slave_mii_bus) {
> +		mdiobus_unregister(priv->ds->slave_mii_bus);
> +		of_node_put(priv->ds->slave_mii_bus->dev.of_node);
> +		mdiobus_free(priv->ds->slave_mii_bus);
> +	}
> +
> +	dev_set_drvdata(priv->dev, NULL);

dev_set_drvdata() is no longer required from remove().
Please keep it in shutdown() though.

> +}
> +EXPORT_SYMBOL(gsw1xx_remove);
> +static const struct regmap_range gsw1xx_valid_regs[] = {
> +	/* GSWIP Core Registers */
> +	regmap_reg_range(GSW1XX_IP_BASE_ADDR,
> +			 GSW1XX_IP_BASE_ADDR + GSW1XX_IP_REG_LEN),
> +	/* Top Level PDI Registers, MDIO Master Reigsters */

s/Reigsters/Registers/

> +	regmap_reg_range(GSW1XX_MDIO_BASE_ADDR,
> +			 GSW1XX_MDIO_BASE_ADDR + GSW1XX_MDIO_REG_LEN),
> +};
  

Hi Camel,
On Tue, 2022-10-25 at 15:52 +0200, Camel Guo wrote:
> Add initial framework for Maxlinear's GSW1xx switch and
> currently only GSW145 in MDIO managed mode is supported.
> 
> Signed-off-by: Camel Guo <camel.guo@axis.com>
> ---
>  MAINTAINERS                   |   3 +
>  drivers/net/dsa/Kconfig       |  16 +
>  drivers/net/dsa/Makefile      |   2 +
>  drivers/net/dsa/gsw1xx.h      |  27 ++
>  drivers/net/dsa/gsw1xx_core.c | 823
> ++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/gsw1xx_mdio.c | 128 ++++++
>  6 files changed, 999 insertions(+)
>  create mode 100644 drivers/net/dsa/gsw1xx.h
>  create mode 100644 drivers/net/dsa/gsw1xx_core.c
>  create mode 100644 drivers/net/dsa/gsw1xx_mdio.c
> 
> 
> diff --git a/drivers/net/dsa/gsw1xx.h b/drivers/net/dsa/gsw1xx.h
> new file mode 100644
> index 000000000000..08b2975e1267
> --- /dev/null
> +++ b/drivers/net/dsa/gsw1xx.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef GSW1XX_H
> +#define GSW1XX_H
> +
> +#include <linux/regmap.h>
> +#include <linux/device.h>

include headers in sorted manner.

> +#include <net/dsa.h>
> +
> +struct gsw1xx_hw_info {
> +	int max_ports;
> +	int cpu_port;
> +};
> +
> +struct gsw1xx_priv {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct dsa_switch *ds;
> +	const struct gsw1xx_hw_info *hw_info;
> +};
> +
> +extern const struct regmap_access_table gsw1xx_register_set;
> +
> +int gsw1xx_probe(struct gsw1xx_priv *priv, struct device *dev);
> +void gsw1xx_remove(struct gsw1xx_priv *priv);
> +void gsw1xx_shutdown(struct gsw1xx_priv *priv);
> +
> +#endif /* GSW1XX_H */
> diff --git a/drivers/net/dsa/gsw1xx_core.c
> b/drivers/net/dsa/gsw1xx_core.c
> new file mode 100644
> index 000000000000..1b3cbee4addc
> --- /dev/null
> +++ b/drivers/net/dsa/gsw1xx_core.c
> @@ -0,0 +1,823 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/delay.h>
> +#include <linux/etherdevice.h>
> +#include <linux/if_bridge.h>
> +#include <linux/if_vlan.h>
> +#include <linux/module.h>
> +#include <linux/of_mdio.h>
> +#include <linux/of_net.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy.h>
> +#include <linux/phylink.h>
> +#include <linux/regmap.h>
> +#include <net/dsa.h>
> +
> +#include "gsw1xx.h"
> +
> +/* GSW1XX MDIO Registers */
> +#define GSW1XX_MDIO_BASE_ADDR		0xF400
> +#define GSW1XX_MDIO_REG_LEN		0x0422
> +#define GSW1XX_MDIO_GLOB		0x00
> +#define  GSW1XX_MDIO_GLOB_ENABLE	BIT(15)
> +#define GSW1XX_MDIO_CTRL		0x08
> +#define  GSW1XX_MDIO_CTRL_BUSY		BIT(12)
> +#define  GSW1XX_MDIO_CTRL_RD		BIT(11)
> +#define  GSW1XX_MDIO_CTRL_WR		BIT(10)
> +#define  GSW1XX_MDIO_CTRL_PHYAD_MASK	0x1F
> +#define  GSW1XX_MDIO_CTRL_PHYAD_SHIFT	5
> +#define  GSW1XX_MDIO_CTRL_REGAD_MASK	0x1F
> +#define GSW1XX_MDIO_READ		0x09
> +#define GSW1XX_MDIO_WRITE		0x0A
> +#define GSW1XX_MDIO_MDC_CFG0		0x0B
> +#define GSW1XX_MDIO_PHYp(p)		(0x15 - (p))

All Capital letters in Macro will be good.

> +#define  GSW1XX_MDIO_PHY_LINK_MASK	0x6000
> +#define  GSW1XX_MDIO_PHY_LINK_AUTO	0x0000
> +#define  GSW1XX_MDIO_PHY_LINK_DOWN	0x4000
> +#define  GSW1XX_MDIO_PHY_LINK_UP	0x2000
> +#define  GSW1XX_MDIO_PHY_SPEED_MASK	0x1800
> +#define  GSW1XX_MDIO_PHY_SPEED_AUTO	0x1800
> +#define  GSW1XX_MDIO_PHY_SPEED_M10	0x0000
> +#define  GSW1XX_MDIO_PHY_SPEED_M100	0x0800
> +#define  GSW1XX_MDIO_PHY_SPEED_G1	0x1000
> +#define  GSW1XX_MDIO_PHY_FDUP_MASK	0x0600
> +#define  GSW1XX_MDIO_PHY_FDUP_AUTO	0x0000
> +#define  GSW1XX_MDIO_PHY_FDUP_EN	0x0200
> +#define  GSW1XX_MDIO_PHY_FDUP_DIS	0x0600
> +#define  GSW1XX_MDIO_PHY_FCONTX_MASK	0x0180
> +#define  GSW1XX_MDIO_PHY_FCONTX_AUTO	0x0000
> +#define  GSW1XX_MDIO_PHY_FCONTX_EN	0x0100
> +#define  GSW1XX_MDIO_PHY_FCONTX_DIS	0x0180
> +#define  GSW1XX_MDIO_PHY_FCONRX_MASK	0x0060
> +#define  GSW1XX_MDIO_PHY_FCONRX_AUTO	0x0000
> +#define  GSW1XX_MDIO_PHY_FCONRX_EN	0x0020
> +#define  GSW1XX_MDIO_PHY_FCONRX_DIS	0x0060
> +#define  GSW1XX_MDIO_PHY_ADDR_MASK	0x001F
> +#define  GSW1XX_MDIO_PHY_MASK		(GSW1XX_MDIO_PHY_ADDR_M
> ASK | \
> +					 GSW1XX_MDIO_PHY_FCONRX_MASK |
> \
> +					 GSW1XX_MDIO_PHY_FCONTX_MASK |
> \
> +					 GSW1XX_MDIO_PHY_LINK_MASK | \
> +					 GSW1XX_MDIO_PHY_SPEED_MASK | \
> +					 GSW1XX_MDIO_PHY_FDUP_MASK)
> +
> 
> +struct gsw1xx_rmon_cnt_desc {
> +	unsigned int size;
> +	unsigned int offset;
> +	const char *name;
> +};
> +
> +#define MIB_DESC(_size, _offset,
> _name)                                        \
> +	{                                                              
>         \
> +		.size = _size, .offset = _offset, .name =
> _name                \
> +	}
> +
> +static const struct gsw1xx_rmon_cnt_desc gsw1xx_rmon_cnt[] = {
> +	/** Receive Packet Count (only packets that are accepted and
> not discarded). */
> +	MIB_DESC(1, 0x1F, "RxGoodPkts"),
> +	MIB_DESC(1, 0x23, "RxUnicastPkts"),
> +	MIB_DESC(1, 0x22, "RxMulticastPkts"),
> +	MIB_DESC(1, 0x21, "RxFCSErrorPkts"),
> +	MIB_DESC(1, 0x1D, "RxUnderSizeGoodPkts"),
> +	MIB_DESC(1, 0x1E, "RxUnderSizeErrorPkts"),
> +	MIB_DESC(1, 0x1B, "RxOversizeGoodPkts"),
> +	MIB_DESC(1, 0x1C, "RxOversizeErrorPkts"),
> +	MIB_DESC(1, 0x20, "RxGoodPausePkts"),
> +	MIB_DESC(1, 0x1A, "RxAlignErrorPkts"),
> +	MIB_DESC(1, 0x12, "Rx64BytePkts"),
> +	MIB_DESC(1, 0x13, "Rx127BytePkts"),
> +	MIB_DESC(1, 0x14, "Rx255BytePkts"),
> +	MIB_DESC(1, 0x15, "Rx511BytePkts"),
> +	MIB_DESC(1, 0x16, "Rx1023BytePkts"),
> +	/** Receive Size 1024-1522 (or more, if configured) Packet
> Count. */
> +	MIB_DESC(1, 0x17, "RxMaxBytePkts"),
> +	MIB_DESC(1, 0x18, "RxDroppedPkts"),
> +	MIB_DESC(1, 0x19, "RxFilteredPkts"),
> +	MIB_DESC(2, 0x24, "RxGoodBytes"),
> +	MIB_DESC(2, 0x26, "RxBadBytes"),
> +	MIB_DESC(1, 0x11, "TxAcmDroppedPkts"),
> +	MIB_DESC(1, 0x0C, "TxGoodPkts"),
> +	MIB_DESC(1, 0x06, "TxUnicastPkts"),
> +	MIB_DESC(1, 0x07, "TxMulticastPkts"),
> +	MIB_DESC(1, 0x00, "Tx64BytePkts"),
> +	MIB_DESC(1, 0x01, "Tx127BytePkts"),
> +	MIB_DESC(1, 0x02, "Tx255BytePkts"),
> +	MIB_DESC(1, 0x03, "Tx511BytePkts"),
> +	MIB_DESC(1, 0x04, "Tx1023BytePkts"),
> +	/** Transmit Size 1024-1522 (or more, if configured) Packet
> Count. */
> +	MIB_DESC(1, 0x05, "TxMaxBytePkts"),
> +	MIB_DESC(1, 0x08, "TxSingleCollCount"),
> +	MIB_DESC(1, 0x09, "TxMultCollCount"),
> +	MIB_DESC(1, 0x0A, "TxLateCollCount"),
> +	MIB_DESC(1, 0x0B, "TxExcessCollCount"),
> +	MIB_DESC(1, 0x0D, "TxPauseCount"),
> +	MIB_DESC(1, 0x10, "TxDroppedPkts"),
> +	MIB_DESC(2, 0x0E, "TxGoodBytes"),
> +};
> +
> +static u32 gsw1xx_switch_r(struct gsw1xx_priv *priv, u32 offset)
> +{
> +	int ret = 0;

In the below apis you have used *err* variable for storing the return
values. Here *ret* is used. It will be good to have either ret or err
in all the apis.
And Do we need to explicitly assign ret  to zero. since it is assigned
return value of regmap_read in the next statement itself.

> +	u32 val = 0;
> +
> +	ret = regmap_read(priv->regmap, GSW1XX_IP_BASE_ADDR + offset,
> &val);
> +
> +	return ret < 0 ? (u32)ret : val;
> +}
> +
> 
> +
> 
> +static u32 gsw1xx_switch_r_timeout(struct gsw1xx_priv *priv, u32
> offset,
> +				   u32 cleared)
> +{
> +	u32 val;
> +
> +	return read_poll_timeout(gsw1xx_switch_r, val, (val & cleared)
> == 0, 20,
> +				 50000, true, priv, offset);
> +}
> +
> +static int gsw1xx_mdio_poll(struct gsw1xx_priv *priv)
> +{
> +	int cnt = 100;

Is it possible to use u8 instead of int since the value is less than
255.

> +
> +	while (likely(cnt--)) {
> +		u32 ctrl = gsw1xx_mdio_r(priv, GSW1XX_MDIO_CTRL);
> +
> +		if ((ctrl & GSW1XX_MDIO_CTRL_BUSY) == 0)
> +			return 0;
> +		usleep_range(20, 40);
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> 
> 
> +static int gsw1xx_mdio(struct gsw1xx_priv *priv, struct device_node
> *mdio_np)
> +{
> +	struct dsa_switch *ds = priv->ds;
> +	int err;
> +
> +	ds->slave_mii_bus = mdiobus_alloc();
> +	if (!ds->slave_mii_bus)
> +		return -ENOMEM;

Is it possible to use devm_mdiobus_alloc api for memory allocation
which doesn't require mdiobus_free.

> +
> +	ds->slave_mii_bus->priv = priv;
> +	ds->slave_mii_bus->read = gsw1xx_mdio_rd;
> +	ds->slave_mii_bus->write = gsw1xx_mdio_wr;
> +	ds->slave_mii_bus->name = "mxl,gsw1xx-mdio";
> +	snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "%s-mii",
> +		 dev_name(priv->dev));

If you have struct mii_bus *bus and assign ds->slave_mii_bus = bus,
then all the above statement can fit in single line.
snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(priv->dev));

> +	ds->slave_mii_bus->parent = priv->dev;
> +	ds->slave_mii_bus->phy_mask = ~ds->phys_mii_mask;
> +
> +	err = of_mdiobus_register(ds->slave_mii_bus, mdio_np);

Simillarly here devm_of_mdiobus_register

> +	if (err)
> +		mdiobus_free(ds->slave_mii_bus);
> +
> +	return err;
> +}
> +
> +static int gsw1xx_setup(struct dsa_switch *ds)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +	unsigned int cpu_port = priv->hw_info->cpu_port;

Is it possible to use u8 instead of unsinged int.

> +	int i;
> +	int err;
> +
> +	gsw1xx_switch_w(priv, GSW1XX_IP_SWRES_R0, GSW1XX_IP_SWRES);
> +	usleep_range(5000, 10000);
> +	gsw1xx_switch_w(priv, 0, GSW1XX_IP_SWRES);
> +
> +	/* disable port fetch/store dma on all ports */
> +	for (i = 0; i < priv->hw_info->max_ports; i++)
> +		gsw1xx_port_disable(ds, i);
> +
> +	/* enable Switch */
> +	gsw1xx_mdio_mask(priv, 0, GSW1XX_MDIO_GLOB_ENABLE,
> GSW1XX_MDIO_GLOB);
> +
> +	gsw1xx_switch_w(priv, 0x7F, GSW1XX_IP_PCE_PMAP2);
> +	gsw1xx_switch_w(priv, 0x7F, GSW1XX_IP_PCE_PMAP3);
> +
> +	/* Deactivate MDIO PHY auto polling since it affects mmd
> read/write.
> +	 */
> +	gsw1xx_mdio_w(priv, 0x0, GSW1XX_MDIO_MDC_CFG0);
> +
> +	gsw1xx_switch_mask(priv, 1, GSW1XX_IP_MAC_CTRL_2_MLEN,
> +			   GSW1XX_IP_MAC_CTRL_2p(cpu_port));
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_BM_QUEUE_GCTRL_GL_MOD,
> +			   GSW1XX_IP_BM_QUEUE_GCTRL);
> +
> +	/* Flush MAC Table */
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_PCE_GCTRL_0_MTFL,
> +			   GSW1XX_IP_PCE_GCTRL_0);
> +	err = gsw1xx_switch_r_timeout(priv, GSW1XX_IP_PCE_GCTRL_0,
> +				      GSW1XX_IP_PCE_GCTRL_0_MTFL);
> +	if (err) {
> +		dev_err(priv->dev, "MAC flushing didn't finish\n");
> +		return err;
> +	}
> +
> +	gsw1xx_port_enable(ds, cpu_port, NULL);
> +
> +	return 0;
> +}
> +
> +static void gsw1xx_port_set_link(struct gsw1xx_priv *priv, int port,
> bool link)
> +{
> +	u32 mdio_phy;
> +
> +	if (link)
> +		mdio_phy = GSW1XX_MDIO_PHY_LINK_UP;
> +	else
> +		mdio_phy = GSW1XX_MDIO_PHY_LINK_DOWN;
> +
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_LINK_MASK, mdio_phy,
> +			 GSW1XX_MDIO_PHYp(port));
> +}
> +
> +static void gsw1xx_port_set_speed(struct gsw1xx_priv *priv, int
> port, int speed,
> +				  phy_interface_t interface)
> +{
> +	u32 mdio_phy = 0, mac_ctrl_0 = 0;

Initialize variable in different line to uniformity in the file.

> +
> +	switch (speed) {
> +	case SPEED_10:
> +		mdio_phy = GSW1XX_MDIO_PHY_SPEED_M10;
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_GMII_MII;
> +		break;
> +
> +	case SPEED_100:
> +		mdio_phy = GSW1XX_MDIO_PHY_SPEED_M100;
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_GMII_MII;
> +		break;
> +
> +	case SPEED_1000:
> +		mdio_phy = GSW1XX_MDIO_PHY_SPEED_G1;
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_GMII_RGMII;
> +		break;
> +	}
> +
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_SPEED_MASK, mdio_phy,
> +			 GSW1XX_MDIO_PHYp(port));
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_MAC_CTRL_0_GMII_MASK,
> mac_ctrl_0,
> +			   GSW1XX_IP_MAC_CTRL_0p(port));
> +}
> +
> +static void gsw1xx_port_set_duplex(struct gsw1xx_priv *priv, int
> port,
> +				   int duplex)
> +{
> +	u32 mac_ctrl_0, mdio_phy;

Simillarly here. In the above function, you have initialized the values
to zero. But not here.

> +
> +	if (duplex == DUPLEX_FULL) {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FDUP_EN;
> +		mdio_phy = GSW1XX_MDIO_PHY_FDUP_EN;
> +	} else {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FDUP_DIS;
> +		mdio_phy = GSW1XX_MDIO_PHY_FDUP_DIS;
> +	}
> +
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_MAC_CTRL_0_FDUP_MASK,
> mac_ctrl_0,
> +			   GSW1XX_IP_MAC_CTRL_0p(port));
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_FDUP_MASK, mdio_phy,
> +			 GSW1XX_MDIO_PHYp(port));
> +}
> +
> +static void gsw1xx_port_set_pause(struct gsw1xx_priv *priv, int
> port,
> +				  bool tx_pause, bool rx_pause)
> +{
> +	u32 mac_ctrl_0, mdio_phy;

Simillarly here.

> +
> +	if (tx_pause && rx_pause) {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_RXTX;
> +		mdio_phy =
> +			GSW1XX_MDIO_PHY_FCONTX_EN |
> GSW1XX_MDIO_PHY_FCONRX_EN;
> +	} else if (tx_pause) {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_TX;
> +		mdio_phy =
> +			GSW1XX_MDIO_PHY_FCONTX_EN |
> GSW1XX_MDIO_PHY_FCONRX_DIS;
> +	} else if (rx_pause) {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_RX;
> +		mdio_phy =
> +			GSW1XX_MDIO_PHY_FCONTX_DIS |
> GSW1XX_MDIO_PHY_FCONRX_EN;
> +	} else {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_NONE;
> +		mdio_phy =
> +			GSW1XX_MDIO_PHY_FCONTX_DIS |
> GSW1XX_MDIO_PHY_FCONRX_DIS;
> +	}
> +
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_MAC_CTRL_0_FCON_MASK,
> mac_ctrl_0,
> +			   GSW1XX_IP_MAC_CTRL_0p(port));
> +	gsw1xx_mdio_mask(priv,
> +			 GSW1XX_MDIO_PHY_FCONTX_MASK |
> GSW1XX_MDIO_PHY_FCONRX_MASK,
> +			 mdio_phy, GSW1XX_MDIO_PHYp(port));
> +}
> +
> 
> 
> 
> +
> +static void gsw1xx_get_ethtool_stats(struct dsa_switch *ds, int
> port,
> +				     uint64_t *data)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +	const struct gsw1xx_rmon_cnt_desc *rmon_cnt;
> +	int i;
> +	u64 high;

Reverse christmas tree

> +
> +	for (i = 0; i < ARRAY_SIZE(gsw1xx_rmon_cnt); i++) {
> +		rmon_cnt = &gsw1xx_rmon_cnt[i];
> +
> +		data[i] =
> +			gsw1xx_bcm_ram_entry_read(priv, port, rmon_cnt-
> >offset);
> +		if (rmon_cnt->size == 2) {
> +			high = gsw1xx_bcm_ram_entry_read(priv, port,
> +							 rmon_cnt-
> >offset + 1);
> +			data[i] |= high << 32;
> +		}
> +	}
> +}
> +
> 
> +static int gsw1xx_get_mac_eee(struct dsa_switch *ds, int port,
> +			      struct ethtool_eee *e)
> +{
> +	struct gsw1xx_priv *priv = (struct gsw1xx_priv *)ds->priv;
> +	u32 val = 0;

Val initialized to zero.

> +
> +	val = gsw1xx_switch_r(priv, GSW1XX_IP_MAC_CTRL_4p(port));
> +	e->tx_lpi_enabled = !!(val & GSW1XX_IP_MAC_CTRL_4_LPIEN);
> +
> +	e->tx_lpi_timer = 20;
> +
> +	return 0;
> +}
> +
> 
> 
> +int gsw1xx_probe(struct gsw1xx_priv *priv, struct device *dev)
> +{
> +	struct device_node *np, *mdio_np;
> +	int err;
> +	u32 version;
> +
> +	if (!priv->regmap || IS_ERR(priv->regmap))
> +		return -EINVAL;
> +
> +	priv->hw_info = of_device_get_match_data(dev);
> +	if (!priv->hw_info)
> +		return -EINVAL;
> +
> +	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
> +	if (!priv->ds)
> +		return -ENOMEM;
> +
> +	priv->ds->dev = dev;
> +	priv->ds->num_ports = priv->hw_info->max_ports;
> +	priv->ds->priv = priv;
> +	priv->ds->ops = &gsw1xx_switch_ops;
> +	priv->dev = dev;
> +	version = gsw1xx_switch_r(priv, GSW1XX_IP_VERSION);
> +
> +	np = dev->of_node;
> +	switch (version) {
> +	case GSW1XX_IP_VERSION_2_3:
> +		if (!of_device_is_compatible(np, "mxl,gsw145-mdio"))
> +			return -EINVAL;
> +		break;
> +	default:
> +		dev_err(dev, "unknown GSW1XX_IP version: 0x%x",
> version);
> +		return -ENOENT;
> +	}
> +
> +	/* bring up the mdio bus */
> +	mdio_np = of_get_child_by_name(np, "mdio");
> +	if (!mdio_np) {
> +		dev_err(dev, "missing child mdio node\n");
> +		return -EINVAL;
> +	}
> +
> +	err = gsw1xx_mdio(priv, mdio_np);
> +	if (err) {
> +		dev_err(dev, "mdio probe failed\n");
> +		goto put_mdio_node;
> +	}
> +
> +	err = dsa_register_switch(priv->ds);
> +	if (err) {
> +		dev_err(dev, "dsa switch register failed: %i\n", err);
> +		goto mdio_bus;

To have readability, can be renamed to mdio_bus_unregister.

> +	}
> +	if (!dsa_is_cpu_port(priv->ds, priv->hw_info->cpu_port)) {
> +		dev_err(dev,
> +			"wrong CPU port defined, HW only supports port:
> %i",
> +			priv->hw_info->cpu_port);
> +		err = -EINVAL;
> +		goto disable_switch;
> +	}
> +
> +	dev_set_drvdata(dev, priv);
> +
> +	return 0;
> +
> +disable_switch:
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_GLOB_ENABLE, 0,
> GSW1XX_MDIO_GLOB);
> +	dsa_unregister_switch(priv->ds);
> +mdio_bus:
> +	if (mdio_np) {
> +		mdiobus_unregister(priv->ds->slave_mii_bus);
> +		mdiobus_free(priv->ds->slave_mii_bus);
> +	}
> +put_mdio_node:
> +	of_node_put(mdio_np);
> +	return err;
> +}
> +EXPORT_SYMBOL(gsw1xx_probe);
> +
> 
> 
> 
> diff --git a/drivers/net/dsa/gsw1xx_mdio.c
> b/drivers/net/dsa/gsw1xx_mdio.c
> new file mode 100644
> index 000000000000..8328001041ed
> --- /dev/null
> +++ b/drivers/net/dsa/gsw1xx_mdio.c
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MaxLinear switch driver for GSW1XX in MDIO managed mode
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
> +#include <linux/of.h>
                                                          
Header file in sorted order.

> +
> +#include "gsw1xx.h"
> +
> +#define GSW1XX_SMDIO_TARGET_BASE_ADDR_REG	0x1F
> +
>