[PATCH 0/2] PCI: keembay: Add support for Intel Keem Bay

[PATCH 0/2] PCI: keembay: Add support for Intel Keem Bay

[Wan Ahmad Zainie]

Hi.

The first patch is to document DT bindings for Keem Bay PCIe controller
for both Root Complex and Endpoint modes.

The second patch is the driver file, a glue driver. Keem Bay PCIe
controller is based on DesignWare PCIe IP.

The patch was tested with Keem Bay evaluation module board, with A0
stepping.

Thank you.

Best regards,
Zainie


Wan Ahmad Zainie (2):
  dt-bindings: PCI: Add Intel Keem Bay PCIe controller
  PCI: keembay: Add support for Intel Keem Bay

 .../bindings/pci/intel,keembay-pcie-ep.yaml   |  86 +++
 .../bindings/pci/intel,keembay-pcie.yaml      | 120 ++++
 drivers/pci/controller/dwc/Kconfig            |  24 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pcie-keembay.c     | 658 ++++++++++++++++++
 5 files changed, 889 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
 create mode 100644 drivers/pci/controller/dwc/pcie-keembay.c

-- 
2.17.1

[PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller

[Wan Ahmad Zainie]

Document DT bindings for PCIe controller found on Intel Keem Bay SoC.

Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
---
 .../bindings/pci/intel,keembay-pcie-ep.yaml   |  86 +++++++++++++
 .../bindings/pci/intel,keembay-pcie.yaml      | 120 ++++++++++++++++++
 2 files changed, 206 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml

diff --git a/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
new file mode 100644
index 000000000000..11962c205744
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
@@ -0,0 +1,86 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie-ep.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Intel Keem Bay PCIe EP controller
+
+maintainers:
+  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
+
+properties:
+  compatible:
+      const: intel,keembay-pcie-ep
+
+  reg:
+    items:
+      - description: DesignWare PCIe registers
+      - description: PCIe configuration space
+      - description: Keem Bay specific registers
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: addr_space
+      - const: apb
+
+  interrupts:
+    items:
+      - description: PCIe interrupt
+      - description: PCIe event interrupt
+      - description: PCIe error interrupt
+      - description: PCIe memory access interrupt
+
+  interrupt-names:
+    items:
+      - const: intr
+      - const: ev_intr
+      - const: err_intr
+      - const: mem_access_intr
+
+  num-ib-windows:
+    description: Number of inbound address translation windows
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  num-ob-windows:
+    description: Number of outbound address translation windows
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  num-lanes:
+    description: Number of lanes to use.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 1, 2, 4, 8 ]
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-names
+  - num-ib-windows
+  - num-ob-windows
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+  - |
+    pcie-ep@37000000 {
+          compatible = "intel,keembay-pcie-ep";
+          reg = <0x37000000 0x00800000>,
+                <0x36000000 0x01000000>,
+                <0x37800000 0x00000200>;
+          reg-names = "dbi", "addr_space", "apb";
+          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 108 IRQ_TYPE_EDGE_RISING>,
+                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+          interrupt-names = "intr", "ev_intr", "err_intr",
+                       "mem_access_intr";
+          num-ib-windows = <4>;
+          num-ob-windows = <4>;
+          num-lanes = <2>;
+    };
diff --git a/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
new file mode 100644
index 000000000000..49e5d3d35bd4
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
@@ -0,0 +1,120 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Intel Keem Bay PCIe RC controller
+
+maintainers:
+  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+
+properties:
+  compatible:
+      const: intel,keembay-pcie
+
+  device_type:
+    const: pci
+
+  "#address-cells":
+    const: 3
+
+  "#size-cells":
+    const: 2
+
+  ranges:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  reg:
+    items:
+      - description: DesignWare PCIe registers
+      - description: PCIe configuration space
+      - description: Keem Bay specific registers
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: config
+      - const: apb
+
+  clocks:
+    items:
+      - description: bus clock
+      - description: auxiliary clock
+
+  clock-names:
+    items:
+      - const: master
+      - const: aux
+
+  interrupts:
+    items:
+      - description: PCIe interrupt
+      - description: PCIe event interrupt
+      - description: PCIe error interrupt
+
+  interrupt-names:
+    items:
+      - const: intr
+      - const: ev_intr
+      - const: err_intr
+
+  num-lanes:
+    description: Number of lanes to use.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 1, 2, 4, 8 ]
+
+  num-viewport:
+    description: Number of view ports configured in hardware.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 2
+
+required:
+  - compatible
+  - device_type
+  - "#address-cells"
+  - "#size-cells"
+  - reg
+  - reg-names
+  - ranges
+  - clocks
+  - interrupts
+  - interrupt-names
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    #define KEEM_BAY_A53_PCIE
+    #define KEEM_BAY_A53_AUX_PCIE
+    pcie@37000000 {
+          compatible = "intel,keembay-pcie";
+          reg = <0x37000000 0x00800000>,
+                <0x36e00000 0x00200000>,
+                <0x37800000 0x00000200>;
+          reg-names = "dbi", "config", "apb";
+          #address-cells = <3>;
+          #size-cells = <2>;
+          device_type = "pci";
+          ranges = <0x02000000 0 0x36000000 0x36000000 0 0x00e00000>;
+          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
+          interrupt-names = "intr", "ev_intr", "err_intr";
+          clocks = <&scmi_clk KEEM_BAY_A53_PCIE>,
+                   <&scmi_clk KEEM_BAY_A53_AUX_PCIE>;
+          clock-names = "master", "aux";
+          reset-gpios = <&pca2 9 GPIO_ACTIVE_LOW>;
+          num-viewport = <4>;
+          num-lanes = <2>;
+    };
-- 
2.17.1

[PATCH 2/2] PCI: keembay: Add support for Intel Keem Bay

[Wan Ahmad Zainie]

Add driver for Intel Keem Bay SoC PCIe controller. This controller
is based on DesignWare PCIe core.

In root complex mode, only internal reference clock is possible for
Keem Bay A0. For Keem Bay B0, external reference clock can be used
and will be the default configuration. Currently, keembay_pcie_of_data
structure has one member. It will be expanded later to handle this
difference.

Endpoint mode link initialization is handled by the boot firmware.

Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pci/controller/dwc/Kconfig        |  24 +
 drivers/pci/controller/dwc/Makefile       |   1 +
 drivers/pci/controller/dwc/pcie-keembay.c | 658 ++++++++++++++++++++++
 3 files changed, 683 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-keembay.c

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index bc049865f8e0..694a1fcacb73 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -219,6 +219,30 @@ config PCIE_INTEL_GW
 	  The PCIe controller uses the DesignWare core plus Intel-specific
 	  hardware wrappers.
 
+config PCIE_KEEMBAY
+	bool
+
+config PCIE_KEEMBAY_HOST
+	bool "Intel Keem Bay PCIe controller - Host mode"
+	depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)
+	depends on PCI && PCI_MSI_IRQ_DOMAIN
+	select PCIE_DW_HOST
+	select PCIE_KEEMBAY
+	help
+	  Say Y here to enable support for the PCIe controller in Keem Bay
+	  to work in host mode. This uses the DesignWare core.
+
+config PCIE_KEEMBAY_EP
+	bool "Intel Keem Bay PCIe controller - Endpoint mode"
+	depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)
+	depends on PCI && PCI_MSI_IRQ_DOMAIN
+	depends on PCI_ENDPOINT
+	select PCIE_DW_EP
+	select PCIE_KEEMBAY
+	help
+	  Say Y here to enable support for the PCIe controller in Keem Bay
+	  to work in endpoint mode. This uses the DesignWare core.
+
 config PCIE_KIRIN
 	depends on OF && (ARM64 || COMPILE_TEST)
 	bool "HiSilicon Kirin series SoCs PCIe controllers"
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index a751553fa0db..95da2c62c426 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
 obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
 obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
 obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
+obj-$(CONFIG_PCIE_KEEMBAY) += pcie-keembay.o
 obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
 obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
 obj-$(CONFIG_PCI_MESON) += pci-meson.o
