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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien and Michal,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> >>>       / {
> >>>           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.
> 
> I agree with that. As this is only a small part of the DT we want to
> focus on what is necessary for the current section.
> 
> > So I decided to remove it.
> 
> I would mention it in the commit message because the change seems
> unrelated otherwise.

Sure.

> 
> The same apply for replacing adding extra "====". But TBH, this change
> feels completely unrelated to this patch. So I think it is better to
> have a separate patch.

I would revert this change for "====", as sending a patch adding
"====" seems to not make too much sense.....

> 
> [...]
> 
> >>> 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 :))
> 
> The code was imported from Linux where it seems to be more common to
> use
> "int" rather than "unsigned".
> 
> >
> > @Julien, what do you think? Shall we modify the return type of these two
> > functions?
> 
> I think it would be good to be consistent. However, there are other
> users of d_n_addr_cells() (some are expecting 'int'). So if you switch
> to a different type then this use will be consistent but not the others.
> 
> I would only suggest to look at it if you have if you have copious time
> and fancy going down the rabbit hole :).
> 
> As to which type to chose, we are phasing out use of uXX in new code. So
> this should be 'uint32_t'. I would also be fine to use 'unsigned int'
> for the outside interface.
> 
> I don't have a strong opinion either way.

I personally would like to keep the current way, changing the type to me
sounds like out of scope with this patch and will bring us the risk of breaking
other places.

So I will address the commit message and "====" changes and send a v2.

Kind regards,
Henry


> 
> Cheers,
> 
> --
> Julien Grall