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

[Daniel Lezcano]

[Added s32@ list which I miss from the initial series]

On 25/03/2025 08:30, Krzysztof Kozlowski wrote:
> On 24/03/2025 11:00, Daniel Lezcano wrote:
> > +
> > +static int __init nxp_stm_clocksource_init(struct device *dev, const char *name,
> > +					   void __iomem *base, struct clk *clk)
> > +{
> > +	struct stm_timer *stm_timer;
> > +	int ret;
> > +
> > +	stm_timer = devm_kzalloc(dev, sizeof(*stm_timer), GFP_KERNEL);
> > +	if (!stm_timer)
> > +		return -ENOMEM;
> > +
> > +	stm_timer->base = base;
> > +	stm_timer->rate = clk_get_rate(clk);
> > +
> > +	stm_timer->scs.cs.name = name;
>
> You are aware that all node names will have exactly the same name? All
> of them will be called "timer"?

From the caller const char *name = of_node_full_name(np);

The names are:

"clocksource: Switched to clocksource stm@4011c000"

Or:

 17:      24150          0          0          0    GICv3  57 Level 
stm@40120000
 18:          0      22680          0          0    GICv3  58 Level 
stm@40124000
 19:          0          0      24110          0    GICv3  59 Level 
stm@40128000
 20:          0          0          0      21164    GICv3  60 Level 
stm@4021c000

And:

cat /sys/devices/system/clocksource/clocksource0/current_clocksource
stm@4011c000

cat /sys/devices/system/clockevents/clockevent*/current_device
stm@40120000
stm@40124000
stm@40128000
stm@4021c000

[ ... ]

> > +
> > +static int __init nxp_stm_timer_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct stm_instances *stm_instances;
> > +	const char *name = of_node_full_name(np);
> > +	void __iomem *base;
> > +	int irq, ret;
> > +	struct clk *clk;
> > +
> > +	stm_instances = (typeof(stm_instances))of_device_get_match_data(dev);
>
> No, you *cannot* drop the const. It's there on purpose. Match data
> should be const because it defines per variant differences. That's why
> the prototype of of_device_get_match_data() has such return type.
> You just want some global singleton, not match data.
>
> > +	if (!stm_instances) {
> > +		dev_err(dev, "No STM instances associated with a cpu");
> > +		return -EINVAL;
> > +	}
> > +
> > +	base = devm_of_iomap(dev, np, 0, NULL);
> > +	if (IS_ERR(base)) {
> > +		dev_err(dev, "Failed to iomap %pOFn\n", np);
>
> You need to clean up the downstream code to match upstream. All of these
> should be return dev_err_probe().

Oh right, thanks for the reminder

> > +		return PTR_ERR(base);
> > +	}
> > +
> > +	irq = irq_of_parse_and_map(np, 0);
> > +	if (irq <= 0) {
> > +		dev_err(dev, "Failed to parse and map IRQ: %d\n", irq);
> > +		return -EINVAL;
> > +	}
> > +
> > +	clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(clk)) {
> > +		dev_err(dev, "Clock not found\n");
>
> And this is clearly incorrect - spamming logs. Syntax is:
> return dev_err_probe
>
> > +		return PTR_ERR(clk);
> > +	}
> > +
> > +	ret = clk_prepare_enable(clk);
>
> Drop, devm_clk_get_enabled.
>
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable STM timer clock: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	if (!stm_instances->clocksource && (stm_instances->features & STM_CLKSRC)) {
> > +
> > +		/*
> > +		 * First probed STM will be a clocksource
> > +		 */
> > +		ret = nxp_stm_clocksource_init(dev, name, base, clk);
> > +		if (ret)
> > +			return ret;
> > +		stm_instances->clocksource++;
>
> That's racy. Devices can be brought async, ideally. This should be
> rather idr or probably entire structure protected with a mutex.

Mmh, interesting. I never had to think about this problem before

Do you know at what moment the probing is parallelized ?

> > +static struct stm_instances s32g_stm_instances = { .features = STM_CLKSRC | STM_CLKEVT_PER_CPU };
>
> Missing const. Or misplaced - all file-scope static variables are
> declared at the top.
>
> > +
> > +static const struct of_device_id nxp_stm_of_match[] = {
> > +	{ .compatible = "nxp,s32g2-stm", &s32g_stm_instances },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, nxp_stm_of_match);
> > +
> > +static struct platform_driver nxp_stm_probe = {
> > +	.probe	= nxp_stm_timer_probe,
> > +	.driver	= {
> > +		.name = "nxp-stm",
> > +		.of_match_table = of_match_ptr(nxp_stm_of_match),
>
> Drop of_match_ptr, you have here warnings.
>
> > +	},
> > +};
> > +module_platform_driver(nxp_stm_probe);
> > +
> > +MODULE_DESCRIPTION("NXP System Timer Module driver");
> > +MODULE_LICENSE("GPL");

Thanks for the review

  -- Daniel


--
<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