[PATCH 0/7] PCI: aardvark: Interrupt fixes

[PATCH 0/7] PCI: aardvark: Interrupt fixes

[Pali Rohár]

Per Lorenzo's request [1] I'm resending [2] some other aardvark patches
which fixes just interrupts.

I addressed review comments from [2], updated patches and added Acked-by
tags.

[1] - https://lore.kernel.org/linux-pci/20210603151605.GA18917@lpieralisi/
[2] - https://lore.kernel.org/linux-pci/20210506153153.30454-1-pali@kernel.org/

Pali Rohár (7):
  PCI: aardvark: Do not touch status bits of masked interrupts in
    interrupt handler
  PCI: aardvark: Check for virq mapping when processing INTx IRQ
  PCI: aardvark: Remove irq_mask_ack callback for INTx interrupts
  PCI: aardvark: Don't mask irq when mapping
  PCI: aardvark: Fix support for MSI interrupts
  PCI: aardvark: Correctly clear and unmask all MSI interrupts
  PCI: aardvark: Fix setting MSI address

 drivers/pci/controller/pci-aardvark.c | 82 ++++++++++++++-------------
 1 file changed, 44 insertions(+), 38 deletions(-)

-- 
2.20.1

[PATCH 1/7] PCI: aardvark: Do not touch status bits of masked interrupts in interrupt handler

[Pali Rohár]

It is incorrect to clear status bits of masked interrupts.

The aardvark driver clears all status interrupt bits when no unmasked
status bit was set. When some unmasked bit was set then masked bits were
not cleared. Fix this so that masked bits are never cleared.

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

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index d4215da17a59..36fcc077ec72 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1210,11 +1210,8 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie)
 	isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG);
 	isr1_status = isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK);
 
-	if (!isr0_status && !isr1_status) {
-		advk_writel(pcie, isr0_val, PCIE_ISR0_REG);
-		advk_writel(pcie, isr1_val, PCIE_ISR1_REG);
+	if (!isr0_status && !isr1_status)
 		return;
-	}
 
 	/* Process MSI interrupts */
 	if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
-- 
2.20.1

[PATCH 2/7] PCI: aardvark: Check for virq mapping when processing INTx IRQ

[Pali Rohár]

It is possible that we receive spurious INTx interrupt. So add needed check
before calling generic_handle_irq() function.

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

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 36fcc077ec72..59f91fad2481 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1226,7 +1226,11 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie)
 			    PCIE_ISR1_REG);
 
 		virq = irq_find_mapping(pcie->irq_domain, i);
-		generic_handle_irq(virq);
+		if (virq)
+			generic_handle_irq(virq);
+		else
+			dev_err_ratelimited(&pcie->pdev->dev, "unexpected INT%c IRQ\n",
+					    (char)i+'A');
 	}
 }
 
-- 
2.20.1

Re: [PATCH 2/7] PCI: aardvark: Check for virq mapping when processing INTx IRQ

[Marc Zyngier]

On 2021-06-25 10:03, Pali Rohár wrote:
> It is possible that we receive spurious INTx interrupt. So add needed 
> check
> before calling generic_handle_irq() function.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/pci/controller/pci-aardvark.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c
> b/drivers/pci/controller/pci-aardvark.c
> index 36fcc077ec72..59f91fad2481 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -1226,7 +1226,11 @@ static void advk_pcie_handle_int(struct 
> advk_pcie *pcie)
>  			    PCIE_ISR1_REG);
> 
>  		virq = irq_find_mapping(pcie->irq_domain, i);
> -		generic_handle_irq(virq);
> +		if (virq)
> +			generic_handle_irq(virq);
> +		else
> +			dev_err_ratelimited(&pcie->pdev->dev, "unexpected INT%c IRQ\n",
> +					    (char)i+'A');
>  	}
>  }

Please use generic_handle_domain_irq() instead of irq_find_mapping()
and generic_handle_irq().

Thanks,

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

Re: [PATCH 2/7] PCI: aardvark: Check for virq mapping when processing INTx IRQ

[Pali Rohár]

