| Message ID | 1453130204-655-7-git-send-email-eric.auger@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 18 January 2016 at 15:16, 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. > > - copy_properties_from_host 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> > + > +/** > + * copy_properties_from_host > + * > + * 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 This is just "asserts", not "self-asserts". > + * 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 > + */ > +/** > + * 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 > + */ > +void fdt_build_clock_node(void *host_fdt, void *guest_fdt, > + uint32_t host_phandle, > + uint32_t guest_phandle); > +void fdt_build_clock_node(void *host_fdt, void *guest_fdt, > + uint32_t host_phandle, > + uint32_t guest_phandle) > +{ > + char *node_path = NULL; > + char *nodename; > + const void *r; > + int ret, node_offset, prop_len, path_len = 16; > + > + node_offset = fdt_node_offset_by_phandle(host_fdt, host_phandle); > + if (node_offset <= 0) { > + error_setg(&error_fatal, > + "not able to locate clock handle %d in host device tree", > + host_phandle); Don't we want to return here, rather than continuing with the rest of the function? ("goto err;" with an err: label before the g_free() at the bottom of the function probably best since some of the error paths need to free that.) > + } > + node_path = g_malloc(path_len); > + while ((ret = fdt_get_path(host_fdt, node_offset, node_path, path_len)) > + == -FDT_ERR_NOSPACE) { > + path_len += 16; > + node_path = g_realloc(node_path, path_len); > + } > + if (ret < 0) { > + error_setg(&error_fatal, > + "not able to retrieve node path for clock handle %d", > + host_phandle); > + } > + > + r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len, > + &error_fatal); > + if (strcmp(r, "fixed-clock")) { > + error_setg(&error_fatal, > + "clock handle %d is not a fixed clock", host_phandle); > + } > + > + nodename = strrchr(node_path, '/'); > + qemu_fdt_add_subnode(guest_fdt, nodename); > + > + copy_properties_from_host(clock_copied_properties, > + ARRAY_SIZE(clock_copied_properties), > + host_fdt, guest_fdt, > + node_path, nodename); > + > + qemu_fdt_setprop_cell(guest_fdt, nodename, "phandle", guest_phandle); > + > + g_free(node_path); > +} > + > /* Device Specific Code */ thanks -- PMM
Hi Peter, On 01/25/2016 03:05 PM, Peter Maydell wrote: > On 18 January 2016 at 15:16, 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. >> >> - copy_properties_from_host 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> > >> + >> +/** >> + * copy_properties_from_host >> + * >> + * 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 > > This is just "asserts", not "self-asserts". ok > >> + * 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 >> + */ > >> +/** >> + * 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 >> + */ >> +void fdt_build_clock_node(void *host_fdt, void *guest_fdt, >> + uint32_t host_phandle, >> + uint32_t guest_phandle); >> +void fdt_build_clock_node(void *host_fdt, void *guest_fdt, >> + uint32_t host_phandle, >> + uint32_t guest_phandle) >> +{ >> + char *node_path = NULL; >> + char *nodename; >> + const void *r; >> + int ret, node_offset, prop_len, path_len = 16; >> + >> + node_offset = fdt_node_offset_by_phandle(host_fdt, host_phandle); >> + if (node_offset <= 0) { >> + error_setg(&error_fatal, >> + "not able to locate clock handle %d in host device tree", >> + host_phandle); > > Don't we want to return here, rather than continuing with the > rest of the function? ("goto err;" with an err: label before > the g_free() at the bottom of the function probably best since > some of the error paths need to free that.) due to the &error_fatal, we are going to exit here. I thought it is not worth going further if we can't find the clock node. Best Regards Eric > >> + } >> + node_path = g_malloc(path_len); >> + while ((ret = fdt_get_path(host_fdt, node_offset, node_path, path_len)) >> + == -FDT_ERR_NOSPACE) { >> + path_len += 16; >> + node_path = g_realloc(node_path, path_len); >> + } >> + if (ret < 0) { >> + error_setg(&error_fatal, >> + "not able to retrieve node path for clock handle %d", >> + host_phandle); >> + } >> + >> + r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len, >> + &error_fatal); >> + if (strcmp(r, "fixed-clock")) { >> + error_setg(&error_fatal, >> + "clock handle %d is not a fixed clock", host_phandle); >> + } >> + >> + nodename = strrchr(node_path, '/'); >> + qemu_fdt_add_subnode(guest_fdt, nodename); >> + >> + copy_properties_from_host(clock_copied_properties, >> + ARRAY_SIZE(clock_copied_properties), >> + host_fdt, guest_fdt, >> + node_path, nodename); >> + >> + qemu_fdt_setprop_cell(guest_fdt, nodename, "phandle", guest_phandle); >> + >> + g_free(node_path); >> +} >> + >> /* Device Specific Code */ > > thanks > -- PMM >
On 25 January 2016 at 14:09, Eric Auger <eric.auger@linaro.org> wrote: > Hi Peter, > On 01/25/2016 03:05 PM, Peter Maydell wrote: >> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote: >> Don't we want to return here, rather than continuing with the >> rest of the function? ("goto err;" with an err: label before >> the g_free() at the bottom of the function probably best since >> some of the error paths need to free that.) > due to the &error_fatal, we are going to exit here. I thought it is not > worth going further if we can't find the clock node. Yes, sorry, I didn't notice the error_fatal til I got to about the last patch I reviewed. You can ignore similar comments I made about some of the other patches. thanks -- PMM
On 25 January 2016 at 14:34, Peter Maydell <peter.maydell@linaro.org> wrote: > On 25 January 2016 at 14:09, Eric Auger <eric.auger@linaro.org> wrote: >> Hi Peter, >> On 01/25/2016 03:05 PM, Peter Maydell wrote: >>> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote: > >>> Don't we want to return here, rather than continuing with the >>> rest of the function? ("goto err;" with an err: label before >>> the g_free() at the bottom of the function probably best since >>> some of the error paths need to free that.) >> due to the &error_fatal, we are going to exit here. I thought it is not >> worth going further if we can't find the clock node. > > Yes, sorry, I didn't notice the error_fatal til I got to about > the last patch I reviewed. You can ignore similar comments > I made about some of the other patches. ...so if you fix the 'asserts' comment thing, you can have Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 01/25/2016 03:43 PM, Peter Maydell wrote: > On 25 January 2016 at 14:34, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 25 January 2016 at 14:09, Eric Auger <eric.auger@linaro.org> wrote: >>> Hi Peter, >>> On 01/25/2016 03:05 PM, Peter Maydell wrote: >>>> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote: >> >>>> Don't we want to return here, rather than continuing with the >>>> rest of the function? ("goto err;" with an err: label before >>>> the g_free() at the bottom of the function probably best since >>>> some of the error paths need to free that.) >>> due to the &error_fatal, we are going to exit here. I thought it is not >>> worth going further if we can't find the clock node. >> >> Yes, sorry, I didn't notice the error_fatal til I got to about >> the last patch I reviewed. You can ignore similar comments >> I made about some of the other patches. > > ...so if you fix the 'asserts' comment thing, you can have > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> OK thanks for the whole review Eric > > thanks > -- PMM >
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c index 9d28797..d85c9e6 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,125 @@ typedef struct NodeCreationPair { int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque); } NodeCreationPair; +/* helpers */ + +typedef struct HostProperty { + const char *name; + bool optional; +} HostProperty; + +/** + * copy_properties_from_host + * + * 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 copy_properties_from_host(HostProperty *props, int nb_props, + void *host_fdt, void *guest_fdt, + char *node_path, char *nodename) +{ + int i, prop_len; + const void *r; + Error *err = NULL; + + for (i = 0; i < nb_props; i++) { + r = qemu_fdt_getprop(host_fdt, node_path, + props[i].name, + &prop_len, + props[i].optional ? &err : &error_fatal); + if (r) { + qemu_fdt_setprop(guest_fdt, nodename, + props[i].name, r, prop_len); + } else { + if (prop_len != -FDT_ERR_NOTFOUND) { + /* optional property not returned although property exists */ + error_report_err(err); + } else { + error_free(err); + } + } + } +} + +/* clock properties whose values are copied/pasted from host */ +static HostProperty clock_copied_properties[] = { + {"compatible", false}, + {"#clock-cells", false}, + {"clock-frequency", true}, + {"clock-output-names", true}, +}; + +/** + * 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 + */ +void fdt_build_clock_node(void *host_fdt, void *guest_fdt, + uint32_t host_phandle, + uint32_t guest_phandle); +void fdt_build_clock_node(void *host_fdt, void *guest_fdt, + uint32_t host_phandle, + uint32_t guest_phandle) +{ + char *node_path = NULL; + char *nodename; + const void *r; + int ret, node_offset, prop_len, path_len = 16; + + node_offset = fdt_node_offset_by_phandle(host_fdt, host_phandle); + if (node_offset <= 0) { + error_setg(&error_fatal, + "not able to locate clock handle %d in host device tree", + host_phandle); + } + node_path = g_malloc(path_len); + while ((ret = fdt_get_path(host_fdt, node_offset, node_path, path_len)) + == -FDT_ERR_NOSPACE) { + path_len += 16; + node_path = g_realloc(node_path, path_len); + } + if (ret < 0) { + error_setg(&error_fatal, + "not able to retrieve node path for clock handle %d", + host_phandle); + } + + r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len, + &error_fatal); + if (strcmp(r, "fixed-clock")) { + error_setg(&error_fatal, + "clock handle %d is not a fixed clock", host_phandle); + } + + nodename = strrchr(node_path, '/'); + qemu_fdt_add_subnode(guest_fdt, nodename); + + copy_properties_from_host(clock_copied_properties, + ARRAY_SIZE(clock_copied_properties), + host_fdt, guest_fdt, + node_path, nodename); + + qemu_fdt_setprop_cell(guest_fdt, nodename, "phandle", guest_phandle); + + g_free(node_path); +} + /* 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. - copy_properties_from_host 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> --- v4 -> v5: - renamed inherit_properties v1 -> v2: - inherit properties now outputs an error message in case qemu_fdt_getprop fails for an existing optional property - no hardcoded fixed buffer length - fdt_build_clock_node becomes void and auto-asserts on error - use boolean values when defining the clock properties RFC -> v1: - use the new proto of qemu_fdt_getprop - remove newline in error_report - fix some style issues --- hw/arm/sysbus-fdt.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) -- 1.9.1