lkml.org 
[lkml]   [2016]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT
    On Thu, Sep 29, 2016 at 02:17:15AM +0800, fu.wei@linaro.org wrote:
    > From: Fu Wei <fu.wei@linaro.org>
    >
    > The patch refactor original memory-mapped timer init code:
    > (1) extract some subfunction for reusing some common code
    > a. get_cnttidr
    > b. is_best_frame
    > (2) move base address and irq code for arch_timer_mem to
    > arch_timer_mem_register
    >
    > Signed-off-by: Fu Wei <fu.wei@linaro.org>
    > ---
    > drivers/clocksource/arm_arch_timer.c | 159 +++++++++++++++++++++--------------
    > 1 file changed, 96 insertions(+), 63 deletions(-)
    >
    > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
    > index c7b0040..e78095f 100644
    > --- a/drivers/clocksource/arm_arch_timer.c
    > +++ b/drivers/clocksource/arm_arch_timer.c
    > @@ -57,6 +57,7 @@
    > static unsigned arch_timers_present __initdata;
    >
    > static void __iomem *arch_counter_base;
    > +static void __iomem *cntctlbase __initdata;
    >
    > struct arch_timer {
    > void __iomem *base;
    > @@ -656,15 +657,49 @@ out:
    > return err;
    > }
    >
    > -static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
    > +static int __init arch_timer_mem_register(struct device_node *np, void *frame)
    > {
    > - int ret;
    > - irq_handler_t func;
    > + struct device_node *frame_node = NULL;
    > struct arch_timer *t;
    > + void __iomem *base;
    > + irq_handler_t func;
    > + unsigned int irq;
    > + int ret;
    > +
    > + if (!frame)
    > + return -EINVAL;

    Why would we call this without a frame?

    > +
    > + if (np) {

    ... or without a node?

    > + frame_node = (struct device_node *)frame;
    > + base = of_iomap(frame_node, 0);
    > + arch_timer_detect_rate(base, np);

    ... BANG! (we check base too late, below).

    Please as Marc requested several versions ago: split the FW parsing
    (ACPI and DT) so that happens first, *then* once we have the data in a
    common format, use that to drive poking the HW, requesting IRQs, etc,
    completely independent of the source.

    In patches, do this by:

    (1) adding the data structures
    (2) splitting the existing DT probing to use them
    (3) Adding ACPI functionality atop

    > -static int __init arch_timer_mem_init(struct device_node *np)
    > +static int __init get_cnttidr(struct device_node *np, u32 *cnttidr)
    > {
    > - struct device_node *frame, *best_frame = NULL;
    > - void __iomem *cntctlbase, *base;
    > - unsigned int irq, ret = -EINVAL;
    > - u32 cnttidr;
    > + if (!cnttidr)
    > + return -EINVAL;
    > +
    > + if (np)
    > + cntctlbase = of_iomap(np, 0);
    > + else
    > + return -EINVAL;

    We want to check this for ACPI too, no?

    Thanks,
    Mark.

    \
     
     \ /
      Last update: 2016-10-21 13:33    [W:4.907 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site