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@xxxxxxxx> > --- > 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 > + >