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.hHi 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@xxxxxxxxx/
Bence
