Attention is currently required from: Fred Reitberger, Jason Glenesk, Martin L Roth, Martin Roth, Matt DeVillier, Nick Kochlowski, Varshit Pandya.
Felix Held has posted comments on this change by Nick Kochlowski. ( https://review.coreboot.org/c/coreboot/+/85632?usp=email )
Change subject: drivers/amd/opensil/mpio: Factor out common MPIO symbols from vendorcode ......................................................................
Patch Set 6:
(5 comments)
Patchset:
PS6:
I wouldn't insist on it, but in my opinion, this would be better split into a few different patches […]
splitting this change into multiple ones would probably be more difficult than necessary, since this is very intertwined. at least to me this patch still does one thing and it's still a reviewable patch size, so i'd keep it as one patch
File src/drivers/amd/opensil/mpio/chip.c:
https://review.coreboot.org/c/coreboot/+/85632/comment/bfa36c21_5ad1e8b1?usp... : PS6, Line 8: struct chip_operations drivers_amd_opensil_mpio_ops = { : .name = "AMD MPIO", : };
Why do these go in chip.c? Obviously the *can* go here, but that's not typically the case. […]
i'd say that the chip.c is the right place for this specific one. if you want to rename the whole file to mpio.c, i won't oppose to that, but i don't see a strong reason to do that. beware that this specific chip driver works differently compared to most other chip drivers, but this was the cleanest solution i was able come up with. the previous solution ran this code not exactly at the time where it should have done that; now that's not a problem any more. it is indeed a bit odd chip driver though where the code doesn't get called from the chip ops, but the soc code calls into this. iirc the previous timing issue was when there were some other devices or chips below the pci device below the mpio chip, but since the soc code just calls this at the right time and the code then processes all mpio chip information, this should just work in any case
File src/vendorcode/amd/opensil/Kconfig:
https://review.coreboot.org/c/coreboot/+/85632/comment/8154aa16_a48b188e?usp... : PS6, Line 28: ../../../../vendorcode
'$(top)/src/vendorcode' ?
if this works, that would be preferrable. not sure if that will work, but would be great if you can test this
File src/vendorcode/amd/opensil/genoa_poc/mpio/chip.c:
https://review.coreboot.org/c/coreboot/+/85632/comment/a691452a_a8d46451?usp... : PS6, Line 134: // /* */ comment style
https://review.coreboot.org/c/coreboot/+/85632/comment/7a353b75_177a5268?usp... : PS6, Line 202: } i think this is missing the final newline