Re: [PATCH v5 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2020-06-29 10:49, Kunihiko Hayashi wrote:
Hi Marc,

On 2020/06/27 18:48, Marc Zyngier wrote:
On Thu, 18 Jun 2020 09:38:09 +0100,
Kunihiko Hayashi <hayashi.kunihiko@xxxxxxxxxxxxx> wrote:

The misc interrupts consisting of PME, AER, and Link event, is handled
by INTx handler, however, these interrupts should be also handled by
MSI handler.

This adds the function uniphier_pcie_misc_isr() that handles misc
interrupts, which is called from both INTx and MSI handlers.
This function detects PME and AER interrupts with the status register,
and invoke PME and AER drivers related to MSI.

And this sets the mask for misc interrupts from INTx if MSI is enabled
and sets the mask for misc interrupts from MSI if MSI is disabled.

Cc: Marc Zyngier <maz@xxxxxxxxxx>
Cc: Jingoo Han <jingoohan1@xxxxxxxxx>
Cc: Gustavo Pimentel <gustavo.pimentel@xxxxxxxxxxxx>
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@xxxxxxxxxxxxx>
---
drivers/pci/controller/dwc/pcie-uniphier.c | 57 ++++++++++++++++++++++++------
  1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index a5401a0..5ce2479 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -44,7 +44,9 @@
  #define PCL_SYS_AUX_PWR_DET		BIT(8)
    #define PCL_RCV_INT			0x8108
+#define PCL_RCV_INT_ALL_INT_MASK	GENMASK(28, 25)
  #define PCL_RCV_INT_ALL_ENABLE		GENMASK(20, 17)
+#define PCL_RCV_INT_ALL_MSI_MASK	GENMASK(12, 9)
  #define PCL_CFG_BW_MGT_STATUS		BIT(4)
  #define PCL_CFG_LINK_AUTO_BW_STATUS	BIT(3)
  #define PCL_CFG_AER_RC_ERR_MSI_STATUS	BIT(2)
@@ -167,7 +169,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci) static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv)
  {
-	writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT);
+	u32 val;
+
+	val = PCL_RCV_INT_ALL_ENABLE;
+	if (pci_msi_enabled())
+		val |= PCL_RCV_INT_ALL_INT_MASK;
+	else
+		val |= PCL_RCV_INT_ALL_MSI_MASK;

Does this affect endpoints? Or just the RC itself?

These interrupts are asserted by RC itself, so this part affects only RC.

+
+	writel(val, priv->base + PCL_RCV_INT);
  	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
  }
@@ -231,32 +241,56 @@ static const struct irq_domain_ops uniphier_intx_domain_ops = {
  	.map = uniphier_pcie_intx_map,
  };
  -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
+static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi)
  {
-	struct pcie_port *pp = irq_desc_get_handler_data(desc);
  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
  	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	unsigned long reg;
-	u32 val, bit, virq;
+	u32 val, virq;
  -	/* INT for debug */
  	val = readl(priv->base + PCL_RCV_INT);
    	if (val & PCL_CFG_BW_MGT_STATUS)
  		dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
+
  	if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
  		dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
-	if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
-		dev_dbg(pci->dev, "Root Error\n");
-	if (val & PCL_CFG_PME_MSI_STATUS)
-		dev_dbg(pci->dev, "PME Interrupt\n");
+
+	if (is_msi) {
+		if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
+			dev_dbg(pci->dev, "Root Error Status\n");
+
+		if (val & PCL_CFG_PME_MSI_STATUS)
+			dev_dbg(pci->dev, "PME Interrupt\n");
+
+		if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
+			   PCL_CFG_PME_MSI_STATUS)) {
+			virq = irq_linear_revmap(pp->irq_domain, 0);
+			generic_handle_irq(virq);
+		}
+	}

Please have two handlers: one for interrupts that are from the RC,
another for interrupts coming from the endpoints.
I assume that this handler treats interrupts from the RC only and
this is set on the member ".msi_host_isr" added in the patch 1/6.
I think that the handler for interrupts coming from endpoints should be
treated as a normal case (after calling .msi_host_isr in
dw_handle_msi_irq()).

It looks pretty odd that you end-up dealing with both from the
same "parent" interrupt. I guess this is in keeping with the
rest of the DW PCIe hacks... :-/

It is for Lorenzo to make up his mind about this anyway.

Thanks,

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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux