Re: [coreboot] pci and console changes

[Peter Stuge]

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

If you take a pass through the comments, it is:

Acked-by: Peter Stuge
> 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