[PATCH 0/8] counter: fix, improvements and stm32 timer events support

[PATCH 0/8] counter: fix, improvements and stm32 timer events support

[Fabrice Gasnier]

This series combines some fix and improvements to the counter interface,
found while stm32 timer counter driver developements.
It also introduces a new tool that can be used for testing.

Then, it improves the stm32 timer counter driver by introducing new signals,
e.g. counting frequency, and missing channels.
It also adds support for interrupt based events using the chrdev interface.
Two event types are added in this series: overflows and capture.

Up to now, stm32 timer counter driver focused mainly on quadrature
encoder feature. With this series, all timer instances can be enabled
for simple counting (with overflow and capture events).

Fabrice Gasnier (8):
  counter: chrdev: fix getting array extensions
  counter: chrdev: remove a typo in header file comment
  tools/counter: add a flexible watch events tool
  mfd: stm32-timers: add support for interrupts
  counter: stm32-timer-cnt: rename quadrature signal
  counter: stm32-timer-cnt: introduce clock signal
  counter: stm32-timer-cnt: populate capture channels and check encoder
  counter: stm32-timer-cnt: add support for events

 drivers/counter/counter-chrdev.c     |   4 +-
 drivers/counter/stm32-timer-cnt.c    | 585 ++++++++++++++++++++++++++-
 drivers/mfd/stm32-timers.c           |  46 +++
 include/linux/mfd/stm32-timers.h     |  26 ++
 include/uapi/linux/counter.h         |   2 +-
 tools/counter/Build                  |   1 +
 tools/counter/Makefile               |   8 +-
 tools/counter/counter_watch_events.c | 348 ++++++++++++++++
 8 files changed, 998 insertions(+), 22 deletions(-)
 create mode 100644 tools/counter/counter_watch_events.c

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/8] counter: chrdev: fix getting array extensions

[Fabrice Gasnier]

When trying to watch a component array extension, and the array isn't the
first extended element, it fails as the type comparison is always done on
the 1st element. Fix it by indexing the 'ext' array.

Example on a dummy struct counter_comp:
static struct counter_comp dummy[] = {
	COUNTER_COMP_DIRECTION(..),
	...,
	COUNTER_COMP_ARRAY_CAPTURE(...),
};
static struct counter_count dummy_cnt = {
	...
	.ext = dummy,
	.num_ext = ARRAY_SIZE(dummy),
}

Currently, counter_get_ext() returns -EINVAL when trying to add a watch
event on one of the capture array element in such example.

Fixes: d2011be1e22f ("counter: Introduce the COUNTER_COMP_ARRAY component type")
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/counter/counter-chrdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
index 80acdf62794a..afc94d0062b1 100644
--- a/drivers/counter/counter-chrdev.c
+++ b/drivers/counter/counter-chrdev.c
@@ -247,8 +247,8 @@ static int counter_get_ext(const struct counter_comp *const ext,
 		if (*id == component_id)
 			return 0;
 
-		if (ext->type == COUNTER_COMP_ARRAY) {
-			element = ext->priv;
+		if (ext[*ext_idx].type == COUNTER_COMP_ARRAY) {
+			element = ext[*ext_idx].priv;
 
 			if (component_id - *id < element->length)
 				return 0;
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] counter: chrdev: fix getting array extensions

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 941 bytes --]

On Tue, Aug 29, 2023 at 03:40:22PM +0200, Fabrice Gasnier wrote:
> When trying to watch a component array extension, and the array isn't the
> first extended element, it fails as the type comparison is always done on
> the 1st element. Fix it by indexing the 'ext' array.
> 
> Example on a dummy struct counter_comp:
> static struct counter_comp dummy[] = {
> 	COUNTER_COMP_DIRECTION(..),
> 	...,
> 	COUNTER_COMP_ARRAY_CAPTURE(...),
> };
> static struct counter_count dummy_cnt = {
> 	...
> 	.ext = dummy,
> 	.num_ext = ARRAY_SIZE(dummy),
> }
> 
> Currently, counter_get_ext() returns -EINVAL when trying to add a watch
> event on one of the capture array element in such example.
> 
> Fixes: d2011be1e22f ("counter: Introduce the COUNTER_COMP_ARRAY component type")
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Applied to counter-fixes as commit 3170256d7bc1.

Thanks,

William Breathitt Gray

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/8] counter: chrdev: remove a typo in header file comment

[Fabrice Gasnier]

Replace COUNTER_COUNT_SCOPE that doesn't exist by the defined
COUNTER_SCOPE_COUNT.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 include/uapi/linux/counter.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
index fc248ef00e86..008a691c254b 100644
--- a/include/uapi/linux/counter.h
+++ b/include/uapi/linux/counter.h
@@ -38,7 +38,7 @@ enum counter_scope {
  *
  * For example, if the Count 2 ceiling extension of Counter device 4 is desired,
  * set type equal to COUNTER_COMPONENT_EXTENSION, scope equal to
- * COUNTER_COUNT_SCOPE, parent equal to 2, and id equal to the value provided by
+ * COUNTER_SCOPE_COUNT, parent equal to 2, and id equal to the value provided by
  * the respective /sys/bus/counter/devices/counter4/count2/ceiling_component_id
  * sysfs attribute.
  */
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/8] counter: chrdev: remove a typo in header file comment

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 311 bytes --]

On Tue, Aug 29, 2023 at 03:40:23PM +0200, Fabrice Gasnier wrote:
> Replace COUNTER_COUNT_SCOPE that doesn't exist by the defined
> COUNTER_SCOPE_COUNT.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Applied to counter-next as commit 631c15d5f14d.

Thanks,

William Breathitt Gray

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/8] tools/counter: add a flexible watch events tool

[Fabrice Gasnier]

This adds a new counter tool to be able to test various watch events.
A flexible watch array can be populated from command line, each field
may be tuned with a dedicated command line argument.
Each argument can be repeated several times: each time it gets repeated,
a corresponding new watch element is allocated.

It also comes with a simple default watch (to monitor overflows), used
when no watch parameters are provided.

The print_usage() routine proposes another example, from the command line,
which generates a 2 elements watch array, to monitor:
- overflow events
- capture events, on channel 3, that reads read captured data by
  specifying the component id (capture3_component_id being 7 here).

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 tools/counter/Build                  |   1 +
 tools/counter/Makefile               |   8 +-
 tools/counter/counter_watch_events.c | 348 +++++++++++++++++++++++++++
 3 files changed, 356 insertions(+), 1 deletion(-)
 create mode 100644 tools/counter/counter_watch_events.c

diff --git a/tools/counter/Build b/tools/counter/Build
index 33f4a51d715e..4bbadb7ec93a 100644
--- a/tools/counter/Build
+++ b/tools/counter/Build
@@ -1 +1,2 @@
 counter_example-y += counter_example.o
+counter_watch_events-y += counter_watch_events.o
diff --git a/tools/counter/Makefile b/tools/counter/Makefile
index b2c2946f44c9..00e211edd768 100644
--- a/tools/counter/Makefile
+++ b/tools/counter/Makefile
@@ -14,7 +14,7 @@ MAKEFLAGS += -r
 
 override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
 
-ALL_TARGETS := counter_example
+ALL_TARGETS := counter_example counter_watch_events
 ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
 
 all: $(ALL_PROGRAMS)
@@ -31,6 +31,12 @@ $(OUTPUT)include/linux/counter.h: ../../include/uapi/linux/counter.h
 
 prepare: $(OUTPUT)include/linux/counter.h
 
+COUNTER_WATCH_EVENTS := $(OUTPUT)counter_watch_events.o
+$(COUNTER_WATCH_EVENTS): prepare FORCE
+	$(Q)$(MAKE) $(build)=counter_watch_events
+$(OUTPUT)counter_watch_events: $(COUNTER_WATCH_EVENTS)
+	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+
 COUNTER_EXAMPLE := $(OUTPUT)counter_example.o
 $(COUNTER_EXAMPLE): prepare FORCE
 	$(Q)$(MAKE) $(build)=counter_example
diff --git a/tools/counter/counter_watch_events.c b/tools/counter/counter_watch_events.c
new file mode 100644
index 000000000000..7f73a1519d8e
--- /dev/null
+++ b/tools/counter/counter_watch_events.c
@@ -0,0 +1,348 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Counter - Test various watch events in a userspace application
+ * inspired by counter_example.c
+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <getopt.h>
+#include <linux/counter.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <unistd.h>
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+
+static struct counter_watch simple_watch[] = {
+	{
+		/* Component data: Count 0 count */
+		.component.type = COUNTER_COMPONENT_COUNT,
+		.component.scope = COUNTER_SCOPE_COUNT,
+		.component.parent = 0,
+		/* Event type: Index */
+		.event = COUNTER_EVENT_OVERFLOW_UNDERFLOW,
+		/* Device event channel 0 */
+		.channel = 0,
+	},
+};
+
+static int find_match_or_number_from_array(char *arg, const char * const str[], int sz, __u8 *val)
+{
+	unsigned int i;
+	char *dummy;
+	unsigned long idx;
+	int ret;
+
+	for (i = 0; i < sz; i++) {
+		ret = strncmp(arg, str[i], strlen(str[i]));
+		if (!ret && strlen(str[i]) == strlen(arg)) {
+			*val = i;
+			return 0;
+		}
+	}
+
+	/* fallback to number */
+	idx = strtoul(optarg, &dummy, 10);
+	if (!errno) {
+		if (idx >= sz)
+			return -EINVAL;
+		*val = idx;
+		return 0;
+	}
+
+	return -errno;
+}
+
+static const char * const counter_event_type_name[] = {
+	"COUNTER_EVENT_OVERFLOW",
+	"COUNTER_EVENT_UNDERFLOW",
+	"COUNTER_EVENT_OVERFLOW_UNDERFLOW",
+	"COUNTER_EVENT_THRESHOLD",
+	"COUNTER_EVENT_INDEX",
+	"COUNTER_EVENT_CHANGE_OF_STATE",
+	"COUNTER_EVENT_CAPTURE",
+};
+
+static int counter_arg_to_event_type(char *arg, __u8 *event)
+{
+	return find_match_or_number_from_array(arg, counter_event_type_name,
+					       ARRAY_SIZE(counter_event_type_name), event);
+}
+
+static const char * const counter_component_type_name[] = {
+	"COUNTER_COMPONENT_NONE",
+	"COUNTER_COMPONENT_SIGNAL",
+	"COUNTER_COMPONENT_COUNT",
+	"COUNTER_COMPONENT_FUNCTION",
+	"COUNTER_COMPONENT_SYNAPSE_ACTION",
+	"COUNTER_COMPONENT_EXTENSION",
+};
+
+static int counter_arg_to_component_type(char *arg, __u8 *type)
+{
+	return find_match_or_number_from_array(arg, counter_component_type_name,
+					       ARRAY_SIZE(counter_component_type_name), type);
+}
+
+static const char * const counter_scope_name[] = {
+	"COUNTER_SCOPE_DEVICE",
+	"COUNTER_SCOPE_SIGNAL",
+	"COUNTER_SCOPE_COUNT",
+};
+
+static int counter_arg_to_scope(char *arg, __u8 *type)
+{
+	return find_match_or_number_from_array(arg, counter_scope_name,
+					       ARRAY_SIZE(counter_scope_name), type);
+}
+
+static void print_usage(void)
+{
+	fprintf(stderr, "Usage: counter_watch_events [options]...\n"
+		"Test various watch events for given counter device\n"
+		"  --channel -c <n>\n"
+		"        Set watch.channel\n"
+		"  --debug -d\n"
+		"        Prints debug information\n"
+		"  --event -e <number or counter_event_type string>\n"
+		"        Sets watch.event\n"
+		"  --help -h\n"
+		"        Prints usage\n"
+		"  --device-num -n <n>\n"
+		"        Set device number (/dev/counter<n>, default to 0)\n"
+		"  --id -i <n>\n"
+		"        Set watch.component.id\n"
+		"  --loop -l <n>\n"
+		"        Loop for a number of events (forever if n < 0)\n"
+		"  --parent -p <n>\n"
+		"        Set watch.component.parent number\n"
+		"  --scope -s <number or counter_scope string>\n"
+		"        Set watch.component.scope\n"
+		"  --type -t <number or counter_component_type string>\n"
+		"        Set watch.component.type\n"
+		"\n"
+		"Example with two watched events:\n\n"
+		"counter_watch_events -d \\\n"
+		"\t-t COUNTER_COMPONENT_COUNT -s COUNTER_SCOPE_COUNT"
+		" -e COUNTER_EVENT_OVERFLOW_UNDERFLOW -i 0 -c 0 \\\n"
+		"\t-t COUNTER_COMPONENT_EXTENSION -s COUNTER_SCOPE_COUNT"
+		" -e COUNTER_EVENT_CAPTURE -i 7 -c 3\n"
+		);
+}
+
+static void print_watch(struct counter_watch *watch, int nwatch)
+{
+	int i;
+
+	/* prints the watch array in C-like structure */
+	printf("watch[%d] = {\n", nwatch);
+	for (i = 0; i < nwatch; i++) {
+		printf(" [%d] =\t{\n"
+		       "\t\t.component.type = %s\n"
+		       "\t\t.component.scope = %s\n"
+		       "\t\t.component.parent = %d\n"
+		       "\t\t.component.id = %d\n"
+		       "\t\t.event = %s\n"
+		       "\t\t.channel = %d\n"
+		       "\t},\n",
+		       i,
+		       counter_component_type_name[watch[i].component.type],
+		       counter_scope_name[watch[i].component.scope],
+		       watch[i].component.parent,
+		       watch[i].component.id,
+		       counter_event_type_name[watch[i].event],
+		       watch[i].channel);
+	}
+	printf("};\n");
+}
+
+static const struct option longopts[] = {
+	{ "channel",		required_argument, 0, 'c' },
+	{ "debug",		no_argument,       0, 'd' },
+	{ "event",		required_argument, 0, 'e' },
+	{ "help",		no_argument,       0, 'h' },
+	{ "device-num",		required_argument, 0, 'n' },
+	{ "id",			required_argument, 0, 'i' },
+	{ "loop",		required_argument, 0, 'l' },
+	{ "parent",		required_argument, 0, 'p' },
+	{ "scope",		required_argument, 0, 's' },
+	{ "type",		required_argument, 0, 't' },
+	{ },
+};
+
+int main(int argc, char **argv)
+{
+	int c, fd, i, ret;
+	struct counter_event event_data;
+	char *device_name = NULL;
+	int debug = 0, loop = -1;
+	char *dummy;
+	int dev_num = 0, nwatch = 0, ncfg[] = {0, 0, 0, 0, 0, 0};
+	int num_chan = 0, num_evt = 0, num_id = 0, num_p = 0, num_s = 0, num_t = 0;
+	struct counter_watch *watches;
+
+	/*
+	 * 1st pass: count events configurations number to allocate the watch array.
+	 * Each watch argument can be repeated several times: each time it gets repeated,
+	 * a corresponding watch is allocated (and configured) in 2nd pass.
+	 */
+	while ((c = getopt_long(argc, argv, "c:de:hn:i:l:p:s:t:", longopts, NULL)) != -1) {
+		switch (c) {
+		case 'c':
+			ncfg[0]++;
+			break;
+		case 'e':
+			ncfg[1]++;
+			break;
+		case 'i':
+			ncfg[2]++;
+			break;
+		case 'p':
+			ncfg[3]++;
+			break;
+		case 's':
+			ncfg[4]++;
+			break;
+		case 't':
+			ncfg[5]++;
+			break;
+		};
+	};
+
+	for (i = 0; i < ARRAY_SIZE(ncfg); i++)
+		if (ncfg[i] > nwatch)
+			nwatch = ncfg[i];
+
+	if (nwatch) {
+		watches = calloc(nwatch, sizeof(*watches));
+	} else {
+		/* default to simple watch example */
+		watches = simple_watch;
+		nwatch = ARRAY_SIZE(simple_watch);
+	}
+
+	/* 2nd pass: read arguments to fill in watch array */
+	optind = 1;
+	while ((c = getopt_long(argc, argv, "c:de:hn:i:l:p:s:t:", longopts, NULL)) != -1) {
+		switch (c) {
+		case 'c':
+			/* watch.channel */
+			watches[num_chan].channel = strtoul(optarg, &dummy, 10);
+			if (errno)
+				return -errno;
+			num_chan++;
+			break;
+		case 'd':
+			debug = 1;
+			break;
+		case 'e':
+			/* watch.event */
+			ret = counter_arg_to_event_type(optarg, &watches[num_evt].event);
+			if (ret)
+				return ret;
+			num_evt++;
+			break;
+		case 'h':
+			print_usage();
+			return -1;
+		case 'n':
+			errno = 0;
+			dev_num = strtoul(optarg, &dummy, 10);
+			if (errno)
+				return -errno;
+			break;
+		case 'i':
+			/* watch.component.id */
+			watches[num_id].component.id = strtoul(optarg, &dummy, 10);
+			if (errno)
+				return -errno;
+			num_id++;
+			break;
+		case 'l':
+			loop = strtol(optarg, &dummy, 10);
+			if (errno)
+				return -errno;
+			break;
+		case 'p':
+			/* watch.component.parent */
+			watches[num_p].component.parent = strtoul(optarg, &dummy, 10);
+			if (errno)
+				return -errno;
+			num_p++;
+			break;
+		case 's':
+			/* watch.component.scope */
+			ret = counter_arg_to_scope(optarg, &watches[num_s].component.scope);
+			if (ret)
+				return ret;
+			num_s++;
+			break;
+		case 't':
+			/* watch.component.type */
+			ret = counter_arg_to_component_type(optarg, &watches[num_t].component.type);
+			if (ret)
+				return ret;
+			num_t++;
+			break;
+		}
+
+	};
+
+	if (debug)
+		print_watch(watches, nwatch);
+
+	ret = asprintf(&device_name, "/dev/counter%d", dev_num);
+	if (ret < 0)
+		return -ENOMEM;
+
+	if (debug)
+		printf("Opening %s\n", device_name);
+
+	fd = open(device_name, O_RDWR);
+	if (fd == -1) {
+		perror("Unable to open counter device");
+		return 1;
+	}
+
+	for (i = 0; i < nwatch; i++) {
+		ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches + i);
+		if (ret == -1) {
+			fprintf(stderr, "Error adding watches[%d]: %s\n", i,
+				strerror(errno));
+			return 1;
+		}
+	}
+
+	ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
+	if (ret == -1) {
+		perror("Error enabling events");
+		return 1;
+	}
+
+	for (i = 0; loop <= 0 || i < loop; i++) {
+		ret = read(fd, &event_data, sizeof(event_data));
+		if (ret == -1) {
+			perror("Failed to read event data");
+			return 1;
+		}
+
+		if (ret != sizeof(event_data)) {
+			fprintf(stderr, "Failed to read event data\n");
+			return -EIO;
+		}
+
+		printf("Timestamp: %llu\tData: %llu\t event: %s\tch: %d\n",
+		       event_data.timestamp, event_data.value,
+		       counter_event_type_name[event_data.watch.event],
+		       event_data.watch.channel);
+
+		if (event_data.status) {
+			fprintf(stderr, "Error %d: %s\n", event_data.status,
+				strerror(event_data.status));
+		}
+	}
+
+	return 0;
+}
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] tools/counter: add a flexible watch events tool

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 13322 bytes --]

On Tue, Aug 29, 2023 at 03:40:24PM +0200, Fabrice Gasnier wrote:
> This adds a new counter tool to be able to test various watch events.
> A flexible watch array can be populated from command line, each field
> may be tuned with a dedicated command line argument.
> Each argument can be repeated several times: each time it gets repeated,
> a corresponding new watch element is allocated.
> 
> It also comes with a simple default watch (to monitor overflows), used
> when no watch parameters are provided.
> 
> The print_usage() routine proposes another example, from the command line,
> which generates a 2 elements watch array, to monitor:
> - overflow events
> - capture events, on channel 3, that reads read captured data by
>   specifying the component id (capture3_component_id being 7 here).
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Hi Fabrice,

This is great idea, it'll make it so much easier to test out drivers
so I'm excited! :-)

This is a new tool, so would you add a MAINTAINERS entry for the
counter_watch_events.c file?

More comments inline below.

> ---
>  tools/counter/Build                  |   1 +
>  tools/counter/Makefile               |   8 +-
>  tools/counter/counter_watch_events.c | 348 +++++++++++++++++++++++++++
>  3 files changed, 356 insertions(+), 1 deletion(-)
>  create mode 100644 tools/counter/counter_watch_events.c
> 
> diff --git a/tools/counter/Build b/tools/counter/Build
> index 33f4a51d715e..4bbadb7ec93a 100644
> --- a/tools/counter/Build
> +++ b/tools/counter/Build
> @@ -1 +1,2 @@
>  counter_example-y += counter_example.o
> +counter_watch_events-y += counter_watch_events.o
> diff --git a/tools/counter/Makefile b/tools/counter/Makefile
> index b2c2946f44c9..00e211edd768 100644
> --- a/tools/counter/Makefile
> +++ b/tools/counter/Makefile
> @@ -14,7 +14,7 @@ MAKEFLAGS += -r
>  
>  override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
>  
> -ALL_TARGETS := counter_example
> +ALL_TARGETS := counter_example counter_watch_events
>  ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
>  
>  all: $(ALL_PROGRAMS)
> @@ -31,6 +31,12 @@ $(OUTPUT)include/linux/counter.h: ../../include/uapi/linux/counter.h
>  
>  prepare: $(OUTPUT)include/linux/counter.h
>  
> +COUNTER_WATCH_EVENTS := $(OUTPUT)counter_watch_events.o
> +$(COUNTER_WATCH_EVENTS): prepare FORCE
> +	$(Q)$(MAKE) $(build)=counter_watch_events
> +$(OUTPUT)counter_watch_events: $(COUNTER_WATCH_EVENTS)
> +	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
> +

Move this below the COUNTER_EXAMPLE block just so we can keep the
recipes in alphabetical order.

>  COUNTER_EXAMPLE := $(OUTPUT)counter_example.o
>  $(COUNTER_EXAMPLE): prepare FORCE
>  	$(Q)$(MAKE) $(build)=counter_example
> diff --git a/tools/counter/counter_watch_events.c b/tools/counter/counter_watch_events.c
> new file mode 100644
> index 000000000000..7f73a1519d8e
> --- /dev/null
> +++ b/tools/counter/counter_watch_events.c
> @@ -0,0 +1,348 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Counter - Test various watch events in a userspace application

"Counter" should be "Counter Watch Events" (or "counter_watch_events").

> + * inspired by counter_example.c

No need to mention counter_example.c, this utility does far more than
and bares little resemblance at this point to counter_example.c which is
really just a bare minimal example of watching Counter events.

> + */
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <getopt.h>
> +#include <linux/counter.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>
> +
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))

My initial reaction was that this macro is already exposed in some
header for us, but my local /usr/include/linux/kernel.h file doesn't
appear to bare it so I guess not. Perhaps it'll be fine for our needs --
I think the only difference between this ARRAY_SIZE and the Linux kernel
one is the addition of __must_be_array(x).

> +
> +static struct counter_watch simple_watch[] = {
> +	{
> +		/* Component data: Count 0 count */
> +		.component.type = COUNTER_COMPONENT_COUNT,
> +		.component.scope = COUNTER_SCOPE_COUNT,
> +		.component.parent = 0,
> +		/* Event type: Index */
> +		.event = COUNTER_EVENT_OVERFLOW_UNDERFLOW,

There's a bit of confusion here. The comment says the event type is
INDEX, but the structure event type is set for OVERFLOW_UNDERFLOW; also,
the commit description states that we're monitoring "overflows" which
implies to me the OVERFLOW event type. So which of the three is it?

> +		/* Device event channel 0 */
> +		.channel = 0,
> +	},
> +};
> +
> +static int find_match_or_number_from_array(char *arg, const char * const str[], int sz, __u8 *val)
> +{
> +	unsigned int i;
> +	char *dummy;
> +	unsigned long idx;
> +	int ret;
> +
> +	for (i = 0; i < sz; i++) {
> +		ret = strncmp(arg, str[i], strlen(str[i]));
> +		if (!ret && strlen(str[i]) == strlen(arg)) {
> +			*val = i;
> +			return 0;
> +		}

This has several strlen calls so I wonder if it's more wasteful than it
needs to me. I suppose the compiler would optimize this away, but I
think there is an alternative solution.

We're checking for an exact match, so you don't need the string length.
Instead, you can compare each character, break when characters differ,
or return 0 when you reached the null byte for both. So something like
this:

    for (j = 0; arg[j] == str[i][j]; j++) {
            /* If we reached the end of the strings */
            if (arg[j] == '\0') {
                    *val = i;
                    return 0;
            }
    }
    /* Strings do not match; continue to the next string */

We end up with the same number of lines, so I'll leave it up to you
whether you want to use this solution, or if you consider the existing
code clearer read.

> +	}
> +
> +	/* fallback to number */

I'm not sure it makes sense to support numbers. Although it's true that
the component type, component scope, and event type are passed as __u8
values, users are expected to treat those values are opaque and pass
them via the respective enum constants. Since we don't guarantee that
the specific enum constant values will remain consistent between kernel
versions, I don't think it's a good idea to give to users that sort of
implication by allowing them to use raw numbers for configuration
selection.

> +	idx = strtoul(optarg, &dummy, 10);
> +	if (!errno) {
> +		if (idx >= sz)
> +			return -EINVAL;
> +		*val = idx;
> +		return 0;
> +	}
> +
> +	return -errno;
> +}
> +
> +static const char * const counter_event_type_name[] = {
> +	"COUNTER_EVENT_OVERFLOW",
> +	"COUNTER_EVENT_UNDERFLOW",
> +	"COUNTER_EVENT_OVERFLOW_UNDERFLOW",
> +	"COUNTER_EVENT_THRESHOLD",
> +	"COUNTER_EVENT_INDEX",
> +	"COUNTER_EVENT_CHANGE_OF_STATE",
> +	"COUNTER_EVENT_CAPTURE",
> +};
> +
> +static int counter_arg_to_event_type(char *arg, __u8 *event)
> +{
> +	return find_match_or_number_from_array(arg, counter_event_type_name,
> +					       ARRAY_SIZE(counter_event_type_name), event);
> +}
> +
> +static const char * const counter_component_type_name[] = {
> +	"COUNTER_COMPONENT_NONE",
> +	"COUNTER_COMPONENT_SIGNAL",
> +	"COUNTER_COMPONENT_COUNT",
> +	"COUNTER_COMPONENT_FUNCTION",
> +	"COUNTER_COMPONENT_SYNAPSE_ACTION",
> +	"COUNTER_COMPONENT_EXTENSION",
> +};
> +
> +static int counter_arg_to_component_type(char *arg, __u8 *type)
> +{
> +	return find_match_or_number_from_array(arg, counter_component_type_name,
> +					       ARRAY_SIZE(counter_component_type_name), type);
> +}
> +
> +static const char * const counter_scope_name[] = {
> +	"COUNTER_SCOPE_DEVICE",
> +	"COUNTER_SCOPE_SIGNAL",
> +	"COUNTER_SCOPE_COUNT",
> +};
> +
> +static int counter_arg_to_scope(char *arg, __u8 *type)
> +{
> +	return find_match_or_number_from_array(arg, counter_scope_name,
> +					       ARRAY_SIZE(counter_scope_name), type);
> +}
> +
> +static void print_usage(void)
> +{
> +	fprintf(stderr, "Usage: counter_watch_events [options]...\n"
> +		"Test various watch events for given counter device\n"
> +		"  --channel -c <n>\n"
> +		"        Set watch.channel\n"
> +		"  --debug -d\n"
> +		"        Prints debug information\n"
> +		"  --event -e <number or counter_event_type string>\n"
> +		"        Sets watch.event\n"
> +		"  --help -h\n"
> +		"        Prints usage\n"
> +		"  --device-num -n <n>\n"
> +		"        Set device number (/dev/counter<n>, default to 0)\n"
> +		"  --id -i <n>\n"
> +		"        Set watch.component.id\n"
> +		"  --loop -l <n>\n"
> +		"        Loop for a number of events (forever if n < 0)\n"
> +		"  --parent -p <n>\n"
> +		"        Set watch.component.parent number\n"
> +		"  --scope -s <number or counter_scope string>\n"
> +		"        Set watch.component.scope\n"
> +		"  --type -t <number or counter_component_type string>\n"
> +		"        Set watch.component.type\n"
> +		"\n"
> +		"Example with two watched events:\n\n"
> +		"counter_watch_events -d \\\n"
> +		"\t-t COUNTER_COMPONENT_COUNT -s COUNTER_SCOPE_COUNT"
> +		" -e COUNTER_EVENT_OVERFLOW_UNDERFLOW -i 0 -c 0 \\\n"
> +		"\t-t COUNTER_COMPONENT_EXTENSION -s COUNTER_SCOPE_COUNT"
> +		" -e COUNTER_EVENT_CAPTURE -i 7 -c 3\n"
> +		);
> +}

Are you following any particular convention for the usage description? I
wonder if there is a particular preferred standard for command-line
interface descriptions. A quick search brought up a few, such as the
POSIX Utility Conventions[^1] and docopt[^2].

One improvement I would recommend here is to put the short form of the
option before the long form and separate them with a command to make it
clearer (e.g. "-h, --help").

