[U-Boot] CONFIG_OF_EMBED/CONFIG_OF_SEPARATE and fdtcontroladdr

[U-Boot] CONFIG_OF_EMBED/CONFIG_OF_SEPARATE and fdtcontroladdr

[Michal Simek]

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;
	} else {
		fdt_addr = _binary_dt_dtb_start; // FIXME special section for it
		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

[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@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