Messages in this thread | | | Date | Fri, 21 Jul 2023 18:58:55 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v3 09/42] clocksource: ep93xx: Add driver for Cirrus Logic EP93xx |
| |
On Thu, Jul 20, 2023 at 02:29:09PM +0300, Nikita Shubin via B4 Relay wrote: > From: Nikita Shubin <nikita.shubin@maquefel.me> > > 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
| |