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.belloni@xxxxxxxxxxx>
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
