[coreboot] [poll] device_t

Aaron Durbin adurbin at google.com
Fri May 8 17:31:51 CEST 2015


On Thu, May 7, 2015 at 3:42 PM, Patrick Georgi via coreboot
<coreboot at coreboot.org> wrote:
> 2015-05-07 22:00 GMT+02:00 Stefan Reinauer <stefan.reinauer at coreboot.org>:
>> With our current bootblock concept, it is never going away on x86 (for
>> bootblock usage)
> Which isn't that much of a problem once we provide separate headers
> for x86 bootblock code. There's really very tiny overlap.
> That could then be reused to deal with raminit on romcc-boards, too:
> from coreboot's point of view, raminit is just an overly large
> piece of cache-as-ram code, followed by a raminit noop.
> This is simplified by the lack of the need for development tools (eg
> printk) to develop new non-car x86 raminits.
>
>

I think bootblock code overlap is tiny. However, device_t is more
complicated than just bootblock and !bootblock. The io routines differ
by stage:

romstage and bootblock use the simple u32 device_t
ramstage uses the sturct device device_t

And lest we not forget SMM on x86 which only has simple u32 device_t.

In romstage *both* struct device and device_t are present but are
completely different types. It's the romstage, ramstage, and smm
overlap that is large and extremely annoying to deal with.

A few examples on boiler plate one needs to deal with for code sharing:
src/soc/intel/baytrail/pmutil.c -- all the get_pcu_dev()
src/soc/intel/baytrail/spi.c

So basically while the function names are the same for the IO routines
the parameter types change. So every caller of a compilation unit of
these IO routines that wants to be shared needs to deal w/ the current
type changes.

I personally feel that changing device_t type based on stage makes the
code non-obvious and hard to follow.

I'd rather we *always* provide simple u32 device_t functions in all
stages while allowing struct device IO functions for use in ramstage.
The simple ones in ramstage can just be a struct device lookup then
call appropriate IO function.


-Aaron



More information about the coreboot mailing list