[PATCH] xen/arm: remove check for generic timer support for arm64

[PATCH] xen/arm: remove check for generic timer support for arm64

[vijay.kilari]

From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

arm64 always supports generic timer. So check is not required
for arm64. For platforms which supports only aarch64 mode this
check always passes and panics

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 xen/arch/arm/time.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 4c3e1a6..801c130 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -138,8 +138,10 @@ int __init init_xen_time(void)
         panic("Timer: Cannot initialize platform timer");
 
     /* Check that this CPU supports the Generic Timer interface */
+#ifdef CONFIG_ARM_32
     if ( !cpu_has_gentimer )
         panic("CPU does not support the Generic Timer v1 interface");
+#endif
 
     res = dt_property_read_u32(dev, "clock-frequency", &rate);
     if ( res )
-- 
1.7.9.5

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Julien Grall]

Hi Vijay,

On 06/02/2014 09:37 AM, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> arm64 always supports generic timer. So check is not required
> for arm64. For platforms which supports only aarch64 mode this
> check always passes and panics

You should explain why it always panics... i.e On AArch-64 only
implementation, this register is RES0.

A link to the ARM ARM section might be also useful.

> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/time.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 4c3e1a6..801c130 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -138,8 +138,10 @@ int __init init_xen_time(void)
>          panic("Timer: Cannot initialize platform timer");
>  
>      /* Check that this CPU supports the Generic Timer interface */
> +#ifdef CONFIG_ARM_32

With this solution, you don't check aarch32 generic timer support on an
ARMv8 host.

I think you have to do smth like:

if ( cpu_has_aarch32 && !cpu_has_gentimer )

>      if ( !cpu_has_gentimer )
>          panic("CPU does not support the Generic Timer v1 interface");
> +#endif
>      res = dt_property_read_u32(dev, "clock-frequency", &rate);
>      if ( res )
> 

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Ian Campbell]

On Mon, 2014-06-02 at 14:07 +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> arm64 always supports generic timer. So check is not required
> for arm64. For platforms which supports only aarch64 mode this
> check always passes and panics

Ah, because the relevant feature flag/register is 32-bit only.

I'd prefer to see this done in the cpufeature header as:
        #ifdef CONFIG_ARM_32
        #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
        #else
        #define cpu_has_gentimer  (1)
        #endif
rather than adding #ifdef to the common code. Likewise for any similar
"always on for aarch64" features.

Thanks,
Ian.
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/time.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 4c3e1a6..801c130 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -138,8 +138,10 @@ int __init init_xen_time(void)
>          panic("Timer: Cannot initialize platform timer");
>  
>      /* Check that this CPU supports the Generic Timer interface */
> +#ifdef CONFIG_ARM_32
>      if ( !cpu_has_gentimer )
>          panic("CPU does not support the Generic Timer v1 interface");
> +#endif
>  
>      res = dt_property_read_u32(dev, "clock-frequency", &rate);
>      if ( res )

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Julien Grall]

On 06/02/2014 12:09 PM, Ian Campbell wrote:
> On Mon, 2014-06-02 at 14:07 +0530, vijay.kilari@gmail.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> arm64 always supports generic timer. So check is not required
>> for arm64. For platforms which supports only aarch64 mode this
>> check always passes and panics
> 
> Ah, because the relevant feature flag/register is 32-bit only.
> 
> I'd prefer to see this done in the cpufeature header as:
>         #ifdef CONFIG_ARM_32
>         #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
>         #else
>         #define cpu_has_gentimer  (1)
>         #endif
> rather than adding #ifdef to the common code. Likewise for any similar
> "always on for aarch64" features.

AFAIU, the feature flag exists on ARMv8 platform with aarch32 support.
So an ifdef may not be the correct solution here.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Ian Campbell]

