Re: [RFC PATCH v1 07/20] gpio: Add output event generation method to GPIOLIB and PMC Driver

Re: [RFC PATCH v1 07/20] gpio: Add output event generation method to GPIOLIB and PMC Driver

[Linus Walleij]

Hi Lakshmi,

On Tue, Aug 24, 2021 at 6:48 PM <lakshmi.sowjanya.d@intel.com> wrote:

> From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
>
> Intel Timed I/O hardware supports output scheduled in hardware. Enable
> this functionality using GPIOlib
>
> Adds GPIOlib generate_output() hook into the driver. The driver is
> supplied with a timestamp in terms of realtime system clock (the same
> used for input timestamping). The driver must know how to translate this
> into a timebase meaningful for the hardware.
>
> Adds userspace write() interface. Output can be selected using the line
> event create ioctl. The write() interface takes a single timestamp
> event request parameter. An output edge rising or falling is generated
> for each event request.
>
> The user application supplies a trigger time in terms of the realtime
> clock the driver converts this into the corresponding ART clock value
> that is used to 'arm' the output.
>
> Work around device quirk that doesn't allow the output to be explicitly
> set. Instead, count the output edges and insert an additional edge as
> needed to reset the output to zero.
>
> Co-developed-by: Christopher Hall <christopher.s.hall@intel.com>
> Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
> Signed-off-by: Tamal Saha <tamal.saha@intel.com>
> Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> Reviewed-by: Mark Gross <mgross@linux.intel.com>

So this is some street organ machine that generates sequences
with determined timing between positive and negative edges
right?

I can't see how this hardware is different from a PWM, or well
I do to some extent, you can control the period of several
subsequent waves, but that is really just an elaborate version
of PWM in my book.

It seems to me that this part of the functionality belongs in the
PWM subsystem which already has interfaces for similar
things, and you should probably extend PWM to handle
random waveforms rather than trying to shoehorn this
into the GPIO subsystem.

Yours,
Linus Walleij

Re: [RFC PATCH v1 15/20] pwm: Add capability for PWM Driver managed state

[Linus Walleij]

On Tue, Aug 24, 2021 at 6:48 PM <lakshmi.sowjanya.d@intel.com> wrote:

> From: Christopher Hall <christopher.s.hall@intel.com>
>
> Add additional flag that can be set by drivers to indicate that the
> driver will manage its own PWM state. When calling pwm_ops.apply the
> driver applies the requested state change to the pwm_device reconciling,
> if possible, any conflicting requests.
>
> Intel(R) Timed I/O devices support very limited PWM capabilities. The
> duty cycle must always be 50% of the period. When changing one parameter
> at a time through the sysfs interface, it isn't possible for the user or
> the PWM subsystem to maintain this relation.
>
> Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
> Signed-off-by: Tamal Saha <tamal.saha@intel.com>
> Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> Reviewed-by: Mark Gross <mgross@linux.intel.com>

These PWM changes clearly need to be reviewed by the PWM
subsystem maintainers, and I think all of the output generation
portions of this really need to go into the PWM subsystem.

Yours,
Linus Walleij

Re: [RFC PATCH v1 07/20] gpio: Add output event generation method to GPIOLIB and PMC Driver

[Uwe Kleine-König]

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

Hello,

