Re: [RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDTtable to initialize arch timer

[Hanjun Guo]

On 2013å12æ04æ 01:04, Mark Rutland wrote:
> On Tue, Dec 03, 2013 at 04:41:30PM +0000, Hanjun Guo wrote:
> > ACPI GTDT (Generic Timer Description Table) contains information for
> > arch timer initialization, this patch use this table to probe arm timer.
> >
> > GTDT table is used for ARM/ARM64 only, please refer to chapter 5.2.24
> > of ACPI 5.0 spec for detailed inforamtion
> >
> > Signed-off-by: Amit Daniel Kachhap <amit.daniel@xxxxxxxxxxx>
> > Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
> > ---
> >   drivers/clocksource/arm_arch_timer.c |  129 ++++++++++++++++++++++++++++++----
> >   include/clocksource/arm_arch_timer.h |    7 +-
> >   2 files changed, 120 insertions(+), 16 deletions(-)
>   
> [...]
>
> > +#ifdef CONFIG_ACPI
> > +void __init arch_timer_acpi_init(void)
> > +{
> > +	struct acpi_table_gtdt *gtdt;
> > +	acpi_size tbl_size;
> > +	int trigger, polarity;
> > +	void __iomem *base = NULL;
> > +
> > +	if (acpi_disabled)
> > +		return;
> > +
> > +	if (arch_timers_present & ARCH_CP15_TIMER) {
> > +		pr_warn("arch_timer: already initialized, skipping\n");
> > +		return;
> > +	}
> > +
> > +	if (ACPI_FAILURE(acpi_get_table_with_size(ACPI_SIG_GTDT, 0,
> > +			(struct acpi_table_header **)&gtdt, &tbl_size))) {
> > +		pr_err("arch_timer: GTDT table not defined\n");
> > +		return;
> > +	}
> > +
> > +	arch_timers_present |= ARCH_CP15_TIMER;
> > +
> > +	/*
> > +	 * Get the timer frequency. Since there is no frequency info
> > +	 * in the GTDT table, so we should read it from CNTFREG register
> > +	 * or hard code here to wait for the new ACPI spec available.
> > +	 */
> If the core's CNTFRQ register does not hold the correct value, that is a
> horrendous firmware bug. The clock-frequency property in the DT is a
> horrific workaround for buggy firmware.
>
> We should not duplicate it in ACPI; people should be strongly encouraged
> to fix their firmware to do what it is supposed to do.
>
> Rely on CNTFRQ only. If it is wrong, then bail out. Let's not create a
> fertile environment for buggy firmware.

Great, this can make things much simple, and the information which contains
in ACPI GTDT table can be sufficient for timer initialization.


> > +	if (!gtdt->address) {
> > +		arch_timer_rate = arch_timer_get_cntfrq();
> > +	} else {
> > +		base = ioremap(gtdt->address, CNTFRQ);
> > +		if (!base) {
> > +			pr_warn("arch_timer: unable to map arch timer base address\n");
> > +			return;
> > +		}
> > +
> > +		arch_timer_rate = readl_relaxed(base + CNTFRQ);
> > +		iounmap(base);
> > +	}
> > +
> > +	if (!arch_timer_rate) {
> > +		/* Hard code here to set frequence ? */
> > +		pr_warn("arch_timer: Could not get frequency from GTDT table or CNTFREG\n");
> > +	}
> > +
> > +	if (gtdt->secure_pl1_interrupt) {
> > +		trigger = (gtdt->secure_pl1_flags & ACPI_GTDT_INTERRUPT_MODE) ?
> > +			ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
> > +		polarity =
> > +			(gtdt->secure_pl1_flags & ACPI_GTDT_INTERRUPT_POLARITY)
> > +			? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> > +		arch_timer_ppi[0] = acpi_register_gsi(NULL,
> > +				gtdt->secure_pl1_interrupt, trigger, polarity);
> > +	}
> This pattern looks like it can be factored out. I don't see why the
> driver needs to have such intimate knowledge of the interrrupt.
>
> > +	if (gtdt->non_secure_pl1_interrupt) {
> > +		trigger =
> > +			(gtdt->non_secure_pl1_flags & ACPI_GTDT_INTERRUPT_MODE)
> > +			? ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
> > +		polarity =
> > +		(gtdt->non_secure_pl1_flags & ACPI_GTDT_INTERRUPT_POLARITY)
> > +			? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> > +		arch_timer_ppi[1] = acpi_register_gsi(NULL,
> > +			gtdt->non_secure_pl1_interrupt, trigger, polarity);
> > +	}
> > +	if (gtdt->virtual_timer_interrupt) {
> > +		trigger = (gtdt->virtual_timer_flags & ACPI_GTDT_INTERRUPT_MODE)
> > +			? ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
> > +		polarity =
> > +		(gtdt->virtual_timer_flags & ACPI_GTDT_INTERRUPT_POLARITY)
> > +			? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> > +		arch_timer_ppi[2] = acpi_register_gsi(NULL,
> > +			gtdt->virtual_timer_interrupt, trigger, polarity);
> > +	}
> > +	if (gtdt->non_secure_pl2_interrupt) {
> > +		trigger =
> > +			(gtdt->non_secure_pl2_flags & ACPI_GTDT_INTERRUPT_MODE)
> > +			? ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
> > +		polarity =
> > +		(gtdt->non_secure_pl2_flags & ACPI_GTDT_INTERRUPT_POLARITY)
> > +			? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> > +		arch_timer_ppi[3] = acpi_register_gsi(NULL,
> > +			gtdt->non_secure_pl2_interrupt, trigger, polarity);
> > +	}
> Please factor out the interrupt parsing.

Ok, will try to factor it out in next version.

Thanks for the comments.

Hanjun



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/