[RFC 00/12] Add functional support for Gigabit Ethernet driver

[RFC 00/12] Add functional support for Gigabit Ethernet driver

[Biju Das]

The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC are
similar to the R-Car Ethernet AVB IP.

The Gigabit Ethernet IP consists of Ethernet controller (E-MAC), Internal
TCP/IP Offload Engine (TOE)  and Dedicated Direct memory access controller
(DMAC).

With a few changes in the driver we can support both IPs.

This patch series is aims to add functional support for Gigabit Ethernet driver
by filling all the stubs.

Ref:-
https://lore.kernel.org/linux-renesas-soc/OS0PR01MB5922240F88E5E0FD989ECDF386AC9@OS0PR01MB5922.jpnprd01.prod.outlook.com/T/#m8dee0a1b14d505d4611cad8c10e4017a30db55d6

RFC changes:
 * used ALIGN macro for calculating the value for max_rx_len.
 * used rx_max_buf_size instead of rx_2k_buffers feature bit.
 * moved struct ravb_rx_desc *gbeth_rx_ring near to ravb_private::rx_ring
   and allocating it for 1 RX queue.
 * Started using gbeth_rx_ring instead of gbeth_rx_ring[q].
 * renamed ravb_alloc_rx_desc to ravb_alloc_rx_desc_rcar
 * renamed ravb_rx_ring_free to ravb_rx_ring_free_rcar
 * renamed ravb_rx_ring_format to ravb_rx_ring_format_rcar
 * renamed ravb_rcar_rx to ravb_rx_rcar
 * renamed "tsrq" variable
 * Updated the comments

Biju Das (12):
  ravb: Use ALIGN macro for max_rx_len
  ravb: Add rx_max_buf_size to struct ravb_hw_info
  ravb: Fillup ravb_set_features_gbeth() stub
  ravb: Fillup ravb_alloc_rx_desc_gbeth() stub
  ravb: Fillup ravb_rx_ring_free_gbeth() stub
  ravb: Fillup ravb_rx_ring_format_gbeth() stub
  ravb: Fillup ravb_rx_gbeth() stub
  ravb: Add carrier_counters to struct ravb_hw_info
  ravb: Add support to retrieve stats for GbEthernet
  ravb: Rename "tsrq" variable
  ravb: Optimize ravb_emac_init_gbeth function
  ravb: Update/Add comments

 drivers/net/ethernet/renesas/ravb.h      |  51 +++-
 drivers/net/ethernet/renesas/ravb_main.c | 349 +++++++++++++++++++++--
 2 files changed, 367 insertions(+), 33 deletions(-)

-- 
2.17.1

[RFC 01/12] ravb: Use ALIGN macro for max_rx_len

[Biju Das]

Use ALIGN macro for calculating the value for max_rx_len.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
 drivers/net/ethernet/renesas/ravb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 9a4888543384..56a97d583950 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2227,7 +2227,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.set_feature = ravb_set_features_gbeth,
 	.dmac_init = ravb_dmac_init_gbeth,
 	.emac_init = ravb_emac_init_gbeth,
-	.max_rx_len = GBETH_RX_BUFF_MAX + RAVB_ALIGN - 1,
+	.max_rx_len = ALIGN(GBETH_RX_BUFF_MAX, RAVB_ALIGN),
 	.tsrq = TCCR_TSRQ0,
 	.aligned_tx = 1,
 	.tx_counters = 1,
-- 
2.17.1

Re: [RFC 01/12] ravb: Use ALIGN macro for max_rx_len

[Sergey Shtylyov]

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

> Use ALIGN macro for calculating the value for max_rx_len.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

[RFC 02/12] ravb: Add rx_max_buf_size to struct ravb_hw_info

[Biju Das]

R-Car AVB-DMAC has maximum 2K size on RX buffer, whereas on RZ/G2L
it is 8K. We need to allow for changing the MTU within the limit
of the maximum size of a descriptor.

Add a rx_max_buf_size variable to struct ravb_hw_info to handle
this difference.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC:
 * used buffer_size instead of feature bit.
---
 drivers/net/ethernet/renesas/ravb.h      | 1 +
 drivers/net/ethernet/renesas/ravb_main.c | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 5dc1324786e0..b147c4a0dc0b 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1010,6 +1010,7 @@ struct ravb_hw_info {
 	int stats_len;
 	size_t max_rx_len;
 	u32 tsrq;
+	u32 rx_max_buf_size;
 	unsigned aligned_tx: 1;
 
 	/* hardware features */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 56a97d583950..ed0328a90200 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2188,6 +2188,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
 	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
 	.tsrq = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
+	.rx_max_buf_size = SZ_2K,
 	.internal_delay = 1,
 	.tx_counters = 1,
 	.multi_irqs = 1,
@@ -2212,6 +2213,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
 	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
 	.tsrq = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
+	.rx_max_buf_size = SZ_2K,
 	.aligned_tx = 1,
 	.gptp = 1,
 	.nc_queue = 1,
@@ -2229,6 +2231,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.emac_init = ravb_emac_init_gbeth,
 	.max_rx_len = ALIGN(GBETH_RX_BUFF_MAX, RAVB_ALIGN),
 	.tsrq = TCCR_TSRQ0,
+	.rx_max_buf_size = SZ_8K,
 	.aligned_tx = 1,
 	.tx_counters = 1,
 	.half_duplex = 1,
@@ -2452,7 +2455,7 @@ static int ravb_probe(struct platform_device *pdev)
 	}
 	clk_prepare_enable(priv->refclk);
 
-	ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
+	ndev->max_mtu = info->rx_max_buf_size - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
 	ndev->min_mtu = ETH_MIN_MTU;
 
 	/* FIXME: R-Car Gen2 has 4byte alignment restriction for tx buffer
-- 
2.17.1

Re: [RFC 02/12] ravb: Add rx_max_buf_size to struct ravb_hw_info

[Sergey Shtylyov]

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

> R-Car AVB-DMAC has maximum 2K size on RX buffer, whereas on RZ/G2L
> it is 8K. We need to allow for changing the MTU within the limit
> of the maximum size of a descriptor.
> 
> Add a rx_max_buf_size variable to struct ravb_hw_info to handle
> this difference.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

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

[Biju Das]

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>
---
RFC changes:
 * Added CSR0 initilization
 * Seperated and created a new patch fro retrieving stats.
---
 drivers/net/ethernet/renesas/ravb.h      | 38 ++++++++++++++++++++++++
 drivers/net/ethernet/renesas/ravb_main.c | 34 ++++++++++++++++++++-
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index b147c4a0dc0b..02f425bf9544 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -202,6 +202,9 @@ enum ravb_reg {
 	TLFRCR	= 0x0758,
 	RFCR	= 0x0760,
 	MAFCR	= 0x0778,
+	CSR0    = 0x0800,	/* RZ/G2L only */
+	CSR1    = 0x0804,	/* RZ/G2L only */
+	CSR2    = 0x0808,	/* RZ/G2L only */
 };
 
 
@@ -962,6 +965,41 @@ enum CXR31_BIT {
 	CXR31_SEL_LINK1	= 0x00000008,
 };
 
+enum CSR0_BIT {
+	CSR0_TPE	= 0x00000010,
+	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,
+};
+
+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,
+};
+
 #define DBAT_ENTRY_NUM	22
 #define RX_QUEUE_OFFSET	4
 #define NUM_RX_QUEUE	2
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
@@ -481,6 +481,7 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
 
 	/* E-MAC status register clear */
 	ravb_write(ndev, ECSR_ICD | ECSR_LCHNG | ECSR_PFRI, ECSR);
+	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);
 
 	/* E-MAC interrupt enable register */
 	ravb_write(ndev, ECSIPR_ICDIP, ECSIPR);
@@ -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,
 				   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;
+		} else {
+			ravb_write(ndev, 0, CSR1);
+		}
+	}
+	ravb_write(ndev, csr0, CSR0);
+
+	ndev->features = features;
+
 	return 0;
 }
 
@@ -2229,6 +2260,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.set_feature = ravb_set_features_gbeth,
 	.dmac_init = ravb_dmac_init_gbeth,
 	.emac_init = ravb_emac_init_gbeth,
+	.net_hw_features = (NETIF_F_HW_CSUM | NETIF_F_RXCSUM),
 	.max_rx_len = ALIGN(GBETH_RX_BUFF_MAX, RAVB_ALIGN),
 	.tsrq = TCCR_TSRQ0,
 	.rx_max_buf_size = SZ_8K,
-- 
2.17.1

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

[Sergey Shtylyov]

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

> 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>
[...]

> 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
[...]
> @@ -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,
>  				   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...

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

   No need to mask off the 'features' field?

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

   Mhm, doesn't that clear NETIF_F_CSUM_MASK?

[...]

MBR, Sergey

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

[Biju Das]

Hi Sergei,

Thanks for the feedback.

> Subject: Re: [RFC 03/12] ravb: Fillup ravb_set_features_gbeth() stub
> 
> On 10/5/21 2:06 PM, Biju Das wrote:
> 
> > 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>
> [...]
> 
> > 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
> [...]
> > @@ -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,
> >  				   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.

Regards,
Biju

> 
> > +		} else {
> > +			ravb_write(ndev, 0, CSR1);
> 
>    No need to mask off the 'features' field?
> 
> > +		}
> > +	}
> > +	ravb_write(ndev, csr0, CSR0);
> > +
> > +	ndev->features = features;
> 
>    Mhm, doesn't that clear NETIF_F_CSUM_MASK?
> 
> [...]
> 
> MBR, Sergey

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

[Biju Das]

> -----Original Message-----
> From: Biju Das <biju.das.jz@bp.renesas.com>
> Sent: 06 October 2021 08:44
> To: Sergey Shtylyov <s.shtylyov@omp.ru>; David S. Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>; Sergey Shtylyov
> <s.shtylyov@omprussia.ru>; Adam Ford <aford173@gmail.com>; Andrew Lunn
> <andrew@lunn.ch>; Yuusuke Ashizuka <ashiduka@fujitsu.com>; Yoshihiro
> Shimoda <yoshihiro.shimoda.uh@renesas.com>; netdev@vger.kernel.org; linux-
> renesas-soc@vger.kernel.org; Chris Paterson <Chris.Paterson2@renesas.com>;
> Biju Das <biju.das@bp.renesas.com>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Subject: RE: [RFC 03/12] ravb: Fillup ravb_set_features_gbeth() stub
> 
> Hi Sergei,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [RFC 03/12] ravb: Fillup ravb_set_features_gbeth() stub
> >
> > On 10/5/21 2:06 PM, Biju Das wrote:
> >
> > > 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>
> > [...]
> >
> > > 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
> > [...]
> > > @@ -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,
> > >  				   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
> 
> >
> > > +		} else {
> > > +			ravb_write(ndev, 0, CSR1);
> >
> >    No need to mask off the 'features' field?
> >
> > > +		}
> > > +	}
> > > +	ravb_write(ndev, csr0, CSR0);
> > > +
> > > +	ndev->features = features;
> >
> >    Mhm, doesn't that clear NETIF_F_CSUM_MASK?
> >
> > [...]
> >
> > MBR, Sergey

[RFC 04/12] ravb: Fillup ravb_alloc_rx_desc_gbeth() stub

[Biju Das]

Fillup ravb_alloc_rx_desc_gbeth() function to support RZ/G2L.

This patch also renames ravb_alloc_rx_desc to ravb_alloc_rx_desc_rcar
to be consistent with the naming convention used in sh_eth driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC->v1:
 * started allocating 1 rx queue for "gbeth_rx_ring"
 * Moved gbeth_rx_ring near to rx_ring in priv structure
 * renamed ravb_alloc_rx_desc to ravb_alloc_rx_desc_rcar
