[PATCH v11 0/3] PCI: uniphier: Add PME/AER support for UniPhier PCIe host controller

[PATCH v11 0/3] PCI: uniphier: Add PME/AER support for UniPhier PCIe host controller

[Kunihiko Hayashi]

This adds a new function called by MSI handler in DesignWare PCIe framework,
that invokes PME and AER funcions to detect the factor from SoC-dependent
registers.

Changes since v10:
- Fix comment style in the header
- Fix "virq" and "vIRQ" in variable name and description to "irq" and "IRQ"

Changes since v9:
- Fix the description of pcie_prot_service_get_irq()

Changes since v8:
- Add uniphier_pcie_host_init_complete() that finds PME/AER IRQ number
  after calling dw_pcie_host_init()
- Add conditions to depend on CONFIG_PCIE_PME and CONFIG_PCIEAER instead
  of CONFIG_PCIEPORTBUS
- Add Acked-by: line to portdrv patch

Changes since v7:
- Add Reviewed-by: line to 1st and 3rd patches

Changes since v6:
- Separate patches for iATU and phy error from this series
- Add Reviewed-by: line to dwc patch

Changes since v5:
- Add pcie_port_service_get_irq() function to pcie/portdrv
- Call pcie_port_service_get_irq() to get IRQ interrupt number for PME/AER
- Rebase to the latest linux-next branch,
  and remove devm_platform_ioremap_resource_byname() replacement patch

Changes since v4:
- Add Acked-by: line to dwc patch

Changes since v3:
- Move msi_host_isr() call into dw_handle_msi_irq()
- Move uniphier_pcie_misc_isr() call into the guard of chained_irq
- Use a bool argument is_msi instead of pci_msi_enabled()
- Consolidate handler calls for the same interrupt
- Fix typos in commit messages

Changes since v2:
- Avoid printing phy error message in case of EPROBE_DEFER
- Fix iATU register mapping method
- dt-bindings: Add Acked-by: line
- Fix typos in commit messages
- Use devm_platform_ioremap_resource_byname()

Changes since v1:
- Add check if struct resource is NULL
- Fix warning in the type of dev_err() argument

Kunihiko Hayashi (3):
  PCI: portdrv: Add pcie_port_service_get_irq() function
  PCI: dwc: Add msi_host_isr() callback
  PCI: uniphier: Add misc interrupt handler to invoke PME and AER

 drivers/pci/controller/dwc/pcie-designware-host.c |   3 +
 drivers/pci/controller/dwc/pcie-designware.h      |   1 +
 drivers/pci/controller/dwc/pcie-uniphier.c        | 105 +++++++++++++++++++---
 drivers/pci/pcie/portdrv.h                        |   1 +
 drivers/pci/pcie/portdrv_core.c                   |  16 ++++
 5 files changed, 112 insertions(+), 14 deletions(-)

-- 
2.7.4

[PATCH v11 1/3] PCI: portdrv: Add pcie_port_service_get_irq() function

[Kunihiko Hayashi]

Add pcie_port_service_get_irq() that returns the virtual IRQ number
for specified portdrv service.

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/portdrv.h      |  1 +
 drivers/pci/pcie/portdrv_core.c | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index 2ff5724..628a3de 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -144,4 +144,5 @@ static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {}
 #endif /* !CONFIG_PCIE_PME */
 
 struct device *pcie_port_find_device(struct pci_dev *dev, u32 service);
