On Thu, Sep 29, 2016 at 02:17:13AM +0800, [email protected] wrote:
> From: Fu Wei <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Hanjun Guo <[email protected]>
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.
On Thu, Oct 20, 2016 at 05:58:17PM +0100, Mark Rutland wrote:
> On Thu, Sep 29, 2016 at 02:17:13AM +0800, [email protected] wrote:
> > + 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.
I see I misread the spec; this is part of the common flags.
Please ignore this point; sorry for the noise.
Thanks,
Mark.
On Fri, Oct 21, 2016 at 12:14:01PM +0100, Mark Rutland wrote:
> On Thu, Oct 20, 2016 at 05:58:17PM +0100, Mark Rutland wrote:
> > On Thu, Sep 29, 2016 at 02:17:13AM +0800, [email protected] wrote:
> > > + 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.
>
> I see I misread the spec; this is part of the common flags.
>
> Please ignore this point; sorry for the noise.
Actually, I misread the spec this time around; the flag *can* differ per
interrupt for the sysreg/cp15 timer, but not for the MMIO timers where
the flag is in a common field.
So please *do* consider the above.
Thanks,
Mark.
Hi Mark,
On 21 October 2016 at 19:21, Mark Rutland <[email protected]> wrote:
> On Fri, Oct 21, 2016 at 12:14:01PM +0100, Mark Rutland wrote:
>> On Thu, Oct 20, 2016 at 05:58:17PM +0100, Mark Rutland wrote:
>> > On Thu, Sep 29, 2016 at 02:17:13AM +0800, [email protected] wrote:
>> > > + 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.
>>
>> I see I misread the spec; this is part of the common flags.
>>
>> Please ignore this point; sorry for the noise.
>
> Actually, I misread the spec this time around; the flag *can* differ per
> interrupt for the sysreg/cp15 timer, but not for the MMIO timers where
> the flag is in a common field.
>
> So please *do* consider the above.
yes , you are right , will do
Thanks :-)
>
> Thanks,
> Mark.
--
Best regards,
Fu Wei
Software Engineer
Red Hat
On 10/21/2016 07:21 PM, Mark Rutland wrote:
> On Fri, Oct 21, 2016 at 12:14:01PM +0100, Mark Rutland wrote:
>> On Thu, Oct 20, 2016 at 05:58:17PM +0100, Mark Rutland wrote:
>>> On Thu, Sep 29, 2016 at 02:17:13AM +0800, [email protected] wrote:
>>>> + 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.
>>
>> I see I misread the spec; this is part of the common flags.
>>
>> Please ignore this point; sorry for the noise.
>
> Actually, I misread the spec this time around; the flag *can* differ per
> interrupt for the sysreg/cp15 timer, but not for the MMIO timers where
> the flag is in a common field.
>
> So please *do* consider the above.
Sorry, misread the email as well...
Check the spec again and it's per interrupt flag.
Thanks
Hanjun