-----Original Message-----
From: Sergey Shtylyov <redacted>
Sent: 30 September 2021 21:39
To: Biju Das <biju.das.jz@bp.renesas.com>; David S. Miller
[off-list ref]; Jakub Kicinski [off-list ref]
Cc: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>;
Andrew Lunn [off-list ref]; Sergei Shtylyov [off-list ref];
Geert Uytterhoeven [off-list ref]; Adam Ford
[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]
Subject: Re: [RFC/PATCH 18/18] ravb: Add set_feature support for RZ/G2L

On 9/23/21 5:08 PM, Biju Das wrote:

quoted

This patch adds set_feature support for RZ/G2L.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      | 32 ++++++++++++++
 drivers/net/ethernet/renesas/ravb_main.c | 56
+++++++++++++++++++++++-
 2 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h

b/drivers/net/ethernet/renesas/ravb.h
index d42e8ea981df..2275f27c0672 100644

--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h

@@ -209,6 +209,8 @@ enum ravb_reg {
 	CXR56	= 0x0770,	/* Documented for RZ/G2L only */
 	MAFCR	= 0x0778,
 	CSR0     = 0x0800,	/* Documented for RZ/G2L only */
+	CSR1     = 0x0804,	/* Documented for RZ/G2L only */
+	CSR2     = 0x0808,	/* Documented for RZ/G2L only */

   These are the TOE regs (CSR0 included), they only exist on RZ/G2L, no?

[...]

quoted

@@ -978,6 +980,36 @@ enum CSR0_BIT {
 	CSR0_RPE	= 0x00000020,
 };

   *enum* CSR0_BIT should be here (as we concluded).

quoted

+enum CSR1_BIT {

[...]

quoted

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

b/drivers/net/ethernet/renesas/ravb_main.c
index 72aea5875bc5..641ae5553b64 100644

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

[...]

quoted

@@ -2290,7 +2308,38 @@ static void ravb_set_rx_csum(struct net_device

*ndev, bool enable)  static int ravb_set_features_rgeth(struct

net_device *ndev,

quoted

 				   netdev_features_t features)
 {
-	/* Place holder */
+	netdev_features_t changed = features ^ ndev->features;
+	unsigned int reg;

   u32 reg;

quoted

+	int error;
+
+	reg = ravb_read(ndev, CSR0);

   ... as this function returns u32.

quoted

+
+	ravb_write(ndev, reg & ~(CSR0_RPE | CSR0_TPE), CSR0);
+	error = ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0);
+	if (error) {
+		ravb_write(ndev, reg, 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, I don't understand this... it would be nice if someone
knowledgeable about the offloads would look at this... Although, without
the register documentation it's possibly vain...

You can download the document from here [1]

[1] https://www.renesas.com/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0?language=en&r=1467981

Regards,
Biju

quoted

+		} else {
+			ravb_write(ndev, 0, CSR1);
+		}
+	}
+	ravb_write(ndev, reg, CSR0);
+
+	ndev->features = features;
+
 	return 0;
 }

@@ -2432,6 +2481,11 @@ static const struct ravb_hw_info rgeth_hw_info =

{

quoted

 	.set_feature = ravb_set_features_rgeth,
 	.dmac_init = ravb_dmac_init_rgeth,
 	.emac_init = ravb_emac_init_rgeth,
+	.net_hw_features = (NETIF_F_HW_CSUM | NETIF_F_RXCSUM),
+	.gstrings_stats = ravb_gstrings_stats_rgeth,
+	.gstrings_size = sizeof(ravb_gstrings_stats_rgeth),
+	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_rgeth),

    These seem unrelated, couldn't it be moved to a spearate patch?

quoted

+	.max_rx_len = RGETH_RX_BUFF_MAX + RAVB_ALIGN - 1,

   This seems unrelsated and misplaced too.

[...]

MBR, Sergey