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:
(2 comments)
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))
This was done in the same way as the per-channel MCHBAR register helpers.
Hmmm, that I didn't review it doesn't mean that I like it ;) I can guess what G, L and C refer too, but probably only because I wrote similar code for GM45 (I did follow some common pratices back then, too, which I might avoid today). I think the names should be more descriptive, and if we want to avoid confusion between DRAM byte lanes and DMI lanes, why not just put the definitions where they are used? or, if they are used in multiple compilation units, add a DMI header?
In any case, I would use `0x20` over `32` as the registers are in hex
sounds good, actually I prefer it :) I was just translating the << 5 while typing, I guess.
https://review.coreboot.org/c/coreboot/+/39634/3/src/northbridge/intel/sandy... PS3, Line 131: MAGIC_CHICKEN
I don't think so, but not giving it a name results in inconsistencies
I would choose readability over cosmetic consistency. However, I can't say what would be more readable in this case. If one wants to compare the code with some dull dump or trace, a number would make things easier. OTOH, if one wanted to compare it to reference code, it probably wouldn't matter.