Pulse per Second (PPS) support for Linux.
Signed-off-by: Rodolfo Giometti <[email protected]>
---
Please, note that this PPS implementation is not RFC 2783 fully
compatible since, IMHO, the RFC simply doesn't consider PPS devices
connected with special GPIOs or other ports different from serial
ports and parallele ports.
Please read the following consideratios before sending to /dev/null!
:)
RFC considerations
------------------
While implementing a PPS API as RFC 2783 defines and using an embedded
CPU GPIO-Pin as physical link to the signal, I encountered a deeper
problem:
At startup it needs a file descriptor as argument for the function
time_pps_create().
This implies that the source has a /dev/... entry. This assumption is
ok for the serial and parallel port, where you can do something
usefull beside(!) the gathering of timestamps as it is the central
task for a PPS-API. But this assumption does not work for a single
purpose GPIO line. In this case even basic file-related functionality
(like read() and write()) makes no sense at all and should not be a
precondition for the use of a PPS-API.
The problem can be simply solved if you change the original RFC 2783:
pps_handle_t type is an opaque __scalar type__ used to represent a
PPS source within the API
into a modified:
pps_handle_t type is an opaque __variable__ used to represent a PPS
source within the API and programs should not access it directly to
it due to its opacity.
This change seems to be neglibile because even the original RFC 2783
does not encourage programs to check (read: use) the pps_handle_t
variable before calling the time_pps_*() functions, since each
function should do this job internally.
If I intentionally separate the concept of "file descriptor" from the
concept of the "PPS source" I'm obliged to provide a solution to find
and register a PPS-source without using a file descriptor: it's done
by the functions time_pps_findsource() and time_pps_findpath() now.
According to this current NTPD drivers' code should be modified as
follows:
+#ifdef PPS_HAVE_FINDPATH
+ /* Get the PPS source's real name */
+ fd = readlink(link, path, STRING_LEN-1);
+ if (fd <= 0)
+ strncpy(path, link, STRING_LEN);
+ else
+ path[fd] = '\0';
+
+ /* Try to find the source */
+ fd = time_pps_findpath(path, STRING_LEN, id, STRING_LEN);
+ if (fd < 0) {
+ msyslog(LOG_ERR, "refclock: cannot find PPS source \"%s\" in the system", path);
+ return 1;
+ }
+ msyslog(LOG_INFO, "refclock: found PPS source #%d \"%s\" on \"%s\"", fd, path, id);
+#endif /* PPS_HAVE_FINDPATH */
+
+
if (time_pps_create(fd, &pps_handle) < 0) {
- pps_handle = 0;
msyslog(LOG_ERR, "refclock: time_pps_create failed: %m");
}
On Fri, Feb 16, 2007 at 07:52:30PM +0100, Rodolfo Giometti wrote:
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 98ec861..cd9a003 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -1315,8 +1315,25 @@ static unsigned int check_modem_status(struct uart_8250_port *up)
> up->port.icount.rng++;
> if (status & UART_MSR_DDSR)
> up->port.icount.dsr++;
> - if (status & UART_MSR_DDCD)
> + if (status & UART_MSR_DDCD) {
> +#ifdef CONFIG_PPS_CLIENT_UART_8250
> + if (status & UART_MSR_DCD) {
> + linuxpps_event(up->port.pps_source,
> + PPS_CAPTUREASSERT, up);
> + dbg("serial8250: PPS assert event at %lu "
> + "on source #%d",
> + jiffies, up->port.pps_source);
> + }
> + else {
> + linuxpps_event(up->port.pps_source,
> + PPS_CAPTURECLEAR, up);
> + dbg("serial8250: PPS clear event at %lu "
> + "on source #%d",
> + jiffies, up->port.pps_source);
> + }
> +#endif
Yuck. Please. No. Doing it this way means you have to modify every
single serial driver out there which is a mamouth task.
> uart_handle_dcd_change(&up->port, status & UART_MSR_DCD);
Did you not look to see what's in this helper? You'll find within here
the following code:
#ifdef CONFIG_HARD_PPS
if ((port->flags & UPF_HARDPPS_CD) && status)
hardpps();
#endif
which should've been a big sign lit up in bright lights in Times Square
pointing you towards the right place to put your code.
> + }
> if (status & UART_MSR_DCTS)
> uart_handle_cts_change(&up->port, status & UART_MSR_CTS);
>
> @@ -2004,6 +2021,9 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
> up->ier |= UART_IER_MSI;
> if (up->capabilities & UART_CAP_UUE)
> up->ier |= UART_IER_UUE | UART_IER_RTOIE;
> +#ifdef CONFIG_PPS_CLIENT_UART_8250
> + up->ier |= UART_IER_MSI; /* enable interrupts */
> +#endif
Why not continue to leave it as a decision of the administrator - if
you want ports to default to having PPS support enabled, change all
the registration to set UPF_HARDPPS_CD. But leave the admin with
the ability to disable it.
> @@ -2124,6 +2130,33 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
> */
> if (!uart_console(port))
> uart_change_pm(state, 3);
> +
> +#ifdef CONFIG_PPS_CLIENT_UART
> + /*
> + * Add the PPS support for the probed port.
> + */
> + snprintf(state->pps_info.name, LINUXPPS_MAX_NAME_LEN,
> + "%s%d", drv->driver_name, port->line);
> + snprintf(state->pps_info.path, LINUXPPS_MAX_NAME_LEN,
> + "/dev/%s%d", drv->dev_name, port->line);
> +
> + state->pps_info.mode = PPS_CAPTUREBOTH | \
> + PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
> + PPS_CANWAIT | PPS_TSFMT_TSPEC;
> +
> + retval = linuxpps_register_source(&state->pps_info,
> + PPS_CAPTUREBOTH | \
> + PPS_OFFSETASSERT | PPS_OFFSETCLEAR,
> + -1 /* PPS ID is up to the system */);
> + if (retval < 0) {
> + err("cannot register PPS source \"%s\"",
> + state->pps_info.path);
> + return;
> + }
> + port->pps_source = retval;
> + info("PPS source #%d \"%s\" added to the system ",
> + port->pps_source, state->pps_info.path);
> +#endif
This means that PPS support is not available for any port which wasn't
autoprobed at device discovery time. That seems quite restrictive.
Maybe it needs to be coupled with the setting/clearing of UPF_HARDPPS_CD ?
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
Some non political comments
Rodolfo Giometti wrote:
> +Coding example
> +--------------
> +
> +To register a PPS source into the kernel you should define a struct
> +linuxpps_source_info_s as follow:
> +
> + static struct linuxpps_source_info_s linuxpps_ktimer_info = {
Drop the linux prefix. It's in the linux kernel after all.
> +PROCFS support
> +--------------
New features shouldn't introduce new /proc stuff.
> +Resources
> +---------
> +
> +Wiki: http://wiki.enneenne.com/index.php/LinuxPPS_support and the LinuxPPS
> +ML: http://ml.enneenne.com/cgi-bin/mailman/listinfo/linuxpps.
Add to MAINTAINERS
Your way to hook into lp and 8250 is pretty gross. It should at least be
possible to deactivate it via the kernel command line, but it would be
a lot nicer to have pps_lp and pps_8250 modules which you can load. Also
what happens if you've multiple lp ports? How do you control which to
grab?
- don't implement your own dbg() stuff, use dprintk and friends
- drop the inlines, gcc will do the right thing.
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> static struct parport_driver lp_driver = {
> diff --git a/drivers/parport/parport_pc.c b/drivers/parport/parport_pc.c
> index b61c17b..426b0ac 100644
> --- a/drivers/parport/parport_pc.c
> +++ b/drivers/parport/parport_pc.c
> @@ -272,6 +272,14 @@ static int clear_epp_timeout(struct parport *pb)
>
> static irqreturn_t parport_pc_interrupt(int irq, void *dev_id)
> {
> +#ifdef CONFIG_PPS_CLIENT_LP_PARPORT_PC
> + struct parport *p = (struct parport *) dev_id;
> +
> + linuxpps_event(p->pps_source, PPS_CAPTUREASSERT, p);
Perhaps just implement empty defines for the none pps cases and get
rid of the ifdefs? But this should really be controllabe via
sysfs or such.
> --- /dev/null
> +++ b/drivers/pps/clients/Kconfig
> @@ -0,0 +1,56 @@
> +#
> +# LinuxPPS clients configuration
> +#
> +
> +if PPS
> +
> +comment "PPS clients support"
> +
> +config PPS_CLIENT_KTIMER
> + tristate "Kernel timer client (Testing client, use for debug)"
> + help
> + If you say yes here you get support for a PPS debugging client
> + which uses a kernel timer to generate the PPS signal.
> +
> + This driver can also be built as a module. If so, the module
> + will be called ktimer.o.
> +
> +comment "UART serial support (forced off)"
> + depends on SERIAL_CORE && ( PPS = m && SERIAL_CORE = y )
> +
> +config PPS_CLIENT_UART
> + bool "UART serial support"
> + depends on SERIAL_CORE && ! ( PPS = m && SERIAL_CORE = y )
help text
> +
> +comment "8250 serial support (forced off)"
> + depends on PPS_CLIENT_UART && SERIAL_8250 && \
> + ( PPS = m && SERIAL_8250 = y )
> +
> +config PPS_CLIENT_UART_8250
> + bool "8250 serial support"
> + depends on PPS_CLIENT_UART && SERIAL_8250 && \
> + ! ( PPS = m && SERIAL_8250 = y )
> + help
> + If you say yes here you get support for a PPS source connected
> + with the CD (Carrier Detect) pin of your 8250 serial line chip.
> +
> +comment "Parallel printer support (forced off)"
> + depends on PRINTER && ( PPS = m && PRINTER = y )
> +
> +config PPS_CLIENT_LP
> + bool "Parallel printer support"
> + depends on PRINTER && ! ( PPS = m && PRINTER = y )
help text
> +comment "Parport PC support (forced off)"
> + depends on PPS_CLIENT_LP && PARPORT_PC && \
> + ( PPS = m && PARPORT_PC = y )
> +
> +config PPS_CLIENT_LP_PARPORT_PC
> + bool "Parport PC support"
> + depends on PPS_CLIENT_LP && PARPORT_PC && \
> + ! ( PPS = m && PARPORT_PC = y )
> + help
> + If you say yes here you get support for a PPS source connected
> + with the interrupt pin of your PC parallel port.
help text and difference to CLIENT_LP?
> +++ b/drivers/pps/kapi.c
> +/* --- Local functions ----------------------------------------------------- */
> +
> +#ifndef NSEC_PER_SEC
> +#define NSEC_PER_SEC 1000000000
> +#endif
What's that for? Why is(n't) it defined?
> + for (i = 0; i < LINUXPPS_MAX_SOURCES; i++)
> + if (!__linuxpps_is_allocated(i))
> + break;
> + if (i >= LINUXPPS_MAX_SOURCES) {
> + err("no free source ids");
> + return -ENOMEM;
> + }
Why no dynamically allocated array?
> +void linuxpps_event(int source, int event, void *data)
> +{
> + struct timespec ts;
> +
> + /* In this function we shouldn't need locking at all since each PPS
> + * source arrives once per second and due the per-PPS source data
> + * array... */
I wouldn't bet on that.
> +++ b/drivers/pps/pps.c
> @@ -0,0 +1,377 @@
> +/*
> + * main.c -- Main driver file
Doesn't match filename
> +++ b/drivers/pps/procfs.c
I'd drop that completely.
> +++ b/include/linux/netlink.h
> @@ -21,7 +21,7 @@
> #define NETLINK_DNRTMSG 14 /* DECnet routing messages */
> #define NETLINK_KOBJECT_UEVENT 15 /* Kernel messages to userspace */
> #define NETLINK_GENERIC 16
> -/* leave room for NETLINK_DM (DM Events) */
> +#define NETLINK_PPSAPI 17 /* linuxPPS support */
You read the comment above your line?
> #define DEFAULT_SPIN_TIME 500 /* us */
> diff --git a/include/linux/pps.h b/include/linux/pps.h
> new file mode 100644
> index 0000000..52f67ce
> --- /dev/null
> +++ b/include/linux/pps.h
> +#ifdef CONFIG_PPS_DEBUG
> +#define dbg(format, arg...) printk(KERN_DEBUG "%s: " format "\n" , \
> + KBUILD_MODNAME , ## arg)
> +#else
> +#define dbg(format, arg...) do {} while (0)
> +#endif
> +
> +#define err(format, arg...) printk(KERN_ERR "%s: " format "\n" , \
> + KBUILD_MODNAME , ## arg)
> +#define info(format, arg...) printk(KERN_INFO "%s: " format "\n" , \
> + KBUILD_MODNAME , ## arg)
These should use dprintk and friends
> +
> +/* --- Global defines ------------------------------------------------------ */
> +
> +#define LINUXPPS_MAX_SOURCES 16
Isn't something like 4 more reasonable (lp + 8250 + ktimer?)
> +/* The main struct */
> +struct linuxpps_s {
> + struct linuxpps_source_info_s *info; /* PSS source info */
> +
> + pps_params_t params; /* PPS's current params */
> +
> + volatile pps_seq_t assert_sequence; /* PPS' assert event seq # */
> + volatile pps_seq_t clear_sequence; /* PPS' clear event seq # */
> + volatile pps_timeu_t assert_tu;
> + volatile pps_timeu_t clear_tu;
I think you can drop the volatiles, there was a discussion some time ago
that they mostly waste of words.
> +static inline int __linuxpps_is_allocated(int source) {
> + return linuxpps_source[source].info != NULL;
> +}
> +
> +static inline int linuxpps_is_allocated(int source) {
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&linuxpps_lock, flags);
> + ret = __linuxpps_is_allocated(source);
> + spin_unlock_irqrestore(&linuxpps_lock, flags);
> +
> + return ret;
> +}
This one looks pretty fishy. After the check you normally want
to use it, don't you? And then you already lost the guarantee.
> +#define to_class_dev(obj) container_of((obj), struct class_device, kobj)
pretty generic name.
> +++ b/include/linux/timepps.h
use a frequency-locked loop */
> +
> +/* --- Here begins the implementation-specific part! ----------------------- */
> +
> +#define LINUXPPS_MAX_NAME_LEN 32
> +struct pps_netlink_msg {
> + int cmd; /* the command to execute */
> + int source;
> + char name[LINUXPPS_MAX_NAME_LEN]; /* symbolic name */
> + char path[LINUXPPS_MAX_NAME_LEN]; /* path of the connected device */
> + int consumer; /* selected kernel consumer */
> + pps_params_t params;
> + int mode; /* edge */
> + int tsformat; /* format of time stamps */
> + pps_info_t info;
> + struct timespec timeout;
> + int ret;
> +};
Have you thought about 32/64bit issues?
> +/* Private functions */
> +
> +static int netlink_msg(int socket, struct pps_netlink_msg *nlpps)
Function in .h?
Jan
On Fri, Feb 16, 2007 at 07:12:08PM +0000, Russell King wrote:
>
> Yuck. Please. No. Doing it this way means you have to modify every
> single serial driver out there which is a mamouth task.
>
> > uart_handle_dcd_change(&up->port, status & UART_MSR_DCD);
>
> Did you not look to see what's in this helper? You'll find within here
> the following code:
>
> #ifdef CONFIG_HARD_PPS
> if ((port->flags & UPF_HARDPPS_CD) && status)
> hardpps();
> #endif
>
> which should've been a big sign lit up in bright lights in Times Square
> pointing you towards the right place to put your code.
Ok.
> Why not continue to leave it as a decision of the administrator - if
> you want ports to default to having PPS support enabled, change all
> the registration to set UPF_HARDPPS_CD. But leave the admin with
> the ability to disable it.
Ok.
> This means that PPS support is not available for any port which wasn't
> autoprobed at device discovery time. That seems quite restrictive.
How I can force probing for a specified uart port?
> Maybe it needs to be coupled with the setting/clearing of UPF_HARDPPS_CD ?
What do you think about? I should enable the PPS support only if the
userland sets the UPF_HARDPPS_CD flag?
Thanks for your comments,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Fri, Feb 16, 2007 at 09:43:36PM +0100, Rodolfo Giometti wrote:
> On Fri, Feb 16, 2007 at 07:12:08PM +0000, Russell King wrote:
> >
> > Yuck. Please. No. Doing it this way means you have to modify every
> > single serial driver out there which is a mamouth task.
> >
> > > uart_handle_dcd_change(&up->port, status & UART_MSR_DCD);
> >
> > Did you not look to see what's in this helper? You'll find within here
> > the following code:
> >
> > #ifdef CONFIG_HARD_PPS
> > if ((port->flags & UPF_HARDPPS_CD) && status)
> > hardpps();
> > #endif
> >
> > which should've been a big sign lit up in bright lights in Times Square
> > pointing you towards the right place to put your code.
>
> Ok.
>
> > Why not continue to leave it as a decision of the administrator - if
> > you want ports to default to having PPS support enabled, change all
> > the registration to set UPF_HARDPPS_CD. But leave the admin with
> > the ability to disable it.
>
> Ok.
>
> > This means that PPS support is not available for any port which wasn't
> > autoprobed at device discovery time. That seems quite restrictive.
>
> How I can force probing for a specified uart port?
You can't because it doesn't go through the interfaces you're hooking
into. Existing interfaces are "changed" to point at the UARTs using
setserial, which does its work via an ioctl.
> > Maybe it needs to be coupled with the setting/clearing of UPF_HARDPPS_CD ?
>
> What do you think about? I should enable the PPS support only if the
> userland sets the UPF_HARDPPS_CD flag?
Not specifically only userland - if it happens to be set when the port
is registered then enable PPS support then as well.
So:
1. uart_configure_port - if UPF_HARDPPS_CD is set, register the port
for PPS support.
2. uart_remove_one_port - if UPF_HARDPPS_CD is set, unregister the port
for PPS support.
3. uart_set_info - if changing UPF_HARDPPS_CD, appropriately register or
unregister the port for PPS support.
PS, [email protected] dropped from the cc: since it rejects my
postings.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Fri, Feb 16, 2007 at 08:56:18PM +0100, Jan Dittmer wrote:
> Drop the linux prefix. It's in the linux kernel after all.
Ok.
> > +PROCFS support
> > +--------------
>
> New features shouldn't introduce new /proc stuff.
It's a must? I can leave procfs for backward compatibility with old
utilities?
> Add to MAINTAINERS
Ok.
> Your way to hook into lp and 8250 is pretty gross. It should at least be
> possible to deactivate it via the kernel command line, but it would be
> a lot nicer to have pps_lp and pps_8250 modules which you can load. Also
I think it's not possible... however the Russell's suggestions should
go in that direction.
> what happens if you've multiple lp ports? How do you control which to
> grab?
No way... I can add a specific flag as for uart lines or a kernel
module parameter.
> - don't implement your own dbg() stuff, use dprintk and friends
> - drop the inlines, gcc will do the right thing.
Ok. Ok.
> Perhaps just implement empty defines for the none pps cases and get
> rid of the ifdefs? But this should really be controllabe via
> sysfs or such.
Mmm... let me think about howto implement that...
> help text
Ok.
> help text and difference to CLIENT_LP?
Ok.
> Why no dynamically allocated array?
It's easier! :P
Also it's very difficult having more that 3 or 4 PPS sources in a
system.
> I wouldn't bet on that.
Why not? =:-o
Also locking instructions may add extra code and delay the timestamp
recording...
> Doesn't match filename
I'm going to fix it.
> > +++ b/drivers/pps/procfs.c
>
> I'd drop that completely.
:'(
> You read the comment above your line?
No, sorry. I'm going to choose another id number... or can I keep 17?
> These should use dprintk and friends
Ok.
> Isn't something like 4 more reasonable (lp + 8250 + ktimer?)
It should be enought...
> I think you can drop the volatiles, there was a discussion some time ago
> that they mostly waste of words.
I see...
> This one looks pretty fishy. After the check you normally want
> to use it, don't you? And then you already lost the guarantee.
You are right...
> > +#define to_class_dev(obj) container_of((obj), struct class_device, kobj)
>
> pretty generic name.
I should change it?
> Have you thought about 32/64bit issues?
No. I have no 64 bits machine to test the code...
> Function in .h?
I'm going to check it.
Thanks for your suggestions,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Fri, Feb 16, 2007 at 08:51:35PM +0000, Russell King wrote:
>
> You can't because it doesn't go through the interfaces you're hooking
> into. Existing interfaces are "changed" to point at the UARTs using
> setserial, which does its work via an ioctl.
I see.
> Not specifically only userland - if it happens to be set when the port
> is registered then enable PPS support then as well.
>
> So:
>
> 1. uart_configure_port - if UPF_HARDPPS_CD is set, register the port
> for PPS support.
> 2. uart_remove_one_port - if UPF_HARDPPS_CD is set, unregister the port
> for PPS support.
> 3. uart_set_info - if changing UPF_HARDPPS_CD, appropriately register or
> unregister the port for PPS support.
Ok. I'm going to study how to modify the code.
> PS, [email protected] dropped from the cc: since it rejects my
> postings.
I just fixed my spam filter (at least I hope so :).
Thanks,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
Rodolfo Giometti wrote:
>>> +PROCFS support
>>> +--------------
>> New features shouldn't introduce new /proc stuff.
>
> It's a must? I can leave procfs for backward compatibility with old
> utilities?
Hmm, as this is a new feature with regard to the mainline kernel, old
utilities don't count (if you can install a new kernel you can also
be expected to install new user-space tools for the new feature).
>> You read the comment above your line?
>
> No, sorry. I'm going to choose another id number... or can I keep 17?
I don't know, ask whoever is responsible for the file.
>>> +#define to_class_dev(obj) container_of((obj), struct class_device, kobj)
>> pretty generic name.
>
> I should change it?
If it's of general use put it in the appropriate header file. If it's
just for the pps subsystem name it as such.
>> Have you thought about 32/64bit issues?
>
> No. I have no 64 bits machine to test the code...
Hmm, think about x86_64 with 64-bit kernel and 32-bit userspace, probably
having got different padding in the struct. Read LDD3, chapter 11,
especially 11.4 .
Jan
Hello,
after yours suggestions I modified my code. Please see the patch
below.
However some issues are still not modified. Here my reasons (lines
with ">" are yours suggestions):
> Your way to hook into lp and 8250 is pretty gross. It should at least be
> possible to deactivate it via the kernel command line, but it would be
> a lot nicer to have pps_lp and pps_8250 modules which you can load. Also
> what happens if you've multiple lp ports? How do you control which to
> grab?
Serial support has "setserial" and I used it to enable/disable PPS
support at runtime. Here the patch for setserial:
--- setserial.c.old 2007-02-17 08:47:45.000000000 +0100
+++ setserial.c 2007-02-17 08:57:26.000000000 +0100
@@ -127,6 +127,7 @@
CMD_FLAG, "hup_notify", ASYNC_HUP_NOTIFY, ASYNC_HUP_NOTIFY, 0, FLAG_CAN_INVERT,
CMD_FLAG, "skip_test", ASYNC_SKIP_TEST,ASYNC_SKIP_TEST,2, FLAG_CAN_INVERT,
CMD_FLAG, "auto_irq", ASYNC_AUTO_IRQ, ASYNC_AUTO_IRQ, 2, FLAG_CAN_INVERT,
+ CMD_FLAG, "hardpps", ASYNC_HARDPPS_CD, ASYNC_HARDPPS_CD, 2, FLAG_CAN_INVERT,
CMD_FLAG, "split_termios", ASYNC_SPLIT_TERMIOS, ASYNC_SPLIT_TERMIOS, 2, FLAG_CAN_INVERT,
CMD_FLAG, "session_lockout", ASYNC_SESSION_LOCKOUT, ASYNC_SESSION_LOCKOUT, 2, FLAG_CAN_INVERT,
CMD_FLAG, "pgrp_lockout", ASYNC_PGRP_LOCKOUT, ASYNC_PGRP_LOCKOUT, 2, FLAG_CAN_INVERT,
@@ -725,6 +726,7 @@
fprintf(stderr, "\t^ fourport\tconfigure the port as an AST Fourport\n");
fprintf(stderr, "\t autoconfig\tautomatically configure the serial port\n");
fprintf(stderr, "\t^ auto_irq\ttry to determine irq during autoconfiguration\n");
+ fprintf(stderr, "\t^ hardpps\tmanage PPS signal when CD changes status\n");
fprintf(stderr, "\t^ skip_test\tskip UART test during autoconfiguration\n");
fprintf(stderr, "\n");
fprintf(stderr, "\t^ sak\t\tset the break key as the Secure Attention Key\n");
For parallel support I just cleaned up the code but I didn't find
something similar to setserial so I decided to leave the code as is
since at interrupt time we just register a timestamp without touching
the parallel port functionality.
> don't implement your own dbg() stuff, use dprintk and friends
My own dbg() stuff are for specific debugging string for PPS code. If
I use pr_dbg & friends I also enable several (and unwanted) kernel
debugging lines.
> drop the inlines, gcc will do the right thing.
I like specify them. :) Hope this is not a problem...
> > + for (i = 0; i < LINUXPPS_MAX_SOURCES; i++)
> > + if (!__linuxpps_is_allocated(i))
> > + break;
> > + if (i >= LINUXPPS_MAX_SOURCES) {
> > + err("no free source ids");
> > + return -ENOMEM;
> > + }
>
> Why no dynamically allocated array?
It's just easier...
> > +void linuxpps_event(int source, int event, void *data)
> > +{
> > + struct timespec ts;
> > +
> > + /* In this function we shouldn't need locking at all since each PPS
> > + * source arrives once per second and due the per-PPS source data
> > + * array... */
>
> I wouldn't bet on that.
I see. However it is not a problem at all since if the device is a PPS
source we have just one interrupt at second, if not we just register a
timestamp never used.
> > +/* The main struct */
> > +struct linuxpps_s {
> > + struct linuxpps_source_info_s *info; /* PSS source info */
> > +
> > + pps_params_t params; /* PPS's current params */
> > +
> > + volatile pps_seq_t assert_sequence; /* PPS' assert event seq # */
> > + volatile pps_seq_t clear_sequence; /* PPS' clear event seq # */
> > + volatile pps_timeu_t assert_tu;
> > + volatile pps_timeu_t clear_tu;
>
> I think you can drop the volatiles, there was a discussion some time ago
> that they mostly waste of words.
As for inlines I like specify volatiles... :P
> > +/* Private functions */
> > +
> > +static int netlink_msg(int socket, struct pps_netlink_msg *nlpps)
>
> Function in .h?
That's why these functions must be used into userland from NTPD and it
doesn't consider specific libraries to link with... it's the same
trick used by PPSkit (another PPS support).
> > +#define LINUXPPS_MAX_NAME_LEN 32
> > +struct pps_netlink_msg {
> > + int cmd; /* the command to execute */
> > + int source;
> > + char name[LINUXPPS_MAX_NAME_LEN]; /* symbolic name */
> > + char path[LINUXPPS_MAX_NAME_LEN]; /* path of the connected device */
> > + int consumer; /* selected kernel consumer */
> > + pps_params_t params;
> > + int mode; /* edge */
> > + int tsformat; /* format of time stamps */
> > + pps_info_t info;
> > + struct timespec timeout;
> > + int ret;
> > +};
>
> Have you thought about 32/64bit issues?
I have no 64 bits hardware to test it... I just used PPC, MIPS and ARM
at 32 bits.
Thanks a lot for your suggestions!
Ciao,
Rodolfo
P.S. After your replies (and some tests) I'll repost my patch for
inclusion.
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
Rodolfo Giometti wrote:
>
> Please read the following consideratios before sending to /dev/null!
> :)
>
> RFC considerations
> ------------------
>
> While implementing a PPS API as RFC 2783 defines and using an embedded
> CPU GPIO-Pin as physical link to the signal, I encountered a deeper
> problem:
>
> At startup it needs a file descriptor as argument for the function
> time_pps_create().
>
> This implies that the source has a /dev/... entry. This assumption is
> ok for the serial and parallel port, where you can do something
> usefull beside(!) the gathering of timestamps as it is the central
> task for a PPS-API. But this assumption does not work for a single
> purpose GPIO line. In this case even basic file-related functionality
> (like read() and write()) makes no sense at all and should not be a
> precondition for the use of a PPS-API.
>
It's not a precondition for a file descriptor, either. There are plenty
of ioctl-only device drivers in existence.
Furthermore, a file descriptor doesn't imply a device entry. Consider
pipe(2), for example.
As far as the kernel is concerned, a file handle is a nice, uniform
system for providing communication between the kernel and user space.
It doesn't matter if one can read() or write() on it; it's perfectly
normal to support only a subset of the normal operations.
-hpa
On Mon, Feb 19, 2007 at 06:56:20PM -0800, H. Peter Anvin wrote:
> It's not a precondition for a file descriptor, either. There are plenty
> of ioctl-only device drivers in existence.
>
> Furthermore, a file descriptor doesn't imply a device entry. Consider
> pipe(2), for example.
>
> As far as the kernel is concerned, a file handle is a nice, uniform
> system for providing communication between the kernel and user space.
> It doesn't matter if one can read() or write() on it; it's perfectly
> normal to support only a subset of the normal operations.
The problem is that sometimes you cannot have a filedescriptor at
all. Think about a PPS source connected with a CPU's GPIO pin. You
have no filedes to use and defining one just for a PPS source or for a
class of PPS sources, I think, is a non sense.
RFC simply doesn't consider the fact that you can have a PPS source
__without__ a filedes connected with, and a single filedes is
considered __always__ connected with a single PPS source.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
Rodolfo Giometti wrote:
>
> The problem is that sometimes you cannot have a filedescriptor at
> all. Think about a PPS source connected with a CPU's GPIO pin. You
> have no filedes to use and defining one just for a PPS source or for a
> class of PPS sources, I think, is a non sense.
>
If you have a kernel driver at all, then it makes perfect sense. If you
don't have a kernel driver at all, then it's irrelevant to the
linux-kernel discussion.
> RFC simply doesn't consider the fact that you can have a PPS source
> __without__ a filedes connected with, and a single filedes is
> considered __always__ connected with a single PPS source.
That's the Unix way.
-hpa
Hi,
On Wednesday 21 February 2007 13:04, Rodolfo Giometti wrote:
> RFC simply doesn't consider the fact that you can have a PPS source
> __without__ a filedes connected with, and a single filedes is
> considered __always__ connected with a single PPS source.
That's not entirely true. It doesn't say that pps_handle_t must be a file
descriptor and it leaves the option for the argument to time_pps_create() not
to be a file descriptor as well.
bye, Roman
On Wed, Feb 21, 2007 at 08:14:14AM -0800, H. Peter Anvin wrote:
>
> If you have a kernel driver at all, then it makes perfect sense. If you
> don't have a kernel driver at all, then it's irrelevant to the
> linux-kernel discussion.
???
So you are told me that if my PPS source is connected with a single
CPU GPIO I have to add a new driver to the kernel? I have to choose a
proper major/minor numbers for just one PIN? =:-o
> >RFC simply doesn't consider the fact that you can have a PPS source
> >__without__ a filedes connected with, and a single filedes is
> >considered __always__ connected with a single PPS source.
>
> That's the Unix way.
But not PPS one. In several embedded systems the GPS antenna is
connected with the serial port and the PPS source, which cames from
the __same__ GPS antenna, is connected with a GPIO.
How I should manage such case? Which fildes I should use? Also
consider that NTPD currently manage only one filedes which should
support both GPS antenna's data and PPS source. My patch can solve
this problem.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Thu, Feb 22, 2007 at 12:51:48AM +0100, Roman Zippel wrote:
> Hi,
>
> On Wednesday 21 February 2007 13:04, Rodolfo Giometti wrote:
>
> > RFC simply doesn't consider the fact that you can have a PPS source
> > __without__ a filedes connected with, and a single filedes is
> > considered __always__ connected with a single PPS source.
>
> That's not entirely true. It doesn't say that pps_handle_t must be a file
> descriptor and it leaves the option for the argument to time_pps_create() not
> to be a file descriptor as well.
Yes. In fact that's my solution!
The problem is that "pps_handle_t" is forced by the RFC to be a scalar
and _not_ a generic (and opaque) type. I suppose that since right now
such handler was simply the filedes of the serial/parallel port
connected with the GPS antenna. But this is not always the case. As
already told the GPS antenna can be connected with the serial line
while the PPS signal is not, so NTPD should always open the serial
port to read GPS data but it must not use such filedes for the
time_pps_create().
My support try to resolve this problem with minor changes in both RFC
and NTPD code.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
Hi!
> @@ -0,0 +1,206 @@
> +
> + PPS - Pulse Per Second
> + ----------------------
> +
> +(C) Copyright 2007 Rodolfo Giometti <[email protected]>
If you add copyright, add license, too.
> +PROCFS support
> +--------------
> +
> +If PROCFS support is enabled a new directory "/proc/pps" is created:
> +
> + $ ls /proc/pps/
> + 00 01 sources
> +
> +The file "sources" holds a brief description of all PPS sources
> +defined in the system:
> +
> + $ cat /proc/pps/sources
> + id mode echo name path
> + ---- ------ ---- ---------------- ----------------
> + 00 1133 no serial0 /dev/ttyS0
> + 01 1133 no serial1 /dev/ttyS1
> +
> +The other entries are directories that hold one or two files according
> +to the ability of the associated source to provide "assert" and
> +"clear" timestamps:
> +
> + $ ls /proc/pps/00
> + assert clear
> +
> +Inside each "assert" and "clear" file you can find the timestamp and a
> +sequence number:
> +
> + $ cat /proc/pps/00/assert
> + 1170026870.983207967 #8
No new /proc files, please.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, Feb 21, 2007 at 10:16:45AM +0000, Pavel Machek wrote:
> Hi!
>
> > @@ -0,0 +1,206 @@
> > +
> > + PPS - Pulse Per Second
> > + ----------------------
> > +
> > +(C) Copyright 2007 Rodolfo Giometti <[email protected]>
>
> If you add copyright, add license, too.
Ok.
> No new /proc files, please.
Already removed. As soon as possible I'll repost a new patch.
Thanks!
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127