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

[PATCH v3 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

 drivers/counter/microchip-tcb-capture.c | 67 +++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
index 2f096a5b973d..fef4bb69b486 100644
--- a/drivers/counter/microchip-tcb-capture.c
+++ b/drivers/counter/microchip-tcb-capture.c
@@ -6,10 +6,12 @@
  */
 #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 <soc/at91/atmel_tcb.h>
@@ -18,6 +20,9 @@
 				 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 +32,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 +300,60 @@ 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, 0);
+	if (sr & ATMEL_TC_LDRAS)
+		counter_push_event(counter, COUNTER_EVENT_CAPTURE, 0);
+	if (sr & ATMEL_TC_LDRBS)
+		counter_push_event(counter, COUNTER_EVENT_CAPTURE, 1);
+	if (sr & ATMEL_TC_CPCS)
+		counter_push_event(counter, COUNTER_EVENT_THRESHOLD, 2);
+	if (sr & ATMEL_TC_COVFS)
+		counter_push_event(counter, COUNTER_EVENT_OVERFLOW, 0);
+
+	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 +438,13 @@ 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 > 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");
-- 
2.48.1

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

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

 drivers/counter/microchip-tcb-capture.c | 125 ++++++++++++++++++++++++
 1 file changed, 125 insertions(+)

-- 
2.48.1

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

[Dharma.B]

Hi Bence,

Thanks for your patch.

On 05/02/25 4:20 pm, 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.

I did a quick test on the SAMA5D4 Xplained board and observed that the 
interrupt count increments on overflow.

By the way, I came across William's response to your question, and from 
that discussion, I gather that you're working on a SAMA5D2 board. Are 
you seeing the count value increase after writing this:

echo increase > /sys/bus/counter/devices/counter0/count0/function

cat /sys/bus/counter/devices/counter0/count0/count

> 
> Bence Csókás (2):
>    counter: microchip-tcb-capture: Add IRQ handling
>    counter: microchip-tcb-capture: Add capture extensions for registers
>      RA-RC
> 
>   drivers/counter/microchip-tcb-capture.c | 125 ++++++++++++++++++++++++
>   1 file changed, 125 insertions(+)
> 


-- 
With Best Regards,
Dharma B.

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

[Dharma.B]

Hi Bence,

On 05/02/25 4:19 pm, Bence Csókás wrote:
> 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
> 
>   drivers/counter/microchip-tcb-capture.c | 67 +++++++++++++++++++++++++
>   1 file changed, 67 insertions(+)
> 
> diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
> index 2f096a5b973d..fef4bb69b486 100644
> --- a/drivers/counter/microchip-tcb-capture.c
> +++ b/drivers/counter/microchip-tcb-capture.c
> @@ -6,10 +6,12 @@
>    */
>   #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 <soc/at91/atmel_tcb.h>
> @@ -18,6 +20,9 @@
>   				 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 +32,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 +300,60 @@ 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, 0);
> +	if (sr & ATMEL_TC_LDRAS)
> +		counter_push_event(counter, COUNTER_EVENT_CAPTURE, 0);
> +	if (sr & ATMEL_TC_LDRBS)
> +		counter_push_event(counter, COUNTER_EVENT_CAPTURE, 1);
> +	if (sr & ATMEL_TC_CPCS)
> +		counter_push_event(counter, COUNTER_EVENT_THRESHOLD, 2);
> +	if (sr & ATMEL_TC_COVFS)
> +		counter_push_event(counter, COUNTER_EVENT_OVERFLOW, 0);

can we have macros for the channel (3rd argument) for better clarity?

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

Can we have it as mchp_tc_irq_init ?

