| Message ID | 20210906183621.21075-1-shruthi.sanil@intel.com |
|---|---|
| Headers | show |
| Series | Add the driver for Intel Keem Bay SoC timer block | expand |
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? > + > + 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. Thanks, tglx
> -----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? Thanks, Shruthi > > Thanks, > > tglx
Hi Thomas > -----Original Message----- > From: Sanil, Shruthi > Sent: Thursday, November 11, 2021 4:12 PM > To: Thomas Gleixner <tglx@linutronix.de>; 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> > Subject: RE: [PATCH v6 2/2] clocksource: Add Intel Keem Bay timer support > > > -----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? > Could you please help me with the above query? Thanks, Shruthi > Thanks, > Shruthi > > > > > Thanks, > > > > tglx
Hi Thomas, Could you please help with the query addressed below regarding multiple timers? > -----Original Message----- > From: Sanil, Shruthi > Sent: Thursday, November 25, 2021 10:59 PM > To: Thomas Gleixner <tglx@linutronix.de>; 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> > Subject: RE: [PATCH v6 2/2] clocksource: Add Intel Keem Bay timer support > > Hi Thomas > > > -----Original Message----- > > From: Sanil, Shruthi > > Sent: Thursday, November 11, 2021 4:12 PM > > To: Thomas Gleixner <tglx@linutronix.de>; 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> > > Subject: RE: [PATCH v6 2/2] clocksource: Add Intel Keem Bay timer > > support > > > > > -----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? > > > > Could you please help me with the above query? > > Thanks, > Shruthi > > > Thanks, > > Shruthi > > > > > > > > Thanks, > > > > > > tglx
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
> -----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
From: Shruthi Sanil <shruthi.sanil@intel.com> The timer block supports 1 64-bit free running counter and 8 32-bit general purpose timers. Patch 1 holds the device tree binding documentation. Patch 2 holds the device driver. This driver is tested on the Keem Bay evaluation module board. Changes since v5: - Created a MFD device for the common configuration register in the device tree bindings. - Updated the timer driver with the MFD framework to access the common configuration register. Changes since v4: - Updated the description in the device tree bindings. - Updated the unit address of all the timers and counter in the device tree binding. Changes since v3: - Update in KConfig file to support COMPILE_TEST for Keem Bay timer. - Update in device tree bindings to remove status field. - Update in device tree bindings to remove 64-bit address space for the child nodes by using non-empty ranges. Changes since v2: - Add multi timer support. - Update in the device tree binding to support multi timers. - Code optimization. Changes since v1: - Add support for KEEMBAY_TIMER to get selected through Kconfig.platforms. - Add CLOCK_EVT_FEAT_DYNIRQ as part of clockevent feature. - Avoid overlapping reg regions across 2 device nodes. - Simplify 2 device nodes as 1 because both are from same IP block. - Adapt the driver code according to the new simplified devicetree. Shruthi Sanil (2): dt-bindings: timer: Add bindings for Intel Keem Bay SoC Timer clocksource: Add Intel Keem Bay timer support .../bindings/timer/intel,keembay-timer.yaml | 173 ++++++++++++ MAINTAINERS | 5 + drivers/clocksource/Kconfig | 11 + drivers/clocksource/Makefile | 1 + drivers/clocksource/timer-keembay.c | 252 ++++++++++++++++++ 5 files changed, 442 insertions(+) create mode 100644 Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml create mode 100644 drivers/clocksource/timer-keembay.c base-commit: 27151f177827d478508e756c7657273261aaf8a9