Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47988 )
Change subject: soc/intel/common/dmi: Add DMI driver support ......................................................................
Patch Set 1:
(11 comments)
https://review.coreboot.org/c/coreboot/+/47988/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47988/1//COMMIT_MSG@9 PS1, Line 9: is drop "is"
https://review.coreboot.org/c/coreboot/+/47988/1//COMMIT_MSG@10 PS1, Line 10: set up the decoding of extended : BIOS region in DMI GPMR configuration can be used for any decoding. One use case is decoding of extended BIOS region.
https://review.coreboot.org/c/coreboot/+/47988/1//COMMIT_MSG@15 PS1, Line 15: It can ensure that the PCR base address is configured Actually, it is SoC's responsibility to ensure that.
https://review.coreboot.org/c/coreboot/+/47988/1//COMMIT_MSG@20 PS1, Line 20: 2 3
https://review.coreboot.org/c/coreboot/+/47988/1/src/soc/intel/common/block/... File src/soc/intel/common/block/dmi/dmi.c:
https://review.coreboot.org/c/coreboot/+/47988/1/src/soc/intel/common/block/... PS1, Line 8: 2 Comment says 3? For now I think it is fine to put these definitions here. But for a future platform we should evaluate if this is SoC specific or can be used across SoCs. Same with macros below.
https://review.coreboot.org/c/coreboot/+/47988/1/src/soc/intel/common/block/... PS1, Line 11: #define DMI_PCR_GPMR_BASE_SHIFT 16 : #define DMI_PCR_GPMR_LIMIT_MASK 0xffff0000 : #define DMI_PCR_GPMR_BASE_MASK 0xffff : #define DMI_PCR_GPMR_EN BIT(31) Typically fields within a register are defined below it and with a single space before the name.
i.e. #define GPMR_OFFSET(x) (0x277c + (x)*8) #define DMI_PCR_GPMR_LIMIT_MASK 0xffff0000 #define DMI_PCR_GPMR_BASE_SHIFT 16 #define DMI_PCR_GPMR_BASE_MASK 0xffff
#define GPMR_DID_OFFSET(x) (0x2780 + (x)*8) #define DMI_PCR_GPMR_EN BIT(31)
https://review.coreboot.org/c/coreboot/+/47988/1/src/soc/intel/common/block/... PS1, Line 29: uint32_t "int" since in the failed case you are returning -1.
https://review.coreboot.org/c/coreboot/+/47988/1/src/soc/intel/common/block/... PS1, Line 50: Chekc if gpmr_num is not -1.
if (gpmr_num == -1) return;
https://review.coreboot.org/c/coreboot/+/47988/1/src/soc/intel/common/block/... PS1, Line 53: size This is not correct. You will have to calculate limit.
uint32_t limit = base + size - 1;
if (base & ~(DMI_PCR_GPMR_BASE_MASK << DMI_PCR_GPMR_BASE_SHIFT)) { printk(BIOS_ERR, "base is not 64-KiB aligned!\n"); return CB_ERR; }
if (limit & ~DMI_PCR_GPMR_LIMIT_MASK) { printk(BIOS_ERR, "limit does not end on a 64-KiB boundary!\n"); return CB_ERR; }
gpmr_write32(GPMR_OFFSET(gpmr_num), (limit & DMI_PCR_GPMR_LIMIT_MASK) | ((base >> DMI_PCR_GPMR_BASE_SHIFT) & DMI_PCR_GPMR_BASE_MASK));
https://review.coreboot.org/c/coreboot/+/47988/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/dmi.h:
https://review.coreboot.org/c/coreboot/+/47988/1/src/soc/intel/common/block/... PS1, Line 9: * Takes base, size and destination ID and configures the GPMR
trailing whitespace
Needs to be fixed.
https://review.coreboot.org/c/coreboot/+/47988/1/src/soc/intel/common/block/... PS1, Line 12: void Probably should return enum cb_err to indicate if the GPMR was enabled.