[PATCH v8 19/30] powerpc/pci: Use pci_scan_host_bridge() for simplicity

[PATCH v8 19/30] powerpc/pci: Use pci_scan_host_bridge() for simplicity

[Yijing Wang]

Now we could use pci_scan_host_bridge() to scan
pci buses, provide powerpc specific pci_host_bridge_ops.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: linuxppc-dev@lists.ozlabs.org
---
 arch/powerpc/kernel/pci-common.c |   60 +++++++++++++++++++++++--------------
 1 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 2c58200..e2b50a2 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -773,6 +773,29 @@ void pcibios_set_root_bus_speed(struct pci_host_bridge *bridge)
 		return ppc_md.pcibios_set_root_bus_speed(bridge);
 }
 
+static int pci_host_scan_bus(struct pci_host_bridge *host)
+{
+	int mode = PCI_PROBE_NORMAL;
+	struct pci_bus *bus = host->bus;
+	struct pci_controller *hose = dev_get_drvdata(&host->dev);
+
+	/* Get probe mode and perform scan */
+	if (hose->dn && ppc_md.pci_probe_mode)
+		mode = ppc_md.pci_probe_mode(bus);
+
+	pr_debug("    probe mode: %d\n", mode);
+	if (mode == PCI_PROBE_DEVTREE)
+		of_scan_bus(hose->dn, bus);
+
+	if (mode == PCI_PROBE_NORMAL) {
+		pci_bus_update_busn_res_end(bus, 255);
+		hose->last_busno = pci_scan_child_bus(bus);
+		pci_bus_update_busn_res_end(bus, hose->last_busno);
+	}
+
+	return pci_bus_child_max_busnr(bus);
+}
+
 /* This header fixup will do the resource fixup for all devices as they are
  * probed, but not for bridge ranges
  */
@@ -1585,6 +1608,11 @@ struct device_node *pcibios_get_phb_of_node(struct pci_bus *bus)
 	return of_node_get(hose->dn);
 }
 
+static struct pci_host_bridge_ops pci_host_ops = {
+	.set_root_bus_speed = pcibios_set_root_bus_speed,
+	.scan_bus = pci_host_scan_bus,
+};
+
 /**
  * pci_scan_phb - Given a pci_controller, setup and scan the PCI bus
  * @hose: Pointer to the PCI host controller instance structure
@@ -1592,9 +1620,8 @@ struct device_node *pcibios_get_phb_of_node(struct pci_bus *bus)
 void pcibios_scan_phb(struct pci_controller *hose)
 {
 	LIST_HEAD(resources);
-	struct pci_bus *bus;
+	struct pci_host_bridge *host;
 	struct device_node *node = hose->dn;
-	int mode;
 
 	pr_debug("PCI: Scanning PHB %s\n", of_node_full_name(node));
 
@@ -1609,31 +1636,18 @@ void pcibios_scan_phb(struct pci_controller *hose)
 	hose->busn.flags = IORESOURCE_BUS;
 	pci_add_resource(&resources, &hose->busn);
 
+	pci_host_ops.pci_ops = hose->ops;
 	/* Create an empty bus for the toplevel */
