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

[PATCH net-next v5 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      | 253 ++++++++++++------
 drivers/net/ethernet/renesas/ravb_ptp.c       |  38 +--
 4 files changed, 231 insertions(+), 106 deletions(-)

-- 
2.55.0

[PATCH net-next v5 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
resuming the system.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
* Changes from v4
- Updated commit message to try and address LLM hallucination.
---
 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 v5 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>
---
* Changes since v4
- Update comment in code to try and address LLM hallucination.
---
 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..ac91a6813368 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 by checking there are no requests. */
 	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 v5 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 v4
- Fix gPTP stop in error path of ravb_open(), thanks LLM a real issue!

* Changes since v1
- Improve spelling in commit message.
---
 drivers/net/ethernet/renesas/ravb_main.c | 36 +++++++++++-------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index a89f6fe7866f..42d208e6b8ec 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 */
@@ -1975,7 +1981,7 @@ static int ravb_open(struct net_device *ndev)
 
 out_ptp_stop:
 	/* Stop PTP Clock driver */
-	if (info->gptp || info->ccc_gac)
+	if (info->ccc_gac)
 		ravb_ptp_stop(ndev);
 	ravb_stop_dma(ndev);
 out_set_reset:
@@ -2020,10 +2026,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 +2058,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 +2372,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 v5 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 42d208e6b8ec..1dfe905832a0 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 ac91a6813368..6817e94721b5 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 v5 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 1dfe905832a0..db3ef87f0991 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;
@@ -2937,7 +2937,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 6817e94721b5..407ae97b76bb 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 v5 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 | 87 +++++++++++++++---------
 2 files changed, 78 insertions(+), 33 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 db3ef87f0991..b558e9f5611e 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);
@@ -1981,8 +1974,8 @@ static int ravb_open(struct net_device *ndev)
 
 out_ptp_stop:
 	/* Stop PTP Clock driver */
-	if (info->ccc_gac)
-		ravb_ptp_stop(ndev);
+	if (info->ptp && info->ptp->ndev_close)
+		info->ptp->ndev_close(ndev);
 	ravb_stop_dma(ndev);
 out_set_reset:
 	ravb_set_opmode(ndev, CCC_OPC_RESET);
@@ -2190,7 +2183,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) {
@@ -2372,8 +2365,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)
@@ -2381,7 +2374,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);
@@ -2663,6 +2656,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,
@@ -2681,12 +2694,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,
@@ -2705,11 +2730,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,
 };
@@ -2736,7 +2761,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,
 };
@@ -2761,7 +2786,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 v5 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 b558e9f5611e..77e66a9870ab 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;
 
@@ -2656,6 +2649,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;
@@ -2671,6 +2671,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,
@@ -2707,6 +2708,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,
@@ -2766,6 +2768,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,
@@ -2786,8 +2806,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,
 };
@@ -2979,12 +2998,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");
@@ -3037,10 +3054,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 v5 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 77e66a9870ab..2dff47c2a125 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;
@@ -2656,6 +2661,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;
@@ -2672,6 +2687,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,
@@ -2709,6 +2725,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,
@@ -2781,6 +2798,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 v5 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 v5 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 2dff47c2a125..5a07cc0180d8 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2758,6 +2758,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,
@@ -2780,7 +2831,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,
 };
@@ -3194,6 +3245,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 v5 01/10] net: ethernet: ravb: Remove gPTP control from WoL setup and restore

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message claims both removed calls are a "redundant"
  special case because "the ndev will always be closed and re-opened…
- [Medium] Removing `ravb_ptp_stop()` from ravb_wol_setup() also removes
  the gPTP interrupt quiescing that the WoL suspend path relied on:…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is High
  severity and lives in the function the patch edits.

--

net: ethernet: ravb: Remove gPTP control from WoL setup and restore

The patch removes the ravb_ptp_stop() call from ravb_wol_setup() and the
ravb_ptp_init() call from ravb_wol_restore(), on the grounds that gPTP is
already started and stopped by ndo_open/ndo_stop for ccc_gac platforms.
Two questions about the suspend side of that reasoning, plus one
pre-existing issue in the function being edited.

No Link: tags present in the commit header.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ea1c7e536791e..36173a09e2fdb 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: Medium]
The commit message says:

    "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
    resuming the system."

Is that accurate for the suspend side?  ravb_suspend() takes the WoL
branch and returns before ravb_close() is ever reached:

drivers/net/ethernet/renesas/ravb_main.c:ravb_suspend() {
	rtnl_lock();
	if (priv->wol_enabled) {
		ret = ravb_wol_setup(ndev);
		rtnl_unlock();
		return ret;
	}

	ret = ravb_close(ndev);
	...
}

So the deleted "if (priv->info->ccc_gac) ravb_ptp_stop(ndev);" appears to
have been the only gPTP teardown performed before the system slept.  With
it gone, does the gPTP block stay armed and the PHC (/dev/ptpN) stay
registered for the whole WoL suspend window, with teardown happening only
later during resume, when ravb_wol_restore() reaches ravb_close() ->
ravb_ptp_stop()?

There is a second consequence in the resume error paths.
ravb_wol_restore() can return before ravb_close():

drivers/net/ethernet/renesas/ravb_main.c:ravb_wol_restore() {
	error = ravb_set_opmode(ndev, CCC_OPC_RESET);
	if (error)
		return error;

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

Since the clock is now still registered at that point, does a failed WoL
resume leave the PHC exposed to userspace while the gPTP block has just
been driven into reset with CCC_GAC/CSEL cleared?  Previously that state
could not occur because the clock had already been unregistered in
ravb_wol_setup().

ravb_resume() propagates the error and returns without calling
ravb_open(), so there is no gPTP rollback; the PM core logs the failed
resume callback and thaws userspace.  PHC ioctls such as
ravb_ptp_gettime64/settime64/adjtime can then run against the reset block
and each can burn the full ravb_wait() budget:

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;
}

