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