Currently all RX frames are timestamped which results in a performance
penalty when timestamping is not needed. The default is now being
changed to not timestamp any Rx frames (HWTSTAMP_FILTER_NONE), but
support has been added to allow changing the desired RX timestamping
mode (HWTSTAMP_FILTER_ALL - which was the previous setting,
HWTSTAMP_FILTER_PTP_V2_EVENT, HWTSTAMP_FILTER_PTP_V2_SYNC and
HWTSTAMP_FILTER_PTP_V2_DELAY_REQ are the supported options) using
SIOCSHWTSTAMP. All settings were tested using the hwstamp_ctl application.
It is also noted that ptp4l, when started, preconfigures the device to
timestamp using HWTSTAMP_FILTER_PTP_V2_EVENT, so this driver continues
to work properly "out of the box".
Test setup: x64 PC with LAN7430 ---> x64 PC as partner
iperf3 with - Timestamp all incoming packets:
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval Transfer Bitrate Retr
[ 5] 0.00-5.05 sec 517 MBytes 859 Mbits/sec 0 sender
[ 5] 0.00-5.00 sec 515 MBytes 864 Mbits/sec receiver
iperf Done.
iperf3 with - Timestamp only PTP packets:
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval Transfer Bitrate Retr
[ 5] 0.00-5.04 sec 563 MBytes 937 Mbits/sec 0 sender
[ 5] 0.00-5.00 sec 561 MBytes 941 Mbits/sec receiver
Please find the earlier conversation at the link below
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Vishvambar Panth S <[email protected]>
---
.../net/ethernet/microchip/lan743x_ethtool.c | 5 +-
drivers/net/ethernet/microchip/lan743x_main.c | 58 ++++++++++++++++++-
drivers/net/ethernet/microchip/lan743x_main.h | 8 +++
drivers/net/ethernet/microchip/lan743x_ptp.c | 9 +++
4 files changed, 78 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index 2db5949b4c7e..855844df5ea1 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -1047,7 +1047,10 @@ static int lan743x_ethtool_get_ts_info(struct net_device *netdev,
BIT(HWTSTAMP_TX_ON) |
BIT(HWTSTAMP_TX_ONESTEP_SYNC);
ts_info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
- BIT(HWTSTAMP_FILTER_ALL);
+ BIT(HWTSTAMP_FILTER_ALL) |
+ BIT(HWTSTAMP_FILTER_PTP_V2_EVENT) |
+ BIT(HWTSTAMP_FILTER_PTP_V2_SYNC) |
+ BIT(HWTSTAMP_FILTER_PTP_V2_DELAY_REQ);
return 0;
}
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index f940895b14e8..0389bc7cf603 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1870,6 +1870,63 @@ static int lan743x_tx_get_avail_desc(struct lan743x_tx *tx)
return last_head - last_tail - 1;
}
+int lan743x_rx_set_tstamp_mode(struct lan743x_adapter *adapter,
+ int rx_filter)
+{
+ int channel_number;
+ int index;
+ u32 data;
+
+ switch (rx_filter) {
+ case HWTSTAMP_FILTER_PTP_V2_SYNC:
+ data = lan743x_csr_read(adapter, PTP_RX_TS_CFG);
+ data &= ~PTP_RX_TS_CFG_EVENT_MSGS_;
+ data |= PTP_RX_TS_CFG_SYNC_MSG_;
+ lan743x_csr_write(adapter, PTP_RX_TS_CFG, data);
+ break;
+ case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+ data = lan743x_csr_read(adapter, PTP_RX_TS_CFG);
+ data &= ~PTP_RX_TS_CFG_EVENT_MSGS_;
+ data |= PTP_RX_TS_CFG_DELAY_REQ_MSG_;
+ lan743x_csr_write(adapter, PTP_RX_TS_CFG, data);
+ break;
+ case HWTSTAMP_FILTER_PTP_V2_EVENT:
+ data = lan743x_csr_read(adapter, PTP_RX_TS_CFG);
+ data |= PTP_RX_TS_CFG_EVENT_MSGS_;
+ lan743x_csr_write(adapter, PTP_RX_TS_CFG, data);
+ break;
+ case HWTSTAMP_FILTER_NONE:
+ case HWTSTAMP_FILTER_ALL:
+ break;
+ default:
+ netif_warn(adapter, drv, adapter->netdev,
+ "rx timestamp = %d is not supported\n",
+ rx_filter);
+ return -EINVAL;
+ }
+
+ for (index = 0; index < LAN743X_USED_RX_CHANNELS; index++) {
+ channel_number = adapter->rx[index].channel_number;
+ data = lan743x_csr_read(adapter, RX_CFG_B(channel_number));
+ if (rx_filter == HWTSTAMP_FILTER_NONE) {
+ data &= ~(RX_CFG_B_TS_ALL_RX_ |
+ RX_CFG_B_TS_DESCR_EN_);
+ } else if (rx_filter == HWTSTAMP_FILTER_ALL) {
+ data |= RX_CFG_B_TS_ALL_RX_;
+ } else {
+ /* enable storing timestamping in extension descriptor
+ * instead of timestamping all the packets
+ */
+ data &= ~RX_CFG_B_TS_ALL_RX_;
+ data |= RX_CFG_B_TS_DESCR_EN_;
+ }
+ lan743x_csr_write(adapter, RX_CFG_B(channel_number),
+ data);
+ }
+
+ return 0;
+}
+
void lan743x_tx_set_timestamping_mode(struct lan743x_tx *tx,
bool enable_timestamping,
bool enable_onestep_sync)
@@ -2944,7 +3001,6 @@ static int lan743x_rx_open(struct lan743x_rx *rx)
data |= RX_CFG_B_RX_PAD_2_;
data &= ~RX_CFG_B_RX_RING_LEN_MASK_;
data |= ((rx->ring_size) & RX_CFG_B_RX_RING_LEN_MASK_);
- data |= RX_CFG_B_TS_ALL_RX_;
if (!(adapter->csr.flags & LAN743X_CSR_FLAG_IS_A0))
data |= RX_CFG_B_RDMABL_512_;
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 52609fc13ad9..b0ddc32a2d85 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -522,6 +522,10 @@
(((u32)(rx_latency)) & 0x0000FFFF)
#define PTP_CAP_INFO (0x0A60)
#define PTP_CAP_INFO_TX_TS_CNT_GET_(reg_val) (((reg_val) & 0x00000070) >> 4)
+#define PTP_RX_TS_CFG (0x0A68)
+#define PTP_RX_TS_CFG_SYNC_MSG_ BIT(0)
+#define PTP_RX_TS_CFG_DELAY_REQ_MSG_ BIT(1)
+#define PTP_RX_TS_CFG_EVENT_MSGS_ GENMASK(3, 0)
#define PTP_TX_MOD (0x0AA4)
#define PTP_TX_MOD_TX_PTP_SYNC_TS_INSERT_ (0x10000000)
@@ -657,6 +661,7 @@
#define RX_CFG_B(channel) (0xC44 + ((channel) << 6))
#define RX_CFG_B_TS_ALL_RX_ BIT(29)
+#define RX_CFG_B_TS_DESCR_EN_ BIT(28)
#define RX_CFG_B_RX_PAD_MASK_ (0x03000000)
#define RX_CFG_B_RX_PAD_0_ (0x00000000)
#define RX_CFG_B_RX_PAD_2_ (0x02000000)
@@ -991,6 +996,9 @@ struct lan743x_rx {
struct sk_buff *skb_head, *skb_tail;
};
+int lan743x_rx_set_tstamp_mode(struct lan743x_adapter *adapter,
+ int rx_filter);
+
/* SGMII Link Speed Duplex status */
enum lan743x_sgmii_lsd {
POWER_DOWN = 0,
diff --git a/drivers/net/ethernet/microchip/lan743x_ptp.c b/drivers/net/ethernet/microchip/lan743x_ptp.c
index 39e1066ecd5f..2f04bc77a118 100644
--- a/drivers/net/ethernet/microchip/lan743x_ptp.c
+++ b/drivers/net/ethernet/microchip/lan743x_ptp.c
@@ -1493,6 +1493,10 @@ int lan743x_ptp_open(struct lan743x_adapter *adapter)
temp = lan743x_csr_read(adapter, PTP_TX_MOD2);
temp |= PTP_TX_MOD2_TX_PTP_CLR_UDPV4_CHKSUM_;
lan743x_csr_write(adapter, PTP_TX_MOD2, temp);
+
+ /* Default Timestamping */
+ lan743x_rx_set_tstamp_mode(adapter, HWTSTAMP_FILTER_NONE);
+
lan743x_ptp_enable(adapter);
lan743x_csr_write(adapter, INT_EN_SET, INT_BIT_1588_);
lan743x_csr_write(adapter, PTP_INT_EN_SET,
@@ -1653,6 +1657,9 @@ static void lan743x_ptp_disable(struct lan743x_adapter *adapter)
{
struct lan743x_ptp *ptp = &adapter->ptp;
+ /* Disable Timestamping */
+ lan743x_rx_set_tstamp_mode(adapter, HWTSTAMP_FILTER_NONE);
+
mutex_lock(&ptp->command_lock);
if (!lan743x_ptp_is_enabled(adapter)) {
netif_warn(adapter, drv, adapter->netdev,
@@ -1785,6 +1792,8 @@ int lan743x_ptp_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
break;
}
+ ret = lan743x_rx_set_tstamp_mode(adapter, config.rx_filter);
+
if (!ret)
return copy_to_user(ifr->ifr_data, &config,
sizeof(config)) ? -EFAULT : 0;
--
2.25.1
On 9/26/2023 8:56 AM, Vishvambar Panth S wrote:
> Currently all RX frames are timestamped which results in a performance
> penalty when timestamping is not needed. The default is now being
> changed to not timestamp any Rx frames (HWTSTAMP_FILTER_NONE), but
> support has been added to allow changing the desired RX timestamping
> mode (HWTSTAMP_FILTER_ALL - which was the previous setting,
> HWTSTAMP_FILTER_PTP_V2_EVENT, HWTSTAMP_FILTER_PTP_V2_SYNC and
> HWTSTAMP_FILTER_PTP_V2_DELAY_REQ are the supported options) using
> SIOCSHWTSTAMP. All settings were tested using the hwstamp_ctl application.
> It is also noted that ptp4l, when started, preconfigures the device to
> timestamp using HWTSTAMP_FILTER_PTP_V2_EVENT, so this driver continues
> to work properly "out of the box".
>
> Test setup: x64 PC with LAN7430 ---> x64 PC as partner
>
I don't think I would bother to support HWTSTAMP_FILTER_PTP_V2_DELAY_REQ
or HWTSTAMP_FILTER_PTP_V2_SYNC as these are pretty historic and only
useful for hardware which can't do HWTSTAMP_FILTER_PTP_V2_EVENT.
> iperf3 with - Timestamp all incoming packets:
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval Transfer Bitrate Retr
> [ 5] 0.00-5.05 sec 517 MBytes 859 Mbits/sec 0 sender
> [ 5] 0.00-5.00 sec 515 MBytes 864 Mbits/sec receiver
>
> iperf Done.
>
> iperf3 with - Timestamp only PTP packets:
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval Transfer Bitrate Retr
> [ 5] 0.00-5.04 sec 563 MBytes 937 Mbits/sec 0 sender
> [ 5] 0.00-5.00 sec 561 MBytes 941 Mbits/sec receiver
>
>
Pretty significant cost here for the timestamping all frames. Makes
sense to leave the default to NONE unless requested.
Please find the earlier conversation at the link below
> Link: https://lore.kernel.org/all/[email protected]/
>
> Signed-off-by: Vishvambar Panth S <[email protected]>
> ---
> .../net/ethernet/microchip/lan743x_ethtool.c | 5 +-
> drivers/net/ethernet/microchip/lan743x_main.c | 58 ++++++++++++++++++-
> drivers/net/ethernet/microchip/lan743x_main.h | 8 +++
> drivers/net/ethernet/microchip/lan743x_ptp.c | 9 +++
> 4 files changed, 78 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> index 2db5949b4c7e..855844df5ea1 100644
> --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> @@ -1047,7 +1047,10 @@ static int lan743x_ethtool_get_ts_info(struct net_device *netdev,
> BIT(HWTSTAMP_TX_ON) |
> BIT(HWTSTAMP_TX_ONESTEP_SYNC);
> ts_info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> - BIT(HWTSTAMP_FILTER_ALL);
> + BIT(HWTSTAMP_FILTER_ALL) |
> + BIT(HWTSTAMP_FILTER_PTP_V2_EVENT) |
> + BIT(HWTSTAMP_FILTER_PTP_V2_SYNC) |
> + BIT(HWTSTAMP_FILTER_PTP_V2_DELAY_REQ);
> return 0;
> }
>
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index f940895b14e8..0389bc7cf603 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -1870,6 +1870,63 @@ static int lan743x_tx_get_avail_desc(struct lan743x_tx *tx)
> return last_head - last_tail - 1;
> }
>
> +int lan743x_rx_set_tstamp_mode(struct lan743x_adapter *adapter,
> + int rx_filter)
> +{
> + int channel_number;
> + int index;
> + u32 data;
> +
> + switch (rx_filter) {
> + case HWTSTAMP_FILTER_PTP_V2_SYNC:
> + data = lan743x_csr_read(adapter, PTP_RX_TS_CFG);
> + data &= ~PTP_RX_TS_CFG_EVENT_MSGS_;
> + data |= PTP_RX_TS_CFG_SYNC_MSG_;
> + lan743x_csr_write(adapter, PTP_RX_TS_CFG, data);
> + break;
> + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> + data = lan743x_csr_read(adapter, PTP_RX_TS_CFG);
> + data &= ~PTP_RX_TS_CFG_EVENT_MSGS_;
> + data |= PTP_RX_TS_CFG_DELAY_REQ_MSG_;
> + lan743x_csr_write(adapter, PTP_RX_TS_CFG, data);
> + break;
> + case HWTSTAMP_FILTER_PTP_V2_EVENT:
> + data = lan743x_csr_read(adapter, PTP_RX_TS_CFG);
> + data |= PTP_RX_TS_CFG_EVENT_MSGS_;
> + lan743x_csr_write(adapter, PTP_RX_TS_CFG, data);
> + break;
> + case HWTSTAMP_FILTER_NONE:
> + case HWTSTAMP_FILTER_ALL:
> + break;
At first this break was a bit confusing to me, since nothing is set here.
> + default:
> + netif_warn(adapter, drv, adapter->netdev,
> + "rx timestamp = %d is not supported\n",
> + rx_filter);
> + return -EINVAL;
> + }
> +
> + for (index = 0; index < LAN743X_USED_RX_CHANNELS; index++) {
> + channel_number = adapter->rx[index].channel_number;
> + data = lan743x_csr_read(adapter, RX_CFG_B(channel_number));
> + if (rx_filter == HWTSTAMP_FILTER_NONE) {
> + data &= ~(RX_CFG_B_TS_ALL_RX_ |
> + RX_CFG_B_TS_DESCR_EN_);
> + } else if (rx_filter == HWTSTAMP_FILTER_ALL) {
> + data |= RX_CFG_B_TS_ALL_RX_;
> + } else {
> + /* enable storing timestamping in extension descriptor
> + * instead of timestamping all the packets
> + */
> + data &= ~RX_CFG_B_TS_ALL_RX_;
> + data |= RX_CFG_B_TS_DESCR_EN_;
> + }
I might have made the decision of what to program in the switch case
above and then done the write here with "data &= ~MASK; data |= setting"
rather than having two separate decision points.
> + lan743x_csr_write(adapter, RX_CFG_B(channel_number),
> + data);
> + }
> +
> + return 0;
> +}
> +
Rest of the code seems fine, and the implementation looks ok.
I'd suggest dropping the not so useful timestamp filters for only sync
or only delay request, keeping only V2_EVENT, but either way:
Reviewed-by: Jacob Keller <[email protected]>
On Tue, 26 Sep 2023 21:26:58 +0530 Vishvambar Panth S wrote:
> + netif_warn(adapter, drv, adapter->netdev,
> + "rx timestamp = %d is not supported\n",
> + rx_filter);
I addition to Jake's comments please also drop this warning.
> + return -EINVAL;
And make sure this is the correct return code.
I thought -ERANGE was more appropriate here, but I could be wrong.
On Wed, 2023-10-04 at 11:00 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Tue, 26 Sep 2023 21:26:58 +0530 Vishvambar Panth S wrote:
> > + netif_warn(adapter, drv, adapter->netdev,
> > + "rx timestamp = %d is not
> > supported\n",
> > + rx_filter);
>
> I addition to Jake's comments please also drop this warning.
>
Hi Jakub,
Thanks for the comments. Sure, will drop the warning or add it as debug
message.
> > + return -EINVAL;
>
> And make sure this is the correct return code.
> I thought -ERANGE was more appropriate here, but I could be wrong.
Will check and add the appropriate return code in v2 patch.
On Tue, 2023-09-26 at 16:48 -0700, Jacob Keller wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On 9/26/2023 8:56 AM, Vishvambar Panth S wrote:
> > Currently all RX frames are timestamped which results in a
> > performance
> > penalty when timestamping is not needed. The default is now being
> > changed to not timestamp any Rx frames (HWTSTAMP_FILTER_NONE), but
> > support has been added to allow changing the desired RX
> > timestamping
> > mode (HWTSTAMP_FILTER_ALL - which was the previous setting,
> > HWTSTAMP_FILTER_PTP_V2_EVENT, HWTSTAMP_FILTER_PTP_V2_SYNC and
> > HWTSTAMP_FILTER_PTP_V2_DELAY_REQ are the supported options) using
> > SIOCSHWTSTAMP. All settings were tested using the hwstamp_ctl
> > application.
> > It is also noted that ptp4l, when started, preconfigures the device
> > to
> > timestamp using HWTSTAMP_FILTER_PTP_V2_EVENT, so this driver
> > continues
> > to work properly "out of the box".
> >
> > Test setup: x64 PC with LAN7430 ---> x64 PC as partner
> >
>
> I don't think I would bother to support
> HWTSTAMP_FILTER_PTP_V2_DELAY_REQ
> or HWTSTAMP_FILTER_PTP_V2_SYNC as these are pretty historic and only
> useful for hardware which can't do HWTSTAMP_FILTER_PTP_V2_EVENT.
>
Hi Jacob,
Thanks for the comments. Just added these filters, as the HW supports
these individually. Since HW also supports
HWTSTAMP_FILTER_PTP_V2_EVENT, will drop the support for
individual HWTSTAMP_FILTER_PTP_V2_SYNC and
HWTSTAMP_FILTER_PTP_V2_DELAY_REQ cases in v2 patch.
> > iperf3 with - Timestamp all incoming packets:
> > - - - - - - - - - - - - - - - - - - - - - - - - -
> > [ ID] Interval Transfer Bitrate Retr
> > [ 5] 0.00-5.05 sec 517 MBytes 859 Mbits/sec 0 sender
> > [ 5] 0.00-5.00 sec 515 MBytes 864 Mbits/sec
> > receiver
> >
> > iperf Done.
> >
> > iperf3 with - Timestamp only PTP packets:
> > - - - - - - - - - - - - - - - - - - - - - - - - -
> > [ ID] Interval Transfer Bitrate Retr
> > [ 5] 0.00-5.04 sec 563 MBytes 937 Mbits/sec 0 sender
> > [ 5] 0.00-5.00 sec 561 MBytes 941 Mbits/sec
> > receiver
> >
> >
>
> Pretty significant cost here for the timestamping all frames. Makes
> sense to leave the default to NONE unless requested.
Thanks for the comment, will maintain as is.
>
> Please find the earlier conversation at the link below
> > Link:
> > https://lore.kernel.org/all/[email protected]/
> >
> > Signed-off-by: Vishvambar Panth S <[email protected]>
> > ---
> > .../net/ethernet/microchip/lan743x_ethtool.c | 5 +-
> > drivers/net/ethernet/microchip/lan743x_main.c | 58
> > ++++++++++++++++++-
> > drivers/net/ethernet/microchip/lan743x_main.h | 8 +++
> > drivers/net/ethernet/microchip/lan743x_ptp.c | 9 +++
> > 4 files changed, 78 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > index 2db5949b4c7e..855844df5ea1 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > @@ -1047,7 +1047,10 @@ static int
> > lan743x_ethtool_get_ts_info(struct net_device *netdev,
> > BIT(HWTSTAMP_TX_ON) |
> > BIT(HWTSTAMP_TX_ONESTEP_SYNC);
> > ts_info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> > - BIT(HWTSTAMP_FILTER_ALL);
> > + BIT(HWTSTAMP_FILTER_ALL) |
> > + BIT(HWTSTAMP_FILTER_PTP_V2_EVENT) |
> > + BIT(HWTSTAMP_FILTER_PTP_V2_SYNC) |
> > + BIT(HWTSTAMP_FILTER_PTP_V2_DELAY_REQ);
> > return 0;
> > }
> >
> > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c
> > b/drivers/net/ethernet/microchip/lan743x_main.c
> > index f940895b14e8..0389bc7cf603 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> > @@ -1870,6 +1870,63 @@ static int lan743x_tx_get_avail_desc(struct
> > lan743x_tx *tx)
> > return last_head - last_tail - 1;
> > }
> >
> > +int lan743x_rx_set_tstamp_mode(struct lan743x_adapter *adapter,
> > + int rx_filter)
> > +{
> > + int channel_number;
> > + int index;
> > + u32 data;
> > +
> > + switch (rx_filter) {
> > + case HWTSTAMP_FILTER_PTP_V2_SYNC:
> > + data = lan743x_csr_read(adapter,
> > PTP_RX_TS_CFG);
> > + data &= ~PTP_RX_TS_CFG_EVENT_MSGS_;
> > + data |= PTP_RX_TS_CFG_SYNC_MSG_;
> > + lan743x_csr_write(adapter, PTP_RX_TS_CFG,
> > data);
> > + break;
> > + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> > + data = lan743x_csr_read(adapter,
> > PTP_RX_TS_CFG);
> > + data &= ~PTP_RX_TS_CFG_EVENT_MSGS_;
> > + data |= PTP_RX_TS_CFG_DELAY_REQ_MSG_;
> > + lan743x_csr_write(adapter, PTP_RX_TS_CFG,
> > data);
> > + break;
> > + case HWTSTAMP_FILTER_PTP_V2_EVENT:
> > + data = lan743x_csr_read(adapter,
> > PTP_RX_TS_CFG);
> > + data |= PTP_RX_TS_CFG_EVENT_MSGS_;
> > + lan743x_csr_write(adapter, PTP_RX_TS_CFG,
> > data);
> > + break;
> > + case HWTSTAMP_FILTER_NONE:
> > + case HWTSTAMP_FILTER_ALL:
> > + break;
>
> At first this break was a bit confusing to me, since nothing is set
> here.
The idea was that in the NONE and ALL case, there need not be any
modification in this register. But will adapt to a more readable
implementation in v2 patch.
>
> > + default:
> > + netif_warn(adapter, drv, adapter->netdev,
> > + "rx timestamp = %d is not
> > supported\n",
> > + rx_filter);
> > + return -EINVAL;
> > + }
> > +
> > + for (index = 0; index < LAN743X_USED_RX_CHANNELS; index++) {
> > + channel_number = adapter->rx[index].channel_number;
> > + data = lan743x_csr_read(adapter,
> > RX_CFG_B(channel_number));
> > + if (rx_filter == HWTSTAMP_FILTER_NONE) {
> > + data &= ~(RX_CFG_B_TS_ALL_RX_ |
> > + RX_CFG_B_TS_DESCR_EN_);
> > + } else if (rx_filter == HWTSTAMP_FILTER_ALL) {
> > + data |= RX_CFG_B_TS_ALL_RX_;
> > + } else {
> > + /* enable storing timestamping in extension
> > descriptor
> > + * instead of timestamping all the packets
> > + */
> > + data &= ~RX_CFG_B_TS_ALL_RX_;
> > + data |= RX_CFG_B_TS_DESCR_EN_;
> > + }
>
> I might have made the decision of what to program in the switch case
> above and then done the write here with "data &= ~MASK; data |=
> setting"
> rather than having two separate decision points.
Sure, will try and adapt to this.
>
> > + lan743x_csr_write(adapter, RX_CFG_B(channel_number),
> > + data);
> > + }
> > +
> > + return 0;
> > +}
> > +
>
> Rest of the code seems fine, and the implementation looks ok.
>
> I'd suggest dropping the not so useful timestamp filters for only
> sync
> or only delay request, keeping only V2_EVENT, but either way:
Thanks, will drop the not so useful filters and submit a v2 patch with
required changes.
>
> Reviewed-by: Jacob Keller <[email protected]>