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;
Date:   Wed, 7 Jun 2023 10:47:16 -0700
From:   Richard Cochran <richardcochran@gmail.com>
To:     Horatiu Vultur <horatiu.vultur@microchip.com>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        andrew@lunn.ch, hkallweit1@gmail.com, linux@armlinux.org.uk,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com
Subject: Re: [PATCH net-next] net: micrel: Change to receive timestamp in the
 frame for lan8841
Message-ID: <ZIDCpPbCFCxKBV2k@hoboy.vegasvil.org>
References: <20230607070948.1746768-1-horatiu.vultur@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230607070948.1746768-1-horatiu.vultur@microchip.com>
Precedence: bulk

On Wed, Jun 07, 2023 at 09:09:48AM +0200, Horatiu Vultur wrote:

> Doing these changes to start to get the received timestamp in the
> reserved field of the header, will give a great CPU usage performance.
> Running ptp4l with logSyncInterval of -9 will give a ~50% CPU
> improvment.

Really?

> -static struct lan8814_ptp_rx_ts *lan8841_ptp_get_rx_ts(struct kszphy_ptp_priv *ptp_priv)
> -{
> -	struct phy_device *phydev = ptp_priv->phydev;
> -	struct lan8814_ptp_rx_ts *rx_ts;
> -	u32 sec, nsec;
> -	u16 seq;
> -
> -	nsec = phy_read_mmd(phydev, 2, LAN8841_PTP_RX_INGRESS_NS_HI);
> -	if (!(nsec & LAN8841_PTP_RX_INGRESS_NSEC_HI_VALID))
> -		return NULL;
> -
> -	nsec = ((nsec & 0x3fff) << 16);
> -	nsec = nsec | phy_read_mmd(phydev, 2, LAN8841_PTP_RX_INGRESS_NS_LO);
> -
> -	sec = phy_read_mmd(phydev, 2, LAN8841_PTP_RX_INGRESS_SEC_HI);
> -	sec = sec << 16;
> -	sec = sec | phy_read_mmd(phydev, 2, LAN8841_PTP_RX_INGRESS_SEC_LO);
> -
> -	seq = phy_read_mmd(phydev, 2, LAN8841_PTP_RX_MSG_HEADER2);

Before: 5x phy_read_mmd() per frame ...

> -	rx_ts = kzalloc(sizeof(*rx_ts), GFP_KERNEL);
> -	if (!rx_ts)
> -		return NULL;
> -
> -	rx_ts->seconds = sec;
> -	rx_ts->nsec = nsec;
> -	rx_ts->seq_id = seq;
> -
> -	return rx_ts;
> -}

> +static void lan8841_ptp_getseconds(struct ptp_clock_info *ptp,
> +				   struct timespec64 *ts)
> +{
> +	struct kszphy_ptp_priv *ptp_priv = container_of(ptp, struct kszphy_ptp_priv,
> +							ptp_clock_info);
> +	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);
> +
> +	/* Read the LTC */
> +	s = phy_read_mmd(phydev, 2, LAN8841_PTP_LTC_RD_SEC_HI);
> +	s <<= 16;
> +	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);

After: 4x phy_read_mmd() per frame.  How does that save 50% cpu?

> +	mutex_unlock(&ptp_priv->ptp_lock);
> +
> +	set_normalized_timespec64(ts, s, 0);
> +}


> +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.

> +		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)
> +			ts.tv_sec -= GENMASK(1, 0) + 1;
> +
> +		shhwtstamps->hwtstamp =
> +			ktime_set((ts.tv_sec & ~(GENMASK(1, 0))) | ts_header >> 30,
> +				  ts_header & GENMASK(29, 0));
> +		LAN8841_SKB_CB(skb)->header->reserved2 = 0;
> +
> +		netif_rx(skb);
> +	}
> +
> +	return -1;
> +}

Thanks,
Richard