[PATCH 00/14] PCI: aardvark controller fixes BATCH 2

[PATCH 00/14] PCI: aardvark controller fixes BATCH 2

[Marek Behún]

Hi Lorenzo,

we are sending second batch of updates for Aardvark PCIe controller.

- patch 1 fixes pci-bridge-emul handling of W1C bits
- patches 2-9 fix MSI interrupts
- patch 10 enables MSI-X interrupts
- patches 11-14 fix registers in emulated PCI bridge

Marek & Pali

Marek Behún (3):
  PCI: pci-bridge-emul: Fix emulation of W1C bits
  PCI: aardvark: Fix return value of MSI domain .alloc() method
  PCI: aardvark: Read all 16-bits from PCIE_MSI_PAYLOAD_REG

Pali Rohár (11):
  PCI: aardvark: Fix support for MSI interrupts
  PCI: aardvark: Fix reading MSI interrupt number
  PCI: aardvark: Clear all MSIs at setup
  PCI: aardvark: Refactor unmasking summary MSI interrupt
  PCI: aardvark: Fix masking MSI interrupts
  PCI: aardvark: Fix setting MSI address
  PCI: aardvark: Enable MSI-X support
  PCI: aardvark: Fix support for bus mastering and PCI_COMMAND on
    emulated bridge
  PCI: aardvark: Set PCI Bridge Class Code to PCI Bridge
  PCI: aardvark: Fix support for PCI_BRIDGE_CTL_BUS_RESET on emulated
    bridge
  PCI: aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge

 drivers/pci/controller/pci-aardvark.c | 226 ++++++++++++++++++++------
 drivers/pci/pci-bridge-emul.c         |  13 ++
 2 files changed, 188 insertions(+), 51 deletions(-)

-- 
2.32.0

[PATCH 01/14] PCI: pci-bridge-emul: Fix emulation of W1C bits

[Marek Behún]

The pci_bridge_emul_conf_write() function correctly clears W1C bits in
cfgspace cache, but it does not inform the underlying implementation
about the clear request: the .write_op() method is given the value with
these bits cleared.

This is wrong if the .write_op() needs to know which bits were requested
to be cleared.

Fix the value to be passed into the .write_op() method to have requested
W1C bits set, so that it can clear them.

Both pci-bridge-emul users (mvebu and aardvark) are compatible with this
change.

Fixes: 23a5fba4d941 ("PCI: Introduce PCI bridge emulated config space common logic")
Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org
Cc: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/pci/pci-bridge-emul.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
index fdaf86a888b7..db97cddfc85e 100644
--- a/drivers/pci/pci-bridge-emul.c
+++ b/drivers/pci/pci-bridge-emul.c
@@ -431,8 +431,21 @@ int pci_bridge_emul_conf_write(struct pci_bridge_emul *bridge, int where,
 	/* Clear the W1C bits */
 	new &= ~((value << shift) & (behavior[reg / 4].w1c & mask));
 
+	/* Save the new value with the cleared W1C bits into the cfgspace */
 	cfgspace[reg / 4] = cpu_to_le32(new);
 
+	/*
+	 * Clear the W1C bits not specified by the write mask, so that the
+	 * write_op() does not clear them.
+	 */
+	new &= ~(behavior[reg / 4].w1c & ~mask);
+
+	/*
+	 * Set the W1C bits specified by the write mask, so that write_op()
+	 * knows about that they are to be cleared.
+	 */
+	new |= (value << shift) & (behavior[reg / 4].w1c & mask);
+
 	if (write_op)
 		write_op(bridge, reg, old, new, mask);
 
-- 
2.32.0

[PATCH 02/14] PCI: aardvark: Fix return value of MSI domain .alloc() method

[Marek Behún]

MSI domain callback .alloc() (implemented by advk_msi_irq_domain_alloc()
function) should return zero on success, since non-zero value indicates
failure.

When the driver was converted to generic MSI API in commit f21a8b1b6837
("PCI: aardvark: Move to MSI handling using generic MSI support"), it
was converted so that it returns hwirq number.

Fix this.

Fixes: f21a8b1b6837 ("PCI: aardvark: Move to MSI handling using generic MSI support")
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/pci/controller/pci-aardvark.c | 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 10476c00b312..b45ff2911c80 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1138,7 +1138,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,
-- 
2.32.0

Re: [PATCH 02/14] PCI: aardvark: Fix return value of MSI domain .alloc() method

[Lorenzo Pieralisi]

On Tue, Oct 12, 2021 at 06:41:33PM +0200, Marek Behún wrote:
> MSI domain callback .alloc() (implemented by advk_msi_irq_domain_alloc()
> function) should return zero on success, since non-zero value indicates
> failure.

AFAICS the .alloc() method is called in:

irq_domain_alloc_irqs_hierarchy()

which in turn is called by:

__irq_domain_alloc_irqs() -> that checks (ret < 0)

irq_domain_push_irq() -> that checks for rv != 0

irq_domain_alloc_irqs_parent() called by many drivers and also
by msi_domain_alloc() (that checks ret < 0)

This patch is fine, I am just asking, given the above:

- How did you detect it (given that aardvark would not fail ret < 0) ?
- Should we consolidate the .alloc() return value handling ?

Apologies if I missed something in the IRQ domain code.

Lorenzo

> When the driver was converted to generic MSI API in commit f21a8b1b6837
> ("PCI: aardvark: Move to MSI handling using generic MSI support"), it
> was converted so that it returns hwirq number.
> 
> Fix this.
> 
> Fixes: f21a8b1b6837 ("PCI: aardvark: Move to MSI handling using generic MSI support")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/pci/controller/pci-aardvark.c | 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 10476c00b312..b45ff2911c80 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -1138,7 +1138,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,
> -- 
> 2.32.0
> 

Re: [PATCH 02/14] PCI: aardvark: Fix return value of MSI domain .alloc() method

[Pali Rohár]

On Wednesday 27 October 2021 12:26:53 Lorenzo Pieralisi wrote:
> On Tue, Oct 12, 2021 at 06:41:33PM +0200, Marek Behún wrote:
> > MSI domain callback .alloc() (implemented by advk_msi_irq_domain_alloc()
> > function) should return zero on success, since non-zero value indicates
> > failure.
> 
> AFAICS the .alloc() method is called in:
> 
> irq_domain_alloc_irqs_hierarchy()
> 
> which in turn is called by:
> 
> __irq_domain_alloc_irqs() -> that checks (ret < 0)
> 
> irq_domain_push_irq() -> that checks for rv != 0
> 
> irq_domain_alloc_irqs_parent() called by many drivers and also
> by msi_domain_alloc() (that checks ret < 0)
> 
> This patch is fine, I am just asking, given the above:
> 
> - How did you detect it (given that aardvark would not fail ret < 0) ?

Last year we have detected that Multi-MSI interrupts do not work
correctly and we have found that return value is incorrect during
debugging. Other drivers are returning 0.

> - Should we consolidate the .alloc() return value handling ?
> 
> Apologies if I missed something in the IRQ domain code.
> 
> Lorenzo
> 
> > When the driver was converted to generic MSI API in commit f21a8b1b6837
> > ("PCI: aardvark: Move to MSI handling using generic MSI support"), it
> > was converted so that it returns hwirq number.
> > 
> > Fix this.
> > 
> > Fixes: f21a8b1b6837 ("PCI: aardvark: Move to MSI handling using generic MSI support")
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <kabel@kernel.org>
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 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 10476c00b312..b45ff2911c80 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -1138,7 +1138,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,
> > -- 
> > 2.32.0
> > 

[PATCH 03/14] PCI: aardvark: Read all 16-bits from PCIE_MSI_PAYLOAD_REG

[Marek Behún]

The PCIE_MSI_PAYLOAD_REG contains 16-bit MSI number, not only lower
8 bits. Fix reading content of this register and add a comment
describing the access to this register.

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

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index b45ff2911c80..389ebba1dd9b 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -119,6 +119,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
@@ -1319,8 +1320,12 @@ 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) & 0xFF;
+		msi_data = advk_readl(pcie, PCIE_MSI_PAYLOAD_REG) & PCIE_MSI_DATA_MASK;
 		generic_handle_irq(msi_data);
 	}
 
-- 
2.32.0

[PATCH 04/14] 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.
Properly map Linux virtual irq numbers and hwirq MSI inner domain numbers
in advk_msi_irq_compose_msi_msg() and advk_pcie_handle_msi().

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.

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

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 389ebba1dd9b..4f232ee1f115 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1106,7 +1106,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,
@@ -1123,15 +1123,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,
@@ -1149,7 +1145,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);
 }
 
@@ -1311,6 +1307,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);
@@ -1326,7 +1323,11 @@ static void advk_pcie_handle_msi(struct advk_pcie *pcie)
 		 */
 		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);
