Hi Caleb,
On Mon, 21 Oct 2024 at 15:28, Caleb Connolly <caleb.conno...@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 <s...@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
