Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32649 )
Change subject: soc/amd/common: Create AcpiMmio functionality from stoneyridge ......................................................................
Patch Set 4:
(7 comments)
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/acpimmio/mm... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/acpimmio/mm... PS4, Line 147: for (i = sizeof(value) - 1 ; i >= 0 ; i--) : value = (value << 8) | biosram_read8(offset + i);
Yes, these should probably be updated to match pm_io_write16(). […]
I'll plan on a follow-on. This was mostly a direct move of existing code.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... File src/soc/amd/common/block/include/amdblocks/acpimmio.h:
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 21: void enable_acpimmio_decode(void);
I'm not going to object to having things better documented.
OK.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 25: TODO
Does it make sense to start with a Kconfig for this which is set to 0x4 by default and can be overri […]
The registers aren't really defined identically. I had started with a Kconfig for indicating new vs. old, and code that would write the proper offset. I ended up removing that in favor of a TODO. I decided I didn't really like having dead code in soc.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 37: All products do not support all blocks below, however AMD has avoided : * redefining addresses and consumes new ranges as necessary.
... define a structure with offsets to each of the blocks below and each platform can set only the offsets/blocks that it actually supports and rest can be set to -1.
I don't yet see the value of that vs. the extra complexity. If I understand your suggestion, the platform (e.g. stoneyridge) would be responsible for loading each structure member with an offset or -1. However, in most cases, it's the platform that will be making the calls. (And if not, IMO we should consider a way to abstract it to that end.) And then for that effort, the caller should do some error checking.
How about this: Let's leave this in iomap.h and just define the ones that are used in an SOC.
The problem there is the common functions don't build, e.g. the xhci_pm registers aren't in Family 17h. So if iomap.h defines only bases of existing blocks, then mmio_util.c won't build. Could fix that with ifdefs, but not my favorite. Protect everything that's not common using CONFIG()?
I've toyed with the idea of reducing the function calls to a set of tokened/concatenated macros instead. I believe if a block's base was left out of iomap.h, it shouldn't cause a build error unless someone tries to use a macro that isn't applicable.
BTW I'm not opposed to making the system more bulletproof, but I think I would prefer any solution in a follow-up and keep this move as clean as possible.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 44: AMD_SB_ACPI_MMIO_ADDR
From my understanding, they have not changed in the last 10 years, since the concept was developed.
In the older implementations it was configurable. Not now, and I have no reason to believe in the future. However, for the older implementations, this was the default address, and I'm not aware of anyone that ever changed it. If writing code to support the older, I would either assume it was still in its reset value (it seems that's what existing code still does) or explicitly load it with the defined value.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 45: 0xfed80000
Set these as (AMD_SB_ACPI_MMIO_ADDR + 0) (AMD_SB_ACPI_MMIO_ADDR + 0x200) to indicate that they are o […]
That was my preference and I started out that way. However that extra complexity made the ASL blow up when using the values there.
https://review.coreboot.org/#/c/32649/4/src/soc/amd/common/block/include/amd... PS4, Line 65: ACPIMMIO_AOAC_BASE
We should only move things when it makes sense and makes the code cleaner. […]
Correct, all blocks are not available in any soc. Examples are that XHCIPM isn't in Family 17h, and surely not prior to the devices supporting xHCI. Another example is that every system uses either (a) older GPIO block at 0xfed80100 which isn't part of this patch or (b) the 3 banks beginning at 0xfed81500.
My current thinking is that I'd set the bar pretty high for common code calling these functions; specifically only when the functionality hasn't changed over time. Otherwise, it should be from soc/ and to a much lesser extent from mainboard/
Also, for an either/or, it's easy enough force a build error, e.g. if you select both the old GPIOs and banked GPIOs.