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

[Csókás Bence]

Hi,

On 2025. 02. 27. 5:59, William Breathitt Gray wrote:
>> Isn't it better to keep API header changes in the same commit as the
>> implementation using them? That way if someone bisects/blames the API
>> header, they get the respective implementation as well.
> 
> Fair enough, we'll keep the header together with the implementation.

I ended up splitting the actual creation of the file (the change to 
MAINTAINERS, along with the new header file containing the include guard 
and the doc-comment) to a new commit, and added the `enum 
counter_mchp_event_channels` to it in the commit containing the IRQ 
handler. I'll send that shortly.

>>> and it looks like this chip has
>>> three timer counter channels described in section 54. Currently, the
>>> microchip-tcb-capture module is exposing only one timer counter channel
>>> (as Count0), correct? Should this driver expose all three channels (as
>>> Count0, Count1, and Count2)?
>>
>> No, as this device is actually instantiated per-channel, i.e. in the DT,
>> there are two TCB nodes (as the SoC has two peripherals, each with 3
>> channels), and then the counter is a sub-node with `reg = <0/1/2>`,
>> specifying which timer channel to use. Or, in quadrature decode mode, you'd
>> have two elements in `reg`, i.e. `reg = <0>, <1>`.
> 
> So right now each timer counter channel is exposed as an independent
> Counter device? That means we're exposing the timer counter blocks
> incorrectly.
> 
> You're not at fault Bence, so you don't need to address this problem
> with your current patchset, but I do want to discuss it briefly here so
> we can come up with a plan for how to resolve it for the future. The
> Generic Counter Interface was nascent at the time, so we likely
> overlooked this problem at the time. I'm CCing some of those present
> during the original introduction of the microchip-tcb-capture driver so
> they are aware of this discussion.
> 
> Let me make sure I understand the situation correctly. This SoC has two
> Timer Counter Blocks (TCB) and each TCB has three Timer Counter Channels
> (TCC); each TCC has a Counter Value (CV) and three general registers
> (RA, RB, RC); RA and RB can store Captures, and RC can be used for
> Compare operations.

That seems to be an accurate description.

> If that is true, then the correct way for this hardware to be exposed is
> to have each TCB be a Counter device where each TCC is exposed as a
> Count. So for this SoC: two Counter devices as counter0 and counter1;
> count0, count1, and count2 as the three TCC; i.e. counter0/count{0,1,2}
> and counter1/count{0,1,2}.

And how would the extensions fit into this? 
`capture{0..6}`/`compare{0..3}? For me, the status quo fits better.

> With that setup, configurations that affect the entire TCB (e.g. Block
> Mode Register) can be exposed as Counter device components. Furthermore,
> this would allow users to set Counter watches to collect component
> values for the other two Counts while triggering off of the events of
> any particular one, which wouldn't be possible if each TCC is isolated
> to its own Counter device as is the case right now.

TCCs are pretty self-contained though. BMR only seems to be used

> Regardless, the three TCC of each TCB should be grouped together
> logically as they can represent related values. For example,  when using
> the quadrature decoder TTC0 CV can represent Speed/Position while TTC1
> CV represents rotation, thus giving a high level of precision on motion
> system position as the datasheet points out.

 From what I gathered from looking at the code, the quadrature mode uses 
a hardware decoder that gives us processed values already. Though I 
don't use it, so I don't know any specifics.

One more thing, as Kamel pointed it out, the current implementation 
allows channels of a TCB to perform different functions, e.g. one used 
for PWM, one for clocksource and a third for counter capture. Whether 
that works in practice, I cannot tell either, we only use TCB0 channel 0 
for clocksource and TCB1 channel 1 for the counter.

>>>> The `mchp_tc_count_function_write()` function already disables PWM mode by
>>>> clearing the `ATMEL_TC_WAVE` bit from the Channel Mode Register (CMR).
>>>
>>> So capture mode is unconditionally set by mchp_tc_count_function_write()
>>> which means the first time the user sets the Count function then PWM
>>> mode will be disabled. However, what happens if the user does not set
>>> the Count function? Should PWM mode be disabled by default in
>>> mchp_tc_probe(), or does that already happen?
>>
>> You're right, and it is a problem I encounter regularly: almost all HW
>> initialization happens in `mchp_tc_count_function_write()`, the probe()
>> function mostly just allocates stuff. Meaning, if you want to do anything
>> with the counter, you have to set the "increase" function first (even
>> though, if you `cat function`, it will seem like it's already in "increase"
>> mode). I don't know if it was deliberate, or what, but again, that would be
>> a separate bugfix patch.
> 
> That does seem like an oversight that goes back to the original commit
> 106b104137fd ("counter: Add microchip TCB capture counter"). I'll submit
> a bug fix patch later separately to address this and we can continue
> discussions about the issue there.

Thank you, squashing this annoyance would be appreciated.

> William Breathitt Gray

Bence