On Thu, Sep 16, 2021 at 11:42:04PM +0200, Linus Walleij wrote:
> Hi Lakshmi,
> 
> On Tue, Aug 24, 2021 at 6:48 PM <lakshmi.sowjanya.d@intel.com> wrote:
> 
> > From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> >
> > Intel Timed I/O hardware supports output scheduled in hardware. Enable
> > this functionality using GPIOlib
> >
> > Adds GPIOlib generate_output() hook into the driver. The driver is
> > supplied with a timestamp in terms of realtime system clock (the same
> > used for input timestamping). The driver must know how to translate this
> > into a timebase meaningful for the hardware.
> >
> > Adds userspace write() interface. Output can be selected using the line
> > event create ioctl. The write() interface takes a single timestamp
> > event request parameter. An output edge rising or falling is generated
> > for each event request.
> >
> > The user application supplies a trigger time in terms of the realtime
> > clock the driver converts this into the corresponding ART clock value
> > that is used to 'arm' the output.
> >
> > Work around device quirk that doesn't allow the output to be explicitly
> > set. Instead, count the output edges and insert an additional edge as
> > needed to reset the output to zero.
> >
> > Co-developed-by: Christopher Hall <christopher.s.hall@intel.com>
> > Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
> > Signed-off-by: Tamal Saha <tamal.saha@intel.com>
> > Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> > Reviewed-by: Mark Gross <mgross@linux.intel.com>
> 
> So this is some street organ machine that generates sequences
> with determined timing between positive and negative edges
> right?
> 
> I can't see how this hardware is different from a PWM, or well
> I do to some extent, you can control the period of several
> subsequent waves, but that is really just an elaborate version
> of PWM in my book.

From looking in the patch I think this is more versatile than the PWM
framework abstracts. I wonder if there is a usecase for the
functionality that cannot be expressed using pwm_apply_state?!

I remember we had approaches before that implemented repeating patterns
(something like: active for 5ms, inactive for 10 ms, active for 30 ms,
inactive for 10 ms, repeat) and limiting the number of periods
(something like: .duty_cycle = 5ms, .period = 20ms, after 5 periods go
into inactive state). These were considered to be too special to be
abstracted in drivers/pwm.

> It seems to me that this part of the functionality belongs in the
> PWM subsystem which already has interfaces for similar
> things, and you should probably extend PWM to handle
> random waveforms rather than trying to shoehorn this
> into the GPIO subsystem.

I agree that GPIO is a worse candidate than PWM to abstract that. But
I'm not convinced (yet?) that it's a good idea to extend PWM
accordingly.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [RFC PATCH v1 07/20] gpio: Add output event generation method to GPIOLIB and PMC Driver

[Linus Walleij]

On Fri, Sep 17, 2021 at 9:27 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Thu, Sep 16, 2021 at 11:42:04PM +0200, Linus Walleij wrote:
> > On Tue, Aug 24, 2021 at 6:48 PM <lakshmi.sowjanya.d@intel.com> wrote:
> >
> > > From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> > >
> > > Intel Timed I/O hardware supports output scheduled in hardware. Enable
> > > this functionality using GPIOlib
> > >
> > > Adds GPIOlib generate_output() hook into the driver. The driver is
> > > supplied with a timestamp in terms of realtime system clock (the same
> > > used for input timestamping). The driver must know how to translate this
> > > into a timebase meaningful for the hardware.
> > >
> > > Adds userspace write() interface. Output can be selected using the line
> > > event create ioctl. The write() interface takes a single timestamp
> > > event request parameter. An output edge rising or falling is generated
> > > for each event request.
> > >
> > > The user application supplies a trigger time in terms of the realtime
> > > clock the driver converts this into the corresponding ART clock value
> > > that is used to 'arm' the output.
> > >
> > > Work around device quirk that doesn't allow the output to be explicitly
> > > set. Instead, count the output edges and insert an additional edge as
> > > needed to reset the output to zero.
> > >
> > > Co-developed-by: Christopher Hall <christopher.s.hall@intel.com>
> > > Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
> > > Signed-off-by: Tamal Saha <tamal.saha@intel.com>
> > > Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> > > Reviewed-by: Mark Gross <mgross@linux.intel.com>
> >
> > So this is some street organ machine that generates sequences
> > with determined timing between positive and negative edges
> > right?
> >
> > I can't see how this hardware is different from a PWM, or well
> > I do to some extent, you can control the period of several
> > subsequent waves, but that is really just an elaborate version
> > of PWM in my book.
>
> From looking in the patch I think this is more versatile than the PWM
> framework abstracts. I wonder if there is a usecase for the
> functionality that cannot be expressed using pwm_apply_state?!
>
> I remember we had approaches before that implemented repeating patterns
> (something like: active for 5ms, inactive for 10 ms, active for 30 ms,
> inactive for 10 ms, repeat) and limiting the number of periods
> (something like: .duty_cycle = 5ms, .period = 20ms, after 5 periods go
> into inactive state). These were considered to be too special to be
> abstracted in drivers/pwm.
>
> > It seems to me that this part of the functionality belongs in the
> > PWM subsystem which already has interfaces for similar
> > things, and you should probably extend PWM to handle
> > random waveforms rather than trying to shoehorn this
> > into the GPIO subsystem.
>
> I agree that GPIO is a worse candidate than PWM to abstract that. But
> I'm not convinced (yet?) that it's a good idea to extend PWM
> accordingly.

