mvebu PCI fixes and cleanups

This small series contains a number of fixes and cleanups to the mvebu PCI host code.

The first patch makes the PCIe host controller more conformant with the requirements of the PCI specification, which requires where a device is present, unused PCI configuration space returns zero. This patch ensures that is the case.

The second patch fixes the very broken idea that it's somehow legal to read-modify-write the PCI configuration space using 32-bit accessors, even when accessing a smaller register. There are _multiple_ registers in PCI space where bits are defined to be "RW1C" - which means "read, write 1 to clear". Registers include the PCI status register, and all PCIe status registers. Hardware which does not support generating 16-bit and 8-bit configuration cycles is essentially broken and non-conformant. Luckily, that is not true of Armada hardware. This patch fixes it. Tested on Armada 388.

These two patches should be considered as fixes. I don't see a need for them to be backported to stable kernels as there is no serious breakage resulting from them with the driver in its current state. The remainder are cleanups.

Patch 3 gets rid of the open coded "of_get_available_child_count()" implementation.

Patch 4 uses for_each_available_child_of_node(), rather than for_each_child_of_node() and checking whether of_device_is_available() returns true. for_each_available_child_of_node() is meant for this.

Patch 5 makes the warning message for the lack of the "marvell,pcie-port" property more useful, by printing the DT path of the offending node rather than the wooly "PCIe DT node".

Lastly, patch 6 converts all the "PCIe%d.%d" and "pcie%d.%d" with the port name string, which is a pre-prepared string of that format. This means we end up with a consistent name for the port being used everywhere which is good for user experience.

 drivers/pci/host/pci-mvebu.c | 87 ++++++++++++++++++++++++--------------------
 1 file changed, 47 insertions(+), 40 deletions(-)

FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

PCI requires reads to reserved or unimplemented configuration space to return zero and complete normally. However, the root port software implementation was returning 0xfffffff and PCIBIOS_BAD_REGISTER_NUMBER. Fix this.

---
 drivers/pci/host/pci-mvebu.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

index 67ec5e1c99db..b6a096bc9422 100644
@@ -515,8 +515,13 @@ static int mvebu_sw_pci_bridge_read(struct mvebu_pcie_port *port,
 		break;
 
 	default:
-		*value = 0xffffffff;
-		return PCIBIOS_BAD_REGISTER_NUMBER;
+		/*
+		 * PCI defines configuration read accesses to reserved or
+		 * unimplemented registers to read as zero and complete
+		 * normally.
+		 */
+		*value = 0;
+		return PCIBIOS_SUCCESSFUL;
 	}
 
 	if (size == 2)

2.1.0

The idea that you can arbitarily read 32-bits from PCI configuration space, modify a sub-field (like the command register) and write it back without consequence is deeply flawed.

Status registers (such as the status register, PCIe device status register, etc) contain status bits which are read, write-one-to-clear.

What this means is that reading 32-bits from the command register, modifying the command register, and then writing it back has the effect of clearing any status bits that were indicating at that time. Same for the PCIe device control register clearing bits in the PCIe device status register.

Since the Armada chips support byte, 16-bit and 32-bit accesses to the registers (unless otherwise stated) and the PCI configuration data register does not specify otherwise, it seems logical that the chip can indeed generate the proper configuration access cycles down to byte level.

Testing with an ASM1062 PCIe to SATA mini-PCIe card on Armada 388. PCIe capability at 0x80, DevCtl at 0x88, DevSta at 0x8a.

Before:
/# setpci -s 1:0.0 0x88.l - DevSta: CorrErr+
00012810
/# setpci -s 1:0.0 0x88.w=0x2810 - Write DevCtl only
/# setpci -s 1:0.0 0x88.l - CorrErr cleared - FAIL
00002810