diff --git a/drivers/pci/controller/dwc/pcie-keembay.c b/drivers/pci/controller/dwc/pcie-keembay.c
new file mode 100644
index 000000000000..30401ef223ca
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-keembay.c
@@ -0,0 +1,658 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * PCIe controller driver for Intel Keem Bay
+ * Copyright (C) 2020 Intel Corporation
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/init.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+
+#include "pcie-designware.h"
+
+/* PCIE_REGS_APB_SLV Registers */
+#define PCIE_REGS_PCIE_CFG		0x0004
+#define  PCIE_DEVICE_TYPE		BIT(8)
+#define  PCIE_RSTN			BIT(0)
+#define PCIE_REGS_PCIE_APP_CNTRL	0x0008
+#define  APP_LTSSM_ENABLE		BIT(0)
+
+#define PCIE_REGS_PCIE_INTR_ENABLE	0x0018
+#define  LBC_CII_EVENT_EN		BIT(18)
+#define PCIE_REGS_PCIE_INTR_FLAGS	0x001c
+#define PCIE_REGS_PCIE_ERR_INTR_ENABLE	0x0020
+#define  LINK_REQ_RST_EN		BIT(15)
+#define PCIE_REGS_PCIE_ERR_INTR_FLAGS	0x0024
+#define PCIE_REGS_INTERRUPT_ENABLE	0x0028
+#define  MSI_CTRL_INT_EN		BIT(8)
+#define  EDMA_INT_EN			GENMASK(7, 0)
+#define PCIE_REGS_INTERRUPT_STATUS	0x002c
+#define  MSI_CTRL_INT			BIT(8)
+
+#define PCIE_REGS_PCIE_SII_PM_STATE	0x00b0
+#define  SMLH_LINK_UP			BIT(19)
+#define  RDLH_LINK_UP			BIT(8)
+#define PCIE_REGS_PCIE_PHY_CNTL		0x0164
+#define  PHY0_SRAM_BYPASS		BIT(8)
+#define PCIE_REGS_PCIE_PHY_STAT		0x0168
+#define  PHY0_MPLLA_STATE		BIT(1)
+#define PCIE_REGS_LJPLL_STA		0x016c
+#define  LJPLL_LOCK			BIT(0)
+#define PCIE_REGS_LJPLL_CNTRL_0		0x0170
+#define  LJPLL_EN			BIT(29)
+#define  LJPLL_FOUT_EN			GENMASK(24, 21)
+#define PCIE_REGS_LJPLL_CNTRL_2		0x0178
+#define  LJPLL_REF_DIV			GENMASK(17, 12)
+#define  LJPLL_FB_DIV			GENMASK(11, 0)
+#define PCIE_REGS_LJPLL_CNTRL_3		0x017c
+#define  LJPLL_POST_DIV3A		GENMASK(24, 22)
+#define  LJPLL_POST_DIV2A		GENMASK(18, 16)
+
+#define PCIE_REGS_MEM_ACCESS_IRQ_ENABLE	0x0184
+#define  MEM_ACCESS_IRQ_ENABLE		GENMASK(31, 0)
+
+#define PCIE_DBI2_MASK		BIT(20)
+#define PERST_DELAY_US		1000
+#define AUX_CLK_RATE_HZ		24000000
+
+struct keembay_pcie {
+	struct dw_pcie		*pci;
+	void __iomem		*apb_base;
+	enum dw_pcie_device_mode mode;
+
+	int			irq;
+	int			ev_irq;
+	int			err_irq;
+	int			mem_access_irq;
+
+	struct clk		*clk_master;
+	struct clk		*clk_aux;
+	struct gpio_desc	*reset;
+};
+
+struct keembay_pcie_of_data {
+	enum dw_pcie_device_mode mode;
+};
+
+static inline u32 keembay_pcie_readl(struct keembay_pcie *pcie, u32 offset)
+{
+	return readl(pcie->apb_base + offset);
+}
+
+static inline void keembay_pcie_writel(struct keembay_pcie *pcie, u32 offset,
+				       u32 value)
+{
+	writel(value, pcie->apb_base + offset);
+}
+
+static void keembay_ep_reset_assert(struct keembay_pcie *pcie)
+{
+	gpiod_set_value_cansleep(pcie->reset, 1);
+	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
+}
+
+static void keembay_ep_reset_deassert(struct keembay_pcie *pcie)
+{
+	/* Ensure that PERST has been asserted for at least 100ms */
+	msleep(100);
+
+	gpiod_set_value_cansleep(pcie->reset, 0);
+	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
+}
+
+static void keembay_pcie_ltssm_enable(struct keembay_pcie *pcie, bool enable)
+{
+	u32 val;
+
+	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_APP_CNTRL);
+	if (enable)
+		val |= APP_LTSSM_ENABLE;
+	else
+		val &= ~APP_LTSSM_ENABLE;
+	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_APP_CNTRL, val);
+}
+
+static int keembay_pcie_link_up(struct dw_pcie *pci)
+{
+	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
+	u32 val, mask;
+
+	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_SII_PM_STATE);
+	mask = SMLH_LINK_UP | RDLH_LINK_UP;
+
+	return !!((val & mask) == mask);
+}
+
+static int keembay_pcie_establish_link(struct dw_pcie *pci)
+{
+	return 0;
+}
+
+static void keembay_pcie_stop_link(struct dw_pcie *pci)
+{
+	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
+
+	keembay_pcie_ltssm_enable(pcie, false);
+}
+
+static const struct dw_pcie_ops keembay_pcie_ops = {
+	.link_up	= keembay_pcie_link_up,
+	.start_link	= keembay_pcie_establish_link,
+	.stop_link	= keembay_pcie_stop_link,
+};
+
+/*
+ * Initialize the internal PCIe PLL in Host mode.
+ * See the following sections in Keem Bay data book,
+ * (1) 6.4.6.1 PCIe Subsystem Example Initialization,
+ * (2) 6.8 PCIe Low Jitter PLL for Ref Clk Generation.
+ */
+static int keembay_pcie_pll_init(struct keembay_pcie *pcie)
+{
+	struct dw_pcie *pci = pcie->pci;
+	u32 val;
+	int ret;
+
+	val = FIELD_PREP(LJPLL_REF_DIV, 0) | FIELD_PREP(LJPLL_FB_DIV, 0x32);
+	keembay_pcie_writel(pcie, PCIE_REGS_LJPLL_CNTRL_2, val);
+
+	val = FIELD_PREP(LJPLL_POST_DIV3A, 0x2) |
+		FIELD_PREP(LJPLL_POST_DIV2A, 0x2);
+	keembay_pcie_writel(pcie, PCIE_REGS_LJPLL_CNTRL_3, val);
+
+	val = FIELD_PREP(LJPLL_EN, 0x1) | FIELD_PREP(LJPLL_FOUT_EN, 0xc);
+	keembay_pcie_writel(pcie, PCIE_REGS_LJPLL_CNTRL_0, val);
+
+	ret = readl_poll_timeout(pcie->apb_base + PCIE_REGS_LJPLL_STA,
+				 val, val & LJPLL_LOCK, 20,
+				 500 * USEC_PER_MSEC);
+	if (ret)
+		dev_err(pci->dev, "Low jitter PLL is not locked\n");
+
+	return ret;
+}
+
+static irqreturn_t keembay_pcie_irq_handler(int irq, void *arg)
+{
+	struct keembay_pcie *pcie = arg;
+	struct dw_pcie *pci = pcie->pci;
+	struct pcie_port *pp = &pci->pp;
+	u32 val, mask, status;
+
+	val = keembay_pcie_readl(pcie, PCIE_REGS_INTERRUPT_STATUS);
+	mask = keembay_pcie_readl(pcie, PCIE_REGS_INTERRUPT_ENABLE);
+
+	status = val & mask;
+	if (!status)
+		return IRQ_NONE;
+
+	if (status & MSI_CTRL_INT)
+		dw_handle_msi_irq(pp);
+
+	keembay_pcie_writel(pcie, PCIE_REGS_INTERRUPT_STATUS, status);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t keembay_pcie_ev_irq_handler(int irq, void *arg)
+{
+	struct keembay_pcie *pcie = arg;
+	u32 val, mask, status;
+
+	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_INTR_FLAGS);
+	mask = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_INTR_ENABLE);
+
+	status = val & mask;
+	if (!status)
+		return IRQ_NONE;
+
+	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_INTR_FLAGS, status);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t keembay_pcie_err_irq_handler(int irq, void *arg)
+{
+	struct keembay_pcie *pcie = arg;
+	u32 val, mask, status;
+
+	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_ERR_INTR_FLAGS);
+	mask = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_ERR_INTR_ENABLE);
+
+	status = val & mask;
+	if (!status)
+		return IRQ_NONE;
+
+	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_ERR_INTR_FLAGS, status);
+
+	return IRQ_HANDLED;
+}
+
+static int keembay_pcie_setup_irq(struct keembay_pcie *pcie)
+{
+
+	struct dw_pcie *pci = pcie->pci;
+	struct device *dev = pci->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	int ret;
+
+	pcie->irq = platform_get_irq_byname(pdev, "intr");
+	if (pcie->irq < 0)
+		return pcie->irq;
+
+	pcie->ev_irq = platform_get_irq_byname(pdev, "ev_intr");
+	if (pcie->ev_irq < 0)
+		return pcie->ev_irq;
+
+	pcie->err_irq = platform_get_irq_byname(pdev, "err_intr");
+	if (pcie->err_irq < 0)
+		return pcie->err_irq;
+
+	if (pcie->mode == DW_PCIE_EP_TYPE) {
+		int irq;
+
+		irq = platform_get_irq_byname(pdev, "mem_access_intr");
+		if (irq < 0)
+			return irq;
+
+		pcie->mem_access_irq = irq;
+		return 0;
+	}
+
+	ret = devm_request_irq(dev, pcie->irq, keembay_pcie_irq_handler,
+			       IRQF_SHARED | IRQF_NO_THREAD, "pcie-intr", pcie);
+	if (ret) {
+		dev_err(dev, "Failed to request IRQ: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_request_irq(dev, pcie->ev_irq, keembay_pcie_ev_irq_handler,
+			       0, "pcie-ev-intr", pcie);
+	if (ret) {
+		dev_err(dev, "Failed to request event IRQ: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_request_irq(dev, pcie->err_irq, keembay_pcie_err_irq_handler,
+			       0, "pcie-err-intr", pcie);
+	if (ret)
+		dev_err(dev, "Failed to request error IRQ: %d\n", ret);
+
+	return ret;
+}
+
+static int keembay_pcie_rc_establish_link(struct dw_pcie *pci)
+{
+	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
+	u32 val;
+	int ret;
+
+	if (dw_pcie_link_up(pci))
+		return 0;
+
+	keembay_pcie_ltssm_enable(pcie, false);
+
+	ret = readl_poll_timeout(pcie->apb_base + PCIE_REGS_PCIE_PHY_STAT,
+				 val, val & PHY0_MPLLA_STATE, 20,
+				 500 * USEC_PER_MSEC);
+	if (ret) {
+		dev_err(pci->dev, "MPLLA is not locked\n");
+		return ret;
+	}
+
+	keembay_pcie_ltssm_enable(pcie, true);
+
+	return dw_pcie_wait_for_link(pci);
+}
+
+static void keembay_pcie_enable_interrupts(struct keembay_pcie *pcie)
+{
+	u32 val;
+
+	/* Enable interrupt */
+	val = keembay_pcie_readl(pcie, PCIE_REGS_INTERRUPT_ENABLE);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		val |= MSI_CTRL_INT_EN;
+
+	keembay_pcie_writel(pcie, PCIE_REGS_INTERRUPT_ENABLE, val);
+
+	/* Enable event interrupt */
+	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_INTR_ENABLE);
+	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_INTR_ENABLE, val);
+
+	/* Enable error interrupt */
+	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_ERR_INTR_ENABLE);
+	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_ERR_INTR_ENABLE, val);
+}
+
+static int __init keembay_pcie_host_init(struct pcie_port *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
+	int ret;
+
+	dw_pcie_setup_rc(pp);
+	ret = keembay_pcie_rc_establish_link(pci);
+	if (ret)
+		return ret;
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		dw_pcie_msi_init(pp);
+
+	keembay_pcie_enable_interrupts(pcie);
+
+	/* Disable BARs */
+	dw_pcie_dbi_ro_wr_en(pci);
+	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0 | PCIE_DBI2_MASK, 0);
+	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_1 | PCIE_DBI2_MASK, 0);
+	dw_pcie_dbi_ro_wr_dis(pci);
+
+	return 0;
+}
+
+static const struct dw_pcie_host_ops keembay_pcie_host_ops = {
+	.host_init	= keembay_pcie_host_init,
+};
+
+static void keembay_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
+	u32 val;
+
+	/* Enable DMA completion/error interrupts */
+	val = keembay_pcie_readl(pcie, PCIE_REGS_INTERRUPT_ENABLE);
+	keembay_pcie_writel(pcie, PCIE_REGS_INTERRUPT_ENABLE,
+			    val | EDMA_INT_EN);
+
+	/* Enable CII event interrupt */
+	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_INTR_ENABLE);
+	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_INTR_ENABLE,
+			    val | LBC_CII_EVENT_EN);
+
+	/* Enable link request reset */
+	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_ERR_INTR_ENABLE);
+	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_ERR_INTR_ENABLE,
+			    val | LINK_REQ_RST_EN);
+
+	/* Enable host memory access interrupt */
+	keembay_pcie_writel(pcie, PCIE_REGS_MEM_ACCESS_IRQ_ENABLE,
+			    MEM_ACCESS_IRQ_ENABLE);
+}
+
+static int keembay_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
+				     enum pci_epc_irq_type type,
+				     u16 interrupt_num)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+
+	switch (type) {
+	case PCI_EPC_IRQ_LEGACY:
+		/* Legacy interrupts are not supported in Keem Bay */
+		dev_err(pci->dev, "Unsupported IRQ type\n");
+		return -EINVAL;
+	case PCI_EPC_IRQ_MSI:
+		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
+	case PCI_EPC_IRQ_MSIX:
+		return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num);
+	default:
+		dev_err(pci->dev, "Unknown IRQ type\n");
+		return -EINVAL;
+	}
+}
+
+static const struct pci_epc_features keembay_pcie_epc_features = {
+	.linkup_notifier	= false,
+	.msi_capable		= true,
+	.msix_capable		= true,
+	.reserved_bar		= BIT(BAR_1) | BIT(BAR_3) | BIT(BAR_5),
+	.bar_fixed_64bit	= BIT(BAR_0) | BIT(BAR_2) | BIT(BAR_4),
+	.align			= SZ_16K,
+};
+
+static const struct pci_epc_features *
+keembay_pcie_get_features(struct dw_pcie_ep *ep)
+{
+	return &keembay_pcie_epc_features;
+}
+
+static const struct dw_pcie_ep_ops keembay_pcie_ep_ops = {
+	.ep_init	= keembay_pcie_ep_init,
+	.raise_irq	= keembay_pcie_ep_raise_irq,
+	.get_features	= keembay_pcie_get_features,
+};
+
+static inline struct clk *keembay_pcie_probe_clock(struct device *dev,
+						   const char *id, u64 rate)
+{
+	struct clk *clk;
+	int ret;
+
+	clk = devm_clk_get(dev, id);
+	if (IS_ERR(clk))
+		return clk;
+
+	if (rate) {
+		ret = clk_set_rate(clk, rate);
+		if (ret)
+			return ERR_PTR(ret);
+	}
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = devm_add_action_or_reset(dev,
+				       (void(*)(void *))clk_disable_unprepare,
+				       clk);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return clk;
+}
+
+static int keembay_pcie_probe_clocks(struct keembay_pcie *pcie)
+{
+	struct dw_pcie *pci = pcie->pci;
+	struct device *dev = pci->dev;
+
+	pcie->clk_master = keembay_pcie_probe_clock(dev, "master", 0);
+	if (IS_ERR(pcie->clk_master))
+		return dev_err_probe(dev, PTR_ERR(pcie->clk_master),
+				     "Failed to enable master clock");
+
+	pcie->clk_aux = keembay_pcie_probe_clock(dev, "aux", AUX_CLK_RATE_HZ);
+	if (IS_ERR(pcie->clk_aux))
+		return dev_err_probe(dev, PTR_ERR(pcie->clk_aux),
+				     "Failed to enable auxiliary clock");
+
+	return 0;
+}
+
+static int keembay_pcie_add_pcie_port(struct keembay_pcie *pcie,
+				      struct platform_device *pdev)
+{
+	struct dw_pcie *pci = pcie->pci;
+	struct pcie_port *pp = &pci->pp;
+	struct device *dev = &pdev->dev;
+	u32 val;
+	int ret;
+
+	pcie->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(pcie->reset))
+		return PTR_ERR(pcie->reset);
+
+	ret = keembay_pcie_probe_clocks(pcie);
+	if (ret)
+		return ret;
+
+	/* Set SRAM bypass mode. */
+	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_PHY_CNTL);
+	val |= PHY0_SRAM_BYPASS;
+	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_PHY_CNTL, val);
+
+	/* Set the PCIe controller to be a Root Complex. */
+	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_CFG, PCIE_DEVICE_TYPE);
+
+	ret = keembay_pcie_pll_init(pcie);
+	if (ret)
+		return ret;
+
+	/* Deassert PCIe subsystem power-on-reset. */
+	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_CFG);
+	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_CFG, val | PCIE_RSTN);
+
+	keembay_ep_reset_deassert(pcie);
+
+	ret = keembay_pcie_setup_irq(pcie);
+	if (ret)
+		return ret;
+
+	pp->ops = &keembay_pcie_host_ops;
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		keembay_ep_reset_assert(pcie);
+		dev_err(dev, "Failed to initialize host: %d\n", ret);
+	}
+
+	return ret;
+}
+
+static int keembay_pcie_add_pcie_ep(struct keembay_pcie *pcie,
+				    struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dw_pcie *pci = pcie->pci;
+	struct dw_pcie_ep *ep;
+	struct resource *res;
+	int ret;
+
+	ep = &pci->ep;
+	ep->ops = &keembay_pcie_ep_ops;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
+	if (!res)
+		return -EINVAL;
+
+	ep->phys_base = res->start;
+	ep->addr_size = resource_size(res);
+
+	ret = keembay_pcie_setup_irq(pcie);
+	if (ret)
+		return ret;
+
+	ret = dw_pcie_ep_init(ep);
+	if (ret)
+		dev_err(dev, "Failed to initialize endpoint: %d\n", ret);
+
+	return ret;
+}
+
+static int keembay_pcie_probe(struct platform_device *pdev)
+{
+	const struct keembay_pcie_of_data *data;
+	struct device *dev = &pdev->dev;
+	struct keembay_pcie *pcie;
+	struct dw_pcie *pci;
+	enum dw_pcie_device_mode mode;
+	int ret;
+
+	data = device_get_match_data(dev);
+	if (!data)
+		return -ENODEV;
+
+	mode = (enum dw_pcie_device_mode)data->mode;
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+	if (!pci)
+		return -ENOMEM;
+
+	pci->dev = dev;
+	pci->ops = &keembay_pcie_ops;
+
+	pcie->pci = pci;
+	pcie->mode = mode;
+
+	pcie->apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
+	if (IS_ERR(pcie->apb_base))
+		return PTR_ERR(pcie->apb_base);
+
+	pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
+	if (IS_ERR(pci->dbi_base))
+		return PTR_ERR(pci->dbi_base);
+
+	/* DBI2 shadow register */
+	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_MASK;
+
+	platform_set_drvdata(pdev, pcie);
+
+	switch (pcie->mode) {
+	case DW_PCIE_RC_TYPE:
+		if (!IS_ENABLED(CONFIG_PCIE_KEEMBAY_HOST))
+			return -ENODEV;
+
+		ret = keembay_pcie_add_pcie_port(pcie, pdev);
+		if (ret < 0)
+			return ret;
+		break;
+	case DW_PCIE_EP_TYPE:
+		if (!IS_ENABLED(CONFIG_PCIE_KEEMBAY_EP))
+			return -ENODEV;
+
+		ret = keembay_pcie_add_pcie_ep(pcie, pdev);
+		if (ret < 0)
+			return ret;
+		break;
+	default:
+		dev_err(dev, "Invalid device type %d\n", pcie->mode);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static const struct keembay_pcie_of_data keembay_pcie_rc_of_data = {
+	.mode = DW_PCIE_RC_TYPE,
+};
+
+static const struct keembay_pcie_of_data keembay_pcie_ep_of_data = {
+	.mode = DW_PCIE_EP_TYPE,
+};
+
+static const struct of_device_id keembay_pcie_of_match[] = {
+	{
+		.compatible = "intel,keembay-pcie",
+		.data = &keembay_pcie_rc_of_data,
+	},
+	{
+		.compatible = "intel,keembay-pcie-ep",
+		.data = &keembay_pcie_ep_of_data,
+	},
+	{}
+};
+
+static struct platform_driver keembay_pcie_driver = {
+	.driver = {
+		.name = "keembay-pcie",
+		.of_match_table = keembay_pcie_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe  = keembay_pcie_probe,
+};
+builtin_platform_driver(keembay_pcie_driver);
-- 
2.17.1

Re: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller

[Rob Herring]

On Tue, 27 Oct 2020 14:00:10 +0800, Wan Ahmad Zainie wrote:
> Document DT bindings for PCIe controller found on Intel Keem Bay SoC.
> 
> Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> ---
>  .../bindings/pci/intel,keembay-pcie-ep.yaml   |  86 +++++++++++++
>  .../bindings/pci/intel,keembay-pcie.yaml      | 120 ++++++++++++++++++
>  2 files changed, 206 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
>  create mode 100644 Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml:14:7: [warning] wrong indentation: expected 4 but found 6 (indentation)
./Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml:17:7: [warning] wrong indentation: expected 4 but found 6 (indentation)

dtschema/dtc warnings/errors:


See https://patchwork.ozlabs.org/patch/1388284

The base for the patch is generally the last rc1. Any dependencies
should be noted.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 2/2] PCI: keembay: Add support for Intel Keem Bay

[Rob Herring]

On Tue, Oct 27, 2020 at 02:00:11PM +0800, Wan Ahmad Zainie wrote:
> Add driver for Intel Keem Bay SoC PCIe controller. This controller
> is based on DesignWare PCIe core.
> 
> In root complex mode, only internal reference clock is possible for
> Keem Bay A0. For Keem Bay B0, external reference clock can be used
> and will be the default configuration. Currently, keembay_pcie_of_data
> structure has one member. It will be expanded later to handle this
> difference.
> 
> Endpoint mode link initialization is handled by the boot firmware.
> 
> Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pci/controller/dwc/Kconfig        |  24 +
>  drivers/pci/controller/dwc/Makefile       |   1 +
>  drivers/pci/controller/dwc/pcie-keembay.c | 658 ++++++++++++++++++++++
>  3 files changed, 683 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-keembay.c
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index bc049865f8e0..694a1fcacb73 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -219,6 +219,30 @@ config PCIE_INTEL_GW
>  	  The PCIe controller uses the DesignWare core plus Intel-specific
>  	  hardware wrappers.
>  
> +config PCIE_KEEMBAY
> +	bool
> +
> +config PCIE_KEEMBAY_HOST
> +	bool "Intel Keem Bay PCIe controller - Host mode"
> +	depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)

Why only ARM64 for compile test?

> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
> +	select PCIE_DW_HOST
> +	select PCIE_KEEMBAY
> +	help
> +	  Say Y here to enable support for the PCIe controller in Keem Bay
> +	  to work in host mode. This uses the DesignWare core.
> +
> +config PCIE_KEEMBAY_EP
> +	bool "Intel Keem Bay PCIe controller - Endpoint mode"
> +	depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)
> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
> +	depends on PCI_ENDPOINT
> +	select PCIE_DW_EP
> +	select PCIE_KEEMBAY
> +	help
> +	  Say Y here to enable support for the PCIe controller in Keem Bay
> +	  to work in endpoint mode. This uses the DesignWare core.
> +
>  config PCIE_KIRIN
>  	depends on OF && (ARM64 || COMPILE_TEST)
>  	bool "HiSilicon Kirin series SoCs PCIe controllers"
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index a751553fa0db..95da2c62c426 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
>  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
>  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>  obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
> +obj-$(CONFIG_PCIE_KEEMBAY) += pcie-keembay.o
>  obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
>  obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
>  obj-$(CONFIG_PCI_MESON) += pci-meson.o
> diff --git a/drivers/pci/controller/dwc/pcie-keembay.c b/drivers/pci/controller/dwc/pcie-keembay.c
> new file mode 100644
> index 000000000000..30401ef223ca
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-keembay.c
> @@ -0,0 +1,658 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * PCIe controller driver for Intel Keem Bay
> + * Copyright (C) 2020 Intel Corporation
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/init.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +
> +#include "pcie-designware.h"
> +
> +/* PCIE_REGS_APB_SLV Registers */
> +#define PCIE_REGS_PCIE_CFG		0x0004
> +#define  PCIE_DEVICE_TYPE		BIT(8)
> +#define  PCIE_RSTN			BIT(0)
> +#define PCIE_REGS_PCIE_APP_CNTRL	0x0008
> +#define  APP_LTSSM_ENABLE		BIT(0)
> +
> +#define PCIE_REGS_PCIE_INTR_ENABLE	0x0018
> +#define  LBC_CII_EVENT_EN		BIT(18)
> +#define PCIE_REGS_PCIE_INTR_FLAGS	0x001c
> +#define PCIE_REGS_PCIE_ERR_INTR_ENABLE	0x0020
> +#define  LINK_REQ_RST_EN		BIT(15)
> +#define PCIE_REGS_PCIE_ERR_INTR_FLAGS	0x0024
> +#define PCIE_REGS_INTERRUPT_ENABLE	0x0028
> +#define  MSI_CTRL_INT_EN		BIT(8)
> +#define  EDMA_INT_EN			GENMASK(7, 0)
> +#define PCIE_REGS_INTERRUPT_STATUS	0x002c
> +#define  MSI_CTRL_INT			BIT(8)
> +
> +#define PCIE_REGS_PCIE_SII_PM_STATE	0x00b0
> +#define  SMLH_LINK_UP			BIT(19)
> +#define  RDLH_LINK_UP			BIT(8)
> +#define PCIE_REGS_PCIE_PHY_CNTL		0x0164
> +#define  PHY0_SRAM_BYPASS		BIT(8)
> +#define PCIE_REGS_PCIE_PHY_STAT		0x0168
> +#define  PHY0_MPLLA_STATE		BIT(1)
> +#define PCIE_REGS_LJPLL_STA		0x016c
> +#define  LJPLL_LOCK			BIT(0)
> +#define PCIE_REGS_LJPLL_CNTRL_0		0x0170
> +#define  LJPLL_EN			BIT(29)
> +#define  LJPLL_FOUT_EN			GENMASK(24, 21)
> +#define PCIE_REGS_LJPLL_CNTRL_2		0x0178
> +#define  LJPLL_REF_DIV			GENMASK(17, 12)
> +#define  LJPLL_FB_DIV			GENMASK(11, 0)
> +#define PCIE_REGS_LJPLL_CNTRL_3		0x017c
> +#define  LJPLL_POST_DIV3A		GENMASK(24, 22)
> +#define  LJPLL_POST_DIV2A		GENMASK(18, 16)
> +
> +#define PCIE_REGS_MEM_ACCESS_IRQ_ENABLE	0x0184
> +#define  MEM_ACCESS_IRQ_ENABLE		GENMASK(31, 0)
> +
> +#define PCIE_DBI2_MASK		BIT(20)
> +#define PERST_DELAY_US		1000
> +#define AUX_CLK_RATE_HZ		24000000
> +
> +struct keembay_pcie {
> +	struct dw_pcie		*pci;

Make this a struct, not a pointer. Then one less alloc.

> +	void __iomem		*apb_base;
> +	enum dw_pcie_device_mode mode;
> +
> +	int			irq;
> +	int			ev_irq;
> +	int			err_irq;
> +	int			mem_access_irq;

There's no need to store the irq numbers forever.

> +
> +	struct clk		*clk_master;
> +	struct clk		*clk_aux;
> +	struct gpio_desc	*reset;
> +};
> +
> +struct keembay_pcie_of_data {
> +	enum dw_pcie_device_mode mode;
> +};
> +
> +static inline u32 keembay_pcie_readl(struct keembay_pcie *pcie, u32 offset)
> +{
> +	return readl(pcie->apb_base + offset);
> +}
> +
> +static inline void keembay_pcie_writel(struct keembay_pcie *pcie, u32 offset,
> +				       u32 value)
> +{
> +	writel(value, pcie->apb_base + offset);
> +}
> +
> +static void keembay_ep_reset_assert(struct keembay_pcie *pcie)
> +{
> +	gpiod_set_value_cansleep(pcie->reset, 1);
> +	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> +}
> +
> +static void keembay_ep_reset_deassert(struct keembay_pcie *pcie)
> +{
> +	/* Ensure that PERST has been asserted for at least 100ms */
> +	msleep(100);
> +
> +	gpiod_set_value_cansleep(pcie->reset, 0);
> +	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> +}
> +
> +static void keembay_pcie_ltssm_enable(struct keembay_pcie *pcie, bool enable)
> +{
> +	u32 val;
> +
> +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_APP_CNTRL);
> +	if (enable)
> +		val |= APP_LTSSM_ENABLE;
> +	else
> +		val &= ~APP_LTSSM_ENABLE;
> +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_APP_CNTRL, val);
> +}
> +
> +static int keembay_pcie_link_up(struct dw_pcie *pci)
> +{
> +	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
> +	u32 val, mask;
> +
> +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_SII_PM_STATE);
> +	mask = SMLH_LINK_UP | RDLH_LINK_UP;
> +
> +	return !!((val & mask) == mask);
> +}
> +
> +static int keembay_pcie_establish_link(struct dw_pcie *pci)

