RE: [RFC 03/12] ravb: Fillup ravb_set_features_gbeth() stub

-----Original Message-----
From: Biju Das <biju.das.jz@bp.renesas.com>
Sent: 06 October 2021 08:44
To: Sergey Shtylyov <redacted>; David S. Miller
[off-list ref]; Jakub Kicinski [off-list ref]
Cc: Geert Uytterhoeven <geert+renesas@glider.be>; Sergey Shtylyov
[off-list ref]; Adam Ford [off-list ref]; Andrew Lunn
[off-list ref]; Yuusuke Ashizuka [off-list ref]; Yoshihiro
Shimoda [off-list ref]; netdev@vger.kernel.org; linux-
renesas-soc@vger.kernel.org; Chris Paterson [off-list ref];
Biju Das [off-list ref]; Prabhakar Mahadev Lad
[off-list ref]
Subject: RE: [RFC 03/12] ravb: Fillup ravb_set_features_gbeth() stub

Hi Sergei,

Thanks for the feedback.

quoted

Subject: Re: [RFC 03/12] ravb: Fillup ravb_set_features_gbeth() stub

On 10/5/21 2:06 PM, Biju Das wrote:

quoted

Fillup ravb_set_features_gbeth() function to support RZ/G2L.
Also set the net_hw_features bits supported by GbEthernet

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

[...]

quoted

diff --git a/drivers/net/ethernet/renesas/ravb_main.c

b/drivers/net/ethernet/renesas/ravb_main.c
index ed0328a90200..37f50c041114 100644

--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c

[...]

quoted

@@ -2086,7 +2087,37 @@ static void ravb_set_rx_csum(struct

net_device *ndev, bool enable)  static int
ravb_set_features_gbeth(struct

net_device *ndev,

quoted

 				   netdev_features_t features)
 {
-	/* Place holder */
+	netdev_features_t changed = features ^ ndev->features;
+	int error;
+	u32 csr0;
+
+	csr0 = ravb_read(ndev, CSR0);
+	ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0);
+	error = ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0);
+	if (error) {
+		ravb_write(ndev, csr0, CSR0);
+		return error;
+	}
+
+	if (changed & NETIF_F_RXCSUM) {
+		if (features & NETIF_F_RXCSUM)
+			ravb_write(ndev, CSR2_ALL, CSR2);
+		else
+			ravb_write(ndev, 0, CSR2);
+	}
+
+	if (changed & NETIF_F_HW_CSUM) {
+		if (features & NETIF_F_HW_CSUM) {
+			ravb_write(ndev, CSR1_ALL, CSR1);
+			ndev->features |= NETIF_F_CSUM_MASK;

   Hm, the >linux/netdev_features.h> says those are contradictory to
have both NETIF_F_HW_CSUM and NETIF_F_CSUM_MASK set...

It is a mistake from my side, I am taking out this setting. Any way below
code overrides it.
This will answer all your comments below.

I am deferring this patch and will take out  RX checksum offload functionality from patch#7

Will post this 2 patches as RFC, as looks like it needs more discussions related to HW checksum.

Regards,
Biju

Regards,
Biju

quoted

quoted

+		} else {
+			ravb_write(ndev, 0, CSR1);

   No need to mask off the 'features' field?

quoted

+		}
+	}
+	ravb_write(ndev, csr0, CSR0);
+
+	ndev->features = features;

   Mhm, doesn't that clear NETIF_F_CSUM_MASK?

[...]

MBR, Sergey