On Friday 06 August 2021 09:29:02 Marc Zyngier wrote:
> On 2021-06-25 10:03, Pali Rohár wrote:
> > It is possible that we receive spurious INTx interrupt. So add needed
> > check
> > before calling generic_handle_irq() function.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <kabel@kernel.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c
> > b/drivers/pci/controller/pci-aardvark.c
> > index 36fcc077ec72..59f91fad2481 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -1226,7 +1226,11 @@ static void advk_pcie_handle_int(struct advk_pcie
> > *pcie)
> >  			    PCIE_ISR1_REG);
> > 
> >  		virq = irq_find_mapping(pcie->irq_domain, i);
> > -		generic_handle_irq(virq);
> > +		if (virq)
> > +			generic_handle_irq(virq);
> > +		else
> > +			dev_err_ratelimited(&pcie->pdev->dev, "unexpected INT%c IRQ\n",
> > +					    (char)i+'A');
> >  	}
> >  }
> 
> Please use generic_handle_domain_irq() instead of irq_find_mapping()
> and generic_handle_irq().

Ok! At the time when I was sending these patches there was no function
generic_handle_domain_irq().

As all these interrupt related patches are targeting also stable tress
where is no generic_handle_domain_irq() function too, it would be easier
for backporting to use irq_find_mapping() + generic_handle_irq(). And
later after applying all interrupt related patches, include a patch
which converts all usage to generic_handle_domain_irq().

> Thanks,
> 
>         M.
> -- 
> Jazz is not dead. It just smells funny...

Re: [PATCH 2/7] PCI: aardvark: Check for virq mapping when processing INTx IRQ

[Marc Zyngier]

On Mon, 23 Aug 2021 17:33:05 +0100,
Pali Rohár <pali@kernel.org> wrote:
> 
> On Friday 06 August 2021 09:29:02 Marc Zyngier wrote:
> > On 2021-06-25 10:03, Pali Rohár wrote:
> > > It is possible that we receive spurious INTx interrupt. So add needed
> > > check
> > > before calling generic_handle_irq() function.
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/pci/controller/pci-aardvark.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > b/drivers/pci/controller/pci-aardvark.c
> > > index 36fcc077ec72..59f91fad2481 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -1226,7 +1226,11 @@ static void advk_pcie_handle_int(struct advk_pcie
> > > *pcie)
> > >  			    PCIE_ISR1_REG);
> > > 
> > >  		virq = irq_find_mapping(pcie->irq_domain, i);
> > > -		generic_handle_irq(virq);
> > > +		if (virq)
> > > +			generic_handle_irq(virq);
> > > +		else
> > > +			dev_err_ratelimited(&pcie->pdev->dev, "unexpected INT%c IRQ\n",
> > > +					    (char)i+'A');
> > >  	}
> > >  }
> > 
> > Please use generic_handle_domain_irq() instead of irq_find_mapping()
> > and generic_handle_irq().
> 
> Ok! At the time when I was sending these patches there was no function
> generic_handle_domain_irq().
> 
> As all these interrupt related patches are targeting also stable tress
> where is no generic_handle_domain_irq() function too, it would be easier
> for backporting to use irq_find_mapping() + generic_handle_irq(). And
> later after applying all interrupt related patches, include a patch
> which converts all usage to generic_handle_domain_irq().

That's not how it works, and I have already done the work converting
all the existing users to the new API, already queued in -next.

Please send a patch that makes sense for upstream, and a different
patch that applies to previous version.

	M.

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

[PATCH 3/7] PCI: aardvark: Remove irq_mask_ack callback for INTx interrupts

[Pali Rohár]

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>
Reviewed-by: Marek Behún <kabel@kernel.org>
Acked-by: Marc Zyngier <maz@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 59f91fad2481..bf44d6bfd0ca 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1152,7 +1152,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.20.1

[PATCH 4/7] PCI: aardvark: Don't mask irq when mapping

[Pali Rohár]

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>
Reviewed-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 bf44d6bfd0ca..c4fa64a31733 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1060,7 +1060,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.20.1

[PATCH 5/7] PCI: aardvark: Fix support for MSI interrupts

[Pali Rohár]

MSI domain callback .alloc (implemented by advk_msi_irq_domain_alloc()
function) should return zero on success. Returning non-zero value indicates
failure. Fix return value of this function as in many cases it now returns
failure while allocating IRQs.

Aardvark hardware supports Multi-MSI and MSI_FLAG_MULTI_PCI_MSI is already
set. But when allocating MSI interrupt numbers for Multi-MSI, they 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). So use hwirq numbers allocated by the function
bitmap_find_free_region(), which are aligned. This needs an update in
advk_msi_irq_compose_msi_msg() and advk_pcie_handle_msi() functions to do
proper mapping between Linux virtual irq numbers and hwirq MSI inner domain
numbers.

