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

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

On 25/03/2025 13:21, Ghennadi Procopciuc wrote:
> On 3/25/2025 2:09 PM, Daniel Lezcano wrote:
>> On 25/03/2025 12:40, Ghennadi Procopciuc wrote:
>>> On 3/25/2025 12:53 PM, 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;
>>>>>> +    stm_timer->scs.cs.rating = 460;
>>>>>> +    stm_timer->scs.cs.read = nxp_stm_clocksource_read;
>>>>>> +    stm_timer->scs.cs.enable = nxp_stm_clocksource_enable;
>>>>>> +    stm_timer->scs.cs.disable = nxp_stm_clocksource_disable;
>>>>>> +    stm_timer->scs.cs.suspend = nxp_stm_clocksource_suspend;
>>>>>> +    stm_timer->scs.cs.resume = nxp_stm_clocksource_resume;
>>>>>> +    stm_timer->scs.cs.mask = CLOCKSOURCE_MASK(32);
>>>>>> +    stm_timer->scs.cs.flags = CLOCK_SOURCE_IS_CONTINUOUS;
>>>>>> +
>>>>>> +    ret = clocksource_register_hz(&stm_timer->scs.cs,
>>>>>> stm_timer->rate);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>
>>>>> clocksource_unregister during remove callback for cleanup?
>>>>
>>>> Sorry I don't get it :/
>>>>
>>>> There is no cleanup after the clocksource_register_hz() is successful
>>>>
>>>
>>> I noticed that other drivers, such as
>>> drivers/clocksource/timer-tegra186.c and
>>> drivers/clocksource/timer-sun5i.c, perform clocksource_unregister during
>>> their platform_driver.remove callback. Shouldn't this apply here as well?
>>
>> The tegra186 registers with one probe several timers and clocksources.
>> The timer-nxp probes for each node.
>>
>> The timer-sun5i.c has the remove callback but it is pointless as it can
>> not be compiled as module. So it should not have it.
>>
> 
> Ok.
> 
>>> [ ... ]
>>>>
>>>>>> +static int nxp_stm_clockevent_set_next_event(unsigned long delta,
>>>>>> struct clock_event_device *ced)
>>>>>> +{
>>>>>> +    struct stm_timer *stm_timer = ced_to_stm(ced);
>>>>>> +    u32 val;
>>>>>> +
>>>>>> +    nxp_stm_clockevent_disable(stm_timer);
>>>>>
>>>>> While examining the code base, I came across the
>>>>> drivers/clocksource/timer-imx-gpt.c file, specifically the
>>>>> mx1_2_set_next_event function, which includes a protection against
>>>>> missing events. Using a similar approach would allow us to keep the STM
>>>>> module enabled while only altering the channel's register state. This
>>>>> risk can also be mitigated by adjusting min_delta_ns based on tick
>>>>> frequency.
>>>>
>>>> How would you validate that ?
>>>>
>>>
>>> How would I validate that this works?
>>>
>>> If this is the question, I see that the core performs an auto adjustment
>>> of the minimum delta as part of the clockevents_program_min_delta
>>> function when CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST is enabled.
>>> Initially, I would examine how many times dev->set_next_event() returns
>>> -ETIME. At the end of the function, min_delta_ns should reflect the
>>> actual minimum delta the device can handle.
>>
>> That is an area of optimization and I would prefer to keep that as a
>> separate change focused on validating this approach.
>>
> 
> This suggestion supports the argument presented below.
> 
>>> [ ... ]
>>>>>> +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 ?



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