> +/* Forwarding engine block (MFWD) */ > +static void rswitch_fwd_init(struct rswitch_private *priv) > +{ > + int i; > + > + for (i = 0; i < RSWITCH_NUM_HW; i++) { > + iowrite32(FWPC0_DEFAULT, priv->addr + FWPC0(i)); > + iowrite32(0, priv->addr + FWPBFC(i)); > + } What is RSWITCH_NUM_HW? > + > + for (i = 0; i < RSWITCH_NUM_ETHA; i++) { RSWITCH_NUM_ETHA appears to be the number of ports? > +static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv, int rate) > +{ > + u32 gwgrlulc, gwgrlc; > + > + switch (rate) { > + case 1000: > + gwgrlulc = 0x0000005f; > + gwgrlc = 0x00010260; > + break; > + default: > + dev_err(&priv->pdev->dev, "%s: This rate is not supported (%d)\n", __func__, rate); > + return; > + } Is this setting the Mbps between the switch matrix and the CPU? Why limit the rate? Especially if you have 3 ports, would not 3000 make sense? > +static void rswitch_get_data_irq_status(struct rswitch_private *priv, u32 *dis) > +{ > + int i; > + > + for (i = 0; i < RSWITCH_NUM_IRQ_REGS; i++) { > + dis[i] = ioread32(priv->addr + GWDIS(i)); > + dis[i] &= ioread32(priv->addr + GWDIE(i)); > + } > +} > + > +static void rswitch_enadis_data_irq(struct rswitch_private *priv, int index, bool enable) > +{ > + u32 offs = enable ? GWDIE(index / 32) : GWDID(index / 32); > + > + iowrite32(BIT(index % 32), priv->addr + offs); > +} > + > +static void rswitch_ack_data_irq(struct rswitch_private *priv, int index) > +{ > + u32 offs = GWDIS(index / 32); > + > + iowrite32(BIT(index % 32), priv->addr + offs); > +} > + > +static bool rswitch_is_chain_rxed(struct rswitch_gwca_chain *c) > +{ > + struct rswitch_ext_ts_desc *desc; > + int entry; > + > + entry = c->dirty % c->num_ring; > + desc = &c->ts_ring[entry]; > + > + if ((desc->die_dt & DT_MASK) != DT_FEMPTY) > + return true; > + > + return false; Is a chain a queue? Also known as a ring? The naming is different to most drivers, which is making this harder to understand. Ideally, you want to follow the basic naming the majority of other drivers use. > +} > + > +static int rswitch_gwca_chain_alloc_skb(struct rswitch_gwca_chain *c, > + int start, int num) > +{ > + int i, index; > + > + for (i = start; i < (start + num); i++) { > + index = i % c->num_ring; Why this? Would it not make more sense to validate that start + num < num_ring? It seems like bad parameters passed here could destroy some other skb in the ring? More naming... Here you use num_ring, not num_chain. Try to be consistent. Also, num_ring makes my think of ring 7 of 9 rings. When this actually appears to be the size of the ring. So c->ring_size would be a better name. > + if (c->skb[index]) > + continue; > + c->skb[index] = dev_alloc_skb(PKT_BUF_SZ + RSWITCH_ALIGN - 1); > + if (!c->skb[index]) > + goto err; > + skb_reserve(c->skb[index], NET_IP_ALIGN); netdev_alloc_skb_ip_align()? > +static void rswitch_gwca_chain_free(struct net_device *ndev, > + struct rswitch_gwca_chain *c) > +{ > + int i; > + > + if (c->gptp) { > + dma_free_coherent(ndev->dev.parent, > + sizeof(struct rswitch_ext_ts_desc) * > + (c->num_ring + 1), c->ts_ring, c->ring_dma); > + c->ts_ring = NULL; > + } else { > + dma_free_coherent(ndev->dev.parent, > + sizeof(struct rswitch_ext_desc) * > + (c->num_ring + 1), c->ring, c->ring_dma); > + c->ring = NULL; > + } > + > + if (!c->dir_tx) { > + for (i = 0; i < c->num_ring; i++) > + dev_kfree_skb(c->skb[i]); > + } > + > + kfree(c->skb); > + c->skb = NULL; When i see code like this, i wonder why an API call like dev_kfree_skb() is not being used. I would suggest reaming this to something other than skb, which has a very well understood meaning. > +static bool rswitch_rx(struct net_device *ndev, int *quota) > +{ > + struct rswitch_device *rdev = netdev_priv(ndev); > + struct rswitch_gwca_chain *c = rdev->rx_chain; > + int entry = c->cur % c->num_ring; > + struct rswitch_ext_ts_desc *desc; > + int limit, boguscnt, num, ret; > + struct sk_buff *skb; > + dma_addr_t dma_addr; > + u16 pkt_len; > + > + boguscnt = min_t(int, c->dirty + c->num_ring - c->cur, *quota); > + limit = boguscnt; > + > + desc = &c->ts_ring[entry]; > + while ((desc->die_dt & DT_MASK) != DT_FEMPTY) { > + dma_rmb(); > + pkt_len = le16_to_cpu(desc->info_ds) & RX_DS; > + if (--boguscnt < 0) > + break; Why the barrier, read the length and then decide to break out of the loop? > +static int rswitch_open(struct net_device *ndev) > +{ > + struct rswitch_device *rdev = netdev_priv(ndev); > + struct device_node *phy; > + int err = 0; > + > + if (rdev->etha) { Can this happen? What would a netdev without an etha mean? Andrew