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

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

[Kunihiko Hayashi]

The original subject up to v6 is
"PCI: uniphier: Add features for UniPhier PCIe host controller".

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.

The iATU patches is split from this series as
"PCI: dwc: Move iATU register mapping to common framework".

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 vIRQ 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        | 77 +++++++++++++++++++----
 drivers/pci/pcie/portdrv.h                        |  1 +
 drivers/pci/pcie/portdrv_core.c                   | 16 +++++
 5 files changed, 87 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH v8 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>
---
 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 af7cf23..e256456 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -150,4 +150,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 50a9522..f92daf8 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -480,6 +480,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

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

[Bjorn Helgaas]

On Wed, Oct 28, 2020 at 10:31:41AM +0900, Kunihiko Hayashi wrote:
> 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>

I don't like portdrv and am sorry to add more uses of it, but I don't
have anything better, so:

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 af7cf23..e256456 100644
> --- a/drivers/pci/pcie/portdrv.h
> +++ b/drivers/pci/pcie/portdrv.h
> @@ -150,4 +150,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 50a9522..f92daf8 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -480,6 +480,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
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v8 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 674f32d..45eea6f 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 9d2f511..2ce3133 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -176,6 +176,7 @@ struct dw_pcie_host_ops {
 	int (*host_init)(struct pcie_port *pp);
 	void (*set_num_vectors)(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 v8 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 with the internal register and invokes the interrupt
with PME/AER vIRQ numbers.

These vIRQ numbers is obtained from portdrv in uniphier_add_pcie_port()
function.

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 | 77 +++++++++++++++++++++++++-----
 1 file changed, 66 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index 4817626..237537a 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)
@@ -167,7 +172,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);
 }
 
@@ -231,28 +244,52 @@ 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, virq;
+
+	uniphier_pcie_misc_isr(pp, false);
 
 	/* INTx */
 	chained_irq_enter(chip, desc);
@@ -329,6 +366,7 @@ static int uniphier_pcie_host_init(struct pcie_port *pp)
 
 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_add_pcie_port(struct uniphier_pcie_priv *priv,
@@ -337,6 +375,7 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,
 	struct dw_pcie *pci = &priv->pci;
 	struct pcie_port *pp = &pci->pp;
 	struct device *dev = &pdev->dev;
+	struct pci_dev *pcidev;
 	int ret;
 
 	pp->ops = &uniphier_pcie_host_ops;
@@ -353,6 +392,22 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,
 		return ret;
 	}
 
+	/* irq for PME */
+	list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) {
+		priv->pme_irq =
+			pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_PME);
+		if (priv->pme_irq)
+			break;
+	}
+
+	/* irq for AER */
+	list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) {
+		priv->aer_irq =
+			pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_AER);
+		if (priv->aer_irq)
+			break;
+	}
+
 	return 0;
 }
 
-- 
2.7.4

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

[Bjorn Helgaas]

On Wed, Oct 28, 2020 at 10:31:43AM +0900, 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.

I don't know what "PME/AER signals are assigned to the same signal as
MSI" means.  

I'm trying to figure out if this is talking about PME/AER MSI vector
numbers (probably not) or some internal wire that's not
architecturally visible or what.

Probably also not related to the fact that PME, hotplug, and bandwidth
notifications share the same MSI/MSI-X vector.

Is this something that's going to be applicable to all the DWC-based
drivers?

> DWC MSI handler calls .msi_host_isr() callback function, that detects
> PME/AER signals with the internal register and invokes the interrupt
> with PME/AER vIRQ numbers.
> 
> These vIRQ numbers is obtained from portdrv in uniphier_add_pcie_port()
> function.
> 
> 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 | 77 +++++++++++++++++++++++++-----
>  1 file changed, 66 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
> index 4817626..237537a 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)
> @@ -167,7 +172,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;

I'm confused about how this works.  Root Ports can signal AER errors
with either INTx or MSI.  This is controlled by the architected
Interrupt Disable bit and the MSI/MSI-X enable bits (I'm looking at
PCIe r5.0, sec 6.2.4.1.2).

The code here doesn't look related to those bits.  Does this code mean
that if pci_msi_enabled(), the Root Port will always signal with MSI
(if MSI Enable is set) and will *never* signal with INTx?

> +	writel(val, priv->base + PCL_RCV_INT);
>  	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
>  }
>  
> @@ -231,28 +244,52 @@ 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");
> +

Looks like a spurious whitespace change?

>  	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, virq;
> +
> +	uniphier_pcie_misc_isr(pp, false);
>  
>  	/* INTx */
>  	chained_irq_enter(chip, desc);
> @@ -329,6 +366,7 @@ static int uniphier_pcie_host_init(struct pcie_port *pp)
>  
>  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_add_pcie_port(struct uniphier_pcie_priv *priv,
> @@ -337,6 +375,7 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,
>  	struct dw_pcie *pci = &priv->pci;
>  	struct pcie_port *pp = &pci->pp;
>  	struct device *dev = &pdev->dev;
> +	struct pci_dev *pcidev;
>  	int ret;
>  
>  	pp->ops = &uniphier_pcie_host_ops;
> @@ -353,6 +392,22 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,
>  		return ret;
>  	}
>  
> +	/* irq for PME */
> +	list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) {
> +		priv->pme_irq =
> +			pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_PME);
> +		if (priv->pme_irq)
> +			break;