On Mon, 2014-06-02 at 12:13 +0100, Julien Grall wrote:
> On 06/02/2014 12:09 PM, Ian Campbell wrote:
> > On Mon, 2014-06-02 at 14:07 +0530, vijay.kilari@gmail.com wrote:
> >> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> >>
> >> arm64 always supports generic timer. So check is not required
> >> for arm64. For platforms which supports only aarch64 mode this
> >> check always passes and panics
> > 
> > Ah, because the relevant feature flag/register is 32-bit only.
> > 
> > I'd prefer to see this done in the cpufeature header as:
> >         #ifdef CONFIG_ARM_32
> >         #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
> >         #else
> >         #define cpu_has_gentimer  (1)
> >         #endif
> > rather than adding #ifdef to the common code. Likewise for any similar
> > "always on for aarch64" features.
> 
> AFAIU, the feature flag exists on ARMv8 platform with aarch32 support.
> So an ifdef may not be the correct solution here.

The flag might exist in the AArch32 feature registers (for compat with
v7) but AIUI the feature is not actually optional on v8.

Unless you think it is within the ARMv8 spec for a processor to
implement generic timers only when running in AArch64 mode and not
expose them to AArch32 mode? I don't think that is allowed.

Ian.

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Julien Grall]

On 06/02/2014 02:03 PM, Ian Campbell wrote:
> On Mon, 2014-06-02 at 12:13 +0100, Julien Grall wrote:
>> On 06/02/2014 12:09 PM, Ian Campbell wrote:
>>> On Mon, 2014-06-02 at 14:07 +0530, vijay.kilari@gmail.com wrote:
>>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>>
>>>> arm64 always supports generic timer. So check is not required
>>>> for arm64. For platforms which supports only aarch64 mode this
>>>> check always passes and panics
>>>
>>> Ah, because the relevant feature flag/register is 32-bit only.
>>>
>>> I'd prefer to see this done in the cpufeature header as:
>>>         #ifdef CONFIG_ARM_32
>>>         #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
>>>         #else
>>>         #define cpu_has_gentimer  (1)
>>>         #endif
>>> rather than adding #ifdef to the common code. Likewise for any similar
>>> "always on for aarch64" features.
>>
>> AFAIU, the feature flag exists on ARMv8 platform with aarch32 support.
>> So an ifdef may not be the correct solution here.
> 
> The flag might exist in the AArch32 feature registers (for compat with
> v7) but AIUI the feature is not actually optional on v8.

The manual says the ARM Generic Timer is an optional extension to an
ARMv8 implementation.

> Unless you think it is within the ARMv8 spec for a processor to
> implement generic timers only when running in AArch64 mode and not
> expose them to AArch32 mode? I don't think that is allowed.

I can't find anything in the manual about the Generic Timer is required
when AArch64 is supported.

Anyway, I think it's harmless to check this bit on ARMv8 which support
aarch32.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Ian Campbell]

On Mon, 2014-06-02 at 14:23 +0100, Julien Grall wrote:
> On 06/02/2014 02:03 PM, Ian Campbell wrote:
> > On Mon, 2014-06-02 at 12:13 +0100, Julien Grall wrote:
> >> On 06/02/2014 12:09 PM, Ian Campbell wrote:
> >>> On Mon, 2014-06-02 at 14:07 +0530, vijay.kilari@gmail.com wrote:
> >>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> >>>>
> >>>> arm64 always supports generic timer. So check is not required
> >>>> for arm64. For platforms which supports only aarch64 mode this
> >>>> check always passes and panics
> >>>
> >>> Ah, because the relevant feature flag/register is 32-bit only.
> >>>
> >>> I'd prefer to see this done in the cpufeature header as:
> >>>         #ifdef CONFIG_ARM_32
> >>>         #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
> >>>         #else
> >>>         #define cpu_has_gentimer  (1)
> >>>         #endif
> >>> rather than adding #ifdef to the common code. Likewise for any similar
> >>> "always on for aarch64" features.
> >>
> >> AFAIU, the feature flag exists on ARMv8 platform with aarch32 support.
> >> So an ifdef may not be the correct solution here.
> > 
> > The flag might exist in the AArch32 feature registers (for compat with
> > v7) but AIUI the feature is not actually optional on v8.
> 
> The manual says the ARM Generic Timer is an optional extension to an
> ARMv8 implementation.

