| Message ID | 20170228063535.32069-5-afaerber@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + > + 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. > +} > + > +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. > + 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); } > + > +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); } > +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) ? > + 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); > + 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); > + > + return IRQ_HANDLED; > +} > + > +static const struct owl_timer_info s500_timer_info = { > + .timer_offset[0] = 0x08, > + .timer_offset[1] = 0x14, > +}; > + > +static const struct of_device_id owl_timer_of_matches[] = { > + { .compatible = "actions,s500-timer", .data = &s500_timer_info }, > + { } > +}; > + > +static int __init owl_timer_init(struct device_node *node) > +{ > + const struct of_device_id *match; > + struct clk *clk; > + unsigned long rate; > + int timer1_irq, i, ret; > + > + match = of_match_node(owl_timer_of_matches, node); > + if (!match || !match->data) { > + pr_err("Unknown compatible"); > + return -EINVAL; > + } > + > + owl_timer_info = match->data; > + > + owl_timer_base = of_io_request_and_map(node, 0, "owl-timer"); > + if (IS_ERR(owl_timer_base)) { > + pr_err("Can't map timer registers"); > + return PTR_ERR(owl_timer_base); > + } > + > + timer1_irq = of_irq_get_byname(node, "Timer1"); > + if (timer1_irq <= 0) { > + pr_err("Can't parse Timer1 IRQ"); > + return -EINVAL; > + } > + > + clk = of_clk_get(node, 0); > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > + > + rate = clk_get_rate(clk); > + > + for (i = 0; i < OWL_MAX_Tx; i++) > + owl_timer_reset(i); > + > + writel(OWL_Tx_CTL_EN, owl_timer_get_base(0) + OWL_Tx_CTL); > + > + sched_clock_register(owl_timer_sched_read, 32, rate); > + clocksource_mmio_init(owl_timer_get_base(0) + OWL_Tx_VAL, node->name, > + rate, 200, 32, clocksource_mmio_readl_up); > + > + ret = request_irq(timer1_irq, owl_timer1_interrupt, IRQF_TIMER, > + "owl-timer", &owl_clockevent); > + if (ret) { > + pr_err("failed to request irq %d\n", timer1_irq); > + return ret; > + } > + > + owl_clockevent.cpumask = cpumask_of(0); > + owl_clockevent.irq = timer1_irq; > + > + clockevents_config_and_register(&owl_clockevent, rate, > + 0xf, 0xffffffff); > + > + return 0; > +} > +CLOCKSOURCE_OF_DECLARE(owl_s500, "actions,s500-timer", owl_timer_init); > -- > 2.10.2 >
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
On Tue, Feb 28, 2017 at 06:08:06PM +0100, Andreas Färber wrote: [ ... ] > > 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... Not sure to get what the 'previous simpler behavior' is, but until it does not recompute the offset each time, I'm fine with that. > >> +} > >> + > >> +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. May be I missed something, but so far, the base addresses must be setup before reset is called, no? > >> + 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. I don't get the point. Writing this simple function has the benefit to give the reader the information about the disabling register. Even if it does make sense for you, for me it has its purpose when I try to factor out different drivers code. > >> +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? I meant, base + OWL_Tx_CTL and base + OWL_Tx_VAL are set to zero since the beginning. If their values do not change, it is not necessary to set their values to zero again. > >> + 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? Mmh, good question. I'm not sure it makes a different. > >> + > >> + 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)
Am 28.02.2017 um 18:39 schrieb Daniel Lezcano: > On Tue, Feb 28, 2017 at 06:08:06PM +0100, Andreas Färber wrote: >>> 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... > > Not sure to get what the 'previous simpler behavior' is, v2. :) > but until it does not > recompute the offset each time, I'm fine with that. >>>> +} >>>> + >>>> +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. > > May be I missed something, but so far, the base addresses must be setup before > reset is called, no? They are known in advance, yes. Where/how we set them up is the culprit. >>> 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. > > I don't get the point. Writing this simple function has the benefit to give the > reader the information about the disabling register. Even if it does make sense > for you, for me it has its purpose when I try to factor out different drivers > code. I mean a proper _disable() function would need to do: val = readl() val &= ~bit; writel(val) Not just writel(0), overwriting any other bits. Therefore an inline write would be faster - your concern elsewhere. I'll happily implement the proper API if you prefer. >>>> +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_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? > > I meant, base + OWL_Tx_CTL and base + OWL_Tx_VAL are set to zero since the > beginning. If their values do not change, it is not necessary to set their > values to zero again. This is a callback, which I thought is re-entrant. VAL changes when the timer is running, and CTL changes every time we enable the timer. We could call _reset() here, but then we would be initializing CMP twice, which again would be less performant then just setting the registers to their final values directly. >>>> + 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? > > Mmh, good question. I'm not sure it makes a different. > >>>> + >>>> + return IRQ_HANDLED; >>>> +} Thanks, Andreas
On Tue, 28 Feb 2017, Daniel Lezcano wrote: > On Tue, Feb 28, 2017 at 06:08:06PM +0100, Andreas Färber wrote: > > >> +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? > > Mmh, good question. I'm not sure it makes a different. It makes a difference depending on what this flag does. If it clears the current pending interrupt, then you _must_ do it before rearming the timer. Otherwise you might clear the pending interrupt of the rearmed timer (when the timeout is very small) which makes the machine wait forever for the next timer interrupt. Thanks, tglx
On Tue, 28 Feb 2017, Andreas Färber wrote: > This is a callback, which I thought is re-entrant. It's not reentrant at least not on the same CPU. On a SMP machine this function might be called concurrently on several cores (assumed that the whole thing is replicated across cores). > VAL changes when the timer is running, and CTL changes every time we > enable the timer. We could call _reset() here, but then we would be > initializing CMP twice, which again would be less performant then just > setting the registers to their final values directly. Makes sense. Thanks, tglx
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; + + return owl_timer_base + owl_timer_info->timer_offset[timer_nr]; +} + +static inline void owl_timer_reset(unsigned index) +{ + void __iomem *base; + + base = owl_timer_get_base(index); + if (!base) + return; + + writel(0, base + OWL_Tx_CTL); + writel(0, base + OWL_Tx_VAL); + writel(0, base + OWL_Tx_CMP); +} + +static u64 notrace owl_timer_sched_read(void) +{ + return (u64)readl(owl_timer_get_base(0) + OWL_Tx_VAL); +} + +static int owl_timer_set_state_shutdown(struct clock_event_device *evt) +{ + writel(0, owl_timer_get_base(0) + OWL_Tx_CTL); + + return 0; +} + +static int owl_timer_set_state_oneshot(struct clock_event_device *evt) +{ + owl_timer_reset(1); + + 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); + 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); + + return IRQ_HANDLED; +} + +static const struct owl_timer_info s500_timer_info = { + .timer_offset[0] = 0x08, + .timer_offset[1] = 0x14, +}; + +static const struct of_device_id owl_timer_of_matches[] = { + { .compatible = "actions,s500-timer", .data = &s500_timer_info }, + { } +}; + +static int __init owl_timer_init(struct device_node *node) +{ + const struct of_device_id *match; + struct clk *clk; + unsigned long rate; + int timer1_irq, i, ret; + + match = of_match_node(owl_timer_of_matches, node); + if (!match || !match->data) { + pr_err("Unknown compatible"); + return -EINVAL; + } + + owl_timer_info = match->data; + + owl_timer_base = of_io_request_and_map(node, 0, "owl-timer"); + if (IS_ERR(owl_timer_base)) { + pr_err("Can't map timer registers"); + return PTR_ERR(owl_timer_base); + } + + timer1_irq = of_irq_get_byname(node, "Timer1"); + if (timer1_irq <= 0) { + pr_err("Can't parse Timer1 IRQ"); + return -EINVAL; + } + + clk = of_clk_get(node, 0); + if (IS_ERR(clk)) + return PTR_ERR(clk); + + rate = clk_get_rate(clk); + + for (i = 0; i < OWL_MAX_Tx; i++) + owl_timer_reset(i); + + writel(OWL_Tx_CTL_EN, owl_timer_get_base(0) + OWL_Tx_CTL); + + sched_clock_register(owl_timer_sched_read, 32, rate); + clocksource_mmio_init(owl_timer_get_base(0) + OWL_Tx_VAL, node->name, + rate, 200, 32, clocksource_mmio_readl_up); + + ret = request_irq(timer1_irq, owl_timer1_interrupt, IRQF_TIMER, + "owl-timer", &owl_clockevent); + if (ret) { + pr_err("failed to request irq %d\n", timer1_irq); + return ret; + } + + owl_clockevent.cpumask = cpumask_of(0); + owl_clockevent.irq = timer1_irq; + + clockevents_config_and_register(&owl_clockevent, rate, + 0xf, 0xffffffff); + + return 0; +} +CLOCKSOURCE_OF_DECLARE(owl_s500, "actions,s500-timer", owl_timer_init);
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