[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZLqrPw933NOv1J8v@smile.fi.intel.com>
Date: Fri, 21 Jul 2023 18:58:55 +0300
From: Andy Shevchenko <andy@...nel.org>
To: nikita.shubin@...uefel.me
Cc: Hartley Sweeten <hsweeten@...ionengravers.com>,
Lennert Buytenhek <kernel@...tstofly.org>,
Alexander Sverdlin <alexander.sverdlin@...il.com>,
Russell King <linux@...linux.org.uk>,
Lukasz Majewski <lukma@...x.de>,
Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Thomas Gleixner <tglx@...utronix.de>,
Alessandro Zummo <a.zummo@...ertech.it>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Wim Van Sebroeck <wim@...ux-watchdog.org>,
Guenter Roeck <linux@...ck-us.net>,
Sebastian Reichel <sre@...nel.org>,
Thierry Reding <thierry.reding@...il.com>,
Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
Mark Brown <broonie@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Vinod Koul <vkoul@...nel.org>,
Miquel Raynal <miquel.raynal@...tlin.com>,
Richard Weinberger <richard@....at>,
Vignesh Raghavendra <vigneshr@...com>,
Damien Le Moal <dlemoal@...nel.org>,
Sergey Shtylyov <s.shtylyov@....ru>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Arnd Bergmann <arnd@...db.de>, Olof Johansson <olof@...om.net>,
soc@...nel.org, Liam Girdwood <lgirdwood@...il.com>,
Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
Michael Peters <mpeters@...eddedts.com>,
Kris Bahnsen <kris@...eddedts.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
linux-clk@...r.kernel.org, linux-rtc@...r.kernel.org,
linux-watchdog@...r.kernel.org, linux-pm@...r.kernel.org,
linux-pwm@...r.kernel.org, linux-spi@...r.kernel.org,
netdev@...r.kernel.org, dmaengine@...r.kernel.org,
linux-mtd@...ts.infradead.org, linux-ide@...r.kernel.org,
linux-input@...r.kernel.org, alsa-devel@...a-project.org
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@...uefel.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
Powered by blists - more mailing lists