<p><a href="https://review.coreboot.org/28104">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/28104/2/src/lib/fit_payload.c">File src/lib/fit_payload.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/28104/2/src/lib/fit_payload.c@112">Patch Set #2, Line 112:</a> <code style="font-family:monospace,monospace">        // Need to add 'ranges' to the intermediate node to make 'reg' work.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Remember to convert everything to coreboot comment style.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/28104/2/src/lib/fit_payload.c@119">Patch Set #2, Line 119:</a> <code style="font-family:monospace,monospace">(char *)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Do you need this cast? Looks like we didn't need it in depthcharge so I'm confused...</p><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe we should change all the dt_add_..._prop() stuff to take const values?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/28104/2/src/lib/fit_payload.c@138">Patch Set #2, Line 138:</a> <code style="font-family:monospace,monospace">       reg_sizes[1] = cbmem_overhead_size();</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">These aren't the right numbers... at least not the numbers used by depthcharge. We don't actually have any uses for this field yet, so it's not that big of a deal, but we should try to keep it consistent.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The correct range (that depthcharge uses) is found by cbmem_add_bootmem(). Maybe we should expand that function to a slightly more generic cbmem_full_area(&start, &size) or something like that, so that we can use it both for bootmem and here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/28104/2/src/lib/fit_payload.c@143">Patch Set #2, Line 143:</a> <code style="font-family:monospace,monospace">exported from coreboot</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">nit: not really "exported from coreboot" anymore now, since we're still in coreboot</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/28104">change 28104</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/28104"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ib714a021a24f51407558f484cd97aa58ecd43977 </div>
<div style="display:none"> Gerrit-Change-Number: 28104 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-CC: Julius Werner <jwerner@chromium.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 15 Aug 2018 23:11:24 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>