[coreboot-gerrit] Change in coreboot[master]: Revert "amd/pi/hudson: Move ACPI IO registers"

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/19454 )

Change subject: Revert "amd/pi/hudson: Move ACPI IO registers"
......................................................................


Patch Set 2: Code-Review+2

Maybe change the commit message.  It's a PM register and not a PCI config register.

It needs to be reverted though.  Marc, maybe we can put a not-for-merge patch in the stack until we can figure out the best way to work around the conflict with the EC’s address on Kahlee.

>So what you are saying is that HUDSON_ACPI_IO_BASE needs to be equal to DFLT_ACPI_PM1_EVT_BLOCK_ADDRESS?

> Reading the source I would say this is the case. Eg. acpi_get_sleep_type() requires that vendorcode has already setup ACPI PM decode at HUDSON_ACPI_IO_BASE, but programming was for DFLT_ACPI_PM1_EVT_BLOCK_ADDRESS.

I agree.  Maybe an improvement in hudson source would be to check the registers’ base address before using them.

> Is there an option to configure what the blob does?

> Runtime? Not without new PI blob builds as far as I can say. That build-time constant is also referenced elsewhere in the blob so simply overwriting those ACPI PM base registers does not look like a feasible solution for this. Easiest way out would appear to make this IO base change for Stoney only together with a new PI build.

Also agree.  It’s frustrating that was made as a build configuration and no way to override it.  This is left over from how AGESA was originally put together.  For example:
 #ifdef BLDCFG_ACPI_PM1_EVT_BLOCK_ADDRESS
  #define CFG_ACPI_PM1_EVT_BLOCK_ADDRESS    BLDCFG_ACPI_PM1_EVT_BLOCK_ADDRESS
 #else
  #define CFG_ACPI_PM1_EVT_BLOCK_ADDRESS DFLT_ACPI_PM1_EVT_BLOCK_ADDRESS
 #endif

Kyosti, a bit of a side topic, but would it improve the agesa-based systems to not require #define values to stay in sync?  For example, hudson.h ACPI_PM_EVT_BLK and mainboard buildOpts.c DFLT_ACPI_PM1_EVT_BLOCK_ADDRESS, etc.  What about including hudson.h and using its defined values for the DFLTs?

-- 
To view, visit https://review.coreboot.org/19454
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I36700e49e21cc675e8e22b06efffb40e9c1e4236
Gerrit-PatchSet: 2
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki at gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki at gmail.com>
Gerrit-Reviewer: Marc Jones <marc at marcjonesconsulting.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd at gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: No