Message-ID: <aCxlwAgX2A0dVn5l@ishi>
Date: Tue, 20 May 2025 20:21:36 +0900
From: William Breathitt Gray <wbg@...nel.org>
To: Dharma.B@...rochip.com
Cc: kamel.bouhara@...tlin.com, linux-arm-kernel@...ts.infradead.org,
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] counter: microchip-tcb-capture: Add watch validation
 support

On Mon, May 19, 2025 at 10:47:50AM +0000, Dharma.B@...rochip.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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists