Re: [RFC] Periodic Output, Timestamped Input

[Felipe Balbi <felipe.balbi@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3704 bytes --]


Hi,

Linus Walleij <linus.walleij@linaro.org> writes:
> On Wed, Nov 29, 2017 at 2:56 PM, Felipe Balbi
> <felipe.balbi@linux.intel.com> wrote:
>> Me:
>>> For the other thing: timestamping of GPIO events, we already
>>> support timestamps for userspace GPIOs, but all it does is use
>>> the kernel time, see gpiolib.c:
>>>
>>> static irqreturn_t lineevent_irq_thread(int irq, void *p)
>>> {
>>>         struct lineevent_state *le = p;
>>>         struct gpioevent_data ge;
>>>         int ret, level;
>>>
>>>         ge.timestamp = ktime_get_real_ns();
>>>         level = gpiod_get_value_cansleep(le->desc);
>>
>> this is running as a thread with interrupts enabled, AFAICT. This means
>> this thread can be preempted at least on PREEMPT_RT kernels, so your
>> timestamp can be wrong, right?
>
> Yes, it can be off. What we should do to get i better is
> something like what I did in:
> drivers/iio/gyro/mpu3050-core.c
>
> Here I have both a hard and a soft IRQ handler (fast/slow if
> you like) and take the timestamp in the hard IRQ, then use
> it in the thread.
>
> This should be done identically in gpiolib to increase precision
> in the general case.
>
> I was thinking about it already when implementing it but it fell
> out of my mind. I'm putting in on my TODO. (CC to bartosz
> who might be interested, he's using these ABIs quite a bit.)

fair enough. In that case, it'll probably be easier to implement
HW-based timestamping with something like below:

modified   drivers/gpio/gpiolib.c
@@ -731,7 +731,11 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
 	struct gpioevent_data ge;
 	int ret, level;
 
-	ge.timestamp = ktime_get_real_ns();
+	if (le->desc->flags & FLAG_HAS_HW_TIMESTAMP)
+		ge.timestamp = gpio_get_hw_timestamp(le->desc);
+	else
+		ge.timestamp = ktime_get_real_ns();
+
 	level = gpiod_get_value_cansleep(le->desc);
 
 	if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE
modified   drivers/gpio/gpiolib.h
@@ -206,6 +206,7 @@ struct gpio_desc {
 #define FLAG_USED_AS_IRQ 9	/* GPIO is connected to an IRQ */
 #define FLAG_IS_HOGGED	11	/* GPIO is hogged */
 #define FLAG_SLEEP_MAY_LOSE_VALUE 12	/* GPIO may lose value in sleep */
+#define FLAG_HAS_HW_TIMESTAMP 13 /* GPIO has HW-based timestamping */
 
 	/* Connection label */
 	const char		*label;

We may even extract ktime_get_real_ns() to gpio_get_timestamp() and do
the branching in that function, like:

modified   drivers/gpio/gpiolib.c
@@ -731,7 +731,7 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
 	struct gpioevent_data ge;
 	int ret, level;
 
-	ge.timestamp = ktime_get_real_ns();
+	ge.timestamp = gpiod_get_timestamp(le->desc);
 	level = gpiod_get_value_cansleep(le->desc);
 
 	if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE
@@ -3155,6 +3155,14 @@ int gpiod_get_value_cansleep(const struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_get_value_cansleep);
 
+u64 gpiod_get_timestamp(const struct gpio_desc *desc)
+{
+	if (desc->flags & FLAG_HAS_HW_TIMESTAMP)
+		return gpiod_get_raw_timestamp(desc);
+	else
+		return ktime_get_real_ns();
+}
+
 /**
  * gpiod_get_raw_array_value_cansleep() - read raw values from an array of GPIOs
  * @array_size: number of elements in the descriptor / value arrays
modified   drivers/gpio/gpiolib.h
@@ -206,6 +206,7 @@ struct gpio_desc {
 #define FLAG_USED_AS_IRQ 9	/* GPIO is connected to an IRQ */
 #define FLAG_IS_HOGGED	11	/* GPIO is hogged */
 #define FLAG_SLEEP_MAY_LOSE_VALUE 12	/* GPIO may lose value in sleep */
+#define FLAG_HAS_HW_TIMESTAMP 13 /* GPIO has HW-based timestamping */
 
 	/* Connection label */
 	const char		*label;

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]