[PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[Bence Csókás]

The TCB has three R/W-able "general purpose" hardware registers:
RA, RB and RC. The hardware is capable of:
* sampling Counter Value Register (CV) to RA/RB on a trigger edge
* sending an interrupt of this change
* sending an interrupt on CV change due to trigger
* triggering an interrupt on CV compare to RC
* stop counting after sampling to RB

To enable using these features in user-space, an interrupt handler
was added, generating the necessary counter events. On top, RA/B/C
registers are added as Count Extensions. To aid interoperation, a
uapi header was also added, containing the various numeral IDs of
the Extensions, Event channels etc.

Bence Csókás (2):
  counter: microchip-tcb-capture: Add IRQ handling
  counter: microchip-tcb-capture: Add capture extensions for registers
    RA-RC

 MAINTAINERS                                   |   1 +
 drivers/counter/microchip-tcb-capture.c       | 137 ++++++++++++++++++
 .../linux/counter/microchip-tcb-capture.h     |  49 +++++++
 3 files changed, 187 insertions(+)
 create mode 100644 include/uapi/linux/counter/microchip-tcb-capture.h

-- 
2.48.1

[PATCH v4 1/2] counter: microchip-tcb-capture: Add IRQ handling

[Bence Csókás]

Add interrupt servicing to allow userspace to wait for the following events:
* Change-of-state caused by external trigger
* Capture of timer value into RA/RB
* Compare to RC register
* Overflow

Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
---

Notes:
    New in v2
    Changes in v3:
    * Add IRQs for Capture events (from next patch)
    * Add IRQ for RC Compare
    * Add events as bullet points to commit msg
    Changes in v4:
    * Add uapi header, names for the event channels
    * Add check for -EPROBE_DEFER from `of_irq_get()`

 MAINTAINERS                                   |  1 +
 drivers/counter/microchip-tcb-capture.c       | 75 +++++++++++++++++++
 .../linux/counter/microchip-tcb-capture.h     | 40 ++++++++++
 3 files changed, 116 insertions(+)
 create mode 100644 include/uapi/linux/counter/microchip-tcb-capture.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 8e047e20fbd8..0a8ce24ff467 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15579,6 +15579,7 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	drivers/counter/microchip-tcb-capture.c
+F:	include/uapi/linux/counter/microchip-tcb-capture.c
 
 MICROCHIP USB251XB DRIVER
 M:	Richard Leitner <richard.leitner@skidata.com>
diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
index 2f096a5b973d..cc12c2e2113a 100644
--- a/drivers/counter/microchip-tcb-capture.c
+++ b/drivers/counter/microchip-tcb-capture.c
@@ -6,18 +6,24 @@
  */
 #include <linux/clk.h>
 #include <linux/counter.h>
+#include <linux/interrupt.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <uapi/linux/counter/microchip-tcb-capture.h>
 #include <soc/at91/atmel_tcb.h>
 
 #define ATMEL_TC_CMR_MASK	(ATMEL_TC_LDRA_RISING | ATMEL_TC_LDRB_FALLING | \
 				 ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_LDBDIS | \
 				 ATMEL_TC_LDBSTOP)
 
+#define ATMEL_TC_DEF_IRQS	(ATMEL_TC_ETRGS | ATMEL_TC_COVFS | \
+				 ATMEL_TC_LDRAS | ATMEL_TC_LDRBS | ATMEL_TC_CPCS)
+
 #define ATMEL_TC_QDEN			BIT(8)
 #define ATMEL_TC_POSEN			BIT(9)
 
