Re: [PATCH net-next 3/8] net: dsa: microchip: add DSA support for microchip lan937x

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

On Fri, 2021-01-29 at 02:07 +0100, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe

Thanks for your time on reviewing the patches.

> 
> > +bool lan937x_is_internal_phy_port(struct ksz_device *dev, int
> > port)
> > +{
> > +     /* Check if the port is RGMII */
> > +     if (port == LAN937X_RGMII_1_PORT || port ==
> > LAN937X_RGMII_2_PORT)
> > +             return false;
> > +
> > +     /* Check if the port is SGMII */
> > +     if (port == LAN937X_SGMII_PORT &&
> > +         GET_CHIP_ID_LSB(dev->chip_id) == CHIP_ID_73)
> > +             return false;
> > +
> > +     return true;
> > +}
> > +
> > +static u32 lan937x_get_port_addr(int port, int offset)
> > +{
> > +     return PORT_CTRL_ADDR(port, offset);
> > +}
> > +
> > +bool lan937x_is_internal_tx_phy_port(struct ksz_device *dev, int
> > port)
> > +{
> > +     /* Check if the port is internal tx phy port */
> 
> What is an internal TX phy port? Is it actually a conventional t2
> Fast
> Ethernet port, as opposed to a t1 port?
This is 100 Base-Tx phy which is compliant with
802.3/802.3u standards. Two of the SKUs have both T1 and TX integrated
Phys as mentioned in the patch intro mail.

> 
> > +     if (lan937x_is_internal_phy_port(dev, port) && port ==
> > LAN937X_TXPHY_PORT)
> > +             if ((GET_CHIP_ID_LSB(dev->chip_id) == CHIP_ID_71) ||
> > +                 (GET_CHIP_ID_LSB(dev->chip_id) == CHIP_ID_72))
> > +                     return true;
> > +
> > +     return false;
> > +}
> > +
> > +bool lan937x_is_internal_t1_phy_port(struct ksz_device *dev, int
> > port)
> > +{
> > +     /* Check if the port is internal t1 phy port */
> > +     if (lan937x_is_internal_phy_port(dev, port) &&
> > +         !lan937x_is_internal_tx_phy_port(dev, port))
> > +             return true;
> > +
> > +     return false;
> > +}
> > +
> > +int lan937x_t1_tx_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 0;
> 
> All this t1 and tx is confusing. I think lan937x_internal_phy_write()
> would be better.
Sure, will rename it to lan937x_internal_phy_write()

> 
> I also wonder if -EOPNOTSUPP would be better, or -EINVAL?
-EOPNOTSUPP would be better. Currently return value has not checked for
phy_write() calls, i will add that as well in the list

