Re: [coreboot] r983 - in coreboot-v3: device include/device

[Carl-Daniel Hailfinger]

Hi Myles,

I'm sorry I didn't have time to review your patch a bit sooner.
AFAICS it introduced a few bugs regarding conditional compilation.
Review follows.

On 05.11.2008 23:27, [e-mail] wrote:
> Author: myles
> Date: 2008-11-05 23:27:36 +0100 (Wed, 05 Nov 2008)
> New Revision: 983
>
> Modified:
>    coreboot-v3/device/Makefile
>    coreboot-v3/device/cardbus_device.c
>    coreboot-v3/device/device.c
>    coreboot-v3/device/pci_device.c
>    coreboot-v3/device/pcie_device.c
>    coreboot-v3/include/device/cardbus.h
>    coreboot-v3/include/device/device.h
>    coreboot-v3/include/device/pci.h
>    coreboot-v3/include/device/pcie.h
> Log:
> This patch continues the device code cleanup.
>
> The largest changes are to get_pci_bridge_ops, and related changes to make it
> compile and use correct declarations.
>
> While I was doing that I moved the checks for CONFIG_<BUS>_PLUGIN_SUPPORT to
> the Makefile.
>
> The only functional difference is a possible NULL dereference in a debug
> statement.
>
> I also added a few more consts, now that my other patch is in.
>
> Signed-off-by: Myles Watson 
> Acked-by: Ronald G. Minnich
> Modified: coreboot-v3/device/Makefile
>
> --- coreboot-v3/device/Makefile	2008-11-05 22:18:53 UTC (rev 982)
> +++ coreboot-v3/device/Makefile	2008-11-05 22:27:36 UTC (rev 983)
> @@ -29,13 +29,27 @@
>    		smbus_ops.c
>
> # this is only needed on the K8
> +# This could also check for CONFIG_HYPERTRANSPORT_PLUGIN_SUPPORT
>  ifeq ($(CONFIG_NORTHBRIDGE_AMD_K8),y)
>  STAGE2_DEVICE_SRC += hypertransport.c
>  endif
>
> # this is only needed for pcix devices
> +# This should also check for CONFIG_PCIX_PLUGIN_SUPPORT
>  ifeq ($(CONFIG_SOUTHBRIDGE_AMD_AMD8132),y)
>  STAGE2_DEVICE_SRC += pcix_device.c
>  endif
>
> +ifeq ($(CONFIG_PCIE_PLUGIN_SUPPORT),y)
> +STAGE2_DEVICE_SRC += pcie_device.c
> +endif
>
> +ifeq ($(CONFIG_CARDBUS_PLUGIN_SUPPORT),y)
> +STAGE2_DEVICE_SRC += cardbus_device.c
> +endif
>
> +ifeq ($(CONFIG_AGP_PLUGIN_SUPPORT),y)
> +STAGE2_DEVICE_SRC += agp_device.c
> +endif
>
> $(obj)/device/pci_device.o: $(src)/device/pci_device.c $(obj)/statictree.h
> Modified: coreboot-v3/device/pci_device.c
> ===================================================================
> --- coreboot-v3/device/pci_device.c	2008-11-05 22:18:53 UTC (rev 982)
> +++ coreboot-v3/device/pci_device.c	2008-11-05 22:27:36 UTC (rev 983)
> @@ -30,28 +30,7 @@
>  #include <device/device.h>
>  #include <device/pci.h>
>  #include <device/pci_ids.h>
> -/* We should move these so they're really config options */
> -#define CONFIG_HYPERTRANSPORT_PLUGIN_SUPPORT 0
> -#define CONFIG_PCIX_PLUGIN_SUPPORT 0
> -#define CONFIG_PCIE_PLUGIN_SUPPORT 0
> -#define CONFIG_CARDBUS_PLUGIN_SUPPORT 0
> -#define CONFIG_AGP_PLUGIN_SUPPORT 0
>
> -#if CONFIG_HYPERTRANSPORT_PLUGIN_SUPPORT == 1
> -#include <device/hypertransport.h>
> -#endif
> -#if CONFIG_PCIX_PLUGIN_SUPPORT == 1
> -#include <device/pcix.h>
> -#endif
> -#if CONFIG_PCIE_PLUGIN_SUPPORT == 1
> -#include <device/pcie.h>
> -#endif
> -#if CONFIG_AGP_PLUGIN_SUPPORT == 1
> -#include <device/agp.h>
> -#endif
> -#if CONFIG_CARDBUS_PLUGIN_SUPPORT == 1
> -#include <device/cardbus.h>
> -#endif
>  #include <statictree.h>
>
> u8 pci_moving_config8(struct device *dev, unsigned int reg)
> @@ -801,26 +780,28 @@
> @param dev Pointer to the device structure of the bridge.
> @return Appropriate bridge operations.
>
> */
> -static struct device_operations *get_pci_bridge_ops(struct device *dev)
> +static const struct device_operations *get_pci_bridge_ops(struct device *dev)
>  {
> // unsigned int pos;
>
> -#if CONFIG_PCIX_PLUGIN_SUPPORT == 1
> pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> if (pos) {
>
> +#ifdef DEVICE_PCIX_H

That won't work the way you want it to work. The ifdef above will never
be true.
> unsigned int pcix_pos;
> pcix_pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> if (pcix_pos) {
> printk(BIOS_DEBUG, "%s subordinate bus PCI-X\n",
>       dev_path(dev));
> return &default_pcix_ops_bus;
> }
>
> #endif
> -#if CONFIG_AGP_PLUGIN_SUPPORT == 1
> +#ifdef DEVICE_AGP_H

Same here. The ifdef will never be true.
> /* How do I detect an PCI to AGP bridge? */
> +#warning AGP detection not implemented, so AGP bridge plugin not supported.
>
> #endif
> -#if CONFIG_HYPERTRANSPORT_PLUGIN_SUPPORT == 1
> pos = 0;
> while ((pos = pci_find_next_capability(dev, PCI_CAP_ID_HT, pos))) {
>
> +#ifdef DEVICE_HYPERTRANSPORT_H

And here.
> unsigned int ht_pos;
> ht_pos = 0;
> while ((ht_pos = pci_find_next_capability(dev, PCI_CAP_ID_HT, ht_pos))) {
> unsigned int flags;
> flags = pci_read_config16(dev, pos + PCI_CAP_FLAGS);
> flags = pci_read_config16(dev, ht_pos + PCI_CAP_FLAGS);
> if ((flags >> 13) == 1) {
> 	/* Host or Secondary Interface. */
> 	printk(BIOS_DEBUG,
>
> @@ -830,11 +811,12 @@
>    	}
>    }
>  #endif
> -#if CONFIG_PCIE_PLUGIN_SUPPORT == 1
> pos = pci_find_capability(dev, PCI_CAP_ID_PCIE);
> if (pos) {
>
> +#ifdef DEVICE_PCIE_H

And here.
> unsigned int pcie_pos;
> pcie_pos = pci_find_capability(dev, PCI_CAP_ID_PCIE);
> if (pcie_pos) {
> unsigned int flags;
> flags = pci_read_config16(dev, pos + PCI_EXP_FLAGS);
> flags = pci_read_config16(dev, pcie_pos + PCI_EXP_FLAGS);
> switch ((flags & PCI_EXP_FLAGS_TYPE) >> 4) {
> case PCI_EXP_TYPE_ROOT_PORT:
> case PCI_EXP_TYPE_UPSTREAM:
>
> @@ -864,7 +846,6 @@
>  static void set_pci_ops(struct device *dev)
>  {
>    struct device_operations *c;
>
> struct device_id id;
>
> if (dev->ops) {
> printk(BIOS_SPEW, "%s: dev %s already has ops of type %x\n",
>
> @@ -890,21 +869,30 @@
>    switch (dev->hdr_type & 0x7f) {	/* Header type. */
>    case PCI_HEADER_TYPE_NORMAL:	/* Standard header. */
>    	if ((dev->class >> 8) == PCI_CLASS_BRIDGE_PCI)
> 	goto bad;
> dev->ops = &default_pci_ops_dev;
> 	printk(BIOS_ERR,
> 	       "%s [%s] hdr_type %02x doesn't match"
> 	       "class %06x, ignoring.\n", dev_path(dev),
> 	       dev_id_string(&dev->id), dev->class >> 8,
> 	       dev->hdr_type);
> else
> 	dev->ops = &default_pci_ops_dev;
> break;
> case PCI_HEADER_TYPE_BRIDGE:
> if ((dev->class >> 8) != PCI_CLASS_BRIDGE_PCI)
> 	goto bad;
> dev->ops = get_pci_bridge_ops(dev);
> 	printk(BIOS_ERR,
> 	       "%s [%s] hdr_type %02x doesn't match"
> 	       "class %06x, ignoring.\n", dev_path(dev),
> 	       dev_id_string(&dev->id), dev->class >> 8,
> 	       dev->hdr_type);
> else
> 	dev->ops = get_pci_bridge_ops(dev);
> break;
>
> -#if CONFIG_CARDBUS_PLUGIN_SUPPORT == 1
> +#ifdef DEVICE_CARDBUS_H

And here.
> case PCI_HEADER_TYPE_CARDBUS:
>    	dev->ops = &default_cardbus_ops_bus;
>    	break;
>  #endif
>    default:
>      bad:
> if (dev->enabled) {
>   	printk(BIOS_ERR,
>   	       "%s [%s/%06x] has unknown header "

Regards,
Carl-Daniel

r983 - in coreboot-v3: device include/device

[Myles Watson]

> -----Original Message-----
> From: Carl-Daniel Hailfinger [mailto:[e-mail]]
> Sent: Thursday, November 06, 2008 3:36 AM
> To: [e-mail]; Myles Watson; ron minnich
> Subject: Re: [coreboot] r983 - in coreboot-v3: device include/device
>
> Hi Myles,
>
> I'm sorry I didn't have time to review your patch a bit sooner.
> AFAICS it introduced a few bugs regarding conditional compilation.
> Review follows.

Whoa.  You're right.  I guess it's the same as it was, since it was never
compiled in before either.

It brings up the interesting question of how this should work, since if you
try to compile in that support it breaks because it can't find the default
ops structures.

Should there be a table of default ops somewhere?  How do we compile in
support for plug ins?

Thanks,
Myles

r983 - in coreboot-v3: device include/device

[ron minnich]

v2 uses the #if X == 1

v3 uses #ifdef X

I doubt we want condifional compilation at all. coreboot is firmware,
not an OS, and we don't want lots of variant versions. I lean toward
yanking this type of conditional compilation.

thanks

ron

r983 - in coreboot-v3: device include/device

[Myles Watson]

> v2 uses the #if X == 1
>
> v3 uses #ifdef X
>
> I doubt we want condifional compilation at all. coreboot is firmware,
> not an OS, and we don't want lots of variant versions. I lean toward
> yanking this type of conditional compilation.

I think the reason this was originally done was for size.  There might be
some people who are unhappy with HT, PCIe, and PCIx support compiled in for
their embedded box.

That being said, if the size isn't an issue, I'm all for yanking the ifs.

Myles

r983 - in coreboot-v3: device include/device

[ron minnich]

On Thu, Nov 6, 2008 at 7:40 AM, Myles Watson  wrote:
> > v2 uses the #if X == 1
> >
> > v3 uses #ifdef X
> >
> > I doubt we want condifional compilation at all. coreboot is firmware,
> > not an OS, and we don't want lots of variant versions. I lean toward
> > yanking this type of conditional compilation.
>
> I think the reason this was originally done was for size.  There might be
> some people who are unhappy with HT, PCIe, and PCIx support compiled in for
> their embedded box.

Let me restate. I like the idea of NOT compiling in un-needed drivers.
I just wonder about the way it was done in device code.

Sorry, have not had breakfast yet, just got off the phone with comcast.

ron

r983 - in coreboot-v3: device include/device

[Vincent Legoll]

On Thu, Nov 6, 2008 at 4:40 PM, Myles Watson  wrote:
> > I doubt we want condifional compilation at all. coreboot is firmware,
> > not an OS, and we don't want lots of variant versions. I lean toward
> > yanking this type of conditional compilation.
>
> I think the reason this was originally done was for size.  There might be
> some people who are unhappy with HT, PCIe, and PCIx support compiled in for
> their embedded box.

Isn't all that kind of things doable via function pointers and link-time dead
code elimination ? That would achieve the no ifdeffery goal, and may be
cleaner code...

Am I on crack ?

r983 - in coreboot-v3: device include/device

[ron minnich]

On Thu, Nov 6, 2008 at 7:49 AM, Vincent Legoll  wrote:
> Isn't all that kind of things doable via function pointers and link-time dead
> code elimination ? That would achieve the no ifdeffery goal, and may be
> cleaner code...
>
> Am I on crack ?

no we can do something like that Hold on, it's still early for me, I'm
not awake yet.
ron

r983 - in coreboot-v3: device include/device

[Stefan Reinauer]

Vincent Legoll wrote:
> Isn't all that kind of things doable via function pointers and link-time dead
> code elimination ? That would achieve the no ifdeffery goal, and may be
> cleaner code...

Link time optimization would suggest we look into compiling with LLVM
instead of gcc.

Has anyone tried this, yet?

r983 - in coreboot-v3: device include/device

[Myles Watson]

> -----Original Message-----
> From: Stefan Reinauer [mailto:[e-mail]]
> Sent: Thursday, November 06, 2008 9:17 AM
> To: Vincent Legoll
> Cc: Myles Watson; ron minnich; Carl-Daniel Hailfinger;
> [e-mail]
> Subject: Re: [coreboot] r983 - in coreboot-v3: device include/device
>
> Vincent Legoll wrote:
> > Isn't all that kind of things doable via function pointers and link-time
>
> dead
> > code elimination ? That would achieve the no ifdeffery goal, and may be
> > cleaner code...
>
> Link time optimization would suggest we look into compiling with LLVM
> instead of gcc.
>
> Has anyone tried this, yet?

As a start that wouldn't change the code much and would still work with gcc,
we could have an enum or defines in the device code and a table where the
device code that was compiled in could initialize a pointer to its default
ops.  In practice it only affects dynamically detected bridges, but it would
be nice to support them.

Something like

#define DEVICE_PCI_DRIVER_NUM 1
#define DEVICE_HT_DRIVER_NUM 2
#define DEVICE_PCIE_DRIVER_NUM 1
#define DEVICE_PCIX_DRIVER_NUM 1
#define DEVICE_MAX_DRIVERS DEVICE_PCIX_DRIVER_NUM+1

static const device_operations * default_drivers[DEVICE_MAX_DRIVERS];

Then each device could set them:

default_drivers[DEVICE_HT_DRIVER_NUM] = &default_ht_ops_bus;

Or something similar.  Note that this is just dry coding.

Thanks,
Myles

Thanks,
Myles

r983 - in coreboot-v3: device include/device

[Vincent Legoll]

On Thu, Nov 6, 2008 at 5:16 PM, Stefan Reinauer  wrote:
> Vincent Legoll wrote:
> > Isn't all that kind of things doable via function pointers and link-time dead
> > code elimination ? That would achieve the no ifdeffery goal, and may be
> > cleaner code...
>
> Link time optimization would suggest we look into compiling with LLVM
> instead of gcc.

I was believing that current linkers discarded code of unused functions.
Would LLVM really be needed for that to happen ? Or are you thing of
more involved optimizations ?
> Has anyone tried this, yet?

Not me

r983 - in coreboot-v3: device include/device

[Stefan Reinauer]

Vincent Legoll wrote:
> On Thu, Nov 6, 2008 at 5:16 PM, Stefan Reinauer  wrote:
> > Vincent Legoll wrote:
> > > Isn't all that kind of things doable via function pointers and link-time dead
> > > code elimination ? That would achieve the no ifdeffery goal, and may be
> > > cleaner code...
> >
> > Link time optimization would suggest we look into compiling with LLVM
> > instead of gcc.
>
> I was believing that current linkers discarded code of unused functions.
> Would LLVM really be needed for that to happen ? Or are you thing of
> more involved optimizations ?

Any non-static functions in a file will stay in the final binary, unless
you do some trickery like -combine, but that usually requires some
changes to the build system.

llvm will kick those functions out at link time. As does the plan9
linker, as far as I understood.

r983 - in coreboot-v3: device include/device

[ron minnich]

On Thu, Nov 6, 2008 at 12:21 PM, Stefan Reinauer  wrote:
> As does the plan9
> linker, as far as I understood.

you got it :-)

ron

r983 - in coreboot-v3: device include/device

[Carl-Daniel Hailfinger]

On 06.11.2008 21:21, Stefan Reinauer wrote:
> Vincent Legoll wrote:
> > On Thu, Nov 6, 2008 at 5:16 PM, Stefan Reinauer  wrote:
> > > Vincent Legoll wrote:
> > > > Isn't all that kind of things doable via function pointers and link-time dead
> > > > code elimination ? That would achieve the no ifdeffery goal, and may be
> > > > cleaner code...
> > >
> > > Link time optimization would suggest we look into compiling with LLVM
> > > instead of gcc.
> >
> > I was believing that current linkers discarded code of unused functions.
> > Would LLVM really be needed for that to happen ? Or are you thing of
> > more involved optimizations ?
>
> Any non-static functions in a file will stay in the final binary, unless
> you do some trickery like -combine, but that usually requires some
> changes to the build system.

We already have CONFIG_WHOLE_PROGRAM_COMPILE in v3. I can make sure it
is available for all stages. Right now it is only used for initram.

Regards,
Carl-Daniel

r983 - in coreboot-v3: device include/device

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> > Any non-static functions in a file will stay in the final binary, unless
> > you do some trickery like -combine, but that usually requires some
> > changes to the build system.
>
> We already have CONFIG_WHOLE_PROGRAM_COMPILE in v3. I can make sure it
> is available for all stages. Right now it is only used for initram.

As I said, it requires changes to the build system. :-)

r983 - in coreboot-v3: device include/device

[Carl-Daniel Hailfinger]

On 06.11.2008 17:16, Stefan Reinauer wrote:
> Vincent Legoll wrote:
> > Isn't all that kind of things doable via function pointers and link-time dead
> > code elimination ? That would achieve the no ifdeffery goal, and may be
> > cleaner code...
>
> Link time optimization would suggest we look into compiling with LLVM
> instead of gcc.
>
> Has anyone tried this, yet?

I tested LLVM and the qemu target boots fine. However, the generated
code is ~30% bigger than gcc 4.2.1. According to some LLVM people, this
is due to the fact that nobody is working on making the generated code
small. Most people just want speed and don't care for code size.

And if we compile with -fwhole-program, gcc sizes go down even further.

Regards,
Carl-Daniel

r983 - in coreboot-v3: device include/device

[Carl-Daniel Hailfinger]

On 06.11.2008 14:26, Myles Watson wrote:
> > -----Original Message-----
> > From: Carl-Daniel Hailfinger [mailto:[e-mail]]
> > Sent: Thursday, November 06, 2008 3:36 AM
> > To: [e-mail]; Myles Watson; ron minnich
> > Subject: Re: [coreboot] r983 - in coreboot-v3: device include/device
> >
> > Hi Myles,
> >
> > I'm sorry I didn't have time to review your patch a bit sooner.
> > AFAICS it introduced a few bugs regarding conditional compilation.
> > Review follows.
>
> Whoa.  You're right.  I guess it's the same as it was, since it was never
> compiled in before either.
>
> It brings up the interesting question of how this should work, since if you
> try to compile in that support it breaks because it can't find the default
> ops structures.

I believe that's why the original code had this:

/* We should move these so they're really config options */
#define CONFIG_HYPERTRANSPORT_PLUGIN_SUPPORT 0
#define CONFIG_PCIX_PLUGIN_SUPPORT 0
#define CONFIG_PCIE_PLUGIN_SUPPORT 0
#define CONFIG_CARDBUS_PLUGIN_SUPPORT 0
#define CONFIG_AGP_PLUGIN_SUPPORT 0

Had these been made real config options instead of being dropped,
everything should have worked.
> Should there be a table of default ops somewhere?  How do we compile in
> support for plug ins?

See above.

Regards,
Carl-Daniel