[PATCH 09/24] PCI, powerpc: Register busn_res for root buses

[PATCH 09/24] PCI, powerpc: Register busn_res for root buses

[Yinghai Lu]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: linuxppc-dev@lists.ozlabs.org
---
 arch/powerpc/kernel/pci-common.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index cce98d7..501f29b 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1732,6 +1732,8 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
 	bus->secondary = hose->first_busno;
 	hose->bus = bus;
 
+	pci_bus_insert_busn_res(bus, hose->first_busno, hose->last_busno);
+
 	/* Get probe mode and perform scan */
 	mode = PCI_PROBE_NORMAL;
 	if (node && ppc_md.pci_probe_mode)
@@ -1742,8 +1744,11 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
 		of_scan_bus(node, bus);
 	}
 
-	if (mode == PCI_PROBE_NORMAL)
+	if (mode == PCI_PROBE_NORMAL) {
+		pci_bus_update_busn_res_end(bus, 255);
 		hose->last_busno = bus->subordinate = pci_scan_child_bus(bus);
+		pci_bus_update_busn_res_end(bus, bus->subordinate);
+	}
 
 	/* Platform gets a chance to do some global fixups before
 	 * we proceed to resource allocation
-- 
1.7.7

Re: [PATCH 09/24] PCI, powerpc: Register busn_res for root buses

[Bjorn Helgaas]

On Sat, Feb 4, 2012 at 10:57 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: linuxppc-dev@lists.ozlabs.org
> ---
> =A0arch/powerpc/kernel/pci-common.c | =A0 =A07 ++++++-
> =A01 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-c=
ommon.c
> index cce98d7..501f29b 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1732,6 +1732,8 @@ void __devinit pcibios_scan_phb(struct pci_controll=
er *hose)
> =A0 =A0 =A0 =A0bus->secondary =3D hose->first_busno;
> =A0 =A0 =A0 =A0hose->bus =3D bus;
>
> + =A0 =A0 =A0 pci_bus_insert_busn_res(bus, hose->first_busno, hose->last_=
busno);
> +
> =A0 =A0 =A0 =A0/* Get probe mode and perform scan */
> =A0 =A0 =A0 =A0mode =3D PCI_PROBE_NORMAL;
> =A0 =A0 =A0 =A0if (node && ppc_md.pci_probe_mode)
> @@ -1742,8 +1744,11 @@ void __devinit pcibios_scan_phb(struct pci_control=
ler *hose)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0of_scan_bus(node, bus);
> =A0 =A0 =A0 =A0}
>
> - =A0 =A0 =A0 if (mode =3D=3D PCI_PROBE_NORMAL)
> + =A0 =A0 =A0 if (mode =3D=3D PCI_PROBE_NORMAL) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pci_bus_update_busn_res_end(bus, 255);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hose->last_busno =3D bus->subordinate =3D =
pci_scan_child_bus(bus);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pci_bus_update_busn_res_end(bus, bus->subor=
dinate);
> + =A0 =A0 =A0 }

