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

quoted

quoted

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

I don't think we have a good name for a conventional fast Ethernet.
But since we call the other T1, since it has a single pair, maybe use
T2, since Fast Ethernet uses 2 pair. I would also suggest a comment
near this code explaining what T1 and T2 mean.

quoted

What PHY driver is actually used? The "Microchip LAN87xx T1" driver?

Phy is basically a LAN87xx PHY. But the driver is customized for
LAN937x.

Does it have its own ID in registers 2 and 3? Can you tell this
customised version from the regular?

quoted

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.

Right, so you need to write such a driver, and put it into
drivers/net/phy.

quoted

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.

So membership is about forwarding packets between ports. Most other
chips handles this itself. But for this device, you need to handle
this in software. O.K.

You only want to forward when in STP state BR_STATE_FORWARDING. But
the code seems more complex than this.

    Andrew