[PATCH v2 00/23] PCI: aardvark controller fixes BATCH 4

[PATCH v2 00/23] PCI: aardvark controller fixes BATCH 4

[Marek Behún]

Hello Lorenzo, Bjorn and Marc,

this is v2 of fourth batch of fixes for the Aardvark PCIe controller
driver.

Stuff is converted to new interrupt APIs and recommendations from Marc.
Marc, could you look at these and acknowledge or comment?

This series mainly fixes and adds support for stuff around interrupts:
the most important thing is fixing MSI support.

The series is rebased on helgaas/next.

Marek

Changes since v1:
- added patches converting irq_chip and msi_domain_info structures into
  static driver structures, instead of creating them dynamically, as
  suggested by Marc
- added some small patches that should be easy to review
  - conversion to use constants from linux/pci.h instead of ad-hoc
    constants int patch 1
  - use dev_fwnode(dev) instead of of_node_to_fwnode(dev->of_node) in
    patch 8
  - fix of a comment in patch 22

Marek Behún (6):
  PCI: aardvark: Make MSI irq_chip structures static driver structures
  PCI: aardvark: Make msi_domain_info structure a static driver
    structure
  PCI: aardvark: Use dev_fwnode() instead of
    of_node_to_fwnode(dev->of_node)
  PCI: aardvark: Drop __maybe_unused from advk_pcie_disable_phy()
  PCI: aardvark: Update comment about link going down after link-up
  PCI: aardvark: Make main irq_chip structure a static driver structure

Pali Rohár (17):
  PCI: aardvark: Replace custom PCIE_CORE_INT_* macros with
    PCI_INTERRUPT_*
  PCI: aardvark: Fix reading MSI interrupt number
  PCI: aardvark: Fix support for MSI interrupts
  PCI: aardvark: Rewrite IRQ code to chained IRQ handler
  PCI: aardvark: Check return value of generic_handle_domain_irq() when
    processing INTx IRQ
  PCI: aardvark: Refactor unmasking summary MSI interrupt
  PCI: aardvark: Add support for masking MSI interrupts
  PCI: aardvark: Fix setting MSI address
  PCI: aardvark: Enable MSI-X support
  PCI: aardvark: Add support for ERR interrupt on emulated bridge
  PCI: aardvark: Fix reading PCI_EXP_RTSTA_PME bit on emulated bridge
  PCI: aardvark: Optimize writing PCI_EXP_RTCTL_PMEIE and
    PCI_EXP_RTSTA_PME on emulated bridge
  PCI: aardvark: Add support for PME interrupts
  PCI: aardvark: Fix support for PME requester on emulated bridge
  PCI: aardvark: Use separate INTA interrupt for emulated root bridge
  PCI: aardvark: Remove irq_mask_ack callback for INTx interrupts
  PCI: aardvark: Don't mask irq when mapping

 drivers/pci/controller/pci-aardvark.c | 415 +++++++++++++++++---------
 1 file changed, 281 insertions(+), 134 deletions(-)

-- 
2.34.1

[PATCH v2 01/23] PCI: aardvark: Replace custom PCIE_CORE_INT_* macros with PCI_INTERRUPT_*

[Marek Behún]

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

Header file linux/pci.h defines enum pci_interrupt_pin with corresponding
PCI_INTERRUPT_* values.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index ec0df426863d..62baddd2ca95 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -39,10 +39,6 @@
 #define     PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX_EN			BIT(6)
 #define     PCIE_CORE_ERR_CAPCTL_ECRC_CHCK			BIT(7)
 #define     PCIE_CORE_ERR_CAPCTL_ECRC_CHCK_RCV			BIT(8)
-#define     PCIE_CORE_INT_A_ASSERT_ENABLE			1
-#define     PCIE_CORE_INT_B_ASSERT_ENABLE			2
-#define     PCIE_CORE_INT_C_ASSERT_ENABLE			3
-#define     PCIE_CORE_INT_D_ASSERT_ENABLE			4
 /* PIO registers base address and register offsets */
 #define PIO_BASE_ADDR				0x4000
 #define PIO_CTRL				(PIO_BASE_ADDR + 0x0)
@@ -968,7 +964,7 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
 	bridge->conf.pref_mem_limit = cpu_to_le16(PCI_PREF_RANGE_TYPE_64);
 
 	/* Support interrupt A for MSI feature */
-	bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
+	bridge->conf.intpin = PCI_INTERRUPT_INTA;
 
 	/* Aardvark HW provides PCIe Capability structure in version 2 */
 	bridge->pcie_conf.cap = cpu_to_le16(2);
-- 
2.34.1

Re: [PATCH v2 01/23] PCI: aardvark: Replace custom PCIE_CORE_INT_* macros with PCI_INTERRUPT_*

[Bjorn Helgaas]

On Mon, Jan 10, 2022 at 02:49:56AM +0100, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Header file linux/pci.h defines enum pci_interrupt_pin with corresponding
> PCI_INTERRUPT_* values.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>

Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks!

> ---
>  drivers/pci/controller/pci-aardvark.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index ec0df426863d..62baddd2ca95 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -39,10 +39,6 @@
>  #define     PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX_EN			BIT(6)
>  #define     PCIE_CORE_ERR_CAPCTL_ECRC_CHCK			BIT(7)
>  #define     PCIE_CORE_ERR_CAPCTL_ECRC_CHCK_RCV			BIT(8)
> -#define     PCIE_CORE_INT_A_ASSERT_ENABLE			1
> -#define     PCIE_CORE_INT_B_ASSERT_ENABLE			2
> -#define     PCIE_CORE_INT_C_ASSERT_ENABLE			3
> -#define     PCIE_CORE_INT_D_ASSERT_ENABLE			4
>  /* PIO registers base address and register offsets */
>  #define PIO_BASE_ADDR				0x4000
>  #define PIO_CTRL				(PIO_BASE_ADDR + 0x0)
> @@ -968,7 +964,7 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
>  	bridge->conf.pref_mem_limit = cpu_to_le16(PCI_PREF_RANGE_TYPE_64);
>  
>  	/* Support interrupt A for MSI feature */
> -	bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
> +	bridge->conf.intpin = PCI_INTERRUPT_INTA;
>  
>  	/* Aardvark HW provides PCIe Capability structure in version 2 */
>  	bridge->pcie_conf.cap = cpu_to_le16(2);
> -- 
> 2.34.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 02/23] PCI: aardvark: Fix reading MSI interrupt number

[Marek Behún]

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

In advk_pcie_handle_msi() the authors expect that when bit i in the W1C
register PCIE_MSI_STATUS_REG is cleared, the PCIE_MSI_PAYLOAD_REG is
updated to contain the MSI number corresponding to index i.

Experiments show that this is not so, and instead PCIE_MSI_PAYLOAD_REG
always contains the number of the last received MSI, overall.

Do not read PCIE_MSI_PAYLOAD_REG register for determining MSI interrupt
number. Since Aardvark already forbids more than 32 interrupts and uses
own allocated hwirq numbers, the msi_idx already corresponds to the
received MSI number.

Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 62baddd2ca95..fd95ad64c887 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1393,7 +1393,6 @@ static void advk_pcie_remove_irq_domain(struct advk_pcie *pcie)
 static void advk_pcie_handle_msi(struct advk_pcie *pcie)
 {
 	u32 msi_val, msi_mask, msi_status, msi_idx;
-	u16 msi_data;
 
 	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
 	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
@@ -1403,13 +1402,9 @@ static void advk_pcie_handle_msi(struct advk_pcie *pcie)
 		if (!(BIT(msi_idx) & msi_status))
 			continue;
 
-		/*
-		 * msi_idx contains bits [4:0] of the msi_data and msi_data
-		 * contains 16bit MSI interrupt number
-		 */
 		advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG);
-		msi_data = advk_readl(pcie, PCIE_MSI_PAYLOAD_REG) & PCIE_MSI_DATA_MASK;
-		generic_handle_irq(msi_data);
+		if (generic_handle_domain_irq(pcie->msi_inner_domain, msi_idx) == -EINVAL)
+			dev_err_ratelimited(&pcie->pdev->dev, "unexpected MSI 0x%02x\n", msi_idx);
 	}
 
 	advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING,
-- 
2.34.1

Re: [PATCH v2 02/23] PCI: aardvark: Fix reading MSI interrupt number

[Lorenzo Pieralisi]

On Mon, Jan 10, 2022 at 02:49:57AM +0100, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> In advk_pcie_handle_msi() the authors expect that when bit i in the W1C
> register PCIE_MSI_STATUS_REG is cleared, the PCIE_MSI_PAYLOAD_REG is
> updated to contain the MSI number corresponding to index i.
> 
> Experiments show that this is not so, and instead PCIE_MSI_PAYLOAD_REG
> always contains the number of the last received MSI, overall.
> 
> Do not read PCIE_MSI_PAYLOAD_REG register for determining MSI interrupt
> number. Since Aardvark already forbids more than 32 interrupts and uses
> own allocated hwirq numbers, the msi_idx already corresponds to the
> received MSI number.
> 
> Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/pci/controller/pci-aardvark.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 62baddd2ca95..fd95ad64c887 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -1393,7 +1393,6 @@ static void advk_pcie_remove_irq_domain(struct advk_pcie *pcie)
>  static void advk_pcie_handle_msi(struct advk_pcie *pcie)
>  {
>  	u32 msi_val, msi_mask, msi_status, msi_idx;
> -	u16 msi_data;
>  
>  	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
>  	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
> @@ -1403,13 +1402,9 @@ static void advk_pcie_handle_msi(struct advk_pcie *pcie)
>  		if (!(BIT(msi_idx) & msi_status))
>  			continue;
>  
> -		/*
> -		 * msi_idx contains bits [4:0] of the msi_data and msi_data
> -		 * contains 16bit MSI interrupt number
> -		 */
>  		advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG);
> -		msi_data = advk_readl(pcie, PCIE_MSI_PAYLOAD_REG) & PCIE_MSI_DATA_MASK;

Ok, it took me a while to understand how aardvark handles MSIs.

IIUC, msi_data contains the payload of the latest MSI write received.

First off, I believe that using a Linux IRQ number for MSI data
(payload)(I guess you rely on its truncated bits [4:0] to trigger the
related MSI IRQs, which I believe is questionable - if not broken) is
not a good idea.

Is my understanding correct ?

This patch and the following one are fixing this. Given that this is IRQ
domain code if Marc can cast a look into it that would help me, to make
sure I have not missed anything.

Certainly replacing the MSI payload to the HW irq number makes sense to
me (and this patch makes sense too, I think mainline code may miss some
MSI IRQs if I understand this patch correctly).

Lorenzo

> -		generic_handle_irq(msi_data);
> +		if (generic_handle_domain_irq(pcie->msi_inner_domain, msi_idx) == -EINVAL)
> +			dev_err_ratelimited(&pcie->pdev->dev, "unexpected MSI 0x%02x\n", msi_idx);
>  	}
>  
>  	advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING,
> -- 
> 2.34.1
> 

Re: [PATCH v2 02/23] PCI: aardvark: Fix reading MSI interrupt number

[Marc Zyngier]

Hi Lorenzo,