Does this mean that all Root Ports must use the same MSI vector?  I
don't think that's a PCIe spec requirement, though of course DWC may
have its own restrictions.

I don't think this depends on CONFIG_PCIEPORTBUS, so it looks like
it's possible to have

  # CONFIG_PCIEPORTBUS is not set
  PCIE_UNIPHIER=y

in which case I think you'll have a link error.

> +	}
> +
> +	/* irq for AER */
> +	list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) {
> +		priv->aer_irq =
> +			pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_AER);
> +		if (priv->aer_irq)
> +			break;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Lorenzo Pieralisi]

On Tue, Nov 24, 2020 at 05:20:37PM -0600, Bjorn Helgaas wrote:
> On Wed, Oct 28, 2020 at 10:31:43AM +0900, 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.
> 
> I don't know what "PME/AER signals are assigned to the same signal as
> MSI" means.  

The host controller embeds an interrupt-controller whose IRQ wire output
is cascaded into the main interrupt controller.

The host-bridge embedded controller receives MSI writes from devices
and it turns them into an edge IRQ into the main interrupt controller.

To ack/mask the MSIs at host contoller interrupt controller level, there
is a control register in the host controller that needs handling upon
IRQ reception.

The *RP* (and AFAIU the RP *only*) signals the PME/AER MSI using the
same wire to the main interrupt controller but its ack/mask is handled
by a different bit in the host bridge control register above, therefore
the cascaded IRQ isr needs to know which virq it is actually handling
to ack/mask accordingly.

IMO this should be modelled with a separate IRQ domain and chip for
the root port (yes this implies describing the root port in the dts
file with a separate msi-parent).

This series as it stands is a kludge.

Lorenzo

> I'm trying to figure out if this is talking about PME/AER MSI vector
> numbers (probably not) or some internal wire that's not
> architecturally visible or what.
> 
> Probably also not related to the fact that PME, hotplug, and bandwidth
> notifications share the same MSI/MSI-X vector.
> 
> Is this something that's going to be applicable to all the DWC-based
> drivers?
> 
> > DWC MSI handler calls .msi_host_isr() callback function, that detects
> > PME/AER signals with the internal register and invokes the interrupt
> > with PME/AER vIRQ numbers.
> > 
> > These vIRQ numbers is obtained from portdrv in uniphier_add_pcie_port()
> > function.
> > 
> > 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 | 77 +++++++++++++++++++++++++-----
> >  1 file changed, 66 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
> > index 4817626..237537a 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)
> > @@ -167,7 +172,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;
> 
> I'm confused about how this works.  Root Ports can signal AER errors
> with either INTx or MSI.  This is controlled by the architected
> Interrupt Disable bit and the MSI/MSI-X enable bits (I'm looking at
> PCIe r5.0, sec 6.2.4.1.2).
> 
> The code here doesn't look related to those bits.  Does this code mean
> that if pci_msi_enabled(), the Root Port will always signal with MSI
> (if MSI Enable is set) and will *never* signal with INTx?
> 
> > +	writel(val, priv->base + PCL_RCV_INT);
> >  	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
> >  }
> >  
> > @@ -231,28 +244,52 @@ 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");
> > +
> 
> Looks like a spurious whitespace change?
> 
> >  	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, virq;
> > +
> > +	uniphier_pcie_misc_isr(pp, false);
> >  
> >  	/* INTx */
> >  	chained_irq_enter(chip, desc);
> > @@ -329,6 +366,7 @@ static int uniphier_pcie_host_init(struct pcie_port *pp)
> >  
> >  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_add_pcie_port(struct uniphier_pcie_priv *priv,
> > @@ -337,6 +375,7 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,
> >  	struct dw_pcie *pci = &priv->pci;
> >  	struct pcie_port *pp = &pci->pp;
> >  	struct device *dev = &pdev->dev;
> > +	struct pci_dev *pcidev;
> >  	int ret;
> >  
> >  	pp->ops = &uniphier_pcie_host_ops;
> > @@ -353,6 +392,22 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,
> >  		return ret;
> >  	}
> >  
> > +	/* irq for PME */
> > +	list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) {
> > +		priv->pme_irq =
> > +			pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_PME);
> > +		if (priv->pme_irq)
> > +			break;
> 
> Does this mean that all Root Ports must use the same MSI vector?  I
> don't think that's a PCIe spec requirement, though of course DWC may
> have its own restrictions.
> 
> I don't think this depends on CONFIG_PCIEPORTBUS, so it looks like
> it's possible to have
> 
>   # CONFIG_PCIEPORTBUS is not set
>   PCIE_UNIPHIER=y
> 
> in which case I think you'll have a link error.
> 
> > +	}
> > +
> > +	/* irq for AER */
> > +	list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) {
> > +		priv->aer_irq =
> > +			pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_AER);
> > +		if (priv->aer_irq)
> > +			break;
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.7.4
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Kunihiko Hayashi]

Hi Bjorn Lorenzo,

