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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

[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