[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.
I also noticed `mainboard_config_iio()`. Should that be moved to the devicetree?
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/tran...
Hi Paul, Thanks for the mail list, the review commit I will reply at the commit directly.
BR, Annie
-----Original Message----- From: Paul Menzel pmenzel@molgen.mpg.de Sent: Thursday, June 8, 2023 10:15 PM To: coreboot@coreboot.org Cc: Chen.AnnieET 陳恩婷 TAO Chen.AnnieET@inventec.com; Yiwei Tang tangyiwei.2022@bytedance.com Subject: How to best handle new Intel Saphire Rapids server boards based on Intel Archer City?
[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.
I also noticed `mainboard_config_iio()`. Should that be moved to the devicetree?
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/tran...
Hi Paul, list,
On Thu, Jun 8, 2023, 16:16 Paul Menzel pmenzel@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:
- mb/inventec: Add Intel SPR server board Inventec Transformers [2] Change-Id: Ic9d99c3aadaa9f69e6d14d4b1a6c5157f5590684
- 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
[4]:
https://review.coreboot.org/c/coreboot/+/75598/5/src/mainboard/inventec/tran... _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Best regards, Angel
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 th3fanbus@gmail.com:
Hi Paul, list,
On Thu, Jun 8, 2023, 16:16 Paul Menzel <> pmenzel@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/tran...
coreboot mailing list -- >> coreboot@coreboot.org To unsubscribe send an email to >> coreboot-leave@coreboot.org
Best regards, Angel
Dear Martin,
Thank you very much for taking the time to answer.
Am 19.06.23 um 22:31 schrieb Martin Roth:
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.
Are you talking in general or in this specific SRP boards. As stated by others, a lot of code was copy-pasted.
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.
I haven’t seen any frustration. Do you know more? I agree, nobody should be frustrated. The question has to be answered though, who is going to do that general maintenance work – I think Kyösti raised a similar question a few weeks back. I welcome all new contributors, but it has to be clear, that coreboot – especially for commercial offerings – needs and expects some kind of maintenance commitment. I am pretty sure, the vendors are going to understand the benefits. It just has to be communicated transparently.
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.
Isn’t `MAINTAINERS` the source for the responsibilities?
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.
Well, that is the thing to discuss. What differences are there going to be and can variants be extended or a new model be found to make it easier to maintain the hopefully vast number of new server boards?
Maybe Angel’s suggestion to submit the boards and then refactor is good. The other way would be to try variants first, and if that does not work, split them out. I’d welcome, that there is some commitment for further work on that though, and I try to avoid that the AGESA experience repeats, where coreboot didn’t want to frustrate AMD and trusted AMD’s promises, but, due to whatever reasons, it only resulted in frustration in the project itself.
Kind regards,
Paul
On 19.06.23 23:01, Paul Menzel wrote:
Am 19.06.23 um 22:31 schrieb Martin Roth:
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.
Are you talking in general or in this specific SRP boards. As stated by others, a lot of code was copy-pasted.
I don't see this. Maybe I'm too tired, but I only see about 500 lines in `archercity_crb/`. Maybe 300 of them being general boilerplate that one can't escape when adding a new board. There's an IIO header file (142 lines) that looks board specific.
The only exception seems to be the already mentioned mainboard_config_ iio() function which looks like something that belongs into platform code (it looks generic with only a few numbers hardcoded that could be added to a struct as well).
After all, if I'm not looking at the wrong branch, this looks cleaner than average for coreboot (wrt. copied code, I didn't look at the individual patches).
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.
I haven’t seen any frustration. Do you know more? I agree, nobody should be frustrated. The question has to be answered though, who is going to do that general maintenance work – I think Kyösti raised a similar question a few weeks back. I welcome all new contributors, but it has to be clear, that coreboot – especially for commercial offerings – needs and expects some kind of maintenance commitment. I am pretty sure, the vendors are going to understand the benefits. It just has to be communicated transparently.
I agree in general, but in this particular case it seems overzealous. We better save our energy until the next entire SoC code gets copied. Or for the review of these new boards. It seems like a good opportu- nity to teach people about coreboot.
Nico
There have always been two approaches to new mainboards in coreboot.
The first, "copy and convert", is to take the most similar board, copy the code, and then change it. I believe this is the strategy paul is unhappy with.
The second, "embrace and extend", is to modify existing board in place to support a new board.
The question of "copy and convert" vs "extend and embrace" has come up many times in the last 23 years. There are no ideal answers, we have found.
When the boards are variants from a single vendor, it can work well. chromebook boards are a good example. One vendor will (we hope) make sure that, as new variants are added, they do not break old variants.
We've had trouble over the years merging boards from different vendors. Even in the simple case, with similar boards from more than one vendor, a merged board will look like the union, with a greatly expanded set of build options. But in some early coreboot experience, e.g. with Opteron ca 2004, we found that some options were incompatible. CKE was a big pain.
In the earliest days, we did make an effort to have common code for similar boards. Problems arose as boards from one vendor changed in ways incompatible with other vendors. We saw this from the beginning with boards that used SiS 630 chipsets, which led to a panic debug session for SC 2000 as we dealt with a new board, allegedly compatible with an earlier board, which had some key differences that could not be accommodated in a single board source tree. We went with copy and convert.
I think it's great to have as much common code as possible. But calling an inventec board and a bytedance board variants just because they happen to use SPR? I think that's a mistake.
On Mon, Jun 19, 2023 at 2:02 PM Paul Menzel pmenzel@molgen.mpg.de wrote:
Dear Martin,
Thank you very much for taking the time to answer.
Am 19.06.23 um 22:31 schrieb Martin Roth:
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.
Are you talking in general or in this specific SRP boards. As stated by others, a lot of code was copy-pasted.
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.
I haven’t seen any frustration. Do you know more? I agree, nobody should be frustrated. The question has to be answered though, who is going to do that general maintenance work – I think Kyösti raised a similar question a few weeks back. I welcome all new contributors, but it has to be clear, that coreboot – especially for commercial offerings – needs and expects some kind of maintenance commitment. I am pretty sure, the vendors are going to understand the benefits. It just has to be communicated transparently.
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.
Isn’t `MAINTAINERS` the source for the responsibilities?
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.
Well, that is the thing to discuss. What differences are there going to be and can variants be extended or a new model be found to make it easier to maintain the hopefully vast number of new server boards?
Maybe Angel’s suggestion to submit the boards and then refactor is good. The other way would be to try variants first, and if that does not work, split them out. I’d welcome, that there is some commitment for further work on that though, and I try to avoid that the AGESA experience repeats, where coreboot didn’t want to frustrate AMD and trusted AMD’s promises, but, due to whatever reasons, it only resulted in frustration in the project itself.
Kind regards,
Paul _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Thanks again everyone for chipping in, this was a pleasant discussion (a bit surprise to me TBH :D ) I agree with Nico and Ron, and also with Angel's suggestion. For now, we can let the MB code go in first, and if there is something in common we can always restructure it, like we used to before. But if I understand correctly on how usually ODM/OEM boards evolves, the first generation will usually be much closer to Intel CRB, but there will be a few more variants of MB to be chucked out later on under same vendor, so it makes perfect sense to let it host under individual vendor folder.
On Tue, 20 Jun 2023 at 00:47, ron minnich rminnich@gmail.com wrote:
There have always been two approaches to new mainboards in coreboot.
The first, "copy and convert", is to take the most similar board, copy the code, and then change it. I believe this is the strategy paul is unhappy with.
The second, "embrace and extend", is to modify existing board in place to support a new board.
The question of "copy and convert" vs "extend and embrace" has come up many times in the last 23 years. There are no ideal answers, we have found.
When the boards are variants from a single vendor, it can work well. chromebook boards are a good example. One vendor will (we hope) make sure that, as new variants are added, they do not break old variants.
We've had trouble over the years merging boards from different vendors. Even in the simple case, with similar boards from more than one vendor, a merged board will look like the union, with a greatly expanded set of build options. But in some early coreboot experience, e.g. with Opteron ca 2004, we found that some options were incompatible. CKE was a big pain.
In the earliest days, we did make an effort to have common code for similar boards. Problems arose as boards from one vendor changed in ways incompatible with other vendors. We saw this from the beginning with boards that used SiS 630 chipsets, which led to a panic debug session for SC 2000 as we dealt with a new board, allegedly compatible with an earlier board, which had some key differences that could not be accommodated in a single board source tree. We went with copy and convert.
I think it's great to have as much common code as possible. But calling an inventec board and a bytedance board variants just because they happen to use SPR? I think that's a mistake.
On Mon, Jun 19, 2023 at 2:02 PM Paul Menzel pmenzel@molgen.mpg.de wrote:
Dear Martin,
Thank you very much for taking the time to answer.
Am 19.06.23 um 22:31 schrieb Martin Roth:
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.
Are you talking in general or in this specific SRP boards. As stated by others, a lot of code was copy-pasted.
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.
I haven’t seen any frustration. Do you know more? I agree, nobody should be frustrated. The question has to be answered though, who is going to do that general maintenance work – I think Kyösti raised a similar question a few weeks back. I welcome all new contributors, but it has to be clear, that coreboot – especially for commercial offerings – needs and expects some kind of maintenance commitment. I am pretty sure, the vendors are going to understand the benefits. It just has to be communicated transparently.
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.
Isn’t `MAINTAINERS` the source for the responsibilities?
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.
Well, that is the thing to discuss. What differences are there going to be and can variants be extended or a new model be found to make it easier to maintain the hopefully vast number of new server boards?
Maybe Angel’s suggestion to submit the boards and then refactor is good. The other way would be to try variants first, and if that does not work, split them out. I’d welcome, that there is some commitment for further work on that though, and I try to avoid that the AGESA experience repeats, where coreboot didn’t want to frustrate AMD and trusted AMD’s promises, but, due to whatever reasons, it only resulted in frustration in the project itself.
Kind regards,
Paul _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Thanks everyone for this great discussion. I looked at making Transformers a variant of Archer City CRB, but came to the conclusion that others foresaw which is that a multi-vendor variant model is not a good fit in this case.
In this case the SOC support is new to the tree and we have boards developed in parallel by multiple entities, so it's unsurprising to see some redundant bits. As Angel mentioned it will be easier to refactor portions of the code with the large patches merged in a buildable and (hopefully) usable/testable state.
On Tue, Jun 20, 2023 at 1:51 AM Lean Sheng Tan sheng.tan@9elements.com wrote:
Thanks again everyone for chipping in, this was a pleasant discussion (a bit surprise to me TBH :D ) I agree with Nico and Ron, and also with Angel's suggestion. For now, we can let the MB code go in first, and if there is something in common we can always restructure it, like we used to before. But if I understand correctly on how usually ODM/OEM boards evolves, the first generation will usually be much closer to Intel CRB, but there will be a few more variants of MB to be chucked out later on under same vendor, so it makes perfect sense to let it host under individual vendor folder.
On Tue, 20 Jun 2023 at 00:47, ron minnich rminnich@gmail.com wrote:
There have always been two approaches to new mainboards in coreboot.
The first, "copy and convert", is to take the most similar board, copy the code, and then change it. I believe this is the strategy paul is unhappy with.
The second, "embrace and extend", is to modify existing board in place to support a new board.
The question of "copy and convert" vs "extend and embrace" has come up many times in the last 23 years. There are no ideal answers, we have found.
When the boards are variants from a single vendor, it can work well. chromebook boards are a good example. One vendor will (we hope) make sure that, as new variants are added, they do not break old variants.
We've had trouble over the years merging boards from different vendors. Even in the simple case, with similar boards from more than one vendor, a merged board will look like the union, with a greatly expanded set of build options. But in some early coreboot experience, e.g. with Opteron ca 2004, we found that some options were incompatible. CKE was a big pain.
In the earliest days, we did make an effort to have common code for similar boards. Problems arose as boards from one vendor changed in ways incompatible with other vendors. We saw this from the beginning with boards that used SiS 630 chipsets, which led to a panic debug session for SC 2000 as we dealt with a new board, allegedly compatible with an earlier board, which had some key differences that could not be accommodated in a single board source tree. We went with copy and convert.
I think it's great to have as much common code as possible. But calling an inventec board and a bytedance board variants just because they happen to use SPR? I think that's a mistake.
On Mon, Jun 19, 2023 at 2:02 PM Paul Menzel pmenzel@molgen.mpg.de wrote:
Dear Martin,
Thank you very much for taking the time to answer.
Am 19.06.23 um 22:31 schrieb Martin Roth:
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.
Are you talking in general or in this specific SRP boards. As stated by others, a lot of code was copy-pasted.
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.
I haven’t seen any frustration. Do you know more? I agree, nobody should be frustrated. The question has to be answered though, who is going to do that general maintenance work – I think Kyösti raised a similar question a few weeks back. I welcome all new contributors, but it has to be clear, that coreboot – especially for commercial offerings – needs and expects some kind of maintenance commitment. I am pretty sure, the vendors are going to understand the benefits. It just has to be communicated transparently.
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.
Isn’t `MAINTAINERS` the source for the responsibilities?
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.
Well, that is the thing to discuss. What differences are there going to be and can variants be extended or a new model be found to make it easier to maintain the hopefully vast number of new server boards?
Maybe Angel’s suggestion to submit the boards and then refactor is good. The other way would be to try variants first, and if that does not work, split them out. I’d welcome, that there is some commitment for further work on that though, and I try to avoid that the AGESA experience repeats, where coreboot didn’t want to frustrate AMD and trusted AMD’s promises, but, due to whatever reasons, it only resulted in frustration in the project itself.
Kind regards,
Paul _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
(sorry if you receive this twice)
David Hendricks wrote:
it will be easier to refactor portions of the code with the large patches merged in a buildable and (hopefully) usable/testable state.
That's pretty weak sauce and I think you all know deep down.
Who pays for refactoring? Probably someone else.
That's unsustainable for the project.
It externalizes refactoring cost from those creating the initial mainboard ports but that's not how any platform code can grow into something well-engineered and versatile.
One can certainly argue that well-engineered is just too costly for our community to strive for but while that does seem the prevailing group think I have to say I would consider it a sad resignation.
//Peter
The approach in the last 24 years (of this unsustainable project :-) has been to get several mainboards of a type, and, once we have them, try to work out what code is truly common and what code is similar but not truly common.
Code that is truly common can then be factored out into places such as src/lib. Code then migrates out of specific mainboards and into common spaces. This progression has happened many times.
And, yes, no question, this is an activity that likely occurs less than it should. Such is our industry.
It is not possible to know, a priori, what those common pieces will be. So we are left with this admittedly non-ideal situation.
There is the further risk (this has happened) that a seemingly harmless change is discovered to break some board, 1.5 years later. This has happened. To me.
As has been pointed out, always in motion, the future is.
On Tue, Jun 20, 2023 at 4:08 PM Peter Stuge peter@stuge.se wrote:
(sorry if you receive this twice)
David Hendricks wrote:
it will be easier to refactor portions of the code with the large patches merged in a buildable and (hopefully) usable/testable state.
That's pretty weak sauce and I think you all know deep down.
Who pays for refactoring? Probably someone else.
That's unsustainable for the project.
It externalizes refactoring cost from those creating the initial mainboard ports but that's not how any platform code can grow into something well-engineered and versatile.
One can certainly argue that well-engineered is just too costly for our community to strive for but while that does seem the prevailing group think I have to say I would consider it a sad resignation.
//Peter
ron minnich wrote:
And, yes, no question, this is an activity that likely occurs less than it should. Such is our industry.
Such is project policy. Maybe because it's the lowest common denominator in industry.
It is not possible to know, a priori, what those common pieces will be.
I think this is where we fundamentally disagree. I think common pieces and their interfaces can be recognized based on the hard IP blocks and their interfaces plus some creative thinking based on development experience.
That working really well requires supportive silicon vendors. AMD is in a good position to lead by example here. ;)
There is the further risk (this has happened) that a seemingly harmless change is discovered to break some board, 1.5 years later. This has happened. To me.
Regression testing and release engineering are for sure important for anyone delivering firmware updates to customers.
As has been pointed out, always in motion, the future is.
Interesting times!
//Peter
On Tue, Jun 20, 2023 at 4:41 PM Peter Stuge peter@stuge.se wrote:
ron minnich wrote:
And, yes, no question, this is an activity that likely occurs less than it should. Such is our industry.
Such is project policy. Maybe because it's the lowest common denominator in industry.
The project's policy is to remove code that becomes difficult to maintain, whether due to bitrot or technical barriers to refactoring.
It is not possible to know, a priori, what those common pieces will be.
I think this is where we fundamentally disagree. I think common pieces and their interfaces can be recognized based on the hard IP blocks and their interfaces plus some creative thinking based on development experience.
I'm pretty sure the Intel engineers who wrote the Archer City CRB code didn't have IBM's and ByteDance's server specs handy or know how they would use the code.
OTOH Intel also has this thing called EDK2 where they implement all kinds of interfaces for each IP block, however it requires a hugely bloated framework and never quite works the way you want, requiring ungodly hacks to the sources and build system to override the default behaviors. I prefer to keep things simple and just refactor a few lines of coreboot code when needed.
On 21.06.23 04:58, David Hendricks wrote:
On Tue, Jun 20, 2023 at 4:41 PM Peter Stuge peter@stuge.se wrote:
ron minnich wrote:
And, yes, no question, this is an activity that likely occurs less than it should. Such is our industry.
Such is project policy. Maybe because it's the lowest common denominator in industry.
The project's policy is to remove code that becomes difficult to maintain, whether due to bitrot or technical barriers to refactoring.
I don't see this "becomes difficult to maintain" much. Much of what we removed in the last years had either an unfortunate design (cf. FSP 1.0) and or was already merged in a difficult to maintain state (cf. AGESA). IMO, this is where we should consider to learn from, not just make it policy and go on.
It is not possible to know, a priori, what those common pieces will be.
I think this is where we fundamentally disagree. I think common pieces and their interfaces can be recognized based on the hard IP blocks and their interfaces plus some creative thinking based on development experience.
I'm pretty sure the Intel engineers who wrote the Archer City CRB code didn't have IBM's and ByteDance's server specs handy or know how they would use the code.
AFAICT, the less board-specific code in archercity_crb/ is platform specific. This may be an important point: FWIW, we focus more on review for the platform code, and are more lax about the board ports. Maybe we should try to keep an eye on it, so that platform code doesn't sneak into board ports.
A bit OT, and sorry, if this is an odd question: Wasn't this the point of OCP, that they can talk to each other? If it's possible, we could try to talk earlier about the code, before it's written and pushed upstream later.
Nico
On Wed, Jun 21, 2023 at 4:19 AM Nico Huber nico.h@gmx.de wrote:
On 21.06.23 04:58, David Hendricks wrote:
On Tue, Jun 20, 2023 at 4:41 PM Peter Stuge peter@stuge.se wrote:
ron minnich wrote:
And, yes, no question, this is an activity that likely occurs less than it should. Such is our industry.
Such is project policy. Maybe because it's the lowest common denominator in industry.
The project's policy is to remove code that becomes difficult to maintain, whether due to bitrot or technical barriers to refactoring.
I don't see this "becomes difficult to maintain" much. Much of what we removed in the last years had either an unfortunate design (cf. FSP 1.0) and or was already merged in a difficult to maintain state (cf. AGESA). IMO, this is where we should consider to learn from, not just make it policy and go on.
I'd also add that we shouldn't stonewall new contributors because of problems outside of their scope, going along with what Martin wrote earlier.
A bit OT, and sorry, if this is an odd question: Wasn't this the point of OCP, that they can talk to each other? If it's possible, we could try to talk earlier about the code, before it's written and pushed upstream later.
It's more about setting some general parameters to ensure the code can be downloaded, modified, and redistributed. That alone is a huge hill to climb in our industry. Perfection will be the next battle once we've removed hurdles to initial development.
That said, there are some collaborative efforts with silicon vendors using their reference platforms, for example Intel's Archer City and AMD's Onyx, however OEM products are not usually developed this way. To be clear, participation in open source projects does *NOT* require companies to share upcoming product details with potential competitors. I've heard FUD from certain parties claiming otherwise.
On 21.06.23 01:23, ron minnich wrote:
The approach in the last 24 years (of this unsustainable project :-) has been to get several mainboards of a type, and, once we have them, try to work out what code is truly common and what code is similar but not truly common.
AFAICT, for the last 5~8 years, this has not been the case.
Code that is truly common can then be factored out into places such as src/lib. Code then migrates out of specific mainboards and into common spaces. This progression has happened many times.
And, yes, no question, this is an activity that likely occurs less than it should. Such is our industry.
Saying our industry is like that is why our industry is like that. Many people don't try to optimize the process just because that's how the process has been, even when there are low-hanging fruits that could help. Felix H. has shown good examples [1] how to optimize the process for a silicon vendor. I don't see why that wouldn't be possible else- where. Quite interesting to see, how the diligence of one individual makes things possible that "our industry" can only dream of.
It is not possible to know, a priori, what those common pieces will be. So we are left with this admittedly non-ideal situation.
IMO, it is possible, when people talk about it.
There is the further risk (this has happened) that a seemingly harmless change is discovered to break some board, 1.5 years later. This has happened. To me.
From my point of view, this is an argument to not refactor later, but as early as possible. Ideally before board ports are merged, because then the known good state is the known maintainable state.
Nico
[1] https://www.osfc.io/2022/talks/exploring-approaches-to-add-firmware-support-...
On 21.06.23 01:07, Peter Stuge wrote:
(sorry if you receive this twice)
David Hendricks wrote:
it will be easier to refactor portions of the code with the large patches merged in a buildable and (hopefully) usable/testable state.
That's pretty weak sauce and I think you all know deep down.
Let's not lose the scope here, please. AFAICT, we are still talking about particular mainboard ports that share, beside boilerplate, maybe 50~70 lines of code. From my point of view, we have much much bigger problems elsewhere.
Who pays for refactoring? Probably someone else.
That's unsustainable for the project.
If we'd make what is said about this case a general rule, maybe yes. But I don't see that David would have implied this.
Nico
-
Angel Pons -
Chen.AnnieET 陳恩婷 TAO -
David Hendricks -
Lean Sheng Tan -
Martin Roth -
Nico Huber -
Paul Menzel -
Peter Stuge -
ron minnich