Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43418 )
Change subject: soc/amd/picasso: use FADT devicetree configuration options ......................................................................
Patch Set 1:
(2 comments)
I do not like the inherent redundancy of specifying such options as-is in the devicetree.
https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c@... PS1, Line 129: fadt->iapc_boot_arch = cfg->fadt_boot_arch; /* legacy free default */ What controls 8042 timer enablement? Maybe hook things up so that this is automatically configured depending on whether the legacy timer is enabled?
https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c@... PS1, Line 132: fadt->flags = cfg->fadt_flags; I don't think repeating all the FADT flags on each devicetree is a good idea. Sure, I understand there will be very few users of this SoC, but it's error-prone. For example, ACPI_FADT_WBINVD is mandatory, ACPI_FADT_RESET_REGISTER depends on options set up below (so it should always be set) and most of the other options seem to be fixed. Maybe do like some Intel southbridges do with the `docking_supported` options instead?