---
 drivers/net/ethernet/renesas/ravb.h      |  1 +
 drivers/net/ethernet/renesas/ravb_main.c | 30 ++++++++++++++++++------
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 02f425bf9544..c63fad2d2049 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1076,6 +1076,7 @@ struct ravb_private {
 	struct ravb_desc *desc_bat;
 	dma_addr_t rx_desc_dma[NUM_RX_QUEUE];
 	dma_addr_t tx_desc_dma[NUM_TX_QUEUE];
+	struct ravb_rx_desc *gbeth_rx_ring;
 	struct ravb_ex_rx_desc *rx_ring[NUM_RX_QUEUE];
 	struct ravb_tx_desc *tx_ring[NUM_TX_QUEUE];
 	void *tx_align[NUM_TX_QUEUE];
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 37f50c041114..31de4e544525 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -385,11 +385,18 @@ static void ravb_ring_format(struct net_device *ndev, int q)
 
 static void *ravb_alloc_rx_desc_gbeth(struct net_device *ndev, int q)
 {
-	/* Place holder */
-	return NULL;
+	struct ravb_private *priv = netdev_priv(ndev);
+	unsigned int ring_size;
+
+	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
+
+	priv->gbeth_rx_ring = dma_alloc_coherent(ndev->dev.parent, ring_size,
+						 &priv->rx_desc_dma[q],
+						 GFP_KERNEL);
+	return priv->gbeth_rx_ring;
 }
 
-static void *ravb_alloc_rx_desc(struct net_device *ndev, int q)
+static void *ravb_alloc_rx_desc_rcar(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	unsigned int ring_size;
@@ -1085,16 +1092,25 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 	struct net_device *ndev = napi->dev;
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
+	bool gptp = info->gptp || info->ccc_gac;
+	struct ravb_rx_desc *desc;
 	unsigned long flags;
 	int q = napi - priv->napi;
 	int mask = BIT(q);
 	int quota = budget;
+	unsigned int entry;
 
+	if (!gptp) {
+		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
+		desc = &priv->gbeth_rx_ring[entry];
+	}
 	/* Processing RX Descriptor Ring */
 	/* Clear RX interrupt */
 	ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
-	if (ravb_rx(ndev, &quota, q))
-		goto out;
+	if (gptp || desc->die_dt != DT_FEMPTY) {
+		if (ravb_rx(ndev, &quota, q))
+			goto out;
+	}
 
 	/* Processing TX Descriptor Ring */
 	spin_lock_irqsave(&priv->lock, flags);
@@ -2206,7 +2222,7 @@ static int ravb_mdio_release(struct ravb_private *priv)
 static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.rx_ring_free = ravb_rx_ring_free,
 	.rx_ring_format = ravb_rx_ring_format,
-	.alloc_rx_desc = ravb_alloc_rx_desc,
+	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
 	.receive = ravb_rcar_rx,
 	.set_rate = ravb_set_rate_rcar,
 	.set_feature = ravb_set_features_rcar,
@@ -2231,7 +2247,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.rx_ring_free = ravb_rx_ring_free,
 	.rx_ring_format = ravb_rx_ring_format,
-	.alloc_rx_desc = ravb_alloc_rx_desc,
+	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
 	.receive = ravb_rcar_rx,
 	.set_rate = ravb_set_rate_rcar,
 	.set_feature = ravb_set_features_rcar,
-- 
2.17.1

Re: [RFC 04/12] ravb: Fillup ravb_alloc_rx_desc_gbeth() stub

[Sergey Shtylyov]

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

> Fillup ravb_alloc_rx_desc_gbeth() function to support RZ/G2L.
> 
> This patch also renames ravb_alloc_rx_desc to ravb_alloc_rx_desc_rcar
> to be consistent with the naming convention used in sh_eth driver.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
[...]

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 37f50c041114..31de4e544525 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> -static void *ravb_alloc_rx_desc(struct net_device *ndev, int q)
> +static void *ravb_alloc_rx_desc_rcar(struct net_device *ndev, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	unsigned int ring_size;
> @@ -1085,16 +1092,25 @@ static int ravb_poll(struct napi_struct *napi, int budget)
>  	struct net_device *ndev = napi->dev;
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *info = priv->info;
> +	bool gptp = info->gptp || info->ccc_gac;
> +	struct ravb_rx_desc *desc;
>  	unsigned long flags;
>  	int q = napi - priv->napi;
>  	int mask = BIT(q);
>  	int quota = budget;
> +	unsigned int entry;
>  
> +	if (!gptp) {
> +		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> +		desc = &priv->gbeth_rx_ring[entry];
> +	}
>  	/* Processing RX Descriptor Ring */
>  	/* Clear RX interrupt */
>  	ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
> -	if (ravb_rx(ndev, &quota, q))
> -		goto out;
> +	if (gptp || desc->die_dt != DT_FEMPTY) {
> +		if (ravb_rx(ndev, &quota, q))
> +			goto out;
> +	}

   Not sure I understand this new logic around the ravb_rx() call, care to explain?

[...]

MBR, Sergey

RE: [RFC 04/12] ravb: Fillup ravb_alloc_rx_desc_gbeth() stub

[Biju Das]

Hi Sergey,

> Subject: Re: [RFC 04/12] ravb: Fillup ravb_alloc_rx_desc_gbeth() stub
> 
> On 10/5/21 2:06 PM, Biju Das wrote:
> 
> > Fillup ravb_alloc_rx_desc_gbeth() function to support RZ/G2L.
> >
> > This patch also renames ravb_alloc_rx_desc to ravb_alloc_rx_desc_rcar
> > to be consistent with the naming convention used in sh_eth driver.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> [...]
> 
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 37f50c041114..31de4e544525 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
> > -static void *ravb_alloc_rx_desc(struct net_device *ndev, int q)
> > +static void *ravb_alloc_rx_desc_rcar(struct net_device *ndev, int q)
> >  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> >  	unsigned int ring_size;
> > @@ -1085,16 +1092,25 @@ static int ravb_poll(struct napi_struct *napi,
> int budget)
> >  	struct net_device *ndev = napi->dev;
> >  	struct ravb_private *priv = netdev_priv(ndev);
> >  	const struct ravb_hw_info *info = priv->info;
> > +	bool gptp = info->gptp || info->ccc_gac;
> > +	struct ravb_rx_desc *desc;
> >  	unsigned long flags;
> >  	int q = napi - priv->napi;
> >  	int mask = BIT(q);
> >  	int quota = budget;
> > +	unsigned int entry;
> >
> > +	if (!gptp) {
> > +		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> > +		desc = &priv->gbeth_rx_ring[entry];
> > +	}
> >  	/* Processing RX Descriptor Ring */
> >  	/* Clear RX interrupt */
> >  	ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
> > -	if (ravb_rx(ndev, &quota, q))
> > -		goto out;
> > +	if (gptp || desc->die_dt != DT_FEMPTY) {
> > +		if (ravb_rx(ndev, &quota, q))
> > +			goto out;
> > +	}
> 
>    Not sure I understand this new logic around the ravb_rx() call, care to
> explain?

The code is simple.

If (gptp ||  --> means non gptp case that is Gbethernet
die_dt --> Descriptor thype

So basically the new logic is , on Gbethernet case, if descriptor is not empty, then process rx.

Regards,
Biju

[RFC 05/12] ravb: Fillup ravb_rx_ring_free_gbeth() stub

[Biju Das]

Fillup ravb_rx_ring_free_gbeth() function to support RZ/G2L.

This patch also renames ravb_rx_ring_free to ravb_rx_ring_free_rcar
to be consistent with the naming convention used in sh_eth driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC Changes:
 * moved "gbeth_rx_ring" to previous patch
 * started using "gbeth_rx_ring" instead of gbeth_rx_ring[q].
 * renamed ravb_rx_ring_free to ravb_rx_ring_free_rcar
---
 drivers/net/ethernet/renesas/ravb_main.c | 28 ++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 31de4e544525..69a2cd871344 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -236,10 +236,30 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
 
 static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
 {
-	/* Place holder */
+	struct ravb_private *priv = netdev_priv(ndev);
+	unsigned int ring_size;
+	unsigned int i;
+
+	if (!priv->gbeth_rx_ring)
+		return;
+
+	for (i = 0; i < priv->num_rx_ring[q]; i++) {
+		struct ravb_rx_desc *desc = &priv->gbeth_rx_ring[i];
+
+		if (!dma_mapping_error(ndev->dev.parent,
+				       le32_to_cpu(desc->dptr)))
+			dma_unmap_single(ndev->dev.parent,
+					 le32_to_cpu(desc->dptr),
+					 GBETH_RX_BUFF_MAX,
+					 DMA_FROM_DEVICE);
+	}
+	ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
+	dma_free_coherent(ndev->dev.parent, ring_size, priv->gbeth_rx_ring,
+			  priv->rx_desc_dma[q]);
+	priv->gbeth_rx_ring = NULL;
 }
 
-static void ravb_rx_ring_free(struct net_device *ndev, int q)
+static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	unsigned int ring_size;
@@ -2220,7 +2240,7 @@ static int ravb_mdio_release(struct ravb_private *priv)
 }
 
 static const struct ravb_hw_info ravb_gen3_hw_info = {
-	.rx_ring_free = ravb_rx_ring_free,
+	.rx_ring_free = ravb_rx_ring_free_rcar,
 	.rx_ring_format = ravb_rx_ring_format,
 	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
 	.receive = ravb_rcar_rx,
@@ -2245,7 +2265,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 };
 
 static const struct ravb_hw_info ravb_gen2_hw_info = {
-	.rx_ring_free = ravb_rx_ring_free,
+	.rx_ring_free = ravb_rx_ring_free_rcar,
 	.rx_ring_format = ravb_rx_ring_format,
 	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
 	.receive = ravb_rcar_rx,
-- 
2.17.1

Re: [RFC 05/12] ravb: Fillup ravb_rx_ring_free_gbeth() stub

[Sergei Shtylyov]

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

> Fillup ravb_rx_ring_free_gbeth() function to support RZ/G2L.
> 
> This patch also renames ravb_rx_ring_free to ravb_rx_ring_free_rcar
> to be consistent with the naming convention used in sh_eth driver.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

[RFC 06/12] ravb: Fillup ravb_rx_ring_format_gbeth() stub

[Biju Das]

Fillup ravb_rx_ring_format_gbeth() function to support RZ/G2L.

This patch also renames ravb_rx_ring_format to ravb_rx_ring_format_rcar
to be consistent with the naming convention used in sh_eth driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC changes:
 * Started using gbeth_rx_ring instead of gbeth_rx_ring[q].
 * renamed ravb_rx_ring_format to ravb_rx_ring_format_rcar
---
 drivers/net/ethernet/renesas/ravb.h      |  1 +
 drivers/net/ethernet/renesas/ravb_main.c | 34 +++++++++++++++++++++---
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index c63fad2d2049..35710da808a6 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1008,6 +1008,7 @@ enum CSR2_BIT {
 #define RX_BUF_SZ	(2048 - ETH_FCS_LEN + sizeof(__sum16))
 
 #define GBETH_RX_BUFF_MAX 8192
+#define GBETH_RX_DESC_DATA_SIZE 4080
 
 struct ravb_tstamp_skb {
 	struct list_head list;
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 69a2cd871344..37164a983156 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -327,10 +327,36 @@ static void ravb_ring_free(struct net_device *ndev, int q)
 
 static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
 {
-	/* Place holder */
+	struct ravb_private *priv = netdev_priv(ndev);
+	struct ravb_rx_desc *rx_desc;
+	unsigned int rx_ring_size;
+	dma_addr_t dma_addr;
+	unsigned int i;
+
+	rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
+	memset(priv->gbeth_rx_ring, 0, rx_ring_size);
+	/* Build RX ring buffer */
+	for (i = 0; i < priv->num_rx_ring[q]; i++) {
+		/* RX descriptor */
+		rx_desc = &priv->gbeth_rx_ring[i];
+		rx_desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
+		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
+					  GBETH_RX_BUFF_MAX,
+					  DMA_FROM_DEVICE);
+		/* We just set the data size to 0 for a failed mapping which
+		 * should prevent DMA from happening...
+		 */
+		if (dma_mapping_error(ndev->dev.parent, dma_addr))
+			rx_desc->ds_cc = cpu_to_le16(0);
+		rx_desc->dptr = cpu_to_le32(dma_addr);
+		rx_desc->die_dt = DT_FEMPTY;
+	}
+	rx_desc = &priv->gbeth_rx_ring[i];
+	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
+	rx_desc->die_dt = DT_LINKFIX; /* type */
 }
 
-static void ravb_rx_ring_format(struct net_device *ndev, int q)
+static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	struct ravb_ex_rx_desc *rx_desc;
@@ -2241,7 +2267,7 @@ static int ravb_mdio_release(struct ravb_private *priv)
 
 static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.rx_ring_free = ravb_rx_ring_free_rcar,
-	.rx_ring_format = ravb_rx_ring_format,
+	.rx_ring_format = ravb_rx_ring_format_rcar,
 	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
 	.receive = ravb_rcar_rx,
 	.set_rate = ravb_set_rate_rcar,
@@ -2266,7 +2292,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 
 static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.rx_ring_free = ravb_rx_ring_free_rcar,
-	.rx_ring_format = ravb_rx_ring_format,
+	.rx_ring_format = ravb_rx_ring_format_rcar,
 	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
 	.receive = ravb_rcar_rx,
 	.set_rate = ravb_set_rate_rcar,
-- 
2.17.1

Re: [RFC 06/12] ravb: Fillup ravb_rx_ring_format_gbeth() stub

[Sergey Shtylyov]

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

> Fillup ravb_rx_ring_format_gbeth() function to support RZ/G2L.
> 
> This patch also renames ravb_rx_ring_format to ravb_rx_ring_format_rcar
> to be consistent with the naming convention used in sh_eth driver.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

[RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

[Biju Das]

Fillup ravb_rx_gbeth() function to support RZ/G2L.

This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
consistent with the naming convention used in sh_eth driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC changes:
 * renamed "rxtop_skb" to "rx_1st_skb" and moved near to rx_skb.
 * removed parameter q from "ravb_get_skb_gbeth"
 * renamed ravb_rcar_rx to ravb_rx_rcar
---
 drivers/net/ethernet/renesas/ravb.h      |   1 +
 drivers/net/ethernet/renesas/ravb_main.c | 167 ++++++++++++++++++++++-
 2 files changed, 163 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 35710da808a6..8c7b2569c7dd 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1081,6 +1081,7 @@ struct ravb_private {
 	struct ravb_ex_rx_desc *rx_ring[NUM_RX_QUEUE];
 	struct ravb_tx_desc *tx_ring[NUM_TX_QUEUE];
 	void *tx_align[NUM_TX_QUEUE];
+	struct sk_buff *rx_1st_skb;
 	struct sk_buff **rx_skb[NUM_RX_QUEUE];
 	struct sk_buff **tx_skb[NUM_TX_QUEUE];
 	u32 rx_over_errors;
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 37164a983156..42573eac82b9 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
 	}
 }
 
+static void ravb_rx_csum_gbeth(struct sk_buff *skb)
+{
+	u8 *hw_csum;
+
+	/* The hardware checksum is contained in sizeof(__sum16) (2) bytes
+	 * appended to packet data
+	 */
+	if (unlikely(skb->len < sizeof(__sum16)))
+		return;
+	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
+
+	if (*hw_csum == 0)
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
+	else
+		skb->ip_summed = CHECKSUM_NONE;
+}
+
 static void ravb_rx_csum(struct sk_buff *skb)
 {
 	u8 *hw_csum;
@@ -735,15 +752,155 @@ static void ravb_rx_csum(struct sk_buff *skb)
 	skb_trim(skb, skb->len - sizeof(__sum16));
 }
 
+static struct sk_buff *ravb_get_skb_gbeth(struct net_device *ndev, int entry,
+					  struct ravb_rx_desc *desc)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	struct sk_buff *skb;
+
+	skb = priv->rx_skb[RAVB_BE][entry];
+	priv->rx_skb[RAVB_BE][entry] = NULL;
+	dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
+			 ALIGN(GBETH_RX_BUFF_MAX, 16), DMA_FROM_DEVICE);
+
+	return skb;
+}
+
 /* Packet receive function for Gigabit Ethernet */
 static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 {
-	/* Place holder */
-	return true;
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_hw_info *info = priv->info;
+	struct net_device_stats *stats;
+	struct ravb_rx_desc *desc;
+	struct sk_buff *skb;
+	dma_addr_t dma_addr;
+	u8  desc_status;
+	int boguscnt;
+	u16 pkt_len;
+	u8  die_dt;
+	int entry;
+	int limit;
+
+	entry = priv->cur_rx[q] % priv->num_rx_ring[q];
+	boguscnt = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
+	stats = &priv->stats[q];
+
+	boguscnt = min(boguscnt, *quota);
+	limit = boguscnt;
+	desc = &priv->gbeth_rx_ring[entry];
+	while (desc->die_dt != DT_FEMPTY) {
+		/* Descriptor type must be checked before all other reads */
+		dma_rmb();
+		desc_status = desc->msc;
+		pkt_len = le16_to_cpu(desc->ds_cc) & RX_DS;
+
+		if (--boguscnt < 0)
+			break;
+
+		/* We use 0-byte descriptors to mark the DMA mapping errors */
+		if (!pkt_len)
+			continue;
+
+		if (desc_status & MSC_MC)
+			stats->multicast++;
+
+		if (desc_status & (MSC_CRC | MSC_RFE | MSC_RTSF | MSC_RTLF | MSC_CEEF)) {
+			stats->rx_errors++;
+			if (desc_status & MSC_CRC)
+				stats->rx_crc_errors++;
+			if (desc_status & MSC_RFE)
+				stats->rx_frame_errors++;
+			if (desc_status & (MSC_RTLF | MSC_RTSF))
+				stats->rx_length_errors++;
+			if (desc_status & MSC_CEEF)
+				stats->rx_missed_errors++;
+		} else {
+			die_dt = desc->die_dt & 0xF0;
+			switch (die_dt) {
+			case DT_FSINGLE:
+				skb = ravb_get_skb_gbeth(ndev, entry, desc);
+				skb_put(skb, pkt_len);
+				skb->protocol = eth_type_trans(skb, ndev);
+				if (ndev->features & NETIF_F_RXCSUM)
+					ravb_rx_csum_gbeth(skb);
+				napi_gro_receive(&priv->napi[q], skb);
+				stats->rx_packets++;
+				stats->rx_bytes += pkt_len;
+				break;
+			case DT_FSTART:
+				priv->rx_1st_skb = ravb_get_skb_gbeth(ndev, entry, desc);
+				skb_put(priv->rx_1st_skb, pkt_len);
+				break;
+			case DT_FMID:
+				skb = ravb_get_skb_gbeth(ndev, entry, desc);
+				skb_copy_to_linear_data_offset(priv->rx_1st_skb,
+							       priv->rx_1st_skb->len,
+							       skb->data,
+							       pkt_len);
+				skb_put(priv->rx_1st_skb, pkt_len);
+				dev_kfree_skb(skb);
+				break;
+			case DT_FEND:
+				skb = ravb_get_skb_gbeth(ndev, entry, desc);
+				skb_copy_to_linear_data_offset(priv->rx_1st_skb,
+							       priv->rx_1st_skb->len,
+							       skb->data,
+							       pkt_len);
+				skb_put(priv->rx_1st_skb, pkt_len);
+				dev_kfree_skb(skb);
+				priv->rx_1st_skb->protocol =
+					eth_type_trans(priv->rx_1st_skb, ndev);
+				if (ndev->features & NETIF_F_RXCSUM)
+					ravb_rx_csum_gbeth(skb);
+				napi_gro_receive(&priv->napi[q],
+						 priv->rx_1st_skb);
+				stats->rx_packets++;
+				stats->rx_bytes += priv->rx_1st_skb->len;
+				break;
+			}
+		}
+
+		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
+		desc = &priv->gbeth_rx_ring[entry];
+	}
+
+	/* Refill the RX ring buffers. */
+	for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
+		entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
+		desc = &priv->gbeth_rx_ring[entry];
+		desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
+
+		if (!priv->rx_skb[q][entry]) {
+			skb = netdev_alloc_skb(ndev, info->max_rx_len);
+			if (!skb)
+				break;
+			ravb_set_buffer_align(skb);
+			dma_addr = dma_map_single(ndev->dev.parent,
+						  skb->data,
+						  GBETH_RX_BUFF_MAX,
+						  DMA_FROM_DEVICE);
+			skb_checksum_none_assert(skb);
+			/* We just set the data size to 0 for a failed mapping
+			 * which should prevent DMA  from happening...
+			 */
+			if (dma_mapping_error(ndev->dev.parent, dma_addr))
+				desc->ds_cc = cpu_to_le16(0);
+			desc->dptr = cpu_to_le32(dma_addr);
+			priv->rx_skb[q][entry] = skb;
+		}
+		/* Descriptor type must be set after all the above writes */
+		dma_wmb();
+		desc->die_dt = DT_FEMPTY;
+	}
+
+	*quota -= limit - (++boguscnt);
+
+	return boguscnt <= 0;
 }
 
 /* Packet receive function for Ethernet AVB */
-static bool ravb_rcar_rx(struct net_device *ndev, int *quota, int q)
+static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
@@ -2269,7 +2426,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.rx_ring_free = ravb_rx_ring_free_rcar,
 	.rx_ring_format = ravb_rx_ring_format_rcar,
 	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
-	.receive = ravb_rcar_rx,
+	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
 	.set_feature = ravb_set_features_rcar,
 	.dmac_init = ravb_dmac_init_rcar,
@@ -2294,7 +2451,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.rx_ring_free = ravb_rx_ring_free_rcar,
 	.rx_ring_format = ravb_rx_ring_format_rcar,
 	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
-	.receive = ravb_rcar_rx,
+	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
 	.set_feature = ravb_set_features_rcar,
 	.dmac_init = ravb_dmac_init_rcar,
-- 
2.17.1

Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

[Sergey Shtylyov]

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

> Fillup ravb_rx_gbeth() function to support RZ/G2L.
> 
> This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
> consistent with the naming convention used in sh_eth driver.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 37164a983156..42573eac82b9 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
>  	}
>  }
>  
> +static void ravb_rx_csum_gbeth(struct sk_buff *skb)
> +{
> +	u8 *hw_csum;
> +
> +	/* The hardware checksum is contained in sizeof(__sum16) (2) bytes
> +	 * appended to packet data
> +	 */
> +	if (unlikely(skb->len < sizeof(__sum16)))
> +		return;
> +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);

   Not 32-bit? The manual says the IP checksum is stored in the first 2 bytes.

