lkml.org 
[lkml]   [2014]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5 21/29] nios2: Time keeping
    On Fri, 24 Oct 2014, Ley Foon Tan wrote:
    > +#ifndef _ASM_NIOS2_TIMEX_H
    > +#define _ASM_NIOS2_TIMEX_H
    > +
    > +typedef unsigned long cycles_t;
    > +
    > +extern cycles_t get_cycles(void);
    > +
    > +#define ARCH_HAS_READ_CURRENT_TIMER

    Why does NIOS need that? Does it have a hardware implementation
    dependent clock frequency which needs to be calibrated at boot time?

    > +struct nios2_clockevent_dev {
    > + struct nios2_timer timer;
    > + struct clock_event_device ced;
    > + struct irqaction irqaction;
    > +};

    Why does this need its private irqaction? Timers are setup after the
    interrupt subsystem, so request_irq() is good enough.

    > +static void nios2_timer_config(struct nios2_timer *timer, unsigned long period,
    > + enum clock_event_mode mode)
    > +{
    > + u16 ctrl;
    > +
    > + /* The timer's actual period is one cycle greater than the value
    > + * stored in the period register. */
    > + if (period)
    > + period--;

    Pointless conditional. Set ce->min_delta_ticks to 1, so the core code
    will never call this with period == 0 and you can unconditionally
    decrement period.

    > +static __init void nios2_clockevent_init(struct device_node *timer)
    > +{
    > + struct nios2_clockevent_dev *ce;
    > + void __iomem *iobase;
    > + u32 freq;
    > + int irq;
    > +
    > + ce = kzalloc(sizeof(*ce), GFP_KERNEL);
    > + if (!ce)
    > + panic("Failed to allocate memory for %s\n", timer->name);

    What's the point of this allocation? You only install one of those, so
    you can really make that whole thing statically allocated and
    initialized. Or do you expect systems which use a different timer IP
    for this?

    > +static __init void nios2_clocksource_init(struct device_node *timer)
    > +{
    > + unsigned int ctrl;
    > + void __iomem *iobase;
    > + u32 freq;
    > +
    > + nios2_cs = kzalloc(sizeof(*nios2_cs), GFP_KERNEL);
    > + if (!nios2_cs)
    > + panic("Failed to allocate memory for %s\n", timer->name);

    Ditto.

    > +/*
    > + * The first timer instance will use as a clockevent. If there are two or
    > + * more instances, the second one gets used as clocksource and all
    > + * others are unused.
    > +*/
    > +static int num_called;

    This thing, horrible as it is, wants to be at least inside the
    nios2_time_init() function. It has no other scope and should go away
    after init along with the function itself.

    > +static void __init nios2_time_init(struct device_node *timer)

    Thanks,

    tglx


    \
     
     \ /
      Last update: 2014-10-25 23:21    [W:5.193 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site