Attention is currently required from: Angel Pons, Annie Chen, Arthur Heymans, David Hendricks, Hung-Wei Chen, Jonathan Zhang, Lean Sheng Tan, Paul Menzel.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75598?usp=email )
Change subject: mb/inventec: Add Intel SPR server board Inventec Transformers ......................................................................
Patch Set 8: Code-Review+2
(8 comments)
Patchset:
PS5:
Hi Felix, […]
I'm sorry for coming back to this late. I just wanted to say here that I am absolutely fine with having this as a separate mainboard. I didn't mean to kick off a huge discussion. Not at all.
I proposed the idea for two reasons:
* I am not really into OCP, so I don't know how much the mainboards under the OCP users differ. I just saw this mainboard code and many parts are similar to the CRB one. So I thought other OCP mainbaords might be similar as well and you all could take advantage of a common base / common mainboard, which wasn't even meant to be done now. However, it was just an idea how you could take much more advantage of open source.
* Sometimes it happens that we do treewide changes to unify or clean up things. The more mainboards we have in the tree, the more work someone needs to put into it. With the variant concept, it might be possible to reduce the work for the upstream developers. Though, as I said before, it's a case-by-case thing and it's totally fine to separate mainboards.
However, thanks a lot for your contribution and for putting in the effort!
Patchset:
PS8: Just a couple of nits and a question about some methods, which are confusing to me. Otherwise it looks good to me!
File src/mainboard/inventec/transformers/Kconfig:
https://review.coreboot.org/c/coreboot/+/75598/comment/b5130645_c9f225de : PS8, Line 24: string Please remove, no need to to redefine the type
https://review.coreboot.org/c/coreboot/+/75598/comment/82c2c9bc_a09e6bff : PS8, Line 28: string same here
https://review.coreboot.org/c/coreboot/+/75598/comment/ec185198_e8a7000c : PS8, Line 32: string same
https://review.coreboot.org/c/coreboot/+/75598/comment/65455655_fab5e7c5 : PS8, Line 36: int same
File src/mainboard/inventec/transformers/devicetree.cb:
PS8: Please use tabs instead of spaces in lines 9-11 and 25-39. Also, if a device does not have config options in its scope, then please move the end keyword into the same line.
File src/mainboard/inventec/transformers/ramstage.c:
https://review.coreboot.org/c/coreboot/+/75598/comment/b2abc913_52f99eca : PS8, Line 10: #define SLOT_ID_LEN 2 : static char slot_id_str[SLOT_ID_LEN]; : extern uint32_t heci_fw_sts(void); : extern uint32_t heci_cse_normal(void); : extern uint32_t heci_cse_done(void); : : //extern void cse_init(uintptr_t tempbar); : I just noticed these declarations. Are they needed? As far as I can see, only Apollo Lake uses these.