[PATCH v3 0/6] PCI: uniphier: Add features for UniPhier PCIe host controller

[PATCH v3 0/6] PCI: uniphier: Add features for UniPhier PCIe host controller

[Kunihiko Hayashi]

This series adds some features for UniPhier PCIe host controller.

- Add support for PME and AER invoked by MSI interrupt
- Add iATU register view support for PCIe version >= 4.80
- Add an error message when failing to get phy driver

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

Changes since 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 (6):
  PCI: dwc: Add msi_host_isr() callback
  PCI: uniphier: Add misc interrupt handler to invoke PME and AER
  dt-bindings: PCI: uniphier: Add iATU register description
  PCI: uniphier: Add iATU register support
  PCI: uniphier: Add error message when failed to get phy
  PCI: uniphier: Use devm_platform_ioremap_resource_byname()

 .../devicetree/bindings/pci/uniphier-pcie.txt      |  1 +
 drivers/pci/controller/dwc/pcie-designware-host.c  |  8 +--
 drivers/pci/controller/dwc/pcie-designware.h       |  1 +
 drivers/pci/controller/dwc/pcie-uniphier.c         | 69 +++++++++++++++++-----
 4 files changed, 60 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH v3 1/6] 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>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++----
 drivers/pci/controller/dwc/pcie-designware.h      | 1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 0a4a5aa..9b628a2 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -112,13 +112,13 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 static void dw_chained_msi_isr(struct irq_desc *desc)
 {
 	struct irq_chip *chip = irq_desc_get_chip(desc);
-	struct pcie_port *pp;
+	struct pcie_port *pp = irq_desc_get_handler_data(desc);
 
-	chained_irq_enter(chip, desc);
+	if (pp->ops->msi_host_isr)
+		pp->ops->msi_host_isr(pp);
 
-	pp = irq_desc_get_handler_data(desc);
+	chained_irq_enter(chip, desc);
 	dw_handle_msi_irq(pp);
-
 	chained_irq_exit(chip, desc);
 }
 
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 656e00f..e741967 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -170,6 +170,7 @@ struct dw_pcie_host_ops {
 	void (*scan_bus)(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 v3 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER

[Kunihiko Hayashi]

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

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

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

Cc: Marc Zyngier <maz@kernel.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/pci/controller/dwc/pcie-uniphier.c | 53 +++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index a5401a0..a8dda39 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -44,7 +44,9 @@
 #define PCL_SYS_AUX_PWR_DET		BIT(8)
 
 #define PCL_RCV_INT			0x8108
+#define PCL_RCV_INT_ALL_INT_MASK	GENMASK(28, 25)
 #define PCL_RCV_INT_ALL_ENABLE		GENMASK(20, 17)
+#define PCL_RCV_INT_ALL_MSI_MASK	GENMASK(12, 9)
 #define PCL_CFG_BW_MGT_STATUS		BIT(4)
 #define PCL_CFG_LINK_AUTO_BW_STATUS	BIT(3)
 #define PCL_CFG_AER_RC_ERR_MSI_STATUS	BIT(2)
@@ -167,7 +169,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci)
 
 static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv)
 {
-	writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT);
+	u32 val;
+
+	val = PCL_RCV_INT_ALL_ENABLE;
+	if (pci_msi_enabled())
+		val |= PCL_RCV_INT_ALL_INT_MASK;
+	else
+		val |= PCL_RCV_INT_ALL_MSI_MASK;
+
+	writel(val, priv->base + PCL_RCV_INT);
 	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
 }
 
@@ -231,28 +241,48 @@ 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)
 {
-	struct pcie_port *pp = irq_desc_get_handler_data(desc);
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	unsigned long reg;
-	u32 val, bit, virq;
+	u32 val, virq;
 
-	/* INT for debug */
 	val = readl(priv->base + PCL_RCV_INT);
 
 	if (val & PCL_CFG_BW_MGT_STATUS)
 		dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
+
 	if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
 		dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
-	if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
-		dev_dbg(pci->dev, "Root Error\n");
-	if (val & PCL_CFG_PME_MSI_STATUS)
-		dev_dbg(pci->dev, "PME Interrupt\n");
+
+	if (pci_msi_enabled()) {
+		if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) {
+			dev_dbg(pci->dev, "Root Error Status\n");
+			virq = irq_linear_revmap(pp->irq_domain, 0);
+			generic_handle_irq(virq);
+		}
+
+		if (val & PCL_CFG_PME_MSI_STATUS) {
+			dev_dbg(pci->dev, "PME Interrupt\n");
+			virq = irq_linear_revmap(pp->irq_domain, 0);
+			generic_handle_irq(virq);
+		}
+	}
 
 	writel(val, priv->base + PCL_RCV_INT);
