Re: [v9,5/7] PCI: mediatek-gen3: Add MSI support
On Sat, 27 Mar 2021 20:29:04 +,
Pali Rohár wrote:
>
> On Saturday 27 March 2021 19:44:30 Marc Zyngier wrote:
> > On Sat, 27 Mar 2021 19:28:37 +,
> > Pali Rohár wrote:
> > >
> > > On Wednesday 24 March 2021 11:05:08 Jianjun Wang wrote:
> > > > +static void mtk_pcie_msi_handler(struct mtk_pcie_port *port, int
> > > > set_idx)
> > > > +{
> > > > + struct mtk_msi_set *msi_set = >msi_sets[set_idx];
> > > > + unsigned long msi_enable, msi_status;
> > > > + unsigned int virq;
> > > > + irq_hw_number_t bit, hwirq;
> > > > +
> > > > + msi_enable = readl_relaxed(msi_set->base +
> > > > PCIE_MSI_SET_ENABLE_OFFSET);
> > > > +
> > > > + do {
> > > > + msi_status = readl_relaxed(msi_set->base +
> > > > + PCIE_MSI_SET_STATUS_OFFSET);
> > > > + msi_status &= msi_enable;
> > > > + if (!msi_status)
> > > > + break;
> > > > +
> > > > + for_each_set_bit(bit, _status,
> > > > PCIE_MSI_IRQS_PER_SET) {
> > > > + hwirq = bit + set_idx * PCIE_MSI_IRQS_PER_SET;
> > > > + virq =
> > > > irq_find_mapping(port->msi_bottom_domain, hwirq);
> > > > + generic_handle_irq(virq);
> > > > + }
> > > > + } while (true);
> > >
> > > Hello!
> > >
> > > Just a question, cannot this while-loop cause block of processing other
> > > interrupts?
> >
> > This is a level interrupt. You don't have much choice but to handle it
> > immediately, although an alternative would be to mask it and deal with
> > it in a thread. And since Linux doesn't deal with interrupt priority,
> > a screaming interrupt is never a good thing.
>
> I see. Something like "interrupt priority" (which does not exist?) would
> be needed to handle it.
Interrupt priorities definitely exist, but Linux doesn't use
them. Furthermore, This wouldn't be relevant here as you get a bunch
of MSI multiplexed onto a single one. Where would you apply the
priority?
>
> > > I have done tests with different HW (aardvark) but with same while(true)
> > > loop logic. One XHCI PCIe controller was sending MSI interrupts too fast
> > > and interrupt handler with this while(true) logic was in infinite loop.
> > > During one IRQ it was calling infinite many times generic_handle_irq()
> > > as HW was feeding new and new MSI hwirq into status register.
> >
> > Define "too fast".
>
> Fast - next interrupt comes prior checking if while(true)-loop should stop.
That's definitely not something you can easily fix at the interrupt
handling level. You need to prevent this from happening. That's
usually the result of a misprogramming or a HW bug.
> > If something in the system is able to program the
> > XHCI device in such a way that it causes a screaming interrupt, that's
> > the place to look for problems, and probably not in the interrupt
> > handling itself, which does what it is supposed to do.
> >
> > > But this is different HW, so it can have different behavior and does not
> > > have to cause above issue.
> > >
> > > I have just spotted same code pattern for processing MSI interrupts...
> >
> > This is a common pattern that you will find in pretty much any
> > interrupt handling/demuxing, and is done this way when the cost of
> > taking the exception is high compared to that of handling it.
>
> And would not help if while(true)-loop is replaced by loop with upper
> limit of iterations? Or just call only one iteration?
That wouldn't change much: you would still have the interrupt being
pending, and it would fire again at the earliest opportunity.
At best, the root interrupt controller is able to present you with
another interrupt before forcing you to deal with the one you have
ignored again. But you cannot rely on that either.
And to be honest, other interrupts are only a part of the problem you
are describing. With a screaming interrupt, you can't execute
userspace. This is as bad as it gets.
M.
--
Without deviation from the norm, progress is not possible.
Re: [v9,5/7] PCI: mediatek-gen3: Add MSI support
On Saturday 27 March 2021 19:44:30 Marc Zyngier wrote:
> On Sat, 27 Mar 2021 19:28:37 +,
> Pali Rohár wrote:
> >
> > On Wednesday 24 March 2021 11:05:08 Jianjun Wang wrote:
> > > +static void mtk_pcie_msi_handler(struct mtk_pcie_port *port, int set_idx)
> > > +{
> > > + struct mtk_msi_set *msi_set = >msi_sets[set_idx];
> > > + unsigned long msi_enable, msi_status;
> > > + unsigned int virq;
> > > + irq_hw_number_t bit, hwirq;
> > > +
> > > + msi_enable = readl_relaxed(msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
> > > +
> > > + do {
> > > + msi_status = readl_relaxed(msi_set->base +
> > > +PCIE_MSI_SET_STATUS_OFFSET);
> > > + msi_status &= msi_enable;
> > > + if (!msi_status)
> > > + break;
> > > +
> > > + for_each_set_bit(bit, _status, PCIE_MSI_IRQS_PER_SET) {
> > > + hwirq = bit + set_idx * PCIE_MSI_IRQS_PER_SET;
> > > + virq = irq_find_mapping(port->msi_bottom_domain, hwirq);
> > > + generic_handle_irq(virq);
> > > + }
> > > + } while (true);
> >
> > Hello!
> >
> > Just a question, cannot this while-loop cause block of processing other
> > interrupts?
>
> This is a level interrupt. You don't have much choice but to handle it
> immediately, although an alternative would be to mask it and deal with
> it in a thread. And since Linux doesn't deal with interrupt priority,
> a screaming interrupt is never a good thing.
I see. Something like "interrupt priority" (which does not exist?) would
be needed to handle it.
> > I have done tests with different HW (aardvark) but with same while(true)
> > loop logic. One XHCI PCIe controller was sending MSI interrupts too fast
> > and interrupt handler with this while(true) logic was in infinite loop.
> > During one IRQ it was calling infinite many times generic_handle_irq()
> > as HW was feeding new and new MSI hwirq into status register.
>
> Define "too fast".
Fast - next interrupt comes prior checking if while(true)-loop should stop.
> If something in the system is able to program the
> XHCI device in such a way that it causes a screaming interrupt, that's
> the place to look for problems, and probably not in the interrupt
> handling itself, which does what it is supposed to do.
>
> > But this is different HW, so it can have different behavior and does not
> > have to cause above issue.
> >
> > I have just spotted same code pattern for processing MSI interrupts...
>
> This is a common pattern that you will find in pretty much any
> interrupt handling/demuxing, and is done this way when the cost of
> taking the exception is high compared to that of handling it.
And would not help if while(true)-loop is replaced by loop with upper
limit of iterations? Or just call only one iteration?
> Which is pretty much any of the badly designed, level-driving,
> DW-inspired, sorry excuse for MSI implementations that are popular on
> low-end ARM SoCs.
Ok. So thank you for information!
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
Re: [v9,5/7] PCI: mediatek-gen3: Add MSI support
On Sat, 27 Mar 2021 19:28:37 +,
Pali Rohár wrote:
>
> On Wednesday 24 March 2021 11:05:08 Jianjun Wang wrote:
> > +static void mtk_pcie_msi_handler(struct mtk_pcie_port *port, int set_idx)
> > +{
> > + struct mtk_msi_set *msi_set = >msi_sets[set_idx];
> > + unsigned long msi_enable, msi_status;
> > + unsigned int virq;
> > + irq_hw_number_t bit, hwirq;
> > +
> > + msi_enable = readl_relaxed(msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
> > +
> > + do {
> > + msi_status = readl_relaxed(msi_set->base +
> > + PCIE_MSI_SET_STATUS_OFFSET);
> > + msi_status &= msi_enable;
> > + if (!msi_status)
> > + break;
> > +
> > + for_each_set_bit(bit, _status, PCIE_MSI_IRQS_PER_SET) {
> > + hwirq = bit + set_idx * PCIE_MSI_IRQS_PER_SET;
> > + virq = irq_find_mapping(port->msi_bottom_domain, hwirq);
> > + generic_handle_irq(virq);
> > + }
> > + } while (true);
>
> Hello!
>
> Just a question, cannot this while-loop cause block of processing other
> interrupts?
This is a level interrupt. You don't have much choice but to handle it
immediately, although an alternative would be to mask it and deal with
it in a thread. And since Linux doesn't deal with interrupt priority,
a screaming interrupt is never a good thing.
> I have done tests with different HW (aardvark) but with same while(true)
> loop logic. One XHCI PCIe controller was sending MSI interrupts too fast
> and interrupt handler with this while(true) logic was in infinite loop.
> During one IRQ it was calling infinite many times generic_handle_irq()
> as HW was feeding new and new MSI hwirq into status register.
Define "too fast". If something in the system is able to program the
XHCI device in such a way that it causes a screaming interrupt, that's
the place to look for problems, and probably not in the interrupt
handling itself, which does what it is supposed to do.
> But this is different HW, so it can have different behavior and does not
> have to cause above issue.
>
> I have just spotted same code pattern for processing MSI interrupts...
This is a common pattern that you will find in pretty much any
interrupt handling/demuxing, and is done this way when the cost of
taking the exception is high compared to that of handling it.
Which is pretty much any of the badly designed, level-driving,
DW-inspired, sorry excuse for MSI implementations that are popular on
low-end ARM SoCs.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Re: [v9,5/7] PCI: mediatek-gen3: Add MSI support
On Wednesday 24 March 2021 11:05:08 Jianjun Wang wrote:
> +static void mtk_pcie_msi_handler(struct mtk_pcie_port *port, int set_idx)
> +{
> + struct mtk_msi_set *msi_set = >msi_sets[set_idx];
> + unsigned long msi_enable, msi_status;
> + unsigned int virq;
> + irq_hw_number_t bit, hwirq;
> +
> + msi_enable = readl_relaxed(msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
> +
> + do {
> + msi_status = readl_relaxed(msi_set->base +
> +PCIE_MSI_SET_STATUS_OFFSET);
> + msi_status &= msi_enable;
> + if (!msi_status)
> + break;
> +
> + for_each_set_bit(bit, _status, PCIE_MSI_IRQS_PER_SET) {
> + hwirq = bit + set_idx * PCIE_MSI_IRQS_PER_SET;
> + virq = irq_find_mapping(port->msi_bottom_domain, hwirq);
> + generic_handle_irq(virq);
> + }
> + } while (true);
Hello!
Just a question, cannot this while-loop cause block of processing other
interrupts?
I have done tests with different HW (aardvark) but with same while(true)
loop logic. One XHCI PCIe controller was sending MSI interrupts too fast
and interrupt handler with this while(true) logic was in infinite loop.
During one IRQ it was calling infinite many times generic_handle_irq()
as HW was feeding new and new MSI hwirq into status register.
But this is different HW, so it can have different behavior and does not
have to cause above issue.
I have just spotted same code pattern for processing MSI interrupts...
> +}
> +
> static void mtk_pcie_irq_handler(struct irq_desc *desc)
> {
> struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);
> @@ -405,6 +673,14 @@ static void mtk_pcie_irq_handler(struct irq_desc *desc)
> generic_handle_irq(virq);
> }
>
> + irq_bit = PCIE_MSI_SHIFT;
> + for_each_set_bit_from(irq_bit, , PCIE_MSI_SET_NUM +
> + PCIE_MSI_SHIFT) {
> + mtk_pcie_msi_handler(port, irq_bit - PCIE_MSI_SHIFT);
> +
> + writel_relaxed(BIT(irq_bit), port->base + PCIE_INT_STATUS_REG);
> + }
> +
> chained_irq_exit(irqchip, desc);
> }
>
> --
> 2.25.1
>
Re: [v9,5/7] PCI: mediatek-gen3: Add MSI support
On Wed, 24 Mar 2021 03:05:08 +, Jianjun Wang wrote: > > Add MSI support for MediaTek Gen3 PCIe controller. > > This PCIe controller supports up to 256 MSI vectors, the MSI hardware > block diagram is as follows: > > +-+ > | GIC | > +-+ > ^ > | > port->irq > | > +-+-+-+-+-+-+-+-+ > |0|1|2|3|4|5|6|7| (PCIe intc) > +-+-+-+-+-+-+-+-+ > ^ ^ ^ > | |...| > +---+ +--++---+ > ||| > +-+-+---+--+--+ +-+-+---+--+--+ +-+-+---+--+--+ > |0|1|...|30|31| |0|1|...|30|31| |0|1|...|30|31| (MSI sets) > +-+-+---+--+--+ +-+-+---+--+--+ +-+-+---+--+--+ > ^ ^ ^ ^^ ^ ^ ^^ ^ ^ ^ > | | | || | | || | | | (MSI vectors) > | | | || | | || | | | > > (MSI SET0) (MSI SET1) ... (MSI SET7) > > With 256 MSI vectors supported, the MSI vectors are composed of 8 sets, > each set has its own address for MSI message, and supports 32 MSI vectors > to generate interrupt. > > Signed-off-by: Jianjun Wang > Acked-by: Ryder Lee Reviewed-by: Marc Zyngier M. -- Without deviation from the norm, progress is not possible.
[v9,5/7] PCI: mediatek-gen3: Add MSI support
Add MSI support for MediaTek Gen3 PCIe controller.
This PCIe controller supports up to 256 MSI vectors, the MSI hardware
block diagram is as follows:
+-+
| GIC |
+-+
^
|
port->irq
|
+-+-+-+-+-+-+-+-+
|0|1|2|3|4|5|6|7| (PCIe intc)
+-+-+-+-+-+-+-+-+
^ ^ ^
| |...|
+---+ +--++---+
|||
+-+-+---+--+--+ +-+-+---+--+--+ +-+-+---+--+--+
|0|1|...|30|31| |0|1|...|30|31| |0|1|...|30|31| (MSI sets)
+-+-+---+--+--+ +-+-+---+--+--+ +-+-+---+--+--+
^ ^ ^ ^^ ^ ^ ^^ ^ ^ ^
| | | || | | || | | | (MSI vectors)
| | | || | | || | | |
(MSI SET0) (MSI SET1) ... (MSI SET7)
With 256 MSI vectors supported, the MSI vectors are composed of 8 sets,
each set has its own address for MSI message, and supports 32 MSI vectors
to generate interrupt.
Signed-off-by: Jianjun Wang
Acked-by: Ryder Lee
---
drivers/pci/controller/pcie-mediatek-gen3.c | 276
1 file changed, 276 insertions(+)
diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
b/drivers/pci/controller/pcie-mediatek-gen3.c
index ff91ad587461..ee1b51207d11 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -48,12 +49,29 @@
#define PCIE_LINK_STATUS_REG 0x154
#define PCIE_PORT_LINKUP BIT(8)
+#define PCIE_MSI_SET_NUM 8
+#define PCIE_MSI_IRQS_PER_SET 32
+#define PCIE_MSI_IRQS_NUM \
+ (PCIE_MSI_IRQS_PER_SET * PCIE_MSI_SET_NUM)
+
#define PCIE_INT_ENABLE_REG0x180
+#define PCIE_MSI_ENABLEGENMASK(PCIE_MSI_SET_NUM + 8 -
1, 8)
+#define PCIE_MSI_SHIFT 8
#define PCIE_INTX_SHIFT24
#define PCIE_INTX_ENABLE \
GENMASK(PCIE_INTX_SHIFT + PCI_NUM_INTX - 1, PCIE_INTX_SHIFT)
#define PCIE_INT_STATUS_REG0x184
+#define PCIE_MSI_SET_ENABLE_REG0x190
+#define PCIE_MSI_SET_ENABLEGENMASK(PCIE_MSI_SET_NUM - 1, 0)
+
+#define PCIE_MSI_SET_BASE_REG 0xc00
+#define PCIE_MSI_SET_OFFSET0x10
+#define PCIE_MSI_SET_STATUS_OFFSET 0x04
+#define PCIE_MSI_SET_ENABLE_OFFSET 0x08
+
+#define PCIE_MSI_SET_ADDR_HI_BASE 0xc80
+#define PCIE_MSI_SET_ADDR_HI_OFFSET0x04
#define PCIE_TRANS_TABLE_BASE_REG 0x800
#define PCIE_ATR_SRC_ADDR_MSB_OFFSET 0x4
@@ -73,6 +91,16 @@
#define PCIE_ATR_TLP_TYPE_MEM PCIE_ATR_TLP_TYPE(0)
#define PCIE_ATR_TLP_TYPE_IO PCIE_ATR_TLP_TYPE(2)
+/**
+ * struct mtk_msi_set - MSI information for each set
+ * @base: IO mapped register base
+ * @msg_addr: MSI message address
+ */
+struct mtk_msi_set {
+ void __iomem *base;
+ phys_addr_t msg_addr;
+};
+
/**
* struct mtk_pcie_port - PCIe port information
* @dev: pointer to PCIe device
@@ -86,6 +114,11 @@
* @irq: PCIe controller interrupt number
* @irq_lock: lock protecting IRQ register access
* @intx_domain: legacy INTx IRQ domain
+ * @msi_domain: MSI IRQ domain
+ * @msi_bottom_domain: MSI IRQ bottom domain
+ * @msi_sets: MSI sets information
+ * @lock: lock protecting IRQ bit map
+ * @msi_irq_in_use: bit map for assigned MSI IRQ
*/
struct mtk_pcie_port {
struct device *dev;
@@ -100,6 +133,11 @@ struct mtk_pcie_port {
int irq;
raw_spinlock_t irq_lock;
struct irq_domain *intx_domain;
+ struct irq_domain *msi_domain;
+ struct irq_domain *msi_bottom_domain;
+ struct mtk_msi_set msi_sets[PCIE_MSI_SET_NUM];
+ struct mutex lock;
+ DECLARE_BITMAP(msi_irq_in_use, PCIE_MSI_IRQS_NUM);
};
/**
@@ -196,6 +234,35 @@ static int mtk_pcie_set_trans_table(struct mtk_pcie_port
*port,
return 0;
}
+static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
+{
+ int i;
+ u32 val;
+
+ for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
+ struct mtk_msi_set *msi_set = >msi_sets[i];
+
+ msi_set->base = port->base + PCIE_MSI_SET_BASE_REG +
+ i * PCIE_MSI_SET_OFFSET;
+ msi_set->msg_addr = port->reg_base + PCIE_MSI_SET_BASE_REG +
+ i * PCIE_MSI_SET_OFFSET;
+
+ /* Configure the MSI capture address */
+ writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
+ writel_relaxed(upper_32_bits(msi_set->msg_addr),
+ port->base + PCIE_MSI_SET_ADDR_HI_BASE +
+ i * PCIE_MSI_SET_ADDR_HI_OFFSET);
+ }
+
+ val = readl_relaxed(port->base +
