Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Received-SPF: Fail (protection.outlook.com: domain of arri.de does not
 designate 217.111.95.7 as permitted sender) receiver=protection.outlook.com;
 client-ip=217.111.95.7; helo=mta.arri.de;
From:   Christian Eggers <ceggers@arri.de>
To:     <ceggers@arri.de>, <Arun.Ramadoss@microchip.com>,
        <olteanv@gmail.com>
CC:     <andrew@lunn.ch>, <linux-kernel@vger.kernel.org>,
        <UNGLinuxDriver@microchip.com>, <vivien.didelot@gmail.com>,
        <linux@armlinux.org.uk>, <Tristram.Ha@microchip.com>,
        <f.fainelli@gmail.com>, <kuba@kernel.org>, <edumazet@google.com>,
        <pabeni@redhat.com>, <richardcochran@gmail.com>,
        <netdev@vger.kernel.org>, <Woojung.Huh@microchip.com>,
        <davem@davemloft.net>, <b.hutchman@gmail.com>
Subject: Re: [RFC Patch net-next 0/6] net: dsa: microchip: add gPTP support for LAN937x switch
Date:   Wed, 26 Oct 2022 18:47:53 +0200
Message-ID: <20221026164753.13866-1-ceggers@arri.de>
In-Reply-To: <d8459511785de2f6d503341ce5f87c6e6064d7b5.camel@microchip.com>
References: <d8459511785de2f6d503341ce5f87c6e6064d7b5.camel@microchip.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Oct 2022 16:48:43.6035
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 3ca0d98e-3524-49d1-8391-08dab771f172
X-MS-Exchange-CrossTenant-Id: e6a73a5a-614d-4c51-b3e3-53b660a9433a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=e6a73a5a-614d-4c51-b3e3-53b660a9433a;Ip=[217.111.95.7];Helo=[mta.arri.de]
X-MS-Exchange-CrossTenant-AuthSource: AM0EUR02FT020.eop-EUR02.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM9PR07MB7107
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Arun, hi Vladimir,

On Tuesday, 18 October 2022, 15:42:41 CEST, Arun.Ramadoss@microchip.com wrote:
> ...
> Thanks Vladimir. I will wait for Christian feedback.
> 
> Hi Christian,
> To test this patch on KSZ9563, we need to increase the number of
> interrupts port_nirqs in KSZ9893 from 2 to 3. Since the chip id of
> KSZ9893 and KSZ9563 are same, I had reused the ksz_chip_data same for
> both chips. But this chip differ with number of port interrupts. So we
> need to update it. We are generating a new patch for adding the new
> element in the ksz_chip_data for KSZ9563.
> For now, you can update the code as below for testing the patch

today I hard first success with your patch series on KSZ9563! ptp4l reported
delay measurements between switch port 1 and the connected Meinberg clock:

> ptp4l[75.590]: port 2: new foreign master ec4670.fffe.0a9dcc-1
> ptp4l[79.590]: selected best master clock ec4670.fffe.0a9dcc
> ptp4l[79.590]: updating UTC offset to 37
> ptp4l[79.591]: port 2: LISTENING to UNCALIBRATED on RS_SLAVE
> ptp4l[81.114]: port 2: delay timeout
> ptp4l[81.117]: delay   filtered        338   raw        338
> ptp4l[81.118]: port 2: minimum delay request interval 2^1
> ptp4l[81.434]: port 1: announce timeout
> ptp4l[81.434]: config item lan0.udp_ttl is 1
> ptp4l[81.436]: config item (null).dscp_event is 0
> ptp4l[81.437]: config item (null).dscp_general is 0
> ptp4l[81.437]: selected best master clock ec4670.fffe.0a9dcc
> ptp4l[81.438]: updating UTC offset to 37
> ptp4l[81.843]: master offset         33 s0 freq   +6937 path delay       338
> ptp4l[82.844]: master offset         26 s2 freq   +6930 path delay       338
> ptp4l[82.844]: port 2: UNCALIBRATED to SLAVE on MASTER_CLOCK_SELECTED
> ptp4l[83.844]: master offset         32 s2 freq   +6962 path delay       338
> ptp4l[84.844]: master offset          3 s2 freq   +6943 path delay       338
> ptp4l[85.844]: master offset        -14 s2 freq   +6927 path delay       338
> ptp4l[86.042]: port 2: delay timeout
> ptp4l[86.045]: delay   filtered        336   raw        335
> ptp4l[86.211]: port 2: delay timeout
> ptp4l[86.213]: delay   filtered        335   raw        331
> ptp4l[86.844]: master offset          3 s2 freq   +6939 path delay       335
> ptp4l[87.847]: master offset         -7 s2 freq   +6930 path delay       335