On 2020/11/25 19:23, Lorenzo Pieralisi wrote:
> On Tue, Nov 24, 2020 at 05:20:37PM -0600, Bjorn Helgaas wrote:
>> On Wed, Oct 28, 2020 at 10:31:43AM +0900, 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.
>>
>> I don't know what "PME/AER signals are assigned to the same signal as
>> MSI" means.
> 
> The host controller embeds an interrupt-controller whose IRQ wire output
> is cascaded into the main interrupt controller.
> 
> The host-bridge embedded controller receives MSI writes from devices
> and it turns them into an edge IRQ into the main interrupt controller.
> 
> To ack/mask the MSIs at host contoller interrupt controller level, there
> is a control register in the host controller that needs handling upon
> IRQ reception.

Thanks for explaining that.
In my understanding, PME/AER signals are cascaded to MSI by embedded
interrupt controller (not "assigned").


> The *RP* (and AFAIU the RP *only*) signals the PME/AER MSI using the
> same wire to the main interrupt controller but its ack/mask is handled
> by a different bit in the host bridge control register above, therefore
> the cascaded IRQ isr needs to know which virq it is actually handling
> to ack/mask accordingly.

Sorry what is RP? Root complex or something?


> IMO this should be modelled with a separate IRQ domain and chip for
> the root port (yes this implies describing the root port in the dts
> file with a separate msi-parent).
> 
> This series as it stands is a kludge.

I see. However I need some time to consider the way to separate IRQ domain.
Is there any idea or example to handle PME/AER with IRQ domain?


>> I'm trying to figure out if this is talking about PME/AER MSI vector
>> numbers (probably not) or some internal wire that's not
>> architecturally visible or what.
>>
>> Probably also not related to the fact that PME, hotplug, and bandwidth
>> notifications share the same MSI/MSI-X vector.
>>
>> Is this something that's going to be applicable to all the DWC-based
>> drivers?

I think that this feature depends on the vendor specification.
At least, the registers to control or check these signals are implemented
in the vendor's logic.


>>> DWC MSI handler calls .msi_host_isr() callback function, that detects
>>> PME/AER signals with the internal register and invokes the interrupt
>>> with PME/AER vIRQ numbers.
>>>
>>> These vIRQ numbers is obtained from portdrv in uniphier_add_pcie_port()
>>> function.
>>>
>>> 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 | 77 +++++++++++++++++++++++++-----
>>>   1 file changed, 66 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
>>> index 4817626..237537a 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)
>>> @@ -167,7 +172,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;
>>
>> I'm confused about how this works.  Root Ports can signal AER errors
>> with either INTx or MSI.  This is controlled by the architected
>> Interrupt Disable bit and the MSI/MSI-X enable bits (I'm looking at
>> PCIe r5.0, sec 6.2.4.1.2).
>>
>> The code here doesn't look related to those bits.  Does this code mean
>> that if pci_msi_enabled(), the Root Port will always signal with MSI
>> (if MSI Enable is set) and will *never* signal with INTx?

According to the spec sheet, we need to set interrupt enable bit for either
INTx or MSI, the other bit should be reset. These bits are in config space
and handled by the framework.

The controller signals AER errors with the interrupt that is either INTx
or MSI enabled. I think that the only way to know if MSI is enabled
(and INTX is disabled) is to use pci_msi_enabled().


>>> +	writel(val, priv->base + PCL_RCV_INT);
>>>   	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
>>>   }
>>>   
>>> @@ -231,28 +244,52 @@ 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");
>>> +
>>
>> Looks like a spurious whitespace change?

Oops, I'll remove it.


>>>   	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, virq;
>>> +
>>> +	uniphier_pcie_misc_isr(pp, false);
>>>   
>>>   	/* INTx */
>>>   	chained_irq_enter(chip, desc);
>>> @@ -329,6 +366,7 @@ static int uniphier_pcie_host_init(struct pcie_port *pp)
>>>   
>>>   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_add_pcie_port(struct uniphier_pcie_priv *priv,
>>> @@ -337,6 +375,7 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,
>>>   	struct dw_pcie *pci = &priv->pci;
>>>   	struct pcie_port *pp = &pci->pp;
>>>   	struct device *dev = &pdev->dev;
>>> +	struct pci_dev *pcidev;
>>>   	int ret;
>>>   
>>>   	pp->ops = &uniphier_pcie_host_ops;
>>> @@ -353,6 +392,22 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,
>>>   		return ret;
>>>   	}
>>>   
>>> +	/* irq for PME */
>>> +	list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) {
>>> +		priv->pme_irq =
>>> +			pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_PME);
>>> +		if (priv->pme_irq)
>>> +			break;
>>
>> Does this mean that all Root Ports must use the same MSI vector?  I
>> don't think that's a PCIe spec requirement, though of course DWC may
>> have its own restrictions.
>>

This controller has one port implementation only,
so this assumes that there is one root port.


>> I don't think this depends on CONFIG_PCIEPORTBUS, so it looks like
>> it's possible to have
>>
>>    # CONFIG_PCIEPORTBUS is not set
>>    PCIE_UNIPHIER=y
>>
>> in which case I think you'll have a link error.

Indeed. To use port functions needs to define PCIEPORTBUS.
I'll update PCIE_UNIPHIER in Kconfig.


Thank you,

---
Best Regards
Kunihiko Hayashi

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

[Pali Rohár]

