Re: [PATCH v2 2/2] fdt: make fdt handling reusable across arch

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 8/3/23 08:48, Michal Orzel wrote:
> Hi Daniel,
>
> On 03/08/2023 12:44, Daniel P. Smith wrote:
> > This refactors reusable code from Arm's bootfdt.c and device-tree.h that is
> > general fdt handling code.  The Kconfig parameter CORE_DEVICE_TREE is
> IIUC, you just try to untangle the code for fdt from dt (unflattened). 
> CORE_DEVICE_TREE is
> a bit ambiguous in my opinion, so maybe just CONFIG_FDT, especially since you 
> use it to guard libfdt/?
Untangle is a very good way to phrase it. ( ^_^)

Yes, I don't see any reason why CONFIG_FDT could be used instead.

> > introduced for when the ability of parsing DTB files is needed by a capability
> > such as hyperlaunch.
> >
> > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > ---
> >   MAINTAINERS                      |   8 +-
> >   xen/arch/arm/bootfdt.c           | 141 +---------------------------
> >   xen/arch/arm/domain_build.c      |   1 +
> >   xen/arch/arm/include/asm/setup.h |   6 --
> >   xen/common/Kconfig               |   4 +
> >   xen/common/Makefile              |   3 +-
> >   xen/common/fdt.c                 | 153 +++++++++++++++++++++++++++++++
> >   xen/include/xen/device_tree.h    |  50 +---------
> >   xen/include/xen/fdt.h            |  79 ++++++++++++++++
> >   9 files changed, 246 insertions(+), 199 deletions(-)
> >   create mode 100644 xen/common/fdt.c
> >   create mode 100644 xen/include/xen/fdt.h
> [...]
>
> > diff --git a/xen/common/fdt.c b/xen/common/fdt.c
> > new file mode 100644
> > index 0000000000..8d7acaaa43
> > --- /dev/null
> > +++ b/xen/common/fdt.c
> > @@ -0,0 +1,153 @@
> > +/*
> SPDX missing for a new file
Ack.

> > + * Flattened Device Tree
> > + *
> > + * Copyright (C) 2012-2014 Citrix Systems, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +#include <xen/fdt.h>
> > +#include <xen/init.h>
> > +#include <xen/lib.h>
> > +#include <xen/libfdt/libfdt.h>
> > +#include <xen/types.h>
> > +
> > +bool __init device_tree_node_matches(
> > +    const void *fdt, int node, const char *match)
> FWICS, this code style (that you use for every function in this patch) is 
> rather uncommon in Xen so maybe better to follow the generic style.
> Also, this would avoid changing the style of functions/prototypes you move.
Um, my understanding is that this is the official style for Xen function 
definitions, unless it has changed due to MISRA, and that anytime a 
function definition is changed that the style should be corrected to 
this form.
Another coding style change I have been pinged on in the past that I 
surprised I have not been pinged on for this series is allowing the use 
of the short hand notation for ints, e.g. u8, u16, and u32.
> [...]
>
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index 1d79e23b28..82db38b140 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -14,13 +14,12 @@
> >   #include <asm/device.h>
> >   #include <public/xen.h>
> >   #include <public/device_tree_defs.h>
> > +#include <xen/fdt.h>
> >   #include <xen/kernel.h>
> >   #include <xen/string.h>
> >   #include <xen/types.h>
> >   #include <xen/list.h>
> >
> > -#define DEVICE_TREE_MAX_DEPTH 16
> > -
> >   /*
> >    * Struct used for matching a device
> >    */
> > @@ -159,17 +158,8 @@ struct dt_raw_irq {
> >       u32 specifier[DT_MAX_IRQ_SPEC];
> >   };
> >
> > -typedef int (*device_tree_node_func)(const void *fdt,
> > -                                     int node, const char *name, int depth,
> > -                                     u32 address_cells, u32 size_cells,
> > -                                     void *data);
> > -
> >   extern const void *device_tree_flattened;
> >
> > -int device_tree_for_each_node(const void *fdt, int node,
> > -                              device_tree_node_func func,
> > -                              void *data);
> > -
> >   /**
> >    * dt_unflatten_host_device_tree - Unflatten the host device tree
> >    *
> > @@ -214,14 +204,6 @@ extern const struct dt_device_node 
> > *dt_interrupt_controller;
> >   struct dt_device_node *
> >   dt_find_interrupt_controller(const struct dt_device_match *matches);
> >
> > -#define dt_prop_cmp(s1, s2) strcmp((s1), (s2))
> > -#define dt_node_cmp(s1, s2) strcasecmp((s1), (s2))
> > -#define dt_compat_cmp(s1, s2) strcasecmp((s1), (s2))
> > -
> > -/* Default #address and #size cells */
> > -#define DT_ROOT_NODE_ADDR_CELLS_DEFAULT 2
> > -#define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1
> > -
> >   #define dt_for_each_property_node(dn, pp)                   \
> >       for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next )
> >
> > @@ -231,16 +213,6 @@ dt_find_interrupt_controller(const struct dt_device_match 
> > *matches);
> >   #define dt_for_each_child_node(dt, dn)                      \
> >       for ( dn = (dt)->child; (dn) != NULL; dn = (dn)->sibling )
> >
> > -/* Helper to read a big number; size is in cells (not bytes) */
> > -static inline u64 dt_read_number(const __be32 *cell, int size)
> > -{
> > -    u64 r = 0;
> > -
> > -    while ( size-- )
> > -        r = (r << 32) | be32_to_cpu(*(cell++));
> > -    return r;
> > -}
> > -
> >   /* Wrapper for dt_read_number() to return paddr_t (instead of uint64_t) */
> >   static inline paddr_t dt_read_paddr(const __be32 *cell, int size)
> Shouldn't this also go to fdt.h as it is just a wrapper for dt_read_number() 
> you moved?
This patch came from hyperlaunch v1 series were I only moved what I 
needed, not necessarily everything that could reasonably be moved. I 
will gladly incorporate any additional macros, inlines, and functions 
that is felt is general enough to be included.
v/r,
dps