[coreboot] r1009 - in coreboot-v3: mainboard/kontron/986lcd-m northbridge/intel/i945 southbridge/intel/i82801gx
ron minnich
rminnich at gmail.com
Thu Nov 13 02:38:14 CET 2008
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 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
> 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.
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. 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. But to tailor
the code, we either reference it in the makefile for the mainboard, or
we have lots of control variables.
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.
>
>
>> +++ coreboot-v3/mainboard/kontron/986lcd-m/dts 2008-11-12 23:09:42 UTC (rev 1009)
>> @@ -181,6 +181,13 @@
>> pci at 1f,0{/* which ich? */
>> /config/("southbridge/intel/i82801gx/ich7m_dh_lpc.dts");
>> };
>> + pci at 1f,2{
>> + /config/("southbridge/intel/i82801gx/sata.dts");
>> + };
>> + pci at 1f,3{
>> + /config/("southbridge/intel/i82801gx/smbus.dts");
>> + };
>
> I'd like the same principle for dts. More reuse and one single
> include line pulling in whatever is needed for one complex device.
Do you mean device or chip? Again, this was discussed and this is how
we decided to go. One dts per device.
>
>
>> -static void pci_domain_set_resources(struct device * dev)
>> +static void I945_pci_domain_set_resources(struct device * dev)
>
> This is just one example, there are many cases of copypaste like
> this. Is it really neccessary? I think the design has failed if we
> can't reuse this kind of code.
I am copying this from v2. v2 has been acked. The principle is that if
it was acked in v2, we can consider it acked in v3. That said, I don't
understand why the code is this way.
>
>
>> /* Report the memory regions */
>> - ram_resource(dev, 3, 0, 640);
>> - ram_resource(dev, 4, 768, (tolmk - 768));
>> + i945_ram_resource(dev, 3, 0, 640);
>> + i945_ram_resource(dev, 4, 768, (tolmk - 768));
>> if (tomk > 4 * 1024 * 1024) {
>> - ram_resource(dev, 5, 4096 * 1024, tomk - 4 * 1024 * 1024);
>> + i945_ram_resource(dev, 5, 4096 * 1024, tomk - 4 * 1024 * 1024);
>
> So what's the difference? I just cut the i945_ram_resource() changes
> out, but..
I don't know.
>
>
>> 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.
Any recommendation that makes sense is welcome.
For example, we could have 11 control variables to selectively include
the files we want.
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.
OK, I broke it out. Now, before we decide that breaking it out was
bad, let's make sure this time. :-)
>> +++ 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.
>
>
>> +void i82801gx_enable(struct device * dev);
>
> Here it is again. It appears a few more times.
Yep, It's because the include file can't be included in stage1 and
stage2 if that function is left in.
This code is rough. Every time we bring forward a new part, we learn
some things. That's why I'm doing it. My committing this stuff is not
an endorsement of the structure or design.
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.
The rule of one dts for one device is simple. I like simple rules. It
was also the result of a long argument. I did not design it that way
initially -- I changed it at people's request.
If we want to go back to one dts/file per chip, fine; see the
southbridge/amd/cs5536/cs5536.c which, IIRC, a number of people were
not happy with; that unhappiness is why I tried the experiment of
splitting things out in subsequent chips.
Thanks
ron
More information about the coreboot
mailing list