LinuxPPS low-level IRQs timestamps & ldisc

LinuxPPS low-level IRQs timestamps & ldisc

[Rodolfo Giometti]

Hello,

I write to both of you since my questions are related. :)

My last modifications to LinuxPPS are focused on adding a PPS ldisc
and a low-level IRQs timestamps recording.

First I added a new dcd_change() ldisc method which can be used as
follow:

static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status)
{
       int id = (int) tty->disc_data;
       struct timespec __ts;
       struct pps_ktime ts;

       /* First of all we get the time stamp... */
       getnstimeofday(&__ts);

       /* ... and translate it to PPS time data struct */
       ts.sec = __ts.tv_sec;
       ts.nsec = __ts.tv_nsec;

       /* Now do the PPS event report */
       pps_event(id, &ts,
                       status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR, tty);

       pr_debug("[STDev] PPS %s at %lu on source #%d\n",
                       status ? "assert" : "clear", jiffies, id);
}

However this solution gives very low precision timestamps so that's
why I propose to add the following code to register IRQ timestamps as
soon as possible (here the code for x86_32 architecture):

diff --git a/arch/x86/kernel/irq_32.c b/arch/x86/kernel/irq_32.c
index d3fde94..a3b8457 100644
--- a/arch/x86/kernel/irq_32.c
+++ b/arch/x86/kernel/irq_32.c
@@ -15,6 +15,7 @@
 #include <linux/notifier.h>
 #include <linux/cpu.h>
 #include <linux/delay.h>
+#include <linux/pps.h>
 
 #include <asm/apic.h>
 #include <asm/uaccess.h>
@@ -25,6 +26,11 @@ EXPORT_PER_CPU_SYMBOL(irq_stat);
 DEFINE_PER_CPU(struct pt_regs *, irq_regs);
 EXPORT_PER_CPU_SYMBOL(irq_regs);
 
+#ifdef CONFIG_PPS_IRQ_EVENTS
+struct pps_ktime pps_irq_ts[NR_IRQS];
+EXPORT_SYMBOL(pps_irq_ts);
+#endif
+
 /*
  * 'what should we do if we get a hw irq event on an illegal vector'.
  * each architecture has to answer this themselves.
@@ -76,6 +82,12 @@ fastcall unsigned int do_IRQ(struct pt_regs *regs)
        union irq_ctx *curctx, *irqctx;
        u32 *isp;
 #endif
+#ifdef CONFIG_PPS_IRQ_EVENTS
+       struct timespec __ts;
+
+       /* Get IRQ timestamps as soon as possible for the PPS layer */
+       getnstimeofday(&__ts);
+#endif
 
        if (unlikely((unsigned)irq >= NR_IRQS)) {
                printk(KERN_EMERG "%s: cannot handle IRQ %d\n",
@@ -83,6 +95,12 @@ fastcall unsigned int do_IRQ(struct pt_regs *regs)
                BUG();
        }
 
+#ifdef CONFIG_PPS_IRQ_EVENTS
+       /* Then, after sanity check, store the IRQ timestamp */
+       pps_irq_ts[irq].sec = __ts.tv_sec;
+       pps_irq_ts[irq].nsec = __ts.tv_nsec;
+#endif
+
        old_regs = set_irq_regs(regs);
        irq_enter();
 #ifdef CONFIG_DEBUG_STACKOVERFLOW

This allow the following modifications to the dcd_change() method:

static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int irq,
                               unsigned int status)
{
       int id = (int) tty->disc_data;

       pps_event(id, &pps_irq_ts[irq],
                       status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR, tty);

       pr_debug("[IRQev] PPS %s at %lu on source #%d\n",
                       status ? "assert" : "clear", jiffies, id);
}

Note that in this case I need also the "irq" parameter.

This solution gives very high timestamps precision! Better that
before! :D

What do you think about this solution? More precisely:

1) The low-level IRQ patch can be acceptable?

2) The new dcd_change() method is well implemented? Can I add the
"irq" parameter or I can find it somewhere?

Thanks a lot for your suggestions,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti@enneenne.com
Linux Device Driver                             giometti@linux.it
Embedded Systems                     phone:	+39 349 2432127
UNIX programming                     skype:     rodolfo.giometti

Re: LinuxPPS low-level IRQs timestamps & ldisc

[Lennart Sorensen]

