[coreboot-gerrit] Change in coreboot[master]: soc/intel/common/block: Add Intel common Graphics controller support

[Furquan Shaikh (Code Review)]

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/graphics.c
File src/soc/intel/common/block/graphics/graphics.c:

https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/graphics.c@24
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/graphics.c@36
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/graphics.c@72
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/graphics.c@71
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/graphics.c@78
PS1, Line 78: graphics_get_gtt_base
Same comment as above.


https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/graphics.c@90
PS1, Line 90:    
use tabs instead of spaces like other structures?


https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/include/intelblocks/graphics.h
File src/soc/intel/common/block/include/intelblocks/graphics.h:

https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/include/intelblocks/graphics.h@40
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/intelblocks/graphics.h@49
PS1, Line 49: memory_base
Do you want to specify gmadr here to make it clear what base is being returned?



-- 
To view, visit https://review.coreboot.org/22614
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idbc73854ce9fc21a8a3e3663a98e01fc94d5a5e4
Gerrit-Change-Number: 22614
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Comment-Date: Tue, 28 Nov 2017 20:06:12 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20171128/fed6e47c/attachment.html>