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

[Kamel Bouhara <kamel.bouhara@bootlin.com>]

On Thu, Feb 27, 2025 at 01:59:57PM +0900, William Breathitt Gray wrote:
> 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?

Hello,

Indeed, each TCC operates independently except when quadrature mode is
enabled. I assume this approach was taken to provide more flexibility by
exposing them separately.

Currently only one channel is configured this would need to rework the
driver to make the 3 TCCs exposed.

Greetings,
Kamel

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



--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com