-	bus = pci_create_root_bus(hose->parent, hose->global_number,
-			hose->first_busno, hose->ops, hose, &resources);
-	if (bus == NULL) {
-		pr_err("Failed to create bus for PCI domain %04x\n",
-			hose->global_number);
+	host = pci_scan_host_bridge(hose->parent, hose->global_number,
+			hose->first_busno, hose, &resources, &pci_host_ops);
+	if (host == NULL) {
+		pr_err("Failed to create host bridge for pci%04x:%02x\n",
+			hose->global_number, hose->first_busno);
 		pci_free_resource_list(&resources);
 		return;
 	}
 	hose->bus = bus;
 
-	/* Get probe mode and perform scan */
-	mode = PCI_PROBE_NORMAL;
-	if (node && ppc_md.pci_probe_mode)
-		mode = ppc_md.pci_probe_mode(bus);
-	pr_debug("    probe mode: %d\n", mode);
-	if (mode == PCI_PROBE_DEVTREE)
-		of_scan_bus(node, bus);
-
-	if (mode == PCI_PROBE_NORMAL) {
-		pci_bus_update_busn_res_end(bus, 255);
-		hose->last_busno = pci_scan_child_bus(bus);
-		pci_bus_update_busn_res_end(bus, hose->last_busno);
-	}
-
 	/* Platform gets a chance to do some global fixups before
 	 * we proceed to resource allocation
 	 */
@@ -1641,9 +1655,9 @@ void pcibios_scan_phb(struct pci_controller *hose)
 		ppc_md.pcibios_fixup_phb(hose);
 
 	/* Configure PCI Express settings */
-	if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
+	if (host->bus && !pci_has_flag(PCI_PROBE_ONLY)) {
 		struct pci_bus *child;
-		list_for_each_entry(child, &bus->children, node)
+		list_for_each_entry(child, &host->bus->children, node)
 			pcie_bus_configure_settings(child);
 	}
 }
-- 
1.7.1

Re: [PATCH v8 19/30] powerpc/pci: Use pci_scan_host_bridge() for simplicity

[Daniel Axtens]

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

On Tue, 2015-03-24 at 11:34 +0800, Yijing Wang wrote:
> Now we could use pci_scan_host_bridge() to scan
> pci buses, provide powerpc specific pci_host_bridge_ops.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: linuxppc-dev@lists.ozlabs.org
> ---
>  arch/powerpc/kernel/pci-common.c |   60 +++++++++++++++++++++++--------------
>  1 files changed, 37 insertions(+), 23 deletions(-)

> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 2c58200..e2b50a2 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -773,6 +773,29 @@ void pcibios_set_root_bus_speed(struct pci_host_bridge *bridge)
>  		return ppc_md.pcibios_set_root_bus_speed(bridge);
>  }
>  
> +static int pci_host_scan_bus(struct pci_host_bridge *host)
> +{
> +	int mode = PCI_PROBE_NORMAL;
> +	struct pci_bus *bus = host->bus;
> +	struct pci_controller *hose = dev_get_drvdata(&host->dev);
Is there any reason this isn't *hose = pci_bus_to_host(bus)?
> +
> +	/* Get probe mode and perform scan */
> +	if (hose->dn && ppc_md.pci_probe_mode)
> +		mode = ppc_md.pci_probe_mode(bus);
> +
> +	pr_debug("    probe mode: %d\n", mode);
> +	if (mode == PCI_PROBE_DEVTREE)
> +		of_scan_bus(hose->dn, bus);
> +
> +	if (mode == PCI_PROBE_NORMAL) {
> +		pci_bus_update_busn_res_end(bus, 255);
> +		hose->last_busno = pci_scan_child_bus(bus);
> +		pci_bus_update_busn_res_end(bus, hose->last_busno);
> +	}
> +
> +	return pci_bus_child_max_busnr(bus);
> +}
> +
I'm having trouble convincing myself that this patch covers every
variation within our PCI implementations. In particular, there's a
stanza in of_scan_pci_bridge in kernel/pci_of_scan.c that's almost
identical to this function. Does that implementation need to be cleaned
up and replaced with this function too?


> @@ -1641,9 +1655,9 @@ void pcibios_scan_phb(struct pci_controller *hose)
>  		ppc_md.pcibios_fixup_phb(hose);
>  
>  	/* Configure PCI Express settings */
> -	if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
> +	if (host->bus && !pci_has_flag(PCI_PROBE_ONLY)) {
>  		struct pci_bus *child;
> -		list_for_each_entry(child, &bus->children, node)
> +		list_for_each_entry(child, &host->bus->children, node)
>  			pcie_bus_configure_settings(child);
>  	}
>  }
Two things: Firstly, the function uses hose throughout, not host.
Secondly, you're not deleting the bus variable: what's the purpose of
this change?

