[coreboot] pci and console changes
Peter Stuge
peter at stuge.se
Sat Aug 9 22:37:26 CEST 2008
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 at gmail.com>
If you take a pass through the comments, it is:
Acked-by: Peter Stuge <peter at 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
More information about the coreboot
mailing list