Duplicated code between mainboards isn't a big issue in my opinion. It allows the boards to be customized without worrying about other companies' mainboards. We've tried to make mainboards as small as we can, and we can keep refactoring things out where it makes sense.
If some common code fits under the SoC, that's great, and I'm all for moving it there, but let's not force the burden of that refactoring work onto inexperienced mainboard maintainers. Doing so just encourages vendors to keep their mainboards in private repositories - the opposite of what we should be working for. Even if this means that it doesn't get refactored and gets a bit out-of-date, I find that preferable to making contributors (more) frustrated about getting their boards accepted. CRBs are (as the name says) reference boards and it should be absolutely fine to duplicate their code when another mainboard vendor uses the CRB as their base - that's what the CRB is for - as an example. Forcing the board to be under the intel vendor directory tells me that intel is responsible for the board, when that isn't the case. In my opinion, Mainboards should be free to customize anything and everything in their directories without having to worry about what other mainboards are affected. I think for the most part, variants should be reserved for duplicates that are owned/maintained by the same vendor. Whitelabel vendors like Clevo can be an exception to this, but the chip vendors' CRBs should not be forced as a baseboard for some other company's design. Respectfully,Martin Jun 19, 2023, 07:08 by th3fan...@gmail.com: > Hi Paul, list, > > On Thu, Jun 8, 2023, 16:16 Paul Menzel <> pmen...@molgen.mpg.de> > wrote: > >> [Annie, Yiwei, I only added you to Cc. It’d be great if you made sure >> that all involved people are subscribed to the coreboot mailing list.] >> >> Dear coreboot folks, >> >> >> Two server boards based on Intel Archer City board, commit 30e743e7cc7f >> (mb/ibm: Add 4 SPR sockets server board IBM SBP1) [1], were pushed to >> Gerrit for review: >> >> 1. mb/inventec: Add Intel SPR server board Inventec Transformers [2] >> Change-Id: Ic9d99c3aadaa9f69e6d14d4b1a6c5157f5590684 >> 2. bytedance/bd_egs (Eaglestream) [3] >> Change-Id: I091bc78e39cd76b3c6b9a10a1fcf58e9d671ef5d >> >> Some code seems to be copied – like bootblock.c [4] – and almost >> identical to the reference platform. To avoid future maintenance burden, >> could more knowledgeable people comment, if server boards differ >> drastically so separate boards are justified or if they should be made >> variants. >> > > A variant setup with boards from different vendors sounds like a maintenance > nightmare. We still have a common place to put the redundant code, though: in > chipset (SoC) code. The bootblock LPC decode can definitely be factored out. > > That being said, it may be easier to factor things out after these three > board ports have been submitted. The boot > > >> I also noticed `mainboard_config_iio()`. Should that be moved to the >> devicetree? >> > > If the settings are only used in ramstage (where the devicetree is R/W), they > could be moved to the devicetree. However, if these settings are used in > romstage, the devicetree would negatively impact runtime-dependent > configuration (e.g. when using straps to control IIO bifurcation depending on > slot occupancy), as the devicetree cannot be updated at runtime in romstage > (or earlier). > > >> Kind regards, >> >> Paul >> >> >> [1]: >> https://review.coreboot.org/c/coreboot/+/73392 >> "mb/ibm: Add 4 SPR sockets server board IBM SBP1" >> [2]: >> https://review.coreboot.org/c/coreboot/+/75598 >> "mb/inventec: Add Intel SPR server board Inventec Transformers" >> [3]: >> https://review.coreboot.org/c/coreboot/+/75722 >> "mb/bytedance: Add 2 SPR sockets server board bd_egs" >> [4]: >> >> >> https://review.coreboot.org/c/coreboot/+/75598/5/src/mainboard/inventec/transformers/bootblock.c#18 >> _______________________________________________ >> coreboot mailing list -- >> coreboot@coreboot.org >> To unsubscribe send an email to >> coreboot-le...@coreboot.org >> > > Best regards, > Angel > > _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
