Re: [PATCH v2 2/2] clocksource/drivers/nxp-timer: Add the System Timer Module for the s32g platform

[Ghennadi Procopciuc <ghennadi.procopciuc@oss.nxp.com>]

On 3/28/2025 3:42 PM, Daniel Lezcano wrote:
> STM supports commonly required system and application software timing
> functions. STM includes a 32-bit count-up timer and four 32-bit
> compare channels with a separate interrupt source for each
> channel. The timer is driven by the STM module clock divided by an
> 8-bit prescale value (1 to 256).
> 
> STM has the following features:
>     • One 32-bit count-up timer with an 8-bit prescaler
>     • Four 32-bit compare channels
>     • An independent interrupt source for each channel
>     • Ability to stop the timer in Debug mode
> 
> The s32g platform is declined into two versions, the s32g2 and the
> s32g3. The former has a STM block with 8 timers and the latter has 12
> timers.
> 
> The platform is designed to have one usable STM instance per core on
> the system which is composed of 3 x Cortex-M3 + 4 Cortex-A53 for the
> s32g2 and 3 x Cortex-M3 + 8 Cortex-A53.
Missing: ' ... for S32G3' ?

> 
> There is a special STM instance called STM_TS which is dedicated to
> the timestamp. The 7th STM instance STM_07 is directly tied to the
> STM_TS which means it is not usable as a clockevent.
> 
> The driver instanciate each STM instance described in the device tree
instanciate  - > instantiate ?

[ ... ]
> +#include <linux/clk.h>
> +#include <linux/clockchips.h>
> +#include <linux/cpuhotplug.h>
> +#include <linux/debugfs.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/sched_clock.h>
> +#include <linux/units.h>
> +
> +#define STM_CR(__base)		(__base)
> +
> +#define STM_CR_TEN		BIT(0)
> +#define STM_CR_FRZ		BIT(1)
> +#define STM_CR_CPS_OFFSET	8u
> +#define STM_CR_CPS_MASK		GENMASK(15, STM_CR_CPS_OFFSET)
> +
> +#define STM_CNT(__base)		(__base + 0x04)

Shouldn't the macro arguments be enclosed in parentheses to avoid
potential evaluation order issues?

[ ... ]
> +static int __init nxp_stm_timer_probe(struct platform_device *pdev)
> +{
> +	struct stm_timer *stm_timer;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	const char *name = of_node_full_name(np);
> +	struct clk *clk;
> +	void __iomem *base;
> +	int irq, ret;
> +
> +	/*
> +	 * The device tree can have multiple STM nodes described, so
> +	 * it makes this driver a good candidate for the async probe.
> +	 * It is still unclear if the time framework does correctly
> +	 * handle a parallel loading of the timers but at least this
> +	 * driver is ready to support the option.
> +	 */
> +	guard(stm_instances)(&stm_instances_lock);
> +
> +	/*
> +	 * The S32Gx are SoCs featuring a diverse set of cores. Linux
> +	 * is expected to run on Cortex-A53 cores, while other
> +	 * software stacks will operate on Cortex-M cores. The number
> +	 * of STM instances has been sized to include at most one
> +	 * instance per core.
> +	 *
> +	 * As we need a clocksource and a clockevent per cpu, we
> +	 * simply initialize a clocksource per cpu along with the
> +	 * clockevent which makes the resulting code simpler.
> +	 *
> +	 * However if the device tree is describing more STM instances
> +	 * than the number of cores, then we ignore them.
> +	 */
> +	if (stm_instances >= num_possible_cpus())
> +		return 0;
> +
> +	base = devm_of_iomap(dev, np, 0, NULL);
> +	if (IS_ERR(base))
> +		return dev_err_probe(dev, PTR_ERR(base), "Failed to iomap %pOFn\n", np);
> +
> +	irq = irq_of_parse_and_map(np, 0);
> +	if (irq <= 0)
> +		return dev_err_probe(dev, irq, "Failed to parse and map IRQ\n");
> +
> +	ret = devm_add_action_or_reset(dev, devm_irq_dispose_mapping, &irq);
> +	if (ret) {
> +		irq_dispose_mapping(irq);
> +		return dev_err_probe(dev, ret, "Failed to add devm action\n");
> +	}

Wouldn't platform_get_irq achieve the same result, but with less hassle?

-- 
Regards,
Ghennadi