| Message ID | 762d81b2cf12922ba28fb58b3ea4d5b7072abefb.1421821466.git.baruch@tkos.co.il (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/21/2015 07:36 AM, Baruch Siach wrote: > Add clocksource driver to the Conexant CX92755 SoC, part of the Digicolor SoCs > series. Hardware provides 8 timers, A to H. Timer A is dedicated to a future > watchdog driver so we don't use it here. Use timer B for sched_clock, and timer > C for clock_event. > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > drivers/clocksource/Makefile | 1 + > drivers/clocksource/timer-digicolor.c | 164 ++++++++++++++++++++++++++++++++++ > 2 files changed, 165 insertions(+) > create mode 100644 drivers/clocksource/timer-digicolor.c > > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile > index 94d90b24b56b..a993c108be67 100644 > --- a/drivers/clocksource/Makefile > +++ b/drivers/clocksource/Makefile > @@ -10,6 +10,7 @@ obj-$(CONFIG_SH_TIMER_TMU) += sh_tmu.o > obj-$(CONFIG_EM_TIMER_STI) += em_sti.o > obj-$(CONFIG_CLKBLD_I8253) += i8253.o > obj-$(CONFIG_CLKSRC_MMIO) += mmio.o > +obj-$(CONFIG_ARCH_DIGICOLOR) += timer-digicolor.o > obj-$(CONFIG_DW_APB_TIMER) += dw_apb_timer.o > obj-$(CONFIG_DW_APB_TIMER_OF) += dw_apb_timer_of.o > obj-$(CONFIG_CLKSRC_NOMADIK_MTU) += nomadik-mtu.o > diff --git a/drivers/clocksource/timer-digicolor.c b/drivers/clocksource/timer-digicolor.c > new file mode 100644 > index 000000000000..014c50675980 > --- /dev/null > +++ b/drivers/clocksource/timer-digicolor.c > @@ -0,0 +1,164 @@ > +/* > + * Conexant Digicolor timer driver > + * > + * Author: Baruch Siach <baruch@tkos.co.il> > + * > + * Copyright (C) 2014 Paradox Innovation Ltd. > + * > + * Based on: > + * Allwinner SoCs hstimer driver If this is based on the Allwinnner driver, may be you can have a look at the patchset sent by Maxim to make sure your implementation is complete ? https://lkml.org/lkml/2015/1/11/52 > + * Copyright (C) 2013 Maxime Ripard > + * > + * Maxime Ripard <maxime.ripard@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +/* > + * Conexant Digicolor SoCs have 8 configurable timers, named from "Timer A" to > + * "Timer H". Timer A is the only one with watchdog support, so it is dedicated > + * to the watchdog driver. This driver uses Timer B for sched_clock(), and > + * Timer C for clockevents. > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#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> > + > +enum { > + TIMER_A, > + TIMER_B, > + TIMER_C, > + TIMER_D, > + TIMER_E, > + TIMER_F, > + TIMER_G, > + TIMER_H, > +}; > + > +#define CONTROL(t) ((t)*8) > +#define COUNT(t) ((t)*8 + 4) > + > +static void __iomem *timer_base; > +static u32 ticks_per_jiffy; Mind to encapsulate those global variables in a structure and use container_of to access those fields like: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/mtk_timer.c#n56 ? > +static void digicolor_clkevt_mode(enum clock_event_mode mode, > + struct clock_event_device *clk) > +{ > + switch (mode) { > + case CLOCK_EVT_MODE_PERIODIC: > + writeb(0, timer_base + CONTROL(TIMER_C)); Even if that sounds overkill, please replace '0', '1' with whatever macro name (eg. TIMER_DISABLE/ENABLE). > + writel(ticks_per_jiffy, timer_base + COUNT(TIMER_C)); > + writeb(0x21, timer_base + CONTROL(TIMER_C)); Please replace 0x21 occurrences by a macro. > + break; > + case CLOCK_EVT_MODE_ONESHOT: > + writeb(0, timer_base + CONTROL(TIMER_C)); > + writeb(0x11, timer_base + CONTROL(TIMER_C)); Please replace 0x11 occurrences by a macro. > + break; > + case CLOCK_EVT_MODE_UNUSED: > + case CLOCK_EVT_MODE_SHUTDOWN: > + default: > + writeb(0, timer_base + CONTROL(TIMER_C)); > + break; > + } > +} > + > +static int digicolor_clkevt_next_event(unsigned long evt, > + struct clock_event_device *unused) > +{ > + writeb(0, timer_base + CONTROL(TIMER_C)); > + writel(evt, timer_base + COUNT(TIMER_C)); > + writeb(0x11, timer_base + CONTROL(TIMER_C)); ditto. > + return 0; > +} > + > +static struct clock_event_device digicolor_clockevent = { > + .name = "digicolor_tick", > + .rating = 340, > + .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, > + .set_mode = digicolor_clkevt_mode, > + .set_next_event = digicolor_clkevt_next_event, > +}; > + > + > +static irqreturn_t digicolor_timer_interrupt(int irq, void *dev_id) > +{ > + struct clock_event_device *evt = (struct clock_event_device *)dev_id; > + > + evt->event_handler(evt); > + > + return IRQ_HANDLED; > +} > + > +static struct irqaction digicolor_timer_irq = { > + .name = "digicolor_timerC", > + .flags = IRQF_TIMER | IRQF_IRQPOLL, > + .handler = digicolor_timer_interrupt, > + .dev_id = &digicolor_clockevent, > +}; The current trend is to use 'request_irq', so no such structure declaration/initialization is needed. > +static u64 digicolor_timer_sched_read(void) > +{ > + return ~readl(timer_base + COUNT(TIMER_B)); > +} > + > +static void __init digicolor_timer_init(struct device_node *node) > +{ > + unsigned long rate; > + struct clk *clk; > + int ret, irq; > + > + timer_base = of_iomap(node, 0); > + if (!timer_base) { > + pr_err("Can't map registers"); > + return; > + } > + > + irq = irq_of_parse_and_map(node, TIMER_C); > + if (irq <= 0) { > + pr_err("Can't parse IRQ"); > + return; > + } > + > + clk = of_clk_get(node, 0); > + if (IS_ERR(clk)) { > + pr_err("Can't get timer clock"); > + return; > + } > + clk_prepare_enable(clk); > + rate = clk_get_rate(clk); > + > + writeb(0, timer_base + CONTROL(TIMER_B)); > + writel(~0, timer_base + COUNT(TIMER_B)); s/~0/UINT_MAX/ ? > + writeb(1, timer_base + CONTROL(TIMER_B)); > + > + sched_clock_register(digicolor_timer_sched_read, 32, rate); > + clocksource_mmio_init(timer_base + COUNT(TIMER_B), node->name, > + rate, 340, 32, clocksource_mmio_readl_down); > + > + ticks_per_jiffy = DIV_ROUND_UP(rate, HZ); > + > + ret = setup_irq(irq, &digicolor_timer_irq); > + if (ret) > + pr_warn("failed to setup timer irq %d (%d)\n", irq, ret); s/setup_irq/request_irq/ > + > + digicolor_clockevent.cpumask = cpu_possible_mask; > + digicolor_clockevent.irq = irq; > + > + clockevents_config_and_register(&digicolor_clockevent, rate, 0, > + 0xffffffff); > +} > +CLOCKSOURCE_OF_DECLARE(conexant_digicolor, "cnxt,cx92755-timer", > + digicolor_timer_init); >
Hi Daniel, On Thu, Jan 22, 2015 at 01:54:39PM +0100, Daniel Lezcano wrote: > On 01/21/2015 07:36 AM, Baruch Siach wrote: > >+ * Based on: > >+ * Allwinner SoCs hstimer driver > > If this is based on the Allwinnner driver, may be you can have a look at the > patchset sent by Maxim to make sure your implementation is complete ? > > https://lkml.org/lkml/2015/1/11/52 Thanks for the tip. I'll take the applicable parts from that series. [...] > >+static void __iomem *timer_base; > >+static u32 ticks_per_jiffy; > > Mind to encapsulate those global variables in a structure and use > container_of to access those fields like: > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/mtk_timer.c#n56 > > ? The trouble is that the sched_clock callback (digicolor_timer_sched_read) needs a static timer_base to find the timer counter. Do you think adding a structure is worth it only for ticks_per_jiffy? > >+static void digicolor_clkevt_mode(enum clock_event_mode mode, > >+ struct clock_event_device *clk) > >+{ > >+ switch (mode) { > >+ case CLOCK_EVT_MODE_PERIODIC: > >+ writeb(0, timer_base + CONTROL(TIMER_C)); > > Even if that sounds overkill, please replace '0', '1' with whatever macro > name (eg. TIMER_DISABLE/ENABLE). Will do for the entire file. [...] > >+static struct irqaction digicolor_timer_irq = { > >+ .name = "digicolor_timerC", > >+ .flags = IRQF_TIMER | IRQF_IRQPOLL, > >+ .handler = digicolor_timer_interrupt, > >+ .dev_id = &digicolor_clockevent, > >+}; > > The current trend is to use 'request_irq', so no such structure > declaration/initialization is needed. Thanks. Will do. > >+ writel(~0, timer_base + COUNT(TIMER_B)); > > s/~0/UINT_MAX/ ? Ack. Thanks for reviewing. I'll post an updated series shortly. baruch
On 01/25/2015 03:46 PM, Baruch Siach wrote: > Hi Daniel, > > On Thu, Jan 22, 2015 at 01:54:39PM +0100, Daniel Lezcano wrote: >> On 01/21/2015 07:36 AM, Baruch Siach wrote: >>> + * Based on: >>> + * Allwinner SoCs hstimer driver >> >> If this is based on the Allwinnner driver, may be you can have a look at the >> patchset sent by Maxim to make sure your implementation is complete ? >> >> https://lkml.org/lkml/2015/1/11/52 > > Thanks for the tip. I'll take the applicable parts from that series. > > [...] > >>> +static void __iomem *timer_base; >>> +static u32 ticks_per_jiffy; >> >> Mind to encapsulate those global variables in a structure and use >> container_of to access those fields like: >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/mtk_timer.c#n56 >> >> ? > > The trouble is that the sched_clock callback (digicolor_timer_sched_read) > needs a static timer_base to find the timer counter. Do you think adding a > structure is worth it only for ticks_per_jiffy? Well you will have to define the global variable for this structure. So you should be able to access the timer base directly. I agree it is not perfect but at least that will encapsulate the code for the clockevents side. >>> +static void digicolor_clkevt_mode(enum clock_event_mode mode, >>> + struct clock_event_device *clk) >>> +{ >>> + switch (mode) { >>> + case CLOCK_EVT_MODE_PERIODIC: >>> + writeb(0, timer_base + CONTROL(TIMER_C)); >> >> Even if that sounds overkill, please replace '0', '1' with whatever macro >> name (eg. TIMER_DISABLE/ENABLE). > > Will do for the entire file. > > [...] > >>> +static struct irqaction digicolor_timer_irq = { >>> + .name = "digicolor_timerC", >>> + .flags = IRQF_TIMER | IRQF_IRQPOLL, >>> + .handler = digicolor_timer_interrupt, >>> + .dev_id = &digicolor_clockevent, >>> +}; >> >> The current trend is to use 'request_irq', so no such structure >> declaration/initialization is needed. > > Thanks. Will do. > >>> + writel(~0, timer_base + COUNT(TIMER_B)); >> >> s/~0/UINT_MAX/ ? > > Ack. > > Thanks for reviewing. I'll post an updated series shortly. Ok, thanks. -- Daniel
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile index 94d90b24b56b..a993c108be67 100644 --- a/drivers/clocksource/Makefile +++ b/drivers/clocksource/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_SH_TIMER_TMU) += sh_tmu.o obj-$(CONFIG_EM_TIMER_STI) += em_sti.o obj-$(CONFIG_CLKBLD_I8253) += i8253.o obj-$(CONFIG_CLKSRC_MMIO) += mmio.o +obj-$(CONFIG_ARCH_DIGICOLOR) += timer-digicolor.o obj-$(CONFIG_DW_APB_TIMER) += dw_apb_timer.o obj-$(CONFIG_DW_APB_TIMER_OF) += dw_apb_timer_of.o obj-$(CONFIG_CLKSRC_NOMADIK_MTU) += nomadik-mtu.o diff --git a/drivers/clocksource/timer-digicolor.c b/drivers/clocksource/timer-digicolor.c new file mode 100644 index 000000000000..014c50675980 --- /dev/null +++ b/drivers/clocksource/timer-digicolor.c @@ -0,0 +1,164 @@ +/* + * Conexant Digicolor timer driver + * + * Author: Baruch Siach <baruch@tkos.co.il> + * + * Copyright (C) 2014 Paradox Innovation Ltd. + * + * Based on: + * Allwinner SoCs hstimer driver + * + * Copyright (C) 2013 Maxime Ripard + * + * Maxime Ripard <maxime.ripard@free-electrons.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +/* + * Conexant Digicolor SoCs have 8 configurable timers, named from "Timer A" to + * "Timer H". Timer A is the only one with watchdog support, so it is dedicated + * to the watchdog driver. This driver uses Timer B for sched_clock(), and + * Timer C for clockevents. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#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> + +enum { + TIMER_A, + TIMER_B, + TIMER_C, + TIMER_D, + TIMER_E, + TIMER_F, + TIMER_G, + TIMER_H, +}; + +#define CONTROL(t) ((t)*8) +#define COUNT(t) ((t)*8 + 4) + +static void __iomem *timer_base; +static u32 ticks_per_jiffy; + +static void digicolor_clkevt_mode(enum clock_event_mode mode, + struct clock_event_device *clk) +{ + switch (mode) { + case CLOCK_EVT_MODE_PERIODIC: + writeb(0, timer_base + CONTROL(TIMER_C)); + writel(ticks_per_jiffy, timer_base + COUNT(TIMER_C)); + writeb(0x21, timer_base + CONTROL(TIMER_C)); + break; + case CLOCK_EVT_MODE_ONESHOT: + writeb(0, timer_base + CONTROL(TIMER_C)); + writeb(0x11, timer_base + CONTROL(TIMER_C)); + break; + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + default: + writeb(0, timer_base + CONTROL(TIMER_C)); + break; + } +} + +static int digicolor_clkevt_next_event(unsigned long evt, + struct clock_event_device *unused) +{ + writeb(0, timer_base + CONTROL(TIMER_C)); + writel(evt, timer_base + COUNT(TIMER_C)); + writeb(0x11, timer_base + CONTROL(TIMER_C)); + + return 0; +} + +static struct clock_event_device digicolor_clockevent = { + .name = "digicolor_tick", + .rating = 340, + .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, + .set_mode = digicolor_clkevt_mode, + .set_next_event = digicolor_clkevt_next_event, +}; + + +static irqreturn_t digicolor_timer_interrupt(int irq, void *dev_id) +{ + struct clock_event_device *evt = (struct clock_event_device *)dev_id; + + evt->event_handler(evt); + + return IRQ_HANDLED; +} + +static struct irqaction digicolor_timer_irq = { + .name = "digicolor_timerC", + .flags = IRQF_TIMER | IRQF_IRQPOLL, + .handler = digicolor_timer_interrupt, + .dev_id = &digicolor_clockevent, +}; + +static u64 digicolor_timer_sched_read(void) +{ + return ~readl(timer_base + COUNT(TIMER_B)); +} + +static void __init digicolor_timer_init(struct device_node *node) +{ + unsigned long rate; + struct clk *clk; + int ret, irq; + + timer_base = of_iomap(node, 0); + if (!timer_base) { + pr_err("Can't map registers"); + return; + } + + irq = irq_of_parse_and_map(node, TIMER_C); + if (irq <= 0) { + pr_err("Can't parse IRQ"); + return; + } + + clk = of_clk_get(node, 0); + if (IS_ERR(clk)) { + pr_err("Can't get timer clock"); + return; + } + clk_prepare_enable(clk); + rate = clk_get_rate(clk); + + writeb(0, timer_base + CONTROL(TIMER_B)); + writel(~0, timer_base + COUNT(TIMER_B)); + writeb(1, timer_base + CONTROL(TIMER_B)); + + sched_clock_register(digicolor_timer_sched_read, 32, rate); + clocksource_mmio_init(timer_base + COUNT(TIMER_B), node->name, + rate, 340, 32, clocksource_mmio_readl_down); + + ticks_per_jiffy = DIV_ROUND_UP(rate, HZ); + + ret = setup_irq(irq, &digicolor_timer_irq); + if (ret) + pr_warn("failed to setup timer irq %d (%d)\n", irq, ret); + + digicolor_clockevent.cpumask = cpu_possible_mask; + digicolor_clockevent.irq = irq; + + clockevents_config_and_register(&digicolor_clockevent, rate, 0, + 0xffffffff); +} +CLOCKSOURCE_OF_DECLARE(conexant_digicolor, "cnxt,cx92755-timer", + digicolor_timer_init);
Add clocksource driver to the Conexant CX92755 SoC, part of the Digicolor SoCs series. Hardware provides 8 timers, A to H. Timer A is dedicated to a future watchdog driver so we don't use it here. Use timer B for sched_clock, and timer C for clock_event. Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- drivers/clocksource/Makefile | 1 + drivers/clocksource/timer-digicolor.c | 164 ++++++++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+) create mode 100644 drivers/clocksource/timer-digicolor.c