* [PATCH v2 1/6] tools/counter: add a flexible watch events tool
2023-09-22 14:39 [PATCH v2 0/6] counter: fix, improvements and stm32 timer events support Fabrice Gasnier
@ 2023-09-22 14:39 ` Fabrice Gasnier
2023-10-04 1:37 ` William Breathitt Gray
2023-09-22 14:39 ` [PATCH v2 2/6] counter: stm32-timer-cnt: rename quadrature signal Fabrice Gasnier
` (5 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Fabrice Gasnier @ 2023-09-22 14:39 UTC (permalink / raw)
To: william.gray
Cc: lee, alexandre.torgue, fabrice.gasnier, linux-iio, linux-stm32,
linux-arm-kernel, linux-kernel
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 sub-option in "--watch" string.
Several watch events can be defined, each can have specific watch options,
by using "--watch <watch 1 options> --watch <watch 2 options>".
Watch options is a comma separated list.
It also comes with a simple default watch (to monitor overflow/underflow
events), used when no watch parameters are provided. It's equivalent to:
counter_watch_events -w comp_count,scope_count,evt_ovf_udf
The print_usage() routine proposes another example, from the command line,
which generates a 2 elements watch array, to monitor:
- overflow underflow 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>
---
Changes in v2:
Review comments from William:
- revisit watch options to be less error prone: add --watch with
sub-options to properly define each watch one by one, as a comma
separated list
- by the way, drop string/array parsing routines, replaced by getsubopt()
- Improve command-line interface descriptions, e.g. like "-h, --help"
- Makefile: adopt ARRAY_SIZE from tools/include/linux.kernel.h (via CFLAG)
- remove reference to counter_example
- clarify commit message, code comment: Index/overflow/underflow event
- check calloc return value
- Makefile: sort count_watch_events in alphabetic order
- Makefile: add a clean rule to delete .*.o.cmd files
---
tools/counter/Build | 1 +
tools/counter/Makefile | 12 +-
tools/counter/counter_watch_events.c | 368 +++++++++++++++++++++++++++
3 files changed, 379 insertions(+), 2 deletions(-)
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..d82d35a520f6 100644
--- a/tools/counter/Makefile
+++ b/tools/counter/Makefile
@@ -12,9 +12,10 @@ endif
# (this improves performance and avoids hard-to-debug behaviour);
MAKEFLAGS += -r
-override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
+override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include \
+ -I$(srctree)/tools/include
-ALL_TARGETS := counter_example
+ALL_TARGETS := counter_example counter_watch_events
ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
all: $(ALL_PROGRAMS)
@@ -37,12 +38,19 @@ $(COUNTER_EXAMPLE): prepare FORCE
$(OUTPUT)counter_example: $(COUNTER_EXAMPLE)
$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+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 $@
+
clean:
rm -f $(ALL_PROGRAMS)
rm -rf $(OUTPUT)include/linux/counter.h
rm -df $(OUTPUT)include/linux
rm -df $(OUTPUT)include
find $(or $(OUTPUT),.) -name '*.o' -delete -o -name '\.*.d' -delete
+ find $(or $(OUTPUT),.) -name '\.*.o.cmd' -delete
install: $(ALL_PROGRAMS)
install -d -m 755 $(DESTDIR)$(bindir); \
diff --git a/tools/counter/counter_watch_events.c b/tools/counter/counter_watch_events.c
new file mode 100644
index 000000000000..17eae0ab9cef
--- /dev/null
+++ b/tools/counter/counter_watch_events.c
@@ -0,0 +1,368 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Counter Watch Events - Test various counter watch events in a userspace application */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <getopt.h>
+#include <linux/counter.h>
+#include <linux/kernel.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <unistd.h>
+
+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: overflow or underflow */
+ .event = COUNTER_EVENT_OVERFLOW_UNDERFLOW,
+ /* Device event channel 0 */
+ .channel = 0,
+ },
+};
+
+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 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 const char * const counter_scope_name[] = {
+ "COUNTER_SCOPE_DEVICE",
+ "COUNTER_SCOPE_SIGNAL",
+ "COUNTER_SCOPE_COUNT",
+};
+
+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 void print_usage(void)
+{
+ fprintf(stderr, "Usage:\n\n"
+ "counter_watch_events [options] [-w <watchoptions>]\n"
+ "counter_watch_events [options] [-w <watch1 options>] [-w <watch2 options>]...\n"
+ "\n"
+ "When no --watch option has been provided, simple watch example is used:\n"
+ "counter_watch_events [options] -w comp_count,scope_count,evt_ovf_udf\n"
+ "\n"
+ "Test various watch events for given counter device.\n"
+ "\n"
+ "Options:\n"
+ " -d, --debug Prints debug information\n"
+ " -h, --help Prints usage\n"
+ " -n, --device-num <n> Use /dev/counter<n> [default: /dev/counter0]\n"
+ " -l, --loop <n> Loop for <n> events [default: 0 (forever)]\n"
+ " -w, --watch <watchoptions> comma-separated list of watch options\n"
+ "\n"
+ "Watch options:\n"
+ " scope_device (COUNTER_SCOPE_DEVICE) [default: scope_device]\n"
+ " scope_signal (COUNTER_SCOPE_SIGNAL)\n"
+ " scope_count (COUNTER_SCOPE_COUNT)\n"
+ "\n"
+ " comp_none (COUNTER_COMPONENT_NONE) [default: comp_none]\n"
+ " comp_signal (COUNTER_COMPONENT_SIGNAL)\n"
+ " comp_count (COUNTER_COMPONENT_COUNT)\n"
+ " comp_function (COUNTER_COMPONENT_FUNCTION)\n"
+ " comp_synapse_action (COUNTER_COMPONENT_SYNAPSE_ACTION)\n"
+ " comp_extension (COUNTER_COMPONENT_EXTENSION)\n"
+ "\n"
+ " evt_ovf (COUNTER_EVENT_OVERFLOW) [default: evt_ovf]\n"
+ " evt_udf (COUNTER_EVENT_UNDERFLOW)\n"
+ " evt_ovf_udf (COUNTER_EVENT_OVERFLOW_UNDERFLOW)\n"
+ " evt_threshold (COUNTER_EVENT_THRESHOLD)\n"
+ " evt_index (COUNTER_EVENT_INDEX)\n"
+ " evt_change_of_state (COUNTER_EVENT_CHANGE_OF_STATE)\n"
+ " evt_capture (COUNTER_EVENT_CAPTURE)\n"
+ "\n"
+ " chan=<n> channel <n> for this watch [default: 0]\n"
+ " id=<n> componend id <n> for this watch [default: 0]\n"
+ " parent=<n> componend parent <n> for this watch [default: 0]\n"
+ "\n"
+ "Example with two watched events:\n\n"
+ "counter_watch_events -d \\\n"
+ "\t-w comp_count,scope_count,evt_ovf_udf \\\n"
+ "\t-w comp_extension,scope_count,evt_capture,id=7,chan=3\n"
+ );
+}
+
+static const struct option longopts[] = {
+ { "debug", no_argument, 0, 'd' },
+ { "help", no_argument, 0, 'h' },
+ { "device-num", required_argument, 0, 'n' },
+ { "loop", required_argument, 0, 'l' },
+ { "watch", required_argument, 0, 'w' },
+ { },
+};
+
+/* counter watch subopts */
+enum {
+ WATCH_SCOPE_DEVICE,
+ WATCH_SCOPE_SIGNAL,
+ WATCH_SCOPE_COUNT,
+ WATCH_COMPONENT_NONE,
+ WATCH_COMPONENT_SIGNAL,
+ WATCH_COMPONENT_COUNT,
+ WATCH_COMPONENT_FUNCTION,
+ WATCH_COMPONENT_SYNAPSE_ACTION,
+ WATCH_COMPONENT_EXTENSION,
+ WATCH_EVENT_OVERFLOW,
+ WATCH_EVENT_UNDERFLOW,
+ WATCH_EVENT_OVERFLOW_UNDERFLOW,
+ WATCH_EVENT_THRESHOLD,
+ WATCH_EVENT_INDEX,
+ WATCH_EVENT_CHANGE_OF_STATE,
+ WATCH_EVENT_CAPTURE,
+ WATCH_CHANNEL,
+ WATCH_ID,
+ WATCH_PARENT,
+ WATCH_SUBOPTS_MAX,
+};
+
+static char * const counter_watch_subopts[WATCH_SUBOPTS_MAX + 1] = {
+ /* component.scope */
+ [WATCH_SCOPE_DEVICE] = "scope_device",
+ [WATCH_SCOPE_SIGNAL] = "scope_signal",
+ [WATCH_SCOPE_COUNT] = "scope_count",
+ /* component.type */
+ [WATCH_COMPONENT_NONE] = "comp_none",
+ [WATCH_COMPONENT_SIGNAL] = "comp_signal",
+ [WATCH_COMPONENT_COUNT] = "comp_count",
+ [WATCH_COMPONENT_FUNCTION] = "comp_function",
+ [WATCH_COMPONENT_SYNAPSE_ACTION] = "comp_synapse_action",
+ [WATCH_COMPONENT_EXTENSION] = "comp_extension",
+ /* event */
+ [WATCH_EVENT_OVERFLOW] = "evt_ovf",
+ [WATCH_EVENT_UNDERFLOW] = "evt_udf",
+ [WATCH_EVENT_OVERFLOW_UNDERFLOW] = "evt_ovf_udf",
+ [WATCH_EVENT_THRESHOLD] = "evt_threshold",
+ [WATCH_EVENT_INDEX] = "evt_index",
+ [WATCH_EVENT_CHANGE_OF_STATE] = "evt_change_of_state",
+ [WATCH_EVENT_CAPTURE] = "evt_capture",
+ /* channel, id, parent */
+ [WATCH_CHANNEL] = "chan",
+ [WATCH_ID] = "id",
+ [WATCH_PARENT] = "parent",
+ /* Empty entry ends the opts array */
+ NULL
+};
+
+int main(int argc, char **argv)
+{
+ int c, fd, i, ret, debug = 0, loop = 0, dev_num = 0, nwatch = 0;
+ struct counter_event event_data;
+ char *device_name = NULL, *subopts, *value;
+ struct counter_watch *watches;
+
+ /*
+ * 1st pass:
+ * - list watch events number to allocate the watch array.
+ * - parse normal options (other than watch options)
+ */
+ while ((c = getopt_long(argc, argv, "dhn:l:w:", longopts, NULL)) != -1) {
+ switch (c) {
+ case 'd':
+ debug = 1;
+ break;
+ case 'h':
+ print_usage();
+ return -1;
+ case 'n':
+ dev_num = strtoul(optarg, NULL, 10);
+ if (errno)
+ return -errno;
+ break;
+ case 'l':
+ loop = strtol(optarg, NULL, 10);
+ if (errno)
+ return -errno;
+ break;
+ case 'w':
+ nwatch++;
+ break;
+ default:
+ return -1;
+ };
+ };
+
+ if (nwatch) {
+ watches = calloc(nwatch, sizeof(*watches));
+ if (!watches) {
+ perror("Error allocating watches");
+ return 1;
+ }
+ } else {
+ /* default to simple watch example */
+ watches = simple_watch;
+ nwatch = ARRAY_SIZE(simple_watch);
+ }
+
+ /* 2nd pass: parse watch sub-options to fill in watch array */
+ optind = 1;
+ i = 0;
+ while ((c = getopt_long(argc, argv, "dhn:l:w:", longopts, NULL)) != -1) {
+ switch (c) {
+ case 'w':
+ subopts = optarg;
+ while (*subopts != '\0') {
+ ret = getsubopt(&subopts, counter_watch_subopts, &value);
+ switch (ret) {
+ case WATCH_SCOPE_DEVICE:
+ case WATCH_SCOPE_SIGNAL:
+ case WATCH_SCOPE_COUNT:
+ /* match with counter_scope */
+ watches[i].component.scope = ret;
+ break;
+ case WATCH_COMPONENT_NONE:
+ case WATCH_COMPONENT_SIGNAL:
+ case WATCH_COMPONENT_COUNT:
+ case WATCH_COMPONENT_FUNCTION:
+ case WATCH_COMPONENT_SYNAPSE_ACTION:
+ case WATCH_COMPONENT_EXTENSION:
+ /* match counter_component_type: subtract enum value */
+ ret -= WATCH_COMPONENT_NONE;
+ watches[i].component.type = ret;
+ break;
+ case WATCH_EVENT_OVERFLOW:
+ case WATCH_EVENT_UNDERFLOW:
+ case WATCH_EVENT_OVERFLOW_UNDERFLOW:
+ case WATCH_EVENT_THRESHOLD:
+ case WATCH_EVENT_INDEX:
+ case WATCH_EVENT_CHANGE_OF_STATE:
+ case WATCH_EVENT_CAPTURE:
+ /* match counter_event_type: subtract enum value */
+ ret -= WATCH_EVENT_OVERFLOW;
+ watches[i].event = ret;
+ break;
+ case WATCH_CHANNEL:
+ if (!value) {
+ fprintf(stderr, "Missing chan=<number>\n");
+ return -EINVAL;
+ }
+ watches[i].channel = strtoul(value, NULL, 10);
+ if (errno)
+ return -errno;
+ break;
+ case WATCH_ID:
+ if (!value) {
+ fprintf(stderr, "Missing id=<number>\n");
+ return -EINVAL;
+ }
+ watches[i].component.id = strtoul(value, NULL, 10);
+ if (errno)
+ return -errno;
+ break;
+ case WATCH_PARENT:
+ if (!value) {
+ fprintf(stderr, "Missing parent=<number>\n");
+ return -EINVAL;
+ }
+ watches[i].component.parent = strtoul(value, NULL, 10);
+ if (errno)
+ return -errno;
+ break;
+ default:
+ fprintf(stderr, "Unknown suboption '%s'\n", value);
+ return -EINVAL;
+ }
+ }
+ i++;
+ 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/6] tools/counter: add a flexible watch events tool
2023-09-22 14:39 ` [PATCH v2 1/6] tools/counter: add a flexible watch events tool Fabrice Gasnier
@ 2023-10-04 1:37 ` William Breathitt Gray
0 siblings, 0 replies; 14+ messages in thread
From: William Breathitt Gray @ 2023-10-04 1:37 UTC (permalink / raw)
To: Fabrice Gasnier
Cc: lee, alexandre.torgue, linux-iio, linux-stm32, linux-arm-kernel,
linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 3387 bytes --]
On Fri, Sep 22, 2023 at 04:39:15PM +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 sub-option in "--watch" string.
> Several watch events can be defined, each can have specific watch options,
> by using "--watch <watch 1 options> --watch <watch 2 options>".
> Watch options is a comma separated list.
>
> It also comes with a simple default watch (to monitor overflow/underflow
> events), used when no watch parameters are provided. It's equivalent to:
> counter_watch_events -w comp_count,scope_count,evt_ovf_udf
>
> The print_usage() routine proposes another example, from the command line,
> which generates a 2 elements watch array, to monitor:
> - overflow underflow 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 tool is independent from the rest of the patches in this patchset,
so I suggest separating and submitting the next revision of this patch
independently.
> ---
> Changes in v2:
> Review comments from William:
> - revisit watch options to be less error prone: add --watch with
> sub-options to properly define each watch one by one, as a comma
> separated list
> - by the way, drop string/array parsing routines, replaced by getsubopt()
> - Improve command-line interface descriptions, e.g. like "-h, --help"
> - Makefile: adopt ARRAY_SIZE from tools/include/linux.kernel.h (via CFLAG)
> - remove reference to counter_example
> - clarify commit message, code comment: Index/overflow/underflow event
> - check calloc return value
> - Makefile: sort count_watch_events in alphabetic order
> - Makefile: add a clean rule to delete .*.o.cmd files
It looks like you implemented all the changes I requested in the first
review so I don't have much to comment inline for this revision.
It looks like the memory allocated (via calloc()) for the watches array
is never freed, so fix that in the next revision.
Also, add a MAINTAINERS entry with at least you as the point of contact
or someone else (an ST engineer?) who is willing to respond to any
bug reports the mailing list could get for this utility.
Regarding watch options, I looked up how a few other utilities handle
similar situations. Some utilities like the nftables nft command line
utility takes in a configuration file where you can specify the verbose
rule-sets, while others such as bfptrace allows entire programs to be
specified via one-liner constructs passed in a single option ("-e").
Although powerful, I found those approaches to be far too complex for
our simple test utility here. Instead, uilities with a simpler interface
take an approach similar to yours by providing several well-defined
sub-options; for example, to filter tcpdump packets users can provide
the particular sub-options they desire ("-i eth0 port 22").
The watch option solution here with sub-options is simple and clear, so
for now let's go with it as you have it. If the need arises in the
future for a more complex option interface, we'll tackle it as it comes.
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/6] counter: stm32-timer-cnt: rename quadrature signal
2023-09-22 14:39 [PATCH v2 0/6] counter: fix, improvements and stm32 timer events support Fabrice Gasnier
2023-09-22 14:39 ` [PATCH v2 1/6] tools/counter: add a flexible watch events tool Fabrice Gasnier
@ 2023-09-22 14:39 ` Fabrice Gasnier
2023-09-22 14:39 ` [PATCH v2 3/6] counter: stm32-timer-cnt: rename counter Fabrice Gasnier
` (4 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Fabrice Gasnier @ 2023-09-22 14:39 UTC (permalink / raw)
To: william.gray
Cc: lee, alexandre.torgue, fabrice.gasnier, linux-iio, linux-stm32,
linux-arm-kernel, linux-kernel
Drop the Quadrature convention in the signal name. On stm32-timer:
- Quadrature A signal corresponds to timer input ch1, hence "Channel 1"
- Quadrature B signal corresponds to timer input ch2, hence "Channel 2".
So name these signals after their channel. I suspect it referred to the
(unique) quadrature counter support earlier, but the physical input
really is CH1/CH2. This will be easier to support other counter modes.
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v2:
- Drop the "Quadrature" convention from the signal name, as suggested by
William
---
drivers/counter/stm32-timer-cnt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index 6206d2dc3d47..36d812ddf162 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -279,11 +279,11 @@ static const struct counter_ops stm32_timer_cnt_ops = {
static struct counter_signal stm32_signals[] = {
{
.id = 0,
- .name = "Channel 1 Quadrature A"
+ .name = "Channel 1"
},
{
.id = 1,
- .name = "Channel 1 Quadrature B"
+ .name = "Channel 2"
}
};
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/6] counter: stm32-timer-cnt: rename counter
2023-09-22 14:39 [PATCH v2 0/6] counter: fix, improvements and stm32 timer events support Fabrice Gasnier
2023-09-22 14:39 ` [PATCH v2 1/6] tools/counter: add a flexible watch events tool Fabrice Gasnier
2023-09-22 14:39 ` [PATCH v2 2/6] counter: stm32-timer-cnt: rename quadrature signal Fabrice Gasnier
@ 2023-09-22 14:39 ` Fabrice Gasnier
2023-09-22 14:39 ` [PATCH v2 4/6] counter: stm32-timer-cnt: introduce clock signal Fabrice Gasnier
` (3 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Fabrice Gasnier @ 2023-09-22 14:39 UTC (permalink / raw)
To: william.gray
Cc: lee, alexandre.torgue, fabrice.gasnier, linux-iio, linux-stm32,
linux-arm-kernel, linux-kernel
The STM32 timer may count on various sources or channels. The counter
isn't specifically counting on channe1 1. So rename it to avoid a
confusion.
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 36d812ddf162..668e9d1061d3 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -302,7 +302,7 @@ static struct counter_synapse stm32_count_synapses[] = {
static struct counter_count stm32_counts = {
.id = 0,
- .name = "Channel 1 Count",
+ .name = "STM32 Timer Counter",
.functions_list = stm32_count_functions,
.num_functions = ARRAY_SIZE(stm32_count_functions),
.synapses = stm32_count_synapses,
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 4/6] counter: stm32-timer-cnt: introduce clock signal
2023-09-22 14:39 [PATCH v2 0/6] counter: fix, improvements and stm32 timer events support Fabrice Gasnier
` (2 preceding siblings ...)
2023-09-22 14:39 ` [PATCH v2 3/6] counter: stm32-timer-cnt: rename counter Fabrice Gasnier
@ 2023-09-22 14:39 ` Fabrice Gasnier
2023-10-13 21:22 ` William Breathitt Gray
2023-09-22 14:39 ` [PATCH v2 5/6] counter: stm32-timer-cnt: populate capture channels and check encoder Fabrice Gasnier
` (2 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Fabrice Gasnier @ 2023-09-22 14:39 UTC (permalink / raw)
To: william.gray
Cc: lee, alexandre.torgue, fabrice.gasnier, linux-iio, linux-stm32,
linux-arm-kernel, linux-kernel
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 668e9d1061d3..11c66876b213 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"
},
{
- .id = 1,
+ .id = STM32_CH2_SIG,
.name = "Channel 2"
}
};
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/6] counter: stm32-timer-cnt: introduce clock signal
2023-09-22 14:39 ` [PATCH v2 4/6] counter: stm32-timer-cnt: introduce clock signal Fabrice Gasnier
@ 2023-10-13 21:22 ` William Breathitt Gray
0 siblings, 0 replies; 14+ messages in thread
From: William Breathitt Gray @ 2023-10-13 21:22 UTC (permalink / raw)
To: Fabrice Gasnier
Cc: lee, alexandre.torgue, linux-iio, linux-stm32, linux-arm-kernel,
linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 7171 bytes --]
On Fri, Sep 22, 2023 at 04:39:18PM +0200, Fabrice Gasnier wrote:
> 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.
Hi Fabrice,
Split the addition of "frequency" and "prescaler" extensions each to
their own respective patches so we can keep the clock signal
introduction code separate (useful in case we need to git bisect an
issue in the future).
>
> Whit this patch, signal action reports consistent state when "increase"
Looks like a typo there for the first word.
> 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
Since you're fixing this description anyway, indent the shell example by
four spaces to make it stand-out and look nice.
>
> 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 668e9d1061d3..11c66876b213 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,
This will break userspace programs that expect signal0 to represent the
"Channel 1" Signal. Instead, add the clock Signal to the end of the
stm32_signals array so that the existing Signals are not reordered.
Although the clock signal may be represented by an id of 0 on the
device, the Counter API Signal id is a more abstract concept so it does
not necessarily need to match the device's numbering scheme.
Side note: you can keep the "id" member value the same if you want. The
Counter subsystem uses the array position to index the Signals; the "id"
value is ignored by the subsystem in that regard, and is rather provided
for the driver's internal use so it can differentiate between the
Signals.
> + .name = "Clock Signal",
> + .ext = stm32_count_clock_ext,
> + .num_ext = ARRAY_SIZE(stm32_count_clock_ext),
> + },
> + {
> + .id = STM32_CH1_SIG,
> .name = "Channel 1"
> },
> {
> - .id = 1,
> + .id = STM32_CH2_SIG,
> .name = "Channel 2"
> }
> };
>
> 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]
> + },
Same reordering issue here as the previous comment.
William Breathitt Gray
> {
> .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
>
[-- 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 5/6] counter: stm32-timer-cnt: populate capture channels and check encoder
2023-09-22 14:39 [PATCH v2 0/6] counter: fix, improvements and stm32 timer events support Fabrice Gasnier
` (3 preceding siblings ...)
2023-09-22 14:39 ` [PATCH v2 4/6] counter: stm32-timer-cnt: introduce clock signal Fabrice Gasnier
@ 2023-09-22 14:39 ` Fabrice Gasnier
2023-10-13 22:48 ` William Breathitt Gray
2023-09-22 14:39 ` [PATCH v2 6/6] counter: stm32-timer-cnt: add support for events Fabrice Gasnier
2023-10-13 22:57 ` [PATCH v2 0/6] counter: fix, improvements and stm32 timer events support William Breathitt Gray
6 siblings, 1 reply; 14+ messages in thread
From: Fabrice Gasnier @ 2023-09-22 14:39 UTC (permalink / raw)
To: william.gray
Cc: lee, alexandre.torgue, fabrice.gasnier, linux-iio, linux-stm32,
linux-arm-kernel, linux-kernel
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>
---
Changes in v2:
- rework to simplify since channel names has been updated.
---
drivers/counter/stm32-timer-cnt.c | 204 +++++++++++++++++++++++++++++-
1 file changed, 198 insertions(+), 6 deletions(-)
diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index 11c66876b213..acde993891f5 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,
@@ -347,10 +357,19 @@ static struct counter_signal stm32_signals[] = {
{
.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[] = {
+/* STM32 Timer with 4 capture channels and quadrature encoder */
+static struct counter_synapse stm32_count_synapses_4ch_enc[] = {
{
.actions_list = stm32_clock_synapse_actions,
.num_actions = ARRAY_SIZE(stm32_clock_synapse_actions),
@@ -365,20 +384,159 @@ static struct counter_synapse stm32_count_synapses[] = {
.actions_list = stm32_synapse_actions,
.num_actions = ARRAY_SIZE(stm32_synapse_actions),
.signal = &stm32_signals[STM32_CH2_SIG]
- }
+ },
+ {
+ .actions_list = stm32_synapse_ch_actions,
+ .num_actions = ARRAY_SIZE(stm32_synapse_ch_actions),
+ .signal = &stm32_signals[STM32_CH3_SIG]
+ },
+ {
+ .actions_list = stm32_synapse_ch_actions,
+ .num_actions = ARRAY_SIZE(stm32_synapse_ch_actions),
+ .signal = &stm32_signals[STM32_CH4_SIG]
+ },
};
-static struct counter_count stm32_counts = {
+static struct counter_count stm32_counts_enc_4ch = {
.id = 0,
.name = "STM32 Timer Counter",
.functions_list = stm32_count_functions,
.num_functions = ARRAY_SIZE(stm32_count_functions),
+ .synapses = stm32_count_synapses_4ch_enc,
+ .num_synapses = ARRAY_SIZE(stm32_count_synapses_4ch_enc),
+ .ext = stm32_count_ext,
+ .num_ext = ARRAY_SIZE(stm32_count_ext)
+};
+
+/* STM32 Timer with up to 4 capture channels */
+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_ch_actions,
+ .num_actions = ARRAY_SIZE(stm32_synapse_ch_actions),
+ .signal = &stm32_signals[STM32_CH1_SIG]
+ },
+ {
+ .actions_list = stm32_synapse_ch_actions,
+ .num_actions = ARRAY_SIZE(stm32_synapse_ch_actions),
+ .signal = &stm32_signals[STM32_CH2_SIG]
+ },
+ {
+ .actions_list = stm32_synapse_ch_actions,
+ .num_actions = ARRAY_SIZE(stm32_synapse_ch_actions),
+ .signal = &stm32_signals[STM32_CH3_SIG]
+ },
+ {
+ .actions_list = stm32_synapse_ch_actions,
+ .num_actions = ARRAY_SIZE(stm32_synapse_ch_actions),
+ .signal = &stm32_signals[STM32_CH4_SIG]
+ },
+};
+
+static struct counter_count stm32_counts_4ch = {
+ .id = 0,
+ .name = "STM32 Timer Counter",
+ .functions_list = stm32_count_functions,
+ .num_functions = 1, /* increase */
.synapses = stm32_count_synapses,
.num_synapses = ARRAY_SIZE(stm32_count_synapses),
.ext = stm32_count_ext,
.num_ext = ARRAY_SIZE(stm32_count_ext)
};
+static struct counter_count stm32_counts_2ch = {
+ .id = 0,
+ .name = "STM32 Timer Counter",
+ .functions_list = stm32_count_functions,
+ .num_functions = 1, /* increase */
+ .synapses = stm32_count_synapses,
+ .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 = "STM32 Timer Counter",
+ .functions_list = stm32_count_functions,
+ .num_functions = 1, /* increase */
+ .synapses = stm32_count_synapses,
+ .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 = "STM32 Timer Counter",
+ .functions_list = stm32_count_functions,
+ .num_functions = 1, /* increase */
+ .synapses = stm32_count_synapses,
+ .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 +558,47 @@ 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;
+ else
+ counter->counts = &stm32_counts_4ch;
+ counter->signals = stm32_signals;
+ counter->num_signals = ARRAY_SIZE(stm32_signals);
+ break;
+ case 2:
+ counter->counts = &stm32_counts_2ch;
+ counter->signals = stm32_signals;
+ counter->num_signals = 3; /* clock, ch1 and ch2 */
+ break;
+ case 1:
+ counter->counts = &stm32_counts_1ch;
+ counter->signals = stm32_signals;
+ counter->num_signals = 2; /* clock, ch1 */
+ break;
+ default:
+ counter->counts = &stm32_counts;
+ counter->signals = stm32_signals;
+ 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 5/6] counter: stm32-timer-cnt: populate capture channels and check encoder
2023-09-22 14:39 ` [PATCH v2 5/6] counter: stm32-timer-cnt: populate capture channels and check encoder Fabrice Gasnier
@ 2023-10-13 22:48 ` William Breathitt Gray
2023-12-15 17:13 ` Fabrice Gasnier
0 siblings, 1 reply; 14+ messages in thread
From: William Breathitt Gray @ 2023-10-13 22:48 UTC (permalink / raw)
To: Fabrice Gasnier
Cc: lee, alexandre.torgue, linux-iio, linux-stm32, linux-arm-kernel,
linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 3754 bytes --]
On Fri, Sep 22, 2023 at 04:39:19PM +0200, Fabrice Gasnier wrote:
> 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.
Hi Fabrice,
Let's split the encoder capability probing code, detect number of
channels code, and channel introduction code to their own patches in
order to simplify things.
> 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>
I think this patch is more complicated than it needs to be.
> @@ -400,13 +558,47 @@ 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);
Keep this the same.
> +
> + /*
> + * 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;
> + else
> + counter->counts = &stm32_counts_4ch;
> + counter->signals = stm32_signals;
> + counter->num_signals = ARRAY_SIZE(stm32_signals);
> + break;
> + case 2:
> + counter->counts = &stm32_counts_2ch;
> + counter->signals = stm32_signals;
> + counter->num_signals = 3; /* clock, ch1 and ch2 */
> + break;
> + case 1:
> + counter->counts = &stm32_counts_1ch;
> + counter->signals = stm32_signals;
> + counter->num_signals = 2; /* clock, ch1 */
> + break;
> + default:
> + counter->counts = &stm32_counts;
> + counter->signals = stm32_signals;
> + counter->num_signals = 1; /* clock */
> + break;
> + }
Rather than adjusting the number of counts and signals, keep the
configuration static and use a single stm32_counts array. The reason is
that in the Counter subsystem paradigm Signals do not necessary
correlate to specific hardware signals but are rather an abstract
representation of the device behavior at a high level. In other words, a
Synapse with an action mode set to COUNTER_SYNAPSE_ACTION_NONE can be
viewed as representing a Signal that does not affect the Count (i.e. in
this case equivalent to an unconnected line).
What you'll need to do instead is check priv->nchannels during
stm32_action_read and stm32_count_function_read calls in order to return
the correct synapse action and count function for the particular
channels configuration you have. In stm32_count_function_write you would
return an -EINVAL (maybe -EOPNOTSUPP would be better?) when the channels
configuration does not support a particular count function.
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 5/6] counter: stm32-timer-cnt: populate capture channels and check encoder
2023-10-13 22:48 ` William Breathitt Gray
@ 2023-12-15 17:13 ` Fabrice Gasnier
2023-12-18 17:58 ` William Breathitt Gray
0 siblings, 1 reply; 14+ messages in thread
From: Fabrice Gasnier @ 2023-12-15 17:13 UTC (permalink / raw)
To: William Breathitt Gray
Cc: lee, alexandre.torgue, linux-iio, linux-stm32, linux-arm-kernel,
linux-kernel
On 10/14/23 00:48, William Breathitt Gray wrote:
> On Fri, Sep 22, 2023 at 04:39:19PM +0200, Fabrice Gasnier wrote:
>> 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.
>
> Hi Fabrice,
>
> Let's split the encoder capability probing code, detect number of
> channels code, and channel introduction code to their own patches in
> order to simplify things.
>
>> 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>
>
> I think this patch is more complicated than it needs to be.
>
>> @@ -400,13 +558,47 @@ 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);
>
> Keep this the same.
>
>> +
>> + /*
>> + * 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;
>> + else
>> + counter->counts = &stm32_counts_4ch;
>> + counter->signals = stm32_signals;
>> + counter->num_signals = ARRAY_SIZE(stm32_signals);
>> + break;
>> + case 2:
>> + counter->counts = &stm32_counts_2ch;
>> + counter->signals = stm32_signals;
>> + counter->num_signals = 3; /* clock, ch1 and ch2 */
>> + break;
>> + case 1:
>> + counter->counts = &stm32_counts_1ch;
>> + counter->signals = stm32_signals;
>> + counter->num_signals = 2; /* clock, ch1 */
>> + break;
>> + default:
>> + counter->counts = &stm32_counts;
>> + counter->signals = stm32_signals;
>> + counter->num_signals = 1; /* clock */
>> + break;
>> + }
>
> Rather than adjusting the number of counts and signals, keep the
> configuration static and use a single stm32_counts array. The reason is
> that in the Counter subsystem paradigm Signals do not necessary
> correlate to specific hardware signals but are rather an abstract
> representation of the device behavior at a high level. In other words, a
> Synapse with an action mode set to COUNTER_SYNAPSE_ACTION_NONE can be
> viewed as representing a Signal that does not affect the Count (i.e. in
> this case equivalent to an unconnected line).
>
> What you'll need to do instead is check priv->nchannels during
> stm32_action_read and stm32_count_function_read calls in order to return
> the correct synapse action and count function for the particular
> channels configuration you have. In stm32_count_function_write you would
> return an -EINVAL (maybe -EOPNOTSUPP would be better?) when the channels
> configuration does not support a particular count function.
Hi William,
Sorry for the long delay to address your comments here. Many thanks for
these guidelines.
I'm preparing a v3, to address these. I'll probably send it soon, so we
can start to review also the capture part of it. Still there are few
things here I'm wondering about (as an anticipation task).
Basically, removing all the diversity here means the most featured timer
model will be represented here (with all possible signals).
When I wrote the various configuration arrays, I'd have been tempted to
allocate them dynamically upon probing to avoid having all these
variants described as const arrays. This may have eased other signals
additions later. But that's not the direction. So, this simplifies the
description here, clearly, to describe the full-featured timer/counter,
and handle the ("unconnected") variants by returning errors.
I still have in mind the replacement of the last IIO_COUNT device [1]
(not addressed in this series), e.g. in
drivers/iio/trigger/stm32-timer-trigger.c. Here, there are
"valids_table" that are used to cascade two timers (one timer output
being the input to another timer). With this table currently, an IIO
user knows the name of the signal it selects (then the driver looks up
the 'valids' table to set SMCR / TS bits, e.g. trigger select). Each
individual timer has a different input mapping, so called peripheral
interconnect in STM32.
What bothers me here is: with an abstracted full-featured timer, without
any diversity on the signal names, I fear the userland has no clue on
which signal would be used. Abstracting the timer this way would mean
the user only knows it selects "Internal Trigger 0" for example, without
knowing which internal signal in the SoC it has selected.
Even if this is out of scope for this series, would you have some clue
so I can anticipate it ? Or if we stick with abstract names? In which
case the userland may need to be aware of the signals mapping (where
currently in IIO_COUNT driver, the signal names are privided). I'd be
glad to get some direction here.
Please advise,
Best Regards,
Fabrice
[1] https://lore.kernel.org/linux-arm-kernel/Y0vzlOmFrVCQVXMq@fedora/
>
> William Breathitt Gray
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 5/6] counter: stm32-timer-cnt: populate capture channels and check encoder
2023-12-15 17:13 ` Fabrice Gasnier
@ 2023-12-18 17:58 ` William Breathitt Gray
0 siblings, 0 replies; 14+ messages in thread
From: William Breathitt Gray @ 2023-12-18 17:58 UTC (permalink / raw)
To: Fabrice Gasnier
Cc: lee, alexandre.torgue, linux-iio, linux-stm32, linux-arm-kernel,
linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 5477 bytes --]
On Fri, Dec 15, 2023 at 06:13:51PM +0100, Fabrice Gasnier wrote:
> >> + /*
> >> + * 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;
> >> + else
> >> + counter->counts = &stm32_counts_4ch;
> >> + counter->signals = stm32_signals;
> >> + counter->num_signals = ARRAY_SIZE(stm32_signals);
> >> + break;
> >> + case 2:
> >> + counter->counts = &stm32_counts_2ch;
> >> + counter->signals = stm32_signals;
> >> + counter->num_signals = 3; /* clock, ch1 and ch2 */
> >> + break;
> >> + case 1:
> >> + counter->counts = &stm32_counts_1ch;
> >> + counter->signals = stm32_signals;
> >> + counter->num_signals = 2; /* clock, ch1 */
> >> + break;
> >> + default:
> >> + counter->counts = &stm32_counts;
> >> + counter->signals = stm32_signals;
> >> + counter->num_signals = 1; /* clock */
> >> + break;
> >> + }
> >
> > Rather than adjusting the number of counts and signals, keep the
> > configuration static and use a single stm32_counts array. The reason is
> > that in the Counter subsystem paradigm Signals do not necessary
> > correlate to specific hardware signals but are rather an abstract
> > representation of the device behavior at a high level. In other words, a
> > Synapse with an action mode set to COUNTER_SYNAPSE_ACTION_NONE can be
> > viewed as representing a Signal that does not affect the Count (i.e. in
> > this case equivalent to an unconnected line).
> >
> > What you'll need to do instead is check priv->nchannels during
> > stm32_action_read and stm32_count_function_read calls in order to return
> > the correct synapse action and count function for the particular
> > channels configuration you have. In stm32_count_function_write you would
> > return an -EINVAL (maybe -EOPNOTSUPP would be better?) when the channels
> > configuration does not support a particular count function.
>
> Hi William,
>
> Sorry for the long delay to address your comments here. Many thanks for
> these guidelines.
>
> I'm preparing a v3, to address these. I'll probably send it soon, so we
> can start to review also the capture part of it. Still there are few
> things here I'm wondering about (as an anticipation task).
>
> Basically, removing all the diversity here means the most featured timer
> model will be represented here (with all possible signals).
> When I wrote the various configuration arrays, I'd have been tempted to
> allocate them dynamically upon probing to avoid having all these
> variants described as const arrays. This may have eased other signals
> additions later. But that's not the direction. So, this simplifies the
> description here, clearly, to describe the full-featured timer/counter,
> and handle the ("unconnected") variants by returning errors.
>
> I still have in mind the replacement of the last IIO_COUNT device [1]
> (not addressed in this series), e.g. in
> drivers/iio/trigger/stm32-timer-trigger.c. Here, there are
> "valids_table" that are used to cascade two timers (one timer output
> being the input to another timer). With this table currently, an IIO
> user knows the name of the signal it selects (then the driver looks up
> the 'valids' table to set SMCR / TS bits, e.g. trigger select). Each
> individual timer has a different input mapping, so called peripheral
> interconnect in STM32.
> What bothers me here is: with an abstracted full-featured timer, without
> any diversity on the signal names, I fear the userland has no clue on
> which signal would be used. Abstracting the timer this way would mean
> the user only knows it selects "Internal Trigger 0" for example, without
> knowing which internal signal in the SoC it has selected.
>
> Even if this is out of scope for this series, would you have some clue
> so I can anticipate it ? Or if we stick with abstract names? In which
> case the userland may need to be aware of the signals mapping (where
> currently in IIO_COUNT driver, the signal names are privided). I'd be
> glad to get some direction here.
>
> Please advise,
> Best Regards,
> Fabrice
>
> [1] https://lore.kernel.org/linux-arm-kernel/Y0vzlOmFrVCQVXMq@fedora/
Hi Fabrice,
I took a look the stm32-timer-trigger.c file, but I'm having trouble
understanding it well (probably the cascading is confusing me). If I
understand the logic correctly, the user selects the trigger they want
by the attribute's name which is compared via strncmp() against the
'valids' table. It's possible we could dynamically configure the signal
names, but keep the signal id static so the driver code won't need some
many variations; alternatively, we could introduce a new signal
extension attribute that could display the trigger name.
Would you walk me through an example of using the stm32-timer-trigger
IIO sysfs interface? I think that would help me understand how users
currently interact with the triggers for that driver, and then maybe I
can figure out an appropriate equivalent in the Counter paradigm for
the migration. Right now, the timer cascade is confusing me so if I can
grok it well, the right solution for this may come to me more easily.
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 6/6] counter: stm32-timer-cnt: add support for events
2023-09-22 14:39 [PATCH v2 0/6] counter: fix, improvements and stm32 timer events support Fabrice Gasnier
` (4 preceding siblings ...)
2023-09-22 14:39 ` [PATCH v2 5/6] counter: stm32-timer-cnt: populate capture channels and check encoder Fabrice Gasnier
@ 2023-09-22 14:39 ` Fabrice Gasnier
2023-10-13 23:03 ` William Breathitt Gray
2023-10-13 22:57 ` [PATCH v2 0/6] counter: fix, improvements and stm32 timer events support William Breathitt Gray
6 siblings, 1 reply; 14+ messages in thread
From: Fabrice Gasnier @ 2023-09-22 14:39 UTC (permalink / raw)
To: william.gray
Cc: lee, alexandre.torgue, fabrice.gasnier, linux-iio, linux-stm32,
linux-arm-kernel, linux-kernel
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.
Acked-by: Lee Jones <lee@kernel.org>
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v2:
- fix warnings (kernel test robot)
- fix a typo
- add collected ack from Lee
---
drivers/counter/stm32-timer-cnt.c | 280 +++++++++++++++++++++++++++++-
include/linux/mfd/stm32-timers.h | 15 ++
2 files changed, 286 insertions(+), 9 deletions(-)
diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index acde993891f5..baa6a2c0ccbf 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,131 @@ 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;
+ }
+ return 0;
+ 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 +618,8 @@ static struct counter_count stm32_counts_enc_4ch = {
.num_functions = ARRAY_SIZE(stm32_count_functions),
.synapses = stm32_count_synapses_4ch_enc,
.num_synapses = ARRAY_SIZE(stm32_count_synapses_4ch_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)
};
/* STM32 Timer with up to 4 capture channels */
@@ -444,8 +658,8 @@ static struct counter_count stm32_counts_4ch = {
.num_functions = 1, /* increase */
.synapses = stm32_count_synapses,
.num_synapses = ARRAY_SIZE(stm32_count_synapses),
- .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 = {
@@ -455,8 +669,8 @@ static struct counter_count stm32_counts_2ch = {
.num_functions = 1, /* increase */
.synapses = stm32_count_synapses,
.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 = {
@@ -466,8 +680,8 @@ static struct counter_count stm32_counts_1ch = {
.num_functions = 1, /* increase */
.synapses = stm32_count_synapses,
.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 = {
@@ -481,6 +695,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)
{
@@ -543,7 +793,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;
@@ -557,6 +807,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)
@@ -602,6 +854,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..5291be74fba1 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 capture 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 6/6] counter: stm32-timer-cnt: add support for events
2023-09-22 14:39 ` [PATCH v2 6/6] counter: stm32-timer-cnt: add support for events Fabrice Gasnier
@ 2023-10-13 23:03 ` William Breathitt Gray
0 siblings, 0 replies; 14+ messages in thread
From: William Breathitt Gray @ 2023-10-13 23:03 UTC (permalink / raw)
To: Fabrice Gasnier
Cc: lee, alexandre.torgue, linux-iio, linux-stm32, linux-arm-kernel,
linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 660 bytes --]
On Fri, Sep 22, 2023 at 04:39:20PM +0200, 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.
>
> Acked-by: Lee Jones <lee@kernel.org>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Hi Fabrice,
Please split the capture and overflow events code to their own patches.
I think there will be some changes to this patch anyway due to the
changes you'll make in the precursor patches, so I'll hold off until v3
to review.
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 0/6] counter: fix, improvements and stm32 timer events support
2023-09-22 14:39 [PATCH v2 0/6] counter: fix, improvements and stm32 timer events support Fabrice Gasnier
` (5 preceding siblings ...)
2023-09-22 14:39 ` [PATCH v2 6/6] counter: stm32-timer-cnt: add support for events Fabrice Gasnier
@ 2023-10-13 22:57 ` William Breathitt Gray
6 siblings, 0 replies; 14+ messages in thread
From: William Breathitt Gray @ 2023-10-13 22:57 UTC (permalink / raw)
To: Fabrice Gasnier
Cc: lee, alexandre.torgue, linux-iio, linux-stm32, linux-arm-kernel,
linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 2406 bytes --]
On Fri, Sep 22, 2023 at 04:39:14PM +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).
Hi Fabrice,
Would you give a brief summary of the possible configurations we can
have for these devices. For example, the existing driver supports a
quadrature counting mode where Channel 1 and Channel 2 serve as A and B
inputs; I also see that another mode is a simple tally counter with the
internal clock serving as the input. How do Channel 3 and Channel 4 fit
in here?
If you list out all the possible arrangements, I think it'll help me
understand how the signals match up with each particular mode.
Thanks,
William Breathitt Gray
>
> Changes in v2:
> - counter fix and improvement patch applied, hence dropped in v2 series
> - mfd patch applied, hence dropped in v2 series
> - revisit watch events tool (mainly command line interface)
> - add one patch to rename STM32 Timer counter
> - various review comments from v1
>
> Fabrice Gasnier (6):
> tools/counter: add a flexible watch events tool
> counter: stm32-timer-cnt: rename quadrature signal
> counter: stm32-timer-cnt: rename counter
> 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/stm32-timer-cnt.c | 558 ++++++++++++++++++++++++++-
> include/linux/mfd/stm32-timers.h | 15 +
> tools/counter/Build | 1 +
> tools/counter/Makefile | 12 +-
> tools/counter/counter_watch_events.c | 368 ++++++++++++++++++
> 5 files changed, 934 insertions(+), 20 deletions(-)
> create mode 100644 tools/counter/counter_watch_events.c
>
> --
> 2.25.1
>
[-- 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
^ permalink raw reply [flat|nested] 14+ messages in thread