that is up to 100 ms inside spin_lock_irqsave(&priv->lock).

Would it make sense either to keep the suspend-side stop, or to state in
the commit message that gPTP is deliberately left running across a WoL
suspend?

[Severity: Medium]
Removing ravb_ptp_stop() here also removes the gPTP interrupt quiescing
that this function relied on.  ravb_ptp_stop() was the only caller of:

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);
}

static void ravb_ptp_sync_irqs(struct net_device *ndev)
{
	synchronize_irq(ndev->irq);
	if (priv->info->err_mgmt_irqs) {
		synchronize_irq(priv->err_irq);
		synchronize_irq(priv->mgmt_irq);
	}
}

The rest of ravb_wol_setup() still quiesces every other source under the
"Only allow ECI interrupts" comment (RIC0/RIC2/TIC cleared,
synchronize_irq(priv->emac_irq), napi_disable(), ECSIPR = ECSIPR_MPDIP),
but only emac_irq is now synchronized.

If userspace armed EXTTS or PEROUT through the PHC before suspending,
ravb_ptp_extts()/ravb_ptp_perout() leave the gPTP source enabled:

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_perout() {
			/* Unmask interrupt */
			if (!info->irq_en_dis)
				ravb_modify(ndev, GIC, GIC_PTME, GIC_PTME);
			else
				ravb_write(ndev, GIE_PTMS0, GIE);
}

Does that source now stay enabled across the whole WoL suspend and early
resume interval, unflushed on the DMAC/err/mgmt lines?  A gPTP condition
latched while the IRQ core had the line suspended would be replayed in
the noirq resume phase, and further events can fire while
ravb_wol_restore() drives the block through ravb_set_opmode(ndev,
CCC_OPC_RESET) and ravb_set_config_mode() (register writes made without
priv->lock), before ravb_close() finally does the gPTP stop.

ravb_multi_interrupt() then dispatches into the PTP code in that window:

drivers/net/ethernet/renesas/ravb_main.c:ravb_multi_interrupt() {
	spin_lock(&priv->lock);
	iss = ravb_read(ndev, ISS);
	...
	/* gPTP interrupt status summary */
	if (iss & ISS_CGIS) {
		ravb_ptp_interrupt(ndev);
		result = IRQ_HANDLED;
	}
}

Can this deliver a PTP_CLOCK_EXTTS event with a timestamp read from a
gPTP block that has just been reset, or reprogram GPTC/GCCR on a block in
reset/config mode?

> @@ -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]);

[Severity: High]
This isn't a bug introduced by this patch, but since the patch touches
the code right above these napi_enable() calls it seems worth raising.

If either ravb_set_opmode(ndev, CCC_OPC_RESET) or ravb_set_config_mode()
fails earlier in ravb_wol_restore(), the function returns before the
napi_enable() pair above, so the napi_disable() calls done by
ravb_wol_setup() during suspend are never balanced:

drivers/net/ethernet/renesas/ravb_main.c:ravb_wol_setup() {
	if (info->nc_queues)
		napi_disable(&priv->napi[RAVB_NC]);
	napi_disable(&priv->napi[RAVB_BE]);
}

ravb_resume() propagates the error and never calls ravb_open():

drivers/net/ethernet/renesas/ravb_main.c:ravb_resume() {
	if (priv->wol_enabled)
		ret = ravb_wol_restore(ndev);
	else
		ret = pm_runtime_force_resume(dev);
	if (ret) {
		rtnl_unlock();
		return ret;
	}
}

The netdev stays registered and IFF_UP, so the next ndo_stop ("ip link
set <dev> down", or dev_close() from unregister_netdev() in
ravb_remove()) reaches ravb_close(), which calls napi_disable() a second
time on the already-disabled instances.  Does that spin forever in
napi_disable_locked()?

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

Both bits were left set by the first napi_disable() and only napi_enable()
clears them, so the caller would loop in process context while holding
RTNL and the netdev instance lock.

The concrete error source is the -ETIMEDOUT from ravb_wait() on the
CSR_OPS poll inside ravb_set_opmode().  Would moving the napi_enable()
pair before the opmode transitions, or adding error unwinding to the two
early returns, be preferable?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921175037.4084310-1-niklas.soderlund%2Brenesas%40ragnatech.se

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In ravb_ptp_init() (drivers/net/ethernet/renesas/ravb_ptp.c),
  the patch writes priv->gti_tiv to GTI, requests a timer-increment load…
- [Medium] The commit message presents the change as moving the GTI
  programming out of ravb_open() into ravb_ptp_init() (plus a locking…

Pre-existing issues:
- [Medium] ravb_ptp_init() discards the return value of ravb_wait(ndev,
  GCCR, GCCR_TCR, GCCR_TCR_NOREQ) and unconditionally continues: it…

--

net: ethernet: ravb: Move programming of gPTP timer interval

This removes ravb_set_gti() from ravb_main.c and moves the GTI write plus
the GCCR.LTI load request into ravb_ptp_init(), where it is now done under
priv->lock.  ravb_ptp_init() also stops seeding priv->ptp.default_addend
from the GTI register and uses the cached priv->gti_tiv instead.

A few questions below.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 36173a09e2fdb..a89f6fe7866fe 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);
> -}
> -

[ ... ]

> @@ -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);

[Severity: Medium]
The commit message describes this as moving the programming out of the RAVB
main logic into the gPTP init function, but ravb_open() is not the only
caller of ravb_ptp_init().  ravb_set_ringparam() and ravb_tx_timeout_work()
also call it, and neither of those ever called ravb_set_gti() before, nor do
they reset the device (ravb_set_ringparam() only stops DMA, and
ravb_tx_timeout_work() can reach its "out:" label even after ravb_stop_dma()
failed):

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

	netif_device_attach(ndev);
	...
}

