Re: [PATCH 19/20] clocksource/drivers/nxp-pit: Add NXP Automotive s32g2 / s32g3 support

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

On 07/07/2025 14:02, Ghennadi Procopciuc wrote:
> On 7/5/2025 7:01 PM, Daniel Lezcano wrote:
>> The S32G platform has two Periodic Interrupt Timer (PIT). The IP is
>> exactly the same as the VF platform.
>>
>> The setup found for this platform is each timer instance is bound to
>> CPU0 and CPU1.
>>
>> The changes bring the support for multiple timer instances. When we
>> reach the maximum PIT instances for this platform, which is described
>> in the match table data, the hotplug callbacks are called where they
>> finish the per CPU setup.
>>
>> Tested on a s32g274a-rdb2.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
> 
> The current description does not clearly explain how the resources are used within the driver. It would be helpful to mention that channel 2 is used as the clocksource, while channel 3 is designated for clockevents.

The changes are to allow to support multiple instances of the PIT timer. 
The way the channels are used is unchanged. I can put a sentence to 
remind how they are used.

> Additionally, the S32G2 platform supports suspend and resume functionality. However, this capability is not yet implemented in the driver. Do you plan to include support for it in a future patch?

Yes, usually PM is added after.

> [...]
> 
>> -static int __init pit_timer_init(struct device_node *np)
>> +static int pit_timer_init(struct device_node *np)
>>   {
>>   	struct pit_timer *pit;
>>   	struct clk *pit_clk;
>> @@ -261,16 +296,31 @@ static int __init pit_timer_init(struct device_node *np)
>>   
>>   	clk_rate = clk_get_rate(pit_clk);
>>   
>> -	/* enable the pit module */
>> -	pit_module_enable(timer_base);
>> +	pit_module_disable(timer_base);
>>   
>>   	ret = pit_clocksource_init(pit, name, timer_base, clk_rate);
>> -	if (ret)
>> +	if (ret) {
>> +		pr_err("Failed to initialize clocksource '%pOF'\n", np);
>>   		goto out_pit_module_disable;
>> +	}
>>   
>> -	ret = pit_clockevent_init(pit, name, timer_base, clk_rate, irq, 0);
>> -	if (ret)
>> +	ret = pit_clockevent_per_cpu_init(pit, name, timer_base, clk_rate, irq, pit_instances);
>> +	if (ret) {
>> +		pr_err("Failed to initialize clockevent '%pOF'\n", np);
>>   		goto out_pit_clocksource_unregister;
>> +	}
> 
> This mechanism is very restrictive and will allow to assign the PIT0 and PIT1 to CPU0 and CPU1 only. Have you considered alternatives where the mapping is given as mask through early_param?

Yes, we can consider putting in place a mechanism to configure how the 
PIT are mapped to a CPU but that would be a separate feature to be added 
later.


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