> +{
> +	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 +438,13 @@ 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 > 0) {
> +		ret = mchp_tc_irq_enable(counter);

missing error handling in irq retrieval (check for -EPROBE_DEFER).

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


-- 
With Best Regards,
Dharma B.

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

[Csókás Bence]

Hi,

On 2025. 02. 07. 9:12, Dharma.B@microchip.com wrote:
> can we have macros for the channel (3rd argument) for better clarity?

I myself have also thought about that, even about adding an uapi header. 
Thoughts?

>> +static int mchp_tc_irq_enable(struct counter_device *const counter)
> 
> Can we have it as mchp_tc_irq_init ?

Why, what's wrong with the current name? It requests IRQ servicing from 
Linux, then writes the peripheral's Interrupt Enable Register.

>> +{
>> +	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 +438,13 @@ 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 > 0) {
>> +		ret = mchp_tc_irq_enable(counter);
> 
> missing error handling in irq retrieval (check for -EPROBE_DEFER).

Hmm, what should happen on `priv->irq == -EPROBE_DEFER`? `return 
-EPROBE_DEFER`?

Bence

[PATCH v3 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

 drivers/counter/microchip-tcb-capture.c | 58 +++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
index fef4bb69b486..1445ac512c52 100644
--- a/drivers/counter/microchip-tcb-capture.c
+++ b/drivers/counter/microchip-tcb-capture.c
@@ -253,6 +253,62 @@ 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;
+
+	switch (idx) {
+	case 0:
+		regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RA), &cnt);
+		break;
+	case 1:
+		regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RB), &cnt);
+		break;
+	case 2:
+		regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RC), &cnt);
+		break;
+	default:
+		return -EINVAL;
+	}
+	*val = cnt;
+
+	return 0;
+}
+
+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);
+
+	if (val > U32_MAX)
+		return -ERANGE;
+
+	switch (idx) {
+	case 0:
+		regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], RA), val);
+		break;
+	case 1:
+		regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], RB), val);
+		break;
+	case 2:
+		regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], RC), val);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+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,
@@ -261,6 +317,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),
 	},
 };
 
-- 
2.48.1

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

[Dharma.B]

Hi Bence,

On 05/02/25 4:19 pm, Bence Csókás wrote:
> 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
> 
>   drivers/counter/microchip-tcb-capture.c | 58 +++++++++++++++++++++++++
>   1 file changed, 58 insertions(+)
> 
> diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
> index fef4bb69b486..1445ac512c52 100644
> --- a/drivers/counter/microchip-tcb-capture.c
> +++ b/drivers/counter/microchip-tcb-capture.c
> @@ -253,6 +253,62 @@ 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)

The registers RA/B/C are 32 bit registers, hence use of u64 is unnecessary.

> +{
> +	struct mchp_tc_data *const priv = counter_priv(counter);
> +	u32 cnt;
> +
> +	switch (idx) {
> +	case 0:
> +		regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RA), &cnt);
> +		break;
> +	case 1:
> +		regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RB), &cnt);
> +		break;
> +	case 2:
> +		regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RC), &cnt);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

The regmap_read() returns an error code, which is currently ignored. If 
regmap_read() fails, cnt remains uninitialized, potentially returning 
garbage data.

> +	*val = cnt;
> +
> +	return 0;
> +}
> +
> +static int mchp_tc_count_cap_write(struct counter_device *counter,
> +				   struct counter_count *count, size_t idx, u64 val)

ditto

> +{
> +	struct mchp_tc_data *const priv = counter_priv(counter);
> +
> +	if (val > U32_MAX)
> +		return -ERANGE;
> +
> +	switch (idx) {
> +	case 0:
> +		regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], RA), val);

ditto

> +		break;
> +	case 1:
> +		regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], RB), val);
> +		break;
> +	case 2:
> +		regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], RC), val);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +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,
> @@ -261,6 +317,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),
>   	},
>   };
>   


-- 
With Best Regards,
Dharma B.

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

[Csókás Bence]

Hi,

On 2025. 02. 07. 9:19, Dharma.B@microchip.com wrote:
> The registers RA/B/C are 32 bit registers, hence use of u64 is unnecessary.

Correct, but the definition of COUNTER_COMP_ARRAY_CAPTURE 
(include/linux/counter.h:623) is:

	#define COUNTER_COMP_ARRAY_CAPTURE(_read, _write, _array) \
		COUNTER_COMP_COUNT_ARRAY_U64("capture", _read, _write, _array)

> The regmap_read() returns an error code, which is currently ignored. If
> regmap_read() fails, cnt remains uninitialized, potentially returning
> garbage data.

Will fix.

Thanks,
Bence