RE: [PATCH v1 4/6] pps: generators: Add PPS Generator TIO Driver

[D, Lakshmi Sowjanya]

> -----Original Message-----
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Sent: Tuesday, October 17, 2023 9:58 PM
> To: D, Lakshmi Sowjanya <lakshmi.sowjanya.d@xxxxxxxxx>;
> jstultz@xxxxxxxxxx; giometti@xxxxxxxxxxxx; corbet@xxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Cc: x86@xxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx;
> andriy.shevchenko@xxxxxxxxxxxxxxx; Dong, Eddie <eddie.dong@xxxxxxxxx>; Hall,
> Christopher S <christopher.s.hall@xxxxxxxxx>; N, Pandith
> <pandith.n@xxxxxxxxx>; Sangannavar, Mallikarjunappa
> <mallikarjunappa.sangannavar@xxxxxxxxx>; T R, Thejesh Reddy
> <thejesh.reddy.t.r@xxxxxxxxx>; D, Lakshmi Sowjanya
> <lakshmi.sowjanya.d@xxxxxxxxx>; Peter Hilber
> <peter.hilber@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v1 4/6] pps: generators: Add PPS Generator TIO Driver
> 
> On Tue, Oct 17 2023 at 10:54, lakshmi.sowjanya.d@xxxxxxxxx wrote:
> > +
> > +static inline ktime_t first_event(struct pps_tio *tio) {
> > +	struct timespec64 ts;
> > +
> > +	ktime_get_real_ts64(&ts);
> > +
> > +	return ktime_set(ts.tv_sec + 1, NSEC_PER_SEC - PREP_INTERVAL_NS);
> 
>         return ktime_set(ktime_get_real_seconds() + 1, MAGIC_CONSTANT);
> 
> Perhaps?
> 
> PREP_INTERVAL_NS is a misnomer as it has nothing to do with an interval. It's
> the time substracted from the actual pulse target time to allow the hrtimer
> callback to setup the hardware for the pulse.
> 
> Naming matters really.

#define SAFE_TIME_NS  (10*NSEC_PER_MSEC) #define FIRST_EVENT_NS  (NSEC_PER_SEC -  SAFE_TIME_NS )

return ktime_set(ktime_get_real_seconds() + 1, FIRST_EVENT_NS);

> 
> > +static int translate_system_time_to_art_cycles(struct timespec64 ts, u64
> *art_timestamp,
> > +					       bool *real_to_tsc_result) {
> > +	struct system_counterval_t sys_counter;
> > +	ktime_t sys_realtime;
> > +	int err;
> > +
> > +	sys_realtime = timespec64_to_ktime(ts);
> 
> Why are you handing timespecs around? Because timespec math is so awesome,
> right?
> 
> > +	err = ktime_convert_real_to_system_counter(sys_realtime,
> &sys_counter);
> > +	if (err) {
> > +		*real_to_tsc_result = true;
> 
> This makes my bad taste sensors reach saturation.
> 
> > +		return err;
> > +	}
> > +
> > +	return convert_tsc_to_art(&sys_counter, art_timestamp); }
> 
> > +static int pps_tio_generate_output(struct pps_tio *tio, struct
> > +timespec64 time) {
> > +	bool real_to_tsc_result;
> > +	u64 art_timestamp;
> > +	int err;
> > +
> > +	real_to_tsc_result = false;
> > +	err = translate_system_time_to_art_cycles(time, &art_timestamp,
> &real_to_tsc_result);
> > +	if (err) {
> > +		pps_tio_disable(tio);
> > +		dev_err(tio->dev, "Disabling PPS due to failure in conversion of
> %s",
> > +			real_to_tsc_result ? "realtime to system_counter" : "tsc
> to art");
> > +		return err;
> 
> Clearly nothing in the call chain cares about the actual error code, right? So
> instead of having all these undocumented -E* all over the place, just make the
> inner functions bool and then only for
> translate_system_time_to_art_cycles() use
> 
> enum {
> 	SUCCESS,
>         FAIL_SC,
>         FAIL_ART,
> };
> 
> or something like that to make this error printout happy.
> 
> pps_tio_generate_output() itself can return bool too.
> 
> > +	}
> > +	/* The timed IO hardware adds a two cycle delay on output */
> > +	art_timestamp -= 2;
> > +	pps_compv_write(tio, art_timestamp);
> > +
> > +	return 0;
> > +}
> > +
> > +static int schedule_event(struct hrtimer *timer, struct timespec64
> > +*next_event) {
> > +	struct pps_tio *tio = container_of(timer, struct pps_tio, timer);
> > +	struct timespec64 expire_time, cur_time, roundoff;
> > +	long half_sec_ns = NSEC_PER_SEC / 2;
> > +
> > +	/* get the current time */
> > +	ktime_get_real_ts64(&cur_time);
> > +	expire_time = ktime_to_timespec64(hrtimer_get_softexpires(timer));
> > +
> > +	/*
> > +	 * Figure out if it is in "top half" or "bottom half" of the second
> > +	 * and round-off to the nearest 500ms
> > +	 */
> > +	if (cur_time.tv_nsec > half_sec_ns) {
> > +		roundoff.tv_sec = cur_time.tv_sec + 1;
> > +		roundoff.tv_nsec = 0;
> > +		next_event->tv_sec = roundoff.tv_sec;
> > +		next_event->tv_nsec = half_sec_ns;
> > +	} else {
> > +		roundoff.tv_sec = cur_time.tv_sec;
> > +		roundoff.tv_nsec = half_sec_ns;
> > +		next_event->tv_sec = roundoff.tv_sec;
> > +		next_event->tv_nsec = roundoff.tv_nsec + half_sec_ns;
> > +	}
> > +	next_event->tv_nsec -= PREP_INTERVAL_NS;
> > +	/* Check for elapsed time */
> > +	if (expire_time.tv_sec != cur_time.tv_sec ||
> > +	    (cur_time.tv_nsec - PREP_INTERVAL_NS) > expire_time.tv_nsec) {
> 
> The timer is considered on time when cur_time <= T_pulse?
> 
> How do you ensure that there is enough time to actually convert and arm the
> timer? Not at all. If cur_time is close to T_pulse then you end up arming it late.

We have a 10 msec of SAFE_TIME away from T_Pulse here.

> 
> > +		dev_warn(tio->dev, "Time expired, edge not scheduled at time:
> %lld.%09ld\n",
> > +			 cur_time.tv_sec, cur_time.tv_nsec);
> > +		return 0;
> > +	}
> > +
> > +	return pps_tio_generate_output(tio, roundoff); }
> > +
> > +static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer) {
> > +	struct pps_tio *tio = container_of(timer, struct pps_tio, timer);
> > +	struct timespec64 next_event;
> > +	int err = 0;
> > +
> > +	scoped_guard(spinlock_irqsave, &tio->lock) {
> > +		if (tio->enabled)
> > +			err = schedule_event(timer, &next_event);
> > +	}
> > +	if (err)
> > +		return HRTIMER_NORESTART;
> > +
> > +	hrtimer_set_expires(timer, ktime_set(next_event.tv_sec,
> next_event.tv_nsec));
> > +	return HRTIMER_RESTART;
> 
> All of this is overengineered complexity. Initially you start the hrtimer with
> 
> 	hrtimer_start(&tio->timer, first_event(tio), HRTIMER_MODE_ABS);
> 
> and that sets the first event to expire TPREP_NS before the full second. After
> that you want to schedule the timer periodically every 0.5s, right?
> 
> hrtimers provide periodic schedule already. So all of the gunk above can be
> replaced with:
> 
> static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer) {
> 	struct pps_tio *tio = container_of(timer, struct pps_tio, timer);
>         ktime_t expires, now;
> 
>         guard(spinlock)(&tio->lock);
> 
> 	expires = hrtimer_get_expires(timer);
> 	now = ktime_get_real();
> 
> 	if (now - expires < TOO_LATE) {
>         	if (!pps_arm_next_pulse(tio, expires + TPREP_NS))
>                 	return HRTIMER_NORESTART;
> 	}
> 
> 	hrtimer_forward(timer, now, NSEC_PER_SEC / 2);
>        	return HRTIMER_RESTART;
> }
> 
> and
> 
> static bool pps_arm_next_pulse(struct pps_tio *tio, ktime_t expires) {
> 	u64 art;
> 
> 	if (!ktime_real_to_base_clock(expires, CSID_X86_ART, &art))
>         	return false;
> 
> 	pps_compv_write(tio, art - ART_HW_DELAY_CYCLES);
>         return true;
> }
> 
> ktime_real_to_base_clock() does not exist, but that's the function you really
> want to have.
> 
> Not this convoluted construct of indirections and therefore we need to rethink
> the whole related clock mechanism from ground up.
> 
> As I said vs. patch 3/6 already this smells badly of the wrong abstractions and
> data representations. So this needs to be fixed first instead of adding several
> layers of duct tape.
> 
> > +static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
> const char *buf,
> > +			    size_t count)
> > +{
> > +	struct pps_tio *tio = dev_get_drvdata(dev);
> > +	bool enable;
> > +	int err;
> > +
> > +	err = kstrtobool(buf, &enable);
> > +	if (err)
> > +		return err;
> > +
> > +	guard(spinlock_irqsave)(&tio->lock);
> > +	if (enable && !tio->enabled) {
> > +		if (!is_current_clocksource_art_related()) {
> > +			dev_err(tio->dev, "PPS cannot be started as clock is not
> related to ART");
> > +			return -EPERM;
> > +		}
> 
> Ah. Here is the usecase for this magic patch 3/6 hackery. Again, it's the wrong
> abstraction. You want something like:
> 
>     timekeeping_clocksource_has_base(CSID_X86_ART);
> 
> or something like this, which can be handled completely in the core code.
> 
> All of this needs some serious rework. See the below disfunctional mockup patch
> for illustration.
> 
> There is also a patch series, which tried to replace the clocksource pointer in
> system_counterval_t with a clocksource ID:
> 
>   https://lore.kernel.org/all/20230818011256.211078-1-
> peter.hilber@xxxxxxxxxxxxxxx
> 
> That went nowhere, but has some valid points. I took some of Peter's (cc'ed)
> ideas into the mockup, but did it slightly different to make all of this indirection
> mess go away.
> 
> There are certainly bugs and thinkos in that mockup. If you find them, you can
> keep and fix them :)
> 

Thanks for the mock-up patch !
We are planning to include ktime_real_to_base_clock() in the next version of this patch series.

The fixes done in the mock-up patch are shared below

> Thanks,
> 
>         tglx
> ---
>  arch/x86/include/asm/tsc.h                        |    3
>  arch/x86/kernel/kvmclock.c                        |    1
>  arch/x86/kernel/tsc.c                             |   78 ++--------------
>  drivers/clocksource/arm_arch_timer.c              |    7 -
>  drivers/net/ethernet/intel/e1000e/ptp.c           |    3
>  drivers/net/ethernet/intel/ice/ice_ptp.c          |    4
>  drivers/net/ethernet/intel/igc/igc_ptp.c          |    8 +
>  drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c |    3
>  drivers/ptp/ptp_kvm_common.c                      |    9 -
>  drivers/ptp/ptp_kvm_x86.c                         |    5 -
>  include/linux/clocksource.h                       |   24 +++++
>  include/linux/clocksource_ids.h                   |    3
>  include/linux/ptp_kvm.h                           |    3
>  include/linux/timekeeping.h                       |    8 +
>  kernel/time/timekeeping.c                         |  103 ++++++++++++++++++++--
>  sound/pci/hda/hda_controller.c                    |    3
>  16 files changed, 169 insertions(+), 96 deletions(-)
> 
> --- a/arch/x86/include/asm/tsc.h
> +++ b/arch/x86/include/asm/tsc.h
> @@ -27,9 +27,6 @@ static inline cycles_t get_cycles(void)  }  #define get_cycles
> get_cycles
> 
> -extern struct system_counterval_t convert_art_to_tsc(u64 art); -extern struct
> system_counterval_t convert_art_ns_to_tsc(u64 art_ns);
> -
>  extern void tsc_early_init(void);
>  extern void tsc_init(void);
>  extern void mark_tsc_unstable(char *reason);
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -157,6 +157,7 @@ static int kvm_cs_enable(struct clocksou  struct
> clocksource kvm_clock = {
>  	.name	= "kvm-clock",
>  	.read	= kvm_clock_get_cycles,
> +	.cs_id	= CSID_X86_KVM_CLK,
>  	.rating	= 400,
>  	.mask	= CLOCKSOURCE_MASK(64),
>  	.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -50,9 +50,13 @@ int tsc_clocksource_reliable;
> 
>  static int __read_mostly tsc_force_recalibrate;
> 
> +static struct clocksource_base art_base_clk = {
> +	.id	= CSID_X86_ART,
> +};
> +
>  static u32 art_to_tsc_numerator;
>  static u32 art_to_tsc_denominator;
> -static u64 art_to_tsc_offset;
> +static u64 art_base_clk.offset;
>  static struct clocksource *art_related_clocksource;
> 
>  struct cyc2ns {
> @@ -1089,13 +1093,13 @@ static void __init detect_art(void)
>  	    tsc_async_resets)
>  		return;
> 
> -	cpuid(ART_CPUID_LEAF, &art_to_tsc_denominator,
> -	      &art_to_tsc_numerator, unused, unused+1);
> +	cpuid(ART_CPUID_LEAF, &art_base_clk.denominator,
> +	      &art_base_clk.numerator, &art_base_clk.freq_khz, unused+1);
> 
 
+    base->freq_khz /= KHZ;

> -	if (art_to_tsc_denominator < ART_MIN_DENOMINATOR)
> +	if (art_base_clk.denominator < ART_MIN_DENOMINATOR)
>  		return;
> 
> -	rdmsrl(MSR_IA32_TSC_ADJUST, art_to_tsc_offset);
> +	rdmsrl(MSR_IA32_TSC_ADJUST, art_base_clk.offset);
> 
>  	/* Make this sticky over multiple CPU init calls */
>  	setup_force_cpu_cap(X86_FEATURE_ART);
> @@ -1190,6 +1194,7 @@ static struct clocksource clocksource_ts
>  				  CLOCK_SOURCE_VALID_FOR_HRES |
>  				  CLOCK_SOURCE_MUST_VERIFY |
>  				  CLOCK_SOURCE_VERIFY_PERCPU,
> +	.id			= CSID_X86_TSC,
>  	.vdso_clock_mode	= VDSO_CLOCKMODE_TSC,
>  	.enable			= tsc_cs_enable,
>  	.resume			= tsc_resume,
> @@ -1294,65 +1299,6 @@ int unsynchronized_tsc(void)
>  	return 0;
>  }
> 
> -/*
> - * Convert ART to TSC given numerator/denominator found in detect_art()
> - */
> -struct system_counterval_t convert_art_to_tsc(u64 art) -{
> -	u64 tmp, res, rem;
> -
> -	rem = do_div(art, art_to_tsc_denominator);
> -
> -	res = art * art_to_tsc_numerator;
> -	tmp = rem * art_to_tsc_numerator;
> -
> -	do_div(tmp, art_to_tsc_denominator);
> -	res += tmp + art_to_tsc_offset;
> -
> -	return (struct system_counterval_t) {.cs = art_related_clocksource,
> -			.cycles = res};
> -}
> -EXPORT_SYMBOL(convert_art_to_tsc);
> -
> -/**
> - * convert_art_ns_to_tsc() - Convert ART in nanoseconds to TSC.
> - * @art_ns: ART (Always Running Timer) in unit of nanoseconds
> - *
> - * PTM requires all timestamps to be in units of nanoseconds. When user
> - * software requests a cross-timestamp, this function converts system
> timestamp
> - * to TSC.
> - *
> - * This is valid when CPU feature flag X86_FEATURE_TSC_KNOWN_FREQ is set
> - * indicating the tsc_khz is derived from CPUID[15H]. Drivers should check
> - * that this flag is set before conversion to TSC is attempted.
> - *
> - * Return:
> - * struct system_counterval_t - system counter value with the pointer to the
> - *	corresponding clocksource
> - *	@cycles:	System counter value
> - *	@cs:		Clocksource corresponding to system counter value.
> Used
> - *			by timekeeping code to verify comparability of two
> cycle
> - *			values.
> - */
> -
> -struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns) -{
> -	u64 tmp, res, rem;
> -
> -	rem = do_div(art_ns, USEC_PER_SEC);
> -
> -	res = art_ns * tsc_khz;
> -	tmp = rem * tsc_khz;
> -
> -	do_div(tmp, USEC_PER_SEC);
> -	res += tmp;
> -
> -	return (struct system_counterval_t) { .cs = art_related_clocksource,
> -					      .cycles = res};
> -}
> -EXPORT_SYMBOL(convert_art_ns_to_tsc);
> -
> -
>  static void tsc_refine_calibration_work(struct work_struct *work);  static
> DECLARE_DELAYED_WORK(tsc_irqwork, tsc_refine_calibration_work);
>  /**
> @@ -1454,8 +1400,10 @@ static void tsc_refine_calibration_work(
>  	if (tsc_unstable)
>  		goto unreg;
> 
> -	if (boot_cpu_has(X86_FEATURE_ART))
> +	if (boot_cpu_has(X86_FEATURE_ART)) {
>  		art_related_clocksource = &clocksource_tsc;
> +		clocksource_tsc.base_clk = &art_base_clk;
> +	}
>  	clocksource_register_khz(&clocksource_tsc, tsc_khz);
>  unreg:
>  	clocksource_unregister(&clocksource_tsc_early);
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -1784,8 +1784,7 @@ static int __init arch_timer_acpi_init(s
> TIMER_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT, arch_timer_acpi_init);
> #endif
> 
> -int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *ts,
> -				 struct clocksource **cs)
> +int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *ts, int
> +cs_id)
>  {
>  	struct arm_smccc_res hvc_res;
>  	u32 ptp_counter;
> @@ -1809,8 +1808,8 @@ int kvm_arch_ptp_get_crosststamp(u64 *cy
>  	*ts = ktime_to_timespec64(ktime);
>  	if (cycle)
>  		*cycle = (u64)hvc_res.a2 << 32 | hvc_res.a3;
> -	if (cs)
> -		*cs = &clocksource_counter;
> +	if (cs_id)
> +		*cs_id = clocksource_counter.id;
> 
>  	return 0;
>  }
> --- a/drivers/net/ethernet/intel/e1000e/ptp.c
> +++ b/drivers/net/ethernet/intel/e1000e/ptp.c
> @@ -124,7 +124,8 @@ static int e1000e_phc_get_syncdevicetime
>  	sys_cycles = er32(PLTSTMPH);
>  	sys_cycles <<= 32;
>  	sys_cycles |= er32(PLTSTMPL);
> -	*system = convert_art_to_tsc(sys_cycles);
> +	system->cycles = sys_cycles;
> +	system->cs_id = CSID_X86_ART;
> 
>  	return 0;
>  }
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1989,6 +1989,8 @@ ice_ptp_get_syncdevicetime(ktime_t *devi
>  	wr32(hw, GLHH_ART_CTL, hh_art_ctl);
> 
>  #define MAX_HH_LOCK_TRIES 100
> +	system->cs_id = CSID_X86_ART;
> +	system->nsecs = true;
> 
>  	for (i = 0; i < MAX_HH_LOCK_TRIES; i++) {
>  		/* Wait for sync to complete */
> @@ -2005,7 +2007,7 @@ ice_ptp_get_syncdevicetime(ktime_t *devi
>  			hh_ts_lo = rd32(hw, GLHH_ART_TIME_L);
>  			hh_ts_hi = rd32(hw, GLHH_ART_TIME_H);
>  			hh_ts = ((u64)hh_ts_hi << 32) | hh_ts_lo;
> -			*system = convert_art_ns_to_tsc(hh_ts);
> +			system->cycles = hh_ts;
>  			/* Read Device source clock time */
>  			hh_ts_lo = rd32(hw, GLTSYN_HHTIME_L(tmr_idx));
>  			hh_ts_hi = rd32(hw, GLTSYN_HHTIME_H(tmr_idx));
> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
> @@ -911,7 +911,13 @@ static bool igc_is_crosststamp_supported  static struct
> system_counterval_t igc_device_tstamp_to_system(u64 tstamp)  {  #if
> IS_ENABLED(CONFIG_X86_TSC) && !defined(CONFIG_UML)
> -	return convert_art_ns_to_tsc(tstamp);
> +	// FIXME: How has this ensured that ART exists?
> +	return (struct system_counterval_t) {
> +		.cs_id	= CSID_X86_ART,
> +		.cycles	= tstamp,
> +		.nsecs	= true,
> +	};
> +
>  #else
>  	return (struct system_counterval_t) { };  #endif
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> @@ -390,10 +390,11 @@ static int intel_crosststamp(ktime_t *de
>  		*device = ns_to_ktime(ptp_time);
>  		read_unlock_irqrestore(&priv->ptp_lock, flags);
>  		get_arttime(priv->mii, intel_priv->mdio_adhoc_addr,
> &art_time);
> -		*system = convert_art_to_tsc(art_time);
> +		system->cycles = art_time;
>  	}
> 
>  	system->cycles *= intel_priv->crossts_adj;
> +	system->cs_id = CSID_X86_ART;
>  	priv->plat->flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN;
> 
>  	return 0;
> --- a/drivers/ptp/ptp_kvm_common.c
> +++ b/drivers/ptp/ptp_kvm_common.c
> @@ -28,15 +28,14 @@ static int ptp_kvm_get_time_fn(ktime_t *
>  			       struct system_counterval_t *system_counter,
>  			       void *ctx)
>  {
> -	long ret;
> -	u64 cycle;
>  	struct timespec64 tspec;
> -	struct clocksource *cs;
> +	int ret, cs_id;
> +	u64 cycle;
> 
>  	spin_lock(&kvm_ptp_lock);
> 
>  	preempt_disable_notrace();
> -	ret = kvm_arch_ptp_get_crosststamp(&cycle, &tspec, &cs);
> +	ret = kvm_arch_ptp_get_crosststamp(&cycle, &tspec, &cs_id);
>  	if (ret) {
>  		spin_unlock(&kvm_ptp_lock);
>  		preempt_enable_notrace();
> @@ -46,7 +45,7 @@ static int ptp_kvm_get_time_fn(ktime_t *
>  	preempt_enable_notrace();
> 
>  	system_counter->cycles = cycle;
> -	system_counter->cs = cs;
> +	system_counter->cs_id = cs_id;
> 
>  	*device_time = timespec64_to_ktime(tspec);
> 
> --- a/drivers/ptp/ptp_kvm_x86.c
> +++ b/drivers/ptp/ptp_kvm_x86.c
> @@ -92,8 +92,7 @@ int kvm_arch_ptp_get_clock(struct timesp
>  	return 0;
>  }
> 
> -int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *tspec,
> -			      struct clocksource **cs)
> +int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *tspec,
> +int *cs_id)
>  {
>  	struct pvclock_vcpu_time_info *src;
>  	unsigned int version;
> @@ -123,7 +122,7 @@ int kvm_arch_ptp_get_crosststamp(u64 *cy
>  		*cycle = __pvclock_read_cycles(src, clock_pair->tsc);
>  	} while (pvclock_read_retry(src, version));
> 
> -	*cs = &kvm_clock;
> +	*cs_id = kvm_clock.id;
> 
>  	return 0;
>  }
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -21,6 +21,7 @@
>  #include <asm/div64.h>
>  #include <asm/io.h>
> 
> +struct clocksource_base;
>  struct clocksource;
>  struct module;
> 
> @@ -70,6 +71,7 @@ struct module;
>   *			validate the clocksource from which the snapshot was
>   *			taken.
>   * @flags:		Flags describing special properties
> + * @base_clk:		Optional pointer to an underlying base clock

+  *freq_khz:		Clock source frequency in KHz	

>   * @enable:		Optional function to enable the clocksource
>   * @disable:		Optional function to disable the clocksource
>   * @suspend:		Optional suspend function for the clocksource
> @@ -111,6 +113,7 @@ struct clocksource {
>  	enum clocksource_ids	id;
>  	enum vdso_clock_mode	vdso_clock_mode;
>  	unsigned long		flags;
> +	struct clocksource_base	*base_clk;

+  u32     freq_khz;

> 
>  	int			(*enable)(struct clocksource *cs);
>  	void			(*disable)(struct clocksource *cs);
> @@ -294,4 +297,25 @@ static inline void timer_probe(void) {}  extern ulong
> max_cswd_read_retries;  void clocksource_verify_percpu(struct clocksource
> *cs);
> 
> +/**
> + * struct clocksource_base - hardware abstraction for clock on which a
> clocksource is based
> + * @id:			Defaults to CSID_GENERIC. The id value is used
> for conversion
> + *			functions which require that the current clocksource is
> based
> + *			on a clocksource_base with a particular ID
> + *			in certain snapshot functions to allow callers to
> + *			validate the clocksource from which the snapshot was
> + *			taken.
> + * @freq_khz:		Nominal frequency of the base clock in kHz
> + * @offset:		Offset between the base clock and the clocksource
> + * @numerator:		Numerator of the clock ratio between base
> clock and the clocksource
> + * @denominator:	Denominator of the clock ratio between base clock and
> the clocksource
> + */
> +struct clocksource_base {
> +	enum clocksource_ids	id;
> +	u32			freq_khz;
> +	u64			offset;
> +	u32			numerator;
> +	u32			denominator;
> +};
> +
>  #endif /* _LINUX_CLOCKSOURCE_H */
> --- a/include/linux/clocksource_ids.h
> +++ b/include/linux/clocksource_ids.h
> @@ -6,6 +6,9 @@
>  enum clocksource_ids {
>  	CSID_GENERIC		= 0,
>  	CSID_ARM_ARCH_COUNTER,
> +	CSID_X86_TSC,
> +	CSID_X86_TSC_ART,
> +	CSID_X86_KVM_CLK,
>  	CSID_MAX,
>  };
> 
> --- a/include/linux/ptp_kvm.h
> +++ b/include/linux/ptp_kvm.h
> @@ -16,7 +16,6 @@ struct clocksource;
>  int kvm_arch_ptp_init(void);
>  void kvm_arch_ptp_exit(void);
>  int kvm_arch_ptp_get_clock(struct timespec64 *ts); -int
> kvm_arch_ptp_get_crosststamp(u64 *cycle,
> -		struct timespec64 *tspec, struct clocksource **cs);
> +int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *tspec,
> +int csid);
> 
>  #endif /* _PTP_KVM_H_ */
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -270,12 +270,14 @@ struct system_device_crosststamp {
>   * struct system_counterval_t - system counter value with the pointer to the
>   *				corresponding clocksource
>   * @cycles:	System counter value
> - * @cs:		Clocksource corresponding to system counter value.
> Used by
> - *		timekeeping code to verify comparibility of two cycle values
> + * @cs_id:	Clocksource ID. Either the ID of the current clocksource
> + *		or the ID of a clocksource base.
> + * @nsecs:	@cycles is in nanoseconds
>   */
>  struct system_counterval_t {
>  	u64			cycles;
> -	struct clocksource	*cs;
> +	enum clocksource_ids	cs_id;
> +	bool			nsecs;
>  };
> 
>  /*
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1191,6 +1191,78 @@ static bool cycle_between(u64 before, u6
>  	return false;
>  }
> 
> +static u64 convert_clock(u64 val, u32 numerator, u32 denominator) {
> +	u64 rem, res;
> +
> +	res = div_u64_rem(val, denominator, &rem) * numerator;
> +	return res + div_u64(rem * numerator, denominator); }

static bool convert_clock(u64 *val, u32 numerator, u32 denominator) {
    u64 res,rem;

        if (numerator == 0 || denominator == 0)
           return false;

    res = div64_u64_rem(*val, denominator, &rem) * numerator;
    *val = res + div_u64(rem * numerator, denominator);
   return true;
}

> +
> +static bool convert_base_to_cs(struct system_counterval_t *scv) {
> +	struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
> +	struct clocksource_base *csb = clock->base;
> +
> +	/* The timestamp was taken from the time keeper clock source */
> +	if (cs->id == scv->cs_id)
> +		return true;
> +
> +	/* Check whether cs_id matches the base clock */
> +	if (!base || base->id != scv->cs_id)
> +		return false;
> +
> +	if (scv->nsecs)
> +		scv->cycles = convert_clock(scv->cycles, base->freq_khz,
> +USEC_PER_SEC);
> +
> +	scv->cycles = convert_clock(scv->cycles, base->numerator, base-
> >denominator);

/* Avoid conversion to a less precise clock */
if (scv->nsecs && && cs->freq_khz != 0 && base->freq_khz < cs->freq_khz) {
           if (!convert_clock(&(scv->cycles), cs->freq_khz, USEC_PER_SEC))
	    return false;
    } else {
           if (scv->nsecs)
                   if (!convert_clock(&(scv->cycles), base->freq_khz, USEC_PER_SEC))
                           return false;
           if (!convert_clock(&(scv->cycles), base->numerator, base->denominator))
                   return false;
    }

> +	scv->cycles += base->offset;
> +	return true;
> +}
> +
> +static bool convert_cs_to_base(u64 *cycles, enum clocksource_ids
> +base_id) {
> +	struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
> +	struct clocksource_base *csb = clock->base;
> +
> +	/* Check whether base_id matches the base clock */
> +	if (!base || base->id != base_id)
> +		return false;
> +
> +	*cycles = convert_clock(cycles - base->offset, base->denominator,
> base->numerator);
> +	return true;
> +}
> +
> +static u64 convert_ns_to_cs(u64 delta)
> +{
> +	struct tk_read *tkr = &tk_core.timekeeper.tkr_mono;
> +
> +	return div_u64(delta << tkr->shift, tkr->mult); }

return div_u64( (delta << tkr->shift) - tkr->xtime_nsec, tkr->mult);

> +
> +bool ktime_real_to_base_clock(ktime_t treal, enum clocksource_ids
> +base_id, u64 *cycles) {
> +	struct timekeeper *tk = &tk_core.timekeeper;
> +	struct clocksource_base *csb;
> +	unsigned int seq;
> +	u64 delta;
> +
> +	do {
> +		seq = read_seqcount_begin(&tk_core.seq);
> +
> +		delta = (u64)treal - tk->tkr_mono.base_real;
> +		if (delta > tk->tkr_mono.clock->max_idle_ns)
> +			return false;
> +
> +		*cycles = tk->tkr_mono.cycle_last + convert_ns_to_cs(delta);
> +		if (!convert_cs_to_base(cycles, base_id))
> +			return false;
> +
> +	} while (read_seqcount_retry(&tk_core.seq, seq));
> +
> +	return true;
> +}
> +
>  /**
>   * get_device_system_crosststamp - Synchronously capture system/device
> timestamp
>   * @get_time_fn:	Callback to get simultaneous device time and
> @@ -1231,13 +1303,9 @@ int get_device_system_crosststamp(int (*
>  		if (ret)
>  			return ret;
> 
> -		/*
> -		 * Verify that the clocksource associated with the captured
> -		 * system counter value is the same as the currently installed
> -		 * timekeeper clocksource
> -		 */
> -		if (tk->tkr_mono.clock != system_counterval.cs)
> +		if (!convert_base_to_cs(&system_counterval))
>  			return -ENODEV;
> +
>  		cycles = system_counterval.cycles;
> 
>  		/*
> @@ -1304,6 +1372,29 @@ int get_device_system_crosststamp(int (*
> EXPORT_SYMBOL_GPL(get_device_system_crosststamp);
> 
>  /**
> + * timekeeping_clocksource_has_base - Check whether the current clocksource
> has a base clock
> + * @id:	The clocksource ID to check for
> + *
> + * Note: The return value is a snapshot which can become invalid right
> + *	 after the function returns.
> + *
> + * Returns: True if the timekeeper clocksource has a base clock with
> +@id, false otherwise  */ bool timekeeping_clocksource_has_base(enum
> +clocksource_ids id) {
> +	unsigned int seq;
> +	bool ret;
> +
> +	do {
> +		seq = read_seqcount_begin(&tk_core.seq);
> +		ret = tk_core.timekeeper.tkr_mono.clock->base ?
> +			tk_core.timekeeper.tkr_mono.clock->base.id == id :
> false;
> +	} (read_seqcount_retry(&tk_core.seq, seq));
> +
> +	return ret;
> +}
> +
> +/**
>   * do_settimeofday64 - Sets the time of day.
>   * @ts:     pointer to the timespec64 variable containing the new time
>   *
> --- a/sound/pci/hda/hda_controller.c
> +++ b/sound/pci/hda/hda_controller.c
> @@ -457,7 +457,8 @@ static int azx_get_sync_time(ktime_t *de
>  	*device = ktime_add_ns(*device, (wallclk_cycles * NSEC_PER_SEC) /
>  			       ((HDA_MAX_CYCLE_VALUE + 1) * runtime->rate));
> 
> -	*system = convert_art_to_tsc(tsc_counter);
> +	system->cycles = tsc_counter;
> +	system->cs_id = CSID_X86_ART;
> 
>  	return 0;
>  }

Regards
Lakshmi Sowjanya D