Messages in this thread | | | Subject | Re: [PATCH v3 04/25] clocksource: Add Owl timer | From | Andreas Färber <> | Date | Tue, 28 Feb 2017 18:08:06 +0100 |
| |
Am 28.02.2017 um 17:47 schrieb Daniel Lezcano: > On Tue, Feb 28, 2017 at 07:35:14AM +0100, Andreas Färber wrote: >> The Actions Semi S500 SoC provides four timers, 2Hz0/1 and 32-bit TIMER0/1. >> >> Use TIMER0 as clocksource and TIMER1 as clockevents. >> >> Based on LeMaker linux-actions tree. >> >> An S500 datasheet can be found on the LeMaker Guitar pages: >> http://www.lemaker.org/product-guitar-download-29.html >> >> Signed-off-by: Andreas Färber <afaerber@suse.de> >> --- >> v2 -> v3: >> * Cleared interrupt pending flag for Timer1 >> * Adopted named interrupts for Timer1 >> * Extended commit message (Daniel) >> * Adopted BIT() macros (Daniel) >> * Adopted PTR_ERR() (Daniel) >> * Adopted request_irq() (Daniel) >> * Factored timer reset out (Daniel) >> * Adopted CLOCK_EVT_FEAT_DYNIRQ (Daniel) >> * Adopted clk input for rate (Daniel) >> * Prepared for S900, adopting S500 DT compatible >> >> v2: new >> >> drivers/clocksource/Kconfig | 7 ++ >> drivers/clocksource/Makefile | 1 + >> drivers/clocksource/owl-timer.c | 193 ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 201 insertions(+) >> create mode 100644 drivers/clocksource/owl-timer.c >> >> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig >> index 3356ab8..2551365 100644 >> --- a/drivers/clocksource/Kconfig >> +++ b/drivers/clocksource/Kconfig >> @@ -107,6 +107,13 @@ config ORION_TIMER >> help >> Enables the support for the Orion timer driver >> >> +config OWL_TIMER >> + bool "Owl timer driver" if COMPILE_TEST >> + depends on GENERIC_CLOCKEVENTS >> + select CLKSRC_MMIO >> + help >> + Enables the support for the Actions Semi Owl timer driver. >> + >> config SUN4I_TIMER >> bool "Sun4i timer driver" if COMPILE_TEST >> depends on GENERIC_CLOCKEVENTS >> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile >> index d227d13..801b65a 100644 >> --- a/drivers/clocksource/Makefile >> +++ b/drivers/clocksource/Makefile >> @@ -53,6 +53,7 @@ obj-$(CONFIG_CLKSRC_PISTACHIO) += time-pistachio.o >> obj-$(CONFIG_CLKSRC_TI_32K) += timer-ti-32k.o >> obj-$(CONFIG_CLKSRC_NPS) += timer-nps.o >> obj-$(CONFIG_OXNAS_RPS_TIMER) += timer-oxnas-rps.o >> +obj-$(CONFIG_OWL_TIMER) += owl-timer.o >> >> obj-$(CONFIG_ARC_TIMERS) += arc_timer.o >> obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o >> diff --git a/drivers/clocksource/owl-timer.c b/drivers/clocksource/owl-timer.c >> new file mode 100644 >> index 0000000..1b1e26d >> --- /dev/null >> +++ b/drivers/clocksource/owl-timer.c >> @@ -0,0 +1,193 @@ >> +/* >> + * Actions Semi Owl timer >> + * >> + * Copyright 2012 Actions Semi Inc. >> + * Author: Actions Semi, Inc. >> + * >> + * Copyright (c) 2017 SUSE Linux GmbH >> + * Author: Andreas Färber >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License as published by the >> + * Free Software Foundation; either version 2 of the License, or (at your >> + * option) any later version. >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/clockchips.h> >> +#include <linux/interrupt.h> >> +#include <linux/irq.h> >> +#include <linux/irqreturn.h> >> +#include <linux/sched_clock.h> >> +#include <linux/of.h> >> +#include <linux/of_address.h> >> +#include <linux/of_irq.h> >> + >> +#define OWL_Tx_CTL 0x0 >> +#define OWL_Tx_CMP 0x4 >> +#define OWL_Tx_VAL 0x8 >> + >> +#define OWL_Tx_CTL_PD BIT(0) >> +#define OWL_Tx_CTL_INTEN BIT(1) >> +#define OWL_Tx_CTL_EN BIT(2) >> + >> +#define OWL_MAX_Tx 2 >> + >> +struct owl_timer_info { >> + int timer_offset[OWL_MAX_Tx]; >> +}; >> + >> +static const struct owl_timer_info *owl_timer_info; >> + >> +static void __iomem *owl_timer_base; >> + >> +static inline void __iomem *owl_timer_get_base(unsigned timer_nr) >> +{ >> + if (timer_nr >= OWL_MAX_Tx) >> + return NULL; > > The driver is supposed to know what is doing. owl_timer_get_base won't be > called with an invalid index. This test will have a cost as it is called from > owl_timer_sched_read.
OK
>> + >> + return owl_timer_base + owl_timer_info->timer_offset[timer_nr]; > > Actually, you just need a clkevt base @ and a clocksource base @. > > Instead of computing again and again the base, why not just precompute: > > owl_clksrc_base = owl_timer_base + owl_timer_info->timer_offset[OWL_TIMER0] > owl_clkevt_base = owl_timer_base + owl_timer_info->timer_offset[OWL_TIMER1] > > at init time. > > And use these variables directly in the functions.
Either that, or revert to previous simpler behavior...
>> +} >> + >> +static inline void owl_timer_reset(unsigned index) >> +{ >> + void __iomem *base; >> + >> + base = owl_timer_get_base(index); >> + if (!base) >> + return; > > Same here, this test is pointless.
Seems like you didn't look at the following patch yet. It sets two S500 offsets as -1, i.e. non-existant, which then results in NULL here.
>> + writel(0, base + OWL_Tx_CTL); >> + writel(0, base + OWL_Tx_VAL); >> + writel(0, base + OWL_Tx_CMP); >> +} > > I suggest: > > static inline void owl_timer_reset(void __iomem *addr) > { > writel(0, addr + OWL_Tx_CTL); > writel(0, addr + OWL_Tx_VAL); > writel(0, addr + OWL_Tx_CMP); > }
OK
>> + >> +static u64 notrace owl_timer_sched_read(void) >> +{ >> + return (u64)readl(owl_timer_get_base(0) + OWL_Tx_VAL); > > return (u64)readl(owl_clksrc_base + OWL_Tx_VAL); > >> +} >> + > > static inline int owl_timer_set_state_disable(struct clock_event_device *evt) > { > return writel(0, owl_clkevt_base + OWL_Tx_CTL); > }
That I don't like. Disabling is just setting a bit. We save a readl by just writing where we know it's safe. An API like this is not safe.
>> +static int owl_timer_set_state_shutdown(struct clock_event_device *evt) >> +{ >> + writel(0, owl_timer_get_base(0) + OWL_Tx_CTL); > > return owl_timer_set_state_disable(evt); > >> + >> + return 0; >> +} >> + >> +static int owl_timer_set_state_oneshot(struct clock_event_device *evt) >> +{ >> + owl_timer_reset(1); > > Do you really need to do reset here ? Why not just owl_timer_set_state_disable(evt) ?
Matches downstream, will consider changing.
>> + return 0; >> +} >> + >> +static int owl_timer_tick_resume(struct clock_event_device *evt) >> +{ >> + return 0; >> +} >> + >> +static int owl_timer_set_next_event(unsigned long evt, >> + struct clock_event_device *ev) >> +{ >> + void __iomem *base = owl_timer_get_base(1); >> + >> + writel(0, base + OWL_Tx_CTL); >> + >> + writel(0, base + OWL_Tx_VAL); >
Are you suggesting a while line here? The point was disable first, then initialize (2x), then activate. Maybe add comments instead?
>> + writel(evt, base + OWL_Tx_CMP); >> + >> + writel(OWL_Tx_CTL_EN | OWL_Tx_CTL_INTEN, base + OWL_Tx_CTL); >> + >> + return 0; >> +} >> + >> +static struct clock_event_device owl_clockevent = { >> + .name = "owl_tick", >> + .rating = 200, >> + .features = CLOCK_EVT_FEAT_ONESHOT | >> + CLOCK_EVT_FEAT_DYNIRQ, >> + .set_state_shutdown = owl_timer_set_state_shutdown, >> + .set_state_oneshot = owl_timer_set_state_oneshot, >> + .tick_resume = owl_timer_tick_resume, >> + .set_next_event = owl_timer_set_next_event, >> +}; >> + >> +static irqreturn_t owl_timer1_interrupt(int irq, void *dev_id) >> +{ >> + struct clock_event_device *evt = (struct clock_event_device *)dev_id; >> + >> + writel(OWL_Tx_CTL_PD, owl_timer_get_base(1) + OWL_Tx_CTL); >> + >> + evt->event_handler(evt);
Is there any guideline as to whether to clear such flag before or after?
>> + >> + return IRQ_HANDLED; >> +}
Regards, Andreas
-- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)
| |