Hi all,
in Nov 2008 (around rev. 3760) I had done Coreboot support for 2 of our boards (LiPPERT RoadRunner-LX and SpaceRunner-LX). Now I'd like to add support for 2 more, based on the code for the previous ones as the hardware is quite similar.
However in the past 2 years / 2000 revs Coreboot has evolved quite a lot! Even though SpaceRunner support still compiles, it doesn't quite seem to work as it did back then. Looks like I have to try and bring the 2 older board's code up to scratch first before I can base anything on it.
The old code defined a config byte (for some GPIOs) in struct mainboard_lippert_spacerunner_lx_config (chip.h), which used to be set in Config.lb with a simple register "..." = "..." statement and used in mainboard.c's init() by casting dev->chip_info to struct mainboard_lippert_spacerunner_lx_config.
The current code still has the struct (renamed to mainboard_config), but while changing Config.lb to devicetree.cb the register statement got dropped so there's no longer any value assigned to the config byte. I tried to re-add the register line where it was before (before chip northbridge/...) but this seems no longer to be allowed. And more importantly, init() which uses the config byte is still there but does not seem to be executed any more. At least I couldn't see its debug output.
I tried to look at the AMD DB800, which had served as my example at the time, but the DB800 code hasn't changed, so probably its init() isn't called now either. (Only the DB800 doesn't do anything important there.)
I noticed that enable_dev() is still being called. Is this a bug? Or is this on purpose? Am I meant to move my code from init() to enable_dev()? But then why does dev->ops still contain an init field?
I tried reading the docs both in the wiki and the documentation/ dir, but this seems seriously outdated and more misleading than helpful, because I as a newbie cannot possibly know which parts of the docs still apply and which to ignore. Doxygen produced 50998 files with details about each function and struct, which is rather overwhelming when you don't even know what you're looking for and trying to get sort of a general overview.
I tried looking at other boards which define something in struct mainboard_config, but they all seemed to have suffered a similar fate, I couldn't find one which still assigns a value to it in devicetree.cb. About a dozen use dev->ops->init in mainboard.c but I can't see what they would do differently that their init() would be actually called. - Maybe it isn't either?
Any pointer in the right direction would be appreciated.
Thanks in advance, Jens Rottmann
On Mon, Aug 16, 2010 at 7:57 AM, Jens Rottmann JRottmann@lippertembedded.de wrote:
Hi all,
in Nov 2008 (around rev. 3760) I had done Coreboot support for 2 of our boards (LiPPERT RoadRunner-LX and SpaceRunner-LX). Now I'd like to add support for 2 more, based on the code for the previous ones as the hardware is quite similar.
However in the past 2 years / 2000 revs Coreboot has evolved quite a lot! Even though SpaceRunner support still compiles, it doesn't quite seem to work as it did back then. Looks like I have to try and bring the 2 older board's code up to scratch first before I can base anything on it.
Sorry about the breakage. Things have changed quite a bit.
The old code defined a config byte (for some GPIOs) in struct mainboard_lippert_spacerunner_lx_config (chip.h), which used to be set in Config.lb with a simple register "..." = "..." statement and used in mainboard.c's init() by casting dev->chip_info to struct mainboard_lippert_spacerunner_lx_config.
The current code still has the struct (renamed to mainboard_config), but while changing Config.lb to devicetree.cb the register statement got dropped so there's no longer any value assigned to the config byte. I tried to re-add the register line where it was before (before chip northbridge/...) but this seems no longer to be allowed. And more importantly, init() which uses the config byte is still there but does not seem to be executed any more. At least I couldn't see its debug output.
I think that happened when the devicetree.cb files were created automatically. There weren't many configuration values for the mainboard.
I tried to look at the AMD DB800, which had served as my example at the time, but the DB800 code hasn't changed, so probably its init() isn't called now either. (Only the DB800 doesn't do anything important there.)
The attached patch calls init() again. It makes it match the comments again.
Signed-off-by: Myles Watson mylesgw@gmail.com
I think that the value that was left out of devicetree.cb should move to the mainboard init() or into the device that's being configured (cs5536.) It makes sense to use values in the device tree to parameterize device code for specific mainboards, but I don't see the point of parameterizing mainboard-specific code from the device tree.
Thanks, Myles
Myles Watson wrote:
The attached patch calls init() again. It makes it match the comments again.
Signed-off-by: Myles Watson mylesgw@gmail.com
Acked-by: Peter Stuge peter@stuge.se
It makes sense to use values in the device tree to parameterize device code for specific mainboards, but I don't see the point of parameterizing mainboard-specific code from the device tree.
It's no secret that I'd like coreboot to have zero mainboard-specific code, and allow parameters to be used to completely describe a board.
//Peter
Myles Watson wrote:
The attached patch calls init() again. It makes it match the comments
again.
Signed-off-by: Myles Watson mylesgw@gmail.com
Acked-by: Peter Stuge peter@stuge.se
It makes sense to use values in the device tree to parameterize device code for specific mainboards, but I don't see the point of parameterizing mainboard-specific code from the device tree.
It's no secret that I'd like coreboot to have zero mainboard-specific code, and allow parameters to be used to completely describe a board.
In that case, are you sure you want to ack it? Right now there are very few boards that have mainboard init functions ( < 20). I think now is the time to migrate away from it if it's a bad idea.
I don't think it would take very long to help Jens change the code so that the initialization that was done by the mainboard code gets done in the cs5536 code where it belongs.
Thanks, Myles
It makes sense to use values in the device tree to parameterize device code for specific mainboards, but I don't see the point of parameterizing mainboard-specific code from the device tree.
It's no secret that I'd like coreboot to have zero mainboard-specific code, and allow parameters to be used to completely describe a board.
In that case, are you sure you want to ack it? Right now there are very few boards that have mainboard init functions ( < 20). I think now is the time to migrate away from it if it's a bad idea.
I just checked, and the number of boards is 31 counting all the ones that use enable_dev for their mainboards.
Thanks, Myles
On 8/16/10 5:11 PM, Myles Watson wrote:
I think that the value that was left out of devicetree.cb should move to the mainboard init() or into the device that's being configured (cs5536.) It makes sense to use values in the device tree to parameterize device code for specific mainboards, but I don't see the point of parameterizing mainboard-specific code from the device tree.
Thanks, Myles Index: src/devices/device.c =================================================================== --- src/devices/device.c (revision 5692) +++ src/devices/device.c (working copy) @@ -1104,6 +1104,9 @@
printk(BIOS_INFO, "Initializing devices...\n");
- /* First call the mainboard init. */
- init_dev(&dev_root);
- /* now initialize everything. */ for (link = dev_root.link_list; link; link = link->next) init_link(link);
Acked-by: Stefan Reinauer stepan@coresystems.de
Thanks for fixing coreboot. mainboard specific setup code is critical for many boards.
Stefan
On 8/16/10 6:11 PM, Stefan Reinauer wrote:
On 8/16/10 5:11 PM, Myles Watson wrote:
Index: src/devices/device.c
--- src/devices/device.c (revision 5692) +++ src/devices/device.c (working copy) @@ -1104,6 +1104,9 @@
printk(BIOS_INFO, "Initializing devices...\n");
- /* First call the mainboard init. */
- init_dev(&dev_root);
- /* now initialize everything. */ for (link = dev_root.link_list; link; link = link->next) init_link(link);
Acked-by: Stefan Reinauer stepan@coresystems.de
Thanks for fixing coreboot. mainboard specific setup code is critical for many boards.
Stefan
Do we need a similar workaround for the enable_dev() call?
On Mon, Aug 16, 2010 at 10:15 AM, Stefan Reinauer stefan.reinauer@coresystems.de wrote:
On 8/16/10 6:11 PM, Stefan Reinauer wrote:
On 8/16/10 5:11 PM, Myles Watson wrote:
Index: src/devices/device.c
--- src/devices/device.c (revision 5692) +++ src/devices/device.c (working copy) @@ -1104,6 +1104,9 @@
printk(BIOS_INFO, "Initializing devices...\n");
- /* First call the mainboard init. */
- init_dev(&dev_root);
- /* now initialize everything. */ for (link = dev_root.link_list; link; link = link->next) init_link(link);
Acked-by: Stefan Reinauer stepan@coresystems.de
Rev 5698.
Do we need a similar workaround for the enable_dev() call?
It's already there. I think init() just got missed because so few boards use it.
Thanks, Myles
Stefan Reinauer wrote:
Thanks for fixing coreboot.
This was also why I acked. I think even if it's not what I would like, it's how things work now, so better fix it. :)
mainboard specific setup code is critical for many boards.
What does it do? Is 986lcd-m a good place to find complicated mb-specific setup code?
//Peter
On 8/16/10 6:45 PM, Peter Stuge wrote:
Stefan Reinauer wrote:
Thanks for fixing coreboot.
This was also why I acked. I think even if it's not what I would like, it's how things work now, so better fix it. :)
mainboard specific setup code is critical for many boards.
What does it do? Is 986lcd-m a good place to find complicated mb-specific setup code?
On some boards it sets up the fans. On others it makes sure the display brightness is initialized correctly. Sometimes it installs board specific interrupt handlers for oprom init.
Stefan
Stefan Reinauer wrote:
mainboard specific setup code is critical for many boards.
What does it do?
On some boards it sets up the fans. On others it makes sure the display brightness is initialized correctly.
What determines the correct way to do those tasks? Is it really mainboard specific; ie. circuit design? Even if there is a unique supporting circuit on the boards, the chips that we talk to from software will only have a finite ways they can work, so we should always be able to use parameters..
Sometimes it installs board specific interrupt handlers for oprom init.
Thanks, I "forgot" about that one. :) What makes those interrupt handlers board specific though? What they return might also be easy to control by simple parameters.
//Peter
On 8/16/10 7:11 PM, Peter Stuge wrote:
Stefan Reinauer wrote:
mainboard specific setup code is critical for many boards.
What does it do?
On some boards it sets up the fans. On others it makes sure the display brightness is initialized correctly.
What determines the correct way to do those tasks? Is it really mainboard specific; ie. circuit design?
Yes. Hence they're done in mainboard.c
Some of them _might_ live in mainboard.c because creating a new framework like a "display brightness framework" into coreboot and writing a driver for whatever actual hardware controls brightness just for getting one value written into one register didn't seem appropriate at the time. So in the end it makes life easier for porters and readers than having to cope with yet another framework.
Even if there is a unique supporting circuit on the boards, the chips that we talk to from software will only have a finite ways they can work, so we should always be able to use parameters..
Yes. But you have to weigh things up. While there might be a "formula for everything" it might not be the most effective way to solve one given problem, even if the model sounds nicer in theory.
Sometimes it installs board specific interrupt handlers for oprom init.
Thanks, I "forgot" about that one. :) What makes those interrupt handlers board specific though? What they return might also be easy to control by simple parameters.
That might be very different things, like how wide the graphics bus is or how fast it can be clocked based on how the mainboard designed layed out the wires. Or whether the board has a TV-Out that should be selected.
I agree a lot of this _could_ go into the config / device tree, but right now there are bigger drawbacks one could fight if going for a sconfig overhaul. Such as putting interrupt routing information in there. Maybe even dropping the idea of "register" in sconfig completely and coming up with something better.
Stefan
Hi Jens,
On 8/16/10 3:57 PM, Jens Rottmann wrote:
The old code defined a config byte (for some GPIOs) in struct mainboard_lippert_spacerunner_lx_config (chip.h), which used to be set in Config.lb with a simple register "..." = "..." statement and used in mainboard.c's init() by casting dev->chip_info to struct mainboard_lippert_spacerunner_lx_config.
The current code still has the struct (renamed to mainboard_config), but while changing Config.lb to devicetree.cb the register statement got dropped so there's no longer any value assigned to the config byte. I tried to re-add the register line where it was before (before chip northbridge/...) but this seems no longer to be allowed. And more importantly, init() which uses the config byte is still there but does not seem to be executed any more. At least I couldn't see its debug output.
Does the following patch fix the problem for you?
Index: src/Kconfig =================================================================== --- src/Kconfig (revision 5692) +++ src/Kconfig (working copy) @@ -740,7 +740,7 @@
config WARNINGS_ARE_ERRORS bool - default y + default n
config ID_SECTION_OFFSET hex Index: src/mainboard/lippert/spacerunner-lx/devicetree.cb =================================================================== --- src/mainboard/lippert/spacerunner-lx/devicetree.cb (revision 5692) +++ src/mainboard/lippert/spacerunner-lx/devicetree.cb (working copy) @@ -1,3 +1,7 @@ +# See also SMC_CONFIG in cache_as_ram_auto.c. +# Bit0 turns off Live LED, bit1 switches Com1 to RS485, bit2 same for Com2. +register "sio_gp1x_config" = "0x01" + chip northbridge/amd/lx device pci_domain 0 on device pci 1.0 on end # Northbridge
Stefan
Hi,
Myles Watson wrote:
Sorry about the breakage. Things have changed quite a bit.
No need to apologize, 2000 commits is really impressive, no one can expect this to be possible without occasional hiccups, especially with code that doesn't get tested. It's really great what has become of coreboot!! Most visible to me (so far) is Kconfig which makes things a lot less scary than they used to be and Geode-LX VGA support.
The attached patch calls init() again.
Indeed, it does. Excellent, thanks a lot!!! :)
Stefan Reinauer wrote:
Does the following patch fix the problem for you?
Umm ... nope, neither with nor without Myles's patch:
Reading resources... Unknown device path type: -1 read_resources bus cd6 link: 15 Unexpected Exception: 13 @ 10:0000674e - Halting Code: 0 eflags: 00010086 eax: ffffffff ebx: 0000000f ecx: 0000fedf edx: 00000003 edi: 00000000 esi: 4623fc0f ebp: 0000fbeb esp: 0001ff14
But ...
Myles Watson wrote:
I think that the value that was left out of devicetree.cb should move to the mainboard init() or into the device that's being configured (cs5536.)
In fact I was already wondering whether the whole code in mainboard init() is actually at the right place. That's one of the things I meant when saying "bring the 2 older board's code up to scratch first". The init() code does 3 things:
- set up misc CS5536 GPIOs - configure the EC, which is a device of the IT8712F SIO. However config isn't done via the usual SIO ports 2E/2F but via the EC's own I/O adresses. - set misc IT8712 GPIOs, via the Simple-I/O port (with the config value that was dropped from devicetree.cb)
Other parts of the SIO config however, e.g. assigning the EC and Simple-I/O base addresses is done in devicetree.cb whereas setting up which SIO GPIOs are outputs and which are mapped for Simple-I/O is done in romstage.c because parts of it are better be done early.
This mess makes me think if there is a cleaner way and which parts belong where.
It makes sense to use values in the device tree to parameterize device code for specific mainboards, but I don't see the point of parameterizing mainboard-specific code from the device tree.
I on the other hand was intending to separate the configuration _data_ from the _code_ that writes it into the chip. The customers will still have to customize coreboot for their purpose. Some might want to move the COM1 from 3F8 to 3E8, because they have added an external RS232 on PC/104 (ISA). And some might disable LPT because they need the IRQ for something else. With a standard BIOS they can do it in the BIOS Setup. Here they can edit a common config _data_ file devicetree.cb. They don't have to mess with e.g. the superio/ite/it8712f/superio.c _code_. And switching COM1 from RS232 to RS485 mode can be done in the standard BIOS Setup, too. That's why I placed this mainboard config value in devicetree.cb, too, rather than having customers mess with the code in mainboard.c. I had guessed that this is the purpose why there is a struct mainboard_config in the mainboard's chip.h, just as there is a struct superio_..._config in all superio's chip.h. In what way is configuring hardware implemented inside a SIO chip so much different from configuring hardware implemented as a couple of discrete logic gates on the mainboard? Am I making any sense?? :)
There is second mainboard-specific config value (SMC_CONFIG), which I'd have prefered in devicetree.cb, too. But I couldn't get this to work 2 years ago, because the code is run early from romstage.c ...
Thanks and regards, Jens
It's really great what has become of coreboot!! Most visible to me (so far) is Kconfig which makes things a lot less scary than they used to be and Geode-LX VGA support.
The attached patch calls init() again.
Indeed, it does. Excellent, thanks a lot!!! :)
OK. Hopefully you're almost there.
Myles Watson wrote:
I think that the value that was left out of devicetree.cb should move to the mainboard init() or into the device that's being configured
(cs5536.)
In fact I was already wondering whether the whole code in mainboard init() is actually at the right place. That's one of the things I meant when saying "bring the 2 older board's code up to scratch first". The init() code does 3 things:
- set up misc CS5536 GPIOs
- configure the EC, which is a device of the IT8712F SIO. However config
isn't done via the usual SIO ports 2E/2F but via the EC's own I/O adresses.
- set misc IT8712 GPIOs, via the Simple-I/O port (with the config value
that was dropped from devicetree.cb)
Other parts of the SIO config however, e.g. assigning the EC and Simple- I/O base addresses is done in devicetree.cb whereas setting up which SIO GPIOs are outputs and which are mapped for Simple-I/O is done in romstage.c because parts of it are better be done early.
Yes. Since the device tree isn't available until RAM is initialized, early things can't live there.
This mess makes me think if there is a cleaner way and which parts belong where.
Great.
It makes sense to use values in the device tree to parameterize device code for specific mainboards, but I don't see the point of parameterizing mainboard-specific code from the device tree.
I on the other hand was intending to separate the configuration _data_ from the _code_ that writes it into the chip. The customers will still have to customize coreboot for their purpose. Some might want to move the COM1 from 3F8 to 3E8, because they have added an external RS232 on PC/104 (ISA). And some might disable LPT because they need the IRQ for something else. With a standard BIOS they can do it in the BIOS Setup. Here they can edit a common config _data_ file devicetree.cb. They don't have to mess with e.g. the superio/ite/it8712f/superio.c _code_. And switching COM1 from RS232 to RS485 mode can be done in the standard BIOS Setup, too. That's why I placed this mainboard config value in devicetree.cb, too, rather than having customers mess with the code in mainboard.c. I had guessed that this is the purpose why there is a struct mainboard_config in the mainboard's chip.h, just as there is a struct superio_..._config in all superio's chip.h. In what way is configuring hardware implemented inside a SIO chip so much different from configuring hardware implemented as a couple of discrete logic gates on the mainboard? Am I making any sense?? :)
Yes. You're right that the separation is important.
There is second mainboard-specific config value (SMC_CONFIG), which I'd have prefered in devicetree.cb, too. But I couldn't get this to work 2 years ago, because the code is run early from romstage.c ...
I think you need Kconfig variables for both of those things. The device tree isn't for options. It's for immutable properties of the mainboard.
I'd suggest that your options depend on the motherboard.
Thanks, Myles
Hi,
sorry for the delay, I'm continually jumping between projects and especially if a customer wants something I have to immediately drop everything else.
I wrote:
[...] separate the configuration _data_ from the _code_ that writes it into the chip. [...] I had guessed that this is the purpose why there is a struct mainboard_config in the mainboard's chip.h, just as there is a struct superio_..._config in all superio's chip.h.
Myles wrote:
The device tree isn't for options. It's for immutable properties of the mainboard.
I'm not sure why things like 'register "com1_address" = "0x3E8"', which is in devicetree.cb, and which is changable in most standard BIOSes' Setup, are considered more immutable than an option switching RS232 to RS485 (via some mainboard logic).
In this sense I experimented doing this:
chip mainboard/CONFIG_MAINBOARD_DIR # Bit0 turns off Live LED, bit1 switches Com1 to RS485, bit2 same for Com2 register "sio_gp1x_config" = "0x01" chip northbridge/amd/lx device pci_domain 0 on ...
i.e. make the mainboard the root device of the device tree - seemed plausible to me, somehow. This compiles and boots fine, but the 'register' statement still doesn't have any effect, sio_gp1x_config in the mainboard's chip.h still isn't set - hmm, tough luck ... Why have a struct mainboard_config if it can't be used? Maybe it should be removed completely??
I think you need Kconfig variables for both of those things.
Ok, then I'll take this approach instead - and tripped over something else:
The vendor's Kconfig looks like this: choice prompt "Mainboard model" source board/1 ... endchoice
This means you can't put any (visible) options in the board's Kconfig, because they'd all end up _inside_ the choice instead of as seperate options. So you'd have to put them all in the vendor Kconfig - but this would soon become very ugly with all the "depends on"s once the list of boards and options grows. And board specific options just belong in the board specific Kconfig, not in the vendor's.
So I did this: choice prompt "Mainboard model" config BOARD_1 bool "Board 1" endchoice source board/1
This made the board Kconfig: config BOARD_1 bool "Board 1" select FOO
lose the BOARD_1 option. But I needed something to attach the selects to: config BOARD_SPECIFIC_OPTIONS # dummy def_bool y select FOO
(Dependencies skipped for simplicity, see attached full patch.) This works fine, but I don't like the dummy config option BOARD_SPECIFIC_OPTIONS, which is always y and doesn't actually configure anything.
- Does anyone have a better idea, avoiding the dummy option? - I'm going to use the same scheme for the RoadRunner-LX and also for the 2 additional boards I'm planning to base on this. Maybe this restructuring should be applied to all boards/vendors?? I've seen there is one other board with a private config option, which is so far kept in the vendor's Kconfig.
Any comments are most welcome.
Thanks and cheers, Jens
sio_gp1x_config can no longer be set from devicetree.cb, so convert this into a Kconfig option instead. And while I'm at it, also add option for SMC_CONFIG so user doesn't have to mess around in C files.
Signed-off-by: Jens Rottmann JRottmann@LiPPERTEmbedded.de ---
--- src/mainboard/lippert/Kconfig (rev 5739) +++ src/mainboard/lippert/Kconfig (working copy) @@ -1,10 +1,16 @@ +if VENDOR_LIPPERT + choice prompt "Mainboard model" - depends on VENDOR_LIPPERT
source "src/mainboard/lippert/frontrunner/Kconfig" source "src/mainboard/lippert/roadrunner-lx/Kconfig" -source "src/mainboard/lippert/spacerunner-lx/Kconfig" + +config BOARD_LIPPERT_SPACERUNNER_LX + bool "Cool SpaceRunner-LX"
endchoice
+source "src/mainboard/lippert/spacerunner-lx/Kconfig" + +endif # VENDOR_LIPPERT --- src/mainboard/lippert/spacerunner-lx/Kconfig (rev 5739) +++ src/mainboard/lippert/spacerunner-lx/Kconfig (working copy) @@ -1,5 +1,7 @@ -config BOARD_LIPPERT_SPACERUNNER_LX - bool "Cool SpaceRunner-LX" +if BOARD_LIPPERT_SPACERUNNER_LX + +config BOARD_SPECIFIC_OPTIONS # dummy + def_bool y select ARCH_X86 select CPU_AMD_LX select NORTHBRIDGE_AMD_LX @@ -9,24 +11,37 @@ select PIRQ_ROUTE select UDELAY_TSC select USE_DCACHE_RAM + # Board is equipped with a 1 MB SPI flash, however, due to limitations + # of the IT8712F Super I/O, only the top 512 KB are directly mapped. select BOARD_ROMSIZE_KB_512
config MAINBOARD_DIR string default lippert/spacerunner-lx - depends on BOARD_LIPPERT_SPACERUNNER_LX
config MAINBOARD_PART_NUMBER string default "Cool SpaceRunner-LX" - depends on BOARD_LIPPERT_SPACERUNNER_LX
config IRQ_SLOT_COUNT int default 7 - depends on BOARD_LIPPERT_SPACERUNNER_LX
config RAMBASE hex default 0x4000 - depends on BOARD_LIPPERT_SPACERUNNER_LX + +config ONBOARD_UARTS_RS485 + bool "Switch on-board serial ports to RS485" + default n + help + If selected, both on-board serial ports will operate in RS485 mode + instead of RS232. + +config ONBOARD_IDE_SLAVE + bool "Make on-board SSD act as Slave" + default n + help + If selected, the on-board SSD will act as IDE Slave instead of Master. + +endif # BOARD_LIPPERT_SPACERUNNER_LX --- src/mainboard/lippert/spacerunner-lx/chip.h (rev 5739) +++ src/mainboard/lippert/spacerunner-lx/chip.h (working copy) @@ -20,11 +20,7 @@
/* Based on chip.h from AMD's DB800 mainboard. */
-#include <stdint.h> - extern struct chip_operations mainboard_ops;
struct mainboard_config { - /* bit2 = RS485_EN2, bit1 = RS485_EN1, bit0 = Live LED */ - u8 sio_gp1x_config; }; --- src/mainboard/lippert/spacerunner-lx/mainboard.c (rev 5739) +++ src/mainboard/lippert/spacerunner-lx/mainboard.c (working copy) @@ -29,6 +29,13 @@ #include <device/pci_ids.h> #include "chip.h"
+/* Bit0 turns off Live LED, bit1 switches Com1 to RS485, bit2 same for Com2. */ +#if CONFIG_ONBOARD_UARTS_RS485 + #define SIO_GP1X_CONFIG 0x07 +#else + #define SIO_GP1X_CONFIG 0x01 +#endif + static const u16 ec_init_table[] = { /* hi=data, lo=index */ 0x1900, /* Enable monitoring */ 0x3050, /* VIN4,5 enabled */ @@ -41,7 +48,6 @@
static void init(struct device *dev) { - struct mainboard_config *mb = dev->chip_info; unsigned int gpio_base, i; printk(BIOS_DEBUG, "LiPPERT SpaceRunner-LX ENTER %s\n", __func__);
@@ -65,7 +71,9 @@ outb(val >> 8, 0x0296); }
- outb(mb->sio_gp1x_config, 0x1220); /* Simple-I/O GP17-10 */ + /* bit2 = RS485_EN2, bit1 = RS485_EN1, bit0 = Live LED */ + outb(SIO_GP1X_CONFIG, 0x1220); /* Simple-I/O GP17-10 */ + printk(BIOS_DEBUG, "LiPPERT SpaceRunner-LX EXIT %s\n", __func__); }
--- src/mainboard/lippert/spacerunner-lx/romstage.c (rev 5739) +++ src/mainboard/lippert/spacerunner-lx/romstage.c (working copy) @@ -41,7 +41,11 @@ #include "superio/ite/it8712f/it8712f_early_serial.c"
/* Bit0 enables Spread Spectrum, bit1 makes on-board SSD act as IDE slave. */ -#define SMC_CONFIG 0x01 +#if CONFIG_ONBOARD_IDE_SLAVE + #define SMC_CONFIG 0x03 +#else + #define SMC_CONFIG 0x01 +#endif
#define ManualConf 1 /* No automatic strapped PLL config */ #define PLLMSRhi 0x0000059C /* Manual settings for the PLL */ @@ -201,4 +205,3 @@ /* Memory is setup. Return to cache_as_ram.inc and continue to boot. */ return; } - _
Am 25.08.2010 18:10, schrieb Jens Rottmann:
i.e. make the mainboard the root device of the device tree - seemed plausible to me, somehow. This compiles and boots fine, but the 'register' statement still doesn't have any effect, sio_gp1x_config in the mainboard's chip.h still isn't set - hmm, tough luck ... Why have a struct mainboard_config if it can't be used? Maybe it should be removed completely??
That mechanism could be fixed to work correctly again. Attached patch should do that.
Signed-off-by: Patrick Georgi patrick.georgi@coresystems.de
Am 25.08.2010 18:10, schrieb Jens Rottmann:
i.e. make the mainboard the root device of the device tree - seemed
plausible to
me, somehow. This compiles and boots fine, but the 'register' statement
still
doesn't have any effect, sio_gp1x_config in the mainboard's chip.h still
sio_gp1x_config sounds like it should be in the superio, not the mainboard. I'm not sure I understand the need for mainboard_config. I'd rather just see it die. I think everything that's been there could be moved to devices or Kconfig.
How many boards try to use mainboard_config?
Thanks, Myles
On Thu, Aug 26, 2010 at 10:42 AM, Myles Watson mylesgw@gmail.com wrote:
Am 25.08.2010 18:10, schrieb Jens Rottmann:
i.e. make the mainboard the root device of the device tree - seemed
plausible to
me, somehow. This compiles and boots fine, but the 'register' statement
still
doesn't have any effect, sio_gp1x_config in the mainboard's chip.h still
sio_gp1x_config sounds like it should be in the superio, not the mainboard. I'm not sure I understand the need for mainboard_config. I'd rather just see it die. I think everything that's been there could be moved to devices or Kconfig.
How many boards try to use mainboard_config?
Three. Two lippert boards and amd/rumba.
rumba uses it for a NIC IRQ setting.
Thanks, Myles
One reason for keeping other settings out of the mainboard is that they may get overwritten later.
It looks like the register at gpio_address + 0x18 (GPIOL_PULLUP_ENABLE) is getting written in cs5536.c and spacerunner-lx/mainboard.c.
Thanks, Myles
Myles wrote:
It looks like the register at gpio_address + 0x18 (GPIOL_PULLUP_ENABLE) is getting written in cs5536.c and spacerunner-lx/mainboard.c.
No, that's fine. These are not normal r/w registers, they set and clear bits only by writing 1-bits. Writing 0-bits has no effect. The writes in cs5536.c and mainboard.c address different bits so are independent of each other.
Patrick wrote:
Am 25.08.2010 18:10, schrieb Jens Rottmann:
i.e. make the mainboard the root device of the device tree - seemed
plausible to
me, somehow. This compiles and boots fine, but the 'register' statement still doesn't have any effect, sio_gp1x_config in the mainboard's chip.h still isn't set - hmm, tough luck ... Why have a struct mainboard_config if it can't be used? Maybe it should be removed completely??
That mechanism could be fixed to work correctly again. Attached patch should do that.
Tried chip mainboard / register ... in several locations but couldn't get it to work, sometimes it wouldn't compile or else the mainboard's enable_dev + init weren't called at all.
However, as I said in my last mail, I'll go for the Kconfig approach now, just as Myles suggested.
In my last mail I described some restructuring to make (user visible) options in the board specific Kconfig files possible (i.e. don't source from within choice/endchoice). Another change I'd like to add is to move MAINBOARD_VENDOR and MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID to the vendor's Kconfigs.
I'm going to try and prepare a patch for _all_ vendors/boards. If someone is already doing this or has objections or suggestions how to do this better (like how to avoid the dummy option!) please tell me before I waste too much time.
Cheers, Jens
On 8/27/10 4:24 PM, Jens Rottmann wrote:
me, somehow. This compiles and boots fine, but the 'register' statement still doesn't have any effect, sio_gp1x_config in the mainboard's chip.h still isn't set - hmm, tough luck ... Why have a struct mainboard_config if it can't be used? Maybe it should be removed completely??
That mechanism could be fixed to work correctly again. Attached patch should do that.
Tried chip mainboard / register ... in several locations but couldn't get it to work, sometimes it wouldn't compile or else the mainboard's enable_dev + init weren't called at all.
Patricks patch, plus this addition should do the job:
macpro:coreboot stepan$ svn diff Index: src/mainboard/lippert/spacerunner-lx/devicetree.cb =================================================================== --- src/mainboard/lippert/spacerunner-lx/devicetree.cb (revision 5745) +++ src/mainboard/lippert/spacerunner-lx/devicetree.cb (working copy) @@ -1,3 +1,7 @@ +chip mainboard/lippert/spacerunner-lx + +register "sio_gp1x_config" = "1" + chip northbridge/amd/lx device pci_domain 0 on device pci 1.0 on end # Northbridge @@ -88,3 +92,5 @@ end end end + +end
If it does not, we should fix the mechanism, independently of your use of Kconfig,...
Stefan
Patricks patch, plus this addition should do the job:
You're right, it does. My mistake was trying to be extra clever and writing
chip mainboard/CONFIG_MAINBOARD_DIR
instead of
chip mainboard/lippert/spacerunner-lx
because CONFIG_MAINBOARD_DIR == lippert/spacerunner-lx. This compiled and even booted, but appearently still broke something ... sorry!
Tested-by: Jens Rottmann JRottmann@LiPPERTEmbedded.de :-)
Cheers, Jens
Hi,
In my last mail I described some restructuring to make (user visible) options in the board specific Kconfig files possible (i.e. don't source from within choice/endchoice). Another change I'd like to add is to move MAINBOARD_VENDOR and MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID to the vendor's Kconfigs.
I'm going to try and prepare a patch for _all_ vendors/boards. If someone is already doing this or has objections or suggestions how to do this better (like how to avoid the dummy option!) please tell me before I waste too much time.
Ok, here is it. Turned out larger than I had expeced, so I bzip2ed it. Hope you like it.
This applies _on top_ of Andreas Schultz's "[PATCH 3/3] support for Lanner EM-8510 Board", so his patch doesn't need to be changed.
Cheers, Jens
On 8/30/10 5:47 PM, Jens Rottmann wrote:
Hi,
In my last mail I described some restructuring to make (user visible) options in the board specific Kconfig files possible (i.e. don't source from within choice/endchoice). Another change I'd like to add is to move MAINBOARD_VENDOR and MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID to the vendor's Kconfigs.
I'm going to try and prepare a patch for _all_ vendors/boards. If someone is already doing this or has objections or suggestions how to do this better (like how to avoid the dummy option!) please tell me before I waste too much time.
Ok, here is it. Turned out larger than I had expeced, so I bzip2ed it. Hope you like it.
This applies _on top_ of Andreas Schultz's "[PATCH 3/3] support for Lanner EM-8510 Board", so his patch doesn't need to be changed.
Very nice, thank you!
This significantly reduces the number of changes to Kconfig when creating a new board, starting out as a copy from another board.
Checked in as r5754
Stefan
-
Jens Rottmann -
Myles Watson -
Patrick Georgi -
Peter Stuge -
Stefan Reinauer