_start_link() to align with the ops name.

> +{
> +	return 0;

Shouldn't this call keembay_pcie_ltssm_enable?

> +}
> +
> +static void keembay_pcie_stop_link(struct dw_pcie *pci)
> +{
> +	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
> +
> +	keembay_pcie_ltssm_enable(pcie, false);
> +}
> +
> +static const struct dw_pcie_ops keembay_pcie_ops = {
> +	.link_up	= keembay_pcie_link_up,
> +	.start_link	= keembay_pcie_establish_link,
> +	.stop_link	= keembay_pcie_stop_link,
> +};
> +
> +/*
> + * Initialize the internal PCIe PLL in Host mode.
> + * See the following sections in Keem Bay data book,
> + * (1) 6.4.6.1 PCIe Subsystem Example Initialization,
> + * (2) 6.8 PCIe Low Jitter PLL for Ref Clk Generation.
> + */
> +static int keembay_pcie_pll_init(struct keembay_pcie *pcie)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +	u32 val;
> +	int ret;
> +
> +	val = FIELD_PREP(LJPLL_REF_DIV, 0) | FIELD_PREP(LJPLL_FB_DIV, 0x32);
> +	keembay_pcie_writel(pcie, PCIE_REGS_LJPLL_CNTRL_2, val);
> +
> +	val = FIELD_PREP(LJPLL_POST_DIV3A, 0x2) |
> +		FIELD_PREP(LJPLL_POST_DIV2A, 0x2);
> +	keembay_pcie_writel(pcie, PCIE_REGS_LJPLL_CNTRL_3, val);
> +
> +	val = FIELD_PREP(LJPLL_EN, 0x1) | FIELD_PREP(LJPLL_FOUT_EN, 0xc);
> +	keembay_pcie_writel(pcie, PCIE_REGS_LJPLL_CNTRL_0, val);
> +
> +	ret = readl_poll_timeout(pcie->apb_base + PCIE_REGS_LJPLL_STA,
> +				 val, val & LJPLL_LOCK, 20,
> +				 500 * USEC_PER_MSEC);
> +	if (ret)
> +		dev_err(pci->dev, "Low jitter PLL is not locked\n");
> +
> +	return ret;
> +}
> +
> +static irqreturn_t keembay_pcie_irq_handler(int irq, void *arg)
> +{
> +	struct keembay_pcie *pcie = arg;
> +	struct dw_pcie *pci = pcie->pci;
> +	struct pcie_port *pp = &pci->pp;
> +	u32 val, mask, status;
> +
> +	val = keembay_pcie_readl(pcie, PCIE_REGS_INTERRUPT_STATUS);
> +	mask = keembay_pcie_readl(pcie, PCIE_REGS_INTERRUPT_ENABLE);
> +
> +	status = val & mask;
> +	if (!status)
> +		return IRQ_NONE;
> +
> +	if (status & MSI_CTRL_INT)
> +		dw_handle_msi_irq(pp);
> +
> +	keembay_pcie_writel(pcie, PCIE_REGS_INTERRUPT_STATUS, status);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t keembay_pcie_ev_irq_handler(int irq, void *arg)
> +{
> +	struct keembay_pcie *pcie = arg;
> +	u32 val, mask, status;
> +
> +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_INTR_FLAGS);
> +	mask = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_INTR_ENABLE);
> +
> +	status = val & mask;
> +	if (!status)
> +		return IRQ_NONE;
> +
> +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_INTR_FLAGS, status);

