Message-ID: <7b853b4c-1a25-455b-a515-0188e8e37b69@microchip.com>
Date: Tue, 20 May 2025 14:19:51 +0000
From: <Dharma.B@...rochip.com>
To: <wbg@...nel.org>
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 20/05/25 4:51 pm, William Breathitt Gray wrote:
> 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.

Thanks for the comprehensive explanation, that really helped me clearly 
understand the purpose and design of the event channels.

It may also help someone looking for the same answer in the mailing 
list. Thanks again.


> 
> William Breathitt Gray


-- 
With Best Regards,
Dharma B.

Powered by blists - more mailing lists