<p style="white-space: pre-wrap; word-wrap: break-word;">How (if) does this relate to `Work in progress: Early eMMC phase 1`?</p><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 style="white-space: pre-wrap; word-wrap: break-word;">I would focus on the `exact value`. There was this weird eMMC on Intel hardware (that's why there is commonlib/storage) that needed much more care (continuous polling, iirc).</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 style="white-space: pre-wrap; word-wrap: break-word;">what about `pinfo->identifier`?</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 style="white-space: pre-wrap; word-wrap: break-word;">Please move parameter checks to the top, it can be decided immediately.</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 style="white-space: pre-wrap; word-wrap: break-word;">use read8() / write8()</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 style="white-space: pre-wrap; word-wrap: break-word;">sorry, you might have already noticed, I'm allergic to comments (especially those that cover broken code up).</p><p style="white-space: pre-wrap; word-wrap: break-word;">what is this line supposed to do in the long run?</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 style="white-space: pre-wrap; word-wrap: break-word;">Given that the consumer has to expect outdated information, what information does this actually carry?</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 style="white-space: pre-wrap; word-wrap: break-word;">I still don't get it. How is this different from the CBMEM id? It's hard to see what you are up to as you didn't use it in your own code.</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: 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: Sat, 15 Dec 2018 10:59:43 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>