+}
+
+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;
+
+	/* misc interrupt */
+	uniphier_pcie_misc_isr(pp);
 
 	/* INTx */
 	chained_irq_enter(chip, desc);
@@ -330,6 +360,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_misc_isr,
 };
 
 static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,
-- 
2.7.4

[PATCH v3 3/6] dt-bindings: PCI: uniphier: Add iATU register description

[Kunihiko Hayashi]

In the dt-bindings, "atu" reg-names is required to get the register space
for iATU in Synopsys DWC version 4.80 or later.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/uniphier-pcie.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pci/uniphier-pcie.txt b/Documentation/devicetree/bindings/pci/uniphier-pcie.txt
index 1fa2c59..c4b7381 100644
--- a/Documentation/devicetree/bindings/pci/uniphier-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/uniphier-pcie.txt
@@ -16,6 +16,7 @@ Required properties:
     "dbi"    - controller configuration registers
     "link"   - SoC-specific glue layer registers
     "config" - PCIe configuration space
+    "atu"    - iATU registers for DWC version 4.80 or later
 - clocks: A phandle to the clock gate for PCIe glue layer including
 	the host controller.
 - resets: A phandle to the reset line for PCIe glue layer including
-- 
2.7.4

[PATCH v3 4/6] PCI: uniphier: Add iATU register support

[Kunihiko Hayashi]

This gets iATU register area from reg property. In Synopsys DWC version
4.80 or later, since iATU register area is separated from core register
area, this area is necessary to get from DT independently.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/pci/controller/dwc/pcie-uniphier.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index a8dda39..ad14e67 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -447,6 +447,11 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->pci.dbi_base))
 		return PTR_ERR(priv->pci.dbi_base);
 
+	priv->pci.atu_base =
+		devm_platform_ioremap_resource_byname(pdev, "atu");
+	if (IS_ERR(priv->pci.atu_base))
+		priv->pci.atu_base = NULL;
+
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "link");
 	priv->base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(priv->base))
-- 
2.7.4

[PATCH v3 5/6] PCI: uniphier: Add error message when failed to get phy

[Kunihiko Hayashi]

Even if phy driver doesn't probe, the error message can't be distinguished
from other errors. This displays error message caused by the phy driver
explicitly.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/pci/controller/dwc/pcie-uniphier.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index ad14e67..3b51561 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -466,8 +466,12 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->rst);
 
 	priv->phy = devm_phy_optional_get(dev, "pcie-phy");
-	if (IS_ERR(priv->phy))
-		return PTR_ERR(priv->phy);
+	if (IS_ERR(priv->phy)) {
+		ret = PTR_ERR(priv->phy);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get phy (%d)\n", ret);
+		return ret;
+	}
 
 	platform_set_drvdata(pdev, priv);
 
-- 
2.7.4

[PATCH v3 6/6] PCI: uniphier: Use devm_platform_ioremap_resource_byname()

[Kunihiko Hayashi]

Use devm_platform_ioremap_resource_byname() to simplify the code a bit.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/pci/controller/dwc/pcie-uniphier.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index 3b51561..ce47622 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -452,8 +452,7 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->pci.atu_base))
 		priv->pci.atu_base = NULL;
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "link");
-	priv->base = devm_ioremap_resource(dev, res);
+	priv->base = devm_platform_ioremap_resource_byname(pdev, "link");
 	if (IS_ERR(priv->base))
 		return PTR_ERR(priv->base);
 
-- 
2.7.4

Re: [PATCH v3 1/6] PCI: dwc: Add msi_host_isr() callback

[Marc Zyngier]

