[coreboot-gerrit] Change in coreboot[master]: lib/fit_payload: Add coreboot tables support for FDT.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/28104 )

Change subject: lib/fit_payload: Add coreboot tables support for FDT.
......................................................................


Patch Set 8:

(6 comments)

https://review.coreboot.org/#/c/28104/8/src/lib/device_tree.c
File src/lib/device_tree.c:

https://review.coreboot.org/#/c/28104/8/src/lib/device_tree.c@865
PS8, Line 865: (void *)
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.


https://review.coreboot.org/#/c/28104/7/src/lib/fit_payload.c
File src/lib/fit_payload.c:

https://review.coreboot.org/#/c/28104/7/src/lib/fit_payload.c@153
PS7, Line 153: payload.*
> Why userspace ? I'd use the term payload here
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.


https://review.coreboot.org/#/c/28104/7/src/lib/fit_payload.c@202
PS7, Line 202: 
> Should be after dt_apply_fixups(), as it could remove and regenerate the whole devicetree.
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.

Anyway, shouldn't make a difference where it runs.


https://review.coreboot.org/#/c/28104/8/src/lib/fit_payload.c
File src/lib/fit_payload.c:

https://review.coreboot.org/#/c/28104/8/src/lib/fit_payload.c@139
PS8, Line 139: 	includes the coreboot table). */
nit: Does this fit on one line?


https://review.coreboot.org/#/c/28104/8/src/lib/fit_payload.c@140
PS8, Line 140: cbmem_get_imd
I'm confused... how does this build? cbmem_get_imd() is a static inline in another .c file.

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.

(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.)


https://review.coreboot.org/#/c/28104/8/src/lib/fit_payload.c@153
PS8, Line 153: 	 payload.*/
nit: Doesn't need to wrap



-- 
To view, visit https://review.coreboot.org/28104
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib714a021a24f51407558f484cd97aa58ecd43977
Gerrit-Change-Number: 28104
Gerrit-PatchSet: 8
Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Patrick Rudolph <siro at das-labor.org>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Julius Werner <jwerner at chromium.org>
Gerrit-Comment-Date: Thu, 16 Aug 2018 21:02:42 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180816/d4853f19/attachment.html>