> +
> +	if (*hw_csum == 0)

   You only check the 1st byte, not the full checksum!

> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
> +	else
> +		skb->ip_summed = CHECKSUM_NONE;

  So the TCP/UDP/ICMP checksums are not dealt with? Why enable them then?

> +}
> +
>  static void ravb_rx_csum(struct sk_buff *skb)

static void ravb_rx_csum_rcar(struct sk_buff *skb)?

[...]

MBR, Sergey

RE: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

[Biju Das]

Hi Sergey,

Thanks for thefeedback.

> Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
> 
> On 10/5/21 2:06 PM, Biju Das wrote:
> 
> > Fillup ravb_rx_gbeth() function to support RZ/G2L.
> >
> > This patch also renames ravb_rcar_rx to ravb_rx_rcar to be consistent
> > with the naming convention used in sh_eth driver.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar
> > <prabhakar.mahadev-lad.rj@bp.renesas.com>[...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 37164a983156..42573eac82b9 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct net_device
> *ndev)
> >  	}
> >  }
> >
> > +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
> > +	u8 *hw_csum;
> > +
> > +	/* The hardware checksum is contained in sizeof(__sum16) (2) bytes
> > +	 * appended to packet data
> > +	 */
> > +	if (unlikely(skb->len < sizeof(__sum16)))
> > +		return;
> > +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
> 
>    Not 32-bit? The manual says the IP checksum is stored in the first 2
> bytes.

It is 16 bit. It is on last 2 bytes.

> 
> > +
> > +	if (*hw_csum == 0)
> 
>    You only check the 1st byte, not the full checksum!

As I said earlier, "0" value on last 16 bit, means no checksum error.

> 
> > +		skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +	else
> > +		skb->ip_summed = CHECKSUM_NONE;
> 
>   So the TCP/UDP/ICMP checksums are not dealt with? Why enable them then?

If last 2bytes is zero, means there is no checksum error w.r.to TCP/UDP/ICMP checksums.

RZ/G2L checksum part is different from R-Car Gen3. There is no TOE block at all for R-Car Gen3.

Regards,
Biju

> 
> > +}
> > +
> >  static void ravb_rx_csum(struct sk_buff *skb)
> 
> static void ravb_rx_csum_rcar(struct sk_buff *skb)?
> 
> [...]
> 
> MBR, Sergey

Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

[Sergey Shtylyov]

On 10/6/21 11:22 PM, Biju Das wrote:

[...]
>>> Fillup ravb_rx_gbeth() function to support RZ/G2L.
>>>
>>> This patch also renames ravb_rcar_rx to ravb_rx_rcar to be consistent
>>> with the naming convention used in sh_eth driver.
>>>
>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>> Reviewed-by: Lad Prabhakar
>>> <prabhakar.mahadev-lad.rj@bp.renesas.com>[...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 37164a983156..42573eac82b9 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct net_device
>> *ndev)
>>>  	}
>>>  }
>>>
>>> +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
>>> +	u8 *hw_csum;
>>> +
>>> +	/* The hardware checksum is contained in sizeof(__sum16) (2) bytes
>>> +	 * appended to packet data
>>> +	 */
>>> +	if (unlikely(skb->len < sizeof(__sum16)))
>>> +		return;
>>> +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
>>
>>    Not 32-bit? The manual says the IP checksum is stored in the first 2
>> bytes.
> 
> It is 16 bit. It is on last 2 bytes.

   So you're saying the manual is wrong?

>>
>>> +
>>> +	if (*hw_csum == 0)
>>
>>    You only check the 1st byte, not the full checksum!
> 
> As I said earlier, "0" value on last 16 bit, means no checksum error.

   How's that? 'hw_csum' is declaread as 'u8 *'!

>>> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
>>> +	else
>>> +		skb->ip_summed = CHECKSUM_NONE;
>>
>>   So the TCP/UDP/ICMP checksums are not dealt with? Why enable them then?
> 
> If last 2bytes is zero, means there is no checksum error w.r.to TCP/UDP/ICMP checksums.

   Why checksum them independently then?

> RZ/G2L checksum part is different from R-Car Gen3. There is no TOE block at all for R-Car Gen3.
> 
> Regards,
> Biju

[...]

MBR, Sergey

RE: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

[Biju Das]

Hi Sergey,

> Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
> 
> On 10/6/21 11:22 PM, Biju Das wrote:
> 
> [...]
> >>> Fillup ravb_rx_gbeth() function to support RZ/G2L.
> >>>
> >>> This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
> >>> consistent with the naming convention used in sh_eth driver.
> >>>
> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>> Reviewed-by: Lad Prabhakar
> >>> <prabhakar.mahadev-lad.rj@bp.renesas.com>[...]
> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >>> b/drivers/net/ethernet/renesas/ravb_main.c
> >>> index 37164a983156..42573eac82b9 100644
> >>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>> @@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct
> >>> net_device
> >> *ndev)
> >>>  	}
> >>>  }
> >>>
> >>> +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
> >>> +	u8 *hw_csum;
> >>> +
> >>> +	/* The hardware checksum is contained in sizeof(__sum16) (2) bytes
> >>> +	 * appended to packet data
> >>> +	 */
> >>> +	if (unlikely(skb->len < sizeof(__sum16)))
> >>> +		return;
> >>> +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
> >>
> >>    Not 32-bit? The manual says the IP checksum is stored in the first
> >> 2 bytes.
> >
> > It is 16 bit. It is on last 2 bytes.
> 
>    So you're saying the manual is wrong?

I am not sure which manual you are referring here.

