Message-ID: <Z7vihBqOgP3fBUVq@ishi>
Date: Mon, 24 Feb 2025 12:07:48 +0900
From: William Breathitt Gray <wbg@...nel.org>
To: Csókás Bence <csokas.bence@...lan.hu>
Cc: linux-arm-kernel@...ts.infradead.org, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Kamel Bouhara <kamel.bouhara@...tlin.com>, Dharma.B@...rochip.com
Subject: Re: [PATCH v4 0/2] microchip-tcb-capture: Add Capture, Compare,
 Overflow etc. events

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

In the 104-quad-8 module,  the `preset` component is used for a number
of threshold-like purposes; you can see some of the uses by reading the
description of the /sys/bus/counter/devices/counterX/countY/count_mode
attribute. Of relevance to the SAMA5D2, the 104-quad-8 has a mode where
the current Count value is compared against the preset register and an
interrupt is fired when they match (i.e. our Counter THRESHOLD event).

I think it'll be fine to use the preset component to expose the RC
register because the 104-quad-8 already uses the preset component in a
similar fashion; also I would like to keep things simple in this
patchset to avoid the complexities of introducing a new `threshold`
component interface, at least until we get the rest of the capture
functionality in this driver merged.

So for now, use COUNTER_COMP_PRESET() to expose the RC register as a
`preset` component. In the next patchset revision, put this code in its
own patch after the capture components patch, so that we can review the
RC register code separately.

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

Ah okay, I think I understand now. Thank you for clarifying.

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

I can see your rationale, and I don't have too strong of an opinion
either way, so I'll leave it up to your best judgement. Ultimately, as
you point out we do have a large enough availability of channels that
will accommodate future introductions, which can be abstracted to users
via the uapi header, so we should be fine.

Regarding future additions, I took a look at the Microchip SAMA5D2
datasheet[^1] (is the right document?) 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)?

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

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?

William Breathitt Gray

[^1] https://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists