On 1/8/24 17:46, William Breathitt Gray wrote:
> On Wed, Dec 20, 2023 at 03:57:20PM +0100, Fabrice Gasnier wrote:
>> Introduce the internal clock signal, used to count when in simple rising
>> function. Also add the "frequency" extension to the clock signal.
>>
>> With this patch, signal action reports a consistent state when "increase"
>> function is used, and the counting frequency:
>> $ echo increase > function
>> $ grep -H "" signal*_action
>> signal0_action:none
>> signal1_action:none
>> signal2_action:rising edge
>> $ echo 1 > enable
>> $ cat count
>> 25425
>> $ cat count
>> 44439
>> $ cat ../signal2/frequency
>> 208877930
>>
>> Signed-off-by: Fabrice Gasnier <[email protected]>
>
> Reviewed-by: William Breathitt Gray <[email protected]>
>
> The code is all right, but some minor suggestions below.
>
>> +static struct counter_comp stm32_count_clock_ext[] = {
>> + COUNTER_COMP_SIGNAL_U64("frequency", stm32_count_clk_get_freq, NULL),
>
> It might be worth introducing a new COUNTER_COMP_FREQUENCY() macro now
> that we have a second driver with the 'frequency' extension
> (ti-ecap-capture also has 'frequency'). But it's up to you if you want
> to add a precursor patch to this series, or I'll introduce it separately
> myself in a independent patch.

Thanks for suggesting.

I added a precursor patch to this series.
I guess you wishes to see it used in both ti-ecap-capture and
stm32-timer-cnt. I only cared about stm32-timer-cnt in this series.

Can I let you do ti-ecap-capture change if/when you're going to apply it?

>
>> @@ -287,7 +321,13 @@ static struct counter_signal stm32_signals[] = {
>> {
>> .id = STM32_CH2_SIG,
>> .name = "Channel 2"
>> - }
>> + },
>> + {
>> + .id = STM32_CLOCK_SIG,
>> + .name = "Clock Signal",
>
> The word "Signal" feels unnecessary to me when both the sysfs path and
> data structure will have 'signal' already. Do you think "Clock" by
> itself is clear enough?

Agreed, I updated in v4.

Best Regards,
Fabrice

>
> William Breathitt Gray

2024-02-27 17:44:47

by Fabrice Gasnier

[permalink] [raw]

Subject: Re: [PATCH v3 10/10] counter: stm32-timer-cnt: add support for capture events

On 1/8/24 23:07, William Breathitt Gray wrote:
> On Wed, Dec 20, 2023 at 03:57:26PM +0100, Fabrice Gasnier wrote:
>> + /*
>> + * configure channel in input capture mode, map channel 1 on TI1, channel2 on TI2...
>> + * Select both edges / non-inverted to trigger a capture.
>> + */
>
> I suggest defining a new local variable 'cc' to point to stm32_cc[ch]. I
> think that's make the code look nicer here to avoid all the array index
> syntax every time you access stm32_cc[ch].

Hi William,

Thanks for suggesting.
Done.

>
>> + if (enable) {
>> + /* first clear possibly latched capture flag upon enabling */
>> + regmap_read(priv->regmap, TIM_CCER, &ccer);
>> + if (!(ccer & stm32_cc[ch].ccer_bits)) {
>
> Try regmap_test_bits() here instead of using regmap_read().

Done,

>
>> + sr = ~TIM_SR_CC_IF(ch);
>> + regmap_write(priv->regmap, TIM_SR, sr);
>
> Eliminate 'sr' by regmap_write(priv->regmap, TIM_SR, ~TIM_SR_CC_IF(ch)).
>
>> @@ -366,6 +460,12 @@ static int stm32_count_events_configure(struct counter_device *counter)
>> regmap_write(priv->regmap, TIM_SR, (u32)~TIM_SR_UIF);
>> dier |= TIM_DIER_UIE;
>> break;
>> + case COUNTER_EVENT_CAPTURE:
>> + ret = stm32_count_capture_configure(counter, event_node->channel, true);
>> + if (ret)
>> + return ret;
>> + dier |= TIM_DIER_CC_IE(event_node->channel);
>
> Ah, now I understand why the previous patch OR'd TIM_DIER_UIE to dier.
> Apologies for the noise.
>
>> @@ -374,6 +474,15 @@ static int stm32_count_events_configure(struct counter_device *counter)
>>
>> regmap_write(priv->regmap, TIM_DIER, dier);
>>
>> + /* check for disabled capture events */
>> + for (i = 0 ; i < priv->nchannels; i++) {
>> + if (!(dier & TIM_DIER_CC_IE(i))) {
>> + ret = stm32_count_capture_configure(counter, i, false);
>> + if (ret)
>> + return ret;
>> + }
>
> Would for_each_clear_bitrange() in linux/find.h work for this loop?

I had a look, but it requires to add some variables, for start and stop
bit in the bitmap. For now, I've kept the simple BIT macro and bit ops.

>
>> @@ -504,7 +620,7 @@ static irqreturn_t stm32_timer_cnt_isr(int irq, void *ptr)
>> * 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;
>> + dier &= (TIM_DIER_UIE | TIM_DIER_CC1IE | TIM_DIER_CC2IE | TIM_DIER_CC3IE | TIM_DIER_CC4IE);
>
> Again, sorry for the noise on the previous patch; this makes sense now.
>
>> @@ -515,6 +631,15 @@ static irqreturn_t stm32_timer_cnt_isr(int irq, void *ptr)
>> clr &= ~TIM_SR_UIF;
>> }
>>
>> + /* Check capture events */
>> + for (i = 0 ; i < priv->nchannels; i++) {
>> + if (sr & TIM_SR_CC_IF(i)) {
>
> Would for_each_set_bitrange() in linux/find.h work for this loop?

same.

>
>> + counter_push_event(counter, COUNTER_EVENT_CAPTURE, i);
>> + clr &= ~TIM_SR_CC_IF(i);
>
> Perhaps u32p_replace_bits(&clr, 0, TIM_SR_CC_IF(i)) is clearer here.

I've hit some build issue with TIM_SR_CC_IF(i) macro, e.g.:
/include/linux/bitfield.h:165:17: error: call to ‘__bad_mask’ declared
with attribute error: bad bitfield mask
165 | __bad_mask();

So I've kept the simple bit operation here.

Thanks & Best Regards,
Fabrice

>
>> @@ -627,8 +752,11 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
>> }
>> } else {
>> for (i = 0; i < priv->nr_irqs; i++) {
>> - /* Only take care of update IRQ for overflow events */
>> - if (i != STM32_TIMERS_IRQ_UP)
>> + /*
>> + * Only take care of update IRQ for overflow events, and cc for
>> + * capture events.
>> + */
>> + if (i != STM32_TIMERS_IRQ_UP && i != STM32_TIMERS_IRQ_CC)
>> continue;
>
> Okay, I see now why you have this check. This should be fine as it'll
> makes adding support in the future for the other IRQs a less invasive
> change.
>
> William Breathitt Gray

2024-02-27 17:45:02

by Fabrice Gasnier

[permalink] [raw]

Subject: Re: [PATCH v3 09/10] counter: stm32-timer-cnt: add support for overflow events

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 <[email protected]>
>> Signed-off-by: Fabrice Gasnier <[email protected]>
>
> 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

2024-02-29 18:58:27

by William Breathitt Gray

[permalink] [raw]

Subject: Re: [PATCH v3 04/10] counter: stm32-timer-cnt: introduce clock signal

On Tue, Feb 27, 2024 at 06:43:20PM +0100, Fabrice Gasnier wrote:
> On 1/8/24 17:46, William Breathitt Gray wrote:
> > On Wed, Dec 20, 2023 at 03:57:20PM +0100, Fabrice Gasnier wrote:
> >> Introduce the internal clock signal, used to count when in simple rising
> >> function. Also add the "frequency" extension to the clock signal.
> >>
> >> With this patch, signal action reports a consistent state when "increase"
> >> function is used, and the counting frequency:
> >> $ echo increase > function
> >> $ grep -H "" signal*_action
> >> signal0_action:none
> >> signal1_action:none
> >> signal2_action:rising edge
> >> $ echo 1 > enable
> >> $ cat count
> >> 25425
> >> $ cat count
> >> 44439
> >> $ cat ../signal2/frequency
> >> 208877930
> >>
> >> Signed-off-by: Fabrice Gasnier <[email protected]>
> >
> > Reviewed-by: William Breathitt Gray <[email protected]>
> >
> > The code is all right, but some minor suggestions below.
> >
> >> +static struct counter_comp stm32_count_clock_ext[] = {
> >> + COUNTER_COMP_SIGNAL_U64("frequency", stm32_count_clk_get_freq, NULL),
> >
> > It might be worth introducing a new COUNTER_COMP_FREQUENCY() macro now
> > that we have a second driver with the 'frequency' extension
> > (ti-ecap-capture also has 'frequency'). But it's up to you if you want
> > to add a precursor patch to this series, or I'll introduce it separately
> > myself in a independent patch.
>
> Thanks for suggesting.
>
> I added a precursor patch to this series.
> I guess you wishes to see it used in both ti-ecap-capture and
> stm32-timer-cnt. I only cared about stm32-timer-cnt in this series.
>
> Can I let you do ti-ecap-capture change if/when you're going to apply it?

Thanks Fabrice, I'll pick up the precursor patch so we an start using it
in other drivers. Syed will take on the ti-ecap-capture change, and I've
CC'd Vignesh and Julien to this email as well so they are aware of the
incoming patch.

William Breathitt Gray

Attachments:

(No filename) (2.02 kB)
signature.asc (235.00 B)
Download all attachments