Nitrokey boards coreboot version bump to match Dasharo+Heads heads+ coreboot version used in their v0.9.0 - 2024-02-29 BOM #1662
Conversation
modules/nitrokey-blobs
Outdated
| @@ -1,11 +1,11 @@ | |||
| modules-$(CONFIG_NITROKEY_BLOBS) += nitrokey-blobs | |||
|
|
|||
| nitrokey-blobs_base_dir := nitrokey-blobs | |||
| nitrokey-blobs_version := b629bad31046ff2f5f363656326646fe2081d033 | |||
| nitrokey-blobs_version := cba08e83d8bbd7d3470769afd7dbc8e61d6cd8b5 | |||
There was a problem hiding this comment.
This is referred by https://docs.dasharo.com/variants/novacustom_nv4x_adl/heads/#v090-2024-02-29
-> https://github.com/Dasharo/coreboot/tree/3a9aa3a4
You should review other blobs references here since Nitrokey maintains their ow coreboot blobs repo.
|
Don't try 4417b6e |
tlaurion
force-pushed
the
nitrokey_board_unification_clean-enable_htop_validated_autoboot-novacustom_coreboot_version_bump
branch
from
4417b6e
to
fa7ec37
Compare
|
@miczyg1 I had to cherry-pick your commits, but also resolve conflicts. |
|
@daringer @alex-nitrokey : tested fa7ec37 against nv41 and was no regression. Note that this changes the size of the bootsplash to be left alone (not resized) as per rebranding. For heads default logo, its now being showed without seeing the refresh happening with GOP vs libgfxinit, the later exposing the fb when filled as opposed to GOP writing it to screen as it goes. So here is the result: smaller, centered. I guess your bootplash will do the same per downstream heads config. |
|
Some diff present between coreboot configs of nv41/ns50: |
|
TODO:
|
tlaurion
changed the title
|
Flashed and works on nv41 5d7a66b |
|
@nestire @alex-nitrokey please test and approve. Note that this pr is based on prior pr + upstreaming novacustom coreboot release of Dasharo heads 0.9 of February. Delay of upstreaming and syncing with novacustom is way too long. |
LGTM, hope I didn't mess up anything in these commits I made. Thank you for picking them up... It was supposed to go back to main repo... |
This looks suspicious though. |
|
g5d7a66b is not working on ns50 I flashed it internally and externally the laptop is turning on (green DEL + turning fan) but the screen remains black |
tlaurion
force-pushed
the
nitrokey_board_unification_clean-enable_htop_validated_autoboot-novacustom_coreboot_version_bump
branch
from
5d7a66b
to
07dd373
Compare
@alexgithublab please review and test changes of 07dd373 |
@miczyg1 indeed. What should it be? And why is it that value from defconfig? |
@miczyg1 I do not know what would be the best process here. As discussed many times in the past, since you guys downstream apply changes, they should be in a branch and propose a PR upstream and when both succeed, merged and used in releases. Otherwise it creates a maintenance burden and a mess in the future, needing to fix cherry-picks, rebsasing and losing track of unique commits that should be the same. Those are all bad practices. I am not sure of what is the best way now and forward for this, both technically and politically. This creates a really big mess under Heads/Novacustom/Nitrokey issues, user experience problems and created issues blaming either of the forks or upstream for things having been fixed upstream outside of the coreboot parts. More collaboration is indeed needed. |
|
@miczyg1 the branding part of code under modules/coreboot is two folded, and then exposed through SMBIOS: Then Is that desired? If one goes through "Heads System Information" as of today (07dd373)for both Nitrokey and Novacustom boards, the result would be based on: |
|
@tlaurion the idea behind these changes is to let site-local override the aforementioned SMBIOS fields. If CONFIG_COREBOOT_LOCALVERSION and CONFIG_COREBOOT_SMBIOS_PRODUCT_NAME are not defined in the environment, the previous behavior is supposed to be preserved ( Example of site-local override of these values: https://github.com/Dasharo/heads/blob/master/site-local/config The sed commands are supposed to remove existing CONFIG_LOCALVERSION and CONFIG_MAINBOARD_SMBIOS_MANUFACTURER in the coreboot's config file and place the new entries accordingly when coreboot module is configured: Dasharo@4d0c7a1 It is done to ensure there are no duplicate CONFIG_* entries. |
Ohh that may be our fault. There is no setting for nv4x: https://github.com/Dasharo/coreboot/blob/38215f5a2bb3ea8ead10bf9c481caeb07d3a19ad/src/mainboard/clevo/adl-p/Kconfig#L118 |
@miczyg1 will force that on both boards. |
Thanks, the pin for TPM_PIRQ is the some for both boards, so it will be good. |
@miczyg1 dont forget to add it in Kconfig for defconfig |
|
Rebasing on master which now contains #1640 |
…unification_clean-enable_htop_validated_autoboot-novacustom_coreboot_version_bump
|
85bd68f clarifies where to get support from if upstream forks are used, to not spam upstream for unfixed downstream forks issues. |
…unification_clean-enable_htop_validated_autoboot-novacustom_coreboot_version_bump
|
If future maintenance is provided by 3mdeb/Dasharo or NovaCustom I'm ok with the suggested rebranding. Not sure if this is the agreed approach. |
@jans23 @daringer @miczyg1 @pietrushnic Quick recap on actionnable items.
As of now, there is no maintenance of upstream Heads code in the sense of PR pushed to move modules/coreboot nitropad/novacustom coreboot by any 3rd party in time. My point here is that needs to change, but if it doesn't, I could only track Dasharo+heads releases and cherry-pick considering that it would be one source to follow where commits should be able to be cherry-picked without having to resolve conflicts nor rewrite the git history because the commits, if that was done in time. Of course, the ideal for Heads would be to (we already discussed that numerous times, on which I would agree):
If we agree on something like this, I think the maintenance burden could be shared easily and we could agree on who will do it
As for the current state of this PR, since nv41 was tested working:
The only question requiring a coupe of minutes thinking is what format the patches should have under
The board renaming, and rebranding, will happen in another PR. This PR goal was to bring Heads nv41/ns50 boards par to coreboot+heads release of 2024-02.29 and stop using coreboot of December 13 2023 with all bugs being reported at 3 places, most of which are currently fixed, and for me to not have to close eyes on what is being discussed everywhere else including forums for bugs that I know are fixed under Novacustom laptops and creating bad UX and discrepencies in usage for the same physical platforms. |
I will switch to PR based patches under |
…am dasharo+heads unreleased patches to apply on top of last release PR numbers being numerical and hopefully not conflicting with each other, keeping track of commits per their upstream PR should make sure they can be applied cleanly on top of each other as opposed to commit id related patches that git apply will apply in random order. Signed-off-by: Thierry Laurion <[email protected]>
|
@tlaurion Thanks for bringing this up. TBH it is difficult to follow the pace and volume of the comments in this MR. I trust that #1662 (comment) summarizes all action items, we will provide feedback on this ASAP, preferably tomorrow. Please ping me directly for this, I will try to allocate someone to help with this effort. Especially, no need to ping @miczyg1 right now, as he will not be able to help much more here right now in a short timeframe, due to other assignments. |
@macpijan Yes, that comment was produced to give current state and desired state to merge this PR to make downstream release par with master and to move forward without the current and evolving mess and discrepencies between Heads forks that become silos and commits that cannot be Cherry-picked, which needs to change now. And the last commit changed strategy to pick commit of merged PR downstream to be easily tracked for upstream/downstram discrepencies and commits needed to fix problems not yet released under dasharo+heads releases. Let's remember that Heads upstream is a rolling release, and therefore should be considered bleeding edge as compared to downstream releases that pick a heads commit. If we start acting accordingly and use heads PR for users to test fixes and features, Heads is the perfect place to create PR that will self build reproducible ROMs per CircleCI and finally act as designed: being a rolling release having latest changes to test and be picked downstream for releases, **not the other way around. ** |
@macpijan Yes, that comment was produced to give current state and desired state to merge this PR to make downstream release par with master and to move forward without the current and evolving mess and discrepencies between Heads forks that become silos and commits that cannot be Cherry-picked, which needs to change now. And the last commit changed strategy to pick commit of merged PR downstream to be easily tracked for upstream/downstram discrepencies and commits needed to fix problems not yet released under dasharo+heads releases. Let's remember that Heads upstream is a rolling release, and therefore should be considered bleeding edge as compared to downstream releases that pick a heads commit. If we start acting accordingly and use heads PR for users to test fixes and features, Heads is the perfect place to create PR that will self build reproducible ROMs per CircleCI and finally act as designed: being a rolling release having latest changes to test and be picked downstream for releases, not the other way around. |
This is incorrect and misleading. Dasharo (coreboot+Heads) releases are under Dasharo Revenue Sharing, which means all work (not counting the initial coreboot port paid by NovaCustom and the initial Heads port made by Nitrokey) was created by the Dasharo Team as an investment in the hope of selling Dasharo Entry Subscription, so NovaCustom never paid and will not pay to 3mdeb for Heads related development. |
@pietrushnic @jans23 sorry for the verbiage shortcut I took there. What I meant is exactly what you said @pietrushnic : this gets paid only if OEM resellers of the same platforms/boards, or if platforms depending on coreboot fork and maintained heads code (coreboot+Heads) are part of the Dasharo Revenue Sharing agreement, so that Dasharo entry subscription are mandatory sold when buying the product so that people doing the work behind the scenes actually get paid and are motivated into doing the hidden work. Else, you have Novacustom selling Dasharo Entry Subscription and Nitrokey not selling subscription for same platform: users don't understand really the difference but in price differences at time of buying unless highly ethical and wanting to support firmware development and maintenance. I think its an ethical responsibility to support pr,ofit sharing, since the model exists, otherwise its expected that the work would be done in a timely manner ffrom Nitrokey internal resources and upstreaming and collaboration done under upstream forks to balance things out, otherwise users and the whole ecosystem suffers (and becomes bitter. Who wouldn't?) But at the end, Nitrokey practically uses code of both coreboot port and Heads for free as in free beer after initial porting effort. I'm grateful for the initial port (and the bugfixes I provided and worked countless or hours fixing bugs, referring to issues, closing issues, answering forum posts) for free (and also participated in each past releases for free). This is politics and business decisions. Not something that can be fixed in code. I'm being told to stop working for free. But honestly, how can I do but by dropping those boards from main tree and make the forks accountable if things are not fixed in time and that Heads branding is used making Heads accountable for things it isn't? It's not that doing that is illegal. Most of immoral actions are actually not illegal in our world. It's about doing the right thing and forcing the subscription at moment of buying in the shop (no: not optional), so people doing the work actually gets paid. Users wanting to buy such platforms having the same coherent options, including the Dasharo Entry Subscription. But noone can force anybody to do the moral> legal choices. Only illegal is punished by law, and using other people's work/forks in open source is common all around. It always hurt the ecosystem, but sometimes, like here, it has been discussed in the back scenes so much and nothing changed for so long that a maintainer has to say: hey, that's enough right? What can I say but tell : let's not make the story repeat itself and include the Dasharo Entry subscription in your shop alongside sold products, only offering differences in years of subscription, not as an option. This probable issue was discussed early in development #121 but discarded. But this is GPLv2 based licence, guys. Without falling in the licencing legal issues and resolution, the idea here is to contribute back without the burden being on my upstream projects to be accountable for bad downstream practices. But at the end of the day, it's a business decision, having massive hidden impacts, some exposed here, but still a legal one to not have people working behind the scenes paid for their work properly or being compensated by code contribution in a responsible, timely manner. And also legal to offer lesser good UX to end users that don't understand why things are broken and unfixed for so long, not understanding that code used is from nearly a year ago from coreboot and months ago from heads side. But this is a legal and valid business decision. Tldr: this part cannot be fixed as code. It's a really weird thing to be in my shoes and having to deal with platforms sold on one side ethically promoting open source business ecosystem and making it thrive the best way possible, while in the other side having one that uses both code bases, do not maintain their own code base downstream nor upstream which bot bitrot and creates chaos among the same platform users from the same machines bought from two OEM seeling the same physical platform and me having to deal with this for free, being asked to "not do anything" which if I did, would give a bad reputation to both Dasharo coreboot codebase and Heads codebase since lack of proper and due diligence based maintenance and collaboration. If one doesn;t have internal resource, then one should sell Dasharo Entry subscription if that exists for that platform. It should not more complicated then that, but it is. Why? Not for me to answer. I have no solution here but that :
Politics cannot be translated properly in code here. It's about doing the right thing or not. Yes: that simple. |
|
@macpijan merge, no merge? |
|
Modified OP. Waiting for a go. |
On 3mdeb side, we are fine with pushing the changes before the next release first to upstream, and keeping just the logo and stuff in fork. Just a note, that currently we only "support" NV41 model for heads. We are fine with having both under NovaCustom and providing coreboot base (we will continue to working on coreboot base for all of the models going forward). But we may not be able/willing to test the units with heads (such as ns50) not sold by @wessel-novacustom . |
Let's hope there will be not that many cases of such patches, and we can just update the coreboot revision. But if this is required, I would be in favor of single commit per patch. But ordering of application matters. The typical approach would be to prepend the patch with 0001, 0002 etc. I think it's a default for |
|
Ok I will try to address this properly in subsequent PR. Let's merge this PR and sorry for the coreboot+heads commit history that will need to be rewritten in your branches @macpijan . Hopefully we will have learned from this and not repeat the past errors moving forward. Merging. |
This is not the problem at all. |
@macpijan arrrrrg. Pushed merge after seing chat approval. So I merged with format of patches being PR name, which is numerical and should not conflict. Should I push fix to change this to 0001-501.patch or 0001-commit.patch (see a51a7af) |
No, let's save some energy. This should be fine as well. |
@jans23 this needs to be talked off channel after poundering past decisions and future ones. |
…41|s50] coreboot.modify_and_save_oldconfig_in_place removes a comment: -# CONFIG_DASHARO_FIRMWARE_UPDATE_MODE is not set - Unify ns50/nv41 - CONFIG_TPM_PIRQ=0x27 in both nv41/ns50 as per linuxboot#1662 (comment) NOTE that this doesn't stick when calling make[1]: Leaving directory '/home/user/heads/build/x86/coreboot-dasharo' user@heads-tests-deb12:~/heads$ git diff diff --git a/config/coreboot-nitropad-nv41.config b/config/coreboot-nitropad-nv41.config index 9484aaf5122..ddd4e5d7c56 100644 --- a/config/coreboot-nitropad-nv41.config +++ b/config/coreboot-nitropad-nv41.config @@ -143,7 +143,7 @@ CONFIG_BOARD_CLEVO_NV40PZ_BASE=y CONFIG_MAINBOARD_SMBIOS_PRODUCT_NAME="Nitropad NV41" CONFIG_CONSOLE_POST=y # CONFIG_USE_PM_ACPI_TIMER is not set -CONFIG_TPM_PIRQ=0x27 +CONFIG_TPM_PIRQ=0x0 # CONFIG_SOC_INTEL_CSE_SEND_EOP_EARLY is not set CONFIG_VBOOT_FWID_VERSION="$(CONFIG_LOCALVERSION)" CONFIG_EC_SYSTEM76_EC_BAT_THRESHOLDS=y Also note that CONFIG_EC_SYSTEM76_EC_DGPU=y is not present on ns50 as opposed to nv41, whatever that does. user@heads-tests-deb12:~/heads$ diff -u config/coreboot-nitropad-nv41.config config/coreboot-nitropad-ns50.config --- config/coreboot-nitropad-nv41.config 2024-05-10 14:59:42.156754718 -0400 +++ config/coreboot-nitropad-ns50.config 2024-05-10 14:55:37.699761391 -0400 @@ -110,7 +110,7 @@ # CONFIG_VENDOR_TI is not set # CONFIG_VENDOR_UP is not set CONFIG_MAINBOARD_FAMILY="Not Applicable" -CONFIG_MAINBOARD_PART_NUMBER="nv40pz" +CONFIG_MAINBOARD_PART_NUMBER="ns50pu" CONFIG_MAINBOARD_VERSION="v2.1" CONFIG_MAINBOARD_DIR="clevo/adl-p" CONFIG_DIMM_MAX=4 @@ -128,7 +128,7 @@ CONFIG_DEVICETREE="devicetree.cb" # CONFIG_VBOOT is not set CONFIG_VBOOT_VBNV_OFFSET=0x28 -CONFIG_VARIANT_DIR="nv40pz" +CONFIG_VARIANT_DIR="ns50pu" CONFIG_OVERRIDE_DEVICETREE="variants/$(CONFIG_VARIANT_DIR)/overridetree.cb" # CONFIG_VGA_BIOS is not set CONFIG_MAINBOARD_SMBIOS_MANUFACTURER="Nitrokey" @@ -139,8 +139,8 @@ CONFIG_CMOS_LAYOUT_FILE="src/mainboard/$(MAINBOARDDIR)/cmos.layout" CONFIG_BOOT_DEVICE_SPI_FLASH_BUS=0 CONFIG_BOARD_CLEVO_ADLP_COMMON=y -CONFIG_BOARD_CLEVO_NV40PZ_BASE=y -CONFIG_MAINBOARD_SMBIOS_PRODUCT_NAME="Nitropad NV41" +CONFIG_BOARD_CLEVO_NS50PU_BASE=y +CONFIG_MAINBOARD_SMBIOS_PRODUCT_NAME="Nitropad NS51" CONFIG_CONSOLE_POST=y # CONFIG_USE_PM_ACPI_TIMER is not set CONFIG_TPM_PIRQ=0x27 @@ -158,8 +158,8 @@ CONFIG_HAVE_INTEL_FIRMWARE=y CONFIG_MRC_SETTINGS_CACHE_SIZE=0x10000 CONFIG_DRIVERS_INTEL_WIFI=y -CONFIG_IFD_BIN_PATH="3rdparty/dasharo-blobs/novacustom/nv4x_adl/descriptor.bin" -CONFIG_ME_BIN_PATH="3rdparty/dasharo-blobs/novacustom/nv4x_adl/me.bin" +CONFIG_IFD_BIN_PATH="3rdparty/dasharo-blobs/novacustom/ns5x_adl/descriptor.bin" +CONFIG_ME_BIN_PATH="3rdparty/dasharo-blobs/novacustom/ns5x_adl/me.bin" CONFIG_CONSOLE_CBMEM_BUFFER_SIZE=0x20000 CONFIG_VBT_DATA_SIZE_KB=9 CONFIG_CARDBUS_PLUGIN_SUPPORT=y @@ -176,8 +176,8 @@ # # Alder Lake P (2022) # -# CONFIG_BOARD_NOVACUSTOM_NS5X_ADLP is not set -CONFIG_BOARD_NOVACUSTOM_NV4X_ADLP=y +CONFIG_BOARD_NOVACUSTOM_NS5X_ADLP=y +# CONFIG_BOARD_NOVACUSTOM_NV4X_ADLP is not set # # Tiger Lake U (2021) @@ -503,7 +503,6 @@ # CONFIG_EC_ACPI=y CONFIG_EC_SYSTEM76_EC=y -CONFIG_EC_SYSTEM76_EC_DGPU=y # # Intel Firmware Signed-off-by: Thierry Laurion <[email protected]>
EDIT: state of the current situation prior of this PR being merged at #1662 (comment)
Based on #1640 + coreboot config changes, Makefile and modules/coreboot changes made downstream in Dasharo+heads branch in commit as can be seen in this snapshot made today: https://archive.is/zYFbU#selection-6978.0-6984.0
Cherry-picked (and resolved conflicts) from relevant commits from referred BOM's
@daringer please don't let this bitrot and report from testing laptops:
Important changes to review here:
needed patches/ for nitrokey fork gone (need to be crafted again unless merged under dasharo's coreboot fork)EDIT: no more usage of nitrokey's coreboot fork nor patches. Actually the directory of clevo patches is gone under patches/*patches/coreboot-dasharo-unreleasedin the form of Dasharo PR number under dasharo+heads tree as PR_NUMBER.patch