So it does. In that case cpu_has_gentimer should turn into a check of
ID_PFR1_EL1.GenTimer for arm64 builds.

Ian.

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Julien Grall]

On 06/02/2014 02:30 PM, Ian Campbell wrote:
> On Mon, 2014-06-02 at 14:23 +0100, Julien Grall wrote:
>> On 06/02/2014 02:03 PM, Ian Campbell wrote:
>>> On Mon, 2014-06-02 at 12:13 +0100, Julien Grall wrote:
>>>> On 06/02/2014 12:09 PM, Ian Campbell wrote:
>>>>> On Mon, 2014-06-02 at 14:07 +0530, vijay.kilari@gmail.com wrote:
>>>>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>>>>
>>>>>> arm64 always supports generic timer. So check is not required
>>>>>> for arm64. For platforms which supports only aarch64 mode this
>>>>>> check always passes and panics
>>>>>
>>>>> Ah, because the relevant feature flag/register is 32-bit only.
>>>>>
>>>>> I'd prefer to see this done in the cpufeature header as:
>>>>>         #ifdef CONFIG_ARM_32
>>>>>         #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
>>>>>         #else
>>>>>         #define cpu_has_gentimer  (1)
>>>>>         #endif
>>>>> rather than adding #ifdef to the common code. Likewise for any similar
>>>>> "always on for aarch64" features.
>>>>
>>>> AFAIU, the feature flag exists on ARMv8 platform with aarch32 support.
>>>> So an ifdef may not be the correct solution here.
>>>
>>> The flag might exist in the AArch32 feature registers (for compat with
>>> v7) but AIUI the feature is not actually optional on v8.
>>
>> The manual says the ARM Generic Timer is an optional extension to an
>> ARMv8 implementation.
> 
> So it does. In that case cpu_has_gentimer should turn into a check of
> ID_PFR1_EL1.GenTimer for arm64 builds.

This is already the case (without Vijay's patch). But on AArch64-only
implementation, this register is RAZ.

It looks like ID_PFR{0,1}_EL1 are only used for ARMv7 and ARMv8 which
support 32-bit.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Vijay Kilari]

On Mon, Jun 2, 2014 at 7:09 PM, Julien Grall <julien.grall@linaro.org> wrote:
> On 06/02/2014 02:30 PM, Ian Campbell wrote:
>> On Mon, 2014-06-02 at 14:23 +0100, Julien Grall wrote:
>>> On 06/02/2014 02:03 PM, Ian Campbell wrote:
>>>> On Mon, 2014-06-02 at 12:13 +0100, Julien Grall wrote:
>>>>> On 06/02/2014 12:09 PM, Ian Campbell wrote:
>>>>>> On Mon, 2014-06-02 at 14:07 +0530, vijay.kilari@gmail.com wrote:
>>>>>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>>>>>
>>>>>>> arm64 always supports generic timer. So check is not required
>>>>>>> for arm64. For platforms which supports only aarch64 mode this
>>>>>>> check always passes and panics
>>>>>>
>>>>>> Ah, because the relevant feature flag/register is 32-bit only.
>>>>>>
>>>>>> I'd prefer to see this done in the cpufeature header as:
>>>>>>         #ifdef CONFIG_ARM_32
>>>>>>         #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
>>>>>>         #else
>>>>>>         #define cpu_has_gentimer  (1)
>>>>>>         #endif
>>>>>> rather than adding #ifdef to the common code. Likewise for any similar
>>>>>> "always on for aarch64" features.
>>>>>
>>>>> AFAIU, the feature flag exists on ARMv8 platform with aarch32 support.
>>>>> So an ifdef may not be the correct solution here.
>>>>
>>>> The flag might exist in the AArch32 feature registers (for compat with
>>>> v7) but AIUI the feature is not actually optional on v8.
>>>
>>> The manual says the ARM Generic Timer is an optional extension to an
>>> ARMv8 implementation.
>>
>> So it does. In that case cpu_has_gentimer should turn into a check of
>> ID_PFR1_EL1.GenTimer for arm64 builds.
>
> This is already the case (without Vijay's patch). But on AArch64-only
> implementation, this register is RAZ.
>
> It looks like ID_PFR{0,1}_EL1 are only used for ARMv7 and ARMv8 which
> support 32-bit.

AFAIK, for arm64, irrespective of whether it supports aarch64 or
aarch32 or both,
generic timer is required. just because arm64 supports aarch32 it is
not required
to check for ID_PFR{0,1}_EL1 register

In foundation model it is working without my patch because it support
aarch32 mode
and ID_PFR{0,1}_EL1 has generic timer information. However  generic timer
is required for aarch64 mode. Hence check is not required for platform that
supports aarch32 mode

>
> Regards,
>
> --
> Julien Grall

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Ian Campbell]

