ron minnich wrote:
On Wed, Nov 12, 2008 at 3:58 PM, Peter Stuge peter@stuge.se wrote:
svn@coreboot.org wrote:
+++ coreboot-v3/mainboard/kontron/986lcd-m/Makefile 2008-11-12 23:09:42 UTC (rev 1009) @@ -28,6 +28,18 @@ INITRAM_SRC= $(src)/mainboard/$(MAINBOARDDIR)/initram.c \ $(src)/northbridge/intel/i945/raminit.c \
+STAGE2_CHIPSET_SRC=\
$(src)/southbridge/intel/i82801gx/ac97.c \$(src)/southbridge/intel/i82801gx/lpc.c \$(src)/southbridge/intel/i82801gx/nic.c \$(src)/southbridge/intel/i82801gx/pci.c \$(src)/southbridge/intel/i82801gx/pcie.c \$(src)/southbridge/intel/i82801gx/sata.c \$(src)/southbridge/intel/i82801gx/smbus.c \$(src)/southbridge/intel/i82801gx/usb_ehci.c \$(src)/southbridge/intel/i82801gx/usb.c \$(src)/southbridge/intel/i82801gx/watchdog.c+# $(src)/southbridge/intel/i82801gx/libsmbus.c \
We must fix this design. This list of files has to go in southbridge/intel/i82801gx/ somewhere
Absolutely right.
and mainboard/ Makefiles should just reference the right south Makefile per Kconfig settings, preferably using one common rule.
Did we consider using the scheme that Linux uses to pick which objects to build? STAGE2_CHIPSET_SRC-$(CONFIG_INTEL_I82801GX) += srcfiles
We considered it and decided not to do it. But the problem is a different one.
in south/ and always include every Makefile in every build. (Maybe that's done anyway?)
We already include every makefile in every build.
not the issue. The issue is you're going to need a control variable for a.most every file in the southbridge. That is no less wordy, and no less convenient, than just listing the files in the mainboard makefile.
How so? Just having one for "include all code for that southbridge" is fine.
There's another problem. If you don't reference ide in the dts, then the ide.c won't compile -- no config struct for it.
Darn. We're back and re-implemented the v2 nastyness with a completely different set of tools. That's really a pity.
So you have to build the ide.c in and reference it in the dts. I don't like that -- people should not have to include code they don't want.
How does that go with the idea of "not having defines, because it is bios code it should all be one code flow"?
But to tailor the code, we either reference it in the makefile for the mainboard, or we have lots of control variables.
I disagree. Nobody would ever want to include the code of half a southbridge.
I definitely don't want to revisit this argument: STAGE2_CHIPSET_SRC-$(CONFIG_INTEL_I82801GX) += srcfiles
we've been there and decided not to. I don't wish to rehash.
I don't understand what you're trying to say. We're doing
ifeq ($(CONFIG_SOUTHBRIDGE_INTEL_I82371EB),y)
STAGE2_CHIPSET_SRC += $(src)/southbridge/intel/i82371eb/i82371eb.c STAGE2_CHIPSET_SRC += $(src)/southbridge/intel/i82371eb/i82371eb_foo.c STAGE2_CHIPSET_SRC += $(src)/southbridge/intel/i82371eb/i82371eb_bar.c STAGE2_CHIPSET_SRC += $(src)/southbridge/intel/i82371eb/i82371eb_blubb.c endif
in southbridge/intel/i82371eb/Makefile. Why would that not be appropriate for the ich7?
Modified: coreboot-v3/southbridge/intel/i82801gx/Makefile
--- coreboot-v3/southbridge/intel/i82801gx/Makefile 2008-11-12 22:43:50 UTC (rev 1008) +++ coreboot-v3/southbridge/intel/i82801gx/Makefile 2008-11-12 23:09:42 UTC (rev 1009) @@ -24,18 +24,6 @@ STAGE2_CHIPSET_SRC += $(src)/southbridge/intel/i82801gx/i82801gx.c
STAGE2_CHIPSET_SRC += \
$(src)/southbridge/intel/i82801gx/ac97.c \$(src)/southbridge/intel/i82801gx/ide.c \$(src)/southbridge/intel/i82801gx/lpc.c \$(src)/southbridge/intel/i82801gx/nic.c \$(src)/southbridge/intel/i82801gx/pci.c \$(src)/southbridge/intel/i82801gx/pcie.c \$(src)/southbridge/intel/i82801gx/sata.c \$(src)/southbridge/intel/i82801gx/smbus.c \$(src)/southbridge/intel/i82801gx/usb_ehci.c \$(src)/southbridge/intel/i82801gx/usb.c \$(src)/southbridge/intel/i82801gx/watchdog.c-# $(src)/southbridge/intel/i82801gx/libsmbus.c \
This has to change back. Any solution that does not keep this list of files in southbridge/ is plain wrong. Sorry.
Ack.
Any recommendation that makes sense is welcome.
For example, we could have 11 control variables to selectively include the files we want.
What for? Please describe the problem you're seeing with above suggestion.
Remember, long ago, however, I was putting everything in one file because of this kind of situation. The sense of the community was that we should break it out.
I don't remember what this was about. pci drivers in v3?
OK, I broke it out. Now, before we decide that breaking it out was bad, let's make sure this time. :-)
While it makes sense to have one "driver" in one file to make the code readable, it sure does not make much sense to choose to have the ich7 usb driver in the bios image but not the ich7 ehci driver.
+++ coreboot-v3/southbridge/intel/i82801gx/ac97.c 2008-11-12 23:09:42 UTC (rev 1009) +void i82801gx_enable(struct device * dev);
What? This function has to get a better name.
It's the v2 name. I'm not going to change it.
It's the function that supposedly enables/disables devices on the i82801gx chip. So the name is actually very fitting. I'm not using it in that sense in the ICH7 code because I saw weird effects and started shortcutting it.
What's interesting to me is how many times unchanged v2 code is found unacceptable for v3 -- is anyone looking at v2? -- or we keep revisiting decisions we made already.
It's so much easier to bitch about v2 and dream about v3 than to look and learn from v2 and not doing micro-optimizations in v3. We've been endorsing this behavior, unfortunately.
Stefan