[PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Li, Aubrey]

On ACPI hardware reduced platform, the legacy PIC and PIT may not be
initialized even though they may be present in silicon. Touching
these legacy components causes unexpected result on system.

On Bay Trail-T(ASUS-T100) platform, touching these legacy components
blocks platform hardware low idle power state(S0ix) during system suspend.
So we should bypass them on ACPI hardware reduced platform.

Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Li Aubrey <aubrey.li@linux.intel.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 arch/x86/kernel/irqinit.c | 6 +++++-
 arch/x86/kernel/time.c    | 3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
index 70e181e..9a64cc3 100644
--- a/arch/x86/kernel/irqinit.c
+++ b/arch/x86/kernel/irqinit.c
@@ -75,7 +75,11 @@ void __init init_ISA_irqs(void)
 #if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)
 	init_bsp_APIC();
 #endif
-	legacy_pic->init(0);
+	if (acpi_gbl_reduced_hardware) {
+		pr_info("Using NULL legacy PIC\n");
+		legacy_pic = &null_legacy_pic;
+	} else
+		legacy_pic->init(0);
 
 	for (i = 0; i < nr_legacy_irqs(); i++)
 		irq_set_chip_and_handler(i, chip, handle_level_irq);
diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
index 25adc0e..5ba94fa 100644
--- a/arch/x86/kernel/time.c
+++ b/arch/x86/kernel/time.c
@@ -14,6 +14,7 @@
 #include <linux/i8253.h>
 #include <linux/time.h>
 #include <linux/export.h>
+#include <linux/acpi.h>
 
 #include <asm/vsyscall.h>
 #include <asm/x86_init.h>
@@ -76,7 +77,7 @@ void __init setup_default_timer_irq(void)
 /* Default timer init function */
 void __init hpet_time_init(void)
 {
-	if (!hpet_enable())
+	if (!hpet_enable() && !acpi_gbl_reduced_hardware)
 		setup_pit_timer();
 	setup_default_timer_irq();
 }
-- 
1.9.1

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Ingo Molnar]

* Li, Aubrey <aubrey.li@linux.intel.com> wrote:

> On ACPI hardware reduced platform, the legacy PIC and PIT may not be
> initialized even though they may be present in silicon. Touching
> these legacy components causes unexpected result on system.
> 
> On Bay Trail-T(ASUS-T100) platform, touching these legacy components
> blocks platform hardware low idle power state(S0ix) during system suspend.
> So we should bypass them on ACPI hardware reduced platform.
> 
> Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
> Signed-off-by: Li Aubrey <aubrey.li@linux.intel.com>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  arch/x86/kernel/irqinit.c | 6 +++++-
>  arch/x86/kernel/time.c    | 3 ++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
> index 70e181e..9a64cc3 100644
> --- a/arch/x86/kernel/irqinit.c
> +++ b/arch/x86/kernel/irqinit.c
> @@ -75,7 +75,11 @@ void __init init_ISA_irqs(void)
>  #if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)
>  	init_bsp_APIC();
>  #endif
> -	legacy_pic->init(0);
> +	if (acpi_gbl_reduced_hardware) {
> +		pr_info("Using NULL legacy PIC\n");
> +		legacy_pic = &null_legacy_pic;
> +	} else
> +		legacy_pic->init(0);
>  
>  	for (i = 0; i < nr_legacy_irqs(); i++)
>  		irq_set_chip_and_handler(i, chip, handle_level_irq);
> diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
> index 25adc0e..5ba94fa 100644
> --- a/arch/x86/kernel/time.c
> +++ b/arch/x86/kernel/time.c
> @@ -14,6 +14,7 @@
>  #include <linux/i8253.h>
>  #include <linux/time.h>
>  #include <linux/export.h>
> +#include <linux/acpi.h>
>  
>  #include <asm/vsyscall.h>
>  #include <asm/x86_init.h>
> @@ -76,7 +77,7 @@ void __init setup_default_timer_irq(void)
>  /* Default timer init function */
>  void __init hpet_time_init(void)
>  {
> -	if (!hpet_enable())
> +	if (!hpet_enable() && !acpi_gbl_reduced_hardware)
>  		setup_pit_timer();
>  	setup_default_timer_irq();
>  }

So the whole acpi_gbl_reduced_hardware flaggery sucks as it mixes 
various hardware drivers that have little relation to each other...

Instead of having a proper platform init this flag hooks into various 
drivers and generic code, such as the efi reboot and shutdown code, 
and now the generic irq init code.

For this IRQ init problem, why not add a proper callback to 
x86_platform_ops, define your own IRQ init function, initialize it in 
your platform init sequence and let it be called? That solves it 
without creating an ugly mix of different platform methods.

For the EFI shutdown case, what's wrong with setting your own 
pm_power_off handler like most of the other platforms are doing? Plus 
the EFI code in drivers/firmware/efi/reboot.c should probably only set 
the shutdown handler if pm_power_off is still NULL.

Thanks,

	Ingo

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Li, Aubrey]

On 2015/3/4 13:08, Ingo Molnar wrote:
> 
> * Li, Aubrey <aubrey.li@linux.intel.com> wrote:
> 
>> On ACPI hardware reduced platform, the legacy PIC and PIT may not be
>> initialized even though they may be present in silicon. Touching
>> these legacy components causes unexpected result on system.
>>
>> On Bay Trail-T(ASUS-T100) platform, touching these legacy components
>> blocks platform hardware low idle power state(S0ix) during system suspend.
>> So we should bypass them on ACPI hardware reduced platform.
>>
>> Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
>> Signed-off-by: Li Aubrey <aubrey.li@linux.intel.com>
>> Cc: Len Brown <len.brown@intel.com>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>  arch/x86/kernel/irqinit.c | 6 +++++-
>>  arch/x86/kernel/time.c    | 3 ++-
>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
>> index 70e181e..9a64cc3 100644
>> --- a/arch/x86/kernel/irqinit.c
>> +++ b/arch/x86/kernel/irqinit.c
>> @@ -75,7 +75,11 @@ void __init init_ISA_irqs(void)
>>  #if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)
>>  	init_bsp_APIC();
>>  #endif
>> -	legacy_pic->init(0);
>> +	if (acpi_gbl_reduced_hardware) {
>> +		pr_info("Using NULL legacy PIC\n");
>> +		legacy_pic = &null_legacy_pic;
>> +	} else
>> +		legacy_pic->init(0);
>>  
>>  	for (i = 0; i < nr_legacy_irqs(); i++)
>>  		irq_set_chip_and_handler(i, chip, handle_level_irq);
>> diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
>> index 25adc0e..5ba94fa 100644
>> --- a/arch/x86/kernel/time.c
>> +++ b/arch/x86/kernel/time.c
>> @@ -14,6 +14,7 @@
>>  #include <linux/i8253.h>
>>  #include <linux/time.h>
>>  #include <linux/export.h>
>> +#include <linux/acpi.h>
>>  
>>  #include <asm/vsyscall.h>
>>  #include <asm/x86_init.h>
>> @@ -76,7 +77,7 @@ void __init setup_default_timer_irq(void)
>>  /* Default timer init function */
>>  void __init hpet_time_init(void)
>>  {
>> -	if (!hpet_enable())
>> +	if (!hpet_enable() && !acpi_gbl_reduced_hardware)
>>  		setup_pit_timer();
>>  	setup_default_timer_irq();
>>  }
> 
> So the whole acpi_gbl_reduced_hardware flaggery sucks as it mixes 
> various hardware drivers that have little relation to each other...
> 
> Instead of having a proper platform init this flag hooks into various 
> drivers and generic code, such as the efi reboot and shutdown code, 
> and now the generic irq init code.
> 
> For this IRQ init problem, why not add a proper callback to 
> x86_platform_ops, define your own IRQ init function, initialize it in 
> your platform init sequence and let it be called? That solves it 
> without creating an ugly mix of different platform methods.
> 
> For the EFI shutdown case, what's wrong with setting your own 
> pm_power_off handler like most of the other platforms are doing? Plus 
> the EFI code in drivers/firmware/efi/reboot.c should probably only set 
> the shutdown handler if pm_power_off is still NULL.

