Re: [RFC] Periodic Output, Timestamped Input

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

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