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

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

On 26/03/2025 08:44, Ghennadi Procopciuc wrote:
> On 3/25/2025 3:54 PM, Daniel Lezcano wrote:
>> On 25/03/2025 14:21, Ghennadi Procopciuc wrote:
>>> On 3/25/2025 2:51 PM, Daniel Lezcano wrote:
>>> [ ... ]
>>>>>>>>>> +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);
>>>>>>>>>> +    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);
>>>>>>>>>> +        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;
>>>>>>>>>> +    }
>>>>>>>>>
>>>>>>>>>      From commit description:
>>>>>>>>>
>>>>>>>>>> The first probed STM is used as a clocksource, the second will be
>>>>>>>>>> the
>>>>>>>>>> broadcast timer and the rest are used as a clockevent with the
>>>>>>>>>> affinity set to a CPU.
>>>>>>>>>
>>>>>>>>> Why is the interrupt mandatory when the node is probed as a
>>>>>>>>> clocksource?
>>>>>>>>
>>>>>>>> It relies on the DT description and it does not hurt to have a
>>>>>>>> consistent code path for clockevent / clocksource even if the
>>>>>>>> interrupt
>>>>>>>> is not requested for the clocksource later.
>>>>>>>>
>>>>>>>
>>>>>>> If so, in my opinion, it would make sense to use the same STM
>>>>>>> instance
>>>>>>> for both the clocksource and broadcast clockevent, as both functions
>>>>>>> can
>>>>>>> be accommodated within a single STM instance, which will help reduce
>>>>>>> the
>>>>>>> number of STM instances used.
>>>>>>
>>>>>> The broadcast timer is stopped when it is unused which is the case for
>>>>>> the s32 because there are no idle state powering down the local
>>>>>> timers.
>>>>>> They have to have be separated.
>>>>>>
>>>>>
>>>>> This wouldn't be the case if the STM is kept running/counting during
>>>>> the
>>>>> clock event setup, with only the clock event interrupt being disabled
>>>>> (CCR.CEN).
>>>>
>>>> Are you asking to use two different channels for the same STM instance,
>>>> one for the clocksource and one for the clockevent ?
>>>>
>>>
>>> I suggested using the CNT register to obtain the count for the clock
>>> source, while using one of the STM channels for the clock event.
>>
>> Ah, ok.
>>
>> I think it is preferable to keep them separated to keep the code
>> modular. Given the number of STM on the platform, it does not hurt
>>
> 
> The S32G2 and S32G3 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. Allocating six instances (1 clock
> source, 1 broadcast clock event, and 4 clock events for all A53 cores)
> to Linux on the S32G2 leaves the M7 software stacks without adequate STM
> coverage.

Given this description I'm wondering why one STM has to be used as a 
clocksource if the STM_07 is already a TS one. And also, we can get rid 
of the broadcast timer as there is no idle state forcing a switch to an 
always-on timer.

IIUC, on the S32g2 there are 4 x Cortex-A53 and 3 x Cortex-M3, that 
means we need 7 timers.

The system has 7 timers + a special one for timestamp.

So if I follow the reasoning, the broadcast timer should not be used 
otherwise one cortex-M3 will end up without a timer.

That leads us to one clocksource for the first per CPU timer initialized 
or alternatively one STM instance == 1 clocksource and 1 clockevent 
which makes things simpler


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