Also the whole 16-bit MSI number is stored in the PCIE_MSI_PAYLOAD_REG
register, not only lower 8 bits. Fix reading content of this register.

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 change, NVMe disks just freeze booting Linux on Armada 3720
boards as linux nvme-core.c driver is waiting 60s for an interrupt.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-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 | 32 ++++++++++++++++-----------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index c4fa64a31733..0e81d89f307d 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -118,6 +118,7 @@
 #define PCIE_MSI_STATUS_REG			(CONTROL_BASE_ADDR + 0x58)
 #define PCIE_MSI_MASK_REG			(CONTROL_BASE_ADDR + 0x5C)
 #define PCIE_MSI_PAYLOAD_REG			(CONTROL_BASE_ADDR + 0x9C)
+#define     PCIE_MSI_DATA_MASK			GENMASK(15, 0)
 
 /* PCIe window configuration */
 #define OB_WIN_BASE_ADDR			0x4c00
@@ -981,7 +982,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,
@@ -998,15 +999,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,
@@ -1014,7 +1011,7 @@ static int advk_msi_irq_domain_alloc(struct irq_domain *domain,
 				    domain->host_data, handle_simple_irq,
 				    NULL, NULL);
 
-	return hwirq;
+	return 0;
 }
 
 static void advk_msi_irq_domain_free(struct irq_domain *domain,
@@ -1024,7 +1021,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);
 }
 
@@ -1176,6 +1173,7 @@ static void advk_pcie_handle_msi(struct advk_pcie *pcie)
 {
 	u32 msi_val, msi_mask, msi_status, msi_idx;
 	u16 msi_data;
+	int virq;
 
 	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
 	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
@@ -1185,9 +1183,17 @@ 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 from MSI inner domain
+		 */
 		advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG);
-		msi_data = advk_readl(pcie, PCIE_MSI_PAYLOAD_REG) & 0xFF;
-		generic_handle_irq(msi_data);
+		msi_data = advk_readl(pcie, PCIE_MSI_PAYLOAD_REG) & PCIE_MSI_DATA_MASK;
+		virq = irq_find_mapping(pcie->msi_inner_domain, msi_data);
+		if (virq)
+			generic_handle_irq(virq);
+		else
+			dev_err_ratelimited(&pcie->pdev->dev, "unexpected MSI 0x%04hx\n", msi_data);
 	}
 
 	advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING,
-- 
2.20.1

[PATCH 6/7] PCI: aardvark: Correctly clear and unmask all MSI interrupts

[Pali Rohár]

Define a new macro PCIE_MSI_ALL_MASK and use it for masking, unmasking and
clearing all MSI interrupts.

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

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 0e81d89f307d..7cad6d989f6c 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -117,6 +117,7 @@
 #define PCIE_MSI_ADDR_HIGH_REG			(CONTROL_BASE_ADDR + 0x54)
 #define PCIE_MSI_STATUS_REG			(CONTROL_BASE_ADDR + 0x58)
 #define PCIE_MSI_MASK_REG			(CONTROL_BASE_ADDR + 0x5C)
+#define     PCIE_MSI_ALL_MASK			GENMASK(31, 0)
 #define PCIE_MSI_PAYLOAD_REG			(CONTROL_BASE_ADDR + 0x9C)
 #define     PCIE_MSI_DATA_MASK			GENMASK(15, 0)
 
@@ -470,19 +471,22 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
 
 	/* Clear all interrupts */
+	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_STATUS_REG);
 	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_REG);
 	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 */
-	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, 0, PCIE_MSI_MASK_REG);
+	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);
@@ -1177,7 +1181,7 @@ static void advk_pcie_handle_msi(struct advk_pcie *pcie)
 
 	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
 	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
-	msi_status = msi_val & ~msi_mask;
+	msi_status = msi_val & ((~msi_mask) & PCIE_MSI_ALL_MASK);
 
 	for (msi_idx = 0; msi_idx < MSI_IRQ_NUM; msi_idx++) {
 		if (!(BIT(msi_idx) & msi_status))
-- 
2.20.1

Re: [PATCH 6/7] PCI: aardvark: Correctly clear and unmask all MSI interrupts

[Marc Zyngier]

On 2021-06-25 10:03, Pali Rohár wrote:
> Define a new macro PCIE_MSI_ALL_MASK and use it for masking, unmasking 
> and
> clearing all MSI interrupts.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/pci/controller/pci-aardvark.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c
> b/drivers/pci/controller/pci-aardvark.c
> index 0e81d89f307d..7cad6d989f6c 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -117,6 +117,7 @@
>  #define PCIE_MSI_ADDR_HIGH_REG			(CONTROL_BASE_ADDR + 0x54)
>  #define PCIE_MSI_STATUS_REG			(CONTROL_BASE_ADDR + 0x58)
>  #define PCIE_MSI_MASK_REG			(CONTROL_BASE_ADDR + 0x5C)
> +#define     PCIE_MSI_ALL_MASK			GENMASK(31, 0)
>  #define PCIE_MSI_PAYLOAD_REG			(CONTROL_BASE_ADDR + 0x9C)
>  #define     PCIE_MSI_DATA_MASK			GENMASK(15, 0)
> 
> @@ -470,19 +471,22 @@ static void advk_pcie_setup_hw(struct advk_pcie 
> *pcie)
>  	advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
> 
>  	/* Clear all interrupts */
> +	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_STATUS_REG);
>  	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_REG);
>  	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 */
> -	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, 0, PCIE_MSI_MASK_REG);
> +	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);
> @@ -1177,7 +1181,7 @@ static void advk_pcie_handle_msi(struct advk_pcie 
> *pcie)
> 
>  	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
>  	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
> -	msi_status = msi_val & ~msi_mask;
> +	msi_status = msi_val & ((~msi_mask) & PCIE_MSI_ALL_MASK);
> 
>  	for (msi_idx = 0; msi_idx < MSI_IRQ_NUM; msi_idx++) {
>  		if (!(BIT(msi_idx) & msi_status))

This still begs the question: why would you ever enable all
MSIs the first place? They should be enabled on request only.

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

Re: [PATCH 6/7] PCI: aardvark: Correctly clear and unmask all MSI interrupts

[Pali Rohár]

On Friday 06 August 2021 09:32:24 Marc Zyngier wrote:
> On 2021-06-25 10:03, Pali Rohár wrote:
> > Define a new macro PCIE_MSI_ALL_MASK and use it for masking, unmasking
> > and
> > clearing all MSI interrupts.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <kabel@kernel.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c
> > b/drivers/pci/controller/pci-aardvark.c
> > index 0e81d89f307d..7cad6d989f6c 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -117,6 +117,7 @@
> >  #define PCIE_MSI_ADDR_HIGH_REG			(CONTROL_BASE_ADDR + 0x54)
> >  #define PCIE_MSI_STATUS_REG			(CONTROL_BASE_ADDR + 0x58)
> >  #define PCIE_MSI_MASK_REG			(CONTROL_BASE_ADDR + 0x5C)
> > +#define     PCIE_MSI_ALL_MASK			GENMASK(31, 0)
> >  #define PCIE_MSI_PAYLOAD_REG			(CONTROL_BASE_ADDR + 0x9C)
> >  #define     PCIE_MSI_DATA_MASK			GENMASK(15, 0)
> > 
> > @@ -470,19 +471,22 @@ static void advk_pcie_setup_hw(struct advk_pcie
> > *pcie)
> >  	advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
> > 
> >  	/* Clear all interrupts */
> > +	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_STATUS_REG);
> >  	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_REG);
> >  	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 */
> > -	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, 0, PCIE_MSI_MASK_REG);
> > +	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);
> > @@ -1177,7 +1181,7 @@ static void advk_pcie_handle_msi(struct advk_pcie
> > *pcie)
> > 
> >  	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
> >  	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
> > -	msi_status = msi_val & ~msi_mask;
> > +	msi_status = msi_val & ((~msi_mask) & PCIE_MSI_ALL_MASK);
> > 
> >  	for (msi_idx = 0; msi_idx < MSI_IRQ_NUM; msi_idx++) {
> >  		if (!(BIT(msi_idx) & msi_status))
> 
> This still begs the question: why would you ever enable all
> MSIs the first place? They should be enabled on request only.

This is going to be fixed in followup patches. Just Lorenzo asked to
send smaller patch series, so I have to split that followup fix into
the next round.

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

Re: [PATCH 6/7] PCI: aardvark: Correctly clear and unmask all MSI interrupts

[Pali Rohár]

On Friday 06 August 2021 10:35:22 Pali Rohár wrote:
> On Friday 06 August 2021 09:32:24 Marc Zyngier wrote:
> > On 2021-06-25 10:03, Pali Rohár wrote:
> > > Define a new macro PCIE_MSI_ALL_MASK and use it for masking, unmasking
> > > and
> > > clearing all MSI interrupts.
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/pci/controller/pci-aardvark.c | 16 ++++++++++------
> > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > b/drivers/pci/controller/pci-aardvark.c
> > > index 0e81d89f307d..7cad6d989f6c 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -117,6 +117,7 @@
> > >  #define PCIE_MSI_ADDR_HIGH_REG			(CONTROL_BASE_ADDR + 0x54)
> > >  #define PCIE_MSI_STATUS_REG			(CONTROL_BASE_ADDR + 0x58)
> > >  #define PCIE_MSI_MASK_REG			(CONTROL_BASE_ADDR + 0x5C)
> > > +#define     PCIE_MSI_ALL_MASK			GENMASK(31, 0)
> > >  #define PCIE_MSI_PAYLOAD_REG			(CONTROL_BASE_ADDR + 0x9C)
> > >  #define     PCIE_MSI_DATA_MASK			GENMASK(15, 0)
> > > 
> > > @@ -470,19 +471,22 @@ static void advk_pcie_setup_hw(struct advk_pcie
> > > *pcie)
> > >  	advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
> > > 
> > >  	/* Clear all interrupts */
> > > +	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_STATUS_REG);
> > >  	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_REG);
> > >  	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 */
> > > -	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, 0, PCIE_MSI_MASK_REG);
> > > +	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);
> > > @@ -1177,7 +1181,7 @@ static void advk_pcie_handle_msi(struct advk_pcie
> > > *pcie)
> > > 
> > >  	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
> > >  	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
> > > -	msi_status = msi_val & ~msi_mask;
> > > +	msi_status = msi_val & ((~msi_mask) & PCIE_MSI_ALL_MASK);
> > > 
> > >  	for (msi_idx = 0; msi_idx < MSI_IRQ_NUM; msi_idx++) {
> > >  		if (!(BIT(msi_idx) & msi_status))
> > 
> > This still begs the question: why would you ever enable all
> > MSIs the first place? They should be enabled on request only.
> 
> This is going to be fixed in followup patches. Just Lorenzo asked to
> send smaller patch series, so I have to split that followup fix into
> the next round.

Just for reference, patch series which is handling it:
https://lore.kernel.org/linux-pci/20210815103624.19528-1-pali@kernel.org/t/#u

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

[PATCH 7/7] PCI: aardvark: Fix setting MSI address

[Pali Rohár]

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
registers with MSI address from advk_pcie_init_msi_irq_domain() function to
advk_pcie_setup_hw() function before enabling PCIE_CORE_CTRL2_MSI_ENABLE.

As part of this change, also remove unused variable 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

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Acked-by: Marc Zyngier <maz@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 7cad6d989f6c..84ecc418e6be 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -244,7 +244,6 @@ struct advk_pcie {
 	struct msi_domain_info msi_domain_info;
 	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;
@@ -403,6 +402,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;
 
@@ -465,6 +465,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;
@@ -982,10 +987,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;
 }
 
@@ -1080,7 +1085,6 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 	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);
 
@@ -1098,13 +1102,6 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 		MSI_FLAG_MULTI_PCI_MSI;
 	msi_di->chip = msi_ic;
 
-	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.20.1

Re: [PATCH 0/7] PCI: aardvark: Interrupt fixes

[Pali Rohár]

On Friday 25 June 2021 11:03:12 Pali Rohár wrote:
> Per Lorenzo's request [1] I'm resending [2] some other aardvark patches
> which fixes just interrupts.
> 
> I addressed review comments from [2], updated patches and added Acked-by
> tags.
> 
> [1] - https://lore.kernel.org/linux-pci/20210603151605.GA18917@lpieralisi/
> [2] - https://lore.kernel.org/linux-pci/20210506153153.30454-1-pali@kernel.org/

Could you review these patches?

> Pali Rohár (7):
>   PCI: aardvark: Do not touch status bits of masked interrupts in
>     interrupt handler
>   PCI: aardvark: Check for virq mapping when processing INTx IRQ
>   PCI: aardvark: Remove irq_mask_ack callback for INTx interrupts
>   PCI: aardvark: Don't mask irq when mapping
>   PCI: aardvark: Fix support for MSI interrupts
>   PCI: aardvark: Correctly clear and unmask all MSI interrupts
>   PCI: aardvark: Fix setting MSI address
> 
>  drivers/pci/controller/pci-aardvark.c | 82 ++++++++++++++-------------
>  1 file changed, 44 insertions(+), 38 deletions(-)
> 
> -- 
> 2.20.1
>