Now that the counter subsystem has a new chrdev for events, we can use this to
add new features to the TI eQEP driver to be able to do accurate speed
measurement.
This adds two new device-level components, a Unit Timer and an Edge Capture
Unit. I don't have much knowledge about other available counter hardware, so
I don't know if it makes sense to try to make these more generic, e.g.
counterX/timerY/* and counterX/captureY/*. For now, they are just flat
(counterX/unit_timer_* and counterX/edge_capture_unit_*) and assume there is
only one instance per counter device.
This has been tested on a BeagleBone Blue with LEGO MINDSTORMS motors.
David Lechner (8):
counter/ti-eqep: implement over/underflow events
counter/ti-eqep: add support for direction
counter/ti-eqep: add support for unit timer
docs: counter: add unit timer sysfs attributes
counter/ti-eqep: add support for latched position
docs: counter: add latch_mode and latched_count sysfs attributes
counter/ti-eqep: add support for edge capture unit
docs: counter: add edge_capture_unit_* attributes
Documentation/ABI/testing/sysfs-bus-counter | 100 +++-
drivers/counter/ti-eqep.c | 482 +++++++++++++++++++-
include/uapi/linux/counter.h | 4 +
3 files changed, 575 insertions(+), 11 deletions(-)
--
2.25.1
This documents new counterX/latch_mode* and
counterX/countY/latched_count* attributes.
The counterX/signalY/*_available are moved to the
counterX/countY/*_available section similar to the way we already have
The counterX/*_component_id and The counterX/signalY/*_component_id
grouped together so that we don't have to start a 3rd redundant section
for device-level *_available section.
Signed-off-by: David Lechner <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-counter | 39 ++++++++++++++++-----
1 file changed, 30 insertions(+), 9 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
index 37d960a8cb1b..78bb1a501007 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter
@@ -59,10 +59,13 @@ What: /sys/bus/counter/devices/counterX/countY/error_noise_available
What: /sys/bus/counter/devices/counterX/countY/function_available
What: /sys/bus/counter/devices/counterX/countY/prescaler_available
What: /sys/bus/counter/devices/counterX/countY/signalZ_action_available
+What: /sys/bus/counter/devices/counterX/latch_mode_available
+What: /sys/bus/counter/devices/counterX/signalY/index_polarity_available
+What: /sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
KernelVersion: 5.2
Contact: [email protected]
Description:
- Discrete set of available values for the respective Count Y
+ Discrete set of available values for the respective component
configuration are listed in this file. Values are delimited by
newline characters.
@@ -147,6 +150,14 @@ Description:
updates the respective count. Quadrature encoding
determines the direction.
+What: /sys/bus/counter/devices/counterX/countY/latched_count
+KernelVersion: 5.16
+Contact: [email protected]
+Description:
+ Latched count data of Count Y represented as a string. The value
+ is latched in based on the trigger selected by the
+ counterX/latch_mode attribute.
+
What: /sys/bus/counter/devices/counterX/countY/name
KernelVersion: 5.2
Contact: [email protected]
@@ -209,6 +220,7 @@ What: /sys/bus/counter/devices/counterX/countY/count_mode_component_id
What: /sys/bus/counter/devices/counterX/countY/direction_component_id
What: /sys/bus/counter/devices/counterX/countY/enable_component_id
What: /sys/bus/counter/devices/counterX/countY/error_noise_component_id
+What: /sys/bus/counter/devices/counterX/countY/latched_count_component_id
What: /sys/bus/counter/devices/counterX/countY/prescaler_component_id
What: /sys/bus/counter/devices/counterX/countY/preset_component_id
What: /sys/bus/counter/devices/counterX/countY/preset_enable_component_id
@@ -218,6 +230,7 @@ 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/latch_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
@@ -244,6 +257,22 @@ Description:
counter_event data structures. The number of elements will be
rounded-up to a power of 2.
+What: /sys/bus/counter/devices/counterX/latch_mode
+KernelVersion: 5.16
+Contact: [email protected]
+Description:
+ Read/write attribute that selects the trigger for latching
+ values. Valid values are device-specific (given by
+ latch_mode_available attribute) and may include:
+
+ "Read count":
+ Reading the countY/count attribute latches values.
+
+ "Unit timeout":
+ Unit timer timeout event latches values.
+
+ The latched values can be read from latched_* attributes.
+
What: /sys/bus/counter/devices/counterX/name
KernelVersion: 5.2
Contact: [email protected]
@@ -298,14 +327,6 @@ Description:
Active level of index input Signal Y; irrelevant in
non-synchronous load mode.
-What: /sys/bus/counter/devices/counterX/signalY/index_polarity_available
-What: /sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
-KernelVersion: 5.2
-Contact: [email protected]
-Description:
- Discrete set of available values for the respective Signal Y
- configuration are listed in this file.
-
What: /sys/bus/counter/devices/counterX/signalY/name
KernelVersion: 5.2
Contact: [email protected]
--
2.25.1
This adds support to the TI eQEP counter driver for subscribing to
overflow and underflow events using the counter chrdev interface.
Since this is the first event added, this involved adding an irq
handler. Also, additional range checks had to be added to the ceiling
attribute to avoid infinite interrupts.
Signed-off-by: David Lechner <[email protected]>
---
drivers/counter/ti-eqep.c | 119 +++++++++++++++++++++++++++++++++++++-
1 file changed, 117 insertions(+), 2 deletions(-)
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index 09817c953f9a..b7c79435e127 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -7,6 +7,7 @@
#include <linux/bitops.h>
#include <linux/counter.h>
+#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/mod_devicetable.h>
#include <linux/module.h>
@@ -67,6 +68,44 @@
#define QEPCTL_UTE BIT(1)
#define QEPCTL_WDE BIT(0)
+#define QEINT_UTO BIT(11)
+#define QEINT_IEL BIT(10)
+#define QEINT_SEL BIT(9)
+#define QEINT_PCM BIT(8)
+#define QEINT_PCR BIT(7)
+#define QEINT_PCO BIT(6)
+#define QEINT_PCU BIT(5)
+#define QEINT_WTO BIT(4)
+#define QEINT_QDC BIT(3)
+#define QEINT_PHE BIT(2)
+#define QEINT_PCE BIT(1)
+
+#define QFLG_UTO BIT(11)
+#define QFLG_IEL BIT(10)
+#define QFLG_SEL BIT(9)
+#define QFLG_PCM BIT(8)
+#define QFLG_PCR BIT(7)
+#define QFLG_PCO BIT(6)
+#define QFLG_PCU BIT(5)
+#define QFLG_WTO BIT(4)
+#define QFLG_QDC BIT(3)
+#define QFLG_PHE BIT(2)
+#define QFLG_PCE BIT(1)
+#define QFLG_INT BIT(0)
+
+#define QCLR_UTO BIT(11)
+#define QCLR_IEL BIT(10)
+#define QCLR_SEL BIT(9)
+#define QCLR_PCM BIT(8)
+#define QCLR_PCR BIT(7)
+#define QCLR_PCO BIT(6)
+#define QCLR_PCU BIT(5)
+#define QCLR_WTO BIT(4)
+#define QCLR_QDC BIT(3)
+#define QCLR_PHE BIT(2)
+#define QCLR_PCE BIT(1)
+#define QCLR_INT BIT(0)
+
/* EQEP Inputs */
enum {
TI_EQEP_SIGNAL_QEPA, /* QEPA/XCLK */
@@ -233,12 +272,46 @@ static int ti_eqep_action_read(struct counter_device *counter,
}
}
+static int ti_eqep_events_configure(struct counter_device *counter)
+{
+ struct ti_eqep_cnt *priv = counter->priv;
+ struct counter_event_node *event_node;
+ u32 qeint = 0;
+
+ list_for_each_entry(event_node, &counter->events_list, l) {
+ switch (event_node->event) {
+ case COUNTER_EVENT_OVERFLOW:
+ qeint |= QEINT_PCO;
+ break;
+ case COUNTER_EVENT_UNDERFLOW:
+ qeint |= QEINT_PCU;
+ break;
+ }
+ }
+
+ return regmap_write_bits(priv->regmap16, QEINT, ~0, qeint);
+}
+
+static int ti_eqep_watch_validate(struct counter_device *counter,
+ const struct counter_watch *watch)
+{
+ switch (watch->event) {
+ case COUNTER_EVENT_OVERFLOW:
+ case COUNTER_EVENT_UNDERFLOW:
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
static const struct counter_ops ti_eqep_counter_ops = {
.count_read = ti_eqep_count_read,
.count_write = ti_eqep_count_write,
.function_read = ti_eqep_function_read,
.function_write = ti_eqep_function_write,
.action_read = ti_eqep_action_read,
+ .events_configure = ti_eqep_events_configure,
+ .watch_validate = ti_eqep_watch_validate,
};
static int ti_eqep_position_ceiling_read(struct counter_device *counter,
@@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter,
u64 ceiling)
{
struct ti_eqep_cnt *priv = counter->priv;
+ u32 qposmax = ceiling;
- if (ceiling != (u32)ceiling)
+ /* ensure that value fits in 32-bit register */
+ if (qposmax != ceiling)
return -ERANGE;
- regmap_write(priv->regmap32, QPOSMAX, ceiling);
+ /* protect against infinite overflow interrupts */
+ if (qposmax == 0)
+ return -EINVAL;
+
+ regmap_write(priv->regmap32, QPOSMAX, qposmax);
return 0;
}
@@ -349,6 +428,25 @@ static struct counter_count ti_eqep_counts[] = {
},
};
+static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
+{
+ struct ti_eqep_cnt *priv = dev_id;
+ struct counter_device *counter = &priv->counter;
+ u32 qflg;
+
+ regmap_read(priv->regmap16, QFLG, &qflg);
+
+ if (qflg & QFLG_PCO)
+ counter_push_event(counter, COUNTER_EVENT_OVERFLOW, 0);
+
+ if (qflg & QFLG_PCU)
+ counter_push_event(counter, COUNTER_EVENT_UNDERFLOW, 0);
+
+ regmap_set_bits(priv->regmap16, QCLR, ~0);
+
+ return IRQ_HANDLED;
+}
+
static const struct regmap_config ti_eqep_regmap32_config = {
.name = "32-bit",
.reg_bits = 32,
@@ -371,6 +469,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
struct ti_eqep_cnt *priv;
void __iomem *base;
int err;
+ int irq;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
@@ -390,6 +489,15 @@ static int ti_eqep_probe(struct platform_device *pdev)
if (IS_ERR(priv->regmap16))
return PTR_ERR(priv->regmap16);
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ err = devm_request_threaded_irq(dev, irq, NULL, ti_eqep_irq_handler,
+ IRQF_ONESHOT, dev_name(dev), priv);
+ if (err < 0)
+ return err;
+
priv->counter.name = dev_name(dev);
priv->counter.parent = dev;
priv->counter.ops = &ti_eqep_counter_ops;
@@ -409,6 +517,13 @@ static int ti_eqep_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
pm_runtime_get_sync(dev);
+ /*
+ * We can end up with an interupt infinite loop (interrupts triggered
+ * as soon as they are cleared) if we leave this at the default value
+ * of 0 and events are enabled.
+ */
+ regmap_write(priv->regmap32, QPOSMAX, UINT_MAX);
+
err = counter_register(&priv->counter);
if (err < 0) {
pm_runtime_put_sync(dev);
--
2.25.1
This adds support to the TI eQEP counter driver for the Unit Timer.
The Unit Timer is a device-level extension that provides a timer to be
used for speed calculations. The sysfs interface for the Unit Timer is
new and will be documented in a later commit. It contains a R/W time
attribute for the current time, a R/W period attribute for the timeout
period and a R/W enable attribute to start/stop the timer. It also
implements a timeout event on the chrdev interface that is triggered
each time the period timeout is reached.
Signed-off-by: David Lechner <[email protected]>
---
drivers/counter/ti-eqep.c | 132 ++++++++++++++++++++++++++++++++++-
include/uapi/linux/counter.h | 2 +
2 files changed, 133 insertions(+), 1 deletion(-)
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index 9881e5115da6..a4a5a4486329 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -6,6 +6,7 @@
*/
#include <linux/bitops.h>
+#include <linux/clk.h>
#include <linux/counter.h>
#include <linux/interrupt.h>
#include <linux/kernel.h>
@@ -131,6 +132,7 @@ enum ti_eqep_count_func {
struct ti_eqep_cnt {
struct counter_device counter;
+ unsigned long sysclkout_rate;
struct regmap *regmap32;
struct regmap *regmap16;
};
@@ -298,6 +300,9 @@ static int ti_eqep_events_configure(struct counter_device *counter)
case COUNTER_EVENT_DIRECTION_CHANGE:
qeint |= QEINT_QDC;
break;
+ case COUNTER_EVENT_TIMEOUT:
+ qeint |= QEINT_UTO;
+ break;
}
}
@@ -311,6 +316,7 @@ static int ti_eqep_watch_validate(struct counter_device *counter,
case COUNTER_EVENT_OVERFLOW:
case COUNTER_EVENT_UNDERFLOW:
case COUNTER_EVENT_DIRECTION_CHANGE:
+ case COUNTER_EVENT_TIMEOUT:
return 0;
default:
return -EINVAL;
@@ -457,6 +463,106 @@ static struct counter_count ti_eqep_counts[] = {
},
};
+static int ti_eqep_unit_timer_time_read(struct counter_device *counter,
+ u64 *value)
+{
+ struct ti_eqep_cnt *priv = counter->priv;
+ u32 qutmr;
+
+ regmap_read(priv->regmap32, QUTMR, &qutmr);
+
+ /* convert timer ticks to nanoseconds */
+ *value = mul_u64_u32_div(qutmr, NSEC_PER_SEC, priv->sysclkout_rate);
+
+ return 0;
+}
+
+static int ti_eqep_unit_timer_time_write(struct counter_device *counter,
+ u64 value)
+{
+ struct ti_eqep_cnt *priv = counter->priv;
+ u32 qutmr;
+
+ /* convert nanoseconds to timer ticks */
+ qutmr = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
+ if (qutmr != value)
+ return -ERANGE;
+
+ regmap_write(priv->regmap32, QUTMR, qutmr);
+
+ return 0;
+}
+
+static int ti_eqep_unit_timer_period_read(struct counter_device *counter,
+ u64 *value)
+{
+ struct ti_eqep_cnt *priv = counter->priv;
+ u32 quprd;
+
+ regmap_read(priv->regmap32, QUPRD, &quprd);
+
+ /* convert timer ticks to nanoseconds */
+ *value = mul_u64_u32_div(quprd, NSEC_PER_SEC, priv->sysclkout_rate);
+
+ return 0;
+}
+
+static int ti_eqep_unit_timer_period_write(struct counter_device *counter,
+ u64 value)
+{
+ struct ti_eqep_cnt *priv = counter->priv;
+ u32 quprd;
+
+ /* convert nanoseconds to timer ticks */
+ quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
+ if (quprd != value)
+ return -ERANGE;
+
+ /* protect against infinite unit timeout interrupts */
+ if (quprd == 0)
+ return -EINVAL;
+
+ regmap_write(priv->regmap32, QUPRD, quprd);
+
+ return 0;
+}
+
+static int ti_eqep_unit_timer_enable_read(struct counter_device *counter,
+ u8 *value)
+{
+ struct ti_eqep_cnt *priv = counter->priv;
+ u32 qepctl;
+
+ regmap_read(priv->regmap16, QEPCTL, &qepctl);
+ *value = !!(qepctl & QEPCTL_UTE);
+
+ return 0;
+}
+
+static int ti_eqep_unit_timer_enable_write(struct counter_device *counter,
+ u8 value)
+{
+ struct ti_eqep_cnt *priv = counter->priv;
+
+ if (value)
+ regmap_set_bits(priv->regmap16, QEPCTL, QEPCTL_UTE);
+ else
+ regmap_clear_bits(priv->regmap16, QEPCTL, QEPCTL_UTE);
+
+ return 0;
+}
+
+static struct counter_comp ti_eqep_device_ext[] = {
+ COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read,
+ ti_eqep_unit_timer_time_write),
+ COUNTER_COMP_DEVICE_U64("unit_timer_period",
+ ti_eqep_unit_timer_period_read,
+ ti_eqep_unit_timer_period_write),
+ COUNTER_COMP_DEVICE_BOOL("unit_timer_enable",
+ ti_eqep_unit_timer_enable_read,
+ ti_eqep_unit_timer_enable_write),
+};
+
static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
{
struct ti_eqep_cnt *priv = dev_id;
@@ -474,6 +580,8 @@ static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
if (qflg & QFLG_QDC)
counter_push_event(counter, COUNTER_EVENT_DIRECTION_CHANGE, 0);
+ if (qflg & QFLG_UTO)
+ counter_push_event(counter, COUNTER_EVENT_TIMEOUT, 0);
regmap_set_bits(priv->regmap16, QCLR, ~0);
@@ -500,6 +608,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct ti_eqep_cnt *priv;
+ struct clk *clk;
void __iomem *base;
int err;
int irq;
@@ -508,6 +617,24 @@ static int ti_eqep_probe(struct platform_device *pdev)
if (!priv)
return -ENOMEM;
+ clk = devm_clk_get(dev, "sysclkout");
+ if (IS_ERR(clk)) {
+ if (PTR_ERR(clk) != -EPROBE_DEFER)
+ dev_err(dev, "failed to get sysclkout");
+ return PTR_ERR(clk);
+ }
+
+ priv->sysclkout_rate = clk_get_rate(clk);
+ if (priv->sysclkout_rate == 0) {
+ dev_err(dev, "failed to get sysclkout rate");
+ /* prevent divide by zero */
+ priv->sysclkout_rate = 1;
+ /*
+ * This error is not expected and the driver is mostly usable
+ * without clock rate anyway, so don't exit here.
+ */
+ }
+
base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(base))
return PTR_ERR(base);
@@ -536,6 +663,8 @@ static int ti_eqep_probe(struct platform_device *pdev)
priv->counter.ops = &ti_eqep_counter_ops;
priv->counter.counts = ti_eqep_counts;
priv->counter.num_counts = ARRAY_SIZE(ti_eqep_counts);
+ priv->counter.ext = ti_eqep_device_ext;
+ priv->counter.num_ext = ARRAY_SIZE(ti_eqep_device_ext);
priv->counter.signals = ti_eqep_signals;
priv->counter.num_signals = ARRAY_SIZE(ti_eqep_signals);
priv->counter.priv = priv;
@@ -552,10 +681,11 @@ static int ti_eqep_probe(struct platform_device *pdev)
/*
* We can end up with an interupt infinite loop (interrupts triggered
- * as soon as they are cleared) if we leave this at the default value
+ * as soon as they are cleared) if we leave these at the default value
* of 0 and events are enabled.
*/
regmap_write(priv->regmap32, QPOSMAX, UINT_MAX);
+ regmap_write(priv->regmap32, QUPRD, UINT_MAX);
err = counter_register(&priv->counter);
if (err < 0) {
diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
index 36dd3b474d09..640d9719b88c 100644
--- a/include/uapi/linux/counter.h
+++ b/include/uapi/linux/counter.h
@@ -63,6 +63,8 @@ enum counter_event_type {
COUNTER_EVENT_INDEX,
/* Direction change detected */
COUNTER_EVENT_DIRECTION_CHANGE,
+ /* Timer exceeded timeout */
+ COUNTER_EVENT_TIMEOUT,
};
/**
--
2.25.1
On Sat, 16 Oct 2021 20:33:41 -0500
David Lechner <[email protected]> wrote:
> This documents new counterX/latch_mode* and
> counterX/countY/latched_count* attributes.
>
> The counterX/signalY/*_available are moved to the
> counterX/countY/*_available section similar to the way we already have
> The counterX/*_component_id and The counterX/signalY/*_component_id
> grouped together so that we don't have to start a 3rd redundant section
> for device-level *_available section.
Two unrelated changes in the same patch. Please redo this as multiple patches.
Thanks,
Jonathan
>
> Signed-off-by: David Lechner <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-bus-counter | 39 ++++++++++++++++-----
> 1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index 37d960a8cb1b..78bb1a501007 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -59,10 +59,13 @@ What: /sys/bus/counter/devices/counterX/countY/error_noise_available
> What: /sys/bus/counter/devices/counterX/countY/function_available
> What: /sys/bus/counter/devices/counterX/countY/prescaler_available
> What: /sys/bus/counter/devices/counterX/countY/signalZ_action_available
> +What: /sys/bus/counter/devices/counterX/latch_mode_available
> +What: /sys/bus/counter/devices/counterX/signalY/index_polarity_available
> +What: /sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
> KernelVersion: 5.2
> Contact: [email protected]
> Description:
> - Discrete set of available values for the respective Count Y
> + Discrete set of available values for the respective component
> configuration are listed in this file. Values are delimited by
> newline characters.
>
> @@ -147,6 +150,14 @@ Description:
> updates the respective count. Quadrature encoding
> determines the direction.
>
> +What: /sys/bus/counter/devices/counterX/countY/latched_count
> +KernelVersion: 5.16
> +Contact: [email protected]
> +Description:
> + Latched count data of Count Y represented as a string. The value
> + is latched in based on the trigger selected by the
> + counterX/latch_mode attribute.
> +
> What: /sys/bus/counter/devices/counterX/countY/name
> KernelVersion: 5.2
> Contact: [email protected]
> @@ -209,6 +220,7 @@ What: /sys/bus/counter/devices/counterX/countY/count_mode_component_id
> What: /sys/bus/counter/devices/counterX/countY/direction_component_id
> What: /sys/bus/counter/devices/counterX/countY/enable_component_id
> What: /sys/bus/counter/devices/counterX/countY/error_noise_component_id
> +What: /sys/bus/counter/devices/counterX/countY/latched_count_component_id
> What: /sys/bus/counter/devices/counterX/countY/prescaler_component_id
> What: /sys/bus/counter/devices/counterX/countY/preset_component_id
> What: /sys/bus/counter/devices/counterX/countY/preset_enable_component_id
> @@ -218,6 +230,7 @@ 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/latch_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
> @@ -244,6 +257,22 @@ Description:
> counter_event data structures. The number of elements will be
> rounded-up to a power of 2.
>
> +What: /sys/bus/counter/devices/counterX/latch_mode
> +KernelVersion: 5.16
> +Contact: [email protected]
> +Description:
> + Read/write attribute that selects the trigger for latching
> + values. Valid values are device-specific (given by
> + latch_mode_available attribute) and may include:
> +
> + "Read count":
> + Reading the countY/count attribute latches values.
> +
> + "Unit timeout":
> + Unit timer timeout event latches values.
> +
> + The latched values can be read from latched_* attributes.
> +
> What: /sys/bus/counter/devices/counterX/name
> KernelVersion: 5.2
> Contact: [email protected]
> @@ -298,14 +327,6 @@ Description:
> Active level of index input Signal Y; irrelevant in
> non-synchronous load mode.
>
> -What: /sys/bus/counter/devices/counterX/signalY/index_polarity_available
> -What: /sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
> -KernelVersion: 5.2
> -Contact: [email protected]
> -Description:
> - Discrete set of available values for the respective Signal Y
> - configuration are listed in this file.
> -
> What: /sys/bus/counter/devices/counterX/signalY/name
> KernelVersion: 5.2
> Contact: [email protected]
On Sat, 16 Oct 2021 20:33:38 -0500
David Lechner <[email protected]> wrote:
> This adds support to the TI eQEP counter driver for the Unit Timer.
> The Unit Timer is a device-level extension that provides a timer to be
> used for speed calculations. The sysfs interface for the Unit Timer is
> new and will be documented in a later commit. It contains a R/W time
> attribute for the current time, a R/W period attribute for the timeout
> period and a R/W enable attribute to start/stop the timer. It also
> implements a timeout event on the chrdev interface that is triggered
> each time the period timeout is reached.
>
> Signed-off-by: David Lechner <[email protected]>
No comments on the interface in here as leaving that for William / later.
A few minor comments on the implementation.
Thanks,
Jonathan
> ---
> drivers/counter/ti-eqep.c | 132 ++++++++++++++++++++++++++++++++++-
> include/uapi/linux/counter.h | 2 +
> 2 files changed, 133 insertions(+), 1 deletion(-)
...
> +static int ti_eqep_unit_timer_time_write(struct counter_device *counter,
> + u64 value)
> +{
> + struct ti_eqep_cnt *priv = counter->priv;
> + u32 qutmr;
> +
> + /* convert nanoseconds to timer ticks */
> + qutmr = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
Hmm. This pattern strikes me as 'too clever' and also likely to trip up static
checkers who will moan about the truncation if they don't understand this trick.
I think I'd prefer you just put the answer in an u64 and then do a simple bounds
check before casting down.
> + if (qutmr != value)
> + return -ERANGE;
> +
> + regmap_write(priv->regmap32, QUTMR, qutmr);
> +
> + return 0;
> +}
> +
...
> static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
> {
> struct ti_eqep_cnt *priv = dev_id;
> @@ -474,6 +580,8 @@ static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
> if (qflg & QFLG_QDC)
> counter_push_event(counter, COUNTER_EVENT_DIRECTION_CHANGE, 0);
>
> + if (qflg & QFLG_UTO)
> + counter_push_event(counter, COUNTER_EVENT_TIMEOUT, 0);
>
> regmap_set_bits(priv->regmap16, QCLR, ~0);
>
> @@ -500,6 +608,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct ti_eqep_cnt *priv;
> + struct clk *clk;
> void __iomem *base;
> int err;
> int irq;
> @@ -508,6 +617,24 @@ static int ti_eqep_probe(struct platform_device *pdev)
> if (!priv)
> return -ENOMEM;
>
> + clk = devm_clk_get(dev, "sysclkout");
> + if (IS_ERR(clk)) {
> + if (PTR_ERR(clk) != -EPROBE_DEFER)
> + dev_err(dev, "failed to get sysclkout");
dev_err_probe() which both removes most of this boilerplate
and stashes the reason for the deferred probe such that it can be checked when
debugging.
> + return PTR_ERR(clk);
> + }
No need to enable the clock?
> +
> + priv->sysclkout_rate = clk_get_rate(clk);
> + if (priv->sysclkout_rate == 0) {
> + dev_err(dev, "failed to get sysclkout rate");
> + /* prevent divide by zero */
> + priv->sysclkout_rate = 1;
> + /*
> + * This error is not expected and the driver is mostly usable
> + * without clock rate anyway, so don't exit here.
> + */
> + }
> +
>
> /**
On Sat, 16 Oct 2021 20:33:36 -0500
David Lechner <[email protected]> wrote:
> This adds support to the TI eQEP counter driver for subscribing to
> overflow and underflow events using the counter chrdev interface.
>
> Since this is the first event added, this involved adding an irq
> handler. Also, additional range checks had to be added to the ceiling
> attribute to avoid infinite interrupts.
>
> Signed-off-by: David Lechner <[email protected]>
Hi David,
A few minor things inline. I've not commented on the new counter interface
stuff though as it's still a bit vague in my head, so over to William for that :)
Jonathan
> ---
> drivers/counter/ti-eqep.c | 119 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 117 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index 09817c953f9a..b7c79435e127 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -7,6 +7,7 @@
>
> #include <linux/bitops.h>
> #include <linux/counter.h>
> +#include <linux/interrupt.h>
> #include <linux/kernel.h>
> #include <linux/mod_devicetable.h>
> #include <linux/module.h>
> @@ -67,6 +68,44 @@
> #define QEPCTL_UTE BIT(1)
> #define QEPCTL_WDE BIT(0)
>
> +#define QEINT_UTO BIT(11)
> +#define QEINT_IEL BIT(10)
> +#define QEINT_SEL BIT(9)
> +#define QEINT_PCM BIT(8)
> +#define QEINT_PCR BIT(7)
> +#define QEINT_PCO BIT(6)
> +#define QEINT_PCU BIT(5)
> +#define QEINT_WTO BIT(4)
> +#define QEINT_QDC BIT(3)
> +#define QEINT_PHE BIT(2)
> +#define QEINT_PCE BIT(1)
> +
> +#define QFLG_UTO BIT(11)
> +#define QFLG_IEL BIT(10)
> +#define QFLG_SEL BIT(9)
> +#define QFLG_PCM BIT(8)
> +#define QFLG_PCR BIT(7)
> +#define QFLG_PCO BIT(6)
> +#define QFLG_PCU BIT(5)
> +#define QFLG_WTO BIT(4)
> +#define QFLG_QDC BIT(3)
> +#define QFLG_PHE BIT(2)
> +#define QFLG_PCE BIT(1)
> +#define QFLG_INT BIT(0)
> +
> +#define QCLR_UTO BIT(11)
> +#define QCLR_IEL BIT(10)
> +#define QCLR_SEL BIT(9)
> +#define QCLR_PCM BIT(8)
> +#define QCLR_PCR BIT(7)
> +#define QCLR_PCO BIT(6)
> +#define QCLR_PCU BIT(5)
> +#define QCLR_WTO BIT(4)
> +#define QCLR_QDC BIT(3)
> +#define QCLR_PHE BIT(2)
> +#define QCLR_PCE BIT(1)
> +#define QCLR_INT BIT(0)
> +
> /* EQEP Inputs */
> enum {
> TI_EQEP_SIGNAL_QEPA, /* QEPA/XCLK */
> @@ -233,12 +272,46 @@ static int ti_eqep_action_read(struct counter_device *counter,
> }
> }
>
> +static int ti_eqep_events_configure(struct counter_device *counter)
> +{
> + struct ti_eqep_cnt *priv = counter->priv;
> + struct counter_event_node *event_node;
> + u32 qeint = 0;
> +
> + list_for_each_entry(event_node, &counter->events_list, l) {
> + switch (event_node->event) {
> + case COUNTER_EVENT_OVERFLOW:
> + qeint |= QEINT_PCO;
> + break;
> + case COUNTER_EVENT_UNDERFLOW:
> + qeint |= QEINT_PCU;
> + break;
> + }
> + }
> +
> + return regmap_write_bits(priv->regmap16, QEINT, ~0, qeint);
regmap_write() given mask is all bits.
> +}
> +
> +static int ti_eqep_watch_validate(struct counter_device *counter,
> + const struct counter_watch *watch)
> +{
> + switch (watch->event) {
> + case COUNTER_EVENT_OVERFLOW:
> + case COUNTER_EVENT_UNDERFLOW:
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static const struct counter_ops ti_eqep_counter_ops = {
> .count_read = ti_eqep_count_read,
> .count_write = ti_eqep_count_write,
> .function_read = ti_eqep_function_read,
> .function_write = ti_eqep_function_write,
> .action_read = ti_eqep_action_read,
> + .events_configure = ti_eqep_events_configure,
> + .watch_validate = ti_eqep_watch_validate,
> };
>
> static int ti_eqep_position_ceiling_read(struct counter_device *counter,
> @@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter,
> u64 ceiling)
> {
> struct ti_eqep_cnt *priv = counter->priv;
> + u32 qposmax = ceiling;
>
> - if (ceiling != (u32)ceiling)
> + /* ensure that value fits in 32-bit register */
> + if (qposmax != ceiling)
> return -ERANGE;
>
> - regmap_write(priv->regmap32, QPOSMAX, ceiling);
> + /* protect against infinite overflow interrupts */
> + if (qposmax == 0)
> + return -EINVAL;
> +
> + regmap_write(priv->regmap32, QPOSMAX, qposmax);
>
> return 0;
> }
> @@ -349,6 +428,25 @@ static struct counter_count ti_eqep_counts[] = {
> },
> };
>
> +static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
> +{
> + struct ti_eqep_cnt *priv = dev_id;
> + struct counter_device *counter = &priv->counter;
> + u32 qflg;
> +
> + regmap_read(priv->regmap16, QFLG, &qflg);
> +
> + if (qflg & QFLG_PCO)
> + counter_push_event(counter, COUNTER_EVENT_OVERFLOW, 0);
> +
> + if (qflg & QFLG_PCU)
> + counter_push_event(counter, COUNTER_EVENT_UNDERFLOW, 0);
> +
> + regmap_set_bits(priv->regmap16, QCLR, ~0);
Generally avoid clearing bits reflecting interrupt events you haven't handled.
I'm guessing there is a potential race in here where we read qflg and additional
events then occur before we clear. Those events we never see because they
are unconditionally cleared by this write.
We are better off interrupting again if that race happens.
> +
> + return IRQ_HANDLED;
> +}
> +
> static const struct regmap_config ti_eqep_regmap32_config = {
> .name = "32-bit",
> .reg_bits = 32,
> @@ -371,6 +469,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
> struct ti_eqep_cnt *priv;
> void __iomem *base;
> int err;
> + int irq;
>
> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> @@ -390,6 +489,15 @@ static int ti_eqep_probe(struct platform_device *pdev)
> if (IS_ERR(priv->regmap16))
> return PTR_ERR(priv->regmap16);
>
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + err = devm_request_threaded_irq(dev, irq, NULL, ti_eqep_irq_handler,
> + IRQF_ONESHOT, dev_name(dev), priv);
> + if (err < 0)
> + return err;
> +
> priv->counter.name = dev_name(dev);
> priv->counter.parent = dev;
> priv->counter.ops = &ti_eqep_counter_ops;
> @@ -409,6 +517,13 @@ static int ti_eqep_probe(struct platform_device *pdev)
> pm_runtime_enable(dev);
> pm_runtime_get_sync(dev);
>
> + /*
> + * We can end up with an interupt infinite loop (interrupts triggered
> + * as soon as they are cleared) if we leave this at the default value
> + * of 0 and events are enabled.
> + */
> + regmap_write(priv->regmap32, QPOSMAX, UINT_MAX);
> +
> err = counter_register(&priv->counter);
> if (err < 0) {
> pm_runtime_put_sync(dev);
On Sat, Oct 16, 2021 at 08:33:36PM -0500, David Lechner wrote:
> This adds support to the TI eQEP counter driver for subscribing to
> overflow and underflow events using the counter chrdev interface.
>
> Since this is the first event added, this involved adding an irq
> handler. Also, additional range checks had to be added to the ceiling
> attribute to avoid infinite interrupts.
>
> Signed-off-by: David Lechner <[email protected]>
Hi David,
This looks functionally okay, but I have a couple minor comments inline.
> ---
> drivers/counter/ti-eqep.c | 119 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 117 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index 09817c953f9a..b7c79435e127 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -7,6 +7,7 @@
>
> #include <linux/bitops.h>
> #include <linux/counter.h>
> +#include <linux/interrupt.h>
> #include <linux/kernel.h>
> #include <linux/mod_devicetable.h>
> #include <linux/module.h>
> @@ -67,6 +68,44 @@
> #define QEPCTL_UTE BIT(1)
> #define QEPCTL_WDE BIT(0)
>
> +#define QEINT_UTO BIT(11)
> +#define QEINT_IEL BIT(10)
> +#define QEINT_SEL BIT(9)
> +#define QEINT_PCM BIT(8)
> +#define QEINT_PCR BIT(7)
> +#define QEINT_PCO BIT(6)
> +#define QEINT_PCU BIT(5)
> +#define QEINT_WTO BIT(4)
> +#define QEINT_QDC BIT(3)
> +#define QEINT_PHE BIT(2)
> +#define QEINT_PCE BIT(1)
> +
> +#define QFLG_UTO BIT(11)
> +#define QFLG_IEL BIT(10)
> +#define QFLG_SEL BIT(9)
> +#define QFLG_PCM BIT(8)
> +#define QFLG_PCR BIT(7)
> +#define QFLG_PCO BIT(6)
> +#define QFLG_PCU BIT(5)
> +#define QFLG_WTO BIT(4)
> +#define QFLG_QDC BIT(3)
> +#define QFLG_PHE BIT(2)
> +#define QFLG_PCE BIT(1)
> +#define QFLG_INT BIT(0)
> +
> +#define QCLR_UTO BIT(11)
> +#define QCLR_IEL BIT(10)
> +#define QCLR_SEL BIT(9)
> +#define QCLR_PCM BIT(8)
> +#define QCLR_PCR BIT(7)
> +#define QCLR_PCO BIT(6)
> +#define QCLR_PCU BIT(5)
> +#define QCLR_WTO BIT(4)
> +#define QCLR_QDC BIT(3)
> +#define QCLR_PHE BIT(2)
> +#define QCLR_PCE BIT(1)
> +#define QCLR_INT BIT(0)
> +
> /* EQEP Inputs */
> enum {
> TI_EQEP_SIGNAL_QEPA, /* QEPA/XCLK */
> @@ -233,12 +272,46 @@ static int ti_eqep_action_read(struct counter_device *counter,
> }
> }
>
> +static int ti_eqep_events_configure(struct counter_device *counter)
> +{
> + struct ti_eqep_cnt *priv = counter->priv;
> + struct counter_event_node *event_node;
> + u32 qeint = 0;
> +
> + list_for_each_entry(event_node, &counter->events_list, l) {
> + switch (event_node->event) {
> + case COUNTER_EVENT_OVERFLOW:
> + qeint |= QEINT_PCO;
> + break;
> + case COUNTER_EVENT_UNDERFLOW:
> + qeint |= QEINT_PCU;
> + break;
> + }
> + }
> +
> + return regmap_write_bits(priv->regmap16, QEINT, ~0, qeint);
> +}
> +
> +static int ti_eqep_watch_validate(struct counter_device *counter,
> + const struct counter_watch *watch)
> +{
> + switch (watch->event) {
> + case COUNTER_EVENT_OVERFLOW:
> + case COUNTER_EVENT_UNDERFLOW:
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static const struct counter_ops ti_eqep_counter_ops = {
> .count_read = ti_eqep_count_read,
> .count_write = ti_eqep_count_write,
> .function_read = ti_eqep_function_read,
> .function_write = ti_eqep_function_write,
> .action_read = ti_eqep_action_read,
> + .events_configure = ti_eqep_events_configure,
> + .watch_validate = ti_eqep_watch_validate,
> };
>
> static int ti_eqep_position_ceiling_read(struct counter_device *counter,
> @@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter,
> u64 ceiling)
> {
> struct ti_eqep_cnt *priv = counter->priv;
> + u32 qposmax = ceiling;
>
> - if (ceiling != (u32)ceiling)
> + /* ensure that value fits in 32-bit register */
> + if (qposmax != ceiling)
> return -ERANGE;
>
> - regmap_write(priv->regmap32, QPOSMAX, ceiling);
> + /* protect against infinite overflow interrupts */
> + if (qposmax == 0)
> + return -EINVAL;
Would you be able to explain this scenario a bit further? My expectation
would be that an overflow event would only occur if the position
increased past the ceiling (i.e. increased to greater than 0). Of
course, running the device with a ceiling of 0 effectively guarantees
overflow eventss with every movement, but I would expect a stationary
device to sit with a position of 0 and thus no overflow events.
> +
> + regmap_write(priv->regmap32, QPOSMAX, qposmax);
>
> return 0;
> }
> @@ -349,6 +428,25 @@ static struct counter_count ti_eqep_counts[] = {
> },
> };
>
> +static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
> +{
> + struct ti_eqep_cnt *priv = dev_id;
> + struct counter_device *counter = &priv->counter;
> + u32 qflg;
> +
> + regmap_read(priv->regmap16, QFLG, &qflg);
> +
> + if (qflg & QFLG_PCO)
> + counter_push_event(counter, COUNTER_EVENT_OVERFLOW, 0);
> +
> + if (qflg & QFLG_PCU)
> + counter_push_event(counter, COUNTER_EVENT_UNDERFLOW, 0);
> +
> + regmap_set_bits(priv->regmap16, QCLR, ~0);
I agree with Jonathan here, it looks like this is unconditionally
clearing all interrupt flags but it's possible new ones could have
appear between the time after you read QFLG to here. Is it possible to
clear just the interrupt flags you've handled here instead of all?
William Breathitt Gray
> +
> + return IRQ_HANDLED;
> +}
> +
> static const struct regmap_config ti_eqep_regmap32_config = {
> .name = "32-bit",
> .reg_bits = 32,
> @@ -371,6 +469,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
> struct ti_eqep_cnt *priv;
> void __iomem *base;
> int err;
> + int irq;
>
> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> @@ -390,6 +489,15 @@ static int ti_eqep_probe(struct platform_device *pdev)
> if (IS_ERR(priv->regmap16))
> return PTR_ERR(priv->regmap16);
>
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + err = devm_request_threaded_irq(dev, irq, NULL, ti_eqep_irq_handler,
> + IRQF_ONESHOT, dev_name(dev), priv);
> + if (err < 0)
> + return err;
> +
> priv->counter.name = dev_name(dev);
> priv->counter.parent = dev;
> priv->counter.ops = &ti_eqep_counter_ops;
> @@ -409,6 +517,13 @@ static int ti_eqep_probe(struct platform_device *pdev)
> pm_runtime_enable(dev);
> pm_runtime_get_sync(dev);
>
> + /*
> + * We can end up with an interupt infinite loop (interrupts triggered
> + * as soon as they are cleared) if we leave this at the default value
> + * of 0 and events are enabled.
> + */
> + regmap_write(priv->regmap32, QPOSMAX, UINT_MAX);
> +
> err = counter_register(&priv->counter);
> if (err < 0) {
> pm_runtime_put_sync(dev);
> --
> 2.25.1
>
On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote:
> This adds support to the TI eQEP counter driver for the Unit Timer.
> The Unit Timer is a device-level extension that provides a timer to be
> used for speed calculations. The sysfs interface for the Unit Timer is
> new and will be documented in a later commit. It contains a R/W time
> attribute for the current time, a R/W period attribute for the timeout
> period and a R/W enable attribute to start/stop the timer. It also
> implements a timeout event on the chrdev interface that is triggered
> each time the period timeout is reached.
>
> Signed-off-by: David Lechner <[email protected]>
I'll comment on the sysfs interface in the respective docs patch. Some
comments regarding this patch below.
> ---
> drivers/counter/ti-eqep.c | 132 ++++++++++++++++++++++++++++++++++-
> include/uapi/linux/counter.h | 2 +
> 2 files changed, 133 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index 9881e5115da6..a4a5a4486329 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -6,6 +6,7 @@
> */
>
> #include <linux/bitops.h>
> +#include <linux/clk.h>
> #include <linux/counter.h>
> #include <linux/interrupt.h>
> #include <linux/kernel.h>
> @@ -131,6 +132,7 @@ enum ti_eqep_count_func {
>
> struct ti_eqep_cnt {
> struct counter_device counter;
> + unsigned long sysclkout_rate;
> struct regmap *regmap32;
> struct regmap *regmap16;
> };
> @@ -298,6 +300,9 @@ static int ti_eqep_events_configure(struct counter_device *counter)
> case COUNTER_EVENT_DIRECTION_CHANGE:
> qeint |= QEINT_QDC;
> break;
> + case COUNTER_EVENT_TIMEOUT:
> + qeint |= QEINT_UTO;
> + break;
> }
> }
>
> @@ -311,6 +316,7 @@ static int ti_eqep_watch_validate(struct counter_device *counter,
> case COUNTER_EVENT_OVERFLOW:
> case COUNTER_EVENT_UNDERFLOW:
> case COUNTER_EVENT_DIRECTION_CHANGE:
> + case COUNTER_EVENT_TIMEOUT:
> return 0;
> default:
> return -EINVAL;
> @@ -457,6 +463,106 @@ static struct counter_count ti_eqep_counts[] = {
> },
> };
>
> +static int ti_eqep_unit_timer_time_read(struct counter_device *counter,
> + u64 *value)
> +{
> + struct ti_eqep_cnt *priv = counter->priv;
> + u32 qutmr;
> +
> + regmap_read(priv->regmap32, QUTMR, &qutmr);
> +
> + /* convert timer ticks to nanoseconds */
> + *value = mul_u64_u32_div(qutmr, NSEC_PER_SEC, priv->sysclkout_rate);
> +
> + return 0;
> +}
> +
> +static int ti_eqep_unit_timer_time_write(struct counter_device *counter,
> + u64 value)
> +{
> + struct ti_eqep_cnt *priv = counter->priv;
> + u32 qutmr;
> +
> + /* convert nanoseconds to timer ticks */
> + qutmr = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
> + if (qutmr != value)
> + return -ERANGE;
> +
> + regmap_write(priv->regmap32, QUTMR, qutmr);
> +
> + return 0;
> +}
> +
> +static int ti_eqep_unit_timer_period_read(struct counter_device *counter,
> + u64 *value)
> +{
> + struct ti_eqep_cnt *priv = counter->priv;
> + u32 quprd;
> +
> + regmap_read(priv->regmap32, QUPRD, &quprd);
> +
> + /* convert timer ticks to nanoseconds */
> + *value = mul_u64_u32_div(quprd, NSEC_PER_SEC, priv->sysclkout_rate);
> +
> + return 0;
> +}
> +
> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter,
> + u64 value)
> +{
> + struct ti_eqep_cnt *priv = counter->priv;
> + u32 quprd;
> +
> + /* convert nanoseconds to timer ticks */
> + quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
> + if (quprd != value)
> + return -ERANGE;
> +
> + /* protect against infinite unit timeout interrupts */
> + if (quprd == 0)
> + return -EINVAL;
I doubt there's any practical reason for a user to set the timer period
to 0, but perhaps we should not try to protect users from themselves
here. It's very obvious and expected that setting the timer period to 0
results in timeouts as quickly as possible, so really the user should be
left to reap the fruits of their decision regardless of how asinine that
decision is.
> +
> + regmap_write(priv->regmap32, QUPRD, quprd);
> +
> + return 0;
> +}
> +
> +static int ti_eqep_unit_timer_enable_read(struct counter_device *counter,
> + u8 *value)
> +{
> + struct ti_eqep_cnt *priv = counter->priv;
> + u32 qepctl;
> +
> + regmap_read(priv->regmap16, QEPCTL, &qepctl);
> + *value = !!(qepctl & QEPCTL_UTE);
> +
> + return 0;
> +}
> +
> +static int ti_eqep_unit_timer_enable_write(struct counter_device *counter,
> + u8 value)
> +{
> + struct ti_eqep_cnt *priv = counter->priv;
> +
> + if (value)
> + regmap_set_bits(priv->regmap16, QEPCTL, QEPCTL_UTE);
> + else
> + regmap_clear_bits(priv->regmap16, QEPCTL, QEPCTL_UTE);
> +
> + return 0;
> +}
> +
> +static struct counter_comp ti_eqep_device_ext[] = {
> + COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read,
> + ti_eqep_unit_timer_time_write),
> + COUNTER_COMP_DEVICE_U64("unit_timer_period",
> + ti_eqep_unit_timer_period_read,
> + ti_eqep_unit_timer_period_write),
> + COUNTER_COMP_DEVICE_BOOL("unit_timer_enable",
> + ti_eqep_unit_timer_enable_read,
> + ti_eqep_unit_timer_enable_write),
> +};
> +
> static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
> {
> struct ti_eqep_cnt *priv = dev_id;
> @@ -474,6 +580,8 @@ static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
> if (qflg & QFLG_QDC)
> counter_push_event(counter, COUNTER_EVENT_DIRECTION_CHANGE, 0);
>
> + if (qflg & QFLG_UTO)
> + counter_push_event(counter, COUNTER_EVENT_TIMEOUT, 0);
>
> regmap_set_bits(priv->regmap16, QCLR, ~0);
Same comment here as the previous patches about clearing all interrupt
flags.
>
> @@ -500,6 +608,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct ti_eqep_cnt *priv;
> + struct clk *clk;
> void __iomem *base;
> int err;
> int irq;
> @@ -508,6 +617,24 @@ static int ti_eqep_probe(struct platform_device *pdev)
> if (!priv)
> return -ENOMEM;
>
> + clk = devm_clk_get(dev, "sysclkout");
> + if (IS_ERR(clk)) {
> + if (PTR_ERR(clk) != -EPROBE_DEFER)
> + dev_err(dev, "failed to get sysclkout");
> + return PTR_ERR(clk);
> + }
> +
> + priv->sysclkout_rate = clk_get_rate(clk);
> + if (priv->sysclkout_rate == 0) {
> + dev_err(dev, "failed to get sysclkout rate");
> + /* prevent divide by zero */
> + priv->sysclkout_rate = 1;
> + /*
> + * This error is not expected and the driver is mostly usable
> + * without clock rate anyway, so don't exit here.
> + */
If the values for these new attributes are expected to be denominated in
nanoseconds then we must guarantee that. You should certainly error out
here if you can't guarantee such.
Alternatively, you could provide an additional set of attributes that
are denominated in units of raw timer ticks rather than nanoseconds.
That way if you can't determine the clock rate you can simply have the
nanosecond-denominated timer attributes return an EOPNOTSUPP error code
or similar while still providing users with the raw timer ticks
attributes.
William Breathitt Gray
> + }
> +
> base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(base))
> return PTR_ERR(base);
> @@ -536,6 +663,8 @@ static int ti_eqep_probe(struct platform_device *pdev)
> priv->counter.ops = &ti_eqep_counter_ops;
> priv->counter.counts = ti_eqep_counts;
> priv->counter.num_counts = ARRAY_SIZE(ti_eqep_counts);
> + priv->counter.ext = ti_eqep_device_ext;
> + priv->counter.num_ext = ARRAY_SIZE(ti_eqep_device_ext);
> priv->counter.signals = ti_eqep_signals;
> priv->counter.num_signals = ARRAY_SIZE(ti_eqep_signals);
> priv->counter.priv = priv;
> @@ -552,10 +681,11 @@ static int ti_eqep_probe(struct platform_device *pdev)
>
> /*
> * We can end up with an interupt infinite loop (interrupts triggered
> - * as soon as they are cleared) if we leave this at the default value
> + * as soon as they are cleared) if we leave these at the default value
> * of 0 and events are enabled.
> */
> regmap_write(priv->regmap32, QPOSMAX, UINT_MAX);
> + regmap_write(priv->regmap32, QUPRD, UINT_MAX);
>
> err = counter_register(&priv->counter);
> if (err < 0) {
> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> index 36dd3b474d09..640d9719b88c 100644
> --- a/include/uapi/linux/counter.h
> +++ b/include/uapi/linux/counter.h
> @@ -63,6 +63,8 @@ enum counter_event_type {
> COUNTER_EVENT_INDEX,
> /* Direction change detected */
> COUNTER_EVENT_DIRECTION_CHANGE,
> + /* Timer exceeded timeout */
> + COUNTER_EVENT_TIMEOUT,
> };
>
> /**
> --
> 2.25.1
>
On Sat, Oct 16, 2021 at 08:33:41PM -0500, David Lechner wrote:
> This documents new counterX/latch_mode* and
> counterX/countY/latched_count* attributes.
>
> The counterX/signalY/*_available are moved to the
> counterX/countY/*_available section similar to the way we already have
> The counterX/*_component_id and The counterX/signalY/*_component_id
> grouped together so that we don't have to start a 3rd redundant section
> for device-level *_available section.
>
> Signed-off-by: David Lechner <[email protected]>
Please separate these two distinct changes into two distinct patches.
> ---
> Documentation/ABI/testing/sysfs-bus-counter | 39 ++++++++++++++++-----
> 1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index 37d960a8cb1b..78bb1a501007 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -59,10 +59,13 @@ What: /sys/bus/counter/devices/counterX/countY/error_noise_available
> What: /sys/bus/counter/devices/counterX/countY/function_available
> What: /sys/bus/counter/devices/counterX/countY/prescaler_available
> What: /sys/bus/counter/devices/counterX/countY/signalZ_action_available
> +What: /sys/bus/counter/devices/counterX/latch_mode_available
> +What: /sys/bus/counter/devices/counterX/signalY/index_polarity_available
> +What: /sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
> KernelVersion: 5.2
> Contact: [email protected]
> Description:
> - Discrete set of available values for the respective Count Y
> + Discrete set of available values for the respective component
> configuration are listed in this file. Values are delimited by
> newline characters.
>
> @@ -147,6 +150,14 @@ Description:
> updates the respective count. Quadrature encoding
> determines the direction.
>
> +What: /sys/bus/counter/devices/counterX/countY/latched_count
> +KernelVersion: 5.16
> +Contact: [email protected]
> +Description:
> + Latched count data of Count Y represented as a string. The value
> + is latched in based on the trigger selected by the
> + counterX/latch_mode attribute.
> +
Latches are pretty common components of devices, and not simply limited
to latching the count data. I wonder if it would be better to omit the
"_count" suffix in order to make this more general. Well, the name
"latched_count" is suitable for counters so you probably don't need to
change it, but it's something to think about in the future.
> What: /sys/bus/counter/devices/counterX/countY/name
> KernelVersion: 5.2
> Contact: [email protected]
> @@ -209,6 +220,7 @@ What: /sys/bus/counter/devices/counterX/countY/count_mode_component_id
> What: /sys/bus/counter/devices/counterX/countY/direction_component_id
> What: /sys/bus/counter/devices/counterX/countY/enable_component_id
> What: /sys/bus/counter/devices/counterX/countY/error_noise_component_id
> +What: /sys/bus/counter/devices/counterX/countY/latched_count_component_id
> What: /sys/bus/counter/devices/counterX/countY/prescaler_component_id
> What: /sys/bus/counter/devices/counterX/countY/preset_component_id
> What: /sys/bus/counter/devices/counterX/countY/preset_enable_component_id
> @@ -218,6 +230,7 @@ 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/latch_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
> @@ -244,6 +257,22 @@ Description:
> counter_event data structures. The number of elements will be
> rounded-up to a power of 2.
>
> +What: /sys/bus/counter/devices/counterX/latch_mode
> +KernelVersion: 5.16
> +Contact: [email protected]
> +Description:
> + Read/write attribute that selects the trigger for latching
> + values. Valid values are device-specific (given by
> + latch_mode_available attribute) and may include:
> +
> + "Read count":
> + Reading the countY/count attribute latches values.
> +
> + "Unit timeout":
> + Unit timer timeout event latches values.
> +
> + The latched values can be read from latched_* attributes.
> +
To make these modes more generic for use in future drivers, I suggest
removing the "Unit " prefix and just leaving that mode as "Timeout". In
a similar vein, rewording "Read count" to "Count read" would make this
mode easier to understand in case a future driver introduces a mode
called "Signal read" or similar.
William Breathitt Gray
> What: /sys/bus/counter/devices/counterX/name
> KernelVersion: 5.2
> Contact: [email protected]
> @@ -298,14 +327,6 @@ Description:
> Active level of index input Signal Y; irrelevant in
> non-synchronous load mode.
>
> -What: /sys/bus/counter/devices/counterX/signalY/index_polarity_available
> -What: /sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
> -KernelVersion: 5.2
> -Contact: [email protected]
> -Description:
> - Discrete set of available values for the respective Signal Y
> - configuration are listed in this file.
> -
> What: /sys/bus/counter/devices/counterX/signalY/name
> KernelVersion: 5.2
> Contact: [email protected]
> --
> 2.25.1
>
On 10/25/21 3:48 AM, William Breathitt Gray wrote:
> On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote:
>> This adds support to the TI eQEP counter driver for the Unit Timer.
>> The Unit Timer is a device-level extension that provides a timer to be
>> used for speed calculations. The sysfs interface for the Unit Timer is
>> new and will be documented in a later commit. It contains a R/W time
>> attribute for the current time, a R/W period attribute for the timeout
>> period and a R/W enable attribute to start/stop the timer. It also
>> implements a timeout event on the chrdev interface that is triggered
>> each time the period timeout is reached.
>>
>> Signed-off-by: David Lechner <[email protected]>
>
> I'll comment on the sysfs interface in the respective docs patch. Some
> comments regarding this patch below.
>
...
>> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter,
>> + u64 value)
>> +{
>> + struct ti_eqep_cnt *priv = counter->priv;
>> + u32 quprd;
>> +
>> + /* convert nanoseconds to timer ticks */
>> + quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
>> + if (quprd != value)
>> + return -ERANGE;
>> +
>> + /* protect against infinite unit timeout interrupts */
>> + if (quprd == 0)
>> + return -EINVAL;
>
> I doubt there's any practical reason for a user to set the timer period
> to 0, but perhaps we should not try to protect users from themselves
> here. It's very obvious and expected that setting the timer period to 0
> results in timeouts as quickly as possible, so really the user should be
> left to reap the fruits of their decision regardless of how asinine that
> decision is.
Even if the operating system ceases operation because the interrupt
handler keeps running because clearing the interrupt has no effect
in this condition?
...
>> @@ -500,6 +608,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> struct ti_eqep_cnt *priv;
>> + struct clk *clk;
>> void __iomem *base;
>> int err;
>> int irq;
>> @@ -508,6 +617,24 @@ static int ti_eqep_probe(struct platform_device *pdev)
>> if (!priv)
>> return -ENOMEM;
>>
>> + clk = devm_clk_get(dev, "sysclkout");
>> + if (IS_ERR(clk)) {
>> + if (PTR_ERR(clk) != -EPROBE_DEFER)
>> + dev_err(dev, "failed to get sysclkout");
>> + return PTR_ERR(clk);
>> + }
>> +
>> + priv->sysclkout_rate = clk_get_rate(clk);
>> + if (priv->sysclkout_rate == 0) {
>> + dev_err(dev, "failed to get sysclkout rate");
>> + /* prevent divide by zero */
>> + priv->sysclkout_rate = 1;
>> + /*
>> + * This error is not expected and the driver is mostly usable
>> + * without clock rate anyway, so don't exit here.
>> + */
>
> If the values for these new attributes are expected to be denominated in
> nanoseconds then we must guarantee that. You should certainly error out
> here if you can't guarantee such.
>
> Alternatively, you could provide an additional set of attributes that
> are denominated in units of raw timer ticks rather than nanoseconds.
> That way if you can't determine the clock rate you can simply have the
> nanosecond-denominated timer attributes return an EOPNOTSUPP error code
> or similar while still providing users with the raw timer ticks
> attributes.
I think we should just fail here.
On 10/27/21 2:54 AM, William Breathitt Gray wrote:
> On Sat, Oct 16, 2021 at 08:33:41PM -0500, David Lechner wrote:
>> This documents new counterX/latch_mode* and
>> counterX/countY/latched_count* attributes.
>>
>> The counterX/signalY/*_available are moved to the
>> counterX/countY/*_available section similar to the way we already have
>> The counterX/*_component_id and The counterX/signalY/*_component_id
>> grouped together so that we don't have to start a 3rd redundant section
>> for device-level *_available section.
>>
>> @@ -147,6 +150,14 @@ Description:
>> updates the respective count. Quadrature encoding
>> determines the direction.
>>
>> +What: /sys/bus/counter/devices/counterX/countY/latched_count
>> +KernelVersion: 5.16
>> +Contact: [email protected]
>> +Description:
>> + Latched count data of Count Y represented as a string. The value
>> + is latched in based on the trigger selected by the
>> + counterX/latch_mode attribute.
>> +
>
> Latches are pretty common components of devices, and not simply limited
> to latching the count data. I wonder if it would be better to omit the
> "_count" suffix in order to make this more general. Well, the name
> "latched_count" is suitable for counters so you probably don't need to
> change it, but it's something to think about in the future.
>
I chose the name counterX/countY/latched_count since we already have
counterX/countY/count to read the same (not latched) count. This
indicates that they are the same quantity, just from a different
point in time.
Also for consideration, this particular hardware actually has 3
independent latched counts. One is triggered by the selected
latched_mode. One is triggered by the index signal and one is
triggered by the strobe signal.
The latter two are not implemented in this series, but if there were a
use for those, I would probably submit attributes index_latched_count
and strobe_latched_count. These are unaffected by the latch_mode.
Similarly, the unit timer has a timer latch and a period latch. If we
change the unit timer to be a Count as suggested, then the latched
timer would basically be the same as latched_count. Both of these
are triggered by the selected latch_mode.
So, I supposed if we wanted to keep things really generic, we would
want to introduce some sort of "latch trigger" component (synapse?).
There could theoretically be multiple configurable triggers, so
the proposed latch_mode might need to be made indexed or part of
an index component/extension.
>> What: /sys/bus/counter/devices/counterX/countY/name
>> KernelVersion: 5.2
>> Contact: [email protected]
>> @@ -209,6 +220,7 @@ What: /sys/bus/counter/devices/counterX/countY/count_mode_component_id
>> What: /sys/bus/counter/devices/counterX/countY/direction_component_id
>> What: /sys/bus/counter/devices/counterX/countY/enable_component_id
>> What: /sys/bus/counter/devices/counterX/countY/error_noise_component_id
>> +What: /sys/bus/counter/devices/counterX/countY/latched_count_component_id
>> What: /sys/bus/counter/devices/counterX/countY/prescaler_component_id
>> What: /sys/bus/counter/devices/counterX/countY/preset_component_id
>> What: /sys/bus/counter/devices/counterX/countY/preset_enable_component_id
>> @@ -218,6 +230,7 @@ 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/latch_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
Just noticing here, I missed the counterX in the device-level components.
>> @@ -244,6 +257,22 @@ Description:
>> counter_event data structures. The number of elements will be
>> rounded-up to a power of 2.
>>
>> +What: /sys/bus/counter/devices/counterX/latch_mode
>> +KernelVersion: 5.16
>> +Contact: [email protected]
>> +Description:
>> + Read/write attribute that selects the trigger for latching
>> + values. Valid values are device-specific (given by
>> + latch_mode_available attribute) and may include:
>> +
>> + "Read count":
>> + Reading the countY/count attribute latches values.
>> +
>> + "Unit timeout":
>> + Unit timer timeout event latches values.
>> +
>> + The latched values can be read from latched_* attributes.
>> +
>
> To make these modes more generic for use in future drivers, I suggest
> removing the "Unit " prefix and just leaving that mode as "Timeout". In
> a similar vein, rewording "Read count" to "Count read" would make this
> mode easier to understand in case a future driver introduces a mode
> called "Signal read" or similar.
>
Continuing my thoughts from above and taking this suggestion into
consideration...
Maybe we need a /sys/bus/counter/devices/counterX/latchY component.
This would represent the trigger for a latch. For the TI eQEP in this
series, there are potentially 3 of these (only one implemented for
now).
latchY would have a required `trigger` attribute that would describe
what triggers the latch. If the trigger is selectable, there would be
a `triggers_available` attribute that would list the possible triggers,
otherwise the `trigger` attribute would just be read-only. Available
triggers could could be "X read" where X is a fully qualified component
name, e.g. "Count0 count read" or a fully qualified event, e.g.
"Count1 overflow event" (this is unit timer timeout in generic counter
terms). But, there may be potential triggers that don't fit either
of these patterns.
Although not currently needed, the triggers for the index and strobe
latches on the eQEP get more interesting. The `triggers_available` for
the index latch are "index rising edge", "index falling edge" and
"software" (this would require a `software_trigger` attribute that
would be written to trigger the latch). The `triggers_available` for
the strobe latch are "strobe rising edge" and "strobe rising edge if
direction is clockwise and strobe falling edge if direction is
counterclockwise".
Circling back to the beginning, to read latched registers, there
would be attributes like counterX/countY/latchY_count instead of
the proposed counterX/countY/latched_count. So for the eQEP there
would be counter0/count0/latch0_count (triggered by reading
counter0/count0/count or counter0/count1 overflow event),
counter0/count0/latch1_count (triggered by index signal),
counter0/count0/latch2_count (triggered by strobe signal),
counter0/count1/latch0_count (unit timer latched timer trigger
by same trigger as counter0/count0/latch0_count) and
counter0/count0/latch0_ceiling (unit timer latched period
triggered by same trigger as counter0/count0/latch0_count).
On 10/25/21 2:13 AM, William Breathitt Gray wrote:
> On Sat, Oct 16, 2021 at 08:33:36PM -0500, David Lechner wrote:
>> This adds support to the TI eQEP counter driver for subscribing to
>> overflow and underflow events using the counter chrdev interface.
>>
>> Since this is the first event added, this involved adding an irq
>> handler. Also, additional range checks had to be added to the ceiling
>> attribute to avoid infinite interrupts.
>>
>> Signed-off-by: David Lechner <[email protected]>
>
> Hi David,
>
> This looks functionally okay, but I have a couple minor comments inline.
>
>> ---
>> drivers/counter/ti-eqep.c | 119 +++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 117 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
>> index 09817c953f9a..b7c79435e127 100644
>> --- a/drivers/counter/ti-eqep.c
>> +++ b/drivers/counter/ti-eqep.c
>> @@ -7,6 +7,7 @@
>>
>> #include <linux/bitops.h>
>> #include <linux/counter.h>
>> +#include <linux/interrupt.h>
>> #include <linux/kernel.h>
>> #include <linux/mod_devicetable.h>
>> #include <linux/module.h>
>> @@ -67,6 +68,44 @@
>> #define QEPCTL_UTE BIT(1)
>> #define QEPCTL_WDE BIT(0)
>>
>> +#define QEINT_UTO BIT(11)
>> +#define QEINT_IEL BIT(10)
>> +#define QEINT_SEL BIT(9)
>> +#define QEINT_PCM BIT(8)
>> +#define QEINT_PCR BIT(7)
>> +#define QEINT_PCO BIT(6)
>> +#define QEINT_PCU BIT(5)
>> +#define QEINT_WTO BIT(4)
>> +#define QEINT_QDC BIT(3)
>> +#define QEINT_PHE BIT(2)
>> +#define QEINT_PCE BIT(1)
>> +
>> +#define QFLG_UTO BIT(11)
>> +#define QFLG_IEL BIT(10)
>> +#define QFLG_SEL BIT(9)
>> +#define QFLG_PCM BIT(8)
>> +#define QFLG_PCR BIT(7)
>> +#define QFLG_PCO BIT(6)
>> +#define QFLG_PCU BIT(5)
>> +#define QFLG_WTO BIT(4)
>> +#define QFLG_QDC BIT(3)
>> +#define QFLG_PHE BIT(2)
>> +#define QFLG_PCE BIT(1)
>> +#define QFLG_INT BIT(0)
>> +
>> +#define QCLR_UTO BIT(11)
>> +#define QCLR_IEL BIT(10)
>> +#define QCLR_SEL BIT(9)
>> +#define QCLR_PCM BIT(8)
>> +#define QCLR_PCR BIT(7)
>> +#define QCLR_PCO BIT(6)
>> +#define QCLR_PCU BIT(5)
>> +#define QCLR_WTO BIT(4)
>> +#define QCLR_QDC BIT(3)
>> +#define QCLR_PHE BIT(2)
>> +#define QCLR_PCE BIT(1)
>> +#define QCLR_INT BIT(0)
>> +
>> /* EQEP Inputs */
>> enum {
>> TI_EQEP_SIGNAL_QEPA, /* QEPA/XCLK */
>> @@ -233,12 +272,46 @@ static int ti_eqep_action_read(struct counter_device *counter,
>> }
>> }
>>
>> +static int ti_eqep_events_configure(struct counter_device *counter)
>> +{
>> + struct ti_eqep_cnt *priv = counter->priv;
>> + struct counter_event_node *event_node;
>> + u32 qeint = 0;
>> +
>> + list_for_each_entry(event_node, &counter->events_list, l) {
>> + switch (event_node->event) {
>> + case COUNTER_EVENT_OVERFLOW:
>> + qeint |= QEINT_PCO;
>> + break;
>> + case COUNTER_EVENT_UNDERFLOW:
>> + qeint |= QEINT_PCU;
>> + break;
>> + }
>> + }
>> +
>> + return regmap_write_bits(priv->regmap16, QEINT, ~0, qeint);
>> +}
>> +
>> +static int ti_eqep_watch_validate(struct counter_device *counter,
>> + const struct counter_watch *watch)
>> +{
>> + switch (watch->event) {
>> + case COUNTER_EVENT_OVERFLOW:
>> + case COUNTER_EVENT_UNDERFLOW:
>> + return 0;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> static const struct counter_ops ti_eqep_counter_ops = {
>> .count_read = ti_eqep_count_read,
>> .count_write = ti_eqep_count_write,
>> .function_read = ti_eqep_function_read,
>> .function_write = ti_eqep_function_write,
>> .action_read = ti_eqep_action_read,
>> + .events_configure = ti_eqep_events_configure,
>> + .watch_validate = ti_eqep_watch_validate,
>> };
>>
>> static int ti_eqep_position_ceiling_read(struct counter_device *counter,
>> @@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter,
>> u64 ceiling)
>> {
>> struct ti_eqep_cnt *priv = counter->priv;
>> + u32 qposmax = ceiling;
>>
>> - if (ceiling != (u32)ceiling)
>> + /* ensure that value fits in 32-bit register */
>> + if (qposmax != ceiling)
>> return -ERANGE;
>>
>> - regmap_write(priv->regmap32, QPOSMAX, ceiling);
>> + /* protect against infinite overflow interrupts */
>> + if (qposmax == 0)
>> + return -EINVAL;
>
> Would you be able to explain this scenario a bit further? My expectation
> would be that an overflow event would only occur if the position
> increased past the ceiling (i.e. increased to greater than 0). Of
> course, running the device with a ceiling of 0 effectively guarantees
> overflow eventss with every movement, but I would expect a stationary
> device to sit with a position of 0 and thus no overflow events.
>
This is just the way the hardware works. I discovered this the first
time I enabled interrupts. Even if you clear the interrupt, it is
triggered again immediately when QPOSMAX == 0.
On Wed, Oct 27, 2021 at 10:23:13AM -0500, David Lechner wrote:
> On 10/25/21 2:13 AM, William Breathitt Gray wrote:
> > On Sat, Oct 16, 2021 at 08:33:36PM -0500, David Lechner wrote:
> >> @@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter,
> >> u64 ceiling)
> >> {
> >> struct ti_eqep_cnt *priv = counter->priv;
> >> + u32 qposmax = ceiling;
> >>
> >> - if (ceiling != (u32)ceiling)
> >> + /* ensure that value fits in 32-bit register */
> >> + if (qposmax != ceiling)
> >> return -ERANGE;
> >>
> >> - regmap_write(priv->regmap32, QPOSMAX, ceiling);
> >> + /* protect against infinite overflow interrupts */
> >> + if (qposmax == 0)
> >> + return -EINVAL;
> >
> > Would you be able to explain this scenario a bit further? My expectation
> > would be that an overflow event would only occur if the position
> > increased past the ceiling (i.e. increased to greater than 0). Of
> > course, running the device with a ceiling of 0 effectively guarantees
> > overflow eventss with every movement, but I would expect a stationary
> > device to sit with a position of 0 and thus no overflow events.
> >
>
> This is just the way the hardware works. I discovered this the first
> time I enabled interrupts. Even if you clear the interrupt, it is
> triggered again immediately when QPOSMAX == 0.
For this device, does an overflow event occur once the count value
increases to equal the ceiling value, or once the count value increases
past the ceiling value?
The Counter interface defines ceiling as an inclusive upper limit (count
value is capable of reaching ceiling) and defines COUNTER_EVENT_OVERFLOW
as occuring when the count value increases past ceiling. I want to make
sure the ceiling extension and COUNTER_EVENT_OVERFLOW events for this
driver are behaving as expected of the Counter interface.
Let's use a non-zero example to be clear. Suppose we set ceiling equal
to 10. If count is currently at 9 and increases by 1, count should
become 10 and no COUNTER_EVENT_OVERFLOW event is expected to trigger; if
count is 10 and further increases, count should _not_ become 11 (staying
at 10 or starting over at the floor) but a COUNTER_EVENT_OVERFLOW event
does trigger. Does the driver behave like this currently?
William Breathitt Gray
On Wed, Oct 27, 2021 at 10:28:59AM -0500, David Lechner wrote:
> On 10/25/21 3:48 AM, William Breathitt Gray wrote:
> > On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote:
> >> This adds support to the TI eQEP counter driver for the Unit Timer.
> >> The Unit Timer is a device-level extension that provides a timer to be
> >> used for speed calculations. The sysfs interface for the Unit Timer is
> >> new and will be documented in a later commit. It contains a R/W time
> >> attribute for the current time, a R/W period attribute for the timeout
> >> period and a R/W enable attribute to start/stop the timer. It also
> >> implements a timeout event on the chrdev interface that is triggered
> >> each time the period timeout is reached.
> >>
> >> Signed-off-by: David Lechner <[email protected]>
> >
> > I'll comment on the sysfs interface in the respective docs patch. Some
> > comments regarding this patch below.
> >
>
> ...
>
> >> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter,
> >> + u64 value)
> >> +{
> >> + struct ti_eqep_cnt *priv = counter->priv;
> >> + u32 quprd;
> >> +
> >> + /* convert nanoseconds to timer ticks */
> >> + quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
> >> + if (quprd != value)
> >> + return -ERANGE;
> >> +
> >> + /* protect against infinite unit timeout interrupts */
> >> + if (quprd == 0)
> >> + return -EINVAL;
> >
> > I doubt there's any practical reason for a user to set the timer period
> > to 0, but perhaps we should not try to protect users from themselves
> > here. It's very obvious and expected that setting the timer period to 0
> > results in timeouts as quickly as possible, so really the user should be
> > left to reap the fruits of their decision regardless of how asinine that
> > decision is.
>
> Even if the operating system ceases operation because the interrupt
> handler keeps running because clearing the interrupt has no effect
> in this condition?
I don't disagree with you that configuring the device to repeatedly
timeout without pause would be a waste of system resources. However, it
is more appropriate for this protection to be located in a userspace
application rather than the driver code here.
The purpose of a driver is to expose the functionality of a device in an
understandable and consistent manner. Drivers should not dictate what a
user does with their device, but rather should help facilitate the
user's control so that the device behaves as would be expected given
such an interface.
For this particular case, the device is capable of sending an interrupt
when a timeout events occurs, and the timeout period can be adjusted;
setting the timeout period lower and lower results in less and less time
between timeout events. The behavior and result of setting the timeout
period lower is well-defined and predictable; it is intuitive that
configuring the timeout period to 0, the lowest value possible, results
in the shortest time possible between timeouts: no pause at all.
As long as the functionality of this device is exposed in such an
understandable and consistent manner, the driver succeeds in serving its
purpose. So I believe a timeout period of 0 is a valid configuration
for this driver to allow, albeit a seemingly pointless one for users to
actually choose. To that end, simply set the default value of QUPRD to
non-zero on probe() as you do already in this patch and let the user be
free to adjust if they so decide.
William Breathitt Gray
On 10/28/21 2:48 AM, William Breathitt Gray wrote:
> On Wed, Oct 27, 2021 at 10:28:59AM -0500, David Lechner wrote:
>> On 10/25/21 3:48 AM, William Breathitt Gray wrote:
>>> On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote:
>>>> This adds support to the TI eQEP counter driver for the Unit Timer.
>>>> The Unit Timer is a device-level extension that provides a timer to be
>>>> used for speed calculations. The sysfs interface for the Unit Timer is
>>>> new and will be documented in a later commit. It contains a R/W time
>>>> attribute for the current time, a R/W period attribute for the timeout
>>>> period and a R/W enable attribute to start/stop the timer. It also
>>>> implements a timeout event on the chrdev interface that is triggered
>>>> each time the period timeout is reached.
>>>>
>>>> Signed-off-by: David Lechner <[email protected]>
>>>
>>> I'll comment on the sysfs interface in the respective docs patch. Some
>>> comments regarding this patch below.
>>>
>>
>> ...
>>
>>>> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter,
>>>> + u64 value)
>>>> +{
>>>> + struct ti_eqep_cnt *priv = counter->priv;
>>>> + u32 quprd;
>>>> +
>>>> + /* convert nanoseconds to timer ticks */
>>>> + quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
>>>> + if (quprd != value)
>>>> + return -ERANGE;
>>>> +
>>>> + /* protect against infinite unit timeout interrupts */
>>>> + if (quprd == 0)
>>>> + return -EINVAL;
>>>
>>> I doubt there's any practical reason for a user to set the timer period
>>> to 0, but perhaps we should not try to protect users from themselves
>>> here. It's very obvious and expected that setting the timer period to 0
>>> results in timeouts as quickly as possible, so really the user should be
>>> left to reap the fruits of their decision regardless of how asinine that
>>> decision is.
>>
>> Even if the operating system ceases operation because the interrupt
>> handler keeps running because clearing the interrupt has no effect
>> in this condition?
>
> I don't disagree with you that configuring the device to repeatedly
> timeout without pause would be a waste of system resources. However, it
> is more appropriate for this protection to be located in a userspace
> application rather than the driver code here.
>
> The purpose of a driver is to expose the functionality of a device in an
> understandable and consistent manner. Drivers should not dictate what a
> user does with their device, but rather should help facilitate the
> user's control so that the device behaves as would be expected given
> such an interface.
>
> For this particular case, the device is capable of sending an interrupt
> when a timeout events occurs, and the timeout period can be adjusted;
> setting the timeout period lower and lower results in less and less time
> between timeout events. The behavior and result of setting the timeout
> period lower is well-defined and predictable; it is intuitive that
> configuring the timeout period to 0, the lowest value possible, results
> in the shortest time possible between timeouts: no pause at all.
>
> As long as the functionality of this device is exposed in such an
> understandable and consistent manner, the driver succeeds in serving its
> purpose. So I believe a timeout period of 0 is a valid configuration
> for this driver to allow, albeit a seemingly pointless one for users to
> actually choose. To that end, simply set the default value of QUPRD to
> non-zero on probe() as you do already in this patch and let the user be
> free to adjust if they so decide.
>
> William Breathitt Gray
>
I disagree. I consider this a crash. The system becomes completely
unusable and you have to pull power to turn it off, potentially
leading to data loss and disk corruption.
On Wed, Oct 27, 2021 at 12:00:24PM -0500, David Lechner wrote:
> On 10/27/21 2:54 AM, William Breathitt Gray wrote:
> > On Sat, Oct 16, 2021 at 08:33:41PM -0500, David Lechner wrote:
> >> @@ -147,6 +150,14 @@ Description:
> >> updates the respective count. Quadrature encoding
> >> determines the direction.
> >>
> >> +What: /sys/bus/counter/devices/counterX/countY/latched_count
> >> +KernelVersion: 5.16
> >> +Contact: [email protected]
> >> +Description:
> >> + Latched count data of Count Y represented as a string. The value
> >> + is latched in based on the trigger selected by the
> >> + counterX/latch_mode attribute.
> >> +
> >
> > Latches are pretty common components of devices, and not simply limited
> > to latching the count data. I wonder if it would be better to omit the
> > "_count" suffix in order to make this more general. Well, the name
> > "latched_count" is suitable for counters so you probably don't need to
> > change it, but it's something to think about in the future.
> >
>
> I chose the name counterX/countY/latched_count since we already have
> counterX/countY/count to read the same (not latched) count. This
> indicates that they are the same quantity, just from a different
> point in time.
>
> Also for consideration, this particular hardware actually has 3
> independent latched counts. One is triggered by the selected
> latched_mode. One is triggered by the index signal and one is
> triggered by the strobe signal.
>
> The latter two are not implemented in this series, but if there were a
> use for those, I would probably submit attributes index_latched_count
> and strobe_latched_count. These are unaffected by the latch_mode.
>
> Similarly, the unit timer has a timer latch and a period latch. If we
> change the unit timer to be a Count as suggested, then the latched
> timer would basically be the same as latched_count. Both of these
> are triggered by the selected latch_mode.
>
> So, I supposed if we wanted to keep things really generic, we would
> want to introduce some sort of "latch trigger" component (synapse?).
> There could theoretically be multiple configurable triggers, so
> the proposed latch_mode might need to be made indexed or part of
> an index component/extension.
Aside from deriving their latched values from the current and historical
count values, these latches don't seem to be related to Counters in an
operational sense; i.e. they don't really fit into the Counter subsystem
paradigm because they aren't functionally counters, but rather just use
the count values here as source data for their own operations. As such,
I'm not sure yet if they really belong in the Counter subsystem or
somewhere else in the IIO subsystem.
>
> >> What: /sys/bus/counter/devices/counterX/countY/name
> >> KernelVersion: 5.2
> >> Contact: [email protected]
> >> @@ -209,6 +220,7 @@ What: /sys/bus/counter/devices/counterX/countY/count_mode_component_id
> >> What: /sys/bus/counter/devices/counterX/countY/direction_component_id
> >> What: /sys/bus/counter/devices/counterX/countY/enable_component_id
> >> What: /sys/bus/counter/devices/counterX/countY/error_noise_component_id
> >> +What: /sys/bus/counter/devices/counterX/countY/latched_count_component_id
> >> What: /sys/bus/counter/devices/counterX/countY/prescaler_component_id
> >> What: /sys/bus/counter/devices/counterX/countY/preset_component_id
> >> What: /sys/bus/counter/devices/counterX/countY/preset_enable_component_id
> >> @@ -218,6 +230,7 @@ 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/latch_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
>
> Just noticing here, I missed the counterX in the device-level components.
>
> >> @@ -244,6 +257,22 @@ Description:
> >> counter_event data structures. The number of elements will be
> >> rounded-up to a power of 2.
> >>
> >> +What: /sys/bus/counter/devices/counterX/latch_mode
> >> +KernelVersion: 5.16
> >> +Contact: [email protected]
> >> +Description:
> >> + Read/write attribute that selects the trigger for latching
> >> + values. Valid values are device-specific (given by
> >> + latch_mode_available attribute) and may include:
> >> +
> >> + "Read count":
> >> + Reading the countY/count attribute latches values.
> >> +
> >> + "Unit timeout":
> >> + Unit timer timeout event latches values.
> >> +
> >> + The latched values can be read from latched_* attributes.
> >> +
> >
> > To make these modes more generic for use in future drivers, I suggest
> > removing the "Unit " prefix and just leaving that mode as "Timeout". In
> > a similar vein, rewording "Read count" to "Count read" would make this
> > mode easier to understand in case a future driver introduces a mode
> > called "Signal read" or similar.
> >
>
> Continuing my thoughts from above and taking this suggestion into
> consideration...
>
> Maybe we need a /sys/bus/counter/devices/counterX/latchY component.
> This would represent the trigger for a latch. For the TI eQEP in this
> series, there are potentially 3 of these (only one implemented for
> now).
>
> latchY would have a required `trigger` attribute that would describe
> what triggers the latch. If the trigger is selectable, there would be
> a `triggers_available` attribute that would list the possible triggers,
> otherwise the `trigger` attribute would just be read-only. Available
> triggers could could be "X read" where X is a fully qualified component
> name, e.g. "Count0 count read" or a fully qualified event, e.g.
> "Count1 overflow event" (this is unit timer timeout in generic counter
> terms). But, there may be potential triggers that don't fit either
> of these patterns.
>
> Although not currently needed, the triggers for the index and strobe
> latches on the eQEP get more interesting. The `triggers_available` for
> the index latch are "index rising edge", "index falling edge" and
> "software" (this would require a `software_trigger` attribute that
> would be written to trigger the latch). The `triggers_available` for
> the strobe latch are "strobe rising edge" and "strobe rising edge if
> direction is clockwise and strobe falling edge if direction is
> counterclockwise".
>
> Circling back to the beginning, to read latched registers, there
> would be attributes like counterX/countY/latchY_count instead of
> the proposed counterX/countY/latched_count. So for the eQEP there
> would be counter0/count0/latch0_count (triggered by reading
> counter0/count0/count or counter0/count1 overflow event),
> counter0/count0/latch1_count (triggered by index signal),
> counter0/count0/latch2_count (triggered by strobe signal),
> counter0/count1/latch0_count (unit timer latched timer trigger
> by same trigger as counter0/count0/latch0_count) and
> counter0/count0/latch0_ceiling (unit timer latched period
> triggered by same trigger as counter0/count0/latch0_count).
The complexity of configuration here is a good indication that these
latches deserve their own tree structure as you suggest. Furthermore, we
see that there at least three of these latches available for this
particular device, so just a single "latch_count" or similar will not be
sufficient -- enumeration of the form /sys/bus/../latchY or similar
would be prudent.
Jonathan, perhaps you have some insight here. From a functional aspect,
latches are not unique to counter devices, so I wonder if the IIO
subsytem has already encountered similar functionality amongst its
drivers. Essentially, a latch is just a memory buffer provided by the
device.
For the TI eQEP device here, the buffer for each latch provides a single
read-only value that is updated by the device; the update behavior can
be configured by respective control registers. However, it's not so
far-fetched to assume that there a other devices out that that have
buffers spanning multiple latched values storing historical data.
Because a latch can theoretically provide any sort of data, not
necessary count values, it seems reasonable that supporting latches
would involve their own interface independent of the Counter paradigm.
How that interface looks is the question. Should the TI eQEP latches
here be exposed through some sort of generic latches interface, or
would it be better to have a more abstract representation of what these
latches are for; e.g. if these latches are used to measure speed, then
some sort of IIO speedometer interface might be appropriate).
William Breathitt Gray
On Thu, Oct 28, 2021 at 08:42:49AM -0500, David Lechner wrote:
> On 10/28/21 2:48 AM, William Breathitt Gray wrote:
> > On Wed, Oct 27, 2021 at 10:28:59AM -0500, David Lechner wrote:
> >> On 10/25/21 3:48 AM, William Breathitt Gray wrote:
> >>> On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote:
> >>>> This adds support to the TI eQEP counter driver for the Unit Timer.
> >>>> The Unit Timer is a device-level extension that provides a timer to be
> >>>> used for speed calculations. The sysfs interface for the Unit Timer is
> >>>> new and will be documented in a later commit. It contains a R/W time
> >>>> attribute for the current time, a R/W period attribute for the timeout
> >>>> period and a R/W enable attribute to start/stop the timer. It also
> >>>> implements a timeout event on the chrdev interface that is triggered
> >>>> each time the period timeout is reached.
> >>>>
> >>>> Signed-off-by: David Lechner <[email protected]>
> >>>
> >>> I'll comment on the sysfs interface in the respective docs patch. Some
> >>> comments regarding this patch below.
> >>>
> >>
> >> ...
> >>
> >>>> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter,
> >>>> + u64 value)
> >>>> +{
> >>>> + struct ti_eqep_cnt *priv = counter->priv;
> >>>> + u32 quprd;
> >>>> +
> >>>> + /* convert nanoseconds to timer ticks */
> >>>> + quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
> >>>> + if (quprd != value)
> >>>> + return -ERANGE;
> >>>> +
> >>>> + /* protect against infinite unit timeout interrupts */
> >>>> + if (quprd == 0)
> >>>> + return -EINVAL;
> >>>
> >>> I doubt there's any practical reason for a user to set the timer period
> >>> to 0, but perhaps we should not try to protect users from themselves
> >>> here. It's very obvious and expected that setting the timer period to 0
> >>> results in timeouts as quickly as possible, so really the user should be
> >>> left to reap the fruits of their decision regardless of how asinine that
> >>> decision is.
> >>
> >> Even if the operating system ceases operation because the interrupt
> >> handler keeps running because clearing the interrupt has no effect
> >> in this condition?
> >
> > I don't disagree with you that configuring the device to repeatedly
> > timeout without pause would be a waste of system resources. However, it
> > is more appropriate for this protection to be located in a userspace
> > application rather than the driver code here.
> >
> > The purpose of a driver is to expose the functionality of a device in an
> > understandable and consistent manner. Drivers should not dictate what a
> > user does with their device, but rather should help facilitate the
> > user's control so that the device behaves as would be expected given
> > such an interface.
> >
> > For this particular case, the device is capable of sending an interrupt
> > when a timeout events occurs, and the timeout period can be adjusted;
> > setting the timeout period lower and lower results in less and less time
> > between timeout events. The behavior and result of setting the timeout
> > period lower is well-defined and predictable; it is intuitive that
> > configuring the timeout period to 0, the lowest value possible, results
> > in the shortest time possible between timeouts: no pause at all.
> >
> > As long as the functionality of this device is exposed in such an
> > understandable and consistent manner, the driver succeeds in serving its
> > purpose. So I believe a timeout period of 0 is a valid configuration
> > for this driver to allow, albeit a seemingly pointless one for users to
> > actually choose. To that end, simply set the default value of QUPRD to
> > non-zero on probe() as you do already in this patch and let the user be
> > free to adjust if they so decide.
> >
> > William Breathitt Gray
> >
>
> I disagree. I consider this a crash. The system becomes completely
> unusable and you have to pull power to turn it off, potentially
> leading to data loss and disk corruption.
I hope I'm not being excessively pedantic here -- I'll concede to a
third opion on this if someone else wants to chime in -- but I want to
ensure that we are not going outside the scope of what a driver should
do. Note that any device is capable of flooding the system with
interrupts (e.g. a counter operating on a high enough frequency
overflowing a low ceiling), so I don't think that reason alone will pass
muster. Nevertheless, it is important to define when a driver should
return an error or not.
Take for example, the period range check right above. If a user requests
the device do something it cannot, such as counting down from a period
value that is too high for it to represent internally, then it is
appropriate to return an error: the device cannot perform the request
and as such the request is not valid input for the driver.
However, when we apply the same method to the zero value case -- a user
requests a timeout period of 0 -- the device is capable of performing
that request: the device is capable of waiting 0 nanoseconds and as such
the request is a valid input for the driver because it can be performed
by the device exactly as expected by the user. As long as the behavior
of a request is well-defined, we must assume the user knows what they
are doing, and thus should be permitted to request their device perform
that behavior.
A driver should not speculate on the intent of a user's actions.
Restricting what a user can do with their device is a matter of
configuration policy, and configuration policy belongs appropriately in
userspace. Rather, the scope of a driver should be limited narrowly to
exposure of a device functionality in a standard and predictable way.
William Breathitt Gray
On Sat, 30 Oct 2021 10:32:57 +0900
William Breathitt Gray <[email protected]> wrote:
> On Wed, Oct 27, 2021 at 12:00:24PM -0500, David Lechner wrote:
> > On 10/27/21 2:54 AM, William Breathitt Gray wrote:
> > > On Sat, Oct 16, 2021 at 08:33:41PM -0500, David Lechner wrote:
> > >> @@ -147,6 +150,14 @@ Description:
> > >> updates the respective count. Quadrature encoding
> > >> determines the direction.
> > >>
> > >> +What: /sys/bus/counter/devices/counterX/countY/latched_count
> > >> +KernelVersion: 5.16
> > >> +Contact: [email protected]
> > >> +Description:
> > >> + Latched count data of Count Y represented as a string. The value
> > >> + is latched in based on the trigger selected by the
> > >> + counterX/latch_mode attribute.
> > >> +
> > >
> > > Latches are pretty common components of devices, and not simply limited
> > > to latching the count data. I wonder if it would be better to omit the
> > > "_count" suffix in order to make this more general. Well, the name
> > > "latched_count" is suitable for counters so you probably don't need to
> > > change it, but it's something to think about in the future.
> > >
> >
> > I chose the name counterX/countY/latched_count since we already have
> > counterX/countY/count to read the same (not latched) count. This
> > indicates that they are the same quantity, just from a different
> > point in time.
> >
> > Also for consideration, this particular hardware actually has 3
> > independent latched counts. One is triggered by the selected
> > latched_mode. One is triggered by the index signal and one is
> > triggered by the strobe signal.
> >
> > The latter two are not implemented in this series, but if there were a
> > use for those, I would probably submit attributes index_latched_count
> > and strobe_latched_count. These are unaffected by the latch_mode.
> >
> > Similarly, the unit timer has a timer latch and a period latch. If we
> > change the unit timer to be a Count as suggested, then the latched
> > timer would basically be the same as latched_count. Both of these
> > are triggered by the selected latch_mode.
> >
> > So, I supposed if we wanted to keep things really generic, we would
> > want to introduce some sort of "latch trigger" component (synapse?).
> > There could theoretically be multiple configurable triggers, so
> > the proposed latch_mode might need to be made indexed or part of
> > an index component/extension.
>
> Aside from deriving their latched values from the current and historical
> count values, these latches don't seem to be related to Counters in an
> operational sense; i.e. they don't really fit into the Counter subsystem
> paradigm because they aren't functionally counters, but rather just use
> the count values here as source data for their own operations. As such,
> I'm not sure yet if they really belong in the Counter subsystem or
> somewhere else in the IIO subsystem.
In this particular case I think we are talking about latching counts rather
than something else? So one event happens and we latch the count at that
point.
The IIO equivalent is a trigger event driving data into a buffer.
There are a few examples of this though it's pretty rare.
The most general corner case is probably what we see with impact sensors.
In those cases we have data captured around an event (rather than a single
latched value).
They are rather complex beasts but the best we've managed is a special
trigger used only with that device and some control attributes to say what
is captured when the trigger fires. Note this is a stetch in IIO because
normally triggers are one per sample...
>
> >
> > >> What: /sys/bus/counter/devices/counterX/countY/name
> > >> KernelVersion: 5.2
> > >> Contact: [email protected]
> > >> @@ -209,6 +220,7 @@ What: /sys/bus/counter/devices/counterX/countY/count_mode_component_id
> > >> What: /sys/bus/counter/devices/counterX/countY/direction_component_id
> > >> What: /sys/bus/counter/devices/counterX/countY/enable_component_id
> > >> What: /sys/bus/counter/devices/counterX/countY/error_noise_component_id
> > >> +What: /sys/bus/counter/devices/counterX/countY/latched_count_component_id
> > >> What: /sys/bus/counter/devices/counterX/countY/prescaler_component_id
> > >> What: /sys/bus/counter/devices/counterX/countY/preset_component_id
> > >> What: /sys/bus/counter/devices/counterX/countY/preset_enable_component_id
> > >> @@ -218,6 +230,7 @@ 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/latch_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
> >
> > Just noticing here, I missed the counterX in the device-level components.
> >
> > >> @@ -244,6 +257,22 @@ Description:
> > >> counter_event data structures. The number of elements will be
> > >> rounded-up to a power of 2.
> > >>
> > >> +What: /sys/bus/counter/devices/counterX/latch_mode
> > >> +KernelVersion: 5.16
> > >> +Contact: [email protected]
> > >> +Description:
> > >> + Read/write attribute that selects the trigger for latching
> > >> + values. Valid values are device-specific (given by
> > >> + latch_mode_available attribute) and may include:
> > >> +
> > >> + "Read count":
> > >> + Reading the countY/count attribute latches values.
> > >> +
> > >> + "Unit timeout":
> > >> + Unit timer timeout event latches values.
> > >> +
> > >> + The latched values can be read from latched_* attributes.
> > >> +
> > >
> > > To make these modes more generic for use in future drivers, I suggest
> > > removing the "Unit " prefix and just leaving that mode as "Timeout". In
> > > a similar vein, rewording "Read count" to "Count read" would make this
> > > mode easier to understand in case a future driver introduces a mode
> > > called "Signal read" or similar.
> > >
> >
> > Continuing my thoughts from above and taking this suggestion into
> > consideration...
> >
> > Maybe we need a /sys/bus/counter/devices/counterX/latchY component.
> > This would represent the trigger for a latch. For the TI eQEP in this
> > series, there are potentially 3 of these (only one implemented for
> > now).
> >
> > latchY would have a required `trigger` attribute that would describe
> > what triggers the latch. If the trigger is selectable, there would be
> > a `triggers_available` attribute that would list the possible triggers,
> > otherwise the `trigger` attribute would just be read-only. Available
> > triggers could could be "X read" where X is a fully qualified component
> > name, e.g. "Count0 count read" or a fully qualified event, e.g.
> > "Count1 overflow event" (this is unit timer timeout in generic counter
> > terms). But, there may be potential triggers that don't fit either
> > of these patterns.
> >
> > Although not currently needed, the triggers for the index and strobe
> > latches on the eQEP get more interesting. The `triggers_available` for
> > the index latch are "index rising edge", "index falling edge" and
> > "software" (this would require a `software_trigger` attribute that
> > would be written to trigger the latch). The `triggers_available` for
> > the strobe latch are "strobe rising edge" and "strobe rising edge if
> > direction is clockwise and strobe falling edge if direction is
> > counterclockwise".
> >
> > Circling back to the beginning, to read latched registers, there
> > would be attributes like counterX/countY/latchY_count instead of
> > the proposed counterX/countY/latched_count. So for the eQEP there
> > would be counter0/count0/latch0_count (triggered by reading
> > counter0/count0/count or counter0/count1 overflow event),
> > counter0/count0/latch1_count (triggered by index signal),
> > counter0/count0/latch2_count (triggered by strobe signal),
> > counter0/count1/latch0_count (unit timer latched timer trigger
> > by same trigger as counter0/count0/latch0_count) and
> > counter0/count0/latch0_ceiling (unit timer latched period
> > triggered by same trigger as counter0/count0/latch0_count).
>
> The complexity of configuration here is a good indication that these
> latches deserve their own tree structure as you suggest. Furthermore, we
> see that there at least three of these latches available for this
> particular device, so just a single "latch_count" or similar will not be
> sufficient -- enumeration of the form /sys/bus/../latchY or similar
> would be prudent.
>
> Jonathan, perhaps you have some insight here. From a functional aspect,
> latches are not unique to counter devices, so I wonder if the IIO
> subsytem has already encountered similar functionality amongst its
> drivers. Essentially, a latch is just a memory buffer provided by the
> device.
As mentioned above, they exist but are fairly rare, unless you think
of time triggers as being in this category in which case any data
ready signal is basically like this. Ignoring that common case,
we map them onto a device specific trigger (one that has a
validate_device callback to check it's being assigned to that right device).
Another case where we do odd things like this is SoC ADCs that support touch
screen type functionality. In those case we are grabbing data only when
the screen is touched.
>
> For the TI eQEP device here, the buffer for each latch provides a single
> read-only value that is updated by the device; the update behavior can
> be configured by respective control registers. However, it's not so
> far-fetched to assume that there a other devices out that that have
> buffers spanning multiple latched values storing historical data.
A fifo filled on 'index' event or similar would indeed be a reasonable
bit of hardware to build. I've world with PLC code that does this sort
of things so the requirements are there (tracking products on a conveyor
belt would be a classic case - you latch the count when a light gate is
broken).
>
> Because a latch can theoretically provide any sort of data, not
> necessary count values, it seems reasonable that supporting latches
> would involve their own interface independent of the Counter paradigm.
> How that interface looks is the question. Should the TI eQEP latches
> here be exposed through some sort of generic latches interface, or
> would it be better to have a more abstract representation of what these
> latches are for; e.g. if these latches are used to measure speed, then
> some sort of IIO speedometer interface might be appropriate).
I'd suggest trying to avoid being too generic about this. There might
be some reason to do it in the future but trying to define interfaces
across subsystems is a pain. More likely we'll get some bridging
type drivers to map between different abstractions if they are needed.
Jonathan
>
> William Breathitt Gray
On Sat, Oct 30, 2021 at 03:39:39PM +0100, Jonathan Cameron wrote:
> On Sat, 30 Oct 2021 10:32:57 +0900
> William Breathitt Gray <[email protected]> wrote:
>
> > On Wed, Oct 27, 2021 at 12:00:24PM -0500, David Lechner wrote:
> > > On 10/27/21 2:54 AM, William Breathitt Gray wrote:
> > > > On Sat, Oct 16, 2021 at 08:33:41PM -0500, David Lechner wrote:
> > > >> @@ -147,6 +150,14 @@ Description:
> > > >> updates the respective count. Quadrature encoding
> > > >> determines the direction.
> > > >>
> > > >> +What: /sys/bus/counter/devices/counterX/countY/latched_count
> > > >> +KernelVersion: 5.16
> > > >> +Contact: [email protected]
> > > >> +Description:
> > > >> + Latched count data of Count Y represented as a string. The value
> > > >> + is latched in based on the trigger selected by the
> > > >> + counterX/latch_mode attribute.
> > > >> +
> > > >
> > > > Latches are pretty common components of devices, and not simply limited
> > > > to latching the count data. I wonder if it would be better to omit the
> > > > "_count" suffix in order to make this more general. Well, the name
> > > > "latched_count" is suitable for counters so you probably don't need to
> > > > change it, but it's something to think about in the future.
> > > >
> > >
> > > I chose the name counterX/countY/latched_count since we already have
> > > counterX/countY/count to read the same (not latched) count. This
> > > indicates that they are the same quantity, just from a different
> > > point in time.
> > >
> > > Also for consideration, this particular hardware actually has 3
> > > independent latched counts. One is triggered by the selected
> > > latched_mode. One is triggered by the index signal and one is
> > > triggered by the strobe signal.
> > >
> > > The latter two are not implemented in this series, but if there were a
> > > use for those, I would probably submit attributes index_latched_count
> > > and strobe_latched_count. These are unaffected by the latch_mode.
> > >
> > > Similarly, the unit timer has a timer latch and a period latch. If we
> > > change the unit timer to be a Count as suggested, then the latched
> > > timer would basically be the same as latched_count. Both of these
> > > are triggered by the selected latch_mode.
> > >
> > > So, I supposed if we wanted to keep things really generic, we would
> > > want to introduce some sort of "latch trigger" component (synapse?).
> > > There could theoretically be multiple configurable triggers, so
> > > the proposed latch_mode might need to be made indexed or part of
> > > an index component/extension.
> >
> > Aside from deriving their latched values from the current and historical
> > count values, these latches don't seem to be related to Counters in an
> > operational sense; i.e. they don't really fit into the Counter subsystem
> > paradigm because they aren't functionally counters, but rather just use
> > the count values here as source data for their own operations. As such,
> > I'm not sure yet if they really belong in the Counter subsystem or
> > somewhere else in the IIO subsystem.
>
> In this particular case I think we are talking about latching counts rather
> than something else? So one event happens and we latch the count at that
> point.
It looks like the unit timer has latches for its time and period values
as well IIUC, but at this patch only implements support for the count
latch. I think if we were to implement support for these other latches,
we would probably expose them in a similar way and just call them
respectively latched_count, latched_period, etc.
> The IIO equivalent is a trigger event driving data into a buffer.
> There are a few examples of this though it's pretty rare.
> The most general corner case is probably what we see with impact sensors.
> In those cases we have data captured around an event (rather than a single
> latched value).
>
> They are rather complex beasts but the best we've managed is a special
> trigger used only with that device and some control attributes to say what
> is captured when the trigger fires. Note this is a stetch in IIO because
> normally triggers are one per sample...
Yes, complexity is something I would like to avoid. I don't want to
shoehorn functionality into the Counter subsystem if a simpler approach
is possible. However, it may be good news to hear that these kind of
complex devices are rarer, so a simple approach with the TI eQEP might
be the best option.
> >
> > >
> > > >> What: /sys/bus/counter/devices/counterX/countY/name
> > > >> KernelVersion: 5.2
> > > >> Contact: [email protected]
> > > >> @@ -209,6 +220,7 @@ What: /sys/bus/counter/devices/counterX/countY/count_mode_component_id
> > > >> What: /sys/bus/counter/devices/counterX/countY/direction_component_id
> > > >> What: /sys/bus/counter/devices/counterX/countY/enable_component_id
> > > >> What: /sys/bus/counter/devices/counterX/countY/error_noise_component_id
> > > >> +What: /sys/bus/counter/devices/counterX/countY/latched_count_component_id
> > > >> What: /sys/bus/counter/devices/counterX/countY/prescaler_component_id
> > > >> What: /sys/bus/counter/devices/counterX/countY/preset_component_id
> > > >> What: /sys/bus/counter/devices/counterX/countY/preset_enable_component_id
> > > >> @@ -218,6 +230,7 @@ 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/latch_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
> > >
> > > Just noticing here, I missed the counterX in the device-level components.
> > >
> > > >> @@ -244,6 +257,22 @@ Description:
> > > >> counter_event data structures. The number of elements will be
> > > >> rounded-up to a power of 2.
> > > >>
> > > >> +What: /sys/bus/counter/devices/counterX/latch_mode
> > > >> +KernelVersion: 5.16
> > > >> +Contact: [email protected]
> > > >> +Description:
> > > >> + Read/write attribute that selects the trigger for latching
> > > >> + values. Valid values are device-specific (given by
> > > >> + latch_mode_available attribute) and may include:
By the way, the latch_mode_available comment here can be removed as it's
already obvious the respective *_available sysfs attribute is where the
possible values are given.
> > > >> +
> > > >> + "Read count":
> > > >> + Reading the countY/count attribute latches values.
> > > >> +
> > > >> + "Unit timeout":
> > > >> + Unit timer timeout event latches values.
> > > >> +
> > > >> + The latched values can be read from latched_* attributes.
> > > >> +
> > > >
> > > > To make these modes more generic for use in future drivers, I suggest
> > > > removing the "Unit " prefix and just leaving that mode as "Timeout". In
> > > > a similar vein, rewording "Read count" to "Count read" would make this
> > > > mode easier to understand in case a future driver introduces a mode
> > > > called "Signal read" or similar.
> > > >
> > >
> > > Continuing my thoughts from above and taking this suggestion into
> > > consideration...
> > >
> > > Maybe we need a /sys/bus/counter/devices/counterX/latchY component.
> > > This would represent the trigger for a latch. For the TI eQEP in this
> > > series, there are potentially 3 of these (only one implemented for
> > > now).
> > >
> > > latchY would have a required `trigger` attribute that would describe
> > > what triggers the latch. If the trigger is selectable, there would be
> > > a `triggers_available` attribute that would list the possible triggers,
> > > otherwise the `trigger` attribute would just be read-only. Available
> > > triggers could could be "X read" where X is a fully qualified component
> > > name, e.g. "Count0 count read" or a fully qualified event, e.g.
> > > "Count1 overflow event" (this is unit timer timeout in generic counter
> > > terms). But, there may be potential triggers that don't fit either
> > > of these patterns.
> > >
> > > Although not currently needed, the triggers for the index and strobe
> > > latches on the eQEP get more interesting. The `triggers_available` for
> > > the index latch are "index rising edge", "index falling edge" and
> > > "software" (this would require a `software_trigger` attribute that
> > > would be written to trigger the latch). The `triggers_available` for
> > > the strobe latch are "strobe rising edge" and "strobe rising edge if
> > > direction is clockwise and strobe falling edge if direction is
> > > counterclockwise".
> > >
> > > Circling back to the beginning, to read latched registers, there
> > > would be attributes like counterX/countY/latchY_count instead of
> > > the proposed counterX/countY/latched_count. So for the eQEP there
> > > would be counter0/count0/latch0_count (triggered by reading
> > > counter0/count0/count or counter0/count1 overflow event),
> > > counter0/count0/latch1_count (triggered by index signal),
> > > counter0/count0/latch2_count (triggered by strobe signal),
> > > counter0/count1/latch0_count (unit timer latched timer trigger
> > > by same trigger as counter0/count0/latch0_count) and
> > > counter0/count0/latch0_ceiling (unit timer latched period
> > > triggered by same trigger as counter0/count0/latch0_count).
> >
> > The complexity of configuration here is a good indication that these
> > latches deserve their own tree structure as you suggest. Furthermore, we
> > see that there at least three of these latches available for this
> > particular device, so just a single "latch_count" or similar will not be
> > sufficient -- enumeration of the form /sys/bus/../latchY or similar
> > would be prudent.
> >
> > Jonathan, perhaps you have some insight here. From a functional aspect,
> > latches are not unique to counter devices, so I wonder if the IIO
> > subsytem has already encountered similar functionality amongst its
> > drivers. Essentially, a latch is just a memory buffer provided by the
> > device.
>
> As mentioned above, they exist but are fairly rare, unless you think
> of time triggers as being in this category in which case any data
> ready signal is basically like this. Ignoring that common case,
> we map them onto a device specific trigger (one that has a
> validate_device callback to check it's being assigned to that right device).
> Another case where we do odd things like this is SoC ADCs that support touch
> screen type functionality. In those case we are grabbing data only when
> the screen is touched.
>
>
> >
> > For the TI eQEP device here, the buffer for each latch provides a single
> > read-only value that is updated by the device; the update behavior can
> > be configured by respective control registers. However, it's not so
> > far-fetched to assume that there a other devices out that that have
> > buffers spanning multiple latched values storing historical data.
>
> A fifo filled on 'index' event or similar would indeed be a reasonable
> bit of hardware to build. I've world with PLC code that does this sort
> of things so the requirements are there (tracking products on a conveyor
> belt would be a classic case - you latch the count when a light gate is
> broken).
I've constructed similar devices where I push the current count to a
fifo to track a part's relative position to others on a conveyor belt.
It's possible some quadrature encoding device out has a similar
functionality, but I don't want to jump the gun and start developing an
interface for this if those kinds of device are so rare as to not be
worth the effort.
> > Because a latch can theoretically provide any sort of data, not
> > necessary count values, it seems reasonable that supporting latches
> > would involve their own interface independent of the Counter paradigm.
> > How that interface looks is the question. Should the TI eQEP latches
> > here be exposed through some sort of generic latches interface, or
> > would it be better to have a more abstract representation of what these
> > latches are for; e.g. if these latches are used to measure speed, then
> > some sort of IIO speedometer interface might be appropriate).
>
> I'd suggest trying to avoid being too generic about this. There might
> be some reason to do it in the future but trying to define interfaces
> across subsystems is a pain. More likely we'll get some bridging
> type drivers to map between different abstractions if they are needed.
>
> Jonathan
I agree, after considering this some more I think it's best to keep this
specific to the use case of the TI eQEP for now. If something more
complex appears in the future then we can address the possibility of a
deadicated interface, but for the time being I think simple extensions
to the Counter subsystem will suffice.
Instead of treating each latch with its own enumerated tree structure,
let's go with the simple approach taken initially here:
* counterX/countY/latched_count
* counterX/latch_mode
If desired, a latched_time can be implemented as well; the edge capture
attributes can be exposed in a similar fashion (e.g. latched_period).
Whether latch_mode belongs as a device extension or Count extension is
something I'm still contemplating. If we expose the unit timer under its
own counterX/countY, then latch_mode should be a device extension
because latched_time would simply become latched_count for the unit
timer. On the other hand, if unit timer is not exposed like that, then
perhaps it makes more sense to move latch_mode to counterX/countY, and
introduce latched_time and latched_period under there as well.
William Breathitt Gray