Yeah it is a bit unfortunate.

I think we need to fully understand the intended usecase before
we can deal with this: exactly what was this hardware constructed
to handle? Sound? Robotic stepper motors? It must be something
and apparently there are users.

Maybe even a new subsystem is needed, like a
drivers/gpio-patterns or drivers/stepper-motor or whatever this
is supposed to drive.

Yours,
Linus Walleij

Re: [RFC PATCH v1 07/20] gpio: Add output event generation method to GPIOLIB and PMC Driver

[Clemens Gruber]

On Sun, Sep 19, 2021 at 09:38:58PM +0200, Linus Walleij wrote:
> On Fri, Sep 17, 2021 at 9:27 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Thu, Sep 16, 2021 at 11:42:04PM +0200, Linus Walleij wrote:
> > > On Tue, Aug 24, 2021 at 6:48 PM <lakshmi.sowjanya.d@intel.com> wrote:
> > >
> > > > From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> > > >
> > > > Intel Timed I/O hardware supports output scheduled in hardware. Enable
> > > > this functionality using GPIOlib
> > > >
> > > > Adds GPIOlib generate_output() hook into the driver. The driver is
> > > > supplied with a timestamp in terms of realtime system clock (the same
> > > > used for input timestamping). The driver must know how to translate this
> > > > into a timebase meaningful for the hardware.
> > > >
> > > > Adds userspace write() interface. Output can be selected using the line
> > > > event create ioctl. The write() interface takes a single timestamp
> > > > event request parameter. An output edge rising or falling is generated
> > > > for each event request.
> > > >
> > > > The user application supplies a trigger time in terms of the realtime
> > > > clock the driver converts this into the corresponding ART clock value
> > > > that is used to 'arm' the output.
> > > >
> > > > Work around device quirk that doesn't allow the output to be explicitly
> > > > set. Instead, count the output edges and insert an additional edge as
> > > > needed to reset the output to zero.
> > > >
> > > > Co-developed-by: Christopher Hall <christopher.s.hall@intel.com>
> > > > Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
> > > > Signed-off-by: Tamal Saha <tamal.saha@intel.com>
> > > > Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> > > > Reviewed-by: Mark Gross <mgross@linux.intel.com>
> > >
> > > So this is some street organ machine that generates sequences
> > > with determined timing between positive and negative edges
> > > right?
> > >
> > > I can't see how this hardware is different from a PWM, or well
> > > I do to some extent, you can control the period of several
> > > subsequent waves, but that is really just an elaborate version
> > > of PWM in my book.
> >
> > From looking in the patch I think this is more versatile than the PWM
> > framework abstracts. I wonder if there is a usecase for the
> > functionality that cannot be expressed using pwm_apply_state?!
> >
> > I remember we had approaches before that implemented repeating patterns
> > (something like: active for 5ms, inactive for 10 ms, active for 30 ms,
> > inactive for 10 ms, repeat) and limiting the number of periods
> > (something like: .duty_cycle = 5ms, .period = 20ms, after 5 periods go
> > into inactive state). These were considered to be too special to be
> > abstracted in drivers/pwm.
> >
> > > It seems to me that this part of the functionality belongs in the
> > > PWM subsystem which already has interfaces for similar
> > > things, and you should probably extend PWM to handle
> > > random waveforms rather than trying to shoehorn this
> > > into the GPIO subsystem.
> >
> > I agree that GPIO is a worse candidate than PWM to abstract that. But
> > I'm not convinced (yet?) that it's a good idea to extend PWM
> > accordingly.
> 
> Yeah it is a bit unfortunate.
> 
> I think we need to fully understand the intended usecase before
> we can deal with this: exactly what was this hardware constructed
> to handle? Sound? Robotic stepper motors? It must be something
> and apparently there are users.
> 
> Maybe even a new subsystem is needed, like a
> drivers/gpio-patterns or drivers/stepper-motor or whatever this
> is supposed to drive.

