Srinidhi N Kaushik 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"
Ack
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.
Ack
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.
Ack
https://review.coreboot.org/c/coreboot/+/47988/1//COMMIT_MSG@20 PS1, Line 20: 2
3
Ack
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. […]
I was using it as 0-2 (3). I can change it to 3 and do 1-3, let me know. Yes, for future platforms we should move if there are any changes to number of GPMR's.
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. […]
Ack
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.
Ack
https://review.coreboot.org/c/coreboot/+/47988/1/src/soc/intel/common/block/... PS1, Line 50:
Chekc if gpmr_num is not -1. […]
Ack
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. […]
Agree, I was initially under the impression that size was limit. I understood from your previous comment and the document. I will change this.
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
Needs to be fixed.
Ack
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.
Ack