As a next step I'll try to configure the external output for 1PPS. Is this
already implemented in your patches? The file /sys/class/ptp/ptp2/n_periodic_outputs
shows '0' on my system.

BTW: Which is the preferred delay measurement which I shall test (E2E/P2P)? I
started with E2E is this was configured in the hardware and needs no 1-step
time stamping, but I had to add PTP_MSGTYPE_DELAY_REQ in ksz_port_txtstamp().

On Tuesday, 25 October 2022, 05:41:26 CEST, Arun.Ramadoss@microchip.com wrote:
> On Sun, 2022-10-23 at 22:15 +0200, Christian Eggers wrote:
> > ...
> > After applying the patch series, I had some trouble with linking. I
> > had
> > configured nearly everything as a module:
> > 
> > CONFIG_PTP_1588_CLOCK=m
> > CONFIG_NET_DSA=m
> > CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON=m [ksz_switch.ko]
> > CONFIG_NET_DSA_MICROCHIP_KSZ9477_I2C=m
> > CONFIG_NET_DSA_MICROCHIP_KSZ_PTP=y [builtin]
> > 
> > With this configuration, kbuild doesn't even try to compile
> > ksz_ptp.c:
> > 
> > ERROR: modpost: "ksz_hwtstamp_get"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_hwtstamp_set"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_port_txtstamp"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_ptp_clock_register"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_port_deferred_xmit"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_ptp_clock_unregister"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_ptp_irq_free"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_tstamp_reconstruct"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_get_ts_info"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_ptp_irq_setup"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > 
> > After setting all of the above to 'y', the build process works (but I
> > would prefer
> > being able to build as modules). 
> 
> May be this is due to kconfig of config_ksz_ptp  defined bool instead
> of tristate. Do I need to change the config_ksz_ptp to tristate in
> order to compile as modules?
I'm not an expert for kbuild and cannot tell whether it's allowed to use
bool options which depend on tristate options. At least ksz_ptp.c is compiled
by kbuild if tristate is used. But I needed to add additional EXPORT_SYMBOL()
statements for all non-static functions (see below) for successful linking.

I'm unsure whether it makes sense to build ksz_ptp as a separate module.
Perhaps it should be (conditionally) added to ksz_switch.ko.

On Tuesday, 18 October 2022, 08:44:04 CEST, Arun.Ramadoss@microchip.com wrote:
> I had developed this patch set to add gPTP support for LAN937x based on
> the Christian eggers patch for KSZ9563.
Maybe this could be mentioned somewhere (e.g. extra line in file header of
ksz_ptp.c).  It took a lot of effort (for me) to get this initially running
(e.g. due to limited documentation / support by Microchip).  But I'm quite happy
that this is continued now as it is likely that I'll need PTP support for the
KSZ9563 soon.

For KSZ9563, we will need support for 1-step time stamping as two-step
is not possible.

I've stashed all my local changes into an additional patch (see below).
Please feel free to integrate this into your series.  As soon I get 1PPS
running, I'll continue testing.  Note that I'll be unavailable between Friday
and next Tuesday.

regards,
Christian
---
 drivers/net/dsa/microchip/Kconfig       |  2 +-
 drivers/net/dsa/microchip/ksz9477_i2c.c |  2 +
 drivers/net/dsa/microchip/ksz_common.c  |  2 +-
 drivers/net/dsa/microchip/ksz_ptp.c     | 13 +++++-
 net/dsa/tag_ksz.c                       | 60 +++++++++++++++++++++++--
 5 files changed, 73 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/Kconfig b/drivers/net/dsa/microchip/Kconfig
index 1e9712ff64e2..ac34c01f39a6 100644
--- a/drivers/net/dsa/microchip/Kconfig
+++ b/drivers/net/dsa/microchip/Kconfig
@@ -22,7 +22,7 @@ config NET_DSA_MICROCHIP_KSZ_SPI
 	  Select to enable support for registering switches configured through SPI.
 
 config NET_DSA_MICROCHIP_KSZ_PTP
-	bool "Support for the PTP clock on the KSZ9563/LAN937x Ethernet Switch"
+	tristate "Support for the PTP clock on the KSZ9563/LAN937x Ethernet Switch"
 	depends on NET_DSA_MICROCHIP_KSZ_COMMON && PTP_1588_CLOCK
 	help
 	  This enables support for timestamping & PTP clock manipulation
diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
index 3763930dc6fc..7eb7d887bf43 100644
--- a/drivers/net/dsa/microchip/ksz9477_i2c.c
+++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
@@ -41,6 +41,8 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c,
 	if (i2c->dev.platform_data)
 		dev->pdata = i2c->dev.platform_data;
 
+	dev->irq = i2c->irq;
+
 	ret = ksz_switch_register(dev);
 
 	/* Main DSA driver may not be started yet. */
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 889b3d398def..679c66f1e420 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1266,7 +1266,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.num_statics = 16,
 		.cpu_ports = 0x07,	/* can be configured as cpu port */
 		.port_cnt = 3,		/* total port count */
-		.port_nirqs = 2,
+		.port_nirqs = 3,
 		.ops = &ksz9477_dev_ops,
 		.mib_names = ksz9477_mib_names,
 		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index d11a490a6c87..6e6814286dec 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -68,6 +68,7 @@ int ksz_get_ts_info(struct dsa_switch *ds, int port, struct ethtool_ts_info *ts)
 
 	return 0;
 }
+EXPORT_SYMBOL(ksz_get_ts_info);
 
 int ksz_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
 {
@@ -90,6 +91,7 @@ int ksz_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
 	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ?
 		-EFAULT : 0;
 }
+EXPORT_SYMBOL(ksz_hwtstamp_get);
 
 static int ksz_set_hwtstamp_config(struct ksz_device *dev, int port,
 				   struct hwtstamp_config *config)
@@ -106,7 +108,7 @@ static int ksz_set_hwtstamp_config(struct ksz_device *dev, int port,
 	case HWTSTAMP_TX_OFF:
 		prt->hwts_tx_en = false;
 		break;
-	case HWTSTAMP_TX_ON:
+	case HWTSTAMP_TX_ONESTEP_P2P:
 		prt->hwts_tx_en = true;
 		break;
 	default:
@@ -162,6 +164,7 @@ int ksz_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
 	mutex_unlock(&ptp_data->lock);
 	return ret;
 }
+EXPORT_SYMBOL(ksz_hwtstamp_set);
 
 void ksz_port_txtstamp(struct dsa_switch *ds, int port,
 		       struct sk_buff *skb)
@@ -187,6 +190,7 @@ void ksz_port_txtstamp(struct dsa_switch *ds, int port,
 	ptp_msg_type = ptp_get_msgtype(hdr, type);
 
 	switch (ptp_msg_type) {
+	case PTP_MSGTYPE_DELAY_REQ:
 	case PTP_MSGTYPE_PDELAY_REQ:
 	case PTP_MSGTYPE_PDELAY_RESP:
 	case PTP_MSGTYPE_SYNC:
@@ -202,6 +206,7 @@ void ksz_port_txtstamp(struct dsa_switch *ds, int port,
 
 	KSZ_SKB_CB(skb)->clone = clone;
 }
+EXPORT_SYMBOL(ksz_port_txtstamp);
 
 /* These are function related to the ptp clock info */
 static int _ksz_ptp_gettime(struct ksz_device *dev, struct timespec64 *ts)
@@ -436,6 +441,7 @@ ktime_t ksz_tstamp_reconstruct(struct dsa_switch *ds, ktime_t tstamp)
 
 	return timespec64_to_ktime(ts);
 }
+EXPORT_SYMBOL(ksz_tstamp_reconstruct);
 
 static void ksz_ptp_txtstamp_skb(struct ksz_device *dev,
 				 struct ksz_port *prt, struct sk_buff *skb)
@@ -479,6 +485,7 @@ void ksz_port_deferred_xmit(struct kthread_work *work)
 
 	kfree(xmit_work);
 }
+EXPORT_SYMBOL(ksz_port_deferred_xmit);
 
 static const struct ptp_clock_info ksz_ptp_caps = {
 	.owner		= THIS_MODULE,
@@ -524,6 +531,7 @@ int ksz_ptp_clock_register(struct dsa_switch *ds)
 	ptp_clock_unregister(ptp_data->clock);
 	return ret;
 }
+EXPORT_SYMBOL(ksz_ptp_clock_register);
 
 void ksz_ptp_clock_unregister(struct dsa_switch *ds)
 {
@@ -535,6 +543,7 @@ void ksz_ptp_clock_unregister(struct dsa_switch *ds)
 
 	ptp_clock_unregister(ptp_data->clock);
 }
