Hi Sergey,
> -----Original Message-----
> From: Sergey Shtylyov <s.shtylyov@xxxxxx>
> Sent: Monday, February 5, 2024 8:27 PM
> Subject: Re: [PATCH v4 net-next 2/2] ravb: Add Tx 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 Tx checksum offload supported by TOE for IPv4 and TCP/UDP.
> >
> > For Tx, the result of checksum calculation is set to the checksum
> > field of each IPv4 Header/TCP/UDP/ICMP of ethernet frames. For the
> > unsupported frames, those fields are not changed. If a transmission
> > frame is an UDPv4 frame and its checksum value in the UDP header field
> > is 0x0000, TOE does not calculate checksum for UDP part of this frame
> > as it is optional function as per standards.
> >
> > We can test this functionality by the below commands
> >
> > ethtool -K eth0 tx on --> to turn on Tx checksum offload ethtool -K
> > eth0 tx off --> to turn off Tx 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 64bf29d01ad0..d7b1c6d15a17 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> [...]
> > @@ -981,6 +982,21 @@ enum CSR0_BIT {
> > CSR0_RPE = 0x00000020,
> > };
> >
> > +enum CSR1_BIT {
> > + CSR1_TIP4 = 0x00000001,
> > + CSR1_TTCP4 = 0x00000010,
> > + CSR1_TUDP4 = 0x00000020,
> > + CSR1_TICMP4 = 0x00000040,
> > + CSR1_TTCP6 = 0x00100000,
> > + CSR1_TUDP6 = 0x00200000,
> > + CSR1_TICMP6 = 0x00400000,
> > + CSR1_THOP = 0x01000000,
> > + CSR1_TROUT = 0x02000000,
> > + CSR1_TAHD = 0x04000000,
> > + CSR1_TDHD = 0x08000000,
> > + CSR1_ALL = 0x0F700071,
>
> I doubt we really need CSR1_ALL...
OK, will drop it.
>
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 4f310bcee7c0..fee771f14fc5 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
> > @@ -524,16 +525,29 @@ static int ravb_ring_init(struct net_device
> > *ndev, int q)
> >
> > static void ravb_csum_init_gbeth(struct net_device *ndev) {
> > - if (!(ndev->features & NETIF_F_RXCSUM))
> > + bool tx_enable = ndev->features & NETIF_F_HW_CSUM;
> > + bool rx_enable = ndev->features & NETIF_F_RXCSUM;
> > +
> > + if (!(tx_enable || rx_enable))
> > goto done;
> >
> > ravb_write(ndev, 0, CSR0);
> > - if (ravb_wait(ndev, CSR0, CSR0_RPE, 0)) {
> > + if (ravb_wait(ndev, CSR0, CSR0_TPE | CSR0_RPE, 0)) {
> > netdev_err(ndev, "Timeout enabling hardware checksum\n");
> > - ndev->features &= ~NETIF_F_RXCSUM;
> > +
> > + if (tx_enable)
> > + ndev->features &= ~NETIF_F_HW_CSUM;
> > +
> > + if (rx_enable)
> > + ndev->features &= ~NETIF_F_RXCSUM;
> > } else {
> > - ravb_write(ndev, CSR2_ALL & ~(CSR2_RTCP6 | CSR2_RUDP6 |
> > - CSR2_RICMP6), CSR2);
> > + if (tx_enable)
> > + ravb_write(ndev, CSR1_ALL & ~(CSR1_TICMP4 | CSR1_TTCP6 |
> > + CSR1_TUDP6 | CSR1_TICMP6),
> CSR1);
>
> With the v6 bits 20...22 being 0, the bits 24...27 are ignored anyway,
> the manual says. So I think I'd prefer:
>
> ravb_write(ndev, CSR1_TIP4 | CSR1_TTCP4 | CSR1_TUDP4,
> CSR1); [...]
OK.
> > @@ -2418,6 +2465,18 @@ static int ravb_set_features_gbeth(struct
> net_device *ndev,
> > goto done;
> > }
> >
> > + if (changed & NETIF_F_HW_CSUM) {
> > + if (features & NETIF_F_HW_CSUM)
> > + val = CSR1_ALL & ~(CSR1_TICMP4 | CSR1_TTCP6 |
> > + CSR1_TUDP6 | CSR1_TICMP6);
>
> Likewise, I'd prefer:
>
> val = CSR1_TIP4 | CSR1_TTCP4 | CS12_TUDP4;
OK, val = CSR1_TIP4 | CSR1_TTCP4 | CSR1_TUDP4;
Cheers,
Biju
>
> [...]
>
> MBR, Sergey