I think our goal is to make the code as generic as possible for all x86
platform, rather than creating a new x86 branch, I added Alan Cox for
this strategy discussion.

Do you have any inputs for the patch itself?

Thanks,
-Aubrey

> 
> Thanks,
> 
> 	Ingo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Ingo Molnar]

* Li, Aubrey <aubrey.li@linux.intel.com> wrote:

> On 2015/3/4 13:08, Ingo Molnar wrote:
> > 
> > * Li, Aubrey <aubrey.li@linux.intel.com> wrote:
> > 
> >> On ACPI hardware reduced platform, the legacy PIC and PIT may not be
> >> initialized even though they may be present in silicon. Touching
> >> these legacy components causes unexpected result on system.
> >>
> >> On Bay Trail-T(ASUS-T100) platform, touching these legacy components
> >> blocks platform hardware low idle power state(S0ix) during system suspend.
> >> So we should bypass them on ACPI hardware reduced platform.
> >>
> >> Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
> >> Signed-off-by: Li Aubrey <aubrey.li@linux.intel.com>
> >> Cc: Len Brown <len.brown@intel.com>
> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> ---
> >>  arch/x86/kernel/irqinit.c | 6 +++++-
> >>  arch/x86/kernel/time.c    | 3 ++-
> >>  2 files changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
> >> index 70e181e..9a64cc3 100644
> >> --- a/arch/x86/kernel/irqinit.c
> >> +++ b/arch/x86/kernel/irqinit.c
> >> @@ -75,7 +75,11 @@ void __init init_ISA_irqs(void)
> >>  #if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)
> >>  	init_bsp_APIC();
> >>  #endif
> >> -	legacy_pic->init(0);
> >> +	if (acpi_gbl_reduced_hardware) {
> >> +		pr_info("Using NULL legacy PIC\n");
> >> +		legacy_pic = &null_legacy_pic;
> >> +	} else
> >> +		legacy_pic->init(0);
> >>  
> >>  	for (i = 0; i < nr_legacy_irqs(); i++)
> >>  		irq_set_chip_and_handler(i, chip, handle_level_irq);
> >> diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
> >> index 25adc0e..5ba94fa 100644
> >> --- a/arch/x86/kernel/time.c
> >> +++ b/arch/x86/kernel/time.c
> >> @@ -14,6 +14,7 @@
> >>  #include <linux/i8253.h>
> >>  #include <linux/time.h>
> >>  #include <linux/export.h>
> >> +#include <linux/acpi.h>
> >>  
> >>  #include <asm/vsyscall.h>
> >>  #include <asm/x86_init.h>
> >> @@ -76,7 +77,7 @@ void __init setup_default_timer_irq(void)
> >>  /* Default timer init function */
> >>  void __init hpet_time_init(void)
> >>  {
> >> -	if (!hpet_enable())
> >> +	if (!hpet_enable() && !acpi_gbl_reduced_hardware)
> >>  		setup_pit_timer();
> >>  	setup_default_timer_irq();
> >>  }
> > 
> > So the whole acpi_gbl_reduced_hardware flaggery sucks as it mixes 
> > various hardware drivers that have little relation to each other...
> > 
> > Instead of having a proper platform init this flag hooks into various 
> > drivers and generic code, such as the efi reboot and shutdown code, 
> > and now the generic irq init code.
> > 
> > For this IRQ init problem, why not add a proper callback to 
> > x86_platform_ops, define your own IRQ init function, initialize it in 
> > your platform init sequence and let it be called? That solves it 
> > without creating an ugly mix of different platform methods.
> > 
> > For the EFI shutdown case, what's wrong with setting your own 
> > pm_power_off handler like most of the other platforms are doing? Plus 
> > the EFI code in drivers/firmware/efi/reboot.c should probably only set 
> > the shutdown handler if pm_power_off is still NULL.
> 
> I think our goal is to make the code as generic as possible for all 
> x86 platform, rather than creating a new x86 branch, I added Alan 
> Cox for this strategy discussion.
> 
> Do you have any inputs for the patch itself?

Other than that the patch is unacceptable for an upstream merge in its 
current form for the reason I mentioned? No.

Thanks,

	Ingo

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Li, Aubrey]

