If PTP is enabled in the hardware, then 4 bytes are added in the tail
tag. When PTP is enabled and 4 bytes are not added then messages are
corrupted.
Signed-off-by: Arun Ramadoss <[email protected]>
---
Patch v1
- Patch is new
---
MAINTAINERS | 1 +
drivers/net/dsa/microchip/ksz_common.h | 1 -
drivers/net/dsa/microchip/ksz_ptp.c | 24 +++++--
include/linux/dsa/ksz_common.h | 23 ++++++
net/dsa/tag_ksz.c | 99 ++++++++++++++++++++++++--
5 files changed, 136 insertions(+), 12 deletions(-)
create mode 100644 include/linux/dsa/ksz_common.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 61fe86968111..721d2c5dfa46 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13515,6 +13515,7 @@ S: Maintained
F: Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
F: Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
F: drivers/net/dsa/microchip/*
+F: include/linux/dsa/ksz_common.h
F: include/linux/platform_data/microchip-ksz.h
F: net/dsa/tag_ksz.c
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index cd20f39a565f..4c5b35a7883c 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -105,7 +105,6 @@ struct ksz_port {
u8 num;
#if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ_PTP)
u8 hwts_tx_en;
- bool hwts_rx_en;
#endif
};
diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index a41418c6adf6..184aa57a8489 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -3,6 +3,7 @@
* Copyright (C) 2022 Microchip Technology Inc.
*/
+#include <linux/dsa/ksz_common.h>
#include <linux/kernel.h>
#include <linux/ptp_classify.h>
#include <linux/ptp_clock_kernel.h>
@@ -19,6 +20,16 @@
#define KSZ_PTP_INC_NS 40 /* HW clock is incremented every 40 ns (by 40) */
#define KSZ_PTP_SUBNS_BITS 32
+static int ksz_ptp_enable_mode(struct ksz_device *dev, bool enable)
+{
+ u16 data = 0;
+
+ if (enable)
+ data = PTP_ENABLE;
+
+ return ksz_rmw16(dev, REG_PTP_MSG_CONF1, PTP_ENABLE, data);
+}
+
/* The function is return back the capability of timestamping feature when
* requested through ethtool -T <interface> utility
*/
@@ -47,6 +58,7 @@ int ksz_get_ts_info(struct dsa_switch *ds, int port, struct ethtool_ts_info *ts)
int ksz_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
{
+ struct ksz_tagger_data *tagger_data = ksz_tagger_data(ds);
struct ksz_device *dev = ds->priv;
struct hwtstamp_config config;
@@ -54,7 +66,7 @@ int ksz_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
config.tx_type = dev->ports[port].hwts_tx_en;
- if (dev->ports[port].hwts_rx_en)
+ if (tagger_data->hwtstamp_get_state(ds))
config.rx_filter = HWTSTAMP_FILTER_ALL;
else
config.rx_filter = HWTSTAMP_FILTER_NONE;
@@ -66,7 +78,9 @@ int ksz_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
static int ksz_set_hwtstamp_config(struct ksz_device *dev, int port,
struct hwtstamp_config *config)
{
+ struct ksz_tagger_data *tagger_data = ksz_tagger_data(dev->ds);
struct ksz_port *prt = &dev->ports[port];
+ bool rx_on;
if (config->flags)
return -EINVAL;
@@ -82,14 +96,16 @@ static int ksz_set_hwtstamp_config(struct ksz_device *dev, int port,
switch (config->rx_filter) {
case HWTSTAMP_FILTER_NONE:
- prt->hwts_rx_en = false;
+ rx_on = false;
break;
default:
- prt->hwts_rx_en = true;
+ rx_on = true;
break;
}
- return 0;
+ tagger_data->hwtstamp_set_state(dev->ds, rx_on);
+
+ return ksz_ptp_enable_mode(dev, rx_on);
}
int ksz_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
diff --git a/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
new file mode 100644
index 000000000000..62996860b887
--- /dev/null
+++ b/include/linux/dsa/ksz_common.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Microchip switch tag common header
+ *
+ * Copyright (C) 2022 Microchip Technology Inc.
+ */
+
+#ifndef _NET_DSA_KSZ_COMMON_H_
+#define _NET_DSA_KSZ_COMMON_H_
+
+#include <net/dsa.h>
+
+struct ksz_tagger_data {
+ bool (*hwtstamp_get_state)(struct dsa_switch *ds);
+ void (*hwtstamp_set_state)(struct dsa_switch *ds, bool on);
+};
+
+static inline struct ksz_tagger_data *
+ksz_tagger_data(struct dsa_switch *ds)
+{
+ return ds->tagger_data;
+}
+
+#endif /* _NET_DSA_KSZ_COMMON_H_ */
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 0f6ae143afc9..828af38f0598 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -4,6 +4,7 @@
* Copyright (c) 2017 Microchip Technology
*/
+#include <linux/dsa/ksz_common.h>
#include <linux/etherdevice.h>
#include <linux/list.h>
#include <net/dsa.h>
@@ -16,9 +17,66 @@
#define LAN937X_NAME "lan937x"
/* Typically only one byte is used for tail tag. */
+#define KSZ_PTP_TAG_LEN 4
#define KSZ_EGRESS_TAG_LEN 1
#define KSZ_INGRESS_TAG_LEN 1
+#define KSZ_HWTS_EN 0
+
+struct ksz_tagger_private {
+ struct ksz_tagger_data data; /* Must be first */
+ unsigned long state;
+};
+
+static struct ksz_tagger_private *
+ksz_tagger_private(struct dsa_switch *ds)
+{
+ return ds->tagger_data;
+}
+
+static bool ksz_hwtstamp_get_state(struct dsa_switch *ds)
+{
+ struct ksz_tagger_private *priv = ksz_tagger_private(ds);
+
+ return test_bit(KSZ_HWTS_EN, &priv->state);
+}
+
+static void ksz_hwtstamp_set_state(struct dsa_switch *ds, bool on)
+{
+ struct ksz_tagger_private *priv = ksz_tagger_private(ds);
+
+ if (on)
+ set_bit(KSZ_HWTS_EN, &priv->state);
+ else
+ clear_bit(KSZ_HWTS_EN, &priv->state);
+}
+
+static void ksz_disconnect(struct dsa_switch *ds)
+{
+ struct ksz_tagger_private *priv = ds->tagger_data;
+
+ kfree(priv);
+ ds->tagger_data = NULL;
+}
+
+static int ksz_connect(struct dsa_switch *ds)
+{
+ struct ksz_tagger_data *tagger_data;
+ struct ksz_tagger_private *priv;
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ /* Export functions for switch driver use */
+ tagger_data = &priv->data;
+ tagger_data->hwtstamp_get_state = ksz_hwtstamp_get_state;
+ tagger_data->hwtstamp_set_state = ksz_hwtstamp_set_state;
+ ds->tagger_data = priv;
+
+ return 0;
+}
+
static struct sk_buff *ksz_common_rcv(struct sk_buff *skb,
struct net_device *dev,
unsigned int port, unsigned int len)
@@ -91,10 +149,11 @@ DSA_TAG_DRIVER(ksz8795_netdev_ops);
MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ8795, KSZ8795_NAME);
/*
- * For Ingress (Host -> KSZ9477), 2 bytes are added before FCS.
+ * For Ingress (Host -> KSZ9477), 2/6 bytes are added before FCS.
* ---------------------------------------------------------------------------
- * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
+ * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|ts(4bytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
* ---------------------------------------------------------------------------
+ * ts : time stamp (Present only if PTP is enabled in the Hardware)
* tag0 : Prioritization (not used now)
* tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x10=port5)
*
@@ -113,6 +172,19 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ8795, KSZ8795_NAME);
#define KSZ9477_TAIL_TAG_OVERRIDE BIT(9)
#define KSZ9477_TAIL_TAG_LOOKUP BIT(10)
+/* Time stamp tag is only inserted if PTP is enabled in hardware. */
+static void ksz_xmit_timestamp(struct dsa_port *dp, struct sk_buff *skb)
+{
+ struct ksz_tagger_private *priv;
+
+ priv = ksz_tagger_private(dp->ds);
+
+ if (!test_bit(KSZ_HWTS_EN, &priv->state))
+ return;
+
+ put_unaligned_be32(0, skb_put(skb, KSZ_PTP_TAG_LEN));
+}
+
static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
struct net_device *dev)
{
@@ -125,6 +197,8 @@ static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
return NULL;
/* Tag encoding */
+ ksz_xmit_timestamp(dp, skb);
+
tag = skb_put(skb, KSZ9477_INGRESS_TAG_LEN);
addr = skb_mac_header(skb);
@@ -157,7 +231,9 @@ static const struct dsa_device_ops ksz9477_netdev_ops = {
.proto = DSA_TAG_PROTO_KSZ9477,
.xmit = ksz9477_xmit,
.rcv = ksz9477_rcv,
- .needed_tailroom = KSZ9477_INGRESS_TAG_LEN,
+ .connect = ksz_connect,
+ .disconnect = ksz_disconnect,
+ .needed_tailroom = KSZ9477_INGRESS_TAG_LEN + KSZ_PTP_TAG_LEN,
};
DSA_TAG_DRIVER(ksz9477_netdev_ops);
@@ -177,6 +253,8 @@ static struct sk_buff *ksz9893_xmit(struct sk_buff *skb,
return NULL;
/* Tag encoding */
+ ksz_xmit_timestamp(dp, skb);
+
tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
addr = skb_mac_header(skb);
@@ -193,16 +271,19 @@ static const struct dsa_device_ops ksz9893_netdev_ops = {
.proto = DSA_TAG_PROTO_KSZ9893,
.xmit = ksz9893_xmit,
.rcv = ksz9477_rcv,
- .needed_tailroom = KSZ_INGRESS_TAG_LEN,
+ .connect = ksz_connect,
+ .disconnect = ksz_disconnect,
+ .needed_tailroom = KSZ_INGRESS_TAG_LEN + KSZ_PTP_TAG_LEN,
};
DSA_TAG_DRIVER(ksz9893_netdev_ops);
MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9893, KSZ9893_NAME);
-/* For xmit, 2 bytes are added before FCS.
+/* For xmit, 2/6 bytes are added before FCS.
* ---------------------------------------------------------------------------
- * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
+ * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|ts(4bytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
* ---------------------------------------------------------------------------
+ * ts : time stamp (Present only if PTP is enabled in the Hardware)
* tag0 : represents tag override, lookup and valid
* tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x80=port8)
*
@@ -231,6 +312,8 @@ static struct sk_buff *lan937x_xmit(struct sk_buff *skb,
if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
return NULL;
+ ksz_xmit_timestamp(dp, skb);
+
tag = skb_put(skb, LAN937X_EGRESS_TAG_LEN);
val = BIT(dp->index);
@@ -251,7 +334,9 @@ static const struct dsa_device_ops lan937x_netdev_ops = {
.proto = DSA_TAG_PROTO_LAN937X,
.xmit = lan937x_xmit,
.rcv = ksz9477_rcv,
- .needed_tailroom = LAN937X_EGRESS_TAG_LEN,
+ .connect = ksz_connect,
+ .disconnect = ksz_disconnect,
+ .needed_tailroom = LAN937X_EGRESS_TAG_LEN + KSZ_PTP_TAG_LEN,
};
DSA_TAG_DRIVER(lan937x_netdev_ops);
--
2.36.1
On Mon, Nov 28, 2022 at 04:02:18PM +0530, Arun Ramadoss wrote:
> If PTP is enabled in the hardware, then 4 bytes are added in the tail
> tag. When PTP is enabled and 4 bytes are not added then messages are
> corrupted.
Comment in the code please. Also, please spell it out explicitly that
the tail tag size changes for all TX packets, PTP or not, if PTP
timestamping is enabled. Your phrasing can be unclear and the reader may
think that only PTP packets require a larger tail tag.
>
> Signed-off-by: Arun Ramadoss <[email protected]>
> ---
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index cd20f39a565f..4c5b35a7883c 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -105,7 +105,6 @@ struct ksz_port {
> u8 num;
> #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ_PTP)
> u8 hwts_tx_en;
> - bool hwts_rx_en;
> #endif
> };
>
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index a41418c6adf6..184aa57a8489 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -54,7 +66,7 @@ int ksz_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
>
> config.tx_type = dev->ports[port].hwts_tx_en;
>
> - if (dev->ports[port].hwts_rx_en)
> + if (tagger_data->hwtstamp_get_state(ds))
Let's be clear, hwtstamp_get_state() deals with TX timestamping, and
config.rx_filter deals with RX timestamping. Don't mix the two.
Using custom programs like testptp, you can enable RX timestamping but
not TX timestamping, or the other way around. You don't want the driver
to get confused.
> config.rx_filter = HWTSTAMP_FILTER_ALL;
Can the switch provide RX timestamps for all kinds of Ethernet packets,
not just PTP? If not, then report just what it can timestamp.
> else
> config.rx_filter = HWTSTAMP_FILTER_NONE;
> int ksz_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> index 0f6ae143afc9..828af38f0598 100644
> --- a/net/dsa/tag_ksz.c
> +++ b/net/dsa/tag_ksz.c
> @@ -4,6 +4,7 @@
> * Copyright (c) 2017 Microchip Technology
> */
>
> +#include <linux/dsa/ksz_common.h>
> #include <linux/etherdevice.h>
> #include <linux/list.h>
> #include <net/dsa.h>
> @@ -16,9 +17,66 @@
> #define LAN937X_NAME "lan937x"
>
> /* Typically only one byte is used for tail tag. */
> +#define KSZ_PTP_TAG_LEN 4
> #define KSZ_EGRESS_TAG_LEN 1
> #define KSZ_INGRESS_TAG_LEN 1
>
> +#define KSZ_HWTS_EN 0
> +
> +struct ksz_tagger_private {
> + struct ksz_tagger_data data; /* Must be first */
> + unsigned long state;
> +};
> +
> +static struct ksz_tagger_private *
> +ksz_tagger_private(struct dsa_switch *ds)
> +{
> + return ds->tagger_data;
> +}
> +
> +static bool ksz_hwtstamp_get_state(struct dsa_switch *ds)
> +{
> + struct ksz_tagger_private *priv = ksz_tagger_private(ds);
> +
> + return test_bit(KSZ_HWTS_EN, &priv->state);
> +}
As discussed, I don't really think there exists a case for hwtstamp_get_state().
Don't abuse the tagger-owned storage.
> +
> +static void ksz_hwtstamp_set_state(struct dsa_switch *ds, bool on)
> +{
> + struct ksz_tagger_private *priv = ksz_tagger_private(ds);
> +
> + if (on)
> + set_bit(KSZ_HWTS_EN, &priv->state);
> + else
> + clear_bit(KSZ_HWTS_EN, &priv->state);
> +}
> +
> +static void ksz_disconnect(struct dsa_switch *ds)
> +{
> + struct ksz_tagger_private *priv = ds->tagger_data;
> +
> + kfree(priv);
> + ds->tagger_data = NULL;
> +}
> +
> +static int ksz_connect(struct dsa_switch *ds)
> +{
> + struct ksz_tagger_data *tagger_data;
> + struct ksz_tagger_private *priv;
> +
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + /* Export functions for switch driver use */
> + tagger_data = &priv->data;
> + tagger_data->hwtstamp_get_state = ksz_hwtstamp_get_state;
> + tagger_data->hwtstamp_set_state = ksz_hwtstamp_set_state;
> + ds->tagger_data = priv;
> +
> + return 0;
> +}
> +
> static struct sk_buff *ksz_common_rcv(struct sk_buff *skb,
> struct net_device *dev,
> unsigned int port, unsigned int len)
> @@ -91,10 +149,11 @@ DSA_TAG_DRIVER(ksz8795_netdev_ops);
> MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ8795, KSZ8795_NAME);
>
> /*
> - * For Ingress (Host -> KSZ9477), 2 bytes are added before FCS.
> + * For Ingress (Host -> KSZ9477), 2/6 bytes are added before FCS.
> * ---------------------------------------------------------------------------
> - * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
> + * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|ts(4bytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
> * ---------------------------------------------------------------------------
> + * ts : time stamp (Present only if PTP is enabled in the Hardware)
> * tag0 : Prioritization (not used now)
> * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x10=port5)
> *
> @@ -113,6 +172,19 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ8795, KSZ8795_NAME);
> #define KSZ9477_TAIL_TAG_OVERRIDE BIT(9)
> #define KSZ9477_TAIL_TAG_LOOKUP BIT(10)
>
> +/* Time stamp tag is only inserted if PTP is enabled in hardware. */
Stronger. Time stamp tag *needs* to be inserted if PTP is enabled in hardware.
Regardless of whether this is a PTP frame or not.
I think you don't think this is confusing. But it is confusing.
2 years from now, when this patch gets submitted again for being merged,
I don't want to ask the same questions again.
> +static void ksz_xmit_timestamp(struct dsa_port *dp, struct sk_buff *skb)
> +{
> + struct ksz_tagger_private *priv;
> +
> + priv = ksz_tagger_private(dp->ds);
> +
> + if (!test_bit(KSZ_HWTS_EN, &priv->state))
> + return;
> +
> + put_unaligned_be32(0, skb_put(skb, KSZ_PTP_TAG_LEN));
> +}
Hi Vladimir,
On Thu, 2022-12-01 at 02:52 +0200, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Mon, Nov 28, 2022 at 04:02:18PM +0530, Arun Ramadoss wrote:
> > If PTP is enabled in the hardware, then 4 bytes are added in the
> > tail
> > tag. When PTP is enabled and 4 bytes are not added then messages
> > are
> > corrupted.
>
> Comment in the code please. Also, please spell it out explicitly that
> the tail tag size changes for all TX packets, PTP or not, if PTP
> timestamping is enabled. Your phrasing can be unclear and the reader
> may
> think that only PTP packets require a larger tail tag.
I will elaborate the commit description, why the additional 4 bytes are
required.
>
> >
> > Signed-off-by: Arun Ramadoss <[email protected]>
> > ---
> > diff --git a/drivers/net/dsa/microchip/ksz_common.h
> > b/drivers/net/dsa/microchip/ksz_common.h
> > index cd20f39a565f..4c5b35a7883c 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.h
> > +++ b/drivers/net/dsa/microchip/ksz_common.h
> > @@ -105,7 +105,6 @@ struct ksz_port {
> > u8 num;
> > #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ_PTP)
> > u8 hwts_tx_en;
> > - bool hwts_rx_en;
> > #endif
> > };
> >
> > diff --git a/drivers/net/dsa/microchip/ksz_ptp.c
> > b/drivers/net/dsa/microchip/ksz_ptp.c
> > index a41418c6adf6..184aa57a8489 100644
> > --- a/drivers/net/dsa/microchip/ksz_ptp.c
> > +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> > @@ -54,7 +66,7 @@ int ksz_hwtstamp_get(struct dsa_switch *ds, int
> > port, struct ifreq *ifr)
> >
> > config.tx_type = dev->ports[port].hwts_tx_en;
> >
> > - if (dev->ports[port].hwts_rx_en)
> > + if (tagger_data->hwtstamp_get_state(ds))
>
> Let's be clear, hwtstamp_get_state() deals with TX timestamping, and
> config.rx_filter deals with RX timestamping. Don't mix the two.
> Using custom programs like testptp, you can enable RX timestamping
> but
> not TX timestamping, or the other way around. You don't want the
> driver
> to get confused.
Initially I thought like using one variable in tagger_data to control
the whether to add 4 bytes in tail tag or not. And another variable in
ksz_port to check whether rx timestamping enabled or not.
To avoid using two variables to track the timestamping, I thought
reusing the tagger variable to check rx timestamping as well as PTP
enabled in hardware.
I need to change algorithm such a way that,
- When either Tx timestamping or Rx timestamping enabled in any one of
the port, enable PTP in hardware and add 4 additional bytes in tail
tag.
- Add hwtstamp_config variable in ksz_port, to set and get the hwtstamp
configuration.
>
> > config.rx_filter = HWTSTAMP_FILTER_ALL;
>
> Can the switch provide RX timestamps for all kinds of Ethernet
> packets,
> not just PTP? If not, then report just what it can timestamp.
Ok. I will update it.
>
> > else
> > config.rx_filter = HWTSTAMP_FILTER_NONE;
> > int ksz_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq
> > *ifr)
> > diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> > index 0f6ae143afc9..828af38f0598 100644
> > --- a/net/dsa/tag_ksz.c
> > +++ b/net/dsa/tag_ksz.c
> > @@ -4,6 +4,7 @@
> > * Copyright (c) 2017 Microchip Technology
> > */
> >
> > +#include <linux/dsa/ksz_common.h>
> > #include <linux/etherdevice.h>
> > #include <linux/list.h>
> > #include <net/dsa.h>
> > @@ -16,9 +17,66 @@
> > #define LAN937X_NAME "lan937x"
> >
> > /* Typically only one byte is used for tail tag. */
> > +#define KSZ_PTP_TAG_LEN 4
> > #define KSZ_EGRESS_TAG_LEN 1
> > #define KSZ_INGRESS_TAG_LEN 1
> >
> > +#define KSZ_HWTS_EN 0
> > +
> > +struct ksz_tagger_private {
> > + struct ksz_tagger_data data; /* Must be first */
> > + unsigned long state;
> > +};
> > +
> > +static struct ksz_tagger_private *
> > +ksz_tagger_private(struct dsa_switch *ds)
> > +{
> > + return ds->tagger_data;
> > +}
> > +
> > +static bool ksz_hwtstamp_get_state(struct dsa_switch *ds)
> > +{
> > + struct ksz_tagger_private *priv = ksz_tagger_private(ds);
> > +
> > + return test_bit(KSZ_HWTS_EN, &priv->state);
> > +}
>
> As discussed, I don't really think there exists a case for
> hwtstamp_get_state().
> Don't abuse the tagger-owned storage.
>
> > +
> > +static void ksz_hwtstamp_set_state(struct dsa_switch *ds, bool on)
> > +{
> > + struct ksz_tagger_private *priv = ksz_tagger_private(ds);
> > +
> > + if (on)
> > + set_bit(KSZ_HWTS_EN, &priv->state);
> > + else
> > + clear_bit(KSZ_HWTS_EN, &priv->state);
> > +}
> > +
> >
> > static struct sk_buff *ksz_common_rcv(struct sk_buff *skb,
> > struct net_device *dev,
> > unsigned int port, unsigned int
> > len)
> > @@ -91,10 +149,11 @@ DSA_TAG_DRIVER(ksz8795_netdev_ops);
> > MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ8795, KSZ8795_NAME);
> >
> > /*
> > - * For Ingress (Host -> KSZ9477), 2 bytes are added before FCS.
> > + * For Ingress (Host -> KSZ9477), 2/6 bytes are added before FCS.
> > * -------------------------------------------------------------
> > --------------
> > - *
> > DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS
> > (4bytes)
> > + *
> > DA(6bytes)|SA(6bytes)|....|Data(nbytes)|ts(4bytes)|tag0(1byte)|tag1
> > (1byte)|FCS(4bytes)
> > * -------------------------------------------------------------
> > --------------
> > + * ts : time stamp (Present only if PTP is enabled in the
> > Hardware)
> > * tag0 : Prioritization (not used now)
> > * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2,
> > 0x10=port5)
> > *
> > @@ -113,6 +172,19 @@
> > MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ8795, KSZ8795_NAME);
> > #define KSZ9477_TAIL_TAG_OVERRIDE BIT(9)
> > #define KSZ9477_TAIL_TAG_LOOKUP BIT(10)
> >
> > +/* Time stamp tag is only inserted if PTP is enabled in hardware.
> > */
>
> Stronger. Time stamp tag *needs* to be inserted if PTP is enabled in
> hardware.
> Regardless of whether this is a PTP frame or not.
Ok. I will update it.
>
> I think you don't think this is confusing. But it is confusing.
> 2 years from now, when this patch gets submitted again for being
> merged,
> I don't want to ask the same questions again.
>
> > +static void ksz_xmit_timestamp(struct dsa_port *dp, struct sk_buff
> > *skb)
> > +{
> > + struct ksz_tagger_private *priv;
> > +
> > + priv = ksz_tagger_private(dp->ds);
> > +
> > + if (!test_bit(KSZ_HWTS_EN, &priv->state))
> > + return;
> > +
> > + put_unaligned_be32(0, skb_put(skb, KSZ_PTP_TAG_LEN));
> > +}