Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
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
> > Signed-off-by: Manivannan Sadhasivam
> > ---
>
> [ ... ]
>
> > +static int __init rda_timer_init(struct device_node *node)
> > +{
> > + unsigned long rate = 200;
> > + 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(_clocksource, rate);
> > +
> > + ret = request_irq(ostimer_irq, rda_ostimer_interrupt, IRQF_TIMER,
> > + "rda-ostimer", _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
>
>
> --
> Facebook |
> Twitter |
>
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
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
> > Signed-off-by: Manivannan Sadhasivam
> > ---
>
> [ ... ]
>
> > +static int __init rda_timer_init(struct device_node *node)
> > +{
> > + unsigned long rate = 200;
> > + 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(_clocksource, rate);
> > +
> > + ret = request_irq(ostimer_irq, rda_ostimer_interrupt, IRQF_TIMER,
> > + "rda-ostimer", _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
>
>
> --
> Facebook |
> Twitter |
>
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
On Tue, Nov 20, 2018 at 11:05:41AM +, Marc Zyngier wrote:
> On 20/11/2018 08:56, Linus Walleij wrote:
> > On Tue, Nov 20, 2018 at 9:17 AM Marc Zyngier 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...
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
On Tue, Nov 20, 2018 at 11:05:41AM +, Marc Zyngier wrote:
> On 20/11/2018 08:56, Linus Walleij wrote:
> > On Tue, Nov 20, 2018 at 9:17 AM Marc Zyngier 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...
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
On 20/11/2018 08:56, Linus Walleij wrote:
> On Tue, Nov 20, 2018 at 9:17 AM Marc Zyngier 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.
--
Jazz is not dead. It just smells funny...
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
On 20/11/2018 08:56, Linus Walleij wrote:
> On Tue, Nov 20, 2018 at 9:17 AM Marc Zyngier 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.
--
Jazz is not dead. It just smells funny...
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
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
> Signed-off-by: Manivannan Sadhasivam
> ---
[ ... ]
> +static int __init rda_timer_init(struct device_node *node)
> +{
> + unsigned long rate = 200;
> + 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(_clocksource, rate);
> +
> + ret = request_irq(ostimer_irq, rda_ostimer_interrupt, IRQF_TIMER,
> + "rda-ostimer", _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
--
Facebook |
Twitter |
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
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
> Signed-off-by: Manivannan Sadhasivam
> ---
[ ... ]
> +static int __init rda_timer_init(struct device_node *node)
> +{
> + unsigned long rate = 200;
> + 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(_clocksource, rate);
> +
> + ret = request_irq(ostimer_irq, rda_ostimer_interrupt, IRQF_TIMER,
> + "rda-ostimer", _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
--
Facebook |
Twitter |
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
On Tue, Nov 20, 2018 at 9:17 AM Marc Zyngier 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
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
On Tue, Nov 20, 2018 at 9:17 AM Marc Zyngier 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
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
On Tue, 20 Nov 2018 05:06:50 +,
Manivannan Sadhasivam wrote:
>
> Hi Marc,
>
> On Mon, Nov 19, 2018 at 05:57:12PM +, 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
> > > Signed-off-by: Manivannan Sadhasivam
> > > ---
> > > 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 ..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
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#define RDA_OSTIMER_LOADVAL_L0x000
> > > +#define RDA_OSTIMER_CTRL 0x004
> > > +#define RDA_HWTIMER_LOCKVAL_L0x024
> > > +#define RDA_HWTIMER_LOCKVAL_H0x028
> > > +#define RDA_TIMER_IRQ_MASK_SET 0x02c
> > > +#define RDA_TIMER_IRQ_CLR0x034
> > > +
> > > +#define RDA_OSTIMER_CTRL_ENABLE BIT(24)
> > > +#define RDA_OSTIMER_CTRL_REPEAT BIT(28)
> > > +#define RDA_OSTIMER_CTRL_LOADBIT(30)
> > > +
> > > +#define RDA_TIMER_IRQ_MASK_SET_OSTIMER BIT(0)
> > > +
> > > +#define RDA_TIMER_IRQ_CLR_OSTIMERBIT(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.
> >
> >
> > lo = 0x;
> >
> > hi = 0x0001;
> >
> > 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
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
On Tue, 20 Nov 2018 05:06:50 +,
Manivannan Sadhasivam wrote:
>
> Hi Marc,
>
> On Mon, Nov 19, 2018 at 05:57:12PM +, 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
> > > Signed-off-by: Manivannan Sadhasivam
> > > ---
> > > 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 ..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
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#define RDA_OSTIMER_LOADVAL_L0x000
> > > +#define RDA_OSTIMER_CTRL 0x004
> > > +#define RDA_HWTIMER_LOCKVAL_L0x024
> > > +#define RDA_HWTIMER_LOCKVAL_H0x028
> > > +#define RDA_TIMER_IRQ_MASK_SET 0x02c
> > > +#define RDA_TIMER_IRQ_CLR0x034
> > > +
> > > +#define RDA_OSTIMER_CTRL_ENABLE BIT(24)
> > > +#define RDA_OSTIMER_CTRL_REPEAT BIT(28)
> > > +#define RDA_OSTIMER_CTRL_LOADBIT(30)
> > > +
> > > +#define RDA_TIMER_IRQ_MASK_SET_OSTIMER BIT(0)
> > > +
> > > +#define RDA_TIMER_IRQ_CLR_OSTIMERBIT(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.
> >
> >
> > lo = 0x;
> >
> > hi = 0x0001;
> >
> > 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
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
Hi Marc,
On Mon, Nov 19, 2018 at 05:57:12PM +, 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
> > Signed-off-by: Manivannan Sadhasivam
> > ---
> > 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 ..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
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#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_ENABLEBIT(24)
> > +#define RDA_OSTIMER_CTRL_REPEATBIT(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.
>
>
> lo = 0x;
>
> hi = 0x0001;
>
> 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;
>
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
Hi Marc,
On Mon, Nov 19, 2018 at 05:57:12PM +, 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
> > Signed-off-by: Manivannan Sadhasivam
> > ---
> > 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 ..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
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#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_ENABLEBIT(24)
> > +#define RDA_OSTIMER_CTRL_REPEATBIT(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.
>
>
> lo = 0x;
>
> hi = 0x0001;
>
> 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;
>
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
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
> Signed-off-by: Manivannan Sadhasivam
> ---
> 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 ..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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define RDA_OSTIMER_LOADVAL_L0x000
> +#define RDA_OSTIMER_CTRL 0x004
> +#define RDA_HWTIMER_LOCKVAL_L0x024
> +#define RDA_HWTIMER_LOCKVAL_H0x028
> +#define RDA_TIMER_IRQ_MASK_SET 0x02c
> +#define RDA_TIMER_IRQ_CLR0x034
> +
> +#define RDA_OSTIMER_CTRL_ENABLE BIT(24)
> +#define RDA_OSTIMER_CTRL_REPEAT BIT(28)
> +#define RDA_OSTIMER_CTRL_LOADBIT(30)
> +
> +#define RDA_TIMER_IRQ_MASK_SET_OSTIMER BIT(0)
> +
> +#define RDA_TIMER_IRQ_CLR_OSTIMERBIT(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.
lo = 0x;
hi = 0x0001;
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) & 0xff);
> + 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
Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
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
> Signed-off-by: Manivannan Sadhasivam
> ---
> 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 ..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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define RDA_OSTIMER_LOADVAL_L0x000
> +#define RDA_OSTIMER_CTRL 0x004
> +#define RDA_HWTIMER_LOCKVAL_L0x024
> +#define RDA_HWTIMER_LOCKVAL_H0x028
> +#define RDA_TIMER_IRQ_MASK_SET 0x02c
> +#define RDA_TIMER_IRQ_CLR0x034
> +
> +#define RDA_OSTIMER_CTRL_ENABLE BIT(24)
> +#define RDA_OSTIMER_CTRL_REPEAT BIT(28)
> +#define RDA_OSTIMER_CTRL_LOADBIT(30)
> +
> +#define RDA_TIMER_IRQ_MASK_SET_OSTIMER BIT(0)
> +
> +#define RDA_TIMER_IRQ_CLR_OSTIMERBIT(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.
lo = 0x;
hi = 0x0001;
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) & 0xff);
> + 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
[PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
Add clock driver for RDA Micro RDA8810PL SoC supporting OSTIMER
and HWTIMER.
Signed-off-by: Andreas Färber
Signed-off-by: Manivannan Sadhasivam
---
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 ..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
+#include
+#include
+#include
+#include
+#include
+#include
+
+#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_ENABLEBIT(24)
+#define RDA_OSTIMER_CTRL_REPEATBIT(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) & 0xff);
+ 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 *
+
[PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC
Add clock driver for RDA Micro RDA8810PL SoC supporting OSTIMER
and HWTIMER.
Signed-off-by: Andreas Färber
Signed-off-by: Manivannan Sadhasivam
---
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 ..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
+#include
+#include
+#include
+#include
+#include
+#include
+
+#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_ENABLEBIT(24)
+#define RDA_OSTIMER_CTRL_REPEATBIT(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) & 0xff);
+ 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 *
+
