[PATCH v2 0/2] PCI: aardvark controller changes BATCH 5 (subset)

[PATCH v2 0/2] PCI: aardvark controller changes BATCH 5 (subset)

[Marek Behún]

Hello Bjorn,

since Lorenzo is AFK but gave his review for patches 3 and 5 of fifth
batch of Aardvark changes, and you only requested to use FIELD_PREP [1]
in order to avoid adding new _SHIFT macros, I am now sending these two
patches to you. Could we get this merged?

Thanks.

Marek

Changes since v1:
- dropped all patches but 3 and 5
- changed patch 5 to use FIELD_PREP instead of _SHIFT macro

[1] https://lore.kernel.org/linux-arm-kernel/20220518202729.GA4606@bhelgaas/

Pali Rohár (2):
  PCI: aardvark: Add support for AER registers on emulated bridge
  PCI: aardvark: Fix reporting Slot capabilities on emulated bridge

 drivers/pci/controller/pci-aardvark.c | 110 +++++++++++++++++++++++---
 1 file changed, 101 insertions(+), 9 deletions(-)

-- 
2.35.1

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

[Marek Behún]

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(+)

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,
 };
 
 /*
-- 
2.35.1

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

[Rob Herring]

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

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

[Marek Behún]

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/

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

[Rob Herring]

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

[PATCH v2 2/2] PCI: aardvark: Fix reporting Slot capabilities on emulated bridge

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Slot capabilities are currently not reported because emulated bridge
does not report the PCI_EXP_FLAGS_SLOT flag.

Set PCI_EXP_FLAGS_SLOT to let the kernel know that PCI_EXP_SLT*
registers are supported.

Move setting of PCI_EXP_SLTCTL register from "dynamic" pcie_conf_read
function to static buffer as it is only statically filled the
PCI_EXP_SLTSTA_PDS flag and dynamic read callback is not needed for this
register.

Set Presence State Bit to 1 since there is no support for unplugging the
card and there is currently no platform able to detect presence of
a card - in such a case the bit needs to be set to 1.

Finally correctly set Physical Slot Number to 1 since there is only one
port and zero value is reserved for ports within the same silicon as
Root Port which is not our case for Aardvark HW.

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 | 33 +++++++++++++++++++--------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index d71c9bc95934..fa7422a1a2e0 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -8,6 +8,7 @@
  * Author: Hezi Shahmoon <hezi.shahmoon@marvell.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
@@ -858,14 +859,11 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
 
 
 	switch (reg) {
-	case PCI_EXP_SLTCTL:
-		*value = PCI_EXP_SLTSTA_PDS << 16;
-		return PCI_BRIDGE_EMUL_HANDLED;
-
 	/*
-	 * PCI_EXP_RTCTL and PCI_EXP_RTSTA are also supported, but do not need
-	 * to be handled here, because their values are stored in emulated
-	 * config space buffer, and we read them from there when needed.
+	 * PCI_EXP_SLTCAP, PCI_EXP_SLTCTL, PCI_EXP_RTCTL and PCI_EXP_RTSTA are
+	 * also supported, but do not need to be handled here, because their
+	 * values are stored in emulated config space buffer, and we read them
+	 * from there when needed.
 	 */
 
 	case PCI_EXP_LNKCAP: {
@@ -1054,8 +1052,25 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
 	/* Support interrupt A for MSI feature */
 	bridge->conf.intpin = PCI_INTERRUPT_INTA;
 
-	/* Aardvark HW provides PCIe Capability structure in version 2 */
-	bridge->pcie_conf.cap = cpu_to_le16(2);
+	/*
+	 * Aardvark HW provides PCIe Capability structure in version 2 and
+	 * indicate slot support, which is emulated.
+	 */
+	bridge->pcie_conf.cap = cpu_to_le16(2 | PCI_EXP_FLAGS_SLOT);
+
+	/*
+	 * Set Presence Detect State bit permanently since there is no support
+	 * for unplugging the card nor detecting whether it is plugged. (If a
+	 * platform exists in the future that supports it, via a GPIO for
+	 * example, it should be implemented via this bit.)
+	 *
+	 * Set physical slot number to 1 since there is only one port and zero
+	 * value is reserved for ports within the same silicon as Root Port
+	 * which is not our case.
+	 */
+	bridge->pcie_conf.slotcap = cpu_to_le32(FIELD_PREP(PCI_EXP_SLTCAP_PSN,
+							   1));
+	bridge->pcie_conf.slotsta = cpu_to_le16(PCI_EXP_SLTSTA_PDS);
 
 	/* Indicates supports for Completion Retry Status */
 	bridge->pcie_conf.rootcap = cpu_to_le16(PCI_EXP_RTCAP_CRSVIS);
-- 
2.35.1

Re: [PATCH v2 0/2] PCI: aardvark controller changes BATCH 5 (subset)

[Bjorn Helgaas]

On Tue, May 24, 2022 at 03:28:25PM +0200, Marek Behún wrote:
> Hello Bjorn,
> 
> since Lorenzo is AFK but gave his review for patches 3 and 5 of fifth
> batch of Aardvark changes, and you only requested to use FIELD_PREP [1]
> in order to avoid adding new _SHIFT macros, I am now sending these two
> patches to you. Could we get this merged?
> 
> Thanks.
> 
> Marek
> 
> Changes since v1:
> - dropped all patches but 3 and 5
> - changed patch 5 to use FIELD_PREP instead of _SHIFT macro
> 
> [1] https://lore.kernel.org/linux-arm-kernel/20220518202729.GA4606@bhelgaas/
> 
> Pali Rohár (2):
>   PCI: aardvark: Add support for AER registers on emulated bridge
>   PCI: aardvark: Fix reporting Slot capabilities on emulated bridge
> 
>  drivers/pci/controller/pci-aardvark.c | 110 +++++++++++++++++++++++---
>  1 file changed, 101 insertions(+), 9 deletions(-)

Applied to pci/ctrl/aardvark for v5.20, thanks!