Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add initial support for the ThinkPad T440p #1282
Add initial support for the ThinkPad T440p #1282
Changes from all commits
5cce937
e325976
96f0c5b
f079211
65be2c5
24d23ff
c23ed54
7a29db1
ed8c74e
9368404
7c32d4e
e6c34bd
144f9c1
1dc5d4e
5083ba3
3efec15
63eab71
e4a09e8
1dd9c26
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a novel approach here. I didn't want to duplicate any config. I realized since this is just a Makefile, we can use the
include
directive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These magic values were copied from the X230 config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make
rule syntax is:targets : prerequisites recipe …
Where a "target" is a path to a file that needs to exist. So, we create two targets for
mrc.bin
andme.bin
with recipes that run the corresponding blob scripts. And we make the Coreboot build target (defined elsewhere) depend on these blob targets.Since a target is a path to a file, if the blobs already exist (e.g., user-supplied blobs),
make
will skip these recipes.See: https://www.gnu.org/software/make/manual/make.html#Rule-Syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value for the CBFS size is arbitrary. Originally, I had totaled the size of all binary blobs, subtracted that from the T440p's ROM size (12 MiB), and used the remaining space as the CBFS size (~11.68 MiB). However, this caused very long RAM initialization times (courtesy of
cbmem -t
). And, an anecdote in https://groups.google.com/a/chromium.org/g/chromium-os-reviews/c/lUqRrGUoEBY/m/ka7L1f2BS8gJ suggested that this value needs to be a power of 2.So, I picked a size I expected our Linux payload to fit into that was a power of 2 that I also expected would leave enough space in the ROM for the IFD, ME, GbE, and Coreboot.
Now, it takes less than a second for RAM initialization after flashing/first boot (anecdotally, it seems the MRC needs to be "trained?").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the impression that your initial CBFS_SIZE value might have been good, but not matching ifd defined region).
Ifd needs to be modified to take output of neutered ME size while ME region needs to be reduced under ifd, and the freed space added to BIOS region there. That BIOS size would become the size of cbfs region under coreboot config from memory, but will have to find traces again which I was unsuccessful finding last time.
Will try to find that information back since it needs to be part of the porting guide as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay, this makes sense. I ran me_cleaner on my original ROM again and then used
ifdtool
to dump the IFD regions:Putting this into a notepad (using Soulver), I determined the size of each region and that there is 1 byte of padding between each region:
So maybe
0xBDEFFF
is the maximum CBFS size? I will recompile, flash, and see. It would be cool if we could automate this math.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update:☹️ .
0xBDEFFF
did not work. RAM initialization times are back up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rbreslow I finally joined coreboot channel and posted my question directly to them here: https://matrix.to/#/!BZxDFuoBnMKnzVZwEp:libera.chat/$N_Nh4qJeU9gquoE9tzjeERYzsxgYB6j79uB6uvnoPdM?via=libera.chat&via=matrix.org&via=foss.wtf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
icon on coreboot channel answered. Excerpt:
I replied:
Then icon replied:
nic3-14159 added:
@rbreslow Can you adjust the calculations and test with new CBFS? Might be a limitation of the MRC blob on which we might need to accept caching time (and document properly under board config as current limitation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tlaurion Here are my
cbmem
logs from theCBFS_SIZE
set0xBDEFFF
: https://gist.github.com/rbreslow/50b45e538437dd10f9e2bf51abd260de. What size would you like me to try now?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, the right calculation is taking output of ifdtool -f layout.txt path/to/ifd_output_from_me_cleaner_on_fullrom_backup.rom and parsing the BIOS region, substracting end address to begin address.
@rbreslow Seems like xx30 and xx20 might have been off all along. Redoing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this, neither Qubes OS nor the Qubes OS installer would start. Presumably, because we're "kexecing" from an already running kernel, we need this set at the Coreboot level? Testing revealed that including
intel_iommu=igfx_off
in theCONFIG_BOOT_KERNEL_ADD
board config option did nothing. And, the Qubes OS default boot option already containsintel_iommu=igfx_off
.See:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. Otherwise the iommu is configured incorrectly since first kernel boot would try to isolate Intel iommu and the kexec'ed kernel would not be able set differently.
Coreboot defined kernel option defines Heads behavior.