@@ -27,6 +33,7 @@ struct mchp_tc_data {
 	int qdec_mode;
 	int num_channels;
 	int channel[2];
+	int irq;
 };
 
 static const enum counter_function mchp_tc_count_functions[] = {
@@ -294,6 +301,65 @@ static const struct of_device_id atmel_tc_of_match[] = {
 	{ /* sentinel */ }
 };
 
+static irqreturn_t mchp_tc_isr(int irq, void *dev_id)
+{
+	struct counter_device *const counter = dev_id;
+	struct mchp_tc_data *const priv = counter_priv(counter);
+	u32 sr, mask;
+
+	regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], SR), &sr);
+	regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], IMR), &mask);
+
+	sr &= mask;
+	if (!(sr & ATMEL_TC_ALL_IRQ))
+		return IRQ_NONE;
+
+	if (sr & ATMEL_TC_ETRGS)
+		counter_push_event(counter, COUNTER_EVENT_CHANGE_OF_STATE,
+				   COUNTER_MCHP_EVCHN_CV);
+	if (sr & ATMEL_TC_LDRAS)
+		counter_push_event(counter, COUNTER_EVENT_CAPTURE,
+				   COUNTER_MCHP_EVCHN_RA);
+	if (sr & ATMEL_TC_LDRBS)
+		counter_push_event(counter, COUNTER_EVENT_CAPTURE,
+				   COUNTER_MCHP_EVCHN_RB);
+	if (sr & ATMEL_TC_CPCS)
+		counter_push_event(counter, COUNTER_EVENT_THRESHOLD,
+				   COUNTER_MCHP_EVCHN_RC);
+	if (sr & ATMEL_TC_COVFS)
+		counter_push_event(counter, COUNTER_EVENT_OVERFLOW,
+				   COUNTER_MCHP_EVCHN_CV);
+
+	return IRQ_HANDLED;
+}
+
+static void mchp_tc_irq_remove(void *ptr)
+{
+	struct mchp_tc_data *priv = ptr;
+
+	regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], IDR), ATMEL_TC_DEF_IRQS);
+}
+
+static int mchp_tc_irq_enable(struct counter_device *const counter)
+{
+	struct mchp_tc_data *const priv = counter_priv(counter);
+	int ret = devm_request_irq(counter->parent, priv->irq, mchp_tc_isr, 0,
+				   dev_name(counter->parent), counter);
+
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], IER), ATMEL_TC_DEF_IRQS);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_add_action_or_reset(counter->parent, mchp_tc_irq_remove, priv);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static void mchp_tc_clk_remove(void *ptr)
 {
 	clk_disable_unprepare((struct clk *)ptr);
@@ -378,6 +444,15 @@ static int mchp_tc_probe(struct platform_device *pdev)
 	counter->num_signals = ARRAY_SIZE(mchp_tc_count_signals);
 	counter->signals = mchp_tc_count_signals;
 
+	priv->irq = of_irq_get(np->parent, 0);
+	if (priv->irq == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+	if (priv->irq > 0) {
+		ret = mchp_tc_irq_enable(counter);
+		if (ret < 0)
+			return dev_err_probe(&pdev->dev, ret, "Failed to set up IRQ");
+	}
+
 	ret = devm_counter_add(&pdev->dev, counter);
 	if (ret < 0)
 		return dev_err_probe(&pdev->dev, ret, "Failed to add counter\n");
diff --git a/include/uapi/linux/counter/microchip-tcb-capture.h b/include/uapi/linux/counter/microchip-tcb-capture.h
new file mode 100644
index 000000000000..b91d8954f06e
--- /dev/null
+++ b/include/uapi/linux/counter/microchip-tcb-capture.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Channel numbers used by the microchip-tcb-capture driver
+ * Copyright (C) 2025 Bence Csókás
+ */
+#ifndef _UAPI_COUNTER_MCHP_TCB_H_
+#define _UAPI_COUNTER_MCHP_TCB_H_
+
+/*
+ * The driver defines the following components:
+ *
+ * Count 0
+ * \__  Synapse 0 -- Signal 0 (Channel A, i.e. TIOA)
+ * \__  Synapse 1 -- Signal 1 (Channel B, i.e. TIOB)
+ *
+ * It also supports the following events:
+ *
+ * Channel 0:
+ * - CV register changed
+ * - CV overflowed
+ * - RA captured
+ * Channel 1:
+ * - RB captured
+ * Channel 2:
+ * - RC compare triggered
+ */
+
+enum counter_mchp_signals {
+	COUNTER_MCHP_SIG_TIOA,
+	COUNTER_MCHP_SIG_TIOB,
+};
+
+enum counter_mchp_event_channels {
+	COUNTER_MCHP_EVCHN_CV = 0,
+	COUNTER_MCHP_EVCHN_RA = 0,
+	COUNTER_MCHP_EVCHN_RB,
+	COUNTER_MCHP_EVCHN_RC,
+};
+
+#endif /* _UAPI_COUNTER_MCHP_TCB_H_ */
-- 
2.48.1

[PATCH v4 2/2] counter: microchip-tcb-capture: Add capture extensions for registers RA-RC

[Bence Csókás]

TCB hardware is capable of capturing the timer value to registers RA and
RB. On top, it is capable of triggering on compare against a third
register, RC. Add these registers as extensions.

Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
---

Notes:
    Changes in v2:
    * Add IRQs
    Changes in v3:
    * Move IRQs to previous patch
    Changes in v4:
    * Return the status of the regmap_*() operations
    * Add names for the extension numbers

 drivers/counter/microchip-tcb-capture.c       | 62 +++++++++++++++++++
 .../linux/counter/microchip-tcb-capture.h     |  9 +++
 2 files changed, 71 insertions(+)

diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
index cc12c2e2113a..369f69aaf14f 100644
--- a/drivers/counter/microchip-tcb-capture.c
+++ b/drivers/counter/microchip-tcb-capture.c
@@ -254,6 +254,66 @@ static int mchp_tc_count_read(struct counter_device *counter,
 	return 0;
 }
 
+static int mchp_tc_count_cap_read(struct counter_device *counter,
+				  struct counter_count *count, size_t idx, u64 *val)
+{
+	struct mchp_tc_data *const priv = counter_priv(counter);
+	u32 cnt;
+	int ret;
+
+	switch (idx) {
+	case COUNTER_MCHP_EXCAP_RA:
+		ret = regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RA), &cnt);
+		break;
+	case COUNTER_MCHP_EXCAP_RB:
+		ret = regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RB), &cnt);
+		break;
+	case COUNTER_MCHP_EXCAP_RC:
+		ret = regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RC), &cnt);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (!ret)
+		*val = cnt;
+
+	return ret;
+}
+
+static int mchp_tc_count_cap_write(struct counter_device *counter,
+				   struct counter_count *count, size_t idx, u64 val)
+{
+	struct mchp_tc_data *const priv = counter_priv(counter);
+	int ret;
+
+	if (val > U32_MAX)
+		return -ERANGE;
+
+	switch (idx) {
+	case COUNTER_MCHP_EXCAP_RA:
+		ret = regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], RA), val);
+		break;
+	case COUNTER_MCHP_EXCAP_RB:
+		ret = regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], RB), val);
+		break;
+	case COUNTER_MCHP_EXCAP_RC:
+		ret = regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], RC), val);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static DEFINE_COUNTER_ARRAY_CAPTURE(mchp_tc_cnt_cap_array, 3);
+
+static struct counter_comp mchp_tc_count_ext[] = {
+	COUNTER_COMP_ARRAY_CAPTURE(mchp_tc_count_cap_read, mchp_tc_count_cap_write,
+				   mchp_tc_cnt_cap_array),
+};
+
 static struct counter_count mchp_tc_counts[] = {
 	{
 		.id = 0,
@@ -262,6 +322,8 @@ static struct counter_count mchp_tc_counts[] = {
 		.num_functions = ARRAY_SIZE(mchp_tc_count_functions),
 		.synapses = mchp_tc_count_synapses,
 		.num_synapses = ARRAY_SIZE(mchp_tc_count_synapses),
+		.ext = mchp_tc_count_ext,
+		.num_ext = ARRAY_SIZE(mchp_tc_count_ext),
 	},
 };
 
diff --git a/include/uapi/linux/counter/microchip-tcb-capture.h b/include/uapi/linux/counter/microchip-tcb-capture.h
index b91d8954f06e..0bef1c9d1535 100644
--- a/include/uapi/linux/counter/microchip-tcb-capture.h
+++ b/include/uapi/linux/counter/microchip-tcb-capture.h
@@ -12,6 +12,9 @@
  * Count 0
  * \__  Synapse 0 -- Signal 0 (Channel A, i.e. TIOA)
  * \__  Synapse 1 -- Signal 1 (Channel B, i.e. TIOB)
+ * \__  Extension capture0    (RA register)
+ * \__  Extension capture1    (RB register)
+ * \__  Extension capture2    (RC register)
  * 
  * It also supports the following events:
  *
@@ -30,6 +33,12 @@ enum counter_mchp_signals {
 	COUNTER_MCHP_SIG_TIOB,
 };
 
+enum counter_mchp_capture_extensions {
+	COUNTER_MCHP_EXCAP_RA,
+	COUNTER_MCHP_EXCAP_RB,
+	COUNTER_MCHP_EXCAP_RC,
+};
+
 enum counter_mchp_event_channels {
 	COUNTER_MCHP_EVCHN_CV = 0,
 	COUNTER_MCHP_EVCHN_RA = 0,
-- 
2.48.1

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[William Breathitt Gray]

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

On Tue, Feb 11, 2025 at 04:19:11PM +0100, Bence Csókás wrote:
> The TCB has three R/W-able "general purpose" hardware registers:
> RA, RB and RC. The hardware is capable of:
> * sampling Counter Value Register (CV) to RA/RB on a trigger edge
> * sending an interrupt of this change
> * sending an interrupt on CV change due to trigger
> * triggering an interrupt on CV compare to RC
> * stop counting after sampling to RB
> 
> To enable using these features in user-space, an interrupt handler
> was added, generating the necessary counter events. On top, RA/B/C
> registers are added as Count Extensions. To aid interoperation, a
> uapi header was also added, containing the various numeral IDs of
> the Extensions, Event channels etc.
> 
> Bence Csókás (2):
>   counter: microchip-tcb-capture: Add IRQ handling
>   counter: microchip-tcb-capture: Add capture extensions for registers
>     RA-RC
> 
>  MAINTAINERS                                   |   1 +
>  drivers/counter/microchip-tcb-capture.c       | 137 ++++++++++++++++++
>  .../linux/counter/microchip-tcb-capture.h     |  49 +++++++
>  3 files changed, 187 insertions(+)
>  create mode 100644 include/uapi/linux/counter/microchip-tcb-capture.h

Hi Bence,

I had some time to read over your description of the three hardware
registers (RA, RB, and RC)[^1] and I have some suggestions.

First, register RC seems to serve only as a threshold value for a
compare operation. So it shouldn't be exposed as "capture2", but rather
as its own dedicated threshold component. I think the 104-quad-8 module
is the only other driver supporting THRESHOLD events; it exposes the
threshold value configuration via the "preset" component, but perhaps we
should introduce a proper "threshold" component instead so counter
drivers have a standard way to expose this functionality. What do you
think?

Regarding registers RA and RB, these do hold historic captures of count
data so it does seem appropriate to expose these as "capture0" and
"capture1". However, I'm still somewhat confused about why there are two
registers holding the same sampled CV (or do RA and RB hold different
values from each other?). Does a single external line trigger the sample
of CV to both RA and RB, or are there two separate external lines
triggering the samples independently; or is this a situation where it's
a single external line where rising edge triggers a sample to RA while
falling edge triggers a sample to RB?

Next, the driver right now has three separate event channels, but I
believe you only need two. The purpose of counter event channels is to
provide a way for users to differentiate between the same type of event
being issued from different sources. An example might clarify what I
mean.

Suppose a hypothetical counter device has two independent timers. When
either timer overflows, the device fires off a single interrupt. We can
consider this interrupt a counter OVERFLOW event. As users we can watch
for COUNTER_EVENT_OVERFLOW to collect these events. However, a problem
arises: we know an OVERFLOW event occurred, but we don't know which
particular timer is the source of the overflow. The solution is to
dedicate each source to its own event channel so that users can
differentiate between them (e.g. channel 0 are events sourced from the
first timer whereas channel 1 are events sourced from the second timer).

Going back to your driver, there seems to be no ambiguity about the
source of the CHANGE-OF-STATE, THRESHOLD, and OVERFLOW events, so these
events can all coexist in the same event channel. The only possible
ambiguity I see is regarding the CAPTURE events, which could be sourced
by either a sample to RA or RB. Given this, I believe all your events
could go in channel 0, with channel 1 serving to simply differentiate
CAPTURE events that are sourced by samples to RB.

Finally, you mentioned that this device supports a PWM mode that also
makes use of the RA, RB, and RC registers. To prevent globbering the
registers when in the wrong mode, you should verify the device is in the
counter capture mode before handling the capture components (or return
an appropriate "Operation not support" error code when in PWM mode). You
don't need to worry about adding these checks for now because it looks
like this driver does not support PWM mode yet, but it's something to
keep in mind if you do add support for it in the future.

William Breathitt Gray

[^1] https://lore.kernel.org/all/7b581014-a351-4077-8832-d3d347b4fdb5@prolan.hu/

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

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[Csókás Bence]

Hi William,

On 2025. 02. 21. 13:39, William Breathitt Gray wrote:
> On Tue, Feb 11, 2025 at 04:19:11PM +0100, Bence Csókás wrote:
>> The TCB has three R/W-able "general purpose" hardware registers:
>> RA, RB and RC. The hardware is capable of:
>> * sampling Counter Value Register (CV) to RA/RB on a trigger edge
>> * sending an interrupt of this change
>> * sending an interrupt on CV change due to trigger
>> * triggering an interrupt on CV compare to RC
>> * stop counting after sampling to RB
>>
>> To enable using these features in user-space, an interrupt handler
>> was added, generating the necessary counter events. On top, RA/B/C
>> registers are added as Count Extensions. To aid interoperation, a
>> uapi header was also added, containing the various numeral IDs of
>> the Extensions, Event channels etc.
>>
>> Bence Csókás (2):
>>    counter: microchip-tcb-capture: Add IRQ handling
>>    counter: microchip-tcb-capture: Add capture extensions for registers
>>      RA-RC
>>
>>   MAINTAINERS                                   |   1 +
>>   drivers/counter/microchip-tcb-capture.c       | 137 ++++++++++++++++++
>>   .../linux/counter/microchip-tcb-capture.h     |  49 +++++++
>>   3 files changed, 187 insertions(+)
>>   create mode 100644 include/uapi/linux/counter/microchip-tcb-capture.h
> 
> Hi Bence,
> 
> I had some time to read over your description of the three hardware
> registers (RA, RB, and RC)[^1] and I have some suggestions.
> 
> First, register RC seems to serve only as a threshold value for a
> compare operation. So it shouldn't be exposed as "capture2", but rather
> as its own dedicated threshold component. I think the 104-quad-8 module
> is the only other driver supporting THRESHOLD events; it exposes the
> threshold value configuration via the "preset" component, but perhaps we
> should introduce a proper "threshold" component instead so counter
> drivers have a standard way to expose this functionality. What do you
> think?

Possibly. What's the semantics of the `preset` component BTW? If we can 
re-use that here as well, that could work too.

> Regarding registers RA and RB, these do hold historic captures of count
> data so it does seem appropriate to expose these as "capture0" and
> "capture1". However, I'm still somewhat confused about why there are two
> registers holding the same sampled CV (or do RA and RB hold different
> values from each other?). Does a single external line trigger the sample
> of CV to both RA and RB, or are there two separate external lines
> triggering the samples independently; or is this a situation where it's
> a single external line where rising edge triggers a sample to RA while
> falling edge triggers a sample to RB?

It is exactly the latter. And you can configure which edge should sample 
which register; you can also set them to the same edge in which case 
they would (presumably) hold the same value, but as you said, it 
wouldn't be practical.

> Next, the driver right now has three separate event channels, but I
> believe you only need two. The purpose of counter event channels is to
> provide a way for users to differentiate between the same type of event
> being issued from different sources. An example might clarify what I
> mean.

Yeah true, I could shove the RC compare event to event channel 0. It 
just made more sense to have event channels 0, 1, 2 correspond to RA, RB 
and RC. And it's not like we're short on channels, it's a u8, and we 
have 5 events; if we wanted to, we could give each a channel and still 
have plenty left over. I also thought about separating RA capture from 
channel 0, but I figured it would be fine, as you said, the event type 
would differentiate among them.

The reason I did not put the RC compare event to channel 0 as well is 
that I only have the SAMA5D2, and I don't know whether other parts are 
capable of generating other events related to RC, potentially clashing 
with other channel 0 use, if we later decide to support them. One 
channel per event-sourcing register seems like a safe bet; having CV and 
RA on the same channel still seems to be an acceptable compromise (but 
again, I don't know about the other parts' capabilities, so it _might_ 
still lead to clashes).

> Suppose a hypothetical counter device has two independent timers. When
> either timer overflows, the device fires off a single interrupt. We can
> consider this interrupt a counter OVERFLOW event. As users we can watch
> for COUNTER_EVENT_OVERFLOW to collect these events. However, a problem
> arises: we know an OVERFLOW event occurred, but we don't know which
> particular timer is the source of the overflow. The solution is to
> dedicate each source to its own event channel so that users can
> differentiate between them (e.g. channel 0 are events sourced from the
> first timer whereas channel 1 are events sourced from the second timer).
> 
> Going back to your driver, there seems to be no ambiguity about the
> source of the CHANGE-OF-STATE, THRESHOLD, and OVERFLOW events, so these
> events can all coexist in the same event channel. The only possible
> ambiguity I see is regarding the CAPTURE events, which could be sourced
> by either a sample to RA or RB. Given this, I believe all your events
> could go in channel 0, with channel 1 serving to simply differentiate
> CAPTURE events that are sourced by samples to RB.
> 
> Finally, you mentioned that this device supports a PWM mode that also
> makes use of the RA, RB, and RC registers. To prevent globbering the
> registers when in the wrong mode, you should verify the device is in the
> counter capture mode before handling the capture components (or return
> an appropriate "Operation not support" error code when in PWM mode). You
> don't need to worry about adding these checks for now because it looks
> like this driver does not support PWM mode yet, but it's something to
> keep in mind if you do add support for it in the future.

The `mchp_tc_count_function_write()` function already disables PWM mode 
by clearing the `ATMEL_TC_WAVE` bit from the Channel Mode Register (CMR).

> William Breathitt Gray
> 
> [^1] https://lore.kernel.org/all/7b581014-a351-4077-8832-d3d347b4fdb5@prolan.hu/

Bence

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[William Breathitt Gray]

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

On Fri, Feb 21, 2025 at 03:14:44PM +0100, Csókás Bence wrote:
> On 2025. 02. 21. 13:39, William Breathitt Gray wrote:
> > First, register RC seems to serve only as a threshold value for a
> > compare operation. So it shouldn't be exposed as "capture2", but rather
> > as its own dedicated threshold component. I think the 104-quad-8 module
> > is the only other driver supporting THRESHOLD events; it exposes the
> > threshold value configuration via the "preset" component, but perhaps we
> > should introduce a proper "threshold" component instead so counter
> > drivers have a standard way to expose this functionality. What do you
> > think?
> 
> Possibly. What's the semantics of the `preset` component BTW? If we can
> re-use that here as well, that could work too.

You can find the semantics of each attribute under the sysfs ABI doc
file located at Documentation/ABI/testing/sysfs-bus-counter. For the
`preset` component, its essential purpose is to configure a value to
preset register to reload the Count when some condition is met (e.g.
when an external INDEX/SYNC trigger line goes high).

In the 104-quad-8 module,  the `preset` component is used for a number
of threshold-like purposes; you can see some of the uses by reading the
description of the /sys/bus/counter/devices/counterX/countY/count_mode
attribute. Of relevance to the SAMA5D2, the 104-quad-8 has a mode where
the current Count value is compared against the preset register and an
interrupt is fired when they match (i.e. our Counter THRESHOLD event).

I think it'll be fine to use the preset component to expose the RC
register because the 104-quad-8 already uses the preset component in a
similar fashion; also I would like to keep things simple in this
patchset to avoid the complexities of introducing a new `threshold`
component interface, at least until we get the rest of the capture
functionality in this driver merged.

So for now, use COUNTER_COMP_PRESET() to expose the RC register as a
`preset` component. In the next patchset revision, put this code in its
own patch after the capture components patch, so that we can review the
RC register code separately.

In the same vein, move the uapi header introduction to its own patch.
That will separate the userspace-exposed changes and make things easier
for future users when bisecting the linux kernel history when tracking
down possible bugs.
 
> > Regarding registers RA and RB, these do hold historic captures of count
> > data so it does seem appropriate to expose these as "capture0" and
> > "capture1". However, I'm still somewhat confused about why there are two
> > registers holding the same sampled CV (or do RA and RB hold different
> > values from each other?). Does a single external line trigger the sample
> > of CV to both RA and RB, or are there two separate external lines
> > triggering the samples independently; or is this a situation where it's
> > a single external line where rising edge triggers a sample to RA while
> > falling edge triggers a sample to RB?
> 
> It is exactly the latter. And you can configure which edge should sample
> which register; you can also set them to the same edge in which case they
> would (presumably) hold the same value, but as you said, it wouldn't be
> practical.

Ah okay, I think I understand now. Thank you for clarifying.

> > Next, the driver right now has three separate event channels, but I
> > believe you only need two. The purpose of counter event channels is to
> > provide a way for users to differentiate between the same type of event
> > being issued from different sources. An example might clarify what I
> > mean.
> 
> Yeah true, I could shove the RC compare event to event channel 0. It just
> made more sense to have event channels 0, 1, 2 correspond to RA, RB and RC.
> And it's not like we're short on channels, it's a u8, and we have 5 events;
> if we wanted to, we could give each a channel and still have plenty left
> over. I also thought about separating RA capture from channel 0, but I
> figured it would be fine, as you said, the event type would differentiate
> among them.
> 
> The reason I did not put the RC compare event to channel 0 as well is that I
> only have the SAMA5D2, and I don't know whether other parts are capable of
> generating other events related to RC, potentially clashing with other
> channel 0 use, if we later decide to support them. One channel per
> event-sourcing register seems like a safe bet; having CV and RA on the same
> channel still seems to be an acceptable compromise (but again, I don't know
> about the other parts' capabilities, so it _might_ still lead to clashes).

I can see your rationale, and I don't have too strong of an opinion
either way, so I'll leave it up to your best judgement. Ultimately, as
you point out we do have a large enough availability of channels that
will accommodate future introductions, which can be abstracted to users
via the uapi header, so we should be fine.

Regarding future additions, I took a look at the Microchip SAMA5D2
datasheet[^1] (is the right document?) and it looks like this chip has
three timer counter channels described in section 54. Currently, the
microchip-tcb-capture module is exposing only one timer counter channel
(as Count0), correct? Should this driver expose all three channels (as
Count0, Count1, and Count2)?

> > Suppose a hypothetical counter device has two independent timers. When
> > either timer overflows, the device fires off a single interrupt. We can
> > consider this interrupt a counter OVERFLOW event. As users we can watch
> > for COUNTER_EVENT_OVERFLOW to collect these events. However, a problem
> > arises: we know an OVERFLOW event occurred, but we don't know which
> > particular timer is the source of the overflow. The solution is to
> > dedicate each source to its own event channel so that users can
> > differentiate between them (e.g. channel 0 are events sourced from the
> > first timer whereas channel 1 are events sourced from the second timer).
> > 
> > Going back to your driver, there seems to be no ambiguity about the
> > source of the CHANGE-OF-STATE, THRESHOLD, and OVERFLOW events, so these
> > events can all coexist in the same event channel. The only possible
> > ambiguity I see is regarding the CAPTURE events, which could be sourced
> > by either a sample to RA or RB. Given this, I believe all your events
> > could go in channel 0, with channel 1 serving to simply differentiate
> > CAPTURE events that are sourced by samples to RB.
> > 
> > Finally, you mentioned that this device supports a PWM mode that also
> > makes use of the RA, RB, and RC registers. To prevent globbering the
> > registers when in the wrong mode, you should verify the device is in the
> > counter capture mode before handling the capture components (or return
> > an appropriate "Operation not support" error code when in PWM mode). You
> > don't need to worry about adding these checks for now because it looks
> > like this driver does not support PWM mode yet, but it's something to
> > keep in mind if you do add support for it in the future.
> 
> The `mchp_tc_count_function_write()` function already disables PWM mode by
> clearing the `ATMEL_TC_WAVE` bit from the Channel Mode Register (CMR).

So capture mode is unconditionally set by mchp_tc_count_function_write()
which means the first time the user sets the Count function then PWM
mode will be disabled. However, what happens if the user does not set
the Count function? Should PWM mode be disabled by default in
mchp_tc_probe(), or does that already happen?

William Breathitt Gray

[^1] https://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf

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

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[Csókás Bence]

Hi,

On 2025. 02. 24. 4:07, William Breathitt Gray wrote:
> On Fri, Feb 21, 2025 at 03:14:44PM +0100, Csókás Bence wrote:
>> On 2025. 02. 21. 13:39, William Breathitt Gray wrote:
>>> First, register RC seems to serve only as a threshold value for a
>>> compare operation. So it shouldn't be exposed as "capture2", but rather
>>> as its own dedicated threshold component. I think the 104-quad-8 module
>>> is the only other driver supporting THRESHOLD events; it exposes the
>>> threshold value configuration via the "preset" component, but perhaps we
>>> should introduce a proper "threshold" component instead so counter
>>> drivers have a standard way to expose this functionality. What do you
>>> think?
>>
>> Possibly. What's the semantics of the `preset` component BTW? If we can
>> re-use that here as well, that could work too.
> 
> You can find the semantics of each attribute under the sysfs ABI doc
> file located at Documentation/ABI/testing/sysfs-bus-counter. For the
> `preset` component, its essential purpose is to configure a value to
> preset register to reload the Count when some condition is met (e.g.
> when an external INDEX/SYNC trigger line goes high).

Hmm, that doesn't really match this use case. All right, then, for now, 
I'll skip the RC part, and then we can add it in a later patch when the 
"threshold" component is in place and used by the 104-quad-8 module.

> In the same vein, move the uapi header introduction to its own patch.
> That will separate the userspace-exposed changes and make things easier
> for future users when bisecting the linux kernel history when tracking
> down possible bugs.

Isn't it better to keep API header changes in the same commit as the 
implementation using them? That way if someone bisects/blames the API 
header, they get the respective implementation as well.

> Regarding future additions, I took a look at the Microchip SAMA5D2
> datasheet[^1] (is the right document?)

There are many versions, but yes, it is one of them, and is usable.

> and it looks like this chip has
> three timer counter channels described in section 54. Currently, the
> microchip-tcb-capture module is exposing only one timer counter channel
> (as Count0), correct? Should this driver expose all three channels (as
> Count0, Count1, and Count2)?

No, as this device is actually instantiated per-channel, i.e. in the DT, 
there are two TCB nodes (as the SoC has two peripherals, each with 3 
channels), and then the counter is a sub-node with `reg = <0/1/2>`, 
specifying which timer channel to use. Or, in quadrature decode mode, 
you'd have two elements in `reg`, i.e. `reg = <0>, <1>`.

>> The `mchp_tc_count_function_write()` function already disables PWM mode by
>> clearing the `ATMEL_TC_WAVE` bit from the Channel Mode Register (CMR).
> 
> So capture mode is unconditionally set by mchp_tc_count_function_write()
> which means the first time the user sets the Count function then PWM
> mode will be disabled. However, what happens if the user does not set
> the Count function? Should PWM mode be disabled by default in
> mchp_tc_probe(), or does that already happen?

You're right, and it is a problem I encounter regularly: almost all HW 
initialization happens in `mchp_tc_count_function_write()`, the probe() 
function mostly just allocates stuff. Meaning, if you want to do 
anything with the counter, you have to set the "increase" function first 
(even though, if you `cat function`, it will seem like it's already in 
"increase" mode). I don't know if it was deliberate, or what, but again, 
that would be a separate bugfix patch.

Bence

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[William Breathitt Gray]

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

On Wed, Feb 26, 2025 at 01:58:37PM +0100, Csókás Bence wrote:
> On 2025. 02. 24. 4:07, William Breathitt Gray wrote:
> > On Fri, Feb 21, 2025 at 03:14:44PM +0100, Csókás Bence wrote:
> > > On 2025. 02. 21. 13:39, William Breathitt Gray wrote:
> > > > First, register RC seems to serve only as a threshold value for a
> > > > compare operation. So it shouldn't be exposed as "capture2", but rather
> > > > as its own dedicated threshold component. I think the 104-quad-8 module
> > > > is the only other driver supporting THRESHOLD events; it exposes the
> > > > threshold value configuration via the "preset" component, but perhaps we
> > > > should introduce a proper "threshold" component instead so counter
> > > > drivers have a standard way to expose this functionality. What do you
> > > > think?
> > > 
> > > Possibly. What's the semantics of the `preset` component BTW? If we can
> > > re-use that here as well, that could work too.
> > 
> > You can find the semantics of each attribute under the sysfs ABI doc
> > file located at Documentation/ABI/testing/sysfs-bus-counter. For the
> > `preset` component, its essential purpose is to configure a value to
> > preset register to reload the Count when some condition is met (e.g.
> > when an external INDEX/SYNC trigger line goes high).
> 
> Hmm, that doesn't really match this use case. All right, then, for now, I'll
> skip the RC part, and then we can add it in a later patch when the
> "threshold" component is in place and used by the 104-quad-8 module.

Understood, I'll work on a separate patchset introducing a "threshold"
component (perhaps "compare" is a better name) to the 104-quad-8 and
once that is complete we can add it to the microchip-tcb-capture as its
own patch to support the RC register functionality.

> > In the same vein, move the uapi header introduction to its own patch.
> > That will separate the userspace-exposed changes and make things easier
> > for future users when bisecting the linux kernel history when tracking
> > down possible bugs.
> 
> Isn't it better to keep API header changes in the same commit as the
> implementation using them? That way if someone bisects/blames the API
> header, they get the respective implementation as well.

Fair enough, we'll keep the header together with the implementation.

> > and it looks like this chip has
> > three timer counter channels described in section 54. Currently, the
> > microchip-tcb-capture module is exposing only one timer counter channel
> > (as Count0), correct? Should this driver expose all three channels (as
> > Count0, Count1, and Count2)?
> 
> No, as this device is actually instantiated per-channel, i.e. in the DT,
> there are two TCB nodes (as the SoC has two peripherals, each with 3
> channels), and then the counter is a sub-node with `reg = <0/1/2>`,
> specifying which timer channel to use. Or, in quadrature decode mode, you'd
> have two elements in `reg`, i.e. `reg = <0>, <1>`.

So right now each timer counter channel is exposed as an independent
Counter device? That means we're exposing the timer counter blocks
incorrectly.

You're not at fault Bence, so you don't need to address this problem
with your current patchset, but I do want to discuss it briefly here so
we can come up with a plan for how to resolve it for the future. The
Generic Counter Interface was nascent at the time, so we likely
overlooked this problem at the time. I'm CCing some of those present
during the original introduction of the microchip-tcb-capture driver so
they are aware of this discussion.

Let me make sure I understand the situation correctly. This SoC has two
Timer Counter Blocks (TCB) and each TCB has three Timer Counter Channels
(TCC); each TCC has a Counter Value (CV) and three general registers
(RA, RB, RC); RA and RB can store Captures, and RC can be used for
Compare operations.

If that is true, then the correct way for this hardware to be exposed is
to have each TCB be a Counter device where each TCC is exposed as a
Count. So for this SoC: two Counter devices as counter0 and counter1;
count0, count1, and count2 as the three TCC; i.e. counter0/count{0,1,2}
and counter1/count{0,1,2}.

With that setup, configurations that affect the entire TCB (e.g. Block
Mode Register) can be exposed as Counter device components. Furthermore,
this would allow users to set Counter watches to collect component
values for the other two Counts while triggering off of the events of
any particular one, which wouldn't be possible if each TCC is isolated
to its own Counter device as is the case right now.

Regardless, the three TCC of each TCB should be grouped together
logically as they can represent related values. For example,  when using
the quadrature decoder TTC0 CV can represent Speed/Position while TTC1
CV represents rotation, thus giving a high level of precision on motion
system position as the datasheet points out.

Kamel, what would it take for us to rectify this situation so that the
TCC are organized together by TCB under the same Counter devices?

> > > The `mchp_tc_count_function_write()` function already disables PWM mode by
> > > clearing the `ATMEL_TC_WAVE` bit from the Channel Mode Register (CMR).
> > 
> > So capture mode is unconditionally set by mchp_tc_count_function_write()
> > which means the first time the user sets the Count function then PWM
> > mode will be disabled. However, what happens if the user does not set
> > the Count function? Should PWM mode be disabled by default in
> > mchp_tc_probe(), or does that already happen?
> 
> You're right, and it is a problem I encounter regularly: almost all HW
> initialization happens in `mchp_tc_count_function_write()`, the probe()
> function mostly just allocates stuff. Meaning, if you want to do anything
> with the counter, you have to set the "increase" function first (even
> though, if you `cat function`, it will seem like it's already in "increase"
> mode). I don't know if it was deliberate, or what, but again, that would be
> a separate bugfix patch.

That does seem like an oversight that goes back to the original commit
106b104137fd ("counter: Add microchip TCB capture counter"). I'll submit
a bug fix patch later separately to address this and we can continue
discussions about the issue there.

William Breathitt Gray

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

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[Kamel Bouhara]

On Thu, Feb 27, 2025 at 01:59:57PM +0900, William Breathitt Gray wrote:
> On Wed, Feb 26, 2025 at 01:58:37PM +0100, Csókás Bence wrote:
> > On 2025. 02. 24. 4:07, William Breathitt Gray wrote:
> > > On Fri, Feb 21, 2025 at 03:14:44PM +0100, Csókás Bence wrote:
> > > > On 2025. 02. 21. 13:39, William Breathitt Gray wrote:
> > > > > First, register RC seems to serve only as a threshold value for a
> > > > > compare operation. So it shouldn't be exposed as "capture2", but rather
> > > > > as its own dedicated threshold component. I think the 104-quad-8 module
> > > > > is the only other driver supporting THRESHOLD events; it exposes the
> > > > > threshold value configuration via the "preset" component, but perhaps we
> > > > > should introduce a proper "threshold" component instead so counter
> > > > > drivers have a standard way to expose this functionality. What do you
> > > > > think?
> > > >
> > > > Possibly. What's the semantics of the `preset` component BTW? If we can
> > > > re-use that here as well, that could work too.
> > >
> > > You can find the semantics of each attribute under the sysfs ABI doc
> > > file located at Documentation/ABI/testing/sysfs-bus-counter. For the
> > > `preset` component, its essential purpose is to configure a value to
> > > preset register to reload the Count when some condition is met (e.g.
> > > when an external INDEX/SYNC trigger line goes high).
> >
> > Hmm, that doesn't really match this use case. All right, then, for now, I'll
> > skip the RC part, and then we can add it in a later patch when the
> > "threshold" component is in place and used by the 104-quad-8 module.
>
> Understood, I'll work on a separate patchset introducing a "threshold"
> component (perhaps "compare" is a better name) to the 104-quad-8 and
> once that is complete we can add it to the microchip-tcb-capture as its
> own patch to support the RC register functionality.
>
> > > In the same vein, move the uapi header introduction to its own patch.
> > > That will separate the userspace-exposed changes and make things easier
> > > for future users when bisecting the linux kernel history when tracking
> > > down possible bugs.
> >
> > Isn't it better to keep API header changes in the same commit as the
> > implementation using them? That way if someone bisects/blames the API
> > header, they get the respective implementation as well.
>
> Fair enough, we'll keep the header together with the implementation.
>
> > > and it looks like this chip has
> > > three timer counter channels described in section 54. Currently, the
> > > microchip-tcb-capture module is exposing only one timer counter channel
> > > (as Count0), correct? Should this driver expose all three channels (as
> > > Count0, Count1, and Count2)?
> >
> > No, as this device is actually instantiated per-channel, i.e. in the DT,
> > there are two TCB nodes (as the SoC has two peripherals, each with 3
> > channels), and then the counter is a sub-node with `reg = <0/1/2>`,
> > specifying which timer channel to use. Or, in quadrature decode mode, you'd
> > have two elements in `reg`, i.e. `reg = <0>, <1>`.
>
> So right now each timer counter channel is exposed as an independent
> Counter device? That means we're exposing the timer counter blocks
> incorrectly.
>
> You're not at fault Bence, so you don't need to address this problem
> with your current patchset, but I do want to discuss it briefly here so
> we can come up with a plan for how to resolve it for the future. The
> Generic Counter Interface was nascent at the time, so we likely
> overlooked this problem at the time. I'm CCing some of those present
> during the original introduction of the microchip-tcb-capture driver so
> they are aware of this discussion.
>
> Let me make sure I understand the situation correctly. This SoC has two
> Timer Counter Blocks (TCB) and each TCB has three Timer Counter Channels
> (TCC); each TCC has a Counter Value (CV) and three general registers
> (RA, RB, RC); RA and RB can store Captures, and RC can be used for
> Compare operations.
>
> If that is true, then the correct way for this hardware to be exposed is
> to have each TCB be a Counter device where each TCC is exposed as a
> Count. So for this SoC: two Counter devices as counter0 and counter1;
> count0, count1, and count2 as the three TCC; i.e. counter0/count{0,1,2}
> and counter1/count{0,1,2}.
>
> With that setup, configurations that affect the entire TCB (e.g. Block
> Mode Register) can be exposed as Counter device components. Furthermore,
> this would allow users to set Counter watches to collect component
> values for the other two Counts while triggering off of the events of
> any particular one, which wouldn't be possible if each TCC is isolated
> to its own Counter device as is the case right now.
>
> Regardless, the three TCC of each TCB should be grouped together
> logically as they can represent related values. For example,  when using
> the quadrature decoder TTC0 CV can represent Speed/Position while TTC1
> CV represents rotation, thus giving a high level of precision on motion
> system position as the datasheet points out.
>
> Kamel, what would it take for us to rectify this situation so that the
> TCC are organized together by TCB under the same Counter devices?

Hello,

Indeed, each TCC operates independently except when quadrature mode is
enabled. I assume this approach was taken to provide more flexibility by
exposing them separately.

Currently only one channel is configured this would need to rework the
driver to make the 3 TCCs exposed.

Greetings,
Kamel

>
> > > > The `mchp_tc_count_function_write()` function already disables PWM mode by
> > > > clearing the `ATMEL_TC_WAVE` bit from the Channel Mode Register (CMR).
> > >
> > > So capture mode is unconditionally set by mchp_tc_count_function_write()
> > > which means the first time the user sets the Count function then PWM
> > > mode will be disabled. However, what happens if the user does not set
> > > the Count function? Should PWM mode be disabled by default in
> > > mchp_tc_probe(), or does that already happen?
> >
> > You're right, and it is a problem I encounter regularly: almost all HW
> > initialization happens in `mchp_tc_count_function_write()`, the probe()
> > function mostly just allocates stuff. Meaning, if you want to do anything
> > with the counter, you have to set the "increase" function first (even
> > though, if you `cat function`, it will seem like it's already in "increase"
> > mode). I don't know if it was deliberate, or what, but again, that would be
> > a separate bugfix patch.
>
> That does seem like an oversight that goes back to the original commit
> 106b104137fd ("counter: Add microchip TCB capture counter"). I'll submit
> a bug fix patch later separately to address this and we can continue
> discussions about the issue there.
>
> William Breathitt Gray



--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[Kamel Bouhara]

On Thu, Feb 27, 2025 at 02:53:33PM +0100, Kamel Bouhara wrote:
> On Thu, Feb 27, 2025 at 01:59:57PM +0900, William Breathitt Gray wrote:
> > On Wed, Feb 26, 2025 at 01:58:37PM +0100, Csókás Bence wrote:
> > > On 2025. 02. 24. 4:07, William Breathitt Gray wrote:
> > > > On Fri, Feb 21, 2025 at 03:14:44PM +0100, Csókás Bence wrote:
> > > > > On 2025. 02. 21. 13:39, William Breathitt Gray wrote:
> > > > > > First, register RC seems to serve only as a threshold value for a
> > > > > > compare operation. So it shouldn't be exposed as "capture2", but rather
> > > > > > as its own dedicated threshold component. I think the 104-quad-8 module
> > > > > > is the only other driver supporting THRESHOLD events; it exposes the
> > > > > > threshold value configuration via the "preset" component, but perhaps we
> > > > > > should introduce a proper "threshold" component instead so counter
> > > > > > drivers have a standard way to expose this functionality. What do you
> > > > > > think?
> > > > >
> > > > > Possibly. What's the semantics of the `preset` component BTW? If we can
> > > > > re-use that here as well, that could work too.
> > > >
> > > > You can find the semantics of each attribute under the sysfs ABI doc
> > > > file located at Documentation/ABI/testing/sysfs-bus-counter. For the
> > > > `preset` component, its essential purpose is to configure a value to
> > > > preset register to reload the Count when some condition is met (e.g.
> > > > when an external INDEX/SYNC trigger line goes high).
> > >
> > > Hmm, that doesn't really match this use case. All right, then, for now, I'll
> > > skip the RC part, and then we can add it in a later patch when the
> > > "threshold" component is in place and used by the 104-quad-8 module.
> >
> > Understood, I'll work on a separate patchset introducing a "threshold"
> > component (perhaps "compare" is a better name) to the 104-quad-8 and
> > once that is complete we can add it to the microchip-tcb-capture as its
> > own patch to support the RC register functionality.
> >
> > > > In the same vein, move the uapi header introduction to its own patch.
> > > > That will separate the userspace-exposed changes and make things easier
> > > > for future users when bisecting the linux kernel history when tracking
> > > > down possible bugs.
> > >
> > > Isn't it better to keep API header changes in the same commit as the
> > > implementation using them? That way if someone bisects/blames the API
> > > header, they get the respective implementation as well.
> >
> > Fair enough, we'll keep the header together with the implementation.
> >
> > > > and it looks like this chip has
> > > > three timer counter channels described in section 54. Currently, the
> > > > microchip-tcb-capture module is exposing only one timer counter channel
> > > > (as Count0), correct? Should this driver expose all three channels (as
> > > > Count0, Count1, and Count2)?
> > >
> > > No, as this device is actually instantiated per-channel, i.e. in the DT,
> > > there are two TCB nodes (as the SoC has two peripherals, each with 3
> > > channels), and then the counter is a sub-node with `reg = <0/1/2>`,
> > > specifying which timer channel to use. Or, in quadrature decode mode, you'd
> > > have two elements in `reg`, i.e. `reg = <0>, <1>`.
> >
> > So right now each timer counter channel is exposed as an independent
> > Counter device? That means we're exposing the timer counter blocks
> > incorrectly.
> >
> > You're not at fault Bence, so you don't need to address this problem
> > with your current patchset, but I do want to discuss it briefly here so
> > we can come up with a plan for how to resolve it for the future. The
> > Generic Counter Interface was nascent at the time, so we likely
> > overlooked this problem at the time. I'm CCing some of those present
> > during the original introduction of the microchip-tcb-capture driver so
> > they are aware of this discussion.
> >
> > Let me make sure I understand the situation correctly. This SoC has two
> > Timer Counter Blocks (TCB) and each TCB has three Timer Counter Channels
> > (TCC); each TCC has a Counter Value (CV) and three general registers
> > (RA, RB, RC); RA and RB can store Captures, and RC can be used for
> > Compare operations.
> >
> > If that is true, then the correct way for this hardware to be exposed is
> > to have each TCB be a Counter device where each TCC is exposed as a
> > Count. So for this SoC: two Counter devices as counter0 and counter1;
> > count0, count1, and count2 as the three TCC; i.e. counter0/count{0,1,2}
> > and counter1/count{0,1,2}.
> >
> > With that setup, configurations that affect the entire TCB (e.g. Block
> > Mode Register) can be exposed as Counter device components. Furthermore,
> > this would allow users to set Counter watches to collect component
> > values for the other two Counts while triggering off of the events of
> > any particular one, which wouldn't be possible if each TCC is isolated
> > to its own Counter device as is the case right now.
> >
> > Regardless, the three TCC of each TCB should be grouped together
> > logically as they can represent related values. For example,  when using
> > the quadrature decoder TTC0 CV can represent Speed/Position while TTC1
> > CV represents rotation, thus giving a high level of precision on motion
> > system position as the datasheet points out.
> >
> > Kamel, what would it take for us to rectify this situation so that the
> > TCC are organized together by TCB under the same Counter devices?
>
> Hello,
>
> Indeed, each TCC operates independently except when quadrature mode is
> enabled. I assume this approach was taken to provide more flexibility by
> exposing them separately.
>
> Currently only one channel is configured this would need to rework the
> driver to make the 3 TCCs exposed.
>

One important point to consider is that each TCC can be configured
in Capture/QDEC, PWM or clocksource mode. We have written a blog post
that describes each modes in detail [1], which can help clarify how the
TCC works on Microchip devices.

> Greetings,
> Kamel
>
> >
> > > > > The `mchp_tc_count_function_write()` function already disables PWM mode by
> > > > > clearing the `ATMEL_TC_WAVE` bit from the Channel Mode Register (CMR).
> > > >
> > > > So capture mode is unconditionally set by mchp_tc_count_function_write()
> > > > which means the first time the user sets the Count function then PWM
> > > > mode will be disabled. However, what happens if the user does not set
> > > > the Count function? Should PWM mode be disabled by default in
> > > > mchp_tc_probe(), or does that already happen?
> > >
> > > You're right, and it is a problem I encounter regularly: almost all HW
> > > initialization happens in `mchp_tc_count_function_write()`, the probe()
> > > function mostly just allocates stuff. Meaning, if you want to do anything
> > > with the counter, you have to set the "increase" function first (even
> > > though, if you `cat function`, it will seem like it's already in "increase"
> > > mode). I don't know if it was deliberate, or what, but again, that would be
> > > a separate bugfix patch.
> >
> > That does seem like an oversight that goes back to the original commit
> > 106b104137fd ("counter: Add microchip TCB capture counter"). I'll submit
> > a bug fix patch later separately to address this and we can continue
> > discussions about the issue there.
> >
> > William Breathitt Gray
>
>
>
> --
> Kamel Bouhara, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com

--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[1]: https://bootlin.com/blog/timer-counters-linux-microchip/

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[William Breathitt Gray]

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

On Thu, Feb 27, 2025 at 02:53:30PM +0100, Kamel Bouhara wrote:
> On Thu, Feb 27, 2025 at 01:59:57PM +0900, William Breathitt Gray wrote:
> > Let me make sure I understand the situation correctly. This SoC has two
> > Timer Counter Blocks (TCB) and each TCB has three Timer Counter Channels
> > (TCC); each TCC has a Counter Value (CV) and three general registers
> > (RA, RB, RC); RA and RB can store Captures, and RC can be used for
> > Compare operations.
> >
> > If that is true, then the correct way for this hardware to be exposed is
> > to have each TCB be a Counter device where each TCC is exposed as a
> > Count. So for this SoC: two Counter devices as counter0 and counter1;
> > count0, count1, and count2 as the three TCC; i.e. counter0/count{0,1,2}
> > and counter1/count{0,1,2}.

[...]

> > Kamel, what would it take for us to rectify this situation so that the
> > TCC are organized together by TCB under the same Counter devices?
> 
> Hello,
> 
> Indeed, each TCC operates independently except when quadrature mode is
> enabled. I assume this approach was taken to provide more flexibility by
> exposing them separately.
> 
> Currently only one channel is configured this would need to rework the
> driver to make the 3 TCCs exposed.
> 
> Greetings,
> Kamel

Skimming through the driver, it looks like what we'll need is for
mchp_tc_counts[] to have all three TCCs defined, then have
mchp_tc_probe() match on a TCB node and configure each TCC. Once that's
setup, then whenever we need to identify which TCC a callback is
exposing, we can get it from count->id.

So for example, the TC_CV register offset is calculated as 0x00 +
channel * 0x40 + 0x10. In the count_read() callback we can leverage
count->id to identify the TCC and thus get the respective TC_CV register
at offset + count->id * 0x40 + 0x10.

William Breathitt Gray

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

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[Alexandre Belloni]

On 27/02/2025 23:22:36+0900, William Breathitt Gray wrote:
> On Thu, Feb 27, 2025 at 02:53:30PM +0100, Kamel Bouhara wrote:
> > On Thu, Feb 27, 2025 at 01:59:57PM +0900, William Breathitt Gray wrote:
> > > Let me make sure I understand the situation correctly. This SoC has two
> > > Timer Counter Blocks (TCB) and each TCB has three Timer Counter Channels
> > > (TCC); each TCC has a Counter Value (CV) and three general registers
> > > (RA, RB, RC); RA and RB can store Captures, and RC can be used for
> > > Compare operations.
> > >
> > > If that is true, then the correct way for this hardware to be exposed is
> > > to have each TCB be a Counter device where each TCC is exposed as a
> > > Count. So for this SoC: two Counter devices as counter0 and counter1;
> > > count0, count1, and count2 as the three TCC; i.e. counter0/count{0,1,2}
> > > and counter1/count{0,1,2}.
> 
> [...]
> 
> > > Kamel, what would it take for us to rectify this situation so that the
> > > TCC are organized together by TCB under the same Counter devices?
> > 
> > Hello,
> > 
> > Indeed, each TCC operates independently except when quadrature mode is
> > enabled. I assume this approach was taken to provide more flexibility by
> > exposing them separately.
> > 
> > Currently only one channel is configured this would need to rework the
> > driver to make the 3 TCCs exposed.
> > 
> > Greetings,
> > Kamel
> 
> Skimming through the driver, it looks like what we'll need is for
> mchp_tc_counts[] to have all three TCCs defined, then have
> mchp_tc_probe() match on a TCB node and configure each TCC. Once that's
> setup, then whenever we need to identify which TCC a callback is
> exposing, we can get it from count->id.
> 
> So for example, the TC_CV register offset is calculated as 0x00 +
> channel * 0x40 + 0x10. In the count_read() callback we can leverage
> count->id to identify the TCC and thus get the respective TC_CV register
> at offset + count->id * 0x40 + 0x10.
> 

We can't do that because the TCC of a single TCB can have a mix of
different features. I struggled with the breakage to move away from the
one TCB, one feature state we had.
Be fore this, it was not possible to mix features on a single TCB, now,
we can have e.g. the clocksource on TCC 0 and 1 of TCB0 and a PWM on
TCC 2. mchp_tc_probe must not match on a TCB node...

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[William Breathitt Gray]

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

On Thu, Feb 27, 2025 at 03:37:28PM +0100, Alexandre Belloni wrote:
> On 27/02/2025 23:22:36+0900, William Breathitt Gray wrote:
> > On Thu, Feb 27, 2025 at 02:53:30PM +0100, Kamel Bouhara wrote:
> > > On Thu, Feb 27, 2025 at 01:59:57PM +0900, William Breathitt Gray wrote:
> > > > Let me make sure I understand the situation correctly. This SoC has two
> > > > Timer Counter Blocks (TCB) and each TCB has three Timer Counter Channels
> > > > (TCC); each TCC has a Counter Value (CV) and three general registers
> > > > (RA, RB, RC); RA and RB can store Captures, and RC can be used for
> > > > Compare operations.
> > > >
> > > > If that is true, then the correct way for this hardware to be exposed is
> > > > to have each TCB be a Counter device where each TCC is exposed as a
> > > > Count. So for this SoC: two Counter devices as counter0 and counter1;
> > > > count0, count1, and count2 as the three TCC; i.e. counter0/count{0,1,2}
> > > > and counter1/count{0,1,2}.
> > 
> > [...]
> > 
> > > > Kamel, what would it take for us to rectify this situation so that the
> > > > TCC are organized together by TCB under the same Counter devices?
> > > 
> > > Hello,
> > > 
> > > Indeed, each TCC operates independently except when quadrature mode is
> > > enabled. I assume this approach was taken to provide more flexibility by
> > > exposing them separately.
> > > 
> > > Currently only one channel is configured this would need to rework the
> > > driver to make the 3 TCCs exposed.
> > > 
> > > Greetings,
> > > Kamel
> > 
> > Skimming through the driver, it looks like what we'll need is for
> > mchp_tc_counts[] to have all three TCCs defined, then have
> > mchp_tc_probe() match on a TCB node and configure each TCC. Once that's
> > setup, then whenever we need to identify which TCC a callback is
> > exposing, we can get it from count->id.
> > 
> > So for example, the TC_CV register offset is calculated as 0x00 +
> > channel * 0x40 + 0x10. In the count_read() callback we can leverage
> > count->id to identify the TCC and thus get the respective TC_CV register
> > at offset + count->id * 0x40 + 0x10.
> > 
> 
> We can't do that because the TCC of a single TCB can have a mix of
> different features. I struggled with the breakage to move away from the
> one TCB, one feature state we had.
> Be fore this, it was not possible to mix features on a single TCB, now,
> we can have e.g. the clocksource on TCC 0 and 1 of TCB0 and a PWM on
> TCC 2. mchp_tc_probe must not match on a TCB node...

Okay I see what you mean, if we match on a TCB mode then we wouldn't be
able to define the cases where one TCC is different from the next in the
same TCB.

The goal however isn't to support all functionality (i.e. PWM-related
settings, etc.) in the counter driver, but just expose the TCB
configuration options that affect the TCCs when configured for counter
mode. For example, the sysfs attributes can be created, but they don't
have to be available until the TCC is in the appropriate mode (e.g.
return -EBUSY until they are in a counter mode).

Is there a way to achieve that? Maybe there's a way we can populate the
sysfs tree on the first encountered TCC, and then somehow indicate when
additional TCCs match. Attributes can become available then dynamically
based on the TCCs that match.

William Breathitt Gray

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

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[William Breathitt Gray]

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

On Fri, Feb 28, 2025 at 12:13:00AM +0900, William Breathitt Gray wrote:
> On Thu, Feb 27, 2025 at 03:37:28PM +0100, Alexandre Belloni wrote:
> > On 27/02/2025 23:22:36+0900, William Breathitt Gray wrote:
> > > Skimming through the driver, it looks like what we'll need is for
> > > mchp_tc_counts[] to have all three TCCs defined, then have
> > > mchp_tc_probe() match on a TCB node and configure each TCC. Once that's
> > > setup, then whenever we need to identify which TCC a callback is
> > > exposing, we can get it from count->id.
> > > 
> > > So for example, the TC_CV register offset is calculated as 0x00 +
> > > channel * 0x40 + 0x10. In the count_read() callback we can leverage
> > > count->id to identify the TCC and thus get the respective TC_CV register
> > > at offset + count->id * 0x40 + 0x10.
> > > 
> > 
> > We can't do that because the TCC of a single TCB can have a mix of
> > different features. I struggled with the breakage to move away from the
> > one TCB, one feature state we had.
> > Be fore this, it was not possible to mix features on a single TCB, now,
> > we can have e.g. the clocksource on TCC 0 and 1 of TCB0 and a PWM on
> > TCC 2. mchp_tc_probe must not match on a TCB node...
> 
> Okay I see what you mean, if we match on a TCB mode then we wouldn't be
> able to define the cases where one TCC is different from the next in the
> same TCB.
> 
> The goal however isn't to support all functionality (i.e. PWM-related
> settings, etc.) in the counter driver, but just expose the TCB
> configuration options that affect the TCCs when configured for counter
> mode. For example, the sysfs attributes can be created, but they don't
> have to be available until the TCC is in the appropriate mode (e.g.
> return -EBUSY until they are in a counter mode).
> 
> Is there a way to achieve that? Maybe there's a way we can populate the
> sysfs tree on the first encountered TCC, and then somehow indicate when
> additional TCCs match. Attributes can become available then dynamically
> based on the TCCs that match.
> 
> William Breathitt Gray

Sorry, let me step back for a moment because maybe I'm trying to solve
a problem that might not actually be a problem.

I see functionality settings available in the TC Block Mode Register
(BMR) that can affect multiple TCCs at a time. Are these BMR settings
exposed already to users in someway? If not, do we have a way to
introduce these settings if someone wants them; e.g. would the
AutoCorrection function enable bit be exposed as a sysfs attribute, or
configured in the devicetree?

Finally, if there's not much interest in general for exposing these BMR
settings, then I suppose there is no need to change how things are right
now with the microchip-tcb-capture module and we can just keep it the
way it is. That's my only concern, whether there are users that want to
control these settings but don't have a way right now.

William Breathitt Gray

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

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[Csókás Bence]

Hi,

On 2025. 02. 27. 16:52, William Breathitt Gray wrote:
> Sorry, let me step back for a moment because maybe I'm trying to solve
> a problem that might not actually be a problem.
> 
> I see functionality settings available in the TC Block Mode Register
> (BMR) that can affect multiple TCCs at a time. Are these BMR settings
> exposed already to users in someway? If not, do we have a way to
> introduce these settings if someone wants them; e.g. would the
> AutoCorrection function enable bit be exposed as a sysfs attribute, or
> configured in the devicetree?
> 
> Finally, if there's not much interest in general for exposing these BMR
> settings, then I suppose there is no need to change how things are right
> now with the microchip-tcb-capture module and we can just keep it the
> way it is. That's my only concern, whether there are users that want to
> control these settings but don't have a way right now.

My knee-jerk answer to this is that if they do, they will bring it up by 
submitting a patch or bug request. But I'll let others chime in, we only 
use an extremely small subset of the features of the TCBs.

Bence

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[William Breathitt Gray]

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

On Thu, Feb 27, 2025 at 04:56:17PM +0100, Csókás Bence wrote:
> Hi,
> 
> On 2025. 02. 27. 16:52, William Breathitt Gray wrote:
> > Sorry, let me step back for a moment because maybe I'm trying to solve
> > a problem that might not actually be a problem.
> > 
> > I see functionality settings available in the TC Block Mode Register
> > (BMR) that can affect multiple TCCs at a time. Are these BMR settings
> > exposed already to users in someway? If not, do we have a way to
> > introduce these settings if someone wants them; e.g. would the
> > AutoCorrection function enable bit be exposed as a sysfs attribute, or
> > configured in the devicetree?
> > 
> > Finally, if there's not much interest in general for exposing these BMR
> > settings, then I suppose there is no need to change how things are right
> > now with the microchip-tcb-capture module and we can just keep it the
> > way it is. That's my only concern, whether there are users that want to
> > control these settings but don't have a way right now.
> 
> My knee-jerk answer to this is that if they do, they will bring it up by
> submitting a patch or bug request. But I'll let others chime in, we only use
> an extremely small subset of the features of the TCBs.
> 
> Bence

I think that's a reasonable stance to take. I don't have an elegant
solution either to this situation, so I'll defer trying to solve it
until an actual user shows up who needs the functionality. Until then,
it seems that what we have right now is adequate for the current
usecases.

William Breathitt Gray

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

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[Alexandre Belloni]

On 28/02/2025 00:52:34+0900, William Breathitt Gray wrote:
> On Fri, Feb 28, 2025 at 12:13:00AM +0900, William Breathitt Gray wrote:
> > On Thu, Feb 27, 2025 at 03:37:28PM +0100, Alexandre Belloni wrote:
> > > On 27/02/2025 23:22:36+0900, William Breathitt Gray wrote:
> > > > Skimming through the driver, it looks like what we'll need is for
> > > > mchp_tc_counts[] to have all three TCCs defined, then have
> > > > mchp_tc_probe() match on a TCB node and configure each TCC. Once that's
> > > > setup, then whenever we need to identify which TCC a callback is
> > > > exposing, we can get it from count->id.
> > > > 
> > > > So for example, the TC_CV register offset is calculated as 0x00 +
> > > > channel * 0x40 + 0x10. In the count_read() callback we can leverage
> > > > count->id to identify the TCC and thus get the respective TC_CV register
> > > > at offset + count->id * 0x40 + 0x10.
> > > > 
> > > 
> > > We can't do that because the TCC of a single TCB can have a mix of
> > > different features. I struggled with the breakage to move away from the
> > > one TCB, one feature state we had.
> > > Be fore this, it was not possible to mix features on a single TCB, now,
> > > we can have e.g. the clocksource on TCC 0 and 1 of TCB0 and a PWM on
> > > TCC 2. mchp_tc_probe must not match on a TCB node...
> > 
> > Okay I see what you mean, if we match on a TCB mode then we wouldn't be
> > able to define the cases where one TCC is different from the next in the
> > same TCB.
> > 
> > The goal however isn't to support all functionality (i.e. PWM-related
> > settings, etc.) in the counter driver, but just expose the TCB
> > configuration options that affect the TCCs when configured for counter
> > mode. For example, the sysfs attributes can be created, but they don't
> > have to be available until the TCC is in the appropriate mode (e.g.
> > return -EBUSY until they are in a counter mode).
> > 
> > Is there a way to achieve that? Maybe there's a way we can populate the
> > sysfs tree on the first encountered TCC, and then somehow indicate when
> > additional TCCs match. Attributes can become available then dynamically
> > based on the TCCs that match.
> > 
> > William Breathitt Gray
> 
> Sorry, let me step back for a moment because maybe I'm trying to solve
> a problem that might not actually be a problem.
> 
> I see functionality settings available in the TC Block Mode Register
> (BMR) that can affect multiple TCCs at a time. Are these BMR settings
> exposed already to users in someway? If not, do we have a way to
> introduce these settings if someone wants them; e.g. would the
> AutoCorrection function enable bit be exposed as a sysfs attribute, or
> configured in the devicetree?