[^1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
[^2] http://docopt.org

> +
> +static void print_watch(struct counter_watch *watch, int nwatch)
> +{
> +	int i;
> +
> +	/* prints the watch array in C-like structure */
> +	printf("watch[%d] = {\n", nwatch);
> +	for (i = 0; i < nwatch; i++) {
> +		printf(" [%d] =\t{\n"
> +		       "\t\t.component.type = %s\n"
> +		       "\t\t.component.scope = %s\n"
> +		       "\t\t.component.parent = %d\n"
> +		       "\t\t.component.id = %d\n"
> +		       "\t\t.event = %s\n"
> +		       "\t\t.channel = %d\n"
> +		       "\t},\n",
> +		       i,
> +		       counter_component_type_name[watch[i].component.type],
> +		       counter_scope_name[watch[i].component.scope],
> +		       watch[i].component.parent,
> +		       watch[i].component.id,
> +		       counter_event_type_name[watch[i].event],
> +		       watch[i].channel);
> +	}
> +	printf("};\n");
> +}
> +
> +static const struct option longopts[] = {
> +	{ "channel",		required_argument, 0, 'c' },
> +	{ "debug",		no_argument,       0, 'd' },
> +	{ "event",		required_argument, 0, 'e' },
> +	{ "help",		no_argument,       0, 'h' },
> +	{ "device-num",		required_argument, 0, 'n' },
> +	{ "id",			required_argument, 0, 'i' },
> +	{ "loop",		required_argument, 0, 'l' },
> +	{ "parent",		required_argument, 0, 'p' },
> +	{ "scope",		required_argument, 0, 's' },
> +	{ "type",		required_argument, 0, 't' },
> +	{ },
> +};
> +
> +int main(int argc, char **argv)
> +{
> +	int c, fd, i, ret;
> +	struct counter_event event_data;
> +	char *device_name = NULL;
> +	int debug = 0, loop = -1;
> +	char *dummy;
> +	int dev_num = 0, nwatch = 0, ncfg[] = {0, 0, 0, 0, 0, 0};
> +	int num_chan = 0, num_evt = 0, num_id = 0, num_p = 0, num_s = 0, num_t = 0;
> +	struct counter_watch *watches;
> +
> +	/*
> +	 * 1st pass: count events configurations number to allocate the watch array.
> +	 * Each watch argument can be repeated several times: each time it gets repeated,
> +	 * a corresponding watch is allocated (and configured) in 2nd pass.
> +	 */

It feels a somewhat prone to error (at least cumbersome) to populate
each watch via individual arguments for each field. Since a watch always
has these fields, perhaps instead we could pass some format string that
represents a watch, and deliminate watches via commas. For example, we
could have --watch="cco00,ecc73" to represent the two watches in the
usage example.

Of course, we'd need to define a more robust format string convention
than in my example to ensure the correct configuration is properly
communicated. What do you think, would this approach would make things
simpler, or just more complicated in the end?

> +	while ((c = getopt_long(argc, argv, "c:de:hn:i:l:p:s:t:", longopts, NULL)) != -1) {
> +		switch (c) {
> +		case 'c':
> +			ncfg[0]++;
> +			break;
> +		case 'e':
> +			ncfg[1]++;
> +			break;
> +		case 'i':
> +			ncfg[2]++;
> +			break;
> +		case 'p':
> +			ncfg[3]++;
> +			break;
> +		case 's':
> +			ncfg[4]++;
> +			break;
> +		case 't':
> +			ncfg[5]++;
> +			break;
> +		};
> +	};
> +
> +	for (i = 0; i < ARRAY_SIZE(ncfg); i++)
> +		if (ncfg[i] > nwatch)
> +			nwatch = ncfg[i];
> +
> +	if (nwatch) {
> +		watches = calloc(nwatch, sizeof(*watches));

We need to check if calloc fails, right?

William Breathitt Gray 

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] tools/counter: add a flexible watch events tool

[Fabrice Gasnier]

On 9/17/23 21:07, William Breathitt Gray wrote:
> On Tue, Aug 29, 2023 at 03:40:24PM +0200, Fabrice Gasnier wrote:
>> This adds a new counter tool to be able to test various watch events.
>> A flexible watch array can be populated from command line, each field
>> may be tuned with a dedicated command line argument.
>> Each argument can be repeated several times: each time it gets repeated,
>> a corresponding new watch element is allocated.
>>
>> It also comes with a simple default watch (to monitor overflows), used
>> when no watch parameters are provided.
>>
>> The print_usage() routine proposes another example, from the command line,
>> which generates a 2 elements watch array, to monitor:
>> - overflow events
>> - capture events, on channel 3, that reads read captured data by
>>   specifying the component id (capture3_component_id being 7 here).
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> 
> Hi Fabrice,
> 
> This is great idea, it'll make it so much easier to test out drivers
> so I'm excited! :-)

Hi William,

Thanks

> 
> This is a new tool, so would you add a MAINTAINERS entry for the
> counter_watch_events.c file?

I haven't thought about it.
I can add a MAINTAINERS entry, yes!
Who would you suggest ?

> 
> More comments inline below.
> 
>> ---
>>  tools/counter/Build                  |   1 +
>>  tools/counter/Makefile               |   8 +-
>>  tools/counter/counter_watch_events.c | 348 +++++++++++++++++++++++++++
>>  3 files changed, 356 insertions(+), 1 deletion(-)
>>  create mode 100644 tools/counter/counter_watch_events.c
>>
>> diff --git a/tools/counter/Build b/tools/counter/Build
>> index 33f4a51d715e..4bbadb7ec93a 100644
>> --- a/tools/counter/Build
>> +++ b/tools/counter/Build
>> @@ -1 +1,2 @@
>>  counter_example-y += counter_example.o
>> +counter_watch_events-y += counter_watch_events.o
>> diff --git a/tools/counter/Makefile b/tools/counter/Makefile
>> index b2c2946f44c9..00e211edd768 100644
>> --- a/tools/counter/Makefile
>> +++ b/tools/counter/Makefile
>> @@ -14,7 +14,7 @@ MAKEFLAGS += -r
>>  
>>  override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
>>  
>> -ALL_TARGETS := counter_example
>> +ALL_TARGETS := counter_example counter_watch_events
>>  ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
>>  
>>  all: $(ALL_PROGRAMS)
>> @@ -31,6 +31,12 @@ $(OUTPUT)include/linux/counter.h: ../../include/uapi/linux/counter.h
>>  
>>  prepare: $(OUTPUT)include/linux/counter.h
>>  
>> +COUNTER_WATCH_EVENTS := $(OUTPUT)counter_watch_events.o
>> +$(COUNTER_WATCH_EVENTS): prepare FORCE
>> +	$(Q)$(MAKE) $(build)=counter_watch_events
>> +$(OUTPUT)counter_watch_events: $(COUNTER_WATCH_EVENTS)
>> +	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
>> +
> 
> Move this below the COUNTER_EXAMPLE block just so we can keep the
> recipes in alphabetical order.

Ack, will update it.

> 
>>  COUNTER_EXAMPLE := $(OUTPUT)counter_example.o
>>  $(COUNTER_EXAMPLE): prepare FORCE
>>  	$(Q)$(MAKE) $(build)=counter_example
>> diff --git a/tools/counter/counter_watch_events.c b/tools/counter/counter_watch_events.c
>> new file mode 100644
>> index 000000000000..7f73a1519d8e
>> --- /dev/null
>> +++ b/tools/counter/counter_watch_events.c
>> @@ -0,0 +1,348 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Counter - Test various watch events in a userspace application
> 
> "Counter" should be "Counter Watch Events" (or "counter_watch_events").
> 
>> + * inspired by counter_example.c
> 
> No need to mention counter_example.c, this utility does far more than
> and bares little resemblance at this point to counter_example.c which is
> really just a bare minimal example of watching Counter events.

Ack

> 
>> + */
>> +
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <getopt.h>
>> +#include <linux/counter.h>
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <sys/ioctl.h>
>> +#include <unistd.h>
>> +
>> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> 
> My initial reaction was that this macro is already exposed in some
> header for us, but my local /usr/include/linux/kernel.h file doesn't
> appear to bare it so I guess not. Perhaps it'll be fine for our needs --
> I think the only difference between this ARRAY_SIZE and the Linux kernel
> one is the addition of __must_be_array(x).

I had the same reaction when trying to use it. Then, I figured out
several tools redefine this macro.
Digging further, I just found out some tools have in their Makefile CFLAGS:
-I$(srctree)/tools/include
and include from there: <linux/kernel.h>

I'll update the Makefile in v2, and remove this definition from here.

> 
>> +
>> +static struct counter_watch simple_watch[] = {
>> +	{
>> +		/* Component data: Count 0 count */
>> +		.component.type = COUNTER_COMPONENT_COUNT,
>> +		.component.scope = COUNTER_SCOPE_COUNT,
>> +		.component.parent = 0,
>> +		/* Event type: Index */
>> +		.event = COUNTER_EVENT_OVERFLOW_UNDERFLOW,
> 
> There's a bit of confusion here. The comment says the event type is
> INDEX, but the structure event type is set for OVERFLOW_UNDERFLOW; also,
> the commit description states that we're monitoring "overflows" which
> implies to me the OVERFLOW event type. So which of the three is it?

Ah yes, It's a mix of bad copy paste and updates. I'll fix it.

> 
>> +		/* Device event channel 0 */
>> +		.channel = 0,
>> +	},
>> +};
>> +
>> +static int find_match_or_number_from_array(char *arg, const char * const str[], int sz, __u8 *val)
>> +{
>> +	unsigned int i;
>> +	char *dummy;
>> +	unsigned long idx;
>> +	int ret;
>> +
>> +	for (i = 0; i < sz; i++) {
>> +		ret = strncmp(arg, str[i], strlen(str[i]));
>> +		if (!ret && strlen(str[i]) == strlen(arg)) {
>> +			*val = i;
>> +			return 0;
>> +		}
> 
> This has several strlen calls so I wonder if it's more wasteful than it
> needs to me. I suppose the compiler would optimize this away, but I
> think there is an alternative solution.
> 
> We're checking for an exact match, so you don't need the string length.
> Instead, you can compare each character, break when characters differ,
> or return 0 when you reached the null byte for both. So something like
> this:
> 
>     for (j = 0; arg[j] == str[i][j]; j++) {
>             /* If we reached the end of the strings */
>             if (arg[j] == '\0') {
>                     *val = i;
>                     return 0;
>             }
>     }
>     /* Strings do not match; continue to the next string */
> 
> We end up with the same number of lines, so I'll leave it up to you
> whether you want to use this solution, or if you consider the existing
> code clearer read.

I'll look forward in the direction you propose. First, we need to
confirm in which form the arguments can be expected. It depends on your
proposal to use a --watch string formatted arguments.

> 
>> +	}
>> +
>> +	/* fallback to number */
> 
> I'm not sure it makes sense to support numbers. Although it's true that
> the component type, component scope, and event type are passed as __u8
> values, users are expected to treat those values are opaque and pass
> them via the respective enum constants. Since we don't guarantee that
> the specific enum constant values will remain consistent between kernel
> versions, I don't think it's a good idea to give to users that sort of
> implication by allowing them to use raw numbers for configuration
> selection.

Ack, I can remove this.

I'm a bit surprised by this statement. I may be wrong... I'd expect a
userland binary to be compatible when updating to a newer kernel: e.g.
user API (ABI?) definitions to be stable (including enum constants) ?

> 
>> +	idx = strtoul(optarg, &dummy, 10);
>> +	if (!errno) {
>> +		if (idx >= sz)
>> +			return -EINVAL;
>> +		*val = idx;
>> +		return 0;
>> +	}
>> +
>> +	return -errno;
>> +}
>> +
>> +static const char * const counter_event_type_name[] = {
>> +	"COUNTER_EVENT_OVERFLOW",
>> +	"COUNTER_EVENT_UNDERFLOW",
>> +	"COUNTER_EVENT_OVERFLOW_UNDERFLOW",
>> +	"COUNTER_EVENT_THRESHOLD",
>> +	"COUNTER_EVENT_INDEX",
>> +	"COUNTER_EVENT_CHANGE_OF_STATE",
>> +	"COUNTER_EVENT_CAPTURE",
>> +};
>> +
>> +static int counter_arg_to_event_type(char *arg, __u8 *event)
>> +{
>> +	return find_match_or_number_from_array(arg, counter_event_type_name,
>> +					       ARRAY_SIZE(counter_event_type_name), event);
>> +}
>> +
>> +static const char * const counter_component_type_name[] = {
>> +	"COUNTER_COMPONENT_NONE",
>> +	"COUNTER_COMPONENT_SIGNAL",
>> +	"COUNTER_COMPONENT_COUNT",
>> +	"COUNTER_COMPONENT_FUNCTION",
>> +	"COUNTER_COMPONENT_SYNAPSE_ACTION",
>> +	"COUNTER_COMPONENT_EXTENSION",
>> +};
>> +
>> +static int counter_arg_to_component_type(char *arg, __u8 *type)
>> +{
>> +	return find_match_or_number_from_array(arg, counter_component_type_name,
>> +					       ARRAY_SIZE(counter_component_type_name), type);
>> +}
>> +
>> +static const char * const counter_scope_name[] = {
>> +	"COUNTER_SCOPE_DEVICE",
>> +	"COUNTER_SCOPE_SIGNAL",
>> +	"COUNTER_SCOPE_COUNT",
>> +};
>> +
>> +static int counter_arg_to_scope(char *arg, __u8 *type)
>> +{
>> +	return find_match_or_number_from_array(arg, counter_scope_name,
>> +					       ARRAY_SIZE(counter_scope_name), type);
>> +}
>> +
>> +static void print_usage(void)
>> +{
>> +	fprintf(stderr, "Usage: counter_watch_events [options]...\n"
>> +		"Test various watch events for given counter device\n"
>> +		"  --channel -c <n>\n"
>> +		"        Set watch.channel\n"
>> +		"  --debug -d\n"
>> +		"        Prints debug information\n"
>> +		"  --event -e <number or counter_event_type string>\n"
>> +		"        Sets watch.event\n"
>> +		"  --help -h\n"
>> +		"        Prints usage\n"
>> +		"  --device-num -n <n>\n"
>> +		"        Set device number (/dev/counter<n>, default to 0)\n"
>> +		"  --id -i <n>\n"
>> +		"        Set watch.component.id\n"
>> +		"  --loop -l <n>\n"
>> +		"        Loop for a number of events (forever if n < 0)\n"
>> +		"  --parent -p <n>\n"
>> +		"        Set watch.component.parent number\n"
>> +		"  --scope -s <number or counter_scope string>\n"
>> +		"        Set watch.component.scope\n"
>> +		"  --type -t <number or counter_component_type string>\n"
>> +		"        Set watch.component.type\n"
>> +		"\n"
>> +		"Example with two watched events:\n\n"
>> +		"counter_watch_events -d \\\n"
>> +		"\t-t COUNTER_COMPONENT_COUNT -s COUNTER_SCOPE_COUNT"
>> +		" -e COUNTER_EVENT_OVERFLOW_UNDERFLOW -i 0 -c 0 \\\n"
>> +		"\t-t COUNTER_COMPONENT_EXTENSION -s COUNTER_SCOPE_COUNT"
>> +		" -e COUNTER_EVENT_CAPTURE -i 7 -c 3\n"
>> +		);
>> +}
> 
> Are you following any particular convention for the usage description? I
> wonder if there is a particular preferred standard for command-line
> interface descriptions. A quick search brought up a few, such as the
> POSIX Utility Conventions[^1] and docopt[^2].
> 
> One improvement I would recommend here is to put the short form of the
> option before the long form and separate them with a command to make it
> clearer (e.g. "-h, --help").
> 
> [^1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
> [^2] http://docopt.org

Thanks for pointing this! So definitely a good pointer, and suggestion
to look at!

I'll try to improve in v2.

> 
>> +
>> +static void print_watch(struct counter_watch *watch, int nwatch)
>> +{
>> +	int i;
>> +
>> +	/* prints the watch array in C-like structure */
>> +	printf("watch[%d] = {\n", nwatch);
>> +	for (i = 0; i < nwatch; i++) {
>> +		printf(" [%d] =\t{\n"
>> +		       "\t\t.component.type = %s\n"
>> +		       "\t\t.component.scope = %s\n"
>> +		       "\t\t.component.parent = %d\n"
>> +		       "\t\t.component.id = %d\n"
>> +		       "\t\t.event = %s\n"
>> +		       "\t\t.channel = %d\n"
>> +		       "\t},\n",
>> +		       i,
>> +		       counter_component_type_name[watch[i].component.type],
>> +		       counter_scope_name[watch[i].component.scope],
>> +		       watch[i].component.parent,
>> +		       watch[i].component.id,
>> +		       counter_event_type_name[watch[i].event],
>> +		       watch[i].channel);
>> +	}
>> +	printf("};\n");
>> +}
>> +
>> +static const struct option longopts[] = {
>> +	{ "channel",		required_argument, 0, 'c' },
>> +	{ "debug",		no_argument,       0, 'd' },
>> +	{ "event",		required_argument, 0, 'e' },
>> +	{ "help",		no_argument,       0, 'h' },
>> +	{ "device-num",		required_argument, 0, 'n' },
>> +	{ "id",			required_argument, 0, 'i' },
>> +	{ "loop",		required_argument, 0, 'l' },
>> +	{ "parent",		required_argument, 0, 'p' },
>> +	{ "scope",		required_argument, 0, 's' },
>> +	{ "type",		required_argument, 0, 't' },
>> +	{ },
>> +};
>> +
>> +int main(int argc, char **argv)
>> +{
>> +	int c, fd, i, ret;
>> +	struct counter_event event_data;
>> +	char *device_name = NULL;
>> +	int debug = 0, loop = -1;
>> +	char *dummy;
>> +	int dev_num = 0, nwatch = 0, ncfg[] = {0, 0, 0, 0, 0, 0};
>> +	int num_chan = 0, num_evt = 0, num_id = 0, num_p = 0, num_s = 0, num_t = 0;
>> +	struct counter_watch *watches;
>> +
>> +	/*
>> +	 * 1st pass: count events configurations number to allocate the watch array.
>> +	 * Each watch argument can be repeated several times: each time it gets repeated,
>> +	 * a corresponding watch is allocated (and configured) in 2nd pass.
>> +	 */
> 
> It feels a somewhat prone to error (at least cumbersome) to populate

Yes, this could be error prone. This is also why I added a print of the
gathered arguments when using --debug option.
Perhaps this could be better to always print it (e.g. print_watch()) ?

> each watch via individual arguments for each field. Since a watch always
> has these fields, perhaps instead we could pass some format string that
> represents a watch, and deliminate watches via commas. For example, we
> could have --watch="cco00,ecc73" to represent the two watches in the
> usage example.

I like the idea, to concatenate as a string. With current approach, the
command line quickly becomes very long.

It makes it obvious in your example, that two watches are used, and no
argument is omitted.
On the opposite, each argument isn't very easy to understand compared to
plain text definition.

> 
> Of course, we'd need to define a more robust format string convention
> than in my example to ensure the correct configuration is properly

Indeed, by using a single letter, we could face limitations (ex:
overflow, underflow, overflow_underflow, which letter for the 3rd here?)

If we go this way, probably need to brainstorm a bit.

> communicated. What do you think, would this approach would make things
> simpler, or just more complicated in the end?

I'm not 100% sure if some helpers like getopt() will help here? So, I
guess this could be more complicated. This may also be against the
guideline "options should be preceded by the '-' delimiter character."
in [^1] (Ok, this would rather be the --watch option, fed with watch data.)

Would you have suggestions regarding possible helpers ? Or do you have
in mind some others tools that already adopted such approach ?

> 
>> +	while ((c = getopt_long(argc, argv, "c:de:hn:i:l:p:s:t:", longopts, NULL)) != -1) {
>> +		switch (c) {
>> +		case 'c':
>> +			ncfg[0]++;
>> +			break;
>> +		case 'e':
>> +			ncfg[1]++;
>> +			break;
>> +		case 'i':
>> +			ncfg[2]++;
>> +			break;
>> +		case 'p':
>> +			ncfg[3]++;
>> +			break;
>> +		case 's':
>> +			ncfg[4]++;
>> +			break;
>> +		case 't':
>> +			ncfg[5]++;
>> +			break;
>> +		};
>> +	};
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ncfg); i++)
>> +		if (ncfg[i] > nwatch)
>> +			nwatch = ncfg[i];
>> +
>> +	if (nwatch) {
>> +		watches = calloc(nwatch, sizeof(*watches));
> 
> We need to check if calloc fails, right?

Yes, you're right, will fix this too.

Thanks for reviewing!
Best regards,
Fabrice

> 
> William Breathitt Gray 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] tools/counter: add a flexible watch events tool

[Fabrice Gasnier]

On 9/19/23 17:37, Fabrice Gasnier wrote:
> On 9/17/23 21:07, William Breathitt Gray wrote:
>> On Tue, Aug 29, 2023 at 03:40:24PM +0200, Fabrice Gasnier wrote:
>>> This adds a new counter tool to be able to test various watch events.
>>> A flexible watch array can be populated from command line, each field
>>> may be tuned with a dedicated command line argument.
>>> Each argument can be repeated several times: each time it gets repeated,
>>> a corresponding new watch element is allocated.
>>>
>>> It also comes with a simple default watch (to monitor overflows), used
>>> when no watch parameters are provided.
>>>
>>> The print_usage() routine proposes another example, from the command line,
>>> which generates a 2 elements watch array, to monitor:
>>> - overflow events
>>> - capture events, on channel 3, that reads read captured data by
>>>   specifying the component id (capture3_component_id being 7 here).
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
>>
>> Hi Fabrice,
>>
>> This is great idea, it'll make it so much easier to test out drivers
>> so I'm excited! :-)
> 
> Hi William,
> 
> Thanks
> 
>>
>> This is a new tool, so would you add a MAINTAINERS entry for the
>> counter_watch_events.c file?
> 
> I haven't thought about it.
> I can add a MAINTAINERS entry, yes!
> Who would you suggest ?
> 
>>
>> More comments inline below.
>>
>>> ---
>>>  tools/counter/Build                  |   1 +
>>>  tools/counter/Makefile               |   8 +-
>>>  tools/counter/counter_watch_events.c | 348 +++++++++++++++++++++++++++
>>>  3 files changed, 356 insertions(+), 1 deletion(-)
>>>  create mode 100644 tools/counter/counter_watch_events.c
>>>
>>> diff --git a/tools/counter/Build b/tools/counter/Build
>>> index 33f4a51d715e..4bbadb7ec93a 100644
>>> --- a/tools/counter/Build
>>> +++ b/tools/counter/Build
>>> @@ -1 +1,2 @@
>>>  counter_example-y += counter_example.o
>>> +counter_watch_events-y += counter_watch_events.o
>>> diff --git a/tools/counter/Makefile b/tools/counter/Makefile
>>> index b2c2946f44c9..00e211edd768 100644
>>> --- a/tools/counter/Makefile
>>> +++ b/tools/counter/Makefile
>>> @@ -14,7 +14,7 @@ MAKEFLAGS += -r
>>>  
>>>  override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
>>>  
>>> -ALL_TARGETS := counter_example
>>> +ALL_TARGETS := counter_example counter_watch_events
>>>  ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
>>>  
>>>  all: $(ALL_PROGRAMS)
>>> @@ -31,6 +31,12 @@ $(OUTPUT)include/linux/counter.h: ../../include/uapi/linux/counter.h
>>>  
>>>  prepare: $(OUTPUT)include/linux/counter.h
>>>  
>>> +COUNTER_WATCH_EVENTS := $(OUTPUT)counter_watch_events.o
>>> +$(COUNTER_WATCH_EVENTS): prepare FORCE
>>> +	$(Q)$(MAKE) $(build)=counter_watch_events
>>> +$(OUTPUT)counter_watch_events: $(COUNTER_WATCH_EVENTS)
>>> +	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
>>> +
>>
>> Move this below the COUNTER_EXAMPLE block just so we can keep the
>> recipes in alphabetical order.
> 
> Ack, will update it.
> 
>>
>>>  COUNTER_EXAMPLE := $(OUTPUT)counter_example.o
>>>  $(COUNTER_EXAMPLE): prepare FORCE
>>>  	$(Q)$(MAKE) $(build)=counter_example
>>> diff --git a/tools/counter/counter_watch_events.c b/tools/counter/counter_watch_events.c
>>> new file mode 100644
>>> index 000000000000..7f73a1519d8e
>>> --- /dev/null
>>> +++ b/tools/counter/counter_watch_events.c
>>> @@ -0,0 +1,348 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/* Counter - Test various watch events in a userspace application
>>
>> "Counter" should be "Counter Watch Events" (or "counter_watch_events").
>>
>>> + * inspired by counter_example.c
>>
>> No need to mention counter_example.c, this utility does far more than
>> and bares little resemblance at this point to counter_example.c which is
>> really just a bare minimal example of watching Counter events.
> 
> Ack
> 
>>
>>> + */
>>> +
>>> +#include <errno.h>
>>> +#include <fcntl.h>
>>> +#include <getopt.h>
>>> +#include <linux/counter.h>
>>> +#include <stdlib.h>
>>> +#include <stdio.h>
>>> +#include <string.h>
>>> +#include <sys/ioctl.h>
>>> +#include <unistd.h>
>>> +
>>> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>>
>> My initial reaction was that this macro is already exposed in some
>> header for us, but my local /usr/include/linux/kernel.h file doesn't
>> appear to bare it so I guess not. Perhaps it'll be fine for our needs --
>> I think the only difference between this ARRAY_SIZE and the Linux kernel
>> one is the addition of __must_be_array(x).
> 
> I had the same reaction when trying to use it. Then, I figured out
> several tools redefine this macro.
> Digging further, I just found out some tools have in their Makefile CFLAGS:
> -I$(srctree)/tools/include
> and include from there: <linux/kernel.h>
> 
> I'll update the Makefile in v2, and remove this definition from here.
> 
>>
>>> +
>>> +static struct counter_watch simple_watch[] = {
>>> +	{
>>> +		/* Component data: Count 0 count */
>>> +		.component.type = COUNTER_COMPONENT_COUNT,
>>> +		.component.scope = COUNTER_SCOPE_COUNT,
>>> +		.component.parent = 0,
>>> +		/* Event type: Index */
>>> +		.event = COUNTER_EVENT_OVERFLOW_UNDERFLOW,
>>
>> There's a bit of confusion here. The comment says the event type is
>> INDEX, but the structure event type is set for OVERFLOW_UNDERFLOW; also,
>> the commit description states that we're monitoring "overflows" which
>> implies to me the OVERFLOW event type. So which of the three is it?
> 
> Ah yes, It's a mix of bad copy paste and updates. I'll fix it.
> 
>>
>>> +		/* Device event channel 0 */
>>> +		.channel = 0,
>>> +	},
>>> +};
>>> +
>>> +static int find_match_or_number_from_array(char *arg, const char * const str[], int sz, __u8 *val)
>>> +{
>>> +	unsigned int i;
>>> +	char *dummy;
>>> +	unsigned long idx;
>>> +	int ret;
>>> +
>>> +	for (i = 0; i < sz; i++) {
>>> +		ret = strncmp(arg, str[i], strlen(str[i]));
>>> +		if (!ret && strlen(str[i]) == strlen(arg)) {
>>> +			*val = i;
>>> +			return 0;
>>> +		}
>>
>> This has several strlen calls so I wonder if it's more wasteful than it
>> needs to me. I suppose the compiler would optimize this away, but I
>> think there is an alternative solution.
>>
>> We're checking for an exact match, so you don't need the string length.
>> Instead, you can compare each character, break when characters differ,
>> or return 0 when you reached the null byte for both. So something like
>> this:
>>
>>     for (j = 0; arg[j] == str[i][j]; j++) {
>>             /* If we reached the end of the strings */
>>             if (arg[j] == '\0') {
>>                     *val = i;
>>                     return 0;
>>             }
>>     }
>>     /* Strings do not match; continue to the next string */
>>
>> We end up with the same number of lines, so I'll leave it up to you
>> whether you want to use this solution, or if you consider the existing
>> code clearer read.
> 
> I'll look forward in the direction you propose. First, we need to
> confirm in which form the arguments can be expected. It depends on your
> proposal to use a --watch string formatted arguments.
> 
>>
>>> +	}
>>> +
>>> +	/* fallback to number */
>>
>> I'm not sure it makes sense to support numbers. Although it's true that
>> the component type, component scope, and event type are passed as __u8
>> values, users are expected to treat those values are opaque and pass
>> them via the respective enum constants. Since we don't guarantee that
>> the specific enum constant values will remain consistent between kernel
>> versions, I don't think it's a good idea to give to users that sort of
>> implication by allowing them to use raw numbers for configuration
>> selection.
> 
> Ack, I can remove this.
> 
> I'm a bit surprised by this statement. I may be wrong... I'd expect a
> userland binary to be compatible when updating to a newer kernel: e.g.
> user API (ABI?) definitions to be stable (including enum constants) ?
> 
>>
>>> +	idx = strtoul(optarg, &dummy, 10);
>>> +	if (!errno) {
>>> +		if (idx >= sz)
>>> +			return -EINVAL;
>>> +		*val = idx;
>>> +		return 0;
>>> +	}
>>> +
>>> +	return -errno;
>>> +}
>>> +
>>> +static const char * const counter_event_type_name[] = {
>>> +	"COUNTER_EVENT_OVERFLOW",
>>> +	"COUNTER_EVENT_UNDERFLOW",
>>> +	"COUNTER_EVENT_OVERFLOW_UNDERFLOW",
>>> +	"COUNTER_EVENT_THRESHOLD",
>>> +	"COUNTER_EVENT_INDEX",
>>> +	"COUNTER_EVENT_CHANGE_OF_STATE",
>>> +	"COUNTER_EVENT_CAPTURE",
>>> +};
>>> +
>>> +static int counter_arg_to_event_type(char *arg, __u8 *event)
>>> +{
>>> +	return find_match_or_number_from_array(arg, counter_event_type_name,
>>> +					       ARRAY_SIZE(counter_event_type_name), event);
>>> +}
>>> +
>>> +static const char * const counter_component_type_name[] = {
>>> +	"COUNTER_COMPONENT_NONE",
>>> +	"COUNTER_COMPONENT_SIGNAL",
>>> +	"COUNTER_COMPONENT_COUNT",
>>> +	"COUNTER_COMPONENT_FUNCTION",
>>> +	"COUNTER_COMPONENT_SYNAPSE_ACTION",
>>> +	"COUNTER_COMPONENT_EXTENSION",
>>> +};
>>> +
>>> +static int counter_arg_to_component_type(char *arg, __u8 *type)
>>> +{
>>> +	return find_match_or_number_from_array(arg, counter_component_type_name,
>>> +					       ARRAY_SIZE(counter_component_type_name), type);
>>> +}
>>> +
>>> +static const char * const counter_scope_name[] = {
>>> +	"COUNTER_SCOPE_DEVICE",
>>> +	"COUNTER_SCOPE_SIGNAL",
>>> +	"COUNTER_SCOPE_COUNT",
>>> +};
>>> +
>>> +static int counter_arg_to_scope(char *arg, __u8 *type)
>>> +{
>>> +	return find_match_or_number_from_array(arg, counter_scope_name,
>>> +					       ARRAY_SIZE(counter_scope_name), type);
>>> +}
>>> +
>>> +static void print_usage(void)
>>> +{
>>> +	fprintf(stderr, "Usage: counter_watch_events [options]...\n"
>>> +		"Test various watch events for given counter device\n"
>>> +		"  --channel -c <n>\n"
>>> +		"        Set watch.channel\n"
>>> +		"  --debug -d\n"
>>> +		"        Prints debug information\n"
>>> +		"  --event -e <number or counter_event_type string>\n"
>>> +		"        Sets watch.event\n"
>>> +		"  --help -h\n"
>>> +		"        Prints usage\n"
>>> +		"  --device-num -n <n>\n"
>>> +		"        Set device number (/dev/counter<n>, default to 0)\n"
>>> +		"  --id -i <n>\n"
>>> +		"        Set watch.component.id\n"
>>> +		"  --loop -l <n>\n"
>>> +		"        Loop for a number of events (forever if n < 0)\n"
>>> +		"  --parent -p <n>\n"
>>> +		"        Set watch.component.parent number\n"
>>> +		"  --scope -s <number or counter_scope string>\n"
>>> +		"        Set watch.component.scope\n"
>>> +		"  --type -t <number or counter_component_type string>\n"
>>> +		"        Set watch.component.type\n"
>>> +		"\n"
>>> +		"Example with two watched events:\n\n"
>>> +		"counter_watch_events -d \\\n"
>>> +		"\t-t COUNTER_COMPONENT_COUNT -s COUNTER_SCOPE_COUNT"
>>> +		" -e COUNTER_EVENT_OVERFLOW_UNDERFLOW -i 0 -c 0 \\\n"
>>> +		"\t-t COUNTER_COMPONENT_EXTENSION -s COUNTER_SCOPE_COUNT"
>>> +		" -e COUNTER_EVENT_CAPTURE -i 7 -c 3\n"
>>> +		);
>>> +}
>>
>> Are you following any particular convention for the usage description? I
>> wonder if there is a particular preferred standard for command-line
>> interface descriptions. A quick search brought up a few, such as the
>> POSIX Utility Conventions[^1] and docopt[^2].
>>
>> One improvement I would recommend here is to put the short form of the
>> option before the long form and separate them with a command to make it
>> clearer (e.g. "-h, --help").
>>
>> [^1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
>> [^2] http://docopt.org
> 
> Thanks for pointing this! So definitely a good pointer, and suggestion
> to look at!
> 
> I'll try to improve in v2.
> 
>>
>>> +
>>> +static void print_watch(struct counter_watch *watch, int nwatch)
>>> +{
>>> +	int i;
>>> +
>>> +	/* prints the watch array in C-like structure */
>>> +	printf("watch[%d] = {\n", nwatch);
>>> +	for (i = 0; i < nwatch; i++) {
>>> +		printf(" [%d] =\t{\n"
>>> +		       "\t\t.component.type = %s\n"
>>> +		       "\t\t.component.scope = %s\n"
>>> +		       "\t\t.component.parent = %d\n"
>>> +		       "\t\t.component.id = %d\n"
>>> +		       "\t\t.event = %s\n"
>>> +		       "\t\t.channel = %d\n"
>>> +		       "\t},\n",
>>> +		       i,
>>> +		       counter_component_type_name[watch[i].component.type],
>>> +		       counter_scope_name[watch[i].component.scope],
>>> +		       watch[i].component.parent,
>>> +		       watch[i].component.id,
>>> +		       counter_event_type_name[watch[i].event],
>>> +		       watch[i].channel);
>>> +	}
>>> +	printf("};\n");
>>> +}
>>> +
>>> +static const struct option longopts[] = {
>>> +	{ "channel",		required_argument, 0, 'c' },
>>> +	{ "debug",		no_argument,       0, 'd' },
>>> +	{ "event",		required_argument, 0, 'e' },
>>> +	{ "help",		no_argument,       0, 'h' },
>>> +	{ "device-num",		required_argument, 0, 'n' },
>>> +	{ "id",			required_argument, 0, 'i' },
>>> +	{ "loop",		required_argument, 0, 'l' },
>>> +	{ "parent",		required_argument, 0, 'p' },
>>> +	{ "scope",		required_argument, 0, 's' },
>>> +	{ "type",		required_argument, 0, 't' },
>>> +	{ },
>>> +};
>>> +
>>> +int main(int argc, char **argv)
>>> +{
>>> +	int c, fd, i, ret;
>>> +	struct counter_event event_data;
>>> +	char *device_name = NULL;
>>> +	int debug = 0, loop = -1;
>>> +	char *dummy;
>>> +	int dev_num = 0, nwatch = 0, ncfg[] = {0, 0, 0, 0, 0, 0};
>>> +	int num_chan = 0, num_evt = 0, num_id = 0, num_p = 0, num_s = 0, num_t = 0;
>>> +	struct counter_watch *watches;
>>> +
>>> +	/*
>>> +	 * 1st pass: count events configurations number to allocate the watch array.
>>> +	 * Each watch argument can be repeated several times: each time it gets repeated,
>>> +	 * a corresponding watch is allocated (and configured) in 2nd pass.
>>> +	 */
>>
>> It feels a somewhat prone to error (at least cumbersome) to populate
> 
> Yes, this could be error prone. This is also why I added a print of the
> gathered arguments when using --debug option.
> Perhaps this could be better to always print it (e.g. print_watch()) ?
> 
>> each watch via individual arguments for each field. Since a watch always
>> has these fields, perhaps instead we could pass some format string that
>> represents a watch, and deliminate watches via commas. For example, we
>> could have --watch="cco00,ecc73" to represent the two watches in the
>> usage example.
> 
> I like the idea, to concatenate as a string. With current approach, the
> command line quickly becomes very long.
> 
> It makes it obvious in your example, that two watches are used, and no
> argument is omitted.
> On the opposite, each argument isn't very easy to understand compared to
> plain text definition.
> 
>>
>> Of course, we'd need to define a more robust format string convention
>> than in my example to ensure the correct configuration is properly
> 
> Indeed, by using a single letter, we could face limitations (ex:
> overflow, underflow, overflow_underflow, which letter for the 3rd here?)
> 
> If we go this way, probably need to brainstorm a bit.
> 
>> communicated. What do you think, would this approach would make things
>> simpler, or just more complicated in the end?
> 
> I'm not 100% sure if some helpers like getopt() will help here? So, I
> guess this could be more complicated. This may also be against the
> guideline "options should be preceded by the '-' delimiter character."
> in [^1] (Ok, this would rather be the --watch option, fed with watch data.)
> 
> Would you have suggestions regarding possible helpers ? Or do you have
> in mind some others tools that already adopted such approach ?


Hi William,

I've prototyped something to follow your suggestion regarding --watch=
string arguments. This may endup in more easy to read, and hopefully
simpler approach :-).

I'll post a V2 soon for this series (removing some patches that seems
already applied), or just this tool.

Thanks,
Fabrice

> 
>>
>>> +	while ((c = getopt_long(argc, argv, "c:de:hn:i:l:p:s:t:", longopts, NULL)) != -1) {
>>> +		switch (c) {
>>> +		case 'c':
>>> +			ncfg[0]++;
>>> +			break;
>>> +		case 'e':
>>> +			ncfg[1]++;
>>> +			break;
>>> +		case 'i':
>>> +			ncfg[2]++;
>>> +			break;
>>> +		case 'p':
>>> +			ncfg[3]++;
>>> +			break;
>>> +		case 's':
>>> +			ncfg[4]++;
>>> +			break;
>>> +		case 't':
>>> +			ncfg[5]++;
>>> +			break;
>>> +		};
>>> +	};
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(ncfg); i++)
>>> +		if (ncfg[i] > nwatch)
>>> +			nwatch = ncfg[i];
>>> +
>>> +	if (nwatch) {
>>> +		watches = calloc(nwatch, sizeof(*watches));
>>
>> We need to check if calloc fails, right?
> 
> Yes, you're right, will fix this too.
> 
> Thanks for reviewing!
> Best regards,
> Fabrice
> 
>>
>> William Breathitt Gray 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] tools/counter: add a flexible watch events tool

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 3766 bytes --]

