[net-next 0/6] net: renesas: Cleanup usage of gPTP flags

[net-next 0/6] 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/6 is a drive by patch where RSWITCH specific define was added in 
the wrong header. Patch 2/6 removes a short-cut used in RTSN and RSWITCH 
that prevents extending Gen4 support to RAVB without fuss. While patch 
3/6 to 6/6 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/6, 4/6 and 6/6 one can clearly 
see how the code have been copied from RAVB to the later implementations 
in RTSN and RSWITCH.

Niklas Söderlund (6):
  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: Use common defines for time stamping control

 drivers/net/ethernet/renesas/ravb.h          | 14 +----
 drivers/net/ethernet/renesas/ravb_main.c     | 62 +++++++++-----------
 drivers/net/ethernet/renesas/rcar_gen4_ptp.h | 13 ----
 drivers/net/ethernet/renesas/rswitch.h       |  3 +
 drivers/net/ethernet/renesas/rswitch_main.c  | 41 ++++---------
 drivers/net/ethernet/renesas/rtsn.c          | 47 ++++-----------
 6 files changed, 59 insertions(+), 121 deletions(-)

-- 
2.51.0

[net-next 1/6] 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>
---
 drivers/net/ethernet/renesas/rcar_gen4_ptp.h | 2 --
 drivers/net/ethernet/renesas/rswitch_main.c  | 2 ++
 2 files changed, 2 insertions(+), 2 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 ff5f966c98a9..72fa31008144 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 RCAR_GEN4_GPTP_OFFSET_S4	0x00018000
+
 static int rswitch_reg_wait(void __iomem *addr, u32 offs, u32 mask, u32 expected)
 {
 	u32 val;
-- 
2.51.0

[net-next 2/6] 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>
---
 drivers/net/ethernet/renesas/rcar_gen4_ptp.h |  2 --
 drivers/net/ethernet/renesas/rswitch.h       |  3 +++
 drivers/net/ethernet/renesas/rswitch_main.c  | 16 +++++++---------
 drivers/net/ethernet/renesas/rtsn.c          | 17 ++++++++---------
 4 files changed, 18 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 72fa31008144..b541202b8f3e 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;
@@ -1800,15 +1800,13 @@ static struct net_device_stats *rswitch_get_stats(struct net_device *ndev)
 static int rswitch_hwstamp_get(struct net_device *ndev, struct ifreq *req)
 {
 	struct rswitch_device *rdev = netdev_priv(ndev);
-	struct rcar_gen4_ptp_private *ptp_priv;
+	struct rswitch_private *priv = rdev->priv;
 	struct hwtstamp_config config;
 
-	ptp_priv = rdev->priv->ptp_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;
@@ -1860,8 +1858,8 @@ static int rswitch_hwstamp_set(struct net_device *ndev, struct ifreq *req)
 		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 copy_to_user(req->ifr_data, &config, sizeof(config)) ? -EFAULT : 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.0

[net-next 3/6] 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.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/ethernet/renesas/rswitch.h      |  4 +--
 drivers/net/ethernet/renesas/rswitch_main.c | 31 ++++++---------------
 2 files changed, 10 insertions(+), 25 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 b541202b8f3e..c31e26002253 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;
@@ -1804,19 +1804,8 @@ static int rswitch_hwstamp_get(struct net_device *ndev, struct ifreq *req)
 	struct hwtstamp_config config;
 
 	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 copy_to_user(req->ifr_data, &config, sizeof(config)) ? -EFAULT : 0;
 }
@@ -1824,9 +1813,9 @@ static int rswitch_hwstamp_get(struct net_device *ndev, struct ifreq *req)
 static int rswitch_hwstamp_set(struct net_device *ndev, struct ifreq *req)
 {
 	struct rswitch_device *rdev = netdev_priv(ndev);
-	u32 tstamp_rx_ctrl = RCAR_GEN4_RXTSTAMP_ENABLED;
+	enum hwtstamp_rx_filters tstamp_rx_ctrl;
+	enum hwtstamp_tx_types tstamp_tx_ctrl;
 	struct hwtstamp_config config;
-	u32 tstamp_tx_ctrl;
 
 	if (copy_from_user(&config, req->ifr_data, sizeof(config)))
 		return -EFAULT;
@@ -1836,10 +1825,8 @@ static int rswitch_hwstamp_set(struct net_device *ndev, struct ifreq *req)
 
 	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;
@@ -1847,14 +1834,12 @@ static int rswitch_hwstamp_set(struct net_device *ndev, struct ifreq *req)
 
 	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.0

[net-next 4/6] 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.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 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.0

[net-next 5/6] 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>
---
 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.0

[net-next 6/6] 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.

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 | 62 +++++++++++-------------
 2 files changed, 30 insertions(+), 46 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 7b48060c250b..655a9c968ce5 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 9d3bd65b85ff..a187a18fb9e6 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -992,7 +992,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,18 +1009,27 @@ 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);
+
+			if (priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE) {
+				bool get_ts = false;
+
+				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) {
+					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);
+				}
 			}
 
 			skb_put(skb, pkt_len);
