[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 <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