The only architecture-specific thing here is discovering the range of
bus numbers below a host bridge.  The architecture should not have to
mess around with pci_bus_update_busn_res_end() like this.  It should
be able to say "here's my bus number range" (and of course the PCI
core can default to 0-255 if the arch doesn't supply a range) and the
core should take care of the rest.

Bjorn

Re: [PATCH 09/24] PCI, powerpc: Register busn_res for root buses

[Yinghai Lu]

On Wed, Feb 8, 2012 at 7:58 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Sat, Feb 4, 2012 at 10:57 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: linuxppc-dev@lists.ozlabs.org
>> ---
>> =A0arch/powerpc/kernel/pci-common.c | =A0 =A07 ++++++-
>> =A01 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-=
common.c
>> index cce98d7..501f29b 100644
>> --- a/arch/powerpc/kernel/pci-common.c
>> +++ b/arch/powerpc/kernel/pci-common.c
>> @@ -1732,6 +1732,8 @@ void __devinit pcibios_scan_phb(struct pci_control=
ler *hose)
>> =A0 =A0 =A0 =A0bus->secondary =3D hose->first_busno;
>> =A0 =A0 =A0 =A0hose->bus =3D bus;
>>
>> + =A0 =A0 =A0 pci_bus_insert_busn_res(bus, hose->first_busno, hose->last=
_busno);
>> +
>> =A0 =A0 =A0 =A0/* Get probe mode and perform scan */
>> =A0 =A0 =A0 =A0mode =3D PCI_PROBE_NORMAL;
>> =A0 =A0 =A0 =A0if (node && ppc_md.pci_probe_mode)
>> @@ -1742,8 +1744,11 @@ void __devinit pcibios_scan_phb(struct pci_contro=
ller *hose)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0of_scan_bus(node, bus);
>> =A0 =A0 =A0 =A0}
>>
>> - =A0 =A0 =A0 if (mode =3D=3D PCI_PROBE_NORMAL)
>> + =A0 =A0 =A0 if (mode =3D=3D PCI_PROBE_NORMAL) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pci_bus_update_busn_res_end(bus, 255);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hose->last_busno =3D bus->subordinate =3D=
 pci_scan_child_bus(bus);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pci_bus_update_busn_res_end(bus, bus->subo=
rdinate);
>> + =A0 =A0 =A0 }
>
> The only architecture-specific thing here is discovering the range of
> bus numbers below a host bridge. =A0The architecture should not have to
> mess around with pci_bus_update_busn_res_end() like this. =A0It should
> be able to say "here's my bus number range" (and of course the PCI
> core can default to 0-255 if the arch doesn't supply a range) and the
> core should take care of the rest.

during the pci_scan_child_bus,  child bus busn_res will be inserted
under parent bus busn_res.

So need to make sure parent busn_res.end is bigger enough.


Yinghai

Re: [PATCH 09/24] PCI, powerpc: Register busn_res for root buses

[Benjamin Herrenschmidt]

On Wed, 2012-02-08 at 07:58 -0800, Bjorn Helgaas wrote:
> The only architecture-specific thing here is discovering the range of
> bus numbers below a host bridge.  The architecture should not have to
> mess around with pci_bus_update_busn_res_end() like this.  It should
> be able to say "here's my bus number range" (and of course the PCI
> core can default to 0-255 if the arch doesn't supply a range) and the
> core should take care of the rest. 

So it's a bit messy in here because we deal with several things.

What the firmware gives us is the range it assigned, but that isn't
necessarily the HW limits (almost never is in fact).

In some cases we honor it, for example when in "probe only" mode where
we prevent any reassigning, and in some case, we ignore it and let the
PCI core renumber things (typically because the FW "forgot" to set aside
bus numbers for a cardbus slot for example, that sort of things).

So it's a bit of a tricky situation.

Off the top of my head, I'm pretty sure that most if not all of our PCI
host bridges simply support a full 0...255 range and there is no sharing
between bridges like on x86, they are just different domains.

But I can't vouch 100% for some of the oddball cases like Pegasos or
some freescale gear.

Cheers,
Ben.

Re: [PATCH 09/24] PCI, powerpc: Register busn_res for root buses

[Bjorn Helgaas]

On Wed, Feb 8, 2012 at 2:02 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2012-02-08 at 07:58 -0800, Bjorn Helgaas wrote:
>> The only architecture-specific thing here is discovering the range of
>> bus numbers below a host bridge. =A0The architecture should not have to
>> mess around with pci_bus_update_busn_res_end() like this. =A0It should
>> be able to say "here's my bus number range" (and of course the PCI
>> core can default to 0-255 if the arch doesn't supply a range) and the
>> core should take care of the rest.
>
> So it's a bit messy in here because we deal with several things.
>
> What the firmware gives us is the range it assigned, but that isn't
> necessarily the HW limits (almost never is in fact).
>
> In some cases we honor it, for example when in "probe only" mode where
> we prevent any reassigning, and in some case, we ignore it and let the
> PCI core renumber things (typically because the FW "forgot" to set aside
> bus numbers for a cardbus slot for example, that sort of things).
>
> So it's a bit of a tricky situation.
>
> Off the top of my head, I'm pretty sure that most if not all of our PCI
> host bridges simply support a full 0...255 range and there is no sharing
> between bridges like on x86, they are just different domains.

My point is that the interface between the arch and the PCI core
should be simply the arch telling the core "this is the range of bus
numbers you can use."  If the firmware doesn't give you the HW limits,
that's the arch's problem.  If you want to assume 0..255 are
available, again, that's the arch's decision.

But the answer to the question "what bus numbers are available to me"
depends only on the host bridge HW configuration.  It does not depend
on what pci_scan_child_bus() found.  Therefore, I think we can come up
with a design where pci_bus_update_busn_res_end() is unnecessary.

Bjorn

Re: [PATCH 09/24] PCI, powerpc: Register busn_res for root buses

[Benjamin Herrenschmidt]

On Thu, 2012-02-09 at 11:24 -0800, Bjorn Helgaas wrote:
> My point is that the interface between the arch and the PCI core
> should be simply the arch telling the core "this is the range of bus
> numbers you can use."  If the firmware doesn't give you the HW limits,
> that's the arch's problem.  If you want to assume 0..255 are
> available, again, that's the arch's decision.
> 
> But the answer to the question "what bus numbers are available to me"
> depends only on the host bridge HW configuration.  It does not depend
> on what pci_scan_child_bus() found.  Therefore, I think we can come up
> with a design where pci_bus_update_busn_res_end() is unnecessary.

In an ideal world yes. In a world where there are reverse engineered
platforms on which we aren't 100% sure how thing actually work under the
hood and have the code just adapt on "what's there" (and try to fix it
up -sometimes-), thinks can get a bit murky :-)

But yes, I see your point. As for what is the "correct" setting that
needs to be done so that the patch doesn't end up a regression for us,
I'll have to dig into some ancient HW to dbl check a few things. I hope
0...255 will just work but I can't guarantee it.

What I'll probably do is constraint the core to the values in
hose->min/max, and update selected platforms to put 0..255 in there when
I know for sure they can cope.

Cheers,
Ben.

Re: [PATCH 09/24] PCI, powerpc: Register busn_res for root buses

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 2896 bytes --]

On Fri, 10 Feb 2012 08:35:58 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Thu, 2012-02-09 at 11:24 -0800, Bjorn Helgaas wrote:
> > My point is that the interface between the arch and the PCI core
> > should be simply the arch telling the core "this is the range of bus
> > numbers you can use."  If the firmware doesn't give you the HW limits,
> > that's the arch's problem.  If you want to assume 0..255 are
> > available, again, that's the arch's decision.
> > 
> > But the answer to the question "what bus numbers are available to me"
> > depends only on the host bridge HW configuration.  It does not depend
> > on what pci_scan_child_bus() found.  Therefore, I think we can come up
> > with a design where pci_bus_update_busn_res_end() is unnecessary.
> 
> In an ideal world yes. In a world where there are reverse engineered
> platforms on which we aren't 100% sure how thing actually work under the
> hood and have the code just adapt on "what's there" (and try to fix it
> up -sometimes-), thinks can get a bit murky :-)
> 
> But yes, I see your point. As for what is the "correct" setting that
> needs to be done so that the patch doesn't end up a regression for us,
> I'll have to dig into some ancient HW to dbl check a few things. I hope
> 0...255 will just work but I can't guarantee it.
> 
> What I'll probably do is constraint the core to the values in
> hose->min/max, and update selected platforms to put 0..255 in there when
> I know for sure they can cope.

