<p style="white-space: pre-wrap; word-wrap: break-word;">I have exercised a simple number example on PS5 to show you the align-down problem. It's assuming two memory resources, the first 4KiB, the second 8KiB in size.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Please don't put further effort into the implementation at this time, until we have discussed the API.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Your use case is quite simple and I think most are. You just want to access a single memory resource. All you need from the caller is to tell what resource, mapped where. But the API you provide here is much more flexible and obviously results in error-prone implementations. So please, trim this down to what you actually need. If we ever need more flexibility, we can still implement that later.</p><p><a href="https://review.coreboot.org/c/coreboot/+/30215">View Change</a></p><p>10 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@80">Patch Set #4, Line 80:</a> <code style="font-family:monospace,monospace"> else</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't know the exact answer to your question. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Which PCI enumeration? coreboot's?</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> git grep PCI_COMMAND_MASTER -- src/device/</pre><p style="white-space: pre-wrap; word-wrap: break-word;">shows only two cases: IORESOURCE_PCI_BRIDGE and PCI_BASE_CLASS_SYSTEM.<br>I guess the first case is only a shortcut so you don't have to walk<br>the path up to set it everywhere in case you need to set it for a<br>downstream device (so that upstream requests can be forwarded all<br>the way to the host bridge).</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/30215/5/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/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@21">Patch Set #5, Line 21:</a> <code style="font-family:monospace,monospace"> uint32_t *base, uint32_t start, uint32_t *size)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">1. start = 0xf4000000<br>2. start = 0xf4001000</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@28">Patch Set #5, Line 28:</a> <code style="font-family:monospace,monospace"> value = pci_read_config32(dev, TEMP_BASE(number));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">1. resource (4KiB): value = 0xfffff000<br>2. resource (8KiB): value = 0xffffe000</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@33">Patch Set #5, Line 33:</a> <code style="font-family:monospace,monospace"> value &= 0xfffffc00; /* At least 1K between addresses */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">1. value = 0xfffff000<br>2. value = 0xffffe000</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@34">Patch Set #5, Line 34:</a> <code style="font-family:monospace,monospace"> *size = 1 + ~value;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">1. size = 0x1000<br>2. size = 0x2000</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@35">Patch Set #5, Line 35:</a> <code style="font-family:monospace,monospace"> value &= start;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">1. value = 0xf4000000<br>2. value = 0xffffe000 & 0xf4001000 = 0xf4000000 oops</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@36">Patch Set #5, Line 36:</a> <code style="font-family:monospace,monospace"> *base = value;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">1. base = 0xf4000000</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@76">Patch Set #5, Line 76:</a> <code style="font-family:monospace,monospace"> next_mem = TP_DEFAULT_MEM_START;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">next_mem = 0xf4000000</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@87">Patch Set #5, Line 87:</a> <code style="font-family:monospace,monospace"> case MEMORY_ADDRESS:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">1. next_mem = 0xf4000000<br>2. next_mem = 0xf4001000</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@93">Patch Set #5, Line 93:</a> <code style="font-family:monospace,monospace"> next_mem = (return_base + return_size);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">1. next_mem = 0xf4001000</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: 5 </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: Tue, 18 Dec 2018 15:53:43 +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>