lkml.org 
[lkml]   [2018]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH net-next 5/8] net: mscc: Add initial Ocelot switch support
    From
    Date
    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@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

    \
     
     \ /
      Last update: 2018-03-23 22:42    [W:3.666 / U:0.152 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site