Attention is currently required from: Jakub Czapiga, Martin L Roth, Maximilian Brune.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81081?usp=email )
Change subject: lib/device_tree: Add some FDT helper functions ......................................................................
Patch Set 7:
(15 comments)
Patchset:
PS7: I'm not really convinced about the wildcard API tbh, that is way more complicated than what we have on the unflattened side. Hopefully we can find a simpler solution for that, but I'd need to know some more about your use cases first.
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/f4198bb6_93ef33d5 : PS6, Line 27: if (be32toh(ptr[index++]) != FDT_TOKEN_PROPERTY) BTW, upon reading the device tree spec I noticed that in all of these parsing cases we don't take into account that there may be NOP tokens (0x4) mixed in between any other tokens. This code was originally written for ChromeOS where we know we're always only reading device trees fresh out of `dtc` that won't have those, but if you want to apply it more broadly you may want to add support for that.
https://review.coreboot.org/c/coreboot/+/81081/comment/dea56b21_1109943f : PS6, Line 53: (char *) This cast becomes unnecessary then too.
https://review.coreboot.org/c/coreboot/+/81081/comment/cde4e199_b7517a04 : PS6, Line 81: fdt_read_prop_in_node nit: Would be nice to have some naming consistency between the functions that all read properties, e.g. ``` fdt_read_property() fdt_read_reg_property() fdt_read_alias_property() ``` or ``` fdt_read_prop() fdt_read_reg_prop() fdt_read_alias_prop() ```
https://review.coreboot.org/c/coreboot/+/81081/comment/00e0a469_2ff8ee27 : PS6, Line 99: uintptr_t *addrs, uintptr_t *sizes I feel like it might be cleaner to create a small struct that holds an addr and a size, and then have an array of those passed in here? Then there's less disconnect between which addr belongs to which size.
Also you probably want to use uint64_t for those because on some systems physical address space is larger what the processor can logically address in one register.
https://review.coreboot.org/c/coreboot/+/81081/comment/3c5cd48d_9c12e49a : PS6, Line 105: printk(BIOS_DEBUG, "no reg property found\n"); nit: maybe print node_offset for all these to provide some context?
https://review.coreboot.org/c/coreboot/+/81081/comment/89238fd0_ca96a28b : PS6, Line 118: //TODO replace with something more computational Something like ``` uint8_t *ptr = prop.data; ... if (addr_cells > 2) ...error out... addrs[i] = 0; for (int j = 0; j < addr_cells * 4; j++) addrs[i] = (addrs[i] << 8) | *ptr++; ```
https://review.coreboot.org/c/coreboot/+/81081/comment/f6c07421_e19e917d : PS6, Line 142: are are what?
https://review.coreboot.org/c/coreboot/+/81081/comment/b7a2db6e_eaea82a0 : PS6, Line 157: wildcard_idx = path_sub_len; Wildcard globbing in a firmware API feels pretty out of place. Can you show an example where you actually need that?
https://review.coreboot.org/c/coreboot/+/81081/comment/3553d95a_d42ba4ad : PS6, Line 227: //TODO more sanity checks Let's not leave TODOs in the code we check in. If you think something more needs to happen here, you should implement it now. (In general I think we should keep all the sanity checking for the FDT blob in `fdt_is_valid()`, and then only call that once per boot, not on every access.)
https://review.coreboot.org/c/coreboot/+/81081/comment/55f5c2c4_70ec8cdc : PS6, Line 244: * @param create 1: Create node(s) if not found. 0: Return NULL instead. Documentation doesn't match the function.
https://review.coreboot.org/c/coreboot/+/81081/comment/c1e2a2b1_4ea83a18 : PS6, Line 278: static uintptr_t fdt_read_alias_prop(const void *blob, const char *alias_name, Why not just return a `const char *` to the resulting path directly? That should normally be the only thing you care to look up in an alias.
https://review.coreboot.org/c/coreboot/+/81081/comment/a638851d_3a544cc6 : PS6, Line 303: alias_prop.name I'm confused, isn't `alias_prop.name` the same as `alias_name`? Aren't you actually interested in `alias_prop.data`?
https://review.coreboot.org/c/coreboot/+/81081/comment/374674de_c936c128 : PS6, Line 501: printk(BIOS_ERR, "%s failed\n", __func__); nit: not really necessary to print this if the callee already printed info. (If you're concerned about the right error levels, I think it would be more useful to just bump the messages inside `fdt_is_valid()` to warning or error.)
File tests/lib/device_tree-test.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/0c3a431e_d1b2d421 : PS7, Line 13: // regular FDT from RISC-V QEMU virt target Note that you can also just have the test executable read in a binary FDT file, that's a bit cleaner. See lib/lzma-test for an example (we should probably factor out that functionality into a common test helper somehow).