RE: [RFC/PATCH 06/18] ravb: Add multi_tsrq to struct ravb_hw_info

Hi Sergei,

Subject: RE: [RFC/PATCH 06/18] ravb: Add multi_tsrq to struct ravb_hw_info

Hi Sergei,

Thanks for the feedback.

quoted

Subject: Re: [RFC/PATCH 06/18] ravb: Add multi_tsrq to struct
ravb_hw_info

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

quoted

R-Car AVB-DMAC has 4 Transmit start Request queues, whereas RZ/G2L
has only 1 Transmit start Request queue(Best Effort)

Add a multi_tsrq hw feature bit to struct ravb_hw_info to enable
this only for R-Car. This will allow us to add single TSRQ support
for RZ/G2L.

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

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

b/drivers/net/ethernet/renesas/ravb.h
index bb92469d770e..c043ee555be4 100644

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

@@ -1006,6 +1006,7 @@ struct ravb_hw_info {
 	unsigned multi_irqs:1;		/* AVB-DMAC and E-MAC has multiple

irqs */

quoted

 	unsigned no_gptp:1;		/* AVB-DMAC does not support gPTP

feature */

quoted

 	unsigned ccc_gac:1;		/* AVB-DMAC has gPTP support active in

config mode */

quoted

+	unsigned multi_tsrq:1;		/* AVB-DMAC has MULTI TSRQ */

   Maybe 'single_tx_q' instead?

Since it is called transmit start request queue, it is better to be named
as single_tsrq to match with hardware manual and I will update the comment
with "GbEthernet DMAC has single TSRQ"
Please let me know are you ok with it. Other wise I would like to use
existing name.

On the next revision as you proposed for [1],
I will use a u32 tsrq, instead of bit, there by we can avoid a check.

https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210923140813.13541-12-biju.das.jz@bp.renesas.com/

quoted

quoted

 };

 struct ravb_private {

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

b/drivers/net/ethernet/renesas/ravb_main.c
index 8663d83507a0..d37d73f6d984 100644

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

@@ -776,11 +776,17 @@ 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);
+	const struct ravb_hw_info *info = priv->info;
 	int error;

 	/* Wait for stopping the hardware TX process */
-	error = ravb_wait(ndev, TCCR,
-			  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3, 0);
+	if (info->multi_tsrq)
+		error = ravb_wait(ndev, TCCR,
+				  TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 |

TCCR_TSRQ3, 0);

quoted

+	else
+		error = ravb_wait(ndev, TCCR, TCCR_TSRQ0, 0);

   Aren't the TSRQ1/2/3 bits reserved on RZ/G2L? If so, this new flag
adds a little value, I think... unless you plan to use this flag
further in the series?

It will be confusing for RZ/G2L users. HW manual does not describes
TSRQ1/2/3 and we are writing undocumented registers which is reserved.

Tomorrow it can happen that this reserved bits(90% it will not happen)
will be used for describing something else.

It is unsafe to use reserved bits. Are you agreeing with this?

As per the above discussion, we can replace the above check as you proposed for [1]

error = ravb_wait(ndev, TCCR, info->tsrq, 0);

[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210923140813.13541-12-biju.das.jz@bp.renesas.com/

regards,
Biju