On 11/11/2021 11:42, Sanil, Shruthi wrote:
>> -----Original Message-----
>> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Sent: Monday, September 27, 2021 3:11 AM
>> To: Sanil, Shruthi <shruthi.sanil@xxxxxxxxx>; daniel.lezcano@xxxxxxxxxx;
>> robh+dt@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>> devicetree@xxxxxxxxxxxxxxx
>> Cc: andriy.shevchenko@xxxxxxxxxxxxxxx; kris.pan@xxxxxxxxxxxxxxx;
>> mgross@xxxxxxxxxxxxxxx; Thokala, Srikanth <srikanth.thokala@xxxxxxxxx>;
>> Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@xxxxxxxxx>;
>> Sangannavar, Mallikarjunappa <mallikarjunappa.sangannavar@xxxxxxxxx>;
>> Sanil, Shruthi <shruthi.sanil@xxxxxxxxx>
>> Subject: Re: [PATCH v6 2/2] clocksource: Add Intel Keem Bay timer support
>>
>> On Tue, Sep 07 2021 at 00:06, shruthi sanil wrote:
>>> +
>>> +/* Provides a unique ID for each timer */ static
>>> +DEFINE_IDA(keembay_timer_ida);
>>
>>> +
>>> + timer_id = ida_alloc(&keembay_timer_ida, GFP_KERNEL);
>>> + if (timer_id < 0) {
>>> + ret = timer_id;
>>> + goto err_keembay_ce_to_free;
>>> + }
>>
>> May I ask what the purpose of the IDA, which is backed by a full blown
>> xarray, is here?
>>
>> AFAICT all you want is a unique number for the timer name for up to 8
>> timers.
>>
>>> + timer_name = kasprintf(GFP_KERNEL, "keembay_timer%d",
>> timer_id);
>>
>> So what's wrong about:
>>
>> static unsigned int keembay_timer_id;
>>
>> timer_name = kasprintf(GFP_KERNEL, "keembay_timer%d",
>> keembay_timer_id++);
>>
>> Hmm?
>
> Yes, we had initially implemented it in the similar way,
> but in the course of review it got changed to use IDA.
>
>>
>>> +
>>> + clockevents_config_and_register(&keembay_ce_to->clkevt,
>>> + timer_of_rate(keembay_ce_to),
>>> + 1,
>>> + U32_MAX);
>>
>> Aside of that what's the point of registering more than one of those timers as
>> clock event? The core will only use one and the rest is just going to use
>> memory for no value.
>
> Instead of
> keembay_ce_to->clkevt.cpumask = cpumask_of(0);
> can I update it as
> keembay_ce_to->clkevt.cpumask = cpu_possible_mask;
> so that each timer would be associated with different cores?
Let me try to clarify:
The Intel Keem bay Soc is a 4 x Cortex-A53
The arch ARM timer is per CPU on this platform.
Case 1:
-------
- the architected timer is not desired and this timer is wanted to be
used instead (but rating tells the opposite) => rewrite per cpu code
Case 2:
-------
- the architected timer are desired and this timer is used as a
broadcast timer when a core is going done with cpuidle. One timer is needed.
- In order to prevent useless wakeup, the timer uses the flag DYNIRQ.
However, cpumask_of(0) is set and makes inoperative this flag. In order
to make full use of it, clkevt.cpumask must be cpu_possible_mask
Hope that helps
-- Daniel
--
<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