On those paths GTI still holds whatever ravb_ptp_adjfine() last programmed:

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_adjfine() {
	addend = (u32)adjust_by_scaled_ppm(priv->ptp.default_addend, scaled_ppm);
	...
	ravb_write(ndev, addend & GTI_TIV, GTI);
}

So does an "ethtool -G" ring reconfiguration, or a TX watchdog timeout, now
silently reset the PHC frequency back to nominal and discard the frequency
adjustment userspace applied?

The new behaviour may well be the more correct one, since the old code
re-seeded default_addend from an already adjusted register value and later
adjfine() scaling then compounded off a drifted base.  If that is the intent,
could the commit message mention the behaviour change, and should this carry
a Fixes tag?

> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index 43218bc15b151..ac91a6813368d 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 by checking there are no requests. */
>  	ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ);
> +
>  	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
>  	spin_unlock_irqrestore(&priv->lock, flags);

[Severity: Medium]
Does this ravb_wait() actually observe the load request that was just
issued?  The request is GCCR.LTI, but the poll masks GCCR.TCR, and the two
are disjoint 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,

and ravb_wait() only tests the mask it is handed:

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;
}

TCR is already NOREQ here, so the poll returns on its first read.  The
driver's own convention elsewhere in this file treats LTI as the pending
increment load indicator:

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);
}

Given that, can the following read-modify-write read GCCR with LTI still
asserted and write that bit back, re-issuing an increment load that was not
intended?

	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);

And can TCSS end up switched to ADJGPTP before the increment value is known
to be latched, with a subsequent ravb_ptp_adjfine() then returning -EBUSY
because it does check GCCR_LTI?

Should the wait poll GCCR.LTI until the hardware clears it instead, and
should the new GTI write mask the value with GTI_TIV the way
ravb_ptp_adjfine() does?

Two related notes on the wording.  The new comment says "Wait for GIT
loading" - GIT should be GTI.  The commit message states:

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

With the poll looking at TCR rather than LTI, is that acknowledgement
actually happening?  There is also a typo, "acknowledge" should be
"acknowledged".

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but the return
value of ravb_wait() here is discarded, and ravb_ptp_init() returns void, so
GCCR.TCSS is programmed and ptp_clock_register() runs even when the poll
returned -ETIMEDOUT.  ravb_open() then calls
netif_tx_start_all_queues() and returns success with no indication that the
gPTP register sequence did not complete, and ravb_set_ringparam() and
ravb_tx_timeout_work() get no status either.

What makes this worth mentioning here is that the patch moves the GTI write
and the LTI load request into this same unchecked sequence, and newly exposes
that sequence to the ring-parameter and TX-timeout recovery paths.  Would it
make sense to check the ravb_wait() result while the code is being reworked?

For reference, the unchecked ravb_wait() is still unchecked at the end of the
series (02206b03b69a), even after the later patch "net: ethernet: ravb:
Propagate error from ptp_clock_register()" changes ravb_ptp_init() to return
int.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921175037.4084310-1-niklas.soderlund%2Brenesas%40ragnatech.se

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Error-path bypass: for `info->gptp` devices (Gen2
  `ravb_gen2_hw_info`, RZ/V2M `ravb_rzv2m_hw_info`) the gPTP clock is…
- [Medium] Asymmetric gPTP start/stop in `ravb_tx_timeout_work()`:
  `ravb_stop_dma()` now performs the gptp `ravb_ptp_stop()` at…
- [Medium] Loss of IRQ quiescence before the unlocked timestamp-list
  teardown in `ravb_close()` for `info->gptp` devices.

--

net: ethernet: ravb: Simplify gPTP start and stop

This folds the gptp-flag gPTP clock registration into ravb_dmac_init() and
the unregistration into ravb_stop_dma(), and drops the info->gptp checks
from the ravb_open()/ravb_close() call sites so only info->ccc_gac devices
are handled there. A few questions below about the error paths that are now
reached differently.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index a89f6fe7866fe..42d208e6b8ecd 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);
>  }

[Severity: High]
The registration side in ravb_dmac_init() is unconditional once the DMAC
started, but the new unregistration sits after the three fallible waits in
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;
	...
	/* Stop PTP Clock driver */
	if (info->gptp)
		ravb_ptp_stop(ndev);

Can the gPTP clock stay registered when any of those waits returns
-ETIMEDOUT?  Both callers tolerate that failure:  ravb_close() only logs
"device will be stopped after h/w processes are done." and then continues to
ravb_set_opmode(ndev, CCC_OPC_RESET) and pm_runtime_put_autosuspend(), and
the out_ptp_stop path of ravb_open() discards the ravb_stop_dma() return
value entirely.

If so, does /dev/ptpN survive on a block that has been put into
CCC_OPC_RESET with its clocks runtime suspended, so PHC ioctls do MMIO on a
gated block?

Also, does a subsequent open leak the first ptp_clock?  ravb_ptp_init()
ends with:

	clock = ptp_clock_register(&priv->ptp.info, &pdev->dev);
	...
	WRITE_ONCE(priv->ptp.clock, clock);

so the stale pointer is overwritten without unregistering, and
ravb_ptp_stop() only consumes the current pointer via xchg().

And since ravb_remove() has no ravb_ptp_stop() fallback:

	reset_control_assert(priv->rstc);
	free_netdev(ndev);

does a still-registered clock then keep a pointer to &priv->ptp.info inside
the freed ravb_private, reachable from userspace through the surviving PHC
device?

Would doing the gptp ravb_ptp_stop() before the fallible wait sequence, or
keeping the explicit teardown in the callers, preserve the previous
ordering?

The commit message says:

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

Given the error-path difference above, could that "no functional change"
claim be qualified?