Hello Kunihiko! Now I found also this older email...

On Friday 27 November 2020 21:02:05 Kunihiko Hayashi wrote:
> Hi Bjorn Lorenzo,
> 
> On 2020/11/25 19:23, Lorenzo Pieralisi wrote:
> > On Tue, Nov 24, 2020 at 05:20:37PM -0600, Bjorn Helgaas wrote:
> > > On Wed, Oct 28, 2020 at 10:31:43AM +0900, 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.
> > > 
> > > I don't know what "PME/AER signals are assigned to the same signal as
> > > MSI" means.
> > 
> > The host controller embeds an interrupt-controller whose IRQ wire output
> > is cascaded into the main interrupt controller.
> > 
> > The host-bridge embedded controller receives MSI writes from devices
> > and it turns them into an edge IRQ into the main interrupt controller.
> > 
> > To ack/mask the MSIs at host contoller interrupt controller level, there
> > is a control register in the host controller that needs handling upon
> > IRQ reception.
> 
> Thanks for explaining that.
> In my understanding, PME/AER signals are cascaded to MSI by embedded
> interrupt controller (not "assigned").
> 
> 
> > The *RP* (and AFAIU the RP *only*) signals the PME/AER MSI using the
> > same wire to the main interrupt controller but its ack/mask is handled
> > by a different bit in the host bridge control register above, therefore
> > the cascaded IRQ isr needs to know which virq it is actually handling
> > to ack/mask accordingly.
> 
> Sorry what is RP? Root complex or something?

RP = Root Port

In lspci output you can find it as "root" of the tree topology and
should have "PCI bridge" class/name.

> > IMO this should be modelled with a separate IRQ domain and chip for
> > the root port (yes this implies describing the root port in the dts
> > file with a separate msi-parent).
> > 
> > This series as it stands is a kludge.
> 
> I see. However I need some time to consider the way to separate IRQ domain.
> Is there any idea or example to handle PME/AER with IRQ domain?

Seems that you are dealing with very similar issues as me with aardvark
driver.

As an inspiration look at my aardvark patch which setup separate IRQ
domain for PME, AER and HP interrupts:
https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/

Thanks to custom driver map_irq function, it is not needed to describe
root port with separate msi-parent in DTS.

> > > I'm trying to figure out if this is talking about PME/AER MSI vector
> > > numbers (probably not)

Bjorn, see my email, based on my experience with aardvark controller I
think they are MSI vector numbers, but controller instead uses own
proprietary way how to signal PME and AER interrupts.
https://lore.kernel.org/linux-pci/20210718002614.3l74hlondwgthuby@pali/

> > > or some internal wire that's not
> > > architecturally visible or what.
> > > 
> > > Probably also not related to the fact that PME, hotplug, and bandwidth
> > > notifications share the same MSI/MSI-X vector.
> > > 
> > > Is this something that's going to be applicable to all the DWC-based
> > > drivers?
> 
> I think that this feature depends on the vendor specification.
> At least, the registers to control or check these signals are implemented
> in the vendor's logic.
> 
> 
> > > > DWC MSI handler calls .msi_host_isr() callback function, that detects
> > > > PME/AER signals with the internal register and invokes the interrupt
> > > > with PME/AER vIRQ numbers.
> > > > 
> > > > These vIRQ numbers is obtained from portdrv in uniphier_add_pcie_port()
> > > > function.
> > > > 
> > > > 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 | 77 +++++++++++++++++++++++++-----
> > > >   1 file changed, 66 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
> > > > index 4817626..237537a 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)
> > > > @@ -167,7 +172,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;
> > > 
> > > I'm confused about how this works.  Root Ports can signal AER errors
> > > with either INTx or MSI.  This is controlled by the architected
> > > Interrupt Disable bit and the MSI/MSI-X enable bits (I'm looking at
> > > PCIe r5.0, sec 6.2.4.1.2).
> > > 
> > > The code here doesn't look related to those bits.  Does this code mean
> > > that if pci_msi_enabled(), the Root Port will always signal with MSI
> > > (if MSI Enable is set) and will *never* signal with INTx?
> 
> According to the spec sheet, we need to set interrupt enable bit for either
> INTx or MSI, the other bit should be reset. These bits are in config space
> and handled by the framework.

Is spec sheet available publicly?

