[PATCH/RFC 0/2] Add Gigabit Ethernet driver support

[PATCH/RFC 0/2] Add Gigabit Ethernet driver support

[Biju Das]

The DMAC and EMAC blocks of Gigabit Ethernet IP is almost
similar to Ethernet AVB.

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

With few canges in driver, we can support Gigabit ethernet driver as well.
I have prototyped the driver and tested with renesas-drivers master branch.

Please share your valuable comments.

Thanks.

Biju Das (2):
  ravb: Preparation for supporting Gigabit Ethernet driver
  ravb: Add GbEthernet driver support

 drivers/net/ethernet/renesas/ravb.h      |  94 ++-
 drivers/net/ethernet/renesas/ravb_main.c | 830 +++++++++++++++++++----
 2 files changed, 774 insertions(+), 150 deletions(-)

-- 
2.17.1

[PATCH/RFC 1/2] ravb: Preparation for supporting Gigabit Ethernet driver

[Biju Das]

The DMAC and EMAC blocks of Gigabit Ethernet IP is almost
similar to Ethernet AVB. With few canges in driver we can
support both the IP. This patch is in preparation for
supporting the same.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |   2 +-
 drivers/net/ethernet/renesas/ravb_main.c | 183 ++++++++++++++---------
 2 files changed, 110 insertions(+), 75 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 86a1eb0634e8..80e62ca2e3d3 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -864,7 +864,7 @@ enum GECMR_BIT {
 
 /* The Ethernet AVB descriptor definitions. */
 struct ravb_desc {
-	__le16 ds;		/* Descriptor size */
+	__le16 ds;	/* Descriptor size */
 	u8 cc;		/* Content control MSBs (reserved) */
 	u8 die_dt;	/* Descriptor interrupt enable and type */
 	__le32 dptr;	/* Descriptor pointer */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 4afff320dfd0..7e6feda59f4a 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -217,6 +217,29 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
 }
 
 /* Free skb's and DMA buffers for Ethernet AVB */
+static void ravb_ring_free_ex(struct net_device *ndev, int q)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int ring_size;
+	int i;
+
+	for (i = 0; i < priv->num_rx_ring[q]; i++) {
+		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
+
+		if (!dma_mapping_error(ndev->dev.parent,
+				       le32_to_cpu(desc->dptr)))
+			dma_unmap_single(ndev->dev.parent,
+					 le32_to_cpu(desc->dptr),
+					 RX_BUF_SZ,
+					 DMA_FROM_DEVICE);
+	}
+	ring_size = sizeof(struct ravb_ex_rx_desc) *
+		    (priv->num_rx_ring[q] + 1);
+	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
+			  priv->rx_desc_dma[q]);
+	priv->rx_ring[q] = NULL;
+}
+
 static void ravb_ring_free(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -225,21 +248,7 @@ static void ravb_ring_free(struct net_device *ndev, int q)
 	int i;
 
 	if (priv->rx_ring[q]) {
-		for (i = 0; i < priv->num_rx_ring[q]; i++) {
-			struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
-
-			if (!dma_mapping_error(ndev->dev.parent,
-					       le32_to_cpu(desc->dptr)))
-				dma_unmap_single(ndev->dev.parent,
-						 le32_to_cpu(desc->dptr),
-						 RX_BUF_SZ,
-						 DMA_FROM_DEVICE);
-		}
-		ring_size = sizeof(struct ravb_ex_rx_desc) *
-			    (priv->num_rx_ring[q] + 1);
-		dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
-				  priv->rx_desc_dma[q]);
-		priv->rx_ring[q] = NULL;
+		ravb_ring_free_ex(ndev, q);
 	}
 
 	if (priv->tx_ring[q]) {
@@ -272,26 +281,15 @@ static void ravb_ring_free(struct net_device *ndev, int q)
 }
 
 /* Format skb and descriptor buffer for Ethernet AVB */
-static void ravb_ring_format(struct net_device *ndev, int q)
+static void ravb_ring_format_ex(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	int num_tx_desc = priv->num_tx_desc;
 	struct ravb_ex_rx_desc *rx_desc;
-	struct ravb_tx_desc *tx_desc;
-	struct ravb_desc *desc;
 	int rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
-	int tx_ring_size = sizeof(*tx_desc) * priv->num_tx_ring[q] *
-			   num_tx_desc;
 	dma_addr_t dma_addr;
 	int i;
 
-	priv->cur_rx[q] = 0;
-	priv->cur_tx[q] = 0;
-	priv->dirty_rx[q] = 0;
-	priv->dirty_tx[q] = 0;
-
 	memset(priv->rx_ring[q], 0, rx_ring_size);
-	/* Build RX ring buffer */
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
 		/* RX descriptor */
 		rx_desc = &priv->rx_ring[q][i];
@@ -310,6 +308,25 @@ static void ravb_ring_format(struct net_device *ndev, int q)
 	rx_desc = &priv->rx_ring[q][i];
 	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
 	rx_desc->die_dt = DT_LINKFIX; /* type */
+}
+
+static void ravb_ring_format(struct net_device *ndev, int q)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int num_tx_desc = priv->num_tx_desc;
+	struct ravb_tx_desc *tx_desc;
+	struct ravb_desc *desc;
+	int tx_ring_size = sizeof(*tx_desc) * priv->num_tx_ring[q] *
+			   num_tx_desc;
+	int i;
+
+	priv->cur_rx[q] = 0;
+	priv->cur_tx[q] = 0;
+	priv->dirty_rx[q] = 0;
+	priv->dirty_tx[q] = 0;
+
+	/* Build RX ring buffer */
+	ravb_ring_format_ex(ndev, q);
 
 	memset(priv->tx_ring[q], 0, tx_ring_size);
 	/* Build TX ring buffer */
@@ -339,6 +356,7 @@ static void ravb_ring_format(struct net_device *ndev, int q)
 static int ravb_ring_init(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	size_t skb_sz = RX_BUF_SZ;
 	int num_tx_desc = priv->num_tx_desc;
 	struct sk_buff *skb;
 	int ring_size;
@@ -353,7 +371,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 		goto error;
 
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
-		skb = netdev_alloc_skb(ndev, RX_BUF_SZ + RAVB_ALIGN - 1);
+		skb = netdev_alloc_skb(ndev, skb_sz + RAVB_ALIGN - 1);
 		if (!skb)
 			goto error;
 		ravb_set_buffer_align(skb);
@@ -396,7 +414,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 }
 
 /* E-MAC init function */
-static void ravb_emac_init(struct net_device *ndev)
+static void ravb_emac_init_ex(struct net_device *ndev)
 {
 	/* Receive frame limit set register */
 	ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);
@@ -422,29 +440,15 @@ static void ravb_emac_init(struct net_device *ndev)
 	ravb_write(ndev, ECSIPR_ICDIP | ECSIPR_MPDIP | ECSIPR_LCHNGIP, ECSIPR);
 }
 
+static void ravb_emac_init(struct net_device *ndev)
+{
+	ravb_emac_init_ex(ndev);
+}
+
 /* Device init function for Ethernet AVB */
-static int ravb_dmac_init(struct net_device *ndev)
+static void ravb_dmac_init_ex(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	int error;
-
-	/* Set CONFIG mode */
-	error = ravb_config(ndev);
-	if (error)
-		return error;
-
-	error = ravb_ring_init(ndev, RAVB_BE);
-	if (error)
-		return error;
-	error = ravb_ring_init(ndev, RAVB_NC);
-	if (error) {
-		ravb_ring_free(ndev, RAVB_BE);
-		return error;
-	}
-
-	/* Descriptor format */
-	ravb_ring_format(ndev, RAVB_BE);
-	ravb_ring_format(ndev, RAVB_NC);
 
 	/* Set AVB RX */
 	ravb_write(ndev,
@@ -471,6 +475,31 @@ static int ravb_dmac_init(struct net_device *ndev)
 	ravb_write(ndev, RIC2_QFE0 | RIC2_QFE1 | RIC2_RFFE, RIC2);
 	/* Frame transmitted, timestamp FIFO updated */
 	ravb_write(ndev, TIC_FTE0 | TIC_FTE1 | TIC_TFUE, TIC);
+}
+
+static int ravb_dmac_init(struct net_device *ndev)
+{
+	int error;
+
+	/* Set CONFIG mode */
+	error = ravb_config(ndev);
+	if (error)
+		return error;
+
+	error = ravb_ring_init(ndev, RAVB_BE);
+	if (error)
+		return error;
+	error = ravb_ring_init(ndev, RAVB_NC);
+	if (error) {
+		ravb_ring_free(ndev, RAVB_BE);
+		return error;
+	}
+
+	/* Descriptor format */
+	ravb_ring_format(ndev, RAVB_BE);
+	ravb_ring_format(ndev, RAVB_NC);
+
+	ravb_dmac_init_ex(ndev);
 
 	/* Setting the control will start the AVB-DMAC process. */
 	ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_OPERATION);
@@ -532,7 +561,7 @@ static void ravb_rx_csum(struct sk_buff *skb)
 }
 
 /* Packet receive function for Ethernet AVB */
-static bool ravb_rx(struct net_device *ndev, int *quota, int q)
+static bool ravb_rx_ex(struct net_device *ndev, int *quota, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
@@ -647,6 +676,11 @@ static bool ravb_rx(struct net_device *ndev, int *quota, int q)
 	return boguscnt <= 0;
 }
 
