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 6:
(7 comments)
https://review.coreboot.org/c/coreboot/+/45517/6/src/northbridge/intel/commo... File src/northbridge/intel/common/mchbar_ops.h:
https://review.coreboot.org/c/coreboot/+/45517/6/src/northbridge/intel/commo... PS6, Line 8: #if CONFIG(NORTHBRIDGE_INTEL_X4X) : #include <northbridge/intel/x4x/iomap.h> : #elif CONFIG(NORTHBRIDGE_INTEL_PINEVIEW) : #include <northbridge/intel/pineview/memmap.h> : #elif CONFIG(NORTHBRIDGE_INTEL_SANDYBRIDGE) : #include <northbridge/intel/sandybridge/sandybridge.h> : #elif CONFIG(NORTHBRIDGE_INTEL_IRONLAKE) : #include <northbridge/intel/ironlake/ironlake.h> : #elif CONFIG(NORTHBRIDGE_INTEL_HASWELL) : #include <northbridge/intel/haswell/memmap.h> : #endif yeah... this is something I wanted to avoid. I guess this will have to do in the meantime.
I would add an #else-block with an #error to avoid potential problems
https://review.coreboot.org/c/coreboot/+/45517/6/src/northbridge/intel/commo... PS6, Line 20: x This is unsafe. Please add braces around the macro parameters.
https://review.coreboot.org/c/coreboot/+/45517/6/src/northbridge/intel/commo... PS6, Line 24: int This should never be negative
https://review.coreboot.org/c/coreboot/+/45517/6/src/northbridge/intel/commo... PS6, Line 27: void mchbar8_and(int offset, u8 value; Where did the closing brace go?
https://review.coreboot.org/c/coreboot/+/45517/6/src/northbridge/intel/commo... File src/northbridge/intel/common/mchbar_ops.c:
https://review.coreboot.org/c/coreboot/+/45517/6/src/northbridge/intel/commo... PS6, Line 13: void mchbar8_unset_and_set(int offset, u8 unset, u8 set) Any specific reason to not make these inline? I'd recommend trying both and checking which results in a smaller binary when actually used in code.
https://review.coreboot.org/c/coreboot/+/45517/6/src/northbridge/intel/commo... PS6, Line 40: void mchbar8_and(int offset, u8 value) For consistency, these should be `mchbarX_unset` functions (or you will have the same problem with the masks)
https://review.coreboot.org/c/coreboot/+/45517/6/src/northbridge/intel/commo... PS6, Line 64: void mchbar8_or(int offset, u8 value) And these should be called mchbarX_set