| Message ID | 1473168352-5156-6-git-send-email-fu.wei@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 6 Sep 2016, fu.wei@linaro.org wrote: > + if (timer_count < 0) > + pr_err("Failed to get platform timer info, skipping.\n"); So this prints something about skipping. But then it continues as if nothing went wrong. That's either wrong or confusing or both. > - arch_timer_init(); > - return 0; > + return arch_timer_init(); Thanks, tglx
Hi Thomas On 6 September 2016 at 22:36, Thomas Gleixner <tglx@linutronix.de> wrote: > On Tue, 6 Sep 2016, fu.wei@linaro.org wrote: >> + if (timer_count < 0) >> + pr_err("Failed to get platform timer info, skipping.\n"); > > So this prints something about skipping. But then it continues as if > nothing went wrong. That's either wrong or confusing or both. yes, you are right, this info is confusing. maybe we just delete the "skipping" ? “timer_count < 0” is caused by some firmware bug, in gtdt.c: ---- int __init acpi_gtdt_init(struct acpi_table_header *table) { ...... if (start < (void *)table + sizeof(struct acpi_table_gtdt)) { pr_err(FW_BUG "Failed to retrieve timer info from firmware: invalid data.\n"); return -EINVAL; ...... } ---- But in this situation( without platform timers ), system still can work. So I thing we just need to print a error. > >> - arch_timer_init(); >> - return 0; >> + return arch_timer_init(); > > Thanks, > > tglx
Hi Thomas, Daniel, For these arm_arch_timer patches, do you have any other suggestion or comment? I have deleted "skipping" in the error message. I have prepared v12 (rebase to rc6 and on the top of IORT v11), should I send it now (if you are OK with my arm_arch_timer patches ), or anything I can do to improve this patchset ? Thanks. On 7 September 2016 at 17:23, Fu Wei <fu.wei@linaro.org> wrote: > Hi Thomas > > On 6 September 2016 at 22:36, Thomas Gleixner <tglx@linutronix.de> wrote: >> On Tue, 6 Sep 2016, fu.wei@linaro.org wrote: >>> + if (timer_count < 0) >>> + pr_err("Failed to get platform timer info, skipping.\n"); >> >> So this prints something about skipping. But then it continues as if >> nothing went wrong. That's either wrong or confusing or both. > > yes, you are right, this info is confusing. > maybe we just delete the "skipping" ? > > “timer_count < 0” is caused by some firmware bug, in gtdt.c: > ---- > int __init acpi_gtdt_init(struct acpi_table_header *table) > { > ...... > if (start < (void *)table + sizeof(struct acpi_table_gtdt)) { > pr_err(FW_BUG "Failed to retrieve timer info from firmware: > invalid data.\n"); > return -EINVAL; > ...... > } > ---- > > But in this situation( without platform timers ), system still can work. > So I thing we just need to print a error. > >> >>> - arch_timer_init(); >>> - return 0; >>> + return arch_timer_init(); >> >> Thanks, >> >> tglx > > > > -- > Best regards, > > Fu Wei > Software Engineer > Red Hat
Hi Mark, Marc, Sorry for missing you in the cc list Do you have any suggestion for the arm_arch_timer patches? Could you help me to review these patches ? Great thanks ! On 13 September 2016 at 17:22, Fu Wei <fu.wei@linaro.org> wrote: > Hi Thomas, Daniel, > > For these arm_arch_timer patches, do you have any other suggestion or comment? > I have deleted "skipping" in the error message. > > I have prepared v12 (rebase to rc6 and on the top of IORT v11), > should I send it now (if you are OK with my arm_arch_timer patches ), > or anything I can do to improve this patchset ? > > Thanks. > > On 7 September 2016 at 17:23, Fu Wei <fu.wei@linaro.org> wrote: >> Hi Thomas >> >> On 6 September 2016 at 22:36, Thomas Gleixner <tglx@linutronix.de> wrote: >>> On Tue, 6 Sep 2016, fu.wei@linaro.org wrote: >>>> + if (timer_count < 0) >>>> + pr_err("Failed to get platform timer info, skipping.\n"); >>> >>> So this prints something about skipping. But then it continues as if >>> nothing went wrong. That's either wrong or confusing or both. >> >> yes, you are right, this info is confusing. >> maybe we just delete the "skipping" ? >> >> “timer_count < 0” is caused by some firmware bug, in gtdt.c: >> ---- >> int __init acpi_gtdt_init(struct acpi_table_header *table) >> { >> ...... >> if (start < (void *)table + sizeof(struct acpi_table_gtdt)) { >> pr_err(FW_BUG "Failed to retrieve timer info from firmware: >> invalid data.\n"); >> return -EINVAL; >> ...... >> } >> ---- >> >> But in this situation( without platform timers ), system still can work. >> So I thing we just need to print a error. >> >>> >>>> - arch_timer_init(); >>>> - return 0; >>>> + return arch_timer_init(); >>> >>> Thanks, >>> >>> tglx >> >> >> >> -- >> Best regards, >> >> Fu Wei >> Software Engineer >> Red Hat > > > > -- > Best regards, > > Fu Wei > Software Engineer > Red Hat
Fu Wei wrote: > I have prepared v12 (rebase to rc6 and on the top of IORT v11), > should I send it now Yes. Please don't wait to release new versions of your patches. Time is running out to get these into 4.9.
Hi Timur On 09/13/2016 07:38 PM, Timur Tabi wrote: > Fu Wei wrote: >> I have prepared v12 (rebase to rc6 and on the top of IORT v11), >> should I send it now > > Yes. > > Please don't wait to release new versions of your patches. Time is running out to get these into 4.9. > yes, v12 is posted working on v13(improving memory-mapped timer code following Marc's suggestion)
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index b58d259..a63bf12 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -8,6 +8,7 @@ config CLKSRC_OF config CLKSRC_ACPI bool select CLKSRC_PROBE + select ACPI_GTDT if ARM64 config CLKSRC_PROBE bool diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 7d48349..a01cf22 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -887,61 +887,35 @@ out: CLOCKSOURCE_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem", arch_timer_mem_init); -#ifdef CONFIG_ACPI -static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags) -{ - int trigger, polarity; - - if (!interrupt) - return 0; - - trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE - : ACPI_LEVEL_SENSITIVE; - - polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW - : ACPI_ACTIVE_HIGH; - - return acpi_register_gsi(NULL, interrupt, trigger, polarity); -} - +#ifdef CONFIG_ACPI_GTDT /* Initialize per-processor generic timer */ static int __init arch_timer_acpi_init(struct acpi_table_header *table) { - struct acpi_table_gtdt *gtdt; + int timer_count; if (arch_timers_present & ARCH_CP15_TIMER) { pr_warn("already initialized, skipping\n"); return -EINVAL; } - gtdt = container_of(table, struct acpi_table_gtdt, header); - arch_timers_present |= ARCH_CP15_TIMER; - arch_timer_ppi[PHYS_SECURE_PPI] = - map_generic_timer_interrupt(gtdt->secure_el1_interrupt, - gtdt->secure_el1_flags); - - arch_timer_ppi[PHYS_NONSECURE_PPI] = - map_generic_timer_interrupt(gtdt->non_secure_el1_interrupt, - gtdt->non_secure_el1_flags); + timer_count = acpi_gtdt_init(table); - arch_timer_ppi[VIRT_PPI] = - map_generic_timer_interrupt(gtdt->virtual_timer_interrupt, - gtdt->virtual_timer_flags); - - arch_timer_ppi[HYP_PPI] = - map_generic_timer_interrupt(gtdt->non_secure_el2_interrupt, - gtdt->non_secure_el2_flags); + arch_timer_ppi[PHYS_SECURE_PPI] = acpi_gtdt_map_ppi(PHYS_SECURE_PPI); + arch_timer_ppi[PHYS_NONSECURE_PPI] = acpi_gtdt_map_ppi(PHYS_NONSECURE_PPI); + arch_timer_ppi[VIRT_PPI] = acpi_gtdt_map_ppi(VIRT_PPI); + arch_timer_ppi[HYP_PPI] = acpi_gtdt_map_ppi(HYP_PPI); + /* Always-on capability */ + arch_timer_c3stop = acpi_gtdt_c3stop(); /* Get the frequency from CNTFRQ */ arch_timer_detect_rate(NULL, NULL); - /* Always-on capability */ - arch_timer_c3stop = !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON); + if (timer_count < 0) + pr_err("Failed to get platform timer info, skipping.\n"); - arch_timer_init(); - return 0; + return arch_timer_init(); } CLOCKSOURCE_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT, arch_timer_acpi_init); #endif