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

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:

quoted

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>

[...]

quoted

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.

[...]

quoted

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.

quoted

+{
+	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);

[...]

quoted

@@ -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?

[..]

quoted

@@ -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... :-)

[...]

quoted

@@ -422,29 +440,15 @@ static void ravb_emac_init(struct net_device

*ndev)

quoted

 	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.

quoted

+}
+
 /* 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...

[...]

quoted

@@ -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)

quoted

 	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.

[...]

quoted

@@ -920,7 +954,7 @@ static int ravb_poll(struct napi_struct *napi, int

budget)

quoted

 	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.

[...]

quoted

@@ -2059,17 +2094,22 @@ static int ravb_probe(struct platform_device

*pdev)

quoted

 	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.

quoted

 	if (chip_id == RCAR_GEN3)
 		irq = platform_get_irq_byname(pdev, "ch22");
 	else

[...]

quoted

@@ -2257,7 +2292,7 @@ static int ravb_remove(struct platform_device

*pdev)

quoted

 	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)

quoted

 	/* 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