+int pcie_port_service_get_irq(struct pci_dev *dev, u32 service);
 #endif /* _PORTDRV_H_ */
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index e1fed664..93027d3 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -478,6 +478,22 @@ struct device *pcie_port_find_device(struct pci_dev *dev,
 EXPORT_SYMBOL_GPL(pcie_port_find_device);
 
 /**
+ * pcie_port_service_get_irq - get irq of the service
+ * @dev: PCI Express port the service is associated with
+ * @service: For the service to find
+ *
+ * Get irq number associated with given service on a pci_dev
+ */
+int pcie_port_service_get_irq(struct pci_dev *dev, u32 service)
+{
+	struct pcie_device *pciedev;
+
+	pciedev = to_pcie_device(pcie_port_find_device(dev, service));
+
+	return pciedev->irq;
+}
+
+/**
  * pcie_port_device_remove - unregister PCI Express port service devices
  * @dev: PCI Express port the service devices to unregister are associated with
  *
-- 
2.7.4

[PATCH v11 2/3] PCI: dwc: Add msi_host_isr() callback

[Kunihiko Hayashi]

This adds msi_host_isr() callback function support to describe
SoC-dependent service triggered by MSI.

For example, when AER interrupt is triggered by MSI, the callback function
reads SoC-dependent registers and detects that the interrupt is from AER,
and invoke AER interrupts related to MSI.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++
 drivers/pci/controller/dwc/pcie-designware.h      | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 24192b4..2e5ad68 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -61,6 +61,9 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 	irqreturn_t ret = IRQ_NONE;
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 
+	if (pp->ops->msi_host_isr)
+		pp->ops->msi_host_isr(pp);
+
 	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
 
 	for (i = 0; i < num_ctrls; i++) {
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 7d6e9b75..c90960f 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -175,6 +175,7 @@ enum dw_pcie_device_mode {
 struct dw_pcie_host_ops {
 	int (*host_init)(struct pcie_port *pp);
 	int (*msi_host_init)(struct pcie_port *pp);
+	void (*msi_host_isr)(struct pcie_port *pp);
 };
 
 struct pcie_port {
-- 
2.7.4

[PATCH v11 3/3] PCI: uniphier: Add misc interrupt handler to invoke PME and AER

[Kunihiko Hayashi]

This patch adds misc interrupt handler to detect and invoke PME/AER event.

In UniPhier PCIe controller, PME/AER signals are assigned to the same
signal as MSI by the internal logic. These signals should be detected by
the internal register, however, DWC MSI handler can't handle these signals.

DWC MSI handler calls .msi_host_isr() callback function, that detects
PME/AER signals using the internal register and invokes the interrupt
with PME/AER IRQ numbers.

These IRQ numbers is obtained by uniphier_pcie_port_get_irq() function,
that finds the device that matches PME/AER from the devices associated
with Root Port, and returns its IRQ number.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 drivers/pci/controller/dwc/pcie-uniphier.c | 105 +++++++++++++++++++++++++----
 1 file changed, 91 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index 7e8bad3..dcd8fa8 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -21,6 +21,7 @@
 #include <linux/reset.h>
 
 #include "pcie-designware.h"
+#include "../../pcie/portdrv.h"
 
 #define PCL_PINCTRL0			0x002c
 #define PCL_PERST_PLDN_REGEN		BIT(12)
@@ -44,7 +45,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)
@@ -68,6 +71,8 @@ struct uniphier_pcie_priv {
 	struct reset_control *rst;
 	struct phy *phy;
 	struct irq_domain *legacy_irq_domain;
+	int aer_irq;
+	int pme_irq;
 };
 
 #define to_uniphier_pcie(x)	dev_get_drvdata((x)->dev)
@@ -164,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;
+
+	writel(val, priv->base + PCL_RCV_INT);
 	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
 }
 
@@ -228,28 +241,51 @@ 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;
 
-	/* 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 (priv->aer_irq)
+				generic_handle_irq(priv->aer_irq);
+		}
+
+		if (val & PCL_CFG_PME_MSI_STATUS) {
+			dev_dbg(pci->dev, "PME Interrupt\n");
+			if (priv->pme_irq)
+				generic_handle_irq(priv->pme_irq);
+		}
+	}
 
 	writel(val, priv->base + PCL_RCV_INT);
+}
+
+static void uniphier_pcie_msi_host_isr(struct pcie_port *pp)
+{
+	uniphier_pcie_misc_isr(pp, true);
+}
+
+static void uniphier_pcie_irq_handler(struct irq_desc *desc)
+{
+	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, irq;
+
+	uniphier_pcie_misc_isr(pp, false);
 
 	/* INTx */
 	chained_irq_enter(chip, desc);
@@ -258,8 +294,8 @@ static void uniphier_pcie_irq_handler(struct irq_desc *desc)
 	reg = FIELD_GET(PCL_RCV_INTX_ALL_STATUS, val);
 
 	for_each_set_bit(bit, &reg, PCI_NUM_INTX) {
-		virq = irq_linear_revmap(priv->legacy_irq_domain, bit);
-		generic_handle_irq(virq);
+		irq = irq_linear_revmap(priv->legacy_irq_domain, bit);
+		generic_handle_irq(irq);
 	}
 
 	chained_irq_exit(chip, desc);
@@ -317,8 +353,45 @@ static int uniphier_pcie_host_init(struct pcie_port *pp)
 	return 0;
 }
 
