[PATCH 0/6] mvebu PCI fixes and cleanups

[PATCH 0/6] mvebu PCI fixes and cleanups

[Russell King - ARM Linux]

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.

[PATCH 1/6] pci: mvebu: provide a compliant PCI configuration space

[Russell King]

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.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 67ec5e1c99db..b6a096bc9422 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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

[PATCH 2/6] pci: mvebu: generate proper configuration access cycles

[Russell King]

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

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index b6a096bc9422..0d9f3eae4315 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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

Re: [PATCH 2/6] pci: mvebu: generate proper configuration access cycles

[Andrew Lunn]

On Wed, Sep 23, 2015 at 06:17:32PM +0100, Russell King wrote:
> 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.

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

Re: [PATCH 2/6] pci: mvebu: generate proper configuration access cycles

[Andrew Lunn]

> 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

Hi Russell

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

root@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@dir665:~# setpci -s 1:0.0 0x88.l
00000000

Nothing there, so your test does not work directly.

I tried

root@dir665:~# setpci -s 1:0.0 0xe8.l
00102000
root@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

Re: [PATCH 2/6] pci: mvebu: generate proper configuration access cycles

[Russell King - ARM Linux]

On Fri, Sep 25, 2015 at 12:23:22AM +0200, Andrew Lunn wrote:
> > Testing with an ASM1062 PCIe to SATA mini-PCIe card on Armada 388.
> > PCIe capability at 0x80, DevCtl at 0x88, DevSta at 0x8a.

>From this, the PCIe DevCtl register is at an offset of 8 bytes from the
start of the PCIe capability.

>         Capabilities: [e0] Express (v1) Legacy Endpoint, MSI 00

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

>                 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

This register is at 0xe8, and:

>                 DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-

is at 0xea.

> root@dir665:~# setpci -s 1:0.0 0x88.l
> 00000000
> 
> Nothing there, so your test does not work directly.

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.

> I tried
> 
> root@dir665:~# setpci -s 1:0.0 0xe8.l
> 00102000
> root@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.

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.

Re: [PATCH 2/6] pci: mvebu: generate proper configuration access cycles

[Andrew Lunn]

> > I tried
> > 
> > root@dir665:~# setpci -s 1:0.0 0xe8.l
> > 00102000
> > root@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.
> 
> 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.

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

[PATCH 3/6] pci: mvebu: use of_get_available_child_count()

[Russell King]

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

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 0d9f3eae4315..0ed14f477bf8 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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

[PATCH 4/6] pci: mvebu: use for_each_available_child_of_node() to walk child nodes

[Russell King]

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

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 0ed14f477bf8..d331d2664580 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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

[PATCH 5/6] pci: mvebu: report full node name when reporting a DT error

[Russell King]

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.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index d331d2664580..8c715444b722 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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

[PATCH 6/6] pci: mvebu: use port->name rather than "PCIe%d.%d"

[Russell King]

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.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 8c715444b722..19144ed7bdad 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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

Re: [PATCH 0/6] mvebu PCI fixes and cleanups

[Andrew Lunn]

On Wed, Sep 23, 2015 at 06:17:07PM +0100, Russell King - ARM Linux wrote:
> This small series contains a number of fixes and cleanups to the
> mvebu PCI host code.

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.

Tested-by: Andrew Lunn <andrew@lunn.ch>

Thanks
	Andrew

Re: [PATCH 0/6] mvebu PCI fixes and cleanups

[Thomas Petazzoni]

Russell,

On Wed, 23 Sep 2015 18:17:07 +0100, Russell King - ARM Linux wrote:
> This small series contains a number of fixes and cleanups to the
> mvebu PCI host code.

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.

Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

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

Re: [PATCH 0/6] mvebu PCI fixes and cleanups

[Bjorn Helgaas]

On Fri, Sep 25, 2015 at 09:38:58AM +0200, Thomas Petazzoni wrote:
> Russell,
> 
> On Wed, 23 Sep 2015 18:17:07 +0100, Russell King - ARM Linux wrote:
> > This small series contains a number of fixes and cleanups to the
> > mvebu PCI host code.
> 
> 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.
> 
> Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> 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?

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

Re: [PATCH 0/6] mvebu PCI fixes and cleanups

[Bjorn Helgaas]

On Wed, Sep 23, 2015 at 06:17:07PM +0100, Russell King - ARM Linux wrote:
> 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(-)

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

Bjorn