* Removing the last of IIO_COUNT (stm32-timer-trigger) @ 2022-10-16 12:05 William Breathitt Gray 2022-10-27 9:41 ` Fabrice Gasnier 0 siblings, 1 reply; 4+ messages in thread From: William Breathitt Gray @ 2022-10-16 12:05 UTC (permalink / raw) To: jic23, lars, benjamin.gaignard, mcoquelin.stm32, alexandre.torgue Cc: fabrice.gasnier, linux-iio, linux-stm32, linux-arm-kernel, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 1202 bytes --] Hi all, The drivers/iio/trigger/stm32-timer-trigger.c file remains the last consumer if the IIO_COUNT type. The IIO_COUNT type was deprecated some time ago with the introduction of the Counter subsystem. Most of the previous IIO_COUNT consumers were migrated successfully to the Counter subsystem, but the stm32-timer-trigger driver remains as the sole module preventing the final removal of IIO_COUNT. At the time we deprecated IIO_COUNT, the Counter subsystem was nascent and lacked some of the functionality we have nowadays such as a character device interface, timestamping, hardware buffer support, etc. If I recall correctly, the decision to delay the migration of stm32-timer-trigger to the Counter subsystem was a lack of some functionality the Counter subsystem could not provide at the time. I hoping someone can evaluate stm32-timer-trigger to see if we are able transition now to the Counter subsystem, or if that necessary functionality is still missing today. Even if it turns out that we are unable to migrate, it'll be useful to know what's left to implement in the Counter subsystem to support stm32-timer-trigger or similar devices in the future. Thanks, William Breathitt Gray [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Removing the last of IIO_COUNT (stm32-timer-trigger) 2022-10-16 12:05 Removing the last of IIO_COUNT (stm32-timer-trigger) William Breathitt Gray @ 2022-10-27 9:41 ` Fabrice Gasnier 2022-11-27 0:23 ` William Breathitt Gray 0 siblings, 1 reply; 4+ messages in thread From: Fabrice Gasnier @ 2022-10-27 9:41 UTC (permalink / raw) To: William Breathitt Gray, jic23, lars, benjamin.gaignard, mcoquelin.stm32, alexandre.torgue Cc: linux-iio, linux-stm32, linux-arm-kernel, linux-kernel On 10/16/22 14:05, William Breathitt Gray wrote: > Hi all, > > The drivers/iio/trigger/stm32-timer-trigger.c file remains the last > consumer if the IIO_COUNT type. The IIO_COUNT type was deprecated some > time ago with the introduction of the Counter subsystem. Most of the > previous IIO_COUNT consumers were migrated successfully to the Counter > subsystem, but the stm32-timer-trigger driver remains as the sole module > preventing the final removal of IIO_COUNT. > > At the time we deprecated IIO_COUNT, the Counter subsystem was nascent > and lacked some of the functionality we have nowadays such as a > character device interface, timestamping, hardware buffer support, etc. > If I recall correctly, the decision to delay the migration of > stm32-timer-trigger to the Counter subsystem was a lack of some > functionality the Counter subsystem could not provide at the time. > Hi William, As far as I remember, initial work on stm32-timer-counter focused only on porting the quadrature interface away from stm32-timer-trigger. Unfortunately, I've followed from afar all the progress you did in the framework since then. From the infrastructure point of view, there's probably not much to add to be able to move away the IIO_COUNT channel (and the IIO device) part of stm32-timer-trigger driver. Let's focus only on the modes implemented here. Besides this, I may have further questions on the "hardware buffer support" (could you point this as I miss it for now), and also the capture interface. There has been a separate discussion here ("pwm: Make capture support optional"): https://lore.kernel.org/linux-pwm/Yz%2F4V0gH%2FvrWSS8U@orome/T/#u I'd be glad to get your opinion, on possibly moving the PWM input capture feature to the counter framework too. > I hoping someone can evaluate stm32-timer-trigger to see if we are able > transition now to the Counter subsystem, or if that necessary > functionality is still missing today. Even if it turns out that we are > unable to migrate, it'll be useful to know what's left to implement in > the Counter subsystem to support stm32-timer-trigger or similar devices > in the future. As you're asking, I just tried to narrow down specific things in this driver, and assess possible impacts. Please find some details here after and first as an introduction: The IIO device registered in this driver has two specific extensions, to manage specific ("slave") modes: "always", "gated", "triggered". E.g the *enable_mode*. The last 2 modes depends on specific hardware *triggers* being associated in IIO sysfs, to select the trigger (e.g. echo the source trigger name > trigger/current_trigger # the destination timer to trig). The list of triggers is specific to each timer instance in STM32. In other words, some timers outputs can be used as input on other timers. Here comes the *trigger_mode* attribute (see after). --- enable_mode --- In Documentation/ABI/testing/sysfs-bus-iio-timer-stm32, this correspond to: - /sys/bus/iio/devices/iio:deviceX/in_count0_enable_mode - /sys/bus/iio/devices/iio:deviceX/in_count_enable_mode_available always: Counter is always ON. gated: Counting is enabled when connected trigger signal level is high else counting is disabled. triggered: Counting is enabled on rising edge of the connected trigger, and remains enabled for the duration of this selected mode. Basically, the "always" mode is already used by default in stm32-timer-cnt driver, and matches: COUNTER_COUNT_MODE_NORMAL, By referring to: Documentation/ABI/testing/sysfs-bus-counter normal: Counting is continuous in either direction. Please find some thoughts/proposal here: - This could lead to add two counter modes to the list of "range limit", "non-recycle" and "modulo-n". The STM32 timer trigger inputs could be described as signals (list being specific for each timer instance, see valids_table[] arrays in stm32-timer-trigger). - OR, maybe the 2 modes could be described as a specific synapse action, (trigger input also being described as a signal)? In both "gated" and "triggered" modes, the timer counts on its internal clock input (but not continuously). But it doesn't really match the "normal" mode. - Last, maybe a mix of a new "trigger" count mode, and synapse action (gated/triggered) could be used ? Note: The last one may open the door to other modes that aren't implemented in current stm32-timer-trigger driver, by extending the synapse actions (like reset the counter upon trigger signal... and other combined modes specified in the STM32 timer spec). --- trigger_mode --- In Documentation/ABI/testing/sysfs-bus-iio-timer-stm32, this correspond to: - /sys/bus/iio/devices/iio:deviceX/in_count_trigger_mode_available - /sys/bus/iio/devices/iio:deviceX/in_count0_trigger_mode In the STM32 timer spec this is: External Clock Mode 1 - Rising edges of the selected trigger (TRGI) clock the counter. In this configuration, IMHO, this matches the "normal" counter mode. This lead also here to define trigger inputs as a signals. Then the standard increase/decrease functions suffice. --- dt-bindings --- This could be a tight part. Listing/probing the triggers relies on the reg property defined in the trigger node: - Documentation/devicetree/bindings/mfd/st,stm32-timers.yaml timers2: timer@40000000 { timer@1 { compatible = "st,stm32-timer-trigger"; reg = <1>; } counter { compatible = "st,stm32-timer-counter"; }; } Ideally... adding the reg property to the counter node could have helped. But we need to enforce the backward compatibility with existing DT binaries. So I think that there's no choice to keep the current bindings. This could lead to add some code to parse the trigger node for probing, looking for the "reg" property either from: - the MFD driver part drivers/mfd/stm32-timers.c - the counter driver, although it seems non-standard way to parse aside nodes. I have no strong opinion and I'm open to suggestions. --- To conclude, there some open items here, but hopefully nothing blocking. In case we sort all these, this will allow to remove the IIO_COUNT channel (along with the IIO device) being registered. There will still remain some specific attributes in the stm32-timer-trigger driver, related to the trigger device: - /sys/bus/iio/devices/triggerX/master_mode - /sys/bus/iio/devices/triggerX/master_mode_available But this shouldn't be an issue as it isn't related to the IIO_COUNT part of the driver. Please advise, Best Regards, Fabrice > > Thanks, > > William Breathitt Gray _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Removing the last of IIO_COUNT (stm32-timer-trigger) 2022-10-27 9:41 ` Fabrice Gasnier @ 2022-11-27 0:23 ` William Breathitt Gray 2022-12-03 16:20 ` Jonathan Cameron 0 siblings, 1 reply; 4+ messages in thread From: William Breathitt Gray @ 2022-11-27 0:23 UTC (permalink / raw) To: Fabrice Gasnier Cc: jic23, lars, benjamin.gaignard, mcoquelin.stm32, alexandre.torgue, linux-iio, linux-stm32, linux-arm-kernel, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 10160 bytes --] On Thu, Oct 27, 2022 at 11:41:18AM +0200, Fabrice Gasnier wrote: > On 10/16/22 14:05, William Breathitt Gray wrote: > > Hi all, > > > > The drivers/iio/trigger/stm32-timer-trigger.c file remains the last > > consumer if the IIO_COUNT type. The IIO_COUNT type was deprecated some > > time ago with the introduction of the Counter subsystem. Most of the > > previous IIO_COUNT consumers were migrated successfully to the Counter > > subsystem, but the stm32-timer-trigger driver remains as the sole module > > preventing the final removal of IIO_COUNT. > > > > At the time we deprecated IIO_COUNT, the Counter subsystem was nascent > > and lacked some of the functionality we have nowadays such as a > > character device interface, timestamping, hardware buffer support, etc. > > If I recall correctly, the decision to delay the migration of > > stm32-timer-trigger to the Counter subsystem was a lack of some > > functionality the Counter subsystem could not provide at the time. > > > > Hi William, > > As far as I remember, initial work on stm32-timer-counter focused only > on porting the quadrature interface away from stm32-timer-trigger. > Unfortunately, I've followed from afar all the progress you did in the > framework since then. > From the infrastructure point of view, there's probably not much to add > to be able to move away the IIO_COUNT channel (and the IIO device) part > of stm32-timer-trigger driver. > Let's focus only on the modes implemented here. Hi Fabrice, Sorry for the delay in my response; it took me a while to think through all this, but I believe you have the right approach in your suggestions. > > Besides this, I may have further questions on the "hardware buffer > support" (could you point this as I miss it for now), and also the > capture interface. > There has been a separate discussion here ("pwm: Make capture support > optional"): > https://lore.kernel.org/linux-pwm/Yz%2F4V0gH%2FvrWSS8U@orome/T/#u > I'd be glad to get your opinion, on possibly moving the PWM input > capture feature to the counter framework too. I replied to the PWM discussion to provide some context about the Counter events (capture) interface. You can find out more about the Counter hardware buffer support in the Counter array components support introduction patch series: https://lore.kernel.org/all/cover.1664204990.git.william.gray@linaro.org/ Essentially Counter arrays are a convenience tool to allow drivers to group and handle multiple components of the same type. Hardware buffers are thus supported by exposing each buffer element as an individual component to the user; those components are handled by the Counter driver via a callback with the index for the particular element passed in as an argument. > > > I hoping someone can evaluate stm32-timer-trigger to see if we are able > > transition now to the Counter subsystem, or if that necessary > > functionality is still missing today. Even if it turns out that we are > > unable to migrate, it'll be useful to know what's left to implement in > > the Counter subsystem to support stm32-timer-trigger or similar devices > > in the future. > > As you're asking, I just tried to narrow down specific things in this > driver, and assess possible impacts. Please find some details here after > and first as an introduction: > > The IIO device registered in this driver has two specific extensions, to > manage specific ("slave") modes: "always", "gated", "triggered". > E.g the *enable_mode*. > > The last 2 modes depends on specific hardware *triggers* being > associated in IIO sysfs, to select the trigger (e.g. echo the source > trigger name > trigger/current_trigger # the destination timer to trig). > The list of triggers is specific to each timer instance in STM32. > In other words, some timers outputs can be used as input on other > timers. Here comes the *trigger_mode* attribute (see after). > > --- enable_mode --- > In Documentation/ABI/testing/sysfs-bus-iio-timer-stm32, this correspond to: > - /sys/bus/iio/devices/iio:deviceX/in_count0_enable_mode > - /sys/bus/iio/devices/iio:deviceX/in_count_enable_mode_available > always: > Counter is always ON. > gated: > Counting is enabled when connected trigger signal > level is high else counting is disabled. > triggered: > Counting is enabled on rising edge of the connected > trigger, and remains enabled for the duration of this > selected mode. Okay, we need to support these three modes. The "always" mode seems straight-forward to support, but "gated" and "triggered" are what need special consideration for this driver. > > Basically, the "always" mode is already used by default in > stm32-timer-cnt driver, and matches: COUNTER_COUNT_MODE_NORMAL, By > referring to: > Documentation/ABI/testing/sysfs-bus-counter > normal: > Counting is continuous in either direction. > > Please find some thoughts/proposal here: > - This could lead to add two counter modes to the list of "range limit", > "non-recycle" and "modulo-n". The STM32 timer trigger inputs could be > described as signals (list being specific for each timer instance, see > valids_table[] arrays in stm32-timer-trigger). It makes sense to represent these inputs as Signals because they can directly affect the value change of the respective Count, so I think we should organize this as such. However, "count_mode" wouldn't be the correct place for configuration of the "gated" and "triggered" modes. "count_mode" is for configuring how the Count value updates once a Synapse action triggers, but "gated" and "triggered" are rather more so determining whether those Synapse actions should trigger in the first place. > > - OR, maybe the 2 modes could be described as a specific synapse action, > (trigger input also being described as a signal)? In both "gated" and > "triggered" modes, the timer counts on its internal clock input (but not > continuously). But it doesn't really match the "normal" mode. Yes, I think this is the approach to take. Synapse actions represent when the Count function is evaluated to update the respective Count; "gated" and "triggered" represent modes where a trigger input serves to determine when that update occurs, so those are naturally Synapse action modes. To add support for these, you would just need to introduce to include/uapi/linux/counter.h two new enum counter_synapse_action constants: COUNTER_SYNAPSE_ACTION_GATED and COUNTER_SYNAPSE_ACTION_TRIGGERED. You can then use those new enums in your action_read() callbacks. > > - Last, maybe a mix of a new "trigger" count mode, and synapse action > (gated/triggered) could be used ? > Note: The last one may open the door to other modes that aren't > implemented in current stm32-timer-trigger driver, by extending the > synapse actions (like reset the counter upon trigger signal... and other > combined modes specified in the STM32 timer spec). Instead of "count_mode", a new Count component should be introduced to handle this configuration. Maybe we could call it "gate_mode" (or perhaps your might have a better name) and it can be implemented using the COUNTER_COMP_COUNT_ENUM() macro to allow for selection between "always", "gated", and "triggered". > > --- trigger_mode --- > In Documentation/ABI/testing/sysfs-bus-iio-timer-stm32, this correspond to: > - /sys/bus/iio/devices/iio:deviceX/in_count_trigger_mode_available > - /sys/bus/iio/devices/iio:deviceX/in_count0_trigger_mode > In the STM32 timer spec this is: External Clock Mode 1 - Rising edges of > the selected trigger (TRGI) clock the counter. This can be implemented as a COUNTER_COMP_COUNT_ENUM() to select the particular trigger input. > > In this configuration, IMHO, this matches the "normal" counter mode. > This lead also here to define trigger inputs as a signals. Then the > standard increase/decrease functions suffice. > > --- dt-bindings --- > This could be a tight part. Listing/probing the triggers relies on the > reg property defined in the trigger node: > - Documentation/devicetree/bindings/mfd/st,stm32-timers.yaml > > timers2: timer@40000000 { > timer@1 { > compatible = "st,stm32-timer-trigger"; > reg = <1>; > } > counter { > compatible = "st,stm32-timer-counter"; > }; > } > > Ideally... adding the reg property to the counter node could have > helped. But we need to enforce the backward compatibility with existing > DT binaries. So I think that there's no choice to keep the current bindings. > > This could lead to add some code to parse the trigger node for probing, > looking for the "reg" property either from: > - the MFD driver part drivers/mfd/stm32-timers.c > - the counter driver, although it seems non-standard way to parse aside > nodes. > I have no strong opinion and I'm open to suggestions. I don't have much of a strong opinion either, but I'd consider the MFD driver to be the more natural location to place this. However, I'm also open to suggestions if someone thinks it better somewhere else. > > --- > To conclude, there some open items here, but hopefully nothing blocking. > In case we sort all these, this will allow to remove the IIO_COUNT > channel (along with the IIO device) being registered. I'm certain Jonathan will want some sort of deprecation schedule first to make sure any existing users have time to migrate to the Counter interface before we remove the IIO one, but it will give me a nice feeling of completion to see the last of IIO_COUNT superceded by the Counter interface. ;-) William Breathitt Gray > > There will still remain some specific attributes in the > stm32-timer-trigger driver, related to the trigger device: > - /sys/bus/iio/devices/triggerX/master_mode > - /sys/bus/iio/devices/triggerX/master_mode_available > But this shouldn't be an issue as it isn't related to the IIO_COUNT part > of the driver. > > Please advise, > Best Regards, > Fabrice > > > > > Thanks, > > > > William Breathitt Gray [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Removing the last of IIO_COUNT (stm32-timer-trigger) 2022-11-27 0:23 ` William Breathitt Gray @ 2022-12-03 16:20 ` Jonathan Cameron 0 siblings, 0 replies; 4+ messages in thread From: Jonathan Cameron @ 2022-12-03 16:20 UTC (permalink / raw) To: William Breathitt Gray Cc: Fabrice Gasnier, lars, benjamin.gaignard, mcoquelin.stm32, alexandre.torgue, linux-iio, linux-stm32, linux-arm-kernel, linux-kernel ... > > --- > > To conclude, there some open items here, but hopefully nothing blocking. > > In case we sort all these, this will allow to remove the IIO_COUNT > > channel (along with the IIO device) being registered. > > I'm certain Jonathan will want some sort of deprecation schedule first > to make sure any existing users have time to migrate to the Counter > interface before we remove the IIO one, but it will give me a nice > feeling of completion to see the last of IIO_COUNT superceded by the > Counter interface. ;-) If there is nothing in tree, I'm fine 'removing' it. However, note that we'll have to keep a placeholder for some of the defines to avoid renumbering other types etc. So probably just stick a couple of underscores in front to make it __IIO_COUNT and add a comment to say it should not be used. Jonathan > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-12-03 16:09 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-10-16 12:05 Removing the last of IIO_COUNT (stm32-timer-trigger) William Breathitt Gray 2022-10-27 9:41 ` Fabrice Gasnier 2022-11-27 0:23 ` William Breathitt Gray 2022-12-03 16:20 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).