[PATCH v3 00/10] counter: Add stm32 timer events support

[PATCH v3 00/10] counter: Add stm32 timer events support

[Fabrice Gasnier]

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 on channel 1 & 2. It was limited to timers that has
the HW quadrature encoder.

With this series, all timer instances can be enabled for:
- Quadrature encoder on channel 1 and channel 2 (when supported in HW)
- Simple counting on internal clock
- Capture counter value when rising and falling edges are observed at
  channel 1 to channel 4 input (number of channels varies depending on
  the timer instance used)
- Push overflow and/or capture events through the chardev interface

Changes in V3:
- This series has been split [1] and renamed.
[1] https://lore.kernel.org/lkml/20230922143920.3144249-1-fabrice.gasnier@foss.st.com/
- Watch events tool patch applied, hence dropped from this series
- Refactor / split several patches
- Keep original signals order, to avoid breaking existing user space
- Don't adjust number of signals: adopt static configuration. rely on
  error mechanism to manage diversity across STM32 Timers instances.

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 (10):
  counter: stm32-timer-cnt: rename quadrature signal
  counter: stm32-timer-cnt: rename counter
  counter: stm32-timer-cnt: adopt signal definitions
  counter: stm32-timer-cnt: introduce clock signal
  counter: stm32-timer-cnt: add counter prescaler extension
  counter: stm32-timer-cnt: add checks on quadrature encoder capability
  counter: stm32-timer-cnt: introduce channels
  counter: stm32-timer-cnt: probe number of channels from registers
  counter: stm32-timer-cnt: add support for overflow events
  counter: stm32-timer-cnt: add support for capture events

 drivers/counter/stm32-timer-cnt.c | 455 +++++++++++++++++++++++++++++-
 include/linux/mfd/stm32-timers.h  |  13 +
 2 files changed, 454 insertions(+), 14 deletions(-)

-- 
2.25.1


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

[PATCH v3 01/10] counter: stm32-timer-cnt: rename quadrature signal

[Fabrice Gasnier]

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

Re: [PATCH v3 01/10] counter: stm32-timer-cnt: rename quadrature signal

[William Breathitt Gray]

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

On Wed, Dec 20, 2023 at 03:57:17PM +0100, Fabrice Gasnier wrote:
> 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>

Reviewed-by: William Breathitt Gray <william.gray@linaro.org>

[-- 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 v3 02/10] counter: stm32-timer-cnt: rename counter

[Fabrice Gasnier]

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

Re: [PATCH v3 02/10] counter: stm32-timer-cnt: rename counter

[William Breathitt Gray]

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

On Wed, Dec 20, 2023 at 03:57:18PM +0100, Fabrice Gasnier wrote:
> 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>

Reviewed-by: William Breathitt Gray <william.gray@linaro.org>

[-- 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 v3 03/10] counter: stm32-timer-cnt: adopt signal definitions

[Fabrice Gasnier]

Adopt signals definitions to ease later signals additions.
There are no intended functional changes here.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v3:
 New patch split from "counter: stm32-timer-cnt: introduce clock signal"
---
 drivers/counter/stm32-timer-cnt.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index 668e9d1061d3..c34747d7857e 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -21,6 +21,9 @@
 #define TIM_CCER_MASK	(TIM_CCER_CC1P | TIM_CCER_CC1NP | \
 			 TIM_CCER_CC2P | TIM_CCER_CC2NP)
 