+static int uniphier_pcie_port_get_irq(struct pcie_port *pp, u32 service)
+{
+	struct pci_dev *pcidev;
+	int irq = 0;
+
+	if (!IS_ENABLED(CONFIG_PCIEAER) && !IS_ENABLED(CONFIG_PCIE_PME))
+		return 0;
+
+	/*
+	 * Finds the device that matches 'service' from the devices
+	 * associated with Root Port, and returns its IRQ number.
+	 */
+	list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) {
+		irq = pcie_port_service_get_irq(pcidev, service);
+		if (irq)
+			break;
+	}
+
+	return irq;
+}
+
+static int uniphier_pcie_host_init_complete(struct pcie_port *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
+
+	if (IS_ENABLED(CONFIG_PCIE_PME))
+		priv->pme_irq =
+			uniphier_pcie_port_get_irq(pp, PCIE_PORT_SERVICE_PME);
+	if (IS_ENABLED(CONFIG_PCIEAER))
+		priv->aer_irq =
+			uniphier_pcie_port_get_irq(pp, PCIE_PORT_SERVICE_AER);
+
+	return 0;
+}
+
 static const struct dw_pcie_host_ops uniphier_pcie_host_ops = {
 	.host_init = uniphier_pcie_host_init,
+	.msi_host_isr = uniphier_pcie_msi_host_isr,
 };
 
 static int uniphier_pcie_host_enable(struct uniphier_pcie_priv *priv)
@@ -398,7 +471,11 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
 
 	priv->pci.pp.ops = &uniphier_pcie_host_ops;
 
