[XL] Change in coreboot[main]: lib/device_tree: Add some FDT helper functions

[Julius Werner (Code Review)]

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 18:

(8 comments)

File src/include/device_tree.h:

https://review.coreboot.org/c/coreboot/+/81081/comment/c20e785d_52a59bf7 :
PS18, Line 203: 
nit: Please add at least a line of documentation, particularly to clarify here what the return value is.

File src/lib/device_tree.c:

https://review.coreboot.org/c/coreboot/+/81081/comment/3890d52e_cafa86d8 :
PS12, Line 200: 		if (!strncmp(*path, node_name, path_sub_len)) {
> I changed it now to use a 'path_array' instead of the path directly (although personally I find the  […]

Ack, that works too. (I think the code should get quite a bit simpler still with some of my other suggestions, e.g. pull `find_next_node_name()` into the loop condition and reduce the number of arguments.)

https://review.coreboot.org/c/coreboot/+/81081/comment/97f6c220_a7cc00b8 :
PS12, Line 214: ince address-cells and size-cells are not inherited
> I am not sure what to do now. According to the issue, we should not treat them as inheritable.

I mean sounds like they're acknowledging that Linux is intentionally doing something else to improve compatibility. I would say in that case following Linux would make it more likely that our code works with stuff than following the spec. People who generate device trees test against implementations, not against an abstract spec.

Either way, I think it's important that this library is internally consistent. All the non-flattened APIs (e.g. `dt_find_node()` currently do what Linux does. You should either match that behavior or change all those function to follow the spec.

File src/lib/device_tree.c:

https://review.coreboot.org/c/coreboot/+/81081/comment/1396a001_0d9f13c0 :
PS18, Line 175:  ""
I don't really understand the `""` here at the start, what does that represent? There shouldn't be an extra string for the root node, that's where you start after all (i.e. where node_offset already is). If someone does `fdt_find_node_by_path("/")`, that should get translated into `fdt_find_node(blob, root_node_offset, { /* empty array */ }, 0, 0, ...)` (or, with my suggestion below, `fdt_find_node(blob, root_node_offset, { NULL }, ...)`).

https://review.coreboot.org/c/coreboot/+/81081/comment/f583e281_d1571979 :
PS18, Line 188: size_t path_count
I think you should match `dt_find_node()` with this API for consistency (e.g. no `path_index`, no `path_count` and instead terminate the path array with `NULL`).

https://review.coreboot.org/c/coreboot/+/81081/comment/b52b9b93_88fb4caa :
PS18, Line 251: #define PATH_ARRAY_MAX 10
I think this should go at the top of the file (maybe call it FDT_PATH_MAX_DEPTH to clarify that it doesn't count for the unflattened DT functions).

https://review.coreboot.org/c/coreboot/+/81081/comment/697322b7_56f00672 :
PS18, Line 255: 128
This should also be a named constant (e.g. FDT_PATH_MAX_LEN).

https://review.coreboot.org/c/coreboot/+/81081/comment/36643a45_f6a4aae6 :
PS18, Line 265: 	}
I think this would be cleaner with `memcpy()` and `strtok_r()`:
```
char path_copy[128];
memcpy(path_copy, path + 1, path_size);
char *cur = path_copy;
for (int i = 0; i < PATH_ARRAY_MAX; i++) {
  path_array[i] = strtok_r(NULL, '/', &cur);
  if (!path_array[i])
    break;
}
assert(i < PATH_ARRAY_MAX);
```

That way you also get the `NULL` terminator for the path array right away (for my suggestion on how to change the `fdt_find_node()` signature), and I think `strtok_r()` automatically ignores the trailing slash.