Attention is currently required from: Felix Held. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61077 )
Change subject: soc/amd/sabrina: add new SoC as copy of soc/amd/cezanne ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
Patchset:
PS2:
I always wonder if this "sort-of duplicated" code is worth it. Naturally, all the code that actually ends up duplicated could be common code, and the sort-of duplicated code needs to be changed anyway, so there shouldn't be much gain from copy-pasting whole files.
https://review.coreboot.org/c/coreboot/+/61084/3/src/soc/amd/sabrina/gpio.c or most of https://review.coreboot.org/c/coreboot/+/61077/2/src/soc/amd/sabrina/chip.c or most of https://review.coreboot.org/c/coreboot/+/61086/3/src/soc/amd/sabrina/uart.c would be examples where I don't see how we could get rid of all remaining sort-of duplicated but not the same code. A lot of the common functions are moved to common and are then called from the SoC-specific code that is slightly different for each SoC; I'd say that that is the best approach right now.
Sorry, I guess you misunderstood. I didn't mean to say we should avoid all sort-of-duplicated code. We won't ever achieve that. Just the way to get some new sort-of-duplicated code, should that be copy-first or could we push the adapted version already. I don't know, so still wondering what would be better, theoretically, in case we'd at some point have no real / unwanted duplication left. So let's just hope we get in the position to ask this question at some point ;) but don't worry about it now.
I'd say that having certain things implemented in each SoC code makes it easier to understand; for example calling mp_init_with_smm somewhere from the cpu cluster's ops is easier to follow than calling it somewhere from some BOOT_STATE_INIT_ENTRY in some soc/vendor/common/block file despite the latter resulting in less code duplication.
Absolutely, yes ;) (I might add that I hate boot-state hooks because people constantly lose track of what they do when.)
-
Nico Huber (Code Review)