> The controller signals AER errors with the interrupt that is either INTx
> or MSI enabled. I think that the only way to know if MSI is enabled
> (and INTX is disabled) is to use pci_msi_enabled().
> 
> 
> > > > +	writel(val, priv->base + PCL_RCV_INT);
> > > >   	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
> > > >   }
> > > > @@ -231,28 +244,52 @@ 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");
> > > > +
> > > 
> > > Looks like a spurious whitespace change?
> 
> Oops, I'll remove it.
> 
> 
> > > >   	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, virq;
> > > > +
> > > > +	uniphier_pcie_misc_isr(pp, false);
> > > >   	/* INTx */
> > > >   	chained_irq_enter(chip, desc);
> > > > @@ -329,6 +366,7 @@ static int uniphier_pcie_host_init(struct pcie_port *pp)
> > > >   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_add_pcie_port(struct uniphier_pcie_priv *priv,
> > > > @@ -337,6 +375,7 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,
> > > >   	struct dw_pcie *pci = &priv->pci;
> > > >   	struct pcie_port *pp = &pci->pp;
> > > >   	struct device *dev = &pdev->dev;
> > > > +	struct pci_dev *pcidev;
> > > >   	int ret;
> > > >   	pp->ops = &uniphier_pcie_host_ops;
> > > > @@ -353,6 +392,22 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,
> > > >   		return ret;
> > > >   	}
> > > > +	/* irq for PME */
> > > > +	list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) {
> > > > +		priv->pme_irq =
> > > > +			pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_PME);
> > > > +		if (priv->pme_irq)
> > > > +			break;
> > > 
> > > Does this mean that all Root Ports must use the same MSI vector?  I
> > > don't think that's a PCIe spec requirement, though of course DWC may
> > > have its own restrictions.
> > > 
> 
> This controller has one port implementation only,
> so this assumes that there is one root port.
> 
> 
> > > I don't think this depends on CONFIG_PCIEPORTBUS, so it looks like
> > > it's possible to have
> > > 
> > >    # CONFIG_PCIEPORTBUS is not set
> > >    PCIE_UNIPHIER=y
> > > 
> > > in which case I think you'll have a link error.
> 
> Indeed. To use port functions needs to define PCIEPORTBUS.
> I'll update PCIE_UNIPHIER in Kconfig.
> 
> 
> Thank you,
> 
> ---
> Best Regards
> Kunihiko Hayashi

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

[Kunihiko Hayashi]

Hi Pali.

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

 > Hello Kunihiko! Now I found also this older email...
 >
 > On Friday 27 November 2020 21:02:05 Kunihiko Hayashi wrote:
 > > Hi Bjorn Lorenzo,
 > >
 > > On 2020/11/25 19:23, Lorenzo Pieralisi wrote:
 > > > On Tue, Nov 24, 2020 at 05:20:37PM -0600, Bjorn Helgaas wrote:
 > > > > On Wed, Oct 28, 2020 at 10:31:43AM +0900, 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.
 > > > >
 > > > > I don't know what "PME/AER signals are assigned to the same signal as
 > > > > MSI" means.
 > > >
 > > > The host controller embeds an interrupt-controller whose IRQ wire output
 > > > is cascaded into the main interrupt controller.
 > > >
 > > > The host-bridge embedded controller receives MSI writes from devices
 > > > and it turns them into an edge IRQ into the main interrupt controller.
 > > >
 > > > To ack/mask the MSIs at host contoller interrupt controller level, there
 > > > is a control register in the host controller that needs handling upon
 > > > IRQ reception.
 > >
 > > Thanks for explaining that.
 > > In my understanding, PME/AER signals are cascaded to MSI by embedded
 > > interrupt controller (not "assigned").
 > >
 > >
 > > > The *RP* (and AFAIU the RP *only*) signals the PME/AER MSI using the
 > > > same wire to the main interrupt controller but its ack/mask is handled
 > > > by a different bit in the host bridge control register above, therefore
 > > > the cascaded IRQ isr needs to know which virq it is actually handling
 > > > to ack/mask accordingly.
 > >
 > > Sorry what is RP? Root complex or something?
 >
 > RP = Root Port
 >
 > In lspci output you can find it as "root" of the tree topology and
 > should have "PCI bridge" class/name.

Ok, I understand.

 >
 > > > IMO this should be modelled with a separate IRQ domain and chip for
 > > > the root port (yes this implies describing the root port in the dts
 > > > file with a separate msi-parent).
 > > >
 > > > This series as it stands is a kludge.
 > >
 > > I see. However I need some time to consider the way to separate IRQ domain.
 > > Is there any idea or example to handle PME/AER with IRQ domain?
 >
 > Seems that you are dealing with very similar issues as me with aardvark
 > driver.
 >
 > As an inspiration look at my aardvark patch which setup separate IRQ
 > domain for PME, AER and HP interrupts:
 > https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/
 >
 > Thanks to custom driver map_irq function, it is not needed to describe
 > root port with separate msi-parent in DTS.

I need to understand your solution, though, this might be the same situation as my driver.

 > > > > I'm trying to figure out if this is talking about PME/AER MSI vector
 > > > > numbers (probably not)
 >
 > Bjorn, see my email, based on my experience with aardvark controller I
 > think they are MSI vector numbers, but controller instead uses own
 > proprietary way how to signal PME and AER interrupts.
 > https://lore.kernel.org/linux-pci/20210718002614.3l74hlondwgthuby@pali/
 >
 > > > > or some internal wire that's not
 > > > > architecturally visible or what.
 > > > >
 > > > > Probably also not related to the fact that PME, hotplug, and bandwidth
 > > > > notifications share the same MSI/MSI-X vector.
 > > > >
 > > > > Is this something that's going to be applicable to all the DWC-based
 > > > > drivers?
 > >
 > > I think that this feature depends on the vendor specification.
 > > At least, the registers to control or check these signals are implemented
 > > in the vendor's logic.
 > >
 > >
 > > > > > DWC MSI handler calls .msi_host_isr() callback function, that detects
 > > > > > PME/AER signals with the internal register and invokes the interrupt
 > > > > > with PME/AER vIRQ numbers.
 > > > > >
 > > > > > These vIRQ numbers is obtained from portdrv in uniphier_add_pcie_port()
 > > > > > function.
 > > > > >
 > > > > > 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 | 77 +++++++++++++++++++++++++-----
 > > > > >   1 file changed, 66 insertions(+), 11 deletions(-)
 > > > > >
 > > > > > diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c 