Why an interrupt handler that doesn't do anything?

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t keembay_pcie_err_irq_handler(int irq, void *arg)
> +{
> +	struct keembay_pcie *pcie = arg;
> +	u32 val, mask, status;
> +
> +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_ERR_INTR_FLAGS);
> +	mask = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_ERR_INTR_ENABLE);
> +
> +	status = val & mask;
> +	if (!status)
> +		return IRQ_NONE;
> +
> +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_ERR_INTR_FLAGS, status);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int keembay_pcie_setup_irq(struct keembay_pcie *pcie)
> +{
> +
> +	struct dw_pcie *pci = pcie->pci;
> +	struct device *dev = pci->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	int ret;
> +
> +	pcie->irq = platform_get_irq_byname(pdev, "intr");
> +	if (pcie->irq < 0)
> +		return pcie->irq;
> +
> +	pcie->ev_irq = platform_get_irq_byname(pdev, "ev_intr");
> +	if (pcie->ev_irq < 0)
> +		return pcie->ev_irq;
> +
> +	pcie->err_irq = platform_get_irq_byname(pdev, "err_intr");
> +	if (pcie->err_irq < 0)
> +		return pcie->err_irq;
> +
> +	if (pcie->mode == DW_PCIE_EP_TYPE) {
> +		int irq;
> +
> +		irq = platform_get_irq_byname(pdev, "mem_access_intr");
> +		if (irq < 0)
> +			return irq;
> +
> +		pcie->mem_access_irq = irq;
> +		return 0;
> +	}
> +
> +	ret = devm_request_irq(dev, pcie->irq, keembay_pcie_irq_handler,
> +			       IRQF_SHARED | IRQF_NO_THREAD, "pcie-intr", pcie);
> +	if (ret) {
> +		dev_err(dev, "Failed to request IRQ: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_request_irq(dev, pcie->ev_irq, keembay_pcie_ev_irq_handler,
> +			       0, "pcie-ev-intr", pcie);
> +	if (ret) {
> +		dev_err(dev, "Failed to request event IRQ: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_request_irq(dev, pcie->err_irq, keembay_pcie_err_irq_handler,
> +			       0, "pcie-err-intr", pcie);
> +	if (ret)
> +		dev_err(dev, "Failed to request error IRQ: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int keembay_pcie_rc_establish_link(struct dw_pcie *pci)
> +{
> +	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
> +	u32 val;
> +	int ret;
> +
> +	if (dw_pcie_link_up(pci))
> +		return 0;
> +
> +	keembay_pcie_ltssm_enable(pcie, false);
> +
> +	ret = readl_poll_timeout(pcie->apb_base + PCIE_REGS_PCIE_PHY_STAT,
> +				 val, val & PHY0_MPLLA_STATE, 20,
> +				 500 * USEC_PER_MSEC);
> +	if (ret) {
> +		dev_err(pci->dev, "MPLLA is not locked\n");
> +		return ret;
> +	}
> +
> +	keembay_pcie_ltssm_enable(pcie, true);
> +
> +	return dw_pcie_wait_for_link(pci);
> +}
> +
> +static void keembay_pcie_enable_interrupts(struct keembay_pcie *pcie)
> +{
> +	u32 val;
> +
> +	/* Enable interrupt */
> +	val = keembay_pcie_readl(pcie, PCIE_REGS_INTERRUPT_ENABLE);
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		val |= MSI_CTRL_INT_EN;
> +
> +	keembay_pcie_writel(pcie, PCIE_REGS_INTERRUPT_ENABLE, val);
> +
> +	/* Enable event interrupt */
> +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_INTR_ENABLE);
> +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_INTR_ENABLE, val);
> +
> +	/* Enable error interrupt */
> +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_ERR_INTR_ENABLE);
> +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_ERR_INTR_ENABLE, val);
> +}
> +
> +static int __init keembay_pcie_host_init(struct pcie_port *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
> +	int ret;
> +
> +	dw_pcie_setup_rc(pp);
> +	ret = keembay_pcie_rc_establish_link(pci);
> +	if (ret)
> +		return ret;
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		dw_pcie_msi_init(pp);
> +
> +	keembay_pcie_enable_interrupts(pcie);
> +
> +	/* Disable BARs */

Why? I don't see other DWC drivers doing this.

> +	dw_pcie_dbi_ro_wr_en(pci);
> +	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0 | PCIE_DBI2_MASK, 0);
> +	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_1 | PCIE_DBI2_MASK, 0);
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +
> +	return 0;
> +}
> +
> +static const struct dw_pcie_host_ops keembay_pcie_host_ops = {
> +	.host_init	= keembay_pcie_host_init,
> +};
> +
> +static void keembay_pcie_ep_init(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
> +	u32 val;
> +
> +	/* Enable DMA completion/error interrupts */
> +	val = keembay_pcie_readl(pcie, PCIE_REGS_INTERRUPT_ENABLE);
> +	keembay_pcie_writel(pcie, PCIE_REGS_INTERRUPT_ENABLE,
> +			    val | EDMA_INT_EN);
> +
> +	/* Enable CII event interrupt */
> +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_INTR_ENABLE);
> +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_INTR_ENABLE,
> +			    val | LBC_CII_EVENT_EN);
> +
> +	/* Enable link request reset */
> +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_ERR_INTR_ENABLE);
> +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_ERR_INTR_ENABLE,
> +			    val | LINK_REQ_RST_EN);
> +
> +	/* Enable host memory access interrupt */
> +	keembay_pcie_writel(pcie, PCIE_REGS_MEM_ACCESS_IRQ_ENABLE,
> +			    MEM_ACCESS_IRQ_ENABLE);
> +}
> +
> +static int keembay_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> +				     enum pci_epc_irq_type type,
> +				     u16 interrupt_num)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +
> +	switch (type) {
> +	case PCI_EPC_IRQ_LEGACY:
> +		/* Legacy interrupts are not supported in Keem Bay */
> +		dev_err(pci->dev, "Unsupported IRQ type\n");
> +		return -EINVAL;
> +	case PCI_EPC_IRQ_MSI:
> +		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
> +	case PCI_EPC_IRQ_MSIX:
> +		return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num);
> +	default:
> +		dev_err(pci->dev, "Unknown IRQ type\n");
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct pci_epc_features keembay_pcie_epc_features = {
> +	.linkup_notifier	= false,
> +	.msi_capable		= true,
> +	.msix_capable		= true,
> +	.reserved_bar		= BIT(BAR_1) | BIT(BAR_3) | BIT(BAR_5),
> +	.bar_fixed_64bit	= BIT(BAR_0) | BIT(BAR_2) | BIT(BAR_4),
> +	.align			= SZ_16K,
> +};
> +
> +static const struct pci_epc_features *
> +keembay_pcie_get_features(struct dw_pcie_ep *ep)
> +{
> +	return &keembay_pcie_epc_features;
> +}
> +
> +static const struct dw_pcie_ep_ops keembay_pcie_ep_ops = {
> +	.ep_init	= keembay_pcie_ep_init,
> +	.raise_irq	= keembay_pcie_ep_raise_irq,
> +	.get_features	= keembay_pcie_get_features,
> +};
> +
> +static inline struct clk *keembay_pcie_probe_clock(struct device *dev,
> +						   const char *id, u64 rate)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	clk = devm_clk_get(dev, id);
> +	if (IS_ERR(clk))
> +		return clk;
> +
> +	if (rate) {
> +		ret = clk_set_rate(clk, rate);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = devm_add_action_or_reset(dev,
> +				       (void(*)(void *))clk_disable_unprepare,
> +				       clk);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return clk;
> +}
> +
> +static int keembay_pcie_probe_clocks(struct keembay_pcie *pcie)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +	struct device *dev = pci->dev;
> +
> +	pcie->clk_master = keembay_pcie_probe_clock(dev, "master", 0);
> +	if (IS_ERR(pcie->clk_master))
> +		return dev_err_probe(dev, PTR_ERR(pcie->clk_master),
> +				     "Failed to enable master clock");
> +
> +	pcie->clk_aux = keembay_pcie_probe_clock(dev, "aux", AUX_CLK_RATE_HZ);
> +	if (IS_ERR(pcie->clk_aux))
> +		return dev_err_probe(dev, PTR_ERR(pcie->clk_aux),
> +				     "Failed to enable auxiliary clock");
> +
> +	return 0;
> +}
> +
> +static int keembay_pcie_add_pcie_port(struct keembay_pcie *pcie,
> +				      struct platform_device *pdev)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +	struct pcie_port *pp = &pci->pp;
> +	struct device *dev = &pdev->dev;
> +	u32 val;
> +	int ret;
> +
> +	pcie->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(pcie->reset))
> +		return PTR_ERR(pcie->reset);
> +
> +	ret = keembay_pcie_probe_clocks(pcie);
> +	if (ret)
> +		return ret;
> +
> +	/* Set SRAM bypass mode. */
> +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_PHY_CNTL);
> +	val |= PHY0_SRAM_BYPASS;
> +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_PHY_CNTL, val);
> +
> +	/* Set the PCIe controller to be a Root Complex. */
> +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_CFG, PCIE_DEVICE_TYPE);
> +
> +	ret = keembay_pcie_pll_init(pcie);
> +	if (ret)
> +		return ret;
> +
> +	/* Deassert PCIe subsystem power-on-reset. */
> +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_CFG);
> +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_CFG, val | PCIE_RSTN);

Doesn't EP mode need to do all this setup too?

