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

On 06/09/16 14:25, fu.wei@linaro.org wrote:

From: Fu Wei fu.wei@linaro.org

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@linaro.org

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.