[ntp:hackers] u-blox reference clock driver

Philip Prindeville philipp_subx at redfish-solutions.com
Mon Aug 13 04:03:29 UTC 2018


> On Aug 10, 2018, at 5:03 AM, Lukas Senger <lukas at fridolin.com> wrote:
> 
> Hi,
> 
> I've been writing a reference driver for u-blox GPS receivers as part
> of my master's thesis and thought ntp hackers might be interested in it.
> Additionally to normal PPS operation this driver can make use of the
> u-blox's "Timemark" functionality.
> 
> It does this by enabling the PPSAPI echo, which generates an echo pulse
> right after receiving a PPS pulse. This echo is connected to EXTINT0 on
> the u-blox, where it is timestamped. So now I have the PPS timestamp as
> a local time and the timemark as the receiver time to calculate an
> offset. The cool thing about this is, that the offset does not include
> the local interrupt latency anymore, which leads to less jitter.
> 
> Here's the driver:
> https://gitlab.cs.fau.de/luksen/ntp-ubx/blob/ubx/ntpd/refclock_ubx.c
> 
> The echo/timemark feature requires kernel patches on Linux (with gpio
> pins) and FreeBSD (parallel port). It's probably easiest to test on a
> Raspberry Pi with the Linux patch and via GPIO pins.
> 
> Linux patch: https://gitlab.cs.fau.de/snippets/28
> FreeBSD patch: https://gitlab.cs.fau.de/snippets/29
> 
> I originally tried to send this message some time ago but it seems like
> there were some problems with the mailing list. By now the thesis is
> finished and you can find it here:
> https://wwwcip.informatik.uni-erlangen.de/~in18otin/thesis.pdf
> 
> --Lukas



Comments inline as [pap]...

diff --git a/arch/arm/boot/dts/overlays/pps-gpio-overlay.dts b/arch/arm/boot/dts/overlays/pps-gpio-overlay.dts
index 9ee4bdfa6167..bc299f581868 100644
--- a/arch/arm/boot/dts/overlays/pps-gpio-overlay.dts
+++ b/arch/arm/boot/dts/overlays/pps-gpio-overlay.dts
@@ -11,6 +11,7 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&pps_pins>;
 				gpios = <&gpio 18 0>;
+				echo-gpios = <&gpio 17 0>;

[pap] Guessing this only works on platforms with device tree support?

 				status = "okay";
 			};
 		};
@@ -20,9 +21,9 @@
 		target = <&gpio>;
 		__overlay__ {
 			pps_pins: pps_pins at 12 {
-				brcm,pins =     <18>;
-				brcm,function = <0>;    // in
-				brcm,pull =     <0>;    // off
+				brcm,pins =     <18 17>;
+				brcm,function = <0 1>;    // in  out
+				brcm,pull =     <0 0>;    // off off
 			};
 		};
 	};
@@ -32,6 +33,8 @@
 			  <&pps>,"reg:0",
 			  <&pps_pins>,"brcm,pins:0",
 			  <&pps_pins>,"reg:0";
+		echopin = <&pps>,"echo-gpios:4",
+			  <&pps_pins>,"brcm,pins:4";
 		assert_falling_edge = <&pps>,"assert-falling-edge?";
 	};
 };
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 333ad7d5b45b..08f2ebdab5e5 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -35,6 +35,8 @@
 #include <linux/list.h>
 #include <linux/of_device.h>
 #include <linux/of_gpio.h>
+#include <linux/delay.h>
+

