[PATCH] arm/timer: fix panic when booting with DT

[PATCH] arm/timer: fix panic when booting with DT

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

While to support ACPI, patch "arm/acpi: Parse GTDT to initialize timer"
refactors the functions preinit_xen_time and init_xen_time. But it
wrongly moves the platform_get_irq from init_xen_time to
preinit_dt_xen_time and this will cause booting failure.

So move platform_get_irq back to init_xen_time to fix it.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 xen/arch/arm/time.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 5f8f974..66a4520 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -119,7 +119,6 @@ static void __init preinit_dt_xen_time(void)
     };
     int res;
     u32 rate;
-    unsigned int i;
 
     timer = dt_find_matching_node(NULL, timer_ids);
     if ( !timer )
@@ -133,16 +132,6 @@ static void __init preinit_dt_xen_time(void)
         cpu_khz = rate / 1000;
         timer_dt_clock_frequency = rate;
     }
-
-    /* Retrieve all IRQs for the timer */
-    for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
-    {
-        res = platform_get_irq(timer, i);
-
-        if ( res < 0 )
-            panic("Timer: Unable to retrieve IRQ %u from the device tree", i);
-        timer_irq[i] = res;
-    }
 }
 
 void __init preinit_xen_time(void)
@@ -168,6 +157,22 @@ void __init preinit_xen_time(void)
 /* Set up the timer on the boot CPU (late init function) */
 int __init init_xen_time(void)
 {
+    int res;
+    unsigned int i;
+
+    if ( acpi_disabled )
+    {
+        /* Retrieve all IRQs for the timer */
+        for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
+        {
+            res = platform_get_irq(timer, i);
+
+            if ( res < 0 )
+                panic("Timer: Unable to retrieve IRQ %u from the device tree", i);
+            timer_irq[i] = res;
+        }
+    }
+
     /* Check that this CPU supports the Generic Timer interface */
     if ( !cpu_has_gentimer )
         panic("CPU does not support the Generic Timer v1 interface");
-- 
2.0.4



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] arm/timer: fix panic when booting with DT

[Stefano Stabellini]

On Thu, 3 Mar 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> While to support ACPI, patch "arm/acpi: Parse GTDT to initialize timer"
> refactors the functions preinit_xen_time and init_xen_time. But it
> wrongly moves the platform_get_irq from init_xen_time to
> preinit_dt_xen_time and this will cause booting failure.
> 
> So move platform_get_irq back to init_xen_time to fix it.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>

Shannon,
thank you for fixing the issue so quickly, very appreciated!


>  xen/arch/arm/time.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 5f8f974..66a4520 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -119,7 +119,6 @@ static void __init preinit_dt_xen_time(void)
>      };
>      int res;
>      u32 rate;
> -    unsigned int i;
>  
>      timer = dt_find_matching_node(NULL, timer_ids);
>      if ( !timer )
> @@ -133,16 +132,6 @@ static void __init preinit_dt_xen_time(void)
>          cpu_khz = rate / 1000;
>          timer_dt_clock_frequency = rate;
>      }
> -
> -    /* Retrieve all IRQs for the timer */
> -    for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
> -    {
> -        res = platform_get_irq(timer, i);
> -
> -        if ( res < 0 )
> -            panic("Timer: Unable to retrieve IRQ %u from the device tree", i);
> -        timer_irq[i] = res;
> -    }
>  }
>  
>  void __init preinit_xen_time(void)
> @@ -168,6 +157,22 @@ void __init preinit_xen_time(void)
>  /* Set up the timer on the boot CPU (late init function) */
>  int __init init_xen_time(void)
>  {
> +    int res;
> +    unsigned int i;
> +
> +    if ( acpi_disabled )
> +    {
> +        /* Retrieve all IRQs for the timer */
> +        for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
> +        {
> +            res = platform_get_irq(timer, i);
> +
> +            if ( res < 0 )
> +                panic("Timer: Unable to retrieve IRQ %u from the device tree", i);
> +            timer_irq[i] = res;
> +        }
> +    }

Could you please introduce a small little init_dt_xen_time function and
call that instead from here?  Thanks!



>      /* Check that this CPU supports the Generic Timer interface */
>      if ( !cpu_has_gentimer )
>          panic("CPU does not support the Generic Timer v1 interface");
> -- 
> 2.0.4
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] arm/timer: fix panic when booting with DT

[Shannon Zhao]

