Re: [PATCH net-next] net: micrel: Change to receive timestamp in the frame for lan8841

[Horatiu Vultur]

The 06/07/2023 22:51, Richard Cochran wrote:
> 
> On Wed, Jun 07, 2023 at 10:47:16AM -0700, Richard Cochran wrote:
> > On Wed, Jun 07, 2023 at 09:09:48AM +0200, Horatiu Vultur wrote:
> > > +static long lan8841_ptp_do_aux_work(struct ptp_clock_info *ptp)
> > > +{
> > > +   struct kszphy_ptp_priv *ptp_priv = container_of(ptp, struct kszphy_ptp_priv,
> > > +                                                   ptp_clock_info);
> > > +   struct skb_shared_hwtstamps *shhwtstamps;
> > > +   struct timespec64 ts;
> > > +   struct sk_buff *skb;
> > > +   u32 ts_header;
> > > +
> > > +   while ((skb = skb_dequeue(&ptp_priv->rx_queue)) != NULL) {
> > > +           lan8841_ptp_getseconds(ptp, &ts);
> >
> > No need to call this once per frame.  It would be sufficent to call it
> > once every 2 seconds and cache the result.

Funny thing is that I have already a patch for this, it is not 100%
complete, because of one of the possible issues that you mention below.
But in my case I read the seconds twice per second, regardless of the RX
frame rate.

> 
> Okay, this is tricky.
> 
> - If you call lan8841_ptp_getseconds() after gathering the received
>   frames, then the frame timestamps are clearly in the past WRT the
>   call to getseconds.  That makes the wrap check simpler.  But the
>   getseconds call really should be placed before the 'while' loop.
> 
> - If the Rx frame rate exceeds 1/second, then it would be more
>   efficient to call getseconds every second, and cache the result.
>   But then the wrap around check needs to account for the fact that
>   the cached value may have occurred either before or after the frame
>   timestamp.
> 
> I'll explain that second point when my brain wakes again...

I am looking forward for your explanation.

In the end don't you think it should be 2 different patches, first patch
which changes to mode to get the RX timestamp, while the second one
optimize this?

Bellow just for reference I added the patch which it tries to get the second
part twice per second.

---
Subject: [PATCH] net: micrel: Schedule work to read seconds for lan8841

Instead of reading the seconds part of the received frame for each of
the frames, schedule a workqueue to read the seconds part every 500ms
and then for each of the received frames use this information instead of
reading again the seconds part. Because if for example running with 512
frames per second, there is no point to read 512 times the second part.
Of course care needs to be taken in case of the less two significant
bits are 0 or 3, to make sure there are no seconds wraparound.
This will improve the CPU usage by ~20% and also it is possible to receive
1024 Sync frames per second.

Signed-off-by: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
---
 drivers/net/phy/micrel.c | 49 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 28365006b2067..9832eea404377 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -33,6 +33,7 @@
 #include <linux/ptp_classify.h>
 #include <linux/net_tstamp.h>
 #include <linux/gpio/consumer.h>
+#include <linux/workqueue.h>
 
 /* Operation Mode Strap Override */
 #define MII_KSZPHY_OMSO				0x16
@@ -254,6 +255,9 @@
 #define PS_TO_REG				200
 #define FIFO_SIZE				8
 
+/* Delay used to get the second part from the LTC */
+#define LAN8841_GET_SEC_LTC_DELAY		(500 * NSEC_PER_MSEC)
+
 struct kszphy_hw_stat {
 	const char *string;
 	u8 reg;
@@ -321,6 +325,9 @@ struct kszphy_ptp_priv {
 	/* Lock for ptp_clock */
 	struct mutex ptp_lock;
 	struct ptp_pin_desc *pin_config;
+
+	s64 seconds;
+	struct delayed_work seconds_work;
 };
 
 struct kszphy_priv {
@@ -3840,6 +3847,12 @@ static void lan8841_ptp_enable_processing(struct kszphy_ptp_priv *ptp_priv,
 			       LAN8841_PTP_INSERT_TS_32BIT,
 			       LAN8841_PTP_INSERT_TS_EN |
 			       LAN8841_PTP_INSERT_TS_32BIT);
+
+		/* Schedule the work to read the seconds, which will be used in
+		 * the received timestamp
+		 */
+		schedule_delayed_work(&ptp_priv->seconds_work,
+				      nsecs_to_jiffies(LAN8841_GET_SEC_LTC_DELAY));
 	} else {
 		/* Disable interrupts on the TX side */
 		phy_modify_mmd(phydev, 2, LAN8841_PTP_INT_EN,
@@ -3853,6 +3866,11 @@ static void lan8841_ptp_enable_processing(struct kszphy_ptp_priv *ptp_priv,
 			       LAN8841_PTP_INSERT_TS_32BIT, 0);
 
 		ptp_cancel_worker_sync(ptp_priv->ptp_clock);
+
+		/* Stop the work, as there is no reason to continue to read the
+		 * seconds if there is no timestamping enabled
+		 */
+		cancel_delayed_work_sync(&ptp_priv->seconds_work);
 	}
 }
 
@@ -4062,6 +4080,8 @@ static int lan8841_ptp_settime64(struct ptp_clock_info *ptp,
 	phy_write_mmd(phydev, 2, LAN8841_PTP_LTC_SET_NS_LO, lower_16_bits(ts->tv_nsec));
 	phy_write_mmd(phydev, 2, LAN8841_PTP_LTC_SET_NS_HI, upper_16_bits(ts->tv_nsec) & 0x3fff);
 
+	ptp_priv->seconds = ts->tv_sec;
+
 	/* Set the command to load the LTC */
 	phy_write_mmd(phydev, 2, LAN8841_PTP_CMD_CTL,
 		      LAN8841_PTP_CMD_CTL_PTP_LTC_LOAD);
@@ -4116,7 +4136,6 @@ static void lan8841_ptp_getseconds(struct ptp_clock_info *ptp,
 	struct phy_device *phydev = ptp_priv->phydev;
 	time64_t s;
 
-	mutex_lock(&ptp_priv->ptp_lock);
 	/* Issue the command to read the LTC */
 	phy_write_mmd(phydev, 2, LAN8841_PTP_CMD_CTL,
 		      LAN8841_PTP_CMD_CTL_PTP_LTC_READ);
@@ -4127,7 +4146,6 @@ static void lan8841_ptp_getseconds(struct ptp_clock_info *ptp,
 	s |= phy_read_mmd(phydev, 2, LAN8841_PTP_LTC_RD_SEC_MID);
 	s <<= 16;
 	s |= phy_read_mmd(phydev, 2, LAN8841_PTP_LTC_RD_SEC_LO);
-	mutex_unlock(&ptp_priv->ptp_lock);
 
 	set_normalized_timespec64(ts, s, 0);
 }
@@ -4644,15 +4662,20 @@ static long lan8841_ptp_do_aux_work(struct ptp_clock_info *ptp)
 	u32 ts_header;
 
 	while ((skb = skb_dequeue(&ptp_priv->rx_queue)) != NULL) {
-		lan8841_ptp_getseconds(ptp, &ts);
+		mutex_lock(&ptp_priv->ptp_lock);
+		ts.tv_sec = ptp_priv->seconds;
+		mutex_unlock(&ptp_priv->ptp_lock);
+
 		ts_header = __be32_to_cpu(LAN8841_SKB_CB(skb)->header->reserved2);
 
 		shhwtstamps = skb_hwtstamps(skb);
 		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
 
 		/* Check for any wrap arounds for the second part */
-		if ((ts.tv_sec & GENMASK(1, 0)) < ts_header >> 30)
+		if ((ts.tv_sec & GENMASK(1, 0)) == 0 && (ts_header >> 30) == 3)
 			ts.tv_sec -= GENMASK(1, 0) + 1;
+		else if ((ts.tv_sec & GENMASK(1, 0)) == 3 && (ts_header >> 30) == 0)
+			ts.tv_sec += 1;
 
 		shhwtstamps->hwtstamp =
 			ktime_set((ts.tv_sec & ~(GENMASK(1, 0))) | ts_header >> 30,
@@ -4665,6 +4688,21 @@ static long lan8841_ptp_do_aux_work(struct ptp_clock_info *ptp)
 	return -1;
 }
 
+static void lan8841_ptp_seconds_work(struct work_struct *seconds_work)
+{
+	struct kszphy_ptp_priv *ptp_priv =
+		container_of(seconds_work, struct kszphy_ptp_priv, seconds_work.work);
+	struct timespec64 ts;
+
+	mutex_lock(&ptp_priv->ptp_lock);
+	lan8841_ptp_getseconds(&ptp_priv->ptp_clock_info, &ts);
+	ptp_priv->seconds = ts.tv_sec;
+	mutex_unlock(&ptp_priv->ptp_lock);
+
+	schedule_delayed_work(&ptp_priv->seconds_work,
+			      nsecs_to_jiffies(LAN8841_GET_SEC_LTC_DELAY));
+}
+
 static struct ptp_clock_info lan8841_ptp_clock_info = {
 	.owner		= THIS_MODULE,
 	.name		= "lan8841 ptp",
@@ -4749,6 +4787,8 @@ static int lan8841_probe(struct phy_device *phydev)
 
 	phydev->mii_ts = &ptp_priv->mii_ts;
 
+	INIT_DELAYED_WORK(&ptp_priv->seconds_work, lan8841_ptp_seconds_work);
+
 	return 0;
 }
 
@@ -4758,6 +4798,7 @@ static int lan8841_suspend(struct phy_device *phydev)
 	struct kszphy_ptp_priv *ptp_priv = &priv->ptp_priv;
 
 	ptp_cancel_worker_sync(ptp_priv->ptp_clock);
+	cancel_delayed_work_sync(&ptp_priv->seconds_work);
 
 	return genphy_suspend(phydev);
 }
-- 
---

> 
> Thanks,
> Richard

-- 
/Horatiu