+		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.32.0

[PATCH 05/14] 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>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org
---
Previous patches also touch PCIE_MSI_PAYLOAD_REG (adding comments, fixing
reading of the register) and now we are removing it's usage completely.
This is because we wanted the patches to be atomic and for the kernel to
contain the explanation and comments in it's history.
---
 drivers/pci/controller/pci-aardvark.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 4f232ee1f115..735fd70e8dc3 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1306,7 +1306,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;
 	int virq;
 
 	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
@@ -1317,17 +1316,13 @@ 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;
-		virq = irq_find_mapping(pcie->msi_inner_domain, msi_data);
+
+		virq = irq_find_mapping(pcie->msi_inner_domain, msi_idx);
 		if (virq)
 			generic_handle_irq(virq);
 		else
-			dev_err_ratelimited(&pcie->pdev->dev, "unexpected MSI 0x%04hx\n", msi_data);
+			dev_err_ratelimited(&pcie->pdev->dev, "unexpected MSI 0x%02x\n", msi_idx);
 	}
 
 	advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING,
-- 
2.32.0

[PATCH 06/14] PCI: aardvark: Clear all MSIs at setup

[Marek Behún]

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

We already clear all the other interrupts (ISR0, ISR1, HOST_CTRL_INT).

Define a new macro PCIE_MSI_ALL_MASK and do the same clearing for MSIs,
to ensure that we don't start receiving spurious interrupts.

Use this new mask in advk_pcie_handle_msi();

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

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 735fd70e8dc3..a5fbe24b9612 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -118,6 +118,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)
 
@@ -548,6 +549,7 @@ 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);
@@ -560,7 +562,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	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);
 
 	/* Enable summary interrupt for GIC SPI source */
 	reg = PCIE_IRQ_ALL_MASK & (~PCIE_IRQ_ENABLE_INTS_MASK);
@@ -1310,7 +1312,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.32.0

[PATCH 07/14] 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>
Reviewed-by: Marek Behún <kabel@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 a5fbe24b9612..ab0a0864a300 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -555,15 +555,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.32.0

[PATCH 08/14] PCI: aardvark: Fix 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.

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

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index ab0a0864a300..1adb4c4b11b5 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -282,6 +282,7 @@ struct advk_pcie {
 	struct irq_domain *msi_inner_domain;
 	struct irq_chip msi_bottom_irq_chip;
 	struct irq_chip msi_irq_chip;
+	raw_spinlock_t msi_irq_lock;
 	struct msi_domain_info msi_domain_info;
 	DECLARE_BITMAP(msi_used, MSI_IRQ_NUM);
 	struct mutex msi_used_lock;
@@ -554,12 +555,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);
@@ -1119,6 +1118,46 @@ 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 int advk_msi_irq_domain_alloc(struct irq_domain *domain,
 				     unsigned int virq,
 				     unsigned int nr_irqs, void *args)
@@ -1213,6 +1252,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 	struct msi_domain_info *msi_di;
 	phys_addr_t msi_msg_phys;
 
+	raw_spin_lock_init(&pcie->msi_irq_lock);
 	mutex_init(&pcie->msi_used_lock);
 
 	bottom_ic = &pcie->msi_bottom_irq_chip;
@@ -1220,9 +1260,13 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 	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;
+	bottom_ic->irq_mask = advk_msi_irq_mask;
+	bottom_ic->irq_unmask = advk_msi_irq_unmask;
 
 	msi_ic = &pcie->msi_irq_chip;
 	msi_ic->name = "advk-MSI";
+	msi_ic->irq_mask = advk_msi_top_irq_mask;
+	msi_ic->irq_unmask = advk_msi_top_irq_unmask;
 
 	msi_di = &pcie->msi_domain_info;
 	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
-- 
2.32.0

[PATCH 09/14] 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>
Reviewed-by: Marek Behún <kabel@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 1adb4c4b11b5..b703b271c6b1 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -286,7 +286,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;
@@ -481,6 +480,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;
 
@@ -544,6 +544,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;
@@ -1105,10 +1110,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;
 }
 
@@ -1250,7 +1255,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;
 
 	raw_spin_lock_init(&pcie->msi_irq_lock);
 	mutex_init(&pcie->msi_used_lock);
@@ -1273,13 +1277,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.32.0

[PATCH 10/14] 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>
Reviewed-by: Marek Behún <kabel@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 b703b271c6b1..337b61508799 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1274,7 +1274,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 
 	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_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
 	msi_di->chip = msi_ic;
 
 	pcie->msi_inner_domain =
-- 
2.32.0

Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Lorenzo Pieralisi]

On Tue, Oct 12, 2021 at 06:41:41PM +0200, Marek Behún wrote:
> 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.

It is the controller driver that defines the MSI-X data field yes, what
I don't get is why we have to add this comment in the commit log.

Basically Aardvark can support MSI-X up to 32 MSI-X vectors and you
are enabling them with this patch.

Is there anything *else* I am missing wrt 16-bit/32-bit data fields
that we need to know ?

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

I don't understand what you want to convey with this commit log.

To me, the whole comment does not add anything (if I understood it),
please let me know what you want to express with it.

To me this patch enables MSI-X support because the HW can support them,
that's it.

Lorenzo

> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/pci/controller/pci-aardvark.c | 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 b703b271c6b1..337b61508799 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -1274,7 +1274,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
>  
>  	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_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
>  	msi_di->chip = msi_ic;
>  
>  	pcie->msi_inner_domain =
> -- 
> 2.32.0
> 

Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Pali Rohár]

On Wednesday 27 October 2021 15:12:46 Lorenzo Pieralisi wrote:
> On Tue, Oct 12, 2021 at 06:41:41PM +0200, Marek Behún wrote:
> > 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.
> 
> It is the controller driver that defines the MSI-X data field yes, what
> I don't get is why we have to add this comment in the commit log.
> 
> Basically Aardvark can support MSI-X up to 32 MSI-X vectors and you
> are enabling them with this patch.
> 
> Is there anything *else* I am missing wrt 16-bit/32-bit data fields
> that we need to know ?
> 
> > Testing proves that kernel can correctly receive MSI-X interrupts from
> > PCIe cards which supports both MSI and MSI-X interrupts.
> 
> I don't understand what you want to convey with this commit log.
> 
> To me, the whole comment does not add anything (if I understood it),
> please let me know what you want to express with it.
> 
> To me this patch enables MSI-X support because the HW can support them,
> that's it.

My understanding is that MSI-X by definition uses 32-bit write
operations to doorbell address and so, HW needs to support catching of
32-bit write operations.

Aardvark hw seems to support only 16-bit write operation to doorbell
address. But our testing proved that hw can catch also lower 16-bits of
32-bit write operation to doorbell address.

So if driver enforces that every 32-bit write operation to doorbell
address would have upper 16-bit zeroed then MSI-X should work.

In commit message I originally tried to explain it that after applying
all previous patches which are fixing MSI and Multi-MSI support (part of
them is enforcement to use only MSI numbers 0..31), it makes driver
compatible with also MSI-X interrupts.

If you want to rewrite commit message, let us know, there is no problem.

> Lorenzo
> 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <kabel@kernel.org>
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 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 b703b271c6b1..337b61508799 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -1274,7 +1274,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
> >  
> >  	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_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
> >  	msi_di->chip = msi_ic;
> >  
> >  	pcie->msi_inner_domain =
> > -- 
> > 2.32.0
> > 

Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Lorenzo Pieralisi]

On Wed, Oct 27, 2021 at 04:23:07PM +0200, Pali Rohár wrote:
> On Wednesday 27 October 2021 15:12:46 Lorenzo Pieralisi wrote:
> > On Tue, Oct 12, 2021 at 06:41:41PM +0200, Marek Behún wrote:
> > > 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.
> > 
> > It is the controller driver that defines the MSI-X data field yes, what
> > I don't get is why we have to add this comment in the commit log.
> > 
> > Basically Aardvark can support MSI-X up to 32 MSI-X vectors and you
> > are enabling them with this patch.
> > 
> > Is there anything *else* I am missing wrt 16-bit/32-bit data fields
> > that we need to know ?
> > 
> > > Testing proves that kernel can correctly receive MSI-X interrupts from
> > > PCIe cards which supports both MSI and MSI-X interrupts.
> > 
> > I don't understand what you want to convey with this commit log.
> > 
> > To me, the whole comment does not add anything (if I understood it),
> > please let me know what you want to express with it.
> > 
> > To me this patch enables MSI-X support because the HW can support them,
> > that's it.
> 
> My understanding is that MSI-X by definition uses 32-bit write
> operations to doorbell address and so, HW needs to support catching of
> 32-bit write operations.
> 
> Aardvark hw seems to support only 16-bit write operation to doorbell
> address. But our testing proved that hw can catch also lower 16-bits of
> 32-bit write operation to doorbell address.
> 
> So if driver enforces that every 32-bit write operation to doorbell
> address would have upper 16-bit zeroed then MSI-X should work.

That's clearer than the current commit log.

> In commit message I originally tried to explain it that after applying
> all previous patches which are fixing MSI and Multi-MSI support (part of
> them is enforcement to use only MSI numbers 0..31), it makes driver
> compatible with also MSI-X interrupts.
> 
> If you want to rewrite commit message, let us know, there is no problem.

I think we should.

> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Reviewed-by: Marek Behún <kabel@kernel.org>

By the way, this tag should be removed. Marek signed it off, that
applies to other patches in this series as well.

Lorenzo

> > > 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 b703b271c6b1..337b61508799 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -1274,7 +1274,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
> > >  
> > >  	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_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
> > >  	msi_di->chip = msi_ic;
> > >  
> > >  	pcie->msi_inner_domain =
> > > -- 
> > > 2.32.0
> > > 

Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Pali Rohár]

On Thursday 28 October 2021 12:08:35 Lorenzo Pieralisi wrote:
> On Wed, Oct 27, 2021 at 04:23:07PM +0200, Pali Rohár wrote:
> > On Wednesday 27 October 2021 15:12:46 Lorenzo Pieralisi wrote:
> > > On Tue, Oct 12, 2021 at 06:41:41PM +0200, Marek Behún wrote:
> > > > 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.
> > > 
> > > It is the controller driver that defines the MSI-X data field yes, what
> > > I don't get is why we have to add this comment in the commit log.
> > > 
> > > Basically Aardvark can support MSI-X up to 32 MSI-X vectors and you
> > > are enabling them with this patch.
> > > 
> > > Is there anything *else* I am missing wrt 16-bit/32-bit data fields
> > > that we need to know ?
> > > 
> > > > Testing proves that kernel can correctly receive MSI-X interrupts from
> > > > PCIe cards which supports both MSI and MSI-X interrupts.
> > > 
> > > I don't understand what you want to convey with this commit log.
> > > 
> > > To me, the whole comment does not add anything (if I understood it),
> > > please let me know what you want to express with it.
> > > 
> > > To me this patch enables MSI-X support because the HW can support them,
> > > that's it.
> > 
> > My understanding is that MSI-X by definition uses 32-bit write
> > operations to doorbell address and so, HW needs to support catching of
> > 32-bit write operations.
> > 
> > Aardvark hw seems to support only 16-bit write operation to doorbell
> > address. But our testing proved that hw can catch also lower 16-bits of
> > 32-bit write operation to doorbell address.
> > 
> > So if driver enforces that every 32-bit write operation to doorbell
> > address would have upper 16-bit zeroed then MSI-X should work.
> 
> That's clearer than the current commit log.
> 
> > In commit message I originally tried to explain it that after applying
> > all previous patches which are fixing MSI and Multi-MSI support (part of
> > them is enforcement to use only MSI numbers 0..31), it makes driver
> > compatible with also MSI-X interrupts.
> > 
> > If you want to rewrite commit message, let us know, there is no problem.
> 
> I think we should.
> 
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > Reviewed-by: Marek Behún <kabel@kernel.org>
> 
> By the way, this tag should be removed. Marek signed it off, that
> applies to other patches in this series as well.

Ok! Is this the only issue with this patch series? Or something other
needs to be fixed?

> Lorenzo
> 
> > > > 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 b703b271c6b1..337b61508799 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -1274,7 +1274,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
> > > >  
> > > >  	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_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
> > > >  	msi_di->chip = msi_ic;
> > > >  
> > > >  	pcie->msi_inner_domain =
> > > > -- 
> > > > 2.32.0
> > > > 

Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Lorenzo Pieralisi]

On Thu, Oct 28, 2021 at 01:13:02PM +0200, Pali Rohár wrote:

[...]

> > > In commit message I originally tried to explain it that after applying
> > > all previous patches which are fixing MSI and Multi-MSI support (part of
> > > them is enforcement to use only MSI numbers 0..31), it makes driver
> > > compatible with also MSI-X interrupts.
> > > 
> > > If you want to rewrite commit message, let us know, there is no problem.
> > 
> > I think we should.
> > 
> > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > 
> > By the way, this tag should be removed. Marek signed it off, that
> > applies to other patches in this series as well.
> 
> Ok! Is this the only issue with this patch series? Or something other
> needs to be fixed?

The series looks fine to me - only thing for patch[4-10] I'd like
to have evidence MarcZ is happy with the approach (you have an
ACK on patch 9, for other patches if they were discussed on
ML add a Link: tag to the relevant discussion please).

I am trying to keep the IRQ domain management in PCI controller
drivers in check, that's why I am asking.

I can definitely pull [1-3] and [11-14] since they look good to me.

Thanks,
Lorenzo

> 
> > Lorenzo
> > 
> > > > > 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 b703b271c6b1..337b61508799 100644
> > > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > > @@ -1274,7 +1274,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
> > > > >  
> > > > >  	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_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
> > > > >  	msi_di->chip = msi_ic;
> > > > >  
> > > > >  	pcie->msi_inner_domain =
> > > > > -- 
> > > > > 2.32.0
> > > > > 

Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Pali Rohár]

On Thursday 28 October 2021 12:30:30 Lorenzo Pieralisi wrote:
> On Thu, Oct 28, 2021 at 01:13:02PM +0200, Pali Rohár wrote:
> 
> [...]
> 
> > > > In commit message I originally tried to explain it that after applying
> > > > all previous patches which are fixing MSI and Multi-MSI support (part of
> > > > them is enforcement to use only MSI numbers 0..31), it makes driver
> > > > compatible with also MSI-X interrupts.
> > > > 
> > > > If you want to rewrite commit message, let us know, there is no problem.
> > > 
> > > I think we should.
> > > 
> > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > 
> > > By the way, this tag should be removed. Marek signed it off, that
> > > applies to other patches in this series as well.
> > 
> > Ok! Is this the only issue with this patch series? Or something other
> > needs to be fixed?
> 
> The series looks fine to me - only thing for patch[4-10] I'd like
> to have evidence MarcZ is happy with the approach

Marc, could you look at patches 4-10 if you are happy with them? Link:
https://lore.kernel.org/linux-pci/20211012164145.14126-5-kabel@kernel.org/

> (you have an
> ACK on patch 9, for other patches if they were discussed on
> ML add a Link: tag to the relevant discussion please).

Only that one patch was acked in past:
https://lore.kernel.org/linux-pci/87a6p6q1r9.wl-maz@kernel.org/

> I am trying to keep the IRQ domain management in PCI controller
> drivers in check, that's why I am asking.
> 
> I can definitely pull [1-3] and [11-14] since they look good to me.

Ok!

> Thanks,
> Lorenzo
> 
> > 
> > > Lorenzo
> > > 
> > > > > > 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 b703b271c6b1..337b61508799 100644
> > > > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > > > @@ -1274,7 +1274,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
> > > > > >  
> > > > > >  	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_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
> > > > > >  	msi_di->chip = msi_ic;
> > > > > >  
> > > > > >  	pcie->msi_inner_domain =
> > > > > > -- 
> > > > > > 2.32.0
> > > > > > 

Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Marc Zyngier]

On Thu, 28 Oct 2021 12:37:24 +0100,
Pali Rohár <pali@kernel.org> wrote:
> 
> On Thursday 28 October 2021 12:30:30 Lorenzo Pieralisi wrote:
> > On Thu, Oct 28, 2021 at 01:13:02PM +0200, Pali Rohár wrote:
> > 
> > [...]
> > 
> > > > > In commit message I originally tried to explain it that after applying
> > > > > all previous patches which are fixing MSI and Multi-MSI support (part of
> > > > > them is enforcement to use only MSI numbers 0..31), it makes driver
> > > > > compatible with also MSI-X interrupts.
> > > > > 
> > > > > If you want to rewrite commit message, let us know, there is no problem.
> > > > 
> > > > I think we should.
> > > > 
> > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > > 
> > > > By the way, this tag should be removed. Marek signed it off, that
> > > > applies to other patches in this series as well.
> > > 
> > > Ok! Is this the only issue with this patch series? Or something other
> > > needs to be fixed?
> > 
> > The series looks fine to me - only thing for patch[4-10] I'd like
> > to have evidence MarcZ is happy with the approach
> 
> Marc, could you look at patches 4-10 if you are happy with them? Link:
> https://lore.kernel.org/linux-pci/20211012164145.14126-5-kabel@kernel.org/

Started with patch #4, and saw that you are still using
irq_find_mapping + generic_handle_irq which I objected to every time I
looked at this patch ([1], [2]).

My NAK still stands, and I haven't looked any further, because you
obviously don't really care about review comments.

	M.

[1] https://lore.kernel.org/r/8735r0qfab.wl-maz@kernel.org
[2] https://lore.kernel.org/r/871r6kqf2d.wl-maz@kernel.org

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

Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Pali Rohár]

On Thursday 28 October 2021 16:24:08 Marc Zyngier wrote:
> On Thu, 28 Oct 2021 12:37:24 +0100,
> Pali Rohár <pali@kernel.org> wrote:
> > 
> > On Thursday 28 October 2021 12:30:30 Lorenzo Pieralisi wrote:
> > > On Thu, Oct 28, 2021 at 01:13:02PM +0200, Pali Rohár wrote:
> > > 
> > > [...]
> > > 
> > > > > > In commit message I originally tried to explain it that after applying
> > > > > > all previous patches which are fixing MSI and Multi-MSI support (part of
> > > > > > them is enforcement to use only MSI numbers 0..31), it makes driver
> > > > > > compatible with also MSI-X interrupts.
> > > > > > 
> > > > > > If you want to rewrite commit message, let us know, there is no problem.
> > > > > 
> > > > > I think we should.
> > > > > 
> > > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > > > 
> > > > > By the way, this tag should be removed. Marek signed it off, that
> > > > > applies to other patches in this series as well.
> > > > 
> > > > Ok! Is this the only issue with this patch series? Or something other
> > > > needs to be fixed?
> > > 
> > > The series looks fine to me - only thing for patch[4-10] I'd like
> > > to have evidence MarcZ is happy with the approach
> > 
> > Marc, could you look at patches 4-10 if you are happy with them? Link:
> > https://lore.kernel.org/linux-pci/20211012164145.14126-5-kabel@kernel.org/
> 
> Started with patch #4, and saw that you are still using
> irq_find_mapping + generic_handle_irq which I objected to every time I
> looked at this patch ([1], [2]).
> 
> My NAK still stands, and I haven't looked any further, because you
> obviously don't really care about review comments.

I passed to Marek all patches including handling and fixing these
issues. But because Lorenzo wanted smaller patch series, Marek probably
has not included them in this batch 2.

> 	M.
> 
> [1] https://lore.kernel.org/r/8735r0qfab.wl-maz@kernel.org
> [2] https://lore.kernel.org/r/871r6kqf2d.wl-maz@kernel.org
> 
> -- 
> Without deviation from the norm, progress is not possible.

Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Marek Behún]

On Thu, 28 Oct 2021 16:24:08 +0100
Marc Zyngier <maz@kernel.org> wrote:

> On Thu, 28 Oct 2021 12:37:24 +0100,
> Pali Rohár <pali@kernel.org> wrote:
> > 
> > On Thursday 28 October 2021 12:30:30 Lorenzo Pieralisi wrote:  
> > > On Thu, Oct 28, 2021 at 01:13:02PM +0200, Pali Rohár wrote:
> > > 
> > > [...]
> > >   
> > > > > > In commit message I originally tried to explain it that after applying
> > > > > > all previous patches which are fixing MSI and Multi-MSI support (part of
> > > > > > them is enforcement to use only MSI numbers 0..31), it makes driver
> > > > > > compatible with also MSI-X interrupts.
> > > > > > 
> > > > > > If you want to rewrite commit message, let us know, there is no problem.  
> > > > > 
> > > > > I think we should.
> > > > >   
> > > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > > > Reviewed-by: Marek Behún <kabel@kernel.org>  
> > > > > 
> > > > > By the way, this tag should be removed. Marek signed it off, that
> > > > > applies to other patches in this series as well.  
> > > > 
> > > > Ok! Is this the only issue with this patch series? Or something other
> > > > needs to be fixed?  
> > > 
> > > The series looks fine to me - only thing for patch[4-10] I'd like
> > > to have evidence MarcZ is happy with the approach  
> > 
> > Marc, could you look at patches 4-10 if you are happy with them? Link:
> > https://lore.kernel.org/linux-pci/20211012164145.14126-5-kabel@kernel.org/  
> 
> Started with patch #4, and saw that you are still using
> irq_find_mapping + generic_handle_irq which I objected to every time I
> looked at this patch ([1], [2]).
> 
> My NAK still stands, and I haven't looked any further, because you
> obviously don't really care about review comments.
> 
> 	M.
> 
> [1] https://lore.kernel.org/r/8735r0qfab.wl-maz@kernel.org
> [2] https://lore.kernel.org/r/871r6kqf2d.wl-maz@kernel.org
> 

Marc, we have ~70 patches ready for the aardvark controller driver.

It is patch 53 [1] that converts the old irq_find_mapping() +
generic_handle_irq() API to the new API, so it isn't that Pali did
not address your comments, it is that, due to convenience, he addressed
them in a later patch.

The last time Pali sent a larger number of paches (in a previous
version, which was 42 patches [1]), it was requested that we split the
series into smaller sets, so that it is easier to merge.

Since then some more changes accumulated, resulting in the current ~70
patches, which I have been sending in smaller batches.

I could rebase the entire thing so that the patch changing the usage of
the old irq_find_mapping() + generic_handle_irq() API is first. But
that would require rebasing and testing all the patches one by one,
since the patches in-between touch everything almost everything else.

If it is really that problematic to review the changes while they use
the old API, please let me know and I will rebase it. But if you could
find it in yourself to review the patches with old API usage, it would
really save a lot of time and the result will be the same, to your
satisfaction.

Marek

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/commit/?h=pci-aardvark&id=c77d04754fbe85ed37fd7517cee253022f8428fe

[2]
https://patchwork.kernel.org/project/linux-pci/cover/20210506153153.30454-1-pali@kernel.org/

Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Marc Zyngier]

On Thu, 28 Oct 2021 16:51:50 +0100,
Marek Behún <kabel@kernel.org> wrote:
> 
> On Thu, 28 Oct 2021 16:24:08 +0100
> Marc Zyngier <maz@kernel.org> wrote:
> 
> > On Thu, 28 Oct 2021 12:37:24 +0100,
> > Pali Rohár <pali@kernel.org> wrote:
> > > 
> > > On Thursday 28 October 2021 12:30:30 Lorenzo Pieralisi wrote:  
> > > > On Thu, Oct 28, 2021 at 01:13:02PM +0200, Pali Rohár wrote:
> > > > 
> > > > [...]
> > > >   
> > > > > > > In commit message I originally tried to explain it that after applying
> > > > > > > all previous patches which are fixing MSI and Multi-MSI support (part of
> > > > > > > them is enforcement to use only MSI numbers 0..31), it makes driver
> > > > > > > compatible with also MSI-X interrupts.
> > > > > > > 
> > > > > > > If you want to rewrite commit message, let us know, there is no problem.  
> > > > > > 
> > > > > > I think we should.
> > > > > >   
> > > > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > > > > Reviewed-by: Marek Behún <kabel@kernel.org>  
> > > > > > 
> > > > > > By the way, this tag should be removed. Marek signed it off, that
> > > > > > applies to other patches in this series as well.  
> > > > > 
> > > > > Ok! Is this the only issue with this patch series? Or something other
> > > > > needs to be fixed?  
> > > > 
> > > > The series looks fine to me - only thing for patch[4-10] I'd like
> > > > to have evidence MarcZ is happy with the approach  
> > > 
> > > Marc, could you look at patches 4-10 if you are happy with them? Link:
> > > https://lore.kernel.org/linux-pci/20211012164145.14126-5-kabel@kernel.org/  
> > 
> > Started with patch #4, and saw that you are still using
> > irq_find_mapping + generic_handle_irq which I objected to every time I
> > looked at this patch ([1], [2]).
> > 
> > My NAK still stands, and I haven't looked any further, because you
> > obviously don't really care about review comments.
> > 
> > 	M.
> > 
> > [1] https://lore.kernel.org/r/8735r0qfab.wl-maz@kernel.org
> > [2] https://lore.kernel.org/r/871r6kqf2d.wl-maz@kernel.org
> > 
> 
> Marc, we have ~70 patches ready for the aardvark controller driver.
> 
> It is patch 53 [1] that converts the old irq_find_mapping() +
> generic_handle_irq() API to the new API, so it isn't that Pali did
> not address your comments, it is that, due to convenience, he addressed
> them in a later patch.

I don't see the point in merging patches that use an API that we are
actively removing. You have known that since August. I didn't say
'Just add a patch on top'. I said 'Please send a patch that makes
sense for upstream'. This makes *zero* sense for upstream. At the
time, Pali argued for your approach on the grounds of "but stable!",
and I said no to this. Since then, my opinion hasn't changed.

At the end of the day, the fate of these patches are in your own
hands. You can carry on piling useless changes on top of each others,
leading to a number of patches that is larger than a full new Linux
port. Or you can do something that is actually realistic by dropping
all the pointless intermediate states.

> The last time Pali sent a larger number of paches (in a previous
> version, which was 42 patches [1]), it was requested that we split the
> series into smaller sets, so that it is easier to merge.
> 
> Since then some more changes accumulated, resulting in the current ~70
> patches, which I have been sending in smaller batches.
> 
> I could rebase the entire thing so that the patch changing the usage of
> the old irq_find_mapping() + generic_handle_irq() API is first. But
> that would require rebasing and testing all the patches one by one,
> since the patches in-between touch everything almost everything else.

I'm sorry if you didn't realise that, but we do rebase and change
patches in patch series *all the time*. That's how it works. I someone
spots a problem in the middle of one of my patch series, I don't stick
a fix on top. I fix it in situ, and rebase the following patches.

>
> If it is really that problematic to review the changes while they use
> the old API, please let me know and I will rebase it. But if you could
> find it in yourself to review the patches with old API usage, it would
> really save a lot of time and the result will be the same, to your
> satisfaction.

I've said what I expected to see in August. I haven't changed my mind.

Thanks,

	M.

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

Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Marek Behún]

On Thu, 28 Oct 2021 17:51:50 +0200
Marek Behún <kabel@kernel.org> wrote:

> Marc, we have ~70 patches ready for the aardvark controller driver.
> 
> It is patch 53 [1] that converts the old irq_find_mapping() +
> generic_handle_irq() API to the new API, so it isn't that Pali did
> not address your comments, it is that, due to convenience, he addressed
> them in a later patch.
> 
> The last time Pali sent a larger number of paches (in a previous
> version, which was 42 patches [1]), it was requested that we split the
> series into smaller sets, so that it is easier to merge.
> 
> Since then some more changes accumulated, resulting in the current ~70
> patches, which I have been sending in smaller batches.
> 
> I could rebase the entire thing so that the patch changing the usage of
> the old irq_find_mapping() + generic_handle_irq() API is first. But
> that would require rebasing and testing all the patches one by one,
> since the patches in-between touch everything almost everything else.
> 
> If it is really that problematic to review the changes while they use
> the old API, please let me know and I will rebase it. But if you could
> find it in yourself to review the patches with old API usage, it would
> really save a lot of time and the result will be the same, to your
> satisfaction.

Lorenzo, Marc, Bjorn,

I have one more question.

Pali prepared the ~70 patches so that fixes come first, and
new features / changes to new API later.

He did it in this way so that the patches could be then conventiently
backported to stable versions - were we to first change the API usage
to the new API, and then fix the ~20 IRQ related things, we would
afterwards have to backport the fixes by rewriting them one by one.

Is this really how we should do this? Should we ignore stable while
developing for master, regardless of how much other work would need to
be spent by backporting to master, even if it could be much simpler by
applying the patches in master in another order?

Marek

Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Marc Zyngier]

On Thu, 28 Oct 2021 17:25:14 +0100,
Marek Behún <kabel@kernel.org> wrote:
> 
> On Thu, 28 Oct 2021 17:51:50 +0200
> Marek Behún <kabel@kernel.org> wrote:
> 
> > Marc, we have ~70 patches ready for the aardvark controller driver.
> > 
> > It is patch 53 [1] that converts the old irq_find_mapping() +
> > generic_handle_irq() API to the new API, so it isn't that Pali did
> > not address your comments, it is that, due to convenience, he addressed
> > them in a later patch.
> > 
> > The last time Pali sent a larger number of paches (in a previous
> > version, which was 42 patches [1]), it was requested that we split the
> > series into smaller sets, so that it is easier to merge.
> > 
> > Since then some more changes accumulated, resulting in the current ~70
> > patches, which I have been sending in smaller batches.
> > 
> > I could rebase the entire thing so that the patch changing the usage of
> > the old irq_find_mapping() + generic_handle_irq() API is first. But
> > that would require rebasing and testing all the patches one by one,
> > since the patches in-between touch everything almost everything else.
> > 
> > If it is really that problematic to review the changes while they use
> > the old API, please let me know and I will rebase it. But if you could
> > find it in yourself to review the patches with old API usage, it would
> > really save a lot of time and the result will be the same, to your
> > satisfaction.
> 
> Lorenzo, Marc, Bjorn,
> 
> I have one more question.
> 
> Pali prepared the ~70 patches so that fixes come first, and
> new features / changes to new API later.
> 
> He did it in this way so that the patches could be then conventiently
> backported to stable versions - were we to first change the API usage
> to the new API, and then fix the ~20 IRQ related things, we would
> afterwards have to backport the fixes by rewriting them one by one.
> 
> Is this really how we should do this? Should we ignore stable while
> developing for master, regardless of how much other work would need to
> be spent by backporting to master, even if it could be much simpler by
> applying the patches in master in another order?

I already replied to that in August. Upstream is the primary
development target. If you want to backport patches, do them and make
the changes required so that they are correct for whatever kernel you
target. Stable doesn't matter to upstream at all.

	M.

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

Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Lorenzo Pieralisi]

On Thu, Oct 28, 2021 at 06:00:47PM +0100, Marc Zyngier wrote:
> On Thu, 28 Oct 2021 17:25:14 +0100,
> Marek Behún <kabel@kernel.org> wrote:
> > 
> > On Thu, 28 Oct 2021 17:51:50 +0200
> > Marek Behún <kabel@kernel.org> wrote:
> > 
> > > Marc, we have ~70 patches ready for the aardvark controller driver.
> > > 
> > > It is patch 53 [1] that converts the old irq_find_mapping() +
> > > generic_handle_irq() API to the new API, so it isn't that Pali did
> > > not address your comments, it is that, due to convenience, he addressed
> > > them in a later patch.
> > > 
> > > The last time Pali sent a larger number of paches (in a previous
> > > version, which was 42 patches [1]), it was requested that we split the
> > > series into smaller sets, so that it is easier to merge.
> > > 
> > > Since then some more changes accumulated, resulting in the current ~70
> > > patches, which I have been sending in smaller batches.
> > > 
> > > I could rebase the entire thing so that the patch changing the usage of
> > > the old irq_find_mapping() + generic_handle_irq() API is first. But
> > > that would require rebasing and testing all the patches one by one,
> > > since the patches in-between touch everything almost everything else.
> > > 
> > > If it is really that problematic to review the changes while they use
> > > the old API, please let me know and I will rebase it. But if you could
> > > find it in yourself to review the patches with old API usage, it would
> > > really save a lot of time and the result will be the same, to your
> > > satisfaction.
> > 
> > Lorenzo, Marc, Bjorn,
> > 
> > I have one more question.
> > 
> > Pali prepared the ~70 patches so that fixes come first, and
> > new features / changes to new API later.
> > 
> > He did it in this way so that the patches could be then conventiently
> > backported to stable versions - were we to first change the API usage
> > to the new API, and then fix the ~20 IRQ related things, we would
> > afterwards have to backport the fixes by rewriting them one by one.
> > 
> > Is this really how we should do this? Should we ignore stable while
> > developing for master, regardless of how much other work would need to
> > be spent by backporting to master, even if it could be much simpler by
> > applying the patches in master in another order?
> 
> I already replied to that in August. Upstream is the primary
> development target. If you want to backport patches, do them and make
> the changes required so that they are correct for whatever kernel you
> target. Stable doesn't matter to upstream at all.

+1

Please don't write patch series with stable backports in mind, don't.

Let's focus on mailine with one series at a time, I understand it is
hard but that's the only way we can work and I can keep track of what
you are doing, if we keep splitting patch series I can't track reviews
and then we end up in this situation. I asked if you received Marc's
feedback exactly because I can't track the original discussion and if I
merged the series (the MSI bits) I would have ignored what Marc
requested you to do and that's not OK.

So, given the timing, I will try to merge patches [1-3] and [11-14]
if I can rebase the series cleanly; maybe I can include patch 9 if
it does not depend on previous patches.

Thanks,
Lorenzo

Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Marek Behún]

On Thu, 28 Oct 2021 18:47:18 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:

> On Thu, Oct 28, 2021 at 06:00:47PM +0100, Marc Zyngier wrote:
> > On Thu, 28 Oct 2021 17:25:14 +0100,
> > Marek Behún <kabel@kernel.org> wrote:  
> > > 
> > > On Thu, 28 Oct 2021 17:51:50 +0200
> > > Marek Behún <kabel@kernel.org> wrote:
> > >   
> > > > Marc, we have ~70 patches ready for the aardvark controller driver.
> > > > 
> > > > It is patch 53 [1] that converts the old irq_find_mapping() +
> > > > generic_handle_irq() API to the new API, so it isn't that Pali did
> > > > not address your comments, it is that, due to convenience, he addressed
> > > > them in a later patch.
> > > > 
> > > > The last time Pali sent a larger number of paches (in a previous
> > > > version, which was 42 patches [1]), it was requested that we split the
> > > > series into smaller sets, so that it is easier to merge.
> > > > 
> > > > Since then some more changes accumulated, resulting in the current ~70
> > > > patches, which I have been sending in smaller batches.
> > > > 
> > > > I could rebase the entire thing so that the patch changing the usage of
> > > > the old irq_find_mapping() + generic_handle_irq() API is first. But
> > > > that would require rebasing and testing all the patches one by one,
> > > > since the patches in-between touch everything almost everything else.
> > > > 
> > > > If it is really that problematic to review the changes while they use
> > > > the old API, please let me know and I will rebase it. But if you could
> > > > find it in yourself to review the patches with old API usage, it would
> > > > really save a lot of time and the result will be the same, to your
> > > > satisfaction.  
> > > 
> > > Lorenzo, Marc, Bjorn,
> > > 
> > > I have one more question.
> > > 
> > > Pali prepared the ~70 patches so that fixes come first, and
> > > new features / changes to new API later.
> > > 
> > > He did it in this way so that the patches could be then conventiently
> > > backported to stable versions - were we to first change the API usage
> > > to the new API, and then fix the ~20 IRQ related things, we would
> > > afterwards have to backport the fixes by rewriting them one by one.
> > > 
> > > Is this really how we should do this? Should we ignore stable while
> > > developing for master, regardless of how much other work would need to
> > > be spent by backporting to master, even if it could be much simpler by
> > > applying the patches in master in another order?  
> > 
> > I already replied to that in August. Upstream is the primary
> > development target. If you want to backport patches, do them and make
> > the changes required so that they are correct for whatever kernel you
> > target. Stable doesn't matter to upstream at all.  
> 
> +1
> 
> Please don't write patch series with stable backports in mind, don't.
> 
> Let's focus on mailine with one series at a time, I understand it is
> hard but that's the only way we can work and I can keep track of what
> you are doing, if we keep splitting patch series I can't track reviews
> and then we end up in this situation. I asked if you received Marc's
> feedback exactly because I can't track the original discussion and if I
> merged the series (the MSI bits) I would have ignored what Marc
> requested you to do and that's not OK.
> 
> So, given the timing, I will try to merge patches [1-3] and [11-14]
> if I can rebase the series cleanly; maybe I can include patch 9 if
> it does not depend on previous patches.

Very well. Ignore patch 9, since it touches IRQ. In the next batch I
shall send IRQ changes, with the first or second patch converting to
this new API.

Marek

[PATCH 11/14] PCI: aardvark: Fix support for bus mastering and PCI_COMMAND on emulated bridge

[Marek Behún]

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

From very vague, ambiguous and incomplete information from Marvell we
deduced that the 32-bit Aardvark register at address 0x4
(PCIE_CORE_CMD_STATUS_REG), which is not documented for Root Complex mode
in the Functional Specification (only for Endpoint mode), controls two
16-bit PCIe registers: Command Register and Status Registers of PCIe Root
Port.

This means that bit 2 controls bus mastering and forwarding of memory and
I/O requests in the upstream direction. According to PCI specifications
bits [0:2] of Command Register, this should be by default disabled on
reset. So explicitly disable these bits at early setup of the Aardvark
driver.

Remove code which unconditionally enables all 3 bits and let kernel code
(via pci_set_master() function) to handle bus mastering of Root PCIe
Bridge via emulated PCI_COMMAND on emulated bridge.

Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space")
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org # b2a56469d550 ("PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access")
---
 drivers/pci/controller/pci-aardvark.c | 54 +++++++++++++++++++--------
 1 file changed, 38 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 337b61508799..289cd45ed1ec 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -31,9 +31,6 @@
 /* PCIe core registers */
 #define PCIE_CORE_DEV_ID_REG					0x0
 #define PCIE_CORE_CMD_STATUS_REG				0x4
-#define     PCIE_CORE_CMD_IO_ACCESS_EN				BIT(0)
-#define     PCIE_CORE_CMD_MEM_ACCESS_EN				BIT(1)
-#define     PCIE_CORE_CMD_MEM_IO_REQ_EN				BIT(2)
 #define PCIE_CORE_DEV_REV_REG					0x8
 #define PCIE_CORE_PCIEXP_CAP					0xc0
 #define PCIE_CORE_ERR_CAPCTL_REG				0x118
@@ -516,6 +513,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	reg = (PCI_VENDOR_ID_MARVELL << 16) | PCI_VENDOR_ID_MARVELL;
 	advk_writel(pcie, reg, VENDOR_ID_REG);
 
+	/* Disable Root Bridge I/O space, memory space and bus mastering */
+	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
+	reg &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
+	advk_writel(pcie, reg, PCIE_CORE_CMD_STATUS_REG);
+
 	/* Set Advanced Error Capabilities and Control PF0 register */
 	reg = PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX |
 		PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX_EN |
@@ -620,19 +622,6 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 		advk_pcie_disable_ob_win(pcie, i);
 
 	advk_pcie_train_link(pcie);
-
-	/*
-	 * FIXME: The following register update is suspicious. This register is
-	 * applicable only when the PCI controller is configured for Endpoint
-	 * mode, not as a Root Complex. But apparently when this code is
-	 * removed, some cards stop working. This should be investigated and
-	 * a comment explaining this should be put here.
-	 */
-	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
-	reg |= PCIE_CORE_CMD_MEM_ACCESS_EN |
-		PCIE_CORE_CMD_IO_ACCESS_EN |
-		PCIE_CORE_CMD_MEM_IO_REQ_EN;
-	advk_writel(pcie, reg, PCIE_CORE_CMD_STATUS_REG);
 }
 
 static int advk_pcie_check_pio_status(struct advk_pcie *pcie, bool allow_crs, u32 *val)
@@ -761,6 +750,37 @@ static int advk_pcie_wait_pio(struct advk_pcie *pcie)
 	return -ETIMEDOUT;
 }
 
+static pci_bridge_emul_read_status_t
+advk_pci_bridge_emul_base_conf_read(struct pci_bridge_emul *bridge,
+				    int reg, u32 *value)
+{
+	struct advk_pcie *pcie = bridge->data;
+
+	switch (reg) {
+	case PCI_COMMAND:
+		*value = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
+		return PCI_BRIDGE_EMUL_HANDLED;
+
+	default:
+		return PCI_BRIDGE_EMUL_NOT_HANDLED;
+	}
+}
+
+static void
+advk_pci_bridge_emul_base_conf_write(struct pci_bridge_emul *bridge,
+				     int reg, u32 old, u32 new, u32 mask)
+{
+	struct advk_pcie *pcie = bridge->data;
+
+	switch (reg) {
+	case PCI_COMMAND:
+		advk_writel(pcie, new, PCIE_CORE_CMD_STATUS_REG);
+		break;
+
+	default:
+		break;
+	}
+}
 
 static pci_bridge_emul_read_status_t
 advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
@@ -862,6 +882,8 @@ advk_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge,
 }
 
 static struct pci_bridge_emul_ops advk_pci_bridge_emul_ops = {
+	.read_base = advk_pci_bridge_emul_base_conf_read,
+	.write_base = advk_pci_bridge_emul_base_conf_write,
 	.read_pcie = advk_pci_bridge_emul_pcie_conf_read,
 	.write_pcie = advk_pci_bridge_emul_pcie_conf_write,
 };
-- 
2.32.0

[PATCH 12/14] PCI: aardvark: Set PCI Bridge Class Code to PCI Bridge

[Marek Behún]

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

Aardvark controller has something like config space of a Root Port
available at offset 0x0 of internal registers - these registers are used
for implementation of the emulated bridge.

The default value of Class Code of this bridge corresponds to a RAID Mass
storage controller, though. (This is probably intended for when the
controller is used as Endpoint.)

Change the Class Code to correspond to a PCI Bridge.

Add comment explaining this change.

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

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 289cd45ed1ec..801657e7da93 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -513,6 +513,26 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	reg = (PCI_VENDOR_ID_MARVELL << 16) | PCI_VENDOR_ID_MARVELL;
 	advk_writel(pcie, reg, VENDOR_ID_REG);
 
+	/*
+	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400),
+	 * because the default value is Mass storage controller (0x010400).
+	 *
+	 * Note that this Aardvark PCI Bridge does not have compliant Type 1
+	 * Configuration Space and it even cannot be accessed via Aardvark's
+	 * PCI config space access method. Something like config space is
+	 * available in internal Aardvark registers starting at offset 0x0
+	 * and is reported as Type 0. In range 0x10 - 0x34 it has totally
+	 * different registers.
+	 *
+	 * Therefore driver uses emulation of PCI Bridge which emulates
+	 * access to configuration space via internal Aardvark registers or
+	 * emulated configuration buffer.
+	 */
+	reg = advk_readl(pcie, PCIE_CORE_DEV_REV_REG);
+	reg &= ~0xffffff00;
+	reg |= (PCI_CLASS_BRIDGE_PCI << 8) << 8;
+	advk_writel(pcie, reg, PCIE_CORE_DEV_REV_REG);
+
 	/* Disable Root Bridge I/O space, memory space and bus mastering */
 	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
 	reg &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
-- 
2.32.0

Re: [PATCH 12/14] PCI: aardvark: Set PCI Bridge Class Code to PCI Bridge

[Lorenzo Pieralisi]

On Tue, Oct 12, 2021 at 06:41:43PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Aardvark controller has something like config space of a Root Port
> available at offset 0x0 of internal registers - these registers are used
> for implementation of the emulated bridge.
> 
> The default value of Class Code of this bridge corresponds to a RAID Mass
> storage controller, though. (This is probably intended for when the
> controller is used as Endpoint.)
> 
> Change the Class Code to correspond to a PCI Bridge.
> 
> Add comment explaining this change.
> 
> Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/pci/controller/pci-aardvark.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 289cd45ed1ec..801657e7da93 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -513,6 +513,26 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  	reg = (PCI_VENDOR_ID_MARVELL << 16) | PCI_VENDOR_ID_MARVELL;
>  	advk_writel(pcie, reg, VENDOR_ID_REG);
>  
> +	/*
> +	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400),
> +	 * because the default value is Mass storage controller (0x010400).
> +	 *
> +	 * Note that this Aardvark PCI Bridge does not have compliant Type 1
> +	 * Configuration Space and it even cannot be accessed via Aardvark's
> +	 * PCI config space access method. Something like config space is
> +	 * available in internal Aardvark registers starting at offset 0x0
> +	 * and is reported as Type 0. In range 0x10 - 0x34 it has totally
> +	 * different registers.

Is the RP enumerated as a PCI device with type 0 header ?

> +	 *
> +	 * Therefore driver uses emulation of PCI Bridge which emulates
> +	 * access to configuration space via internal Aardvark registers or
> +	 * emulated configuration buffer.
> +	 */
> +	reg = advk_readl(pcie, PCIE_CORE_DEV_REV_REG);
> +	reg &= ~0xffffff00;
> +	reg |= (PCI_CLASS_BRIDGE_PCI << 8) << 8;
> +	advk_writel(pcie, reg, PCIE_CORE_DEV_REV_REG);

I remember Bjorn commenting on something similar in the past - he may
have some comments on whether this change is the right thing to do.

Lorenzo

>  	/* Disable Root Bridge I/O space, memory space and bus mastering */
>  	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
>  	reg &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> -- 
> 2.32.0
> 

Re: [PATCH 12/14] PCI: aardvark: Set PCI Bridge Class Code to PCI Bridge

[Pali Rohár]

On Thursday 28 October 2021 19:30:54 Lorenzo Pieralisi wrote:
> On Tue, Oct 12, 2021 at 06:41:43PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > Aardvark controller has something like config space of a Root Port
> > available at offset 0x0 of internal registers - these registers are used
> > for implementation of the emulated bridge.
> > 
> > The default value of Class Code of this bridge corresponds to a RAID Mass
> > storage controller, though. (This is probably intended for when the
> > controller is used as Endpoint.)
> > 
> > Change the Class Code to correspond to a PCI Bridge.
> > 
> > Add comment explaining this change.
> > 
> > Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space")
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <kabel@kernel.org>
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index 289cd45ed1ec..801657e7da93 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -513,6 +513,26 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> >  	reg = (PCI_VENDOR_ID_MARVELL << 16) | PCI_VENDOR_ID_MARVELL;
> >  	advk_writel(pcie, reg, VENDOR_ID_REG);
> >  
> > +	/*
> > +	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400),
> > +	 * because the default value is Mass storage controller (0x010400).
> > +	 *
> > +	 * Note that this Aardvark PCI Bridge does not have compliant Type 1
> > +	 * Configuration Space and it even cannot be accessed via Aardvark's
> > +	 * PCI config space access method. Something like config space is
> > +	 * available in internal Aardvark registers starting at offset 0x0
> > +	 * and is reported as Type 0. In range 0x10 - 0x34 it has totally
> > +	 * different registers.
> 
> Is the RP enumerated as a PCI device with type 0 header ?

Yes.

And pci-bridge-emul.c "converts" it to type 1 header. So lspci correctly
see it as type 1.

> > +	 *
> > +	 * Therefore driver uses emulation of PCI Bridge which emulates
> > +	 * access to configuration space via internal Aardvark registers or
> > +	 * emulated configuration buffer.
> > +	 */
> > +	reg = advk_readl(pcie, PCIE_CORE_DEV_REV_REG);
> > +	reg &= ~0xffffff00;
> > +	reg |= (PCI_CLASS_BRIDGE_PCI << 8) << 8;
> > +	advk_writel(pcie, reg, PCIE_CORE_DEV_REV_REG);
> 
> I remember Bjorn commenting on something similar in the past - he may
> have some comments on whether this change is the right thing to do.

Root Port should have PCI Bridge class code, but aardvark HW initialize
it to class code for Mass Storage (as explained above).

pci-bridge-emul.c again transparently "converts" it to PCI Bridge class
code.

Similar issue has also mvebu hw, see email:
https://lore.kernel.org/linux-pci/20211003120944.3lmwxylnhlp2kfj7@pali/

And similar fixup was applied for kirkwood:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1dc831bf53fddcc6443f74a39e72db5bcea4f15d

Are there any issues with it?

> Lorenzo
> 
> >  	/* Disable Root Bridge I/O space, memory space and bus mastering */
> >  	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
> >  	reg &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> > -- 
> > 2.32.0
> > 

Re: [PATCH 12/14] PCI: aardvark: Set PCI Bridge Class Code to PCI Bridge

[Bjorn Helgaas]

On Thu, Oct 28, 2021 at 08:45:57PM +0200, Pali Rohár wrote:
> On Thursday 28 October 2021 19:30:54 Lorenzo Pieralisi wrote:
> > On Tue, Oct 12, 2021 at 06:41:43PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > > 
> > > Aardvark controller has something like config space of a Root Port
> > > available at offset 0x0 of internal registers - these registers are used
> > > for implementation of the emulated bridge.
> > > 
> > > The default value of Class Code of this bridge corresponds to a RAID Mass
> > > storage controller, though. (This is probably intended for when the
> > > controller is used as Endpoint.)
> > > 
> > > Change the Class Code to correspond to a PCI Bridge.
> > > 
> > > Add comment explaining this change.
> > > 
> > > Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space")
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Reviewed-by: Marek Behún <kabel@kernel.org>
> > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/pci/controller/pci-aardvark.c | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > index 289cd45ed1ec..801657e7da93 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -513,6 +513,26 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> > >  	reg = (PCI_VENDOR_ID_MARVELL << 16) | PCI_VENDOR_ID_MARVELL;
> > >  	advk_writel(pcie, reg, VENDOR_ID_REG);
> > >  
> > > +	/*
> > > +	 * Change Class Code of PCI Bridge device to PCI Bridge (0x600400),
> > > +	 * because the default value is Mass storage controller (0x010400).
> > > +	 *
> > > +	 * Note that this Aardvark PCI Bridge does not have compliant Type 1
> > > +	 * Configuration Space and it even cannot be accessed via Aardvark's
> > > +	 * PCI config space access method. Something like config space is
> > > +	 * available in internal Aardvark registers starting at offset 0x0
> > > +	 * and is reported as Type 0. In range 0x10 - 0x34 it has totally
> > > +	 * different registers.
> > 
> > Is the RP enumerated as a PCI device with type 0 header ?
> 
> Yes.
> 
> And pci-bridge-emul.c "converts" it to type 1 header. So lspci correctly
> see it as type 1.
> 
> > > +	 *
> > > +	 * Therefore driver uses emulation of PCI Bridge which emulates
> > > +	 * access to configuration space via internal Aardvark registers or
> > > +	 * emulated configuration buffer.
> > > +	 */
> > > +	reg = advk_readl(pcie, PCIE_CORE_DEV_REV_REG);
> > > +	reg &= ~0xffffff00;
> > > +	reg |= (PCI_CLASS_BRIDGE_PCI << 8) << 8;
> > > +	advk_writel(pcie, reg, PCIE_CORE_DEV_REV_REG);
> > 
> > I remember Bjorn commenting on something similar in the past - he may
> > have some comments on whether this change is the right thing to do.

No comments from me :)

> Root Port should have PCI Bridge class code, but aardvark HW initialize
> it to class code for Mass Storage (as explained above).
>
> pci-bridge-emul.c again transparently "converts" it to PCI Bridge class
> code.
> 
> Similar issue has also mvebu hw, see email:
> https://lore.kernel.org/linux-pci/20211003120944.3lmwxylnhlp2kfj7@pali/
> 
> And similar fixup was applied for kirkwood:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1dc831bf53fddcc6443f74a39e72db5bcea4f15d
> 
> Are there any issues with it?
> 
> > Lorenzo
> > 
> > >  	/* Disable Root Bridge I/O space, memory space and bus mastering */
> > >  	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
> > >  	reg &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> > > -- 
> > > 2.32.0
> > > 

[PATCH 13/14] PCI: aardvark: Fix support for PCI_BRIDGE_CTL_BUS_RESET on emulated bridge

[Marek Behún]

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

Aardvark supports PCIe Hot Reset via PCIE_CORE_CTRL1_REG.

Use it for implementing PCI_BRIDGE_CTL_BUS_RESET bit of PCI_BRIDGE_CONTROL
register on emulated bridge.

With this, the function pci_reset_secondary_bus() starts working and can
reset connected PCIe card. Custom userspace script [1] which uses setpci
can trigger PCIe Hot Reset and reset the card manually.

[1] https://alexforencich.com/wiki/en/pcie/hot-reset-linux

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

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 801657e7da93..ad31a172c5c2 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -781,6 +781,22 @@ advk_pci_bridge_emul_base_conf_read(struct pci_bridge_emul *bridge,
 		*value = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
 		return PCI_BRIDGE_EMUL_HANDLED;
 
+	case PCI_INTERRUPT_LINE: {
+		/*
+		 * From the whole 32bit register we support reading from HW only
+		 * one bit: PCI_BRIDGE_CTL_BUS_RESET.
+		 * 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_CORE_CTRL1_REG) & HOT_RESET_GEN)
+			val |= PCI_BRIDGE_CTL_BUS_RESET << 16;
+		else
+			val &= ~(PCI_BRIDGE_CTL_BUS_RESET << 16);
+		*value = val;
+		return PCI_BRIDGE_EMUL_HANDLED;
+	}
+
 	default:
 		return PCI_BRIDGE_EMUL_NOT_HANDLED;
 	}
@@ -797,6 +813,17 @@ advk_pci_bridge_emul_base_conf_write(struct pci_bridge_emul *bridge,
 		advk_writel(pcie, new, PCIE_CORE_CMD_STATUS_REG);
 		break;
 
+	case PCI_INTERRUPT_LINE:
+		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))
+				val |= HOT_RESET_GEN;
+			else
+				val &= ~HOT_RESET_GEN;
+			advk_writel(pcie, val, PCIE_CORE_CTRL1_REG);
+		}
+		break;
+
 	default:
 		break;
 	}
-- 
2.32.0

[PATCH 14/14] PCI: aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge

[Marek Behún]

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

This register is exported at address offset 0x30.

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

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index ad31a172c5c2..5d944e57c1a6 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -32,6 +32,7 @@
 #define PCIE_CORE_DEV_ID_REG					0x0
 #define PCIE_CORE_CMD_STATUS_REG				0x4
 #define PCIE_CORE_DEV_REV_REG					0x8
+#define PCIE_CORE_EXP_ROM_BAR_REG				0x30
 #define PCIE_CORE_PCIEXP_CAP					0xc0
 #define PCIE_CORE_ERR_CAPCTL_REG				0x118
 #define     PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX			BIT(5)
@@ -781,6 +782,10 @@ advk_pci_bridge_emul_base_conf_read(struct pci_bridge_emul *bridge,
 		*value = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
 		return PCI_BRIDGE_EMUL_HANDLED;
 
+	case PCI_ROM_ADDRESS1:
+		*value = advk_readl(pcie, PCIE_CORE_EXP_ROM_BAR_REG);
+		return PCI_BRIDGE_EMUL_HANDLED;
+
 	case PCI_INTERRUPT_LINE: {
 		/*
 		 * From the whole 32bit register we support reading from HW only
@@ -813,6 +818,10 @@ advk_pci_bridge_emul_base_conf_write(struct pci_bridge_emul *bridge,
 		advk_writel(pcie, new, PCIE_CORE_CMD_STATUS_REG);
 		break;
 
+	case PCI_ROM_ADDRESS1:
+		advk_writel(pcie, new, PCIE_CORE_EXP_ROM_BAR_REG);
+		break;
+
 	case PCI_INTERRUPT_LINE:
 		if (mask & (PCI_BRIDGE_CTL_BUS_RESET << 16)) {
 			u32 val = advk_readl(pcie, PCIE_CORE_CTRL1_REG);
-- 
2.32.0

Re: [PATCH 00/14] PCI: aardvark controller fixes BATCH 2

[Pali Rohár]

Hello! Could you please review these patches? We have another 40+
patches for pci-aardvark driver which we would like to send for review
targeting next 5.16.

On Tuesday 12 October 2021 18:41:31 Marek Behún wrote:
> Hi Lorenzo,
> 
> we are sending second batch of updates for Aardvark PCIe controller.
> 
> - patch 1 fixes pci-bridge-emul handling of W1C bits
> - patches 2-9 fix MSI interrupts
> - patch 10 enables MSI-X interrupts
> - patches 11-14 fix registers in emulated PCI bridge
> 
> Marek & Pali
> 
> Marek Behún (3):
>   PCI: pci-bridge-emul: Fix emulation of W1C bits
>   PCI: aardvark: Fix return value of MSI domain .alloc() method
>   PCI: aardvark: Read all 16-bits from PCIE_MSI_PAYLOAD_REG
> 
> Pali Rohár (11):
>   PCI: aardvark: Fix support for MSI interrupts
>   PCI: aardvark: Fix reading MSI interrupt number
>   PCI: aardvark: Clear all MSIs at setup
>   PCI: aardvark: Refactor unmasking summary MSI interrupt
>   PCI: aardvark: Fix masking MSI interrupts
>   PCI: aardvark: Fix setting MSI address
>   PCI: aardvark: Enable MSI-X support
>   PCI: aardvark: Fix support for bus mastering and PCI_COMMAND on
>     emulated bridge
>   PCI: aardvark: Set PCI Bridge Class Code to PCI Bridge
>   PCI: aardvark: Fix support for PCI_BRIDGE_CTL_BUS_RESET on emulated
>     bridge
>   PCI: aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge
> 
>  drivers/pci/controller/pci-aardvark.c | 226 ++++++++++++++++++++------
>  drivers/pci/pci-bridge-emul.c         |  13 ++
>  2 files changed, 188 insertions(+), 51 deletions(-)
> 
> -- 
> 2.32.0
> 

Re: [PATCH 00/14] PCI: aardvark controller fixes BATCH 2

[Lorenzo Pieralisi]

On Tue, Oct 12, 2021 at 06:41:31PM +0200, Marek Behún wrote:
> Hi Lorenzo,
> 
> we are sending second batch of updates for Aardvark PCIe controller.
> 
> - patch 1 fixes pci-bridge-emul handling of W1C bits
> - patches 2-9 fix MSI interrupts
> - patch 10 enables MSI-X interrupts
> - patches 11-14 fix registers in emulated PCI bridge

I tried to merge patch [1-3] and [11-14] but [11-14] need rebasing
if I drop the ones in between.

Please if you have time resend [1-3][11-14] as a series and I shall
pull them (I have a pending question on patch 12 though).

Thanks,
Lorenzo

> Marek & Pali
> 
> Marek Behún (3):
>   PCI: pci-bridge-emul: Fix emulation of W1C bits
>   PCI: aardvark: Fix return value of MSI domain .alloc() method
>   PCI: aardvark: Read all 16-bits from PCIE_MSI_PAYLOAD_REG
> 
> Pali Rohár (11):
>   PCI: aardvark: Fix support for MSI interrupts
>   PCI: aardvark: Fix reading MSI interrupt number
>   PCI: aardvark: Clear all MSIs at setup
>   PCI: aardvark: Refactor unmasking summary MSI interrupt
>   PCI: aardvark: Fix masking MSI interrupts
>   PCI: aardvark: Fix setting MSI address
>   PCI: aardvark: Enable MSI-X support
>   PCI: aardvark: Fix support for bus mastering and PCI_COMMAND on
>     emulated bridge
>   PCI: aardvark: Set PCI Bridge Class Code to PCI Bridge
>   PCI: aardvark: Fix support for PCI_BRIDGE_CTL_BUS_RESET on emulated
>     bridge
>   PCI: aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge
> 
>  drivers/pci/controller/pci-aardvark.c | 226 ++++++++++++++++++++------
>  drivers/pci/pci-bridge-emul.c         |  13 ++
>  2 files changed, 188 insertions(+), 51 deletions(-)
> 
> -- 
> 2.32.0
>