Hi,
As a heads-up, on v4.9-rc1 I see conflicts at least against
arch/arm64/Kconfig. Luckily git am -3 seems to be able to fix that up
automatically, but this will need to be rebased before the next posting
and/or merging.
On Thu, Sep 29, 2016 at 02:17:12AM +0800, [email protected] wrote:
> +static int __init map_gt_gsi(u32 interrupt, u32 flags)
> +{
> + int trigger, polarity;
> +
> + if (!interrupt)
> + return 0;
Urgh.
Only the secure interrupt (which we do not need) is optional in this
manner, and (hilariously), zero appears to also be a valid GSIV, per
figure 5-24 in the ACPI 6.1 spec.
So, I think that:
(a) we should not bother parsing the secure interrupt
(b) we should drop the check above
(c) we should report the spec issue to the ASWG
> +/*
> + * acpi_gtdt_c3stop - got c3stop info from GTDT
> + *
> + * Returns 1 if the timer is powered in deep idle state, 0 otherwise.
> + */
> +bool __init acpi_gtdt_c3stop(void)
> +{
> + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt;
> +
> + return !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON);
> +}
It looks like this can differ per interrupt. Shouldn't we check the
appropriate one?
> +int __init acpi_gtdt_init(struct acpi_table_header *table)
> +{
> + void *start;
> + struct acpi_table_gtdt *gtdt;
> +
> + gtdt = container_of(table, struct acpi_table_gtdt, header);
> +
> + acpi_gtdt_desc.gtdt = gtdt;
> + acpi_gtdt_desc.gtdt_end = (void *)table + table->length;
> +
> + if (table->revision < 2) {
> + pr_debug("Revision:%d doesn't support Platform Timers.\n",
> + table->revision);
> + return 0;
> + }
> +
> + if (!gtdt->platform_timer_count) {
> + pr_debug("No Platform Timer.\n");
> + return 0;
> + }
> +
> + start = (void *)gtdt + gtdt->platform_timer_offset;
> + 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;
> + }
> + acpi_gtdt_desc.platform_timer_start = start;
> +
> + return gtdt->platform_timer_count;
> +}
This is never used as anything other than a status code.
Just return zero; we haven't parsed the platform timers themselves at
this point anyway.
Thanks,
Mark.
Hi Mark,
On 21 October 2016 at 00:37, Mark Rutland <[email protected]> wrote:
> Hi,
>
> As a heads-up, on v4.9-rc1 I see conflicts at least against
> arch/arm64/Kconfig. Luckily git am -3 seems to be able to fix that up
> automatically, but this will need to be rebased before the next posting
> and/or merging.
>
> On Thu, Sep 29, 2016 at 02:17:12AM +0800, [email protected] wrote:
>> +static int __init map_gt_gsi(u32 interrupt, u32 flags)
>> +{
>> + int trigger, polarity;
>> +
>> + if (!interrupt)
>> + return 0;
>
> Urgh.
>
> Only the secure interrupt (which we do not need) is optional in this
> manner, and (hilariously), zero appears to also be a valid GSIV, per
> figure 5-24 in the ACPI 6.1 spec.
>
> So, I think that:
>
> (a) we should not bother parsing the secure interrupt
If I understand correctly, from this point of view, kernel don't
handle the secure interrupt.
But the current arm_arch_timer driver still enable/disable/request
PHYS_SECURE_PPI
with PHYS_NONSECURE_PPI.
That means we still need to parse the secure interrupt.
Please correct me, if I misunderstand something? :-)
> (b) we should drop the check above
yes, if zero is a valid GSIV, this makes sense.
> (c) we should report the spec issue to the ASWG
>
>> +/*
>> + * acpi_gtdt_c3stop - got c3stop info from GTDT
>> + *
>> + * Returns 1 if the timer is powered in deep idle state, 0 otherwise.
>> + */
>> +bool __init acpi_gtdt_c3stop(void)
>> +{
>> + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt;
>> +
>> + return !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON);
>> +}
>
> It looks like this can differ per interrupt. Shouldn't we check the
> appropriate one?
yes, I think you are right.
>
>> +int __init acpi_gtdt_init(struct acpi_table_header *table)
>> +{
>> + void *start;
>> + struct acpi_table_gtdt *gtdt;
>> +
>> + gtdt = container_of(table, struct acpi_table_gtdt, header);
>> +
>> + acpi_gtdt_desc.gtdt = gtdt;
>> + acpi_gtdt_desc.gtdt_end = (void *)table + table->length;
>> +
>> + if (table->revision < 2) {
>> + pr_debug("Revision:%d doesn't support Platform Timers.\n",
>> + table->revision);
>> + return 0;
>> + }
>> +
>> + if (!gtdt->platform_timer_count) {
>> + pr_debug("No Platform Timer.\n");
>> + return 0;
>> + }
>> +
>> + start = (void *)gtdt + gtdt->platform_timer_offset;
>> + 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;
>> + }
>> + acpi_gtdt_desc.platform_timer_start = start;
>> +
>> + return gtdt->platform_timer_count;
>> +}
>
> This is never used as anything other than a status code.
>
> Just return zero; we haven't parsed the platform timers themselves at
> this point anyway.
Sorry, in my driver, I use this return value to inform driver
negative number : error
0 : we don't have platform timer in GTDT table.
positive number: the number of platform timer we have in GTDT table.
please correct me, if I am doing it in wrong way. Thanks :-)
>
> Thanks,
> Mark.
--
Best regards,
Fu Wei
Software Engineer
Red Hat
On 26/10/16 12:10, Fu Wei wrote:
> Hi Mark,
>
> On 21 October 2016 at 00:37, Mark Rutland <[email protected]> wrote:
>> Hi,
>>
>> As a heads-up, on v4.9-rc1 I see conflicts at least against
>> arch/arm64/Kconfig. Luckily git am -3 seems to be able to fix that up
>> automatically, but this will need to be rebased before the next posting
>> and/or merging.
>>
>> On Thu, Sep 29, 2016 at 02:17:12AM +0800, [email protected] wrote:
>>> +static int __init map_gt_gsi(u32 interrupt, u32 flags)
>>> +{
>>> + int trigger, polarity;
>>> +
>>> + if (!interrupt)
>>> + return 0;
>>
>> Urgh.
>>
>> Only the secure interrupt (which we do not need) is optional in this
>> manner, and (hilariously), zero appears to also be a valid GSIV, per
>> figure 5-24 in the ACPI 6.1 spec.
>>
>> So, I think that:
>>
>> (a) we should not bother parsing the secure interrupt
>
> If I understand correctly, from this point of view, kernel don't
> handle the secure interrupt.
> But the current arm_arch_timer driver still enable/disable/request
> PHYS_SECURE_PPI
> with PHYS_NONSECURE_PPI.
> That means we still need to parse the secure interrupt.
> Please correct me, if I misunderstand something? :-)
That's because we can use the per-cpu timer when 32bit Linux is running
on the secure side (and we cannot distinguish between secure and
non-secure at runtime). ACPI is 64bit only, and Linux on 64bit isn't
supported on the secure side, so only registering the non-secure timer
is perfectly acceptable.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Hi Marc,
On 26 October 2016 at 20:11, Marc Zyngier <[email protected]> wrote:
> On 26/10/16 12:10, Fu Wei wrote:
>> Hi Mark,
>>
>> On 21 October 2016 at 00:37, Mark Rutland <[email protected]> wrote:
>>> Hi,
>>>
>>> As a heads-up, on v4.9-rc1 I see conflicts at least against
>>> arch/arm64/Kconfig. Luckily git am -3 seems to be able to fix that up
>>> automatically, but this will need to be rebased before the next posting
>>> and/or merging.
>>>
>>> On Thu, Sep 29, 2016 at 02:17:12AM +0800, [email protected] wrote:
>>>> +static int __init map_gt_gsi(u32 interrupt, u32 flags)
>>>> +{
>>>> + int trigger, polarity;
>>>> +
>>>> + if (!interrupt)
>>>> + return 0;
>>>
>>> Urgh.
>>>
>>> Only the secure interrupt (which we do not need) is optional in this
>>> manner, and (hilariously), zero appears to also be a valid GSIV, per
>>> figure 5-24 in the ACPI 6.1 spec.
>>>
>>> So, I think that:
>>>
>>> (a) we should not bother parsing the secure interrupt
>>
>> If I understand correctly, from this point of view, kernel don't
>> handle the secure interrupt.
>> But the current arm_arch_timer driver still enable/disable/request
>> PHYS_SECURE_PPI
>> with PHYS_NONSECURE_PPI.
>> That means we still need to parse the secure interrupt.
>> Please correct me, if I misunderstand something? :-)
>
> That's because we can use the per-cpu timer when 32bit Linux is running
> on the secure side (and we cannot distinguish between secure and
> non-secure at runtime). ACPI is 64bit only, and Linux on 64bit isn't
> supported on the secure side, so only registering the non-secure timer
> is perfectly acceptable.
Great thanks for your explanation :-)
So we just don't need to fill arch_timer_ppi[PHYS_SECURE_PPI] , just skip it.
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
--
Best regards,
Fu Wei
Software Engineer
Red Hat
On 10/26/2016 07:10 PM, Fu Wei wrote:
> Hi Mark,
>
> On 21 October 2016 at 00:37, Mark Rutland <[email protected]> wrote:
>> Hi,
>>
>> As a heads-up, on v4.9-rc1 I see conflicts at least against
>> arch/arm64/Kconfig. Luckily git am -3 seems to be able to fix that up
>> automatically, but this will need to be rebased before the next posting
>> and/or merging.
>>
>> On Thu, Sep 29, 2016 at 02:17:12AM +0800, [email protected] wrote:
>>> +static int __init map_gt_gsi(u32 interrupt, u32 flags)
>>> +{
>>> + int trigger, polarity;
>>> +
>>> + if (!interrupt)
>>> + return 0;
>>
>> Urgh.
>>
>> Only the secure interrupt (which we do not need) is optional in this
>> manner, and (hilariously), zero appears to also be a valid GSIV, per
>> figure 5-24 in the ACPI 6.1 spec.
>>
>> So, I think that:
>>
>> (a) we should not bother parsing the secure interrupt
>
> If I understand correctly, from this point of view, kernel don't
> handle the secure interrupt.
> But the current arm_arch_timer driver still enable/disable/request
> PHYS_SECURE_PPI
> with PHYS_NONSECURE_PPI.
> That means we still need to parse the secure interrupt.
> Please correct me, if I misunderstand something? :-)
>
>> (b) we should drop the check above
>
> yes, if zero is a valid GSIV, this makes sense.
>
>> (c) we should report the spec issue to the ASWG
>>
>>> +/*
>>> + * acpi_gtdt_c3stop - got c3stop info from GTDT
>>> + *
>>> + * Returns 1 if the timer is powered in deep idle state, 0 otherwise.
>>> + */
>>> +bool __init acpi_gtdt_c3stop(void)
>>> +{
>>> + struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt;
>>> +
>>> + return !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON);
>>> +}
>>
>> It looks like this can differ per interrupt. Shouldn't we check the
>> appropriate one?
>
> yes, I think you are right.
I think Mark already clarified this it's a global flag which defined
in the spec, and we don't need to update it.
Thanks
Hanjun
Hi Mark,
Sorry for the late reply.
On 10/21/2016 12:37 AM, Mark Rutland wrote:
> Hi,
>
> As a heads-up, on v4.9-rc1 I see conflicts at least against
> arch/arm64/Kconfig. Luckily git am -3 seems to be able to fix that up
> automatically, but this will need to be rebased before the next posting
> and/or merging.
>
> On Thu, Sep 29, 2016 at 02:17:12AM +0800, [email protected] wrote:
>> +static int __init map_gt_gsi(u32 interrupt, u32 flags)
>> +{
>> + int trigger, polarity;
>> +
>> + if (!interrupt)
>> + return 0;
>
> Urgh.
>
> Only the secure interrupt (which we do not need) is optional in this
> manner, and (hilariously), zero appears to also be a valid GSIV, per
> figure 5-24 in the ACPI 6.1 spec.
>
> So, I think that:
>
> (a) we should not bother parsing the secure interrupt
> (b) we should drop the check above
> (c) we should report the spec issue to the ASWG
Sorry, I willing to do that, but I need to figure out the issue here.
What kind of issue in detail? do you mean that zero should not be valid
for arch timer interrupts?
Thanks
Hanjun
On 11/11/2016 09:46 PM, Hanjun Guo wrote:
> Hi Mark,
>
> Sorry for the late reply.
>
> On 10/21/2016 12:37 AM, Mark Rutland wrote:
>> Hi,
>>
>> As a heads-up, on v4.9-rc1 I see conflicts at least against
>> arch/arm64/Kconfig. Luckily git am -3 seems to be able to fix that up
>> automatically, but this will need to be rebased before the next posting
>> and/or merging.
>>
>> On Thu, Sep 29, 2016 at 02:17:12AM +0800, [email protected] wrote:
>>> +static int __init map_gt_gsi(u32 interrupt, u32 flags)
>>> +{
>>> + int trigger, polarity;
>>> +
>>> + if (!interrupt)
>>> + return 0;
>>
>> Urgh.
>>
>> Only the secure interrupt (which we do not need) is optional in this
>> manner, and (hilariously), zero appears to also be a valid GSIV, per
>> figure 5-24 in the ACPI 6.1 spec.
>>
>> So, I think that:
>>
>> (a) we should not bother parsing the secure interrupt
>> (b) we should drop the check above
>> (c) we should report the spec issue to the ASWG
>
> Sorry, I willing to do that, but I need to figure out the issue here.
> What kind of issue in detail? do you mean that zero should not be valid
> for arch timer interrupts?
OK, I think you are referring to "we don't need the secure interrupt",
correct me if I'm wrong (still in jet lag...).
Thanks
Hanjun
On Fri, Nov 11, 2016 at 09:46:29PM +0800, Hanjun Guo wrote:
> On 10/21/2016 12:37 AM, Mark Rutland wrote:
> >On Thu, Sep 29, 2016 at 02:17:12AM +0800, [email protected] wrote:
> >>+static int __init map_gt_gsi(u32 interrupt, u32 flags)
> >>+{
> >>+ int trigger, polarity;
> >>+
> >>+ if (!interrupt)
> >>+ return 0;
> >
> >Urgh.
> >
> >Only the secure interrupt (which we do not need) is optional in this
> >manner, and (hilariously), zero appears to also be a valid GSIV, per
> >figure 5-24 in the ACPI 6.1 spec.
> >
> >So, I think that:
> >
> >(a) we should not bother parsing the secure interrupt
> >(b) we should drop the check above
> >(c) we should report the spec issue to the ASWG
>
> Sorry, I willing to do that, but I need to figure out the issue here.
> What kind of issue in detail? do you mean that zero should not be valid
> for arch timer interrupts?
As above, zero is a valid GSIV, and is valid for the non-secure timer
interrupts. The check is wrong for non-secure interrupts.
We can ignore the secure timer interrupt since it's irrelevant to us,
and remove the check.
Regardless, the spec is inconsistent w.r.t. the secure interrupt being
zero if not present, since zero is a valid GSIV. That should be reported
to the ASWG.
Thanks,
Mark.