[U-Boot] style issue: ft_board_setup() versus ft_verify_fdt()?

[U-Boot] style issue: ft_board_setup() versus ft_verify_fdt()?

[Robert P. J. Day]

  curious about recommended coding style related to modifying and
verifying FDTs ... i'm perusing common/image-fdt.c and can see the
order of (possible) operations here in the function
image_setup_libfdt():

 * arch_fixup_fdt()
 * ft_board_setup()
 * ft_system_setup()
 * fdt_fixup_ethernet()
   ... etc etc ...
 * ft_verify_fdt()

where an earlier comment explains that last routine:

/*
 * Verify the device tree.
 *
 * This function is called after all device tree fix-ups have been enacted,
 * so that the final device tree can be verified.  The definition of "verified"
 * is up to the specific implementation.  However, it generally means that the
 * addresses of some of the devices in the device tree are compared with the
 * actual addresses at which U-Boot has placed them.
 *
 * Returns 1 on success, 0 on failure.  If 0 is returned, U-Boot will halt the
 * boot process.
 */
__weak int ft_verify_fdt(void *fdt)
{
        return 1;
}


  which seems reasonable ... after you've mangled your FDT in every
way you need, optionally supply a routine to make sure it looks sane.

  however, for the freescale board mpc8641hpcn.c, here's part of the
ft_board_setup() routine (actually, this is pretty much all that
function does):


        if (tmp) {
                u64 addr;

                if (addrcells == 1)
                        addr = *(u32 *)tmp;
                else
                        addr = *tmp;

                if (addr != CONFIG_SYS_CCSRBAR_PHYS)
                        printf("WARNING: The CCSRBAR address in your .dts "
                               "does not match the address of the CCSR "
                               "in u-boot.  This means your .dts might "
                               "be old.\n");
        }

so the board setup routine is doing some quick sanity checking, which
is perfectly fine, but seems to be exactly the sort of thing
ft_verify_fdt() was designed for.

  obviously, one can add sanity checking to any of those routines
depending on how early you want to notice stuff -- is there somewhere
a style guide that gives recommendations? just looking for "best
practices," thanks.

  i'm sure there will be more FDT-related questions coming ... thank
you for your patience.

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

[U-Boot] style issue: ft_board_setup() versus ft_verify_fdt()?

[Simon Glass]

Hi Robert,

On 19 May 2016 at 06:51, Robert P. J. Day <rpjday@crashcourse.ca> wrote:
>
>   curious about recommended coding style related to modifying and
> verifying FDTs ... i'm perusing common/image-fdt.c and can see the
> order of (possible) operations here in the function
> image_setup_libfdt():
>
>  * arch_fixup_fdt()
>  * ft_board_setup()
>  * ft_system_setup()
>  * fdt_fixup_ethernet()
>    ... etc etc ...
>  * ft_verify_fdt()
>
> where an earlier comment explains that last routine:
>
> /*
>  * Verify the device tree.
>  *
>  * This function is called after all device tree fix-ups have been enacted,
>  * so that the final device tree can be verified.  The definition of "verified"
>  * is up to the specific implementation.  However, it generally means that the
>  * addresses of some of the devices in the device tree are compared with the
>  * actual addresses at which U-Boot has placed them.
>  *
>  * Returns 1 on success, 0 on failure.  If 0 is returned, U-Boot will halt the
>  * boot process.
>  */
> __weak int ft_verify_fdt(void *fdt)
> {
>         return 1;
> }
>
>
>   which seems reasonable ... after you've mangled your FDT in every
> way you need, optionally supply a routine to make sure it looks sane.
>
>   however, for the freescale board mpc8641hpcn.c, here's part of the
> ft_board_setup() routine (actually, this is pretty much all that
> function does):
>
>
>         if (tmp) {
>                 u64 addr;
>
>                 if (addrcells == 1)
>                         addr = *(u32 *)tmp;
>                 else
>                         addr = *tmp;
>
>                 if (addr != CONFIG_SYS_CCSRBAR_PHYS)
>                         printf("WARNING: The CCSRBAR address in your .dts "
>                                "does not match the address of the CCSR "
>                                "in u-boot.  This means your .dts might "
>                                "be old.\n");
>         }
>
> so the board setup routine is doing some quick sanity checking, which
> is perfectly fine, but seems to be exactly the sort of thing
> ft_verify_fdt() was designed for.
>
>   obviously, one can add sanity checking to any of those routines
> depending on how early you want to notice stuff -- is there somewhere
> a style guide that gives recommendations? just looking for "best
> practices," thanks.
>
>   i'm sure there will be more FDT-related questions coming ... thank
> you for your patience.

My suggestion would be to always use an 'fdt_' prefix.

Regards,
Simon