Hi Andy,
On Thu, Jun 28, 2018 at 3:57 PM, Andy Shevchenko
<andy.shevche...@gmail.com> wrote:
> On Thu, Jun 28, 2018 at 6:38 AM, Bin Meng <bmeng...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
