On Sun, 2022-03-20 at 02:17 +0100, Andrew Lunn wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you know the
content is safe

quoted

+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?

Actually lan937x_reset_switch() gets called from setup() as well.

quoted

+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?

Its a kind of unconfiguring and you are right, Auto aging & disabling interrupts
can be directly moved to setup().

quoted

+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

+}
+
+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.

Sure, i think it can be made except if check lan937x_is_internal_phy_port() 

quoted

+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?

It works because direct assignments are used. But using one type is the right
way. I will change it in the next patch.

quoted

+     /* 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