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

[Krzysztof Kozlowski]

On 25/03/2025 13:23, Daniel Lezcano wrote:
> 
> [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);

Ah, right, it's the full name.

> 
> The names are:
> 
> "clocksource: Switched to clocksource stm@4011c000"
> 
> Or:
> 
>   17:      24150          0          0          0    GICv3  57 Level 
> stm@40120000

This all will be timer@, but anyway I get your point.

>   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

Ack

> 
> [ ... ]
> 
>>> +
>>> +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 ?

You don't have PROBE_PREFER_ASYNCHRONOUS, so currently this will be
still sync, but I don't think we want it to be that way forever. I think
new drivers should not rely on implicit sync, because converting it
later to async will be difficult. It's easier to design it now or even
choose async explicitly (after testing).

BTW, PREEMPT_RT and all fast-boot-use-cases would be only happier with
probe being async.


Best regards,
Krzysztof