BMR is already available and used by the individual drivers. The current
microchip-tcb-capture already uses it to enable qdec mode.
timer-atmel-tcb uses it to chain timers.

Note that we already have a driver for the pwm function too in
drivers/pwm/pwm-atmel-tcb.c. In fact all the other TCB drivers predate
the microchip-tcb-capture driver.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[Csókás Bence]

Hi,

On 2025. 02. 27. 5:59, William Breathitt Gray wrote:
>> Isn't it better to keep API header changes in the same commit as the
>> implementation using them? That way if someone bisects/blames the API
>> header, they get the respective implementation as well.
> 
> Fair enough, we'll keep the header together with the implementation.

I ended up splitting the actual creation of the file (the change to 
MAINTAINERS, along with the new header file containing the include guard 
and the doc-comment) to a new commit, and added the `enum 
counter_mchp_event_channels` to it in the commit containing the IRQ 
handler. I'll send that shortly.

>>> and it looks like this chip has
>>> three timer counter channels described in section 54. Currently, the
>>> microchip-tcb-capture module is exposing only one timer counter channel
>>> (as Count0), correct? Should this driver expose all three channels (as
>>> Count0, Count1, and Count2)?
>>
>> No, as this device is actually instantiated per-channel, i.e. in the DT,
>> there are two TCB nodes (as the SoC has two peripherals, each with 3
>> channels), and then the counter is a sub-node with `reg = <0/1/2>`,
>> specifying which timer channel to use. Or, in quadrature decode mode, you'd
>> have two elements in `reg`, i.e. `reg = <0>, <1>`.
> 
> So right now each timer counter channel is exposed as an independent
> Counter device? That means we're exposing the timer counter blocks
> incorrectly.
> 
> You're not at fault Bence, so you don't need to address this problem
> with your current patchset, but I do want to discuss it briefly here so
> we can come up with a plan for how to resolve it for the future. The
> Generic Counter Interface was nascent at the time, so we likely
> overlooked this problem at the time. I'm CCing some of those present
> during the original introduction of the microchip-tcb-capture driver so
> they are aware of this discussion.
> 
> Let me make sure I understand the situation correctly. This SoC has two
> Timer Counter Blocks (TCB) and each TCB has three Timer Counter Channels
> (TCC); each TCC has a Counter Value (CV) and three general registers
> (RA, RB, RC); RA and RB can store Captures, and RC can be used for
> Compare operations.

