| Message ID | 1450355367-14818-6-git-send-email-eric.auger@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 17 December 2015 at 12:29, Eric Auger <eric.auger@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.auger@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. > + > +/** > + * 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?) > + 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). > + 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. > + 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
On 12/18/2015 03:54 PM, Peter Maydell wrote: > On 17 December 2015 at 12:29, Eric Auger <eric.auger@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.auger@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 >
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; + +/** + * 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); + 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]; + 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; + } + 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 */ /**
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.auger@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(+) -- 1.9.1