RE: [PATCH net-next 06/18] ravb: Factorise ptp feature

Hi Sergei,

Thanks for the feedback.

Subject: Re: [PATCH net-next 06/18] ravb: Factorise ptp feature

HEllo!

On 7/22/21 5:13 PM, Biju Das wrote:

quoted

Gptp is active in CONFIG mode for R-Car Gen3, where as it is not

   It's gPTP, the manuals say. :-)

Ok.

quoted

active in CONFIG mode for R-Car Gen2. Add feature bits to handle both
cases.

   I have no idea why this single diff requires 2 fetaure bits....

Basically this is a HW feature.

1) for R-Car Gen3, gPTP is active in config mode (R-Car Gen3)
2) for R-Car Gen2, gPTP is not active in config mode (R-Car Gen2)
3) RZ/G2L does not support ptp feature.

quoted

RZ/G2L does not support ptp feature.

   Ah, that explains it. :-)
   It doesn't explain why we should bother with the 2nd bit in the same
patch tho...

See above it is HW feature diff between R-Car Gen3 and R-Car Gen2.

quoted

Factorise ptp feature
specific to R-Car.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  1 +
 drivers/net/ethernet/renesas/ravb_main.c | 81
++++++++++++++++--------
 2 files changed, 56 insertions(+), 26 deletions(-)

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

b/drivers/net/ethernet/renesas/ravb.h
index 0ed21262f26b..a474ed68db22 100644

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

@@ -998,6 +998,7 @@ struct ravb_drv_data {
 	size_t skb_sz;
 	u8 num_tx_desc;
 	enum ravb_chip_id chip_id;
+	u32 features;

   You didn't like bitfelds (in sh_eth) so much? :-)

OK. I will change to bit fields, if there is no objection.

[...]

quoted

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

b/drivers/net/ethernet/renesas/ravb_main.c
index 84ebd6fef711..e966b76df32c 100644

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

@@ -40,6 +40,14 @@
 		 NETIF_MSG_RX_ERR | \
 		 NETIF_MSG_TX_ERR)

+#define RAVB_PTP_CONFIG_ACTIVE		BIT(0)
+#define RAVB_PTP_CONFIG_INACTIVE	BIT(1)

   If both bits are 0, it means GbEth?

Yes that is correct. ptp is not supported in GbEth.

quoted

+
+#define RAVB_PTP	(RAVB_PTP_CONFIG_ACTIVE |

RAVB_PTP_CONFIG_INACTIVE)

   Hm?

quoted

+
+#define RAVB_RCAR_GEN3_FEATURES	RAVB_PTP_CONFIG_ACTIVE
+#define RAVB_RCAR_GEN2_FEATURES	RAVB_PTP_CONFIG_INACTIVE

   Not sure whtehr these are necessary...

If there is no objection for using bit fields, then the above definitions not required.

[...]

quoted

 	}

 	/* gPTP interrupt status summary */
-	if (iss & ISS_CGIS) {
+	if ((info->features & RAVB_PTP) && (iss & ISS_CGIS)) {

   This is not a transparent change -- the fearture check came fromn
nownere...

I have added commit statement to make it clear, "RZ/G2L does not support ptp feature. Factorise ptp feature
specific to R-Car".

Do you see any issues with this? If needed we can factorize this portion of the code again to make 
It simpler. First patch is ptp feature for r-car differences(R-Car Gen3 and R-Car Gen2) and second patch( with "transparent" comments you have mentioned in this patch)

Please let me know.

quoted

 		ravb_ptp_interrupt(ndev);
 		result = IRQ_HANDLED;
 	}

[...]

quoted

@@ -1275,7 +1286,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 (data->features & RAVB_PTP)

   Again, not transparent...

See above.

quoted

+		info->phc_index = ptp_clock_index(priv->ptp.clock);

 	return 0;
 }

[...]

quoted

@@ -1992,14 +2009,20 @@ static int ravb_set_gti(struct net_device

*ndev)  static void ravb_set_config_mode(struct net_device *ndev)  {
 	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;

-	if (priv->chip_id == RCAR_GEN2) {
+	switch (info->features & RAVB_PTP) {
+	case RAVB_PTP_CONFIG_INACTIVE:
 		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
 		/* Set CSEL value */
 		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
-	} else {
+		break;
+	case RAVB_PTP_CONFIG_ACTIVE:
 		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
 			    CCC_GAC | CCC_CSEL_HPB);
+		break;
+	default:
+		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);

   Not trasparent again...

See above.

First Case is for R-Car Gen3 where ptp is active in config mode.
Second Case is for R-Car Gen2 where ptp is not active in config mode.
Third Case is default for RZ/G2L where ptp is not present.

Do you see any issues with this?

[...]

quoted

@@ -2182,13 +2205,15 @@ static int ravb_probe(struct platform_device

*pdev)

quoted

 	/* Set AVB config mode */
 	ravb_set_config_mode(ndev);

-	/* Set GTI value */
-	error = ravb_set_gti(ndev);
-	if (error)
-		goto out_disable_refclk;
+	if (info->features & RAVB_PTP) {

   Not transparent enough yet again...

See above.

quoted

+		/* Set GTI value */
+		error = ravb_set_gti(ndev);
+		if (error)
+			goto out_disable_refclk;

-	/* Request GTI loading */
-	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+		/* Request GTI loading */
+		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+	}

 	if (priv->chip_id != RCAR_GEN2) {
 		ravb_parse_delay_mode(np, ndev);

[...]

quoted

@@ -2377,13 +2404,15 @@ static int __maybe_unused ravb_resume(struct

device *dev)

quoted

 	/* Set AVB config mode */
 	ravb_set_config_mode(ndev);

-	/* Set GTI value */
-	ret = ravb_set_gti(ndev);
-	if (ret)
-		return ret;
+	if (info->features & RAVB_PTP) {

   Not transparent enough again...

See above.

Cheers,
Biju