+static bool ravb_rx(struct net_device *ndev, int *quota, int q)
+{
+	return ravb_rx_ex(ndev, quota, q);
+}
+
 static void ravb_rcv_snd_disable(struct net_device *ndev)
 {
 	/* Disable TX and RX */
@@ -766,7 +800,7 @@ static bool ravb_queue_interrupt(struct net_device *ndev, int q)
 	if (((ris0 & ric0) & BIT(q)) || ((tis  & tic)  & BIT(q))) {
 		if (napi_schedule_prep(&priv->napi[q])) {
 			/* Mask RX and TX interrupts */
-			if (priv->chip_id == RCAR_GEN2) {
+			if (priv->chip_id != RCAR_GEN3) {
 				ravb_write(ndev, ric0 & ~BIT(q), RIC0);
 				ravb_write(ndev, tic & ~BIT(q), TIC);
 			} else {
@@ -920,7 +954,7 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 	if (ravb_rx(ndev, &quota, q))
 		goto out;
 
-	/* Processing RX Descriptor Ring */
+	/* Processing TX Descriptor Ring */
 	spin_lock_irqsave(&priv->lock, flags);
 	/* Clear TX interrupt */
 	ravb_write(ndev, ~(mask | TIS_RESERVED), TIS);
@@ -932,7 +966,7 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 
 	/* Re-enable RX/TX interrupts */
 	spin_lock_irqsave(&priv->lock, flags);
-	if (priv->chip_id == RCAR_GEN2) {
+	if (priv->chip_id != RCAR_GEN3) {
 		ravb_modify(ndev, RIC0, mask, mask);
 		ravb_modify(ndev, TIC,  mask, mask);
 	} else {
@@ -1149,11 +1183,12 @@ static void ravb_get_ethtool_stats(struct net_device *ndev,
 				   struct ethtool_stats *estats, u64 *data)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	int num_queue = NUM_RX_QUEUE;
 	int i = 0;
 	int q;
 
 	/* Device-specific stats */
-	for (q = RAVB_BE; q < NUM_RX_QUEUE; q++) {
+	for (q = RAVB_BE; q < num_queue; q++) {
 		struct net_device_stats *stats = &priv->stats[q];
 
 		data[i++] = priv->cur_rx[q];
@@ -1341,7 +1376,7 @@ static int ravb_open(struct net_device *ndev)
 	napi_enable(&priv->napi[RAVB_BE]);
 	napi_enable(&priv->napi[RAVB_NC]);
 
-	if (priv->chip_id == RCAR_GEN2) {
+	if (priv->chip_id != RCAR_GEN3) {
 		error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED,
 				    ndev->name, ndev);
 		if (error) {
@@ -1708,7 +1743,7 @@ static int ravb_close(struct net_device *ndev)
 			of_phy_deregister_fixed_link(np);
 	}
 
-	if (priv->chip_id != RCAR_GEN2) {
+	if (priv->chip_id == RCAR_GEN3) {
 		free_irq(priv->tx_irqs[RAVB_NC], ndev);
 		free_irq(priv->rx_irqs[RAVB_NC], ndev);
 		free_irq(priv->tx_irqs[RAVB_BE], ndev);
@@ -1963,7 +1998,7 @@ static void ravb_set_config_mode(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 
-	if (priv->chip_id == RCAR_GEN2) {
+	if (priv->chip_id != RCAR_GEN3) {
 		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
 		/* Set CSEL value */
 		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
@@ -2059,17 +2094,22 @@ static int ravb_probe(struct platform_device *pdev)
 	if (!ndev)
 		return -ENOMEM;
 
+	/* The Ether-specific entries in the device structure. */
+	ndev->base_addr = res->start;
+
+	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
+
+	SET_NETDEV_DEV(ndev, &pdev->dev);
+
+	priv = netdev_priv(ndev);
+	priv->chip_id = chip_id;
+
 	ndev->features = NETIF_F_RXCSUM;
 	ndev->hw_features = NETIF_F_RXCSUM;
 
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
-	/* The Ether-specific entries in the device structure. */
-	ndev->base_addr = res->start;
-
-	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
-
 	if (chip_id == RCAR_GEN3)
 		irq = platform_get_irq_byname(pdev, "ch22");
 	else
@@ -2080,9 +2120,6 @@ static int ravb_probe(struct platform_device *pdev)
 	}
 	ndev->irq = irq;
 
-	SET_NETDEV_DEV(ndev, &pdev->dev);
-
-	priv = netdev_priv(ndev);
 	priv->ndev = ndev;
 	priv->pdev = pdev;
 	priv->num_tx_ring[RAVB_BE] = BE_TX_RING_SIZE;
@@ -2131,8 +2168,6 @@ static int ravb_probe(struct platform_device *pdev)
 		}
 	}
 
-	priv->chip_id = chip_id;
-
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		error = PTR_ERR(priv->clk);
@@ -2149,8 +2184,8 @@ static int ravb_probe(struct platform_device *pdev)
 	ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
 	ndev->min_mtu = ETH_MIN_MTU;
 
-	priv->num_tx_desc = chip_id == RCAR_GEN2 ?
-		NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3;
+	priv->num_tx_desc = chip_id == RCAR_GEN3 ?
+		NUM_TX_DESC_GEN3 : NUM_TX_DESC_GEN2;
 
 	/* Set function */
 	ndev->netdev_ops = &ravb_netdev_ops;
@@ -2167,7 +2202,7 @@ static int ravb_probe(struct platform_device *pdev)
 	/* Request GTI loading */
 	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
 
-	if (priv->chip_id != RCAR_GEN2) {
+	if (priv->chip_id == RCAR_GEN3) {
 		ravb_parse_delay_mode(np, ndev);
 		ravb_set_delay_mode(ndev);
 	}
@@ -2191,7 +2226,7 @@ static int ravb_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&priv->ts_skb_list);
 
 	/* Initialise PTP Clock driver */
-	if (chip_id != RCAR_GEN2)
+	if (chip_id == RCAR_GEN3)
 		ravb_ptp_init(ndev, pdev);
 
 	/* Debug message level */
@@ -2239,7 +2274,7 @@ static int ravb_probe(struct platform_device *pdev)
 			  priv->desc_bat_dma);
 
 	/* Stop PTP Clock driver */
-	if (chip_id != RCAR_GEN2)
+	if (chip_id == RCAR_GEN3)
 		ravb_ptp_stop(ndev);
 out_disable_refclk:
 	clk_disable_unprepare(priv->refclk);
@@ -2257,7 +2292,7 @@ static int ravb_remove(struct platform_device *pdev)
 	struct ravb_private *priv = netdev_priv(ndev);
 
 	/* Stop PTP Clock driver */
-	if (priv->chip_id != RCAR_GEN2)
+	if (priv->chip_id == RCAR_GEN3)
 		ravb_ptp_stop(ndev);
 
 	clk_disable_unprepare(priv->refclk);
@@ -2362,7 +2397,7 @@ static int __maybe_unused ravb_resume(struct device *dev)
 	/* Request GTI loading */
 	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
 
-	if (priv->chip_id != RCAR_GEN2)
+	if (priv->chip_id == RCAR_GEN3)
 		ravb_set_delay_mode(ndev);
 
 	/* Restore descriptor base address table */
-- 
2.17.1

Re: [PATCH/RFC 1/2] ravb: Preparation for supporting Gigabit Ethernet driver

[Andrew Lunn]

On Wed, Jul 14, 2021 at 03:54:07PM +0100, Biju Das wrote:
> The DMAC and EMAC blocks of Gigabit Ethernet IP is almost
> similar to Ethernet AVB. With few canges in driver we can

changes

> support both the IP. This patch is in preparation for
> supporting the same.

Please break this up a bit, it is hard to review. You can put all the
refactoring into helpers into one patch. But changes like

> -			if (priv->chip_id == RCAR_GEN2) {
> +			if (priv->chip_id != RCAR_GEN3) {

should be in a seperate patch with an explanation.

You are aiming for lots of very simple patches which are obviously
correct.

> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 86a1eb0634e8..80e62ca2e3d3 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -864,7 +864,7 @@ enum GECMR_BIT {
>  
>  /* The Ethernet AVB descriptor definitions. */
>  struct ravb_desc {
> -	__le16 ds;		/* Descriptor size */
> +	__le16 ds;	/* Descriptor size */
>  	u8 cc;		/* Content control MSBs (reserved) */
>  	u8 die_dt;	/* Descriptor interrupt enable and type */
>  	__le32 dptr;	/* Descriptor pointer */

Please put white spaces changes in a patch of its own.

       Andrew

RE: [PATCH/RFC 1/2] ravb: Preparation for supporting Gigabit Ethernet driver

[Biju Das]

Hi Andrew Lunn,

Thanks for the feedback.

> Subject: Re: [PATCH/RFC 1/2] ravb: Preparation for supporting Gigabit
> Ethernet driver
> 
> On Wed, Jul 14, 2021 at 03:54:07PM +0100, Biju Das wrote:
> > The DMAC and EMAC blocks of Gigabit Ethernet IP is almost similar to
> > Ethernet AVB. With few canges in driver we can
> 
> changes

Ok. Will fix it.

> 
> > support both the IP. This patch is in preparation for supporting the
> > same.
> 
> Please break this up a bit, it is hard to review. You can put all the
> refactoring into helpers into one patch. But changes like

> > -			if (priv->chip_id == RCAR_GEN2) {
> > +			if (priv->chip_id != RCAR_GEN3) {
> 
> should be in a seperate patch with an explanation.

Ok.

> 
> You are aiming for lots of very simple patches which are obviously
> correct.

OK will make simple patches

1) Code common to R-Car Gen2 and RZ/G2L (priv->chip_id != RCAR_GEN3)
2) Code specific to R-Car Gen3.
3) Spelling mistake
4) White space 
5) Refactorization patches


> 
> > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > b/drivers/net/ethernet/renesas/ravb.h
> > index 86a1eb0634e8..80e62ca2e3d3 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -864,7 +864,7 @@ enum GECMR_BIT {
> >
> >  /* The Ethernet AVB descriptor definitions. */  struct ravb_desc {
> > -	__le16 ds;		/* Descriptor size */
> > +	__le16 ds;	/* Descriptor size */
> >  	u8 cc;		/* Content control MSBs (reserved) */
> >  	u8 die_dt;	/* Descriptor interrupt enable and type */
> >  	__le32 dptr;	/* Descriptor pointer */
> 
> Please put white spaces changes in a patch of its own.

OK.

Thanks,
Biju

Re: [PATCH/RFC 1/2] ravb: Preparation for supporting Gigabit Ethernet driver

[Sergei Shtylyov]

Hello!

On 7/14/21 5:54 PM, Biju Das wrote:

> The DMAC and EMAC blocks of Gigabit Ethernet IP is almost
> similar to Ethernet AVB. With few canges in driver we can
> support both the IP. This patch is in preparation for
> supporting the same.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 86a1eb0634e8..80e62ca2e3d3 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -864,7 +864,7 @@ enum GECMR_BIT {
>  
>  /* The Ethernet AVB descriptor definitions. */
>  struct ravb_desc {
> -	__le16 ds;		/* Descriptor size */
> +	__le16 ds;	/* Descriptor size */

    Oops! But this should be a matter of the seperate patch if you want to fix whitespace...

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 4afff320dfd0..7e6feda59f4a 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -217,6 +217,29 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
>  }
>  
>  /* Free skb's and DMA buffers for Ethernet AVB */
> +static void ravb_ring_free_ex(struct net_device *ndev, int q)

   What does _ex() suffix mean (seems rather poor chice)? Perhaps rx would be better?

> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	int ring_size;
> +	int i;
> +
> +	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> +		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
> +
> +		if (!dma_mapping_error(ndev->dev.parent,
> +				       le32_to_cpu(desc->dptr)))
> +			dma_unmap_single(ndev->dev.parent,
> +					 le32_to_cpu(desc->dptr),
> +					 RX_BUF_SZ,
> +					 DMA_FROM_DEVICE);
> +	}
> +	ring_size = sizeof(struct ravb_ex_rx_desc) *
> +		    (priv->num_rx_ring[q] + 1);
> +	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
> +			  priv->rx_desc_dma[q]);
> +	priv->rx_ring[q] = NULL;
> +}
> +
>  static void ravb_ring_free(struct net_device *ndev, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
[...]
> @@ -272,26 +281,15 @@ static void ravb_ring_free(struct net_device *ndev, int q)
>  }
>  
>  /* Format skb and descriptor buffer for Ethernet AVB */
> -static void ravb_ring_format(struct net_device *ndev, int q)
> +static void ravb_ring_format_ex(struct net_device *ndev, int q)

   Again, what the _ex suffix mean?

[..]
> @@ -396,7 +414,7 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>  }
>  
>  /* E-MAC init function */
> -static void ravb_emac_init(struct net_device *ndev)
> +static void ravb_emac_init_ex(struct net_device *ndev)

   Same question for the 3rd time... :-)

[...]
> @@ -422,29 +440,15 @@ static void ravb_emac_init(struct net_device *ndev)
>  	ravb_write(ndev, ECSIPR_ICDIP | ECSIPR_MPDIP | ECSIPR_LCHNGIP, ECSIPR);
>  }
>  
> +static void ravb_emac_init(struct net_device *ndev)
> +{
> +	ravb_emac_init_ex(ndev);

   Hm, looks pretty useless...

> +}
> +
>  /* Device init function for Ethernet AVB */
> -static int ravb_dmac_init(struct net_device *ndev)
> +static void ravb_dmac_init_ex(struct net_device *ndev)

   4th _ex...

[...]
> @@ -532,7 +561,7 @@ static void ravb_rx_csum(struct sk_buff *skb)
>  }
>  
>  /* Packet receive function for Ethernet AVB */
> -static bool ravb_rx(struct net_device *ndev, int *quota, int q)
> +static bool ravb_rx_ex(struct net_device *ndev, int *quota, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> @@ -647,6 +676,11 @@ static bool ravb_rx(struct net_device *ndev, int *quota, int q)
>  	return boguscnt <= 0;
>  }
>  
> +static bool ravb_rx(struct net_device *ndev, int *quota, int q)
> +{
> +	return ravb_rx_ex(ndev, quota, q);
> +}
> +

   Looks pretty useless...

[...]
> @@ -920,7 +954,7 @@ static int ravb_poll(struct napi_struct *napi, int budget)
>  	if (ravb_rx(ndev, &quota, q))
>  		goto out;
>  
> -	/* Processing RX Descriptor Ring */
> +	/* Processing TX Descriptor Ring */

    Hm, looka like a missing comment fix from the patch refactoring ravb_poll()...

[...]
> @@ -2059,17 +2094,22 @@ static int ravb_probe(struct platform_device *pdev)
>  	if (!ndev)
>  		return -ENOMEM;
>  
> +	/* The Ether-specific entries in the device structure. */
> +	ndev->base_addr = res->start;
> +
> +	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
> +
> +	SET_NETDEV_DEV(ndev, &pdev->dev);
> +
> +	priv = netdev_priv(ndev);
> +	priv->chip_id = chip_id;
> +
>  	ndev->features = NETIF_F_RXCSUM;
>  	ndev->hw_features = NETIF_F_RXCSUM;
>  
>  	pm_runtime_enable(&pdev->dev);
>  	pm_runtime_get_sync(&pdev->dev);
>  
> -	/* The Ether-specific entries in the device structure. */
> -	ndev->base_addr = res->start;
> -
> -	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
> -

   What does that hunk achieve? 

>  	if (chip_id == RCAR_GEN3)
>  		irq = platform_get_irq_byname(pdev, "ch22");
>  	else
[...]
> @@ -2257,7 +2292,7 @@ static int ravb_remove(struct platform_device *pdev)
>  	struct ravb_private *priv = netdev_priv(ndev);
>  
>  	/* Stop PTP Clock driver */
> -	if (priv->chip_id != RCAR_GEN2)
> +	if (priv->chip_id == RCAR_GEN3)
>  		ravb_ptp_stop(ndev);
>  
>  	clk_disable_unprepare(priv->refclk);
> @@ -2362,7 +2397,7 @@ static int __maybe_unused ravb_resume(struct device *dev)
>  	/* Request GTI loading */
>  	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
>  
> -	if (priv->chip_id != RCAR_GEN2)
> +	if (priv->chip_id == RCAR_GEN3)
>  		ravb_set_delay_mode(ndev);

   Probably, all those chip_id check fixes deserve a patch of their own...

MBR, Sergei

RE: [PATCH/RFC 1/2] ravb: Preparation for supporting Gigabit Ethernet driver

[Biju Das]

Hello Sergei Shtylyov,

Thanks for the feedback.

> Subject: Re: [PATCH/RFC 1/2] ravb: Preparation for supporting Gigabit
> Ethernet driver
> 
> Hello!
> 
> On 7/14/21 5:54 PM, Biju Das wrote:
> 
> > The DMAC and EMAC blocks of Gigabit Ethernet IP is almost similar to
> > Ethernet AVB. With few canges in driver we can support both the IP.
> > This patch is in preparation for supporting the same.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > b/drivers/net/ethernet/renesas/ravb.h
> > index 86a1eb0634e8..80e62ca2e3d3 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -864,7 +864,7 @@ enum GECMR_BIT {
> >
> >  /* The Ethernet AVB descriptor definitions. */  struct ravb_desc {
> > -	__le16 ds;		/* Descriptor size */
> > +	__le16 ds;	/* Descriptor size */
> 
>     Oops! But this should be a matter of the seperate patch if you want to
> fix whitespace...

OK. Will send as separate patch.

> 
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 4afff320dfd0..7e6feda59f4a 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -217,6 +217,29 @@ static int ravb_tx_free(struct net_device *ndev,
> > int q, bool free_txed_only)  }
> >
> >  /* Free skb's and DMA buffers for Ethernet AVB */
> > +static void ravb_ring_free_ex(struct net_device *ndev, int q)
> 
>    What does _ex() suffix mean (seems rather poor chice)? Perhaps rx would
> be better?

The rationale behind suffix "_ex" is "extend" the existing function to support both AVB and Gigabit ethernet IP.

> 
> > +{
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +	int ring_size;
> > +	int i;
> > +
> > +	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> > +		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
> > +
> > +		if (!dma_mapping_error(ndev->dev.parent,
> > +				       le32_to_cpu(desc->dptr)))
> > +			dma_unmap_single(ndev->dev.parent,
> > +					 le32_to_cpu(desc->dptr),
> > +					 RX_BUF_SZ,
> > +					 DMA_FROM_DEVICE);
> > +	}
> > +	ring_size = sizeof(struct ravb_ex_rx_desc) *
> > +		    (priv->num_rx_ring[q] + 1);
> > +	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
> > +			  priv->rx_desc_dma[q]);
> > +	priv->rx_ring[q] = NULL;
> > +}
> > +
> >  static void ravb_ring_free(struct net_device *ndev, int q)  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> [...]
> > @@ -272,26 +281,15 @@ static void ravb_ring_free(struct net_device
> > *ndev, int q)  }
> >
> >  /* Format skb and descriptor buffer for Ethernet AVB */ -static void
> > ravb_ring_format(struct net_device *ndev, int q)
> > +static void ravb_ring_format_ex(struct net_device *ndev, int q)
> 
>    Again, what the _ex suffix mean?
> 
> [..]
> > @@ -396,7 +414,7 @@ static int ravb_ring_init(struct net_device *ndev,
> > int q)  }
> >
> >  /* E-MAC init function */
> > -static void ravb_emac_init(struct net_device *ndev)
> > +static void ravb_emac_init_ex(struct net_device *ndev)
> 
>    Same question for the 3rd time... :-)
> 
> [...]
> > @@ -422,29 +440,15 @@ static void ravb_emac_init(struct net_device
> *ndev)
> >  	ravb_write(ndev, ECSIPR_ICDIP | ECSIPR_MPDIP | ECSIPR_LCHNGIP,
> > ECSIPR);  }
> >
> > +static void ravb_emac_init(struct net_device *ndev) {
> > +	ravb_emac_init_ex(ndev);
> 
>    Hm, looks pretty useless...

Will make this as a separate single factorisation patch supporting both IP's. So that
Reviewing will be easier.

> 
> > +}
> > +
> >  /* Device init function for Ethernet AVB */ -static int
> > ravb_dmac_init(struct net_device *ndev)
> > +static void ravb_dmac_init_ex(struct net_device *ndev)
> 
>    4th _ex...
> 
> [...]
> > @@ -532,7 +561,7 @@ static void ravb_rx_csum(struct sk_buff *skb)  }
> >
> >  /* Packet receive function for Ethernet AVB */ -static bool
> > ravb_rx(struct net_device *ndev, int *quota, int q)
> > +static bool ravb_rx_ex(struct net_device *ndev, int *quota, int q)
> >  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> >  	int entry = priv->cur_rx[q] % priv->num_rx_ring[q]; @@ -647,6
> > +676,11 @@ static bool ravb_rx(struct net_device *ndev, int *quota, int
> q)
> >  	return boguscnt <= 0;
> >  }
> >
> > +static bool ravb_rx(struct net_device *ndev, int *quota, int q) {
> > +	return ravb_rx_ex(ndev, quota, q);
> > +}
> > +
> 
>    Looks pretty useless...

OK. Will make this as a separate single factorisation patch supporting both IP's. So that
Reviewing will be easier.

> 
> [...]
> > @@ -920,7 +954,7 @@ static int ravb_poll(struct napi_struct *napi, int
> budget)
> >  	if (ravb_rx(ndev, &quota, q))
> >  		goto out;
> >
> > -	/* Processing RX Descriptor Ring */
> > +	/* Processing TX Descriptor Ring */
> 
>     Hm, looka like a missing comment fix from the patch refactoring
> ravb_poll()...

OK. Will send this as separate patch.

> 
> [...]
> > @@ -2059,17 +2094,22 @@ static int ravb_probe(struct platform_device
> *pdev)
> >  	if (!ndev)
> >  		return -ENOMEM;
> >
> > +	/* The Ether-specific entries in the device structure. */
> > +	ndev->base_addr = res->start;
> > +
> > +	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
> > +
> > +	SET_NETDEV_DEV(ndev, &pdev->dev);
> > +
> > +	priv = netdev_priv(ndev);
> > +	priv->chip_id = chip_id;
> > +
> >  	ndev->features = NETIF_F_RXCSUM;
> >  	ndev->hw_features = NETIF_F_RXCSUM;
> >
> >  	pm_runtime_enable(&pdev->dev);
> >  	pm_runtime_get_sync(&pdev->dev);
> >
> > -	/* The Ether-specific entries in the device structure. */
> > -	ndev->base_addr = res->start;
> > -
> > -	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
> > -
> 
>    What does that hunk achieve?

RZ/G2L ethernet needs to de-assert the reset(in case bootloader didn't dessert). In that case
Clocks need to be turned on after that.

> 
> >  	if (chip_id == RCAR_GEN3)
> >  		irq = platform_get_irq_byname(pdev, "ch22");
> >  	else
> [...]
> > @@ -2257,7 +2292,7 @@ static int ravb_remove(struct platform_device
> *pdev)
> >  	struct ravb_private *priv = netdev_priv(ndev);
> >
> >  	/* Stop PTP Clock driver */
> > -	if (priv->chip_id != RCAR_GEN2)
> > +	if (priv->chip_id == RCAR_GEN3)
> >  		ravb_ptp_stop(ndev);
> >
> >  	clk_disable_unprepare(priv->refclk);
> > @@ -2362,7 +2397,7 @@ static int __maybe_unused ravb_resume(struct
> device *dev)
> >  	/* Request GTI loading */
> >  	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> >
> > -	if (priv->chip_id != RCAR_GEN2)
> > +	if (priv->chip_id == RCAR_GEN3)
> >  		ravb_set_delay_mode(ndev);
> 
>    Probably, all those chip_id check fixes deserve a patch of their own...

OK. Agreed, since the delays are specifics to GEN3.

Regards,
Biju

> 
> MBR, Sergei

[PATCH/RFC 2/2] ravb: Add GbEthernet driver support

[Biju Das]

Add Gigabit Ethernet driver support.

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

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  92 ++-
 drivers/net/ethernet/renesas/ravb_main.c | 683 ++++++++++++++++++++---
 2 files changed, 682 insertions(+), 93 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 80e62ca2e3d3..4e65683fb458 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -81,6 +81,7 @@ enum ravb_reg {
 	RQC3	= 0x00A0,
 	RQC4	= 0x00A4,
 	RPC	= 0x00B0,
+	RTC	= 0x00B4,	/* RZ/G2L only */
 	UFCW	= 0x00BC,
 	UFCS	= 0x00C0,
 	UFCV0	= 0x00C4,
@@ -156,6 +157,7 @@ enum ravb_reg {
 	TIS	= 0x037C,
 	ISS	= 0x0380,
 	CIE	= 0x0384,	/* R-Car Gen3 only */
+	RIC3	= 0x0388,	/* RZ/G2L only */
 	GCCR	= 0x0390,
 	GMTT	= 0x0394,
 	GPTC	= 0x0398,
@@ -181,25 +183,39 @@ enum ravb_reg {
 
 	/* E-MAC registers */
 	ECMR	= 0x0500,
+	CXR20	= 0x0500,	/* Documented for RZ/G2L only */
 	RFLR	= 0x0508,
+	CXR2A	= 0x0508,	/* Documented for RZ/G2L only */
 	ECSR	= 0x0510,
 	ECSIPR	= 0x0518,
 	PIR	= 0x0520,
 	PSR	= 0x0528,
 	PIPR	= 0x052c,
+	CXR31	= 0x0530,	/* Documented for RZ/G2L only */
+	CXR35	= 0x0540,	/* Documented for RZ/G2L only */
 	MPR	= 0x0558,
 	PFTCR	= 0x055c,
 	PFRCR	= 0x0560,
 	GECMR	= 0x05b0,
 	MAHR	= 0x05c0,
 	MALR	= 0x05c8,
-	TROCR	= 0x0700,	/* R-Car Gen3 only */
+	TROCR	= 0x0700,	/* R-Car Gen3 and RZ/G2L only */
+	CDCR	= 0x0708,	/* Documented for RZ/G2L only */
+	LCCR	= 0x0710,	/* Documented for RZ/G2L only */
 	CEFCR	= 0x0740,
 	FRECR	= 0x0748,
 	TSFRCR	= 0x0750,
 	TLFRCR	= 0x0758,
 	RFCR	= 0x0760,
+	CERCR	= 0x0768,	/* Documented for RZ/G2L only */
+	CEECR	= 0x0770,	/* Documented for RZ/G2L only */
 	MAFCR	= 0x0778,
+	LPTXMOD2 = 0x07B4,	/* Documented for RZ/G2L only */
+	LPTXGTH1 = 0x07C0,	/* Documented for RZ/G2L only */
+	LPTXMTH1 = 0x07D0,	/* Documented for RZ/G2L only */
+	CSR0     = 0x0800,	/* Documented for RZ/G2L only */
+	CSR1     = 0x0804,	/* Documented for RZ/G2L only */
+	CSR2     = 0x0808,	/* Documented for RZ/G2L only */
 };
 
 
@@ -216,6 +232,7 @@ enum CCC_BIT {
 	CCC_CSEL_HPB	= 0x00010000,
 	CCC_CSEL_ETH_TX	= 0x00020000,
 	CCC_CSEL_GMII_REF = 0x00030000,
+	CCC_BOC		= 0x00100000,	/* Documented for RZ/G2L only */
 	CCC_LBME	= 0x01000000,
 };
 
@@ -804,16 +821,21 @@ enum TID_BIT {
 enum ECMR_BIT {
 	ECMR_PRM	= 0x00000001,
 	ECMR_DM		= 0x00000002,
+	CXR20_LPM	= 0x00000010,	/* Documented for RZ/G2L only */
 	ECMR_TE		= 0x00000020,
 	ECMR_RE		= 0x00000040,
 	ECMR_MPDE	= 0x00000200,
+	CXR20_CER	= 0x00001000,	/* Documented for RZ/G2L only */
 	ECMR_TXF	= 0x00010000,	/* Documented for R-Car Gen3 only */
 	ECMR_RXF	= 0x00020000,
 	ECMR_PFR	= 0x00040000,
 	ECMR_ZPF	= 0x00080000,	/* Documented for R-Car Gen3 only */
 	ECMR_RZPF	= 0x00100000,
 	ECMR_DPAD	= 0x00200000,
+	CXR20_CXSER	= 0x00400000,	/* Documented for RZ/G2L only */
 	ECMR_RCSC	= 0x00800000,
+	CXR20_TCPT	= 0x01000000,	/* Documented for RZ/G2L only */
+	CXR20_RCPT	= 0x02000000,	/* Documented for RZ/G2L only */
 	ECMR_TRCCM	= 0x04000000,
 };
 
@@ -823,6 +845,7 @@ enum ECSR_BIT {
 	ECSR_MPD	= 0x00000002,
 	ECSR_LCHNG	= 0x00000004,
 	ECSR_PHYI	= 0x00000008,
+	ECSR_RFRI	= 0x00000010,	/* Documented for RZ/G2L only */
 };
 
 /* ECSIPR */
@@ -862,6 +885,14 @@ enum GECMR_BIT {
 	GECMR_SPEED_1000 = 0x00000001,
 };
 
+/* GECMR */
+enum RGETH_GECMR_BIT {
+	RGETH_GECMR_SPEED	= 0x00000030,
+	RGETH_GECMR_SPEED_10	= 0x00000000,
+	RGETH_GECMR_SPEED_100	= 0x00000010,
+	RGETH_GECMR_SPEED_1000	= 0x00000020,
+};
+
 /* The Ethernet AVB descriptor definitions. */
 struct ravb_desc {
 	__le16 ds;	/* Descriptor size */
@@ -949,6 +980,54 @@ enum RAVB_QUEUE {
 	RAVB_NC,	/* Network Control Queue */
 };
 
+enum CXR31_BIT {
+	CXR31_SEL_LINK0	= 0x00000001,
+	CXR31_SEL_LINK1	= 0x00000008,
+};
+
+enum CXR35_BIT {
+	CXR35_SEL_MODIN	= 0x00000100,
+};
+
+enum CSR0_BIT {
+	CSR0_CCM	= 0x00000001,
+	CSR0_TPE	= 0x00000010,
+	CSR0_RPE	= 0x00000020,
+	CSR0_TBP	= 0x00000100,
+	CSR0_RBP	= 0x00000200,
+	CSR0_FIFOCAP	= 0x00003000,
+};
+
+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
@@ -956,8 +1035,11 @@ enum RAVB_QUEUE {
 
 #define RX_BUF_SZ	(2048 - ETH_FCS_LEN + sizeof(__sum16))
 
+#define RGETH_RCV_BUFF_MAX 8192
+#define RGETH_RCV_DESCRIPTOR_DATA_SIZE 4080
+
 /* TX descriptors per packet */
-#define NUM_TX_DESC_GEN2	2
+#define NUM_TX_DESC_GEN2	2	/* RCar Gen2 or RZ/G2L */
 #define NUM_TX_DESC_GEN3	1
 
 struct ravb_tstamp_skb {
@@ -986,6 +1068,7 @@ struct ravb_ptp {
 enum ravb_chip_id {
 	RCAR_GEN2,
 	RCAR_GEN3,
+	RZ_G2L,
 };
 
 struct ravb_private {
@@ -1040,6 +1123,11 @@ struct ravb_private {
 	unsigned txcidm:1;		/* TX Clock Internal Delay Mode */
 	unsigned rgmii_override:1;	/* Deprecated rgmii-*id behavior */
 	int num_tx_desc;		/* TX descriptors per packet */
+
+	int duplex;
+	struct ravb_rx_desc *rgeth_rx_ring[NUM_RX_QUEUE];
+	struct sk_buff *rxtop_skb;
+	struct reset_control *rstc;
 };
 
 static inline u32 ravb_read(struct net_device *ndev, enum ravb_reg reg)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 7e6feda59f4a..e28a63de553d 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -29,6 +29,7 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/sys_soc.h>
+#include <linux/reset.h>
 
 #include <asm/div64.h>
 
@@ -82,6 +83,23 @@ static int ravb_config(struct net_device *ndev)
 	return error;
 }
 
+static void rgeth_set_rate(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	switch (priv->speed) {
+	case 10:                /* 10BASE */
+		ravb_write(ndev, RGETH_GECMR_SPEED_10, GECMR);
+		break;
+	case 100:               /* 100BASE */
+		ravb_write(ndev, RGETH_GECMR_SPEED_100, GECMR);
+		break;
+	case 1000:              /* 1000BASE */
+		ravb_write(ndev, RGETH_GECMR_SPEED_1000, GECMR);
+		break;
+	}
+}
+
 static void ravb_set_rate(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -217,6 +235,29 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
 }
 
 /* Free skb's and DMA buffers for Ethernet AVB */
+static void rgeth_ring_free_ex(struct net_device *ndev, int q)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int ring_size;
+	int i;
+
+	for (i = 0; i < priv->num_rx_ring[q]; i++) {
+		struct ravb_rx_desc *desc = &priv->rgeth_rx_ring[q][i];
+
+		if (!dma_mapping_error(ndev->dev.parent,
+				       le32_to_cpu(desc->dptr)))
+			dma_unmap_single(ndev->dev.parent,
+					 le32_to_cpu(desc->dptr),
+					 RGETH_RCV_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->rgeth_rx_ring[q],
+			  priv->rx_desc_dma[q]);
+	priv->rgeth_rx_ring[q] = NULL;
+}
+
 static void ravb_ring_free_ex(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -247,8 +288,12 @@ static void ravb_ring_free(struct net_device *ndev, int q)
 	int ring_size;
 	int i;
 
-	if (priv->rx_ring[q]) {
-		ravb_ring_free_ex(ndev, q);
+	if (priv->chip_id == RZ_G2L) {
+		if (priv->rgeth_rx_ring[q])
+			rgeth_ring_free_ex(ndev, q);
+	} else {
+		if (priv->rx_ring[q])
+			ravb_ring_free_ex(ndev, q);
 	}
 
 	if (priv->tx_ring[q]) {
@@ -281,6 +326,36 @@ static void ravb_ring_free(struct net_device *ndev, int q)
 }
 
 /* Format skb and descriptor buffer for Ethernet AVB */
+static void rgeth_ring_format_ex(struct net_device *ndev, int q)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	struct ravb_rx_desc *rx_desc;
+	int rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
+	dma_addr_t dma_addr;
+	int i;
+
+	memset(priv->rgeth_rx_ring[q], 0, rx_ring_size);
+	/* Build RX ring buffer */
+	for (i = 0; i < priv->num_rx_ring[q]; i++) {
+		/* RX descriptor */
+		rx_desc = &priv->rgeth_rx_ring[q][i];
+		rx_desc->ds_cc = cpu_to_le16(RGETH_RCV_DESCRIPTOR_DATA_SIZE);
+		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
+					  RGETH_RCV_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->rgeth_rx_ring[q][i];
+	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
+	rx_desc->die_dt = DT_LINKFIX; /* type */
+}
+
 static void ravb_ring_format_ex(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -326,7 +401,10 @@ static void ravb_ring_format(struct net_device *ndev, int q)
 	priv->dirty_tx[q] = 0;
 
 	/* Build RX ring buffer */
-	ravb_ring_format_ex(ndev, q);
+	if (priv->chip_id == RZ_G2L)
+		rgeth_ring_format_ex(ndev, q);
+	else
+		ravb_ring_format_ex(ndev, q);
 
 	memset(priv->tx_ring[q], 0, tx_ring_size);
 	/* Build TX ring buffer */
@@ -356,7 +434,7 @@ static void ravb_ring_format(struct net_device *ndev, int q)
 static int ravb_ring_init(struct net_device *ndev, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	size_t skb_sz = RX_BUF_SZ;
+	size_t skb_sz = (priv->chip_id == RZ_G2L) ? RGETH_RCV_BUFF_MAX : RX_BUF_SZ;
 	int num_tx_desc = priv->num_tx_desc;
 	struct sk_buff *skb;
 	int ring_size;
@@ -387,12 +465,23 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 	}
 
 	/* Allocate all RX descriptors. */
-	ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] + 1);
-	priv->rx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
-					      &priv->rx_desc_dma[q],
-					      GFP_KERNEL);
-	if (!priv->rx_ring[q])
-		goto error;
+	if (priv->chip_id == RZ_G2L) {
+		ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
+
+		priv->rgeth_rx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
+							    &priv->rx_desc_dma[q],
+							    GFP_KERNEL);
+		if (!priv->rgeth_rx_ring[q])
+			goto error;
+	} else {
+		ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] + 1);
+
+		priv->rx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
+						      &priv->rx_desc_dma[q],
+						      GFP_KERNEL);
+		if (!priv->rx_ring[q])
+			goto error;
+	}
 
 	priv->dirty_rx[q] = 0;
 
@@ -414,6 +503,45 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 }
 
 /* E-MAC init function */
+static void rgeth_emac_init_ex(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	/* Receive frame limit set register */
+	ravb_write(ndev, RGETH_RCV_BUFF_MAX + ETH_FCS_LEN, RFLR);
+
+	/* PAUSE prohibition */
+	ravb_write(ndev, ECMR_ZPF | (priv->duplex ? ECMR_DM : 0) |
+			 ECMR_TE | ECMR_RE | CXR20_RCPT |
+			 ECMR_TXF | ECMR_RXF | ECMR_PRM, ECMR);
+
+	rgeth_set_rate(ndev);
+
+	/* Set MAC address */
+	ravb_write(ndev,
+		   (ndev->dev_addr[0] << 24) | (ndev->dev_addr[1] << 16) |
+		   (ndev->dev_addr[2] << 8)  | (ndev->dev_addr[3]), MAHR);
+	ravb_write(ndev,
+		   (ndev->dev_addr[4] << 8)  | (ndev->dev_addr[5]), MALR);
+
+	/* E-MAC status register clear */
+	ravb_write(ndev, ECSR_ICD | ECSR_LCHNG | ECSR_RFRI, ECSR);
+	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);
+
+	/* E-MAC interrupt enable register */
+	ravb_write(ndev, ECSIPR_ICDIP, ECSIPR);
+
+	ravb_write(ndev, ravb_read(ndev, CXR31)
+			 & ~CXR31_SEL_LINK1, CXR31);
+	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID) {
+		ravb_write(ndev, ravb_read(ndev, CXR31)
+			 | CXR31_SEL_LINK0, CXR31);
+	} else {
+		ravb_write(ndev, ravb_read(ndev, CXR31)
+			 & ~CXR31_SEL_LINK0, CXR31);
+	}
+}
+
 static void ravb_emac_init_ex(struct net_device *ndev)
 {
 	/* Receive frame limit set register */
@@ -442,10 +570,41 @@ static void ravb_emac_init_ex(struct net_device *ndev)
 
 static void ravb_emac_init(struct net_device *ndev)
 {
-	ravb_emac_init_ex(ndev);
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	if (priv->chip_id == RZ_G2L)
+		rgeth_emac_init_ex(ndev);
+	else
+		ravb_emac_init_ex(ndev);
+
 }
 
 /* Device init function for Ethernet AVB */
+static void rgeth_dmac_init_ex(struct net_device *ndev)
+{
+	/* Set AVB RX */
+	ravb_write(ndev, 0x60000000, RCR);
+
+	/* Set Max Frame Length (RTC) */
+	ravb_write(ndev, 0x7ffc0000 | RGETH_RCV_BUFF_MAX, RTC);
+
+	/* Set FIFO size */
+	ravb_write(ndev, 0x00222200, TGC);
+
+	ravb_write(ndev, 0, TCCR);
+
+	/* Frame receive */
+	ravb_write(ndev, RIC0_FRE0, RIC0);
+	/* Disable FIFO full warning */
+	ravb_write(ndev, 0x0, RIC1);
+	/* Receive FIFO full error, descriptor empty */
+	ravb_write(ndev, RIC2_QFE0 | RIC2_RFFE, RIC2);
+
+	ravb_write(ndev, 0x0, RIC3);
+
+	ravb_write(ndev, TIC_FTE0, TIC);
+}
+
 static void ravb_dmac_init_ex(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -479,6 +638,7 @@ static void ravb_dmac_init_ex(struct net_device *ndev)
 
 static int ravb_dmac_init(struct net_device *ndev)
 {
+	struct ravb_private *priv = netdev_priv(ndev);
 	int error;
 
 	/* Set CONFIG mode */
@@ -499,7 +659,10 @@ static int ravb_dmac_init(struct net_device *ndev)
 	ravb_ring_format(ndev, RAVB_BE);
 	ravb_ring_format(ndev, RAVB_NC);
 
-	ravb_dmac_init_ex(ndev);
+	if (priv->chip_id == RZ_G2L)
+		rgeth_dmac_init_ex(ndev);
+	else
+		ravb_dmac_init_ex(ndev);
 
 	/* Setting the control will start the AVB-DMAC process. */
 	ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_OPERATION);
@@ -545,6 +708,23 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
 	}
 }
 
+static void rgeth_rx_csum(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;
@@ -561,6 +741,143 @@ static void ravb_rx_csum(struct sk_buff *skb)
 }
 
 /* Packet receive function for Ethernet AVB */
+struct sk_buff *rgeth_get_skb(struct net_device *ndev,  int q, int entry,
+			      struct ravb_rx_desc *desc)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	struct sk_buff *skb;
+
+	skb = priv->rx_skb[q][entry];
+	priv->rx_skb[q][entry] = NULL;
+	dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
+			 ALIGN(RGETH_RCV_BUFF_MAX, 16), DMA_FROM_DEVICE);
+
+	return skb;
+}
+
+static bool rgeth_rx_ex(struct net_device *ndev, int *quota, int q)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
+	int boguscnt = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
+	struct net_device_stats *stats = &priv->stats[q];
+	struct ravb_rx_desc *desc;
+	struct sk_buff *skb;
+	dma_addr_t dma_addr;
+	u8  desc_status;
+	u8  die_dt;
+	u16 pkt_len;
+	int limit;
+
+	boguscnt = min(boguscnt, *quota);
+	limit = boguscnt;
+	desc = &priv->rgeth_rx_ring[q][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;
+			if (die_dt == DT_FSINGLE) {
+				skb = rgeth_get_skb(ndev, q, entry, desc);
+				skb_put(skb, pkt_len);
+				skb->protocol = eth_type_trans(skb, ndev);
+				if (ndev->features & NETIF_F_RXCSUM)
+					rgeth_rx_csum(skb);
+				napi_gro_receive(&priv->napi[q], skb);
+				stats->rx_packets++;
+				stats->rx_bytes += pkt_len;
+			} else if (die_dt == DT_FSTART) {
+				priv->rxtop_skb = rgeth_get_skb(ndev, q, entry, desc);
+				skb_put(priv->rxtop_skb, pkt_len);
+			} else if (die_dt == DT_FMID) {
+				skb = rgeth_get_skb(ndev, q, entry, desc);
+				skb_copy_to_linear_data_offset(priv->rxtop_skb,
+							       priv->rxtop_skb->len,
+							       skb->data,
+							       pkt_len);
+				skb_put(priv->rxtop_skb, pkt_len);
+				dev_kfree_skb(skb);
+			} else if (die_dt == DT_FEND) {
+				skb = rgeth_get_skb(ndev, q, entry, desc);
+				skb_copy_to_linear_data_offset(priv->rxtop_skb,
+							       priv->rxtop_skb->len,
+							       skb->data,
+							       pkt_len);
+				skb_put(priv->rxtop_skb, pkt_len);
+				dev_kfree_skb(skb);
+				priv->rxtop_skb->protocol =
+					eth_type_trans(priv->rxtop_skb, ndev);
+				if (ndev->features & NETIF_F_RXCSUM)
+					rgeth_rx_csum(skb);
+				napi_gro_receive(&priv->napi[q],
+						 priv->rxtop_skb);
+				stats->rx_packets++;
+				stats->rx_bytes += priv->rxtop_skb->len;
+			}
+		}
+
+		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
+		desc = &priv->rgeth_rx_ring[q][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->rgeth_rx_ring[q][entry];
+		desc->ds_cc = cpu_to_le16(RGETH_RCV_DESCRIPTOR_DATA_SIZE);
+
+		if (!priv->rx_skb[q][entry]) {
+			skb = netdev_alloc_skb(ndev,
+					       RGETH_RCV_BUFF_MAX + RAVB_ALIGN - 1);
+			if (!skb)
+				break;  /* Better luck next round. */
+			ravb_set_buffer_align(skb);
+			dma_addr = dma_map_single(ndev->dev.parent,
+						  skb->data,
+						  le16_to_cpu(RGETH_RCV_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;
+}
+
 static bool ravb_rx_ex(struct net_device *ndev, int *quota, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -678,7 +995,10 @@ static bool ravb_rx_ex(struct net_device *ndev, int *quota, int q)
 
 static bool ravb_rx(struct net_device *ndev, int *quota, int q)
 {
-	return ravb_rx_ex(ndev, quota, q);
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	return (priv->chip_id == RZ_G2L) ?
+		rgeth_rx_ex(ndev, quota, q) : ravb_rx_ex(ndev, quota, q);
 }
 
 static void ravb_rcv_snd_disable(struct net_device *ndev)
@@ -696,11 +1016,15 @@ static void ravb_rcv_snd_enable(struct net_device *ndev)
 /* function for waiting dma process finished */
 static int ravb_stop_dma(struct net_device *ndev)
 {
+	struct ravb_private *priv = netdev_priv(ndev);
 	int error;
 
 	/* Wait for stopping the hardware TX process */
-	error = ravb_wait(ndev, TCCR,
-			  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0);
+	if (priv->chip_id == RZ_G2L)
+		error = ravb_wait(ndev, TCCR, TCCR_TSRQ0, 0);
+	else
+		error = ravb_wait(ndev, TCCR,
+				  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0);
 	if (error)
 		return error;
 
@@ -730,7 +1054,7 @@ static void ravb_emac_interrupt_unlocked(struct net_device *ndev)
 	ecsr = ravb_read(ndev, ECSR);
 	ravb_write(ndev, ecsr, ECSR);	/* clear interrupt */
 
-	if (ecsr & ECSR_MPD)
+	if (priv->chip_id != RZ_G2L && (ecsr & ECSR_MPD))
 		pm_wakeup_event(&priv->pdev->dev, 0);
 	if (ecsr & ECSR_ICD)
 		ndev->stats.tx_carrier_errors++;
@@ -823,11 +1147,13 @@ static bool ravb_queue_interrupt(struct net_device *ndev, int q)
 
 static bool ravb_timestamp_interrupt(struct net_device *ndev)
 {
+	struct ravb_private *priv = netdev_priv(ndev);
 	u32 tis = ravb_read(ndev, TIS);
 
 	if (tis & TIS_TFUF) {
 		ravb_write(ndev, ~(TIS_TFUF | TIS_RESERVED), TIS);
-		ravb_get_tx_tstamp(ndev);
+		if (priv->chip_id != RZ_G2L)
+			ravb_get_tx_tstamp(ndev);
 		return true;
 	}
 	return false;
@@ -872,7 +1198,7 @@ static irqreturn_t ravb_interrupt(int irq, void *dev_id)
 	}
 
 	/* gPTP interrupt status summary */
-	if (iss & ISS_CGIS) {
+	if (priv->chip_id != RZ_G2L && (iss & ISS_CGIS)) {
 		ravb_ptp_interrupt(ndev);
 		result = IRQ_HANDLED;
 	}
@@ -947,12 +1273,20 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 	int q = napi - priv->napi;
 	int mask = BIT(q);
 	int quota = budget;
+	int entry;
+	struct ravb_rx_desc *desc;
 
+	if (priv->chip_id == RZ_G2L) {
+		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
+		desc = &priv->rgeth_rx_ring[q][entry];
+	}
 	/* Processing RX Descriptor Ring */
 	/* Clear RX interrupt */
 	ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
-	if (ravb_rx(ndev, &quota, q))
-		goto out;
+	if (priv->chip_id != RZ_G2L || desc->die_dt != DT_FEMPTY) {
+		if (ravb_rx(ndev, &quota, q))
+			goto out;
+	}
 
 	/* Processing TX Descriptor Ring */
 	spin_lock_irqsave(&priv->lock, flags);
@@ -986,6 +1320,18 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 	return budget - quota;
 }
 
+static void rgeth_set_duplex(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	u32 ecmr = ravb_read(ndev, ECMR);
+
+	if (priv->duplex)       /* Full */
+		ecmr |=  ECMR_DM;
+	else                    /* Half */
+		ecmr &= ~ECMR_DM;
+	ravb_write(ndev, ecmr, ECMR);
+}
+
 /* PHY state control function */
 static void ravb_adjust_link(struct net_device *ndev)
 {
@@ -1001,10 +1347,19 @@ static void ravb_adjust_link(struct net_device *ndev)
 		ravb_rcv_snd_disable(ndev);
 
 	if (phydev->link) {
+		if (priv->chip_id == RZ_G2L && phydev->duplex != priv->duplex) {
+			new_state = true;
+			priv->duplex = phydev->duplex;
+			rgeth_set_duplex(ndev);
+		}
+
 		if (phydev->speed != priv->speed) {
 			new_state = true;
 			priv->speed = phydev->speed;
-			ravb_set_rate(ndev);
+			if (priv->chip_id == RZ_G2L)
+				rgeth_set_rate(ndev);
+			else
+				ravb_set_rate(ndev);
 		}
 		if (!priv->link) {
 			ravb_modify(ndev, ECMR, ECMR_TXF, 0);
@@ -1015,6 +1370,8 @@ static void ravb_adjust_link(struct net_device *ndev)
 		new_state = true;
 		priv->link = 0;
 		priv->speed = 0;
+		if (priv->chip_id == RZ_G2L)
+			priv->duplex = -1;
 	}
 
 	/* Enable TX and RX right over here, if E-MAC change is ignored */
@@ -1044,6 +1401,7 @@ static int ravb_phy_init(struct net_device *ndev)
 
 	priv->link = 0;
 	priv->speed = 0;
+	priv->duplex = -1;
 
 	/* Try connecting to PHY */
 	pn = of_parse_phandle(np, "phy-handle", 0);
@@ -1082,15 +1440,23 @@ static int ravb_phy_init(struct net_device *ndev)
 		netdev_info(ndev, "limited PHY to 100Mbit/s\n");
 	}
 
-	/* 10BASE, Pause and Asym Pause is not supported */
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
+	if (priv->chip_id == RZ_G2L) {
+		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID)
+			ravb_write(ndev, ravb_read(ndev, CXR35) | CXR35_SEL_MODIN, CXR35);
+		else if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII)
+			ravb_write(ndev, 0x3E80000, CXR35);
+	} else {
 
-	/* Half Duplex is not supported */
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
+		/* 10BASE, Pause and Asym Pause is not supported */
+		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
+		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
+		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
+		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
+
+		/* Half Duplex is not supported */
+		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
+		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
+	}
 
 	phy_attached_info(phydev);
 
@@ -1133,6 +1499,24 @@ static void ravb_set_msglevel(struct net_device *ndev, u32 value)
 	priv->msg_enable = value;
 }
 
+static const char rgeth_gstrings_stats[][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",
@@ -1168,12 +1552,15 @@ static const char ravb_gstrings_stats[][ETH_GSTRING_LEN] = {
 };
 
 #define RAVB_STATS_LEN	ARRAY_SIZE(ravb_gstrings_stats)
+#define RGETH_STATS_LEN	ARRAY_SIZE(rgeth_gstrings_stats)
 
 static int ravb_get_sset_count(struct net_device *netdev, int sset)
 {
+	struct ravb_private *priv = netdev_priv(netdev);
+
 	switch (sset) {
 	case ETH_SS_STATS:
-		return RAVB_STATS_LEN;
+		return (priv->chip_id == RZ_G2L) ? RGETH_STATS_LEN : RAVB_STATS_LEN;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -1183,7 +1570,7 @@ static void ravb_get_ethtool_stats(struct net_device *ndev,
 				   struct ethtool_stats *estats, u64 *data)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	int num_queue = NUM_RX_QUEUE;
+	int num_queue = (priv->chip_id == RZ_G2L) ? 1 : NUM_RX_QUEUE;
 	int i = 0;
 	int q;
 
@@ -1211,9 +1598,14 @@ static void ravb_get_ethtool_stats(struct net_device *ndev,
 
 static void ravb_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
 {
+	struct ravb_private *priv = netdev_priv(ndev);
+
 	switch (stringset) {
 	case ETH_SS_STATS:
-		memcpy(data, ravb_gstrings_stats, sizeof(ravb_gstrings_stats));
+		if (priv->chip_id == RZ_G2L)
+			memcpy(data, rgeth_gstrings_stats, sizeof(rgeth_gstrings_stats));
+		else
+			memcpy(data, ravb_gstrings_stats, sizeof(ravb_gstrings_stats));
 		break;
 	}
 }
@@ -1304,7 +1696,8 @@ static int ravb_get_ts_info(struct net_device *ndev,
 		(1 << HWTSTAMP_FILTER_NONE) |
 		(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
 		(1 << HWTSTAMP_FILTER_ALL);
-	info->phc_index = ptp_clock_index(priv->ptp.clock);
+	if (priv->chip_id != RZ_G2L)
+		info->phc_index = ptp_clock_index(priv->ptp.clock);
 
 	return 0;
 }
@@ -1348,6 +1741,21 @@ static const struct ethtool_ops ravb_ethtool_ops = {
 	.set_wol		= ravb_set_wol,
 };
 
+static const struct ethtool_ops rgeth_ethtool_ops = {
+	.nway_reset		= phy_ethtool_nway_reset,
+	.get_msglevel		= ravb_get_msglevel,
+	.set_msglevel		= ravb_set_msglevel,
+	.get_link		= ethtool_op_get_link,
+	.get_strings		= ravb_get_strings,
+	.get_ethtool_stats	= ravb_get_ethtool_stats,
+	.get_sset_count		= ravb_get_sset_count,
+	.get_ringparam		= ravb_get_ringparam,
+	.set_ringparam		= ravb_set_ringparam,
+	.get_ts_info		= ravb_get_ts_info,
+	.get_link_ksettings	= phy_ethtool_get_link_ksettings,
+	.set_link_ksettings	= phy_ethtool_set_link_ksettings,
+};
+
 static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
 				struct net_device *ndev, struct device *dev,
 				const char *ch)
@@ -1599,28 +2007,30 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	desc->dptr = cpu_to_le32(dma_addr);
 
 	/* TX timestamp required */
-	if (q == RAVB_NC) {
-		ts_skb = kmalloc(sizeof(*ts_skb), GFP_ATOMIC);
-		if (!ts_skb) {
-			if (num_tx_desc > 1) {
-				desc--;
-				dma_unmap_single(ndev->dev.parent, dma_addr,
-						 len, DMA_TO_DEVICE);
+	if (priv->chip_id != RZ_G2L) {
+		if (q == RAVB_NC) {
+			ts_skb = kmalloc(sizeof(*ts_skb), GFP_ATOMIC);
+			if (!ts_skb) {
+				if (num_tx_desc > 1) {
+					desc--;
+					dma_unmap_single(ndev->dev.parent, dma_addr,
+							 len, DMA_TO_DEVICE);
+				}
+				goto unmap;
 			}
-			goto unmap;
+			ts_skb->skb = skb_get(skb);
+			ts_skb->tag = priv->ts_skb_tag++;
+			priv->ts_skb_tag &= 0x3ff;
+			list_add_tail(&ts_skb->list, &priv->ts_skb_list);
+
+			/* TAG and timestamp required flag */
+			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+			desc->tagh_tsr = (ts_skb->tag >> 4) | TX_TSR;
+			desc->ds_tagl |= cpu_to_le16(ts_skb->tag << 12);
 		}
-		ts_skb->skb = skb_get(skb);
-		ts_skb->tag = priv->ts_skb_tag++;
-		priv->ts_skb_tag &= 0x3ff;
-		list_add_tail(&ts_skb->list, &priv->ts_skb_list);
 
-		/* TAG and timestamp required flag */
-		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
-		desc->tagh_tsr = (ts_skb->tag >> 4) | TX_TSR;
-		desc->ds_tagl |= cpu_to_le16(ts_skb->tag << 12);
+		skb_tx_timestamp(skb);
 	}
-
-	skb_tx_timestamp(skb);
 	/* Descriptor type must be set after all the above writes */
 	dma_wmb();
 	if (num_tx_desc > 1) {
@@ -1669,6 +2079,20 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
 	stats0 = &priv->stats[RAVB_BE];
 	stats1 = &priv->stats[RAVB_NC];
 
+	if (priv->chip_id == RZ_G2L) {
+		nstats->tx_dropped += ravb_read(ndev, TROCR);
+		ravb_write(ndev, 0, TROCR);	/* (write clear) */
+		nstats->collisions += ravb_read(ndev, CDCR);
+		ravb_write(ndev, 0, CDCR);	/* (write clear) */
+		nstats->tx_carrier_errors += ravb_read(ndev, LCCR);
+		ravb_write(ndev, 0, LCCR);	/* (write clear) */
+
+		nstats->tx_carrier_errors += ravb_read(ndev, CERCR);
+		ravb_write(ndev, 0, CERCR);	/* (write clear) */
+		nstats->tx_carrier_errors += ravb_read(ndev, CEECR);
+		ravb_write(ndev, 0, CEECR);	/* (write clear) */
+	}
+
 	if (priv->chip_id == RCAR_GEN3) {
 		nstats->tx_dropped += ravb_read(ndev, TROCR);
 		ravb_write(ndev, 0, TROCR);	/* (write clear) */
@@ -1729,10 +2153,12 @@ static int ravb_close(struct net_device *ndev)
 			   "device will be stopped after h/w processes are done.\n");
 
 	/* Clear the timestamp list */
-	list_for_each_entry_safe(ts_skb, ts_skb2, &priv->ts_skb_list, list) {
-		list_del(&ts_skb->list);
-		kfree_skb(ts_skb->skb);
-		kfree(ts_skb);
+	if (priv->chip_id != RZ_G2L) {
+		list_for_each_entry_safe(ts_skb, ts_skb2, &priv->ts_skb_list, list) {
+			list_del(&ts_skb->list);
+			kfree_skb(ts_skb->skb);
+			kfree(ts_skb);
+		}
 	}
 
 	/* PHY disconnect */
@@ -1899,6 +2325,44 @@ static int ravb_set_features(struct net_device *ndev,
 	return 0;
 }
 
+static int rgeth_set_features(struct net_device *ndev,
+			      netdev_features_t features)
+{
+	int error;
+	unsigned int reg = 0;
+	netdev_features_t changed = features ^ ndev->features;
+
+	reg = ravb_read(ndev, CSR0);
+
+	ravb_write(ndev, ravb_read(ndev, CSR0) & ~(CSR0_RPE | CSR0_TPE), CSR0);
+	error = ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0);
+	if (error) {
+		ravb_write(ndev, reg, CSR0);
+		return error;
+	}
+
+	if (changed & NETIF_F_RXCSUM) {
+		if (features & NETIF_F_RXCSUM)
+			ravb_write(ndev, CSR2_ALL, CSR2);
+		else
+			ravb_write(ndev, 0, CSR2);
+	}
+
+	if (changed & NETIF_F_HW_CSUM) {
+		if (features & NETIF_F_HW_CSUM) {
+			ravb_write(ndev, CSR1_ALL, CSR1);
+			ndev->features |= NETIF_F_CSUM_MASK;
+		} else {
+			ravb_write(ndev, 0, CSR1);
+		}
+	}
+	ravb_write(ndev, reg, CSR0);
+
+	ndev->features = features;
+
+	return 0;
+}
+
 static const struct net_device_ops ravb_netdev_ops = {
 	.ndo_open		= ravb_open,
 	.ndo_stop		= ravb_close,
@@ -1914,6 +2378,20 @@ static const struct net_device_ops ravb_netdev_ops = {
 	.ndo_set_features	= ravb_set_features,
 };
 
+static const struct net_device_ops rgeth_netdev_ops = {
+	.ndo_open               = ravb_open,
+	.ndo_stop               = ravb_close,
+	.ndo_start_xmit         = ravb_start_xmit,
+	.ndo_select_queue       = ravb_select_queue,
+	.ndo_get_stats          = ravb_get_stats,
+	.ndo_set_rx_mode        = ravb_set_rx_mode,
+	.ndo_tx_timeout         = ravb_tx_timeout,
+	.ndo_do_ioctl           = ravb_do_ioctl,
+	.ndo_validate_addr      = eth_validate_addr,
+	.ndo_set_mac_address    = eth_mac_addr,
+	.ndo_set_features       = rgeth_set_features,
+};
+
 /* MDIO bus init function */
 static int ravb_mdio_init(struct ravb_private *priv)
 {
@@ -1930,7 +2408,10 @@ static int ravb_mdio_init(struct ravb_private *priv)
 		return -ENOMEM;
 
 	/* Hook up MII support for ethtool */
-	priv->mii_bus->name = "ravb_mii";
+	if (priv->chip_id == RZ_G2L)
+		priv->mii_bus->name = "rgeth_mii";
+	else
+		priv->mii_bus->name = "ravb_mii";
 	priv->mii_bus->parent = dev;
 	snprintf(priv->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
 		 pdev->name, pdev->id);
@@ -1965,6 +2446,7 @@ static const struct of_device_id ravb_match_table[] = {
 	{ .compatible = "renesas,etheravb-rcar-gen2", .data = (void *)RCAR_GEN2 },
 	{ .compatible = "renesas,etheravb-r8a7795", .data = (void *)RCAR_GEN3 },
 	{ .compatible = "renesas,etheravb-rcar-gen3", .data = (void *)RCAR_GEN3 },
+	{ .compatible = "renesas,rzg2l-gether", .data = (void *)RZ_G2L },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ravb_match_table);
@@ -2001,7 +2483,8 @@ static void ravb_set_config_mode(struct net_device *ndev)
 	if (priv->chip_id != RCAR_GEN3) {
 		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
 		/* Set CSEL value */
-		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
+		if (priv->chip_id == RCAR_GEN2)
+			ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
 	} else {
 		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
 			    CCC_GAC | CCC_CSEL_HPB);
@@ -2082,20 +2565,11 @@ static int ravb_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	/* Get base address */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(&pdev->dev, "invalid resource\n");
-		return -EINVAL;
-	}
-
 	ndev = alloc_etherdev_mqs(sizeof(struct ravb_private),
 				  NUM_TX_QUEUE, NUM_RX_QUEUE);
 	if (!ndev)
 		return -ENOMEM;
 
-	/* The Ether-specific entries in the device structure. */
-	ndev->base_addr = res->start;
 
 	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
 
@@ -2104,11 +2578,19 @@ static int ravb_probe(struct platform_device *pdev)
 	priv = netdev_priv(ndev);
 	priv->chip_id = chip_id;
 
-	ndev->features = NETIF_F_RXCSUM;
-	ndev->hw_features = NETIF_F_RXCSUM;
-
-	pm_runtime_enable(&pdev->dev);
-	pm_runtime_get_sync(&pdev->dev);
+	if (chip_id == RZ_G2L) {
+		ndev->hw_features |= (NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
+		priv->rstc = devm_reset_control_get(&pdev->dev, NULL);
+		if (IS_ERR(priv->rstc)) {
+			dev_err(&pdev->dev, "failed to get cpg reset\n");
+			free_netdev(ndev);
+			return PTR_ERR(priv->rstc);
+		}
+		reset_control_deassert(priv->rstc);
+	} else {
+		ndev->features = NETIF_F_RXCSUM;
+		ndev->hw_features = NETIF_F_RXCSUM;
+	}
 
 	if (chip_id == RCAR_GEN3)
 		irq = platform_get_irq_byname(pdev, "ch22");
@@ -2120,18 +2602,24 @@ static int ravb_probe(struct platform_device *pdev)
 	}
 	ndev->irq = irq;
 
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
 	priv->ndev = ndev;
 	priv->pdev = pdev;
 	priv->num_tx_ring[RAVB_BE] = BE_TX_RING_SIZE;
 	priv->num_rx_ring[RAVB_BE] = BE_RX_RING_SIZE;
 	priv->num_tx_ring[RAVB_NC] = NC_TX_RING_SIZE;
 	priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
-	priv->addr = devm_ioremap_resource(&pdev->dev, res);
+	priv->addr = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
 	if (IS_ERR(priv->addr)) {
 		error = PTR_ERR(priv->addr);
 		goto out_release;
 	}
 
+	/* The Ether-specific entries in the device structure. */
+	ndev->base_addr = res->start;
+
 	spin_lock_init(&priv->lock);
 	INIT_WORK(&priv->work, ravb_tx_timeout_work);
 
@@ -2168,6 +2656,8 @@ static int ravb_probe(struct platform_device *pdev)
 		}
 	}
 
+	priv->chip_id = chip_id;
+
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		error = PTR_ERR(priv->clk);
@@ -2181,30 +2671,39 @@ 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->min_mtu = ETH_MIN_MTU;
+	if (chip_id != RZ_G2L) {
+		ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
+		ndev->min_mtu = ETH_MIN_MTU;
+	}
 
 	priv->num_tx_desc = chip_id == RCAR_GEN3 ?
 		NUM_TX_DESC_GEN3 : NUM_TX_DESC_GEN2;
 
 	/* Set function */
-	ndev->netdev_ops = &ravb_netdev_ops;
-	ndev->ethtool_ops = &ravb_ethtool_ops;
+	if  (chip_id == RZ_G2L) {
+		ndev->netdev_ops = &rgeth_netdev_ops;
+		ndev->ethtool_ops = &rgeth_ethtool_ops;
+	} else {
+		ndev->netdev_ops = &ravb_netdev_ops;
+		ndev->ethtool_ops = &ravb_ethtool_ops;
+	}
 
 	/* Set AVB config mode */
 	ravb_set_config_mode(ndev);
 
-	/* Set GTI value */
-	error = ravb_set_gti(ndev);
-	if (error)
-		goto out_disable_refclk;
+	if (priv->chip_id != RZ_G2L) {
+		/* Set GTI value */
+		error = ravb_set_gti(ndev);
+		if (error)
+			goto out_disable_refclk;
 
-	/* Request GTI loading */
-	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+		/* Request GTI loading */
+		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
 
-	if (priv->chip_id == RCAR_GEN3) {
-		ravb_parse_delay_mode(np, ndev);
-		ravb_set_delay_mode(ndev);
+		if (priv->chip_id == RCAR_GEN3) {
+			ravb_parse_delay_mode(np, ndev);
+			ravb_set_delay_mode(ndev);
+		}
 	}
 
 	/* Allocate descriptor base address table */
@@ -2389,13 +2888,15 @@ static int __maybe_unused ravb_resume(struct device *dev)
 	/* Set AVB config mode */
 	ravb_set_config_mode(ndev);
 
-	/* Set GTI value */
-	ret = ravb_set_gti(ndev);
-	if (ret)
-		return ret;
+	if (priv->chip_id != RZ_G2L) {
+		/* Set GTI value */
+		ret = ravb_set_gti(ndev);
+		if (ret)
+			return ret;
 
-	/* Request GTI loading */
-	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+		/* Request GTI loading */
+		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+	}
 
 	if (priv->chip_id == RCAR_GEN3)
 		ravb_set_delay_mode(ndev);
-- 
2.17.1

Re: [PATCH/RFC 2/2] ravb: Add GbEthernet driver support

[Andrew Lunn]

> +	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID) {
> +		ravb_write(ndev, ravb_read(ndev, CXR31)
> +			 | CXR31_SEL_LINK0, CXR31);
> +	} else {
> +		ravb_write(ndev, ravb_read(ndev, CXR31)
> +			 & ~CXR31_SEL_LINK0, CXR31);
> +	}

You need to be very careful here. What value is passed to the PHY?

There is some funky code:

       /* Fall back to legacy rgmii-*id behavior */
        if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
            priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID) {
                priv->rxcidm = 1;
                priv->rgmii_override = 1;
        }

        if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
            priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) {
                if (!WARN(soc_device_match(ravb_delay_mode_quirk_match),
                          "phy-mode %s requires TX clock internal delay mode which is not supported by this hardware revision. Please update device tree",
                          phy_modes(priv->phy_interface))) {
                        priv->txcidm = 1;
                        priv->rgmii_override = 1;
                }
        }
...

        iface = priv->rgmii_override ? PHY_INTERFACE_MODE_RGMII
                                     : priv->phy_interface;
        phydev = of_phy_connect(ndev, pn, ravb_adjust_link, 0, iface);

So it looks like, with PHY_INTERFACE_MODE_RGMII_ID,
PHY_INTERFACE_MODE_RGMII_TXID, PHY_INTERFACE_MODE_RGMII_RXID the PHY
is passed PHY_INTERFACE_MODE_RGMII, with the assumption the MAC is
adding the delay. But it looks like you are only adding a delay for
PHY_INTERFACE_MODE_RGMII_ID. So this appears wrong.

> @@ -1082,15 +1440,23 @@ static int ravb_phy_init(struct net_device *ndev)
>  		netdev_info(ndev, "limited PHY to 100Mbit/s\n");
>  	}
>  
> -	/* 10BASE, Pause and Asym Pause is not supported */
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
> +	if (priv->chip_id == RZ_G2L) {
> +		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID)
> +			ravb_write(ndev, ravb_read(ndev, CXR35) | CXR35_SEL_MODIN, CXR35);
> +		else if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII)
> +			ravb_write(ndev, 0x3E80000, CXR35);

This is not obviously correct. What about the other two RGMII modes?

> @@ -1348,6 +1741,21 @@ static const struct ethtool_ops ravb_ethtool_ops = {
>  	.set_wol		= ravb_set_wol,
>  };
>  
> +static const struct ethtool_ops rgeth_ethtool_ops = {
> +	.nway_reset		= phy_ethtool_nway_reset,
> +	.get_msglevel		= ravb_get_msglevel,
> +	.set_msglevel		= ravb_set_msglevel,
> +	.get_link		= ethtool_op_get_link,
> +	.get_strings		= ravb_get_strings,
> +	.get_ethtool_stats	= ravb_get_ethtool_stats,
> +	.get_sset_count		= ravb_get_sset_count,
> +	.get_ringparam		= ravb_get_ringparam,
> +	.set_ringparam		= ravb_set_ringparam,
> +	.get_ts_info		= ravb_get_ts_info,
> +	.get_link_ksettings	= phy_ethtool_get_link_ksettings,
> +	.set_link_ksettings	= phy_ethtool_set_link_ksettings,
> +};

It is not obvious why you need a seperate ethtool_ops structure? Does
it not support WOL?

> +static const struct net_device_ops rgeth_netdev_ops = {
> +	.ndo_open               = ravb_open,
> +	.ndo_stop               = ravb_close,
> +	.ndo_start_xmit         = ravb_start_xmit,
> +	.ndo_select_queue       = ravb_select_queue,
> +	.ndo_get_stats          = ravb_get_stats,
> +	.ndo_set_rx_mode        = ravb_set_rx_mode,
> +	.ndo_tx_timeout         = ravb_tx_timeout,
> +	.ndo_do_ioctl           = ravb_do_ioctl,
> +	.ndo_validate_addr      = eth_validate_addr,
> +	.ndo_set_mac_address    = eth_mac_addr,
> +	.ndo_set_features       = rgeth_set_features,

It seems like .ndo_set_features is the only difference. Maybe handle
that in actual function?

> @@ -1965,6 +2446,7 @@ static const struct of_device_id ravb_match_table[] = {
>  	{ .compatible = "renesas,etheravb-rcar-gen2", .data = (void *)RCAR_GEN2 },
>  	{ .compatible = "renesas,etheravb-r8a7795", .data = (void *)RCAR_GEN3 },
>  	{ .compatible = "renesas,etheravb-rcar-gen3", .data = (void *)RCAR_GEN3 },
> +	{ .compatible = "renesas,rzg2l-gether", .data = (void *)RZ_G2L },
>  	{ }
>  };

Please document the new compatible string in the DT binding.

       Andrew

RE: [PATCH/RFC 2/2] ravb: Add GbEthernet driver support

[Biju Das]

Hi Andrew Lunn,

Thanks for the feedback.

> Subject: Re: [PATCH/RFC 2/2] ravb: Add GbEthernet driver support
> 
> > +	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID) {
> > +		ravb_write(ndev, ravb_read(ndev, CXR31)
> > +			 | CXR31_SEL_LINK0, CXR31);
> > +	} else {
> > +		ravb_write(ndev, ravb_read(ndev, CXR31)
> > +			 & ~CXR31_SEL_LINK0, CXR31);
> > +	}
> 
> You need to be very careful here. What value is passed to the PHY?

PHY_INTERFACE_MODE_RGMII is the value passed to PHY.

The below code is used only by R-Car3, where MAC supports internal delay.

> 
> There is some funky code:
> 
>        /* Fall back to legacy rgmii-*id behavior */
>         if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
>             priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID) {
>                 priv->rxcidm = 1;
>                 priv->rgmii_override = 1;
>         }
> 
>         if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
>             priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID) {
>                 if (!WARN(soc_device_match(ravb_delay_mode_quirk_match),
>                           "phy-mode %s requires TX clock internal delay
> mode which is not supported by this hardware revision. Please update
> device tree",
>                           phy_modes(priv->phy_interface))) {
>                         priv->txcidm = 1;
>                         priv->rgmii_override = 1;
>                 }
>         }
> ...
> 
>         iface = priv->rgmii_override ? PHY_INTERFACE_MODE_RGMII
>                                      : priv->phy_interface;
>         phydev = of_phy_connect(ndev, pn, ravb_adjust_link, 0, iface);
> 
> So it looks like, with PHY_INTERFACE_MODE_RGMII_ID,
> PHY_INTERFACE_MODE_RGMII_TXID, PHY_INTERFACE_MODE_RGMII_RXID the PHY is
> passed PHY_INTERFACE_MODE_RGMII, with the assumption the MAC is adding the
> delay. But it looks like you are only adding a delay for
> PHY_INTERFACE_MODE_RGMII_ID. So this appears wrong.

This SoC family doesn't use that code and doesn't support MAC internal delay. 
It is applicable only for R-Car Gen3 whet it sets "priv->rgmii_override"
For other SoC's it is just priv->phy_interface.

> 
> > @@ -1082,15 +1440,23 @@ static int ravb_phy_init(struct net_device
> *ndev)
> >  		netdev_info(ndev, "limited PHY to 100Mbit/s\n");
> >  	}
> >
> > -	/* 10BASE, Pause and Asym Pause is not supported */
> > -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> > -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
> > -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
> > -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
> > +	if (priv->chip_id == RZ_G2L) {
> > +		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID)
> > +			ravb_write(ndev, ravb_read(ndev, CXR35) |
> CXR35_SEL_MODIN, CXR35);
> > +		else if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII)
> > +			ravb_write(ndev, 0x3E80000, CXR35);
> 
> This is not obviously correct. What about the other two RGMII modes?
> 
> > @@ -1348,6 +1741,21 @@ static const struct ethtool_ops ravb_ethtool_ops
> = {
> >  	.set_wol		= ravb_set_wol,
> >  };
> >
> > +static const struct ethtool_ops rgeth_ethtool_ops = {
> > +	.nway_reset		= phy_ethtool_nway_reset,
> > +	.get_msglevel		= ravb_get_msglevel,
> > +	.set_msglevel		= ravb_set_msglevel,
> > +	.get_link		= ethtool_op_get_link,
> > +	.get_strings		= ravb_get_strings,
> > +	.get_ethtool_stats	= ravb_get_ethtool_stats,
> > +	.get_sset_count		= ravb_get_sset_count,
> > +	.get_ringparam		= ravb_get_ringparam,
> > +	.set_ringparam		= ravb_set_ringparam,
> > +	.get_ts_info		= ravb_get_ts_info,
> > +	.get_link_ksettings	= phy_ethtool_get_link_ksettings,
> > +	.set_link_ksettings	= phy_ethtool_set_link_ksettings,
> > +};
> 
> It is not obvious why you need a seperate ethtool_ops structure? Does it
> not support WOL?

Yes,WOL is supported by WAKE_PHY. As per HW manual MAGIC packet detection is not supported.
OK I will add wol. Also it supports eee which we are planning to add later stage.


> 

> > +static const struct net_device_ops rgeth_netdev_ops = {
> > +	.ndo_open               = ravb_open,
> > +	.ndo_stop               = ravb_close,
> > +	.ndo_start_xmit         = ravb_start_xmit,
> > +	.ndo_select_queue       = ravb_select_queue,
> > +	.ndo_get_stats          = ravb_get_stats,
> > +	.ndo_set_rx_mode        = ravb_set_rx_mode,
> > +	.ndo_tx_timeout         = ravb_tx_timeout,
> > +	.ndo_do_ioctl           = ravb_do_ioctl,
> > +	.ndo_validate_addr      = eth_validate_addr,
> > +	.ndo_set_mac_address    = eth_mac_addr,
> > +	.ndo_set_features       = rgeth_set_features,
> 
> It seems like .ndo_set_features is the only difference. Maybe handle that
> in actual function?

OK will do.

> 
> > @@ -1965,6 +2446,7 @@ static const struct of_device_id
> ravb_match_table[] = {
> >  	{ .compatible = "renesas,etheravb-rcar-gen2", .data = (void
> *)RCAR_GEN2 },
> >  	{ .compatible = "renesas,etheravb-r8a7795", .data = (void
> *)RCAR_GEN3 },
> >  	{ .compatible = "renesas,etheravb-rcar-gen3", .data = (void
> > *)RCAR_GEN3 },
> > +	{ .compatible = "renesas,rzg2l-gether", .data = (void *)RZ_G2L },
> >  	{ }
> >  };
> 
> Please document the new compatible string in the DT binding.

OK. Will do

Thanks,
Biju
> 
>        Andrew

Re: [PATCH/RFC 2/2] ravb: Add GbEthernet driver support

[Andrew Lunn]

On Wed, Jul 14, 2021 at 05:01:34PM +0000, Biju Das wrote:
> Hi Andrew Lunn,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH/RFC 2/2] ravb: Add GbEthernet driver support
> > 
> > > +	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID) {
> > > +		ravb_write(ndev, ravb_read(ndev, CXR31)
> > > +			 | CXR31_SEL_LINK0, CXR31);
> > > +	} else {
> > > +		ravb_write(ndev, ravb_read(ndev, CXR31)
> > > +			 & ~CXR31_SEL_LINK0, CXR31);
> > > +	}
> > 
> > You need to be very careful here. What value is passed to the PHY?
> 
> PHY_INTERFACE_MODE_RGMII is the value passed to PHY.

In all four cases? So if DT contains rgmii-txid, or rgmii-rxid, the
requested delay will not happen in either the MAC or the PHY?

In general in Linux, MAC drivers don't add any delays, and request the
PHY to do it. There are some MAC drivers which do it differently, they
add the delays, but that is uncommon. So unless you have a good reason
not to, i would suggest you leave the PHY to do the delays.

	  Andrew

RE: [PATCH/RFC 2/2] ravb: Add GbEthernet driver support

[Biju Das]

Hi Andrew Lunn,

Thanks for the feedback.

> Subject: Re: [PATCH/RFC 2/2] ravb: Add GbEthernet driver support
> 
> On Wed, Jul 14, 2021 at 05:01:34PM +0000, Biju Das wrote:
> > Hi Andrew Lunn,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH/RFC 2/2] ravb: Add GbEthernet driver support
> > >
> > > > +	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID) {
> > > > +		ravb_write(ndev, ravb_read(ndev, CXR31)
> > > > +			 | CXR31_SEL_LINK0, CXR31);
> > > > +	} else {
> > > > +		ravb_write(ndev, ravb_read(ndev, CXR31)
> > > > +			 & ~CXR31_SEL_LINK0, CXR31);
> > > > +	}
> > >
> > > You need to be very careful here. What value is passed to the PHY?
> >
> > PHY_INTERFACE_MODE_RGMII is the value passed to PHY.
> 
> In all four cases? So if DT contains rgmii-txid, or rgmii-rxid, the
> requested delay will not happen in either the MAC or the PHY?

