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

[Andrew Lunn <andrew@lunn.ch>]

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

> > 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?

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

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