Hi Sergei,

Subject: RE: [RFC/PATCH 05/18] ravb: Exclude gPTP feature support for
RZ/G2L

Hi Sergei,

quoted

Subject: Re: [RFC/PATCH 05/18] ravb: Exclude gPTP feature support for
RZ/G2L

On 9/23/21 10:13 PM, Biju Das wrote:

[...]

quoted

quoted

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

quoted

quoted

quoted

+	if (!info->no_gptp && (iss & ISS_CGIS)) {

   Then extending this check doesn't seem necessary?

I have dropped this check in new version.

quoted

quoted

quoted

quoted

 		ravb_ptp_interrupt(ndev);
 		result = IRQ_HANDLED;
 	}

[...]

quoted

quoted

quoted

@@ -2116,6 +2119,7 @@ static const struct ravb_hw_info

rgeth_hw_info =

{

quoted

quoted

quoted

 	.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.

quoted

Please correct me, if any one have different opinion.

   I'm afraid you'd get a partly functioning device with the RZ/G2
info introduced amidst of the series and then the necessary
flags/values added to it. This should definitely be avoided.

It is ok, It is understood, After replacing all  the place holders only we
get full functionality.
That is the reason place holders added in first patch, so that we can fill
each function at later stage By smaller patcher. Same case for feature
bits.

OK, the new patch excluded gPTP support for RZ/G2L and Also as per your suggestion,dropped timestamp feature bit and merged that code in this patch.

Regards,
Biju