This would be interesting. Maybe even more abstract, not just supporting
GPIO patterns but also PWM patterns.

E.g. Set gpiochip1 line 2 to 1, wait 5ms, set it to 0
Or set pwmchip1 pwm 2 to 100%, wait 250ms, set it back to 50% duty cycle

This subsystem could then implement the patterns with hrtimers and be
usable with every GPIO or PWM device supported in Linux, and for
special hardware like the Intel Timed I/O, it could configure it to
output the pattern itself.

One usecase besides stepper motors and Robotics would be solenoid
valves: You often have different sequences for opening, closing and
maintenance. E.g. for liquid valves, especially if the liquid is
viscuous, you have to first use 100% duty cycle PWM for e.g. 250ms to
get it open and then dial back to 50% to keep it open without
overheating it.

Of course this can be done in userspace.. but it may also be useful to
have some kind of pattern generator in the kernel. What do you think?

Clemens

Re: [RFC PATCH v1 07/20] gpio: Add output event generation method to GPIOLIB and PMC Driver

[Uwe Kleine-König]

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

On Sun, Sep 19, 2021 at 11:21:22PM +0200, Clemens Gruber wrote:
> On Sun, Sep 19, 2021 at 09:38:58PM +0200, Linus Walleij wrote:
> > On Fri, Sep 17, 2021 at 9:27 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Thu, Sep 16, 2021 at 11:42:04PM +0200, Linus Walleij wrote:
> > > > On Tue, Aug 24, 2021 at 6:48 PM <lakshmi.sowjanya.d@intel.com> wrote:
> > > >
> > > > > From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> > > > >
> > > > > Intel Timed I/O hardware supports output scheduled in hardware. Enable
> > > > > this functionality using GPIOlib
> > > > >
> > > > > Adds GPIOlib generate_output() hook into the driver. The driver is
> > > > > supplied with a timestamp in terms of realtime system clock (the same
> > > > > used for input timestamping). The driver must know how to translate this
> > > > > into a timebase meaningful for the hardware.
> > > > >
> > > > > Adds userspace write() interface. Output can be selected using the line
> > > > > event create ioctl. The write() interface takes a single timestamp
> > > > > event request parameter. An output edge rising or falling is generated
> > > > > for each event request.
> > > > >
> > > > > The user application supplies a trigger time in terms of the realtime
> > > > > clock the driver converts this into the corresponding ART clock value
> > > > > that is used to 'arm' the output.
> > > > >
> > > > > Work around device quirk that doesn't allow the output to be explicitly
> > > > > set. Instead, count the output edges and insert an additional edge as
> > > > > needed to reset the output to zero.
> > > > >
> > > > > Co-developed-by: Christopher Hall <christopher.s.hall@intel.com>
> > > > > Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
> > > > > Signed-off-by: Tamal Saha <tamal.saha@intel.com>
> > > > > Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> > > > > Reviewed-by: Mark Gross <mgross@linux.intel.com>
> > > >
> > > > So this is some street organ machine that generates sequences
> > > > with determined timing between positive and negative edges
> > > > right?
> > > >
> > > > I can't see how this hardware is different from a PWM, or well
> > > > I do to some extent, you can control the period of several
> > > > subsequent waves, but that is really just an elaborate version
> > > > of PWM in my book.
> > >
> > > From looking in the patch I think this is more versatile than the PWM
> > > framework abstracts. I wonder if there is a usecase for the
> > > functionality that cannot be expressed using pwm_apply_state?!
> > >
> > > I remember we had approaches before that implemented repeating patterns
> > > (something like: active for 5ms, inactive for 10 ms, active for 30 ms,
> > > inactive for 10 ms, repeat) and limiting the number of periods
> > > (something like: .duty_cycle = 5ms, .period = 20ms, after 5 periods go
> > > into inactive state). These were considered to be too special to be
> > > abstracted in drivers/pwm.
> > >
> > > > It seems to me that this part of the functionality belongs in the
> > > > PWM subsystem which already has interfaces for similar
> > > > things, and you should probably extend PWM to handle
> > > > random waveforms rather than trying to shoehorn this
> > > > into the GPIO subsystem.
> > >
> > > I agree that GPIO is a worse candidate than PWM to abstract that. But
> > > I'm not convinced (yet?) that it's a good idea to extend PWM
> > > accordingly.
> > 
> > Yeah it is a bit unfortunate.
> > 
> > I think we need to fully understand the intended usecase before
> > we can deal with this: exactly what was this hardware constructed
> > to handle? Sound? Robotic stepper motors? It must be something
> > and apparently there are users.
> > 
> > Maybe even a new subsystem is needed, like a
> > drivers/gpio-patterns or drivers/stepper-motor or whatever this
> > is supposed to drive.
> 
> This would be interesting. Maybe even more abstract, not just supporting
> GPIO patterns but also PWM patterns.
> 
> E.g. Set gpiochip1 line 2 to 1, wait 5ms, set it to 0
> Or set pwmchip1 pwm 2 to 100%, wait 250ms, set it back to 50% duty cycle