I am referring to Rev.1.00 Sep, 2021 of RZ/G2L hardware manual and
I have shared the link[1] for you to download. Hope you are referring same manual


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

Please check the section 30.5.6.1 checksum calculation handling
And figure 30.25 the field of checksum attaching field

Also see Table 30.17 for checksum values for non-error conditions.

TCP/UDP/ICPM checksum is at last 2bytes.

> 
> >>
> >>> +
> >>> +	if (*hw_csum == 0)
> >>
> >>    You only check the 1st byte, not the full checksum!
> >
> > As I said earlier, "0" value on last 16 bit, means no checksum error.
> 
>    How's that? 'hw_csum' is declared as 'u8 *'!

It is my mistake, which will be taken care in the next patch by using u16 *.

> 
> >>> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
> >>> +	else
> >>> +		skb->ip_summed = CHECKSUM_NONE;
> >>
> >>   So the TCP/UDP/ICMP checksums are not dealt with? Why enable them
> then?
> >
> > If last 2bytes is zero, means there is no checksum error w.r.to
> TCP/UDP/ICMP checksums.
> 
>    Why checksum them independently then?

It is a hardware feature. 

Regards,
Biju

> 
> > RZ/G2L checksum part is different from R-Car Gen3. There is no TOE block
> at all for R-Car Gen3.
> >
> > Regards,
> > Biju
> 
> [...]
> 
> MBR, Sergey

Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

[Sergey Shtylyov]

On 10/7/21 8:49 AM, Biju Das wrote:

[...]
>>>>> Fillup ravb_rx_gbeth() function to support RZ/G2L.
>>>>>
>>>>> This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
>>>>> consistent with the naming convention used in sh_eth driver.
>>>>>
>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>>>> Reviewed-by: Lad Prabhakar
>>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com>[...]
>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>>>> index 37164a983156..42573eac82b9 100644
>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>>> @@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct
>>>>> net_device
>>>> *ndev)
>>>>>  	}
>>>>>  }
>>>>>
>>>>> +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
>>>>> +	u8 *hw_csum;
>>>>> +
>>>>> +	/* The hardware checksum is contained in sizeof(__sum16) (2) bytes
>>>>> +	 * appended to packet data
>>>>> +	 */
>>>>> +	if (unlikely(skb->len < sizeof(__sum16)))
>>>>> +		return;
>>>>> +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
>>>>
>>>>    Not 32-bit? The manual says the IP checksum is stored in the first
>>>> 2 bytes.
>>>
>>> It is 16 bit. It is on last 2 bytes.

   The IP checksum is at the 1st 2 bytes of the overall 4-byte checksum (coming after
the packet payload), no?

>>    So you're saying the manual is wrong?
> 
> I am not sure which manual you are referring here.
> 
> I am referring to Rev.1.00 Sep, 2021 of RZ/G2L hardware manual and

   Same here.

[...]

> Please check the section 30.5.6.1 checksum calculation handling> And figure 30.25 the field of checksum attaching field

   I have.

> Also see Table 30.17 for checksum values for non-error conditions.

> TCP/UDP/ICPM checksum is at last 2bytes.

   What are you arguing with then? :-)
   My point was that your code fetched the TCP/UDP/ICMP checksum ISO the IP checksum
because it subtracts sizeof(__sum16), while should probably subtract sizeof(__wsum).

>>>>> +
>>>>> +	if (*hw_csum == 0)
>>>>
>>>>    You only check the 1st byte, not the full checksum!
>>>
>>> As I said earlier, "0" value on last 16 bit, means no checksum error.
>>
>>    How's that? 'hw_csum' is declared as 'u8 *'!
> 
> It is my mistake, which will be taken care in the next patch by using u16 *.

   Note that this 'u16' halfword can be unaligned, that's why the current code uses get_unaligned_le16().

>>>>> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
>>>>> +	else
>>>>> +		skb->ip_summed = CHECKSUM_NONE;
>>>>
>>>>   So the TCP/UDP/ICMP checksums are not dealt with? Why enable them
>> then?
>>>
>>> If last 2bytes is zero, means there is no checksum error w.r.to
>> TCP/UDP/ICMP checksums.
>>
>>    Why checksum them independently then?
> 
> It is a hardware feature. 

   Switchable, isn't it?

> Regards,
> Biju

[...]

MBR, Sergey

RE: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

[Biju Das]

Hi Sergey,


Thanks for the feedback.

> Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
> 
> On 10/7/21 8:49 AM, Biju Das wrote:
> 
> [...]
> >>>>> Fillup ravb_rx_gbeth() function to support RZ/G2L.
> >>>>>
> >>>>> This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
> >>>>> consistent with the naming convention used in sh_eth driver.
> >>>>>
> >>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>>>> Reviewed-by: Lad Prabhakar
> >>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com>[...]
> >>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >>>>> b/drivers/net/ethernet/renesas/ravb_main.c
> >>>>> index 37164a983156..42573eac82b9 100644
> >>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>>>> @@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct
> >>>>> net_device
> >>>> *ndev)
> >>>>>  	}
> >>>>>  }
> >>>>>
> >>>>> +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
> >>>>> +	u8 *hw_csum;
> >>>>> +
> >>>>> +	/* The hardware checksum is contained in sizeof(__sum16) (2)
> bytes
> >>>>> +	 * appended to packet data
> >>>>> +	 */
> >>>>> +	if (unlikely(skb->len < sizeof(__sum16)))
> >>>>> +		return;
> >>>>> +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
> >>>>
> >>>>    Not 32-bit? The manual says the IP checksum is stored in the
> >>>> first
> >>>> 2 bytes.
> >>>
> >>> It is 16 bit. It is on last 2 bytes.
> 
>    The IP checksum is at the 1st 2 bytes of the overall 4-byte checksum
> (coming after the packet payload), no?

Sorry, I got confused with your question earlier. Now it is clear for me.

I agree the checksum part is stored in last 4bytes. Of this, the first 2 bytes IPV4 checksum
and last 2 bytes TCP/UDP/ICMP checksum.

> 
> >>    So you're saying the manual is wrong?
> >
> > I am not sure which manual you are referring here.
> >
> > I am referring to Rev.1.00 Sep, 2021 of RZ/G2L hardware manual and
> 
>    Same here.
> 
> [...]
> 
> > Please check the section 30.5.6.1 checksum calculation handling> And
> > figure 30.25 the field of checksum attaching field
> 
>    I have.
> 
> > Also see Table 30.17 for checksum values for non-error conditions.
> 
> > TCP/UDP/ICPM checksum is at last 2bytes.
> 
>    What are you arguing with then? :-)
>    My point was that your code fetched the TCP/UDP/ICMP checksum ISO the
> IP checksum because it subtracts sizeof(__sum16), while should probably
> subtract sizeof(__wsum)

Agreed. My code missed IP4 checksum result. May be we need to extract 2 checksum info
from last 4 bytes.  First checksum(2bytes) is IP4 header checksum and next checksum(2 bytes)  for TCP/UDP/ICMP and use this info finding the non error case mentioned in  Table 30.17.

For eg:-
IPV6 non error-condition -->  "0xFFFF"-->IPV4HeaderCSum value and "0x0000" TCP/UDP/ICMP CSUM value

IPV4 non error-condition -->  "0x0000"-->IPV4HeaderCSum value and "0x0000" TCP/UDP/ICMP CSUM value

Do you agree?

Regards,
Biju
> 
> >>>>> +
> >>>>> +	if (*hw_csum == 0)
> >>>>
> >>>>    You only check the 1st byte, not the full checksum!
> >>>
> >>> As I said earlier, "0" value on last 16 bit, means no checksum error.
> >>
> >>    How's that? 'hw_csum' is declared as 'u8 *'!
> >
> > It is my mistake, which will be taken care in the next patch by using
> u16 *.
> 
>    Note that this 'u16' halfword can be unaligned, that's why the current
> code uses get_unaligned_le16().
> 
> >>>>> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
> >>>>> +	else
> >>>>> +		skb->ip_summed = CHECKSUM_NONE;
> >>>>
> >>>>   So the TCP/UDP/ICMP checksums are not dealt with? Why enable them
> >> then?
> >>>
> >>> If last 2bytes is zero, means there is no checksum error w.r.to
> >> TCP/UDP/ICMP checksums.
> >>
> >>    Why checksum them independently then?
> >
> > It is a hardware feature.
> 
>    Switchable, isn't it?

> 
> > Regards,
> > Biju
> 
> [...]
> 
> MBR, Sergey

RE: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

[Biju Das]

Hi Sergey,

> Subject: RE: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
> 
> Hi Sergey,
> 
> 
> Thanks for the feedback.
> 
> > Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
> >
> > On 10/7/21 8:49 AM, Biju Das wrote:
> >
> > [...]
> > >>>>> Fillup ravb_rx_gbeth() function to support RZ/G2L.
> > >>>>>
> > >>>>> This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
> > >>>>> consistent with the naming convention used in sh_eth driver.
> > >>>>>
> > >>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > >>>>> Reviewed-by: Lad Prabhakar
> > >>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com>[...]
> > >>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > >>>>> b/drivers/net/ethernet/renesas/ravb_main.c
> > >>>>> index 37164a983156..42573eac82b9 100644
> > >>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> > >>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > >>>>> @@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct
> > >>>>> net_device
> > >>>> *ndev)
> > >>>>>  	}
> > >>>>>  }
> > >>>>>
> > >>>>> +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
> > >>>>> +	u8 *hw_csum;
> > >>>>> +
> > >>>>> +	/* The hardware checksum is contained in sizeof(__sum16) (2)
> > bytes
> > >>>>> +	 * appended to packet data
> > >>>>> +	 */
> > >>>>> +	if (unlikely(skb->len < sizeof(__sum16)))
> > >>>>> +		return;
> > >>>>> +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
> > >>>>
> > >>>>    Not 32-bit? The manual says the IP checksum is stored in the
> > >>>> first
> > >>>> 2 bytes.
> > >>>
> > >>> It is 16 bit. It is on last 2 bytes.
> >
> >    The IP checksum is at the 1st 2 bytes of the overall 4-byte
> > checksum (coming after the packet payload), no?
> 
> Sorry, I got confused with your question earlier. Now it is clear for me.
> 
> I agree the checksum part is stored in last 4bytes. Of this, the first 2
> bytes IPV4 checksum and last 2 bytes TCP/UDP/ICMP checksum.
> 
> >
> > >>    So you're saying the manual is wrong?
> > >
> > > I am not sure which manual you are referring here.
> > >
> > > I am referring to Rev.1.00 Sep, 2021 of RZ/G2L hardware manual and
> >
> >    Same here.
> >
> > [...]
> >
> > > Please check the section 30.5.6.1 checksum calculation handling> And
> > > figure 30.25 the field of checksum attaching field
> >
> >    I have.
> >
> > > Also see Table 30.17 for checksum values for non-error conditions.
> >
> > > TCP/UDP/ICPM checksum is at last 2bytes.
> >
> >    What are you arguing with then? :-)
> >    My point was that your code fetched the TCP/UDP/ICMP checksum ISO
> > the IP checksum because it subtracts sizeof(__sum16), while should
> > probably subtract sizeof(__wsum)
> 
> Agreed. My code missed IP4 checksum result. May be we need to extract 2
> checksum info from last 4 bytes.  First checksum(2bytes) is IP4 header
> checksum and next checksum(2 bytes)  for TCP/UDP/ICMP and use this info
> finding the non error case mentioned in  Table 30.17.
> 
> For eg:-
> IPV6 non error-condition -->  "0xFFFF"-->IPV4HeaderCSum value and "0x0000"
> TCP/UDP/ICMP CSUM value
> 
> IPV4 non error-condition -->  "0x0000"-->IPV4HeaderCSum value and "0x0000"
> TCP/UDP/ICMP CSUM value
> 
> Do you agree?

What I meant here is some thing like below, please let me know if you have any issues with
this, otherwise I would like to send the patch with below changes.

Further improvements can happen later.

Please let me know.

