Re: [RFC] Periodic Output, Timestamped Input

Re: [RFC] Periodic Output, Timestamped Input

[Linus Walleij]

Hi Felipe,

this is nice and interesting technology!

I am quoting the whole mail over to linux-pwm and Thierry
as well as  linux-iio and Jon Cameron for consideration. My comments
at the end.

On Thu, Nov 16, 2017 at 10:29 AM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>
> Hi Linus,
>
> Before I put a lot of code down, I wanted to get a feeling from you as
> to how you'd like two new features to be implemented.
>
> In a future platform we may have two new features in our GPIO
> controller which allows for periodic assertion of GPIO Output
> (controlled by HW, we just program the interval) and Timestamping of
> GPIO Input events.
>
> I was thinking that we could use pin config for that. Something along
> the lines of:
>
> @@ -700,6 +700,12 @@ static int intel_config_set_debounce(struct intel_pinctrl *pctrl, unsigned pin,
>         return ret;
>  }
>
> +static int intel_config_set_output_periodic(struct intel_pinctrl *pctrl,
> +                                           unsigned pin, unsigned period_ms)
> +{
> +       return 0;
> +}
> +
>  static int intel_config_set(struct pinctrl_dev *pctldev, unsigned pin,
>                           unsigned long *configs, unsigned nconfigs)
>  {
> @@ -726,6 +732,13 @@ static int intel_config_set(struct pinctrl_dev *pctldev, unsigned pin,
>                                 return ret;
>                         break;
>
> +               case PIN_CONFIG_OUTPUT_PERIODIC:
> +                       ret = intel_config_set_output_periodic(pctrl, pin,
> +                               pinconf_to_config_argument(configs[i]));
> +                       if (ret)
> +                               return ret;
> +                       break;
> +
>                 default:
>                         return -ENOTSUPP;
>                 }
> modified   include/linux/pinctrl/pinconf-generic.h
> @@ -90,6 +90,9 @@
>   * @PIN_CONFIG_SLEW_RATE: if the pin can select slew rate, the argument to
>   *     this parameter (on a custom format) tells the driver which alternative
>   *     slew rate to use.
> + * @PIN_CONFIG_OUTPUT_PERIODIC: this will configure the pin as an
> + *     output periodic toggle. The argument is period in
> + *     microseconds.
>   * @PIN_CONFIG_END: this is the last enumerator for pin configurations, if
>   *     you need to pass in custom configurations to the pin controller, use
>   *     PIN_CONFIG_END+1 as the base offset.
> @@ -117,6 +120,7 @@ enum pin_config_param {
>         PIN_CONFIG_POWER_SOURCE,
>         PIN_CONFIG_SLEEP_HARDWARE_STATE,
>         PIN_CONFIG_SLEW_RATE,
> +       PIN_CONFIG_OUTPUT_PERIODIC,
>         PIN_CONFIG_END = 0x7F,
>         PIN_CONFIG_MAX = 0xFF,
>  };
>
> As for timestamp of input, we would likely request the input as an IRQ
> and use the IRQ to read whatever register, wherever it may be.
>
> Do you have any comments about this? Should I go ahead with current
> assumptions?

So the first thing: periodic assetion of a GPIO, sounds awfully lot
like PWM does it not? I guess the output is a square wave?

While it is possible to model things like this as pin config, have you
considered just adding a PWM interface and present it to the kernel
as a PWM (possibly with a separate resource, in DT we use &phandle
but in ACPI I guess you Intel people have something else for PWM)?
If need be we can ask the ACPI people.

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);

        if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE
            && le->eflags & GPIOEVENT_REQUEST_FALLING_EDGE) {
                if (level)
                        /* Emit low-to-high event */
                        ge.id = GPIOEVENT_EVENT_RISING_EDGE;
                else
                        /* Emit high-to-low event */
                        ge.id = GPIOEVENT_EVENT_FALLING_EDGE;
        } else if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE && level) {
                /* Emit low-to-high event */
                ge.id = GPIOEVENT_EVENT_RISING_EDGE;
        } else if (le->eflags & GPIOEVENT_REQUEST_FALLING_EDGE && !level) {
                /* Emit high-to-low event */
                ge.id = GPIOEVENT_EVENT_FALLING_EDGE;
        } else {
                return IRQ_NONE;
        }

        ret = kfifo_put(&le->events, ge);
        if (ret != 0)
                wake_up_poll(&le->wait, POLLIN);

        return IRQ_HANDLED;
}

IIO already has a variety of timestamps to use, see
drivers/iio/industrialio-event.c.

What is on my TODO is to unify GPIO event timestamping with that
of IIO.

If there is further a *hardware* timestamp, that would be yet another
alternative timestamp, so we should first extend to use the different
timestamps supported by IIO and then add "hardware-native"
timestamping to the mix IMO. Unless there are other ideas.