Regards,
Daniel


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 860 bytes --]

Re: [PATCH v8 19/30] powerpc/pci: Use pci_scan_host_bridge() for simplicity

[Yijing Wang]

On 2015/3/25 7:58, Daniel Axtens wrote:
> On Tue, 2015-03-24 at 11:34 +0800, Yijing Wang wrote:
>> Now we could use pci_scan_host_bridge() to scan
>> pci buses, provide powerpc specific pci_host_bridge_ops.
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> CC: linuxppc-dev@lists.ozlabs.org
>> ---
>>  arch/powerpc/kernel/pci-common.c |   60 +++++++++++++++++++++++--------------
>>  1 files changed, 37 insertions(+), 23 deletions(-)
> 
>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>> index 2c58200..e2b50a2 100644
>> --- a/arch/powerpc/kernel/pci-common.c
>> +++ b/arch/powerpc/kernel/pci-common.c
>> @@ -773,6 +773,29 @@ void pcibios_set_root_bus_speed(struct pci_host_bridge *bridge)
>>  		return ppc_md.pcibios_set_root_bus_speed(bridge);
>>  }
>>  
>> +static int pci_host_scan_bus(struct pci_host_bridge *host)
>> +{
>> +	int mode = PCI_PROBE_NORMAL;
>> +	struct pci_bus *bus = host->bus;
>> +	struct pci_controller *hose = dev_get_drvdata(&host->dev);
> Is there any reason this isn't *hose = pci_bus_to_host(bus)?

Hi Daniel, thanks for your review and comments. We want to make a generic pci_host_bridge,
which would hold the common host information, for example, pci domain is common info for
pci host bridge, this series saved domain in pci_host_bridge, then we no need to
extract out domain by pci_bus->sysdata by platform specific pci_domain_nr().
Also we store the sysdata in pci_host_bridge, and pci_bus_to_host() is the platform
interface, I think use the common interface would be better.

>> +
>> +	/* Get probe mode and perform scan */
>> +	if (hose->dn && ppc_md.pci_probe_mode)
>> +		mode = ppc_md.pci_probe_mode(bus);
>> +
>> +	pr_debug("    probe mode: %d\n", mode);
>> +	if (mode == PCI_PROBE_DEVTREE)
>> +		of_scan_bus(hose->dn, bus);
>> +
>> +	if (mode == PCI_PROBE_NORMAL) {
>> +		pci_bus_update_busn_res_end(bus, 255);
>> +		hose->last_busno = pci_scan_child_bus(bus);
>> +		pci_bus_update_busn_res_end(bus, hose->last_busno);
>> +	}
>> +
>> +	return pci_bus_child_max_busnr(bus);
>> +}
>> +
> I'm having trouble convincing myself that this patch covers every
> variation within our PCI implementations. In particular, there's a
> stanza in of_scan_pci_bridge in kernel/pci_of_scan.c that's almost
> identical to this function. Does that implementation need to be cleaned
> up and replaced with this function too?
> 

This is a pci_host_bridge_ops hook function, which would be called in
PCI core, and after applied this series, we only need to call pci_scan_host_bridge()
to scan pci devices, and this function is also extracted from the pcibios_scan_phb(),
it's not the redundant code.

> 
>> @@ -1641,9 +1655,9 @@ void pcibios_scan_phb(struct pci_controller *hose)
>>  		ppc_md.pcibios_fixup_phb(hose);
>>  
>>  	/* Configure PCI Express settings */
>> -	if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
>> +	if (host->bus && !pci_has_flag(PCI_PROBE_ONLY)) {
>>  		struct pci_bus *child;
>> -		list_for_each_entry(child, &bus->children, node)
>> +		list_for_each_entry(child, &host->bus->children, node)
>>  			pcie_bus_configure_settings(child);
>>  	}
>>  }
> Two things: Firstly, the function uses hose throughout, not host.
> Secondly, you're not deleting the bus variable: what's the purpose of
> this change?