On 2015/3/4 13:31, Ingo Molnar wrote:
> 
> * Li, Aubrey <aubrey.li@linux.intel.com> wrote:
> 
>> On 2015/3/4 13:08, Ingo Molnar wrote:
>>>
>>> * Li, Aubrey <aubrey.li@linux.intel.com> wrote:
>>>
>>>> On ACPI hardware reduced platform, the legacy PIC and PIT may not be
>>>> initialized even though they may be present in silicon. Touching
>>>> these legacy components causes unexpected result on system.
>>>>
>>>> On Bay Trail-T(ASUS-T100) platform, touching these legacy components
>>>> blocks platform hardware low idle power state(S0ix) during system suspend.
>>>> So we should bypass them on ACPI hardware reduced platform.
>>>>
>>>> Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
>>>> Signed-off-by: Li Aubrey <aubrey.li@linux.intel.com>
>>>> Cc: Len Brown <len.brown@intel.com>
>>>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> ---
>>>>  arch/x86/kernel/irqinit.c | 6 +++++-
>>>>  arch/x86/kernel/time.c    | 3 ++-
>>>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
>>>> index 70e181e..9a64cc3 100644
>>>> --- a/arch/x86/kernel/irqinit.c
>>>> +++ b/arch/x86/kernel/irqinit.c
>>>> @@ -75,7 +75,11 @@ void __init init_ISA_irqs(void)
>>>>  #if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)
>>>>  	init_bsp_APIC();
>>>>  #endif
>>>> -	legacy_pic->init(0);
>>>> +	if (acpi_gbl_reduced_hardware) {
>>>> +		pr_info("Using NULL legacy PIC\n");
>>>> +		legacy_pic = &null_legacy_pic;
>>>> +	} else
>>>> +		legacy_pic->init(0);
>>>>  
>>>>  	for (i = 0; i < nr_legacy_irqs(); i++)
>>>>  		irq_set_chip_and_handler(i, chip, handle_level_irq);
>>>> diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
>>>> index 25adc0e..5ba94fa 100644
>>>> --- a/arch/x86/kernel/time.c
>>>> +++ b/arch/x86/kernel/time.c
>>>> @@ -14,6 +14,7 @@
>>>>  #include <linux/i8253.h>
>>>>  #include <linux/time.h>
>>>>  #include <linux/export.h>
>>>> +#include <linux/acpi.h>
>>>>  
>>>>  #include <asm/vsyscall.h>
>>>>  #include <asm/x86_init.h>
>>>> @@ -76,7 +77,7 @@ void __init setup_default_timer_irq(void)
>>>>  /* Default timer init function */
>>>>  void __init hpet_time_init(void)
>>>>  {
>>>> -	if (!hpet_enable())
>>>> +	if (!hpet_enable() && !acpi_gbl_reduced_hardware)
>>>>  		setup_pit_timer();
>>>>  	setup_default_timer_irq();
>>>>  }
>>>
>>> So the whole acpi_gbl_reduced_hardware flaggery sucks as it mixes 
>>> various hardware drivers that have little relation to each other...
>>>
>>> Instead of having a proper platform init this flag hooks into various 
>>> drivers and generic code, such as the efi reboot and shutdown code, 
>>> and now the generic irq init code.
>>>
>>> For this IRQ init problem, why not add a proper callback to 
>>> x86_platform_ops, define your own IRQ init function, initialize it in 
>>> your platform init sequence and let it be called? That solves it 
>>> without creating an ugly mix of different platform methods.
>>>
>>> For the EFI shutdown case, what's wrong with setting your own 
>>> pm_power_off handler like most of the other platforms are doing? Plus 
>>> the EFI code in drivers/firmware/efi/reboot.c should probably only set 
>>> the shutdown handler if pm_power_off is still NULL.
>>
>> I think our goal is to make the code as generic as possible for all 
>> x86 platform, rather than creating a new x86 branch, I added Alan 
>> Cox for this strategy discussion.
>>
>> Do you have any inputs for the patch itself?
> 
> Other than that the patch is unacceptable for an upstream merge in its 
> current form for the reason I mentioned? No.

So you are suggesting we extend a new x86 platform branch and override
the x86_platform and pm_power_off and reboot, like what intel_mid does?

Thanks,
-Aubrey

> 
> Thanks,
> 
> 	Ingo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Ingo Molnar]

* Li, Aubrey <aubrey.li@linux.intel.com> wrote:

> On 2015/3/4 13:31, Ingo Molnar wrote:
> > 
> > * Li, Aubrey <aubrey.li@linux.intel.com> wrote:
> > 
> >> On 2015/3/4 13:08, Ingo Molnar wrote:
> >>>
> >>> * Li, Aubrey <aubrey.li@linux.intel.com> wrote:
> >>>
> >>>> On ACPI hardware reduced platform, the legacy PIC and PIT may not be
> >>>> initialized even though they may be present in silicon. Touching
> >>>> these legacy components causes unexpected result on system.
> >>>>
> >>>> On Bay Trail-T(ASUS-T100) platform, touching these legacy components
> >>>> blocks platform hardware low idle power state(S0ix) during system suspend.
> >>>> So we should bypass them on ACPI hardware reduced platform.
> >>>>
> >>>> Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
> >>>> Signed-off-by: Li Aubrey <aubrey.li@linux.intel.com>
> >>>> Cc: Len Brown <len.brown@intel.com>
> >>>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>> ---
> >>>>  arch/x86/kernel/irqinit.c | 6 +++++-
> >>>>  arch/x86/kernel/time.c    | 3 ++-
> >>>>  2 files changed, 7 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
> >>>> index 70e181e..9a64cc3 100644
> >>>> --- a/arch/x86/kernel/irqinit.c
> >>>> +++ b/arch/x86/kernel/irqinit.c
> >>>> @@ -75,7 +75,11 @@ void __init init_ISA_irqs(void)
> >>>>  #if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)
> >>>>  	init_bsp_APIC();
> >>>>  #endif
> >>>> -	legacy_pic->init(0);
> >>>> +	if (acpi_gbl_reduced_hardware) {
> >>>> +		pr_info("Using NULL legacy PIC\n");
> >>>> +		legacy_pic = &null_legacy_pic;
> >>>> +	} else
> >>>> +		legacy_pic->init(0);
> >>>>  
> >>>>  	for (i = 0; i < nr_legacy_irqs(); i++)
> >>>>  		irq_set_chip_and_handler(i, chip, handle_level_irq);
> >>>> diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
> >>>> index 25adc0e..5ba94fa 100644
> >>>> --- a/arch/x86/kernel/time.c
> >>>> +++ b/arch/x86/kernel/time.c
> >>>> @@ -14,6 +14,7 @@
> >>>>  #include <linux/i8253.h>
> >>>>  #include <linux/time.h>
> >>>>  #include <linux/export.h>
> >>>> +#include <linux/acpi.h>
> >>>>  
> >>>>  #include <asm/vsyscall.h>
> >>>>  #include <asm/x86_init.h>
> >>>> @@ -76,7 +77,7 @@ void __init setup_default_timer_irq(void)
> >>>>  /* Default timer init function */
> >>>>  void __init hpet_time_init(void)
> >>>>  {
> >>>> -	if (!hpet_enable())
> >>>> +	if (!hpet_enable() && !acpi_gbl_reduced_hardware)
> >>>>  		setup_pit_timer();
> >>>>  	setup_default_timer_irq();
> >>>>  }
> >>>
> >>> So the whole acpi_gbl_reduced_hardware flaggery sucks as it mixes 
> >>> various hardware drivers that have little relation to each other...
> >>>
> >>> Instead of having a proper platform init this flag hooks into various 
> >>> drivers and generic code, such as the efi reboot and shutdown code, 
> >>> and now the generic irq init code.
> >>>
> >>> For this IRQ init problem, why not add a proper callback to 
> >>> x86_platform_ops, define your own IRQ init function, initialize it in 
> >>> your platform init sequence and let it be called? That solves it 
> >>> without creating an ugly mix of different platform methods.
> >>>
> >>> For the EFI shutdown case, what's wrong with setting your own 
> >>> pm_power_off handler like most of the other platforms are doing? Plus 
> >>> the EFI code in drivers/firmware/efi/reboot.c should probably only set 
> >>> the shutdown handler if pm_power_off is still NULL.
> >>
> >> I think our goal is to make the code as generic as possible for all 
> >> x86 platform, rather than creating a new x86 branch, I added Alan 
> >> Cox for this strategy discussion.
> >>
> >> Do you have any inputs for the patch itself?
> > 
> > Other than that the patch is unacceptable for an upstream merge in its 
> > current form for the reason I mentioned? No.
> 
> So you are suggesting we extend a new x86 platform branch and 
> override the x86_platform and pm_power_off and reboot, like what 
> intel_mid does?

