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
