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

[Thomas Gleixner]

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

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

[Thomas Gleixner]

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

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

[Thomas Gleixner]

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

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

[Thomas Gleixner]

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

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

[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 
> ---
>  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 000..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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define OWL_Tx_CTL   0x0
> +#define OWL_Tx_CMP   0x4
> +#define OWL_Tx_VAL   0x8
> +
> +#define OWL_Tx_CTL_PDBIT(0)
> +#define OWL_Tx_CTL_INTEN BIT(1)
> +#define OWL_Tx_CTL_ENBIT(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 

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

[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 
> ---
>  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 000..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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define OWL_Tx_CTL   0x0
> +#define OWL_Tx_CMP   0x4
> +#define OWL_Tx_VAL   0x8
> +
> +#define OWL_Tx_CTL_PDBIT(0)
> +#define OWL_Tx_CTL_INTEN BIT(1)
> +#define OWL_Tx_CTL_ENBIT(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) + 

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

[Andreas Färber]

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)

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

[Andreas Färber]

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)

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

[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, 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)

-- 

  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

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

[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, 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)

-- 

  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

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

[Andreas Färber]

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 
>> ---
>>  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 000..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 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#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_INTENBIT(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, 

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

[Andreas Färber]

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 
>> ---
>>  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 000..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 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#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_INTENBIT(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);

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

[Andreas Färber]

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 
---
 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 000..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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#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,
+   

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

[Andreas Färber]

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 
---
 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 000..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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#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 =