[pap] Don’t need the extra whitespace (newline).
 
 /* Info for each registered platform device */
 struct pps_gpio_device_data {
@@ -44,6 +46,7 @@ struct pps_gpio_device_data {
 	bool assert_falling_edge;
 	bool capture_clear;
 	unsigned int gpio_pin;
+	unsigned int echo_pin;
 };
 
 /*
@@ -64,15 +67,46 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
 	rising_edge = gpio_get_value(info->gpio_pin);
 	if ((rising_edge && !info->assert_falling_edge) ||
 			(!rising_edge && info->assert_falling_edge))
-		pps_event(info->pps, &ts, PPS_CAPTUREASSERT, NULL);
+		pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);
 	else if (info->capture_clear &&
 			((rising_edge && info->assert_falling_edge) ||
 			 (!rising_edge && !info->assert_falling_edge)))
-		pps_event(info->pps, &ts, PPS_CAPTURECLEAR, NULL);
+		pps_event(info->pps, &ts, PPS_CAPTURECLEAR, data);
+
+	if (info->pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR))
+		return IRQ_WAKE_THREAD;
+	return IRQ_HANDLED;
+}
+
+static void pps_gpio_echo(struct pps_device *pps, int event, void *data)
+{
+	const struct pps_gpio_device_data *info;
+
+	info = data;

[pap] Please combine the declaration and initialization.

+
+	if (event == PPS_CAPTUREASSERT && (pps->params.mode & PPS_ECHOASSERT))
+		gpio_set_value(info->echo_pin, 1);
+	else if (event == PPS_CAPTURECLEAR && (pps->params.mode & PPS_ECHOCLEAR))

[pap] Since both if's have the same then, please combine them as a single if with ||

+		gpio_set_value(info->echo_pin, 1);
+}
+
+
+/* If we sent an echo pulse, we set the output pin back to low. This results in
+ * an echo pulse of roughly 100ms length.
+ */
+static irqreturn_t pps_gpio_irq_threaded(int irq, void *data)
+{
+	const struct pps_gpio_device_data *info;
+
+	info = data;

[pap] Please combine the declaration and initialization.

+
+	msleep(100);

[pap] That's a long sleep... why so long?

+	gpio_set_value(info->echo_pin, 0);
 
 	return IRQ_HANDLED;
 }
 
+

[pap] Please remove any whitespace-only changes.

 static unsigned long
 get_irqf_trigger_flags(const struct pps_gpio_device_data *data)
 {
@@ -104,17 +138,24 @@ static int pps_gpio_probe(struct platform_device *pdev)
 
 	if (pdata) {
 		data->gpio_pin = pdata->gpio_pin;
+		data->echo_pin = pdata->echo_pin;
 		gpio_label = pdata->gpio_label;
 
 		data->assert_falling_edge = pdata->assert_falling_edge;
 		data->capture_clear = pdata->capture_clear;
 	} else {
-		ret = of_get_gpio(np, 0);
+		ret = of_get_named_gpio(np, "gpios", 0);
 		if (ret < 0) {
 			dev_err(&pdev->dev, "failed to get GPIO from device tree\n");
 			return ret;
 		}
 		data->gpio_pin = ret;
+		ret = of_get_named_gpio(np, "echo-gpios", 0);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to get second GPIO from device tree\n");
+			return ret;
+		}
+		data->echo_pin = ret;
 		gpio_label = PPS_GPIO_NAME;
 
 		if (of_get_property(np, "assert-falling-edge", NULL))
@@ -131,7 +172,20 @@ static int pps_gpio_probe(struct platform_device *pdev)
 
 	ret = gpio_direction_input(data->gpio_pin);
 	if (ret) {
-		dev_err(&pdev->dev, "failed to set pin direction\n");
+		dev_err(&pdev->dev, "failed to set pin as input\n");
+		return -EINVAL;
+	}
+
+	ret = devm_gpio_request(&pdev->dev, data->echo_pin, gpio_label);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request GPIO %u\n",
+			data->echo_pin);
+		return ret;
+	}
+
+	ret = gpio_direction_output(data->echo_pin, 0);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to set pin as output\n");
 		return -EINVAL;
 	}
 
@@ -152,6 +206,7 @@ static int pps_gpio_probe(struct platform_device *pdev)
 	data->info.owner = THIS_MODULE;
 	snprintf(data->info.name, PPS_MAX_NAME_LEN - 1, "%s.%d",
 		 pdev->name, pdev->id);
+	data->info.echo = pps_gpio_echo;
 
 	/* register PPS source */
 	pps_default_params = PPS_CAPTUREASSERT | PPS_OFFSETASSERT;
@@ -165,7 +220,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
 	}
 
 	/* register IRQ interrupt handler */
-	ret = devm_request_irq(&pdev->dev, data->irq, pps_gpio_irq_handler,
+	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
+			pps_gpio_irq_handler, pps_gpio_irq_threaded,
 			get_irqf_trigger_flags(data), data->info.name, data);
 	if (ret) {
 		pps_unregister_source(data->pps);
diff --git a/include/linux/pps-gpio.h b/include/linux/pps-gpio.h
index 0035abe41b9a..055edd43012a 100644
--- a/include/linux/pps-gpio.h
+++ b/include/linux/pps-gpio.h
@@ -26,6 +26,7 @@ struct pps_gpio_platform_data {
 	bool assert_falling_edge;
 	bool capture_clear;
 	unsigned int gpio_pin;
+	unsigned int echo_pin;
 	const char *gpio_label;
 };


More information about the hackers mailing list