@@ -2396,18 +2404,8 @@ static int ravb_hwtstamp_get(struct net_device *ndev, struct ifreq *req)
 	struct hwtstamp_config config;
 
 	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 copy_to_user(req->ifr_data, &config, sizeof(config)) ?
 		-EFAULT : 0;
@@ -2417,19 +2415,17 @@ static int ravb_hwtstamp_get(struct net_device *ndev, struct ifreq *req)
 static int ravb_hwtstamp_set(struct net_device *ndev, struct ifreq *req)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	enum hwtstamp_rx_filters tstamp_rx_ctrl;
+	enum hwtstamp_tx_types tstamp_tx_ctrl;
 	struct hwtstamp_config config;
-	u32 tstamp_rx_ctrl = RAVB_RXTSTAMP_ENABLED;
-	u32 tstamp_tx_ctrl;
 
 	if (copy_from_user(&config, req->ifr_data, sizeof(config)))
 		return -EFAULT;
 
 	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;
@@ -2437,14 +2433,12 @@ static int ravb_hwtstamp_set(struct net_device *ndev, struct ifreq *req)
 
 	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.0

Re: [net-next 1/6] net: rswitch: Move definition of S4 gPTP offset

[Andrew Lunn]

On Tue, Sep 16, 2025 at 12:10:50PM +0200, Niklas Söderlund wrote:
> 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.

This is a nice simple patch to understand, which is good. But i do
wounder about naming schemes. Since this is a RSWITCH define, should
it use the RSWITCH_ prefix? 

Are there other implementations which have an equivalent of
RCAR_GEN4_GPTP_OFFSET_S4? How are they named?

	Andrew

Re: [net-next 1/6] net: rswitch: Move definition of S4 gPTP offset

[Niklas Söderlund]

On 2025-09-16 14:16:09 +0200, Andrew Lunn wrote:
> On Tue, Sep 16, 2025 at 12:10:50PM +0200, Niklas Söderlund wrote:
> > 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.
> 
> This is a nice simple patch to understand, which is good. But i do
> wounder about naming schemes. Since this is a RSWITCH define, should
> it use the RSWITCH_ prefix? 

It could, I opted for the least disruptive path and just moved it as is.  
Would you prefers I rename?

> 
> Are there other implementations which have an equivalent of
> RCAR_GEN4_GPTP_OFFSET_S4? How are they named?

There are none, at least not so far as the RSWITCH IP are only available 
on the R-Car S4 SoC.

> 
> 	Andrew

-- 
Kind Regards,
Niklas Söderlund

Re: [net-next 2/6] net: rcar_gen4_ptp: Move control fields to users

[Andrew Lunn]

On Tue, Sep 16, 2025 at 12:10:51PM +0200, Niklas Söderlund wrote:
> 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>

    Andrew

Re: [net-next 3/6] net: rswitch: Use common defines for time stamping control

[Andrew Lunn]

> -		get_ts = rdev->priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT;
> +		get_ts = rdev->priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE;

That is not an obvious transformation. The first looks like a specific
subset of events, while the second looks like any sort. It might be
worth commenting about this in the commit message.

    Andrew

---
pw-bot: cr

Re: [net-next 5/6] net: rcar_gen4_ptp: Remove unused defines

[Andrew Lunn]

On Tue, Sep 16, 2025 at 12:10:54PM +0200, Niklas Söderlund wrote:
> 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>

    Andrew

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

[Andrew Lunn]

> @@ -1010,18 +1009,27 @@ 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);
> +
> +			if (priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE) {
> +				bool get_ts = false;
> +
> +				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) {
> +					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);
> +				}

This hunk is bigger than it needs to be because this block has been
indented further. Maybe keep get_ts as function scope, initialised to
false, so you don't need to touch this block?

	Andrew

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

[Niklas Söderlund]

On 2025-09-16 14:38:58 +0200, Andrew Lunn wrote:
> > @@ -1010,18 +1009,27 @@ 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);
> > +
> > +			if (priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE) {
> > +				bool get_ts = false;
> > +
> > +				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) {
> > +					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);
> > +				}
> 
> This hunk is bigger than it needs to be because this block has been
> indented further. Maybe keep get_ts as function scope, initialised to
> false, so you don't need to touch this block?