On Mon, 2014-06-02 at 19:29 +0530, Vijay Kilari wrote:
> However generic timer is required for aarch64 mode.

Do you have an ARM ARM reference for that? If true it would indeed
simplify things, but I was unable to find a statement along those
lines...

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Vijay Kilari]

On Mon, Jun 2, 2014 at 7:32 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-06-02 at 19:29 +0530, Vijay Kilari wrote:
>> However generic timer is required for aarch64 mode.
>
> Do you have an ARM ARM reference for that? If true it would indeed
> simplify things, but I was unable to find a statement along those
> lines...

  Sorry, I too didn't not get any reference for this.
But in arm64 where Aarch32 is optional, why should one rely on Aarch32
feature register
if generic timer is anyway required for Aarch64

>
>
>

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Ian Campbell]

On Mon, 2014-06-02 at 19:38 +0530, Vijay Kilari wrote:
> On Mon, Jun 2, 2014 at 7:32 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-06-02 at 19:29 +0530, Vijay Kilari wrote:
> >> However generic timer is required for aarch64 mode.
> >
> > Do you have an ARM ARM reference for that? If true it would indeed
> > simplify things, but I was unable to find a statement along those
> > lines...
> 
>   Sorry, I too didn't not get any reference for this.
> But in arm64 where Aarch32 is optional, why should one rely on Aarch32
> feature register
> if generic timer is anyway required for Aarch64

We shouldn't, the bug is that we are checking the AArch32 feature
register instead of the AArch64 one in this case (see the rest of this
thread).

Ian.

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Julien Grall]

On 06/02/2014 03:11 PM, Ian Campbell wrote:
> On Mon, 2014-06-02 at 19:38 +0530, Vijay Kilari wrote:
>> On Mon, Jun 2, 2014 at 7:32 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>> On Mon, 2014-06-02 at 19:29 +0530, Vijay Kilari wrote:
>>>> However generic timer is required for aarch64 mode.
>>>
>>> Do you have an ARM ARM reference for that? If true it would indeed
>>> simplify things, but I was unable to find a statement along those
>>> lines...
>>
>>   Sorry, I too didn't not get any reference for this.
>> But in arm64 where Aarch32 is optional, why should one rely on Aarch32
>> feature register
>> if generic timer is anyway required for Aarch64
> 
> We shouldn't, the bug is that we are checking the AArch32 feature
> register instead of the AArch64 one in this case (see the rest of this
> thread).

AArch64 doesn't have a similar feature bit in AA64PFR*.

-- 
Julien Grall

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Ian Campbell]

On Mon, 2014-06-02 at 15:13 +0100, Julien Grall wrote:
> On 06/02/2014 03:11 PM, Ian Campbell wrote:
> > On Mon, 2014-06-02 at 19:38 +0530, Vijay Kilari wrote:
> >> On Mon, Jun 2, 2014 at 7:32 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >>> On Mon, 2014-06-02 at 19:29 +0530, Vijay Kilari wrote:
> >>>> However generic timer is required for aarch64 mode.
> >>>
> >>> Do you have an ARM ARM reference for that? If true it would indeed
> >>> simplify things, but I was unable to find a statement along those
> >>> lines...
> >>
> >>   Sorry, I too didn't not get any reference for this.
> >> But in arm64 where Aarch32 is optional, why should one rely on Aarch32
> >> feature register
> >> if generic timer is anyway required for Aarch64
> > 
> > We shouldn't, the bug is that we are checking the AArch32 feature
> > register instead of the AArch64 one in this case (see the rest of this
> > thread).
> 
> AArch64 doesn't have a similar feature bit in AA64PFR*.