But I think the point is, can't we intiialize the busn resource after
the first & last bus numbers have been determined?  E.g. rather than
Yinghai's current:
+	pci_bus_insert_busn_res(bus, hose->first_busno, hose->last_busno);
+
 	/* Get probe mode and perform scan */
 	mode = PCI_PROBE_NORMAL;
 	if (node && ppc_md.pci_probe_mode)
@@ -1742,8 +1744,11 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
 		of_scan_bus(node, bus);
 	}
 
-	if (mode == PCI_PROBE_NORMAL)
+	if (mode == PCI_PROBE_NORMAL) {
+		pci_bus_update_busn_res_end(bus, 255);
 		hose->last_busno = bus->subordinate = pci_scan_child_bus(bus);
+		pci_bus_update_busn_res_end(bus, bus->subordinate);
+	}

we'd have something more like:

 	/* Get probe mode and perform scan */
 	mode = PCI_PROBE_NORMAL;
 	if (node && ppc_md.pci_probe_mode)
@@ -1742,8 +1744,11 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
 		of_scan_bus(node, bus);
 	}
 
	if (mode == PCI_PROBE_NORMAL)
 		hose->last_busno = bus->subordinate = pci_scan_child_bus(bus);

+	pci_bus_insert_busn_res(bus, hose->first_busno, hose->last_busno);

