Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
flat view
From: Jakub Kicinski <kuba@kernel.org>
Date: 2020-12-05 03:37:57
Also in:
linux-arm-kernel, linux-devicetree, linux-samsung-soc, lkml
Possibly related (same subject, not in this thread)
- 2020-12-17 · Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver · Jakub Kicinski <kuba@kernel.org>
- 2020-12-17 · Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver · Lukasz Stelmach <l.stelmach@samsung.com>
- 2020-12-16 · Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver · Jakub Kicinski <kuba@kernel.org>
- 2020-12-16 · Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver · Lukasz Stelmach <l.stelmach@samsung.com>
- 2020-12-16 · Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver · Jakub Kicinski <kuba@kernel.org>
On Wed, 2 Dec 2020 22:47:09 +0100 Łukasz Stelmach wrote:
ASIX AX88796[1] is a versatile ethernet adapter chip, that can be connected to a CPU with a 8/16-bit bus or with an SPI. This driver supports SPI connection. The driver has been ported from the vendor kernel for ARTIK5[2] boards. Several changes were made to adapt it to the current kernel which include: + updated DT configuration, + clock configuration moved to DT, + new timer, ethtool and gpio APIs, + dev_* instead of pr_* and custom printk() wrappers, + removed awkward vendor power managemtn. + introduced ethtool tunable to control SPI compression [1] https://www.asix.com.tw/products.php?op=pItemdetail&PItemID=104;65;86&PLine=65 [2] https://git.tizen.org/cgit/profile/common/platform/kernel/linux-3.10-artik/ The other ax88796 driver is for NE2000 compatible AX88796L chip. These chips are not compatible. Hence, two separate drivers are required. Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
quoted hunk ↗ jump to hunk
+ case ETHTOOL_SPI_COMPRESSION: + if (netif_running(ndev)) + return -EBUSY; + if ((*(u32 *)data) != 1) + return -EINVAL; + ax_local->capabilities &= ~AX_CAP_COMP; + ax_local->capabilities |= (*(u32 *)data) == 1 ? AX_CAP_COMP : 0;
Since you're just using a single bit of information please use a private driver flag.
quoted hunk ↗ jump to hunk
+ headroom = skb_headroom(skb); + tailroom = skb_tailroom(skb); + padlen = ((pkt_len + 3) & 0x7FC) - pkt_len;
round_up(pkt_len, 4) - pkt_len;
+ tol_len = ((pkt_len + 3) & 0x7FC) + + TX_OVERHEAD + TX_EOP_SIZE + spi_len;
Ditto
quoted hunk ↗ jump to hunk
+ seq_num = ++ax_local->seq_num & 0x1F; + + info = (struct tx_pkt_info *)skb->cb; + info->pkt_len = pkt_len; + + if ((!skb_cloned(skb)) && + (headroom >= (TX_OVERHEAD + spi_len)) && + (tailroom >= (padlen + TX_EOP_SIZE))) {
+ } else {I think you can just use pskb_expand_head() instead of all this
quoted hunk ↗ jump to hunk
+ tx_skb = alloc_skb(tol_len, GFP_KERNEL); + if (!tx_skb) + return NULL; + + /* Write SPI TXQ header */ + memcpy(skb_put(tx_skb, spi_len), ax88796c_tx_cmd_buf, spi_len); + + info->seq_num = seq_num; + ax88796c_proc_tx_hdr(info, skb->ip_summed); + + /* SOP and SEG header */ + memcpy(skb_put(tx_skb, TX_OVERHEAD), + &info->sop, TX_OVERHEAD); + + /* Packet */ + memcpy(skb_put(tx_skb, ((pkt_len + 3) & 0xFFFC)), + skb->data, pkt_len); + + /* EOP header */ + memcpy(skb_put(tx_skb, TX_EOP_SIZE), + &info->eop, TX_EOP_SIZE); + + skb_unlink(skb, q); + dev_kfree_skb(skb); + }
quoted hunk ↗ jump to hunk
+static void +ax88796c_skb_return(struct ax88796c_device *ax_local, struct sk_buff *skb, + struct rx_header *rxhdr) +{ + struct net_device *ndev = ax_local->ndev; + int status; + + do { + if (!(ndev->features & NETIF_F_RXCSUM)) + break; + + /* checksum error bit is set */ + if ((rxhdr->flags & RX_HDR3_L3_ERR) || + (rxhdr->flags & RX_HDR3_L4_ERR)) + break; + + /* Other types may be indicated by more than one bit. */ + if ((rxhdr->flags & RX_HDR3_L4_TYPE_TCP) || + (rxhdr->flags & RX_HDR3_L4_TYPE_UDP)) + skb->ip_summed = CHECKSUM_UNNECESSARY; + } while (0); + + ax_local->stats.rx_packets++; + ax_local->stats.rx_bytes += skb->len; + skb->dev = ndev; + + skb->truesize = skb->len + sizeof(struct sk_buff);
Why do you modify truesize?
quoted hunk ↗ jump to hunk
+ skb->protocol = eth_type_trans(skb, ax_local->ndev); + + netif_info(ax_local, rx_status, ndev, "< rx, len %zu, type 0x%x\n", + skb->len + sizeof(struct ethhdr), skb->protocol); + + status = netif_rx_ni(skb); + if (status != NET_RX_SUCCESS) + netif_info(ax_local, rx_err, ndev, + "netif_rx status %d\n", status); +} + +static void +ax88796c_rx_fixup(struct ax88796c_device *ax_local, struct sk_buff *rx_skb) +{ + struct rx_header *rxhdr = (struct rx_header *)rx_skb->data; + struct net_device *ndev = ax_local->ndev; + u16 len; + + be16_to_cpus(&rxhdr->flags_len); + be16_to_cpus(&rxhdr->seq_lenbar); + be16_to_cpus(&rxhdr->flags); + + if ((((short)rxhdr->flags_len) & RX_HDR1_PKT_LEN) != + (~((short)rxhdr->seq_lenbar) & 0x7FF)) {
Why do you cast these values to signed types? Is the casting necessary at all?
quoted hunk ↗ jump to hunk
+ netif_err(ax_local, rx_err, ndev, "Header error\n"); + + ax_local->stats.rx_frame_errors++; + kfree_skb(rx_skb); + return; + } + + if ((rxhdr->flags_len & RX_HDR1_MII_ERR) || + (rxhdr->flags_len & RX_HDR1_CRC_ERR)) { + netif_err(ax_local, rx_err, ndev, "CRC or MII error\n"); + + ax_local->stats.rx_crc_errors++; + kfree_skb(rx_skb); + return; + } + + len = rxhdr->flags_len & RX_HDR1_PKT_LEN; + if (netif_msg_pktdata(ax_local)) { + char pfx[IFNAMSIZ + 7]; + + snprintf(pfx, sizeof(pfx), "%s: ", ndev->name); + netdev_info(ndev, "RX data, total len %d, packet len %d\n", + rx_skb->len, len); + + netdev_info(ndev, " Dump RX packet header:"); + print_hex_dump(KERN_INFO, pfx, DUMP_PREFIX_OFFSET, 16, 1, + rx_skb->data, sizeof(*rxhdr), 0); + + netdev_info(ndev, " Dump RX packet:"); + print_hex_dump(KERN_INFO, pfx, DUMP_PREFIX_OFFSET, 16, 1, + rx_skb->data + sizeof(*rxhdr), len, 0); + } + + skb_pull(rx_skb, sizeof(*rxhdr)); + __pskb_trim(rx_skb, len);
Why __pskb_trim? skb_trim() should do.
+ return ax88796c_skb_return(ax_local, rx_skb, rxhdr);
please drop the "return", both caller and callee are void.
quoted hunk ↗ jump to hunk
+} + +static int ax88796c_receive(struct net_device *ndev) +{ + struct ax88796c_device *ax_local = to_ax88796c_device(ndev); + struct skb_data *entry; + u16 w_count, pkt_len; + struct sk_buff *skb; + u8 pkt_cnt; + + WARN_ON(!mutex_is_locked(&ax_local->spi_lock)); + + /* check rx packet and total word count */ + AX_WRITE(&ax_local->ax_spi, AX_READ(&ax_local->ax_spi, P0_RTWCR) + | RTWCR_RX_LATCH, P0_RTWCR); + + pkt_cnt = AX_READ(&ax_local->ax_spi, P0_RXBCR2) & RXBCR2_PKT_MASK; + if (!pkt_cnt) + return 0; + + pkt_len = AX_READ(&ax_local->ax_spi, P0_RCPHR) & 0x7FF; + + w_count = ((pkt_len + 6 + 3) & 0xFFFC) >> 1; + + skb = alloc_skb((w_count * 2), GFP_ATOMIC);
netdev_alloc_skb()
quoted hunk ↗ jump to hunk
+ if (!skb) { + AX_WRITE(&ax_local->ax_spi, RXBCR1_RXB_DISCARD, P0_RXBCR1); + return 0; + } + entry = (struct skb_data *)skb->cb; + + AX_WRITE(&ax_local->ax_spi, RXBCR1_RXB_START | w_count, P0_RXBCR1); + + axspi_read_rxq(&ax_local->ax_spi, + skb_put(skb, w_count * 2), skb->len); + + /* Check if rx bridge is idle */ + if ((AX_READ(&ax_local->ax_spi, P0_RXBCR2) & RXBCR2_RXB_IDLE) == 0) { + netif_err(ax_local, rx_err, ndev, + "Rx Bridge is not idle\n"); + AX_WRITE(&ax_local->ax_spi, RXBCR2_RXB_REINIT, P0_RXBCR2); + + entry->state = rx_err; + } else { + entry->state = rx_done; + } + + AX_WRITE(&ax_local->ax_spi, ISR_RXPKT, P0_ISR); + + ax88796c_rx_fixup(ax_local, skb); + + return 1; +}
quoted hunk ↗ jump to hunk
+static irqreturn_t ax88796c_interrupt(int irq, void *dev_instance) +{ + struct ax88796c_device *ax_local; + struct net_device *ndev; + + ndev = dev_instance; + if (!ndev) { + pr_err("irq %d for unknown device.\n", irq); + return IRQ_RETVAL(0); + } + ax_local = to_ax88796c_device(ndev); + + disable_irq_nosync(irq); + + netif_dbg(ax_local, intr, ndev, "Interrupt occurred\n"); + + set_bit(EVENT_INTR, &ax_local->flags); + schedule_work(&ax_local->ax_work); + + return IRQ_HANDLED; +}
Since you always punt to a workqueue did you consider just using threaded interrupts instead? Also you're not checking if this is actually your devices IRQ that triggered. You can't set IRQF_SHARED.
quoted hunk ↗ jump to hunk
+static int +ax88796c_open(struct net_device *ndev) +{ + struct ax88796c_device *ax_local = to_ax88796c_device(ndev); + unsigned long irq_flag = IRQF_SHARED; + int fc = AX_FC_NONE; + int ret; + + ret = request_irq(ndev->irq, ax88796c_interrupt, + irq_flag, ndev->name, ndev); + if (ret) { + netdev_err(ndev, "unable to get IRQ %d (errno=%d).\n", + ndev->irq, ret); + return ret; + } + + mutex_lock(&ax_local->spi_lock); + + ret = ax88796c_soft_reset(ax_local); + if (ret < 0) { + mutex_unlock(&ax_local->spi_lock); + return ret;
What frees the IRQ?
quoted hunk ↗ jump to hunk
+ } + ax_local->seq_num = 0x1f; + + ax88796c_set_mac_addr(ndev); + ax88796c_set_csums(ax_local); + + /* Disable stuffing packet */ + AX_WRITE(&ax_local->ax_spi, + AX_READ(&ax_local->ax_spi, P1_RXBSPCR) + & ~RXBSPCR_STUF_ENABLE, P1_RXBSPCR);
Please use a temporary variable or create a RMW helper.
quoted hunk ↗ jump to hunk
+ /* Enable RX packet process */ + AX_WRITE(&ax_local->ax_spi, RPPER_RXEN, P1_RPPER); + + AX_WRITE(&ax_local->ax_spi, AX_READ(&ax_local->ax_spi, P0_FER) + | FER_RXEN | FER_TXEN | FER_BSWAP | FER_IRQ_PULL, P0_FER);
Ditto. etc.
quoted hunk ↗ jump to hunk
+ ndev = devm_alloc_etherdev(&spi->dev, sizeof(*ax_local)); + if (!ndev) + return -ENOMEM; + + SET_NETDEV_DEV(ndev, &spi->dev); + + ax_local = to_ax88796c_device(ndev); + memset(ax_local, 0, sizeof(*ax_local));
No need to zero out netdev priv, it's zalloced.
quoted hunk ↗ jump to hunk
+ mutex_lock(&ax_local->spi_lock); + + /* ax88796c gpio reset */ + ax88796c_hard_reset(ax_local); + + /* Reset AX88796C */ + ret = ax88796c_soft_reset(ax_local); + if (ret < 0) { + ret = -ENODEV; + goto err; + } + /* Check board revision */ + temp = AX_READ(&ax_local->ax_spi, P2_CRIR); + if ((temp & 0xF) != 0x0) { + dev_err(&spi->dev, "spi read failed: %d\n", temp); + ret = -ENODEV; + goto err; + }
These jump out without releasing the lock.
quoted hunk ↗ jump to hunk
+ /* Disable power saving */ + AX_WRITE(&ax_local->ax_spi, (AX_READ(&ax_local->ax_spi, P0_PSCR) + & PSCR_PS_MASK) | PSCR_PS_D0, P0_PSCR);
This is asking for a temporary variable or a RMW helper.
quoted hunk ↗ jump to hunk
+ u8 plat_endian; + #define PLAT_LITTLE_ENDIAN 0 + #define PLAT_BIG_ENDIAN 1
Why do you store this little nugget of information?
quoted hunk ↗ jump to hunk
+/* Tx headers structure */ +struct tx_sop_header { + /* bit 15-11: flags, bit 10-0: packet length */ + u16 flags_len; + /* bit 15-11: sequence number, bit 11-0: packet length bar */ + u16 seq_lenbar; +} __packed; + +struct tx_segment_header { + /* bit 15-14: flags, bit 13-11: segment number */ + /* bit 10-0: segment length */ + u16 flags_seqnum_seglen; + /* bit 15-14: end offset, bit 13-11: start offset */ + /* bit 10-0: segment length bar */ + u16 eo_so_seglenbar; +} __packed; + +struct tx_eop_header { + /* bit 15-11: sequence number, bit 10-0: packet length */ + u16 seq_len; + /* bit 15-11: sequence number bar, bit 10-0: packet length bar */ + u16 seqbar_lenbar; +} __packed; + +struct tx_pkt_info { + struct tx_sop_header sop; + struct tx_segment_header seg; + struct tx_eop_header eop; + u16 pkt_len; + u16 seq_num; +} __packed; + +/* Rx headers structure */ +struct rx_header { + u16 flags_len; + u16 seq_lenbar; + u16 flags; +} __packed;
These all look like multiple of 2 bytes. Why do they need to be packed?
quoted hunk ↗ jump to hunk
+u16 axspi_read_reg(struct axspi_data *ax_spi, u8 reg) +{ + int ret; + int len = ax_spi->comp ? 3 : 4; + + ax_spi->cmd_buf[0] = 0x03; /* OP code read register */ + ax_spi->cmd_buf[1] = reg; /* register address */ + ax_spi->cmd_buf[2] = 0xFF; /* dumy cycle */ + ax_spi->cmd_buf[3] = 0xFF; /* dumy cycle */ + ret = spi_write_then_read(ax_spi->spi, + ax_spi->cmd_buf, len, + ax_spi->rx_buf, 2); + if (ret) + dev_err(&ax_spi->spi->dev, "%s() failed: ret = %d\n", __func__, ret); + else + le16_to_cpus(ax_spi->rx_buf); + + return *(u16 *)ax_spi->rx_buf;
No need to return some specific pattern on failure? Like 0xffff?