[v2,5/7] hw/arm/sysbus-fdt: helpers for clock node generation

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>

---

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(+)

On Wed, Jan 06, 2016 at 03:13:23PM +0000, Eric Auger 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

I dislike the name, since the first thing I think when I see "inherit"
is that it's about a node inheriting a property from an ancestor node,
not the guest inheriting properties from the host.  Maybe
"passthrough_properties()"?

> - 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>
> 
> ---
> 
> 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(+)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 9d28797..a1cf57b 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;
> +
> +/**
> + * 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;
> +    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_inherited_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);
> +
> +    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);
> +
> +    g_free(node_path);
> +}
> +
>  /* Device Specific Code */
>  
>  /**

Hi David,
On 01/11/2016 03:41 AM, David Gibson wrote:
> On Wed, Jan 06, 2016 at 03:13:23PM +0000, Eric Auger 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
> 
> I dislike the name, since the first thing I think when I see "inherit"
> is that it's about a node inheriting a property from an ancestor node,
> not the guest inheriting properties from the host.  Maybe
> "passthrough_properties()"?
No Problem, I will rename the function

Best Regards

Eric

> 
>> - 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>
>>
>> ---
>>
>> 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(+)
>>
>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>> index 9d28797..a1cf57b 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;
>> +
>> +/**
>> + * 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;
>> +    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_inherited_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);
>> +
>> +    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);
>> +
>> +    g_free(node_path);
>> +}
>> +
>>  /* Device Specific Code */
>>  
>>  /**
>