Well, what I suggested above was to add an IRQ init method to 
x86_platform (and make use of it on your platform), and to
use the existing pm_power_off method for the reboot quirk.

Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code
is a mistake.

Thanks,

	Ingo

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Arjan van de Ven]

>
> Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code
> is a mistake.

ideally, the presence of that flag in the firmware table will clear/set more global settings,
for example, having that flag should cause the 8042 input code to not probe for the 8042.

for interrupts, there really ought to be a "apic first/only" mode, which is then used on
all modern systems (not just hw reduced).

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Borislav Petkov]

On Wed, Mar 04, 2015 at 12:43:08AM -0800, Arjan van de Ven wrote:
> >
> >Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code
> >is a mistake.
> 
> ideally, the presence of that flag in the firmware table will clear/set more global settings,
> for example, having that flag should cause the 8042 input code to not probe for the 8042.
> 
> for interrupts, there really ought to be a "apic first/only" mode, which is then used on
> all modern systems (not just hw reduced).

Do we need some sort of platform-specific querying interfaces now too,
similar to cpu_has()? I.e., platform_has()...

	if (platform_has(X86_PLATFORM_REDUCED_HW))
		do stuff..

Hmmm.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Borislav Petkov]

On Wed, Mar 04, 2015 at 03:16:07PM +0100, Rafael J. Wysocki wrote:
> Sort of.  What we need is a "do not touch PIC/PIT" bit for the code that
> tries to fall back to them in some cases (which may appear to work if
> the hardware is physically there, but it may confuse the platform).

Can "some cases" detection be nicely put into a x86_platform
platform-specific method?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Rafael J. Wysocki]

On Wednesday, March 04, 2015 10:50:11 AM Borislav Petkov wrote:
> On Wed, Mar 04, 2015 at 12:43:08AM -0800, Arjan van de Ven wrote:
> > >
> > >Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code
> > >is a mistake.
> > 
> > ideally, the presence of that flag in the firmware table will clear/set more global settings,
> > for example, having that flag should cause the 8042 input code to not probe for the 8042.
> > 
> > for interrupts, there really ought to be a "apic first/only" mode, which is then used on
> > all modern systems (not just hw reduced).
> 
> Do we need some sort of platform-specific querying interfaces now too,
> similar to cpu_has()? I.e., platform_has()...
> 
> 	if (platform_has(X86_PLATFORM_REDUCED_HW))
> 		do stuff..
> 
> Hmmm.

Sort of.  What we need is a "do not touch PIC/PIT" bit for the code that
tries to fall back to them in some cases (which may appear to work if
the hardware is physically there, but it may confuse the platform).


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Arjan van de Ven]

On 3/4/2015 1:50 AM, Borislav Petkov wrote:
> On Wed, Mar 04, 2015 at 12:43:08AM -0800, Arjan van de Ven wrote:
>>>
>>> Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code
>>> is a mistake.
>>
>> ideally, the presence of that flag in the firmware table will clear/set more global settings,
>> for example, having that flag should cause the 8042 input code to not probe for the 8042.
>>
>> for interrupts, there really ought to be a "apic first/only" mode, which is then used on
>> all modern systems (not just hw reduced).
>
> Do we need some sort of platform-specific querying interfaces now too,
> similar to cpu_has()? I.e., platform_has()...
>
> 	if (platform_has(X86_PLATFORM_REDUCED_HW))
> 		do stuff..

more like

platform_has(X86_PLATFORM_PIT)

etc, one for each legacy io item

so we can clear it on hw reduced, but also in other cases.
hw reduced is one way, but I'd be surprised if there weren't other ways (like quirks)
where we'd want to do the same things

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Rafael J. Wysocki]

On Wednesday, March 04, 2015 03:05:55 PM Borislav Petkov wrote:
> On Wed, Mar 04, 2015 at 03:16:07PM +0100, Rafael J. Wysocki wrote:
> > Sort of.  What we need is a "do not touch PIC/PIT" bit for the code that
> > tries to fall back to them in some cases (which may appear to work if
> > the hardware is physically there, but it may confuse the platform).
> 
> Can "some cases" detection be nicely put into a x86_platform
> platform-specific method?

I guess so.

The problem is that we just do that fallback unconditionally today which
evidently doesn't always work.

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Ingo Molnar]

* Arjan van de Ven <arjan@linux.intel.com> wrote:

> On 3/4/2015 1:50 AM, Borislav Petkov wrote:
> >On Wed, Mar 04, 2015 at 12:43:08AM -0800, Arjan van de Ven wrote:
> >>>
> >>>Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code
> >>>is a mistake.
> >>
> >>ideally, the presence of that flag in the firmware table will clear/set more global settings,
> >>for example, having that flag should cause the 8042 input code to not probe for the 8042.
> >>
> >>for interrupts, there really ought to be a "apic first/only" mode, which is then used on
> >>all modern systems (not just hw reduced).
> >
> >Do we need some sort of platform-specific querying interfaces now too,
> >similar to cpu_has()? I.e., platform_has()...
> >
> >	if (platform_has(X86_PLATFORM_REDUCED_HW))
> >		do stuff..
> 
> more like
> 
> platform_has(X86_PLATFORM_PIT)
> 
> etc, one for each legacy io item

Precisely. The main problem is the generic, 'lumps everything 
together' nature of the acpi_gbl_reduced_hardware flag.

(Like the big kernel lock lumped together all sorts of locking rules 
and semantics.)

Properly split out, feature-ish or driver-ish interfaces for PIT and 
other legacy details are the proper approach to 'turn them off'.

 - x86_platform is a function pointer driven, driver-ish interface.

 - platform_has(X86_PLATFORM_IT) is a flag driven, feature-flag-ish
   interface.

Both are fine - for something as separate as the PIT (or the PIC) it 
might make more sense to go towards a 'driver' interface though, as 
modern drivers are (and will be) much different from the legacy PIT.

Whichever method is used, low level platforms can just switch them 
on/off in their enumeration/detection routines, while the generic code 
will have them enabled by default.

> so we can clear it on hw reduced, but also in other cases. hw 
> reduced is one way, but I'd be surprised if there weren't other ways 
> (like quirks) where we'd want to do the same things

Exactly. The key step is the proper, clean separation out of hardware 
interfaces.

Thanks,

	Ingo

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Alan Cox]

On Wed, 2015-03-04 at 10:50 +0100, Borislav Petkov wrote:
> On Wed, Mar 04, 2015 at 12:43:08AM -0800, Arjan van de Ven wrote:
> > >
> > >Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code
> > >is a mistake.
> > 
> > ideally, the presence of that flag in the firmware table will clear/set more global settings,
> > for example, having that flag should cause the 8042 input code to not probe for the 8042.
> > 
> > for interrupts, there really ought to be a "apic first/only" mode, which is then used on
> > all modern systems (not just hw reduced).
> 
> Do we need some sort of platform-specific querying interfaces now too,
> similar to cpu_has()? I.e., platform_has()...
> 
> 	if (platform_has(X86_PLATFORM_REDUCED_HW))
> 		do stuff..

