Re: [PATCH v6 2/2] clocksource: Add Intel Keem Bay timer support

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

On 11/11/2021 11:42, Sanil, Shruthi wrote:
>> -----Original Message-----
>> From: Thomas Gleixner <tglx@linutronix.de>
>> Sent: Monday, September 27, 2021 3:11 AM
>> To: Sanil, Shruthi <shruthi.sanil@intel.com>; daniel.lezcano@linaro.org;
>> robh+dt@kernel.org; linux-kernel@vger.kernel.org;
>> devicetree@vger.kernel.org
>> Cc: andriy.shevchenko@linux.intel.com; kris.pan@linux.intel.com;
>> mgross@linux.intel.com; Thokala, Srikanth <srikanth.thokala@intel.com>;
>> Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>> Sangannavar, Mallikarjunappa <mallikarjunappa.sangannavar@intel.com>;
>> Sanil, Shruthi <shruthi.sanil@intel.com>
>> 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