Re: [PATCH] ptp: Don't print an error if ptp_kvm is not supported
On Tue, Apr 20, 2021 at 02:24:19PM +0100, Jon Hunter wrote:
> Commit 300bb1fe7671 ("ptp: arm/arm64: Enable ptp_kvm for arm/arm64")
> enable ptp_kvm support for ARM platforms and for any ARM platform that
> does not support this, the following error message is displayed ...
>
> ERR KERN fail to initialize ptp_kvm
>
> For platforms that do not support ptp_kvm this error is a bit misleading
> and so fix this by only printing this message if the error returned by
> kvm_arch_ptp_init() is not -EOPNOTSUPP. Note that -EOPNOTSUPP is only
> returned by ARM platforms today if ptp_kvm is not supported.
>
> Fixes: 300bb1fe7671 ("ptp: arm/arm64: Enable ptp_kvm for arm/arm64")
> Signed-off-by: Jon Hunter
Acked-by: Richard Cochran
Re: [net-next 1/3] net: dsa: optimize tx timestamp request handling
On Sun, Apr 18, 2021 at 12:18:42PM +0300, Vladimir Oltean wrote: > > How about not passing "clone" back to DSA as an argument by reference, > but instead require the driver to populate DSA_SKB_CB(skb)->clone if it > needs to do so? > > Also, how about changing the return type to void? Returning true or > false makes no difference. +1 Thanks, Richard
Re: [net-next 1/3] net: dsa: optimize tx timestamp request handling
On Fri, Apr 16, 2021 at 08:36:53PM +0800, Yangbo Lu wrote:
> @@ -628,9 +620,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb,
> struct net_device *dev)
>
> DSA_SKB_CB(skb)->clone = NULL;
>
> - /* Identify PTP protocol packets, clone them, and pass them to the
> - * switch driver
> - */
> + /* Handle tx timestamp request if has */
"if has" what?
> dsa_skb_tx_timestamp(p, skb);
>
> if (dsa_realloc_skb(skb, dev)) {
> --
> 2.25.1
>
Thanks,
Richard
Re: [net-next 1/3] net: dsa: optimize tx timestamp request handling
On Fri, Apr 16, 2021 at 08:36:53PM +0800, Yangbo Lu wrote: > Optimization could be done on dsa_skb_tx_timestamp(), and dsa device > drivers should adapt to it. > > - Check SKBTX_HW_TSTAMP request flag at the very beginning, instead of in > port_txtstamp, so that most skbs not requiring tx timestamp just return. > > - No longer to identify PTP packets, and limit tx timestamping only for PTP > packets. If device driver likes, let device driver do. > > - It is a waste to clone skb directly in dsa_skb_tx_timestamp(). > For one-step timestamping, a clone is not needed. For any failure of > port_txtstamp (this may usually happen), the skb clone has to be freed. > So put skb cloning into port_txtstamp where it really needs. This patch changes three things ^^^ at once. These are AFAICT independent changes, and so please break this one patch into three for easier review. Thanks, Richard
Re: [PATCH net-next v4 1/1] net: stmmac: Add support for external trigger timestamping
On Wed, Apr 14, 2021 at 08:16:17AM +0800, Wong Vee Khee wrote: > From: Tan Tee Min > > The Synopsis MAC controller supports auxiliary snapshot feature that > allows user to store a snapshot of the system time based on an external > event. > > This patch add supports to the above mentioned feature. Users will be > able to triggered capturing the time snapshot from user-space using > application such as testptp or any other applications that uses the > PTP_EXTTS_REQUEST ioctl request. > > Cc: Richard Cochran > Signed-off-by: Tan Tee Min > Co-developed-by: Wong Vee Khee > Signed-off-by: Wong Vee Khee Acked-by: Richard Cochran
Re: [PATCH net-next 0/8] ionic: hwstamp tweaks
On Mon, Apr 12, 2021 at 09:33:29AM -0700, Shannon Nelson wrote: > If the original patches hadn't already been pulled into net-next, this is > what I would have done. My understanding is that once the patches have been > pulled into the repo that we need to do delta patches, not new versions of > the same patch, as folks don't normally like changing published tree > history. Oh, the series you posted on April 1 was merged on April 2 without any review. That seems surprising to me, but perhaps the development tempo has increased. Wow, and this delta series was also: posted Date: Wed, 7 Apr 2021 16:19:53 -0700 merged Date: Thu, 08 Apr 2021 20:30:28 + That is a pretty good turn around time, less that 24 hours! Oh well, too late to add my Acked-by. Thanks, Richard
Re: [PATCH net-next 0/8] ionic: hwstamp tweaks
On Wed, Apr 07, 2021 at 04:19:53PM -0700, Shannon Nelson wrote: > A few little changes after review comments and > additional internal testing. This series is a delta against the previously posted one. Please follow the process by re-basing your changes into the original series, putting a "v2" into the Subject line, and adding a brief change log into the cover letter. Thanks, Richard
Re: [PATCH net-next v3 1/1] net: stmmac: Add support for external trigger timestamping
On Sun, Apr 11, 2021 at 10:40:28AM +0800, Wong Vee Khee wrote:
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> index 60566598d644..60e17fd24aba 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> @@ -296,6 +296,13 @@ static int intel_crosststamp(ktime_t *device,
>
> intel_priv = priv->plat->bsp_priv;
>
> + /* Both internal crosstimestamping and external triggered event
> + * timestamping cannot be run concurrently.
> + */
> + if (priv->plat->ext_snapshot_en)
> + return -EBUSY;
> +
> + mutex_lock(&priv->aux_ts_lock);
Lock, then ...
> /* Enable Internal snapshot trigger */
> acr_value = readl(ptpaddr + PTP_ACR);
> acr_value &= ~PTP_ACR_MASK;
> @@ -321,6 +328,7 @@ static int intel_crosststamp(ktime_t *device,
> acr_value = readl(ptpaddr + PTP_ACR);
> acr_value |= PTP_ACR_ATSFC;
> writel(acr_value, ptpaddr + PTP_ACR);
> + mutex_unlock(&priv->aux_ts_lock);
unlock, then ...
> /* Trigger Internal snapshot signal
>* Create a rising edge by just toggle the GPO1 to low
> @@ -355,6 +363,8 @@ static int intel_crosststamp(ktime_t *device,
> *system = convert_art_to_tsc(art_time);
> }
>
> + /* Release the mutex */
> + mutex_unlock(&priv->aux_ts_lock);
unlock again? Huh?
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index c49debb62b05..abadcd8cdc41 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -239,6 +239,9 @@ struct stmmac_priv {
> int use_riwt;
> int irq_wake;
> spinlock_t ptp_lock;
> + /* Mutex lock for Auxiliary Snapshots */
> + struct mutex aux_ts_lock;
In the comment, please be specific about which data are protected.
For example:
/* Protects auxiliary snapshot registers from concurrent access. */
> @@ -163,6 +166,43 @@ static void get_ptptime(void __iomem *ptpaddr, u64
> *ptp_time)
> *ptp_time = ns;
> }
>
> +static void timestamp_interrupt(struct stmmac_priv *priv)
> +{
> + struct ptp_clock_event event;
> + unsigned long flags;
> + u32 num_snapshot;
> + u32 ts_status;
> + u32 tsync_int;
Please group same types together (u32) in a one-line list.
> + u64 ptp_time;
> + int i;
> +
> + tsync_int = readl(priv->ioaddr + GMAC_INT_STATUS) & GMAC_INT_TSIE;
> +
> + if (!tsync_int)
> + return;
> +
> + /* Read timestamp status to clear interrupt from either external
> + * timestamp or start/end of PPS.
> + */
> + ts_status = readl(priv->ioaddr + GMAC_TIMESTAMP_STATUS);
Reading this register has a side effect of clearing status? If so,
doesn't it need protection against concurrent access?
The function, intel_crosststamp() also reads this bit.
> + if (!priv->plat->ext_snapshot_en)
> + return;
Doesn't this test come too late? Setting ts_status just cleared the
bit used by the other code path.
> + num_snapshot = (ts_status & GMAC_TIMESTAMP_ATSNS_MASK) >>
> +GMAC_TIMESTAMP_ATSNS_SHIFT;
> +
> + for (i = 0; i < num_snapshot; i++) {
> + spin_lock_irqsave(&priv->ptp_lock, flags);
> + get_ptptime(priv->ptpaddr, &ptp_time);
> + spin_unlock_irqrestore(&priv->ptp_lock, flags);
> + event.type = PTP_CLOCK_EXTTS;
> + event.index = 0;
> + event.timestamp = ptp_time;
> + ptp_clock_event(priv->ptp_clock, &event);
> + }
> +}
> +
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index b164ae22e35f..d668c33a0746 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> @@ -135,9 +135,13 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
> {
> struct stmmac_priv *priv =
> container_of(ptp, struct stmmac_priv, ptp_clock_ops);
> + void __iomem *ptpaddr = priv->ptpaddr;
> + void __iomem *ioaddr = priv->hw->pcsr;
> struct stmmac_pps_cfg *cfg;
> int ret = -EOPNOTSUPP;
> unsigned long flags;
> + u32 intr_value;
> + u32 acr_value;
Please group same types together.
Thanks,
Richard
Re: [PATCH v19 3/7] ptp: Reorganize ptp_kvm.c to make it arch-independent
On Wed, Apr 07, 2021 at 10:28:44AM +0100, Marc Zyngier wrote: > On Tue, 30 Mar 2021 15:54:26 +0100, > Marc Zyngier wrote: > > > > From: Jianyong Wu > > > > Currently, the ptp_kvm module contains a lot of x86-specific code. > > Let's move this code into a new arch-specific file in the same directory, > > and rename the arch-independent file to ptp_kvm_common.c. > > > > Reviewed-by: Andre Przywara > > Signed-off-by: Jianyong Wu > > Signed-off-by: Marc Zyngier > > Link: https://lore.kernel.org/r/20201209060932.212364-4-jianyong...@arm.com > > Richard, Paolo, > > Can I get an Ack on this and patch #7? We're getting pretty close to > the next merge window, and this series has been going on for a couple > of years now... For both patches: Acked-by: Richard Cochran
Re: [PATCH net-next 09/12] ionic: add and enable tx and rx timestamp handling
On Tue, Apr 06, 2021 at 04:06:09PM -0700, Shannon Nelson wrote: > Interesting... I doubt that our particular MAC and PHY will ever be > separate, but it makes sense to watch for this in the general case. I've got > an update coming for this. Even if your HW can never support stacked MAC/PHY HW time stamping, still your driver should conform to the correct pattern. Why? Because new driver authors copy/paste stuff they find in the tree. Thanks, Richard
Re: [PATCH net-next 05/12] ionic: add hw timestamp support files
On Tue, Apr 06, 2021 at 04:18:00PM -0700, Shannon Nelson wrote: > On 4/5/21 11:17 AM, Richard Cochran wrote: > > On Mon, Apr 05, 2021 at 09:16:39AM -0700, Shannon Nelson wrote: > > > On 4/4/21 4:05 PM, Richard Cochran wrote: > > > > This check is unneeded, because the ioctl layer never passes NULL here. > > > Yes, the ioctl layer never calls this with NULL, but we call it from > > > within > > > the driver when we spin operations back up after a FW reset. > > So why not avoid the special case and pass a proper request? > > We do this because our firmware reset path is a special case that we have to > handle, and we do so by replaying the previous configuration request. > Passing the NULL request gives the code the ability to watch for this case > while keeping the special case handling simple: the code that drives the > replay logic doesn't need to know the hwstamp details, it just needs to > signal the replay and let the hwstamp code keep track of its own data and > request history. > > I can update the comment to make that replay case more obvious. No, please, I am asking you to provide a hwtstamp_config from your driver. What is so hard about that? Thanks, Richard
Re: [PATCH net-next 12/12] ionic: advertise support for hardware timestamps
On Mon, Apr 05, 2021 at 09:33:46AM -0700, Shannon Nelson wrote: > Yes, I supposed they could have gone together. However, I believe that in a > bisection this will only slightly confuse the user space tools, but won't > cause any kernel pain. Bisection typically involves running tests from user space. The test failing or passing determines whether the commit will be marked GOOD or BAD. This use case is a real thing. Thanks, Richard
Re: [PATCH net-next 09/12] ionic: add and enable tx and rx timestamp handling
On Mon, Apr 05, 2021 at 09:28:44AM -0700, Shannon Nelson wrote: > On 4/4/21 4:41 PM, Richard Cochran wrote: > > On Thu, Apr 01, 2021 at 10:56:07AM -0700, Shannon Nelson wrote: > > > > > @@ -1150,6 +1232,10 @@ netdev_tx_t ionic_start_xmit(struct sk_buff *skb, > > > struct net_device *netdev) > > > return NETDEV_TX_OK; > > > } > > > + if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) > > > + if (lif->hwstamp_txq) > > > + return ionic_start_hwstamp_xmit(skb, netdev); > > The check for SKBTX_HW_TSTAMP and hwstamp_txq is good, but I didn't > > see hwstamp_txq getting cleared in ionic_lif_hwstamp_set() when the > > user turns off Tx time stamping via the SIOCSHWTSTAMP ioctl. > > Once the hwstamp queues are up, we leave them there for future use until the > interface is stopped, Fine, but > assuming that the stack isn't going to send us > SKBTX_HW_STAMP after it has disabled the offload. you can't assume that. This is an important point, especially considering the possibiliy of stacked HW time stamp providers. I'm working on a patch set that will allow the user to switch between MAC and PHY time stamping at run time. Thanks, Richard
Re: [PATCH net-next 05/12] ionic: add hw timestamp support files
On Mon, Apr 05, 2021 at 09:16:39AM -0700, Shannon Nelson wrote: > On 4/4/21 4:05 PM, Richard Cochran wrote: > > This check is unneeded, because the ioctl layer never passes NULL here. > > Yes, the ioctl layer never calls this with NULL, but we call it from within > the driver when we spin operations back up after a FW reset. So why not avoid the special case and pass a proper request? Thanks, Richard
Re: [PATCH net-next 12/12] ionic: advertise support for hardware timestamps
On Thu, Apr 01, 2021 at 10:56:10AM -0700, Shannon Nelson wrote: > Let the network stack know we've got support for timestamping > the packets. Actually, you already advertised the support to user space in Patch 10, so this present patch should go before that one (or together). Thanks, Richard
Re: [PATCH net-next 09/12] ionic: add and enable tx and rx timestamp handling
On Thu, Apr 01, 2021 at 10:56:07AM -0700, Shannon Nelson wrote: > @@ -1150,6 +1232,10 @@ netdev_tx_t ionic_start_xmit(struct sk_buff *skb, > struct net_device *netdev) > return NETDEV_TX_OK; > } > > + if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) > + if (lif->hwstamp_txq) > + return ionic_start_hwstamp_xmit(skb, netdev); The check for SKBTX_HW_TSTAMP and hwstamp_txq is good, but I didn't see hwstamp_txq getting cleared in ionic_lif_hwstamp_set() when the user turns off Tx time stamping via the SIOCSHWTSTAMP ioctl. In addition, the code should set skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS; once the above tests pass. Thanks, Richard
Re: [PATCH net-next 05/12] ionic: add hw timestamp support files
On Thu, Apr 01, 2021 at 10:56:03AM -0700, Shannon Nelson wrote:
> +int ionic_lif_hwstamp_set(struct ionic_lif *lif, struct ifreq *ifr)
> +{
> + struct ionic *ionic = lif->ionic;
> + struct hwtstamp_config config;
> + int tx_mode = 0;
> + u64 rx_filt = 0;
> + int err, err2;
> + bool rx_all;
> + __le64 mask;
> +
> + if (!lif->phc || !lif->phc->ptp)
> + return -EOPNOTSUPP;
> +
> + if (ifr) {
> + if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
> + return -EFAULT;
> + } else {
> + /* if called with ifr == NULL, behave as if called with the
> + * current ts_config from the initial cleared state.
> + */
> + memcpy(&config, &lif->phc->ts_config, sizeof(config));
> + memset(&lif->phc->ts_config, 0, sizeof(config));
> + }
> +
> + tx_mode = ionic_hwstamp_tx_mode(config.tx_type);
> + if (tx_mode < 0)
> + return tx_mode;
> +
> + mask = cpu_to_le64(BIT_ULL(tx_mode));
> + if ((ionic->ident.lif.eth.hwstamp_tx_modes & mask) != mask)
> + return -ERANGE;
> +
> + rx_filt = ionic_hwstamp_rx_filt(config.rx_filter);
> + rx_all = config.rx_filter != HWTSTAMP_FILTER_NONE && !rx_filt;
> +
> + mask = cpu_to_le64(rx_filt);
> + if ((ionic->ident.lif.eth.hwstamp_rx_filters & mask) != mask) {
> + rx_filt = 0;
> + rx_all = true;
> + config.rx_filter = HWTSTAMP_FILTER_ALL;
> + }
> +
> + dev_dbg(ionic->dev, "config_rx_filter %d rx_filt %#llx rx_all %d\n",
> + config.rx_filter, rx_filt, rx_all);
> +
> + mutex_lock(&lif->phc->config_lock);
> +
> + if (tx_mode) {
> + err = ionic_lif_create_hwstamp_txq(lif);
This function NDE yet. It first appears in Patch #6. Please make
sure each patch compiles. That way, bisection always works.
Thanks,
Richard
Re: [PATCH net-next 05/12] ionic: add hw timestamp support files
On Thu, Apr 01, 2021 at 10:56:03AM -0700, Shannon Nelson wrote:
> @@ -0,0 +1,589 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2017 - 2021 Pensando Systems, Inc */
> +
> +#include
> +#include
> +
> +#include "ionic.h"
> +#include "ionic_bus.h"
> +#include "ionic_lif.h"
> +#include "ionic_ethtool.h"
> +
> +static int ionic_hwstamp_tx_mode(int config_tx_type)
> +{
> + switch (config_tx_type) {
> + case HWTSTAMP_TX_OFF:
> + return IONIC_TXSTAMP_OFF;
> + case HWTSTAMP_TX_ON:
> + return IONIC_TXSTAMP_ON;
> + case HWTSTAMP_TX_ONESTEP_SYNC:
> + return IONIC_TXSTAMP_ONESTEP_SYNC;
> +#ifdef HAVE_HWSTAMP_TX_ONESTEP_P2P
> + case HWTSTAMP_TX_ONESTEP_P2P:
> + return IONIC_TXSTAMP_ONESTEP_P2P;
> +#endif
This ifdef is not needed. (I guess you have to support older kernel
versions, but my understanding of the policy is that new code
shouldn't carry such stuff).
> + default:
> + return -ERANGE;
> + }
> +}
> +int ionic_lif_hwstamp_set(struct ionic_lif *lif, struct ifreq *ifr)
> +{
> + struct ionic *ionic = lif->ionic;
> + struct hwtstamp_config config;
> + int tx_mode = 0;
> + u64 rx_filt = 0;
> + int err, err2;
> + bool rx_all;
> + __le64 mask;
> +
> + if (!lif->phc || !lif->phc->ptp)
> + return -EOPNOTSUPP;
> +
> + if (ifr) {
> + if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
> + return -EFAULT;
> + } else {
> + /* if called with ifr == NULL, behave as if called with the
> + * current ts_config from the initial cleared state.
> + */
This check is unneeded, because the ioctl layer never passes NULL here.
> + memcpy(&config, &lif->phc->ts_config, sizeof(config));
> + memset(&lif->phc->ts_config, 0, sizeof(config));
> + }
Thanks,
Richard
Re: [PATCH net-next 02/12] ionic: add handling of larger descriptors
On Thu, Apr 01, 2021 at 10:56:00AM -0700, Shannon Nelson wrote:
> @@ -1722,11 +1722,15 @@ static void ionic_txrx_free(struct ionic_lif *lif)
>
> static int ionic_txrx_alloc(struct ionic_lif *lif)
> {
> - unsigned int sg_desc_sz;
> + unsigned int num_desc, desc_sz, comp_sz, sg_desc_sz;
> unsigned int flags;
> unsigned int i;
Coding Style nit: List of ints wants alphabetically order,
List can also fir 'flags' and 'i' with the others.
> @@ -2246,9 +2258,9 @@ static void ionic_swap_queues(struct ionic_qcq *a,
> struct ionic_qcq *b)
> int ionic_reconfigure_queues(struct ionic_lif *lif,
>struct ionic_queue_params *qparam)
> {
> + unsigned int num_desc, desc_sz, comp_sz, sg_desc_sz;
> struct ionic_qcq **tx_qcqs = NULL;
> struct ionic_qcq **rx_qcqs = NULL;
> - unsigned int sg_desc_sz;
> unsigned int flags;
Ditto.
Thanks,
Richard
Re: [PATCH] ptp_qoriq: fix overflow in ptp_qoriq_adjfine() u64 calcalation
On Tue, Mar 23, 2021 at 04:02:29PM +0800, Yangbo Lu wrote: > Current calculation for diff of TMR_ADD register value may have > 64-bit overflow in this code line, when long type scaled_ppm is > large. > > adj *= scaled_ppm; > > This patch is to resolve it by using mul_u64_u64_div_u64(). > > Signed-off-by: Yangbo Lu Acked-by: Richard Cochran
Re: GTE - The hardware timestamping engine
On Tue, Mar 23, 2021 at 10:03:18AM +0100, Thierry Reding wrote: > I agree. My understanding is the the TSC is basically an SoC-wide clock > that can be (and is) used by several hardware blocks. There's an > interface for software to read out the value, but it's part of a block > called TKE (time-keeping engine, if I recall correctly) that implements > various clock sources and watchdog functionality. ... > Anyway, I think given that the GTE doesn't provide that clock itself but > rather just a means of taking a snapshot of that clock and stamping > certain events with that, it makes more sense to provide that clock from > the TKE driver. It sounds like TKE + GTE together act like a PHC, and GTE doesn't need/want its own SW interface. Thanks, Richard
Re: [Intel-wired-lan] [PATCH next-queue v2 3/3] igc: Add support for PTP getcrosststamp()
On Mon, Mar 22, 2021 at 08:36:26AM -0700, Vinicius Costa Gomes wrote: > After a long time, a couple of internal misunderstandings, fixing some > typos in the delay adjustment constants and better error handling, this > one shot method is working well. > > I will propose a new version, implementing PTP_SYS_OFFSET_PRECISE using > the one shot way. +1
Re: GTE - The hardware timestamping engine
On Sat, Mar 20, 2021 at 01:44:20PM +0100, Arnd Bergmann wrote: > Adding Richard Cochran as well, for drivers/ptp/, he may be able to > identify whether this should be integrated into that framework in some > form. I'm not familiar with the GTE, but it sounds like it is a (free running?) clock with time stamping inputs. If so, then it could expose a PHC. That gets you functionality: - clock_gettime() and friends - comparison ioctl between GTE clock and CLOCK_REALTIME - time stamping channels with programmable input selection The mentioned applications (robotics and autonomous vehicle, so near and dear to my heart) surely already use the PHC API for dealing with network and system time sources, and so exposing the GTE as a PHC means that user space programs will have a consistent API. [ The only drawback I can see is the naming of the C language identifiers in include/uapi/linux/ptp_clock.h. If that bothers people, then these can be changed to something more generic while keeping compatibility aliases. ] Thanks, Richard
Re: [PATCH 0/4] Rid W=1 warnings from PTP
On Fri, Mar 12, 2021 at 11:09:06AM +, Lee Jones wrote: > This set is part of a larger effort attempting to clean-up W=1 > kernel builds, which are currently overwhelmingly riddled with > niggly little warnings. > > Lee Jones (4): > ptp_pch: Remove unused function 'pch_ch_control_read()' > ptp_pch: Move 'pch_*()' prototypes to shared header > ptp: ptp_clockmatrix: Demote non-kernel-doc header to standard comment > ptp: ptp_p: Demote non-conformant kernel-doc headers and supply a > param description For the series: Acked-by: Richard Cochran
Re: [PATCH net-next] net: add a helper to avoid issues with HW TX timestamping and SO_TXTIME
On Wed, Mar 10, 2021 at 04:50:44PM +0200, Vladimir Oltean wrote:
> As explained in commit 29d98f54a4fe ("net: enetc: allow hardware
> timestamping on TX queues with tc-etf enabled"), hardware TX
> timestamping requires an skb with skb->tstamp = 0. When a packet is sent
> with SO_TXTIME, the skb->skb_mstamp_ns corrupts the value of skb->tstamp,
> so the drivers need to explicitly reset skb->tstamp to zero after
> consuming the TX time.
>
> Create a helper named skb_txtime_consumed() which does just that. All
Bikeshedding about the name: "consumed" suggests much more to me than
what is going on.
How about this? skb_reset_txtime();
Thanks,
Richard
Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled
On Thu, Feb 25, 2021 at 10:15:16PM +0100, Heiko Thiery wrote: > When accessing the timecounter register on an i.MX8MQ the kernel hangs. > This is only the case when the interface is down. This can be reproduced > by reading with 'phc_ctrl eth0 get'. > > Like described in the change in 91c0d987a9788dcc5fe26baafd73bf9242b68900 > the igp clock is disabled when the interface is down and leads to a > system hang. > > So we check if the ptp clock status before reading the timecounter > register. > > Signed-off-by: Heiko Thiery > --- > v2: > - add mutex (thanks to Richard) > > v3: > I did a mistake and did not test properly > - add parenteses > - fix the used variable Acked-by: Richard Cochran
Re: [PATCH 1/1] net: fec: ptp: avoid register access when ipg clock is disabled
On Thu, Feb 25, 2021 at 03:05:32PM +0100, Heiko Thiery wrote: > But the explanation why it is currently disabled that way can be found > in the commit 91c0d987a9788dcc5fe26baafd73bf9242b68900. Okay, without re-factoring the entire driver, I agree that the gettime lock up aught to be fixed in a similar way. I missed the original patch, but the diff fragment in this thread doesn't appear to take the mutex as it should. Thanks, Richard
Re: [PATCH 1/1] net: fec: ptp: avoid register access when ipg clock is disabled
On Tue, Feb 23, 2021 at 04:04:16PM +0100, Heiko Thiery wrote: > It is not only the PHC clock that stops. Rather, it is the entire > ethernet building block in the SOC that is disabled, including the > PHC. Sure, but why does the driver do that? Thanks, Richard
Re: [PATCH 1/1] net: fec: ptp: avoid register access when ipg clock is disabled
On Tue, Feb 23, 2021 at 09:00:32AM +0100, Heiko Thiery wrote: > HI Jakub, > > Am Di., 23. Feb. 2021 um 04:00 Uhr schrieb Jakub Kicinski : > > Why is the PTP interface registered when it can't be accessed? > > > > Perhaps the driver should unregister the PTP clock when it's brought > > down? I don't see any reason why a clock should stop ticking just because the interface is down. This is a poor driver design, but sadly it gets copied and even defended. > Good question, but I do not know what happens e.g. with linuxptp when > the device that was opened before will be gone. If a network interface goes down, ptp4l will notice via rtnl and close the interface. Then it re-opens the sockets on rtnl up. However, the file descriptor representing the dynamic posix clock stays opened. Thanks, Richard
Re: [PATCH net-next 2/2] ptp: ptp_clockmatrix: Add alignment of 1 PPS to idtcm_perout_enable.
On Thu, Feb 11, 2021 at 11:38:45PM -0500, vincent.cheng...@renesas.com wrote: > From: Vincent Cheng > > When enabling output using PTP_CLK_REQ_PEROUT, need to align the output > clock to the internal 1 PPS clock. > > Signed-off-by: Vincent Cheng Acked-by: Richard Cochran
Re: [PATCH net-next 1/2] ptp: ptp_clockmatrix: Add wait_for_sys_apll_dpll_lock.
On Thu, Feb 11, 2021 at 11:38:44PM -0500, vincent.cheng...@renesas.com wrote:
> +static int wait_for_sys_apll_dpll_lock(struct idtcm *idtcm)
> +{
> + char *fmt = "%d ms SYS lock timeout: APLL Loss Lock %d DPLL state %d";
Probably you want: const char *fmt
> diff --git a/drivers/ptp/ptp_clockmatrix.h b/drivers/ptp/ptp_clockmatrix.h
> index 645de2c..fb32327 100644
> --- a/drivers/ptp/ptp_clockmatrix.h
> +++ b/drivers/ptp/ptp_clockmatrix.h
> @@ -15,7 +15,6 @@
> #define FW_FILENAME "idtcm.bin"
> #define MAX_TOD (4)
> #define MAX_PLL (8)
> -#define MAX_OUTPUT (12)
...
> @@ -123,7 +137,6 @@ struct idtcm_channel {
> enum pll_mode pll_mode;
> u8 pll;
> u16 output_mask;
> - u8 output_phase_adj[MAX_OUTPUT][4];
> };
Looks like this removal is unrelated to the patch subject, and so it
deserves its own small patch.
Acked-by: Richard Cochran
Re: [PATCH net 2/4] net: mvpp2: Remove unneeded Kconfig dependency.
On Sat, Jan 23, 2021 at 12:12:27PM -0800, Jakub Kicinski wrote: > I see. The only thing I'm worried about then is the churn in patch 3. > This would land in Linus's tree shortly before rc6, kinda late to be > taking chances in the name of minor optimizations :S ;^) Yeah, by all means, avoid ARM churn... I remember Bad Things there... Maybe you could take #1 and #2 for net-next? I should probably submit 3-4 throught the SoC tree anyhow. Thanks, Richard
Re: [PATCH net 2/4] net: mvpp2: Remove unneeded Kconfig dependency.
On Fri, Jan 22, 2021 at 06:14:44PM -0800, Jakub Kicinski wrote:
> (I would put it in net-next tho, given the above this at most a space
> optimization.)
It isn't just about space but also time. The reason why I targeted
net and not net-next was that NETWORK_PHY_TIMESTAMPING activates a
function call to skb_clone_tx_timestamp() for every transmitted frame.
static inline void skb_tx_timestamp(struct sk_buff *skb)
{
skb_clone_tx_timestamp(skb);
if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
skb_tstamp_tx(skb, NULL);
}
In the abscence of a PHY time stamping device, the check for its
presence inside of skb_clone_tx_timestamp() will of course fail, but
this still incurs the cost of the call on every transmitted skb.
Similarly netif_receive_skb() futilely calls skb_defer_rx_timestamp()
on every received skb.
I would argue that most users don't want this option activated by
accident.
(And yes, we could avoid the functions call by moving the check
outside of the global functions and inline to the call sites. I'll be
sure to have that in the shiny new improved scheme under discussion.)
Thanks,
Richard
Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
On Thu, Jan 21, 2021 at 05:22:37PM +0100, Andrew Lunn wrote: > There is a growing interesting in PTP, the number of drivers keeps > going up. The likelihood of MAC/PHY combination having two > timestamping sources is growing all the time. So the stack needs to > change to support the selection of the timestamp source. Fine, but How should the support look like? - New/extended time stamping API that delivers multiple time stamps? - sysctl to select MAC/PHY preference at run time globally? - per-interface ethtool control? - per-socket control? (probably not feasible, but heh) Back of the napkin design ideas appreciated! Thanks, Richard
Re: [PATCH net 0/4] Remove unneeded PHY time stamping option.
On Wed, Jan 20, 2021 at 08:05:59PM -0800, Richard Cochran wrote: > The NETWORK_PHY_TIMESTAMPING configuration option adds additional > checks into the networking hot path, and it is only needed by two > rather esoteric devices, Correction: there are three legitimate users of PHY time stamping... > namely the TI DP83640 PHYTER and the ZHAW > InES 1588 IP core. There is also net/phy/mscc. Thanks, Richard
Re: [PATCH net 2/4] net: mvpp2: Remove unneeded Kconfig dependency.
On Thu, Jan 21, 2021 at 10:27:54AM +, Russell King - ARM Linux admin wrote:
> On Wed, Jan 20, 2021 at 08:06:01PM -0800, Richard Cochran wrote:
> > The mvpp2 is an Ethernet driver, and it implements MAC style time
> > stamping of PTP frames. It has no need of the expensive option to
> > enable PHY time stamping. Remove the incorrect dependency.
> >
> > Signed-off-by: Richard Cochran
> > Fixes: 91dd71950bd7 ("net: mvpp2: ptp: add TAI support")
>
> NAK.
Can you please explain why mvpp2 requires NETWORK_PHY_TIMESTAMING?
Thanks,
Richard
Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
On Thu, Jan 21, 2021 at 10:27:38AM +, Russell King - ARM Linux admin wrote: > As I already explained to you, you can *NOT* use kernel configuration > to make the choice. ARM is a multi-platform kernel, and we will not > stand for platform choices dictated by kernel configuration options. This has nothing to do with ARM as a platform. The same limitation applies to x86 and all the rest. The networking stack does not support simultaneous PHY and MAC time stamping. If you enable both MAC and PHY time stamping, then only one will be reported, and that one is the PHY. Thanks, Richard
Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
On Thu, Jan 14, 2021 at 10:38:00PM +, Russell King - ARM Linux admin wrote: > So, I think the only way to prevent a regression with the code as > it is today is that we _never_ support PTP on Marvell PHYs - because > doing so _will_ break the existing MVPP2 driver's implementation and > cause a regression. The situation isn't as bad as it seems. For one thing, mvpp2 incorrectly selects NETWORK_PHY_TIMESTAMPING. It really shouldn't. I'm submitting a fix soon. As long as the new PHY driver (or at least the PTP bit) depends on NETWORK_PHY_TIMESTAMPING, then that allows users who _really_ want that to enable the option at compile time. This option adds extra checks into the networking hot path, and almost everyone should avoid enabling it. > Right now, there is no option: if a PHY supports PTP, then the only > option is to use the PHYs PTP. Which is utterly rediculous. > > Unless you can see a way around it. Because I can't. I still think the original and best method is to hide the two (and with your new driver, three) esoteric PHY time stamping drivers behind a Kconfig option, and structure the code to favor PHY devices. The idea to favor the MACs, from back in July, doesn't really change the fundamental limitations that - MAC and PHY time stamping cannot work simultaneously, and - Users of PHY devices (representing a tiny minority) must enable the otherwise uninteresting NETWORK_PHY_TIMESTAMPING option at compile time. Thanks, Richard
[PATCH net 4/4] ARM: axm55xx_defconfig: Disable PHY time stamping by default.
The NETWORK_PHY_TIMESTAMPING configuration option adds overhead into the networking stack. When enabled, all transmitted and received frames are subjected to extra tests to determine whether they just might be PTP frames to be presented to esoteric PHY time stamping drivers. However, no System on Chip, least ways not the axm55xx SoC, includes such a PHY time stamping device. Disable the unneeded option by default. Signed-off-by: Richard Cochran --- arch/arm/configs/axm55xx_defconfig | 1 - 1 file changed, 1 deletion(-) diff --git a/arch/arm/configs/axm55xx_defconfig b/arch/arm/configs/axm55xx_defconfig index 46075216ee6d..2d1a45066649 100644 --- a/arch/arm/configs/axm55xx_defconfig +++ b/arch/arm/configs/axm55xx_defconfig @@ -73,7 +73,6 @@ CONFIG_INET_AH=y CONFIG_INET_ESP=y CONFIG_INET_IPCOMP=y # CONFIG_IPV6 is not set -CONFIG_NETWORK_PHY_TIMESTAMPING=y CONFIG_BRIDGE=y # CONFIG_WIRELESS is not set CONFIG_DEVTMPFS=y -- 2.20.1
[PATCH net 2/4] net: mvpp2: Remove unneeded Kconfig dependency.
The mvpp2 is an Ethernet driver, and it implements MAC style time
stamping of PTP frames. It has no need of the expensive option to
enable PHY time stamping. Remove the incorrect dependency.
Signed-off-by: Richard Cochran
Fixes: 91dd71950bd7 ("net: mvpp2: ptp: add TAI support")
---
drivers/net/ethernet/marvell/Kconfig | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/Kconfig
b/drivers/net/ethernet/marvell/Kconfig
index 41815b609569..7fe15a3286f4 100644
--- a/drivers/net/ethernet/marvell/Kconfig
+++ b/drivers/net/ethernet/marvell/Kconfig
@@ -94,7 +94,6 @@ config MVPP2
config MVPP2_PTP
bool "Marvell Armada 8K Enable PTP support"
- depends on NETWORK_PHY_TIMESTAMPING
depends on (PTP_1588_CLOCK = y && MVPP2 = y) || \
(PTP_1588_CLOCK && MVPP2 = m)
--
2.20.1
[PATCH net 3/4] ARM: socfpga_defconfig: Disable PHY time stamping by default.
The NETWORK_PHY_TIMESTAMPING configuration option adds overhead into the networking stack. When enabled, all transmitted and received frames are subjected to extra tests to determine whether they just might be PTP frames to be presented to esoteric PHY time stamping drivers. However, no System on Chip, least ways not the socfpga SoC, includes such a PHY time stamping device. Disable the unneeded option by default. The diff includes a bit of extra churn caused by "make savedefconfig", but the generated defconfig is now in the canonical form. Signed-off-by: Richard Cochran --- arch/arm/configs/socfpga_defconfig | 6 +- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/arch/arm/configs/socfpga_defconfig b/arch/arm/configs/socfpga_defconfig index e73c97b0f5b0..5bd433028285 100644 --- a/arch/arm/configs/socfpga_defconfig +++ b/arch/arm/configs/socfpga_defconfig @@ -14,8 +14,6 @@ CONFIG_ARM_THUMBEE=y CONFIG_SMP=y CONFIG_NR_CPUS=2 CONFIG_HIGHMEM=y -CONFIG_ZBOOT_ROM_TEXT=0x0 -CONFIG_ZBOOT_ROM_BSS=0x0 CONFIG_VFP=y CONFIG_NEON=y CONFIG_OPROFILE=y @@ -33,7 +31,6 @@ CONFIG_IP_PNP=y CONFIG_IP_PNP_DHCP=y CONFIG_IP_PNP_BOOTP=y CONFIG_IP_PNP_RARP=y -CONFIG_NETWORK_PHY_TIMESTAMPING=y CONFIG_VLAN_8021Q=y CONFIG_VLAN_8021Q_GVRP=y CONFIG_CAN=y @@ -48,12 +45,10 @@ CONFIG_DEVTMPFS=y CONFIG_DEVTMPFS_MOUNT=y CONFIG_MTD=y CONFIG_MTD_BLOCK=y -CONFIG_MTD_M25P80=y CONFIG_MTD_RAW_NAND=y CONFIG_MTD_NAND_DENALI_DT=y CONFIG_MTD_SPI_NOR=y # CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is not set -CONFIG_SPI_CADENCE_QUADSPI=y CONFIG_OF_OVERLAY=y CONFIG_BLK_DEV_LOOP=y CONFIG_BLK_DEV_RAM=y @@ -89,6 +84,7 @@ CONFIG_I2C=y CONFIG_I2C_CHARDEV=y CONFIG_I2C_DESIGNWARE_PLATFORM=y CONFIG_SPI=y +CONFIG_SPI_CADENCE_QUADSPI=y CONFIG_SPI_DESIGNWARE=y CONFIG_SPI_DW_MMIO=y CONFIG_SPI_SPIDEV=y -- 2.20.1
[PATCH net 0/4] Remove unneeded PHY time stamping option.
The NETWORK_PHY_TIMESTAMPING configuration option adds additional checks into the networking hot path, and it is only needed by two rather esoteric devices, namely the TI DP83640 PHYTER and the ZHAW InES 1588 IP core. Very few end users have these devices, and those that do have them are building specialized embedded systems. Unfortunately two unrelated drivers depend on this option, and two defconfigs enable it. It is probably my fault for not paying enough attention in reviews. This series corrects the gratuitous use of NETWORK_PHY_TIMESTAMPING. Richard Cochran (4): net: dsa: mv88e6xxx: Remove bogus Kconfig dependency. net: mvpp2: Remove unneeded Kconfig dependency. ARM: socfpga_defconfig: Disable PHY time stamping by default. ARM: axm55xx_defconfig: Disable PHY time stamping by default. arch/arm/configs/axm55xx_defconfig | 1 - arch/arm/configs/socfpga_defconfig | 6 +- drivers/net/dsa/mv88e6xxx/Kconfig| 1 - drivers/net/ethernet/marvell/Kconfig | 1 - 4 files changed, 1 insertion(+), 8 deletions(-) -- 2.20.1
[PATCH net 1/4] net: dsa: mv88e6xxx: Remove bogus Kconfig dependency.
The mv88e6xxx is a DSA driver, and it implements DSA style time
stamping of PTP frames. It has no need of the expensive option to
enable PHY time stamping. Remove the bogus dependency.
Signed-off-by: Richard Cochran
Fixes: 2fa8d3af4bad ("net: dsa: mv88e6xxx: expose switch time as a PTP hardware
clock")
---
drivers/net/dsa/mv88e6xxx/Kconfig | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/dsa/mv88e6xxx/Kconfig
b/drivers/net/dsa/mv88e6xxx/Kconfig
index 51185e4d7d15..b17540926c11 100644
--- a/drivers/net/dsa/mv88e6xxx/Kconfig
+++ b/drivers/net/dsa/mv88e6xxx/Kconfig
@@ -25,7 +25,6 @@ config NET_DSA_MV88E6XXX_PTP
default n
depends on NET_DSA_MV88E6XXX_GLOBAL2
depends on PTP_1588_CLOCK
- imply NETWORK_PHY_TIMESTAMPING
help
Say Y to enable PTP hardware timestamping on Marvell 88E6xxx switch
chips that support it.
--
2.20.1
Re: [PATCH net-next v4 2/2] net: flow_dissector: Parse PTP L2 packet header
On Tue, Jan 12, 2021 at 09:07:13PM +0200, Eran Ben Elisha wrote: > Add support for parsing PTP L2 packet header. Such packet consists > of an L2 header (with ethertype of ETH_P_1588), PTP header, body > and an optional suffix. > > Signed-off-by: Eran Ben Elisha > Reviewed-by: Tariq Toukan Acked-by: Richard Cochran
Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
On Thu, Jan 14, 2021 at 01:32:35PM +, Russell King - ARM Linux admin wrote: > > We had already discussed this patch last year, and you agreed with it > > then. What has changed? > > See the discussion in this sub-thread: > > https://lore.kernel.org/netdev/20200729105807.gz1...@shell.armlinux.org.uk/ Thanks for the reminder. We ended up with having to review the MAC drivers that support phydev. https://lore.kernel.org/netdev/20200730194427.ge1...@shell.armlinux.org.uk/ There is at least the FEC that supports phydev. I have a board that combines the FEC with the dp83640 PHYTER, and your patch would break this setup. (In the case of this HW combination, the PHYTER is superior in every way.) Another combination that I have seen twice is the TI am335x with its cpsw MAC and the PHYTER. Unfortunately I don't have one of these boards, but people made them because the cpsw MAC supports time stamping in a way that is inadequate. I *think* the cpsw/phyter combination would work with your patch, but only if the users disable CONFIG_TI_CPTS at compile time. Thanks, Richard
Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
On Sun, Jan 10, 2021 at 11:13:44AM +, Russell King wrote: > This allows network drivers such as mvpp2 to use their more accurate > timestamping implementation than using a less accurate implementation > in the PHY. Network drivers can opt to defer to phylib by returning > -EOPNOTSUPP. My expectation is that PHY time stamping is more accurate than MAC time stamping. > This change will be needed if the Marvell PHY drivers add support for > PTP. Huh? The mvpp2 appears to be a MAC. If this device has integrated PHYs then I don't see the issue. If your board has the nvpp2 device with the dp83640 PHYTER, then don't you want to actually use the PHYTER? >From my observation of the product offerings, I have yet to see a new PHY (besides the dp83640 PHYTER) that implement time stamping. The PHYTER is 100 megabit only, and my understanding that it is too difficult or even impossible to provide time stamps from within a gigabit+ PHY. So you needn't fear new time stamping PHYs to spoil your setup! > Note: this may cause a change for any drivers that use phylib and > provide get_ts_info(). It is not obvious if any such cases exist. Up until now, the code always favored PHY devices and devices external to the MAC that snoop on the MII bus. The assumption is that anyone who builds a board with such specialty devices really wants to use them. Thanks, Richard
Re: [PATCH net-next v3 2/2] net: flow_dissector: Parse PTP L2 packet header
On Mon, Jan 11, 2021 at 08:17:48PM +0200, Eran Ben Elisha wrote:
> Add support for parsing PTP L2 packet header. Such packet consists
> of an L2 header (with ethertype of ETH_P_1588), PTP header, body
> and an optional suffix.
>
> Signed-off-by: Eran Ben Elisha
> Reviewed-by: Tariq Toukan
> ---
> net/core/flow_dissector.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 6f1adba6695f..fcaa223c7cdc 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -1251,6 +1252,21 @@ bool __skb_flow_dissect(const struct net *net,
> &proto, &nhoff, hlen, flags);
> break;
>
> + case htons(ETH_P_1588): {
> + struct ptp_header *hdr, _hdr;
> +
> + hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data,
> +hlen, &_hdr);
> + if (!hdr || (hlen - nhoff) < sizeof(_hdr)) {
I'm not really familiar with the flow dissector, but why check (hlen - nhoff)
here?
None of the other cases do that. Doesn't skb_copy_bits() in
__skb_header_pointer() already handle that?
Thanks,
Richard
> + fdret = FLOW_DISSECT_RET_OUT_BAD;
> + break;
> + }
> +
> + nhoff += ntohs(hdr->message_length);
> + fdret = FLOW_DISSECT_RET_OUT_GOOD;
> + break;
> + }
> +
> default:
> fdret = FLOW_DISSECT_RET_OUT_BAD;
> break;
> --
> 2.17.1
>
Re: [PATCH v3 1/1] can: dev: add software tx timestamps
On Tue, Jan 12, 2021 at 09:00:33AM +0900, Vincent MAILHOL wrote: > Out of curiosity, which programs do you use? I guess wireshark > but please let me know if you use any other programs (I just use > to write a small C program to do the stuff). I was thinking of PTP over DeviceNET (which, in turn, is over CAN). This is specified in Annex G of IEEE 1588. The linuxptp stack has modular design and could one day support DeviceNET. It would be much easier for linuxptp if CAN interfaces support hardware time stamping in the same way as other network interfaces. Thanks, Richard
Re: [PATCH v3 1/1] can: dev: add software tx timestamps
On Sun, Jan 10, 2021 at 09:49:03PM +0900, Vincent Mailhol wrote: > * The hardware rx timestamp of a local loopback message is the > hardware tx timestamp. This means that there are no needs to > implement SOF_TIMESTAMPING_TX_HARDWARE for CAN sockets. I can't agree with that statement. The local loopback is a special "feature" of CAN sockets, and some programs turn it off. Furthermore, requiring user space to handle CAN sockets differently WRT Tx time stamps is user-unfriendly. So I would strongly support adding SOF_TIMESTAMPING_TX_HARDWARE to the CAN layer in the future. (This isn't a criticism of the current patch, though.) Thanks, Richard
Re: [PATCH] ptp: ptp_ines: prevent build when HAS_IOMEM is not set
On Tue, Jan 05, 2021 at 08:25:31PM -0800, Randy Dunlap wrote: > ptp_ines.c uses devm_platform_ioremap_resource(), which is only > built/available when CONFIG_HAS_IOMEM is enabled. > CONFIG_HAS_IOMEM is not enabled for arch/s390/, so builds on S390 > have a build error: > > s390-linux-ld: drivers/ptp/ptp_ines.o: in function `ines_ptp_ctrl_probe': > ptp_ines.c:(.text+0x17e6): undefined reference to > `devm_platform_ioremap_resource' > > Prevent builds of ptp_ines.c when HAS_IOMEM is not set. Acked-by: Richard Cochran
Re: [PATCH 2/7] phy: dp83640: select CONFIG_CRC32
On Sun, Jan 03, 2021 at 10:36:18PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> Without crc32, this driver fails to link:
>
> arm-linux-gnueabi-ld: drivers/net/phy/dp83640.o: in function `match':
> dp83640.c:(.text+0x476c): undefined reference to `crc32_le'
>
> Fixes: 539e44d26855 ("dp83640: Include hash in timestamp/packet matching")
> Signed-off-by: Arnd Bergmann
Acked-by: Richard Cochran
Re: [PATCH rfc 3/3] virtio-net: support transmit timestamp
On Mon, Dec 28, 2020 at 11:22:33AM -0500, Willem de Bruijn wrote: > From: Willem de Bruijn > > Add optional delivery time (SO_TXTIME) offload for virtio-net. > > The Linux TCP/IP stack tries to avoid bursty transmission and network > congestion through pacing: computing an skb delivery time based on > congestion information. Userspace protocol implementations can achieve > the same with SO_TXTIME. This may also reduce scheduling jitter and > improve RTT estimation. This description is clear, but the Subject line is confusing. It made me wonder whether this series is somehow about host/guest synchronization (but your comments do explain that that isn't the case). How about this instead? virtio-net: support future packet transmit time Thanks, Richard
Re: [PATCH rfc 2/3] virtio-net: support receive timestamp
On Mon, Dec 28, 2020 at 07:57:20PM -0500, Willem de Bruijn wrote: > There is a well understood method for synchronizing guest and host > clock in KVM using ptp_kvm. For virtual environments without NIC > hardware offload, the when host timestamps in software, this suffices. > > Syncing host with NIC is assumed if the host advertises the feature > and implements using real hardware timestamps. Sounds reasonable. Thanks, Richard
Re: [PATCH net] net: ethernet: ti: cpts: fix ethtool output when no ptp_clock registered
On Thu, Dec 24, 2020 at 06:24:05PM +0200, Grygorii Strashko wrote:
> The CPTS driver registers PTP PHC clock when first netif is going up and
> unregister it when all netif are down. Now ethtool will show:
> - PTP PHC clock index 0 after boot until first netif is up;
> - the last assigned PTP PHC clock index even if PTP PHC clock is not
> registered any more after all netifs are down.
>
> This patch ensures that -1 is returned by ethtool when PTP PHC clock is not
> registered any more.
>
> Fixes: 8a2c9a5ab4b9 ("net: ethernet: ti: cpts: rework
> initialization/deinitialization")
> Signed-off-by: Grygorii Strashko
Acked-by: Richard Cochran
Re: [PATCH v4 net-next 2/2] net: phy: mchp: Add 1588 support for LAN8814 Quad PHY
On Thu, Dec 17, 2020 at 06:11:50PM +0530, Divya Koppera wrote:
> +struct lan8814_ptphdr {
> + u8 tsmt; /* transportSpecific | messageType */
> + u8 ver; /* reserved0 | versionPTP */
> + __be16 msglen;
> + u8 domain;
> + u8 rsrvd1;
> + __be16 flags;
> + __be64 correction;
> + __be32 rsrvd2;
> + __be64 clk_identity;
> + __be16 src_port_id;
> + __be16 seq_id;
> + u8 ctrl;
> + u8 log_interval;
> +} __attribute__((__packed__));
Please use the new 'struct ptp_header' from ptp_classify.h instead.
> +struct kszphy_ptp_priv {
> + struct mii_timestamper mii_ts;
> + struct phy_device *phydev;
> + struct lan8814_ptp ptp;
> + int hwts_tx_en;
> + int hwts_rx_en;
> + int layer;
> + int version;
> +};
> +
> struct kszphy_priv {
> + struct kszphy_ptp_priv ptp_priv;
> const struct kszphy_type *type;
> int led_mode;
> bool rmii_ref_clk_sel;
> @@ -171,6 +313,38 @@ static const struct kszphy_type ksz9021_type = {
> .interrupt_level_mask = BIT(14),
> };
>
> +static void lan8814_ptp_clock_get(struct phy_device *phydev,
> + u32 *seconds, u32 *nano_seconds,
> + u32 *sub_nano_seconds);
> +
> +static int lan8814_read_page_reg(struct phy_device *phydev,
> + int page, u32 addr)
> +{
> + u32 data;
> +
> + phy_write(phydev, KSZ_EXT_PAGE_ACCESS_CONTROL, page);
> + phy_write(phydev, KSZ_EXT_PAGE_ACCESS_ADDRESS_DATA, addr);
> + phy_write(phydev, KSZ_EXT_PAGE_ACCESS_CONTROL, (page | 0x4000));
What prevents concurrent reads from happening? Nothing, AFAICT.
> + data = phy_read(phydev, KSZ_EXT_PAGE_ACCESS_ADDRESS_DATA);
> +
> + return data;
> +}
> +
> +static int lan8814_write_page_reg(struct phy_device *phydev,
> + int page, u16 addr, u16 val)
> +{
> + phy_write(phydev, KSZ_EXT_PAGE_ACCESS_CONTROL, page);
Consider caching the page value in order to make a sequence of
reads/writes more efficient. (example in dp83640.c)
> + phy_write(phydev, KSZ_EXT_PAGE_ACCESS_ADDRESS_DATA, addr);
> + phy_write(phydev, KSZ_EXT_PAGE_ACCESS_CONTROL, (page | 0x4000));
Again, this needs some kind of serialization. Maybe not here but
rather in the callers... you need to sort that out.
> + val = phy_write(phydev, KSZ_EXT_PAGE_ACCESS_ADDRESS_DATA, val);
> + if (val != 0) {
> + phydev_err(phydev, "Error: phy_write has returned error %d\n",
> +val);
> + return val;
> + }
> + return 0;
> +}
> +
> static int kszphy_extended_write(struct phy_device *phydev,
> u32 regnum, u16 val)
> {
> @@ -185,10 +359,156 @@ static int kszphy_extended_read(struct phy_device
> *phydev,
> return phy_read(phydev, MII_KSZPHY_EXTREG_READ);
> }
>
> +static void lan8814_ptp_tx_ts_get(struct phy_device *phydev,
> + u32 *seconds, u32 *nano_seconds,
> + u32 *seq_id);
> +
> +static struct lan8814_ptphdr *get_ptp_header_l4(struct sk_buff *skb,
> + struct iphdr *iphdr,
> + struct udphdr *udphdr)
> +{
> + if (iphdr->version != 4 || iphdr->protocol != IPPROTO_UDP)
> + return NULL;
> +
> + return (struct lan8814_ptphdr *)(((unsigned char *)udphdr) + UDP_HLEN);
> +}
> +
> +static struct lan8814_ptphdr *get_ptp_header_tx(struct sk_buff *skb)
> +{
> + struct ethhdr *ethhdr = eth_hdr(skb);
> + struct udphdr *udphdr;
> + struct iphdr *iphdr;
> +
> + if (ethhdr->h_proto == htons(ETH_P_1588))
> + return (struct lan8814_ptphdr *)(((unsigned char *)ethhdr) +
> + skb_mac_header_len(skb));
> +
> + if (ethhdr->h_proto != htons(ETH_P_IP))
> + return NULL;
> +
> + iphdr = ip_hdr(skb);
> + udphdr = udp_hdr(skb);
> +
> + return get_ptp_header_l4(skb, iphdr, udphdr);
Use ptp_parse_header() from ptp_classify.h
> +}
> +
> +static int get_sig(struct sk_buff *skb, u8 *sig)
> +{
> + struct lan8814_ptphdr *ptphdr = get_ptp_header_tx(skb);
> + struct ethhdr *ethhdr = eth_hdr(skb);
> + unsigned int i;
> +
> + if (!ptphdr)
> + return -EOPNOTSUPP;
> +
> + sig[0] = (__force u16)ptphdr->seq_id >> 8;
> + sig[1] = (__force u16)ptphdr->seq_id & GENMASK(7, 0);
> + sig[2] = ptphdr->domain;
> + sig[3] = ptphdr->tsmt & GENMASK(3, 0);
> +
> + memcpy(&sig[4], ethhdr->h_dest, ETH_ALEN);
> +
> + /* Fill the last bytes of the signature to reach a 16B signature */
> + for (i = 10; i < 16; i++)
> + sig[i] = ptphdr->tsmt & GENMASK(3, 0);
> +
> + return 0;
> +}
> +
> +static void lan8814_dequeue_skb(struct lan8814_ptp *ptp)
> +{
> + struct kszphy_ptp_priv *priv = container_of(ptp, struct
> kszphy_ptp_priv, ptp);
> + struct phy_device *phyd
Re: [PATCH 1/2] net: stmmac: retain PTP-clock at hwtstamp_set
On Wed, Dec 16, 2020 at 05:13:34PM -0800, Jakub Kicinski wrote: > On Wed, 16 Dec 2020 12:32:38 +0100 Holger Assmann wrote: > > As it is, valid SIOCSHWTSTAMP ioctl calls - i.e. enable/disable time > > stamping or changing filter settings - lead to synchronization of the > > NIC's hardware clock with CLOCK_REALTIME. This might be necessary > > during system initialization, but at runtime, when the PTP clock has > > already been synchronized to a grand master, a reset of the timestamp > > settings might result in a clock jump. +1 for keeping the PHC time continuous. > Please remember to CC Richard, the PTP maintainer. +1 to that, too! Thanks, Richard
Re: [PATCH net-next 1/4] ptp: clockmatrix: reset device and check BOOT_STATUS
On Tue, Dec 08, 2020 at 10:41:54AM -0500, min.li...@renesas.com wrote: > From: Min Li > > SM_RESET device only when loading full configuration and check > for BOOT_STATUS. Also remove polling for write trigger done in > _idtcm_settime(). > > Changes since v1: > -Correct warnings from strict checkpatch Next time, please put "v2" in the Subject line. That helps reviewers keep track. > Signed-off-by: Min Li Acked-by: Richard Cochran
Re: [net-next V2 08/15] net/mlx5e: Add TX PTP port object support
On Mon, Dec 07, 2020 at 12:42:33PM -0800, Jakub Kicinski wrote: > The behavior is not entirely dissimilar to the time stamps on > multi-layered devices (e.g. DSA switches). The time stamp can either > be generated when the packet enters the device (current mlx5 behavior) > or when it actually egresses thru the MAC (what this set adds). To be useful, the time stamps must be taken on the external ports. Generating the time stamp at the DMA reception in the device doesn't even make sense, unless the delay through the device is constant. > My main concern is the user friendliness. I think there is no question > that user running ptp4l would want this mlx5 knob to be enabled. Right. > Would > we rather see a patch to ptp4l that turns per driver knob or should we > shoot for some form of an API that tells the kernel that we're > expecting ns level time accuracy? This is a hardware-specific "feature". One of the guiding principles of the linuxptp user space stack is not to become a catalog of workarounds for random hardware. IMO the kernel's API should not encourage "special" hardware either. After all, we have lots and lots of PTP hardware supported, all of them already working with the existing API just fine. My preference is for a global knob for users of this hardware, either - a compile time Kconfig option on the driver, or - some kind of sysctl/debugfs knob Thanks, Richard
Re: [net-next V2 08/15] net/mlx5e: Add TX PTP port object support
On Mon, Dec 07, 2020 at 12:37:45AM -0800, Saeed Mahameed wrote: > we are not adding any new mechanism. Sorry, I didn't catch the beginning of this thread. Are you proposing adding HWTSTAMP_TX_ON_TIME_CRITICAL_ONLY to net_tstamp.h ? If so, then ... > Our driver feature is and internal enhancement yes, but the suggested > flag is very far from indicating any internal enhancement, is actually > an enhancement to the current API, and is a very simple extension with > wide range of improvements to all layers. No, that would be no enhancement but rather a hack for poorly designed hardware. > Our driver can optimize accuracy when this flag is set, other drivers > might be happy to implement it since they already have a slow hw Name three other drivers that would "be happy" to implement this. Can you name even one other? Thanks, Richard
Re: [net-next V2 08/15] net/mlx5e: Add TX PTP port object support
On Sun, Dec 06, 2020 at 03:37:47PM +0200, Eran Ben Elisha wrote: > Adding new enum to the ioctl means we have add > (HWTSTAMP_TX_ON_TIME_CRITICAL_ONLY for example) all the way - drivers, > kernel ptp, user space ptp, ethtool. > > My concerns are: > 1. Timestamp applications (like ptp4l or similar) will have to add support > for configuring the driver to use HWTSTAMP_TX_ON_TIME_CRITICAL_ONLY if > supported via ioctl prior to packets transmit. From application point of > view, the dual-modes (HWTSTAMP_TX_ON_TIME_CRITICAL_ONLY , HWTSTAMP_TX_ON) > support is redundant, as it offers nothing new. Well said. > 2. Other vendors will have to support it as well, when not sure what is the > expectation from them if they cannot improve accuracy between them. If there were multiple different devices out there with this kind of implementation (different levels of accuracy with increasing run time performance cost), then we could consider such a flag. However, to my knowledge, this feature is unique to your device. > This feature is just an internal enhancement, and as such it should be added > only as a vendor private configuration flag. We are not offering here about > any standard for others to follow. +1 Thanks, Richard
Re: [net-next V2 08/15] net/mlx5e: Add TX PTP port object support
On Sat, Dec 05, 2020 at 03:49:27AM +0200, Vladimir Oltean wrote: > So there you go, it just says "the reference plane marking the boundary > between the PTP node and the network". So it depends on how you draw the > borders. It depends on the physical link technology. You can't just "draw the borders" anywhere you like! Thanks, Richard
Re: [PATCH 1/1 v3 net-next] ptp: Add clock driver for the OpenCompute TimeCard.
On Thu, Dec 03, 2020 at 07:51:28PM -0800, Jonathan Lemon wrote: > The OpenCompute time card is an atomic clock along with > a GPS receiver that provides a Grandmaster clock source > for a PTP enabled network. > > More information is available at http://www.timingcard.com/ > > Signed-off-by: Jonathan Lemon Acked-by: Richard Cochran
Re: [PATCH v2 net-next] ptp: Add clock driver for the OpenCompute TimeCard.
On Thu, Dec 03, 2020 at 06:00:37PM -0800, Jonathan Lemon wrote: > On Thu, Dec 03, 2020 at 04:56:24PM -0800, Richard Cochran wrote: > > The name here is a bit confusing since "timex" has a special meaning > > in the NTP/PTP API. > > The .gettimex64 call is used here so the time returned from the > clock can be correlated to the system time. [facepalm] man, its in the interface naming. Oh well. > > This driver looks fine, but I'm curious how you will use it. Can it > > provide time stamping for network frames or other IO? > > The card does have a PPS pulse output, so it can be wired to a network > card which takes an external PPS signal. Cool. So the new ts2phc program can synchronize the NICs for you. All that is missing here is ptp_clock_info.enable(). With that in place, it will work out of the box. > Right now, the current model (which certainly can be improved on) is using > phc2sys to discipline the system and NIC clocks. Yeah, ts2phc will fill in the gap. > I'll send a v3. I also need to open a discussion on how this should > return the leap second changes to the user - there doesn't seem to be > anything in the current API for this. There is the clock_adjtime() system call, analogue of adjtimex. Right now there is no PHC driver access to the timex.status field (which carries the leap flags), but that can be changed... kernel/time/posix-timers.c do_clock_adjtime() calls kernel/time/posix-clock.c pc_clock_adjtime() calls drivers/ptp/ptp_clock.c ptp_clock_adjtime() At this point the PHC layer could invoke a new, optional driver callback that returns the leap second status flags, and then the PHC layer could set timex.status appropriately. Having said all that, the ts2phc program takes the approach of getting the leap second information from the leap seconds file. Of course, this requires administratively updating the file at least once every six months. I considered and rejected the idea of trying to get the current leap second status from GPS for two reasons. First, there is no standardized and universally implemented way of querying this from a GPS. Second, and more importantly, the leap second information is only broadcast every 12.5 minutes, and that it is *way* too long to wait after cold boot for applications I am interested in. So the choice boiled down to either having to keep a file up to date, say every month or so, or waiting 15 minutes in the worst case. I chose the lesser of two evils. Thanks, Richard
Re: [PATCH net-next v5 9/9] net: dsa: microchip: ksz9477: add periodic output support
On Fri, Dec 04, 2020 at 03:00:50AM +0200, Vladimir Oltean wrote: > On Thu, Dec 03, 2020 at 04:45:56PM -0800, Richard Cochran wrote: > > Yes, that would make sense. It would bring sysfs back to feature > > parity with the ioctls. > > Which is a good thing? Yes, of course it is. I'm sorry I didn't insist on it in the first place! > Anyway, Christian, if you do decide to do that, here's some context why > I didn't do it when I added the additional knobs for periodic output: > https://www.mail-archive.com/linuxptp-devel@lists.sourceforge.net/msg04150.html I think Christian is proposing a different sysfs file, not a flag in the existing ones. That makes sense to me. Thanks, Richard
Re: [PATCH v2 net-next] ptp: Add clock driver for the OpenCompute TimeCard.
On Thu, Dec 03, 2020 at 10:29:25AM -0800, Jonathan Lemon wrote:
> The OpenCompute time card is an atomic clock along with
> a GPS receiver that provides a Grandmaster clock source
> for a PTP enabled network.
>
> More information is available at http://www.timingcard.com/
>
> Signed-off-by: Jonathan Lemon
What changed in v2?
(please include a change log for future patches)
> +static int
> +ptp_ocp_gettimex(struct ptp_clock_info *ptp_info, struct timespec64 *ts,
> + struct ptp_system_timestamp *sts)
> +{
The name here is a bit confusing since "timex" has a special meaning
in the NTP/PTP API.
Suggest plain old ptp_ocp_gettime();
> + struct ptp_ocp *bp = container_of(ptp_info, struct ptp_ocp, ptp_info);
> + unsigned long flags;
> + int err;
> +
> + spin_lock_irqsave(&bp->lock, flags);
> + err = __ptp_ocp_gettime_locked(bp, ts, sts);
> + spin_unlock_irqrestore(&bp->lock, flags);
> +
> + return err;
> +}
> +
> +static void
> +__ptp_ocp_settime_locked(struct ptp_ocp *bp, const struct timespec64 *ts)
> +{
> + u32 ctrl, time_sec, time_ns;
> + u32 select;
> +
> + time_ns = ts->tv_nsec;
> + time_sec = ts->tv_sec;
> +
> + select = ioread32(&bp->reg->select);
> + iowrite32(OCP_SELECT_CLK_REG, &bp->reg->select);
> +
> + iowrite32(time_ns, &bp->reg->adjust_ns);
> + iowrite32(time_sec, &bp->reg->adjust_sec);
> +
> + ctrl = ioread32(&bp->reg->ctrl);
> + ctrl |= OCP_CTRL_ADJUST_TIME;
> + iowrite32(ctrl, &bp->reg->ctrl);
> +
> + /* restore clock selection */
> + iowrite32(select >> 16, &bp->reg->select);
> +}
> +
> +static int
> +ptp_ocp_settime(struct ptp_clock_info *ptp_info, const struct timespec64 *ts)
> +{
> + struct ptp_ocp *bp = container_of(ptp_info, struct ptp_ocp, ptp_info);
> + unsigned long flags;
> +
> + if (ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC)
> + return 0;
> +
> + dev_info(&bp->pdev->dev, "settime to: %lld.%ld\n",
> + ts->tv_sec, ts->tv_nsec);
No need for this dmesg spam. Change to _debug if you really want to
keep it.
> + spin_lock_irqsave(&bp->lock, flags);
> + __ptp_ocp_settime_locked(bp, ts);
> + spin_unlock_irqrestore(&bp->lock, flags);
> +
> + return 0;
> +}
> +static int
> +ptp_ocp_null_adjfine(struct ptp_clock_info *ptp_info, long scaled_ppm)
> +{
> + struct ptp_ocp *bp = container_of(ptp_info, struct ptp_ocp, ptp_info);
> +
> + if (scaled_ppm == 0)
> + return 0;
> +
> + dev_info(&bp->pdev->dev, "adjfine, scaled by: %ld\n", scaled_ppm);
No need for this either.
> + return -EOPNOTSUPP;
You can set the offset but not the frequency adjustment. Makes sense
for an atomic clock.
> +}
This driver looks fine, but I'm curious how you will use it. Can it
provide time stamping for network frames or other IO?
If not, then that seems unfortunate. Still you can regulate the Linux
system clock in software, but that of course introduces time error.
Thanks,
Richard
Re: [PATCH net-next v5 9/9] net: dsa: microchip: ksz9477: add periodic output support
On Thu, Dec 03, 2020 at 04:36:26PM +0100, Christian Eggers wrote: > Should ptp_sysfs be extended with a "pulse" attribute with calls > enable() with only PTP_PEROUT_DUTY_CYCLE set? Yes, that would make sense. It would bring sysfs back to feature parity with the ioctls. Thanks, Richard
Re: [PATCH net-next v5 9/9] net: dsa: microchip: ksz9477: add periodic output support
On Thu, Dec 03, 2020 at 11:21:17AM +0100, Christian Eggers wrote: > The KSZ9563 has a Trigger Output Unit (TOU) which can be used to > generate periodic signals. > > The pulse length can be altered via a device attribute. Device tree is the wrong place for that. Aren't you using PTP_PEROUT_DUTY_CYCLE anyhow? Thanks, Richard
Re: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
On Mon, Nov 30, 2020 at 09:01:25PM +, tristram...@microchip.com wrote: > The 1588 PTP engine in the KSZ switches was designed to be controlled closely > by > a PTP stack, so it is a little difficult to use when there is a layer of > kernel support > between the application and the driver. Are you saying that linuxptp is not a PTP stack? Maybe it would be wiser to design your HW so that it can work under Linux? Nah, nobody cares about Linux support these days.
Re: [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp
On Tue, Nov 24, 2020 at 11:01:26AM -0500, min.li...@renesas.com wrote: > From: Min Li > > Feed kstrtou8 with NULL terminated string. > > Changes since v1: > -Use sscanf to get rid of adhoc string parse. This is much nicer. Small issue remains... > + u8 ver1[3], ver2[3]; > + int i; > + > + if (sscanf(version1, "%hhu.%hhu.%hhu", > +&ver1[0], &ver1[1], &ver1[2]) < 0) > + return -1; The sscanf function returns the number of scanned items, and so you should check that it returns 3 (three). > + if (sscanf(version2, "%hhu.%hhu.%hhu", > +&ver2[0], &ver2[1], &ver2[2]) < 0) > + return -1; Same here. Thanks, Richard
Re: [PATCH net-next v2 0/3] net: ptp: use common defines for PTP message types in further drivers
On Tue, Nov 24, 2020 at 08:44:15AM +0100, Christian Eggers wrote: > Changes in v2: > > - resend, as v1 was sent before the prerequisites were merged > - removed mismatch between From: and Signed-off-by: > - [2/3] Reviewed-by: Ido Schimmel > - [3/3] Reviewed-by: Antoine Tenart > - [3/3] removed dead email addresses from Cc: > > > This series replaces further driver internal enumeration / uses of magic > numbers with the newly introduced PTP_MSGTYPE_* defines. For the series: Acked-by: Richard Cochran
Re: [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp
On Mon, Nov 23, 2020 at 03:20:06PM -0500, min.li...@renesas.com wrote:
> From: Min Li
>
> Feed kstrtou8 with NULL terminated string.
>
> Changes since v1:
> -Use strscpy instead of strncpy for safety.
>
> Signed-off-by: Min Li
> ---
> drivers/ptp/ptp_clockmatrix.c | 60
> ++-
> tools/bpf/example | 12 +
> tools/bpf/novlan | 7 +
> 3 files changed, 61 insertions(+), 18 deletions(-)
> create mode 100644 tools/bpf/example
> create mode 100644 tools/bpf/novlan
>
> diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
> index e020faf..d4e434b 100644
> --- a/drivers/ptp/ptp_clockmatrix.c
> +++ b/drivers/ptp/ptp_clockmatrix.c
> @@ -103,42 +103,66 @@ static int timespec_to_char_array(struct timespec64
> const *ts,
> return 0;
> }
>
> -static int idtcm_strverscmp(const char *ver1, const char *ver2)
> +static int idtcm_strverscmp(const char *version1, const char *version2)
> {
> u8 num1;
> u8 num2;
> int result = 0;
> + char ver1[16];
> + char ver2[16];
> + char *cur1;
> + char *cur2;
> + char *next1;
> + char *next2;
> +
> + if (strscpy(ver1, version1, 16) < 0 ||
> + strscpy(ver2, version2, 16) < 0)
> + return -1;
> + cur1 = ver1;
> + cur2 = ver2;
>
> /* loop through each level of the version string */
> while (result == 0) {
> + next1 = strchr(cur1, '.');
> + next2 = strchr(cur2, '.');
> +
> + /* kstrtou8 could fail for dot */
> + if (next1) {
> + *next1 = '\0';
> + next1++;
> + }
> +
> + if (next2) {
> + *next2 = '\0';
> + next2++;
> + }
> +
All of this looping and ad-hoc string parsing can be make MUCH
simpler by using sscanf() and then comparing the binary values
directly.
> /* extract leading version numbers */
> - if (kstrtou8(ver1, 10, &num1) < 0)
> + if (kstrtou8(cur1, 10, &num1) < 0)
> return -1;
>
> - if (kstrtou8(ver2, 10, &num2) < 0)
> + if (kstrtou8(cur2, 10, &num2) < 0)
> return -1;
>
> /* if numbers differ, then set the result */
> if (num1 < num2)
> + return -1;
> + if (num1 > num2)
> + return 1;
> +
> + /* if numbers are the same, go to next level */
> + if (!next1 && !next2)
> + break;
> + else if (!next1) {
> result = -1;
> - else if (num1 > num2)
> + } else if (!next2) {
> result = 1;
> - else {
> - /* if numbers are the same, go to next level */
> - ver1 = strchr(ver1, '.');
> - ver2 = strchr(ver2, '.');
> - if (!ver1 && !ver2)
> - break;
> - else if (!ver1)
> - result = -1;
> - else if (!ver2)
> - result = 1;
> - else {
> - ver1++;
> - ver2++;
> - }
> + } else {
> + cur1 = next1;
> + cur2 = next2;
> }
> }
> +
> return result;
> }
>
> diff --git a/tools/bpf/example b/tools/bpf/example
> new file mode 100644
> index 000..a0ac81f
> --- /dev/null
> +++ b/tools/bpf/example
> @@ -0,0 +1,12 @@
> + ldh [12]
> + jne #0x8100, nonvlan
> + ldh [16]
> + jne #0x88f7, bad
> + ldb [18]
> + ja test
> + nonvlan: jne #0x88f7, bad
> + ldb [14]
> + test: and #0x8
> + jeq #0, bad
> + good: ret #1500
> + bad: ret #0
Looks like this hunk and the next got included by mistake.
Thanks,
Richard
> diff --git a/tools/bpf/novlan b/tools/bpf/novlan
> new file mode 100644
> index 000..fe35288
> --- /dev/null
> +++ b/tools/bpf/novlan
> @@ -0,0 +1,7 @@
> + ldh [12]
> + jne #0x88f7, bad
> + ldb [14]
> + and #0x8
> + jeq #0, bad
> + good: ret #1500
> + bad: ret #0
> --
> 2.7.4
>
Re: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
On Sat, Nov 21, 2020 at 03:26:11AM +0200, Vladimir Oltean wrote:
> On Thu, Nov 19, 2020 at 06:51:15PM +, tristram...@microchip.com wrote:
> > I think the right implementation is for the driver to remember this receive
> > timestamp
> > of Pdelay_Req and puts it in the tail tag when it sees a 1-step Pdelay_Resp
> > is sent.
As long as this is transparent to user space, it could work. Remember
that user space simply copies the correction field from the Request
into the Response. If the driver correctly accumulates the turnaround
time into the correction field of the response, then all is well.
> I have mixed feelings about this. IIUC, you're saying "let's implement a
> fixed-size FIFO of RX timestamps of Pdelay_Req messages, and let's match
> them on TX to Pdelay_Resp messages, by {sequenceId, domainNumber}."
>
> But how deep should we make that FIFO? I.e. how many Pdelay_Req messages
> should we expect before the user space will inject back a Pdelay_Resp
> for transmission?
Good question. Normally you would expect just one Request pending at
any one time, but nothing guarantees that, and so the driver would
have to match the Req/Resp exactly and deal with rogue/buggy requests
and responses.
Thanks,
Richard
Re: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
On Sat, Nov 21, 2020 at 03:26:11AM +0200, Vladimir Oltean wrote: > On Thu, Nov 19, 2020 at 06:51:15PM +, tristram...@microchip.com wrote: > > The receive and transmit latencies are different for different connected > > speed. So the > > driver needs to change them when the link changes. For that reason the PTP > > stack > > should not use its own latency values as generally the application does not > > care about > > the linked speed. > > The thing is, ptp4l already has ingressLatency and egressLatency > settings, and I would not be surprised if those config options would get > extended to cover values at multiple link speeds. > > In the general case, the ksz9477 MAC could be attached to any external > PHY, having its own propagation delay characteristics, or any number of > other things that cause clock domain crossings. I'm not sure how feasible > it is for the kernel to abstract this away completely, and adjust > timestamps automatically based on any and all combinations of MAC and > PHY. Maybe this is just wishful thinking. The idea that the driver will correctly adjust time stamps according to link speed sounds nice in theory, but in practice it fails. There is a at least one other driver that attempted this, but, surprise, surprise, the hard coded correction values turned out to be wrong. I think the best way would be to let user space monitor the link speed and apply the matching correction value. That way, we avoid bogus, hard coded values in kernel space. (This isn't implemented in linuxptp, but it certainly could be.) Thanks, Richard
Re: [Intel-wired-lan] [PATCH next-queue v2 3/3] igc: Add support for PTP getcrosststamp()
On Wed, Nov 18, 2020 at 04:22:37PM -0800, Vinicius Costa Gomes wrote:
> Talking with the hardware folks, they recommended using the periodic
> method, the one shot method was implemented as a debug/evaluation aid.
I'm guessing ...
The HW generates pairs of time stamps, right?
And these land in the device driver by means of an interrupt, right?
If that is so, then maybe the best way to expose the pair to user
space is to have a readable character device, like we have for the
PTP_EXTTS_REQUEST2. The ioctl to enable reporting could also set the
message rate.
Although it will be a bit clunky, it looks like we have reserved room
enough for a second, eight-byte time stamp.
struct ptp_clock_time {
__s64 sec; /* seconds */
__u32 nsec; /* nanoseconds */
__u32 reserved;
// four here
};
struct ptp_extts_event {
struct ptp_clock_time t; /* Time event occured. */
unsigned int index; /* Which channel produced the event. */
unsigned int flags; /* Reserved for future use. */
unsigned int rsv[2]; /* Reserved for future use. */
// eight here
};
You could set 'flags' to mark this as a time stamp pair, and then
stuff the system time stamp into rsv[2].
Thoughts?
Richard
Re: [PATCH net-next v3 0/3] net: ptp: introduce common defines for PTP message types
On Fri, Nov 20, 2020 at 09:41:03AM +0100, Christian Eggers wrote: > This series introduces commen defines for PTP event messages. Driver > internal defines are removed and some uses of magic numbers are replaced > by the new defines. Nice cleanup! Reviewed-by: Richard Cochran Thanks, Richard
Re: [PATCH net-next 1/4] net: ptp: introduce enum ptp_msg_type
On Tue, Nov 17, 2020 at 08:31:21PM +0100, Christian Eggers wrote:
> Using a PTP wide enum will obsolete different driver internal defines
> and uses of magic numbers.
I like the general idea, but ...
> +enum ptp_msg_type {
> + PTP_MSGTYPE_SYNC= 0x0,
> + PTP_MSGTYPE_DELAY_REQ = 0x1,
> + PTP_MSGTYPE_PDELAY_REQ = 0x2,
> + PTP_MSGTYPE_PDELAY_RESP = 0x3,
> +};
I would argue that these should be #defines. After all, they are
protocol data fields and not an abstract enumerated types.
For example ...
> @@ -103,10 +110,10 @@ struct ptp_header *ptp_parse_header(struct sk_buff
> *skb, unsigned int type);
> *
> * Return: The message type
> */
> -static inline u8 ptp_get_msgtype(const struct ptp_header *hdr,
> +static inline enum ptp_msg_type ptp_get_msgtype(const struct ptp_header *hdr,
>unsigned int type)
> {
> - u8 msgtype;
> + enum ptp_msg_type msgtype;
>
> if (unlikely(type & PTP_CLASS_V1)) {
> /* msg type is located at the control field for ptp v1 */
msgtype = hdr->control;
} else {
msgtype = hdr->tsmt & 0x0f;
This assigns directly from protocol data into an enumerated type.
}
return msgtype;
Thanks,
Richard
Re: [PATCH net-next v1] ptp: document struct ptp_clock_request members
On Tue, Nov 17, 2020 at 10:38:26PM +0100, Ahmad Fatoum wrote: > It's arguable most people interested in configuring a PPS signal > want it as external output, not as kernel input. PTP_CLK_REQ_PPS > is for input though. Add documentation to nudge readers into > the correct direction. > > Signed-off-by: Ahmad Fatoum Thanks! Acked-by: Richard Cochran
Re: [Intel-wired-lan] [PATCH next-queue v2 3/3] igc: Add support for PTP getcrosststamp()
On Tue, Nov 17, 2020 at 05:21:48PM -0800, Vinicius Costa Gomes wrote: > Agreed that would be easiest/simplest. But what I have in hand seems to > not like it, i.e. I have an earlier series implementing this "one shot" way > and it's not reliable over long periods of time or against having the > system time adjusted. Before we go inventing a new API, I think we should first understand why the one shot thing fails. If there is problem with the system time being adjusted during PTM, then that needs solving in any case! Thanks, Richard
Re: [net-next 1/4] i40e: add support for PTP external synchronization clock
On Mon, Nov 16, 2020 at 05:07:37PM -0800, Jakub Kicinski wrote: > On Fri, 13 Nov 2020 16:10:54 -0800 Tony Nguyen wrote: > > +/* get PTP pins for ioctl */ > > +#define SIOCGPINS (SIOCDEVPRIVATE + 0) > > +/* set PTP pins for ioctl */ > > +#define SIOCSPINS (SIOCDEVPRIVATE + 1) > > This is unexpected.. is it really normal to declare private device > IOCTLs to configure PPS pins? Or are you just exposing this so you're > able to play with GPIOs from user space? I missed the full patch, but I'd like to point out that we already have a rather fully featured pin control API. If something is missing from that API, then please, let's discuss it! Thanks, Richard
Re: [Intel-wired-lan] [PATCH next-queue v2 3/3] igc: Add support for PTP getcrosststamp()
On Mon, Nov 16, 2020 at 05:06:30PM -0800, Vinicius Costa Gomes wrote: > The PTM dialogs are a pair of messages: a Request from the endpoint (in > my case, the NIC) to the PCIe root (or switch), and a Response from the > other side (this message includes the Master Root Time, and the > calculated propagation delay). > > The interface exposed by the NIC I have allows basically to start/stop > these PTM dialogs (I was calling them PTM cycles) and to configure the > interval between each cycle (~1ms - ~512ms). Ah, now I am starting to understand... Just to be clear, this is yet another time measurement over PCIe, different than the cross time stamp that we already have, right? Also, what is the point of providing time measurements every 1 millisecond? > Another thing of note, is that trying to start the PTM dialogs "on > demand" syncronously with the ioctl() doesn't seem too reliable, it > seems to want to be kept running for a longer time. So, I think the simplest thing would be to have a one-shot measurement, if possible. Then you could use the existing API and let the user space trigger the time stamps. Thanks, Richard
Re: Re: [PATCH V4 net-bugfixs] net/ethernet: Update ret when ptp_clock is ERROR
On Fri, Nov 13, 2020 at 05:21:43PM +0100, Arnd Bergmann wrote:
> I've prototyped a patch that I think makes this more sensible
> again: https://pastebin.com/AQ5nWS9e
I like the behavior described in the text.
Instead of this ...
- if a built-in driver calls PTP interface functions but fails
to select HAVE_PTP_1588_CLOCK or depend on PTP_1588_CLOCK,
and PTP support is a loadable module, we get a link error
instead of having an unusable clock.
how about simply deleting the #else clause of
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -173,7 +173,7 @@ struct ptp_clock_event {
};
};
-#if IS_REACHABLE(CONFIG_PTP_1588_CLOCK)
+#if IS_ENABLED(CONFIG_PTP_1588_CLOCK)
so that invalid configurations throw a compile time error instead?
Thanks,
Richard
Re: [Intel-wired-lan] [PATCH next-queue v2 3/3] igc: Add support for PTP getcrosststamp()
On Fri, Nov 13, 2020 at 11:10:58AM -0800, Vinicius Costa Gomes wrote: > I am proposing a series that adds support for PCIe PTM (for the igc > driver), exporting the values via the PTP_SYS_OFFSET_PRECISE ioctl(). > > The way PTM works in the NIC I have, kind of forces me to start the PTM > dialogs during initialization, and they are kept running in background, > what the _PRECISE ioctl() does is basically collecting the most recent > measurement. What is a PTM? Why does a PTM have dialogs? Can it talk? Forgive my total ignorance! Thanks, Richard
Re: [PATCH net-next v2 10/11] net: dsa: microchip: ksz9477: add Pulse Per Second (PPS) support
On Fri, Nov 13, 2020 at 04:53:11AM +0200, Vladimir Oltean wrote: > Richard, do you think we can clarify the intended usage of PTP_CLK_REQ_PPS > in the documentation? It doesn't appear to be written anywhere that > PTP_ENABLE_PPS is supposed to enable event generation for the drivers/pps > subsystem. You would sort of have to know before you could find out... Yes, please! The poor naming is a source of eternal confusion. I think that the "hard pps" thing from NTP is not used very often, maybe never, but I didn't know that when I first drafted the whole PHC subsystem. Naturally developers of PHC device drivers think that this the PPS that they need to implement. After all, the name matches! (Actually, at the time I thought that this would be the way to synchronize the system clock to the PHC, but it turned out that Miroslav's generic method in phc2sys worked very well, and so the hard pps thing has little, if any, practical value.) The documentation is vague, yes, but I think even more important would be to remove the word PPS from the C-language identifiers. I'm open to suggestions/patches on this... Thanks, Richard
Re: [PATCH net-next v2 09/11] net: dsa: microchip: ksz9477: add hardware time stamping support
On Fri, Nov 13, 2020 at 04:40:20AM +0200, Vladimir Oltean wrote: > > @@ -103,6 +108,10 @@ static int ksz9477_ptp_adjtime(struct ptp_clock_info > > *ptp, s64 delta) > > if (ret) > > goto error_return; > > > > + spin_lock_irqsave(&dev->ptp_clock_lock, flags); > > I believe that spin_lock_irqsave is unnecessary, since there is no code > that runs in hardirq context. The .adjtime method is in a system call context. If all of the code that manipulates dev->ptp_clock_time can sleep, then you can use a mutex. Otherwise spin_lock_irqsave is the safe choice. Thanks, Richard
Re: [Intel-wired-lan] [PATCH next-queue v2 3/3] igc: Add support for PTP getcrosststamp()
On Thu, Nov 12, 2020 at 03:46:12PM -0800, Vinicius Costa Gomes wrote: > I wanted it so using PCIe PTM was transparent to applications, so adding > another API wouldn't be my preference. > > That being said, having a trigger from the application to start/stop the > PTM cycles doesn't sound too bad an idea. So, not too opposed to this > idea. > > Richard, any opinions here? Sorry, I only have the last two message from this thread, and so I'm missing the backstory. Richard
Re: Re: [PATCH V4 net-bugfixs] net/ethernet: Update ret when ptp_clock is ERROR
On Thu, Nov 12, 2020 at 10:21:21PM +0100, Arnd Bergmann wrote: > I agree that the 'imply' keyword in Kconfig is what made this a > lot worse, and it would be best to replace that with normal > dependencies. IIRC, this all started with tinification and wanting dynamic posix clocks to be optional at compile time. I would like to simplify this whole mess: - restore dynamic posix clocks to be always included - make PHC always included when the MAC has that feature (by saying "select" in the MAC Kconfig) -- I think this is what davem had wanted back in the day ... I'm not against tinification in principle, but I believe it is a lost cause. Thanks, Richard
Re: Re: [PATCH V4 net-bugfixs] net/ethernet: Update ret when ptp_clock is ERROR
On Thu, Nov 12, 2020 at 09:25:12AM +0100, Arnd Bergmann wrote:
> This is not really getting any better. If Richard is worried about
> Kconfig getting changed here, I would suggest handling the
> case of PTP being disabled by returning an error early on in the
> function, like
>
> struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs,
>struct device_node *node)
> {
> struct am65_cpts *cpts;
> int ret, i;
>
> if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK))
> return -ENODEV;
No, please, no. That only adds confusion. The NULL return value
already signals that the compile time support was missing. That was
the entire point of this...
* ptp_clock_register() - register a PTP hardware clock driver
*
* @info: Structure describing the new clock.
* @parent: Pointer to the parent device of the new clock.
*
* Returns a valid pointer on success or PTR_ERR on failure. If PHC
* support is missing at the configuration level, this function
* returns NULL, and drivers are expected to gracefully handle that
* case separately.
Thanks,
Richard
Re: [PATCH V4 net-bugfixs] net/ethernet: Update ret when ptp_clock is ERROR
On Thu, Nov 12, 2020 at 12:05:25PM +0200, Grygorii Strashko wrote: > But, as Richard mentioned [1], ptp_clock_register() may return NULL even if > PTP_1588_CLOCK=y > (which I can't confirm neither deny - from the fast look at > ptp_clock_register() > code it seems should not return NULL) This whole "implies" thing turned out to be a colossal PITA. I regret the fact that it got merged. It wasn't my idea. I will push back on playing games with the Kconfig settings. Even if that happens to work for your particular driver, still the call site of ptp_clock_register() must follow the correct pattern. Why? Because others will copy/paste it. Thanks, Richard
Re: [PATCH V4 net-bugfixs] net/ethernet: Update ret when ptp_clock is ERROR
On Wed, Nov 11, 2020 at 03:24:33PM +0200, Grygorii Strashko wrote: > > Following Richard's comments v1 of the patch has to be applied [1]. > I've also added my Reviewed-by there. > > [1] https://lore.kernel.org/patchwork/patch/1334067/ +1 Jakub, can you please take the original v1 of this patch? Thanks, Richard
Re: [PATCH V4 net-bugfixs] net/ethernet: Update ret when ptp_clock is ERROR
On Wed, Nov 11, 2020 at 05:24:41PM +0800, Wang Qing wrote: > We always have to update the value of ret, otherwise the error value > may be the previous one. And ptp_clock_register() never return NULL > when PTP_1588_CLOCK enable. NAK. Your code must handle the possibility that ptp_clock_register() can return NULL. Why? 1. Because that follows the documented API. 2. Because people will copy/paste this driver. 3. Because the Kconfig for your driver can change without warning. Thanks, Richard
Re: [PATCH] net/ethernet: update ret when ptp_clock is ERROR
On Fri, Nov 06, 2020 at 03:56:45PM +0800, Wang Qing wrote:
> We always have to update the value of ret, otherwise the
> error value may be the previous one.
>
> Signed-off-by: Wang Qing
Acked-by: Richard Cochran
> ---
> drivers/net/ethernet/ti/am65-cpts.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/am65-cpts.c
> b/drivers/net/ethernet/ti/am65-cpts.c
> index 75056c1..b77ff61
> --- a/drivers/net/ethernet/ti/am65-cpts.c
> +++ b/drivers/net/ethernet/ti/am65-cpts.c
> @@ -1001,8 +1001,7 @@ struct am65_cpts *am65_cpts_create(struct device *dev,
> void __iomem *regs,
> if (IS_ERR_OR_NULL(cpts->ptp_clock)) {
> dev_err(dev, "Failed to register ptp clk %ld\n",
> PTR_ERR(cpts->ptp_clock));
> - if (!cpts->ptp_clock)
> - ret = -ENODEV;
> + ret = cpts->ptp_clock ? cpts->ptp_clock : (-ENODEV);
> goto refclk_disable;
> }
> cpts->phc_index = ptp_clock_index(cpts->ptp_clock);
> --
> 2.7.4
>
Re: [PATCH] net/ethernet: update ret when ptp_clock is ERROR
On Fri, Nov 06, 2020 at 01:34:04PM +0200, Grygorii Strashko wrote: > And ptp_clock_register() can return NULL only if PTP support is disabled. Not true in general ... > In which case, we should not even get here. only because the Kconfig uses "depends on" instead of "implies" PTP_1588_CLOCK. > So, I'd propose to s/IS_ERR_OR_NULL/IS_ERR above, > and just assign ret = PTR_ERR(cpts->ptp_clock) here. No, please no -- don't make another bad example for people to copy/paste. Thanks, Richard
Re: [V2] [PATCH] net/ethernet: update ret when ptp_clock is ERROR
On Sat, Nov 07, 2020 at 11:38:38AM +0800, Wang Qing wrote:
> We always have to update the value of ret, otherwise the error value
> may be the previous one. And ptp_clock_register() never return NULL
> when PTP_1588_CLOCK enable, so we use IS_ERR here.
>
> Signed-off-by: Wang Qing
> ---
> drivers/net/ethernet/ti/am65-cpts.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/am65-cpts.c
> b/drivers/net/ethernet/ti/am65-cpts.c
> index 75056c1..ec8e56d
> --- a/drivers/net/ethernet/ti/am65-cpts.c
> +++ b/drivers/net/ethernet/ti/am65-cpts.c
> @@ -998,11 +998,10 @@ struct am65_cpts *am65_cpts_create(struct device *dev,
> void __iomem *regs,
> am65_cpts_settime(cpts, ktime_to_ns(ktime_get_real()));
>
> cpts->ptp_clock = ptp_clock_register(&cpts->ptp_info, cpts->dev);
> - if (IS_ERR_OR_NULL(cpts->ptp_clock)) {
This test was correct.
> + if (IS_ERR(cpts->ptp_clock)) {
This one is wrong.
Thanks,
Richard
> dev_err(dev, "Failed to register ptp clk %ld\n",
> PTR_ERR(cpts->ptp_clock));
> - if (!cpts->ptp_clock)
> - ret = -ENODEV;
> + ret = PTR_ERR(cpts->ptp_clock);
> goto refclk_disable;
> }
> cpts->phc_index = ptp_clock_index(cpts->ptp_clock);
> --
> 2.7.4
>
Re: [PATCH] pcp_clock: return EOPNOTSUPP if !CONFIG_PTP_1588_CLOCK
On Sat, Nov 07, 2020 at 11:28:23AM +0800, Wang Qing wrote: > pcp_clock_register() is checked with IS_ERR(), and will crash if !PTP, > change return value to ERR_PTR(-EOPNOTSUPP) for the !CONFIG_PTP_1588_CLOCK > and so question resolved. NAK. Drivers must use the documented interface. Thanks, Richard
Re: [PATCH v2 net-next 1/3] ptp: idt82p33: add adjphase support
On Wed, Nov 04, 2020 at 03:45:08PM -0800, Jakub Kicinski wrote: > Also are you sure the last patch is okay? Richard suggested it's not > worth the risk AFAIU. I took a look, and I can't find anything wrong with it. Thanks, Richard
Re: [PATCH net-next 3/3] ptp: idt82p33: optimize _idt82p33_adjfine
On Thu, Nov 05, 2020 at 02:35:56AM +0200, Vladimir Oltean wrote: > On the other hand and with all due respect, saying that it may have been > 'buggy on some archs back in the day' and then not bringing any evidence > is a bit of a strange claim to make. You're right. I made the effort to look back into the days of v3.0, and the only thing I could find is that the 32 bit implementation of div_s64 does extra operations and invokes an additional function call. But the difference in performance, if any, is probably not very large. > I am actively using div_s64 in drivers/net/dsa/sja1105/sja1105_ptp.c > successfully on arm and arm64. Yeah, I see div_s64 has found its way into the ntp code, too, so it must be fine. Thanks, Richard
Re: [PATCH net-next 3/3] ptp: idt82p33: optimize _idt82p33_adjfine
On Wed, Nov 04, 2020 at 11:01:49AM -0500, min.li...@renesas.com wrote:
> From: Min Li
>
> Use div_s64 so that the neg_adj is not needed.
Back in the day, I coded the neg_adj because there was some issue with
signed 64 bit division that I can't recall now. Either div_s64 didn't
exist or it was buggy on some archs... there was _some_ reason.
So unless you are sure that this works on all platforms, I would leave
it alone.
Thanks,
Richard
> Signed-off-by: Min Li
> ---
> drivers/ptp/ptp_idt82p33.c | 10 +-
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/ptp/ptp_idt82p33.c b/drivers/ptp/ptp_idt82p33.c
> index b1528a0..e970379d 100644
> --- a/drivers/ptp/ptp_idt82p33.c
> +++ b/drivers/ptp/ptp_idt82p33.c
> @@ -320,7 +320,6 @@ static int _idt82p33_adjfine(struct idt82p33_channel
> *channel, long scaled_ppm)
> {
> struct idt82p33 *idt82p33 = channel->idt82p33;
> unsigned char buf[5] = {0};
> - int neg_adj = 0;
> int err, i;
> s64 fcw;
>
> @@ -340,16 +339,9 @@ static int _idt82p33_adjfine(struct idt82p33_channel
> *channel, long scaled_ppm)
>* FCW = -
>* 168 * 2^4
>*/
> - if (scaled_ppm < 0) {
> - neg_adj = 1;
> - scaled_ppm = -scaled_ppm;
> - }
>
> fcw = scaled_ppm * 244140625ULL;
> - fcw = div_u64(fcw, 2688);
> -
> - if (neg_adj)
> - fcw = -fcw;
> + fcw = div_s64(fcw, 2688);
>
> for (i = 0; i < 5; i++) {
> buf[i] = fcw & 0xff;
> --
> 2.7.4
>
Re: [PATCH net-next 2/3] ptp: idt82p33: use i2c_master_send for bus write
On Wed, Nov 04, 2020 at 11:01:48AM -0500, min.li...@renesas.com wrote: > From: Min Li > > Refactor idt82p33_xfer and use i2c_master_send for write operation. > Because some I2C controllers are only working with single-burst write > transaction. > > Signed-off-by: Min Li Acked-by: Richard Cochran
Re: [PATCH net-next 1/3] ptp: idt82p33: add adjphase support
On Wed, Nov 04, 2020 at 11:01:47AM -0500, min.li...@renesas.com wrote: > From: Min Li > > Add idt82p33_adjphase() to support PHC write phase mode. > > Signed-off-by: Min Li Acked-by: Richard Cochran
Re: [PATCH] net: ethernet: ti: cpsw: disable PTPv1 hw timestamping advertisement
On Sat, Oct 31, 2020 at 11:40:42AM -0700, Jakub Kicinski wrote:
> On Thu, 29 Oct 2020 21:09:10 +0200 Grygorii Strashko wrote:
> > The TI CPTS does not natively support PTPv1, only PTPv2. But, as it
> > happens, the CPTS can provide HW timestamp for PTPv1 Sync messages, because
> > CPTS HW parser looks for PTP messageType id in PTP message octet 0 which
> > value is 0 for PTPv1. As result, CPTS HW can detect Sync messages for PTPv1
> > and PTPv2 (Sync messageType = 0 for both), but it fails for any other PTPv1
> > messages (Delay_req/resp) and will return PTP messageType id 0 for them.
> >
> > The commit e9523a5a32a1 ("net: ethernet: ti: cpsw: enable
> > HWTSTAMP_FILTER_PTP_V1_L4_EVENT filter") added PTPv1 hw timestamping
> > advertisement by mistake, only to make Linux Kernel "timestamping" utility
> > work, and this causes issues with only PTPv1 compatible HW/SW - Sync HW
> > timestamped, but Delay_req/resp are not.
> >
> > Hence, fix it disabling PTPv1 hw timestamping advertisement, so only PTPv1
> > compatible HW/SW can properly roll back to SW timestamping.
> >
> > Fixes: e9523a5a32a1 ("net: ethernet: ti: cpsw: enable
> > HWTSTAMP_FILTER_PTP_V1_L4_EVENT filter")
> > Signed-off-by: Grygorii Strashko
>
> CC: Richard
Acked-by: Richard Cochran
