At 4:11 PM -0600 21/7/03, Eric W. Biederman wrote:
ron minnich rminnich@lanl.gov writes:
On Mon, 21 Jul 2003, Stefan Reinauer wrote:
we're not going to fork.
That was more of a warning about what could happen. We have lots of committers out there and more are going to join. We may not always agree on how things are done, and we must not get into the habit of ripping each other's code out -- that way lies madness.
There are some major design decisions coming before we freeze the 2.0 core, and this is just a warning that we need to be looking at these things closely, and that there are some strong opinions out there. I never had any intention of forking.
The target is to have a hardware independent hardwaremain.c
We have called a cease fire for a until I get back with Ron and Greg promising to look at what I have done with the device stuff.
If a enumerate_static_devices can be written that converts from the chip tree to the device tree and we get everyone working with and familiar with both pieces of the code we should be able to have some more rational discussion about what needs to happen.
Conversations beyond this need to start hitting the list.
Eric
The issue for me is that I need to have static initialization working before I can continue, so my intention is build something that will hopefully be acceptable to everyone. I can probably merge the chip and device structures that gives me a way forward without replicating what Eric is doing. However a single call to enumerate_static_devices, particularly at the current location, is NOT acceptable to me.
Here's my suggestion to move forward:
1. I need to be able to enable/configure static devices at specific points in hardwaremain, particularly on entry, after console_init and prior to pci enumeration. I suggest a routine called device_configure() that is passed the root of the device tree and a pass flag:
void device_configure(struct device *, enum dev_pass)
enum dev_pass { DEV_PASS_PRE_CONSOLE, DEV_PASS_PRE_PCI, DEV_PASS_ENUMERATE, DEV_PASS_CONFIGURE, DEV_PASS_ENABLE, DEV_PASS_INITIALIZE };
Calls to device_configure() with the appropriate flag will be inserted in the appropriate locations in hardwaremain. More flags/calls call be added if necessary. Calls to dev_enumerate(), dev_configure(), etc. to probe and configure dynamic bus devices will be replaced with calls to device_configure() with the appropriate flag. Dynamic devices will be added to the device tree as appropriate.
2. New fields added to struct device, including a 'static' flag that is set for static devices, and something analogous to the chip structure for static device info.
3. Continue use of the chip.h files in configuration directories (although the name could be changed to device.h) and the 'register' configuration directive.
4. Removal of all device specific calls from hardwaremain, such as 'init_timer()' into static initialization files using (4) above.
5. The configuration process will build a static device tree that will be called device_{target}.c in the target build directory and linked with the linuxbios image.
My intention is to start coding this tomorrow, so if anyone has any major objections/suggestions, please let me know asap.
Regards,
Greg
Greg Watson gwatson@lanl.gov writes:
The issue for me is that I need to have static initialization working before I can continue, so my intention is build something that will hopefully be acceptable to everyone. I can probably merge the chip and device structures that
gives me a way forward without replicating what Eric is doing. However a single call to enumerate_static_devices, particularly at the current location, is NOT acceptable to me.
That is fine. What I have right now in hardwaremain is just a first pass not a final solution to this problem.
Here's my suggestion to move forward:
- I need to be able to enable/configure static devices at specific points in
hardwaremain, particularly on entry, after console_init and prior to pci enumeration. I suggest a routine called device_configure() that is passed the root of the device tree and a pass flag:
void device_configure(struct device *, enum dev_pass)
enum dev_pass { DEV_PASS_PRE_CONSOLE, DEV_PASS_PRE_PCI, DEV_PASS_ENUMERATE, DEV_PASS_CONFIGURE, DEV_PASS_ENABLE, DEV_PASS_INITIALIZE };
Calls to device_configure() with the appropriate flag will be inserted in the appropriate locations in hardwaremain. More flags/calls call be added if necessary. Calls to dev_enumerate(), dev_configure(), etc. to probe and configure dynamic bus devices will be replaced with calls to device_configure() with the appropriate flag. Dynamic devices will be added to the device tree as appropriate.
I have nits to pick with a single device_configure(). After we have something working I will start picking them.
- New fields added to struct device, including a 'static' flag that is set for
static devices, and something analogous to the chip structure for static device info.
Reasonable.
- Continue use of the chip.h files in configuration directories (although the
name could be changed to device.h) and the 'register' configuration directive.
I don't have a problem with that.
- Removal of all device specific calls from hardwaremain, such as
'init_timer()' into static initialization files using (4) above.
OK. I think.
In general timer services are a generic part of LinuxBIOS and just like the console we may need an init routine. We have avoided it many times in the past by having a static variable and having the timer code magically do something the very first time it is called.
When I built code that could be compiled with romcc I discovered that idiom would not always work.
- The configuration process will build a static device tree that will be called
device_{target}.c in the target build directory and linked with the linuxbios image.
My intention is to start coding this tomorrow, so if anyone has any major objections/suggestions, please let me know asap.
Long term I would like to see if we can get the cpus into this structure as well. But expressing the dependencies is an interesting process.
I plan on reviewing what you have when I get back from OLS.
Eric
On 21 Jul 2003, Eric W. Biederman wrote:
I have nits to pick with a single device_configure(). After we have something working I will start picking them.
do you mean a single call or a single function? The plan is multiple calls to that function.
In general timer services are a generic part of LinuxBIOS and just like the console we may need an init routine. We have avoided it many times in the past by having a static variable and having the timer code magically do something the very first time it is called.
Right. But if the timer is a device in the static tree, and it has a device_configure function, that will get called at some point with a PASS parameter which will tell it to turn on.
Greg, you need to fill in the blanks a bit more about the PASS_ stuff.
Possibly show your hardwaremain()
Long term I would like to see if we can get the cpus into this structure as well. But expressing the dependencies is an interesting process.
The CPUs are in the static structure almost, as a class of CPU. I assume you want a static struct for each one. Very doable. But it makes more sense to make the instances of CPUs dynamic, and continue with the class as a static initializer.
ron
On Tue, 2003-07-22 at 11:37, ron minnich wrote:
On 21 Jul 2003, Eric W. Biederman wrote:
I have nits to pick with a single device_configure(). After we have something working I will start picking them.
do you mean a single call or a single function? The plan is multiple calls to that function.
Why do you want to call a SINGLE function in different places with different enum value ? Is there any disadvantage to make each call into it own function ?
On 22 Jul 2003, ollie lho wrote:
Why do you want to call a SINGLE function in different places with different enum value ? Is there any disadvantage to make each call into it own function ?
OK, to elaborate.
In superio code from 1.0, we had this: struct superio_control { void (*pre_pci_init)(struct superio *s); void (*init)(struct superio *s); void (*finishup)(struct superio *s); unsigned int defaultport; /* the defaultport. Can be overridden * by commands in config */ // This is the print name for debugging char *name; };
The names pre_pci_init, init, and finishup were an implicit definition of those functions. They could be filled in, or NULL, as in:
struct superio_control superio_winbond_w83977ef_control = { pre_pci_init: (void *) 0, init: setup_devices, finishup: (void *) 0, defaultport: PNPADDR, name: "WinBond w83977tf" };
These three functions roughly correspond to "passes" in hardwaremain, and are intended to be called as hardwaremain proceeds through platform configuration.
In other words, you had some degree of control of when a device's control functions were called. This has been very important -- some of these devices need code before PCI enumeration, for example.
The problem is, are the three functions defined sufficient? how many should there be? What if hardwaremain changes and we add new "passes", do we have to add some new set of functions? If we add new passes, how do we go about fixing all the parts that we have written code for?
Also, we wanted to generalize beyond just superios to all devices.
Finally, the PPC has very different rules than standard PCs, and we're trying to come up with a non-Pentium-centric way of doing things.
Now, internally, the code that walked the superio tree was ALWAYS called with a 'pass', as in:
void handle_superio(int pass, struct superio *all_superio[], int nsuperio) { . . . // need to have both pre_pci_init and devfn defined. if (s->super->pre_pci_init && (pass == 0)) { printk_debug(" Call pre_pci_init\n"); s->super->pre_pci_init(s); } else if (s->super->init && (pass == 1)) { printk_debug(" Call init\n"); s->super->init(s); } else if (s->super->finishup && (pass == 2)) { printk_debug(" Call finishup\n"); s->super->finishup(s); }
. . . }
This code has been this way for two or more years.
So all we're really doing is creating ONE interface function to the chips, not three:
/* there is one of these for each TYPE of chip */ struct chip_control { void (*enable)(struct chip *, enum chip_pass); char *path; /* the default path. Can be overridden * by commands in config */ // This is the print name for debugging char *name; };
and that code will now be called with a pass. We expect that in this enable function we'll see things like this:
static void enable(struct chip *, enum chip_pass) { struct superio *s = chip->chip_info; switch (chip_pass) { default: break; case DEV_PASS_PRE_CONSOLE: /* make sure serial is enabled; this chip is weird */ if (s->com1.enable) serial_enable(&s->com1); break; DEV_PASS_PRE_PCI: /* if the ide_enable static initializer is set, * set the IDE controller so it is visible on PCI */ if (s->ide_enable) ide_on(); break; }
}
In other words, most passes don't matter, but if some passes do, then the code can manage it. We'll thus have a standard "template" for all resources on the motherboard, and a standard way to enable resources during different passes (phases, maybe we should call it?) of hardwaremain().
hardwaremain changes a bit too.
For instance, this: post_code(0x80); /* displayinit MUST PRECEDE ALL PRINTK! */ console_init();
becomes this: device_configure(chip_root, DEV_PASS_PRE_CONSOLE); post_code(0x80); /* displayinit MUST PRECEDE ALL PRINTK! */ console_init();
and so on. device_configure is called at different phases in hardwaremain, as the platform becomes more configured. device_configure is capable for walking the device tree, and calling functions as needed.
That's the basic idea, anyway. It is motivated by the goal of replacing this type of code:
/* PCI Interface */ .byte 0x80, 0x72 # .byte 0x81, 0x07 # .byte 0x82, 0xFF #
with C code that people can deal with. There was a lot of confusion on how to enable/disable built-in-ethernet on the SiS 950, for example, and we're trying to reduce confusion.
Comments welcome.
ron
ron minnich rminnich@lanl.gov writes:
On 22 Jul 2003, ollie lho wrote:
Why do you want to call a SINGLE function in different places with different enum value ? Is there any disadvantage to make each call into it own function ?
I hereby reserve judgment until I see Greg's implementation.
I think I am going to complain but I think there is a lot we can work with as well.
The problem is, are the three functions defined sufficient? how many should there be? What if hardwaremain changes and we add new "passes", do we have to add some new set of functions? If we add new passes, how do we go about fixing all the parts that we have written code for?
It is trivial to simply add another function to a table of function pointers if you have something appropriate.
Experience tells me specific functions will do a better job than a pass argument. But it in the big picture it doesn't much matter. It is which hooks you have that matter.
Adding entry points is not a problem. I added the enable function to the device side of things and everything that didn't care just worked.
As far as I am concerned the real debate begins when I get back and can sink my teeth into Greg's code.
For the rest who haven't looked so closely. Greg and Ron are taking the existing superio model and generalizing it.
I have taken the existing pci device model and generalized it.
And now we have the design review/fight/slug out as we meet in the middle.
I suspect the union of the two sets of functions from struct device_operations and from the superio side is two high. But the way forward is to get that union and have everyone understand the pieces backwards and forwards.
Eric
On Tue, 2003-07-22 at 13:49, Eric W. Biederman wrote:
For the rest who haven't looked so closely. Greg and Ron are taking the existing superio model and generalizing it.
I have taken the existing pci device model and generalized it.
And now we have the design review/fight/slug out as we meet in the middle.
I suspect the union of the two sets of functions from struct device_operations and from the superio side is two high. But the way forward is to get that union and have everyone understand the pieces backwards and forwards.
I still don't see the point of the dispute. In the PCI model, you make the operations (functions) a separated entity and in the superio model, the operations are embedded in the data structure. Is there any fundamental or design conflict between these two ?
ollie lho ollie@sis.com.tw writes:
On Tue, 2003-07-22 at 13:49, Eric W. Biederman wrote:
For the rest who haven't looked so closely. Greg and Ron are taking the existing superio model and generalizing it.
I have taken the existing pci device model and generalized it.
And now we have the design review/fight/slug out as we meet in the middle.
I suspect the union of the two sets of functions from struct device_operations and from the superio side is two high. But the way forward is to get that union and have everyone understand the pieces backwards and forwards.
I still don't see the point of the dispute. In the PCI model, you make the operations (functions) a separated entity and in the superio model, the operations are embedded in the data structure. Is there any fundamental or design conflict between these two ?
Fundamentally no. And things should work out ok.
The was a point earlier today where things were not meeting in the middle as they should. So the comprehension or the communication that needed to be there, at least appeared absent.
It was especially aggravated by the fact, that I am gone the rest of the week, and the situation could have continued to the point where it would be nasty to put the pieces back together.
My impression of the flow of development is that I asked for some static devices structures for the pci devices, since Ron and Greg are reworking the configuration files. And what I got was the a generalized version of the superio complete with methods.
The ideas have been discussed in the past on this list and I thought I had communicated clearly. Given that my vision and Ron and Greg's have not yet meshed. It is clear that the communicate was not as clear as I thought. The only way I see to make this clear is for people to start using the code. And putting the two halves together.
Then I can give constructive criticism, on the small details.
For me this is sort of like when I was pushing the ELF file format for the payload. Until it happened and I had the code in place people just didn't see it.
From working with it, Stefan, Yhlu, and I seem to have a pretty good
grasp of how to make the dynamic side of it work, at least for the easy things. Ron and Greg seem to have a good feel for the dynamic side. But we haven't merged the two so there is no good global feel for things yet.
So until the static and the dynamic pieces are fit together feel that it is ineffective and inappropriate to talk about little details of the design. I will talk about what needs to happen to glue the two pieces together (An appropriate enumerate_static_devices method).
So what you see is much more pent of design discussion and frustration than actual conflict.
The goals of the design are lofty. - A universal hardwaremain.c that can be used on multiple architectures. - A device architecture that handles both static and dynamic devices. - Clear requirements from the methods so things fit together cleanly and intuitively instead of a weird mish mash that just happens to work.
We have a pretty good position on the basic architecture, a device tree. But refining it from there in a practical matter is a problem.
Adding to the fun is the fact that there are customers that have to use the code and don't care how brilliant the design is. They want something that works today. So there has been a lot of work put in on the Opteron side lately just to make things work. Which painfully lengthens the amount of time disconnects in the design exist.
Eric
On 22 Jul 2003, ollie lho wrote:
I still don't see the point of the dispute.
I don't think there is a dispute, just uncertainty about how to hook them up.
Also, we can in fact have lots of different entry points in the struct, but there is disagreement about having one function with a pass parameter or 15 functions. It's really a matter of taste.
ron
-
ebiederman＠lnxi.com -
Greg Watson -
ollie lho -
ron minnich