diff mbox series

[12/16] clocksource: Add clock driver for RDA8810PL SoC

Message ID 20181119170939.19153-13-manivannan.sadhasivam@linaro.org (mailing list archive)
State Superseded
Headers show
Series Add initial RDA8810PL SoC and Orange Pi boards support | expand

Commit Message

Manivannan Sadhasivam Nov. 19, 2018, 5:09 p.m. UTC 
Add clock driver for RDA Micro RDA8810PL SoC supporting OSTIMER
and HWTIMER.

Signed-off-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm/mach-rda/Kconfig       |   1 +
 drivers/clocksource/Kconfig     |   7 ++
 drivers/clocksource/Makefile    |   1 +
 drivers/clocksource/timer-rda.c | 187 ++++++++++++++++++++++++++++++++
 4 files changed, 196 insertions(+)
 create mode 100644 drivers/clocksource/timer-rda.c

Comments

Marc Zyngier Nov. 19, 2018, 5:57 p.m. UTC | #1 
On 19/11/2018 17:09, Manivannan Sadhasivam wrote:
> Add clock driver for RDA Micro RDA8810PL SoC supporting OSTIMER
> and HWTIMER.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  arch/arm/mach-rda/Kconfig       |   1 +
>  drivers/clocksource/Kconfig     |   7 ++
>  drivers/clocksource/Makefile    |   1 +
>  drivers/clocksource/timer-rda.c | 187 ++++++++++++++++++++++++++++++++
>  4 files changed, 196 insertions(+)
>  create mode 100644 drivers/clocksource/timer-rda.c
> 
> diff --git a/arch/arm/mach-rda/Kconfig b/arch/arm/mach-rda/Kconfig
> index 29012bc68ca4..1ea753f57b2d 100644
> --- a/arch/arm/mach-rda/Kconfig
> +++ b/arch/arm/mach-rda/Kconfig
> @@ -4,5 +4,6 @@ menuconfig ARCH_RDA
>  	select COMMON_CLK
>  	select GENERIC_IRQ_CHIP
>  	select RDA_INTC
> +	select RDA_TIMER
>  	help
>  	  This enables support for the RDA Micro 8810PL SoC family.
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 55c77e44bb2d..f51eee3a72ea 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -105,6 +105,13 @@ config OWL_TIMER
>  	help
>  	  Enables the support for the Actions Semi Owl timer driver.
>  
> +config RDA_TIMER
> +	bool "RDA timer driver" if COMPILE_TEST
> +	depends on GENERIC_CLOCKEVENTS
> +	select CLKSRC_MMIO
> +	help
> +	  Enables the support for the RDA Micro timer driver.
> +
>  config SUN4I_TIMER
>  	bool "Sun4i timer driver" if COMPILE_TEST
>  	depends on HAS_IOMEM
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index dd9138104568..150020a90707 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -57,6 +57,7 @@ obj-$(CONFIG_OXNAS_RPS_TIMER)	+= timer-oxnas-rps.o
>  obj-$(CONFIG_OWL_TIMER)		+= timer-owl.o
>  obj-$(CONFIG_SPRD_TIMER)	+= timer-sprd.o
>  obj-$(CONFIG_NPCM7XX_TIMER)	+= timer-npcm7xx.o
> +obj-$(CONFIG_RDA_TIMER)		+= timer-rda.o
>  
>  obj-$(CONFIG_ARC_TIMERS)		+= arc_timer.o
>  obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
> diff --git a/drivers/clocksource/timer-rda.c b/drivers/clocksource/timer-rda.c
> new file mode 100644
> index 000000000000..3aa684d92c5d
> --- /dev/null
> +++ b/drivers/clocksource/timer-rda.c
> @@ -0,0 +1,187 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * RDA8810PL SoC timer driver
> + *
> + * Copyright RDA Microelectronics Company Limited
> + * Copyright (c) 2017 Andreas Färber
> + * Copyright (c) 2018 Manivannan Sadhasivam
> + */
> +
> +#include <linux/clockchips.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqreturn.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +#define RDA_OSTIMER_LOADVAL_L	0x000
> +#define RDA_OSTIMER_CTRL	0x004
> +#define RDA_HWTIMER_LOCKVAL_L	0x024
> +#define RDA_HWTIMER_LOCKVAL_H	0x028
> +#define RDA_TIMER_IRQ_MASK_SET	0x02c
> +#define RDA_TIMER_IRQ_CLR	0x034
> +
> +#define RDA_OSTIMER_CTRL_ENABLE		BIT(24)
> +#define RDA_OSTIMER_CTRL_REPEAT		BIT(28)
> +#define RDA_OSTIMER_CTRL_LOAD		BIT(30)
> +
> +#define RDA_TIMER_IRQ_MASK_SET_OSTIMER	BIT(0)
> +
> +#define RDA_TIMER_IRQ_CLR_OSTIMER	BIT(0)
> +
> +static void __iomem *rda_timer_base;
> +
> +static u64 rda_hwtimer_read(struct clocksource *cs)
> +{
> +	u32 lo, hi;
> +
> +	/* Always read low 32 bits first */
> +	lo = readl(rda_timer_base + RDA_HWTIMER_LOCKVAL_L);
> +	hi = readl(rda_timer_base + RDA_HWTIMER_LOCKVAL_H);

Please use the relaxed accessors throughout this driver. There is zero
reason to use the non-relaxed versions here.

Now, I'm pretty sure this thing isn't correct.

	<timer = 0x00000000ffffffff>
	lo = 0xffffffff;
	<tick, timer = 0x0000000100000000>
	hi = 0x00000001;

Bingo. You cannot read a 64bit counter with only two 32bit accesses.

> +
> +	return ((u64)hi << 32) | lo;
> +}
> +
> +static struct clocksource rda_clocksource = {
> +	.name           = "rda-timer",
> +	.rating         = 400,
> +	.read           = rda_hwtimer_read,
> +	.mask           = CLOCKSOURCE_MASK(64),

This is a 64bit counter? See below.

> +	.flags          = CLOCK_SOURCE_IS_CONTINUOUS,
> +};
> +
> +static int rda_ostimer_start(bool periodic, u64 cycles)
> +{
> +	u32 ctrl, load_l;
> +
> +	load_l = (u32)cycles;
> +	ctrl = ((cycles >> 32) & 0xffffff);
> +	ctrl |= RDA_OSTIMER_CTRL_LOAD | RDA_OSTIMER_CTRL_ENABLE;
> +	if (periodic)
> +		ctrl |= RDA_OSTIMER_CTRL_REPEAT;
> +
> +	/* Enable ostimer interrupt first */
> +	writel(RDA_TIMER_IRQ_MASK_SET_OSTIMER,
> +	       rda_timer_base + RDA_TIMER_IRQ_MASK_SET);

Is it masking or enabling the interrupt?

> +
> +	/* Write low 32 bits first, high 24 bits are with ctrl */

You're saying that you can only write 56 bits? This contradicts the 64bt
counter thing above.

> +	writel(load_l, rda_timer_base + RDA_OSTIMER_LOADVAL_L);
> +	writel(ctrl, rda_timer_base + RDA_OSTIMER_CTRL);
> +
> +	return 0;
> +}
> +
> +static int rda_ostimer_stop(void)
> +{
> +	/* Disable ostimer interrupt first */
> +	writel(0, rda_timer_base + RDA_TIMER_IRQ_MASK_SET);
> +
> +	writel(0, rda_timer_base + RDA_OSTIMER_CTRL);
> +
> +	return 0;
> +}
> +
> +static int rda_ostimer_set_state_shutdown(struct clock_event_device *evt)
> +{
> +	rda_ostimer_stop();
> +
> +	return 0;
> +}
> +
> +static int rda_ostimer_set_state_oneshot(struct clock_event_device *evt)
> +{
> +	rda_ostimer_stop();
> +
> +	return 0;
> +}
> +
> +static int rda_ostimer_set_state_periodic(struct clock_event_device *evt)
> +{
> +	unsigned long cycles_per_jiffy;
> +
> +	rda_ostimer_stop();
> +
> +	cycles_per_jiffy = ((unsigned long long)NSEC_PER_SEC / HZ *
> +			     evt->mult) >> evt->shift;
> +	rda_ostimer_start(true, cycles_per_jiffy);
> +
> +	return 0;
> +}
> +
> +static int rda_ostimer_tick_resume(struct clock_event_device *evt)
> +{
> +	return 0;
> +}
> +
> +static int rda_ostimer_set_next_event(unsigned long evt,
> +				      struct clock_event_device *ev)
> +{
> +	rda_ostimer_start(false, evt);
> +
> +	return 0;
> +}
> +
> +static struct clock_event_device rda_clockevent = {
> +	.name			= "rda-ostimer",
> +	.rating			= 250,
> +	.features		= CLOCK_EVT_FEAT_PERIODIC |
> +				  CLOCK_EVT_FEAT_ONESHOT |
> +				  CLOCK_EVT_FEAT_DYNIRQ,
> +	.set_state_shutdown	= rda_ostimer_set_state_shutdown,
> +	.set_state_oneshot	= rda_ostimer_set_state_oneshot,
> +	.set_state_periodic	= rda_ostimer_set_state_periodic,
> +	.tick_resume		= rda_ostimer_tick_resume,
> +	.set_next_event		= rda_ostimer_set_next_event,
> +};
> +
> +static irqreturn_t rda_ostimer_interrupt(int irq, void *dev_id)
> +{
> +	struct clock_event_device *evt = dev_id;
> +
> +	/* clear timer int */
> +	writel(RDA_TIMER_IRQ_CLR_OSTIMER, rda_timer_base + RDA_TIMER_IRQ_CLR);
> +
> +	if (evt->event_handler)
> +		evt->event_handler(evt);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int __init rda_timer_init(struct device_node *node)
> +{
> +	unsigned long rate = 2000000;
> +	int ostimer_irq, ret;
> +
> +	rda_timer_base = of_io_request_and_map(node, 0, "rda-timer");
> +	if (IS_ERR(rda_timer_base)) {
> +		pr_err("Can't map timer registers");
> +		return PTR_ERR(rda_timer_base);
> +	}
> +
> +	ostimer_irq = of_irq_get_byname(node, "ostimer");
> +	if (ostimer_irq <= 0) {
> +		pr_err("Can't parse ostimer IRQ");
> +		return -EINVAL;

Leaking IO space.

> +	}
> +
> +	clocksource_register_hz(&rda_clocksource, rate);
> +
> +	ret = request_irq(ostimer_irq, rda_ostimer_interrupt, IRQF_TIMER,
> +			  "rda-ostimer", &rda_clockevent);
> +	if (ret) {
> +		pr_err("failed to request irq %d\n", ostimer_irq);
> +		return ret;

Same here.

> +	}
> +
> +	irq_force_affinity(ostimer_irq, cpumask_of(0));

Why?

> +
> +	rda_clockevent.cpumask = cpumask_of(0);
> +	rda_clockevent.irq = ostimer_irq;
> +	clockevents_config_and_register(&rda_clockevent, rate,
> +					0x2, 0xffffffff);
> +
> +	return 0;
> +}
> +
> +TIMER_OF_DECLARE(rda8810pl, "rda,8810pl-timer", rda_timer_init);
> 

Thanks,

	M.
Manivannan Sadhasivam Nov. 20, 2018, 5:06 a.m. UTC | #2 
Hi Marc,

On Mon, Nov 19, 2018 at 05:57:12PM +0000, Marc Zyngier wrote:
> On 19/11/2018 17:09, Manivannan Sadhasivam wrote:
> > Add clock driver for RDA Micro RDA8810PL SoC supporting OSTIMER
> > and HWTIMER.
> > 
> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  arch/arm/mach-rda/Kconfig       |   1 +
> >  drivers/clocksource/Kconfig     |   7 ++
> >  drivers/clocksource/Makefile    |   1 +
> >  drivers/clocksource/timer-rda.c | 187 ++++++++++++++++++++++++++++++++
> >  4 files changed, 196 insertions(+)
> >  create mode 100644 drivers/clocksource/timer-rda.c
> > 
> > diff --git a/arch/arm/mach-rda/Kconfig b/arch/arm/mach-rda/Kconfig
> > index 29012bc68ca4..1ea753f57b2d 100644
> > --- a/arch/arm/mach-rda/Kconfig
> > +++ b/arch/arm/mach-rda/Kconfig
> > @@ -4,5 +4,6 @@ menuconfig ARCH_RDA
> >  	select COMMON_CLK
> >  	select GENERIC_IRQ_CHIP
> >  	select RDA_INTC
> > +	select RDA_TIMER
> >  	help
> >  	  This enables support for the RDA Micro 8810PL SoC family.
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index 55c77e44bb2d..f51eee3a72ea 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -105,6 +105,13 @@ config OWL_TIMER
> >  	help
> >  	  Enables the support for the Actions Semi Owl timer driver.
> >  
> > +config RDA_TIMER
> > +	bool "RDA timer driver" if COMPILE_TEST
> > +	depends on GENERIC_CLOCKEVENTS
> > +	select CLKSRC_MMIO
> > +	help
> > +	  Enables the support for the RDA Micro timer driver.
> > +
> >  config SUN4I_TIMER
> >  	bool "Sun4i timer driver" if COMPILE_TEST
> >  	depends on HAS_IOMEM
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index dd9138104568..150020a90707 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -57,6 +57,7 @@ obj-$(CONFIG_OXNAS_RPS_TIMER)	+= timer-oxnas-rps.o
> >  obj-$(CONFIG_OWL_TIMER)		+= timer-owl.o
> >  obj-$(CONFIG_SPRD_TIMER)	+= timer-sprd.o
> >  obj-$(CONFIG_NPCM7XX_TIMER)	+= timer-npcm7xx.o
> > +obj-$(CONFIG_RDA_TIMER)		+= timer-rda.o
> >  
> >  obj-$(CONFIG_ARC_TIMERS)		+= arc_timer.o
> >  obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
> > diff --git a/drivers/clocksource/timer-rda.c b/drivers/clocksource/timer-rda.c
> > new file mode 100644
> > index 000000000000..3aa684d92c5d
> > --- /dev/null
> > +++ b/drivers/clocksource/timer-rda.c
> > @@ -0,0 +1,187 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * RDA8810PL SoC timer driver
> > + *
> > + * Copyright RDA Microelectronics Company Limited
> > + * Copyright (c) 2017 Andreas Färber
> > + * Copyright (c) 2018 Manivannan Sadhasivam
> > + */
> > +
> > +#include <linux/clockchips.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqreturn.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +
> > +#define RDA_OSTIMER_LOADVAL_L	0x000
> > +#define RDA_OSTIMER_CTRL	0x004
> > +#define RDA_HWTIMER_LOCKVAL_L	0x024
> > +#define RDA_HWTIMER_LOCKVAL_H	0x028
> > +#define RDA_TIMER_IRQ_MASK_SET	0x02c
> > +#define RDA_TIMER_IRQ_CLR	0x034
> > +
> > +#define RDA_OSTIMER_CTRL_ENABLE		BIT(24)
> > +#define RDA_OSTIMER_CTRL_REPEAT		BIT(28)
> > +#define RDA_OSTIMER_CTRL_LOAD		BIT(30)
> > +
> > +#define RDA_TIMER_IRQ_MASK_SET_OSTIMER	BIT(0)
> > +
> > +#define RDA_TIMER_IRQ_CLR_OSTIMER	BIT(0)
> > +
> > +static void __iomem *rda_timer_base;
> > +
> > +static u64 rda_hwtimer_read(struct clocksource *cs)
> > +{
> > +	u32 lo, hi;
> > +
> > +	/* Always read low 32 bits first */
> > +	lo = readl(rda_timer_base + RDA_HWTIMER_LOCKVAL_L);
> > +	hi = readl(rda_timer_base + RDA_HWTIMER_LOCKVAL_H);
> 
> Please use the relaxed accessors throughout this driver. There is zero
> reason to use the non-relaxed versions here.
> 

Okay.

> Now, I'm pretty sure this thing isn't correct.
> 
> 	<timer = 0x00000000ffffffff>
> 	lo = 0xffffffff;
> 	<tick, timer = 0x0000000100000000>
> 	hi = 0x00000001;
> 
> Bingo. You cannot read a 64bit counter with only two 32bit accesses.
> 

I think the lack of description makes confusion here. In this SoC, there
are two independent timers available: OSTIMER (56 bit) and HWTIMER (64 bit)
with optional interrupt support. I have used OSTIMER for clockevents and
HWTIMER for clocksource. Will add this information in driver.

Please let me know whether I have to model these two clocksources
differently!

> > +
> > +	return ((u64)hi << 32) | lo;
> > +}
> > +
> > +static struct clocksource rda_clocksource = {
> > +	.name           = "rda-timer",
> > +	.rating         = 400,
> > +	.read           = rda_hwtimer_read,
> > +	.mask           = CLOCKSOURCE_MASK(64),
> 
> This is a 64bit counter? See below.
> 

Yes, this is the HWTIMER and is 64 bit.

> > +	.flags          = CLOCK_SOURCE_IS_CONTINUOUS,
> > +};
> > +
> > +static int rda_ostimer_start(bool periodic, u64 cycles)
> > +{
> > +	u32 ctrl, load_l;
> > +
> > +	load_l = (u32)cycles;
> > +	ctrl = ((cycles >> 32) & 0xffffff);
> > +	ctrl |= RDA_OSTIMER_CTRL_LOAD | RDA_OSTIMER_CTRL_ENABLE;
> > +	if (periodic)
> > +		ctrl |= RDA_OSTIMER_CTRL_REPEAT;
> > +
> > +	/* Enable ostimer interrupt first */
> > +	writel(RDA_TIMER_IRQ_MASK_SET_OSTIMER,
> > +	       rda_timer_base + RDA_TIMER_IRQ_MASK_SET);
> 
> Is it masking or enabling the interrupt?
> 

On this platform, we need to set corresponding bit in the RDA_TIMER_IRQ_MASK_SET
register to enable an interrupt.

> > +
> > +	/* Write low 32 bits first, high 24 bits are with ctrl */
> 
> You're saying that you can only write 56 bits? This contradicts the 64bt
> counter thing above.
> 
> > +	writel(load_l, rda_timer_base + RDA_OSTIMER_LOADVAL_L);
> > +	writel(ctrl, rda_timer_base + RDA_OSTIMER_CTRL);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rda_ostimer_stop(void)
> > +{
> > +	/* Disable ostimer interrupt first */
> > +	writel(0, rda_timer_base + RDA_TIMER_IRQ_MASK_SET);
> > +

Here the register should be RDA_TIMER_IRQ_MASK_CLR.

> > +	writel(0, rda_timer_base + RDA_OSTIMER_CTRL);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rda_ostimer_set_state_shutdown(struct clock_event_device *evt)
> > +{
> > +	rda_ostimer_stop();
> > +
> > +	return 0;
> > +}
> > +
> > +static int rda_ostimer_set_state_oneshot(struct clock_event_device *evt)
> > +{
> > +	rda_ostimer_stop();
> > +
> > +	return 0;
> > +}
> > +
> > +static int rda_ostimer_set_state_periodic(struct clock_event_device *evt)
> > +{
> > +	unsigned long cycles_per_jiffy;
> > +
> > +	rda_ostimer_stop();
> > +
> > +	cycles_per_jiffy = ((unsigned long long)NSEC_PER_SEC / HZ *
> > +			     evt->mult) >> evt->shift;
> > +	rda_ostimer_start(true, cycles_per_jiffy);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rda_ostimer_tick_resume(struct clock_event_device *evt)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int rda_ostimer_set_next_event(unsigned long evt,
> > +				      struct clock_event_device *ev)
> > +{
> > +	rda_ostimer_start(false, evt);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct clock_event_device rda_clockevent = {
> > +	.name			= "rda-ostimer",
> > +	.rating			= 250,
> > +	.features		= CLOCK_EVT_FEAT_PERIODIC |
> > +				  CLOCK_EVT_FEAT_ONESHOT |
> > +				  CLOCK_EVT_FEAT_DYNIRQ,
> > +	.set_state_shutdown	= rda_ostimer_set_state_shutdown,
> > +	.set_state_oneshot	= rda_ostimer_set_state_oneshot,
> > +	.set_state_periodic	= rda_ostimer_set_state_periodic,
> > +	.tick_resume		= rda_ostimer_tick_resume,
> > +	.set_next_event		= rda_ostimer_set_next_event,
> > +};
> > +
> > +static irqreturn_t rda_ostimer_interrupt(int irq, void *dev_id)
> > +{
> > +	struct clock_event_device *evt = dev_id;
> > +
> > +	/* clear timer int */
> > +	writel(RDA_TIMER_IRQ_CLR_OSTIMER, rda_timer_base + RDA_TIMER_IRQ_CLR);
> > +
> > +	if (evt->event_handler)
> > +		evt->event_handler(evt);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int __init rda_timer_init(struct device_node *node)
> > +{
> > +	unsigned long rate = 2000000;
> > +	int ostimer_irq, ret;
> > +
> > +	rda_timer_base = of_io_request_and_map(node, 0, "rda-timer");
> > +	if (IS_ERR(rda_timer_base)) {
> > +		pr_err("Can't map timer registers");
> > +		return PTR_ERR(rda_timer_base);
> > +	}
> > +
> > +	ostimer_irq = of_irq_get_byname(node, "ostimer");
> > +	if (ostimer_irq <= 0) {
> > +		pr_err("Can't parse ostimer IRQ");
> > +		return -EINVAL;
> 
> Leaking IO space.
> 

Ack.

> > +	}
> > +
> > +	clocksource_register_hz(&rda_clocksource, rate);
> > +
> > +	ret = request_irq(ostimer_irq, rda_ostimer_interrupt, IRQF_TIMER,
> > +			  "rda-ostimer", &rda_clockevent);
> > +	if (ret) {
> > +		pr_err("failed to request irq %d\n", ostimer_irq);
> > +		return ret;
> 
> Same here.
> 

Ack.

> > +	}
> > +
> > +	irq_force_affinity(ostimer_irq, cpumask_of(0));
> 
> Why?
> 

Not needed, will remove it.

Thanks,
Mani

> > +
> > +	rda_clockevent.cpumask = cpumask_of(0);
> > +	rda_clockevent.irq = ostimer_irq;
> > +	clockevents_config_and_register(&rda_clockevent, rate,
> > +					0x2, 0xffffffff);
> > +
> > +	return 0;
> > +}
> > +
> > +TIMER_OF_DECLARE(rda8810pl, "rda,8810pl-timer", rda_timer_init);
> > 
> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...
Marc Zyngier Nov. 20, 2018, 8:16 a.m. UTC | #3 
On Tue, 20 Nov 2018 05:06:50 +0000,
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
> 
> Hi Marc,
> 
> On Mon, Nov 19, 2018 at 05:57:12PM +0000, Marc Zyngier wrote:
> > On 19/11/2018 17:09, Manivannan Sadhasivam wrote:
> > > Add clock driver for RDA Micro RDA8810PL SoC supporting OSTIMER
> > > and HWTIMER.
> > > 
> > > Signed-off-by: Andreas Färber <afaerber@suse.de>
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  arch/arm/mach-rda/Kconfig       |   1 +
> > >  drivers/clocksource/Kconfig     |   7 ++
> > >  drivers/clocksource/Makefile    |   1 +
> > >  drivers/clocksource/timer-rda.c | 187 ++++++++++++++++++++++++++++++++
> > >  4 files changed, 196 insertions(+)
> > >  create mode 100644 drivers/clocksource/timer-rda.c
> > > 
> > > diff --git a/arch/arm/mach-rda/Kconfig b/arch/arm/mach-rda/Kconfig
> > > index 29012bc68ca4..1ea753f57b2d 100644
> > > --- a/arch/arm/mach-rda/Kconfig
> > > +++ b/arch/arm/mach-rda/Kconfig
> > > @@ -4,5 +4,6 @@ menuconfig ARCH_RDA
> > >  	select COMMON_CLK
> > >  	select GENERIC_IRQ_CHIP
> > >  	select RDA_INTC
> > > +	select RDA_TIMER
> > >  	help
> > >  	  This enables support for the RDA Micro 8810PL SoC family.
> > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > > index 55c77e44bb2d..f51eee3a72ea 100644
> > > --- a/drivers/clocksource/Kconfig
> > > +++ b/drivers/clocksource/Kconfig
> > > @@ -105,6 +105,13 @@ config OWL_TIMER
> > >  	help
> > >  	  Enables the support for the Actions Semi Owl timer driver.
> > >  
> > > +config RDA_TIMER
> > > +	bool "RDA timer driver" if COMPILE_TEST
> > > +	depends on GENERIC_CLOCKEVENTS
> > > +	select CLKSRC_MMIO
> > > +	help
> > > +	  Enables the support for the RDA Micro timer driver.
> > > +
> > >  config SUN4I_TIMER
> > >  	bool "Sun4i timer driver" if COMPILE_TEST
> > >  	depends on HAS_IOMEM
> > > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > > index dd9138104568..150020a90707 100644
> > > --- a/drivers/clocksource/Makefile
> > > +++ b/drivers/clocksource/Makefile
> > > @@ -57,6 +57,7 @@ obj-$(CONFIG_OXNAS_RPS_TIMER)	+= timer-oxnas-rps.o
> > >  obj-$(CONFIG_OWL_TIMER)		+= timer-owl.o
> > >  obj-$(CONFIG_SPRD_TIMER)	+= timer-sprd.o
> > >  obj-$(CONFIG_NPCM7XX_TIMER)	+= timer-npcm7xx.o
> > > +obj-$(CONFIG_RDA_TIMER)		+= timer-rda.o
> > >  
> > >  obj-$(CONFIG_ARC_TIMERS)		+= arc_timer.o
> > >  obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
> > > diff --git a/drivers/clocksource/timer-rda.c b/drivers/clocksource/timer-rda.c
> > > new file mode 100644
> > > index 000000000000..3aa684d92c5d
> > > --- /dev/null
> > > +++ b/drivers/clocksource/timer-rda.c
> > > @@ -0,0 +1,187 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * RDA8810PL SoC timer driver
> > > + *
> > > + * Copyright RDA Microelectronics Company Limited
> > > + * Copyright (c) 2017 Andreas Färber
> > > + * Copyright (c) 2018 Manivannan Sadhasivam
> > > + */
> > > +
> > > +#include <linux/clockchips.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/irqreturn.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_irq.h>
> > > +
> > > +#define RDA_OSTIMER_LOADVAL_L	0x000
> > > +#define RDA_OSTIMER_CTRL	0x004
> > > +#define RDA_HWTIMER_LOCKVAL_L	0x024
> > > +#define RDA_HWTIMER_LOCKVAL_H	0x028
> > > +#define RDA_TIMER_IRQ_MASK_SET	0x02c
> > > +#define RDA_TIMER_IRQ_CLR	0x034
> > > +
> > > +#define RDA_OSTIMER_CTRL_ENABLE		BIT(24)
> > > +#define RDA_OSTIMER_CTRL_REPEAT		BIT(28)
> > > +#define RDA_OSTIMER_CTRL_LOAD		BIT(30)
> > > +
> > > +#define RDA_TIMER_IRQ_MASK_SET_OSTIMER	BIT(0)
> > > +
> > > +#define RDA_TIMER_IRQ_CLR_OSTIMER	BIT(0)
> > > +
> > > +static void __iomem *rda_timer_base;
> > > +
> > > +static u64 rda_hwtimer_read(struct clocksource *cs)
> > > +{
> > > +	u32 lo, hi;
> > > +
> > > +	/* Always read low 32 bits first */
> > > +	lo = readl(rda_timer_base + RDA_HWTIMER_LOCKVAL_L);
> > > +	hi = readl(rda_timer_base + RDA_HWTIMER_LOCKVAL_H);
> > 
> > Please use the relaxed accessors throughout this driver. There is zero
> > reason to use the non-relaxed versions here.
> > 
> 
> Okay.
> 
> > Now, I'm pretty sure this thing isn't correct.
> > 
> > 	<timer = 0x00000000ffffffff>
> > 	lo = 0xffffffff;
> > 	<tick, timer = 0x0000000100000000>
> > 	hi = 0x00000001;
> > 
> > Bingo. You cannot read a 64bit counter with only two 32bit accesses.
> > 
> 
> I think the lack of description makes confusion here. In this SoC, there
> are two independent timers available: OSTIMER (56 bit) and HWTIMER (64 bit)
> with optional interrupt support. I have used OSTIMER for clockevents and
> HWTIMER for clocksource. Will add this information in driver.

How does this change anything with the fact that the above code is
broken? 56 or 64 bit, you cannot read this counter with a single
access, or two. The canonical way of reading such a counter is
something like this:

	do {
		lo = readl_relaxed(LO);
		hi = readl_relaxed(HI);
	} while (hi != read_relaxed(HI));

And yes, please add some documentation, as I have no idea which one is
which. Having structure and function names that match the IP blocks
used would also help.

Thanks,

	M.
Linus Walleij Nov. 20, 2018, 8:56 a.m. UTC | #4 
On Tue, Nov 20, 2018 at 9:17 AM Marc Zyngier <marc.zyngier@arm.com> wrote:

> How does this change anything with the fact that the above code is
> broken? 56 or 64 bit, you cannot read this counter with a single
> access, or two. The canonical way of reading such a counter is
> something like this:
>
>         do {
>                 lo = readl_relaxed(LO);
>                 hi = readl_relaxed(HI);
>         } while (hi != read_relaxed(HI));

To be fair, I have seen hardware that employ a logic latch
such that when a read access is done to the LO register,
the value of the whole counter is latched, also for the HI
register, so when you read the HI register in the second
step, it is never subject to wrapping. (Conversely reading
the HI before the LO will always give you insane values
:D)

However the above code should be fine unless you know
for sure the hardware was constructed with a clever latch.

Yours,
Linus Walleij
Daniel Lezcano Nov. 20, 2018, 10:32 a.m. UTC | #5 
Hi Manivannan,


On 19/11/2018 18:09, Manivannan Sadhasivam wrote:
> Add clock driver for RDA Micro RDA8810PL SoC supporting OSTIMER
> and HWTIMER.

As it is a new driver, can you elaborate the log and describe the timer.

> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---

[ ... ]

> +static int __init rda_timer_init(struct device_node *node)
> +{
> +	unsigned long rate = 2000000;
> +	int ostimer_irq, ret;
> +
> +	rda_timer_base = of_io_request_and_map(node, 0, "rda-timer");
> +	if (IS_ERR(rda_timer_base)) {
> +		pr_err("Can't map timer registers");
> +		return PTR_ERR(rda_timer_base);
> +	}
> +
> +	ostimer_irq = of_irq_get_byname(node, "ostimer");
> +	if (ostimer_irq <= 0) {
> +		pr_err("Can't parse ostimer IRQ");
> +		return -EINVAL;
> +	}
> +
> +	clocksource_register_hz(&rda_clocksource, rate);
> +
> +	ret = request_irq(ostimer_irq, rda_ostimer_interrupt, IRQF_TIMER,
> +			  "rda-ostimer", &rda_clockevent);
> +	if (ret) {
> +		pr_err("failed to request irq %d\n", ostimer_irq);
> +		return ret;
> +	}
> +

Use the timer-of API.

> +
> +TIMER_OF_DECLARE(rda8810pl, "rda,8810pl-timer", rda_timer_init);


Thanks

  -- Daniel
Marc Zyngier Nov. 20, 2018, 11:05 a.m. UTC | #6 
On 20/11/2018 08:56, Linus Walleij wrote:
> On Tue, Nov 20, 2018 at 9:17 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
> 
>> How does this change anything with the fact that the above code is
>> broken? 56 or 64 bit, you cannot read this counter with a single
>> access, or two. The canonical way of reading such a counter is
>> something like this:
>>
>>         do {
>>                 lo = readl_relaxed(LO);
>>                 hi = readl_relaxed(HI);
>>         } while (hi != read_relaxed(HI));
> 
> To be fair, I have seen hardware that employ a logic latch
> such that when a read access is done to the LO register,
> the value of the whole counter is latched, also for the HI
> register, so when you read the HI register in the second
> step, it is never subject to wrapping. (Conversely reading
> the HI before the LO will always give you insane values
> :D)

I've seen such HW indeed, and I've also seen it being broken... ;-)

It this timer is built around such a (non-broken) logic, I'd really like
to see it spelled out. It will otherwise be a real pain to debug...

> However the above code should be fine unless you know
> for sure the hardware was constructed with a clever latch.

Let's find out!

Thanks,

	M.
Manivannan Sadhasivam Nov. 20, 2018, 12:09 p.m. UTC | #7 
On Tue, Nov 20, 2018 at 11:05:41AM +0000, Marc Zyngier wrote:
> On 20/11/2018 08:56, Linus Walleij wrote:
> > On Tue, Nov 20, 2018 at 9:17 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
> > 
> >> How does this change anything with the fact that the above code is
> >> broken? 56 or 64 bit, you cannot read this counter with a single
> >> access, or two. The canonical way of reading such a counter is
> >> something like this:
> >>
> >>         do {
> >>                 lo = readl_relaxed(LO);
> >>                 hi = readl_relaxed(HI);
> >>         } while (hi != read_relaxed(HI));
> > 
> > To be fair, I have seen hardware that employ a logic latch
> > such that when a read access is done to the LO register,
> > the value of the whole counter is latched, also for the HI
> > register, so when you read the HI register in the second
> > step, it is never subject to wrapping. (Conversely reading
> > the HI before the LO will always give you insane values
> > :D)
> 
> I've seen such HW indeed, and I've also seen it being broken... ;-)
> 
> It this timer is built around such a (non-broken) logic, I'd really like
> to see it spelled out. It will otherwise be a real pain to debug...
>

There is no information about HW latch in datasheet and vendor code. But the
vendor driver doesn't use any logic to prevent wrapping. However, this doesn't
mean that we can assume that the hardware is capable of preventing overrun.
So I guess it is best to go with Marc's suggestion here.

Thanks,
Mani

> > However the above code should be fine unless you know
> > for sure the hardware was constructed with a clever latch.
> 
> Let's find out!
> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...
Manivannan Sadhasivam Nov. 20, 2018, 12:11 p.m. UTC | #8 
Hi Daniel,

On Tue, Nov 20, 2018 at 11:32:52AM +0100, Daniel Lezcano wrote:
> 
> Hi Manivannan,
> 
> 
> On 19/11/2018 18:09, Manivannan Sadhasivam wrote:
> > Add clock driver for RDA Micro RDA8810PL SoC supporting OSTIMER
> > and HWTIMER.
> 
> As it is a new driver, can you elaborate the log and describe the timer.
>

Sure, will add the brief in commit description and also in driver.
 
> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> 
> [ ... ]
> 
> > +static int __init rda_timer_init(struct device_node *node)
> > +{
> > +	unsigned long rate = 2000000;
> > +	int ostimer_irq, ret;
> > +
> > +	rda_timer_base = of_io_request_and_map(node, 0, "rda-timer");
> > +	if (IS_ERR(rda_timer_base)) {
> > +		pr_err("Can't map timer registers");
> > +		return PTR_ERR(rda_timer_base);
> > +	}
> > +
> > +	ostimer_irq = of_irq_get_byname(node, "ostimer");
> > +	if (ostimer_irq <= 0) {
> > +		pr_err("Can't parse ostimer IRQ");
> > +		return -EINVAL;
> > +	}
> > +
> > +	clocksource_register_hz(&rda_clocksource, rate);
> > +
> > +	ret = request_irq(ostimer_irq, rda_ostimer_interrupt, IRQF_TIMER,
> > +			  "rda-ostimer", &rda_clockevent);
> > +	if (ret) {
> > +		pr_err("failed to request irq %d\n", ostimer_irq);
> > +		return ret;
> > +	}
> > +
> 
> Use the timer-of API.
> 

Okay, will use it for both IO and IRQ requests.

Thanks,
Mani

> > +
> > +TIMER_OF_DECLARE(rda8810pl, "rda,8810pl-timer", rda_timer_init);
> 
> 
> Thanks
> 
>   -- 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
>
diff mbox series

Patch

diff --git a/arch/arm/mach-rda/Kconfig b/arch/arm/mach-rda/Kconfig
index 29012bc68ca4..1ea753f57b2d 100644
--- a/arch/arm/mach-rda/Kconfig
+++ b/arch/arm/mach-rda/Kconfig
@@ -4,5 +4,6 @@  menuconfig ARCH_RDA
 	select COMMON_CLK
 	select GENERIC_IRQ_CHIP
 	select RDA_INTC
+	select RDA_TIMER
 	help
 	  This enables support for the RDA Micro 8810PL SoC family.
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 55c77e44bb2d..f51eee3a72ea 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -105,6 +105,13 @@  config OWL_TIMER
 	help
 	  Enables the support for the Actions Semi Owl timer driver.
 
+config RDA_TIMER
+	bool "RDA timer driver" if COMPILE_TEST
+	depends on GENERIC_CLOCKEVENTS
+	select CLKSRC_MMIO
+	help
+	  Enables the support for the RDA Micro timer driver.
+
 config SUN4I_TIMER
 	bool "Sun4i timer driver" if COMPILE_TEST
 	depends on HAS_IOMEM
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index dd9138104568..150020a90707 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -57,6 +57,7 @@  obj-$(CONFIG_OXNAS_RPS_TIMER)	+= timer-oxnas-rps.o
 obj-$(CONFIG_OWL_TIMER)		+= timer-owl.o
 obj-$(CONFIG_SPRD_TIMER)	+= timer-sprd.o
 obj-$(CONFIG_NPCM7XX_TIMER)	+= timer-npcm7xx.o
+obj-$(CONFIG_RDA_TIMER)		+= timer-rda.o
 
 obj-$(CONFIG_ARC_TIMERS)		+= arc_timer.o
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
diff --git a/drivers/clocksource/timer-rda.c b/drivers/clocksource/timer-rda.c
new file mode 100644
index 000000000000..3aa684d92c5d
--- /dev/null
+++ b/drivers/clocksource/timer-rda.c
@@ -0,0 +1,187 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * RDA8810PL SoC timer driver
+ *
+ * Copyright RDA Microelectronics Company Limited
+ * Copyright (c) 2017 Andreas Färber
+ * Copyright (c) 2018 Manivannan Sadhasivam
+ */
+
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#define RDA_OSTIMER_LOADVAL_L	0x000
+#define RDA_OSTIMER_CTRL	0x004
+#define RDA_HWTIMER_LOCKVAL_L	0x024
+#define RDA_HWTIMER_LOCKVAL_H	0x028
+#define RDA_TIMER_IRQ_MASK_SET	0x02c
+#define RDA_TIMER_IRQ_CLR	0x034
+
+#define RDA_OSTIMER_CTRL_ENABLE		BIT(24)
+#define RDA_OSTIMER_CTRL_REPEAT		BIT(28)
+#define RDA_OSTIMER_CTRL_LOAD		BIT(30)
+
+#define RDA_TIMER_IRQ_MASK_SET_OSTIMER	BIT(0)
+
+#define RDA_TIMER_IRQ_CLR_OSTIMER	BIT(0)
+
+static void __iomem *rda_timer_base;
+
+static u64 rda_hwtimer_read(struct clocksource *cs)
+{
+	u32 lo, hi;
+
+	/* Always read low 32 bits first */
+	lo = readl(rda_timer_base + RDA_HWTIMER_LOCKVAL_L);
+	hi = readl(rda_timer_base + RDA_HWTIMER_LOCKVAL_H);
+
+	return ((u64)hi << 32) | lo;
+}
+
+static struct clocksource rda_clocksource = {
+	.name           = "rda-timer",
+	.rating         = 400,
+	.read           = rda_hwtimer_read,
+	.mask           = CLOCKSOURCE_MASK(64),
+	.flags          = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+static int rda_ostimer_start(bool periodic, u64 cycles)
+{
+	u32 ctrl, load_l;
+
+	load_l = (u32)cycles;
+	ctrl = ((cycles >> 32) & 0xffffff);
+	ctrl |= RDA_OSTIMER_CTRL_LOAD | RDA_OSTIMER_CTRL_ENABLE;
+	if (periodic)
+		ctrl |= RDA_OSTIMER_CTRL_REPEAT;
+
+	/* Enable ostimer interrupt first */
+	writel(RDA_TIMER_IRQ_MASK_SET_OSTIMER,
+	       rda_timer_base + RDA_TIMER_IRQ_MASK_SET);
+
+	/* Write low 32 bits first, high 24 bits are with ctrl */
+	writel(load_l, rda_timer_base + RDA_OSTIMER_LOADVAL_L);
+	writel(ctrl, rda_timer_base + RDA_OSTIMER_CTRL);
+
+	return 0;
+}
+
+static int rda_ostimer_stop(void)
+{
+	/* Disable ostimer interrupt first */
+	writel(0, rda_timer_base + RDA_TIMER_IRQ_MASK_SET);
+
+	writel(0, rda_timer_base + RDA_OSTIMER_CTRL);
+
+	return 0;
+}
+
+static int rda_ostimer_set_state_shutdown(struct clock_event_device *evt)
+{
+	rda_ostimer_stop();
+
+	return 0;
+}
+
+static int rda_ostimer_set_state_oneshot(struct clock_event_device *evt)
+{
+	rda_ostimer_stop();
+
+	return 0;
+}
+
+static int rda_ostimer_set_state_periodic(struct clock_event_device *evt)
+{
+	unsigned long cycles_per_jiffy;
+
+	rda_ostimer_stop();
+
+	cycles_per_jiffy = ((unsigned long long)NSEC_PER_SEC / HZ *
+			     evt->mult) >> evt->shift;
+	rda_ostimer_start(true, cycles_per_jiffy);
+
+	return 0;
+}
+
+static int rda_ostimer_tick_resume(struct clock_event_device *evt)
+{
+	return 0;
+}
+
+static int rda_ostimer_set_next_event(unsigned long evt,
+				      struct clock_event_device *ev)
+{
+	rda_ostimer_start(false, evt);
+
+	return 0;
+}
+
+static struct clock_event_device rda_clockevent = {
+	.name			= "rda-ostimer",
+	.rating			= 250,
+	.features		= CLOCK_EVT_FEAT_PERIODIC |
+				  CLOCK_EVT_FEAT_ONESHOT |
+				  CLOCK_EVT_FEAT_DYNIRQ,
+	.set_state_shutdown	= rda_ostimer_set_state_shutdown,
+	.set_state_oneshot	= rda_ostimer_set_state_oneshot,
+	.set_state_periodic	= rda_ostimer_set_state_periodic,
+	.tick_resume		= rda_ostimer_tick_resume,
+	.set_next_event		= rda_ostimer_set_next_event,
+};
+
+static irqreturn_t rda_ostimer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+
+	/* clear timer int */
+	writel(RDA_TIMER_IRQ_CLR_OSTIMER, rda_timer_base + RDA_TIMER_IRQ_CLR);
+
+	if (evt->event_handler)
+		evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
+static int __init rda_timer_init(struct device_node *node)
+{
+	unsigned long rate = 2000000;
+	int ostimer_irq, ret;
+
+	rda_timer_base = of_io_request_and_map(node, 0, "rda-timer");
+	if (IS_ERR(rda_timer_base)) {
+		pr_err("Can't map timer registers");
+		return PTR_ERR(rda_timer_base);
+	}
+
+	ostimer_irq = of_irq_get_byname(node, "ostimer");
+	if (ostimer_irq <= 0) {
+		pr_err("Can't parse ostimer IRQ");
+		return -EINVAL;
+	}
+
+	clocksource_register_hz(&rda_clocksource, rate);
+
+	ret = request_irq(ostimer_irq, rda_ostimer_interrupt, IRQF_TIMER,
+			  "rda-ostimer", &rda_clockevent);
+	if (ret) {
+		pr_err("failed to request irq %d\n", ostimer_irq);
+		return ret;
+	}
+
+	irq_force_affinity(ostimer_irq, cpumask_of(0));
+
+	rda_clockevent.cpumask = cpumask_of(0);
+	rda_clockevent.irq = ostimer_irq;
+	clockevents_config_and_register(&rda_clockevent, rate,
+					0x2, 0xffffffff);
+
+	return 0;
+}
+
+TIMER_OF_DECLARE(rda8810pl, "rda,8810pl-timer", rda_timer_init);