Fixes for CNS3xxx platform

Hi,

1. Fix debug UART.

The debug UART ("earlyprintk") has never worked on this platform. This should be hardly a surprise, given that the Cavium Econa CNS3xxx CPU has 8250-compatible UARTs (instead of AMBA ports) and the addresses (all the debug UART code, in fact) seem to be simply copied unchanged from a random other platform :-)

The patch makes it work.

2. Fix logical PCIe topology.

The CNS3xxx code used a hack which made the kernel "think" that it's not necessary to go through the root PCIe bridge in order to reach other PCI(e) devices. This was the source of regression described in
https://lkml.org/lkml/2014/2/28/150 (#2). Broken since v3.12-rc1. Actually, going through the root bridge is necessary and the patch fixes this problem.

3. Fix PCIe read size limit.
This fixes a (probably rare) PCI setup bug.

All of this stuff has been tested on Gateworks Laguna platform, masqueraded as the CNS3xxx devel board, with v3.17-rc5. I guess this is v3.17 material, though it doesn't make much sense for "stable" branches.

Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

UARTs on CNS3xxx are 8250-compatible, not AMBA. The base address for UART0 is 0x78000000 (physical) and 0xfb002000 (virtual).

index b11ad54..6a5b496 100644
@@ -147,7 +147,7 @@ choice
 	config DEBUG_CNS3XXX
 		bool "Kernel Kernel low-level debugging on Cavium Networks CNS3xxx"
 		depends on ARCH_CNS3XXX
-		select DEBUG_UART_PL01X
+		select DEBUG_UART_8250
 		help
 		  Say Y here if you want the debug print routines to direct
                   their output to the CNS3xxx UART0.

@@ -1068,7 +1068,7 @@ config DEBUG_UART_PHYS
 	default 0x02530c00 if DEBUG_KEYSTONE_UART0
 	default 0x02531000 if DEBUG_KEYSTONE_UART1
 	default 0x03010fe0 if ARCH_RPC
-	default 0x10009000 if DEBUG_REALVIEW_STD_PORT || DEBUG_CNS3XXX || \
+	default 0x10009000 if DEBUG_REALVIEW_STD_PORT || \
 				DEBUG_VEXPRESS_UART0_CA9
 	default 0x1010c000 if DEBUG_REALVIEW_PB1176_PORT
 	default 0x10124000 if DEBUG_RK3X_UART0

@@ -1094,6 +1094,7 @@ config DEBUG_UART_PHYS
 	default 0x50008000 if DEBUG_S3C24XX_UART && (DEBUG_S3C_UART2 || \
 				DEBUG_S3C2410_UART2)
 	default 0x7c0003f8 if FOOTBRIDGE
+	default 0x78000000 if DEBUG_CNS3XXX
 	default 0x80070000 if DEBUG_IMX23_UART
 	default 0x80074000 if DEBUG_IMX28_UART
 	default 0x80230000 if DEBUG_PICOXCELL_UART

@@ -1133,7 +1134,6 @@ config DEBUG_UART_VIRT
 	default 0xe0010fe0 if ARCH_RPC
 	default 0xe1000000 if DEBUG_MSM_UART
 	default 0xf0000be0 if ARCH_EBSA110
-	default 0xf0009000 if DEBUG_CNS3XXX
 	default 0xf01fb000 if DEBUG_NOMADIK_UART
 	default 0xf0201000 if DEBUG_BCM2835
 	default 0xf1000300 if DEBUG_BCM_5301X

@@ -1155,6 +1155,7 @@ config DEBUG_UART_VIRT
 	default 0xf8009000 if DEBUG_VEXPRESS_UART0_CA9
 	default 0xf8090000 if DEBUG_VEXPRESS_UART0_RS1
 	default 0xfa71e000 if DEBUG_QCOM_UARTDM
+	default 0xfb002000 if DEBUG_CNS3XXX
 	default 0xfb009000 if DEBUG_REALVIEW_STD_PORT
 	default 0xfb10c000 if DEBUG_REALVIEW_PB1176_PORT
 	default 0xfd000000 if ARCH_SPEAR3XX || ARCH_SPEAR6XX

