Re: [Qemu-devel] [PATCH v5 6/8] hw/arm/sysbus-fdt: helpers for clock node generation

[Peter Maydell]

On 18 January 2016 at 15:16, Eric Auger <address@hidden> 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 <address@hidden>

> +
> +/**
> + * 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