Messages in this thread | | | Subject | Re: [PATCH 04/17] clocksource: Add PLX Technology RPS Timer | From | Daniel Lezcano <> | Date | Thu, 17 Mar 2016 17:13:44 +0100 |
| |
Hi Neil,
On 03/03/2016 12:39 PM, Neil Armstrong wrote: > Add clocksource and clockevent driver from dual RPS timer.
please elaborate the log: describe the timer, how it works, is there a pointer to doc, freq, etc ...
> CC: Ma Haijun <mahaijuns@gmail.com> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/clocksource/Kconfig | 6 + > drivers/clocksource/Makefile | 1 + > drivers/clocksource/timer-rps.c | 249 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 256 insertions(+) > create mode 100644 drivers/clocksource/timer-rps.c > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > index 33db740..f79bc0f 100644 > --- a/drivers/clocksource/Kconfig > +++ b/drivers/clocksource/Kconfig > @@ -276,6 +276,12 @@ config VF_PIT_TIMER > help > Support for Period Interrupt Timer on Freescale Vybrid Family SoCs. > > +config CLKSRC_RPS_TIMER > + bool > + select CLKSRC_MMIO > + help > + This enables support for the PLX Tech OXNAS RPS timers. > +
Please check DIGICOLOR_TIMER or DW_APB_TIMER or ROCKCHIP_TIMER or ARMADA_370_XP_TIMER or ... config section and try to stick to the same pattern.
> config SYS_SUPPORTS_SH_CMT > bool > > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile > index dc2b899..120bc09 100644 > --- a/drivers/clocksource/Makefile > +++ b/drivers/clocksource/Makefile > @@ -46,6 +46,7 @@ obj-$(CONFIG_CLKSRC_QCOM) += qcom-timer.o > obj-$(CONFIG_MTK_TIMER) += mtk_timer.o > obj-$(CONFIG_CLKSRC_PISTACHIO) += time-pistachio.o > obj-$(CONFIG_CLKSRC_TI_32K) += timer-ti-32k.o > +obj-$(CONFIG_CLKSRC_RPS_TIMER) += timer-rps.o > > obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o > obj-$(CONFIG_ARM_GLOBAL_TIMER) += arm_global_timer.o > diff --git a/drivers/clocksource/timer-rps.c b/drivers/clocksource/timer-rps.c > new file mode 100644 > index 0000000..79621b8 > --- /dev/null > +++ b/drivers/clocksource/timer-rps.c > @@ -0,0 +1,249 @@ > +/* > + * drivers/clocksource/timer-rps.c > + * > + * Copyright (C) 2009 Oxford Semiconductor Ltd > + * Copyright (C) 2013 Ma Haijun <mahaijuns@gmail.com> > + * Copyright (C) 2016 Neil Armstrong <narmstrong@baylibre.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/io.h> > +#include <linux/clockchips.h> > +#include <linux/clk.h> > +#include <linux/of_irq.h> > +#include <linux/of_address.h> > +#include <linux/sched_clock.h> > + > +/* TIMER1 as tick > + * TIMER2 as clocksource > + */
Comment format:
/* * TIMER1 used as tick * TIMER2 used as clocksource */
> +enum { > + TIMER_LOAD = 0, > + TIMER_CURR = 4, > + TIMER_CTRL = 8, > + TIMER_CLRINT = 0xC, > + > + TIMER_BITS = 24, > + > + TIMER_MAX_VAL = (1 << TIMER_BITS) - 1, > + > + TIMER_PERIODIC = (1 << 6), > + TIMER_ENABLE = (1 << 7), > + > + TIMER_DIV1 = (0 << 2), > + TIMER_DIV16 = (1 << 2), > + TIMER_DIV256 = (2 << 2), > + > + TIMER1_OFFSET = 0, > + TIMER2_OFFSET = 0x20, > +};
What is the point of having this unorganized enum with duplicate values ? and why TIMER_DIV1 = (0 << 2) ?
Replace by #define.
> +/* Clockevent */ > + > +static unsigned long timer_period = HZ; > +static unsigned timer_prescaler = 1; > +static void __iomem *timer_base;
Encapsulate these variables into a structure and use container_of.
> + > +static irqreturn_t rps_timer_irq(int irq, void *dev_id) > +{ > + struct clock_event_device *evt = dev_id; > + > + iowrite32(0, timer_base + TIMER_CLRINT); > + > + evt->event_handler(evt); > + > + return IRQ_HANDLED; > +} > + > +static void rps_timer_config(unsigned long period, unsigned periodic) > +{ > + uint32_t cfg = 0; > + > + if (period) > + cfg |= TIMER_ENABLE; > + > + if (periodic) > + cfg |= TIMER_PERIODIC; > + > + switch (timer_prescaler) { > + case 1: > + cfg |= TIMER_DIV1; > + break; > + case 16: > + cfg |= TIMER_DIV16; > + break; > + case 256: > + cfg |= TIMER_DIV256; > + break; > + }
No direct value (1, 16, 256) but macros please.
> + iowrite32(period, timer_base + TIMER_LOAD); > + iowrite32(cfg, timer_base + TIMER_CTRL); > +} > + > +static int rps_timer_shutdown(struct clock_event_device *evt) > +{ > + if (!clockevent_state_periodic(evt)) > + return 0; > + > + rps_timer_config(0, 0); > + > + return 0; > +} > + > +static int rps_timer_set_periodic(struct clock_event_device *evt) > +{ > + rps_timer_config(timer_period, 1); > + > + return 0; > +} > + > +static int rps_timer_set_oneshot(struct clock_event_device *evt) > +{ > + rps_timer_config(timer_period, 0); > + > + return 0; > +} > + > +static int rps_timer_next_event(unsigned long delta, > + struct clock_event_device *evt) > +{ > + rps_timer_config(delta, 0); > + > + return 0; > +} > + > +static struct clock_event_device rps_clockevent = { > + .name = "rps", > + .features = CLOCK_EVT_FEAT_PERIODIC | > + CLOCK_EVT_FEAT_ONESHOT, > + .tick_resume = rps_timer_shutdown, > + .set_state_shutdown = rps_timer_shutdown, > + .set_state_periodic = rps_timer_set_periodic, > + .set_state_oneshot = rps_timer_set_oneshot, > + .set_next_event = rps_timer_next_event, > + .rating = 200, > +}; > + > +static void __init rps_clockevent_init(void __iomem *base, ulong ref_rate, > + int irq) > +{ > + timer_base = base; > + > + /* Start with prescaler 1 */ > + timer_prescaler = 1; > + timer_period = DIV_ROUND_UP(ref_rate, HZ); > + > + if (timer_period > TIMER_MAX_VAL) { > + timer_prescaler = 16; > + timer_period = DIV_ROUND_UP(ref_rate / timer_prescaler, HZ);
Using macro and bit shifting will be nicer.
> + } > + if (timer_period > TIMER_MAX_VAL) { > + timer_prescaler = 256; > + timer_period = DIV_ROUND_UP(ref_rate / timer_prescaler, HZ); > + } > + rps_clockevent.cpumask = cpu_possible_mask; > + rps_clockevent.irq = irq;
So if you are using such cpumask, I suggest you have a look at the DYNIRQ flag.
> + clockevents_config_and_register(&rps_clockevent, > + ref_rate / timer_prescaler, > + 1, > + TIMER_MAX_VAL); > + > + pr_info("rps: Registered clock event rate %luHz prescaler %d period %lu\n", > + ref_rate, > + timer_prescaler, > + timer_period); > +} > + > +/* Clocksource */ > + > +static void __iomem *timer_curr; > + > +static u64 notrace rps_read_sched_clock(void) > +{ > + return ~readl_relaxed(timer_curr); > +} > + > +static void __init rps_clocksource_init(void __iomem *base, ulong ref_rate) > +{ > + int ret; > + ulong clock_rate; > + /* use prescale 16 */ > + clock_rate = ref_rate / 16; > + > + iowrite32(TIMER_MAX_VAL, base + TIMER_LOAD); > + iowrite32(TIMER_PERIODIC | TIMER_ENABLE | TIMER_DIV16, > + base + TIMER_CTRL); > + > + timer_curr = base + TIMER_CURR; > + sched_clock_register(rps_read_sched_clock, TIMER_BITS, clock_rate); > + ret = clocksource_mmio_init(base + TIMER_CURR, "rps_clocksource_timer", > + clock_rate, 250, TIMER_BITS, > + clocksource_mmio_readl_down); > + if (ret) > + panic("can't register clocksource\n");
No panic, just an error.
> + pr_info("rps: Registered clocksource rate %luHz\n", clock_rate); > +} > + > +static struct irqaction rps_timer_irqaction = { > + .name = "rps_timer", > + .flags = IRQF_TIMER | IRQF_IRQPOLL, > + .handler = rps_timer_irq, > + .dev_id = &rps_clockevent, > +}; > + > +static void __init rps_timer_init(struct device_node *np) > +{ > + struct clk *refclk; > + unsigned long ref_rate; > + void __iomem *base; > + int irq, ret; > + > + refclk = of_clk_get(np, 0); > + > + if (IS_ERR(refclk) || clk_prepare_enable(refclk)) > + panic("rps_timer_init: failed to get refclk\n");
no panic.
> + ref_rate = clk_get_rate(refclk); > + > + base = of_iomap(np, 0); > + if (!base) > + panic("rps_timer_init: failed to map io\n"); > + > + irq = irq_of_parse_and_map(np, 0); > + if (irq < 0) > + panic("rps_timer_init: failed to parse IRQ\n");
if (!irq) no panic.
> + > + /* Disable timers */ > + iowrite32(0, base + TIMER1_OFFSET + TIMER_CTRL); > + iowrite32(0, base + TIMER2_OFFSET + TIMER_CTRL); > + iowrite32(0, base + TIMER1_OFFSET + TIMER_LOAD); > + iowrite32(0, base + TIMER2_OFFSET + TIMER_LOAD); > + iowrite32(0, base + TIMER1_OFFSET + TIMER_CLRINT); > + iowrite32(0, base + TIMER2_OFFSET + TIMER_CLRINT); > + > + rps_clocksource_init(base + TIMER2_OFFSET, ref_rate); > + rps_clockevent_init(base + TIMER1_OFFSET, ref_rate, irq); > + > + ret = setup_irq(irq, &rps_timer_irqaction);
Replace setup_irq by request_irq.
> + if (ret) > + panic("rps_timer_init: failed to request irq\n"); > +} > + > +CLOCKSOURCE_OF_DECLARE(nas782x, "plxtech,nas782x-rps-timer", rps_timer_init);
Thanks !
-- 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
| |