Without this patch, each root port and the device connected directly to it seem to be located on a shared (virtual) bus #0. It creates problems with enabling devices (the PCI code doesn't know that the root bridge must be enabled in order to access other devices).
The PCIe topology shown by lspci doesn't reflect reality, e.g.:

0000:00:00.0 PCI bridge: Cavium Networks Device 3400
0000:00:01.0 PCI bridge: Texas Instruments XIO2001 PCI Express-to-PCI Bridge 0000:02:...
0001:00:00.0 PCI bridge: Cavium Networks Device 3400 (for the second lane/bus)

-+-[0001:00]---00.0-[01]--
\-[0000:00]-+-00.0-[01]--

             | ^^^^ root bridge
             \-01.0-[02]----...
               ^^^^ first external device

With this patch, the first external PCIe device is connected to bus #1 (behind the root bridge).

-+-[0001:00]---00.0-[01]--
\-[0000:00]---00.0-[01-02]----------00.0-[02]----...
^^^^ root bridge ^^^^ first external device

index 413134c..4ddb974 100644
@@ -60,11 +60,10 @@ static void __iomem *cns3xxx_pci_cfg_base(struct pci_bus *bus,
 	struct cns3xxx_pcie *cnspci = pbus_to_cnspci(bus);
 	int busno = bus->number;
 	int slot = PCI_SLOT(devfn);
-	int offset;
 	void __iomem *base;
 
 	/* If there is no link, just show the CNS PCI bridge. */
-	if (!cnspci->linked && (busno > 0 || slot > 0))
+	if (!cnspci->linked && busno > 0)
 		return NULL;
 
 	/*

@@ -72,22 +71,21 @@ static void __iomem *cns3xxx_pci_cfg_base(struct pci_bus *bus,
 	 * we still want to access it. For this to work, we must place
 	 * the first device on the same bus as the CNS PCI bridge.
 	 */
-	if (busno == 0) { /* directly connected PCIe bus */
-		switch (slot) {
-		case 0: /* host bridge device, function 0 only */
+	if (busno == 0) { /* internal PCIe bus, host bridge device */
+		if (devfn == 0) /* device# and function# are ignored by hw */
 			base = cnspci->host_regs;
-			break;
-		case 1: /* directly connected device */
+		else
+			return NULL; /* no such device */
+
+	} else if (busno == 1) { /* directly connected PCIe device */
+		if (slot == 0) /* device# is ignored by hw */
 			base = cnspci->cfg0_regs;
-			break;
-		default:
+		else
 			return NULL; /* no such device */
-		}
 	} else /* remote PCI bus */
-		base = cnspci->cfg1_regs;
+		base = cnspci->cfg1_regs + ((busno & 0xf) << 20);
 
-	offset = ((busno & 0xf) << 20) | (devfn << 12) | (where & 0xffc);
-	return base + offset;
+	return base + (where & 0xffc) + (devfn << 12);
 }
 
 static int cns3xxx_pci_read_config(struct pci_bus *bus, unsigned int devfn,

@@ -167,7 +165,7 @@ static struct pci_ops cns3xxx_pcie_ops = {
 static int cns3xxx_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 {
 	struct cns3xxx_pcie *cnspci = pdev_to_cnspci(dev);
-	int irq = cnspci->irqs[slot];
+	int irq = cnspci->irqs[!!dev->bus->number];
 
 	pr_info("PCIe map irq: %04d:%02x:%02x.%02x slot %d, pin %d, irq: %d\n",
 		pci_domain_nr(dev->bus), dev->bus->number, PCI_SLOT(dev->devfn),

@@ -297,7 +295,8 @@ static void __init cns3xxx_pcie_hw_init(struct cns3xxx_pcie *cnspci)
 		return;
 
 	/* Set Device Max_Read_Request_Size to 128 byte */
-	devfn = PCI_DEVFN(1, 0);
+	bus.number = 1; /* directly connected PCIe device */
+	devfn = PCI_DEVFN(0, 0);
 	pos = pci_bus_find_capability(&bus, devfn, PCI_CAP_ID_EXP);
 	pci_bus_read_config_word(&bus, devfn, pos + PCI_EXP_DEVCTL, &dc);
 	dc &= ~(0x3 << 12);	/* Clear Device Control Register [14:12] */

Max_Read_Request_Size is 3 bits wide, not 2 bits.
Also fix the message.

index 4ddb974..85e2135 100644
@@ -299,12 +299,15 @@ static void __init cns3xxx_pcie_hw_init(struct cns3xxx_pcie *cnspci)
 	devfn = PCI_DEVFN(0, 0);
 	pos = pci_bus_find_capability(&bus, devfn, PCI_CAP_ID_EXP);
 	pci_bus_read_config_word(&bus, devfn, pos + PCI_EXP_DEVCTL, &dc);
-	dc &= ~(0x3 << 12);	/* Clear Device Control Register [14:12] */
-	pci_bus_write_config_word(&bus, devfn, pos + PCI_EXP_DEVCTL, dc);
-	pci_bus_read_config_word(&bus, devfn, pos + PCI_EXP_DEVCTL, &dc);
-	if (!(dc & (0x3 << 12)))
-		pr_info("PCIe: Set Device Max_Read_Request_Size to 128 byte\n");
-
+	if (dc & PCI_EXP_DEVCTL_READRQ) {
+		dc &= ~PCI_EXP_DEVCTL_READRQ;
+		pci_bus_write_config_word(&bus, devfn, pos + PCI_EXP_DEVCTL, dc);
+		pci_bus_read_config_word(&bus, devfn, pos + PCI_EXP_DEVCTL, &dc);
+		if (dc & PCI_EXP_DEVCTL_READRQ)
+			pr_warn("PCIe: Unable to set device Max_Read_Request_Size\n");
+		else
+			pr_info("PCIe: Max_Read_Request_Size set to 128 bytes\n");
+	}
 	/* Disable PCIe0 Interrupt Mask INTA to INTD */
 	__raw_writel(~0x3FFF, MISC_PCIE_INT_MASK(port));
 }

Thanks a lot for your bug fixes!

Sorry for the delay, but as both Olof and I were attending Linaro Connect last week, we had built up a large backlog to look at.

I ended up putting your patches into the next/fixes-non-critical branch, which we will submit for 3.18, not 3.17, because by now it is very late in the cycle and we mainly want to fix regressions against 3.16. Let us know if there is a strong reason to have them in 3.17 after all.

On a related note, Anton Vorontsov mentioned that he can no longer maintain cns3xxx upstream, would you mind becoming the official contact? If that's ok with you, please send an update for the MAINTAINERS file listing yourself and any co-maintainers (Felix?) as well as the level of support you can give ("Maintained" or "Odd Fixes" I guess).

Arnd

No problem. I mentioned 3.17 because it was at a bit earlier stage then.

Will do.

BTW, as Stephen and Russell pointed out, I've misplaced the UART CONFIG_* address. I will send a corrected patch shortly.

Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Sent. Would you like an incremental patch instead?

Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

UARTs on CNS3xxx are 8250-compatible, not AMBA. The base address for UART0 is 0x78000000 (physical) and 0xfb002000 (virtual).

index b11ad54..6a5b496 100644
@@ -147,7 +147,7 @@ choice
 	config DEBUG_CNS3XXX
 		bool "Kernel Kernel low-level debugging on Cavium Networks CNS3xxx"
 		depends on ARCH_CNS3XXX
-		select DEBUG_UART_PL01X
+		select DEBUG_UART_8250
 		help
 		  Say Y here if you want the debug print routines to direct
                   their output to the CNS3xxx UART0.

@@ -1068,7 +1068,7 @@ config DEBUG_UART_PHYS
 	default 0x02530c00 if DEBUG_KEYSTONE_UART0
 	default 0x02531000 if DEBUG_KEYSTONE_UART1
 	default 0x03010fe0 if ARCH_RPC
-	default 0x10009000 if DEBUG_REALVIEW_STD_PORT || DEBUG_CNS3XXX || \
+	default 0x10009000 if DEBUG_REALVIEW_STD_PORT || \
 				DEBUG_VEXPRESS_UART0_CA9
 	default 0x1010c000 if DEBUG_REALVIEW_PB1176_PORT
 	default 0x10124000 if DEBUG_RK3X_UART0

@@ -1094,6 +1094,7 @@ config DEBUG_UART_PHYS
 	default 0x50008000 if DEBUG_S3C24XX_UART && (DEBUG_S3C_UART2 || \
 				DEBUG_S3C2410_UART2)
+	default 0x78000000 if DEBUG_CNS3XXX
 	default 0x7c0003f8 if FOOTBRIDGE
 	default 0x80070000 if DEBUG_IMX23_UART
 	default 0x80074000 if DEBUG_IMX28_UART
 	default 0x80230000 if DEBUG_PICOXCELL_UART

@@ -1133,7 +1134,6 @@ config DEBUG_UART_VIRT
 	default 0xe0010fe0 if ARCH_RPC
 	default 0xe1000000 if DEBUG_MSM_UART
 	default 0xf0000be0 if ARCH_EBSA110
-	default 0xf0009000 if DEBUG_CNS3XXX
 	default 0xf01fb000 if DEBUG_NOMADIK_UART
 	default 0xf0201000 if DEBUG_BCM2835
 	default 0xf1000300 if DEBUG_BCM_5301X

@@ -1155,6 +1155,7 @@ config DEBUG_UART_VIRT
 	default 0xf8009000 if DEBUG_VEXPRESS_UART0_CA9
 	default 0xf8090000 if DEBUG_VEXPRESS_UART0_RS1
 	default 0xfa71e000 if DEBUG_QCOM_UARTDM
+	default 0xfb002000 if DEBUG_CNS3XXX
 	default 0xfb009000 if DEBUG_REALVIEW_STD_PORT
 	default 0xfb10c000 if DEBUG_REALVIEW_PB1176_PORT
 	default 0xfd000000 if ARCH_SPEAR3XX || ARCH_SPEAR6XX

I've applied it on next/soc now, as this seems to be an important fix but it also interacts with other stuff we have in next/soc already and I want to avoid conflicts.

One question though, just to make sure it's correct:

Interesting indeed. I thought it's a regular page mapping.

I think a supersection is a 16 MB mapping and the early code in __create_page_tables() uses 1 MB section instead (probably for simplicity). This still means the physical address actually used is 0x78002000, not exactly per specs.

#define CNS3XXX_UART0_BASE                      0x78000000      /* UART 0 */
#define CNS3XXX_UART0_BASE_VIRT                 0xFB002000
#define CNS3XXX_UART1_BASE                      0x78400000      /* UART 1 */
#define CNS3XXX_UART2_BASE                      0x78800000      /* UART 2 */

I wonder if the early ll debug code (with the initial section mapping) produces any output at all (by default). The first thing I see on the serial console is:

Uncompressing Linux... done, booting the kernel.
Booting Linux on physical CPU 0x900
Linux version 3.17.0-rc7+ xxx
CPU: ARMv6-compatible processor [410fb024] revision 4 (ARMv7), cr=00c5787d

(this ARMv7 thing is a bit misleading, though).
The "Booting..." and "Linux version" lines are printed with the initial mapping, before setup_arch()->debug_ll_io_init() updates it, right?

FWIW I have changed CNS3XXX_UART0_BASE to 0x78002000 and it works the same. The registers are probably "aliased" all over the place.

I guess we should eventually change the mappings so the UART is at a section boundary.
--
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Right, my mistake: I meant section mapping not supersection.

There are actually three stages: the first one uses the physical address before we enable the MMU, the second one uses the mapping created in __create_page_tables, and the third one comes from either debug_ll_io_init or machine_desc->map_io. In case of CNS3420VB, the third mapping is created explicitly in cns3420_map_io using a 4K mapping from CNS3XXX_UART0_BASE to CNS3XXX_UART0_BASE_VIRT.

Interesting, that would certainly explain why it just works.

Yes. FWIW the current policy for new platforms is to have as few static mappings as possible, and we have been able to basically eliminate the need for them in common code, but changing the platform code does involve some nontrivial work. It would come naturally if you want to migrate cns3xxx to DT-only though.

Arnd