ACPI hw reduced is not an x86 specific concept so quite possibly yes.

ACPI is the usual source for a variety of generic platform information
such as absence and presence of prehistoric PC compatibility goo -
increasingly so in fact. It tells you if the device is probably a
tablet, if it is more efficient to idle via suspend/resume or by asking
the cpu to idle. It tells you if low power modes are supported and so
on.

I don't think it makes sense to treat "ACPI reduced" as some kind of
platform concept. You could easily get basically identical hardware that
is or is not "hw reduced" depending upon firmware choices.

Alan

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Alan Cox]

On Wed, 2015-03-04 at 15:05 +0100, Borislav Petkov wrote:
> On Wed, Mar 04, 2015 at 03:16:07PM +0100, Rafael J. Wysocki wrote:
> > Sort of.  What we need is a "do not touch PIC/PIT" bit for the code that
> > tries to fall back to them in some cases (which may appear to work if
> > the hardware is physically there, but it may confuse the platform).
> 
> Can "some cases" detection be nicely put into a x86_platform
> platform-specific method?

In some cases they don't belong in x86, ACPI is also used for ARM64.

However

	if ( has_8259_pic() )

is trivally 0, 1 or some platform or acpi provided method.

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Rafael J. Wysocki]

On Wednesday, March 04, 2015 08:21:01 PM Alan Cox wrote:
> On Wed, 2015-03-04 at 15:05 +0100, Borislav Petkov wrote:
> > On Wed, Mar 04, 2015 at 03:16:07PM +0100, Rafael J. Wysocki wrote:
> > > Sort of.  What we need is a "do not touch PIC/PIT" bit for the code that
> > > tries to fall back to them in some cases (which may appear to work if
> > > the hardware is physically there, but it may confuse the platform).
> > 
> > Can "some cases" detection be nicely put into a x86_platform
> > platform-specific method?
> 
> In some cases they don't belong in x86, ACPI is also used for ARM64.
> 
> However
> 
> 	if ( has_8259_pic() )
> 
> is trivally 0, 1 or some platform or acpi provided method.

And which is how that should have been implemented to start with IMO.

Besides, the "ACPI reduced hardware" case is kind of a red herring here,
because it most likely is not the only case when we'll want has_8259_pic()
to return 0 (quite likely, we'll want that on all BayTrail-based systems,
for example).


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Li, Aubrey]

On 2015/3/5 4:11, Ingo Molnar wrote:
> 
> * Arjan van de Ven <arjan@linux.intel.com> wrote:
> 
>> On 3/4/2015 1:50 AM, Borislav Petkov wrote:
>>> On Wed, Mar 04, 2015 at 12:43:08AM -0800, Arjan van de Ven wrote:
>>>>>
>>>>> Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code
>>>>> is a mistake.
>>>>
>>>> ideally, the presence of that flag in the firmware table will clear/set more global settings,
>>>> for example, having that flag should cause the 8042 input code to not probe for the 8042.
>>>>
>>>> for interrupts, there really ought to be a "apic first/only" mode, which is then used on
>>>> all modern systems (not just hw reduced).
>>>
>>> Do we need some sort of platform-specific querying interfaces now too,
>>> similar to cpu_has()? I.e., platform_has()...
>>>
>>> 	if (platform_has(X86_PLATFORM_REDUCED_HW))
>>> 		do stuff..
>>
>> more like
>>
>> platform_has(X86_PLATFORM_PIT)
>>
>> etc, one for each legacy io item
> 
> Precisely. The main problem is the generic, 'lumps everything 
> together' nature of the acpi_gbl_reduced_hardware flag.
> 
> (Like the big kernel lock lumped together all sorts of locking rules 
> and semantics.)
> 
> Properly split out, feature-ish or driver-ish interfaces for PIT and 
> other legacy details are the proper approach to 'turn them off'.
> 
>  - x86_platform is a function pointer driven, driver-ish interface.
> 
>  - platform_has(X86_PLATFORM_IT) is a flag driven, feature-flag-ish
>    interface.
> 
> Both are fine - for something as separate as the PIT (or the PIC) it 
> might make more sense to go towards a 'driver' interface though, as 
> modern drivers are (and will be) much different from the legacy PIT.
> 
> Whichever method is used, low level platforms can just switch them 
> on/off in their enumeration/detection routines, while the generic code 
> will have them enabled by default.

Whichever method is used, we will face a problem how to determine PIT
exists or not.

When we enabled Bay Trail-T platform at the beginning, we were trying to
make the code as generic as possible, and it works properly up to now.
So we don't have a SUBARCH like X86_SUBARCH_INTEL_MID to use the
platform specific functions. And for now I'm not quite sure it's a good
idea to create one.

If we make it as a flag driven, I don't know there is a flag in firmware
better than ACPI HW reduced flag(Of course it's not good enough to cover
all the cases). Or if we want to use platform info to turn on/off this
flag, we'll have to maintain a platform list, which may be longer and
more complicated than worth doing that.

Thanks,
-Aubrey
> 
>> so we can clear it on hw reduced, but also in other cases. hw 
>> reduced is one way, but I'd be surprised if there weren't other ways 
>> (like quirks) where we'd want to do the same things
> 
> Exactly. The key step is the proper, clean separation out of hardware 
> interfaces.
> 
> Thanks,
> 
> 	Ingo
> 
> 

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Li, Aubrey]

On 2015/3/5 5:52, Rafael J. Wysocki wrote:
> On Wednesday, March 04, 2015 08:21:01 PM Alan Cox wrote:
>> On Wed, 2015-03-04 at 15:05 +0100, Borislav Petkov wrote:
>>> On Wed, Mar 04, 2015 at 03:16:07PM +0100, Rafael J. Wysocki wrote:
>>>> Sort of.  What we need is a "do not touch PIC/PIT" bit for the code that
>>>> tries to fall back to them in some cases (which may appear to work if
>>>> the hardware is physically there, but it may confuse the platform).
>>>
>>> Can "some cases" detection be nicely put into a x86_platform
>>> platform-specific method?
>>
>> In some cases they don't belong in x86, ACPI is also used for ARM64.
>>
>> However
>>
>> 	if ( has_8259_pic() )
>>
>> is trivally 0, 1 or some platform or acpi provided method.
> 
> And which is how that should have been implemented to start with IMO.
> 
> Besides, the "ACPI reduced hardware" case is kind of a red herring here,
> because it most likely is not the only case when we'll want has_8259_pic()
> to return 0 (quite likely, we'll want that on all BayTrail-based systems,
> for example).
> 
BayTrail-based systems has BayTrail-I, BayTrail-M, BayTrail-D,
BayTrail-T, BayTrail-T/CR. BayTrail-D is a desktop and BayTrail-M is a
mobile/laptop and 8259 exists on both systems and I don't think we want
to bypass it.

ACPI reduced hardware is the best case in my mind unless you want to
enumerate the platform one by one. can we make a global variable