Damn, I was looking at ID_PFR (the 32-bit version) again!

If there is no AArch64 feature bit then surely this must be a
non-optional extension. Just need to find the actual reference for that!

Ian.

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Julien Grall]

On 06/02/2014 03:02 PM, Ian Campbell wrote:
> On Mon, 2014-06-02 at 19:29 +0530, Vijay Kilari wrote:
>> However generic timer is required for aarch64 mode.
> 
> Do you have an ARM ARM reference for that? If true it would indeed
> simplify things, but I was unable to find a statement along those
> lines...

I'm wondering if this requirement comes from the SBSA.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Ian Campbell]

On Mon, 2014-06-02 at 15:10 +0100, Julien Grall wrote:
> On 06/02/2014 03:02 PM, Ian Campbell wrote:
> > On Mon, 2014-06-02 at 19:29 +0530, Vijay Kilari wrote:
> >> However generic timer is required for aarch64 mode.
> > 
> > Do you have an ARM ARM reference for that? If true it would indeed
> > simplify things, but I was unable to find a statement along those
> > lines...
> 
> I'm wondering if this requirement comes from the SBSA.

It's very possible that's where I remember it from. But Xen in general
does not require SBSA compliance, so we can't rely on that.

Ian.

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Ian Campbell]

On Mon, 2014-06-02 at 14:39 +0100, Julien Grall wrote:
> On 06/02/2014 02:30 PM, Ian Campbell wrote:
> > On Mon, 2014-06-02 at 14:23 +0100, Julien Grall wrote:
> >> On 06/02/2014 02:03 PM, Ian Campbell wrote:
> >>> On Mon, 2014-06-02 at 12:13 +0100, Julien Grall wrote:
> >>>> On 06/02/2014 12:09 PM, Ian Campbell wrote:
> >>>>> On Mon, 2014-06-02 at 14:07 +0530, vijay.kilari@gmail.com wrote:
> >>>>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> >>>>>>
> >>>>>> arm64 always supports generic timer. So check is not required
> >>>>>> for arm64. For platforms which supports only aarch64 mode this
> >>>>>> check always passes and panics
> >>>>>
> >>>>> Ah, because the relevant feature flag/register is 32-bit only.
> >>>>>
> >>>>> I'd prefer to see this done in the cpufeature header as:
> >>>>>         #ifdef CONFIG_ARM_32
> >>>>>         #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
> >>>>>         #else
> >>>>>         #define cpu_has_gentimer  (1)
> >>>>>         #endif
> >>>>> rather than adding #ifdef to the common code. Likewise for any similar
> >>>>> "always on for aarch64" features.
> >>>>
> >>>> AFAIU, the feature flag exists on ARMv8 platform with aarch32 support.
> >>>> So an ifdef may not be the correct solution here.
> >>>
> >>> The flag might exist in the AArch32 feature registers (for compat with
> >>> v7) but AIUI the feature is not actually optional on v8.
> >>
> >> The manual says the ARM Generic Timer is an optional extension to an
> >> ARMv8 implementation.
> > 
> > So it does. In that case cpu_has_gentimer should turn into a check of
> > ID_PFR1_EL1.GenTimer for arm64 builds.
> 
> This is already the case (without Vijay's patch).

cpu_has_gentimer uses boot_cpu_feature32 and is therefore only testing
for AArch32 features. It should use boot_cpu_feature64 to check for the
AArch64 feature instead.

>  But on AArch64-only
> implementation, this register is RAZ.
> 
> It looks like ID_PFR{0,1}_EL1 are only used for ARMv7 and ARMv8 which
> support 32-bit.

Yes, these registers contains AArch32 features. AArch64 features are in
ID_AA64PFR0_EL1 et al.

Ian.

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Ian Campbell]

