I have no idea if this helps. But there's been discussion of "DRAM settings in DTS" and "where does CARBASE go" and so on, so I think we need to try to document some rules. Here is a first cut.
thanks
ron
Ron, awesome writeup! Thanks for putting it together so well!
On Thu, Feb 28, 2008 at 10:40:41AM -0800, ron minnich wrote:
C code
These control variables are for very low level details of a given image
IMO an image is merely an instance of code. Fairly important distinction since "image" doesn't say where code in question came from.
that should never change, and that should never be visible,
Details belong to components or boards, not images.
as modifying them is very hard to get right, very easy to get wrong, and the right and wrong cases are very hard to distinguish.
Agree.
A simple example is DRAM timing.
Violently disagree.
Some boards may have pre-configured DRAM timing.
Exactly. We agree that which RAM is used and that what the PCB layout looks like are board properties, not component properties.
As we learned from OLPC, it is very, very hard to get this right if DRAMs from multiple vendors are used.
Ouch - people (and Microsoft, Xbox) could have told you before hand. :\
It is quite easy to create timing that is optimized for one vendor that will fail in subtle ways on another vendor -- even if the DRAMs are supposed to be identical! Once the right choice is made, it should almost never be changed.
Right.
In general, control variables that are not intended to be changed, and which can cause undetectable problems, should remain in C code.
Disagree. Or - well - variables controlling _what_ exactly?
I am inclined to include Cache As Ram (CAR) settings here. CAR is very tricky. It's easy to get it slightly wrong, and not know it. I think CAR settings should, once they are right, remain hidden and hard to adjust.
DRAM timing that is fixed belongs here too.
Code should be used for runtime logic. I have a huge issue of principle with code being used as data storage when there exists another good method intended specifically for the purpose.
One example is to make a USB device look like a HID when in fact it is a UPS, because Windows programming is easier with HIDs or whatever.
Another is a compiler/linker that generates code which doesn't use a data segment for storage, but the code segment. Static initialized data is compiled into move instructions that copy data from code at runtime.
(An exception to this which I think is fine is PIC microcontrollers which have no data storage, so code is used for lookup tables. There are surely more exceptions. The point is that there is no option in PICs.)
That's not to say we can't make the details of DRAM timing available to payloads. The dts can be added to by coreboot, and new entries added as coreboot runs. It would be easy to add new entries for the DRAM timing as it was set up, such that these variables would be visible in the run time dts.
I counter with:
If data is worth having in the device tree after boot and can never change at runtime - it belongs in the dts file at build time.
It comes down to: I would like to have zero lines of code in mainboard/
One can argue that's too ambitious for v3 but I don't think it is. v3 is already SO close to achieving this awesome goal:
stuge@n410c ~/co/v3/mainboard/pcengines/alix1c $ ls -l total 36 -rw-r--r-- 1 stuge stuge 1082 Feb 15 03:02 Kconfig -rw-r--r-- 1 stuge stuge 1330 Feb 15 03:02 Makefile -rw-r--r-- 1 stuge stuge 2517 Jan 24 04:14 cmos.layout -rw-r--r-- 1 stuge stuge 1717 Feb 29 01:16 dts -rw-r--r-- 1 stuge stuge 4437 Feb 29 01:16 initram.c -rw-r--r-- 1 stuge stuge 5936 Feb 15 03:02 irq_tables.c -rw-r--r-- 1 stuge stuge 1641 Feb 15 03:02 stage1.c stuge@n410c ~/co/v3/mainboard/pcengines/alix1c $
Granted, K8 will be more interesting than Geode in this regard.
Device Tree Specification (dts)
Oh! I always thought it was Device Tree Source. :)
- include dts node for the components and set their properties
correctly for that mainboard
I would like to have RAM components.
- define the topology of these components, via the node hierarchy
in the mainboard dts
Yes!
- define the paths of each component, e.g. a southbridge might have
path of 0000:0:1.2
RAM might have a path of 0 or 1.
dts should be used for control variables that are per-mainboard.
Yes! I equate this with specifics of the hardware design. What is connected where and in cases where it matters also how it is connected. For example how interrupts are routed to PCI slots, if those four muxed signals on the sobridge are used for COM2 or SATA on this board, and the timing compensation constant that is needed for the particular series resistance to RAM on this board. (for MCs that have such settings.)
The dts variables are currently usually static for a given mainboard, since they reflect the hardware of the mainboard.
Yep.
The device tree compiler compiles dts down to C code for use by the image. Currently this code is initialized structures and a .h file with type definitions.
I agree 100% with you on the dts, but I think your reasoning around the C code goes against it. :\
Kconfig should rarely, if ever, be used to set device options that belong in the dts; or, set very low level build options such as DRAM timing.
It may be tempting to add (invisble) data into Kconfig. This is presumably because it is simple to create dependencies and easy to access in code.
That would be a testament to dts not being easy enough to deal with.
I just had another thought; I could probably be convinced that components don't need a dts. We're currently using dtc as a C preprocessor and there's not much point. But it doesn't do any harm either and I do not feel at all strongly about it.
Makefiles define how an image is built. The makefile structure is rarely changed. Makefiles should never contain settings for a single mainboard. Makefiles contain settings that are global to the system as a whole.
System meaning coreboot build system.
//Peter
On Thu, Feb 28, 2008 at 4:39 PM, Peter Stuge peter@stuge.se wrote:
IMO an image is merely an instance of code. Fairly important distinction since "image" doesn't say where code in question came from.
let's call them targets then.
A simple example is DRAM timing.
Violently disagree.
well, we might never agree on this one, but here is an offer. If you want to prototype a dts which would have this info in it, and post it, let's take a look. I admit I know it can be done.
In general, control variables that are not intended to be changed, and which can cause undetectable problems, should remain in C code.
Disagree. Or - well - variables controlling _what_ exactly?
I introduced a term and did not define it.
How do we define variables, which are pretty special, such as CARBASE and dram timing? What's the name?
One example is to make a USB device look like a HID when in fact it is a UPS, because Windows programming is easier with HIDs or whatever.
point taken.
If data is worth having in the device tree after boot and can never change at runtime - it belongs in the dts file at build time.
But almost all dram timing is unknown at runtime. We should design for that case.
stuge@n410c ~/co/v3/mainboard/pcengines/alix1c $ ls -l total 36 -rw-r--r-- 1 stuge stuge 1082 Feb 15 03:02 Kconfig -rw-r--r-- 1 stuge stuge 1330 Feb 15 03:02 Makefile -rw-r--r-- 1 stuge stuge 2517 Jan 24 04:14 cmos.layout -rw-r--r-- 1 stuge stuge 1717 Feb 29 01:16 dts -rw-r--r-- 1 stuge stuge 4437 Feb 29 01:16 initram.c -rw-r--r-- 1 stuge stuge 5936 Feb 15 03:02 irq_tables.c -rw-r--r-- 1 stuge stuge 1641 Feb 15 03:02 stage1.c stuge@n410c ~/co/v3/mainboard/pcengines/alix1c $
yea, but that code is really tricky due to mainboard inefficiencies and bugs and ... I think squeezing that very last bit out is going to be hard, and in the end, we might end up with the moral equivalent of GNU autoconfig - - sure, there's less code to see, but it's impossible to figure out what's going on.
Some times, 50 lines of special case code are better than 5000 lines of generic incomprehensible code.
Granted, K8 will be more interesting than Geode in this regard.
yes, let's see how k8 goes ...
I would like to have RAM components.
but it will only cover a trivial case -- it won't help most of our targets.
I just had another thought; I could probably be convinced that components don't need a dts. We're currently using dtc as a C preprocessor and there's not much point. But it doesn't do any harm either and I do not feel at all strongly about it.
The did not initially have a dts, but some people strong-armed me into doing it :-) In the end, I really like it.
ron
On 29.02.2008 01:39, Peter Stuge wrote:
Ron, awesome writeup! Thanks for putting it together so well!
Indeed, thanks!
On Thu, Feb 28, 2008 at 10:40:41AM -0800, ron minnich wrote:
In general, control variables that are not intended to be changed, and which can cause undetectable problems, should remain in C code.
Disagree. Or - well - variables controlling _what_ exactly?
If they are constant for all code, simply use one #define or declare the stuff as const for better type checking.
I am inclined to include Cache As Ram (CAR) settings here. CAR is very tricky. It's easy to get it slightly wrong, and not know it. I think CAR settings should, once they are right, remain hidden and hard to adjust.
DRAM timing that is fixed belongs here too.
Code should be used for runtime logic. I have a huge issue of principle with code being used as data storage when there exists another good method intended specifically for the purpose. [...] Another is a compiler/linker that generates code which doesn't use a data segment for storage, but the code segment. Static initialized data is compiled into move instructions that copy data from code at runtime.
Move instructions shouldn't happen if the data is declared const.
dts should be used for control variables that are per-mainboard.
Yes! I equate this with specifics of the hardware design. What is connected where and in cases where it matters also how it is connected. For example how interrupts are routed to PCI slots, if those four muxed signals on the sobridge are used for COM2 or SATA on this board, and the timing compensation constant that is needed for the particular series resistance to RAM on this board. (for MCs that have such settings.)
What about per-mainboard settings which are totally uninteresting to stage2 and payload?
The dts variables are currently usually static for a given mainboard, since they reflect the hardware of the mainboard.
Yep.
The device tree compiler compiles dts down to C code for use by the image. Currently this code is initialized structures and a .h file with type definitions.
I agree 100% with you on the dts, but I think your reasoning around the C code goes against it. :\
Kconfig should rarely, if ever, be used to set device options that belong in the dts; or, set very low level build options such as DRAM timing.
It may be tempting to add (invisble) data into Kconfig. This is presumably because it is simple to create dependencies and easy to access in code.
That would be a testament to dts not being easy enough to deal with.
We already have invisible data in Kconfig because we need to access that data when the device tree is not yet available. CARBASE is an example of that. It is constant for a given subarch, but also used by the generic printk code (and that code is not subarch-specific).
I wrote this in another thread:
We currently abuse Kconfig for settings which should be available before/outside stage2, iow when the device tree is not available. Depending on our design stategy, we may want to generate a subset of the device tree as #defines suitable for inclusion outside stage2.
Regards, Carl-Daniel
On 28/02/08 10:40 -0800, ron minnich wrote:
I have no idea if this helps. But there's been discussion of "DRAM settings in DTS" and "where does CARBASE go" and so on, so I think we need to try to document some rules. Here is a first cut.
Excellent. I feel compelled to chime in on behalf of buildrom. The fewer files that we need to copy and/or modify from buildrom to coreboot-v3 to get what we need, the happier we will be. If we find ourselves constantly changing values in the dts or in Makefiles, then that should be a really good clue that perhaps the value belongs in Kconfig instead. I don't think anybody likes how we're doing configuration now - I look forward to being able to copy a single .config file in place, or, at the worst, being able to construct a single .config file with the changes we need.
Please keep this in mind.
Jordan
On Fri, Feb 29, 2008 at 7:35 AM, Jordan Crouse jordan.crouse@amd.com wrote:
Excellent. I feel compelled to chime in on behalf of buildrom. The fewer files that we need to copy and/or modify from buildrom to coreboot-v3 to get what we need, the happier we will be. If we find ourselves constantly changing values in the dts or in Makefiles, then that should be a really good clue that perhaps the value belongs in Kconfig instead. I don't think anybody likes how we're doing configuration now - I look forward to being able to copy a single .config file in place, or, at the worst, being able to construct a single .config file with the changes we need.
What would you change? Do you mean that the settings in the mainboard dts should be in a .config file?
If we do that, we're going to need to make parsing the dts part of the make menuconfig process. Or did I miss the point?
ron
On 05.03.2008 06:08, ron minnich wrote:
On Fri, Feb 29, 2008 at 7:35 AM, Jordan Crouse jordan.crouse@amd.com wrote:
Excellent. I feel compelled to chime in on behalf of buildrom. The fewer files that we need to copy and/or modify from buildrom to coreboot-v3 to get what we need, the happier we will be. If we find ourselves constantly changing values in the dts or in Makefiles, then that should be a really good clue that perhaps the value belongs in Kconfig instead. I don't think anybody likes how we're doing configuration now - I look forward to being able to copy a single .config file in place, or, at the worst, being able to construct a single .config file with the changes we need.
What would you change? Do you mean that the settings in the mainboard dts should be in a .config file?
If we do that, we're going to need to make parsing the dts part of the make menuconfig process. Or did I miss the point?
I think the dts already has override capabilities, so buildrom should be able to just supply another mainboard-level dts which only contains the settings you want to override in the mainboard dts.
Regards, Carl-Daniel
On 28.02.2008 19:40, ron minnich wrote:
I have no idea if this helps. But there's been discussion of "DRAM settings in DTS" and "where does CARBASE go" and so on, so I think we need to try to document some rules. Here is a first cut.
[...] Kconfig
Kconfig has two uses. First, Kconfig is used to select a mainboard and set build options for the mainboard, e.g. whether compression is used or not. Kconfig can also set runtime parameters, such as baud rate for the console. Kconfig should rarely, if ever, be used to set device options that belong in the dts;
There is a problem with that approach. I disagree that baud rate for the console is different from the address of the serial port when you consider their scope. So it does not make sense to have one in Kconfig and one in the dts. Sure, the baud rate is going to be changed more often, but the amount of change a variable is going to see during configuration is not a consideration of the dts/Kconfig placing decision yet.
The address of the serial port is the poster child for how complicated the interaction between Kconfig and code and dts is: mainboard/pcengines/alix1c/stage1.c:#define SERIAL_IOBASE 0x3f8 superio/winbond/w83627hf/dts: com1io = "0x3f8"; We need the #define in stage1 code because we have to access to the device tree yet. However, we also store the value in the dts. We also could place this in Kconfig (I'm against that) and control this centrally.
How about a way to mark a dts property "must be available as #define in stage1"?
Regards, Carl-Daniel
On Wed, Mar 5, 2008 at 6:51 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
How about a way to mark a dts property "must be available as #define in stage1"?
not sure. statictree.c in stage 1? Then we bring in all that mechanism.
Think think ... not sure
ron
On 05.03.2008 16:53, ron minnich wrote:
On Wed, Mar 5, 2008 at 6:51 AM, Carl-Daniel Hailfinger wrote:
How about a way to mark a dts property "must be available as #define in stage1"?
not sure. statictree.c in stage 1? Then we bring in all that mechanism.
No, not the tree mechanism. I was thinking more a long the lines of another dtc output mode which creates a flat .h file which has no structure, but contains all specially marked dts properties as #define.
Think think ... not sure
I hope I clarified this. statictree.c in stage 1 would indeed be a bad idea.
Regards, Carl-Daniel
On Wed, Mar 5, 2008 at 8:03 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
No, not the tree mechanism. I was thinking more a long the lines of another dtc output mode which creates a flat .h file which has no structure, but contains all specially marked dts properties as #define.
go ahead and propose a syntax, I'm very slow in the morning.
I hope I clarified this. statictree.c in stage 1 would indeed be a bad idea.
Ah, but would it. Look at the cs5536 startup code -- we don't parameterize it yet. We need to.
We could do this: if there is a property in a node called "cpp", it is a list of values in that node that should be emitted as #defines. The names will be prefixed with the pathname of the dts, so if we had cpp = "com1enable"; we get (ugly) SOUTHBRiDGE_AMD_CS5536_COM1ENABLE or some such.
ron
On 05.03.2008 17:10, ron minnich wrote:
On Wed, Mar 5, 2008 at 8:03 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
No, not the tree mechanism. I was thinking more a long the lines of another dtc output mode which creates a flat .h file which has no structure, but contains all specially marked dts properties as #define.
go ahead and propose a syntax, I'm very slow in the morning.
The syntax you propose below would suit me just fine.
I hope I clarified this. statictree.c in stage 1 would indeed be a bad idea.
Ah, but would it. Look at the cs5536 startup code -- we don't parameterize it yet. We need to.
We could do this: if there is a property in a node called "cpp", it is a list of values in that node that should be emitted as #defines. The names will be prefixed with the pathname of the dts, so if we had cpp = "com1enable"; we get (ugly) SOUTHBRiDGE_AMD_CS5536_COM1ENABLE or some such.
Nice one! I really like it.
Regards, Carl-Daniel
We have one really bad offender which totally violates the Kconfig vs. dts distinction:
config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID hex # TODO: Fix PCI ID. default 0x1022 depends BOARD_AMD_NORWICH help Mainboard specific PCI subsystem vendor ID.
config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID hex # TODO: Fix PCI ID. default 0x2323 depends BOARD_AMD_NORWICH help Mainboard specific PCI subsystem device ID.
This absolutely belongs in the dts. The question is: Where in the dts should it live?
Regards, Carl-Daniel
On Wed, Mar 5, 2008 at 3:12 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
We have one really bad offender which totally violates the Kconfig vs. dts distinction:
config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID hex # TODO: Fix PCI ID. default 0x1022 depends BOARD_AMD_NORWICH help Mainboard specific PCI subsystem vendor ID.
config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID hex # TODO: Fix PCI ID. default 0x2323 depends BOARD_AMD_NORWICH help Mainboard specific PCI subsystem device ID.
Right in the mainboard dts? Right at the top.
ron
On 06.03.2008 00:27, ron minnich wrote:
On Wed, Mar 5, 2008 at 3:12 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
We have one really bad offender which totally violates the Kconfig vs. dts distinction:
config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID hex # TODO: Fix PCI ID. default 0x1022 depends BOARD_AMD_NORWICH help Mainboard specific PCI subsystem vendor ID.
config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID hex # TODO: Fix PCI ID. default 0x2323 depends BOARD_AMD_NORWICH help Mainboard specific PCI subsystem device ID.
Right in the mainboard dts? Right at the top.
Done. Patch is compile tested.
Move default mainboard vendor/subsystem from Kconfig to dts.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/Kconfig =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/Kconfig (Revision 632) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/Kconfig (Arbeitskopie) @@ -26,19 +26,3 @@ help This is the name for the Advanced Digital Logic MSM800SEV mainboard.
-config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID - hex - # TODO: Fix the PCI ID. - default 0x1022 - depends BOARD_ADL_MSM800SEV - help - Mainboard specific PCI subsystem vendor ID. - -config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID - hex - # TODO: Fix the PCI ID. - default 0x2323 - depends BOARD_ADL_MSM800SEV - help - Mainboard specific PCI subsystem vendor ID. - Index: LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/dts =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/dts (Revision 632) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/dts (Arbeitskopie) @@ -23,6 +23,8 @@ /{ mainboard-vendor = "Advanced Digital Logic"; mainboard-name = "MSM800SEV"; + mainboard_pci_subsystem_vendor = "0x1022"; + mainboard_pci_subsystem_device = "0x2323"; cpus { }; apic@0 { /config/("northbridge/amd/geodelx/apic"); Index: LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/Kconfig =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/Kconfig (Revision 632) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/Kconfig (Arbeitskopie) @@ -26,19 +26,3 @@ help This is the default mainboard name.
-config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID - hex - # TODO: Fix PCI ID. - default 0x1022 - depends BOARD_AMD_NORWICH - help - Mainboard specific PCI subsystem vendor ID. - -config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID - hex - # TODO: Fix PCI ID. - default 0x2323 - depends BOARD_AMD_NORWICH - help - Mainboard specific PCI subsystem device ID. - Index: LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/dts =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/dts (Revision 632) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/dts (Arbeitskopie) @@ -21,6 +21,8 @@ /{ mainboard-vendor = "AMD"; mainboard-name = "NORWICH"; + mainboard_pci_subsystem_vendor = "0x1022"; + mainboard_pci_subsystem_device = "0x2323"; cpus { }; apic@0 { /config/("northbridge/amd/geodelx/apic"); Index: LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/Kconfig =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/Kconfig (Revision 632) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/Kconfig (Arbeitskopie) @@ -26,19 +26,3 @@ help This is the default mainboard name.
-config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID - hex - # TODO: Fix PCI ID. - default 0x1022 - depends BOARD_ARTECGROUP_DBE61 - help - Mainboard specific PCI subsystem vendor ID. - -config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID - hex - # TODO: Fix PCI ID. - default 0x2323 - depends BOARD_ARTECGROUP_DBE61 - help - Mainboard specific PCI subsystem device ID. - Index: LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/dts =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/dts (Revision 632) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/dts (Arbeitskopie) @@ -75,6 +75,8 @@ /{ mainboard-vendor = "Artec Group"; mainboard-name = "DBE61"; + mainboard_pci_subsystem_vendor = "0x1022"; + mainboard_pci_subsystem_device = "0x2323"; cpus { }; apic@0 { /config/("northbridge/amd/geodelx/apic"); Index: LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/Kconfig =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/Kconfig (Revision 632) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/Kconfig (Arbeitskopie) @@ -27,17 +27,3 @@ help This is the default mainboard name.
-config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID - hex - default 0x15ad - depends BOARD_EMULATION_QEMU_X86 - help - Mainboard specific PCI subsystem vendor ID. - -config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID - hex - default 0x1976 - depends BOARD_EMULATION_QEMU_X86 - help - Mainboard specific PCI subsystem device ID. - Index: LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/dts =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/dts (Revision 632) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/dts (Arbeitskopie) @@ -21,6 +21,8 @@ /{ mainboard-vendor = "Emulation"; mainboard-name = "QEMU x86"; + mainboard_pci_subsystem_vendor = "0x15ad"; + mainboard_pci_subsystem_device = "0x1976"; device_operations = "qemuvga_pci_ops_dev"; cpus {}; domain@0 { Index: LinuxBIOSv3-mainboard_subsystem/device/pci_device.c =================================================================== --- LinuxBIOSv3-mainboard_subsystem/device/pci_device.c (Revision 632) +++ LinuxBIOSv3-mainboard_subsystem/device/pci_device.c (Arbeitskopie) @@ -50,6 +50,7 @@ #if CONFIG_CARDBUS_PLUGIN_SUPPORT == 1 #include <device/cardbus.h> #endif +#include <statictree.h>
u8 pci_moving_config8(struct device *dev, unsigned int reg) { @@ -627,19 +628,17 @@ /* Set the subsystem vendor and device ID for mainboard devices. */ ops = ops_pci(dev);
-#if defined(CONFIG_MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID) && \ - defined(CONFIG_MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID) - if (dev->on_mainboard && ops && ops->set_subsystem) { + if (mainboard_pci_subsystem_set && dev->on_mainboard && ops && + ops->set_subsystem) { printk(BIOS_DEBUG, "%s: Setting subsystem VID/DID to %02x/%02x\n", - dev_path(dev), CONFIG_MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID, - CONFIG_MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID); + dev_path(dev), mainboard_pci_subsystem_vendor, + mainboard_pci_subsystem_device);
- ops->set_subsystem(dev, - CONFIG_MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID, - CONFIG_MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID); + ops->set_subsystem(dev, mainboard_pci_subsystem_vendor, + mainboard_pci_subsystem_device); } -#endif + command = pci_read_config16(dev, PCI_COMMAND); command |= dev->command; command |= (PCI_COMMAND_PARITY + PCI_COMMAND_SERR); // Error check. Index: LinuxBIOSv3-mainboard_subsystem/util/dtc/flattree.c =================================================================== --- LinuxBIOSv3-mainboard_subsystem/util/dtc/flattree.c (Revision 632) +++ LinuxBIOSv3-mainboard_subsystem/util/dtc/flattree.c (Arbeitskopie) @@ -1304,7 +1304,7 @@ extern char *code; struct node *next; extern struct node *first_node; - int found_mainboard_vendor = 0, found_mainboard_partnumber = 0; + int found_mainboard_vendor = 0, found_mainboard_partnumber = 0, found_mainboard_subsys = 0;
labeltree(bi->dt);
@@ -1337,6 +1337,14 @@ found_mainboard_partnumber = 1; fprintf(f, "const char *mainboard_part_number = "%s";\n", prop->val.val); } + if (streq(prop->name, "mainboard_pci_subsystem_vendor")){ + found_mainboard_subsys++; + fprintf(f, "const u16 mainboard_pci_subsystem_vendor = %s;\n", prop->val.val); + } + if (streq(prop->name, "mainboard_pci_subsystem_device")){ + found_mainboard_subsys++; + fprintf(f, "const u16 mainboard_pci_subsystem_device = %s;\n", prop->val.val); + } }
if (! found_mainboard_vendor){ @@ -1349,6 +1357,20 @@ "Please add one." "(and make sure there is a mainboard-vendor property too"); } + switch (found_mainboard_subsys) { + case 0: + fprintf(f, "const u16 mainboard_pci_subsystem_vendor = 0;\n"); + fprintf(f, "const u16 mainboard_pci_subsystem_device = 0;\n"); + fprintf(f, "const char mainboard_pci_subsystem_set = 0;\n"); + case 1: + die("There is only one of mainboard_pci_subsystem_vendor and " + "mainboard_pci_subsystem_device properties in the root. " + "Please add the other one or remove the existing one."); + break; + case 2: + fprintf(f, "const char mainboard_pci_subsystem_set = 1;\n"); + break; + }
/* emit the code, if any */ @@ -1392,6 +1414,9 @@ /* emit any includes that we need -- TODO: ONLY ONCE PER TYPE*/ fprintf(f, "#include <device/device.h>\n#include <device/pci.h>\n"); fprintf(f, "extern const char *mainboard_vendor, *mainboard_part_number;\n"); + fprintf(f, "extern const u16 mainboard_pci_subsystem_vendor;\n"); + fprintf(f, "extern const u16 mainboard_pci_subsystem_device;\n"); + fprintf(f, "extern const char mainboard_pci_subsystem_set;\n"); flatten_tree_emit_includes(bi->dt, &coreboot_emitter, f, &strbuf, vi);
flatten_tree_emit_structdecls(bi->dt, &coreboot_emitter, f, &strbuf, vi);
On 06.03.2008 01:20, Carl-Daniel Hailfinger wrote:
Move default mainboard vendor/subsystem from Kconfig to dts.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
And attached for Ron.
Regards, Carl-Daniel
OK, the thought occurs:
/number,number{ }
and number, number is the thing we want.
Not sure. Just wondering. It fits the scheme.
ron
On 06.03.2008 02:00, ron minnich wrote:
OK, the thought occurs:
/number,number{ }
and number, number is the thing we want.
Not sure. Just wondering. It fits the scheme.
It fits the scheme at first glance, but the sub{vendor,device} stuff is pci specific and having it in a place where the identifier/address would normally be makes me uncomfortable.
Regards, Carl-Daniel
On Thu, Mar 6, 2008 at 4:07 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
It fits the scheme at first glance, but the sub{vendor,device} stuff is pci specific and having it in a place where the identifier/address would normally be makes me uncomfortable.
I think I agree. I'll test your patch on its own, before I test the other one.
Thanks
ron
On 07.03.2008 01:32, ron minnich wrote:
On Thu, Mar 6, 2008 at 4:07 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
It fits the scheme at first glance, but the sub{vendor,device} stuff is pci specific and having it in a place where the identifier/address would normally be makes me uncomfortable.
I think I agree. I'll test your patch on its own, before I test the other one.
If possible, I'd like to delay committing the patch a bit until I'm sure about the final design. Testing would be great, though.
Regards, Carl-Daniel
On 07.03.2008 01:53, Carl-Daniel Hailfinger wrote:
On 07.03.2008 01:32, ron minnich wrote:
On Thu, Mar 6, 2008 at 4:07 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
It fits the scheme at first glance, but the sub{vendor,device} stuff is pci specific and having it in a place where the identifier/address would normally be makes me uncomfortable.
I think I agree. I'll test your patch on its own, before I test the other one.
If possible, I'd like to delay committing the patch a bit until I'm sure about the final design. Testing would be great, though.
Redesigned, more efficient and more readable patch follows.
Move default mainboard vendor/subsystem from Kconfig to dts. Compile tested including boundary cases.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/Kconfig =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/Kconfig (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/Kconfig (Arbeitskopie) @@ -26,19 +26,3 @@ help This is the name for the Advanced Digital Logic MSM800SEV mainboard.
-config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID - hex - # TODO: Fix the PCI ID. - default 0x1022 - depends BOARD_ADL_MSM800SEV - help - Mainboard specific PCI subsystem vendor ID. - -config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID - hex - # TODO: Fix the PCI ID. - default 0x2323 - depends BOARD_ADL_MSM800SEV - help - Mainboard specific PCI subsystem vendor ID. - Index: LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/dts =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/dts (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/dts (Arbeitskopie) @@ -23,6 +23,8 @@ /{ mainboard-vendor = "Advanced Digital Logic"; mainboard-name = "MSM800SEV"; + mainboard_pci_subsystem_vendor = "0x1022"; + mainboard_pci_subsystem_device = "0x2323"; cpus { }; apic@0 { /config/("northbridge/amd/geodelx/apic"); Index: LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/Kconfig =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/Kconfig (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/Kconfig (Arbeitskopie) @@ -26,19 +26,3 @@ help This is the default mainboard name.
-config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID - hex - # TODO: Fix PCI ID. - default 0x1022 - depends BOARD_AMD_NORWICH - help - Mainboard specific PCI subsystem vendor ID. - -config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID - hex - # TODO: Fix PCI ID. - default 0x2323 - depends BOARD_AMD_NORWICH - help - Mainboard specific PCI subsystem device ID. - Index: LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/dts =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/dts (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/dts (Arbeitskopie) @@ -21,6 +21,8 @@ /{ mainboard-vendor = "AMD"; mainboard-name = "NORWICH"; + mainboard_pci_subsystem_vendor = "0x1022"; + mainboard_pci_subsystem_device = "0x2323"; cpus { }; apic@0 { /config/("northbridge/amd/geodelx/apic"); Index: LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/Kconfig =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/Kconfig (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/Kconfig (Arbeitskopie) @@ -26,19 +26,3 @@ help This is the default mainboard name.
-config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID - hex - # TODO: Fix PCI ID. - default 0x1022 - depends BOARD_ARTECGROUP_DBE61 - help - Mainboard specific PCI subsystem vendor ID. - -config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID - hex - # TODO: Fix PCI ID. - default 0x2323 - depends BOARD_ARTECGROUP_DBE61 - help - Mainboard specific PCI subsystem device ID. - Index: LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/dts =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/dts (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/dts (Arbeitskopie) @@ -75,6 +75,8 @@ /{ mainboard-vendor = "Artec Group"; mainboard-name = "DBE61"; + mainboard_pci_subsystem_vendor = "0x1022"; + mainboard_pci_subsystem_device = "0x2323"; cpus { }; apic@0 { /config/("northbridge/amd/geodelx/apic"); Index: LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/Kconfig =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/Kconfig (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/Kconfig (Arbeitskopie) @@ -27,17 +27,3 @@ help This is the default mainboard name.
-config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID - hex - default 0x15ad - depends BOARD_EMULATION_QEMU_X86 - help - Mainboard specific PCI subsystem vendor ID. - -config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID - hex - default 0x1976 - depends BOARD_EMULATION_QEMU_X86 - help - Mainboard specific PCI subsystem device ID. - Index: LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/dts =================================================================== --- LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/dts (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/dts (Arbeitskopie) @@ -21,6 +21,8 @@ /{ mainboard-vendor = "Emulation"; mainboard-name = "QEMU x86"; + mainboard_pci_subsystem_vendor = "0x15ad"; + mainboard_pci_subsystem_device = "0x1976"; device_operations = "qemuvga_pci_ops_dev"; cpus {}; domain@0 { Index: LinuxBIOSv3-mainboard_subsystem/device/pci_device.c =================================================================== --- LinuxBIOSv3-mainboard_subsystem/device/pci_device.c (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/device/pci_device.c (Arbeitskopie) @@ -50,6 +50,7 @@ #if CONFIG_CARDBUS_PLUGIN_SUPPORT == 1 #include <device/cardbus.h> #endif +#include <statictree.h>
u8 pci_moving_config8(struct device *dev, unsigned int reg) { @@ -627,19 +628,18 @@ /* Set the subsystem vendor and device ID for mainboard devices. */ ops = ops_pci(dev);
-#if defined(CONFIG_MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID) && \ - defined(CONFIG_MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID) +#ifdef HAVE_MAINBOARD_PCI_SUBSYSTEM_ID if (dev->on_mainboard && ops && ops->set_subsystem) { printk(BIOS_DEBUG, "%s: Setting subsystem VID/DID to %02x/%02x\n", - dev_path(dev), CONFIG_MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID, - CONFIG_MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID); + dev_path(dev), mainboard_pci_subsystem_vendor, + mainboard_pci_subsystem_device);
- ops->set_subsystem(dev, - CONFIG_MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID, - CONFIG_MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID); + ops->set_subsystem(dev, mainboard_pci_subsystem_vendor, + mainboard_pci_subsystem_device); } #endif + command = pci_read_config16(dev, PCI_COMMAND); command |= dev->command; command |= (PCI_COMMAND_PARITY + PCI_COMMAND_SERR); // Error check. Index: LinuxBIOSv3-mainboard_subsystem/util/dtc/flattree.c =================================================================== --- LinuxBIOSv3-mainboard_subsystem/util/dtc/flattree.c (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/util/dtc/flattree.c (Arbeitskopie) @@ -1328,7 +1328,7 @@ extern char *code; struct node *next; extern struct node *first_node; - int found_mainboard_vendor = 0, found_mainboard_partnumber = 0; + int found_mainboard_vendor = 0, found_mainboard_partnumber = 0, found_mainboard_subsys = 0;
labeltree(bi->dt);
@@ -1361,6 +1361,14 @@ found_mainboard_partnumber = 1; fprintf(f, "const char *mainboard_part_number = "%s";\n", prop->val.val); } + if (streq(prop->name, "mainboard_pci_subsystem_vendor")){ + found_mainboard_subsys++; + fprintf(f, "const u16 mainboard_pci_subsystem_vendor = %s;\n", prop->val.val); + } + if (streq(prop->name, "mainboard_pci_subsystem_device")){ + found_mainboard_subsys++; + fprintf(f, "const u16 mainboard_pci_subsystem_device = %s;\n", prop->val.val); + } }
if (! found_mainboard_vendor){ @@ -1374,6 +1382,17 @@ "(and make sure there is a mainboard-vendor property too"); }
+ switch (found_mainboard_subsys) { + case 0: + break; + case 1: + die("There is only one of mainboard_pci_subsystem_vendor and " + "mainboard_pci_subsystem_device properties in the root. " + "Please add the other one or remove the existing one."); + break; + case 2: + break; + }
/* emit the code, if any */ if (code) @@ -1395,6 +1414,8 @@ char *symprefix = "dt"; extern char *code; struct node *next; + struct property *prop; + int found_mainboard_subsys = 0; extern struct node *first_node;
labeltree(bi->dt); @@ -1416,6 +1437,31 @@ /* emit any includes that we need -- TODO: ONLY ONCE PER TYPE*/ fprintf(f, "#include <device/device.h>\n#include <device/pci.h>\n"); fprintf(f, "extern const char *mainboard_vendor, *mainboard_part_number;\n"); + + for_each_property(bi->dt, prop) { + if (streq(prop->name, "mainboard_pci_subsystem_vendor")){ + found_mainboard_subsys++; + } + if (streq(prop->name, "mainboard_pci_subsystem_device")){ + found_mainboard_subsys++; + } + } + + switch (found_mainboard_subsys) { + case 0: + break; + case 1: + die("There is only one of mainboard_pci_subsystem_vendor and " + "mainboard_pci_subsystem_device properties in the root. " + "Please add the other one or remove the existing one."); + break; + case 2: + fprintf(f, "#define HAVE_MAINBOARD_PCI_SUBSYSTEM_ID\n"); + fprintf(f, "extern const u16 mainboard_pci_subsystem_vendor;\n"); + fprintf(f, "extern const u16 mainboard_pci_subsystem_device;\n"); + break; + } + flatten_tree_emit_includes(bi->dt, &coreboot_emitter, f, &strbuf, vi);
flatten_tree_emit_structdecls(bi->dt, &coreboot_emitter, f, &strbuf, vi);
Can someone please review this? Besides moving mainboard-specific hardware configuration from Kconfig to DTS (where it belongs), it also reduces the amount of per-mainboard code in the tree.
Regards, Carl-Daniel
On 05.07.2008 16:35, Carl-Daniel Hailfinger wrote:
Move default mainboard vendor/subsystem from Kconfig to dts. Compile tested including boundary cases.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/Kconfig
--- LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/Kconfig (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/Kconfig (Arbeitskopie) @@ -26,19 +26,3 @@ help This is the name for the Advanced Digital Logic MSM800SEV mainboard.
-config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID
- hex
- # TODO: Fix the PCI ID.
- default 0x1022
- depends BOARD_ADL_MSM800SEV
- help
Mainboard specific PCI subsystem vendor ID.-config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID
- hex
- # TODO: Fix the PCI ID.
- default 0x2323
- depends BOARD_ADL_MSM800SEV
- help
Mainboard specific PCI subsystem vendor ID.Index: LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/dts
--- LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/dts (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/adl/msm800sev/dts (Arbeitskopie) @@ -23,6 +23,8 @@ /{ mainboard-vendor = "Advanced Digital Logic"; mainboard-name = "MSM800SEV";
- mainboard_pci_subsystem_vendor = "0x1022";
- mainboard_pci_subsystem_device = "0x2323"; cpus { }; apic@0 { /config/("northbridge/amd/geodelx/apic");
Index: LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/Kconfig
--- LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/Kconfig (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/Kconfig (Arbeitskopie) @@ -26,19 +26,3 @@ help This is the default mainboard name.
-config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID
- hex
- # TODO: Fix PCI ID.
- default 0x1022
- depends BOARD_AMD_NORWICH
- help
Mainboard specific PCI subsystem vendor ID.-config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID
- hex
- # TODO: Fix PCI ID.
- default 0x2323
- depends BOARD_AMD_NORWICH
- help
Mainboard specific PCI subsystem device ID.Index: LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/dts
--- LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/dts (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/amd/norwich/dts (Arbeitskopie) @@ -21,6 +21,8 @@ /{ mainboard-vendor = "AMD"; mainboard-name = "NORWICH";
- mainboard_pci_subsystem_vendor = "0x1022";
- mainboard_pci_subsystem_device = "0x2323"; cpus { }; apic@0 { /config/("northbridge/amd/geodelx/apic");
Index: LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/Kconfig
--- LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/Kconfig (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/Kconfig (Arbeitskopie) @@ -26,19 +26,3 @@ help This is the default mainboard name.
-config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID
- hex
- # TODO: Fix PCI ID.
- default 0x1022
- depends BOARD_ARTECGROUP_DBE61
- help
Mainboard specific PCI subsystem vendor ID.-config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID
- hex
- # TODO: Fix PCI ID.
- default 0x2323
- depends BOARD_ARTECGROUP_DBE61
- help
Mainboard specific PCI subsystem device ID.Index: LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/dts
--- LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/dts (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/artecgroup/dbe61/dts (Arbeitskopie) @@ -75,6 +75,8 @@ /{ mainboard-vendor = "Artec Group"; mainboard-name = "DBE61";
- mainboard_pci_subsystem_vendor = "0x1022";
- mainboard_pci_subsystem_device = "0x2323"; cpus { }; apic@0 { /config/("northbridge/amd/geodelx/apic");
Index: LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/Kconfig
--- LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/Kconfig (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/Kconfig (Arbeitskopie) @@ -27,17 +27,3 @@ help This is the default mainboard name.
-config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID
- hex
- default 0x15ad
- depends BOARD_EMULATION_QEMU_X86
- help
Mainboard specific PCI subsystem vendor ID.-config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID
- hex
- default 0x1976
- depends BOARD_EMULATION_QEMU_X86
- help
Mainboard specific PCI subsystem device ID.Index: LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/dts
--- LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/dts (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/mainboard/emulation/qemu-x86/dts (Arbeitskopie) @@ -21,6 +21,8 @@ /{ mainboard-vendor = "Emulation"; mainboard-name = "QEMU x86";
- mainboard_pci_subsystem_vendor = "0x15ad";
- mainboard_pci_subsystem_device = "0x1976"; device_operations = "qemuvga_pci_ops_dev"; cpus {}; domain@0 {
Index: LinuxBIOSv3-mainboard_subsystem/device/pci_device.c
--- LinuxBIOSv3-mainboard_subsystem/device/pci_device.c (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/device/pci_device.c (Arbeitskopie) @@ -50,6 +50,7 @@ #if CONFIG_CARDBUS_PLUGIN_SUPPORT == 1 #include <device/cardbus.h> #endif +#include <statictree.h>
u8 pci_moving_config8(struct device *dev, unsigned int reg) { @@ -627,19 +628,18 @@ /* Set the subsystem vendor and device ID for mainboard devices. */ ops = ops_pci(dev);
-#if defined(CONFIG_MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID) && \
- defined(CONFIG_MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID)
+#ifdef HAVE_MAINBOARD_PCI_SUBSYSTEM_ID if (dev->on_mainboard && ops && ops->set_subsystem) { printk(BIOS_DEBUG, "%s: Setting subsystem VID/DID to %02x/%02x\n",
dev_path(dev), CONFIG_MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID,CONFIG_MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID);
dev_path(dev), mainboard_pci_subsystem_vendor,mainboard_pci_subsystem_device);
ops->set_subsystem(dev,CONFIG_MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID,CONFIG_MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID);
ops->set_subsystem(dev, mainboard_pci_subsystem_vendor,mainboard_pci_subsystem_device);#endif
- command = pci_read_config16(dev, PCI_COMMAND); command |= dev->command; command |= (PCI_COMMAND_PARITY + PCI_COMMAND_SERR); // Error check.
Index: LinuxBIOSv3-mainboard_subsystem/util/dtc/flattree.c
--- LinuxBIOSv3-mainboard_subsystem/util/dtc/flattree.c (Revision 692) +++ LinuxBIOSv3-mainboard_subsystem/util/dtc/flattree.c (Arbeitskopie) @@ -1328,7 +1328,7 @@ extern char *code; struct node *next; extern struct node *first_node;
- int found_mainboard_vendor = 0, found_mainboard_partnumber = 0;
int found_mainboard_vendor = 0, found_mainboard_partnumber = 0, found_mainboard_subsys = 0;
labeltree(bi->dt);
@@ -1361,6 +1361,14 @@ found_mainboard_partnumber = 1; fprintf(f, "const char *mainboard_part_number = "%s";\n", prop->val.val); }
if (streq(prop->name, "mainboard_pci_subsystem_vendor")){found_mainboard_subsys++;fprintf(f, "const u16 mainboard_pci_subsystem_vendor = %s;\n", prop->val.val);}if (streq(prop->name, "mainboard_pci_subsystem_device")){found_mainboard_subsys++;fprintf(f, "const u16 mainboard_pci_subsystem_device = %s;\n", prop->val.val);}}
if (! found_mainboard_vendor){
@@ -1374,6 +1382,17 @@ "(and make sure there is a mainboard-vendor property too"); }
switch (found_mainboard_subsys) {
case 0:
break;case 1:
die("There is only one of mainboard_pci_subsystem_vendor and ""mainboard_pci_subsystem_device properties in the root. ""Please add the other one or remove the existing one.");break;case 2:
break;}
/* emit the code, if any */ if (code)
@@ -1395,6 +1414,8 @@ char *symprefix = "dt"; extern char *code; struct node *next;
struct property *prop;
int found_mainboard_subsys = 0; extern struct node *first_node;
labeltree(bi->dt);
@@ -1416,6 +1437,31 @@ /* emit any includes that we need -- TODO: ONLY ONCE PER TYPE*/ fprintf(f, "#include <device/device.h>\n#include <device/pci.h>\n"); fprintf(f, "extern const char *mainboard_vendor, *mainboard_part_number;\n");
for_each_property(bi->dt, prop) {
if (streq(prop->name, "mainboard_pci_subsystem_vendor")){found_mainboard_subsys++;}if (streq(prop->name, "mainboard_pci_subsystem_device")){found_mainboard_subsys++;}}
switch (found_mainboard_subsys) {
case 0:
break;case 1:
die("There is only one of mainboard_pci_subsystem_vendor and ""mainboard_pci_subsystem_device properties in the root. ""Please add the other one or remove the existing one.");break;case 2:
fprintf(f, "#define HAVE_MAINBOARD_PCI_SUBSYSTEM_ID\n");fprintf(f, "extern const u16 mainboard_pci_subsystem_vendor;\n");fprintf(f, "extern const u16 mainboard_pci_subsystem_device;\n");break;}
flatten_tree_emit_includes(bi->dt, &coreboot_emitter, f, &strbuf, vi);
flatten_tree_emit_structdecls(bi->dt, &coreboot_emitter, f, &strbuf, vi);
there's something about the patches you post that leaves me with a gmail blob that patch can not use. If you send me an attachment I will apply and test.
thanks
ron
Tested on dbe62. Works fine. Nice patch!
Acked-by: Ronald G. Minnich rminnich@gmail.com
On 09.07.2008 17:55, ron minnich wrote:
Tested on dbe62. Works fine. Nice patch!
Acked-by: Ronald G. Minnich rminnich@gmail.com
Thanks for testing! committed in r693.
Regards, Carl-Daniel
On Sat, Jul 05, 2008 at 04:35:29PM +0200, Carl-Daniel Hailfinger wrote:
mainboard-vendor = "Emulation"; mainboard-name = "QEMU x86";
- mainboard_pci_subsystem_vendor = "0x15ad";
- mainboard_pci_subsystem_device = "0x1976"; device_operations = "qemuvga_pci_ops_dev";
The dts syntax is somewhat inconsistent, sometimes - sometimes _. Is there a rule?
And why are hex numbers specified as strings? That is silly.
//Peter
Peter Stuge wrote:
On Sat, Jul 05, 2008 at 04:35:29PM +0200, Carl-Daniel Hailfinger wrote:
mainboard-vendor = "Emulation"; mainboard-name = "QEMU x86";
- mainboard_pci_subsystem_vendor = "0x15ad";
- mainboard_pci_subsystem_device = "0x1976";
Also, the assumption that there is a single subsystem ID for all devices on a mainboard is wrong. There is no such ID per mainboard. Each device can have a different subsystem ID. I suggest that we find a better solution for that in v3.
On 10.07.2008 16:32, Stefan Reinauer wrote:
Peter Stuge wrote:
On Sat, Jul 05, 2008 at 04:35:29PM +0200, Carl-Daniel Hailfinger wrote:
mainboard-vendor = "Emulation"; mainboard-name = "QEMU x86";
- mainboard_pci_subsystem_vendor = "0x15ad";
- mainboard_pci_subsystem_device = "0x1976";
Also, the assumption that there is a single subsystem ID for all devices on a mainboard is wrong. There is no such ID per mainboard. Each device can have a different subsystem ID. I suggest that we find a better solution for that in v3.
True. This patch was intended to move subsystem IDs to the DTS. Future patches to make the subsystem ID a per-device property are definitely the way to go.
Regards, Carl-Daniel
On 10.07.2008 14:01, Peter Stuge wrote:
On Sat, Jul 05, 2008 at 04:35:29PM +0200, Carl-Daniel Hailfinger wrote:
mainboard-vendor = "Emulation"; mainboard-name = "QEMU x86";
- mainboard_pci_subsystem_vendor = "0x15ad";
- mainboard_pci_subsystem_device = "0x1976"; device_operations = "qemuvga_pci_ops_dev";
The dts syntax is somewhat inconsistent, sometimes - sometimes _. Is there a rule?
My goal was to have the same name in the dts which is also used as variable name in C code. The existing code has a somewhat peculiar mapping from organic growth over time: mainboard-vendor -> const char *mainboard_vendor mainboard-name -> const char *mainboard_part_number If we can agree on whether the variable should be called "mainboard_name" or "mainboard_part_number", I'd be willing to prepare a patch.
And why are hex numbers specified as strings? That is silly.
Honestly, this is one of my first interactions with the device tree code and I just adapted existing code. I tried specifying them as numbers, but I got a syntax error back from dtc. You may have noticed that all numbers in all dts files are specified as strings if they are values of properties. Someone more familiar with the DTS spec may want to comment on that.
Regards, Carl-Daniel
On 10.07.2008 20:48, Carl-Daniel Hailfinger wrote:
On 10.07.2008 14:01, Peter Stuge wrote:
On Sat, Jul 05, 2008 at 04:35:29PM +0200, Carl-Daniel Hailfinger wrote:
mainboard-vendor = "Emulation"; mainboard-name = "QEMU x86";
- mainboard_pci_subsystem_vendor = "0x15ad";
- mainboard_pci_subsystem_device = "0x1976"; device_operations = "qemuvga_pci_ops_dev";
The dts syntax is somewhat inconsistent, sometimes - sometimes _. Is there a rule?
My goal was to have the same name in the dts which is also used as variable name in C code. The existing code has a somewhat peculiar mapping from organic growth over time: mainboard-vendor -> const char *mainboard_vendor mainboard-name -> const char *mainboard_part_number If we can agree on whether the variable should be called "mainboard_name" or "mainboard_part_number", I'd be willing to prepare a patch.
OK, here comes a patch which makes mainboard-vendor naming more consistent. mainboard-name naming has been postponed because it's not clear what the real name should be.
Generated code is identical to the state before the patch.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: corebootv3-mainboard_vendor/mainboard/adl/msm800sev/dts =================================================================== --- corebootv3-mainboard_vendor/mainboard/adl/msm800sev/dts (Revision 693) +++ corebootv3-mainboard_vendor/mainboard/adl/msm800sev/dts (Arbeitskopie) @@ -21,7 +21,7 @@ */
/{ - mainboard-vendor = "Advanced Digital Logic"; + mainboard_vendor = "Advanced Digital Logic"; mainboard-name = "MSM800SEV"; mainboard_pci_subsystem_vendor = "0x1022"; mainboard_pci_subsystem_device = "0x2323"; Index: corebootv3-mainboard_vendor/mainboard/amd/norwich/dts =================================================================== --- corebootv3-mainboard_vendor/mainboard/amd/norwich/dts (Revision 693) +++ corebootv3-mainboard_vendor/mainboard/amd/norwich/dts (Arbeitskopie) @@ -19,7 +19,7 @@ */
/{ - mainboard-vendor = "AMD"; + mainboard_vendor = "AMD"; mainboard-name = "NORWICH"; mainboard_pci_subsystem_vendor = "0x1022"; mainboard_pci_subsystem_device = "0x2323"; Index: corebootv3-mainboard_vendor/mainboard/amd/db800/dts =================================================================== --- corebootv3-mainboard_vendor/mainboard/amd/db800/dts (Revision 693) +++ corebootv3-mainboard_vendor/mainboard/amd/db800/dts (Arbeitskopie) @@ -19,7 +19,7 @@ */
/{ - mainboard-vendor = "AMD"; + mainboard_vendor = "AMD"; mainboard-name = "DB800"; mainboard_pci_subsystem_vendor = "0x1022"; mainboard_pci_subsystem_device = "0x2323"; Index: corebootv3-mainboard_vendor/mainboard/artecgroup/dbe61/dts =================================================================== --- corebootv3-mainboard_vendor/mainboard/artecgroup/dbe61/dts (Revision 693) +++ corebootv3-mainboard_vendor/mainboard/artecgroup/dbe61/dts (Arbeitskopie) @@ -73,7 +73,7 @@ */
/{ - mainboard-vendor = "Artec Group"; + mainboard_vendor = "Artec Group"; mainboard-name = "DBE61"; mainboard_pci_subsystem_vendor = "0x1022"; mainboard_pci_subsystem_device = "0x2323"; Index: corebootv3-mainboard_vendor/mainboard/artecgroup/dbe62/dts =================================================================== --- corebootv3-mainboard_vendor/mainboard/artecgroup/dbe62/dts (Revision 693) +++ corebootv3-mainboard_vendor/mainboard/artecgroup/dbe62/dts (Arbeitskopie) @@ -19,7 +19,7 @@ */
/{ - mainboard-vendor = "Artec"; + mainboard_vendor = "Artec"; mainboard-name = "DBE62"; cpus { }; apic@0 { Index: corebootv3-mainboard_vendor/mainboard/pcengines/alix1c/dts =================================================================== --- corebootv3-mainboard_vendor/mainboard/pcengines/alix1c/dts (Revision 693) +++ corebootv3-mainboard_vendor/mainboard/pcengines/alix1c/dts (Arbeitskopie) @@ -19,7 +19,7 @@ */
/{ - mainboard-vendor = "PC Engines"; + mainboard_vendor = "PC Engines"; mainboard-name = "ALIX1.C"; cpus { }; apic@0 { Index: corebootv3-mainboard_vendor/mainboard/pcengines/alix2c3/dts =================================================================== --- corebootv3-mainboard_vendor/mainboard/pcengines/alix2c3/dts (Revision 693) +++ corebootv3-mainboard_vendor/mainboard/pcengines/alix2c3/dts (Arbeitskopie) @@ -19,7 +19,7 @@ */
/{ - mainboard-vendor = "PC Engines"; + mainboard_vendor = "PC Engines"; mainboard-name = "ALIX.2C3"; cpus { }; apic@0 { Index: corebootv3-mainboard_vendor/mainboard/emulation/qemu-x86/dts =================================================================== --- corebootv3-mainboard_vendor/mainboard/emulation/qemu-x86/dts (Revision 693) +++ corebootv3-mainboard_vendor/mainboard/emulation/qemu-x86/dts (Arbeitskopie) @@ -19,7 +19,7 @@ */
/{ - mainboard-vendor = "Emulation"; + mainboard_vendor = "Emulation"; mainboard-name = "QEMU x86"; mainboard_pci_subsystem_vendor = "0x15ad"; mainboard_pci_subsystem_device = "0x1976"; Index: corebootv3-mainboard_vendor/util/dtc/flattree.c =================================================================== --- corebootv3-mainboard_vendor/util/dtc/flattree.c (Revision 693) +++ corebootv3-mainboard_vendor/util/dtc/flattree.c (Arbeitskopie) @@ -1353,7 +1353,7 @@
/* special for the root. Emit the names for the mainboard vendor and part # */ for_each_property(bi->dt, prop) { - if (streq(prop->name, "mainboard-vendor")){ + if (streq(prop->name, "mainboard_vendor")){ found_mainboard_vendor = 1; fprintf(f, "const char *mainboard_vendor = "%s";\n", prop->val.val); } @@ -1372,14 +1372,14 @@ }
if (! found_mainboard_vendor){ - die("There is no mainboard-vendor property in the root. Please add one." + die("There is no mainboard_vendor property in the root. Please add one." "(and make sure there is a mainboard-name property too"); }
if (! found_mainboard_partnumber){ die("There is no mainboard-name property in the root. " "Please add one." - "(and make sure there is a mainboard-vendor property too"); + "(and make sure there is a mainboard_vendor property too"); }
switch (found_mainboard_subsys) {
On Fri, Jul 11, 2008 at 03:06:43AM +0200, Carl-Daniel Hailfinger wrote:
My goal was to have the same name in the dts which is also used as variable name in C code. The existing code has a somewhat peculiar mapping from organic growth over time: mainboard-vendor -> const char *mainboard_vendor mainboard-name -> const char *mainboard_part_number If we can agree on whether the variable should be called "mainboard_name" or "mainboard_part_number", I'd be willing to prepare a patch.
mainboard_name sounds good.
OK, here comes a patch which makes mainboard-vendor naming more consistent. mainboard-name naming has been postponed because it's not clear what the real name should be.
Generated code is identical to the state before the patch.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Uwe Hermann uwe@hermann-uwe.de
Build-tested with a few targets.
Uwe.
On 11.07.2008 13:59, Uwe Hermann wrote:
On Fri, Jul 11, 2008 at 03:06:43AM +0200, Carl-Daniel Hailfinger wrote:
My goal was to have the same name in the dts which is also used as variable name in C code. The existing code has a somewhat peculiar mapping from organic growth over time: mainboard-vendor -> const char *mainboard_vendor mainboard-name -> const char *mainboard_part_number If we can agree on whether the variable should be called "mainboard_name" or "mainboard_part_number", I'd be willing to prepare a patch.
mainboard_name sounds good.
Indeed. The only problem is that this sort of conflicts with CONFIG_MAINBOARD_NAME, but that Kconfig variable is misnamed anyway (it contains the mainboard directory).
OK, here comes a patch which makes mainboard-vendor naming more consistent. mainboard-name naming has been postponed because it's not clear what the real name should be.
Generated code is identical to the state before the patch.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Uwe Hermann uwe@hermann-uwe.de
Build-tested with a few targets.
Thanks, r694.
Regards, Carl-Daniel
Following "new to coreboot", briefly, coreboot.rom compiled OK but uniflash filed to load it; however it was able to reload my original saved image.
Leaving uniflash for now, I can get an OS running here, enough to load flashrom from "utilities" in the download- but I note from http://www.coreboot.org/MSI_MS-6178_Build_Tutorial that flashrom is a "no". Does that mean that the coreboot.rom was written to the chip away from the MS6178 board & that flashrom cannot flash the chip from the board itself? Thanks-Philip
On Fri, Jul 11, 2008 at 03:40:49PM +0100, Philip Aston wrote:
Following "new to coreboot", briefly, coreboot.rom compiled OK but uniflash filed to load it; however it was able to reload my original saved image.
Leaving uniflash for now, I can get an OS running here, enough to load flashrom from "utilities" in the download- but I note from http://www.coreboot.org/MSI_MS-6178_Build_Tutorial that flashrom is a "no". Does that mean that the coreboot.rom was written to the chip away from the MS6178 board & that flashrom cannot flash the chip from the board itself?
Flashrom might be able to read the current contents of the ROM chip on this board, but it will not be able to _write_ a correct image onto the chip. There's likely some board-specific write-enable required, but this is not yet supported and also non-trivial to find out (never documented).
Uwe.
-
Carl-Daniel Hailfinger -
Jordan Crouse -
Peter Stuge -
Philip Aston -
ron minnich -
Stefan Reinauer -
Uwe Hermann