<p><a href="https://review.coreboot.org/28104">View Change</a></p><p>6 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/28104/8/src/lib/device_tree.c">File src/lib/device_tree.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/8/src/lib/device_tree.c@865">Patch Set #8, Line 865:</a> <code style="font-family:monospace,monospace">(void *)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Well, this just moves the problem around. I meant that we should make the value arguments of *all* the dt_add_xxx_prop() functions const (I guess you really only need to change dt_add_bin_prop(), the others can convert implicitly from non-const to const). And (struct device_tree_prop).prop.data would then also need to become a pointer to const. Then, I think, this should be possible without any casts.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/28104/7/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/7/src/lib/fit_payload.c@153">Patch Set #7, Line 153:</a> <code style="font-family:monospace,monospace">payload.*</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why userspace ? I'd use the term payload here</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">These values are currently not read by any kernel driver, they're just there for consumption through /proc/device-tree. Of course, that may change, so "expose ... to the OS" or something might be more accurate.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/28104/7/src/lib/fit_payload.c@202">Patch Set #7, Line 202:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should be after dt_apply_fixups(), as it could remove and regenerate the whole devicetree.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">That's... not really the idea of fixups, though, they're just supposed to be for minor additions or changes. In depthcharge, this *is* a fixup, and execution order between different fixups is random.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Anyway, shouldn't make a difference where it runs.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/28104/8/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/8/src/lib/fit_payload.c@139">Patch Set #8, Line 139:</a> <code style="font-family:monospace,monospace">      includes the coreboot table). */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">nit: Does this fit on one line?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/28104/8/src/lib/fit_payload.c@140">Patch Set #8, Line 140:</a> <code style="font-family:monospace,monospace">cbmem_get_imd</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm confused... how does this build? cbmem_get_imd() is a static inline in another .c file.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Actually, is it possible that Jenkins doesn't test any of this code because none of our platforms enables CONFIG_PAYLOAD_FIT_SUPPORT? It would be great if you guys could select that for the Cavium board so that we have test coverage for this code.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(Please also test that this gives you the right numbers. You can dump this via /proc/device-tree/firmware/coreboot/reg, and it should give you big-endian numbers that match the CBMEM ranges coreboot prints during boot.)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/28104/8/src/lib/fit_payload.c@153">Patch Set #8, Line 153:</a> <code style="font-family:monospace,monospace">  payload.*/</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">nit: Doesn't need to wrap</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: 8 </div>
<div style="display:none"> Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> </div>
<div style="display:none"> Gerrit-Reviewer: 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: Thu, 16 Aug 2018 21:02:42 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>