[net-next,v2 0/7] net: renesas: Cleanup usage of gPTP flags

[net-next,v2 0/7] net: renesas: Cleanup usage of gPTP flags

[Niklas Söderlund]

Hello,

This series aim is to prepare for future work that will enable the use
of gPTP on R-Car RAVB on Gen4. Currently RAVB have a dedicated gPTP
implementation supported on Gen2 and Gen3 (ravb_ptp.c). For Gen4 a new
implementation that is already upstream (rcar_gen4_ptp.c) and used by
other Gen4 devices such as RTSN and RSWITCH is needed.

Unfortunately the design of the Gen2/Gen3 RAVB driver where driver
specific flags to control gPTP behavior have been mimicked in RTSN and
RSWITCH. This was OK as there was no overlap between the two gPTP
implementations. Now that RAVB needs to be able to use both having to
translate between driver specific flags and common net code flags
becomes even more cumbersome as there are two sets of driver specific
flags to pick from.

This series cleans this up for all Renesas drivers using gPTP by
removing all driver specific flags and using the common flags directly.
This simplifies drivers while at the same time prepare RAVB to be
extended with Gen4 support.

Patch 1/7 is a drive by patch where RSWITCH specific define was added in
the wrong header. Patch 2/7 removes a short-cut used in RTSN and RSWITCH
that prevents extending Gen4 support to RAVB without fuss. While patch
3/7 to 7/7 rework the Renesas drivers to use the common flags instead of
driver specific ones.

There is no intentional behavior change and only a small rework in logic
in the RAVB driver. Looking at patch 3/7, 4/7 and 7/7 one can clearly
see how the code have been copied from RAVB to the later implementations
in RTSN and RSWITCH.

The delay between v1 and v2 of this series is due to lots of nice work 
by Vadim Fedorenko converting lots of drivers to the ndo_hwtstamp API, 
including these ones and I did not want to cause conflicts with that 
work. This series have been rebased on-top of that work that is now in 
net-next.

Niklas Söderlund (7):
  net: rswitch: Move definition of S4 gPTP offset
  net: rcar_gen4_ptp: Move control fields to users
  net: rswitch: Use common defines for time stamping control
  net: rtsn: Use common defines for time stamping control
  net: rcar_gen4_ptp: Remove unused defines
  net: ravb: Break out Rx hardware timestamping
  net: ravb: Use common defines for time stamping control

 drivers/net/ethernet/renesas/ravb.h          | 14 +---
 drivers/net/ethernet/renesas/ravb_main.c     | 67 ++++++++++----------
 drivers/net/ethernet/renesas/rcar_gen4_ptp.h | 13 ----
 drivers/net/ethernet/renesas/rswitch.h       |  3 +
 drivers/net/ethernet/renesas/rswitch_main.c  | 43 ++++---------
 drivers/net/ethernet/renesas/rtsn.c          | 47 ++++----------
 6 files changed, 64 insertions(+), 123 deletions(-)

-- 
2.51.1

[net-next,v2 1/7] net: rswitch: Move definition of S4 gPTP offset

[Niklas Söderlund]

The files rcar_gen4_ptp.{c,h} implements an abstraction of the gPTP
support implemented together with different other IP blocks. The first
device added which supported this was RSWITCH on R-Car S4.

While doing so the RSWITCH R-Car S4 specific offset was added to the
generic Gen4 gPTP header file. Move it to the RSWITCH driver to make it
clear it only applies to this driver.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v1
- Change the prefix of the define to RSWITCH_.
---
 drivers/net/ethernet/renesas/rcar_gen4_ptp.h | 2 --
 drivers/net/ethernet/renesas/rswitch_main.c  | 4 +++-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rcar_gen4_ptp.h b/drivers/net/ethernet/renesas/rcar_gen4_ptp.h
index f77e79e47357..536badd798cc 100644
--- a/drivers/net/ethernet/renesas/rcar_gen4_ptp.h
+++ b/drivers/net/ethernet/renesas/rcar_gen4_ptp.h
@@ -9,8 +9,6 @@
 
 #include <linux/ptp_clock_kernel.h>
 
-#define RCAR_GEN4_GPTP_OFFSET_S4	0x00018000
-
 /* driver's definitions */
 #define RCAR_GEN4_RXTSTAMP_ENABLED		BIT(0)
 #define RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT	BIT(1)
diff --git a/drivers/net/ethernet/renesas/rswitch_main.c b/drivers/net/ethernet/renesas/rswitch_main.c
index f21a814aa9d1..24ed33ac4bcd 100644
--- a/drivers/net/ethernet/renesas/rswitch_main.c
+++ b/drivers/net/ethernet/renesas/rswitch_main.c
@@ -30,6 +30,8 @@
 #include "rswitch.h"
 #include "rswitch_l2.h"
 
+#define RSWITCH_GPTP_OFFSET_S4 0x00018000
+
 static int rswitch_reg_wait(void __iomem *addr, u32 offs, u32 mask, u32 expected)
 {
 	u32 val;
@@ -2175,7 +2177,7 @@ static int renesas_eth_sw_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->addr))
 		return PTR_ERR(priv->addr);
 