On Tue, Sep 19, 2023 at 05:37:34PM +0200, Fabrice Gasnier wrote:
> On 9/17/23 21:07, William Breathitt Gray wrote:
> > On Tue, Aug 29, 2023 at 03:40:24PM +0200, Fabrice Gasnier wrote:
> >> This adds a new counter tool to be able to test various watch events.
> >> A flexible watch array can be populated from command line, each field
> >> may be tuned with a dedicated command line argument.
> >> Each argument can be repeated several times: each time it gets repeated,
> >> a corresponding new watch element is allocated.
> >>
> >> It also comes with a simple default watch (to monitor overflows), used
> >> when no watch parameters are provided.
> >>
> >> The print_usage() routine proposes another example, from the command line,
> >> which generates a 2 elements watch array, to monitor:
> >> - overflow events
> >> - capture events, on channel 3, that reads read captured data by
> >>   specifying the component id (capture3_component_id being 7 here).
> >>
> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Fabrice,

Sorry for the delay, I'm currently working through my backlog. I see you
have already submitted a version 2 of this patchset, so I'll continue my
review there but I do want to make some direct replys here below as
well.

> > This is a new tool, so would you add a MAINTAINERS entry for the
> > counter_watch_events.c file?
> 
> I haven't thought about it.
> I can add a MAINTAINERS entry, yes!
> Who would you suggest ?

Typically the author of the initial patch will also maintain what they
are introducing, but an alternative person is acceptable to serve as
maintainer if that's the plan that's agreed upon. I assume you're
introducing this tool because you're using it internally at ST for
testing, so I suppose the intention is not to get this merged upstream
just to abandon it (i.e. you intend to keep using and improving it). Is
the plan for you to maintain this utility, or is someone else at ST
interested in it?

> >> +	}
> >> +
> >> +	/* fallback to number */
> > 
> > I'm not sure it makes sense to support numbers. Although it's true that
> > the component type, component scope, and event type are passed as __u8
> > values, users are expected to treat those values are opaque and pass
> > them via the respective enum constants. Since we don't guarantee that
> > the specific enum constant values will remain consistent between kernel
> > versions, I don't think it's a good idea to give to users that sort of
> > implication by allowing them to use raw numbers for configuration
> > selection.
> 
> Ack, I can remove this.
> 
> I'm a bit surprised by this statement. I may be wrong... I'd expect a
> userland binary to be compatible when updating to a newer kernel: e.g.
> user API (ABI?) definitions to be stable (including enum constants) ?

I was wrong in my previous reply. You're absolutely correct[^1] that
userspace ABI must be consistent ("Breaking user programs simply isn't
acceptable"[^2]) so the specific values must remain the same between
kernel versions.

Regardless, I don't think raw numbers provide much benefit for the
use-case of this particular utility; users are testing watch
configurations for a particular device, not the specific constant values
in the data structures. So in the end I still think the raw numbers
code path should be removed.

[^1] Well technically Linux kernel ABI README documentation file
     (Documentation/ABI/README) states that backwards compatibility for
     stable interfaces is only guaranteed for at least 2 years, but in
     practice we strive to never change the user-facing ABI.
[^2] https://yarchive.net/comp/linux/gcc_vs_kernel_stability.html

William Breathitt Gray

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/8] mfd: stm32-timers: add support for interrupts

[Fabrice Gasnier]

There are two types of STM32 timers that may have:
- a global interrupt line
- 4 dedicated interrupt lines.
Those interrupts are optional as defined in the dt-bindings. Enforce checks
on either one, four or no interrupts are provided with their names.
Optionally get them here, to be used by child devices.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/mfd/stm32-timers.c       | 46 ++++++++++++++++++++++++++++++++
 include/linux/mfd/stm32-timers.h | 11 ++++++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
index 44ed2fce0319..51fb97bdab9c 100644
--- a/drivers/mfd/stm32-timers.c
+++ b/drivers/mfd/stm32-timers.c
@@ -214,6 +214,48 @@ static void stm32_timers_dma_remove(struct device *dev,
 			dma_release_channel(ddata->dma.chans[i]);
 }
 
+static const char * const stm32_timers_irq_name[STM32_TIMERS_MAX_IRQS] = {
+	"brk", "up", "trg-com", "cc"
+};
+
+static int stm32_timers_irq_probe(struct platform_device *pdev,
+				  struct stm32_timers *ddata)
+{
+	int i, ret;
+
+	/*
+	 * STM32 Timer may have either:
+	 * - a unique global interrupt line
+	 * - four dedicated interrupt lines that may be handled separately.
+	 * Optionally get them here, to be used by child devices.
+	 */
+	ret = platform_get_irq_byname_optional(pdev, "global");
+	if (ret < 0 && ret != -ENXIO) {
+		return ret;
+	} else if (ret != -ENXIO) {
+		ddata->irq[STM32_TIMERS_IRQ_GLOBAL_BRK] = ret;
+		ddata->nr_irqs = 1;
+		return 0;
+	}
+
+	for (i = 0; i < STM32_TIMERS_MAX_IRQS; i++) {
+		ret = platform_get_irq_byname_optional(pdev, stm32_timers_irq_name[i]);
+		if (ret < 0 && ret != -ENXIO) {
+			return ret;
+		} else if (ret != -ENXIO) {
+			ddata->irq[i] = ret;
+			ddata->nr_irqs++;
+		}
+	}
+
+	if (ddata->nr_irqs && ddata->nr_irqs != STM32_TIMERS_MAX_IRQS) {
+		dev_err(&pdev->dev, "Invalid number of IRQs %d\n", ddata->nr_irqs);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int stm32_timers_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -245,6 +287,10 @@ static int stm32_timers_probe(struct platform_device *pdev)
 
 	stm32_timers_get_arr_size(ddata);
 
+	ret = stm32_timers_irq_probe(pdev, ddata);
+	if (ret)
+		return ret;
+
 	ret = stm32_timers_dma_probe(dev, ddata);
 	if (ret) {
 		stm32_timers_dma_remove(dev, ddata);
diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
index 1b94325febb3..ca35af30745f 100644
--- a/include/linux/mfd/stm32-timers.h
+++ b/include/linux/mfd/stm32-timers.h
@@ -102,6 +102,15 @@ enum stm32_timers_dmas {
 	STM32_TIMERS_MAX_DMAS,
 };
 
+/* STM32 Timer may have either a unique global interrupt or 4 interrupt lines */
+enum stm32_timers_irqs {
+	STM32_TIMERS_IRQ_GLOBAL_BRK, /* global or brk IRQ */
+	STM32_TIMERS_IRQ_UP,
+	STM32_TIMERS_IRQ_TRG_COM,
+	STM32_TIMERS_IRQ_CC,
+	STM32_TIMERS_MAX_IRQS,
+};
+
 /**
  * struct stm32_timers_dma - STM32 timer DMA handling.
  * @completion:		end of DMA transfer completion
@@ -123,6 +132,8 @@ struct stm32_timers {
 	struct regmap *regmap;
 	u32 max_arr;
 	struct stm32_timers_dma dma; /* Only to be used by the parent */
+	unsigned int nr_irqs;
+	int irq[STM32_TIMERS_MAX_IRQS];
 };
 
 #if IS_REACHABLE(CONFIG_MFD_STM32_TIMERS)
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (subset) [PATCH 4/8] mfd: stm32-timers: add support for interrupts

[Lee Jones]

On Tue, 29 Aug 2023 15:40:25 +0200, Fabrice Gasnier wrote:
> There are two types of STM32 timers that may have:
> - a global interrupt line
> - 4 dedicated interrupt lines.
> Those interrupts are optional as defined in the dt-bindings. Enforce checks
> on either one, four or no interrupts are provided with their names.
> Optionally get them here, to be used by child devices.
> 
> [...]

Applied, thanks!

[4/8] mfd: stm32-timers: add support for interrupts
      commit: f747b627d395c4eb8a82849dcccf31bf46b21504

--
Lee Jones [李琼斯]


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/8] counter: stm32-timer-cnt: rename quadrature signal

[Fabrice Gasnier]

Rename "Channel 1 Quadrature B", as it corresponds to timer input ch2.
I suspect it referred to the (unique) counter earlier, but the physical
input really is CH2.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/counter/stm32-timer-cnt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index 6206d2dc3d47..eae851f6db2c 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -283,7 +283,7 @@ static struct counter_signal stm32_signals[] = {
 	},
 	{
 		.id = 1,
-		.name = "Channel 1 Quadrature B"
+		.name = "Channel 2 Quadrature B"
 	}
 };
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/8] counter: stm32-timer-cnt: rename quadrature signal

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 1089 bytes --]

On Tue, Aug 29, 2023 at 03:40:26PM +0200, Fabrice Gasnier wrote:
> Rename "Channel 1 Quadrature B", as it corresponds to timer input ch2.
> I suspect it referred to the (unique) counter earlier, but the physical
> input really is CH2.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> ---
>  drivers/counter/stm32-timer-cnt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
> index 6206d2dc3d47..eae851f6db2c 100644
> --- a/drivers/counter/stm32-timer-cnt.c
> +++ b/drivers/counter/stm32-timer-cnt.c
> @@ -283,7 +283,7 @@ static struct counter_signal stm32_signals[] = {
>  	},
>  	{
>  		.id = 1,
> -		.name = "Channel 1 Quadrature B"
> +		.name = "Channel 2 Quadrature B"
>  	}
>  };
>  
> -- 
> 2.25.1
> 

If these signals are going to be named after their channels, then I
prefer we just remove the "Quadrature A" and "Quadrature B" conventions
and leave the signals named "Channel 1" and "Channel 2" respectively.

William Breathitt Gray

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/8] counter: stm32-timer-cnt: introduce clock signal

[Fabrice Gasnier]

Introduce the internal clock signal, used to count when in simple rising
function. Define signal ids, to improve readability. Also add the
"frequency" attribute for the clock signal, and "prescaler" for the
counter.

Whit this patch, signal action reports consistent state when "increase"
function is used, and the counting frequency:
$ echo increase > function
$ grep -H "" signal*_action
signal0_action:rising edge
signal1_action:none
signal2_action:none
$ echo 1 > enable
$ cat count
25425
$ cat count
44439
$ cat ../signal0/frequency
208877930

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/counter/stm32-timer-cnt.c | 84 ++++++++++++++++++++++++++++---
 1 file changed, 76 insertions(+), 8 deletions(-)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index eae851f6db2c..b8d201383a64 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -21,6 +21,10 @@
 #define TIM_CCER_MASK	(TIM_CCER_CC1P | TIM_CCER_CC1NP | \
 			 TIM_CCER_CC2P | TIM_CCER_CC2NP)
 
