Hi there,
the intel southbridge drivers are a mess.
I suggest to do the following:
cd coreboot/src/southbridge svn mv i82801ca i82801cx svn mv i82801dbm i82801dx svn mv i82801er i82801ex svn copy i82801xx i82801bx svn mv i82801xx i82801ax
(plus, fixing up the filenames in these directories and the romstage.c and Kconfig files of the mainboards using those drivers)
And then cleaning up all the code undermining the driver infrastructure in the 82801xx driver, so we can drop the spaghetti code that checks for PCI devices in actual init functions all over the place.
Why? Because it is simply unmaintainable. The unified driver is not unified, it's rather incomplete enough so nobody noticed. The ICH4 and ICH4M are very similar. So are the ICH5 and ICH5M and ICH5R. It's a good idea to have a "unified" driver per chipset generation. But not across several generations of chipsets.
In order to fix graphics output for the i830/ich4 combination I'd like to add an SMM handler and MBI interface to the ICH4 specific version of the code. But there's not much gain in trying to check ICH0 datasheets to see if what was never intended to be done on that chipset is possible. And if I go and add guards for PCI IDs in the code, I'll rather clean up the code to get it do the PCI ID matching that coreboot intended for such cases instead of spaghettiing them into runtime checks.
I don't think that a shared driver like this is worth the ugliness that comes with it. It's fair to say that if you don't know if you have an ICH0 or ICH4, you can't do a coreboot port, because you will fail in so many other places before and after that.
So folks, if you agree that we should clean up coreboot and make it easier for people, send me an Acked-by: for the above idea.
Signed-off-by: Stefan Reinauer stepan@coresystems.de
On 02/20/2010 05:32 PM, Stefan Reinauer wrote:
Hi there,
the intel southbridge drivers are a mess.
I suggest to do the following:
cd coreboot/src/southbridge svn mv i82801ca i82801cx svn mv i82801dbm i82801dx svn mv i82801er i82801ex svn copy i82801xx i82801bx svn mv i82801xx i82801ax
(plus, fixing up the filenames in these directories and the romstage.c and Kconfig files of the mainboards using those drivers)
And then cleaning up all the code undermining the driver infrastructure in the 82801xx driver, so we can drop the spaghetti code that checks for PCI devices in actual init functions all over the place.
Why? Because it is simply unmaintainable. The unified driver is not unified, it's rather incomplete enough so nobody noticed. The ICH4 and ICH4M are very similar. So are the ICH5 and ICH5M and ICH5R. It's a good idea to have a "unified" driver per chipset generation. But not across several generations of chipsets.
In order to fix graphics output for the i830/ich4 combination I'd like to add an SMM handler and MBI interface to the ICH4 specific version of the code. But there's not much gain in trying to check ICH0 datasheets to see if what was never intended to be done on that chipset is possible. And if I go and add guards for PCI IDs in the code, I'll rather clean up the code to get it do the PCI ID matching that coreboot intended for such cases instead of spaghettiing them into runtime checks.
I don't think that a shared driver like this is worth the ugliness that comes with it. It's fair to say that if you don't know if you have an ICH0 or ICH4, you can't do a coreboot port, because you will fail in so many other places before and after that.
So folks, if you agree that we should clean up coreboot and make it easier for people, send me an Acked-by: for the above idea.
Signed-off-by: Stefan Reinauer stepan@coresystems.de
You have my Acked-by: Joseph Smith joe@settoplinux.org
But you may want to hear from others first.
On Sat, 20 Feb 2010 17:39:03 -0500, Joseph Smith joe@settoplinux.org wrote:
On 02/20/2010 05:32 PM, Stefan Reinauer wrote:
Hi there,
the intel southbridge drivers are a mess.
I suggest to do the following:
cd coreboot/src/southbridge svn mv i82801ca i82801cx svn mv i82801dbm i82801dx svn mv i82801er i82801ex svn copy i82801xx i82801bx svn mv i82801xx i82801ax
(plus, fixing up the filenames in these directories and the romstage.c and Kconfig files of the mainboards using those drivers)
And then cleaning up all the code undermining the driver infrastructure in the 82801xx driver, so we can drop the spaghetti code that checks for PCI devices in actual init functions all over the place.
Why? Because it is simply unmaintainable. The unified driver is not unified, it's rather incomplete enough so nobody noticed. The ICH4 and ICH4M are very similar. So are the ICH5 and ICH5M and ICH5R. It's a good idea to have a "unified" driver per chipset generation. But not across several generations of chipsets.
In order to fix graphics output for the i830/ich4 combination I'd like to add an SMM handler and MBI interface to the ICH4 specific version of the code. But there's not much gain in trying to check ICH0 datasheets to see if what was never intended to be done on that chipset is possible. And if I go and add guards for PCI IDs in the code, I'll rather clean up the code to get it do the PCI ID matching that coreboot intended for such cases instead of spaghettiing them into runtime
checks.
I don't think that a shared driver like this is worth the ugliness that comes with it. It's fair to say that if you don't know if you have an ICH0 or ICH4, you can't do a coreboot port, because you will fail in so many other places before and after that.
So folks, if you agree that we should clean up coreboot and make it easier for people, send me an Acked-by: for the above idea.
Signed-off-by: Stefan Reinauer stepan@coresystems.de
You have my Acked-by: Joseph Smith joe@settoplinux.org
But you may want to hear from others first.
Anyone else want to comment on this?
On Sat, Feb 20, 2010 at 11:32:45PM +0100, Stefan Reinauer wrote:
Hi there,
the intel southbridge drivers are a mess.
Ack.
Why? Because it is simply unmaintainable. The unified driver is not unified, it's rather incomplete enough so nobody noticed. The ICH4 and ICH4M are very similar. So are the ICH5 and ICH5M and ICH5R. It's a good idea to have a "unified" driver per chipset generation. But not across several generations of chipsets.
Hm, I think you're right.
So folks, if you agree that we should clean up coreboot and make it easier for people, send me an Acked-by: for the above idea.
Signed-off-by: Stefan Reinauer stepan@coresystems.de
Acked-by: Uwe Hermann uwe@hermann-uwe.de
However, there are some follow-up TODOs. All of those SBs have missing license headers and partly inconsistent coding-style (which is to be expected though, as the coding guidelines were not in place back then). I might be able to take care of that sooner or later, but any help is welcome.
My main worry with this kind of stuff is that we get a certain degree of code duplication. As you correctly noted some of that just _looks_ duplicated as the i82801xx driver is incomplete. But there's also stuff which _is_ generic for most ICHs. Maybe we can later factor out some of that to some ichlib.c or something like that. For example the code in i82801*_early_smbus.c is likely a good candidate (didn't check the datasheets closely yet, though).
Uwe.
On Mon, Feb 22, 2010 at 6:57 AM, Uwe Hermann uwe@hermann-uwe.de wrote:
My main worry with this kind of stuff is that we get a certain degree of code duplication. As you correctly noted some of that just _looks_ duplicated as the i82801xx driver is incomplete. But there's also stuff which _is_ generic for most ICHs. Maybe we can later factor out some of that to some ichlib.c or something like that. For example the code in i82801*_early_smbus.c is likely a good candidate (didn't check the datasheets closely yet, though).
How many of these parts are still made? And, how many of them have started to diverge from data sheets? I worry about the case that somebody changes ichlib.c for one part and it breaks another. I think this is one case where code duplication is a good thing.
ron
On 2/22/10 3:57 PM, Uwe Hermann wrote:
So folks, if you agree that we should clean up coreboot and make it easier for people, send me an Acked-by: for the above idea.
Signed-off-by: Stefan Reinauer stepan@coresystems.de
Acked-by: Uwe Hermann uwe@hermann-uwe.de
Awesome, thanks a lot!
However, there are some follow-up TODOs. All of those SBs have missing license headers and partly inconsistent coding-style (which is to be expected though, as the coding guidelines were not in place back then). I might be able to take care of that sooner or later, but any help is welcome.
Yes, we have massive cleanups to do, especially now that we got rid of the old config system, too.
I'll try to clean up things on the go, as I do other stuff, but I am glad to see how much easier it became to clean up and to the right thing now.
My main worry with this kind of stuff is that we get a certain degree of code duplication. As you correctly noted some of that just _looks_ duplicated as the i82801xx driver is incomplete. But there's also stuff which _is_ generic for most ICHs. Maybe we can later factor out some of that to some ichlib.c or something like that. For example the code in i82801*_early_smbus.c is likely a good candidate (didn't check the datasheets closely yet, though).
My main concern is that some things (like USB bug workarounds in ICH7) are very hard to figure out for more than one chipset. Even if the exact registers with the same name and meaning exists across chipsets, it might still be wrong to set such a register to a value that is a hard requirement on the other chipset with that register. As for the code duplication, I don't think we should create "libs" too soon; but there is an incredible amount of code that could be dropped by moving code from across the trees into their "drivers". Like a lot of romstage.c files have half the fallback and cache as ram magic built in....
Well, we'll see... I'll try to do the renaming orgy in the next two days...
Stefan
On Mon, 22 Feb 2010 17:53:59 +0100, Stefan Reinauer stepan@coresystems.de wrote:
On 2/22/10 3:57 PM, Uwe Hermann wrote:
So folks, if you agree that we should clean up coreboot and make it easier for people, send me an Acked-by: for the above idea.
Signed-off-by: Stefan Reinauer stepan@coresystems.de
Acked-by: Uwe Hermann uwe@hermann-uwe.de
Awesome, thanks a lot!
However, there are some follow-up TODOs. All of those SBs have missing license headers and partly inconsistent coding-style (which is to be expected though, as the coding guidelines were not in place back then). I might be able to take care of that sooner or later, but any help is welcome.
I would be glad to help out, as soon as Stefan starts the orgy :-)
-
Joseph Smith -
ron minnich -
Stefan Reinauer -
Uwe Hermann