| Message ID | 20250211151914.313585-3-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| Headers | show |
| Series | microchip-tcb-capture: Add Capture, Compare, Overflow etc. events | expand |
On Tue, Feb 11, 2025 at 04:19:11PM +0100, Bence Csókás wrote: > The TCB has three R/W-able "general purpose" hardware registers: > RA, RB and RC. The hardware is capable of: > * sampling Counter Value Register (CV) to RA/RB on a trigger edge > * sending an interrupt of this change > * sending an interrupt on CV change due to trigger > * triggering an interrupt on CV compare to RC > * stop counting after sampling to RB > > To enable using these features in user-space, an interrupt handler > was added, generating the necessary counter events. On top, RA/B/C > registers are added as Count Extensions. To aid interoperation, a > uapi header was also added, containing the various numeral IDs of > the Extensions, Event channels etc. > > Bence Csókás (2): > counter: microchip-tcb-capture: Add IRQ handling > counter: microchip-tcb-capture: Add capture extensions for registers > RA-RC > > MAINTAINERS | 1 + > drivers/counter/microchip-tcb-capture.c | 137 ++++++++++++++++++ > .../linux/counter/microchip-tcb-capture.h | 49 +++++++ > 3 files changed, 187 insertions(+) > create mode 100644 include/uapi/linux/counter/microchip-tcb-capture.h Hi Bence, I had some time to read over your description of the three hardware registers (RA, RB, and RC)[^1] and I have some suggestions. 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? 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? 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. 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. William Breathitt Gray [^1] https://lore.kernel.org/all/7b581014-a351-4077-8832-d3d347b4fdb5@prolan.hu/
Hi William, On 2025. 02. 21. 13:39, William Breathitt Gray wrote: > On Tue, Feb 11, 2025 at 04:19:11PM +0100, Bence Csókás wrote: >> The TCB has three R/W-able "general purpose" hardware registers: >> RA, RB and RC. The hardware is capable of: >> * sampling Counter Value Register (CV) to RA/RB on a trigger edge >> * sending an interrupt of this change >> * sending an interrupt on CV change due to trigger >> * triggering an interrupt on CV compare to RC >> * stop counting after sampling to RB >> >> To enable using these features in user-space, an interrupt handler >> was added, generating the necessary counter events. On top, RA/B/C >> registers are added as Count Extensions. To aid interoperation, a >> uapi header was also added, containing the various numeral IDs of >> the Extensions, Event channels etc. >> >> Bence Csókás (2): >> counter: microchip-tcb-capture: Add IRQ handling >> counter: microchip-tcb-capture: Add capture extensions for registers >> RA-RC >> >> MAINTAINERS | 1 + >> drivers/counter/microchip-tcb-capture.c | 137 ++++++++++++++++++ >> .../linux/counter/microchip-tcb-capture.h | 49 +++++++ >> 3 files changed, 187 insertions(+) >> create mode 100644 include/uapi/linux/counter/microchip-tcb-capture.h > > Hi Bence, > > I had some time to read over your description of the three hardware > registers (RA, RB, and RC)[^1] and I have some suggestions. > > 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. > 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. > 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). > 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). > William Breathitt Gray > > [^1] https://lore.kernel.org/all/7b581014-a351-4077-8832-d3d347b4fdb5@prolan.hu/ Bence
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
Hi, 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. > 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. > Regarding future additions, I took a look at the Microchip SAMA5D2 > datasheet[^1] (is the right document?) There are many versions, but yes, it is one of them, and is usable. > 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>`. >> 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. Bence
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
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
On Thu, Feb 27, 2025 at 02:53:33PM +0100, Kamel Bouhara wrote: > 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. > One important point to consider is that each TCC can be configured in Capture/QDEC, PWM or clocksource mode. We have written a blog post that describes each modes in detail [1], which can help clarify how the TCC works on Microchip devices. > 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 -- Kamel Bouhara, Bootlin Embedded Linux and kernel engineering https://bootlin.com [1]: https://bootlin.com/blog/timer-counters-linux-microchip/
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
On Thu, Feb 27, 2025 at 02:53:30PM +0100, Kamel Bouhara wrote: > On Thu, Feb 27, 2025 at 01:59:57PM +0900, William Breathitt Gray wrote: > > 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}. [...] > > 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 Skimming through the driver, it looks like what we'll need is for mchp_tc_counts[] to have all three TCCs defined, then have mchp_tc_probe() match on a TCB node and configure each TCC. Once that's setup, then whenever we need to identify which TCC a callback is exposing, we can get it from count->id. So for example, the TC_CV register offset is calculated as 0x00 + channel * 0x40 + 0x10. In the count_read() callback we can leverage count->id to identify the TCC and thus get the respective TC_CV register at offset + count->id * 0x40 + 0x10. William Breathitt Gray
On 27/02/2025 23:22:36+0900, William Breathitt Gray wrote: > On Thu, Feb 27, 2025 at 02:53:30PM +0100, Kamel Bouhara wrote: > > On Thu, Feb 27, 2025 at 01:59:57PM +0900, William Breathitt Gray wrote: > > > 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}. > > [...] > > > > 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 > > Skimming through the driver, it looks like what we'll need is for > mchp_tc_counts[] to have all three TCCs defined, then have > mchp_tc_probe() match on a TCB node and configure each TCC. Once that's > setup, then whenever we need to identify which TCC a callback is > exposing, we can get it from count->id. > > So for example, the TC_CV register offset is calculated as 0x00 + > channel * 0x40 + 0x10. In the count_read() callback we can leverage > count->id to identify the TCC and thus get the respective TC_CV register > at offset + count->id * 0x40 + 0x10. > We can't do that because the TCC of a single TCB can have a mix of different features. I struggled with the breakage to move away from the one TCB, one feature state we had. Be fore this, it was not possible to mix features on a single TCB, now, we can have e.g. the clocksource on TCC 0 and 1 of TCB0 and a PWM on TCC 2. mchp_tc_probe must not match on a TCB node...
On Thu, Feb 27, 2025 at 03:17:55PM +0100, Csókás Bence wrote: > On 2025. 02. 27. 5:59, William Breathitt Gray wrote: > > 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. For your patchset here, treat it as if nothing is changing. So leave it as capture0 and capture1 exposing RA and RB respectively. > > 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 In the Generic Counter Interface, hardware functionality exposure is scoped by its influence over particular components of the Generic Counter paradigm. That means, configurations that affect a particular Count are grouped under that Count (i.e. "count0/enable" enables Count0) while configurations that affect a particular Signal are grouped under that Signal (i.e. "signal2/polarity" sets Signal2's polarity). In the same way, configurations that affect the Counter device as a whole are exposed as device components. That's what the functionality BMR provides does, so it's appropriate to expose it as several Counter device components. Right now the microchip-tcb-capture doesn't do much with BMR, but looking at the datasheet I see that it does have the capable to configure several settings that affect the entire TCB. For example, you can swap PHA and PHB (via SWAP bit), you can enable autocorrection for missing pulses (via AUTOC bit), you can define a maximum filter threshold (via MAXFILT bitfield), etc. These are controls users would expect to find as Counter device components rather than Count components for example. > > 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. In theory this shouldn't be a problem because the counter driver shouldn't try to perform non-counter functions, just expose the counter-specific ones when available. We would need to check which mode the channel is in and return -EBUSY if that respective channel's components are accessed during a non-supported mode. This is how we handle similar situations in other counter driver such as the stm32-lptimer-cnt and intel-qep modules. William Breathitt Gray
On Thu, Feb 27, 2025 at 03:37:28PM +0100, Alexandre Belloni wrote: > On 27/02/2025 23:22:36+0900, William Breathitt Gray wrote: > > On Thu, Feb 27, 2025 at 02:53:30PM +0100, Kamel Bouhara wrote: > > > On Thu, Feb 27, 2025 at 01:59:57PM +0900, William Breathitt Gray wrote: > > > > 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}. > > > > [...] > > > > > > 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 > > > > Skimming through the driver, it looks like what we'll need is for > > mchp_tc_counts[] to have all three TCCs defined, then have > > mchp_tc_probe() match on a TCB node and configure each TCC. Once that's > > setup, then whenever we need to identify which TCC a callback is > > exposing, we can get it from count->id. > > > > So for example, the TC_CV register offset is calculated as 0x00 + > > channel * 0x40 + 0x10. In the count_read() callback we can leverage > > count->id to identify the TCC and thus get the respective TC_CV register > > at offset + count->id * 0x40 + 0x10. > > > > We can't do that because the TCC of a single TCB can have a mix of > different features. I struggled with the breakage to move away from the > one TCB, one feature state we had. > Be fore this, it was not possible to mix features on a single TCB, now, > we can have e.g. the clocksource on TCC 0 and 1 of TCB0 and a PWM on > TCC 2. mchp_tc_probe must not match on a TCB node... Okay I see what you mean, if we match on a TCB mode then we wouldn't be able to define the cases where one TCC is different from the next in the same TCB. The goal however isn't to support all functionality (i.e. PWM-related settings, etc.) in the counter driver, but just expose the TCB configuration options that affect the TCCs when configured for counter mode. For example, the sysfs attributes can be created, but they don't have to be available until the TCC is in the appropriate mode (e.g. return -EBUSY until they are in a counter mode). Is there a way to achieve that? Maybe there's a way we can populate the sysfs tree on the first encountered TCC, and then somehow indicate when additional TCCs match. Attributes can become available then dynamically based on the TCCs that match. William Breathitt Gray
On Fri, Feb 28, 2025 at 12:13:00AM +0900, William Breathitt Gray wrote: > On Thu, Feb 27, 2025 at 03:37:28PM +0100, Alexandre Belloni wrote: > > On 27/02/2025 23:22:36+0900, William Breathitt Gray wrote: > > > Skimming through the driver, it looks like what we'll need is for > > > mchp_tc_counts[] to have all three TCCs defined, then have > > > mchp_tc_probe() match on a TCB node and configure each TCC. Once that's > > > setup, then whenever we need to identify which TCC a callback is > > > exposing, we can get it from count->id. > > > > > > So for example, the TC_CV register offset is calculated as 0x00 + > > > channel * 0x40 + 0x10. In the count_read() callback we can leverage > > > count->id to identify the TCC and thus get the respective TC_CV register > > > at offset + count->id * 0x40 + 0x10. > > > > > > > We can't do that because the TCC of a single TCB can have a mix of > > different features. I struggled with the breakage to move away from the > > one TCB, one feature state we had. > > Be fore this, it was not possible to mix features on a single TCB, now, > > we can have e.g. the clocksource on TCC 0 and 1 of TCB0 and a PWM on > > TCC 2. mchp_tc_probe must not match on a TCB node... > > Okay I see what you mean, if we match on a TCB mode then we wouldn't be > able to define the cases where one TCC is different from the next in the > same TCB. > > The goal however isn't to support all functionality (i.e. PWM-related > settings, etc.) in the counter driver, but just expose the TCB > configuration options that affect the TCCs when configured for counter > mode. For example, the sysfs attributes can be created, but they don't > have to be available until the TCC is in the appropriate mode (e.g. > return -EBUSY until they are in a counter mode). > > Is there a way to achieve that? Maybe there's a way we can populate the > sysfs tree on the first encountered TCC, and then somehow indicate when > additional TCCs match. Attributes can become available then dynamically > based on the TCCs that match. > > William Breathitt Gray Sorry, let me step back for a moment because maybe I'm trying to solve a problem that might not actually be a problem. I see functionality settings available in the TC Block Mode Register (BMR) that can affect multiple TCCs at a time. Are these BMR settings exposed already to users in someway? If not, do we have a way to introduce these settings if someone wants them; e.g. would the AutoCorrection function enable bit be exposed as a sysfs attribute, or configured in the devicetree? Finally, if there's not much interest in general for exposing these BMR settings, then I suppose there is no need to change how things are right now with the microchip-tcb-capture module and we can just keep it the way it is. That's my only concern, whether there are users that want to control these settings but don't have a way right now. William Breathitt Gray
Hi, On 2025. 02. 27. 16:52, William Breathitt Gray wrote: > Sorry, let me step back for a moment because maybe I'm trying to solve > a problem that might not actually be a problem. > > I see functionality settings available in the TC Block Mode Register > (BMR) that can affect multiple TCCs at a time. Are these BMR settings > exposed already to users in someway? If not, do we have a way to > introduce these settings if someone wants them; e.g. would the > AutoCorrection function enable bit be exposed as a sysfs attribute, or > configured in the devicetree? > > Finally, if there's not much interest in general for exposing these BMR > settings, then I suppose there is no need to change how things are right > now with the microchip-tcb-capture module and we can just keep it the > way it is. That's my only concern, whether there are users that want to > control these settings but don't have a way right now. My knee-jerk answer to this is that if they do, they will bring it up by submitting a patch or bug request. But I'll let others chime in, we only use an extremely small subset of the features of the TCBs. Bence
On 28/02/2025 00:52:34+0900, William Breathitt Gray wrote: > On Fri, Feb 28, 2025 at 12:13:00AM +0900, William Breathitt Gray wrote: > > On Thu, Feb 27, 2025 at 03:37:28PM +0100, Alexandre Belloni wrote: > > > On 27/02/2025 23:22:36+0900, William Breathitt Gray wrote: > > > > Skimming through the driver, it looks like what we'll need is for > > > > mchp_tc_counts[] to have all three TCCs defined, then have > > > > mchp_tc_probe() match on a TCB node and configure each TCC. Once that's > > > > setup, then whenever we need to identify which TCC a callback is > > > > exposing, we can get it from count->id. > > > > > > > > So for example, the TC_CV register offset is calculated as 0x00 + > > > > channel * 0x40 + 0x10. In the count_read() callback we can leverage > > > > count->id to identify the TCC and thus get the respective TC_CV register > > > > at offset + count->id * 0x40 + 0x10. > > > > > > > > > > We can't do that because the TCC of a single TCB can have a mix of > > > different features. I struggled with the breakage to move away from the > > > one TCB, one feature state we had. > > > Be fore this, it was not possible to mix features on a single TCB, now, > > > we can have e.g. the clocksource on TCC 0 and 1 of TCB0 and a PWM on > > > TCC 2. mchp_tc_probe must not match on a TCB node... > > > > Okay I see what you mean, if we match on a TCB mode then we wouldn't be > > able to define the cases where one TCC is different from the next in the > > same TCB. > > > > The goal however isn't to support all functionality (i.e. PWM-related > > settings, etc.) in the counter driver, but just expose the TCB > > configuration options that affect the TCCs when configured for counter > > mode. For example, the sysfs attributes can be created, but they don't > > have to be available until the TCC is in the appropriate mode (e.g. > > return -EBUSY until they are in a counter mode). > > > > Is there a way to achieve that? Maybe there's a way we can populate the > > sysfs tree on the first encountered TCC, and then somehow indicate when > > additional TCCs match. Attributes can become available then dynamically > > based on the TCCs that match. > > > > William Breathitt Gray > > Sorry, let me step back for a moment because maybe I'm trying to solve > a problem that might not actually be a problem. > > I see functionality settings available in the TC Block Mode Register > (BMR) that can affect multiple TCCs at a time. Are these BMR settings > exposed already to users in someway? If not, do we have a way to > introduce these settings if someone wants them; e.g. would the > AutoCorrection function enable bit be exposed as a sysfs attribute, or > configured in the devicetree? BMR is already available and used by the individual drivers. The current microchip-tcb-capture already uses it to enable qdec mode. timer-atmel-tcb uses it to chain timers. Note that we already have a driver for the pwm function too in drivers/pwm/pwm-atmel-tcb.c. In fact all the other TCB drivers predate the microchip-tcb-capture driver.
On Thu, Feb 27, 2025 at 04:56:17PM +0100, Csókás Bence wrote: > Hi, > > On 2025. 02. 27. 16:52, William Breathitt Gray wrote: > > Sorry, let me step back for a moment because maybe I'm trying to solve > > a problem that might not actually be a problem. > > > > I see functionality settings available in the TC Block Mode Register > > (BMR) that can affect multiple TCCs at a time. Are these BMR settings > > exposed already to users in someway? If not, do we have a way to > > introduce these settings if someone wants them; e.g. would the > > AutoCorrection function enable bit be exposed as a sysfs attribute, or > > configured in the devicetree? > > > > Finally, if there's not much interest in general for exposing these BMR > > settings, then I suppose there is no need to change how things are right > > now with the microchip-tcb-capture module and we can just keep it the > > way it is. That's my only concern, whether there are users that want to > > control these settings but don't have a way right now. > > My knee-jerk answer to this is that if they do, they will bring it up by > submitting a patch or bug request. But I'll let others chime in, we only use > an extremely small subset of the features of the TCBs. > > Bence I think that's a reasonable stance to take. I don't have an elegant solution either to this situation, so I'll defer trying to solve it until an actual user shows up who needs the functionality. Until then, it seems that what we have right now is adequate for the current usecases. William Breathitt Gray
The TCB has three R/W-able "general purpose" hardware registers: RA, RB and RC. The hardware is capable of: * sampling Counter Value Register (CV) to RA/RB on a trigger edge * sending an interrupt of this change * sending an interrupt on CV change due to trigger * triggering an interrupt on CV compare to RC * stop counting after sampling to RB To enable using these features in user-space, an interrupt handler was added, generating the necessary counter events. On top, RA/B/C registers are added as Count Extensions. To aid interoperation, a uapi header was also added, containing the various numeral IDs of the Extensions, Event channels etc. Bence Csókás (2): counter: microchip-tcb-capture: Add IRQ handling counter: microchip-tcb-capture: Add capture extensions for registers RA-RC MAINTAINERS | 1 + drivers/counter/microchip-tcb-capture.c | 137 ++++++++++++++++++ .../linux/counter/microchip-tcb-capture.h | 49 +++++++ 3 files changed, 187 insertions(+) create mode 100644 include/uapi/linux/counter/microchip-tcb-capture.h