On 12/18/2015 03:54 PM, Peter Maydell wrote: > On 17 December 2015 at 12:29, Eric Auger <eric.au...@linaro.org> wrote: >> Some passthrough'ed devices depend on clock nodes. Those need to be >> generated in the guest device tree. This patch introduces some helpers >> to build a clock node from information retrieved in the host device tree. >> >> - inherit_properties copies properties from a host device tree node to >> a guest device tree node >> - fdt_build_clock_node builds a guest clock node and checks the host >> fellow clock is a fixed one. >> >> fdt_build_clock_node will become static as soon as it gets used. A >> dummy pre-declaration is needed for compilation of this patch. >> >> Signed-off-by: Eric Auger <eric.au...@linaro.org> >> >> --- >> >> RFC -> v1: >> - use the new proto of qemu_fdt_getprop >> - remove newline in error_report >> - fix some style issues >> --- >> hw/arm/sysbus-fdt.c | 111 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 111 insertions(+) >> >> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c >> index 9d28797..c2d813b 100644 >> --- a/hw/arm/sysbus-fdt.c >> +++ b/hw/arm/sysbus-fdt.c >> @@ -21,6 +21,7 @@ >> * >> */ >> >> +#include <libfdt.h> >> #include "hw/arm/sysbus-fdt.h" >> #include "qemu/error-report.h" >> #include "sysemu/device_tree.h" >> @@ -56,6 +57,116 @@ typedef struct NodeCreationPair { >> int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque); >> } NodeCreationPair; >> >> +/* helpers */ >> + >> +struct HostProperty { >> + const char *name; >> + bool optional; >> +}; >> + >> +typedef struct HostProperty HostProperty; > > You can combine the typedef and the struct declaration if you like.
ok > >> + >> +/** >> + * inherit_properties >> + * >> + * copies properties listed in an array from host device tree to >> + * guest device tree. If a non optional property is not found, the >> + * function self-asserts. An optional property is ignored if not found >> + * in the host device tree. >> + * @props: array of HostProperty to copy >> + * @nb_props: number of properties in the array >> + * @host_dt: host device tree blob >> + * @guest_dt: guest device tree blob >> + * @node_path: host dt node path where the property is supposed to be >> + found >> + * @nodename: guest node name the properties should be added to >> + */ >> +static void inherit_properties(HostProperty *props, int nb_props, >> + void *host_fdt, void *guest_fdt, >> + char *node_path, char *nodename) >> +{ >> + int i, prop_len; >> + const void *r; >> + >> + for (i = 0; i < nb_props; i++) { >> + r = qemu_fdt_getprop(host_fdt, node_path, >> + props[i].name, >> + &prop_len, >> + props[i].optional ? NULL : &error_fatal); > > We'll get an error here if the host device tree doesn't match > up correctly, right? Is the error message going to be sufficiently > informative to the end user about what's gone wrong? (What does > it end up looking like?) hum you're right, in case of a mandated property there is auto-assert with a good error message. In case of an optional property, there is currently no message output to the end-user and the host property is ignored/not propagated to the guest, with potential functional consequences. I intend to use an error object instead and print the error message in case the error is != FDT_ERR_NOTFOUND. > >> + if (r) { >> + qemu_fdt_setprop(guest_fdt, nodename, >> + props[i].name, r, prop_len); >> + } >> + } >> +} >> + >> +/* clock properties whose values are copied/pasted from host */ >> +static HostProperty clock_inherited_properties[] = { >> + {"compatible", 0}, >> + {"#clock-cells", 0}, >> + {"clock-frequency", 1}, >> + {"clock-output-names", 1}, >> +}; >> + >> +/** >> + * fdt_build_clock_node >> + * >> + * Build a guest clock node, used as a dependency from a passthrough'ed >> + * device. Most information are retrieved from the host clock node. >> + * Also check the host clock is a fixed one. >> + * >> + * @host_fdt: host device tree blob from which info are retrieved >> + * @guest_fdt: guest device tree blob where the clock node is added >> + * @host_phandle: phandle of the clock in host device tree >> + * @guest_phandle: phandle to assign to the guest node >> + */ >> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt, >> + uint32_t host_phandle, >> + uint32_t guest_phandle); >> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt, >> + uint32_t host_phandle, >> + uint32_t guest_phandle) >> +{ >> + char node_path[256]; > > Please don't use hardcoded fixed buffer lengths (see previous patch > review comments). OK > >> + char *nodename; >> + const void *r; >> + int ret, prop_len; >> + >> + ret = fdt_node_offset_by_phandle(host_fdt, host_phandle); >> + if (ret <= 0) { >> + error_report("not able to locate clock handle %d in host device >> tree", >> + host_phandle); >> + goto out; >> + } > > If we're going to return to the caller in the error case rather than just > exiting here, I think we should return the error to the caller via an > Error** rather than calling error_report() directly ourselves. agreed Thanks! Eric > >> + ret = fdt_get_path(host_fdt, ret, node_path, 256); >> + if (ret < 0) { >> + error_report("not able to retrieve node path for clock handle %d", >> + host_phandle); >> + goto out; >> + } >> + >> + r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len, >> + &error_fatal); >> + if (strcmp(r, "fixed-clock")) { >> + error_report("clock handle %d is not a fixed clock", host_phandle); >> + ret = -1; >> + goto out; >> + } >> + >> + nodename = strrchr(node_path, '/'); >> + qemu_fdt_add_subnode(guest_fdt, nodename); >> + >> + inherit_properties(clock_inherited_properties, >> + ARRAY_SIZE(clock_inherited_properties), >> + host_fdt, guest_fdt, >> + node_path, nodename); >> + >> + qemu_fdt_setprop_cell(guest_fdt, nodename, "phandle", guest_phandle); >> + >> +out: >> + return ret; >> +} >> + >> /* Device Specific Code */ > > thanks > -- PMM >