Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45517 )
Change subject: nb/intel/common: Introduce common MCHBAR accessor functions ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45517/13/src/northbridge/intel/comm... File src/northbridge/intel/common/mchbar_ops.h:
https://review.coreboot.org/c/coreboot/+/45517/13/src/northbridge/intel/comm... PS13, Line 88: __always_inline void mchbar8_set(unsigned int offset, u8 set) Ugh, apologies. I just realized that the `mchbarX_set` name is very misleading:
mchbar32_set(SAPMCTL, 1UL << 31);
When I first read this, I understood the following:
set MCHBAR register `SAPMCTL` to `1UL << 31`
Which is not at all what this is doing. One could use `mchbarX_set_mask` (and update all other functions in a similar fashion), but this will make the function names rather long...
I think we could use the setbitsX/clrbitsX/clrsetbitsX macros from `src/include/device/mmio.h`, which have shorter names and provide the same `unset_and_set` semantics. What's more, we might as well just use the clrsetbitsX stuff directly:
#define __mchbar_clrsetbits_impl(bits, reg, clear, set) \ __clrsetbits_impl(bits, DEFAULT_MCHBAR + (reg), clear, set)
#define mchbar_clrsetbits8(reg, clear, set) __clrsetbits_impl(8, reg, clear, set)