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
+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?
+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?
+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;...
+}
+
+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.
+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.
+
+ /* 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.
+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?
+ /* 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