+/* Hardware checksum status */
+#define IPV4_RX_CSUM_OK                0x00000000
+#define IPV6_RX_CSUM_OK                0xFFFF0000
+
 enum ravb_reg {
        /* AVB-DMAC registers */
        CCC     = 0x0000,
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index bbb42e5328e4..d9201fbbd472 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -722,16 +722,18 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
 
 static void ravb_rx_csum_gbeth(struct sk_buff *skb)
 {
-       u16 *hw_csum;
+       u32 csum_result;
+       u8 *hw_csum;
 
        /* The hardware checksum is contained in sizeof(__sum16) (2) bytes
         * appended to packet data
         */
-       if (unlikely(skb->len < sizeof(__sum16)))
+       if (unlikely(skb->len < sizeof(__wsum)))
                return;
-       hw_csum = (u16*)(skb_tail_pointer(skb) - sizeof(__sum16));
+       hw_csum = skb_tail_pointer(skb) - sizeof(__wsum);
+       csum_result = get_unaligned_le32(hw_csum);
 
-       if (*hw_csum == 0)
+       if (csum_result == IPV4_RX_CSUM_OK || csum_result == IPV6_RX_CSUM_OK)

Regards,
Biju

> 
> Regards,
> Biju
> >
> > >>>>> +
> > >>>>> +	if (*hw_csum == 0)
> > >>>>
> > >>>>    You only check the 1st byte, not the full checksum!
> > >>>
> > >>> As I said earlier, "0" value on last 16 bit, means no checksum
> error.
> > >>
> > >>    How's that? 'hw_csum' is declared as 'u8 *'!
> > >
> > > It is my mistake, which will be taken care in the next patch by
> > > using
> > u16 *.
> >
> >    Note that this 'u16' halfword can be unaligned, that's why the
> > current code uses get_unaligned_le16().
> >
> > >>>>> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
> > >>>>> +	else
> > >>>>> +		skb->ip_summed = CHECKSUM_NONE;
> > >>>>
> > >>>>   So the TCP/UDP/ICMP checksums are not dealt with? Why enable
> > >>>> them
> > >> then?
> > >>>
> > >>> If last 2bytes is zero, means there is no checksum error w.r.to
> > >> TCP/UDP/ICMP checksums.
> > >>
> > >>    Why checksum them independently then?
> > >
> > > It is a hardware feature.
> >
> >    Switchable, isn't it?
> 
> >
> > > Regards,
> > > Biju
> >
> > [...]
> >
> > MBR, Sergey

Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

[Sergey Shtylyov]

On 10/8/21 9:46 AM, Biju Das wrote:

[...]
>>>>>>>> Fillup ravb_rx_gbeth() function to support RZ/G2L.
>>>>>>>>
>>>>>>>> This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
>>>>>>>> consistent with the naming convention used in sh_eth driver.
>>>>>>>>
>>>>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>>>>>>> Reviewed-by: Lad Prabhakar
>>>>>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com>[...]
>>>>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>> index 37164a983156..42573eac82b9 100644
>>>>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>> @@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct
>>>>>>>> net_device
>>>>>>> *ndev)
>>>>>>>>  	}
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
>>>>>>>> +	u8 *hw_csum;
>>>>>>>> +
>>>>>>>> +	/* The hardware checksum is contained in sizeof(__sum16) (2)
>>> bytes
>>>>>>>> +	 * appended to packet data
>>>>>>>> +	 */
>>>>>>>> +	if (unlikely(skb->len < sizeof(__sum16)))
>>>>>>>> +		return;
>>>>>>>> +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
[...]
>>>> Please check the section 30.5.6.1 checksum calculation handling> And
>>>> figure 30.25 the field of checksum attaching field
>>>
>>>    I have.
>>>
>>>> Also see Table 30.17 for checksum values for non-error conditions.
>>>
>>>> TCP/UDP/ICPM checksum is at last 2bytes.
>>>
>>>    What are you arguing with then? :-)
>>>    My point was that your code fetched the TCP/UDP/ICMP checksum ISO
>>> the IP checksum because it subtracts sizeof(__sum16), while should
>>> probably subtract sizeof(__wsum)
>>
>> Agreed. My code missed IP4 checksum result. May be we need to extract 2
>> checksum info from last 4 bytes.  First checksum(2bytes) is IP4 header
>> checksum and next checksum(2 bytes)  for TCP/UDP/ICMP and use this info
>> finding the non error case mentioned in  Table 30.17.
>>
>> For eg:-
>> IPV6 non error-condition -->  "0xFFFF"-->IPV4HeaderCSum value and "0x0000"
>> TCP/UDP/ICMP CSUM value
>>
>> IPV4 non error-condition -->  "0x0000"-->IPV4HeaderCSum value and "0x0000"
>> TCP/UDP/ICMP CSUM value
>>
>> Do you agree?

> What I meant here is some thing like below, please let me know if you have any issues with
> this, otherwise I would like to send the patch with below changes.
> 
> Further improvements can happen later.
> 
> Please let me know.
> 
> +/* Hardware checksum status */
> +#define IPV4_RX_CSUM_OK                0x00000000
> +#define IPV6_RX_CSUM_OK                0xFFFF0000

   Mhm, this should prolly come from the IP headers...

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index bbb42e5328e4..d9201fbbd472 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -722,16 +722,18 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
>  
>  static void ravb_rx_csum_gbeth(struct sk_buff *skb)
>  {
> -       u16 *hw_csum;
> +       u32 csum_result;

   This is not against the patch currently under investigation. :-)

> +       u8 *hw_csum;
>  
>         /* The hardware checksum is contained in sizeof(__sum16) (2) bytes
>          * appended to packet data
>          */
> -       if (unlikely(skb->len < sizeof(__sum16)))
> +       if (unlikely(skb->len < sizeof(__wsum)))

   I think this usage of __wsum is valid (I remember that I suggested it). We have 2 16-bit checksums here
covered by that, not a 32-bit sum...

>                 return;
> -       hw_csum = (u16*)(skb_tail_pointer(skb) - sizeof(__sum16));
> +       hw_csum = skb_tail_pointer(skb) - sizeof(__wsum);
> +       csum_result = get_unaligned_le32(hw_csum);
>  
> -       if (*hw_csum == 0)
> +       if (csum_result == IPV4_RX_CSUM_OK || csum_result == IPV6_RX_CSUM_OK)

   I don't think there's a hard-and-fast way to differentiate the valid packet just from
the 2 16-bit checksums...

[...]
>>>>>>>> +
>>>>>>>> +	if (*hw_csum == 0)
>>>>>>>
>>>>>>>    You only check the 1st byte, not the full checksum!
>>>>>>
>>>>>> As I said earlier, "0" value on last 16 bit, means no checksum
>> error.
>>>>>
>>>>>    How's that? 'hw_csum' is declared as 'u8 *'!
>>>>
>>>> It is my mistake, which will be taken care in the next patch by
>>>> using
>>> u16 *.

   That won't do it, I'm afraid...

   From an IRC discuassion on IRC we concluded that we don't need to check the checksum's
value, we just need to store it for the upper layers to catch the invalid sums...

[...]

MBR, Sergey

Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

[Sergey Shtylyov]

On 10/8/21 8:59 PM, Sergey Shtylyov wrote:
> On 10/8/21 9:46 AM, Biju Das wrote:
> 
> [...]
>>>>>>>>> Fillup ravb_rx_gbeth() function to support RZ/G2L.
>>>>>>>>>
>>>>>>>>> This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
>>>>>>>>> consistent with the naming convention used in sh_eth driver.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>>>>>>>> Reviewed-by: Lad Prabhakar
>>>>>>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com>[...]
>>>>>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>>> index 37164a983156..42573eac82b9 100644
>>>>>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>>> @@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct
>>>>>>>>> net_device
>>>>>>>> *ndev)
>>>>>>>>>  	}
>>>>>>>>>  }
>>>>>>>>>
>>>>>>>>> +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
>>>>>>>>> +	u8 *hw_csum;
>>>>>>>>> +
>>>>>>>>> +	/* The hardware checksum is contained in sizeof(__sum16) (2)
>>>> bytes
>>>>>>>>> +	 * appended to packet data
>>>>>>>>> +	 */
>>>>>>>>> +	if (unlikely(skb->len < sizeof(__sum16)))
>>>>>>>>> +		return;
>>>>>>>>> +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
> [...]
>>>>> Please check the section 30.5.6.1 checksum calculation handling> And
>>>>> figure 30.25 the field of checksum attaching field
>>>>
>>>>    I have.
>>>>
>>>>> Also see Table 30.17 for checksum values for non-error conditions.
>>>>
>>>>> TCP/UDP/ICPM checksum is at last 2bytes.
>>>>
>>>>    What are you arguing with then? :-)
>>>>    My point was that your code fetched the TCP/UDP/ICMP checksum ISO
>>>> the IP checksum because it subtracts sizeof(__sum16), while should
>>>> probably subtract sizeof(__wsum)
>>>
>>> Agreed. My code missed IP4 checksum result. May be we need to extract 2
>>> checksum info from last 4 bytes.  First checksum(2bytes) is IP4 header
>>> checksum and next checksum(2 bytes)  for TCP/UDP/ICMP and use this info
>>> finding the non error case mentioned in  Table 30.17.
>>>
>>> For eg:-
>>> IPV6 non error-condition -->  "0xFFFF"-->IPV4HeaderCSum value and "0x0000"
>>> TCP/UDP/ICMP CSUM value
>>>
>>> IPV4 non error-condition -->  "0x0000"-->IPV4HeaderCSum value and "0x0000"
>>> TCP/UDP/ICMP CSUM value
>>>
>>> Do you agree?
> 
>> What I meant here is some thing like below, please let me know if you have any issues with
>> this, otherwise I would like to send the patch with below changes.
>>
>> Further improvements can happen later.
>>
>> Please let me know.
>>
>> +/* Hardware checksum status */
>> +#define IPV4_RX_CSUM_OK                0x00000000
>> +#define IPV6_RX_CSUM_OK                0xFFFF0000
> 
>    Mhm, this should prolly come from the IP headers...
> 
> [...]
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index bbb42e5328e4..d9201fbbd472 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -722,16 +722,18 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
>>  
>>  static void ravb_rx_csum_gbeth(struct sk_buff *skb)
>>  {
>> -       u16 *hw_csum;
>> +       u32 csum_result;
> 
>    This is not against the patch currently under investigation. :-)
> 
>> +       u8 *hw_csum;
>>  
>>         /* The hardware checksum is contained in sizeof(__sum16) (2) bytes
>>          * appended to packet data
>>          */
>> -       if (unlikely(skb->len < sizeof(__sum16)))
>> +       if (unlikely(skb->len < sizeof(__wsum)))
> 
>    I think this usage of __wsum is valid (I remember that I suggested it). We have 2 16-bit checksums here

   I meant "I don't think", of course. :-)

[...]

MBR, Sergey

RE: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

[Biju Das]

Hi Sergey,

> Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
> 
> On 10/8/21 8:59 PM, Sergey Shtylyov wrote:
> > On 10/8/21 9:46 AM, Biju Das wrote:
> >
> > [...]
> >>>>>>>>> Fillup ravb_rx_gbeth() function to support RZ/G2L.
> >>>>>>>>>
> >>>>>>>>> This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
> >>>>>>>>> consistent with the naming convention used in sh_eth driver.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>>>>>>>> Reviewed-by: Lad Prabhakar
> >>>>>>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com>[...]
> >>>>>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >>>>>>>>> b/drivers/net/ethernet/renesas/ravb_main.c
> >>>>>>>>> index 37164a983156..42573eac82b9 100644
> >>>>>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>>>>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>>>>>>>> @@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct
> >>>>>>>>> net_device
> >>>>>>>> *ndev)
> >>>>>>>>>  	}
> >>>>>>>>>  }
> >>>>>>>>>
> >>>>>>>>> +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
> >>>>>>>>> +	u8 *hw_csum;
> >>>>>>>>> +
> >>>>>>>>> +	/* The hardware checksum is contained in sizeof(__sum16) (2)
> >>>> bytes
> >>>>>>>>> +	 * appended to packet data
> >>>>>>>>> +	 */
> >>>>>>>>> +	if (unlikely(skb->len < sizeof(__sum16)))
> >>>>>>>>> +		return;
> >>>>>>>>> +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
> > [...]
> >>>>> Please check the section 30.5.6.1 checksum calculation handling>
> >>>>> And figure 30.25 the field of checksum attaching field
> >>>>
> >>>>    I have.
> >>>>
> >>>>> Also see Table 30.17 for checksum values for non-error conditions.
> >>>>
> >>>>> TCP/UDP/ICPM checksum is at last 2bytes.
> >>>>
> >>>>    What are you arguing with then? :-)
> >>>>    My point was that your code fetched the TCP/UDP/ICMP checksum
> >>>> ISO the IP checksum because it subtracts sizeof(__sum16), while
> >>>> should probably subtract sizeof(__wsum)
> >>>
> >>> Agreed. My code missed IP4 checksum result. May be we need to
> >>> extract 2 checksum info from last 4 bytes.  First checksum(2bytes)
> >>> is IP4 header checksum and next checksum(2 bytes)  for TCP/UDP/ICMP
> >>> and use this info finding the non error case mentioned in  Table
> 30.17.
> >>>
> >>> For eg:-
> >>> IPV6 non error-condition -->  "0xFFFF"-->IPV4HeaderCSum value and
> "0x0000"
> >>> TCP/UDP/ICMP CSUM value
> >>>
> >>> IPV4 non error-condition -->  "0x0000"-->IPV4HeaderCSum value and
> "0x0000"
> >>> TCP/UDP/ICMP CSUM value
> >>>
> >>> Do you agree?
> >
> >> What I meant here is some thing like below, please let me know if you
> >> have any issues with this, otherwise I would like to send the patch
> with below changes.
> >>
> >> Further improvements can happen later.
> >>
> >> Please let me know.
> >>
> >> +/* Hardware checksum status */
> >> +#define IPV4_RX_CSUM_OK                0x00000000
> >> +#define IPV6_RX_CSUM_OK                0xFFFF0000
> >
> >    Mhm, this should prolly come from the IP headers...
> >
> > [...]
> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >> b/drivers/net/ethernet/renesas/ravb_main.c
> >> index bbb42e5328e4..d9201fbbd472 100644
> >> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >> @@ -722,16 +722,18 @@ static void ravb_get_tx_tstamp(struct
> >> net_device *ndev)
> >>
> >>  static void ravb_rx_csum_gbeth(struct sk_buff *skb)  {
> >> -       u16 *hw_csum;
> >> +       u32 csum_result;
> >
> >    This is not against the patch currently under investigation. :-)
> >
> >> +       u8 *hw_csum;
> >>
> >>         /* The hardware checksum is contained in sizeof(__sum16) (2)
> bytes
> >>          * appended to packet data
> >>          */
> >> -       if (unlikely(skb->len < sizeof(__sum16)))
> >> +       if (unlikely(skb->len < sizeof(__wsum)))
> >
> >    I think this usage of __wsum is valid (I remember that I suggested
> it). We have 2 16-bit checksums here
> 
>    I meant "I don't think", of course. :-)

