[PATCH v5 0/3] ECAP support on TI AM62x SoC

[PATCH v5 0/3] ECAP support on TI AM62x SoC

[Julien Panis]

The Enhanced Capture (ECAP) module can be used to timestamp events
detected on signal input pin. It can be used for time measurements
of pulse train signals.

ECAP module includes 4 timestamp capture registers. For all 4 sequenced
timestamp capture events (1->2->3->4->1->...), edge polarity (falling/rising
edge) can be selected.

This driver leverages counter subsystem to :
- select edge polarity for all 4 capture events (event mode)
- log timestamps for each capture event
Event polarity, and CAP1/2/3/4 timestamps give all the information
about the input pulse train. Further information can easily be computed :
period and/or duty cycle if frequency is constant, elapsed time between
pulses, etc...

Modifications since v4:
	- Modify yaml commit message prefix (dt-bindings)
	- Modify driver file name & Makefile (ti-ecap-capture)
	- Modify compilation flag name in Kconfig (TI_ECAP_CAPTURE)
	- Select REGMAP_MMIO in Kconfig
	- Add capture items to sysfs-bus-counter ABI documentation
	- Cleanup probe function (dev_err_probe & devm_clk_get_enabled & devm_add_action_or_reset for PM)
	- Enable/Disable device clock in suspend/resume functions
	- Add PM explanations
	- Add ECAP clock signal & 'frequency' sysfs entry
	- Replace elapsed_time & spinlock by nb_ovf (atomic_t) & 'count_cumul' sysfs entry
	- Add counter overflow event
	- Modify 'name' sysfs entry for signal0 & signal1 & count0
	- Modify watch_validate function
	- Add macros for callbacks related to cap1/2/3/4

Userspace commands :
	### SIGNAL INPUT ###
	cd /sys/bus/counter/devices/counter0/signal0

	# Get available polarities for each capture event
	cat polarity1_available
	cat polarity2_available
	cat polarity3_available
	cat polarity4_available

	# Get polarity for each capture event
	cat polarity1
	cat polarity2
	cat polarity3
	cat polarity4

	# Set polarity for each capture event
	echo rising edge > polarity1
	echo falling edge > polarity2
	echo rising edge > polarity3
	echo falling edge > polarity4

	### SIGNAL CLOCK ###
	cd /sys/bus/counter/devices/counter0/signal1

	# Get clock rate
	cat frequency

	### COUNT ###
	cd /sys/bus/counter/devices/counter0/count0

	# Run ECAP
	echo 1 > enable

	# Get current timebase counter value
	cat count

	# Get cumulated counter value
	cat count_cumul

	# Get captured timestamps
	cat capture1
	cat capture2
	cat capture3
	cat capture4

	# Note that counter watches can also be used to get
	# data from userspace application
	# -> see tools/counter/counter_example.c

	# Stop ECAP
	echo 0 > enable

Julien Panis (3):
  dt-bindings: counter: add ti,am62-ecap-capture.yaml
  Documentation: ABI: sysfs-bus-counter: add capture items
  counter: ti-ecap-capture: capture driver support for ECAP

 Documentation/ABI/testing/sysfs-bus-counter   |  49 ++
 .../counter/ti,am62-ecap-capture.yaml         |  61 ++
 drivers/counter/Kconfig                       |  15 +
 drivers/counter/Makefile                      |   1 +
 drivers/counter/ti-ecap-capture.c             | 624 ++++++++++++++++++
 include/uapi/linux/counter.h                  |   2 +
 6 files changed, 752 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml
 create mode 100644 drivers/counter/ti-ecap-capture.c

-- 
2.25.1

[PATCH v5 1/3] dt-bindings: counter: add ti,am62-ecap-capture.yaml

[Julien Panis]

This commit adds a YAML binding for TI ECAP used in capture operating mode.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../counter/ti,am62-ecap-capture.yaml         | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml

diff --git a/Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml b/Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml
new file mode 100644
index 000000000000..4e0b2d2b303e
--- /dev/null
+++ b/Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/counter/ti,am62-ecap-capture.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments Enhanced Capture (eCAP) Module
+
+maintainers:
+  - Julien Panis <jpanis@baylibre.com>
+
+description: |
+  The eCAP module resources can be used to capture timestamps
+  on input signal events (falling/rising edges).
+
+properties:
+  compatible:
+    const: ti,am62-ecap-capture
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: fck
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        capture@23100000 { /* eCAP in capture mode on am62x */
+            compatible = "ti,am62-ecap-capture";
+            reg = <0x00 0x23100000 0x00 0x100>;
+            interrupts = <GIC_SPI 113 IRQ_TYPE_EDGE_RISING>;
+            power-domains = <&k3_pds 51 TI_SCI_PD_EXCLUSIVE>;
+            clocks = <&k3_clks 51 0>;
+            clock-names = "fck";
+        };
+    };
-- 
2.25.1

[PATCH v5 2/3] Documentation: ABI: sysfs-bus-counter: add capture items

[Julien Panis]

This commit adds capture items to counter ABI file
(e.g. TI ECAP used in capture operating mode).

Signed-off-by: Julien Panis <jpanis@baylibre.com>
---
 Documentation/ABI/testing/sysfs-bus-counter | 49 +++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
index 06c2b3e27e0b..52ddec7cc76a 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter
@@ -203,6 +203,24 @@ Description:
 		both edges:
 			Any state transition.
 
+What:		/sys/bus/counter/devices/counterX/countY/count_cumul
+KernelVersion:	6.0
+Contact:	jpanis@baylibre.com
+Description:
+		Read-only attribute that indicates the raw cumulated count
+		since count Y start, computed as follow:
+		count_cumul = (max_counter_val + 1) * nb_counter_overflows
+
+What:		/sys/bus/counter/devices/counterX/countY/captureZ
+KernelVersion:	6.0
+Contact:	jpanis@baylibre.com
+Description:
+		Read-only attributes that indicate the last raw timestamp captured
+		for the respective capture Z register.
+		Raw timestamp can be converted to nanoseconds as follow:
+		time_ns = 10^9 * (captureZ + count_cumul) / frequency
+		count_cumul and frequency parameters are described in this document.
+
 What:		/sys/bus/counter/devices/counterX/countY/ceiling_component_id
 What:		/sys/bus/counter/devices/counterX/countY/floor_component_id
 What:		/sys/bus/counter/devices/counterX/countY/count_mode_component_id
@@ -213,6 +231,8 @@ 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
 What:		/sys/bus/counter/devices/counterX/countY/signalZ_action_component_id
+What:		/sys/bus/counter/devices/counterX/countY/count_cumul_component_id
+What:		/sys/bus/counter/devices/counterX/countY/captureZ_component_id
 What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_component_id
 What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
 What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
@@ -345,3 +365,32 @@ Description:
 			via index_polarity. The index function (as enabled via
 			preset_enable) is performed synchronously with the
 			quadrature clock on the active level of the index input.