b/drivers/pci/controller/dwc/pcie-uniphier.c
 > > > > > index 4817626..237537a 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)
 > > > > > @@ -167,7 +172,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;
 > > > >
 > > > > I'm confused about how this works.  Root Ports can signal AER errors
 > > > > with either INTx or MSI.  This is controlled by the architected
 > > > > Interrupt Disable bit and the MSI/MSI-X enable bits (I'm looking at
 > > > > PCIe r5.0, sec 6.2.4.1.2).
 > > > >
 > > > > The code here doesn't look related to those bits.  Does this code mean
 > > > > that if pci_msi_enabled(), the Root Port will always signal with MSI
 > > > > (if MSI Enable is set) and will *never* signal with INTx?
 > >
 > > According to the spec sheet, we need to set interrupt enable bit for either
 > > INTx or MSI, the other bit should be reset. These bits are in config space
 > > and handled by the framework.
 >
 > Is spec sheet available publicly?

Sorry the spec sheet isn't open to the public.
Currently initial configuration is to use MSI, and I assume MSI interrupt is used
as default.

Thank you,

---
Best Regards
Kunihiko Hayashi

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

[Pali Rohár]

On Friday 23 July 2021 01:54:10 Kunihiko Hayashi wrote:
> On 2021/07/18 9:51, Pali Rohar wrote:
> > > > IMO this should be modelled with a separate IRQ domain and chip for
> > > > the root port (yes this implies describing the root port in the dts
> > > > file with a separate msi-parent).
> > > >
> > > > This series as it stands is a kludge.
> > >
> > > I see. However I need some time to consider the way to separate IRQ domain.
> > > Is there any idea or example to handle PME/AER with IRQ domain?
> >
> > Seems that you are dealing with very similar issues as me with aardvark
> > driver.
> >
> > As an inspiration look at my aardvark patch which setup separate IRQ
> > domain for PME, AER and HP interrupts:
> > https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/
> >
> > Thanks to custom driver map_irq function, it is not needed to describe
> > root port with separate msi-parent in DTS.
> 
> I need to understand your solution, though, this might be the same situation as my driver.

I think it is very very similar as aardvark also returns zero as hw irq
number (and it is not possible to change it).

So simple solution for you is also to register separate IRQ domain for
Root Port Bridge and then re-trigger interrupt with number 0 (which you
wrote that is default) as:

    virq = irq_find_mapping(priv->irq_domain, 0);
    generic_handle_irq(virq);

in your uniphier_pcie_misc_isr() function.

There is no need to modify DTS. And also no need to use complicated
logic for finding registered virq number via pcie_port_service_get_irq()
and uniphier_pcie_port_get_irq() functions.

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

[Kunihiko Hayashi]

Hi Pali,

On 2021/07/23 2:26, Pali Rohár wrote:
> On Friday 23 July 2021 01:54:10 Kunihiko Hayashi wrote:
>> On 2021/07/18 9:51, Pali Rohar wrote:
>>>>> IMO this should be modelled with a separate IRQ domain and chip for
>>>>> the root port (yes this implies describing the root port in the dts
>>>>> file with a separate msi-parent).
>>>>>
>>>>> This series as it stands is a kludge.
>>>>
>>>> I see. However I need some time to consider the way to separate IRQ domain.
>>>> Is there any idea or example to handle PME/AER with IRQ domain?
>>>
>>> Seems that you are dealing with very similar issues as me with aardvark
>>> driver.
>>>
>>> As an inspiration look at my aardvark patch which setup separate IRQ
>>> domain for PME, AER and HP interrupts:
>>> https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/
>>>
>>> Thanks to custom driver map_irq function, it is not needed to describe
>>> root port with separate msi-parent in DTS.
>>
>> I need to understand your solution, though, this might be the same situation as my driver.
> 
> I think it is very very similar as aardvark also returns zero as hw irq
> number (and it is not possible to change it).
> 
> So simple solution for you is also to register separate IRQ domain for
> Root Port Bridge and then re-trigger interrupt with number 0 (which you
> wrote that is default) as:
> 
>      virq = irq_find_mapping(priv->irq_domain, 0);
>      generic_handle_irq(virq);
> 
> in your uniphier_pcie_misc_isr() function.

I'm not sure "register separate IRQ domain for Root Port Bridge".
Do you mean that your suggestion is to create new IRQ domain, and add this domain to root port?
Or could you show me something example?

The re-trigger part is the same method as v5 patch I wrote.

> There is no need to modify DTS. And also no need to use complicated
> logic for finding registered virq number via pcie_port_service_get_irq()
> and uniphier_pcie_port_get_irq() functions.

I see.
GIC interrupt for MSI is handled by the MSI domain by pcie-designware-host.c.
My concern is how to trigger PME/AER event with another IRQ domain.

Thank you,

---
Best Regards
Kunihiko Hayashi

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

[Pali Rohár]