u8 has_8259;

and initialize it by acpi reduced hardware flag? or a wrapper function?

Thanks,
-Aubrey

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Ingo Molnar]

* Li, Aubrey <aubrey.li@linux.intel.com> wrote:

> On 2015/3/5 4:11, Ingo Molnar wrote:
> > 
> > * Arjan van de Ven <arjan@linux.intel.com> wrote:
> > 
> >> On 3/4/2015 1:50 AM, Borislav Petkov wrote:
> >>> On Wed, Mar 04, 2015 at 12:43:08AM -0800, Arjan van de Ven wrote:
> >>>>>
> >>>>> Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code
> >>>>> is a mistake.
> >>>>
> >>>> ideally, the presence of that flag in the firmware table will clear/set more global settings,
> >>>> for example, having that flag should cause the 8042 input code to not probe for the 8042.
> >>>>
> >>>> for interrupts, there really ought to be a "apic first/only" mode, which is then used on
> >>>> all modern systems (not just hw reduced).
> >>>
> >>> Do we need some sort of platform-specific querying interfaces now too,
> >>> similar to cpu_has()? I.e., platform_has()...
> >>>
> >>> 	if (platform_has(X86_PLATFORM_REDUCED_HW))
> >>> 		do stuff..
> >>
> >> more like
> >>
> >> platform_has(X86_PLATFORM_PIT)
> >>
> >> etc, one for each legacy io item
> > 
> > Precisely. The main problem is the generic, 'lumps everything 
> > together' nature of the acpi_gbl_reduced_hardware flag.
> > 
> > (Like the big kernel lock lumped together all sorts of locking rules 
> > and semantics.)
> > 
> > Properly split out, feature-ish or driver-ish interfaces for PIT and 
> > other legacy details are the proper approach to 'turn them off'.
> > 
> >  - x86_platform is a function pointer driven, driver-ish interface.
> > 
> >  - platform_has(X86_PLATFORM_IT) is a flag driven, feature-flag-ish
> >    interface.
> > 
> > Both are fine - for something as separate as the PIT (or the PIC) 
> > it might make more sense to go towards a 'driver' interface 
> > though, as modern drivers are (and will be) much different from 
> > the legacy PIT.
> > 
> > Whichever method is used, low level platforms can just switch them 
> > on/off in their enumeration/detection routines, while the generic 
> > code will have them enabled by default.
> 
> Whichever method is used, we will face a problem how to determine 
> PIT exists or not.
> 
> When we enabled Bay Trail-T platform at the beginning, we were 
> trying to make the code as generic as possible, and it works 
> properly up to now. So we don't have a SUBARCH like 
> X86_SUBARCH_INTEL_MID to use the platform specific functions. And 
> for now I'm not quite sure it's a good idea to create one.
> 
> If we make it as a flag driven, I don't know there is a flag in 
> firmware better than ACPI HW reduced flag(Of course it's not good 
> enough to cover all the cases). Or if we want to use platform info 
> to turn on/off this flag, we'll have to maintain a platform list, 
> which may be longer and more complicated than worth doing that.

Well, it's not nearly so difficult, because you already have a 
platform flag: acpi_gbl_reduced_hardware.

What I object against is to infest generic codepaths with unreadable, 
unrobust crap like:

+       if (acpi_gbl_reduced_hardware) {
+               pr_info("Using NULL legacy PIC\n");
+               legacy_pic = &null_legacy_pic;
+       } else
+               legacy_pic->init(0);

To solve that, add a small (early) init function (say 
'x86_reduced_hw_init()') that sets up the right driver
selections if acpi_gbl_reduced_hardware is set:

 - in x86_reduced_hw_init() set 'legacy_pic' to 'null_legacy_pic'

 - clean up 'global_clock_event' handling: instead of a global 
   variable, move its management into x86_platform_ops::get_clockevent()
   and set the method to hpet/pit/abp/etc. specific handlers that
   return the right clockevent device.

 - in your x86_reduced_hw_init() function add the hpet clockevent
   device to x86_platform_ops::get_clockevent, overriding the default
   PIT.

 - in x86_reduced_hw_init() set pm_power_off.

 - set 'reboot_type' and remove the acpi_gbl_reduced_hardware hack
   from efi_reboot_required().

etc.

Just keep the generic init codepaths free of those random selections 
based on global flags, okay?

Thanks,

	Ingo

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Li, Aubrey]

On 2015/3/5 19:36, Ingo Molnar wrote:
> 
> * Li, Aubrey <aubrey.li@linux.intel.com> wrote:
> 
>> On 2015/3/5 4:11, Ingo Molnar wrote:
>>>
>>> * Arjan van de Ven <arjan@linux.intel.com> wrote:
>>>
>>>> On 3/4/2015 1:50 AM, Borislav Petkov wrote:
>>>>> On Wed, Mar 04, 2015 at 12:43:08AM -0800, Arjan van de Ven wrote:
>>>>>>>
>>>>>>> Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code
>>>>>>> is a mistake.
>>>>>>
>>>>>> ideally, the presence of that flag in the firmware table will clear/set more global settings,
>>>>>> for example, having that flag should cause the 8042 input code to not probe for the 8042.
>>>>>>
>>>>>> for interrupts, there really ought to be a "apic first/only" mode, which is then used on
>>>>>> all modern systems (not just hw reduced).
>>>>>
>>>>> Do we need some sort of platform-specific querying interfaces now too,
>>>>> similar to cpu_has()? I.e., platform_has()...
>>>>>
>>>>> 	if (platform_has(X86_PLATFORM_REDUCED_HW))
>>>>> 		do stuff..
>>>>
>>>> more like
>>>>
>>>> platform_has(X86_PLATFORM_PIT)
>>>>
>>>> etc, one for each legacy io item
>>>
>>> Precisely. The main problem is the generic, 'lumps everything 
>>> together' nature of the acpi_gbl_reduced_hardware flag.
>>>
>>> (Like the big kernel lock lumped together all sorts of locking rules 
>>> and semantics.)
>>>
>>> Properly split out, feature-ish or driver-ish interfaces for PIT and 
>>> other legacy details are the proper approach to 'turn them off'.
>>>
>>>  - x86_platform is a function pointer driven, driver-ish interface.
>>>
>>>  - platform_has(X86_PLATFORM_IT) is a flag driven, feature-flag-ish
>>>    interface.
>>>
>>> Both are fine - for something as separate as the PIT (or the PIC) 
>>> it might make more sense to go towards a 'driver' interface 
>>> though, as modern drivers are (and will be) much different from 
>>> the legacy PIT.
>>>
>>> Whichever method is used, low level platforms can just switch them 
>>> on/off in their enumeration/detection routines, while the generic 
>>> code will have them enabled by default.
>>
>> Whichever method is used, we will face a problem how to determine 
>> PIT exists or not.
>>
>> When we enabled Bay Trail-T platform at the beginning, we were 
>> trying to make the code as generic as possible, and it works 
>> properly up to now. So we don't have a SUBARCH like 
>> X86_SUBARCH_INTEL_MID to use the platform specific functions. And 
>> for now I'm not quite sure it's a good idea to create one.
>>
>> If we make it as a flag driven, I don't know there is a flag in 
>> firmware better than ACPI HW reduced flag(Of course it's not good 
>> enough to cover all the cases). Or if we want to use platform info 
>> to turn on/off this flag, we'll have to maintain a platform list, 
>> which may be longer and more complicated than worth doing that.
> 
> Well, it's not nearly so difficult, because you already have a 
> platform flag: acpi_gbl_reduced_hardware.
> 
> What I object against is to infest generic codepaths with unreadable, 
> unrobust crap like:
> 
> +       if (acpi_gbl_reduced_hardware) {
> +               pr_info("Using NULL legacy PIC\n");
> +               legacy_pic = &null_legacy_pic;
> +       } else
> +               legacy_pic->init(0);
> 
> To solve that, add a small (early) init function (say 
> 'x86_reduced_hw_init()') that sets up the right driver
> selections if acpi_gbl_reduced_hardware is set:
> 
>  - in x86_reduced_hw_init() set 'legacy_pic' to 'null_legacy_pic'
> 
>  - clean up 'global_clock_event' handling: instead of a global 
>    variable, move its management into x86_platform_ops::get_clockevent()
>    and set the method to hpet/pit/abp/etc. specific handlers that
>    return the right clockevent device.
> 
>  - in your x86_reduced_hw_init() function add the hpet clockevent
>    device to x86_platform_ops::get_clockevent, overriding the default
>    PIT.
> 

