Re: [PATCH v9 net-next 06/11] net: dsa: microchip: add DSA support for microchip lan937x
From: Andrew Lunn <andrew@lunn.ch>
Date: 2022-03-20 01:17:50
Also in:
linux-devicetree, lkml
quoted hunk ↗ jump to hunk
+int lan937x_reset_switch(struct ksz_device *dev) +{ + u32 data32; + u8 data8; + int ret; + + /* reset switch */ + ret = lan937x_cfg(dev, REG_SW_OPERATION, SW_RESET, true); + if (ret < 0) + return ret; + + ret = ksz_read8(dev, REG_SW_LUE_CTRL_1, &data8); + if (ret < 0) + return ret; + + /* Enable Auto Aging */ + ret = ksz_write8(dev, REG_SW_LUE_CTRL_1, data8 | SW_LINK_AUTO_AGING); + if (ret < 0) + return ret; + + /* disable interrupts */ + ret = ksz_write32(dev, REG_SW_INT_MASK__4, SWITCH_INT_MASK); + if (ret < 0) + return ret; + + ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, 0xFF); + if (ret < 0) + return ret; + + return ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data32);
I would probably enable auto aging in the setup routing, not the reset. Disabling interrupts is less clear, maybe it also belongs in setup?
quoted hunk ↗ jump to hunk
+static void lan937x_switch_exit(struct ksz_device *dev) +{ + lan937x_reset_switch(dev); +}
Humm. Does a reset leave the switch in a dumb mode, or is it totally disabled?
quoted hunk ↗ jump to hunk
+int lan937x_internal_phy_write(struct ksz_device *dev, int addr, int reg, + u16 val) +{ + u16 temp, addr_base; + unsigned int value; + int ret; + + /* Check for internal phy port */ + if (!lan937x_is_internal_phy_port(dev, addr)) + return -EOPNOTSUPP; + + if (lan937x_is_internal_base_tx_phy_port(dev, addr)) + addr_base = REG_PORT_TX_PHY_CTRL_BASE; + else + addr_base = REG_PORT_T1_PHY_CTRL_BASE; + + temp = PORT_CTRL_ADDR(addr, (addr_base + (reg << 2))); + + ret = ksz_write16(dev, REG_VPHY_IND_ADDR__2, temp); + if (ret < 0) + return ret;
...
quoted hunk ↗ jump to hunk
+} + +int lan937x_internal_phy_read(struct ksz_device *dev, int addr, int reg, + u16 *val) +{ + u16 temp, addr_base; + unsigned int value; + int ret; + + /* Check for internal phy port, return 0xffff for non-existent phy */ + if (!lan937x_is_internal_phy_port(dev, addr)) + return 0xffff; + + if (lan937x_is_internal_base_tx_phy_port(dev, addr)) + addr_base = REG_PORT_TX_PHY_CTRL_BASE; + else + addr_base = REG_PORT_T1_PHY_CTRL_BASE; + + /* get register address based on the logical port */ + temp = PORT_CTRL_ADDR(addr, (addr_base + (reg << 2))); + + ret = ksz_write16(dev, REG_VPHY_IND_ADDR__2, temp); + if (ret < 0) + return ret;
Looks pretty similar to me. You should pull this out into a helper.
quoted hunk ↗ jump to hunk
+void lan937x_port_setup(struct ksz_device *dev, int port, bool cpu_port) +{ + struct dsa_switch *ds = dev->ds; + u8 member; + + /* enable tag tail for host port */ + if (cpu_port) { + lan937x_port_cfg(dev, port, REG_PORT_CTRL_0, + PORT_TAIL_TAG_ENABLE, true); + }
Checkpatch probably warns here that the {} are not needed.
quoted hunk ↗ jump to hunk
+ + /* disable frame check length field */ + lan937x_port_cfg(dev, port, REG_PORT_MAC_CTRL_0, PORT_FR_CHK_LENGTH, + false); + + /* set back pressure for half duplex */ + lan937x_port_cfg(dev, port, REG_PORT_MAC_CTRL_1, PORT_BACK_PRESSURE, + true); + + /* enable 802.1p priority */ + lan937x_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_PRIO_ENABLE, true); + + if (!lan937x_is_internal_phy_port(dev, port)) { + /* enable flow control */ + lan937x_port_cfg(dev, port, REG_PORT_XMII_CTRL_0, + PORT_TX_FLOW_CTRL | PORT_RX_FLOW_CTRL, + true); + }
Here as well.
quoted hunk ↗ jump to hunk
+struct lan_alu_struct { + /* entry 1 */ + u8 is_static:1; + u8 is_src_filter:1; + u8 is_dst_filter:1; + u8 prio_age:3; + u32 _reserv_0_1:23; + u8 mstp:3;
I assume this works O.K, but bitfields are nornaly defined using one type. I would of used u32 for them all. Is there some advantage of missing u8 and u32?
quoted hunk ↗ jump to hunk
+ /* entry 2 */ + u8 is_override:1; + u8 is_use_fid:1; + u32 _reserv_1_1:22; + u8 port_forward:8; + /* entry 3 & 4*/ + u32 _reserv_2_1:9; + u8 fid:7; + u8 mac[ETH_ALEN]; +};
Andrew