[coreboot] r1009 - in coreboot-v3: mainboard/kontron/986lcd-m northbridge/intel/i945 southbridge/intel/i82801gx

[Stefan Reinauer]

ron minnich wrote:
> On Wed, Nov 12, 2008 at 3:58 PM, Peter Stuge <peter at stuge.se> wrote:
>   
>> svn at 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