diff mbox series

[v2,1/2] PCI: aardvark: Add support for AER registers on emulated bridge

Message ID 20220524132827.8837-2-kabel@kernel.org
State New
Headers show
Series PCI: aardvark controller changes BATCH 5 (subset) | expand

Commit Message

Marek Behún May 24, 2022, 1:28 p.m. UTC 
From: Pali Rohár <pali@kernel.org>

Aardvark controller supports Advanced Error Reporting configuration
registers.

Export these registers on the emulated root bridge via the new .read_ext
and .write_ext methods.

Note that in the Advanced Error Reporting Capability header the offset
to the next Extended Capability header is set, but it is not documented
in Armada 3700 Functional Specification. Since this change adds support
only for Advanced Error Reporting, explicitly clear PCI_EXT_CAP_NEXT
bits in AER capability header.

Now the pcieport driver correctly detects AER support and allows PCIe
AER driver to start receiving ERR interrupts. Kernel log now says:

    [    4.358401] pcieport 0000:00:00.0: AER: enabled with IRQ 52

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 drivers/pci/controller/pci-aardvark.c | 77 +++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

Comments

Rob Herring (Arm) May 26, 2022, 8:38 p.m. UTC | #1 
On Tue, May 24, 2022 at 03:28:26PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Aardvark controller supports Advanced Error Reporting configuration
> registers.
> 
> Export these registers on the emulated root bridge via the new .read_ext
> and .write_ext methods.
> 
> Note that in the Advanced Error Reporting Capability header the offset
> to the next Extended Capability header is set, but it is not documented
> in Armada 3700 Functional Specification. Since this change adds support
> only for Advanced Error Reporting, explicitly clear PCI_EXT_CAP_NEXT
> bits in AER capability header.
> 
> Now the pcieport driver correctly detects AER support and allows PCIe
> AER driver to start receiving ERR interrupts. Kernel log now says:
> 
>     [    4.358401] pcieport 0000:00:00.0: AER: enabled with IRQ 52
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

Did you mean Reviewed-by? Signed-off-by is only correct if Lorenzo 
applied or rewrote these. If he applied them, then Bjorn will pick them 
up.

Rob
Marek Behún May 29, 2022, 10:08 a.m. UTC | #2 
On Thu, 26 May 2022 15:38:01 -0500
Rob Herring <robh@kernel.org> wrote:

> On Tue, May 24, 2022 at 03:28:26PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > Aardvark controller supports Advanced Error Reporting configuration
> > registers.
> > 
> > Export these registers on the emulated root bridge via the new .read_ext
> > and .write_ext methods.
> > 
> > Note that in the Advanced Error Reporting Capability header the offset
> > to the next Extended Capability header is set, but it is not documented
> > in Armada 3700 Functional Specification. Since this change adds support
> > only for Advanced Error Reporting, explicitly clear PCI_EXT_CAP_NEXT
> > bits in AER capability header.
> > 
> > Now the pcieport driver correctly detects AER support and allows PCIe
> > AER driver to start receiving ERR interrupts. Kernel log now says:
> > 
> >     [    4.358401] pcieport 0000:00:00.0: AER: enabled with IRQ 52
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>  
> 
> Did you mean Reviewed-by? Signed-off-by is only correct if Lorenzo 
> applied or rewrote these. If he applied them, then Bjorn will pick them 
> up.

Hmm. Well, Lorenzo applied the subset I am sending (patches 3 and 5) to
his tree, with SOB, meaning to send it to Bjorn [1].

Then we discovered that patch 4 is also required for the _SHIFT
macros, which was discussed previously that we want to avoid those, and
use FIELD_PREP() / FIELD_GET() instead [2].

So I updated the second patch to use FIELD_PREP() / FIELD_GET() instead
of the _SHIFT macros. I guess this version isn't SOB by Lorenzo, but
the first version was... I should probably change it to Reviewed-by for
both patches anyway, right?

Marek

[1]
https://lore.kernel.org/linux-arm-kernel/165288925279.7950.90687082853412954.b4-ty@arm.com/

[2]
https://lore.kernel.org/linux-arm-kernel/20220518202729.GA4606@bhelgaas/
Rob Herring (Arm) May 31, 2022, 7:55 p.m. UTC | #3 
On Sun, May 29, 2022 at 12:08:13PM +0200, Marek Behún wrote:
> On Thu, 26 May 2022 15:38:01 -0500
> Rob Herring <robh@kernel.org> wrote:
> 
> > On Tue, May 24, 2022 at 03:28:26PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > > 
> > > Aardvark controller supports Advanced Error Reporting configuration
> > > registers.
> > > 
> > > Export these registers on the emulated root bridge via the new .read_ext
> > > and .write_ext methods.
> > > 
> > > Note that in the Advanced Error Reporting Capability header the offset
> > > to the next Extended Capability header is set, but it is not documented
> > > in Armada 3700 Functional Specification. Since this change adds support
> > > only for Advanced Error Reporting, explicitly clear PCI_EXT_CAP_NEXT
> > > bits in AER capability header.
> > > 
> > > Now the pcieport driver correctly detects AER support and allows PCIe
> > > AER driver to start receiving ERR interrupts. Kernel log now says:
> > > 
> > >     [    4.358401] pcieport 0000:00:00.0: AER: enabled with IRQ 52
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>  
> > 
> > Did you mean Reviewed-by? Signed-off-by is only correct if Lorenzo 
> > applied or rewrote these. If he applied them, then Bjorn will pick them 
> > up.
> 
> Hmm. Well, Lorenzo applied the subset I am sending (patches 3 and 5) to
> his tree, with SOB, meaning to send it to Bjorn [1].
> 
> Then we discovered that patch 4 is also required for the _SHIFT
> macros, which was discussed previously that we want to avoid those, and
> use FIELD_PREP() / FIELD_GET() instead [2].
> 
> So I updated the second patch to use FIELD_PREP() / FIELD_GET() instead
> of the _SHIFT macros. I guess this version isn't SOB by Lorenzo, but
> the first version was... I should probably change it to Reviewed-by for
> both patches anyway, right?