On Fri, 04 Feb 2022 17:24:00 +0000,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
> On Mon, Jan 10, 2022 at 02:49:57AM +0100, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > In advk_pcie_handle_msi() the authors expect that when bit i in the W1C
> > register PCIE_MSI_STATUS_REG is cleared, the PCIE_MSI_PAYLOAD_REG is
> > updated to contain the MSI number corresponding to index i.
> > 
> > Experiments show that this is not so, and instead PCIE_MSI_PAYLOAD_REG
> > always contains the number of the last received MSI, overall.
> > 
> > Do not read PCIE_MSI_PAYLOAD_REG register for determining MSI interrupt
> > number. Since Aardvark already forbids more than 32 interrupts and uses
> > own allocated hwirq numbers, the msi_idx already corresponds to the
> > received MSI number.
> > 
> > Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index 62baddd2ca95..fd95ad64c887 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -1393,7 +1393,6 @@ static void advk_pcie_remove_irq_domain(struct advk_pcie *pcie)
> >  static void advk_pcie_handle_msi(struct advk_pcie *pcie)
> >  {
> >  	u32 msi_val, msi_mask, msi_status, msi_idx;
> > -	u16 msi_data;
> >  
> >  	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
> >  	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
> > @@ -1403,13 +1402,9 @@ static void advk_pcie_handle_msi(struct advk_pcie *pcie)
> >  		if (!(BIT(msi_idx) & msi_status))
> >  			continue;
> >  
> > -		/*
> > -		 * msi_idx contains bits [4:0] of the msi_data and msi_data
> > -		 * contains 16bit MSI interrupt number
> > -		 */
> >  		advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG);
> > -		msi_data = advk_readl(pcie, PCIE_MSI_PAYLOAD_REG) & PCIE_MSI_DATA_MASK;
> 
> Ok, it took me a while to understand how aardvark handles MSIs.
> 
> IIUC, msi_data contains the payload of the latest MSI write received.
>
> First off, I believe that using a Linux IRQ number for MSI data
> (payload)(I guess you rely on its truncated bits [4:0] to trigger the
> related MSI IRQs, which I believe is questionable - if not broken) is
> not a good idea.
> 
> Is my understanding correct ?
>
> This patch and the following one are fixing this. Given that this is IRQ
> domain code if Marc can cast a look into it that would help me, to make
> sure I have not missed anything.

I had a look at this series a long while ago, and nothing seem out of
sorts on the MSI front. If anything, this is better than what is
currently there.

As for the rest of the series:

Patch 18 is odd: the use of the handle_simple_irq() flow is likely
papering over something (I'd expect RP interrupts to be edge
triggered), and it is impossible to mask them. But hey, after being
singled out by the author as the big bad bully who prevents people
from fixing things, I can't say I care.

I only objected to patch 23, which is a total no-go.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

[PATCH v2 03/23] PCI: aardvark: Fix support for MSI interrupts

[Marek Behún]

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

Aardvark hardware supports Multi-MSI and MSI_FLAG_MULTI_PCI_MSI is already
set for the MSI chip. But when allocating MSI interrupt numbers for
Multi-MSI, the numbers need to be properly aligned, otherwise endpoint
devices send MSI interrupt with incorrect numbers.

Fix this issue by using function bitmap_find_free_region() instead of
bitmap_find_next_zero_area().

To ensure that aligned MSI interrupt numbers are used by endpoint devices,
we cannot use Linux virtual irq numbers (as they are random and not
properly aligned). Instead we need to use the aligned hwirq numbers.

This change fixes receiving MSI interrupts on Armada 3720 boards and
allows using NVMe disks which use Multi-MSI feature with 3 interrupts.

Without this NVMe disks freeze booting as linux nvme-core.c is waiting
60s for an interrupt.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index fd95ad64c887..346d38835539 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1189,7 +1189,7 @@ static void advk_msi_irq_compose_msi_msg(struct irq_data *data,
 
 	msg->address_lo = lower_32_bits(msi_msg);
 	msg->address_hi = upper_32_bits(msi_msg);
-	msg->data = data->irq;
+	msg->data = data->hwirq;
 }
 
 static int advk_msi_set_affinity(struct irq_data *irq_data,
@@ -1206,15 +1206,11 @@ static int advk_msi_irq_domain_alloc(struct irq_domain *domain,
 	int hwirq, i;
 
 	mutex_lock(&pcie->msi_used_lock);
-	hwirq = bitmap_find_next_zero_area(pcie->msi_used, MSI_IRQ_NUM,
-					   0, nr_irqs, 0);
-	if (hwirq >= MSI_IRQ_NUM) {
-		mutex_unlock(&pcie->msi_used_lock);
-		return -ENOSPC;
-	}
-
-	bitmap_set(pcie->msi_used, hwirq, nr_irqs);
+	hwirq = bitmap_find_free_region(pcie->msi_used, MSI_IRQ_NUM,
+					order_base_2(nr_irqs));
 	mutex_unlock(&pcie->msi_used_lock);
+	if (hwirq < 0)
+		return -ENOSPC;
 
 	for (i = 0; i < nr_irqs; i++)
 		irq_domain_set_info(domain, virq + i, hwirq + i,
@@ -1232,7 +1228,7 @@ static void advk_msi_irq_domain_free(struct irq_domain *domain,
 	struct advk_pcie *pcie = domain->host_data;
 
 	mutex_lock(&pcie->msi_used_lock);
-	bitmap_clear(pcie->msi_used, d->hwirq, nr_irqs);
+	bitmap_release_region(pcie->msi_used, d->hwirq, order_base_2(nr_irqs));
 	mutex_unlock(&pcie->msi_used_lock);
 }
 
-- 
2.34.1

[PATCH v2 04/23] PCI: aardvark: Rewrite IRQ code to chained IRQ handler

[Marek Behún]

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

Rewrite the code to use irq_set_chained_handler_and_data() handler with
chained_irq_enter() and chained_irq_exit() processing instead of using
devm_request_irq().

advk_pcie_irq_handler() reads IRQ status bits and calls other functions
based on which bits are set. These functions then read its own IRQ status
bits and calls other aardvark functions based on these bits. Finally
generic_handle_domain_irq() with translated linux IRQ numbers are called.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 48 +++++++++++++++------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 346d38835539..315147f2812f 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -269,6 +269,7 @@ struct advk_pcie {
 		u32 actions;
 	} wins[OB_WIN_COUNT];
 	u8 wins_count;
+	int irq;
 	struct irq_domain *irq_domain;
 	struct irq_chip irq_chip;
 	raw_spinlock_t irq_lock;
@@ -1437,21 +1438,26 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie)
 	}
 }
 
-static irqreturn_t advk_pcie_irq_handler(int irq, void *arg)
+static void advk_pcie_irq_handler(struct irq_desc *desc)
 {
-	struct advk_pcie *pcie = arg;
-	u32 status;
+	struct advk_pcie *pcie = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u32 val, mask, status;
 
-	status = advk_readl(pcie, HOST_CTRL_INT_STATUS_REG);
-	if (!(status & PCIE_IRQ_CORE_INT))
-		return IRQ_NONE;
+	chained_irq_enter(chip, desc);
 
-	advk_pcie_handle_int(pcie);
+	val = advk_readl(pcie, HOST_CTRL_INT_STATUS_REG);
+	mask = advk_readl(pcie, HOST_CTRL_INT_MASK_REG);
+	status = val & ((~mask) & PCIE_IRQ_ALL_MASK);
 
-	/* Clear interrupt */
-	advk_writel(pcie, PCIE_IRQ_CORE_INT, HOST_CTRL_INT_STATUS_REG);
+	if (status & PCIE_IRQ_CORE_INT) {
+		advk_pcie_handle_int(pcie);
 
-	return IRQ_HANDLED;
+		/* Clear interrupt */
+		advk_writel(pcie, PCIE_IRQ_CORE_INT, HOST_CTRL_INT_STATUS_REG);
+	}
+
+	chained_irq_exit(chip, desc);
 }
 
 static void __maybe_unused advk_pcie_disable_phy(struct advk_pcie *pcie)
@@ -1518,7 +1524,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	struct advk_pcie *pcie;
 	struct pci_host_bridge *bridge;
 	struct resource_entry *entry;
-	int ret, irq;
+	int ret;
 
 	bridge = devm_pci_alloc_host_bridge(dev, sizeof(struct advk_pcie));
 	if (!bridge)
@@ -1604,17 +1610,9 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(pcie->base))
 		return PTR_ERR(pcie->base);
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
-
-	ret = devm_request_irq(dev, irq, advk_pcie_irq_handler,
-			       IRQF_SHARED | IRQF_NO_THREAD, "advk-pcie",
-			       pcie);
-	if (ret) {
-		dev_err(dev, "Failed to register interrupt\n");
-		return ret;
-	}
+	pcie->irq = platform_get_irq(pdev, 0);
+	if (pcie->irq < 0)
+		return pcie->irq;
 
 	pcie->reset_gpio = devm_gpiod_get_from_of_node(dev, dev->of_node,
 						       "reset-gpios", 0,
@@ -1663,11 +1661,14 @@ static int advk_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	irq_set_chained_handler_and_data(pcie->irq, advk_pcie_irq_handler, pcie);
+
 	bridge->sysdata = pcie;
 	bridge->ops = &advk_pcie_ops;
 
 	ret = pci_host_probe(bridge);
 	if (ret < 0) {
+		irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
 		advk_pcie_remove_msi_irq_domain(pcie);
 		advk_pcie_remove_irq_domain(pcie);
 		return ret;
@@ -1715,6 +1716,9 @@ static int advk_pcie_remove(struct platform_device *pdev)
 	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
 	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
 
+	/* Remove IRQ handler */
+	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
+
 	/* Remove IRQ domains */
 	advk_pcie_remove_msi_irq_domain(pcie);
 	advk_pcie_remove_irq_domain(pcie);
-- 
2.34.1

[PATCH v2 05/23] PCI: aardvark: Check return value of generic_handle_domain_irq() when processing INTx IRQ

[Marek Behún]

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

It is possible that we receive spurious INTx interrupt. Check for the
return value of generic_handle_domain_irq() when processing INTx IRQ.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 315147f2812f..252033a46e1e 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1434,7 +1434,9 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie)
 		advk_writel(pcie, PCIE_ISR1_INTX_ASSERT(i),
 			    PCIE_ISR1_REG);
 
-		generic_handle_domain_irq(pcie->irq_domain, i);
+		if (generic_handle_domain_irq(pcie->irq_domain, i) == -EINVAL)
+			dev_err_ratelimited(&pcie->pdev->dev, "unexpected INT%c IRQ\n",
+					    (char)i + 'A');
 	}
 }
 
-- 
2.34.1

[PATCH v2 06/23] PCI: aardvark: Make MSI irq_chip structures static driver structures

[Marek Behún]

Marc Zyngier says [1] that we should use struct irq_chip as a global
static struct in the driver. Even though the structure currently
contains a dynamic member (parent_device), Marc says [2] that he plans
to kill it and make the structure completely static.

Convert Aardvark's priv->msi_bottom_irq_chip and priv->msi_irq_chip to
static driver structure.

[1] https://lore.kernel.org/linux-pci/877dbcvngf.wl-maz@kernel.org/
[2] https://lore.kernel.org/linux-pci/874k6gvkhz.wl-maz@kernel.org/

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 252033a46e1e..441100bacb68 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -275,8 +275,6 @@ struct advk_pcie {
 	raw_spinlock_t irq_lock;
 	struct irq_domain *msi_domain;
 	struct irq_domain *msi_inner_domain;
-	struct irq_chip msi_bottom_irq_chip;
-	struct irq_chip msi_irq_chip;
 	struct msi_domain_info msi_domain_info;
 	DECLARE_BITMAP(msi_used, MSI_IRQ_NUM);
 	struct mutex msi_used_lock;
@@ -1199,6 +1197,12 @@ static int advk_msi_set_affinity(struct irq_data *irq_data,
 	return -EINVAL;
 }
 
+static struct irq_chip advk_msi_bottom_irq_chip = {
+	.name			= "MSI",
+	.irq_compose_msi_msg	= advk_msi_irq_compose_msi_msg,
+	.irq_set_affinity	= advk_msi_set_affinity,
+};
+
 static int advk_msi_irq_domain_alloc(struct irq_domain *domain,
 				     unsigned int virq,
 				     unsigned int nr_irqs, void *args)
@@ -1215,7 +1219,7 @@ static int advk_msi_irq_domain_alloc(struct irq_domain *domain,
 
 	for (i = 0; i < nr_irqs; i++)
 		irq_domain_set_info(domain, virq + i, hwirq + i,
-				    &pcie->msi_bottom_irq_chip,
+				    &advk_msi_bottom_irq_chip,
 				    domain->host_data, handle_simple_irq,
 				    NULL, NULL);
 
@@ -1285,29 +1289,23 @@ static const struct irq_domain_ops advk_pcie_irq_domain_ops = {
 	.xlate = irq_domain_xlate_onecell,
 };
 
+static struct irq_chip advk_msi_irq_chip = {
+	.name = "advk-MSI",
+};
+
 static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 {
 	struct device *dev = &pcie->pdev->dev;
 	struct device_node *node = dev->of_node;
-	struct irq_chip *bottom_ic, *msi_ic;
 	struct msi_domain_info *msi_di;
 	phys_addr_t msi_msg_phys;
 
 	mutex_init(&pcie->msi_used_lock);
 
-	bottom_ic = &pcie->msi_bottom_irq_chip;
-
-	bottom_ic->name = "MSI";
-	bottom_ic->irq_compose_msi_msg = advk_msi_irq_compose_msi_msg;
-	bottom_ic->irq_set_affinity = advk_msi_set_affinity;
-
-	msi_ic = &pcie->msi_irq_chip;
-	msi_ic->name = "advk-MSI";
-
 	msi_di = &pcie->msi_domain_info;
 	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
 		MSI_FLAG_MULTI_PCI_MSI;
-	msi_di->chip = msi_ic;
+	msi_di->chip = &advk_msi_irq_chip;
 
 	msi_msg_phys = virt_to_phys(&pcie->msi_msg);
 
-- 
2.34.1

[PATCH v2 07/23] PCI: aardvark: Make msi_domain_info structure a static driver structure

[Marek Behún]

Make Aardvark's msi_domain_info structure into a private driver structure.
Domain info is same for every potential instatination of a controller.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 441100bacb68..5ab107f65c6c 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -275,7 +275,6 @@ struct advk_pcie {
 	raw_spinlock_t irq_lock;
 	struct irq_domain *msi_domain;
 	struct irq_domain *msi_inner_domain;
-	struct msi_domain_info msi_domain_info;
 	DECLARE_BITMAP(msi_used, MSI_IRQ_NUM);
 	struct mutex msi_used_lock;
 	u16 msi_msg;
@@ -1293,20 +1292,20 @@ static struct irq_chip advk_msi_irq_chip = {
 	.name = "advk-MSI",
 };
 
+static struct msi_domain_info advk_msi_domain_info = {
+	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+		  MSI_FLAG_MULTI_PCI_MSI,
+	.chip	= &advk_msi_irq_chip,
+};
+
 static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 {
 	struct device *dev = &pcie->pdev->dev;
 	struct device_node *node = dev->of_node;
-	struct msi_domain_info *msi_di;
 	phys_addr_t msi_msg_phys;
 
 	mutex_init(&pcie->msi_used_lock);
 
-	msi_di = &pcie->msi_domain_info;
-	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
-		MSI_FLAG_MULTI_PCI_MSI;
-	msi_di->chip = &advk_msi_irq_chip;
-
 	msi_msg_phys = virt_to_phys(&pcie->msi_msg);
 
 	advk_writel(pcie, lower_32_bits(msi_msg_phys),
@@ -1322,7 +1321,8 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 
 	pcie->msi_domain =
 		pci_msi_create_irq_domain(of_node_to_fwnode(node),
-					  msi_di, pcie->msi_inner_domain);
+					  &advk_msi_domain_info,
+					  pcie->msi_inner_domain);
 	if (!pcie->msi_domain) {
 		irq_domain_remove(pcie->msi_inner_domain);
 		return -ENOMEM;
-- 
2.34.1

[PATCH v2 08/23] PCI: aardvark: Use dev_fwnode() instead of of_node_to_fwnode(dev->of_node)

[Marek Behún]

Use simple
  dev_fwnode(dev)
instead of
  struct device_node *node = dev->of_node;
  of_node_to_fwnode(node)
especially since the node variable is not used elsewhere in the function.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 5ab107f65c6c..3f7515814167 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1301,7 +1301,6 @@ static struct msi_domain_info advk_msi_domain_info = {
 static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 {
 	struct device *dev = &pcie->pdev->dev;
-	struct device_node *node = dev->of_node;
 	phys_addr_t msi_msg_phys;
 
 	mutex_init(&pcie->msi_used_lock);
@@ -1320,7 +1319,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 		return -ENOMEM;
 
 	pcie->msi_domain =
-		pci_msi_create_irq_domain(of_node_to_fwnode(node),
+		pci_msi_create_irq_domain(dev_fwnode(dev),
 					  &advk_msi_domain_info,
 					  pcie->msi_inner_domain);
 	if (!pcie->msi_domain) {
-- 
2.34.1

[PATCH v2 09/23] PCI: aardvark: Refactor unmasking summary MSI interrupt

[Marek Behún]

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

Refactor the masking of ISR0/1 Sources and unmasking of summary MSI interrupt
so that it corresponds to the comments:
- first mask all ISR0/1
- then unmask all MSIs
- then unmask summary MSI interrupt

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 3f7515814167..9d2462f076c1 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -572,15 +572,17 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
 
 	/* Disable All ISR0/1 Sources */
-	reg = PCIE_ISR0_ALL_MASK;
-	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
-	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
-
+	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_MASK_REG);
 	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
 
 	/* Unmask all MSIs */
 	advk_writel(pcie, ~(u32)PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
 
+	/* Unmask summary MSI interrupt */
+	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
+	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
+	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
+
 	/* Enable summary interrupt for GIC SPI source */
 	reg = PCIE_IRQ_ALL_MASK & (~PCIE_IRQ_ENABLE_INTS_MASK);
 	advk_writel(pcie, reg, HOST_CTRL_INT_MASK_REG);
-- 
2.34.1

[PATCH v2 10/23] PCI: aardvark: Add support for masking MSI interrupts

[Marek Behún]

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

We should not unmask MSIs at setup, but only when kernel asks for them
to be unmasked.

At setup, mask all MSIs, and implement IRQ chip callbacks for masking
and unmasking particular MSIs.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 54 ++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 9d2462f076c1..51fedbcb41fb 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -275,6 +275,7 @@ struct advk_pcie {
 	raw_spinlock_t irq_lock;
 	struct irq_domain *msi_domain;
 	struct irq_domain *msi_inner_domain;
+	raw_spinlock_t msi_irq_lock;
 	DECLARE_BITMAP(msi_used, MSI_IRQ_NUM);
 	struct mutex msi_used_lock;
 	u16 msi_msg;
@@ -571,12 +572,10 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
 	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
 
-	/* Disable All ISR0/1 Sources */
+	/* Disable All ISR0/1 and MSI Sources */
 	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_MASK_REG);
 	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
-
-	/* Unmask all MSIs */
-	advk_writel(pcie, ~(u32)PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
+	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
 
 	/* Unmask summary MSI interrupt */
 	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
@@ -1198,10 +1197,52 @@ static int advk_msi_set_affinity(struct irq_data *irq_data,
 	return -EINVAL;
 }
 
+static void advk_msi_irq_mask(struct irq_data *d)
+{
+	struct advk_pcie *pcie = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 mask;
+
+	raw_spin_lock_irqsave(&pcie->msi_irq_lock, flags);
+	mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
+	mask |= BIT(hwirq);
+	advk_writel(pcie, mask, PCIE_MSI_MASK_REG);
+	raw_spin_unlock_irqrestore(&pcie->msi_irq_lock, flags);
+}
+
+static void advk_msi_irq_unmask(struct irq_data *d)
+{
+	struct advk_pcie *pcie = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 mask;
+
+	raw_spin_lock_irqsave(&pcie->msi_irq_lock, flags);
+	mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
+	mask &= ~BIT(hwirq);
+	advk_writel(pcie, mask, PCIE_MSI_MASK_REG);
+	raw_spin_unlock_irqrestore(&pcie->msi_irq_lock, flags);
+}
+
+static void advk_msi_top_irq_mask(struct irq_data *d)
+{
+	pci_msi_mask_irq(d);
+	irq_chip_mask_parent(d);
+}
+
+static void advk_msi_top_irq_unmask(struct irq_data *d)
+{
+	pci_msi_unmask_irq(d);
+	irq_chip_unmask_parent(d);
+}
+
 static struct irq_chip advk_msi_bottom_irq_chip = {
 	.name			= "MSI",
 	.irq_compose_msi_msg	= advk_msi_irq_compose_msi_msg,
 	.irq_set_affinity	= advk_msi_set_affinity,
+	.irq_mask		= advk_msi_irq_mask,
+	.irq_unmask		= advk_msi_irq_unmask,
 };
 
 static int advk_msi_irq_domain_alloc(struct irq_domain *domain,
@@ -1291,7 +1332,9 @@ static const struct irq_domain_ops advk_pcie_irq_domain_ops = {
 };
 
 static struct irq_chip advk_msi_irq_chip = {
-	.name = "advk-MSI",
+	.name		= "advk-MSI",
+	.irq_mask	= advk_msi_top_irq_mask,
+	.irq_unmask	= advk_msi_top_irq_unmask,
 };
 
 static struct msi_domain_info advk_msi_domain_info = {
@@ -1305,6 +1348,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 	struct device *dev = &pcie->pdev->dev;
 	phys_addr_t msi_msg_phys;
 
+	raw_spin_lock_init(&pcie->msi_irq_lock);
 	mutex_init(&pcie->msi_used_lock);
 
 	msi_msg_phys = virt_to_phys(&pcie->msi_msg);
-- 
2.34.1

[PATCH v2 11/23] PCI: aardvark: Fix setting MSI address

[Marek Behún]

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

MSI address for receiving MSI interrupts needs to be correctly set before
enabling processing of MSI interrupts.

Move code for setting PCIE_MSI_ADDR_LOW_REG and PCIE_MSI_ADDR_HIGH_REG
from advk_pcie_init_msi_irq_domain() to advk_pcie_setup_hw(), before
enabling PCIE_CORE_CTRL2_MSI_ENABLE.

After this we can remove the now unused member msi_msg, which was used
only for MSI doorbell address. MSI address can be any address which cannot
be used to DMA to. So change it to the address of the main struct advk_pcie.

Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
Signed-off-by: Pali Rohár <pali@kernel.org>
Acked-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org # f21a8b1b6837 ("PCI: aardvark: Move to MSI handling using generic MSI support")
---
 drivers/pci/controller/pci-aardvark.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 51fedbcb41fb..79102704d82f 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -278,7 +278,6 @@ struct advk_pcie {
 	raw_spinlock_t msi_irq_lock;
 	DECLARE_BITMAP(msi_used, MSI_IRQ_NUM);
 	struct mutex msi_used_lock;
-	u16 msi_msg;
 	int link_gen;
 	struct pci_bridge_emul bridge;
 	struct gpio_desc *reset_gpio;
@@ -473,6 +472,7 @@ static void advk_pcie_disable_ob_win(struct advk_pcie *pcie, u8 win_num)
 
 static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 {
+	phys_addr_t msi_addr;
 	u32 reg;
 	int i;
 
@@ -561,6 +561,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	reg |= LANE_COUNT_1;
 	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
 
+	/* Set MSI address */
+	msi_addr = virt_to_phys(pcie);
+	advk_writel(pcie, lower_32_bits(msi_addr), PCIE_MSI_ADDR_LOW_REG);
+	advk_writel(pcie, upper_32_bits(msi_addr), PCIE_MSI_ADDR_HIGH_REG);
+
 	/* Enable MSI */
 	reg = advk_readl(pcie, PCIE_CORE_CTRL2_REG);
 	reg |= PCIE_CORE_CTRL2_MSI_ENABLE;
@@ -1184,10 +1189,10 @@ static void advk_msi_irq_compose_msi_msg(struct irq_data *data,
 					 struct msi_msg *msg)
 {
 	struct advk_pcie *pcie = irq_data_get_irq_chip_data(data);
-	phys_addr_t msi_msg = virt_to_phys(&pcie->msi_msg);
+	phys_addr_t msi_addr = virt_to_phys(pcie);
 
-	msg->address_lo = lower_32_bits(msi_msg);
-	msg->address_hi = upper_32_bits(msi_msg);
+	msg->address_lo = lower_32_bits(msi_addr);
+	msg->address_hi = upper_32_bits(msi_addr);
 	msg->data = data->hwirq;
 }
 
@@ -1346,18 +1351,10 @@ static struct msi_domain_info advk_msi_domain_info = {
 static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 {
 	struct device *dev = &pcie->pdev->dev;
-	phys_addr_t msi_msg_phys;
 
 	raw_spin_lock_init(&pcie->msi_irq_lock);
 	mutex_init(&pcie->msi_used_lock);
 
-	msi_msg_phys = virt_to_phys(&pcie->msi_msg);
-
-	advk_writel(pcie, lower_32_bits(msi_msg_phys),
-		    PCIE_MSI_ADDR_LOW_REG);
-	advk_writel(pcie, upper_32_bits(msi_msg_phys),
-		    PCIE_MSI_ADDR_HIGH_REG);
-
 	pcie->msi_inner_domain =
 		irq_domain_add_linear(NULL, MSI_IRQ_NUM,
 				      &advk_msi_domain_ops, pcie);
-- 
2.34.1

Re: [PATCH v2 11/23] PCI: aardvark: Fix setting MSI address

[Bjorn Helgaas]

On Mon, Jan 10, 2022 at 02:50:06AM +0100, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> MSI address for receiving MSI interrupts needs to be correctly set before
> enabling processing of MSI interrupts.
> 
> Move code for setting PCIE_MSI_ADDR_LOW_REG and PCIE_MSI_ADDR_HIGH_REG
> from advk_pcie_init_msi_irq_domain() to advk_pcie_setup_hw(), before
> enabling PCIE_CORE_CTRL2_MSI_ENABLE.
> 
> After this we can remove the now unused member msi_msg, which was used
> only for MSI doorbell address. MSI address can be any address which cannot
> be used to DMA to. So change it to the address of the main struct advk_pcie.
> 
> Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Acked-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Cc: stable@vger.kernel.org # f21a8b1b6837 ("PCI: aardvark: Move to MSI handling using generic MSI support")
> ---
>  drivers/pci/controller/pci-aardvark.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 51fedbcb41fb..79102704d82f 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -278,7 +278,6 @@ struct advk_pcie {
>  	raw_spinlock_t msi_irq_lock;
>  	DECLARE_BITMAP(msi_used, MSI_IRQ_NUM);
>  	struct mutex msi_used_lock;
> -	u16 msi_msg;
>  	int link_gen;
>  	struct pci_bridge_emul bridge;
>  	struct gpio_desc *reset_gpio;
> @@ -473,6 +472,7 @@ static void advk_pcie_disable_ob_win(struct advk_pcie *pcie, u8 win_num)
>  
>  static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  {
> +	phys_addr_t msi_addr;
>  	u32 reg;
>  	int i;
>  
> @@ -561,6 +561,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  	reg |= LANE_COUNT_1;
>  	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
>  
> +	/* Set MSI address */
> +	msi_addr = virt_to_phys(pcie);

Strictly speaking, msi_addr should be a pci_bus_addr_t, not a
phys_addr_t, and virt_to_phys() doesn't return a bus address.

> +	advk_writel(pcie, lower_32_bits(msi_addr), PCIE_MSI_ADDR_LOW_REG);
> +	advk_writel(pcie, upper_32_bits(msi_addr), PCIE_MSI_ADDR_HIGH_REG);
> +
>  	/* Enable MSI */
>  	reg = advk_readl(pcie, PCIE_CORE_CTRL2_REG);
>  	reg |= PCIE_CORE_CTRL2_MSI_ENABLE;
> @@ -1184,10 +1189,10 @@ static void advk_msi_irq_compose_msi_msg(struct irq_data *data,
>  					 struct msi_msg *msg)
>  {
>  	struct advk_pcie *pcie = irq_data_get_irq_chip_data(data);
> -	phys_addr_t msi_msg = virt_to_phys(&pcie->msi_msg);
> +	phys_addr_t msi_addr = virt_to_phys(pcie);
>  
> -	msg->address_lo = lower_32_bits(msi_msg);
> -	msg->address_hi = upper_32_bits(msi_msg);
> +	msg->address_lo = lower_32_bits(msi_addr);
> +	msg->address_hi = upper_32_bits(msi_addr);
>  	msg->data = data->hwirq;
>  }
>  
> @@ -1346,18 +1351,10 @@ static struct msi_domain_info advk_msi_domain_info = {
>  static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
>  {
>  	struct device *dev = &pcie->pdev->dev;
> -	phys_addr_t msi_msg_phys;
>  
>  	raw_spin_lock_init(&pcie->msi_irq_lock);
>  	mutex_init(&pcie->msi_used_lock);
>  
> -	msi_msg_phys = virt_to_phys(&pcie->msi_msg);
> -
> -	advk_writel(pcie, lower_32_bits(msi_msg_phys),
> -		    PCIE_MSI_ADDR_LOW_REG);
> -	advk_writel(pcie, upper_32_bits(msi_msg_phys),
> -		    PCIE_MSI_ADDR_HIGH_REG);
> -
>  	pcie->msi_inner_domain =
>  		irq_domain_add_linear(NULL, MSI_IRQ_NUM,
>  				      &advk_msi_domain_ops, pcie);
> -- 
> 2.34.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 11/23] PCI: aardvark: Fix setting MSI address

[Marek Behún]

On Thu, 17 Feb 2022 11:14:52 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:

> > +	phys_addr_t msi_addr;
> >  	u32 reg;
> >  	int i;
> >  
> > @@ -561,6 +561,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> >  	reg |= LANE_COUNT_1;
> >  	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
> >  
> > +	/* Set MSI address */
> > +	msi_addr = virt_to_phys(pcie);  
> 
> Strictly speaking, msi_addr should be a pci_bus_addr_t, not a
> phys_addr_t, and virt_to_phys() doesn't return a bus address.

Dear Bjorn,

the problem here is that as far as we know currently there is no
function that converts a virtual address to pci_bus_addr_t like
virt_to_phys() does to convert to phys_addr_t.

On Armada 3720 there are PCIe Controller Address Decoder Registers,
which such a translating function would need to consult to do the
translation. But the default settings of these registers is to map PCIe
addresses 1 to 1 to physical addresses, and no driver changes these
registers.

Pali says that other drivers also use phys_addr_t, and most hardware
maps 1 to 1 by default.

So we think that until at least an API for such a function exists, we
shuld leave it as it is. I am not against converting the phys_addr_to
to a pci_bus_addr_t, but Pali thinks that for now we should leave even
that as it is, because the virt_to_phys() function returns phys_addr_t.

We can add a comment there explaining this, if you want.

What do you think?

Marek

Re: [PATCH v2 11/23] PCI: aardvark: Fix setting MSI address

[Bjorn Helgaas]

On Fri, Feb 18, 2022 at 03:43:29PM +0100, Marek Behún wrote:
> On Thu, 17 Feb 2022 11:14:52 -0600
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > > +	phys_addr_t msi_addr;
> > >  	u32 reg;
> > >  	int i;
> > >  
> > > @@ -561,6 +561,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> > >  	reg |= LANE_COUNT_1;
> > >  	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
> > >  
> > > +	/* Set MSI address */
> > > +	msi_addr = virt_to_phys(pcie);  
> > 
> > Strictly speaking, msi_addr should be a pci_bus_addr_t, not a
> > phys_addr_t, and virt_to_phys() doesn't return a bus address.
> 
> the problem here is that as far as we know currently there is no
> function that converts a virtual address to pci_bus_addr_t like
> virt_to_phys() does to convert to phys_addr_t.
> 
> On Armada 3720 there are PCIe Controller Address Decoder Registers,
> which such a translating function would need to consult to do the
> translation. But the default settings of these registers is to map PCIe
> addresses 1 to 1 to physical addresses, and no driver changes these
> registers.

The poorly-named pcibios_resource_to_bus() (I think the name is my
fault) is the way to convert a CPU physical address to a PCI bus
address.

This is implemented in terms of the host bridge windows and the
translation offset in struct resource_entry, which should be set up
via the pci_add_resource_offset() called from
devm_of_pci_get_host_bridge_resources().

> Pali says that other drivers also use phys_addr_t, and most hardware
> maps 1 to 1 by default.

Yes.  I think they're all technically incorrect.  Most systems do map
CPU phys == PCI bus, but I point it out because it's a case where
copying that pattern to new drivers will eventually bite us.

> So we think that until at least an API for such a function exists, we
> shuld leave it as it is. I am not against converting the phys_addr_to
> to a pci_bus_addr_t, but Pali thinks that for now we should leave even
> that as it is, because the virt_to_phys() function returns phys_addr_t.
> 
> We can add a comment there explaining this, if you want.
> 
> What do you think?
> 
> Marek

Re: [PATCH v2 11/23] PCI: aardvark: Fix setting MSI address

[Pali Rohár]

On Wednesday 23 February 2022 12:13:12 Bjorn Helgaas wrote:
> On Fri, Feb 18, 2022 at 03:43:29PM +0100, Marek Behún wrote:
> > On Thu, 17 Feb 2022 11:14:52 -0600
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > 
> > > > +	phys_addr_t msi_addr;
> > > >  	u32 reg;
> > > >  	int i;
> > > >  
> > > > @@ -561,6 +561,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> > > >  	reg |= LANE_COUNT_1;
> > > >  	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
> > > >  
> > > > +	/* Set MSI address */
> > > > +	msi_addr = virt_to_phys(pcie);  
> > > 
> > > Strictly speaking, msi_addr should be a pci_bus_addr_t, not a
> > > phys_addr_t, and virt_to_phys() doesn't return a bus address.
> > 
> > the problem here is that as far as we know currently there is no
> > function that converts a virtual address to pci_bus_addr_t like
> > virt_to_phys() does to convert to phys_addr_t.
> > 
> > On Armada 3720 there are PCIe Controller Address Decoder Registers,
> > which such a translating function would need to consult to do the
> > translation. But the default settings of these registers is to map PCIe
> > addresses 1 to 1 to physical addresses, and no driver changes these
> > registers.
> 
> The poorly-named pcibios_resource_to_bus() (I think the name is my
> fault) is the way to convert a CPU physical address to a PCI bus
> address.

But here it is needed to do something different. It is needed to do
inverse mapping of function which converts PCI bus address to CPU
physical address of CPU memory. So to converting CPU physical address of
CPU memory to PCI bus address from PCI bus point of view.

I think that this information is stored in dma_ranges member of struct
pci_host_bridge. But function pcibios_resource_to_bus() looks at the
->windows member which converts CPU physical address of PCI memory (not
CPU memory) to PCI bus address, which is something different. So
pcibios_resource_to_bus() would not work here and it may return
incorrect values (as PCI memory may be different from CPU point of view
and PCI bus point of view).

> This is implemented in terms of the host bridge windows and the
> translation offset in struct resource_entry, which should be set up
> via the pci_add_resource_offset() called from
> devm_of_pci_get_host_bridge_resources().
> 
> > Pali says that other drivers also use phys_addr_t, and most hardware
> > maps 1 to 1 by default.
> 
> Yes.  I think they're all technically incorrect.  Most systems do map
> CPU phys == PCI bus, but I point it out because it's a case where
> copying that pattern to new drivers will eventually bite us.

I agree, it is incorrect but I do not see a way how to do it correctly
because of missing function (which for pci-aardvark should return
identity).

> > So we think that until at least an API for such a function exists, we
> > shuld leave it as it is. I am not against converting the phys_addr_to
> > to a pci_bus_addr_t, but Pali thinks that for now we should leave even
> > that as it is, because the virt_to_phys() function returns phys_addr_t.
> > 
> > We can add a comment there explaining this, if you want.
> > 
> > What do you think?
> > 
> > Marek

Re: [PATCH v2 11/23] PCI: aardvark: Fix setting MSI address

[Bjorn Helgaas]

[+cc Rob]

On Thu, Feb 24, 2022 at 01:59:01PM +0100, Pali Rohár wrote:
> On Wednesday 23 February 2022 12:13:12 Bjorn Helgaas wrote:
> > On Fri, Feb 18, 2022 at 03:43:29PM +0100, Marek Behún wrote:
> > > On Thu, 17 Feb 2022 11:14:52 -0600
> > > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > 
> > > > > +	phys_addr_t msi_addr;
> > > > >  	u32 reg;
> > > > >  	int i;
> > > > >  
> > > > > @@ -561,6 +561,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> > > > >  	reg |= LANE_COUNT_1;
> > > > >  	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
> > > > >  
> > > > > +	/* Set MSI address */
> > > > > +	msi_addr = virt_to_phys(pcie);  
> > > > 
> > > > Strictly speaking, msi_addr should be a pci_bus_addr_t, not a
> > > > phys_addr_t, and virt_to_phys() doesn't return a bus address.

On second thought, probably a dma_addr_t, not a pci_bus_addr_t.

> > > the problem here is that as far as we know currently there is no
> > > function that converts a virtual address to pci_bus_addr_t like
> > > virt_to_phys() does to convert to phys_addr_t.
> > > 
> > > On Armada 3720 there are PCIe Controller Address Decoder Registers,
> > > which such a translating function would need to consult to do the
> > > translation. But the default settings of these registers is to map PCIe
> > > addresses 1 to 1 to physical addresses, and no driver changes these
> > > registers.
> > 
> > The poorly-named pcibios_resource_to_bus() (I think the name is my
> > fault) is the way to convert a CPU physical address to a PCI bus
> > address.
> 
> But here it is needed to do something different. It is needed to do
> inverse mapping of function which converts PCI bus address to CPU
> physical address of CPU memory. So to converting CPU physical address of
> CPU memory to PCI bus address from PCI bus point of view.
>
> I think that this information is stored in dma_ranges member of struct
> pci_host_bridge. But function pcibios_resource_to_bus() looks at the
> ->windows member which converts CPU physical address of PCI memory (not
> CPU memory) to PCI bus address, which is something different. So
> pcibios_resource_to_bus() would not work here and it may return
> incorrect values (as PCI memory may be different from CPU point of view
> and PCI bus point of view).

Oh, sorry, indeed you are correct and I was completely on the wrong
track.  pcibios_resource_to_bus() is what you need for doing MMIO --
CPU accesses to things on PCI.

MSI is the reverse.  From the device's point of view, an MSI is
basically a DMA as you allude to above, so I would expect the DMA API
to be involved somehow.

I do see a couple drivers using the DMA API for this:

  struct pcie_port {
    dma_addr_t msi_data;
  };

  dw_pcie_host_init
    pp->msi_data = dma_map_single_attrs(..., &pp->msi_msg, ...)
    dw_pcie_setup_rc
      dw_pcie_msi_init
        u64 msi_target = (u64)pp->msi_data;
	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));

  dw_pci_setup_msi_msg
    u64 msi_target = (u64)pp->msi_data;
    msg->address_lo = lower_32_bits(msi_target);

  -----------------------------------------------------------

  struct tegra_msi {
    dma_addr_t phys;
  };

  tegra_pcie_probe
    tegra_pcie_msi_setup
      msi->virt = dma_alloc_attrs(..., &msi->phys, ...);

  tegra_pcie_enable_msi
    afi_writel(pcie, msi->phys, ...);

  tegra_compose_msi_msg
    msg->address_low = lower_32_bits(msi->phys);

Bjorn

[PATCH v2 12/23] PCI: aardvark: Enable MSI-X support

[Marek Behún]

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

According to PCI 3.0 specification, sending both MSI and MSI-X interrupts
is done by DWORD memory write operation to doorbell message address. The
write operation for MSI has zero upper 16 bits and the MSI interrupt number
in the lower 16 bits, while the write operation for MSI-X contains a 32-bit
value from MSI-X table.

Since the driver only uses interrupt numbers from range 0..31, the upper
16 bits of the DWORD memory write operation to doorbell message address
are zero even for MSI-X interrupts. Thus we can enable MSI-X interrupts.

Testing proves that kernel can correctly receive MSI-X interrupts from PCIe
cards which supports both MSI and MSI-X interrupts.

Signed-off-by: Pali Rohár <pali@kernel.org>
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 79102704d82f..a892f22510da 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1344,7 +1344,7 @@ static struct irq_chip advk_msi_irq_chip = {
 
 static struct msi_domain_info advk_msi_domain_info = {
 	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
-		  MSI_FLAG_MULTI_PCI_MSI,
+		  MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX,
 	.chip	= &advk_msi_irq_chip,
 };
 
-- 
2.34.1

[PATCH v2 13/23] PCI: aardvark: Add support for ERR interrupt on emulated bridge

[Marek Behún]

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

ERR interrupt is triggered when corresponding bit is unmasked in both ISR0
and PCI_EXP_DEVCTL registers. Unmasking ERR bits in PCI_EXP_DEVCTL register
is not enough. This means that currently the ERR interrupt is never
triggered.

Unmask ERR bits in ISR0 register at driver probe time. ERR interrupt is not
triggered until ERR bits are unmasked also in PCI_EXP_DEVCTL register,
which is done by AER driver. So it is safe to unconditionally unmask all
ERR bits in aardvark probe.

Aardvark HW sets PCI_ERR_ROOT_AER_IRQ to zero and when corresponding bits
in ISR0 and PCI_EXP_DEVCTL are enabled, the HW triggers a generic interrupt
on GIC. Chain this interrupt to PCIe interrupt 0 with
generic_handle_domain_irq() to allow processing of ERR interrupts.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 35 ++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index a892f22510da..6fc6c3199954 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -99,6 +99,10 @@
 #define PCIE_MSG_PM_PME_MASK			BIT(7)
 #define PCIE_ISR0_MASK_REG			(CONTROL_BASE_ADDR + 0x44)
 #define     PCIE_ISR0_MSI_INT_PENDING		BIT(24)
+#define     PCIE_ISR0_CORR_ERR			BIT(11)
+#define     PCIE_ISR0_NFAT_ERR			BIT(12)
+#define     PCIE_ISR0_FAT_ERR			BIT(13)
+#define     PCIE_ISR0_ERR_MASK			GENMASK(13, 11)
 #define     PCIE_ISR0_INTX_ASSERT(val)		BIT(16 + (val))
 #define     PCIE_ISR0_INTX_DEASSERT(val)	BIT(20 + (val))
 #define     PCIE_ISR0_ALL_MASK			GENMASK(31, 0)
@@ -783,11 +787,15 @@ advk_pci_bridge_emul_base_conf_read(struct pci_bridge_emul *bridge,
 	case PCI_INTERRUPT_LINE: {
 		/*
 		 * From the whole 32bit register we support reading from HW only
-		 * one bit: PCI_BRIDGE_CTL_BUS_RESET.
+		 * two bits: PCI_BRIDGE_CTL_BUS_RESET and PCI_BRIDGE_CTL_SERR.
 		 * Other bits are retrieved only from emulated config buffer.
 		 */
 		__le32 *cfgspace = (__le32 *)&bridge->conf;
 		u32 val = le32_to_cpu(cfgspace[PCI_INTERRUPT_LINE / 4]);
+		if (advk_readl(pcie, PCIE_ISR0_MASK_REG) & PCIE_ISR0_ERR_MASK)
+			val &= ~(PCI_BRIDGE_CTL_SERR << 16);
+		else
+			val |= PCI_BRIDGE_CTL_SERR << 16;
 		if (advk_readl(pcie, PCIE_CORE_CTRL1_REG) & HOT_RESET_GEN)
 			val |= PCI_BRIDGE_CTL_BUS_RESET << 16;
 		else
@@ -817,6 +825,19 @@ advk_pci_bridge_emul_base_conf_write(struct pci_bridge_emul *bridge,
 		break;
 
 	case PCI_INTERRUPT_LINE:
+		/*
+		 * According to Figure 6-3: Pseudo Logic Diagram for Error
+		 * Message Controls in PCIe base specification, SERR# Enable bit
+		 * in Bridge Control register enable receiving of ERR_* messages
+		 */
+		if (mask & (PCI_BRIDGE_CTL_SERR << 16)) {
+			u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG);
+			if (new & (PCI_BRIDGE_CTL_SERR << 16))
+				val &= ~PCIE_ISR0_ERR_MASK;
+			else
+				val |= PCIE_ISR0_ERR_MASK;
+			advk_writel(pcie, val, PCIE_ISR0_MASK_REG);
+		}
 		if (mask & (PCI_BRIDGE_CTL_BUS_RESET << 16)) {
 			u32 val = advk_readl(pcie, PCIE_CORE_CTRL1_REG);
 			if (new & (PCI_BRIDGE_CTL_BUS_RESET << 16))
@@ -1462,6 +1483,18 @@ 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);
 
+	/* Process ERR interrupt */
+	if (isr0_status & PCIE_ISR0_ERR_MASK) {
+		advk_writel(pcie, PCIE_ISR0_ERR_MASK, PCIE_ISR0_REG);
+
+		/*
+		 * Aardvark HW returns zero for PCI_ERR_ROOT_AER_IRQ, so use
+		 * PCIe interrupt 0
+		 */
+		if (generic_handle_domain_irq(pcie->irq_domain, 0) == -EINVAL)
+			dev_err_ratelimited(&pcie->pdev->dev, "unhandled ERR IRQ\n");
+	}
+
 	/* Process MSI interrupts */
 	if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
 		advk_pcie_handle_msi(pcie);
-- 
2.34.1

[PATCH v2 14/23] PCI: aardvark: Fix reading PCI_EXP_RTSTA_PME bit on emulated bridge

[Marek Behún]

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

The emulated bridge returns incorrect value for PCI_EXP_RTSTA register
during readout in advk_pci_bridge_emul_pcie_conf_read() function: the
correct bit is BIT(16), but we are setting BIT(23), because the code
does
  *value = (isr0 & PCIE_MSG_PM_PME_MASK) << 16
where
  PCIE_MSG_PM_PME_MASK
is
  BIT(7).

The code should probably have been something like
  *value = (!!(isr0 & PCIE_MSG_PM_PME_MASK)) << 16,
but we are better of using an if() and using the proper macro for this
bit.

Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space")
Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 6fc6c3199954..8706a5f58eb5 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -876,7 +876,9 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
 	case PCI_EXP_RTSTA: {
 		u32 isr0 = advk_readl(pcie, PCIE_ISR0_REG);
 		u32 msglog = advk_readl(pcie, PCIE_MSG_LOG_REG);
-		*value = (isr0 & PCIE_MSG_PM_PME_MASK) << 16 | (msglog >> 16);
+		*value = msglog >> 16;
+		if (isr0 & PCIE_MSG_PM_PME_MASK)
+			*value |= PCI_EXP_RTSTA_PME;
 		return PCI_BRIDGE_EMUL_HANDLED;
 	}
 
-- 
2.34.1

[PATCH v2 15/23] PCI: aardvark: Optimize writing PCI_EXP_RTCTL_PMEIE and PCI_EXP_RTSTA_PME on emulated bridge

[Marek Behún]

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

To optimize advk_pci_bridge_emul_pcie_conf_write() code, touch
PCIE_ISR0_REG and PCIE_ISR0_MASK_REG registers only when it is really
needed, when processing PCI_EXP_RTCTL_PMEIE and PCI_EXP_RTSTA_PME bits.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 8706a5f58eb5..e7b9ca31c79c 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -934,19 +934,21 @@ advk_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge,
 			advk_pcie_wait_for_retrain(pcie);
 		break;
 
-	case PCI_EXP_RTCTL: {
+	case PCI_EXP_RTCTL:
 		/* Only mask/unmask PME interrupt */
-		u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG) &
-			~PCIE_MSG_PM_PME_MASK;
-		if ((new & PCI_EXP_RTCTL_PMEIE) == 0)
-			val |= PCIE_MSG_PM_PME_MASK;
-		advk_writel(pcie, val, PCIE_ISR0_MASK_REG);
+		if (mask & PCI_EXP_RTCTL_PMEIE) {
+			u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG);
+			if (new & PCI_EXP_RTCTL_PMEIE)
+				val &= ~PCIE_MSG_PM_PME_MASK;
+			else
+				val |= PCIE_MSG_PM_PME_MASK;
+			advk_writel(pcie, val, PCIE_ISR0_MASK_REG);
+		}
 		break;
-	}
 
 	case PCI_EXP_RTSTA:
-		new = (new & PCI_EXP_RTSTA_PME) >> 9;
-		advk_writel(pcie, new, PCIE_ISR0_REG);
+		if (new & PCI_EXP_RTSTA_PME)
+			advk_writel(pcie, PCIE_MSG_PM_PME_MASK, PCIE_ISR0_REG);
 		break;
 
 	case PCI_EXP_DEVCTL:
-- 
2.34.1

[PATCH v2 16/23] PCI: aardvark: Add support for PME interrupts

[Marek Behún]

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

Currently enabling PCI_EXP_RTSTA_PME bit in PCI_EXP_RTCTL register does
nothing. This is because PCIe PME driver expects to receive PCIe interrupt
defined in PCI_EXP_FLAGS_IRQ register, but aardvark hardware does not
trigger PCIe INTx/MSI interrupt for PME event, rather it triggers custom
aardvark interrupt which this driver is not processing yet.

Fix this issue by handling PME interrupt in advk_pcie_handle_int() and
chaining it to PCIe interrupt 0 with generic_handle_domain_irq() (since
aardvark sets PCI_EXP_FLAGS_IRQ to zero). With this change PCIe PME driver
finally starts receiving PME interrupt.

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

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index e7b9ca31c79c..3f2d570bfbb5 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1487,6 +1487,18 @@ 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);
 
+	/* Process PME interrupt */
+	if (isr0_status & PCIE_MSG_PM_PME_MASK) {
+		/*
+		 * Do not clear PME interrupt bit in ISR0, it is cleared by IRQ
+		 * receiver by writing to the PCI_EXP_RTSTA register of emulated
+		 * root bridge. Aardvark HW returns zero for PCI_EXP_FLAGS_IRQ,
+		 * so use PCIe interrupt 0.
+		 */
+		if (generic_handle_domain_irq(pcie->irq_domain, 0) == -EINVAL)
+			dev_err_ratelimited(&pcie->pdev->dev, "unhandled PME IRQ\n");
+	}
+
 	/* Process ERR interrupt */
 	if (isr0_status & PCIE_ISR0_ERR_MASK) {
 		advk_writel(pcie, PCIE_ISR0_ERR_MASK, PCIE_ISR0_REG);
-- 
2.34.1

[PATCH v2 17/23] PCI: aardvark: Fix support for PME requester on emulated bridge

[Marek Behún]

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

Enable aardvark PME interrupt unconditionally by unmasking it and read PME
requester ID to emulated bridge config space immediately after receiving
interrupt.

PME requester ID is stored in the PCIE_MSG_LOG_REG register, which contains
the last inbound message. So when new inbound message is received by HW
(including non-PM), the content in PCIE_MSG_LOG_REG register is replaced by
a new value.

PCIe specification mandates that subsequent PMEs are kept pending until the
PME Status Register bit is cleared by software by writing a 1b.

Support for masking/unmasking PME interrupt on emulated bridge via
PCI_EXP_RTCTL_PMEIE bit is now implemented only in emulated bridge config
space, to ensure that we do not miss any aardvark PME interrupt.

Reading of PCI_EXP_RTCAP and PCI_EXP_RTSTA registers is simplified as final
value is now always stored into emulated bridge config space by the
interrupt handler, so there is no need to implement support for these
registers in read_pcie callback.

Clearing of W1C bit PCI_EXP_RTSTA_PME is now also simplified as it is done
by pci-bridge-emul.c code for emulated bridge config space. So there is no
need to implement support for clearing this bit in write_pcie callback.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 91 +++++++++++++++------------
 1 file changed, 50 insertions(+), 41 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 3f2d570bfbb5..730ff1ffc952 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -591,6 +591,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
 	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
 
+	/* Unmask PME interrupt for processing of PME requester */
+	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
+	reg &= ~PCIE_MSG_PM_PME_MASK;
+	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
+
 	/* Enable summary interrupt for GIC SPI source */
 	reg = PCIE_IRQ_ALL_MASK & (~PCIE_IRQ_ENABLE_INTS_MASK);
 	advk_writel(pcie, reg, HOST_CTRL_INT_MASK_REG);
@@ -865,22 +870,11 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
 		*value = PCI_EXP_SLTSTA_PDS << 16;
 		return PCI_BRIDGE_EMUL_HANDLED;
 
-	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;
-	}
-
-	case PCI_EXP_RTSTA: {
-		u32 isr0 = advk_readl(pcie, PCIE_ISR0_REG);
-		u32 msglog = advk_readl(pcie, PCIE_MSG_LOG_REG);
-		*value = msglog >> 16;
-		if (isr0 & PCIE_MSG_PM_PME_MASK)
-			*value |= PCI_EXP_RTSTA_PME;
-		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.
+	 */
 
 	case PCI_EXP_LNKCAP: {
 		u32 val = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + reg);
@@ -934,22 +928,19 @@ advk_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge,
 			advk_pcie_wait_for_retrain(pcie);
 		break;
 
-	case PCI_EXP_RTCTL:
-		/* Only mask/unmask PME interrupt */
-		if (mask & PCI_EXP_RTCTL_PMEIE) {
-			u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG);
-			if (new & PCI_EXP_RTCTL_PMEIE)
-				val &= ~PCIE_MSG_PM_PME_MASK;
-			else
-				val |= PCIE_MSG_PM_PME_MASK;
-			advk_writel(pcie, val, PCIE_ISR0_MASK_REG);
-		}
+	case PCI_EXP_RTCTL: {
+		u16 rootctl = le16_to_cpu(bridge->pcie_conf.rootctl);
+		/* Only emulation of PMEIE and CRSSVE bits is provided */
+		rootctl &= PCI_EXP_RTCTL_PMEIE | PCI_EXP_RTCTL_CRSSVE;
+		bridge->pcie_conf.rootctl = cpu_to_le16(rootctl);
 		break;
+	}
 
-	case PCI_EXP_RTSTA:
-		if (new & PCI_EXP_RTSTA_PME)
-			advk_writel(pcie, PCIE_MSG_PM_PME_MASK, PCIE_ISR0_REG);
-		break;
+	/*
+	 * PCI_EXP_RTSTA is also supported, but does not need to be handled
+	 * here, because its value is stored in emulated config space buffer,
+	 * and we write it there when needed.
+	 */
 
 	case PCI_EXP_DEVCTL:
 	case PCI_EXP_DEVCTL2:
@@ -1452,6 +1443,32 @@ static void advk_pcie_remove_irq_domain(struct advk_pcie *pcie)
 	irq_domain_remove(pcie->irq_domain);
 }
 
+static void advk_pcie_handle_pme(struct advk_pcie *pcie)
+{
+	u32 requester = advk_readl(pcie, PCIE_MSG_LOG_REG) >> 16;
+
+	advk_writel(pcie, PCIE_MSG_PM_PME_MASK, PCIE_ISR0_REG);
+
+	/*
+	 * PCIE_MSG_LOG_REG contains the last inbound message, so store
+	 * the requester ID only when PME was not asserted yet.
+	 * Also do not trigger PME interrupt when PME is still asserted.
+	 */
+	if (!(le32_to_cpu(pcie->bridge.pcie_conf.rootsta) & PCI_EXP_RTSTA_PME)) {
+		pcie->bridge.pcie_conf.rootsta = cpu_to_le32(requester | PCI_EXP_RTSTA_PME);
+
+		/*
+		 * Trigger PME interrupt only if PMEIE bit in Root Control is set.
+		 * Aardvark HW returns zero for PCI_EXP_FLAGS_IRQ, so use PCIe interrupt 0.
+		 */
+		if (!(le16_to_cpu(pcie->bridge.pcie_conf.rootctl) & PCI_EXP_RTCTL_PMEIE))
+			return;
+
+		if (generic_handle_domain_irq(pcie->irq_domain, 0) == -EINVAL)
+			dev_err_ratelimited(&pcie->pdev->dev, "unhandled PME IRQ\n");
+	}
+}
+
 static void advk_pcie_handle_msi(struct advk_pcie *pcie)
 {
 	u32 msi_val, msi_mask, msi_status, msi_idx;
@@ -1487,17 +1504,9 @@ 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);
 
-	/* Process PME interrupt */
-	if (isr0_status & PCIE_MSG_PM_PME_MASK) {
-		/*
-		 * Do not clear PME interrupt bit in ISR0, it is cleared by IRQ
-		 * receiver by writing to the PCI_EXP_RTSTA register of emulated
-		 * root bridge. Aardvark HW returns zero for PCI_EXP_FLAGS_IRQ,
-		 * so use PCIe interrupt 0.
-		 */
-		if (generic_handle_domain_irq(pcie->irq_domain, 0) == -EINVAL)
-			dev_err_ratelimited(&pcie->pdev->dev, "unhandled PME IRQ\n");
-	}
+	/* Process PME interrupt as the first one to do not miss PME requester id */
+	if (isr0_status & PCIE_MSG_PM_PME_MASK)
+		advk_pcie_handle_pme(pcie);
 
 	/* Process ERR interrupt */
 	if (isr0_status & PCIE_ISR0_ERR_MASK) {
-- 
2.34.1

[PATCH v2 18/23] PCI: aardvark: Use separate INTA interrupt for emulated root bridge

[Marek Behún]

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

Emulated root bridge currently provides only one Legacy INTA interrupt
which is used for reporting PCIe PME and ERR events and handled by kernel
PCIe PME and AER drivers.

Aardvark HW reports these PME and ERR events separately, so there is no
need to mix real INTA interrupt and emulated INTA interrupt for PCIe PME
and AER drivers.

Register a new advk-RP (as in Root Port) irq chip and a new irq domain
for emulated root bridge and use this new separate irq domain for
providing INTA interrupt from emulated root bridge for PME and ERR events.

The real INTA interrupt from real devices is now separate.

A custom map_irq callback function on PCI host bridge structure is used to
allocate IRQ mapping for emulated root bridge from new irq domain. Original
callback of_irq_parse_and_map_pci() is used for all other devices as before.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 69 ++++++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 730ff1ffc952..ca519cadcdfe 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -274,6 +274,7 @@ struct advk_pcie {
 	} wins[OB_WIN_COUNT];
 	u8 wins_count;
 	int irq;
+	struct irq_domain *rp_irq_domain;
 	struct irq_domain *irq_domain;
 	struct irq_chip irq_chip;
 	raw_spinlock_t irq_lock;
@@ -1443,6 +1444,44 @@ static void advk_pcie_remove_irq_domain(struct advk_pcie *pcie)
 	irq_domain_remove(pcie->irq_domain);
 }
 
+static struct irq_chip advk_rp_irq_chip = {
+	.name = "advk-RP",
+};
+
+static int advk_pcie_rp_irq_map(struct irq_domain *h,
+				unsigned int virq, irq_hw_number_t hwirq)
+{
+	struct advk_pcie *pcie = h->host_data;
+
+	irq_set_chip_and_handler(virq, &advk_rp_irq_chip, handle_simple_irq);
+	irq_set_chip_data(virq, pcie);
+
+	return 0;
+}
+
+static const struct irq_domain_ops advk_pcie_rp_irq_domain_ops = {
+	.map = advk_pcie_rp_irq_map,
+	.xlate = irq_domain_xlate_onecell,
+};
+
+static int advk_pcie_init_rp_irq_domain(struct advk_pcie *pcie)
+{
+	pcie->rp_irq_domain = irq_domain_add_linear(NULL, 1,
+						    &advk_pcie_rp_irq_domain_ops,
+						    pcie);
+	if (!pcie->rp_irq_domain) {
+		dev_err(&pcie->pdev->dev, "Failed to add Root Port IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static void advk_pcie_remove_rp_irq_domain(struct advk_pcie *pcie)
+{
+	irq_domain_remove(pcie->rp_irq_domain);
+}
+
 static void advk_pcie_handle_pme(struct advk_pcie *pcie)
 {
 	u32 requester = advk_readl(pcie, PCIE_MSG_LOG_REG) >> 16;
@@ -1464,7 +1503,7 @@ static void advk_pcie_handle_pme(struct advk_pcie *pcie)
 		if (!(le16_to_cpu(pcie->bridge.pcie_conf.rootctl) & PCI_EXP_RTCTL_PMEIE))
 			return;
 
-		if (generic_handle_domain_irq(pcie->irq_domain, 0) == -EINVAL)
+		if (generic_handle_domain_irq(pcie->rp_irq_domain, 0) == -EINVAL)
 			dev_err_ratelimited(&pcie->pdev->dev, "unhandled PME IRQ\n");
 	}
 }
@@ -1516,7 +1555,7 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie)
 		 * Aardvark HW returns zero for PCI_ERR_ROOT_AER_IRQ, so use
 		 * PCIe interrupt 0
 		 */
-		if (generic_handle_domain_irq(pcie->irq_domain, 0) == -EINVAL)
+		if (generic_handle_domain_irq(pcie->rp_irq_domain, 0) == -EINVAL)
 			dev_err_ratelimited(&pcie->pdev->dev, "unhandled ERR IRQ\n");
 	}
 
@@ -1560,6 +1599,21 @@ static void advk_pcie_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static int advk_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
+{
+	struct advk_pcie *pcie = dev->bus->sysdata;
+
+	/*
+	 * Emulated root bridge has its own emulated irq chip and irq domain.
+	 * Argument pin is the INTx pin (1=INTA, 2=INTB, 3=INTC, 4=INTD) and
+	 * hwirq for irq_create_mapping() is indexed from zero.
+	 */
+	if (pci_is_root_bus(dev->bus))
+		return irq_create_mapping(pcie->rp_irq_domain, pin - 1);
+	else
+		return of_irq_parse_and_map_pci(dev, slot, pin);
+}
+
 static void __maybe_unused advk_pcie_disable_phy(struct advk_pcie *pcie)
 {
 	phy_power_off(pcie->phy);
@@ -1761,14 +1815,24 @@ static int advk_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = advk_pcie_init_rp_irq_domain(pcie);
+	if (ret) {
+		dev_err(dev, "Failed to initialize irq\n");
+		advk_pcie_remove_msi_irq_domain(pcie);
+		advk_pcie_remove_irq_domain(pcie);
+		return ret;
+	}
+
 	irq_set_chained_handler_and_data(pcie->irq, advk_pcie_irq_handler, pcie);
 
 	bridge->sysdata = pcie;
 	bridge->ops = &advk_pcie_ops;
+	bridge->map_irq = advk_pcie_map_irq;
 
 	ret = pci_host_probe(bridge);
 	if (ret < 0) {
 		irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
+		advk_pcie_remove_rp_irq_domain(pcie);
 		advk_pcie_remove_msi_irq_domain(pcie);
 		advk_pcie_remove_irq_domain(pcie);
 		return ret;
@@ -1820,6 +1884,7 @@ static int advk_pcie_remove(struct platform_device *pdev)
 	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
 
 	/* Remove IRQ domains */
+	advk_pcie_remove_rp_irq_domain(pcie);
 	advk_pcie_remove_msi_irq_domain(pcie);
 	advk_pcie_remove_irq_domain(pcie);
 
-- 
2.34.1

[PATCH v2 19/23] PCI: aardvark: Remove irq_mask_ack callback for INTx interrupts

[Marek Behún]

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

Callback for irq_mask_ack is the same as for irq_mask. As there is no
special handling for irq_ack, there is no need to define irq_mask_ack too.

Signed-off-by: Pali Rohár <pali@kernel.org>
Acked-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index ca519cadcdfe..a41e3e206478 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1422,7 +1422,6 @@ static int advk_pcie_init_irq_domain(struct advk_pcie *pcie)
 	}
 
 	irq_chip->irq_mask = advk_pcie_irq_mask;
-	irq_chip->irq_mask_ack = advk_pcie_irq_mask;
 	irq_chip->irq_unmask = advk_pcie_irq_unmask;
 
 	pcie->irq_domain =
-- 
2.34.1

[PATCH v2 20/23] PCI: aardvark: Don't mask irq when mapping

[Marek Behún]

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

By default, all Legacy INTx interrupts are masked, so there is no need to
mask this interrupt during irq_map callback.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index a41e3e206478..0e3dcd584f7e 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1339,7 +1339,6 @@ static int advk_pcie_irq_map(struct irq_domain *h,
 {
 	struct advk_pcie *pcie = h->host_data;
 
-	advk_pcie_irq_mask(irq_get_irq_data(virq));
 	irq_set_status_flags(virq, IRQ_LEVEL);
 	irq_set_chip_and_handler(virq, &pcie->irq_chip,
 				 handle_level_irq);
-- 
2.34.1

[PATCH v2 21/23] PCI: aardvark: Drop __maybe_unused from advk_pcie_disable_phy()

[Marek Behún]

This function is now always used in driver remove method, drop the
__maybe_unused attribute.

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 0e3dcd584f7e..360e2e3b3aa6 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1612,7 +1612,7 @@ static int advk_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 		return of_irq_parse_and_map_pci(dev, slot, pin);
 }
 
-static void __maybe_unused advk_pcie_disable_phy(struct advk_pcie *pcie)
+static void advk_pcie_disable_phy(struct advk_pcie *pcie)
 {
 	phy_power_off(pcie->phy);
 	phy_exit(pcie->phy);
-- 
2.34.1

[PATCH v2 22/23] PCI: aardvark: Update comment about link going down after link-up

[Marek Behún]

Update the comment about what happens when link goes down after we have
checked for link-up. If a PIO request is done while link-down, we have
a serious problem.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 360e2e3b3aa6..2c5cc929b94f 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1007,8 +1007,12 @@ static bool advk_pcie_valid_device(struct advk_pcie *pcie, struct pci_bus *bus,
 		return false;
 
 	/*
-	 * If the link goes down after we check for link-up, nothing bad
-	 * happens but the config access times out.
+	 * If the link goes down after we check for link-up, we have a problem:
+	 * if a PIO request is executed while link-down, the whole controller
+	 * gets stuck in a non-functional state, and even after link comes up
+	 * again, PIO requests won't work anymore, and a reset of the whole PCIe
+	 * controller is needed. Therefore we need to prevent sending PIO
+	 * requests while the link is down.
 	 */
 	if (!pci_is_root_bus(bus) && !advk_pcie_link_up(pcie))
 		return false;
-- 
2.34.1

[PATCH v2 23/23] PCI: aardvark: Make main irq_chip structure a static driver structure

[Marek Behún]

Marc Zyngier says [1] that we should use struct irq_chip as a global
static struct in the driver. Even though the structure currently
contains a dynamic member (parent_device), Marc says [2] that he plans
to kill it and make the structure completely static.

We have already converted others irq_chip structures in this driver in
this way, but we omitted this one because the .name member is
dynamically created from device's name, and the name is displayed in
sysfs, so changing it would break sysfs ABI.

The rationale for changing the name (to "advk-INT") in spite of sysfs
ABI, and thus allowing to convert to a static structure, is that after
the other changes we made in this series, the IRQ chip is basically
something different: it no logner generates ERR and PME interrupts (they
are generated by emulated bridge's rp_irq_chip).

[1] https://lore.kernel.org/linux-pci/877dbcvngf.wl-maz@kernel.org/
[2] https://lore.kernel.org/linux-pci/874k6gvkhz.wl-maz@kernel.org/

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 2c5cc929b94f..087a0b22d573 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -276,7 +276,6 @@ struct advk_pcie {
 	int irq;
 	struct irq_domain *rp_irq_domain;
 	struct irq_domain *irq_domain;
-	struct irq_chip irq_chip;
 	raw_spinlock_t irq_lock;
 	struct irq_domain *msi_domain;
 	struct irq_domain *msi_inner_domain;
@@ -1338,14 +1337,19 @@ static void advk_pcie_irq_unmask(struct irq_data *d)
 	raw_spin_unlock_irqrestore(&pcie->irq_lock, flags);
 }
 
+static struct irq_chip advk_irq_chip = {
+	.name		= "advk-INT",
+	.irq_mask	= advk_pcie_irq_mask,
+	.irq_unmask	= advk_pcie_irq_unmask,
+};
+
 static int advk_pcie_irq_map(struct irq_domain *h,
 			     unsigned int virq, irq_hw_number_t hwirq)
 {
 	struct advk_pcie *pcie = h->host_data;
 
 	irq_set_status_flags(virq, IRQ_LEVEL);
-	irq_set_chip_and_handler(virq, &pcie->irq_chip,
-				 handle_level_irq);
+	irq_set_chip_and_handler(virq, &advk_irq_chip, handle_level_irq);
 	irq_set_chip_data(virq, pcie);
 
 	return 0;
@@ -1404,7 +1408,6 @@ static int advk_pcie_init_irq_domain(struct advk_pcie *pcie)
 	struct device *dev = &pcie->pdev->dev;
 	struct device_node *node = dev->of_node;
 	struct device_node *pcie_intc_node;
-	struct irq_chip *irq_chip;
 	int ret = 0;
 
 	raw_spin_lock_init(&pcie->irq_lock);
@@ -1415,28 +1418,14 @@ static int advk_pcie_init_irq_domain(struct advk_pcie *pcie)
 		return -ENODEV;
 	}
 
-	irq_chip = &pcie->irq_chip;
-
-	irq_chip->name = devm_kasprintf(dev, GFP_KERNEL, "%s-irq",
-					dev_name(dev));
-	if (!irq_chip->name) {
-		ret = -ENOMEM;
-		goto out_put_node;
-	}
-
-	irq_chip->irq_mask = advk_pcie_irq_mask;
-	irq_chip->irq_unmask = advk_pcie_irq_unmask;
-
 	pcie->irq_domain =
 		irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX,
 				      &advk_pcie_irq_domain_ops, pcie);
 	if (!pcie->irq_domain) {
 		dev_err(dev, "Failed to get a INTx IRQ domain\n");
 		ret = -ENOMEM;
-		goto out_put_node;
 	}
 
-out_put_node:
 	of_node_put(pcie_intc_node);
 	return ret;
 }
-- 
2.34.1

Re: [PATCH v2 23/23] PCI: aardvark: Make main irq_chip structure a static driver structure

[Marc Zyngier]

On 2022-01-10 01:50, Marek Behún wrote:
> Marc Zyngier says [1] that we should use struct irq_chip as a global
> static struct in the driver. Even though the structure currently
> contains a dynamic member (parent_device), Marc says [2] that he plans
> to kill it and make the structure completely static.
> 
> We have already converted others irq_chip structures in this driver in
> this way, but we omitted this one because the .name member is
> dynamically created from device's name, and the name is displayed in
> sysfs, so changing it would break sysfs ABI.
> 
> The rationale for changing the name (to "advk-INT") in spite of sysfs
> ABI, and thus allowing to convert to a static structure, is that after
> the other changes we made in this series, the IRQ chip is basically
> something different: it no logner generates ERR and PME interrupts 
> (they
> are generated by emulated bridge's rp_irq_chip).

There is no 'is spite of the ABI'. If you don't understand why
we don't break the ABI, you have an even bigger problem.

So NAK to this patch, now and forever. Any change to the structure to
make it read-only must allow the preservation of the existing names
when they are generated by the driver.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 23/23] PCI: aardvark: Make main irq_chip structure a static driver structure

[Marek Behún]

On Mon, 10 Jan 2022 09:28:39 +0000
Marc Zyngier <maz@kernel.org> wrote:

> On 2022-01-10 01:50, Marek Behún wrote:
> > Marc Zyngier says [1] that we should use struct irq_chip as a global
> > static struct in the driver. Even though the structure currently
> > contains a dynamic member (parent_device), Marc says [2] that he plans
> > to kill it and make the structure completely static.
> > 
> > We have already converted others irq_chip structures in this driver in
> > this way, but we omitted this one because the .name member is
> > dynamically created from device's name, and the name is displayed in
> > sysfs, so changing it would break sysfs ABI.
> > 
> > The rationale for changing the name (to "advk-INT") in spite of sysfs
> > ABI, and thus allowing to convert to a static structure, is that after
> > the other changes we made in this series, the IRQ chip is basically
> > something different: it no logner generates ERR and PME interrupts 
> > (they
> > are generated by emulated bridge's rp_irq_chip).  
> 
> There is no 'is spite of the ABI'. If you don't understand why
> we don't break the ABI, you have an even bigger problem.
> 
> So NAK to this patch, now and forever. Any change to the structure to
> make it read-only must allow the preservation of the existing names
> when they are generated by the driver.

Dear Marc,

That's why I put it as a last patch here :)

I have the questions

1) the first is that this driver has only ever been used on Armada 37xx,
   where there has always been only one PCIe controller, and it's name
   always was d0070000.pcie, so the irq_chip was always called
   d0070000.pcie-irq.
   So we could theoretically infer from config options if we are
   building for Armada 37xx: if ARM64 and ARMADA_37XX_CLK config options
   are enabled, make the name d0070000.pcie-irq, otherwise advk-INT ?

2) I tried to look for the name d0070000.pcie-irq in /proc and /sysfs,
   but couldn't find it there (not in /proc/interrupts nor anywhere
   else). It is possible that I omitted something, or that with other
   PCIe card it will show up when corresponding driver is loaded.
   But theoretically, if I could prove that until now this never
   appeared anywhere in sysfs for some reason, then we could change it,
   right? Becuase that way it isn't sysfs ABI change.

Thanks.

Marek

Re: [PATCH v2 23/23] PCI: aardvark: Make main irq_chip structure a static driver structure

[Pali Rohár]

On Monday 10 January 2022 09:28:39 Marc Zyngier wrote:
> On 2022-01-10 01:50, Marek Behún wrote:
> > Marc Zyngier says [1] that we should use struct irq_chip as a global
> > static struct in the driver. Even though the structure currently
> > contains a dynamic member (parent_device), Marc says [2] that he plans
> > to kill it and make the structure completely static.
> > 
> > We have already converted others irq_chip structures in this driver in
> > this way, but we omitted this one because the .name member is
> > dynamically created from device's name, and the name is displayed in
> > sysfs, so changing it would break sysfs ABI.
> > 
> > The rationale for changing the name (to "advk-INT") in spite of sysfs
> > ABI, and thus allowing to convert to a static structure, is that after
> > the other changes we made in this series, the IRQ chip is basically
> > something different: it no logner generates ERR and PME interrupts (they
> > are generated by emulated bridge's rp_irq_chip).
> 
> There is no 'is spite of the ABI'. If you don't understand why
> we don't break the ABI, you have an even bigger problem.
> 
> So NAK to this patch, now and forever. Any change to the structure to
> make it read-only must allow the preservation of the existing names
> when they are generated by the driver.

Marc, you already presented that you do not like Armada 3720 platform
and that you do not care about it.

But please do not slowdown development for this platform.

Arguments about ABIs, breaking it and similar are not relevant here as
this current kernel implementation is broken. And has to be replaced by
a working one. We are doing on it for more than year.

It really does not make sense to try doing some backward compatibility
with something which is broken by design and does not work. It just take
lot of time without any value.

We really need to more forward and fix driver as in current state is
PCIe on Armada 3720 unusable.

Re: [PATCH v2 23/23] PCI: aardvark: Make main irq_chip structure a static driver structure

[Marc Zyngier]

On Mon, 10 Jan 2022 10:53:24 +0000,
Pali Rohár <pali@kernel.org> wrote:
> 
> On Monday 10 January 2022 09:28:39 Marc Zyngier wrote:
> > On 2022-01-10 01:50, Marek Behún wrote:
> > > Marc Zyngier says [1] that we should use struct irq_chip as a global
> > > static struct in the driver. Even though the structure currently
> > > contains a dynamic member (parent_device), Marc says [2] that he plans
> > > to kill it and make the structure completely static.
> > > 
> > > We have already converted others irq_chip structures in this driver in
> > > this way, but we omitted this one because the .name member is
> > > dynamically created from device's name, and the name is displayed in
> > > sysfs, so changing it would break sysfs ABI.
> > > 
> > > The rationale for changing the name (to "advk-INT") in spite of sysfs
> > > ABI, and thus allowing to convert to a static structure, is that after
> > > the other changes we made in this series, the IRQ chip is basically
> > > something different: it no logner generates ERR and PME interrupts (they
> > > are generated by emulated bridge's rp_irq_chip).
> > 
> > There is no 'is spite of the ABI'. If you don't understand why
> > we don't break the ABI, you have an even bigger problem.
> > 
> > So NAK to this patch, now and forever. Any change to the structure to
> > make it read-only must allow the preservation of the existing names
> > when they are generated by the driver.
> 
> Marc, you already presented that you do not like Armada 3720 platform
> and that you do not care about it.

What I like or not is irrelevant here. What I ask for is that
userspace ABIs are not broken.

> But please do not slowdown development for this platform.

That's quite an accusation.

> Arguments about ABIs, breaking it and similar are not relevant here as
> this current kernel implementation is broken. And has to be replaced by
> a working one. We are doing on it for more than year.
>
> It really does not make sense to try doing some backward compatibility
> with something which is broken by design and does not work. It just take
> lot of time without any value.
> 
> We really need to more forward and fix driver as in current state is
> PCIe on Armada 3720 unusable.

This patch doesn't fix anything. It has the potential to break
userspace, and I'm not having any of it. You may not care about
backward compatibility, but this is thankfully *not* your pet
playground.

You can claim that I am doing a bad job. In which case, feel free to
submit a patch removing me from the MAINTAINER file, and we can have
that discussion.

In the meantime, I will continue to oppose these kind of patches that
pretend to 'fix' things without adding any value.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 23/23] PCI: aardvark: Make main irq_chip structure a static driver structure

[Marek Behún]

On Mon, 10 Jan 2022 14:44:17 +0000
Marc Zyngier <maz@kernel.org> wrote:

> On Mon, 10 Jan 2022 10:53:24 +0000,
> Pali Rohár <pali@kernel.org> wrote:
> > 
> > On Monday 10 January 2022 09:28:39 Marc Zyngier wrote:  
> > > On 2022-01-10 01:50, Marek Behún wrote:  
> > > > Marc Zyngier says [1] that we should use struct irq_chip as a global
> > > > static struct in the driver. Even though the structure currently
> > > > contains a dynamic member (parent_device), Marc says [2] that he plans
> > > > to kill it and make the structure completely static.
> > > > 
> > > > We have already converted others irq_chip structures in this driver in
> > > > this way, but we omitted this one because the .name member is
> > > > dynamically created from device's name, and the name is displayed in
> > > > sysfs, so changing it would break sysfs ABI.
> > > > 
> > > > The rationale for changing the name (to "advk-INT") in spite of sysfs
> > > > ABI, and thus allowing to convert to a static structure, is that after
> > > > the other changes we made in this series, the IRQ chip is basically
> > > > something different: it no logner generates ERR and PME interrupts (they
> > > > are generated by emulated bridge's rp_irq_chip).  
> > > 
> > > There is no 'is spite of the ABI'. If you don't understand why
> > > we don't break the ABI, you have an even bigger problem.
> > > 
> > > So NAK to this patch, now and forever. Any change to the structure to
> > > make it read-only must allow the preservation of the existing names
> > > when they are generated by the driver.  
> > 
> > Marc, you already presented that you do not like Armada 3720 platform
> > and that you do not care about it.  
> 
> What I like or not is irrelevant here. What I ask for is that
> userspace ABIs are not broken.
> 
> > But please do not slowdown development for this platform.  
> 
> That's quite an accusation.
> 
> > Arguments about ABIs, breaking it and similar are not relevant here as
> > this current kernel implementation is broken. And has to be replaced by
> > a working one. We are doing on it for more than year.
> >
> > It really does not make sense to try doing some backward compatibility
> > with something which is broken by design and does not work. It just take
> > lot of time without any value.
> > 
> > We really need to more forward and fix driver as in current state is
> > PCIe on Armada 3720 unusable.  
> 
> This patch doesn't fix anything. It has the potential to break
> userspace, and I'm not having any of it. You may not care about
> backward compatibility, but this is thankfully *not* your pet
> playground.
> 
> You can claim that I am doing a bad job. In which case, feel free to
> submit a patch removing me from the MAINTAINER file, and we can have
> that discussion.
> 
> In the meantime, I will continue to oppose these kind of patches that
> pretend to 'fix' things without adding any value.
> 
> 	M.

Dear Marc,

that is why I put this patch as last patch of this series, so that it
could be potentially dropped.

I mostly agree with your points, Pali does not. Pali, let's not sabotage
ourselves with needless arguments. Marc, Pali means well, but sometimes
when he has different opinion, he can get quite argumentative. Let's
ignore this patch for now.

Marc, what do you think about the other patches? Did you have time to
look at them?

Thanks.

Marek

Re: (subset) [PATCH v2 00/23] PCI: aardvark controller fixes BATCH 4

[Lorenzo Pieralisi]

On Mon, 10 Jan 2022 02:49:55 +0100, Marek Behún wrote:
> this is v2 of fourth batch of fixes for the Aardvark PCIe controller
> driver.
> 
> Stuff is converted to new interrupt APIs and recommendations from Marc.
> Marc, could you look at these and acknowledge or comment?
> 
> This series mainly fixes and adds support for stuff around interrupts:
> the most important thing is fixing MSI support.
> 
> [...]

Applied to pci/aardvark, thanks!

[01/23] PCI: aardvark: Replace custom PCIE_CORE_INT_* macros with PCI_INTERRUPT_*
        https://git.kernel.org/lpieralisi/pci/c/1d86abf1f8
[02/23] PCI: aardvark: Fix reading MSI interrupt number
        https://git.kernel.org/lpieralisi/pci/c/805dfc18dd
[03/23] PCI: aardvark: Fix support for MSI interrupts
        https://git.kernel.org/lpieralisi/pci/c/b0b0b8b897
[04/23] PCI: aardvark: Rewrite IRQ code to chained IRQ handler
        https://git.kernel.org/lpieralisi/pci/c/1571d67dc1
[05/23] PCI: aardvark: Check return value of generic_handle_domain_irq() when processing INTx IRQ
        https://git.kernel.org/lpieralisi/pci/c/51f96e287c
[06/23] PCI: aardvark: Make MSI irq_chip structures static driver structures
        https://git.kernel.org/lpieralisi/pci/c/c3cb8e5183
[07/23] PCI: aardvark: Make msi_domain_info structure a static driver structure
        https://git.kernel.org/lpieralisi/pci/c/26bcd54e4a
[08/23] PCI: aardvark: Use dev_fwnode() instead of of_node_to_fwnode(dev->of_node)
        https://git.kernel.org/lpieralisi/pci/c/222af78532
[09/23] PCI: aardvark: Refactor unmasking summary MSI interrupt
        https://git.kernel.org/lpieralisi/pci/c/4689c09163
[10/23] PCI: aardvark: Add support for masking MSI interrupts
        https://git.kernel.org/lpieralisi/pci/c/e77d9c9069
[11/23] PCI: aardvark: Fix setting MSI address
        https://git.kernel.org/lpieralisi/pci/c/46ad3dc417
[12/23] PCI: aardvark: Enable MSI-X support
        https://git.kernel.org/lpieralisi/pci/c/754e449889
[13/23] PCI: aardvark: Add support for ERR interrupt on emulated bridge
        https://git.kernel.org/lpieralisi/pci/c/3ebfefa396
[14/23] PCI: aardvark: Fix reading PCI_EXP_RTSTA_PME bit on emulated bridge
        https://git.kernel.org/lpieralisi/pci/c/735f5ae49e
[15/23] PCI: aardvark: Optimize writing PCI_EXP_RTCTL_PMEIE and PCI_EXP_RTSTA_PME on emulated bridge
        https://git.kernel.org/lpieralisi/pci/c/7122bcb332
[16/23] PCI: aardvark: Add support for PME interrupts
        https://git.kernel.org/lpieralisi/pci/c/0fc75d8745
[17/23] PCI: aardvark: Fix support for PME requester on emulated bridge
        https://git.kernel.org/lpieralisi/pci/c/273ddd86d6
[18/23] PCI: aardvark: Use separate INTA interrupt for emulated root bridge
        https://git.kernel.org/lpieralisi/pci/c/815bc31368
[19/23] PCI: aardvark: Remove irq_mask_ack callback for INTx interrupts
        https://git.kernel.org/lpieralisi/pci/c/b08e5b53d1
[20/23] PCI: aardvark: Don't mask irq when mapping
        https://git.kernel.org/lpieralisi/pci/c/befa710001
[21/23] PCI: aardvark: Drop __maybe_unused from advk_pcie_disable_phy()
        https://git.kernel.org/lpieralisi/pci/c/0c36ab437e
[22/23] PCI: aardvark: Update comment about link going down after link-up
        https://git.kernel.org/lpieralisi/pci/c/92f4ffecc4

Thanks,
Lorenzo