| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <529F3B4C.1080400@linaro.org>
Date: Wed, 04 Dec 2013 22:25:16 +0800
From: Hanjun Guo <hanjun.guo@...aro.org>
To: Mark Rutland <mark.rutland@....com>
CC: "Rafael J. Wysocki" <rjw@...ysocki.net>,
Catalin Marinas <Catalin.Marinas@....com>,
Will Deacon <Will.Deacon@....com>,
Russell King - ARM Linux <linux@....linux.org.uk>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"grant.likely@...aro.org" <grant.likely@...aro.org>,
Matthew Garrett <mjg59@...f.ucam.org>,
Olof Johansson <olof@...om.net>,
Linus Walleij <linus.walleij@...aro.org>,
Bjorn Helgaas <bhelgaas@...gle.com>,
"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
Jon Masters <jonathan@...masters.org>,
"patches@...aro.org" <patches@...aro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linaro-kernel@...ts.linaro.org" <linaro-kernel@...ts.linaro.org>,
"linaro-acpi@...ts.linaro.org" <linaro-acpi@...ts.linaro.org>,
Amit Daniel Kachhap <amit.daniel@...sung.com>
Subject: Re: [RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT
table to initialize arch timer
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@...sung.com>
>> Signed-off-by: Hanjun Guo <hanjun.guo@...aro.org>
>> ---
>> 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 **)>dt, &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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists