[PATCH] counter: microchip-tcb-capture: Add watch validation support

[PATCH] counter: microchip-tcb-capture: Add watch validation support

[Dharma Balasubiramani]

Introduce a watch validation callback to restrict supported event and
channel combinations. This allows userspace to receive notifications only
for valid event types and sources. Specifically, enable the following
supported events on channels RA, RB, and RC:

  - COUNTER_EVENT_CAPTURE
  - COUNTER_EVENT_CHANGE_OF_STATE
  - COUNTER_EVENT_OVERFLOW
  - COUNTER_EVENT_THRESHOLD

Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
---
 drivers/counter/microchip-tcb-capture.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
index 1de3c50b9804..179ff5595143 100644
--- a/drivers/counter/microchip-tcb-capture.c
+++ b/drivers/counter/microchip-tcb-capture.c
@@ -337,6 +337,27 @@ static struct counter_comp mchp_tc_count_ext[] = {
 	COUNTER_COMP_COMPARE(mchp_tc_count_compare_read, mchp_tc_count_compare_write),
 };
 
+static int mchp_tc_watch_validate(struct counter_device *counter,
+				  const struct counter_watch *watch)
+{
+	switch (watch->channel) {
+	case COUNTER_MCHP_EVCHN_RA:
+	case COUNTER_MCHP_EVCHN_RB:
+	case COUNTER_MCHP_EVCHN_RC:
+		switch (watch->event) {
+		case COUNTER_EVENT_CAPTURE:
+		case COUNTER_EVENT_CHANGE_OF_STATE:
+		case COUNTER_EVENT_OVERFLOW:
+		case COUNTER_EVENT_THRESHOLD:
+			return 0;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
 static struct counter_count mchp_tc_counts[] = {
 	{
 		.id = 0,
@@ -351,12 +372,13 @@ static struct counter_count mchp_tc_counts[] = {
 };
 
 static const struct counter_ops mchp_tc_ops = {
-	.signal_read    = mchp_tc_count_signal_read,
+	.action_read    = mchp_tc_count_action_read,
+	.action_write   = mchp_tc_count_action_write,
 	.count_read     = mchp_tc_count_read,
 	.function_read  = mchp_tc_count_function_read,
 	.function_write = mchp_tc_count_function_write,
-	.action_read    = mchp_tc_count_action_read,
-	.action_write   = mchp_tc_count_action_write
+	.signal_read    = mchp_tc_count_signal_read,
+	.watch_validate = mchp_tc_watch_validate,
 };
 
 static const struct atmel_tcb_config tcb_rm9200_config = {

---
base-commit: 9f35e33144ae5377d6a8de86dd3bd4d995c6ac65
change-id: 20250515-counter-tcb-b6ae1945210b

Best regards,
-- 
Dharma Balasubiramani <dharma.b@microchip.com>

Re: [PATCH] counter: microchip-tcb-capture: Add watch validation support

[William Breathitt Gray]

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

On Thu, May 15, 2025 at 10:28:25AM +0530, Dharma Balasubiramani wrote:
> Introduce a watch validation callback to restrict supported event and
> channel combinations. This allows userspace to receive notifications only
> for valid event types and sources. Specifically, enable the following
> supported events on channels RA, RB, and RC:
> 
>   - COUNTER_EVENT_CAPTURE
>   - COUNTER_EVENT_CHANGE_OF_STATE
>   - COUNTER_EVENT_OVERFLOW
>   - COUNTER_EVENT_THRESHOLD
> 
> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
> ---
>  drivers/counter/microchip-tcb-capture.c | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
> index 1de3c50b9804..179ff5595143 100644
> --- a/drivers/counter/microchip-tcb-capture.c
> +++ b/drivers/counter/microchip-tcb-capture.c
> @@ -337,6 +337,27 @@ static struct counter_comp mchp_tc_count_ext[] = {
>  	COUNTER_COMP_COMPARE(mchp_tc_count_compare_read, mchp_tc_count_compare_write),
>  };
> 
> +static int mchp_tc_watch_validate(struct counter_device *counter,
> +				  const struct counter_watch *watch)
> +{
> +	switch (watch->channel) {
> +	case COUNTER_MCHP_EVCHN_RA:
> +	case COUNTER_MCHP_EVCHN_RB:
> +	case COUNTER_MCHP_EVCHN_RC:

Hello Dharma,

Include COUNTER_MCHP_EVCHN_CV as well for the sake of completeness. I
know COUNTER_MCHP_EVCHN_CV and COUNTER_MCHP_EVCHN_RA have the same
underlying channel id, but we're abstracting this fact so it's good to
maintain the consistency of the abstraction across all callbacks.

> +		switch (watch->event) {
> +		case COUNTER_EVENT_CAPTURE:
> +		case COUNTER_EVENT_CHANGE_OF_STATE:
> +		case COUNTER_EVENT_OVERFLOW:
> +		case COUNTER_EVENT_THRESHOLD:
> +			return 0;

The watch_validate callback is used to ensure that the requested watch
configuration is valid: i.e. the watch event is appropriate for the
watch channel. 

Looking at include/uapi/linux/counter/microchip-tcb-capture.h:

     * Channel 0:
     * - CV register changed
     * - CV overflowed
     * - RA captured
     * Channel 1:
     * - RB captured
     * Channel 2:
     * - RC compare triggered

If I'm understanding correctly, channel 0 supports only the
CHANGE_OF_STATE, OVERFLOW, and CAPTURE events; channel 1 supports only
CAPTURE events; and channel 2 supports only THRESHOLD events.

Adjust the code to ensure those limitations.

> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static struct counter_count mchp_tc_counts[] = {
>  	{
>  		.id = 0,
> @@ -351,12 +372,13 @@ static struct counter_count mchp_tc_counts[] = {
>  };
> 
>  static const struct counter_ops mchp_tc_ops = {
> -	.signal_read    = mchp_tc_count_signal_read,
> +	.action_read    = mchp_tc_count_action_read,
> +	.action_write   = mchp_tc_count_action_write,
>  	.count_read     = mchp_tc_count_read,
>  	.function_read  = mchp_tc_count_function_read,
>  	.function_write = mchp_tc_count_function_write,
> -	.action_read    = mchp_tc_count_action_read,
> -	.action_write   = mchp_tc_count_action_write
> +	.signal_read    = mchp_tc_count_signal_read,

It's nice to alphabetize the counter_ops callbacks, but it's also
unrelated to the watch_validate implementation. Move the alphabetization
cleanup to a separate patch so that this patch remains dedicated to
just watch_validate changes.

Thanks,

William Breathitt Gray

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

Re: [PATCH] counter: microchip-tcb-capture: Add watch validation support

[William Breathitt Gray]

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

On Mon, May 19, 2025 at 10:47:50AM +0000, Dharma.B@microchip.com wrote:
> On 18/05/25 1:11 pm, William Breathitt Gray wrote:
> > Include COUNTER_MCHP_EVCHN_CV as well for the sake of completeness. I
> > know COUNTER_MCHP_EVCHN_CV and COUNTER_MCHP_EVCHN_RA have the same
> > underlying channel id, but we're abstracting this fact so it's good to
> > maintain the consistency of the abstraction across all callbacks.
> 
> To avoid the compiler error due to COUNTER_MCHP_EVCHN_CV and
> COUNTER_MCHP_EVCHN_RA sharing the same underlying value, would it be
> sufficient to include a comment indicating that both represent the same
> channel ID? Or would you prefer that I duplicate the logic explicitly
> for the sake of abstraction consistency, despite the shared value?

I see you use a conditional check in the v2 patch you submitted. That
solution works well to address both so we'll go with that way as you
have it.

> >> +		switch (watch->event) {
> >> +		case COUNTER_EVENT_CAPTURE:
> >> +		case COUNTER_EVENT_CHANGE_OF_STATE:
> >> +		case COUNTER_EVENT_OVERFLOW:
> >> +		case COUNTER_EVENT_THRESHOLD:
> >> +			return 0;
> >
> > The watch_validate callback is used to ensure that the requested watch
> > configuration is valid: i.e. the watch event is appropriate for the
> > watch channel.
> >
> > Looking at include/uapi/linux/counter/microchip-tcb-capture.h:
> >
> >       * Channel 0:
> >       * - CV register changed
> >       * - CV overflowed
> >       * - RA captured
> >       * Channel 1:
> >       * - RB captured
> >       * Channel 2:
> >       * - RC compare triggered
> >
> > If I'm understanding correctly, channel 0 supports only the
> > CHANGE_OF_STATE, OVERFLOW, and CAPTURE events; channel 1 supports only
> > CAPTURE events; and channel 2 supports only THRESHOLD events.
> 
> Shouldn't it be
> 
> /*
>   * Channel 0 (EVCHN_CV):
>   *   - CV register changed             → COUNTER_EVENT_CHANGE_OF_STATE
>   *   - CV overflowed                   → COUNTER_EVENT_OVERFLOW
>   *
>   * Channel 1 (EVCHN_RA):
>   *   - RA captured                     → COUNTER_EVENT_CAPTURE
>   *
>   * Channel 2 (EVCHN_RB):
>   *   - RB captured                     → COUNTER_EVENT_CAPTURE
>   *
>   * Channel 3 (EVCHN_RC):
>   *   - RC compare threshold reached    → COUNTER_EVENT_THRESHOLD
>   */

These Counter event channels are established ultimately by where you
push the events via counter_push_event(). So in theory, when these
events were introduced we could have arranged them onto four channels as
you describe. Unfortunately, we can't change it now (unless a serious
defect is found) because it'll break the ABI for existing programs.

> Could you please help me understand whether these are logical channels
> or hardware channels related to the reg?

You can consider these Counter event channels as "logical" and not
necessarily tied to the underlying the hardware registers.

I regret naming this functionality "channel" because it has caused
confusion before in other drivers as well. The origin of the term was
because I envisioned these Counter "event channels" providing a flow of
events streamed from the driver to userspace (much like a water channel
provides a flow of water). Notice how that concept lies completely on
the software side; i.e. between userspace and kernel -- not necessarily
between kernel and hardware.

In practice, we are limited to the capabilities of the hardware so that
does factor into how much freedom we have in defining our Counter events
channels. So let's walkthrough a scenario where we might want to offer
muliple Counter event channels for a driver rather than funneling all
events through a single channel.

Suppose a hypothetical device has two Counts that increase independent
of each other and can overflow back to a value of 0. This device is able
to issue an interrupt when either Count overflows. Now imagine we have a
race between these two Counts to see which overflows first: to determine
that the race ended we just need to check that a COUNTER_EVENT_OVERFLOW
event has been pushed; but what do we do if we want to determine the
winner of the race?

One solution is to check the values of both Counts: when a Count
overflows it starts over at 0, so the winner will have a Count value of
0. That is correct, but if one Count did not increase during the race
while the other Count did then both Counts will be 0 at the end: the
winner cannot be differentiated in this case.

That's a scenario where a second Counter event channel is useful: to
differentiate between events of the same type (in this case
COUNTER_EVENT_OVERFLOW). So if we dedicate the first Counter event
channel to the first Count and the second Counter event channel to the
second Count, we can definitely determine that a COUNTER_EVENT_OVERFLOW
watched on a particular channel represents an overflow for that
particular Count.

I hope that helps showcase why we offer multiple Counter event channels
even though we as driver authors have the freedom to define these
channels arbitrarily and push events to channels as we see fit.

William Breathitt Gray

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