Re: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT

[Mark Rutland <mark.rutland@arm.com>]

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.