On Sun, Jun 01, 2008 at 06:15:10PM +0200, Rodolfo Giometti wrote:
> Hello,
> 
> I write to both of you since my questions are related. :)
> 
> My last modifications to LinuxPPS are focused on adding a PPS ldisc
> and a low-level IRQs timestamps recording.
> 
> First I added a new dcd_change() ldisc method which can be used as
> follow:
> 
> static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status)
> {
>        int id = (int) tty->disc_data;
>        struct timespec __ts;
>        struct pps_ktime ts;
> 
>        /* First of all we get the time stamp... */
>        getnstimeofday(&__ts);
> 
>        /* ... and translate it to PPS time data struct */
>        ts.sec = __ts.tv_sec;
>        ts.nsec = __ts.tv_nsec;
> 
>        /* Now do the PPS event report */
>        pps_event(id, &ts,
>                        status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR, tty);
> 
>        pr_debug("[STDev] PPS %s at %lu on source #%d\n",
>                        status ? "assert" : "clear", jiffies, id);
> }
> 
> However this solution gives very low precision timestamps so that's
> why I propose to add the following code to register IRQ timestamps as
> soon as possible (here the code for x86_32 architecture):
> 
> diff --git a/arch/x86/kernel/irq_32.c b/arch/x86/kernel/irq_32.c
> index d3fde94..a3b8457 100644
> --- a/arch/x86/kernel/irq_32.c
> +++ b/arch/x86/kernel/irq_32.c
> @@ -15,6 +15,7 @@
>  #include <linux/notifier.h>
>  #include <linux/cpu.h>
>  #include <linux/delay.h>
> +#include <linux/pps.h>
>  
>  #include <asm/apic.h>
>  #include <asm/uaccess.h>
> @@ -25,6 +26,11 @@ EXPORT_PER_CPU_SYMBOL(irq_stat);
>  DEFINE_PER_CPU(struct pt_regs *, irq_regs);
>  EXPORT_PER_CPU_SYMBOL(irq_regs);
>  
> +#ifdef CONFIG_PPS_IRQ_EVENTS
> +struct pps_ktime pps_irq_ts[NR_IRQS];
> +EXPORT_SYMBOL(pps_irq_ts);
> +#endif
> +
>  /*
>   * 'what should we do if we get a hw irq event on an illegal vector'.
>   * each architecture has to answer this themselves.
> @@ -76,6 +82,12 @@ fastcall unsigned int do_IRQ(struct pt_regs *regs)
>         union irq_ctx *curctx, *irqctx;
>         u32 *isp;
>  #endif
> +#ifdef CONFIG_PPS_IRQ_EVENTS
> +       struct timespec __ts;
> +
> +       /* Get IRQ timestamps as soon as possible for the PPS layer */
> +       getnstimeofday(&__ts);
> +#endif

How expensive is this call?  Would it be cheaper to check whether this
IRQ has anyone requesting the time info before doing it?  I suspect most
systems will have at most one IRQ that actually needs this done, so does
doing it for all IRQs make sense?

>         if (unlikely((unsigned)irq >= NR_IRQS)) {
>                 printk(KERN_EMERG "%s: cannot handle IRQ %d\n",
> @@ -83,6 +95,12 @@ fastcall unsigned int do_IRQ(struct pt_regs *regs)
>                 BUG();
>         }
>  
> +#ifdef CONFIG_PPS_IRQ_EVENTS
> +       /* Then, after sanity check, store the IRQ timestamp */
> +       pps_irq_ts[irq].sec = __ts.tv_sec;
> +       pps_irq_ts[irq].nsec = __ts.tv_nsec;
> +#endif
> +
>         old_regs = set_irq_regs(regs);
>         irq_enter();
>  #ifdef CONFIG_DEBUG_STACKOVERFLOW
> 
> This allow the following modifications to the dcd_change() method:
> 
> static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int irq,
>                                unsigned int status)
> {
>        int id = (int) tty->disc_data;
> 
>        pps_event(id, &pps_irq_ts[irq],
>                        status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR, tty);
> 
>        pr_debug("[IRQev] PPS %s at %lu on source #%d\n",
>                        status ? "assert" : "clear", jiffies, id);
> }
> 
> Note that in this case I need also the "irq" parameter.
> 
> This solution gives very high timestamps precision! Better that
> before! :D
> 
> What do you think about this solution? More precisely:
> 
> 1) The low-level IRQ patch can be acceptable?
> 
> 2) The new dcd_change() method is well implemented? Can I add the
> "irq" parameter or I can find it somewhere?
> 
> Thanks a lot for your suggestions,