I would suggest you send these without either (unless Lorenzo actually 
gave a Reviewed-by) and just state that Lorenzo applied these, but then 
you had to make another change as you described above.

But if Bjorn applies the original and doesn't want to rebase (he 
usually will rebase if needed), then an incremental patch will be 
needed.

Rob
diff mbox series

Patch

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index ffec82c8a523..d71c9bc95934 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -33,6 +33,7 @@ 
 #define PCIE_CORE_CMD_STATUS_REG				0x4
 #define PCIE_CORE_DEV_REV_REG					0x8
 #define PCIE_CORE_PCIEXP_CAP					0xc0
+#define PCIE_CORE_PCIERR_CAP					0x100
 #define PCIE_CORE_ERR_CAPCTL_REG				0x118
 #define     PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX			BIT(5)
 #define     PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX_EN			BIT(6)
@@ -944,11 +945,87 @@  advk_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge,
 	}
 }
 
+static pci_bridge_emul_read_status_t
+advk_pci_bridge_emul_ext_conf_read(struct pci_bridge_emul *bridge,
+				   int reg, u32 *value)
+{
+	struct advk_pcie *pcie = bridge->data;
+
+	switch (reg) {
+	case 0:
+		*value = advk_readl(pcie, PCIE_CORE_PCIERR_CAP + reg);
+		/*
+		 * PCI_EXT_CAP_NEXT bits are set to offset 0x150, but Armada
+		 * 3700 Functional Specification does not document registers
+		 * at those addresses.
+		 * Thus we clear PCI_EXT_CAP_NEXT bits to make Advanced Error
+		 * Reporting Capability header the last of Extended
+		 * Capabilities. (If we obtain documentation for those
+		 * registers in the future, this can be changed.)
+		 */
+		*value &= 0x000fffff;
+		return PCI_BRIDGE_EMUL_HANDLED;
+
+	case PCI_ERR_UNCOR_STATUS:
+	case PCI_ERR_UNCOR_MASK:
+	case PCI_ERR_UNCOR_SEVER:
+	case PCI_ERR_COR_STATUS:
+	case PCI_ERR_COR_MASK:
+	case PCI_ERR_CAP:
+	case PCI_ERR_HEADER_LOG + 0:
+	case PCI_ERR_HEADER_LOG + 4:
+	case PCI_ERR_HEADER_LOG + 8:
+	case PCI_ERR_HEADER_LOG + 12:
+	case PCI_ERR_ROOT_COMMAND:
+	case PCI_ERR_ROOT_STATUS:
+	case PCI_ERR_ROOT_ERR_SRC:
+		*value = advk_readl(pcie, PCIE_CORE_PCIERR_CAP + reg);
+		return PCI_BRIDGE_EMUL_HANDLED;
+
+	default:
+		return PCI_BRIDGE_EMUL_NOT_HANDLED;
+	}
+}
+
+static void
+advk_pci_bridge_emul_ext_conf_write(struct pci_bridge_emul *bridge,
+				    int reg, u32 old, u32 new, u32 mask)
+{
+	struct advk_pcie *pcie = bridge->data;
+
+	switch (reg) {
+	/* These are W1C registers, so clear other bits */
+	case PCI_ERR_UNCOR_STATUS:
+	case PCI_ERR_COR_STATUS:
+	case PCI_ERR_ROOT_STATUS:
+		new &= mask;
+		fallthrough;
+
+	case PCI_ERR_UNCOR_MASK:
+	case PCI_ERR_UNCOR_SEVER:
+	case PCI_ERR_COR_MASK:
+	case PCI_ERR_CAP:
+	case PCI_ERR_HEADER_LOG + 0:
+	case PCI_ERR_HEADER_LOG + 4:
+	case PCI_ERR_HEADER_LOG + 8:
+	case PCI_ERR_HEADER_LOG + 12:
+	case PCI_ERR_ROOT_COMMAND:
+	case PCI_ERR_ROOT_ERR_SRC:
+		advk_writel(pcie, new, PCIE_CORE_PCIERR_CAP + reg);
+		break;
+
+	default:
+		break;
+	}
+}
+
 static const struct pci_bridge_emul_ops advk_pci_bridge_emul_ops = {
 	.read_base = advk_pci_bridge_emul_base_conf_read,
 	.write_base = advk_pci_bridge_emul_base_conf_write,
 	.read_pcie = advk_pci_bridge_emul_pcie_conf_read,
 	.write_pcie = advk_pci_bridge_emul_pcie_conf_write,
+	.read_ext = advk_pci_bridge_emul_ext_conf_read,
+	.write_ext = advk_pci_bridge_emul_ext_conf_write,
 };
 
 /*