+
+What:		/sys/bus/counter/devices/counterX/signalY/polarityZ
+KernelVersion:	6.0
+Contact:	jpanis@baylibre.com
+Description:
+		Capture modules include N timestamp capture registers.
+		For all N sequenced timestamp capture events
+		(1->2->3->...->N->1->...), edge polarity can be selected.
+		The following polarities are available:
+
+		rising edge:
+			Low state transitions to high state.
+
+		falling edge:
+			High state transitions to low state.
+
+What:		/sys/bus/counter/devices/counterX/signalY/polarityZ_available
+KernelVersion:	6.0
+Contact:	jpanis@baylibre.com
+Description:
+		Discrete set of available values for the respective polarity Z
+		configuration are listed in this file. Values are delimited by
+		newline characters.
+
+What:		/sys/bus/counter/devices/counterX/signalY/frequency
+KernelVersion:	6.0
+Contact:	jpanis@baylibre.com
+Description:
+		Read-only attribute that indicates the signal Y frequency, in Hz.
-- 
2.25.1

Re: [PATCH v5 2/3] Documentation: ABI: sysfs-bus-counter: add capture items

[William Breathitt Gray]

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

On Wed, Aug 17, 2022 at 04:16:19PM +0200, Julien Panis wrote:
> This commit adds capture items to counter ABI file
> (e.g. TI ECAP used in capture operating mode).
> 
> Signed-off-by: Julien Panis <jpanis@baylibre.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-counter | 49 +++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index 06c2b3e27e0b..52ddec7cc76a 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -203,6 +203,24 @@ Description:
>  		both edges:
>  			Any state transition.
>  
> +What:		/sys/bus/counter/devices/counterX/countY/count_cumul
> +KernelVersion:	6.0
> +Contact:	jpanis@baylibre.com
> +Description:
> +		Read-only attribute that indicates the raw cumulated count
> +		since count Y start, computed as follow:
> +		count_cumul = (max_counter_val + 1) * nb_counter_overflows

Implement this instead as "num_overflows" (COUNTER_COMP_COUNT_U64) and
"ceiling" (COUNTER_COMP_CEILING). You need to expose the count ceiling
anyway and a new "num_overflows" attribute would be useful for other
drivers as well.

> +What:		/sys/bus/counter/devices/counterX/countY/captureZ
> +KernelVersion:	6.0
> +Contact:	jpanis@baylibre.com
> +Description:
> +		Read-only attributes that indicate the last raw timestamp captured
> +		for the respective capture Z register.
> +		Raw timestamp can be converted to nanoseconds as follow:
> +		time_ns = 10^9 * (captureZ + count_cumul) / frequency
> +		count_cumul and frequency parameters are described in this document.

Rework this description to make it more general: remove the nanoseconds
conversion formula and references to timestamps. The concept of a
capture is general enough that it could be provided by counter devices
that are not necessarily timestamping. Instead this description should
simply state that this attribute is a historical capture of the Count Y
count data where Z (if present) is the respective capture buffer element
offset.

>  What:		/sys/bus/counter/devices/counterX/countY/ceiling_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/floor_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/count_mode_component_id
> @@ -213,6 +231,8 @@ 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
>  What:		/sys/bus/counter/devices/counterX/countY/signalZ_action_component_id
> +What:		/sys/bus/counter/devices/counterX/countY/count_cumul_component_id
> +What:		/sys/bus/counter/devices/counterX/countY/captureZ_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
> @@ -345,3 +365,32 @@ Description:
>  			via index_polarity. The index function (as enabled via
>  			preset_enable) is performed synchronously with the
>  			quadrature clock on the active level of the index input.
> +
> +What:		/sys/bus/counter/devices/counterX/signalY/polarityZ
> +KernelVersion:	6.0
> +Contact:	jpanis@baylibre.com
> +Description:
> +		Capture modules include N timestamp capture registers.
> +		For all N sequenced timestamp capture events
> +		(1->2->3->...->N->1->...), edge polarity can be selected.
> +		The following polarities are available:
> +
> +		rising edge:
> +			Low state transitions to high state.
> +
> +		falling edge:
> +			High state transitions to low state.

Other drivers could use "polarity" for signals that are not related to
captures, so remove the language referencing timestamping and generalize
it to simply refer to selecting the Signal Y edge polarity where Z (if
present) is the respective polarity sequence position.

William Breathitt Gray

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

[PATCH v5 3/3] counter: ti-ecap-capture: capture driver support for ECAP

[Julien Panis]

ECAP hardware on TI AM62x SoC supports capture feature. It can be used
to timestamp events (falling/rising edges) detected on signal input pin.

This commit adds capture driver support for ECAP hardware on AM62x SoC.

In the ECAP hardware, capture pin can also be configured to be in
PWM mode. Current implementation only supports capture operating mode.
Hardware also supports timebase sync between multiple instances, but
this driver supports simple independent capture functionality.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
---
 drivers/counter/Kconfig           |  15 +
 drivers/counter/Makefile          |   1 +
 drivers/counter/ti-ecap-capture.c | 624 ++++++++++++++++++++++++++++++
 include/uapi/linux/counter.h      |   2 +
 4 files changed, 642 insertions(+)
 create mode 100644 drivers/counter/ti-ecap-capture.c

diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 5edd155f1911..08235268af0b 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -101,4 +101,19 @@ config INTEL_QEP
 	  To compile this driver as a module, choose M here: the module
 	  will be called intel-qep.
 
+config TI_ECAP_CAPTURE
+	tristate "TI eCAP capture driver"
+	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
+	depends on HAS_IOMEM
+	select REGMAP_MMIO
+	help
+	  Select this option to enable the Texas Instruments Enhanced Capture
+	  (eCAP) driver in input mode.
+
+	  It can be used to timestamp events (falling/rising edges) detected
+	  on signal input pin.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called ti-ecap-capture.
+
 endif # COUNTER
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index 8fde6c100ebc..b9a369e0d4fc 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
 obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
 obj-$(CONFIG_MICROCHIP_TCB_CAPTURE)	+= microchip-tcb-capture.o
 obj-$(CONFIG_INTEL_QEP)		+= intel-qep.o
