Messages in this thread | | | From | "Sanil, Shruthi" <> | Subject | RE: [PATCH v6 2/2] clocksource: Add Intel Keem Bay timer support | Date | Fri, 24 Dec 2021 09:40:53 +0000 |
| |
> -----Original Message----- > From: Daniel Lezcano <daniel.lezcano@linaro.org> > Sent: Thursday, December 23, 2021 7:46 PM > To: Sanil, Shruthi <shruthi.sanil@intel.com>; Thomas Gleixner > <tglx@linutronix.de>; 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> > Subject: Re: [PATCH v6 2/2] clocksource: Add Intel Keem Bay timer support > > 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 >
Thank You Daniel for the explanation. In case of KMB, we are using the ARM architecture timer. We would be using the timer for case2. So I need to register Just 1 timer. I'll check and make the changes accordingly and submit the next patch.
Thank You! Regards, Shruthi
> > > > > > > > -- > <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
| |