Attention is currently required from: Alper Nebi Yasak.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80322?usp=email )
Change subject: device_tree: Add function to get top of memory from a FDT blob ......................................................................
Patch Set 3: Code-Review+1
(6 comments)
Patchset:
PS3: Thanks for the effort and the elaborate commit message!
Look pretty good to me. Though, I would extract a few common functions like fast-forwarding to a specific node/property.
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/80322/comment/718f6b9b_8b6d2708 : PS3, Line 188: int addr_cells = 0; Please use `unsigned` so we don't have to think about potential overflows.
https://review.coreboot.org/c/coreboot/+/80322/comment/7a0259d6_ba745c36 : PS3, Line 191: if (!strncmp(prop.name, "#address-cells", sizeof("#address-cells"))) Why the str*n*cmp()? Isn't "#address_cells" the whole string we should match?
https://review.coreboot.org/c/coreboot/+/80322/comment/d9dca217_7d7d4a59 : PS3, Line 192: addr_cells = be32toh(*(uint32_t *)prop.data); read_be32() would be easier to use here. <commonlib/endian.h>
https://review.coreboot.org/c/coreboot/+/80322/comment/a3471f23_9f900247 : PS3, Line 229: return 0; Should we also check that `reg_size >= (addr_cells + size_cells) * sizeof(uint32_t)`?
https://review.coreboot.org/c/coreboot/+/80322/comment/4346e473_74575c68 : PS3, Line 253: } It's not wrong to write it as a loop, but the code would become much clearer if we'd just handle the two cases `size_cells == 1` and `size_cells == 2`.
Also, the logic could be extracted into a common function, so this could become e.g. ``` static uint64_t read_reg_value(const uint32_t *reg_data, unsigned int num_cells);
mem_base = read_reg_value(reg_data, addr_cells); mem_size = read_reg_value(reg_data + addr_cells, size_cells); ```