+EXPORT_SYMBOL(ksz_ptp_clock_unregister);
 
 static irqreturn_t ksz_ptp_msg_thread_fn(int irq, void *dev_id)
 {
@@ -734,6 +743,7 @@ int ksz_ptp_irq_setup(struct dsa_switch *ds, u8 p)
 
 	return ret;
 }
+EXPORT_SYMBOL(ksz_ptp_irq_setup);
 
 void ksz_ptp_irq_free(struct dsa_switch *ds, u8 p)
 {
@@ -749,6 +759,7 @@ void ksz_ptp_irq_free(struct dsa_switch *ds, u8 p)
 
 	irq_domain_remove(ptpirq->domain);
 }
+EXPORT_SYMBOL(ksz_ptp_irq_free);
 
 MODULE_AUTHOR("Arun Ramadoss <arun.ramadoss@microchip.com>");
 MODULE_DESCRIPTION("PTP support for KSZ switch");
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 582add3398d3..e7680718b478 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -251,17 +251,69 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9477);
 #define KSZ9893_TAIL_TAG_OVERRIDE	BIT(5)
 #define KSZ9893_TAIL_TAG_LOOKUP		BIT(6)
 
+/* Time stamp tag is only inserted if PTP is enabled in hardware. */
+static void ksz9893_xmit_timestamp(struct sk_buff *skb)
+{
+//	struct sk_buff *clone = KSZ9477_SKB_CB(skb)->clone;
+//	struct ptp_header *ptp_hdr;
+//	unsigned int ptp_type;
+	u32 tstamp_raw = 0;
+	put_unaligned_be32(tstamp_raw, skb_put(skb, KSZ9477_PTP_TAG_LEN));
+}
+
+/* Defer transmit if waiting for egress time stamp is required.  */
+static struct sk_buff *ksz9893_defer_xmit(struct dsa_port *dp,
+					  struct sk_buff *skb)
+{
+	struct ksz_tagger_data *tagger_data = ksz_tagger_data(dp->ds);
+	struct ksz_tagger_private *priv = ksz_tagger_private(dp->ds);
+	void (*xmit_work_fn)(struct kthread_work *work);
+	struct sk_buff *clone = KSZ_SKB_CB(skb)->clone;
+	struct ksz_deferred_xmit_work *xmit_work;
+	struct kthread_worker *xmit_worker;
+
+	if (!clone)
+		return skb;  /* no deferred xmit for this packet */
+
+	xmit_work_fn = tagger_data->xmit_work_fn;
+	xmit_worker = priv->xmit_worker;
+
+	if (!xmit_work_fn || !xmit_worker)
+		return NULL;
+
+	xmit_work = kzalloc(sizeof(*xmit_work), GFP_ATOMIC);
+	if (!xmit_work)
+		return NULL;
+
+	kthread_init_work(&xmit_work->work, xmit_work_fn);
+	/* Increase refcount so the kfree_skb in dsa_slave_xmit
+	 * won't really free the packet.
+	 */
+	xmit_work->dp = dp;
+	xmit_work->skb = skb_get(skb);
+
+	kthread_queue_work(xmit_worker, &xmit_work->work);
+
+	return NULL;
+}
+
 static struct sk_buff *ksz9893_xmit(struct sk_buff *skb,
 				    struct net_device *dev)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct ksz_tagger_private *priv;
 	u8 *addr;
 	u8 *tag;
 
+	priv = ksz_tagger_private(dp->ds);
+
+	/* Tag encoding */
+	if (test_bit(KSZ_HWTS_EN, &priv->state))
+		ksz9893_xmit_timestamp(skb);
+
 	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
 		return NULL;
 
-	/* Tag encoding */
 	tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
 	addr = skb_mac_header(skb);
 
@@ -270,7 +322,7 @@ static struct sk_buff *ksz9893_xmit(struct sk_buff *skb,
 	if (is_link_local_ether_addr(addr))
 		*tag |= KSZ9893_TAIL_TAG_OVERRIDE;
 
-	return skb;
+	return ksz9893_defer_xmit(dp, skb);
 }
 
 static const struct dsa_device_ops ksz9893_netdev_ops = {
@@ -278,7 +330,9 @@ 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 + KSZ9477_PTP_TAG_LEN,
 };
 
 DSA_TAG_DRIVER(ksz9893_netdev_ops);
-- 
2.35.3