This also appears to persist to the end of the series: at 02206b03 the call
is "if (info->ptp && info->ptp->dmac_stop) info->ptp->dmac_stop(ndev);"
still after the same early returns, Gen2/RZ-V2M provide only
.dmac_start/.dmac_stop with no .ndev_close, and ravb_remove() still ends in
free_netdev() with no PTP fallback.

[ ... ]

> @@ -1975,7 +1981,7 @@ static int ravb_open(struct net_device *ndev)
>  
>  out_ptp_stop:
>  	/* Stop PTP Clock driver */
> -	if (info->gptp || info->ccc_gac)
> +	if (info->ccc_gac)
>  		ravb_ptp_stop(ndev);
>  	ravb_stop_dma(ndev);
>  out_set_reset:

> @@ -2020,10 +2026,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 +2058,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]
In ravb_tx_timeout_work(), can the gptp clock end up stopped with nothing
restarting it?

ravb_stop_dma() now does the gptp ravb_ptp_stop() immediately before its
last fallible step:

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

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

and ravb_set_opmode() can return -ETIMEDOUT from
ravb_wait(ndev, CSR, CSR_OPS, csr_ops).  In that case
ravb_tx_timeout_work() takes:

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

which skips ravb_dmac_init(), the only remaining place that re-registers the
PHC for gptp devices, and the removed ravb_ptp_init() at out: no longer
compensates.  Does the interface then resume TX/RX with priv->ptp.clock ==
NULL and phc_index == -1, while ravb_start_xmit() still requests TX
timestamps under "if (info->gptp || info->ccc_gac)"?

The commit message states:

    are always just after a call to into ravb_dmac_init()

Is that true for the out: label, which is also reached from the
ravb_stop_dma() failure branch where ravb_dmac_init() never runs?

At 02206b03 the ordering in ravb_stop_dma() and the contents of the out:
label look unchanged, so this appears to carry through the series.

> @@ -2374,7 +2372,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 */

[Severity: Medium]
For info->gptp devices this also removes the unconditional IRQ drain that
used to run before the timestamp list is cleared.  ravb_ptp_stop() ends
with:

	ravb_ptp_disable(ndev);
	ravb_ptp_sync_irqs(ndev);

where ravb_ptp_sync_irqs() does synchronize_irq(ndev->irq) plus the err and
mgmt IRQs when info->err_mgmt_irqs (RZ/V2M).  After this patch that drain
happens only inside ravb_stop_dma(), i.e. after its early -ETIMEDOUT
returns, while ravb_close() continues regardless:

	/* 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");

	/* Clear the timestamp list */
	if (info->gptp || info->ccc_gac) {
		list_for_each_entry_safe(ts_skb, ts_skb2, &priv->ts_skb_list, list) {
			list_del(&ts_skb->list);
			kfree_skb(ts_skb->skb);
			kfree(ts_skb);
		}
	}