-	priv->ptp_priv->addr = priv->addr + RCAR_GEN4_GPTP_OFFSET_S4;
+	priv->ptp_priv->addr = priv->addr + RSWITCH_GPTP_OFFSET_S4;
 
 	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
 	if (ret < 0) {
-- 
2.51.1

[net-next,v2 2/7] net: rcar_gen4_ptp: Move control fields to users

[Niklas Söderlund]

The struct rcar_gen4_ptp_private provides two fields for convenience of
its users, tstamp_tx_ctrl and tstamp_rx_ctrl. These fields are not used
by the rcar_gen4_ptp driver itself but only by the drivers using it.

Upcoming work will enable the RAVB driver currently only supporting gPTP
on pre-Gen4 SoCs to use the Gen4 implementation as well. To facilitate
this the convenience of having these fields in struct
rcar_gen4_ptp_private becomes a problem as the RAVB driver already have
it's own driver specific fields for the same thing.

Move the fields from struct rcar_gen4_ptp_private to each driver using
the Gen4 gPTP clocks own private data structures. There is no functional
change.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/renesas/rcar_gen4_ptp.h |  2 --
 drivers/net/ethernet/renesas/rswitch.h       |  3 +++
 drivers/net/ethernet/renesas/rswitch_main.c  | 17 ++++++++---------
 drivers/net/ethernet/renesas/rtsn.c          | 17 ++++++++---------
 4 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rcar_gen4_ptp.h b/drivers/net/ethernet/renesas/rcar_gen4_ptp.h
index 536badd798cc..1a1e43add129 100644
--- a/drivers/net/ethernet/renesas/rcar_gen4_ptp.h
+++ b/drivers/net/ethernet/renesas/rcar_gen4_ptp.h
@@ -23,8 +23,6 @@ struct rcar_gen4_ptp_private {
 	struct ptp_clock *clock;
 	struct ptp_clock_info info;
 	spinlock_t lock;	/* For multiple registers access */
-	u32 tstamp_tx_ctrl;
-	u32 tstamp_rx_ctrl;
 	s64 default_addend;
 	bool initialized;
 };
diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
index a1d4a877e5bd..3b348ebf6742 100644
--- a/drivers/net/ethernet/renesas/rswitch.h
+++ b/drivers/net/ethernet/renesas/rswitch.h
@@ -1063,6 +1063,9 @@ struct rswitch_private {
 	bool etha_no_runtime_change;
 	bool gwca_halt;
 	struct net_device *offload_brdev;
+
+	u32 tstamp_tx_ctrl;
+	u32 tstamp_rx_ctrl;
 };
 
 bool is_rdev(const struct net_device *ndev);
diff --git a/drivers/net/ethernet/renesas/rswitch_main.c b/drivers/net/ethernet/renesas/rswitch_main.c
index 24ed33ac4bcd..31aabc6fc462 100644
--- a/drivers/net/ethernet/renesas/rswitch_main.c
+++ b/drivers/net/ethernet/renesas/rswitch_main.c
@@ -845,7 +845,7 @@ static bool rswitch_rx(struct net_device *ndev, int *quota)
 		if (!skb)
 			goto out;
 
-		get_ts = rdev->priv->ptp_priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT;
+		get_ts = rdev->priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT;
 		if (get_ts) {
 			struct skb_shared_hwtstamps *shhwtstamps;
 			struct timespec64 ts;
@@ -1799,14 +1799,13 @@ static int rswitch_hwstamp_get(struct net_device *ndev,
 			       struct kernel_hwtstamp_config *config)
 {
 	struct rswitch_device *rdev = netdev_priv(ndev);
-	struct rcar_gen4_ptp_private *ptp_priv;
-
-	ptp_priv = rdev->priv->ptp_priv;
+	struct rswitch_private *priv = rdev->priv;
 
 	config->flags = 0;
-	config->tx_type = ptp_priv->tstamp_tx_ctrl ? HWTSTAMP_TX_ON :
-						    HWTSTAMP_TX_OFF;
-	switch (ptp_priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE) {
+	config->tx_type =
+		priv->tstamp_tx_ctrl ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
+
+	switch (priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE) {
 	case RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT:
 		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
 		break;
@@ -1856,8 +1855,8 @@ static int rswitch_hwstamp_set(struct net_device *ndev,
 		break;
 	}
 
-	rdev->priv->ptp_priv->tstamp_tx_ctrl = tstamp_tx_ctrl;
-	rdev->priv->ptp_priv->tstamp_rx_ctrl = tstamp_rx_ctrl;
+	rdev->priv->tstamp_tx_ctrl = tstamp_tx_ctrl;
+	rdev->priv->tstamp_rx_ctrl = tstamp_rx_ctrl;
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/renesas/rtsn.c b/drivers/net/ethernet/renesas/rtsn.c
index 15a043e85431..958c19808472 100644
--- a/drivers/net/ethernet/renesas/rtsn.c
+++ b/drivers/net/ethernet/renesas/rtsn.c
@@ -62,6 +62,9 @@ struct rtsn_private {
 
 	int tx_data_irq;
 	int rx_data_irq;
+
+	u32 tstamp_tx_ctrl;
+	u32 tstamp_rx_ctrl;
 };
 
 static u32 rtsn_read(struct rtsn_private *priv, enum rtsn_reg reg)
@@ -162,7 +165,7 @@ static int rtsn_rx(struct net_device *ndev, int budget)
 	unsigned int i;
 	bool get_ts;
 
-	get_ts = priv->ptp_priv->tstamp_rx_ctrl &
+	get_ts = priv->tstamp_rx_ctrl &
 		RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT;
 
 	ndescriptors = priv->dirty_rx + priv->num_rx_ring - priv->cur_rx;
@@ -1122,21 +1125,19 @@ static int rtsn_do_ioctl(struct net_device *ndev, struct ifreq *ifr, int cmd)
 static int rtsn_hwtstamp_get(struct net_device *ndev,
 			     struct kernel_hwtstamp_config *config)
 {
-	struct rcar_gen4_ptp_private *ptp_priv;
 	struct rtsn_private *priv;
 
 	if (!netif_running(ndev))
 		return -ENODEV;
 
 	priv = netdev_priv(ndev);
-	ptp_priv = priv->ptp_priv;
 
 	config->flags = 0;
 
 	config->tx_type =
-		ptp_priv->tstamp_tx_ctrl ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
+		priv->tstamp_tx_ctrl ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
 
-	switch (ptp_priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE) {
+	switch (priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE) {
 	case RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT:
 		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
 		break;
@@ -1155,7 +1156,6 @@ static int rtsn_hwtstamp_set(struct net_device *ndev,
 			     struct kernel_hwtstamp_config *config,
 			     struct netlink_ext_ack *extack)
 {
-	struct rcar_gen4_ptp_private *ptp_priv;
 	struct rtsn_private *priv;
 	u32 tstamp_rx_ctrl;
 	u32 tstamp_tx_ctrl;
@@ -1164,7 +1164,6 @@ static int rtsn_hwtstamp_set(struct net_device *ndev,
 		return -ENODEV;
 
 	priv = netdev_priv(ndev);
-	ptp_priv = priv->ptp_priv;
 
 	if (config->flags)
 		return -EINVAL;
@@ -1195,8 +1194,8 @@ static int rtsn_hwtstamp_set(struct net_device *ndev,
 		break;
 	}
 
-	ptp_priv->tstamp_tx_ctrl = tstamp_tx_ctrl;
-	ptp_priv->tstamp_rx_ctrl = tstamp_rx_ctrl;
+	priv->tstamp_tx_ctrl = tstamp_tx_ctrl;
+	priv->tstamp_rx_ctrl = tstamp_rx_ctrl;
 
 	return 0;
 }
-- 
2.51.1

[net-next,v2 3/7] net: rswitch: Use common defines for time stamping control

[Niklas Söderlund]

Instead of translating to/from driver specific flags for packet time
stamp control use the common flags directly. This simplifies the driver
as the translating code can be removed while at the same time making it
clear the flags are not flags written to hardware registers.

One thing to note is that the bit-wise and check in rswitch_rx() of
RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT is replaced with a not set check of
HWTSTAMP_FILTER_NONE. This is okay as the bit of device specific event
replaced was set for all modes except HWTSTAMP_FILTER_NONE.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v1
- Expand commit message about the non-obvious transformation of of
  'rdev->priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVET' to
  'rdev->priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE'.
---
 drivers/net/ethernet/renesas/rswitch.h      |  4 +--
 drivers/net/ethernet/renesas/rswitch_main.c | 32 ++++++---------------
 2 files changed, 10 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
index 3b348ebf6742..aa605304fed0 100644
--- a/drivers/net/ethernet/renesas/rswitch.h
+++ b/drivers/net/ethernet/renesas/rswitch.h
@@ -1064,8 +1064,8 @@ struct rswitch_private {
 	bool gwca_halt;
 	struct net_device *offload_brdev;
 
-	u32 tstamp_tx_ctrl;
-	u32 tstamp_rx_ctrl;
+	enum hwtstamp_tx_types tstamp_tx_ctrl;
+	enum hwtstamp_rx_filters tstamp_rx_ctrl;
 };
 
 bool is_rdev(const struct net_device *ndev);
diff --git a/drivers/net/ethernet/renesas/rswitch_main.c b/drivers/net/ethernet/renesas/rswitch_main.c
index 31aabc6fc462..e14b21148f27 100644
--- a/drivers/net/ethernet/renesas/rswitch_main.c
+++ b/drivers/net/ethernet/renesas/rswitch_main.c
@@ -845,7 +845,7 @@ static bool rswitch_rx(struct net_device *ndev, int *quota)
 		if (!skb)
 			goto out;
 
-		get_ts = rdev->priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT;
+		get_ts = rdev->priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE;
 		if (get_ts) {
 			struct skb_shared_hwtstamps *shhwtstamps;
 			struct timespec64 ts;
@@ -1802,20 +1802,8 @@ static int rswitch_hwstamp_get(struct net_device *ndev,
 	struct rswitch_private *priv = rdev->priv;
 
 	config->flags = 0;
-	config->tx_type =
-		priv->tstamp_tx_ctrl ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
-
-	switch (priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE) {
-	case RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT:
-		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
-		break;
-	case RCAR_GEN4_RXTSTAMP_TYPE_ALL:
-		config->rx_filter = HWTSTAMP_FILTER_ALL;
-		break;
-	default:
-		config->rx_filter = HWTSTAMP_FILTER_NONE;
-		break;
-	}
+	config->tx_type = priv->tstamp_tx_ctrl;
+	config->rx_filter = priv->tstamp_rx_ctrl;
 
 	return 0;
 }
@@ -1825,18 +1813,16 @@ static int rswitch_hwstamp_set(struct net_device *ndev,
 			       struct netlink_ext_ack *extack)
 {
 	struct rswitch_device *rdev = netdev_priv(ndev);
-	u32 tstamp_rx_ctrl = RCAR_GEN4_RXTSTAMP_ENABLED;
-	u32 tstamp_tx_ctrl;
+	enum hwtstamp_rx_filters tstamp_rx_ctrl;
+	enum hwtstamp_tx_types tstamp_tx_ctrl;
 
 	if (config->flags)
 		return -EINVAL;
 
 	switch (config->tx_type) {
 	case HWTSTAMP_TX_OFF:
-		tstamp_tx_ctrl = 0;
-		break;
 	case HWTSTAMP_TX_ON:
-		tstamp_tx_ctrl = RCAR_GEN4_TXTSTAMP_ENABLED;
+		tstamp_tx_ctrl = config->tx_type;
 		break;
 	default:
 		return -ERANGE;
@@ -1844,14 +1830,12 @@ static int rswitch_hwstamp_set(struct net_device *ndev,
 
 	switch (config->rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
-		tstamp_rx_ctrl = 0;
-		break;
 	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
-		tstamp_rx_ctrl |= RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT;
+		tstamp_rx_ctrl = config->rx_filter;
 		break;
 	default:
 		config->rx_filter = HWTSTAMP_FILTER_ALL;
-		tstamp_rx_ctrl |= RCAR_GEN4_RXTSTAMP_TYPE_ALL;
+		tstamp_rx_ctrl = HWTSTAMP_FILTER_ALL;
 		break;
 	}
 
-- 
2.51.1

[net-next,v2 4/7] net: rtsn: Use common defines for time stamping control

[Niklas Söderlund]

Instead of translating to/from driver specific flags for packet time
stamp control use the common flags directly. This simplifies the driver
as the translating code can be removed while at the same time making it
clear the flags are not flags written to hardware registers.

One thing to note is that the bit-wise and check in rtsn_rx() of
RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT is replaced with a not set check of
HWTSTAMP_FILTER_NONE. This is okay as the bit of device specific event
replaced was set for all modes except HWTSTAMP_FILTER_NONE.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v1
- Expand commit message about the non-obvious transformation of of
  'priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVET' to
  'priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE'.
---
 drivers/net/ethernet/renesas/rtsn.c | 36 +++++++----------------------
 1 file changed, 8 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rtsn.c b/drivers/net/ethernet/renesas/rtsn.c
index 958c19808472..fdb1e7b7fb06 100644
--- a/drivers/net/ethernet/renesas/rtsn.c
+++ b/drivers/net/ethernet/renesas/rtsn.c
@@ -165,8 +165,7 @@ static int rtsn_rx(struct net_device *ndev, int budget)
 	unsigned int i;
 	bool get_ts;
 
-	get_ts = priv->tstamp_rx_ctrl &
-		RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT;
+	get_ts = priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE;
 
 	ndescriptors = priv->dirty_rx + priv->num_rx_ring - priv->cur_rx;
 	rx_packets = 0;
@@ -1133,21 +1132,8 @@ static int rtsn_hwtstamp_get(struct net_device *ndev,
 	priv = netdev_priv(ndev);
 
 	config->flags = 0;
-
-	config->tx_type =
-		priv->tstamp_tx_ctrl ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
-
-	switch (priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE) {
-	case RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT:
-		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
-		break;
-	case RCAR_GEN4_RXTSTAMP_TYPE_ALL:
-		config->rx_filter = HWTSTAMP_FILTER_ALL;
-		break;
-	default:
-		config->rx_filter = HWTSTAMP_FILTER_NONE;
-		break;
-	}
+	config->tx_type = priv->tstamp_tx_ctrl;
+	config->rx_filter = priv->tstamp_rx_ctrl;
 
 	return 0;
 }
@@ -1156,9 +1142,9 @@ static int rtsn_hwtstamp_set(struct net_device *ndev,
 			     struct kernel_hwtstamp_config *config,
 			     struct netlink_ext_ack *extack)
 {
+	enum hwtstamp_rx_filters tstamp_rx_ctrl;
+	enum hwtstamp_tx_types tstamp_tx_ctrl;
 	struct rtsn_private *priv;
-	u32 tstamp_rx_ctrl;
-	u32 tstamp_tx_ctrl;
 
 	if (!netif_running(ndev))
 		return -ENODEV;
@@ -1170,10 +1156,8 @@ static int rtsn_hwtstamp_set(struct net_device *ndev,
 
 	switch (config->tx_type) {
 	case HWTSTAMP_TX_OFF:
-		tstamp_tx_ctrl = 0;
-		break;
 	case HWTSTAMP_TX_ON:
-		tstamp_tx_ctrl = RCAR_GEN4_TXTSTAMP_ENABLED;
+		tstamp_tx_ctrl = config->tx_type;
 		break;
 	default:
 		return -ERANGE;
@@ -1181,16 +1165,12 @@ static int rtsn_hwtstamp_set(struct net_device *ndev,
 
 	switch (config->rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
-		tstamp_rx_ctrl = 0;
-		break;
 	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
-		tstamp_rx_ctrl = RCAR_GEN4_RXTSTAMP_ENABLED |
-			RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT;
+		tstamp_rx_ctrl = config->rx_filter;
 		break;
 	default:
 		config->rx_filter = HWTSTAMP_FILTER_ALL;
-		tstamp_rx_ctrl = RCAR_GEN4_RXTSTAMP_ENABLED |
-			RCAR_GEN4_RXTSTAMP_TYPE_ALL;
+		tstamp_rx_ctrl = HWTSTAMP_FILTER_ALL;
 		break;
 	}
 
-- 
2.51.1

[net-next,v2 5/7] net: rcar_gen4_ptp: Remove unused defines

[Niklas Söderlund]

The driver specific flags to control packet time stamps have all been
replaced by values from enum hwtstamp_tx_types and enum
hwtstamp_rx_filters. Remove the driver specific flags as there are no
more users.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/renesas/rcar_gen4_ptp.h | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rcar_gen4_ptp.h b/drivers/net/ethernet/renesas/rcar_gen4_ptp.h
index 1a1e43add129..9a9c232c854e 100644
--- a/drivers/net/ethernet/renesas/rcar_gen4_ptp.h
+++ b/drivers/net/ethernet/renesas/rcar_gen4_ptp.h
@@ -9,15 +9,6 @@
 
 #include <linux/ptp_clock_kernel.h>
 
-/* driver's definitions */
-#define RCAR_GEN4_RXTSTAMP_ENABLED		BIT(0)
-#define RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT	BIT(1)
-#define RCAR_GEN4_RXTSTAMP_TYPE_ALL		(RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT | BIT(2))
-#define RCAR_GEN4_RXTSTAMP_TYPE			RCAR_GEN4_RXTSTAMP_TYPE_ALL
-
-#define RCAR_GEN4_TXTSTAMP_ENABLED		BIT(0)
-
-
 struct rcar_gen4_ptp_private {
 	void __iomem *addr;
 	struct ptp_clock *clock;
-- 
2.51.1

[net-next,v2 6/7] net: ravb: Break out Rx hardware timestamping

[Niklas Söderlund]

Prepare for moving away from device specific bit-fields to track how to
do hardware Rx timestamping to using net common enums by breaking out
the timestamping to a helper function. This is done to create cleaner
code and prepare for easier changes improving the hardware timestapming.

There is no functional change.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v1
- This is a new patch in v2, suggested by Andrew Lunn to make the final
  patch in the series easier to review. I agree with Andrew the end
  result is much nicer, thanks!
---
 drivers/net/ethernet/renesas/ravb_main.c | 38 +++++++++++++++---------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index cc619dbebf9d..5477bb5c69ae 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -946,6 +946,29 @@ static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
 	return rx_packets;
 }
 
+static void ravb_rx_rcar_hwstamp(struct ravb_private *priv, int q,
+				 struct ravb_ex_rx_desc *desc,
+				 struct sk_buff *skb)
+{
+	u32 get_ts = priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE;
+	struct skb_shared_hwtstamps *shhwtstamps;
+	struct timespec64 ts;
+
+	get_ts &= (q == RAVB_NC) ?
+		RAVB_RXTSTAMP_TYPE_V2_L2_EVENT :
+		~RAVB_RXTSTAMP_TYPE_V2_L2_EVENT;
+
+	if (!get_ts)
+		return;
+
+	shhwtstamps = skb_hwtstamps(skb);
+	memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+	ts.tv_sec = ((u64)le16_to_cpu(desc->ts_sh) << 32)
+		| le32_to_cpu(desc->ts_sl);
+	ts.tv_nsec = le32_to_cpu(desc->ts_n);
+	shhwtstamps->hwtstamp = timespec64_to_ktime(ts);
+}
+
 /* Packet receive function for Ethernet AVB */
 static int ravb_rx_rcar(struct net_device *ndev, int budget, int q)
 {
@@ -955,7 +978,6 @@ static int ravb_rx_rcar(struct net_device *ndev, int budget, int q)
 	struct ravb_ex_rx_desc *desc;
 	unsigned int limit, i;
 	struct sk_buff *skb;
-	struct timespec64 ts;
 	int rx_packets = 0;
 	u8  desc_status;
 	u16 pkt_len;
@@ -992,7 +1014,6 @@ static int ravb_rx_rcar(struct net_device *ndev, int budget, int q)
 			if (desc_status & MSC_CEEF)
 				stats->rx_missed_errors++;
 		} else {
-			u32 get_ts = priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE;
 			struct ravb_rx_buffer *rx_buff;
 			void *rx_addr;
 
@@ -1010,19 +1031,8 @@ static int ravb_rx_rcar(struct net_device *ndev, int budget, int q)
 				break;
 			}
 			skb_mark_for_recycle(skb);
-			get_ts &= (q == RAVB_NC) ?
-					RAVB_RXTSTAMP_TYPE_V2_L2_EVENT :
-					~RAVB_RXTSTAMP_TYPE_V2_L2_EVENT;
-			if (get_ts) {
-				struct skb_shared_hwtstamps *shhwtstamps;
 
-				shhwtstamps = skb_hwtstamps(skb);
-				memset(shhwtstamps, 0, sizeof(*shhwtstamps));
-				ts.tv_sec = ((u64) le16_to_cpu(desc->ts_sh) <<
-					     32) | le32_to_cpu(desc->ts_sl);
-				ts.tv_nsec = le32_to_cpu(desc->ts_n);
-				shhwtstamps->hwtstamp = timespec64_to_ktime(ts);
-			}
+			ravb_rx_rcar_hwstamp(priv, q, desc, skb);
 
 			skb_put(skb, pkt_len);
 			skb->protocol = eth_type_trans(skb, ndev);
-- 
2.51.1

[net-next,v2 7/7] net: ravb: Use common defines for time stamping control

[Niklas Söderlund]

Instead of translating to/from driver specific flags for packet time
stamp control use the common flags directly. This simplifies the driver
as the translating code can be removed while at the same time making it
clear the flags are not flags written to hardware registers.

The change from a device specific bit-field track variable to the common
enum datatypes forces us to touch the ravb_rx_rcar_hwstamp() in a non
trivial way. To make this cleaner and easier to understand expand the
nested conditions.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/ethernet/renesas/ravb.h      | 14 ++-------
 drivers/net/ethernet/renesas/ravb_main.c | 37 ++++++++----------------
 2 files changed, 14 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index d65cd83ddd16..5e56ec9b1013 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -35,16 +35,6 @@
 /* Driver's parameters */
 #define RAVB_ALIGN	128
 
-/* Hardware time stamp */
-#define RAVB_TXTSTAMP_VALID	0x00000001	/* TX timestamp valid */
-#define RAVB_TXTSTAMP_ENABLED	0x00000010	/* Enable TX timestamping */
-
-#define RAVB_RXTSTAMP_VALID	0x00000001	/* RX timestamp valid */
-#define RAVB_RXTSTAMP_TYPE	0x00000006	/* RX type mask */
-#define RAVB_RXTSTAMP_TYPE_V2_L2_EVENT 0x00000002
-#define RAVB_RXTSTAMP_TYPE_ALL	0x00000006
-#define RAVB_RXTSTAMP_ENABLED	0x00000010	/* Enable RX timestamping */
-
 enum ravb_reg {
 	/* AVB-DMAC registers */
 	CCC	= 0x0000,
@@ -1114,8 +1104,8 @@ struct ravb_private {
 	u32 rx_over_errors;
 	u32 rx_fifo_errors;
 	struct net_device_stats stats[NUM_RX_QUEUE];
-	u32 tstamp_tx_ctrl;
-	u32 tstamp_rx_ctrl;
+	enum hwtstamp_tx_types tstamp_tx_ctrl;
+	enum hwtstamp_rx_filters tstamp_rx_ctrl;
 	struct list_head ts_skb_list;
 	u32 ts_skb_tag;
 	struct ravb_ptp ptp;
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 5477bb5c69ae..1680e94b9242 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -950,13 +950,14 @@ static void ravb_rx_rcar_hwstamp(struct ravb_private *priv, int q,
 				 struct ravb_ex_rx_desc *desc,
 				 struct sk_buff *skb)
 {
-	u32 get_ts = priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE;
 	struct skb_shared_hwtstamps *shhwtstamps;
 	struct timespec64 ts;
+	bool get_ts;
 
-	get_ts &= (q == RAVB_NC) ?
-		RAVB_RXTSTAMP_TYPE_V2_L2_EVENT :
-		~RAVB_RXTSTAMP_TYPE_V2_L2_EVENT;
+	if (q == RAVB_NC)
+		get_ts = priv->tstamp_rx_ctrl == HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+	else
+		get_ts = priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
 
 	if (!get_ts)
 		return;
@@ -2424,18 +2425,8 @@ static int ravb_hwtstamp_get(struct net_device *ndev,
 	struct ravb_private *priv = netdev_priv(ndev);
 
 	config->flags = 0;
-	config->tx_type = priv->tstamp_tx_ctrl ? HWTSTAMP_TX_ON :
-						 HWTSTAMP_TX_OFF;
-	switch (priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE) {
-	case RAVB_RXTSTAMP_TYPE_V2_L2_EVENT:
-		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
-		break;
-	case RAVB_RXTSTAMP_TYPE_ALL:
-		config->rx_filter = HWTSTAMP_FILTER_ALL;
-		break;
-	default:
-		config->rx_filter = HWTSTAMP_FILTER_NONE;
-	}
+	config->tx_type = priv->tstamp_tx_ctrl;
+	config->rx_filter = priv->tstamp_rx_ctrl;
 
 	return 0;
 }
@@ -2446,15 +2437,13 @@ static int ravb_hwtstamp_set(struct net_device *ndev,
 			     struct netlink_ext_ack *extack)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	u32 tstamp_rx_ctrl = RAVB_RXTSTAMP_ENABLED;
-	u32 tstamp_tx_ctrl;
+	enum hwtstamp_rx_filters tstamp_rx_ctrl;
+	enum hwtstamp_tx_types tstamp_tx_ctrl;
 
 	switch (config->tx_type) {
 	case HWTSTAMP_TX_OFF:
-		tstamp_tx_ctrl = 0;
-		break;
 	case HWTSTAMP_TX_ON:
-		tstamp_tx_ctrl = RAVB_TXTSTAMP_ENABLED;
+		tstamp_tx_ctrl = config->tx_type;
 		break;
 	default:
 		return -ERANGE;
@@ -2462,14 +2451,12 @@ static int ravb_hwtstamp_set(struct net_device *ndev,
 
 	switch (config->rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
-		tstamp_rx_ctrl = 0;
-		break;
 	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
-		tstamp_rx_ctrl |= RAVB_RXTSTAMP_TYPE_V2_L2_EVENT;
+		tstamp_rx_ctrl = config->rx_filter;
 		break;
 	default:
 		config->rx_filter = HWTSTAMP_FILTER_ALL;
-		tstamp_rx_ctrl |= RAVB_RXTSTAMP_TYPE_ALL;
+		tstamp_rx_ctrl = HWTSTAMP_FILTER_ALL;
 	}
 
 	priv->tstamp_tx_ctrl = tstamp_tx_ctrl;
-- 
2.51.1

Re: [net-next,v2 0/7] net: renesas: Cleanup usage of gPTP flags

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  4 Nov 2025 23:24:13 +0100 you wrote:
> Hello,
> 
> This series aim is to prepare for future work that will enable the use
> of gPTP on R-Car RAVB on Gen4. Currently RAVB have a dedicated gPTP
> implementation supported on Gen2 and Gen3 (ravb_ptp.c). For Gen4 a new
> implementation that is already upstream (rcar_gen4_ptp.c) and used by
> other Gen4 devices such as RTSN and RSWITCH is needed.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/7] net: rswitch: Move definition of S4 gPTP offset
    https://git.kernel.org/netdev/net-next/c/e98d8792929d
  - [net-next,v2,2/7] net: rcar_gen4_ptp: Move control fields to users
    https://git.kernel.org/netdev/net-next/c/50ab1c6becde
  - [net-next,v2,3/7] net: rswitch: Use common defines for time stamping control
    https://git.kernel.org/netdev/net-next/c/b314e4f7a9d9
  - [net-next,v2,4/7] net: rtsn: Use common defines for time stamping control
    https://git.kernel.org/netdev/net-next/c/e43791f40b81
  - [net-next,v2,5/7] net: rcar_gen4_ptp: Remove unused defines
    https://git.kernel.org/netdev/net-next/c/3614d249d1da
  - [net-next,v2,6/7] net: ravb: Break out Rx hardware timestamping
    https://git.kernel.org/netdev/net-next/c/5ce97b8d6132
  - [net-next,v2,7/7] net: ravb: Use common defines for time stamping control
    https://git.kernel.org/netdev/net-next/c/16e2e6cf75e6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [net-next,v2 7/7] net: ravb: Use common defines for time stamping control

[Simon Horman]

On Tue, Nov 04, 2025 at 11:24:20PM +0100, Niklas Söderlund wrote:
> Instead of translating to/from driver specific flags for packet time
> stamp control use the common flags directly. This simplifies the driver
> as the translating code can be removed while at the same time making it
> clear the flags are not flags written to hardware registers.
> 
> The change from a device specific bit-field track variable to the common
> enum datatypes forces us to touch the ravb_rx_rcar_hwstamp() in a non
> trivial way. To make this cleaner and easier to understand expand the
> nested conditions.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

...

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 5477bb5c69ae..1680e94b9242 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -950,13 +950,14 @@ static void ravb_rx_rcar_hwstamp(struct ravb_private *priv, int q,
>  				 struct ravb_ex_rx_desc *desc,
>  				 struct sk_buff *skb)
>  {
> -	u32 get_ts = priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE;
>  	struct skb_shared_hwtstamps *shhwtstamps;
>  	struct timespec64 ts;
> +	bool get_ts;
>  
> -	get_ts &= (q == RAVB_NC) ?
> -		RAVB_RXTSTAMP_TYPE_V2_L2_EVENT :
> -		~RAVB_RXTSTAMP_TYPE_V2_L2_EVENT;
> +	if (q == RAVB_NC)
> +		get_ts = priv->tstamp_rx_ctrl == HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
> +	else
> +		get_ts = priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
>  
>  	if (!get_ts)
>  		return;

Hi Niklas,

It is Friday evening and I'm exercising a new tool, so please forgive me if
this analysis is wrong. But it seems that there are cases where there old
bit-based logic and the new integer equality based logic don't match.

1. If q == RAVB_NC then previously timestamping would occur
   for HWTSTAMP_FILTER_ALL, because:

   (RAVB_TXTSTAMP_ENABLED | RAVB_RXTSTAMP_TYPE_ALL) &
    RAVB_RXTSTAMP_TYPE & RAVB_RXTSTAMP_TYPE_V2_L2_EVENT =
   (0x10 | 0x6) & 0x06 & 0x02 = 0x2, which is non-zero.

   But with the new logic timestamping does not occur because:

   HWTSTAMP_FILTER_ALL == HWTSTAMP_FILTER_PTP_V2_L2_EVENT is false

2. If q != RAVB_NC then previously timestamping would not occur
   for HWTSTAMP_FILTER_NONE because:

   0 & RAVB_RXTSTAMP_TYPE & RAVB_RXTSTAMP_TYPE_V2_L2_EVENT = 0

   But with the new logic timestamping does occur because:

   HWTSTAMP_FILTER_NONE != HWTSTAMP_FILTER_PTP_V2_L2_EVENT is true

I came across this by chance because this patch is currently
the most recent patch in net-next that touches C code. And I was
exercising Claude Code with https://github.com/masoncl/review-prompts
It reported the above and after significantly
more thinking I've come to agree with it.

But it is Friday evening, so YMMV.

For reference, I've provided the text generated by Claude Code at the end of
this email.

...

> @@ -2446,15 +2437,13 @@ static int ravb_hwtstamp_set(struct net_device *ndev,
>  			     struct netlink_ext_ack *extack)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> -	u32 tstamp_rx_ctrl = RAVB_RXTSTAMP_ENABLED;
> -	u32 tstamp_tx_ctrl;
> +	enum hwtstamp_rx_filters tstamp_rx_ctrl;
> +	enum hwtstamp_tx_types tstamp_tx_ctrl;
>  
>  	switch (config->tx_type) {
>  	case HWTSTAMP_TX_OFF:
> -		tstamp_tx_ctrl = 0;
> -		break;
>  	case HWTSTAMP_TX_ON:
> -		tstamp_tx_ctrl = RAVB_TXTSTAMP_ENABLED;
> +		tstamp_tx_ctrl = config->tx_type;
>  		break;
>  	default:
>  		return -ERANGE;
> @@ -2462,14 +2451,12 @@ static int ravb_hwtstamp_set(struct net_device *ndev,
>  
>  	switch (config->rx_filter) {
>  	case HWTSTAMP_FILTER_NONE:
> -		tstamp_rx_ctrl = 0;
> -		break;
>  	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> -		tstamp_rx_ctrl |= RAVB_RXTSTAMP_TYPE_V2_L2_EVENT;
> +		tstamp_rx_ctrl = config->rx_filter;
>  		break;
>  	default:
>  		config->rx_filter = HWTSTAMP_FILTER_ALL;
> -		tstamp_rx_ctrl |= RAVB_RXTSTAMP_TYPE_ALL;
> +		tstamp_rx_ctrl = HWTSTAMP_FILTER_ALL;
>  	}
>  
>  	priv->tstamp_tx_ctrl = tstamp_tx_ctrl;
> -- 
> 2.51.1
> 
> 

Text generated by Claude Code:

Is this logic translation correct? Looking at the old bit-field
operations and new enum comparisons, the behavior seems to change in
two cases:

Old behavior (from ravb_hwtstamp_set):
  - FILTER_NONE stored as 0
  - FILTER_PTP_V2_L2_EVENT stored as 0x12 (ENABLED | TYPE_V2_L2)
  - FILTER_ALL stored as 0x16 (ENABLED | TYPE_ALL)

Old ravb_rx_rcar_hwstamp for RAVB_NC queue:
  get_ts = (0x12 & 0x06) & 0x02 = 0x02 & 0x02 = 0x02 (timestamp)
  get_ts = (0x16 & 0x06) & 0x02 = 0x06 & 0x02 = 0x02 (timestamp)

So the old code timestamps RAVB_NC queue packets for both
FILTER_PTP_V2_L2_EVENT and FILTER_ALL.

New ravb_rx_rcar_hwstamp for RAVB_NC queue with FILTER_ALL:
  get_ts = (HWTSTAMP_FILTER_ALL == HWTSTAMP_FILTER_PTP_V2_L2_EVENT)
  get_ts = (1 == 9) = false (no timestamp)

Similarly for non-RAVB_NC queue, old behavior with FILTER_NONE:
  get_ts = (0 & 0x06) & ~0x02 = 0 & ~0x02 = 0 (no timestamp)

New behavior for non-RAVB_NC queue with FILTER_NONE:
  get_ts = (HWTSTAMP_FILTER_NONE != HWTSTAMP_FILTER_PTP_V2_L2_EVENT)
  get_ts = (0 != 9) = true (timestamp)

Can the refactoring introduce these two behavior changes?

1. RAVB_NC queue with FILTER_ALL: old code timestamps, new code doesn't
2. Non-RAVB_NC queue with FILTER_NONE: old code doesn't timestamp, new
   code does

Re: [net-next,v2 7/7] net: ravb: Use common defines for time stamping control

[Niklas Söderlund]

Hi Simon,

Thanks for your test of new tools!

On 2025-11-07 17:50:02 +0000, Simon Horman wrote:
> On Tue, Nov 04, 2025 at 11:24:20PM +0100, Niklas Söderlund wrote:
> > Instead of translating to/from driver specific flags for packet time
> > stamp control use the common flags directly. This simplifies the driver
> > as the translating code can be removed while at the same time making it
> > clear the flags are not flags written to hardware registers.
> > 
> > The change from a device specific bit-field track variable to the common
> > enum datatypes forces us to touch the ravb_rx_rcar_hwstamp() in a non
> > trivial way. To make this cleaner and easier to understand expand the
> > nested conditions.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index 5477bb5c69ae..1680e94b9242 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -950,13 +950,14 @@ static void ravb_rx_rcar_hwstamp(struct ravb_private *priv, int q,
> >  				 struct ravb_ex_rx_desc *desc,
> >  				 struct sk_buff *skb)
> >  {
> > -	u32 get_ts = priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE;
> >  	struct skb_shared_hwtstamps *shhwtstamps;
> >  	struct timespec64 ts;
> > +	bool get_ts;
> >  
> > -	get_ts &= (q == RAVB_NC) ?
> > -		RAVB_RXTSTAMP_TYPE_V2_L2_EVENT :
> > -		~RAVB_RXTSTAMP_TYPE_V2_L2_EVENT;
> > +	if (q == RAVB_NC)
> > +		get_ts = priv->tstamp_rx_ctrl == HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
> > +	else
> > +		get_ts = priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
> >  
> >  	if (!get_ts)
> >  		return;
> 
> Hi Niklas,
> 
> It is Friday evening and I'm exercising a new tool, so please forgive me if
> this analysis is wrong. But it seems that there are cases where there old
> bit-based logic and the new integer equality based logic don't match.
> 
> 1. If q == RAVB_NC then previously timestamping would occur
>    for HWTSTAMP_FILTER_ALL, because:
> 
>    (RAVB_TXTSTAMP_ENABLED | RAVB_RXTSTAMP_TYPE_ALL) &
>     RAVB_RXTSTAMP_TYPE & RAVB_RXTSTAMP_TYPE_V2_L2_EVENT =
>    (0x10 | 0x6) & 0x06 & 0x02 = 0x2, which is non-zero.
> 
>    But with the new logic timestamping does not occur because:
> 
>    HWTSTAMP_FILTER_ALL == HWTSTAMP_FILTER_PTP_V2_L2_EVENT is false
> 
> 2. If q != RAVB_NC then previously timestamping would not occur
>    for HWTSTAMP_FILTER_NONE because:
> 
>    0 & RAVB_RXTSTAMP_TYPE & RAVB_RXTSTAMP_TYPE_V2_L2_EVENT = 0
> 
>    But with the new logic timestamping does occur because:
> 
>    HWTSTAMP_FILTER_NONE != HWTSTAMP_FILTER_PTP_V2_L2_EVENT is true

I must have written this code a late Friday night.. you are absolutely 
correct! Those two error cases exists. I wrote a test program to really 
verify it.

NG q: RAVB_BE old: 0=0, new=HWTSTAMP_FILTER_NONE=1
OK q: RAVB_NC old: 0=0, new=HWTSTAMP_FILTER_NONE=0
OK q: RAVB_BE old: RAVB_RXTSTAMP_TYPE_V2_L2_EVENT=0, new=HWTSTAMP_FILTER_PTP_V2_L2_EVENT=0
OK q: RAVB_NC old: RAVB_RXTSTAMP_TYPE_V2_L2_EVENT=1, new=HWTSTAMP_FILTER_PTP_V2_L2_EVENT=1
OK q: RAVB_BE old: RAVB_RXTSTAMP_TYPE_ALL=1, new=HWTSTAMP_FILTER_ALL=1
NG q: RAVB_NC old: RAVB_RXTSTAMP_TYPE_ALL=1, new=HWTSTAMP_FILTER_ALL=0

The correct conversion to int comparison are,

        if (q == RAVB_NC)
                get_ts = tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE;
        else
                get_ts = tstamp_rx_ctrl == HWTSTAMP_FILTER_ALL;

And yes, I did use my test to verify it :-)

OK q: RAVB_BE old: 0=0, new=HWTSTAMP_FILTER_NONE=0
OK q: RAVB_NC old: 0=0, new=HWTSTAMP_FILTER_NONE=0
OK q: RAVB_BE old: RAVB_RXTSTAMP_TYPE_V2_L2_EVENT=0, new=HWTSTAMP_FILTER_PTP_V2_L2_EVENT=0
OK q: RAVB_NC old: RAVB_RXTSTAMP_TYPE_V2_L2_EVENT=1, new=HWTSTAMP_FILTER_PTP_V2_L2_EVENT=1
OK q: RAVB_BE old: RAVB_RXTSTAMP_TYPE_ALL=1, new=HWTSTAMP_FILTER_ALL=1
OK q: RAVB_NC old: RAVB_RXTSTAMP_TYPE_ALL=1, new=HWTSTAMP_FILTER_ALL=1

I will submit a correction promptly.

> 
> I came across this by chance because this patch is currently
> the most recent patch in net-next that touches C code. And I was
> exercising Claude Code with https://github.com/masoncl/review-prompts
> It reported the above and after significantly
> more thinking I've come to agree with it.

I'm happy you did! Thanks for spotting this.

> 
> But it is Friday evening, so YMMV.

Obviously, but mostly on for the non-AI entities it seems :-)

> 
> For reference, I've provided the text generated by Claude Code at the end of
> this email.

Seems like a very useful tool!

> 
> ...
> 
> > @@ -2446,15 +2437,13 @@ static int ravb_hwtstamp_set(struct net_device *ndev,
> >  			     struct netlink_ext_ack *extack)
> >  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> > -	u32 tstamp_rx_ctrl = RAVB_RXTSTAMP_ENABLED;
> > -	u32 tstamp_tx_ctrl;
> > +	enum hwtstamp_rx_filters tstamp_rx_ctrl;
> > +	enum hwtstamp_tx_types tstamp_tx_ctrl;
> >  
> >  	switch (config->tx_type) {
> >  	case HWTSTAMP_TX_OFF:
> > -		tstamp_tx_ctrl = 0;
> > -		break;
> >  	case HWTSTAMP_TX_ON:
> > -		tstamp_tx_ctrl = RAVB_TXTSTAMP_ENABLED;
> > +		tstamp_tx_ctrl = config->tx_type;
> >  		break;
> >  	default:
> >  		return -ERANGE;
> > @@ -2462,14 +2451,12 @@ static int ravb_hwtstamp_set(struct net_device *ndev,
> >  
> >  	switch (config->rx_filter) {
> >  	case HWTSTAMP_FILTER_NONE:
> > -		tstamp_rx_ctrl = 0;
> > -		break;
> >  	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> > -		tstamp_rx_ctrl |= RAVB_RXTSTAMP_TYPE_V2_L2_EVENT;
> > +		tstamp_rx_ctrl = config->rx_filter;
> >  		break;
> >  	default:
> >  		config->rx_filter = HWTSTAMP_FILTER_ALL;
> > -		tstamp_rx_ctrl |= RAVB_RXTSTAMP_TYPE_ALL;
> > +		tstamp_rx_ctrl = HWTSTAMP_FILTER_ALL;
> >  	}
> >  
> >  	priv->tstamp_tx_ctrl = tstamp_tx_ctrl;
> > -- 
> > 2.51.1
> > 
> > 
> 
> Text generated by Claude Code:
> 
> Is this logic translation correct? Looking at the old bit-field
> operations and new enum comparisons, the behavior seems to change in
> two cases:
> 
> Old behavior (from ravb_hwtstamp_set):
>   - FILTER_NONE stored as 0
>   - FILTER_PTP_V2_L2_EVENT stored as 0x12 (ENABLED | TYPE_V2_L2)
>   - FILTER_ALL stored as 0x16 (ENABLED | TYPE_ALL)
> 
> Old ravb_rx_rcar_hwstamp for RAVB_NC queue:
>   get_ts = (0x12 & 0x06) & 0x02 = 0x02 & 0x02 = 0x02 (timestamp)
>   get_ts = (0x16 & 0x06) & 0x02 = 0x06 & 0x02 = 0x02 (timestamp)
> 
> So the old code timestamps RAVB_NC queue packets for both
> FILTER_PTP_V2_L2_EVENT and FILTER_ALL.
> 
> New ravb_rx_rcar_hwstamp for RAVB_NC queue with FILTER_ALL:
>   get_ts = (HWTSTAMP_FILTER_ALL == HWTSTAMP_FILTER_PTP_V2_L2_EVENT)
>   get_ts = (1 == 9) = false (no timestamp)
> 
> Similarly for non-RAVB_NC queue, old behavior with FILTER_NONE:
>   get_ts = (0 & 0x06) & ~0x02 = 0 & ~0x02 = 0 (no timestamp)
> 
> New behavior for non-RAVB_NC queue with FILTER_NONE:
>   get_ts = (HWTSTAMP_FILTER_NONE != HWTSTAMP_FILTER_PTP_V2_L2_EVENT)
>   get_ts = (0 != 9) = true (timestamp)
> 
> Can the refactoring introduce these two behavior changes?
> 
> 1. RAVB_NC queue with FILTER_ALL: old code timestamps, new code doesn't
> 2. Non-RAVB_NC queue with FILTER_NONE: old code doesn't timestamp, new
>    code does

-- 
Kind Regards,
Niklas Söderlund