Re: [PATCH v4 3/3] counter: Add STM based counter

[Daniel Lezcano]

On 1/8/26 17:41, Frank Li wrote:
> On Wed, Jan 07, 2026 at 02:39:52PM +0100, Daniel Lezcano wrote:
> > The NXP S32G2 automotive platform integrates four Cortex-A53 cores and
> > three Cortex-M7 cores, along with a large number of timers and
> > counters. These hardware blocks can be used as clocksources or
> > clockevents, or as timestamp counters shared across the various
> > subsystems running alongside the Linux kernel, such as firmware
> > components. Their actual usage depends on the overall platform
> > software design.
> >
> > In a Linux-based system, the kernel controls the counter, which is a
> > read-only shared resource for the other subsystems. One of its primary
> > purposes is to act as a common timestamp source for messages or
> > traces, allowing correlation of events occurring in different
> > operating system contexts.
> >
> > These changes introduce a basic counter driver that can start, stop,
> > and reset the counter. It also handles overflow accounting and
> > configures the prescaler value.
> >
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > ---

[ ... ]

> > +
> > +static struct counter_comp stm_cnt_count_ext[] = {
> > +	COUNTER_COMP_COUNT_U8("prescaler", nxp_stm_cnt_prescaler_read, nxp_stm_cnt_prescaler_write),
> > +	COUNTER_COMP_ENABLE(nxp_stm_cnt_count_enable_read, nxp_stm_cnt_count_enable_write),
> > +};
> > +
> > +static int nxp_stm_cnt_action_read(struct counter_device *counter,
> > +				   struct counter_count *count,
> > +				   struct counter_synapse *synapse,
> > +				   enum counter_synapse_action *action)
> > +{
> > +	*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> > +
> > +	return 0;
> > +}
> > +
> > +static int nxp_stm_cnt_count_read(struct counter_device *dev,
> > +				  struct counter_count *count, u64 *val)
> > +{
> > +	struct nxp_stm_cnt *stm_cnt = counter_priv(dev);
> > +	unsigned long irqflags;
> > +
> > +	spin_lock_irqsave(&stm_cnt->lock, irqflags);
> > +	*val = stm_cnt->counter + nxp_stm_cnt_get_counter(stm_cnt);
> > +	spin_unlock_irqrestore(&stm_cnt->lock, irqflags);
>
> It think there are still rise condition here.
>
> cpu 0                                            cpu 1
> 1:  nxp_stm_cnt_get_counter(stm_cnt); (0)
> 2: 						 irq_handle()
> 					         counter += INT_MAX;
> 3:
>
> when wrap happen, nxp_stm_cnt_get_counter() return 0, but, irq still not
> handle yet.
>
> so nxp_stm_cnt_count_read() return 0 at 1.
>
> it return INT_MAX at 3 suddently.

Thanks for the review. Yes, it seems like there is a race for the reader 
result.

Except disabling / enabling the counter when reading the value, I don't 
see how to handle that case.



bool is_started = nxp_stm_cnt_is_started(stm_cnt);

if (is_started)
	nxp_stm_cnt_disable(stm_cnt);

/* If the counter wrapped, the interrupt fired already */

spin_lock_irqsave(&stm_cnt->lock, irqflags);

/* Counter can be zero (or more), we don't care, UINT_MAX was added */
*val = stm_cnt->counter + nxp_stm_cnt_get_counter(stm_cnt);

spin_unlock_irqrestore(&stm_cnt->lock, irqflags);

if (is_started)
	nxp_stm_cnt_enable(stm_cnt);





--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog