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

[PATCH v6 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 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 (6):
  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
  dt-bindings: PCI: uniphier: Add iATU register description
  PCI: uniphier: Add iATU register support
  PCI: uniphier: Add error message when failed to get phy

 .../devicetree/bindings/pci/uniphier-pcie.txt      |  1 +
 drivers/pci/controller/dwc/pcie-designware-host.c  |  3 +
 drivers/pci/controller/dwc/pcie-designware.h       |  1 +
 drivers/pci/controller/dwc/pcie-uniphier.c         | 90 ++++++++++++++++++----
 drivers/pci/pcie/portdrv.h                         |  1 +
 drivers/pci/pcie/portdrv_core.c                    | 16 ++++
 6 files changed, 99 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH v6 1/6] 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>
---
 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

[PATCH v6 2/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>
Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 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 9dafecb..7948bf1 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -83,6 +83,9 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 	u32 status, num_ctrls;
 	irqreturn_t ret = IRQ_NONE;
 
+	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 f911760..401cbd9 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 v6 3/6] 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>
---
 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 3a7f403..55a7166 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);
@@ -330,6 +367,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,
@@ -338,6 +376,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;
@@ -354,6 +393,22 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,
 		return ret;
 	}
 
+	/* irq for PME */
+	list_for_each_entry(pcidev, &pp->root_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->root_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

[PATCH v6 4/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 v6 5/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 55a7166..93ef608 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -471,6 +471,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;
+
 	priv->base = devm_platform_ioremap_resource_byname(pdev, "link");
 	if (IS_ERR(priv->base))
 		return PTR_ERR(priv->base);
-- 
2.7.4

[PATCH v6 6/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 93ef608..7c8721e 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -489,8 +489,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

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

[Rob Herring]

On Fri, Aug 7, 2020 at 4:25 AM Kunihiko Hayashi
<hayashi.kunihiko@socionext.com> wrote:
>
> 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 93ef608..7c8721e 100644
> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
> @@ -489,8 +489,12 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
>                 return PTR_ERR(priv->rst);
>
>         priv->phy = devm_phy_optional_get(dev, "pcie-phy");

The point of the optional variant vs. devm_phy_get() is whether or not
you get an error message. So shouldn't you switch to devm_phy_get
instead?

> -       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
>

Re: [PATCH v6 5/6] PCI: uniphier: Add iATU register support

[Rob Herring]

On Fri, Aug 7, 2020 at 4:25 AM Kunihiko Hayashi
<hayashi.kunihiko@socionext.com> wrote:
>
> 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 55a7166..93ef608 100644
> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
> @@ -471,6 +471,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;

Keystone has the same 'atu' resource setup. Please move its code to
the DW core and use that.

Rob

Re: [PATCH v6 5/6] PCI: uniphier: Add iATU register support

[Kunihiko Hayashi]

On 2020/08/18 1:48, Rob Herring wrote:
> On Fri, Aug 7, 2020 at 4:25 AM Kunihiko Hayashi
> <hayashi.kunihiko@socionext.com> wrote:
>>
>> 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 55a7166..93ef608 100644
>> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
>> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
>> @@ -471,6 +471,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;
> 
> Keystone has the same 'atu' resource setup. Please move its code to
> the DW core and use that.

There are some platforms that pci.atu_base is set by other way.
The 'atu' code shouldn't be conflicted with the following existing code.

   drivers/pci/controller/dwc/pci-keystone.c:              atu_base = devm_platform_ioremap_resource_byname(pdev, "atu");
   drivers/pci/controller/dwc/pci-keystone.c:              pci->atu_base = atu_base;
   drivers/pci/controller/dwc/pcie-designware.c:                   pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
   drivers/pci/controller/dwc/pcie-intel-gw.c:     pci->atu_base = pci->dbi_base + data->pcie_atu_offset;
   drivers/pci/controller/dwc/pcie-tegra194.c:     pci->atu_base = devm_ioremap_resource(dev, atu_dma_res);

So I'm not sure where to move the code in the DW core.
Is there any idea?

Thank you,

---
Best Regards
Kunihiko Hayashi

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

[Kunihiko Hayashi]

On 2020/08/18 1:39, Rob Herring wrote:
> On Fri, Aug 7, 2020 at 4:25 AM Kunihiko Hayashi
> <hayashi.kunihiko@socionext.com> wrote:
>>
>> 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 93ef608..7c8721e 100644
>> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
>> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
>> @@ -489,8 +489,12 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
>>                  return PTR_ERR(priv->rst);
>>
>>          priv->phy = devm_phy_optional_get(dev, "pcie-phy");
> 
> The point of the optional variant vs. devm_phy_get() is whether or not
> you get an error message. So shouldn't you switch to devm_phy_get
> instead?
> 
>> -       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;
>> +       }

The 'phys' property is optional, so if there isn't 'phys' in the PCIe node,
devm_phy_get() returns -ENODEV, and devm_phy_optional_get() returns NULL.

When devm_phy_optional_get() replaces devm_phy_get(),
condition for displaying an error message changes to:

    (ret != -EPROBE_DEFER && ret != -ENODEV)

This won't be simple, but should it be replaced?

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH v6 5/6] PCI: uniphier: Add iATU register support

[Rob Herring]

On Fri, Aug 21, 2020 at 1:05 AM Kunihiko Hayashi
<hayashi.kunihiko@socionext.com> wrote:
>
> On 2020/08/18 1:48, Rob Herring wrote:
> > On Fri, Aug 7, 2020 at 4:25 AM Kunihiko Hayashi
> > <hayashi.kunihiko@socionext.com> wrote:
> >>
> >> 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 55a7166..93ef608 100644
> >> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> >> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
> >> @@ -471,6 +471,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;
> >
> > Keystone has the same 'atu' resource setup. Please move its code to
> > the DW core and use that.
>
> There are some platforms that pci.atu_base is set by other way.
> The 'atu' code shouldn't be conflicted with the following existing code.

No, it's not a conflict but needless duplication.

>    drivers/pci/controller/dwc/pci-keystone.c:              atu_base = devm_platform_ioremap_resource_byname(pdev, "atu");
>    drivers/pci/controller/dwc/pci-keystone.c:              pci->atu_base = atu_base;
>    drivers/pci/controller/dwc/pcie-designware.c:                   pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>    drivers/pci/controller/dwc/pcie-intel-gw.c:     pci->atu_base = pci->dbi_base + data->pcie_atu_offset;

This one should have had an 'atu' region in DT.

>    drivers/pci/controller/dwc/pcie-tegra194.c:     pci->atu_base = devm_ioremap_resource(dev, atu_dma_res);

Unfortunately, a different name was used. That is the mess which is
the DW PCI controller.

>
> So I'm not sure where to move the code in the DW core.
> Is there any idea?

You just need this and then remove the keystone code:

diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index b723e0cc41fb..680084467447 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -556,6 +556,8 @@ void dw_pcie_setup(struct dw_pcie *pci)
                                       dw_pcie_iatu_unroll_enabled(pci))) {
                pci->iatu_unroll_enabled = true;
                if (!pci->atu_base)
+                       pci->atu_base =
devm_platform_ioremap_resource_byname(pdev, "atu");
+               if (IS_ERR(pci->atu_base))
                        pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
        }
        dev_dbg(pci->dev, "iATU unroll: %s\n", pci->iatu_unroll_enabled ?

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

[Rob Herring]

On Fri, Aug 21, 2020 at 1:05 AM Kunihiko Hayashi
<hayashi.kunihiko@socionext.com> wrote:
>
> On 2020/08/18 1:39, Rob Herring wrote:
> > On Fri, Aug 7, 2020 at 4:25 AM Kunihiko Hayashi
> > <hayashi.kunihiko@socionext.com> wrote:
> >>
> >> 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 93ef608..7c8721e 100644
> >> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> >> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
> >> @@ -489,8 +489,12 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
> >>                  return PTR_ERR(priv->rst);
> >>
> >>          priv->phy = devm_phy_optional_get(dev, "pcie-phy");
> >
> > The point of the optional variant vs. devm_phy_get() is whether or not
> > you get an error message. So shouldn't you switch to devm_phy_get
> > instead?
> >
> >> -       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;
> >> +       }
>
> The 'phys' property is optional, so if there isn't 'phys' in the PCIe node,
> devm_phy_get() returns -ENODEV, and devm_phy_optional_get() returns NULL.
>
> When devm_phy_optional_get() replaces devm_phy_get(),
> condition for displaying an error message changes to:
>
>     (ret != -EPROBE_DEFER && ret != -ENODEV)
>
> This won't be simple, but should it be replaced?

Nevermind. I was thinking we had some error prints for the optional
vs. non-optional variants.

Rob

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

[Kunihiko Hayashi]

Hi Rob,

On 2020/09/04 7:25, Rob Herring wrote:
> On Fri, Aug 21, 2020 at 1:05 AM Kunihiko Hayashi
> <hayashi.kunihiko@socionext.com> wrote:
>>
>> On 2020/08/18 1:39, Rob Herring wrote:
>>> On Fri, Aug 7, 2020 at 4:25 AM Kunihiko Hayashi
>>> <hayashi.kunihiko@socionext.com> wrote:
>>>>
>>>> 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 93ef608..7c8721e 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
>>>> @@ -489,8 +489,12 @@ static int uniphier_pcie_probe(struct platform_device *pdev)
>>>>                   return PTR_ERR(priv->rst);
>>>>
>>>>           priv->phy = devm_phy_optional_get(dev, "pcie-phy");
>>>
>>> The point of the optional variant vs. devm_phy_get() is whether or not
>>> you get an error message. So shouldn't you switch to devm_phy_get
>>> instead?
>>>
>>>> -       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;
>>>> +       }
>>
>> The 'phys' property is optional, so if there isn't 'phys' in the PCIe node,
>> devm_phy_get() returns -ENODEV, and devm_phy_optional_get() returns NULL.
>>
>> When devm_phy_optional_get() replaces devm_phy_get(),
>> condition for displaying an error message changes to:
>>
>>      (ret != -EPROBE_DEFER && ret != -ENODEV)
>>
>> This won't be simple, but should it be replaced?
> 
> Nevermind. I was thinking we had some error prints for the optional
> vs. non-optional variants.
I understand.
As long as this phy is "optional", this doesn't need to print error message.
Once I cancel this patch, and leave the phy as "optional".

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH v6 5/6] PCI: uniphier: Add iATU register support