I certainly like high precision time stamps.  Very useful for ntp.

-- 
Len Sorensen

Re: LinuxPPS low-level IRQs timestamps & ldisc

[Alan Cox]

> First I added a new dcd_change() ldisc method which can be used as
> follow:

This looks good.
> 
> static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status)
> {
>        int id = (int) tty->disc_data;
>        struct timespec __ts;
>        struct pps_ktime ts;
> 
>        /* First of all we get the time stamp... */
>        getnstimeofday(&__ts);
> 
>        /* ... and translate it to PPS time data struct */
>        ts.sec = __ts.tv_sec;
>        ts.nsec = __ts.tv_nsec;
> 
>        /* Now do the PPS event report */
>        pps_event(id, &ts,
>                        status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR, tty);
> 
>        pr_debug("[STDev] PPS %s at %lu on source #%d\n",
>                        status ? "assert" : "clear", jiffies, id);
> }
> 
> However this solution gives very low precision timestamps so that's

Where are you calling it from, and how ?

> 2) The new dcd_change() method is well implemented? Can I add the
> "irq" parameter or I can find it somewhere?

Not all tty devices have an IRQ (I suspect those which do not are useless
for PPS reporting however). 

Do you get decent reports if you change the tty driver to do

	getnstimeofday(&ts);
	[blah blah lots of serial port I/O code]
	if (dcd ^ old_dcd)
		ld->dcd_change(tty, dcd, &ts);

??

Alan

Re: LinuxPPS low-level IRQs timestamps & ldisc

[Rodolfo Giometti]

On Mon, Jun 02, 2008 at 07:51:13AM +0100, Alan Cox wrote:
> > First I added a new dcd_change() ldisc method which can be used as
> > follow:
> 
> This looks good.

:)

> > 
> > static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status)
> > {
> >        int id = (int) tty->disc_data;
> >        struct timespec __ts;
> >        struct pps_ktime ts;
> > 
> >        /* First of all we get the time stamp... */
> >        getnstimeofday(&__ts);
> > 
> >        /* ... and translate it to PPS time data struct */
> >        ts.sec = __ts.tv_sec;
> >        ts.nsec = __ts.tv_nsec;
> > 
> >        /* Now do the PPS event report */
> >        pps_event(id, &ts,
> >                        status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR, tty);
> > 
> >        pr_debug("[STDev] PPS %s at %lu on source #%d\n",
> >                        status ? "assert" : "clear", jiffies, id);
> > }
> > 
> > However this solution gives very low precision timestamps so that's
> 
> Where are you calling it from, and how ?

>From serial_core.h as follow:

static inline void
uart_handle_dcd_change(struct uart_port *port, unsigned int status)
{
        struct uart_info *info = port->info;
        struct tty_ldisc *ld = tty_ldisc_ref(info->tty);

        if (ld->dcd_change)
                ld->dcd_change(info->tty, port->irq, status);
        tty_ldisc_deref(ld);

        port->icount.dcd++;

        if (info->flags & UIF_CHECK_CD) {
                if (status)
                        wake_up_interruptible(&info->open_wait);
                else if (info->tty)
                        tty_hangup(info->tty);
        }
}

> > 2) The new dcd_change() method is well implemented? Can I add the
> > "irq" parameter or I can find it somewhere?
> 
> Not all tty devices have an IRQ (I suspect those which do not are useless
> for PPS reporting however). 

Yes. Those devices are completely useless.

> Do you get decent reports if you change the tty driver to do
> 
> 	getnstimeofday(&ts);
> 	[blah blah lots of serial port I/O code]
> 	if (dcd ^ old_dcd)
> 		ld->dcd_change(tty, dcd, &ts);
> 
> ??

It could be a bit better... I did as above since I supposed that you
wished all PPS code should be removed from serial port code.

However, using your suggestion, I can do:

static inline void
uart_handle_dcd_change(struct uart_port *port, unsigned int status)
{
        struct uart_info *info = port->info;
        struct tty_ldisc *ld = tty_ldisc_ref(info->tty);
        struct pps_ktime ts;

#ifndef CONFIG_PPS_IRQ_EVENTS
        struct timespec __ts;  

        /* First of all we get the time stamp... */
        getnstimeofday(&__ts);

        /* ... and translate it to PPS time data struct */
        ts.sec = __ts.tv_sec;
        ts.nsec = __ts.tv_nsec;
#else
	/* Get the IRQ timestamp from system array */
	ts = pps_irq_ts[irq];
#endif

	if (ld->dcd_change)
		ld->dcd_change(info->tty, status, &ts);
	tty_ldisc_deref(ld);

	...

This is better then ever! :)

