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@xxxxxxxxxxx> > Reviewed-by: Andrew Lunn <andrew@xxxxxxx> > + 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. > + 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 > + 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 > + 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); > + } > +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? > + 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? > + 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. > +} > + > +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() > + 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; > +} > +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. > +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? > + } > + 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. > + /* 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. > + 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. > + 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. > + /* 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. > + u8 plat_endian; > + #define PLAT_LITTLE_ENDIAN 0 > + #define PLAT_BIG_ENDIAN 1 Why do you store this little nugget of information? > +/* 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? > +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?
