Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1589230 > unrolled thread

[PATCH v3 04/25] clocksource: Add Owl timer

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 04/25] clocksource: Add Owl timer Andreas Färber <afaerber@suse.de> - 2017-02-28 08:00 +0100
    Re: [PATCH v3 04/25] clocksource: Add Owl timer Andreas Färber <afaerber@suse.de> - 2017-02-28 18:10 +0100
      Re: [PATCH v3 04/25] clocksource: Add Owl timer Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-02-28 18:50 +0100
        Re: [PATCH v3 04/25] clocksource: Add Owl timer Andreas Färber <afaerber@suse.de> - 2017-02-28 19:20 +0100
          Re: [PATCH v3 04/25] clocksource: Add Owl timer Thomas Gleixner <tglx@linutronix.de> - 2017-02-28 21:40 +0100
        Re: [PATCH v3 04/25] clocksource: Add Owl timer Thomas Gleixner <tglx@linutronix.de> - 2017-02-28 20:40 +0100
    Re: [PATCH v3 04/25] clocksource: Add Owl timer Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-02-28 20:00 +0100

#1589230 — [PATCH v3 04/25] clocksource: Add Owl timer

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;
+
+	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);
-- 
2.10.2

[toc] | [next] | [standalone]

#1589631

Am 28.02.2017 um 17:47 schrieb Daniel Lezcano:
> On Tue, Feb 28, 2017 at 07:35:14AM +0100, Andreas Färber wrote:
>> The Actions Semi S500 SoC provides four timers, 2Hz0/1 and 32-bit TIMER0/1.
>>
>> Use TIMER0 as clocksource and TIMER1 as clockevents.
>>
>> Based on LeMaker linux-actions tree.
>>
>> An S500 datasheet can be found on the LeMaker Guitar pages:
>> http://www.lemaker.org/product-guitar-download-29.html
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  v2 -> v3:
>>  * Cleared interrupt pending flag for Timer1
>>  * Adopted named interrupts for Timer1
>>  * Extended commit message (Daniel)
>>  * Adopted BIT() macros (Daniel)
>>  * Adopted PTR_ERR() (Daniel)
>>  * Adopted request_irq() (Daniel)
>>  * Factored timer reset out (Daniel)
>>  * Adopted CLOCK_EVT_FEAT_DYNIRQ (Daniel)
>>  * Adopted clk input for rate (Daniel)
>>  * Prepared for S900, adopting S500 DT compatible
>>  
>>  v2: new
>>  
>>  drivers/clocksource/Kconfig     |   7 ++
>>  drivers/clocksource/Makefile    |   1 +
>>  drivers/clocksource/owl-timer.c | 193 ++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 201 insertions(+)
>>  create mode 100644 drivers/clocksource/owl-timer.c
>>
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index 3356ab8..2551365 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -107,6 +107,13 @@ config ORION_TIMER
>>  	help
>>  	  Enables the support for the Orion timer driver
>>  
>> +config OWL_TIMER
>> +	bool "Owl timer driver" if COMPILE_TEST
>> +	depends on GENERIC_CLOCKEVENTS
>> +	select CLKSRC_MMIO
>> +	help
>> +	  Enables the support for the Actions Semi Owl timer driver.
>> +
>>  config SUN4I_TIMER
>>  	bool "Sun4i timer driver" if COMPILE_TEST
>>  	depends on GENERIC_CLOCKEVENTS
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> index d227d13..801b65a 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -53,6 +53,7 @@ obj-$(CONFIG_CLKSRC_PISTACHIO)	+= time-pistachio.o
>>  obj-$(CONFIG_CLKSRC_TI_32K)	+= timer-ti-32k.o
>>  obj-$(CONFIG_CLKSRC_NPS)	+= timer-nps.o
>>  obj-$(CONFIG_OXNAS_RPS_TIMER)	+= timer-oxnas-rps.o
>> +obj-$(CONFIG_OWL_TIMER)		+= owl-timer.o
>>  
>>  obj-$(CONFIG_ARC_TIMERS)		+= arc_timer.o
>>  obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
>> diff --git a/drivers/clocksource/owl-timer.c b/drivers/clocksource/owl-timer.c
>> new file mode 100644
>> index 0000000..1b1e26d
>> --- /dev/null
>> +++ b/drivers/clocksource/owl-timer.c
>> @@ -0,0 +1,193 @@
>> +/*
>> + * Actions Semi Owl timer
>> + *
>> + * Copyright 2012 Actions Semi Inc.
>> + * Author: Actions Semi, Inc.
>> + *
>> + * Copyright (c) 2017 SUSE Linux GmbH
>> + * Author: Andreas Färber
>> + *
>> + * This program is free software; you can redistribute  it and/or modify it
>> + * under  the terms of  the GNU General  Public License as published by the
>> + * Free Software Foundation;  either version 2 of the  License, or (at your
>> + * option) any later version.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqreturn.h>
>> +#include <linux/sched_clock.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +
>> +#define OWL_Tx_CTL		0x0
>> +#define OWL_Tx_CMP		0x4
>> +#define OWL_Tx_VAL		0x8
>> +
>> +#define OWL_Tx_CTL_PD		BIT(0)
>> +#define OWL_Tx_CTL_INTEN	BIT(1)
>> +#define OWL_Tx_CTL_EN		BIT(2)
>> +
>> +#define OWL_MAX_Tx 2
>> +
>> +struct owl_timer_info {
>> +	int timer_offset[OWL_MAX_Tx];
>> +};
>> +
>> +static const struct owl_timer_info *owl_timer_info;
>> +
>> +static void __iomem *owl_timer_base;
>> +
>> +static inline void __iomem *owl_timer_get_base(unsigned timer_nr)
>> +{
>> +	if (timer_nr >= OWL_MAX_Tx)
>> +		return NULL;
> 
> The driver is supposed to know what is doing. owl_timer_get_base won't be
> called with an invalid index. This test will have a cost as it is called from
> owl_timer_sched_read.

OK

>> +
>> +	return owl_timer_base + owl_timer_info->timer_offset[timer_nr];
> 
> Actually, you just need a clkevt base @ and a clocksource base @.
> 
> Instead of computing again and again the base, why not just precompute:
> 
> 	owl_clksrc_base = owl_timer_base + owl_timer_info->timer_offset[OWL_TIMER0]
> 	owl_clkevt_base = owl_timer_base + owl_timer_info->timer_offset[OWL_TIMER1]
> 
>   at init time.
> 
> And use these variables directly in the functions.

Either that, or revert to previous simpler behavior...

>> +}
>> +
>> +static inline void owl_timer_reset(unsigned index)
>> +{
>> +	void __iomem *base;
>> +
>> +	base = owl_timer_get_base(index);
>> +	if (!base)
>> +		return;
> 
> Same here, this test is pointless.

Seems like you didn't look at the following patch yet. It sets two S500
offsets as -1, i.e. non-existant, which then results in NULL here.

>> +	writel(0, base + OWL_Tx_CTL);
>> +	writel(0, base + OWL_Tx_VAL);
>> +	writel(0, base + OWL_Tx_CMP);
>> +}
> 
> I suggest:
> 
> static inline void owl_timer_reset(void __iomem *addr)
> {
> 	writel(0, addr + OWL_Tx_CTL);
> 	writel(0, addr + OWL_Tx_VAL);
> 	writel(0, addr + OWL_Tx_CMP);
> }

OK

>> +
>> +static u64 notrace owl_timer_sched_read(void)
>> +{
>> +	return (u64)readl(owl_timer_get_base(0) + OWL_Tx_VAL);
> 
> 	return (u64)readl(owl_clksrc_base + OWL_Tx_VAL);
> 
>> +}
>> +
> 
> static inline int owl_timer_set_state_disable(struct clock_event_device *evt)
> {
> 	return writel(0, owl_clkevt_base + OWL_Tx_CTL);
> }

That I don't like. Disabling is just setting a bit. We save a readl by
just writing where we know it's safe. An API like this is not safe.

>> +static int owl_timer_set_state_shutdown(struct clock_event_device *evt)
>> +{
>> +	writel(0, owl_timer_get_base(0) + OWL_Tx_CTL);
> 
> 	return owl_timer_set_state_disable(evt);
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int owl_timer_set_state_oneshot(struct clock_event_device *evt)
>> +{
>> +	owl_timer_reset(1);
> 
> Do you really need to do reset here ? Why not just owl_timer_set_state_disable(evt) ?

Matches downstream, will consider changing.

>> +	return 0;
>> +}
>> +
>> +static int owl_timer_tick_resume(struct clock_event_device *evt)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int owl_timer_set_next_event(unsigned long evt,
>> +				    struct clock_event_device *ev)
>> +{
>> +	void __iomem *base = owl_timer_get_base(1);
>> +
>> +	writel(0, base + OWL_Tx_CTL);
>> +
>> +	writel(0, base + OWL_Tx_VAL);
> 

Are you suggesting a while line here? The point was disable first, then
initialize (2x), then activate. Maybe add comments instead?

>> +	writel(evt, base + OWL_Tx_CMP);
>> +
>> +	writel(OWL_Tx_CTL_EN | OWL_Tx_CTL_INTEN, base + OWL_Tx_CTL);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct clock_event_device owl_clockevent = {
>> +	.name			= "owl_tick",
>> +	.rating			= 200,
>> +	.features		= CLOCK_EVT_FEAT_ONESHOT |
>> +				  CLOCK_EVT_FEAT_DYNIRQ,
>> +	.set_state_shutdown	= owl_timer_set_state_shutdown,
>> +	.set_state_oneshot	= owl_timer_set_state_oneshot,
>> +	.tick_resume		= owl_timer_tick_resume,
>> +	.set_next_event		= owl_timer_set_next_event,
>> +};
>> +
>> +static irqreturn_t owl_timer1_interrupt(int irq, void *dev_id)
>> +{
>> +	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
>> +
>> +	writel(OWL_Tx_CTL_PD, owl_timer_get_base(1) + OWL_Tx_CTL);
>> +
>> +	evt->event_handler(evt);

Is there any guideline as to whether to clear such flag before or after?

>> +
>> +	return IRQ_HANDLED;
>> +}

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1589654

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)

-- 

 <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

[toc] | [prev] | [next] | [standalone]

#1589666

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

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1589756

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1589720

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1589706

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
> 

-- 

 <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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web