Hi Sergei,

Subject: Re: [RFC/PATCH 03/18] ravb: Initialize GbEthernet dmac

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

quoted

Initialize GbEthernet dmac found on RZ/G2L SoC.

   DMAC (or AVB-DMAC).

OK will change it to DMAC. AVB-DMAC is not applicable for GbEthernet.

quoted

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

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

[...]

quoted

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

b/drivers/net/ethernet/renesas/ravb_main.c
index 2422e74d9b4f..54c4d31a6950 100644

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

@@ -83,6 +83,11 @@ static int ravb_config(struct net_device *ndev)
 	return error;
 }

+static void ravb_rgeth_set_rate(struct net_device *ndev)

   What does 'rgeth' stand for? And why not the trailing part of the name,
like the other cases?

'rgeth' stands for Renesas Gigabit Ethernet like 'ravb' renesas AVB.
OK it can be moved to trailing part.

quoted

+{
+	/* Place holder */
+}
+
 static void ravb_set_rate(struct net_device *ndev)  {
 	struct ravb_private *priv = netdev_priv(ndev); @@ -217,6 +222,11 @@
static int ravb_tx_free(struct net_device *ndev, int q, bool

free_txed_only)

quoted

 	return free_num;
 }

+static void ravb_rx_ring_free_rgeth(struct net_device *ndev, int q)

   rgeth? 

Are you ok with this function name right? Since based on previous comment
to trailing part.

quoted

+{
+	/* Place holder */
+}
+
 static void ravb_rx_ring_free(struct net_device *ndev, int q)  {
 	struct ravb_private *priv = netdev_priv(ndev); @@ -283,6 +293,11 @@
static void ravb_ring_free(struct net_device *ndev, int q)
 	priv->tx_skb[q] = NULL;
 }

+static void ravb_rx_ring_format_rgeth(struct net_device *ndev, int q)

   rgeth?

Are you ok with this function name right? Since based on previous comment
to trailing part.

quoted

+{
+	/* Place holder */
+}
+
 static void ravb_rx_ring_format(struct net_device *ndev, int q)  {
 	struct ravb_private *priv = netdev_priv(ndev); @@ -356,6 +371,12 @@
static void ravb_ring_format(struct net_device *ndev, int q)
 	desc->dptr = cpu_to_le32((u32)priv->tx_desc_dma[q]);
 }

+static void *ravb_rgeth_alloc_rx_desc(struct net_device *ndev, int q)

   Again, why rgeth is not in the symbol's tail?

Ok will move to the tail.

quoted

+{
+	/* Place holder */
+	return NULL;
+}
+
 static void *ravb_alloc_rx_desc(struct net_device *ndev, int q)  {
 	struct ravb_private *priv = netdev_priv(ndev); @@ -426,6 +447,11 @@
static int ravb_ring_init(struct net_device *ndev, int q)
 	return -ENOMEM;
 }

+static void ravb_rgeth_emac_init(struct net_device *ndev)

   Same here...

OK will move to the tail.

quoted

+{
+	/* Place holder */
+}
+
 static void ravb_rcar_emac_init(struct net_device *ndev)  {
 	/* Receive frame limit set register */ @@ -461,7 +487,32 @@ static
void ravb_emac_init(struct net_device *ndev)
 	info->emac_init(ndev);
 }

-static void ravb_rcar_dmac_init(struct net_device *ndev)
+static void ravb_dmac_init_rgeth(struct net_device *ndev) {
+	/* Set AVB RX */
+	ravb_write(ndev, 0x60000000, RCR);
+
+	/* Set Max Frame Length (RTC) */
+	ravb_write(ndev, 0x7ffc0000 | RGETH_RX_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);
+}

   Ah, so 'rgeth' stands for GbEthernet... why not 'gbeth' then?

'rgeth" is Renesas Gigabit ethernet. If needed I can change it to 'gbeth' if there is no 
Objection from others.

[...]

quoted

@@ -579,6 +630,14 @@ static void ravb_rx_csum(struct sk_buff *skb)
 	skb_trim(skb, skb->len - sizeof(__sum16));  }

+/* Packet receive function for Gigabit Ethernet */ static bool
+ravb_rgeth_rx(struct net_device *ndev, int *quota, int q) {
+	/* Place holder */
+	return true;
+}
+
+/* Packet receive function for Ethernet AVB */
 static bool ravb_rcar_rx(struct net_device *ndev, int *quota, int q)

{
 	struct ravb_private *priv = netdev_priv(ndev); @@ -1918,6 +1977,13

@@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
 	spin_unlock_irqrestore(&priv->lock, flags);  }

+static int ravb_set_features_rgeth(struct net_device *ndev,
+				   netdev_features_t features)
+{
+	/* Place holder */
+	return 0;
+}
+
 static int ravb_set_features_rcar(struct net_device *ndev,
 				  netdev_features_t features)
 {

@@ -2007,7 +2073,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info

= {

quoted

 	.receive = ravb_rcar_rx,
 	.set_rate = ravb_set_rate,
 	.set_feature = ravb_set_features_rcar,
-	.dmac_init = ravb_rcar_dmac_init,
+	.dmac_init = ravb_dmac_init_rcar,
 	.emac_init = ravb_rcar_emac_init,
 	.gstrings_stats = ravb_gstrings_stats,
 	.gstrings_size = sizeof(ravb_gstrings_stats), @@ -2028,7 +2094,7 @@
static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.receive = ravb_rcar_rx,
 	.set_rate = ravb_set_rate,
 	.set_feature = ravb_set_features_rcar,
-	.dmac_init = ravb_rcar_dmac_init,
+	.dmac_init = ravb_dmac_init_rcar,
 	.emac_init = ravb_rcar_emac_init,
 	.gstrings_stats = ravb_gstrings_stats,
 	.gstrings_size = sizeof(ravb_gstrings_stats), @@ -2039,12 +2105,24

@@ static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.aligned_tx = 1,
 };

+static const struct ravb_hw_info rgeth_hw_info = {
+	.rx_ring_free = ravb_rx_ring_free_rgeth,
+	.rx_ring_format = ravb_rx_ring_format_rgeth,
+	.alloc_rx_desc = ravb_rgeth_alloc_rx_desc,
+	.receive = ravb_rgeth_rx,
+	.set_rate = ravb_rgeth_set_rate,
+	.set_feature = ravb_set_features_rgeth,
+	.dmac_init = ravb_dmac_init_rgeth,
+	.emac_init = ravb_rgeth_emac_init,
+};
+>  static const struct of_device_id ravb_match_table[] = {
 	{ .compatible = "renesas,etheravb-r8a7790", .data =

&ravb_gen2_hw_info },

quoted

 	{ .compatible = "renesas,etheravb-r8a7794", .data =

&ravb_gen2_hw_info },

quoted

 	{ .compatible = "renesas,etheravb-rcar-gen2", .data =

&ravb_gen2_hw_info },

quoted

 	{ .compatible = "renesas,etheravb-r8a7795", .data =

&ravb_gen3_hw_info },

quoted

 	{ .compatible = "renesas,etheravb-rcar-gen3", .data =
&ravb_gen3_hw_info },
+	{ .compatible = "renesas,rzg2l-gbeth", .data = &rgeth_hw_info },

    Mhm, I thought this parch should come lst of the series, without any
placeholders...

It is OK. 

Like you commented on https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210923140813.13541-5-biju.das.jz@bp.renesas.com/, why there is a patch required for this feature bits?

Cheers,
Biju

[...]

MBR, Sergey