Re: [PATCH v11 7/8] clocksource/drivers/arm_arch_timer: Add GTDT support for memory-mapped timer

[Marc Zyngier]

On 06/09/16 14:25, fu.wei@xxxxxxxxxx wrote:
> From: Fu Wei <fu.wei@xxxxxxxxxx>
> 
> The patch add memory-mapped timer register support by using the information
> provided by the new GTDT driver of ACPI.
> 
> Signed-off-by: Fu Wei <fu.wei@xxxxxxxxxx>
> ---
>  drivers/clocksource/arm_arch_timer.c | 127 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 124 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index a01cf22..c80a2f2 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -888,7 +888,128 @@ CLOCKSOURCE_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem",
>  		       arch_timer_mem_init);
>  
>  #ifdef CONFIG_ACPI_GTDT
> -/* Initialize per-processor generic timer */
> +static struct gt_timer_data __init *arch_timer_mem_get_timer(
> +						struct gt_block_data *gt_blocks)
> +{
> +	struct gt_block_data *gt_block = gt_blocks;
> +	struct gt_timer_data *best_frame = NULL;
> +	void __iomem *cntctlbase;
> +	u32 cnttidr;
> +	int i;
> +
> +	/*
> +	 * According to ARMv8 Architecture Reference Manual(ARM),
> +	 * the size of CNTCTLBase frame of memory-mapped timer
> +	 * is SZ_4K(Offset 0x000 â 0xFFF).
> +	 */
> +	cntctlbase = ioremap(gt_block->cntctlbase_phy, SZ_4K);
> +	if (!cntctlbase) {
> +		pr_err("Failed to map mem timer control frame base address\n");
> +		return NULL;
> +	}
> +	cnttidr = readl_relaxed(cntctlbase + CNTTIDR);
> +
> +	/*
> +	 * Try to find a virtual capable frame. Otherwise fall back to a
> +	 * physical capable frame.
> +	 */
> +	for (i = 0; i < gt_block->timer_count; i++) {
> +		int n;
> +		u32 cntacr;
> +
> +		n = gt_block->timer[i].frame_nr;
> +
> +		/* Try enabling everything, and see what sticks */
> +		cntacr = CNTACR_RFRQ | CNTACR_RWPT | CNTACR_RPCT |
> +			 CNTACR_RWVT | CNTACR_RVOFF | CNTACR_RVCT;
> +		writel_relaxed(cntacr, cntctlbase + CNTACR(n));
> +		cntacr = readl_relaxed(cntctlbase + CNTACR(n));
> +
> +		if ((cnttidr & CNTTIDR_VIRT(n)) &&
> +		    !(~cntacr & (CNTACR_RWVT | CNTACR_RVCT))) {
> +			best_frame = &gt_block->timer[i];
> +			arch_timer_mem_use_virtual = true;
> +			break;
> +		}
> +
> +		if (~cntacr & (CNTACR_RWPT | CNTACR_RPCT))
> +			continue;
> +
> +		best_frame = &gt_block->timer[i];
> +	}
> +	iounmap(cntctlbase);

So you're duplicating the code that is already in arch_timer_mem_init().
I don't think that's a good idea. Please change the existing code to a
set of functions that can be used in a firmware agnostic way, and plug
your ACPI code onto in.

Look at what we've done for the GIC: Firmware-specific code that decode
their respective tables, stash that information in a
firmware-independent way if required, and call into into common code.

> +
> +	return best_frame;
> +}
> +
> +static int __init arch_timer_mem_acpi_init(size_t timer_count)
> +{
> +	struct gt_block_data *gt_blocks;
> +	struct gt_timer_data *gt_timer;
> +	void __iomem *timer_cntbase;
> +	int ret = -EINVAL;
> +	int timer_irq;
> +
> +	/*
> +	 * If we don't have any Platform Timer Structures, just return.
> +	 */
> +	if (!timer_count)
> +		return 0;
> +
> +	/*
> +	 * before really check all the Platform Timer Structures,
> +	 * we assume they are GT block, and allocate memory for them.
> +	 * We will free these memory once we finish the initialization.
> +	 */
> +	gt_blocks = kcalloc(timer_count, sizeof(*gt_blocks), GFP_KERNEL);
> +	if (!gt_blocks)
> +		return -ENOMEM;
> +
> +	if (gtdt_arch_timer_mem_init(gt_blocks) > 0) {
> +		gt_timer = arch_timer_mem_get_timer(gt_blocks);
> +		if (!gt_timer) {
> +			pr_err("Failed to get mem timer info.\n");
> +			goto error;
> +		}
> +
> +		if (arch_timer_mem_use_virtual)
> +			timer_irq = gt_timer->virtual_irq;
> +		else
> +			timer_irq = gt_timer->irq;
> +		if (!timer_irq) {
> +			pr_err("Failed to get %s irq for mem timer.",
> +			       arch_timer_mem_use_virtual ? "virt" : "phys");
> +			goto error;
> +		}
> +
> +		/*
> +		 * According to ARMv8 Architecture Reference Manual(ARM),
> +		 * the size of CNTBaseN frames of memory-mapped timer
> +		 * is SZ_4K(Offset 0x000 â 0xFFF).
> +		 */
> +		timer_cntbase = ioremap(gt_timer->cntbase_phy, SZ_4K);
> +		if (!timer_cntbase) {
> +			pr_err("Failed to map mem timer base address.\n");
> +			goto error;
> +		}
> +
> +		ret = arch_timer_mem_register(timer_cntbase, timer_irq);
> +		if (ret) {
> +			iounmap(timer_cntbase);
> +			pr_err("Failed to register mem timer.\n");
> +			goto error;
> +		}
> +
> +		arch_counter_base = timer_cntbase;
> +		arch_timers_present |= ARCH_MEM_TIMER;
> +	}

Same here. A lot of that is a duplication of the DT code.

> +
> +error:
> +	kfree(gt_blocks);
> +	return ret;
> +}
> +
> +/* Initialize per-processor generic timer and memory-mapped timer(if present) */
>  static int __init arch_timer_acpi_init(struct acpi_table_header *table)
>  {
>  	int timer_count;
> @@ -912,8 +1033,8 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
>  	/* Get the frequency from CNTFRQ */
>  	arch_timer_detect_rate(NULL, NULL);
>  
> -	if (timer_count < 0)
> -		pr_err("Failed to get platform timer info, skipping.\n");
> +	if (timer_count < 0 || arch_timer_mem_acpi_init((size_t)timer_count))
> +		pr_err("Failed to initialize memory-mapped timer, skipping.\n");
>  
>  	return arch_timer_init();
>  }
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...