host is the common pci_host_bridge which is created by PCI core for pci host bridge driver,
the hose is the platform data used in powerpc. The purpose of the patch/series is to simplify
pci enumeration interface, and try to reduce the weak functions which were used to setup pci bus/devices
during PCI enumeration.

> 
> Regards,
> Daniel
> 


-- 
Thanks!
Yijing

Re: [PATCH v8 19/30] powerpc/pci: Use pci_scan_host_bridge() for simplicity

[Daniel Axtens]

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

Hi Yijing,

I wasn't quite sure I understood your comments, so I was trying to apply
your patch series and test it, but patch 3 doesn't apply cleanly to
4.0-rc5 or master. Can you respin the series?

Thanks,
Daniel


> Hi Daniel, thanks for your review and comments. We want to make a generic pci_host_bridge,
> which would hold the common host information, for example, pci domain is common info for
> pci host bridge, this series saved domain in pci_host_bridge, then we no need to
> extract out domain by pci_bus->sysdata by platform specific pci_domain_nr().
> Also we store the sysdata in pci_host_bridge, and pci_bus_to_host() is the platform
> interface, I think use the common interface would be better.
> 
> >> +
> >> +	/* Get probe mode and perform scan */
> >> +	if (hose->dn && ppc_md.pci_probe_mode)
> >> +		mode = ppc_md.pci_probe_mode(bus);
> >> +
> >> +	pr_debug("    probe mode: %d\n", mode);
> >> +	if (mode == PCI_PROBE_DEVTREE)
> >> +		of_scan_bus(hose->dn, bus);
> >> +
> >> +	if (mode == PCI_PROBE_NORMAL) {
> >> +		pci_bus_update_busn_res_end(bus, 255);
> >> +		hose->last_busno = pci_scan_child_bus(bus);
> >> +		pci_bus_update_busn_res_end(bus, hose->last_busno);
> >> +	}
> >> +
> >> +	return pci_bus_child_max_busnr(bus);
> >> +}
> >> +
> > I'm having trouble convincing myself that this patch covers every
> > variation within our PCI implementations. In particular, there's a
> > stanza in of_scan_pci_bridge in kernel/pci_of_scan.c that's almost
> > identical to this function. Does that implementation need to be cleaned
> > up and replaced with this function too?
> > 
> 
> This is a pci_host_bridge_ops hook function, which would be called in
> PCI core, and after applied this series, we only need to call pci_scan_host_bridge()
> to scan pci devices, and this function is also extracted from the pcibios_scan_phb(),
> it's not the redundant code.
> 
> > 
> >> @@ -1641,9 +1655,9 @@ void pcibios_scan_phb(struct pci_controller *hose)
> >>  		ppc_md.pcibios_fixup_phb(hose);
> >>  
> >>  	/* Configure PCI Express settings */
> >> -	if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
> >> +	if (host->bus && !pci_has_flag(PCI_PROBE_ONLY)) {
> >>  		struct pci_bus *child;
> >> -		list_for_each_entry(child, &bus->children, node)
> >> +		list_for_each_entry(child, &host->bus->children, node)
> >>  			pcie_bus_configure_settings(child);
> >>  	}
> >>  }
> > Two things: Firstly, the function uses hose throughout, not host.
> > Secondly, you're not deleting the bus variable: what's the purpose of
> > this change?
> 
> host is the common pci_host_bridge which is created by PCI core for pci host bridge driver,
> the hose is the platform data used in powerpc. The purpose of the patch/series is to simplify
> pci enumeration interface, and try to reduce the weak functions which were used to setup pci bus/devices
> during PCI enumeration.
> 
> > 
> > Regards,
> > Daniel
> > 
> 
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 860 bytes --]