On Mon, 2014-06-02 at 14:07 +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> arm64 always supports generic timer. So check is not required
> for arm64. For platforms which supports only aarch64 mode this
> check always passes and panics
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/time.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 4c3e1a6..801c130 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -138,8 +138,10 @@ int __init init_xen_time(void)
>          panic("Timer: Cannot initialize platform timer");
>  
>      /* Check that this CPU supports the Generic Timer interface */
> +#ifdef CONFIG_ARM_32
>      if ( !cpu_has_gentimer )
>          panic("CPU does not support the Generic Timer v1 interface");
> +#endif

Coming at this form a different angle (ignoring feature flags etc), we
have by this point already been told by the device tree that a generic
timer is present (we panic if we don't find a node). So this check is a
bit redundant (I suppose it would catch people with an incorrect DT, but
I expect we'd figure that out pretty soon even without this check).

Perhaps we should just remove this check altogether? Or the suggestion
to #define it to 1 on arm64 would also work.

Ian.

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Julien Grall]

On 06/02/2014 03:48 PM, Ian Campbell wrote:
> On Mon, 2014-06-02 at 14:07 +0530, vijay.kilari@gmail.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> arm64 always supports generic timer. So check is not required
>> for arm64. For platforms which supports only aarch64 mode this
>> check always passes and panics
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>> ---
>>  xen/arch/arm/time.c |    2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>> index 4c3e1a6..801c130 100644
>> --- a/xen/arch/arm/time.c
>> +++ b/xen/arch/arm/time.c
>> @@ -138,8 +138,10 @@ int __init init_xen_time(void)
>>          panic("Timer: Cannot initialize platform timer");
>>  
>>      /* Check that this CPU supports the Generic Timer interface */
>> +#ifdef CONFIG_ARM_32
>>      if ( !cpu_has_gentimer )
>>          panic("CPU does not support the Generic Timer v1 interface");
>> +#endif
> 
> Coming at this form a different angle (ignoring feature flags etc), we
> have by this point already been told by the device tree that a generic
> timer is present (we panic if we don't find a node). So this check is a
> bit redundant (I suppose it would catch people with an incorrect DT, but
> I expect we'd figure that out pretty soon even without this check).
> 
> Perhaps we should just remove this check altogether? Or the suggestion
> to #define it to 1 on arm64 would also work.

I though about removing this bit... but I'm not sure if ACPI will give
us the necessary information for the generic timer.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: remove check for generic timer support for arm64

[Ian Campbell]

On Mon, 2014-06-02 at 15:50 +0100, Julien Grall wrote:
> On 06/02/2014 03:48 PM, Ian Campbell wrote:
> > On Mon, 2014-06-02 at 14:07 +0530, vijay.kilari@gmail.com wrote:
> >> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> >>
> >> arm64 always supports generic timer. So check is not required
> >> for arm64. For platforms which supports only aarch64 mode this
> >> check always passes and panics
> >>
> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> >> ---
> >>  xen/arch/arm/time.c |    2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> >> index 4c3e1a6..801c130 100644
> >> --- a/xen/arch/arm/time.c
> >> +++ b/xen/arch/arm/time.c
> >> @@ -138,8 +138,10 @@ int __init init_xen_time(void)
> >>          panic("Timer: Cannot initialize platform timer");
> >>  
> >>      /* Check that this CPU supports the Generic Timer interface */
> >> +#ifdef CONFIG_ARM_32
> >>      if ( !cpu_has_gentimer )
> >>          panic("CPU does not support the Generic Timer v1 interface");
> >> +#endif
> > 
> > Coming at this form a different angle (ignoring feature flags etc), we
> > have by this point already been told by the device tree that a generic
> > timer is present (we panic if we don't find a node). So this check is a
> > bit redundant (I suppose it would catch people with an incorrect DT, but
> > I expect we'd figure that out pretty soon even without this check).
> > 
> > Perhaps we should just remove this check altogether? Or the suggestion
> > to #define it to 1 on arm64 would also work.
> 
> I though about removing this bit... but I'm not sure if ACPI will give
> us the necessary information for the generic timer.

It had better, since we need it to operate...