Periodic Output, Timestamped Input

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:

(quoted diff) +13 -0

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

(quoted diff) +4 -0

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

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.

Quoted 61 lines · Felipe Balbi

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:

(quoted diff) +13 -0

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

(quoted diff) +4 -0

@@ -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,
 };

Quoted 5 lines

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.

Me:

Quoted 10 lines

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;

Quoted 2 lines

        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?

Quoted 11 lines · Felipe Balbi

Me:

Quoted 10 lines

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;

Quoted 6 lines

Quoted 2 lines

        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

Quoted 11 lines · Felipe Balbi

Me:

Quoted 10 lines

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;

Quoted 6 lines

Quoted 2 lines

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

[Me]:

Quoted 3 lines

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:

Quoted 8 lines · Felipe Balbi

[Me]:

Quoted 3 lines

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.