Re: [PATCH v8 19/30] powerpc/pci: Use pci_scan_host_bridge() for simplicity

[Yijing Wang]

On 2015/3/26 6:13, Daniel Axtens wrote:
> Hi Yijing,
> 
> I wasn't quite sure I understood your comments, so I was trying to apply
> your patch series and test it, but patch 3 doesn't apply cleanly to
> 4.0-rc5 or master. Can you respin the series?

Hi Daniel,
   Could you pull the series from Bjorn's git tree ? The URL is
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git pci/enumeration-yw8

Thanks!
Yijing.

> 
> Thanks,
> Daniel
> 
> 
>> Hi Daniel, thanks for your review and comments. We want to make a generic pci_host_bridge,
>> which would hold the common host information, for example, pci domain is common info for
>> pci host bridge, this series saved domain in pci_host_bridge, then we no need to
>> extract out domain by pci_bus->sysdata by platform specific pci_domain_nr().
>> Also we store the sysdata in pci_host_bridge, and pci_bus_to_host() is the platform
>> interface, I think use the common interface would be better.
>>
>>>> +
>>>> +	/* Get probe mode and perform scan */
>>>> +	if (hose->dn && ppc_md.pci_probe_mode)
>>>> +		mode = ppc_md.pci_probe_mode(bus);
>>>> +
>>>> +	pr_debug("    probe mode: %d\n", mode);
>>>> +	if (mode == PCI_PROBE_DEVTREE)
>>>> +		of_scan_bus(hose->dn, bus);
>>>> +
>>>> +	if (mode == PCI_PROBE_NORMAL) {
>>>> +		pci_bus_update_busn_res_end(bus, 255);
>>>> +		hose->last_busno = pci_scan_child_bus(bus);
>>>> +		pci_bus_update_busn_res_end(bus, hose->last_busno);
>>>> +	}
>>>> +
>>>> +	return pci_bus_child_max_busnr(bus);
>>>> +}
>>>> +
>>> I'm having trouble convincing myself that this patch covers every
>>> variation within our PCI implementations. In particular, there's a
>>> stanza in of_scan_pci_bridge in kernel/pci_of_scan.c that's almost
>>> identical to this function. Does that implementation need to be cleaned
>>> up and replaced with this function too?
>>>
>>
>> This is a pci_host_bridge_ops hook function, which would be called in
>> PCI core, and after applied this series, we only need to call pci_scan_host_bridge()
>> to scan pci devices, and this function is also extracted from the pcibios_scan_phb(),
>> it's not the redundant code.
>>
>>>
>>>> @@ -1641,9 +1655,9 @@ void pcibios_scan_phb(struct pci_controller *hose)
>>>>  		ppc_md.pcibios_fixup_phb(hose);
>>>>  
>>>>  	/* Configure PCI Express settings */
>>>> -	if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
>>>> +	if (host->bus && !pci_has_flag(PCI_PROBE_ONLY)) {
>>>>  		struct pci_bus *child;
>>>> -		list_for_each_entry(child, &bus->children, node)
>>>> +		list_for_each_entry(child, &host->bus->children, node)
>>>>  			pcie_bus_configure_settings(child);
>>>>  	}
>>>>  }
>>> Two things: Firstly, the function uses hose throughout, not host.
>>> Secondly, you're not deleting the bus variable: what's the purpose of
>>> this change?
>>
>> host is the common pci_host_bridge which is created by PCI core for pci host bridge driver,
>> the hose is the platform data used in powerpc. The purpose of the patch/series is to simplify
>> pci enumeration interface, and try to reduce the weak functions which were used to setup pci bus/devices
>> during PCI enumeration.
>>
>>>
>>> Regards,
>>> Daniel
>>>
>>
>>
> 


-- 
Thanks!
Yijing

Re: [PATCH v8 19/30] powerpc/pci: Use pci_scan_host_bridge() for simplicity

[Daniel Axtens]

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

