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

Hi Caleb,

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