After:
/# setpci -s 1:0.0 0x88.l - DevSta: CorrErr+
00012810
/# setpci -s 1:0.0 0x88.w=0x2810 - check DevCtl only write
/# setpci -s 1:0.0 0x88.l - CorErr remains set
00012810
/# setpci -s 1:0.0 0x88.w=0x281f - check DevCtl write works
/# setpci -s 1:0.0 0x88.l - devctl field updated
0001281f
/# setpci -s 1:0.0 0x8a.w=0xffff - clear DevSta
/# setpci -s 1:0.0 0x88.l - CorrErr now cleared
0000281f
/# setpci -s 1:0.0 0x88.w=0x2810 - restore DevCtl
/# setpci -s 1:0.0 0x88.l - check
00002810

---
 drivers/pci/host/pci-mvebu.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

index b6a096bc9422..0d9f3eae4315 100644
@@ -254,15 +254,22 @@ static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port,
 				 struct pci_bus *bus,
 				 u32 devfn, int where, int size, u32 *val)
 {
+	void __iomem *conf_data = port->base + PCIE_CONF_DATA_OFF;
+
 	mvebu_writel(port, PCIE_CONF_ADDR(bus->number, devfn, where),
 		     PCIE_CONF_ADDR_OFF);
 
-	*val = mvebu_readl(port, PCIE_CONF_DATA_OFF);
-
-	if (size == 1)
-		*val = (*val >> (8 * (where & 3))) & 0xff;
-	else if (size == 2)
-		*val = (*val >> (8 * (where & 3))) & 0xffff;
+	switch (size) {
+	case 1:
+		*val = readb_relaxed(conf_data + (where & 3));
+		break;
+	case 2:
+		*val = readw_relaxed(conf_data + (where & 2));
+		break;
+	case 4:
+		*val = readl_relaxed(conf_data);
+		break;
+	}
 
 	return PCIBIOS_SUCCESSFUL;
 }

@@ -271,22 +278,24 @@ static int mvebu_pcie_hw_wr_conf(struct mvebu_pcie_port *port,
 				 struct pci_bus *bus,
 				 u32 devfn, int where, int size, u32 val)
 {
-	u32 _val, shift = 8 * (where & 3);
+	void __iomem *conf_data = port->base + PCIE_CONF_DATA_OFF;
 
 	mvebu_writel(port, PCIE_CONF_ADDR(bus->number, devfn, where),
 		     PCIE_CONF_ADDR_OFF);
-	_val = mvebu_readl(port, PCIE_CONF_DATA_OFF);
 
-	if (size == 4)
-		_val = val;
-	else if (size == 2)
-		_val = (_val & ~(0xffff << shift)) | ((val & 0xffff) << shift);
-	else if (size == 1)
-		_val = (_val & ~(0xff << shift)) | ((val & 0xff) << shift);
-	else
+	switch (size) {
+	case 1:
+		writeb(val, conf_data + (where & 3));
+		break;
+	case 2:
+		writew(val, conf_data + (where & 2));
+		break;
+	case 4:
+		writel(val, conf_data);
+		break;
+	default:
 		return PCIBIOS_BAD_REGISTER_NUMBER;
-
-	mvebu_writel(port, _val, PCIE_CONF_DATA_OFF);
+	}
 
 	return PCIBIOS_SUCCESSFUL;
 }

2.1.0

The Armada 388 is a pretty new SoC. It would be good to test this on a much older device as well, e.g. Kirkwood. I will add it to my TODO list, but it anybody else gets there first, please let me know.

Andrew

Hi Russell

Can you give me some hints how to test this in my Kirkwood board.

root at dir665:~# lspci -nvvvv
00:01.0 0604: 11ab:6281 (rev 02) (prog-if 00 [Normal decode])
Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr+ Stepping- SERR+ FastB2B- DisINTx-
Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
I/O behind bridge: 00010000-00010fff
Memory behind bridge: e0000000-e00fffff
Prefetchable memory behind bridge: 00000000-000fffff
Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B-
PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-

01:00.0 0200: 11ab:2a40
Subsystem: 11ab:2a40
Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr+ Stepping- SERR+ FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Interrupt: pin A routed to IRQ 83
Region 0: Memory at e0000000 (64-bit, non-prefetchable) [disabled] [size=64K]
Region 2: Memory at e0010000 (64-bit, non-prefetchable) [disabled] [size=64K]
Capabilities: [40] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=1 PME-
Capabilities: [5c] MSI: Enable- Count=1/1 Maskable- 64bit+
Address: 0000000000000000 Data: 0000
Capabilities: [e0] Express (v1) Legacy Endpoint, MSI 00
DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s unlimited, L1 unlimited
ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset-
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
MaxPayload 128 bytes, MaxReadReq 512 bytes
DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s unlimited, L1 unlimited
ClockPM+ Surprise- LLActRep- BwNot-
LnkCtl: ASPM Disabled; RCB 128 bytes Disabled- CommClk-
ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
Capabilities: [100 v1] Advanced Error Reporting
UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
AERCap: First Error Pointer: 1f, GenCap- CGenEn- ChkCap- ChkEn-
Capabilities: [130 v1] Device Serial Number 00-00-00-00-00-00-00-00

root at dir665:~# setpci -s 1:0.0 0x88.l
00000000

Nothing there, so your test does not work directly.

I tried

root at dir665:~# setpci -s 1:0.0 0xe8.l
00102000
root at dir665:~# setpci -s 1:0.0 0xe8.w=0x2000

root@dir665:~# setpci -s 1:0.0 0xe8.l
00102000

but that is not producing the FAIL you had.

Thanks
Andrew

start of the PCIe capability.

which, here, starts at 0xe0. So...

This register is at 0xe8, and:

is at 0xea.

As is expected, because PCI configuration addresses between 0x40..0xff are freely assignable by the vendor to place whatever they want in that space - and the capabilities form a linked list.

The only bit you have set in your DevSta register is:

#define PCI_EXP_DEVSTA_AUXPD 0x0010 /* AUX Power Detected */

which is not a RW1C bit.

I don't know why I get the CorrErr bit set here, but it being set is very useful to test for correct behaviour. I don't yet know how to cause PCIe errors, I just know that I end up with that bit set each time I reboot the board I have here.

FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