> +
> +	keembay_ep_reset_deassert(pcie);
> +
> +	ret = keembay_pcie_setup_irq(pcie);
> +	if (ret)
> +		return ret;
> +
> +	pp->ops = &keembay_pcie_host_ops;
> +
> +	ret = dw_pcie_host_init(pp);
> +	if (ret) {
> +		keembay_ep_reset_assert(pcie);
> +		dev_err(dev, "Failed to initialize host: %d\n", ret);
> +	}
> +
> +	return ret;
> +}
> +
> +static int keembay_pcie_add_pcie_ep(struct keembay_pcie *pcie,
> +				    struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct dw_pcie *pci = pcie->pci;
> +	struct dw_pcie_ep *ep;
> +	struct resource *res;
> +	int ret;
> +
> +	ep = &pci->ep;
> +	ep->ops = &keembay_pcie_ep_ops;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> +	if (!res)
> +		return -EINVAL;
> +
> +	ep->phys_base = res->start;
> +	ep->addr_size = resource_size(res);
> +
> +	ret = keembay_pcie_setup_irq(pcie);

Both RC and EP mode call this. Should be done from a common spot 
(probe?).

> +	if (ret)
> +		return ret;
> +
> +	ret = dw_pcie_ep_init(ep);
> +	if (ret)
> +		dev_err(dev, "Failed to initialize endpoint: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int keembay_pcie_probe(struct platform_device *pdev)
> +{
> +	const struct keembay_pcie_of_data *data;
> +	struct device *dev = &pdev->dev;
> +	struct keembay_pcie *pcie;
> +	struct dw_pcie *pci;
> +	enum dw_pcie_device_mode mode;
> +	int ret;
> +
> +	data = device_get_match_data(dev);
> +	if (!data)
> +		return -ENODEV;
> +
> +	mode = (enum dw_pcie_device_mode)data->mode;
> +
> +	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> +	if (!pcie)
> +		return -ENOMEM;
> +
> +	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> +	if (!pci)
> +		return -ENOMEM;
> +
> +	pci->dev = dev;
> +	pci->ops = &keembay_pcie_ops;
> +
> +	pcie->pci = pci;
> +	pcie->mode = mode;
> +
> +	pcie->apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
> +	if (IS_ERR(pcie->apb_base))
> +		return PTR_ERR(pcie->apb_base);
> +
> +	pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
> +	if (IS_ERR(pci->dbi_base))
> +		return PTR_ERR(pci->dbi_base);
> +
> +	/* DBI2 shadow register */
> +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_MASK;

This should be a 'dbi2' reg entry in DT.

FYI, I'm working on moving that and 'dbi' setup into the core code.

> +
> +	platform_set_drvdata(pdev, pcie);
> +
> +	switch (pcie->mode) {
> +	case DW_PCIE_RC_TYPE:
> +		if (!IS_ENABLED(CONFIG_PCIE_KEEMBAY_HOST))
> +			return -ENODEV;
> +
> +		ret = keembay_pcie_add_pcie_port(pcie, pdev);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case DW_PCIE_EP_TYPE:
> +		if (!IS_ENABLED(CONFIG_PCIE_KEEMBAY_EP))
> +			return -ENODEV;
> +
> +		ret = keembay_pcie_add_pcie_ep(pcie, pdev);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	default:
> +		dev_err(dev, "Invalid device type %d\n", pcie->mode);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct keembay_pcie_of_data keembay_pcie_rc_of_data = {
> +	.mode = DW_PCIE_RC_TYPE,
> +};
> +
> +static const struct keembay_pcie_of_data keembay_pcie_ep_of_data = {
> +	.mode = DW_PCIE_EP_TYPE,
> +};
> +
> +static const struct of_device_id keembay_pcie_of_match[] = {
> +	{
> +		.compatible = "intel,keembay-pcie",
> +		.data = &keembay_pcie_rc_of_data,
> +	},
> +	{
> +		.compatible = "intel,keembay-pcie-ep",
> +		.data = &keembay_pcie_ep_of_data,
> +	},
> +	{}
> +};
> +
> +static struct platform_driver keembay_pcie_driver = {
> +	.driver = {
> +		.name = "keembay-pcie",
> +		.of_match_table = keembay_pcie_of_match,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe  = keembay_pcie_probe,
> +};
> +builtin_platform_driver(keembay_pcie_driver);
> -- 
> 2.17.1
> 

Re: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller

[Rob Herring]

On Tue, Oct 27, 2020 at 02:00:10PM +0800, Wan Ahmad Zainie wrote:
> Document DT bindings for PCIe controller found on Intel Keem Bay SoC.
> 
> Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> ---
>  .../bindings/pci/intel,keembay-pcie-ep.yaml   |  86 +++++++++++++
>  .../bindings/pci/intel,keembay-pcie.yaml      | 120 ++++++++++++++++++
>  2 files changed, 206 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
>  create mode 100644 Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> new file mode 100644
> index 000000000000..11962c205744
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> @@ -0,0 +1,86 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie-ep.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Intel Keem Bay PCIe EP controller
> +
> +maintainers:
> +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> +
> +properties:
> +  compatible:
> +      const: intel,keembay-pcie-ep
> +
> +  reg:
> +    items:
> +      - description: DesignWare PCIe registers
> +      - description: PCIe configuration space
> +      - description: Keem Bay specific registers
> +
> +  reg-names:
> +    items:
> +      - const: dbi
> +      - const: addr_space
> +      - const: apb
> +
> +  interrupts:
> +    items:
> +      - description: PCIe interrupt
> +      - description: PCIe event interrupt
> +      - description: PCIe error interrupt
> +      - description: PCIe memory access interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: intr
> +      - const: ev_intr
> +      - const: err_intr
> +      - const: mem_access_intr

'_intr' is redundant. Drop it. You'll need a better name for the first 
one though.

> +
> +  num-ib-windows:
> +    description: Number of inbound address translation windows
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  num-ob-windows:
> +    description: Number of outbound address translation windows
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  num-lanes:
> +    description: Number of lanes to use.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 1, 2, 4, 8 ]
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - interrupt-names
> +  - num-ib-windows
> +  - num-ob-windows
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +  - |
> +    pcie-ep@37000000 {
> +          compatible = "intel,keembay-pcie-ep";
> +          reg = <0x37000000 0x00800000>,
> +                <0x36000000 0x01000000>,
> +                <0x37800000 0x00000200>;
> +          reg-names = "dbi", "addr_space", "apb";
> +          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
> +                       <GIC_SPI 108 IRQ_TYPE_EDGE_RISING>,
> +                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>,
> +                       <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> +          interrupt-names = "intr", "ev_intr", "err_intr",
> +                       "mem_access_intr";
> +          num-ib-windows = <4>;
> +          num-ob-windows = <4>;
> +          num-lanes = <2>;
> +    };
> diff --git a/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> new file mode 100644
> index 000000000000..49e5d3d35bd4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> @@ -0,0 +1,120 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Intel Keem Bay PCIe RC controller
> +
> +maintainers:
> +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +
> +properties:
> +  compatible:
> +      const: intel,keembay-pcie

> +
> +  device_type:
> +    const: pci
> +
> +  "#address-cells":
> +    const: 3
> +
> +  "#size-cells":
> +    const: 2

Can drop these 3 as pci-bus.yaml defines them.

> +
> +  ranges:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  reg:
> +    items:
> +      - description: DesignWare PCIe registers
> +      - description: PCIe configuration space
> +      - description: Keem Bay specific registers
> +
> +  reg-names:
> +    items:
> +      - const: dbi
> +      - const: config
> +      - const: apb
> +
> +  clocks:
> +    items:
> +      - description: bus clock
> +      - description: auxiliary clock

The EP doesn't have clocks? You should have roughly the same resources 
for RC and EP modes.

> +
> +  clock-names:
> +    items:
> +      - const: master
> +      - const: aux
> +
> +  interrupts:
> +    items:
> +      - description: PCIe interrupt
> +      - description: PCIe event interrupt
> +      - description: PCIe error interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: intr
> +      - const: ev_intr
> +      - const: err_intr
> +
> +  num-lanes:
> +    description: Number of lanes to use.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 1, 2, 4, 8 ]
> +
> +  num-viewport:
> +    description: Number of view ports configured in hardware.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 2

Pretty sure it's not 2 if num-ib-windows and num-ob-windows are 4.

> +
> +required:
> +  - compatible

> +  - device_type
> +  - "#address-cells"
> +  - "#size-cells"

Can drop these too.

> +  - reg
> +  - reg-names
> +  - ranges
> +  - clocks
> +  - interrupts
> +  - interrupt-names
> +  - reset-gpios
> +
> +additionalProperties: false

Use 'unevaluatedProperties: false' instead.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    #define KEEM_BAY_A53_PCIE
> +    #define KEEM_BAY_A53_AUX_PCIE
> +    pcie@37000000 {
> +          compatible = "intel,keembay-pcie";
> +          reg = <0x37000000 0x00800000>,
> +                <0x36e00000 0x00200000>,
> +                <0x37800000 0x00000200>;
> +          reg-names = "dbi", "config", "apb";
> +          #address-cells = <3>;
> +          #size-cells = <2>;
> +          device_type = "pci";
> +          ranges = <0x02000000 0 0x36000000 0x36000000 0 0x00e00000>;
> +          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
> +                       <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>,
> +                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
> +          interrupt-names = "intr", "ev_intr", "err_intr";
> +          clocks = <&scmi_clk KEEM_BAY_A53_PCIE>,
> +                   <&scmi_clk KEEM_BAY_A53_AUX_PCIE>;
> +          clock-names = "master", "aux";
> +          reset-gpios = <&pca2 9 GPIO_ACTIVE_LOW>;
> +          num-viewport = <4>;
> +          num-lanes = <2>;
> +    };
> -- 
> 2.17.1
> 

Re: [PATCH 2/2] PCI: keembay: Add support for Intel Keem Bay

[Andy Shevchenko]

On Wed, Oct 28, 2020 at 09:22:47AM -0500, Rob Herring wrote:
> On Tue, Oct 27, 2020 at 02:00:11PM +0800, Wan Ahmad Zainie wrote:

...

> > +config PCIE_KEEMBAY_HOST
> > +	bool "Intel Keem Bay PCIe controller - Host mode"
> > +	depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)
> 
> Why only ARM64 for compile test?

It's just for consistency with other Keem Bay drivers which may or may not use
ARM64 specific call. Since DWC3 core PCI driver is used on many platforms and
is being tested on other architectures, I think it's fine. This glue driver is
for certain platform. But what I'm wondering now is if the existing glue code
for some other platform can be reused here.

> > +	depends on PCI && PCI_MSI_IRQ_DOMAIN
> > +	select PCIE_DW_HOST
> > +	select PCIE_KEEMBAY


-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller

[Wan Mohamad, Wan Ahmad Zainie]

Hi Rob.

Thanks for the review.

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Wednesday, October 28, 2020 10:42 PM
> To: Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>
> Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-
> pci@vger.kernel.org; devicetree@vger.kernel.org;
> andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Raja
> Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>
> Subject: Re: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller
> 
> On Tue, Oct 27, 2020 at 02:00:10PM +0800, Wan Ahmad Zainie wrote:
> > Document DT bindings for PCIe controller found on Intel Keem Bay SoC.
> >
> > Signed-off-by: Wan Ahmad Zainie
> > <wan.ahmad.zainie.wan.mohamad@intel.com>
> > ---
> >  .../bindings/pci/intel,keembay-pcie-ep.yaml   |  86 +++++++++++++
> >  .../bindings/pci/intel,keembay-pcie.yaml      | 120 ++++++++++++++++++
> >  2 files changed, 206 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> >  create mode 100644
> > Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> > b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> > new file mode 100644
> > index 000000000000..11962c205744
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-
> ep.yaml
> > @@ -0,0 +1,86 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie-ep.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Intel Keem Bay PCIe EP controller
> > +
> > +maintainers:
> > +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> > +
> > +properties:
> > +  compatible:
> > +      const: intel,keembay-pcie-ep

Fixed in v2, wrong indentation as per report.

> > +
> > +  reg:
> > +    items:
> > +      - description: DesignWare PCIe registers
> > +      - description: PCIe configuration space
> > +      - description: Keem Bay specific registers
> > +
> > +  reg-names:
> > +    items:
> > +      - const: dbi
> > +      - const: addr_space
> > +      - const: apb
> > +
> > +  interrupts:
> > +    items:
> > +      - description: PCIe interrupt
> > +      - description: PCIe event interrupt
> > +      - description: PCIe error interrupt
> > +      - description: PCIe memory access interrupt
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: intr
> > +      - const: ev_intr
> > +      - const: err_intr
> > +      - const: mem_access_intr
> 
> '_intr' is redundant. Drop it. You'll need a better name for the first one
> though.

I will drop _intr in v2.
I will send out once I get suitable name from Keem Bay data book.

> 
> > +
> > +  num-ib-windows:
> > +    description: Number of inbound address translation windows
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  num-ob-windows:
> > +    description: Number of outbound address translation windows
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  num-lanes:
> > +    description: Number of lanes to use.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [ 1, 2, 4, 8 ]
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - interrupts
> > +  - interrupt-names
> > +  - num-ib-windows
> > +  - num-ob-windows
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +  - |
> > +    pcie-ep@37000000 {
> > +          compatible = "intel,keembay-pcie-ep";
> > +          reg = <0x37000000 0x00800000>,
> > +                <0x36000000 0x01000000>,
> > +                <0x37800000 0x00000200>;
> > +          reg-names = "dbi", "addr_space", "apb";
> > +          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
> > +                       <GIC_SPI 108 IRQ_TYPE_EDGE_RISING>,
> > +                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>,
> > +                       <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> > +          interrupt-names = "intr", "ev_intr", "err_intr",
> > +                       "mem_access_intr";
> > +          num-ib-windows = <4>;
> > +          num-ob-windows = <4>;
> > +          num-lanes = <2>;
> > +    };
> > diff --git
> > a/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > new file mode 100644
> > index 000000000000..49e5d3d35bd4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > @@ -0,0 +1,120 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Intel Keem Bay PCIe RC controller
> > +
> > +maintainers:
> > +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/pci-bus.yaml#
> > +
> > +properties:
> > +  compatible:
> > +      const: intel,keembay-pcie

Wrong indentation as per report.
I will fix in v2.

> 
> > +
> > +  device_type:
> > +    const: pci
> > +
> > +  "#address-cells":
> > +    const: 3
> > +
> > +  "#size-cells":
> > +    const: 2
> 
> Can drop these 3 as pci-bus.yaml defines them.

I will drop these 3 in v2.

> 
> > +
> > +  ranges:
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    maxItems: 1
> > +
> > +  reg:
> > +    items:
> > +      - description: DesignWare PCIe registers
> > +      - description: PCIe configuration space
> > +      - description: Keem Bay specific registers
> > +
> > +  reg-names:
> > +    items:
> > +      - const: dbi
> > +      - const: config
> > +      - const: apb
> > +
> > +  clocks:
> > +    items:
> > +      - description: bus clock
> > +      - description: auxiliary clock
> 
> The EP doesn't have clocks? You should have roughly the same resources for
> RC and EP modes.

For Keem Bay, EP mode link initialization is done in boot firmware.
This include setup the clocks.
That's why I do not include clocks for EP.

> 
> > +
> > +  clock-names:
> > +    items:
> > +      - const: master
> > +      - const: aux
> > +
> > +  interrupts:
> > +    items:
> > +      - description: PCIe interrupt
> > +      - description: PCIe event interrupt
> > +      - description: PCIe error interrupt
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: intr
> > +      - const: ev_intr
> > +      - const: err_intr
> > +
> > +  num-lanes:
> > +    description: Number of lanes to use.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [ 1, 2, 4, 8 ]
> > +
> > +  num-viewport:
> > +    description: Number of view ports configured in hardware.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 2
> 
> Pretty sure it's not 2 if num-ib-windows and num-ob-windows are 4.

As per pcie-designware-host.c, default value is 2, if it is not set.
My example and the DT in my system is 4.
I will fix in v2, by using const: 4.
Should I drop default?

> 
> > +
> > +required:
> > +  - compatible
> 
> > +  - device_type
> > +  - "#address-cells"
> > +  - "#size-cells"
> 
> Can drop these too.

I will drop them in v2.

> 
> > +  - reg
> > +  - reg-names
> > +  - ranges
> > +  - clocks
> > +  - interrupts
> > +  - interrupt-names
> > +  - reset-gpios
> > +
> > +additionalProperties: false
> 
> Use 'unevaluatedProperties: false' instead.

I will fix in v2.

> 
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #define KEEM_BAY_A53_PCIE
> > +    #define KEEM_BAY_A53_AUX_PCIE
> > +    pcie@37000000 {
> > +          compatible = "intel,keembay-pcie";
> > +          reg = <0x37000000 0x00800000>,
> > +                <0x36e00000 0x00200000>,
> > +                <0x37800000 0x00000200>;
> > +          reg-names = "dbi", "config", "apb";
> > +          #address-cells = <3>;
> > +          #size-cells = <2>;
> > +          device_type = "pci";
> > +          ranges = <0x02000000 0 0x36000000 0x36000000 0 0x00e00000>;
> > +          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
> > +                       <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>,
> > +                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
> > +          interrupt-names = "intr", "ev_intr", "err_intr";
> > +          clocks = <&scmi_clk KEEM_BAY_A53_PCIE>,
> > +                   <&scmi_clk KEEM_BAY_A53_AUX_PCIE>;
> > +          clock-names = "master", "aux";
> > +          reset-gpios = <&pca2 9 GPIO_ACTIVE_LOW>;
> > +          num-viewport = <4>;
> > +          num-lanes = <2>;
> > +    };
> > --
> > 2.17.1
> >

Best regards,
Zainie

Re: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller

[Rob Herring]

On Fri, Oct 30, 2020 at 8:05 AM Wan Mohamad, Wan Ahmad Zainie
<wan.ahmad.zainie.wan.mohamad@intel.com> wrote:
>
> Hi Rob.
>
> Thanks for the review.
>
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > Sent: Wednesday, October 28, 2020 10:42 PM
> > To: Wan Mohamad, Wan Ahmad Zainie
> > <wan.ahmad.zainie.wan.mohamad@intel.com>
> > Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-
> > pci@vger.kernel.org; devicetree@vger.kernel.org;
> > andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Raja
> > Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>
> > Subject: Re: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller
> >
> > On Tue, Oct 27, 2020 at 02:00:10PM +0800, Wan Ahmad Zainie wrote:
> > > Document DT bindings for PCIe controller found on Intel Keem Bay SoC.
> > >
> > > Signed-off-by: Wan Ahmad Zainie
> > > <wan.ahmad.zainie.wan.mohamad@intel.com>
> > > ---
> > >  .../bindings/pci/intel,keembay-pcie-ep.yaml   |  86 +++++++++++++
> > >  .../bindings/pci/intel,keembay-pcie.yaml      | 120 ++++++++++++++++++
> > >  2 files changed, 206 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> > >  create mode 100644
> > > Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> > > b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> > > new file mode 100644
> > > index 000000000000..11962c205744
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-
> > ep.yaml
> > > @@ -0,0 +1,86 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie-ep.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: Intel Keem Bay PCIe EP controller
> > > +
> > > +maintainers:
> > > +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +      const: intel,keembay-pcie-ep
>
> Fixed in v2, wrong indentation as per report.
>
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: DesignWare PCIe registers
> > > +      - description: PCIe configuration space
> > > +      - description: Keem Bay specific registers
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: dbi
> > > +      - const: addr_space
> > > +      - const: apb
> > > +
> > > +  interrupts:
> > > +    items:
> > > +      - description: PCIe interrupt
> > > +      - description: PCIe event interrupt
> > > +      - description: PCIe error interrupt
> > > +      - description: PCIe memory access interrupt
> > > +
> > > +  interrupt-names:
> > > +    items:
> > > +      - const: intr
> > > +      - const: ev_intr
> > > +      - const: err_intr
> > > +      - const: mem_access_intr
> >
> > '_intr' is redundant. Drop it. You'll need a better name for the first one
> > though.
>
> I will drop _intr in v2.
> I will send out once I get suitable name from Keem Bay data book.
>
> >
> > > +
> > > +  num-ib-windows:
> > > +    description: Number of inbound address translation windows
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +  num-ob-windows:
> > > +    description: Number of outbound address translation windows
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +  num-lanes:
> > > +    description: Number of lanes to use.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [ 1, 2, 4, 8 ]
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - reg-names
> > > +  - interrupts
> > > +  - interrupt-names
> > > +  - num-ib-windows
> > > +  - num-ob-windows
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +  - |
> > > +    pcie-ep@37000000 {
> > > +          compatible = "intel,keembay-pcie-ep";
> > > +          reg = <0x37000000 0x00800000>,
> > > +                <0x36000000 0x01000000>,
> > > +                <0x37800000 0x00000200>;
> > > +          reg-names = "dbi", "addr_space", "apb";
> > > +          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
> > > +                       <GIC_SPI 108 IRQ_TYPE_EDGE_RISING>,
> > > +                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>,
> > > +                       <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> > > +          interrupt-names = "intr", "ev_intr", "err_intr",
> > > +                       "mem_access_intr";
> > > +          num-ib-windows = <4>;
> > > +          num-ob-windows = <4>;
> > > +          num-lanes = <2>;
> > > +    };
> > > diff --git
> > > a/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > > b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > > new file mode 100644
> > > index 000000000000..49e5d3d35bd4
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > > @@ -0,0 +1,120 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: Intel Keem Bay PCIe RC controller
> > > +
> > > +maintainers:
> > > +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/pci/pci-bus.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +      const: intel,keembay-pcie
>
> Wrong indentation as per report.
> I will fix in v2.
>
> >
> > > +
> > > +  device_type:
> > > +    const: pci
> > > +
> > > +  "#address-cells":
> > > +    const: 3
> > > +
> > > +  "#size-cells":
> > > +    const: 2
> >
> > Can drop these 3 as pci-bus.yaml defines them.
>
> I will drop these 3 in v2.
>
> >
> > > +
> > > +  ranges:
> > > +    maxItems: 1
> > > +
> > > +  reset-gpios:
> > > +    maxItems: 1
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: DesignWare PCIe registers
> > > +      - description: PCIe configuration space
> > > +      - description: Keem Bay specific registers
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: dbi
> > > +      - const: config
> > > +      - const: apb
> > > +
> > > +  clocks:
> > > +    items:
> > > +      - description: bus clock
> > > +      - description: auxiliary clock
> >
> > The EP doesn't have clocks? You should have roughly the same resources for
> > RC and EP modes.
>
> For Keem Bay, EP mode link initialization is done in boot firmware.
> This include setup the clocks.
> That's why I do not include clocks for EP.
>
> >
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: master
> > > +      - const: aux
> > > +
> > > +  interrupts:
> > > +    items:
> > > +      - description: PCIe interrupt
> > > +      - description: PCIe event interrupt
> > > +      - description: PCIe error interrupt
> > > +
> > > +  interrupt-names:
> > > +    items:
> > > +      - const: intr
> > > +      - const: ev_intr
> > > +      - const: err_intr
> > > +
> > > +  num-lanes:
> > > +    description: Number of lanes to use.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [ 1, 2, 4, 8 ]
> > > +
> > > +  num-viewport:
> > > +    description: Number of view ports configured in hardware.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    default: 2
> >
> > Pretty sure it's not 2 if num-ib-windows and num-ob-windows are 4.
>
> As per pcie-designware-host.c, default value is 2, if it is not set.

Yes, that's true.

> My example and the DT in my system is 4.
> I will fix in v2, by using const: 4.
> Should I drop default?

Yes.

BTW, I'm going to make all 3 properties obsolete. I'm working on a
patch to detect all this. It's pretty straight-forward, just see how
many registers are writable. The WIP patch is on my for-kernelci
branch.

The problem with these properties is they are defined as RC and EP
specific, but they are really fixed h/w config independent of the
mode. And num-viewport is incomplete because the inbound and outbound
sizes are independent. The driver just currently doesn't use inbound
windows for RC mode. Also, the driver claims there can be up to 256
windows, but I'm not really sure that's right. There's 2 platforms
upstream (ls1088a and ls208xa) claiming 256 windows in DT, but testing
with the detection code indicates they only have 16 IB and 16 OB
windows. Perhaps if you have the DWC manual you could confirm what's
possible.

Rob

RE: [PATCH 2/2] PCI: keembay: Add support for Intel Keem Bay

[Wan Mohamad, Wan Ahmad Zainie]

Hi Rob.

Thanks for the review.

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Wednesday, October 28, 2020 10:23 PM
> To: Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>
> Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-
> pci@vger.kernel.org; devicetree@vger.kernel.org;
> andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Raja
> Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>
> Subject: Re: [PATCH 2/2] PCI: keembay: Add support for Intel Keem Bay
> 
> On Tue, Oct 27, 2020 at 02:00:11PM +0800, Wan Ahmad Zainie wrote:
> > Add driver for Intel Keem Bay SoC PCIe controller. This controller is
> > based on DesignWare PCIe core.
> >
> > In root complex mode, only internal reference clock is possible for
> > Keem Bay A0. For Keem Bay B0, external reference clock can be used and
> > will be the default configuration. Currently, keembay_pcie_of_data
> > structure has one member. It will be expanded later to handle this
> > difference.
> >
> > Endpoint mode link initialization is handled by the boot firmware.
> >
> > Signed-off-by: Wan Ahmad Zainie
> > <wan.ahmad.zainie.wan.mohamad@intel.com>
> > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/pci/controller/dwc/Kconfig        |  24 +
> >  drivers/pci/controller/dwc/Makefile       |   1 +
> >  drivers/pci/controller/dwc/pcie-keembay.c | 658
> > ++++++++++++++++++++++
> >  3 files changed, 683 insertions(+)
> >  create mode 100644 drivers/pci/controller/dwc/pcie-keembay.c
> >
> > diff --git a/drivers/pci/controller/dwc/Kconfig
> > b/drivers/pci/controller/dwc/Kconfig
> > index bc049865f8e0..694a1fcacb73 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -219,6 +219,30 @@ config PCIE_INTEL_GW
> >  	  The PCIe controller uses the DesignWare core plus Intel-specific
> >  	  hardware wrappers.
> >
> > +config PCIE_KEEMBAY
> > +	bool
> > +
> > +config PCIE_KEEMBAY_HOST
> > +	bool "Intel Keem Bay PCIe controller - Host mode"
> > +	depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)
> 
> Why only ARM64 for compile test?

Andy did response to this.
https://lore.kernel.org/linux-pci/20201028153454.GV4077@smile.fi.intel.com/

I did not receive it my mailbox, so I copy paste here, in case.

> 
> > +	depends on PCI && PCI_MSI_IRQ_DOMAIN
> > +	select PCIE_DW_HOST
> > +	select PCIE_KEEMBAY
> > +	help
> > +	  Say Y here to enable support for the PCIe controller in Keem Bay
> > +	  to work in host mode. This uses the DesignWare core.
> > +
> > +config PCIE_KEEMBAY_EP
> > +	bool "Intel Keem Bay PCIe controller - Endpoint mode"
> > +	depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)
> > +	depends on PCI && PCI_MSI_IRQ_DOMAIN
> > +	depends on PCI_ENDPOINT
> > +	select PCIE_DW_EP
> > +	select PCIE_KEEMBAY
> > +	help
> > +	  Say Y here to enable support for the PCIe controller in Keem Bay
> > +	  to work in endpoint mode. This uses the DesignWare core.
> > +
> >  config PCIE_KIRIN
> >  	depends on OF && (ARM64 || COMPILE_TEST)
> >  	bool "HiSilicon Kirin series SoCs PCIe controllers"
> > diff --git a/drivers/pci/controller/dwc/Makefile
> > b/drivers/pci/controller/dwc/Makefile
> > index a751553fa0db..95da2c62c426 100644
> > --- a/drivers/pci/controller/dwc/Makefile
> > +++ b/drivers/pci/controller/dwc/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> >  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> >  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> >  obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
> > +obj-$(CONFIG_PCIE_KEEMBAY) += pcie-keembay.o
> >  obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
> >  obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
> >  obj-$(CONFIG_PCI_MESON) += pci-meson.o diff --git
> > a/drivers/pci/controller/dwc/pcie-keembay.c
> > b/drivers/pci/controller/dwc/pcie-keembay.c
> > new file mode 100644
> > index 000000000000..30401ef223ca
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-keembay.c
> > @@ -0,0 +1,658 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * PCIe controller driver for Intel Keem Bay
> > + * Copyright (C) 2020 Intel Corporation  */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/init.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/property.h>
> > +
> > +#include "pcie-designware.h"
> > +
> > +/* PCIE_REGS_APB_SLV Registers */
> > +#define PCIE_REGS_PCIE_CFG		0x0004
> > +#define  PCIE_DEVICE_TYPE		BIT(8)
> > +#define  PCIE_RSTN			BIT(0)
> > +#define PCIE_REGS_PCIE_APP_CNTRL	0x0008
> > +#define  APP_LTSSM_ENABLE		BIT(0)
> > +
> > +#define PCIE_REGS_PCIE_INTR_ENABLE	0x0018
> > +#define  LBC_CII_EVENT_EN		BIT(18)
> > +#define PCIE_REGS_PCIE_INTR_FLAGS	0x001c
> > +#define PCIE_REGS_PCIE_ERR_INTR_ENABLE	0x0020
> > +#define  LINK_REQ_RST_EN		BIT(15)
> > +#define PCIE_REGS_PCIE_ERR_INTR_FLAGS	0x0024
> > +#define PCIE_REGS_INTERRUPT_ENABLE	0x0028
> > +#define  MSI_CTRL_INT_EN		BIT(8)
> > +#define  EDMA_INT_EN			GENMASK(7, 0)
> > +#define PCIE_REGS_INTERRUPT_STATUS	0x002c
> > +#define  MSI_CTRL_INT			BIT(8)
> > +
> > +#define PCIE_REGS_PCIE_SII_PM_STATE	0x00b0
> > +#define  SMLH_LINK_UP			BIT(19)
> > +#define  RDLH_LINK_UP			BIT(8)
> > +#define PCIE_REGS_PCIE_PHY_CNTL		0x0164
> > +#define  PHY0_SRAM_BYPASS		BIT(8)
> > +#define PCIE_REGS_PCIE_PHY_STAT		0x0168
> > +#define  PHY0_MPLLA_STATE		BIT(1)
> > +#define PCIE_REGS_LJPLL_STA		0x016c
> > +#define  LJPLL_LOCK			BIT(0)
> > +#define PCIE_REGS_LJPLL_CNTRL_0		0x0170
> > +#define  LJPLL_EN			BIT(29)
> > +#define  LJPLL_FOUT_EN			GENMASK(24, 21)
> > +#define PCIE_REGS_LJPLL_CNTRL_2		0x0178
> > +#define  LJPLL_REF_DIV			GENMASK(17, 12)
> > +#define  LJPLL_FB_DIV			GENMASK(11, 0)
> > +#define PCIE_REGS_LJPLL_CNTRL_3		0x017c
> > +#define  LJPLL_POST_DIV3A		GENMASK(24, 22)
> > +#define  LJPLL_POST_DIV2A		GENMASK(18, 16)
> > +
> > +#define PCIE_REGS_MEM_ACCESS_IRQ_ENABLE	0x0184
> > +#define  MEM_ACCESS_IRQ_ENABLE		GENMASK(31, 0)
> > +
> > +#define PCIE_DBI2_MASK		BIT(20)
> > +#define PERST_DELAY_US		1000
> > +#define AUX_CLK_RATE_HZ		24000000
> > +
> > +struct keembay_pcie {
> > +	struct dw_pcie		*pci;
> 
> Make this a struct, not a pointer. Then one less alloc.

I will fix in v2.

> 
> > +	void __iomem		*apb_base;
> > +	enum dw_pcie_device_mode mode;
> > +
> > +	int			irq;
> > +	int			ev_irq;
> > +	int			err_irq;
> > +	int			mem_access_irq;
> 
> There's no need to store the irq numbers forever.

I will fix this in v2.
Need to modify keembay_pcie_setup_irq().

> 
> > +
> > +	struct clk		*clk_master;
> > +	struct clk		*clk_aux;
> > +	struct gpio_desc	*reset;
> > +};
> > +
> > +struct keembay_pcie_of_data {
> > +	enum dw_pcie_device_mode mode;
> > +};
> > +
> > +static inline u32 keembay_pcie_readl(struct keembay_pcie *pcie, u32
> > +offset) {
> > +	return readl(pcie->apb_base + offset); }
> > +
> > +static inline void keembay_pcie_writel(struct keembay_pcie *pcie, u32
> offset,
> > +				       u32 value)
> > +{
> > +	writel(value, pcie->apb_base + offset); }
> > +
> > +static void keembay_ep_reset_assert(struct keembay_pcie *pcie) {
> > +	gpiod_set_value_cansleep(pcie->reset, 1);
> > +	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500); }
> > +
> > +static void keembay_ep_reset_deassert(struct keembay_pcie *pcie) {
> > +	/* Ensure that PERST has been asserted for at least 100ms */
> > +	msleep(100);
> > +
> > +	gpiod_set_value_cansleep(pcie->reset, 0);
> > +	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500); }
> > +
> > +static void keembay_pcie_ltssm_enable(struct keembay_pcie *pcie, bool
> > +enable) {
> > +	u32 val;
> > +
> > +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_APP_CNTRL);
> > +	if (enable)
> > +		val |= APP_LTSSM_ENABLE;
> > +	else
> > +		val &= ~APP_LTSSM_ENABLE;
> > +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_APP_CNTRL, val); }
> > +
> > +static int keembay_pcie_link_up(struct dw_pcie *pci) {
> > +	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
> > +	u32 val, mask;
> > +
> > +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_SII_PM_STATE);
> > +	mask = SMLH_LINK_UP | RDLH_LINK_UP;
> > +
> > +	return !!((val & mask) == mask);
> > +}
> > +
> > +static int keembay_pcie_establish_link(struct dw_pcie *pci)
> 
> _start_link() to align with the ops name.

