Re: [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties

[Mark Salter]

On Thu, 2019-10-31 at 16:59 +0100, Javier Martinez Canillas wrote:
> On 10/31/19 12:56 PM, Vladimir 'phcoder' Serbinenko wrote:
> > This patch looks incomplete. What if "chosen" is there but does not contain
> > the relevant fields?
> > 
> 
> My understanding is that if there's a chosen node already defined in the
> Device Tree but without #address-cells and #size-cells properties, then
> is a malformed DT and is not up to GRUB to correct it.

Right. IIRC, this was the case where firmware only has ACPI tables and grub
has to create a DT with just a chosen node.

> 
> But if GRUB is adding a child node as is the case of the chosen node, then
> is up to GRUB to make sure that there are #address-cells and #size-cells
> properties also defined so the child nodes are addressed correctly.
> 
> But maybe I'm wrong on this, so I'll let Mark comment since he authored
> the patch and knows better what's the problem that's fixing.
> 
> > On Thu, 31 Oct 2019, 11:39 Javier Martinez Canillas, <address@hidden>
> > wrote:
> > 
> > > From: Mark Salter <address@hidden>
> > > 
> > > Recent upstream changes to kexec-tools relies on #address-cells
> > > and #size-cells properties in the FDT. If grub2 needs to create
> > > a chosen node, it is likely because firmware did not provide one.
> > > In that case, set #address-cells and #size-cells properties to
> > > make sure they exist.
> > > 
> > > Signed-off-by: Mark Salter <address@hidden>
> > > Signed-off-by: Javier Martinez Canillas <address@hidden>
> > > ---
> > > 
> > >  grub-core/loader/arm64/linux.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git grub-core/loader/arm64/linux.c grub-core/loader/arm64/linux.c
> > > index ef3e9f9444c..24d73732d89 100644
> > > --- grub-core/loader/arm64/linux.c
> > > +++ grub-core/loader/arm64/linux.c
> > > @@ -78,7 +78,21 @@ finalize_params_linux (void)
> > > 
> > >    node = grub_fdt_find_subnode (fdt, 0, "chosen");
> > >    if (node < 0)
> > > -    node = grub_fdt_add_subnode (fdt, 0, "chosen");
> > > +    {
> > > +      /*
> > > +       * If we have to create a chosen node, Make sure we
> > > +       * have #address-cells and #size-cells properties.
> > > +       */
> > > +      retval = grub_fdt_set_prop32(fdt, 0, "#address-cells", 2);
> > > +      if (retval)
> > > +        goto failure;
> > > +
> > > +      retval = grub_fdt_set_prop32(fdt, 0, "#size-cells", 2);
> > > +      if (retval)
> > > +        goto failure;
> > > +
> > > +      node = grub_fdt_add_subnode (fdt, 0, "chosen");
> > > +    }
> > > 
> > >    if (node < 1)
> > >      goto failure;
> > > --
> > > 2.21.0
> > > 
> > > 
>  
> Best regards,