On 2016/3/3 23:44, Stefano Stabellini wrote:
> On Thu, 3 Mar 2016, Shannon Zhao wrote:
>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> > 
>> > While to support ACPI, patch "arm/acpi: Parse GTDT to initialize timer"
>> > refactors the functions preinit_xen_time and init_xen_time. But it
>> > wrongly moves the platform_get_irq from init_xen_time to
>> > preinit_dt_xen_time and this will cause booting failure.
>> > 
>> > So move platform_get_irq back to init_xen_time to fix it.
>> > 
>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> Shannon,
> thank you for fixing the issue so quickly, very appreciated!
> 
> 
>> >  xen/arch/arm/time.c | 27 ++++++++++++++++-----------
>> >  1 file changed, 16 insertions(+), 11 deletions(-)
>> > 
>> > diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>> > index 5f8f974..66a4520 100644
>> > --- a/xen/arch/arm/time.c
>> > +++ b/xen/arch/arm/time.c
>> > @@ -119,7 +119,6 @@ static void __init preinit_dt_xen_time(void)
>> >      };
>> >      int res;
>> >      u32 rate;
>> > -    unsigned int i;
>> >  
>> >      timer = dt_find_matching_node(NULL, timer_ids);
>> >      if ( !timer )
>> > @@ -133,16 +132,6 @@ static void __init preinit_dt_xen_time(void)
>> >          cpu_khz = rate / 1000;
>> >          timer_dt_clock_frequency = rate;
>> >      }
>> > -
>> > -    /* Retrieve all IRQs for the timer */
>> > -    for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>> > -    {
>> > -        res = platform_get_irq(timer, i);
>> > -
>> > -        if ( res < 0 )
>> > -            panic("Timer: Unable to retrieve IRQ %u from the device tree", i);
>> > -        timer_irq[i] = res;
>> > -    }
>> >  }
>> >  
>> >  void __init preinit_xen_time(void)
>> > @@ -168,6 +157,22 @@ void __init preinit_xen_time(void)
>> >  /* Set up the timer on the boot CPU (late init function) */
>> >  int __init init_xen_time(void)
>> >  {
>> > +    int res;
>> > +    unsigned int i;
>> > +
>> > +    if ( acpi_disabled )
>> > +    {
>> > +        /* Retrieve all IRQs for the timer */
>> > +        for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>> > +        {
>> > +            res = platform_get_irq(timer, i);
>> > +
>> > +            if ( res < 0 )
>> > +                panic("Timer: Unable to retrieve IRQ %u from the device tree", i);
>> > +            timer_irq[i] = res;
>> > +        }
>> > +    }
> Could you please introduce a small little init_dt_xen_time function and
> call that instead from here?  Thanks!
> 
Not sure if it's necessary since it's only used here. But if you really
want that I'll add.

Thanks,
-- 
Shannon


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] arm/timer: fix panic when booting with DT

[Jan Beulich]

>>> On 04.03.16 at 02:17, <zhaoshenglong@huawei.com> wrote:
> On 2016/3/3 23:44, Stefano Stabellini wrote:
>> On Thu, 3 Mar 2016, Shannon Zhao wrote:
>>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>>> > 
>>> > While to support ACPI, patch "arm/acpi: Parse GTDT to initialize timer"
>>> > refactors the functions preinit_xen_time and init_xen_time. But it
>>> > wrongly moves the platform_get_irq from init_xen_time to
>>> > preinit_dt_xen_time and this will cause booting failure.
>>> > 
>>> > So move platform_get_irq back to init_xen_time to fix it.
>>> > 
>>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> Shannon,
>> thank you for fixing the issue so quickly, very appreciated!
>> 
>> 
>>> >  xen/arch/arm/time.c | 27 ++++++++++++++++-----------
>>> >  1 file changed, 16 insertions(+), 11 deletions(-)
>>> > 
>>> > diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>>> > index 5f8f974..66a4520 100644
>>> > --- a/xen/arch/arm/time.c
>>> > +++ b/xen/arch/arm/time.c
>>> > @@ -119,7 +119,6 @@ static void __init preinit_dt_xen_time(void)
>>> >      };
>>> >      int res;
>>> >      u32 rate;
>>> > -    unsigned int i;
>>> >  
>>> >      timer = dt_find_matching_node(NULL, timer_ids);
>>> >      if ( !timer )
>>> > @@ -133,16 +132,6 @@ static void __init preinit_dt_xen_time(void)
>>> >          cpu_khz = rate / 1000;
>>> >          timer_dt_clock_frequency = rate;
>>> >      }
>>> > -
>>> > -    /* Retrieve all IRQs for the timer */
>>> > -    for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>> > -    {
>>> > -        res = platform_get_irq(timer, i);
>>> > -
>>> > -        if ( res < 0 )
>>> > -            panic("Timer: Unable to retrieve IRQ %u from the device tree", i);
>>> > -        timer_irq[i] = res;
>>> > -    }
>>> >  }
>>> >  
>>> >  void __init preinit_xen_time(void)
>>> > @@ -168,6 +157,22 @@ void __init preinit_xen_time(void)
>>> >  /* Set up the timer on the boot CPU (late init function) */
>>> >  int __init init_xen_time(void)
>>> >  {
>>> > +    int res;
>>> > +    unsigned int i;
>>> > +
>>> > +    if ( acpi_disabled )
>>> > +    {
>>> > +        /* Retrieve all IRQs for the timer */
>>> > +        for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>> > +        {
>>> > +            res = platform_get_irq(timer, i);
>>> > +
>>> > +            if ( res < 0 )
>>> > +                panic("Timer: Unable to retrieve IRQ %u from the device tree", i);
>>> > +            timer_irq[i] = res;
>>> > +        }
>>> > +    }
>> Could you please introduce a small little init_dt_xen_time function and
>> call that instead from here?  Thanks!
>> 
> Not sure if it's necessary since it's only used here. But if you really
> want that I'll add.

Can both of you please aim at getting to an agreement relatively
quickly, to stop the flood of smoke test failure reports? (In general,
Shannon, I would say that if you were asked by the maintainer to
do a certain transformation, there's not overly much reason to ask
back again.) Or alternatively please indicate how far back the
previous series would need to be reverted.

Thanks, Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] arm/timer: fix panic when booting with DT

