[U-Boot] [PATCH 2/3] x86: acpi: Don't touch ACPI hardware in write_acpi_tables()

[Bin Meng]

Hi Andy,

On Thu, Jun 28, 2018 at 3:57 PM, Andy Shevchenko
<andy.shevchenko at gmail.com> wrote:
> On Thu, Jun 28, 2018 at 6:38 AM, Bin Meng <bmeng.cn at 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