[PATCH net-next v4 00/10] ravb: Add gPTP support for Gen4

[PATCH net-next v4 00/10] ravb: Add gPTP support for Gen4

[Niklas Söderlund]

Hello,

This series is the second part cleaning up how PTP timer support is
implemented on R-Car Gen4. Currently there is partial support for it in
some of the Ethernet devices that can use it, but not all.

On Gen2 and Gen3 each RAVB instance have it's own private PTP clock as
part of the RAVB register space. For this reason the PTP functionally
was implemented directly in the RAVB driver. For Gen4 however there is a
system-wide PTP clock shared by all RAVB instances, and on some Gen4
platforms with other Ethernet devices.

The RAVB Gen4 driver currently advertise PTP support to user-space, but
the support is in fact completely broken. It tries to use RAVB private
PTP clock, which do not exist on Gen4.

Further more the PTP clock behaved slightly different on Gen2 and Gen3.
These decencies have been handled by adding platform specific flags
inside the driver.

The usage of these flags have grown organically and been extended all
over the driver as it gained new features. Adding a new third set of
flags for Gen4 would be messy and add to the confusion.

Therefore patches 1/10 thru 8/10 refactors the usage of the PTP flags
into optional callbacks and untangles the usage. This allows adding Gen4
support easy as it can just implement it's own set of Gen4 specific
callbacks without altering the driver logic.

Patch 9/10 is a small DT binding addition adding an optional phandle to
link a RAVB device to the external PTP clock. Ideally this property
should be mandatory, but for backward comp ability is is made optional.
If the phandle is not set, or the PTP clock not enabled, the RAVB driver
will continue to faction as before, but no longer advertise PTP support
to user space.

Finally patch 10/10 adds the Gen4 specific PTP callbacks which allows
the driver to use the external PTP clock.

For part one of this work see [1]. The two series are independent of
each other but both are needed before a third series liking the RAVB to
the PTP clock in the platforms device tree. I will hold posting the
third series until all dependencies are merged. For this reason there is
no user of the new renesas,gptp device tree property added in this
series.

The work is tested on both Gen3 and Gen4 R-Car devices (with [1] and
appropriate DTS). I do however not have access to any Gen2 device where
the RAVB IP is wired to a MAC, so the small rework of the flags for Gen2
to callbacks have only been compiled tested. If anybody have a RZ device
where RAVB is wired and uses the Gen2 method of starting/stopping the
PTP timer together with the DMAC please test this.

1.  https://lore.kernel.org/all/20260803095232.3527111-1-niklas.soderlund+renesas@ragnatech.se/

Niklas Söderlund (10):
  net: ethernet: ravb: Remove gPTP control from WoL setup and restore
  net: ethernet: ravb: Move programming of gPTP timer interval
  net: ethernet: ravb: Simplify gPTP start and stop
  net: ethernet: ravb: Remove redundant argument to ravb_ptp_init()
  net: ethernet: ravb: Propagate error from ptp_clock_register()
  net: ethernet: ravb: Replace gPTP flags with callbacks
  net: ethernet: ravb: Add callback for gPTP probe
  net: ethernet: ravb: Add callback for gPTP clock index
  dt-bindings: net: renesas,etheravb: Add optional gPTP phandle for Gen4
  net: ethernet: ravb: Add gPTP support for Gen4

 .../bindings/net/renesas,etheravb.yaml        |  11 +
 drivers/net/ethernet/renesas/ravb.h           |  35 ++-
 drivers/net/ethernet/renesas/ravb_main.c      | 252 ++++++++++++------
 drivers/net/ethernet/renesas/ravb_ptp.c       |  38 +--
 4 files changed, 229 insertions(+), 107 deletions(-)

-- 
2.55.0

[PATCH net-next v4 01/10] net: ethernet: ravb: Remove gPTP control from WoL setup and restore

[Niklas Söderlund]

Since commit a6a85ba36fd0 ("net: ravb: Move PTP initialization in the
driver's ndo_open API for ccc_gac platorms") the gPTP clock (if
supported) is stopped and started by opening and closing the ndev.

This makes the special case to stop and start it when resuming from WoL
redundant. As the ndev will always be closed and re-opened when
suspending and resuming the system.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index ea1c7e536791..36173a09e2fd 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -3182,9 +3182,6 @@ static int ravb_wol_setup(struct net_device *ndev)
 	/* Enable MagicPacket */
 	ravb_modify(ndev, ECMR, ECMR_MPDE, ECMR_MPDE);
 
-	if (priv->info->ccc_gac)
-		ravb_ptp_stop(ndev);
-
 	return enable_irq_wake(priv->emac_irq);
 }
 
@@ -3204,9 +3201,6 @@ static int ravb_wol_restore(struct net_device *ndev)
 	if (error)
 		return error;
 
-	if (priv->info->ccc_gac)
-		ravb_ptp_init(ndev, priv->pdev);
-
 	if (info->nc_queues)
 		napi_enable(&priv->napi[RAVB_NC]);
 	napi_enable(&priv->napi[RAVB_BE]);
-- 
2.55.0

[PATCH net-next v4 02/10] net: ethernet: ravb: Move programming of gPTP timer interval

[Niklas Söderlund]

Commit f384ab481cab ("net: ravb: Split GTI computation and set
operations") broke apart the operations of computing the timer interval
and programming of it. However it kept the programming of the interval
in the RAVB main logic.

Having split the two apart this can be improved further by moving the
programming to the gPTP initialization function, as the first action of
the gPTP init function is to wait for the timer interval programming to
be acknowledge by the hardware.

As an added bonus the interaction with the gPTP registers for the
programming can then also be done while holding the gPTP registers lock.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 16 ----------------
 drivers/net/ethernet/renesas/ravb_ptp.c  | 11 ++++++++++-
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 36173a09e2fd..a89f6fe7866f 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1846,20 +1846,6 @@ static int ravb_set_config_mode(struct net_device *ndev)
 	return error;
 }
 
-static void ravb_set_gti(struct net_device *ndev)
-{
-	struct ravb_private *priv = netdev_priv(ndev);
-	const struct ravb_hw_info *info = priv->info;
-
-	if (!(info->gptp || info->ccc_gac))
-		return;
-
-	ravb_write(ndev, priv->gti_tiv, GTI);
-
-	/* Request GTI loading */
-	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
-}
-
 static int ravb_compute_gti(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -1974,8 +1960,6 @@ static int ravb_open(struct net_device *ndev)
 
 	ravb_emac_init(ndev);
 
-	ravb_set_gti(ndev);
-
 	/* Initialise PTP Clock driver */
 	if (info->gptp || info->ccc_gac)
 		ravb_ptp_init(ndev, priv->pdev);
diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
index 43218bc15b15..157fb78209c7 100644
--- a/drivers/net/ethernet/renesas/ravb_ptp.c
+++ b/drivers/net/ethernet/renesas/ravb_ptp.c
@@ -321,11 +321,20 @@ void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
 
 	priv->ptp.info = ravb_ptp_info;
 
-	priv->ptp.default_addend = ravb_read(ndev, GTI);
+	priv->ptp.default_addend = priv->gti_tiv;
 	priv->ptp.current_addend = priv->ptp.default_addend;
 
 	spin_lock_irqsave(&priv->lock, flags);
+
+	/* Set gPTP Timer Increment Value. */
+	ravb_write(ndev, priv->ptp.default_addend, GTI);
+
+	/* Request GTI loading. */
+	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+
+	/* Wait for GIT loading to complete. */
 	ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ);
+
 	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-- 
2.55.0

[PATCH net-next v4 03/10] net: ethernet: ravb: Simplify gPTP start and stop

[Niklas Söderlund]

For devices that do not support the gPTP clock in config mode the
somewhat oddly named flag gptp is set, compared to devices that do
support the gPTP clock in config and operation mode where the flag
ccc_gac is set instead. The two flags are mutually exclusive.

For the gptp-flag devices (Gen2) the clock is tied to the AVB-DMAC, when
it is stopped so is the gPTP clock. For ccc_gac-flag devices (Gen3) the
gPTP clock is available whenever the ndev is open.

Prepare to add Gen4 support which will add a third way by cleaning the
Gen2 and Gen3 cases up a bit.

Fold the gptp-flag start and stop calls into ravb_dmac_init() and
ravb_stop_dma(), which start and stop the AVB-DMAC. There are no
functional change as all call sites to the construct:

    if (info->gptp)
        ravb_ptp_init(ndev, priv->pdev);

are always just after a call to into ravb_dmac_init() and all call sites
to the construct:

    if (info->gptp)
        ravb_ptp_stop(ndev);

are always directly followed by a call to ravb_stop_dma().

There are two special cases where the calling construct covers both the
gptp-flag and info->ccc_gac devices, one for start and one for stop. The
condition that it is preceded by a call to ravb_dmac_init(), or followed
by a call to ravb_stop_dma() are however true for them too. Reworked the
two special cases to drop the check of info->gptp.

The end result is that the gPTP clock will be started or stopped for the
gptp-flag devices in tandem with the AVB-DMAC, while the info->ccc_gac
devices will be controlled, as before, when the ndev is opened or
closed.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
* Changes since v1
- Improve spelling in commit message.
---
 drivers/net/ethernet/renesas/ravb_main.c | 37 ++++++++++--------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index a89f6fe7866f..2d0022a4f85c 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -707,7 +707,15 @@ static int ravb_dmac_init(struct net_device *ndev)
 		return error;
 
 	/* Setting the control will start the AVB-DMAC process. */
-	return ravb_set_opmode(ndev, CCC_OPC_OPERATION);
+	error = ravb_set_opmode(ndev, CCC_OPC_OPERATION);
+	if (error)
+		return error;
+
+	/* Initialise PTP Clock driver */
+	if (info->gptp)
+		ravb_ptp_init(ndev, priv->pdev);
+
+	return 0;
 }
 
 static void ravb_get_tx_tstamp(struct net_device *ndev)
@@ -1115,6 +1123,10 @@ static int ravb_stop_dma(struct net_device *ndev)
 			netdev_err(ndev, "failed to stop AXI BUS\n");
 	}
 
+	/* Stop PTP Clock driver */
+	if (info->gptp)
+		ravb_ptp_stop(ndev);
+
 	/* Stop AVB-DMAC process */
 	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
 }
