Re: [Qemu-devel] [PATCH v2 2/9] devicetree: Added new APIs to make use of more fdt functions

[Auger Eric]

Hi Prem,

> SMMUv3 needs device tree entry like below
To me the commit message should be more explicit and mention appendprop
functionality
> 
>      interrupt-names = "gerror", "priq", "eventq", "cmdq-sync";
> 
> This patch introduces helper function to add entries like above
> 
> Signed-off-by: Prem Mallappa <address@hidden>
> ---
>  device_tree.c                | 35 +++++++++++++++++++++++++++++++++++
>  include/sysemu/device_tree.h | 18 ++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/device_tree.c b/device_tree.c
> index 6e06320..5d5966e 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -297,6 +297,24 @@ int qemu_fdt_setprop(void *fdt, const char *node_path,
>      return r;
>  }
>  
> +int qemu_fdt_appendprop(void *fdt, const char *node_path,
> +                     const char *property, const void *val, int size)
> +{
> +    int r;
> +
> +    r = fdt_appendprop(fdt, findnode_nofail(fdt, node_path), property,
> +                       val, size);
> +    if (r < 0) {
> +        error_report("%s: Couldn't set %s/%s: %s", __func__, node_path,
> +                     property, fdt_strerror(r));
> +        exit(1);
> +    }
> +
> +    return r;
> +}
> +
> +
spare void lines
> +
>  int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
>                            const char *property, uint32_t val)
>  {
> @@ -319,6 +337,23 @@ int qemu_fdt_setprop_u64(void *fdt, const char 
> *node_path,
>      return qemu_fdt_setprop(fdt, node_path, property, &val, sizeof(val));
>  }
>  
> +int qemu_fdt_appendprop_string(void *fdt, const char *node_path,
> +                            const char *property, const char *string)
> +{
> +    int r;
> +
> +    r = fdt_appendprop_string(fdt, findnode_nofail(fdt, node_path),
> +                              property, string);
> +    if (r < 0) {
> +        error_report("%s: Couldn't set %s/%s = %s: %s", __func__,
> +                     node_path, property, string, fdt_strerror(r));
> +        exit(1);
> +    }
> +
> +    return r;
> +}
> +
same
> +
>  int qemu_fdt_setprop_string(void *fdt, const char *node_path,
>                              const char *property, const char *string)
>  {
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index 705650a..5a0a297 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -45,12 +45,16 @@ char **qemu_fdt_node_path(void *fdt, const char *name, 
> char *compat,
>  
>  int qemu_fdt_setprop(void *fdt, const char *node_path,
>                       const char *property, const void *val, int size);
> +int qemu_fdt_appendprop(void *fdt, const char *node_path,
> +                     const char *property, const void *val, int size);
>  int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
>                            const char *property, uint32_t val);
>  int qemu_fdt_setprop_u64(void *fdt, const char *node_path,
>                           const char *property, uint64_t val);
>  int qemu_fdt_setprop_string(void *fdt, const char *node_path,
>                              const char *property, const char *string);
> +int qemu_fdt_appendprop_string(void *fdt, const char *node_path,
> +                               const char *property, const char *string);
>  int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
>                               const char *property,
>                               const char *target_node_path);
> @@ -98,6 +102,20 @@ int qemu_fdt_add_subnode(void *fdt, const char *name);
>                           sizeof(qdt_tmp));                                   
>  \
>      } while (0)
>  
> +
same
> +#define qemu_fdt_appendprop_cells(fdt, node_path, property, ...)             
>  \
> +    do {                                                                     
>  \
> +        uint32_t qdt_tmp[] = { __VA_ARGS__ };                                
>  \
> +        int i;                                                               
>  \
> +                                                                             
>  \
> +        for (i = 0; i < ARRAY_SIZE(qdt_tmp); i++) {                          
>  \
> +            qdt_tmp[i] = cpu_to_be32(qdt_tmp[i]);                            
>  \
> +        }                                                                    
>  \
> +        qemu_fdt_appendprop(fdt, node_path, property, qdt_tmp,               
>  \
> +                         sizeof(qdt_tmp));                                   
>  \
> +    } while (0)
> +
> +
same here.

While I understand the benefits I think we could manage without this new
API by using qemu_fdt_setprop and populating the uint32_t array separately.

I understood the QEMU API for manipulating flattened trees was not
supposed to grow too much but I don't have a strong option here. You
should CC David Gibson I think.

Thanks

Eric


>  void qemu_fdt_dumpdtb(void *fdt, int size);
>  
>  /**
>