> 
> > +
> > +     if (lan937x_is_internal_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)));
> > +
> > +     ksz_write16(dev, REG_VPHY_IND_ADDR__2, temp);
> > +
> > +     /* Write the data to be written to the VPHY reg */
> > +     ksz_write16(dev, REG_VPHY_IND_DATA__2, val);
> > +
> > +     /* Write the Write En and Busy bit */
> > +     ksz_write16(dev, REG_VPHY_IND_CTRL__2, (VPHY_IND_WRITE
> > +                             | VPHY_IND_BUSY));
> > +
> > +     ret = regmap_read_poll_timeout(dev->regmap[1],
> > +                                    REG_VPHY_IND_CTRL__2,
> > +                             value, !(value & VPHY_IND_BUSY), 10,
> > 1000);
> > +
> > +     /* failed to write phy register. get out of loop */
> > +     if (ret) {
> > +             dev_err(dev->dev, "Failed to write phy register\n");
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +int lan937x_t1_tx_phy_read(struct ksz_device *dev, int addr,
> > +                        int reg, u16 *val)
> > +{
> > +     u16 temp, addr_base;
> > +     unsigned int value;
> > +     int ret;
> > +
> > +     if (lan937x_is_internal_phy_port(dev, addr)) {
> > +             if (lan937x_is_internal_tx_phy_port(dev, addr))
> > +                     addr_base = REG_PORT_TX_PHY_CTRL_BASE;
> > +             else
> > +                     addr_base = REG_PORT_T1_PHY_CTRL_BASE;
> 
> You could reduce the indentation by doing what you did above:
Sure, will change it.

> 
> > +     /* Check for internal phy port */
> > +     if (!lan937x_is_internal_phy_port(dev, addr))
> > +             return 0;
> 
> You might want to return 0xffff here, which is what a read on a
> non-existent device on an MDIO bus should return.
Sure, will change it

> 
> 
> > +
> > +             /* get register address based on the logical port */
> > +             temp = PORT_CTRL_ADDR(addr, (addr_base + (reg <<
> > 2)));
> > +
> > +             ksz_write16(dev, REG_VPHY_IND_ADDR__2, temp);
> > +             /* Write Read and Busy bit to start the transaction*/
> > +             ksz_write16(dev, REG_VPHY_IND_CTRL__2,
> > VPHY_IND_BUSY);
> > +
> > +             ret = regmap_read_poll_timeout(dev->regmap[1],
> > +                                            REG_VPHY_IND_CTRL__2,
> > +                                     value, !(value &
> > VPHY_IND_BUSY), 10, 1000);
> > +
> > +             /*  failed to read phy register. get out of loop */
> > +             if (ret) {
> > +                     dev_err(dev->dev, "Failed to read phy
> > register\n");
> > +                     return ret;
> > +             }
> > +             /* Read the VPHY register which has the PHY data*/
> > +             ksz_read16(dev, REG_VPHY_IND_DATA__2, val);
> > +     }
> > +
> > +     return 0;
> > +}
> > +static void tx_phy_setup(struct ksz_device *dev, int port)
> > +{
> > +     u16 data_lo;
> > +
> > +     lan937x_t1_tx_phy_read(dev, port, REG_PORT_TX_SPECIAL_MODES,
> > &data_lo);
> > +     /* Need to change configuration from 6 to other value. */
> > +     data_lo &= TX_PHYADDR_M;
> > +
> > +     lan937x_t1_tx_phy_write(dev, port, REG_PORT_TX_SPECIAL_MODES,
> > data_lo);
> > +
> > +     /* Need to toggle test_mode bit to enable DSP access. */
> > +     lan937x_t1_tx_phy_write(dev, port, REG_PORT_TX_IND_CTRL,
> > TX_TEST_MODE);
> > +     lan937x_t1_tx_phy_write(dev, port, REG_PORT_TX_IND_CTRL, 0);
> > +
> > +     /* Note TX_TEST_MODE is then always enabled so this is not
> > required. */
> > +     lan937x_t1_tx_phy_write(dev, port, REG_PORT_TX_IND_CTRL,
> > TX_TEST_MODE);
> > +     lan937x_t1_tx_phy_write(dev, port, REG_PORT_TX_IND_CTRL, 0);
> 
> This is only accessing PHY registers, not switch registers. So this
> code belongs in the PHY driver, not the switch driver.
> 
> What PHY driver is actually used? The "Microchip LAN87xx T1" driver?
Phy is basically a LAN87xx PHY. But the driver is customized for
LAN937x.

> 
> > +static void tx_phy_port_init(struct ksz_device *dev, int port)
> > +{
> > +     u32 data;
> > +
> > +     /* Software reset. */
> > +     lan937x_t1_tx_phy_mod_bits(dev, port, MII_BMCR, BMCR_RESET,
> > true);
> > +
> > +     /* tx phy setup */
> > +     tx_phy_setup(dev, port);
> 
> And which PHY driver is used here? "Microchip LAN88xx"? All this code
> should be in the PHY driver.
As of now, no driver is available in the kernel since its part of
LAN937x.

> s
> > +void lan937x_port_setup(struct ksz_device *dev, int port, bool
> > cpu_port)
> > +{
> > +     struct ksz_port *p = &dev->ports[port];
> > +     u8 data8, member;
> > +     } else {
> > +             /* force flow control off*/
> > +             lan937x_port_cfg(dev, port, REG_PORT_XMII_CTRL_0,
> > +                              PORT_FORCE_TX_FLOW_CTRL |
> > PORT_FORCE_RX_FLOW_CTRL,
> > +                          false);
> > +
> > +             lan937x_pread8(dev, port, REG_PORT_XMII_CTRL_1,
> > &data8);
> > +
> > +             /* clear MII selection & set it based on interface
> > later */
> > +             data8 &= ~PORT_MII_SEL_M;
> > +
> > +             /* configure MAC based on p->interface */
> > +             switch (p->interface) {
> > +             case PHY_INTERFACE_MODE_MII:
> > +                     lan937x_set_gbit(dev, false, &data8);
> > +                     data8 |= PORT_MII_SEL;
> > +                     break;
> > +             case PHY_INTERFACE_MODE_RMII:
> > +                     lan937x_set_gbit(dev, false, &data8);
> > +                     data8 |= PORT_RMII_SEL;
> > +                     break;
> > +             default:
> > +                     lan937x_set_gbit(dev, true, &data8);
> > +                     data8 |= PORT_RGMII_SEL;
> > +
> > +                     data8 &= ~PORT_RGMII_ID_IG_ENABLE;
> > +                     data8 &= ~PORT_RGMII_ID_EG_ENABLE;
> > +
> > +                     if (p->interface ==
> > PHY_INTERFACE_MODE_RGMII_ID ||
> > +                         p->interface ==
> > PHY_INTERFACE_MODE_RGMII_RXID)
> > +                             data8 |= PORT_RGMII_ID_IG_ENABLE;
> > +
> > +                     if (p->interface ==
> > PHY_INTERFACE_MODE_RGMII_ID ||
> > +                         p->interface ==
> > PHY_INTERFACE_MODE_RGMII_TXID)
> > +                             data8 |= PORT_RGMII_ID_EG_ENABLE;
> 
> Normally, the PHY inserts the delay, not the MAC. If the MAC is doing
> the delay, you need to ensure the PHY knows this, when you call
> phy_connect() you need to pass PHY_INTERFACE_MODE_RGMII, so it does
> not add delays.
Okay, understand.

> 
> > +                     break;
> > +             }
> > +             lan937x_pwrite8(dev, port, REG_PORT_XMII_CTRL_1,
> > data8);
> > +     }
> > +
> > +     if (cpu_port)
> > +             member = dev->port_mask;
> > +     else
> > +             member = dev->host_mask | p->vid_member;
> > +
> > +     lan937x_cfg_port_member(dev, port, member);
> > +}
> > +
> > +static int lan937x_switch_init(struct ksz_device *dev)
> > +{
> > +     int i;
> > +
> > +     dev->ds->ops = &lan937x_switch_ops;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(lan937x_switch_chips); i++) {
> > +             const struct lan937x_chip_data *chip =
> > &lan937x_switch_chips[i];
> > +
> > +             if (dev->chip_id == chip->chip_id) {
> > +                     dev->name = chip->dev_name;
> > +                     dev->num_vlans = chip->num_vlans;
> > +                     dev->num_alus = chip->num_alus;
> > +                     dev->num_statics = chip->num_statics;
> > +                     dev->port_cnt = chip->port_cnt;
> > +                     dev->cpu_ports = chip->cpu_ports;
> > +                     break;
> > +             }
> > +     }
> 
> Please verify that the switch found actually matches the DT
> compatible
> string. With 4 compatible strings, if you don't verify it, you will
> find that 3/4 of the boards have it wrong, but it still works. You
> then get into trouble when you actually need to use the compatible
> string for something.
> 
> Or just use a single compatible string.
Okay, will add dt-compatible checks.

> 
> > +static int lan937x_get_link_status(struct ksz_device *dev, int
> > port)
> > +{
> > +     u16 val1, val2;
> > +
> > +     lan937x_t1_tx_phy_read(dev, port, REG_PORT_T1_PHY_M_STATUS,
> > +                            &val1);
> > +
> > +     lan937x_t1_tx_phy_read(dev, port, REG_PORT_T1_MODE_STAT,
> > &val2);
> > +
> > +     if (val1 & (PORT_T1_LOCAL_RX_OK | PORT_T1_REMOTE_RX_OK) &&
> > +         val2 & (T1_PORT_DSCR_LOCK_STATUS_MSK |
> > T1_PORT_LINK_UP_MSK))
> > +             return PHY_LINK_UP;
> > +
> > +     return PHY_LINK_DOWN;
> > +}
> 
> The PHY driver should tell you if the link is up. You should not
> being
> accessing the PHY directly.
> 
> It actually looks like you have your PHY drivers here, embedded
> inside
> this driver. That is wrong, they should move into drivers/net/phy.
> The
> switch driver should just give access to the registers, so that the
> PHY driver, and phylib and drive the PHY.
Sure, i think T1 & Tx phy drivers to be submitted.

> 
> > +static void lan937x_port_stp_state_set(struct dsa_switch *ds, int
> > port,
> > +                                    u8 state)
> > +{
> > +     struct ksz_device *dev = ds->priv;
> > +     struct ksz_port *p = &dev->ports[port];
> > +     int forward = dev->member;
> > +     int member = -1;
> > +     u8 data;
> > +
> > +     lan937x_pread8(dev, port, P_STP_CTRL, &data);
> > +     data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE |
> > PORT_LEARN_DISABLE);
> > +
> > +     switch (state) {
> > +     case BR_STATE_DISABLED:
> > +             data |= PORT_LEARN_DISABLE;
> > +             if (port != dev->cpu_port)
> > +                     member = 0;
> 
> You can remove all the tests for cpu_port. It should never
> happen.  If
> it does, something is broken in the DSA core.
Okay, will remove the checks of cpu_port.

> 
> > +             break;
> > +     case BR_STATE_LISTENING:
> > +             data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE);
> > +             if (port != dev->cpu_port &&
> > +                 p->stp_state == BR_STATE_DISABLED)
> > +                     member = dev->host_mask | p->vid_member;
> > +             break;
> > +     case BR_STATE_LEARNING:
> > +             data |= PORT_RX_ENABLE;
> > +             break;
> > +     case BR_STATE_FORWARDING:
> > +             data |= (PORT_TX_ENABLE | PORT_RX_ENABLE);
> > +
> > +             /* This function is also used internally. */
> > +             if (port == dev->cpu_port)
> > +                     break;
> 
> You probably want to refactor this. Move the code which is needed for
> the CPU port into a helper, and call it directly.
Sure, will create one helper function.

> 
> > +
> > +             member = dev->host_mask | p->vid_member;
> > +             mutex_lock(&dev->dev_mutex);
> > +
> > +             /* Port is a member of a bridge. */
> > +             if (dev->br_member & (1 << port)) {
> > +                     dev->member |= (1 << port);
> > +                     member = dev->member;
> > +             }
> > +             mutex_unlock(&dev->dev_mutex);
> > +             break;
> > +     case BR_STATE_BLOCKING:
> > +             data |= PORT_LEARN_DISABLE;
> > +             if (port != dev->cpu_port &&
> > +                 p->stp_state == BR_STATE_DISABLED)
> > +                     member = dev->host_mask | p->vid_member;
> > +             break;
> > +     default:
> > +             dev_err(ds->dev, "invalid STP state: %d\n", state);
> > +             return;
> > +     }
> > +
> > +     lan937x_pwrite8(dev, port, P_STP_CTRL, data);
> > +
> > +     p->stp_state = state;
> > +     mutex_lock(&dev->dev_mutex);
> > +
> > +     /* Port membership may share register with STP state. */
> > +     if (member >= 0 && member != p->member)
> > +             lan937x_cfg_port_member(dev, port, (u8)member);
> > +
> > +     /* Check if forwarding needs to be updated. */
> > +     if (state != BR_STATE_FORWARDING) {
> > +             if (dev->br_member & (1 << port))
> > +                     dev->member &= ~(1 << port);
> > +     }
> > +
> > +     /* When topology has changed the function
> > ksz_update_port_member
> > +      * should be called to modify port forwarding behavior.
> > +      */
> > +     if (forward != dev->member)
> > +             ksz_update_port_member(dev, port);
> 
> Please could you explain more what is going on with membership?
> Generally, STP state is specific to the port, and nothing else
> changes. So it is not clear what this membership is all about.
It updates the membership for the forwarding behavior based on
forwarding state of the port.

> 
> 
> > +     mutex_unlock(&dev->dev_mutex);
> > +}
> > +
> > +static void lan937x_config_cpu_port(struct dsa_switch *ds)
> > +{
> > +     struct ksz_device *dev = ds->priv;
> > +     struct ksz_port *p;
> > +     int i;
> > +
> > +     ds->num_ports = dev->port_cnt;
> > +
> > +     for (i = 0; i < dev->port_cnt; i++) {
> > +             if (dsa_is_cpu_port(ds, i) && (dev->cpu_ports & (1 <<
> > i))) {
> > +                     phy_interface_t interface;
> > +                     const char *prev_msg;
> > +                     const char *prev_mode;
> > +
> > +                     dev->cpu_port = i;
> > +                     dev->host_mask = (1 << dev->cpu_port);
> > +                     dev->port_mask |= dev->host_mask;
> > +                     p = &dev->ports[i];
> > +
> > +                     /* Read from XMII register to determine host
> > port
> > +                      * interface.  If set specifically in device
> > tree
> > +                      * note the difference to help debugging.
> > +                      */
> > +                     interface = lan937x_get_interface(dev, i);
> > +                     if (!p->interface) {
> > +                             if (dev->compat_interface) {
> > +                                     dev_warn(dev->dev,
> > +                                              "Using legacy switch
> > \"phy-mode\" property, because it is missing on port %d node.
> > Please update your device tree.\n",
> > +                                              i);
> 
> Since this is a new driver, there cannot be any legacy DT blobs which
> needs workarounds.
Understand, will remove the warning message.

> 
>       Andrew