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

[William Breathitt Gray <wbg@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6409 bytes --]

On Wed, Feb 26, 2025 at 01:58:37PM +0100, Csókás Bence wrote:
> On 2025. 02. 24. 4:07, William Breathitt Gray wrote:
> > On Fri, Feb 21, 2025 at 03:14:44PM +0100, Csókás Bence wrote:
> > > On 2025. 02. 21. 13:39, William Breathitt Gray wrote:
> > > > 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.
> > 
> > You can find the semantics of each attribute under the sysfs ABI doc
> > file located at Documentation/ABI/testing/sysfs-bus-counter. For the
> > `preset` component, its essential purpose is to configure a value to
> > preset register to reload the Count when some condition is met (e.g.
> > when an external INDEX/SYNC trigger line goes high).
> 
> Hmm, that doesn't really match this use case. All right, then, for now, I'll
> skip the RC part, and then we can add it in a later patch when the
> "threshold" component is in place and used by the 104-quad-8 module.

Understood, I'll work on a separate patchset introducing a "threshold"
component (perhaps "compare" is a better name) to the 104-quad-8 and
once that is complete we can add it to the microchip-tcb-capture as its
own patch to support the RC register functionality.

> > In the same vein, move the uapi header introduction to its own patch.
> > That will separate the userspace-exposed changes and make things easier
> > for future users when bisecting the linux kernel history when tracking
> > down possible bugs.
> 
> 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.

> > 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.

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}.

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.

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.

Kamel, what would it take for us to rectify this situation so that the
TCC are organized together by TCB under the same Counter devices?

> > > 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.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]