+obj-$(CONFIG_TI_ECAP_CAPTURE)	+= ti-ecap-capture.o
diff --git a/drivers/counter/ti-ecap-capture.c b/drivers/counter/ti-ecap-capture.c
new file mode 100644
index 000000000000..b00ddf122bbd
--- /dev/null
+++ b/drivers/counter/ti-ecap-capture.c
@@ -0,0 +1,624 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * ECAP Capture driver
+ *
+ * Copyright (C) 2022 Julien Panis <jpanis@baylibre.com>
+ */
+
+#include <linux/atomic.h>
+#include <linux/clk.h>
+#include <linux/counter.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#define ECAP_DRV_NAME "ecap"
+
+/* ECAP event IDs */
+enum ecap_event_id {
+	/* Capture events */
+	ECAP_CEVT1,
+	ECAP_CEVT2,
+	ECAP_CEVT3,
+	ECAP_CEVT4,
+	/* Counter overflow event */
+	ECAP_CNTOVF,
+	/* Helpers for capture events */
+	ECAP_CEVT_LAST = ECAP_CEVT4,
+	ECAP_NB_CEVT = ECAP_CEVT_LAST + 1,
+	/* Helpers for all events */
+	ECAP_EVT_LAST = ECAP_CNTOVF,
+	ECAP_NB_EVT = ECAP_EVT_LAST + 1,
+};
+
+/* Registers */
+#define ECAP_NB_CAP			ECAP_NB_CEVT
+
+#define ECAP_TSCNT_REG			0x00
+
+#define ECAP_CAP_REG(i)		(((i) << 2) + 0x08)
+
+#define ECAP_ECCTL_REG			0x28
+#define ECAP_CAPPOL_BIT(i)		BIT((i) << 1)
+#define ECAP_EV_MODE_MASK		GENMASK(7, 0)
+#define ECAP_CAPLDEN_BIT		BIT(8)
+#define ECAP_CONT_ONESHT_BIT		BIT(16)
+#define ECAP_STOPVALUE_MASK		GENMASK(18, 17)
+#define ECAP_REARM_RESET_BIT		BIT(19)
+#define ECAP_TSCNTSTP_BIT		BIT(20)
+#define ECAP_SYNCO_DIS_MASK		GENMASK(23, 22)
+#define ECAP_CAP_APWM_BIT		BIT(25)
+#define ECAP_ECCTL_EN_MASK		(ECAP_CAPLDEN_BIT | ECAP_TSCNTSTP_BIT)
+#define ECAP_ECCTL_CFG_MASK		(ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK	\
+					| ECAP_ECCTL_EN_MASK | ECAP_REARM_RESET_BIT	\
+					| ECAP_CAP_APWM_BIT | ECAP_CONT_ONESHT_BIT)
+
+#define ECAP_ECINT_EN_FLG_REG		0x2c
+#define ECAP_EVT_EN_MASK		GENMASK(ECAP_NB_EVT, ECAP_NB_CEVT)
+#define ECAP_EVT_FLG_BIT(i)		BIT((i) + 17)
+
+#define ECAP_ECINT_CLR_FRC_REG	0x30
+#define ECAP_INT_CLR_BIT		BIT(0)
+#define ECAP_EVT_CLR_BIT(i)		BIT((i) + 1)
+#define ECAP_EVT_CLR_MASK		GENMASK(ECAP_NB_EVT, 0)
+
+#define ECAP_PID_REG			0x5c
+
+/*
+ * Event modes
+ * One bit for each CAPx register : 1 = falling edge / 0 = rising edge
+ * e.g. mode = 13 = 0xd = 0b1101
+ * -> falling edge for CAP1-3-4 / rising edge for CAP2
+ */
+#define ECAP_EV_MODE_BIT(i)		BIT(i)
+
+/* ECAP signals */
+#define ECAP_INPUT_SIG 0
+#define ECAP_CLOCK_SIG 1
+
+#define ECAP_CHAN 0
+
+static const struct regmap_config ecap_cnt_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.max_register = ECAP_PID_REG,
+};
+
+/**
+ * struct ecap_cnt_dev - device private data structure
+ * @enabled:  device state
+ * @clk:      device clock
+ * @clk_rate: device clock rate
+ * @regmap:   device register map
+ * @nb_ovf:   number of overflows since capture start
+ * @pm_ctx:   device context for PM operations
+ */
+struct ecap_cnt_dev {
+	bool enabled;
+	struct clk *clk;
+	unsigned long clk_rate;
+	struct regmap *regmap;
+	atomic_t nb_ovf;
+	struct {
+		u8 ev_mode;
+		unsigned int time_cntr;
+	} pm_ctx;
+};
+
+static u8 ecap_cnt_capture_get_evmode(struct counter_device *counter)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+	u8 ev_mode = 0;
+	unsigned int regval;
+	int i;
+
+	pm_runtime_get_sync(counter->parent);
+	regmap_read(ecap_dev->regmap, ECAP_ECCTL_REG, &regval);
+	pm_runtime_put_sync(counter->parent);
+
+	for (i = 0 ; i < ECAP_NB_CAP ; i++) {
+		if (regval & ECAP_CAPPOL_BIT(i))
+			ev_mode |= ECAP_EV_MODE_BIT(i);
+	}
+
+	return ev_mode;
+}
+
+static void ecap_cnt_capture_set_evmode(struct counter_device *counter, u8 ev_mode)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+	unsigned int regval = 0;
+	int i;
+
+	for (i = 0 ; i < ECAP_NB_CAP ; i++) {
+		if (ev_mode & ECAP_EV_MODE_BIT(i))
+			regval |= ECAP_CAPPOL_BIT(i);
+	}
+
+	pm_runtime_get_sync(counter->parent);
+	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_EV_MODE_MASK, regval);
+	pm_runtime_put_sync(counter->parent);
+}
+
+static void ecap_cnt_capture_enable(struct counter_device *counter, bool rearm)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+	unsigned int regval;
+
+	pm_runtime_get_sync(counter->parent);
+
+	/* Enable interrupts on events */
+	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG,
+			   ECAP_EVT_EN_MASK, ECAP_EVT_EN_MASK);
+
+	/* Run counter */
+	regval = ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK;
+	if (rearm) {
+		atomic_set(&ecap_dev->nb_ovf, 0);
+		regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, 0);
+		regval |= ECAP_REARM_RESET_BIT;
+	}
+	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_CFG_MASK, regval);
+}
+
+static void ecap_cnt_capture_disable(struct counter_device *counter)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+
+	/* Disable interrupts on events */
+	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, ECAP_EVT_EN_MASK, 0);
+
+	/* Stop counter */
+	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_EN_MASK, 0);
+
+	pm_runtime_put_sync(counter->parent);
+}
+
+static int ecap_cnt_count_get_val(struct counter_device *counter, unsigned int reg, u64 *val)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+	unsigned int regval;
+
+	pm_runtime_get_sync(counter->parent);
+	regmap_read(ecap_dev->regmap, reg, &regval);
+	pm_runtime_put_sync(counter->parent);
+
+	*val = regval;
+
+	return 0;
+}
+
+static inline int ecap_cnt_count_read(struct counter_device *counter,
+				      struct counter_count *count,
+				      u64 *val)
+{
+	return ecap_cnt_count_get_val(counter, ECAP_TSCNT_REG, val);
+}
+
+static int ecap_cnt_function_read(struct counter_device *counter,
+				  struct counter_count *count,
+				  enum counter_function *function)
+{
+	*function = COUNTER_FUNCTION_INCREASE;
+
+	return 0;
+}
+
+static int ecap_cnt_action_read(struct counter_device *counter,
+				struct counter_count *count,
+				struct counter_synapse *synapse,
+				enum counter_synapse_action *action)
+{
+	*action = COUNTER_SYNAPSE_ACTION_NONE;
+
+	return 0;
+}
+
+static int ecap_cnt_watch_validate(struct counter_device *counter,
+				   const struct counter_watch *watch)
+{
+	return (watch->channel == ECAP_CHAN &&
+		(watch->event == COUNTER_EVENT_CAPTURE ||
+		 watch->event == COUNTER_EVENT_OVERFLOW)) ? 0 : -EINVAL;
+}
+
+static int ecap_cnt_enable_read(struct counter_device *counter,
+				struct counter_count *count,
+				u8 *enable)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+
+	*enable = ecap_dev->enabled;
+
+	return 0;
+}
+
+static int ecap_cnt_enable_write(struct counter_device *counter,
+				 struct counter_count *count,
+				 u8 enable)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+
+	if (enable == ecap_dev->enabled)
+		return 0;
+	if (enable)
+		ecap_cnt_capture_enable(counter, true);
+	else
+		ecap_cnt_capture_disable(counter);
+	ecap_dev->enabled = enable;
+
+	return 0;
+}
+
+static int ecap_cnt_cap_get_pol(struct counter_device *counter,
+				struct counter_signal *signal,
+				u8 inst, u32 *pol)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+
+	pm_runtime_get_sync(counter->parent);
+	*pol = regmap_test_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
+	pm_runtime_put_sync(counter->parent);
+
+	return 0;
+}
+
+static int ecap_cnt_cap_set_pol(struct counter_device *counter,
+				struct counter_signal *signal,
+				u8 inst, u32 pol)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+
+	if (ecap_dev->enabled)
+		return -EBUSY;
+	if (pol > 1)
+		return -EINVAL;
+
+	pm_runtime_get_sync(counter->parent);
+	if (pol)
+		regmap_set_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
+	else
+		regmap_clear_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
+	pm_runtime_put_sync(counter->parent);
+
+	return 0;
+}
+
+static inline int ecap_cnt_cap_read(struct counter_device *counter,
+				    struct counter_count *count,
+				    u8 inst, u64 *cap)
+{
+	return ecap_cnt_count_get_val(counter, ECAP_CAP_REG(inst), cap);
+}
+
+#define ECAP_CAP_GET_POL(i) int ecap_cnt_cap##i##_get_pol(struct counter_device *counter,	\
+							    struct counter_signal *signal,	\
+							    u32 *pol)				\
+{												\
+	return ecap_cnt_cap_get_pol(counter, signal, (i) - 1, pol);				\
+}
+
+#define ECAP_CAP_SET_POL(i) int ecap_cnt_cap##i##_set_pol(struct counter_device *counter,	\
+							    struct counter_signal *signal,	\
+							    u32 pol)				\
+{												\
+	return ecap_cnt_cap_set_pol(counter, signal, (i) - 1, pol);				\
+}
+
+#define ECAP_CAP_READ(i) int ecap_cnt_cap##i##_read(struct counter_device *counter,		\
+						     struct counter_count *count,		\
+						     u64 *cap)					\
+{												\
+	return ecap_cnt_cap_read(counter, count, (i) - 1, cap);				\
+}
+
+static inline ECAP_CAP_GET_POL(1)
+static inline ECAP_CAP_GET_POL(2)
+static inline ECAP_CAP_GET_POL(3)
+static inline ECAP_CAP_GET_POL(4)
+static inline ECAP_CAP_SET_POL(1)
+static inline ECAP_CAP_SET_POL(2)
+static inline ECAP_CAP_SET_POL(3)
+static inline ECAP_CAP_SET_POL(4)
+static inline ECAP_CAP_READ(1)
+static inline ECAP_CAP_READ(2)
+static inline ECAP_CAP_READ(3)
+static inline ECAP_CAP_READ(4)
+
+static int ecap_cnt_count_cumul_read(struct counter_device *counter,
+				     struct counter_count *count, u64 *val)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+
+	*val = ((u64)U32_MAX + 1) * atomic_read(&ecap_dev->nb_ovf);
+
+	return 0;
+}
+
+static int ecap_cnt_clk_get_freq(struct counter_device *counter,
+				 struct counter_signal *signal, u64 *freq)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+
+	*freq = ecap_dev->clk_rate;
+
+	return 0;
+}
+
+static const struct counter_ops ecap_cnt_ops = {
+	.count_read = ecap_cnt_count_read,
+	.function_read = ecap_cnt_function_read,
+	.action_read = ecap_cnt_action_read,
+	.watch_validate = ecap_cnt_watch_validate,
+};
+
+static const enum counter_function ecap_cnt_functions[] = {
+	COUNTER_FUNCTION_INCREASE,
+};
+
+static const enum counter_synapse_action ecap_cnt_actions[] = {
+	COUNTER_SYNAPSE_ACTION_NONE,
+};
+
+static const char *const ecap_cnt_cap_polarities[] = {
+	"rising edge",
+	"falling edge",
+};
+
+static DEFINE_COUNTER_ENUM(ecap_cnt_cap_avail_pol, ecap_cnt_cap_polarities);
+
+static struct counter_comp ecap_cnt_signal_ext[] = {
+	COUNTER_COMP_SIGNAL_ENUM("polarity1", ecap_cnt_cap1_get_pol,
+				 ecap_cnt_cap1_set_pol, ecap_cnt_cap_avail_pol),
+	COUNTER_COMP_SIGNAL_ENUM("polarity2", ecap_cnt_cap2_get_pol,
+				 ecap_cnt_cap2_set_pol, ecap_cnt_cap_avail_pol),
+	COUNTER_COMP_SIGNAL_ENUM("polarity3", ecap_cnt_cap3_get_pol,
+				 ecap_cnt_cap3_set_pol, ecap_cnt_cap_avail_pol),
+	COUNTER_COMP_SIGNAL_ENUM("polarity4", ecap_cnt_cap4_get_pol,
+				 ecap_cnt_cap4_set_pol, ecap_cnt_cap_avail_pol),
+};
+
+static struct counter_comp ecap_cnt_clock_ext[] = {
+	COUNTER_COMP_SIGNAL_U64("frequency", ecap_cnt_clk_get_freq, NULL),
+};
+
+static struct counter_signal ecap_cnt_signals[] = {
+	{
+		.id = ECAP_INPUT_SIG,
+		.name = "ECAP Input Signal",
+		.ext = ecap_cnt_signal_ext,
+		.num_ext = ARRAY_SIZE(ecap_cnt_signal_ext),
+	},
+	{
+		.id = ECAP_CLOCK_SIG,
+		.name = "ECAP Clock Signal",
+		.ext = ecap_cnt_clock_ext,
+		.num_ext = ARRAY_SIZE(ecap_cnt_clock_ext),
+	},
+};
+
+static struct counter_synapse ecap_cnt_synapses[] = {
+	{
+		.actions_list = ecap_cnt_actions,
+		.num_actions = ARRAY_SIZE(ecap_cnt_actions),
+		.signal = &ecap_cnt_signals[ECAP_INPUT_SIG],
+	},
+	{
+		.actions_list = ecap_cnt_actions,
+		.num_actions = ARRAY_SIZE(ecap_cnt_actions),
+		.signal = &ecap_cnt_signals[ECAP_CLOCK_SIG],
+	},
+};
+
+static struct counter_comp ecap_cnt_count_ext[] = {
+	COUNTER_COMP_COUNT_U64("capture1", ecap_cnt_cap1_read, NULL),
+	COUNTER_COMP_COUNT_U64("capture2", ecap_cnt_cap2_read, NULL),
+	COUNTER_COMP_COUNT_U64("capture3", ecap_cnt_cap3_read, NULL),
+	COUNTER_COMP_COUNT_U64("capture4", ecap_cnt_cap4_read, NULL),
+	COUNTER_COMP_COUNT_U64("count_cumul", ecap_cnt_count_cumul_read, NULL),
+	COUNTER_COMP_ENABLE(ecap_cnt_enable_read, ecap_cnt_enable_write),
+};
+
+static struct counter_count ecap_cnt_counts[] = {
+	{
+		.id = 0,
+		.name = "ECAP Timestamp Counter",
+		.functions_list = ecap_cnt_functions,
+		.num_functions = ARRAY_SIZE(ecap_cnt_functions),
+		.synapses = ecap_cnt_synapses,
+		.num_synapses = ARRAY_SIZE(ecap_cnt_synapses),
+		.ext = ecap_cnt_count_ext,
+		.num_ext = ARRAY_SIZE(ecap_cnt_count_ext),
+	},
+};
+
+static irqreturn_t ecap_cnt_isr(int irq, void *dev_id)
+{
+	struct counter_device *counter_dev = dev_id;
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
+	unsigned int clr = 0;
+	unsigned int flg;
+	int i;
+
+	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
+
+	for (i = ECAP_CEVT_LAST ; i <= ECAP_EVT_LAST ; i++) {
+		if (flg & ECAP_EVT_FLG_BIT(i)) {
+			if (i != ECAP_CNTOVF) {
+				/* Input signal edge detected on last CAP (CAP4) */
+				counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, ECAP_CHAN);
+			} else {
+				/* Counter overflow */
+				counter_push_event(counter_dev, COUNTER_EVENT_OVERFLOW, ECAP_CHAN);
+				atomic_inc(&ecap_dev->nb_ovf);
+			}
+
+			clr |= ECAP_EVT_CLR_BIT(i);
+		}
+	}
+
+	clr |= ECAP_INT_CLR_BIT;
+	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_CLR_FRC_REG, ECAP_EVT_CLR_MASK, clr);
+
+	return IRQ_HANDLED;
+}
+
+static void ecap_cnt_pm_disable(void *dev)
+{
+	pm_runtime_disable(dev);
+}
+
+static int ecap_cnt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ecap_cnt_dev *ecap_dev;
+	struct counter_device *counter_dev;
+	void __iomem *mmio_base;
+	int ret;
+
+	counter_dev = devm_counter_alloc(dev, sizeof(*ecap_dev));
+	if (IS_ERR(counter_dev))
+		return PTR_ERR(counter_dev);
+
+	counter_dev->name = ECAP_DRV_NAME;
+	counter_dev->parent = dev;
+	counter_dev->ops = &ecap_cnt_ops;
+	counter_dev->signals = ecap_cnt_signals;
+	counter_dev->num_signals = ARRAY_SIZE(ecap_cnt_signals);
+	counter_dev->counts = ecap_cnt_counts;
+	counter_dev->num_counts = ARRAY_SIZE(ecap_cnt_counts);
+
+	ecap_dev = counter_priv(counter_dev);
+
+	ecap_dev->clk = devm_clk_get_enabled(dev, "fck");
+	if (IS_ERR(ecap_dev->clk))
+		return dev_err_probe(dev, PTR_ERR(ecap_dev->clk), "failed to get clock\n");
+
+	ecap_dev->clk_rate = clk_get_rate(ecap_dev->clk);
+	if (!ecap_dev->clk_rate) {
+		dev_err(dev, "failed to get clock rate\n");
+		return -EINVAL;
+	}
+
+	mmio_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mmio_base))
+		return PTR_ERR(mmio_base);
+
+	ecap_dev->regmap = devm_regmap_init_mmio(dev, mmio_base, &ecap_cnt_regmap_config);
+	if (IS_ERR(ecap_dev->regmap))
+		return dev_err_probe(dev, PTR_ERR(ecap_dev->regmap), "failed to init regmap\n");
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to get irq\n");
+
+	ret = devm_request_irq(dev, ret, ecap_cnt_isr, 0, pdev->name, counter_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to request irq\n");
+
+	platform_set_drvdata(pdev, counter_dev);
+
+	pm_runtime_enable(dev);
+
+	/* Register a cleanup callback to care for disabling PM */
+	ret = devm_add_action_or_reset(dev, ecap_cnt_pm_disable, dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to add pm disable action\n");
+
+	ecap_dev->enabled = 0;
+	ecap_cnt_capture_set_evmode(counter_dev, 0);
+
+	ret = devm_counter_add(dev, counter_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to add counter\n");
+
+	return 0;
+}
+
+static int ecap_cnt_remove(struct platform_device *pdev)
+{
+	struct counter_device *counter_dev = platform_get_drvdata(pdev);
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
+
+	if (ecap_dev->enabled)
+		ecap_cnt_capture_disable(counter_dev);
+
+	return 0;
+}
+
+static int ecap_cnt_suspend(struct device *dev)
+{
+	struct counter_device *counter_dev = dev_get_drvdata(dev);
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
+
+	/* If eCAP is running, stop capture then save timestamp counter */
+	if (ecap_dev->enabled) {
+		/*
+		 * Disabling capture has the following effects:
+		 * - interrupts are disabled
+		 * - loading of capture registers is disabled
+		 * - timebase counter is stopped
+		 */
+		ecap_cnt_capture_disable(counter_dev);
+
+		pm_runtime_get_sync(dev);
+		regmap_read(ecap_dev->regmap, ECAP_TSCNT_REG, &ecap_dev->pm_ctx.time_cntr);
+		pm_runtime_put_sync(dev);
+	}
+
+	ecap_dev->pm_ctx.ev_mode = ecap_cnt_capture_get_evmode(counter_dev);
+
+	clk_disable(ecap_dev->clk);
+
+	return 0;
+}
+
+static int ecap_cnt_resume(struct device *dev)
+{
+	struct counter_device *counter_dev = dev_get_drvdata(dev);
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
+
+	clk_enable(ecap_dev->clk);
+
+	ecap_cnt_capture_set_evmode(counter_dev, ecap_dev->pm_ctx.ev_mode);
+
+	/* If eCAP was running, restore timestamp counter then run capture */
+	if (ecap_dev->enabled) {
+		pm_runtime_get_sync(dev);
+		regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, ecap_dev->pm_ctx.time_cntr);
+		pm_runtime_put_sync(dev);
+
+		ecap_cnt_capture_enable(counter_dev, false);
+	}
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(ecap_cnt_pm_ops, ecap_cnt_suspend, ecap_cnt_resume);
+
+static const struct of_device_id ecap_cnt_of_match[] = {
+	{ .compatible	= "ti,am62-ecap-capture" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ecap_cnt_of_match);
+
+static struct platform_driver ecap_cnt_driver = {
+	.probe = ecap_cnt_probe,
+	.remove = ecap_cnt_remove,
+	.driver = {
+		.name = "ecap-capture",
+		.of_match_table = ecap_cnt_of_match,
+		.pm = pm_sleep_ptr(&ecap_cnt_pm_ops),
+	},
+};
+module_platform_driver(ecap_cnt_driver);
+
+MODULE_DESCRIPTION("ECAP Capture driver");
+MODULE_AUTHOR("Julien Panis <jpanis@baylibre.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
index 96c5ffd368ad..4c5372c6f2a3 100644
--- a/include/uapi/linux/counter.h
+++ b/include/uapi/linux/counter.h
@@ -63,6 +63,8 @@ enum counter_event_type {
 	COUNTER_EVENT_INDEX,
 	/* State of counter is changed */
 	COUNTER_EVENT_CHANGE_OF_STATE,
+	/* Count value is captured */
+	COUNTER_EVENT_CAPTURE,
 };
 
 /**
-- 
2.25.1

Re: [PATCH v5 3/3] counter: ti-ecap-capture: capture driver support for ECAP

[William Breathitt Gray]

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

On Wed, Aug 17, 2022 at 04:16:20PM +0200, Julien Panis wrote:
> ECAP hardware on TI AM62x SoC supports capture feature. It can be used
> to timestamp events (falling/rising edges) detected on signal input pin.
> 
> This commit adds capture driver support for ECAP hardware on AM62x SoC.
> 
> In the ECAP hardware, capture pin can also be configured to be in
> PWM mode. Current implementation only supports capture operating mode.
> Hardware also supports timebase sync between multiple instances, but
> this driver supports simple independent capture functionality.
> 
> Signed-off-by: Julien Panis <jpanis@baylibre.com>
> ---
>  drivers/counter/Kconfig           |  15 +
>  drivers/counter/Makefile          |   1 +
>  drivers/counter/ti-ecap-capture.c | 624 ++++++++++++++++++++++++++++++
>  include/uapi/linux/counter.h      |   2 +
>  4 files changed, 642 insertions(+)
>  create mode 100644 drivers/counter/ti-ecap-capture.c
> 
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 5edd155f1911..08235268af0b 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -101,4 +101,19 @@ config INTEL_QEP
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called intel-qep.
>  
> +config TI_ECAP_CAPTURE
> +	tristate "TI eCAP capture driver"
> +	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	select REGMAP_MMIO
> +	help
> +	  Select this option to enable the Texas Instruments Enhanced Capture
> +	  (eCAP) driver in input mode.
> +
> +	  It can be used to timestamp events (falling/rising edges) detected
> +	  on signal input pin.

The phrase "signal input pin" sounds ambiguous; perhaps "ECAP input
signal" or similar works better here.

> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called ti-ecap-capture.
> +
>  endif # COUNTER
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index 8fde6c100ebc..b9a369e0d4fc 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
>  obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
>  obj-$(CONFIG_MICROCHIP_TCB_CAPTURE)	+= microchip-tcb-capture.o
>  obj-$(CONFIG_INTEL_QEP)		+= intel-qep.o
> +obj-$(CONFIG_TI_ECAP_CAPTURE)	+= ti-ecap-capture.o
> diff --git a/drivers/counter/ti-ecap-capture.c b/drivers/counter/ti-ecap-capture.c
> new file mode 100644
> index 000000000000..b00ddf122bbd
> --- /dev/null
> +++ b/drivers/counter/ti-ecap-capture.c
> @@ -0,0 +1,624 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * ECAP Capture driver
> + *
> + * Copyright (C) 2022 Julien Panis <jpanis@baylibre.com>
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/clk.h>
> +#include <linux/counter.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +#define ECAP_DRV_NAME "ecap"
> +
> +/* ECAP event IDs */
> +enum ecap_event_id {
> +	/* Capture events */
> +	ECAP_CEVT1,
> +	ECAP_CEVT2,
> +	ECAP_CEVT3,
> +	ECAP_CEVT4,
> +	/* Counter overflow event */
> +	ECAP_CNTOVF,
> +	/* Helpers for capture events */
> +	ECAP_CEVT_LAST = ECAP_CEVT4,
> +	ECAP_NB_CEVT = ECAP_CEVT_LAST + 1,
> +	/* Helpers for all events */
> +	ECAP_EVT_LAST = ECAP_CNTOVF,
> +	ECAP_NB_EVT = ECAP_EVT_LAST + 1,
> +};

You're relying on a side-effect of enum structures to define your ECAP_*
constants in sequence; this obscures the intention of your code and
makes it more difficult to understand the purpose of the constants.
The enum structure is unnecessary; these are register flags so C defines
are sufficent and clear enough to represent these values.

> +static void ecap_cnt_capture_enable(struct counter_device *counter, bool rearm)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +	unsigned int regval;
> +
> +	pm_runtime_get_sync(counter->parent);
> +
> +	/* Enable interrupts on events */
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG,
> +			   ECAP_EVT_EN_MASK, ECAP_EVT_EN_MASK);
> +
> +	/* Run counter */
> +	regval = ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK;
> +	if (rearm) {
> +		atomic_set(&ecap_dev->nb_ovf, 0);
> +		regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, 0);

Looks like you're resetting both nb_ovf and TSCNT here. This combines
too many operations in one extension, whereas extensions are intended to
serve a restricted and well-defined scope of behavior. The "enable"
extension is intended to serve as a pause/unpause mechanism for the
respective Count, so don't reset the Count data here. Instead break
these operations into their own extensions: implement a count_write()
callback to reset TSCNT, and similarly for nb_ovf.

> +static int ecap_cnt_action_read(struct counter_device *counter,
> +				struct counter_count *count,
> +				struct counter_synapse *synapse,
> +				enum counter_synapse_action *action)
> +{
> +	*action = COUNTER_SYNAPSE_ACTION_NONE;
> +
> +	return 0;
> +}

This action_read() callback serves both "ECAP Input Signal" and "ECAP
Clock Signal" so you need to check the struct counter_synapse to see
which Signal is being handled. Return COUNTER_SYNAPSE_ACTION_NONE for
ECAP_INPUT_SIG, and COUNTER_SYNAPSE_ACTION_BOTH_EDGES for ECAP_CLOCK_SIG
(assuming TSCNT increments on both edges of the clock).

> +
> +static int ecap_cnt_watch_validate(struct counter_device *counter,
> +				   const struct counter_watch *watch)
> +{
> +	return (watch->channel == ECAP_CHAN &&
> +		(watch->event == COUNTER_EVENT_CAPTURE ||
> +		 watch->event == COUNTER_EVENT_OVERFLOW)) ? 0 : -EINVAL;

One-liners are typically nice, but it is difficult to parse the meaning
of expressions with multiple layers. The logic of this conditional check
is clearer if you separate the channel validation from the event type
validation with their own if-statement blocks.

> +static int ecap_cnt_cap_set_pol(struct counter_device *counter,
> +				struct counter_signal *signal,
> +				u8 inst, u32 pol)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	if (ecap_dev->enabled)
> +		return -EBUSY;
> +	if (pol > 1)
> +		return -EINVAL;

The boundary check for "pol" can be removed; COUNTER_COMP_SIGNAL_ENUM
already ensures that "pol" will be within the boundaries of your Counter
enum array.

> +static int ecap_cnt_count_cumul_read(struct counter_device *counter,
> +				     struct counter_count *count, u64 *val)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	*val = ((u64)U32_MAX + 1) * atomic_read(&ecap_dev->nb_ovf);
> +
> +	return 0;
> +}

Reimplement this as "ceiling" and "num_overflows"; simply return U32_MAX
and nb_ovf respectively.

> +static int ecap_cnt_clk_get_freq(struct counter_device *counter,
> +				 struct counter_signal *signal, u64 *freq)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	*freq = ecap_dev->clk_rate;

Looks like this is the only place you ever use clk_rate. Just perform
the clk_get_rate() call here and you won't need clk_rate in ecap_dev
anymore.

> +static const struct counter_ops ecap_cnt_ops = {
> +	.count_read = ecap_cnt_count_read,
> +	.function_read = ecap_cnt_function_read,
> +	.action_read = ecap_cnt_action_read,
> +	.watch_validate = ecap_cnt_watch_validate,
> +};

Add a count_write callback to set TSCNT. This will allow users to reset
TSCNT back to 0 when desired.

> +static const enum counter_synapse_action ecap_cnt_actions[] = {
> +	COUNTER_SYNAPSE_ACTION_NONE,
> +};

Define a second array with just COUNTER_SYNAPSE_ACTION_BOTH_EDGES for
the clock Signal (assuming the count increments on both edges) and pass
it to the respective clock signal Synapse actions_list.

> +static struct counter_comp ecap_cnt_signal_ext[] = {
> +	COUNTER_COMP_SIGNAL_ENUM("polarity1", ecap_cnt_cap1_get_pol,
> +				 ecap_cnt_cap1_set_pol, ecap_cnt_cap_avail_pol),
> +	COUNTER_COMP_SIGNAL_ENUM("polarity2", ecap_cnt_cap2_get_pol,
> +				 ecap_cnt_cap2_set_pol, ecap_cnt_cap_avail_pol),
> +	COUNTER_COMP_SIGNAL_ENUM("polarity3", ecap_cnt_cap3_get_pol,
> +				 ecap_cnt_cap3_set_pol, ecap_cnt_cap_avail_pol),
> +	COUNTER_COMP_SIGNAL_ENUM("polarity4", ecap_cnt_cap4_get_pol,
> +				 ecap_cnt_cap4_set_pol, ecap_cnt_cap_avail_pol),
> +};

Zero-based indexing is typically expected so start at polarity0 instead
of polarity1.

> +static struct counter_signal ecap_cnt_signals[] = {
> +	{
> +		.id = ECAP_INPUT_SIG,
> +		.name = "ECAP Input Signal",
> +		.ext = ecap_cnt_signal_ext,
> +		.num_ext = ARRAY_SIZE(ecap_cnt_signal_ext),
> +	},
> +	{
> +		.id = ECAP_CLOCK_SIG,
> +		.name = "ECAP Clock Signal",
> +		.ext = ecap_cnt_clock_ext,
> +		.num_ext = ARRAY_SIZE(ecap_cnt_clock_ext),
> +	},
> +};
> +
> +static struct counter_synapse ecap_cnt_synapses[] = {
> +	{
> +		.actions_list = ecap_cnt_actions,
> +		.num_actions = ARRAY_SIZE(ecap_cnt_actions),
> +		.signal = &ecap_cnt_signals[ECAP_INPUT_SIG],
> +	},
> +	{
> +		.actions_list = ecap_cnt_actions,
> +		.num_actions = ARRAY_SIZE(ecap_cnt_actions),
> +		.signal = &ecap_cnt_signals[ECAP_CLOCK_SIG],
> +	},
> +};

I find it more natural to list the clock signal first because that's
what's incrementing the TSCNT counter. But this isn't necessarily wrong,
so I'll leave it up to you if you want to reorder your Signals and
Synapses arrays.

> +static struct counter_comp ecap_cnt_count_ext[] = {
> +	COUNTER_COMP_COUNT_U64("capture1", ecap_cnt_cap1_read, NULL),
> +	COUNTER_COMP_COUNT_U64("capture2", ecap_cnt_cap2_read, NULL),
> +	COUNTER_COMP_COUNT_U64("capture3", ecap_cnt_cap3_read, NULL),
> +	COUNTER_COMP_COUNT_U64("capture4", ecap_cnt_cap4_read, NULL),

Same comment about zero-based indexing here as with the polarity
extensions: start with capture0 instead of capture 1.

> +	COUNTER_COMP_COUNT_U64("count_cumul", ecap_cnt_count_cumul_read, NULL),
> +	COUNTER_COMP_ENABLE(ecap_cnt_enable_read, ecap_cnt_enable_write),
> +};

Replace "count_cumul" with "num_overflows" (COUNTER_COMP_COUNT_U64) and
"ceiling" (COUNTER_COMP_CEILING).

> +static struct counter_count ecap_cnt_counts[] = {
> +	{
> +		.id = 0,
> +		.name = "ECAP Timestamp Counter",

You probably don't need "ECAP" in the Count name because users already
know they interacting with an ECAP device when they read the sysfs
"counterX/name" attribute.

> +static irqreturn_t ecap_cnt_isr(int irq, void *dev_id)
> +{
> +	struct counter_device *counter_dev = dev_id;
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
> +	unsigned int clr = 0;
> +	unsigned int flg;
> +	int i;
> +
> +	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
> +
> +	for (i = ECAP_CEVT_LAST ; i <= ECAP_EVT_LAST ; i++) {
> +		if (flg & ECAP_EVT_FLG_BIT(i)) {

Use the for_each_set_bit() macro to simplify this for-loop+if check.

> +			if (i != ECAP_CNTOVF) {

Move the check for overflow outside of the for-loop because it's
conceptually unrelated to the other flags. The for-loop can then be
focused on just the capture events.

> +				/* Input signal edge detected on last CAP (CAP4) */
> +				counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, ECAP_CHAN);

Restricting event pushes to just CAP4 means losing three quarters of the
possible captures events this device is capable of reporting. By pushing
a Counter event on every capture, users have much finer control over the
operation of their device, allowing them to perform measurements of
instantaneous pulse widths among other such cycle analyses.

Your particular use-case requires analysing four captures at a time. If
you push every capture event on the same channel, you would have to
ignore the majority of them; checking for CAP4 on every capture event is
cumbersome for userspace in such a scenario. To reconcile the need to
report all device events with the desire to filter some of those capture
events, the solution is to push each CAPx event to respective Counter
event channels::

    counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, i);

This intuitively matches an update to a capture buffer element with a
respective Counter event channel. For your use-case, this requires no
change to a userspace application reading the sysfs attributes, and a
minimal change to one interacting with the character device node:
specifying the particular Counter event channel in your Counter watches;
the rest of users are able to watch all capture events, or just some, as
they please.

> +static int ecap_cnt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ecap_cnt_dev *ecap_dev;
> +	struct counter_device *counter_dev;
> +	void __iomem *mmio_base;
> +	int ret;
> +
> +	counter_dev = devm_counter_alloc(dev, sizeof(*ecap_dev));
> +	if (IS_ERR(counter_dev))
> +		return PTR_ERR(counter_dev);
> +
> +	counter_dev->name = ECAP_DRV_NAME;
> +	counter_dev->parent = dev;
> +	counter_dev->ops = &ecap_cnt_ops;
> +	counter_dev->signals = ecap_cnt_signals;
> +	counter_dev->num_signals = ARRAY_SIZE(ecap_cnt_signals);
> +	counter_dev->counts = ecap_cnt_counts;
> +	counter_dev->num_counts = ARRAY_SIZE(ecap_cnt_counts);
> +
> +	ecap_dev = counter_priv(counter_dev);
> +
> +	ecap_dev->clk = devm_clk_get_enabled(dev, "fck");
> +	if (IS_ERR(ecap_dev->clk))
> +		return dev_err_probe(dev, PTR_ERR(ecap_dev->clk), "failed to get clock\n");
> +
> +	ecap_dev->clk_rate = clk_get_rate(ecap_dev->clk);
> +	if (!ecap_dev->clk_rate) {
> +		dev_err(dev, "failed to get clock rate\n");
> +		return -EINVAL;
> +	}
> +
> +	mmio_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(mmio_base))
> +		return PTR_ERR(mmio_base);
> +
> +	ecap_dev->regmap = devm_regmap_init_mmio(dev, mmio_base, &ecap_cnt_regmap_config);
> +	if (IS_ERR(ecap_dev->regmap))
> +		return dev_err_probe(dev, PTR_ERR(ecap_dev->regmap), "failed to init regmap\n");
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to get irq\n");
> +
> +	ret = devm_request_irq(dev, ret, ecap_cnt_isr, 0, pdev->name, counter_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to request irq\n");
> +
> +	platform_set_drvdata(pdev, counter_dev);
> +
> +	pm_runtime_enable(dev);
> +
> +	/* Register a cleanup callback to care for disabling PM */
> +	ret = devm_add_action_or_reset(dev, ecap_cnt_pm_disable, dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to add pm disable action\n");
> +
> +	ecap_dev->enabled = 0;

The devm_counter_alloc() function ensures your ecap_dev structure is
zero-initialized so you don't need to explicitly set "enabled" to 0
here. However, it's not harmful so I'll leave it up to you if you want
to remove this line.

William Breathitt Gray

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

Re: [PATCH v5 0/3] ECAP support on TI AM62x SoC

[William Breathitt Gray]

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

On Wed, Aug 17, 2022 at 04:16:17PM +0200, Julien Panis wrote:
> The Enhanced Capture (ECAP) module can be used to timestamp events
> detected on signal input pin. It can be used for time measurements
> of pulse train signals.
> 
> ECAP module includes 4 timestamp capture registers. For all 4 sequenced
> timestamp capture events (1->2->3->4->1->...), edge polarity (falling/rising
> edge) can be selected.
> 
> This driver leverages counter subsystem to :
> - select edge polarity for all 4 capture events (event mode)
> - log timestamps for each capture event
> Event polarity, and CAP1/2/3/4 timestamps give all the information
> about the input pulse train. Further information can easily be computed :
> period and/or duty cycle if frequency is constant, elapsed time between
> pulses, etc...
> 
> Modifications since v4:
> 	- Modify yaml commit message prefix (dt-bindings)
> 	- Modify driver file name & Makefile (ti-ecap-capture)
> 	- Modify compilation flag name in Kconfig (TI_ECAP_CAPTURE)
> 	- Select REGMAP_MMIO in Kconfig
> 	- Add capture items to sysfs-bus-counter ABI documentation
> 	- Cleanup probe function (dev_err_probe & devm_clk_get_enabled & devm_add_action_or_reset for PM)
> 	- Enable/Disable device clock in suspend/resume functions
> 	- Add PM explanations
> 	- Add ECAP clock signal & 'frequency' sysfs entry
> 	- Replace elapsed_time & spinlock by nb_ovf (atomic_t) & 'count_cumul' sysfs entry
> 	- Add counter overflow event
> 	- Modify 'name' sysfs entry for signal0 & signal1 & count0
> 	- Modify watch_validate function
> 	- Add macros for callbacks related to cap1/2/3/4
> 
> Userspace commands :
> 	### SIGNAL INPUT ###
> 	cd /sys/bus/counter/devices/counter0/signal0
> 
> 	# Get available polarities for each capture event
> 	cat polarity1_available
> 	cat polarity2_available
> 	cat polarity3_available
> 	cat polarity4_available
> 
> 	# Get polarity for each capture event
> 	cat polarity1
> 	cat polarity2
> 	cat polarity3
> 	cat polarity4
> 
> 	# Set polarity for each capture event
> 	echo rising edge > polarity1
> 	echo falling edge > polarity2
> 	echo rising edge > polarity3
> 	echo falling edge > polarity4
> 
> 	### SIGNAL CLOCK ###
> 	cd /sys/bus/counter/devices/counter0/signal1
> 
> 	# Get clock rate
> 	cat frequency
> 
> 	### COUNT ###
> 	cd /sys/bus/counter/devices/counter0/count0
> 
> 	# Run ECAP
> 	echo 1 > enable
> 
> 	# Get current timebase counter value
> 	cat count
> 
> 	# Get cumulated counter value
> 	cat count_cumul
> 
> 	# Get captured timestamps
> 	cat capture1
> 	cat capture2
> 	cat capture3
> 	cat capture4
> 
> 	# Note that counter watches can also be used to get
> 	# data from userspace application
> 	# -> see tools/counter/counter_example.c
> 
> 	# Stop ECAP
> 	echo 0 > enable
> 
> Julien Panis (3):
>   dt-bindings: counter: add ti,am62-ecap-capture.yaml
>   Documentation: ABI: sysfs-bus-counter: add capture items
>   counter: ti-ecap-capture: capture driver support for ECAP
> 
>  Documentation/ABI/testing/sysfs-bus-counter   |  49 ++
>  .../counter/ti,am62-ecap-capture.yaml         |  61 ++
>  drivers/counter/Kconfig                       |  15 +
>  drivers/counter/Makefile                      |   1 +
>  drivers/counter/ti-ecap-capture.c             | 624 ++++++++++++++++++
>  include/uapi/linux/counter.h                  |   2 +
>  6 files changed, 752 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml
>  create mode 100644 drivers/counter/ti-ecap-capture.c
> 
> -- 
> 2.25.1

Hello Julien,

I'm CCing a number of other developers here who have indicated interest
in counter timestamp functionality in past, just in case they would like
to particpate in this discussion.

Adding buffers to the Counter subsystem will be a user-visible ABI
change, so I want to make sure we get the interface design correct
before we merge any of those changes; once it's exposed to userspace it
can't be changed. However, we can still improve your patches while we
develop this interface, so revisions are welcome even if I can't merge
your counter driver until we iron out the Counter subsystem buffer
interface.

So in the v4 patchset we discussed introducing a new counter_comp_type
COUNTER_COMP_BUFFER_U64 enum constant with respective counter_comp read
callbacks::

     int (*count_buffer_u64_read)(struct counter_device *counter,
                                  struct counter_count *count,
                                  size_t index, u64 *val); 

Drive authors can then handle buffer reads by receiving an "index",
locating the value at that buffer offset, and returning the value via
the "val" u64 pointer.

Defining a buffer as Count extensions could be done using a helper
macro::

     COUNTER_COMP_COUNT_BUFFER_U64("capture", ecap_cnt_cap_read, 4)

Originally I considered unrolling this into four COUNTER_COMP_COUNT_U64,
but I'm unsure if that is possible in GCC. Regardless, I believe it's
feasible to implement this in counter-chrdev.c by passing the buffer
length in the "priv" member of struct counter_comp and handling it when
creating sysfs attributes. I might be able to write a prototype for this
in the next couple weeks.

In the end, we should have four buffer elements exposed as sysfs
attributes under the respective count directory:

* /sys/bus/counter/devices/counterX/count0/capture0
* /sys/bus/counter/devices/counterX/count0/capture1
* /sys/bus/counter/devices/counterX/count0/capture2
* /sys/bus/counter/devices/counterX/count0/capture3 

One worry I do have is whether this will scale well enough; I can
imagine some future device having a timestamp history buffer of much
later than four elements. In cases with large buffers, it might be more
practical to expose a FIFO queue to deliver buffer data. However, the
existing Counter character device interface isn't designed for data of
arbitrary length, so we'd likely have to introduce a secondary character
device to provide the queue.

We can postpone implementation of such a queue until the need arises and
focus on just the sysfs interface for this particular driver. If we
expose each element of the buffer as its own sysfs attribute, then the
existing Counter character device interface gets access to these
elements for free without any additional code changes to that part of
the Counter subsystem.

So my concerns right now are making sure this is a sane design and the
right path forward to expose device buffer elements in sysfs.

William Breathitt Gray

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