[coreboot] pci and console changes

ron minnich rminnich at gmail.com
Sat Aug 9 23:11:43 CEST 2008


On Sat, Aug 9, 2008 at 1:37 PM, Peter Stuge <peter at 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




More information about the coreboot mailing list