All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
From: "Y.b. Lu" <yangbo.lu@nxp.com>
To: Richard Cochran <richardcochran@gmail.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-kselftest@vger.kernel.org"
	<linux-kselftest@vger.kernel.org>,
	"mptcp@lists.linux.dev" <mptcp@lists.linux.dev>,
	"David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Mat Martineau <mathew.j.martineau@linux.intel.com>,
	Matthieu Baerts <matthieu.baerts@tessares.net>,
	Shuah Khan <shuah@kernel.org>, Michal Kubecek <mkubecek@suse.cz>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>, Rui Sousa <rui.sousa@nxp.com>,
	Sebastien Laveze <sebastien.laveze@nxp.com>
Subject: RE: [net-next, v3, 01/10] ptp: add ptp virtual clock driver framework
Date: Tue, 22 Jun 2021 10:17:46 +0000	[thread overview]
Message-ID: <DB7PR04MB5017E87F4E95BBCB1D326EB4F8099@DB7PR04MB5017.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20210617173237.GA4770@localhost>

Hi Richard,

> -----Original Message-----
> From: Richard Cochran <richardcochran@gmail.com>
> Sent: 2021年6月18日 1:33
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-kselftest@vger.kernel.org; mptcp@lists.linux.dev; David S . Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Mat Martineau
> <mathew.j.martineau@linux.intel.com>; Matthieu Baerts
> <matthieu.baerts@tessares.net>; Shuah Khan <shuah@kernel.org>; Michal
> Kubecek <mkubecek@suse.cz>; Florian Fainelli <f.fainelli@gmail.com>;
> Andrew Lunn <andrew@lunn.ch>; Rui Sousa <rui.sousa@nxp.com>; Sebastien
> Laveze <sebastien.laveze@nxp.com>
> Subject: Re: [net-next, v3, 01/10] ptp: add ptp virtual clock driver framework
> 
> On Tue, Jun 15, 2021 at 05:45:08PM +0800, Yangbo Lu wrote:
> > diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile index
> > 8673d1743faa..3c6a905760e2 100644
> > --- a/drivers/ptp/Makefile
> > +++ b/drivers/ptp/Makefile
> > @@ -3,7 +3,7 @@
> >  # Makefile for PTP 1588 clock support.
> >  #
> >
> > -ptp-y					:= ptp_clock.o ptp_chardev.o ptp_sysfs.o
> > +ptp-y					:= ptp_clock.o ptp_vclock.o ptp_chardev.o
> ptp_sysfs.o
> 
> Nit: Please place ptp_vclock.o at the end of the list.

Ok.

> 
> >  ptp_kvm-$(CONFIG_X86)			:= ptp_kvm_x86.o ptp_kvm_common.o
> >  ptp_kvm-$(CONFIG_HAVE_ARM_SMCCC)	:= ptp_kvm_arm.o
> ptp_kvm_common.o
> >  obj-$(CONFIG_PTP_1588_CLOCK)		+= ptp.o
> 
> > diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
> > index 6b97155148f1..3f388d63904c 100644
> > --- a/drivers/ptp/ptp_private.h
> > +++ b/drivers/ptp/ptp_private.h
> > @@ -48,6 +48,20 @@ struct ptp_clock {
> >  	struct kthread_delayed_work aux_work;  };
> >
> > +#define info_to_vclock(d) container_of((d), struct ptp_vclock, info)
> > +#define cc_to_vclock(d) container_of((d), struct ptp_vclock, cc)
> > +#define dw_to_vclock(d) container_of((d), struct ptp_vclock,
> > +refresh_work)
> > +
> > +struct ptp_vclock {
> > +	struct ptp_clock *pclock;
> > +	struct ptp_clock_info info;
> > +	struct ptp_clock *clock;
> > +	struct cyclecounter cc;
> > +	struct timecounter tc;
> > +	spinlock_t lock;	/* protects tc/cc */
> > +	struct delayed_work refresh_work;
> 
> Can we please have a kthread worker here instead of work?
> 
> Experience shows that plain work can be delayed for a long, long time on busy
> systems.
> 

I think do_aux_work callback could be utilized for ptp virtual clock, right?

> > +};
> > +
> >  /*
> >   * The function queue_cnt() is safe for readers to call without
> >   * holding q->lock. Readers use this function to verify that the
> > queue @@ -89,4 +103,6 @@ extern const struct attribute_group
> > *ptp_groups[];  int ptp_populate_pin_groups(struct ptp_clock *ptp);
> > void ptp_cleanup_pin_groups(struct ptp_clock *ptp);
> >
> > +struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock);
> > +void ptp_vclock_unregister(struct ptp_vclock *vclock);
> >  #endif
> 
> > diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c new
> > file mode 100644 index 000000000000..b8f500677314
> > --- /dev/null
> > +++ b/drivers/ptp/ptp_vclock.c
> > @@ -0,0 +1,154 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * PTP virtual clock driver
> > + *
> > + * Copyright 2021 NXP
> > + */
> > +#include <linux/slab.h>
> > +#include "ptp_private.h"
> > +
> > +#define PTP_VCLOCK_CC_MULT		(1 << 31)
> > +#define PTP_VCLOCK_CC_SHIFT		31
> 
> These two are okay, but ...
> 
> > +#define PTP_VCLOCK_CC_MULT_NUM		(1 << 9)
> > +#define PTP_VCLOCK_CC_MULT_DEM		15625ULL
> 
> the similarity of naming is confusing for these two.  They are only used in
> the .adjfine method.  How about this?
> 
>   PTP_VCLOCK_FADJ_NUMERATOR, or even PTP_VCLOCK_FADJ_SHIFT (see
> below)
>   PTP_VCLOCK_FADJ_DENOMINATOR
> 
> > +#define PTP_VCLOCK_CC_REFRESH_INTERVAL	(HZ * 2)
> 
> Consider dropping CC from the name.
> PTP_VCLOCK_REFRESH_INTERVAL sounds good to me.
> 

