Re: [PATCH v9 net-next 06/11] net: dsa: microchip: add DSA support for microchip lan937x

[Prasanna Vengateshan <prasanna.vengateshan@microchip.com>]

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

> 
> > +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().


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

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

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

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