Skip to contentSkip to conversation

CONFIG_OF_EMBED/CONFIG_OF_SEPARATE and fdtcontroladdr

U-Boot Michal Simek 2 participants 2 messages Jul 4, 2012 – Jul 19, 2012

lore
Share & tools
lore
#0 lore raw

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

Signature · 4 lines
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
#1 lore raw

Hi Michal,

Quoted 44 lines · 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;
}
Quoted 8 lines

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.

Quoted 3 lines

} 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?

Quoted 6 lines
                if(!fdt_prepare(fdt_addr)) {
                        gd->fdt_blob = fdt_addr;
                }
        }
}
Quoted 8 lines

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