<p><a href="https://review.coreboot.org/c/coreboot/+/30215">View Change</a></p><p>4 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/30215/4/src/drivers/generic/temp_pcie_bridge/bridge_rom.c">File src/drivers/generic/temp_pcie_bridge/bridge_rom.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/30215/4/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@28">Patch Set #4, Line 28:</a> <code style="font-family:monospace,monospace"> outl(reg_2_cf8(dev, reg), 0xCF8);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">> Existing access functions assume bus 0. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I could not make it work. I can try again, but so far I could not make pci_read_configx work on a non-zero bus. Also, at ramstage it assumes that we are pointing to a structure which contains the information about the exact PCI... That's not the case for any device behind a bridge (that I'm aware of), as the exact bus is dependent on the exact PCI tree. Add/remove a card, or change the bridge being used for the device and bus will change.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30215/4/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@66">Patch Set #4, Line 66:</a> <code style="font-family:monospace,monospace">             value &= start;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">> It does not matter. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">If you follow the code, it'll never go to an address bellow next_mem (except if you assign a range that's not aligned) The code will actually force holes to align any other BAR to the next 1K above the last used by simply forcing any size to be at least 1K. If the actual required size is less then 1K, it'll create a hole between 2 BAR.</p><p style="white-space: pre-wrap; word-wrap: break-word;">It does not matter - because this happens before PCI enumeration, used briefly, and destroyed before the enumeration happens.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30215/4/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@79">Patch Set #4, Line 79:</a> <code style="font-family:monospace,monospace">    pci_write32(dev, TEMP_BASE(number), value);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Which code? I haven't seen it.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I see, the bits I thought I was forcing to 0 are actually read only. The code works because on my particular bridge the high address is reset to 0... but there's no warranty that all bridges will follow. Will force the high address to 0.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30215/4/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@80">Patch Set #4, Line 80:</a> <code style="font-family:monospace,monospace">    command |= PCI_COMMAND_MASTER;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Please elaborate, I'm not used to it. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't know the exact answer to your question. What I do know is that PCI enumeration always set this bit on any bridge/device that has at least one BAR (memory or IO) being used.</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/c/coreboot/+/30215">change 30215</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/+/30215"/><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: Iaec1fcc4f8db8c3b4cfd3786d3ff589dc9cb22f5 </div>
<div style="display:none"> Gerrit-Change-Number: 30215 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Richard Spiegel <richard.spiegel@silverbackltd.com> </div>
<div style="display:none"> Gerrit-Reviewer: Daniel Kurtz <djkurtz@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Martin Roth <martinroth@google.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: Raul Rangel <rrangel@chromium.org> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Spiegel <richard.spiegel@silverbackltd.com> </div>
<div style="display:none"> Gerrit-Reviewer: Simon Glass <sjg@chromium.org> </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: Mon, 17 Dec 2018 15:26:44 +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: Richard Spiegel <richard.spiegel@silverbackltd.com> </div>
<div style="display:none"> Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>