Hi Yijing,

Pulled.

I'm now getting build errors:

/scratch/dja/linux-patches/arch/powerpc/kernel/pci-common.c: In function 'pcibios_scan_phb':
/scratch/dja/linux-patches/arch/powerpc/kernel/pci-common.c:1638:14: error: 'bus' undeclared (first use in this function)
/scratch/dja/linux-patches/arch/powerpc/kernel/pci-common.c:1638:14: note: each undeclared identifier is reported only once for each function it appears in

That relates to your changes to pcibios_scan_phb:
-       struct pci_bus *bus;
+       struct pci_host_bridge *host;
clashing with line 1638:
	hose->bus = bus;
which occurs later in the same unction.

If I fix that, I'll get more errors with lines in that function that
still reference hose instead of host.

As well as line 1638, there's still big chunks of the function that
reference hose, for example:

	/* Get some IO space for the new PHB */
	pcibios_setup_phb_io_space(hose);

	/* Wire up PHB bus resources */
	pcibios_setup_phb_resources(hose, &resources);

	hose->busn.start = hose->first_busno;
	hose->busn.end	 = hose->last_busno;
	hose->busn.flags = IORESOURCE_BUS;
	pci_add_resource(&resources, &hose->busn);

	pci_host_ops.pci_ops = hose->ops;


Basically, the changes in PowerPC need to be quite a bit more through
just to get the changes to build.

I'm keen to work on simplifying PCI code in PowerPC, but at this point,
it will need a lot more work before it's ready to go in.

Regards,
Daniel


On Thu, 2015-03-26 at 09:17 +0800, Yijing Wang wrote:
> On 2015/3/26 6:13, Daniel Axtens wrote:
> > Hi Yijing,
> > 
> > I wasn't quite sure I understood your comments, so I was trying to apply
> > your patch series and test it, but patch 3 doesn't apply cleanly to
> > 4.0-rc5 or master. Can you respin the series?
> 
> Hi Daniel,
>    Could you pull the series from Bjorn's git tree ? The URL is
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git pci/enumeration-yw8
> 
> Thanks!
> Yijing.
> 
> > 
> > Thanks,
> > Daniel
> > 
> > 
> >> Hi Daniel, thanks for your review and comments. We want to make a generic pci_host_bridge,
> >> which would hold the common host information, for example, pci domain is common info for
> >> pci host bridge, this series saved domain in pci_host_bridge, then we no need to
> >> extract out domain by pci_bus->sysdata by platform specific pci_domain_nr().
> >> Also we store the sysdata in pci_host_bridge, and pci_bus_to_host() is the platform
> >> interface, I think use the common interface would be better.
> >>
> >>>> +
> >>>> +	/* Get probe mode and perform scan */
> >>>> +	if (hose->dn && ppc_md.pci_probe_mode)
> >>>> +		mode = ppc_md.pci_probe_mode(bus);
> >>>> +
> >>>> +	pr_debug("    probe mode: %d\n", mode);
> >>>> +	if (mode == PCI_PROBE_DEVTREE)
> >>>> +		of_scan_bus(hose->dn, bus);
> >>>> +
> >>>> +	if (mode == PCI_PROBE_NORMAL) {
> >>>> +		pci_bus_update_busn_res_end(bus, 255);
> >>>> +		hose->last_busno = pci_scan_child_bus(bus);
> >>>> +		pci_bus_update_busn_res_end(bus, hose->last_busno);
> >>>> +	}
> >>>> +
> >>>> +	return pci_bus_child_max_busnr(bus);
> >>>> +}
> >>>> +
> >>> I'm having trouble convincing myself that this patch covers every
> >>> variation within our PCI implementations. In particular, there's a
> >>> stanza in of_scan_pci_bridge in kernel/pci_of_scan.c that's almost
> >>> identical to this function. Does that implementation need to be cleaned
> >>> up and replaced with this function too?
> >>>
> >>
> >> This is a pci_host_bridge_ops hook function, which would be called in
> >> PCI core, and after applied this series, we only need to call pci_scan_host_bridge()
> >> to scan pci devices, and this function is also extracted from the pcibios_scan_phb(),
> >> it's not the redundant code.
> >>
> >>>
> >>>> @@ -1641,9 +1655,9 @@ void pcibios_scan_phb(struct pci_controller *hose)
> >>>>  		ppc_md.pcibios_fixup_phb(hose);
> >>>>  
> >>>>  	/* Configure PCI Express settings */
> >>>> -	if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
> >>>> +	if (host->bus && !pci_has_flag(PCI_PROBE_ONLY)) {
> >>>>  		struct pci_bus *child;
> >>>> -		list_for_each_entry(child, &bus->children, node)
> >>>> +		list_for_each_entry(child, &host->bus->children, node)
> >>>>  			pcie_bus_configure_settings(child);
> >>>>  	}
> >>>>  }
> >>> Two things: Firstly, the function uses hose throughout, not host.
> >>> Secondly, you're not deleting the bus variable: what's the purpose of
> >>> this change?
> >>
> >> host is the common pci_host_bridge which is created by PCI core for pci host bridge driver,
> >> the hose is the platform data used in powerpc. The purpose of the patch/series is to simplify
> >> pci enumeration interface, and try to reduce the weak functions which were used to setup pci bus/devices
> >> during PCI enumeration.
> >>
> >>>
> >>> Regards,
> >>> Daniel
> >>>
> >>
> >>
> > 
> 
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 860 bytes --]

