<p style="white-space: pre-wrap; word-wrap: break-word;">Hi Maulik, I think this driver could easily be modified to work with<br>any coreboot framebuffer (not just FSP ones). Though, it's hard to<br>anticipate the requirements without seeing it hooked up.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Is there already a patch in queue that makes use of this?</p><p><a href="https://review.coreboot.org/c/coreboot/+/30182">View Change</a></p><p>14 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/Kconfig">File src/drivers/intel/fb/Kconfig:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/Kconfig@3">Patch Set #1, Line 3:</a> <code style="font-family:monospace,monospace">    depends on ARCH_X86</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The current implementation depends on PLATFORM_USES_FSP2_0 and<br>RUN_FSP_GOP. Though, a generic implementation should only depend<br>on LINEAR_FRAMEBUFFER.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c">File src/drivers/intel/fb/fbdriver.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@28">Patch Set #1, Line 28:</a> <code style="font-family:monospace,monospace">#define FI fb_info</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This seems unnecessary, `fb_info` should already be short enough?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@29">Patch Set #1, Line 29:</a> <code style="font-family:monospace,monospace">unsigned long</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Use `uintptr_t` instead of `unsigned long`. There's no guarantee<br>that the latter has the same width as a pointer.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@29">Patch Set #1, Line 29:</a> <code style="font-family:monospace,monospace"> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">no space, please</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@41">Patch Set #1, Line 41:</a> <code style="font-family:monospace,monospace">0xFF</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It seems more common in coreboot to write hex-digits in lower case,<br>i.e. `0xff`. In any case, please make sure to use one style within<br>this file.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Hmmm, also why not make it an explicit `unsigned char`?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@42">Patch Set #1, Line 42:</a> <code style="font-family:monospace,monospace">/</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please put spaces around the /'s</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@51">Patch Set #1, Line 51:</a> <code style="font-family:monospace,monospace"> pci_write_config16(dev1, 0x04, 0x07);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">There are definitions for these registers/bits in `device/pci_def.h`.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think, it's legal to activate IO and MMIO here, and also<br>bus mastering should be taken care of by the device driver.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@53">Patch Set #1, Line 53:</a> <code style="font-family:monospace,monospace">  status = fsp_fill_lb_framebuffer (&fb_info);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">space prohibited between function name and open parenthesis '('</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">If you'd use fill_lb_framebuffer() instead, this driver could work<br>with all framebuffers in coreboot.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Please always check the return value.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@54">Patch Set #1, Line 54:</a> <code style="font-family:monospace,monospace">   fb_info.physical_address = pci_read_config32(dev1, 0x18);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Shouldn't be necessary if you use fill_lb_framebuffer() proper.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@60">Patch Set #1, Line 60:</a> <code style="font-family:monospace,monospace">x_resolution</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">bytes_per_line</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@61">Patch Set #1, Line 61:</a> <code style="font-family:monospace,monospace">(void *)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Cast to (void *) shouldn't be necessary.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@73">Patch Set #1, Line 73:</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;">     fb_inverse = 0;<br>       previous_write = 0;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@79">Patch Set #1, Line 79:</a> <code style="font-family:monospace,monospace"> = 0</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">unnecessary initialization</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@101">Patch Set #1, Line 101:</a> <code style="font-family:monospace,monospace">             for(x = font_width - 1; x >= 0; x--) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">space required before the open parenthesis '('</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">why the reverse loop (from font_width down to 0)?</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/c/coreboot/+/30182">change 30182</a>. To unsubscribe, or for help writing mail filters, 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/c/coreboot/+/30182"/><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-Change-Id: I70715d9c84dae2d0efa1631649ce27ea3c39e237 </div>
<div style="display:none"> Gerrit-Change-Number: 30182 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Maulik V Vaghela <maulik.v.vaghela@intel.com> </div>
<div style="display:none"> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan@intel.com> </div>
<div style="display:none"> Gerrit-Reviewer: Martin Roth <martinroth@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela@intel.com> </div>
<div style="display:none"> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> </div>
<div style="display:none"> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi@intel.com> </div>
<div style="display:none"> Gerrit-Reviewer: Subrata Banik <subrata.banik@intel.com> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-CC: Nico Huber <nico.h@gmx.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 12 Dec 2018 12:22:13 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>