Hi Sergei,

Thanks for the review.

-----Original Message-----
From: Sergey Shtylyov <redacted>
Sent: 23 September 2021 20:00
To: Biju Das <biju.das.jz@bp.renesas.com>; David S. Miller
[off-list ref]; Jakub Kicinski [off-list ref]
Cc: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>;
Andrew Lunn [off-list ref]; Sergei Shtylyov [off-list ref];
Geert Uytterhoeven [off-list ref]; Adam Ford
[off-list ref]; Yoshihiro Shimoda
[off-list ref]; netdev@vger.kernel.org; linux-renesas-
soc@vger.kernel.org; Chris Paterson [off-list ref]; Biju
Das [off-list ref]
Subject: Re: [RFC/PATCH 05/18] ravb: Exclude gPTP feature support for
RZ/G2L

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

quoted

R-Car supports gPTP feature whereas RZ/G2L does not support it.
This patch excludes gtp feature support for RZ/G2L by enabling no_gptp
feature bit.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 46
++++++++++++++----------
 1 file changed, 28 insertions(+), 18 deletions(-)

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

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

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

[...]

quoted

@@ -953,7 +954,7 @@ static irqreturn_t ravb_interrupt(int irq, void

*dev_id)

quoted

 	}

 	/* gPTP interrupt status summary */
-	if (iss & ISS_CGIS) {

   Isn't this bit always 0 on RZ/G2L?

This CGIM bit(BIT13) which is present on R-Car Gen3 is not present in RZ/G2L. As per the HW manual
BIT13 is reserved bit and read is always 0.

quoted

+	if (!info->no_gptp && (iss & ISS_CGIS)) {
 		ravb_ptp_interrupt(ndev);
 		result = IRQ_HANDLED;
 	}

@@ -1378,6 +1379,7 @@ static int ravb_get_ts_info(struct net_device

*ndev,

quoted

 			    struct ethtool_ts_info *info)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_hw_info *hw_info = priv->info;

 	info->so_timestamping =
 		SOF_TIMESTAMPING_TX_SOFTWARE |

@@ -1391,7 +1393,8 @@ static int ravb_get_ts_info(struct net_device

*ndev,

quoted

 		(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 (!hw_info->no_gptp)
+		info->phc_index = ptp_clock_index(priv->ptp.clock);

 	return 0;
 }

@@ -2116,6 +2119,7 @@ static const struct ravb_hw_info rgeth_hw_info = {
 	.emac_init = ravb_rgeth_emac_init,
 	.aligned_tx = 1,
 	.tx_counters = 1,
+	.no_gptp = 1,

   Mhm, I definitely don't like the way you "extend" the GbEthernet info
structure. All the applicable flags should be set in the last patch of the
series, not amidst of it.

According to me, It is clearer with smaller patches like, what we have done with previous 2 patch sets for factorisation.
Please correct me, if any one have different opinion.

Regards,
Biju

[...]

MBR, Sergey