I will fix in v2.

> 
> > +{
> > +	return 0;
> 
> Shouldn't this call keembay_pcie_ltssm_enable?

Prior to changes in for-kernelci branch, start_link ops is
used for endpoint mode only. Since Keem Bay endpoint
initialization is done by boot firmware, I override this ops
to return 0.

> 
> > +}
> > +
> > +static void keembay_pcie_stop_link(struct dw_pcie *pci) {
> > +	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
> > +
> > +	keembay_pcie_ltssm_enable(pcie, false); }
> > +
> > +static const struct dw_pcie_ops keembay_pcie_ops = {
> > +	.link_up	= keembay_pcie_link_up,
> > +	.start_link	= keembay_pcie_establish_link,
> > +	.stop_link	= keembay_pcie_stop_link,
> > +};
> > +
> > +/*
> > + * Initialize the internal PCIe PLL in Host mode.
> > + * See the following sections in Keem Bay data book,
> > + * (1) 6.4.6.1 PCIe Subsystem Example Initialization,
> > + * (2) 6.8 PCIe Low Jitter PLL for Ref Clk Generation.
> > + */
> > +static int keembay_pcie_pll_init(struct keembay_pcie *pcie) {
> > +	struct dw_pcie *pci = pcie->pci;
> > +	u32 val;
> > +	int ret;
> > +
> > +	val = FIELD_PREP(LJPLL_REF_DIV, 0) | FIELD_PREP(LJPLL_FB_DIV,
> 0x32);
> > +	keembay_pcie_writel(pcie, PCIE_REGS_LJPLL_CNTRL_2, val);
> > +
> > +	val = FIELD_PREP(LJPLL_POST_DIV3A, 0x2) |
> > +		FIELD_PREP(LJPLL_POST_DIV2A, 0x2);
> > +	keembay_pcie_writel(pcie, PCIE_REGS_LJPLL_CNTRL_3, val);
> > +
> > +	val = FIELD_PREP(LJPLL_EN, 0x1) | FIELD_PREP(LJPLL_FOUT_EN, 0xc);
> > +	keembay_pcie_writel(pcie, PCIE_REGS_LJPLL_CNTRL_0, val);
> > +
> > +	ret = readl_poll_timeout(pcie->apb_base + PCIE_REGS_LJPLL_STA,
> > +				 val, val & LJPLL_LOCK, 20,
> > +				 500 * USEC_PER_MSEC);
> > +	if (ret)
> > +		dev_err(pci->dev, "Low jitter PLL is not locked\n");
> > +
> > +	return ret;
> > +}
> > +
> > +static irqreturn_t keembay_pcie_irq_handler(int irq, void *arg) {
> > +	struct keembay_pcie *pcie = arg;
> > +	struct dw_pcie *pci = pcie->pci;
> > +	struct pcie_port *pp = &pci->pp;
> > +	u32 val, mask, status;
> > +
> > +	val = keembay_pcie_readl(pcie, PCIE_REGS_INTERRUPT_STATUS);
> > +	mask = keembay_pcie_readl(pcie, PCIE_REGS_INTERRUPT_ENABLE);
> > +
> > +	status = val & mask;
> > +	if (!status)
> > +		return IRQ_NONE;
> > +
> > +	if (status & MSI_CTRL_INT)
> > +		dw_handle_msi_irq(pp);
> > +
> > +	keembay_pcie_writel(pcie, PCIE_REGS_INTERRUPT_STATUS, status);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t keembay_pcie_ev_irq_handler(int irq, void *arg) {
> > +	struct keembay_pcie *pcie = arg;
> > +	u32 val, mask, status;
> > +
> > +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_INTR_FLAGS);
> > +	mask = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_INTR_ENABLE);
> > +
> > +	status = val & mask;
> > +	if (!status)
> > +		return IRQ_NONE;
> > +
> > +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_INTR_FLAGS, status);
> 
> Why an interrupt handler that doesn't do anything?

