On 2/1/24 10:45 PM, Biju Das wrote: > TOE has hardware support for calculating IP header and TCP/UDP/ICMP > checksum for both IPv4 and IPv6. > > Add Rx checksum offload supported by TOE for IPv4 and TCP/UDP protocols. > > For Rx, the 4-byte result of checksum calculation is attached to the > Ethernet frames.First 2-bytes is result of IPv4 header checksum and next > 2-bytes is TCP/UDP/ICMP checksum. > > If a frame does not have checksum error, 0x0000 is attached as checksum > calculation result. For unsupported frames 0xFFFF is attached as checksum > calculation result. In case of an IPv6 packet, IPv4 checksum is always set > to 0xFFFF. > > We can test this functionality by the below commands > > ethtool -K eth0 rx on --> to turn on Rx checksum offload > ethtool -K eth0 rx off --> to turn off Rx checksum offload > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> [...] > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > index e0f8276cffed..a7fe9d8b6b41 100644 > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -206,6 +206,7 @@ enum ravb_reg { > RFCR = 0x0760, > MAFCR = 0x0778, Would be good to add this comment here: /* TOE registers */ > CSR0 = 0x0800, /* RZ/G2L only */ > + CSR2 = 0x0808, /* RZ/G2L only */ > }; > > [...] > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 0e3731f50fc2..c4dc6ec54287 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -522,6 +522,23 @@ static int ravb_ring_init(struct net_device *ndev, int q) > return -ENOMEM; > } > > +static void ravb_csum_init_gbeth(struct net_device *ndev) > +{ > + if (!(ndev->features & NETIF_F_RXCSUM)) > + goto done; > + > + ravb_write(ndev, 0, CSR0); > + if (ravb_wait(ndev, CSR0, CSR0_RPE, 0)) { > + netdev_err(ndev, "Timeout enabling hardware checksum\n"); > + ndev->features &= ~NETIF_F_RXCSUM; > + } else { > + ravb_write(ndev, CSR2_ALL, CSR2); Does it make sense to set the IPv6 specific bits if we don't yet support IPv6 checksumming anyways? > + } > + > +done: > + ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0); > +} > + > static void ravb_emac_init_gbeth(struct net_device *ndev) > { > struct ravb_private *priv = netdev_priv(ndev); [...] > @@ -734,6 +752,31 @@ static void ravb_get_tx_tstamp(struct net_device *ndev) > } > } > > +static void ravb_rx_csum_gbeth(struct sk_buff *skb) > +{ > + __wsum csum_ip_hdr, csum_proto; > + u8 *hw_csum; > + > + /* The hardware checksum status is contained in sizeof(__sum16) * 2 = 4 > + * bytes appended to packet data. First 2 bytes is ip header csum and > + * last 2 bytes is protocol csum. Hm, maybe spell out csum as checksum? > + */ > + if (unlikely(skb->len < sizeof(__sum16) * 2)) > + return; > + > + hw_csum = skb_tail_pointer(skb) - sizeof(__sum16); > + csum_proto = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum)); > + > + hw_csum -= sizeof(__sum16); > + csum_ip_hdr = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum)); > + skb_trim(skb, skb->len - 2 * sizeof(__sum16)); > + > + /* TODO: IPV6 Rx csum */ > + if (skb->protocol == htons(ETH_P_IP) && !csum_ip_hdr && !csum_proto) > + /* Hardware validated our checksum */ > + skb->ip_summed = CHECKSUM_UNNECESSARY; I think you need {} because of the comment (but would be good without it as well)... [...] > @@ -2334,11 +2381,49 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable) > spin_unlock_irqrestore(&priv->lock, flags); > } > > +static int ravb_endisable_csum_gbeth(struct net_device *ndev, enum ravb_reg reg, > + u32 val, u32 mask) I'd suggest to mimic ravb_wait() with the the mask param followed by the val[ue] param... > +{ > + u32 csr0; > + int ret; > + > + csr0 = ravb_read(ndev, CSR0); Hm... I think we already know CSR0's value as ravb_csum_init_gbeth() sets it to (CSR0_TPE | CSR0_RPE) on exit... > + ravb_write(ndev, csr0 & ~mask, CSR0); > + ret = ravb_wait(ndev, CSR0, mask, 0); > + if (!ret) > + ravb_write(ndev, val, reg); > + > + ravb_write(ndev, csr0, CSR0); > + > + return ret; > +} > + > static int ravb_set_features_gbeth(struct net_device *ndev, > netdev_features_t features) > { > - /* Place holder */ > - return 0; > + netdev_features_t changed = ndev->features ^ features; > + struct ravb_private *priv = netdev_priv(ndev); > + unsigned long flags; > + int ret = 0; > + u32 val; > + > + spin_lock_irqsave(&priv->lock, flags); > + if (changed & NETIF_F_RXCSUM) { > + if (features & NETIF_F_RXCSUM) > + val = CSR2_ALL; Again, does it make sense to set the IPv6 bits in CSR2? > + else > + val = 0; > + > + ret = ravb_endisable_csum_gbeth(ndev, CSR2, val, CSR0_RPE); > + if (ret) > + goto done; > + } > + > + ndev->features = features; > +done: > + spin_unlock_irqrestore(&priv->lock, flags); > + > + return ret; > } > > static int ravb_set_features_rcar(struct net_device *ndev, [...] MBR, Sergey