Re: [PATCH v8 19/30] powerpc/pci: Use pci_scan_host_bridge() for simplicity

[Yijing Wang]

On 2015/3/26 13:19, Daniel Axtens wrote:
> Hi Yijing,
> 
> Pulled.
> 
> I'm now getting build errors:
> 
> /scratch/dja/linux-patches/arch/powerpc/kernel/pci-common.c: In function 'pcibios_scan_phb':
> /scratch/dja/linux-patches/arch/powerpc/kernel/pci-common.c:1638:14: error: 'bus' undeclared (first use in this function)
> /scratch/dja/linux-patches/arch/powerpc/kernel/pci-common.c:1638:14: note: each undeclared identifier is reported only once for each function it appears in
> 
> That relates to your changes to pcibios_scan_phb:
> -       struct pci_bus *bus;
> +       struct pci_host_bridge *host;
> clashing with line 1638:
> 	hose->bus = bus;
> which occurs later in the same unction.
> 
> If I fix that, I'll get more errors with lines in that function that
> still reference hose instead of host.
> 
> As well as line 1638, there's still big chunks of the function that
> reference hose, for example:
> 
> 	/* Get some IO space for the new PHB */
> 	pcibios_setup_phb_io_space(hose);
> 
> 	/* Wire up PHB bus resources */
> 	pcibios_setup_phb_resources(hose, &resources);
> 
> 	hose->busn.start = hose->first_busno;
> 	hose->busn.end	 = hose->last_busno;
> 	hose->busn.flags = IORESOURCE_BUS;
> 	pci_add_resource(&resources, &hose->busn);
> 
> 	pci_host_ops.pci_ops = hose->ops;
> 
> 
> Basically, the changes in PowerPC need to be quite a bit more through
> just to get the changes to build.
> 
> I'm keen to work on simplifying PCI code in PowerPC, but at this point,
> it will need a lot more work before it's ready to go in.

Hi Daniel, I'm really so sorry about that, I will update it today.

Thanks!
Yijing.