[Kunihiko Hayashi]

Hi Rob,

On 2020/09/04 7:12, Rob Herring wrote:
> On Fri, Aug 21, 2020 at 1:05 AM Kunihiko Hayashi
> <hayashi.kunihiko@socionext.com> wrote:
>>
>> On 2020/08/18 1:48, Rob Herring wrote:
>>> On Fri, Aug 7, 2020 at 4:25 AM Kunihiko Hayashi
>>> <hayashi.kunihiko@socionext.com> wrote:
>>>>
>>>> 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 55a7166..93ef608 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
>>>> @@ -471,6 +471,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;
>>>
>>> Keystone has the same 'atu' resource setup. Please move its code to
>>> the DW core and use that.
>>
>> There are some platforms that pci.atu_base is set by other way.
>> The 'atu' code shouldn't be conflicted with the following existing code.
> 
> No, it's not a conflict but needless duplication.

I see.

>>     drivers/pci/controller/dwc/pci-keystone.c:              atu_base = devm_platform_ioremap_resource_byname(pdev, "atu");
>>     drivers/pci/controller/dwc/pci-keystone.c:              pci->atu_base = atu_base;
>>     drivers/pci/controller/dwc/pcie-designware.c:                   pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>>     drivers/pci/controller/dwc/pcie-intel-gw.c:     pci->atu_base = pci->dbi_base + data->pcie_atu_offset;
> 
> This one should have had an 'atu' region in DT.
> 
>>     drivers/pci/controller/dwc/pcie-tegra194.c:     pci->atu_base = devm_ioremap_resource(dev, atu_dma_res);
> 
> Unfortunately, a different name was used. That is the mess which is
> the DW PCI controller.

Okay, this has already set atu_base, so ignore it for this patch.
  
>>
>> So I'm not sure where to move the code in the DW core.
>> Is there any idea?
> 
> You just need this and then remove the keystone code:
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> b/drivers/pci/controller/dwc/pcie-designware.c
> index b723e0cc41fb..680084467447 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -556,6 +556,8 @@ void dw_pcie_setup(struct dw_pcie *pci)
>                                         dw_pcie_iatu_unroll_enabled(pci))) {
>                  pci->iatu_unroll_enabled = true;
>                  if (!pci->atu_base)
> +                       pci->atu_base =
> devm_platform_ioremap_resource_byname(pdev, "atu");
> +               if (IS_ERR(pci->atu_base))
>                          pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>          }
>          dev_dbg(pci->dev, "iATU unroll: %s\n", pci->iatu_unroll_enabled ?
> 

I've got it. I think I need to add a bit of code to this though,
I'll try to apply this and remove the duplicate code.

I'll separate this from other PER/AER patches and send new series.

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH v6 2/6] PCI: dwc: Add msi_host_isr() callback

[Rob Herring]

On Fri, 07 Aug 2020 19:25:18 +0900, 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>
> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++
>  drivers/pci/controller/dwc/pcie-designware.h      | 1 +
>  2 files changed, 4 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

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

[Rob Herring]

On Fri, 07 Aug 2020 19:25:22 +0900, Kunihiko Hayashi wrote:
> 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(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>