Messages in this thread | | | Date | Thu, 20 Oct 2016 17:58:17 +0100 | From | Mark Rutland <> | Subject | Re: [PATCH v14 5/9] clocksource/drivers/arm_arch_timer: Simplify ACPI support code. |
| |
On Thu, Sep 29, 2016 at 02:17:13AM +0800, fu.wei@linaro.org wrote: > From: Fu Wei <fu.wei@linaro.org> > > The patch update arm_arch_timer driver to use the function > provided by the new GTDT driver of ACPI. > By this way, arm_arch_timer.c can be simplified, and separate > all the ACPI GTDT knowledge from this timer driver. > > Signed-off-by: Fu Wei <fu.wei@linaro.org> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
This generally looks fine, but:
> + arch_timer_ppi[PHYS_SECURE_PPI] = acpi_gtdt_map_ppi(PHYS_SECURE_PPI);
As mentioned on the prior patch, I think we shouldn't bother parsing the secure interrupt, given the problem with the GSIV, and the fact that we shouldn't need it in Linux.
> + 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();
... I think we should check the flag on the relevant interrupt, though that's worth clarifying.
> > - /* 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.\n");
Why don't we log this in the code that would try to initialise the MMIO timer? We can still fail after this.
Thanks, Mark.
| |