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
-
Julius Werner (Code Review)