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

[Peter Stuge]

ron minnich wrote:
> >> +++ coreboot-v3/mainboard/kontron/986lcd-m/Makefile   2008-11-12 23:09:42 UTC (rev 1009)
..
> >> +STAGE2_CHIPSET_SRC=\
> >> +                                     $(src)/southbridge/intel/i82801gx/ac97.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.

Hmm, how come? Because a board dts might not have all of them?


> That is no less wordy, and no less convenient, than just listing
> the files in the mainboard makefile.

Agree, that's no good either.


> 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.

Could they be set automatically? The dts dictates what code we need,
couldn't it be used also to create build dependencies?


> 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.

Not neccessarily the best way. Just an idea. We need to improve.


> >> + 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.

Actually, I would like both. :) A component dts (geodelx, cs5536, c7,
vt8237, all superios) could include atom device dtses, or might
choose not to. A mainboard dts includes complex component dtses.


> >> -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.

Aha! I don't really with that universally. My interpretation of the
principle is that code that accomplishes something good in v2 could
be moved over to v3 without separate review, but not without making
sure it adheres to the v3 model. Also if the code was only casually
reviewed before going into v2, maybe more effort would be suitable
for v3.


> That said, I don't understand why the code is this way.

That's the reason I don't think it should be committed as is.


> >>  STAGE2_CHIPSET_SRC += \
> >> -                                     $(src)/southbridge/intel/i82801gx/ac97.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.

My only problem at the moment is that I don't have a real good
overview of the v3 Makefile layout, and no time to study it.


> For example, we could have 11 control variables to selectively
> include the files we want.

I wouldn't mind that if they were set automatically.


> 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. :-)

What do you think of the "multi tier" approach?


> >> +++ 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.

Spot on. Remember what Marc said. This is our chance to get it right.


> 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.

That makes a lot of sense. Thanks for spelling it out. But at the
same time it implicitly endorses both structure and design because
effort is put into adapting code to fit them, and because with every
added part there is a lot more work to do for any major changes that
we want to do. (And I think there are a few.)


> What's interesting to me is how many times unchanged v2 code is
> found unacceptable for v3 -- is anyone looking at v2?

Honestly I don't think anyone is. I'm not. I think there are a great
many things that should be fixed before code goes over into v3.


> -- or we keep revisiting decisions we made already.

Which is just what I think we should do for v3. :)


> 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.

Maybe the rule is too simple.


> 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.

Hm, you mean because there is code in there for different functions
of the same component? I don't think that is so bad?


//Peter