Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v8 0/3] PCI: uniphier: Add PME/AER support for UniPhier PCIe host controller
@ 2020-10-28  1:31 Kunihiko Hayashi
  2020-10-28  1:31 ` [PATCH v8 1/3] PCI: portdrv: Add pcie_port_service_get_irq() function Kunihiko Hayashi
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Kunihiko Hayashi @ 2020-10-28  1:31 UTC (permalink / raw)
  To: Bjorn Helgaas, Rob Herring, Lorenzo Pieralisi, Jingoo Han,
	Gustavo Pimentel, Marc Zyngier
  Cc: linux-pci, linux-arm-kernel, linux-kernel, Masami Hiramatsu,
	Jassi Brar, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v8 1/3] PCI: portdrv: Add pcie_port_service_get_irq() function
  2020-10-28  1:31 [PATCH v8 0/3] PCI: uniphier: Add PME/AER support for UniPhier PCIe host controller Kunihiko Hayashi
@ 2020-10-28  1:31 ` Kunihiko Hayashi
  2020-11-24 23:25   ` Bjorn Helgaas
  2020-10-28  1:31 ` [PATCH v8 2/3] PCI: dwc: Add msi_host_isr() callback Kunihiko Hayashi
  2020-10-28  1:31 ` [PATCH v8 3/3] PCI: uniphier: Add misc interrupt handler to invoke PME and AER Kunihiko Hayashi
  2 siblings, 1 reply; 8+ messages in thread
From: Kunihiko Hayashi @ 2020-10-28  1:31 UTC (permalink / raw)
  To: Bjorn Helgaas, Rob Herring, Lorenzo Pieralisi, Jingoo Han,
	Gustavo Pimentel, Marc Zyngier
  Cc: linux-pci, linux-arm-kernel, linux-kernel, Masami Hiramatsu,
	Jassi Brar, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v8 2/3] PCI: dwc: Add msi_host_isr() callback
  2020-10-28  1:31 [PATCH v8 0/3] PCI: uniphier: Add PME/AER support for UniPhier PCIe host controller Kunihiko Hayashi
  2020-10-28  1:31 ` [PATCH v8 1/3] PCI: portdrv: Add pcie_port_service_get_irq() function Kunihiko Hayashi
@ 2020-10-28  1:31 ` Kunihiko Hayashi
  2020-10-28  1:31 ` [PATCH v8 3/3] PCI: uniphier: Add misc interrupt handler to invoke PME and AER Kunihiko Hayashi
  2 siblings, 0 replies; 8+ messages in thread
From: Kunihiko Hayashi @ 2020-10-28  1:31 UTC (permalink / raw)
  To: Bjorn Helgaas, Rob Herring, Lorenzo Pieralisi, Jingoo Han,
	Gustavo Pimentel, Marc Zyngier
  Cc: linux-pci, linux-arm-kernel, linux-kernel, Masami Hiramatsu,
	Jassi Brar, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v8 3/3] PCI: uniphier: Add misc interrupt handler to invoke PME and AER
  2020-10-28  1:31 [PATCH v8 0/3] PCI: uniphier: Add PME/AER support for UniPhier PCIe host controller Kunihiko Hayashi
  2020-10-28  1:31 ` [PATCH v8 1/3] PCI: portdrv: Add pcie_port_service_get_irq() function Kunihiko Hayashi
  2020-10-28  1:31 ` [PATCH v8 2/3] PCI: dwc: Add msi_host_isr() callback Kunihiko Hayashi
@ 2020-10-28  1:31 ` Kunihiko Hayashi
  2020-11-24 23:20   ` Bjorn Helgaas
  2 siblings, 1 reply; 8+ messages in thread
From: Kunihiko Hayashi @ 2020-10-28  1:31 UTC (permalink / raw)
  To: Bjorn Helgaas, Rob Herring, Lorenzo Pieralisi, Jingoo Han,
	Gustavo Pimentel, Marc Zyngier
  Cc: linux-pci, linux-arm-kernel, linux-kernel, Masami Hiramatsu,
	Jassi Brar, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v8 3/3] PCI: uniphier: Add misc interrupt handler to invoke PME and AER
  2020-10-28  1:31 ` [PATCH v8 3/3] PCI: uniphier: Add misc interrupt handler to invoke PME and AER Kunihiko Hayashi
@ 2020-11-24 23:20   ` Bjorn Helgaas
  2020-11-25 10:23     ` Lorenzo Pieralisi
  0 siblings, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2020-11-24 23:20 UTC (permalink / raw)
  To: Kunihiko Hayashi
  Cc: Bjorn Helgaas, Rob Herring, Lorenzo Pieralisi, Jingoo Han,
	Gustavo Pimentel, Marc Zyngier, linux-pci, linux-kernel,
	Jassi Brar, Masami Hiramatsu, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v8 1/3] PCI: portdrv: Add pcie_port_service_get_irq() function
  2020-10-28  1:31 ` [PATCH v8 1/3] PCI: portdrv: Add pcie_port_service_get_irq() function Kunihiko Hayashi
@ 2020-11-24 23:25   ` Bjorn Helgaas
  0 siblings, 0 replies; 8+ messages in thread
From: Bjorn Helgaas @ 2020-11-24 23:25 UTC (permalink / raw)
  To: Kunihiko Hayashi
  Cc: Bjorn Helgaas, Rob Herring, Lorenzo Pieralisi, Jingoo Han,
	Gustavo Pimentel, Marc Zyngier, linux-pci, linux-kernel,
	Jassi Brar, Masami Hiramatsu, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v8 3/3] PCI: uniphier: Add misc interrupt handler to invoke PME and AER
  2020-11-24 23:20   ` Bjorn Helgaas
@ 2020-11-25 10:23     ` Lorenzo Pieralisi
  2020-11-27 12:02       ` Kunihiko Hayashi
  0 siblings, 1 reply; 8+ messages in thread
From: Lorenzo Pieralisi @ 2020-11-25 10:23 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Kunihiko Hayashi, Bjorn Helgaas, Rob Herring, Jingoo Han,
	Gustavo Pimentel, Marc Zyngier, linux-pci, linux-kernel,
	Jassi Brar, Masami Hiramatsu, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v8 3/3] PCI: uniphier: Add misc interrupt handler to invoke PME and AER
  2020-11-25 10:23     ` Lorenzo Pieralisi
@ 2020-11-27 12:02       ` Kunihiko Hayashi
  0 siblings, 0 replies; 8+ messages in thread
From: Kunihiko Hayashi @ 2020-11-27 12:02 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Bjorn Helgaas
  Cc: Bjorn Helgaas, Rob Herring, Jingoo Han, Gustavo Pimentel,
	Marc Zyngier, linux-pci, linux-kernel, Jassi Brar,
	Masami Hiramatsu, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-28  1:31 [PATCH v8 0/3] PCI: uniphier: Add PME/AER support for UniPhier PCIe host controller Kunihiko Hayashi
2020-10-28  1:31 ` [PATCH v8 1/3] PCI: portdrv: Add pcie_port_service_get_irq() function Kunihiko Hayashi
2020-11-24 23:25   ` Bjorn Helgaas
2020-10-28  1:31 ` [PATCH v8 2/3] PCI: dwc: Add msi_host_isr() callback Kunihiko Hayashi
2020-10-28  1:31 ` [PATCH v8 3/3] PCI: uniphier: Add misc interrupt handler to invoke PME and AER Kunihiko Hayashi
2020-11-24 23:20   ` Bjorn Helgaas
2020-11-25 10:23     ` Lorenzo Pieralisi
2020-11-27 12:02       ` Kunihiko Hayashi

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git