<p><a href="https://review.coreboot.org/c/coreboot/+/30225">View Change</a></p><p>7 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/30225/1//COMMIT_MSG">Commit Message:</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/30225/1//COMMIT_MSG@14">Patch Set #1, Line 14:</a> <code style="font-family:monospace,monospace">second time). This might save up to 100 milliseconds (exact value TBD).</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I would focus on the `exact value`. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">We are using a bayhub eMMC which will not accept any command until a software reset (bit 0 of register offset 2fh) is set and cleared. It clears by itself once the reset is completed, and that's what takes a long time. We are reducing the total boot time by making it happen at coreboot time, so that when payload takes over, it's either completed or about to complete (exact situation passed through cbmem) and the payload (in this board depthcharge) don't need to send it again. It either continues after reset is completed (ideal situation) or just waits its completion (still saving time). The exact value depends on how long it takes between the fist command and payload execution.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/30225/1/src/drivers/emmc/emmc_ram.c">File src/drivers/emmc/emmc_ram.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/30225/1/src/drivers/emmc/emmc_ram.c@28">Patch Set #1, Line 28:</a> <code style="font-family:monospace,monospace">            return EMMC_STATUS_CBMEM_FAILED;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">what about `pinfo->identifier`?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">See the comment where it's defined. It's a way for other eMMC that needs a different sequence to tell who they are. In theory, payload has to read the identifier before interpreting any byte(s) following it.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30225/1/src/drivers/emmc/emmc_ram.c@30">Patch Set #1, Line 30:</a> <code style="font-family:monospace,monospace">               return EMMC_STATUS_INVALID_ADDR;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Please move parameter checks to the top, it can be decided immediately.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">will do.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30225/1/src/drivers/emmc/emmc_ram.c@34">Patch Set #1, Line 34:</a> <code style="font-family:monospace,monospace"> byte = *reset;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">use read8() / write8()</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">will do.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/30225/1/src/drivers/emmc/emmc_rom.c">File src/drivers/emmc/emmc_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/30225/1/src/drivers/emmc/emmc_rom.c@40">Patch Set #1, Line 40:</a> <code style="font-family:monospace,monospace">     byte = 0; /* preclear */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">sorry, you might have already noticed, I'm allergic to comments (especially those that cover broken  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I just used byte above with bit 0 set. If reads fail for any reason and there's no actual value loaded into byte, bit 0 will be already set, thus mascaraing a failure situation. By clearing bit 0 first than checking if it's set, I guarantee a read was executed.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/30225/1/src/include/device/emmc.h">File src/include/device/emmc.h:</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/30225/1/src/include/device/emmc.h@23">Patch Set #1, Line 23:</a> <code style="font-family:monospace,monospace">#define EMMC_RESET_COMPLETED                4</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Given that the consumer has to expect outdated information, what information does this actually carr […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">If cbmem for reset status is not found or is status is EMMC_SEND_RESET, something went wrong and payload has to execute full reset.<br>if status = EMMC_CONFIRMED_RESET_ACCEPTED, then reset has not been checked for complition, and payload should act just as in the case of EMMC_RESET_STILL_ACTIVE.<br>If status = EMMC_RESET_STILL_ACTIVE, payload has to wait bit 0 of register 2fh to become clear before continuing code.<br>If status = EMMC_RESET_COMPLETED, payload continues from its code after the call to reset the eMMC.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/30225/1/src/include/device/emmc.h@51">Patch Set #1, Line 51:</a> <code style="font-family:monospace,monospace"> */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I still don't get it. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">What I'm saying is that other code can use the same cbmem id. As you mentioned, there's an Intel eMMC where apparently something else needs to be done (though apparently depthcharge knows nothing about it, as I copy depthcharge reset code for eMMC). So I'm creating an opportunity for payload to know it has something else happening. There's a single cbmem id (so payload can easely immediately identify if there was an early attempt to reset the eMMC, and then by checking this variable decide how it has to act.</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/c/coreboot/+/30225">change 30225</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/+/30225"/><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: I4bb55fde23e3b70da1cca76e5672e880317b8bd2 </div>
<div style="display:none"> Gerrit-Change-Number: 30225 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </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 14:56:42 +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: Nico Huber <nico.h@gmx.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>