diff mbox

[GIT,PULL] Greybus driver subsystem for 4.9-rc1

Message ID 20160914182952.GA21615@kroah.com (mailing list archive)
State New, archived
Headers show

Commit Message

Greg Kroah-Hartman Sept. 14, 2016, 6:29 p.m. UTC
On Wed, Sep 14, 2016 at 08:07:54PM +0200, Greg KH wrote:
> On Wed, Sep 14, 2016 at 06:36:26PM +0100, Mark Rutland wrote:
> > Hi Greg,
> > 
> > On Wed, Sep 14, 2016 at 12:09:49PM +0200, Greg KH wrote:
> > > Given that it's never a good idea to keep subsystems out of the mainline
> > > kernel, I've put together this pull request that adds the greybus driver
> > > layer to drivers/greybus/.  Because this was 2 1/2 years of work, with
> > > many many developers contributing, I didn't want to flatten all of their
> > > effort into a few small patches, as that wouldn't be very fair.  So I've
> > > built a git tree with all of the changes going back to the first commit,
> > > and merged it into the kernel tree, just like btrfs was merged into the
> > > kernel.
> > 
> > > Unless people point out some major problems with this, I'd like to get
> > > it merged into 4.9-rc1.
> > 
> > I'm extremely concerned that these patches have *never* seen upstream
> > review, and this pull request gives no real opportunity for people to
> > make a judgement regarding the code, as many relevant parties have not
> > been Cc'd.
> 
> As I said, I will send a set of simple patches, I wanted to get this out
> as soon as possible and other things came up today.  Will do it in the
> morning, sorry.

Here's the timesync code pulled out into a simple patch if you want to
see it.

Bryan, any explanations you want to provide that would help in
clarifying Mark's issues?

thanks,

greg k-h


---
 drivers/greybus/timesync.c          | 1357 ++++++++++++++++++++++++++++++++++++
 drivers/greybus/timesync.h          |   45 +
 drivers/greybus/timesync_platform.c |   77 ++
 3 files changed, 1479 insertions(+)

Comments

Joe Perches Sept. 14, 2016, 7:05 p.m. UTC | #1
On Wed, 2016-09-14 at 20:29 +0200, Greg KH wrote:

trivial note:

> +static size_t gb_timesync_log_frame_time(struct gb_timesync_svc *timesync_svc,
> +					 char *buf, size_t buflen)
> +{
> +	struct gb_svc *svc = timesync_svc->svc;
> +	struct gb_host_device *hd;
> +	struct gb_timesync_interface *timesync_interface;
> +	struct gb_interface *interface;
> +	unsigned int len;
> +	size_t off;
> +
> +	/* AP/SVC */
> +	off = snprintf(buf, buflen, "%s frametime: ap=%llu %s=%llu ",
> +		       greybus_bus_type.name,
> +		       timesync_svc->ap_ping_frame_time, dev_name(&svc->dev),
> +		       timesync_svc->svc_ping_frame_time);
> +	len = buflen - off;
> +
> +	/* APB/GPB */
> +	if (len < buflen) {
> +		hd = timesync_svc->timesync_hd->hd;
> +		off += snprintf(&buf[off], len, "%s=%llu ", dev_name(&hd->dev),
> +				timesync_svc->timesync_hd->ping_frame_time);
> +		len = buflen - off;
> +	}
> +
> +	list_for_each_entry(timesync_interface,
> +			    &timesync_svc->interface_list, list) {
> +		if (len < buflen) {
> +			interface = timesync_interface->interface;
> +			off += snprintf(&buf[off], len, "%s=%llu ",
> +					dev_name(&interface->dev),
> +					timesync_interface->ping_frame_time);
> +			len = buflen - off;
> +		}
> +	}
> +	if (len < buflen)
> +		off += snprintf(&buf[off], len, "\n");
> +	return off;
> +}