Lars-Peter Clausen may have a better idea of what to do here, he
has run a few complex use cases like GNUradio (IIRC) using
timestamping. Also I think sensorfusion-type scenarios use these
timestamps quite a lot.

Yours,
Linus Walleij

Re: [RFC] Periodic Output, Timestamped Input

[Felipe Balbi]

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


Hi,

Linus Walleij <linus.walleij@linaro.org> writes:
> Hi Felipe,
>
> this is nice and interesting technology!
>
> I am quoting the whole mail over to linux-pwm and Thierry
> as well as  linux-iio and Jon Cameron for consideration. My comments
> at the end.
>
> On Thu, Nov 16, 2017 at 10:29 AM, Felipe Balbi
> <felipe.balbi@linux.intel.com> wrote:
>>
>> Hi Linus,
>>
>> Before I put a lot of code down, I wanted to get a feeling from you as
>> to how you'd like two new features to be implemented.
>>
>> In a future platform we may have two new features in our GPIO
>> controller which allows for periodic assertion of GPIO Output
>> (controlled by HW, we just program the interval) and Timestamping of
>> GPIO Input events.
>>
>> I was thinking that we could use pin config for that. Something along
>> the lines of:
>>
>> @@ -700,6 +700,12 @@ static int intel_config_set_debounce(struct intel_pinctrl *pctrl, unsigned pin,
>>         return ret;
>>  }
>>
>> +static int intel_config_set_output_periodic(struct intel_pinctrl *pctrl,
>> +                                           unsigned pin, unsigned period_ms)
>> +{
>> +       return 0;
>> +}
>> +
>>  static int intel_config_set(struct pinctrl_dev *pctldev, unsigned pin,
>>                           unsigned long *configs, unsigned nconfigs)
>>  {
>> @@ -726,6 +732,13 @@ static int intel_config_set(struct pinctrl_dev *pctldev, unsigned pin,
>>                                 return ret;
>>                         break;
>>
>> +               case PIN_CONFIG_OUTPUT_PERIODIC:
>> +                       ret = intel_config_set_output_periodic(pctrl, pin,
>> +                               pinconf_to_config_argument(configs[i]));
>> +                       if (ret)
>> +                               return ret;
>> +                       break;
>> +
>>                 default:
>>                         return -ENOTSUPP;
>>                 }
>> modified   include/linux/pinctrl/pinconf-generic.h
>> @@ -90,6 +90,9 @@
>>   * @PIN_CONFIG_SLEW_RATE: if the pin can select slew rate, the argument to
>>   *     this parameter (on a custom format) tells the driver which alternative
>>   *     slew rate to use.
>> + * @PIN_CONFIG_OUTPUT_PERIODIC: this will configure the pin as an
>> + *     output periodic toggle. The argument is period in
>> + *     microseconds.
>>   * @PIN_CONFIG_END: this is the last enumerator for pin configurations, if
>>   *     you need to pass in custom configurations to the pin controller, use
>>   *     PIN_CONFIG_END+1 as the base offset.
>> @@ -117,6 +120,7 @@ enum pin_config_param {
>>         PIN_CONFIG_POWER_SOURCE,
>>         PIN_CONFIG_SLEEP_HARDWARE_STATE,
>>         PIN_CONFIG_SLEW_RATE,
>> +       PIN_CONFIG_OUTPUT_PERIODIC,
>>         PIN_CONFIG_END = 0x7F,
>>         PIN_CONFIG_MAX = 0xFF,
>>  };
>>
>> As for timestamp of input, we would likely request the input as an IRQ
>> and use the IRQ to read whatever register, wherever it may be.
>>
>> Do you have any comments about this? Should I go ahead with current
>> assumptions?
>
> So the first thing: periodic assetion of a GPIO, sounds awfully lot
> like PWM does it not? I guess the output is a square wave?

Well, possibly. But that's not the idea of the feature. The idea is to
synchronize time. Say you program GPIO to fire at every 1 ms, a remote
processor/accelerator/etc can synchronize its own time to this 1 ms
tick.

That's at least my understanding.

> While it is possible to model things like this as pin config, have you
> considered just adding a PWM interface and present it to the kernel
> as a PWM (possibly with a separate resource, in DT we use &phandle
> but in ACPI I guess you Intel people have something else for PWM)?
> If need be we can ask the ACPI people.

Sure, however the same feature is also used for timestampped input :-)

Say another processor is the one asserting a GPIO every 1 ms. I can
sample it as quickly as possible, but we're bound to face SW
overhead. When the timestamp is latched, then the overhead can be
accounted for.

> 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?

