Change in coreboot[master]: soc/intel/common/block: Add Intel common Graphics controller support
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/22614 )
Change subject: soc/intel/common/block: Add Intel common Graphics controller support ......................................................................
Patch Set 1:
(8 comments)
https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/... File src/soc/intel/common/block/graphics/graphics.c:
https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/... PS1, Line 24: __attribute__((weak)) void graphics_soc_init(struct device *dev) : { : /* no-op */ : } : : __attribute__((weak)) uintptr_t graphics_soc_write_acpi_opregion( : device_t device, uintptr_t current, : struct acpi_rsdp *rsdp) : { : return 0; : } Are these functions really optional? If not I think its better not to provide weak definitions.
https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/... PS1, Line 36: uintptr_t graphics_get_memory_base(void) : { : struct device *dev = SA_DEV_IGD; : struct resource *gm_res; : : /* : * GFX PCI config space offset 0x18 know as Graphics : * Memory Range Address (GMADR) : */ : gm_res = find_resource(dev, PCI_BASE_ADDRESS_2); : if (!gm_res || !gm_res->base) : return 0; : : return gm_res->base; : } : : static uintptr_t graphics_get_gtt_base(void) : { : struct device *dev = SA_DEV_IGD; : struct resource *gtt_res; : : /* : * GFX PCI config space offset 0x10 know as Graphics : * Translation Table Memory Mapped Range Address : * (GTTMMADR) : */ : gtt_res = find_resource(dev, PCI_BASE_ADDRESS_0); : if (!gtt_res || !gtt_res->base) : return 0; : : return gtt_res->base; You can also add a helper function for this:
static uintptr_t graphics_get_bar(unsigned long index) { struct device *dev = SA_DEV_IGD; struct resource *gtt_res;
gtt_res = find_resource(dev, index); if (!gtt_res) return 0;
return gtt_res->base; }
and then use it: uintptr_t graphics_get_memory_base(void) { return graphics_get_bar(PCI_BASE_ADDRESS_2); }
....
https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/... PS1, Line 72: graphics_get_gtt_base Shouldn't you check the return value of graphics_get_gtt_base before performing the read.
https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/... PS1, Line 71: uint32_t val; : val = read32((void *)(graphics_get_gtt_base() + reg)); : return val; Why not just: return read32(...);
https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/... PS1, Line 78: graphics_get_gtt_base Same comment as above.
https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/... PS1, Line 90: use tabs instead of spaces like other structures?
https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/graphics.h:
https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/include/i... PS1, Line 40: 0 = Error Shouldn't this return current as the caller is not validating the return value and it is being used for future calls to write_acpi_tables.
https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/include/i... PS1, Line 49: memory_base Do you want to specify gmadr here to make it clear what base is being returned?
-
Furquan Shaikh (Code Review)