since we should have the final bus range by then?  Setting the end to
255 and then changing it again doesn't make sense; and definitely makes
the code hard to follow.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 09/24] PCI, powerpc: Register busn_res for root buses

[Bjorn Helgaas]

On Thu, Feb 23, 2012 at 12:25 PM, Jesse Barnes <jbarnes@virtuousgeek.org> w=
rote:
> On Fri, 10 Feb 2012 08:35:58 +1100
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
>> On Thu, 2012-02-09 at 11:24 -0800, Bjorn Helgaas wrote:
>> > My point is that the interface between the arch and the PCI core
>> > should be simply the arch telling the core "this is the range of bus
>> > numbers you can use." =A0If the firmware doesn't give you the HW limit=
s,
>> > that's the arch's problem. =A0If you want to assume 0..255 are
>> > available, again, that's the arch's decision.
>> >
>> > But the answer to the question "what bus numbers are available to me"
>> > depends only on the host bridge HW configuration. =A0It does not depen=
d
>> > on what pci_scan_child_bus() found. =A0Therefore, I think we can come =
up
>> > with a design where pci_bus_update_busn_res_end() is unnecessary.
>>
>> In an ideal world yes. In a world where there are reverse engineered
>> platforms on which we aren't 100% sure how thing actually work under the
>> hood and have the code just adapt on "what's there" (and try to fix it
>> up -sometimes-), thinks can get a bit murky :-)
>>
>> But yes, I see your point. As for what is the "correct" setting that
>> needs to be done so that the patch doesn't end up a regression for us,
>> I'll have to dig into some ancient HW to dbl check a few things. I hope
>> 0...255 will just work but I can't guarantee it.
>>
>> What I'll probably do is constraint the core to the values in
>> hose->min/max, and update selected platforms to put 0..255 in there when
>> I know for sure they can cope.
>
> But I think the point is, can't we intiialize the busn resource after
> the first & last bus numbers have been determined? =A0E.g. rather than
> Yinghai's current:
> + =A0 =A0 =A0 pci_bus_insert_busn_res(bus, hose->first_busno, hose->last_=
busno);
> +
> =A0 =A0 =A0 =A0/* Get probe mode and perform scan */
> =A0 =A0 =A0 =A0mode =3D PCI_PROBE_NORMAL;
> =A0 =A0 =A0 =A0if (node && ppc_md.pci_probe_mode)
> @@ -1742,8 +1744,11 @@ void __devinit pcibios_scan_phb(struct pci_control=
ler *hose)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0of_scan_bus(node, bus);
> =A0 =A0 =A0 =A0}
>
> - =A0 =A0 =A0 if (mode =3D=3D PCI_PROBE_NORMAL)
> + =A0 =A0 =A0 if (mode =3D=3D PCI_PROBE_NORMAL) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pci_bus_update_busn_res_end(bus, 255);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hose->last_busno =3D bus->subordinate =3D =
pci_scan_child_bus(bus);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pci_bus_update_busn_res_end(bus, bus->subor=
dinate);
> + =A0 =A0 =A0 }
>
> we'd have something more like:
>
> =A0 =A0 =A0 =A0/* Get probe mode and perform scan */
> =A0 =A0 =A0 =A0mode =3D PCI_PROBE_NORMAL;
> =A0 =A0 =A0 =A0if (node && ppc_md.pci_probe_mode)
> @@ -1742,8 +1744,11 @@ void __devinit pcibios_scan_phb(struct pci_control=
ler *hose)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0of_scan_bus(node, bus);
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0if (mode =3D=3D PCI_PROBE_NORMAL)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hose->last_busno =3D bus->subordinate =3D =
pci_scan_child_bus(bus);
>
> + =A0 =A0 =A0 pci_bus_insert_busn_res(bus, hose->first_busno, hose->last_=
busno);
>
> since we should have the final bus range by then? =A0Setting the end to
> 255 and then changing it again doesn't make sense; and definitely makes
> the code hard to follow.

I have two issues here:

1) hose->last_busno is currently the highest bus number found by
pci_scan_child_bus().  If I understand correctly,
pci_bus_insert_busn_res() is supposed to update the core's idea of the
host bridge's bus number aperture.  (Actually, I guess it just updates
the *end* of the aperture, since we supply the start directly to
pci_scan_root_bus()).  The aperture and the highest bus number we
found are not related, except that we should have:

    hose->first_busno <=3D bus->subordinate <=3D hose->last_busno

If we set the aperture to [first_busno - last_busno], we artificially
prevent some hotplug.

2) We already have a way to add resources to a root bus: the
pci_add_resource() used to add I/O port and MMIO apertures.  I think
it'd be a lot simpler to just use that same interface for the bus
number aperture, e.g.,

    pci_add_resource(&resources, hose->io_space);
    pci_add_resource(&resources, hose->mem_space);
    pci_add_resource(&resources, hose->busnr_space);
    bus =3D pci_scan_root_bus(dev, next_busno, pci_ops, sysdata, &resources=
);

This is actually a bit redundant, since "next_busno" should be the
same as hose->busnr_space->start.  So if we adopted this approach, we
might want to eventually drop the "next_busno" argument.

 Bjorn

Re: [PATCH 09/24] PCI, powerpc: Register busn_res for root buses

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 5202 bytes --]

On Thu, 23 Feb 2012 12:51:30 -0800
Bjorn Helgaas <bhelgaas@google.com> wrote:

> On Thu, Feb 23, 2012 at 12:25 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Fri, 10 Feb 2012 08:35:58 +1100
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> >
> >> On Thu, 2012-02-09 at 11:24 -0800, Bjorn Helgaas wrote:
> >> > My point is that the interface between the arch and the PCI core
> >> > should be simply the arch telling the core "this is the range of bus
> >> > numbers you can use."  If the firmware doesn't give you the HW limits,
> >> > that's the arch's problem.  If you want to assume 0..255 are
> >> > available, again, that's the arch's decision.
> >> >
> >> > But the answer to the question "what bus numbers are available to me"
> >> > depends only on the host bridge HW configuration.  It does not depend
> >> > on what pci_scan_child_bus() found.  Therefore, I think we can come up
> >> > with a design where pci_bus_update_busn_res_end() is unnecessary.
> >>
> >> In an ideal world yes. In a world where there are reverse engineered
> >> platforms on which we aren't 100% sure how thing actually work under the
> >> hood and have the code just adapt on "what's there" (and try to fix it
> >> up -sometimes-), thinks can get a bit murky :-)
> >>
> >> But yes, I see your point. As for what is the "correct" setting that
> >> needs to be done so that the patch doesn't end up a regression for us,
> >> I'll have to dig into some ancient HW to dbl check a few things. I hope
> >> 0...255 will just work but I can't guarantee it.
> >>
> >> What I'll probably do is constraint the core to the values in
> >> hose->min/max, and update selected platforms to put 0..255 in there when
> >> I know for sure they can cope.
> >
> > But I think the point is, can't we intiialize the busn resource after
> > the first & last bus numbers have been determined?  E.g. rather than
> > Yinghai's current:
> > +       pci_bus_insert_busn_res(bus, hose->first_busno, hose->last_busno);
> > +
> >        /* Get probe mode and perform scan */
> >        mode = PCI_PROBE_NORMAL;
> >        if (node && ppc_md.pci_probe_mode)
> > @@ -1742,8 +1744,11 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
> >                of_scan_bus(node, bus);
> >        }
> >
> > -       if (mode == PCI_PROBE_NORMAL)
> > +       if (mode == PCI_PROBE_NORMAL) {
> > +               pci_bus_update_busn_res_end(bus, 255);
> >                hose->last_busno = bus->subordinate = pci_scan_child_bus(bus);
> > +               pci_bus_update_busn_res_end(bus, bus->subordinate);
> > +       }
> >
> > we'd have something more like:
> >
> >        /* Get probe mode and perform scan */
> >        mode = PCI_PROBE_NORMAL;
> >        if (node && ppc_md.pci_probe_mode)
> > @@ -1742,8 +1744,11 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
> >                of_scan_bus(node, bus);
> >        }
> >
> >        if (mode == PCI_PROBE_NORMAL)
> >                hose->last_busno = bus->subordinate = pci_scan_child_bus(bus);
> >
> > +       pci_bus_insert_busn_res(bus, hose->first_busno, hose->last_busno);
> >
> > since we should have the final bus range by then?  Setting the end to
> > 255 and then changing it again doesn't make sense; and definitely makes
> > the code hard to follow.
> 
> I have two issues here:
> 
> 1) hose->last_busno is currently the highest bus number found by
> pci_scan_child_bus().  If I understand correctly,
> pci_bus_insert_busn_res() is supposed to update the core's idea of the
> host bridge's bus number aperture.  (Actually, I guess it just updates
> the *end* of the aperture, since we supply the start directly to
> pci_scan_root_bus()).  The aperture and the highest bus number we
> found are not related, except that we should have:
> 
>     hose->first_busno <= bus->subordinate <= hose->last_busno
> 
> If we set the aperture to [first_busno - last_busno], we artificially
> prevent some hotplug.

Oh true, we'll need to allocate any extra bus number space somehow so
that hot plug of bridges is possible in the future w/o renumbering
(until our glorious future when we can move resources on the fly by
stopping drivers).

> 
> 2) We already have a way to add resources to a root bus: the
> pci_add_resource() used to add I/O port and MMIO apertures.  I think
> it'd be a lot simpler to just use that same interface for the bus
> number aperture, e.g.,
> 
>     pci_add_resource(&resources, hose->io_space);
>     pci_add_resource(&resources, hose->mem_space);
>     pci_add_resource(&resources, hose->busnr_space);
>     bus = pci_scan_root_bus(dev, next_busno, pci_ops, sysdata, &resources);
> 
> This is actually a bit redundant, since "next_busno" should be the
> same as hose->busnr_space->start.  So if we adopted this approach, we
> might want to eventually drop the "next_busno" argument.

Yeah that would be nice, the call would certainly make more sense that
way.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 09/24] PCI, powerpc: Register busn_res for root buses

[Yinghai Lu]

