RE: Intel Timed-IO driver in IIO/Counter subsystem

["Hall, Christopher S" <christopher.s.hall@intel.com>]

Hi Kent, William, and Linus:

Thank you for your input.

Kent Gibson <warthog618@gmail.com> wrote on Monday, July 04, 2022 8:17 PM:
> To: Hall, Christopher S <christopher.s.hall@intel.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski <brgl@bgdev.pl>; Shevchenko, Andriy
> <andriy.shevchenko@intel.com>; Dipen Patel <dipenp@nvidia.com>; N, Pandith <pandith.n@intel.com>; D,
> Lakshmi Sowjanya <lakshmi.sowjanya.d@intel.com>; Lars-Peter Clausen <lars@metafoo.de>; linux-
> iio@vger.kernel.org; vilhelm.gray@gmail.com; jic23@kernel.org; Sangannavar, Mallikarjunappa
> <mallikarjunappa.sangannavar@intel.com>; T R, Thejesh Reddy <thejesh.reddy.t.r@intel.com>
> Subject: Re: Intel Timed-IO driver in IIO/Counter subsystem
> 
> On Thu, Jun 23, 2022 at 02:21:30PM +0200, Linus Walleij wrote:
> > On Sat, Jun 18, 2022 at 4:01 AM Hall, Christopher S
> > <christopher.s.hall@intel.com> wrote:
> > > Friday, June 17, 2022 4:40 AM Linus Walleij <linus.walleij@linaro.org> wrote:

> > > We want to be able to output a clock from 1 Hz (1 PPS) up to 1 KHz that is
> > > synchronized with the system clock.
> > >
> > > It is possible to represent the periodic output function as a PWM device,
> > > but the PWM subsystem output - without modification - is not aligned to
> > > any clock which breaks the timing application.
> >
> > Is it "just" a clock then? As in drivers/clk?
> >
> > It's of course annoying that the functionality of a certain hardware falls
> > between the subsystems so we end up using pieces of a subsystem in
> > another one, but there are several precedents, like network switch chips
> > and USB UART chips with GPIO inside them, or graphic chips with
> > clock dividers inside.
> >
> > > > If a "single event" is something
> > > > like pulling a GPIO line high/low at a specific (wall clock) time in the
> > > > future, it should probably be in the GPIO subsystem, like a triggered
> > > > GPIO event or so, that sounds a bit hard but certainly doable with some
> > > > thinking and tinkering.
> > >
> > > Earlier, we proposed a linereq_write() method in addition to the already
> > > existing linereq_read().
> > >
> > > https://lkml.org/lkml/2021/8/24/807
> >
> > This might be a good approach for this part of the hardware, as long
> > as we can make sure we get the userspace API abstract enough
> > that other hardware can make use of it and userspace does not
> > need to know what provides the timed output, just that there is
> > some hardware that does.
> >
> > The ABI in the patch looks a bit dangerous, what happens if
> > you set an event like that and something decides to shake the
> > line by setting the output while the scheduled event is pending?
> >
> > The direction seems sound however, but Bartosz and Kent need
> > to look at it in detail, their effort on the userspace ABI has been
> > tremendous.
> >
> 
> Extending the GPIO uAPI seems like a reasonable approach for timed
> sets, but as Linus mentioned, you need to consider the semantics of
> your operation for the general case.  What if the set time has already
> passed?  What happens with subsequent sets when one is already pending?
> Can they be cancelled?
> 
> My first thought was that you could extend the SET_VALUES ioctl but,
> while we have reserved space for future use in most of the ioctls it
> turns out we overlooked sets and gets, so you would be looking at a new
> ioctl.  And you need to keep in mind how the SET_VALUES ioctl would
> interact with it (Linus' point).

I think we worked around this in a previous patchset by disallowing
(return error) the 'set' method. The pin/pad is assigned (by mux
configured in BIOS) to either GPIO or Timed I/O logic and both cannot be
used simultaneously. There is not a set method in the Timed IO hardware
logic, but one could be simulated by reading the current TSC clock value
and writing that to the COMPV (trigger time) register. This necessarily
cancels any trigger in progress unless the COMPV value is cached in
software and rewritten after the level is set. Having said that, I
cannot think of a good reason to mix the two trigger methods. For
example, if timed IO is used to produce a PPS output it is not very
useful to insert an untimed edge.

If we implement the set method, I think we should not allow setting the
output and return the error if an edge is already scheduled. The 'ban'
is lifted after the timer expires or is canceled.

> I don't see the linereq_write() approach flying - an ioctl is more
> appropriate.

This is OK. I am not overly attached to the write() interface

I think we could do this with a two ioctls:

struct lineevent_trigger {
	clockid_t clkid; /* We may want to select another clock */
	struct timespec trigger;
} ltrig_spec;

ioctl(fd, GPIOHANDLE_SET_LINE_EVENT_TRIGGER_ENABLE_IOCTL, &ltrig_spec) /* Set trigger time */
ioctl(fd, GPIOHANDLE_SET_LINE_EVENT_TRIGGER_CANCEL_IOCTL, &ltrig_spec) /* Cancel a trigger */

I do not see a need for a querying trigger status. The software can set
a timer or poll with clock_gettime()

I think this covers a single event. Periodic output is more of a challenge.
As noted above, the PWM interface is not a great fit mainly because the
PWM pulse is not aligned to a clock. We can augment the interface above

struct lineevent_trigger_periodic {
	clockid_t clkid;
	struct timespec trigger;
	struct timespec period;
} ltrig_spec;

A period of 0 indicates a one-shot timer. This is consistent with the
timer_settime() API.

The trick here is that we can specify the nominal *TSC* output period, but
we need to adjust the period to align the output with a local clock - or
since this feature is likely to be used with TSN - a PTP network clock.

The hardware can count the number of events. Using this, we compute
the *actual* output period with respect to the system clock:

delta(system clock) / delta(event count)

We can nudge the output period to match whichever clock we are tracking
as long as we know its relation to the system clock.

The counter interface can be used find the deltas with the system/ART
timestamp extension suggested by William.

> Cheers,
> Kent.

Thanks,
Christopher