>  - in x86_reduced_hw_init() set pm_power_off.
> 
>  - set 'reboot_type' and remove the acpi_gbl_reduced_hardware hack
>    from efi_reboot_required().
> 
I'll do more investigation above items but I want to leave at least
these two as the quirk today unless I am convinced I can do that because
from my understanding, UEFI runtime services should not be supported in
reduced hw mode.

> etc.
> 
> Just keep the generic init codepaths free of those random selections 
> based on global flags, okay?
>
Agree.

Thanks,
-Aubrey

> Thanks,
> 
> 	Ingo
> 
> 

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Alan Cox]

> Besides, the "ACPI reduced hardware" case is kind of a red herring here,
> because it most likely is not the only case when we'll want has_8259_pic()
> to return 0 (quite likely, we'll want that on all BayTrail-based systems,
> for example).

No. Only those with ACPI reduced firmware. For others you do want to
touch the 8259.

It *is* about ACPI reduced.

Alan

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Alan Cox]

> I'll do more investigation above items but I want to leave at least
> these two as the quirk today unless I am convinced I can do that because
> from my understanding, UEFI runtime services should not be supported in
> reduced hw mode.

What actually matters in this space is what Microsoft does. The spec is
unfortunately rather irrelevant. If MS do UEFI runtime services in
reduced hw mode then the firmware and system will do so.

Alan

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Li, Aubrey]

On 2015/3/5 20:42, Li, Aubrey wrote:
> On 2015/3/5 19:36, Ingo Molnar wrote:
>>
>> * Li, Aubrey <aubrey.li@linux.intel.com> wrote:
>>
>>> On 2015/3/5 4:11, Ingo Molnar wrote:
>>>>
>>>> * Arjan van de Ven <arjan@linux.intel.com> wrote:
>>>>
>>>>> On 3/4/2015 1:50 AM, Borislav Petkov wrote:
>>>>>> On Wed, Mar 04, 2015 at 12:43:08AM -0800, Arjan van de Ven wrote:
>>>>>>>>
>>>>>>>> Using 'acpi_gbl_reduced_hardware' flag outside the ACPI code
>>>>>>>> is a mistake.
>>>>>>>
>>>>>>> ideally, the presence of that flag in the firmware table will clear/set more global settings,
>>>>>>> for example, having that flag should cause the 8042 input code to not probe for the 8042.
>>>>>>>
>>>>>>> for interrupts, there really ought to be a "apic first/only" mode, which is then used on
>>>>>>> all modern systems (not just hw reduced).
>>>>>>
>>>>>> Do we need some sort of platform-specific querying interfaces now too,
>>>>>> similar to cpu_has()? I.e., platform_has()...
>>>>>>
>>>>>> 	if (platform_has(X86_PLATFORM_REDUCED_HW))
>>>>>> 		do stuff..
>>>>>
>>>>> more like
>>>>>
>>>>> platform_has(X86_PLATFORM_PIT)
>>>>>
>>>>> etc, one for each legacy io item
>>>>
>>>> Precisely. The main problem is the generic, 'lumps everything 
>>>> together' nature of the acpi_gbl_reduced_hardware flag.
>>>>
>>>> (Like the big kernel lock lumped together all sorts of locking rules 
>>>> and semantics.)
>>>>
>>>> Properly split out, feature-ish or driver-ish interfaces for PIT and 
>>>> other legacy details are the proper approach to 'turn them off'.
>>>>
>>>>  - x86_platform is a function pointer driven, driver-ish interface.
>>>>
>>>>  - platform_has(X86_PLATFORM_IT) is a flag driven, feature-flag-ish
>>>>    interface.
>>>>
>>>> Both are fine - for something as separate as the PIT (or the PIC) 
>>>> it might make more sense to go towards a 'driver' interface 
>>>> though, as modern drivers are (and will be) much different from 
>>>> the legacy PIT.
>>>>
>>>> Whichever method is used, low level platforms can just switch them 
>>>> on/off in their enumeration/detection routines, while the generic 
>>>> code will have them enabled by default.
>>>
>>> Whichever method is used, we will face a problem how to determine 
>>> PIT exists or not.
>>>
>>> When we enabled Bay Trail-T platform at the beginning, we were 
>>> trying to make the code as generic as possible, and it works 
>>> properly up to now. So we don't have a SUBARCH like 
>>> X86_SUBARCH_INTEL_MID to use the platform specific functions. And 
>>> for now I'm not quite sure it's a good idea to create one.
>>>
>>> If we make it as a flag driven, I don't know there is a flag in 
>>> firmware better than ACPI HW reduced flag(Of course it's not good 
>>> enough to cover all the cases). Or if we want to use platform info 
>>> to turn on/off this flag, we'll have to maintain a platform list, 
>>> which may be longer and more complicated than worth doing that.
>>
>> Well, it's not nearly so difficult, because you already have a 
>> platform flag: acpi_gbl_reduced_hardware.
>>
>> What I object against is to infest generic codepaths with unreadable, 
>> unrobust crap like:
>>
>> +       if (acpi_gbl_reduced_hardware) {
>> +               pr_info("Using NULL legacy PIC\n");
>> +               legacy_pic = &null_legacy_pic;
>> +       } else
>> +               legacy_pic->init(0);
>>
>> To solve that, add a small (early) init function (say 
>> 'x86_reduced_hw_init()') that sets up the right driver
>> selections if acpi_gbl_reduced_hardware is set:
>>
>>  - in x86_reduced_hw_init() set 'legacy_pic' to 'null_legacy_pic'
>>
>>  - clean up 'global_clock_event' handling: instead of a global 
>>    variable, move its management into x86_platform_ops::get_clockevent()
>>    and set the method to hpet/pit/abp/etc. specific handlers that
>>    return the right clockevent device.
>>
>>  - in your x86_reduced_hw_init() function add the hpet clockevent
>>    device to x86_platform_ops::get_clockevent, overriding the default
>>    PIT.
>>