On 2020-06-03 09:54, Kunihiko Hayashi wrote:
> 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>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++----
>  drivers/pci/controller/dwc/pcie-designware.h      | 1 +
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 0a4a5aa..9b628a2 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -112,13 +112,13 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port 
> *pp)
>  static void dw_chained_msi_isr(struct irq_desc *desc)
>  {
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> -	struct pcie_port *pp;
> +	struct pcie_port *pp = irq_desc_get_handler_data(desc);
> 
> -	chained_irq_enter(chip, desc);
> +	if (pp->ops->msi_host_isr)
> +		pp->ops->msi_host_isr(pp);

Why is this call outside of the enter/exit guards?
Do you still need to execute the standard handler?

> 
> -	pp = irq_desc_get_handler_data(desc);
> +	chained_irq_enter(chip, desc);
>  	dw_handle_msi_irq(pp);
> -
>  	chained_irq_exit(chip, desc);
>  }
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> b/drivers/pci/controller/dwc/pcie-designware.h
> index 656e00f..e741967 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -170,6 +170,7 @@ struct dw_pcie_host_ops {
>  	void (*scan_bus)(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 {

Thanks,

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

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

[Marc Zyngier]

On 2020-06-03 09:54, Kunihiko Hayashi wrote:
> The misc interrupts consisting of PME, AER, and Link event, is handled
> by INTx handler, however, these interrupts should be also handled by
> MSI handler.
> 
> This adds the function uniphier_pcie_misc_isr() that handles misc
> intterupts, which is called from both INTx and MSI handlers.

interrupts

> This function detects PME and AER interrupts with the status register,
> and invoke PME and AER drivers related to INTx or MSI.
> 
> And this sets the mask for misc interrupts from INTx if MSI is enabled
> and sets the mask for misc interrupts from MSI if MSI is disabled.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  drivers/pci/controller/dwc/pcie-uniphier.c | 53 
> +++++++++++++++++++++++-------
>  1 file changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c
> b/drivers/pci/controller/dwc/pcie-uniphier.c
> index a5401a0..a8dda39 100644
> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
> @@ -44,7 +44,9 @@
>  #define PCL_SYS_AUX_PWR_DET		BIT(8)
> 
>  #define PCL_RCV_INT			0x8108
> +#define PCL_RCV_INT_ALL_INT_MASK	GENMASK(28, 25)
>  #define PCL_RCV_INT_ALL_ENABLE		GENMASK(20, 17)
> +#define PCL_RCV_INT_ALL_MSI_MASK	GENMASK(12, 9)
>  #define PCL_CFG_BW_MGT_STATUS		BIT(4)
>  #define PCL_CFG_LINK_AUTO_BW_STATUS	BIT(3)
>  #define PCL_CFG_AER_RC_ERR_MSI_STATUS	BIT(2)
> @@ -167,7 +169,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie 
> *pci)
> 
>  static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv)
>  {
> -	writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT);
> +	u32 val;
> +
> +	val = PCL_RCV_INT_ALL_ENABLE;
> +	if (pci_msi_enabled())
> +		val |= PCL_RCV_INT_ALL_INT_MASK;
> +	else
> +		val |= PCL_RCV_INT_ALL_MSI_MASK;
> +
> +	writel(val, priv->base + PCL_RCV_INT);
>  	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
>  }
> 
> @@ -231,28 +241,48 @@ 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)
>  {
> -	struct pcie_port *pp = irq_desc_get_handler_data(desc);
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> -	struct irq_chip *chip = irq_desc_get_chip(desc);
> -	unsigned long reg;
> -	u32 val, bit, virq;
> +	u32 val, virq;
> 
> -	/* INT for debug */
>  	val = readl(priv->base + PCL_RCV_INT);
> 
>  	if (val & PCL_CFG_BW_MGT_STATUS)
>  		dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
> +
>  	if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
>  		dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
> -	if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
> -		dev_dbg(pci->dev, "Root Error\n");
> -	if (val & PCL_CFG_PME_MSI_STATUS)
> -		dev_dbg(pci->dev, "PME Interrupt\n");
> +
> +	if (pci_msi_enabled()) {

This checks whether the kernel supports MSIs. Not that they are
enabled in your controller. Is that really what you want to do?

> +		if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) {
> +			dev_dbg(pci->dev, "Root Error Status\n");
> +			virq = irq_linear_revmap(pp->irq_domain, 0);
> +			generic_handle_irq(virq);
> +		}
> +
> +		if (val & PCL_CFG_PME_MSI_STATUS) {
> +			dev_dbg(pci->dev, "PME Interrupt\n");
> +			virq = irq_linear_revmap(pp->irq_domain, 0);
> +			generic_handle_irq(virq);
> +		}

These two cases do the exact same thing, calling the same interrupt.
What is the point of dealing with them independently?

> +	}
> 
>  	writel(val, priv->base + PCL_RCV_INT);
> +}
> +
> +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;
> +
> +	/* misc interrupt */
> +	uniphier_pcie_misc_isr(pp);

