| Message ID | 1530157132-4598-2-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | [U-Boot,1/3] x86: acpi: Move APIs unrelated to ACPI tables generation to a separate library | expand |
On Thu, Jun 28, 2018 at 6:38 AM, Bin Meng <bmeng.cn@gmail.com> wrote: > write_acpi_tables() currently touches ACPI hardware to switch to > ACPI mode at the end. Move such operation out of this function, > so that it only does what the function name tells us. > { > board_final_cleanup(); > + struct acpi_fadt __maybe_unused *fadt; I guess this would be first line in the function. No? And personally I don't like to see __maybe_unused near to local variables (see also below). > +#ifdef CONFIG_HAVE_ACPI_RESUME > + fadt = acpi_find_fadt(); > > - if (fadt != NULL && gd->arch.prev_sleep_state == ACPI_S3) > + if (fadt && gd->arch.prev_sleep_state == ACPI_S3) > acpi_resume(fadt); > #endif > > write_tables(); > +#ifdef CONFIG_GENERATE_ACPI_TABLE > + fadt = acpi_find_fadt(); This sounds superfluous, we create tables ourselves, why do we need traverse again to find them? Looks like you would do something such as struct acpi_fadt *fadt /* maybe save to do as well: = acpi_find_fadt() */; ... fadt = write_tables(); //or any other way to get the pointer without traversing ... > + > + /* Don't touch ACPI hardware on HW reduced platforms */ > + if (fadt && !(fadt->flags & ACPI_FADT_HW_REDUCED_ACPI)) { > + /* > + * Other than waiting for OSPM to request us to switch to ACPI > + * mode, do it by ourselves, since SMI will not be triggered. > + */ > + enter_acpi_mode(fadt->pm1a_cnt_blk); > + }
Hi Andy, On Thu, Jun 28, 2018 at 3:57 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Jun 28, 2018 at 6:38 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >> write_acpi_tables() currently touches ACPI hardware to switch to >> ACPI mode at the end. Move such operation out of this function, >> so that it only does what the function name tells us. > >> { >> board_final_cleanup(); >> + struct acpi_fadt __maybe_unused *fadt; > > I guess this would be first line in the function. No? Yes, it should be the first line. Will fix. > And personally I don't like to see __maybe_unused near to local > variables (see also below). > Without __maybe_unused >> +#ifdef CONFIG_HAVE_ACPI_RESUME >> + fadt = acpi_find_fadt(); >> >> - if (fadt != NULL && gd->arch.prev_sleep_state == ACPI_S3) >> + if (fadt && gd->arch.prev_sleep_state == ACPI_S3) >> acpi_resume(fadt); >> #endif >> >> write_tables(); > >> +#ifdef CONFIG_GENERATE_ACPI_TABLE >> + fadt = acpi_find_fadt(); > > This sounds superfluous, we create tables ourselves, why do we need > traverse again to find them? > > Looks like you would do something such as > > struct acpi_fadt *fadt /* maybe save to do as well: = acpi_find_fadt() */; Do the assignment here only works for S3 path. > > ... > > fadt = write_tables(); //or any other way to get the pointer without traversing Let write_tables() return FADT address is odd. write_tables() is generic API for writing various configuration tables. > ... > >> + >> + /* Don't touch ACPI hardware on HW reduced platforms */ >> + if (fadt && !(fadt->flags & ACPI_FADT_HW_REDUCED_ACPI)) { >> + /* >> + * Other than waiting for OSPM to request us to switch to ACPI >> + * mode, do it by ourselves, since SMI will not be triggered. >> + */ >> + enter_acpi_mode(fadt->pm1a_cnt_blk); >> + } > Regards, Bin
diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index f7601b3..3a45677 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -206,16 +206,30 @@ __weak void board_final_cleanup(void) int last_stage_init(void) { board_final_cleanup(); + struct acpi_fadt __maybe_unused *fadt; -#if CONFIG_HAVE_ACPI_RESUME - struct acpi_fadt *fadt = acpi_find_fadt(); +#ifdef CONFIG_HAVE_ACPI_RESUME + fadt = acpi_find_fadt(); - if (fadt != NULL && gd->arch.prev_sleep_state == ACPI_S3) + if (fadt && gd->arch.prev_sleep_state == ACPI_S3) acpi_resume(fadt); #endif write_tables(); +#ifdef CONFIG_GENERATE_ACPI_TABLE + fadt = acpi_find_fadt(); + + /* Don't touch ACPI hardware on HW reduced platforms */ + if (fadt && !(fadt->flags & ACPI_FADT_HW_REDUCED_ACPI)) { + /* + * Other than waiting for OSPM to request us to switch to ACPI + * mode, do it by ourselves, since SMI will not be triggered. + */ + enter_acpi_mode(fadt->pm1a_cnt_blk); + } +#endif + return 0; } #endif diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c index e26c54d..e48c9b9 100644 --- a/arch/x86/lib/acpi_table.c +++ b/arch/x86/lib/acpi_table.c @@ -12,7 +12,6 @@ #include <dm/uclass-internal.h> #include <version.h> #include <asm/acpi/global_nvs.h> -#include <asm/acpi.h> #include <asm/acpi_table.h> #include <asm/ioapic.h> #include <asm/lapic.h> @@ -444,16 +443,6 @@ ulong write_acpi_tables(ulong start) acpi_rsdp_addr = (unsigned long)rsdp; debug("ACPI: done\n"); - /* Don't touch ACPI hardware on HW reduced platforms */ - if (fadt->flags & ACPI_FADT_HW_REDUCED_ACPI) - return current; - - /* - * Other than waiting for OSPM to request us to switch to ACPI mode, - * do it by ourselves, since SMI will not be triggered. - */ - enter_acpi_mode(fadt->pm1a_cnt_blk); - return current; }
write_acpi_tables() currently touches ACPI hardware to switch to ACPI mode at the end. Move such operation out of this function, so that it only does what the function name tells us. Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- arch/x86/cpu/cpu.c | 20 +++++++++++++++++--- arch/x86/lib/acpi_table.c | 11 ----------- 2 files changed, 17 insertions(+), 14 deletions(-)