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