So i cannot actually test this on my hardware :-(

I guess the best i can do is apply the patch, load the wifi driver, and ensure i can still join a network.

Andrew

Rather than open-coding of_get_available_child_count(), use the provided helper instead.

---
 drivers/pci/host/pci-mvebu.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

index 0d9f3eae4315..0ed14f477bf8 100644
@@ -933,7 +933,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 	struct mvebu_pcie *pcie;
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *child;
-	int i, ret;
+	int num, i, ret;
 
 	pcie = devm_kzalloc(&pdev->dev, sizeof(struct mvebu_pcie),
 			    GFP_KERNEL);

@@ -969,14 +969,9 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	i = 0;
-	for_each_child_of_node(pdev->dev.of_node, child) {
-		if (!of_device_is_available(child))
-			continue;
-		i++;
-	}
+	num = of_get_available_child_count(pdev->dev.of_node);
 
-	pcie->ports = devm_kzalloc(&pdev->dev, i *
+	pcie->ports = devm_kzalloc(&pdev->dev, num *
 				   sizeof(struct mvebu_pcie_port),
 				   GFP_KERNEL);
 	if (!pcie->ports)

2.1.0

Rather than using for_each_child_of_node() and testing each child's availability, use the for_each_available_child_of_node() helper instead.

---
 drivers/pci/host/pci-mvebu.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index 0ed14f477bf8..d331d2664580 100644
@@ -978,13 +978,10 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	i = 0;
-	for_each_child_of_node(pdev->dev.of_node, child) {
+	for_each_available_child_of_node(pdev->dev.of_node, child) {
 		struct mvebu_pcie_port *port = &pcie->ports[i];
 		enum of_gpio_flags flags;
 
-		if (!of_device_is_available(child))
-			continue;
-
 		port->pcie = pcie;
 
 		if (of_property_read_u32(child, "marvell,pcie-port",

2.1.0

If we have a missing required property, report the full node name rather than a vague "PCIe DT node" statement. This allows the exact node in error to be identified immediately.

---
 drivers/pci/host/pci-mvebu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index d331d2664580..8c715444b722 100644
@@ -987,7 +987,8 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 		if (of_property_read_u32(child, "marvell,pcie-port",
 					 &port->port)) {
 			dev_warn(&pdev->dev,
-				 "ignoring PCIe DT node, missing pcie-port property\n");
+				 "ignoring %s, missing pcie-port property\n",
+				 of_node_full_name(child));
 			continue;
 		}
 

2.1.0

Use the port->name string which we previously formatted when referring to the name of a port, rather than manually creating the port name each time.

---
 drivers/pci/host/pci-mvebu.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

index 8c715444b722..19144ed7bdad 100644
@@ -1006,8 +1006,8 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 		ret = mvebu_get_tgt_attr(np, port->devfn, IORESOURCE_MEM,
 					 &port->mem_target, &port->mem_attr);
 		if (ret < 0) {
-			dev_err(&pdev->dev, "PCIe%d.%d: cannot get tgt/attr for mem window\n",
-				port->port, port->lane);
+			dev_err(&pdev->dev, "%s: cannot get tgt/attr for mem window\n",
+				port->name);
 			continue;
 		}
 

@@ -1025,8 +1025,8 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 			u32 reset_udelay = 20000;
 
 			port->reset_active_low = flags & OF_GPIO_ACTIVE_LOW;
-			port->reset_name = kasprintf(GFP_KERNEL,
-				     "pcie%d.%d-reset", port->port, port->lane);
+			port->reset_name = kasprintf(GFP_KERNEL, "%s-reset",
+						     port->name);
 			of_property_read_u32(child, "reset-delay-us",
 					     &reset_udelay);
 

@@ -1045,8 +1045,8 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 
 		port->clk = of_clk_get_by_name(child, NULL);
 		if (IS_ERR(port->clk)) {
-			dev_err(&pdev->dev, "PCIe%d.%d: cannot get clock\n",
-			       port->port, port->lane);
+			dev_err(&pdev->dev, "%s: cannot get clock\n",
+				port->name);
 			continue;
 		}
 

@@ -1056,8 +1056,8 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 
 		port->base = mvebu_pcie_map_registers(pdev, child, port);
 		if (IS_ERR(port->base)) {
-			dev_err(&pdev->dev, "PCIe%d.%d: cannot map registers\n",
-				port->port, port->lane);
+			dev_err(&pdev->dev, "%s: cannot map registers\n",
+				port->name);
 			port->base = NULL;
 			clk_disable_unprepare(port->clk);
 			continue;

2.1.0

Hi Russell

I've tested this on my Kirkwood based DIR665, which has a Marvell TopDog wifi card on PCIe. I can join my wifi network, so no obvious regression.

Thanks
Andrew

Russell,

I've tested this series on my Armada XP GP board, with 3 PCIe cards connected (one Intel e1000e NIC, one generic AHCI SATA card, and one Marvell SATA card), and I saw no obvious regressions. The PCIe devices are still properly enumerated, and they seem to work fine. There is no change in the 'lspci -vv' output.

Moreover, I've reviewed the different patches, and they look alright to me.

Bjorn, will you propagate the Tested-by/Reviewed-by to all patches on the series, or since I believe you're using patchwork, you want us to post Tested-by/Reviewed-by replies to each patch?

Thanks!

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

I'll propagate them, thanks. I use patchwork, but really only as a to-do list. I actually apply patches from mutt and tweak them using stgit.

Bjorn

Applied to pci/host-mvebu for v4.4, with Tested-by Thomas and Andrew and Reviewed-by Thomas. Thanks, Russell!

Bjorn