how about this one?

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index b9e30da..70955d6 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1541,6 +1541,16 @@ int __init early_acpi_boot_init(void)
 	 */
 	early_acpi_process_madt();

+	/*
+	 * Override x86_init functions and bypass legacy pic
+	 * in hardware-reduced ACPI mode
+	 */
+	if (acpi_gbl_reduced_hardware) {
+		x86_init.timers.timer_init = x86_init_noop;
+		x86_init.irqs.pre_vector_init = x86_init_noop;
+		legacy_pic = &null_legacy_pic;
+	}
+
 	return 0;
 }

> 
>>  - in x86_reduced_hw_init() set pm_power_off.
>>
>>  - set 'reboot_type' and remove the acpi_gbl_reduced_hardware hack
>>    from efi_reboot_required().
>>
> I'll do more investigation above items but I want to leave at least
> these two as the quirk today unless I am convinced I can do that because
> from my understanding, UEFI runtime services should not be supported in
> reduced hw mode.
> 

If the above makes sense, I'll send poweroff and reboot change together
in a seperate patch.

Thanks,
-Aubrey

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Ingo Molnar]

* Li, Aubrey <aubrey.li@linux.intel.com> wrote:

> >>  - in x86_reduced_hw_init() set 'legacy_pic' to 'null_legacy_pic'
> >>
> >>  - clean up 'global_clock_event' handling: instead of a global 
> >>    variable, move its management into x86_platform_ops::get_clockevent()
> >>    and set the method to hpet/pit/abp/etc. specific handlers that
> >>    return the right clockevent device.
> >>
> >>  - in your x86_reduced_hw_init() function add the hpet clockevent
> >>    device to x86_platform_ops::get_clockevent, overriding the default
> >>    PIT.
> 
> how about this one?
> 
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index b9e30da..70955d6 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -1541,6 +1541,16 @@ int __init early_acpi_boot_init(void)
>  	 */
>  	early_acpi_process_madt();
> 
> +	/*
> +	 * Override x86_init functions and bypass legacy pic
> +	 * in hardware-reduced ACPI mode
> +	 */
> +	if (acpi_gbl_reduced_hardware) {
> +		x86_init.timers.timer_init = x86_init_noop;
> +		x86_init.irqs.pre_vector_init = x86_init_noop;
> +		legacy_pic = &null_legacy_pic;
> +	}

Sounds good to me, assuming it builds, boots and works! :-)

A couple of extra suggestions:

1)

I'd suggest moving it into its own dedicated, appropriately named 
static function, to make it clear that 'ACPI Reduced Hardware' init 
happens there.

2)

I'd also initialize it like this:

		x86_init.timers.timer_init	= x86_init_noop;
		x86_init.irqs.pre_vector_init	= x86_init_noop;
		legacy_pic			= &null_legacy_pic;

to make the noop patterns stand out better.

3)

Once all is said and done, please also make acpi_gbl_reduced_hardware 
a flag internal to the ACPI code, not exposed to and used by other 
bits of x86 code.

> If the above makes sense, I'll send poweroff and reboot change 
> together in a seperate patch.

Yeah, please send them in a single series though, so they form a 
logical group.

Thanks,

	Ingo

Re: [PATCH] x86: Bypass legacy PIC and PIT on ACPI hardware reduced platform

[Li, Aubrey]

On 2015/3/10 16:06, Ingo Molnar wrote:
> 
> * Li, Aubrey <aubrey.li@linux.intel.com> wrote:
> 
>>>>  - in x86_reduced_hw_init() set 'legacy_pic' to 'null_legacy_pic'
>>>>
>>>>  - clean up 'global_clock_event' handling: instead of a global 
>>>>    variable, move its management into x86_platform_ops::get_clockevent()
>>>>    and set the method to hpet/pit/abp/etc. specific handlers that
>>>>    return the right clockevent device.
>>>>
>>>>  - in your x86_reduced_hw_init() function add the hpet clockevent
>>>>    device to x86_platform_ops::get_clockevent, overriding the default
>>>>    PIT.
>>
>> how about this one?
>>
>> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
>> index b9e30da..70955d6 100644
>> --- a/arch/x86/kernel/acpi/boot.c
>> +++ b/arch/x86/kernel/acpi/boot.c
>> @@ -1541,6 +1541,16 @@ int __init early_acpi_boot_init(void)
>>  	 */
>>  	early_acpi_process_madt();
>>
>> +	/*
>> +	 * Override x86_init functions and bypass legacy pic
>> +	 * in hardware-reduced ACPI mode
>> +	 */
>> +	if (acpi_gbl_reduced_hardware) {
>> +		x86_init.timers.timer_init = x86_init_noop;
>> +		x86_init.irqs.pre_vector_init = x86_init_noop;
>> +		legacy_pic = &null_legacy_pic;
>> +	}
> 
> Sounds good to me, assuming it builds, boots and works! :-)

Yeah, it's verified on my ASUS-T100 machine.

> 
> A couple of extra suggestions:
> 
> 1)
> 
> I'd suggest moving it into its own dedicated, appropriately named 
> static function, to make it clear that 'ACPI Reduced Hardware' init 
> happens there.
> 
> 2)
> 
> I'd also initialize it like this:
> 
> 		x86_init.timers.timer_init	= x86_init_noop;
> 		x86_init.irqs.pre_vector_init	= x86_init_noop;
> 		legacy_pic			= &null_legacy_pic;
> 
> to make the noop patterns stand out better.

Thanks for the suggestions, I'll send the patch out soon.

> 
> 3)
> 
> Once all is said and done, please also make acpi_gbl_reduced_hardware 
> a flag internal to the ACPI code, not exposed to and used by other 
> bits of x86 code.
> 
>> If the above makes sense, I'll send poweroff and reboot change 
>> together in a seperate patch.
> 
> Yeah, please send them in a single series though, so they form a 
> logical group.

Execution flow:

->early_acpi_boot_init()
-->acpi_reduced_hw_init()
->reboot_init()
->acpi_sleep_init()
->efi_shutdown_init()

For poweroff, it will take no effect if we override pm_power_off during
reduced hardware initialization, because acpi_sleep_init() will override
it again to acpi_power_off.

For reboot, it's really a quirk to have to force reboot mode to be
EFI_RESET_WARM, so we can't just set the reboot type and done, there is
also a logic that DMI quirks table takes precedence over EFI quirk.

So, IMHO, we either need an EFI cross function called from ACPI to ask
EFI to reboot and poweroff system in reduced hw mode, or we need a copy
of acpi_gbl_reduced_hardware in EFI to do so.

What do you think?

Thanks,
-Aubrey