* [PATCH 00/13] PCI: aardvark controller fixes
@ 2021-10-01 19:58 Marek Behún
2021-10-01 19:58 ` [PATCH 01/13] PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros Marek Behún
` (13 more replies)
0 siblings, 14 replies; 47+ messages in thread
From: Marek Behún @ 2021-10-01 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali, Marek Behún
Hi Lorenzo,
this series fixes some issues with the Aardvark PCIe controller driver.
Most of them are small changes. Patch 11 has a rather long commit message
since it explains how the bugs were introduced from multiple misleading
names and comments of some registers.
We have another 56 fixes for aardvark, but last time nobody wanted to
review such a large series, so we are now trying in smaller batches.
It would be great if you could find time to review, since we would like
this to land in 5.16. Preferably we would like to send another batch for
5.16, but we will see how fast this one goes
Marek & Pali
Marek Behún (2):
PCI: aardvark: Don't spam about PIO Response Status
PCI: aardvark: Deduplicate code in advk_pcie_rd_conf()
Pali Rohár (11):
PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros
PCI: aardvark: Fix PCIe Max Payload Size setting
PCI: aardvark: Fix preserving PCI_EXP_RTCTL_CRSSVE flag on emulated
bridge
PCI: aardvark: Fix configuring Reference clock
PCI: aardvark: Do not clear status bits of masked interrupts
PCI: aardvark: Do not unmask unused interrupts
PCI: aardvark: Implement re-issuing config requests on CRS response
PCI: aardvark: Simplify initialization of rootcap on virtual bridge
PCI: aardvark: Fix link training
PCI: aardvark: Fix checking for link up via LTSSM state
PCI: aardvark: Fix reporting Data Link Layer Link Active
drivers/pci/controller/pci-aardvark.c | 364 +++++++++++++++-----------
include/uapi/linux/pci_regs.h | 6 +
2 files changed, 212 insertions(+), 158 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 01/13] PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
@ 2021-10-01 19:58 ` Marek Behún
2021-10-02 16:05 ` Bjorn Helgaas
2021-10-04 8:43 ` Lorenzo Pieralisi
2021-10-01 19:58 ` [PATCH 02/13] PCI: aardvark: Fix PCIe Max Payload Size setting Marek Behún
` (12 subsequent siblings)
13 siblings, 2 replies; 47+ messages in thread
From: Marek Behún @ 2021-10-01 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali, Marek Behún
From: Pali Rohár <pali@kernel.org>
Define a macro PCI_EXP_DEVCTL_PAYLOAD_* for every possible Max Payload
Size in linux/pci_regs.h, in the same style as PCI_EXP_DEVCTL_READRQ_*.
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
include/uapi/linux/pci_regs.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e709ae8235e7..ff6ccbc6efe9 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -504,6 +504,12 @@
#define PCI_EXP_DEVCTL_URRE 0x0008 /* Unsupported Request Reporting En. */
#define PCI_EXP_DEVCTL_RELAX_EN 0x0010 /* Enable relaxed ordering */
#define PCI_EXP_DEVCTL_PAYLOAD 0x00e0 /* Max_Payload_Size */
+#define PCI_EXP_DEVCTL_PAYLOAD_128B 0x0000 /* 128 Bytes */
+#define PCI_EXP_DEVCTL_PAYLOAD_256B 0x0020 /* 256 Bytes */
+#define PCI_EXP_DEVCTL_PAYLOAD_512B 0x0040 /* 512 Bytes */
+#define PCI_EXP_DEVCTL_PAYLOAD_1024B 0x0060 /* 1024 Bytes */
+#define PCI_EXP_DEVCTL_PAYLOAD_2048B 0x0080 /* 2048 Bytes */
+#define PCI_EXP_DEVCTL_PAYLOAD_4096B 0x00a0 /* 4096 Bytes */
#define PCI_EXP_DEVCTL_EXT_TAG 0x0100 /* Extended Tag Field Enable */
#define PCI_EXP_DEVCTL_PHANTOM 0x0200 /* Phantom Functions Enable */
#define PCI_EXP_DEVCTL_AUX_PME 0x0400 /* Auxiliary Power PM Enable */
--
2.32.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 02/13] PCI: aardvark: Fix PCIe Max Payload Size setting
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
2021-10-01 19:58 ` [PATCH 01/13] PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros Marek Behún
@ 2021-10-01 19:58 ` Marek Behún
2021-10-03 17:44 ` Marek Behún
2021-10-01 19:58 ` [PATCH 03/13] PCI: aardvark: Don't spam about PIO Response Status Marek Behún
` (11 subsequent siblings)
13 siblings, 1 reply; 47+ messages in thread
From: Marek Behún @ 2021-10-01 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali,
Marek Behún, stable
From: Pali Rohár <pali@kernel.org>
Change PCIe Max Payload Size setting in PCIe Device Control register to 512
bytes to align with PCIe Link Initialization sequence as defined in Marvell
Armada 3700 Functional Specification. According to the specification,
maximal Max Payload Size supported by this device is 512 bytes.
Without this kernel prints suspicious line:
pci 0000:01:00.0: Upstream bridge's Max Payload Size set to 256 (was 16384, max 512)
With this change it changes to:
pci 0000:01:00.0: Upstream bridge's Max Payload Size set to 256 (was 512, max 512)
Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org
---
drivers/pci/controller/pci-aardvark.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 596ebcfcc82d..884510630bae 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -488,8 +488,9 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
reg &= ~PCI_EXP_DEVCTL_RELAX_EN;
reg &= ~PCI_EXP_DEVCTL_NOSNOOP_EN;
+ reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
reg &= ~PCI_EXP_DEVCTL_READRQ;
- reg |= PCI_EXP_DEVCTL_PAYLOAD; /* Set max payload size */
+ reg |= PCI_EXP_DEVCTL_PAYLOAD_512B;
reg |= PCI_EXP_DEVCTL_READRQ_512B;
advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
--
2.32.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 03/13] PCI: aardvark: Don't spam about PIO Response Status
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
2021-10-01 19:58 ` [PATCH 01/13] PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros Marek Behún
2021-10-01 19:58 ` [PATCH 02/13] PCI: aardvark: Fix PCIe Max Payload Size setting Marek Behún
@ 2021-10-01 19:58 ` Marek Behún
2021-10-01 19:58 ` [PATCH 04/13] PCI: aardvark: Fix preserving PCI_EXP_RTCTL_CRSSVE flag on emulated bridge Marek Behún
` (10 subsequent siblings)
13 siblings, 0 replies; 47+ messages in thread
From: Marek Behún @ 2021-10-01 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali, Marek Behún
Use dev_dbg() instead of dev_err() in advk_pcie_check_pio_status().
For example CRS is not an error status, it just says that the request
should be retried.
Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
drivers/pci/controller/pci-aardvark.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 884510630bae..a7903c531aa3 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -683,7 +683,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
else
str_posted = "Posted";
- dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
+ dev_dbg(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
return -EFAULT;
--
2.32.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 04/13] PCI: aardvark: Fix preserving PCI_EXP_RTCTL_CRSSVE flag on emulated bridge
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
` (2 preceding siblings ...)
2021-10-01 19:58 ` [PATCH 03/13] PCI: aardvark: Don't spam about PIO Response Status Marek Behún
@ 2021-10-01 19:58 ` Marek Behún
2021-10-01 19:58 ` [PATCH 05/13] PCI: aardvark: Fix configuring Reference clock Marek Behún
` (9 subsequent siblings)
13 siblings, 0 replies; 47+ messages in thread
From: Marek Behún @ 2021-10-01 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali, Marek Behún
From: Pali Rohár <pali@kernel.org>
Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") started
using CRSSVE flag for handling CRS responses.
PCI_EXP_RTCTL_CRSSVE flag is stored only in emulated config space buffer
and there is handler for PCI_EXP_RTCTL register. So every read operation
from config space automatically clears CRSSVE flag as it is not defined in
PCI_EXP_RTCTL read handler.
Fix this by reading current CRSSVE bit flag from emulated space buffer and
appending it to PCI_EXP_RTCTL read response.
Fixes: 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value")
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
drivers/pci/controller/pci-aardvark.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index a7903c531aa3..bb57ca6aed35 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -724,6 +724,7 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
case PCI_EXP_RTCTL: {
u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG);
*value = (val & PCIE_MSG_PM_PME_MASK) ? 0 : PCI_EXP_RTCTL_PMEIE;
+ *value |= le16_to_cpu(bridge->pcie_conf.rootctl) & PCI_EXP_RTCTL_CRSSVE;
*value |= PCI_EXP_RTCAP_CRSVIS << 16;
return PCI_BRIDGE_EMUL_HANDLED;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 05/13] PCI: aardvark: Fix configuring Reference clock
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
` (3 preceding siblings ...)
2021-10-01 19:58 ` [PATCH 04/13] PCI: aardvark: Fix preserving PCI_EXP_RTCTL_CRSSVE flag on emulated bridge Marek Behún
@ 2021-10-01 19:58 ` Marek Behún
2021-10-01 19:58 ` [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts Marek Behún
` (8 subsequent siblings)
13 siblings, 0 replies; 47+ messages in thread
From: Marek Behún @ 2021-10-01 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali,
Marek Behún, stable
From: Pali Rohár <pali@kernel.org>
Commit 366697018c9a ("PCI: aardvark: Add PHY support") introduced
configuration of PCIe Reference clock via PCIE_CORE_REF_CLK_REG register,
but did it incorrectly.
PCIe Reference clock differential pair is routed from system board to
endpoint card, so on CPU side it has output direction. Therefore it is
required to enable transmitting and disable receiving.
Default configuration according to Armada 3700 Functional Specifications is
enabled receiver part and disabled transmitter.
We need this change because otherwise PCIe Reference clock is configured to
some undefined state when differential pair is used for both transmitting
and receiving.
Fix this by disabling receiver part.
Fixes: 366697018c9a ("PCI: aardvark: Add PHY support")
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org
---
drivers/pci/controller/pci-aardvark.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index bb57ca6aed35..d5d6f92e5143 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -99,6 +99,7 @@
#define PCIE_CORE_CTRL2_MSI_ENABLE BIT(10)
#define PCIE_CORE_REF_CLK_REG (CONTROL_BASE_ADDR + 0x14)
#define PCIE_CORE_REF_CLK_TX_ENABLE BIT(1)
+#define PCIE_CORE_REF_CLK_RX_ENABLE BIT(2)
#define PCIE_MSG_LOG_REG (CONTROL_BASE_ADDR + 0x30)
#define PCIE_ISR0_REG (CONTROL_BASE_ADDR + 0x40)
#define PCIE_MSG_PM_PME_MASK BIT(7)
@@ -451,9 +452,15 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
u32 reg;
int i;
- /* Enable TX */
+ /*
+ * Configure PCIe Reference clock. Direction is from the PCIe
+ * controller to the endpoint card, so enable transmitting of
+ * Reference clock differential signal off-chip and disable
+ * receiving off-chip differential signal.
+ */
reg = advk_readl(pcie, PCIE_CORE_REF_CLK_REG);
reg |= PCIE_CORE_REF_CLK_TX_ENABLE;
+ reg &= ~PCIE_CORE_REF_CLK_RX_ENABLE;
advk_writel(pcie, reg, PCIE_CORE_REF_CLK_REG);
/* Set to Direct mode */
--
2.32.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
` (4 preceding siblings ...)
2021-10-01 19:58 ` [PATCH 05/13] PCI: aardvark: Fix configuring Reference clock Marek Behún
@ 2021-10-01 19:58 ` Marek Behún
2021-10-04 14:06 ` Lorenzo Pieralisi
2021-10-01 19:58 ` [PATCH 07/13] PCI: aardvark: Do not unmask unused interrupts Marek Behún
` (7 subsequent siblings)
13 siblings, 1 reply; 47+ messages in thread
From: Marek Behún @ 2021-10-01 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali,
Marek Behún, stable
From: Pali Rohár <pali@kernel.org>
It is incorrect to clear status bits of masked interrupts.
The aardvark driver clears all status interrupt bits if no unmasked
status bit is set. Masked bits should never be cleared.
Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org
---
drivers/pci/controller/pci-aardvark.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index d5d6f92e5143..e4986806a189 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1295,11 +1295,8 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie)
isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG);
isr1_status = isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
- if (!isr0_status && !isr1_status) {
- advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
- advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
+ if (!isr0_status && !isr1_status)
return;
- }
/* Process MSI interrupts */
if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
--
2.32.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 07/13] PCI: aardvark: Do not unmask unused interrupts
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
` (5 preceding siblings ...)
2021-10-01 19:58 ` [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts Marek Behún
@ 2021-10-01 19:58 ` Marek Behún
2021-10-01 19:58 ` [PATCH 08/13] PCI: aardvark: Deduplicate code in advk_pcie_rd_conf() Marek Behún
` (6 subsequent siblings)
13 siblings, 0 replies; 47+ messages in thread
From: Marek Behún @ 2021-10-01 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali,
Marek Behún, stable
From: Pali Rohár <pali@kernel.org>
There are lot of undocumented interrupt bits. Fix all *_ALL_MASK macros to
define all interrupt bits, so that driver can properly mask all interrupts,
including those which are undocumented.
Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org
---
drivers/pci/controller/pci-aardvark.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index e4986806a189..46fbaba1e6e4 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -107,13 +107,13 @@
#define PCIE_ISR0_MSI_INT_PENDING BIT(24)
#define PCIE_ISR0_INTX_ASSERT(val) BIT(16 + (val))
#define PCIE_ISR0_INTX_DEASSERT(val) BIT(20 + (val))
-#define PCIE_ISR0_ALL_MASK GENMASK(26, 0)
+#define PCIE_ISR0_ALL_MASK GENMASK(31, 0)
#define PCIE_ISR1_REG (CONTROL_BASE_ADDR + 0x48)
#define PCIE_ISR1_MASK_REG (CONTROL_BASE_ADDR + 0x4C)
#define PCIE_ISR1_POWER_STATE_CHANGE BIT(4)
#define PCIE_ISR1_FLUSH BIT(5)
#define PCIE_ISR1_INTX_ASSERT(val) BIT(8 + (val))
-#define PCIE_ISR1_ALL_MASK GENMASK(11, 4)
+#define PCIE_ISR1_ALL_MASK GENMASK(31, 0)
#define PCIE_MSI_ADDR_LOW_REG (CONTROL_BASE_ADDR + 0x50)
#define PCIE_MSI_ADDR_HIGH_REG (CONTROL_BASE_ADDR + 0x54)
#define PCIE_MSI_STATUS_REG (CONTROL_BASE_ADDR + 0x58)
@@ -199,7 +199,7 @@
#define PCIE_IRQ_MSI_INT2_DET BIT(21)
#define PCIE_IRQ_RC_DBELL_DET BIT(22)
#define PCIE_IRQ_EP_STATUS BIT(23)
-#define PCIE_IRQ_ALL_MASK 0xfff0fb
+#define PCIE_IRQ_ALL_MASK GENMASK(31, 0)
#define PCIE_IRQ_ENABLE_INTS_MASK PCIE_IRQ_CORE_INT
/* Transaction types */
--
2.32.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 08/13] PCI: aardvark: Deduplicate code in advk_pcie_rd_conf()
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
` (6 preceding siblings ...)
2021-10-01 19:58 ` [PATCH 07/13] PCI: aardvark: Do not unmask unused interrupts Marek Behún
@ 2021-10-01 19:58 ` Marek Behún
2021-10-01 19:58 ` [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response Marek Behún
` (5 subsequent siblings)
13 siblings, 0 replies; 47+ messages in thread
From: Marek Behún @ 2021-10-01 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali, Marek Behún
Avoid code repetition in advk_pcie_rd_conf() by handling errors with
goto jump, as is customary in kernel.
Fixes: 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
drivers/pci/controller/pci-aardvark.c | 48 +++++++++++----------------
1 file changed, 20 insertions(+), 28 deletions(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 46fbaba1e6e4..51d2955d9cca 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -920,18 +920,8 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
(le16_to_cpu(pcie->bridge.pcie_conf.rootctl) &
PCI_EXP_RTCTL_CRSSVE);
- if (advk_pcie_pio_is_running(pcie)) {
- /*
- * If it is possible return Completion Retry Status so caller
- * tries to issue the request again instead of failing.
- */
- if (allow_crs) {
- *val = CFG_RD_CRS_VAL;
- return PCIBIOS_SUCCESSFUL;
- }
- *val = 0xffffffff;
- return PCIBIOS_SET_FAILED;
- }
+ if (advk_pcie_pio_is_running(pcie))
+ goto try_crs;
/* Program the control register */
reg = advk_readl(pcie, PIO_CTRL);
@@ -955,25 +945,13 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
advk_writel(pcie, 1, PIO_START);
ret = advk_pcie_wait_pio(pcie);
- if (ret < 0) {
- /*
- * If it is possible return Completion Retry Status so caller
- * tries to issue the request again instead of failing.
- */
- if (allow_crs) {
- *val = CFG_RD_CRS_VAL;
- return PCIBIOS_SUCCESSFUL;
- }
- *val = 0xffffffff;
- return PCIBIOS_SET_FAILED;
- }
+ if (ret < 0)
+ goto try_crs;
/* Check PIO status and get the read result */
ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
- if (ret < 0) {
- *val = 0xffffffff;
- return PCIBIOS_SET_FAILED;
- }
+ if (ret < 0)
+ goto fail;
if (size == 1)
*val = (*val >> (8 * (where & 3))) & 0xff;
@@ -981,6 +959,20 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
*val = (*val >> (8 * (where & 3))) & 0xffff;
return PCIBIOS_SUCCESSFUL;
+
+try_crs:
+ /*
+ * If it is possible, return Completion Retry Status so that caller
+ * tries to issue the request again instead of failing.
+ */
+ if (allow_crs) {
+ *val = CFG_RD_CRS_VAL;
+ return PCIBIOS_SUCCESSFUL;
+ }
+
+fail:
+ *val = 0xffffffff;
+ return PCIBIOS_SET_FAILED;
}
static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
--
2.32.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
` (7 preceding siblings ...)
2021-10-01 19:58 ` [PATCH 08/13] PCI: aardvark: Deduplicate code in advk_pcie_rd_conf() Marek Behún
@ 2021-10-01 19:58 ` Marek Behún
2021-10-02 16:35 ` Bjorn Helgaas
2021-10-01 19:58 ` [PATCH 10/13] PCI: aardvark: Simplify initialization of rootcap on virtual bridge Marek Behún
` (4 subsequent siblings)
13 siblings, 1 reply; 47+ messages in thread
From: Marek Behún @ 2021-10-01 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali, Marek Behún
From: Pali Rohár <pali@kernel.org>
Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
handling of CRS response and when CRSSVE flag was not enabled it marked CRS
response as failed transaction (due to simplicity).
But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
for PIO config response and so we can with a small change implement
re-issuing of config requests as described in PCIe base specification.
This change implements re-issuing of config requests when response is CRS.
Set upper bound of wait cycles to around PIO_RETRY_CNT, afterwards the
transaction is marked as failed and an all-ones value is returned as
before.
We do this by returning appropriate error codes from function
advk_pcie_check_pio_status(). On CRS we return -EAGAIN and caller then
reissues transaction.
Fixes: 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value")
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
drivers/pci/controller/pci-aardvark.c | 67 +++++++++++++++++----------
1 file changed, 43 insertions(+), 24 deletions(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 51d2955d9cca..7b9870d0b81f 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -603,6 +603,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
u32 reg;
unsigned int status;
char *strcomp_status, *str_posted;
+ int ret;
reg = advk_readl(pcie, PIO_STAT);
status = (reg & PIO_COMPLETION_STATUS_MASK) >>
@@ -627,6 +628,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
case PIO_COMPLETION_STATUS_OK:
if (reg & PIO_ERR_STATUS) {
strcomp_status = "COMP_ERR";
+ ret = -EFAULT;
break;
}
/* Get the read result */
@@ -634,9 +636,11 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
*val = advk_readl(pcie, PIO_RD_DATA);
/* No error */
strcomp_status = NULL;
+ ret = 0;
break;
case PIO_COMPLETION_STATUS_UR:
strcomp_status = "UR";
+ ret = -EOPNOTSUPP;
break;
case PIO_COMPLETION_STATUS_CRS:
if (allow_crs && val) {
@@ -654,6 +658,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
*/
*val = CFG_RD_CRS_VAL;
strcomp_status = NULL;
+ ret = 0;
break;
}
/* PCIe r4.0, sec 2.3.2, says:
@@ -669,21 +674,24 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
* Request and taking appropriate action, e.g., complete the
* Request to the host as a failed transaction.
*
- * To simplify implementation do not re-issue the Configuration
- * Request and complete the Request as a failed transaction.
+ * So return -EAGAIN and caller (pci-aardvark.c driver) will
+ * re-issue request again up to the PIO_RETRY_CNT retries.
*/
strcomp_status = "CRS";
+ ret = -EAGAIN;
break;
case PIO_COMPLETION_STATUS_CA:
strcomp_status = "CA";
+ ret = -ECANCELED;
break;
default:
strcomp_status = "Unknown";
+ ret = -EINVAL;
break;
}
if (!strcomp_status)
- return 0;
+ return ret;
if (reg & PIO_NON_POSTED_REQ)
str_posted = "Non-posted";
@@ -693,7 +701,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
dev_dbg(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
- return -EFAULT;
+ return ret;
}
static int advk_pcie_wait_pio(struct advk_pcie *pcie)
@@ -701,13 +709,13 @@ static int advk_pcie_wait_pio(struct advk_pcie *pcie)
struct device *dev = &pcie->pdev->dev;
int i;
- for (i = 0; i < PIO_RETRY_CNT; i++) {
+ for (i = 1; i <= PIO_RETRY_CNT; i++) {
u32 start, isr;
start = advk_readl(pcie, PIO_START);
isr = advk_readl(pcie, PIO_ISR);
if (!start && isr)
- return 0;
+ return i;
udelay(PIO_RETRY_DELAY);
}
@@ -898,6 +906,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
int where, int size, u32 *val)
{
struct advk_pcie *pcie = bus->sysdata;
+ int retry_count;
bool allow_crs;
u32 reg;
int ret;
@@ -940,16 +949,22 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
/* Program the data strobe */
advk_writel(pcie, 0xf, PIO_WR_DATA_STRB);
- /* Clear PIO DONE ISR and start the transfer */
- advk_writel(pcie, 1, PIO_ISR);
- advk_writel(pcie, 1, PIO_START);
+ retry_count = 0;
+ do {
+ /* Clear PIO DONE ISR and start the transfer */
+ advk_writel(pcie, 1, PIO_ISR);
+ advk_writel(pcie, 1, PIO_START);
- ret = advk_pcie_wait_pio(pcie);
- if (ret < 0)
- goto try_crs;
+ ret = advk_pcie_wait_pio(pcie);
+ if (ret < 0)
+ goto try_crs;
+
+ retry_count += ret;
+
+ /* Check PIO status and get the read result */
+ ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
+ } while (ret == -EAGAIN && retry_count < PIO_RETRY_CNT);
- /* Check PIO status and get the read result */
- ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
if (ret < 0)
goto fail;
@@ -981,6 +996,7 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
struct advk_pcie *pcie = bus->sysdata;
u32 reg;
u32 data_strobe = 0x0;
+ int retry_count;
int offset;
int ret;
@@ -1022,19 +1038,22 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
/* Program the data strobe */
advk_writel(pcie, data_strobe, PIO_WR_DATA_STRB);
- /* Clear PIO DONE ISR and start the transfer */
- advk_writel(pcie, 1, PIO_ISR);
- advk_writel(pcie, 1, PIO_START);
+ retry_count = 0;
+ do {
+ /* Clear PIO DONE ISR and start the transfer */
+ advk_writel(pcie, 1, PIO_ISR);
+ advk_writel(pcie, 1, PIO_START);
- ret = advk_pcie_wait_pio(pcie);
- if (ret < 0)
- return PCIBIOS_SET_FAILED;
+ ret = advk_pcie_wait_pio(pcie);
+ if (ret < 0)
+ return PCIBIOS_SET_FAILED;
- ret = advk_pcie_check_pio_status(pcie, false, NULL);
- if (ret < 0)
- return PCIBIOS_SET_FAILED;
+ retry_count += ret;
- return PCIBIOS_SUCCESSFUL;
+ ret = advk_pcie_check_pio_status(pcie, false, NULL);
+ } while (ret == -EAGAIN && retry_count < PIO_RETRY_CNT);
+
+ return ret < 0 ? PCIBIOS_SET_FAILED : PCIBIOS_SUCCESSFUL;
}
static struct pci_ops advk_pcie_ops = {
--
2.32.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 10/13] PCI: aardvark: Simplify initialization of rootcap on virtual bridge
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
` (8 preceding siblings ...)
2021-10-01 19:58 ` [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response Marek Behún
@ 2021-10-01 19:58 ` Marek Behún
2021-10-04 9:44 ` Lorenzo Pieralisi
2021-10-01 19:58 ` [PATCH 11/13] PCI: aardvark: Fix link training Marek Behún
` (3 subsequent siblings)
13 siblings, 1 reply; 47+ messages in thread
From: Marek Behún @ 2021-10-01 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali, Marek Behún
From: Pali Rohár <pali@kernel.org>
PCIe config space can be initialized also before pci_bridge_emul_init()
call, so move rootcap initialization after PCI config space initialization.
This simplifies the function a little since it removes one if (ret < 0)
check.
Fixes: 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value")
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
drivers/pci/controller/pci-aardvark.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 7b9870d0b81f..74d1ec7eff16 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -822,7 +822,6 @@ static struct pci_bridge_emul_ops advk_pci_bridge_emul_ops = {
static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
{
struct pci_bridge_emul *bridge = &pcie->bridge;
- int ret;
bridge->conf.vendor =
cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff);
@@ -842,19 +841,14 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
/* Support interrupt A for MSI feature */
bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
+ /* Indicates supports for Completion Retry Status */
+ bridge->pcie_conf.rootcap = cpu_to_le16(PCI_EXP_RTCAP_CRSVIS);
+
bridge->has_pcie = true;
bridge->data = pcie;
bridge->ops = &advk_pci_bridge_emul_ops;
- /* PCIe config space can be initialized after pci_bridge_emul_init() */
- ret = pci_bridge_emul_init(bridge, 0);
- if (ret < 0)
- return ret;
-
- /* Indicates supports for Completion Retry Status */
- bridge->pcie_conf.rootcap = cpu_to_le16(PCI_EXP_RTCAP_CRSVIS);
-
- return 0;
+ return pci_bridge_emul_init(bridge, 0);
}
static bool advk_pcie_valid_device(struct advk_pcie *pcie, struct pci_bus *bus,
--
2.32.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 11/13] PCI: aardvark: Fix link training
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
` (9 preceding siblings ...)
2021-10-01 19:58 ` [PATCH 10/13] PCI: aardvark: Simplify initialization of rootcap on virtual bridge Marek Behún
@ 2021-10-01 19:58 ` Marek Behún
2021-10-01 19:58 ` [PATCH 12/13] PCI: aardvark: Fix checking for link up via LTSSM state Marek Behún
` (2 subsequent siblings)
13 siblings, 0 replies; 47+ messages in thread
From: Marek Behún @ 2021-10-01 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali,
Marek Behún, stable
From: Pali Rohár <pali@kernel.org>
Fix multiple link training issues in aardvark driver. The main reason of
these issues was misunderstanding of what certain registers do, since their
names and comments were misleading: before commit 96be36dbffac ("PCI:
aardvark: Replace custom macros by standard linux/pci_regs.h macros"), the
pci-aardvark.c driver used custom macros for accessing standard PCIe Root
Bridge registers, and misleading comments did not help to understand what
the code was really doing.
After doing more tests and experiments I've come to the conclusion that the
SPEED_GEN register in aardvark sets the PCIe revision / generation
compliance and forces maximal link speed. Both GEN3 and GEN2 values set the
read-only PCI_EXP_FLAGS_VERS bits (PCIe capabilities version of Root
Bridge) to value 2, while GEN1 value sets PCI_EXP_FLAGS_VERS to 1, which
matches with PCI Express specifications revisions 3, 2 and 1 respectively.
Changing SPEED_GEN also sets the read-only bits PCI_EXP_LNKCAP_SLS and
PCI_EXP_LNKCAP2_SLS to corresponding speed.
(Note that PCI Express rev 1 specification does not define PCI_EXP_LNKCAP2
and PCI_EXP_LNKCTL2 registers and when SPEED_GEN is set to GEN1 (which
also sets PCI_EXP_FLAGS_VERS set to 1), lspci cannot access
PCI_EXP_LNKCAP2 and PCI_EXP_LNKCTL2 registers.)
Changing PCIe link speed can be done via PCI_EXP_LNKCTL2_TLS bits of
PCI_EXP_LNKCTL2 register. Armada 3700 Functional Specifications says that
the default value of PCI_EXP_LNKCTL2_TLS is based on SPEED_GEN value, but
tests showed that the default value is always 8.0 GT/s, independently of
speed set by SPEED_GEN. So after setting SPEED_GEN, we must also set value
in PCI_EXP_LNKCTL2 register via PCI_EXP_LNKCTL2_TLS bits.
Triggering PCI_EXP_LNKCTL_RL bit immediately after setting LINK_TRAINING_EN
bit actually doesn't do anything. Tests have shown that a delay is needed
after enabling LINK_TRAINING_EN bit. As triggering PCI_EXP_LNKCTL_RL
currently does nothing, remove it.
Commit 43fc679ced18 ("PCI: aardvark: Improve link training") introduced
code which sets SPEED_GEN register based on negotiated link speed from
PCI_EXP_LNKSTA_CLS bits of PCI_EXP_LNKSTA register. This code was added to
fix detection of Compex WLE900VX (Atheros QCA9880) WiFi GEN1 PCIe cards, as
otherwise these cards were "invisible" on PCIe bus (probably because they
crashed). But apparently more people reported the same issues with these
cards also with other PCIe controllers [1] and I was able to reproduce this
issue also with other "noname" WiFi cards based on Atheros QCA9890 chip
(with the same PCI vendor/device ids as Atheros QCA9880). So this is not an
issue in aardvark but rather an issue in Atheros QCA98xx chips. Also, this
issue only exists if the kernel is compiled with PCIe ASPM support, and a
generic workaround for this is to change PCIe Bridge to 2.5 GT/s link speed
via PCI_EXP_LNKCTL2_TLS_2_5GT bits in PCI_EXP_LNKCTL2 register [2], before
triggering PCI_EXP_LNKCTL_RL bit. This workaround also works when SPEED_GEN
is set to value GEN2 (5 GT/s). So remove this hack completely in the
aardvark driver and always set SPEED_GEN to value from 'max-link-speed' DT
property. Fix for Atheros QCA98xx chips is handled separately by patch [2].
These two things (code for triggering PCI_EXP_LNKCTL_RL bit and changing
SPEED_GEN value) also explain why commit 6964494582f5 ("PCI: aardvark:
Train link immediately after enabling training") somehow fixed detection of
those problematic Compex cards with Atheros chips: if triggering link
retraining (via PCI_EXP_LNKCTL_RL bit) was done immediately after enabling
link training (via LINK_TRAINING_EN), it did nothing. If there was a
specific delay, aardvark HW already initialized PCIe link and therefore
triggering link retraining caused the above issue. Compex cards triggered
link down event and disappeared from the PCIe bus.
Commit f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready before
training link") added 100ms sleep before calling 'Start link training'
command and explained that it is a requirement of PCI Express
specification. But the code after this 100ms sleep was not doing 'Start
link training', rather it triggered PCI_EXP_LNKCTL_RL bit via PCIe Root
Bridge to put link into Recovery state.
The required delay after fundamental reset is already done in function
advk_pcie_wait_for_link() which also checks whether PCIe link is up.
So after removing the code which triggers PCI_EXP_LNKCTL_RL bit on PCIe
Root Bridge, there is no need to wait 100ms again. Remove the extra
msleep() call and update comment about the delay required by the PCI
Express specification.
According to Marvell Armada 3700 Functional Specifications, Link training
should be enabled via aardvark register LINK_TRAINING_EN after selecting
PCIe generation and x1 lane. There is no need to disable it prior resetting
card via PERST# signal. This disabling code was introduced in commit
5169a9851daa ("PCI: aardvark: Issue PERST via GPIO") as a workaround for
some Atheros cards. It turns out that this also is Atheros specific issue
and affects any PCIe controller, not only aardvark. Moreover this Atheros
issue was triggered by juggling with PCI_EXP_LNKCTL_RL, LINK_TRAINING_EN
and SPEED_GEN bits interleaved with sleeps. Now, after removing triggering
PCI_EXP_LNKCTL_RL, there is no need to explicitly disable LINK_TRAINING_EN
bit. So remove this code too. The problematic Compex cards described in
previous git commits are correctly detected in advk_pcie_train_link()
function even after applying all these changes.
Note that with this patch, and also prior this patch, some NVMe disks which
support PCIe GEN3 with 8 GT/s speed are negotiated only at the lowest link
speed 2.5 GT/s, independently of SPEED_GEN value. After manually triggering
PCI_EXP_LNKCTL_RL bit (e.g. from userspace via setpci), these NVMe disks
change link speed to 5 GT/s when SPEED_GEN was configured to GEN2. This
issue first needs to be properly investigated. I will send a fix in the
future.
On the other hand, some other GEN2 PCIe cards with 5 GT/s speed are
autonomously by HW autonegotiated at full 5 GT/s speed without need of any
software interaction.
Armada 3700 Functional Specifications describes the following steps for
link training: set SPEED_GEN to GEN2, enable LINK_TRAINING_EN, poll until
link training is complete, trigger PCI_EXP_LNKCTL_RL, poll until signal
rate is 5 GT/s, poll until link training is complete, enable ASPM L0s.
The requirement for triggering PCI_EXP_LNKCTL_RL can be explained by the
need to achieve 5 GT/s speed (as changing link speed is done by throw to
recovery state entered by PCI_EXP_LNKCTL_RL) or maybe as a part of enabling
ASPM L0s (but in this case ASPM L0s should have been enabled prior
PCI_EXP_LNKCTL_RL).
It is unknown why the original pci-aardvark.c driver was triggering
PCI_EXP_LNKCTL_RL bit before waiting for the link to be up. This does not
align with neither PCIe base specifications nor with Armada 3700 Functional
Specification. (Note that in older versions of aardvark, this bit was
called incorrectly PCIE_CORE_LINK_TRAINING, so this may be the reason.)
It is also unknown why Armada 3700 Functional Specification says that it is
needed to trigger PCI_EXP_LNKCTL_RL for GEN2 mode, as according to PCIe
base specification 5 GT/s speed negotiation is supposed to be entirely
autonomous, even if initial speed is 2.5 GT/s.
[1] - https://lore.kernel.org/linux-pci/87h7l8axqp.fsf@toke.dk/
[2] - https://lore.kernel.org/linux-pci/20210326124326.21163-1-pali@kernel.org/
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org # f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready before training link")
Cc: stable@vger.kernel.org # 6964494582f5 ("PCI: aardvark: Train link immediately after enabling training")
Cc: stable@vger.kernel.org # 43fc679ced18 ("PCI: aardvark: Improve link training")
Cc: stable@vger.kernel.org # 5169a9851daa ("PCI: aardvark: Issue PERST via GPIO")
Cc: stable@vger.kernel.org # 96be36dbffac ("PCI: aardvark: Replace custom macros by standard linux/pci_regs.h macros")
Cc: stable@vger.kernel.org # d0c6a3475b03 ("PCI: aardvark: Move PCIe reset card code to advk_pcie_train_link()")
Cc: stable@vger.kernel.org # 1d1cd163d0de ("PCI: aardvark: Update comment about disabling link training")
---
drivers/pci/controller/pci-aardvark.c | 117 ++++++++------------------
1 file changed, 34 insertions(+), 83 deletions(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 74d1ec7eff16..5387d9cc3eba 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -258,11 +258,6 @@ static inline u32 advk_readl(struct advk_pcie *pcie, u64 reg)
return readl(pcie->base + reg);
}
-static inline u16 advk_read16(struct advk_pcie *pcie, u64 reg)
-{
- return advk_readl(pcie, (reg & ~0x3)) >> ((reg & 0x3) * 8);
-}
-
static int advk_pcie_link_up(struct advk_pcie *pcie)
{
u32 val, ltssm_state;
@@ -300,23 +295,9 @@ static void advk_pcie_wait_for_retrain(struct advk_pcie *pcie)
static void advk_pcie_issue_perst(struct advk_pcie *pcie)
{
- u32 reg;
-
if (!pcie->reset_gpio)
return;
- /*
- * As required by PCI Express spec (PCI Express Base Specification, REV.
- * 4.0 PCI Express, February 19 2014, 6.6.1 Conventional Reset) a delay
- * for at least 100ms after de-asserting PERST# signal is needed before
- * link training is enabled. So ensure that link training is disabled
- * prior de-asserting PERST# signal to fulfill that PCI Express spec
- * requirement.
- */
- reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
- reg &= ~LINK_TRAINING_EN;
- advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
-
/* 10ms delay is needed for some cards */
dev_info(&pcie->pdev->dev, "issuing PERST via reset GPIO for 10ms\n");
gpiod_set_value_cansleep(pcie->reset_gpio, 1);
@@ -324,53 +305,46 @@ static void advk_pcie_issue_perst(struct advk_pcie *pcie)
gpiod_set_value_cansleep(pcie->reset_gpio, 0);
}
-static int advk_pcie_train_at_gen(struct advk_pcie *pcie, int gen)
+static void advk_pcie_train_link(struct advk_pcie *pcie)
{
- int ret, neg_gen;
+ struct device *dev = &pcie->pdev->dev;
u32 reg;
+ int ret;
- /* Setup link speed */
+ /*
+ * Setup PCIe rev / gen compliance based on device tree property
+ * 'max-link-speed' which also forces maximal link speed.
+ */
reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
reg &= ~PCIE_GEN_SEL_MSK;
- if (gen == 3)
+ if (pcie->link_gen == 3)
reg |= SPEED_GEN_3;
- else if (gen == 2)
+ else if (pcie->link_gen == 2)
reg |= SPEED_GEN_2;
else
reg |= SPEED_GEN_1;
advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
/*
- * Enable link training. This is not needed in every call to this
- * function, just once suffices, but it does not break anything either.
+ * Set maximal link speed value also into PCIe Link Control 2 register.
+ * Armada 3700 Functional Specification says that default value is based
+ * on SPEED_GEN but tests showed that default value is always 8.0 GT/s.
*/
+ reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_LNKCTL2);
+ reg &= ~PCI_EXP_LNKCTL2_TLS;
+ if (pcie->link_gen == 3)
+ reg |= PCI_EXP_LNKCTL2_TLS_8_0GT;
+ else if (pcie->link_gen == 2)
+ reg |= PCI_EXP_LNKCTL2_TLS_5_0GT;
+ else
+ reg |= PCI_EXP_LNKCTL2_TLS_2_5GT;
+ advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_LNKCTL2);
+
+ /* Enable link training after selecting PCIe generation */
reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
reg |= LINK_TRAINING_EN;
advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
- /*
- * Start link training immediately after enabling it.
- * This solves problems for some buggy cards.
- */
- reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_LNKCTL);
- reg |= PCI_EXP_LNKCTL_RL;
- advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_LNKCTL);
-
- ret = advk_pcie_wait_for_link(pcie);
- if (ret)
- return ret;
-
- reg = advk_read16(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_LNKSTA);
- neg_gen = reg & PCI_EXP_LNKSTA_CLS;
-
- return neg_gen;
-}
-
-static void advk_pcie_train_link(struct advk_pcie *pcie)
-{
- struct device *dev = &pcie->pdev->dev;
- int neg_gen = -1, gen;
-
/*
* Reset PCIe card via PERST# signal. Some cards are not detected
* during link training when they are in some non-initial state.
@@ -381,41 +355,18 @@ static void advk_pcie_train_link(struct advk_pcie *pcie)
* PERST# signal could have been asserted by pinctrl subsystem before
* probe() callback has been called or issued explicitly by reset gpio
* function advk_pcie_issue_perst(), making the endpoint going into
- * fundamental reset. As required by PCI Express spec a delay for at
- * least 100ms after such a reset before link training is needed.
- */
- msleep(PCI_PM_D3COLD_WAIT);
-
- /*
- * Try link training at link gen specified by device tree property
- * 'max-link-speed'. If this fails, iteratively train at lower gen.
- */
- for (gen = pcie->link_gen; gen > 0; --gen) {
- neg_gen = advk_pcie_train_at_gen(pcie, gen);
- if (neg_gen > 0)
- break;
- }
-
- if (neg_gen < 0)
- goto err;
-
- /*
- * After successful training if negotiated gen is lower than requested,
- * train again on negotiated gen. This solves some stability issues for
- * some buggy gen1 cards.
+ * fundamental reset. As required by PCI Express spec (PCI Express
+ * Base Specification, REV. 4.0 PCI Express, February 19 2014, 6.6.1
+ * Conventional Reset) a delay for at least 100ms after such a reset
+ * before sending a Configuration Request to the device is needed.
+ * So wait until PCIe link is up. Function advk_pcie_wait_for_link()
+ * waits for link at least 900ms.
*/
- if (neg_gen < gen) {
- gen = neg_gen;
- neg_gen = advk_pcie_train_at_gen(pcie, gen);
- }
-
- if (neg_gen == gen) {
- dev_info(dev, "link up at gen %i\n", gen);
- return;
- }
-
-err:
- dev_err(dev, "link never came up\n");
+ ret = advk_pcie_wait_for_link(pcie);
+ if (ret < 0)
+ dev_err(dev, "link never came up\n");
+ else
+ dev_info(dev, "link up\n");
}
/*
--
2.32.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 12/13] PCI: aardvark: Fix checking for link up via LTSSM state
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
` (10 preceding siblings ...)
2021-10-01 19:58 ` [PATCH 11/13] PCI: aardvark: Fix link training Marek Behún
@ 2021-10-01 19:58 ` Marek Behún
2021-10-04 13:35 ` Lorenzo Pieralisi
2021-10-01 19:58 ` [PATCH 13/13] PCI: aardvark: Fix reporting Data Link Layer Link Active Marek Behún
2021-10-04 9:53 ` [PATCH 00/13] PCI: aardvark controller fixes Lorenzo Pieralisi
13 siblings, 1 reply; 47+ messages in thread
From: Marek Behún @ 2021-10-01 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali,
Marek Behún, Remi Pommarel, stable
From: Pali Rohár <pali@kernel.org>
Current implementation of advk_pcie_link_up() is wrong as it marks also
link disabled or hot reset states as link up.
Fix it by marking link up only to those states which are defined in PCIe
Base specification 3.0, Table 4-14: Link Status Mapped to the LTSSM.
To simplify implementation, Define macros for every LTSSM state which
aardvark hardware can return in CFG_REG register.
Fix also checking for link training according to the same Table 4-14.
Define a new function advk_pcie_link_training() for this purpose.
Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: Remi Pommarel <repk@triplefau.lt>
Cc: stable@vger.kernel.org
---
drivers/pci/controller/pci-aardvark.c | 69 +++++++++++++++++++++++++--
1 file changed, 64 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 5387d9cc3eba..9465b630cede 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -165,7 +165,44 @@
#define CFG_REG (LMI_BASE_ADDR + 0x0)
#define LTSSM_SHIFT 24
#define LTSSM_MASK 0x3f
+#define LTSSM_DETECT_QUIET 0x0
+#define LTSSM_DETECT_ACTIVE 0x1
+#define LTSSM_POLLING_ACTIVE 0x2
+#define LTSSM_POLLING_COMPLIANCE 0x3
+#define LTSSM_POLLING_CONFIGURATION 0x4
+#define LTSSM_CONFIG_LINKWIDTH_START 0x5
+#define LTSSM_CONFIG_LINKWIDTH_ACCEPT 0x6
+#define LTSSM_CONFIG_LANENUM_ACCEPT 0x7
+#define LTSSM_CONFIG_LANENUM_WAIT 0x8
+#define LTSSM_CONFIG_COMPLETE 0x9
+#define LTSSM_CONFIG_IDLE 0xa
+#define LTSSM_RECOVERY_RCVR_LOCK 0xb
+#define LTSSM_RECOVERY_SPEED 0xc
+#define LTSSM_RECOVERY_RCVR_CFG 0xd
+#define LTSSM_RECOVERY_IDLE 0xe
#define LTSSM_L0 0x10
+#define LTSSM_RX_L0S_ENTRY 0x11
+#define LTSSM_RX_L0S_IDLE 0x12
+#define LTSSM_RX_L0S_FTS 0x13
+#define LTSSM_TX_L0S_ENTRY 0x14
+#define LTSSM_TX_L0S_IDLE 0x15
+#define LTSSM_TX_L0S_FTS 0x16
+#define LTSSM_L1_ENTRY 0x17
+#define LTSSM_L1_IDLE 0x18
+#define LTSSM_L2_IDLE 0x19
+#define LTSSM_L2_TRANSMIT_WAKE 0x1a
+#define LTSSM_DISABLED 0x20
+#define LTSSM_LOOPBACK_ENTRY_MASTER 0x21
+#define LTSSM_LOOPBACK_ACTIVE_MASTER 0x22
+#define LTSSM_LOOPBACK_EXIT_MASTER 0x23
+#define LTSSM_LOOPBACK_ENTRY_SLAVE 0x24
+#define LTSSM_LOOPBACK_ACTIVE_SLAVE 0x25
+#define LTSSM_LOOPBACK_EXIT_SLAVE 0x26
+#define LTSSM_HOT_RESET 0x27
+#define LTSSM_RECOVERY_EQUALIZATION_PHASE0 0x28
+#define LTSSM_RECOVERY_EQUALIZATION_PHASE1 0x29
+#define LTSSM_RECOVERY_EQUALIZATION_PHASE2 0x2a
+#define LTSSM_RECOVERY_EQUALIZATION_PHASE3 0x2b
#define RC_BAR_CONFIG 0x300
#define VENDOR_ID_REG (LMI_BASE_ADDR + 0x44)
@@ -258,13 +295,35 @@ static inline u32 advk_readl(struct advk_pcie *pcie, u64 reg)
return readl(pcie->base + reg);
}
-static int advk_pcie_link_up(struct advk_pcie *pcie)
+static u8 advk_pcie_ltssm_state(struct advk_pcie *pcie)
{
- u32 val, ltssm_state;
+ u32 val;
+ u8 ltssm_state;
val = advk_readl(pcie, CFG_REG);
ltssm_state = (val >> LTSSM_SHIFT) & LTSSM_MASK;
- return ltssm_state >= LTSSM_L0;
+ return ltssm_state;
+}
+
+static inline bool advk_pcie_link_up(struct advk_pcie *pcie)
+{
+ /* check if LTSSM is in normal operation - some L* state */
+ u8 ltssm_state = advk_pcie_ltssm_state(pcie);
+ return ltssm_state >= LTSSM_L0 && ltssm_state < LTSSM_DISABLED;
+}
+
+static inline bool advk_pcie_link_training(struct advk_pcie *pcie)
+{
+ /*
+ * According to PCIe Base specification 3.0, Table 4-14: Link
+ * Status Mapped to the LTSSM is Link Training mapped to LTSSM
+ * Configuration and Recovery states.
+ */
+ u8 ltssm_state = advk_pcie_ltssm_state(pcie);
+ return ((ltssm_state >= LTSSM_CONFIG_LINKWIDTH_START &&
+ ltssm_state < LTSSM_L0) ||
+ (ltssm_state >= LTSSM_RECOVERY_EQUALIZATION_PHASE0 &&
+ ltssm_state <= LTSSM_RECOVERY_EQUALIZATION_PHASE3));
}
static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
@@ -287,7 +346,7 @@ static void advk_pcie_wait_for_retrain(struct advk_pcie *pcie)
size_t retries;
for (retries = 0; retries < RETRAIN_WAIT_MAX_RETRIES; ++retries) {
- if (!advk_pcie_link_up(pcie))
+ if (advk_pcie_link_training(pcie))
break;
udelay(RETRAIN_WAIT_USLEEP_US);
}
@@ -706,7 +765,7 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
/* u32 contains both PCI_EXP_LNKCTL and PCI_EXP_LNKSTA */
u32 val = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + reg) &
~(PCI_EXP_LNKSTA_LT << 16);
- if (!advk_pcie_link_up(pcie))
+ if (advk_pcie_link_training(pcie))
val |= (PCI_EXP_LNKSTA_LT << 16);
*value = val;
return PCI_BRIDGE_EMUL_HANDLED;
--
2.32.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 13/13] PCI: aardvark: Fix reporting Data Link Layer Link Active
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
` (11 preceding siblings ...)
2021-10-01 19:58 ` [PATCH 12/13] PCI: aardvark: Fix checking for link up via LTSSM state Marek Behún
@ 2021-10-01 19:58 ` Marek Behún
2021-10-04 9:53 ` [PATCH 00/13] PCI: aardvark controller fixes Lorenzo Pieralisi
13 siblings, 0 replies; 47+ messages in thread
From: Marek Behún @ 2021-10-01 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali,
Marek Behún, stable
From: Pali Rohár <pali@kernel.org>
Add support for reporting PCI_EXP_LNKSTA_DLLLA bit in Link Control register
on emulated bridge via current LTSSM state. Also correctly indicate DLLLA
capability via PCI_EXP_LNKCAP_DLLLARC bit in Link Control Capability
register.
Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space")
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org
---
drivers/pci/controller/pci-aardvark.c | 29 ++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 9465b630cede..f429c89ba4a1 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -312,6 +312,20 @@ static inline bool advk_pcie_link_up(struct advk_pcie *pcie)
return ltssm_state >= LTSSM_L0 && ltssm_state < LTSSM_DISABLED;
}
+static inline bool advk_pcie_link_active(struct advk_pcie *pcie)
+{
+ /*
+ * According to PCIe Base specification 3.0, Table 4-14: Link
+ * Status Mapped to the LTSSM, and 4.2.6.3.6 Configuration.Idle
+ * is Link Up mapped to LTSSM Configuration.Idle, Recovery, L0,
+ * L0s, L1 and L2 states. And according to 3.2.1. Data Link
+ * Control and Management State Machine Rules is DL Up status
+ * reported in DL Active state.
+ */
+ u8 ltssm_state = advk_pcie_ltssm_state(pcie);
+ return ltssm_state >= LTSSM_CONFIG_IDLE && ltssm_state < LTSSM_DISABLED;
+}
+
static inline bool advk_pcie_link_training(struct advk_pcie *pcie)
{
/*
@@ -761,12 +775,26 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
return PCI_BRIDGE_EMUL_HANDLED;
}
+ case PCI_EXP_LNKCAP: {
+ u32 val = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + reg);
+ /*
+ * PCI_EXP_LNKCAP_DLLLARC bit is hardwired in aardvark HW to 0.
+ * But support for PCI_EXP_LNKSTA_DLLLA is emulated via ltssm
+ * state so explicitly enable PCI_EXP_LNKCAP_DLLLARC flag.
+ */
+ val |= PCI_EXP_LNKCAP_DLLLARC;
+ *value = val;
+ return PCI_BRIDGE_EMUL_HANDLED;
+ }
+
case PCI_EXP_LNKCTL: {
/* u32 contains both PCI_EXP_LNKCTL and PCI_EXP_LNKSTA */
u32 val = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + reg) &
~(PCI_EXP_LNKSTA_LT << 16);
if (advk_pcie_link_training(pcie))
val |= (PCI_EXP_LNKSTA_LT << 16);
+ if (advk_pcie_link_active(pcie))
+ val |= (PCI_EXP_LNKSTA_DLLLA << 16);
*value = val;
return PCI_BRIDGE_EMUL_HANDLED;
}
@@ -774,7 +802,6 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
case PCI_CAP_LIST_ID:
case PCI_EXP_DEVCAP:
case PCI_EXP_DEVCTL:
- case PCI_EXP_LNKCAP:
*value = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + reg);
return PCI_BRIDGE_EMUL_HANDLED;
default:
--
2.32.0
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 01/13] PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros
2021-10-01 19:58 ` [PATCH 01/13] PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros Marek Behún
@ 2021-10-02 16:05 ` Bjorn Helgaas
2021-10-04 8:43 ` Lorenzo Pieralisi
1 sibling, 0 replies; 47+ messages in thread
From: Bjorn Helgaas @ 2021-10-02 16:05 UTC (permalink / raw)
To: Marek Behún
Cc: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Fri, Oct 01, 2021 at 09:58:44PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
>
> Define a macro PCI_EXP_DEVCTL_PAYLOAD_* for every possible Max Payload
> Size in linux/pci_regs.h, in the same style as PCI_EXP_DEVCTL_READRQ_*.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> include/uapi/linux/pci_regs.h | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e709ae8235e7..ff6ccbc6efe9 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -504,6 +504,12 @@
> #define PCI_EXP_DEVCTL_URRE 0x0008 /* Unsupported Request Reporting En. */
> #define PCI_EXP_DEVCTL_RELAX_EN 0x0010 /* Enable relaxed ordering */
> #define PCI_EXP_DEVCTL_PAYLOAD 0x00e0 /* Max_Payload_Size */
> +#define PCI_EXP_DEVCTL_PAYLOAD_128B 0x0000 /* 128 Bytes */
> +#define PCI_EXP_DEVCTL_PAYLOAD_256B 0x0020 /* 256 Bytes */
> +#define PCI_EXP_DEVCTL_PAYLOAD_512B 0x0040 /* 512 Bytes */
> +#define PCI_EXP_DEVCTL_PAYLOAD_1024B 0x0060 /* 1024 Bytes */
> +#define PCI_EXP_DEVCTL_PAYLOAD_2048B 0x0080 /* 2048 Bytes */
> +#define PCI_EXP_DEVCTL_PAYLOAD_4096B 0x00a0 /* 4096 Bytes */
> #define PCI_EXP_DEVCTL_EXT_TAG 0x0100 /* Extended Tag Field Enable */
> #define PCI_EXP_DEVCTL_PHANTOM 0x0200 /* Phantom Functions Enable */
> #define PCI_EXP_DEVCTL_AUX_PME 0x0400 /* Auxiliary Power PM Enable */
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response
2021-10-01 19:58 ` [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response Marek Behún
@ 2021-10-02 16:35 ` Bjorn Helgaas
2021-10-04 7:21 ` Marek Behún
0 siblings, 1 reply; 47+ messages in thread
From: Bjorn Helgaas @ 2021-10-02 16:35 UTC (permalink / raw)
To: Marek Behún
Cc: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Fri, Oct 01, 2021 at 09:58:52PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
>
> Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
> handling of CRS response and when CRSSVE flag was not enabled it marked CRS
> response as failed transaction (due to simplicity).
>
> But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
> for PIO config response and so we can with a small change implement
> re-issuing of config requests as described in PCIe base specification.
>
> This change implements re-issuing of config requests when response is CRS.
> Set upper bound of wait cycles to around PIO_RETRY_CNT, afterwards the
> transaction is marked as failed and an all-ones value is returned as
> before.
Does this fix a problem?
> We do this by returning appropriate error codes from function
> advk_pcie_check_pio_status(). On CRS we return -EAGAIN and caller then
> reissues transaction.
>
> Fixes: 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
> drivers/pci/controller/pci-aardvark.c | 67 +++++++++++++++++----------
> 1 file changed, 43 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 51d2955d9cca..7b9870d0b81f 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -603,6 +603,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
> u32 reg;
> unsigned int status;
> char *strcomp_status, *str_posted;
> + int ret;
>
> reg = advk_readl(pcie, PIO_STAT);
> status = (reg & PIO_COMPLETION_STATUS_MASK) >>
> @@ -627,6 +628,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
> case PIO_COMPLETION_STATUS_OK:
> if (reg & PIO_ERR_STATUS) {
> strcomp_status = "COMP_ERR";
> + ret = -EFAULT;
> break;
> }
> /* Get the read result */
> @@ -634,9 +636,11 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
> *val = advk_readl(pcie, PIO_RD_DATA);
> /* No error */
> strcomp_status = NULL;
> + ret = 0;
> break;
> case PIO_COMPLETION_STATUS_UR:
> strcomp_status = "UR";
> + ret = -EOPNOTSUPP;
> break;
> case PIO_COMPLETION_STATUS_CRS:
> if (allow_crs && val) {
> @@ -654,6 +658,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
> */
> *val = CFG_RD_CRS_VAL;
> strcomp_status = NULL;
> + ret = 0;
> break;
> }
> /* PCIe r4.0, sec 2.3.2, says:
> @@ -669,21 +674,24 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
> * Request and taking appropriate action, e.g., complete the
> * Request to the host as a failed transaction.
> *
> - * To simplify implementation do not re-issue the Configuration
> - * Request and complete the Request as a failed transaction.
> + * So return -EAGAIN and caller (pci-aardvark.c driver) will
> + * re-issue request again up to the PIO_RETRY_CNT retries.
> */
> strcomp_status = "CRS";
> + ret = -EAGAIN;
> break;
> case PIO_COMPLETION_STATUS_CA:
> strcomp_status = "CA";
> + ret = -ECANCELED;
> break;
> default:
> strcomp_status = "Unknown";
> + ret = -EINVAL;
> break;
> }
>
> if (!strcomp_status)
> - return 0;
> + return ret;
>
> if (reg & PIO_NON_POSTED_REQ)
> str_posted = "Non-posted";
> @@ -693,7 +701,7 @@ static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u3
> dev_dbg(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
> str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
>
> - return -EFAULT;
> + return ret;
> }
>
> static int advk_pcie_wait_pio(struct advk_pcie *pcie)
> @@ -701,13 +709,13 @@ static int advk_pcie_wait_pio(struct advk_pcie *pcie)
> struct device *dev = &pcie->pdev->dev;
> int i;
>
> - for (i = 0; i < PIO_RETRY_CNT; i++) {
> + for (i = 1; i <= PIO_RETRY_CNT; i++) {
> u32 start, isr;
>
> start = advk_readl(pcie, PIO_START);
> isr = advk_readl(pcie, PIO_ISR);
> if (!start && isr)
> - return 0;
> + return i;
> udelay(PIO_RETRY_DELAY);
> }
>
> @@ -898,6 +906,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> int where, int size, u32 *val)
> {
> struct advk_pcie *pcie = bus->sysdata;
> + int retry_count;
> bool allow_crs;
> u32 reg;
> int ret;
> @@ -940,16 +949,22 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> /* Program the data strobe */
> advk_writel(pcie, 0xf, PIO_WR_DATA_STRB);
>
> - /* Clear PIO DONE ISR and start the transfer */
> - advk_writel(pcie, 1, PIO_ISR);
> - advk_writel(pcie, 1, PIO_START);
> + retry_count = 0;
> + do {
> + /* Clear PIO DONE ISR and start the transfer */
> + advk_writel(pcie, 1, PIO_ISR);
> + advk_writel(pcie, 1, PIO_START);
>
> - ret = advk_pcie_wait_pio(pcie);
> - if (ret < 0)
> - goto try_crs;
> + ret = advk_pcie_wait_pio(pcie);
> + if (ret < 0)
> + goto try_crs;
> +
> + retry_count += ret;
> +
> + /* Check PIO status and get the read result */
> + ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
> + } while (ret == -EAGAIN && retry_count < PIO_RETRY_CNT);
>
> - /* Check PIO status and get the read result */
> - ret = advk_pcie_check_pio_status(pcie, allow_crs, val);
> if (ret < 0)
> goto fail;
>
> @@ -981,6 +996,7 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> struct advk_pcie *pcie = bus->sysdata;
> u32 reg;
> u32 data_strobe = 0x0;
> + int retry_count;
> int offset;
> int ret;
>
> @@ -1022,19 +1038,22 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> /* Program the data strobe */
> advk_writel(pcie, data_strobe, PIO_WR_DATA_STRB);
>
> - /* Clear PIO DONE ISR and start the transfer */
> - advk_writel(pcie, 1, PIO_ISR);
> - advk_writel(pcie, 1, PIO_START);
> + retry_count = 0;
> + do {
> + /* Clear PIO DONE ISR and start the transfer */
> + advk_writel(pcie, 1, PIO_ISR);
> + advk_writel(pcie, 1, PIO_START);
>
> - ret = advk_pcie_wait_pio(pcie);
> - if (ret < 0)
> - return PCIBIOS_SET_FAILED;
> + ret = advk_pcie_wait_pio(pcie);
> + if (ret < 0)
> + return PCIBIOS_SET_FAILED;
>
> - ret = advk_pcie_check_pio_status(pcie, false, NULL);
> - if (ret < 0)
> - return PCIBIOS_SET_FAILED;
> + retry_count += ret;
>
> - return PCIBIOS_SUCCESSFUL;
> + ret = advk_pcie_check_pio_status(pcie, false, NULL);
> + } while (ret == -EAGAIN && retry_count < PIO_RETRY_CNT);
> +
> + return ret < 0 ? PCIBIOS_SET_FAILED : PCIBIOS_SUCCESSFUL;
> }
>
> static struct pci_ops advk_pcie_ops = {
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 02/13] PCI: aardvark: Fix PCIe Max Payload Size setting
2021-10-01 19:58 ` [PATCH 02/13] PCI: aardvark: Fix PCIe Max Payload Size setting Marek Behún
@ 2021-10-03 17:44 ` Marek Behún
0 siblings, 0 replies; 47+ messages in thread
From: Marek Behún @ 2021-10-03 17:44 UTC (permalink / raw)
To: stable
Cc: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
Greg, Sasha,
I accidentally sent these patches to stable even though they are not
merged in upstream yet (by not configuring git-send-email correctly).
Sorry about that, please ignore this series in stable for now.
Marek
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response
2021-10-02 16:35 ` Bjorn Helgaas
@ 2021-10-04 7:21 ` Marek Behún
2021-10-04 8:53 ` Lorenzo Pieralisi
0 siblings, 1 reply; 47+ messages in thread
From: Marek Behún @ 2021-10-04 7:21 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Sat, 2 Oct 2021 11:35:19 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Oct 01, 2021 at 09:58:52PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> >
> > Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
> > handling of CRS response and when CRSSVE flag was not enabled it marked CRS
> > response as failed transaction (due to simplicity).
> >
> > But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
> > for PIO config response and so we can with a small change implement
> > re-issuing of config requests as described in PCIe base specification.
> >
> > This change implements re-issuing of config requests when response is CRS.
> > Set upper bound of wait cycles to around PIO_RETRY_CNT, afterwards the
> > transaction is marked as failed and an all-ones value is returned as
> > before.
>
> Does this fix a problem?
Hello Bjorn,
Pali has suspisions that certain Marvell WiFi cards may need this [1]
to work, but we do not have them so we cannot test this.
I guess you think this should be considered a new feature instead of a
fix, so that the Fixes tag should be removed, yes? Pali was of the
opinion that this patch "fixes" the driver to conform more to the PCIe
specification, that is why we added the Fixes tag.
Anyway if you think this should be considered a new feature, this patch
can be skipped. The following patches apply even without it.
Marek
[1]
https://lore.kernel.org/linux-wireless/CAHp75Vd5iCLELx8s+Zvcj8ufd2bN6CK26soDMkZyC1CwMO2Qeg@mail.gmail.com/
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 01/13] PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros
2021-10-01 19:58 ` [PATCH 01/13] PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros Marek Behún
2021-10-02 16:05 ` Bjorn Helgaas
@ 2021-10-04 8:43 ` Lorenzo Pieralisi
2021-10-04 9:32 ` Marek Behún
1 sibling, 1 reply; 47+ messages in thread
From: Lorenzo Pieralisi @ 2021-10-04 8:43 UTC (permalink / raw)
To: Marek Behún, Bjorn Helgaas; +Cc: Thomas Petazzoni, linux-pci, pali
On Fri, Oct 01, 2021 at 09:58:44PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
>
> Define a macro PCI_EXP_DEVCTL_PAYLOAD_* for every possible Max Payload
> Size in linux/pci_regs.h, in the same style as PCI_EXP_DEVCTL_READRQ_*.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
> include/uapi/linux/pci_regs.h | 6 ++++++
> 1 file changed, 6 insertions(+)
This requires Bjorn's ACK.
Thanks,
Lorenzo
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e709ae8235e7..ff6ccbc6efe9 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -504,6 +504,12 @@
> #define PCI_EXP_DEVCTL_URRE 0x0008 /* Unsupported Request Reporting En. */
> #define PCI_EXP_DEVCTL_RELAX_EN 0x0010 /* Enable relaxed ordering */
> #define PCI_EXP_DEVCTL_PAYLOAD 0x00e0 /* Max_Payload_Size */
> +#define PCI_EXP_DEVCTL_PAYLOAD_128B 0x0000 /* 128 Bytes */
> +#define PCI_EXP_DEVCTL_PAYLOAD_256B 0x0020 /* 256 Bytes */
> +#define PCI_EXP_DEVCTL_PAYLOAD_512B 0x0040 /* 512 Bytes */
> +#define PCI_EXP_DEVCTL_PAYLOAD_1024B 0x0060 /* 1024 Bytes */
> +#define PCI_EXP_DEVCTL_PAYLOAD_2048B 0x0080 /* 2048 Bytes */
> +#define PCI_EXP_DEVCTL_PAYLOAD_4096B 0x00a0 /* 4096 Bytes */
> #define PCI_EXP_DEVCTL_EXT_TAG 0x0100 /* Extended Tag Field Enable */
> #define PCI_EXP_DEVCTL_PHANTOM 0x0200 /* Phantom Functions Enable */
> #define PCI_EXP_DEVCTL_AUX_PME 0x0400 /* Auxiliary Power PM Enable */
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response
2021-10-04 7:21 ` Marek Behún
@ 2021-10-04 8:53 ` Lorenzo Pieralisi
2021-10-04 10:19 ` Marek Behún
0 siblings, 1 reply; 47+ messages in thread
From: Lorenzo Pieralisi @ 2021-10-04 8:53 UTC (permalink / raw)
To: Marek Behún
Cc: Bjorn Helgaas, Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Mon, Oct 04, 2021 at 09:21:48AM +0200, Marek Behún wrote:
> On Sat, 2 Oct 2021 11:35:19 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> > On Fri, Oct 01, 2021 at 09:58:52PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > >
> > > Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
> > > handling of CRS response and when CRSSVE flag was not enabled it marked CRS
> > > response as failed transaction (due to simplicity).
> > >
> > > But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
> > > for PIO config response and so we can with a small change implement
> > > re-issuing of config requests as described in PCIe base specification.
> > >
> > > This change implements re-issuing of config requests when response is CRS.
> > > Set upper bound of wait cycles to around PIO_RETRY_CNT, afterwards the
> > > transaction is marked as failed and an all-ones value is returned as
> > > before.
> >
> > Does this fix a problem?
>
> Hello Bjorn,
>
> Pali has suspisions that certain Marvell WiFi cards may need this [1]
> to work, but we do not have them so we cannot test this.
>
> I guess you think this should be considered a new feature instead of a
> fix, so that the Fixes tag should be removed, yes? Pali was of the
> opinion that this patch "fixes" the driver to conform more to the PCIe
> specification, that is why we added the Fixes tag.
>
> Anyway if you think this should be considered a new feature, this patch
> can be skipped. The following patches apply even without it.
I do not think we should apply to the mainline a fix that can't be
tested sorry, I will skip this patch.
Thanks,
Lorenzo
> Marek
>
> [1]
> https://lore.kernel.org/linux-wireless/CAHp75Vd5iCLELx8s+Zvcj8ufd2bN6CK26soDMkZyC1CwMO2Qeg@mail.gmail.com/
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 01/13] PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros
2021-10-04 8:43 ` Lorenzo Pieralisi
@ 2021-10-04 9:32 ` Marek Behún
2021-10-04 9:35 ` Lorenzo Pieralisi
0 siblings, 1 reply; 47+ messages in thread
From: Marek Behún @ 2021-10-04 9:32 UTC (permalink / raw)
To: Lorenzo Pieralisi; +Cc: Bjorn Helgaas, Thomas Petazzoni, linux-pci, pali
On Mon, 4 Oct 2021 09:43:50 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Fri, Oct 01, 2021 at 09:58:44PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> >
> > Define a macro PCI_EXP_DEVCTL_PAYLOAD_* for every possible Max Payload
> > Size in linux/pci_regs.h, in the same style as PCI_EXP_DEVCTL_READRQ_*.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <kabel@kernel.org>
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> > include/uapi/linux/pci_regs.h | 6 ++++++
> > 1 file changed, 6 insertions(+)
>
> This requires Bjorn's ACK.
Lorenzo, Bjorn already sent his Reviewed-by for this patch. Did you not
receive it?
Marek
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 01/13] PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros
2021-10-04 9:32 ` Marek Behún
@ 2021-10-04 9:35 ` Lorenzo Pieralisi
0 siblings, 0 replies; 47+ messages in thread
From: Lorenzo Pieralisi @ 2021-10-04 9:35 UTC (permalink / raw)
To: Marek Behún; +Cc: Bjorn Helgaas, Thomas Petazzoni, linux-pci, pali
On Mon, Oct 04, 2021 at 11:32:51AM +0200, Marek Behún wrote:
> On Mon, 4 Oct 2021 09:43:50 +0100
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>
> > On Fri, Oct 01, 2021 at 09:58:44PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > >
> > > Define a macro PCI_EXP_DEVCTL_PAYLOAD_* for every possible Max Payload
> > > Size in linux/pci_regs.h, in the same style as PCI_EXP_DEVCTL_READRQ_*.
> > >
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > ---
> > > include/uapi/linux/pci_regs.h | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> >
> > This requires Bjorn's ACK.
>
> Lorenzo, Bjorn already sent his Reviewed-by for this patch. Did you not
> receive it?
Apologies - missed it, nothing to do then.
Lorenzo
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 10/13] PCI: aardvark: Simplify initialization of rootcap on virtual bridge
2021-10-01 19:58 ` [PATCH 10/13] PCI: aardvark: Simplify initialization of rootcap on virtual bridge Marek Behún
@ 2021-10-04 9:44 ` Lorenzo Pieralisi
2021-10-04 9:56 ` Marek Behún
0 siblings, 1 reply; 47+ messages in thread
From: Lorenzo Pieralisi @ 2021-10-04 9:44 UTC (permalink / raw)
To: Marek Behún; +Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Fri, Oct 01, 2021 at 09:58:53PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
>
> PCIe config space can be initialized also before pci_bridge_emul_init()
> call, so move rootcap initialization after PCI config space initialization.
>
> This simplifies the function a little since it removes one if (ret < 0)
> check.
>
> Fixes: 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value")
Is this a fix ? If not I will remove this tag.
Lorenzo
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
> drivers/pci/controller/pci-aardvark.c | 14 ++++----------
> 1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 7b9870d0b81f..74d1ec7eff16 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -822,7 +822,6 @@ static struct pci_bridge_emul_ops advk_pci_bridge_emul_ops = {
> static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> {
> struct pci_bridge_emul *bridge = &pcie->bridge;
> - int ret;
>
> bridge->conf.vendor =
> cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff);
> @@ -842,19 +841,14 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> /* Support interrupt A for MSI feature */
> bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
>
> + /* Indicates supports for Completion Retry Status */
> + bridge->pcie_conf.rootcap = cpu_to_le16(PCI_EXP_RTCAP_CRSVIS);
> +
> bridge->has_pcie = true;
> bridge->data = pcie;
> bridge->ops = &advk_pci_bridge_emul_ops;
>
> - /* PCIe config space can be initialized after pci_bridge_emul_init() */
> - ret = pci_bridge_emul_init(bridge, 0);
> - if (ret < 0)
> - return ret;
> -
> - /* Indicates supports for Completion Retry Status */
> - bridge->pcie_conf.rootcap = cpu_to_le16(PCI_EXP_RTCAP_CRSVIS);
> -
> - return 0;
> + return pci_bridge_emul_init(bridge, 0);
> }
>
> static bool advk_pcie_valid_device(struct advk_pcie *pcie, struct pci_bus *bus,
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 00/13] PCI: aardvark controller fixes
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
` (12 preceding siblings ...)
2021-10-01 19:58 ` [PATCH 13/13] PCI: aardvark: Fix reporting Data Link Layer Link Active Marek Behún
@ 2021-10-04 9:53 ` Lorenzo Pieralisi
2021-10-04 10:40 ` Marek Behún
13 siblings, 1 reply; 47+ messages in thread
From: Lorenzo Pieralisi @ 2021-10-04 9:53 UTC (permalink / raw)
To: Marek Behún; +Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Fri, Oct 01, 2021 at 09:58:43PM +0200, Marek Behún wrote:
> Hi Lorenzo,
>
> this series fixes some issues with the Aardvark PCIe controller driver.
>
> Most of them are small changes. Patch 11 has a rather long commit message
> since it explains how the bugs were introduced from multiple misleading
> names and comments of some registers.
>
> We have another 56 fixes for aardvark, but last time nobody wanted to
> review such a large series, so we are now trying in smaller batches.
>
> It would be great if you could find time to review, since we would like
> this to land in 5.16. Preferably we would like to send another batch for
> 5.16, but we will see how fast this one goes
>
> Marek & Pali
OK, so what's the overlap between this series and:
https://patchwork.kernel.org/user/todo/linux-pci/?series=506773
https://patchwork.kernel.org/user/todo/linux-pci/?series=507035
I need to keep track of reviews in the series above and make
sure they are reflected into *this* series if some of the
patches overlap or they are a rework.
Please let me know, thank you.
Lorenzo
> Marek Behún (2):
> PCI: aardvark: Don't spam about PIO Response Status
> PCI: aardvark: Deduplicate code in advk_pcie_rd_conf()
>
> Pali Rohár (11):
> PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros
> PCI: aardvark: Fix PCIe Max Payload Size setting
> PCI: aardvark: Fix preserving PCI_EXP_RTCTL_CRSSVE flag on emulated
> bridge
> PCI: aardvark: Fix configuring Reference clock
> PCI: aardvark: Do not clear status bits of masked interrupts
> PCI: aardvark: Do not unmask unused interrupts
> PCI: aardvark: Implement re-issuing config requests on CRS response
> PCI: aardvark: Simplify initialization of rootcap on virtual bridge
> PCI: aardvark: Fix link training
> PCI: aardvark: Fix checking for link up via LTSSM state
> PCI: aardvark: Fix reporting Data Link Layer Link Active
>
> drivers/pci/controller/pci-aardvark.c | 364 +++++++++++++++-----------
> include/uapi/linux/pci_regs.h | 6 +
> 2 files changed, 212 insertions(+), 158 deletions(-)
>
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 10/13] PCI: aardvark: Simplify initialization of rootcap on virtual bridge
2021-10-04 9:44 ` Lorenzo Pieralisi
@ 2021-10-04 9:56 ` Marek Behún
2021-10-04 10:10 ` Lorenzo Pieralisi
0 siblings, 1 reply; 47+ messages in thread
From: Marek Behún @ 2021-10-04 9:56 UTC (permalink / raw)
To: Lorenzo Pieralisi; +Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Mon, 4 Oct 2021 10:44:22 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Fri, Oct 01, 2021 at 09:58:53PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> >
> > PCIe config space can be initialized also before pci_bridge_emul_init()
> > call, so move rootcap initialization after PCI config space initialization.
> >
> > This simplifies the function a little since it removes one if (ret < 0)
> > check.
> >
> > Fixes: 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value")
>
> Is this a fix ? If not I will remove this tag.
It is not a fix, but we put the Fixes patch there because another patch
that is a fix depends on it. But that another patch will come in
another batch, after this was is applied. So maybe we could drop this
patch now.
Or you can remove the Fixes tag, but then we will have to send this by
hand to stable.
Marek
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 10/13] PCI: aardvark: Simplify initialization of rootcap on virtual bridge
2021-10-04 9:56 ` Marek Behún
@ 2021-10-04 10:10 ` Lorenzo Pieralisi
0 siblings, 0 replies; 47+ messages in thread
From: Lorenzo Pieralisi @ 2021-10-04 10:10 UTC (permalink / raw)
To: Marek Behún; +Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Mon, Oct 04, 2021 at 11:56:56AM +0200, Marek Behún wrote:
> On Mon, 4 Oct 2021 10:44:22 +0100
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>
> > On Fri, Oct 01, 2021 at 09:58:53PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > >
> > > PCIe config space can be initialized also before pci_bridge_emul_init()
> > > call, so move rootcap initialization after PCI config space initialization.
> > >
> > > This simplifies the function a little since it removes one if (ret < 0)
> > > check.
> > >
> > > Fixes: 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value")
> >
> > Is this a fix ? If not I will remove this tag.
>
> It is not a fix, but we put the Fixes patch there because another patch
> that is a fix depends on it. But that another patch will come in
> another batch, after this was is applied. So maybe we could drop this
> patch now.
>
> Or you can remove the Fixes tag, but then we will have to send this by
> hand to stable.
It would help me to focus on a series at a time please, I understand
it has been taking a while to get them reviewed and it is complicated
to handle a big patch stack, let's focus on a self-contained series
at a time please.
I'd keep this patch and remove the Fixes: tag, stable dependencies
can be specified if and when required.
Lorenzo
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response
2021-10-04 8:53 ` Lorenzo Pieralisi
@ 2021-10-04 10:19 ` Marek Behún
2021-10-05 19:28 ` Bjorn Helgaas
0 siblings, 1 reply; 47+ messages in thread
From: Marek Behún @ 2021-10-04 10:19 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Bjorn Helgaas, Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Mon, 4 Oct 2021 09:53:35 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Mon, Oct 04, 2021 at 09:21:48AM +0200, Marek Behún wrote:
> > On Sat, 2 Oct 2021 11:35:19 -0500
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > > On Fri, Oct 01, 2021 at 09:58:52PM +0200, Marek Behún wrote:
> > > > From: Pali Rohár <pali@kernel.org>
> > > >
> > > > Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
> > > > handling of CRS response and when CRSSVE flag was not enabled it marked CRS
> > > > response as failed transaction (due to simplicity).
> > > >
> > > > But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
> > > > for PIO config response and so we can with a small change implement
> > > > re-issuing of config requests as described in PCIe base specification.
> > > >
> > > > This change implements re-issuing of config requests when response is CRS.
> > > > Set upper bound of wait cycles to around PIO_RETRY_CNT, afterwards the
> > > > transaction is marked as failed and an all-ones value is returned as
> > > > before.
> > >
> > > Does this fix a problem?
> >
> > Hello Bjorn,
> >
> > Pali has suspisions that certain Marvell WiFi cards may need this [1]
> > to work, but we do not have them so we cannot test this.
> >
> > I guess you think this should be considered a new feature instead of a
> > fix, so that the Fixes tag should be removed, yes? Pali was of the
> > opinion that this patch "fixes" the driver to conform more to the PCIe
> > specification, that is why we added the Fixes tag.
> >
> > Anyway if you think this should be considered a new feature, this patch
> > can be skipped. The following patches apply even without it.
>
> I do not think we should apply to the mainline a fix that can't be
> tested sorry, I will skip this patch.
Lorenzo,
my explanation was incorrect.
The functionality added by this patch _is_ tested and correctly does a
retry: it was done by simulating a CRS reply.
We just don't know whether there are real cards used by users that
need this functionality (the mentioned Marvell card may be such a card).
Marek
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 00/13] PCI: aardvark controller fixes
2021-10-04 9:53 ` [PATCH 00/13] PCI: aardvark controller fixes Lorenzo Pieralisi
@ 2021-10-04 10:40 ` Marek Behún
0 siblings, 0 replies; 47+ messages in thread
From: Marek Behún @ 2021-10-04 10:40 UTC (permalink / raw)
To: Lorenzo Pieralisi; +Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
> OK, so what's the overlap between this series and:
>
> https://patchwork.kernel.org/user/todo/linux-pci/?series=506773
>
> https://patchwork.kernel.org/user/todo/linux-pci/?series=507035
Lorenzo, both those patch series are superseded by this one and the
following I plan to send once this one is applied (since I wanted to
avoid sending 60+ patches in one go).
(From the first series you applied one patch:
PCI: aardvark: Implement workaround for the readback value of VEND_ID
Another patch
PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros
got an Ack from Bjorn, but was not applied, so we sent it again (the
1st patch in this series). Now we have also Bjorn's Reviewed-by for
that patch.)
Regarding links to patchwork, I am unable to see patches not delegated
to me (I can't remove delegate=kabel filter), so I can't set their
status to Superseded myself.
> I need to keep track of reviews in the series above and make
> sure they are reflected into *this* series if some of the
> patches overlap or they are a rework.
>
> Please let me know, thank you.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 12/13] PCI: aardvark: Fix checking for link up via LTSSM state
2021-10-01 19:58 ` [PATCH 12/13] PCI: aardvark: Fix checking for link up via LTSSM state Marek Behún
@ 2021-10-04 13:35 ` Lorenzo Pieralisi
0 siblings, 0 replies; 47+ messages in thread
From: Lorenzo Pieralisi @ 2021-10-04 13:35 UTC (permalink / raw)
To: Marek Behún
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali, Remi Pommarel, stable
On Fri, Oct 01, 2021 at 09:58:55PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
>
> Current implementation of advk_pcie_link_up() is wrong as it marks also
> link disabled or hot reset states as link up.
>
> Fix it by marking link up only to those states which are defined in PCIe
> Base specification 3.0, Table 4-14: Link Status Mapped to the LTSSM.
>
> To simplify implementation, Define macros for every LTSSM state which
> aardvark hardware can return in CFG_REG register.
>
> Fix also checking for link training according to the same Table 4-14.
> Define a new function advk_pcie_link_training() for this purpose.
>
> Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Cc: Remi Pommarel <repk@triplefau.lt>
> Cc: stable@vger.kernel.org
> ---
> drivers/pci/controller/pci-aardvark.c | 69 +++++++++++++++++++++++++--
> 1 file changed, 64 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 5387d9cc3eba..9465b630cede 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -165,7 +165,44 @@
> #define CFG_REG (LMI_BASE_ADDR + 0x0)
> #define LTSSM_SHIFT 24
> #define LTSSM_MASK 0x3f
> +#define LTSSM_DETECT_QUIET 0x0
> +#define LTSSM_DETECT_ACTIVE 0x1
> +#define LTSSM_POLLING_ACTIVE 0x2
> +#define LTSSM_POLLING_COMPLIANCE 0x3
> +#define LTSSM_POLLING_CONFIGURATION 0x4
> +#define LTSSM_CONFIG_LINKWIDTH_START 0x5
> +#define LTSSM_CONFIG_LINKWIDTH_ACCEPT 0x6
> +#define LTSSM_CONFIG_LANENUM_ACCEPT 0x7
> +#define LTSSM_CONFIG_LANENUM_WAIT 0x8
> +#define LTSSM_CONFIG_COMPLETE 0x9
> +#define LTSSM_CONFIG_IDLE 0xa
> +#define LTSSM_RECOVERY_RCVR_LOCK 0xb
> +#define LTSSM_RECOVERY_SPEED 0xc
> +#define LTSSM_RECOVERY_RCVR_CFG 0xd
> +#define LTSSM_RECOVERY_IDLE 0xe
> #define LTSSM_L0 0x10
> +#define LTSSM_RX_L0S_ENTRY 0x11
> +#define LTSSM_RX_L0S_IDLE 0x12
> +#define LTSSM_RX_L0S_FTS 0x13
> +#define LTSSM_TX_L0S_ENTRY 0x14
> +#define LTSSM_TX_L0S_IDLE 0x15
> +#define LTSSM_TX_L0S_FTS 0x16
> +#define LTSSM_L1_ENTRY 0x17
> +#define LTSSM_L1_IDLE 0x18
> +#define LTSSM_L2_IDLE 0x19
> +#define LTSSM_L2_TRANSMIT_WAKE 0x1a
> +#define LTSSM_DISABLED 0x20
> +#define LTSSM_LOOPBACK_ENTRY_MASTER 0x21
> +#define LTSSM_LOOPBACK_ACTIVE_MASTER 0x22
> +#define LTSSM_LOOPBACK_EXIT_MASTER 0x23
> +#define LTSSM_LOOPBACK_ENTRY_SLAVE 0x24
> +#define LTSSM_LOOPBACK_ACTIVE_SLAVE 0x25
> +#define LTSSM_LOOPBACK_EXIT_SLAVE 0x26
> +#define LTSSM_HOT_RESET 0x27
> +#define LTSSM_RECOVERY_EQUALIZATION_PHASE0 0x28
> +#define LTSSM_RECOVERY_EQUALIZATION_PHASE1 0x29
> +#define LTSSM_RECOVERY_EQUALIZATION_PHASE2 0x2a
> +#define LTSSM_RECOVERY_EQUALIZATION_PHASE3 0x2b
An enum would be nicer IMO, for readability. Don't repost the series
yet.
Lorenzo
> #define RC_BAR_CONFIG 0x300
> #define VENDOR_ID_REG (LMI_BASE_ADDR + 0x44)
>
> @@ -258,13 +295,35 @@ static inline u32 advk_readl(struct advk_pcie *pcie, u64 reg)
> return readl(pcie->base + reg);
> }
>
> -static int advk_pcie_link_up(struct advk_pcie *pcie)
> +static u8 advk_pcie_ltssm_state(struct advk_pcie *pcie)
> {
> - u32 val, ltssm_state;
> + u32 val;
> + u8 ltssm_state;
>
> val = advk_readl(pcie, CFG_REG);
> ltssm_state = (val >> LTSSM_SHIFT) & LTSSM_MASK;
> - return ltssm_state >= LTSSM_L0;
> + return ltssm_state;
> +}
> +
> +static inline bool advk_pcie_link_up(struct advk_pcie *pcie)
> +{
> + /* check if LTSSM is in normal operation - some L* state */
> + u8 ltssm_state = advk_pcie_ltssm_state(pcie);
> + return ltssm_state >= LTSSM_L0 && ltssm_state < LTSSM_DISABLED;
> +}
> +
> +static inline bool advk_pcie_link_training(struct advk_pcie *pcie)
> +{
> + /*
> + * According to PCIe Base specification 3.0, Table 4-14: Link
> + * Status Mapped to the LTSSM is Link Training mapped to LTSSM
> + * Configuration and Recovery states.
> + */
> + u8 ltssm_state = advk_pcie_ltssm_state(pcie);
> + return ((ltssm_state >= LTSSM_CONFIG_LINKWIDTH_START &&
> + ltssm_state < LTSSM_L0) ||
> + (ltssm_state >= LTSSM_RECOVERY_EQUALIZATION_PHASE0 &&
> + ltssm_state <= LTSSM_RECOVERY_EQUALIZATION_PHASE3));
> }
>
> static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
> @@ -287,7 +346,7 @@ static void advk_pcie_wait_for_retrain(struct advk_pcie *pcie)
> size_t retries;
>
> for (retries = 0; retries < RETRAIN_WAIT_MAX_RETRIES; ++retries) {
> - if (!advk_pcie_link_up(pcie))
> + if (advk_pcie_link_training(pcie))
> break;
> udelay(RETRAIN_WAIT_USLEEP_US);
> }
> @@ -706,7 +765,7 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
> /* u32 contains both PCI_EXP_LNKCTL and PCI_EXP_LNKSTA */
> u32 val = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + reg) &
> ~(PCI_EXP_LNKSTA_LT << 16);
> - if (!advk_pcie_link_up(pcie))
> + if (advk_pcie_link_training(pcie))
> val |= (PCI_EXP_LNKSTA_LT << 16);
> *value = val;
> return PCI_BRIDGE_EMUL_HANDLED;
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts
2021-10-01 19:58 ` [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts Marek Behún
@ 2021-10-04 14:06 ` Lorenzo Pieralisi
2021-10-04 15:18 ` Marek Behún
2021-10-04 15:31 ` Marc Zyngier
0 siblings, 2 replies; 47+ messages in thread
From: Lorenzo Pieralisi @ 2021-10-04 14:06 UTC (permalink / raw)
To: Marek Behún
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali, stable, maz
[+Marc - always better to have his eyes on IRQ handling code]
On Fri, Oct 01, 2021 at 09:58:49PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
>
> It is incorrect to clear status bits of masked interrupts.
>
> The aardvark driver clears all status interrupt bits if no unmasked
> status bit is set. Masked bits should never be cleared.
>
> Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Cc: stable@vger.kernel.org
> ---
> drivers/pci/controller/pci-aardvark.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index d5d6f92e5143..e4986806a189 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -1295,11 +1295,8 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie)
> isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG);
> isr1_status = isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
>
> - if (!isr0_status && !isr1_status) {
> - advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
> - advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
This looks fine - on the other hand if no interrupt is set in the status
registers (that are filtered with the masks) we are dealing with a
spurious IRQ right ? Just gauging how severe this is.
Lorenzo
> + if (!isr0_status && !isr1_status)
> return;
> - }
>
> /* Process MSI interrupts */
> if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts
2021-10-04 14:06 ` Lorenzo Pieralisi
@ 2021-10-04 15:18 ` Marek Behún
2021-10-04 15:31 ` Marc Zyngier
1 sibling, 0 replies; 47+ messages in thread
From: Marek Behún @ 2021-10-04 15:18 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali, stable, maz
On Mon, 4 Oct 2021 15:06:53 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> [+Marc - always better to have his eyes on IRQ handling code]
>
> > - if (!isr0_status && !isr1_status) {
> > - advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
> > - advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
>
> This looks fine - on the other hand if no interrupt is set in the status
> registers (that are filtered with the masks) we are dealing with a
> spurious IRQ right ? Just gauging how severe this is.
Yes, spurious IRQ can really happen.
Patch 7 in this series fixes an issue where aardvark does not mask all
interrupts, and then kernel can think that an interrupt is masked but
it really isn't.
Also, some interrupts may be masked by the user of the emulated bridge
(some other driver), so that they can be polled. But if we clear all of
them in the status, even the masked ones, then the other driver which
is polling will always get a zero.
Marek
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts
2021-10-04 14:06 ` Lorenzo Pieralisi
2021-10-04 15:18 ` Marek Behún
@ 2021-10-04 15:31 ` Marc Zyngier
2021-10-05 12:13 ` Marek Behún
1 sibling, 1 reply; 47+ messages in thread
From: Marc Zyngier @ 2021-10-04 15:31 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Marek Behún, Thomas Petazzoni, Bjorn Helgaas, linux-pci,
pali, stable
On Mon, 04 Oct 2021 15:06:53 +0100,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>
> [+Marc - always better to have his eyes on IRQ handling code]
>
> On Fri, Oct 01, 2021 at 09:58:49PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> >
> > It is incorrect to clear status bits of masked interrupts.
> >
> > The aardvark driver clears all status interrupt bits if no unmasked
> > status bit is set. Masked bits should never be cleared.
> >
> > Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <kabel@kernel.org>
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > Cc: stable@vger.kernel.org
> > ---
> > drivers/pci/controller/pci-aardvark.c | 5 +----
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index d5d6f92e5143..e4986806a189 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -1295,11 +1295,8 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie)
> > isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG);
> > isr1_status = isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
> >
> > - if (!isr0_status && !isr1_status) {
> > - advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
> > - advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
>
> This looks fine - on the other hand if no interrupt is set in the status
> registers (that are filtered with the masks) we are dealing with a
> spurious IRQ right ? Just gauging how severe this is.
>
> Lorenzo
>
> > + if (!isr0_status && !isr1_status)
> > return;
The whole thing is a bit odd. What the commit message doesn't say is
whether the status register shows the status of the line before
masking, or after masking.
The code seems to imply the former, but then the behaviour is
awkward. How did we end-up here the first place? if that's only a
spurious interrupt, then I'd probably simplify the code altogether,
and drop all the early return code. Something like below, as usual
completely untested.
M.
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 596ebcfcc82d..1d8f257ecb63 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1275,7 +1275,8 @@ static void advk_pcie_handle_msi(struct advk_pcie *pcie)
static void advk_pcie_handle_int(struct advk_pcie *pcie)
{
u32 isr0_val, isr0_mask, isr0_status;
- u32 isr1_val, isr1_mask, isr1_status;
+ u32 isr1_val, isr1_mask;
+ unsigned long isr1_status;
int i;
isr0_val = advk_readl(pcie, PCIE_ISR0_REG);
@@ -1285,22 +1286,14 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie)
isr1_val = advk_readl(pcie, PCIE_ISR1_REG);
isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG);
isr1_status = isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
-
- if (!isr0_status && !isr1_status) {
- advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
- advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
- return;
- }
+ isr1_status >> 8;
/* Process MSI interrupts */
if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
advk_pcie_handle_msi(pcie);
/* Process legacy interrupts */
- for (i = 0; i < PCI_NUM_INTX; i++) {
- if (!(isr1_status & PCIE_ISR1_INTX_ASSERT(i)))
- continue;
-
+ for_each_set_bit(i, &isr1_status, PCI_NUM_INTX) {
advk_writel(pcie, PCIE_ISR1_INTX_ASSERT(i),
PCIE_ISR1_REG);
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts
2021-10-04 15:31 ` Marc Zyngier
@ 2021-10-05 12:13 ` Marek Behún
2021-10-05 12:42 ` Marc Zyngier
0 siblings, 1 reply; 47+ messages in thread
From: Marek Behún @ 2021-10-05 12:13 UTC (permalink / raw)
To: Marc Zyngier
Cc: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, linux-pci,
pali, stable
On Mon, 04 Oct 2021 16:31:54 +0100
Marc Zyngier <maz@kernel.org> wrote:
> On Mon, 04 Oct 2021 15:06:53 +0100,
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> >
> > [+Marc - always better to have his eyes on IRQ handling code]
> >
> > On Fri, Oct 01, 2021 at 09:58:49PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > >
> > > It is incorrect to clear status bits of masked interrupts.
> > >
> > > The aardvark driver clears all status interrupt bits if no
> > > unmasked status bit is set. Masked bits should never be cleared.
> > >
> > > Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host
> > > controller driver") Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > Cc: stable@vger.kernel.org
> > > ---
> > > drivers/pci/controller/pci-aardvark.c | 5 +----
> > > 1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > b/drivers/pci/controller/pci-aardvark.c index
> > > d5d6f92e5143..e4986806a189 100644 ---
> > > a/drivers/pci/controller/pci-aardvark.c +++
> > > b/drivers/pci/controller/pci-aardvark.c @@ -1295,11 +1295,8 @@
> > > static void advk_pcie_handle_int(struct advk_pcie *pcie)
> > > isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG); isr1_status =
> > > isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
> > > - if (!isr0_status && !isr1_status) {
> > > - advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
> > > - advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
> >
> > This looks fine - on the other hand if no interrupt is set in the
> > status registers (that are filtered with the masks) we are dealing
> > with a spurious IRQ right ? Just gauging how severe this is.
> >
> > Lorenzo
> >
> > > + if (!isr0_status && !isr1_status)
> > > return;
>
> The whole thing is a bit odd. What the commit message doesn't say is
> whether the status register shows the status of the line before
> masking, or after masking.
I don't quite understand what you are asking about.
If you are asking about the register itself:
the PCIE_ISR1_REG says which interrupts are currently set / active,
including those which are masked.
If you are asking about the isr1_status variable, it is the
status of the line after masking. I.e. masked interrupts are not set in
this variable, only active interrupts which are also unmasked. That is
obvious from the code.
> The code seems to imply the former, but then the behaviour is
> awkward. How did we end-up here the first place?
I answered this in reply to Lorenzo's comment on this patch, see
https://lore.kernel.org/linux-pci/20211004171823.0288684e@thinkpad/
> if that's only a
> spurious interrupt, then I'd probably simplify the code altogether,
> and drop all the early return code. Something like below, as usual
> completely untested.
>
> M.
>
> diff --git a/drivers/pci/controller/pci-aardvark.c
> b/drivers/pci/controller/pci-aardvark.c index
> 596ebcfcc82d..1d8f257ecb63 100644 ---
> a/drivers/pci/controller/pci-aardvark.c +++
> b/drivers/pci/controller/pci-aardvark.c @@ -1275,7 +1275,8 @@ static
> void advk_pcie_handle_msi(struct advk_pcie *pcie) static void
> advk_pcie_handle_int(struct advk_pcie *pcie) {
> u32 isr0_val, isr0_mask, isr0_status;
> - u32 isr1_val, isr1_mask, isr1_status;
> + u32 isr1_val, isr1_mask;
> + unsigned long isr1_status;
> int i;
>
> isr0_val = advk_readl(pcie, PCIE_ISR0_REG);
> @@ -1285,22 +1286,14 @@ static void advk_pcie_handle_int(struct
> advk_pcie *pcie) isr1_val = advk_readl(pcie, PCIE_ISR1_REG);
> isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG);
> isr1_status = isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
> -
> - if (!isr0_status && !isr1_status) {
> - advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
> - advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
> - return;
> - }
> + isr1_status >> 8;
>
> /* Process MSI interrupts */
> if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> advk_pcie_handle_msi(pcie);
>
> /* Process legacy interrupts */
> - for (i = 0; i < PCI_NUM_INTX; i++) {
> - if (!(isr1_status & PCIE_ISR1_INTX_ASSERT(i)))
> - continue;
> -
> + for_each_set_bit(i, &isr1_status, PCI_NUM_INTX) {
> advk_writel(pcie, PCIE_ISR1_INTX_ASSERT(i),
> PCIE_ISR1_REG);
1. what you are doing here is code cleanup. We are currently in the
state where we have lots of fixes for this driver, which we are
hoping will go also to stable. Some of them depend on these changes.
Can we please first apply those fixes (we want to send them in
batches to avoid sending 60 patchs in one series, since last time
nobody wanted to review all of that) and do this afterwards?
2. you are throwing away lower 8 bits of isr1_status. We have follow-up
patches (not in this series, but in another batch which we want to
send after this) that will be using those lower 8 bits, so we do not
want to throw away them now.
Marek
PS: You can look at all the prepared changes at
https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-aardvark
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts
2021-10-05 12:13 ` Marek Behún
@ 2021-10-05 12:42 ` Marc Zyngier
2021-10-05 13:15 ` Pali Rohár
0 siblings, 1 reply; 47+ messages in thread
From: Marc Zyngier @ 2021-10-05 12:42 UTC (permalink / raw)
To: Marek Behún
Cc: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, linux-pci,
pali, stable
On Tue, 05 Oct 2021 13:13:40 +0100,
Marek Behún <kabel@kernel.org> wrote:
>
> On Mon, 04 Oct 2021 16:31:54 +0100
> Marc Zyngier <maz@kernel.org> wrote:
>
> > On Mon, 04 Oct 2021 15:06:53 +0100,
> > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > >
> > > [+Marc - always better to have his eyes on IRQ handling code]
> > >
> > > On Fri, Oct 01, 2021 at 09:58:49PM +0200, Marek Behún wrote:
> > > > From: Pali Rohár <pali@kernel.org>
> > > >
> > > > It is incorrect to clear status bits of masked interrupts.
> > > >
> > > > The aardvark driver clears all status interrupt bits if no
> > > > unmasked status bit is set. Masked bits should never be cleared.
> > > >
> > > > Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host
> > > > controller driver") Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > > Cc: stable@vger.kernel.org
> > > > ---
> > > > drivers/pci/controller/pci-aardvark.c | 5 +----
> > > > 1 file changed, 1 insertion(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > > b/drivers/pci/controller/pci-aardvark.c index
> > > > d5d6f92e5143..e4986806a189 100644 ---
> > > > a/drivers/pci/controller/pci-aardvark.c +++
> > > > b/drivers/pci/controller/pci-aardvark.c @@ -1295,11 +1295,8 @@
> > > > static void advk_pcie_handle_int(struct advk_pcie *pcie)
> > > > isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG); isr1_status =
> > > > isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
> > > > - if (!isr0_status && !isr1_status) {
> > > > - advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
> > > > - advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
> > >
> > > This looks fine - on the other hand if no interrupt is set in the
> > > status registers (that are filtered with the masks) we are dealing
> > > with a spurious IRQ right ? Just gauging how severe this is.
> > >
> > > Lorenzo
> > >
> > > > + if (!isr0_status && !isr1_status)
> > > > return;
> >
> > The whole thing is a bit odd. What the commit message doesn't say is
> > whether the status register shows the status of the line before
> > masking, or after masking.
>
> I don't quite understand what you are asking about.
> If you are asking about the register itself:
> the PCIE_ISR1_REG says which interrupts are currently set / active,
> including those which are masked.
Then please say so in the commit message.
> If you are asking about the isr1_status variable, it is the
> status of the line after masking. I.e. masked interrupts are not set in
> this variable, only active interrupts which are also unmasked. That is
> obvious from the code.
Which is what I have said... two lines below. If you are going to
reply, please do so in context.
>
> > The code seems to imply the former, but then the behaviour is
> > awkward. How did we end-up here the first place?
>
> I answered this in reply to Lorenzo's comment on this patch, see
> https://lore.kernel.org/linux-pci/20211004171823.0288684e@thinkpad/
It did grace my inbox, thanks.
> > if that's only a
> > spurious interrupt, then I'd probably simplify the code altogether,
> > and drop all the early return code. Something like below, as usual
> > completely untested.
> >
> > M.
> >
> > diff --git a/drivers/pci/controller/pci-aardvark.c
> > b/drivers/pci/controller/pci-aardvark.c index
> > 596ebcfcc82d..1d8f257ecb63 100644 ---
> > a/drivers/pci/controller/pci-aardvark.c +++
> > b/drivers/pci/controller/pci-aardvark.c @@ -1275,7 +1275,8 @@ static
> > void advk_pcie_handle_msi(struct advk_pcie *pcie) static void
> > advk_pcie_handle_int(struct advk_pcie *pcie) {
> > u32 isr0_val, isr0_mask, isr0_status;
> > - u32 isr1_val, isr1_mask, isr1_status;
> > + u32 isr1_val, isr1_mask;
> > + unsigned long isr1_status;
> > int i;
> >
> > isr0_val = advk_readl(pcie, PCIE_ISR0_REG);
> > @@ -1285,22 +1286,14 @@ static void advk_pcie_handle_int(struct
> > advk_pcie *pcie) isr1_val = advk_readl(pcie, PCIE_ISR1_REG);
> > isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG);
> > isr1_status = isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
> > -
> > - if (!isr0_status && !isr1_status) {
> > - advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
> > - advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
> > - return;
> > - }
> > + isr1_status >> 8;
> >
> > /* Process MSI interrupts */
> > if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> > advk_pcie_handle_msi(pcie);
> >
> > /* Process legacy interrupts */
> > - for (i = 0; i < PCI_NUM_INTX; i++) {
> > - if (!(isr1_status & PCIE_ISR1_INTX_ASSERT(i)))
> > - continue;
> > -
> > + for_each_set_bit(i, &isr1_status, PCI_NUM_INTX) {
> > advk_writel(pcie, PCIE_ISR1_INTX_ASSERT(i),
> > PCIE_ISR1_REG);
>
> 1. what you are doing here is code cleanup. We are currently in the
> state where we have lots of fixes for this driver, which we are
> hoping will go also to stable.
Yes, it is code cleanup. Because I don't find this patch to be very
good, TBH. As for going into stable, that's not relevant for this
discussion.
> Some of them depend on these changes.
> Can we please first apply those fixes (we want to send them in
> batches to avoid sending 60 patchs in one series, since last time
> nobody wanted to review all of that) and do this afterwards?
It would be better to start with patches that are in a better
shape. After all, this is what the code review process is about. This
isn't "just take my patches".
> 2. you are throwing away lower 8 bits of isr1_status. We have follow-up
> patches (not in this series, but in another batch which we want to
> send after this) that will be using those lower 8 bits, so we do not
> want to throw away them now.
I'm discarding these bits because *in isolation*, that's the correct
thing to do. Feel free to propose a better patch that doesn't discard
these bits and still makes the code more palatable.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts
2021-10-05 12:42 ` Marc Zyngier
@ 2021-10-05 13:15 ` Pali Rohár
2021-10-05 15:42 ` Marc Zyngier
2021-10-05 16:04 ` Lorenzo Pieralisi
0 siblings, 2 replies; 47+ messages in thread
From: Pali Rohár @ 2021-10-05 13:15 UTC (permalink / raw)
To: Marc Zyngier
Cc: Marek Behún, Lorenzo Pieralisi, Thomas Petazzoni,
Bjorn Helgaas, linux-pci, stable
On Tuesday 05 October 2021 13:42:02 Marc Zyngier wrote:
> On Tue, 05 Oct 2021 13:13:40 +0100,
> Marek Behún <kabel@kernel.org> wrote:
> >
> > On Mon, 04 Oct 2021 16:31:54 +0100
> > Marc Zyngier <maz@kernel.org> wrote:
> >
> > > On Mon, 04 Oct 2021 15:06:53 +0100,
> > > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > > >
> > > > [+Marc - always better to have his eyes on IRQ handling code]
> > > >
> > > > On Fri, Oct 01, 2021 at 09:58:49PM +0200, Marek Behún wrote:
> > > > > From: Pali Rohár <pali@kernel.org>
> > > > >
> > > > > It is incorrect to clear status bits of masked interrupts.
> > > > >
> > > > > The aardvark driver clears all status interrupt bits if no
> > > > > unmasked status bit is set. Masked bits should never be cleared.
> > > > >
> > > > > Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host
> > > > > controller driver") Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > > > Cc: stable@vger.kernel.org
> > > > > ---
> > > > > drivers/pci/controller/pci-aardvark.c | 5 +----
> > > > > 1 file changed, 1 insertion(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > > > b/drivers/pci/controller/pci-aardvark.c index
> > > > > d5d6f92e5143..e4986806a189 100644 ---
> > > > > a/drivers/pci/controller/pci-aardvark.c +++
> > > > > b/drivers/pci/controller/pci-aardvark.c @@ -1295,11 +1295,8 @@
> > > > > static void advk_pcie_handle_int(struct advk_pcie *pcie)
> > > > > isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG); isr1_status =
> > > > > isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
> > > > > - if (!isr0_status && !isr1_status) {
> > > > > - advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
> > > > > - advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
> > > >
> > > > This looks fine - on the other hand if no interrupt is set in the
> > > > status registers (that are filtered with the masks) we are dealing
> > > > with a spurious IRQ right ? Just gauging how severe this is.
> > > >
> > > > Lorenzo
> > > >
> > > > > + if (!isr0_status && !isr1_status)
> > > > > return;
> > >
> > > The whole thing is a bit odd. What the commit message doesn't say is
> > > whether the status register shows the status of the line before
> > > masking, or after masking.
> >
> > I don't quite understand what you are asking about.
> > If you are asking about the register itself:
> > the PCIE_ISR1_REG says which interrupts are currently set / active,
> > including those which are masked.
>
> Then please say so in the commit message.
Very well, we shall do so.
> > If you are asking about the isr1_status variable, it is the
> > status of the line after masking. I.e. masked interrupts are not set in
> > this variable, only active interrupts which are also unmasked. That is
> > obvious from the code.
>
> Which is what I have said... two lines below. If you are going to
> reply, please do so in context.
>
> >
> > > The code seems to imply the former, but then the behaviour is
> > > awkward. How did we end-up here the first place?
> >
> > I answered this in reply to Lorenzo's comment on this patch, see
> > https://lore.kernel.org/linux-pci/20211004171823.0288684e@thinkpad/
>
> It did grace my inbox, thanks.
>
> > > if that's only a
> > > spurious interrupt, then I'd probably simplify the code altogether,
> > > and drop all the early return code. Something like below, as usual
> > > completely untested.
> > >
> > > M.
> > >
> > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > b/drivers/pci/controller/pci-aardvark.c index
> > > 596ebcfcc82d..1d8f257ecb63 100644 ---
> > > a/drivers/pci/controller/pci-aardvark.c +++
> > > b/drivers/pci/controller/pci-aardvark.c @@ -1275,7 +1275,8 @@ static
> > > void advk_pcie_handle_msi(struct advk_pcie *pcie) static void
> > > advk_pcie_handle_int(struct advk_pcie *pcie) {
> > > u32 isr0_val, isr0_mask, isr0_status;
> > > - u32 isr1_val, isr1_mask, isr1_status;
> > > + u32 isr1_val, isr1_mask;
> > > + unsigned long isr1_status;
> > > int i;
> > >
> > > isr0_val = advk_readl(pcie, PCIE_ISR0_REG);
> > > @@ -1285,22 +1286,14 @@ static void advk_pcie_handle_int(struct
> > > advk_pcie *pcie) isr1_val = advk_readl(pcie, PCIE_ISR1_REG);
> > > isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG);
> > > isr1_status = isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
> > > -
> > > - if (!isr0_status && !isr1_status) {
> > > - advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
> > > - advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
> > > - return;
> > > - }
> > > + isr1_status >> 8;
Hello!
I dislike this approach. It adds another magic number which is just
causing issues. Please read commit message for patch 11/13 where we
describe why such magic constants are bad and already caused lot of
issues in this driver.
> > > /* Process MSI interrupts */
> > > if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> > > advk_pcie_handle_msi(pcie);
> > >
> > > /* Process legacy interrupts */
> > > - for (i = 0; i < PCI_NUM_INTX; i++) {
> > > - if (!(isr1_status & PCIE_ISR1_INTX_ASSERT(i)))
> > > - continue;
> > > -
> > > + for_each_set_bit(i, &isr1_status, PCI_NUM_INTX) {
> > > advk_writel(pcie, PCIE_ISR1_INTX_ASSERT(i),
> > > PCIE_ISR1_REG);
> >
> > 1. what you are doing here is code cleanup. We are currently in the
> > state where we have lots of fixes for this driver, which we are
> > hoping will go also to stable.
>
> Yes, it is code cleanup. Because I don't find this patch to be very
> good, TBH. As for going into stable, that's not relevant for this
> discussion.
>
> > Some of them depend on these changes.
> > Can we please first apply those fixes (we want to send them in
> > batches to avoid sending 60 patchs in one series, since last time
> > nobody wanted to review all of that) and do this afterwards?
>
> It would be better to start with patches that are in a better
> shape. After all, this is what the code review process is about. This
> isn't "just take my patches".
>
> > 2. you are throwing away lower 8 bits of isr1_status. We have follow-up
> > patches (not in this series, but in another batch which we want to
> > send after this) that will be using those lower 8 bits, so we do not
> > want to throw away them now.
>
> I'm discarding these bits because *in isolation*, that's the correct
> thing to do. Feel free to propose a better patch that doesn't discard
> these bits and still makes the code more palatable.
The code pattern in this function is: compose irs*_status variable and
then compare it with register macros defined at the top of driver. Each
bit in this register represent some event and for each event there is
simple macro to match.
So with your proposed change it would break all macros (as they are
going to be shifted by magic constant) and then this code disallow
access to events represented by low bits. And also it makes code pattern
different for isr0_status and isr1_status variables which is very
confusing and probably source for introduction of new bugs.
Also the whole early-return optimization can be removed as it does not
change functionality. So we will do so.
But we do not agree with the lower 8 bit discard of the isr1_status
variable as explained above.
So if we add the explanation to commit message and drop the early
return, would it be ok?
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts
2021-10-05 13:15 ` Pali Rohár
@ 2021-10-05 15:42 ` Marc Zyngier
2021-10-05 18:48 ` Pali Rohár
2021-10-05 16:04 ` Lorenzo Pieralisi
1 sibling, 1 reply; 47+ messages in thread
From: Marc Zyngier @ 2021-10-05 15:42 UTC (permalink / raw)
To: Pali Rohár
Cc: Marek Behún, Lorenzo Pieralisi, Thomas Petazzoni,
Bjorn Helgaas, linux-pci, stable
On Tue, 05 Oct 2021 14:15:45 +0100,
Pali Rohár <pali@kernel.org> wrote:
>
> Hello!
>
> I dislike this approach. It adds another magic number which is just
> causing issues. Please read commit message for patch 11/13 where we
> describe why such magic constants are bad and already caused lot of
> issues in this driver.
As I said, feel free to write something better.
>
> > > > /* Process MSI interrupts */
> > > > if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> > > > advk_pcie_handle_msi(pcie);
> > > >
> > > > /* Process legacy interrupts */
> > > > - for (i = 0; i < PCI_NUM_INTX; i++) {
> > > > - if (!(isr1_status & PCIE_ISR1_INTX_ASSERT(i)))
> > > > - continue;
> > > > -
> > > > + for_each_set_bit(i, &isr1_status, PCI_NUM_INTX) {
> > > > advk_writel(pcie, PCIE_ISR1_INTX_ASSERT(i),
> > > > PCIE_ISR1_REG);
> > >
> > > 1. what you are doing here is code cleanup. We are currently in the
> > > state where we have lots of fixes for this driver, which we are
> > > hoping will go also to stable.
> >
> > Yes, it is code cleanup. Because I don't find this patch to be very
> > good, TBH. As for going into stable, that's not relevant for this
> > discussion.
> >
> > > Some of them depend on these changes.
> > > Can we please first apply those fixes (we want to send them in
> > > batches to avoid sending 60 patchs in one series, since last time
> > > nobody wanted to review all of that) and do this afterwards?
> >
> > It would be better to start with patches that are in a better
> > shape. After all, this is what the code review process is about. This
> > isn't "just take my patches".
> >
> > > 2. you are throwing away lower 8 bits of isr1_status. We have follow-up
> > > patches (not in this series, but in another batch which we want to
> > > send after this) that will be using those lower 8 bits, so we do not
> > > want to throw away them now.
> >
> > I'm discarding these bits because *in isolation*, that's the correct
> > thing to do. Feel free to propose a better patch that doesn't discard
> > these bits and still makes the code more palatable.
>
> The code pattern in this function is: compose irs*_status variable and
> then compare it with register macros defined at the top of driver. Each
> bit in this register represent some event and for each event there is
> simple macro to match.
>
> So with your proposed change it would break all macros (as they are
> going to be shifted by magic constant) and then this code disallow
> access to events represented by low bits. And also it makes code pattern
> different for isr0_status and isr1_status variables which is very
> confusing and probably source for introduction of new bugs.
Read what I have said: I'm suggesting changes based on this patch *in
isolation*. I don't see any other related patch in my inbox (nor do I
want to receive any). Feel free to suggest something better (that's
the third time I write this...).
> Also the whole early-return optimization can be removed as it does not
> change functionality. So we will do so.
>
> But we do not agree with the lower 8 bit discard of the isr1_status
> variable as explained above.
>
> So if we add the explanation to commit message and drop the early
> return, would it be ok?
Do what you want. I have no interest in this particular piece of code,
and only replied to Lorenzo's request. It doesn't change what I think
of this patch, but I have too much on my plate to get dragged into
sterile arguments.
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts
2021-10-05 13:15 ` Pali Rohár
2021-10-05 15:42 ` Marc Zyngier
@ 2021-10-05 16:04 ` Lorenzo Pieralisi
1 sibling, 0 replies; 47+ messages in thread
From: Lorenzo Pieralisi @ 2021-10-05 16:04 UTC (permalink / raw)
To: Pali Rohár
Cc: Marc Zyngier, Marek Behún, Thomas Petazzoni, Bjorn Helgaas,
linux-pci, stable
On Tue, Oct 05, 2021 at 03:15:45PM +0200, Pali Rohár wrote:
> On Tuesday 05 October 2021 13:42:02 Marc Zyngier wrote:
> > On Tue, 05 Oct 2021 13:13:40 +0100,
> > Marek Behún <kabel@kernel.org> wrote:
> > >
> > > On Mon, 04 Oct 2021 16:31:54 +0100
> > > Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > > On Mon, 04 Oct 2021 15:06:53 +0100,
> > > > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > > > >
> > > > > [+Marc - always better to have his eyes on IRQ handling code]
> > > > >
> > > > > On Fri, Oct 01, 2021 at 09:58:49PM +0200, Marek Behún wrote:
> > > > > > From: Pali Rohár <pali@kernel.org>
> > > > > >
> > > > > > It is incorrect to clear status bits of masked interrupts.
> > > > > >
> > > > > > The aardvark driver clears all status interrupt bits if no
> > > > > > unmasked status bit is set. Masked bits should never be cleared.
> > > > > >
> > > > > > Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host
> > > > > > controller driver") Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > > > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > > > > Cc: stable@vger.kernel.org
> > > > > > ---
> > > > > > drivers/pci/controller/pci-aardvark.c | 5 +----
> > > > > > 1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > > > > b/drivers/pci/controller/pci-aardvark.c index
> > > > > > d5d6f92e5143..e4986806a189 100644 ---
> > > > > > a/drivers/pci/controller/pci-aardvark.c +++
> > > > > > b/drivers/pci/controller/pci-aardvark.c @@ -1295,11 +1295,8 @@
> > > > > > static void advk_pcie_handle_int(struct advk_pcie *pcie)
> > > > > > isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG); isr1_status =
> > > > > > isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
> > > > > > - if (!isr0_status && !isr1_status) {
> > > > > > - advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
> > > > > > - advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
> > > > >
> > > > > This looks fine - on the other hand if no interrupt is set in the
> > > > > status registers (that are filtered with the masks) we are dealing
> > > > > with a spurious IRQ right ? Just gauging how severe this is.
> > > > >
> > > > > Lorenzo
> > > > >
> > > > > > + if (!isr0_status && !isr1_status)
> > > > > > return;
> > > >
> > > > The whole thing is a bit odd. What the commit message doesn't say is
> > > > whether the status register shows the status of the line before
> > > > masking, or after masking.
> > >
> > > I don't quite understand what you are asking about.
> > > If you are asking about the register itself:
> > > the PCIE_ISR1_REG says which interrupts are currently set / active,
> > > including those which are masked.
> >
> > Then please say so in the commit message.
>
> Very well, we shall do so.
>
> > > If you are asking about the isr1_status variable, it is the
> > > status of the line after masking. I.e. masked interrupts are not set in
> > > this variable, only active interrupts which are also unmasked. That is
> > > obvious from the code.
> >
> > Which is what I have said... two lines below. If you are going to
> > reply, please do so in context.
> >
> > >
> > > > The code seems to imply the former, but then the behaviour is
> > > > awkward. How did we end-up here the first place?
> > >
> > > I answered this in reply to Lorenzo's comment on this patch, see
> > > https://lore.kernel.org/linux-pci/20211004171823.0288684e@thinkpad/
> >
> > It did grace my inbox, thanks.
> >
> > > > if that's only a
> > > > spurious interrupt, then I'd probably simplify the code altogether,
> > > > and drop all the early return code. Something like below, as usual
> > > > completely untested.
> > > >
> > > > M.
> > > >
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > > b/drivers/pci/controller/pci-aardvark.c index
> > > > 596ebcfcc82d..1d8f257ecb63 100644 ---
> > > > a/drivers/pci/controller/pci-aardvark.c +++
> > > > b/drivers/pci/controller/pci-aardvark.c @@ -1275,7 +1275,8 @@ static
> > > > void advk_pcie_handle_msi(struct advk_pcie *pcie) static void
> > > > advk_pcie_handle_int(struct advk_pcie *pcie) {
> > > > u32 isr0_val, isr0_mask, isr0_status;
> > > > - u32 isr1_val, isr1_mask, isr1_status;
> > > > + u32 isr1_val, isr1_mask;
> > > > + unsigned long isr1_status;
> > > > int i;
> > > >
> > > > isr0_val = advk_readl(pcie, PCIE_ISR0_REG);
> > > > @@ -1285,22 +1286,14 @@ static void advk_pcie_handle_int(struct
> > > > advk_pcie *pcie) isr1_val = advk_readl(pcie, PCIE_ISR1_REG);
> > > > isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG);
> > > > isr1_status = isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
> > > > -
> > > > - if (!isr0_status && !isr1_status) {
> > > > - advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
> > > > - advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
> > > > - return;
> > > > - }
> > > > + isr1_status >> 8;
>
> Hello!
>
> I dislike this approach. It adds another magic number which is just
> causing issues. Please read commit message for patch 11/13 where we
> describe why such magic constants are bad and already caused lot of
> issues in this driver.
>
> > > > /* Process MSI interrupts */
> > > > if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> > > > advk_pcie_handle_msi(pcie);
> > > >
> > > > /* Process legacy interrupts */
> > > > - for (i = 0; i < PCI_NUM_INTX; i++) {
> > > > - if (!(isr1_status & PCIE_ISR1_INTX_ASSERT(i)))
> > > > - continue;
> > > > -
> > > > + for_each_set_bit(i, &isr1_status, PCI_NUM_INTX) {
> > > > advk_writel(pcie, PCIE_ISR1_INTX_ASSERT(i),
> > > > PCIE_ISR1_REG);
> > >
> > > 1. what you are doing here is code cleanup. We are currently in the
> > > state where we have lots of fixes for this driver, which we are
> > > hoping will go also to stable.
> >
> > Yes, it is code cleanup. Because I don't find this patch to be very
> > good, TBH. As for going into stable, that's not relevant for this
> > discussion.
> >
> > > Some of them depend on these changes.
> > > Can we please first apply those fixes (we want to send them in
> > > batches to avoid sending 60 patchs in one series, since last time
> > > nobody wanted to review all of that) and do this afterwards?
> >
> > It would be better to start with patches that are in a better
> > shape. After all, this is what the code review process is about. This
> > isn't "just take my patches".
> >
> > > 2. you are throwing away lower 8 bits of isr1_status. We have follow-up
> > > patches (not in this series, but in another batch which we want to
> > > send after this) that will be using those lower 8 bits, so we do not
> > > want to throw away them now.
> >
> > I'm discarding these bits because *in isolation*, that's the correct
> > thing to do. Feel free to propose a better patch that doesn't discard
> > these bits and still makes the code more palatable.
>
> The code pattern in this function is: compose irs*_status variable and
> then compare it with register macros defined at the top of driver. Each
> bit in this register represent some event and for each event there is
> simple macro to match.
>
> So with your proposed change it would break all macros (as they are
> going to be shifted by magic constant) and then this code disallow
> access to events represented by low bits. And also it makes code pattern
> different for isr0_status and isr1_status variables which is very
> confusing and probably source for introduction of new bugs.
>
> Also the whole early-return optimization can be removed as it does not
> change functionality. So we will do so.
>
> But we do not agree with the lower 8 bit discard of the isr1_status
> variable as explained above.
>
> So if we add the explanation to commit message and drop the early
> return, would it be ok?
Please repost a v2 of this series with the review comments you
received (and tags removed if they are not applicable as things
stand in mainline) and we will take it from there.
Thanks,
Lorenzo
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts
2021-10-05 15:42 ` Marc Zyngier
@ 2021-10-05 18:48 ` Pali Rohár
0 siblings, 0 replies; 47+ messages in thread
From: Pali Rohár @ 2021-10-05 18:48 UTC (permalink / raw)
To: Marc Zyngier
Cc: Marek Behún, Lorenzo Pieralisi, Thomas Petazzoni,
Bjorn Helgaas, linux-pci, stable
On Tuesday 05 October 2021 16:42:29 Marc Zyngier wrote:
> As I said, feel free to write something better.
Marek now sent a new version, I hope it is better:
https://lore.kernel.org/linux-pci/20211005180952.6812-7-kabel@kernel.org/
Anyway, I was thinking more if it is possible to end up in state when
*_status variables are zero also after applying patch 7/13.
I do not know how precisely are bits 8-11 of ISR1 reg handled in HW as
description is completely missing in all documentations which I read.
These bits represents events for legacy INTx interrupts.
And maybe it is possible that driver for endpoint card run some function
in context when all CPU interrupts are disabled and do something which
cause card to send Assert_INTA message followed by Deassert_INTA (e.g.
poke some register and immediately process event which deassert intx).
After endpoint driver function finish its execution then CPU receive
PCIe summary interrupt and pci controller driver would see that no event
is pending and noting to process.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response
2021-10-04 10:19 ` Marek Behún
@ 2021-10-05 19:28 ` Bjorn Helgaas
2021-10-05 22:45 ` Marek Behún
2021-10-06 16:29 ` [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response Lorenzo Pieralisi
0 siblings, 2 replies; 47+ messages in thread
From: Bjorn Helgaas @ 2021-10-05 19:28 UTC (permalink / raw)
To: Marek Behún
Cc: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Mon, Oct 04, 2021 at 12:19:38PM +0200, Marek Behún wrote:
> On Mon, 4 Oct 2021 09:53:35 +0100
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > On Mon, Oct 04, 2021 at 09:21:48AM +0200, Marek Behún wrote:
> > > On Sat, 2 Oct 2021 11:35:19 -0500
> > > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > > On Fri, Oct 01, 2021 at 09:58:52PM +0200, Marek Behún wrote:
> > > > > From: Pali Rohár <pali@kernel.org>
> > > > >
> > > > > Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
> > > > > handling of CRS response and when CRSSVE flag was not enabled it marked CRS
> > > > > response as failed transaction (due to simplicity).
> > > > >
> > > > > But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
> > > > > for PIO config response and so we can with a small change implement
> > > > > re-issuing of config requests as described in PCIe base specification.
> > > > >
> > > > > This change implements re-issuing of config requests when response is CRS.
> > > > > Set upper bound of wait cycles to around PIO_RETRY_CNT, afterwards the
> > > > > transaction is marked as failed and an all-ones value is returned as
> > > > > before.
> > > >
> > > > Does this fix a problem?
> > >
> > > Hello Bjorn,
> > >
> > > Pali has suspisions that certain Marvell WiFi cards may need this [1]
> > > to work, but we do not have them so we cannot test this.
> > >
> > > I guess you think this should be considered a new feature instead of a
> > > fix, so that the Fixes tag should be removed, yes? Pali was of the
> > > opinion that this patch "fixes" the driver to conform more to the PCIe
> > > specification, that is why we added the Fixes tag.
> > >
> > > Anyway if you think this should be considered a new feature, this patch
> > > can be skipped. The following patches apply even without it.
> >
> > I do not think we should apply to the mainline a fix that can't be
> > tested sorry, I will skip this patch.
>
> Lorenzo,
>
> my explanation was incorrect.
>
> The functionality added by this patch _is_ tested and correctly does a
> retry: it was done by simulating a CRS reply.
>
> We just don't know whether there are real cards used by users that
> need this functionality (the mentioned Marvell card may be such a card).
My claim is that the spec allows root complexes that retry zero times,
so we must assume such a root complex exists and we cannot rely on any
retries. If such a root complex exists, this patch might fix a
problem, but only for aardvark. It would be better to fix the problem
in a way that works for all PCIe controllers.
I'm playing devil's advocate here, and it's quite possible that I'm
interpreting the spec incorrectly. Maybe the Marvell card is a way to
test this in the real world.
Bjorn
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response
2021-10-05 19:28 ` Bjorn Helgaas
@ 2021-10-05 22:45 ` Marek Behún
2021-10-11 18:15 ` Usage of PCBIOS_* macros (Was: Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response) Pali Rohár
2021-10-06 16:29 ` [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response Lorenzo Pieralisi
1 sibling, 1 reply; 47+ messages in thread
From: Marek Behún @ 2021-10-05 22:45 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Lorenzo Pieralisi, Bjorn Helgaas, linux-pci, pali
On Tue, 5 Oct 2021 14:28:26 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:
> My claim is that the spec allows root complexes that retry zero times,
> so we must assume such a root complex exists and we cannot rely on any
> retries. If such a root complex exists, this patch might fix a
> problem, but only for aardvark. It would be better to fix the problem
> in a way that works for all PCIe controllers.
>
> I'm playing devil's advocate here, and it's quite possible that I'm
> interpreting the spec incorrectly. Maybe the Marvell card is a way to
> test this in the real world.
>
> Bjorn
Hello Bjorn,
this is what I understand from Pali's explanation, please correct me if
something is wrong
- If HW supports CRSSVE bit, OS can ask HW to switch from HW-retry to
SW-retry mode by setting this CRSSVE bit.
- If HW does not support CRSSVE bit, it means that HW supports only
HW-retry.
- By default CRSSVE is disabled, and it is optional, so HW is required
to support HW-retry.
- Linux' PCI core supports handling CRSSVE in probe.c: when HW says it
supports it, PCI core enables it and retries on 0xffff0001 in function
pci_bus_wait_crs().
- Aardvark controller violates specification: it does not support
HW-retry even if it is mandatory. Pali is solving this in his patch by
doing the retry in the driver when CRSSVE is disabled. He is able to do
this because he gets the information about CRS from another channel
(another register).
- You are talking about wanting to implement an abstraction for what
Pali's patch does in PCI core, so that if CRSSVE is not set and someone
reads PCI_VENDOR_ID, you want to make PCI core doing this retry.
Am I correct here?
This could be done by changing Pali's patch so that instead of
retrying, the pci_ops->read() method would instead return a value
indicating that a retry should be done (this would be a new value,
PCIBIOS_CRS), and then in access.c in the pci_bus_read_config_dword()
(and pci_user_read_config_dword()), if the pci_ops->read() method
returns this PCIBIOS_CRS value, the function will retry reading the
register.
Is this what you mean?
It would make sense to do this, if there are other controllers where
HW-retry does not work and instead informs about it via side-channel
even when CRSSVE is disabled.
Marek
PS: Btw, looking at the code, why do we use these PCIBIOS_* macros? And
then sometimes convert them to error codes with pcibios_err_to_errno()?
Is this some legacy thing? Should this be converted to errno?
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response
2021-10-05 19:28 ` Bjorn Helgaas
2021-10-05 22:45 ` Marek Behún
@ 2021-10-06 16:29 ` Lorenzo Pieralisi
2021-10-06 20:13 ` Bjorn Helgaas
1 sibling, 1 reply; 47+ messages in thread
From: Lorenzo Pieralisi @ 2021-10-06 16:29 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Marek Behún, Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Tue, Oct 05, 2021 at 02:28:26PM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 04, 2021 at 12:19:38PM +0200, Marek Beh�n wrote:
> > On Mon, 4 Oct 2021 09:53:35 +0100
> > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > > On Mon, Oct 04, 2021 at 09:21:48AM +0200, Marek Beh�n wrote:
> > > > On Sat, 2 Oct 2021 11:35:19 -0500
> > > > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > >
> > > > > On Fri, Oct 01, 2021 at 09:58:52PM +0200, Marek Beh�n wrote:
> > > > > > From: Pali Roh�r <pali@kernel.org>
> > > > > >
> > > > > > Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
> > > > > > handling of CRS response and when CRSSVE flag was not enabled it marked CRS
> > > > > > response as failed transaction (due to simplicity).
> > > > > >
> > > > > > But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
> > > > > > for PIO config response and so we can with a small change implement
> > > > > > re-issuing of config requests as described in PCIe base specification.
> > > > > >
> > > > > > This change implements re-issuing of config requests when response is CRS.
> > > > > > Set upper bound of wait cycles to around PIO_RETRY_CNT, afterwards the
> > > > > > transaction is marked as failed and an all-ones value is returned as
> > > > > > before.
> > > > >
> > > > > Does this fix a problem?
> > > >
> > > > Hello Bjorn,
> > > >
> > > > Pali has suspisions that certain Marvell WiFi cards may need this [1]
> > > > to work, but we do not have them so we cannot test this.
> > > >
> > > > I guess you think this should be considered a new feature instead of a
> > > > fix, so that the Fixes tag should be removed, yes? Pali was of the
> > > > opinion that this patch "fixes" the driver to conform more to the PCIe
> > > > specification, that is why we added the Fixes tag.
> > > >
> > > > Anyway if you think this should be considered a new feature, this patch
> > > > can be skipped. The following patches apply even without it.
> > >
> > > I do not think we should apply to the mainline a fix that can't be
> > > tested sorry, I will skip this patch.
> >
> > Lorenzo,
> >
> > my explanation was incorrect.
> >
> > The functionality added by this patch _is_ tested and correctly does a
> > retry: it was done by simulating a CRS reply.
> >
> > We just don't know whether there are real cards used by users that
> > need this functionality (the mentioned Marvell card may be such a card).
>
> My claim is that the spec allows root complexes that retry zero times,
> so we must assume such a root complex exists and we cannot rely on any
> retries. If such a root complex exists, this patch might fix a
> problem, but only for aardvark. It would be better to fix the problem
> in a way that works for all PCIe controllers.
We need a way for those PCI controllers to communicate to SW that
they actually received a CRS completion (and that they don't retry
in HW).
By implementing the logic in the aardvark controller that platform
information is there so to the best of my knowledge this patch
is sound.
I assume that the HW retry is in the specs because there is no
architected way if CRS Software Visibility is not enabled/present to
report CRS completion in an architected PCI manner but I just
don't know the entire background behind this.
Lorenzo
> I'm playing devil's advocate here, and it's quite possible that I'm
> interpreting the spec incorrectly. Maybe the Marvell card is a way to
> test this in the real world.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response
2021-10-06 16:29 ` [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response Lorenzo Pieralisi
@ 2021-10-06 20:13 ` Bjorn Helgaas
2021-10-07 11:51 ` Lorenzo Pieralisi
0 siblings, 1 reply; 47+ messages in thread
From: Bjorn Helgaas @ 2021-10-06 20:13 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Marek Behún, Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Wed, Oct 06, 2021 at 05:29:34PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Oct 05, 2021 at 02:28:26PM -0500, Bjorn Helgaas wrote:
> > On Mon, Oct 04, 2021 at 12:19:38PM +0200, Marek Beh�n wrote:
> > > On Mon, 4 Oct 2021 09:53:35 +0100
> > > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > > > On Mon, Oct 04, 2021 at 09:21:48AM +0200, Marek Beh�n wrote:
> > > > > On Sat, 2 Oct 2021 11:35:19 -0500
> > > > > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > >
> > > > > > On Fri, Oct 01, 2021 at 09:58:52PM +0200, Marek Beh�n wrote:
> > > > > > > From: Pali Roh�r <pali@kernel.org>
> > > > > > >
> > > > > > > Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
> > > > > > > handling of CRS response and when CRSSVE flag was not enabled it marked CRS
> > > > > > > response as failed transaction (due to simplicity).
> > > > > > >
> > > > > > > But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
> > > > > > > for PIO config response and so we can with a small change implement
> > > > > > > re-issuing of config requests as described in PCIe base specification.
> > > > > > >
> > > > > > > This change implements re-issuing of config requests when response is CRS.
> > > > > > > Set upper bound of wait cycles to around PIO_RETRY_CNT, afterwards the
> > > > > > > transaction is marked as failed and an all-ones value is returned as
> > > > > > > before.
> > > > > >
> > > > > > Does this fix a problem?
> > > > >
> > > > > Hello Bjorn,
> > > > >
> > > > > Pali has suspisions that certain Marvell WiFi cards may need this [1]
> > > > > to work, but we do not have them so we cannot test this.
> > > > >
> > > > > I guess you think this should be considered a new feature instead of a
> > > > > fix, so that the Fixes tag should be removed, yes? Pali was of the
> > > > > opinion that this patch "fixes" the driver to conform more to the PCIe
> > > > > specification, that is why we added the Fixes tag.
> > > > >
> > > > > Anyway if you think this should be considered a new feature, this patch
> > > > > can be skipped. The following patches apply even without it.
> > > >
> > > > I do not think we should apply to the mainline a fix that can't be
> > > > tested sorry, I will skip this patch.
> > >
> > > Lorenzo,
> > >
> > > my explanation was incorrect.
> > >
> > > The functionality added by this patch _is_ tested and correctly does a
> > > retry: it was done by simulating a CRS reply.
> > >
> > > We just don't know whether there are real cards used by users that
> > > need this functionality (the mentioned Marvell card may be such a card).
> >
> > My claim is that the spec allows root complexes that retry zero times,
> > so we must assume such a root complex exists and we cannot rely on any
> > retries. If such a root complex exists, this patch might fix a
> > problem, but only for aardvark. It would be better to fix the problem
> > in a way that works for all PCIe controllers.
>
> We need a way for those PCI controllers to communicate to SW that
> they actually received a CRS completion (and that they don't retry
> in HW).
AFAICT this would be controller-dependent. I think some controllers
have registers that control the number of retries, but of course
that's completely controller-dependent, too.
> By implementing the logic in the aardvark controller that platform
> information is there so to the best of my knowledge this patch
> is sound.
>
> I assume that the HW retry is in the specs because there is no
> architected way if CRS Software Visibility is not enabled/present to
> report CRS completion in an architected PCI manner but I just
> don't know the entire background behind this.
I assume an error bit would be set in the Status or Secondary Status
register when a PCIe transaction fails. I'm not sure anybody *looks*
at those, though.
This still looks like a PCI controller band-aid for a device or driver
problem that will likely still exist on other platforms.
Bjorn
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response
2021-10-06 20:13 ` Bjorn Helgaas
@ 2021-10-07 11:51 ` Lorenzo Pieralisi
2021-10-07 12:36 ` Marek Behún
0 siblings, 1 reply; 47+ messages in thread
From: Lorenzo Pieralisi @ 2021-10-07 11:51 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Marek Behún, Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Wed, Oct 06, 2021 at 03:13:05PM -0500, Bjorn Helgaas wrote:
[...]
> > We need a way for those PCI controllers to communicate to SW that
> > they actually received a CRS completion (and that they don't retry
> > in HW).
>
> AFAICT this would be controller-dependent. I think some controllers
> have registers that control the number of retries, but of course
> that's completely controller-dependent, too.
>
> > By implementing the logic in the aardvark controller that platform
> > information is there so to the best of my knowledge this patch
> > is sound.
> >
> > I assume that the HW retry is in the specs because there is no
> > architected way if CRS Software Visibility is not enabled/present to
> > report CRS completion in an architected PCI manner but I just
> > don't know the entire background behind this.
>
> I assume an error bit would be set in the Status or Secondary Status
> register when a PCIe transaction fails. I'm not sure anybody *looks*
> at those, though.
>
> This still looks like a PCI controller band-aid for a device or driver
> problem that will likely still exist on other platforms.
Yes that's true. I believe we can merge this patch (?) but it would
also be good if Pali/Marek have time to test on other HW and
maybe generalize the concept.
Lorenzo
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response
2021-10-07 11:51 ` Lorenzo Pieralisi
@ 2021-10-07 12:36 ` Marek Behún
2021-10-07 19:25 ` Bjorn Helgaas
0 siblings, 1 reply; 47+ messages in thread
From: Marek Behún @ 2021-10-07 12:36 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Bjorn Helgaas, Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Thu, 7 Oct 2021 12:51:57 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Wed, Oct 06, 2021 at 03:13:05PM -0500, Bjorn Helgaas wrote:
>
> [...]
>
> > > We need a way for those PCI controllers to communicate to SW that
> > > they actually received a CRS completion (and that they don't retry
> > > in HW).
> >
> > AFAICT this would be controller-dependent. I think some controllers
> > have registers that control the number of retries, but of course
> > that's completely controller-dependent, too.
> >
> > > By implementing the logic in the aardvark controller that platform
> > > information is there so to the best of my knowledge this patch
> > > is sound.
> > >
> > > I assume that the HW retry is in the specs because there is no
> > > architected way if CRS Software Visibility is not enabled/present to
> > > report CRS completion in an architected PCI manner but I just
> > > don't know the entire background behind this.
> >
> > I assume an error bit would be set in the Status or Secondary Status
> > register when a PCIe transaction fails. I'm not sure anybody *looks*
> > at those, though.
> >
> > This still looks like a PCI controller band-aid for a device or driver
> > problem that will likely still exist on other platforms.
>
> Yes that's true. I believe we can merge this patch (?) but it would
> also be good if Pali/Marek have time to test on other HW and
> maybe generalize the concept.
We are willing to try to implement a generic API if you propose should
such API look (at least some hints).
But let's merge this into aardvark for now, since the generic case will
take a non-trivial time to implement.
Marek
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response
2021-10-07 12:36 ` Marek Behún
@ 2021-10-07 19:25 ` Bjorn Helgaas
0 siblings, 0 replies; 47+ messages in thread
From: Bjorn Helgaas @ 2021-10-07 19:25 UTC (permalink / raw)
To: Marek Behún
Cc: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, linux-pci, pali
On Thu, Oct 07, 2021 at 02:36:25PM +0200, Marek Behún wrote:
> On Thu, 7 Oct 2021 12:51:57 +0100
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>
> > On Wed, Oct 06, 2021 at 03:13:05PM -0500, Bjorn Helgaas wrote:
> >
> > [...]
> >
> > > > We need a way for those PCI controllers to communicate to SW that
> > > > they actually received a CRS completion (and that they don't retry
> > > > in HW).
> > >
> > > AFAICT this would be controller-dependent. I think some controllers
> > > have registers that control the number of retries, but of course
> > > that's completely controller-dependent, too.
> > >
> > > > By implementing the logic in the aardvark controller that platform
> > > > information is there so to the best of my knowledge this patch
> > > > is sound.
> > > >
> > > > I assume that the HW retry is in the specs because there is no
> > > > architected way if CRS Software Visibility is not enabled/present to
> > > > report CRS completion in an architected PCI manner but I just
> > > > don't know the entire background behind this.
> > >
> > > I assume an error bit would be set in the Status or Secondary Status
> > > register when a PCIe transaction fails. I'm not sure anybody *looks*
> > > at those, though.
> > >
> > > This still looks like a PCI controller band-aid for a device or driver
> > > problem that will likely still exist on other platforms.
> >
> > Yes that's true. I believe we can merge this patch (?) but it would
> > also be good if Pali/Marek have time to test on other HW and
> > maybe generalize the concept.
>
> We are willing to try to implement a generic API if you propose should
> such API look (at least some hints).
That's the whole point -- CRS SV *is* the generic way for controllers
to report CRS completions to software. If the controller doesn't
support CRS SV, I don't think a generic API is possible.
Or maybe I don't understand the sort of generic API you have in mind.
What sort of information do you envision the API might provide?
Bjorn
^ permalink raw reply [flat|nested] 47+ messages in thread
* Usage of PCBIOS_* macros (Was: Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response)
2021-10-05 22:45 ` Marek Behún
@ 2021-10-11 18:15 ` Pali Rohár
2021-10-11 20:58 ` Bjorn Helgaas
0 siblings, 1 reply; 47+ messages in thread
From: Pali Rohár @ 2021-10-11 18:15 UTC (permalink / raw)
To: Bjorn Helgaas, Lorenzo Pieralisi, Bjorn Helgaas,
Marek Behún, linux-pci
On Wednesday 06 October 2021 00:45:31 Marek Behún wrote:
> PS: Btw, looking at the code, why do we use these PCIBIOS_* macros? And
> then sometimes convert them to error codes with pcibios_err_to_errno()?
> Is this some legacy thing? Should this be converted to errno?
Hello! I would like to remind this Marek's PS. Do you know why config
read and write functions return these PCIBIOS_* macros and not standard
linux errno codes?
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: Usage of PCBIOS_* macros (Was: Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response)
2021-10-11 18:15 ` Usage of PCBIOS_* macros (Was: Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response) Pali Rohár
@ 2021-10-11 20:58 ` Bjorn Helgaas
0 siblings, 0 replies; 47+ messages in thread
From: Bjorn Helgaas @ 2021-10-11 20:58 UTC (permalink / raw)
To: Pali Rohár
Cc: Lorenzo Pieralisi, Bjorn Helgaas, Marek Behún, linux-pci
On Mon, Oct 11, 2021 at 08:15:53PM +0200, Pali Rohár wrote:
> On Wednesday 06 October 2021 00:45:31 Marek Behún wrote:
> > PS: Btw, looking at the code, why do we use these PCIBIOS_* macros? And
> > then sometimes convert them to error codes with pcibios_err_to_errno()?
> > Is this some legacy thing? Should this be converted to errno?
>
> Hello! I would like to remind this Marek's PS. Do you know why config
> read and write functions return these PCIBIOS_* macros and not standard
> linux errno codes?
I don't know. The PCIBIOS_SUCCESSFUL..PCIBIOS_BUFFER_TOO_SMALL codes
are actually still in the latest PCI Firmware Spec, r3.3, of
1/20/2021, sec 2.9, but are clearly x86-specific.
I think it would be nicer if their usage were confined to arch/x86,
but they've leaked out somewhat.
^ permalink raw reply [flat|nested] 47+ messages in thread
end of thread, other threads:[~2021-10-11 20:58 UTC | newest]
Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-01 19:58 [PATCH 00/13] PCI: aardvark controller fixes Marek Behún
2021-10-01 19:58 ` [PATCH 01/13] PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros Marek Behún
2021-10-02 16:05 ` Bjorn Helgaas
2021-10-04 8:43 ` Lorenzo Pieralisi
2021-10-04 9:32 ` Marek Behún
2021-10-04 9:35 ` Lorenzo Pieralisi
2021-10-01 19:58 ` [PATCH 02/13] PCI: aardvark: Fix PCIe Max Payload Size setting Marek Behún
2021-10-03 17:44 ` Marek Behún
2021-10-01 19:58 ` [PATCH 03/13] PCI: aardvark: Don't spam about PIO Response Status Marek Behún
2021-10-01 19:58 ` [PATCH 04/13] PCI: aardvark: Fix preserving PCI_EXP_RTCTL_CRSSVE flag on emulated bridge Marek Behún
2021-10-01 19:58 ` [PATCH 05/13] PCI: aardvark: Fix configuring Reference clock Marek Behún
2021-10-01 19:58 ` [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts Marek Behún
2021-10-04 14:06 ` Lorenzo Pieralisi
2021-10-04 15:18 ` Marek Behún
2021-10-04 15:31 ` Marc Zyngier
2021-10-05 12:13 ` Marek Behún
2021-10-05 12:42 ` Marc Zyngier
2021-10-05 13:15 ` Pali Rohár
2021-10-05 15:42 ` Marc Zyngier
2021-10-05 18:48 ` Pali Rohár
2021-10-05 16:04 ` Lorenzo Pieralisi
2021-10-01 19:58 ` [PATCH 07/13] PCI: aardvark: Do not unmask unused interrupts Marek Behún
2021-10-01 19:58 ` [PATCH 08/13] PCI: aardvark: Deduplicate code in advk_pcie_rd_conf() Marek Behún
2021-10-01 19:58 ` [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response Marek Behún
2021-10-02 16:35 ` Bjorn Helgaas
2021-10-04 7:21 ` Marek Behún
2021-10-04 8:53 ` Lorenzo Pieralisi
2021-10-04 10:19 ` Marek Behún
2021-10-05 19:28 ` Bjorn Helgaas
2021-10-05 22:45 ` Marek Behún
2021-10-11 18:15 ` Usage of PCBIOS_* macros (Was: Re: [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response) Pali Rohár
2021-10-11 20:58 ` Bjorn Helgaas
2021-10-06 16:29 ` [PATCH 09/13] PCI: aardvark: Implement re-issuing config requests on CRS response Lorenzo Pieralisi
2021-10-06 20:13 ` Bjorn Helgaas
2021-10-07 11:51 ` Lorenzo Pieralisi
2021-10-07 12:36 ` Marek Behún
2021-10-07 19:25 ` Bjorn Helgaas
2021-10-01 19:58 ` [PATCH 10/13] PCI: aardvark: Simplify initialization of rootcap on virtual bridge Marek Behún
2021-10-04 9:44 ` Lorenzo Pieralisi
2021-10-04 9:56 ` Marek Behún
2021-10-04 10:10 ` Lorenzo Pieralisi
2021-10-01 19:58 ` [PATCH 11/13] PCI: aardvark: Fix link training Marek Behún
2021-10-01 19:58 ` [PATCH 12/13] PCI: aardvark: Fix checking for link up via LTSSM state Marek Behún
2021-10-04 13:35 ` Lorenzo Pieralisi
2021-10-01 19:58 ` [PATCH 13/13] PCI: aardvark: Fix reporting Data Link Layer Link Active Marek Behún
2021-10-04 9:53 ` [PATCH 00/13] PCI: aardvark controller fixes Lorenzo Pieralisi
2021-10-04 10:40 ` Marek Behún
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).