RE: [RFC/PATCH 12/18] ravb: Add timestamp to struct ravb_hw_info

Hi Sergei,

Subject: Re: [RFC/PATCH 12/18] ravb: Add timestamp to struct ravb_hw_info

On 9/26/21 9:34 AM, Biju Das wrote:

quoted

quoted

-----Original Message-----
Subject: Re: [RFC/PATCH 12/18] ravb: Add timestamp to struct
ravb_hw_info

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

quoted

R-Car AVB-DMAC supports timestamp feature.
Add a timestamp hw feature bit to struct ravb_hw_info to add this
feature only for R-Car.

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 | 68
+++++++++++++++---------
 2 files changed, 45 insertions(+), 25 deletions(-)

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

b/drivers/net/ethernet/renesas/ravb.h
index ab4909244276..2505de5d4a28 100644

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

@@ -1034,6 +1034,7 @@ struct ravb_hw_info {
 	unsigned mii_rgmii_selection:1;	/* E-MAC supports mii/rgmii

selection */

quoted

 	unsigned half_duplex:1;		/* E-MAC supports half duplex mode */
 	unsigned rx_2k_buffers:1;	/* AVB-DMAC has Max 2K buf size on RX

*/

quoted

+	unsigned timestamp:1;		/* AVB-DMAC has timestamp */

   Isn't this a matter of the gPTP support as well, i.e. no separate
flag needed?

Agreed. Previously it is suggested to use timestamp. I will change it to

as part of gPTP support cases.

   TIA. :-)

quoted

quoted

[...]

quoted

@@ -1089,6 +1090,7 @@ struct ravb_private {
 	unsigned int num_tx_desc;	/* TX descriptors per packet */

 	int duplex;
+	struct ravb_rx_desc *rgeth_rx_ring[NUM_RX_QUEUE];

   Strange place to declare this...

Agreed. This has to be on later patch. Will move it.

   I only meant that these fields should go together with rx_ring[].
Apparently we have a case of wrong patch ordering here (as this patch
needs this field declared)...

Exactly that is the case, it is moved to later patch (https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210923140813.13541-14-biju.das.jz@bp.renesas.com/) which is the first user of
This variable.

Regards,
Biju

quoted

quoted

quoted

 	const struct ravb_hw_info *info;
 	struct reset_control *rstc;

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

b/drivers/net/ethernet/renesas/ravb_main.c
index 9c0d35f4b221..2c375002ebcb 100644

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

@@ -949,11 +949,14 @@ 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);
+	const struct ravb_hw_info *info = priv->info;
 	u32 tis = ravb_read(ndev, TIS);

 	if (tis & TIS_TFUF) {
 		ravb_write(ndev, ~(TIS_TFUF | TIS_RESERVED), TIS);
-		ravb_get_tx_tstamp(ndev);
+		if (info->timestamp)
+			ravb_get_tx_tstamp(ndev);

   Shouldn't we just disable TIS.TFUF permanently instead for the
non-gPTP case?

Good catch. As ravb_dmac_init_rgeth(will be renamed to

"ravb_dmac_init_gbeth") is not enabling this interrupt as it is not
documented in RZ/G2L hardware manual.

quoted

So this function never gets called for non-gPTP case.

I will remove this check.

   TIA!

quoted

Regards,
Biju

MBR, Sergey