Kirkwood: add support for rev A1 of the 88f6192 and 88f6180 chips.

This patch extens the kirkwood's PCIe support up to 2 controllers as in the 6282 devices.

Signed-off-by Saeed Bishara <[email protected]>

arch/arm/mach-kirkwood/pcie.c +58 -35

 void __init kirkwood_pcie_id(u32 *dev, u32 *rev)
 {
 	*dev = orion_pcie_dev_id(PCIE_BASE);
 	*rev = orion_pcie_rev(PCIE_BASE);
 }
 
-static int pcie_valid_config(int bus, int dev)
+static int pcie_valid_config(struct pcie_port *pp, int bus, int dev)
 {
 	/*
 	 * Don't go out when trying to access --
 	 * 1. nonexisting device on local bus
 	 * 2. where there's no device connected (no link)
 	 */
-	if (bus == 0 && dev == 0)
+	if (bus == pp->root_bus_nr && dev == 0)
 		return 1;
 
-	if (!orion_pcie_link_up(PCIE_BASE))
+	if (!orion_pcie_link_up(pp->base))
 		return 0;
 
-	if (bus == 0 && dev != 1)
+	if (bus == pp->root_bus_nr && dev != 1)
 		return 0;
 
 	return 1;

@@ -52,22 +66,22 @@ static int pcie_valid_config(int bus, int dev)
  * and then reading the PCIE_CONF_DATA register. Need to make sure these
  * transactions are atomic.
  */
-static DEFINE_SPINLOCK(kirkwood_pcie_lock);
 
 static int pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 			int size, u32 *val)
 {
+	struct pcie_port *pp = bus_to_port(bus->number);
 	unsigned long flags;
 	int ret;
 
-	if (pcie_valid_config(bus->number, PCI_SLOT(devfn)) == 0) {
+	if (pcie_valid_config(pp, bus->number, PCI_SLOT(devfn)) == 0) {
 		*val = 0xffffffff;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
-	spin_lock_irqsave(&kirkwood_pcie_lock, flags);
-	ret = orion_pcie_rd_conf(PCIE_BASE, bus, devfn, where, size, val);
-	spin_unlock_irqrestore(&kirkwood_pcie_lock, flags);
+	spin_lock_irqsave(&pp->conf_lock, flags);
+	ret = orion_pcie_rd_conf(pp->base, bus, devfn, where, size, val);
+	spin_unlock_irqrestore(&pp->conf_lock, flags);
 
 	return ret;
 }

@@ -75,15 +89,16 @@ static int pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 static int pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 			int where, int size, u32 val)
 {
+	struct pcie_port *pp = bus_to_port(bus->number);
 	unsigned long flags;
 	int ret;
 
-	if (pcie_valid_config(bus->number, PCI_SLOT(devfn)) == 0)
+	if (pcie_valid_config(pp, bus->number, PCI_SLOT(devfn)) == 0)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
-	spin_lock_irqsave(&kirkwood_pcie_lock, flags);
-	ret = orion_pcie_wr_conf(PCIE_BASE, bus, devfn, where, size, val);
-	spin_unlock_irqrestore(&kirkwood_pcie_lock, flags);
+	spin_lock_irqsave(&pp->conf_lock, flags);
+	ret = orion_pcie_wr_conf(pp->base, bus, devfn, where, size, val);
+	spin_unlock_irqrestore(&pp->conf_lock, flags);
 
 	return ret;
 }

@@ -96,51 +111,84 @@ static struct pci_ops pcie_ops = {
 
 static int __init kirkwood_pcie_setup(int nr, struct pci_sys_data *sys)
 {
-	struct resource *res;
 	extern unsigned int kirkwood_clk_ctrl;
+	struct pcie_port *pp;
+
+	if (nr >= num_pcie_ports)
+		return 0;
+
+	pp = &pcie_port[nr];
+	pp->root_bus_nr = sys->busnr;
 
 	/*
 	 * Generic PCIe unit setup.
 	 */
-	orion_pcie_setup(PCIE_BASE, &kirkwood_mbus_dram_info);
+	orion_pcie_set_local_bus_nr(pp->base, sys->busnr);
+
+	orion_pcie_setup(pp->base, &kirkwood_mbus_dram_info);
 
-	/*
-	 * Request resources.
-	 */
-	res = kzalloc(sizeof(struct resource) * 2, GFP_KERNEL);
-	if (!res)
-		panic("pcie_setup unable to alloc resources");
 
 	/*
 	 * IORESOURCE_IO
 	 */
-	res[0].name = "PCIe I/O Space";
-	res[0].flags = IORESOURCE_IO;
-	res[0].start = KIRKWOOD_PCIE_IO_BUS_BASE;
-	res[0].end = res[0].start + KIRKWOOD_PCIE_IO_SIZE - 1;
-	if (request_resource(&ioport_resource, &res[0]))
+	snprintf(pp->io_space_name, sizeof(pp->io_space_name),
+		 "PCIe %d I/O", pp->index);
+	pp->io_space_name[sizeof(pp->io_space_name) - 1] = 0;
+	pp->res[0].name = pp->io_space_name;
+	if (pp->index == 0) {
+		pp->res[0].start = KIRKWOOD_PCIE_IO_PHYS_BASE;
+		pp->res[0].end = pp->res[0].start + KIRKWOOD_PCIE_IO_SIZE - 1;
+	} else {
+		pp->res[0].start = KIRKWOOD_PCIE1_IO_PHYS_BASE;
+		pp->res[0].end = pp->res[0].start + KIRKWOOD_PCIE1_IO_SIZE - 1;
+	}
+	pp->res[0].flags = IORESOURCE_IO;
+	if (request_resource(&ioport_resource, &pp->res[0]))
 		panic("Request PCIe IO resource failed\n");
-	sys->resource[0] = &res[0];
+	sys->resource[0] = &pp->res[0];
 
 	/*
 	 * IORESOURCE_MEM
 	 */
-	res[1].name = "PCIe Memory Space";
-	res[1].flags = IORESOURCE_MEM;
-	res[1].start = KIRKWOOD_PCIE_MEM_BUS_BASE;
-	res[1].end = res[1].start + KIRKWOOD_PCIE_MEM_SIZE - 1;
-	if (request_resource(&iomem_resource, &res[1]))
+	snprintf(pp->mem_space_name, sizeof(pp->mem_space_name),
+		 "PCIe %d MEM", pp->index);
+	pp->mem_space_name[sizeof(pp->mem_space_name) - 1] = 0;
+	pp->res[1].name = pp->mem_space_name;
+	if (pp->index == 0) {
+		pp->res[1].start = KIRKWOOD_PCIE_MEM_PHYS_BASE;
+		pp->res[1].end = pp->res[1].start + KIRKWOOD_PCIE_MEM_SIZE - 1;
+	} else {
+		pp->res[1].start = KIRKWOOD_PCIE1_MEM_PHYS_BASE;
+		pp->res[1].end = pp->res[1].start + KIRKWOOD_PCIE1_MEM_SIZE - 1;
+	}
+	pp->res[1].flags = IORESOURCE_MEM;
+	if (request_resource(&iomem_resource, &pp->res[1]))
 		panic("Request PCIe Memory resource failed\n");
-	sys->resource[1] = &res[1];
+	sys->resource[1] = &pp->res[1];
 
 	sys->resource[2] = NULL;
 	sys->io_offset = 0;
 
-	kirkwood_clk_ctrl |= CGC_PEX0;
-
+	if (nr == 0)
+		kirkwood_clk_ctrl |= CGC_PEX0;
+	else
+		kirkwood_clk_ctrl |= CGC_PEX1;
 	return 1;
 }
 
+static struct pcie_port *bus_to_port(int bus)
+{

arch/arm/mach-kirkwood/pcie.c +1 -1

 static void __devinit rc_pci_fixup(struct pci_dev *dev)
 {
 	/*

@@ -163,7 +211,7 @@ kirkwood_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 {
 	struct pci_bus *bus;
 
-	if (nr == 0) {
+	if (nr < num_pcie_ports) {
 		bus = pci_scan_bus(sys->busnr, &pcie_ops, sys);
 	} else {
 		bus = NULL;

arch/arm/mach-kirkwood/pcie.c +9 -1

 	.swizzle	= pci_std_swizzle,
 	.setup		= kirkwood_pcie_setup,
 	.scan		= kirkwood_pcie_scan_bus,
 	.map_irq	= kirkwood_pcie_map_irq,
 };
 
-void __init kirkwood_pcie_init(void)
+static void __init add_pcie_port(int index, unsigned long base)
 {
+	printk(KERN_INFO "Kirkwood PCIe port %d: ", index);
+
+	if (orion_pcie_link_up((void __iomem *)base)) {
+		struct pcie_port *pp = &pcie_port[num_pcie_ports++];
+
+		printk(KERN_INFO "link up\n");
+
+		pp->index = index;

I think this looks like a bit too much code for what this does. ?To simplify things, you could:

1) Duplicate kirkwood_pcie_setup() into kirkwood_pcie0_setup() and ? kirkwood_pcie1_setup() instead. ?This will remove a couple ? "if (pp->index == 0)" and the snprintf() which could remain ? purely static initializations.

2) Remove bus_to_port() entirely, and store the address of the appropriate ? struct pcie_port instance into the sysdata pointer of the pci_bus ? structure. ?This struct pcie_port could even be kmalloc'd as needed.

Quoted 8 lines

I think this looks like a bit too much code for what this does. ?To simplify things, you could:

1) Duplicate kirkwood_pcie_setup() into kirkwood_pcie0_setup() and ? kirkwood_pcie1_setup() instead. ?This will remove a couple ? "if (pp->index == 0)" and the snprintf() which could remain ? purely static initializations.

Quoted 3 lines

2) Remove bus_to_port() entirely, and store the address of the appropriate ? struct pcie_port instance into the sysdata pointer of the pci_bus ? structure. ?This struct pcie_port could even be kmalloc'd as needed.

The sysdata already used by arm to store the pci_sys_data,

I think we will have to add a private pointer to the pci_sys_data structure, that pointer will hold mach specific data per pci controller. this pointer is needed for any platform that has multiple pci controller. agree?

Quoted 3 lines · Saeed Bishara

This patch extens the kirkwood's PCIe support up to 2 controllers as in the 6282 devices.

Signed-off-by Saeed Bishara <[email protected]>

Comments below.

[...]

arch/arm/mach-kirkwood/mv88f6281gtw_ge-setup.c static void __init mv88f6281gtw_ge_init(void) · line 158

Quoted 10 lines

arch/arm/mach-kirkwood/mv88f6281gtw_ge-setup.c +1 -1

index 5e6f711..6e67f3c 100644
@@ -155,7 +155,7 @@ static void __init mv88f6281gtw_ge_init(void)
 ?static int __init mv88f6281gtw_ge_pci_init(void)
 ?{
 ? ? ? if (machine_is_mv88f6281gtw_ge())
- ? ? ? ? ? ? kirkwood_pcie_init();
+ ? ? ? ? ? ? kirkwood_pcie_init(0, 1);

This is going to initialize the non existing PCIe1 and disable PCIe0. You made the same mistake multiple times.

[...]

arch/arm/mach-kirkwood/pcie.c line 22

Quoted 7 lines

arch/arm/mach-kirkwood/pcie.c +0 -0

index dee1eff..5c33324 100644
@@ -21,26 +21,40 @@
 
 ?#define PCIE_BASE ? ?((void __iomem *)PCIE_VIRT_BASE)

Is this PCIE_BASE still used?

Quoted 4 lines

arch/arm/mach-kirkwood/pcie.c +3 -0

+struct pcie_port {
+ ? ? u8 ? ? ? ? ? ? ? ? ? ? ?index;
+ ? ? u8 ? ? ? ? ? ? ? ? ? ? ?root_bus_nr;

arch/arm/mach-kirkwood/pcie.c line 35

Quoted 10 lines

arch/arm/mach-kirkwood/pcie.c +10 -0

+ ? ? void __iomem ? ? ? ? ? ?*base;
+ ? ? spinlock_t ? ? ? ? ? ? ?conf_lock;
+ ? ? char ? ? ? ? ? ? ? ? ? ?io_space_name[16];
+ ? ? char ? ? ? ? ? ? ? ? ? ?mem_space_name[16];
+ ? ? struct resource ? ? ? ? res[2];
+};
+
+static struct pcie_port pcie_port[2];
+static int num_pcie_ports;
+static struct pcie_port *bus_to_port(int bus);

You should move the actual function here allowing you to get rid of that forward declaration.

Quoted 169 lines

arch/arm/mach-kirkwood/pcie.c +1 -1

 ?void __init kirkwood_pcie_id(u32 *dev, u32 *rev)
 ?{
 ? ? ? *dev = orion_pcie_dev_id(PCIE_BASE);
 ? ? ? *rev = orion_pcie_rev(PCIE_BASE);
 ?}
 
-static int pcie_valid_config(int bus, int dev)
+static int pcie_valid_config(struct pcie_port *pp, int bus, int dev)
 ?{

? ? ? /*
? ? ? ?* Don't go out when trying to access -- ? ? ? ?* 1. nonexisting device on local bus
? ? ? ?* 2. where there's no device connected (no link) ? ? ? ?*/
- ? ? if (bus == 0 && dev == 0)
+ ? ? if (bus == pp->root_bus_nr && dev == 0) ? ? ? ? ? ? ? return 1;

- ? ? if (!orion_pcie_link_up(PCIE_BASE))
+ ? ? if (!orion_pcie_link_up(pp->base))
? ? ? ? ? ? ? return 0;

- ? ? if (bus == 0 && dev != 1)
+ ? ? if (bus == pp->root_bus_nr && dev != 1)
? ? ? ? ? ? ? return 0;

? ? ? return 1;

arch/arm/mach-kirkwood/pcie.c +0 -0

@@ -52,22 +66,22 @@ static int pcie_valid_config(int bus, int dev)

? * and then reading the PCIE_CONF_DATA register. Need to make sure these ? * transactions are atomic.
? */
-static DEFINE_SPINLOCK(kirkwood_pcie_lock);

?static int pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, ? ? ? ? ? ? ? ? ? ? ? int size, u32 *val)
?{
+ ? ? struct pcie_port *pp = bus_to_port(bus->number);
? ? ? unsigned long flags;
? ? ? int ret;

- ? ? if (pcie_valid_config(bus->number, PCI_SLOT(devfn)) == 0) {
+ ? ? if (pcie_valid_config(pp, bus->number, PCI_SLOT(devfn)) == 0) { ? ? ? ? ? ? ? *val = 0xffffffff;
? ? ? ? ? ? ? return PCIBIOS_DEVICE_NOT_FOUND;
? ? ? }

- ? ? spin_lock_irqsave(&kirkwood_pcie_lock, flags);
- ? ? ret = orion_pcie_rd_conf(PCIE_BASE, bus, devfn, where, size, val);
- ? ? spin_unlock_irqrestore(&kirkwood_pcie_lock, flags);
+ ? ? spin_lock_irqsave(&pp->conf_lock, flags);
+ ? ? ret = orion_pcie_rd_conf(pp->base, bus, devfn, where, size, val);
+ ? ? spin_unlock_irqrestore(&pp->conf_lock, flags);

? ? ? return ret;
?}

arch/arm/mach-kirkwood/pcie.c +12 -5

@@ -75,15 +89,16 @@ static int pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 ?static int pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 ? ? ? ? ? ? ? ? ? ? ? int where, int size, u32 val)
 ?{
+ ? ? struct pcie_port *pp = bus_to_port(bus->number);
 ? ? ? unsigned long flags;
 ? ? ? int ret;
 
- ? ? if (pcie_valid_config(bus->number, PCI_SLOT(devfn)) == 0)
+ ? ? if (pcie_valid_config(pp, bus->number, PCI_SLOT(devfn)) == 0)
 ? ? ? ? ? ? ? return PCIBIOS_DEVICE_NOT_FOUND;
 
- ? ? spin_lock_irqsave(&kirkwood_pcie_lock, flags);
- ? ? ret = orion_pcie_wr_conf(PCIE_BASE, bus, devfn, where, size, val);
- ? ? spin_unlock_irqrestore(&kirkwood_pcie_lock, flags);
+ ? ? spin_lock_irqsave(&pp->conf_lock, flags);
+ ? ? ret = orion_pcie_wr_conf(pp->base, bus, devfn, where, size, val);
+ ? ? spin_unlock_irqrestore(&pp->conf_lock, flags);
 
 ? ? ? return ret;
 ?}

@@ -96,51 +111,84 @@ static struct pci_ops pcie_ops = {
 
 ?static int __init kirkwood_pcie_setup(int nr, struct pci_sys_data *sys)
 ?{
- ? ? struct resource *res;
 ? ? ? extern unsigned int kirkwood_clk_ctrl;
+ ? ? struct pcie_port *pp;
+
+ ? ? if (nr >= num_pcie_ports)
+ ? ? ? ? ? ? return 0;
+
+ ? ? pp = &pcie_port[nr];
+ ? ? pp->root_bus_nr = sys->busnr;
 

? ? ? /*
? ? ? ?* Generic PCIe unit setup.
? ? ? ?*/
- ? ? orion_pcie_setup(PCIE_BASE, &kirkwood_mbus_dram_info);
+ ? ? orion_pcie_set_local_bus_nr(pp->base, sys->busnr); +
+ ? ? orion_pcie_setup(pp->base, &kirkwood_mbus_dram_info);

- ? ? /*
- ? ? ?* Request resources.
- ? ? ?*/
- ? ? res = kzalloc(sizeof(struct resource) * 2, GFP_KERNEL);
- ? ? if (!res)
- ? ? ? ? ? ? panic("pcie_setup unable to alloc resources");

? ? ? /*
? ? ? ?* IORESOURCE_IO
? ? ? ?*/
- ? ? res[0].name = "PCIe I/O Space";
- ? ? res[0].flags = IORESOURCE_IO;
- ? ? res[0].start = KIRKWOOD_PCIE_IO_BUS_BASE;
- ? ? res[0].end = res[0].start + KIRKWOOD_PCIE_IO_SIZE - 1;
- ? ? if (request_resource(&ioport_resource, &res[0]))
+ ? ? snprintf(pp->io_space_name, sizeof(pp->io_space_name),
+ ? ? ? ? ? ? ?"PCIe %d I/O", pp->index);
+ ? ? pp->io_space_name[sizeof(pp->io_space_name) - 1] = 0;
+ ? ? pp->res[0].name = pp->io_space_name;
+ ? ? if (pp->index == 0) {
+ ? ? ? ? ? ? pp->res[0].start = KIRKWOOD_PCIE_IO_PHYS_BASE;
+ ? ? ? ? ? ? pp->res[0].end = pp->res[0].start + KIRKWOOD_PCIE_IO_SIZE - 1;
+ ? ? } else {
+ ? ? ? ? ? ? pp->res[0].start = KIRKWOOD_PCIE1_IO_PHYS_BASE;
+ ? ? ? ? ? ? pp->res[0].end = pp->res[0].start + KIRKWOOD_PCIE1_IO_SIZE - 1;
+ ? ? }
+ ? ? pp->res[0].flags = IORESOURCE_IO;
+ ? ? if (request_resource(&ioport_resource, &pp->res[0]))
? ? ? ? ? ? ? panic("Request PCIe IO resource failed\n");
- ? ? sys->resource[0] = &res[0];
+ ? ? sys->resource[0] = &pp->res[0];

? ? ? /*
? ? ? ?* IORESOURCE_MEM
? ? ? ?*/
- ? ? res[1].name = "PCIe Memory Space";
- ? ? res[1].flags = IORESOURCE_MEM;
- ? ? res[1].start = KIRKWOOD_PCIE_MEM_BUS_BASE;
- ? ? res[1].end = res[1].start + KIRKWOOD_PCIE_MEM_SIZE - 1;
- ? ? if (request_resource(&iomem_resource, &res[1]))
+ ? ? snprintf(pp->mem_space_name, sizeof(pp->mem_space_name),
+ ? ? ? ? ? ? ?"PCIe %d MEM", pp->index);
+ ? ? pp->mem_space_name[sizeof(pp->mem_space_name) - 1] = 0;
+ ? ? pp->res[1].name = pp->mem_space_name;
+ ? ? if (pp->index == 0) {
+ ? ? ? ? ? ? pp->res[1].start = KIRKWOOD_PCIE_MEM_PHYS_BASE;
+ ? ? ? ? ? ? pp->res[1].end = pp->res[1].start + KIRKWOOD_PCIE_MEM_SIZE - 1;
+ ? ? } else {
+ ? ? ? ? ? ? pp->res[1].start = KIRKWOOD_PCIE1_MEM_PHYS_BASE;
+ ? ? ? ? ? ? pp->res[1].end = pp->res[1].start + KIRKWOOD_PCIE1_MEM_SIZE - 1;
+ ? ? }
+ ? ? pp->res[1].flags = IORESOURCE_MEM;
+ ? ? if (request_resource(&iomem_resource, &pp->res[1]))
? ? ? ? ? ? ? panic("Request PCIe Memory resource failed\n");
- ? ? sys->resource[1] = &res[1];
+ ? ? sys->resource[1] = &pp->res[1];

? ? ? sys->resource[2] = NULL;
? ? ? sys->io_offset = 0;

- ? ? kirkwood_clk_ctrl |= CGC_PEX0;
-
+ ? ? if (nr == 0)
+ ? ? ? ? ? ? kirkwood_clk_ctrl |= CGC_PEX0;
+ ? ? else
+ ? ? ? ? ? ? kirkwood_clk_ctrl |= CGC_PEX1;
? ? ? return 1;
?}

arch/arm/mach-kirkwood/pcie.c +2 -0

+static struct pcie_port *bus_to_port(int bus)
+{

arch/arm/mach-kirkwood/pcie.c

Quoted 10 lines

arch/arm/mach-kirkwood/pcie.c +10 -0

+ ? ? int i;
+
+ ? ? for (i = num_pcie_ports - 1; i >= 0; i--) {
+ ? ? ? ? ? ? int rbus = pcie_port[i].root_bus_nr;
+ ? ? ? ? ? ? if (rbus != -1 && rbus <= bus)
+ ? ? ? ? ? ? ? ? ? ? break;
+ ? ? }
+
+ ? ? return i >= 0 ? pcie_port + i : NULL;
+}

I think this looks like a bit too much code for what this does. ?To simplify things, you could:

1) Duplicate kirkwood_pcie_setup() into kirkwood_pcie0_setup() and ? kirkwood_pcie1_setup() instead. ?This will remove a couple ? "if (pp->index == 0)" and the snprintf() which could remain ? purely static initializations.

2) Remove bus_to_port() entirely, and store the address of the appropriate ? struct pcie_port instance into the sysdata pointer of the pci_bus ? structure. ?This struct pcie_port could even be kmalloc'd as needed.

Quoted 13 lines

?static void __devinit rc_pci_fixup(struct pci_dev *dev) ?{
? ? ? /*

arch/arm/mach-kirkwood/pcie.c +1 -1

@@ -163,7 +211,7 @@ kirkwood_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 ?{
 ? ? ? struct pci_bus *bus;
 
- ? ? if (nr == 0) {
+ ? ? if (nr < num_pcie_ports) {
 ? ? ? ? ? ? ? bus = pci_scan_bus(sys->busnr, &pcie_ops, sys);
 ? ? ? } else {
 ? ? ? ? ? ? ? bus = NULL;

arch/arm/mach-kirkwood/pcie.c kirkwood_pcie_scan_bus(int nr, struct pci_sys_data *sys) · line 228

Quoted 7 lines

arch/arm/mach-kirkwood/pcie.c +3 -1

@@ -175,18 +223,45 @@ kirkwood_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 
 ?static int __init kirkwood_pcie_map_irq(struct pci_dev *dev, u8 slot, u8 pin)
 ?{
- ? ? return IRQ_KIRKWOOD_PCIE;
+ ? ? struct pcie_port *pp = bus_to_port(dev->bus->number);
+
+ ? ? return pp->index ? IRQ_KIRKWOOD_PCIE1 : IRQ_KIRKWOOD_PCIE;

Maybe having pp->irq and returning that directly would be better.

arch/arm/mach-kirkwood/pcie.c kirkwood_pcie_scan_bus(int nr, struct pci_sys_data *sys) · line 230

Quoted 3 lines

arch/arm/mach-kirkwood/pcie.c +1 -1

 ?static struct hw_pci kirkwood_pci __initdata = {
- ? ? .nr_controllers = 1,
+ ? ? .nr_controllers = 2,

I think this should be initialized at run time to 2 only when running on a 6282.

Quoted 25 lines

arch/arm/mach-kirkwood/pcie.c +16 -1

 ? ? ? .swizzle ? ? ? ?= pci_std_swizzle,
 ? ? ? .setup ? ? ? ? ?= kirkwood_pcie_setup,
 ? ? ? .scan ? ? ? ? ? = kirkwood_pcie_scan_bus,
 ? ? ? .map_irq ? ? ? ?= kirkwood_pcie_map_irq,
 ?};
 
-void __init kirkwood_pcie_init(void)
+static void __init add_pcie_port(int index, unsigned long base)
 ?{
+ ? ? printk(KERN_INFO "Kirkwood PCIe port %d: ", index);
+
+ ? ? if (orion_pcie_link_up((void __iomem *)base)) {
+ ? ? ? ? ? ? struct pcie_port *pp = &pcie_port[num_pcie_ports++];
+
+ ? ? ? ? ? ? printk(KERN_INFO "link up\n");
+
+ ? ? ? ? ? ? pp->index = index;
+ ? ? ? ? ? ? pp->root_bus_nr = -1;
+ ? ? ? ? ? ? pp->base = (void __iomem *)base;
+ ? ? ? ? ? ? spin_lock_init(&pp->conf_lock);
+ ? ? ? ? ? ? memset(pp->res, 0, sizeof(pp->res));
+ ? ? } else {
+ ? ? ? ? ? ? printk(KERN_INFO "link down, ignoring\n");
+ ? ? }

arch/arm/mach-kirkwood/pcie.c kirkwood_pcie_scan_bus(int nr, struct pci_sys_data *sys) · line 258

Quoted 3 lines

arch/arm/mach-kirkwood/pcie.c +3 -0

+}
+
+void __init kirkwood_pcie_init(int init_port0, int init_port1)

And instead of having (int init_port0, int init_port1), this should be (int port) which is much less error prone as demonstrated by your own kirkwood_pcie_init(0, 1) usage. ?Those machines with both PCIe ports would simply have to call this twice.

Nicolas

Quoted 7 lines · Nicolas Pitre

Quoted 1 line · Saeed Bishara

+void __init kirkwood_pcie_init(int init_port0, int init_port1)

And instead of having (int init_port0, int init_port1), this should be (int port) which is much less error prone as demonstrated by your own kirkwood_pcie_init(0, 1) usage. ?Those machines with both PCIe ports would simply have to call this twice.

nico, this is different from other controller, we can't have multiple registrations as the pci_commont_init() can't be called multiple times.

This patch extens the kirkwood's PCIe support up to 2 controllers as in the 6282 devices.

arch/arm/mach-kirkwood/db88f6281-bp-setup.c +1 -2

index 39bdf4b..c428348 100644
@@ -74,9 +74,15 @@ static void __init db88f6281_init(void)
 
  static int __init db88f6281_pci_init(void)
  {
-	if (machine_is_db88f6281_bp())
-		kirkwood_pcie_init();
+	if (machine_is_db88f6281_bp()) {

Quoted 18 lines · Saeed Bishara

This patch extens the kirkwood's PCIe support up to 2 controllers as in the 6282 devices.

arch/arm/mach-kirkwood/db88f6281-bp-setup.c +2 -2

index 39bdf4b..c428348 100644
@@ -74,9 +74,15 @@ static void __init db88f6281_init(void)
 
 ?static int __init db88f6281_pci_init(void)
 ?{
- ? ? ? if (machine_is_db88f6281_bp())
- ? ? ? ? ? ? ? kirkwood_pcie_init();
+ ? ? ? if (machine_is_db88f6281_bp()) {
+ ? ? ? ? ? ? ? u32 dev, rev;
 

arch/arm/mach-kirkwood/db88f6281-bp-setup.c static void __init db88f6281_init(void) · line 87

Quoted 8 lines

arch/arm/mach-kirkwood/db88f6281-bp-setup.c +6 -0

+ ? ? ? ? ? ? ? kirkwood_pcie_id(&dev,&rev);
+ ? ? ? ? ? ? ? if (dev == MV88F6282_DEV_ID)
+ ? ? ? ? ? ? ? ? ? ? ? kirkwood_pcie_init(KW_PCIE1 | KW_PCIE0);
+ ? ? ? ? ? ? ? else
+ ? ? ? ? ? ? ? ? ? ? ? kirkwood_pcie_init(KW_PCIE0);
+ ? ? ? }
 ? ? ? ?return 0;
 

This is still not likely to be possible: I don't think a given board can have 2 different SoCs.

Quoted 6 lines

arch/arm/mach-kirkwood/db88f6281-bp-setup.c +1 -0

+#define CGC_PEX1 ? ? ? ? ? ? ? (1<< ?18)

?#define CGC_GE1 ? ? ? ? ? ? ? ? ? ? ? (1<< ?19) ?#define CGC_TDM ? ? ? ? ? ? ? ? ? ? ? (1<< ?20) ?#define CGC_RESERVED ? ? ? ? ?((1<< ?18) | (0x6<< ?21))

(1 << 18) is no longer reserved.

Except from that you still miss the mapping definition and phy initialization in common.c

See
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-June/017176.html
for details.

Quoted 8 lines · Saeed Bishara

(quoted diff) +0 -0

@@ -106,17 +108,21 @@ void __init kirkwood_setup_cpu_mbus(void)
 ? ? ? ? ? ? ? ? ? ? ?TARGET_PCIE, ATTR_PCIE_IO,
 KIRKWOOD_PCIE_IO_BUS_BASE);
 ? ? ? ?setup_cpu_win(1, KIRKWOOD_PCIE_MEM_PHYS_BASE,
 KIRKWOOD_PCIE_MEM_SIZE,
 ? ? ? ? ? ? ? ? ? ? ?TARGET_PCIE, ATTR_PCIE_MEM,
 KIRKWOOD_PCIE_MEM_BUS_BASE);

Review void __init kirkwood_setup_cpu_mbus(void) · line 123

Quoted 10 lines

(quoted diff) +4 -0

+ ? ? ? setup_cpu_win(2, KIRKWOOD_PCIE1_IO_PHYS_BASE,
 KIRKWOOD_PCIE1_IO_SIZE,
+ ? ? ? ? ? ? ? ? ? ? TARGET_PCIE, ATTR_PCIE1_IO,
 KIRKWOOD_PCIE1_IO_BUS_BASE);
+ ? ? ? setup_cpu_win(3, KIRKWOOD_PCIE1_MEM_PHYS_BASE,
 KIRKWOOD_PCIE1_MEM_SIZE,
+ ? ? ? ? ? ? ? ? ? ? TARGET_PCIE, ATTR_PCIE1_MEM,
 KIRKWOOD_PCIE1_MEM_BUS_BASE);
 
 

I'd have used an id, and incremented it at each setup_cpu_win() call. Also only doing the last 2 calls on 6282.

Quoted 7 lines

? ? ? ?/* Make sure those units are accessible */
- ? ? ? writel(curr | CGC_SATA0 | CGC_SATA1 | CGC_PEX0, CLOCK_GATING_CTRL);
+ ? ? ? writel(curr | CGC_SATA0 | CGC_SATA1 | CGC_PEX0 | CGC_PEX1, CLOCK_GATING_CTRL);

Why not (pseudo code):

int flags = curr | CGC_SATA0 | CGC_SATA1 | CGC_PEX0; if (6282) flags |= CGC_PEX1;
writel(flags, ...);

Quoted 5 lines

+ ? ? ? /* For PCIe 1: first shutdown the phy */
+ ? ? ? if (dev == MV88F6282_DEV_ID) {
+ ? ? ? ? ? ? ? if (!(kirkwood_clk_ctrl& ?CGC_PEX1)) {
+ ? ? ? ? ? ? ? ? ? ? ? writel(readl(PCIE1_LINK_CTRL) | 0x10, PCIE1_LINK_CTRL);

Quoted 10 lines

+ ? ? ? ? ? ? ? ? ? ? ? while (1)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (readl(PCIE1_STATUS)& ?0x1)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? ? ? ? ? writel(readl(PCIE1_LINK_CTRL)& ?~0x10, PCIE1_LINK_CTRL);
+ ? ? ? ? ? ? ? }
+ ? ? ? } else ?/* keep this bit set for devices that don't have PCIe1 */
+ ? ? ? ? ? ? ? kirkwood_clk_ctrl |= CGC_PEX1;

the else part is useless with the proposal above i guess.

Except from that, when using your patch I was unable to boot kernel anymore on 6281 boards.

Quoted 55 lines · Benjamin Zores

Quoted 8 lines · Saeed Bishara

(quoted diff) +1 -0

@@ -106,17 +108,21 @@ void __init kirkwood_setup_cpu_mbus(void)
                      TARGET_PCIE, ATTR_PCIE_IO,
 KIRKWOOD_PCIE_IO_BUS_BASE);
        setup_cpu_win(1, KIRKWOOD_PCIE_MEM_PHYS_BASE,
 KIRKWOOD_PCIE_MEM_SIZE,
                      TARGET_PCIE, ATTR_PCIE_MEM,
 KIRKWOOD_PCIE_MEM_BUS_BASE);
+       setup_cpu_win(2, KIRKWOOD_PCIE1_IO_PHYS_BASE,

Review void __init kirkwood_setup_cpu_mbus(void) · line 124

Quoted 10 lines

(quoted diff) +3 -0

 KIRKWOOD_PCIE1_IO_SIZE,
+                     TARGET_PCIE, ATTR_PCIE1_IO,
 KIRKWOOD_PCIE1_IO_BUS_BASE);
+       setup_cpu_win(3, KIRKWOOD_PCIE1_MEM_PHYS_BASE,
 KIRKWOOD_PCIE1_MEM_SIZE,
+                     TARGET_PCIE, ATTR_PCIE1_MEM,
 KIRKWOOD_PCIE1_MEM_BUS_BASE);
 
 
 

I'd have used an id, and incremented it at each setup_cpu_win() call. Also only doing the last 2 calls on 6282.

Review void __init kirkwood_setup_cpu_mbus(void) · line 128

Quoted 8 lines

(quoted diff) +1 -1

        /* Make sure those units are accessible */
-       writel(curr | CGC_SATA0 | CGC_SATA1 | CGC_PEX0,
 CLOCK_GATING_CTRL);
+       writel(curr | CGC_SATA0 | CGC_SATA1 | CGC_PEX0 | CGC_PEX1,
 CLOCK_GATING_CTRL);

Why not (pseudo code):

int flags = curr | CGC_SATA0 | CGC_SATA1 | CGC_PEX0; if (6282) flags |= CGC_PEX1;
writel(flags, ...);

Quoted 16 lines

arch/arm/mach-kirkwood/common.c +4 -0

+       /* For PCIe 1: first shutdown the phy */
+       if (dev == MV88F6282_DEV_ID) {
+               if (!(kirkwood_clk_ctrl&    CGC_PEX1)) {
+                       writel(readl(PCIE1_LINK_CTRL) | 0x10,

PCIE1_LINK_CTRL);

arch/arm/mach-kirkwood/common.c +4 -0

+                       while (1)
+                               if (readl(PCIE1_STATUS)&    0x1)
+                                       break;
+                       writel(readl(PCIE1_LINK_CTRL)&    ~0x10,

PCIE1_LINK_CTRL);

arch/arm/mach-kirkwood/common.c +3 -0

+               }
+       } else  /* keep this bit set for devices that don't have PCIe1 */
+               kirkwood_clk_ctrl |= CGC_PEX1;

Quoted 5 lines

the else part is useless with the proposal above i guess.

Except from that, when using your patch I was unable to boot kernel anymore on 6281 boards.

can you try the patch I send regarding nand mpps?

Quoted 74 lines · saeed bishara

Quoted 69 lines · On Tue, Jun 8, 2010 at 2:43 PM, Benjamin Zores…

Quoted 9 lines · Saeed Bishara

(quoted diff) +1 -0

@@ -106,17 +108,21 @@ void __init kirkwood_setup_cpu_mbus(void)
 ? ? ? ? ? ? ? ? ? ? ?TARGET_PCIE, ATTR_PCIE_IO,
 KIRKWOOD_PCIE_IO_BUS_BASE);
 ? ? ? ?setup_cpu_win(1, KIRKWOOD_PCIE_MEM_PHYS_BASE,
 KIRKWOOD_PCIE_MEM_SIZE,
 ? ? ? ? ? ? ? ? ? ? ?TARGET_PCIE, ATTR_PCIE_MEM,
 KIRKWOOD_PCIE_MEM_BUS_BASE);
+ ? ? ? setup_cpu_win(2, KIRKWOOD_PCIE1_IO_PHYS_BASE,

Review void __init kirkwood_setup_cpu_mbus(void) · line 124

Quoted 10 lines

(quoted diff) +3 -0

 KIRKWOOD_PCIE1_IO_SIZE,
+ ? ? ? ? ? ? ? ? ? ? TARGET_PCIE, ATTR_PCIE1_IO,
 KIRKWOOD_PCIE1_IO_BUS_BASE);
+ ? ? ? setup_cpu_win(3, KIRKWOOD_PCIE1_MEM_PHYS_BASE,
 KIRKWOOD_PCIE1_MEM_SIZE,
+ ? ? ? ? ? ? ? ? ? ? TARGET_PCIE, ATTR_PCIE1_MEM,
 KIRKWOOD_PCIE1_MEM_BUS_BASE);
 
 
 

I'd have used an id, and incremented it at each setup_cpu_win() call. Also only doing the last 2 calls on 6282.

Quoted 9 lines

? ? ? ?/* Make sure those units are accessible */
- ? ? ? writel(curr | CGC_SATA0 | CGC_SATA1 | CGC_PEX0, CLOCK_GATING_CTRL);
+ ? ? ? writel(curr | CGC_SATA0 | CGC_SATA1 | CGC_PEX0 | CGC_PEX1, CLOCK_GATING_CTRL);

Why not (pseudo code):

int flags = curr | CGC_SATA0 | CGC_SATA1 | CGC_PEX0; if (6282) flags |= CGC_PEX1;
writel(flags, ...);

Quoted 18 lines

+ ? ? ? /* For PCIe 1: first shutdown the phy */
+ ? ? ? if (dev == MV88F6282_DEV_ID) {
+ ? ? ? ? ? ? ? if (!(kirkwood_clk_ctrl& ? ?CGC_PEX1)) {
+ ? ? ? ? ? ? ? ? ? ? ? writel(readl(PCIE1_LINK_CTRL) | 0x10, PCIE1_LINK_CTRL);
+ ? ? ? ? ? ? ? ? ? ? ? while (1)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (readl(PCIE1_STATUS)& ? ?0x1)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? ? ? ? ? writel(readl(PCIE1_LINK_CTRL)& ? ?~0x10, PCIE1_LINK_CTRL);
+ ? ? ? ? ? ? ? }
+ ? ? ? } else ?/* keep this bit set for devices that don't have PCIe1 */
+ ? ? ? ? ? ? ? kirkwood_clk_ctrl |= CGC_PEX1;

the else part is useless with the proposal above i guess.

Except from that, when using your patch I was unable to boot kernel anymore
on 6281 boards.

Quoted 2 lines

can you try the patch I send regarding nand mpps?

Unfortunately no, I'm running on OpenRD currently.

Ben