OK, it is my mistake. I specified "rgmii" in DT. 

Without phy delays (rx and tx) ethernet won't work. Will fix it as "rgmii-id" in dt.

The above register(In-band Status set register) is basically used for selecting the link.

> 
> In general in Linux, MAC drivers don't add any delays, and request the PHY
> to do it. There are some MAC drivers which do it differently, they add the
> delays, but that is uncommon. So unless you have a good reason not to, i
> would suggest you leave the PHY to do the delays.

OK. 


Regards,
Biju

Re: [PATCH/RFC 2/2] ravb: Add GbEthernet driver support

[Geert Uytterhoeven]

Hi Biju,

On Wed, Jul 14, 2021 at 4:54 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Add Gigabit Ethernet driver support.
>
> The Gigabit Etherner IP consists of Ethernet controller (E-MAC),
> Internal TCP/IP Offload Engine (TOE) and Dedicated Direct memory
> access controller (DMAC).
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

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

> @@ -986,6 +1068,7 @@ struct ravb_ptp {
>  enum ravb_chip_id {
>         RCAR_GEN2,
>         RCAR_GEN3,
> +       RZ_G2L,
>  };

Instead of adding another chip type, it may be better to replace
the chip type by a structure with feature bits, values, and function
pointers (see examples below).

BTW given the ravb driver is based on the sh_eth driver ("Ethernet
AVB includes an Gigabit Ethernet controller (E-MAC) that is basically
compatible with SuperH Gigabit Ethernet E-MAC"), and seeing the amount
of changes, I'm wondering if rgeth is closer to sh_eth? ;-)

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

> @@ -247,8 +288,12 @@ static void ravb_ring_free(struct net_device *ndev, int q)
>         int ring_size;
>         int i;
>
> -       if (priv->rx_ring[q]) {
> -               ravb_ring_free_ex(ndev, q);
> +       if (priv->chip_id == RZ_G2L) {
> +               if (priv->rgeth_rx_ring[q])
> +                       rgeth_ring_free_ex(ndev, q);
> +       } else {
> +               if (priv->rx_ring[q])
> +                       ravb_ring_free_ex(ndev, q);
>         }

Could be called through a function pointer instead.

> @@ -356,7 +434,7 @@ static void ravb_ring_format(struct net_device *ndev, int q)
>  static int ravb_ring_init(struct net_device *ndev, int q)
>  {
>         struct ravb_private *priv = netdev_priv(ndev);
> -       size_t skb_sz = RX_BUF_SZ;
> +       size_t skb_sz = (priv->chip_id == RZ_G2L) ? RGETH_RCV_BUFF_MAX : RX_BUF_SZ;

Could use a value in the structure.

> @@ -730,7 +1054,7 @@ static void ravb_emac_interrupt_unlocked(struct net_device *ndev)
>         ecsr = ravb_read(ndev, ECSR);
>         ravb_write(ndev, ecsr, ECSR);   /* clear interrupt */
>
> -       if (ecsr & ECSR_MPD)
> +       if (priv->chip_id != RZ_G2L && (ecsr & ECSR_MPD))

Could use a feature bit.

> @@ -2104,11 +2578,19 @@ static int ravb_probe(struct platform_device *pdev)
>         priv = netdev_priv(ndev);
>         priv->chip_id = chip_id;
>
> -       ndev->features = NETIF_F_RXCSUM;
> -       ndev->hw_features = NETIF_F_RXCSUM;
> -
> -       pm_runtime_enable(&pdev->dev);
> -       pm_runtime_get_sync(&pdev->dev);
> +       if (chip_id == RZ_G2L) {
> +               ndev->hw_features |= (NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
> +               priv->rstc = devm_reset_control_get(&pdev->dev, NULL);

R-Car Gen2 and Gen3 describe a reset in DT, too.
Does it hurt to always use the reset?

> +               if (IS_ERR(priv->rstc)) {
> +                       dev_err(&pdev->dev, "failed to get cpg reset\n");

dev_err_probe(), to avoid printing an error on -EPROBE_DEFER.

> +                       free_netdev(ndev);
> +                       return PTR_ERR(priv->rstc);
> +               }
> +               reset_control_deassert(priv->rstc);
> +       } else {
> +               ndev->features = NETIF_F_RXCSUM;
> +               ndev->hw_features = NETIF_F_RXCSUM;
> +       }


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH/RFC 2/2] ravb: Add GbEthernet driver support

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH/RFC 2/2] ravb: Add GbEthernet driver support
> 
> Hi Biju,
> 
> On Wed, Jul 14, 2021 at 4:54 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Add Gigabit Ethernet driver support.
> >
> > The Gigabit Etherner IP consists of Ethernet controller (E-MAC),
> > Internal TCP/IP Offload Engine (TOE) and Dedicated Direct memory
> > access controller (DMAC).
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> 
> > @@ -986,6 +1068,7 @@ struct ravb_ptp {  enum ravb_chip_id {
> >         RCAR_GEN2,
> >         RCAR_GEN3,
> > +       RZ_G2L,
> >  };
> 
> Instead of adding another chip type, it may be better to replace the chip
> type by a structure with feature bits, values, and function pointers (see
> examples below).
 
> BTW given the ravb driver is based on the sh_eth driver ("Ethernet AVB
> includes an Gigabit Ethernet controller (E-MAC) that is basically
> compatible with SuperH Gigabit Ethernet E-MAC"), and seeing the amount of
> changes, I'm wondering if rgeth is closer to sh_eth? ;-)

With sh_eth driver,We can reuse only E-MAC IP. But with RAVB we can reuse both DMAC and E-MAC.
Hence I believe it is closer to AVB.

> 
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> 
> > @@ -247,8 +288,12 @@ static void ravb_ring_free(struct net_device *ndev,
> int q)
> >         int ring_size;
> >         int i;
> >
> > -       if (priv->rx_ring[q]) {
> > -               ravb_ring_free_ex(ndev, q);
> > +       if (priv->chip_id == RZ_G2L) {
> > +               if (priv->rgeth_rx_ring[q])
> > +                       rgeth_ring_free_ex(ndev, q);
> > +       } else {
> > +               if (priv->rx_ring[q])
> > +                       ravb_ring_free_ex(ndev, q);
> >         }
> 
> Could be called through a function pointer instead.

OK.
> 
> > @@ -356,7 +434,7 @@ static void ravb_ring_format(struct net_device
> > *ndev, int q)  static int ravb_ring_init(struct net_device *ndev, int
> > q)  {
> >         struct ravb_private *priv = netdev_priv(ndev);
> > -       size_t skb_sz = RX_BUF_SZ;
> > +       size_t skb_sz = (priv->chip_id == RZ_G2L) ? RGETH_RCV_BUFF_MAX
> > + : RX_BUF_SZ;
> 
> Could use a value in the structure.
OK.
> 
> > @@ -730,7 +1054,7 @@ static void ravb_emac_interrupt_unlocked(struct
> net_device *ndev)
> >         ecsr = ravb_read(ndev, ECSR);
> >         ravb_write(ndev, ecsr, ECSR);   /* clear interrupt */
> >
> > -       if (ecsr & ECSR_MPD)
> > +       if (priv->chip_id != RZ_G2L && (ecsr & ECSR_MPD))
> 
> Could use a feature bit.

OK.

> 
> > @@ -2104,11 +2578,19 @@ static int ravb_probe(struct platform_device
> *pdev)
> >         priv = netdev_priv(ndev);
> >         priv->chip_id = chip_id;
> >
> > -       ndev->features = NETIF_F_RXCSUM;
> > -       ndev->hw_features = NETIF_F_RXCSUM;
> > -
> > -       pm_runtime_enable(&pdev->dev);
> > -       pm_runtime_get_sync(&pdev->dev);
> > +       if (chip_id == RZ_G2L) {
> > +               ndev->hw_features |= (NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
> > +               priv->rstc = devm_reset_control_get(&pdev->dev, NULL);
> 
> R-Car Gen2 and Gen3 describe a reset in DT, too.
> Does it hurt to always use the reset?

OK will use reset.


> 
> > +               if (IS_ERR(priv->rstc)) {
> > +                       dev_err(&pdev->dev, "failed to get cpg
> > + reset\n");
> 
> dev_err_probe(), to avoid printing an error on -EPROBE_DEFER.

OK.


Regards,
Biju

> 
> > +                       free_netdev(ndev);
> > +                       return PTR_ERR(priv->rstc);
> > +               }
> > +               reset_control_deassert(priv->rstc);
> > +       } else {
> > +               ndev->features = NETIF_F_RXCSUM;
> > +               ndev->hw_features = NETIF_F_RXCSUM;
> > +       }
> 
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds

Re: [PATCH/RFC 2/2] ravb: Add GbEthernet driver support

[Sergei Shtylyov]

Hello!

On 7/14/21 5:54 PM, Biju Das wrote:

> Add Gigabit Ethernet driver support.
> 
> The Gigabit Etherner IP consists of Ethernet controller (E-MAC),
> Internal TCP/IP Offload Engine (TOE) and Dedicated Direct memory
> access controller (DMAC).
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  92 ++-
>  drivers/net/ethernet/renesas/ravb_main.c | 683 ++++++++++++++++++++---
>  2 files changed, 682 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 80e62ca2e3d3..4e65683fb458 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
[...]
> @@ -181,25 +183,39 @@ enum ravb_reg {
>  
>  	/* E-MAC registers */
>  	ECMR	= 0x0500,
> +	CXR20	= 0x0500,	/* Documented for RZ/G2L only */

   This is the same register as ECMR. Those cryptic CXR<n> names we first encountered in
the R-Car gen1 SoCs. :-)

>  	RFLR	= 0x0508,
> +	CXR2A	= 0x0508,	/* Documented for RZ/G2L only */

   Same comment -- no need to re-#define the macro.

[...]
> @@ -216,6 +232,7 @@ enum CCC_BIT {
>  	CCC_CSEL_HPB	= 0x00010000,
>  	CCC_CSEL_ETH_TX	= 0x00020000,
>  	CCC_CSEL_GMII_REF = 0x00030000,
> +	CCC_BOC		= 0x00100000,	/* Documented for RZ/G2L only */

   Hm, where do you use it? I'm not seeing...

[...]
> @@ -804,16 +821,21 @@ enum TID_BIT {
>  enum ECMR_BIT {
>  	ECMR_PRM	= 0x00000001,
>  	ECMR_DM		= 0x00000002,
> +	CXR20_LPM	= 0x00000010,	/* Documented for RZ/G2L only */

   Please use the ECMR_ prefix.

>  	ECMR_TE		= 0x00000020,
>  	ECMR_RE		= 0x00000040,
>  	ECMR_MPDE	= 0x00000200,
> +	CXR20_CER	= 0x00001000,	/* Documented for RZ/G2L only */

   Ditto.

>  	ECMR_TXF	= 0x00010000,	/* Documented for R-Car Gen3 only */
>  	ECMR_RXF	= 0x00020000,
>  	ECMR_PFR	= 0x00040000,
>  	ECMR_ZPF	= 0x00080000,	/* Documented for R-Car Gen3 only */
>  	ECMR_RZPF	= 0x00100000,
>  	ECMR_DPAD	= 0x00200000,
> +	CXR20_CXSER	= 0x00400000,	/* Documented for RZ/G2L only */

    Ditto.

>  	ECMR_RCSC	= 0x00800000,
> +	CXR20_TCPT	= 0x01000000,	/* Documented for RZ/G2L only */
> +	CXR20_RCPT	= 0x02000000,	/* Documented for RZ/G2L only */

   Ditto.

[...]
> @@ -823,6 +845,7 @@ enum ECSR_BIT {
>  	ECSR_MPD	= 0x00000002,
>  	ECSR_LCHNG	= 0x00000004,
>  	ECSR_PHYI	= 0x00000008,
> +	ECSR_RFRI	= 0x00000010,	/* Documented for RZ/G2L only */

   Not PFRI?
   Also, my R-Car gen2/3 manual dociment the bit.

[...]
> @@ -862,6 +885,14 @@ enum GECMR_BIT {
>  	GECMR_SPEED_1000 = 0x00000001,
>  };
>  
> +/* GECMR */
> +enum RGETH_GECMR_BIT {
> +	RGETH_GECMR_SPEED	= 0x00000030,
> +	RGETH_GECMR_SPEED_10	= 0x00000000,
> +	RGETH_GECMR_SPEED_100	= 0x00000010,
> +	RGETH_GECMR_SPEED_1000	= 0x00000020,

   Why not place those values in the existing GECMR *enum* with the necessary annotations?
Also I think the prefixes should be GETH, not RGETH -- RAVB is the EtherAVB driver's name, not
the actual hardware...

[...]
> @@ -956,8 +1035,11 @@ enum RAVB_QUEUE {
>  
>  #define RX_BUF_SZ	(2048 - ETH_FCS_LEN + sizeof(__sum16))
>  
> +#define RGETH_RCV_BUFF_MAX 8192
> +#define RGETH_RCV_DESCRIPTOR_DATA_SIZE 4080

   Please shorten DESCRIPTOR to DESC. And RCV_ should probably be renemed to R?..

[...]> @@ -1040,6 +1123,11 @@ struct ravb_private {
>  	unsigned txcidm:1;		/* TX Clock Internal Delay Mode */
>  	unsigned rgmii_override:1;	/* Deprecated rgmii-*id behavior */
>  	int num_tx_desc;		/* TX descriptors per packet */
> +
> +	int duplex;
> +	struct ravb_rx_desc *rgeth_rx_ring[NUM_RX_QUEUE];
> +	struct sk_buff *rxtop_skb;
> +	struct reset_control *rstc;

   I'd have apreciated some comments here... I don't quite understand what the first 3 are for...

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 7e6feda59f4a..e28a63de553d 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -82,6 +83,23 @@ static int ravb_config(struct net_device *ndev)
>  	return error;
>  }
>  
> +static void rgeth_set_rate(struct net_device *ndev)

   What about ravb_set_rate_rzgl? I think we use this convention in sh_eth.c...

[...]
> @@ -217,6 +235,29 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
>  }
>  
>  /* Free skb's and DMA buffers for Ethernet AVB */
> +static void rgeth_ring_free_ex(struct net_device *ndev, int q)

   It definitely should be called ravb_rx_ring_free_rgeth(), I think...

> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	int ring_size;
> +	int i;
> +
> +	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> +		struct ravb_rx_desc *desc = &priv->rgeth_rx_ring[q][i];
> +
> +		if (!dma_mapping_error(ndev->dev.parent,
> +				       le32_to_cpu(desc->dptr)))
> +			dma_unmap_single(ndev->dev.parent,
> +					 le32_to_cpu(desc->dptr),
> +					 RGETH_RCV_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->rgeth_rx_ring[q],
> +			  priv->rx_desc_dma[q]);
> +	priv->rgeth_rx_ring[q] = NULL;
> +}
> +
>  static void ravb_ring_free_ex(struct net_device *ndev, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> @@ -247,8 +288,12 @@ static void ravb_ring_free(struct net_device *ndev, int q)
>  	int ring_size;
>  	int i;
>  
> -	if (priv->rx_ring[q]) {
> -		ravb_ring_free_ex(ndev, q);
> +	if (priv->chip_id == RZ_G2L) {
> +		if (priv->rgeth_rx_ring[q])
> +			rgeth_ring_free_ex(ndev, q);
> +	} else {
> +		if (priv->rx_ring[q])
> +			ravb_ring_free_ex(ndev, q);

   I definitely don't like that _ex suffix...

[...]
> @@ -281,6 +326,36 @@ static void ravb_ring_free(struct net_device *ndev, int q)
>  }
>  
>  /* Format skb and descriptor buffer for Ethernet AVB */
> +static void rgeth_ring_format_ex(struct net_device *ndev, int q)

    How about ravb_rx_ring_format_geth()?

> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	struct ravb_rx_desc *rx_desc;
> +	int rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
> +	dma_addr_t dma_addr;
> +	int i;
> +
> +	memset(priv->rgeth_rx_ring[q], 0, rx_ring_size);
> +	/* Build RX ring buffer */
> +	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> +		/* RX descriptor */
> +		rx_desc = &priv->rgeth_rx_ring[q][i];
> +		rx_desc->ds_cc = cpu_to_le16(RGETH_RCV_DESCRIPTOR_DATA_SIZE);

    Mhm, I think we should further parametrize the RX data in order to save the code duplication...

> +		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
> +					  RGETH_RCV_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->rgeth_rx_ring[q][i];
> +	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
> +	rx_desc->die_dt = DT_LINKFIX; /* type */
> +}
> +
>  static void ravb_ring_format_ex(struct net_device *ndev, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> @@ -326,7 +401,10 @@ static void ravb_ring_format(struct net_device *ndev, int q)
>  	priv->dirty_tx[q] = 0;
>  
>  	/* Build RX ring buffer */
> -	ravb_ring_format_ex(ndev, q);
> +	if (priv->chip_id == RZ_G2L)
> +		rgeth_ring_format_ex(ndev, q);
> +	else
> +		ravb_ring_format_ex(ndev, q);

   ... so that we don't have alike code snippets anymore...

[...]
> @@ -356,7 +434,7 @@ static void ravb_ring_format(struct net_device *ndev, int q)
>  static int ravb_ring_init(struct net_device *ndev, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> -	size_t skb_sz = RX_BUF_SZ;
> +	size_t skb_sz = (priv->chip_id == RZ_G2L) ? RGETH_RCV_BUFF_MAX : RX_BUF_SZ;

    Should be also parametrized...

[...] 
> @@ -414,6 +503,45 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>  }
>  
>  /* E-MAC init function */
> +static void rgeth_emac_init_ex(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	/* Receive frame limit set register */
> +	ravb_write(ndev, RGETH_RCV_BUFF_MAX + ETH_FCS_LEN, RFLR);
> +
> +	/* PAUSE prohibition */
> +	ravb_write(ndev, ECMR_ZPF | (priv->duplex ? ECMR_DM : 0) |
> +			 ECMR_TE | ECMR_RE | CXR20_RCPT |
> +			 ECMR_TXF | ECMR_RXF | ECMR_PRM, ECMR);
> +
> +	rgeth_set_rate(ndev);
> +
> +	/* Set MAC address */
> +	ravb_write(ndev,
> +		   (ndev->dev_addr[0] << 24) | (ndev->dev_addr[1] << 16) |
> +		   (ndev->dev_addr[2] << 8)  | (ndev->dev_addr[3]), MAHR);
> +	ravb_write(ndev,
> +		   (ndev->dev_addr[4] << 8)  | (ndev->dev_addr[5]), MALR);
> +
> +	/* E-MAC status register clear */
> +	ravb_write(ndev, ECSR_ICD | ECSR_LCHNG | ECSR_RFRI, ECSR);
> +	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);
> +
> +	/* E-MAC interrupt enable register */
> +	ravb_write(ndev, ECSIPR_ICDIP, ECSIPR);
> +
> +	ravb_write(ndev, ravb_read(ndev, CXR31)
> +			 & ~CXR31_SEL_LINK1, CXR31);
> +	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID) {
> +		ravb_write(ndev, ravb_read(ndev, CXR31)
> +			 | CXR31_SEL_LINK0, CXR31);
> +	} else {
> +		ravb_write(ndev, ravb_read(ndev, CXR31)
> +			 & ~CXR31_SEL_LINK0, CXR31);
> +	}
> +}

   The more I look at your patches, the more I think Geert's suggestion to mimic the sh_eth's
approach to handling the SoC differencies is worth using in this driver as well...

[...]
> @@ -442,10 +570,41 @@ static void ravb_emac_init_ex(struct net_device *ndev)
[...]
>  /* Device init function for Ethernet AVB */
> +static void rgeth_dmac_init_ex(struct net_device *ndev)
> +{
> +	/* Set AVB RX */
> +	ravb_write(ndev, 0x60000000, RCR);

   What those (undocumented?) bits mean, I wonder?

[...]
> @@ -545,6 +708,23 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
>  	}
>  }
>  
> +static void rgeth_rx_csum(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;
> +}

   So does GbEth device sumpport checksuming or not?

[...]
> @@ -561,6 +741,143 @@ static void ravb_rx_csum(struct sk_buff *skb)
>  }
>  
>  /* Packet receive function for Ethernet AVB */
> +struct sk_buff *rgeth_get_skb(struct net_device *ndev, int q, int entry,

   Not *static*?

> +			      struct ravb_rx_desc *desc)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	struct sk_buff *skb;
> +
> +	skb = priv->rx_skb[q][entry];
> +	priv->rx_skb[q][entry] = NULL;
> +	dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
> +			 ALIGN(RGETH_RCV_BUFF_MAX, 16), DMA_FROM_DEVICE);
> +
> +	return skb;
> +}
> +
> +static bool rgeth_rx_ex(struct net_device *ndev, int *quota, int q)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> +	int boguscnt = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> +	struct net_device_stats *stats = &priv->stats[q];
> +	struct ravb_rx_desc *desc;
> +	struct sk_buff *skb;
> +	dma_addr_t dma_addr;
> +	u8  desc_status;
> +	u8  die_dt;
> +	u16 pkt_len;
> +	int limit;
> +
> +	boguscnt = min(boguscnt, *quota);
> +	limit = boguscnt;
> +	desc = &priv->rgeth_rx_ring[q][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;
> +			if (die_dt == DT_FSINGLE) {

   Why not *switch*?

> +				skb = rgeth_get_skb(ndev, q, entry, desc);
> +				skb_put(skb, pkt_len);
> +				skb->protocol = eth_type_trans(skb, ndev);
> +				if (ndev->features & NETIF_F_RXCSUM)
> +					rgeth_rx_csum(skb);
> +				napi_gro_receive(&priv->napi[q], skb);
> +				stats->rx_packets++;
> +				stats->rx_bytes += pkt_len;
> +			} else if (die_dt == DT_FSTART) {
> +				priv->rxtop_skb = rgeth_get_skb(ndev, q, entry, desc);
> +				skb_put(priv->rxtop_skb, pkt_len);
> +			} else if (die_dt == DT_FMID) {
> +				skb = rgeth_get_skb(ndev, q, entry, desc);
> +				skb_copy_to_linear_data_offset(priv->rxtop_skb,
> +							       priv->rxtop_skb->len,
> +							       skb->data,
> +							       pkt_len);
> +				skb_put(priv->rxtop_skb, pkt_len);
> +				dev_kfree_skb(skb);
> +			} else if (die_dt == DT_FEND) {
> +				skb = rgeth_get_skb(ndev, q, entry, desc);
> +				skb_copy_to_linear_data_offset(priv->rxtop_skb,
> +							       priv->rxtop_skb->len,
> +							       skb->data,
> +							       pkt_len);
> +				skb_put(priv->rxtop_skb, pkt_len);
> +				dev_kfree_skb(skb);
> +				priv->rxtop_skb->protocol =
> +					eth_type_trans(priv->rxtop_skb, ndev);
> +				if (ndev->features & NETIF_F_RXCSUM)
> +					rgeth_rx_csum(skb);
> +				napi_gro_receive(&priv->napi[q],
> +						 priv->rxtop_skb);
> +				stats->rx_packets++;
> +				stats->rx_bytes += priv->rxtop_skb->len;
> +			}

   So why it's necessary to handle the multidecriptor packets?

[...]
> @@ -678,7 +995,10 @@ static bool ravb_rx_ex(struct net_device *ndev, int *quota, int q)
>  
>  static bool ravb_rx(struct net_device *ndev, int *quota, int q)
>  {
> -	return ravb_rx_ex(ndev, quota, q);
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	return (priv->chip_id == RZ_G2L) ?
> +		rgeth_rx_ex(ndev, quota, q) : ravb_rx_ex(ndev, quota, q);

   Definitely better with the pre-init'ed function pointers if needed at all)...

[...]
> @@ -696,11 +1016,15 @@ static void ravb_rcv_snd_enable(struct net_device *ndev)
>  /* function for waiting dma process finished */
>  static int ravb_stop_dma(struct net_device *ndev)
>  {
> +	struct ravb_private *priv = netdev_priv(ndev);
>  	int error;
>  
>  	/* Wait for stopping the hardware TX process */
> -	error = ravb_wait(ndev, TCCR,
> -			  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0);
> +	if (priv->chip_id == RZ_G2L)
> +		error = ravb_wait(ndev, TCCR, TCCR_TSRQ0, 0);

   Only a single TX queue?

> +	else
> +		error = ravb_wait(ndev, TCCR,
> +				  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0);

   This is better expressed thru a mask in the private data...

[...]
> @@ -823,11 +1147,13 @@ static bool ravb_queue_interrupt(struct net_device *ndev, int q)
>  
>  static bool ravb_timestamp_interrupt(struct net_device *ndev)
>  {
> +	struct ravb_private *priv = netdev_priv(ndev);
>  	u32 tis = ravb_read(ndev, TIS);
>  
>  	if (tis & TIS_TFUF) {
>  		ravb_write(ndev, ~(TIS_TFUF | TIS_RESERVED), TIS);
> -		ravb_get_tx_tstamp(ndev);
> +		if (priv->chip_id != RZ_G2L)
> +			ravb_get_tx_tstamp(ndev);

   So, no TX pakcet timestaming?..

[...]
> @@ -872,7 +1198,7 @@ static irqreturn_t ravb_interrupt(int irq, void *dev_id)
>  	}
>  
>  	/* gPTP interrupt status summary */
> -	if (iss & ISS_CGIS) {
> +	if (priv->chip_id != RZ_G2L && (iss & ISS_CGIS)) {

   ...and no gPTP IRQ?

>  		ravb_ptp_interrupt(ndev);
>  		result = IRQ_HANDLED;
>  	}
> @@ -947,12 +1273,20 @@ static int ravb_poll(struct napi_struct *napi, int budget)
>  	int q = napi - priv->napi;
>  	int mask = BIT(q);
>  	int quota = budget;
> +	int entry;
> +	struct ravb_rx_desc *desc;

   The lines with the local variables should be sotrted by length, the longer going first
(so caleld reverse Xmas tree)...

[...]
> @@ -1082,15 +1440,23 @@ static int ravb_phy_init(struct net_device *ndev)
>  		netdev_info(ndev, "limited PHY to 100Mbit/s\n");
>  	}
>  
> -	/* 10BASE, Pause and Asym Pause is not supported */
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
> +	if (priv->chip_id == RZ_G2L) {

  Why not *switch*?

cxr35> +		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID)
> +			ravb_write(ndev, ravb_read(ndev, CXR35) | CXR35_SEL_MODIN, CXR35);
> +		else if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII)
> +			ravb_write(ndev, 0x3E80000, CXR35);
> +	} else {
>  
[...]
> @@ -1133,6 +1499,24 @@ static void ravb_set_msglevel(struct net_device *ndev, u32 value)
>  	priv->msg_enable = value;
>  }
>  
> +static const char rgeth_gstrings_stats[][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",
> +};

   I think we can get out without duplicating the stgring arrays...

[...]
> @@ -1669,6 +2079,20 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
>  	stats0 = &priv->stats[RAVB_BE];
>  	stats1 = &priv->stats[RAVB_NC];
>  
> +	if (priv->chip_id == RZ_G2L) {
> +		nstats->tx_dropped += ravb_read(ndev, TROCR);
> +		ravb_write(ndev, 0, TROCR);	/* (write clear) */

   Why duplicated with R-Car gen3?

> +		nstats->collisions += ravb_read(ndev, CDCR);
> +		ravb_write(ndev, 0, CDCR);	/* (write clear) */
> +		nstats->tx_carrier_errors += ravb_read(ndev, LCCR);
> +		ravb_write(ndev, 0, LCCR);	/* (write clear) */
> +
> +		nstats->tx_carrier_errors += ravb_read(ndev, CERCR);
> +		ravb_write(ndev, 0, CERCR);	/* (write clear) */
> +		nstats->tx_carrier_errors += ravb_read(ndev, CEECR);
> +		ravb_write(ndev, 0, CEECR);	/* (write clear) */
> +	}
> +
>  	if (priv->chip_id == RCAR_GEN3) {

   Must be else if here...

[...]
> @@ -1899,6 +2325,44 @@ static int ravb_set_features(struct net_device *ndev,
>  	return 0;
>  }
>  
> +static int rgeth_set_features(struct net_device *ndev,
> +			      netdev_features_t features)
> +{
> +	int error;
> +	unsigned int reg = 0;
> +	netdev_features_t changed = features ^ ndev->features;

   Reverse Xmas tree, please.

> +
> +	reg = ravb_read(ndev, CSR0);
> +
> +	ravb_write(ndev, ravb_read(ndev, CSR0) & ~(CSR0_RPE | CSR0_TPE), CSR0);

   Why read CSR0 twice?

[...]
> @@ -1930,7 +2408,10 @@ static int ravb_mdio_init(struct ravb_private *priv)
>  		return -ENOMEM;
>  
>  	/* Hook up MII support for ethtool */
> -	priv->mii_bus->name = "ravb_mii";
> +	if (priv->chip_id == RZ_G2L)
> +		priv->mii_bus->name = "rgeth_mii";
> +	else
> +		priv->mii_bus->name = "ravb_mii";

   Hm... I don't think we need this.

[...]
> @@ -1965,6 +2446,7 @@ static const struct of_device_id ravb_match_table[] = {
>  	{ .compatible = "renesas,etheravb-rcar-gen2", .data = (void *)RCAR_GEN2 },
>  	{ .compatible = "renesas,etheravb-r8a7795", .data = (void *)RCAR_GEN3 },
>  	{ .compatible = "renesas,etheravb-rcar-gen3", .data = (void *)RCAR_GEN3 },
> +	{ .compatible = "renesas,rzg2l-gether", .data = (void *)RZ_G2L },

   Mhm, we reserve the right to call the hardware "gether" to the to the rdriver...
Would  the "renesas,rzg2l-gbeth" string fit?

[...]
> @@ -2082,20 +2565,11 @@ static int ravb_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	/* Get base address */
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(&pdev->dev, "invalid resource\n");
> -		return -EINVAL;
> -	}
> -
>  	ndev = alloc_etherdev_mqs(sizeof(struct ravb_private),
>  				  NUM_TX_QUEUE, NUM_RX_QUEUE);
>  	if (!ndev)
>  		return -ENOMEM;
>  
> -	/* The Ether-specific entries in the device structure. */
> -	ndev->base_addr = res->start;
>  
>  	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
>  
> @@ -2104,11 +2578,19 @@ static int ravb_probe(struct platform_device *pdev)
>  	priv = netdev_priv(ndev);
>  	priv->chip_id = chip_id;
>  
> -	ndev->features = NETIF_F_RXCSUM;
> -	ndev->hw_features = NETIF_F_RXCSUM;
> -
> -	pm_runtime_enable(&pdev->dev);
> -	pm_runtime_get_sync(&pdev->dev);
> +	if (chip_id == RZ_G2L) {
> +		ndev->hw_features |= (NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
> +		priv->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +		if (IS_ERR(priv->rstc)) {
> +			dev_err(&pdev->dev, "failed to get cpg reset\n");
> +			free_netdev(ndev);
> +			return PTR_ERR(priv->rstc);
> +		}
> +		reset_control_deassert(priv->rstc);

   This asks for the serpate patch.

[...]
> @@ -2120,18 +2602,24 @@ static int ravb_probe(struct platform_device *pdev)
>  	}
>  	ndev->irq = irq;
>  
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_get_sync(&pdev->dev);
> +
>  	priv->ndev = ndev;
>  	priv->pdev = pdev;
>  	priv->num_tx_ring[RAVB_BE] = BE_TX_RING_SIZE;
>  	priv->num_rx_ring[RAVB_BE] = BE_RX_RING_SIZE;
>  	priv->num_tx_ring[RAVB_NC] = NC_TX_RING_SIZE;
>  	priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
> -	priv->addr = devm_ioremap_resource(&pdev->dev, res);
> +	priv->addr = devm_platform_get_and_ioremap_resource(pdev, 0, &res);

   Separate patch, please.

[...]
> @@ -2181,30 +2671,39 @@ static int ravb_probe(struct platform_device *pdev)
[...]
>  	/* Set AVB config mode */
>  	ravb_set_config_mode(ndev);
>  
> -	/* Set GTI value */
> -	error = ravb_set_gti(ndev);
> -	if (error)
> -		goto out_disable_refclk;
> +	if (priv->chip_id != RZ_G2L) {
> +		/* Set GTI value */
> +		error = ravb_set_gti(ndev);
> +		if (error)
> +			goto out_disable_refclk;
>  
> -	/* Request GTI loading */
> -	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +		/* Request GTI loading */
> +		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
>  
> -	if (priv->chip_id == RCAR_GEN3) {
> -		ravb_parse_delay_mode(np, ndev);
> -		ravb_set_delay_mode(ndev);
> +		if (priv->chip_id == RCAR_GEN3) {
> +			ravb_parse_delay_mode(np, ndev);
> +			ravb_set_delay_mode(ndev);
> +		}
>  	}

   Ugh... needs to be untagled too.

   Went thru the large patch at last! If you need help addressing comments,
I can probably make the driver patches using data parametrization vs chip_id.

[...]

MBR, Sergei

RE: [PATCH/RFC 2/2] ravb: Add GbEthernet driver support

[Biju Das]

Hi Sergei,

Thanks for the comments.

> Subject: Re: [PATCH/RFC 2/2] ravb: Add GbEthernet driver support
> 
> Hello!
> 
> On 7/14/21 5:54 PM, Biju Das wrote:
> 
> > Add Gigabit Ethernet driver support.
> >
> > The Gigabit Etherner IP consists of Ethernet controller (E-MAC),
> > Internal TCP/IP Offload Engine (TOE) and Dedicated Direct memory
> > access controller (DMAC).
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/ravb.h      |  92 ++-
> >  drivers/net/ethernet/renesas/ravb_main.c | 683
> > ++++++++++++++++++++---
> >  2 files changed, 682 insertions(+), 93 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > b/drivers/net/ethernet/renesas/ravb.h
> > index 80e62ca2e3d3..4e65683fb458 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> [...]
> > @@ -181,25 +183,39 @@ enum ravb_reg {
> >
> >  	/* E-MAC registers */
> >  	ECMR	= 0x0500,
> > +	CXR20	= 0x0500,	/* Documented for RZ/G2L only */
> 
>    This is the same register as ECMR. Those cryptic CXR<n> names we first
> encountered in the R-Car gen1 SoCs. :-)
> 
> >  	RFLR	= 0x0508,
> > +	CXR2A	= 0x0508,	/* Documented for RZ/G2L only */
> 
>    Same comment -- no need to re-#define the macro.
> 
> [...]
> > @@ -216,6 +232,7 @@ enum CCC_BIT {
> >  	CCC_CSEL_HPB	= 0x00010000,
> >  	CCC_CSEL_ETH_TX	= 0x00020000,
> >  	CCC_CSEL_GMII_REF = 0x00030000,
> > +	CCC_BOC		= 0x00100000,	/* Documented for RZ/G2L only */
> 
>    Hm, where do you use it? I'm not seeing...

It was for setting byte order with little_endian compilation flag. I have removed the code
since reset value is little endian.

> 
> [...]
> > @@ -804,16 +821,21 @@ enum TID_BIT {
> >  enum ECMR_BIT {
> >  	ECMR_PRM	= 0x00000001,
> >  	ECMR_DM		= 0x00000002,
> > +	CXR20_LPM	= 0x00000010,	/* Documented for RZ/G2L only */
> 
>    Please use the ECMR_ prefix.
OK.
> 
> >  	ECMR_TE		= 0x00000020,
> >  	ECMR_RE		= 0x00000040,
> >  	ECMR_MPDE	= 0x00000200,
> > +	CXR20_CER	= 0x00001000,	/* Documented for RZ/G2L only */
> 
>    Ditto.
> 
> >  	ECMR_TXF	= 0x00010000,	/* Documented for R-Car Gen3 only */
> >  	ECMR_RXF	= 0x00020000,
> >  	ECMR_PFR	= 0x00040000,
> >  	ECMR_ZPF	= 0x00080000,	/* Documented for R-Car Gen3 only */
> >  	ECMR_RZPF	= 0x00100000,
> >  	ECMR_DPAD	= 0x00200000,
> > +	CXR20_CXSER	= 0x00400000,	/* Documented for RZ/G2L only */
> 
>     Ditto.
> 
> >  	ECMR_RCSC	= 0x00800000,
> > +	CXR20_TCPT	= 0x01000000,	/* Documented for RZ/G2L only */
> > +	CXR20_RCPT	= 0x02000000,	/* Documented for RZ/G2L only */
> 
>    Ditto.
> 
> [...]
> > @@ -823,6 +845,7 @@ enum ECSR_BIT {
> >  	ECSR_MPD	= 0x00000002,
> >  	ECSR_LCHNG	= 0x00000004,
> >  	ECSR_PHYI	= 0x00000008,
> > +	ECSR_RFRI	= 0x00000010,	/* Documented for RZ/G2L only */
> 
>    Not PFRI?
>    Also, my R-Car gen2/3 manual dociment the bit.

You are correct, it is PFRI.

> 
> [...]
> > @@ -862,6 +885,14 @@ enum GECMR_BIT {
> >  	GECMR_SPEED_1000 = 0x00000001,
> >  };
> >
> > +/* GECMR */
> > +enum RGETH_GECMR_BIT {
> > +	RGETH_GECMR_SPEED	= 0x00000030,
> > +	RGETH_GECMR_SPEED_10	= 0x00000000,
> > +	RGETH_GECMR_SPEED_100	= 0x00000010,
> > +	RGETH_GECMR_SPEED_1000	= 0x00000020,
> 
>    Why not place those values in the existing GECMR *enum* with the
> necessary annotations?
OK.

> Also I think the prefixes should be GETH, not RGETH -- RAVB is the
> EtherAVB driver's name, not the actual hardware...
OK.

> 
> [...]
> > @@ -956,8 +1035,11 @@ enum RAVB_QUEUE {
> >
> >  #define RX_BUF_SZ	(2048 - ETH_FCS_LEN + sizeof(__sum16))
> >
> > +#define RGETH_RCV_BUFF_MAX 8192
> > +#define RGETH_RCV_DESCRIPTOR_DATA_SIZE 4080
> 
>    Please shorten DESCRIPTOR to DESC. And RCV_ should probably be renemed
> to R?..
OK.

> 
> [...]> @@ -1040,6 +1123,11 @@ struct ravb_private {
> >  	unsigned txcidm:1;		/* TX Clock Internal Delay Mode */
> >  	unsigned rgmii_override:1;	/* Deprecated rgmii-*id behavior */
> >  	int num_tx_desc;		/* TX descriptors per packet */
> > +
> > +	int duplex;
> > +	struct ravb_rx_desc *rgeth_rx_ring[NUM_RX_QUEUE];
> > +	struct sk_buff *rxtop_skb;
> > +	struct reset_control *rstc;
> 
>    I'd have apreciated some comments here... I don't quite understand what
> the first 3 are for...

For supporting halfduplex/full_duplex, the descriptor/datastructur for handling normal desc instead of extended desc in RX.

> 
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 7e6feda59f4a..e28a63de553d 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
> > @@ -82,6 +83,23 @@ static int ravb_config(struct net_device *ndev)
> >  	return error;
> >  }
> >
> > +static void rgeth_set_rate(struct net_device *ndev)
> 
>    What about ravb_set_rate_rzgl? I think we use this convention in
> sh_eth.c...
OK.
> 
> [...]
> > @@ -217,6 +235,29 @@ static int ravb_tx_free(struct net_device *ndev,
> > int q, bool free_txed_only)  }
> >
> >  /* Free skb's and DMA buffers for Ethernet AVB */
> > +static void rgeth_ring_free_ex(struct net_device *ndev, int q)
> 
>    It definitely should be called ravb_rx_ring_free_rgeth(), I think...
OK. 
> 
> > +{
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +	int ring_size;
> > +	int i;
> > +
> > +	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> > +		struct ravb_rx_desc *desc = &priv->rgeth_rx_ring[q][i];
> > +
> > +		if (!dma_mapping_error(ndev->dev.parent,
> > +				       le32_to_cpu(desc->dptr)))
> > +			dma_unmap_single(ndev->dev.parent,
> > +					 le32_to_cpu(desc->dptr),
> > +					 RGETH_RCV_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-
> >rgeth_rx_ring[q],
> > +			  priv->rx_desc_dma[q]);
> > +	priv->rgeth_rx_ring[q] = NULL;
> > +}
> > +
> >  static void ravb_ring_free_ex(struct net_device *ndev, int q)  {
> >  	struct ravb_private *priv = netdev_priv(ndev); @@ -247,8 +288,12 @@
> > static void ravb_ring_free(struct net_device *ndev, int q)
> >  	int ring_size;
> >  	int i;
> >
> > -	if (priv->rx_ring[q]) {
> > -		ravb_ring_free_ex(ndev, q);
> > +	if (priv->chip_id == RZ_G2L) {
> > +		if (priv->rgeth_rx_ring[q])
> > +			rgeth_ring_free_ex(ndev, q);
> > +	} else {
> > +		if (priv->rx_ring[q])
> > +			ravb_ring_free_ex(ndev, q);
> 
>    I definitely don't like that _ex suffix...
OK.
> 
> [...]
> > @@ -281,6 +326,36 @@ static void ravb_ring_free(struct net_device
> > *ndev, int q)  }
> >
> >  /* Format skb and descriptor buffer for Ethernet AVB */
> > +static void rgeth_ring_format_ex(struct net_device *ndev, int q)
> 
>     How about ravb_rx_ring_format_geth()?
OK.
> 
> > +{
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +	struct ravb_rx_desc *rx_desc;
> > +	int rx_ring_size = sizeof(*rx_desc) * priv->num_rx_ring[q];
> > +	dma_addr_t dma_addr;
> > +	int i;
> > +
> > +	memset(priv->rgeth_rx_ring[q], 0, rx_ring_size);
> > +	/* Build RX ring buffer */
> > +	for (i = 0; i < priv->num_rx_ring[q]; i++) {
> > +		/* RX descriptor */
> > +		rx_desc = &priv->rgeth_rx_ring[q][i];
> > +		rx_desc->ds_cc = cpu_to_le16(RGETH_RCV_DESCRIPTOR_DATA_SIZE);
> 
>     Mhm, I think we should further parametrize the RX data in order to
> save the code duplication...

As per Geert's suggestion, I have prototyped driver with structure replacing
data with values, features and function pointers.

> 
> > +		dma_addr = dma_map_single(ndev->dev.parent, priv-
> >rx_skb[q][i]->data,
> > +					  RGETH_RCV_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->rgeth_rx_ring[q][i];
> > +	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
> > +	rx_desc->die_dt = DT_LINKFIX; /* type */ }
> > +
> >  static void ravb_ring_format_ex(struct net_device *ndev, int q)  {
> >  	struct ravb_private *priv = netdev_priv(ndev); @@ -326,7 +401,10 @@
> > static void ravb_ring_format(struct net_device *ndev, int q)
> >  	priv->dirty_tx[q] = 0;
> >
> >  	/* Build RX ring buffer */
> > -	ravb_ring_format_ex(ndev, q);
> > +	if (priv->chip_id == RZ_G2L)
> > +		rgeth_ring_format_ex(ndev, q);
> > +	else
> > +		ravb_ring_format_ex(ndev, q);
> 
>    ... so that we don't have alike code snippets anymore...

Yes, it will be part of function pointers.

> 
> [...]
> > @@ -356,7 +434,7 @@ static void ravb_ring_format(struct net_device
> > *ndev, int q)  static int ravb_ring_init(struct net_device *ndev, int
> > q)  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> > -	size_t skb_sz = RX_BUF_SZ;
> > +	size_t skb_sz = (priv->chip_id == RZ_G2L) ? RGETH_RCV_BUFF_MAX :
> > +RX_BUF_SZ;
> 
>     Should be also parametrized...
Yes, it will be part of values.
> 
> [...]
> > @@ -414,6 +503,45 @@ static int ravb_ring_init(struct net_device
> > *ndev, int q)  }
> >
> >  /* E-MAC init function */
> > +static void rgeth_emac_init_ex(struct net_device *ndev) {
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +
> > +	/* Receive frame limit set register */
> > +	ravb_write(ndev, RGETH_RCV_BUFF_MAX + ETH_FCS_LEN, RFLR);
> > +
> > +	/* PAUSE prohibition */
> > +	ravb_write(ndev, ECMR_ZPF | (priv->duplex ? ECMR_DM : 0) |
> > +			 ECMR_TE | ECMR_RE | CXR20_RCPT |
> > +			 ECMR_TXF | ECMR_RXF | ECMR_PRM, ECMR);
> > +
> > +	rgeth_set_rate(ndev);
> > +
> > +	/* Set MAC address */
> > +	ravb_write(ndev,
> > +		   (ndev->dev_addr[0] << 24) | (ndev->dev_addr[1] << 16) |
> > +		   (ndev->dev_addr[2] << 8)  | (ndev->dev_addr[3]), MAHR);
> > +	ravb_write(ndev,
> > +		   (ndev->dev_addr[4] << 8)  | (ndev->dev_addr[5]), MALR);
> > +
> > +	/* E-MAC status register clear */
> > +	ravb_write(ndev, ECSR_ICD | ECSR_LCHNG | ECSR_RFRI, ECSR);
> > +	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);
> > +
> > +	/* E-MAC interrupt enable register */
> > +	ravb_write(ndev, ECSIPR_ICDIP, ECSIPR);
> > +
> > +	ravb_write(ndev, ravb_read(ndev, CXR31)
> > +			 & ~CXR31_SEL_LINK1, CXR31);
> > +	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID) {
> > +		ravb_write(ndev, ravb_read(ndev, CXR31)
> > +			 | CXR31_SEL_LINK0, CXR31);
> > +	} else {
> > +		ravb_write(ndev, ravb_read(ndev, CXR31)
> > +			 & ~CXR31_SEL_LINK0, CXR31);
> > +	}
> > +}
> 
>    The more I look at your patches, the more I think Geert's suggestion to
> mimic the sh_eth's approach to handling the SoC differencies is worth
> using in this driver as well...

Yes I have incorporated Geert's suggestion and tested the changes on RZ/G2[M,N] and RZ/G1N.
The code is under internal review now.

> 
> [...]
> > @@ -442,10 +570,41 @@ static void ravb_emac_init_ex(struct net_device
> > *ndev)
> [...]
> >  /* Device init function for Ethernet AVB */
> > +static void rgeth_dmac_init_ex(struct net_device *ndev) {
> > +	/* Set AVB RX */
> > +	ravb_write(ndev, 0x60000000, RCR);
> 
>    What those (undocumented?) bits mean, I wonder?

It is basically sets the settings related to reception for the DMA(Reception FIFO critical level) default
Value is 0x6000.

> 
> [...]
> > @@ -545,6 +708,23 @@ static void ravb_get_tx_tstamp(struct net_device
> *ndev)
> >  	}
> >  }
> >
> > +static void rgeth_rx_csum(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;
> > +}
> 
>    So does GbEth device sumpport checksuming or not?

The TOE block calculates Checksum by hardware.

> 
> [...]
> > @@ -561,6 +741,143 @@ static void ravb_rx_csum(struct sk_buff *skb)  }
> >
> >  /* Packet receive function for Ethernet AVB */
> > +struct sk_buff *rgeth_get_skb(struct net_device *ndev, int q, int
> > +entry,
> 
>    Not *static*?
OK. Will fix it.

> 
> > +			      struct ravb_rx_desc *desc)
> > +{
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +	struct sk_buff *skb;
> > +
> > +	skb = priv->rx_skb[q][entry];
> > +	priv->rx_skb[q][entry] = NULL;
> > +	dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
> > +			 ALIGN(RGETH_RCV_BUFF_MAX, 16), DMA_FROM_DEVICE);
> > +
> > +	return skb;
> > +}
> > +
> > +static bool rgeth_rx_ex(struct net_device *ndev, int *quota, int q) {
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +	int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> > +	int boguscnt = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv-
> >cur_rx[q];
> > +	struct net_device_stats *stats = &priv->stats[q];
> > +	struct ravb_rx_desc *desc;
> > +	struct sk_buff *skb;
> > +	dma_addr_t dma_addr;
> > +	u8  desc_status;
> > +	u8  die_dt;
> > +	u16 pkt_len;
> > +	int limit;
> > +
> > +	boguscnt = min(boguscnt, *quota);
> > +	limit = boguscnt;
> > +	desc = &priv->rgeth_rx_ring[q][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;
> > +			if (die_dt == DT_FSINGLE) {
> 
>    Why not *switch*?
> 
> > +				skb = rgeth_get_skb(ndev, q, entry, desc);
> > +				skb_put(skb, pkt_len);
> > +				skb->protocol = eth_type_trans(skb, ndev);
> > +				if (ndev->features & NETIF_F_RXCSUM)
> > +					rgeth_rx_csum(skb);
> > +				napi_gro_receive(&priv->napi[q], skb);
> > +				stats->rx_packets++;
> > +				stats->rx_bytes += pkt_len;
> > +			} else if (die_dt == DT_FSTART) {
> > +				priv->rxtop_skb = rgeth_get_skb(ndev, q, entry,
> desc);
> > +				skb_put(priv->rxtop_skb, pkt_len);
> > +			} else if (die_dt == DT_FMID) {
> > +				skb = rgeth_get_skb(ndev, q, entry, desc);
> > +				skb_copy_to_linear_data_offset(priv->rxtop_skb,
> > +							       priv->rxtop_skb->len,
> > +							       skb->data,
> > +							       pkt_len);
> > +				skb_put(priv->rxtop_skb, pkt_len);
> > +				dev_kfree_skb(skb);
> > +			} else if (die_dt == DT_FEND) {
> > +				skb = rgeth_get_skb(ndev, q, entry, desc);
> > +				skb_copy_to_linear_data_offset(priv->rxtop_skb,
> > +							       priv->rxtop_skb->len,
> > +							       skb->data,
> > +							       pkt_len);
> > +				skb_put(priv->rxtop_skb, pkt_len);
> > +				dev_kfree_skb(skb);
> > +				priv->rxtop_skb->protocol =
> > +					eth_type_trans(priv->rxtop_skb, ndev);
> > +				if (ndev->features & NETIF_F_RXCSUM)
> > +					rgeth_rx_csum(skb);
> > +				napi_gro_receive(&priv->napi[q],
> > +						 priv->rxtop_skb);
> > +				stats->rx_packets++;
> > +				stats->rx_bytes += priv->rxtop_skb->len;
> > +			}
> 
>    So why it's necessary to handle the multidecriptor packets?

On RZ/G2L we have normal descriptor compared to extended descriptor packets.

> 
> [...]
> > @@ -678,7 +995,10 @@ static bool ravb_rx_ex(struct net_device *ndev,
> > int *quota, int q)
> >
> >  static bool ravb_rx(struct net_device *ndev, int *quota, int q)  {
> > -	return ravb_rx_ex(ndev, quota, q);
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +
> > +	return (priv->chip_id == RZ_G2L) ?
> > +		rgeth_rx_ex(ndev, quota, q) : ravb_rx_ex(ndev, quota, q);
> 
>    Definitely better with the pre-init'ed function pointers if needed at
> all)...

Agreed.

> 
> [...]
> > @@ -696,11 +1016,15 @@ static void ravb_rcv_snd_enable(struct
> > net_device *ndev)
> >  /* function for waiting dma process finished */  static int
> > ravb_stop_dma(struct net_device *ndev)  {
> > +	struct ravb_private *priv = netdev_priv(ndev);
> >  	int error;
> >
> >  	/* Wait for stopping the hardware TX process */
> > -	error = ravb_wait(ndev, TCCR,
> > -			  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0);
> > +	if (priv->chip_id == RZ_G2L)
> > +		error = ravb_wait(ndev, TCCR, TCCR_TSRQ0, 0);
> 
>    Only a single TX queue?

Correct.

> 
> > +	else
> > +		error = ravb_wait(ndev, TCCR,
> > +				  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 |
> TCCR_TSRQ3, 0);
> 
>    This is better expressed thru a mask in the private data...

Taken care using feature bit.

> 
> [...]
> > @@ -823,11 +1147,13 @@ static bool ravb_queue_interrupt(struct
> > net_device *ndev, int q)
> >
> >  static bool ravb_timestamp_interrupt(struct net_device *ndev)  {
> > +	struct ravb_private *priv = netdev_priv(ndev);
> >  	u32 tis = ravb_read(ndev, TIS);
> >
> >  	if (tis & TIS_TFUF) {
> >  		ravb_write(ndev, ~(TIS_TFUF | TIS_RESERVED), TIS);
> > -		ravb_get_tx_tstamp(ndev);
> > +		if (priv->chip_id != RZ_G2L)
> > +			ravb_get_tx_tstamp(ndev);
> 
>    So, no TX pakcet timestaming?..

Yes, That is correct.
> 
> [...]
> > @@ -872,7 +1198,7 @@ static irqreturn_t ravb_interrupt(int irq, void
> *dev_id)
> >  	}
> >
> >  	/* gPTP interrupt status summary */
> > -	if (iss & ISS_CGIS) {
> > +	if (priv->chip_id != RZ_G2L && (iss & ISS_CGIS)) {
> 
>    ...and no gPTP IRQ?
Yes that is correct.

> 
> >  		ravb_ptp_interrupt(ndev);
> >  		result = IRQ_HANDLED;
> >  	}
> > @@ -947,12 +1273,20 @@ static int ravb_poll(struct napi_struct *napi,
> int budget)
> >  	int q = napi - priv->napi;
> >  	int mask = BIT(q);
> >  	int quota = budget;
> > +	int entry;
> > +	struct ravb_rx_desc *desc;
> 
>    The lines with the local variables should be sotrted by length, the
> longer going first (so caleld reverse Xmas tree)...
> 
OK.

> [...]
> > @@ -1082,15 +1440,23 @@ static int ravb_phy_init(struct net_device
> *ndev)
> >  		netdev_info(ndev, "limited PHY to 100Mbit/s\n");
> >  	}
> >
> > -	/* 10BASE, Pause and Asym Pause is not supported */
> > -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> > -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
> > -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
> > -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
> > +	if (priv->chip_id == RZ_G2L) {
> 
>   Why not *switch*?

This replaced with feature bits.

> 
> cxr35> +		if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID)
> > +			ravb_write(ndev, ravb_read(ndev, CXR35) |
> CXR35_SEL_MODIN, CXR35);
> > +		else if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII)
> > +			ravb_write(ndev, 0x3E80000, CXR35);
> > +	} else {
> >
> [...]
> > @@ -1133,6 +1499,24 @@ static void ravb_set_msglevel(struct net_device
> *ndev, u32 value)
> >  	priv->msg_enable = value;
> >  }
> >
> > +static const char rgeth_gstrings_stats[][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",
> > +};
> 
>    I think we can get out without duplicating the stgring arrays...
> 
> [...]
> > @@ -1669,6 +2079,20 @@ static struct net_device_stats
> *ravb_get_stats(struct net_device *ndev)
> >  	stats0 = &priv->stats[RAVB_BE];
> >  	stats1 = &priv->stats[RAVB_NC];
> >
> > +	if (priv->chip_id == RZ_G2L) {
> > +		nstats->tx_dropped += ravb_read(ndev, TROCR);
> > +		ravb_write(ndev, 0, TROCR);	/* (write clear) */
> 
>    Why duplicated with R-Car gen3?

It is addressed in later version with feature bit.

> 
> > +		nstats->collisions += ravb_read(ndev, CDCR);
> > +		ravb_write(ndev, 0, CDCR);	/* (write clear) */
> > +		nstats->tx_carrier_errors += ravb_read(ndev, LCCR);
> > +		ravb_write(ndev, 0, LCCR);	/* (write clear) */
> > +
> > +		nstats->tx_carrier_errors += ravb_read(ndev, CERCR);
> > +		ravb_write(ndev, 0, CERCR);	/* (write clear) */
> > +		nstats->tx_carrier_errors += ravb_read(ndev, CEECR);
> > +		ravb_write(ndev, 0, CEECR);	/* (write clear) */
> > +	}
> > +
> >  	if (priv->chip_id == RCAR_GEN3) {
> 
>    Must be else if here...
> 
> [...]
> > @@ -1899,6 +2325,44 @@ static int ravb_set_features(struct net_device
> *ndev,
> >  	return 0;
> >  }
> >
> > +static int rgeth_set_features(struct net_device *ndev,
> > +			      netdev_features_t features)
> > +{
> > +	int error;
> > +	unsigned int reg = 0;
> > +	netdev_features_t changed = features ^ ndev->features;
> 
>    Reverse Xmas tree, please.
OK.
> 
> > +
> > +	reg = ravb_read(ndev, CSR0);
> > +
> > +	ravb_write(ndev, ravb_read(ndev, CSR0) & ~(CSR0_RPE | CSR0_TPE),
> > +CSR0);
> 
>    Why read CSR0 twice?

Will fix it.

> 
> [...]
> > @@ -1930,7 +2408,10 @@ static int ravb_mdio_init(struct ravb_private
> *priv)
> >  		return -ENOMEM;
> >
> >  	/* Hook up MII support for ethtool */
> > -	priv->mii_bus->name = "ravb_mii";
> > +	if (priv->chip_id == RZ_G2L)
> > +		priv->mii_bus->name = "rgeth_mii";
> > +	else
> > +		priv->mii_bus->name = "ravb_mii";
> 
>    Hm... I don't think we need this.

Now it is part of data. Will use "ravb_mii" for all SoC's. 

> 
> [...]
> > @@ -1965,6 +2446,7 @@ static const struct of_device_id
> ravb_match_table[] = {
> >  	{ .compatible = "renesas,etheravb-rcar-gen2", .data = (void
> *)RCAR_GEN2 },
> >  	{ .compatible = "renesas,etheravb-r8a7795", .data = (void
> *)RCAR_GEN3 },
> >  	{ .compatible = "renesas,etheravb-rcar-gen3", .data = (void
> > *)RCAR_GEN3 },
> > +	{ .compatible = "renesas,rzg2l-gether", .data = (void *)RZ_G2L },
> 
>    Mhm, we reserve the right to call the hardware "gether" to the to the
> rdriver...
> Would  the "renesas,rzg2l-gbeth" string fit?

OK.

> 
> [...]
> > @@ -2082,20 +2565,11 @@ static int ravb_probe(struct platform_device
> *pdev)
> >  		return -EINVAL;
> >  	}
> >
> > -	/* Get base address */
> > -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -	if (!res) {
> > -		dev_err(&pdev->dev, "invalid resource\n");
> > -		return -EINVAL;
> > -	}
> > -
> >  	ndev = alloc_etherdev_mqs(sizeof(struct ravb_private),
> >  				  NUM_TX_QUEUE, NUM_RX_QUEUE);
> >  	if (!ndev)
> >  		return -ENOMEM;
> >
> > -	/* The Ether-specific entries in the device structure. */
> > -	ndev->base_addr = res->start;
> >
> >  	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
> >
> > @@ -2104,11 +2578,19 @@ static int ravb_probe(struct platform_device
> *pdev)
> >  	priv = netdev_priv(ndev);
> >  	priv->chip_id = chip_id;
> >
> > -	ndev->features = NETIF_F_RXCSUM;
> > -	ndev->hw_features = NETIF_F_RXCSUM;
> > -
> > -	pm_runtime_enable(&pdev->dev);
> > -	pm_runtime_get_sync(&pdev->dev);
> > +	if (chip_id == RZ_G2L) {
> > +		ndev->hw_features |= (NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
> > +		priv->rstc = devm_reset_control_get(&pdev->dev, NULL);
> > +		if (IS_ERR(priv->rstc)) {
> > +			dev_err(&pdev->dev, "failed to get cpg reset\n");
> > +			free_netdev(ndev);
> > +			return PTR_ERR(priv->rstc);
> > +		}
> > +		reset_control_deassert(priv->rstc);
> 
>    This asks for the serpate patch.
Done already.
> 
> [...]
> > @@ -2120,18 +2602,24 @@ static int ravb_probe(struct platform_device
> *pdev)
> >  	}
> >  	ndev->irq = irq;
> >
> > +	pm_runtime_enable(&pdev->dev);
> > +	pm_runtime_get_sync(&pdev->dev);
> > +
> >  	priv->ndev = ndev;
> >  	priv->pdev = pdev;
> >  	priv->num_tx_ring[RAVB_BE] = BE_TX_RING_SIZE;
> >  	priv->num_rx_ring[RAVB_BE] = BE_RX_RING_SIZE;
> >  	priv->num_tx_ring[RAVB_NC] = NC_TX_RING_SIZE;
> >  	priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
> > -	priv->addr = devm_ioremap_resource(&pdev->dev, res);
> > +	priv->addr = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> 
>    Separate patch, please.
> 
> [...]
> > @@ -2181,30 +2671,39 @@ static int ravb_probe(struct platform_device
> > *pdev)
> [...]
> >  	/* Set AVB config mode */
> >  	ravb_set_config_mode(ndev);
> >
> > -	/* Set GTI value */
> > -	error = ravb_set_gti(ndev);
> > -	if (error)
> > -		goto out_disable_refclk;
> > +	if (priv->chip_id != RZ_G2L) {
> > +		/* Set GTI value */
> > +		error = ravb_set_gti(ndev);
> > +		if (error)
> > +			goto out_disable_refclk;
> >
> > -	/* Request GTI loading */
> > -	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> > +		/* Request GTI loading */
> > +		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> >
> > -	if (priv->chip_id == RCAR_GEN3) {
> > -		ravb_parse_delay_mode(np, ndev);
> > -		ravb_set_delay_mode(ndev);
> > +		if (priv->chip_id == RCAR_GEN3) {
> > +			ravb_parse_delay_mode(np, ndev);
> > +			ravb_set_delay_mode(ndev);
> > +		}
> >  	}
> 
>    Ugh... needs to be untagled too.
> 
>    Went thru the large patch at last! If you need help addressing
> comments, I can probably make the driver patches using data
> parametrization vs chip_id.

As per Geert's review comment as well as you are also agreeing as per review comment
The driver needs to replace data with a structure with values, features and function pointers.

I have done the code changes and it tested the changes on RZ/G2[M,N,L] and RZ/G1N.
The code is in internal review. I should be able to send this patch with in couple
Of days with fixing some of extra suggestions pointed out by you.

Thanks for your suggestions.

Regards,
Biju


> 
> [...]
> 
> MBR, Sergei

Re: [PATCH/RFC 0/2] Add Gigabit Ethernet driver support

[Sergei Shtylyov]

On 7/14/21 5:54 PM, Biju Das wrote:

> The DMAC and EMAC blocks of Gigabit Ethernet IP is almost
> similar to Ethernet AVB.
> 
> The Gigabit Etherner IP consists of Ethernet controller (E-MAC),
> Internal TCP/IP Offload Engine (TOE) and Dedicated Direct memory
> access controller (DMAC).

   Do you have a mnaual for this SoC? I haven't found any...

> With few canges in driver, we can support Gigabit ethernet driver as well.
> I have prototyped the driver and tested with renesas-drivers master branch.
> 
> Please share your valuable comments.

   Commented on the patch 1/2 already, will continue with patch 2/2 review
tomorrow...

[...]

MBR, Sergei