Support Genoa #123
Support Genoa #123
Conversation
src/ondisk.rs
Outdated
| // XXX: Not that useful. | ||
| impl Default for DdrDqPinMapElementLane { |
There was a problem hiding this comment.
Can we drop this impl then or is it actually used somewhere?
There was a problem hiding this comment.
No--since it is used.
It's a design limitation of the impl_struct_serde_conversion macro.
The reason is if we have a struct like this
struct X {
a: u32,
b: u32,
}
... and then build an instance up like this (which is what impl_struct_serde_conversion does):
X::builder().with_a(3).with_b(4).build()
... it actually returns an instance of X at X::builder() already. That means it has to default it--what would be the values of a and b otherwise? Even let's say we avoided that somehow, what's the value of the expression X::builder().with_a(3) ? Remember that X is the actual memory-mapped AMD struct--so we can't just change it to make all fields optional or something.
This could be avoided by having some (smarter) intermediate builder struct--but most of the time you really DO want defaults anyway.
In the specific case of DdrDqPinMapElementLane you want to use all 32 possible values but then you'd repeat them again anyhow--so it's not clear how to materially improve the accessors or validation.
src/ondisk.rs
Outdated
| #[derive(FromBytes, AsBytes, Unaligned, PartialEq, Debug, Copy, Clone)] | ||
| #[repr(C, packed)] | ||
| pub struct Ddr5CaPinMapElementLane { | ||
| pins: [u8; 14], // TODO: nicer pin type instead of u8; especially for 0xff "un" |
There was a problem hiding this comment.
Maybe a general clean memory types clean up issue to link with these TODOs.
|
I have much the same questions as Luqman. Also, we should rename the lone WIP commit to something that's not a WIP. |
daym
force-pushed
the
issue-110
branch
2 times, most recently
from
6735d0d to
596a131
Compare
…isted under wrong data types.
daym
force-pushed
the
issue-110
branch
2 times, most recently
from
ce31b34 to
a76f2dd
Compare
|
Hi, this breaks compatibility with several CPU families (at least Renoir/Cezanne), which use "BCBA" as signature ending (not sure it was fully compatible before but after this commit it is definitely not). Confirmed: Renoir/Cezanne did work before this commit. |
Fixes #110.
Successfully tested on opal.