[Stefano Stabellini]

On Fri, 4 Mar 2016, Shannon Zhao wrote:
> On 2016/3/3 23:44, Stefano Stabellini wrote:
> > On Thu, 3 Mar 2016, Shannon Zhao wrote:
> >> > From: Shannon Zhao <shannon.zhao@linaro.org>
> >> > 
> >> > While to support ACPI, patch "arm/acpi: Parse GTDT to initialize timer"
> >> > refactors the functions preinit_xen_time and init_xen_time. But it
> >> > wrongly moves the platform_get_irq from init_xen_time to
> >> > preinit_dt_xen_time and this will cause booting failure.
> >> > 
> >> > So move platform_get_irq back to init_xen_time to fix it.
> >> > 
> >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > Shannon,
> > thank you for fixing the issue so quickly, very appreciated!
> > 
> > 
> >> >  xen/arch/arm/time.c | 27 ++++++++++++++++-----------
> >> >  1 file changed, 16 insertions(+), 11 deletions(-)
> >> > 
> >> > diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> >> > index 5f8f974..66a4520 100644
> >> > --- a/xen/arch/arm/time.c
> >> > +++ b/xen/arch/arm/time.c
> >> > @@ -119,7 +119,6 @@ static void __init preinit_dt_xen_time(void)
> >> >      };
> >> >      int res;
> >> >      u32 rate;
> >> > -    unsigned int i;
> >> >  
> >> >      timer = dt_find_matching_node(NULL, timer_ids);
> >> >      if ( !timer )
> >> > @@ -133,16 +132,6 @@ static void __init preinit_dt_xen_time(void)
> >> >          cpu_khz = rate / 1000;
> >> >          timer_dt_clock_frequency = rate;
> >> >      }
> >> > -
> >> > -    /* Retrieve all IRQs for the timer */
> >> > -    for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
> >> > -    {
> >> > -        res = platform_get_irq(timer, i);
> >> > -
> >> > -        if ( res < 0 )
> >> > -            panic("Timer: Unable to retrieve IRQ %u from the device tree", i);
> >> > -        timer_irq[i] = res;
> >> > -    }
> >> >  }
> >> >  
> >> >  void __init preinit_xen_time(void)
> >> > @@ -168,6 +157,22 @@ void __init preinit_xen_time(void)
> >> >  /* Set up the timer on the boot CPU (late init function) */
> >> >  int __init init_xen_time(void)
> >> >  {
> >> > +    int res;
> >> > +    unsigned int i;
> >> > +
> >> > +    if ( acpi_disabled )
> >> > +    {
> >> > +        /* Retrieve all IRQs for the timer */
> >> > +        for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
> >> > +        {
> >> > +            res = platform_get_irq(timer, i);
> >> > +
> >> > +            if ( res < 0 )
> >> > +                panic("Timer: Unable to retrieve IRQ %u from the device tree", i);
> >> > +            timer_irq[i] = res;
> >> > +        }
> >> > +    }
> > Could you please introduce a small little init_dt_xen_time function and
> > call that instead from here?  Thanks!
> > 
> Not sure if it's necessary since it's only used here. But if you really
> want that I'll add.

Please do, it is not necessary in terms of fixing the issue, but it
would make things nicer.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel