Hi,
We have a couple of chipsets in the tree that require external data in CBFS, sometimes with placement requirements (eg. for embedded controllers), and there will be more of that kind to come. Right now, we're adding Kconfig options for each and every of these new files, but that's not a sustainable model.
The patch provides a way for each Makefile.inc to add such files by setting up a couple of variables: # -y can, as usual be used for conditional inclusion cbfs-files-y += filename filename-name := CBFS filename filename-type := CBFS type filename-position := location in CBFS (eg. 0xfff80000)
filename can either be a filename in the current directory (or a relative path from there) or, if that doesn't match a file, a path starting from the tree root. filename in filename-name etc. means the actual filename, for example: cbfs-files-y += mbi.bin mbi.bin-name := mbi.bin mbi.bin-type := 0x80
filename-position is an optional argument. If it doesn't exist, CBFS is free in its placement of the file. The files are added immediately after creation of the CBFS image to make sure that they'll have the space. Even the romstage is added only afterwards.
mbi.bin-* (in the example above) are cleaned after processing to avoid option conflicts if two files of the same name (with different CBFS names) are added.
To avoid code duplication, I moved CBFS image creation from two locations into src/arch/x86/Makefile.inc. That's also part of the patch.
The patch doesn't yet rework MBI, VGABIOS and bootsplash handling into the new method of adding generic files. I'm reasonably sure that MBI and VGABIOS can be moved out of Kconfig without much effect to users (as they're pretty much a binary decision, if the file is used at all, but the file is the same everywhere), but the bootsplash is probably a more individualistic matter, so should be kept in Kconfig. Opinions?
Anyway, the patch is Signed-off-by: Patrick Georgi patrick.georgi@coresystems.de
Regards, Patrick
On 12/15/2010 06:51 AM, Patrick Georgi wrote:
Hi,
We have a couple of chipsets in the tree that require external data in CBFS, sometimes with placement requirements (eg. for embedded controllers), and there will be more of that kind to come. Right now, we're adding Kconfig options for each and every of these new files, but that's not a sustainable model.
The patch provides a way for each Makefile.inc to add such files by setting up a couple of variables: # -y can, as usual be used for conditional inclusion cbfs-files-y += filename filename-name := CBFS filename filename-type := CBFS type filename-position := location in CBFS (eg. 0xfff80000)
filename can either be a filename in the current directory (or a relative path from there) or, if that doesn't match a file, a path starting from the tree root. filename in filename-name etc. means the actual filename, for example: cbfs-files-y += mbi.bin mbi.bin-name := mbi.bin mbi.bin-type := 0x80
filename-position is an optional argument. If it doesn't exist, CBFS is free in its placement of the file. The files are added immediately after creation of the CBFS image to make sure that they'll have the space. Even the romstage is added only afterwards.
mbi.bin-* (in the example above) are cleaned after processing to avoid option conflicts if two files of the same name (with different CBFS names) are added.
To avoid code duplication, I moved CBFS image creation from two locations into src/arch/x86/Makefile.inc. That's also part of the patch.
The patch doesn't yet rework MBI, VGABIOS and bootsplash handling into the new method of adding generic files. I'm reasonably sure that MBI and VGABIOS can be moved out of Kconfig without much effect to users (as they're pretty much a binary decision, if the file is used at all, but the file is the same everywhere), but the bootsplash is probably a more individualistic matter, so should be kept in Kconfig. Opinions?
Hmm, still a little confused what you mean here. So your patch just reworks the code to handle all binary blobs the same???
- Joe
Am Mittwoch, 15. Dezember 2010, um 14:36:51 schrieb Joseph Smith:
Hmm, still a little confused what you mean here. So your patch just reworks the code to handle all binary blobs the same???
It provides an easier way to add binary components. Right now we add them to Kconfig and have common code that switches based on these Kconfig flags in src/arch/x86/Makefile.inc. With this patch, if a chipset component requires some binary, it can add that binary to the build easily without having to touch global code.
src/arch/x86/Makefile.inc contains code for MBI, which is (as far as I can see) specific to a single chipset. That simply doesn't belong in there, but so far there is no other way to do it.
Future chipsets will require more of that rather than less. The growing use of embedded controllers, even in desktop chipsets, also plays a part in this.
What this patch doesn't do is to rewire our existing binary component handling to use the new framework. That's a separate step.
Patrick
On 12/15/2010 09:10 AM, Patrick Georgi wrote:
Am Mittwoch, 15. Dezember 2010, um 14:36:51 schrieb Joseph Smith:
Hmm, still a little confused what you mean here. So your patch just reworks the code to handle all binary blobs the same???
It provides an easier way to add binary components. Right now we add them to Kconfig and have common code that switches based on these Kconfig flags in src/arch/x86/Makefile.inc. With this patch, if a chipset component requires some binary, it can add that binary to the build easily without having to touch global code.
Ok, so does it just search for the file names and if found adds them to the build?
src/arch/x86/Makefile.inc contains code for MBI, which is (as far as I can see) specific to a single chipset. That simply doesn't belong in there, but so far there is no other way to do it.
Oh no, the MBI code can work with all Intel chipsets with tv-out and lvds capabilities (whether internal or external). Currently only i945/ICH7 and i830/ICH4 use it...and another one is coming soon :-)
Future chipsets will require more of that rather than less. The growing use of embedded controllers, even in desktop chipsets, also plays a part in this.
Agreed.
What this patch doesn't do is to rewire our existing binary component handling to use the new framework.
Ok good.
Acked-by: Joseph Smith joe@settoplinux.org
That's a separate step.
Hmm, I think the "separate step" will require some major testing.... I really do not want MBI to get broken.
* Joseph Smith joe@settoplinux.org [101215 15:48]:
src/arch/x86/Makefile.inc contains code for MBI, which is (as far as I can see) specific to a single chipset. That simply doesn't belong in there, but so far there is no other way to do it.
Oh no, the MBI code can work with all Intel chipsets with tv-out and lvds capabilities (whether internal or external). Currently only i945/ICH7 and i830/ICH4 use it...and another one is coming soon :-)
It's really only implemented on i830. i945 has an SMM handler but no MBI support.
Stefan
On 12/15/10 6:48 AM, Joseph Smith wrote:
Acked-by: Joseph Smith joe@settoplinux.org
And
Acked-by: Stefan Reinauer stepan@coreboot.org
Please go ahead and check in.
Index: coreboot-poulsbo/src/arch/x86/Makefile.bigbootblock.inc
--- coreboot-poulsbo.orig/src/arch/x86/Makefile.bigbootblock.inc 2010-12-15 12:32:42.476455002 +0100 +++ coreboot-poulsbo/src/arch/x86/Makefile.bigbootblock.inc 2010-12-15 12:32:43.896455000 +0100 @@ -3,7 +3,7 @@
$(obj)/coreboot.pre: $(obj)/coreboot.bootblock $(CBFSTOOL) rm -f $@
- $(CBFSTOOL) $@ create $(CONFIG_COREBOOT_ROMSIZE_KB)K $(obj)/coreboot.bootblock
- cp $(obj)/coreboot.pre1 $@
This looks odd. Is it needed?
Am Donnerstag, 16. Dezember 2010, um 06:09:50 schrieb Stefan Reinauer:
$(obj)/coreboot.pre: $(obj)/coreboot.bootblock $(CBFSTOOL)
rm -f $@
- $(CBFSTOOL) $@ create $(CONFIG_COREBOOT_ROMSIZE_KB)K
$(obj)/coreboot.bootblock
- cp $(obj)/coreboot.pre1 $@
This looks odd. Is it needed?
Yes: The generic Makefile.inc code in arch/x86 creates a coreboot.pre1 with the cbfs-files-y data. Makefile.bootblock.inc then adds the romstage and calls the result coreboot.pre. Makefile.bigbootblock.inc has nothing to do in that intermediate step from .pre1 to .pre, but to keep the dependency graph identical, we need to copy the data around.
- The coreboot.pre1 rule is now generic code. - The coreboot.pre rule is bootblock/bigbootblock code. - The further processing (coreboot.pre -> coreboot.rom) is generic code again.
This used to be: - The coreboot.pre rule is bootblock/bigbootblock code (and the bootblock code uses .pre1 as intermediate step) - The further processing (coreboot.pre -> coreboot.rom) is generic code.
Patrick
Patrick Georgi wrote:
The generic Makefile.inc code in arch/x86 creates a coreboot.pre1 with the cbfs-files-y data.
Could it have a more informative name than .pre1? Maybe .cbfs-files ?
Makefile.bootblock.inc then adds the romstage and calls the result coreboot.pre.
Ditto? Maybe coreboot.cbfs-bootblock ?
Makefile.bigbootblock.inc has nothing to do in that intermediate step from .pre1 to .pre, but to keep the dependency graph identical, we need to copy the data around.
- The coreboot.pre1 rule is now generic code.
- The coreboot.pre rule is bootblock/bigbootblock code.
- The further processing (coreboot.pre -> coreboot.rom) is generic code again.
Very nice!
Acked-by: Peter Stuge peter@stuge.se
Great idea in general.
Patrick Georgi wrote:
filename-position
This one is tricky. Blobs may need to have alignment, a negative offset (ie. start at end of flash - $amount) rather than a positive, etc. How could we handle those cases?
//Peter
Am Mittwoch, 15. Dezember 2010, um 17:04:00 schrieb Peter Stuge:
Great idea in general.
Patrick Georgi wrote:
filename-position
This one is tricky. Blobs may need to have alignment, a negative offset (ie. start at end of flash - $amount) rather than a positive, etc. How could we handle those cases?
As this is the default case on our platform, we already handle it that way. filename-position is supposed to be the location in the target's address space, so if you want to store a file in the middle of a 1MB flash chip, you tell cbfstool (and this mechanism which forwards that value unchanged to cbfstool) to put the file at 0xfff80000, not 0x80000 (which is the location in the rom image where the file will end up).
The routine also shifts addressing enough to make space for the cbfs header - it's all covered :-)
Once we encounter some weird device where you need to put stuff at the beginning of the flash address space, we can still extend cbfstool to handle positions as follows: Values from 0 to 4 MB: flash chip's address space. Everything else (esp. 0xffc00000..0xffffffff): target address space (ie. like now)
Patrick
* Patrick Georgi patrick@georgi-clan.de [101215 17:18]:
As this is the default case on our platform, we already handle it that way. filename-position is supposed to be the location in the target's address space, so if you want to store a file in the middle of a 1MB flash chip, you tell cbfstool (and this mechanism which forwards that value unchanged to cbfstool) to put the file at 0xfff80000, not 0x80000 (which is the location in the rom image where the file will end up).
I think this is quite awesome! Now the extra mile question that burns on my mind is: What happens if someone resizes a CBFS. Will it blow up?
Stefan
Am Mittwoch, 15. Dezember 2010, um 19:41:57 schrieb Stefan Reinauer:
I think this is quite awesome! Now the extra mile question that burns on my mind is: What happens if someone resizes a CBFS. Will it blow up?
As CBFS would have to be top-aligned, no. The file _still_ resides at 0xfff80000 in the end, even though it's new flash address changes from 0x80000 to 0x180000 (assuming a resize from 1MB to 2MB here).
It's really simple, just think within the address space of the target system, not in file/flash addressing :-)
Patrick
On 12/15/2010 11:04 AM, Peter Stuge wrote:
Great idea in general.
Patrick Georgi wrote:
filename-position
This one is tricky. Blobs may need to have alignment, a negative offset (ie. start at end of flash - $amount) rather than a positive, etc. How could we handle those cases?
//Peter
Ah, great point Peter! The MBI binary bolb modules ***-have-*** to be aligned in order for the VGA Bios to read them, other wise they are useless blobs of crap, and the VGA Bios is smart enough to know that and reject them.
Am Mittwoch, 15. Dezember 2010, um 17:20:33 schrieb Joseph Smith:
Ah, great point Peter! The MBI binary bolb modules ***-have-*** to be aligned in order for the VGA Bios to read them, other wise they are useless blobs of crap, and the VGA Bios is smart enough to know that and reject them.
That's exactly what filename-position is about. If the current variant works, the new one will, too: They do precisely the same thing, they merely move configuration elsewhere.
Patrick
* Joseph Smith joe@settoplinux.org [101215 17:20]:
On 12/15/2010 11:04 AM, Peter Stuge wrote:
Great idea in general.
Patrick Georgi wrote:
filename-position
This one is tricky. Blobs may need to have alignment, a negative offset (ie. start at end of flash - $amount) rather than a positive, etc. How could we handle those cases?
//Peter
Ah, great point Peter! The MBI binary bolb modules ***-have-*** to be aligned in order for the VGA Bios to read them, other wise they are useless blobs of crap, and the VGA Bios is smart enough to know that and reject them.
The VGA BIOS takes care of that. In the coreboot image the MBI binary does not require any special alignment, as access to it is completely handled by our own SMI handler which copies the modules to the addresses requested by the VGA BIOS. No worries here.
Stefan
* Patrick Georgi patrick@georgi-clan.de [101215 12:51]:
Hi,
We have a couple of chipsets in the tree that require external data in CBFS, sometimes with placement requirements (eg. for embedded controllers), and there will be more of that kind to come. Right now, we're adding Kconfig options for each and every of these new files, but that's not a sustainable model.
The patch provides a way for each Makefile.inc to add such files by setting up a couple of variables: # -y can, as usual be used for conditional inclusion cbfs-files-y += filename filename-name := CBFS filename filename-type := CBFS type filename-position := location in CBFS (eg. 0xfff80000)
filename can either be a filename in the current directory (or a relative path from there) or, if that doesn't match a file, a path starting from the tree root. filename in filename-name etc. means the actual filename, for example: cbfs-files-y += mbi.bin mbi.bin-name := mbi.bin mbi.bin-type := 0x80
How does that work with specifying those filenames in Kconfig? Would we just say
cbfs-files-y += $(CONFIG_MBI_FILE)
or some such?
Or should we go away from that and just require people to put the right files in place with the right file names?
Am Mittwoch, 15. Dezember 2010, um 19:50:14 schrieb Stefan Reinauer:
How does that work with specifying those filenames in Kconfig? Would we just say
cbfs-files-y += $(CONFIG_MBI_FILE)
or some such?
Or should we go away from that and just require people to put the right files in place with the right file names?
Should work - the only issue is in which Makefile.inc to put the line above.
The lookup rules are: First relative to the directory of the Makefile.inc that contains the line, then relative to $(top). That's a different behaviour from now (where it only runs relative to the top level directory), but I think a sensible change.
By the way, I think it should be cbfs-files-$(CONFIG_INTEL_MBI) += $(CONFIG_MBI_FILE) ;-)
Patrick
-
Joseph Smith -
Patrick Georgi -
Peter Stuge -
Stefan Reinauer -
Stefan Reinauer