On Fri, Feb 24, 2012 at 2:24 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wr=
ote:
> On Thu, 23 Feb 2012 12:51:30 -0800
> Bjorn Helgaas <bhelgaas@google.com> wrote:
>> 2) We already have a way to add resources to a root bus: the
>> pci_add_resource() used to add I/O port and MMIO apertures. =A0I think
>> it'd be a lot simpler to just use that same interface for the bus
>> number aperture, e.g.,
>>
>> =A0 =A0 pci_add_resource(&resources, hose->io_space);
>> =A0 =A0 pci_add_resource(&resources, hose->mem_space);
>> =A0 =A0 pci_add_resource(&resources, hose->busnr_space);
>> =A0 =A0 bus =3D pci_scan_root_bus(dev, next_busno, pci_ops, sysdata, &re=
sources);
>>
>> This is actually a bit redundant, since "next_busno" should be the
>> same as hose->busnr_space->start. =A0So if we adopted this approach, we
>> might want to eventually drop the "next_busno" argument.
>
> Yeah that would be nice, the call would certainly make more sense that
> way.

no, I don't think so.

using pci_add_resource will need to create dummy resource abut bus range.

there is lots of pci_scan_root_bus(),  and those user does not bus end
yet before scan.
so could just hide pci_insert_busn_res in pci_scan_root_bus, and
update busn_res end there.

other arch like x86, ia64, powerpc, sparc, will insert exact bus range
between pci_create_root_bus and
pci_scan_child_bus, will not need to update busn_res end.

please check v7 of this patchset.

    	git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.gi=
t
for-pci-busn-alloc

It should be clean and have minimum lines of change.

Thanks

      Yinghai

Re: [PATCH 09/24] PCI, powerpc: Register busn_res for root buses

[Bjorn Helgaas]

On Sat, Feb 25, 2012 at 12:47 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Feb 24, 2012 at 2:24 PM, Jesse Barnes <jbarnes@virtuousgeek.org> =
wrote:
>> On Thu, 23 Feb 2012 12:51:30 -0800
>> Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> 2) We already have a way to add resources to a root bus: the
>>> pci_add_resource() used to add I/O port and MMIO apertures. =A0I think
>>> it'd be a lot simpler to just use that same interface for the bus
>>> number aperture, e.g.,
>>>
>>> =A0 =A0 pci_add_resource(&resources, hose->io_space);
>>> =A0 =A0 pci_add_resource(&resources, hose->mem_space);
>>> =A0 =A0 pci_add_resource(&resources, hose->busnr_space);
>>> =A0 =A0 bus =3D pci_scan_root_bus(dev, next_busno, pci_ops, sysdata, &r=
esources);
>>>
>>> This is actually a bit redundant, since "next_busno" should be the
>>> same as hose->busnr_space->start. =A0So if we adopted this approach, we
>>> might want to eventually drop the "next_busno" argument.
>>
>> Yeah that would be nice, the call would certainly make more sense that
>> way.
>
> no, I don't think so.
>
> using pci_add_resource will need to create dummy resource abut bus range.

I don't see the problem here.  The bus number aperture is a
fundamental property of a host bridge, and any firmware or native
bridge driver that tells you about a bridge but doesn't tell you the
bus number aperture is just broken.

Every arch already has struct resources for the MMIO and IO port
regions available on the bus.  ACPI already has a resource for the bus
number range.  It makes sense to me that the arch should supply a bus
number resource.

Conceptually, it's just like the MMIO and IO resources, so it makes
sense to me that the code for bus numbers should look like the code
for MMIO and IO ports.

> there is lots of pci_scan_root_bus(), =A0and those user does not bus end
> yet before scan.
> so could just hide pci_insert_busn_res in pci_scan_root_bus, and
> update busn_res end there.

pci_scan_child_bus() does NOT tell you the end of the bus number
aperture, and we shouldn't pretend that it does.  It might give you a
lower bound on the end of the aperture (as long as you're willing to
trust the current PCI config and you don't change anything).

> other arch like x86, ia64, powerpc, sparc, will insert exact bus range
> between pci_create_root_bus and
> pci_scan_child_bus, will not need to update busn_res end.
>
> please check v7 of this patchset.
>
> =A0 =A0 =A0 =A0git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linu=
x-yinghai.git
> for-pci-busn-alloc

I looked at your git tree, but I can't tell whether what's there is v7
or not and it's too much trouble to try to figure it out.

Bjorn