Thanks. Will rename the MACROs per your suggestion.

> > +static int ptp_vclock_adjfine(struct ptp_clock_info *ptp, long
> > +scaled_ppm) {
> > +	struct ptp_vclock *vclock = info_to_vclock(ptp);
> > +	unsigned long flags;
> > +	s64 adj;
> > +
> > +	adj = (s64)scaled_ppm * PTP_VCLOCK_CC_MULT_NUM;
> 
> Rather than
> 
>     scaled_ppm * (1 << 9)
> 
> I suggest
> 
>     scaled_ppm << 9
> 
> instead.  I suppose a good compiler would replace the multiplication with a
> bit shift, but it never hurts to spell it out.

Ok.

> 
> > +	adj = div_s64(adj, PTP_VCLOCK_CC_MULT_DEM);
> > +
> > +	spin_lock_irqsave(&vclock->lock, flags);
> > +	timecounter_read(&vclock->tc);
> > +	vclock->cc.mult = PTP_VCLOCK_CC_MULT + adj;
> > +	spin_unlock_irqrestore(&vclock->lock, flags);
> > +
> > +	return 0;
> > +}
> 
> Thanks,
> Richard

  reply	other threads:[~2021-06-22 10:17 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-15  9:45 [net-next, v3, 00/10] ptp: support virtual clocks and timestamping Yangbo Lu
2021-06-15  9:45 ` [net-next, v3, 01/10] ptp: add ptp virtual clock driver framework Yangbo Lu
2021-06-17 17:32   ` Richard Cochran
2021-06-22 10:17     ` Y.b. Lu [this message]
2021-06-15  9:45 ` [net-next, v3, 02/10] ptp: support ptp physical/virtual clocks conversion Yangbo Lu
2021-06-17 17:42   ` Richard Cochran
2021-06-22 10:18     ` Y.b. Lu
2021-06-17 18:27   ` Richard Cochran
2021-06-22 10:35     ` Y.b. Lu
2021-06-25 11:09       ` Y.b. Lu
2021-06-19  4:06   ` Richard Cochran
2021-06-22 10:39     ` Y.b. Lu
2021-06-15  9:45 ` [net-next, v3, 03/10] ptp: track available ptp vclocks information Yangbo Lu
2021-06-15  9:45 ` [net-next, v3, 04/10] ptp: add kernel API ptp_get_vclocks_index() Yangbo Lu
2021-06-15  9:45 ` [net-next, v3, 05/10] ethtool: add a new command for getting PHC virtual clocks Yangbo Lu
2021-06-15 19:49   ` Jakub Kicinski
2021-06-22 10:10     ` Y.b. Lu
2021-06-15 23:24   ` Michal Kubecek
2021-06-22 10:10     ` Y.b. Lu
2021-06-15  9:45 ` [net-next, v3, 06/10] ptp: add kernel API ptp_convert_timestamp() Yangbo Lu
2021-06-15  9:45 ` [net-next, v3, 07/10] net: sock: extend SO_TIMESTAMPING for PHC binding Yangbo Lu
2021-06-16  1:00   ` Mat Martineau
2021-06-22 10:14     ` Y.b. Lu
2021-06-16 10:27   ` kernel test robot
2021-06-16 10:27     ` kernel test robot
2021-06-16 10:39   ` kernel test robot
2021-06-16 10:39     ` kernel test robot
2021-06-15  9:45 ` [net-next, v3, 08/10] net: socket: support hardware timestamp conversion to PHC bound Yangbo Lu
2021-06-15  9:45 ` [net-next, v3, 09/10] selftests/net: timestamping: support binding PHC Yangbo Lu
2021-06-15  9:45 ` [net-next, v3, 10/10] MAINTAINERS: add entry for PTP virtual clock driver Yangbo Lu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DB7PR04MB5017E87F4E95BBCB1D326EB4F8099@DB7PR04MB5017.eurprd04.prod.outlook.com \
    --to=yangbo.lu@nxp.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=mathew.j.martineau@linux.intel.com \
    --cc=matthieu.baerts@tessares.net \
    --cc=mkubecek@suse.cz \
    --cc=mptcp@lists.linux.dev \
    --cc=netdev@vger.kernel.org \
    --cc=richardcochran@gmail.com \
    --cc=rui.sousa@nxp.com \
    --cc=sebastien.laveze@nxp.com \
    --cc=shuah@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.