| Message ID | 20230922143920.3144249-6-fabrice.gasnier@foss.st.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | counter: fix, improvements and stm32 timer events support | expand |
On Fri, Sep 22, 2023 at 04:39:19PM +0200, Fabrice Gasnier wrote: > This is a precursor patch to support capture channels on all possible > channels and stm32 timer types. Original driver was intended to be used > only as quadrature encoder and simple counter on internal clock. > > So, add ch3 and ch4 definition. Also add a check on encoder capability, > so the driver may be probed for timer instances without encoder feature. > This way, all timers may be used as simple counter on internal clock, > starting from here. Hi Fabrice, Let's split the encoder capability probing code, detect number of channels code, and channel introduction code to their own patches in order to simplify things. > Encoder capability is retrieved by using the timer index (originally in > stm32-timer-trigger driver and dt-bindings). The need to keep backward > compatibility with existing device tree lead to parse aside trigger node. > Add diversity as STM32 timers with capture feature may have either 4, 2, > 1 or no cc (capture/compare) channels. > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> I think this patch is more complicated than it needs to be. > @@ -400,13 +558,47 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev) > priv->clk = ddata->clk; > priv->max_arr = ddata->max_arr; > > + ret = stm32_timer_cnt_probe_encoder(pdev, priv); > + if (ret) > + return ret; > + > + stm32_timer_cnt_detect_channels(pdev, priv); > + > counter->name = dev_name(dev); > counter->parent = dev; > counter->ops = &stm32_timer_cnt_ops; > - counter->counts = &stm32_counts; > counter->num_counts = 1; > - counter->signals = stm32_signals; > - counter->num_signals = ARRAY_SIZE(stm32_signals); Keep this the same. > + > + /* > + * Handle diversity for stm32 timers features. For now encoder is found with > + * advanced timers or gp timers with 4 channels. Timers with less channels > + * doesn't support encoder. > + */ > + switch (priv->nchannels) { > + case 4: > + if (priv->has_encoder) > + counter->counts = &stm32_counts_enc_4ch; > + else > + counter->counts = &stm32_counts_4ch; > + counter->signals = stm32_signals; > + counter->num_signals = ARRAY_SIZE(stm32_signals); > + break; > + case 2: > + counter->counts = &stm32_counts_2ch; > + counter->signals = stm32_signals; > + counter->num_signals = 3; /* clock, ch1 and ch2 */ > + break; > + case 1: > + counter->counts = &stm32_counts_1ch; > + counter->signals = stm32_signals; > + counter->num_signals = 2; /* clock, ch1 */ > + break; > + default: > + counter->counts = &stm32_counts; > + counter->signals = stm32_signals; > + counter->num_signals = 1; /* clock */ > + break; > + } Rather than adjusting the number of counts and signals, keep the configuration static and use a single stm32_counts array. The reason is that in the Counter subsystem paradigm Signals do not necessary correlate to specific hardware signals but are rather an abstract representation of the device behavior at a high level. In other words, a Synapse with an action mode set to COUNTER_SYNAPSE_ACTION_NONE can be viewed as representing a Signal that does not affect the Count (i.e. in this case equivalent to an unconnected line). What you'll need to do instead is check priv->nchannels during stm32_action_read and stm32_count_function_read calls in order to return the correct synapse action and count function for the particular channels configuration you have. In stm32_count_function_write you would return an -EINVAL (maybe -EOPNOTSUPP would be better?) when the channels configuration does not support a particular count function. William Breathitt Gray
On 10/14/23 00:48, William Breathitt Gray wrote: > On Fri, Sep 22, 2023 at 04:39:19PM +0200, Fabrice Gasnier wrote: >> This is a precursor patch to support capture channels on all possible >> channels and stm32 timer types. Original driver was intended to be used >> only as quadrature encoder and simple counter on internal clock. >> >> So, add ch3 and ch4 definition. Also add a check on encoder capability, >> so the driver may be probed for timer instances without encoder feature. >> This way, all timers may be used as simple counter on internal clock, >> starting from here. > > Hi Fabrice, > > Let's split the encoder capability probing code, detect number of > channels code, and channel introduction code to their own patches in > order to simplify things. > >> Encoder capability is retrieved by using the timer index (originally in >> stm32-timer-trigger driver and dt-bindings). The need to keep backward >> compatibility with existing device tree lead to parse aside trigger node. >> Add diversity as STM32 timers with capture feature may have either 4, 2, >> 1 or no cc (capture/compare) channels. >> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> > > I think this patch is more complicated than it needs to be. > >> @@ -400,13 +558,47 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev) >> priv->clk = ddata->clk; >> priv->max_arr = ddata->max_arr; >> >> + ret = stm32_timer_cnt_probe_encoder(pdev, priv); >> + if (ret) >> + return ret; >> + >> + stm32_timer_cnt_detect_channels(pdev, priv); >> + >> counter->name = dev_name(dev); >> counter->parent = dev; >> counter->ops = &stm32_timer_cnt_ops; >> - counter->counts = &stm32_counts; >> counter->num_counts = 1; >> - counter->signals = stm32_signals; >> - counter->num_signals = ARRAY_SIZE(stm32_signals); > > Keep this the same. > >> + >> + /* >> + * Handle diversity for stm32 timers features. For now encoder is found with >> + * advanced timers or gp timers with 4 channels. Timers with less channels >> + * doesn't support encoder. >> + */ >> + switch (priv->nchannels) { >> + case 4: >> + if (priv->has_encoder) >> + counter->counts = &stm32_counts_enc_4ch; >> + else >> + counter->counts = &stm32_counts_4ch; >> + counter->signals = stm32_signals; >> + counter->num_signals = ARRAY_SIZE(stm32_signals); >> + break; >> + case 2: >> + counter->counts = &stm32_counts_2ch; >> + counter->signals = stm32_signals; >> + counter->num_signals = 3; /* clock, ch1 and ch2 */ >> + break; >> + case 1: >> + counter->counts = &stm32_counts_1ch; >> + counter->signals = stm32_signals; >> + counter->num_signals = 2; /* clock, ch1 */ >> + break; >> + default: >> + counter->counts = &stm32_counts; >> + counter->signals = stm32_signals; >> + counter->num_signals = 1; /* clock */ >> + break; >> + } > > Rather than adjusting the number of counts and signals, keep the > configuration static and use a single stm32_counts array. The reason is > that in the Counter subsystem paradigm Signals do not necessary > correlate to specific hardware signals but are rather an abstract > representation of the device behavior at a high level. In other words, a > Synapse with an action mode set to COUNTER_SYNAPSE_ACTION_NONE can be > viewed as representing a Signal that does not affect the Count (i.e. in > this case equivalent to an unconnected line). > > What you'll need to do instead is check priv->nchannels during > stm32_action_read and stm32_count_function_read calls in order to return > the correct synapse action and count function for the particular > channels configuration you have. In stm32_count_function_write you would > return an -EINVAL (maybe -EOPNOTSUPP would be better?) when the channels > configuration does not support a particular count function. Hi William, Sorry for the long delay to address your comments here. Many thanks for these guidelines. I'm preparing a v3, to address these. I'll probably send it soon, so we can start to review also the capture part of it. Still there are few things here I'm wondering about (as an anticipation task). Basically, removing all the diversity here means the most featured timer model will be represented here (with all possible signals). When I wrote the various configuration arrays, I'd have been tempted to allocate them dynamically upon probing to avoid having all these variants described as const arrays. This may have eased other signals additions later. But that's not the direction. So, this simplifies the description here, clearly, to describe the full-featured timer/counter, and handle the ("unconnected") variants by returning errors. I still have in mind the replacement of the last IIO_COUNT device [1] (not addressed in this series), e.g. in drivers/iio/trigger/stm32-timer-trigger.c. Here, there are "valids_table" that are used to cascade two timers (one timer output being the input to another timer). With this table currently, an IIO user knows the name of the signal it selects (then the driver looks up the 'valids' table to set SMCR / TS bits, e.g. trigger select). Each individual timer has a different input mapping, so called peripheral interconnect in STM32. What bothers me here is: with an abstracted full-featured timer, without any diversity on the signal names, I fear the userland has no clue on which signal would be used. Abstracting the timer this way would mean the user only knows it selects "Internal Trigger 0" for example, without knowing which internal signal in the SoC it has selected. Even if this is out of scope for this series, would you have some clue so I can anticipate it ? Or if we stick with abstract names? In which case the userland may need to be aware of the signals mapping (where currently in IIO_COUNT driver, the signal names are privided). I'd be glad to get some direction here. Please advise, Best Regards, Fabrice [1] https://lore.kernel.org/linux-arm-kernel/Y0vzlOmFrVCQVXMq@fedora/ > > William Breathitt Gray
On Fri, Dec 15, 2023 at 06:13:51PM +0100, Fabrice Gasnier wrote: > >> + /* > >> + * Handle diversity for stm32 timers features. For now encoder is found with > >> + * advanced timers or gp timers with 4 channels. Timers with less channels > >> + * doesn't support encoder. > >> + */ > >> + switch (priv->nchannels) { > >> + case 4: > >> + if (priv->has_encoder) > >> + counter->counts = &stm32_counts_enc_4ch; > >> + else > >> + counter->counts = &stm32_counts_4ch; > >> + counter->signals = stm32_signals; > >> + counter->num_signals = ARRAY_SIZE(stm32_signals); > >> + break; > >> + case 2: > >> + counter->counts = &stm32_counts_2ch; > >> + counter->signals = stm32_signals; > >> + counter->num_signals = 3; /* clock, ch1 and ch2 */ > >> + break; > >> + case 1: > >> + counter->counts = &stm32_counts_1ch; > >> + counter->signals = stm32_signals; > >> + counter->num_signals = 2; /* clock, ch1 */ > >> + break; > >> + default: > >> + counter->counts = &stm32_counts; > >> + counter->signals = stm32_signals; > >> + counter->num_signals = 1; /* clock */ > >> + break; > >> + } > > > > Rather than adjusting the number of counts and signals, keep the > > configuration static and use a single stm32_counts array. The reason is > > that in the Counter subsystem paradigm Signals do not necessary > > correlate to specific hardware signals but are rather an abstract > > representation of the device behavior at a high level. In other words, a > > Synapse with an action mode set to COUNTER_SYNAPSE_ACTION_NONE can be > > viewed as representing a Signal that does not affect the Count (i.e. in > > this case equivalent to an unconnected line). > > > > What you'll need to do instead is check priv->nchannels during > > stm32_action_read and stm32_count_function_read calls in order to return > > the correct synapse action and count function for the particular > > channels configuration you have. In stm32_count_function_write you would > > return an -EINVAL (maybe -EOPNOTSUPP would be better?) when the channels > > configuration does not support a particular count function. > > Hi William, > > Sorry for the long delay to address your comments here. Many thanks for > these guidelines. > > I'm preparing a v3, to address these. I'll probably send it soon, so we > can start to review also the capture part of it. Still there are few > things here I'm wondering about (as an anticipation task). > > Basically, removing all the diversity here means the most featured timer > model will be represented here (with all possible signals). > When I wrote the various configuration arrays, I'd have been tempted to > allocate them dynamically upon probing to avoid having all these > variants described as const arrays. This may have eased other signals > additions later. But that's not the direction. So, this simplifies the > description here, clearly, to describe the full-featured timer/counter, > and handle the ("unconnected") variants by returning errors. > > I still have in mind the replacement of the last IIO_COUNT device [1] > (not addressed in this series), e.g. in > drivers/iio/trigger/stm32-timer-trigger.c. Here, there are > "valids_table" that are used to cascade two timers (one timer output > being the input to another timer). With this table currently, an IIO > user knows the name of the signal it selects (then the driver looks up > the 'valids' table to set SMCR / TS bits, e.g. trigger select). Each > individual timer has a different input mapping, so called peripheral > interconnect in STM32. > What bothers me here is: with an abstracted full-featured timer, without > any diversity on the signal names, I fear the userland has no clue on > which signal would be used. Abstracting the timer this way would mean > the user only knows it selects "Internal Trigger 0" for example, without > knowing which internal signal in the SoC it has selected. > > Even if this is out of scope for this series, would you have some clue > so I can anticipate it ? Or if we stick with abstract names? In which > case the userland may need to be aware of the signals mapping (where > currently in IIO_COUNT driver, the signal names are privided). I'd be > glad to get some direction here. > > Please advise, > Best Regards, > Fabrice > > [1] https://lore.kernel.org/linux-arm-kernel/Y0vzlOmFrVCQVXMq@fedora/ Hi Fabrice, I took a look the stm32-timer-trigger.c file, but I'm having trouble understanding it well (probably the cascading is confusing me). If I understand the logic correctly, the user selects the trigger they want by the attribute's name which is compared via strncmp() against the 'valids' table. It's possible we could dynamically configure the signal names, but keep the signal id static so the driver code won't need some many variations; alternatively, we could introduce a new signal extension attribute that could display the trigger name. Would you walk me through an example of using the stm32-timer-trigger IIO sysfs interface? I think that would help me understand how users currently interact with the triggers for that driver, and then maybe I can figure out an appropriate equivalent in the Counter paradigm for the migration. Right now, the timer cascade is confusing me so if I can grok it well, the right solution for this may come to me more easily. Thanks, William Breathitt Gray
diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c index 11c66876b213..acde993891f5 100644 --- a/drivers/counter/stm32-timer-cnt.c +++ b/drivers/counter/stm32-timer-cnt.c @@ -11,6 +11,7 @@ #include <linux/mfd/stm32-timers.h> #include <linux/mod_devicetable.h> #include <linux/module.h> +#include <linux/of.h> #include <linux/pinctrl/consumer.h> #include <linux/platform_device.h> #include <linux/types.h> @@ -24,6 +25,8 @@ #define STM32_CLOCK_SIG 0 #define STM32_CH1_SIG 1 #define STM32_CH2_SIG 2 +#define STM32_CH3_SIG 3 +#define STM32_CH4_SIG 4 struct stm32_timer_regs { u32 cr1; @@ -38,6 +41,9 @@ struct stm32_timer_cnt { u32 max_arr; bool enabled; struct stm32_timer_regs bak; + bool has_encoder; + u32 idx; + unsigned int nchannels; }; static const enum counter_function stm32_count_functions[] = { @@ -265,6 +271,10 @@ static const enum counter_synapse_action stm32_synapse_actions[] = { COUNTER_SYNAPSE_ACTION_BOTH_EDGES }; +static const enum counter_synapse_action stm32_synapse_ch_actions[] = { + COUNTER_SYNAPSE_ACTION_NONE, +}; + static int stm32_action_read(struct counter_device *counter, struct counter_count *count, struct counter_synapse *synapse, @@ -347,10 +357,19 @@ static struct counter_signal stm32_signals[] = { { .id = STM32_CH2_SIG, .name = "Channel 2" + }, + { + .id = STM32_CH3_SIG, + .name = "Channel 3" + }, + { + .id = STM32_CH4_SIG, + .name = "Channel 4" } }; -static struct counter_synapse stm32_count_synapses[] = { +/* STM32 Timer with 4 capture channels and quadrature encoder */ +static struct counter_synapse stm32_count_synapses_4ch_enc[] = { { .actions_list = stm32_clock_synapse_actions, .num_actions = ARRAY_SIZE(stm32_clock_synapse_actions), @@ -365,20 +384,159 @@ static struct counter_synapse stm32_count_synapses[] = { .actions_list = stm32_synapse_actions, .num_actions = ARRAY_SIZE(stm32_synapse_actions), .signal = &stm32_signals[STM32_CH2_SIG] - } + }, + { + .actions_list = stm32_synapse_ch_actions, + .num_actions = ARRAY_SIZE(stm32_synapse_ch_actions), + .signal = &stm32_signals[STM32_CH3_SIG] + }, + { + .actions_list = stm32_synapse_ch_actions, + .num_actions = ARRAY_SIZE(stm32_synapse_ch_actions), + .signal = &stm32_signals[STM32_CH4_SIG] + }, }; -static struct counter_count stm32_counts = { +static struct counter_count stm32_counts_enc_4ch = { .id = 0, .name = "STM32 Timer Counter", .functions_list = stm32_count_functions, .num_functions = ARRAY_SIZE(stm32_count_functions), + .synapses = stm32_count_synapses_4ch_enc, + .num_synapses = ARRAY_SIZE(stm32_count_synapses_4ch_enc), + .ext = stm32_count_ext, + .num_ext = ARRAY_SIZE(stm32_count_ext) +}; + +/* STM32 Timer with up to 4 capture channels */ +static struct counter_synapse stm32_count_synapses[] = { + { + .actions_list = stm32_clock_synapse_actions, + .num_actions = ARRAY_SIZE(stm32_clock_synapse_actions), + .signal = &stm32_signals[STM32_CLOCK_SIG] + }, + { + .actions_list = stm32_synapse_ch_actions, + .num_actions = ARRAY_SIZE(stm32_synapse_ch_actions), + .signal = &stm32_signals[STM32_CH1_SIG] + }, + { + .actions_list = stm32_synapse_ch_actions, + .num_actions = ARRAY_SIZE(stm32_synapse_ch_actions), + .signal = &stm32_signals[STM32_CH2_SIG] + }, + { + .actions_list = stm32_synapse_ch_actions, + .num_actions = ARRAY_SIZE(stm32_synapse_ch_actions), + .signal = &stm32_signals[STM32_CH3_SIG] + }, + { + .actions_list = stm32_synapse_ch_actions, + .num_actions = ARRAY_SIZE(stm32_synapse_ch_actions), + .signal = &stm32_signals[STM32_CH4_SIG] + }, +}; + +static struct counter_count stm32_counts_4ch = { + .id = 0, + .name = "STM32 Timer Counter", + .functions_list = stm32_count_functions, + .num_functions = 1, /* increase */ .synapses = stm32_count_synapses, .num_synapses = ARRAY_SIZE(stm32_count_synapses), .ext = stm32_count_ext, .num_ext = ARRAY_SIZE(stm32_count_ext) }; +static struct counter_count stm32_counts_2ch = { + .id = 0, + .name = "STM32 Timer Counter", + .functions_list = stm32_count_functions, + .num_functions = 1, /* increase */ + .synapses = stm32_count_synapses, + .num_synapses = 3, /* clock, ch1 and ch2 */ + .ext = stm32_count_ext, + .num_ext = ARRAY_SIZE(stm32_count_ext) +}; + +static struct counter_count stm32_counts_1ch = { + .id = 0, + .name = "STM32 Timer Counter", + .functions_list = stm32_count_functions, + .num_functions = 1, /* increase */ + .synapses = stm32_count_synapses, + .num_synapses = 2, /* clock, ch1 */ + .ext = stm32_count_ext, + .num_ext = ARRAY_SIZE(stm32_count_ext) +}; + +static struct counter_count stm32_counts = { + .id = 0, + .name = "STM32 Timer Counter", + .functions_list = stm32_count_functions, + .num_functions = 1, /* increase */ + .synapses = stm32_count_synapses, + .num_synapses = 1, /* clock only */ + .ext = stm32_count_ext, + .num_ext = ARRAY_SIZE(stm32_count_ext) +}; + +static void stm32_timer_cnt_detect_channels(struct platform_device *pdev, + struct stm32_timer_cnt *priv) +{ + u32 ccer, ccer_backup; + + regmap_read(priv->regmap, TIM_CCER, &ccer_backup); + regmap_set_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE); + regmap_read(priv->regmap, TIM_CCER, &ccer); + regmap_write(priv->regmap, TIM_CCER, ccer_backup); + priv->nchannels = hweight32(ccer & TIM_CCER_CCXE); + + dev_dbg(&pdev->dev, "has %d cc channels\n", priv->nchannels); +} + +/* encoder supported on TIM1 TIM2 TIM3 TIM4 TIM5 TIM8 */ +#define STM32_TIM_ENCODER_SUPPORTED (BIT(0) | BIT(1) | BIT(2) | BIT(3) | BIT(4) | BIT(7)) + +static const char * const stm32_timer_trigger_compat[] = { + "st,stm32-timer-trigger", + "st,stm32h7-timer-trigger", +}; + +static int stm32_timer_cnt_probe_encoder(struct platform_device *pdev, + struct stm32_timer_cnt *priv) +{ + struct device *parent = pdev->dev.parent; + struct device_node *tnode = NULL, *pnode = parent->of_node; + int i, ret; + + /* + * Need to retrieve the trigger node index from DT, to be able + * to determine if the counter supports encoder mode. It also + * enforce backward compatibility, and allow to support other + * counter modes in this driver (when the timer doesn't support + * encoder). + */ + for (i = 0; i < ARRAY_SIZE(stm32_timer_trigger_compat) && !tnode; i++) + tnode = of_get_compatible_child(pnode, stm32_timer_trigger_compat[i]); + if (!tnode) { + dev_err(&pdev->dev, "Can't find trigger node\n"); + return -ENODATA; + } + + ret = of_property_read_u32(tnode, "reg", &priv->idx); + if (ret) { + dev_err(&pdev->dev, "Can't get index (%d)\n", ret); + return ret; + } + + priv->has_encoder = !!(STM32_TIM_ENCODER_SUPPORTED & BIT(priv->idx)); + + dev_dbg(&pdev->dev, "encoder support: %s\n", priv->has_encoder ? "yes" : "no"); + + return 0; +} + static int stm32_timer_cnt_probe(struct platform_device *pdev) { struct stm32_timers *ddata = dev_get_drvdata(pdev->dev.parent); @@ -400,13 +558,47 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev) priv->clk = ddata->clk; priv->max_arr = ddata->max_arr; + ret = stm32_timer_cnt_probe_encoder(pdev, priv); + if (ret) + return ret; + + stm32_timer_cnt_detect_channels(pdev, priv); + counter->name = dev_name(dev); counter->parent = dev; counter->ops = &stm32_timer_cnt_ops; - counter->counts = &stm32_counts; counter->num_counts = 1; - counter->signals = stm32_signals; - counter->num_signals = ARRAY_SIZE(stm32_signals); + + /* + * Handle diversity for stm32 timers features. For now encoder is found with + * advanced timers or gp timers with 4 channels. Timers with less channels + * doesn't support encoder. + */ + switch (priv->nchannels) { + case 4: + if (priv->has_encoder) + counter->counts = &stm32_counts_enc_4ch; + else + counter->counts = &stm32_counts_4ch; + counter->signals = stm32_signals; + counter->num_signals = ARRAY_SIZE(stm32_signals); + break; + case 2: + counter->counts = &stm32_counts_2ch; + counter->signals = stm32_signals; + counter->num_signals = 3; /* clock, ch1 and ch2 */ + break; + case 1: + counter->counts = &stm32_counts_1ch; + counter->signals = stm32_signals; + counter->num_signals = 2; /* clock, ch1 */ + break; + default: + counter->counts = &stm32_counts; + counter->signals = stm32_signals; + counter->num_signals = 1; /* clock */ + break; + } platform_set_drvdata(pdev, priv);
This is a precursor patch to support capture channels on all possible channels and stm32 timer types. Original driver was intended to be used only as quadrature encoder and simple counter on internal clock. So, add ch3 and ch4 definition. Also add a check on encoder capability, so the driver may be probed for timer instances without encoder feature. This way, all timers may be used as simple counter on internal clock, starting from here. Encoder capability is retrieved by using the timer index (originally in stm32-timer-trigger driver and dt-bindings). The need to keep backward compatibility with existing device tree lead to parse aside trigger node. Add diversity as STM32 timers with capture feature may have either 4, 2, 1 or no cc (capture/compare) channels. Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> --- Changes in v2: - rework to simplify since channel names has been updated. --- drivers/counter/stm32-timer-cnt.c | 204 +++++++++++++++++++++++++++++- 1 file changed, 198 insertions(+), 6 deletions(-)