| Message ID | 20231220145726.640627-10-fabrice.gasnier@foss.st.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | counter: Add stm32 timer events support | expand |
On Wed, Dec 20, 2023 at 03:57:25PM +0100, Fabrice Gasnier wrote: > Add support overflow events. Also add the related validation and > configuration routine. Register and enable interrupts to push events. > STM32 Timers can have either 1 global interrupt, or 4 dedicated interrupt > lines. Request only the necessary interrupt, e.g. either global interrupt > that can report all event types, or update interrupt only for overflow > event. > > Acked-by: Lee Jones <lee@kernel.org> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> Hi Fabrice, I've CC'd Will and Peter in case they can provide some suggestions regarding my atomic_t comment inline below. > @@ -44,6 +45,9 @@ struct stm32_timer_cnt { > bool has_encoder; > u32 idx; > unsigned int nchannels; > + unsigned int nr_irqs; > + u32 *irq; Looks like we only need this 'irq' array for registering the ISR in stm32_timer_cnt_probe(). Since we won't need it anymore after that, let's use ddata->irq directly instead of defining priv->irq. > + atomic_t nb_ovf; > }; > > static const enum counter_function stm32_count_functions[] = { > @@ -259,6 +263,29 @@ static int stm32_count_prescaler_write(struct counter_device *counter, > return regmap_write(priv->regmap, TIM_PSC, psc); > } > > +static int stm32_count_nb_ovf_read(struct counter_device *counter, > + struct counter_count *count, u64 *val) > +{ > + struct stm32_timer_cnt *const priv = counter_priv(counter); > + > + *val = atomic_read(&priv->nb_ovf); > + > + return 0; > +} > + > +static int stm32_count_nb_ovf_write(struct counter_device *counter, > + struct counter_count *count, u64 val) > +{ > + struct stm32_timer_cnt *const priv = counter_priv(counter); > + > + if (val != (typeof(priv->nb_ovf.counter))val) > + return -ERANGE; > + > + atomic_set(&priv->nb_ovf, val); So you want to check that the atomic_t 'nb_ovf' is able hold the value provided by the u64 'val'. My understanding is that atomic_t should be treated as an opaque type, so I don't think we should be accessing the 'counter' member directly for this test (interrupt-cnt does this but I believe it's wrong to do so). I don't know if we have any existing way to check for the value range of an atomic_t (I don't see anything under include/linux/limits.h specifically for it). However, you do use atomic_set() which takes an int parameter, so perhaps we should compare against INT_MAX instead. > +static int stm32_count_events_configure(struct counter_device *counter) > +{ > + struct stm32_timer_cnt *const priv = counter_priv(counter); > + struct counter_event_node *event_node; > + u32 val, dier = 0; > + > + list_for_each_entry(event_node, &counter->events_list, l) { > + switch (event_node->event) { > + case COUNTER_EVENT_OVERFLOW_UNDERFLOW: > + /* first clear possibly latched UIF before enabling */ > + regmap_read(priv->regmap, TIM_DIER, &val); > + if (!(val & TIM_DIER_UIE)) You can eliminate 'val' and the regmap_read() line like this: if (!regmap_test_bits(priv->regmap, TIM_DIER, TIM_DIER_UIE)) > + regmap_write(priv->regmap, TIM_SR, (u32)~TIM_SR_UIF); > + dier |= TIM_DIER_UIE; > + break; > + default: > + /* should never reach this path */ > + return -EINVAL; > + } > + } > + > + regmap_write(priv->regmap, TIM_DIER, dier); Do you want to overwrite TIM_DIER completely, or did you mean to set only TIM_DIER_UIE and preserve the rest of the register? If the latter, you could redefine 'dier' as a bool and do: regmap_update_bits(priv->regmap, TIM_DIER, TIM_DIER_UIE, dier); There is also a regmap_update_bits_check() available if you want to combine the UIF latch check with the update; but I don't know if that will work in this case because it looks like you want to clear the UIF latch before enabling. > static int stm32_count_clk_get_freq(struct counter_device *counter, > @@ -418,6 +491,35 @@ static struct counter_count stm32_counts = { > .num_ext = ARRAY_SIZE(stm32_count_ext) > }; > > +static irqreturn_t stm32_timer_cnt_isr(int irq, void *ptr) > +{ > + struct counter_device *counter = ptr; > + struct stm32_timer_cnt *const priv = counter_priv(counter); > + u32 clr = GENMASK(31, 0); /* SR flags can be cleared by writing 0 (wr 1 has no effect) */ > + u32 sr, dier; > + > + regmap_read(priv->regmap, TIM_SR, &sr); > + regmap_read(priv->regmap, TIM_DIER, &dier); > + /* > + * Some status bits in SR don't match with the enable bits in DIER. Only take care of > + * the possibly enabled bits in DIER (that matches in between SR and DIER). > + */ > + dier &= TIM_DIER_UIE; > + sr &= dier; > + > + if (sr & TIM_SR_UIF) { Am I understanding this logic correctly? ANDing TIM_DIER_UIE with 'dier' will result in just the state of the TIM_DIER_UIE bit. Next, we AND that state with 'sr'; so sr is 0 when TIM_DIER_UIE state is low, but we get the respective SR bit when TIM_DIER_UIE state is high. Finally, we check the TIM_SR_UIF bit state in 'sr'. If TIM_SR_UIF bit position is expected to match the TIM_DIER_UIE bit position, then (sr & TIM_SR_UIF) will only be true when the state of both the TIM_DIER_UIE bit and TIM_SR_UIF bit are high. That means you can eliminate 'sr', 'dier', and the two regmap_read() operations with this instead: if (regmap_test_bits(priv->regmap, TIM_SR, TIM_SR_UIF) && regmap_test_bits(priv->regmap, TIM_DIER, TIM_DIER_UIE) { > + atomic_inc(&priv->nb_ovf); I wonder what happens when atomic_inc() increments past the atomic_t max value. Does atomic_read() report back a negative value? Do we need to guard against that scenario somehow? > + counter_push_event(counter, COUNTER_EVENT_OVERFLOW_UNDERFLOW, 0); > + dev_dbg(counter->parent, "COUNTER_EVENT_OVERFLOW_UNDERFLOW\n"); > + /* SR flags can be cleared by writing 0, only clear relevant flag */ > + clr &= ~TIM_SR_UIF; You can use u32p_replace_bits(&clr, 0, TIM_SR_UIF) instead after including the include/linux/bitfield.h header. > @@ -511,6 +615,32 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, priv); > > + /* STM32 Timers can have either 1 global, or 4 dedicated interrupts (optional) */ > + if (priv->nr_irqs == 1) { > + /* All events reported through the global interrupt */ > + ret = devm_request_irq(&pdev->dev, priv->irq[0], stm32_timer_cnt_isr, > + 0, dev_name(dev), counter); > + if (ret) { > + dev_err(dev, "Failed to request irq %d (err %d)\n", > + priv->irq[i], ret); This should be irq[0], right? I would also recommend using ddata->irq instead so we can get rid of priv->irq outside of this probe function. > + return ret; > + } > + } else { > + for (i = 0; i < priv->nr_irqs; i++) { > + /* Only take care of update IRQ for overflow events */ > + if (i != STM32_TIMERS_IRQ_UP) > + continue; > + > + ret = devm_request_irq(&pdev->dev, priv->irq[i], stm32_timer_cnt_isr, > + 0, dev_name(dev), counter); > + if (ret) { > + dev_err(dev, "Failed to request irq %d (err %d)\n", > + priv->irq[i], ret); > + return ret; > + } > + } So we only execute the loop body once for this particular STM32_TIMERS_IRQ_UP iteration? Why have the loop at all rather than hardcode irq[STM32_TIMERS_IRQ_UP] for devm_request_irq()? William Breathitt Gray
On Wed, Dec 20, 2023 at 03:57:25PM +0100, Fabrice Gasnier wrote: > diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h > index ca35af30745f..9eb17481b07f 100644 > --- a/include/linux/mfd/stm32-timers.h > +++ b/include/linux/mfd/stm32-timers.h > @@ -41,6 +41,11 @@ > #define TIM_SMCR_SMS (BIT(0) | BIT(1) | BIT(2)) /* Slave mode selection */ > #define TIM_SMCR_TS (BIT(4) | BIT(5) | BIT(6)) /* Trigger selection */ > #define TIM_DIER_UIE BIT(0) /* Update interrupt */ > +#define TIM_DIER_CC1IE BIT(1) /* CC1 Interrupt Enable */ > +#define TIM_DIER_CC2IE BIT(2) /* CC2 Interrupt Enable */ > +#define TIM_DIER_CC3IE BIT(3) /* CC3 Interrupt Enable */ > +#define TIM_DIER_CC4IE BIT(4) /* CC4 Interrupt Enable */ > +#define TIM_DIER_CC_IE(x) BIT((x) + 1) /* CC1, CC2, CC3, CC4 interrupt enable */ > #define TIM_DIER_UDE BIT(8) /* Update DMA request Enable */ > #define TIM_DIER_CC1DE BIT(9) /* CC1 DMA request Enable */ > #define TIM_DIER_CC2DE BIT(10) /* CC2 DMA request Enable */ > @@ -49,6 +54,7 @@ > #define TIM_DIER_COMDE BIT(13) /* COM DMA request Enable */ > #define TIM_DIER_TDE BIT(14) /* Trigger DMA request Enable */ > #define TIM_SR_UIF BIT(0) /* Update interrupt flag */ > +#define TIM_SR_CC_IF(x) BIT((x) + 1) /* CC1, CC2, CC3, CC4 interrupt flag */ > #define TIM_EGR_UG BIT(0) /* Update Generation */ > #define TIM_CCMR_PE BIT(3) /* Channel Preload Enable */ > #define TIM_CCMR_M1 (BIT(6) | BIT(5)) /* Channel PWM Mode 1 */ > @@ -60,16 +66,23 @@ > #define TIM_CCMR_CC1S_TI2 BIT(1) /* IC1/IC3 selects TI2/TI4 */ > #define TIM_CCMR_CC2S_TI2 BIT(8) /* IC2/IC4 selects TI2/TI4 */ > #define TIM_CCMR_CC2S_TI1 BIT(9) /* IC2/IC4 selects TI1/TI3 */ > +#define TIM_CCMR_CC3S (BIT(0) | BIT(1)) /* Capture/compare 3 sel */ > +#define TIM_CCMR_CC4S (BIT(8) | BIT(9)) /* Capture/compare 4 sel */ > +#define TIM_CCMR_CC3S_TI3 BIT(0) /* IC3 selects TI3 */ > +#define TIM_CCMR_CC4S_TI4 BIT(8) /* IC4 selects TI4 */ > #define TIM_CCER_CC1E BIT(0) /* Capt/Comp 1 out Ena */ > #define TIM_CCER_CC1P BIT(1) /* Capt/Comp 1 Polarity */ > #define TIM_CCER_CC1NE BIT(2) /* Capt/Comp 1N out Ena */ > #define TIM_CCER_CC1NP BIT(3) /* Capt/Comp 1N Polarity */ > #define TIM_CCER_CC2E BIT(4) /* Capt/Comp 2 out Ena */ > #define TIM_CCER_CC2P BIT(5) /* Capt/Comp 2 Polarity */ > +#define TIM_CCER_CC2NP BIT(7) /* Capt/Comp 2N Polarity */ > #define TIM_CCER_CC3E BIT(8) /* Capt/Comp 3 out Ena */ > #define TIM_CCER_CC3P BIT(9) /* Capt/Comp 3 Polarity */ > +#define TIM_CCER_CC3NP BIT(11) /* Capt/Comp 3N Polarity */ > #define TIM_CCER_CC4E BIT(12) /* Capt/Comp 4 out Ena */ > #define TIM_CCER_CC4P BIT(13) /* Capt/Comp 4 Polarity */ > +#define TIM_CCER_CC4NP BIT(15) /* Capt/Comp 4N Polarity */ I forgot to mention that you should move the introduction of these defines to the subsequent patch adding support for capture events because that's where the defines are actually used. William Breathitt Gray
On 1/8/24 22:00, William Breathitt Gray wrote: > On Wed, Dec 20, 2023 at 03:57:25PM +0100, Fabrice Gasnier wrote: >> Add support overflow events. Also add the related validation and >> configuration routine. Register and enable interrupts to push events. >> STM32 Timers can have either 1 global interrupt, or 4 dedicated interrupt >> lines. Request only the necessary interrupt, e.g. either global interrupt >> that can report all event types, or update interrupt only for overflow >> event. >> >> Acked-by: Lee Jones <lee@kernel.org> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> > > Hi Fabrice, Hi William, Sorry for the late reply, > > I've CC'd Will and Peter in case they can provide some suggestions > regarding my atomic_t comment inline below. I simply changed the type of nb_ovf below to u64, which better fits with the API IMHO. Please check in v4. > >> @@ -44,6 +45,9 @@ struct stm32_timer_cnt { >> bool has_encoder; >> u32 idx; >> unsigned int nchannels; >> + unsigned int nr_irqs; >> + u32 *irq; > > Looks like we only need this 'irq' array for registering the ISR in > stm32_timer_cnt_probe(). Since we won't need it anymore after that, > let's use ddata->irq directly instead of defining priv->irq. Ack. > >> + atomic_t nb_ovf; >> }; >> >> static const enum counter_function stm32_count_functions[] = { >> @@ -259,6 +263,29 @@ static int stm32_count_prescaler_write(struct counter_device *counter, >> return regmap_write(priv->regmap, TIM_PSC, psc); >> } >> >> +static int stm32_count_nb_ovf_read(struct counter_device *counter, >> + struct counter_count *count, u64 *val) >> +{ >> + struct stm32_timer_cnt *const priv = counter_priv(counter); >> + >> + *val = atomic_read(&priv->nb_ovf); >> + >> + return 0; >> +} >> + >> +static int stm32_count_nb_ovf_write(struct counter_device *counter, >> + struct counter_count *count, u64 val) >> +{ >> + struct stm32_timer_cnt *const priv = counter_priv(counter); >> + >> + if (val != (typeof(priv->nb_ovf.counter))val) >> + return -ERANGE; >> + >> + atomic_set(&priv->nb_ovf, val); > > So you want to check that the atomic_t 'nb_ovf' is able hold the value > provided by the u64 'val'. My understanding is that atomic_t should be > treated as an opaque type, so I don't think we should be accessing the > 'counter' member directly for this test (interrupt-cnt does this but I > believe it's wrong to do so). > > I don't know if we have any existing way to check for the value range of > an atomic_t (I don't see anything under include/linux/limits.h > specifically for it). However, you do use atomic_set() which takes an > int parameter, so perhaps we should compare against INT_MAX instead. Ack. Moving nb_ovf to u64 in v4 should address all these concerns. > >> +static int stm32_count_events_configure(struct counter_device *counter) >> +{ >> + struct stm32_timer_cnt *const priv = counter_priv(counter); >> + struct counter_event_node *event_node; >> + u32 val, dier = 0; >> + >> + list_for_each_entry(event_node, &counter->events_list, l) { >> + switch (event_node->event) { >> + case COUNTER_EVENT_OVERFLOW_UNDERFLOW: >> + /* first clear possibly latched UIF before enabling */ >> + regmap_read(priv->regmap, TIM_DIER, &val); >> + if (!(val & TIM_DIER_UIE)) > > You can eliminate 'val' and the regmap_read() line like this: > > if (!regmap_test_bits(priv->regmap, TIM_DIER, TIM_DIER_UIE)) Ack. Thanks for suggesting. > >> + regmap_write(priv->regmap, TIM_SR, (u32)~TIM_SR_UIF); >> + dier |= TIM_DIER_UIE; >> + break; >> + default: >> + /* should never reach this path */ >> + return -EINVAL; >> + } >> + } >> + >> + regmap_write(priv->regmap, TIM_DIER, dier); > > Do you want to overwrite TIM_DIER completely, or did you mean to set > only TIM_DIER_UIE and preserve the rest of the register? If the latter, > you could redefine 'dier' as a bool and do: > > regmap_update_bits(priv->regmap, TIM_DIER, TIM_DIER_UIE, dier); > > There is also a regmap_update_bits_check() available if you want to > combine the UIF latch check with the update; but I don't know if that > will work in this case because it looks like you want to clear the UIF > latch before enabling. As you've noticed, the subsequent patch answers this. Still I added a comment above this line. > >> static int stm32_count_clk_get_freq(struct counter_device *counter, >> @@ -418,6 +491,35 @@ static struct counter_count stm32_counts = { >> .num_ext = ARRAY_SIZE(stm32_count_ext) >> }; >> >> +static irqreturn_t stm32_timer_cnt_isr(int irq, void *ptr) >> +{ >> + struct counter_device *counter = ptr; >> + struct stm32_timer_cnt *const priv = counter_priv(counter); >> + u32 clr = GENMASK(31, 0); /* SR flags can be cleared by writing 0 (wr 1 has no effect) */ >> + u32 sr, dier; >> + >> + regmap_read(priv->regmap, TIM_SR, &sr); >> + regmap_read(priv->regmap, TIM_DIER, &dier); >> + /* >> + * Some status bits in SR don't match with the enable bits in DIER. Only take care of >> + * the possibly enabled bits in DIER (that matches in between SR and DIER). >> + */ >> + dier &= TIM_DIER_UIE; >> + sr &= dier; >> + >> + if (sr & TIM_SR_UIF) { > > Am I understanding this logic correctly? ANDing TIM_DIER_UIE with 'dier' > will result in just the state of the TIM_DIER_UIE bit. Next, we AND that > state with 'sr'; so sr is 0 when TIM_DIER_UIE state is low, but we get > the respective SR bit when TIM_DIER_UIE state is high. Finally, we check > the TIM_SR_UIF bit state in 'sr'. Same, next patch makes it more clear. > > If TIM_SR_UIF bit position is expected to match the TIM_DIER_UIE bit > position, then (sr & TIM_SR_UIF) will only be true when the state of > both the TIM_DIER_UIE bit and TIM_SR_UIF bit are high. That means you > can eliminate 'sr', 'dier', and the two regmap_read() operations with > this instead: > > if (regmap_test_bits(priv->regmap, TIM_SR, TIM_SR_UIF) && > regmap_test_bits(priv->regmap, TIM_DIER, TIM_DIER_UIE) { > >> + atomic_inc(&priv->nb_ovf); > > I wonder what happens when atomic_inc() increments past the atomic_t max > value. Does atomic_read() report back a negative value? Do we need to > guard against that scenario somehow? Ack, nb_ovf moved to u64 in patch v4. So negative value shouldn't be an issue. Thanks for pointing this. > >> + counter_push_event(counter, COUNTER_EVENT_OVERFLOW_UNDERFLOW, 0); >> + dev_dbg(counter->parent, "COUNTER_EVENT_OVERFLOW_UNDERFLOW\n"); >> + /* SR flags can be cleared by writing 0, only clear relevant flag */ >> + clr &= ~TIM_SR_UIF; > > You can use u32p_replace_bits(&clr, 0, TIM_SR_UIF) instead after > including the include/linux/bitfield.h header. Thanks for suggesting. I tried this here, it seems fine. However, in subsequent patch, doing the same change with TIM_SR_CC_IF(i) macro gives a build error. To be consistent in between the 2 patchs, I prefer to keep simple bit ops here. > >> @@ -511,6 +615,32 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev) >> >> platform_set_drvdata(pdev, priv); >> >> + /* STM32 Timers can have either 1 global, or 4 dedicated interrupts (optional) */ >> + if (priv->nr_irqs == 1) { >> + /* All events reported through the global interrupt */ >> + ret = devm_request_irq(&pdev->dev, priv->irq[0], stm32_timer_cnt_isr, >> + 0, dev_name(dev), counter); >> + if (ret) { >> + dev_err(dev, "Failed to request irq %d (err %d)\n", >> + priv->irq[i], ret); > > This should be irq[0], right? Yes. > > I would also recommend using ddata->irq instead so we can get rid of > priv->irq outside of this probe function. Done. Thanks for reviewing BR, Fabrice > >> + return ret; >> + } >> + } else { >> + for (i = 0; i < priv->nr_irqs; i++) { >> + /* Only take care of update IRQ for overflow events */ >> + if (i != STM32_TIMERS_IRQ_UP) >> + continue; >> + >> + ret = devm_request_irq(&pdev->dev, priv->irq[i], stm32_timer_cnt_isr, >> + 0, dev_name(dev), counter); >> + if (ret) { >> + dev_err(dev, "Failed to request irq %d (err %d)\n", >> + priv->irq[i], ret); >> + return ret; >> + } >> + } > > So we only execute the loop body once for this particular > STM32_TIMERS_IRQ_UP iteration? Why have the loop at all rather than > hardcode irq[STM32_TIMERS_IRQ_UP] for devm_request_irq()? > > William Breathitt Gray
diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c index b5dc4378fecf..d13e4c427965 100644 --- a/drivers/counter/stm32-timer-cnt.c +++ b/drivers/counter/stm32-timer-cnt.c @@ -8,6 +8,7 @@ * */ #include <linux/counter.h> +#include <linux/interrupt.h> #include <linux/mfd/stm32-timers.h> #include <linux/mod_devicetable.h> #include <linux/module.h> @@ -44,6 +45,9 @@ struct stm32_timer_cnt { bool has_encoder; u32 idx; unsigned int nchannels; + unsigned int nr_irqs; + u32 *irq; + atomic_t nb_ovf; }; static const enum counter_function stm32_count_functions[] = { @@ -259,6 +263,29 @@ static int stm32_count_prescaler_write(struct counter_device *counter, return regmap_write(priv->regmap, TIM_PSC, psc); } +static int stm32_count_nb_ovf_read(struct counter_device *counter, + struct counter_count *count, u64 *val) +{ + struct stm32_timer_cnt *const priv = counter_priv(counter); + + *val = atomic_read(&priv->nb_ovf); + + return 0; +} + +static int stm32_count_nb_ovf_write(struct counter_device *counter, + struct counter_count *count, u64 val) +{ + struct stm32_timer_cnt *const priv = counter_priv(counter); + + if (val != (typeof(priv->nb_ovf.counter))val) + return -ERANGE; + + atomic_set(&priv->nb_ovf, val); + + return 0; +} + static struct counter_comp stm32_count_ext[] = { COUNTER_COMP_DIRECTION(stm32_count_direction_read), COUNTER_COMP_ENABLE(stm32_count_enable_read, stm32_count_enable_write), @@ -266,6 +293,7 @@ static struct counter_comp stm32_count_ext[] = { stm32_count_ceiling_write), COUNTER_COMP_COUNT_U64("prescaler", stm32_count_prescaler_read, stm32_count_prescaler_write), + COUNTER_COMP_COUNT_U64("num_overflows", stm32_count_nb_ovf_read, stm32_count_nb_ovf_write), }; static const enum counter_synapse_action stm32_clock_synapse_actions[] = { @@ -323,12 +351,57 @@ static int stm32_action_read(struct counter_device *counter, } } +static int stm32_count_events_configure(struct counter_device *counter) +{ + struct stm32_timer_cnt *const priv = counter_priv(counter); + struct counter_event_node *event_node; + u32 val, dier = 0; + + list_for_each_entry(event_node, &counter->events_list, l) { + switch (event_node->event) { + case COUNTER_EVENT_OVERFLOW_UNDERFLOW: + /* first clear possibly latched UIF before enabling */ + regmap_read(priv->regmap, TIM_DIER, &val); + if (!(val & TIM_DIER_UIE)) + regmap_write(priv->regmap, TIM_SR, (u32)~TIM_SR_UIF); + dier |= TIM_DIER_UIE; + break; + default: + /* should never reach this path */ + return -EINVAL; + } + } + + regmap_write(priv->regmap, TIM_DIER, dier); + + return 0; +} + +static int stm32_count_watch_validate(struct counter_device *counter, + const struct counter_watch *watch) +{ + struct stm32_timer_cnt *const priv = counter_priv(counter); + + /* Interrupts are optional */ + if (!priv->nr_irqs) + return -EOPNOTSUPP; + + switch (watch->event) { + case COUNTER_EVENT_OVERFLOW_UNDERFLOW: + return 0; + default: + return -EINVAL; + } +} + static const struct counter_ops stm32_timer_cnt_ops = { .count_read = stm32_count_read, .count_write = stm32_count_write, .function_read = stm32_count_function_read, .function_write = stm32_count_function_write, .action_read = stm32_action_read, + .events_configure = stm32_count_events_configure, + .watch_validate = stm32_count_watch_validate, }; static int stm32_count_clk_get_freq(struct counter_device *counter, @@ -418,6 +491,35 @@ static struct counter_count stm32_counts = { .num_ext = ARRAY_SIZE(stm32_count_ext) }; +static irqreturn_t stm32_timer_cnt_isr(int irq, void *ptr) +{ + struct counter_device *counter = ptr; + struct stm32_timer_cnt *const priv = counter_priv(counter); + u32 clr = GENMASK(31, 0); /* SR flags can be cleared by writing 0 (wr 1 has no effect) */ + u32 sr, dier; + + regmap_read(priv->regmap, TIM_SR, &sr); + regmap_read(priv->regmap, TIM_DIER, &dier); + /* + * Some status bits in SR don't match with the enable bits in DIER. Only take care of + * the possibly enabled bits in DIER (that matches in between SR and DIER). + */ + dier &= TIM_DIER_UIE; + sr &= dier; + + if (sr & TIM_SR_UIF) { + atomic_inc(&priv->nb_ovf); + counter_push_event(counter, COUNTER_EVENT_OVERFLOW_UNDERFLOW, 0); + dev_dbg(counter->parent, "COUNTER_EVENT_OVERFLOW_UNDERFLOW\n"); + /* SR flags can be cleared by writing 0, only clear relevant flag */ + clr &= ~TIM_SR_UIF; + } + + regmap_write(priv->regmap, TIM_SR, clr); + + return IRQ_HANDLED; +}; + static void stm32_timer_cnt_detect_channels(struct platform_device *pdev, struct stm32_timer_cnt *priv) { @@ -480,7 +582,7 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct stm32_timer_cnt *priv; struct counter_device *counter; - int ret; + int i, ret; if (IS_ERR_OR_NULL(ddata)) return -EINVAL; @@ -494,6 +596,8 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev) priv->regmap = ddata->regmap; priv->clk = ddata->clk; priv->max_arr = ddata->max_arr; + priv->nr_irqs = ddata->nr_irqs; + priv->irq = ddata->irq; ret = stm32_timer_cnt_probe_encoder(pdev, priv); if (ret) @@ -511,6 +615,32 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev) platform_set_drvdata(pdev, priv); + /* STM32 Timers can have either 1 global, or 4 dedicated interrupts (optional) */ + if (priv->nr_irqs == 1) { + /* All events reported through the global interrupt */ + ret = devm_request_irq(&pdev->dev, priv->irq[0], stm32_timer_cnt_isr, + 0, dev_name(dev), counter); + if (ret) { + dev_err(dev, "Failed to request irq %d (err %d)\n", + priv->irq[i], ret); + return ret; + } + } else { + for (i = 0; i < priv->nr_irqs; i++) { + /* Only take care of update IRQ for overflow events */ + if (i != STM32_TIMERS_IRQ_UP) + continue; + + ret = devm_request_irq(&pdev->dev, priv->irq[i], stm32_timer_cnt_isr, + 0, dev_name(dev), counter); + if (ret) { + dev_err(dev, "Failed to request irq %d (err %d)\n", + priv->irq[i], ret); + return ret; + } + } + } + /* Reset input selector to its default input */ regmap_write(priv->regmap, TIM_TISEL, 0x0); diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h index ca35af30745f..9eb17481b07f 100644 --- a/include/linux/mfd/stm32-timers.h +++ b/include/linux/mfd/stm32-timers.h @@ -41,6 +41,11 @@ #define TIM_SMCR_SMS (BIT(0) | BIT(1) | BIT(2)) /* Slave mode selection */ #define TIM_SMCR_TS (BIT(4) | BIT(5) | BIT(6)) /* Trigger selection */ #define TIM_DIER_UIE BIT(0) /* Update interrupt */ +#define TIM_DIER_CC1IE BIT(1) /* CC1 Interrupt Enable */ +#define TIM_DIER_CC2IE BIT(2) /* CC2 Interrupt Enable */ +#define TIM_DIER_CC3IE BIT(3) /* CC3 Interrupt Enable */ +#define TIM_DIER_CC4IE BIT(4) /* CC4 Interrupt Enable */ +#define TIM_DIER_CC_IE(x) BIT((x) + 1) /* CC1, CC2, CC3, CC4 interrupt enable */ #define TIM_DIER_UDE BIT(8) /* Update DMA request Enable */ #define TIM_DIER_CC1DE BIT(9) /* CC1 DMA request Enable */ #define TIM_DIER_CC2DE BIT(10) /* CC2 DMA request Enable */ @@ -49,6 +54,7 @@ #define TIM_DIER_COMDE BIT(13) /* COM DMA request Enable */ #define TIM_DIER_TDE BIT(14) /* Trigger DMA request Enable */ #define TIM_SR_UIF BIT(0) /* Update interrupt flag */ +#define TIM_SR_CC_IF(x) BIT((x) + 1) /* CC1, CC2, CC3, CC4 interrupt flag */ #define TIM_EGR_UG BIT(0) /* Update Generation */ #define TIM_CCMR_PE BIT(3) /* Channel Preload Enable */ #define TIM_CCMR_M1 (BIT(6) | BIT(5)) /* Channel PWM Mode 1 */ @@ -60,16 +66,23 @@ #define TIM_CCMR_CC1S_TI2 BIT(1) /* IC1/IC3 selects TI2/TI4 */ #define TIM_CCMR_CC2S_TI2 BIT(8) /* IC2/IC4 selects TI2/TI4 */ #define TIM_CCMR_CC2S_TI1 BIT(9) /* IC2/IC4 selects TI1/TI3 */ +#define TIM_CCMR_CC3S (BIT(0) | BIT(1)) /* Capture/compare 3 sel */ +#define TIM_CCMR_CC4S (BIT(8) | BIT(9)) /* Capture/compare 4 sel */ +#define TIM_CCMR_CC3S_TI3 BIT(0) /* IC3 selects TI3 */ +#define TIM_CCMR_CC4S_TI4 BIT(8) /* IC4 selects TI4 */ #define TIM_CCER_CC1E BIT(0) /* Capt/Comp 1 out Ena */ #define TIM_CCER_CC1P BIT(1) /* Capt/Comp 1 Polarity */ #define TIM_CCER_CC1NE BIT(2) /* Capt/Comp 1N out Ena */ #define TIM_CCER_CC1NP BIT(3) /* Capt/Comp 1N Polarity */ #define TIM_CCER_CC2E BIT(4) /* Capt/Comp 2 out Ena */ #define TIM_CCER_CC2P BIT(5) /* Capt/Comp 2 Polarity */ +#define TIM_CCER_CC2NP BIT(7) /* Capt/Comp 2N Polarity */ #define TIM_CCER_CC3E BIT(8) /* Capt/Comp 3 out Ena */ #define TIM_CCER_CC3P BIT(9) /* Capt/Comp 3 Polarity */ +#define TIM_CCER_CC3NP BIT(11) /* Capt/Comp 3N Polarity */ #define TIM_CCER_CC4E BIT(12) /* Capt/Comp 4 out Ena */ #define TIM_CCER_CC4P BIT(13) /* Capt/Comp 4 Polarity */ +#define TIM_CCER_CC4NP BIT(15) /* Capt/Comp 4N Polarity */ #define TIM_CCER_CCXE (BIT(0) | BIT(4) | BIT(8) | BIT(12)) #define TIM_BDTR_BKE(x) BIT(12 + (x) * 12) /* Break input enable */ #define TIM_BDTR_BKP(x) BIT(13 + (x) * 12) /* Break input polarity */