This is a chained handler called outside of a chained_irq_enter/exit
block. It isn't acceptable.

> 
>  	/* INTx */
>  	chained_irq_enter(chip, desc);
> @@ -330,6 +360,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_misc_isr,
>  };
> 
>  static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,

Thanks,

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

Re: [PATCH v3 1/6] PCI: dwc: Add msi_host_isr() callback

[Kunihiko Hayashi]

Hi Marc,

On 2020/06/03 20:15, Marc Zyngier wrote:
> On 2020-06-03 09:54, Kunihiko Hayashi wrote:
>> 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>
>> ---
>>  drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++----
>>  drivers/pci/controller/dwc/pcie-designware.h      | 1 +
>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
>> b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 0a4a5aa..9b628a2 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -112,13 +112,13 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
>>  static void dw_chained_msi_isr(struct irq_desc *desc)
>>  {
>>      struct irq_chip *chip = irq_desc_get_chip(desc);
>> -    struct pcie_port *pp;
>> +    struct pcie_port *pp = irq_desc_get_handler_data(desc);
>>
>> -    chained_irq_enter(chip, desc);
>> +    if (pp->ops->msi_host_isr)
>> +        pp->ops->msi_host_isr(pp);
> 
> Why is this call outside of the enter/exit guards?
> Do you still need to execute the standard handler?

I assume that the msi_host_isr() contains chained interrupts in
the second patch and no need to treat as the standard handler,
so this should be called in the guards.
I'll move this call to the top of dw_chained_msi_isr().

Thank you,

---
Best Regards
Kunihiko Hayashi

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

[Kunihiko Hayashi]

Hi Marc,

On 2020/06/03 20:22, Marc Zyngier wrote:
> On 2020-06-03 09:54, Kunihiko Hayashi wrote:
>> The misc interrupts consisting of PME, AER, and Link event, is handled
>> by INTx handler, however, these interrupts should be also handled by
>> MSI handler.
>>
>> This adds the function uniphier_pcie_misc_isr() that handles misc
>> intterupts, which is called from both INTx and MSI handlers.
> 
> interrupts

Okay, I'll fix it.

>> This function detects PME and AER interrupts with the status register,
>> and invoke PME and AER drivers related to INTx or MSI.
>>
>> And this sets the mask for misc interrupts from INTx if MSI is enabled
>> and sets the mask for misc interrupts from MSI if MSI is disabled.
>>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: Jingoo Han <jingoohan1@gmail.com>
>> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>> ---
>>  drivers/pci/controller/dwc/pcie-uniphier.c | 53 +++++++++++++++++++++++-------
>>  1 file changed, 42 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c
>> b/drivers/pci/controller/dwc/pcie-uniphier.c
>> index a5401a0..a8dda39 100644
>> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
>> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
>> @@ -44,7 +44,9 @@
>>  #define PCL_SYS_AUX_PWR_DET        BIT(8)
>>
>>  #define PCL_RCV_INT            0x8108
>> +#define PCL_RCV_INT_ALL_INT_MASK    GENMASK(28, 25)
>>  #define PCL_RCV_INT_ALL_ENABLE        GENMASK(20, 17)
>> +#define PCL_RCV_INT_ALL_MSI_MASK    GENMASK(12, 9)
>>  #define PCL_CFG_BW_MGT_STATUS        BIT(4)
>>  #define PCL_CFG_LINK_AUTO_BW_STATUS    BIT(3)
>>  #define PCL_CFG_AER_RC_ERR_MSI_STATUS    BIT(2)
>> @@ -167,7 +169,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci)
>>
>>  static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv)
>>  {
>> -    writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT);
>> +    u32 val;
>> +
>> +    val = PCL_RCV_INT_ALL_ENABLE;
>> +    if (pci_msi_enabled())
>> +        val |= PCL_RCV_INT_ALL_INT_MASK;
>> +    else
>> +        val |= PCL_RCV_INT_ALL_MSI_MASK;
>> +
>> +    writel(val, priv->base + PCL_RCV_INT);
>>      writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
>>  }
>>
>> @@ -231,28 +241,48 @@ 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)
>>  {
>> -    struct pcie_port *pp = irq_desc_get_handler_data(desc);
>>      struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>      struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
>> -    struct irq_chip *chip = irq_desc_get_chip(desc);
>> -    unsigned long reg;
>> -    u32 val, bit, virq;
>> +    u32 val, virq;
>>
>> -    /* INT for debug */
>>      val = readl(priv->base + PCL_RCV_INT);
>>
>>      if (val & PCL_CFG_BW_MGT_STATUS)
>>          dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
>> +
>>      if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
>>          dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
>> -    if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>> -        dev_dbg(pci->dev, "Root Error\n");
>> -    if (val & PCL_CFG_PME_MSI_STATUS)
>> -        dev_dbg(pci->dev, "PME Interrupt\n");
>> +
>> +    if (pci_msi_enabled()) {
> 
> This checks whether the kernel supports MSIs. Not that they are
> enabled in your controller. Is that really what you want to do?

The below two status bits are valid when the interrupt for MSI is asserted.
That is, pci_msi_enabled() is wrong.

I'll modify the function to check the two bits only if this function is
called from MSI handler.

> 
>> +        if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) {
>> +            dev_dbg(pci->dev, "Root Error Status\n");
>> +            virq = irq_linear_revmap(pp->irq_domain, 0);
>> +            generic_handle_irq(virq);
>> +        }
>> +
>> +        if (val & PCL_CFG_PME_MSI_STATUS) {
>> +            dev_dbg(pci->dev, "PME Interrupt\n");
>> +            virq = irq_linear_revmap(pp->irq_domain, 0);
>> +            generic_handle_irq(virq);
>> +        }
> 
> These two cases do the exact same thing, calling the same interrupt.
> What is the point of dealing with them independently?

Both PME and AER are asserted from MSI-0, and each handler checks its own
status bit in the PCIe register (aer_irq() in pcie/aer.c and pcie_pme_irq()
in pcie/pme.c).
So I think this handler calls generic_handle_irq() for the same MSI-0.

> 
>> +    }
>>
>>      writel(val, priv->base + PCL_RCV_INT);
>> +}
>> +
>> +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;
>> +
>> +    /* misc interrupt */
>> +    uniphier_pcie_misc_isr(pp);
> 
> This is a chained handler called outside of a chained_irq_enter/exit
> block. It isn't acceptable.