That loop runs without priv->lock, and ravb_get_tx_tstamp(), called from
hardirq context via ravb_timestamp_interrupt() in ravb_interrupt() and
ravb_multi_interrupt(), walks and frees entries of the same list:

	list_for_each_entry_safe(ts_skb, ts_skb2, &priv->ts_skb_list, list) {
		...
		list_del(&ts_skb->list);
		kfree(ts_skb);

Can a handler still in flight (or entered because the error summary source
is not masked by close) double free a ravb_tstamp_skb here?  The window for
a newly arriving interrupt existed before this patch; what changes is that
the in-flight drain no longer runs unconditionally for gptp devices.  Would
taking priv->lock around the ts_skb_list teardown be the more robust fix?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921175037.4084310-1-niklas.soderlund%2Brenesas%40ragnatech.se

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 0 · High: 3 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Replacing the cached PHC index with a live dereference re-
  creates a check-then-use of priv->ptp.clock against clock destruction.
- [High] ravb_ptp_stop() no longer detaches priv->ptp.clock before
  disabling the gPTP interrupt and synchronizing IRQs.
- [High] For gptp (Gen2/RZ-V2M) hardware the only gPTP teardown call now
  sits at the end of ravb_stop_dma() (`/* Stop PTP Clock driver */ if…
- [Medium] ravb_get_ts_info() now gates the *static* hardware-
  timestamping capability report on the runtime presence of a registered…
- [Medium] ravb_ptp_init() now publishes the raw ptp_clock_register()
  result — possibly an ERR_PTR — into the shared field priv->ptp.clock…
- [Low] Concern: ravb_ptp_init() is converted to return an errno (and
  drops the previous netdev_err() diagnostic), but both callers in this…

--

net: ethernet: ravb: Propagate error from ptp_clock_register()

This changes ravb_ptp_init() from void to int so that a failing
ptp_clock_register() can be reported to callers, stores the clock pointer
directly in priv->ptp.clock, and drops the cached phc_index in favour of
calling ptp_clock_index() from ravb_get_ts_info().

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

I have a few questions about the interaction with the two recent PTP
fixes in this driver.

> 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 1dfe905832a0e..db3ef87f09912 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]
Is it intentional that the runtime clock check now gates the whole static
capability block in ravb_get_ts_info(), and not just the phc_index lookup?

so_timestamping, tx_types and rx_filters are all inside this if (), so
ethtool -T reports zeroes whenever no PHC happens to be registered:

  - before the first open and after ravb_close()
  - for gptp (Gen2) parts the clock only exists between ravb_dmac_init()
    and ravb_stop_dma(), so also during the stop/re-init window used by
    ravb_set_ringparam() and ravb_tx_timeout_work()
  - permanently on CONFIG_PTP_1588_CLOCK=n builds, since ravb only
    depends on PTP_1588_CLOCK_OPTIONAL and the stub
    ptp_clock_register() returns NULL

The ethtool core only ORs in the software bits it owns:

net/ethtool/common.c:__ethtool_get_ts_info() {
	...
	info->so_timestamping |= SOF_TIMESTAMPING_RX_SOFTWARE |
				 SOF_TIMESTAMPING_SOFTWARE;
	...
}

so SOF_TIMESTAMPING_TX_SOFTWARE disappears from the report on a down
interface, even though ravb_start_xmit() calls skb_tx_timestamp()
unconditionally.

The peer ABI also disagrees with the new report: ravb_hwtstamp_set()
still accepts HWTSTAMP_TX_ON and coerces unknown filters to
HWTSTAMP_FILTER_ALL without looking at priv->ptp.clock or hw_info.

This looks like a partial revert of commit 1f77af0aaf27 ("net: ravb:
avoid dereferencing an invalid PTP clock"), whose changelog says the
cached index was added to preserve the static timestamping capabilities.
Could the commit message mention this user-visible change, since it
currently only describes propagating the registration error?

Note this behaviour also remains at the end of the series, where
ravb_gen2_ptp_clock_index() returns -ENODEV with no clock and
ravb_get_ts_info() fills the block only if (index >= 0).

>  		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 ptp_clock?

priv->ptp.clock is loaded twice here with plain loads, once for the
non-NULL test and once as the argument of the out-of-module call
ptp_clock_index(), which is just:

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

Nothing keeps the object alive between the test and the dereference, and
with this patch ravb_ptp_stop() leaves the pointer published for the
whole duration of the freeing unregister:

	if (priv->ptp.clock) {
		ptp_clock_unregister(priv->ptp.clock);
		priv->ptp.clock = NULL;
	}

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

ravb_get_ts_info() can run without RTNL. An unprivileged task can bind
an AF_INET6 socket to a link-local address with sin6_scope_id
(__inet6_bind() sets sk->sk_bound_dev_if with no capability check) and
then loop:

setsockopt(SO_TIMESTAMPING, SOF_TIMESTAMPING_BIND_PHC)
  sock_timestamping_bind_phc()
    ethtool_get_phc_vclocks()
      __ethtool_get_ts_info()
        ravb_get_ts_info()

holding only a netdevice reference. The RCU section in
__ethtool_get_ts_info() protects dev->hwprov, and a ptp_clock is not
RCU-freed. Meanwhile ravb_ptp_stop() runs from ravb_close() (ip link set
down, PM suspend) or from ravb_stop_dma() on tx-timeout recovery.

Commit 1f77af0aaf27 ("net: ravb: avoid dereferencing an invalid PTP
clock") removed exactly this by caching phc_index and reading it with
READ_ONCE(). Would keeping the cached index, or at least loading the
pointer once and detaching it before unregistering, be preferable here?

The same check-then-dereference is still present at the end of the
series in ravb_gen2_ptp_clock_index():

	if (!priv->ptp.clock)
		return -ENODEV;

	return ptp_clock_index(priv->ptp.clock);

>  	}
>  
>  	return 0;

[ ... ]

> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index 6817e94721b57..407ae97b76bbb 100644
> --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
> @@ -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 publishing the raw ptp_clock_register() result into priv->ptp.clock
safe, given that an ERR_PTR is visible until the following assignment
normalizes it to NULL?

The previous code normalized first and published afterwards:

	clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
	if (IS_ERR(clock)) {
		...
		clock = NULL;
	}
	WRITE_ONCE(priv->ptp.clock, clock);

Commit 1f77af0aaf27 describes that ordering as deliberate: "Registration
failures can also leave an error pointer in priv->ptp.clock ... Normalize
registration errors to NULL".

All consumers only test for NULL, so an ERR_PTR passes:

ravb_ptp_interrupt():
	struct ptp_clock *clock = READ_ONCE(priv->ptp.clock);
	...
	if ((gis & GIS_PTCF) && clock)
		ptp_clock_event(clock, &event);

ravb_get_ts_info():
	if (priv->ptp.clock && (hw_info->gptp || hw_info->ccc_gac)) {
		...
		info->phc_index = ptp_clock_index(priv->ptp.clock);

A reader landing in that window would dereference something like
0xfffffffffffffff4. The window is narrow and requires
ptp_clock_register() to fail, but would assigning to a local and
publishing the normalized value keep the property that commit installed?

As a side note, the WRITE_ONCE() on the store side is gone while
ravb_ptp_interrupt() still uses READ_ONCE(), so the annotations are no
longer paired.

[Severity: Low]
The netdev_err() message is dropped here, and in this commit both
callers still discard the new return value: ravb_dmac_init() calls
ravb_ptp_init(ndev) and then returns 0, and ravb_open() calls it and
continues to ravb_phy_start()/netif_tx_start_all_queues().

The commit message explains this is preparatory, and the later patch
"net: ethernet: ravb: Replace gPTP flags with callbacks" does consume the
error via info->ptp->dmac_start() and info->ptp->ndev_open(), so the only
lasting change appears to be the lost diagnostic. Would it be worth
keeping a message on the failure path, or mentioning its removal?

>  
> -	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]
Dropping the xchg() means the pointer stays published across
ptp_clock_unregister(). Can ravb_ptp_interrupt() still pick it up and
use a freed clock?

Commit 1cb9663789c5 ("net: ravb: serialize PTP clock teardown") added
that detach for this reason: "ravb_ptp_interrupt() can race with
ravb_ptp_stop() and pass the clock to ptp_clock_event() while
ptp_clock_unregister() is freeing it."

The remaining protection is the hardware state (GIC = 0 plus
gis &= ravb_read(ndev, GIC) in the handler), but the PTP chardev stays
live until posix_clock_unregister() completes inside
ptp_clock_unregister(), so a concurrent ioctl can re-arm the interrupt
after ravb_ptp_disable() ran:

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);
}

ravb_ptp_perout() does the same for GIC_PTME. Note also that
ravb_ptp_disable() writes GIC without priv->lock, so the locked
read-modify-write in ravb_ptp_extts() can simply undo it.

The generic cleanup does not cover ravb either, since
ptp_disable_all_events() only touches pin-configured EXTTS sources and
PPS:

drivers/ptp/ptp_chardev.c:ptp_disable_all_events() {
	for (i = 0; i < info->n_pins; i++)
		if (info->pin_config[i].func == PTP_PF_EXTTS)
			ptp_disable_pinfunc(...);
	...
}

and ravb registers no pins and no PPS.

The resulting sequence would be:

CPU0 ravb_close() -> ravb_ptp_stop()
	ravb_ptp_disable();		/* GIC = 0 */
	ravb_ptp_sync_irqs();
	ptp_clock_unregister(priv->ptp.clock);	/* -> kfree(ptp) */

CPU1 PTP_EXTTS_REQUEST on a still-open /dev/ptpN
	ravb_ptp_extts() -> GIC |= GIC_PTCE

CPU2 ravb_ptp_interrupt()
	clock = READ_ONCE(priv->ptp.clock);	/* still published */
	ptp_clock_event(clock, &event);		/* writes ptp->tsevqs */

Would restoring the detach before disabling, for example
clock = xchg(&priv->ptp.clock, NULL) ahead of ravb_ptp_disable(), keep
the guarantee that commit added? The clearing store is also a plain
assignment now while the hardirq reader still uses READ_ONCE().

[Severity: High]
While looking at the teardown side, a related question about the gptp
path: this is not introduced by this commit but by the earlier series
patch "net: ethernet: ravb: Simplify gPTP start and stop", whose
changelog says "There are no functional change".

For gptp (Gen2/RZ-V2M) parts the only ravb_ptp_stop() call is now at the
end of ravb_stop_dma():

	error = ravb_wait(ndev, TCCR, info->tccr_mask, 0);
	if (error)
		return error;
	error = ravb_wait(ndev, CSR, CSR_TPO0 | ... , 0);
	if (error)
		return error;
	...
	error = ravb_wait(ndev, CSR, CSR_RPO, 0);
	if (error)
		return error;
	...
	/* Stop PTP Clock driver */
	if (info->gptp)
		ravb_ptp_stop(ndev);

and ravb_close() only unregisters directly for ccc_gac, then continues
after a ravb_stop_dma() failure:

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

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

If the DMAC does not quiesce and ravb_wait() times out, does a Gen2
interface end up closed with its PHC still registered? A following
ravb_open() -> ravb_dmac_init() -> ravb_ptp_init() then registers
&priv->ptp.info a second time and overwrites priv->ptp.clock, and unbind
later reaches free_netdev() with a registered clock still pointing at the
ptp_clock_info embedded in ravb_private.

Before this series ravb_close(), ravb_tx_timeout_work() and
ravb_set_ringparam() called ravb_ptp_stop() unconditionally before
ravb_stop_dma(), so the error path could not skip it. The ordering is
unchanged at the end of the series, where
if (info->ptp && info->ptp->dmac_stop) info->ptp->dmac_stop(ndev); sits
after the same early returns.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921175037.4084310-1-niklas.soderlund%2Brenesas%40ragnatech.se

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The gPTP clock teardown in this series no longer atomically
  detaches priv->ptp.clock before disabling gPTP interrupts and…
- [Medium] The newly propagated gPTP start error creates a failure return
  in ravb_dmac_init() that happens *after* info->dmac_init() allocated…
- [Medium] Both newly created PTP failure paths leak the RX/TX rings.
- [Low] Undisclosed behavioural change: the commit message presents the
  patch as a mechanical replacement of info->gptp/info->ccc_gac by…

Pre-existing issues:
- [Low] ravb_ptp_init() requests a gPTP timer-increment load by setting
  GCCR.LTI, but then polls the unrelated GCCR.TCR field (ravb_wait(ndev,…

--

net: ethernet: ravb: Replace gPTP flags with callbacks

The two hardware-info flags describing gPTP behaviour, info->gptp (Gen2)
and info->ccc_gac (Gen3), are replaced by a new struct ravb_gptp_info with
set_config_mode/dmac_start/dmac_stop/ndev_open/ndev_close callbacks.  Each
generation's hw_info now points at one of ravb_gen2_ptp_info or
ravb_gen3_ptp_info.

A few questions below about the new error propagation and about the gPTP
teardown this series ends up with.

>     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 description reads as a mechanical flag-to-callback conversion, but the
patch also changes behaviour: the ravb_ptp_init() return value, which was
deliberately discarded at both call sites before, is now propagated.

In ravb_dmac_init() the call becomes "return info->ptp->dmac_start(ndev);"
and in ravb_open() the ndev_open callback error does "goto out_ptp_stop".
Since ravb_ptp_init() returns PTR_ERR(ptp_clock_register()), a PHC
registration failure now makes "ip link set up" fail, makes "ethtool -G"
fail and aborts TX timeout recovery, where previously the interface came up
without a PHC.

Could the commit message mention that open and DMAC init now abort on a
gPTP start error?  The only mention today is the "Handle error from starting
the PTP clock" line in the post-"---" v1 to v2 changelog, which does not end
up in the git history.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index db3ef87f09912..b558e9f5611e1 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]
For ravb_gen2_ptp_info (used by ravb_gen2_hw_info and ravb_rzv2m_hw_info)
dmac_start is ravb_ptp_init(), which returns PTR_ERR(ptp_clock_register()).
Are the three callers of ravb_dmac_init() prepared for a failure that
happens this late?

At this point info->dmac_init() has already allocated and formatted the
BE/NC rings, handed RX descriptors to hardware, and CCC.OPC == OPERATION has
been confirmed via CSR.OPS:

	/* 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);

ravb_open() routes this error to out_set_reset, which skips ravb_stop_dma()
and writes CCC_OPC_RESET straight from OPERATION, a transition the driver
does not otherwise use (ravb_close() and the sibling out_ptp_stop path always
reach RESET from CONFIG via ravb_stop_dma()).  The ravb_wait() result of that
write is also dropped before pm_runtime_put_autosuspend() may suspend the
device with the DMAC still running.

ravb_tx_timeout_work() takes out_unlock, whose comment states an invariant
that no longer holds:

	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().
		 */
		goto out_unlock;
	}

With a gPTP start failure the descriptors are not freed and the DMAC is
running, while ravb_emac_init() and netif_tx_start_all_queues() are skipped
after ravb_rcv_snd_disable() - doesn't that leave the interface dead until an
administrative down/up?

ravb_set_ringparam() has the same shape:

		error = ravb_dmac_init(ndev);
		if (error) {
			netdev_err(ndev, "%s: ravb_dmac_init() failed, error %d\n", __func__, error);
			return error;
		}

which returns after netif_device_detach() and before
ravb_emac_init()/netif_device_attach(), leaving the netdev detached with the
freshly programmed DMAC operating.

Only the Gen3/Gen4 ndev_open failure path calls ravb_stop_dma(), so the three
call sites now disagree about what a ravb_dmac_init() error means.

[ ... ]

> @@ -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);
> @@ -1981,8 +1974,8 @@ static int ravb_open(struct net_device *ndev)
>  
>  out_ptp_stop:
>  	/* Stop PTP Clock driver */
> -	if (info->ccc_gac)
> -		ravb_ptp_stop(ndev);
> +	if (info->ptp && info->ptp->ndev_close)
> +		info->ptp->ndev_close(ndev);
>  	ravb_stop_dma(ndev);
>  out_set_reset:
>  	ravb_set_opmode(ndev, CCC_OPC_RESET);

[Severity: Medium]
Do the two new gPTP failure paths leak the RX/TX rings?

Neither out_ptp_stop (taken for the Gen3/Gen4 ndev_open error) nor
out_set_reset (taken for the Gen2/RZ-V2M ravb_dmac_init error) calls
ravb_ring_free(), yet ravb_dmac_init_rcar() has already run both:

	error = ravb_ring_init(ndev, RAVB_BE);
	if (error)
		return error;
	error = ravb_ring_init(ndev, RAVB_NC);

and ravb_ring_init() allocates per queue:

	priv->rx_pool[q] = page_pool_create(&params);
	...
	priv->rx_buffers[q] = kzalloc_objs(*priv->rx_buffers[q], priv->num_rx_ring[q]);
	...
	priv->tx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size, &priv->tx_desc_dma[q], GFP_KERNEL);

Since ndo_open returned an error, ravb_close() never runs, and ravb_remove()
does not call ravb_ring_free() either.  Wouldn't the next successful
ravb_open() overwrite priv->rx_pool[], priv->rx_buffers[], priv->tx_skb[],
priv->rx_ring[], priv->tx_ring[] and priv->tx_align[], making the previous
page pool with its mapped pages, the kzalloc'd arrays and the
dma_alloc_coherent() rings unreachable?

The same unwind gap already existed for a ravb_phy_start() failure, but this
patch adds a second, independent trigger for it.

> @@ -2663,6 +2656,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,

[Severity: Low]
This isn't a bug introduced by this patch, and ravb_ptp.c isn't touched here,
but while looking at the new dmac_start callback: ravb_ptp_init() requests a
gPTP increment load with GCCR.LTI and then polls a different field:

	/* Request GTI loading. */
	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);

	/* Wait for GIT loading to complete by checking there are no requests. */
	ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ);

GCCR.LTI is the self-clearing load-request bit, as ravb_ptp_adjfine() shows:

	gccr = ravb_read(ndev, GCCR);
	if (gccr & GCCR_LTI) {
		spin_unlock_irqrestore(&priv->lock, flags);
		return -EBUSY;
	}

GCCR.TCR describes timer reset/capture requests and is normally already
GCCR_TCR_NOREQ, so the wait returns immediately, and its return value is
discarded.  Should this poll on GCCR_LTI instead?

> +	.dmac_stop = ravb_ptp_stop,
> +};

[Severity: High]
This isn't a change made by this patch either - it only wires ravb_ptp_stop()
up as a callback - but the preceding patch in this series, "net: ethernet:
ravb: Propagate error from ptp_clock_register()", removed the atomic detach
that commit 1cb9663789c5 ("net: ravb: serialize PTP clock teardown", Cc:
stable) added, and it is still gone at the end of the series.  Can the
use-after-free come back?

ravb_ptp_stop() now keeps the pointer published while the clock is torn down:

	ravb_ptp_disable(ndev);
	ravb_ptp_sync_irqs(ndev);

	if (priv->ptp.clock) {
		ptp_clock_unregister(priv->ptp.clock);
		priv->ptp.clock = NULL;
	}

whereas 1cb9663789c5 did "clock = xchg(&priv->ptp.clock, NULL);" before
ravb_ptp_disable()/ravb_ptp_sync_irqs() so later handlers were guaranteed to
observe NULL.

The IRQ side still dereferences the snapshot:

	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);

Is the GIC = 0 write enough on its own?  A PTP_EXTTS_REQUEST or
PTP_PEROUT_REQUEST ioctl is still serviceable until posix_clock_unregister()
takes clk->rwsem, and ravb_ptp_extts()/ravb_ptp_perout() re-set
GIC_PTCE/GIC_PTME:

CPU0 ravb_close() or the ravb_open() error path
  ravb_ptp_stop()
    ravb_ptp_disable()      /* GIC = 0 */
    ravb_ptp_sync_irqs()
    ptp_clock_unregister(priv->ptp.clock)   /* pointer still published */

CPU1 ioctl on /dev/ptpN
  ravb_ptp_enable()
    ravb_modify(ndev, GIC, GIC_PTCE, GIC_PTCE)
  gPTP capture IRQ -> ravb_ptp_interrupt() reads the non-NULL pointer
    ptp_clock_event(clock, &event)

The same commit also dropped priv->ptp.phc_index, which 1f77af0aaf27 added,
and ravb_get_ts_info() dereferences priv->ptp.clock directly again.  Was that
revert intentional?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921175037.4084310-1-niklas.soderlund%2Brenesas%40ragnatech.se

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new Gen4 PHC resolution can never succeed, so the feature
  the patch advertises ("Add gPTP support for Gen4") is inert.
- [Medium] On Gen4 the new ravb_gen4_ptp_info has no
  .dmac_start/.ndev_open, so ravb_ptp_init() is never run and no PHC…
- [Medium] ravb_gen4_ptp_probe()
  (drivers/net/ethernet/renesas/ravb_main.c:2765) acquires a device_node…
- [Low] The commit message asserts a pre-existing user-visible defect
  ("While driver advertises gPTP support on Gen4 platforms it is in fact…

--

net: ethernet: ravb: Add gPTP support for Gen4

This adds a Gen4 specific struct ravb_gptp_info that resolves a
system-wide gPTP timer through an optional ptp-timer phandle instead of
the non-existing internal gPTP clock, and programs the APSR gPTP source
bits when such a timer is described. Gen4 no longer advertises gPTP
support when the phandle is missing or its node is disabled.

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

[Severity: Low]
The changelog describes fixing an existing user-visible defect and
changes Gen4 behaviour accordingly, but there is no Fixes: tag naming
the commit that introduced the bogus Gen4 gPTP advertisement, and no
note about backportability.

The change depends on the new optional ptp-timer binding added earlier in
the series and on the preceding gPTP refactoring, so a plain Fixes: tag
may not be what is wanted here. Would it help to state explicitly that
this is net-next only and not suitable for stable, or add the tag if a
backport is intended?

> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 27e2b31955ba9..5d35b40d7c584 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h

[ ... ]

> @@ -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 2dff47c2a125a..5a07cc0180d8b 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2758,6 +2758,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;

[Severity: Medium]
Does this leak the ptp-timer device_node reference when probe fails
later on?

of_parse_phandle() returns the node with an incremented refcount, and the
only matching of_node_put() added here is in ravb_remove(). ravb_probe()
calls the new callback early:

	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");
	...

Everything after that point (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())
unwinds through out_napi_del/out_mdio_release/out_reset_mode/out_rpm_put/
out_rpm_disable/out_reset_assert/out_free_netdev, and none of those labels
calls of_node_put(priv->of_gptp) before free_netdev(ndev) frees priv.
Deferred probe retries would repeat this.

The early return in ravb_remove() looks like it has the same effect:

	error = pm_runtime_resume_and_get(dev);
	if (error < 0)
		return;
	...
	of_node_put(priv->of_gptp);

Would it be worth releasing the node in the probe unwind path as well, or
using a devm-managed lifetime for it?

> +
> +	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);

[Severity: High]
Can this lookup ever succeed with the current in-tree gPTP providers?

ptp_clock_index_by_of_node() only matches PTP class devices whose parent
device carries the requested of_node:

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

The only in-tree registration path for the R-Car Gen4 system-wide gPTP
timer registers the clock without a parent:

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

and that is the path used by both rtsn and rswitch, which own the gPTP
block the new ptp-timer phandle points at (on V4H it is the tsn0 node with
reg-names = "tsnes", "gptp").

If dev->parent is always NULL there, ravb_gen4_ptp_clock_index() returns -1
even for a present and enabled ptp-timer, and ravb_get_ts_info() then skips
the capability block:

	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) {

so ethtool -T on a correctly described Gen4 board would still report no
hardware timestamping and no phc_index. Is a companion change passing
&pdev->dev to ptp_clock_register() in rcar_gen4_ptp_register() (or some
other way to resolve the index) missing from the series?

> +}
> +
> +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]
With no .dmac_start/.ndev_open here, ravb_ptp_init() never runs on Gen4,
so there is no PHC and the gPTP counter is left unprogrammed. But
info->ptp stays non-NULL, so the timestamp machinery is still armed.
Should the request and consume paths be gated too?

Timestamp FIFO interrupts are enabled unconditionally:

ravb_dmac_init_rcar() {
	...
	/* Frame transmitted, timestamp FIFO updated */
	ravb_write(ndev, TIC_FTE0 | TIC_FTE1 | TIC_TFUE, TIC);
	...
}

and per-frame capture is armed based only on info->ptp in
ravb_start_xmit():

		desc->tagh_tsr = (ts_skb->tag >> 4) | TX_TSR;

ravb_get_tx_tstamp() then reads TFA0/TFA1/TFA2 and reports the values via
skb_tstamp_tx(), and ravb_rx_rcar_hwstamp() copies descriptor
ts_n/ts_sl/ts_sh into skb_hwtstamps().

There also looks to be a mismatch between what is advertised and what is
accepted: ravb_get_ts_info() reports no hardware timestamping when
clock_index() returns -1, while ravb_hwtstamp_set() still accepts
HWTSTAMP_TX_ON and upgrades unknown filters to HWTSTAMP_FILTER_ALL with no
check that a gPTP timer exists.

In the case where ptp-timer is absent or its node is disabled,
ravb_gen4_ptp_set_config_mode() still asserts CCC_GAC | CCC_CSEL_HPB but
skips the APSR source programming. What do TFA reads and the descriptor
timestamps contain in that configuration, and is it intended that they
still reach user space?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921175037.4084310-1-niklas.soderlund%2Brenesas%40ragnatech.se