Denverton-NS refactoring
A bit of background: I work for Silicom USA and we have several platforms designed around Denverton-NS. Prior to coming to Silicom I was a BIOS architect for Intel, being employed there for 19 years. I was one of the original BIOS engineers for Denverton back when it was first released and ADI Engineering (which became Silicom USA) was a design partner. We still have several customers that use Coreboot on our platforms. When I came on board a year ago I noted that the version we were maintaining was fairly old. Since that time the Intel SoC support has evolved and uses a lot of common code (yay!) but Denverton doesn 't seem to have kept up. Not sure if the maintainers from Intel are still active or not, but I'm sending them an email to see. Over the past few weeks I've brought Denverton support up to date with the Intel SoC common codebase and will be submitting wholesale changes to the tree to do this. Unfortunately, since the code sort of languished for so long, it's not a task that's easily broken up into smaller bits since much of the bits are intertwined across several functional blocks (GPIOs, PMC, ACPI, CPU, etc). While the current in-tree version of Denverton support uses some SOC_INTEL_COMMON_BLOCK_XXXX bits, I've updated the support to use much more and wondering what might be the best way to submit the refactor for easier review. List of supported common bits for Denverton: SOC_INTEL_COMMON_BLOCK SOC_INTEL_COMMON_BLOCK_CPU SOC_INTEL_COMMON_BLOCK_CPU_MPINIT SOC_INTEL_COMMON_BLOCK_SA SOC_INTEL_COMMON_BLOCK_ACPI SOC_INTEL_COMMON_PMC SOC_INTEL_COMMON_BLOKC_PMC_DISCOVERABLE SOC_INTEL_COMMON_BLOCK_ITSS SOC_INTEL_COMMON_BLOCK_TIMER SOC_INTEL_COMMON_BLOCK_P2SB SOC_INTEL_COMMON_BLOCK_GPIO SOC_INTEL_COMMON_BLOCK_ACPI_GPIO SOC_INTEL_COMMON_BLOCK_SPI SOC_INTEL_COMMON_BLOCK_FAST_SPI SOC_INTEL_COMMON_BLOCK_PCR SOC_INTEL_COMMON_BLOCK_LPC SOC_INTEL_COMMON_BLOCK_SMBUS SOC_INTEL_COMMON_BLOCK_TCO SOC_INTEL_COMMON_BLOCK_TCO_ENABLE_THROUGH_SMBUS SOC_INTEL_COMMON_BLOCK_PCIE SOC_INTEL_COMMON_BLOCK_RTC SOC_INTEL_COMMON_BLOCK_SMM SOC_INTEL_COMMON_BLOCK_SMM_IO_TRAP SOC_INTEL_OMMON_BLOCK_POWER_LIMIT
Hello Jeff, that sounds really great. Thanks!
Not sure if the maintainers from Intel are still active or not, but I’m sending them an email to see.
They aren't. The Intel "maintainers" told us they will work on Denverton, while we were about to drop the Denverton support like a year ago, but nothing happened since then. So your patches are highly appreciated :)
wondering what might be the best way to submit the refactor for easier review.
Hard to tell when we don't know what exactly has changed. I would say just push whatever you have to Gerrit and then we will see. They can be still rebased and reworked later :) I suggest adding a topic to your patches (like "denverton_refactoring"), if some aren't in the same relation chain. So they are easier to find through the search. // Felix
Another thing I'd like to say (and it's probably pretty obvious once the majority of the commits start coming through) is that a lot of work has been done for this. In order to make the commit chunks more logical in their grouping will require that the build will end up breaking because changes in one area will break another. I started by submitting the changes to the ifdtool because that's independent of the rest, followed by the changes to the southbridge/firmware area because again it won't break anything. Next, is the pci_ids.h because it's just letting everyone know that I'm changing the names globally (which of course will break the build). The majority of the change is soc/intel/denverton_ns so that commit will be a logical group, without worrying about platforms (again will break the build because the mainboards depend on it), and lastly I'll submit the mainboards/intel/harcuvar in order to have an example of a platform (and not break the build if all the other commits before it are part of the build.) since it look that the buildbot will try and build for each individual commit vs applying an entire set of commits and building, you'll either have to live with the breakage until everything is reviewed before applying everything all at once and fixing it again or someone can point me to an example of how doing a major overhaul like this would be usable. Normally I'd make a branch and do it that way, but it seems that's not the way to do it in this environment? Or did I miss something and I can actually create and push a branch of my own into this repo? -----Original Message----- From: Felix Singer <felixsinger@posteo.net> Sent: Wednesday, January 5, 2022 9:31 PM To: coreboot@coreboot.org Subject: [coreboot] Re: Denverton-NS refactoring Caution: This is an external email. Please take care when clicking links or opening attachments. Hello Jeff, that sounds really great. Thanks!
Not sure if the maintainers from Intel are still active or not, but I’m sending them an email to see.
They aren't. The Intel "maintainers" told us they will work on Denverton, while we were about to drop the Denverton support like a year ago, but nothing happened since then. So your patches are highly appreciated :)
wondering what might be the best way to submit the refactor for easier review.
Hard to tell when we don't know what exactly has changed. I would say just push whatever you have to Gerrit and then we will see. They can be still rebased and reworked later :) I suggest adding a topic to your patches (like "denverton_refactoring"), if some aren't in the same relation chain. So they are easier to find through the search. // Felix _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Hi Jeff, On Fri, Jan 7, 2022 at 5:13 AM Jeff Daly <jeffd@silicom-usa.com> wrote:
Another thing I'd like to say (and it's probably pretty obvious once the majority of the commits start coming through) is that a lot of work has been done for this. In order to make the commit chunks more logical in their grouping will require that the build will end up breaking because changes in one area will break another. I started by submitting the changes to the ifdtool because that's independent of the rest, followed by the changes to the southbridge/firmware area because again it won't break anything. Next, is the pci_ids.h because it's just letting everyone know that I'm changing the names globally (which of course will break the build). The majority of the change is soc/intel/denverton_ns so that commit will be a logical group, without worrying about platforms (again will break the build because the mainboards depend on it), and lastly I'll submit the mainboards/intel/harcuvar in order to have an example of a platform (and not break the build if all the other commits before it are part of the build.) since it look that the buildbot will try and build for each individual commit vs applying an entire set of commits and building, you'll either have to live with the breakage until everything is reviewed before applying everything all at once and fixing it again or someone can point me to an example of how doing a major overhaul like this would be usable. Normally I'd make a branch and do it that way, but it seems that's not the way to do it in this environment? Or did I miss something and I can actually create and push a branch of my own into this repo?
I'm afraid you'll have to restructure your work. Are these commits publicly visible somewhere? If not, you can push them to review.coreboot.org (just for reference, create new changes for the redesigned ). It's hard for me to give concrete suggestions without seeing the code, but I'll happily provide suggestions on how to restructure your work into upstreamable changes. We don't do any merges with Gerrit, so every commit needs to build successfully to get submitted. This is very useful when doing a git bisect to troubleshoot a regression: if the faulty commit got submitted amidst the commits of a patch train that only builds successfully at the end, one would need to fix the build errors while bisecting, with the risk of introducing new bugs which would make it harder to identify where the original problem is. When a commit doesn't break building but results in runtime problems (e.g. boot failure), we call this a regression and we address regressions by either fixing the issue if it's easy to find (e.g. https://review.coreboot.org/55289 used the wrong variable and caused boot issues, https://review.coreboot.org/58558 fixed it) or by reverting the problematic commit. Also, note that intel/harcuvar isn't the only Denverton-NS in the tree: scaleway/tagada would also need to be updated so that it builds successfully. In our workflow, changes to non-mainboard code that impact mainboard code also have to adapt mainboards accordingly. Yes, this is quite annoying when many boards are affected, but having to fix broken boards after non-mainboard code changes didn't update all affected boards accordingly is much worse by several orders of magnitude. Typically, the changes to mainboard code are only annoying because of their repetitive nature, but this shouldn't be an issue for Denverton-NS as there's only two boards in the tree. https://review.coreboot.org/49088 and https://review.coreboot.org/49089 are two examples of non-mainboard-code changes that affect mainboard code. Yes, I could've done this using only one commit, but I think it's easier to review the changes to mainboard code this way: the first commit just removes `cX_battery` settings under the premise that they have the same value as the corresponding `cX_acpower` settings, and the second commit just renames the `cX_acpower` settings to `acpi_cX`. In most cases, it's best to make one commit per logical change, where intermediate states still work even if they're incomplete. When a commit message has a "list of changes" or similar, it's a sign that it can likely be split into several commits. If unsure, it's better to make too many commits and squash some of them later than to make too few commits and end up having to split them up. Intermediate states of my patch trains still work (unless I messed up), but can have ugly things that get fixed later. For example, https://review.coreboot.org/60937 doesn't unindent the body of the original if-block because it gets cleaned up later in https://review.coreboot.org/60938 which is reproducible. A commit is reproducible if the resulting coreboot.rom when building with BUILD_TIMELESS=1 (which avoids including some info like commit hash into the image) is identical before and after the commit, and verifying this is a great way to ensure that some commits are correct. I've (ab)used reproducible commits to make some changes to RAM init code much easier to review. RAM init code has lots of magic values, and changing just one number could have unpredictable consequences that regular boot testing can't detect. https://review.coreboot.org/q/hashtag:iosav-api is one example of such a thing, where I temporarily introduced an awfully offensive C macro with about two dozen parameters to wrap the code around the magic values so that changing how those magic values are used doesn't touch the magic values themselves. Even with the cursed macro in there, the code still built and booted successfully.
-----Original Message----- From: Felix Singer <felixsinger@posteo.net> Sent: Wednesday, January 5, 2022 9:31 PM To: coreboot@coreboot.org Subject: [coreboot] Re: Denverton-NS refactoring
Caution: This is an external email. Please take care when clicking links or opening attachments.
Hello Jeff,
that sounds really great. Thanks!
Not sure if the maintainers from Intel are still active or not, but I’m sending them an email to see.
They aren't. The Intel "maintainers" told us they will work on Denverton, while we were about to drop the Denverton support like a year ago, but nothing happened since then.
So your patches are highly appreciated :)
wondering what might be the best way to submit the refactor for easier review.
Hard to tell when we don't know what exactly has changed. I would say just push whatever you have to Gerrit and then we will see. They can be still rebased and reworked later :)
I suggest adding a topic to your patches (like "denverton_refactoring"), if some aren't in the same relation chain. So they are easier to find through the search.
// Felix
_______________________________________________ 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
Best regards, Angel
Hi Angel, thanks for the links I will definitely check out those examples. In the meantime, I've discovered the concept of gerrit 'topics' so I'm using that currently to start split out the commits into logical related changes for easier review but still be able to build 😊. Right now, the majority of the refactor is happening in the topic 'Denverton Refactor', and is marked as WIP. Still learning gerrit, so if there's a way for you to find that, you'll see the basics of the work I'm doing. I think the topic method is really the only way I can see to break up things cleanly and still have it be able to build to get the +1. Right now I'm pushing them as the soc/intel/denverton_ns chunk, then the mainboard/intel/harcuvar chunk, then the mainboard/silicom/denverton_ns (the way I've structured our multiple boards, I may actually separate them out into individual boards, right now they are baseboard/variant mechanism since they are very similar but I might change my mind). If you have a suggestion that works better than topics I'm all ears, but for now I'm moving forward this way. If you get to look at the code (I'll see if I can just send an invite or something), be assured that I'll be breaking the commit up into digestible chunks so as you said, finding the ones that cause a problem will make it easier. Also, yes I acknowledge that I need to take care of the Tagada board port to the new refactor, I'll probably be doing that this weekend. I did contact them but they response was that they've changed directions so the Tagada is unmaintained at this time (and sounds like it won't become a product). Additionally, I've been in contact with my former coworkers at Intel. They are having a meeting on Tuesday to discuss the support moving forward, but in all likelihood they will pass on the maintenance to me (they've already asked me if I'd be willing in fact). -----Original Message----- From: Angel Pons <th3fanbus@gmail.com> Sent: Saturday, January 8, 2022 8:43 AM To: Jeff Daly <jeffd@silicom-usa.com> Cc: Felix Singer <felixsinger@posteo.net>; coreboot@coreboot.org Subject: Re: [coreboot] Re: Denverton-NS refactoring Caution: This is an external email. Please take care when clicking links or opening attachments. Hi Jeff, On Fri, Jan 7, 2022 at 5:13 AM Jeff Daly <jeffd@silicom-usa.com> wrote:
Another thing I'd like to say (and it's probably pretty obvious once the majority of the commits start coming through) is that a lot of work has been done for this. In order to make the commit chunks more logical in their grouping will require that the build will end up breaking because changes in one area will break another. I started by submitting the changes to the ifdtool because that's independent of the rest, followed by the changes to the southbridge/firmware area because again it won't break anything. Next, is the pci_ids.h because it's just letting everyone know that I'm changing the names globally (which of course will break the build). The majority of the change is soc/intel/denverton_ns so that commit will be a logical group, without worrying about platforms (again will break the build because the mainboards depend on it), and lastly I'll submit the mainboards/intel/harcuvar in order to have an example of a platform (and not break the build if all the other commits before it are part of the build.) since it look that the buildbot will try and build for each individual commit vs applying an entire set of commits and building, you'll either have to live with the breakage until everything is reviewed before applying everything all at once and fixing it again or someone can point me to an example of how doing a major overhaul like this would be usable. Normally I'd make a branch and do it that way, but it seems that's not the way to do it in this environment? Or did I miss something and I can actually create and push a branch of my own into this repo?
I'm afraid you'll have to restructure your work. Are these commits publicly visible somewhere? If not, you can push them to review.coreboot.org (just for reference, create new changes for the redesigned ). It's hard for me to give concrete suggestions without seeing the code, but I'll happily provide suggestions on how to restructure your work into upstreamable changes. We don't do any merges with Gerrit, so every commit needs to build successfully to get submitted. This is very useful when doing a git bisect to troubleshoot a regression: if the faulty commit got submitted amidst the commits of a patch train that only builds successfully at the end, one would need to fix the build errors while bisecting, with the risk of introducing new bugs which would make it harder to identify where the original problem is. When a commit doesn't break building but results in runtime problems (e.g. boot failure), we call this a regression and we address regressions by either fixing the issue if it's easy to find (e.g. https://review.coreboot.org/55289 used the wrong variable and caused boot issues, https://review.coreboot.org/58558 fixed it) or by reverting the problematic commit. Also, note that intel/harcuvar isn't the only Denverton-NS in the tree: scaleway/tagada would also need to be updated so that it builds successfully. In our workflow, changes to non-mainboard code that impact mainboard code also have to adapt mainboards accordingly. Yes, this is quite annoying when many boards are affected, but having to fix broken boards after non-mainboard code changes didn't update all affected boards accordingly is much worse by several orders of magnitude. Typically, the changes to mainboard code are only annoying because of their repetitive nature, but this shouldn't be an issue for Denverton-NS as there's only two boards in the tree. https://review.coreboot.org/49088 and https://review.coreboot.org/49089 are two examples of non-mainboard-code changes that affect mainboard code. Yes, I could've done this using only one commit, but I think it's easier to review the changes to mainboard code this way: the first commit just removes `cX_battery` settings under the premise that they have the same value as the corresponding `cX_acpower` settings, and the second commit just renames the `cX_acpower` settings to `acpi_cX`. In most cases, it's best to make one commit per logical change, where intermediate states still work even if they're incomplete. When a commit message has a "list of changes" or similar, it's a sign that it can likely be split into several commits. If unsure, it's better to make too many commits and squash some of them later than to make too few commits and end up having to split them up. Intermediate states of my patch trains still work (unless I messed up), but can have ugly things that get fixed later. For example, https://review.coreboot.org/60937 doesn't unindent the body of the original if-block because it gets cleaned up later in https://review.coreboot.org/60938 which is reproducible. A commit is reproducible if the resulting coreboot.rom when building with BUILD_TIMELESS=1 (which avoids including some info like commit hash into the image) is identical before and after the commit, and verifying this is a great way to ensure that some commits are correct. I've (ab)used reproducible commits to make some changes to RAM init code much easier to review. RAM init code has lots of magic values, and changing just one number could have unpredictable consequences that regular boot testing can't detect. https://review.coreboot.org/q/hashtag:iosav-api is one example of such a thing, where I temporarily introduced an awfully offensive C macro with about two dozen parameters to wrap the code around the magic values so that changing how those magic values are used doesn't touch the magic values themselves. Even with the cursed macro in there, the code still built and booted successfully.
-----Original Message----- From: Felix Singer <felixsinger@posteo.net> Sent: Wednesday, January 5, 2022 9:31 PM To: coreboot@coreboot.org Subject: [coreboot] Re: Denverton-NS refactoring
Caution: This is an external email. Please take care when clicking links or opening attachments.
Hello Jeff,
that sounds really great. Thanks!
Not sure if the maintainers from Intel are still active or not, but I’m sending them an email to see.
They aren't. The Intel "maintainers" told us they will work on Denverton, while we were about to drop the Denverton support like a year ago, but nothing happened since then.
So your patches are highly appreciated :)
wondering what might be the best way to submit the refactor for easier review.
Hard to tell when we don't know what exactly has changed. I would say just push whatever you have to Gerrit and then we will see. They can be still rebased and reworked later :)
I suggest adding a topic to your patches (like "denverton_refactoring"), if some aren't in the same relation chain. So they are easier to find through the search.
// Felix
_______________________________________________ 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
Best regards, Angel
Ok, so what I've figured out is that it seems gerrit doesn't like it when the code gets refactored. I have several commits that are showing merge conflicts but the files that have conflicts are because a lot of code changes between them as things get refactored. Entire blocks of code in some files are removed, some files are just completely replaced with newer files that have the correct content in them. I'd hate to have to do something dumb like rename the files that have too many changes for gerrit/git to figure out. CB:61015 is a perfect example. Soc/intel/denverton_ns/soc_util.c was changed to delete functions from colliding with others in the SoC common code (or were just plain unnecessary). The change deletes get_tseg_memory(), get_top_of_low_memory(), get_top_of_upper_memory(). No other changes other than deletions and I'm getting a merge conflict. Either I'm dumb or something else is dumb. (and I have been dumb before, so maybe it's me but this one seems kinda obvious unless there's some setting someplace that complains when there's more than a few lines difference). -----Original Message----- From: Angel Pons <th3fanbus@gmail.com> Sent: Saturday, January 8, 2022 8:43 AM To: Jeff Daly <jeffd@silicom-usa.com> Cc: Felix Singer <felixsinger@posteo.net>; coreboot@coreboot.org Subject: Re: [coreboot] Re: Denverton-NS refactoring Caution: This is an external email. Please take care when clicking links or opening attachments. Hi Jeff, On Fri, Jan 7, 2022 at 5:13 AM Jeff Daly <jeffd@silicom-usa.com> wrote:
Another thing I'd like to say (and it's probably pretty obvious once the majority of the commits start coming through) is that a lot of work has been done for this. In order to make the commit chunks more logical in their grouping will require that the build will end up breaking because changes in one area will break another. I started by submitting the changes to the ifdtool because that's independent of the rest, followed by the changes to the southbridge/firmware area because again it won't break anything. Next, is the pci_ids.h because it's just letting everyone know that I'm changing the names globally (which of course will break the build). The majority of the change is soc/intel/denverton_ns so that commit will be a logical group, without worrying about platforms (again will break the build because the mainboards depend on it), and lastly I'll submit the mainboards/intel/harcuvar in order to have an example of a platform (and not break the build if all the other commits before it are part of the build.) since it look that the buildbot will try and build for each individual commit vs applying an entire set of commits and building, you'll either have to live with the breakage until everything is reviewed before applying everything all at once and fixing it again or someone can point me to an example of how doing a major overhaul like this would be usable. Normally I'd make a branch and do it that way, but it seems that's not the way to do it in this environment? Or did I miss something and I can actually create and push a branch of my own into this repo?
I'm afraid you'll have to restructure your work. Are these commits publicly visible somewhere? If not, you can push them to review.coreboot.org (just for reference, create new changes for the redesigned ). It's hard for me to give concrete suggestions without seeing the code, but I'll happily provide suggestions on how to restructure your work into upstreamable changes. We don't do any merges with Gerrit, so every commit needs to build successfully to get submitted. This is very useful when doing a git bisect to troubleshoot a regression: if the faulty commit got submitted amidst the commits of a patch train that only builds successfully at the end, one would need to fix the build errors while bisecting, with the risk of introducing new bugs which would make it harder to identify where the original problem is. When a commit doesn't break building but results in runtime problems (e.g. boot failure), we call this a regression and we address regressions by either fixing the issue if it's easy to find (e.g. https://review.coreboot.org/55289 used the wrong variable and caused boot issues, https://review.coreboot.org/58558 fixed it) or by reverting the problematic commit. Also, note that intel/harcuvar isn't the only Denverton-NS in the tree: scaleway/tagada would also need to be updated so that it builds successfully. In our workflow, changes to non-mainboard code that impact mainboard code also have to adapt mainboards accordingly. Yes, this is quite annoying when many boards are affected, but having to fix broken boards after non-mainboard code changes didn't update all affected boards accordingly is much worse by several orders of magnitude. Typically, the changes to mainboard code are only annoying because of their repetitive nature, but this shouldn't be an issue for Denverton-NS as there's only two boards in the tree. https://review.coreboot.org/49088 and https://review.coreboot.org/49089 are two examples of non-mainboard-code changes that affect mainboard code. Yes, I could've done this using only one commit, but I think it's easier to review the changes to mainboard code this way: the first commit just removes `cX_battery` settings under the premise that they have the same value as the corresponding `cX_acpower` settings, and the second commit just renames the `cX_acpower` settings to `acpi_cX`. In most cases, it's best to make one commit per logical change, where intermediate states still work even if they're incomplete. When a commit message has a "list of changes" or similar, it's a sign that it can likely be split into several commits. If unsure, it's better to make too many commits and squash some of them later than to make too few commits and end up having to split them up. Intermediate states of my patch trains still work (unless I messed up), but can have ugly things that get fixed later. For example, https://review.coreboot.org/60937 doesn't unindent the body of the original if-block because it gets cleaned up later in https://review.coreboot.org/60938 which is reproducible. A commit is reproducible if the resulting coreboot.rom when building with BUILD_TIMELESS=1 (which avoids including some info like commit hash into the image) is identical before and after the commit, and verifying this is a great way to ensure that some commits are correct. I've (ab)used reproducible commits to make some changes to RAM init code much easier to review. RAM init code has lots of magic values, and changing just one number could have unpredictable consequences that regular boot testing can't detect. https://review.coreboot.org/q/hashtag:iosav-api is one example of such a thing, where I temporarily introduced an awfully offensive C macro with about two dozen parameters to wrap the code around the magic values so that changing how those magic values are used doesn't touch the magic values themselves. Even with the cursed macro in there, the code still built and booted successfully.
-----Original Message----- From: Felix Singer <felixsinger@posteo.net> Sent: Wednesday, January 5, 2022 9:31 PM To: coreboot@coreboot.org Subject: [coreboot] Re: Denverton-NS refactoring
Caution: This is an external email. Please take care when clicking links or opening attachments.
Hello Jeff,
that sounds really great. Thanks!
Not sure if the maintainers from Intel are still active or not, but I’m sending them an email to see.
They aren't. The Intel "maintainers" told us they will work on Denverton, while we were about to drop the Denverton support like a year ago, but nothing happened since then.
So your patches are highly appreciated :)
wondering what might be the best way to submit the refactor for easier review.
Hard to tell when we don't know what exactly has changed. I would say just push whatever you have to Gerrit and then we will see. They can be still rebased and reworked later :)
I suggest adding a topic to your patches (like "denverton_refactoring"), if some aren't in the same relation chain. So they are easier to find through the search.
// Felix
_______________________________________________ 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
Best regards, Angel
Dear Jeff, Am 12.01.22 um 15:49 schrieb Jeff Daly:
Ok, so what I've figured out is that it seems gerrit doesn't like it when the code gets refactored. I have several commits that are showing merge conflicts but the files that have conflicts are because a lot of code changes between them as things get refactored. Entire blocks of code in some files are removed, some files are just completely replaced with newer files that have the correct content in them. I'd hate to have to do something dumb like rename the files that have too many changes for gerrit/git to figure out. CB:61015 is a perfect example. Soc/intel/denverton_ns/soc_util.c was changed to delete functions from colliding with others in the SoC common code (or were just plain unnecessary). The change deletes get_tseg_memory(), get_top_of_low_memory(), get_top_of_upper_memory(). No other changes other than deletions and I'm getting a merge conflict. Either I'm dumb or something else is dumb. (and I have been dumb before, so maybe it's me but this one seems kinda obvious unless there's some setting someplace that complains when there's more than a few lines difference).
It is my understanding, that Gerrit displays *Merge Conflict* on a change-set, if the change-set itself cannot be directly be cherry-picked on top of the master branch. So, everything is fine with your change-set, and to be applied to the master branch, the parent changes (from your branch) need to be applied first. Kind regards, Paul
Ok that sort of makes sense, but for example in the case of the XHCI patch, the merge conflict appears to be that since soc/intel/denverton_ns/xhci.c was changed completely to reflect the usage of the common code and shouldn't cause an issue with being cherry-picked onto master....... So, I just tried it and see that it comes up with the merge conflict. I guess the merge operation is super-cautious and prefers the developer to do a manual merge vs always accepting the newer code? Is there a setting that can be modified for that? Having to do a merge resolution for wholesale changes to code seems counter-productive. Next question would be, what's the best mechanism to do this and not mess up my patch train again. I want to keep all the relationships in gerrit so the progression can be visualized. I have a clean master branch and the denverton_refactor branch locally where I'm keeping my changes. I'd like to keep it in denverton_refactor locally..... is it required that I need to apply the merge resolution commit on the master branch or can I rewind my branch to the offending commit^ and do git pull --rebase at that point? -----Original Message----- From: Paul Menzel <pmenzel@molgen.mpg.de> Sent: Wednesday, January 12, 2022 10:06 AM To: coreboot@coreboot.org Subject: [coreboot] Gerrit shows a Merge Conflict (was: Re: Denverton-NS refactoring) Caution: This is an external email. Please take care when clicking links or opening attachments. Dear Jeff, Am 12.01.22 um 15:49 schrieb Jeff Daly:
Ok, so what I've figured out is that it seems gerrit doesn't like it when the code gets refactored. I have several commits that are showing merge conflicts but the files that have conflicts are because a lot of code changes between them as things get refactored. Entire blocks of code in some files are removed, some files are just completely replaced with newer files that have the correct content in them. I'd hate to have to do something dumb like rename the files that have too many changes for gerrit/git to figure out. CB:61015 is a perfect example. Soc/intel/denverton_ns/soc_util.c was changed to delete functions from colliding with others in the SoC common code (or were just plain unnecessary). The change deletes get_tseg_memory(), get_top_of_low_memory(), get_top_of_upper_memory(). No other changes other than deletions and I'm getting a merge conflict. Either I'm dumb or something else is dumb. (and I have been dumb before, so maybe it's me but this one seems kinda obvious unless there's some setting someplace that complains when there's more than a few lines difference).
It is my understanding, that Gerrit displays *Merge Conflict* on a change-set, if the change-set itself cannot be directly be cherry-picked on top of the master branch. So, everything is fine with your change-set, and to be applied to the master branch, the parent changes (from your branch) need to be applied first. Kind regards, Paul _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
On 12.01.22 16:21, Jeff Daly wrote:
Ok that sort of makes sense, but for example in the case of the XHCI patch, the merge conflict appears to be that since soc/intel/denverton_ns/xhci.c was changed completely to reflect the usage of the common code and shouldn't cause an issue with being cherry-picked onto master.......
So, I just tried it and see that it comes up with the merge conflict. I guess the merge operation is super-cautious and prefers the developer to do a manual merge vs always accepting the newer code? Is there a setting that can be modified for that?
I don't know any setting to change that. It's simple tools, AFAICT. If you supply them a patch with something to remove they'll try to find the respective line to remove and fail if that line changed between the two bases. Git cannot see that the intention is to replace the entire file no matter its current contents. To a human that might be obvious but also only in the context of your whole patch train. IMO that's correct behavior. For instance, if somebody else added something to that file in the meantime, would the intention still be to replace the whole file? How to decide that automatically?
Having to do a merge resolution for wholesale changes to code seems counter-productive.
You don't have to do anything unless you want to re-organize your patch train. For instance, change the order of commits you currently have on one branch. But is that really what you want?
Next question would be, what's the best mechanism to do this and not mess up my patch train again. I want to keep all the relationships in gerrit so the progression can be visualized. I have a clean master branch and the denverton_refactor branch locally where I'm keeping my changes. I'd like to keep it in denverton_refactor locally..... is it required that I need to apply the merge resolution commit on the master branch or can I rewind my branch to the offending commit^ and do git pull --rebase at that point?
I'm afraid I don't follow. What is it that you want to do that results in the conflict in the first place? Generally our Gerrit setup doesn't allow merge commits. If you want to amend a commit on your branch, you have to rebase. But a `git pull --rebase` would rebase on top of upstream (I assume). That's sometimes useful, but rather a potential cause of conflicts than a resolution. Nico
If I don't have to do anything, then I won't. I wasn't sure whether commits that are marked with conflicts tend to get less attention than ones that don't. I've been rebasing for amending earlier commits so I guess at least I'm doing that correctly. What I'm confused with is as you said, if I'm removing lines it will try to remove them, and fail if something changed between the 2 bases. I'm the only one making any changes, so that shouldn't be an issue. I understand the file replacement explanation as well, but again nothing changed in the file in the base I'm working on and by rebasing against master before pushing, I'd see the merge conflict. If my change got in before someone elses, then they'd see the merge conflict when they went to push. -----Original Message----- From: Nico Huber <nico.h@gmx.de> Sent: Wednesday, January 12, 2022 12:04 PM To: Jeff Daly <jeffd@silicom-usa.com>; Paul Menzel <pmenzel@molgen.mpg.de>; coreboot@coreboot.org Subject: Re: [coreboot] Re: Gerrit shows a Merge Conflict (was: Re: Denverton-NS refactoring) Caution: This is an external email. Please take care when clicking links or opening attachments. On 12.01.22 16:21, Jeff Daly wrote:
Ok that sort of makes sense, but for example in the case of the XHCI patch, the merge conflict appears to be that since soc/intel/denverton_ns/xhci.c was changed completely to reflect the usage of the common code and shouldn't cause an issue with being cherry-picked onto master.......
So, I just tried it and see that it comes up with the merge conflict. I guess the merge operation is super-cautious and prefers the developer to do a manual merge vs always accepting the newer code? Is there a setting that can be modified for that?
I don't know any setting to change that. It's simple tools, AFAICT. If you supply them a patch with something to remove they'll try to find the respective line to remove and fail if that line changed between the two bases. Git cannot see that the intention is to replace the entire file no matter its current contents. To a human that might be obvious but also only in the context of your whole patch train. IMO that's correct behavior. For instance, if somebody else added something to that file in the meantime, would the intention still be to replace the whole file? How to decide that automatically?
Having to do a merge resolution for wholesale changes to code seems counter-productive.
You don't have to do anything unless you want to re-organize your patch train. For instance, change the order of commits you currently have on one branch. But is that really what you want?
Next question would be, what's the best mechanism to do this and not mess up my patch train again. I want to keep all the relationships in gerrit so the progression can be visualized. I have a clean master branch and the denverton_refactor branch locally where I'm keeping my changes. I'd like to keep it in denverton_refactor locally..... is it required that I need to apply the merge resolution commit on the master branch or can I rewind my branch to the offending commit^ and do git pull --rebase at that point?
I'm afraid I don't follow. What is it that you want to do that results in the conflict in the first place? Generally our Gerrit setup doesn't allow merge commits. If you want to amend a commit on your branch, you have to rebase. But a `git pull --rebase` would rebase on top of upstream (I assume). That's sometimes useful, but rather a potential cause of conflicts than a resolution. Nico
On 12.01.22 18:11, Jeff Daly wrote:
If I don't have to do anything, then I won't. I wasn't sure whether commits that are marked with conflicts tend to get less attention than ones that don't. I've been rebasing for amending earlier commits so I guess at least I'm doing that correctly. What I'm confused with is as you said, if I'm removing lines it will try to remove them, and fail if something changed between the 2 bases. I'm the only one making any changes, so that shouldn't be an issue. I understand the file replacement explanation as well, but again nothing changed in the file in the base I'm working on and by rebasing against master before pushing, I'd see the merge conflict. If my change got in before someone elses, then they'd see the merge conflict when they went to push.
The xHCI patch on Gerrit is currently based on one changing the PCI ID macro in `xhci.c`. When you cherry-pick the xHCI commit alone (on master where the macro name didn't change), the file contents are different. Nico
-----Original Message----- From: Nico Huber <nico.h@gmx.de> Sent: Wednesday, January 12, 2022 12:04 PM To: Jeff Daly <jeffd@silicom-usa.com>; Paul Menzel <pmenzel@molgen.mpg.de>; coreboot@coreboot.org Subject: Re: [coreboot] Re: Gerrit shows a Merge Conflict (was: Re: Denverton-NS refactoring)
Caution: This is an external email. Please take care when clicking links or opening attachments.
On 12.01.22 16:21, Jeff Daly wrote:
Ok that sort of makes sense, but for example in the case of the XHCI patch, the merge conflict appears to be that since soc/intel/denverton_ns/xhci.c was changed completely to reflect the usage of the common code and shouldn't cause an issue with being cherry-picked onto master.......
So, I just tried it and see that it comes up with the merge conflict. I guess the merge operation is super-cautious and prefers the developer to do a manual merge vs always accepting the newer code? Is there a setting that can be modified for that?
I don't know any setting to change that. It's simple tools, AFAICT. If you supply them a patch with something to remove they'll try to find the respective line to remove and fail if that line changed between the two bases. Git cannot see that the intention is to replace the entire file no matter its current contents. To a human that might be obvious but also only in the context of your whole patch train. IMO that's correct behavior. For instance, if somebody else added something to that file in the meantime, would the intention still be to replace the whole file? How to decide that automatically?
Having to do a merge resolution for wholesale changes to code seems counter-productive.
You don't have to do anything unless you want to re-organize your patch train. For instance, change the order of commits you currently have on one branch. But is that really what you want?
Next question would be, what's the best mechanism to do this and not mess up my patch train again. I want to keep all the relationships in gerrit so the progression can be visualized. I have a clean master branch and the denverton_refactor branch locally where I'm keeping my changes. I'd like to keep it in denverton_refactor locally..... is it required that I need to apply the merge resolution commit on the master branch or can I rewind my branch to the offending commit^ and do git pull --rebase at that point?
I'm afraid I don't follow. What is it that you want to do that results in the conflict in the first place?
Generally our Gerrit setup doesn't allow merge commits. If you want to amend a commit on your branch, you have to rebase.
But a `git pull --rebase` would rebase on top of upstream (I assume). That's sometimes useful, but rather a potential cause of conflicts than a resolution.
Nico
Ah right. Too many changes to keep track of in my head. I guess that's what good tools are for. In any case, as long as the commits are in small enough bits for reviewers to check in the context of the refactoring I'm doing then it shouldn't matter. I should done soon, pushing the remaining changes to the mainboards and adding my own platforms should be the end of it. -----Original Message----- From: Nico Huber <nico.h@gmx.de> Sent: Wednesday, January 12, 2022 12:28 PM To: Jeff Daly <jeffd@silicom-usa.com>; Paul Menzel <pmenzel@molgen.mpg.de>; coreboot@coreboot.org Subject: Re: [coreboot] Re: Gerrit shows a Merge Conflict (was: Re: Denverton-NS refactoring) Caution: This is an external email. Please take care when clicking links or opening attachments. On 12.01.22 18:11, Jeff Daly wrote:
If I don't have to do anything, then I won't. I wasn't sure whether commits that are marked with conflicts tend to get less attention than ones that don't. I've been rebasing for amending earlier commits so I guess at least I'm doing that correctly. What I'm confused with is as you said, if I'm removing lines it will try to remove them, and fail if something changed between the 2 bases. I'm the only one making any changes, so that shouldn't be an issue. I understand the file replacement explanation as well, but again nothing changed in the file in the base I'm working on and by rebasing against master before pushing, I'd see the merge conflict. If my change got in before someone elses, then they'd see the merge conflict when they went to push.
The xHCI patch on Gerrit is currently based on one changing the PCI ID macro in `xhci.c`. When you cherry-pick the xHCI commit alone (on master where the macro name didn't change), the file contents are different. Nico
-----Original Message----- From: Nico Huber <nico.h@gmx.de> Sent: Wednesday, January 12, 2022 12:04 PM To: Jeff Daly <jeffd@silicom-usa.com>; Paul Menzel <pmenzel@molgen.mpg.de>; coreboot@coreboot.org Subject: Re: [coreboot] Re: Gerrit shows a Merge Conflict (was: Re: Denverton-NS refactoring)
Caution: This is an external email. Please take care when clicking links or opening attachments.
On 12.01.22 16:21, Jeff Daly wrote:
Ok that sort of makes sense, but for example in the case of the XHCI patch, the merge conflict appears to be that since soc/intel/denverton_ns/xhci.c was changed completely to reflect the usage of the common code and shouldn't cause an issue with being cherry-picked onto master.......
So, I just tried it and see that it comes up with the merge conflict. I guess the merge operation is super-cautious and prefers the developer to do a manual merge vs always accepting the newer code? Is there a setting that can be modified for that?
I don't know any setting to change that. It's simple tools, AFAICT. If you supply them a patch with something to remove they'll try to find the respective line to remove and fail if that line changed between the two bases. Git cannot see that the intention is to replace the entire file no matter its current contents. To a human that might be obvious but also only in the context of your whole patch train. IMO that's correct behavior. For instance, if somebody else added something to that file in the meantime, would the intention still be to replace the whole file? How to decide that automatically?
Having to do a merge resolution for wholesale changes to code seems counter-productive.
You don't have to do anything unless you want to re-organize your patch train. For instance, change the order of commits you currently have on one branch. But is that really what you want?
Next question would be, what's the best mechanism to do this and not mess up my patch train again. I want to keep all the relationships in gerrit so the progression can be visualized. I have a clean master branch and the denverton_refactor branch locally where I'm keeping my changes. I'd like to keep it in denverton_refactor locally..... is it required that I need to apply the merge resolution commit on the master branch or can I rewind my branch to the offending commit^ and do git pull --rebase at that point?
I'm afraid I don't follow. What is it that you want to do that results in the conflict in the first place?
Generally our Gerrit setup doesn't allow merge commits. If you want to amend a commit on your branch, you have to rebase.
But a `git pull --rebase` would rebase on top of upstream (I assume). That's sometimes useful, but rather a potential cause of conflicts than a resolution.
Nico
A quick note, the feedback I got from Intel is that they will be moving away from internal support for DNV coreboot. The current maintainers at Intel will be assigning myself as a main maintainer with Vanessa as Intel's PAE support. -----Original Message----- From: Jeff Daly Sent: Wednesday, January 12, 2022 1:04 PM To: Nico Huber <nico.h@gmx.de>; Paul Menzel <pmenzel@molgen.mpg.de>; coreboot@coreboot.org Subject: RE: [coreboot] Re: Gerrit shows a Merge Conflict (was: Re: Denverton-NS refactoring) Ah right. Too many changes to keep track of in my head. I guess that's what good tools are for. In any case, as long as the commits are in small enough bits for reviewers to check in the context of the refactoring I'm doing then it shouldn't matter. I should done soon, pushing the remaining changes to the mainboards and adding my own platforms should be the end of it. -----Original Message----- From: Nico Huber <nico.h@gmx.de> Sent: Wednesday, January 12, 2022 12:28 PM To: Jeff Daly <jeffd@silicom-usa.com>; Paul Menzel <pmenzel@molgen.mpg.de>; coreboot@coreboot.org Subject: Re: [coreboot] Re: Gerrit shows a Merge Conflict (was: Re: Denverton-NS refactoring) Caution: This is an external email. Please take care when clicking links or opening attachments. On 12.01.22 18:11, Jeff Daly wrote:
If I don't have to do anything, then I won't. I wasn't sure whether commits that are marked with conflicts tend to get less attention than ones that don't. I've been rebasing for amending earlier commits so I guess at least I'm doing that correctly. What I'm confused with is as you said, if I'm removing lines it will try to remove them, and fail if something changed between the 2 bases. I'm the only one making any changes, so that shouldn't be an issue. I understand the file replacement explanation as well, but again nothing changed in the file in the base I'm working on and by rebasing against master before pushing, I'd see the merge conflict. If my change got in before someone elses, then they'd see the merge conflict when they went to push.
The xHCI patch on Gerrit is currently based on one changing the PCI ID macro in `xhci.c`. When you cherry-pick the xHCI commit alone (on master where the macro name didn't change), the file contents are different. Nico
-----Original Message----- From: Nico Huber <nico.h@gmx.de> Sent: Wednesday, January 12, 2022 12:04 PM To: Jeff Daly <jeffd@silicom-usa.com>; Paul Menzel <pmenzel@molgen.mpg.de>; coreboot@coreboot.org Subject: Re: [coreboot] Re: Gerrit shows a Merge Conflict (was: Re: Denverton-NS refactoring)
Caution: This is an external email. Please take care when clicking links or opening attachments.
On 12.01.22 16:21, Jeff Daly wrote:
Ok that sort of makes sense, but for example in the case of the XHCI patch, the merge conflict appears to be that since soc/intel/denverton_ns/xhci.c was changed completely to reflect the usage of the common code and shouldn't cause an issue with being cherry-picked onto master.......
So, I just tried it and see that it comes up with the merge conflict. I guess the merge operation is super-cautious and prefers the developer to do a manual merge vs always accepting the newer code? Is there a setting that can be modified for that?
I don't know any setting to change that. It's simple tools, AFAICT. If you supply them a patch with something to remove they'll try to find the respective line to remove and fail if that line changed between the two bases. Git cannot see that the intention is to replace the entire file no matter its current contents. To a human that might be obvious but also only in the context of your whole patch train. IMO that's correct behavior. For instance, if somebody else added something to that file in the meantime, would the intention still be to replace the whole file? How to decide that automatically?
Having to do a merge resolution for wholesale changes to code seems counter-productive.
You don't have to do anything unless you want to re-organize your patch train. For instance, change the order of commits you currently have on one branch. But is that really what you want?
Next question would be, what's the best mechanism to do this and not mess up my patch train again. I want to keep all the relationships in gerrit so the progression can be visualized. I have a clean master branch and the denverton_refactor branch locally where I'm keeping my changes. I'd like to keep it in denverton_refactor locally..... is it required that I need to apply the merge resolution commit on the master branch or can I rewind my branch to the offending commit^ and do git pull --rebase at that point?
I'm afraid I don't follow. What is it that you want to do that results in the conflict in the first place?
Generally our Gerrit setup doesn't allow merge commits. If you want to amend a commit on your branch, you have to rebase.
But a `git pull --rebase` would rebase on top of upstream (I assume). That's sometimes useful, but rather a potential cause of conflicts than a resolution.
Nico
Thank you for your updates, Jeff. Jeff Daly wrote:
the feedback I got from Intel is that they will be moving away from internal support for DNV coreboot.
It's sad and telling to see one of the most ambitious technology companies in the world, one that routinely pushed technology limits, choose to not continue to accept responsibility and be active in our firmware community. Value is never inherent, it's what we choose to create. Kind regards //Peter
Hi, To clarify situation (Jeff - looks like you got wrong my last email 😉 ) DNV will still be supported by Intel. Vanessa will continue to be primary support contact from Intel side. One thing that will change in next few months is that I and Suresh will move to another project and will have limited time to maintain DNV/HCV code. But we still be around 😉. So that’s main reason to propose Jeff as main maintainer for DNV/HCV. Mariusz -----Original Message----- From: Peter Stuge <peter@stuge.se> Sent: Tuesday, January 18, 2022 2:17 PM To: coreboot@coreboot.org Subject: [coreboot] Re: Gerrit shows a Merge Conflict (was: Re: Denverton-NS refactoring) Thank you for your updates, Jeff. Jeff Daly wrote:
the feedback I got from Intel is that they will be moving away from internal support for DNV coreboot.
It's sad and telling to see one of the most ambitious technology companies in the world, one that routinely pushed technology limits, choose to not continue to accept responsibility and be active in our firmware community. Value is never inherent, it's what we choose to create. Kind regards //Peter _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org -------------------------------------------------------------- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
... and a little note: This is only for DNV/HCV (as we are team dedicated to them). No change for other Intel hw. They are handled by other Intel teams. Mariusz -----Original Message----- From: Szafranski, MariuszX <mariuszx.szafranski@intel.com> Sent: Wednesday, January 19, 2022 10:49 AM To: Peter Stuge <peter@stuge.se>; coreboot@coreboot.org; Daly, Jeff <jeffd@silicom-usa.com> Subject: [coreboot] Re: Gerrit shows a Merge Conflict (was: Re: Denverton-NS refactoring) Hi, To clarify situation (Jeff - looks like you got wrong my last email 😉 ) DNV will still be supported by Intel. Vanessa will continue to be primary support contact from Intel side. One thing that will change in next few months is that I and Suresh will move to another project and will have limited time to maintain DNV/HCV code. But we still be around 😉. So that’s main reason to propose Jeff as main maintainer for DNV/HCV. Mariusz -----Original Message----- From: Peter Stuge <peter@stuge.se> Sent: Tuesday, January 18, 2022 2:17 PM To: coreboot@coreboot.org Subject: [coreboot] Re: Gerrit shows a Merge Conflict (was: Re: Denverton-NS refactoring) Thank you for your updates, Jeff. Jeff Daly wrote:
the feedback I got from Intel is that they will be moving away from internal support for DNV coreboot.
It's sad and telling to see one of the most ambitious technology companies in the world, one that routinely pushed technology limits, choose to not continue to accept responsibility and be active in our firmware community. Value is never inherent, it's what we choose to create. Kind regards //Peter _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org -------------------------------------------------------------- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org -------------------------------------------------------------- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
My *complete* apologies! Peter, I absolutely read Mariusz's email the wrong way and I'm sorry for any confusion or consternation towards Intel this may have caused! I would however say that they *are* accepting responsibility for activity in the firmware community by asking me to step into the role of maintainer for Denverton coreboot. I'm a former BIOS engineer at Intel, and did work closely with ADI Engineering (now Silicom Ltd) during the initial bring-up of Denverton on their platforms. I believe they feel they are putting the maintenance role in good hands with me. What I meant to convey is (as Mariusz has clarified) that there is are resource constraints (with everyone of course) and as Intel continues to push the boundaries of technology they need to move engineers into roles that will allow them to continue to push those boundaries in the most cost-effective and impactful way. Denverton is a mature product, and Intel always has a support plan in place for their products until they are EOL'd (which Denverton definitely is not AFAICT in danger of any time soon). I believe that this rearrangement in their support model is part of that (but I don't speak for Intel of course). Intel continues to support inclusion of support for Denverton in coreboot, but there will be a shift in day to day maintenance is all rather than the maintenance being completely internal. By 'moving away' I didn't intend to imply 'drop'. If that's how my comment was taken, for that I sincerely apologize. -----Original Message----- From: Szafranski, MariuszX <mariuszx.szafranski@intel.com> Sent: Wednesday, January 19, 2022 4:49 AM To: Peter Stuge <peter@stuge.se>; coreboot@coreboot.org; Jeff Daly <jeffd@silicom-usa.com> Subject: RE: [coreboot] Re: Gerrit shows a Merge Conflict (was: Re: Denverton-NS refactoring) Caution: This is an external email. Please take care when clicking links or opening attachments. Hi, To clarify situation (Jeff - looks like you got wrong my last email 😉 ) DNV will still be supported by Intel. Vanessa will continue to be primary support contact from Intel side. One thing that will change in next few months is that I and Suresh will move to another project and will have limited time to maintain DNV/HCV code. But we still be around 😉. So that’s main reason to propose Jeff as main maintainer for DNV/HCV. Mariusz -----Original Message----- From: Peter Stuge <peter@stuge.se> Sent: Tuesday, January 18, 2022 2:17 PM To: coreboot@coreboot.org Subject: [coreboot] Re: Gerrit shows a Merge Conflict (was: Re: Denverton-NS refactoring) Thank you for your updates, Jeff. Jeff Daly wrote:
the feedback I got from Intel is that they will be moving away from internal support for DNV coreboot.
It's sad and telling to see one of the most ambitious technology companies in the world, one that routinely pushed technology limits, choose to not continue to accept responsibility and be active in our firmware community. Value is never inherent, it's what we choose to create. Kind regards //Peter _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org -------------------------------------------------------------- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Hi, On 19.01.22 14:27, Jeff Daly wrote:
Intel continues to support inclusion of support for Denverton in coreboot, but there will be a shift in day to day maintenance is all rather than the maintenance being completely internal.
AFAICT, this doesn't mean it will shift but it may start. Denverton support in coreboot was mostly a code drop, FWIW, including redundant files, dead code. Not really maintained until the first Intel customer (IIRC, jvdg working for Scaleway) started to work on it. Over the years Intel had put a lot of names into our MAINTAINERS file, but there seemed to be a pattern: the more names, the lower coreboot was a priority. Also, more names often means less experience. For the initial code drop, review was basically skipped, so people never got the chance to learn upstream coreboot development. Might be an issue on our side. Later the maintainers list was made a running joke: When deprecation of the Denverton code was requested, Intel did one thing: They updated the list of names. But nothing else changed; e.g. review request from September, still unanswered. My interpretation: For Intel, maintaining DNV support in coreboot means that they provide contacts for their customers. But the maintainers were never given the resources to work with the open-source community. Rant is just about DNV support. It's a completely different story for other teams at Intel. Nico
Hi Jeff, those merge conflicts are only there for cherry-picking the specific patch without the ones before it directly on top of the current top of tree. When the patches before that one in the patch train are submitted, the merge conflict will disappear, so while this might look like a problem right now, it won't be one. Regards, Felix
participants (8)
-
Angel Pons -
Felix Held -
Felix Singer -
Jeff Daly -
Nico Huber -
Paul Menzel -
Peter Stuge -
Szafranski, MariuszX