| Message ID | 20211017013343.3385923-5-david@lechnology.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | counter: ti-eqep: implement features for speed measurement | expand |
On Sat, 16 Oct 2021 20:33:39 -0500 David Lechner <david@lechnology.com> wrote: > This documents new unit timer sysfs attributes for the counter > subsystem. > > Signed-off-by: David Lechner <david@lechnology.com> The ti-eqep.c change needs moving to patch 1 where the typo was introduced. > --- > Documentation/ABI/testing/sysfs-bus-counter | 24 +++++++++++++++++++++ > drivers/counter/ti-eqep.c | 2 +- > 2 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter > index 06c2b3e27e0b..37d960a8cb1b 100644 > --- a/Documentation/ABI/testing/sysfs-bus-counter > +++ b/Documentation/ABI/testing/sysfs-bus-counter > @@ -218,6 +218,9 @@ What: /sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id > What: /sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id > What: /sys/bus/counter/devices/counterX/signalY/index_polarity_component_id > What: /sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id > +What: /sys/bus/counter/devices/unit_timer_enable_component_id > +What: /sys/bus/counter/devices/unit_timer_period_component_id > +What: /sys/bus/counter/devices/unit_timer_time_component_id > KernelVersion: 5.16 > Contact: linux-iio@vger.kernel.org > Description: > @@ -345,3 +348,24 @@ Description: > via index_polarity. The index function (as enabled via > preset_enable) is performed synchronously with the > quadrature clock on the active level of the index input. > + > +What: /sys/bus/counter/devices/unit_timer_enable > +KernelVersion: 5.16 > +Contact: linux-iio@vger.kernel.org > +Description: > + Read/write attribute that starts or stops the unit timer. Valid > + values are boolean. > + > +What: /sys/bus/counter/devices/unit_timer_period > +KernelVersion: 5.16 > +Contact: linux-iio@vger.kernel.org > +Description: > + Read/write attribute that selects the unit timer timeout in > + nanoseconds. > + > +What: /sys/bus/counter/devices/unit_timer_time > +KernelVersion: 5.16 > +Contact: linux-iio@vger.kernel.org > +Description: > + Read/write attribute that indicates the current time of the > + unit timer in nanoseconds. > diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c > index a4a5a4486329..1ba7f3c7cb7e 100644 > --- a/drivers/counter/ti-eqep.c > +++ b/drivers/counter/ti-eqep.c > @@ -680,7 +680,7 @@ static int ti_eqep_probe(struct platform_device *pdev) > pm_runtime_get_sync(dev); > > /* > - * We can end up with an interupt infinite loop (interrupts triggered > + * We can end up with an interrupt infinite loop (interrupts triggered This change should be in a separate patch. > * as soon as they are cleared) if we leave these at the default value > * of 0 and events are enabled. > */
On Sat, Oct 16, 2021 at 08:33:39PM -0500, David Lechner wrote: > This documents new unit timer sysfs attributes for the counter > subsystem. > > Signed-off-by: David Lechner <david@lechnology.com> Hello David, The unit timer is effectively a Count in its own right, so instead of introducing new sysfs attributes you can just implement it as another Count in the driver. Count 0 is "QPOSCNT", so set the name of this new Count 1 as "Unit Timer" (or the datasheet naming if more apt) to differentiate the Counts. You can then provide the "unit_timer_enable", "unit_timer_period", and "unit_timer_time" functionalities as respective Count 1 extensions ("enable" and "period") and Count 1 "count". If you believe it appropriate, you can provide the raw timer ticks via the Count 1 "count" while a nanoseconds interface is provided via a Count 1 extension "timeout" (or something similar). William Breathitt Gray > --- > Documentation/ABI/testing/sysfs-bus-counter | 24 +++++++++++++++++++++ > drivers/counter/ti-eqep.c | 2 +- > 2 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter > index 06c2b3e27e0b..37d960a8cb1b 100644 > --- a/Documentation/ABI/testing/sysfs-bus-counter > +++ b/Documentation/ABI/testing/sysfs-bus-counter > @@ -218,6 +218,9 @@ What: /sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id > What: /sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id > What: /sys/bus/counter/devices/counterX/signalY/index_polarity_component_id > What: /sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id > +What: /sys/bus/counter/devices/unit_timer_enable_component_id > +What: /sys/bus/counter/devices/unit_timer_period_component_id > +What: /sys/bus/counter/devices/unit_timer_time_component_id > KernelVersion: 5.16 > Contact: linux-iio@vger.kernel.org > Description: > @@ -345,3 +348,24 @@ Description: > via index_polarity. The index function (as enabled via > preset_enable) is performed synchronously with the > quadrature clock on the active level of the index input. > + > +What: /sys/bus/counter/devices/unit_timer_enable > +KernelVersion: 5.16 > +Contact: linux-iio@vger.kernel.org > +Description: > + Read/write attribute that starts or stops the unit timer. Valid > + values are boolean. > + > +What: /sys/bus/counter/devices/unit_timer_period > +KernelVersion: 5.16 > +Contact: linux-iio@vger.kernel.org > +Description: > + Read/write attribute that selects the unit timer timeout in > + nanoseconds. > + > +What: /sys/bus/counter/devices/unit_timer_time > +KernelVersion: 5.16 > +Contact: linux-iio@vger.kernel.org > +Description: > + Read/write attribute that indicates the current time of the > + unit timer in nanoseconds. > diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c > index a4a5a4486329..1ba7f3c7cb7e 100644 > --- a/drivers/counter/ti-eqep.c > +++ b/drivers/counter/ti-eqep.c > @@ -680,7 +680,7 @@ static int ti_eqep_probe(struct platform_device *pdev) > pm_runtime_get_sync(dev); > > /* > - * We can end up with an interupt infinite loop (interrupts triggered > + * We can end up with an interrupt infinite loop (interrupts triggered > * as soon as they are cleared) if we leave these at the default value > * of 0 and events are enabled. > */ > -- > 2.25.1 >
On 10/27/21 1:46 AM, William Breathitt Gray wrote: > On Sat, Oct 16, 2021 at 08:33:39PM -0500, David Lechner wrote: >> This documents new unit timer sysfs attributes for the counter >> subsystem. >> >> Signed-off-by: David Lechner <david@lechnology.com> > > Hello David, > > The unit timer is effectively a Count in its own right, so instead of > introducing new sysfs attributes you can just implement it as another > Count in the driver. Count 0 is "QPOSCNT", so set the name of this new > Count 1 as "Unit Timer" (or the datasheet naming if more apt) to > differentiate the Counts. You can then provide the "unit_timer_enable", > "unit_timer_period", and "unit_timer_time" functionalities as respective > Count 1 extensions ("enable" and "period") and Count 1 "count". > > If you believe it appropriate, you can provide the raw timer ticks via > the Count 1 "count" while a nanoseconds interface is provided via a > Count 1 extension "timeout" (or something similar). > Sounds reasonable.
On Wed, Oct 27, 2021 at 10:30:36AM -0500, David Lechner wrote: > On 10/27/21 1:46 AM, William Breathitt Gray wrote: > > On Sat, Oct 16, 2021 at 08:33:39PM -0500, David Lechner wrote: > >> This documents new unit timer sysfs attributes for the counter > >> subsystem. > >> > >> Signed-off-by: David Lechner <david@lechnology.com> > > > > Hello David, > > > > The unit timer is effectively a Count in its own right, so instead of > > introducing new sysfs attributes you can just implement it as another > > Count in the driver. Count 0 is "QPOSCNT", so set the name of this new > > Count 1 as "Unit Timer" (or the datasheet naming if more apt) to > > differentiate the Counts. You can then provide the "unit_timer_enable", > > "unit_timer_period", and "unit_timer_time" functionalities as respective > > Count 1 extensions ("enable" and "period") and Count 1 "count". Actually if the counter function here is COUNTER_FUNCTION_DECREASE, then instead of introducing a new "period" extension, define this as a "ceiling" extension; that's what ceiling represents in the Counter interface: "the upper limit for the respective counter", which is the period of a timer counting down to a timeout. William Breathitt Gray > > > > If you believe it appropriate, you can provide the raw timer ticks via > > the Count 1 "count" while a nanoseconds interface is provided via a > > Count 1 extension "timeout" (or something similar). > > > > Sounds reasonable. >
On 10/28/21 2:59 AM, William Breathitt Gray wrote: > On Wed, Oct 27, 2021 at 10:30:36AM -0500, David Lechner wrote: >> On 10/27/21 1:46 AM, William Breathitt Gray wrote: >>> On Sat, Oct 16, 2021 at 08:33:39PM -0500, David Lechner wrote: >>>> This documents new unit timer sysfs attributes for the counter >>>> subsystem. >>>> >>>> Signed-off-by: David Lechner <david@lechnology.com> >>> >>> Hello David, >>> >>> The unit timer is effectively a Count in its own right, so instead of >>> introducing new sysfs attributes you can just implement it as another >>> Count in the driver. Count 0 is "QPOSCNT", so set the name of this new >>> Count 1 as "Unit Timer" (or the datasheet naming if more apt) to >>> differentiate the Counts. You can then provide the "unit_timer_enable", >>> "unit_timer_period", and "unit_timer_time" functionalities as respective >>> Count 1 extensions ("enable" and "period") and Count 1 "count". > > Actually if the counter function here is COUNTER_FUNCTION_DECREASE, then It is an increasing counter. > instead of introducing a new "period" extension, define this as a > "ceiling" extension; that's what ceiling represents in the Counter > interface: "the upper limit for the respective counter", which is the > period of a timer counting down to a timeout. In one of the other patches, you made a comment about the semantics of ceiling with relation to the overflow event. We can indeed treat the timer as a counter and the period as the ceiling. However, the unit timer event occurs when the count is equal to the period (ceiling) whereas an overflow event occurs when the count exceeds the ceiling. So what would this event be called in generic counter terms? "timeout" doesn't seem right. > > William Breathitt Gray > >>> >>> If you believe it appropriate, you can provide the raw timer ticks via >>> the Count 1 "count" while a nanoseconds interface is provided via a >>> Count 1 extension "timeout" (or something similar). >>> One area where this concept of treating a timer as a counter potentially breaks down is the issue of CPU frequency scaling. By treating the unit timer as a timer, then the kernel could take care of any changes in clock rate internally by automatically adjusting the prescalar and period on rate change events. But if we are just treating it as a counter, then we should probably just have an attribute that provides the clock rate and if we want to support CPU frequency scaling, add an event that indicates that the clock rate changed.
On Sat, Oct 30, 2021 at 11:40:27AM -0500, David Lechner wrote: > On 10/28/21 2:59 AM, William Breathitt Gray wrote: > > On Wed, Oct 27, 2021 at 10:30:36AM -0500, David Lechner wrote: > >> On 10/27/21 1:46 AM, William Breathitt Gray wrote: > >>> On Sat, Oct 16, 2021 at 08:33:39PM -0500, David Lechner wrote: > >>>> This documents new unit timer sysfs attributes for the counter > >>>> subsystem. > >>>> > >>>> Signed-off-by: David Lechner <david@lechnology.com> > >>> > >>> Hello David, > >>> > >>> The unit timer is effectively a Count in its own right, so instead of > >>> introducing new sysfs attributes you can just implement it as another > >>> Count in the driver. Count 0 is "QPOSCNT", so set the name of this new > >>> Count 1 as "Unit Timer" (or the datasheet naming if more apt) to > >>> differentiate the Counts. You can then provide the "unit_timer_enable", > >>> "unit_timer_period", and "unit_timer_time" functionalities as respective > >>> Count 1 extensions ("enable" and "period") and Count 1 "count". > > > > Actually if the counter function here is COUNTER_FUNCTION_DECREASE, then > > It is an increasing counter. > > > instead of introducing a new "period" extension, define this as a > > "ceiling" extension; that's what ceiling represents in the Counter > > interface: "the upper limit for the respective counter", which is the > > period of a timer counting down to a timeout. > > In one of the other patches, you made a comment about the semantics > of ceiling with relation to the overflow event. We can indeed treat > the timer as a counter and the period as the ceiling. However, the > unit timer event occurs when the count is equal to the period (ceiling) > whereas an overflow event occurs when the count exceeds the ceiling. > So what would this event be called in generic counter terms? "timeout" > doesn't seem right. Okay, so COUNTER_EVENT_THRESHOLD would be the respective Counter event type for this behavior because the event triggers once a threshold is reached (ceiling in this case). But implementing the unit timer as a counter might not be the best path forward as you've mentioned below. > > > > William Breathitt Gray > > > >>> > >>> If you believe it appropriate, you can provide the raw timer ticks via > >>> the Count 1 "count" while a nanoseconds interface is provided via a > >>> Count 1 extension "timeout" (or something similar). > >>> > > One area where this concept of treating a timer as a counter potentially > breaks down is the issue of CPU frequency scaling. By treating the unit > timer as a timer, then the kernel could take care of any changes in clock > rate internally by automatically adjusting the prescalar and period on > rate change events. But if we are just treating it as a counter, then we > should probably just have an attribute that provides the clock rate and > if we want to support CPU frequency scaling, add an event that indicates > that the clock rate changed. You're right, treating the unit timer as a counter might not be the most appropriate interface. Because this is a timer afterall, perhaps exposing this via the hrtimer API is better. You then have an existing interface available designed for timer configuration, and you can leverage the struct hrtimer function callback to handle your timeout interrupts. William Breathitt Gray
On Mon, Nov 01, 2021 at 01:08:46PM +0900, William Breathitt Gray wrote: > On Sat, Oct 30, 2021 at 11:40:27AM -0500, David Lechner wrote: > > On 10/28/21 2:59 AM, William Breathitt Gray wrote: > > > On Wed, Oct 27, 2021 at 10:30:36AM -0500, David Lechner wrote: > > >> On 10/27/21 1:46 AM, William Breathitt Gray wrote: > > >>> On Sat, Oct 16, 2021 at 08:33:39PM -0500, David Lechner wrote: > > >>>> This documents new unit timer sysfs attributes for the counter > > >>>> subsystem. > > >>>> > > >>>> Signed-off-by: David Lechner <david@lechnology.com> > > >>> > > >>> Hello David, > > >>> > > >>> The unit timer is effectively a Count in its own right, so instead of > > >>> introducing new sysfs attributes you can just implement it as another > > >>> Count in the driver. Count 0 is "QPOSCNT", so set the name of this new > > >>> Count 1 as "Unit Timer" (or the datasheet naming if more apt) to > > >>> differentiate the Counts. You can then provide the "unit_timer_enable", > > >>> "unit_timer_period", and "unit_timer_time" functionalities as respective > > >>> Count 1 extensions ("enable" and "period") and Count 1 "count". > > > > > > Actually if the counter function here is COUNTER_FUNCTION_DECREASE, then > > > > It is an increasing counter. > > > > > instead of introducing a new "period" extension, define this as a > > > "ceiling" extension; that's what ceiling represents in the Counter > > > interface: "the upper limit for the respective counter", which is the > > > period of a timer counting down to a timeout. > > > > In one of the other patches, you made a comment about the semantics > > of ceiling with relation to the overflow event. We can indeed treat > > the timer as a counter and the period as the ceiling. However, the > > unit timer event occurs when the count is equal to the period (ceiling) > > whereas an overflow event occurs when the count exceeds the ceiling. > > So what would this event be called in generic counter terms? "timeout" > > doesn't seem right. > > Okay, so COUNTER_EVENT_THRESHOLD would be the respective Counter event > type for this behavior because the event triggers once a threshold is > reached (ceiling in this case). > > But implementing the unit timer as a counter might not be the best path > forward as you've mentioned below. > > > > > > > William Breathitt Gray > > > > > >>> > > >>> If you believe it appropriate, you can provide the raw timer ticks via > > >>> the Count 1 "count" while a nanoseconds interface is provided via a > > >>> Count 1 extension "timeout" (or something similar). > > >>> > > > > One area where this concept of treating a timer as a counter potentially > > breaks down is the issue of CPU frequency scaling. By treating the unit > > timer as a timer, then the kernel could take care of any changes in clock > > rate internally by automatically adjusting the prescalar and period on > > rate change events. But if we are just treating it as a counter, then we > > should probably just have an attribute that provides the clock rate and > > if we want to support CPU frequency scaling, add an event that indicates > > that the clock rate changed. > > You're right, treating the unit timer as a counter might not be the most > appropriate interface. Because this is a timer afterall, perhaps > exposing this via the hrtimer API is better. You then have an existing > interface available designed for timer configuration, and you can > leverage the struct hrtimer function callback to handle your timeout > interrupts. > > William Breathitt Gray Sorry, I think I meant the clockevents framework, not hrtimers. I'm not as familiar with timers but perhaps you know more than I do here. William Breathitt Gray
diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter index 06c2b3e27e0b..37d960a8cb1b 100644 --- a/Documentation/ABI/testing/sysfs-bus-counter +++ b/Documentation/ABI/testing/sysfs-bus-counter @@ -218,6 +218,9 @@ What: /sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id What: /sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id What: /sys/bus/counter/devices/counterX/signalY/index_polarity_component_id What: /sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id +What: /sys/bus/counter/devices/unit_timer_enable_component_id +What: /sys/bus/counter/devices/unit_timer_period_component_id +What: /sys/bus/counter/devices/unit_timer_time_component_id KernelVersion: 5.16 Contact: linux-iio@vger.kernel.org Description: @@ -345,3 +348,24 @@ Description: via index_polarity. The index function (as enabled via preset_enable) is performed synchronously with the quadrature clock on the active level of the index input. + +What: /sys/bus/counter/devices/unit_timer_enable +KernelVersion: 5.16 +Contact: linux-iio@vger.kernel.org +Description: + Read/write attribute that starts or stops the unit timer. Valid + values are boolean. + +What: /sys/bus/counter/devices/unit_timer_period +KernelVersion: 5.16 +Contact: linux-iio@vger.kernel.org +Description: + Read/write attribute that selects the unit timer timeout in + nanoseconds. + +What: /sys/bus/counter/devices/unit_timer_time +KernelVersion: 5.16 +Contact: linux-iio@vger.kernel.org +Description: + Read/write attribute that indicates the current time of the + unit timer in nanoseconds. diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c index a4a5a4486329..1ba7f3c7cb7e 100644 --- a/drivers/counter/ti-eqep.c +++ b/drivers/counter/ti-eqep.c @@ -680,7 +680,7 @@ static int ti_eqep_probe(struct platform_device *pdev) pm_runtime_get_sync(dev); /* - * We can end up with an interupt infinite loop (interrupts triggered + * We can end up with an interrupt infinite loop (interrupts triggered * as soon as they are cleared) if we leave these at the default value * of 0 and events are enabled. */
This documents new unit timer sysfs attributes for the counter subsystem. Signed-off-by: David Lechner <david@lechnology.com> --- Documentation/ABI/testing/sysfs-bus-counter | 24 +++++++++++++++++++++ drivers/counter/ti-eqep.c | 2 +- 2 files changed, 25 insertions(+), 1 deletion(-)