| Message ID | 1457014238-12208-1-git-send-email-zhaoshenglong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > >
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,
>>> 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
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.
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");