Added a find function, and other things. I would really appreciate a review here.
Peter is supposed to be in greece on the beach. HA!
This is working on DBE62. Now that it is done I can do some more K8 work -- assuming it meets with approval.
ron
On Sat, Aug 09, 2008 at 01:18:23PM -0700, ron minnich wrote:
Added a find function, and other things. I would really appreciate a review here.
Overall good direction but some comments and one possible issue.
Peter is supposed to be in greece on the beach. HA!
Leaving (a lot) later tonight!
This finishes up an earlier patch. I hope we can get a commit.
Probably.
Console: (1)we now compile in all printks, which is good: we can print any message provided we can change the console log level at any time. (2) The console log level is compiled in and unchangeable, which is bad, as it defeats the purpose of (1).
Add a BIOS_ALWAYS log level. Make console log level a variable. Make functions that set it and get it visible everywhere. Always print out the version message; this is really *not* noise!
I like it!
PCI: Simplify pci functions so that they can be used in stage1 or anywhere for that matter. Add a find function which is needed for many stage1 functions. Note that we copy but also clean up the libpayload stuff just a bit.
Hmm. I don't know about this. What happened to the flat device tree that was intended to be very easy to use? I don't like removing the struct. Continue if you must, for the good of K8, but I will make a fuss about this later on if there's no discussion now.
Get rid of config space type 2. If there was ever a platform that used it, I don't know what it was, and the presence is a needless distraction.
No problem.
tested and working on DBE62 (which means the console and the pci funtions work :-).
Signed-off-by: Ronald G. Minnich rminnich@gmail.com
If you take a pass through the comments, it is:
Acked-by: Peter Stuge peter@stuge.se
Index: include/console.h
--- include/console.h (revision 726) +++ include/console.h (working copy) @@ -21,6 +21,7 @@ #include <shared.h> /* We share symbols from stage 0 */ #include <post_code.h>
+#define BIOS_ALWAYS 0 /* log no matter what; not necessarily an error */ #define BIOS_EMERG 0 /* system is unusable */ #define BIOS_ALERT 1 /* action must be taken immediately */ #define BIOS_CRIT 2 /* critical conditions */ Index: include/device/pci_def.h =================================================================== --- include/device/pci_def.h (revision 726) +++ include/device/pci_def.h (working copy) @@ -481,6 +481,8 @@ #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f) #define PCI_FUNC(devfn) ((devfn) & 0x07) #define PCI_BDF(bus,dev,func) ((bus) << 16 | (dev) << 11 | (func) << 8) +/* bus,devfn pairs are used many places as well */ +#define PCI_BDEVFN(bus,devfn) ((bus) << 16 | (devfn) << 8)
Maybe change PCI_BDF to use PCI_BDEVFN ?
#define PCI_ADDR(bus,dev,func,where) (PCI_BDF((bus),(dev),(func)) << 4 | (where & 0xfff))
#endif /* DEVICE_PCI_DEF_H */ Index: include/device/pci.h =================================================================== --- include/device/pci.h (revision 726) +++ include/device/pci.h (working copy) @@ -49,12 +49,13 @@
/* Common pci bus operations */ struct pci_bus_operations {
- u8 (*read8)(struct bus *pbus, int bus, int devfn, int where);
- u16 (*read16)(struct bus *pbus, int bus, int devfn, int where);
- u32 (*read32)(struct bus *pbus, int bus, int devfn, int where);
- void (*write8)(struct bus *pbus, int bus, int devfn, int where, u8 val);
- void (*write16)(struct bus *pbus, int bus, int devfn, int where, u16 val);
- void (*write32)(struct bus *pbus, int bus, int devfn, int where, u32 val);
- u8 (*read8)(u32 bdf, int where);
- u16 (*read16)(u32 bdf, int where);
- u32 (*read32)(u32 bdf, int where);
- void (*write8)(u32 bdf, int where, u8 val);
- void (*write16)(u32 bdf, int where, u16 val);
- void (*write32)(u32 bdf, int where, u32 val);
- int (*find)(u16 vendid, u16 devid, u32 *busdevfn);
};
struct pci_driver { Index: include/arch/x86/pci_ops.h =================================================================== --- include/arch/x86/pci_ops.h (revision 726) +++ include/arch/x86/pci_ops.h (working copy) @@ -20,7 +20,6 @@ #include <device/device.h>
extern const struct pci_bus_operations pci_cf8_conf1; -extern const struct pci_bus_operations pci_cf8_conf2;
#if defined(CONFIG_MMCONF_SUPPORT) && (CONFIG_MMCONF_SUPPORT==1) extern const struct pci_bus_operations pci_ops_mmconf; Index: mainboard/artecgroup/dbe62/initram.c =================================================================== --- mainboard/artecgroup/dbe62/initram.c (revision 726) +++ mainboard/artecgroup/dbe62/initram.c (working copy) @@ -152,7 +152,7 @@ sdram_enable(DIMM0, DIMM1); printk(BIOS_DEBUG, "done sdram enable\n");
- dumplxmsrs();
- /*dumplxmsrs();*/ /* Check low memory */ /* The RAM is working now. Leave this test commented out but
- here for reference.
Index: device/pci_ops.c
--- device/pci_ops.c (revision 726) +++ device/pci_ops.c (working copy) @@ -48,42 +48,48 @@
u8 pci_read_config8(struct device *dev, unsigned int where) {
- struct bus *pbus = get_pbus(dev);
- return ops_pci_bus(pbus)->read8(pbus, dev->bus->secondary,
dev->path.u.pci.devfn, where);
struct bus *pbus = get_pbus(dev);
Looks like some extra spaces snuck in here.
- return ops_pci_bus(pbus)->read8(PCI_BDEVFN(dev->bus->secondary,
dev->path.u.pci.devfn),where);
Wasn't the union name removed so that this would become dev->path.pci.devfn everywhere?
Index: lib/console.c
--- lib/console.c (revision 726) +++ lib/console.c (working copy) @@ -8,9 +8,30 @@ int vtxprintf(void (*)(unsigned char, void *arg), void *arg, const char *, va_list);
-static int console_loglevel(void) +static int loglevel = CONFIG_DEFAULT_CONSOLE_LOGLEVEL;
+/**
- set the console log level
- There are no invalid settings, although there are ones that
- do not make much sense.
- @param level The new level
- */
+void set_loglevel(unsigned level) {
Some inconsistency here. int above, unsigned here, maybe use u8?
+static unsigned int console_loglevel(void)
Same here of course.
" starting...\n";
" starting... (console logging at %d)\n";
Maybe " starting... (console_loglevel=%d)\n" ?
printk(BIOS_DEBUG, "%s (%lx): %x.%x\n", msrnames[i], msrs[i],
/* don't change the %p to a %s unless you fix the problem.* in particular, don't change or submit a patch UNLESS YOU TEST IT*/printk(BIOS_DEBUG, "%p (%lx): %x.%x\n", msrnames[i], msrs[i],
Sorry about that one. It seems to make such good sense though.
Index: arch/x86/pci_ops_conf1.c
--- arch/x86/pci_ops_conf1.c (revision 726) +++ arch/x86/pci_ops_conf1.c (working copy) @@ -6,58 +6,151 @@ #include <device/pci_ops.h> #include <types.h> #include <io.h> +/*
- This file is part of the libpayload project.
Oh no it's not! :)
- @param busdevfn pointer to a u32 in which the slot is returned.
- @return 1 if found, 0 otherwise
- */
+static int find_on_bus(u16 bus, u16 vid, u16 did, u32 *busdevfn)
+{
- u16 devfn;
A few spurious blank lines here, not such a big deal.
Index: arch/x86/pci_ops_auto.c
--- arch/x86/pci_ops_auto.c (revision 726) +++ arch/x86/pci_ops_auto.c (working copy) @@ -22,7 +22,6 @@ u16 class, vendor; unsigned bus; int devfn;
- struct bus pbus; /* Dummy device */
#define PCI_CLASS_BRIDGE_HOST 0x0600 #define PCI_CLASS_DISPLAY_VGA 0x0300 #define PCI_VENDOR_ID_COMPAQ 0x0e11 @@ -30,8 +29,8 @@ #define PCI_VENDOR_ID_MOTOROLA 0x1057
for (bus = 0, devfn = 0; devfn < 0x100; devfn++) {
class = o->read16(&pbus, bus, devfn, PCI_CLASS_DEVICE);vendor = o->read16(&pbus, bus, devfn, PCI_VENDOR_ID);
class = o->read16(PCI_BDEVFN(bus, devfn), PCI_CLASS_DEVICE);vendor = o->read16(PCI_BDEVFN(bus, devfn), PCI_VENDOR_ID);
Seems there's a whitespace problem here as well.
//Peter
On Sat, Aug 9, 2008 at 1:37 PM, Peter Stuge peter@stuge.se wrote:
Console: (1)we now compile in all printks, which is good: we can print any message provided we can change the console log level at any time. (2) The console log level is compiled in and unchangeable, which is bad, as it defeats the purpose of (1).
Add a BIOS_ALWAYS log level. Make console log level a variable. Make functions that set it and get it visible everywhere. Always print out the version message; this is really *not* noise!
I like it!
I will commit that.
Hmm. I don't know about this. What happened to the flat device tree that was intended to be very easy to use? I don't like removing the struct. Continue if you must, for the good of K8, but I will make a fuss about this later on if there's no discussion now.
Note that the struct was never used in these functions.
not sure what you mean. But in stage1 there is no device tree -- there is no memory in which to store it when many of these pci functions are called. . [[[ wow gmail is awful -- I am typing 30 seconds ahead of the text! -- feature bloat!]]]
The lowest level pci ops should be just that -- pci ops, nothing more. They should take simple parameters. They should not require huge amounts of setup. In general, they should only be directly called in the very early code -- as in stage1. Once things are set up, and we have a device tree, the story changes.
Once we have built a device tree, there is the very handy set of functions in device/device.c etc. But on v2, we not only mixed this all up, we actually bulit two separate libraries at compile time, depending on whether it was romcc or gcc doing the compiling. Talk about code bloat -- two totally separate libraries, each doing the same thing, derived from cpp and other wizardry, with different parameter types -- ow ow ow ow ow my brain hurts.
Why did we do that? Because we needed one type of code for romcc -- there is no memory -- and another type for gcc -- we have memory.
We don't need to do this on v3 -- so we should not.
So to me this change is actually just good design.
Maybe change PCI_BDF to use PCI_BDEVFN ?
let's let it go. I don't think it enhances readability.
u8 pci_read_config8(struct device *dev, unsigned int where) {
struct bus *pbus = get_pbus(dev);return ops_pci_bus(pbus)->read8(pbus, dev->bus->secondary,dev->path.u.pci.devfn, where);
struct bus *pbus = get_pbus(dev);Looks like some extra spaces snuck in here.
my ubuntu emacs has gone nuts. It is not doing linux-c-mode correctly at all any more.
Until I reload with something else, can I count on somebody to format this ?
Wasn't the union name removed so that this would become dev->path.pci.devfn everywhere?
I will commit this and the anon union version later.
Some inconsistency here. int above, unsigned here, maybe use u8?
I'll got with unsigned int.I don't like specify bitwidths unless it really matters.
" starting...\n";
" starting... (console logging at %d)\n";Maybe " starting... (console_loglevel=%d)\n" ?
done.
printk(BIOS_DEBUG, "%s (%lx): %x.%x\n", msrnames[i], msrs[i],
/* don't change the %p to a %s unless you fix the problem.* in particular, don't change or submit a patch UNLESS YOU TEST IT*/printk(BIOS_DEBUG, "%p (%lx): %x.%x\n", msrnames[i], msrs[i],Sorry about that one. It seems to make such good sense though.
yeah, it's very weird.
- This file is part of the libpayload project.
Oh no it's not! :)
fixed.
Seems there's a whitespace problem here as well.
I'll need help until I get my !@$#!@$# emacs fixed.
Committed revision 729.
ron
On 09.08.2008 22:18, ron minnich wrote:
Added a find function, and other things. I would really appreciate a review here.
Peter is supposed to be in greece on the beach. HA!
This is working on DBE62. Now that it is done I can do some more K8 work -- assuming it meets with approval.
[...]
Patch looks mostly OK except one part:
Index: lib/console.c
--- lib/console.c (revision 726) +++ lib/console.c (working copy) @@ -8,9 +8,30 @@ int vtxprintf(void (*)(unsigned char, void *arg), void *arg, const char *, va_list);
-static int console_loglevel(void) +static int loglevel = CONFIG_DEFAULT_CONSOLE_LOGLEVEL;
+/**
- set the console log level
- There are no invalid settings, although there are ones that
- do not make much sense.
- @param level The new level
- */
+void set_loglevel(unsigned level) {
- if (level > BIOS_SPEW)
printk(BIOS_ALWAYS, "Warning: ridiculous log level setting: %d (max %d)\n",level, BIOS_SPEW);- loglevel = level;
+}
+/**
- get the console log level
- @return The level
- */
+static unsigned int console_loglevel(void) {
- return CONFIG_DEFAULT_CONSOLE_LOGLEVEL;
- return loglevel;
}
#ifdef CONFIG_CONSOLE_BUFFER @@ -150,9 +171,9 @@ COREBOOT_EXTRA_VERSION " " COREBOOT_BUILD
" starting...\n";
" starting... (console logging at %d)\n";
- printk(BIOS_INFO, console_test);
- printk(BIOS_ALWAYS, console_test, console_loglevel());
}
/**
NACK that part. It will not work and/or it will crash. Sorry.
I take the NACK back if you explain how you can store a variable (not a constant) in ROM.
Regards, Carl-Daniel
On Sun, Aug 10, 2008 at 12:57:42AM +0200, Carl-Daniel Hailfinger wrote:
-static int console_loglevel(void) +static int loglevel = CONFIG_DEFAULT_CONSOLE_LOGLEVEL;
NACK that part. It will not work and/or it will crash. Sorry.
I take the NACK back if you explain how you can store a variable (not a constant) in ROM.
Doesn't it live on the heap/stack in cache?
//Peter
On 10.08.2008 01:15, Peter Stuge wrote:
On Sun, Aug 10, 2008 at 12:57:42AM +0200, Carl-Daniel Hailfinger wrote:
-static int console_loglevel(void) +static int loglevel = CONFIG_DEFAULT_CONSOLE_LOGLEVEL;
NACK that part. It will not work and/or it will crash. Sorry.
I take the NACK back if you explain how you can store a variable (not a constant) in ROM.
Doesn't it live on the heap/stack in cache?
Unfortunately not. It's a global variable (so not on stack) and we do nothing to allow global variables. Look at the hoops the printk buffer code is jumping through to get some sort of global variables.
The good news is that the printk buffer helpers can be amended to store any number of global variables.
Regards, Carl-Daniel
On 10.08.2008 01:20, Carl-Daniel Hailfinger wrote:
On 10.08.2008 01:15, Peter Stuge wrote:
On Sun, Aug 10, 2008 at 12:57:42AM +0200, Carl-Daniel Hailfinger wrote:
-static int console_loglevel(void) +static int loglevel = CONFIG_DEFAULT_CONSOLE_LOGLEVEL;
NACK that part. It will not work and/or it will crash. Sorry.
I take the NACK back if you explain how you can store a variable (not a constant) in ROM.
Doesn't it live on the heap/stack in cache?
Unfortunately not. It's a global variable (so not on stack) and we do nothing to allow global variables. Look at the hoops the printk buffer code is jumping through to get some sort of global variables.
The good news is that the printk buffer helpers can be amended to store any number of global variables.
Patch for a temporary fixup follows.
Variables can't be stored in ROM. It's called readonly for a reason.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: corebootv3-console_loglevel_global_revert/lib/console.c =================================================================== --- corebootv3-console_loglevel_global_revert/lib/console.c (Revision 729) +++ corebootv3-console_loglevel_global_revert/lib/console.c (Arbeitskopie) @@ -8,8 +8,6 @@ int vtxprintf(void (*)(unsigned char, void *arg), void *arg, const char *, va_list);
-static unsigned int loglevel = CONFIG_DEFAULT_CONSOLE_LOGLEVEL; - /** * set the console log level * There are no invalid settings, although there are ones that @@ -21,7 +19,7 @@ if (level > BIOS_SPEW) printk(BIOS_ALWAYS, "Warning: ridiculous log level setting: %d (max %d)\n", level, BIOS_SPEW); - loglevel = level; +#warning Setting the log level is unimplemented }
/** @@ -31,7 +29,8 @@ */ static unsigned int console_loglevel(void) { - return loglevel; +#warning This always returns the default loglevel + return CONFIG_DEFAULT_CONSOLE_LOGLEVEL; }
#ifdef CONFIG_CONSOLE_BUFFER
I see what you're doing here but -- let's fix it. Where to we put the printk log buffer -- just make it the first 32 bits of it or something. Anything.
Besides, if we leave it a variable, we can patch it in flash. Even that's better than what we have now.
So please let's leave this in.
ron
On 10.08.2008 01:44, ron minnich wrote:
I see what you're doing here but -- let's fix it. Where to we put the printk log buffer -- just make it the first 32 bits of it or something. Anything.
Besides, if we leave it a variable, we can patch it in flash. Even that's better than what we have now.
So please let's leave this in.
Stand by for a generic global variable mechanism. I'm working on that one furiously.
Regards, Carl-Daniel
On 10.08.2008 02:09, Carl-Daniel Hailfinger wrote:
On 10.08.2008 01:44, ron minnich wrote:
I see what you're doing here but -- let's fix it. Where to we put the printk log buffer -- just make it the first 32 bits of it or something. Anything.
Besides, if we leave it a variable, we can patch it in flash. Even that's better than what we have now.
So please let's leave this in.
Stand by for a generic global variable mechanism. I'm working on that one furiously.
Posted in a new thread.
By the way, v3 and K8 are an explosive combination at the moment. If you want it to survive, disable CONFIG_CONSOLE_BUFFER or you will clobber your stack! I'll post a patch to add printk buffer support to K8 sometime in the next few days.
Regards, Carl-Daniel
leave it in. It won't crash, it simply won't work. Somebody fix it so it gets attached to the printk buffer and works.
What we have now is not what we want. We don't compile out a lot of printks and we can't use them. It's the worst case.
ron
-
Carl-Daniel Hailfinger -
Peter Stuge -
ron minnich