I got it.
This call should be called in the block.

Thank you,

---
Best Regards
Kunihiko Hayashi

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

[Marc Zyngier]

On 2020-06-04 10:43, Kunihiko Hayashi wrote:

[...]

>>> -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
>>> +static void uniphier_pcie_misc_isr(struct pcie_port *pp)
>>>  {
>>> -    struct pcie_port *pp = irq_desc_get_handler_data(desc);
>>>      struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>      struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
>>> -    struct irq_chip *chip = irq_desc_get_chip(desc);
>>> -    unsigned long reg;
>>> -    u32 val, bit, virq;
>>> +    u32 val, virq;
>>> 
>>> -    /* INT for debug */
>>>      val = readl(priv->base + PCL_RCV_INT);
>>> 
>>>      if (val & PCL_CFG_BW_MGT_STATUS)
>>>          dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
>>> +
>>>      if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
>>>          dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
>>> -    if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>>> -        dev_dbg(pci->dev, "Root Error\n");
>>> -    if (val & PCL_CFG_PME_MSI_STATUS)
>>> -        dev_dbg(pci->dev, "PME Interrupt\n");
>>> +
>>> +    if (pci_msi_enabled()) {
>> 
>> This checks whether the kernel supports MSIs. Not that they are
>> enabled in your controller. Is that really what you want to do?
> 
> The below two status bits are valid when the interrupt for MSI is 
> asserted.
> That is, pci_msi_enabled() is wrong.
> 
> I'll modify the function to check the two bits only if this function is
> called from MSI handler.
> 
>> 
>>> +        if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) {
>>> +            dev_dbg(pci->dev, "Root Error Status\n");
>>> +            virq = irq_linear_revmap(pp->irq_domain, 0);
>>> +            generic_handle_irq(virq);
>>> +        }
>>> +
>>> +        if (val & PCL_CFG_PME_MSI_STATUS) {
>>> +            dev_dbg(pci->dev, "PME Interrupt\n");
>>> +            virq = irq_linear_revmap(pp->irq_domain, 0);
>>> +            generic_handle_irq(virq);
>>> +        }
>> 
>> These two cases do the exact same thing, calling the same interrupt.
>> What is the point of dealing with them independently?
> 
> Both PME and AER are asserted from MSI-0, and each handler checks its 
> own
> status bit in the PCIe register (aer_irq() in pcie/aer.c and 
> pcie_pme_irq()
> in pcie/pme.c).
> So I think this handler calls generic_handle_irq() for the same MSI-0.

So what is wrong with

         if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
                    PCL_CFG_PME_MSI_STATUS)) {
                 // handle interrupt
         }

