| Message ID | cover.1609306622.git.vijayakannan.ayyathurai@intel.com |
|---|---|
| Headers | show |
| Series | Add drivers for Intel Keem Bay SoC timer block | expand |
Hi, > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> > > 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. > > Vijayakannan Ayyathurai (2): > dt-bindings: timer: Add bindings for Intel Keem Bay SoC timer > clocksource: Add Intel Keem Bay Timer Support Kindly help us to review this updated patch(v2) set. Thanks, Vijay
On 13/01/2021 11:54, Ayyathurai, Vijayakannan wrote: > Hi, > >> From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> >> >> 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. >> >> Vijayakannan Ayyathurai (2): >> dt-bindings: timer: Add bindings for Intel Keem Bay SoC timer >> clocksource: Add Intel Keem Bay Timer Support > > Kindly help us to review this updated patch(v2) set. Review in progress ... :)
On 30/12/2020 07:25, vijayakannan.ayyathurai@intel.com wrote: > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> [ ... ] > +static struct timer_of keembay_ce_to = { > + .flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK, > + .clkevt = { > + .name = "keembay_sys_clkevt", > + .features = CLOCK_EVT_FEAT_PERIODIC | > + CLOCK_EVT_FEAT_ONESHOT | > + CLOCK_EVT_FEAT_DYNIRQ, > + .rating = TIM_RATING, > + .set_next_event = keembay_timer_set_next_event, > + .set_state_periodic = keembay_timer_periodic, > + .set_state_shutdown = keembay_timer_shutdown, > + }, > + .of_base = { > + .index = 0, > + }, > + .of_irq = { > + .handler = keembay_timer_isr, > + .flags = IRQF_TIMER | IRQF_IRQPOLL, Is the IRQPOLL flag really needed here ? > + }, > +}; > + > +static int __init keembay_clockevent_init(struct device_node *np, > + struct keembay_init_data *data) > +{ > + u32 val; > + int ret; > + > + data->mask = TIM_CONFIG_PRESCALER_ENABLE; > + data->cfg = &keembay_ce_to; > + ret = keembay_timer_setup(np, data); > + if (ret) > + return ret; > + > + val = readl(data->base + TIM_RELOAD_VAL_OFFSET); > + > + keembay_ce_to.clkevt.cpumask = cpumask_of(0); > + keembay_ce_to.of_clk.rate = keembay_ce_to.of_clk.rate / (val + 1); > + > + keembay_timer_disable(timer_of_base(&keembay_ce_to)); > + > + clockevents_config_and_register(&keembay_ce_to.clkevt, > + timer_of_rate(&keembay_ce_to), 1, U32_MAX); > + return 0; > +} > + > +static struct timer_of keembay_cs_to = { > + .flags = TIMER_OF_BASE | TIMER_OF_CLOCK, > + .of_base = { > + .index = 1, > + }, > +}; > + > +static u64 notrace keembay_clocksource_read(struct clocksource *cs) > +{ > + return lo_hi_readq(timer_of_base(&keembay_cs_to)); > +} > + > +static struct clocksource keembay_counter = { > + .name = "keembay_sys_counter", > + .rating = TIM_RATING, > + .read = keembay_clocksource_read, > + .mask = CLOCKSOURCE_MASK(TIM_CLKSRC_BITS), > + .flags = CLOCK_SOURCE_IS_CONTINUOUS | > + CLOCK_SOURCE_SUSPEND_NONSTOP, > +}; > + > +static int __init keembay_clocksource_init(struct device_node *np, > + struct keembay_init_data *data) > +{ > + int ret; > + > + data->mask = TIM_CONFIG_ENABLE; > + data->cfg = &keembay_cs_to; > + ret = keembay_timer_setup(np, data); > + if (ret) > + return ret; > + > + return clocksource_register_hz(&keembay_counter, timer_of_rate(&keembay_cs_to)); > +} > + > +static int __init keembay_timer_init(struct device_node *np) > +{ > + struct keembay_init_data data; > + int ret; > + > + data.base = of_iomap(np, 2); > + if (!data.base) > + return -ENXIO; > + > + ret = keembay_clocksource_init(np, &data); > + if (ret) > + goto exit; > + > + ret = keembay_clockevent_init(np, &data); Is this missing ? if (ret) goto exit; return 0; > + > +exit: > + keembay_timer_cleanup(np, &data); > + > + return ret; > +} > + > +TIMER_OF_DECLARE(keembay_timer, "intel,keembay-timer", keembay_timer_init); >
Hi Daniel, > From: Daniel Lezcano <daniel.lezcano@linaro.org> > >> From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> > >> > >> 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. > >> > >> Vijayakannan Ayyathurai (2): > >> dt-bindings: timer: Add bindings for Intel Keem Bay SoC timer > >> clocksource: Add Intel Keem Bay Timer Support > > > > Kindly help us to review this updated patch(v2) set. > > Review in progress ... :) > Thank you for the Review. Thanks, Vijay
Hi Daniel, > From: Daniel Lezcano <daniel.lezcano@linaro.org> > > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> > > [ ... ] > > > +static struct timer_of keembay_ce_to = { > > + .flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK, > > + .clkevt = { > > + .name = "keembay_sys_clkevt", > > + .features = CLOCK_EVT_FEAT_PERIODIC | > > + CLOCK_EVT_FEAT_ONESHOT | > > + CLOCK_EVT_FEAT_DYNIRQ, > > + .rating = TIM_RATING, > > + .set_next_event = > keembay_timer_set_next_event, > > + .set_state_periodic = keembay_timer_periodic, > > + .set_state_shutdown = keembay_timer_shutdown, > > + }, > > + .of_base = { > > + .index = 0, > > + }, > > + .of_irq = { > > + .handler = keembay_timer_isr, > > + .flags = IRQF_TIMER | IRQF_IRQPOLL, > > Is the IRQPOLL flag really needed here ? > Not really needed. I will remove this redundant Flag in my next version. > > +static int __init keembay_timer_init(struct device_node *np) > > +{ > > + struct keembay_init_data data; > > + int ret; > > + > > + data.base = of_iomap(np, 2); > > + if (!data.base) > > + return -ENXIO; > > + > > + ret = keembay_clocksource_init(np, &data); > > + if (ret) > > + goto exit; > > + > > + ret = keembay_clockevent_init(np, &data); > > Is this missing ? > Yes. Either case it goes to the exit path. So I thought of avoiding this error handling code. > if (ret) > goto exit; > > return 0; > > > + > > +exit: > > + keembay_timer_cleanup(np, &data); > > + > > + return ret; > > +} > > + > > +TIMER_OF_DECLARE(keembay_timer, "intel,keembay-timer", > keembay_timer_init); > > > Thanks, Vijay
On Tue, Jan 19, 2021 at 02:56:36AM +0000, Ayyathurai, Vijayakannan wrote: ... > > > + data.base = of_iomap(np, 2); > > > + if (!data.base) > > > + return -ENXIO; > > > + > > > + ret = keembay_clocksource_init(np, &data); > > > + if (ret) > > > + goto exit; > > > + > > > + ret = keembay_clockevent_init(np, &data); > > > > Is this missing ? > > > > Yes. Either case it goes to the exit path. So I thought of avoiding this error handling code. The point is that in success you probably won't call keembay_timer_cleanup(). > > if (ret) > > goto exit; > > > > return 0; > > > > > +exit: > > > + keembay_timer_cleanup(np, &data); > > > + > > > + return ret; > > > +}
Hi Andy, > From: andriy.shevchenko@linux.intel.com > > > > > + data.base = of_iomap(np, 2); > > > > + if (!data.base) > > > > + return -ENXIO; > > > > + > > > > + ret = keembay_clocksource_init(np, &data); > > > > + if (ret) > > > > + goto exit; > > > > + > > > > + ret = keembay_clockevent_init(np, &data); > > > > > > Is this missing ? > > > > > > > Yes. Either case it goes to the exit path. So I thought of avoiding this error > handling code. > > The point is that in success you probably won't call keembay_timer_cleanup(). > Yes. You are right, if I use this error handling code. > > > if (ret) > > > goto exit; > > > > > > return 0; > > > > > > > +exit: > > > > + keembay_timer_cleanup(np, &data); > > > > + > > > > + return ret; > > > > +} > Thanks, Vijay
From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> Hi, This patch set adds the driver for Intel Keem Bay SoC timer block, which contains 32-bit general purpose timers, a 64 bit free running counter. Patch 1 holds the Device Tree binding documentation and Patch 2 holds the Device Driver for clocksource and clockevent. It was tested on the Keem Bay evaluation module board. Thanks, Vijay 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. Vijayakannan Ayyathurai (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 | 52 ++++ arch/arm64/Kconfig.platforms | 1 + drivers/clocksource/Kconfig | 10 + drivers/clocksource/Makefile | 1 + drivers/clocksource/timer-keembay.c | 225 ++++++++++++++++++ 5 files changed, 289 insertions(+) create mode 100644 Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml create mode 100644 drivers/clocksource/timer-keembay.c base-commit: 5c8fe583cce542aa0b84adc939ce85293de36e5e prerequisite-patch-id: bbb340c3a34059ea6960e8b96f5ad3bf0b4ae7b0 prerequisite-patch-id: b71b548284a57a38ce96d9bb523eadfccabd6e02