Re: [PATCH v3 09/42] clocksource: ep93xx: Add driver for Cirrus Logic EP93xx

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 02:29:09PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> 
> 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.

...

+ bits.h

> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/sched_clock.h>

...

> +/*************************************************************************

Won't you marc it as a DOC: section?

> + * 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.
> + *************************************************************************
> + */

...

> +/*
> + * 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

One too many spaces.

> + * saved in the Timer4ValueHigh register.
> + */

...

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

Would 0 suffice?

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

fwnode_iomap()?
See below why it might make sense.

> +	if (!tcu->base) {

> +		pr_err("Can't remap registers\n");

First of all, you may utilize pr_fmt().
Second, you may add %pOF for better user experience.

> +		ret = -ENXIO;
> +		goto out_free;
> +	}

> +	irq = irq_of_parse_and_map(np, 0);

fwnode_irq_get() which is better in terms of error handling.

> +	if (irq == 0)
> +		irq = -EINVAL;
> +	if (irq < 0) {

> +		pr_err("EP93XX Timer Can't parse IRQ %d", irq);

As per above.

> +		goto out_free;
> +	}

...

> +}

-- 
With Best Regards,
Andy Shevchenko