[PATCH 2/2] fdt: Swap the signature for board_fdt_blob_setup()

[Simon Glass]

Hi Caleb,

On Mon, 21 Oct 2024 at 15:28, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>
> Hi Simon,
>
> On 21/10/2024 13:42, Simon Glass wrote:
> > This returns a devicetree and updates a parameter with an error code.
> > Swap it, since this fits better with the way U-Boot normally works. It
> > also (more easily) allows leaving the existing pointer unchanged.
> >
> > No yaks were harmed in this change, but there is a very small code-size
> > reduction.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
>
> ...
>
> > diff --git a/arch/arm/mach-snapdragon/board.c b/arch/arm/mach-snapdragon/board.c
> > index 2ab2ceb5138..7a7a36831c3 100644
> > --- a/arch/arm/mach-snapdragon/board.c
> > +++ b/arch/arm/mach-snapdragon/board.c
> > @@ -147,12 +147,11 @@ static void show_psci_version(void)
> >    * or for supporting quirky devices where it's easier to leave the downstream DT in place
> >    * to improve ABL compatibility. Otherwise, we use the DT provided by ABL.
> >    */
> > -void *board_fdt_blob_setup(int *err)
> > +int board_fdt_blob_setup(void **fdtp)
> >   {
> >       struct fdt_header *fdt;
> >       bool internal_valid, external_valid;
> >
> > -     *err = 0;
> >       fdt = (struct fdt_header *)get_prev_bl_fdt_addr();
> >       external_valid = fdt && !fdt_check_header(fdt);
> >       internal_valid = !fdt_check_header(gd->fdt_blob);
> > @@ -170,7 +169,7 @@ void *board_fdt_blob_setup(int *err)
> >       } else {
> >               debug("Using external FDT\n");
> >               /* So we can use it before returning */
> > -             gd->fdt_blob = fdt;
> > +             *fdtp = fdt;
>
> I believe this is a breaking change. The qcom_parse_memory() call below
> expects gd->fdt_blob to point to the in-use FDT. This is a bit of a
> hack, but doing memory parsing this early simplifies things for us.
>
> Additionally, this change doesn't make the function return -EEXIST when
> it should.

Hmm, OK, thanks. I wonder if there is another place where the memory
can be parsed, rather than as a side-effect of this call. I will
update this patch and try again.

>
> ...
>
> > diff --git a/include/fdtdec.h b/include/fdtdec.h
> > index 555c9520379..9e36acc7e9b 100644
> > --- a/include/fdtdec.h
> > +++ b/include/fdtdec.h
> > @@ -1191,11 +1191,13 @@ int fdtdec_resetup(int *rescan);
> >    *
> >    * The existing devicetree is available at gd->fdt_blob
> >    *
> > - * @err: 0 on success, -EEXIST if the devicetree is already correct, or other
> > + * @fdtp: Existing devicetree blob pointer; update this and return 0 if a
>
> It doesn't looks like this is initialised before calling
> board_fdt_blob_setup()?
>
> Kind regards,
> > + * different devicetree should be used
> > + * Return: 0 on success, -EEXIST if the devicetree is already correct, 0 to use
> > + * *@fdtp as the new devicetree, or other
> >    * internal error code if we fail to setup a DTB
> > - * @returns new devicetree blob pointer
> >    */
> > -void *board_fdt_blob_setup(int *err);
> > +int board_fdt_blob_setup(void **fdtp);
> >
> >   /*
> >    * Decode the size of memory
> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > index 60e28173c03..e876b17f9ad 100644
> > --- a/lib/fdtdec.c
> > +++ b/lib/fdtdec.c
> > @@ -1706,11 +1706,16 @@ int fdtdec_setup(void)
> >
> >       /* Allow the board to override the fdt address. */
> >       if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > -             gd->fdt_blob = board_fdt_blob_setup(&ret);
> > -             if (!ret)
> > +             void *blob;
> > +
> > +             ret = board_fdt_blob_setup(&blob);
> > +             if (ret) {
> > +                     if (ret != -EEXIST)
> > +                             return ret;
> > +             } else {
> >                       gd->fdt_src = FDTSRC_BOARD;
> > -             else if (ret != -EEXIST)
> > -                     return ret;
> > +                     gd->fdt_blob = blob;
> > +             }
> >       }
> >
> >       /* Allow the early environment to override the fdt address */
>
> --
> // Caleb (they/them)
>

Regards,
Simon