I will fix in v2, remove unused interrupt handler.

> 
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t keembay_pcie_err_irq_handler(int irq, void *arg) {
> > +	struct keembay_pcie *pcie = arg;
> > +	u32 val, mask, status;
> > +
> > +	val = keembay_pcie_readl(pcie,
> PCIE_REGS_PCIE_ERR_INTR_FLAGS);
> > +	mask = keembay_pcie_readl(pcie,
> PCIE_REGS_PCIE_ERR_INTR_ENABLE);
> > +
> > +	status = val & mask;
> > +	if (!status)
> > +		return IRQ_NONE;
> > +
> > +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_ERR_INTR_FLAGS,
> status);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int keembay_pcie_setup_irq(struct keembay_pcie *pcie) {
> > +
> > +	struct dw_pcie *pci = pcie->pci;
> > +	struct device *dev = pci->dev;
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	int ret;
> > +
> > +	pcie->irq = platform_get_irq_byname(pdev, "intr");
> > +	if (pcie->irq < 0)
> > +		return pcie->irq;
> > +
> > +	pcie->ev_irq = platform_get_irq_byname(pdev, "ev_intr");
> > +	if (pcie->ev_irq < 0)
> > +		return pcie->ev_irq;
> > +
> > +	pcie->err_irq = platform_get_irq_byname(pdev, "err_intr");
> > +	if (pcie->err_irq < 0)
> > +		return pcie->err_irq;
> > +
> > +	if (pcie->mode == DW_PCIE_EP_TYPE) {
> > +		int irq;
> > +
> > +		irq = platform_get_irq_byname(pdev, "mem_access_intr");
> > +		if (irq < 0)
> > +			return irq;
> > +
> > +		pcie->mem_access_irq = irq;
> > +		return 0;
> > +	}
> > +
> > +	ret = devm_request_irq(dev, pcie->irq, keembay_pcie_irq_handler,
> > +			       IRQF_SHARED | IRQF_NO_THREAD, "pcie-intr",
> pcie);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to request IRQ: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = devm_request_irq(dev, pcie->ev_irq,
> keembay_pcie_ev_irq_handler,
> > +			       0, "pcie-ev-intr", pcie);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to request event IRQ: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = devm_request_irq(dev, pcie->err_irq,
> keembay_pcie_err_irq_handler,
> > +			       0, "pcie-err-intr", pcie);
> > +	if (ret)
> > +		dev_err(dev, "Failed to request error IRQ: %d\n", ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int keembay_pcie_rc_establish_link(struct dw_pcie *pci) {
> > +	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
> > +	u32 val;
> > +	int ret;
> > +
> > +	if (dw_pcie_link_up(pci))
> > +		return 0;
> > +
> > +	keembay_pcie_ltssm_enable(pcie, false);
> > +
> > +	ret = readl_poll_timeout(pcie->apb_base +
> PCIE_REGS_PCIE_PHY_STAT,
> > +				 val, val & PHY0_MPLLA_STATE, 20,
> > +				 500 * USEC_PER_MSEC);
> > +	if (ret) {
> > +		dev_err(pci->dev, "MPLLA is not locked\n");
> > +		return ret;
> > +	}
> > +
> > +	keembay_pcie_ltssm_enable(pcie, true);
> > +
> > +	return dw_pcie_wait_for_link(pci);
> > +}
> > +
> > +static void keembay_pcie_enable_interrupts(struct keembay_pcie *pcie)
> > +{
> > +	u32 val;
> > +
> > +	/* Enable interrupt */
> > +	val = keembay_pcie_readl(pcie, PCIE_REGS_INTERRUPT_ENABLE);
> > +
> > +	if (IS_ENABLED(CONFIG_PCI_MSI))
> > +		val |= MSI_CTRL_INT_EN;
> > +
> > +	keembay_pcie_writel(pcie, PCIE_REGS_INTERRUPT_ENABLE, val);
> > +
> > +	/* Enable event interrupt */
> > +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_INTR_ENABLE);
> > +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_INTR_ENABLE, val);
> > +
> > +	/* Enable error interrupt */
> > +	val = keembay_pcie_readl(pcie,
> PCIE_REGS_PCIE_ERR_INTR_ENABLE);
> > +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_ERR_INTR_ENABLE,
> val); }
> > +
> > +static int __init keembay_pcie_host_init(struct pcie_port *pp) {
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
> > +	int ret;
> > +
> > +	dw_pcie_setup_rc(pp);
> > +	ret = keembay_pcie_rc_establish_link(pci);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (IS_ENABLED(CONFIG_PCI_MSI))
> > +		dw_pcie_msi_init(pp);
> > +
> > +	keembay_pcie_enable_interrupts(pcie);
> > +
> > +	/* Disable BARs */
> 
> Why? I don't see other DWC drivers doing this.

Looks like it is a Keem Bay specific requirement.

It is specified in Keem Bay data book [1]. I quote Section 6.4.9.3.2,
"As BAR0 is configured to have its incoming requests routed to TRGT0 in EP mode,
then you must disable that BAR (through a DBI write) when operating the controller
in RC mode."

[1] https://cdrdv2.intel.com/v1/dl/getContent/615086

> 
> > +	dw_pcie_dbi_ro_wr_en(pci);
> > +	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0 | PCIE_DBI2_MASK,
> 0);
> > +	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_1 | PCIE_DBI2_MASK,
> 0);
> > +	dw_pcie_dbi_ro_wr_dis(pci);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dw_pcie_host_ops keembay_pcie_host_ops = {
> > +	.host_init	= keembay_pcie_host_init,
> > +};
> > +
> > +static void keembay_pcie_ep_init(struct dw_pcie_ep *ep) {
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
> > +	u32 val;
> > +
> > +	/* Enable DMA completion/error interrupts */
> > +	val = keembay_pcie_readl(pcie, PCIE_REGS_INTERRUPT_ENABLE);
> > +	keembay_pcie_writel(pcie, PCIE_REGS_INTERRUPT_ENABLE,
> > +			    val | EDMA_INT_EN);
> > +
> > +	/* Enable CII event interrupt */
> > +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_INTR_ENABLE);
> > +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_INTR_ENABLE,
> > +			    val | LBC_CII_EVENT_EN);
> > +
> > +	/* Enable link request reset */
> > +	val = keembay_pcie_readl(pcie,
> PCIE_REGS_PCIE_ERR_INTR_ENABLE);
> > +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_ERR_INTR_ENABLE,
> > +			    val | LINK_REQ_RST_EN);
> > +
> > +	/* Enable host memory access interrupt */
> > +	keembay_pcie_writel(pcie, PCIE_REGS_MEM_ACCESS_IRQ_ENABLE,
> > +			    MEM_ACCESS_IRQ_ENABLE);
> > +}
> > +
> > +static int keembay_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8
> func_no,
> > +				     enum pci_epc_irq_type type,
> > +				     u16 interrupt_num)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +
> > +	switch (type) {
> > +	case PCI_EPC_IRQ_LEGACY:
> > +		/* Legacy interrupts are not supported in Keem Bay */
> > +		dev_err(pci->dev, "Unsupported IRQ type\n");
> > +		return -EINVAL;
> > +	case PCI_EPC_IRQ_MSI:
> > +		return dw_pcie_ep_raise_msi_irq(ep, func_no,
> interrupt_num);
> > +	case PCI_EPC_IRQ_MSIX:
> > +		return dw_pcie_ep_raise_msix_irq(ep, func_no,
> interrupt_num);
> > +	default:
> > +		dev_err(pci->dev, "Unknown IRQ type\n");
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct pci_epc_features keembay_pcie_epc_features = {
> > +	.linkup_notifier	= false,
> > +	.msi_capable		= true,
> > +	.msix_capable		= true,
> > +	.reserved_bar		= BIT(BAR_1) | BIT(BAR_3) | BIT(BAR_5),
> > +	.bar_fixed_64bit	= BIT(BAR_0) | BIT(BAR_2) | BIT(BAR_4),
> > +	.align			= SZ_16K,
> > +};
> > +
> > +static const struct pci_epc_features *
> > +keembay_pcie_get_features(struct dw_pcie_ep *ep) {
> > +	return &keembay_pcie_epc_features;
> > +}
> > +
> > +static const struct dw_pcie_ep_ops keembay_pcie_ep_ops = {
> > +	.ep_init	= keembay_pcie_ep_init,
> > +	.raise_irq	= keembay_pcie_ep_raise_irq,
> > +	.get_features	= keembay_pcie_get_features,
> > +};
> > +
> > +static inline struct clk *keembay_pcie_probe_clock(struct device *dev,
> > +						   const char *id, u64 rate)
> > +{
> > +	struct clk *clk;
> > +	int ret;
> > +
> > +	clk = devm_clk_get(dev, id);
> > +	if (IS_ERR(clk))
> > +		return clk;
> > +
> > +	if (rate) {
> > +		ret = clk_set_rate(clk, rate);
> > +		if (ret)
> > +			return ERR_PTR(ret);
> > +	}
> > +
> > +	ret = clk_prepare_enable(clk);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	ret = devm_add_action_or_reset(dev,
> > +				       (void(*)(void *))clk_disable_unprepare,
> > +				       clk);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	return clk;
> > +}
> > +
> > +static int keembay_pcie_probe_clocks(struct keembay_pcie *pcie) {
> > +	struct dw_pcie *pci = pcie->pci;
> > +	struct device *dev = pci->dev;
> > +
> > +	pcie->clk_master = keembay_pcie_probe_clock(dev, "master", 0);
> > +	if (IS_ERR(pcie->clk_master))
> > +		return dev_err_probe(dev, PTR_ERR(pcie->clk_master),
> > +				     "Failed to enable master clock");
> > +
> > +	pcie->clk_aux = keembay_pcie_probe_clock(dev, "aux",
> AUX_CLK_RATE_HZ);
> > +	if (IS_ERR(pcie->clk_aux))
> > +		return dev_err_probe(dev, PTR_ERR(pcie->clk_aux),
> > +				     "Failed to enable auxiliary clock");
> > +
> > +	return 0;
> > +}
> > +
> > +static int keembay_pcie_add_pcie_port(struct keembay_pcie *pcie,
> > +				      struct platform_device *pdev) {
> > +	struct dw_pcie *pci = pcie->pci;
> > +	struct pcie_port *pp = &pci->pp;
> > +	struct device *dev = &pdev->dev;
> > +	u32 val;
> > +	int ret;
> > +
> > +	pcie->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(pcie->reset))
> > +		return PTR_ERR(pcie->reset);
> > +
> > +	ret = keembay_pcie_probe_clocks(pcie);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Set SRAM bypass mode. */
> > +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_PHY_CNTL);
> > +	val |= PHY0_SRAM_BYPASS;
> > +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_PHY_CNTL, val);
> > +
> > +	/* Set the PCIe controller to be a Root Complex. */
> > +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_CFG,
> PCIE_DEVICE_TYPE);
> > +
> > +	ret = keembay_pcie_pll_init(pcie);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Deassert PCIe subsystem power-on-reset. */
> > +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_CFG);
> > +	keembay_pcie_writel(pcie, PCIE_REGS_PCIE_CFG, val | PCIE_RSTN);
> 
> Doesn't EP mode need to do all this setup too?

For Keem Bay, endpoint mode link initialization should be done by
boot firmware.