+#define STM32_CH1_SIG		0
+#define STM32_CH2_SIG		1
+
 struct stm32_timer_regs {
 	u32 cr1;
 	u32 cnt;
@@ -247,14 +250,14 @@ static int stm32_action_read(struct counter_device *counter,
 		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;
@@ -278,11 +281,11 @@ static const struct counter_ops stm32_timer_cnt_ops = {
 
 static struct counter_signal stm32_signals[] = {
 	{
-		.id = 0,
+		.id = STM32_CH1_SIG,
 		.name = "Channel 1"
 	},
 	{
-		.id = 1,
+		.id = STM32_CH2_SIG,
 		.name = "Channel 2"
 	}
 };
@@ -291,12 +294,12 @@ static struct counter_synapse stm32_count_synapses[] = {
 	{
 		.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

Re: [PATCH v3 03/10] counter: stm32-timer-cnt: adopt signal definitions

[William Breathitt Gray]

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

On Wed, Dec 20, 2023 at 03:57:19PM +0100, Fabrice Gasnier wrote:
> Adopt signals definitions to ease later signals additions.
> There are no intended functional changes here.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Reviewed-by: William Breathitt Gray <william.gray@linaro.org>

[-- 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 v3 04/10] counter: stm32-timer-cnt: introduce clock signal

[Fabrice Gasnier]

Introduce the internal clock signal, used to count when in simple rising
function. Also add the "frequency" extension to the clock signal.

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

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v3:
- split the patch in 3 parts: signal definition becomes a pre-cursor patch,
  add the "prescaler" extension in its own patch.
- Move the clock signal at the end of the signals array, so existing
  userspace programs that may rely on signal0 being "Channel 1" for
  example will remain compatible.
---
 drivers/counter/stm32-timer-cnt.c | 53 ++++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 4 deletions(-)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index c34747d7857e..21a9c20c4281 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -23,6 +23,7 @@
 
 #define STM32_CH1_SIG		0
 #define STM32_CH2_SIG		1
+#define STM32_CLOCK_SIG		2
 
 struct stm32_timer_regs {
 	u32 cr1;
@@ -226,6 +227,10 @@ static struct counter_comp stm32_count_ext[] = {
 			     stm32_count_ceiling_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[] = {
 	COUNTER_SYNAPSE_ACTION_NONE,
 	COUNTER_SYNAPSE_ACTION_BOTH_EDGES
@@ -246,7 +251,10 @@ 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 */
@@ -264,7 +272,10 @@ static int stm32_action_read(struct counter_device *counter,
 		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;
@@ -279,7 +290,30 @@ 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[] = {
+	/*
+	 * Need to declare all the signals as a static array, and keep the signals order here,
+	 * even if they're unused or unexisting on some timer instances. It's an abstraction,
+	 * e.g. high level view of the counter features.
+	 *
+	 * Userspace programs may rely on signal0 to be "Channel 1", signal1 to be "Channel 2",
+	 * and so on. When a signal is unexisting, the COUNTER_SYNAPSE_ACTION_NONE can be used,
+	 * to indicate that a signal doesn't affect the counter.
+	 */
 	{
 		.id = STM32_CH1_SIG,
 		.name = "Channel 1"
@@ -287,7 +321,13 @@ static struct counter_signal stm32_signals[] = {
 	{
 		.id = STM32_CH2_SIG,
 		.name = "Channel 2"
-	}
+	},
+	{
+		.id = STM32_CLOCK_SIG,
+		.name = "Clock Signal",
+		.ext = stm32_count_clock_ext,
+		.num_ext = ARRAY_SIZE(stm32_count_clock_ext),
+	},
 };
 
 static struct counter_synapse stm32_count_synapses[] = {
@@ -300,7 +340,12 @@ 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_clock_synapse_actions,
+		.num_actions = ARRAY_SIZE(stm32_clock_synapse_actions),
+		.signal = &stm32_signals[STM32_CLOCK_SIG]
+	},
 };
 
 static struct counter_count stm32_counts = {
-- 
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 v3 04/10] counter: stm32-timer-cnt: introduce clock signal

[William Breathitt Gray]

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

On Wed, Dec 20, 2023 at 03:57:20PM +0100, Fabrice Gasnier wrote:
> Introduce the internal clock signal, used to count when in simple rising
> function. Also add the "frequency" extension to the clock signal.
> 
> With this patch, signal action reports a consistent state when "increase"
> function is used, and the counting frequency:
>     $ echo increase > function
>     $ grep -H "" signal*_action
>     signal0_action:none
>     signal1_action:none
>     signal2_action:rising edge
>     $ echo 1 > enable
>     $ cat count
>     25425
>     $ cat count
>     44439
>     $ cat ../signal2/frequency
>     208877930
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Reviewed-by: William Breathitt Gray <william.gray@linaro.org>

The code is all right, but some minor suggestions below.

> +static struct counter_comp stm32_count_clock_ext[] = {
> +	COUNTER_COMP_SIGNAL_U64("frequency", stm32_count_clk_get_freq, NULL),

It might be worth introducing a new COUNTER_COMP_FREQUENCY() macro now
that we have a second driver with the 'frequency' extension
(ti-ecap-capture also has 'frequency'). But it's up to you if you want
to add a precursor patch to this series, or I'll introduce it separately
myself in a independent patch.

> @@ -287,7 +321,13 @@ static struct counter_signal stm32_signals[] = {
>  	{
>  		.id = STM32_CH2_SIG,
>  		.name = "Channel 2"
> -	}
> +	},
> +	{
> +		.id = STM32_CLOCK_SIG,
> +		.name = "Clock Signal",

The word "Signal" feels unnecessary to me when both the sysfs path and
data structure will have 'signal' already. Do you think "Clock" by
itself is clear enough?

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 v3 04/10] counter: stm32-timer-cnt: introduce clock signal

[Fabrice Gasnier]

On 1/8/24 17:46, William Breathitt Gray wrote:
> On Wed, Dec 20, 2023 at 03:57:20PM +0100, Fabrice Gasnier wrote:
>> Introduce the internal clock signal, used to count when in simple rising
>> function. Also add the "frequency" extension to the clock signal.
>>
>> With this patch, signal action reports a consistent state when "increase"
>> function is used, and the counting frequency:
>>     $ echo increase > function
>>     $ grep -H "" signal*_action
>>     signal0_action:none
>>     signal1_action:none
>>     signal2_action:rising edge
>>     $ echo 1 > enable
>>     $ cat count
>>     25425
>>     $ cat count
>>     44439
>>     $ cat ../signal2/frequency
>>     208877930
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> 
> Reviewed-by: William Breathitt Gray <william.gray@linaro.org>
> 
> The code is all right, but some minor suggestions below.
> 
>> +static struct counter_comp stm32_count_clock_ext[] = {
>> +	COUNTER_COMP_SIGNAL_U64("frequency", stm32_count_clk_get_freq, NULL),
> 
> It might be worth introducing a new COUNTER_COMP_FREQUENCY() macro now
> that we have a second driver with the 'frequency' extension
> (ti-ecap-capture also has 'frequency'). But it's up to you if you want
> to add a precursor patch to this series, or I'll introduce it separately
> myself in a independent patch.

Thanks for suggesting.

I added a precursor patch to this series.
I guess you wishes to see it used in both ti-ecap-capture and
stm32-timer-cnt. I only cared about stm32-timer-cnt in this series.

Can I let you do ti-ecap-capture change if/when you're going to apply it?

> 
>> @@ -287,7 +321,13 @@ static struct counter_signal stm32_signals[] = {
>>  	{
>>  		.id = STM32_CH2_SIG,
>>  		.name = "Channel 2"
>> -	}
>> +	},
>> +	{
>> +		.id = STM32_CLOCK_SIG,
>> +		.name = "Clock Signal",
> 
> The word "Signal" feels unnecessary to me when both the sysfs path and
> data structure will have 'signal' already. Do you think "Clock" by
> itself is clear enough?

Agreed, I updated in v4.

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 v3 04/10] counter: stm32-timer-cnt: introduce clock signal

[William Breathitt Gray]

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

On Tue, Feb 27, 2024 at 06:43:20PM +0100, Fabrice Gasnier wrote:
> On 1/8/24 17:46, William Breathitt Gray wrote:
> > On Wed, Dec 20, 2023 at 03:57:20PM +0100, Fabrice Gasnier wrote:
> >> Introduce the internal clock signal, used to count when in simple rising
> >> function. Also add the "frequency" extension to the clock signal.
> >>
> >> With this patch, signal action reports a consistent state when "increase"
> >> function is used, and the counting frequency:
> >>     $ echo increase > function
> >>     $ grep -H "" signal*_action
> >>     signal0_action:none
> >>     signal1_action:none
> >>     signal2_action:rising edge
> >>     $ echo 1 > enable
> >>     $ cat count
> >>     25425
> >>     $ cat count
> >>     44439
> >>     $ cat ../signal2/frequency
> >>     208877930
> >>
> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> > 
> > Reviewed-by: William Breathitt Gray <william.gray@linaro.org>
> > 
> > The code is all right, but some minor suggestions below.
> > 
> >> +static struct counter_comp stm32_count_clock_ext[] = {
> >> +	COUNTER_COMP_SIGNAL_U64("frequency", stm32_count_clk_get_freq, NULL),
> > 
> > It might be worth introducing a new COUNTER_COMP_FREQUENCY() macro now
> > that we have a second driver with the 'frequency' extension
> > (ti-ecap-capture also has 'frequency'). But it's up to you if you want
> > to add a precursor patch to this series, or I'll introduce it separately
> > myself in a independent patch.
> 
> Thanks for suggesting.
> 
> I added a precursor patch to this series.
> I guess you wishes to see it used in both ti-ecap-capture and
> stm32-timer-cnt. I only cared about stm32-timer-cnt in this series.
> 
> Can I let you do ti-ecap-capture change if/when you're going to apply it?

Thanks Fabrice, I'll pick up the precursor patch so we an start using it
in other drivers. Syed will take on the ti-ecap-capture change, and I've
CC'd Vignesh and Julien to this email as well so they are aware of the
incoming patch.

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 v3 05/10] counter: stm32-timer-cnt: add counter prescaler extension

[Fabrice Gasnier]

There's a prescaler in between the selected input signal used for
counting (CK_PSC), and the counter input (CK_CNT).
So add the "prescaler" extension to the counter.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v3:
- New patch split from "counter: stm32-timer-cnt: introduce clock signal"
---
 drivers/counter/stm32-timer-cnt.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index 21a9c20c4281..bf2726dd7f12 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -220,11 +220,40 @@ 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[] = {
-- 
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 v3 05/10] counter: stm32-timer-cnt: add counter prescaler extension

[William Breathitt Gray]

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

On Wed, Dec 20, 2023 at 03:57:21PM +0100, Fabrice Gasnier wrote:
> There's a prescaler in between the selected input signal used for
> counting (CK_PSC), and the counter input (CK_CNT).
> So add the "prescaler" extension to the counter.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> ---
> Changes in v3:
> - New patch split from "counter: stm32-timer-cnt: introduce clock signal"

Reviewed-by: William Breathitt Gray <william.gray@linaro.org>

[-- 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 v3 06/10] counter: stm32-timer-cnt: add checks on quadrature encoder capability

[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 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.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v3:
- New patch split from:
  "counter: stm32-timer-cnt: populate capture channels and check encoder"
- return -EOPNOTSUPP when encoder function isn't supported by the timer
  instance.
---
 drivers/counter/stm32-timer-cnt.c | 55 +++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index bf2726dd7f12..6933d42b16bf 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>
@@ -38,6 +39,8 @@ struct stm32_timer_cnt {
 	u32 max_arr;
 	bool enabled;
 	struct stm32_timer_regs bak;
+	bool has_encoder;
+	u32 idx;
 };
 
 static const enum counter_function stm32_count_functions[] = {
@@ -111,12 +114,18 @@ static int stm32_count_function_write(struct counter_device *counter,
 		sms = TIM_SMCR_SMS_SLAVE_MODE_DISABLED;
 		break;
 	case COUNTER_FUNCTION_QUADRATURE_X2_A:
+		if (!priv->has_encoder)
+			return -EOPNOTSUPP;
 		sms = TIM_SMCR_SMS_ENCODER_MODE_1;
 		break;
 	case COUNTER_FUNCTION_QUADRATURE_X2_B:
+		if (!priv->has_encoder)
+			return -EOPNOTSUPP;
 		sms = TIM_SMCR_SMS_ENCODER_MODE_2;
 		break;
 	case COUNTER_FUNCTION_QUADRATURE_X4:
+		if (!priv->has_encoder)
+			return -EOPNOTSUPP;
 		sms = TIM_SMCR_SMS_ENCODER_MODE_3;
 		break;
 	default:
@@ -388,6 +397,48 @@ static struct counter_count stm32_counts = {
 	.num_ext = ARRAY_SIZE(stm32_count_ext)
 };
 
+/* 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);
@@ -409,6 +460,10 @@ 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;
+
 	counter->name = dev_name(dev);
 	counter->parent = dev;
 	counter->ops = &stm32_timer_cnt_ops;
-- 
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 v3 06/10] counter: stm32-timer-cnt: add checks on quadrature encoder capability

[William Breathitt Gray]

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

On Wed, Dec 20, 2023 at 03:57:22PM +0100, 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 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.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> ---
> Changes in v3:
> - New patch split from:
>   "counter: stm32-timer-cnt: populate capture channels and check encoder"
> - return -EOPNOTSUPP when encoder function isn't supported by the timer
>   instance.
> ---
>  drivers/counter/stm32-timer-cnt.c | 55 +++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
> index bf2726dd7f12..6933d42b16bf 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>
> @@ -38,6 +39,8 @@ struct stm32_timer_cnt {
>  	u32 max_arr;
>  	bool enabled;
>  	struct stm32_timer_regs bak;
> +	bool has_encoder;
> +	u32 idx;

This idx value is only used temporarily to determine whether the device has
support for encoder mode. Do we really need to store it indefinitely
here?

> +static int stm32_timer_cnt_probe_encoder(struct platform_device *pdev,

This function uses 'pdev' only to access 'dev'. It'll be better to pass
a reference to 'dev' directly instead.

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 v3 07/10] counter: stm32-timer-cnt: introduce channels

[Fabrice Gasnier]

Simply add channels 3 and 4 that can be used for capture. Statically
add them, despite some timers doesn't have them. Rather rely on
stm32_action_read that will report "none" action for these currently.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v3:
- New patch split from:
  "counter: stm32-timer-cnt: populate capture channels and check encoder"
---
 drivers/counter/stm32-timer-cnt.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index 6933d42b16bf..55eb6af34750 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -25,6 +25,8 @@
 #define STM32_CH1_SIG		0
 #define STM32_CH2_SIG		1
 #define STM32_CLOCK_SIG		2
+#define STM32_CH3_SIG		3
+#define STM32_CH4_SIG		4
 
 struct stm32_timer_regs {
 	u32 cr1;
@@ -366,6 +368,14 @@ static struct counter_signal stm32_signals[] = {
 		.ext = stm32_count_clock_ext,
 		.num_ext = ARRAY_SIZE(stm32_count_clock_ext),
 	},
+	{
+		.id = STM32_CH3_SIG,
+		.name = "Channel 3"
+	},
+	{
+		.id = STM32_CH4_SIG,
+		.name = "Channel 4"
+	},
 };
 
 static struct counter_synapse stm32_count_synapses[] = {
@@ -384,6 +394,16 @@ static struct counter_synapse stm32_count_synapses[] = {
 		.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[STM32_CH3_SIG]
+	},
+	{
+		.actions_list = stm32_synapse_actions,
+		.num_actions = ARRAY_SIZE(stm32_synapse_actions),
+		.signal = &stm32_signals[STM32_CH4_SIG]
+	},
 };
 
 static struct counter_count stm32_counts = {
-- 
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 v3 07/10] counter: stm32-timer-cnt: introduce channels

[William Breathitt Gray]

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

On Wed, Dec 20, 2023 at 03:57:23PM +0100, Fabrice Gasnier wrote:
> Simply add channels 3 and 4 that can be used for capture. Statically
> add them, despite some timers doesn't have them. Rather rely on
> stm32_action_read that will report "none" action for these currently.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Reviewed-by: William Breathitt Gray <william.gray@linaro.org>

[-- 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 v3 08/10] counter: stm32-timer-cnt: probe number of channels from registers

[Fabrice Gasnier]

Probe the number of capture compare channels, by writing CCER register bits
and read them back. Take care to restore the register original value.

This is a precursor patch to support capture channels.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v3:
- New patch split from:
  "counter: stm32-timer-cnt: populate capture channels and check encoder"
---
 drivers/counter/stm32-timer-cnt.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index 55eb6af34750..b5dc4378fecf 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -43,6 +43,7 @@ struct stm32_timer_cnt {
 	struct stm32_timer_regs bak;
 	bool has_encoder;
 	u32 idx;
+	unsigned int nchannels;
 };
 
 static const enum counter_function stm32_count_functions[] = {
@@ -417,6 +418,20 @@ static struct counter_count stm32_counts = {
 	.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))
 
@@ -484,6 +499,8 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
 	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;
-- 
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 v3 08/10] counter: stm32-timer-cnt: probe number of channels from registers

[William Breathitt Gray]

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

On Wed, Dec 20, 2023 at 03:57:24PM +0100, Fabrice Gasnier wrote:
> Probe the number of capture compare channels, by writing CCER register bits
> and read them back. Take care to restore the register original value.
> 
> This is a precursor patch to support capture channels.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> ---
> Changes in v3:
> - New patch split from:
>   "counter: stm32-timer-cnt: populate capture channels and check encoder"
> ---
>  drivers/counter/stm32-timer-cnt.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
> index 55eb6af34750..b5dc4378fecf 100644
> --- a/drivers/counter/stm32-timer-cnt.c
> +++ b/drivers/counter/stm32-timer-cnt.c
> @@ -43,6 +43,7 @@ struct stm32_timer_cnt {
>  	struct stm32_timer_regs bak;
>  	bool has_encoder;
>  	u32 idx;
> +	unsigned int nchannels;
>  };
>  
>  static const enum counter_function stm32_count_functions[] = {
> @@ -417,6 +418,20 @@ static struct counter_count stm32_counts = {
>  	.num_ext = ARRAY_SIZE(stm32_count_ext)
>  };
>  
> +static void stm32_timer_cnt_detect_channels(struct platform_device *pdev,

Like stm32_timer_cnt_probe_encoder() in one of the previous patches,
this function uses 'pdev' only to access 'dev'. Pass a reference to
'dev' directly instead.

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 v3 09/10] counter: stm32-timer-cnt: add support for overflow events

[Fabrice Gasnier]

Add support overflow events. Also add the related validation and
configuration routine. Register and enable interrupts to push events.
STM32 Timers can have either 1 global interrupt, or 4 dedicated interrupt
lines. Request only the necessary interrupt, e.g. either global interrupt
that can report all event types, or update interrupt only for overflow
event.

Acked-by: Lee Jones <lee@kernel.org>
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v3:
- patch split from: "counter: stm32-timer-cnt: add support for events", to
  focus on the overflow events only here. Move the capture event support
  to a separate patch.
- simplify the patch, by removing diversity introduced by the number of
  possible channels. All channels are now exposed instead.
  Report an error when trying to access a channel that doesn't exist.

Changes in v2:
- fix warnings (kernel test robot)
- fix a typo
- add collected ack from Lee
---
 drivers/counter/stm32-timer-cnt.c | 132 +++++++++++++++++++++++++++++-
 include/linux/mfd/stm32-timers.h  |  13 +++
 2 files changed, 144 insertions(+), 1 deletion(-)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index b5dc4378fecf..d13e4c427965 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>
@@ -44,6 +45,9 @@ struct stm32_timer_cnt {
 	bool has_encoder;
 	u32 idx;
 	unsigned int nchannels;
+	unsigned int nr_irqs;
+	u32 *irq;
+	atomic_t nb_ovf;
 };
 
 static const enum counter_function stm32_count_functions[] = {
@@ -259,6 +263,29 @@ static int stm32_count_prescaler_write(struct counter_device *counter,
 	return regmap_write(priv->regmap, TIM_PSC, psc);
 }
 
+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 != (typeof(priv->nb_ovf.counter))val)
+		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),
@@ -266,6 +293,7 @@ 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 const enum counter_synapse_action stm32_clock_synapse_actions[] = {
@@ -323,12 +351,57 @@ static int stm32_action_read(struct counter_device *counter,
 	}
 }
 
+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;
+	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;
+		default:
+			/* should never reach this path */
+			return -EINVAL;
+		}
+	}
+
+	regmap_write(priv->regmap, TIM_DIER, dier);
+
+	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);
+
+	/* Interrupts are optional */
+	if (!priv->nr_irqs)
+		return -EOPNOTSUPP;
+
+	switch (watch->event) {
+	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,
@@ -418,6 +491,35 @@ 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;
+
+	regmap_read(priv->regmap, TIM_SR, &sr);
+	regmap_read(priv->regmap, TIM_DIER, &dier);
+	/*
+	 * Some status bits in SR don't match with the enable bits in DIER. Only take care of
+	 * the possibly enabled bits in DIER (that matches in between SR and DIER).
+	 */
+	dier &= TIM_DIER_UIE;
+	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;
+	}
+
+	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)
 {
@@ -480,7 +582,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;
@@ -494,6 +596,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)
@@ -511,6 +615,32 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, priv);
 
+	/* STM32 Timers can have either 1 global, or 4 dedicated interrupts (optional) */
+	if (priv->nr_irqs == 1) {
+		/* All events reported through the global interrupt */
+		ret = devm_request_irq(&pdev->dev, priv->irq[0], 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;
+		}
+	} else {
+		for (i = 0; i < priv->nr_irqs; i++) {
+			/* Only take care of update IRQ for overflow events */
+			if (i != STM32_TIMERS_IRQ_UP)
+				continue;
+
+			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..9eb17481b07f 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 */
-- 
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 v3 09/10] counter: stm32-timer-cnt: add support for overflow events

[William Breathitt Gray]

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

On Wed, Dec 20, 2023 at 03:57:25PM +0100, Fabrice Gasnier wrote:
> Add support overflow events. Also add the related validation and
> configuration routine. Register and enable interrupts to push events.
> STM32 Timers can have either 1 global interrupt, or 4 dedicated interrupt
> lines. Request only the necessary interrupt, e.g. either global interrupt
> that can report all event types, or update interrupt only for overflow
> event.
> 
> Acked-by: Lee Jones <lee@kernel.org>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Hi Fabrice,

I've CC'd Will and Peter in case they can provide some suggestions
regarding my atomic_t comment inline below.

> @@ -44,6 +45,9 @@ struct stm32_timer_cnt {
>  	bool has_encoder;
>  	u32 idx;
>  	unsigned int nchannels;
> +	unsigned int nr_irqs;
> +	u32 *irq;

Looks like we only need this 'irq' array for registering the ISR in
stm32_timer_cnt_probe(). Since we won't need it anymore after that,
let's use ddata->irq directly instead of defining priv->irq.

> +	atomic_t nb_ovf;
>  };
>  
>  static const enum counter_function stm32_count_functions[] = {
> @@ -259,6 +263,29 @@ static int stm32_count_prescaler_write(struct counter_device *counter,
>  	return regmap_write(priv->regmap, TIM_PSC, psc);
>  }
>  
> +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 != (typeof(priv->nb_ovf.counter))val)
> +		return -ERANGE;
> +
> +	atomic_set(&priv->nb_ovf, val);

So you want to check that the atomic_t 'nb_ovf' is able hold the value
provided by the u64 'val'. My understanding is that atomic_t should be
treated as an opaque type, so I don't think we should be accessing the
'counter' member directly for this test (interrupt-cnt does this but I
believe it's wrong to do so).

I don't know if we have any existing way to check for the value range of
an atomic_t (I don't see anything under include/linux/limits.h
specifically for it). However, you do use atomic_set() which takes an
int parameter, so perhaps we should compare against INT_MAX instead.

> +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;
> +	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))

You can eliminate 'val' and the regmap_read() line like this:

    if (!regmap_test_bits(priv->regmap, TIM_DIER, TIM_DIER_UIE))

> +				regmap_write(priv->regmap, TIM_SR, (u32)~TIM_SR_UIF);
> +			dier |= TIM_DIER_UIE;
> +			break;
> +		default:
> +			/* should never reach this path */
> +			return -EINVAL;
> +		}
> +	}
> +
> +	regmap_write(priv->regmap, TIM_DIER, dier);

Do you want to overwrite TIM_DIER completely, or did you mean to set
only TIM_DIER_UIE and preserve the rest of the register? If the latter,
you could redefine 'dier' as a bool and do:

    regmap_update_bits(priv->regmap, TIM_DIER, TIM_DIER_UIE, dier);

There is also a regmap_update_bits_check() available if you want to
combine the UIF latch check with the update; but I don't know if that
will work in this case because it looks like you want to clear the UIF
latch before enabling.

>  static int stm32_count_clk_get_freq(struct counter_device *counter,
> @@ -418,6 +491,35 @@ 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;
> +
> +	regmap_read(priv->regmap, TIM_SR, &sr);
> +	regmap_read(priv->regmap, TIM_DIER, &dier);
> +	/*
> +	 * Some status bits in SR don't match with the enable bits in DIER. Only take care of
> +	 * the possibly enabled bits in DIER (that matches in between SR and DIER).
> +	 */
> +	dier &= TIM_DIER_UIE;
> +	sr &= dier;
> +
> +	if (sr & TIM_SR_UIF) {

Am I understanding this logic correctly? ANDing TIM_DIER_UIE with 'dier'
will result in just the state of the TIM_DIER_UIE bit. Next, we AND that
state with 'sr'; so sr is 0 when TIM_DIER_UIE state is low, but we get
the respective SR bit when TIM_DIER_UIE state is high. Finally, we check
the TIM_SR_UIF bit state in 'sr'.

If TIM_SR_UIF bit position is expected to match the TIM_DIER_UIE bit
position, then (sr & TIM_SR_UIF) will only be true when the state of
both the TIM_DIER_UIE bit and TIM_SR_UIF bit are high. That means you
can eliminate 'sr', 'dier', and the two regmap_read() operations with
this instead:

    if (regmap_test_bits(priv->regmap, TIM_SR, TIM_SR_UIF) &&
        regmap_test_bits(priv->regmap, TIM_DIER, TIM_DIER_UIE) {

> +		atomic_inc(&priv->nb_ovf);

I wonder what happens when atomic_inc() increments past the atomic_t max
value. Does atomic_read() report back a negative value? Do we need to
guard against that scenario somehow?

> +		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;

You can use u32p_replace_bits(&clr, 0, TIM_SR_UIF) instead after
including the include/linux/bitfield.h header.

> @@ -511,6 +615,32 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, priv);
>  
> +	/* STM32 Timers can have either 1 global, or 4 dedicated interrupts (optional) */
> +	if (priv->nr_irqs == 1) {
> +		/* All events reported through the global interrupt */
> +		ret = devm_request_irq(&pdev->dev, priv->irq[0], 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);

This should be irq[0], right?

I would also recommend using ddata->irq instead so we can get rid of
priv->irq outside of this probe function.

> +			return ret;
> +		}
> +	} else {
> +		for (i = 0; i < priv->nr_irqs; i++) {
> +			/* Only take care of update IRQ for overflow events */
> +			if (i != STM32_TIMERS_IRQ_UP)
> +				continue;
> +
> +			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;
> +			}
> +		}

So we only execute the loop body once for this particular
STM32_TIMERS_IRQ_UP iteration? Why have the loop at all rather than
hardcode irq[STM32_TIMERS_IRQ_UP] for devm_request_irq()?

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 v3 09/10] counter: stm32-timer-cnt: add support for overflow events

[Fabrice Gasnier]

On 1/8/24 22:00, William Breathitt Gray wrote:
> On Wed, Dec 20, 2023 at 03:57:25PM +0100, Fabrice Gasnier wrote:
>> Add support overflow events. Also add the related validation and
>> configuration routine. Register and enable interrupts to push events.
>> STM32 Timers can have either 1 global interrupt, or 4 dedicated interrupt
>> lines. Request only the necessary interrupt, e.g. either global interrupt
>> that can report all event types, or update interrupt only for overflow
>> event.
>>
>> Acked-by: Lee Jones <lee@kernel.org>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> 
> Hi Fabrice,

Hi William,

Sorry for the late reply,

> 
> I've CC'd Will and Peter in case they can provide some suggestions
> regarding my atomic_t comment inline below.

I simply changed the type of nb_ovf below to u64, which better fits with
the API IMHO. Please check in v4.

> 
>> @@ -44,6 +45,9 @@ struct stm32_timer_cnt {
>>  	bool has_encoder;
>>  	u32 idx;
>>  	unsigned int nchannels;
>> +	unsigned int nr_irqs;
>> +	u32 *irq;
> 
> Looks like we only need this 'irq' array for registering the ISR in
> stm32_timer_cnt_probe(). Since we won't need it anymore after that,
> let's use ddata->irq directly instead of defining priv->irq.

Ack.

> 
>> +	atomic_t nb_ovf;
>>  };
>>  
>>  static const enum counter_function stm32_count_functions[] = {
>> @@ -259,6 +263,29 @@ static int stm32_count_prescaler_write(struct counter_device *counter,
>>  	return regmap_write(priv->regmap, TIM_PSC, psc);
>>  }
>>  
>> +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 != (typeof(priv->nb_ovf.counter))val)
>> +		return -ERANGE;
>> +
>> +	atomic_set(&priv->nb_ovf, val);
> 
> So you want to check that the atomic_t 'nb_ovf' is able hold the value
> provided by the u64 'val'. My understanding is that atomic_t should be
> treated as an opaque type, so I don't think we should be accessing the
> 'counter' member directly for this test (interrupt-cnt does this but I
> believe it's wrong to do so).
> 
> I don't know if we have any existing way to check for the value range of
> an atomic_t (I don't see anything under include/linux/limits.h
> specifically for it). However, you do use atomic_set() which takes an
> int parameter, so perhaps we should compare against INT_MAX instead.

Ack. Moving nb_ovf to u64 in v4 should address all these concerns.

> 
>> +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;
>> +	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))
> 
> You can eliminate 'val' and the regmap_read() line like this:
> 
>     if (!regmap_test_bits(priv->regmap, TIM_DIER, TIM_DIER_UIE))

Ack. Thanks for suggesting.

> 
>> +				regmap_write(priv->regmap, TIM_SR, (u32)~TIM_SR_UIF);
>> +			dier |= TIM_DIER_UIE;
>> +			break;
>> +		default:
>> +			/* should never reach this path */
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	regmap_write(priv->regmap, TIM_DIER, dier);
> 
> Do you want to overwrite TIM_DIER completely, or did you mean to set
> only TIM_DIER_UIE and preserve the rest of the register? If the latter,
> you could redefine 'dier' as a bool and do:
> 
>     regmap_update_bits(priv->regmap, TIM_DIER, TIM_DIER_UIE, dier);
> 
> There is also a regmap_update_bits_check() available if you want to
> combine the UIF latch check with the update; but I don't know if that
> will work in this case because it looks like you want to clear the UIF
> latch before enabling.

As you've noticed, the subsequent patch answers this.
Still I added a comment above this line.

> 
>>  static int stm32_count_clk_get_freq(struct counter_device *counter,
>> @@ -418,6 +491,35 @@ 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;
>> +
>> +	regmap_read(priv->regmap, TIM_SR, &sr);
>> +	regmap_read(priv->regmap, TIM_DIER, &dier);
>> +	/*
>> +	 * Some status bits in SR don't match with the enable bits in DIER. Only take care of
>> +	 * the possibly enabled bits in DIER (that matches in between SR and DIER).
>> +	 */
>> +	dier &= TIM_DIER_UIE;
>> +	sr &= dier;
>> +
>> +	if (sr & TIM_SR_UIF) {
> 
> Am I understanding this logic correctly? ANDing TIM_DIER_UIE with 'dier'
> will result in just the state of the TIM_DIER_UIE bit. Next, we AND that
> state with 'sr'; so sr is 0 when TIM_DIER_UIE state is low, but we get
> the respective SR bit when TIM_DIER_UIE state is high. Finally, we check
> the TIM_SR_UIF bit state in 'sr'.

Same, next patch makes it more clear.

> 
> If TIM_SR_UIF bit position is expected to match the TIM_DIER_UIE bit
> position, then (sr & TIM_SR_UIF) will only be true when the state of
> both the TIM_DIER_UIE bit and TIM_SR_UIF bit are high. That means you
> can eliminate 'sr', 'dier', and the two regmap_read() operations with
> this instead:
> 
>     if (regmap_test_bits(priv->regmap, TIM_SR, TIM_SR_UIF) &&
>         regmap_test_bits(priv->regmap, TIM_DIER, TIM_DIER_UIE) {
> 
>> +		atomic_inc(&priv->nb_ovf);
> 
> I wonder what happens when atomic_inc() increments past the atomic_t max
> value. Does atomic_read() report back a negative value? Do we need to
> guard against that scenario somehow?

Ack, nb_ovf moved to u64 in patch v4. So negative value shouldn't be an
issue. Thanks for pointing this.

> 
>> +		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;
> 
> You can use u32p_replace_bits(&clr, 0, TIM_SR_UIF) instead after
> including the include/linux/bitfield.h header.

Thanks for suggesting. I tried this here, it seems fine. However, in
subsequent patch, doing the same change with TIM_SR_CC_IF(i) macro gives
a build error. To be consistent in between the 2 patchs, I prefer to
keep simple bit ops here.

> 
>> @@ -511,6 +615,32 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
>>  
>>  	platform_set_drvdata(pdev, priv);
>>  
>> +	/* STM32 Timers can have either 1 global, or 4 dedicated interrupts (optional) */
>> +	if (priv->nr_irqs == 1) {
>> +		/* All events reported through the global interrupt */
>> +		ret = devm_request_irq(&pdev->dev, priv->irq[0], 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);
> 
> This should be irq[0], right?

Yes.
> 
> I would also recommend using ddata->irq instead so we can get rid of
> priv->irq outside of this probe function.

Done.

Thanks for reviewing
BR,
Fabrice

> 
>> +			return ret;
>> +		}
>> +	} else {
>> +		for (i = 0; i < priv->nr_irqs; i++) {
>> +			/* Only take care of update IRQ for overflow events */
>> +			if (i != STM32_TIMERS_IRQ_UP)
>> +				continue;
>> +
>> +			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;
>> +			}
>> +		}
> 
> So we only execute the loop body once for this particular
> STM32_TIMERS_IRQ_UP iteration? Why have the loop at all rather than
> hardcode irq[STM32_TIMERS_IRQ_UP] for devm_request_irq()?
> 
> 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 v3 09/10] counter: stm32-timer-cnt: add support for overflow events

[William Breathitt Gray]

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

On Wed, Dec 20, 2023 at 03:57:25PM +0100, Fabrice Gasnier wrote:
> diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
> index ca35af30745f..9eb17481b07f 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   */

I forgot to mention that you should move the introduction of these
defines to the subsequent patch adding support for capture events
because that's where the defines are actually used.

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 v3 10/10] counter: stm32-timer-cnt: add support for capture events

[Fabrice Gasnier]

Add support for capture events. Captured counter value for each channel
can be retrieved through CCRx register.
STM32 timers can have up to 4 capture channels (on input channel 1 to
channel 4), hence need to check the number of channels before reading
the capture data.
The capture configuration is hard-coded to capture signals on both edges
(non-inverted). Interrupts are used to report events independently for
each channel.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v3:
- patch split from: "counter: stm32-timer-cnt: add support for events", to
  focus on the capture events only here.
- only get relevant interrupt line
---
 drivers/counter/stm32-timer-cnt.c | 134 +++++++++++++++++++++++++++++-
 1 file changed, 131 insertions(+), 3 deletions(-)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index d13e4c427965..0b131ca71de6 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -263,6 +263,40 @@ 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;
+
+	if (ch >= priv->nchannels)
+		return -EOPNOTSUPP;
+
+	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 + 1, ccrx);
+
+	*cap = ccrx;
+
+	return 0;
+}
+
 static int stm32_count_nb_ovf_read(struct counter_device *counter,
 				   struct counter_count *count, u64 *val)
 {
@@ -286,6 +320,8 @@ static int stm32_count_nb_ovf_write(struct counter_device *counter,
 	return 0;
 }
 
+static DEFINE_COUNTER_ARRAY_CAPTURE(stm32_count_cap_array, 4);
+
 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),
@@ -293,6 +329,7 @@ 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_ARRAY_CAPTURE(stm32_count_cap_read, NULL, stm32_count_cap_array),
 	COUNTER_COMP_COUNT_U64("num_overflows", stm32_count_nb_ovf_read, stm32_count_nb_ovf_write),
 };
 
@@ -351,11 +388,68 @@ 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) || ch >= priv->nchannels) {
+		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;
 	u32 val, dier = 0;
+	int i, ret;
 
 	list_for_each_entry(event_node, &counter->events_list, l) {
 		switch (event_node->event) {
@@ -366,6 +460,12 @@ static int stm32_count_events_configure(struct counter_device *counter)
 				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;
@@ -374,6 +474,15 @@ static int stm32_count_events_configure(struct counter_device *counter)
 
 	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;
 }
 
@@ -387,6 +496,12 @@ static int stm32_count_watch_validate(struct counter_device *counter,
 		return -EOPNOTSUPP;
 
 	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:
@@ -497,6 +612,7 @@ static irqreturn_t stm32_timer_cnt_isr(int irq, void *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);
@@ -504,7 +620,7 @@ static irqreturn_t stm32_timer_cnt_isr(int irq, void *ptr)
 	 * Some status bits in SR don't match with the enable bits in DIER. Only take care of
 	 * the possibly enabled bits in DIER (that matches in between SR and DIER).
 	 */
-	dier &= TIM_DIER_UIE;
+	dier &= (TIM_DIER_UIE | TIM_DIER_CC1IE | TIM_DIER_CC2IE | TIM_DIER_CC3IE | TIM_DIER_CC4IE);
 	sr &= dier;
 
 	if (sr & TIM_SR_UIF) {
@@ -515,6 +631,15 @@ static irqreturn_t stm32_timer_cnt_isr(int irq, void *ptr)
 		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;
@@ -627,8 +752,11 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
 		}
 	} else {
 		for (i = 0; i < priv->nr_irqs; i++) {
-			/* Only take care of update IRQ for overflow events */
-			if (i != STM32_TIMERS_IRQ_UP)
+			/*
+			 * Only take care of update IRQ for overflow events, and cc for
+			 * capture events.
+			 */
+			if (i != STM32_TIMERS_IRQ_UP && i != STM32_TIMERS_IRQ_CC)
 				continue;
 
 			ret = devm_request_irq(&pdev->dev, priv->irq[i], stm32_timer_cnt_isr,
-- 
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 v3 10/10] counter: stm32-timer-cnt: add support for capture events

[William Breathitt Gray]

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

On Wed, Dec 20, 2023 at 03:57:26PM +0100, Fabrice Gasnier wrote:
> +	/*
> +	 * configure channel in input capture mode, map channel 1 on TI1, channel2 on TI2...
> +	 * Select both edges / non-inverted to trigger a capture.
> +	 */

I suggest defining a new local variable 'cc' to point to stm32_cc[ch]. I
think that's make the code look nicer here to avoid all the array index
syntax every time you access stm32_cc[ch].

> +	if (enable) {
> +		/* first clear possibly latched capture flag upon enabling */
> +		regmap_read(priv->regmap, TIM_CCER, &ccer);
> +		if (!(ccer & stm32_cc[ch].ccer_bits)) {

Try regmap_test_bits() here instead of using regmap_read().

> +			sr = ~TIM_SR_CC_IF(ch);
> +			regmap_write(priv->regmap, TIM_SR, sr);

Eliminate 'sr' by regmap_write(priv->regmap, TIM_SR, ~TIM_SR_CC_IF(ch)).

> @@ -366,6 +460,12 @@ static int stm32_count_events_configure(struct counter_device *counter)
>  				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);

Ah, now I understand why the previous patch OR'd TIM_DIER_UIE to dier.
Apologies for the noise.

> @@ -374,6 +474,15 @@ static int stm32_count_events_configure(struct counter_device *counter)
>  
>  	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;
> +		}

Would for_each_clear_bitrange() in linux/find.h work for this loop?

> @@ -504,7 +620,7 @@ static irqreturn_t stm32_timer_cnt_isr(int irq, void *ptr)
>  	 * Some status bits in SR don't match with the enable bits in DIER. Only take care of
>  	 * the possibly enabled bits in DIER (that matches in between SR and DIER).
>  	 */
> -	dier &= TIM_DIER_UIE;
> +	dier &= (TIM_DIER_UIE | TIM_DIER_CC1IE | TIM_DIER_CC2IE | TIM_DIER_CC3IE | TIM_DIER_CC4IE);

Again, sorry for the noise on the previous patch; this makes sense now.

> @@ -515,6 +631,15 @@ static irqreturn_t stm32_timer_cnt_isr(int irq, void *ptr)
>  		clr &= ~TIM_SR_UIF;
>  	}
>  
> +	/* Check capture events */
> +	for (i = 0 ; i < priv->nchannels; i++) {
> +		if (sr & TIM_SR_CC_IF(i)) {

Would for_each_set_bitrange() in linux/find.h work for this loop?

> +			counter_push_event(counter, COUNTER_EVENT_CAPTURE, i);
> +			clr &= ~TIM_SR_CC_IF(i);

Perhaps u32p_replace_bits(&clr, 0, TIM_SR_CC_IF(i)) is clearer here.

> @@ -627,8 +752,11 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
>  		}
>  	} else {
>  		for (i = 0; i < priv->nr_irqs; i++) {
> -			/* Only take care of update IRQ for overflow events */
> -			if (i != STM32_TIMERS_IRQ_UP)
> +			/*
> +			 * Only take care of update IRQ for overflow events, and cc for
> +			 * capture events.
> +			 */
> +			if (i != STM32_TIMERS_IRQ_UP && i != STM32_TIMERS_IRQ_CC)
>  				continue;

Okay, I see now why you have this check. This should be fine as it'll
makes adding support in the future for the other IRQs a less invasive
change.

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 v3 10/10] counter: stm32-timer-cnt: add support for capture events

[Fabrice Gasnier]

On 1/8/24 23:07, William Breathitt Gray wrote:
> On Wed, Dec 20, 2023 at 03:57:26PM +0100, Fabrice Gasnier wrote:
>> +	/*
>> +	 * configure channel in input capture mode, map channel 1 on TI1, channel2 on TI2...
>> +	 * Select both edges / non-inverted to trigger a capture.
>> +	 */
> 
> I suggest defining a new local variable 'cc' to point to stm32_cc[ch]. I
> think that's make the code look nicer here to avoid all the array index
> syntax every time you access stm32_cc[ch].

Hi William,

Thanks for suggesting.
Done.

> 
>> +	if (enable) {
>> +		/* first clear possibly latched capture flag upon enabling */
>> +		regmap_read(priv->regmap, TIM_CCER, &ccer);
>> +		if (!(ccer & stm32_cc[ch].ccer_bits)) {
> 
> Try regmap_test_bits() here instead of using regmap_read().

Done,

> 
>> +			sr = ~TIM_SR_CC_IF(ch);
>> +			regmap_write(priv->regmap, TIM_SR, sr);
> 
> Eliminate 'sr' by regmap_write(priv->regmap, TIM_SR, ~TIM_SR_CC_IF(ch)).
> 
>> @@ -366,6 +460,12 @@ static int stm32_count_events_configure(struct counter_device *counter)
>>  				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);
> 
> Ah, now I understand why the previous patch OR'd TIM_DIER_UIE to dier.
> Apologies for the noise.
> 
>> @@ -374,6 +474,15 @@ static int stm32_count_events_configure(struct counter_device *counter)
>>  
>>  	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;
>> +		}
> 
> Would for_each_clear_bitrange() in linux/find.h work for this loop?

I had a look, but it requires to add some variables, for start and stop
bit in the bitmap. For now, I've kept the simple BIT macro and bit ops.

> 
>> @@ -504,7 +620,7 @@ static irqreturn_t stm32_timer_cnt_isr(int irq, void *ptr)
>>  	 * Some status bits in SR don't match with the enable bits in DIER. Only take care of
>>  	 * the possibly enabled bits in DIER (that matches in between SR and DIER).
>>  	 */
>> -	dier &= TIM_DIER_UIE;
>> +	dier &= (TIM_DIER_UIE | TIM_DIER_CC1IE | TIM_DIER_CC2IE | TIM_DIER_CC3IE | TIM_DIER_CC4IE);
> 
> Again, sorry for the noise on the previous patch; this makes sense now.
> 
>> @@ -515,6 +631,15 @@ static irqreturn_t stm32_timer_cnt_isr(int irq, void *ptr)
>>  		clr &= ~TIM_SR_UIF;
>>  	}
>>  
>> +	/* Check capture events */
>> +	for (i = 0 ; i < priv->nchannels; i++) {
>> +		if (sr & TIM_SR_CC_IF(i)) {
> 
> Would for_each_set_bitrange() in linux/find.h work for this loop?

same.

> 
>> +			counter_push_event(counter, COUNTER_EVENT_CAPTURE, i);
>> +			clr &= ~TIM_SR_CC_IF(i);
> 
> Perhaps u32p_replace_bits(&clr, 0, TIM_SR_CC_IF(i)) is clearer here.

I've hit some build issue with TIM_SR_CC_IF(i) macro, e.g.:
./include/linux/bitfield.h:165:17: error: call to ‘__bad_mask’ declared
with attribute error: bad bitfield mask
  165 |                 __bad_mask();

So I've kept the simple bit operation here.

Thanks & Best Regards,
Fabrice

> 
>> @@ -627,8 +752,11 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
>>  		}
>>  	} else {
>>  		for (i = 0; i < priv->nr_irqs; i++) {
>> -			/* Only take care of update IRQ for overflow events */
>> -			if (i != STM32_TIMERS_IRQ_UP)
>> +			/*
>> +			 * Only take care of update IRQ for overflow events, and cc for
>> +			 * capture events.
>> +			 */
>> +			if (i != STM32_TIMERS_IRQ_UP && i != STM32_TIMERS_IRQ_CC)
>>  				continue;
> 
> Okay, I see now why you have this check. This should be fine as it'll
> makes adding support in the future for the other IRQs a less invasive
> change.
> 
> William Breathitt Gray

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