Thanks for the suggestion. I could do that. What I like about this is 
that it's immediately clear that all this depends on 
priv->tstamp_rx_ctrl.

If I keep the chunk as-is it gives the impression there is some other 
condition other then priv->tstamp_rx_ctrl that could set get_ts and make 
a valid use-case of reading the timestamp from the descriptor.

I could break it out to a separate function if you prefer to reduce the 
indentation level,

static void ravb_rx_rcar_hwstamp(...)
{
    bool get_ts = false;

    ...

    if (get_ts) {
        ....
    }

}

static int ravb_rx_rcar(..)
{
    ...

    if (priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE)
        ravb_rx_rcar_hwstamp(...);

    ...
}

Would that work ?

> 
> 	Andrew

-- 
Kind Regards,
Niklas Söderlund

Re: [net-next 3/6] net: rswitch: Use common defines for time stamping control

[Niklas Söderlund]

On 2025-09-16 14:31:29 +0200, Andrew Lunn wrote:
> > -		get_ts = rdev->priv->tstamp_rx_ctrl & RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT;
> > +		get_ts = rdev->priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE;
> 
> That is not an obvious transformation. The first looks like a specific
> subset of events, while the second looks like any sort. It might be
> worth commenting about this in the commit message.

Good point, I will do so in v2.

> 
>     Andrew
> 
> ---
> pw-bot: cr

-- 
Kind Regards,
Niklas Söderlund

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

[Andrew Lunn]

On Tue, Sep 16, 2025 at 03:08:48PM +0200, Niklas Söderlund wrote:
> On 2025-09-16 14:38:58 +0200, Andrew Lunn wrote:
> > > @@ -1010,18 +1009,27 @@ 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);
> > > +
> > > +			if (priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE) {
> > > +				bool get_ts = false;
> > > +
> > > +				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) {
> > > +					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);
> > > +				}
> > 
> > This hunk is bigger than it needs to be because this block has been
> > indented further. Maybe keep get_ts as function scope, initialised to
> > false, so you don't need to touch this block?
> 
> Thanks for the suggestion. I could do that. What I like about this is 
> that it's immediately clear that all this depends on 
> priv->tstamp_rx_ctrl.

True.

As a reviewer, i was asking myself, has the code actually setting the
timestamp in the skbuf changed? Do i need to look at it in detail?
There should not be any need for it to change....

> I could break it out to a separate function if you prefer to reduce the 
> indentation level,

It is not really indentation level, but the fact it is in the hunk,
meaning should i look at it?

So maybe add a patch which moves the copying of the timestamp from the
descriptor into the skbuf into a helper. This patch then just calls
the helper, making this hunk much smaller and more obvious?

It does look correct as it is, but its just more effort to review.
Small, simple, obviously correct patches are what we want.

	Andrew

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

[Niklas Söderlund]

On 2025-09-16 16:49:33 +0200, Andrew Lunn wrote:
> On Tue, Sep 16, 2025 at 03:08:48PM +0200, Niklas Söderlund wrote:
> > On 2025-09-16 14:38:58 +0200, Andrew Lunn wrote:
> > > > @@ -1010,18 +1009,27 @@ 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);
> > > > +
> > > > +			if (priv->tstamp_rx_ctrl != HWTSTAMP_FILTER_NONE) {
> > > > +				bool get_ts = false;
> > > > +
> > > > +				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) {
> > > > +					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);
> > > > +				}
> > > 
> > > This hunk is bigger than it needs to be because this block has been
> > > indented further. Maybe keep get_ts as function scope, initialised to
> > > false, so you don't need to touch this block?
> > 
> > Thanks for the suggestion. I could do that. What I like about this is 
> > that it's immediately clear that all this depends on 
> > priv->tstamp_rx_ctrl.
> 
> True.
> 
> As a reviewer, i was asking myself, has the code actually setting the
> timestamp in the skbuf changed? Do i need to look at it in detail?
> There should not be any need for it to change....
> 
> > I could break it out to a separate function if you prefer to reduce the 
> > indentation level,
> 
> It is not really indentation level, but the fact it is in the hunk,
> meaning should i look at it?
> 
> So maybe add a patch which moves the copying of the timestamp from the
> descriptor into the skbuf into a helper. This patch then just calls
> the helper, making this hunk much smaller and more obvious?
> 
> It does look correct as it is, but its just more effort to review.
> Small, simple, obviously correct patches are what we want.

I'm with you on creating small and easy to review patches. I will do as 
you suggest and move this out to a function in a separate patch. Thanks 
for the feedback.

> 
> 	Andrew

-- 
Kind Regards,
Niklas Söderlund