Hi Sergey Shtylyov, Thanks for the feedback. > -----Original Message----- > From: Sergey Shtylyov <s.shtylyov@xxxxxx> > Sent: Monday, February 5, 2024 7:01 PM > Subject: Re: [PATCH v4 net-next 1/2] ravb: Add Rx checksum offload support > for GbEth > > On 2/3/24 5:25 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..64bf29d01ad0 100644 > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > > @@ -205,7 +205,10 @@ enum ravb_reg { > > TLFRCR = 0x0758, > > RFCR = 0x0760, > > MAFCR = 0x0778, > > - CSR0 = 0x0800, /* RZ/G2L only */ > > + > > + /* RZ/G2L TOE registers */ > > Thanks. Though I think I'd prefer /* TOE registers (RZ/G2L only) */... OK. > > [...] > > @@ -978,6 +981,21 @@ enum CSR0_BIT { > > CSR0_RPE = 0x00000020, > > }; > > > > +enum CSR2_BIT { > > + CSR2_RIP4 = 0x00000001, > > + CSR2_RTCP4 = 0x00000010, > > + CSR2_RUDP4 = 0x00000020, > > + CSR2_RICMP4 = 0x00000040, > > + CSR2_RTCP6 = 0x00100000, > > + CSR2_RUDP6 = 0x00200000, > > + CSR2_RICMP6 = 0x00400000, > > + CSR2_RHOP = 0x01000000, > > + CSR2_RROUT = 0x02000000, > > + CSR2_RAHD = 0x04000000, > > + CSR2_RDHD = 0x08000000, > > + CSR2_ALL = 0x0F700071, > > I doubt we really need CSR2_ALL... Will remove it. > > [...] > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index 0e3731f50fc2..4f310bcee7c0 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -522,6 +522,24 @@ 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_RTCP6 | CSR2_RUDP6 | > > + CSR2_RICMP6), CSR2); > > With these 3 bits being 0, the bits 24...27 are ignored anyway, the > manual says. So I think I'd prefer: > > ravb_write(ndev, CSR2_RIP4 | CSR2_RTCP4 | CSR2_RUDP4 | > CSR2_RICMP4, > CSR2); OK. > > > + } > > + > > +done: > > + ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0); > > I think we shouldn't set CSR0.TPE yet at this point, as we n't setup > CSR1 yet... This will cause regression. As board won't boot as tx fails. So CSR0.{TPE,RPE} is must. > > [...] > > @@ -2334,11 +2381,48 @@ static void ravb_set_rx_csum(struct net_device > > *ndev, bool enable) > [...] > > 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 & ~(CSR2_RTCP6 | CSR2_RUDP6 | > CSR2_RICMP6); > > Likewise, I'd prefer: > > val = CSR2_RIP4 | CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4; Ok. > > > + 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, > [...] > > Otherwise LGTM. We're close! :-) I will send v5, based on above. Cheers, Biju