<p>Furquan Shaikh <strong>posted comments</strong> on this change.</p><p><a href="https://review.coreboot.org/22614">View Change</a></p><p>Patch set 1:</p><p>(8 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/graphics.c">File src/soc/intel/common/block/graphics/graphics.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/graphics.c@24">Patch Set #1, Line 24:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">__attribute__((weak)) void graphics_soc_init(struct device *dev)<br>{<br> /* no-op */<br>}<br><br>__attribute__((weak)) uintptr_t graphics_soc_write_acpi_opregion(<br> device_t device, uintptr_t current,<br> struct acpi_rsdp *rsdp)<br>{<br> return 0;<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Are these functions really optional? If not I think its better not to provide weak definitions.</p></li><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/graphics.c@36">Patch Set #1, Line 36:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">uintptr_t graphics_get_memory_base(void)<br>{<br> struct device *dev = SA_DEV_IGD;<br> struct resource *gm_res;<br><br> /*<br> * GFX PCI config space offset 0x18 know as Graphics<br> * Memory Range Address (GMADR)<br> */<br> gm_res = find_resource(dev, PCI_BASE_ADDRESS_2);<br> if (!gm_res || !gm_res->base)<br> return 0;<br><br> return gm_res->base;<br>}<br><br>static uintptr_t graphics_get_gtt_base(void)<br>{<br> struct device *dev = SA_DEV_IGD;<br> struct resource *gtt_res;<br><br> /*<br> * GFX PCI config space offset 0x10 know as Graphics<br> * Translation Table Memory Mapped Range Address<br> * (GTTMMADR)<br> */<br> gtt_res = find_resource(dev, PCI_BASE_ADDRESS_0);<br> if (!gtt_res || !gtt_res->base)<br> return 0;<br><br> return gtt_res->base;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">You can also add a helper function for this:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">static uintptr_t graphics_get_bar(unsigned long index)<br>{<br> struct device *dev = SA_DEV_IGD;<br> struct resource *gtt_res;</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> gtt_res = find_resource(dev, index);<br> if (!gtt_res)<br> return 0;</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> return gtt_res->base;<br>}</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">and then use it:<br>uintptr_t graphics_get_memory_base(void)<br>{<br> return graphics_get_bar(PCI_BASE_ADDRESS_2);<br>}</pre><p style="white-space: pre-wrap; word-wrap: break-word;">....</p></li><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/graphics.c@72">Patch Set #1, Line 72:</a> <code style="font-family:monospace,monospace">graphics_get_gtt_base</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Shouldn't you check the return value of graphics_get_gtt_base before performing the read.</p></li><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/graphics.c@71">Patch Set #1, Line 71:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">uint32_t val;<br> val = read32((void *)(graphics_get_gtt_base() + reg));<br> return val;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why not just:<br>return read32(...);</p></li><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/graphics.c@78">Patch Set #1, Line 78:</a> <code style="font-family:monospace,monospace">graphics_get_gtt_base</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same comment as above.</p></li><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/graphics/graphics.c@90">Patch Set #1, Line 90:</a> <code style="font-family:monospace,monospace"> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">use tabs instead of spaces like other structures?</p></li></ul></li><li><p><a href="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:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/include/intelblocks/graphics.h@40">Patch Set #1, Line 40:</a> <code style="font-family:monospace,monospace">0 = Error</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/22614/1/src/soc/intel/common/block/include/intelblocks/graphics.h@49">Patch Set #1, Line 49:</a> <code style="font-family:monospace,monospace">memory_base</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Do you want to specify gmadr here to make it clear what base is being returned?</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/22614">change 22614</a>. To unsubscribe, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/22614"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Idbc73854ce9fc21a8a3e3663a98e01fc94d5a5e4 </div>
<div style="display:none"> Gerrit-Change-Number: 22614 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> </div>
<div style="display:none"> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 28 Nov 2017 20:06:12 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>