On 03/23/2018 01:11 PM, Alexandre Belloni wrote:
> Add a driver for Microsemi Ocelot Ethernet switch support.
> 
> This makes two modules:
> mscc_ocelot_common handles all the common features that doesn't depend on
> how the switch is integrated in the SoC. Currently, it handles offloading
> bridging to the hardware. ocelot_io.c handles register accesses. This is
> unfortunately needed because the register layout is packed and then depends
> on the number of ports available on the switch. The register definition
> files are automatically generated.
> 
> ocelot_board handles the switch integration on the SoC and on the board.
> 
> Frame injection and extraction to/from the CPU port is currently done using
> register accesses which is quite slow. DMA is possible but the port is not
> able to absorb the whole switch bandwidth.
> 
> Signed-off-by: Alexandre Belloni <alexandre.bell...@bootlin.com>

Random drive by comments because this is quite a number of lines to review!

Overall, looks quite good for a first version. Out of curiosity, is
there a particular switch test you ran this driver against? LNST?

> +static int ocelot_mact_learn(struct ocelot *ocelot, int port,
> +                          const unsigned char mac[ETH_ALEN],
> +                          unsigned int vid,
> +                          enum macaccess_entry_type type)
> +{
> +     u32 macl = 0, mach = 0;
> +
> +     /* Set the MAC address to learn and the vlan associated in a format
> +      * understood by the hardware.
> +      */
> +     mach |= vid    << 16;
> +     mach |= mac[0] << 8;
> +     mach |= mac[1] << 0;
> +     macl |= mac[2] << 24;
> +     macl |= mac[3] << 16;
> +     macl |= mac[4] << 8;
> +     macl |= mac[5] << 0;
> +
> +     ocelot_write(ocelot, macl, ANA_TABLES_MACLDATA);
> +     ocelot_write(ocelot, mach, ANA_TABLES_MACHDATA);

You are repeating this in the function right below, can you factor it
somehow into a common function that this one, and the one right below
could call?

[snip]

> +static void ocelot_port_adjust_link(struct net_device *dev)
> +{

This is fine for now, but I would suggest implementing PHYLINK to be
future proof.

[snip]

> +static int ocelot_port_stop(struct net_device *dev)
> +{
> +     struct ocelot_port *port = netdev_priv(dev);
> +
> +     phy_disconnect(port->phy);
> +
> +     dev->phydev = NULL;

You don't have anything else to do, like disabling the port so it
possibly saves power or anything, aside from the PHY which will be
suspended here.

[snip]

> +static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +     struct ocelot_port *port = netdev_priv(dev);
> +     struct ocelot *ocelot = port->ocelot;
> +     u32 val, ifh[IFH_LEN];
> +     struct frame_info info = {};
> +     u8 grp = 0; /* Send everything on CPU group 0 */
> +     int i, count, last;

unsigned int for these types.

> +
> +     val = ocelot_read(ocelot, QS_INJ_STATUS);
> +     if (!(val & QS_INJ_STATUS_FIFO_RDY(BIT(grp))) ||
> +         (val & QS_INJ_STATUS_WMARK_REACHED(BIT(grp))))
> +             return NETDEV_TX_BUSY;
> +
> +     ocelot_write_rix(ocelot, QS_INJ_CTRL_GAP_SIZE(1) |
> +                      QS_INJ_CTRL_SOF, QS_INJ_CTRL, grp);
> +
> +     info.port = BIT(port->chip_port);
> +     info.cpuq = 0xff;
> +     ocelot_gen_ifh(ifh, &info);
> +
> +     for (i = 0; i < IFH_LEN; i++)
> +             ocelot_write_rix(ocelot, ifh[i], QS_INJ_WR, grp);
> +
> +     count = (skb->len + 3) / 4;
> +     last = skb->len % 4;
> +     for (i = 0; i < count; i++) {
> +             ocelot_write_rix(ocelot, cpu_to_le32(((u32 *)skb->data)[i]),
> +                              QS_INJ_WR, grp);
> +     }
> +
> +     /* Add padding */
> +     while (i < (OCELOT_BUFFER_CELL_SZ / 4)) {
> +             ocelot_write_rix(ocelot, 0, QS_INJ_WR, grp);
> +             i++;
> +     }
> +
> +     /* Indicate EOF and valid bytes in last word */
> +     ocelot_write_rix(ocelot, QS_INJ_CTRL_GAP_SIZE(1) |
> +                      QS_INJ_CTRL_VLD_BYTES(skb->len < OCELOT_BUFFER_CELL_SZ 
> ? 0 : last) |
> +                      QS_INJ_CTRL_EOF,
> +                      QS_INJ_CTRL, grp);
> +
> +     /* Add dummy CRC */
> +     ocelot_write_rix(ocelot, 0, QS_INJ_WR, grp);
> +     skb_tx_timestamp(skb);
> +
> +     dev->stats.tx_packets++;
> +     dev->stats.tx_bytes += skb->len;
> +     dev_kfree_skb_any(skb);

No interrupt to indicate transmit completion?


> +static int ocelot_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
> +                       struct net_device *dev, const unsigned char *addr,
> +                       u16 vid, u16 flags)
> +{
> +     struct ocelot_port *port = netdev_priv(dev);
> +     struct ocelot *ocelot = port->ocelot;
> +
> +     if (!vid) {
> +             if (!port->vlan_aware)
> +                     /* If the bridge is not VLAN aware and no VID was
> +                      * provided, set it to 1 as bridges have a default VID
> +                      * of 1. Otherwise the MAC entry wouldn't match incoming
> +                      * packets as the VID would differ (0 != 1).
> +                      */
> +                     vid = 1;
> +             else
> +                     /* If the bridge is VLAN aware a VID must be provided as
> +                      * otherwise the learnt entry wouldn't match any frame.
> +                      */
> +                     return -EINVAL;
> +     }

So if we are targeting vid = 0 we end-up with vid = 1 possibly?

[snip]

> +static int ocelot_port_attr_stp_state_set(struct ocelot_port *ocelot_port,
> +                                       struct switchdev_trans *trans,
> +                                       u8 state)
> +{
> +     struct ocelot *ocelot = ocelot_port->ocelot;
> +     u32 port_cfg;
> +     int port, i;
> +
> +     if (switchdev_trans_ph_prepare(trans))
> +             return 0;
> +
> +     if (!(BIT(ocelot_port->chip_port) & ocelot->bridge_mask))
> +             return 0;
> +
> +     port_cfg = ocelot_read_gix(ocelot, ANA_PORT_PORT_CFG,
> +                                ocelot_port->chip_port);
> +
> +     switch (state) {
> +     case BR_STATE_FORWARDING:
> +             ocelot->bridge_fwd_mask |= BIT(ocelot_port->chip_port);
> +             /* Fallthrough */
> +     case BR_STATE_LEARNING:
> +             port_cfg |= ANA_PORT_PORT_CFG_LEARN_ENA;
> +             break;
> +
> +     default:
> +             port_cfg &= ~ANA_PORT_PORT_CFG_LEARN_ENA;
> +             ocelot->bridge_fwd_mask &= ~BIT(ocelot_port->chip_port);

Missing break, even if this is the default case.

> +     }
> +
> +     ocelot_write_gix(ocelot, port_cfg, ANA_PORT_PORT_CFG,
> +                      ocelot_port->chip_port);
> +
> +     /* Apply FWD mask. The loop is needed to add/remove the current port as
> +      * a source for the other ports.
> +      */
> +     for (port = 0; port < ocelot->num_phys_ports; port++) {
> +             if (ocelot->bridge_fwd_mask & BIT(port)) {
> +                     unsigned long mask = ocelot->bridge_fwd_mask & 
> ~BIT(port);
> +
> +                     for (i = 0; i < ocelot->num_phys_ports; i++) {
> +                             unsigned long bond_mask = ocelot->lags[i];
> +
> +                             if (!bond_mask)
> +                                     continue;
> +
> +                             if (bond_mask & BIT(port)) {
> +                                     mask &= ~bond_mask;
> +                                     break;
> +                             }
> +                     }
> +
> +                     ocelot_write_rix(ocelot,
> +                                      BIT(ocelot->num_phys_ports) | mask,
> +                                      ANA_PGID_PGID, PGID_SRC + port);
> +             } else {
> +                     /* Only the CPU port, this is compatible with link
> +                      * aggregation.
> +                      */
> +                     ocelot_write_rix(ocelot,
> +                                      BIT(ocelot->num_phys_ports),
> +                                      ANA_PGID_PGID, PGID_SRC + port);
> +             }

All of this sounds like it should be moved into the br_join/leave, this
does not appear to be the right place to do that.

[snip]

> +static int ocelot_port_attr_set(struct net_device *dev,
> +                             const struct switchdev_attr *attr,
> +                             struct switchdev_trans *trans)
> +{
> +     struct ocelot_port *ocelot_port = netdev_priv(dev);
> +     int err = 0;

Should not this be EOPNOTSUPP by default so your cases below are
properly handled, like BRIDGE_FLAGS, MROUTER etc.

> +
> +     switch (attr->id) {
> +     case SWITCHDEV_ATTR_ID_PORT_STP_STATE:
> +             ocelot_port_attr_stp_state_set(ocelot_port, trans,
> +                                            attr->u.stp_state);
> +             break;
> +     case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
> +             break;
> +     case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
> +             ocelot_port_attr_ageing_set(ocelot_port, attr->u.ageing_time);
> +             break;
> +     case SWITCHDEV_ATTR_ID_PORT_MROUTER:
> +             break;
> +     case SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED:
> +             ocelot_port_attr_mc_set(ocelot_port, !attr->u.mc_disabled);
> +             break;
> +     default:
> +             err = -EOPNOTSUPP;
> +             break;
> +     }
> +
> +     return err;
> +}
> +
> +static struct ocelot_multicast *ocelot_multicast_get(struct ocelot *ocelot,
> +                                                  const unsigned char *addr,
> +                                                  u16 vid)
> +{
> +     struct ocelot_multicast *mc;
> +
> +     list_for_each_entry(mc, &ocelot->multicast, list) {
> +             if (ether_addr_equal(mc->addr, addr) && mc->vid == vid)
> +                     return mc;
> +     }
> +
> +     return NULL;
> +}


> +static irqreturn_t ocelot_xtr_irq_handler(int irq, void *arg)
> +{
> +     struct ocelot *ocelot = arg;
> +     int i = 0, grp = 0;
> +     int err = 0;
> +
> +     if (!(ocelot_read(ocelot, QS_XTR_DATA_PRESENT) & BIT(grp)))
> +             return IRQ_NONE;
> +
> +     do {
> +             struct sk_buff *skb;
> +             struct net_device *dev;
> +             u32 *buf;
> +             int sz, len;
> +             u32 ifh[4];
> +             u32 val;
> +             struct frame_info info;
> +
> +             for (i = 0; i < IFH_LEN; i++) {
> +                     err = ocelot_rx_frame_word(ocelot, grp, true, &ifh[i]);
> +                     if (err != 4)
> +                             break;
> +             }

NAPI maybe?

[snip]


> +     ocelot->targets[SYS] = ocelot_io_platform_init(ocelot, pdev, "sys");
> +     if (IS_ERR(ocelot->targets[SYS]))
> +             return PTR_ERR(ocelot->targets[SYS]);

You can clearly make this in a loop instead of repeating this section,
you just need an array of register names to be looking for.

[snip]

> +     if (np) {

Please rework the indentation here, check for !np

> +             for_each_child_of_node(np, portnp) {

for_each_available_child_of_node() you should be able to mark specific
ports as being disabled and skip over these accordingly.


[snip]
> +int ocelot_regfields_init(struct ocelot *ocelot,
> +                       const struct reg_field *const regfields)
> +{
> +     int i;

unsigned int i
-- 
Florian

Reply via email to