That seems to be an accurate description.

> If that is true, then the correct way for this hardware to be exposed is
> to have each TCB be a Counter device where each TCC is exposed as a
> Count. So for this SoC: two Counter devices as counter0 and counter1;
> count0, count1, and count2 as the three TCC; i.e. counter0/count{0,1,2}
> and counter1/count{0,1,2}.

And how would the extensions fit into this? 
`capture{0..6}`/`compare{0..3}? For me, the status quo fits better.

> With that setup, configurations that affect the entire TCB (e.g. Block
> Mode Register) can be exposed as Counter device components. Furthermore,
> this would allow users to set Counter watches to collect component
> values for the other two Counts while triggering off of the events of
> any particular one, which wouldn't be possible if each TCC is isolated
> to its own Counter device as is the case right now.

TCCs are pretty self-contained though. BMR only seems to be used

> Regardless, the three TCC of each TCB should be grouped together
> logically as they can represent related values. For example,  when using
> the quadrature decoder TTC0 CV can represent Speed/Position while TTC1
> CV represents rotation, thus giving a high level of precision on motion
> system position as the datasheet points out.

 From what I gathered from looking at the code, the quadrature mode uses 
a hardware decoder that gives us processed values already. Though I 
don't use it, so I don't know any specifics.

One more thing, as Kamel pointed it out, the current implementation 
allows channels of a TCB to perform different functions, e.g. one used 
for PWM, one for clocksource and a third for counter capture. Whether 
that works in practice, I cannot tell either, we only use TCB0 channel 0 
for clocksource and TCB1 channel 1 for the counter.

>>>> The `mchp_tc_count_function_write()` function already disables PWM mode by
>>>> clearing the `ATMEL_TC_WAVE` bit from the Channel Mode Register (CMR).
>>>
>>> So capture mode is unconditionally set by mchp_tc_count_function_write()
>>> which means the first time the user sets the Count function then PWM
>>> mode will be disabled. However, what happens if the user does not set
>>> the Count function? Should PWM mode be disabled by default in
>>> mchp_tc_probe(), or does that already happen?
>>
>> You're right, and it is a problem I encounter regularly: almost all HW
>> initialization happens in `mchp_tc_count_function_write()`, the probe()
>> function mostly just allocates stuff. Meaning, if you want to do anything
>> with the counter, you have to set the "increase" function first (even
>> though, if you `cat function`, it will seem like it's already in "increase"
>> mode). I don't know if it was deliberate, or what, but again, that would be
>> a separate bugfix patch.
> 
> That does seem like an oversight that goes back to the original commit
> 106b104137fd ("counter: Add microchip TCB capture counter"). I'll submit
> a bug fix patch later separately to address this and we can continue
> discussions about the issue there.

Thank you, squashing this annoyance would be appreciated.

> William Breathitt Gray

Bence

Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[William Breathitt Gray]

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

On Thu, Feb 27, 2025 at 03:17:55PM +0100, Csókás Bence wrote:
> On 2025. 02. 27. 5:59, William Breathitt Gray wrote:
> > If that is true, then the correct way for this hardware to be exposed is
> > to have each TCB be a Counter device where each TCC is exposed as a
> > Count. So for this SoC: two Counter devices as counter0 and counter1;
> > count0, count1, and count2 as the three TCC; i.e. counter0/count{0,1,2}
> > and counter1/count{0,1,2}.
> 
> And how would the extensions fit into this? `capture{0..6}`/`compare{0..3}?
> For me, the status quo fits better.

