On Thu, Jul 08, 2010 at 05:04:32PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jul 07, 2010 at 11:38:58AM +0900, Isaku Yamahata wrote:
> > But you claim it's only for root bus, not for secondary bus.
>
> It is currently, isn't it?
>
> > Now I realized why you've rejected such patches so far.
> > Then, you also mean the current pci_register_secondary_bus() is broken.
>
> Sorry about being dense, what is broken?
I've regarded pci_bus_new() (or _inplace) as new qdev style API.
And pci_register_bus() (or pci_register_secondary_bus()) as old
(so deprecated) API.
So pci_reguster_bus() would be replaced with pci_bus_new() gradually
like the changeset of 7cd9eee0f6fd6953114068dd98d91fca1237880b
I've thought that pci_bus_new() is for both root and secondary bus.
However, according to your comment, the situation seems different.
> > I also think it's broken. So how do we want to fix it?
> > My idea is as follows.
> >
> > - introduce something like pci_secondary_bus_new()
> > (pci_sec_bus_new() for short?) for secondary bus.
> > fix pci_register_secondary_bus() with it.
> >
> > - introduce something like pci_host_bus_new() (or pci_root_bus_new()?)
> > for pci host bus which is more generic than pci_bus_new().
> > It's for
> > - to avoid confusion.
>
> IMHO the confusion comes from the fact we have too
> many functions that do almost, but not quite, the same
> thing, and the function names do not say anything.
>
> We have a ton of 5 line functions with names like
> _allocate_inplace, _new, _register, _simple
Fully Agreed. Some clean up is necessary.
> > - to eliminate assumption of pci_bus_new().
> > pci_bus_new() assumes that its pci segment is 0.
> > keep pci_bus_new() as a convenience wrapper of
> > pci_host_bus_new(segment = 0). Thus we can avoid fixing up
> > all the caller.
>
> We have a single caller, right? I think you mean pci_register_bus?
> So IIUC, you propose that we add pci_register_host_bus,
> and make pci_register_bus a compatibility wrapper?
> Sure, let's just add a comment this is deprecated.
>
> I am not sure why do we need an API to deal with secondary bus:
> it is always a part of the bridge, so all users can and should call
> pci_bridge_init?
Okay, then how about the following?
For root bus:
- pci_host_bus_new()/pci_host_bus_new_inplace()
qbus style api. pci segment must be specified.
New code should use this.
- pci_bus_new()
qbus style API.
convenience wrapper for compatibility of
pci_host_bus_new(pci segment = 0)
In fact, the only current user piix_pci.c. It's easy to remove it.
- pci_register_bus()
old style API. deprecated.
It has been kept for compatibility so far.
This will be gradually replaced with pci_host_bus_new()
For secondary bus:
- pci_bridge_init()
qdev style API.
New code should use this.
- pci_{register, unregister}_secondary_bus():
old stype API. deprecated.
Keep them only for internal use in pci.c
or they can be easily removed or renamed for qdev style.
For pci device:
- pci_create()
qdev style API.
The transitional function until completion of qdev conversion.
If the creation of a device tree from config file is implemented,
this function will be unnecessary.
- pci_create_simple()
qdev style API.
convenience function = pci_create() + qdev_init_nofail()
--
yamahata
