[LinuxPPS] PPS on Raspberry Pi - with device tree

[Jan Lübbe]

On Mi, 2016-08-10 at 06:09 -0400, tlhackque wrote:
> On 10-Aug-16 03:55, Jan Lübbe wrote:
> > On Di, 2016-08-09 at 18:46 -0400, tlhackque wrote:
> >> Add assert_falling_edge=true to config.txt, e.g.:
> >>
> >> dtoverlay=pps-gpio,gpiopin=24,assert_falling_edge=true
> >>
> >> Reboot, and you're in business.
> >>
> >> In debugging this, I noticed that the driver doesn't have a
> >> "capture_clear" parameter for the device tree.
> >> I think one should be added - there is code for this on the "pdata"
> >> path.  Something like:
> >> if (of_get_property(np, "capture-clear", NULL))
> >>             data->capture_clear = true;
> >> at line 122 should do it.  Then the DT and pdata paths should be equivalent.
> >>
> >> For reference, the RPi issue for this is
> >> https://github.com/raspberrypi/linux/issues/1590
> > No. As the DT should describe the hardware, it only describes which edge
> > is the assert. Which edge is captured at is configured by userspace at
> > runtime.
> >
> > NTPd can configure the PPS capture itself or you can use pps-tools.
> >
> > Regards,
> > Jan
> NTPd isn't the only client.
> 
> The driver (drivers/pps/clients/pps-gpio.c) is what captures the
> timestamp at interrupt level.
> *It* is the code which calls pps_register_source().  The PPS userspace
> tools will not see capture_clear events unless the capture_clear flag
> is asserted.
> (Neither will ntpd, though in most configurations it doesn't care.  So
> the default reduces event generation by 1/2.) The flag allows both
> edges to be captured.

Looking again at the driver, it seems that it is missing the code to
check if the GPIO supports configuring an interrupt on both edges.

The GPIO controller knows the actual pin's IRQ capabilities, so pps-gpio
should derive data->info.mode from that. No need to duplicate the pin's
capabilities in the pps-gpio DT node.

> The driver provides the ability to configure capture_clear on the
> traditional configuration code path.
> Since DT is the new way to configure the driver, DT should too. With
> the Raspbian implementation, there is no explicit modprobe; DT
> (proper, or the config.txt overlay) triggers driver loading and is the
> only way to pass parameters.

With "traditional configuration code path" you mean using platform data
from the board file, correct? Having configuration options there is not
an argument to have the same option in the DT binding. DT should not
contain configuration, just HW description.

If pps-gpio would configure the capabilities correct, the userspace
could simply use time_pps_setparams to select whether it's interested in
assert or clear:
https://github.com/ago/pps-tools/blob/master/ppswatch.c#L77

> I don't think 2 lines of code is too high a price for consistency with
> past practice.
> 
> If you look at the overlays/README, you will see other driver
> configuration parameters that aren't strictly hardware configuration.
> E.g. "debug=n", "rotate=n", dma threshold...
> Note that the next release of the RPi kernel will require DT.
"overlays/README" is from the RPi downstream kernel, right?
The debug options seem to apply to driver which are specific to that
kernel or are only in stating and have no official DT bindings.
While there are some cases in the mainline kernel where configuration
options in the DT binding have slipped through review, we should not add
more.

> That said, I don't require capture-clear at the moment, so I'll leave
> further arguments on the merits for when I do - or someone else does.

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |