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

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.

quoted

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

quoted

+     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

quoted

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

quoted

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

quoted

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

quoted

+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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+     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