>         if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE
>             && le->eflags & GPIOEVENT_REQUEST_FALLING_EDGE) {
>                 if (level)
>                         /* Emit low-to-high event */
>                         ge.id = GPIOEVENT_EVENT_RISING_EDGE;
>                 else
>                         /* Emit high-to-low event */
>                         ge.id = GPIOEVENT_EVENT_FALLING_EDGE;
>         } else if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE && level) {
>                 /* Emit low-to-high event */
>                 ge.id = GPIOEVENT_EVENT_RISING_EDGE;
>         } else if (le->eflags & GPIOEVENT_REQUEST_FALLING_EDGE && !level) {
>                 /* Emit high-to-low event */
>                 ge.id = GPIOEVENT_EVENT_FALLING_EDGE;
>         } else {
>                 return IRQ_NONE;
>         }
>
>         ret = kfifo_put(&le->events, ge);
>         if (ret != 0)
>                 wake_up_poll(&le->wait, POLLIN);
>
>         return IRQ_HANDLED;
> }
>
> IIO already has a variety of timestamps to use, see
> drivers/iio/industrialio-event.c.
>
> What is on my TODO is to unify GPIO event timestamping with that
> of IIO.
>
> If there is further a *hardware* timestamp, that would be yet another

yes, it's done by a high resolution timer

> alternative timestamp, so we should first extend to use the different
> timestamps supported by IIO and then add "hardware-native"
> timestamping to the mix IMO. Unless there are other ideas.
>
> Lars-Peter Clausen may have a better idea of what to do here, he
> has run a few complex use cases like GNUradio (IIRC) using
> timestamping. Also I think sensorfusion-type scenarios use these
> timestamps quite a lot.

I see... I wonder if the SW timestamping is suffcient here. Has anybody
done any sort of jitter analysis with heavy loaded CPU for this feature?

cheers

-- 
balbi

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

Re: [RFC] Periodic Output, Timestamped Input

[Linus Walleij]

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.)

Yours,
Linus Walleij

Re: [RFC] Periodic Output, Timestamped Input

[Jonathan Cameron]

On Wed, 29 Nov 2017 23:54:32 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> 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.

I did timing tests for both a long time back but I doubt things have
changed much.  To do fusion you ideally smooth the result on the basis
you are fairly sure the internal clock is better than even then jitter
we get in an interrupt.

No idea where the absolute numbers went though I'm afraid.
Perhaps Gregor can provide more info?

Not sure who else is doing sensor fusion currently...

> 
> 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.)
> 
> Yours,
> Linus Walleij

Re: [RFC] Periodic Output, Timestamped Input

[Felipe Balbi]

[-- 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 --]

Re: [RFC] Periodic Output, Timestamped Input

[Linus Walleij]

On Tue, Dec 5, 2017 at 10:20 AM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
> [Me]:

>> 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:

Indeed.

IIO has a per-device sysfs file for setting up
the POSIX timestamping named current_timestamp_clock.
This was added by Gregor Boire in commit
bc2b7dab629a

The supported timestamps are:
realtime
monotonic
monotonic_raw
realtime_coarse
monotonic_coarse
boottime
tai

I could really need some help here... :/

It feels like "monotonic" could very well be what you're
looking for but I don't know the POSIX definition of these.
Maybe we need to add one more for "device specific"?

What I want to do is factor most of that commit out into
lib/ and reuse for timestamping in GPIO so we can
have coherent timestamps across GPIO and IIO.

OK OK O need to get to it and stop talking...

I guess it should be set per-gpiochip rather than
per-gpio-desc though, but that makes most sense anyways.

Yours,
Linus Walleij

Re: [RFC] Periodic Output, Timestamped Input

[Felipe Balbi]

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


Hi,

Linus Walleij <linus.walleij@linaro.org> writes:
> On Tue, Dec 5, 2017 at 10:20 AM, Felipe Balbi
> <felipe.balbi@linux.intel.com> wrote:
>> [Me]:
>
>>> 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:
>
> Indeed.
>
> IIO has a per-device sysfs file for setting up
> the POSIX timestamping named current_timestamp_clock.
> This was added by Gregor Boire in commit
> bc2b7dab629a
>
> The supported timestamps are:
> realtime
> monotonic
> monotonic_raw
> realtime_coarse
> monotonic_coarse
> boottime
> tai
>
> I could really need some help here... :/
>
> It feels like "monotonic" could very well be what you're
> looking for but I don't know the POSIX definition of these.
> Maybe we need to add one more for "device specific"?
>
> What I want to do is factor most of that commit out into
> lib/ and reuse for timestamping in GPIO so we can
> have coherent timestamps across GPIO and IIO.
>
> OK OK O need to get to it and stop talking...
>
> I guess it should be set per-gpiochip rather than
> per-gpio-desc though, but that makes most sense anyways.

Well, it may be that not every pin on a gpiochip supports HW-backed
timestamping. Just speculating here, that's why I made it per-gpio_desc
on the example.

-- 
balbi

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