> 
> Regards,
> Daniel
> 
> 
> On Thu, 2015-03-26 at 09:17 +0800, Yijing Wang wrote:
>> On 2015/3/26 6:13, Daniel Axtens wrote:
>>> Hi Yijing,
>>>
>>> I wasn't quite sure I understood your comments, so I was trying to apply
>>> your patch series and test it, but patch 3 doesn't apply cleanly to
>>> 4.0-rc5 or master. Can you respin the series?
>>
>> Hi Daniel,
>>    Could you pull the series from Bjorn's git tree ? The URL is
>> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git pci/enumeration-yw8
>>
>> Thanks!
>> Yijing.
>>
>>>
>>> Thanks,
>>> Daniel
>>>
>>>
>>>> Hi Daniel, thanks for your review and comments. We want to make a generic pci_host_bridge,
>>>> which would hold the common host information, for example, pci domain is common info for
>>>> pci host bridge, this series saved domain in pci_host_bridge, then we no need to
>>>> extract out domain by pci_bus->sysdata by platform specific pci_domain_nr().
>>>> Also we store the sysdata in pci_host_bridge, and pci_bus_to_host() is the platform
>>>> interface, I think use the common interface would be better.
>>>>
>>>>>> +
>>>>>> +	/* Get probe mode and perform scan */
>>>>>> +	if (hose->dn && ppc_md.pci_probe_mode)
>>>>>> +		mode = ppc_md.pci_probe_mode(bus);
>>>>>> +
>>>>>> +	pr_debug("    probe mode: %d\n", mode);
>>>>>> +	if (mode == PCI_PROBE_DEVTREE)
>>>>>> +		of_scan_bus(hose->dn, bus);
>>>>>> +
>>>>>> +	if (mode == PCI_PROBE_NORMAL) {
>>>>>> +		pci_bus_update_busn_res_end(bus, 255);
>>>>>> +		hose->last_busno = pci_scan_child_bus(bus);
>>>>>> +		pci_bus_update_busn_res_end(bus, hose->last_busno);
>>>>>> +	}
>>>>>> +
>>>>>> +	return pci_bus_child_max_busnr(bus);
>>>>>> +}
>>>>>> +
>>>>> I'm having trouble convincing myself that this patch covers every
>>>>> variation within our PCI implementations. In particular, there's a
>>>>> stanza in of_scan_pci_bridge in kernel/pci_of_scan.c that's almost
>>>>> identical to this function. Does that implementation need to be cleaned
>>>>> up and replaced with this function too?
>>>>>
>>>>
>>>> This is a pci_host_bridge_ops hook function, which would be called in
>>>> PCI core, and after applied this series, we only need to call pci_scan_host_bridge()
>>>> to scan pci devices, and this function is also extracted from the pcibios_scan_phb(),
>>>> it's not the redundant code.
>>>>
>>>>>
>>>>>> @@ -1641,9 +1655,9 @@ void pcibios_scan_phb(struct pci_controller *hose)
>>>>>>  		ppc_md.pcibios_fixup_phb(hose);
>>>>>>  
>>>>>>  	/* Configure PCI Express settings */
>>>>>> -	if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
>>>>>> +	if (host->bus && !pci_has_flag(PCI_PROBE_ONLY)) {
>>>>>>  		struct pci_bus *child;
>>>>>> -		list_for_each_entry(child, &bus->children, node)
>>>>>> +		list_for_each_entry(child, &host->bus->children, node)
>>>>>>  			pcie_bus_configure_settings(child);
>>>>>>  	}
>>>>>>  }
>>>>> Two things: Firstly, the function uses hose throughout, not host.
>>>>> Secondly, you're not deleting the bus variable: what's the purpose of
>>>>> this change?
>>>>
>>>> host is the common pci_host_bridge which is created by PCI core for pci host bridge driver,
>>>> the hose is the platform data used in powerpc. The purpose of the patch/series is to simplify
>>>> pci enumeration interface, and try to reduce the weak functions which were used to setup pci bus/devices
>>>> during PCI enumeration.
>>>>
>>>>>
>>>>> Regards,
>>>>> Daniel
>>>>>
>>>>
>>>>
>>>
>>
>>
> 


-- 
Thanks!
Yijing