Re: [coreboot] r1009 - in coreboot-v3: mainboard/kontron/986lcd-m northbridge/intel/i945 southbridge/intel/i82801gx
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 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?)
+++ coreboot-v3/mainboard/kontron/986lcd-m/dts 2008-11-12 23:09:42 UTC (rev 1009) @@ -181,6 +181,13 @@ pci@1f,0{/* which ich? */ /config/("southbridge/intel/i82801gx/ich7m_dh_lpc.dts"); };
pci@1f,2{/config/("southbridge/intel/i82801gx/sata.dts");};pci@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.
-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.
/* 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..
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.
+++ 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.
+void i82801gx_enable(struct device * dev);
Here it is again. It appears a few more times.
//Peter
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 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@1f,0{/* which ich? */ /config/("southbridge/intel/i82801gx/ich7m_dh_lpc.dts"); };
pci@1f,2{/config/("southbridge/intel/i82801gx/sata.dts");};pci@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
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
On Wed, Nov 12, 2008 at 6:07 PM, Stefan Reinauer stepan@coresystems.de wrote:
Absolutely right.
I'm fine with going back. Are there any objections to just building in all bits of a chip, even if all the parts are not even used? (oh! see below! I feel happy about things now :-)
How so? Just having one for "include all code for that southbridge" is fine.
Are you sure? If so that's fine with me!
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.
no, it's a totally different problem.
in v2, we defined the struct in chip.h and set settings in Config.lb.
In v3, dtc generates the struct. So you can't build ide.c without referencing ide.dts. So we solved the 'we can't set default values in chip.h' but created a different problem. Why is it always that way :-)
I disagree. Nobody would ever want to include the code of half a southbridge.
as long as we are sure about that, there is no problem. See the patch I sent. It sets it all back.
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
but that's different. (to me).
in southbridge/intel/i82371eb/Makefile. Why would that not be appropriate for the ich7?
Sure it's fine.
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?
it was about the fact that cs5536.c was one file that included all the devices. People stated a preference for one file per device.
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.
That is *good* to hear. You just simplified our life tremendously.
So do we say this:
if you have a chip, such as the i82801gx, then all the files for that chip get compiled in; and all the dts files in that directory should be used in the mainboard dts.
Will that rule be ok?
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.
uh oh. Did I do that too?
Actually, if we do make one set of decisions about build rules out of this discussion, I am glad I stirred the bees nest with that commit :)
thanks
ron
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@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
On Wed, Nov 12, 2008 at 6:57 PM, Peter Stuge peter@stuge.se wrote:
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?
because a board might not have all of them and they might not want to compile them all in.
This is as unexpected for me as it is for you. Chips with this many devices are kind of new to me. There are so many devices on that part that it really strains our ideas about dts.
The Hamburg design is in need of further work.
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?
Sure, if we don't mind bringing forward the v2 config program. I don't think we want that.
- pci@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.
OK, but this is a change, and recall that if the component (can we please just call this a chip) dts includes all the device dtses, you now have the problem of what to do when some of them are turned off and in one case (a la via) the occupy the same pci bus/dev/fn. This is kind of tricky. This is also a change. Again.
So let's see a proposal and see what we can do.
-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.
The v3 model? But this adheres to the v3 model. It will build fine.
Also if the code was only casually reviewed before going into v2, maybe more effort would be suitable for v3.
Sure, but who's going to do it? We have lots of work to do, not all the people we might want.
Saying "more effort would be suitable" is not sufficient to getting the work done.
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.
OK, here's the challenge from me: it seems obvious it can be fixed. So fix it. Clean it up. :-)
I won't mind :-)
STAGE2_CHIPSET_SRC += \
$(src)/southbridge/intel/i82801gx/ac97.c \This has to change back. Any solution that does not keep this list>
//Peter
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
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.
and we're back to that time thing. It's not going to get us anywhere to say "fix this" absent the knowledge of Why Things Are The Way They Are.
Anyway, I changed it back.
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.
And you would do this how?
What do you think of the "multi tier" approach?
I think it needs a convincing demo that it can work.
I think we're all agreed that the current setup is not quite what we want. What's happening is that the integration of lots of devices on chip is overtaking our pretty dts ideas. It's going to get worse. The new pci "extended function space" will allow dozens of devices on chip. What we're doing now won't work.
+++ 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.
But Stefan likes it :-) you will have to argue with him :-)
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.
That may well be.
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?
Um, Peter, I believe you are one of the guys who complained about this :-)
If not, sorry. :-)
Anyway, I think the current dts approach is not workable. We need to try One More Time.
The only good news: all the code to date will continue to work, even if we extend the dts. Thank goodness.
I'm going back to Blue Gene for a while -- but you smart guys need to really think about this. It's hard.
ron
-
Peter Stuge -
ron minnich -
Stefan Reinauer