RE: [PATCH] xen/arm, device-tree: Make static-mem use #{address,size}-cells

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Michal,

Thank you very much for your review, as always :))

> -----Original Message-----
> From: Michal Orzel <michal.orzel@xxxxxxx>
> > @@ -362,14 +362,13 @@ device-tree:
> >
> >      / {
> >          chosen {
> > +            #address-cells = <0x1>;
> > +            #size-cells = <0x1>;
> > +            ...
> >              domU1 {
> >                  compatible = "xen,domain";
> > -                #address-cells = <0x2>;
> > -                #size-cells = <0x2>;
> Why did you remove this set if it relates to the childs of domU1 (e.g. kernel,
> ramdisk) and not to domU1 itself?

Well, I think here the example is just how we setup the static memory, so we 
just
want to emphasize the related part. I agree users can add another #address-cells
and #size-cells for domU1 node for the parts that you mentioned, but that is
not reflected by the current example (I can't find anything related to kernel,
ramdisk, etc. in current example). I might get it wrong but having two 
#address-cells
and #size-cells in that case would be quite misleading from my understanding.
So I decided to remove it.

But I am open to other opinions. So shall we let the discussion go on for a bit 
longer?
I will revert this change if most people think this removing is unnecessary.

> 
> >                  cpus = <2>;
> >                  memory = <0x0 0x80000>;
> > -                #xen,static-mem-address-cells = <0x1>;
> > -                #xen,static-mem-size-cells = <0x1>;
> >                  xen,static-mem = <0x30000000 0x20000000>;
> >                  ...
> >              };
> > diff --git a/docs/misc/arm/passthrough-noiommu.txt
> b/docs/misc/arm/passthrough-noiommu.txt
> > index 3e2ef21ad7..69b8de1975 100644
> > --- a/docs/misc/arm/passthrough-noiommu.txt
> > +++ b/docs/misc/arm/passthrough-noiommu.txt
> > @@ -33,14 +33,13 @@ on static allocation in the device-tree:
> >
> >  / {
> >         chosen {
> > +               #address-cells = <0x1>;
> > +               #size-cells = <0x1>;
> > +               ...
> >                 domU1 {
> >                         compatible = "xen,domain";
> > -                       #address-cells = <0x2>;
> > -                       #size-cells = <0x2>;
> The same here.

Same as above.

> 
> >                         cpus = <2>;
> >                         memory = <0x0 0x80000>;
> > -                       #xen,static-mem-address-cells = <0x1>;
> > -                       #xen,static-mem-size-cells = <0x1>;
> >                         xen,static-mem = <0x30000000 0x20000000>;
> >                         direct-map;
> >                         ...
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index ec81a45de9..cd264793d5 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -352,11 +352,6 @@ static int __init process_domain_node(const void
> *fdt, int node,
> >          /* No "xen,static-mem" present. */
> >          return 0;
> >
> > -    address_cells = device_tree_get_u32(fdt, node,
> > -                                        "#xen,static-mem-address-cells", 
> > 0);
> > -    size_cells = device_tree_get_u32(fdt, node,
> > -                                     "#xen,static-mem-size-cells", 0);
> > -
> >      return device_tree_get_meminfo(fdt, node, "xen,static-mem",
> address_cells,
> >                                     size_cells, &bootinfo.reserved_mem, 
> > true);
> >  }
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index b76a84e8f5..258d74699d 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -563,21 +563,9 @@ static int __init parse_static_mem_prop(const
> struct dt_device_node *node,
> >      const struct dt_property *prop;
> >
> >      prop = dt_find_property(node, "xen,static-mem", NULL);
> > -    if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
> > -                               addr_cells) )
> > -    {
> > -        printk(XENLOG_ERR
> > -               "failed to read \"#xen,static-mem-address-cells\".\n");
> > -        return -EINVAL;
> > -    }
> >
> > -    if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
> > -                               size_cells) )
> > -    {
> > -        printk(XENLOG_ERR
> > -               "failed to read \"#xen,static-mem-size-cells\".\n");
> > -        return -EINVAL;
> > -    }
> > +    *addr_cells = dt_n_addr_cells(node);
> > +    *size_cells = dt_n_size_cells(node);
> There is a type mismatch here as e.g. addr_cells is u32 and dt_n_addr_cells
> return type is int.
> But I don't think this can be harmful and also it's strange for me that
> dt_n_addr_cells
> is defined to return int given that it either returns 2 or be32_to_cpup, which
> means it should return u32.

Yeah. I agree. I did a git blame here and found this function is introduced 9
years ago in "dbd1243 xen/arm: Add helpers to use the device tree". So I think
probably it would be easier to ask the author for the following action directly 
:))

@Julien, what do you think? Shall we modify the return type of these two
functions?

> 
> >
> >      *cell = (const __be32 *)prop->value;
> >      *length = prop->length;
> > --
> > 2.17.1
> >
> >
> Apart from that:
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

Thanks!

Kind regards,
Henry

> 
> ~Michal