[U-Boot] CONFIG_OF_EMBED/CONFIG_OF_SEPARATE and fdtcontroladdr

[Simon Glass]

Hi Michal,

On Wed, Jul 4, 2012 at 7:43 AM, Michal Simek <monstr at monstr.eu> wrote:
> Hi Simon,
>
> I have create another thread because this can be discuss separately.
>
> In board file there is this code (We discussed fdtcontroladdr separately).
>
> #ifdef CONFIG_OF_EMBED
>         /* Get a pointer to the FDT */
>         gd->fdt_blob = _binary_dt_dtb_start;
> #elif defined CONFIG_OF_SEPARATE
>         /* FDT is at end of image */
>         gd->fdt_blob = (void *)(_end_ofs + _TEXT_BASE);
> #endif
>         /* Allow the early environment to override the fdt address */
>         gd->fdt_blob = (void *)getenv_ulong("fdtcontroladdr", 16,
>                                                 (uintptr_t)gd->fdt_blob);
>
>
> Is there any reason to use CONFIG_ options here?
> Because what I think will be the best is to remove CONFIG_ options and
> create
> priorities and checking in at run time.
>
> Priority should be:
> 1. variable
> 2. appended blob
> 3. compiled in blob
>
> Because compiled in blob can be also used as safe blob for cases where
> fdtcontroladdr
> points to nor flash which is broken. For this case current u-boot will fail
> because
> fdt_prepare won't find out valid DTB.
>
> Here is just skeleton to show you what design I would prefer.
>
> int fdtdec_prepare_fdt(void *dtb)
> {
>         if (((uintptr_t)gd->fdt_blob & 3) || fdt_check_header(dtb)) {
>                 return -1;
>         }
>         return 0;
> }
>
> void * fdt_addr = (void *)getenv_ulong("fdtcontroladdr", 16,
> (uintptr_t)gd->fdt_blob);
> if(!fdt_prepare(fdt_addr)) {
>         gd->fdt_blob = fdt_addr;
> } else {
>         fdt_addr = (void *)(_end_ofs + _TEXT_BASE);
>         if(!fdt_prepare(fdt_addr)) {
>                 gd->fdt_blob = fdt_addr;

This much is probably reasonable, although I am not 100% comfortable
with fallbacks. We should perhaps provide at least some debug() stuff
to indicate where the fdt came from.

>         } else {
>                 fdt_addr = _binary_dt_dtb_start; // FIXME special section
> for it

What if this doesn't exist? Are you saying that we should have a
built-in fdt as a backup for when one is not supplied? I worry that
this is going to be incorrect (e.g. it might be for default hardware)
so would just create a hard-to-find bug.

Also what if _binary_dt_dtb_start doesn't exist?

>                 if(!fdt_prepare(fdt_addr)) {
>                         gd->fdt_blob = fdt_addr;
>                 }
>         }
> }
>
> Thanks,
> Michal
>
> --
> Michal Simek, Ing. (M.Eng)
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
> Microblaze U-BOOT custodian

Regards,
Simon