Attention is currently required from: Jason Glenesk, Raul Rangel, Matt DeVillier, Arthur Heymans, Fred Reitberger, Karthik Ramasubramanian, Felix Held.
Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68584 )
Change subject: soc/amd: Define AMD_FWM_POSITION config item ......................................................................
Patch Set 4:
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/68584/comment/fb4b5ca0_e7e97d5f PS4, Line 11: Eventhough Nit: Even though
https://review.coreboot.org/c/coreboot/+/68584/comment/4c633d49_e2069975 PS4, Line 12: we are not sure if : it is correct It's been working for a number of years now, so I don't think this is true. If you just put the 6 values into the formula, you can show that it's correct. It might be better to say that it's difficult to read, understand, maintain, etc.
Patchset:
PS4: Can we move this patch off of the patch train?
It doesn't depend on the rest of the train, and having it in a train tends to keep it from being merged until everything earlier in the train is merged.
This is really a general recommendation - Don't put things in a train unless required.
File src/soc/amd/common/block/psp/Kconfig:
https://review.coreboot.org/c/coreboot/+/68584/comment/15a32650_6a654ee7 PS4, Line 60: default 5 if BOARD_ROMSIZE_KB_16384 Note that these are AMD's recommended defaults, but for ChromeOS, we won't want to use position 5 even when using a 16MiB chip because this is in the RW region. We might want to add a default for any google devices, then we can get rid of that setting from their Kconfig files.
Obviously this could be done in a follow-on patch.
https://review.coreboot.org/c/coreboot/+/68584/comment/b55c3f88_bf382388 PS4, Line 87: default 0xFFFA0000 if AMD_FWM_POSITION_INDEX = 0 : default 0xFFF20000 if AMD_FWM_POSITION_INDEX = 1 : default 0xFFE20000 if AMD_FWM_POSITION_INDEX = 2 : default 0xFFC20000 if AMD_FWM_POSITION_INDEX = 3 : default 0xFF820000 if AMD_FWM_POSITION_INDEX = 4 : default 0xFF020000 if AMD_FWM_POSITION_INDEX = 5
Unfortunately amdfwtool expects it to be x86 memory mapped space. […]
We also need to look ahead to 32 & 64MiB flash sizes and mapping the flash to the area over 4GiB so that we can access more than 16MiB at a time. That would also mean 64-bit coreboot builds.
Obviously those are both larger tasks and not needed yet, though they might be needed on Morgana or Glinda platforms.
-
Martin L Roth (Code Review)