diff mbox series

[net-next,v2,2/2] net: micrel: Schedule work to read seconds for lan8841

Message ID 20230613094526.69532-3-horatiu.vultur@microchip.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: micrel: Change how to read TX timestamp | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 1 maintainers not CCed: richardcochran@gmail.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: line length of 83 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Horatiu Vultur June 13, 2023, 9:45 a.m. UTC 
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@microchip.com>
---
 drivers/net/phy/micrel.c | 49 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 4 deletions(-)

Comments

Richard Cochran June 14, 2023, 4:49 a.m. UTC | #1 
On Tue, Jun 13, 2023 at 11:45:26AM +0200, Horatiu Vultur wrote:
> @@ -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));

Why not do this in the PTP kworker thread?

The thread's scheduling can be easily tuned with chrt to give it
appropriate priority, but work can't.

Also, If you have seconds thread, then you don't have to defer the
received frames.

Thanks,
Richard
Horatiu Vultur June 14, 2023, 9:19 a.m. UTC | #2 
The 06/13/2023 21:49, Richard Cochran wrote:

Hi Richard,

> 
> On Tue, Jun 13, 2023 at 11:45:26AM +0200, Horatiu Vultur wrote:
> > @@ -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));
> 
> Why not do this in the PTP kworker thread?

I presume you mean the work of reading the second part to be done in the
PTP kworker thread and not scheduling the seconds_work.
Because then it make sense to me and I think is a great idea.
> 
> The thread's scheduling can be easily tuned with chrt to give it
> appropriate priority, but work can't.

Nice, I didn't know about this.

> 
> Also, If you have seconds thread, then you don't have to defer the
> received frames.

Exactly, the PTP kworker thread will cache the seconds value while
lan8841_rxtstamp will read this value, so no need to defer these frames.

In this way I can get rid of seconds_work.

> 
> Thanks,
> Richard
>
diff mbox series

Patch

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);
 }