Re: [PATCH] powerpc/fsl-pci: use 'Header Type' to identify PCIE mode

On Sep 20, 2012, at 10:37 PM, Lian Minghaun-b31939 wrote:

Hi Kumar,
=20
please see my comments inline.
=20
=20
On 09/19/2012 10:22 PM, Kumar Gala wrote:

quoted

On Sep 19, 2012, at 2:23 AM, Minghuan Lian wrote:
=20

quoted

The original code uses 'Programming Interface' field to judge if =

PCIE is

quoted

quoted

EP or RC mode, however, some latest silicons do not support this =

functionality.

quoted

quoted

According to PCIE specification, 'Header Type' offset 0x0e is used =

to

quoted

quoted

indicate header type, so change code to use 'Header Type' field to
judge PCIE mode. Because FSL PCI controller does not support 'Header =

Type',

quoted

quoted

patch still uses 'Programming Interface' to identify PCI mode.
=20
Signed-off-by: Minghuan Lian <redacted>
Signed-off-by: Roy Zang <redacted>
---
arch/powerpc/sysdev/fsl_pci.c |   38 =

+++++++++++++++++++++++---------------

quoted

quoted

1 file changed, 23 insertions(+), 15 deletions(-)
=20

diff --git a/arch/powerpc/sysdev/fsl_pci.c =

b/arch/powerpc/sysdev/fsl_pci.c

quoted

quoted

index c37f461..43d30df 100644

--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c

@@ -38,15 +38,15 @@ static int fsl_pcie_bus_fixup, is_mpc83xx_pci;

=20
static void __devinit quirk_fsl_pcie_header(struct pci_dev *dev)
{
-	u8 progif;
+	u8 hdr_type;
=20
	/* if we aren't a PCIe don't bother */
	if (!pci_find_capability(dev, PCI_CAP_ID_EXP))
		return;
=20
	/* if we aren't in host mode don't bother */
-	pci_read_config_byte(dev, PCI_CLASS_PROG, &progif);
-	if (progif & 0x1)
+	pci_read_config_byte(dev, PCI_HEADER_TYPE, &hdr_type);
+	if ((hdr_type & 0x7f) !=3D PCI_HEADER_TYPE_BRIDGE)
		return;
=20
	dev->class =3D PCI_CLASS_BRIDGE_PCI << 8;

@@ -425,7 +425,7 @@ int __init fsl_add_bridge(struct device_node =

*dev, int is_primary)

quoted

quoted

	struct pci_controller *hose;
	struct resource rsrc;
	const int *bus_range;
-	u8 progif;
+	u8 hdr_type, progif;
=20
	if (!of_device_is_available(dev)) {
		pr_warning("%s: disabled\n", dev->full_name);

@@ -457,25 +457,24 @@ int __init fsl_add_bridge(struct device_node =

*dev, int is_primary)

quoted

quoted

	setup_indirect_pci(hose, rsrc.start, rsrc.start + 0x4,
		PPC_INDIRECT_TYPE_BIG_ENDIAN);
=20
-	early_read_config_byte(hose, 0, 0, PCI_CLASS_PROG, &progif);
-	if ((progif & 1) =3D=3D 1) {
-		/* unmap cfg_data & cfg_addr separately if not on same =

page */

quoted

quoted

-		if (((unsigned long)hose->cfg_data & PAGE_MASK) !=3D
-		    ((unsigned long)hose->cfg_addr & PAGE_MASK))
-			iounmap(hose->cfg_data);
-		iounmap(hose->cfg_addr);
-		pcibios_free_controller(hose);
-		return -ENODEV;
-	}
-
	setup_pci_cmd(hose);

I think we should be doing the check before we call setup_pci_cmd().  =

The old code didn't touch the controller registers if we where and =
end-point.  We should maintain that.

[Minghuan] Thanks for you pointing this.
I want to move setup_pci_cmd like this:
=20
pr_debug(" ->Hose at 0x%p, cfg_addr=3D0x%p,cfg_data=3D0x%p\n",
       hose, hose->cfg_addr, hose->cfg_data);
=20
+    setup_pci_cmd(hose);
=20
   /* Interpret the "ranges" property */
   /* This also maps the I/O region and sets isa_io/mem_base */
   pci_process_bridge_OF_ranges(hose, dev, is_primary);
=20
This movement will cause fsl_pcie_check_link() calling before =

setup_pci_cmd().

Is this ok?

I think so, as its how the code is today:

setup_pci_cmd()
..
if (pcie)
	fsl_pcie_check_link()

If not, I will call setup_pci_cmd() for PCI and PCIE respectively.