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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a31a575b-8f6d-6317-f172-b4f8b1a5cda7@oss.nxp.com>
Date: Wed, 26 Mar 2025 09:44:18 +0200
From: Ghennadi Procopciuc <ghennadi.procopciuc@....nxp.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>, tglx@...utronix.de
Cc: linux-kernel@...r.kernel.org, Thomas Fossati <thomas.fossati@...aro.org>,
 Larisa Grigore <Larisa.Grigore@....com>,
 Ghennadi Procopciuc <ghennadi.procopciuc@....com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 "moderated list:ARM/STM32 ARCHITECTURE"
 <linux-stm32@...md-mailman.stormreply.com>,
 "moderated list:ARM/STM32 ARCHITECTURE"
 <linux-arm-kernel@...ts.infradead.org>, NXP S32 Linux Team <s32@....com>
Subject: Re: [PATCH 2/2] clocksource/drivers/nxp-timer: Add the System Timer
 Module for the s32g platform

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.

Additionally, the proposed implementation uses only one STM channel out
of four, which is not optimal hardware usage. I suggest using all STM
channels instead of limiting it to a single channel per instance, given
that the driver already uses a global structure to pair STM instances
with cores. This approach will optimize the number of instances required
by Linux and leverage the capabilities of each STM.

-- 
Regards,
Ghennadi