For your patchset here, treat it as if nothing is changing. So leave it
as capture0 and capture1 exposing RA and RB respectively.

> > With that setup, configurations that affect the entire TCB (e.g. Block
> > Mode Register) can be exposed as Counter device components. Furthermore,
> > this would allow users to set Counter watches to collect component
> > values for the other two Counts while triggering off of the events of
> > any particular one, which wouldn't be possible if each TCC is isolated
> > to its own Counter device as is the case right now.
> 
> TCCs are pretty self-contained though. BMR only seems to be used

In the Generic Counter Interface, hardware functionality exposure is
scoped by its influence over particular components of the Generic
Counter paradigm. That means, configurations that affect a particular
Count are grouped under that Count (i.e. "count0/enable" enables Count0)
while configurations that affect a particular Signal are grouped under
that Signal (i.e. "signal2/polarity" sets Signal2's polarity).

In the same way, configurations that affect the Counter device as a
whole are exposed as device components. That's what the functionality
BMR provides does, so it's appropriate to expose it as several Counter
device components.

Right now the microchip-tcb-capture doesn't do much with BMR, but
looking at the datasheet I see that it does have the capable to
configure several settings that affect the entire TCB. For example, you
can swap PHA and PHB (via SWAP bit), you can enable autocorrection for
missing pulses (via AUTOC bit), you can define a maximum filter threshold
(via MAXFILT bitfield), etc. These are controls users would expect to
find as Counter device components rather than Count components for
example.

> > Regardless, the three TCC of each TCB should be grouped together
> > logically as they can represent related values. For example,  when using
> > the quadrature decoder TTC0 CV can represent Speed/Position while TTC1
> > CV represents rotation, thus giving a high level of precision on motion
> > system position as the datasheet points out.
> 
> From what I gathered from looking at the code, the quadrature mode uses a
> hardware decoder that gives us processed values already. Though I don't use
> it, so I don't know any specifics.
> 
> One more thing, as Kamel pointed it out, the current implementation allows
> channels of a TCB to perform different functions, e.g. one used for PWM, one
> for clocksource and a third for counter capture. Whether that works in
> practice, I cannot tell either, we only use TCB0 channel 0 for clocksource
> and TCB1 channel 1 for the counter.

In theory this shouldn't be a problem because the counter driver
shouldn't try to perform non-counter functions, just expose the
counter-specific ones when available. We would need to check which mode
the channel is in and return -EBUSY if that respective channel's
components are accessed during a non-supported mode. This is how we
handle similar situations in other counter driver such as the
stm32-lptimer-cnt and intel-qep modules.

William Breathitt Gray

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