?

If you have two handlers for the same interrupt, this is a shared
interrupt and each handler will be called in turn.

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

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

[Kunihiko Hayashi]

Hi Marc,

On 2020/06/04 19:11, Marc Zyngier wrote:
> On 2020-06-04 10:43, Kunihiko Hayashi wrote:
> 
> [...]
> 
>>>> -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
>>>> +static void uniphier_pcie_misc_isr(struct pcie_port *pp)
>>>>  {
>>>> -    struct pcie_port *pp = irq_desc_get_handler_data(desc);
>>>>      struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>>      struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
>>>> -    struct irq_chip *chip = irq_desc_get_chip(desc);
>>>> -    unsigned long reg;
>>>> -    u32 val, bit, virq;
>>>> +    u32 val, virq;
>>>>
>>>> -    /* INT for debug */
>>>>      val = readl(priv->base + PCL_RCV_INT);
>>>>
>>>>      if (val & PCL_CFG_BW_MGT_STATUS)
>>>>          dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
>>>> +
>>>>      if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
>>>>          dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
>>>> -    if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>>>> -        dev_dbg(pci->dev, "Root Error\n");
>>>> -    if (val & PCL_CFG_PME_MSI_STATUS)
>>>> -        dev_dbg(pci->dev, "PME Interrupt\n");
>>>> +
>>>> +    if (pci_msi_enabled()) {
>>>
>>> This checks whether the kernel supports MSIs. Not that they are
>>> enabled in your controller. Is that really what you want to do?
>>
>> The below two status bits are valid when the interrupt for MSI is asserted.
>> That is, pci_msi_enabled() is wrong.
>>
>> I'll modify the function to check the two bits only if this function is
>> called from MSI handler.
>>
>>>
>>>> +        if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) {
>>>> +            dev_dbg(pci->dev, "Root Error Status\n");
>>>> +            virq = irq_linear_revmap(pp->irq_domain, 0);
>>>> +            generic_handle_irq(virq);
>>>> +        }
>>>> +
>>>> +        if (val & PCL_CFG_PME_MSI_STATUS) {
>>>> +            dev_dbg(pci->dev, "PME Interrupt\n");
>>>> +            virq = irq_linear_revmap(pp->irq_domain, 0);
>>>> +            generic_handle_irq(virq);
>>>> +        }
>>>
>>> These two cases do the exact same thing, calling the same interrupt.
>>> What is the point of dealing with them independently?
>>
>> Both PME and AER are asserted from MSI-0, and each handler checks its own
>> status bit in the PCIe register (aer_irq() in pcie/aer.c and pcie_pme_irq()
>> in pcie/pme.c).
>> So I think this handler calls generic_handle_irq() for the same MSI-0.
> 
> So what is wrong with
> 
>          if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
>                     PCL_CFG_PME_MSI_STATUS)) {
>                  // handle interrupt
>          }
> 
> ?

No problem.
I'll rewrite it in the same way as yours in handling interrupts.

> If you have two handlers for the same interrupt, this is a shared
> interrupt and each handler will be called in turn.
Yes, MSI-0 is shared with PME and AER, and it will be like that.

Thank you,

---
Best Regards
Kunihiko Hayashi