On Friday 23 July 2021 15:59:12 Kunihiko Hayashi wrote:
> Hi Pali,
> 
> On 2021/07/23 2:26, Pali Rohár wrote:
> > On Friday 23 July 2021 01:54:10 Kunihiko Hayashi wrote:
> > > On 2021/07/18 9:51, Pali Rohar wrote:
> > > > > > IMO this should be modelled with a separate IRQ domain and chip for
> > > > > > the root port (yes this implies describing the root port in the dts
> > > > > > file with a separate msi-parent).
> > > > > > 
> > > > > > This series as it stands is a kludge.
> > > > > 
> > > > > I see. However I need some time to consider the way to separate IRQ domain.
> > > > > Is there any idea or example to handle PME/AER with IRQ domain?
> > > > 
> > > > Seems that you are dealing with very similar issues as me with aardvark
> > > > driver.
> > > > 
> > > > As an inspiration look at my aardvark patch which setup separate IRQ
> > > > domain for PME, AER and HP interrupts:
> > > > https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/
> > > > 
> > > > Thanks to custom driver map_irq function, it is not needed to describe
> > > > root port with separate msi-parent in DTS.
> > > 
> > > I need to understand your solution, though, this might be the same situation as my driver.
> > 
> > I think it is very very similar as aardvark also returns zero as hw irq
> > number (and it is not possible to change it).
> > 
> > So simple solution for you is also to register separate IRQ domain for
> > Root Port Bridge and then re-trigger interrupt with number 0 (which you
> > wrote that is default) as:
> > 
> >      virq = irq_find_mapping(priv->irq_domain, 0);
> >      generic_handle_irq(virq);
> > 
> > in your uniphier_pcie_misc_isr() function.
> 
> I'm not sure "register separate IRQ domain for Root Port Bridge".
> Do you mean that your suggestion is to create new IRQ domain, and add this domain to root port?

Yes.

> Or could you show me something example?

I have already sent link to patch above which it implements for
pci-aardvark.c driver.

https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/

In device prove callback register domain by irq_domain_add_linear().
In bridge map_irq() callback use irq_create_mapping() for Root Port
device (and otherwise default of_irq_parse_and_map_pci()). And in
uniphier_pcie_misc_isr() retrigger interrupt into new domain.

> The re-trigger part is the same method as v5 patch I wrote.

Just you need to specify that new/private IRQ domain into
irq_find_mapping() call.

> > There is no need to modify DTS. And also no need to use complicated
> > logic for finding registered virq number via pcie_port_service_get_irq()
> > and uniphier_pcie_port_get_irq() functions.
> 
> I see.
> GIC interrupt for MSI is handled by the MSI domain by pcie-designware-host.c.
> My concern is how to trigger PME/AER event with another IRQ domain.
> 
> Thank you,
> 
> ---
> Best Regards
> Kunihiko Hayashi

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

[Kunihiko Hayashi]

Hi Pali,

On 2021/07/23 17:37, Pali Rohár wrote:
> On Friday 23 July 2021 15:59:12 Kunihiko Hayashi wrote:
>> Hi Pali,
>>
>> On 2021/07/23 2:26, Pali Rohár wrote:
>>> On Friday 23 July 2021 01:54:10 Kunihiko Hayashi wrote:
>>>> On 2021/07/18 9:51, Pali Rohar wrote:
>>>>>>> IMO this should be modelled with a separate IRQ domain and chip for
>>>>>>> the root port (yes this implies describing the root port in the dts
>>>>>>> file with a separate msi-parent).
>>>>>>>
>>>>>>> This series as it stands is a kludge.
>>>>>>
>>>>>> I see. However I need some time to consider the way to separate IRQ domain.
>>>>>> Is there any idea or example to handle PME/AER with IRQ domain?
>>>>>
>>>>> Seems that you are dealing with very similar issues as me with aardvark
>>>>> driver.
>>>>>
>>>>> As an inspiration look at my aardvark patch which setup separate IRQ
>>>>> domain for PME, AER and HP interrupts:
>>>>> https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/
>>>>>
>>>>> Thanks to custom driver map_irq function, it is not needed to describe
>>>>> root port with separate msi-parent in DTS.
>>>>
>>>> I need to understand your solution, though, this might be the same situation as my driver.
>>>
>>> I think it is very very similar as aardvark also returns zero as hw irq
>>> number (and it is not possible to change it).
>>>
>>> So simple solution for you is also to register separate IRQ domain for
>>> Root Port Bridge and then re-trigger interrupt with number 0 (which you
>>> wrote that is default) as:
>>>
>>>       virq = irq_find_mapping(priv->irq_domain, 0);
>>>       generic_handle_irq(virq);
>>>
>>> in your uniphier_pcie_misc_isr() function.
>>
>> I'm not sure "register separate IRQ domain for Root Port Bridge".
>> Do you mean that your suggestion is to create new IRQ domain, and add this domain to root port?
> 
> Yes.
> 
>> Or could you show me something example?
> 
> I have already sent link to patch above which it implements for
> pci-aardvark.c driver.
> 
> https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/

Thank you for the example.

> In device prove callback register domain by irq_domain_add_linear().
> In bridge map_irq() callback use irq_create_mapping() for Root Port
> device (and otherwise default of_irq_parse_and_map_pci()). And in
> uniphier_pcie_misc_isr() retrigger interrupt into new domain.