Ok will use 2 * sizeof(__sum16) instead and extract IPV4 header csum and TCP/UDP/ICMP csum result.

All error condition/unsupported cases will be passed to stack with CHECKSUM_NONE
and only non-error cases will be set as CHECKSUM_UNNCESSARY.

Does it sounds good to you?

Regards,
Biju



> 
> [...]
> 
> MBR, Sergey

Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

[Sergei Shtylyov]

On 09.10.2021 11:27, Biju Das wrote:

>>> [...]
>>>>>>>>>>> Fillup ravb_rx_gbeth() function to support RZ/G2L.
>>>>>>>>>>>
>>>>>>>>>>> This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
>>>>>>>>>>> consistent with the naming convention used in sh_eth driver.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>>>>>>>>>> Reviewed-by: Lad Prabhakar
>>>>>>>>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com>[...]
>>>>>>>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>>>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>>>>> index 37164a983156..42573eac82b9 100644
>>>>>>>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>>>>> @@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct
>>>>>>>>>>> net_device
>>>>>>>>>> *ndev)
>>>>>>>>>>>   	}
>>>>>>>>>>>   }
>>>>>>>>>>>
>>>>>>>>>>> +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
>>>>>>>>>>> +	u8 *hw_csum;
>>>>>>>>>>> +
>>>>>>>>>>> +	/* The hardware checksum is contained in sizeof(__sum16) (2)
>>>>>> bytes
>>>>>>>>>>> +	 * appended to packet data
>>>>>>>>>>> +	 */
>>>>>>>>>>> +	if (unlikely(skb->len < sizeof(__sum16)))
>>>>>>>>>>> +		return;
>>>>>>>>>>> +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
>>> [...]
>>>>>>> Please check the section 30.5.6.1 checksum calculation handling>
>>>>>>> And figure 30.25 the field of checksum attaching field
>>>>>>
>>>>>>     I have.
>>>>>>
>>>>>>> Also see Table 30.17 for checksum values for non-error conditions.
>>>>>>
>>>>>>> TCP/UDP/ICPM checksum is at last 2bytes.
>>>>>>
>>>>>>     What are you arguing with then? :-)
>>>>>>     My point was that your code fetched the TCP/UDP/ICMP checksum
>>>>>> ISO the IP checksum because it subtracts sizeof(__sum16), while
>>>>>> should probably subtract sizeof(__wsum)
>>>>>
>>>>> Agreed. My code missed IP4 checksum result. May be we need to
>>>>> extract 2 checksum info from last 4 bytes.  First checksum(2bytes)
>>>>> is IP4 header checksum and next checksum(2 bytes)  for TCP/UDP/ICMP
>>>>> and use this info finding the non error case mentioned in  Table
>> 30.17.
>>>>>
>>>>> For eg:-
>>>>> IPV6 non error-condition -->  "0xFFFF"-->IPV4HeaderCSum value and
>> "0x0000"
>>>>> TCP/UDP/ICMP CSUM value
>>>>>
>>>>> IPV4 non error-condition -->  "0x0000"-->IPV4HeaderCSum value and
>> "0x0000"
>>>>> TCP/UDP/ICMP CSUM value
>>>>>
>>>>> Do you agree?
>>>
>>>> What I meant here is some thing like below, please let me know if you
>>>> have any issues with this, otherwise I would like to send the patch
>> with below changes.
>>>>
>>>> Further improvements can happen later.
>>>>
>>>> Please let me know.
>>>>
>>>> +/* Hardware checksum status */
>>>> +#define IPV4_RX_CSUM_OK                0x00000000
>>>> +#define IPV6_RX_CSUM_OK                0xFFFF0000
>>>
>>>     Mhm, this should prolly come from the IP headers...
>>>
>>> [...]
>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>>> index bbb42e5328e4..d9201fbbd472 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>> @@ -722,16 +722,18 @@ static void ravb_get_tx_tstamp(struct
>>>> net_device *ndev)
>>>>
>>>>   static void ravb_rx_csum_gbeth(struct sk_buff *skb)  {
>>>> -       u16 *hw_csum;
>>>> +       u32 csum_result;
>>>
>>>     This is not against the patch currently under investigation. :-)
>>>
>>>> +       u8 *hw_csum;
>>>>
>>>>          /* The hardware checksum is contained in sizeof(__sum16) (2)
>> bytes
>>>>           * appended to packet data
>>>>           */
>>>> -       if (unlikely(skb->len < sizeof(__sum16)))
>>>> +       if (unlikely(skb->len < sizeof(__wsum)))
>>>
>>>     I think this usage of __wsum is valid (I remember that I suggested
>> it). We have 2 16-bit checksums here
>>
>>     I meant "I don't think", of course. :-)
> 
> Ok will use 2 * sizeof(__sum16) instead and extract IPV4 header csum and TCP/UDP/ICMP csum result.

    I'm not sure how to deal with the later...

> All error condition/unsupported cases will be passed to stack with CHECKSUM_NONE
> and only non-error cases will be set as CHECKSUM_UNNCESSARY.
> 
> Does it sounds good to you?

    No. The networking stack needs to know about the bad checksums too.

> Regards,
> Biju

>> [...]

MBR, Sergey

RE: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

[Biju Das]

Hi Sergey,

> Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
> 
> On 09.10.2021 11:27, Biju Das wrote:
> 
> >>> [...]
> >>>>>>>>>>> Fillup ravb_rx_gbeth() function to support RZ/G2L.
> >>>>>>>>>>>
> >>>>>>>>>>> This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
> >>>>>>>>>>> consistent with the naming convention used in sh_eth driver.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>>>>>>>>>> Reviewed-by: Lad Prabhakar
> >>>>>>>>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com>[...]
> >>>>>>>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >>>>>>>>>>> b/drivers/net/ethernet/renesas/ravb_main.c
> >>>>>>>>>>> index 37164a983156..42573eac82b9 100644
> >>>>>>>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>>>>>>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>>>>>>>>>> @@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct
> >>>>>>>>>>> net_device
> >>>>>>>>>> *ndev)
> >>>>>>>>>>>   	}
> >>>>>>>>>>>   }
> >>>>>>>>>>>
> >>>>>>>>>>> +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
> >>>>>>>>>>> +	u8 *hw_csum;
> >>>>>>>>>>> +
> >>>>>>>>>>> +	/* The hardware checksum is contained in sizeof(__sum16)
> >>>>>>>>>>> +(2)
> >>>>>> bytes
> >>>>>>>>>>> +	 * appended to packet data
> >>>>>>>>>>> +	 */
> >>>>>>>>>>> +	if (unlikely(skb->len < sizeof(__sum16)))
> >>>>>>>>>>> +		return;
> >>>>>>>>>>> +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
> >>> [...]
> >>>>>>> Please check the section 30.5.6.1 checksum calculation handling>
> >>>>>>> And figure 30.25 the field of checksum attaching field
> >>>>>>
> >>>>>>     I have.
> >>>>>>
> >>>>>>> Also see Table 30.17 for checksum values for non-error conditions.
> >>>>>>
> >>>>>>> TCP/UDP/ICPM checksum is at last 2bytes.
> >>>>>>
> >>>>>>     What are you arguing with then? :-)
> >>>>>>     My point was that your code fetched the TCP/UDP/ICMP checksum
> >>>>>> ISO the IP checksum because it subtracts sizeof(__sum16), while
> >>>>>> should probably subtract sizeof(__wsum)
> >>>>>
> >>>>> Agreed. My code missed IP4 checksum result. May be we need to
> >>>>> extract 2 checksum info from last 4 bytes.  First checksum(2bytes)
> >>>>> is IP4 header checksum and next checksum(2 bytes)  for
> >>>>> TCP/UDP/ICMP and use this info finding the non error case
> >>>>> mentioned in  Table
> >> 30.17.
> >>>>>
> >>>>> For eg:-
> >>>>> IPV6 non error-condition -->  "0xFFFF"-->IPV4HeaderCSum value and
> >> "0x0000"
> >>>>> TCP/UDP/ICMP CSUM value
> >>>>>
> >>>>> IPV4 non error-condition -->  "0x0000"-->IPV4HeaderCSum value and
> >> "0x0000"
> >>>>> TCP/UDP/ICMP CSUM value
> >>>>>
> >>>>> Do you agree?
> >>>
> >>>> What I meant here is some thing like below, please let me know if
> >>>> you have any issues with this, otherwise I would like to send the
> >>>> patch
> >> with below changes.
> >>>>
> >>>> Further improvements can happen later.
> >>>>
> >>>> Please let me know.
> >>>>
> >>>> +/* Hardware checksum status */
> >>>> +#define IPV4_RX_CSUM_OK                0x00000000
> >>>> +#define IPV6_RX_CSUM_OK                0xFFFF0000
> >>>
> >>>     Mhm, this should prolly come from the IP headers...
> >>>
> >>> [...]
> >>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >>>> b/drivers/net/ethernet/renesas/ravb_main.c
> >>>> index bbb42e5328e4..d9201fbbd472 100644
> >>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>>> @@ -722,16 +722,18 @@ static void ravb_get_tx_tstamp(struct
> >>>> net_device *ndev)
> >>>>
> >>>>   static void ravb_rx_csum_gbeth(struct sk_buff *skb)  {
> >>>> -       u16 *hw_csum;
> >>>> +       u32 csum_result;
> >>>
> >>>     This is not against the patch currently under investigation. :-)
> >>>
> >>>> +       u8 *hw_csum;
> >>>>
> >>>>          /* The hardware checksum is contained in sizeof(__sum16)
> >>>> (2)
> >> bytes
> >>>>           * appended to packet data
> >>>>           */
> >>>> -       if (unlikely(skb->len < sizeof(__sum16)))
> >>>> +       if (unlikely(skb->len < sizeof(__wsum)))
> >>>
> >>>     I think this usage of __wsum is valid (I remember that I
> >>> suggested
> >> it). We have 2 16-bit checksums here
> >>
> >>     I meant "I don't think", of course. :-)
> >
> > Ok will use 2 * sizeof(__sum16) instead and extract IPV4 header csum and
> TCP/UDP/ICMP csum result.
> 
>     I'm not sure how to deal with the later...
> 
> > All error condition/unsupported cases will be passed to stack with
> > CHECKSUM_NONE and only non-error cases will be set as
> CHECKSUM_UNNCESSARY.

> >
> > Does it sounds good to you?
> 
>     No. The networking stack needs to know about the bad checksums too.

Currently some of the drivers is doing this way only. It doesn't pass bad checksum.
Non-error case sets CHECKSUM_UNNCESSARY and other case sets CHECKSUM_NONE to handle
It by stack.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/qualcomm/emac/emac-mac.c#L1147
[2] https://elixir.bootlin.com/linux/latest/source/drivers/staging/octeon/ethernet-rx.c#L343

Regards,
Biju

> 
> > Regards,
> > Biju
> 
> >> [...]
> 
> MBR, Sergey

[RFC 08/12] ravb: Add carrier_counters to struct ravb_hw_info

[Biju Das]

RZ/G2L E-MAC supports carrier counters.
Add a carrier_counter hw feature bit to struct ravb_hw_info
to add this feature only for RZ/G2L.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
RFC changes:
 * Added Sergey's Rb tag.
---
 drivers/net/ethernet/renesas/ravb.h      |  5 +++++
 drivers/net/ethernet/renesas/ravb_main.c | 13 +++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 8c7b2569c7dd..899e16c5eb1a 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -196,11 +196,15 @@ enum ravb_reg {
 	MAHR	= 0x05c0,
 	MALR	= 0x05c8,
 	TROCR	= 0x0700,	/* R-Car Gen3 and RZ/G2L only */
+	CXR41	= 0x0708,	/* RZ/G2L only */
+	CXR42	= 0x0710,	/* RZ/G2L only */
 	CEFCR	= 0x0740,
 	FRECR	= 0x0748,
 	TSFRCR	= 0x0750,
 	TLFRCR	= 0x0758,
 	RFCR	= 0x0760,
+	CXR55	= 0x0768,	/* RZ/G2L only */
+	CXR56	= 0x0770,	/* RZ/G2L only */
 	MAFCR	= 0x0778,
 	CSR0    = 0x0800,	/* RZ/G2L only */
 	CSR1    = 0x0804,	/* RZ/G2L only */
@@ -1061,6 +1065,7 @@ struct ravb_hw_info {
 	unsigned nc_queue:1;		/* AVB-DMAC has NC queue */
 	unsigned magic_pkt:1;		/* E-MAC supports magic packet detection */
 	unsigned half_duplex:1;		/* E-MAC supports half duplex mode */
+	unsigned carrier_counters:1;	/* E-MAC has carrier counters */
 };
 
 struct ravb_private {
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 42573eac82b9..c057de81ec58 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2075,6 +2075,18 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
 		ravb_write(ndev, 0, TROCR);	/* (write clear) */
 	}
 
+	if (info->carrier_counters) {
+		nstats->collisions += ravb_read(ndev, CXR41);
+		ravb_write(ndev, 0, CXR41);	/* (write clear) */
+		nstats->tx_carrier_errors += ravb_read(ndev, CXR42);
+		ravb_write(ndev, 0, CXR42);	/* (write clear) */
+
+		nstats->tx_carrier_errors += ravb_read(ndev, CXR55);
+		ravb_write(ndev, 0, CXR55);	/* (write clear) */
+		nstats->tx_carrier_errors += ravb_read(ndev, CXR56);
+		ravb_write(ndev, 0, CXR56);	/* (write clear) */
+	}
+
 	nstats->rx_packets = stats0->rx_packets;
 	nstats->tx_packets = stats0->tx_packets;
 	nstats->rx_bytes = stats0->rx_bytes;
@@ -2486,6 +2498,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.aligned_tx = 1,
 	.tx_counters = 1,
 	.half_duplex = 1,
+	.carrier_counters = 1,
 };
 
 static const struct of_device_id ravb_match_table[] = {
-- 
2.17.1

Re: [RFC 08/12] ravb: Add carrier_counters to struct ravb_hw_info

[Sergey Shtylyov]

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

> RZ/G2L E-MAC supports carrier counters.
> Add a carrier_counter hw feature bit to struct ravb_hw_info
> to add this feature only for RZ/G2L.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 8c7b2569c7dd..899e16c5eb1a 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
[...]
@@ -1061,6 +1065,7 @@ struct ravb_hw_info {
 	unsigned nc_queue:1;		/* AVB-DMAC has NC queue */
 	unsigned magic_pkt:1;		/* E-MAC supports magic packet detection */
 	unsigned half_duplex:1;		/* E-MAC supports half duplex mode */
+	unsigned carrier_counters:1;	/* E-MAC has carrier counters */



[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 42573eac82b9..c057de81ec58 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2075,6 +2075,18 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
>  		ravb_write(ndev, 0, TROCR);	/* (write clear) */
>  	}
>  
> +	if (info->carrier_counters) {
> +		nstats->collisions += ravb_read(ndev, CXR41);
> +		ravb_write(ndev, 0, CXR41);	/* (write clear) */
> +		nstats->tx_carrier_errors += ravb_read(ndev, CXR42);
> +		ravb_write(ndev, 0, CXR42);	/* (write clear) */
> +
> +		nstats->tx_carrier_errors += ravb_read(ndev, CXR55);

   According to the manual, CXR55 counts RX events (carrier extension lost.

> +		ravb_write(ndev, 0, CXR55);	/* (write clear) */
> +		nstats->tx_carrier_errors += ravb_read(ndev, CXR56);

   And CXR56 counts receive events too...

> +		ravb_write(ndev, 0, CXR56);	/* (write clear) */
> +	}
> +
>  	nstats->rx_packets = stats0->rx_packets;
>  	nstats->tx_packets = stats0->tx_packets;
>  	nstats->rx_bytes = stats0->rx_bytes;
> @@ -2486,6 +2498,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
>  	.aligned_tx = 1,
>  	.tx_counters = 1,
>  	.half_duplex = 1,
> +	.carrier_counters = 1,

   At least init it next to 'tx_counters'. :-)

[...]

MBR, Sergey

Re: [RFC 08/12] ravb: Add carrier_counters to struct ravb_hw_info

[Sergey Shtylyov]

On 10/6/21 7:41 PM, Sergey Shtylyov wrote:

>> RZ/G2L E-MAC supports carrier counters.
>> Add a carrier_counter hw feature bit to struct ravb_hw_info
>> to add this feature only for RZ/G2L.
>>
>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> [...]
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>> index 8c7b2569c7dd..899e16c5eb1a 100644
>> --- a/drivers/net/ethernet/renesas/ravb.h
>> +++ b/drivers/net/ethernet/renesas/ravb.h
>> [...]
>> @@ -1061,6 +1065,7 @@ struct ravb_hw_info {
>>  	unsigned nc_queue:1;		/* AVB-DMAC has NC queue */
>>  	unsigned magic_pkt:1;		/* E-MAC supports magic packet detection */
>>  	unsigned half_duplex:1;		/* E-MAC supports half duplex mode */
>> +	unsigned carrier_counters:1;	/* E-MAC has carrier counters */

   I thought I'd typed here that this field should be declared next to the 'tx_counters' field. :-)
 
[...]

MBR, Sergey

RE: [RFC 08/12] ravb: Add carrier_counters to struct ravb_hw_info

[Biju Das]

Hi Sergey,

> Subject: Re: [RFC 08/12] ravb: Add carrier_counters to struct ravb_hw_info
> 
> On 10/6/21 7:41 PM, Sergey Shtylyov wrote:
> 
> >> RZ/G2L E-MAC supports carrier counters.
> >> Add a carrier_counter hw feature bit to struct ravb_hw_info to add
> >> this feature only for RZ/G2L.
> >>
> >> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> >
> > [...]
> >
> >> diff --git a/drivers/net/ethernet/renesas/ravb.h
> >> b/drivers/net/ethernet/renesas/ravb.h
> >> index 8c7b2569c7dd..899e16c5eb1a 100644
> >> --- a/drivers/net/ethernet/renesas/ravb.h
> >> +++ b/drivers/net/ethernet/renesas/ravb.h
> >> [...]
> >> @@ -1061,6 +1065,7 @@ struct ravb_hw_info {
> >>  	unsigned nc_queue:1;		/* AVB-DMAC has NC queue */
> >>  	unsigned magic_pkt:1;		/* E-MAC supports magic packet
> detection */
> >>  	unsigned half_duplex:1;		/* E-MAC supports half duplex mode */
> >> +	unsigned carrier_counters:1;	/* E-MAC has carrier counters */
> 
>    I thought I'd typed here that this field should be declared next to the
> 'tx_counters' field. :-)

Agreed. Will move to 'tx_counters' field.

Regards,
Biju

RE: [RFC 08/12] ravb: Add carrier_counters to struct ravb_hw_info

[Biju Das]

Hi Sergey,

Thanks for the feedback.

> Subject: Re: [RFC 08/12] ravb: Add carrier_counters to struct ravb_hw_info
> 
> On 10/5/21 2:06 PM, Biju Das wrote:
> 
> > RZ/G2L E-MAC supports carrier counters.
> > Add a carrier_counter hw feature bit to struct ravb_hw_info to add
> > this feature only for RZ/G2L.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> [...]
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h
> b/drivers/net/ethernet/renesas/ravb.h
> index 8c7b2569c7dd..899e16c5eb1a 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> [...]
> @@ -1061,6 +1065,7 @@ struct ravb_hw_info {
>  	unsigned nc_queue:1;		/* AVB-DMAC has NC queue */
>  	unsigned magic_pkt:1;		/* E-MAC supports magic packet
> detection */
>  	unsigned half_duplex:1;		/* E-MAC supports half duplex mode */
> +	unsigned carrier_counters:1;	/* E-MAC has carrier counters */
> 
> 
> 
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 42573eac82b9..c057de81ec58 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -2075,6 +2075,18 @@ static struct net_device_stats
> *ravb_get_stats(struct net_device *ndev)
> >  		ravb_write(ndev, 0, TROCR);	/* (write clear) */
> >  	}
> >
> > +	if (info->carrier_counters) {
> > +		nstats->collisions += ravb_read(ndev, CXR41);
> > +		ravb_write(ndev, 0, CXR41);	/* (write clear) */
> > +		nstats->tx_carrier_errors += ravb_read(ndev, CXR42);
> > +		ravb_write(ndev, 0, CXR42);	/* (write clear) */
> > +
> > +		nstats->tx_carrier_errors += ravb_read(ndev, CXR55);
> 
>    According to the manual, CXR55 counts RX events (carrier extension
> lost.

Agreed. will remove this from tx_carriers.

> 
> > +		ravb_write(ndev, 0, CXR55);	/* (write clear) */
> > +		nstats->tx_carrier_errors += ravb_read(ndev, CXR56);
> 
>    And CXR56 counts receive events too...

Agreed. will remove this from tx_carriers.


> 
> > +		ravb_write(ndev, 0, CXR56);	/* (write clear) */
> > +	}
> > +
> >  	nstats->rx_packets = stats0->rx_packets;
> >  	nstats->tx_packets = stats0->tx_packets;
> >  	nstats->rx_bytes = stats0->rx_bytes; @@ -2486,6 +2498,7 @@ static
> > const struct ravb_hw_info gbeth_hw_info = {
> >  	.aligned_tx = 1,
> >  	.tx_counters = 1,
> >  	.half_duplex = 1,
> > +	.carrier_counters = 1,
> 
>    At least init it next to 'tx_counters'. :-)

Agreed. wo;; move next to tx_counters.

Regards,
Biju

> 
> [...]
> 
> MBR, Sergey

[RFC 09/12] ravb: Add support to retrieve stats for GbEthernet

[Biju Das]

Add support for retrieving stats information for GbEthernet.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC changes:
 * New patch.
---
 drivers/net/ethernet/renesas/ravb_main.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index c057de81ec58..e2238cea9f3e 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1515,6 +1515,24 @@ static void ravb_set_msglevel(struct net_device *ndev, u32 value)
 	priv->msg_enable = value;
 }
 
+static const char ravb_gstrings_stats_gbeth[][ETH_GSTRING_LEN] = {
+	"rx_queue_0_current",
+	"tx_queue_0_current",
+	"rx_queue_0_dirty",
+	"tx_queue_0_dirty",
+	"rx_queue_0_packets",
+	"tx_queue_0_packets",
+	"rx_queue_0_bytes",
+	"tx_queue_0_bytes",
+	"rx_queue_0_mcast_packets",
+	"rx_queue_0_errors",
+	"rx_queue_0_crc_errors",
+	"rx_queue_0_frame_errors",
+	"rx_queue_0_length_errors",
+	"rx_queue_0_csum_offload_errors",
+	"rx_queue_0_over_errors",
+};
+
 static const char ravb_gstrings_stats[][ETH_GSTRING_LEN] = {
 	"rx_queue_0_current",
 	"tx_queue_0_current",
@@ -2492,6 +2510,9 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.dmac_init = ravb_dmac_init_gbeth,
 	.emac_init = ravb_emac_init_gbeth,
 	.net_hw_features = (NETIF_F_HW_CSUM | NETIF_F_RXCSUM),
+	.gstrings_stats = ravb_gstrings_stats_gbeth,
+	.gstrings_size = sizeof(ravb_gstrings_stats_gbeth),
+	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
 	.max_rx_len = ALIGN(GBETH_RX_BUFF_MAX, RAVB_ALIGN),
 	.tsrq = TCCR_TSRQ0,
 	.rx_max_buf_size = SZ_8K,
-- 
2.17.1

Re: [RFC 09/12] ravb: Add support to retrieve stats for GbEthernet

[Sergey Shtylyov]

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

> Add support for retrieving stats information for GbEthernet.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

[RFC 10/12] ravb: Rename "tsrq" variable

[Biju Das]

Rename the variable "tsrq" with "tccr_mask" as we are passing
TCCR mask to the ravb_wait() function.

There is no functional change.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
RFC changes:
 * New patch.
---
 drivers/net/ethernet/renesas/ravb.h      | 2 +-
 drivers/net/ethernet/renesas/ravb_main.c | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 899e16c5eb1a..010dad82091c 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1052,7 +1052,7 @@ struct ravb_hw_info {
 	netdev_features_t net_features;
 	int stats_len;
 	size_t max_rx_len;
-	u32 tsrq;
+	u32 tccr_mask;
 	u32 rx_max_buf_size;
 	unsigned aligned_tx: 1;
 
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index e2238cea9f3e..5d00650e86b0 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1043,7 +1043,7 @@ static int ravb_stop_dma(struct net_device *ndev)
 	int error;
 
 	/* Wait for stopping the hardware TX process */
-	error = ravb_wait(ndev, TCCR, info->tsrq, 0);
+	error = ravb_wait(ndev, TCCR, info->tccr_mask, 0);
 
 	if (error)
 		return error;
@@ -2467,7 +2467,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.net_features = NETIF_F_RXCSUM,
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
 	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
-	.tsrq = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
+	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
 	.rx_max_buf_size = SZ_2K,
 	.internal_delay = 1,
 	.tx_counters = 1,
@@ -2492,7 +2492,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.net_features = NETIF_F_RXCSUM,
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
 	.max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
-	.tsrq = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
+	.tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
 	.rx_max_buf_size = SZ_2K,
 	.aligned_tx = 1,
 	.gptp = 1,
@@ -2514,7 +2514,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.gstrings_size = sizeof(ravb_gstrings_stats_gbeth),
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
 	.max_rx_len = ALIGN(GBETH_RX_BUFF_MAX, RAVB_ALIGN),
-	.tsrq = TCCR_TSRQ0,
+	.tccr_mask = TCCR_TSRQ0,
 	.rx_max_buf_size = SZ_8K,
 	.aligned_tx = 1,
 	.tx_counters = 1,
-- 
2.17.1

Re: [RFC 10/12] ravb: Rename "tsrq" variable

[Sergey Shtylyov]

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

> Rename the variable "tsrq" with "tccr_mask" as we are passing
> TCCR mask to the ravb_wait() function.
> 
> There is no functional change.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

[RFC 11/12] ravb: Optimize ravb_emac_init_gbeth function

[Biju Das]

Optimize CXR31 register initialization on ravb_emac_init_gbeth
function.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
RFC changes:
 * New patch.
---
 drivers/net/ethernet/renesas/ravb_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 5d00650e86b0..dfbbda3681f8 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -539,8 +539,7 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
 	/* E-MAC interrupt enable register */
 	ravb_write(ndev, ECSIPR_ICDIP, ECSIPR);
 
-	ravb_modify(ndev, CXR31, CXR31_SEL_LINK1, 0);
-	ravb_modify(ndev, CXR31, CXR31_SEL_LINK0, CXR31_SEL_LINK0);
+	ravb_modify(ndev, CXR31, CXR31_SEL_LINK0 | CXR31_SEL_LINK1, CXR31_SEL_LINK0);
 }
 
 static void ravb_emac_init_rcar(struct net_device *ndev)
-- 
2.17.1

Re: [RFC 11/12] ravb: Optimize ravb_emac_init_gbeth function

[Sergey Shtylyov]

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

> Optimize CXR31 register initialization on ravb_emac_init_gbeth
> function.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

[RFC 12/12] ravb: Update/Add comments

[Biju Das]

This patch update/add the following comments

1) Fix the typo AVB->DMAC in comment, as the code following the comment
   is for GbEthernet DMAC in ravb_dmac_init_gbeth()

2) Update the comment "PAUSE prohibition"-> "EMAC Mode: PAUSE
   prohibition; Duplex; TX; RX;" in ravb_emac_init_gbeth()

3) Document PFRI register bit, as it is only supported for
   R-Car Gen3 and RZ/G2L.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
RFC changes:
 * New patch.
---
 drivers/net/ethernet/renesas/ravb.h      | 2 +-
 drivers/net/ethernet/renesas/ravb_main.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 010dad82091c..472254612d6a 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -833,7 +833,7 @@ enum ECSR_BIT {
 	ECSR_MPD	= 0x00000002,
 	ECSR_LCHNG	= 0x00000004,
 	ECSR_PHYI	= 0x00000008,
-	ECSR_PFRI	= 0x00000010,
+	ECSR_PFRI	= 0x00000010,	/* Documented for R-Car Gen3 and RZ/G2L */
 };
 
 /* ECSIPR */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index dfbbda3681f8..4a057005a470 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -519,7 +519,7 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
 	/* Receive frame limit set register */
 	ravb_write(ndev, GBETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
 
-	/* PAUSE prohibition */
+	/* EMAC Mode: PAUSE prohibition; Duplex; TX; RX; */
 	ravb_write(ndev, ECMR_ZPF | ((priv->duplex > 0) ? ECMR_DM : 0) |
 			 ECMR_TE | ECMR_RE | ECMR_RCPT |
 			 ECMR_TXF | ECMR_RXF | ECMR_PRM, ECMR);
@@ -588,7 +588,7 @@ static int ravb_dmac_init_gbeth(struct net_device *ndev)
 	/* Descriptor format */
 	ravb_ring_format(ndev, RAVB_BE);
 
-	/* Set AVB RX */
+	/* Set DMAC RX */
 	ravb_write(ndev, 0x60000000, RCR);
 
 	/* Set Max Frame Length (RTC) */
-- 
2.17.1

Re: [RFC 12/12] ravb: Update/Add comments

[Sergey Shtylyov]

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

> This patch update/add the following comments
> 
> 1) Fix the typo AVB->DMAC in comment, as the code following the comment
>    is for GbEthernet DMAC in ravb_dmac_init_gbeth()

   ; not needed at the end of the comment. :-)