Note that adding support to drive PWMs in this "GPIO command sequence"
framework would not increase its expressiveness assuming this framework
has loop support. That's because the sequence "set pwmchip1 pwm 2 to
100% (with a period of X), wait 250ms, set it back to 50% duty cycle
with a period of Y" can be expressed using a GPIO as:

	set GPIO to active
	wait 250 ms + Y/2
	while True:
	    toggle GPIO
	    wait Y/2

That's because this framework could provide a PWM from a GPIO.

(Also note that the original command sequence has some problems. That's
because (depending on the PWM in use and X) doing

	pwm_apply_state(mypwm, &(struct pwm_state){ .period = X, .duty_cycle = X, .enabled = 1 });

and then 250ms later

	pwm_apply_state(mypwm, &(struct pwm_state){ .period = Y, .duty_cycle = Y / 2, .enabled = 1 });

might give you an initially active phase that is considerably longer
than 250 ms + Y/2 because the PWM cannot implement .period = X exactly
and completes the period that is currently running at time 250 ms.

> This subsystem could then implement the patterns with hrtimers and be
> usable with every GPIO or PWM device supported in Linux, and for
> special hardware like the Intel Timed I/O, it could configure it to
> output the pattern itself.
> 
> One usecase besides stepper motors and Robotics would be solenoid
> valves: You often have different sequences for opening, closing and
> maintenance. E.g. for liquid valves, especially if the liquid is
> viscuous, you have to first use 100% duty cycle PWM for e.g. 250ms to
> get it open and then dial back to 50% to keep it open without
> overheating it.
> 
> Of course this can be done in userspace.. but it may also be useful to
> have some kind of pattern generator in the kernel. What do you think?

Without hearing the usecases of the original idea my feeling is:
Implement it in userspace, than the sequences can even contain network
interaction and access to SPI eeproms.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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