@@ -1719,9 +1731,7 @@ static int ravb_set_ringparam(struct net_device *ndev,
 
 	if (netif_running(ndev)) {
 		netif_device_detach(ndev);
-		/* Stop PTP Clock driver */
-		if (info->gptp)
-			ravb_ptp_stop(ndev);
+
 		/* Wait for DMA stopping */
 		error = ravb_stop_dma(ndev);
 		if (error) {
@@ -1752,10 +1762,6 @@ static int ravb_set_ringparam(struct net_device *ndev,
 
 		ravb_emac_init(ndev);
 
-		/* Initialise PTP Clock driver */
-		if (info->gptp)
-			ravb_ptp_init(ndev, priv->pdev);
-
 		netif_device_attach(ndev);
 	}
 
@@ -1961,7 +1967,7 @@ static int ravb_open(struct net_device *ndev)
 	ravb_emac_init(ndev);
 
 	/* Initialise PTP Clock driver */
-	if (info->gptp || info->ccc_gac)
+	if (info->ccc_gac)
 		ravb_ptp_init(ndev, priv->pdev);
 
 	/* PHY control start */
@@ -1974,9 +1980,6 @@ static int ravb_open(struct net_device *ndev)
 	return 0;
 
 out_ptp_stop:
-	/* Stop PTP Clock driver */
-	if (info->gptp || info->ccc_gac)
-		ravb_ptp_stop(ndev);
 	ravb_stop_dma(ndev);
 out_set_reset:
 	ravb_set_opmode(ndev, CCC_OPC_RESET);
@@ -2020,10 +2023,6 @@ static void ravb_tx_timeout_work(struct work_struct *work)
 
 	netif_tx_stop_all_queues(ndev);
 
-	/* Stop PTP Clock driver */
-	if (info->gptp)
-		ravb_ptp_stop(ndev);
-
 	/* Wait for DMA stopping */
 	if (ravb_stop_dma(ndev)) {
 		/* If ravb_stop_dma() fails, the hardware is still operating
@@ -2056,10 +2055,6 @@ static void ravb_tx_timeout_work(struct work_struct *work)
 	ravb_emac_init(ndev);
 
 out:
-	/* Initialise PTP Clock driver */
-	if (info->gptp)
-		ravb_ptp_init(ndev, priv->pdev);
-
 	netif_tx_start_all_queues(ndev);
 
 out_unlock:
@@ -2374,7 +2369,7 @@ static int ravb_close(struct net_device *ndev)
 	}
 
 	/* Stop PTP Clock driver */
-	if (info->gptp || info->ccc_gac)
+	if (info->ccc_gac)
 		ravb_ptp_stop(ndev);
 
 	/* Set the config mode to stop the AVB-DMAC's processes */
-- 
2.55.0

[PATCH net-next v4 04/10] net: ethernet: ravb: Remove redundant argument to ravb_ptp_init()

[Niklas Söderlund]

There is no need to explicitly pass the struct platform_device pointer
to ravb_ptp_init(), it can retrieve it directly from the private data
structure.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
 drivers/net/ethernet/renesas/ravb.h      | 2 +-
 drivers/net/ethernet/renesas/ravb_main.c | 4 ++--
 drivers/net/ethernet/renesas/ravb_ptp.c  | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 3ee4c6108189..aa45f5466001 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1163,7 +1163,7 @@ void ravb_modify(struct net_device *ndev, enum ravb_reg reg, u32 clear,
 int ravb_wait(struct net_device *ndev, enum ravb_reg reg, u32 mask, u32 value);
 
 void ravb_ptp_interrupt(struct net_device *ndev);
-void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev);
+void ravb_ptp_init(struct net_device *ndev);
 void ravb_ptp_stop(struct net_device *ndev);
 
 #endif	/* #ifndef __RAVB_H__ */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 2d0022a4f85c..e44d065f77cc 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -713,7 +713,7 @@ static int ravb_dmac_init(struct net_device *ndev)
 
 	/* Initialise PTP Clock driver */
 	if (info->gptp)
-		ravb_ptp_init(ndev, priv->pdev);
+		ravb_ptp_init(ndev);
 
 	return 0;
 }
@@ -1968,7 +1968,7 @@ static int ravb_open(struct net_device *ndev)
 
 	/* Initialise PTP Clock driver */
 	if (info->ccc_gac)
-		ravb_ptp_init(ndev, priv->pdev);
+		ravb_ptp_init(ndev);
 
 	/* PHY control start */
 	error = ravb_phy_start(ndev);
diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
index 157fb78209c7..f70b616ac847 100644
--- a/drivers/net/ethernet/renesas/ravb_ptp.c
+++ b/drivers/net/ethernet/renesas/ravb_ptp.c
@@ -313,7 +313,7 @@ void ravb_ptp_interrupt(struct net_device *ndev)
 	ravb_write(ndev, ~(gis | GIS_RESERVED), GIS);
 }
 
-void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
+void ravb_ptp_init(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	struct ptp_clock *clock;
@@ -338,7 +338,7 @@ void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
 	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-	clock = ptp_clock_register(&priv->ptp.info, &pdev->dev);
+	clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
 	if (IS_ERR(clock)) {
 		netdev_err(ndev, "failed to register PTP clock: %pe\n", clock);
 		clock = NULL;
-- 
2.55.0

[PATCH net-next v4 05/10] net: ethernet: ravb: Propagate error from ptp_clock_register()

[Niklas Söderlund]

If the PTP clock registration failed the error is silently ignored.
Before reworking all callers of ravb_ptp_init() handle and propagate the
error so it can be used.

Link: https://sashiko.dev/#/patchset/20260610102432.3538432-2-niklas.soderlund+renesas@ragnatech.se?part=1
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
* Changes since v3
- Add check to keep sashiko happy, the remark is fixed already later in
  the series but to keep the bot happy.

* Changes since v2
- Rework to remove the caching of phc_index added by LLM without access
  to hardware.

* Changes since v1
- New in v2.
---
 drivers/net/ethernet/renesas/ravb.h      |  3 +--
 drivers/net/ethernet/renesas/ravb_main.c |  5 ++---
 drivers/net/ethernet/renesas/ravb_ptp.c  | 27 +++++++++++-------------
 3 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index aa45f5466001..0c122a815840 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1028,7 +1028,6 @@ struct ravb_ptp_perout {
 struct ravb_ptp {
 	struct ptp_clock *clock;
 	struct ptp_clock_info info;
-	int phc_index;
 	u32 default_addend;
 	u32 current_addend;
 	int extts[N_EXT_TS];
@@ -1163,7 +1162,7 @@ void ravb_modify(struct net_device *ndev, enum ravb_reg reg, u32 clear,
 int ravb_wait(struct net_device *ndev, enum ravb_reg reg, u32 mask, u32 value);
 
 void ravb_ptp_interrupt(struct net_device *ndev);
-void ravb_ptp_init(struct net_device *ndev);
+int ravb_ptp_init(struct net_device *ndev);
 void ravb_ptp_stop(struct net_device *ndev);
 
 #endif	/* #ifndef __RAVB_H__ */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index e44d065f77cc..5ba322593a01 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1774,7 +1774,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *hw_info = priv->info;
 
-	if (hw_info->gptp || hw_info->ccc_gac) {
+	if (priv->ptp.clock && (hw_info->gptp || hw_info->ccc_gac)) {
 		info->so_timestamping =
 			SOF_TIMESTAMPING_TX_SOFTWARE |
 			SOF_TIMESTAMPING_TX_HARDWARE |
@@ -1785,7 +1785,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
 			(1 << HWTSTAMP_FILTER_NONE) |
 			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
 			(1 << HWTSTAMP_FILTER_ALL);
-		info->phc_index = READ_ONCE(priv->ptp.phc_index);
+		info->phc_index = ptp_clock_index(priv->ptp.clock);
 	}
 
 	return 0;
@@ -2934,7 +2934,6 @@ static int ravb_probe(struct platform_device *pdev)
 	priv->rstc = rstc;
 	priv->ndev = ndev;
 	priv->pdev = pdev;
-	priv->ptp.phc_index = -1;
 	priv->num_tx_ring[RAVB_BE] = BE_TX_RING_SIZE;
 	priv->num_rx_ring[RAVB_BE] = BE_RX_RING_SIZE;
 	if (info->nc_queues) {
diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
index f70b616ac847..37161c0579ce 100644
--- a/drivers/net/ethernet/renesas/ravb_ptp.c
+++ b/drivers/net/ethernet/renesas/ravb_ptp.c
@@ -313,11 +313,11 @@ void ravb_ptp_interrupt(struct net_device *ndev)
 	ravb_write(ndev, ~(gis | GIS_RESERVED), GIS);
 }
 
-void ravb_ptp_init(struct net_device *ndev)
+int ravb_ptp_init(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	struct ptp_clock *clock;
 	unsigned long flags;
+	int ret = 0;
 
 	priv->ptp.info = ravb_ptp_info;
 
@@ -338,15 +338,14 @@ void ravb_ptp_init(struct net_device *ndev)
 	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-	clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
-	if (IS_ERR(clock)) {
-		netdev_err(ndev, "failed to register PTP clock: %pe\n", clock);
-		clock = NULL;
+	priv->ptp.clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
+	if (IS_ERR(priv->ptp.clock)) {
+		ret = PTR_ERR(priv->ptp.clock);
+		priv->ptp.clock = NULL;
+		ravb_ptp_stop(ndev);
 	}
 
-	WRITE_ONCE(priv->ptp.clock, clock);
-	if (clock)
-		WRITE_ONCE(priv->ptp.phc_index, ptp_clock_index(clock));
+	return ret;
 }
 
 static void ravb_ptp_disable(struct net_device *ndev)
@@ -369,14 +368,12 @@ static void ravb_ptp_sync_irqs(struct net_device *ndev)
 void ravb_ptp_stop(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	struct ptp_clock *clock;
-
-	WRITE_ONCE(priv->ptp.phc_index, -1);
-	clock = xchg(&priv->ptp.clock, NULL);
 
 	ravb_ptp_disable(ndev);
 	ravb_ptp_sync_irqs(ndev);
 
-	if (clock)
-		ptp_clock_unregister(clock);
+	if (priv->ptp.clock) {
+		ptp_clock_unregister(priv->ptp.clock);
+		priv->ptp.clock = NULL;
+	}
 }
-- 
2.55.0

[PATCH net-next v4 06/10] net: ethernet: ravb: Replace gPTP flags with callbacks

[Niklas Söderlund]

Prepare for adding Gen4 support which will add a third and new way to
interact with the gPTP clock by replacing the flags for Gen2 behavior
(info->gptp) and Gen3 behavior (info->ccc_gac) with callbacks.

This will make adding Gen4 support cleaner as the code will not have "if
else if else" sprinkled all over to handle each generations special
cases.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
* Changes since v1
- Handle error from starting the PTP clock.
---
 drivers/net/ethernet/renesas/ravb.h      | 24 ++++++-
 drivers/net/ethernet/renesas/ravb_main.c | 83 +++++++++++++++---------
 2 files changed, 76 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 0c122a815840..6603f90b19e1 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1034,6 +1034,27 @@ struct ravb_ptp {
 	struct ravb_ptp_perout perout[N_PER_OUT];
 };
 
+/**
+ * struct ravb_gptp_info - Platform specific gPTP behavior
+ *
+ * Each generation of RAVB have slightly different behaviors when interacting
+ * with the gPTP clock. This struct provides the callbacks to be called at
+ * critical points in the RAVB driver.
+ *
+ * @set_config_mode:	Enter config mode
+ * @dmac_start:		Called when the DMAC starts
+ * @dmac_stop:		Called when the DMAC stops
+ * @ndev_open:		Called when the ndev is opened
+ * @ndev_close:		Called when the ndev is closed
+ */
+struct ravb_gptp_info {
+	int (*set_config_mode)(struct net_device *ndev);
+	int (*dmac_start)(struct net_device *ndev);
+	void (*dmac_stop)(struct net_device *ndev);
+	int (*ndev_open)(struct net_device *ndev);
+	void (*ndev_close)(struct net_device *ndev);
+};
+
 struct ravb_hw_info {
 	int (*receive)(struct net_device *ndev, int budget, int q);
 	void (*set_rate)(struct net_device *ndev);
@@ -1052,6 +1073,7 @@ struct ravb_hw_info {
 	u32 rx_buffer_size;
 	u32 rx_desc_size;
 	u32 dbat_entry_num;
+	const struct ravb_gptp_info *ptp; /* Callbacks to handle gPTP interactions. */
 	unsigned aligned_tx: 1;
 	unsigned coalesce_irqs:1;	/* Needs software IRQ coalescing */
 
@@ -1062,8 +1084,6 @@ struct ravb_hw_info {
 	unsigned multi_irqs:1;		/* AVB-DMAC and E-MAC has multiple irqs */
 	unsigned irq_en_dis:1;		/* Has separate irq enable and disable regs */
 	unsigned err_mgmt_irqs:1;	/* Line1 (Err) and Line2 (Mgmt) irqs are separate */
-	unsigned gptp:1;		/* AVB-DMAC has gPTP support */
-	unsigned ccc_gac:1;		/* AVB-DMAC has gPTP support active in config mode */
 	unsigned gptp_ref_clk:1;	/* gPTP has separate reference clock */
 	unsigned nc_queues:1;		/* AVB-DMAC has RX and TX NC queues */
 	unsigned magic_pkt:1;		/* E-MAC supports magic packet detection */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 5ba322593a01..9e9b1c4fbe95 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -712,8 +712,8 @@ static int ravb_dmac_init(struct net_device *ndev)
 		return error;
 
 	/* Initialise PTP Clock driver */
-	if (info->gptp)
-		ravb_ptp_init(ndev);
+	if (info->ptp && info->ptp->dmac_start)
+		return info->ptp->dmac_start(ndev);
 
 	return 0;
 }
@@ -1124,8 +1124,8 @@ static int ravb_stop_dma(struct net_device *ndev)
 	}
 
 	/* Stop PTP Clock driver */
-	if (info->gptp)
-		ravb_ptp_stop(ndev);
+	if (info->ptp && info->ptp->dmac_stop)
+		info->ptp->dmac_stop(ndev);
 
 	/* Stop AVB-DMAC process */
 	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
@@ -1774,7 +1774,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *hw_info = priv->info;
 
-	if (priv->ptp.clock && (hw_info->gptp || hw_info->ccc_gac)) {
+	if (priv->ptp.clock && hw_info->ptp) {
 		info->so_timestamping =
 			SOF_TIMESTAMPING_TX_SOFTWARE |
 			SOF_TIMESTAMPING_TX_HARDWARE |
@@ -1835,21 +1835,11 @@ static int ravb_set_config_mode(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
-	int error;
 
-	if (info->gptp) {
-		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG);
-		if (error)
-			return error;
-		/* Set CSEL value */
-		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
-	} else if (info->ccc_gac) {
-		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
-	} else {
-		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG);
-	}
+	if (info->ptp && info->ptp->set_config_mode)
+		return info->ptp->set_config_mode(ndev);
 
-	return error;
+	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
 }
 
 static int ravb_compute_gti(struct net_device *ndev)
@@ -1860,7 +1850,7 @@ static int ravb_compute_gti(struct net_device *ndev)
 	unsigned long rate;
 	u64 inc;
 
-	if (!(info->gptp || info->ccc_gac))
+	if (!info->ptp)
 		return 0;
 
 	if (info->gptp_ref_clk)
@@ -1967,8 +1957,11 @@ static int ravb_open(struct net_device *ndev)
 	ravb_emac_init(ndev);
 
 	/* Initialise PTP Clock driver */
-	if (info->ccc_gac)
-		ravb_ptp_init(ndev);
+	if (info->ptp && info->ptp->ndev_open) {
+		error = info->ptp->ndev_open(ndev);
+		if (error)
+			goto out_ptp_stop;
+	}
 
 	/* PHY control start */
 	error = ravb_phy_start(ndev);
@@ -2187,7 +2180,7 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	desc->dptr = cpu_to_le32(dma_addr);
 
 	/* TX timestamp required */
-	if (info->gptp || info->ccc_gac) {
+	if (info->ptp) {
 		if (q == RAVB_NC) {
 			ts_skb = kmalloc_obj(*ts_skb, GFP_ATOMIC);
 			if (!ts_skb) {
@@ -2369,8 +2362,8 @@ static int ravb_close(struct net_device *ndev)
 	}
 
 	/* Stop PTP Clock driver */
-	if (info->ccc_gac)
-		ravb_ptp_stop(ndev);
+	if (info->ptp && info->ptp->ndev_close)
+		info->ptp->ndev_close(ndev);
 
 	/* Set the config mode to stop the AVB-DMAC's processes */
 	if (ravb_stop_dma(ndev) < 0)
@@ -2378,7 +2371,7 @@ static int ravb_close(struct net_device *ndev)
 			   "device will be stopped after h/w processes are done.\n");
 
 	/* Clear the timestamp list */
-	if (info->gptp || info->ccc_gac) {
+	if (info->ptp) {
 		list_for_each_entry_safe(ts_skb, ts_skb2, &priv->ts_skb_list, list) {
 			list_del(&ts_skb->list);
 			kfree_skb(ts_skb->skb);
@@ -2660,6 +2653,26 @@ static int ravb_mdio_release(struct ravb_private *priv)
 	return 0;
 }
 
+static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
+{
+	int ret;
+
+	ret = ravb_set_opmode(ndev, CCC_OPC_CONFIG);
+	if (ret)
+		return ret;
+
+	/* gPTP Clock Select High-speed peripheral bus clock. */
+	ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
+
+	return 0;
+}
+
+static const struct ravb_gptp_info ravb_gen2_ptp_info = {
+	.set_config_mode = ravb_gen2_ptp_set_config_mode,
+	.dmac_start = ravb_ptp_init,
+	.dmac_stop = ravb_ptp_stop,
+};
+
 static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
@@ -2678,12 +2691,24 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
 			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
 	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
 	.dbat_entry_num = 22,
+	.ptp = &ravb_gen2_ptp_info,
 	.aligned_tx = 1,
-	.gptp = 1,
 	.nc_queues = 1,
 	.magic_pkt = 1,
 };
 
+static int ravb_gen3_ptp_set_config_mode(struct net_device *ndev)
+{
+	/* Enable gPTP Clock and Select High-speed peripheral bus clock. */
+	return ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
+}
+
+static const struct ravb_gptp_info ravb_gen3_ptp_info = {
+	.set_config_mode = ravb_gen3_ptp_set_config_mode,
+	.ndev_open = ravb_ptp_init,
+	.ndev_close = ravb_ptp_stop,
+};
+
 static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
@@ -2702,11 +2727,11 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
 	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
 	.dbat_entry_num = 22,
+	.ptp = &ravb_gen3_ptp_info,
 	.internal_delay = 1,
 	.tx_counters = 1,
 	.multi_irqs = 1,
 	.irq_en_dis = 1,
-	.ccc_gac = 1,
 	.nc_queues = 1,
 	.magic_pkt = 1,
 };
@@ -2733,7 +2758,7 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
 	.tx_counters = 1,
 	.multi_irqs = 1,
 	.irq_en_dis = 1,
-	.ccc_gac = 1,
+	.ptp = &ravb_gen3_ptp_info,
 	.nc_queues = 1,
 	.magic_pkt = 1,
 };
@@ -2758,7 +2783,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
 	.dbat_entry_num = 22,
 	.multi_irqs = 1,
 	.err_mgmt_irqs = 1,
-	.gptp = 1,
+	.ptp = &ravb_gen2_ptp_info,
 	.gptp_ref_clk = 1,
 	.nc_queues = 1,
 	.magic_pkt = 1,
-- 
2.55.0

[PATCH net-next v4 07/10] net: ethernet: ravb: Add callback for gPTP probe

[Niklas Söderlund]

Different generations of the RAVB IP have different needs when it probes
the gPTP timer clock. Add a callback in the PTP information to allow
each generation to probe its own way.

With this the last gPTP specific flag (gptp_ref_clk) can be removed.
However the primary motivation for the change is to prepare for Gen4
support, which compared to other generations with gPTP support does not
have the clock as part of the IP itself.

Gen4 will not need to compute GTI value as it have nowhere to write it,
as the gPTP clock is external. For this reason move the computation of
it into the newly gPTP probe specific callbacks for the RAVB IP's that
support it.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
* Changes since v1
- Improve spelling in commit message.
---
 drivers/net/ethernet/renesas/ravb.h      |  3 +-
 drivers/net/ethernet/renesas/ravb_main.c | 53 +++++++++++++++---------
 2 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 6603f90b19e1..b788e8f5260d 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1041,6 +1041,7 @@ struct ravb_ptp {
  * with the gPTP clock. This struct provides the callbacks to be called at
  * critical points in the RAVB driver.
  *
+ * @probe:		Probe the gPTP clock
  * @set_config_mode:	Enter config mode
  * @dmac_start:		Called when the DMAC starts
  * @dmac_stop:		Called when the DMAC stops
@@ -1048,6 +1049,7 @@ struct ravb_ptp {
  * @ndev_close:		Called when the ndev is closed
  */
 struct ravb_gptp_info {
+	int (*probe)(struct net_device *ndev);
 	int (*set_config_mode)(struct net_device *ndev);
 	int (*dmac_start)(struct net_device *ndev);
 	void (*dmac_stop)(struct net_device *ndev);
@@ -1084,7 +1086,6 @@ struct ravb_hw_info {
 	unsigned multi_irqs:1;		/* AVB-DMAC and E-MAC has multiple irqs */
 	unsigned irq_en_dis:1;		/* Has separate irq enable and disable regs */
 	unsigned err_mgmt_irqs:1;	/* Line1 (Err) and Line2 (Mgmt) irqs are separate */
-	unsigned gptp_ref_clk:1;	/* gPTP has separate reference clock */
 	unsigned nc_queues:1;		/* AVB-DMAC has RX and TX NC queues */
 	unsigned magic_pkt:1;		/* E-MAC supports magic packet detection */
 	unsigned half_duplex:1;		/* E-MAC supports half duplex mode */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 9e9b1c4fbe95..1092e8ada188 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1842,21 +1842,14 @@ static int ravb_set_config_mode(struct net_device *ndev)
 	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
 }
 
-static int ravb_compute_gti(struct net_device *ndev)
+static int ravb_compute_gti(struct net_device *ndev, struct clk *clk)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	const struct ravb_hw_info *info = priv->info;
 	struct device *dev = ndev->dev.parent;
 	unsigned long rate;
 	u64 inc;
 
-	if (!info->ptp)
-		return 0;
-
-	if (info->gptp_ref_clk)
-		rate = clk_get_rate(priv->gptp_clk);
-	else
-		rate = clk_get_rate(priv->clk);
+	rate = clk_get_rate(clk);
 	if (!rate)
 		return -EINVAL;
 
@@ -2653,6 +2646,13 @@ static int ravb_mdio_release(struct ravb_private *priv)
 	return 0;
 }
 
+static int ravb_gen2_ptp_probe(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	return ravb_compute_gti(ndev, priv->clk);
+}
+
 static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
 {
 	int ret;
@@ -2668,6 +2668,7 @@ static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
 }
 
 static const struct ravb_gptp_info ravb_gen2_ptp_info = {
+	.probe = ravb_gen2_ptp_probe,
 	.set_config_mode = ravb_gen2_ptp_set_config_mode,
 	.dmac_start = ravb_ptp_init,
 	.dmac_stop = ravb_ptp_stop,
@@ -2704,6 +2705,7 @@ static int ravb_gen3_ptp_set_config_mode(struct net_device *ndev)
 }
 
 static const struct ravb_gptp_info ravb_gen3_ptp_info = {
+	.probe = ravb_gen2_ptp_probe,
 	.set_config_mode = ravb_gen3_ptp_set_config_mode,
 	.ndev_open = ravb_ptp_init,
 	.ndev_close = ravb_ptp_stop,
@@ -2763,6 +2765,24 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
 	.magic_pkt = 1,
 };
 
+static int ravb_rzv2m_ptp_probe(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	priv->gptp_clk = devm_clk_get(&priv->pdev->dev, "gptp");
+	if (IS_ERR(priv->gptp_clk))
+		return PTR_ERR(priv->gptp_clk);
+
+	return ravb_compute_gti(ndev, priv->gptp_clk);
+}
+
+static const struct ravb_gptp_info ravb_rzv2m_ptp_info = {
+	.probe = ravb_rzv2m_ptp_probe,
+	.set_config_mode = ravb_gen2_ptp_set_config_mode,
+	.dmac_start = ravb_ptp_init,
+	.dmac_stop = ravb_ptp_stop,
+};
+
 static const struct ravb_hw_info ravb_rzv2m_hw_info = {
 	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
@@ -2783,8 +2803,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
 	.dbat_entry_num = 22,
 	.multi_irqs = 1,
 	.err_mgmt_irqs = 1,
-	.ptp = &ravb_gen2_ptp_info,
-	.gptp_ref_clk = 1,
+	.ptp = &ravb_rzv2m_ptp_info,
 	.nc_queues = 1,
 	.magic_pkt = 1,
 };
@@ -2976,12 +2995,10 @@ static int ravb_probe(struct platform_device *pdev)
 		goto out_reset_assert;
 	}
 
-	if (info->gptp_ref_clk) {
-		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
-		if (IS_ERR(priv->gptp_clk)) {
-			error = PTR_ERR(priv->gptp_clk);
+	if (info->ptp && info->ptp->probe) {
+		error = info->ptp->probe(ndev);
+		if (error)
 			goto out_reset_assert;
-		}
 	}
 
 	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
@@ -3034,10 +3051,6 @@ static int ravb_probe(struct platform_device *pdev)
 	ndev->netdev_ops = &ravb_netdev_ops;
 	ndev->ethtool_ops = &ravb_ethtool_ops;
 
-	error = ravb_compute_gti(ndev);
-	if (error)
-		goto out_rpm_put;
-
 	ravb_parse_delay_mode(np, ndev);
 
 	/* Allocate descriptor base address table */
-- 
2.55.0

[PATCH net-next v4 08/10] net: ethernet: ravb: Add callback for gPTP clock index

[Niklas Söderlund]

Prepare for adding Gen4 support which have an optional external gPTP
clock. Add a callback to get the clock index and use it to determine if
the device shall report gPTP support.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
* Changes since v3
- Add check to feed sashiko.
---
 drivers/net/ethernet/renesas/ravb.h      |  2 ++
 drivers/net/ethernet/renesas/ravb_main.c | 22 ++++++++++++++++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index b788e8f5260d..27e2b31955ba 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1042,6 +1042,7 @@ struct ravb_ptp {
  * critical points in the RAVB driver.
  *
  * @probe:		Probe the gPTP clock
+ * @clock_index:	Get the PTP clock index, if any
  * @set_config_mode:	Enter config mode
  * @dmac_start:		Called when the DMAC starts
  * @dmac_stop:		Called when the DMAC stops
@@ -1050,6 +1051,7 @@ struct ravb_ptp {
  */
 struct ravb_gptp_info {
 	int (*probe)(struct net_device *ndev);
+	int (*clock_index)(struct net_device *ndev);
 	int (*set_config_mode)(struct net_device *ndev);
 	int (*dmac_start)(struct net_device *ndev);
 	void (*dmac_stop)(struct net_device *ndev);
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 1092e8ada188..8617c30dbb14 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1773,8 +1773,13 @@ static int ravb_get_ts_info(struct net_device *ndev,
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *hw_info = priv->info;
+	int index = -1;
 
-	if (priv->ptp.clock && hw_info->ptp) {
+	if (hw_info->ptp && hw_info->ptp->clock_index)
+		index = hw_info->ptp->clock_index(ndev);
+
+	/* Only advertise ptp clock if present. */
+	if (index >= 0) {
 		info->so_timestamping =
 			SOF_TIMESTAMPING_TX_SOFTWARE |
 			SOF_TIMESTAMPING_TX_HARDWARE |
@@ -1785,7 +1790,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
 			(1 << HWTSTAMP_FILTER_NONE) |
 			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
 			(1 << HWTSTAMP_FILTER_ALL);
-		info->phc_index = ptp_clock_index(priv->ptp.clock);
+		info->phc_index = index;
 	}
 
 	return 0;
@@ -2653,6 +2658,16 @@ static int ravb_gen2_ptp_probe(struct net_device *ndev)
 	return ravb_compute_gti(ndev, priv->clk);
 }
 
+static int ravb_gen2_ptp_clock_index(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	if (!priv->ptp.clock)
+		return -ENODEV;
+
+	return ptp_clock_index(priv->ptp.clock);
+}
+
 static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
 {
 	int ret;
@@ -2669,6 +2684,7 @@ static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
 
 static const struct ravb_gptp_info ravb_gen2_ptp_info = {
 	.probe = ravb_gen2_ptp_probe,
+	.clock_index = ravb_gen2_ptp_clock_index,
 	.set_config_mode = ravb_gen2_ptp_set_config_mode,
 	.dmac_start = ravb_ptp_init,
 	.dmac_stop = ravb_ptp_stop,
@@ -2706,6 +2722,7 @@ static int ravb_gen3_ptp_set_config_mode(struct net_device *ndev)
 
 static const struct ravb_gptp_info ravb_gen3_ptp_info = {
 	.probe = ravb_gen2_ptp_probe,
+	.clock_index = ravb_gen2_ptp_clock_index,
 	.set_config_mode = ravb_gen3_ptp_set_config_mode,
 	.ndev_open = ravb_ptp_init,
 	.ndev_close = ravb_ptp_stop,
@@ -2778,6 +2795,7 @@ static int ravb_rzv2m_ptp_probe(struct net_device *ndev)
 
 static const struct ravb_gptp_info ravb_rzv2m_ptp_info = {
 	.probe = ravb_rzv2m_ptp_probe,
+	.clock_index = ravb_gen2_ptp_clock_index,
 	.set_config_mode = ravb_gen2_ptp_set_config_mode,
 	.dmac_start = ravb_ptp_init,
 	.dmac_stop = ravb_ptp_stop,
-- 
2.55.0

[PATCH net-next v4 09/10] dt-bindings: net: renesas,etheravb: Add optional gPTP phandle for Gen4

[Niklas Söderlund]

The RAVB module on Gen4 have no gPTP clock as part of the RAVB module
itself, instead it relies on an external system wide gPTP clock. The
gPTP clock is shared with RTSN on V4H and RSWITCH on S4.

Add an optional phandle so that the RAVB driver can find and use the
gPTP clock. Ideally this should have been a mandatory property but for
backward compatible it is optional. The RAVB module is capable of
functioning without it, but can in such cases not provided PTP
functionality.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
* Changes since v1
- Use generic ptp-timer property instead of vendor specific one.
- Improve spelling in commit message.
---
 .../devicetree/bindings/net/renesas,etheravb.yaml     | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
index 1e00ef5b3acd..fad002492421 100644
--- a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
+++ b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
@@ -97,6 +97,8 @@ properties:
 
   phy-handle: true
 
+  ptp-timer: true
+
   '#address-cells':
     description: Number of address cells for the MDIO bus.
     const: 1
@@ -305,6 +307,15 @@ allOf:
             items:
               - const: fck
               - const: refclk
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              const: renesas,etheravb-rcar-gen4
+    then:
+      properties:
+        ptp-timer: false
 
 additionalProperties: false
 
-- 
2.55.0

[PATCH net-next v4 10/10] net: ethernet: ravb: Add gPTP support for Gen4

[Niklas Söderlund]

While driver advertises gPTP support on Gen4 platforms it is in fact
completely broken. On R-Car Gen4 devices the RAVB module have no
internal gPTP clock as generations before it. Instead it utilizes a
system-wide gPTP clock.

This change utilizes the refactoring of the RAVB gPTP code to add
support for a system-wide clock and stops the Gen4 devices trying to use
the non-existing internal gPTP clock.

To remain backward compatible the ptp-timer property is optional.If not
present, or not enabled, the RAVB driver will no longer advertise gPTP
support to user-space.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
* Changes since v2
- Use full 100 character width instead of breaking at 80.
- Remove superfluous return statement.

* Changes since v1
- Use the standard generic ptp-timer property from
  ethernet-controller.yaml instead of a vendor specific one.
- Improve spelling in commit message.
---
 drivers/net/ethernet/renesas/ravb.h      |  3 ++
 drivers/net/ethernet/renesas/ravb_main.c | 54 +++++++++++++++++++++++-
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 27e2b31955ba..5d35b40d7c58 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -249,6 +249,8 @@ enum APSR_BIT {
 	APSR_RDM	= 0x00002000,
 	APSR_TDM	= 0x00004000,
 	APSR_MIISELECT	= 0x01000000,	/* R-Car V4M only */
+	APSR_GPTPTIMER_SOURCE = BIT(25), /* Gen4 */
+	APSR_GPTPCLOCK	= BIT(29),	/* Gen4 */
 };
 
 /* RCR */
@@ -1132,6 +1134,7 @@ struct ravb_private {
 	struct list_head ts_skb_list;
 	u32 ts_skb_tag;
 	struct ravb_ptp ptp;
+	struct device_node *of_gptp;	/* Reference to external gPTP clock, if any. */
 	spinlock_t lock;		/* Register access lock */
 	u32 cur_rx[NUM_RX_QUEUE];	/* Consumer ring indices */
 	u32 dirty_rx[NUM_RX_QUEUE];	/* Producer ring indices */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 8617c30dbb14..c8aa40136c3d 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2755,6 +2755,57 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.magic_pkt = 1,
 };
 
+static int ravb_gen4_ptp_probe(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	priv->of_gptp = of_parse_phandle(priv->pdev->dev.of_node, "ptp-timer", 0);
+	if (!priv->of_gptp)
+		return 0;
+
+	if (!of_device_is_available(priv->of_gptp)) {
+		of_node_put(priv->of_gptp);
+		priv->of_gptp = NULL;
+	}
+
+	return 0;
+}
+
+static int ravb_gen4_ptp_clock_index(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	/* If no clock, mimic ptp_clock_index_by_of_node() fail and return -1 */
+	if (!priv->of_gptp)
+		return -1;
+
+	return ptp_clock_index_by_of_node(priv->of_gptp);
+}
+
+static int ravb_gen4_ptp_set_config_mode(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int ret;
+
+	/* Enable gPTP Clock and Select High-speed peripheral bus clock. */
+	ret = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
+	if (ret)
+		return ret;
+
+	/* Set PTP source to GPTP module, only option on Gen4. */
+	if (priv->of_gptp)
+		ravb_modify(ndev, APSR, APSR_GPTPTIMER_SOURCE | APSR_GPTPCLOCK,
+			    APSR_GPTPTIMER_SOURCE | APSR_GPTPCLOCK);
+
+	return 0;
+}
+
+static const struct ravb_gptp_info ravb_gen4_ptp_info = {
+	.probe = ravb_gen4_ptp_probe,
+	.clock_index = ravb_gen4_ptp_clock_index,
+	.set_config_mode = ravb_gen4_ptp_set_config_mode,
+};
+
 static const struct ravb_hw_info ravb_gen4_hw_info = {
 	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
@@ -2777,7 +2828,7 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
 	.tx_counters = 1,
 	.multi_irqs = 1,
 	.irq_en_dis = 1,
-	.ptp = &ravb_gen3_ptp_info,
+	.ptp = &ravb_gen4_ptp_info,
 	.nc_queues = 1,
 	.magic_pkt = 1,
 };
@@ -3191,6 +3242,7 @@ static void ravb_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(dev);
 	clk_unprepare(priv->refclk);
+	of_node_put(priv->of_gptp);
 	reset_control_assert(priv->rstc);
 	free_netdev(ndev);
 	platform_set_drvdata(pdev, NULL);
-- 
2.55.0

Re: [PATCH net-next v4 01/10] net: ethernet: ravb: Remove gPTP control from WoL setup and restore

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: ravb: Remove gPTP control from WoL setup and restore

This removes the ccc_gac special case that stopped the gPTP clock in
ravb_wol_setup() and re-initialized it in ravb_wol_restore(), on the
grounds that ravb_open()/ravb_close() already handle gPTP start/stop.
A few questions about the suspend side of that reasoning below.

>     This makes the special case to stop and start it when resuming from WoL
>     redundant. As the ndev will always be closed and re-opened when
>     suspending and resuming the system.

[Severity: High]
Is the statement "the ndev will always be closed and re-opened when
suspending and resuming the system" accurate for the WoL path?  In
ravb_suspend() the WoL case returns before ravb_close() is reached:

	rtnl_lock();
	if (priv->wol_enabled) {
		ret = ravb_wol_setup(ndev);
		rtnl_unlock();
		return ret;
	}

	ret = ravb_close(ndev);

So on a WoL suspend, ravb_close() (and therefore its
"if (info->gptp || info->ccc_gac) ravb_ptp_stop(ndev);") does not run at
all during suspend; it only runs later, on the resume side, from the
ravb_close() call at the end of ravb_wol_restore().  Does that make the
suspend-side removal something other than a redundancy cleanup?

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ea1c7e536791..36173a09e2fd 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -3182,9 +3182,6 @@ static int ravb_wol_setup(struct net_device *ndev)
>  	/* Enable MagicPacket */
>  	ravb_modify(ndev, ECMR, ECMR_MPDE, ECMR_MPDE);
>  
> -	if (priv->info->ccc_gac)
> -		ravb_ptp_stop(ndev);
> -
>  	return enable_irq_wake(priv->emac_irq);
>  }

[Severity: High]
Following on from the commit message question: was this call also doing
gPTP interrupt masking that nothing else on this path does?  ravb_ptp_stop()
does more than unregister the PHC:

drivers/net/ethernet/renesas/ravb_ptp.c:
static void ravb_ptp_disable(struct net_device *ndev)
{
	ravb_write(ndev, 0, GIC);
	ravb_write(ndev, 0, GIS);
}
...
void ravb_ptp_stop(struct net_device *ndev)
{
	...
	ravb_ptp_disable(ndev);
	ravb_ptp_sync_irqs(ndev);

ravb_wol_setup() clears RIC0/RIC2/TIC and writes ECSIPR, and only
synchronizes and wake-enables priv->emac_irq.  It never touches GIC/GIS,
and it does not synchronize ndev->irq/err_irq/mgmt_irq.  The GIC bits are
armed from userspace via ravb_ptp_extts() (GIC_PTCE) and ravb_ptp_perout()
(GIC_PTME) and are cleared only by ravb_ptp_disable().

Since the WoL path also skips pm_runtime_force_suspend() and
reset_control_assert(), the block stays powered while suspended, so a
programmed compare/capture can still fire on the now-suspended non-wake
line and be latched and replayed by resume_device_irqs().  ravb uses
SYSTEM_SLEEP_PM_OPS(ravb_suspend, ravb_resume), so that replay happens
before ravb_resume() runs.

Can the replayed interrupt then reach ravb_ptp_interrupt() while
ravb_wol_restore() is resetting the block?

	ravb_interrupt()/ravb_multi_interrupt()
	  -> ravb_ptp_interrupt()
	       gis &= ravb_read(ndev, GIC);
	       if ((gis & GIS_PTCF) && clock) {
		       ...
		       event.timestamp = ravb_read(ndev, GCPT);
		       ptp_clock_event(clock, &event);

priv->ptp.clock is still non-NULL after this change, so would that push a
timestamp read from a just-reset GCPT to userspace, and RMW GIS/GCCR/GPTC
concurrently with the unlocked ravb_set_opmode()/ravb_set_config_mode() in
ravb_wol_restore()?

Also, does the remaining "/* Only allow ECI interrupts */" comment still
describe the code for ccc_gac parts, and is the PHC (/dev/ptpN) now meant
to stay registered for the whole suspended interval?

The restore-side removal of ravb_ptp_init() does look redundant; would it
make sense to keep the suspend-side masking and synchronization, or to
spell out in the commit message why dropping it is safe?

> @@ -3204,9 +3201,6 @@ static int ravb_wol_restore(struct net_device *ndev)
>  	if (error)
>  		return error;
>  
> -	if (priv->info->ccc_gac)
> -		ravb_ptp_init(ndev, priv->pdev);
> -

[Severity: Medium]
After this change, is ravb_wol_restore() the only place that drives the
AVB-DMAC through CCC_OPC_RESET while the gPTP PHC is still registered?
The reset and config-mode re-entry happen at the top of the function:

	/* Set reset mode to rearm the WoL logic. */
	error = ravb_set_opmode(ndev, CCC_OPC_RESET);
	if (error)
		return error;

	/* Set AVB config mode. */
	error = ravb_set_config_mode(ndev);

The other opmode transition sites stop the PHC first.  ravb_close():

	/* Stop PTP Clock driver */
	if (info->gptp || info->ccc_gac)
		ravb_ptp_stop(ndev);

and the ravb_open() error path:

out_ptp_stop:
	/* Stop PTP Clock driver */
	if (info->gptp || info->ccc_gac)
		ravb_ptp_stop(ndev);
	ravb_stop_dma(ndev);
out_set_reset:
	ravb_set_opmode(ndev, CCC_OPC_RESET);

The reset drops the GCCR TCSS=ADJGPTP selection and GTI programming that
ravb_ptp_init() установ establishes, and priv->ptp.default_addend /
current_addend still describe the pre-reset hardware; the clock is
unregistered only later, by the ravb_close() at the end of the same
function.  Pre-patch the ccc_gac branch unregistered the PHC in
ravb_wol_setup() and re-created it here, so no registered PHC spanned the
reset.  Should ravb_wol_restore() follow the same ordering as its peers?

>  	if (info->nc_queues)
>  		napi_enable(&priv->napi[RAVB_NC]);
>  	napi_enable(&priv->napi[RAVB_BE]);

[Severity: High]
This isn't a bug introduced by this patch, but while both functions are
being touched: can the NAPI enable/disable counts get out of balance on
the WoL error paths, and hang in napi_disable()?

ravb_wol_setup() disables both NAPI instances and then returns the
enable_irq_wake() result without re-enabling them:

	if (info->nc_queues)
		napi_disable(&priv->napi[RAVB_NC]);
	napi_disable(&priv->napi[RAVB_BE]);
	ravb_write(ndev, ECSIPR_MPDIP, ECSIPR);
	...
	return enable_irq_wake(priv->emac_irq);

ravb_suspend() propagates that error directly.  On the restore side, the
two early returns above skip the napi_enable() calls when ravb_wait()
times out.

Since the WoL suspend path never closes the interface, the netdev stays
netif_running() with NAPI disabled, and a failed suspend callback leaves
dev->power.is_suspended clear so ravb_resume() is skipped.  The next
ravb_close() (from "ip link set dev X down" or unregister_netdev()), or a
retried suspend re-entering ravb_wol_setup(), calls napi_disable() again:

net/core/dev.c:napi_disable_locked() {
	...
		while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
			usleep_range(20, 200);
			val = READ_ONCE(n->state);
		}

		new = val | NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC;
	...
}

A completed disable leaves SCHED|NPSVC set and only napi_enable() clears
them, so does the second disable spin here with no timeout while rtnl and
the netdev instance lock are held?

Re: [PATCH net-next v4 02/10] net: ethernet: ravb: Move programming of gPTP timer interval

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: ravb: Move programming of gPTP timer interval

This moves the GTI write and the GCCR.LTI load request out of
ravb_set_gti() in the main driver logic and into ravb_ptp_init(), so the
programming happens under priv->lock.  ravb_ptp_init() now also seeds
priv->ptp.default_addend from priv->gti_tiv instead of reading GTI back
from the hardware.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 36173a09e2fd..a89f6fe7866f 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1846,20 +1846,6 @@ static int ravb_set_config_mode(struct net_device *ndev)
>  	return error;
>  }
>  
> -static void ravb_set_gti(struct net_device *ndev)
> -{
> -	struct ravb_private *priv = netdev_priv(ndev);
> -	const struct ravb_hw_info *info = priv->info;
> -
> -	if (!(info->gptp || info->ccc_gac))
> -		return;
> -
> -	ravb_write(ndev, priv->gti_tiv, GTI);
> -
> -	/* Request GTI loading */
> -	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> -}
> -

[ ... ]

> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index 43218bc15b15..157fb78209c7 100644
> --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
> @@ -321,11 +321,20 @@ void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
>  
>  	priv->ptp.info = ravb_ptp_info;
>  
> -	priv->ptp.default_addend = ravb_read(ndev, GTI);
> +	priv->ptp.default_addend = priv->gti_tiv;
>  	priv->ptp.current_addend = priv->ptp.default_addend;
>  
>  	spin_lock_irqsave(&priv->lock, flags);
> +
> +	/* Set gPTP Timer Increment Value. */
> +	ravb_write(ndev, priv->ptp.default_addend, GTI);
> +
> +	/* Request GTI loading. */
> +	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +
> +	/* Wait for GIT loading to complete. */
>  	ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ);
> +
>  	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);

[Severity: Medium]
Does this ravb_wait() actually observe the GTI load?  The load request is
GCCR.LTI, but the poll masks GCCR.TCR, and those are separate fields in
ravb.h:

	GCCR_TCR	= 0x00000003,
	GCCR_TCR_NOREQ	= 0x00000000, /* No request */
	GCCR_TCR_RESET	= 0x00000001, /* gPTP/AVTP presentation timer reset */
	GCCR_TCR_CAPTURE = 0x00000003, /* Capture value set in GCCR.TCSS */
	GCCR_LTO	= 0x00000004,
	GCCR_LTI	= 0x00000008,

Setting LTI does not change TCR, and ravb_ptp_tcr_request() both issues and
waits out its TCR request under priv->lock, so no TCR request is outstanding
here.  Doesn't that make the poll return on its first read, leaving the
GTI load unwaited-for?

If so, could the new comment be reworded?  It describes waiting for the
increment load, while the wait is really draining a pending TCR request
before switching GCCR.TCSS.  The comment also spells GTI as "GIT".

The same reasoning applies to the changelog:

  "as the first action of the gPTP init function is to wait for the timer
  interval programming to be acknowledge by the hardware"

Since that first action polls TCR rather than LTI, does the stated
justification for the move still hold?  (There is also a typo, "to be
acknowledge" should be "to be acknowledged".)

There is a follow-on question about the ravb_modify() immediately after the
wait.  It is a read-modify-write of GCCR:

drivers/net/ethernet/renesas/ravb_main.c:ravb_modify() {
	ravb_write(ndev, (ravb_read(ndev, reg) & ~clear) | set, reg);
}

If LTI is still asserted at that point, doesn't this read it back and write
it out again, re-issuing the load request while switching the capture
source?  The driver's own runtime update path treats LTI as the pending
indicator and declines to touch the registers in that state:

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_adjfine() {
	gccr = ravb_read(ndev, GCCR);
	if (gccr & GCCR_LTI) {
		spin_unlock_irqrestore(&priv->lock, flags);
		return -EBUSY;
	}
	ravb_write(ndev, addend & GTI_TIV, GTI);
	ravb_write(ndev, gccr | GCCR_LTI, GCCR);
	...
}

Relatedly, the new code writes GTI without first checking that LTI is
clear.  On the re-init paths that reach ravb_ptp_init() from
ravb_set_ringparam() and ravb_tx_timeout_work() (including the "goto out"
case where ravb_stop_dma() failed and the MAC is still running), can a load
requested by a preceding adjfine() be overwritten while still in flight?
Would polling for (GCCR & GCCR_LTI) == 0 before the GTI write, and again
after requesting the load, be the wait that was intended here?

This behaviour is unchanged at the end of the series (c7fa563cd046 still
carries the same comment and the same GCCR_TCR poll).

[Severity: Medium]
This isn't a bug introduced by this patch, but the return value of that
ravb_wait() is discarded, and it was already unchecked before the move.
ravb_wait() can fail:

drivers/net/ethernet/renesas/ravb_main.c:ravb_wait() {
	for (i = 0; i < 10000; i++) {
		if ((ravb_read(ndev, reg) & mask) == value)
			return 0;
		udelay(10);
	}
	return -ETIMEDOUT;
}

On -ETIMEDOUT the code still programs GCCR.TCSS_ADJGPTP and registers the
PTP clock, so the capture source can be reprogrammed with a reset/capture
request still outstanding.  Since ravb_ptp_init() returns void at this
revision there is nothing to propagate, so this would be a separate change
rather than something for this patch.

>  	spin_unlock_irqrestore(&priv->lock, flags);
>

Re: [PATCH net-next v4 03/10] net: ethernet: ravb: Simplify gPTP start and stop

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: ravb: Simplify gPTP start and stop

The patch folds the gptp-flag ravb_ptp_init() and ravb_ptp_stop() calls into
ravb_dmac_init() and ravb_stop_dma(), and drops the info->gptp part of the
two combined gptp/ccc_gac call sites in ravb_open() and ravb_close().  It is
described as having no functional change.

A few questions about the resulting start/stop balance below.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index a89f6fe7866fe..2d0022a4f85c3 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -707,7 +707,15 @@ static int ravb_dmac_init(struct net_device *ndev)
>  		return error;
>  
>  	/* Setting the control will start the AVB-DMAC process. */
> -	return ravb_set_opmode(ndev, CCC_OPC_OPERATION);
> +	error = ravb_set_opmode(ndev, CCC_OPC_OPERATION);
> +	if (error)
> +		return error;
> +
> +	/* Initialise PTP Clock driver */
> +	if (info->gptp)
> +		ravb_ptp_init(ndev, priv->pdev);
> +
> +	return 0;
>  }
>  
>  static void ravb_get_tx_tstamp(struct net_device *ndev)
> @@ -1115,6 +1123,10 @@ static int ravb_stop_dma(struct net_device *ndev)
>  			netdev_err(ndev, "failed to stop AXI BUS\n");
>  	}
>  
> +	/* Stop PTP Clock driver */
> +	if (info->gptp)
> +		ravb_ptp_stop(ndev);
> +

[Severity: High]
Is the new stop reachable on all exits from ravb_stop_dma()?  It sits after
three unconditional error returns earlier in the same function:

	error = ravb_wait(ndev, TCCR, info->tccr_mask, 0);
	if (error)
		return error;

	error = ravb_wait(ndev, CSR, CSR_TPO0 | CSR_TPO1 | CSR_TPO2 | CSR_TPO3,
			  0);
	if (error)
		return error;
	...
	error = ravb_wait(ndev, CSR, CSR_RPO, 0);
	if (error)
		return error;

Before this patch, ravb_close(), ravb_set_ringparam(),
ravb_tx_timeout_work() and ravb_open()'s unwind all called ravb_ptp_stop()
unconditionally before ravb_stop_dma(), so a wait timeout did not affect the
unregister.

ravb_close() treats a ravb_stop_dma() failure as non-fatal:

	/* Set the config mode to stop the AVB-DMAC's processes */
	if (ravb_stop_dma(ndev) < 0)
		netdev_err(ndev,
			   "device will be stopped after h/w processes are done.\n");

and then goes on to free the rings, program CCC_OPC_RESET and drop the
runtime-PM reference.  For .gptp devices (ravb_gen2_hw_info,
ravb_rzv2m_hw_info) does that leave the PHC registered after ndo_stop has
returned success, with the gPTP interrupt enables still set because
ravb_ptp_disable()/ravb_ptp_sync_irqs() in ravb_ptp_stop() were skipped?

A subsequent open then runs ravb_dmac_init() -> ravb_ptp_init(), whose

	WRITE_ONCE(priv->ptp.clock, clock);

overwrites the only handle, while ravb_ptp_stop() only xchg()s the current
pointer.  Since ravb_remove() has no independent PTP unregister and ends in
free_netdev(), does the stranded registration leave ptp_clock_info pointing
into freed netdev private memory, reachable through the surviving
/dev/ptpN?

ravb_ptp_stop() looks idempotent (xchg of priv->ptp.clock), so would doing
the stop before the fallible ravb_wait() calls, or on all exit paths, keep
the old behaviour?

>  	/* Stop AVB-DMAC process */
>  	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
>  }
> @@ -1719,9 +1731,7 @@ static int ravb_set_ringparam(struct net_device *ndev,
>  
>  	if (netif_running(ndev)) {
>  		netif_device_detach(ndev);
> -		/* Stop PTP Clock driver */
> -		if (info->gptp)
> -			ravb_ptp_stop(ndev);
> +
>  		/* Wait for DMA stopping */
>  		error = ravb_stop_dma(ndev);
>  		if (error) {

[Severity: High]
Same question for this call site: with the explicit pre-stop removed, does a
ravb_stop_dma() timeout here return with the interface detached and the Gen2
gPTP clock still registered?

[ ... ]

> @@ -1961,7 +1967,7 @@ static int ravb_open(struct net_device *ndev)
>  	ravb_emac_init(ndev);
>  
>  	/* Initialise PTP Clock driver */
> -	if (info->gptp || info->ccc_gac)
> +	if (info->ccc_gac)
>  		ravb_ptp_init(ndev, priv->pdev);
>  
>  	/* PHY control start */
> @@ -1974,9 +1980,6 @@ static int ravb_open(struct net_device *ndev)
>  	return 0;
>  
>  out_ptp_stop:
> -	/* Stop PTP Clock driver */
> -	if (info->gptp || info->ccc_gac)
> -		ravb_ptp_stop(ndev);
>  	ravb_stop_dma(ndev);
>  out_set_reset:
>  	ravb_set_opmode(ndev, CCC_OPC_RESET);

[Severity: High]
Does this leak the registered PTP clock for info->ccc_gac devices when
ravb_open() fails?

ravb_open() still calls ravb_ptp_init() for info->ccc_gac, but the
counterpart at out_ptp_stop: is gone, and the relocated teardown in
ravb_stop_dma() is gated on:

	if (info->gptp)
		ravb_ptp_stop(ndev);

info->gptp and info->ccc_gac are mutually exclusive (.gptp in
ravb_gen2_hw_info and ravb_rzv2m_hw_info, .ccc_gac in ravb_gen3_hw_info and
ravb_gen4_hw_info), so nothing unregisters the clock on this path.

The failure right after the init is a plain software path, ravb_phy_start()
-> ravb_phy_init() returns -ENOENT when of_phy_connect() fails, or
propagates an of_phy_register_fixed_link() error:

	error = ravb_phy_init(ndev);
	if (error)
		return error;

Since ndo_open failed, ndo_stop/ravb_close() is never called, so the
if (info->ccc_gac) ravb_ptp_stop(ndev); in ravb_close() does not run either.
Does that leave /dev/ptpN live on a block that ravb_open() then puts into
CCC_OPC_RESET and runtime-suspends, so gettime64/settime64/adjfine perform
MMIO with no runtime-PM reference held, and with GIC/GIS enables still set?

And on a later successful open, ravb_ptp_init()'s
WRITE_ONCE(priv->ptp.clock, clock) replaces the handle, so the first
registration can no longer be reached by ravb_ptp_stop().  With
ravb_remove() calling free_netdev() without any PHC unregister, does that
become a use-after-free through the surviving PTP chardev?

Would keeping if (info->ccc_gac) ravb_ptp_stop(ndev); at out_ptp_stop:
restore the balance?

> @@ -2020,10 +2023,6 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  
>  	netif_tx_stop_all_queues(ndev);
>  
> -	/* Stop PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_stop(ndev);
> -
>  	/* Wait for DMA stopping */
>  	if (ravb_stop_dma(ndev)) {
>  		/* If ravb_stop_dma() fails, the hardware is still operating
> @@ -2056,10 +2055,6 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  	ravb_emac_init(ndev);
>  
>  out:
> -	/* Initialise PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_init(ndev, priv->pdev);
> -
>  	netif_tx_start_all_queues(ndev);
>  
>  out_unlock:

[Severity: Medium]
Can TX-timeout recovery now resume traffic with the Gen2 gPTP clock left
unregistered?

In ravb_stop_dma() the new stop runs just before the fallible final
transition:

	/* Stop PTP Clock driver */
	if (info->gptp)
		ravb_ptp_stop(ndev);

	/* Stop AVB-DMAC process */
	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);

If that ravb_set_opmode() times out, ravb_tx_timeout_work() takes the
"hardware is still operating" branch:

	if (ravb_stop_dma(ndev)) {
		...
		ravb_rcv_snd_enable(ndev);
		goto out;
	}
	...
out:
	netif_tx_start_all_queues(ndev);

With the ravb_ptp_init() at out: removed, RX/TX and the TX queues are
re-enabled for .gptp devices while the PHC has been unregistered and the
gPTP interrupts cleared.  Previously this path always re-registered the
clock when resuming operation.  Does hardware timestamping and /dev/ptpN
now stay gone until a full close/open cycle?

[ ... ]

Re: [PATCH net-next v4 05/10] net: ethernet: ravb: Propagate error from ptp_clock_register()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: ravb: Propagate error from ptp_clock_register()

ravb_ptp_init() is changed to return an int so that a failing
ptp_clock_register() can be propagated to its callers.  Along with that,
the cached priv->ptp.phc_index field is dropped and ravb_get_ts_info()
now reads the index directly from priv->ptp.clock.

Link: https://sashiko.dev/#/patchset/20260610102432.3538432-2-niklas.soderlund+renesas@ragnatech.se?part=1

[Severity: Medium]
The changelog only mentions the error propagation:

    If the PTP clock registration failed the error is silently ignored.
    Before reworking all callers of ravb_ptp_init() handle and propagate the
    error so it can be used.

But the diff also removes the substance of two recent stable-tagged fixes.
git blame on the removed lines points at:

  1f77af0aaf27 ("net: ravb: avoid dereferencing an invalid PTP clock")
      Fixes: a0d2f20650e8, Cc: stable
      -> added priv->ptp.phc_index so ravb_get_ts_info() would not have to
         dereference the clock pointer at all

  1cb9663789c5 ("net: ravb: serialize PTP clock teardown")
      Fixes: a0d2f20650e8, Cc: stable
      -> added the READ_ONCE()/WRITE_ONCE()/xchg() annotations and required
         detaching priv->ptp.clock before disabling the gPTP interrupts and
         unregistering

Both mechanisms are undone here.  The only rationale given is below the ---
line ("Rework to remove the caching of phc_index added by LLM without
access to hardware"), which is dropped when the patch is applied, so git
history will not record that two stable fixes were reverted.

Could the commit message say that these two fixes are being reverted and
explain why the new form is safe?

> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index aa45f5466001d..0c122a8158409 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1028,7 +1028,6 @@ struct ravb_ptp_perout {
>  struct ravb_ptp {
>  	struct ptp_clock *clock;
>  	struct ptp_clock_info info;
> -	int phc_index;
>  	u32 default_addend;
>  	u32 current_addend;
>  	int extts[N_EXT_TS];
> @@ -1163,7 +1162,7 @@ void ravb_modify(struct net_device *ndev, enum ravb_reg reg, u32 clear,
>  int ravb_wait(struct net_device *ndev, enum ravb_reg reg, u32 mask, u32 value);
>  
>  void ravb_ptp_interrupt(struct net_device *ndev);
> -void ravb_ptp_init(struct net_device *ndev);
> +int ravb_ptp_init(struct net_device *ndev);
>  void ravb_ptp_stop(struct net_device *ndev);
>  
>  #endif	/* #ifndef __RAVB_H__ */
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index e44d065f77cc8..5ba322593a014 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1774,7 +1774,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *hw_info = priv->info;
>  
> -	if (hw_info->gptp || hw_info->ccc_gac) {
> +	if (priv->ptp.clock && (hw_info->gptp || hw_info->ccc_gac)) {

[Severity: Medium]
Does this change what ethtool -T reports for a closed interface?

priv->ptp.clock is only non-NULL between ravb_ptp_init() (called from
ravb_dmac_init() for gptp parts and from ravb_open() for ccc_gac parts) and
ravb_ptp_stop(), so it is NULL whenever the interface is down.  It is also
always NULL with CONFIG_PTP_1588_CLOCK=n, since ravb only depends on
PTP_1588_CLOCK_OPTIONAL and the stub ptp_clock_register() returns NULL
(IS_ERR() is false in that case).

Previously the capability bits were reported unconditionally and only
info->phc_index was -1.  Now the whole block is skipped, so
so_timestamping, tx_types and rx_filters all come back empty, while
ravb_hwtstamp_set() still accepts HWTSTAMP_TX_ON and
HWTSTAMP_FILTER_PTP_V2_L2_EVENT and ravb_hwtstamp_get() still reports them:

	priv->tstamp_tx_ctrl = tstamp_tx_ctrl;
	priv->tstamp_rx_ctrl = tstamp_rx_ctrl;

Should the capability bits stay gated on the hardware feature flags, with
only phc_index depending on a registered clock?

This behaviour is still present at the end of the series, where
ravb_get_ts_info() gates everything on hw_info->ptp->clock_index(ndev) >= 0
and ravb_gen2_ptp_clock_index() returns -ENODEV when priv->ptp.clock is
NULL.

>  		info->so_timestamping =
>  			SOF_TIMESTAMPING_TX_SOFTWARE |
>  			SOF_TIMESTAMPING_TX_HARDWARE |
> @@ -1785,7 +1785,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
>  			(1 << HWTSTAMP_FILTER_NONE) |
>  			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
>  			(1 << HWTSTAMP_FILTER_ALL);
> -		info->phc_index = READ_ONCE(priv->ptp.phc_index);
> +		info->phc_index = ptp_clock_index(priv->ptp.clock);

[Severity: High]
Can this dereference a freed struct ptp_clock?

There are now two unsynchronized loads of priv->ptp.clock in
ravb_get_ts_info(): the NULL test and the dereference inside
ptp_clock_index(), which is just:

drivers/ptp/ptp_clock.c:
    int ptp_clock_index(struct ptp_clock *ptp)
    {
	    return ptp->index;
    }

ravb_get_ts_info() is not always called under rtnl.  __ethtool_get_ts_info()
calls ops->get_ts_info(dev, info) with only rcu_read_lock() held, and it is
reached from the setsockopt path:

  sock_timestamping_bind_phc()
    -> ethtool_get_phc_vclocks()
      -> __ethtool_get_ts_info()
        -> ravb_get_ts_info()

So a plain user process with a socket bound to the interface can be inside
the if () block while another CPU runs ip link set down:

  ravb_close()/ravb_stop_dma()
    -> ravb_ptp_stop()
      -> ptp_clock_unregister()
        -> put_device(&ptp->dev)
          -> ptp_clock_release()   /* object freed here */

rcu_read_lock() does not help, because ptp_clock_unregister() frees
synchronously without a grace period.  If the compiler reloads the field
between the test and the call, the same window yields a NULL dereference
instead.

The old code read a single cached int that had already been invalidated to
-1 before unregistering, which is what 1f77af0aaf27 ("net: ravb: avoid
dereferencing an invalid PTP clock") was for.  The same two-load pattern
appears at the end of the series in ravb_gen2_ptp_clock_index(), so it is
not addressed later.

>  	}
>  
>  	return 0;

[ ... ]

> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index f70b616ac847c..37161c0579ce7 100644
> --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
> @@ -313,11 +313,11 @@ void ravb_ptp_interrupt(struct net_device *ndev)
>  	ravb_write(ndev, ~(gis | GIS_RESERVED), GIS);
>  }
>  
> -void ravb_ptp_init(struct net_device *ndev)
> +int ravb_ptp_init(struct net_device *ndev)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> -	struct ptp_clock *clock;
>  	unsigned long flags;
> +	int ret = 0;
>  
>  	priv->ptp.info = ravb_ptp_info;
>  
> @@ -338,15 +338,14 @@ void ravb_ptp_init(struct net_device *ndev)
>  	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
>  	spin_unlock_irqrestore(&priv->lock, flags);
>  
> -	clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
> -	if (IS_ERR(clock)) {
> -		netdev_err(ndev, "failed to register PTP clock: %pe\n", clock);
> -		clock = NULL;
> +	priv->ptp.clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
> +	if (IS_ERR(priv->ptp.clock)) {
> +		ret = PTR_ERR(priv->ptp.clock);
> +		priv->ptp.clock = NULL;
> +		ravb_ptp_stop(ndev);
>  	}

[Severity: Medium]
Is it safe to store the ERR_PTR into the shared field before normalizing it?

On failure, priv->ptp.clock briefly holds an error pointer such as
(void *)-ENOMEM before the next statement sets it to NULL.  Neither
consumer validates for error pointers: ravb_ptp_interrupt() passes the
value straight to ptp_clock_event() after a NULL check only, and
ravb_get_ts_info() calls ptp_clock_index() on it after a NULL check only.

The reader that can hit this window without rtnl is the one described
above:

  setsockopt(SO_TIMESTAMPING | SOF_TIMESTAMPING_BIND_PHC)
    -> sock_timestamping_bind_phc()
      -> ethtool_get_phc_vclocks()
        -> __ethtool_get_ts_info()
          -> ravb_get_ts_info()
            -> ptp_clock_index(priv->ptp.clock)

The baseline registered into a local, normalized IS_ERR() to NULL and only
then published with WRITE_ONCE(), which 1f77af0aaf27 justified with
"Registration failures can also leave an error pointer in
priv->ptp.clock."  Would registering into a local and publishing only the
normalized value keep that property?  The publish-then-normalize sequence
is unchanged at the end of the series.

[Severity: Low]
At this commit the netdev_err() is gone but nothing consumes the new return
value yet:

drivers/net/ethernet/renesas/ravb_main.c:ravb_dmac_init() {
	/* Initialise PTP Clock driver */
	if (info->gptp)
		ravb_ptp_init(ndev);

	return 0;
}

drivers/net/ethernet/renesas/ravb_main.c:ravb_open() {
	/* Initialise PTP Clock driver */
	if (info->ccc_gac)
		ravb_ptp_init(ndev);

	/* PHY control start */
	error = ravb_phy_start(ndev);
	...
}

So a ptp_clock_register() failure is more silent at this commit than
before: no log message, no propagated error, and both callers continue as
if PTP had been set up.  This is resolved later in the series by "net:
ethernet: ravb: Replace gPTP flags with callbacks", where ravb_dmac_init()
returns info->ptp->dmac_start(ndev) and ravb_open() jumps to out_ptp_stop
on failure, so it may only be worth a note in the changelog that the
diagnostic disappears for one commit.

>  
> -	WRITE_ONCE(priv->ptp.clock, clock);
> -	if (clock)
> -		WRITE_ONCE(priv->ptp.phc_index, ptp_clock_index(clock));
> +	return ret;
>  }
>  
>  static void ravb_ptp_disable(struct net_device *ndev)
> @@ -369,14 +368,12 @@ static void ravb_ptp_sync_irqs(struct net_device *ndev)
>  void ravb_ptp_stop(struct net_device *ndev)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> -	struct ptp_clock *clock;
> -
> -	WRITE_ONCE(priv->ptp.phc_index, -1);
> -	clock = xchg(&priv->ptp.clock, NULL);
>  
>  	ravb_ptp_disable(ndev);
>  	ravb_ptp_sync_irqs(ndev);
>  
> -	if (clock)
> -		ptp_clock_unregister(clock);
> +	if (priv->ptp.clock) {
> +		ptp_clock_unregister(priv->ptp.clock);
> +		priv->ptp.clock = NULL;
> +	}
>  }

[Severity: High]
Can ravb_ptp_interrupt() still see the clock while it is being freed?

The new order in ravb_ptp_stop() is GIC=0/GIS=0, synchronize_irq(),
ptp_clock_unregister(), and only then the NULL store.  The pointer stays
published to the hardirq handler, which still reads it locklessly, for the
whole duration of the unregister:

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_interrupt() {
	struct ptp_clock *clock = READ_ONCE(priv->ptp.clock);
	u32 gis = ravb_read(ndev, GIS);

	gis &= ravb_read(ndev, GIC);
	if ((gis & GIS_PTCF) && clock) {
		...
		ptp_clock_event(clock, &event);
	}
	...
}

The only remaining guard is GIC being zero, but GIC is not a write-once
teardown latch.  The PTP core .enable callbacks re-arm it from /dev/ptpN
ioctls:

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_extts() {
	if (!info->irq_en_dis)
		ravb_modify(ndev, GIC, GIC_PTCE, on ? GIC_PTCE : 0);
	else if (on)
		ravb_write(ndev, GIE_PTCS, GIE);
}

Those run under priv->lock, which ravb_ptp_disable() does not take, and
there is no teardown flag.  So a PTP_EXTTS_REQUEST or PTP_PEROUT_REQUEST
landing after ravb_ptp_disable()/synchronize_irq() re-enables the gPTP
interrupt, and a following interrupt sees gis & GIC non-zero and hands the
clock that is being unregistered to ptp_clock_event(), which takes
ptp->tsevqs_lock and walks ptp->tsevqs.  ptp_clock_unregister() drops the
last device reference:

drivers/ptp/ptp_clock.c:ptp_clock_unregister() {
	...
	/* The final put, normally here, will invoke ptp_clock_release(). */
	put_device(&ptp->dev);
}

The xchg() that this hunk removes made the handler observe NULL before GIC
was touched, which is what 1cb9663789c5 described: "Atomically detach it
with xchg() before disabling PTP interrupts, then synchronize all IRQs ...
before unregistering the detached clock."

A secondary effect of the same window: the re-armed enable bit is never
cleared again, so the gPTP capture/compare interrupt stays enabled with no
registered clock and the GIS_PTMF branch keeps advancing
priv->ptp.perout->target.

Would keeping the detach-before-disable ordering, or adding explicit
serialization against the .enable callbacks, close this?  This code is
unchanged at the end of the series.

[Severity: Medium]
This one is not introduced by this patch but by the earlier series patch
"net: ethernet: ravb: Simplify gPTP start and stop"; mentioning it here
since it concerns the same teardown path.

On gptp parts, ravb_stop_dma() tears down PTP before a step that can fail:

drivers/net/ethernet/renesas/ravb_main.c:ravb_stop_dma() {
	...
	/* Stop PTP Clock driver */
	if (info->gptp)
		ravb_ptp_stop(ndev);

	/* Stop AVB-DMAC process */
	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
}

If that final opmode transition times out waiting for CSR_OPS, the tx
timeout worker takes the "hardware is still operating" branch:

drivers/net/ethernet/renesas/ravb_main.c:ravb_tx_timeout_work() {
	if (ravb_stop_dma(ndev)) {
		/* ... this just re-enables the TX and RX and skip the following
		 * re-initialization procedure. */
		ravb_rcv_snd_enable(ndev);
		goto out;
	}
	...
out:
	netif_tx_start_all_queues(ndev);
}

That path skips ravb_dmac_init(), the only remaining caller of
ravb_ptp_init() for these parts, so the interface resumes traffic with
/dev/ptpN removed and GIC/GIS zeroed until the interface is taken down and
up again.  Before the series, the timeout path re-ran ravb_ptp_init() at
its common out: label.  Should the stop path leave PTP alone until the
opmode change has succeeded, or should the recovery branch re-register the
clock?

Re: [PATCH net-next v4 06/10] net: ethernet: ravb: Replace gPTP flags with callbacks

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: ravb: Replace gPTP flags with callbacks

This replaces the info->gptp and info->ccc_gac hardware feature flags with
a new struct ravb_gptp_info callback table hanging off struct ravb_hw_info.
Gen2 and RZ/V2M get ravb_gen2_ptp_info (dmac_start/dmac_stop), Gen3 and Gen4
get ravb_gen3_ptp_info (ndev_open/ndev_close).

A few questions on the conversion below.

> net: ethernet: ravb: Replace gPTP flags with callbacks
>
> Prepare for adding Gen4 support which will add a third and new way to
> interact with the gPTP clock by replacing the flags for Gen2 behavior
> (info->gptp) and Gen3 behavior (info->ccc_gac) with callbacks.

[Severity: Low]
The message describes this as replacing flags with callbacks, but the patch
also changes error semantics.  Before this change both call sites discarded
the return value:

	if (info->gptp)
		ravb_ptp_init(ndev);        /* ravb_dmac_init() */

	if (info->ccc_gac)
		ravb_ptp_init(ndev);        /* ravb_open() */

After this change the value is returned from ravb_dmac_init() and acted on
in ravb_open() with a goto out_ptp_stop.  So a ptp_clock_register() failure
in ravb_ptp_init() that previously only cost hardware timestamping now makes
bringing the interface up fail on every generation.

The only mention is the "Handle error from starting the PTP clock" note
below the --- marker, which does not end up in git history.  Could the
commit message say that error handling behaviour changes too?

> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 0c122a8158409..6603f90b19e15 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1034,6 +1034,27 @@ struct ravb_ptp {
>  	struct ravb_ptp_perout perout[N_PER_OUT];
>  };
>  
> +/**
> + * struct ravb_gptp_info - Platform specific gPTP behavior
> + *
> + * Each generation of RAVB have slightly different behaviors when interacting
> + * with the gPTP clock. This struct provides the callbacks to be called at
> + * critical points in the RAVB driver.
> + *
> + * @set_config_mode:	Enter config mode
> + * @dmac_start:		Called when the DMAC starts
> + * @dmac_stop:		Called when the DMAC stops
> + * @ndev_open:		Called when the ndev is opened
> + * @ndev_close:		Called when the ndev is closed
> + */
> +struct ravb_gptp_info {
> +	int (*set_config_mode)(struct net_device *ndev);
> +	int (*dmac_start)(struct net_device *ndev);
> +	void (*dmac_stop)(struct net_device *ndev);
> +	int (*ndev_open)(struct net_device *ndev);
> +	void (*ndev_close)(struct net_device *ndev);
> +};
> +

[ ... ]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 5ba322593a014..9e9b1c4fbe956 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -712,8 +712,8 @@ static int ravb_dmac_init(struct net_device *ndev)
>  		return error;
>  
>  	/* Initialise PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_init(ndev);
> +	if (info->ptp && info->ptp->dmac_start)
> +		return info->ptp->dmac_start(ndev);
>  
>  	return 0;
>  }

[Severity: Medium]
Should ravb_dmac_init() unwind the DMAC start when dmac_start() fails?

The new return is reached after the block already entered
CCC_OPC_OPERATION and after info->dmac_init() allocated and armed the
descriptor rings:

	/* Setting the control will start the AVB-DMAC process. */
	error = ravb_set_opmode(ndev, CCC_OPC_OPERATION);
	if (error)
		return error;

	/* Initialise PTP Clock driver */
	if (info->ptp && info->ptp->dmac_start)
		return info->ptp->dmac_start(ndev);

For Gen2 and RZ/V2M dmac_start is ravb_ptp_init(), which can fail on a
plain software condition when ptp_clock_register() returns an ERR_PTR.
Previously every ravb_dmac_init() failure returned with the block still in
CONFIG mode, and its three callers were written for that.

ravb_tx_timeout_work() documents the old invariant explicitly:

	error = ravb_dmac_init(ndev);
	if (error) {
		/* If ravb_dmac_init() fails, descriptors are freed. So, this
		 * should return here to avoid re-enabling the TX and RX in
		 * ravb_emac_init().
		 */
		netdev_err(ndev, "%s: ravb_dmac_init() failed, error %d\n",
			   __func__, error);
		goto out_unlock;
	}

With the new failure mode the descriptors are armed and the DMAC is left in
OPERATION mode, while ravb_emac_init() is skipped and the TX queues stay
stopped.  ravb_set_ringparam() has the same shape and returns with
netif_device_detach() still in effect.  In ravb_open() the error goes to
out_set_reset, which skips ravb_stop_dma() entirely.

Would it be simpler to start the gPTP clock before the switch to
CCC_OPC_OPERATION, so a failure still leaves the block in CONFIG mode?

> @@ -1124,8 +1124,8 @@ static int ravb_stop_dma(struct net_device *ndev)
>  	}
>  
>  	/* Stop PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_stop(ndev);
> +	if (info->ptp && info->ptp->dmac_stop)
> +		info->ptp->dmac_stop(ndev);
>  
>  	/* Stop AVB-DMAC process */
>  	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);

[Severity: Critical]
This dispatch is the only PTP teardown reachable from the
ravb_open() error path, and .dmac_stop is NULL in
ravb_gen3_ptp_info, so on Gen3 and Gen4 nothing is stopped here.

The registration for those two generations happens in the new
info->ptp->ndev_open() call in ravb_open(), which is ravb_ptp_init().
If ravb_phy_start() fails right after it, control goes to
out_ptp_stop, which calls ravb_stop_dma(), which reaches this hunk
and finds no .dmac_stop callback. ravb_ptp_stop() is therefore never
called, priv->ptp.clock stays populated, and the ptp_clock plus its
chardev stay registered while ravb_open() goes on to put the block
into CCC_OPC_RESET and runtime suspend the device. Before this
series the label itself did the pairing unconditionally for both
flags, so this path was covered.

The part that worries me most is what happens afterwards. Since
ndo_open failed the interface never came up, so ravb_close() is not
called on unbind either: unregister_netdev() has nothing to stop and
ravb_remove() proceeds to free_netdev(), freeing the ravb_private
area. The leaked ptp_clock still holds priv->ptp.info, which points
into that freed allocation, and it remains reachable from userspace
through /dev/ptpN, so the adjfine/gettime/settime callbacks then run
on freed memory (and would do ravb_read()/ravb_write() MMIO on a
device in RESET even before the free). A userspace loop that keeps
retrying open on a board where PHY connect fails also re-runs
ravb_ptp_init() each time and overwrites priv->ptp.clock, leaking a
registration per attempt.

Where do you want the fix? Calling info->ptp->ndev_close() from the
out_ptp_stop label looks like the smallest change and restores the
old pairing, but having ravb_stop_dma() fall back to ndev_close when
dmac_stop is absent would also work if you would rather keep the
teardown in one place. If it ends up at the label, the label name
finally matches what it does again.

[ ... ]

> @@ -1967,8 +1957,11 @@ static int ravb_open(struct net_device *ndev)
>  	ravb_emac_init(ndev);
>  
>  	/* Initialise PTP Clock driver */
> -	if (info->ccc_gac)
> -		ravb_ptp_init(ndev);
> +	if (info->ptp && info->ptp->ndev_open) {
> +		error = info->ptp->ndev_open(ndev);
> +		if (error)
> +			goto out_ptp_stop;
> +	}
>  
>  	/* PHY control start */
>  	error = ravb_phy_start(ndev);

[Severity: High]
Can this leak the registered ptp_clock on Gen3 and Gen4 when
ravb_phy_start() fails?

ndev_open is ravb_ptp_init() for ravb_gen3_ptp_info, so the clock is
registered here.  If ravb_phy_start() then fails, for example when
ravb_phy_init() cannot connect the PHY and returns -ENOENT, control reaches:

ravb_open() {
	...
out_ptp_stop:
	ravb_stop_dma(ndev);
out_set_reset:
	ravb_set_opmode(ndev, CCC_OPC_RESET);
out_rpm_put:
	pm_runtime_put_autosuspend(dev);
	...
}

but ravb_stop_dma() only dispatches dmac_stop:

	/* Stop PTP Clock driver */
	if (info->ptp && info->ptp->dmac_stop)
		info->ptp->dmac_stop(ndev);

and ravb_gen3_ptp_info leaves .dmac_stop NULL, so ravb_ptp_stop() never
runs.  priv->ptp.clock stays set and the ptp_clock plus its chardev stay
registered while the device is put into CCC_OPC_RESET and runtime
suspended.

Three follow-on effects:

- userspace can still call the ptp_clock_info callbacks, which do
  ravb_read()/ravb_write()/ravb_wait() MMIO on a device in RESET and
  possibly clock gated.

- each retried open re-runs ravb_ptp_init() and overwrites
  priv->ptp.clock, leaking the previous registration.

- ndo_stop never ran, so ravb_close() does not clean up either, and
  ravb_remove() does unregister_netdev() followed by free_netdev() with
  no ravb_ptp_stop() anywhere in ravb_main.c outside the two callback
  tables.  The leaked ptp_clock keeps a priv->ptp.info pointer into the
  freed netdev private area, reachable through /dev/ptpN.

Gen2 and RZ/V2M are not affected since their teardown is dmac_stop, which
ravb_stop_dma() does call.

The label name out_ptp_stop is also now misleading for the Gen3/Gen4
callback set, since nothing stops PTP there.  Would calling
info->ptp->ndev_close() on this path, or wiring ndev_close into the error
label, be the intent?

For reference, the baseline before this series had the pairing at the
label:

	if (info->gptp || info->ccc_gac)
		ravb_ptp_stop(ndev);

[ ... ]

> @@ -2678,12 +2691,24 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
>  			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
>  	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
>  	.dbat_entry_num = 22,
> +	.ptp = &ravb_gen2_ptp_info,
>  	.aligned_tx = 1,
> -	.gptp = 1,
>  	.nc_queues = 1,
>  	.magic_pkt = 1,
>  };
>  
> +static int ravb_gen3_ptp_set_config_mode(struct net_device *ndev)
> +{
> +	/* Enable gPTP Clock and Select High-speed peripheral bus clock. */
> +	return ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
> +}
> +
> +static const struct ravb_gptp_info ravb_gen3_ptp_info = {
> +	.set_config_mode = ravb_gen3_ptp_set_config_mode,
> +	.ndev_open = ravb_ptp_init,
> +	.ndev_close = ravb_ptp_stop,
> +};

This is the table with no .dmac_stop referenced in the ravb_open() comment
above, and it is shared by both ravb_gen3_hw_info and ravb_gen4_hw_info:

> @@ -2733,7 +2758,7 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
>  	.tx_counters = 1,
>  	.multi_irqs = 1,
>  	.irq_en_dis = 1,
> -	.ccc_gac = 1,
> +	.ptp = &ravb_gen3_ptp_info,
>  	.nc_queues = 1,
>  	.magic_pkt = 1,
>  };

[ ... ]

Re: [PATCH net-next v4 07/10] net: ethernet: ravb: Add callback for gPTP probe

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: ravb: Add callback for gPTP probe

This adds a probe callback to struct ravb_gptp_info so each RAVB generation
can probe its gPTP timer clock its own way, and moves the GTI computation
into those per-generation callbacks. The gptp_ref_clk hardware flag is
dropped and RZ/V2M gets its own ravb_rzv2m_ptp_info with a probe callback
that fetches the "gptp" clock.

Two questions below about the gPTP callback error paths, both reached
through ravb_open().

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 9e9b1c4fbe956..1092e8ada188b 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c

[ ... ]

> @@ -2704,6 +2705,7 @@ static int ravb_gen3_ptp_set_config_mode(struct net_device *ndev)
>  }
>  
>  static const struct ravb_gptp_info ravb_gen3_ptp_info = {
> +	.probe = ravb_gen2_ptp_probe,
>  	.set_config_mode = ravb_gen3_ptp_set_config_mode,
>  	.ndev_open = ravb_ptp_init,
>  	.ndev_close = ravb_ptp_stop,
>  };

[Severity: High]
Gen3 wires PTP registration to .ndev_open and teardown only to
.ndev_close, leaving .dmac_stop unset. In ravb_open() the clock is
registered before the PHY is started, and both failures share one label:

	/* Initialise PTP Clock driver */
	if (info->ptp && info->ptp->ndev_open) {
		error = info->ptp->ndev_open(ndev);
		if (error)
			goto out_ptp_stop;
	}

	/* PHY control start */
	error = ravb_phy_start(ndev);
	if (error)
		goto out_ptp_stop;
...
out_ptp_stop:
	ravb_stop_dma(ndev);

The only PTP teardown reachable from out_ptp_stop is in ravb_stop_dma():

	/* Stop PTP Clock driver */
	if (info->ptp && info->ptp->dmac_stop)
		info->ptp->dmac_stop(ndev);

Since ravb_gen3_ptp_info has no .dmac_stop, does a ravb_phy_start()
failure leave the ptp_clock registered by ravb_ptp_init() unregistered?

A failed ndo_open is not followed by ndo_stop, so ravb_ptp_stop() and its
ptp_clock_unregister() would not run at all. A later successful open calls
ravb_ptp_init() again and overwrites priv->ptp.clock, and on unbind
free_netdev() frees the ravb_private holding priv->ptp.info that the still
registered clock points at.

The baseline error label did this explicitly:

	if (info->gptp || info->ccc_gac)
		ravb_ptp_stop(ndev);

That call was removed earlier in this series by "net: ethernet: ravb:
Simplify gPTP start and stop", which states "There are no functional
change". Should that claim be revisited, or should out_ptp_stop also invoke
the ndev_close callback?

The same shape is still present at the end of the series (c7fa563):
ravb_gen3_ptp_info keeps .ndev_open = ravb_ptp_init with no .dmac_stop, and
out_ptp_stop still only calls ravb_stop_dma(). Gen4 is not affected at the
end of the series because ravb_gen4_ptp_info has no .ndev_open.

> @@ -2763,6 +2765,24 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
>  	.magic_pkt = 1,
>  };
>  
> +static int ravb_rzv2m_ptp_probe(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	priv->gptp_clk = devm_clk_get(&priv->pdev->dev, "gptp");
> +	if (IS_ERR(priv->gptp_clk))
> +		return PTR_ERR(priv->gptp_clk);
> +
> +	return ravb_compute_gti(ndev, priv->gptp_clk);
> +}
> +
> +static const struct ravb_gptp_info ravb_rzv2m_ptp_info = {
> +	.probe = ravb_rzv2m_ptp_probe,
> +	.set_config_mode = ravb_gen2_ptp_set_config_mode,
> +	.dmac_start = ravb_ptp_init,
> +	.dmac_stop = ravb_ptp_stop,
> +};

[Severity: Medium]
This isn't a bug introduced by this patch, but the .dmac_start =
ravb_ptp_init hook used here (and by ravb_gen2_ptp_info) can fail after the
DMA rings have been allocated. Can that leak the rings?

ravb_dmac_init() allocates first and calls the gPTP hook last:

	error = info->dmac_init(ndev);
	if (error)
		return error;
...
	/* Initialise PTP Clock driver */
	if (info->ptp && info->ptp->dmac_start)
		return info->ptp->dmac_start(ndev);

info->dmac_init() is ravb_dmac_init_rcar(), which allocates both queues via
ravb_ring_init() (RX/TX rings, page pools, SKB arrays and coherent
descriptor memory). Since "net: ethernet: ravb: Propagate error from
ptp_clock_register()" earlier in this series, ravb_ptp_init() returns the
ptp_clock_register() error, and ravb_dmac_init() passes it up to
ravb_open():

	/* Device init */
	error = ravb_dmac_init(ndev);
	if (error)
		goto out_set_reset;
...
out_ptp_stop:
	ravb_stop_dma(ndev);
out_set_reset:
	ravb_set_opmode(ndev, CCC_OPC_RESET);

Neither label calls ravb_ring_free(), and a failed ndo_open is not paired
with ndo_stop, so the rings allocated by ravb_dmac_init_rcar() stay
allocated. A later successful open overwrites priv->rx_ring[],
priv->tx_ring[] and priv->rx_pool[], and ravb_remove() does not free them
either.

The missing ravb_ring_free() on the ravb_open() error labels predates this
series: in the baseline ravb_dmac_init() could already fail at
ravb_set_opmode(CCC_OPC_OPERATION) after ring allocation, and a
ravb_phy_start() failure already returned through out_ptp_stop without
freeing the rings. Should the ravb_open() failure paths call
ravb_ring_free() for both queues?

Re: [PATCH net-next v4 09/10] dt-bindings: net: renesas,etheravb: Add optional gPTP phandle for Gen4

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
dt-bindings: net: renesas,etheravb: Add optional gPTP phandle for Gen4

This adds an optional ptp-timer phandle property to the EtherAVB binding
and restricts it to the renesas,etheravb-rcar-gen4 compatible, so that a
Gen4 RAVB node can reference the external system-wide gPTP clock shared
with RTSN on V4H and RSWITCH on S4.

> diff --git a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
> index 1e00ef5b3acda..fad002492421a 100644
> --- a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
> +++ b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
> @@ -97,6 +97,8 @@ properties:
>  
>    phy-handle: true
>  
> +  ptp-timer: true
> +

[Severity: High]
Is there an in-tree node that this phandle can point at on Gen4?

The property is re-enabled bare here, so it inherits the contract from
Documentation/devicetree/bindings/net/ethernet-controller.yaml:

  ptp-timer:
    $ref: /schemas/types.yaml#/definitions/phandle
    description:
      Specifies a reference to a node representing an IEEE 1588 PTP device.

On Gen4 there is no standalone PTP device node. On R-Car V4H the gPTP
block only exists as a register window of the RTSN controller node in
arch/arm64/boot/dts/renesas/r8a779g0.dtsi:

  tsn0: ethernet@e6460000 {
      ...
      reg-names = "tsnes", "gptp";

and on S4 it lives inside the RSWITCH node.

The consumer added by the closing patch of this series ("net: ethernet:
ravb: Add gPTP support for Gen4") resolves the phandle in
ravb_gen4_ptp_clock_index() with ptp_clock_index_by_of_node(), which
matches through ptp_clock_of_node_match() in drivers/ptp/ptp_clock.c:

  return (dev->parent && dev_of_node(dev->parent) == parent_np);

But the only Gen4 gPTP providers, rtsn.c and rswitch_main.c, both go
through rcar_gen4_ptp_register() in
drivers/net/ethernet/renesas/rcar_gen4_ptp.c:

  ptp_priv->clock = ptp_clock_register(&ptp_priv->info, NULL);

With a NULL parent, ptp->dev.parent is NULL and the of_node match cannot
succeed, so ravb_gen4_ptp_clock_index() returns -1.

Does that mean a Gen4 DTS writing ptp-timer = <&tsn0> validates and is
parsed but still ends up without PTP support? If so, would it make sense
to also pass the RTSN/RSWITCH device as the ptp_clock_register() parent,
and to add a local description plus an example here so DTS authors and
reviewers have a criterion for which node is a valid target?

>    '#address-cells':
>      description: Number of address cells for the MDIO bus.
>      const: 1

[ ... ]

> @@ -305,6 +307,15 @@ allOf:
>              items:
>                - const: fck
>                - const: refclk
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              const: renesas,etheravb-rcar-gen4
> +    then:
> +      properties:
> +        ptp-timer: false

[Severity: Low]
This isn't a bug, but one note on the ordering within the series, for the
record: at this patch alone the gated ptp-timer property has no reader,
and ravb_gen4_hw_info in drivers/net/ethernet/renesas/ravb_main.c still
sets

  .ptp = &ravb_gen3_ptp_info,

which describes the internal CCC_GAC gPTP with GTI derived from fck, i.e.
the opposite hardware model from the external gPTP this binding
describes.

The following patch, "net: ethernet: ravb: Add gPTP support for Gen4",
resolves that by adding ravb_gen4_ptp_probe() with

  priv->of_gptp = of_parse_phandle(priv->pdev->dev.of_node, "ptp-timer", 0);

and switching ravb_gen4_hw_info to

  .ptp = &ravb_gen4_ptp_info,

so the binding change here is inert documentation on its own and nothing
needs changing for this reason alone.

Re: [PATCH net-next v4 10/10] net: ethernet: ravb: Add gPTP support for Gen4

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: ravb: Add gPTP support for Gen4

This patch gives R-Car Gen4 its own ravb_gptp_info, resolving a system-wide
gPTP timer through a new optional ptp-timer phandle instead of the
non-existing internal gPTP clock, and selects that timer as the timestamp
source in APSR. The node reference is released in ravb_remove().

No Link: tags are present in the commit header.

A few questions below, both on the commit message and on the new code.

[Severity: Low]

The commit message says:

  "While driver advertises gPTP support on Gen4 platforms it is in fact
  completely broken."

That describes a user-visible defect in existing kernels: before this
series ravb_gen4_hw_info had .ccc_gac = 1, ravb_get_ts_info() advertised
so_timestamping and phc_index for those devices, and ravb_open() called
ravb_ptp_init(), so every board matching renesas,etheravb-rcar-gen4
exposes a /dev/ptpN for hardware the commit message says does not exist.

Should this carry a Fixes: tag pointing at 949f252a8594 ("net: ravb: Add
R-Car Gen4 support")?

Since the change depends on the ten-patch gPTP refactor in this series and
on a new DT property, it is presumably not backportable on its own - could
the commit message state that explicitly for the stable maintainers?

Also, small typo in the commit message: "optional.If not present" is
missing a space after the period.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 8617c30dbb14e..c8aa40136c3da 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2755,6 +2755,57 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
>  	.magic_pkt = 1,
>  };
>  
> +static int ravb_gen4_ptp_probe(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	priv->of_gptp = of_parse_phandle(priv->pdev->dev.of_node, "ptp-timer", 0);

[Severity: Medium]

Does this leak the of_gptp device_node reference on probe failure?

of_parse_phandle() returns the node with an incremented reference count,
and ravb_probe() calls this callback early:

drivers/net/ethernet/renesas/ravb_main.c:ravb_probe() {
	...
	if (info->ptp && info->ptp->probe) {
		error = info->ptp->probe(ndev);
		if (error)
			goto out_reset_assert;
	}

	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
	...
}

Every later failure (devm_clk_get_optional(), pm_runtime_resume_and_get(),
devm_platform_get_and_ioremap_resource(), of_get_phy_mode(),
dma_alloc_coherent(), ravb_set_opmode(), ravb_mdio_init(),
register_netdev()) ends up in out_reset_assert / out_free_netdev, which do:

	reset_control_assert(rstc);
	free_netdev(ndev);

free_netdev() destroys the priv that held the only pointer to the node, so
the reference is lost. devm_clk_get_optional() can return -EPROBE_DEFER, so
this can repeat on every retry. Would an extra unwind label, or
devm_add_action_or_reset(), keep the acquisition symmetric?

> +	if (!priv->of_gptp)
> +		return 0;
> +
> +	if (!of_device_is_available(priv->of_gptp)) {
> +		of_node_put(priv->of_gptp);
> +		priv->of_gptp = NULL;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]

This resolves the timer purely from the phandle plus of_device_is_available(),
with no dependency on the driver that actually owns and clocks the shared
gPTP counter (rswitch on S4, rtsn on V4H).

ravb_gen4_ptp_probe() cannot fail and prints nothing, so a dangling
phandle, an unbound or deferred provider, and a genuinely absent ptp-timer
all look identical from user space: ethtool -T simply reports no PHC. Would
a dev_warn() (or -EPROBE_DEFER / a device link) help here?

There is also no check that the shared counter is running before the
timestamp source is switched over in ravb_gen4_ptp_set_config_mode():

	/* Set PTP source to GPTP module, only option on Gen4. */
	if (priv->of_gptp)
		ravb_modify(ndev, APSR, APSR_GPTPTIMER_SOURCE | APSR_GPTPCLOCK,
			    APSR_GPTPTIMER_SOURCE | APSR_GPTPCLOCK);

The provider can stop that counter at any time while RAVB is up -
rtsn_remove() and rswitch_deinit() call rcar_gen4_ptp_unregister(), which
does:

drivers/net/ethernet/renesas/rcar_gen4_ptp.c:rcar_gen4_ptp_unregister() {
	iowrite32(1, ptp_priv->addr + PTPTMDC_REG);
	return ptp_clock_unregister(ptp_priv->clock);
}

Is the resulting cross-driver load-order and unbind dependency intentional?
It is not mentioned in the commit message.

> +
> +static int ravb_gen4_ptp_clock_index(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	/* If no clock, mimic ptp_clock_index_by_of_node() fail and return -1 */
> +	if (!priv->of_gptp)
> +		return -1;
> +
> +	return ptp_clock_index_by_of_node(priv->of_gptp);
> +}

[Severity: High]

Can this lookup ever succeed against an in-tree provider?

ptp_clock_index_by_of_node() matches on the parent device of the registered
clock:

drivers/ptp/ptp_clock.c:ptp_clock_of_node_match() {
	return (dev->parent && dev_of_node(dev->parent) == parent_np);
}

But the only in-tree registrar of the R-Car Gen4 system gPTP timer passes a
NULL parent:

drivers/net/ethernet/renesas/rcar_gen4_ptp.c:rcar_gen4_ptp_register() {
	...
	ptp_priv->clock = ptp_clock_register(&ptp_priv->info, NULL);
	...
}

and ptp_clock_register() stores that as ptp->dev.parent. With parent NULL
the predicate is false for every np, class_find_device() returns NULL, and
the helper returns -1 even for a valid and enabled ptp-timer phandle.

ravb_get_ts_info() only fills anything in when index >= 0:

drivers/net/ethernet/renesas/ravb_main.c:ravb_get_ts_info() {
	...
	/* Only advertise ptp clock if present. */
	if (index >= 0) {
	...
}

So does Gen4 end up never advertising gPTP at all, leaving the feature this
patch adds inert unless rswitch/rtsn are changed to pass the owning device
to ptp_clock_register()?

> +
> +static int ravb_gen4_ptp_set_config_mode(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	/* Enable gPTP Clock and Select High-speed peripheral bus clock. */
> +	ret = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
> +	if (ret)
> +		return ret;
> +
> +	/* Set PTP source to GPTP module, only option on Gen4. */
> +	if (priv->of_gptp)
> +		ravb_modify(ndev, APSR, APSR_GPTPTIMER_SOURCE | APSR_GPTPCLOCK,
> +			    APSR_GPTPTIMER_SOURCE | APSR_GPTPCLOCK);
> +
> +	return 0;
> +}
> +
> +static const struct ravb_gptp_info ravb_gen4_ptp_info = {
> +	.probe = ravb_gen4_ptp_probe,
> +	.clock_index = ravb_gen4_ptp_clock_index,
> +	.set_config_mode = ravb_gen4_ptp_set_config_mode,
> +};
> +

[Severity: Medium]

For Gen4 without an available ptp-timer (the newly optional case), do the
two halves of the timestamping uAPI now disagree?

ravb_get_ts_info() reports nothing, because clock_index() returns -1, so
so_timestamping, tx_types, rx_filters stay zero and phc_index keeps the
caller's -1.

ravb_hwtstamp_set() still accepts the request and returns success:

drivers/net/ethernet/renesas/ravb_main.c:ravb_hwtstamp_set() {
	...
	priv->tstamp_tx_ctrl = tstamp_tx_ctrl;
	priv->tstamp_rx_ctrl = tstamp_rx_ctrl;

	return 0;
}

net_hwtstamp_validate() only range-checks the enums, so HWTSTAMP_TX_ON and
HWTSTAMP_FILTER_ALL are accepted here.

The datapath then keys off info->ptp, which is always non-NULL for Gen4
now:

drivers/net/ethernet/renesas/ravb_main.c:ravb_start_xmit() {
	...
	/* TX timestamp required */
	if (info->ptp) {
		if (q == RAVB_NC) {
	...
}

drivers/net/ethernet/renesas/ravb_main.c:ravb_rx_rcar_hwstamp() {
	if (q == RAVB_NC)
		get_ts = priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE;
	...
}

Should these gates use the same "is a gPTP clock available" predicate as
ravb_get_ts_info(), so user space cannot enable timestamping the driver
declares unsupported and get TFA/descriptor values from a counter the
commit message says Gen4 does not have?

>  static const struct ravb_hw_info ravb_gen4_hw_info = {
>  	.receive = ravb_rx_rcar,
>  	.set_rate = ravb_set_rate_rcar,
> @@ -2777,7 +2828,7 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
>  	.tx_counters = 1,
>  	.multi_irqs = 1,
>  	.irq_en_dis = 1,
> -	.ptp = &ravb_gen3_ptp_info,
> +	.ptp = &ravb_gen4_ptp_info,
>  	.nc_queues = 1,
>  	.magic_pkt = 1,
>  };
> @@ -3191,6 +3242,7 @@ static void ravb_remove(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(dev);
>  	clk_unprepare(priv->refclk);
> +	of_node_put(priv->of_gptp);
>  	reset_control_assert(priv->rstc);
>  	free_netdev(ndev);
>  	platform_set_drvdata(pdev, NULL);

[Severity: Medium]

Is the of_gptp node also leaked on the early return at the top of
ravb_remove()?

drivers/net/ethernet/renesas/ravb_main.c:ravb_remove() {
	error = pm_runtime_resume_and_get(dev);
	if (error < 0)
		return;
	...
	of_node_put(priv->of_gptp);
	...
}

The new of_node_put() sits below that return, so an unbind where runtime
resume fails drops out without releasing the reference.

Two more observations about the gPTP stop paths introduced earlier in this
series, in commit "net: ethernet: ravb: Simplify gPTP start and stop",
which this patch builds on:

[Severity: High]

On Gen3, does a PHY start failure in ravb_open() leave the PTP clock
registered?

ravb_open() registers the clock and then, on ravb_phy_start() failure,
goes to out_ptp_stop:

drivers/net/ethernet/renesas/ravb_main.c:ravb_open() {
	...
	/* Initialise PTP Clock driver */
	if (info->ptp && info->ptp->ndev_open) {
		error = info->ptp->ndev_open(ndev);
		...
	}

	/* PHY control start */
	error = ravb_phy_start(ndev);
	if (error)
		goto out_ptp_stop;
	...
out_ptp_stop:
	ravb_stop_dma(ndev);
	...
}

ravb_stop_dma() only calls dmac_stop:

drivers/net/ethernet/renesas/ravb_main.c:ravb_stop_dma() {
	...
	/* Stop PTP Clock driver */
	if (info->ptp && info->ptp->dmac_stop)
		info->ptp->dmac_stop(ndev);
	...
}

but ravb_gen3_ptp_info puts ravb_ptp_stop() in .ndev_close, not
.dmac_stop:

static const struct ravb_gptp_info ravb_gen3_ptp_info = {
	.probe = ravb_gen2_ptp_probe,
	.clock_index = ravb_gen2_ptp_clock_index,
	.set_config_mode = ravb_gen3_ptp_set_config_mode,
	.ndev_open = ravb_ptp_init,
	.ndev_close = ravb_ptp_stop,
};

so ptp_clock_unregister() is never reached. Since ndo_open failed, ndo_stop
(and .ndev_close) will not run either. A later successful open calls
ravb_ptp_init() again and overwrites priv->ptp.clock, and ravb_remove()
eventually calls free_netdev() while the leaked clock still points at the
embedded priv->ptp.info - can that turn into a use-after-free on a
subsequent /dev/ptpN or /sys/class/ptp/ptpN access?

The earlier commit claims no functional change, but the code it removed
from out_ptp_stop covered the ccc_gac case too:

	if (info->gptp || info->ccc_gac)
		ravb_ptp_stop(ndev);

[Severity: Medium]

Related question for Gen2 and RZ/V2M, where ravb_ptp_stop() is reached only
via dmac_stop at the end of ravb_stop_dma(): the function has three earlier
error returns before that point.

drivers/net/ethernet/renesas/ravb_main.c:ravb_stop_dma() {
	error = ravb_wait(ndev, TCCR, info->tccr_mask, 0);
	if (error)
		return error;

	error = ravb_wait(ndev, CSR, CSR_TPO0 | CSR_TPO1 | CSR_TPO2 | CSR_TPO3,
			  0);
	if (error)
		return error;
	...
	error = ravb_wait(ndev, CSR, CSR_RPO, 0);
	if (error)
		return error;
	...
}

ravb_close() only logs the failure and keeps going:

	if (ravb_stop_dma(ndev) < 0)
		netdev_err(ndev,
			   "device will be stopped after h/w processes are done.\n");

Before the refactor, ravb_close() called ravb_ptp_stop() unconditionally
ahead of the fallible ravb_stop_dma(). Does the clock now stay registered
after the interface goes down when a wait times out, with the next open
overwriting priv->ptp.clock?