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@xxxxxxxxxx> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxxxxxxx> > > 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