The unnecessary trailing blank can be avoided by converting

	snprintf(, "format ', ...);
	for (...)
		snprintf(, "format ", ...);
	snprintf(, "\n");

to

	snprintf(, "format', ...);
	for (...)
		snprintf(, " format", ...);
	snprintf(, "\n");

>
Bryan O'Donoghue Sept. 15, 2016, 9:35 a.m. UTC | #2
On Wed, 2016-09-14 at 20:29 +0200, Greg KH wrote:
> On Wed, Sep 14, 2016 at 08:07:54PM +0200, Greg KH wrote:
> > 
> > On Wed, Sep 14, 2016 at 06:36:26PM +0100, Mark Rutland wrote:
> > > 
> > > Hi Greg,
> > > 
> > > On Wed, Sep 14, 2016 at 12:09:49PM +0200, Greg KH wrote:
> > > > 
> > > > Given that it's never a good idea to keep subsystems out of the
> > > > mainline
> > > > kernel, I've put together this pull request that adds the
> > > > greybus driver
> > > > layer to drivers/greybus/.  Because this was 2 1/2 years of
> > > > work, with
> > > > many many developers contributing, I didn't want to flatten all
> > > > of their
> > > > effort into a few small patches, as that wouldn't be very
> > > > fair.  So I've
> > > > built a git tree with all of the changes going back to the
> > > > first commit,
> > > > and merged it into the kernel tree, just like btrfs was merged
> > > > into the
> > > > kernel.
> > > > 
> > > > Unless people point out some major problems with this, I'd like
> > > > to get
> > > > it merged into 4.9-rc1.
> > > I'm extremely concerned that these patches have *never* seen
> > > upstream
> > > review, and this pull request gives no real opportunity for
> > > people to
> > > make a judgement regarding the code, as many relevant parties
> > > have not
> > > been Cc'd.
> > As I said, I will send a set of simple patches, I wanted to get
> > this out
> > as soon as possible and other things came up today.  Will do it in
> > the
> > morning, sorry.
> Here's the timesync code pulled out into a simple patch if you want
> to
> see it.
> 
> Bryan, any explanations you want to provide that would help in
> clarifying Mark's issues?

As Douglas Adams would say - "don't panic".

If you look at the final state the code ends up in - we're doing
get_cycles(); as opposed to reading an architectural timer directly.

u64 gb_timesync_platform_get_counter(void)
{
        return (u64)get_cycles();
}

You have the entire git history - from the early days where we were reading one of the unused ARMv8 timers the MSM8994 has to the later days where we just do get_cycles()...

At the time when we first started writing the code it wasn't 100% clear if get_cycles() would do, so it was safer to allocate an unused architectural timer and read it directly. Later on and with some experimentation it was possible to switch to get_cycles().

---
bod
Mark Rutland Sept. 15, 2016, 10:13 a.m. UTC | #3
On Thu, Sep 15, 2016 at 10:35:33AM +0100, Bryan O'Donoghue wrote:
> On Wed, 2016-09-14 at 20:29 +0200, Greg KH wrote:
> > Bryan, any explanations you want to provide that would help in
> > clarifying Mark's issues?
> 
> As Douglas Adams would say - "don't panic".
> 
> If you look at the final state the code ends up in - we're doing
> get_cycles(); as opposed to reading an architectural timer directly.
>
> u64 gb_timesync_platform_get_counter(void)
> {
>         return (u64)get_cycles();
> }

I did in fact notice this, though my wording was somewhat unclear on
that part.

> You have the entire git history - from the early days where we were
> reading one of the unused ARMv8 timers the MSM8994 has to the later
> days where we just do get_cycles()...
>
> At the time when we first started writing the code it wasn't 100%
> clear if get_cycles() would do, so it was safer to allocate an unused
> architectural timer and read it directly. Later on and with some
> experimentation it was possible to switch to get_cycles().

I don't think the history matters, and I don't think that one can rely
on get_cycles() in this manner outside of arch code. Looking at the
state of the tree [1] as of the final commit [2] in the greybus branch,
my points still stand:

* The "google,greybus-frame-time-counter" node is superfluous. It does
  not describe a particular device, and duplicates information we have
  elsewhere. It does not explicitly define the relationship with the
  underlying clocksource.

* The clock-frequency property isn't necessary. The architected timer
  drivers know the frequency of the architected timers (MMIO or sysreg),
  and they should be queried as to the frequency.

* In general, get_cycles() isn't guaranteed to be any clocksource or
  cycle counter in particular, so even if the clock-frequency property
  matches the architected timer, that doesn't help.

Beyond that, the fallback code using cpufreq and presumably an actual
cycle counter will be broken in a number of cases -- cycle counts will
not match across CPUs, and on CPUs whic gate clocks in WFI/WFE, they'll
drift randomly.

Per the comment at the top of the file, it looks like you want a
system-wide stable clocksource. If you want that, you need to use a
generic API that allows drivers and arch code to actually provide that,
rather than building one yourself that doesn't work.

If you're trying to synchronise with other agents in the system that are
reading from the MMIO arch timers, then that relationship should be
described explicitly in the DT.

Thanks,
Mark.
 
[1] https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/tree/drivers/greybus/timesync_platform.c?h=greybus&id=f86bfc90a401681838866b5f6826d86cc4c7010c
[2] https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=greybus&id=f86bfc90a401681838866b5f6826d86cc4c7010c
Bryan O'Donoghue Sept. 15, 2016, 10:35 a.m. UTC | #4
On Thu, 2016-09-15 at 11:13 +0100, Mark Rutland wrote:
> On Thu, Sep 15, 2016 at 10:35:33AM +0100, Bryan O'Donoghue wrote:
> > 
> I don't think the history matters, 

Your comment seemed to indicate you thought we were reading a
architectural timer directly - which we aren't.

> and I don't think that one can rely
> on get_cycles() in this manner outside of arch code.

I don't follow your meaning. What's wrong with get_cycles() ? You've
already said you don't think reading an architectural timer directly is
correct.

The objective is to read one of the free-running counters in MSM8994,
clocked by the PMIC. The refclk provided by PMIC is distributed to each
processor in the system.

>  Looking at the
> state of the tree [1] as of the final commit [2] in the greybus
> branch,
> my points still stand:
> 
> * The "google,greybus-frame-time-counter" node is superfluous. It
> does
>   not describe a particular device,

It describes a timer running @ 19.2MHz, clocked by PMIC refclk.

>  and duplicates information we have
>   elsewhere.

Can you give an example ?

>  It does not explicitly define the relationship with the
>   underlying clocksource.


> * The clock-frequency property isn't necessary. The architected timer
>   drivers know the frequency of the architected timers (MMIO or
> sysreg),
>   and they should be queried as to the frequency.

OK so if I'm understanding you. You think get_cycles() is fine but that
instead of encoding a "greybus-frame-time-counter" the platform code
should interrogate the frequency provided - is that correct ?

> Beyond that, the fallback code using cpufreq and presumably an actual
> cycle counter will be broken in a number of cases

Of course the fallback will be broken... it's not supposed to work if
you don't have a timer that can be used - just compile, run and print a
complaint - i.e., this won't really do FrameTime on an x86... then
again since so much of the underlying greybus/unipro hardware -
requires a 19.2MHz refclk - if you were to try to do greybus on x86
you'd need to solve that problem.

> 
> Per the comment at the top of the file, it looks like you want a
> system-wide stable clocksource. If you want that, you need to use a
> generic API that allows drivers and arch code to actually provide
> that,
> rather than building one yourself that doesn't work.

Hmm. The objective is to read one of the timers clocked by the PMIC
refclk input. refclk is provided to each processor in the system - and
on MSM8994 clocks the MMIO timers. It's used to drive the PLL on the
other processors - which in turn drive the timers that the Modules use
to read their own local counters. We want to read that counter on MSM
directly - get_cycles() has worked nicely so far.

> 
> If you're trying to synchronise with other agents in the system that
> are
> reading from the MMIO arch timers,

No. The MMIO timers are useful only to the MSM. We don't have any type
of parallel (or serial) bus that can access that on-chip resource.

MSM8994 -- > USB
             APBridge (timer) -> UniPro bus
                                            -> Module with a UART
                                            -> Module with a GPIO
                                            -> Module with an etc, etc
                              -> SPI bus
                                            -> SVC
                                               Owns FrameTime

So the SVC owns FrameTime and diseminates that to other entities in the
system by way of a GPIO and greybus. It's up to the MSM8994 to select a
timer that works for it - the other processors in the system are
responsible for their own timers.

---
bod
Bryan O'Donoghue Sept. 15, 2016, 10:47 a.m. UTC | #5
On Thu, 2016-09-15 at 11:35 +0100, Bryan O'Donoghue wrote:
> 

Here's a slightly better diagram.

PMIC -> refclk provided to each (timer) element below.

MSM8994(timer) -- > USB
WD8a
               APBridgeA (timer) -> UniPro bus
               WD8a
                                            -> Module(timer) with UART
                                               WD1
                                            -> Module(timer) with GPIO
                                               WD2
                                            -> Module(timer) with blah
                                               WD3
                              -> SPI bus
                                            -> SVC(timer)
                                               Owns FrameTime
                                               GPIO {WD0...WDn}

So yes, each processor has it's own timer. We aren't trying to read the
MSM's FrameTime.

---
bod
Mark Rutland Sept. 15, 2016, 11:20 a.m. UTC | #6
Hi,

More questions below. Perhaps some of these will be implicitly answered
when the linearised patches appear, and I'm happy to wait until then to
continue the discussion, as I suspect otherwise we're all likely to end
up exasperated.

Please do Cc me on those.

Regardless, until those appear and a reasonable time has been given for
replies, my comments regarding the lack of review stand, as does my NAK
for the series.

On Thu, Sep 15, 2016 at 11:35:56AM +0100, Bryan O'Donoghue wrote:
> On Thu, 2016-09-15 at 11:13 +0100, Mark Rutland wrote:
> > On Thu, Sep 15, 2016 at 10:35:33AM +0100, Bryan O'Donoghue wrote:
> > > 
> > I don't think the history matters, 
> 
> Your comment seemed to indicate you thought we were reading a
> architectural timer directly - which we aren't.

Sure, and as I pointed out, the comment in the HEAD commit still claims
it does, even if the code doesn't. This is at best, confusing, and the
history of how it came to be there doesn't really matter...

> > and I don't think that one can rely
> > on get_cycles() in this manner outside of arch code.
> 
> I don't follow your meaning. What's wrong with get_cycles() ? You've
> already said you don't think reading an architectural timer directly is
> correct.

I pointed out a number of issues in my previous reply.

For example, you have absolutely no guarantee as to what backs
get_cycles(). Despite this, the code assumes that get_cycles() is backed
by something running at the frequency described in a
"google,greybus-frame-time-counter" node.

Even if this *happens* to match what some piece of arch code provides
today on some platform, it is in no way *guaranteed*.

> The objective is to read one of the free-running counters in MSM8994,
> clocked by the PMIC. The refclk provided by PMIC is distributed to each
> processor in the system.
> 
> >  Looking at the
> > state of the tree [1] as of the final commit [2] in the greybus
> > branch,
> > my points still stand:
> > 
> > * The "google,greybus-frame-time-counter" node is superfluous. It
> > does
> >   not describe a particular device,
> 
> It describes a timer running @ 19.2MHz, clocked by PMIC refclk.

... which you assume is whatever backs get_cycles(), which you in
practice assume is the architected timer. For which we *already* have a
binding and driver.

Given that, as far as I can tell, "google,greybus-frame-time-counter"
describes a software construct that uses this, not an actual piece of
hardware.

> >  and duplicates information we have   elsewhere.
> 
> Can you give an example ?

Trivially, the CNTFRQ register in the architected timer (which is common
across MMIO/sysreg), which you can query with arch_timer_get_rate().

Note that isn't guaranteed to match get_cycles() either. You need a
better API to call.

> > * The clock-frequency property isn't necessary. The architected timer
> >   drivers know the frequency of the architected timers (MMIO or
> > sysreg),
> >   and they should be queried as to the frequency.
> 
> OK so if I'm understanding you. You think get_cycles() is fine but that
> instead of encoding a "greybus-frame-time-counter" the platform code
> should interrogate the frequency provided - is that correct ?

You should definitely interrogate the relevant driver, somehow.

Without a higher-level view of what you're trying to achieve, it's not
clear to me whether get_cycles() is the right interface.

> > Beyond that, the fallback code using cpufreq and presumably an actual
> > cycle counter will be broken in a number of cases
> 
> Of course the fallback will be broken... it's not supposed to work if
> you don't have a timer that can be used - just compile, run and print a
> complaint - i.e., this won't really do FrameTime on an x86... then
> again since so much of the underlying greybus/unipro hardware -
> requires a 19.2MHz refclk - if you were to try to do greybus on x86
> you'd need to solve that problem.

If it's never going to work, why give the illusion that it might?

If you don't have the necessary prerequisites, fail to probe entirely.
Prevent drivers that depend on the non-existent functionality from
probing, or have them avoid the facility which is not available.

Don't provide them with something that can appear to work for a while,
yet will fall over in a breeze.

> > Per the comment at the top of the file, it looks like you want a
> > system-wide stable clocksource. If you want that, you need to use a
> > generic API that allows drivers and arch code to actually provide
> > that, rather than building one yourself that doesn't work.
> 
> Hmm. The objective is to read one of the timers clocked by the PMIC
> refclk input. refclk is provided to each processor in the system - and
> on MSM8994 clocks the MMIO timers. It's used to drive the PLL on the
> other processors - which in turn drive the timers that the Modules use
> to read their own local counters. We want to read that counter on MSM
> directly - get_cycles() has worked nicely so far.

This is too low-level for me to see what you're actually trying to
achieve. The fact that you want to read a specific timer is an
implementation detail.

Why can't you use any other timer? Correctness? Performance? (Why) is
the fact that the PLL drives module timers important?

> > If you're trying to synchronise with other agents in the system that
> > are reading from the MMIO arch timers,
> 
> No. The MMIO timers are useful only to the MSM. We don't have any type
> of parallel (or serial) bus that can access that on-chip resource.

To be clear, by "other agents in the system", I'm also asking about
other devices within the SoC (e.g. anything other than the CPUs running
this instance of Linux).

> MSM8994 -- > USB
>              APBridge (timer) -> UniPro bus
>                                             -> Module with a UART
>                                             -> Module with a GPIO
>                                             -> Module with an etc, etc
>                               -> SPI bus
>                                             -> SVC
>                                                Owns FrameTime
> 
> So the SVC owns FrameTime and diseminates that to other entities in the
> system by way of a GPIO and greybus. It's up to the MSM8994 to select a
> timer that works for it - the other processors in the system are
> responsible for their own timers.

Sorry, but this doesn't clarify much from my PoV.

* What are the requirements for that timer? 

* Is there_any_ implicit relationship with the module timers derived
  from the fact they share a parent PLL? Is that a requirement somehow?

Thanks,
Mark.
Bryan O'Donoghue Sept. 15, 2016, 11:48 a.m. UTC | #7
On Thu, 2016-09-15 at 12:20 +0100, Mark Rutland wrote:
> Hi,
> 
> More questions below. Perhaps some of these will be implicitly
> answered
> when the linearised patches appear, and I'm happy to wait until then
> to
> continue the discussion, as I suspect otherwise we're all likely to
> end
> up exasperated.
> 
> Please do Cc me on those.
> 
> Regardless, until those appear and a reasonable time has been given
> for
> replies, my comments regarding the lack of review stand, as does my
> NAK
> for the series.
> 
> On Thu, Sep 15, 2016 at 11:35:56AM +0100, Bryan O'Donoghue wrote:
> > 
> > On Thu, 2016-09-15 at 11:13 +0100, Mark Rutland wrote:
> > > 
> > > On Thu, Sep 15, 2016 at 10:35:33AM +0100, Bryan O'Donoghue wrote:
> > > > 
> > > >  
> > > I don't think the history matters, 
> > Your comment seemed to indicate you thought we were reading a
> > architectural timer directly - which we aren't.
> Sure, and as I pointed out, the comment in the HEAD commit still
> claims
> it does, even if the code doesn't. This is at best, confusing, and
> the
> history of how it came to be there doesn't really matter...

TBH a whole git history will invariably contain things developers did,
thought better of and then backed out as is the case here.

> 
> > 
> > > 
> > > and I don't think that one can rely
> > > on get_cycles() in this manner outside of arch code.
> > I don't follow your meaning. What's wrong with get_cycles() ?
> > You've
> > already said you don't think reading an architectural timer
> > directly is
> > correct.
> I pointed out a number of issues in my previous reply.

On MSM8994 the timer backing get_cycles() is one of the MMIO
architectural timers (which is why I switched over in the end). There's
not much else that can be done bar custom silicon - this particular
timer is as good as it gets, more of a "how do we synchronise time with
the hardware we have" than a "lets design in a feature to synchronise
time" - which was something we were focusing in on for later
silicon... 

> 
> For example, you have absolutely no guarantee as to what backs
> get_cycles(). Despite this, the code assumes that get_cycles() is
> backed
> by something running at the frequency described in a
> "google,greybus-frame-time-counter" node.
> 
> Even if this *happens* to match what some piece of arch code provides
> today on some platform, it is in no way *guaranteed*.

That's the point though, if you declare "google,greybus-frame-time-
counter" in your platform code - then you can use 'get_cycles()' in
this manner - if not - then you need to take steps in your own new
platform to provide that same level of functionality. You could switch
to an MSM8996 or an MSM8998 declare this node and bob's your uncle.

OTOH declaring this node on x86 would be a bit pointless. You'd be
better off providing a timer on a PCI bar, and binding that into
greybus with some x86/x86-platform code...

> 
> > 
> > The objective is to read one of the free-running counters in
> > MSM8994,
> > clocked by the PMIC. The refclk provided by PMIC is distributed to
> > each
> > processor in the system.
> > 
> > > 
> > >  Looking at the
> > > state of the tree [1] as of the final commit [2] in the greybus
> > > branch,
> > > my points still stand:
> > > 
> > > * The "google,greybus-frame-time-counter" node is superfluous. It
> > > does
> > >   not describe a particular device,
> > It describes a timer running @ 19.2MHz, clocked by PMIC refclk.
> ... which you assume is whatever backs get_cycles(), which you in
> practice assume is the architected timer. For which we *already* have
> a
> binding and driver.

It's a requirement rather than assumption. If you declare that node,
it's assumed the timer driving get_cycles() does what it says on the
greybus-frame-time-counter tin.

> > >  and duplicates information we have   elsewhere.
> > Can you give an example ?
> Trivially, the CNTFRQ register in the architected timer (which is
> common
> across MMIO/sysreg), which you can query with arch_timer_get_rate().
> 
> Note that isn't guaranteed to match get_cycles() either. You need a
> better API to call.

In that case a DT entry makes sense I'd say.

> You should definitely interrogate the relevant driver, somehow.

Hrmm. TBH if we are ruling out arch_timer_get_rate() then I think a DT
entry (which BTW is greybus specific) is the more intelligent way
forward. The greybus platform implementer needs to understand the
dependencies and take action to meet those dependencies should he or
she wish to support this feature.

I'm not opposed necessarily to calling arch_timer_get_rate() instead of
a DT binding, assuming it works, and drawing a line under it for
MSM8994. As I've said it's up to a system architect for other platforms
to go and do the necessary design to support this feature and this will
almost certainly require new platform code both here and in other
places anyway.

> 
> Without a higher-level view of what you're trying to achieve, it's
> not
> clear to me whether get_cycles() is the right interface.

I appreciate that.

---
bod
Mark Rutland Sept. 15, 2016, 12:46 p.m. UTC | #8
On Thu, Sep 15, 2016 at 12:48:08PM +0100, Bryan O'Donoghue wrote:
> On Thu, 2016-09-15 at 12:20 +0100, Mark Rutland wrote:
> > For example, you have absolutely no guarantee as to what backs
> > get_cycles(). Despite this, the code assumes that get_cycles() is
> > backed by something running at the frequency described in a
> > "google,greybus-frame-time-counter" node.
> > 
> > Even if this *happens* to match what some piece of arch code provides
> > today on some platform, it is in no way *guaranteed*.
> 
> That's the point though, if you declare "google,greybus-frame-time-
> counter" in your platform code - then you can use 'get_cycles()' in
> this manner

To be clear, *some* properties (and perhaps additional nodes) may need
to be in the DT, in order to capture the hardware property or
relationship that you are reliant upon.

However, the DT cannot possibly know anything about get_cycles(), as
get_cycles() is a kernel implementation details that's subject to
arbitrary change at any point in time, independent of the DT.

The "google,greybus-frame-time-counter" node, as it stands, does not
capture any relevant hardware detail, and does not belong in the DT.

> > Without a higher-level view of what you're trying to achieve, it's
> > not clear to me whether get_cycles() is the right interface.
> 
> I appreciate that.

Until that's clarified, we won't make any progress here.

Thanks,
Mark.
Bryan O'Donoghue Sept. 15, 2016, 3:40 p.m. UTC | #9
On Thu, 2016-09-15 at 13:46 +0100, Mark Rutland wrote:
> On Thu, Sep 15, 2016 at 12:48:08PM +0100, Bryan O'Donoghue wrote:
> > 
> > On Thu, 2016-09-15 at 12:20 +0100, Mark Rutland wrote:
> > > 
> > > For example, you have absolutely no guarantee as to what backs
> > > get_cycles(). Despite this, the code assumes that get_cycles() is
> > > backed by something running at the frequency described in a
> > > "google,greybus-frame-time-counter" node.
> > > 
> > > Even if this *happens* to match what some piece of arch code
> > > provides
> > > today on some platform, it is in no way *guaranteed*.
> > That's the point though, if you declare "google,greybus-frame-time-
> > counter" in your platform code - then you can use 'get_cycles()' in
> > this manner
> To be clear, *some* properties (and perhaps additional nodes) may
> need
> to be in the DT, in order to capture the hardware property or
> relationship that you are reliant upon.

Sure but on the relevant platform we know 

1. get_cycles() is derived from an architectured MMIO timer
2. What is clocking that timer

So any platform that declares that property must be aware of what its
doing.

For clarity this is the alternative to reading another register
directly bypassing get_cycles().


> > 
> > > 
> > > Without a higher-level view of what you're trying to achieve,
> > > it's
> > > not clear to me whether get_cycles() is the right interface.
> > I appreciate that.
> Until that's clarified, we won't make any progress here.

Let's see. We're synchronizing a set of distributed timers via a GPIO
pulse. Each processor on the system has at least one timer block
directly driven by the refclk@19.2MHz or a PLL driven by refclk@19.2MHz

FrameTime is a 64 bit free-running counter running @19.2MHz that is
used as a common source of reference for events. On the MSM side this
implies reading one of the timers driven by that refclk directly.

The choices on MSM8994 to access one of those timer blocks are

1. Read the register directly as is done in earlier patches or
2. Read get_cycles() as is done in later patches.

It's no more complex than that.

---
bod
Mark Rutland Sept. 15, 2016, 3:47 p.m. UTC | #10
On Thu, Sep 15, 2016 at 04:40:00PM +0100, Bryan O'Donoghue wrote:
> On Thu, 2016-09-15 at 13:46 +0100, Mark Rutland wrote:
> > On Thu, Sep 15, 2016 at 12:48:08PM +0100, Bryan O'Donoghue wrote:
> > > On Thu, 2016-09-15 at 12:20 +0100, Mark Rutland wrote:
> > > > For example, you have absolutely no guarantee as to what backs
> > > > get_cycles(). Despite this, the code assumes that get_cycles() is
> > > > backed by something running at the frequency described in a
> > > > "google,greybus-frame-time-counter" node.
> > > > 
> > > > Even if this *happens* to match what some piece of arch code
> > > > provides
> > > > today on some platform, it is in no way *guaranteed*.
> > > That's the point though, if you declare "google,greybus-frame-time-
> > > counter" in your platform code - then you can use 'get_cycles()' in
> > > this manner
> > To be clear, *some* properties (and perhaps additional nodes) may
> > need
> > to be in the DT, in order to capture the hardware property or
> > relationship that you are reliant upon.
> 
> Sure but on the relevant platform we know 
> 
> 1. get_cycles() is derived from an architectured MMIO timer
> 2. What is clocking that timer
> 
> So any platform that declares that property must be aware of what its
> doing.

I can't say this any more explicitly:

****************************************************
* The DT *cannot* know anything about get_cycles() *
****************************************************

It's no more complex than that.

Thanks,
Mark.
Bryan O'Donoghue Sept. 15, 2016, 4:09 p.m. UTC | #11
On Thu, 2016-09-15 at 16:47 +0100, Mark Rutland wrote:
> On 
> I can't say this any more explicitly:
> 
> ****************************************************
> * The DT *cannot* know anything about get_cycles() *
> ****************************************************
> 
> It's no more complex than that.

Sure think Mark I understand your point.

I think to satisfy your objection the most sensible change to make is
using arch_timer_get_rate() compared to a desired rate of 19200000.

---
bod
diff mbox

Patch

--- /dev/null
+++ b/drivers/greybus/timesync.c
@@ -0,0 +1,1357 @@ 
+/*
+ * TimeSync API driver.
+ *
+ * Copyright 2016 Google Inc.
+ * Copyright 2016 Linaro Ltd.
+ *
+ * Released under the GPLv2 only.
+ */
+#include <linux/debugfs.h>
+#include <linux/hrtimer.h>
+#include "greybus.h"
+#include "timesync.h"
+#include "greybus_trace.h"
+
+/*
+ * Minimum inter-strobe value of one millisecond is chosen because it
+ * just-about fits the common definition of a jiffy.
+ *
+ * Maximum value OTOH is constrained by the number of bits the SVC can fit
+ * into a 16 bit up-counter. The SVC configures the timer in microseconds
+ * so the maximum allowable value is 65535 microseconds. We clip that value
+ * to 10000 microseconds for the sake of using nice round base 10 numbers
+ * and since right-now there's no imaginable use-case requiring anything
+ * other than a one millisecond inter-strobe time, let alone something
+ * higher than ten milliseconds.
+ */
+#define GB_TIMESYNC_STROBE_DELAY_US		1000
+#define GB_TIMESYNC_DEFAULT_OFFSET_US		1000
+
+/* Work queue timers long, short and SVC strobe timeout */
+#define GB_TIMESYNC_DELAYED_WORK_LONG		msecs_to_jiffies(10)
+#define GB_TIMESYNC_DELAYED_WORK_SHORT		msecs_to_jiffies(1)
+#define GB_TIMESYNC_MAX_WAIT_SVC		msecs_to_jiffies(5000)
+#define GB_TIMESYNC_KTIME_UPDATE		msecs_to_jiffies(1000)
+#define GB_TIMESYNC_MAX_KTIME_CONVERSION	15
+
+/* Maximum number of times we'll retry a failed synchronous sync */
+#define GB_TIMESYNC_MAX_RETRIES			5
+
+/* Reported nanoseconds/femtoseconds per clock */
+static u64 gb_timesync_ns_per_clock;
+static u64 gb_timesync_fs_per_clock;
+
+/* Maximum difference we will accept converting FrameTime to ktime */
+static u32 gb_timesync_max_ktime_diff;
+
+/* Reported clock rate */
+static unsigned long gb_timesync_clock_rate;
+
+/* Workqueue */
+static void gb_timesync_worker(struct work_struct *work);
+
+/* List of SVCs with one FrameTime per SVC */
+static LIST_HEAD(gb_timesync_svc_list);
+
+/* Synchronize parallel contexts accessing a valid timesync_svc pointer */
+static DEFINE_MUTEX(gb_timesync_svc_list_mutex);
+
+/* Structure to convert from FrameTime to timespec/ktime */
+struct gb_timesync_frame_time_data {
+	u64 frame_time;
+	struct timespec ts;
+};
+
+struct gb_timesync_svc {
+	struct list_head list;
+	struct list_head interface_list;
+	struct gb_svc *svc;
+	struct gb_timesync_host_device *timesync_hd;
+
+	spinlock_t spinlock;	/* Per SVC spinlock to sync with ISR */
+	struct mutex mutex;	/* Per SVC mutex for regular synchronization */
+
+	struct dentry *frame_time_dentry;
+	struct dentry *frame_ktime_dentry;
+	struct workqueue_struct *work_queue;
+	wait_queue_head_t wait_queue;
+	struct delayed_work delayed_work;
+	struct timer_list ktime_timer;
+
+	/* The current local FrameTime */
+	u64 frame_time_offset;
+	struct gb_timesync_frame_time_data strobe_data[GB_TIMESYNC_MAX_STROBES];
+	struct gb_timesync_frame_time_data ktime_data;
+
+	/* The SVC FrameTime and relative AP FrameTime @ last TIMESYNC_PING */
+	u64 svc_ping_frame_time;
+	u64 ap_ping_frame_time;
+
+	/* Transitory settings */
+	u32 strobe_mask;
+	bool offset_down;
+	bool print_ping;
+	bool capture_ping;
+	int strobe;
+
+	/* Current state */
+	int state;
+};
+
+struct gb_timesync_host_device {
+	struct list_head list;
+	struct gb_host_device *hd;
+	u64 ping_frame_time;
+};
+
+struct gb_timesync_interface {
+	struct list_head list;
+	struct gb_interface *interface;
+	u64 ping_frame_time;
+};
+
+enum gb_timesync_state {
+	GB_TIMESYNC_STATE_INVALID		= 0,
+	GB_TIMESYNC_STATE_INACTIVE		= 1,
+	GB_TIMESYNC_STATE_INIT			= 2,
+	GB_TIMESYNC_STATE_WAIT_SVC		= 3,
+	GB_TIMESYNC_STATE_AUTHORITATIVE		= 4,
+	GB_TIMESYNC_STATE_PING			= 5,
+	GB_TIMESYNC_STATE_ACTIVE		= 6,
+};
+
+static void gb_timesync_ktime_timer_fn(unsigned long data);
+
+static u64 gb_timesync_adjust_count(struct gb_timesync_svc *timesync_svc,
+				    u64 counts)
+{
+	if (timesync_svc->offset_down)
+		return counts - timesync_svc->frame_time_offset;
+	else
+		return counts + timesync_svc->frame_time_offset;
+}
+
+/*
+ * This function provides the authoritative FrameTime to a calling function. It
+ * is designed to be lockless and should remain that way the caller is assumed
+ * to be state-aware.
+ */
+static u64 __gb_timesync_get_frame_time(struct gb_timesync_svc *timesync_svc)
+{
+	u64 clocks = gb_timesync_platform_get_counter();
+
+	return gb_timesync_adjust_count(timesync_svc, clocks);
+}
+
+static void gb_timesync_schedule_svc_timeout(struct gb_timesync_svc
+					     *timesync_svc)
+{
+	queue_delayed_work(timesync_svc->work_queue,
+			   &timesync_svc->delayed_work,
+			   GB_TIMESYNC_MAX_WAIT_SVC);
+}
+
+static void gb_timesync_set_state(struct gb_timesync_svc *timesync_svc,
+				  int state)
+{
+	switch (state) {
+	case GB_TIMESYNC_STATE_INVALID:
+		timesync_svc->state = state;
+		wake_up(&timesync_svc->wait_queue);
+		break;
+	case GB_TIMESYNC_STATE_INACTIVE:
+		timesync_svc->state = state;
+		wake_up(&timesync_svc->wait_queue);
+		break;
+	case GB_TIMESYNC_STATE_INIT:
+		if (timesync_svc->state != GB_TIMESYNC_STATE_INVALID) {
+			timesync_svc->strobe = 0;
+			timesync_svc->frame_time_offset = 0;
+			timesync_svc->state = state;
+			cancel_delayed_work(&timesync_svc->delayed_work);
+			queue_delayed_work(timesync_svc->work_queue,
+					   &timesync_svc->delayed_work,
+					   GB_TIMESYNC_DELAYED_WORK_LONG);
+		}
+		break;
+	case GB_TIMESYNC_STATE_WAIT_SVC:
+		if (timesync_svc->state == GB_TIMESYNC_STATE_INIT)
+			timesync_svc->state = state;
+		break;
+	case GB_TIMESYNC_STATE_AUTHORITATIVE:
+		if (timesync_svc->state == GB_TIMESYNC_STATE_WAIT_SVC) {
+			timesync_svc->state = state;
+			cancel_delayed_work(&timesync_svc->delayed_work);
+			queue_delayed_work(timesync_svc->work_queue,
+					   &timesync_svc->delayed_work, 0);
+		}
+		break;
+	case GB_TIMESYNC_STATE_PING:
+		if (timesync_svc->state == GB_TIMESYNC_STATE_ACTIVE) {
+			timesync_svc->state = state;
+			queue_delayed_work(timesync_svc->work_queue,
+					   &timesync_svc->delayed_work,
+					   GB_TIMESYNC_DELAYED_WORK_SHORT);
+		}
+		break;
+	case GB_TIMESYNC_STATE_ACTIVE:
+		if (timesync_svc->state == GB_TIMESYNC_STATE_AUTHORITATIVE ||
+		    timesync_svc->state == GB_TIMESYNC_STATE_PING) {
+			timesync_svc->state = state;
+			wake_up(&timesync_svc->wait_queue);
+		}
+		break;
+	}
+
+	if (WARN_ON(timesync_svc->state != state)) {
+		pr_err("Invalid state transition %d=>%d\n",
+		       timesync_svc->state, state);
+	}
+}
+
+static void gb_timesync_set_state_atomic(struct gb_timesync_svc *timesync_svc,
+					 int state)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&timesync_svc->spinlock, flags);
+	gb_timesync_set_state(timesync_svc, state);
+	spin_unlock_irqrestore(&timesync_svc->spinlock, flags);
+}
+
+static u64 gb_timesync_diff(u64 x, u64 y)
+{
+	if (x > y)
+		return x - y;
+	else
+		return y - x;
+}
+
+static void gb_timesync_adjust_to_svc(struct gb_timesync_svc *svc,
+				      u64 svc_frame_time, u64 ap_frame_time)
+{
+	if (svc_frame_time > ap_frame_time) {
+		svc->frame_time_offset = svc_frame_time - ap_frame_time;
+		svc->offset_down = false;
+	} else {
+		svc->frame_time_offset = ap_frame_time - svc_frame_time;
+		svc->offset_down = true;
+	}
+}
+
+/*
+ * Associate a FrameTime with a ktime timestamp represented as struct timespec
+ * Requires the calling context to hold timesync_svc->mutex
+ */
+static void gb_timesync_store_ktime(struct gb_timesync_svc *timesync_svc,
+				    struct timespec ts, u64 frame_time)
+{
+	timesync_svc->ktime_data.ts = ts;
+	timesync_svc->ktime_data.frame_time = frame_time;
+}
+
+/*
+ * Find the two pulses that best-match our expected inter-strobe gap and
+ * then calculate the difference between the SVC time at the second pulse
+ * to the local time at the second pulse.
+ */
+static void gb_timesync_collate_frame_time(struct gb_timesync_svc *timesync_svc,
+					   u64 *frame_time)
+{
+	int i = 0;
+	u64 delta, ap_frame_time;
+	u64 strobe_delay_ns = GB_TIMESYNC_STROBE_DELAY_US * NSEC_PER_USEC;
+	u64 least = 0;
+
+	for (i = 1; i < GB_TIMESYNC_MAX_STROBES; i++) {
+		delta = timesync_svc->strobe_data[i].frame_time -
+			timesync_svc->strobe_data[i - 1].frame_time;
+		delta *= gb_timesync_ns_per_clock;
+		delta = gb_timesync_diff(delta, strobe_delay_ns);
+
+		if (!least || delta < least) {
+			least = delta;
+			gb_timesync_adjust_to_svc(timesync_svc, frame_time[i],
+						  timesync_svc->strobe_data[i].frame_time);
+
+			ap_frame_time = timesync_svc->strobe_data[i].frame_time;
+			ap_frame_time = gb_timesync_adjust_count(timesync_svc,
+								 ap_frame_time);
+			gb_timesync_store_ktime(timesync_svc,
+						timesync_svc->strobe_data[i].ts,
+						ap_frame_time);
+
+			pr_debug("adjust %s local %llu svc %llu delta %llu\n",
+				 timesync_svc->offset_down ? "down" : "up",
+				 timesync_svc->strobe_data[i].frame_time,
+				 frame_time[i], delta);
+		}
+	}
+}
+
+static void gb_timesync_teardown(struct gb_timesync_svc *timesync_svc)
+{
+	struct gb_timesync_interface *timesync_interface;
+	struct gb_svc *svc = timesync_svc->svc;
+	struct gb_interface *interface;
+	struct gb_host_device *hd;
+	int ret;
+
+	list_for_each_entry(timesync_interface,
+			    &timesync_svc->interface_list, list) {
+		interface = timesync_interface->interface;
+		ret = gb_interface_timesync_disable(interface);
+		if (ret) {
+			dev_err(&interface->dev,
+				"interface timesync_disable %d\n", ret);
+		}
+	}
+
+	hd = timesync_svc->timesync_hd->hd;
+	ret = hd->driver->timesync_disable(hd);
+	if (ret < 0) {
+		dev_err(&hd->dev, "host timesync_disable %d\n",
+			ret);
+	}
+
+	gb_svc_timesync_wake_pins_release(svc);
+	gb_svc_timesync_disable(svc);
+	gb_timesync_platform_unlock_bus();
+
+	gb_timesync_set_state_atomic(timesync_svc, GB_TIMESYNC_STATE_INACTIVE);
+}
+
+static void gb_timesync_platform_lock_bus_fail(struct gb_timesync_svc
+						*timesync_svc, int ret)
+{
+	if (ret == -EAGAIN) {
+		gb_timesync_set_state(timesync_svc, timesync_svc->state);
+	} else {
+		pr_err("Failed to lock timesync bus %d\n", ret);
+		gb_timesync_set_state(timesync_svc, GB_TIMESYNC_STATE_INACTIVE);
+	}
+}
+
+static void gb_timesync_enable(struct gb_timesync_svc *timesync_svc)
+{
+	struct gb_svc *svc = timesync_svc->svc;
+	struct gb_host_device *hd;
+	struct gb_timesync_interface *timesync_interface;
+	struct gb_interface *interface;
+	u64 init_frame_time;
+	unsigned long clock_rate = gb_timesync_clock_rate;
+	int ret;
+
+	/*
+	 * Get access to the wake pins in the AP and SVC
+	 * Release these pins either in gb_timesync_teardown() or in
+	 * gb_timesync_authoritative()
+	 */
+	ret = gb_timesync_platform_lock_bus(timesync_svc);
+	if (ret < 0) {
+		gb_timesync_platform_lock_bus_fail(timesync_svc, ret);
+		return;
+	}
+	ret = gb_svc_timesync_wake_pins_acquire(svc, timesync_svc->strobe_mask);
+	if (ret) {
+		dev_err(&svc->dev,
+			"gb_svc_timesync_wake_pins_acquire %d\n", ret);
+		gb_timesync_teardown(timesync_svc);
+		return;
+	}
+
+	/* Choose an initial time in the future */
+	init_frame_time = __gb_timesync_get_frame_time(timesync_svc) + 100000UL;
+
+	/* Send enable command to all relevant participants */
+	list_for_each_entry(timesync_interface, &timesync_svc->interface_list,
+			    list) {
+		interface = timesync_interface->interface;
+		ret = gb_interface_timesync_enable(interface,
+						   GB_TIMESYNC_MAX_STROBES,
+						   init_frame_time,
+						   GB_TIMESYNC_STROBE_DELAY_US,
+						   clock_rate);
+		if (ret) {
+			dev_err(&interface->dev,
+				"interface timesync_enable %d\n", ret);
+		}
+	}
+
+	hd = timesync_svc->timesync_hd->hd;
+	ret = hd->driver->timesync_enable(hd, GB_TIMESYNC_MAX_STROBES,
+					  init_frame_time,
+					  GB_TIMESYNC_STROBE_DELAY_US,
+					  clock_rate);
+	if (ret < 0) {
+		dev_err(&hd->dev, "host timesync_enable %d\n",
+			ret);
+	}
+
+	gb_timesync_set_state_atomic(timesync_svc, GB_TIMESYNC_STATE_WAIT_SVC);
+	ret = gb_svc_timesync_enable(svc, GB_TIMESYNC_MAX_STROBES,
+				     init_frame_time,
+				     GB_TIMESYNC_STROBE_DELAY_US,
+				     clock_rate);
+	if (ret) {
+		dev_err(&svc->dev,
+			"gb_svc_timesync_enable %d\n", ret);
+		gb_timesync_teardown(timesync_svc);
+		return;
+	}
+
+	/* Schedule a timeout waiting for SVC to complete strobing */
+	gb_timesync_schedule_svc_timeout(timesync_svc);
+}
+
+static void gb_timesync_authoritative(struct gb_timesync_svc *timesync_svc)
+{
+	struct gb_svc *svc = timesync_svc->svc;
+	struct gb_host_device *hd;
+	struct gb_timesync_interface *timesync_interface;
+	struct gb_interface *interface;
+	u64 svc_frame_time[GB_TIMESYNC_MAX_STROBES];
+	int ret;
+
+	/* Get authoritative time from SVC and adjust local clock */
+	ret = gb_svc_timesync_authoritative(svc, svc_frame_time);
+	if (ret) {
+		dev_err(&svc->dev,
+			"gb_svc_timesync_authoritative %d\n", ret);
+		gb_timesync_teardown(timesync_svc);
+		return;
+	}
+	gb_timesync_collate_frame_time(timesync_svc, svc_frame_time);
+
+	/* Transmit authoritative time to downstream slaves */
+	hd = timesync_svc->timesync_hd->hd;
+	ret = hd->driver->timesync_authoritative(hd, svc_frame_time);
+	if (ret < 0)
+		dev_err(&hd->dev, "host timesync_authoritative %d\n", ret);
+
+	list_for_each_entry(timesync_interface,
+			    &timesync_svc->interface_list, list) {
+		interface = timesync_interface->interface;
+		ret = gb_interface_timesync_authoritative(
+						interface,
+						svc_frame_time);
+		if (ret) {
+			dev_err(&interface->dev,
+				"interface timesync_authoritative %d\n", ret);
+		}
+	}
+
+	/* Release wake pins */
+	gb_svc_timesync_wake_pins_release(svc);
+	gb_timesync_platform_unlock_bus();
+
+	/* Transition to state ACTIVE */
+	gb_timesync_set_state_atomic(timesync_svc, GB_TIMESYNC_STATE_ACTIVE);
+
+	/* Schedule a ping to verify the synchronized system time */
+	timesync_svc->print_ping = true;
+	gb_timesync_set_state_atomic(timesync_svc, GB_TIMESYNC_STATE_PING);
+}
+
+static int __gb_timesync_get_status(struct gb_timesync_svc *timesync_svc)
+{
+	int ret = -EINVAL;
+
+	switch (timesync_svc->state) {
+	case GB_TIMESYNC_STATE_INVALID:
+	case GB_TIMESYNC_STATE_INACTIVE:
+		ret = -ENODEV;
+		break;
+	case GB_TIMESYNC_STATE_INIT:
+	case GB_TIMESYNC_STATE_WAIT_SVC:
+	case GB_TIMESYNC_STATE_AUTHORITATIVE:
+		ret = -EAGAIN;
+		break;
+	case GB_TIMESYNC_STATE_PING:
+	case GB_TIMESYNC_STATE_ACTIVE:
+		ret = 0;
+		break;
+	}
+	return ret;
+}
+
+/*
+ * This routine takes a FrameTime and derives the difference with-respect
+ * to a reference FrameTime/ktime pair. It then returns the calculated
+ * ktime based on the difference between the supplied FrameTime and
+ * the reference FrameTime.
+ *
+ * The time difference is calculated to six decimal places. Taking 19.2MHz
+ * as an example this means we have 52.083333~ nanoseconds per clock or
+ * 52083333~ femtoseconds per clock.
+ *
+ * Naively taking the count difference and converting to
+ * seconds/nanoseconds would quickly see the 0.0833 component produce
+ * noticeable errors. For example a time difference of one second would
+ * loose 19200000 * 0.08333x nanoseconds or 1.59 seconds.
+ *
+ * In contrast calculating in femtoseconds the same example of 19200000 *
+ * 0.000000083333x nanoseconds per count of error is just 1.59 nanoseconds!
+ *
+ * Continuing the example of 19.2 MHz we cap the maximum error difference
+ * at a worst-case 0.3 microseconds over a potential calculation window of
+ * abount 15 seconds, meaning you can convert a FrameTime that is <= 15
+ * seconds older/younger than the reference time with a maximum error of
+ * 0.2385 useconds. Note 19.2MHz is an example frequency not a requirement.
+ */
+static int gb_timesync_to_timespec(struct gb_timesync_svc *timesync_svc,
+				   u64 frame_time, struct timespec *ts)
+{
+	unsigned long flags;
+	u64 delta_fs, counts, sec, nsec;
+	bool add;
+	int ret = 0;
+
+	memset(ts, 0x00, sizeof(*ts));
+	mutex_lock(&timesync_svc->mutex);
+	spin_lock_irqsave(&timesync_svc->spinlock, flags);
+
+	ret = __gb_timesync_get_status(timesync_svc);
+	if (ret)
+		goto done;
+
+	/* Support calculating ktime upwards or downwards from the reference */
+	if (frame_time < timesync_svc->ktime_data.frame_time) {
+		add = false;
+		counts = timesync_svc->ktime_data.frame_time - frame_time;
+	} else {
+		add = true;
+		counts = frame_time - timesync_svc->ktime_data.frame_time;
+	}
+
+	/* Enforce the .23 of a usecond boundary @ 19.2MHz */
+	if (counts > gb_timesync_max_ktime_diff) {
+		ret = -EINVAL;
+		goto done;
+	}
+
+	/* Determine the time difference in femtoseconds */
+	delta_fs = counts * gb_timesync_fs_per_clock;
+
+	/* Convert to seconds */
+	sec = delta_fs;
+	do_div(sec, NSEC_PER_SEC);
+	do_div(sec, 1000000UL);
+
+	/* Get the nanosecond remainder */
+	nsec = do_div(delta_fs, sec);
+	do_div(nsec, 1000000UL);
+
+	if (add) {
+		/* Add the calculated offset - overflow nanoseconds upwards */
+		ts->tv_sec = timesync_svc->ktime_data.ts.tv_sec + sec;
+		ts->tv_nsec = timesync_svc->ktime_data.ts.tv_nsec + nsec;
+		if (ts->tv_nsec >= NSEC_PER_SEC) {
+			ts->tv_sec++;
+			ts->tv_nsec -= NSEC_PER_SEC;
+		}
+	} else {
+		/* Subtract the difference over/underflow as necessary */
+		if (nsec > timesync_svc->ktime_data.ts.tv_nsec) {
+			sec++;
+			nsec = nsec + timesync_svc->ktime_data.ts.tv_nsec;
+			nsec = do_div(nsec, NSEC_PER_SEC);
+		} else {
+			nsec = timesync_svc->ktime_data.ts.tv_nsec - nsec;
+		}
+		/* Cannot return a negative second value */
+		if (sec > timesync_svc->ktime_data.ts.tv_sec) {
+			ret = -EINVAL;
+			goto done;
+		}
+		ts->tv_sec = timesync_svc->ktime_data.ts.tv_sec - sec;
+		ts->tv_nsec = nsec;
+	}
+done:
+	spin_unlock_irqrestore(&timesync_svc->spinlock, flags);
+	mutex_unlock(&timesync_svc->mutex);
+	return ret;
+}
+
+static size_t gb_timesync_log_frame_time(struct gb_timesync_svc *timesync_svc,
+					 char *buf, size_t buflen)
+{
+	struct gb_svc *svc = timesync_svc->svc;
+	struct gb_host_device *hd;
+	struct gb_timesync_interface *timesync_interface;
+	struct gb_interface *interface;
+	unsigned int len;
+	size_t off;
+
+	/* AP/SVC */
+	off = snprintf(buf, buflen, "%s frametime: ap=%llu %s=%llu ",
+		       greybus_bus_type.name,
+		       timesync_svc->ap_ping_frame_time, dev_name(&svc->dev),
+		       timesync_svc->svc_ping_frame_time);
+	len = buflen - off;
+
+	/* APB/GPB */
+	if (len < buflen) {
+		hd = timesync_svc->timesync_hd->hd;
+		off += snprintf(&buf[off], len, "%s=%llu ", dev_name(&hd->dev),
+				timesync_svc->timesync_hd->ping_frame_time);
+		len = buflen - off;
+	}
+
+	list_for_each_entry(timesync_interface,
+			    &timesync_svc->interface_list, list) {
+		if (len < buflen) {
+			interface = timesync_interface->interface;
+			off += snprintf(&buf[off], len, "%s=%llu ",
+					dev_name(&interface->dev),
+					timesync_interface->ping_frame_time);
+			len = buflen - off;
+		}
+	}
+	if (len < buflen)
+		off += snprintf(&buf[off], len, "\n");
+	return off;
+}
+
+static size_t gb_timesync_log_frame_ktime(struct gb_timesync_svc *timesync_svc,
+					  char *buf, size_t buflen)
+{
+	struct gb_svc *svc = timesync_svc->svc;
+	struct gb_host_device *hd;
+	struct gb_timesync_interface *timesync_interface;
+	struct gb_interface *interface;
+	struct timespec ts;
+	unsigned int len;
+	size_t off;
+
+	/* AP */
+	gb_timesync_to_timespec(timesync_svc, timesync_svc->ap_ping_frame_time,
+				&ts);
+	off = snprintf(buf, buflen, "%s frametime: ap=%lu.%lu ",
+		       greybus_bus_type.name, ts.tv_sec, ts.tv_nsec);
+	len = buflen - off;
+	if (len >= buflen)
+		goto done;
+
+	/* SVC */
+	gb_timesync_to_timespec(timesync_svc, timesync_svc->svc_ping_frame_time,
+				&ts);
+	off += snprintf(&buf[off], len, "%s=%lu.%lu ", dev_name(&svc->dev),
+			ts.tv_sec, ts.tv_nsec);
+	len = buflen - off;
+	if (len >= buflen)
+		goto done;
+
+	/* APB/GPB */
+	hd = timesync_svc->timesync_hd->hd;
+	gb_timesync_to_timespec(timesync_svc,
+				timesync_svc->timesync_hd->ping_frame_time,
+				&ts);
+	off += snprintf(&buf[off], len, "%s=%lu.%lu ",
+			dev_name(&hd->dev),
+			ts.tv_sec, ts.tv_nsec);
+	len = buflen - off;
+	if (len >= buflen)
+		goto done;
+
+	list_for_each_entry(timesync_interface,
+			    &timesync_svc->interface_list, list) {
+		interface = timesync_interface->interface;
+		gb_timesync_to_timespec(timesync_svc,
+					timesync_interface->ping_frame_time,
+					&ts);
+		off += snprintf(&buf[off], len, "%s=%lu.%lu ",
+				dev_name(&interface->dev),
+				ts.tv_sec, ts.tv_nsec);
+		len = buflen - off;
+		if (len >= buflen)
+			goto done;
+	}
+	off += snprintf(&buf[off], len, "\n");
+done:
+	return off;
+}
+
+/*
+ * Send an SVC initiated wake 'ping' to each TimeSync participant.
+ * Get the FrameTime from each participant associated with the wake
+ * ping.
+ */
+static void gb_timesync_ping(struct gb_timesync_svc *timesync_svc)
+{
+	struct gb_svc *svc = timesync_svc->svc;
+	struct gb_host_device *hd;
+	struct gb_timesync_interface *timesync_interface;
+	struct gb_control *control;
+	u64 *ping_frame_time;
+	int ret;
+
+	/* Get access to the wake pins in the AP and SVC */
+	ret = gb_timesync_platform_lock_bus(timesync_svc);
+	if (ret < 0) {
+		gb_timesync_platform_lock_bus_fail(timesync_svc, ret);
+		return;
+	}
+	ret = gb_svc_timesync_wake_pins_acquire(svc, timesync_svc->strobe_mask);
+	if (ret) {
+		dev_err(&svc->dev,
+			"gb_svc_timesync_wake_pins_acquire %d\n", ret);
+		gb_timesync_teardown(timesync_svc);
+		return;
+	}
+
+	/* Have SVC generate a timesync ping */
+	timesync_svc->capture_ping = true;
+	timesync_svc->svc_ping_frame_time = 0;
+	ret = gb_svc_timesync_ping(svc, &timesync_svc->svc_ping_frame_time);
+	timesync_svc->capture_ping = false;
+	if (ret) {
+		dev_err(&svc->dev,
+			"gb_svc_timesync_ping %d\n", ret);
+		gb_timesync_teardown(timesync_svc);
+		return;
+	}
+
+	/* Get the ping FrameTime from each APB/GPB */
+	hd = timesync_svc->timesync_hd->hd;
+	timesync_svc->timesync_hd->ping_frame_time = 0;
+	ret = hd->driver->timesync_get_last_event(hd,
+		&timesync_svc->timesync_hd->ping_frame_time);
+	if (ret)
+		dev_err(&hd->dev, "host timesync_get_last_event %d\n", ret);
+
+	list_for_each_entry(timesync_interface,
+			    &timesync_svc->interface_list, list) {
+		control = timesync_interface->interface->control;
+		timesync_interface->ping_frame_time = 0;
+		ping_frame_time = &timesync_interface->ping_frame_time;
+		ret = gb_control_timesync_get_last_event(control,
+							 ping_frame_time);
+		if (ret) {
+			dev_err(&timesync_interface->interface->dev,
+				"gb_control_timesync_get_last_event %d\n", ret);
+		}
+	}
+
+	/* Ping success - move to timesync active */
+	gb_svc_timesync_wake_pins_release(svc);
+	gb_timesync_platform_unlock_bus();
+	gb_timesync_set_state_atomic(timesync_svc, GB_TIMESYNC_STATE_ACTIVE);
+}
+
+static void gb_timesync_log_ping_time(struct gb_timesync_svc *timesync_svc)
+{
+	char *buf;
+
+	if (!timesync_svc->print_ping)
+		return;
+
+	buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (buf) {
+		gb_timesync_log_frame_time(timesync_svc, buf, PAGE_SIZE);
+		dev_dbg(&timesync_svc->svc->dev, "%s", buf);
+		kfree(buf);
+	}
+}
+
+/*
+ * Perform the actual work of scheduled TimeSync logic.
+ */
+static void gb_timesync_worker(struct work_struct *work)
+{
+	struct delayed_work *delayed_work = to_delayed_work(work);
+	struct gb_timesync_svc *timesync_svc =
+		container_of(delayed_work, struct gb_timesync_svc, delayed_work);
+
+	mutex_lock(&timesync_svc->mutex);
+
+	switch (timesync_svc->state) {
+	case GB_TIMESYNC_STATE_INIT:
+		gb_timesync_enable(timesync_svc);
+		break;
+
+	case GB_TIMESYNC_STATE_WAIT_SVC:
+		dev_err(&timesync_svc->svc->dev,
+			"timeout SVC strobe completion %d/%d\n",
+			timesync_svc->strobe, GB_TIMESYNC_MAX_STROBES);
+		gb_timesync_teardown(timesync_svc);
+		break;
+
+	case GB_TIMESYNC_STATE_AUTHORITATIVE:
+		gb_timesync_authoritative(timesync_svc);
+		break;
+
+	case GB_TIMESYNC_STATE_PING:
+		gb_timesync_ping(timesync_svc);
+		gb_timesync_log_ping_time(timesync_svc);
+		break;
+
+	default:
+		pr_err("Invalid state %d for delayed work\n",
+		       timesync_svc->state);
+		break;
+	}
+
+	mutex_unlock(&timesync_svc->mutex);
+}
+
+/*
+ * Schedule a new TimeSync INIT or PING operation serialized w/r to
+ * gb_timesync_worker().
+ */
+static int gb_timesync_schedule(struct gb_timesync_svc *timesync_svc, int state)
+{
+	int ret = 0;
+
+	if (state != GB_TIMESYNC_STATE_INIT && state != GB_TIMESYNC_STATE_PING)
+		return -EINVAL;
+
+	mutex_lock(&timesync_svc->mutex);
+	if (timesync_svc->state !=  GB_TIMESYNC_STATE_INVALID) {
+		gb_timesync_set_state_atomic(timesync_svc, state);
+	} else {
+		ret = -ENODEV;
+	}
+	mutex_unlock(&timesync_svc->mutex);
+	return ret;
+}
+
+static int __gb_timesync_schedule_synchronous(
+	struct gb_timesync_svc *timesync_svc, int state)
+{
+	unsigned long flags;
+	int ret;
+
+	ret = gb_timesync_schedule(timesync_svc, state);
+	if (ret)
+		return ret;
+
+	ret = wait_event_interruptible(timesync_svc->wait_queue,
+			(timesync_svc->state == GB_TIMESYNC_STATE_ACTIVE ||
+			 timesync_svc->state == GB_TIMESYNC_STATE_INACTIVE ||
+			 timesync_svc->state == GB_TIMESYNC_STATE_INVALID));
+	if (ret)
+		return ret;
+
+	mutex_lock(&timesync_svc->mutex);
+	spin_lock_irqsave(&timesync_svc->spinlock, flags);
+
+	ret = __gb_timesync_get_status(timesync_svc);
+
+	spin_unlock_irqrestore(&timesync_svc->spinlock, flags);
+	mutex_unlock(&timesync_svc->mutex);
+
+	return ret;
+}
+
+static struct gb_timesync_svc *gb_timesync_find_timesync_svc(
+	struct gb_host_device *hd)
+{
+	struct gb_timesync_svc *timesync_svc;
+
+	list_for_each_entry(timesync_svc, &gb_timesync_svc_list, list) {
+		if (timesync_svc->svc == hd->svc)
+			return timesync_svc;
+	}
+	return NULL;
+}
+
+static struct gb_timesync_interface *gb_timesync_find_timesync_interface(
+	struct gb_timesync_svc *timesync_svc,
+	struct gb_interface *interface)
+{
+	struct gb_timesync_interface *timesync_interface;
+
+	list_for_each_entry(timesync_interface, &timesync_svc->interface_list, list) {
+		if (timesync_interface->interface == interface)
+			return timesync_interface;
+	}
+	return NULL;
+}
+
+int gb_timesync_schedule_synchronous(struct gb_interface *interface)
+{
+	int ret;
+	struct gb_timesync_svc *timesync_svc;
+	int retries;
+
+	if (!(interface->features & GREYBUS_INTERFACE_FEATURE_TIMESYNC))
+		return 0;
+
+	mutex_lock(&gb_timesync_svc_list_mutex);
+	for (retries = 0; retries < GB_TIMESYNC_MAX_RETRIES; retries++) {
+		timesync_svc = gb_timesync_find_timesync_svc(interface->hd);
+		if (!timesync_svc) {
+			ret = -ENODEV;
+			goto done;
+		}
+
+		ret = __gb_timesync_schedule_synchronous(timesync_svc,
+						 GB_TIMESYNC_STATE_INIT);
+		if (!ret)
+			break;
+	}
+	if (ret && retries == GB_TIMESYNC_MAX_RETRIES)
+		ret = -ETIMEDOUT;
+done:
+	mutex_unlock(&gb_timesync_svc_list_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gb_timesync_schedule_synchronous);
+
+void gb_timesync_schedule_asynchronous(struct gb_interface *interface)
+{
+	struct gb_timesync_svc *timesync_svc;
+
+	if (!(interface->features & GREYBUS_INTERFACE_FEATURE_TIMESYNC))
+		return;
+
+	mutex_lock(&gb_timesync_svc_list_mutex);
+	timesync_svc = gb_timesync_find_timesync_svc(interface->hd);
+	if (!timesync_svc)
+		goto done;
+
+	gb_timesync_schedule(timesync_svc, GB_TIMESYNC_STATE_INIT);
+done:
+	mutex_unlock(&gb_timesync_svc_list_mutex);
+	return;
+}
+EXPORT_SYMBOL_GPL(gb_timesync_schedule_asynchronous);
+
+static ssize_t gb_timesync_ping_read(struct file *file, char __user *ubuf,
+				     size_t len, loff_t *offset, bool ktime)
+{
+	struct gb_timesync_svc *timesync_svc = file->f_inode->i_private;
+	char *buf;
+	ssize_t ret = 0;
+
+	mutex_lock(&gb_timesync_svc_list_mutex);
+	mutex_lock(&timesync_svc->mutex);
+	if (list_empty(&timesync_svc->interface_list))
+		ret = -ENODEV;
+	timesync_svc->print_ping = false;
+	mutex_unlock(&timesync_svc->mutex);
+	if (ret)
+		goto done;
+
+	ret = __gb_timesync_schedule_synchronous(timesync_svc,
+						 GB_TIMESYNC_STATE_PING);
+	if (ret)
+		goto done;
+
+	buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto done;
+	}
+
+	if (ktime)
+		ret = gb_timesync_log_frame_ktime(timesync_svc, buf, PAGE_SIZE);
+	else
+		ret = gb_timesync_log_frame_time(timesync_svc, buf, PAGE_SIZE);
+	if (ret > 0)
+		ret = simple_read_from_buffer(ubuf, len, offset, buf, ret);
+	kfree(buf);
+done:
+	mutex_unlock(&gb_timesync_svc_list_mutex);
+	return ret;
+}
+
+static ssize_t gb_timesync_ping_read_frame_time(struct file *file,
+						char __user *buf,
+						size_t len, loff_t *offset)
+{
+	return gb_timesync_ping_read(file, buf, len, offset, false);
+}
+
+static ssize_t gb_timesync_ping_read_frame_ktime(struct file *file,
+						 char __user *buf,
+						 size_t len, loff_t *offset)
+{
+	return gb_timesync_ping_read(file, buf, len, offset, true);
+}
+
+static const struct file_operations gb_timesync_debugfs_frame_time_ops = {
+	.read		= gb_timesync_ping_read_frame_time,
+};
+
+static const struct file_operations gb_timesync_debugfs_frame_ktime_ops = {
+	.read		= gb_timesync_ping_read_frame_ktime,
+};
+
+static int gb_timesync_hd_add(struct gb_timesync_svc *timesync_svc,
+			      struct gb_host_device *hd)
+{
+	struct gb_timesync_host_device *timesync_hd;
+
+	timesync_hd = kzalloc(sizeof(*timesync_hd), GFP_KERNEL);
+	if (!timesync_hd)
+		return -ENOMEM;
+
+	WARN_ON(timesync_svc->timesync_hd);
+	timesync_hd->hd = hd;
+	timesync_svc->timesync_hd = timesync_hd;
+
+	return 0;
+}
+
+static void gb_timesync_hd_remove(struct gb_timesync_svc *timesync_svc,
+				  struct gb_host_device *hd)
+{
+	if (timesync_svc->timesync_hd->hd == hd) {
+		kfree(timesync_svc->timesync_hd);
+		timesync_svc->timesync_hd = NULL;
+		return;
+	}
+	WARN_ON(1);
+}
+
+int gb_timesync_svc_add(struct gb_svc *svc)
+{
+	struct gb_timesync_svc *timesync_svc;
+	int ret;
+
+	timesync_svc = kzalloc(sizeof(*timesync_svc), GFP_KERNEL);
+	if (!timesync_svc)
+		return -ENOMEM;
+
+	timesync_svc->work_queue =
+		create_singlethread_workqueue("gb-timesync-work_queue");
+
+	if (!timesync_svc->work_queue) {
+		kfree(timesync_svc);
+		return -ENOMEM;
+	}
+
+	mutex_lock(&gb_timesync_svc_list_mutex);
+	INIT_LIST_HEAD(&timesync_svc->interface_list);
+	INIT_DELAYED_WORK(&timesync_svc->delayed_work, gb_timesync_worker);
+	mutex_init(&timesync_svc->mutex);
+	spin_lock_init(&timesync_svc->spinlock);
+	init_waitqueue_head(&timesync_svc->wait_queue);
+
+	timesync_svc->svc = svc;
+	timesync_svc->frame_time_offset = 0;
+	timesync_svc->capture_ping = false;
+	gb_timesync_set_state_atomic(timesync_svc, GB_TIMESYNC_STATE_INACTIVE);
+
+	timesync_svc->frame_time_dentry =
+		debugfs_create_file("frame-time", S_IRUGO, svc->debugfs_dentry,
+				    timesync_svc,
+				    &gb_timesync_debugfs_frame_time_ops);
+	timesync_svc->frame_ktime_dentry =
+		debugfs_create_file("frame-ktime", S_IRUGO, svc->debugfs_dentry,
+				    timesync_svc,
+				    &gb_timesync_debugfs_frame_ktime_ops);
+
+	list_add(&timesync_svc->list, &gb_timesync_svc_list);
+	ret = gb_timesync_hd_add(timesync_svc, svc->hd);
+	if (ret) {
+		list_del(&timesync_svc->list);
+		debugfs_remove(timesync_svc->frame_ktime_dentry);
+		debugfs_remove(timesync_svc->frame_time_dentry);
+		destroy_workqueue(timesync_svc->work_queue);
+		kfree(timesync_svc);
+		goto done;
+	}
+
+	init_timer(&timesync_svc->ktime_timer);
+	timesync_svc->ktime_timer.function = gb_timesync_ktime_timer_fn;
+	timesync_svc->ktime_timer.expires = jiffies + GB_TIMESYNC_KTIME_UPDATE;
+	timesync_svc->ktime_timer.data = (unsigned long)timesync_svc;
+	add_timer(&timesync_svc->ktime_timer);
+done:
+	mutex_unlock(&gb_timesync_svc_list_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gb_timesync_svc_add);
+
+void gb_timesync_svc_remove(struct gb_svc *svc)
+{
+	struct gb_timesync_svc *timesync_svc;
+	struct gb_timesync_interface *timesync_interface;
+	struct gb_timesync_interface *next;
+
+	mutex_lock(&gb_timesync_svc_list_mutex);
+	timesync_svc = gb_timesync_find_timesync_svc(svc->hd);
+	if (!timesync_svc)
+		goto done;
+
+	cancel_delayed_work_sync(&timesync_svc->delayed_work);
+
+	mutex_lock(&timesync_svc->mutex);
+
+	gb_timesync_set_state_atomic(timesync_svc, GB_TIMESYNC_STATE_INVALID);
+	del_timer_sync(&timesync_svc->ktime_timer);
+	gb_timesync_teardown(timesync_svc);
+
+	gb_timesync_hd_remove(timesync_svc, svc->hd);
+	list_for_each_entry_safe(timesync_interface, next,
+				 &timesync_svc->interface_list, list) {
+		list_del(&timesync_interface->list);
+		kfree(timesync_interface);
+	}
+	debugfs_remove(timesync_svc->frame_ktime_dentry);
+	debugfs_remove(timesync_svc->frame_time_dentry);
+	destroy_workqueue(timesync_svc->work_queue);
+	list_del(&timesync_svc->list);
+
+	mutex_unlock(&timesync_svc->mutex);
+
+	kfree(timesync_svc);
+done:
+	mutex_unlock(&gb_timesync_svc_list_mutex);
+}
+EXPORT_SYMBOL_GPL(gb_timesync_svc_remove);
+
+/*
+ * Add a Greybus Interface to the set of TimeSync Interfaces.
+ */
+int gb_timesync_interface_add(struct gb_interface *interface)
+{
+	struct gb_timesync_svc *timesync_svc;
+	struct gb_timesync_interface *timesync_interface;
+	int ret = 0;
+
+	if (!(interface->features & GREYBUS_INTERFACE_FEATURE_TIMESYNC))
+		return 0;
+
+	mutex_lock(&gb_timesync_svc_list_mutex);
+	timesync_svc = gb_timesync_find_timesync_svc(interface->hd);
+	if (!timesync_svc) {
+		ret = -ENODEV;
+		goto done;
+	}
+
+	timesync_interface = kzalloc(sizeof(*timesync_interface), GFP_KERNEL);
+	if (!timesync_interface) {
+		ret = -ENOMEM;
+		goto done;
+	}
+
+	mutex_lock(&timesync_svc->mutex);
+	timesync_interface->interface = interface;
+	list_add(&timesync_interface->list, &timesync_svc->interface_list);
+	timesync_svc->strobe_mask |= 1 << interface->interface_id;
+	mutex_unlock(&timesync_svc->mutex);
+
+done:
+	mutex_unlock(&gb_timesync_svc_list_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gb_timesync_interface_add);
+
+/*
+ * Remove a Greybus Interface from the set of TimeSync Interfaces.
+ */
+void gb_timesync_interface_remove(struct gb_interface *interface)
+{
+	struct gb_timesync_svc *timesync_svc;
+	struct gb_timesync_interface *timesync_interface;
+
+	if (!(interface->features & GREYBUS_INTERFACE_FEATURE_TIMESYNC))
+		return;
+
+	mutex_lock(&gb_timesync_svc_list_mutex);
+	timesync_svc = gb_timesync_find_timesync_svc(interface->hd);
+	if (!timesync_svc)
+		goto done;
+
+	timesync_interface = gb_timesync_find_timesync_interface(timesync_svc,
+								 interface);
+	if (!timesync_interface)
+		goto done;
+
+	mutex_lock(&timesync_svc->mutex);
+	timesync_svc->strobe_mask &= ~(1 << interface->interface_id);
+	list_del(&timesync_interface->list);
+	kfree(timesync_interface);
+	mutex_unlock(&timesync_svc->mutex);
+done:
+	mutex_unlock(&gb_timesync_svc_list_mutex);
+}
+EXPORT_SYMBOL_GPL(gb_timesync_interface_remove);
+
+/*
+ * Give the authoritative FrameTime to the calling function. Returns zero if we
+ * are not in GB_TIMESYNC_STATE_ACTIVE.
+ */
+static u64 gb_timesync_get_frame_time(struct gb_timesync_svc *timesync_svc)
+{
+	unsigned long flags;
+	u64 ret;
+
+	spin_lock_irqsave(&timesync_svc->spinlock, flags);
+	if (timesync_svc->state == GB_TIMESYNC_STATE_ACTIVE)
+		ret = __gb_timesync_get_frame_time(timesync_svc);
+	else
+		ret = 0;
+	spin_unlock_irqrestore(&timesync_svc->spinlock, flags);
+	return ret;
+}
+
+u64 gb_timesync_get_frame_time_by_interface(struct gb_interface *interface)
+{
+	struct gb_timesync_svc *timesync_svc;
+	u64 ret = 0;
+
+	mutex_lock(&gb_timesync_svc_list_mutex);
+	timesync_svc = gb_timesync_find_timesync_svc(interface->hd);
+	if (!timesync_svc)
+		goto done;
+
+	ret = gb_timesync_get_frame_time(timesync_svc);
+done:
+	mutex_unlock(&gb_timesync_svc_list_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gb_timesync_get_frame_time_by_interface);
+
+u64 gb_timesync_get_frame_time_by_svc(struct gb_svc *svc)
+{
+	struct gb_timesync_svc *timesync_svc;
+	u64 ret = 0;
+
+	mutex_lock(&gb_timesync_svc_list_mutex);
+	timesync_svc = gb_timesync_find_timesync_svc(svc->hd);
+	if (!timesync_svc)
+		goto done;
+
+	ret = gb_timesync_get_frame_time(timesync_svc);
+done:
+	mutex_unlock(&gb_timesync_svc_list_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gb_timesync_get_frame_time_by_svc);
+
+/* Incrementally updates the conversion base from FrameTime to ktime */
+static void gb_timesync_ktime_timer_fn(unsigned long data)
+{
+	struct gb_timesync_svc *timesync_svc =
+		(struct gb_timesync_svc *)data;
+	unsigned long flags;
+	u64 frame_time;
+	struct timespec ts;
+
+	spin_lock_irqsave(&timesync_svc->spinlock, flags);
+
+	if (timesync_svc->state != GB_TIMESYNC_STATE_ACTIVE)
+		goto done;
+
+	ktime_get_ts(&ts);
+	frame_time = __gb_timesync_get_frame_time(timesync_svc);
+	gb_timesync_store_ktime(timesync_svc, ts, frame_time);
+
+done:
+	spin_unlock_irqrestore(&timesync_svc->spinlock, flags);
+	mod_timer(&timesync_svc->ktime_timer,
+		  jiffies + GB_TIMESYNC_KTIME_UPDATE);
+}
+
+int gb_timesync_to_timespec_by_svc(struct gb_svc *svc, u64 frame_time,
+				   struct timespec *ts)
+{
+	struct gb_timesync_svc *timesync_svc;
+	int ret = 0;
+
+	mutex_lock(&gb_timesync_svc_list_mutex);
+	timesync_svc = gb_timesync_find_timesync_svc(svc->hd);
+	if (!timesync_svc) {
+		ret = -ENODEV;
+		goto done;
+	}
+	ret = gb_timesync_to_timespec(timesync_svc, frame_time, ts);
+done:
+	mutex_unlock(&gb_timesync_svc_list_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gb_timesync_to_timespec_by_svc);
+
+int gb_timesync_to_timespec_by_interface(struct gb_interface *interface,
+					 u64 frame_time, struct timespec *ts)
+{
+	struct gb_timesync_svc *timesync_svc;
+	int ret = 0;
+
+	mutex_lock(&gb_timesync_svc_list_mutex);
+	timesync_svc = gb_timesync_find_timesync_svc(interface->hd);
+	if (!timesync_svc) {
+		ret = -ENODEV;
+		goto done;
+	}
+
+	ret = gb_timesync_to_timespec(timesync_svc, frame_time, ts);
+done:
+	mutex_unlock(&gb_timesync_svc_list_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gb_timesync_to_timespec_by_interface);
+
+void gb_timesync_irq(struct gb_timesync_svc *timesync_svc)
+{
+	unsigned long flags;
+	u64 strobe_time;
+	bool strobe_is_ping = true;
+	struct timespec ts;
+
+	ktime_get_ts(&ts);
+	strobe_time = __gb_timesync_get_frame_time(timesync_svc);
+
+	spin_lock_irqsave(&timesync_svc->spinlock, flags);
+
+	if (timesync_svc->state == GB_TIMESYNC_STATE_PING) {
+		if (!timesync_svc->capture_ping)
+			goto done_nolog;
+		timesync_svc->ap_ping_frame_time = strobe_time;
+		goto done_log;
+	} else if (timesync_svc->state != GB_TIMESYNC_STATE_WAIT_SVC) {
+		goto done_nolog;
+	}
+
+	timesync_svc->strobe_data[timesync_svc->strobe].frame_time = strobe_time;
+	timesync_svc->strobe_data[timesync_svc->strobe].ts = ts;
+
+	if (++timesync_svc->strobe == GB_TIMESYNC_MAX_STROBES) {
+		gb_timesync_set_state(timesync_svc,
+				      GB_TIMESYNC_STATE_AUTHORITATIVE);
+	}
+	strobe_is_ping = false;
+done_log:
+	trace_gb_timesync_irq(strobe_is_ping, timesync_svc->strobe,
+			      GB_TIMESYNC_MAX_STROBES, strobe_time);
+done_nolog:
+	spin_unlock_irqrestore(&timesync_svc->spinlock, flags);
+}
+EXPORT_SYMBOL(gb_timesync_irq);
+
+int __init gb_timesync_init(void)
+{
+	int ret = 0;
+
+	ret = gb_timesync_platform_init();
+	if (ret) {
+		pr_err("timesync platform init fail!\n");
+		return ret;
+	}
+
+	gb_timesync_clock_rate = gb_timesync_platform_get_clock_rate();
+
+	/* Calculate nanoseconds and femtoseconds per clock */
+	gb_timesync_fs_per_clock = FSEC_PER_SEC;
+	do_div(gb_timesync_fs_per_clock, gb_timesync_clock_rate);
+	gb_timesync_ns_per_clock = NSEC_PER_SEC;
+	do_div(gb_timesync_ns_per_clock, gb_timesync_clock_rate);
+
+	/* Calculate the maximum number of clocks we will convert to ktime */
+	gb_timesync_max_ktime_diff =
+		GB_TIMESYNC_MAX_KTIME_CONVERSION * gb_timesync_clock_rate;
+
+	pr_info("Time-Sync @ %lu Hz max ktime conversion +/- %d seconds\n",
+		gb_timesync_clock_rate, GB_TIMESYNC_MAX_KTIME_CONVERSION);
+	return 0;
+}
+
+void gb_timesync_exit(void)
+{
+	gb_timesync_platform_exit();
+}
--- /dev/null
+++ b/drivers/greybus/timesync.h
@@ -0,0 +1,45 @@ 
+/*
+ * TimeSync API driver.
+ *
+ * Copyright 2016 Google Inc.
+ * Copyright 2016 Linaro Ltd.
+ *
+ * Released under the GPLv2 only.
+ */
+
+#ifndef __TIMESYNC_H
+#define __TIMESYNC_H
+
+struct gb_svc;
+struct gb_interface;
+struct gb_timesync_svc;
+
+/* Platform */
+u64 gb_timesync_platform_get_counter(void);
+u32 gb_timesync_platform_get_clock_rate(void);
+int gb_timesync_platform_lock_bus(struct gb_timesync_svc *pdata);
+void gb_timesync_platform_unlock_bus(void);
+
+int gb_timesync_platform_init(void);
+void gb_timesync_platform_exit(void);
+
+/* Core API */
+int gb_timesync_interface_add(struct gb_interface *interface);
+void gb_timesync_interface_remove(struct gb_interface *interface);
+int gb_timesync_svc_add(struct gb_svc *svc);
+void gb_timesync_svc_remove(struct gb_svc *svc);
+
+u64 gb_timesync_get_frame_time_by_interface(struct gb_interface *interface);
+u64 gb_timesync_get_frame_time_by_svc(struct gb_svc *svc);
+int gb_timesync_to_timespec_by_svc(struct gb_svc *svc, u64 frame_time,
+				   struct timespec *ts);
+int gb_timesync_to_timespec_by_interface(struct gb_interface *interface,
+					 u64 frame_time, struct timespec *ts);
+
+int gb_timesync_schedule_synchronous(struct gb_interface *intf);
+void gb_timesync_schedule_asynchronous(struct gb_interface *intf);
+void gb_timesync_irq(struct gb_timesync_svc *timesync_svc);
+int gb_timesync_init(void);
+void gb_timesync_exit(void);
+
+#endif /* __TIMESYNC_H */
--- /dev/null
+++ b/drivers/greybus/timesync_platform.c
@@ -0,0 +1,77 @@ 
+/*
+ * TimeSync API driver.
+ *
+ * Copyright 2016 Google Inc.
+ * Copyright 2016 Linaro Ltd.
+ *
+ * Released under the GPLv2 only.
+ *
+ * This code reads directly from an ARMv7 memory-mapped timer that lives in
+ * MMIO space. Since this counter lives inside of MMIO space its shared between
+ * cores and that means we don't have to worry about issues like TSC on x86
+ * where each time-stamp-counter (TSC) is local to a particular core.
+ *
+ * Register-level access code is based on
+ * drivers/clocksource/arm_arch_timer.c
+ */
+#include <linux/cpufreq.h>
+#include <linux/of_platform.h>
+
+#include "greybus.h"
+#include "arche_platform.h"
+
+static u32 gb_timesync_clock_frequency;
+int (*arche_platform_change_state_cb)(enum arche_platform_state state,
+				      struct gb_timesync_svc *pdata);
+EXPORT_SYMBOL_GPL(arche_platform_change_state_cb);
+
+u64 gb_timesync_platform_get_counter(void)
+{
+	return (u64)get_cycles();
+}
+
+u32 gb_timesync_platform_get_clock_rate(void)
+{
+	if (unlikely(!gb_timesync_clock_frequency))
+		return cpufreq_get(0);
+
+	return gb_timesync_clock_frequency;
+}
+
+int gb_timesync_platform_lock_bus(struct gb_timesync_svc *pdata)
+{
+	return arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_TIME_SYNC,
+					      pdata);
+}
+
+void gb_timesync_platform_unlock_bus(void)
+{
+	arche_platform_change_state_cb(ARCHE_PLATFORM_STATE_ACTIVE, NULL);
+}
+
+static const struct of_device_id arch_timer_of_match[] = {
+	{ .compatible   = "google,greybus-frame-time-counter", },
+	{},
+};
+
+int __init gb_timesync_platform_init(void)
+{
+	struct device_node *np;
+
+	np = of_find_matching_node(NULL, arch_timer_of_match);
+	if (!np) {
+		/* Tolerate not finding to allow BBB etc to continue */
+		pr_warn("Unable to find a compatible ARMv7 timer\n");
+		return 0;
+	}
+
+	if (of_property_read_u32(np, "clock-frequency",
+				 &gb_timesync_clock_frequency)) {
+		pr_err("Unable to find timer clock-frequency\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+void gb_timesync_platform_exit(void) {}