+#define STM32_CLOCK_SIG		0
+#define STM32_CH1_SIG		1
+#define STM32_CH2_SIG		2
+
 struct stm32_timer_regs {
 	u32 cr1;
 	u32 cnt;
@@ -216,11 +220,44 @@ static int stm32_count_enable_write(struct counter_device *counter,
 	return 0;
 }
 
+static int stm32_count_prescaler_read(struct counter_device *counter,
+				      struct counter_count *count, u64 *prescaler)
+{
+	struct stm32_timer_cnt *const priv = counter_priv(counter);
+	u32 psc;
+
+	regmap_read(priv->regmap, TIM_PSC, &psc);
+
+	*prescaler = psc + 1;
+
+	return 0;
+}
+
+static int stm32_count_prescaler_write(struct counter_device *counter,
+				       struct counter_count *count, u64 prescaler)
+{
+	struct stm32_timer_cnt *const priv = counter_priv(counter);
+	u32 psc;
+
+	if (!prescaler || prescaler > MAX_TIM_PSC + 1)
+		return -ERANGE;
+
+	psc = prescaler - 1;
+
+	return regmap_write(priv->regmap, TIM_PSC, psc);
+}
+
 static struct counter_comp stm32_count_ext[] = {
 	COUNTER_COMP_DIRECTION(stm32_count_direction_read),
 	COUNTER_COMP_ENABLE(stm32_count_enable_read, stm32_count_enable_write),
 	COUNTER_COMP_CEILING(stm32_count_ceiling_read,
 			     stm32_count_ceiling_write),
+	COUNTER_COMP_COUNT_U64("prescaler", stm32_count_prescaler_read,
+			       stm32_count_prescaler_write),
+};
+
+static const enum counter_synapse_action stm32_clock_synapse_actions[] = {
+	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
 };
 
 static const enum counter_synapse_action stm32_synapse_actions[] = {
@@ -243,25 +280,31 @@ static int stm32_action_read(struct counter_device *counter,
 	switch (function) {
 	case COUNTER_FUNCTION_INCREASE:
 		/* counts on internal clock when CEN=1 */
-		*action = COUNTER_SYNAPSE_ACTION_NONE;
+		if (synapse->signal->id == STM32_CLOCK_SIG)
+			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
+		else
+			*action = COUNTER_SYNAPSE_ACTION_NONE;
 		return 0;
 	case COUNTER_FUNCTION_QUADRATURE_X2_A:
 		/* counts up/down on TI1FP1 edge depending on TI2FP2 level */
-		if (synapse->signal->id == count->synapses[0].signal->id)
+		if (synapse->signal->id == STM32_CH1_SIG)
 			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
 		else
 			*action = COUNTER_SYNAPSE_ACTION_NONE;
 		return 0;
 	case COUNTER_FUNCTION_QUADRATURE_X2_B:
 		/* counts up/down on TI2FP2 edge depending on TI1FP1 level */
-		if (synapse->signal->id == count->synapses[1].signal->id)
+		if (synapse->signal->id == STM32_CH2_SIG)
 			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
 		else
 			*action = COUNTER_SYNAPSE_ACTION_NONE;
 		return 0;
 	case COUNTER_FUNCTION_QUADRATURE_X4:
 		/* counts up/down on both TI1FP1 and TI2FP2 edges */
-		*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
+		if (synapse->signal->id == STM32_CH1_SIG || synapse->signal->id == STM32_CH2_SIG)
+			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
+		else
+			*action = COUNTER_SYNAPSE_ACTION_NONE;
 		return 0;
 	default:
 		return -EINVAL;
@@ -276,27 +319,52 @@ static const struct counter_ops stm32_timer_cnt_ops = {
 	.action_read = stm32_action_read,
 };
 
+static int stm32_count_clk_get_freq(struct counter_device *counter,
+				    struct counter_signal *signal, u64 *freq)
+{
+	struct stm32_timer_cnt *const priv = counter_priv(counter);
+
+	*freq = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static struct counter_comp stm32_count_clock_ext[] = {
+	COUNTER_COMP_SIGNAL_U64("frequency", stm32_count_clk_get_freq, NULL),
+};
+
 static struct counter_signal stm32_signals[] = {
 	{
-		.id = 0,
+		.id = STM32_CLOCK_SIG,
+		.name = "Clock Signal",
+		.ext = stm32_count_clock_ext,
+		.num_ext = ARRAY_SIZE(stm32_count_clock_ext),
+	},
+	{
+		.id = STM32_CH1_SIG,
 		.name = "Channel 1 Quadrature A"
 	},
 	{
-		.id = 1,
+		.id = STM32_CH2_SIG,
 		.name = "Channel 2 Quadrature B"
 	}
 };
 
 static struct counter_synapse stm32_count_synapses[] = {
+	{
+		.actions_list = stm32_clock_synapse_actions,
+		.num_actions = ARRAY_SIZE(stm32_clock_synapse_actions),
+		.signal = &stm32_signals[STM32_CLOCK_SIG]
+	},
 	{
 		.actions_list = stm32_synapse_actions,
 		.num_actions = ARRAY_SIZE(stm32_synapse_actions),
-		.signal = &stm32_signals[0]
+		.signal = &stm32_signals[STM32_CH1_SIG]
 	},
 	{
 		.actions_list = stm32_synapse_actions,
 		.num_actions = ARRAY_SIZE(stm32_synapse_actions),
-		.signal = &stm32_signals[1]
+		.signal = &stm32_signals[STM32_CH2_SIG]
 	}
 };
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 7/8] counter: stm32-timer-cnt: populate capture channels and check encoder

[Fabrice Gasnier]

This is a precursor patch to support capture channels on all possible
channels and stm32 timer types. Original driver was intended to be used
only as quadrature encoder and simple counter on internal clock.

So, add ch3 and ch4 definition. Also add a check on encoder capability,
so the driver may be probed for timer instances without encoder feature.
This way, all timers may be used as simple counter on internal clock,
starting from here.
Encoder capability is retrieved by using the timer index (originally in
stm32-timer-trigger driver and dt-bindings). The need to keep backward
compatibility with existing device tree lead to parse aside trigger node.
Add diversity as STM32 timers with capture feature may have either 4, 2,
1 or no cc (capture/compare) channels.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/counter/stm32-timer-cnt.c | 242 ++++++++++++++++++++++++++++--
 1 file changed, 231 insertions(+), 11 deletions(-)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index b8d201383a64..e39b3964bc9d 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -11,6 +11,7 @@
 #include <linux/mfd/stm32-timers.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
@@ -24,6 +25,8 @@
 #define STM32_CLOCK_SIG		0
 #define STM32_CH1_SIG		1
 #define STM32_CH2_SIG		2
+#define STM32_CH3_SIG		3
+#define STM32_CH4_SIG		4
 
 struct stm32_timer_regs {
 	u32 cr1;
@@ -38,6 +41,9 @@ struct stm32_timer_cnt {
 	u32 max_arr;
 	bool enabled;
 	struct stm32_timer_regs bak;
+	bool has_encoder;
+	u32 idx;
+	unsigned int nchannels;
 };
 
 static const enum counter_function stm32_count_functions[] = {
@@ -265,6 +271,10 @@ static const enum counter_synapse_action stm32_synapse_actions[] = {
 	COUNTER_SYNAPSE_ACTION_BOTH_EDGES
 };
 
+static const enum counter_synapse_action stm32_synapse_ch_actions[] = {
+	COUNTER_SYNAPSE_ACTION_NONE,
+};
+
 static int stm32_action_read(struct counter_device *counter,
 			     struct counter_count *count,
 			     struct counter_synapse *synapse,
@@ -333,7 +343,8 @@ static struct counter_comp stm32_count_clock_ext[] = {
 	COUNTER_COMP_SIGNAL_U64("frequency", stm32_count_clk_get_freq, NULL),
 };
 
-static struct counter_signal stm32_signals[] = {
+/* STM32 Timer with quadrature encoder and 4 capture channels */
+static struct counter_signal stm32_signals_enc_4ch[] = {
 	{
 		.id = STM32_CLOCK_SIG,
 		.name = "Clock Signal",
@@ -347,38 +358,210 @@ static struct counter_signal stm32_signals[] = {
 	{
 		.id = STM32_CH2_SIG,
 		.name = "Channel 2 Quadrature B"
+	},
+	{
+		.id = STM32_CH3_SIG,
+		.name = "Channel 3"
+	},
+	{
+		.id = STM32_CH4_SIG,
+		.name = "Channel 4"
 	}
 };
 
-static struct counter_synapse stm32_count_synapses[] = {
+static struct counter_synapse stm32_count_synapses_enc_4ch[] = {
 	{
 		.actions_list = stm32_clock_synapse_actions,
 		.num_actions = ARRAY_SIZE(stm32_clock_synapse_actions),
-		.signal = &stm32_signals[STM32_CLOCK_SIG]
+		.signal = &stm32_signals_enc_4ch[STM32_CLOCK_SIG]
 	},
 	{
 		.actions_list = stm32_synapse_actions,
 		.num_actions = ARRAY_SIZE(stm32_synapse_actions),
-		.signal = &stm32_signals[STM32_CH1_SIG]
+		.signal = &stm32_signals_enc_4ch[STM32_CH1_SIG]
 	},
 	{
 		.actions_list = stm32_synapse_actions,
 		.num_actions = ARRAY_SIZE(stm32_synapse_actions),
-		.signal = &stm32_signals[STM32_CH2_SIG]
+		.signal = &stm32_signals_enc_4ch[STM32_CH2_SIG]
+	},
+	{
+		.actions_list = stm32_synapse_ch_actions,
+		.num_actions = ARRAY_SIZE(stm32_synapse_ch_actions),
+		.signal = &stm32_signals_enc_4ch[STM32_CH3_SIG]
+	},
+	{
+		.actions_list = stm32_synapse_ch_actions,
+		.num_actions = ARRAY_SIZE(stm32_synapse_ch_actions),
+		.signal = &stm32_signals_enc_4ch[STM32_CH4_SIG]
+	},
+};
+
+static struct counter_count stm32_counts_enc_4ch = {
+	.id = 0,
+	.name = "Channel 1 Count",
+	.functions_list = stm32_count_functions,
+	.num_functions = ARRAY_SIZE(stm32_count_functions),
+	.synapses = stm32_count_synapses_enc_4ch,
+	.num_synapses = ARRAY_SIZE(stm32_count_synapses_enc_4ch),
+	.ext = stm32_count_ext,
+	.num_ext = ARRAY_SIZE(stm32_count_ext)
+};
+
+/* STM32 Timer with up to 4 capture channels (without encoder) */
+static struct counter_signal stm32_signals_4ch[] = {
+	{
+		.id = STM32_CLOCK_SIG,
+		.name = "Clock Signal",
+		.ext = stm32_count_clock_ext,
+		.num_ext = ARRAY_SIZE(stm32_count_clock_ext),
+	},
+	{
+		.id = STM32_CH1_SIG,
+		.name = "Channel 1"
+	},
+	{
+		.id = STM32_CH2_SIG,
+		.name = "Channel 2"
+	},
+	{
+		.id = STM32_CH3_SIG,
+		.name = "Channel 3"
+	},
+	{
+		.id = STM32_CH4_SIG,
+		.name = "Channel 4"
 	}
 };
 
+static struct counter_synapse stm32_count_synapses_no_enc[] = {
+	{
+		.actions_list = stm32_clock_synapse_actions,
+		.num_actions = ARRAY_SIZE(stm32_clock_synapse_actions),
+		.signal = &stm32_signals_enc_4ch[STM32_CLOCK_SIG]
+	},
+	{
+		.actions_list = stm32_synapse_ch_actions,
+		.num_actions = ARRAY_SIZE(stm32_synapse_ch_actions),
+		.signal = &stm32_signals_enc_4ch[STM32_CH1_SIG]
+	},
+	{
+		.actions_list = stm32_synapse_ch_actions,
+		.num_actions = ARRAY_SIZE(stm32_synapse_ch_actions),
+		.signal = &stm32_signals_enc_4ch[STM32_CH2_SIG]
+	},
+	{
+		.actions_list = stm32_synapse_ch_actions,
+		.num_actions = ARRAY_SIZE(stm32_synapse_ch_actions),
+		.signal = &stm32_signals_enc_4ch[STM32_CH3_SIG]
+	},
+	{
+		.actions_list = stm32_synapse_ch_actions,
+		.num_actions = ARRAY_SIZE(stm32_synapse_ch_actions),
+		.signal = &stm32_signals_enc_4ch[STM32_CH4_SIG]
+	},
+};
+
+static struct counter_count stm32_counts_4ch = {
+	.id = 0,
+	.name = "Channel 1 Count",
+	.functions_list = stm32_count_functions,
+	.num_functions = 1, /* increase */
+	.synapses = stm32_count_synapses_no_enc,
+	.num_synapses = ARRAY_SIZE(stm32_count_synapses_no_enc),
+	.ext = stm32_count_ext,
+	.num_ext = ARRAY_SIZE(stm32_count_ext)
+};
+
+static struct counter_count stm32_counts_2ch = {
+	.id = 0,
+	.name = "Channel 1 Count",
+	.functions_list = stm32_count_functions,
+	.num_functions = 1, /* increase */
+	.synapses = stm32_count_synapses_no_enc,
+	.num_synapses = 3, /* clock, ch1 and ch2 */
+	.ext = stm32_count_ext,
+	.num_ext = ARRAY_SIZE(stm32_count_ext)
+};
+
+static struct counter_count stm32_counts_1ch = {
+	.id = 0,
+	.name = "Channel 1 Count",
+	.functions_list = stm32_count_functions,
+	.num_functions = 1, /* increase */
+	.synapses = stm32_count_synapses_no_enc,
+	.num_synapses = 2, /* clock, ch1 */
+	.ext = stm32_count_ext,
+	.num_ext = ARRAY_SIZE(stm32_count_ext)
+};
+
 static struct counter_count stm32_counts = {
 	.id = 0,
 	.name = "Channel 1 Count",
 	.functions_list = stm32_count_functions,
-	.num_functions = ARRAY_SIZE(stm32_count_functions),
-	.synapses = stm32_count_synapses,
-	.num_synapses = ARRAY_SIZE(stm32_count_synapses),
+	.num_functions = 1, /* increase */
+	.synapses = stm32_count_synapses_no_enc,
+	.num_synapses = 1, /* clock only */
 	.ext = stm32_count_ext,
 	.num_ext = ARRAY_SIZE(stm32_count_ext)
 };
 
+static void stm32_timer_cnt_detect_channels(struct platform_device *pdev,
+					    struct stm32_timer_cnt *priv)
+{
+	u32 ccer, ccer_backup;
+
+	regmap_read(priv->regmap, TIM_CCER, &ccer_backup);
+	regmap_set_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE);
+	regmap_read(priv->regmap, TIM_CCER, &ccer);
+	regmap_write(priv->regmap, TIM_CCER, ccer_backup);
+	priv->nchannels = hweight32(ccer & TIM_CCER_CCXE);
+
+	dev_dbg(&pdev->dev, "has %d cc channels\n", priv->nchannels);
+}
+
+/* encoder supported on TIM1 TIM2 TIM3 TIM4 TIM5 TIM8 */
+#define STM32_TIM_ENCODER_SUPPORTED	(BIT(0) | BIT(1) | BIT(2) | BIT(3) | BIT(4) | BIT(7))
+
+static const char * const stm32_timer_trigger_compat[] = {
+	"st,stm32-timer-trigger",
+	"st,stm32h7-timer-trigger",
+};
+
+static int stm32_timer_cnt_probe_encoder(struct platform_device *pdev,
+					 struct stm32_timer_cnt *priv)
+{
+	struct device *parent = pdev->dev.parent;
+	struct device_node *tnode = NULL, *pnode = parent->of_node;
+	int i, ret;
+
+	/*
+	 * Need to retrieve the trigger node index from DT, to be able
+	 * to determine if the counter supports encoder mode. It also
+	 * enforce backward compatibility, and allow to support other
+	 * counter modes in this driver (when the timer doesn't support
+	 * encoder).
+	 */
+	for (i = 0; i < ARRAY_SIZE(stm32_timer_trigger_compat) && !tnode; i++)
+		tnode = of_get_compatible_child(pnode, stm32_timer_trigger_compat[i]);
+	if (!tnode) {
+		dev_err(&pdev->dev, "Can't find trigger node\n");
+		return -ENODATA;
+	}
+
+	ret = of_property_read_u32(tnode, "reg", &priv->idx);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't get index (%d)\n", ret);
+		return ret;
+	}
+
+	priv->has_encoder = !!(STM32_TIM_ENCODER_SUPPORTED & BIT(priv->idx));
+
+	dev_dbg(&pdev->dev, "encoder support: %s\n", priv->has_encoder ? "yes" : "no");
+
+	return 0;
+}
+
 static int stm32_timer_cnt_probe(struct platform_device *pdev)
 {
 	struct stm32_timers *ddata = dev_get_drvdata(pdev->dev.parent);
@@ -400,13 +583,50 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
 	priv->clk = ddata->clk;
 	priv->max_arr = ddata->max_arr;
 
+	ret = stm32_timer_cnt_probe_encoder(pdev, priv);
+	if (ret)
+		return ret;
+
+	stm32_timer_cnt_detect_channels(pdev, priv);
+
 	counter->name = dev_name(dev);
 	counter->parent = dev;
 	counter->ops = &stm32_timer_cnt_ops;
-	counter->counts = &stm32_counts;
 	counter->num_counts = 1;
-	counter->signals = stm32_signals;
-	counter->num_signals = ARRAY_SIZE(stm32_signals);
+
+	/*
+	 * Handle diversity for stm32 timers features. For now encoder is found with
+	 * advanced timers or gp timers with 4 channels. Timers with less channels
+	 * doesn't support encoder.
+	 */
+	switch (priv->nchannels) {
+	case 4:
+		if (priv->has_encoder) {
+			counter->counts = &stm32_counts_enc_4ch;
+			counter->signals = stm32_signals_enc_4ch;
+			counter->num_signals = ARRAY_SIZE(stm32_signals_enc_4ch);
+		} else {
+			counter->counts = &stm32_counts_4ch;
+			counter->signals = stm32_signals_4ch;
+			counter->num_signals = ARRAY_SIZE(stm32_signals_4ch);
+		}
+		break;
+	case 2:
+		counter->counts = &stm32_counts_2ch;
+		counter->signals = stm32_signals_4ch;
+		counter->num_signals = 3; /* clock, ch1 and ch2 */
+		break;
+	case 1:
+		counter->counts = &stm32_counts_1ch;
+		counter->signals = stm32_signals_4ch;
+		counter->num_signals = 2; /* clock, ch1 */
+		break;
+	default:
+		counter->counts = &stm32_counts;
+		counter->signals = stm32_signals_4ch;
+		counter->num_signals = 1; /* clock */
+		break;
+	}
 
 	platform_set_drvdata(pdev, priv);
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 8/8] counter: stm32-timer-cnt: add support for events

[Fabrice Gasnier]

Add support for capture and overflow events. Also add the related
validation and configuration. Captured counter value can be retrieved
through CCRx register. Register and enable interrupts to push events.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/counter/stm32-timer-cnt.c | 279 +++++++++++++++++++++++++++++-
 include/linux/mfd/stm32-timers.h  |  15 ++
 2 files changed, 285 insertions(+), 9 deletions(-)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index e39b3964bc9d..05188f1da0b0 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -8,6 +8,7 @@
  *
  */
 #include <linux/counter.h>
+#include <linux/interrupt.h>
 #include <linux/mfd/stm32-timers.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -37,6 +38,7 @@ struct stm32_timer_regs {
 
 struct stm32_timer_cnt {
 	struct regmap *regmap;
+	atomic_t nb_ovf;
 	struct clk *clk;
 	u32 max_arr;
 	bool enabled;
@@ -44,6 +46,8 @@ struct stm32_timer_cnt {
 	bool has_encoder;
 	u32 idx;
 	unsigned int nchannels;
+	unsigned int nr_irqs;
+	u32 *irq;
 };
 
 static const enum counter_function stm32_count_functions[] = {
@@ -253,6 +257,60 @@ static int stm32_count_prescaler_write(struct counter_device *counter,
 	return regmap_write(priv->regmap, TIM_PSC, psc);
 }
 
+static int stm32_count_cap_read(struct counter_device *counter,
+				struct counter_count *count,
+				size_t ch, u64 *cap)
+{
+	struct stm32_timer_cnt *const priv = counter_priv(counter);
+	u32 ccrx;
+
+	switch (ch) {
+	case 0:
+		regmap_read(priv->regmap, TIM_CCR1, &ccrx);
+		break;
+	case 1:
+		regmap_read(priv->regmap, TIM_CCR2, &ccrx);
+		break;
+	case 2:
+		regmap_read(priv->regmap, TIM_CCR3, &ccrx);
+		break;
+	case 3:
+		regmap_read(priv->regmap, TIM_CCR4, &ccrx);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	dev_dbg(counter->parent, "CCR%zu: 0x%08x\n", ch, ccrx);
+
+	*cap = ccrx;
+
+	return 0;
+}
+
+static int stm32_count_nb_ovf_read(struct counter_device *counter,
+				   struct counter_count *count, u64 *val)
+{
+	struct stm32_timer_cnt *const priv = counter_priv(counter);
+
+	*val = atomic_read(&priv->nb_ovf);
+
+	return 0;
+}
+
+static int stm32_count_nb_ovf_write(struct counter_device *counter,
+				    struct counter_count *count, u64 val)
+{
+	struct stm32_timer_cnt *const priv = counter_priv(counter);
+
+	if (val > U32_MAX)
+		return -ERANGE;
+
+	atomic_set(&priv->nb_ovf, val);
+
+	return 0;
+}
+
 static struct counter_comp stm32_count_ext[] = {
 	COUNTER_COMP_DIRECTION(stm32_count_direction_read),
 	COUNTER_COMP_ENABLE(stm32_count_enable_read, stm32_count_enable_write),
@@ -260,6 +318,43 @@ static struct counter_comp stm32_count_ext[] = {
 			     stm32_count_ceiling_write),
 	COUNTER_COMP_COUNT_U64("prescaler", stm32_count_prescaler_read,
 			       stm32_count_prescaler_write),
+	COUNTER_COMP_COUNT_U64("num_overflows", stm32_count_nb_ovf_read, stm32_count_nb_ovf_write),
+};
+
+static DEFINE_COUNTER_ARRAY_CAPTURE(stm32_count_cap_array_4ch, 4);
+static struct counter_comp stm32_count_4ch_ext[] = {
+	COUNTER_COMP_DIRECTION(stm32_count_direction_read),
+	COUNTER_COMP_ENABLE(stm32_count_enable_read, stm32_count_enable_write),
+	COUNTER_COMP_CEILING(stm32_count_ceiling_read,
+			     stm32_count_ceiling_write),
+	COUNTER_COMP_COUNT_U64("prescaler", stm32_count_prescaler_read,
+			       stm32_count_prescaler_write),
+	COUNTER_COMP_ARRAY_CAPTURE(stm32_count_cap_read, NULL, stm32_count_cap_array_4ch),
+	COUNTER_COMP_COUNT_U64("num_overflows", stm32_count_nb_ovf_read, stm32_count_nb_ovf_write),
+};
+
+static DEFINE_COUNTER_ARRAY_CAPTURE(stm32_count_cap_array_2ch, 2);
+static struct counter_comp stm32_count_2ch_ext[] = {
+	COUNTER_COMP_DIRECTION(stm32_count_direction_read),
+	COUNTER_COMP_ENABLE(stm32_count_enable_read, stm32_count_enable_write),
+	COUNTER_COMP_CEILING(stm32_count_ceiling_read,
+			     stm32_count_ceiling_write),
+	COUNTER_COMP_COUNT_U64("prescaler", stm32_count_prescaler_read,
+			       stm32_count_prescaler_write),
+	COUNTER_COMP_ARRAY_CAPTURE(stm32_count_cap_read, NULL, stm32_count_cap_array_2ch),
+	COUNTER_COMP_COUNT_U64("num_overflows", stm32_count_nb_ovf_read, stm32_count_nb_ovf_write),
+};
+
+static DEFINE_COUNTER_ARRAY_CAPTURE(stm32_count_cap_array_1ch, 1);
+static struct counter_comp stm32_count_1ch_ext[] = {
+	COUNTER_COMP_DIRECTION(stm32_count_direction_read),
+	COUNTER_COMP_ENABLE(stm32_count_enable_read, stm32_count_enable_write),
+	COUNTER_COMP_CEILING(stm32_count_ceiling_read,
+			     stm32_count_ceiling_write),
+	COUNTER_COMP_COUNT_U64("prescaler", stm32_count_prescaler_read,
+			       stm32_count_prescaler_write),
+	COUNTER_COMP_ARRAY_CAPTURE(stm32_count_cap_read, NULL, stm32_count_cap_array_1ch),
+	COUNTER_COMP_COUNT_U64("num_overflows", stm32_count_nb_ovf_read, stm32_count_nb_ovf_write),
 };
 
 static const enum counter_synapse_action stm32_clock_synapse_actions[] = {
@@ -321,12 +416,130 @@ static int stm32_action_read(struct counter_device *counter,
 	}
 }
 
+struct stm32_count_cc_regs {
+	u32 ccmr_reg;
+	u32 ccmr_mask;
+	u32 ccmr_bits;
+	u32 ccer_bits;
+};
+
+static const struct stm32_count_cc_regs stm32_cc[] = {
+	{ TIM_CCMR1, TIM_CCMR_CC1S, TIM_CCMR_CC1S_TI1,
+		TIM_CCER_CC1E | TIM_CCER_CC1P | TIM_CCER_CC1NP },
+	{ TIM_CCMR1, TIM_CCMR_CC2S, TIM_CCMR_CC2S_TI2,
+		TIM_CCER_CC2E | TIM_CCER_CC2P | TIM_CCER_CC2NP },
+	{ TIM_CCMR2, TIM_CCMR_CC3S, TIM_CCMR_CC3S_TI3,
+		TIM_CCER_CC3E | TIM_CCER_CC3P | TIM_CCER_CC3NP },
+	{ TIM_CCMR2, TIM_CCMR_CC4S, TIM_CCMR_CC4S_TI4,
+		TIM_CCER_CC4E | TIM_CCER_CC4P | TIM_CCER_CC4NP },
+};
+
+static int stm32_count_capture_configure(struct counter_device *counter, unsigned int ch,
+					 bool enable)
+{
+	struct stm32_timer_cnt *const priv = counter_priv(counter);
+	u32 ccmr, ccer, sr;
+
+	if (ch >= ARRAY_SIZE(stm32_cc)) {
+		dev_err(counter->parent, "invalid ch: %d\n", ch);
+		return -EINVAL;
+	}
+
+	/*
+	 * configure channel in input capture mode, map channel 1 on TI1, channel2 on TI2...
+	 * Select both edges / non-inverted to trigger a capture.
+	 */
+	if (enable) {
+		/* first clear possibly latched capture flag upon enabling */
+		regmap_read(priv->regmap, TIM_CCER, &ccer);
+		if (!(ccer & stm32_cc[ch].ccer_bits)) {
+			sr = ~TIM_SR_CC_IF(ch);
+			regmap_write(priv->regmap, TIM_SR, sr);
+		}
+		regmap_update_bits(priv->regmap, stm32_cc[ch].ccmr_reg, stm32_cc[ch].ccmr_mask,
+				   stm32_cc[ch].ccmr_bits);
+		regmap_set_bits(priv->regmap, TIM_CCER, stm32_cc[ch].ccer_bits);
+	} else {
+		regmap_clear_bits(priv->regmap, TIM_CCER, stm32_cc[ch].ccer_bits);
+		regmap_clear_bits(priv->regmap, stm32_cc[ch].ccmr_reg, stm32_cc[ch].ccmr_mask);
+	}
+
+	regmap_read(priv->regmap, stm32_cc[ch].ccmr_reg, &ccmr);
+	regmap_read(priv->regmap, TIM_CCER, &ccer);
+	dev_dbg(counter->parent, "%s(%s) ch%d 0x%08x 0x%08x\n", __func__, enable ? "ena" : "dis",
+		ch, ccmr, ccer);
+
+	return 0;
+}
+
+static int stm32_count_events_configure(struct counter_device *counter)
+{
+	struct stm32_timer_cnt *const priv = counter_priv(counter);
+	struct counter_event_node *event_node;
+	int i, ret;
+	u32 val, dier = 0;
+
+	list_for_each_entry(event_node, &counter->events_list, l) {
+		switch (event_node->event) {
+		case COUNTER_EVENT_OVERFLOW_UNDERFLOW:
+			/* first clear possibly latched UIF before enabling */
+			regmap_read(priv->regmap, TIM_DIER, &val);
+			if (!(val & TIM_DIER_UIE))
+				regmap_write(priv->regmap, TIM_SR, (u32)~TIM_SR_UIF);
+			dier |= TIM_DIER_UIE;
+			break;
+		case COUNTER_EVENT_CAPTURE:
+			ret = stm32_count_capture_configure(counter, event_node->channel, true);
+			if (ret)
+				return ret;
+			dier |= TIM_DIER_CC_IE(event_node->channel);
+			break;
+		default:
+			/* should never reach this path */
+			return -EINVAL;
+		}
+	}
+
+	regmap_write(priv->regmap, TIM_DIER, dier);
+
+	/* check for disabled capture events */
+	for (i = 0 ; i < priv->nchannels; i++) {
+		if (!(dier & TIM_DIER_CC_IE(i))) {
+			ret = stm32_count_capture_configure(counter, i, false);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int stm32_count_watch_validate(struct counter_device *counter,
+				      const struct counter_watch *watch)
+{
+	struct stm32_timer_cnt *const priv = counter_priv(counter);
+
+	switch (watch->event) {
+	case COUNTER_EVENT_CAPTURE:
+		if (watch->channel >= priv->nchannels) {
+			dev_err(counter->parent, "Invalid channel %d\n", watch->channel);
+			return -EINVAL;
+		}
+	case COUNTER_EVENT_OVERFLOW_UNDERFLOW:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct counter_ops stm32_timer_cnt_ops = {
 	.count_read = stm32_count_read,
 	.count_write = stm32_count_write,
 	.function_read = stm32_count_function_read,
 	.function_write = stm32_count_function_write,
 	.action_read = stm32_action_read,
+	.events_configure = stm32_count_events_configure,
+	.watch_validate = stm32_count_watch_validate,
 };
 
 static int stm32_count_clk_get_freq(struct counter_device *counter,
@@ -404,8 +617,8 @@ static struct counter_count stm32_counts_enc_4ch = {
 	.num_functions = ARRAY_SIZE(stm32_count_functions),
 	.synapses = stm32_count_synapses_enc_4ch,
 	.num_synapses = ARRAY_SIZE(stm32_count_synapses_enc_4ch),
-	.ext = stm32_count_ext,
-	.num_ext = ARRAY_SIZE(stm32_count_ext)
+	.ext = stm32_count_4ch_ext,
+	.num_ext = ARRAY_SIZE(stm32_count_4ch_ext)
 };
 
 /* STM32 Timer with up to 4 capture channels (without encoder) */
@@ -469,8 +682,8 @@ static struct counter_count stm32_counts_4ch = {
 	.num_functions = 1, /* increase */
 	.synapses = stm32_count_synapses_no_enc,
 	.num_synapses = ARRAY_SIZE(stm32_count_synapses_no_enc),
-	.ext = stm32_count_ext,
-	.num_ext = ARRAY_SIZE(stm32_count_ext)
+	.ext = stm32_count_4ch_ext,
+	.num_ext = ARRAY_SIZE(stm32_count_4ch_ext)
 };
 
 static struct counter_count stm32_counts_2ch = {
@@ -480,8 +693,8 @@ static struct counter_count stm32_counts_2ch = {
 	.num_functions = 1, /* increase */
 	.synapses = stm32_count_synapses_no_enc,
 	.num_synapses = 3, /* clock, ch1 and ch2 */
-	.ext = stm32_count_ext,
-	.num_ext = ARRAY_SIZE(stm32_count_ext)
+	.ext = stm32_count_2ch_ext,
+	.num_ext = ARRAY_SIZE(stm32_count_2ch_ext)
 };
 
 static struct counter_count stm32_counts_1ch = {
@@ -491,8 +704,8 @@ static struct counter_count stm32_counts_1ch = {
 	.num_functions = 1, /* increase */
 	.synapses = stm32_count_synapses_no_enc,
 	.num_synapses = 2, /* clock, ch1 */
-	.ext = stm32_count_ext,
-	.num_ext = ARRAY_SIZE(stm32_count_ext)
+	.ext = stm32_count_1ch_ext,
+	.num_ext = ARRAY_SIZE(stm32_count_1ch_ext)
 };
 
 static struct counter_count stm32_counts = {
@@ -506,6 +719,42 @@ static struct counter_count stm32_counts = {
 	.num_ext = ARRAY_SIZE(stm32_count_ext)
 };
 
+static irqreturn_t stm32_timer_cnt_isr(int irq, void *ptr)
+{
+	struct counter_device *counter = ptr;
+	struct stm32_timer_cnt *const priv = counter_priv(counter);
+	u32 clr = GENMASK(31, 0); /* SR flags can be cleared by writing 0 (wr 1 has no effect) */
+	u32 sr, dier;
+	int i;
+
+	regmap_read(priv->regmap, TIM_SR, &sr);
+	regmap_read(priv->regmap, TIM_DIER, &dier);
+	/* Only take care of enabled IRQs */
+	dier &= (TIM_DIER_UIE | TIM_DIER_CC1IE | TIM_DIER_CC2IE | TIM_DIER_CC3IE | TIM_DIER_CC4IE);
+	sr &= dier;
+
+	if (sr & TIM_SR_UIF) {
+		atomic_inc(&priv->nb_ovf);
+		counter_push_event(counter, COUNTER_EVENT_OVERFLOW_UNDERFLOW, 0);
+		dev_dbg(counter->parent, "COUNTER_EVENT_OVERFLOW_UNDERFLOW\n");
+		/* SR flags can be cleared by writing 0, only clear relevant flag */
+		clr &= ~TIM_SR_UIF;
+	}
+
+	/* Check capture events */
+	for (i = 0 ; i < priv->nchannels; i++) {
+		if (sr & TIM_SR_CC_IF(i)) {
+			counter_push_event(counter, COUNTER_EVENT_CAPTURE, i);
+			clr &= ~TIM_SR_CC_IF(i);
+			dev_dbg(counter->parent, "COUNTER_EVENT_CAPTURE, %d\n", i);
+		}
+	}
+
+	regmap_write(priv->regmap, TIM_SR, clr);
+
+	return IRQ_HANDLED;
+};
+
 static void stm32_timer_cnt_detect_channels(struct platform_device *pdev,
 					    struct stm32_timer_cnt *priv)
 {
@@ -568,7 +817,7 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct stm32_timer_cnt *priv;
 	struct counter_device *counter;
-	int ret;
+	int i, ret;
 
 	if (IS_ERR_OR_NULL(ddata))
 		return -EINVAL;
@@ -582,6 +831,8 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
 	priv->regmap = ddata->regmap;
 	priv->clk = ddata->clk;
 	priv->max_arr = ddata->max_arr;
+	priv->nr_irqs = ddata->nr_irqs;
+	priv->irq = ddata->irq;
 
 	ret = stm32_timer_cnt_probe_encoder(pdev, priv);
 	if (ret)
@@ -630,6 +881,16 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, priv);
 
+	for (i = 0; i < priv->nr_irqs; i++) {
+		ret = devm_request_irq(&pdev->dev, priv->irq[i], stm32_timer_cnt_isr,
+				       0, dev_name(dev), counter);
+		if (ret) {
+			dev_err(dev, "Failed to request irq %d (err %d)\n",
+				priv->irq[i], ret);
+			return ret;
+		}
+	}
+
 	/* Reset input selector to its default input */
 	regmap_write(priv->regmap, TIM_TISEL, 0x0);
 
diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
index ca35af30745f..f0c6e6d2df66 100644
--- a/include/linux/mfd/stm32-timers.h
+++ b/include/linux/mfd/stm32-timers.h
@@ -41,6 +41,11 @@
 #define TIM_SMCR_SMS	(BIT(0) | BIT(1) | BIT(2)) /* Slave mode selection */
 #define TIM_SMCR_TS	(BIT(4) | BIT(5) | BIT(6)) /* Trigger selection */
 #define TIM_DIER_UIE	BIT(0)	/* Update interrupt	   */
+#define TIM_DIER_CC1IE	BIT(1)  /* CC1 Interrupt Enable    */
+#define TIM_DIER_CC2IE	BIT(2)  /* CC2 Interrupt Enable    */
+#define TIM_DIER_CC3IE	BIT(3)  /* CC3 Interrupt Enable    */
+#define TIM_DIER_CC4IE	BIT(4)  /* CC4 Interrupt Enable    */
+#define TIM_DIER_CC_IE(x)	BIT((x) + 1) /* CC1, CC2, CC3, CC4 interrupt enable */
 #define TIM_DIER_UDE	BIT(8)  /* Update DMA request Enable */
 #define TIM_DIER_CC1DE	BIT(9)  /* CC1 DMA request Enable  */
 #define TIM_DIER_CC2DE	BIT(10) /* CC2 DMA request Enable  */
@@ -49,6 +54,7 @@
 #define TIM_DIER_COMDE	BIT(13) /* COM DMA request Enable  */
 #define TIM_DIER_TDE	BIT(14) /* Trigger DMA request Enable */
 #define TIM_SR_UIF	BIT(0)	/* Update interrupt flag   */
+#define TIM_SR_CC_IF(x)	BIT((x) + 1) /* CC1, CC2, CC3, CC4 interrupt flag */
 #define TIM_EGR_UG	BIT(0)	/* Update Generation       */
 #define TIM_CCMR_PE	BIT(3)	/* Channel Preload Enable  */
 #define TIM_CCMR_M1	(BIT(6) | BIT(5))  /* Channel PWM Mode 1 */
@@ -60,16 +66,23 @@
 #define TIM_CCMR_CC1S_TI2	BIT(1)	/* IC1/IC3 selects TI2/TI4 */
 #define TIM_CCMR_CC2S_TI2	BIT(8)	/* IC2/IC4 selects TI2/TI4 */
 #define TIM_CCMR_CC2S_TI1	BIT(9)	/* IC2/IC4 selects TI1/TI3 */
+#define TIM_CCMR_CC3S		(BIT(0) | BIT(1)) /* Capture/compare 3 sel */
+#define TIM_CCMR_CC4S		(BIT(8) | BIT(9)) /* Capture/compare 4 sel */
+#define TIM_CCMR_CC3S_TI3	BIT(0)	/* IC3 selects TI3 */
+#define TIM_CCMR_CC4S_TI4	BIT(8)	/* IC4 selects TI4 */
 #define TIM_CCER_CC1E	BIT(0)	/* Capt/Comp 1  out Ena    */
 #define TIM_CCER_CC1P	BIT(1)	/* Capt/Comp 1  Polarity   */
 #define TIM_CCER_CC1NE	BIT(2)	/* Capt/Comp 1N out Ena    */
 #define TIM_CCER_CC1NP	BIT(3)	/* Capt/Comp 1N Polarity   */
 #define TIM_CCER_CC2E	BIT(4)	/* Capt/Comp 2  out Ena    */
 #define TIM_CCER_CC2P	BIT(5)	/* Capt/Comp 2  Polarity   */
+#define TIM_CCER_CC2NP	BIT(7)	/* Capt/Comp 2N Polarity   */
 #define TIM_CCER_CC3E	BIT(8)	/* Capt/Comp 3  out Ena    */
 #define TIM_CCER_CC3P	BIT(9)	/* Capt/Comp 3  Polarity   */
+#define TIM_CCER_CC3NP	BIT(11)	/* Capt/Comp 3N Polarity   */
 #define TIM_CCER_CC4E	BIT(12)	/* Capt/Comp 4  out Ena    */
 #define TIM_CCER_CC4P	BIT(13)	/* Capt/Comp 4  Polarity   */
+#define TIM_CCER_CC4NP	BIT(15)	/* Capt/Comp 4N Polarity   */
 #define TIM_CCER_CCXE	(BIT(0) | BIT(4) | BIT(8) | BIT(12))
 #define TIM_BDTR_BKE(x)	BIT(12 + (x) * 12) /* Break input enable */
 #define TIM_BDTR_BKP(x)	BIT(13 + (x) * 12) /* Break input polarity */
@@ -91,6 +104,8 @@
 #define TIM_BDTR_BKF_MASK	0xF
 #define TIM_BDTR_BKF_SHIFT(x)	(16 + (x) * 4)
 
+#define MAX_TIM_IC_CHANNELS	4 /* Max number of input capature channels */
+
 enum stm32_timers_dmas {
 	STM32_TIMERS_DMA_CH1,
 	STM32_TIMERS_DMA_CH2,
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] counter: stm32-timer-cnt: add support for events

[kernel test robot]

Hi Fabrice,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.5 next-20230829]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Fabrice-Gasnier/counter-chrdev-fix-getting-array-extensions/20230829-230111
base:   linus/master
patch link:    https://lore.kernel.org/r/20230829134029.2402868-9-fabrice.gasnier%40foss.st.com
patch subject: [PATCH 8/8] counter: stm32-timer-cnt: add support for events
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230830/202308300133.jtLeSGia-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230830/202308300133.jtLeSGia-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308300133.jtLeSGia-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/counter/stm32-timer-cnt.c: In function 'stm32_count_watch_validate':
>> drivers/counter/stm32-timer-cnt.c:524:20: warning: this statement may fall through [-Wimplicit-fallthrough=]
     524 |                 if (watch->channel >= priv->nchannels) {
         |                    ^
   drivers/counter/stm32-timer-cnt.c:528:9: note: here
     528 |         case COUNTER_EVENT_OVERFLOW_UNDERFLOW:
         |         ^~~~


vim +524 drivers/counter/stm32-timer-cnt.c

   516	
   517	static int stm32_count_watch_validate(struct counter_device *counter,
   518					      const struct counter_watch *watch)
   519	{
   520		struct stm32_timer_cnt *const priv = counter_priv(counter);
   521	
   522		switch (watch->event) {
   523		case COUNTER_EVENT_CAPTURE:
 > 524			if (watch->channel >= priv->nchannels) {
   525				dev_err(counter->parent, "Invalid channel %d\n", watch->channel);
   526				return -EINVAL;
   527			}
   528		case COUNTER_EVENT_OVERFLOW_UNDERFLOW:
   529			return 0;
   530		default:
   531			return -EINVAL;
   532		}
   533	}
   534	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] counter: stm32-timer-cnt: add support for events

[kernel test robot]

Hi Fabrice,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.5 next-20230830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Fabrice-Gasnier/counter-chrdev-fix-getting-array-extensions/20230829-230111
base:   linus/master
patch link:    https://lore.kernel.org/r/20230829134029.2402868-9-fabrice.gasnier%40foss.st.com
patch subject: [PATCH 8/8] counter: stm32-timer-cnt: add support for events
config: arm-randconfig-001-20230830 (https://download.01.org/0day-ci/archive/20230830/202308301541.ZqWpJdto-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230830/202308301541.ZqWpJdto-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308301541.ZqWpJdto-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/counter/stm32-timer-cnt.c:528:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
     528 |         case COUNTER_EVENT_OVERFLOW_UNDERFLOW:
         |         ^
   drivers/counter/stm32-timer-cnt.c:528:2: note: insert '__attribute__((fallthrough));' to silence this warning
     528 |         case COUNTER_EVENT_OVERFLOW_UNDERFLOW:
         |         ^
         |         __attribute__((fallthrough)); 
   drivers/counter/stm32-timer-cnt.c:528:2: note: insert 'break;' to avoid fall-through
     528 |         case COUNTER_EVENT_OVERFLOW_UNDERFLOW:
         |         ^
         |         break; 
   1 warning generated.


vim +528 drivers/counter/stm32-timer-cnt.c

   516	
   517	static int stm32_count_watch_validate(struct counter_device *counter,
   518					      const struct counter_watch *watch)
   519	{
   520		struct stm32_timer_cnt *const priv = counter_priv(counter);
   521	
   522		switch (watch->event) {
   523		case COUNTER_EVENT_CAPTURE:
   524			if (watch->channel >= priv->nchannels) {
   525				dev_err(counter->parent, "Invalid channel %d\n", watch->channel);
   526				return -EINVAL;
   527			}
 > 528		case COUNTER_EVENT_OVERFLOW_UNDERFLOW:
   529			return 0;
   530		default:
   531			return -EINVAL;
   532		}
   533	}
   534	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] counter: stm32-timer-cnt: add support for events

[Lee Jones]

On Tue, 29 Aug 2023, Fabrice Gasnier wrote:

> Add support for capture and overflow events. Also add the related
> validation and configuration. Captured counter value can be retrieved
> through CCRx register. Register and enable interrupts to push events.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> ---
>  drivers/counter/stm32-timer-cnt.c | 279 +++++++++++++++++++++++++++++-

>  include/linux/mfd/stm32-timers.h  |  15 ++

Acked-by: Lee Jones <lee@kernel.org>

>  2 files changed, 285 insertions(+), 9 deletions(-)

-- 
Lee Jones [李琼斯]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] counter: fix, improvements and stm32 timer events support

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 2063 bytes --]

On Tue, Aug 29, 2023 at 03:40:21PM +0200, Fabrice Gasnier wrote:
> This series combines some fix and improvements to the counter interface,
> found while stm32 timer counter driver developements.
> It also introduces a new tool that can be used for testing.
> 
> Then, it improves the stm32 timer counter driver by introducing new signals,
> e.g. counting frequency, and missing channels.
> It also adds support for interrupt based events using the chrdev interface.
> Two event types are added in this series: overflows and capture.
> 
> Up to now, stm32 timer counter driver focused mainly on quadrature
> encoder feature. With this series, all timer instances can be enabled
> for simple counting (with overflow and capture events).
> 
> Fabrice Gasnier (8):
>   counter: chrdev: fix getting array extensions
>   counter: chrdev: remove a typo in header file comment
>   tools/counter: add a flexible watch events tool
>   mfd: stm32-timers: add support for interrupts
>   counter: stm32-timer-cnt: rename quadrature signal
>   counter: stm32-timer-cnt: introduce clock signal
>   counter: stm32-timer-cnt: populate capture channels and check encoder
>   counter: stm32-timer-cnt: add support for events
> 
>  drivers/counter/counter-chrdev.c     |   4 +-
>  drivers/counter/stm32-timer-cnt.c    | 585 ++++++++++++++++++++++++++-
>  drivers/mfd/stm32-timers.c           |  46 +++
>  include/linux/mfd/stm32-timers.h     |  26 ++
>  include/uapi/linux/counter.h         |   2 +-
>  tools/counter/Build                  |   1 +
>  tools/counter/Makefile               |   8 +-
>  tools/counter/counter_watch_events.c | 348 ++++++++++++++++
>  8 files changed, 998 insertions(+), 22 deletions(-)
>  create mode 100644 tools/counter/counter_watch_events.c
> 
> -- 
> 2.25.1
> 

Hi Fabrice,

There are a number of precursor changes in this series that are somewhat
independent, so I'll be taking some patches separately to simplify
things and review this patchset a little at a time.

Thanks,

William Breathitt Gray

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel