PCI: aardvark controller fixes BATCH 2
Subsystems (3)
cover PCI: aardvark controller fixes BATCH 2
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(-)
Signature · 1 line
2.32.0
PATCH 1/14 PCI: pci-bridge-emul: Fix emulation of W1C bits
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.
--- drivers/pci/pci-bridge-emul.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
drivers/pci/pci-bridge-emul.c +13 -0
@@ -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);
Signature · 1 line
2.32.0
PATCH 2/14 PCI: aardvark: Fix return value of MSI domain .alloc() method
In reply to Marek BehúnMSI 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.
--- drivers/pci/controller/pci-aardvark.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/pci-aardvark.c +1 -1
@@ -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,
Signature · 1 line
2.32.0
Re: [PATCH 02/14] PCI: aardvark: Fix return value of MSI domain .alloc() method
Quoted 3 lines · 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.
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
Quoted 31 lines
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 <[email protected]>Reviewed-by Marek Behún <[email protected]>Signed-off-by Marek Behún <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pci/controller/pci-aardvark.c +1
-1@@ -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,Signature · 1 line
2.32.0
Pali Rohár
maintainerRe: [PATCH 02/14] PCI: aardvark: Fix return value of MSI domain .alloc() method
Quoted 13 lines · On Wednesday 27 October 2021 12:26:53 Lorenzo Pieralisi wrote:
Quoted 3 lines · 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.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)
Quoted 8 lines
irq_domain_push_irq()-> that checks for rv != 0
irq_domain_alloc_irqs_parent()called by many drivers and also bymsi_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.
Quoted 37 lines
- Should we consolidate the .alloc() return value handling ?
Apologies if I missed something in the IRQ domain code.
Lorenzo
Quoted 31 lines
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 <[email protected]>Reviewed-by Marek Behún <[email protected]>Signed-off-by Marek Behún <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pci/controller/pci-aardvark.c +1
-1@@ -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,Signature · 1 line
2.32.0
PATCH 3/14 PCI: aardvark: Read all 16-bits from PCIE_MSI_PAYLOAD_REG
In reply to Marek BehúnThe 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.
--- drivers/pci/controller/pci-aardvark.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/pci/controller/pci-aardvark.c +6 -1
@@ -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); }
Signature · 1 line
2.32.0
PATCH 4/14 PCI: aardvark: Fix support for MSI interrupts
In reply to Marek BehúnFrom: Pali Rohár <[email protected]>
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.
--- drivers/pci/controller/pci-aardvark.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
drivers/pci/controller/pci-aardvark.c +12 -11
@@ -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,
Signature · 1 line
2.32.0
PATCH 5/14 PCI: aardvark: Fix reading MSI interrupt number
In reply to Marek BehúnFrom: Pali Rohár <[email protected]>
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.
---
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(-)
drivers/pci/controller/pci-aardvark.c +3 -8
@@ -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,
Signature · 1 line
2.32.0
PATCH 6/14 PCI: aardvark: Clear all MSIs at setup
In reply to Marek BehúnFrom: Pali Rohár <[email protected]>
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();
--- drivers/pci/controller/pci-aardvark.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/pci/controller/pci-aardvark.c +4 -2
@@ -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))
Signature · 1 line
2.32.0
PATCH 7/14 PCI: aardvark: Refactor unmasking summary MSI interrupt
In reply to Marek BehúnFrom: Pali Rohár <[email protected]>
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
--- drivers/pci/controller/pci-aardvark.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/pci/controller/pci-aardvark.c +6 -4
@@ -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 );
Signature · 1 line
2.32.0
PATCH 8/14 PCI: aardvark: Fix masking MSI interrupts
In reply to Marek BehúnFrom: Pali Rohár <[email protected]>
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.
--- drivers/pci/controller/pci-aardvark.c | 52 ++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 4 deletions(-)
drivers/pci/controller/pci-aardvark.c +48 -4
@@ -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 |
Signature · 1 line
2.32.0
PATCH 9/14 PCI: aardvark: Fix setting MSI address
In reply to Marek BehúnFrom: Pali Rohár <[email protected]>
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.
--- drivers/pci/controller/pci-aardvark.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
drivers/pci/controller/pci-aardvark.c +9 -12
@@ -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);
Signature · 1 line
2.32.0
PATCH 10/14 PCI: aardvark: Enable MSI-X support
In reply to Marek BehúnFrom: Pali Rohár <[email protected]>
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.
--- drivers/pci/controller/pci-aardvark.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/pci-aardvark.c +1 -1
@@ -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 =
Signature · 1 line
2.32.0
Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support
Quoted 11 lines · Marek Behún
From: Pali Rohár <[email protected]>
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 ?
Quoted 2 lines
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
Quoted 23 lines
Signed-off-by Pali Rohár <[email protected]>Reviewed-by Marek Behún <[email protected]>Signed-off-by Marek Behún <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pci/controller/pci-aardvark.c +1
-1@@ -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 =Signature · 1 line
2.32.0
Pali Rohár
maintainerRe: [PATCH 10/14] PCI: aardvark: Enable MSI-X support
Quoted 25 lines · On Wednesday 27 October 2021 15:12:46 Lorenzo Pieralisi wrote:
Quoted 11 lines · Marek Behún
From: Pali Rohár <[email protected]>
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 ?
Quoted 2 lines
Testing proves that kernel can correctly receive MSI-X interrupts from PCIe cards which supports both MSI and MSI-X interrupts.
Quoted 7 lines
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.
Quoted 25 lines
Lorenzo
Quoted 23 lines
Signed-off-by Pali Rohár <[email protected]>Reviewed-by Marek Behún <[email protected]>Signed-off-by Marek Behún <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pci/controller/pci-aardvark.c +1
-1@@ -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 =Signature · 1 line
2.32.0
Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support
Quoted 38 lines · Pali Rohár
Quoted 25 lines · On Wednesday 27 October 2021 15:12:46 Lorenzo Pieralisi wrote:
Quoted 11 lines · Marek Behún
From: Pali Rohár <[email protected]>
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 ?
Quoted 2 lines
Testing proves that kernel can correctly receive MSI-X interrupts from PCIe cards which supports both MSI and MSI-X interrupts.
Quoted 7 lines
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.
Quoted 6 lines
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.
Quoted 6 lines
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.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Signed-off-by Pali Rohár <[email protected]>Reviewed-by Marek Behún <[email protected]>
By the way, this tag should be removed. Marek signed it off, that applies to other patches in this series as well.
Lorenzo
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Signed-off-by Marek Behún <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pci/controller/pci-aardvark.c +1
-1@@ -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 =Signature · 1 line
2.32.0
Pali Rohár
maintainerRe: [PATCH 10/14] PCI: aardvark: Enable MSI-X support
Quoted 55 lines · On Thursday 28 October 2021 12:08:35 Lorenzo Pieralisi wrote:
Quoted 38 lines · Pali Rohár
Quoted 25 lines · On Wednesday 27 October 2021 15:12:46 Lorenzo Pieralisi wrote:
Quoted 11 lines · Marek Behún
From: Pali Rohár <[email protected]>
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 ?
Quoted 2 lines
Testing proves that kernel can correctly receive MSI-X interrupts from PCIe cards which supports both MSI and MSI-X interrupts.
Quoted 7 lines
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.
Quoted 6 lines
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.
Quoted 6 lines
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.
Quoted 7 lines
I think we should.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Signed-off-by Pali Rohár <[email protected]>Reviewed-by Marek Behún <[email protected]>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?
Quoted 23 lines
Lorenzo
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Signed-off-by Marek Behún <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pci/controller/pci-aardvark.c +1
-1@@ -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 =Signature · 1 line
2.32.0
Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support
On Thu, Oct 28, 2021 at 01:13:02PM +0200, Pali Rohár wrote:
[...]
Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
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.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Signed-off-by Pali Rohár <[email protected]>Reviewed-by Marek Behún <[email protected]>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
Quoted 24 lines
Quoted 24 lines
Lorenzo
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Signed-off-by Marek Behún <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pci/controller/pci-aardvark.c +1
-1@@ -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 =Signature · 1 line
2.32.0
Pali Rohár
maintainerRe: [PATCH 10/14] PCI: aardvark: Enable MSI-X support
Quoted 16 lines · 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:
[...]
Quoted 12 lines
Quoted 12 lines
Quoted 6 lines
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.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Signed-off-by Pali Rohár <[email protected]>Reviewed-by Marek Behún <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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/[email protected]/
Quoted 3 lines
(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/[email protected]/
Quoted 4 lines
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!
Quoted 27 lines
Thanks,
LorenzoQuoted 24 lines
Quoted 24 lines
Lorenzo
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Signed-off-by Marek Behún <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pci/controller/pci-aardvark.c +1
-1@@ -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 =Signature · 1 line
2.32.0
Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support
Quoted 21 lines · Pali Rohár
Quoted 19 lines · 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:
[...]
Quoted 15 lines
Quoted 14 lines
Quoted 6 lines
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.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Signed-off-by Pali Rohár <[email protected]>Reviewed-by Marek Behún <[email protected]>By the way, this tag should be removed. Marek signed it off, that applies to other patches in this series as well.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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/[email protected]/
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/[email protected]
[2] https://lore.kernel.org/r/[email protected]
Signature · 1 line
Without deviation from the norm, progress is not possible.
Pali Rohár
maintainerRe: [PATCH 10/14] PCI: aardvark: Enable MSI-X support
Quoted 32 lines · On Thursday 28 October 2021 16:24:08 Marc Zyngier wrote:
Quoted 21 lines · Pali Rohár
Quoted 19 lines · 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:
[...]
Quoted 15 lines
Quoted 14 lines
Quoted 6 lines
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.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Signed-off-by Pali Rohár <[email protected]>Reviewed-by Marek Behún <[email protected]>By the way, this tag should be removed. Marek signed it off, that applies to other patches in this series as well.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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/[email protected]/
Quoted 6 lines
Started with patch #4, and saw that you are still using
irq_find_mapping+generic_handle_irqwhich 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.
Quoted 7 lines
M.[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support
In reply to Marc ZyngierQuoted 36 lines · On Thu, 28 Oct 2021 16:24:08 +0100 Marc Zyngier <[email protected]> wrote:
Quoted 21 lines · Pali Rohár
Quoted 19 lines · 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:
[...]
Quoted 15 lines
Quoted 14 lines
Quoted 6 lines
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.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Signed-off-by Pali Rohár <[email protected]>Reviewed-by Marek Behún <[email protected]>By the way, this tag should be removed. Marek signed it off, that applies to other patches in this series as well.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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/[email protected]/Started with patch #4, and saw that you are still using
irq_find_mapping+generic_handle_irqwhich I objected to every time I looked at this patch ([1], [2]).
Quoted 8 lines
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/[email protected]
[2] https://lore.kernel.org/r/[email protected]
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
[2]
https://patchwork.kernel.org/project/linux-pci/cover/[email protected]/
Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support
Quoted 49 lines · Marek Behún
Quoted 36 lines · On Thu, 28 Oct 2021 16:24:08 +0100 Marc Zyngier <[email protected]> wrote:
Quoted 21 lines · Pali Rohár
Quoted 19 lines · 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:
[...]
Quoted 15 lines
Quoted 14 lines
Quoted 6 lines
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.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Signed-off-by Pali Rohár <[email protected]>Reviewed-by Marek Behún <[email protected]>By the way, this tag should be removed. Marek signed it off, that applies to other patches in this series as well.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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/[email protected]/Started with patch #4, and saw that you are still using
irq_find_mapping+generic_handle_irqwhich I objected to every time I looked at this patch ([1], [2]).Quoted 8 lines
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/[email protected]
[2] https://lore.kernel.org/r/[email protected]
Quoted 6 lines
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.
Quoted 11 lines
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.
Quoted 6 lines
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.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support
In reply to Marek BehúnQuoted 19 lines · On Thu, 28 Oct 2021 17:51:50 +0200 Marek Behún <[email protected]> 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.
Quoted 5 lines
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
Quoted 41 lines · Marek Behún
Quoted 19 lines · On Thu, 28 Oct 2021 17:51:50 +0200 Marek Behún <[email protected]> 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.Quoted 5 lines
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.
Quoted 4 lines
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.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support
Quoted 48 lines · Marc Zyngier
Quoted 41 lines · Marek Behún
Quoted 19 lines · On Thu, 28 Oct 2021 17:51:50 +0200 Marek Behún <[email protected]> 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.Quoted 5 lines
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.
Quoted 4 lines
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?
Quoted 4 lines
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
Quoted 66 lines · On Thu, 28 Oct 2021 18:47:18 +0100 Lorenzo Pieralisi…
Quoted 48 lines · Marc Zyngier
Quoted 41 lines · Marek Behún
Quoted 19 lines · On Thu, 28 Oct 2021 17:51:50 +0200 Marek Behún <[email protected]> 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.Quoted 5 lines
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.
Quoted 4 lines
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?
Quoted 4 lines
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.
Quoted 3 lines
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
In reply to Marek BehúnFrom: Pali Rohár <[email protected]>
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.
--- drivers/pci/controller/pci-aardvark.c | 54 +++++++++++++++++++-------- 1 file changed, 38 insertions(+), 16 deletions(-)
drivers/pci/controller/pci-aardvark.c +38 -16
@@ -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, };
Signature · 1 line
2.32.0
PATCH 12/14 PCI: aardvark: Set PCI Bridge Class Code to PCI Bridge
In reply to Marek BehúnFrom: Pali Rohár <[email protected]>
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.
--- drivers/pci/controller/pci-aardvark.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
drivers/pci/controller/pci-aardvark.c +20 -0
@@ -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 );
Signature · 1 line
2.32.0
Re: [PATCH 12/14] PCI: aardvark: Set PCI Bridge Class Code to PCI Bridge
Quoted 31 lines · Marek Behún
From: Pali Rohár <[email protected]>
Aardvark controller has something like config space of a Root Port available at offset
0x0of 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 <[email protected]>Reviewed-by Marek Behún <[email protected]>Signed-off-by Marek Behún <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)drivers/pci/controller/pci-aardvark.c +0
-0@@ -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 );
drivers/pci/controller/pci-aardvark.c static void advk_pcie_setup_hw(struct advk_pcie *pcie) · line 525
Quoted 10 lines
drivers/pci/controller/pci-aardvark.c +10
-0+/* +* 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 ?
drivers/pci/controller/pci-aardvark.c static void advk_pcie_setup_hw(struct advk_pcie *pcie) · line 534
Quoted 9 lines
drivers/pci/controller/pci-aardvark.c +9
-0+* +* 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
Quoted 6 lines
drivers/pci/controller/pci-aardvark.c +0
-0/* 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 );Signature · 1 line
2.32.0
Pali Rohár
maintainerRe: [PATCH 12/14] PCI: aardvark: Set PCI Bridge Class Code to PCI Bridge
Quoted 32 lines · On Thursday 28 October 2021 19:30:54 Lorenzo Pieralisi wrote:
Quoted 31 lines · Marek Behún
From: Pali Rohár <[email protected]>
Aardvark controller has something like config space of a Root Port available at offset
0x0of 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 <[email protected]>Reviewed-by Marek Behún <[email protected]>Signed-off-by Marek Behún <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)drivers/pci/controller/pci-aardvark.c +0
-0@@ -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 );
Quoted 12 lines
drivers/pci/controller/pci-aardvark.c static void advk_pcie_setup_hw(struct advk_pcie *pcie) · line 525
Quoted 10 lines
drivers/pci/controller/pci-aardvark.c +10
-0+/* +* 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.
Quoted 4 lines
Quoted 4 lines
drivers/pci/controller/pci-aardvark.c +4
-0+* +* Therefore driver uses emulation of PCI Bridge which emulates +* access to configuration space via internal Aardvark registers or +* emulated configuration buffer.
Quoted 8 lines
drivers/pci/controller/pci-aardvark.c static void advk_pcie_setup_hw(struct advk_pcie *pcie) · line 534
Quoted 5 lines
drivers/pci/controller/pci-aardvark.c +5
-0+*/ + 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?
Quoted 8 lines
Lorenzo
Quoted 6 lines
drivers/pci/controller/pci-aardvark.c +0
-0/* 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 );Signature · 1 line
2.32.0
Re: [PATCH 12/14] PCI: aardvark: Set PCI Bridge Class Code to PCI Bridge
Quoted 46 lines · Pali Rohár
Quoted 32 lines · On Thursday 28 October 2021 19:30:54 Lorenzo Pieralisi wrote:
Quoted 31 lines · Marek Behún
From: Pali Rohár <[email protected]>
Aardvark controller has something like config space of a Root Port available at offset
0x0of 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 <[email protected]>Reviewed-by Marek Behún <[email protected]>Signed-off-by Marek Behún <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)drivers/pci/controller/pci-aardvark.c +0
-0@@ -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 );Quoted 12 lines
drivers/pci/controller/pci-aardvark.c static void advk_pcie_setup_hw(struct advk_pcie *pcie) · line 525
Quoted 10 lines
drivers/pci/controller/pci-aardvark.c +10
-0+/* +* 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 ?
Quoted 17 lines
Yes.
And
pci-bridge-emul.c"converts" it to type 1 header. So lspci correctly see it as type 1.Quoted 12 lines
drivers/pci/controller/pci-aardvark.c static void advk_pcie_setup_hw(struct advk_pcie *pcie) · line 534
Quoted 9 lines
drivers/pci/controller/pci-aardvark.c +9
-0+* +* 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 :)
Quoted 22 lines
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.cagain 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=1dc831bf53fddcc6443f74a39e72db5bcea4f15dAre there any issues with it?
Quoted 8 lines
Lorenzo
Quoted 6 lines
drivers/pci/controller/pci-aardvark.c +0
-0/* 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 );Signature · 1 line
2.32.0
PATCH 13/14 PCI: aardvark: Fix support for PCI_BRIDGE_CTL_BUS_RESET on emulated bridge
In reply to Marek BehúnFrom: Pali Rohár <[email protected]>
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
--- drivers/pci/controller/pci-aardvark.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
drivers/pci/controller/pci-aardvark.c +27 -0
@@ -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 ; }
Signature · 1 line
2.32.0
PATCH 14/14 PCI: aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge
In reply to Marek BehúnFrom: Pali Rohár <[email protected]>
This register is exported at address offset 0x30.
--- drivers/pci/controller/pci-aardvark.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/pci/controller/pci-aardvark.c +9 -0
@@ -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 );
Signature · 1 line
2.32.0
Pali Rohár
maintainerHello! 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.
Quoted 38 lines · 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 bridgeMarek & 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 fromPCIE_MSI_PAYLOAD_REGPali 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 andPCI_COMMANDon
emulated bridge
PCI: aardvark: Set PCI Bridge Class Code to PCI Bridge
PCI: aardvark: Fix support forPCI_BRIDGE_CTL_BUS_RESETon emulated
bridge
PCI: aardvark: Fix support forPCI_ROM_ADDRESS1on emulated bridgedrivers/pci/controller/pci-aardvark.c | 226 ++++++++++++++++++++------ drivers/pci/pci-bridge-emul.c | 13 ++ 2 files changed, 188 insertions(+), 51 deletions(-)Signature · 1 line
2.32.0
Quoted 8 lines · 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
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
Quoted 29 lines
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 fromPCIE_MSI_PAYLOAD_REGPali 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 andPCI_COMMANDon
emulated bridge
PCI: aardvark: Set PCI Bridge Class Code to PCI Bridge
PCI: aardvark: Fix support forPCI_BRIDGE_CTL_BUS_RESETon emulated
bridge
PCI: aardvark: Fix support forPCI_ROM_ADDRESS1on emulated bridgedrivers/pci/controller/pci-aardvark.c | 226 ++++++++++++++++++++------ drivers/pci/pci-bridge-emul.c | 13 ++ 2 files changed, 188 insertions(+), 51 deletions(-)Signature · 1 line
2.32.0