lkml.org 
[lkml]   [2023]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2023-07-21 18:00    [W:4.223 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site