Attention is currently required from: Felix Held, Fred Reitberger, Jason Glenesk, Martin L Roth, Matt DeVillier, Nick Kochlowski, Varshit Pandya.
Martin Roth 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 making changes in specific areas:
1) Add the code to drivers 2) switch phoenix 3) switch birman 4) switch genoa 5) switch onyx 6) remove vendorcode
or something like that. We just prefer not to make changes in so many places at once unless it's required. Maybe it is in this case, but I don't think so...
File src/drivers/amd/opensil/mpio/chip.h:
https://review.coreboot.org/c/coreboot/+/85632/comment/9a851d67_e0824e5c?usp... : PS6, Line 8: void configure_mpio(void); Why does this go in chip.h and not in opensil.h?
File src/drivers/amd/opensil/mpio/chip.c:
https://review.coreboot.org/c/coreboot/+/85632/comment/92b64706_f42dfd62?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.
Looking through the entirety of the drivers subdirectory, I see one case out of maybe 50 where this is in chip.c.
File src/vendorcode/amd/opensil/Kconfig:
https://review.coreboot.org/c/coreboot/+/85632/comment/eedc5615_c05b8f2a?usp... : PS6, Line 28: ../../../../vendorcode '$(top)/src/vendorcode' ?
File src/vendorcode/amd/opensil/stub/mpio/chip.c:
https://review.coreboot.org/c/coreboot/+/85632/comment/5d9433e1_ffe03cb0?usp... : PS6, Line 6: "../../opensil.h" <src/vendorcode/amd/opensil/opensil.h>?