Received-SPF: pass (google.com: domain of linux-kernel+bounces-83760-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <454f8d0f-9801-4caf-a1a6-737525ad947b@foss.st.com>
Date: Tue, 27 Feb 2024 18:43:43 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 09/10] counter: stm32-timer-cnt: add support for
 overflow events
Content-Language: en-US
To: William Breathitt Gray <william.gray@linaro.org>
CC: <lee@kernel.org>, <alexandre.torgue@foss.st.com>,
        <linux-iio@vger.kernel.org>,
        <linux-stm32@st-md-mailman.stormreply.com>,
        <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
        Will
 Deacon <will@kernel.org>, Peter Zijlstra <peterz@infradead.org>
References: <20231220145726.640627-1-fabrice.gasnier@foss.st.com>
 <20231220145726.640627-10-fabrice.gasnier@foss.st.com>
 <ZZxidRueG8H/O7pw@ubuntu-server-vm-macos>
From: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
In-Reply-To: <ZZxidRueG8H/O7pw@ubuntu-server-vm-macos>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

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