| Message ID | 1374172501-26796-7-git-send-email-shc_work@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Alex, (Added Thomas and John in Cc in case they want to review the patch). On 07/18/2013 08:34 PM, Alexander Shiyan wrote: > This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. > Designed primarily for migration CLPS711X subarch for multiplatform & DT, > for this as the "OF" and "not-OF" calls implemented. When a new driver is submitted, I ask for a more detailed changelog about the driver itself: how it works, any specificities, etc ... That will help people to understand the code better at least for the review. > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > --- > arch/arm/Kconfig | 2 - > drivers/clocksource/Kconfig | 6 ++ > drivers/clocksource/Makefile | 1 + > drivers/clocksource/clps711x-clksrc.c | 151 ++++++++++++++++++++++++++++++++++ > 4 files changed, 158 insertions(+), 2 deletions(-) > create mode 100644 drivers/clocksource/clps711x-clksrc.c > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index dfb1e7f..5c60cb7 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -370,10 +370,8 @@ config ARCH_CLPS711X > bool "Cirrus Logic CLPS711x/EP721x/EP731x-based" > select ARCH_REQUIRE_GPIOLIB > select AUTO_ZRELADDR > - select CLKSRC_MMIO > select COMMON_CLK > select CPU_ARM720T > - select GENERIC_CLOCKEVENTS > select MFD_SYSCON > select MULTI_IRQ_HANDLER > select SPARSE_IRQ > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > index b7b9b04..bd6dc82 100644 > --- a/drivers/clocksource/Kconfig > +++ b/drivers/clocksource/Kconfig > @@ -41,6 +41,12 @@ config VT8500_TIMER > config CADENCE_TTC_TIMER > bool > > +config CLKSRC_CLPS711X > + def_bool y if ARCH_CLPS711X > + select CLKSRC_MMIO > + select CLKSRC_OF if OF > + select GENERIC_CLOCKEVENTS > + > config CLKSRC_NOMADIK_MTU > bool > depends on (ARCH_NOMADIK || ARCH_U8500) > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile > index 8b00c5c..da6c102 100644 > --- a/drivers/clocksource/Makefile > +++ b/drivers/clocksource/Makefile > @@ -12,6 +12,7 @@ obj-$(CONFIG_CLKBLD_I8253) += i8253.o > obj-$(CONFIG_CLKSRC_MMIO) += mmio.o > obj-$(CONFIG_DW_APB_TIMER) += dw_apb_timer.o > obj-$(CONFIG_DW_APB_TIMER_OF) += dw_apb_timer_of.o > +obj-$(CONFIG_CLKSRC_CLPS711X) += clps711x-clksrc.o > obj-$(CONFIG_CLKSRC_NOMADIK_MTU) += nomadik-mtu.o > obj-$(CONFIG_CLKSRC_DBX500_PRCMU) += clksrc-dbx500-prcmu.o > obj-$(CONFIG_ARMADA_370_XP_TIMER) += time-armada-370-xp.o > diff --git a/drivers/clocksource/clps711x-clksrc.c b/drivers/clocksource/clps711x-clksrc.c > new file mode 100644 > index 0000000..1749b0b > --- /dev/null > +++ b/drivers/clocksource/clps711x-clksrc.c > @@ -0,0 +1,151 @@ > +/* > + * CLPS711X clocksource driver > + * > + * Copyright (C) 2013 Alexander Shiyan <shc_work@mail.ru> > + * > + * 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/clocksource.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/io.h> > +#include <linux/ioport.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/sched_clock.h> > +#include <linux/slab.h> > + > +#include <linux/mfd/syscon/clps711x.h> > + > +#define CLPS711X_SYSCON1 (0x0100) > +#define CLPS711X_TC1D (0x0300) > +#define CLPS711X_TC2D (0x0340) Alignment. > + > +static struct { > + void __iomem *tc1d; > + int irq; > +} *clps711x_clksrc; You don't need to define this structure, the struct clock_event_device already contains a field with irq. > +static u32 notrace clps711x_sched_clock_read(void) > +{ > + return ~readw(clps711x_clksrc->tc1d); > +} > + > +static void clps711x_clockevent_set_mode(enum clock_event_mode mode, > + struct clock_event_device *evt) > +{ > + disable_irq(clps711x_clksrc->irq); Do you really need to disable the interrupt to re-enable it right after ? > +> + switch (mode) { > + case CLOCK_EVT_MODE_PERIODIC: > + enable_irq(clps711x_clksrc->irq); > + break; > + case CLOCK_EVT_MODE_ONESHOT: > + /* Not supported */ > + case CLOCK_EVT_MODE_SHUTDOWN: > + case CLOCK_EVT_MODE_UNUSED: > + case CLOCK_EVT_MODE_RESUME: > + /* Left event sources disabled, no more interrupts appear */ > + break; > + } > +} I am not expert in interrupts, but it is possible this interrupt could be shared ? There isn't a register to enable/disable the timer ? > + > +static struct clock_event_device clps711x_clockevent = { > + .name = "clps711x-clockevent", > + .rating = 300, > + .features = CLOCK_EVT_FEAT_PERIODIC, > + .set_mode = clps711x_clockevent_set_mode, > +}; > + > +static irqreturn_t clps711x_timer_interrupt(int irq, void *dev_id) > +{ > + clps711x_clockevent.event_handler(&clps711x_clockevent); > + > + return IRQ_HANDLED; > +} > + > +static struct irqaction clps711x_timer_irq = { > + .name = "clps711x-timer", > + .flags = IRQF_TIMER | IRQF_IRQPOLL, Why do you need IRQF_IRQPOOL ? > + .handler = clps711x_timer_interrupt, > +}; > + > +static void __init _clps711x_clksrc_init(phys_addr_t phys_base, int irq, > + struct clk *tc1, struct clk *tc2) > +{ > + unsigned long tc1_rate, tc2_rate; > + void __iomem *tc2d, *syscon1; > + u32 tmp; > + > + BUG_ON(IS_ERR(tc1) || IS_ERR(tc2)); > + > + BUG_ON(!request_mem_region(phys_base + CLPS711X_TC1D, SZ_2, NULL)); > + BUG_ON(!request_mem_region(phys_base + CLPS711X_TC2D, SZ_2, NULL)); > + > + clps711x_clksrc = kzalloc(sizeof(*clps711x_clksrc), GFP_KERNEL); > + BUG_ON(!clps711x_clksrc); > + > + clps711x_clksrc->tc1d = ioremap(phys_base + CLPS711X_TC1D, SZ_2); > + BUG_ON(!clps711x_clksrc->tc1d); > + > + tc2d = ioremap(phys_base + CLPS711X_TC2D, SZ_2); > + BUG_ON(!tc2d); > + > + syscon1 = ioremap(phys_base + CLPS711X_SYSCON1, SZ_4); > + BUG_ON(!syscon1); > + > + clps711x_clksrc->irq = irq; clps711x_clockevent.irq = irq; clps711x_clockevent.cpumask = ? > + tmp = readl(syscon1); > + /* TC1 in free running mode */ > + tmp &= ~SYSCON1_TC1M; > + /* TC2 in prescale mode */ > + tmp |= SYSCON1_TC2M; > + writel(tmp, syscon1); Could you encapsulate these calls inside a static inline function with a more detailed comment please ? > + > + tc1_rate = clk_get_rate(tc1); > + tc2_rate = clk_get_rate(tc2); > + > + clocksource_mmio_init(clps711x_clksrc->tc1d, "clps711x-clocksource", > + tc1_rate, 300, 16, clocksource_mmio_readw_down); > + > + setup_sched_clock(clps711x_sched_clock_read, 16, tc1_rate); > + > + /* Set Timer prescaler */ > + writew(DIV_ROUND_CLOSEST(tc2_rate, HZ), tc2d); > + > + clockevents_config_and_register(&clps711x_clockevent, tc2_rate, 0, 0); > + > + BUG_ON(setup_irq(clps711x_clksrc->irq, &clps711x_timer_irq)); > +} > + > +void __init clps711x_clksrc_init(phys_addr_t phys_base, int irq) Is this function called somewhere ? I don't see the function definition ? > +{ > + struct clk *tc1 = clk_get(NULL, "tc1"); > + struct clk *tc2 = clk_get(NULL, "tc2"); > + > + _clps711x_clksrc_init(phys_base, irq, tc1, tc2); > +} > + > +#ifdef CONFIG_CLKSRC_OF > +static void __init clps711x_clksrc_init_dt(struct device_node *np) > +{ > + struct clk *tc1 = of_clk_get_by_name(np, "tc1"); > + struct clk *tc2 = of_clk_get_by_name(np, "tc2"); > + struct resource res_io, res_irq; > + > + BUG_ON(!of_address_to_resource(np, 0, &res_io)); > + > + BUG_ON(!of_irq_to_resource(np, 0, &res_irq)); > + > + _clps711x_clksrc_init(res_io.start, res_irq.start, tc1, tc2); > +} > +CLOCKSOURCE_OF_DECLARE(clps711x, "cirrus,clps711x-clocksource", > + clps711x_clksrc_init_dt); > +#endif >
On Fri, 19 Jul 2013 16:38:54 +0200 Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > (Added Thomas and John in Cc in case they want to review the patch). > > On 07/18/2013 08:34 PM, Alexander Shiyan wrote: > > This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. > > Designed primarily for migration CLPS711X subarch for multiplatform & DT, > > for this as the "OF" and "not-OF" calls implemented. > > When a new driver is submitted, I ask for a more detailed changelog > about the driver itself: how it works, any specificities, etc ... > > That will help people to understand the code better at least for the review. Basically, this series is a port of the CLPS711X subsystems in places specially designed for this purpose. Additionally added to the standard description of the subsystem for use with DT. As for specs, I'm a little confused by this question. In this case, I'm doing is not anything new for this CPU. The CPU has two timers and I use both to provide the correct time sources. > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > > --- > > arch/arm/Kconfig | 2 - > > drivers/clocksource/Kconfig | 6 ++ > > drivers/clocksource/Makefile | 1 + > > drivers/clocksource/clps711x-clksrc.c | 151 ++++++++++++++++++++++++++++++++++ > > 4 files changed, 158 insertions(+), 2 deletions(-) > > create mode 100644 drivers/clocksource/clps711x-clksrc.c [...] > > +#define CLPS711X_SYSCON1 (0x0100) > > +#define CLPS711X_TC1D (0x0300) > > +#define CLPS711X_TC2D (0x0340) > > Alignment. Alignment? Do not see any problems. Wrong alignment in this case is the result of quote. > > +static struct { > > + void __iomem *tc1d; > > + int irq; > > +} *clps711x_clksrc; > > You don't need to define this structure, the struct clock_event_device > already contains a field with irq. Thank you, I will know. [...] > > +static void clps711x_clockevent_set_mode(enum clock_event_mode mode, > > + struct clock_event_device *evt) > > +{ > > + disable_irq(clps711x_clksrc->irq); > > Do you really need to disable the interrupt to re-enable it right after ? That was the original implementation. Perhaps the best solution is to disable interrupt by CLOCK_EVT_MODE_SHUTDOWN and enable it back by CLOCK_EVT_MODE_RESUME. > > +> + switch (mode) { > > + case CLOCK_EVT_MODE_PERIODIC: > > + enable_irq(clps711x_clksrc->irq); > > + break; > > + case CLOCK_EVT_MODE_ONESHOT: > > + /* Not supported */ > > + case CLOCK_EVT_MODE_SHUTDOWN: > > + case CLOCK_EVT_MODE_UNUSED: > > + case CLOCK_EVT_MODE_RESUME: > > + /* Left event sources disabled, no more interrupts appear */ > > + break; > > + } > > +} > > I am not expert in interrupts, but it is possible this interrupt could > be shared ? Timer interrupt? No. > There isn't a register to enable/disable the timer ? Unfortunately, no. The timer is running always, except STDBY CPU state, so we can control it only by enable/disable interrupt. [...] > > +static struct irqaction clps711x_timer_irq = { > > + .name = "clps711x-timer", > > + .flags = IRQF_TIMER | IRQF_IRQPOLL, > > Why do you need IRQF_IRQPOOL ? linux/interrupt.h: * IRQF_IRQPOLL - Interrupt is used for polling (only the interrupt that is * registered first in an shared interrupt is considered for * performance reasons) I do not see need this flag, but in fact, this flag is traditionally used in almost all subsystems of this type. [...] > > + tmp = readl(syscon1); > > + /* TC1 in free running mode */ > > + tmp &= ~SYSCON1_TC1M; > > + /* TC2 in prescale mode */ > > + tmp |= SYSCON1_TC2M; > > + writel(tmp, syscon1); > > Could you encapsulate these calls inside a static inline function with a > more detailed comment please ? Why static inline? There is a comment line for each bit, You think this is not enough? [...] > > +void __init clps711x_clksrc_init(phys_addr_t phys_base, int irq) > > Is this function called somewhere ? I don't see the function definition ? It is defined in the patch [9/10] for non-DT case. [...] Thanks.
On 07/20/2013 05:44 AM, Alexander Shiyan wrote: > On Fri, 19 Jul 2013 16:38:54 +0200 > Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> (Added Thomas and John in Cc in case they want to review the patch). >> >> On 07/18/2013 08:34 PM, Alexander Shiyan wrote: >>> This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. >>> Designed primarily for migration CLPS711X subarch for multiplatform & DT, >>> for this as the "OF" and "not-OF" calls implemented. >> >> When a new driver is submitted, I ask for a more detailed changelog >> about the driver itself: how it works, any specificities, etc ... >> >> That will help people to understand the code better at least for the review. > > Basically, this series is a port of the CLPS711X subsystems in places specially > designed for this purpose. Additionally added to the standard description of the > subsystem for use with DT. > As for specs, I'm a little confused by this question. In this case, I'm doing > is not anything new for this CPU. The CPU has two timers and I use both to > provide the correct time sources. I understand there is nothing extra in the code but I was talking about some specificity of the driver if there are (eg. timer must be disabled with disable/enable irq). >>> Signed-off-by: Alexander Shiyan <shc_work@mail.ru> >>> --- >>> arch/arm/Kconfig | 2 - >>> drivers/clocksource/Kconfig | 6 ++ >>> drivers/clocksource/Makefile | 1 + >>> drivers/clocksource/clps711x-clksrc.c | 151 ++++++++++++++++++++++++++++++++++ >>> 4 files changed, 158 insertions(+), 2 deletions(-) >>> create mode 100644 drivers/clocksource/clps711x-clksrc.c > [...] >>> +#define CLPS711X_SYSCON1 (0x0100) >>> +#define CLPS711X_TC1D (0x0300) >>> +#define CLPS711X_TC2D (0x0340) >> >> Alignment. > > Alignment? Do not see any problems. > Wrong alignment in this case is the result of quote. Ok. >>> +static struct { >>> + void __iomem *tc1d; >>> + int irq; >>> +} *clps711x_clksrc; >> >> You don't need to define this structure, the struct clock_event_device >> already contains a field with irq. > > Thank you, I will know. > > [...] >>> +static void clps711x_clockevent_set_mode(enum clock_event_mode mode, >>> + struct clock_event_device *evt) >>> +{ >>> + disable_irq(clps711x_clksrc->irq); >> >> Do you really need to disable the interrupt to re-enable it right after ? > > That was the original implementation. Perhaps the best solution is to disable > interrupt by CLOCK_EVT_MODE_SHUTDOWN and enable it back by CLOCK_EVT_MODE_RESUME. > >>> +> + switch (mode) { >>> + case CLOCK_EVT_MODE_PERIODIC: >>> + enable_irq(clps711x_clksrc->irq); >>> + break; >>> + case CLOCK_EVT_MODE_ONESHOT: >>> + /* Not supported */ >>> + case CLOCK_EVT_MODE_SHUTDOWN: >>> + case CLOCK_EVT_MODE_UNUSED: >>> + case CLOCK_EVT_MODE_RESUME: >>> + /* Left event sources disabled, no more interrupts appear */ >>> + break; >>> + } >>> +} >> >> I am not expert in interrupts, but it is possible this interrupt could >> be shared ? > > Timer interrupt? No. > >> There isn't a register to enable/disable the timer ? > > Unfortunately, no. The timer is running always, except STDBY CPU state, > so we can control it only by enable/disable interrupt. Ok. > [...] >>> +static struct irqaction clps711x_timer_irq = { >>> + .name = "clps711x-timer", >>> + .flags = IRQF_TIMER | IRQF_IRQPOLL, >> >> Why do you need IRQF_IRQPOOL ? > > linux/interrupt.h: > * IRQF_IRQPOLL - Interrupt is used for polling (only the interrupt that is > * registered first in an shared interrupt is considered for > * performance reasons) > > I do not see need this flag, but in fact, this flag is traditionally used > in almost all subsystems of this type. So IIUC, IRQPOLL is to switch to polling mode when a lot of interrupts occur, right ? > [...] >>> + tmp = readl(syscon1); >>> + /* TC1 in free running mode */ >>> + tmp &= ~SYSCON1_TC1M; >>> + /* TC2 in prescale mode */ >>> + tmp |= SYSCON1_TC2M; >>> + writel(tmp, syscon1); >> >> Could you encapsulate these calls inside a static inline function with a >> more detailed comment please ? > > Why static inline? > There is a comment line for each bit, You think this is not enough? I prefer to encapsulate readl/writel calls inside a small function with an explicit name, comments are still welcome. > [...] >>> +void __init clps711x_clksrc_init(phys_addr_t phys_base, int irq) >> >> Is this function called somewhere ? I don't see the function definition ? > > It is defined in the patch [9/10] for non-DT case. Ok, thanks.
On Sat, 20 Jul 2013 23:48:30 +0200 Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 07/20/2013 05:44 AM, Alexander Shiyan wrote: > > On Fri, 19 Jul 2013 16:38:54 +0200 > > Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > >> (Added Thomas and John in Cc in case they want to review the patch). > >> > >> On 07/18/2013 08:34 PM, Alexander Shiyan wrote: > >>> This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. > >>> Designed primarily for migration CLPS711X subarch for multiplatform & DT, > >>> for this as the "OF" and "not-OF" calls implemented. [...] > >>> +static struct irqaction clps711x_timer_irq = { > >>> + .name = "clps711x-timer", > >>> + .flags = IRQF_TIMER | IRQF_IRQPOLL, > >> > >> Why do you need IRQF_IRQPOOL ? > > > > linux/interrupt.h: > > * IRQF_IRQPOLL - Interrupt is used for polling (only the interrupt that is > > * registered first in an shared interrupt is considered for > > * performance reasons) > > > > I do not see need this flag, but in fact, this flag is traditionally used > > in almost all subsystems of this type. > > So IIUC, IRQPOLL is to switch to polling mode when a lot of interrupts > occur, right ? I'm also not an expert on these flags, but realized appointment just like you. > >>> + tmp = readl(syscon1); > >>> + /* TC1 in free running mode */ > >>> + tmp &= ~SYSCON1_TC1M; > >>> + /* TC2 in prescale mode */ > >>> + tmp |= SYSCON1_TC2M; > >>> + writel(tmp, syscon1); > >> > >> Could you encapsulate these calls inside a static inline function with a > >> more detailed comment please ? > > > > Why static inline? > > There is a comment line for each bit, You think this is not enough? > > I prefer to encapsulate readl/writel calls inside a small function with > an explicit name, comments are still welcome. Ok. [...]
On Thu, Jul 18, 2013 at 07:34:57PM +0100, Alexander Shiyan wrote: > This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. > Designed primarily for migration CLPS711X subarch for multiplatform & DT, > for this as the "OF" and "not-OF" calls implemented. > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > --- [...] > diff --git a/drivers/clocksource/clps711x-clksrc.c b/drivers/clocksource/clps711x-clksrc.c > new file mode 100644 > index 0000000..1749b0b > --- /dev/null > +++ b/drivers/clocksource/clps711x-clksrc.c > @@ -0,0 +1,151 @@ > +/* > + * CLPS711X clocksource driver > + * > + * Copyright (C) 2013 Alexander Shiyan <shc_work@mail.ru> > + * > + * 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/clocksource.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/io.h> > +#include <linux/ioport.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/sched_clock.h> > +#include <linux/slab.h> > + > +#include <linux/mfd/syscon/clps711x.h> > + > +#define CLPS711X_SYSCON1 (0x0100) > +#define CLPS711X_TC1D (0x0300) > +#define CLPS711X_TC2D (0x0340) > + > +static struct { > + void __iomem *tc1d; > + int irq; > +} *clps711x_clksrc; > + > +static u32 notrace clps711x_sched_clock_read(void) > +{ > + return ~readw(clps711x_clksrc->tc1d); > +} > + > +static void clps711x_clockevent_set_mode(enum clock_event_mode mode, > + struct clock_event_device *evt) > +{ > + disable_irq(clps711x_clksrc->irq); > + > + switch (mode) { > + case CLOCK_EVT_MODE_PERIODIC: > + enable_irq(clps711x_clksrc->irq); > + break; > + case CLOCK_EVT_MODE_ONESHOT: > + /* Not supported */ > + case CLOCK_EVT_MODE_SHUTDOWN: > + case CLOCK_EVT_MODE_UNUSED: > + case CLOCK_EVT_MODE_RESUME: > + /* Left event sources disabled, no more interrupts appear */ > + break; > + } > +} > + > +static struct clock_event_device clps711x_clockevent = { > + .name = "clps711x-clockevent", > + .rating = 300, > + .features = CLOCK_EVT_FEAT_PERIODIC, > + .set_mode = clps711x_clockevent_set_mode, > +}; This seems to be a global clockevent, or a CPU0-only clockevent. Please set the cpumask to clarify this, or core clockevent code will scream at you (see clockevents_register_device). I assume this doesn't stop in low power states (CLOCK_EVT_FEAT_C3STOP)? > + > +static irqreturn_t clps711x_timer_interrupt(int irq, void *dev_id) > +{ > + clps711x_clockevent.event_handler(&clps711x_clockevent); > + > + return IRQ_HANDLED; > +} > + > +static struct irqaction clps711x_timer_irq = { > + .name = "clps711x-timer", > + .flags = IRQF_TIMER | IRQF_IRQPOLL, > + .handler = clps711x_timer_interrupt, > +}; Is there any reason you need to use irqaction and can't use {request,free}_irq? > + > +static void __init _clps711x_clksrc_init(phys_addr_t phys_base, int irq, > + struct clk *tc1, struct clk *tc2) > +{ > + unsigned long tc1_rate, tc2_rate; > + void __iomem *tc2d, *syscon1; > + u32 tmp; > + > + BUG_ON(IS_ERR(tc1) || IS_ERR(tc2)); Is this the only timer possible in the SoCs it'll be used in? You might not be capable of initialising this timer, but another timer may be able to keep the system running... I don't think the BUGs are necessary, and more robust failure handling would be preferable. > + > + BUG_ON(!request_mem_region(phys_base + CLPS711X_TC1D, SZ_2, NULL)); > + BUG_ON(!request_mem_region(phys_base + CLPS711X_TC2D, SZ_2, NULL)); > + > + clps711x_clksrc = kzalloc(sizeof(*clps711x_clksrc), GFP_KERNEL); > + BUG_ON(!clps711x_clksrc); > + > + clps711x_clksrc->tc1d = ioremap(phys_base + CLPS711X_TC1D, SZ_2); > + BUG_ON(!clps711x_clksrc->tc1d); > + > + tc2d = ioremap(phys_base + CLPS711X_TC2D, SZ_2); > + BUG_ON(!tc2d); > + > + syscon1 = ioremap(phys_base + CLPS711X_SYSCON1, SZ_4); > + BUG_ON(!syscon1); > + > + clps711x_clksrc->irq = irq; > + > + tmp = readl(syscon1); > + /* TC1 in free running mode */ > + tmp &= ~SYSCON1_TC1M; > + /* TC2 in prescale mode */ > + tmp |= SYSCON1_TC2M; > + writel(tmp, syscon1); > + > + tc1_rate = clk_get_rate(tc1); > + tc2_rate = clk_get_rate(tc2); > + > + clocksource_mmio_init(clps711x_clksrc->tc1d, "clps711x-clocksource", > + tc1_rate, 300, 16, clocksource_mmio_readw_down); > + > + setup_sched_clock(clps711x_sched_clock_read, 16, tc1_rate); > + > + /* Set Timer prescaler */ > + writew(DIV_ROUND_CLOSEST(tc2_rate, HZ), tc2d); > + > + clockevents_config_and_register(&clps711x_clockevent, tc2_rate, 0, 0); > + > + BUG_ON(setup_irq(clps711x_clksrc->irq, &clps711x_timer_irq)); I think the last two actions should be reordered, what if upon registering the timer the clockevents layer decides to program the timer? You don't have your interrupt set up, yet you'll try to disable and enable it... Thanks, Mark.
On Fri, 2 Aug 2013 11:46:35 +0100 Mark Rutland <mark.rutland@arm.com> wrote: > On Thu, Jul 18, 2013 at 07:34:57PM +0100, Alexander Shiyan wrote: > > This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. > > Designed primarily for migration CLPS711X subarch for multiplatform & DT, > > for this as the "OF" and "not-OF" calls implemented. > > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > > --- > > [...] > > +static struct clock_event_device clps711x_clockevent = { > > + .name = "clps711x-clockevent", > > + .rating = 300, > > + .features = CLOCK_EVT_FEAT_PERIODIC, > > + .set_mode = clps711x_clockevent_set_mode, > > +}; > > This seems to be a global clockevent, or a CPU0-only clockevent. Please > set the cpumask to clarify this, or core clockevent code will scream at > you (see clockevents_register_device). > > I assume this doesn't stop in low power states (CLOCK_EVT_FEAT_C3STOP)? This feature is marked as "x86(64) specific" in the header. [...] > > +static void __init _clps711x_clksrc_init(phys_addr_t phys_base, int irq, > > + struct clk *tc1, struct clk *tc2) > > +{ > > + unsigned long tc1_rate, tc2_rate; > > + void __iomem *tc2d, *syscon1; > > + u32 tmp; > > + > > + BUG_ON(IS_ERR(tc1) || IS_ERR(tc2)); > > Is this the only timer possible in the SoCs it'll be used in? You might > not be capable of initialising this timer, but another timer may be able > to keep the system running... This CPU have two HW timers only. Both are used here.
On Sun, Aug 04, 2013 at 01:31:13PM +0100, Alexander Shiyan wrote: > On Fri, 2 Aug 2013 11:46:35 +0100 > Mark Rutland <mark.rutland@arm.com> wrote: > > > On Thu, Jul 18, 2013 at 07:34:57PM +0100, Alexander Shiyan wrote: > > > This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. > > > Designed primarily for migration CLPS711X subarch for multiplatform & DT, > > > for this as the "OF" and "not-OF" calls implemented. > > > > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > > > --- > > > > [...] > > > > +static struct clock_event_device clps711x_clockevent = { > > > + .name = "clps711x-clockevent", > > > + .rating = 300, > > > + .features = CLOCK_EVT_FEAT_PERIODIC, > > > + .set_mode = clps711x_clockevent_set_mode, > > > +}; > > > > This seems to be a global clockevent, or a CPU0-only clockevent. Please > > set the cpumask to clarify this, or core clockevent code will scream at > > you (see clockevents_register_device). > > > > I assume this doesn't stop in low power states (CLOCK_EVT_FEAT_C3STOP)? > > This feature is marked as "x86(64) specific" in the header. While the misfeature was at one point x86-specific, it's rather generic now. If your device doesn't stop when the CPU is put into a low power state, then that's far better and there's nothing to worry about. I just thought I'd check now to save any future time spent debugging odd stalls :) > > [...] > > > +static void __init _clps711x_clksrc_init(phys_addr_t phys_base, int irq, > > > + struct clk *tc1, struct clk *tc2) > > > +{ > > > + unsigned long tc1_rate, tc2_rate; > > > + void __iomem *tc2d, *syscon1; > > > + u32 tmp; > > > + > > > + BUG_ON(IS_ERR(tc1) || IS_ERR(tc2)); > > > > Is this the only timer possible in the SoCs it'll be used in? You might > > not be capable of initialising this timer, but another timer may be able > > to keep the system running... > > This CPU have two HW timers only. Both are used here. Ok. So there clocks aren't likely to be used in futures SoCs with other clocks? Thanks, Mark.
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index dfb1e7f..5c60cb7 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -370,10 +370,8 @@ config ARCH_CLPS711X bool "Cirrus Logic CLPS711x/EP721x/EP731x-based" select ARCH_REQUIRE_GPIOLIB select AUTO_ZRELADDR - select CLKSRC_MMIO select COMMON_CLK select CPU_ARM720T - select GENERIC_CLOCKEVENTS select MFD_SYSCON select MULTI_IRQ_HANDLER select SPARSE_IRQ diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index b7b9b04..bd6dc82 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -41,6 +41,12 @@ config VT8500_TIMER config CADENCE_TTC_TIMER bool +config CLKSRC_CLPS711X + def_bool y if ARCH_CLPS711X + select CLKSRC_MMIO + select CLKSRC_OF if OF + select GENERIC_CLOCKEVENTS + config CLKSRC_NOMADIK_MTU bool depends on (ARCH_NOMADIK || ARCH_U8500) diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile index 8b00c5c..da6c102 100644 --- a/drivers/clocksource/Makefile +++ b/drivers/clocksource/Makefile @@ -12,6 +12,7 @@ obj-$(CONFIG_CLKBLD_I8253) += i8253.o obj-$(CONFIG_CLKSRC_MMIO) += mmio.o obj-$(CONFIG_DW_APB_TIMER) += dw_apb_timer.o obj-$(CONFIG_DW_APB_TIMER_OF) += dw_apb_timer_of.o +obj-$(CONFIG_CLKSRC_CLPS711X) += clps711x-clksrc.o obj-$(CONFIG_CLKSRC_NOMADIK_MTU) += nomadik-mtu.o obj-$(CONFIG_CLKSRC_DBX500_PRCMU) += clksrc-dbx500-prcmu.o obj-$(CONFIG_ARMADA_370_XP_TIMER) += time-armada-370-xp.o diff --git a/drivers/clocksource/clps711x-clksrc.c b/drivers/clocksource/clps711x-clksrc.c new file mode 100644 index 0000000..1749b0b --- /dev/null +++ b/drivers/clocksource/clps711x-clksrc.c @@ -0,0 +1,151 @@ +/* + * CLPS711X clocksource driver + * + * Copyright (C) 2013 Alexander Shiyan <shc_work@mail.ru> + * + * 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/clocksource.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/io.h> +#include <linux/ioport.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/sched_clock.h> +#include <linux/slab.h> + +#include <linux/mfd/syscon/clps711x.h> + +#define CLPS711X_SYSCON1 (0x0100) +#define CLPS711X_TC1D (0x0300) +#define CLPS711X_TC2D (0x0340) + +static struct { + void __iomem *tc1d; + int irq; +} *clps711x_clksrc; + +static u32 notrace clps711x_sched_clock_read(void) +{ + return ~readw(clps711x_clksrc->tc1d); +} + +static void clps711x_clockevent_set_mode(enum clock_event_mode mode, + struct clock_event_device *evt) +{ + disable_irq(clps711x_clksrc->irq); + + switch (mode) { + case CLOCK_EVT_MODE_PERIODIC: + enable_irq(clps711x_clksrc->irq); + break; + case CLOCK_EVT_MODE_ONESHOT: + /* Not supported */ + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_RESUME: + /* Left event sources disabled, no more interrupts appear */ + break; + } +} + +static struct clock_event_device clps711x_clockevent = { + .name = "clps711x-clockevent", + .rating = 300, + .features = CLOCK_EVT_FEAT_PERIODIC, + .set_mode = clps711x_clockevent_set_mode, +}; + +static irqreturn_t clps711x_timer_interrupt(int irq, void *dev_id) +{ + clps711x_clockevent.event_handler(&clps711x_clockevent); + + return IRQ_HANDLED; +} + +static struct irqaction clps711x_timer_irq = { + .name = "clps711x-timer", + .flags = IRQF_TIMER | IRQF_IRQPOLL, + .handler = clps711x_timer_interrupt, +}; + +static void __init _clps711x_clksrc_init(phys_addr_t phys_base, int irq, + struct clk *tc1, struct clk *tc2) +{ + unsigned long tc1_rate, tc2_rate; + void __iomem *tc2d, *syscon1; + u32 tmp; + + BUG_ON(IS_ERR(tc1) || IS_ERR(tc2)); + + BUG_ON(!request_mem_region(phys_base + CLPS711X_TC1D, SZ_2, NULL)); + BUG_ON(!request_mem_region(phys_base + CLPS711X_TC2D, SZ_2, NULL)); + + clps711x_clksrc = kzalloc(sizeof(*clps711x_clksrc), GFP_KERNEL); + BUG_ON(!clps711x_clksrc); + + clps711x_clksrc->tc1d = ioremap(phys_base + CLPS711X_TC1D, SZ_2); + BUG_ON(!clps711x_clksrc->tc1d); + + tc2d = ioremap(phys_base + CLPS711X_TC2D, SZ_2); + BUG_ON(!tc2d); + + syscon1 = ioremap(phys_base + CLPS711X_SYSCON1, SZ_4); + BUG_ON(!syscon1); + + clps711x_clksrc->irq = irq; + + tmp = readl(syscon1); + /* TC1 in free running mode */ + tmp &= ~SYSCON1_TC1M; + /* TC2 in prescale mode */ + tmp |= SYSCON1_TC2M; + writel(tmp, syscon1); + + tc1_rate = clk_get_rate(tc1); + tc2_rate = clk_get_rate(tc2); + + clocksource_mmio_init(clps711x_clksrc->tc1d, "clps711x-clocksource", + tc1_rate, 300, 16, clocksource_mmio_readw_down); + + setup_sched_clock(clps711x_sched_clock_read, 16, tc1_rate); + + /* Set Timer prescaler */ + writew(DIV_ROUND_CLOSEST(tc2_rate, HZ), tc2d); + + clockevents_config_and_register(&clps711x_clockevent, tc2_rate, 0, 0); + + BUG_ON(setup_irq(clps711x_clksrc->irq, &clps711x_timer_irq)); +} + +void __init clps711x_clksrc_init(phys_addr_t phys_base, int irq) +{ + struct clk *tc1 = clk_get(NULL, "tc1"); + struct clk *tc2 = clk_get(NULL, "tc2"); + + _clps711x_clksrc_init(phys_base, irq, tc1, tc2); +} + +#ifdef CONFIG_CLKSRC_OF +static void __init clps711x_clksrc_init_dt(struct device_node *np) +{ + struct clk *tc1 = of_clk_get_by_name(np, "tc1"); + struct clk *tc2 = of_clk_get_by_name(np, "tc2"); + struct resource res_io, res_irq; + + BUG_ON(!of_address_to_resource(np, 0, &res_io)); + + BUG_ON(!of_irq_to_resource(np, 0, &res_irq)); + + _clps711x_clksrc_init(res_io.start, res_irq.start, tc1, tc2); +} +CLOCKSOURCE_OF_DECLARE(clps711x, "cirrus,clps711x-clocksource", + clps711x_clksrc_init_dt); +#endif
This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT, for this as the "OF" and "not-OF" calls implemented. Signed-off-by: Alexander Shiyan <shc_work@mail.ru> --- arch/arm/Kconfig | 2 - drivers/clocksource/Kconfig | 6 ++ drivers/clocksource/Makefile | 1 + drivers/clocksource/clps711x-clksrc.c | 151 ++++++++++++++++++++++++++++++++++ 4 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 drivers/clocksource/clps711x-clksrc.c