Messages in this thread | | | From | Fu Wei <> | Date | Wed, 26 Oct 2016 23:24:32 +0800 | Subject | Re: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT |
| |
Hi Mark,
On 21 October 2016 at 19:32, Mark Rutland <mark.rutland@arm.com> wrote: > 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?
Sorry, I just verify it , make sure frame is not NULL, Because it is a "static" function, so we do need this check?
> >> + >> + if (np) { > > ... or without a node?
For "np", for now, we just just verify it, but it is just paperation for GTDT support, Because in next patch, if np == NULL, that means we call this function from GTDT, but not DT.
> >> + 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
this patch is a preparation for GTDT support, I have splitted some functions for reusing them in next patch(GTDT support)
if np == NULL, that means we call this function from GTDT, but if np != NULL, that means we call this function from DT
> >> -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?
just like I said above, this is a preparation for GTDT support,
So please correct me if I am doing this in the wrong way, thanks :-)
> > Thanks, > Mark.
-- Best regards,
Fu Wei Software Engineer Red Hat
| |