Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39634 )
Change subject: nb/intel/sandybridge: Use macros for DMI init ......................................................................
Patch Set 3: Code-Review-1
(3 comments)
Sorry for this, but um, I fear this doesn't make the code more readable. I see that it makes things look prettier, though, but that's not the final goal, is it?
When I look at
DMIBAR32(0x900 + x)... DMIBAR32(0x900 + x)...
DMIBAR32(0x904 + x)... DMIBAR32(0x904 + x)...
I see different numbers. But when we use an unspeakable, all-caps macro name, where the difference is only a small portion of the name
DMIBAR32(AFEBNDxCFG0(x))... DMIBAR32(AFEBNDxCFG0(x))...
DMIBAR32(AFEBNDxCFG1(x))... DMIBAR32(AFEBNDxCFG1(x))...
I just see a search-image and have to concentrate to read it.
One way to make this more readable again, would be to make the number an argument, e.g.
DMIBAR32(AFEBNDxCFG(x, 0))... DMIBAR32(AFEBNDxCFG(x, 0))...
DMIBAR32(AFEBNDxCFG(x, 1))... DMIBAR32(AFEBNDxCFG(x, 1))...
Maybe some underscores would help too, `AFE_BNDx_CFG()`?
https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/sandybridge.h:
https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... PS3, Line 118: /* Per-bundle and per-lane register group helpers */ : #define BUNDLE(reg, bundle) ((reg) + ((bundle) << 5)) : #define P_LANE(reg, p_lane) ((reg) + ((p_lane) << 4)) These add another layer of indirection when reading the code, and I don't see what we gain from them. Isn't
(0x900 + (bundle) * 32)
much easier to read than
BUNDLE(0x900, bundle)
?
https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... PS3, Line 122: /* Per-bundle and per-lane register group iterators */ : #define FOR_EACH_BUNDLE(bundle) for (bundle = 0; bundle < 2; bundle++) : #define FOR_EACH_P_LANE(p_lane) for (p_lane = 0; p_lane < 4; p_lane++) Will these be used for non-DMI things? If you define them at this scope, they should have `DMI` in the name, it's too easy to confuse with other lanes. Alternatively, if that works, move them to `early_dmi.c`?
https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... PS3, Line 131: MAGIC_CHICKEN Is this name used anywhere else? If not, how is it better than using the number in the code?