On Mon, Jan 23, 2017 at 08:54:23AM -0500, Chris Brandt wrote: > This patch adds a OSTM driver for the Renesas architecture.
As it is a new driver, please give technical details for the log. Replace ioread/write by readl/writel in the code. > Signed-off-by: Chris Brandt <[email protected]> > > --- > v2: > * changed implementation to be independent channel nodes > --- > arch/arm/mach-shmobile/Kconfig | 1 + > drivers/clocksource/Kconfig | 12 ++ > drivers/clocksource/Makefile | 1 + > drivers/clocksource/renesas-ostm.c | 349 > +++++++++++++++++++++++++++++++++++++ > 4 files changed, 363 insertions(+) > create mode 100644 drivers/clocksource/renesas-ostm.c > > diff --git a/arch/arm/mach-shmobile/Kconfig b/arch/arm/mach-shmobile/Kconfig > index 2bb4b09..b928634 100644 > --- a/arch/arm/mach-shmobile/Kconfig > +++ b/arch/arm/mach-shmobile/Kconfig > @@ -57,6 +57,7 @@ config ARCH_R7S72100 > select PM > select PM_GENERIC_DOMAINS > select SYS_SUPPORTS_SH_MTU2 > + select SYS_SUPPORTS_RENESAS_OSTM - select SYS_SUPPORTS_RENESAS_OSTM + select RENESAS_OSTM > > config ARCH_R8A73A4 > bool "R-Mobile APE6 (R8A73A40)" > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > index 4866f7a..95c8d56 100644 > --- a/drivers/clocksource/Kconfig > +++ b/drivers/clocksource/Kconfig > @@ -431,6 +431,9 @@ config MTK_TIMER > config SYS_SUPPORTS_SH_MTU2 > bool > > +config SYS_SUPPORTS_RENESAS_OSTM > + bool > + -config SYS_SUPPORTS_RENESAS_OSTM - bool - > config SYS_SUPPORTS_SH_TMU > bool > > @@ -467,6 +470,15 @@ config SH_TIMER_MTU2 > Timer Pulse Unit 2 (MTU2) hardware available on SoCs from Renesas. > This hardware comes with 16 bit-timer registers. > > +config RENESAS_OSTM > + bool "Renesas OSTM timer driver" if COMPILE_TEST > + depends on GENERIC_CLOCKEVENTS > + select CLKSRC_MMIO > + default SYS_SUPPORTS_RENESAS_OSTM - default SYS_SUPPORTS_RENESAS_OSTM Except I missing something, I don' the point of having this intermediate config option. > + help > + This enables the build of the OSTM timer driver. > + It creates a clock source and clock event device. > + > config SH_TIMER_TMU > bool "Renesas TMU timer driver" if COMPILE_TEST > depends on GENERIC_CLOCKEVENTS > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile > index a14111e..bbd163b 100644 > --- a/drivers/clocksource/Makefile > +++ b/drivers/clocksource/Makefile > @@ -8,6 +8,7 @@ obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC) += cs5535-clockevt.o > obj-$(CONFIG_CLKSRC_JCORE_PIT) += jcore-pit.o > obj-$(CONFIG_SH_TIMER_CMT) += sh_cmt.o > obj-$(CONFIG_SH_TIMER_MTU2) += sh_mtu2.o > +obj-$(CONFIG_RENESAS_OSTM) += renesas-ostm.o > obj-$(CONFIG_SH_TIMER_TMU) += sh_tmu.o > obj-$(CONFIG_EM_TIMER_STI) += em_sti.o > obj-$(CONFIG_CLKBLD_I8253) += i8253.o > diff --git a/drivers/clocksource/renesas-ostm.c > b/drivers/clocksource/renesas-ostm.c > new file mode 100644 > index 0000000..37f2461 > --- /dev/null > +++ b/drivers/clocksource/renesas-ostm.c > @@ -0,0 +1,349 @@ > +/* > + * Renesas Timer Support - OSTM > + * > + * 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 > + * > + * This program is distributed in the hope that 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. > + * > + */ > + > +#include <linux/platform_device.h> > +#include <linux/clk.h> > +#include <linux/clockchips.h> > +#include <linux/interrupt.h> > +#include <linux/slab.h> > +#include <linux/module.h> Please remove everything module related here and below. This driver is not supposed to be removed and it is compiled in with the Kconfig option. > +#include <linux/pm_runtime.h> > +#include <linux/sched_clock.h> > + > +/* > + * The OSTM contains independent channels. > + * The first OSTM channel probed will be set up as a free running > + * clocksource. Additionally we will use this clocksource for the system > + * schedule timer sched_clock(). > + * > + * The second (or more) channel probed will be set up as an interrupt > + * driven clock event. > + */ > + > +struct ostm_device { > + struct platform_device *pdev; > + int irq; > + struct clk *clk; > + unsigned long rate; > + void __iomem *base; > + unsigned long ticks_per_jiffy; > + struct clock_event_device ced; > +}; You can probably reduce the size of this structure by removing some fields which are used only at init time. > + > +static void __iomem *system_clock; /* For sched_clock() */ > + > +/* OSTM REGISTERS */ > +#define OSTM_CMP 0x000 /* RW,32 */ > +#define OSTM_CNT 0x004 /* R,32 */ > +#define OSTM_TE 0x010 /* R,8 */ > +#define OSTM_TS 0x014 /* W,8 */ > +#define OSTM_TT 0x018 /* W,8 */ > +#define OSTM_CTL 0x020 /* RW,8 */ > + > +#define TE 0x01 > +#define TS 0x01 > +#define TT 0x01 > +#define CTL_PERIODIC 0x00 > +#define CTL_ONESHOT 0x02 > +#define CTL_FREERUN 0x02 > + > +static struct ostm_device *ced_to_ostm(struct clock_event_device *ced) > +{ > + return container_of(ced, struct ostm_device, ced); > +} > + > +static int __init ostm_init_clksrc(struct ostm_device *ostm) > +{ > + int ret; > + > + /* irq not used (clock sources don't use interrupts) */ > + > + /* stop counter */ One line comment is usually in the network code. Please use multiline. /* * Bla bla */ > + iowrite8(TT, ostm->base + OSTM_TT); > + while (ioread8(ostm->base + OSTM_TE) & TE) > + ; Comment here for this dangerous loop. Explain why we can't be stuck here. > + > + /* setup as freerun */ > + iowrite32(0, ostm->base + OSTM_CMP); > + iowrite8(CTL_FREERUN, ostm->base + OSTM_CTL); > + iowrite8(TS, ostm->base + OSTM_TS); > + > + /* register */ > + ret = clocksource_mmio_init(ostm->base + OSTM_CNT, > + "ostm", ostm->rate, > + 300, 32, clocksource_mmio_readl_up); > + > + return ret; return clocksource_mmio_init(...); > +} > + > +static u64 notrace ostm_read_sched_clock(void) > +{ > + return ioread32(system_clock); > +} > + > +static int __init ostm_init_sched_clock(struct ostm_device *ostm) > +{ > + unsigned long flags; > + > + system_clock = ostm->base + OSTM_CNT; > + local_irq_save(flags); > + local_irq_disable(); 1. local_irq_disable() is not needed, local_irq_save() saves the irq flags and disables the irq. 2. Why do you need to disable the irq here ? > + sched_clock_register(ostm_read_sched_clock, 32, ostm->rate); > + local_irq_restore(flags); > + > + return 0; > +} > + > +static void ostm_clkevt_timer_stop(struct ostm_device *ostm) > +{ > + if (ioread8(ostm->base + OSTM_TE) & TE) { > + iowrite8(TT, ostm->base + OSTM_TT); > + while (ioread8(ostm->base + OSTM_TE) & TE) > + ; Same comment as above. > + } > +} > + > +static int ostm_clock_event_next(unsigned long delta, > + struct clock_event_device *ced) > +{ > + struct ostm_device *ostm = ced_to_ostm(ced); > + > + WARN_ON(!clockevent_state_oneshot(ced)); Pointless check. It is up to the time framework to handle that and that is the case. > + > + ostm_clkevt_timer_stop(ostm); > + > + iowrite32(delta, ostm->base + OSTM_CMP); > + iowrite8(CTL_ONESHOT, ostm->base + OSTM_CTL); > + iowrite8(TS, ostm->base + OSTM_TS); > + > + return 0; > +} > + > +static int ostm_shutdown(struct clock_event_device *ced) > +{ > + struct ostm_device *ostm = ced_to_ostm(ced); > + > + ostm_clkevt_timer_stop(ostm); > + > + return 0; > +} > +static int ostm_set_periodic(struct clock_event_device *ced) > +{ > + struct ostm_device *ostm = ced_to_ostm(ced); > + > + if (clockevent_state_oneshot(ced) || clockevent_state_periodic(ced)) > + ostm_clkevt_timer_stop(ostm); > + > + iowrite32(ostm->ticks_per_jiffy - 1, ostm->base + OSTM_CMP); > + iowrite8(CTL_PERIODIC, ostm->base + OSTM_CTL); > + iowrite8(TS, ostm->base + OSTM_TS); > + > + return 0; > +} > + > +static int ostm_set_oneshot(struct clock_event_device *ced) > +{ > + struct ostm_device *ostm = ced_to_ostm(ced); > + > + ostm_clkevt_timer_stop(ostm); > + > + return 0; > +} > + > +static irqreturn_t ostm_timer_interrupt(int irq, void *dev_id) > +{ > + struct ostm_device *ostm = dev_id; > + > + if (clockevent_state_oneshot(&ostm->ced)) > + ostm_clkevt_timer_stop(ostm); > + > + /* notify clockevent layer */ > + if (ostm->ced.event_handler) > + ostm->ced.event_handler(&ostm->ced); > + > + return IRQ_HANDLED; > +} > + > +static int __init ostm_init_clkevt(struct ostm_device *ostm) > +{ > + struct clock_event_device *ced = &ostm->ced; > + int ret = -ENXIO; > + > + ret = request_irq(ostm->irq, ostm_timer_interrupt, > + IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING, > + dev_name(&ostm->pdev->dev), ostm); devm_request_irq Are you sure the IRQF_NOBALANCING flag should be used ? By default the interrupt is pinned to cpu0 below. If this timer is used as a broadcast timer for a system with deep idle, you may want to add the DYNIRQ flag feature to improve the wakeups on the system. > + if (ret) { > + dev_err(&ostm->pdev->dev, "failed to request irq\n"); > + return ret; > + } > + > + ced->name = "ostm"; > + ced->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC; > + ced->set_state_shutdown = ostm_shutdown; > + ced->set_state_periodic = ostm_set_periodic; > + ced->set_state_oneshot = ostm_set_oneshot; > + ced->set_next_event = ostm_clock_event_next; > + ced->shift = 32; > + ced->rating = 300; > + ced->cpumask = cpumask_of(0); > + clockevents_config_and_register(ced, ostm->rate, 0xf, 0xffffffff); > + > + return 0; > +} > + > +static int __init ostm_probe(struct platform_device *pdev) > +{ > + struct ostm_device *ostm; > + struct resource *res; > + int ret = -EFAULT; > + > + if (!is_early_platform_device(pdev)) { > + pm_runtime_set_active(&pdev->dev); > + pm_runtime_enable(&pdev->dev); > + } Can you clarify why the 'early' is needed here ? I don't see the pm_runtime_get/pm_runtime_put in the corresponding function. What is the benefit of using pm_runtime in this driver ? Isn't the timer supposed to be in an always-on power domain ? > + ostm = platform_get_drvdata(pdev); > + if (ostm) { > + dev_info(&pdev->dev, "kept as earlytimer\n"); > + ret = 0; > + goto out; > + } > + > + ostm = kzalloc(sizeof(*ostm), GFP_KERNEL); devm_kzalloc ? > + if (!ostm) { > + dev_err(&pdev->dev, "failed to allocate memory\n"); A memory failure allocation always triggers a dumpstack (IIRC), so this error message is pointless. > + return -ENOMEM; > + } > + > + ostm->pdev = pdev; > + platform_set_drvdata(ostm->pdev, ostm); > + > + res = platform_get_resource(ostm->pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_err(&ostm->pdev->dev, "failed to get I/O memory\n"); > + goto err; > + } > + > + ostm->base = ioremap_nocache(res->start, resource_size(res)); devm_ioremap_nocache ? > + if (!ostm->base) { > + dev_err(&ostm->pdev->dev, "failed to remap I/O memory\n"); > + goto err; > + } > + > + ostm->irq = platform_get_irq(ostm->pdev, 0); > + if (ostm->irq < 0) { > + dev_err(&ostm->pdev->dev, "failed to get irq\n"); > + goto err; > + } > + > + ostm->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(ostm->clk)) { > + dev_err(&ostm->pdev->dev, "failed to get clock\n"); > + ostm->clk = NULL; > + goto err; > + } > + > + ret = clk_prepare_enable(ostm->clk); > + if (ret) { > + dev_err(&ostm->pdev->dev, "failed to enable clock\n"); > + goto err; > + } > + > + ostm->rate = clk_get_rate(ostm->clk); > + ostm->ticks_per_jiffy = (ostm->rate + HZ / 2) / HZ; > + > + /* First probed device will be used as system clocksource */ > + if (!system_clock) { > + /* use as clocksource */ > + ret = ostm_init_clksrc(ostm); > + > + /* use as system scheduling clock */ > + if (!ret) > + ret = ostm_init_sched_clock(ostm); > + > + if (ret) { > + dev_err(&pdev->dev, "failed to use as sched_clock\n"); > + system_clock = (void *)-1; /* prevent future attempts */ > + ret = 0; /* still works as clocksource */ > + } This error code check is unnecessary complex. ostm_init_sched_clock always return zero. if (!system_clock) { system_clock = ostm_init_sched_clock(ostm); ret = ostm_init_clksrc(ostm) if (ret) dev_err("Failed to initialize the clocksource\n"); } else { ... } This is not perfect but until I send the clksrc_of/clkevt_of split changes, we have to deal with these kind of hacks. > + if (!ret) > + dev_info(&pdev->dev, "used for clocksource\n"); > + } else { > + /* use as clock event */ > + ret = ostm_init_clkevt(ostm); > + > + if (!ret) > + dev_info(&pdev->dev, "used for clock events\n"); > + } > + > +err: > + if (ret) { > + if (ostm->clk) > + clk_disable_unprepare(ostm->clk); > + if (ostm->base) > + iounmap(ostm->base); > + kfree(ostm); > + platform_set_drvdata(pdev, NULL); As iomap, kzalloc were done with the devm, then the rollback will be handled with the device framework. > + pm_runtime_idle(&pdev->dev); > + return ret; > + } > + > + if (is_early_platform_device(pdev)) > + return ret; > + > +out: > + pm_runtime_irq_safe(&pdev->dev); > + > + return ret; > +} > + > +static int ostm_remove(struct platform_device *pdev) > +{ > + return -EBUSY; /* cannot unregister clockevent */ > +} > + > +static const struct of_device_id ostm_of_table[] __maybe_unused = { > + { .compatible = "renesas,ostm" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ostm_of_table); > + > +static struct platform_driver ostm_timer = { > + .probe = ostm_probe, > + .remove = ostm_remove, > + .driver = { > + .name = "ostm", > + .of_match_table = of_match_ptr(ostm_of_table), > + }, > +}; > + > +static int __init ostm_init(void) > +{ > + return platform_driver_register(&ostm_timer); > +} > + > +static void __exit ostm_exit(void) > +{ > + platform_driver_unregister(&ostm_timer); > +} > + > +early_platform_init("earlytimer", &ostm_timer); > +subsys_initcall(ostm_init); > +module_exit(ostm_exit); > + > +MODULE_AUTHOR("Chris Brandt"); > +MODULE_DESCRIPTION("Renesas OSTM Timer Driver"); > +MODULE_LICENSE("GPL v2"); Maybe you can try with builtin_platform ? -- 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