Thanks,

Rodolfo	

-- 

GNU/Linux Solutions                  e-mail:    giometti@enneenne.com
Linux Device Driver                             giometti@linux.it
Embedded Systems                     phone:	+39 349 2432127
UNIX programming                     skype:     rodolfo.giometti

Re: LinuxPPS low-level IRQs timestamps & ldisc

[Alan Cox]

> It could be a bit better... I did as above since I supposed that you
> wished all PPS code should be removed from serial port code.

Ideally. But if it is genuinely the case that the serial port IRQ handler
in some cases needs to do

my_interrupt() {
	get_timestamp()
	frob_with_hardware()
	ld->dcd_change(blah, timestamp)
}

then that is still fairly clean and more importantly actually appears to
work. I'd avoid all the ifdefs with this in the serial drivers that need
more accuracy:

{
	struct timespec ts;
	if (ld->dcd_change)
		getnstimeofday(&ts);
	existing tty stuff
	if (ld->dcd_change)
		ld->dcd_change(tty, status, &ts);
}

And in ld->dcd_change do

	struct timespec myts;
	/* Caller passed NULL meaning 'do your own timestamp' */
	if (ts == NULL) {
		ts = &myts;
		getnstimeofday(&myts);
	}

so for uart that would

	uart_dcd_change(struct uart_port *port, unsigned int status,
					struct timespec *ts) {
	{
		...
	}

passing the timestamp from the ttys own IRQ handler

Re: LinuxPPS low-level IRQs timestamps & ldisc

[Rodolfo Giometti]

On Mon, Jun 02, 2008 at 03:02:57PM +0100, Alan Cox wrote:
> > It could be a bit better... I did as above since I supposed that you
> > wished all PPS code should be removed from serial port code.
> 
> Ideally. But if it is genuinely the case that the serial port IRQ handler
> in some cases needs to do
> 
> my_interrupt() {
> 	get_timestamp()
> 	frob_with_hardware()
> 	ld->dcd_change(blah, timestamp)
> }
> 
> then that is still fairly clean and more importantly actually appears to
> work. I'd avoid all the ifdefs with this in the serial drivers that need
> more accuracy:
> 
> {
> 	struct timespec ts;
> 	if (ld->dcd_change)
> 		getnstimeofday(&ts);
> 	existing tty stuff
> 	if (ld->dcd_change)
> 		ld->dcd_change(tty, status, &ts);
> }

I prefere avoid the if clause for getnstimeofday() since each
instruction delay may decrease time precision, so:

{
      struct timespec ts;
      getnstimeofday(&ts);
      existing tty stuff
      if (ld->dcd_change)
              ld->dcd_change(tty, status, &ts);
}

In the wrost case the timestamp is not used.

> And in ld->dcd_change do
> 
> 	struct timespec myts;
> 	/* Caller passed NULL meaning 'do your own timestamp' */
> 	if (ts == NULL) {
> 		ts = &myts;
> 		getnstimeofday(&myts);
> 	}

Ok.

> so for uart that would
> 
> 	uart_dcd_change(struct uart_port *port, unsigned int status,
> 					struct timespec *ts) {
> 	{
> 		...
> 	}
> 
> passing the timestamp from the ttys own IRQ handler

Great!

Thanks,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti@enneenne.com
Linux Device Driver                             giometti@linux.it
Embedded Systems                     phone:	+39 349 2432127
UNIX programming                     skype:     rodolfo.giometti

Re: LinuxPPS low-level IRQs timestamps & ldisc

[Alan Cox]

> I prefere avoid the if clause for getnstimeofday() since each
> instruction delay may decrease time precision, so:

Why should everyone else pay the cost of the getnstimeofday they don't
need ?

The single conditional check at the start won't make the slightest
material difference to your accuracy.

Alan

Re: LinuxPPS low-level IRQs timestamps & ldisc

[Rodolfo Giometti]

On Mon, Jun 02, 2008 at 04:48:40PM +0100, Alan Cox wrote:
> > I prefere avoid the if clause for getnstimeofday() since each
> > instruction delay may decrease time precision, so:
> 
> Why should everyone else pay the cost of the getnstimeofday they don't
> need ?
> 
> The single conditional check at the start won't make the slightest
> material difference to your accuracy.

In the past we noticed that just moving up the getnstimeofday() of
only one instruction, from:

	port->icount.dcd++;
	getnstimeofday(&ts);

to:

	getnstimeofday(&ts);
	port->icount.dcd++;

the time precision improved from 5ms to 2ms...

However, if you prefere that I add the if clause, I'll do it.

Thanks,

Rodolfo 

-- 

GNU/Linux Solutions                  e-mail:    giometti@enneenne.com
Linux Device Driver                             giometti@linux.it
Embedded Systems                     phone:	+39 349 2432127
UNIX programming                     skype:     rodolfo.giometti

Re: LinuxPPS low-level IRQs timestamps & ldisc

[Alan Cox]

> the time precision improved from 5ms to 2ms...
> 
> However, if you prefere that I add the if clause, I'll do it.

I would prefer it - lets get it in, working and tested and then we can
see what can be done about finer details.

Re: LinuxPPS low-level IRQs timestamps & ldisc

[Matti Aarnio]

On Mon, Jun 02, 2008 at 07:22:03PM +0200, Rodolfo Giometti wrote:
...
> In the past we noticed that just moving up the getnstimeofday() of
> only one instruction, from:
> 
> 	port->icount.dcd++;
> 	getnstimeofday(&ts);
> 
> to:
> 
> 	getnstimeofday(&ts);
> 	port->icount.dcd++;
> 
> the time precision improved from 5ms to 2ms...

Perhaps I am looking for unattainable..  but I have HP/Agilent 58534A
(a.k.a Symmetricom 58534A) "GPS Timing Antenna",  which specifications
are telling that it should attain 1PPS timing jitter of less than 40 ns
in proper stabilized position hold mode.

Can that level be handled with standard PC hardware, or should the time
keeping and core parts of NTP be delegated into FPGA entirely ?
Stratum-1 is rather demanding, after all...

> However, if you prefere that I add the if clause, I'll do it.
> Thanks,
> Rodolfo 

Thanks,  Matti

Re: LinuxPPS low-level IRQs timestamps & ldisc

[Alan Cox]

> Perhaps I am looking for unattainable..  but I have HP/Agilent 58534A
> (a.k.a Symmetricom 58534A) "GPS Timing Antenna",  which specifications
> are telling that it should attain 1PPS timing jitter of less than 40 ns
> in proper stabilized position hold mode.
> 
> Can that level be handled with standard PC hardware, or should the time
> keeping and core parts of NTP be delegated into FPGA entirely ?
> Stratum-1 is rather demanding, after all...

That depends on what the NTP logic needs. Hardware IRQ time stamping is
fairly easy to do and providing your algorithms work with knowing the
time that the event occurred relative to a timer on the stamping device
(+/- bus delays and inaccuracies in the timers) you can get very good
numbers - but I doubt in the 40nS range.

Alan (who actually worked on a time stamping hardware toy almost 20 years
ago)

Re: LinuxPPS low-level IRQs timestamps & ldisc

[Lennart Sorensen]

On Mon, Jun 02, 2008 at 04:56:03PM +0200, Rodolfo Giometti wrote:
> I prefere avoid the if clause for getnstimeofday() since each
> instruction delay may decrease time precision, so:

Not if the delay is always constant.  That you can adjust for.  You
already have to adjust for cable length and such for your GPS, so
adjusting for a fixed delay for a conditional is no big deal.

Of course if you have to check of the DCD changed, that may not be
constant.  A check for whether this device currently has anyone that
wants the timestamp though should be a constant time.

-- 
Len Sorensen

Re: LinuxPPS low-level IRQs timestamps & ldisc

[Alan Cox]

> Of course if you have to check of the DCD changed, that may not be
> constant.  A check for whether this device currently has anyone that
> wants the timestamp though should be a constant time.

Not having to take the cost of that for other users is far more important
than a time variance of a jump prediction. CPU cache effects alone will
add way more variance already. It's not a meaningful number to chase
compared to the randomness introduced by the IRQ handling itself.

If you want perfect accuracy do what I²IT did years ago and use a card
which timestamps the physical IRQ bus transition and exposes it to the OS.

Alan