Angel Pons 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:
(3 comments)
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()`?
Right, I can use underscores if you want. For example, `AFE_BNDx_CFGy(x, y)`
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 […]
This was done in the same way as the per-channel MCHBAR register helpers.
In any case, I would use `0x20` over `32` as the registers are in hex
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 […]
They can be used for PCIe, but I think the number of bundles/lanes can change. I can move them to early_dmi.c if desired.
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 […]
I don't think so, but not giving it a name results in inconsistencies