I understand it late.
The main point is to replace bridge->map_irq() with private own map_irq().

> 
>> The re-trigger part is the same method as v5 patch I wrote.
> 
> Just you need to specify that new/private IRQ domain into
> irq_find_mapping() call.

I'll try to replace the events with new IRQ domain.

Thank you,

---
Best Regards
Kunihiko Hayashi

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

[Kunihiko Hayashi]

Hi Lorenzo, Pali,

On 2021/07/23 18:36, Kunihiko Hayashi wrote:
> Hi Pali,

[snip]

>> Just you need to specify that new/private IRQ domain into
>> irq_find_mapping() call.
> 
> I'll try to replace the events with new IRQ domain.
According to Pali's suggestion, the bridge handles INTX and it isn't difficult
to change IRQ's map for Root Port like the example.
It seems that it can't be applied to MSI.

On the other hand, according to Lorenzo's suggestion,

 >>>>>>> IMO this should be modelled with a separate IRQ domain and chip for
 >>>>>>> the root port (yes this implies describing the root port in the dts
 >>>>>>> file with a separate msi-parent).

Interrupts for PME/AER event is assigned to number 0 of MSI IRQ domain.
(pcie_port_enable_irq_vec() in portdrv_core.c)
This expects MSI status bit 0 to be set when the event occurs.

However, in the uniphier PCIe controller, MSI status bit 0 is not set, but
the PME/AER status bit in the glue logic is set.

I think that it's hard to associate the new domain and "MSI-IRQ 0" event
if the new IRQ domain and chip is modelled.
So, I have no idea to handle both new IRQ domain and cascaded MSI event.
Is there any example for that?

Thank you,

---
Best Regards
Kunihiko Hayashi

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

[Pali Rohár]

On Wednesday 28 July 2021 14:29:15 Kunihiko Hayashi wrote:
> Hi Lorenzo, Pali,
> 
> On 2021/07/23 18:36, Kunihiko Hayashi wrote:
> > Hi Pali,
> 
> [snip]
> 
> > > Just you need to specify that new/private IRQ domain into
> > > irq_find_mapping() call.
> > 
> > I'll try to replace the events with new IRQ domain.
> According to Pali's suggestion, the bridge handles INTX and it isn't difficult
> to change IRQ's map for Root Port like the example.
> It seems that it can't be applied to MSI.

Hm... And it is hard to change mapping also for MSI via custom/new
callback?

> On the other hand, according to Lorenzo's suggestion,
> 
> >>>>>>> IMO this should be modelled with a separate IRQ domain and chip for
> >>>>>>> the root port (yes this implies describing the root port in the dts
> >>>>>>> file with a separate msi-parent).
> 
> Interrupts for PME/AER event is assigned to number 0 of MSI IRQ domain.

Yes. This is because Root Port of your PCIe controller provides this
information to OS.

> (pcie_port_enable_irq_vec() in portdrv_core.c)
> This expects MSI status bit 0 to be set when the event occurs.

Obviously (according to PCIe spec).

> However, in the uniphier PCIe controller, MSI status bit 0 is not set, but
> the PME/AER status bit in the glue logic is set.

And this "violates" PCIe spec and your controller needs custom handling
in driver to work...

> I think that it's hard to associate the new domain and "MSI-IRQ 0" event
> if the new IRQ domain and chip is modelled.

No. It was mean to assign all MSI IRQs (not only MSI number 0) which
comes from Root Port to new domain. Assigning just one MSI number to
separate domain is I guess impossible (and also does not make sense).

This is required to difference between MSI number 0 which comes from
real MSI path and "fake MSI number 0" which is just specific for Root
Port and does not share anything with real MSI interrupts. As MSI
interrupts are not shared it is required to prevent "mixing" interrupt
sources. And kernel can do it via different MSI domains.

So in the end you would have two different MSI numbers 0.

> So, I have no idea to handle both new IRQ domain and cascaded MSI event.

It was mean to define Root Port PCIe device in DTS. Then define a new
IRQ chip / domain in DTS. And specify in DTS that this Root Port PCIe
device should use this new IRQ chip / domain instead of default one.
And then you need to implement "driver" for this "virtual" IRQ chip /
domain to handle specific glue logic in this controller driver.

I was hoping that it is possible to set this mapping directly in
controller driver. But if you checked that only legacy IRQs can be done
in this way, and not MSI then it is either needed to go via this DTS
path OR try to figure out how to define this mapping in PCI subsystem
(maybe needs some changes?) also for MSI.

> Is there any example for that?

I do not know. I think you are the first one who have such buggy PCIe
controller which needs this specific kind of configuration and domains.

In my case in pci-aardvark.c, emulated kernel Root Port does not support
MSI interrupts (yet) so these "fake" interrupts are routed as legacy
INTA. And because legacy INTx are shared interrupts they can be mixed
together with real (as it is done prior my patch). My patch (link sent
in previous email) just separates "fake" INTA from "real" INTA via
separate domains for performance reasons.

But you use MSI interrupts, which means that it is required to have
logic to separate them into different domains to prevent mixing.

> Thank you,
> 
> ---
> Best Regards
> Kunihiko Hayashi