> 
> > +
> > +	keembay_ep_reset_deassert(pcie);
> > +
> > +	ret = keembay_pcie_setup_irq(pcie);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pp->ops = &keembay_pcie_host_ops;
> > +
> > +	ret = dw_pcie_host_init(pp);
> > +	if (ret) {
> > +		keembay_ep_reset_assert(pcie);
> > +		dev_err(dev, "Failed to initialize host: %d\n", ret);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int keembay_pcie_add_pcie_ep(struct keembay_pcie *pcie,
> > +				    struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct dw_pcie *pci = pcie->pci;
> > +	struct dw_pcie_ep *ep;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	ep = &pci->ep;
> > +	ep->ops = &keembay_pcie_ep_ops;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "addr_space");
> > +	if (!res)
> > +		return -EINVAL;
> > +
> > +	ep->phys_base = res->start;
> > +	ep->addr_size = resource_size(res);
> > +
> > +	ret = keembay_pcie_setup_irq(pcie);
> 
> Both RC and EP mode call this. Should be done from a common spot
> (probe?).

I will fix in v2, call keembay_pcie_setup_irq() from probe().

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = dw_pcie_ep_init(ep);
> > +	if (ret)
> > +		dev_err(dev, "Failed to initialize endpoint: %d\n", ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int keembay_pcie_probe(struct platform_device *pdev) {
> > +	const struct keembay_pcie_of_data *data;
> > +	struct device *dev = &pdev->dev;
> > +	struct keembay_pcie *pcie;
> > +	struct dw_pcie *pci;
> > +	enum dw_pcie_device_mode mode;
> > +	int ret;
> > +
> > +	data = device_get_match_data(dev);
> > +	if (!data)
> > +		return -ENODEV;
> > +
> > +	mode = (enum dw_pcie_device_mode)data->mode;
> > +
> > +	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> > +	if (!pcie)
> > +		return -ENOMEM;
> > +
> > +	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > +	if (!pci)
> > +		return -ENOMEM;
> > +
> > +	pci->dev = dev;
> > +	pci->ops = &keembay_pcie_ops;
> > +
> > +	pcie->pci = pci;
> > +	pcie->mode = mode;
> > +
> > +	pcie->apb_base =
> devm_platform_ioremap_resource_byname(pdev, "apb");
> > +	if (IS_ERR(pcie->apb_base))
> > +		return PTR_ERR(pcie->apb_base);
> > +
> > +	pci->dbi_base = devm_platform_ioremap_resource_byname(pdev,
> "dbi");
> > +	if (IS_ERR(pci->dbi_base))
> > +		return PTR_ERR(pci->dbi_base);
> > +
> > +	/* DBI2 shadow register */
> > +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_MASK;
> 
> This should be a 'dbi2' reg entry in DT.
> 
> FYI, I'm working on moving that and 'dbi' setup into the core code.

Noted. I did take a look into for-kernelci branch.

I need advice on how I should move forward.
Should I rebase my changes to for-kernelci or mainline branch?

> 
> > +
> > +	platform_set_drvdata(pdev, pcie);
> > +
> > +	switch (pcie->mode) {
> > +	case DW_PCIE_RC_TYPE:
> > +		if (!IS_ENABLED(CONFIG_PCIE_KEEMBAY_HOST))
> > +			return -ENODEV;
> > +
> > +		ret = keembay_pcie_add_pcie_port(pcie, pdev);
> > +		if (ret < 0)
> > +			return ret;
> > +		break;
> > +	case DW_PCIE_EP_TYPE:
> > +		if (!IS_ENABLED(CONFIG_PCIE_KEEMBAY_EP))
> > +			return -ENODEV;
> > +
> > +		ret = keembay_pcie_add_pcie_ep(pcie, pdev);
> > +		if (ret < 0)
> > +			return ret;
> > +		break;
> > +	default:
> > +		dev_err(dev, "Invalid device type %d\n", pcie->mode);
> > +		return -ENODEV;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct keembay_pcie_of_data keembay_pcie_rc_of_data = {
> > +	.mode = DW_PCIE_RC_TYPE,
> > +};
> > +
> > +static const struct keembay_pcie_of_data keembay_pcie_ep_of_data = {
> > +	.mode = DW_PCIE_EP_TYPE,
> > +};
> > +
> > +static const struct of_device_id keembay_pcie_of_match[] = {
> > +	{
> > +		.compatible = "intel,keembay-pcie",
> > +		.data = &keembay_pcie_rc_of_data,
> > +	},
> > +	{
> > +		.compatible = "intel,keembay-pcie-ep",
> > +		.data = &keembay_pcie_ep_of_data,
> > +	},
> > +	{}
> > +};
> > +
> > +static struct platform_driver keembay_pcie_driver = {
> > +	.driver = {
> > +		.name = "keembay-pcie",
> > +		.of_match_table = keembay_pcie_of_match,
> > +		.suppress_bind_attrs = true,
> > +	},
> > +	.probe  = keembay_pcie_probe,
> > +};
> > +builtin_platform_driver(keembay_pcie_driver);
> > --
> > 2.17.1
> >

Best regards,
Zainie

RE: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller

[Wan Mohamad, Wan Ahmad Zainie]

Hi Rob.

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Friday, October 30, 2020 10:56 PM
> To: Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>
> Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-
> pci@vger.kernel.org; devicetree@vger.kernel.org;
> andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Raja
> Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>
> Subject: Re: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller
> 
> On Fri, Oct 30, 2020 at 8:05 AM Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com> wrote:
> >
> > Hi Rob.
> >
> > Thanks for the review.
> >
> > > -----Original Message-----
> > > From: Rob Herring <robh@kernel.org>
> > > Sent: Wednesday, October 28, 2020 10:42 PM
> > > To: Wan Mohamad, Wan Ahmad Zainie
> > > <wan.ahmad.zainie.wan.mohamad@intel.com>
> > > Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-
> > > pci@vger.kernel.org; devicetree@vger.kernel.org;
> > > andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Raja
> > > Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>
> > > Subject: Re: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe
> > > controller
> > >
> > > On Tue, Oct 27, 2020 at 02:00:10PM +0800, Wan Ahmad Zainie wrote:

...

> > > > +  num-viewport:
> > > > +    description: Number of view ports configured in hardware.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    default: 2
> > >
> > > Pretty sure it's not 2 if num-ib-windows and num-ob-windows are 4.
> >
> > As per pcie-designware-host.c, default value is 2, if it is not set.
> 
> Yes, that's true.
> 
> > My example and the DT in my system is 4.
> > I will fix in v2, by using const: 4.
> > Should I drop default?
> 
> Yes.
> 
> BTW, I'm going to make all 3 properties obsolete. I'm working on a patch to
> detect all this. It's pretty straight-forward, just see how many registers are
> writable. The WIP patch is on my for-kernelci branch.

I will take note on this.

I also take a look into for-kernelci branch. I will spend some time to try it
out with my patch.

> 
> The problem with these properties is they are defined as RC and EP specific,
> but they are really fixed h/w config independent of the mode. And num-
> viewport is incomplete because the inbound and outbound sizes are
> independent. The driver just currently doesn't use inbound windows for RC
> mode. Also, the driver claims there can be up to 256 windows, but I'm not
> really sure that's right. There's 2 platforms upstream (ls1088a and ls208xa)
> claiming 256 windows in DT, but testing with the detection code indicates
> they only have 16 IB and 16 OB windows. Perhaps if you have the DWC
> manual you could confirm what's possible.

Unfortunately, I don't have details on DWC manual. As for Keem Bay,
from the information in its databook, it is synthesized with 8 IB and 8 OB
windows. The values that I used for DT is based on recommendation from
our boot firmware team.

> 
> Rob

Best regards,
Zainie

Re: [PATCH 2/2] PCI: keembay: Add support for Intel Keem Bay

[Bjorn Helgaas]

On Tue, Oct 27, 2020 at 02:00:11PM +0800, Wan Ahmad Zainie wrote:

> +static int keembay_pcie_link_up(struct dw_pcie *pci)
> +{
> +	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
> +	u32 val, mask;
> +
> +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_SII_PM_STATE);
> +	mask = SMLH_LINK_UP | RDLH_LINK_UP;
> +
> +	return !!((val & mask) == mask);

I think the "!!" is redundant since you're applying it to a value
that's a boolean already.  So you should be able to do:

  return (val & mask) == mask;

But it seems like "mask" just obfuscates a little bit, too.
Personally I think it'd be easier to add something like:

  #define PCIE_REGS_PCIE_SII_LINK_UP  (SMLH_LINK_UP | RDLH_LINK_UP)

and then:

  if ((val & PCIE_REGS_PCIE_SII_LINK_UP) == PCIE_REGS_PCIE_SII_LINK_UP)
    return 1;
  return 0;

or even:

  return (val & PCIE_REGS_PCIE_SII_LINK_UP) == PCIE_REGS_PCIE_SII_LINK_UP;

> +static int keembay_pcie_establish_link(struct dw_pcie *pci)
> +{
> +	return 0;
> +}

Are you sure you need this?  I see other cases where the .start_link
pointer is left NULL, e.g., pci-exynos.c, pci-imx6.c,
dw_ls1021_pcie_ops, etc.

> +static int keembay_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> +				     enum pci_epc_irq_type type,
> +				     u16 interrupt_num)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +
> +	switch (type) {
> +	case PCI_EPC_IRQ_LEGACY:
> +		/* Legacy interrupts are not supported in Keem Bay */
> +		dev_err(pci->dev, "Unsupported IRQ type\n");

Might be nice to mention "legacy" here.

> +		return -EINVAL;
> +	case PCI_EPC_IRQ_MSI:
> +		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
> +	case PCI_EPC_IRQ_MSIX:
> +		return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num);
> +	default:
> +		dev_err(pci->dev, "Unknown IRQ type\n");

And maybe include the %d of "type"?

Re: [PATCH 2/2] PCI: keembay: Add support for Intel Keem Bay

[Andy Shevchenko]

On Tue, Nov 03, 2020 at 04:22:23PM -0600, Bjorn Helgaas wrote:
> On Tue, Oct 27, 2020 at 02:00:11PM +0800, Wan Ahmad Zainie wrote:
> > +static int keembay_pcie_link_up(struct dw_pcie *pci)
> > +{
> > +	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
> > +	u32 val, mask;
> > +
> > +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_SII_PM_STATE);
> > +	mask = SMLH_LINK_UP | RDLH_LINK_UP;
> > +
> > +	return !!((val & mask) == mask);
> 
> I think the "!!" is redundant since you're applying it to a value
> that's a boolean already.  So you should be able to do:
> 
>   return (val & mask) == mask;
> 
> But it seems like "mask" just obfuscates a little bit, too.
> Personally I think it'd be easier to add something like:
> 
>   #define PCIE_REGS_PCIE_SII_LINK_UP  (SMLH_LINK_UP | RDLH_LINK_UP)
> 
> and then:

>   if ((val & PCIE_REGS_PCIE_SII_LINK_UP) == PCIE_REGS_PCIE_SII_LINK_UP)
>     return 1;
>   return 0;

The !! is usual way to guarantee that *int* type out of boolean will be only
0 or 1. That said, the above proposal suits better.

> or even:
> 
>   return (val & PCIE_REGS_PCIE_SII_LINK_UP) == PCIE_REGS_PCIE_SII_LINK_UP;

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 2/2] PCI: keembay: Add support for Intel Keem Bay

[Wan Mohamad, Wan Ahmad Zainie]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, November 4, 2020 6:22 AM
> To: Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>
> Cc: bhelgaas@google.com; robh+dt@kernel.org; lorenzo.pieralisi@arm.com;
> linux-pci@vger.kernel.org; devicetree@vger.kernel.org;
> andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Raja
> Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>
> Subject: Re: [PATCH 2/2] PCI: keembay: Add support for Intel Keem Bay
> 
> On Tue, Oct 27, 2020 at 02:00:11PM +0800, Wan Ahmad Zainie wrote:
> 
> > +static int keembay_pcie_link_up(struct dw_pcie *pci) {
> > +	struct keembay_pcie *pcie = dev_get_drvdata(pci->dev);
> > +	u32 val, mask;
> > +
> > +	val = keembay_pcie_readl(pcie, PCIE_REGS_PCIE_SII_PM_STATE);
> > +	mask = SMLH_LINK_UP | RDLH_LINK_UP;
> > +
> > +	return !!((val & mask) == mask);
> 
> I think the "!!" is redundant since you're applying it to a value that's a boolean
> already.  So you should be able to do:
> 
>   return (val & mask) == mask;
> 
> But it seems like "mask" just obfuscates a little bit, too.
> Personally I think it'd be easier to add something like:
> 
>   #define PCIE_REGS_PCIE_SII_LINK_UP  (SMLH_LINK_UP | RDLH_LINK_UP)
> 
> and then:
> 
>   if ((val & PCIE_REGS_PCIE_SII_LINK_UP) == PCIE_REGS_PCIE_SII_LINK_UP)
>     return 1;
>   return 0;

I will fix in v2, using the above as agreed by Andy.

> 
> or even:
> 
>   return (val & PCIE_REGS_PCIE_SII_LINK_UP) ==
> PCIE_REGS_PCIE_SII_LINK_UP;
> 
> > +static int keembay_pcie_establish_link(struct dw_pcie *pci) {
> > +	return 0;
> > +}
> 
> Are you sure you need this?  I see other cases where the .start_link pointer is
> left NULL, e.g., pci-exynos.c, pci-imx6.c, dw_ls1021_pcie_ops, etc.

Yes, as in endpoint mode, link initialization is done in boot firmware.
Leaving it NULL will cause pcie-designware-ep.c::dw_pcie_ep_start()
to return -EINVAL.

Rob is refactoring DWC code and looks like .start_link is used in root complex
mode too. I will make changes to above function in v2, by renaming to
keembay_pci2_start_link and add link initialization code for root complex
mode.

> 
> > +static int keembay_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8
> func_no,
> > +				     enum pci_epc_irq_type type,
> > +				     u16 interrupt_num)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +
> > +	switch (type) {
> > +	case PCI_EPC_IRQ_LEGACY:
> > +		/* Legacy interrupts are not supported in Keem Bay */
> > +		dev_err(pci->dev, "Unsupported IRQ type\n");
> 
> Might be nice to mention "legacy" here.

I will fix in v2, using string "Legacy IRQ is not supported".

> 
> > +		return -EINVAL;
> > +	case PCI_EPC_IRQ_MSI:
> > +		return dw_pcie_ep_raise_msi_irq(ep, func_no,
> interrupt_num);
> > +	case PCI_EPC_IRQ_MSIX:
> > +		return dw_pcie_ep_raise_msix_irq(ep, func_no,
> interrupt_num);
> > +	default:
> > +		dev_err(pci->dev, "Unknown IRQ type\n");
> 
> And maybe include the %d of "type"?

I will fix in v2, to show the value of "type".

Best regards,
Zainie