> 
> 2) Update the comment "PAUSE prohibition"-> "EMAC Mode: PAUSE
>    prohibition; Duplex; TX; RX;" in ravb_emac_init_gbeth()
> 
> 3) Document PFRI register bit, as it is only supported for
>    R-Car Gen3 and RZ/G2L.

   Not a good idea to do 3 different things in 1 patch... I know I said that (2) isn't worth
a separate patch but I meant that it shouldbe done as a part of a lrger ravb_emac_init_gbeth()
change. Sorry for not being clear enough...

> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> RFC changes:
>  * New patch.
> ---
>  drivers/net/ethernet/renesas/ravb.h      | 2 +-
>  drivers/net/ethernet/renesas/ravb_main.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index dfbbda3681f8..4a057005a470 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -519,7 +519,7 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
>  	/* Receive frame limit set register */
>  	ravb_write(ndev, GBETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
>  
> -	/* PAUSE prohibition */
> +	/* EMAC Mode: PAUSE prohibition; Duplex; TX; RX; */

   No need for ; after RX.

[...]

MBR, Sergey

RE: [RFC 12/12] ravb: Update/Add comments

[Biju Das]

Hi Sergei,

> Subject: Re: [RFC 12/12] ravb: Update/Add comments
> 
> On 10/5/21 2:06 PM, Biju Das wrote:
> 
> > This patch update/add the following comments
> >
> > 1) Fix the typo AVB->DMAC in comment, as the code following the comment
> >    is for GbEthernet DMAC in ravb_dmac_init_gbeth()
> 
>    ; not needed at the end of the comment. :-)
> 
> >
> > 2) Update the comment "PAUSE prohibition"-> "EMAC Mode: PAUSE
> >    prohibition; Duplex; TX; RX;" in ravb_emac_init_gbeth()
> >
> > 3) Document PFRI register bit, as it is only supported for
> >    R-Car Gen3 and RZ/G2L.
> 
>    Not a good idea to do 3 different things in 1 patch... I know I said
> that (2) isn't worth a separate patch but I meant that it shouldbe done as
> a part of a lrger ravb_emac_init_gbeth() change. Sorry for not being clear
> enough...

We are improving comments on Gbethernet driver, so I thought 1 patch will address
All the comments, since there is no functional change.

OK will create 3 separate patches for fixing these comments.

> 
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > RFC changes:
> >  * New patch.
> > ---
> >  drivers/net/ethernet/renesas/ravb.h      | 2 +-
> >  drivers/net/ethernet/renesas/ravb_main.c | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index dfbbda3681f8..4a057005a470 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -519,7 +519,7 @@ static void ravb_emac_init_gbeth(struct net_device
> *ndev)
> >  	/* Receive frame limit set register */
> >  	ravb_write(ndev, GBETH_RX_BUFF_MAX + ETH_FCS_LEN, RFLR);
> >
> > -	/* PAUSE prohibition */
> > +	/* EMAC Mode: PAUSE prohibition; Duplex; TX; RX; */
> 
>    No need for ; after RX.

OK.

Regards,
Biju

Re: [RFC 00/12] Add functional support for Gigabit Ethernet driver

[Sergey Shtylyov]

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

> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC are
> similar to the R-Car Ethernet AVB IP.
> 
> The Gigabit Ethernet IP consists of Ethernet controller (E-MAC), Internal
> TCP/IP Offload Engine (TOE)  and Dedicated Direct memory access controller
> (DMAC).
> 
> With a few changes in the driver we can support both IPs.
> 
> This patch series is aims to add functional support for Gigabit Ethernet driver
> by filling all the stubs.
> 
> Ref:-
> https://lore.kernel.org/linux-renesas-soc/OS0PR01MB5922240F88E5E0FD989ECDF386AC9@OS0PR01MB5922.jpnprd01.prod.outlook.com/T/#m8dee0a1b14d505d4611cad8c10e4017a30db55d6
> 
> RFC changes:
>  * used ALIGN macro for calculating the value for max_rx_len.
>  * used rx_max_buf_size instead of rx_2k_buffers feature bit.
>  * moved struct ravb_rx_desc *gbeth_rx_ring near to ravb_private::rx_ring
>    and allocating it for 1 RX queue.
>  * Started using gbeth_rx_ring instead of gbeth_rx_ring[q].
>  * renamed ravb_alloc_rx_desc to ravb_alloc_rx_desc_rcar
>  * renamed ravb_rx_ring_free to ravb_rx_ring_free_rcar
>  * renamed ravb_rx_ring_format to ravb_rx_ring_format_rcar
>  * renamed ravb_rcar_rx to ravb_rx_rcar
>  * renamed "tsrq" variable
>  * Updated the comments
> 
> Biju Das (12):
>   ravb: Use ALIGN macro for max_rx_len
>   ravb: Add rx_max_buf_size to struct ravb_hw_info
>   ravb: Fillup ravb_set_features_gbeth() stub
>   ravb: Fillup ravb_alloc_rx_desc_gbeth() stub
>   ravb: Fillup ravb_rx_ring_free_gbeth() stub
>   ravb: Fillup ravb_rx_ring_format_gbeth() stub
>   ravb: Fillup ravb_rx_gbeth() stub
>   ravb: Add carrier_counters to struct ravb_hw_info
>   ravb: Add support to retrieve stats for GbEthernet
>   ravb: Rename "tsrq" variable
>   ravb: Optimize ravb_emac_init_gbeth function
>   ravb: Update/Add comments
> 
>  drivers/net/ethernet/renesas/ravb.h      |  51 +++-
>  drivers/net/ethernet/renesas/ravb_main.c | 349 +++++++++++++++++++++--
>  2 files changed, 367 insertions(+), 33 deletions(-)

   I dodn;'t expect the patchset to be reposted so soon but I'll switch
to reviewing it insted of the previously posted 8-patch series...

MBR, Sergey

RE: [RFC 00/12] Add functional support for Gigabit Ethernet driver

[Biju Das]

Hi Sergey,

> Subject: Re: [RFC 00/12] Add functional support for Gigabit Ethernet
> driver
> 
> On 10/5/21 2:06 PM, Biju Das wrote:
> 
> > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC
> > are similar to the R-Car Ethernet AVB IP.
> >
> > The Gigabit Ethernet IP consists of Ethernet controller (E-MAC),
> > Internal TCP/IP Offload Engine (TOE)  and Dedicated Direct memory
> > access controller (DMAC).
> >
> > With a few changes in the driver we can support both IPs.
> >
> > This patch series is aims to add functional support for Gigabit
> > Ethernet driver by filling all the stubs.
> >
> > Ref:-
> > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Flinux-renesas-soc%2FOS0PR01MB5922240F88E5E0FD989ECDF386A
> > C9%40OS0PR01MB5922.jpnprd01.prod.outlook.com%2FT%2F%23m8dee0a1b14d505d
> > 4611cad8c10e4017a30db55d6&data=04%7C01%7Cbiju.das.jz%40bp.renesas.
> > com%7C880ddc38cf254b0a81fc08d987f6ea17%7C53d82571da1947e49cb4625a166a4
> > a2a%7C0%7C0%7C637690316835703147%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wL
> > jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata
> > =WmbtErppjUTywkNet%2FtDKw9v5gqaqRlcHGjI3PZ1UN8%3D&reserved=0
> >
> > RFC changes:
> >  * used ALIGN macro for calculating the value for max_rx_len.
> >  * used rx_max_buf_size instead of rx_2k_buffers feature bit.
> >  * moved struct ravb_rx_desc *gbeth_rx_ring near to
> ravb_private::rx_ring
> >    and allocating it for 1 RX queue.
> >  * Started using gbeth_rx_ring instead of gbeth_rx_ring[q].
> >  * renamed ravb_alloc_rx_desc to ravb_alloc_rx_desc_rcar
> >  * renamed ravb_rx_ring_free to ravb_rx_ring_free_rcar
> >  * renamed ravb_rx_ring_format to ravb_rx_ring_format_rcar
> >  * renamed ravb_rcar_rx to ravb_rx_rcar
> >  * renamed "tsrq" variable
> >  * Updated the comments
> >
> > Biju Das (12):
> >   ravb: Use ALIGN macro for max_rx_len
> >   ravb: Add rx_max_buf_size to struct ravb_hw_info
> >   ravb: Fillup ravb_set_features_gbeth() stub
> >   ravb: Fillup ravb_alloc_rx_desc_gbeth() stub
> >   ravb: Fillup ravb_rx_ring_free_gbeth() stub
> >   ravb: Fillup ravb_rx_ring_format_gbeth() stub
> >   ravb: Fillup ravb_rx_gbeth() stub
> >   ravb: Add carrier_counters to struct ravb_hw_info
> >   ravb: Add support to retrieve stats for GbEthernet
> >   ravb: Rename "tsrq" variable
> >   ravb: Optimize ravb_emac_init_gbeth function
> >   ravb: Update/Add comments
> >
> >  drivers/net/ethernet/renesas/ravb.h      |  51 +++-
> >  drivers/net/ethernet/renesas/ravb_main.c | 349
> > +++++++++++++++++++++--
> >  2 files changed, 367 insertions(+), 33 deletions(-)
> 
>    I dodn;'t expect the patchset to be reposted so soon but I'll switch to
> reviewing it insted of the previously posted 8-patch series...

Previous patchset posted as actual patch and there is a suggestion[1] to send it as RFC.
That is the reason I have send it as RFC. I have incorporated your lastest comment from previously
posted 8-patch series in this new RFC patchset.

[1] https://lore.kernel.org/linux-renesas-soc/OS0PR01MB5922240F88E5E0FD989ECDF386AC9@OS0PR01MB5922.jpnprd01.prod.outlook.com/T/#m8dee0a1b14d505d4611cad8c10e4017a30db55d6

Regards,
biju

> 
> MBR, Sergey