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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7053e5e5-289a-00d0-8f18-41d1cfc52e9d@oss.nxp.com>
Date: Mon, 31 Mar 2025 13:35:33 +0300
From: Ghennadi Procopciuc <ghennadi.procopciuc@....nxp.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>, tglx@...utronix.de
Cc: linux-kernel@...r.kernel.org, thomas.fossati@...aro.org,
 Larisa.Grigore@....com, ghennadi.procopciuc@....com, krzk@...nel.org,
 S32@....com, Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 "moderated list:ARM/STM32 ARCHITECTURE"
 <linux-stm32@...md-mailman.stormreply.com>,
 "moderated list:ARM/STM32 ARCHITECTURE"
 <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 2/2] clocksource/drivers/nxp-timer: Add the System
 Timer Module for the s32g platform

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