[v1,09/43] clocksource: ep93xx: Add driver for Cirrus Logic EP93xx
Commit Message
This us a rewrite of EP93xx timer driver in
arch/arm/mach-ep93xx/timer-ep93xx.c trying to do everything
the device tree way:
- Make every IO-access relative to a base address and dynamic
so we can do a dynamic ioremap and get going.
- Find register range and interrupt from the device tree.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
Notes:
v0 -> v1:
- fixed headers
drivers/clocksource/Kconfig | 11 ++
drivers/clocksource/Makefile | 1 +
drivers/clocksource/timer-ep93xx.c | 189 +++++++++++++++++++++++++++++
3 files changed, 201 insertions(+)
create mode 100644 drivers/clocksource/timer-ep93xx.c
Comments
Thu, Jun 01, 2023 at 08:34:00AM +0300, Nikita Shubin kirjoitti:
> This us a rewrite of EP93xx timer driver in
> arch/arm/mach-ep93xx/timer-ep93xx.c trying to do everything
> the device tree way:
>
> - Make every IO-access relative to a base address and dynamic
> so we can do a dynamic ioremap and get going.
> - Find register range and interrupt from the device tree.
...
> +config EP93XX_TIMER
> + bool "Cirrus Logic ep93xx timer driver" if COMPILE_TEST
This is strange. What do you gain with this "if COMPILE_TEST"?
> + depends on ARCH_EP93XX
> + depends on GENERIC_CLOCKEVENTS
> + depends on HAS_IOMEM
> + select CLKSRC_MMIO
> + select TIMER_OF
...
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> +#include <linux/sched_clock.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/io.h>
Can you keep that ordered?
Missing bits.h.
+ Blank line.
> +#include <asm/mach/time.h>
...
> +/* This read-only register contains the low word of the time stamp debug timer
> + * ( Timer4). When this register is read, the high byte of the Timer4 counter is
> + * saved in the Timer4ValueHigh register.
> + */
/*
* Wrong multi-line comment style.
* Use this example, for example.
*/
...
> +static struct ep93xx_tcu *ep93xx_tcu;
Global?!
Can it be derived from struct clocksource?
...
> +static u64 ep93xx_clocksource_read(struct clocksource *c)
> +{
> + struct ep93xx_tcu *tcu = ep93xx_tcu;
> + u64 ret;
> +
> + ret = readl(tcu->base + EP93XX_TIMER4_VALUE_LOW);
> + ret |= ((u64) (readl(tcu->base + EP93XX_TIMER4_VALUE_HIGH) & 0xff) << 32);
GENMASK()
Why you are not using non-atomic 64-bit io accessors? Becomes as simple as
return lo_hi_readq() & GENMASK();
> + return (u64) ret;
Redundant casting.
> +}
...
> + irq = irq_of_parse_and_map(np, 0);
> + if (irq <= 0) {
> + pr_err("ERROR: invalid interrupt number\n");
> + ret = -EINVAL;
Shadowed error in case of negative returned code. Why?
> + goto out_free;
> + }
...
> + clockevents_config_and_register(&ep93xx_clockevent,
> + EP93XX_TIMER123_RATE,
> + 1,
> + 0xffffffffU);
UINT_MAX? GENMASK() ?
...
> +
Redundant blank line.
> +TIMER_OF_DECLARE(ep93xx_timer, "cirrus,ep9301-timer", ep93xx_timer_of_init);
On Thu, 2023-06-01 at 08:34 +0300, Nikita Shubin wrote:
> This us a rewrite of EP93xx timer driver in
> arch/arm/mach-ep93xx/timer-ep93xx.c trying to do everything
> the device tree way:
>
> - Make every IO-access relative to a base address and dynamic
> so we can do a dynamic ioremap and get going.
> - Find register range and interrupt from the device tree.
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
Tested-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> ---
>
> Notes:
> v0 -> v1:
>
> - fixed headers
>
> drivers/clocksource/Kconfig | 11 ++
> drivers/clocksource/Makefile | 1 +
> drivers/clocksource/timer-ep93xx.c | 189 +++++++++++++++++++++++++++++
> 3 files changed, 201 insertions(+)
> create mode 100644 drivers/clocksource/timer-ep93xx.c
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 5fc8f0e7fb38..40bfc7c86756 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -721,4 +721,15 @@ config GOLDFISH_TIMER
> help
> Support for the timer/counter of goldfish-rtc
>
> +config EP93XX_TIMER
> + bool "Cirrus Logic ep93xx timer driver" if COMPILE_TEST
> + depends on ARCH_EP93XX
> + depends on GENERIC_CLOCKEVENTS
> + depends on HAS_IOMEM
> + select CLKSRC_MMIO
> + select TIMER_OF
> + help
> + Enables support for the Cirrus Logic timer block
> + EP93XX.
> +
> endmenu
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 64ab547de97b..09c2d4e5d809 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -88,3 +88,4 @@ obj-$(CONFIG_MICROCHIP_PIT64B) += timer-microchip-pit64b.o
> obj-$(CONFIG_MSC313E_TIMER) += timer-msc313e.o
> obj-$(CONFIG_GOLDFISH_TIMER) += timer-goldfish.o
> obj-$(CONFIG_GXP_TIMER) += timer-gxp.o
> +obj-$(CONFIG_EP93XX_TIMER) += timer-ep93xx.o
> diff --git a/drivers/clocksource/timer-ep93xx.c b/drivers/clocksource/timer-ep93xx.c
> new file mode 100644
> index 000000000000..966502169aa0
> --- /dev/null
> +++ b/drivers/clocksource/timer-ep93xx.c
> @@ -0,0 +1,189 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Cirrus Logic EP93xx timer driver.
> + * Copyright (C) 2021 Nikita Shubin <nikita.shubin@maquefel.me>
> + *
> + * Based on a rewrite of arch/arm/mach-ep93xx/timer.c:
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> +#include <linux/sched_clock.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/io.h>
> +#include <asm/mach/time.h>
> +
> +/*************************************************************************
> + * Timer handling for EP93xx
> + *************************************************************************
> + * The ep93xx has four internal timers. Timers 1, 2 (both 16 bit) and
> + * 3 (32 bit) count down at 508 kHz, are self-reloading, and can generate
> + * an interrupt on underflow. Timer 4 (40 bit) counts down at 983.04 kHz,
> + * is free-running, and can't generate interrupts.
> + *
> + * The 508 kHz timers are ideal for use for the timer interrupt, as the
> + * most common values of HZ divide 508 kHz nicely. We pick the 32 bit
> + * timer (timer 3) to get as long sleep intervals as possible when using
> + * CONFIG_NO_HZ.
> + *
> + * The higher clock rate of timer 4 makes it a better choice than the
> + * other timers for use as clock source and for sched_clock(), providing
> + * a stable 40 bit time base.
> + *************************************************************************
> + */
> +
> +#define EP93XX_TIMER1_LOAD 0x00
> +#define EP93XX_TIMER1_VALUE 0x04
> +#define EP93XX_TIMER1_CONTROL 0x08
> +#define EP93XX_TIMER123_CONTROL_ENABLE BIT(7)
> +#define EP93XX_TIMER123_CONTROL_MODE BIT(6)
> +#define EP93XX_TIMER123_CONTROL_CLKSEL BIT(3)
> +#define EP93XX_TIMER1_CLEAR 0x0c
> +#define EP93XX_TIMER2_LOAD 0x20
> +#define EP93XX_TIMER2_VALUE 0x24
> +#define EP93XX_TIMER2_CONTROL 0x28
> +#define EP93XX_TIMER2_CLEAR 0x2c
> +/* This read-only register contains the low word of the time stamp debug timer
> + * ( Timer4). When this register is read, the high byte of the Timer4 counter is
> + * saved in the Timer4ValueHigh register.
> + */
> +#define EP93XX_TIMER4_VALUE_LOW 0x60
> +#define EP93XX_TIMER4_VALUE_HIGH 0x64
> +#define EP93XX_TIMER4_VALUE_HIGH_ENABLE BIT(8)
> +#define EP93XX_TIMER3_LOAD 0x80
> +#define EP93XX_TIMER3_VALUE 0x84
> +#define EP93XX_TIMER3_CONTROL 0x88
> +#define EP93XX_TIMER3_CLEAR 0x8c
> +
> +#define EP93XX_TIMER123_RATE 508469
> +#define EP93XX_TIMER4_RATE 983040
> +
> +struct ep93xx_tcu {
> + void __iomem *base;
> +};
> +
> +static struct ep93xx_tcu *ep93xx_tcu;
> +
> +static u64 ep93xx_clocksource_read(struct clocksource *c)
> +{
> + struct ep93xx_tcu *tcu = ep93xx_tcu;
> + u64 ret;
> +
> + ret = readl(tcu->base + EP93XX_TIMER4_VALUE_LOW);
> + ret |= ((u64) (readl(tcu->base + EP93XX_TIMER4_VALUE_HIGH) & 0xff) << 32);
> + return (u64) ret;
> +}
> +
> +static u64 notrace ep93xx_read_sched_clock(void)
> +{
> + return ep93xx_clocksource_read(NULL);
> +}
> +
> +static int ep93xx_clkevt_set_next_event(unsigned long next,
> + struct clock_event_device *evt)
> +{
> + struct ep93xx_tcu *tcu = ep93xx_tcu;
> + /* Default mode: periodic, off, 508 kHz */
> + u32 tmode = EP93XX_TIMER123_CONTROL_MODE |
> + EP93XX_TIMER123_CONTROL_CLKSEL;
> +
> + /* Clear timer */
> + writel(tmode, tcu->base + EP93XX_TIMER3_CONTROL);
> +
> + /* Set next event */
> + writel(next, tcu->base + EP93XX_TIMER3_LOAD);
> + writel(tmode | EP93XX_TIMER123_CONTROL_ENABLE,
> + tcu->base + EP93XX_TIMER3_CONTROL);
> + return 0;
> +}
> +
> +static int ep93xx_clkevt_shutdown(struct clock_event_device *evt)
> +{
> + struct ep93xx_tcu *tcu = ep93xx_tcu;
> + /* Disable timer */
> + writel(0, tcu->base + EP93XX_TIMER3_CONTROL);
> +
> + return 0;
> +}
> +
> +static struct clock_event_device ep93xx_clockevent = {
> + .name = "timer1",
> + .features = CLOCK_EVT_FEAT_ONESHOT,
> + .set_state_shutdown = ep93xx_clkevt_shutdown,
> + .set_state_oneshot = ep93xx_clkevt_shutdown,
> + .tick_resume = ep93xx_clkevt_shutdown,
> + .set_next_event = ep93xx_clkevt_set_next_event,
> + .rating = 300,
> +};
> +
> +static irqreturn_t ep93xx_timer_interrupt(int irq, void *dev_id)
> +{
> + struct ep93xx_tcu *tcu = ep93xx_tcu;
> + struct clock_event_device *evt = dev_id;
> +
> + /* Writing any value clears the timer interrupt */
> + writel(1, tcu->base + EP93XX_TIMER3_CLEAR);
> +
> + evt->event_handler(evt);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int __init ep93xx_timer_of_init(struct device_node *np)
> +{
> + int irq;
> + unsigned long flags = IRQF_TIMER | IRQF_IRQPOLL;
> + struct ep93xx_tcu *tcu;
> + int ret;
> +
> + tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
> + if (!tcu)
> + return -ENOMEM;
> +
> + tcu->base = of_iomap(np, 0);
> + if (!tcu->base) {
> + pr_err("Can't remap registers\n");
> + ret = -ENXIO;
> + goto out_free;
> + }
> +
> + ep93xx_tcu = tcu;
> +
> + irq = irq_of_parse_and_map(np, 0);
> + if (irq <= 0) {
> + pr_err("ERROR: invalid interrupt number\n");
> + ret = -EINVAL;
> + goto out_free;
> + }
> +
> + /* Enable and register clocksource and sched_clock on timer 4 */
> + writel(EP93XX_TIMER4_VALUE_HIGH_ENABLE,
> + tcu->base + EP93XX_TIMER4_VALUE_HIGH);
> + clocksource_mmio_init(NULL, "timer4",
> + EP93XX_TIMER4_RATE, 200, 40,
> + ep93xx_clocksource_read);
> + sched_clock_register(ep93xx_read_sched_clock, 40,
> + EP93XX_TIMER4_RATE);
> +
> + /* Set up clockevent on timer 3 */
> + if (request_irq(irq, ep93xx_timer_interrupt, flags, "ep93xx timer",
> + &ep93xx_clockevent))
> + pr_err("Failed to request irq %d (ep93xx timer)\n", irq);
> + clockevents_config_and_register(&ep93xx_clockevent,
> + EP93XX_TIMER123_RATE,
> + 1,
> + 0xffffffffU);
> +
> + return 0;
> +
> +out_free:
> + kfree(tcu);
> + return ret;
> +}
> +
> +TIMER_OF_DECLARE(ep93xx_timer, "cirrus,ep9301-timer", ep93xx_timer_of_init);
On Sat, Jun 3, 2023, at 22:06, andy.shevchenko@gmail.com wrote:
> Thu, Jun 01, 2023 at 08:34:00AM +0300, Nikita Shubin kirjoitti:
>> This us a rewrite of EP93xx timer driver in
>> arch/arm/mach-ep93xx/timer-ep93xx.c trying to do everything
>> the device tree way:
>>
>> - Make every IO-access relative to a base address and dynamic
>> so we can do a dynamic ioremap and get going.
>> - Find register range and interrupt from the device tree.
>
> ...
>
>> +config EP93XX_TIMER
>> + bool "Cirrus Logic ep93xx timer driver" if COMPILE_TEST
>
> This is strange. What do you gain with this "if COMPILE_TEST"?
This ensures the driver is compiled in an x86 allmodconfig,
like most other clocksource drivers, but it's hidden on
all other configs without EP93xx support.
As long as the platform selects it, this is the normal
way to add a clocksource driver.
Arnd
On Sun, Jun 4, 2023 at 8:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Sat, Jun 3, 2023, at 22:06, andy.shevchenko@gmail.com wrote:
> > Thu, Jun 01, 2023 at 08:34:00AM +0300, Nikita Shubin kirjoitti:
> >> This us a rewrite of EP93xx timer driver in
> >> arch/arm/mach-ep93xx/timer-ep93xx.c trying to do everything
> >> the device tree way:
> >>
> >> - Make every IO-access relative to a base address and dynamic
> >> so we can do a dynamic ioremap and get going.
> >> - Find register range and interrupt from the device tree.
> >
> > ...
> >
> >> +config EP93XX_TIMER
> >> + bool "Cirrus Logic ep93xx timer driver" if COMPILE_TEST
> >
> > This is strange. What do you gain with this "if COMPILE_TEST"?
>
> This ensures the driver is compiled in an x86 allmodconfig,
> like most other clocksource drivers, but it's hidden on
> all other configs without EP93xx support.
This is cool!
> As long as the platform selects it, this is the normal
> way to add a clocksource driver.
Shall we now require this format of COMPILE_TEST for all new code
(which is selectable)?
On Sun, Jun 4, 2023, at 21:24, Andy Shevchenko wrote:
> On Sun, Jun 4, 2023 at 8:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Sat, Jun 3, 2023, at 22:06, andy.shevchenko@gmail.com wrote:
>> > Thu, Jun 01, 2023 at 08:34:00AM +0300, Nikita Shubin kirjoitti:
>
>> As long as the platform selects it, this is the normal
>> way to add a clocksource driver.
>
> Shall we now require this format of COMPILE_TEST for all new code
> (which is selectable)?
It's somewhat subsystem specific. For irqchip and clocksource drivers,
I think it's already done this way, but these are the ones that expect
a platform Kconfig option to select the drivers, which is usually
not done for other subsystems.
If the driver is not selected by the platform, you usually have
config FOO
tristate "description"
depends on ${PLATFORM} || COMPILE_TEST
default ${PLATFORM}
which makes it possible to still disable it, or (optionally)
hide the option by adding "if COMPILE_TEST && !${PLATFORM}"
which I personally find a little too complicated but is
appropriate for some drivers that are required for booting.
Arnd
On Sun, Jun 4, 2023 at 10:34 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Sun, Jun 4, 2023, at 21:24, Andy Shevchenko wrote:
> > On Sun, Jun 4, 2023 at 8:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Sat, Jun 3, 2023, at 22:06, andy.shevchenko@gmail.com wrote:
> >> > Thu, Jun 01, 2023 at 08:34:00AM +0300, Nikita Shubin kirjoitti:
> >
> >> As long as the platform selects it, this is the normal
> >> way to add a clocksource driver.
> >
> > Shall we now require this format of COMPILE_TEST for all new code
> > (which is selectable)?
>
> It's somewhat subsystem specific. For irqchip and clocksource drivers,
> I think it's already done this way, but these are the ones that expect
> a platform Kconfig option to select the drivers, which is usually
> not done for other subsystems.
>
> If the driver is not selected by the platform, you usually have
>
> config FOO
> tristate "description"
> depends on ${PLATFORM} || COMPILE_TEST
> default ${PLATFORM}
>
> which makes it possible to still disable it, or (optionally)
> hide the option by adding "if COMPILE_TEST && !${PLATFORM}"
> which I personally find a little too complicated but is
> appropriate for some drivers that are required for booting.
Thank you for this nice elaboration (every day to learn something new)!
On Wed, Jun 21, 2023 at 11:22 AM Nikita Shubin
<nikita.shubin@maquefel.me> wrote:
...
> > > + irq = irq_of_parse_and_map(np, 0);
> > > + if (irq <= 0) {
> > > + pr_err("ERROR: invalid interrupt number\n");
> > > + ret = -EINVAL;
> >
> > Shadowed error in case of negative returned code. Why?
>
> Majority of clocksource drivers shadow it. Same like above.
It doesn't mean they are correct or using brand new APIs.
Can you use fwnode_irq_get() instead?
The shadowing is most likely due to nasty =0 comparison.
Also that ERROR is a bit weird, pr_err() is already on error level.
Hello Andy!
Agreed to almost, still... :
>
> ...
>
> > +static struct ep93xx_tcu *ep93xx_tcu;
>
> Global?!
> Can it be derived from struct clocksource?
>
It's look like a common practice for read_sched_clock, even for most
new drivers. I would like for comment from Daniel or Thomas before
ripping it out.
>
> > + irq = irq_of_parse_and_map(np, 0);
> > + if (irq <= 0) {
> > + pr_err("ERROR: invalid interrupt number\n");
> > + ret = -EINVAL;
>
> Shadowed error in case of negative returned code. Why?
Majority of clocksource drivers shadow it. Same like above.
All other comments applied - thank you!
On Thu, Jun 29, 2023 at 4:10 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
> On Wed, 2023-06-21 at 11:28 +0300, Andy Shevchenko wrote:
> > On Wed, Jun 21, 2023 at 11:22 AM Nikita Shubin
> > <nikita.shubin@maquefel.me> wrote:
...
> > > > > + irq = irq_of_parse_and_map(np, 0);
> > > > > + if (irq <= 0) {
> > > > > + pr_err("ERROR: invalid interrupt number\n");
> > > > > + ret = -EINVAL;
> > > >
> > > > Shadowed error in case of negative returned code. Why?
> > >
> > > Majority of clocksource drivers shadow it. Same like above.
> >
> > It doesn't mean they are correct or using brand new APIs.
>
> Or may be this because irq_of_parse_and_map can return zero as error,
> and returning zero from timer_init means success ?
>
> Please correct me if i am wrong here.
0 means no mapped IRQ.
> > Can you use fwnode_irq_get() instead?
>
> Will it help ?
Yes, definitely, in two aspects:
1/ it makes code less OF-dependent (helps also OF people to clean up
the spread of OF headers and APIs where they are not needed);
2/ it takes care about proper error codes.
Hello Andy!
On Wed, 2023-06-21 at 11:28 +0300, Andy Shevchenko wrote:
> On Wed, Jun 21, 2023 at 11:22 AM Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
>
> ...
>
> > > > + irq = irq_of_parse_and_map(np, 0);
> > > > + if (irq <= 0) {
> > > > + pr_err("ERROR: invalid interrupt number\n");
> > > > + ret = -EINVAL;
> > >
> > > Shadowed error in case of negative returned code. Why?
> >
> > Majority of clocksource drivers shadow it. Same like above.
>
> It doesn't mean they are correct or using brand new APIs.
Or may be this because irq_of_parse_and_map can return zero as error,
and returning zero from timer_init means success ?
Please correct me if i am wrong here.
>
> Can you use fwnode_irq_get() instead?
Will it help ?
From docs:
* Return: Linux IRQ number on success. Other values are determined
* according to acpi_irq_get() or of_irq_get() operation.
* of_irq_get()
* Return: Linux IRQ number on success, or 0 on the IRQ mapping failure
> The shadowing is most likely due to nasty =0 comparison.
Indeed.
>
> Also that ERROR is a bit weird, pr_err() is already on error level.
>
Completely agree.
On Thu, 2023-06-29 at 16:39 +0300, Andy Shevchenko wrote:
> On Thu, Jun 29, 2023 at 4:10 PM Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
> > On Wed, 2023-06-21 at 11:28 +0300, Andy Shevchenko wrote:
> > > On Wed, Jun 21, 2023 at 11:22 AM Nikita Shubin
> > > <nikita.shubin@maquefel.me> wrote:
>
> ...
>
> > > > > > + irq = irq_of_parse_and_map(np, 0);
> > > > > > + if (irq <= 0) {
> > > > > > + pr_err("ERROR: invalid interrupt
> > > > > > number\n");
> > > > > > + ret = -EINVAL;
> > > > >
> > > > > Shadowed error in case of negative returned code. Why?
> > > >
> > > > Majority of clocksource drivers shadow it. Same like above.
> > >
> > > It doesn't mean they are correct or using brand new APIs.
> >
> > Or may be this because irq_of_parse_and_map can return zero as
> > error,
> > and returning zero from timer_init means success ?
> >
> > Please correct me if i am wrong here.
>
> 0 means no mapped IRQ.
>
> > > Can you use fwnode_irq_get() instead?
> >
> > Will it help ?
>
> Yes, definitely, in two aspects:
> 1/ it makes code less OF-dependent (helps also OF people to clean up
> the spread of OF headers and APIs where they are not needed);
> 2/ it takes care about proper error codes.
>
Well... every use of fwnode_irq_get in 6.4 looks like:
err = fwnode_irq_get(fwnode, 0);
if (err < 0)
return err;
if (!err)
return -EINVAL;
So i don't see any special care as it still calls of_irq_get just like
irq_of_parse_and_map.
But i am convinced to use the same approach, thank you!
On Thu, Jun 29, 2023 at 5:16 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
> On Thu, 2023-06-29 at 16:39 +0300, Andy Shevchenko wrote:
> > On Thu, Jun 29, 2023 at 4:10 PM Nikita Shubin
> > <nikita.shubin@maquefel.me> wrote:
> > > On Wed, 2023-06-21 at 11:28 +0300, Andy Shevchenko wrote:
> > > > On Wed, Jun 21, 2023 at 11:22 AM Nikita Shubin
> > > > <nikita.shubin@maquefel.me> wrote:
...
> > > > > > > + irq = irq_of_parse_and_map(np, 0);
> > > > > > > + if (irq <= 0) {
> > > > > > > + pr_err("ERROR: invalid interrupt
> > > > > > > number\n");
> > > > > > > + ret = -EINVAL;
> > > > > >
> > > > > > Shadowed error in case of negative returned code. Why?
> > > > >
> > > > > Majority of clocksource drivers shadow it. Same like above.
> > > >
> > > > It doesn't mean they are correct or using brand new APIs.
> > >
> > > Or may be this because irq_of_parse_and_map can return zero as
> > > error,
> > > and returning zero from timer_init means success ?
> > >
> > > Please correct me if i am wrong here.
> >
> > 0 means no mapped IRQ.
> >
> > > > Can you use fwnode_irq_get() instead?
> > >
> > > Will it help ?
> >
> > Yes, definitely, in two aspects:
> > 1/ it makes code less OF-dependent (helps also OF people to clean up
> > the spread of OF headers and APIs where they are not needed);
> > 2/ it takes care about proper error codes.
> >
>
> Well... every use of fwnode_irq_get in 6.4 looks like:
>
> err = fwnode_irq_get(fwnode, 0);
> if (err < 0)
> return err;
> if (!err)
> return -EINVAL;
>
> So i don't see any special care as it still calls of_irq_get just like
> irq_of_parse_and_map.
Well, this change is not a fix and not going to land v6.4 anyway, have
you checked what's in the v6.5-rc1? I believe it will have
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next&id=39d422555e43379516d4d13f5b7162a3dee6e646,
will we?
> But i am convinced to use the same approach, thank you!
No, please, don't copy the old and ugly variant, use a brand new one.
@@ -721,4 +721,15 @@ config GOLDFISH_TIMER
help
Support for the timer/counter of goldfish-rtc
+config EP93XX_TIMER
+ bool "Cirrus Logic ep93xx timer driver" if COMPILE_TEST
+ depends on ARCH_EP93XX
+ depends on GENERIC_CLOCKEVENTS
+ depends on HAS_IOMEM
+ select CLKSRC_MMIO
+ select TIMER_OF
+ help
+ Enables support for the Cirrus Logic timer block
+ EP93XX.
+
endmenu
@@ -88,3 +88,4 @@ obj-$(CONFIG_MICROCHIP_PIT64B) += timer-microchip-pit64b.o
obj-$(CONFIG_MSC313E_TIMER) += timer-msc313e.o
obj-$(CONFIG_GOLDFISH_TIMER) += timer-goldfish.o
obj-$(CONFIG_GXP_TIMER) += timer-gxp.o
+obj-$(CONFIG_EP93XX_TIMER) += timer-ep93xx.o
new file mode 100644
@@ -0,0 +1,189 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Cirrus Logic EP93xx timer driver.
+ * Copyright (C) 2021 Nikita Shubin <nikita.shubin@maquefel.me>
+ *
+ * Based on a rewrite of arch/arm/mach-ep93xx/timer.c:
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+#include <linux/sched_clock.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/io.h>
+#include <asm/mach/time.h>
+
+/*************************************************************************
+ * Timer handling for EP93xx
+ *************************************************************************
+ * The ep93xx has four internal timers. Timers 1, 2 (both 16 bit) and
+ * 3 (32 bit) count down at 508 kHz, are self-reloading, and can generate
+ * an interrupt on underflow. Timer 4 (40 bit) counts down at 983.04 kHz,
+ * is free-running, and can't generate interrupts.
+ *
+ * The 508 kHz timers are ideal for use for the timer interrupt, as the
+ * most common values of HZ divide 508 kHz nicely. We pick the 32 bit
+ * timer (timer 3) to get as long sleep intervals as possible when using
+ * CONFIG_NO_HZ.
+ *
+ * The higher clock rate of timer 4 makes it a better choice than the
+ * other timers for use as clock source and for sched_clock(), providing
+ * a stable 40 bit time base.
+ *************************************************************************
+ */
+
+#define EP93XX_TIMER1_LOAD 0x00
+#define EP93XX_TIMER1_VALUE 0x04
+#define EP93XX_TIMER1_CONTROL 0x08
+#define EP93XX_TIMER123_CONTROL_ENABLE BIT(7)
+#define EP93XX_TIMER123_CONTROL_MODE BIT(6)
+#define EP93XX_TIMER123_CONTROL_CLKSEL BIT(3)
+#define EP93XX_TIMER1_CLEAR 0x0c
+#define EP93XX_TIMER2_LOAD 0x20
+#define EP93XX_TIMER2_VALUE 0x24
+#define EP93XX_TIMER2_CONTROL 0x28
+#define EP93XX_TIMER2_CLEAR 0x2c
+/* This read-only register contains the low word of the time stamp debug timer
+ * ( Timer4). When this register is read, the high byte of the Timer4 counter is
+ * saved in the Timer4ValueHigh register.
+ */
+#define EP93XX_TIMER4_VALUE_LOW 0x60
+#define EP93XX_TIMER4_VALUE_HIGH 0x64
+#define EP93XX_TIMER4_VALUE_HIGH_ENABLE BIT(8)
+#define EP93XX_TIMER3_LOAD 0x80
+#define EP93XX_TIMER3_VALUE 0x84
+#define EP93XX_TIMER3_CONTROL 0x88
+#define EP93XX_TIMER3_CLEAR 0x8c
+
+#define EP93XX_TIMER123_RATE 508469
+#define EP93XX_TIMER4_RATE 983040
+
+struct ep93xx_tcu {
+ void __iomem *base;
+};
+
+static struct ep93xx_tcu *ep93xx_tcu;
+
+static u64 ep93xx_clocksource_read(struct clocksource *c)
+{
+ struct ep93xx_tcu *tcu = ep93xx_tcu;
+ u64 ret;
+
+ ret = readl(tcu->base + EP93XX_TIMER4_VALUE_LOW);
+ ret |= ((u64) (readl(tcu->base + EP93XX_TIMER4_VALUE_HIGH) & 0xff) << 32);
+ return (u64) ret;
+}
+
+static u64 notrace ep93xx_read_sched_clock(void)
+{
+ return ep93xx_clocksource_read(NULL);
+}
+
+static int ep93xx_clkevt_set_next_event(unsigned long next,
+ struct clock_event_device *evt)
+{
+ struct ep93xx_tcu *tcu = ep93xx_tcu;
+ /* Default mode: periodic, off, 508 kHz */
+ u32 tmode = EP93XX_TIMER123_CONTROL_MODE |
+ EP93XX_TIMER123_CONTROL_CLKSEL;
+
+ /* Clear timer */
+ writel(tmode, tcu->base + EP93XX_TIMER3_CONTROL);
+
+ /* Set next event */
+ writel(next, tcu->base + EP93XX_TIMER3_LOAD);
+ writel(tmode | EP93XX_TIMER123_CONTROL_ENABLE,
+ tcu->base + EP93XX_TIMER3_CONTROL);
+ return 0;
+}
+
+static int ep93xx_clkevt_shutdown(struct clock_event_device *evt)
+{
+ struct ep93xx_tcu *tcu = ep93xx_tcu;
+ /* Disable timer */
+ writel(0, tcu->base + EP93XX_TIMER3_CONTROL);
+
+ return 0;
+}
+
+static struct clock_event_device ep93xx_clockevent = {
+ .name = "timer1",
+ .features = CLOCK_EVT_FEAT_ONESHOT,
+ .set_state_shutdown = ep93xx_clkevt_shutdown,
+ .set_state_oneshot = ep93xx_clkevt_shutdown,
+ .tick_resume = ep93xx_clkevt_shutdown,
+ .set_next_event = ep93xx_clkevt_set_next_event,
+ .rating = 300,
+};
+
+static irqreturn_t ep93xx_timer_interrupt(int irq, void *dev_id)
+{
+ struct ep93xx_tcu *tcu = ep93xx_tcu;
+ struct clock_event_device *evt = dev_id;
+
+ /* Writing any value clears the timer interrupt */
+ writel(1, tcu->base + EP93XX_TIMER3_CLEAR);
+
+ evt->event_handler(evt);
+
+ return IRQ_HANDLED;
+}
+
+static int __init ep93xx_timer_of_init(struct device_node *np)
+{
+ int irq;
+ unsigned long flags = IRQF_TIMER | IRQF_IRQPOLL;
+ struct ep93xx_tcu *tcu;
+ int ret;
+
+ tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
+ if (!tcu)
+ return -ENOMEM;
+
+ tcu->base = of_iomap(np, 0);
+ if (!tcu->base) {
+ pr_err("Can't remap registers\n");
+ ret = -ENXIO;
+ goto out_free;
+ }
+
+ ep93xx_tcu = tcu;
+
+ irq = irq_of_parse_and_map(np, 0);
+ if (irq <= 0) {
+ pr_err("ERROR: invalid interrupt number\n");
+ ret = -EINVAL;
+ goto out_free;
+ }
+
+ /* Enable and register clocksource and sched_clock on timer 4 */
+ writel(EP93XX_TIMER4_VALUE_HIGH_ENABLE,
+ tcu->base + EP93XX_TIMER4_VALUE_HIGH);
+ clocksource_mmio_init(NULL, "timer4",
+ EP93XX_TIMER4_RATE, 200, 40,
+ ep93xx_clocksource_read);
+ sched_clock_register(ep93xx_read_sched_clock, 40,
+ EP93XX_TIMER4_RATE);
+
+ /* Set up clockevent on timer 3 */
+ if (request_irq(irq, ep93xx_timer_interrupt, flags, "ep93xx timer",
+ &ep93xx_clockevent))
+ pr_err("Failed to request irq %d (ep93xx timer)\n", irq);
+ clockevents_config_and_register(&ep93xx_clockevent,
+ EP93XX_TIMER123_RATE,
+ 1,
+ 0xffffffffU);
+
+ return 0;
+
+out_free:
+ kfree(tcu);
+ return ret;
+}
+
+TIMER_OF_DECLARE(ep93xx_timer, "cirrus,ep9301-timer", ep93xx_timer_of_init);