-	return dw_pcie_host_init(&priv->pci.pp);
+	ret = dw_pcie_host_init(&priv->pci.pp);
+	if (ret)
+		return ret;
+
+	return uniphier_pcie_host_init_complete(&priv->pci.pp);
 }
 
 static const struct of_device_id uniphier_pcie_match[] = {
-- 
2.7.4

Re: [PATCH v11 0/3] PCI: uniphier: Add PME/AER support for UniPhier PCIe host controller

[Kunihiko Hayashi]

Gentle Ping.
Are there any comments about this series?

Thank you,


On 2021/04/23 2:04, Kunihiko Hayashi wrote:
> This adds a new function called by MSI handler in DesignWare PCIe framework,
> that invokes PME and AER funcions to detect the factor from SoC-dependent
> registers.
> 
> Changes since v10:
> - Fix comment style in the header
> - Fix "virq" and "vIRQ" in variable name and description to "irq" and "IRQ"
> 
> Changes since v9:
> - Fix the description of pcie_prot_service_get_irq()
> 
> Changes since v8:
> - Add uniphier_pcie_host_init_complete() that finds PME/AER IRQ number
>    after calling dw_pcie_host_init()
> - Add conditions to depend on CONFIG_PCIE_PME and CONFIG_PCIEAER instead
>    of CONFIG_PCIEPORTBUS
> - Add Acked-by: line to portdrv patch
> 
> Changes since v7:
> - Add Reviewed-by: line to 1st and 3rd patches
> 
> Changes since v6:
> - Separate patches for iATU and phy error from this series
> - Add Reviewed-by: line to dwc patch
> 
> Changes since v5:
> - Add pcie_port_service_get_irq() function to pcie/portdrv
> - Call pcie_port_service_get_irq() to get IRQ interrupt number for PME/AER
> - Rebase to the latest linux-next branch,
>    and remove devm_platform_ioremap_resource_byname() replacement patch
> 
> Changes since v4:
> - Add Acked-by: line to dwc patch
> 
> Changes since v3:
> - Move msi_host_isr() call into dw_handle_msi_irq()
> - Move uniphier_pcie_misc_isr() call into the guard of chained_irq
> - Use a bool argument is_msi instead of pci_msi_enabled()
> - Consolidate handler calls for the same interrupt
> - Fix typos in commit messages
> 
> Changes since v2:
> - Avoid printing phy error message in case of EPROBE_DEFER
> - Fix iATU register mapping method
> - dt-bindings: Add Acked-by: line
> - Fix typos in commit messages
> - Use devm_platform_ioremap_resource_byname()
> 
> Changes since v1:
> - Add check if struct resource is NULL
> - Fix warning in the type of dev_err() argument
> 
> Kunihiko Hayashi (3):
>    PCI: portdrv: Add pcie_port_service_get_irq() function
>    PCI: dwc: Add msi_host_isr() callback
>    PCI: uniphier: Add misc interrupt handler to invoke PME and AER
> 
>   drivers/pci/controller/dwc/pcie-designware-host.c |   3 +
>   drivers/pci/controller/dwc/pcie-designware.h      |   1 +
>   drivers/pci/controller/dwc/pcie-uniphier.c        | 105 +++++++++++++++++++---
>   drivers/pci/pcie/portdrv.h                        |   1 +
>   drivers/pci/pcie/portdrv_core.c                   |  16 ++++
>   5 files changed, 112 insertions(+), 14 deletions(-)
> 

-- 
---
Best Regards
Kunihiko Hayashi

Re: [PATCH v11 3/3] PCI: uniphier: Add misc interrupt handler to invoke PME and AER

[Pali Rohár]

Hello Kunihiko!

On Friday 23 April 2021 02:04:57 Kunihiko Hayashi wrote:
> This patch adds misc interrupt handler to detect and invoke PME/AER event.
> 
> In UniPhier PCIe controller, PME/AER signals are assigned to the same
> signal as MSI by the internal logic. These signals should be detected by
> the internal register, however, DWC MSI handler can't handle these signals.
> 
> DWC MSI handler calls .msi_host_isr() callback function, that detects
> PME/AER signals using the internal register and invokes the interrupt
> with PME/AER IRQ numbers.
> 
> These IRQ numbers is obtained by uniphier_pcie_port_get_irq() function,
> that finds the device that matches PME/AER from the devices associated
> with Root Port, and returns its IRQ number.

If I understood this issue correctly, it means that your PCIe controller
does not issue regular MSI interrupt for PME and AER events, but rather
it issue controller specific interrupt and you need to figure out what
kind of controller-specific event happened (e.g. PME or AER or something
else).

But if your controller supports PME or AER then it expose in its PCIe
Root Port capabilities register MSI number for these PME and AER events.
Kernel PCIe PME and AER drivers read from capabilities register these
numbers and register irq functions to be called when interrupt happens.

So it means that you do not need to implement uniphier_pcie_port_get_irq
function via this "ugly" foreach and call pcie_port_service_get_irq. But
you can read this MSI interrupt number directly from your controller in
this pcie-uniphier.c driver and then use irq_find_mapping() to convert
hw MSI number to kernel's virq (used in generic_handle_irq()).

Because currently you use in pcie-uniphier.c call to function
pcie_port_service_get_irq() which returns cached interrupt number value
which was read from PCIe Root Port capability register by PCI subsystem
callbacked back to the pcie-uniphier.c driver.

For me this looks like "ugly" if you need to do something in
"complicated" way and add dependency e.g. on compile options like
"if (!IS_ENABLED(CONFIG_PCIEAER) && !IS_ENABLED(CONFIG_PCIE_PME))" if it
can be easily avoided.

I'm writing this because I was solving exactly same problem for aardvark
PCIe controller with PME, AER and HP interrupts (patches are on ML). So
I think that this pcie-uniphier.c implementation can be simplified
without need to use checks for CONFIG_* options and calling
pcie_port_service_get_irq() in list_for_each_entry loop.


Could you please post output of 'lspci -nn -vv'? In my opinion MSI
numbers for AER and PME in Root Port could be constant so it may
simplify implementation even more. (Just to note that in my case
aardvark returns zero as MSI number and it is also documented in spec).

> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-uniphier.c | 105 +++++++++++++++++++++++++----
>  1 file changed, 91 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
> index 7e8bad3..dcd8fa8 100644
> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
> @@ -21,6 +21,7 @@
>  #include <linux/reset.h>
>  
>  #include "pcie-designware.h"
> +#include "../../pcie/portdrv.h"
>  
>  #define PCL_PINCTRL0			0x002c
>  #define PCL_PERST_PLDN_REGEN		BIT(12)
> @@ -44,7 +45,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)
> @@ -68,6 +71,8 @@ struct uniphier_pcie_priv {
>  	struct reset_control *rst;
>  	struct phy *phy;
>  	struct irq_domain *legacy_irq_domain;
> +	int aer_irq;
> +	int pme_irq;
>  };
>  
>  #define to_uniphier_pcie(x)	dev_get_drvdata((x)->dev)
> @@ -164,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;
> +
> +	writel(val, priv->base + PCL_RCV_INT);
>  	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
>  }
>  
> @@ -228,28 +241,51 @@ 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;
>  
> -	/* 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 (priv->aer_irq)
> +				generic_handle_irq(priv->aer_irq);
> +		}
> +
> +		if (val & PCL_CFG_PME_MSI_STATUS) {
> +			dev_dbg(pci->dev, "PME Interrupt\n");
> +			if (priv->pme_irq)
> +				generic_handle_irq(priv->pme_irq);
> +		}
> +	}
>  
>  	writel(val, priv->base + PCL_RCV_INT);
> +}
> +
> +static void uniphier_pcie_msi_host_isr(struct pcie_port *pp)
> +{
> +	uniphier_pcie_misc_isr(pp, true);
> +}
> +
> +static void uniphier_pcie_irq_handler(struct irq_desc *desc)
> +{
> +	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, irq;
> +
> +	uniphier_pcie_misc_isr(pp, false);
>  
>  	/* INTx */
>  	chained_irq_enter(chip, desc);
> @@ -258,8 +294,8 @@ static void uniphier_pcie_irq_handler(struct irq_desc *desc)
>  	reg = FIELD_GET(PCL_RCV_INTX_ALL_STATUS, val);
>  
>  	for_each_set_bit(bit, &reg, PCI_NUM_INTX) {
> -		virq = irq_linear_revmap(priv->legacy_irq_domain, bit);
> -		generic_handle_irq(virq);
> +		irq = irq_linear_revmap(priv->legacy_irq_domain, bit);
> +		generic_handle_irq(irq);
>  	}
>  
>  	chained_irq_exit(chip, desc);
> @@ -317,8 +353,45 @@ static int uniphier_pcie_host_init(struct pcie_port *pp)
>  	return 0;
>  }
>  
> +static int uniphier_pcie_port_get_irq(struct pcie_port *pp, u32 service)
> +{
> +	struct pci_dev *pcidev;
> +	int irq = 0;
> +
> +	if (!IS_ENABLED(CONFIG_PCIEAER) && !IS_ENABLED(CONFIG_PCIE_PME))
> +		return 0;
> +
> +	/*
> +	 * Finds the device that matches 'service' from the devices
> +	 * associated with Root Port, and returns its IRQ number.
> +	 */
> +	list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) {
> +		irq = pcie_port_service_get_irq(pcidev, service);
> +		if (irq)
> +			break;
> +	}
> +
> +	return irq;
> +}
> +
> +static int uniphier_pcie_host_init_complete(struct pcie_port *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> +
> +	if (IS_ENABLED(CONFIG_PCIE_PME))
> +		priv->pme_irq =
> +			uniphier_pcie_port_get_irq(pp, PCIE_PORT_SERVICE_PME);
> +	if (IS_ENABLED(CONFIG_PCIEAER))
> +		priv->aer_irq =
> +			uniphier_pcie_port_get_irq(pp, PCIE_PORT_SERVICE_AER);
> +
> +	return 0;
> +}
> +
>  static const struct dw_pcie_host_ops uniphier_pcie_host_ops = {
>  	.host_init = uniphier_pcie_host_init,
> +	.msi_host_isr = uniphier_pcie_msi_host_isr,
>  };
>  
>  static int uniphier_pcie_host_enable(struct uniphier_pcie_priv *priv)
> @@ -398,7 +471,11 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
>  
>  	priv->pci.pp.ops = &uniphier_pcie_host_ops;
>  
> -	return dw_pcie_host_init(&priv->pci.pp);
> +	ret = dw_pcie_host_init(&priv->pci.pp);
> +	if (ret)
> +		return ret;
> +
> +	return uniphier_pcie_host_init_complete(&priv->pci.pp);
>  }
>  
>  static const struct of_device_id uniphier_pcie_match[] = {
> -- 
> 2.7.4
> 

Re: [PATCH v11 3/3] PCI: uniphier: Add misc interrupt handler to invoke PME and AER

[Kunihiko Hayashi]

Hi Pali,

Thank you for considering about my patch.

On 2021/07/18 9:26, Pali Rohar wrote:

 > Hello Kunihiko!
 >
 > On Friday 23 April 2021 02:04:57 Kunihiko Hayashi wrote:
 > > This patch adds misc interrupt handler to detect and invoke PME/AER event.
 > >
 > > In UniPhier PCIe controller, PME/AER signals are assigned to the same
 > > signal as MSI by the internal logic. These signals should be detected by
 > > the internal register, however, DWC MSI handler can't handle these signals.
 > >
 > > DWC MSI handler calls .msi_host_isr() callback function, that detects
 > > PME/AER signals using the internal register and invokes the interrupt
 > > with PME/AER IRQ numbers.
 > >
 > > These IRQ numbers is obtained by uniphier_pcie_port_get_irq() function,
 > > that finds the device that matches PME/AER from the devices associated
 > > with Root Port, and returns its IRQ number.
 >
 > If I understood this issue correctly, it means that your PCIe controller
 > does not issue regular MSI interrupt for PME and AER events, but rather
 > it issue controller specific interrupt and you need to figure out what
 > kind of controller-specific event happened (e.g. PME or AER or something
 > else).

Your view is almost correct.
This controller consists of Synopsys DWC and the glue logic, and regular
MSI interrupt is handled in dw_pcie_msi_isr() for DWC.

The interrupt for PME/AER event is issued to CPU as the same interrupt
as MSI, though, PME/AER event is detected by the glue logic instead of DWC.
So the regular MSI handler can't handle the interrupt for PME/AER event
directly.


 > But if your controller supports PME or AER then it expose in its PCIe
 > Root Port capabilities register MSI number for these PME and AER events.
 > Kernel PCIe PME and AER drivers read from capabilities register these
 > numbers and register irq functions to be called when interrupt happens.

Yes, the controller also has the MSI number for PME/AER in Root Port
capability register (defined as PCI_ERR_ROOT_AER_IRQ and PCI_EXP_FLAGS_IRQ).

These interrupts are registered with these capability values in
pcie_port_enable_irq_vec().


 > So it means that you do not need to implement uniphier_pcie_port_get_irq
 > function via this "ugly" foreach and call pcie_port_service_get_irq. But
 > you can read this MSI interrupt number directly from your controller in
 > this pcie-uniphier.c driver and then use irq_find_mapping() to convert
 > hw MSI number to kernel's virq (used in generic_handle_irq()).
 >
 > Because currently you use in pcie-uniphier.c call to function
 > pcie_port_service_get_irq() which returns cached interrupt number value
 > which was read from PCIe Root Port capability register by PCI subsystem
 > callbacked back to the pcie-uniphier.c driver.
 >
 > For me this looks like "ugly" if you need to do something in
 > "complicated" way and add dependency e.g. on compile options like
 > "if (!IS_ENABLED(CONFIG_PCIEAER) && !IS_ENABLED(CONFIG_PCIE_PME))" if it
 > can be easily avoided.
 >
 > I'm writing this because I was solving exactly same problem for aardvark
 > PCIe controller with PME, AER and HP interrupts (patches are on ML). So
 > I think that this pcie-uniphier.c implementation can be simplified
 > without need to use checks for CONFIG_* options and calling
 > pcie_port_service_get_irq() in list_for_each_entry loop.

The interrupt for PME/AER event is detected by the glue logic.

When the handler needs to read the status register for PME/AER in the glue
logic and issue the correspond MSI interrupt using generic_handle_irq().

If the driver gets the MSI interrupt number directly like
pcie_message_numbers() function, I think this complicated method is
no longer necessary, too.


 > Could you please post output of 'lspci -nn -vv'? In my opinion MSI
 > numbers for AER and PME in Root Port could be constant so it may
 > simplify implementation even more. (Just to note that in my case
 > aardvark returns zero as MSI number and it is also documented in spec).

I already posted the lspci output[1].

[1] 
https://lore.kernel.org/linux-pci/1592469493-1549-3-git-send-email-hayashi.kunihiko@socionext.com/T/#e1145dab891debed1eadcddbf2b9f5fabb357f8b0

According to the spec, the initial MSI number for PME/AER is zero.
And this series up to v5 used fixed zero as the MSI number for PME/AER.

Thank you,

---
Best Regards
Kunihiko Hayashi