[v7,0/7] PCI: mediatek: Add new generation controller support

[v7,0/7] PCI: mediatek: Add new generation controller support

[Jianjun Wang]

These series patches add pcie-mediatek-gen3.c and dt-bindings file to
support new generation PCIe controller.

Changes in v7:
1. Split the driver patch to core PCIe, INTx, MSI and PM patches;
2. Reshape MSI init and handle flow, use msi_bottom_domain to cover all sets;
3. Replace readl/writel with their relaxed version;
4. Add MSI description in binding document;
5. Add pl_250m clock in binding document.

Changes in v6:
1. Export pci_pio_to_address() to support compiling as kernel module;
2. Replace usleep_range(100 * 1000, 120 * 1000) with msleep(100);
3. Replace dev_notice with dev_err;
4. Fix MSI get hwirq flow;
5. Fix warning for possible recursive locking in mtk_pcie_set_affinity.

Changes in v5:
1. Remove unused macros
2. Modify the config read/write callbacks, set the config byte field
   in TLP header and use pci_generic_config_read32/write32
   to access the config space
3. Fix the settings of translation window, both MEM and IO regions
   works properly
4. Fix typos

Changes in v4:
1. Fix PCIe power up/down flow
2. Use "mac" and "phy" for reset names
3. Add clock names
4. Fix the variables type

Changes in v3:
1. Remove standard property in binding document
2. Return error number when get_optional* API throws an error
3. Use the bulk clk APIs

Changes in v2:
1. Fix the typo of dt-bindings patch
2. Remove the unnecessary properties in binding document
3. dispos the irq mappings of msi top domain when irq teardown


Jianjun Wang (7):
  dt-bindings: PCI: mediatek-gen3: Add YAML schema
  PCI: Export pci_pio_to_address() for module use
  PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
  PCI: mediatek-gen3: Add INTx support
  PCI: mediatek-gen3: Add MSI support
  PCI: mediatek-gen3: Add system PM support
  MAINTAINERS: Add Jianjun Wang as MediaTek PCI co-maintainer

 .../bindings/pci/mediatek-pcie-gen3.yaml      | 172 ++++
 MAINTAINERS                                   |   1 +
 drivers/pci/controller/Kconfig                |  13 +
 drivers/pci/controller/Makefile               |   1 +
 drivers/pci/controller/pcie-mediatek-gen3.c   | 965 ++++++++++++++++++
 drivers/pci/pci.c                             |   1 +
 6 files changed, 1153 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
 create mode 100644 drivers/pci/controller/pcie-mediatek-gen3.c

-- 
2.25.1

[v7,1/7] dt-bindings: PCI: mediatek-gen3: Add YAML schema

[Jianjun Wang]

Add YAML schemas documentation for Gen3 PCIe controller on
MediaTek SoCs.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 .../bindings/pci/mediatek-pcie-gen3.yaml      | 172 ++++++++++++++++++
 1 file changed, 172 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml

diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
new file mode 100644
index 000000000000..f133fb0184f1
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
@@ -0,0 +1,172 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/mediatek-pcie-gen3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Gen3 PCIe controller on MediaTek SoCs
+
+maintainers:
+  - Jianjun Wang <jianjun.wang@mediatek.com>
+
+description: |+
+  PCIe Gen3 MAC controller for MediaTek SoCs, it supports Gen3 speed
+  and compatible with Gen2, Gen1 speed.
+
+  This PCIe controller supports up to 256 MSI vectors, the MSI hardware
+  block diagram is as follows:
+
+                    +-----+
+                    | GIC |
+                    +-----+
+                       ^
+                       |
+                   port->irq
+                       |
+               +-+-+-+-+-+-+-+-+
+               |0|1|2|3|4|5|6|7| (PCIe intc)
+               +-+-+-+-+-+-+-+-+
+                ^ ^           ^
+                | |    ...    |
+        +-------+ +------+    +-----------+
+        |                |                |
+  +-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
+  |0|1|...|30|31|  |0|1|...|30|31|  |0|1|...|30|31| (MSI sets)
+  +-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
+   ^ ^      ^  ^    ^ ^      ^  ^    ^ ^      ^  ^
+   | |      |  |    | |      |  |    | |      |  |  (MSI vectors)
+   | |      |  |    | |      |  |    | |      |  |
+
+    (MSI SET0)       (MSI SET1)  ...   (MSI SET7)
+
+  With 256 MSI vectors supported, the MSI vectors are composed of 8 sets,
+  each set has its own address for MSI message, and supports 32 MSI vectors
+  to generate interrupt.
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+
+properties:
+  compatible:
+    const: mediatek,mt8192-pcie
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  ranges:
+    minItems: 1
+    maxItems: 8
+
+  resets:
+    minItems: 1
+    maxItems: 2
+
+  reset-names:
+    anyOf:
+      - const: mac
+      - const: phy
+
+  clocks:
+    maxItems: 6
+
+  clock-names:
+    items:
+      - const: pl_250m
+      - const: tl_26m
+      - const: tl_96m
+      - const: tl_32k
+      - const: peri_26m
+      - const: top_133m
+
+  assigned-clocks:
+    maxItems: 1
+
+  assigned-clock-parents:
+    maxItems: 1
+
+  phys:
+    maxItems: 1
+
+  '#interrupt-cells':
+    const: 1
+
+  interrupt-controller:
+    description: Interrupt controller node for handling legacy PCI interrupts.
+    type: object
+    properties:
+      '#address-cells':
+        const: 0
+      '#interrupt-cells':
+        const: 1
+      interrupt-controller: true
+
+    required:
+      - '#address-cells'
+      - '#interrupt-cells'
+      - interrupt-controller
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - ranges
+  - clocks
+  - '#interrupt-cells'
+  - interrupt-controller
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pcie: pcie@11230000 {
+            compatible = "mediatek,mt8192-pcie";
+            device_type = "pci";
+            #address-cells = <3>;
+            #size-cells = <2>;
+            reg = <0x00 0x11230000 0x00 0x4000>;
+            reg-names = "pcie-mac";
+            interrupts = <GIC_SPI 251 IRQ_TYPE_LEVEL_HIGH 0>;
+            bus-range = <0x00 0xff>;
+            ranges = <0x82000000 0x00 0x12000000 0x00
+                      0x12000000 0x00 0x1000000>;
+            clocks = <&infracfg 44>,
+                     <&infracfg 40>,
+                     <&infracfg 43>,
+                     <&infracfg 97>,
+                     <&infracfg 99>,
+                     <&infracfg 111>;
+            clock-names = "pl_250m", "tl_26m", "tl_96m",
+                          "tl_32k", "peri_26m", "top_133m";
+            assigned-clocks = <&topckgen 50>;
+            assigned-clock-parents = <&topckgen 91>;
+
+            phys = <&pciephy>;
+            phy-names = "pcie-phy";
+            resets = <&infracfg_rst 0>;
+            reset-names = "phy";
+
+            #interrupt-cells = <1>;
+            interrupt-map-mask = <0 0 0 0x7>;
+            interrupt-map = <0 0 0 1 &pcie_intc 0>,
+                            <0 0 0 2 &pcie_intc 1>,
+                            <0 0 0 3 &pcie_intc 2>,
+                            <0 0 0 4 &pcie_intc 3>;
+            pcie_intc: interrupt-controller {
+                      #address-cells = <0>;
+                      #interrupt-cells = <1>;
+                      interrupt-controller;
+            };
+        };
+    };
-- 
2.25.1

[v7,2/7] PCI: Export pci_pio_to_address() for module use

[Jianjun Wang]

This interface will be used by PCI host drivers for PIO translation,
export it to support compiling those drivers as kernel modules.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
---
 drivers/pci/pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6d4d5a2f923d..3de714db2557 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4016,6 +4016,7 @@ phys_addr_t pci_pio_to_address(unsigned long pio)
 
 	return address;
 }
+EXPORT_SYMBOL(pci_pio_to_address);
 
 unsigned long __weak pci_address_to_pio(phys_addr_t address)
 {
-- 
2.25.1

[v7,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192

[Jianjun Wang]

MediaTek's PCIe host controller has three generation HWs, the new
generation HW is an individual bridge, it supports Gen3 speed and
compatible with Gen2, Gen1 speed.

Add support for new Gen3 controller which can be found on MT8192.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/Kconfig              |  13 +
 drivers/pci/controller/Makefile             |   1 +
 drivers/pci/controller/pcie-mediatek-gen3.c | 463 ++++++++++++++++++++
 3 files changed, 477 insertions(+)
 create mode 100644 drivers/pci/controller/pcie-mediatek-gen3.c

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 64e2f5e379aa..b242b17025b3 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -242,6 +242,19 @@ config PCIE_MEDIATEK
 	  Say Y here if you want to enable PCIe controller support on
 	  MediaTek SoCs.
 
+config PCIE_MEDIATEK_GEN3
+	tristate "MediaTek Gen3 PCIe controller"
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	depends on PCI_MSI_IRQ_DOMAIN
+	help
+	  Adds support for PCIe Gen3 MAC controller for MediaTek SoCs.
+	  This PCIe controller is compatible with Gen3, Gen2 and Gen1 speed,
+	  and support up to 256 MSI interrupt numbers for
+	  multi-function devices.
+
+	  Say Y here if you want to enable Gen3 PCIe controller support on
+	  MediaTek SoCs.
+
 config PCIE_TANGO_SMP8759
 	bool "Tango SMP8759 PCIe controller (DANGEROUS)"
 	depends on ARCH_TANGO && PCI_MSI && OF
diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
index 04c6edc285c5..df5d77d72a9d 100644
--- a/drivers/pci/controller/Makefile
+++ b/drivers/pci/controller/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
 obj-$(CONFIG_PCIE_ROCKCHIP_EP) += pcie-rockchip-ep.o
 obj-$(CONFIG_PCIE_ROCKCHIP_HOST) += pcie-rockchip-host.o
 obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o
+obj-$(CONFIG_PCIE_MEDIATEK_GEN3) += pcie-mediatek-gen3.o
 obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o
 obj-$(CONFIG_VMD) += vmd.o
 obj-$(CONFIG_PCIE_BRCMSTB) += pcie-brcmstb.o
diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
new file mode 100644
index 000000000000..c00ea7c167de
--- /dev/null
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -0,0 +1,463 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MediaTek PCIe host controller driver.
+ *
+ * Copyright (c) 2020 MediaTek Inc.
+ * Author: Jianjun Wang <jianjun.wang@mediatek.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_clk.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/pci.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+
+#include "../pci.h"
+
+#define PCIE_SETTING_REG		0x80
+#define PCIE_PCI_IDS_1			0x9c
+#define PCI_CLASS(class)		(class << 8)
+#define PCIE_RC_MODE			BIT(0)
+
+#define PCIE_CFGNUM_REG			0x140
+#define PCIE_CFG_DEVFN(devfn)		((devfn) & GENMASK(7, 0))
+#define PCIE_CFG_BUS(bus)		(((bus) << 8) & GENMASK(15, 8))
+#define PCIE_CFG_BYTE_EN(bytes)		(((bytes) << 16) & GENMASK(19, 16))
+#define PCIE_CFG_FORCE_BYTE_EN		BIT(20)
+#define PCIE_CFG_OFFSET_ADDR		0x1000
+#define PCIE_CFG_HEADER(bus, devfn) \
+	(PCIE_CFG_BUS(bus) | PCIE_CFG_DEVFN(devfn))
+
+#define PCIE_RST_CTRL_REG		0x148
+#define PCIE_MAC_RSTB			BIT(0)
+#define PCIE_PHY_RSTB			BIT(1)
+#define PCIE_BRG_RSTB			BIT(2)
+#define PCIE_PE_RSTB			BIT(3)
+
+#define PCIE_LTSSM_STATUS_REG		0x150
+
+#define PCIE_LINK_STATUS_REG		0x154
+#define PCIE_PORT_LINKUP		BIT(8)
+
+#define PCIE_TRANS_TABLE_BASE_REG	0x800
+#define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
+#define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8
+#define PCIE_ATR_TRSL_ADDR_MSB_OFFSET	0xc
+#define PCIE_ATR_TRSL_PARAM_OFFSET	0x10
+#define PCIE_ATR_TLB_SET_OFFSET		0x20
+
+#define PCIE_MAX_TRANS_TABLES		8
+#define PCIE_ATR_EN			BIT(0)
+#define PCIE_ATR_SIZE(size) \
+	(((((size) - 1) << 1) & GENMASK(6, 1)) | PCIE_ATR_EN)
+#define PCIE_ATR_ID(id)			((id) & GENMASK(3, 0))
+#define PCIE_ATR_TYPE_MEM		PCIE_ATR_ID(0)
+#define PCIE_ATR_TYPE_IO		PCIE_ATR_ID(1)
+#define PCIE_ATR_TLP_TYPE(type)		(((type) << 16) & GENMASK(18, 16))
+#define PCIE_ATR_TLP_TYPE_MEM		PCIE_ATR_TLP_TYPE(0)
+#define PCIE_ATR_TLP_TYPE_IO		PCIE_ATR_TLP_TYPE(2)
+
+/**
+ * struct mtk_pcie_port - PCIe port information
+ * @dev: pointer to PCIe device
+ * @base: IO mapped register base
+ * @reg_base: Physical register base
+ * @mac_reset: mac reset control
+ * @phy_reset: phy reset control
+ * @phy: PHY controller block
+ * @clks: PCIe clocks
+ * @num_clks: PCIe clocks count for this port
+ */
+struct mtk_pcie_port {
+	struct device *dev;
+	void __iomem *base;
+	phys_addr_t reg_base;
+	struct reset_control *mac_reset;
+	struct reset_control *phy_reset;
+	struct phy *phy;
+	struct clk_bulk_data *clks;
+	int num_clks;
+};
+
+/**
+ * mtk_pcie_config_tlp_header
+ * @bus: PCI bus to query
+ * @devfn: device/function number
+ * @where: offset in config space
+ * @size: data size in TLP header
+ *
+ * Set byte enable field and device information in configuration TLP header.
+ */
+static void mtk_pcie_config_tlp_header(struct pci_bus *bus, unsigned int devfn,
+					int where, int size)
+{
+	struct mtk_pcie_port *port = bus->sysdata;
+	int bytes;
+	u32 val;
+
+	bytes = (GENMASK(size - 1, 0) & 0xf) << (where & 0x3);
+
+	val = PCIE_CFG_FORCE_BYTE_EN | PCIE_CFG_BYTE_EN(bytes) |
+	      PCIE_CFG_HEADER(bus->number, devfn);
+
+	writel_relaxed(val, port->base + PCIE_CFGNUM_REG);
+}
+
+static void __iomem *mtk_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
+				      int where)
+{
+	struct mtk_pcie_port *port = bus->sysdata;
+
+	return port->base + PCIE_CFG_OFFSET_ADDR + where;
+}
+
+static int mtk_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
+				int where, int size, u32 *val)
+{
+	mtk_pcie_config_tlp_header(bus, devfn, where, size);
+
+	return pci_generic_config_read32(bus, devfn, where, size, val);
+}
+
+static int mtk_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
+				 int where, int size, u32 val)
+{
+	mtk_pcie_config_tlp_header(bus, devfn, where, size);
+
+	if (size <= 2)
+		val <<= (where & 0x3) * 8;
+
+	return pci_generic_config_write32(bus, devfn, where, 4, val);
+}
+
+static struct pci_ops mtk_pcie_ops = {
+	.map_bus = mtk_pcie_map_bus,
+	.read  = mtk_pcie_config_read,
+	.write = mtk_pcie_config_write,
+};
+
+static int mtk_pcie_set_trans_table(struct mtk_pcie_port *port,
+				    resource_size_t cpu_addr,
+				    resource_size_t pci_addr,
+				    resource_size_t size,
+				    unsigned long type, int num)
+{
+	void __iomem *table;
+	u32 val;
+
+	if (num >= PCIE_MAX_TRANS_TABLES) {
+		dev_err(port->dev, "not enough translate table[%d] for addr: %#llx, limited to [%d]\n",
+			num, (unsigned long long) cpu_addr,
+			PCIE_MAX_TRANS_TABLES);
+		return -ENODEV;
+	}
+
+	table = port->base + PCIE_TRANS_TABLE_BASE_REG +
+		num * PCIE_ATR_TLB_SET_OFFSET;
+
+	writel_relaxed(lower_32_bits(cpu_addr) | PCIE_ATR_SIZE(fls(size) - 1),
+		       table);
+	writel_relaxed(upper_32_bits(cpu_addr),
+		       table + PCIE_ATR_SRC_ADDR_MSB_OFFSET);
+	writel_relaxed(lower_32_bits(pci_addr),
+		       table + PCIE_ATR_TRSL_ADDR_LSB_OFFSET);
+	writel_relaxed(upper_32_bits(pci_addr),
+		       table + PCIE_ATR_TRSL_ADDR_MSB_OFFSET);
+
+	if (type == IORESOURCE_IO)
+		val = PCIE_ATR_TYPE_IO | PCIE_ATR_TLP_TYPE_IO;
+	else
+		val = PCIE_ATR_TYPE_MEM | PCIE_ATR_TLP_TYPE_MEM;
+
+	writel_relaxed(val, table + PCIE_ATR_TRSL_PARAM_OFFSET);
+
+	return 0;
+}
+
+static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
+{
+	struct resource_entry *entry;
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(port);
+	unsigned int table_index = 0;
+	int err;
+	u32 val;
+
+	/* Set as RC mode */
+	val = readl_relaxed(port->base + PCIE_SETTING_REG);
+	val |= PCIE_RC_MODE;
+	writel_relaxed(val, port->base + PCIE_SETTING_REG);
+
+	/* Set class code */
+	val = readl_relaxed(port->base + PCIE_PCI_IDS_1);
+	val &= ~GENMASK(31, 8);
+	val |= PCI_CLASS(PCI_CLASS_BRIDGE_PCI << 8);
+	writel_relaxed(val, port->base + PCIE_PCI_IDS_1);
+
+	/* Assert all reset signals */
+	val = readl_relaxed(port->base + PCIE_RST_CTRL_REG);
+	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
+	writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
+
+	/* De-assert reset signals */
+	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB);
+	writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
+
+	/* Delay 100ms to wait the reference clocks become stable */
+	msleep(100);
+
+	/* De-assert PERST# signal */
+	val &= ~PCIE_PE_RSTB;
+	writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
+
+	/* Check if the link is up or not */
+	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val,
+				 !!(val & PCIE_PORT_LINKUP), 20,
+				 50 * USEC_PER_MSEC);
+	if (err) {
+		val = readl_relaxed(port->base + PCIE_LTSSM_STATUS_REG);
+		dev_err(port->dev, "PCIe link down, ltssm reg val: %#x\n", val);
+		return err;
+	}
+
+	/* Set PCIe translation windows */
+	resource_list_for_each_entry(entry, &host->windows) {
+		struct resource *res = entry->res;
+		unsigned long type = resource_type(res);
+		resource_size_t cpu_addr;
+		resource_size_t pci_addr;
+		resource_size_t size;
+		const char *range_type;
+
+		if (type == IORESOURCE_IO) {
+			cpu_addr = pci_pio_to_address(res->start);
+			range_type = "IO";
+		} else if (type == IORESOURCE_MEM) {
+			cpu_addr = res->start;
+			range_type = "MEM";
+		} else {
+			continue;
+		}
+
+		pci_addr = res->start - entry->offset;
+		size = resource_size(res);
+		err = mtk_pcie_set_trans_table(port, cpu_addr, pci_addr, size,
+					       type, table_index);
+		if (err)
+			return err;
+
+		dev_dbg(port->dev, "set %s trans window[%d]: cpu_addr = %#llx, pci_addr = %#llx, size = %#llx\n",
+			range_type, table_index, (unsigned long long) cpu_addr,
+			(unsigned long long) pci_addr,
+			(unsigned long long) size);
+
+		table_index++;
+	}
+
+	return 0;
+}
+
+static int mtk_pcie_clk_init(struct mtk_pcie_port *port)
+{
+	int ret;
+
+	port->num_clks = devm_clk_bulk_get_all(port->dev, &port->clks);
+	if (port->num_clks < 0) {
+		dev_err(port->dev, "failed to get PCIe clock\n");
+		return port->num_clks;
+	}
+
+	ret = clk_bulk_prepare_enable(port->num_clks, port->clks);
+	if (ret) {
+		dev_err(port->dev, "failed to enable PCIe clocks\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int mtk_pcie_power_up(struct mtk_pcie_port *port)
+{
+	struct device *dev = port->dev;
+	int err;
+
+	port->phy_reset = devm_reset_control_get_optional_exclusive(dev, "phy");
+	if (IS_ERR(port->phy_reset))
+		return PTR_ERR(port->phy_reset);
+
+	/* PHY power on and enable pipe clock */
+	port->phy = devm_phy_optional_get(dev, "pcie-phy");
+	if (IS_ERR(port->phy))
+		return PTR_ERR(port->phy);
+
+	reset_control_deassert(port->phy_reset);
+
+	err = phy_init(port->phy);
+	if (err) {
+		dev_err(dev, "failed to initialize PCIe phy\n");
+		goto err_phy_init;
+	}
+
+	err = phy_power_on(port->phy);
+	if (err) {
+		dev_err(dev, "failed to power on PCIe phy\n");
+		goto err_phy_on;
+	}
+
+	port->mac_reset = devm_reset_control_get_optional_exclusive(dev, "mac");
+	if (IS_ERR(port->mac_reset)) {
+		err = PTR_ERR(port->mac_reset);
+		goto err_mac_rst;
+	}
+
+	reset_control_deassert(port->mac_reset);
+
+	/* MAC power on and enable transaction layer clocks */
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+
+	err = mtk_pcie_clk_init(port);
+	if (err) {
+		dev_err(dev, "clock init failed\n");
+		goto err_clk_init;
+	}
+
+	return 0;
+
+err_clk_init:
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+	reset_control_assert(port->mac_reset);
+err_mac_rst:
+	phy_power_off(port->phy);
+err_phy_on:
+	phy_exit(port->phy);
+err_phy_init:
+	reset_control_assert(port->phy_reset);
+
+	return err;
+}
+
+static void mtk_pcie_power_down(struct mtk_pcie_port *port)
+{
+	clk_bulk_disable_unprepare(port->num_clks, port->clks);
+
+	pm_runtime_put_sync(port->dev);
+	pm_runtime_disable(port->dev);
+	reset_control_assert(port->mac_reset);
+
+	phy_power_off(port->phy);
+	phy_exit(port->phy);
+	reset_control_assert(port->phy_reset);
+}
+
+static int mtk_pcie_setup(struct mtk_pcie_port *port)
+{
+	struct device *dev = port->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct resource *regs;
+	int err;
+
+	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");
+	port->base = devm_ioremap_resource(dev, regs);
+	if (IS_ERR(port->base)) {
+		dev_err(dev, "failed to map register base\n");
+		return PTR_ERR(port->base);
+	}
+
+	port->reg_base = regs->start;
+
+	/* Don't touch the hardware registers before power up */
+	err = mtk_pcie_power_up(port);
+	if (err)
+		return err;
+
+	/* Try link up */
+	err = mtk_pcie_startup_port(port);
+	if (err) {
+		dev_err(dev, "PCIe startup failed\n");
+		goto err_setup;
+	}
+
+	dev_info(dev, "PCIe link up success!\n");
+
+	return 0;
+
+err_setup:
+	mtk_pcie_power_down(port);
+
+	return err;
+}
+
+static int mtk_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mtk_pcie_port *port;
+	struct pci_host_bridge *host;
+	int err;
+
+	host = devm_pci_alloc_host_bridge(dev, sizeof(*port));
+	if (!host)
+		return -ENOMEM;
+
+	port = pci_host_bridge_priv(host);
+
+	port->dev = dev;
+	platform_set_drvdata(pdev, port);
+
+	err = mtk_pcie_setup(port);
+	if (err)
+		return err;
+
+	host->ops = &mtk_pcie_ops;
+	host->sysdata = port;
+
+	err = pci_host_probe(host);
+	if (err) {
+		mtk_pcie_power_down(port);
+		return err;
+	}
+
+	return 0;
+}
+
+static int mtk_pcie_remove(struct platform_device *pdev)
+{
+	struct mtk_pcie_port *port = platform_get_drvdata(pdev);
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(port);
+
+	pci_lock_rescan_remove();
+	pci_stop_root_bus(host->bus);
+	pci_remove_root_bus(host->bus);
+	pci_unlock_rescan_remove();
+
+	mtk_pcie_power_down(port);
+
+	return 0;
+}
+
+static const struct of_device_id mtk_pcie_of_match[] = {
+	{ .compatible = "mediatek,mt8192-pcie" },
+	{},
+};
+
+static struct platform_driver mtk_pcie_driver = {
+	.probe = mtk_pcie_probe,
+	.remove = mtk_pcie_remove,
+	.driver = {
+		.name = "mtk-pcie",
+		.of_match_table = mtk_pcie_of_match,
+	},
+};
+
+module_platform_driver(mtk_pcie_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

[v7,4/7] PCI: mediatek-gen3: Add INTx support

[Jianjun Wang]

Add INTx support for MediaTek Gen3 PCIe controller.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 163 ++++++++++++++++++++
 1 file changed, 163 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index c00ea7c167de..7979a2856c35 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -9,6 +9,9 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/iopoll.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
@@ -49,6 +52,12 @@
 #define PCIE_LINK_STATUS_REG		0x154
 #define PCIE_PORT_LINKUP		BIT(8)
 
+#define PCIE_INT_ENABLE_REG		0x180
+#define PCIE_INTX_SHIFT			24
+#define PCIE_INTX_MASK			GENMASK(27, 24)
+
+#define PCIE_INT_STATUS_REG		0x184
+
 #define PCIE_TRANS_TABLE_BASE_REG	0x800
 #define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
 #define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8
@@ -77,6 +86,8 @@
  * @phy: PHY controller block
  * @clks: PCIe clocks
  * @num_clks: PCIe clocks count for this port
+ * @irq: PCIe controller interrupt number
+ * @intx_domain: legacy INTx IRQ domain
  */
 struct mtk_pcie_port {
 	struct device *dev;
@@ -87,6 +98,9 @@ struct mtk_pcie_port {
 	struct phy *phy;
 	struct clk_bulk_data *clks;
 	int num_clks;
+
+	int irq;
+	struct irq_domain *intx_domain;
 };
 
 /**
@@ -266,6 +280,149 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
 	return 0;
 }
 
+static int mtk_pcie_set_affinity(struct irq_data *data,
+				 const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static void mtk_intx_mask(struct irq_data *data)
+{
+	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
+	u32 val;
+
+	val = readl_relaxed(port->base + PCIE_INT_ENABLE_REG);
+	val &= ~BIT(data->hwirq + PCIE_INTX_SHIFT);
+	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
+}
+
+static void mtk_intx_unmask(struct irq_data *data)
+{
+	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
+	u32 val;
+
+	val = readl_relaxed(port->base + PCIE_INT_ENABLE_REG);
+	val |= BIT(data->hwirq + PCIE_INTX_SHIFT);
+	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
+}
+
+/**
+ * mtk_intx_eoi
+ * @data: pointer to chip specific data
+ *
+ * As an emulated level IRQ, its interrupt status will remain
+ * until the corresponding de-assert message is received; hence that
+ * the status can only be cleared when the interrupt has been serviced.
+ */
+static void mtk_intx_eoi(struct irq_data *data)
+{
+	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
+	unsigned long hwirq;
+
+	hwirq = data->hwirq + PCIE_INTX_SHIFT;
+	writel_relaxed(BIT(hwirq), port->base + PCIE_INT_STATUS_REG);
+}
+
+static struct irq_chip mtk_intx_irq_chip = {
+	.irq_mask		= mtk_intx_mask,
+	.irq_unmask		= mtk_intx_unmask,
+	.irq_eoi		= mtk_intx_eoi,
+	.irq_set_affinity	= mtk_pcie_set_affinity,
+	.name			= "PCIe",
+};
+
+static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
+			     irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler_name(irq, &mtk_intx_irq_chip,
+				      handle_fasteoi_irq, "INTx");
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops intx_domain_ops = {
+	.map = mtk_pcie_intx_map,
+};
+
+static int mtk_pcie_init_irq_domains(struct mtk_pcie_port *port,
+				     struct device_node *node)
+{
+	struct device *dev = port->dev;
+	struct device_node *intc_node;
+
+	/* Setup INTx */
+	intc_node = of_get_child_by_name(node, "interrupt-controller");
+	if (!intc_node) {
+		dev_err(dev, "missing PCIe Intc node\n");
+		return -ENODEV;
+	}
+
+	port->intx_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX,
+						  &intx_domain_ops, port);
+	if (!port->intx_domain) {
+		dev_err(dev, "failed to get INTx IRQ domain\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static void mtk_pcie_irq_teardown(struct mtk_pcie_port *port)
+{
+	irq_set_chained_handler_and_data(port->irq, NULL, NULL);
+
+	if (port->intx_domain)
+		irq_domain_remove(port->intx_domain);
+
+	irq_dispose_mapping(port->irq);
+}
+
+static void mtk_pcie_irq_handler(struct irq_desc *desc)
+{
+	struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);
+	struct irq_chip *irqchip = irq_desc_get_chip(desc);
+	unsigned long status;
+	unsigned int virq;
+	irq_hw_number_t irq_bit = PCIE_INTX_SHIFT;
+
+	chained_irq_enter(irqchip, desc);
+
+	status = readl_relaxed(port->base + PCIE_INT_STATUS_REG);
+	if (status & PCIE_INTX_MASK) {
+		for_each_set_bit_from(irq_bit, &status, PCI_NUM_INTX +
+				      PCIE_INTX_SHIFT) {
+			virq = irq_find_mapping(port->intx_domain,
+						irq_bit - PCIE_INTX_SHIFT);
+			generic_handle_irq(virq);
+		}
+	}
+
+	chained_irq_exit(irqchip, desc);
+}
+
+static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
+			      struct device_node *node)
+{
+	struct device *dev = port->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	int err;
+
+	err = mtk_pcie_init_irq_domains(port, node);
+	if (err) {
+		dev_err(dev, "failed to init PCIe IRQ domain\n");
+		return err;
+	}
+
+	port->irq = platform_get_irq(pdev, 0);
+	if (port->irq < 0)
+		return port->irq;
+
+	irq_set_chained_handler_and_data(port->irq, mtk_pcie_irq_handler, port);
+
+	return 0;
+}
+
 static int mtk_pcie_clk_init(struct mtk_pcie_port *port)
 {
 	int ret;
@@ -388,6 +545,10 @@ static int mtk_pcie_setup(struct mtk_pcie_port *port)
 		goto err_setup;
 	}
 
+	err = mtk_pcie_setup_irq(port, dev->of_node);
+	if (err)
+		goto err_setup;
+
 	dev_info(dev, "PCIe link up success!\n");
 
 	return 0;
@@ -423,6 +584,7 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 
 	err = pci_host_probe(host);
 	if (err) {
+		mtk_pcie_irq_teardown(port);
 		mtk_pcie_power_down(port);
 		return err;
 	}
@@ -440,6 +602,7 @@ static int mtk_pcie_remove(struct platform_device *pdev)
 	pci_remove_root_bus(host->bus);
 	pci_unlock_rescan_remove();
 
+	mtk_pcie_irq_teardown(port);
 	mtk_pcie_power_down(port);
 
 	return 0;
-- 
2.25.1

[v7,5/7] PCI: mediatek-gen3: Add MSI support

[Jianjun Wang]

Add MSI support for MediaTek Gen3 PCIe controller.

This PCIe controller supports up to 256 MSI vectors, the MSI hardware
block diagram is as follows:

                  +-----+
                  | GIC |
                  +-----+
                     ^
                     |
                 port->irq
                     |
             +-+-+-+-+-+-+-+-+
             |0|1|2|3|4|5|6|7| (PCIe intc)
             +-+-+-+-+-+-+-+-+
              ^ ^           ^
              | |    ...    |
      +-------+ +------+    +-----------+
      |                |                |
+-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
|0|1|...|30|31|  |0|1|...|30|31|  |0|1|...|30|31| (MSI sets)
+-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
 ^ ^      ^  ^    ^ ^      ^  ^    ^ ^      ^  ^
 | |      |  |    | |      |  |    | |      |  |  (MSI vectors)
 | |      |  |    | |      |  |    | |      |  |

  (MSI SET0)       (MSI SET1)  ...   (MSI SET7)

With 256 MSI vectors supported, the MSI vectors are composed of 8 sets,
each set has its own address for MSI message, and supports 32 MSI vectors
to generate interrupt.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 261 ++++++++++++++++++++
 1 file changed, 261 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 7979a2856c35..471d97cd1ef9 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -14,6 +14,7 @@
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/msi.h>
 #include <linux/of_address.h>
 #include <linux/of_clk.h>
 #include <linux/of_pci.h>
@@ -52,11 +53,28 @@
 #define PCIE_LINK_STATUS_REG		0x154
 #define PCIE_PORT_LINKUP		BIT(8)
 
+#define PCIE_MSI_SET_NUM		8
+#define PCIE_MSI_IRQS_PER_SET		32
+#define PCIE_MSI_IRQS_NUM \
+	(PCIE_MSI_IRQS_PER_SET * (PCIE_MSI_SET_NUM))
+
 #define PCIE_INT_ENABLE_REG		0x180
+#define PCIE_MSI_ENABLE			GENMASK(PCIE_MSI_SET_NUM + 8 - 1, 8)
+#define PCIE_MSI_SHIFT			8
 #define PCIE_INTX_SHIFT			24
 #define PCIE_INTX_MASK			GENMASK(27, 24)
 
 #define PCIE_INT_STATUS_REG		0x184
+#define PCIE_MSI_SET_ENABLE_REG		0x190
+#define PCIE_MSI_SET_ENABLE		GENMASK(PCIE_MSI_SET_NUM - 1, 0)
+
+#define PCIE_MSI_SET_BASE_REG		0xc00
+#define PCIE_MSI_SET_OFFSET		0x10
+#define PCIE_MSI_SET_STATUS_OFFSET	0x04
+#define PCIE_MSI_SET_ENABLE_OFFSET	0x08
+
+#define PCIE_MSI_SET_ADDR_HI_BASE	0xc80
+#define PCIE_MSI_SET_ADDR_HI_OFFSET	0x04
 
 #define PCIE_TRANS_TABLE_BASE_REG	0x800
 #define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
@@ -76,6 +94,18 @@
 #define PCIE_ATR_TLP_TYPE_MEM		PCIE_ATR_TLP_TYPE(0)
 #define PCIE_ATR_TLP_TYPE_IO		PCIE_ATR_TLP_TYPE(2)
 
+/**
+ * struct mtk_pcie_msi - MSI information for each set
+ * @dev: pointer to PCIe device
+ * @base: IO mapped register base
+ * @msg_addr: MSI message address
+ */
+struct mtk_msi_set {
+	struct device *dev;
+	void __iomem *base;
+	phys_addr_t msg_addr;
+};
+
 /**
  * struct mtk_pcie_port - PCIe port information
  * @dev: pointer to PCIe device
@@ -88,6 +118,11 @@
  * @num_clks: PCIe clocks count for this port
  * @irq: PCIe controller interrupt number
  * @intx_domain: legacy INTx IRQ domain
+ * @msi_domain: MSI IRQ domain
+ * @msi_bottom_domain: MSI IRQ bottom domain
+ * @msi_sets: MSI sets information
+ * @lock: lock protecting IRQ bit map
+ * @msi_irq_in_use: bit map for assigned MSI IRQ
  */
 struct mtk_pcie_port {
 	struct device *dev;
@@ -101,6 +136,11 @@ struct mtk_pcie_port {
 
 	int irq;
 	struct irq_domain *intx_domain;
+	struct irq_domain *msi_domain;
+	struct irq_domain *msi_bottom_domain;
+	struct mtk_msi_set msi_sets[PCIE_MSI_SET_NUM];
+	struct mutex lock;
+	DECLARE_BITMAP(msi_irq_in_use, PCIE_MSI_IRQS_NUM);
 };
 
 /**
@@ -243,6 +283,15 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
 		return err;
 	}
 
+	/* Enable MSI */
+	val = readl_relaxed(port->base + PCIE_MSI_SET_ENABLE_REG);
+	val |= PCIE_MSI_SET_ENABLE;
+	writel_relaxed(val, port->base + PCIE_MSI_SET_ENABLE_REG);
+
+	val = readl_relaxed(port->base + PCIE_INT_ENABLE_REG);
+	val |= PCIE_MSI_ENABLE;
+	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
+
 	/* Set PCIe translation windows */
 	resource_list_for_each_entry(entry, &host->windows) {
 		struct resource *res = entry->res;
@@ -286,6 +335,129 @@ static int mtk_pcie_set_affinity(struct irq_data *data,
 	return -EINVAL;
 }
 
+static struct irq_chip mtk_msi_irq_chip = {
+	.name = "MSI",
+	.irq_ack = irq_chip_ack_parent,
+};
+
+static struct msi_domain_info mtk_msi_domain_info = {
+	.flags		= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_PCI_MSIX |
+			   MSI_FLAG_USE_DEF_CHIP_OPS | MSI_FLAG_MULTI_PCI_MSI),
+	.chip		= &mtk_msi_irq_chip,
+};
+
+static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
+	unsigned long hwirq;
+
+	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
+
+	msg->address_hi = upper_32_bits(msi_set->msg_addr);
+	msg->address_lo = lower_32_bits(msi_set->msg_addr);
+	msg->data = hwirq;
+	dev_dbg(msi_set->dev, "msi#%#lx address_hi %#x address_lo %#x data %d\n",
+		hwirq, msg->address_hi, msg->address_lo, msg->data);
+}
+
+static void mtk_msi_bottom_irq_ack(struct irq_data *data)
+{
+	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
+	unsigned long hwirq;
+
+	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
+
+	writel_relaxed(BIT(hwirq), msi_set->base + PCIE_MSI_SET_STATUS_OFFSET);
+}
+
+static struct irq_chip mtk_msi_bottom_irq_chip = {
+	.irq_ack		= mtk_msi_bottom_irq_ack,
+	.irq_compose_msi_msg	= mtk_compose_msi_msg,
+	.irq_set_affinity	= mtk_pcie_set_affinity,
+	.name			= "PCIe",
+};
+
+static int mtk_msi_bottom_domain_alloc(struct irq_domain *domain,
+				       unsigned int virq, unsigned int nr_irqs,
+				       void *arg)
+{
+	struct mtk_pcie_port *port = domain->host_data;
+	struct mtk_msi_set *msi_set;
+	int i, hwirq, set_idx;
+
+	mutex_lock(&port->lock);
+
+	hwirq = bitmap_find_free_region(port->msi_irq_in_use, PCIE_MSI_IRQS_NUM,
+					order_base_2(nr_irqs));
+
+	mutex_unlock(&port->lock);
+
+	if (hwirq < 0)
+		return -ENOSPC;
+
+	set_idx = hwirq / PCIE_MSI_IRQS_PER_SET;
+	msi_set = &port->msi_sets[set_idx];
+
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_info(domain, virq + i, hwirq + i,
+				    &mtk_msi_bottom_irq_chip, msi_set,
+				    handle_edge_irq, NULL, NULL);
+
+	return 0;
+}
+
+static void mtk_msi_bottom_domain_free(struct irq_domain *domain,
+				       unsigned int virq, unsigned int nr_irqs)
+{
+	struct mtk_pcie_port *port = domain->host_data;
+	struct irq_data *data = irq_domain_get_irq_data(domain, virq);
+
+	mutex_lock(&port->lock);
+
+	bitmap_clear(port->msi_irq_in_use, data->hwirq, nr_irqs);
+
+	mutex_unlock(&port->lock);
+
+	irq_domain_free_irqs_common(domain, virq, nr_irqs);
+}
+
+static int mtk_msi_bottom_domain_activate(struct irq_domain *domain,
+					  struct irq_data *data, bool reserve)
+{
+	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
+	unsigned long hwirq;
+	u32 val;
+
+	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
+
+	val = readl_relaxed(msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
+	val |= BIT(hwirq);
+	writel_relaxed(val, msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
+
+	return 0;
+}
+
+static void mtk_msi_bottom_domain_deactivate(struct irq_domain *domain,
+					     struct irq_data *data)
+{
+	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
+	unsigned long hwirq;
+	u32 val;
+
+	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
+
+	val = readl_relaxed(msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
+	val &= ~BIT(hwirq);
+	writel_relaxed(val, msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
+}
+
+static const struct irq_domain_ops mtk_msi_bottom_domain_ops = {
+	.alloc = mtk_msi_bottom_domain_alloc,
+	.free = mtk_msi_bottom_domain_free,
+	.activate = mtk_msi_bottom_domain_activate,
+	.deactivate = mtk_msi_bottom_domain_deactivate,
+};
+
 static void mtk_intx_mask(struct irq_data *data)
 {
 	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
@@ -350,6 +522,9 @@ static int mtk_pcie_init_irq_domains(struct mtk_pcie_port *port,
 {
 	struct device *dev = port->dev;
 	struct device_node *intc_node;
+	struct fwnode_handle *fwnode = of_node_to_fwnode(node);
+	struct msi_domain_info *info;
+	int i, ret;
 
 	/* Setup INTx */
 	intc_node = of_get_child_by_name(node, "interrupt-controller");
@@ -365,7 +540,57 @@ static int mtk_pcie_init_irq_domains(struct mtk_pcie_port *port,
 		return -ENODEV;
 	}
 
+	/* Setup MSI */
+	mutex_init(&port->lock);
+
+	port->msi_bottom_domain = irq_domain_add_linear(node, PCIE_MSI_IRQS_NUM,
+				  &mtk_msi_bottom_domain_ops, port);
+	if (!port->msi_bottom_domain) {
+		dev_info(dev, "failed to create MSI bottom domain\n");
+		ret = -ENODEV;
+		goto err_msi_bottom_domain;
+	}
+
+	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
+	if (!info) {
+		ret = -ENOMEM;
+		goto err_msi_bottom_domain;
+	}
+
+	memcpy(info, &mtk_msi_domain_info, sizeof(*info));
+	info->chip_data = port;
+
+	port->msi_domain = pci_msi_create_irq_domain(fwnode, info,
+						     port->msi_bottom_domain);
+	if (!port->msi_domain) {
+		dev_info(dev, "failed to create MSI domain\n");
+		ret = -ENODEV;
+		goto err_msi_domain;
+	}
+
+	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
+		struct mtk_msi_set *msi_set = &port->msi_sets[i];
+
+		msi_set->dev = port->dev;
+		msi_set->base = port->base + PCIE_MSI_SET_BASE_REG +
+				i * PCIE_MSI_SET_OFFSET;
+		msi_set->msg_addr = port->reg_base + PCIE_MSI_SET_BASE_REG +
+				    i * PCIE_MSI_SET_OFFSET;
+
+		writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
+		writel_relaxed(upper_32_bits(msi_set->msg_addr),
+			       port->base + PCIE_MSI_SET_ADDR_HI_BASE +
+			       i * PCIE_MSI_SET_ADDR_HI_OFFSET);
+	}
+
 	return 0;
+
+err_msi_domain:
+	irq_domain_remove(port->msi_bottom_domain);
+err_msi_bottom_domain:
+	irq_domain_remove(port->intx_domain);
+
+	return ret;
 }
 
 static void mtk_pcie_irq_teardown(struct mtk_pcie_port *port)
@@ -375,9 +600,34 @@ static void mtk_pcie_irq_teardown(struct mtk_pcie_port *port)
 	if (port->intx_domain)
 		irq_domain_remove(port->intx_domain);
 
+	if (port->msi_domain)
+		irq_domain_remove(port->msi_domain);
+
+	if (port->msi_bottom_domain)
+		irq_domain_remove(port->msi_bottom_domain);
+
 	irq_dispose_mapping(port->irq);
 }
 
+static void mtk_pcie_msi_handler(struct mtk_pcie_port *port, int set_idx)
+{
+	struct mtk_msi_set *msi_set = &port->msi_sets[set_idx];
+	unsigned long msi_enable, msi_status;
+	unsigned int virq;
+	irq_hw_number_t bit, hwirq;
+
+	msi_enable = readl_relaxed(msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
+	while ((msi_status =
+		readl_relaxed(msi_set->base + PCIE_MSI_SET_STATUS_OFFSET) &
+			      msi_enable)) {
+		for_each_set_bit(bit, &msi_status, PCIE_MSI_IRQS_PER_SET) {
+			hwirq = bit + set_idx * PCIE_MSI_IRQS_PER_SET;
+			virq = irq_find_mapping(port->msi_bottom_domain, hwirq);
+			generic_handle_irq(virq);
+		}
+	}
+}
+
 static void mtk_pcie_irq_handler(struct irq_desc *desc)
 {
 	struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);
@@ -398,6 +648,17 @@ static void mtk_pcie_irq_handler(struct irq_desc *desc)
 		}
 	}
 
+	if (status & PCIE_MSI_ENABLE) {
+		irq_bit = PCIE_MSI_SHIFT;
+		for_each_set_bit_from(irq_bit, &status, PCIE_MSI_SET_NUM +
+				      PCIE_MSI_SHIFT) {
+			mtk_pcie_msi_handler(port, irq_bit - PCIE_MSI_SHIFT);
+
+			writel_relaxed(BIT(irq_bit),
+				       port->base + PCIE_INT_STATUS_REG);
+		}
+	}
+
 	chained_irq_exit(irqchip, desc);
 }
 
-- 
2.25.1

[v7,6/7] PCI: mediatek-gen3: Add system PM support

[Jianjun Wang]

Add suspend_noirq and resume_noirq callback functions to implement
PM system suspend hooks for MediaTek Gen3 PCIe controller.

When system suspend, trigger the PCIe link to L2 state and pull down
the PERST# pin, gating the clocks of MAC layer and power off the
physical layer for the sake of power saving.

When system resum, the PCIe link should be re-established and the
related control register values should be restored.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 78 +++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 152820f28da1..ac6c43cea575 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -49,6 +49,9 @@
 #define PCIE_PE_RSTB			BIT(3)
 
 #define PCIE_LTSSM_STATUS_REG		0x150
+#define PCIE_LTSSM_STATE_MASK		GENMASK(28, 24)
+#define PCIE_LTSSM_STATE(val)		((val & PCIE_LTSSM_STATE_MASK) >> 24)
+#define PCIE_LTSSM_STATE_L2_IDLE	0x14
 
 #define PCIE_LINK_STATUS_REG		0x154
 #define PCIE_PORT_LINKUP		BIT(8)
@@ -76,6 +79,9 @@
 #define PCIE_MSI_SET_ADDR_HI_BASE	0xc80
 #define PCIE_MSI_SET_ADDR_HI_OFFSET	0x04
 
+#define PCIE_ICMD_PM_REG		0x198
+#define PCIE_TURN_OFF_LINK		BIT(4)
+
 #define PCIE_TRANS_TABLE_BASE_REG	0x800
 #define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
 #define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8
@@ -869,6 +875,77 @@ static int mtk_pcie_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int __maybe_unused mtk_pcie_turn_off_link(struct mtk_pcie_port *port)
+{
+	u32 val;
+
+	val = readl_relaxed(port->base + PCIE_ICMD_PM_REG);
+	val |= PCIE_TURN_OFF_LINK;
+	writel_relaxed(val, port->base + PCIE_ICMD_PM_REG);
+
+	/* Check the link is L2 */
+	return readl_poll_timeout(port->base + PCIE_LTSSM_STATUS_REG, val,
+				  (PCIE_LTSSM_STATE(val) ==
+				   PCIE_LTSSM_STATE_L2_IDLE), 20,
+				   50 * USEC_PER_MSEC);
+}
+
+static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
+{
+	struct mtk_pcie_port *port = dev_get_drvdata(dev);
+	int err;
+	u32 val;
+
+	/* Trigger link to L2 state */
+	err = mtk_pcie_turn_off_link(port);
+	if (err) {
+		dev_err(port->dev, "can not enter L2 state\n");
+		return err;
+	}
+
+	/* Pull down the PERST# pin */
+	val = readl_relaxed(port->base + PCIE_RST_CTRL_REG);
+	val |= PCIE_PE_RSTB;
+	writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
+
+	dev_dbg(port->dev, "enter L2 state success");
+
+	clk_bulk_disable_unprepare(port->num_clks, port->clks);
+
+	phy_power_off(port->phy);
+
+	return 0;
+}
+
+static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
+{
+	struct mtk_pcie_port *port = dev_get_drvdata(dev);
+	int err;
+
+	phy_power_on(port->phy);
+
+	err = clk_bulk_prepare_enable(port->num_clks, port->clks);
+	if (err) {
+		dev_dbg(dev, "failed to enable PCIe clocks\n");
+		return err;
+	}
+
+	err = mtk_pcie_startup_port(port);
+	if (err) {
+		dev_err(port->dev, "resume failed\n");
+		return err;
+	}
+
+	dev_dbg(port->dev, "resume done\n");
+
+	return 0;
+}
+
+static const struct dev_pm_ops mtk_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
+				      mtk_pcie_resume_noirq)
+};
+
 static const struct of_device_id mtk_pcie_of_match[] = {
 	{ .compatible = "mediatek,mt8192-pcie" },
 	{},
@@ -880,6 +957,7 @@ static struct platform_driver mtk_pcie_driver = {
 	.driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_of_match,
+		.pm = &mtk_pcie_pm_ops,
 	},
 };
 
-- 
2.25.1

[v7,7/7] MAINTAINERS: Add Jianjun Wang as MediaTek PCI co-maintainer

[Jianjun Wang]

Update entry for MediaTek PCIe controller, add Jianjun Wang
as MediaTek PCI co-maintainer.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e73636b75f29..1a033812c7f9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13622,6 +13622,7 @@ F:	drivers/pci/controller/dwc/pcie-histb.c
 
 PCIE DRIVER FOR MEDIATEK
 M:	Ryder Lee <ryder.lee@mediatek.com>
+M:	Jianjun Wang <jianjun.wang@mediatek.com>
 L:	linux-pci@vger.kernel.org
 L:	linux-mediatek@lists.infradead.org
 S:	Supported
-- 
2.25.1

Re: [v7,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192

[Rob Herring]

On Wed, Jan 13, 2021 at 07:39:57PM +0800, Jianjun Wang wrote:
> MediaTek's PCIe host controller has three generation HWs, the new
> generation HW is an individual bridge, it supports Gen3 speed and
> compatible with Gen2, Gen1 speed.
> 
> Add support for new Gen3 controller which can be found on MT8192.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  drivers/pci/controller/Kconfig              |  13 +
>  drivers/pci/controller/Makefile             |   1 +
>  drivers/pci/controller/pcie-mediatek-gen3.c | 463 ++++++++++++++++++++
>  3 files changed, 477 insertions(+)
>  create mode 100644 drivers/pci/controller/pcie-mediatek-gen3.c
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 64e2f5e379aa..b242b17025b3 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -242,6 +242,19 @@ config PCIE_MEDIATEK
>  	  Say Y here if you want to enable PCIe controller support on
>  	  MediaTek SoCs.
>  
> +config PCIE_MEDIATEK_GEN3
> +	tristate "MediaTek Gen3 PCIe controller"
> +	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	depends on PCI_MSI_IRQ_DOMAIN
> +	help
> +	  Adds support for PCIe Gen3 MAC controller for MediaTek SoCs.
> +	  This PCIe controller is compatible with Gen3, Gen2 and Gen1 speed,
> +	  and support up to 256 MSI interrupt numbers for
> +	  multi-function devices.
> +
> +	  Say Y here if you want to enable Gen3 PCIe controller support on
> +	  MediaTek SoCs.
> +
>  config PCIE_TANGO_SMP8759
>  	bool "Tango SMP8759 PCIe controller (DANGEROUS)"
>  	depends on ARCH_TANGO && PCI_MSI && OF
> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
> index 04c6edc285c5..df5d77d72a9d 100644
> --- a/drivers/pci/controller/Makefile
> +++ b/drivers/pci/controller/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
>  obj-$(CONFIG_PCIE_ROCKCHIP_EP) += pcie-rockchip-ep.o
>  obj-$(CONFIG_PCIE_ROCKCHIP_HOST) += pcie-rockchip-host.o
>  obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o
> +obj-$(CONFIG_PCIE_MEDIATEK_GEN3) += pcie-mediatek-gen3.o
>  obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o
>  obj-$(CONFIG_VMD) += vmd.o
>  obj-$(CONFIG_PCIE_BRCMSTB) += pcie-brcmstb.o
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> new file mode 100644
> index 000000000000..c00ea7c167de
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -0,0 +1,463 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MediaTek PCIe host controller driver.
> + *
> + * Copyright (c) 2020 MediaTek Inc.
> + * Author: Jianjun Wang <jianjun.wang@mediatek.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>

> +#include <linux/of_address.h>
> +#include <linux/of_clk.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>

I don't think these 4 are needed.

> +#include <linux/pci.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +
> +#include "../pci.h"
> +
> +#define PCIE_SETTING_REG		0x80
> +#define PCIE_PCI_IDS_1			0x9c
> +#define PCI_CLASS(class)		(class << 8)
> +#define PCIE_RC_MODE			BIT(0)
> +
> +#define PCIE_CFGNUM_REG			0x140
> +#define PCIE_CFG_DEVFN(devfn)		((devfn) & GENMASK(7, 0))
> +#define PCIE_CFG_BUS(bus)		(((bus) << 8) & GENMASK(15, 8))
> +#define PCIE_CFG_BYTE_EN(bytes)		(((bytes) << 16) & GENMASK(19, 16))
> +#define PCIE_CFG_FORCE_BYTE_EN		BIT(20)
> +#define PCIE_CFG_OFFSET_ADDR		0x1000
> +#define PCIE_CFG_HEADER(bus, devfn) \
> +	(PCIE_CFG_BUS(bus) | PCIE_CFG_DEVFN(devfn))
> +
> +#define PCIE_RST_CTRL_REG		0x148
> +#define PCIE_MAC_RSTB			BIT(0)
> +#define PCIE_PHY_RSTB			BIT(1)
> +#define PCIE_BRG_RSTB			BIT(2)
> +#define PCIE_PE_RSTB			BIT(3)
> +
> +#define PCIE_LTSSM_STATUS_REG		0x150
> +
> +#define PCIE_LINK_STATUS_REG		0x154
> +#define PCIE_PORT_LINKUP		BIT(8)
> +
> +#define PCIE_TRANS_TABLE_BASE_REG	0x800
> +#define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
> +#define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8
> +#define PCIE_ATR_TRSL_ADDR_MSB_OFFSET	0xc
> +#define PCIE_ATR_TRSL_PARAM_OFFSET	0x10
> +#define PCIE_ATR_TLB_SET_OFFSET		0x20
> +
> +#define PCIE_MAX_TRANS_TABLES		8
> +#define PCIE_ATR_EN			BIT(0)
> +#define PCIE_ATR_SIZE(size) \
> +	(((((size) - 1) << 1) & GENMASK(6, 1)) | PCIE_ATR_EN)
> +#define PCIE_ATR_ID(id)			((id) & GENMASK(3, 0))
> +#define PCIE_ATR_TYPE_MEM		PCIE_ATR_ID(0)
> +#define PCIE_ATR_TYPE_IO		PCIE_ATR_ID(1)
> +#define PCIE_ATR_TLP_TYPE(type)		(((type) << 16) & GENMASK(18, 16))
> +#define PCIE_ATR_TLP_TYPE_MEM		PCIE_ATR_TLP_TYPE(0)
> +#define PCIE_ATR_TLP_TYPE_IO		PCIE_ATR_TLP_TYPE(2)
> +
> +/**
> + * struct mtk_pcie_port - PCIe port information
> + * @dev: pointer to PCIe device
> + * @base: IO mapped register base
> + * @reg_base: Physical register base
> + * @mac_reset: mac reset control
> + * @phy_reset: phy reset control
> + * @phy: PHY controller block
> + * @clks: PCIe clocks
> + * @num_clks: PCIe clocks count for this port
> + */
> +struct mtk_pcie_port {
> +	struct device *dev;
> +	void __iomem *base;
> +	phys_addr_t reg_base;
> +	struct reset_control *mac_reset;
> +	struct reset_control *phy_reset;
> +	struct phy *phy;
> +	struct clk_bulk_data *clks;
> +	int num_clks;
> +};
> +
> +/**
> + * mtk_pcie_config_tlp_header
> + * @bus: PCI bus to query
> + * @devfn: device/function number
> + * @where: offset in config space
> + * @size: data size in TLP header
> + *
> + * Set byte enable field and device information in configuration TLP header.
> + */
> +static void mtk_pcie_config_tlp_header(struct pci_bus *bus, unsigned int devfn,
> +					int where, int size)
> +{
> +	struct mtk_pcie_port *port = bus->sysdata;
> +	int bytes;
> +	u32 val;
> +
> +	bytes = (GENMASK(size - 1, 0) & 0xf) << (where & 0x3);
> +
> +	val = PCIE_CFG_FORCE_BYTE_EN | PCIE_CFG_BYTE_EN(bytes) |
> +	      PCIE_CFG_HEADER(bus->number, devfn);
> +
> +	writel_relaxed(val, port->base + PCIE_CFGNUM_REG);
> +}
> +
> +static void __iomem *mtk_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> +				      int where)
> +{
> +	struct mtk_pcie_port *port = bus->sysdata;
> +
> +	return port->base + PCIE_CFG_OFFSET_ADDR + where;
> +}
> +
> +static int mtk_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
> +				int where, int size, u32 *val)
> +{
> +	mtk_pcie_config_tlp_header(bus, devfn, where, size);

This can move to mtk_pcie_map_bus. And if that's the only caller, you 
might as well just merge mtk_pcie_config_tlp_header into 
mtk_pcie_map_bus.

> +
> +	return pci_generic_config_read32(bus, devfn, where, size, val);
> +}
> +
> +static int mtk_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
> +				 int where, int size, u32 val)
> +{
> +	mtk_pcie_config_tlp_header(bus, devfn, where, size);
> +
> +	if (size <= 2)
> +		val <<= (where & 0x3) * 8;
> +
> +	return pci_generic_config_write32(bus, devfn, where, 4, val);
> +}
> +
> +static struct pci_ops mtk_pcie_ops = {
> +	.map_bus = mtk_pcie_map_bus,
> +	.read  = mtk_pcie_config_read,
> +	.write = mtk_pcie_config_write,
> +};
> +
> +static int mtk_pcie_set_trans_table(struct mtk_pcie_port *port,
> +				    resource_size_t cpu_addr,
> +				    resource_size_t pci_addr,
> +				    resource_size_t size,
> +				    unsigned long type, int num)
> +{
> +	void __iomem *table;
> +	u32 val;
> +
> +	if (num >= PCIE_MAX_TRANS_TABLES) {
> +		dev_err(port->dev, "not enough translate table[%d] for addr: %#llx, limited to [%d]\n",
> +			num, (unsigned long long) cpu_addr,
> +			PCIE_MAX_TRANS_TABLES);
> +		return -ENODEV;
> +	}
> +
> +	table = port->base + PCIE_TRANS_TABLE_BASE_REG +
> +		num * PCIE_ATR_TLB_SET_OFFSET;
> +
> +	writel_relaxed(lower_32_bits(cpu_addr) | PCIE_ATR_SIZE(fls(size) - 1),
> +		       table);
> +	writel_relaxed(upper_32_bits(cpu_addr),
> +		       table + PCIE_ATR_SRC_ADDR_MSB_OFFSET);
> +	writel_relaxed(lower_32_bits(pci_addr),
> +		       table + PCIE_ATR_TRSL_ADDR_LSB_OFFSET);
> +	writel_relaxed(upper_32_bits(pci_addr),
> +		       table + PCIE_ATR_TRSL_ADDR_MSB_OFFSET);
> +
> +	if (type == IORESOURCE_IO)
> +		val = PCIE_ATR_TYPE_IO | PCIE_ATR_TLP_TYPE_IO;
> +	else
> +		val = PCIE_ATR_TYPE_MEM | PCIE_ATR_TLP_TYPE_MEM;
> +
> +	writel_relaxed(val, table + PCIE_ATR_TRSL_PARAM_OFFSET);
> +
> +	return 0;
> +}
> +
> +static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
> +{
> +	struct resource_entry *entry;
> +	struct pci_host_bridge *host = pci_host_bridge_from_priv(port);
> +	unsigned int table_index = 0;
> +	int err;
> +	u32 val;
> +
> +	/* Set as RC mode */
> +	val = readl_relaxed(port->base + PCIE_SETTING_REG);
> +	val |= PCIE_RC_MODE;
> +	writel_relaxed(val, port->base + PCIE_SETTING_REG);
> +
> +	/* Set class code */
> +	val = readl_relaxed(port->base + PCIE_PCI_IDS_1);
> +	val &= ~GENMASK(31, 8);
> +	val |= PCI_CLASS(PCI_CLASS_BRIDGE_PCI << 8);
> +	writel_relaxed(val, port->base + PCIE_PCI_IDS_1);
> +
> +	/* Assert all reset signals */
> +	val = readl_relaxed(port->base + PCIE_RST_CTRL_REG);
> +	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> +	writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
> +
> +	/* De-assert reset signals */
> +	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB);
> +	writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
> +
> +	/* Delay 100ms to wait the reference clocks become stable */
> +	msleep(100);
> +
> +	/* De-assert PERST# signal */
> +	val &= ~PCIE_PE_RSTB;
> +	writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
> +
> +	/* Check if the link is up or not */
> +	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val,
> +				 !!(val & PCIE_PORT_LINKUP), 20,
> +				 50 * USEC_PER_MSEC);
> +	if (err) {
> +		val = readl_relaxed(port->base + PCIE_LTSSM_STATUS_REG);
> +		dev_err(port->dev, "PCIe link down, ltssm reg val: %#x\n", val);
> +		return err;
> +	}
> +
> +	/* Set PCIe translation windows */
> +	resource_list_for_each_entry(entry, &host->windows) {
> +		struct resource *res = entry->res;
> +		unsigned long type = resource_type(res);
> +		resource_size_t cpu_addr;
> +		resource_size_t pci_addr;
> +		resource_size_t size;
> +		const char *range_type;
> +
> +		if (type == IORESOURCE_IO) {
> +			cpu_addr = pci_pio_to_address(res->start);
> +			range_type = "IO";
> +		} else if (type == IORESOURCE_MEM) {
> +			cpu_addr = res->start;
> +			range_type = "MEM";
> +		} else {
> +			continue;
> +		}
> +
> +		pci_addr = res->start - entry->offset;
> +		size = resource_size(res);
> +		err = mtk_pcie_set_trans_table(port, cpu_addr, pci_addr, size,
> +					       type, table_index);
> +		if (err)
> +			return err;
> +
> +		dev_dbg(port->dev, "set %s trans window[%d]: cpu_addr = %#llx, pci_addr = %#llx, size = %#llx\n",
> +			range_type, table_index, (unsigned long long) cpu_addr,
> +			(unsigned long long) pci_addr,
> +			(unsigned long long) size);
> +
> +		table_index++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_pcie_clk_init(struct mtk_pcie_port *port)
> +{
> +	int ret;
> +
> +	port->num_clks = devm_clk_bulk_get_all(port->dev, &port->clks);
> +	if (port->num_clks < 0) {
> +		dev_err(port->dev, "failed to get PCIe clock\n");
> +		return port->num_clks;
> +	}
> +
> +	ret = clk_bulk_prepare_enable(port->num_clks, port->clks);
> +	if (ret) {
> +		dev_err(port->dev, "failed to enable PCIe clocks\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_pcie_power_up(struct mtk_pcie_port *port)
> +{
> +	struct device *dev = port->dev;
> +	int err;
> +
> +	port->phy_reset = devm_reset_control_get_optional_exclusive(dev, "phy");
> +	if (IS_ERR(port->phy_reset))
> +		return PTR_ERR(port->phy_reset);
> +
> +	/* PHY power on and enable pipe clock */
> +	port->phy = devm_phy_optional_get(dev, "pcie-phy");
> +	if (IS_ERR(port->phy))
> +		return PTR_ERR(port->phy);
> +
> +	reset_control_deassert(port->phy_reset);
> +
> +	err = phy_init(port->phy);
> +	if (err) {
> +		dev_err(dev, "failed to initialize PCIe phy\n");
> +		goto err_phy_init;
> +	}
> +
> +	err = phy_power_on(port->phy);
> +	if (err) {
> +		dev_err(dev, "failed to power on PCIe phy\n");
> +		goto err_phy_on;
> +	}
> +
> +	port->mac_reset = devm_reset_control_get_optional_exclusive(dev, "mac");
> +	if (IS_ERR(port->mac_reset)) {
> +		err = PTR_ERR(port->mac_reset);
> +		goto err_mac_rst;
> +	}
> +
> +	reset_control_deassert(port->mac_reset);
> +
> +	/* MAC power on and enable transaction layer clocks */
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);
> +
> +	err = mtk_pcie_clk_init(port);
> +	if (err) {
> +		dev_err(dev, "clock init failed\n");
> +		goto err_clk_init;
> +	}
> +
> +	return 0;
> +
> +err_clk_init:
> +	pm_runtime_put_sync(dev);
> +	pm_runtime_disable(dev);
> +	reset_control_assert(port->mac_reset);
> +err_mac_rst:
> +	phy_power_off(port->phy);
> +err_phy_on:
> +	phy_exit(port->phy);
> +err_phy_init:
> +	reset_control_assert(port->phy_reset);
> +
> +	return err;
> +}
> +
> +static void mtk_pcie_power_down(struct mtk_pcie_port *port)
> +{
> +	clk_bulk_disable_unprepare(port->num_clks, port->clks);
> +
> +	pm_runtime_put_sync(port->dev);
> +	pm_runtime_disable(port->dev);
> +	reset_control_assert(port->mac_reset);
> +
> +	phy_power_off(port->phy);
> +	phy_exit(port->phy);
> +	reset_control_assert(port->phy_reset);
> +}
> +
> +static int mtk_pcie_setup(struct mtk_pcie_port *port)
> +{
> +	struct device *dev = port->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct resource *regs;
> +	int err;
> +
> +	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");
> +	port->base = devm_ioremap_resource(dev, regs);

Use devm_platform_ioremap_resource_byname()

> +	if (IS_ERR(port->base)) {
> +		dev_err(dev, "failed to map register base\n");
> +		return PTR_ERR(port->base);
> +	}
> +
> +	port->reg_base = regs->start;
> +
> +	/* Don't touch the hardware registers before power up */
> +	err = mtk_pcie_power_up(port);
> +	if (err)
> +		return err;
> +
> +	/* Try link up */
> +	err = mtk_pcie_startup_port(port);
> +	if (err) {
> +		dev_err(dev, "PCIe startup failed\n");
> +		goto err_setup;
> +	}
> +
> +	dev_info(dev, "PCIe link up success!\n");

Do you really need this, there's lots printed by PCI in the successful 
case.

> +
> +	return 0;
> +
> +err_setup:
> +	mtk_pcie_power_down(port);
> +
> +	return err;
> +}
> +
> +static int mtk_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mtk_pcie_port *port;
> +	struct pci_host_bridge *host;
> +	int err;
> +
> +	host = devm_pci_alloc_host_bridge(dev, sizeof(*port));
> +	if (!host)
> +		return -ENOMEM;
> +
> +	port = pci_host_bridge_priv(host);
> +
> +	port->dev = dev;
> +	platform_set_drvdata(pdev, port);
> +
> +	err = mtk_pcie_setup(port);
> +	if (err)
> +		return err;
> +
> +	host->ops = &mtk_pcie_ops;
> +	host->sysdata = port;
> +
> +	err = pci_host_probe(host);
> +	if (err) {
> +		mtk_pcie_power_down(port);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_pcie_remove(struct platform_device *pdev)
> +{
> +	struct mtk_pcie_port *port = platform_get_drvdata(pdev);
> +	struct pci_host_bridge *host = pci_host_bridge_from_priv(port);
> +
> +	pci_lock_rescan_remove();
> +	pci_stop_root_bus(host->bus);
> +	pci_remove_root_bus(host->bus);
> +	pci_unlock_rescan_remove();
> +
> +	mtk_pcie_power_down(port);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mtk_pcie_of_match[] = {
> +	{ .compatible = "mediatek,mt8192-pcie" },
> +	{},
> +};
> +
> +static struct platform_driver mtk_pcie_driver = {
> +	.probe = mtk_pcie_probe,
> +	.remove = mtk_pcie_remove,
> +	.driver = {
> +		.name = "mtk-pcie",
> +		.of_match_table = mtk_pcie_of_match,
> +	},
> +};
> +
> +module_platform_driver(mtk_pcie_driver);
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.25.1
> 

Re: [v7,1/7] dt-bindings: PCI: mediatek-gen3: Add YAML schema

[Rob Herring]

On Wed, Jan 13, 2021 at 07:39:55PM +0800, Jianjun Wang wrote:
> Add YAML schemas documentation for Gen3 PCIe controller on
> MediaTek SoCs.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  .../bindings/pci/mediatek-pcie-gen3.yaml      | 172 ++++++++++++++++++
>  1 file changed, 172 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> new file mode 100644
> index 000000000000..f133fb0184f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> @@ -0,0 +1,172 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/mediatek-pcie-gen3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Gen3 PCIe controller on MediaTek SoCs
> +
> +maintainers:
> +  - Jianjun Wang <jianjun.wang@mediatek.com>
> +
> +description: |+
> +  PCIe Gen3 MAC controller for MediaTek SoCs, it supports Gen3 speed
> +  and compatible with Gen2, Gen1 speed.
> +
> +  This PCIe controller supports up to 256 MSI vectors, the MSI hardware
> +  block diagram is as follows:
> +
> +                    +-----+
> +                    | GIC |
> +                    +-----+
> +                       ^
> +                       |
> +                   port->irq
> +                       |
> +               +-+-+-+-+-+-+-+-+
> +               |0|1|2|3|4|5|6|7| (PCIe intc)
> +               +-+-+-+-+-+-+-+-+
> +                ^ ^           ^
> +                | |    ...    |
> +        +-------+ +------+    +-----------+
> +        |                |                |
> +  +-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
> +  |0|1|...|30|31|  |0|1|...|30|31|  |0|1|...|30|31| (MSI sets)
> +  +-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
> +   ^ ^      ^  ^    ^ ^      ^  ^    ^ ^      ^  ^
> +   | |      |  |    | |      |  |    | |      |  |  (MSI vectors)
> +   | |      |  |    | |      |  |    | |      |  |
> +
> +    (MSI SET0)       (MSI SET1)  ...   (MSI SET7)
> +
> +  With 256 MSI vectors supported, the MSI vectors are composed of 8 sets,
> +  each set has its own address for MSI message, and supports 32 MSI vectors
> +  to generate interrupt.
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +
> +properties:
> +  compatible:
> +    const: mediatek,mt8192-pcie
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  ranges:
> +    minItems: 1
> +    maxItems: 8
> +
> +  resets:
> +    minItems: 1
> +    maxItems: 2

Why the range? The SoC either has the reset lines or it doesn't.

> +
> +  reset-names:
> +    anyOf:
> +      - const: mac
> +      - const: phy

I don't think this should stay, but if so, better expressed like this:

minItems: 1
maxItems: 2
items:
  enum: [ mac, phy ]


> +
> +  clocks:
> +    maxItems: 6
> +
> +  clock-names:
> +    items:
> +      - const: pl_250m
> +      - const: tl_26m
> +      - const: tl_96m
> +      - const: tl_32k
> +      - const: peri_26m
> +      - const: top_133m
> +
> +  assigned-clocks:
> +    maxItems: 1
> +
> +  assigned-clock-parents:
> +    maxItems: 1
> +
> +  phys:
> +    maxItems: 1
> +
> +  '#interrupt-cells':
> +    const: 1
> +
> +  interrupt-controller:
> +    description: Interrupt controller node for handling legacy PCI interrupts.
> +    type: object
> +    properties:
> +      '#address-cells':
> +        const: 0
> +      '#interrupt-cells':
> +        const: 1
> +      interrupt-controller: true
> +
> +    required:
> +      - '#address-cells'
> +      - '#interrupt-cells'
> +      - interrupt-controller
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - ranges
> +  - clocks
> +  - '#interrupt-cells'
> +  - interrupt-controller
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pcie: pcie@11230000 {
> +            compatible = "mediatek,mt8192-pcie";
> +            device_type = "pci";
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +            reg = <0x00 0x11230000 0x00 0x4000>;
> +            reg-names = "pcie-mac";

Not documented. Drop.

> +            interrupts = <GIC_SPI 251 IRQ_TYPE_LEVEL_HIGH 0>;
> +            bus-range = <0x00 0xff>;
> +            ranges = <0x82000000 0x00 0x12000000 0x00
> +                      0x12000000 0x00 0x1000000>;
> +            clocks = <&infracfg 44>,
> +                     <&infracfg 40>,
> +                     <&infracfg 43>,
> +                     <&infracfg 97>,
> +                     <&infracfg 99>,
> +                     <&infracfg 111>;
> +            clock-names = "pl_250m", "tl_26m", "tl_96m",
> +                          "tl_32k", "peri_26m", "top_133m";
> +            assigned-clocks = <&topckgen 50>;
> +            assigned-clock-parents = <&topckgen 91>;
> +
> +            phys = <&pciephy>;
> +            phy-names = "pcie-phy";
> +            resets = <&infracfg_rst 0>;
> +            reset-names = "phy";
> +
> +            #interrupt-cells = <1>;
> +            interrupt-map-mask = <0 0 0 0x7>;
> +            interrupt-map = <0 0 0 1 &pcie_intc 0>,
> +                            <0 0 0 2 &pcie_intc 1>,
> +                            <0 0 0 3 &pcie_intc 2>,
> +                            <0 0 0 4 &pcie_intc 3>;
> +            pcie_intc: interrupt-controller {
> +                      #address-cells = <0>;
> +                      #interrupt-cells = <1>;
> +                      interrupt-controller;
> +            };
> +        };
> +    };
> -- 
> 2.25.1
> 

Re: [v7,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192

[Jianjun Wang]

On Mon, 2021-01-25 at 13:54 -0600, Rob Herring wrote:
> On Wed, Jan 13, 2021 at 07:39:57PM +0800, Jianjun Wang wrote:
> > MediaTek's PCIe host controller has three generation HWs, the new
> > generation HW is an individual bridge, it supports Gen3 speed and
> > compatible with Gen2, Gen1 speed.
> > 
> > Add support for new Gen3 controller which can be found on MT8192.
> > 
> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  drivers/pci/controller/Kconfig              |  13 +
> >  drivers/pci/controller/Makefile             |   1 +
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 463 ++++++++++++++++++++
> >  3 files changed, 477 insertions(+)
> >  create mode 100644 drivers/pci/controller/pcie-mediatek-gen3.c
> > 
> > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > index 64e2f5e379aa..b242b17025b3 100644
> > --- a/drivers/pci/controller/Kconfig
> > +++ b/drivers/pci/controller/Kconfig
> > @@ -242,6 +242,19 @@ config PCIE_MEDIATEK
> >  	  Say Y here if you want to enable PCIe controller support on
> >  	  MediaTek SoCs.
> >  
> > +config PCIE_MEDIATEK_GEN3
> > +	tristate "MediaTek Gen3 PCIe controller"
> > +	depends on ARCH_MEDIATEK || COMPILE_TEST
> > +	depends on PCI_MSI_IRQ_DOMAIN
> > +	help
> > +	  Adds support for PCIe Gen3 MAC controller for MediaTek SoCs.
> > +	  This PCIe controller is compatible with Gen3, Gen2 and Gen1 speed,
> > +	  and support up to 256 MSI interrupt numbers for
> > +	  multi-function devices.
> > +
> > +	  Say Y here if you want to enable Gen3 PCIe controller support on
> > +	  MediaTek SoCs.
> > +
> >  config PCIE_TANGO_SMP8759
> >  	bool "Tango SMP8759 PCIe controller (DANGEROUS)"
> >  	depends on ARCH_TANGO && PCI_MSI && OF
> > diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
> > index 04c6edc285c5..df5d77d72a9d 100644
> > --- a/drivers/pci/controller/Makefile
> > +++ b/drivers/pci/controller/Makefile
> > @@ -27,6 +27,7 @@ obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
> >  obj-$(CONFIG_PCIE_ROCKCHIP_EP) += pcie-rockchip-ep.o
> >  obj-$(CONFIG_PCIE_ROCKCHIP_HOST) += pcie-rockchip-host.o
> >  obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o
> > +obj-$(CONFIG_PCIE_MEDIATEK_GEN3) += pcie-mediatek-gen3.o
> >  obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o
> >  obj-$(CONFIG_VMD) += vmd.o
> >  obj-$(CONFIG_PCIE_BRCMSTB) += pcie-brcmstb.o
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > new file mode 100644
> > index 000000000000..c00ea7c167de
> > --- /dev/null
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -0,0 +1,463 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * MediaTek PCIe host controller driver.
> > + *
> > + * Copyright (c) 2020 MediaTek Inc.
> > + * Author: Jianjun Wang <jianjun.wang@mediatek.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> 
> > +#include <linux/of_address.h>
> > +#include <linux/of_clk.h>
> > +#include <linux/of_pci.h>
> > +#include <linux/of_platform.h>
> 
> I don't think these 4 are needed.

Yes, I will drop these in the next version.
> 
> > +#include <linux/pci.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/reset.h>
> > +
> > +#include "../pci.h"
> > +
> > +#define PCIE_SETTING_REG		0x80
> > +#define PCIE_PCI_IDS_1			0x9c
> > +#define PCI_CLASS(class)		(class << 8)
> > +#define PCIE_RC_MODE			BIT(0)
> > +
> > +#define PCIE_CFGNUM_REG			0x140
> > +#define PCIE_CFG_DEVFN(devfn)		((devfn) & GENMASK(7, 0))
> > +#define PCIE_CFG_BUS(bus)		(((bus) << 8) & GENMASK(15, 8))
> > +#define PCIE_CFG_BYTE_EN(bytes)		(((bytes) << 16) & GENMASK(19, 16))
> > +#define PCIE_CFG_FORCE_BYTE_EN		BIT(20)
> > +#define PCIE_CFG_OFFSET_ADDR		0x1000
> > +#define PCIE_CFG_HEADER(bus, devfn) \
> > +	(PCIE_CFG_BUS(bus) | PCIE_CFG_DEVFN(devfn))
> > +
> > +#define PCIE_RST_CTRL_REG		0x148
> > +#define PCIE_MAC_RSTB			BIT(0)
> > +#define PCIE_PHY_RSTB			BIT(1)
> > +#define PCIE_BRG_RSTB			BIT(2)
> > +#define PCIE_PE_RSTB			BIT(3)
> > +
> > +#define PCIE_LTSSM_STATUS_REG		0x150
> > +
> > +#define PCIE_LINK_STATUS_REG		0x154
> > +#define PCIE_PORT_LINKUP		BIT(8)
> > +
> > +#define PCIE_TRANS_TABLE_BASE_REG	0x800
> > +#define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
> > +#define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8
> > +#define PCIE_ATR_TRSL_ADDR_MSB_OFFSET	0xc
> > +#define PCIE_ATR_TRSL_PARAM_OFFSET	0x10
> > +#define PCIE_ATR_TLB_SET_OFFSET		0x20
> > +
> > +#define PCIE_MAX_TRANS_TABLES		8
> > +#define PCIE_ATR_EN			BIT(0)
> > +#define PCIE_ATR_SIZE(size) \
> > +	(((((size) - 1) << 1) & GENMASK(6, 1)) | PCIE_ATR_EN)
> > +#define PCIE_ATR_ID(id)			((id) & GENMASK(3, 0))
> > +#define PCIE_ATR_TYPE_MEM		PCIE_ATR_ID(0)
> > +#define PCIE_ATR_TYPE_IO		PCIE_ATR_ID(1)
> > +#define PCIE_ATR_TLP_TYPE(type)		(((type) << 16) & GENMASK(18, 16))
> > +#define PCIE_ATR_TLP_TYPE_MEM		PCIE_ATR_TLP_TYPE(0)
> > +#define PCIE_ATR_TLP_TYPE_IO		PCIE_ATR_TLP_TYPE(2)
> > +
> > +/**
> > + * struct mtk_pcie_port - PCIe port information
> > + * @dev: pointer to PCIe device
> > + * @base: IO mapped register base
> > + * @reg_base: Physical register base
> > + * @mac_reset: mac reset control
> > + * @phy_reset: phy reset control
> > + * @phy: PHY controller block
> > + * @clks: PCIe clocks
> > + * @num_clks: PCIe clocks count for this port
> > + */
> > +struct mtk_pcie_port {
> > +	struct device *dev;
> > +	void __iomem *base;
> > +	phys_addr_t reg_base;
> > +	struct reset_control *mac_reset;
> > +	struct reset_control *phy_reset;
> > +	struct phy *phy;
> > +	struct clk_bulk_data *clks;
> > +	int num_clks;
> > +};
> > +
> > +/**
> > + * mtk_pcie_config_tlp_header
> > + * @bus: PCI bus to query
> > + * @devfn: device/function number
> > + * @where: offset in config space
> > + * @size: data size in TLP header
> > + *
> > + * Set byte enable field and device information in configuration TLP header.
> > + */
> > +static void mtk_pcie_config_tlp_header(struct pci_bus *bus, unsigned int devfn,
> > +					int where, int size)
> > +{
> > +	struct mtk_pcie_port *port = bus->sysdata;
> > +	int bytes;
> > +	u32 val;
> > +
> > +	bytes = (GENMASK(size - 1, 0) & 0xf) << (where & 0x3);
> > +
> > +	val = PCIE_CFG_FORCE_BYTE_EN | PCIE_CFG_BYTE_EN(bytes) |
> > +	      PCIE_CFG_HEADER(bus->number, devfn);
> > +
> > +	writel_relaxed(val, port->base + PCIE_CFGNUM_REG);
> > +}
> > +
> > +static void __iomem *mtk_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> > +				      int where)
> > +{
> > +	struct mtk_pcie_port *port = bus->sysdata;
> > +
> > +	return port->base + PCIE_CFG_OFFSET_ADDR + where;
> > +}
> > +
> > +static int mtk_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
> > +				int where, int size, u32 *val)
> > +{
> > +	mtk_pcie_config_tlp_header(bus, devfn, where, size);
> 
> This can move to mtk_pcie_map_bus. And if that's the only caller, you 
> might as well just merge mtk_pcie_config_tlp_header into 
> mtk_pcie_map_bus.

Thanks for your review, mtk_pcie_config_tlp_header need the TLP data
size to set its byte enable filed, but the map_bus callback dose not
have this value.
> 
> > +
> > +	return pci_generic_config_read32(bus, devfn, where, size, val);
> > +}
> > +
> > +static int mtk_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
> > +				 int where, int size, u32 val)
> > +{
> > +	mtk_pcie_config_tlp_header(bus, devfn, where, size);
> > +
> > +	if (size <= 2)
> > +		val <<= (where & 0x3) * 8;
> > +
> > +	return pci_generic_config_write32(bus, devfn, where, 4, val);
> > +}
> > +
> > +static struct pci_ops mtk_pcie_ops = {
> > +	.map_bus = mtk_pcie_map_bus,
> > +	.read  = mtk_pcie_config_read,
> > +	.write = mtk_pcie_config_write,
> > +};
> > +
> > +static int mtk_pcie_set_trans_table(struct mtk_pcie_port *port,
> > +				    resource_size_t cpu_addr,
> > +				    resource_size_t pci_addr,
> > +				    resource_size_t size,
> > +				    unsigned long type, int num)
> > +{
> > +	void __iomem *table;
> > +	u32 val;
> > +
> > +	if (num >= PCIE_MAX_TRANS_TABLES) {
> > +		dev_err(port->dev, "not enough translate table[%d] for addr: %#llx, limited to [%d]\n",
> > +			num, (unsigned long long) cpu_addr,
> > +			PCIE_MAX_TRANS_TABLES);
> > +		return -ENODEV;
> > +	}
> > +
> > +	table = port->base + PCIE_TRANS_TABLE_BASE_REG +
> > +		num * PCIE_ATR_TLB_SET_OFFSET;
> > +
> > +	writel_relaxed(lower_32_bits(cpu_addr) | PCIE_ATR_SIZE(fls(size) - 1),
> > +		       table);
> > +	writel_relaxed(upper_32_bits(cpu_addr),
> > +		       table + PCIE_ATR_SRC_ADDR_MSB_OFFSET);
> > +	writel_relaxed(lower_32_bits(pci_addr),
> > +		       table + PCIE_ATR_TRSL_ADDR_LSB_OFFSET);
> > +	writel_relaxed(upper_32_bits(pci_addr),
> > +		       table + PCIE_ATR_TRSL_ADDR_MSB_OFFSET);
> > +
> > +	if (type == IORESOURCE_IO)
> > +		val = PCIE_ATR_TYPE_IO | PCIE_ATR_TLP_TYPE_IO;
> > +	else
> > +		val = PCIE_ATR_TYPE_MEM | PCIE_ATR_TLP_TYPE_MEM;
> > +
> > +	writel_relaxed(val, table + PCIE_ATR_TRSL_PARAM_OFFSET);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
> > +{
> > +	struct resource_entry *entry;
> > +	struct pci_host_bridge *host = pci_host_bridge_from_priv(port);
> > +	unsigned int table_index = 0;
> > +	int err;
> > +	u32 val;
> > +
> > +	/* Set as RC mode */
> > +	val = readl_relaxed(port->base + PCIE_SETTING_REG);
> > +	val |= PCIE_RC_MODE;
> > +	writel_relaxed(val, port->base + PCIE_SETTING_REG);
> > +
> > +	/* Set class code */
> > +	val = readl_relaxed(port->base + PCIE_PCI_IDS_1);
> > +	val &= ~GENMASK(31, 8);
> > +	val |= PCI_CLASS(PCI_CLASS_BRIDGE_PCI << 8);
> > +	writel_relaxed(val, port->base + PCIE_PCI_IDS_1);
> > +
> > +	/* Assert all reset signals */
> > +	val = readl_relaxed(port->base + PCIE_RST_CTRL_REG);
> > +	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> > +	writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
> > +
> > +	/* De-assert reset signals */
> > +	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB);
> > +	writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
> > +
> > +	/* Delay 100ms to wait the reference clocks become stable */
> > +	msleep(100);
> > +
> > +	/* De-assert PERST# signal */
> > +	val &= ~PCIE_PE_RSTB;
> > +	writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
> > +
> > +	/* Check if the link is up or not */
> > +	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val,
> > +				 !!(val & PCIE_PORT_LINKUP), 20,
> > +				 50 * USEC_PER_MSEC);
> > +	if (err) {
> > +		val = readl_relaxed(port->base + PCIE_LTSSM_STATUS_REG);
> > +		dev_err(port->dev, "PCIe link down, ltssm reg val: %#x\n", val);
> > +		return err;
> > +	}
> > +
> > +	/* Set PCIe translation windows */
> > +	resource_list_for_each_entry(entry, &host->windows) {
> > +		struct resource *res = entry->res;
> > +		unsigned long type = resource_type(res);
> > +		resource_size_t cpu_addr;
> > +		resource_size_t pci_addr;
> > +		resource_size_t size;
> > +		const char *range_type;
> > +
> > +		if (type == IORESOURCE_IO) {
> > +			cpu_addr = pci_pio_to_address(res->start);
> > +			range_type = "IO";
> > +		} else if (type == IORESOURCE_MEM) {
> > +			cpu_addr = res->start;
> > +			range_type = "MEM";
> > +		} else {
> > +			continue;
> > +		}
> > +
> > +		pci_addr = res->start - entry->offset;
> > +		size = resource_size(res);
> > +		err = mtk_pcie_set_trans_table(port, cpu_addr, pci_addr, size,
> > +					       type, table_index);
> > +		if (err)
> > +			return err;
> > +
> > +		dev_dbg(port->dev, "set %s trans window[%d]: cpu_addr = %#llx, pci_addr = %#llx, size = %#llx\n",
> > +			range_type, table_index, (unsigned long long) cpu_addr,
> > +			(unsigned long long) pci_addr,
> > +			(unsigned long long) size);
> > +
> > +		table_index++;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_pcie_clk_init(struct mtk_pcie_port *port)
> > +{
> > +	int ret;
> > +
> > +	port->num_clks = devm_clk_bulk_get_all(port->dev, &port->clks);
> > +	if (port->num_clks < 0) {
> > +		dev_err(port->dev, "failed to get PCIe clock\n");
> > +		return port->num_clks;
> > +	}
> > +
> > +	ret = clk_bulk_prepare_enable(port->num_clks, port->clks);
> > +	if (ret) {
> > +		dev_err(port->dev, "failed to enable PCIe clocks\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_pcie_power_up(struct mtk_pcie_port *port)
> > +{
> > +	struct device *dev = port->dev;
> > +	int err;
> > +
> > +	port->phy_reset = devm_reset_control_get_optional_exclusive(dev, "phy");
> > +	if (IS_ERR(port->phy_reset))
> > +		return PTR_ERR(port->phy_reset);
> > +
> > +	/* PHY power on and enable pipe clock */
> > +	port->phy = devm_phy_optional_get(dev, "pcie-phy");
> > +	if (IS_ERR(port->phy))
> > +		return PTR_ERR(port->phy);
> > +
> > +	reset_control_deassert(port->phy_reset);
> > +
> > +	err = phy_init(port->phy);
> > +	if (err) {
> > +		dev_err(dev, "failed to initialize PCIe phy\n");
> > +		goto err_phy_init;
> > +	}
> > +
> > +	err = phy_power_on(port->phy);
> > +	if (err) {
> > +		dev_err(dev, "failed to power on PCIe phy\n");
> > +		goto err_phy_on;
> > +	}
> > +
> > +	port->mac_reset = devm_reset_control_get_optional_exclusive(dev, "mac");
> > +	if (IS_ERR(port->mac_reset)) {
> > +		err = PTR_ERR(port->mac_reset);
> > +		goto err_mac_rst;
> > +	}
> > +
> > +	reset_control_deassert(port->mac_reset);
> > +
> > +	/* MAC power on and enable transaction layer clocks */
> > +	pm_runtime_enable(dev);
> > +	pm_runtime_get_sync(dev);
> > +
> > +	err = mtk_pcie_clk_init(port);
> > +	if (err) {
> > +		dev_err(dev, "clock init failed\n");
> > +		goto err_clk_init;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_clk_init:
> > +	pm_runtime_put_sync(dev);
> > +	pm_runtime_disable(dev);
> > +	reset_control_assert(port->mac_reset);
> > +err_mac_rst:
> > +	phy_power_off(port->phy);
> > +err_phy_on:
> > +	phy_exit(port->phy);
> > +err_phy_init:
> > +	reset_control_assert(port->phy_reset);
> > +
> > +	return err;
> > +}
> > +
> > +static void mtk_pcie_power_down(struct mtk_pcie_port *port)
> > +{
> > +	clk_bulk_disable_unprepare(port->num_clks, port->clks);
> > +
> > +	pm_runtime_put_sync(port->dev);
> > +	pm_runtime_disable(port->dev);
> > +	reset_control_assert(port->mac_reset);
> > +
> > +	phy_power_off(port->phy);
> > +	phy_exit(port->phy);
> > +	reset_control_assert(port->phy_reset);
> > +}
> > +
> > +static int mtk_pcie_setup(struct mtk_pcie_port *port)
> > +{
> > +	struct device *dev = port->dev;
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct resource *regs;
> > +	int err;
> > +
> > +	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");
> > +	port->base = devm_ioremap_resource(dev, regs);
> 
> Use devm_platform_ioremap_resource_byname()

The regs will be needed to get the physical address of PCIe register, it
will be used to configure the capture address of MSI.
> 
> > +	if (IS_ERR(port->base)) {
> > +		dev_err(dev, "failed to map register base\n");
> > +		return PTR_ERR(port->base);
> > +	}
> > +
> > +	port->reg_base = regs->start;
> > +
> > +	/* Don't touch the hardware registers before power up */
> > +	err = mtk_pcie_power_up(port);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Try link up */
> > +	err = mtk_pcie_startup_port(port);
> > +	if (err) {
> > +		dev_err(dev, "PCIe startup failed\n");
> > +		goto err_setup;
> > +	}
> > +
> > +	dev_info(dev, "PCIe link up success!\n");
> 
> Do you really need this, there's lots printed by PCI in the successful 
> case.

I will drop this in the next version, thanks for your review.

Thanks.
> 
> > +
> > +	return 0;
> > +
> > +err_setup:
> > +	mtk_pcie_power_down(port);
> > +
> > +	return err;
> > +}
> > +
> > +static int mtk_pcie_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct mtk_pcie_port *port;
> > +	struct pci_host_bridge *host;
> > +	int err;
> > +
> > +	host = devm_pci_alloc_host_bridge(dev, sizeof(*port));
> > +	if (!host)
> > +		return -ENOMEM;
> > +
> > +	port = pci_host_bridge_priv(host);
> > +
> > +	port->dev = dev;
> > +	platform_set_drvdata(pdev, port);
> > +
> > +	err = mtk_pcie_setup(port);
> > +	if (err)
> > +		return err;
> > +
> > +	host->ops = &mtk_pcie_ops;
> > +	host->sysdata = port;
> > +
> > +	err = pci_host_probe(host);
> > +	if (err) {
> > +		mtk_pcie_power_down(port);
> > +		return err;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_pcie_remove(struct platform_device *pdev)
> > +{
> > +	struct mtk_pcie_port *port = platform_get_drvdata(pdev);
> > +	struct pci_host_bridge *host = pci_host_bridge_from_priv(port);
> > +
> > +	pci_lock_rescan_remove();
> > +	pci_stop_root_bus(host->bus);
> > +	pci_remove_root_bus(host->bus);
> > +	pci_unlock_rescan_remove();
> > +
> > +	mtk_pcie_power_down(port);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id mtk_pcie_of_match[] = {
> > +	{ .compatible = "mediatek,mt8192-pcie" },
> > +	{},
> > +};
> > +
> > +static struct platform_driver mtk_pcie_driver = {
> > +	.probe = mtk_pcie_probe,
> > +	.remove = mtk_pcie_remove,
> > +	.driver = {
> > +		.name = "mtk-pcie",
> > +		.of_match_table = mtk_pcie_of_match,
> > +	},
> > +};
> > +
> > +module_platform_driver(mtk_pcie_driver);
> > +MODULE_LICENSE("GPL v2");
> > -- 
> > 2.25.1
> > 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [v7,1/7] dt-bindings: PCI: mediatek-gen3: Add YAML schema

[Jianjun Wang]

On Mon, 2021-01-25 at 14:22 -0600, Rob Herring wrote:
> On Wed, Jan 13, 2021 at 07:39:55PM +0800, Jianjun Wang wrote:
> > Add YAML schemas documentation for Gen3 PCIe controller on
> > MediaTek SoCs.
> > 
> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  .../bindings/pci/mediatek-pcie-gen3.yaml      | 172 ++++++++++++++++++
> >  1 file changed, 172 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> > new file mode 100644
> > index 000000000000..f133fb0184f1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> > @@ -0,0 +1,172 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/mediatek-pcie-gen3.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Gen3 PCIe controller on MediaTek SoCs
> > +
> > +maintainers:
> > +  - Jianjun Wang <jianjun.wang@mediatek.com>
> > +
> > +description: |+
> > +  PCIe Gen3 MAC controller for MediaTek SoCs, it supports Gen3 speed
> > +  and compatible with Gen2, Gen1 speed.
> > +
> > +  This PCIe controller supports up to 256 MSI vectors, the MSI hardware
> > +  block diagram is as follows:
> > +
> > +                    +-----+
> > +                    | GIC |
> > +                    +-----+
> > +                       ^
> > +                       |
> > +                   port->irq
> > +                       |
> > +               +-+-+-+-+-+-+-+-+
> > +               |0|1|2|3|4|5|6|7| (PCIe intc)
> > +               +-+-+-+-+-+-+-+-+
> > +                ^ ^           ^
> > +                | |    ...    |
> > +        +-------+ +------+    +-----------+
> > +        |                |                |
> > +  +-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
> > +  |0|1|...|30|31|  |0|1|...|30|31|  |0|1|...|30|31| (MSI sets)
> > +  +-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
> > +   ^ ^      ^  ^    ^ ^      ^  ^    ^ ^      ^  ^
> > +   | |      |  |    | |      |  |    | |      |  |  (MSI vectors)
> > +   | |      |  |    | |      |  |    | |      |  |
> > +
> > +    (MSI SET0)       (MSI SET1)  ...   (MSI SET7)
> > +
> > +  With 256 MSI vectors supported, the MSI vectors are composed of 8 sets,
> > +  each set has its own address for MSI message, and supports 32 MSI vectors
> > +  to generate interrupt.
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/pci-bus.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: mediatek,mt8192-pcie
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  ranges:
> > +    minItems: 1
> > +    maxItems: 8
> > +
> > +  resets:
> > +    minItems: 1
> > +    maxItems: 2
> 
> Why the range? The SoC either has the reset lines or it doesn't.
> 
> > +
> > +  reset-names:
> > +    anyOf:
> > +      - const: mac
> > +      - const: phy
> 
> I don't think this should stay, but if so, better expressed like this:
> 
> minItems: 1
> maxItems: 2
> items:
>   enum: [ mac, phy ]

Hi Rob,

Thanks for your review, does this means that I should merge the resets
and reset-names together as the following property?

resets:
  minItems: 1
  maxItems: 2
  items:
    enum: [ mac, phy ]
> 
> 
> > +
> > +  clocks:
> > +    maxItems: 6
> > +
> > +  clock-names:
> > +    items:
> > +      - const: pl_250m
> > +      - const: tl_26m
> > +      - const: tl_96m
> > +      - const: tl_32k
> > +      - const: peri_26m
> > +      - const: top_133m
> > +
> > +  assigned-clocks:
> > +    maxItems: 1
> > +
> > +  assigned-clock-parents:
> > +    maxItems: 1
> > +
> > +  phys:
> > +    maxItems: 1
> > +
> > +  '#interrupt-cells':
> > +    const: 1
> > +
> > +  interrupt-controller:
> > +    description: Interrupt controller node for handling legacy PCI interrupts.
> > +    type: object
> > +    properties:
> > +      '#address-cells':
> > +        const: 0
> > +      '#interrupt-cells':
> > +        const: 1
> > +      interrupt-controller: true
> > +
> > +    required:
> > +      - '#address-cells'
> > +      - '#interrupt-cells'
> > +      - interrupt-controller
> > +
> > +    additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - ranges
> > +  - clocks
> > +  - '#interrupt-cells'
> > +  - interrupt-controller
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    bus {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        pcie: pcie@11230000 {
> > +            compatible = "mediatek,mt8192-pcie";
> > +            device_type = "pci";
> > +            #address-cells = <3>;
> > +            #size-cells = <2>;
> > +            reg = <0x00 0x11230000 0x00 0x4000>;
> > +            reg-names = "pcie-mac";
> 
> Not documented. Drop.
I will add the property information for reg-names in the next version,
it's used by controller driver to get the register resource.

Thanks.
> 
> > +            interrupts = <GIC_SPI 251 IRQ_TYPE_LEVEL_HIGH 0>;
> > +            bus-range = <0x00 0xff>;
> > +            ranges = <0x82000000 0x00 0x12000000 0x00
> > +                      0x12000000 0x00 0x1000000>;
> > +            clocks = <&infracfg 44>,
> > +                     <&infracfg 40>,
> > +                     <&infracfg 43>,
> > +                     <&infracfg 97>,
> > +                     <&infracfg 99>,
> > +                     <&infracfg 111>;
> > +            clock-names = "pl_250m", "tl_26m", "tl_96m",
> > +                          "tl_32k", "peri_26m", "top_133m";
> > +            assigned-clocks = <&topckgen 50>;
> > +            assigned-clock-parents = <&topckgen 91>;
> > +
> > +            phys = <&pciephy>;
> > +            phy-names = "pcie-phy";
> > +            resets = <&infracfg_rst 0>;
> > +            reset-names = "phy";
> > +
> > +            #interrupt-cells = <1>;
> > +            interrupt-map-mask = <0 0 0 0x7>;
> > +            interrupt-map = <0 0 0 1 &pcie_intc 0>,
> > +                            <0 0 0 2 &pcie_intc 1>,
> > +                            <0 0 0 3 &pcie_intc 2>,
> > +                            <0 0 0 4 &pcie_intc 3>;
> > +            pcie_intc: interrupt-controller {
> > +                      #address-cells = <0>;
> > +                      #interrupt-cells = <1>;
> > +                      interrupt-controller;
> > +            };
> > +        };
> > +    };
> > -- 
> > 2.25.1
> > 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [v7,4/7] PCI: mediatek-gen3: Add INTx support

[Marc Zyngier]

On 2021-01-13 11:39, Jianjun Wang wrote:
> Add INTx support for MediaTek Gen3 PCIe controller.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 163 ++++++++++++++++++++
>  1 file changed, 163 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> b/drivers/pci/controller/pcie-mediatek-gen3.c
> index c00ea7c167de..7979a2856c35 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -9,6 +9,9 @@
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/iopoll.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of_address.h>
> @@ -49,6 +52,12 @@
>  #define PCIE_LINK_STATUS_REG		0x154
>  #define PCIE_PORT_LINKUP		BIT(8)
> 
> +#define PCIE_INT_ENABLE_REG		0x180
> +#define PCIE_INTX_SHIFT			24
> +#define PCIE_INTX_MASK			GENMASK(27, 24)

I guess this '24' is actually PCIE_INTX_SHIFT? In this case,
please write it as

GENMASK(PCIE_INTX_SHIFT + PCI_NUM_INTX - 1, PCIE_INTX_SHIFT)

to make it clear that you are dealing with one bit per INTx.

> +
> +#define PCIE_INT_STATUS_REG		0x184
> +
>  #define PCIE_TRANS_TABLE_BASE_REG	0x800
>  #define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
>  #define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8
> @@ -77,6 +86,8 @@
>   * @phy: PHY controller block
>   * @clks: PCIe clocks
>   * @num_clks: PCIe clocks count for this port
> + * @irq: PCIe controller interrupt number
> + * @intx_domain: legacy INTx IRQ domain
>   */
>  struct mtk_pcie_port {
>  	struct device *dev;
> @@ -87,6 +98,9 @@ struct mtk_pcie_port {
>  	struct phy *phy;
>  	struct clk_bulk_data *clks;
>  	int num_clks;
> +
> +	int irq;
> +	struct irq_domain *intx_domain;
>  };
> 
>  /**
> @@ -266,6 +280,149 @@ static int mtk_pcie_startup_port(struct
> mtk_pcie_port *port)
>  	return 0;
>  }
> 
> +static int mtk_pcie_set_affinity(struct irq_data *data,
> +				 const struct cpumask *mask, bool force)
> +{
> +	return -EINVAL;
> +}
> +
> +static void mtk_intx_mask(struct irq_data *data)
> +{
> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> +	u32 val;
> +
> +	val = readl_relaxed(port->base + PCIE_INT_ENABLE_REG);
> +	val &= ~BIT(data->hwirq + PCIE_INTX_SHIFT);
> +	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);

This is missing some locking. Otherwise, two concurrent mask/unmask
for different interrupts will corrupt each other's state.

> +}
> +
> +static void mtk_intx_unmask(struct irq_data *data)
> +{
> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> +	u32 val;
> +
> +	val = readl_relaxed(port->base + PCIE_INT_ENABLE_REG);
> +	val |= BIT(data->hwirq + PCIE_INTX_SHIFT);
> +	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);

Same thing here.

> +}
> +
> +/**
> + * mtk_intx_eoi
> + * @data: pointer to chip specific data
> + *
> + * As an emulated level IRQ, its interrupt status will remain
> + * until the corresponding de-assert message is received; hence that
> + * the status can only be cleared when the interrupt has been 
> serviced.
> + */
> +static void mtk_intx_eoi(struct irq_data *data)
> +{
> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> +	unsigned long hwirq;
> +
> +	hwirq = data->hwirq + PCIE_INTX_SHIFT;
> +	writel_relaxed(BIT(hwirq), port->base + PCIE_INT_STATUS_REG);
> +}
> +
> +static struct irq_chip mtk_intx_irq_chip = {
> +	.irq_mask		= mtk_intx_mask,
> +	.irq_unmask		= mtk_intx_unmask,
> +	.irq_eoi		= mtk_intx_eoi,
> +	.irq_set_affinity	= mtk_pcie_set_affinity,
> +	.name			= "PCIe",

nit: "PCIe" is not really descriptive. "INTx" would be a bit better.

> +};
> +
> +static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int 
> irq,
> +			     irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler_name(irq, &mtk_intx_irq_chip,
> +				      handle_fasteoi_irq, "INTx");
> +	irq_set_chip_data(irq, domain->host_data);

You probably want to set the chip_data *before* wiring
the handler, as otherwise you could end-up with a NULL
pointer in any of the callbacks if the interrupt fires
between the two.

> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops intx_domain_ops = {
> +	.map = mtk_pcie_intx_map,
> +};
> +
> +static int mtk_pcie_init_irq_domains(struct mtk_pcie_port *port,
> +				     struct device_node *node)
> +{
> +	struct device *dev = port->dev;
> +	struct device_node *intc_node;
> +
> +	/* Setup INTx */
> +	intc_node = of_get_child_by_name(node, "interrupt-controller");
> +	if (!intc_node) {
> +		dev_err(dev, "missing PCIe Intc node\n");
> +		return -ENODEV;
> +	}
> +
> +	port->intx_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX,
> +						  &intx_domain_ops, port);
> +	if (!port->intx_domain) {
> +		dev_err(dev, "failed to get INTx IRQ domain\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static void mtk_pcie_irq_teardown(struct mtk_pcie_port *port)
> +{
> +	irq_set_chained_handler_and_data(port->irq, NULL, NULL);
> +
> +	if (port->intx_domain)
> +		irq_domain_remove(port->intx_domain);
> +
> +	irq_dispose_mapping(port->irq);
> +}
> +
> +static void mtk_pcie_irq_handler(struct irq_desc *desc)
> +{
> +	struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);
> +	struct irq_chip *irqchip = irq_desc_get_chip(desc);
> +	unsigned long status;
> +	unsigned int virq;
> +	irq_hw_number_t irq_bit = PCIE_INTX_SHIFT;
> +
> +	chained_irq_enter(irqchip, desc);
> +
> +	status = readl_relaxed(port->base + PCIE_INT_STATUS_REG);
> +	if (status & PCIE_INTX_MASK) {

This "if (status & PCIE_INTX_MASK)" is already implicit from
the for_each_set_bit_from() iterator, and you can drop it.

> +		for_each_set_bit_from(irq_bit, &status, PCI_NUM_INTX +
> +				      PCIE_INTX_SHIFT) {
> +			virq = irq_find_mapping(port->intx_domain,
> +						irq_bit - PCIE_INTX_SHIFT);
> +			generic_handle_irq(virq);
> +		}
> +	}
> +
> +	chained_irq_exit(irqchip, desc);
> +}
> +
> +static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
> +			      struct device_node *node)
> +{
> +	struct device *dev = port->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	int err;
> +
> +	err = mtk_pcie_init_irq_domains(port, node);
> +	if (err) {
> +		dev_err(dev, "failed to init PCIe IRQ domain\n");
> +		return err;
> +	}
> +
> +	port->irq = platform_get_irq(pdev, 0);
> +	if (port->irq < 0)
> +		return port->irq;
> +
> +	irq_set_chained_handler_and_data(port->irq, mtk_pcie_irq_handler, 
> port);

You seem to be missing something that will mask all INTx interrupts
as an initial state.

> +
> +	return 0;
> +}
> +
>  static int mtk_pcie_clk_init(struct mtk_pcie_port *port)
>  {
>  	int ret;
> @@ -388,6 +545,10 @@ static int mtk_pcie_setup(struct mtk_pcie_port 
> *port)
>  		goto err_setup;
>  	}
> 
> +	err = mtk_pcie_setup_irq(port, dev->of_node);
> +	if (err)
> +		goto err_setup;
> +
>  	dev_info(dev, "PCIe link up success!\n");
> 
>  	return 0;
> @@ -423,6 +584,7 @@ static int mtk_pcie_probe(struct platform_device 
> *pdev)
> 
>  	err = pci_host_probe(host);
>  	if (err) {
> +		mtk_pcie_irq_teardown(port);
>  		mtk_pcie_power_down(port);
>  		return err;
>  	}
> @@ -440,6 +602,7 @@ static int mtk_pcie_remove(struct platform_device 
> *pdev)
>  	pci_remove_root_bus(host->bus);
>  	pci_unlock_rescan_remove();
> 
> +	mtk_pcie_irq_teardown(port);
>  	mtk_pcie_power_down(port);
> 
>  	return 0;

Thanks,

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

Re: [v7,5/7] PCI: mediatek-gen3: Add MSI support

[Marc Zyngier]

On 2021-01-13 11:39, Jianjun Wang wrote:
> Add MSI support for MediaTek Gen3 PCIe controller.
> 
> This PCIe controller supports up to 256 MSI vectors, the MSI hardware
> block diagram is as follows:
> 
>                   +-----+
>                   | GIC |
>                   +-----+
>                      ^
>                      |
>                  port->irq
>                      |
>              +-+-+-+-+-+-+-+-+
>              |0|1|2|3|4|5|6|7| (PCIe intc)
>              +-+-+-+-+-+-+-+-+
>               ^ ^           ^
>               | |    ...    |
>       +-------+ +------+    +-----------+
>       |                |                |
> +-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
> |0|1|...|30|31|  |0|1|...|30|31|  |0|1|...|30|31| (MSI sets)
> +-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
>  ^ ^      ^  ^    ^ ^      ^  ^    ^ ^      ^  ^
>  | |      |  |    | |      |  |    | |      |  |  (MSI vectors)
>  | |      |  |    | |      |  |    | |      |  |
> 
>   (MSI SET0)       (MSI SET1)  ...   (MSI SET7)
> 
> With 256 MSI vectors supported, the MSI vectors are composed of 8 sets,
> each set has its own address for MSI message, and supports 32 MSI 
> vectors
> to generate interrupt.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 261 ++++++++++++++++++++
>  1 file changed, 261 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 7979a2856c35..471d97cd1ef9 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -14,6 +14,7 @@
>  #include <linux/irqdomain.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/msi.h>
>  #include <linux/of_address.h>
>  #include <linux/of_clk.h>
>  #include <linux/of_pci.h>
> @@ -52,11 +53,28 @@
>  #define PCIE_LINK_STATUS_REG		0x154
>  #define PCIE_PORT_LINKUP		BIT(8)
> 
> +#define PCIE_MSI_SET_NUM		8
> +#define PCIE_MSI_IRQS_PER_SET		32
> +#define PCIE_MSI_IRQS_NUM \
> +	(PCIE_MSI_IRQS_PER_SET * (PCIE_MSI_SET_NUM))

Spurious inner bracketing.

> +
>  #define PCIE_INT_ENABLE_REG		0x180
> +#define PCIE_MSI_ENABLE			GENMASK(PCIE_MSI_SET_NUM + 8 - 1, 8)
> +#define PCIE_MSI_SHIFT			8
>  #define PCIE_INTX_SHIFT			24
>  #define PCIE_INTX_MASK			GENMASK(27, 24)
> 
>  #define PCIE_INT_STATUS_REG		0x184
> +#define PCIE_MSI_SET_ENABLE_REG		0x190
> +#define PCIE_MSI_SET_ENABLE		GENMASK(PCIE_MSI_SET_NUM - 1, 0)
> +
> +#define PCIE_MSI_SET_BASE_REG		0xc00
> +#define PCIE_MSI_SET_OFFSET		0x10
> +#define PCIE_MSI_SET_STATUS_OFFSET	0x04
> +#define PCIE_MSI_SET_ENABLE_OFFSET	0x08
> +
> +#define PCIE_MSI_SET_ADDR_HI_BASE	0xc80
> +#define PCIE_MSI_SET_ADDR_HI_OFFSET	0x04
> 
>  #define PCIE_TRANS_TABLE_BASE_REG	0x800
>  #define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
> @@ -76,6 +94,18 @@
>  #define PCIE_ATR_TLP_TYPE_MEM		PCIE_ATR_TLP_TYPE(0)
>  #define PCIE_ATR_TLP_TYPE_IO		PCIE_ATR_TLP_TYPE(2)
> 
> +/**
> + * struct mtk_pcie_msi - MSI information for each set
> + * @dev: pointer to PCIe device
> + * @base: IO mapped register base
> + * @msg_addr: MSI message address
> + */
> +struct mtk_msi_set {
> +	struct device *dev;
> +	void __iomem *base;
> +	phys_addr_t msg_addr;
> +};
> +
>  /**
>   * struct mtk_pcie_port - PCIe port information
>   * @dev: pointer to PCIe device
> @@ -88,6 +118,11 @@
>   * @num_clks: PCIe clocks count for this port
>   * @irq: PCIe controller interrupt number
>   * @intx_domain: legacy INTx IRQ domain
> + * @msi_domain: MSI IRQ domain
> + * @msi_bottom_domain: MSI IRQ bottom domain
> + * @msi_sets: MSI sets information
> + * @lock: lock protecting IRQ bit map
> + * @msi_irq_in_use: bit map for assigned MSI IRQ
>   */
>  struct mtk_pcie_port {
>  	struct device *dev;
> @@ -101,6 +136,11 @@ struct mtk_pcie_port {
> 
>  	int irq;
>  	struct irq_domain *intx_domain;
> +	struct irq_domain *msi_domain;
> +	struct irq_domain *msi_bottom_domain;
> +	struct mtk_msi_set msi_sets[PCIE_MSI_SET_NUM];
> +	struct mutex lock;
> +	DECLARE_BITMAP(msi_irq_in_use, PCIE_MSI_IRQS_NUM);
>  };
> 
>  /**
> @@ -243,6 +283,15 @@ static int mtk_pcie_startup_port(struct
> mtk_pcie_port *port)
>  		return err;
>  	}
> 
> +	/* Enable MSI */
> +	val = readl_relaxed(port->base + PCIE_MSI_SET_ENABLE_REG);
> +	val |= PCIE_MSI_SET_ENABLE;
> +	writel_relaxed(val, port->base + PCIE_MSI_SET_ENABLE_REG);
> +
> +	val = readl_relaxed(port->base + PCIE_INT_ENABLE_REG);
> +	val |= PCIE_MSI_ENABLE;
> +	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
> +
>  	/* Set PCIe translation windows */
>  	resource_list_for_each_entry(entry, &host->windows) {
>  		struct resource *res = entry->res;
> @@ -286,6 +335,129 @@ static int mtk_pcie_set_affinity(struct irq_data 
> *data,
>  	return -EINVAL;
>  }
> 
> +static struct irq_chip mtk_msi_irq_chip = {
> +	.name = "MSI",
> +	.irq_ack = irq_chip_ack_parent,
> +};
> +
> +static struct msi_domain_info mtk_msi_domain_info = {
> +	.flags		= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_PCI_MSIX |
> +			   MSI_FLAG_USE_DEF_CHIP_OPS | MSI_FLAG_MULTI_PCI_MSI),
> +	.chip		= &mtk_msi_irq_chip,
> +};
> +
> +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg 
> *msg)
> +{
> +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> +	unsigned long hwirq;
> +
> +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> +
> +	msg->address_hi = upper_32_bits(msi_set->msg_addr);
> +	msg->address_lo = lower_32_bits(msi_set->msg_addr);
> +	msg->data = hwirq;
> +	dev_dbg(msi_set->dev, "msi#%#lx address_hi %#x address_lo %#x data 
> %d\n",
> +		hwirq, msg->address_hi, msg->address_lo, msg->data);
> +}
> +
> +static void mtk_msi_bottom_irq_ack(struct irq_data *data)
> +{
> +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> +	unsigned long hwirq;
> +
> +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> +
> +	writel_relaxed(BIT(hwirq), msi_set->base + 
> PCIE_MSI_SET_STATUS_OFFSET);
> +}
> +
> +static struct irq_chip mtk_msi_bottom_irq_chip = {
> +	.irq_ack		= mtk_msi_bottom_irq_ack,
> +	.irq_compose_msi_msg	= mtk_compose_msi_msg,
> +	.irq_set_affinity	= mtk_pcie_set_affinity,
> +	.name			= "PCIe",

nit: "MSI", rather than "PCIe".

> +};
> +
> +static int mtk_msi_bottom_domain_alloc(struct irq_domain *domain,
> +				       unsigned int virq, unsigned int nr_irqs,
> +				       void *arg)
> +{
> +	struct mtk_pcie_port *port = domain->host_data;
> +	struct mtk_msi_set *msi_set;
> +	int i, hwirq, set_idx;
> +
> +	mutex_lock(&port->lock);
> +
> +	hwirq = bitmap_find_free_region(port->msi_irq_in_use, 
> PCIE_MSI_IRQS_NUM,
> +					order_base_2(nr_irqs));
> +
> +	mutex_unlock(&port->lock);
> +
> +	if (hwirq < 0)
> +		return -ENOSPC;
> +
> +	set_idx = hwirq / PCIE_MSI_IRQS_PER_SET;
> +	msi_set = &port->msi_sets[set_idx];
> +
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_info(domain, virq + i, hwirq + i,
> +				    &mtk_msi_bottom_irq_chip, msi_set,
> +				    handle_edge_irq, NULL, NULL);
> +
> +	return 0;
> +}
> +
> +static void mtk_msi_bottom_domain_free(struct irq_domain *domain,
> +				       unsigned int virq, unsigned int nr_irqs)
> +{
> +	struct mtk_pcie_port *port = domain->host_data;
> +	struct irq_data *data = irq_domain_get_irq_data(domain, virq);
> +
> +	mutex_lock(&port->lock);
> +
> +	bitmap_clear(port->msi_irq_in_use, data->hwirq, nr_irqs);
> +
> +	mutex_unlock(&port->lock);
> +
> +	irq_domain_free_irqs_common(domain, virq, nr_irqs);
> +}
> +
> +static int mtk_msi_bottom_domain_activate(struct irq_domain *domain,
> +					  struct irq_data *data, bool reserve)
> +{
> +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> +	unsigned long hwirq;
> +	u32 val;
> +
> +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> +
> +	val = readl_relaxed(msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
> +	val |= BIT(hwirq);
> +	writel_relaxed(val, msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);

This isn't an activate. This is an unmask, which suffers from the same
issue as its INTx sibling.

> +
> +	return 0;
> +}
> +
> +static void mtk_msi_bottom_domain_deactivate(struct irq_domain 
> *domain,
> +					     struct irq_data *data)
> +{
> +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> +	unsigned long hwirq;
> +	u32 val;
> +
> +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> +
> +	val = readl_relaxed(msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
> +	val &= ~BIT(hwirq);
> +	writel_relaxed(val, msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
> +}

Same thing, this is a mask. I don't think this block requires any
activate/deactivate callbacks for its lower irqdomain.

As it stands, you can't mask a MSI at the low-level, which is
pretty bad (you need to mask them at the PCI source, which can
end-up disabling all vectors in the case of Multi-MSI).

> +
> +static const struct irq_domain_ops mtk_msi_bottom_domain_ops = {
> +	.alloc = mtk_msi_bottom_domain_alloc,
> +	.free = mtk_msi_bottom_domain_free,
> +	.activate = mtk_msi_bottom_domain_activate,
> +	.deactivate = mtk_msi_bottom_domain_deactivate,
> +};
> +
>  static void mtk_intx_mask(struct irq_data *data)
>  {
>  	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> @@ -350,6 +522,9 @@ static int mtk_pcie_init_irq_domains(struct
> mtk_pcie_port *port,
>  {
>  	struct device *dev = port->dev;
>  	struct device_node *intc_node;
> +	struct fwnode_handle *fwnode = of_node_to_fwnode(node);
> +	struct msi_domain_info *info;
> +	int i, ret;
> 
>  	/* Setup INTx */
>  	intc_node = of_get_child_by_name(node, "interrupt-controller");
> @@ -365,7 +540,57 @@ static int mtk_pcie_init_irq_domains(struct
> mtk_pcie_port *port,
>  		return -ENODEV;
>  	}
> 
> +	/* Setup MSI */
> +	mutex_init(&port->lock);
> +
> +	port->msi_bottom_domain = irq_domain_add_linear(node, 
> PCIE_MSI_IRQS_NUM,
> +				  &mtk_msi_bottom_domain_ops, port);
> +	if (!port->msi_bottom_domain) {
> +		dev_info(dev, "failed to create MSI bottom domain\n");
> +		ret = -ENODEV;
> +		goto err_msi_bottom_domain;
> +	}
> +
> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +	if (!info) {
> +		ret = -ENOMEM;
> +		goto err_msi_bottom_domain;
> +	}
> +
> +	memcpy(info, &mtk_msi_domain_info, sizeof(*info));

Why the memcpy()? There is nothing in mtk_msi_domain_info that is
per-domain, and you should be able to use this structure for all
ports, shouldn't you?

> +	info->chip_data = port;
> +
> +	port->msi_domain = pci_msi_create_irq_domain(fwnode, info,
> +						     port->msi_bottom_domain);
> +	if (!port->msi_domain) {
> +		dev_info(dev, "failed to create MSI domain\n");
> +		ret = -ENODEV;
> +		goto err_msi_domain;
> +	}
> +
> +	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
> +		struct mtk_msi_set *msi_set = &port->msi_sets[i];
> +
> +		msi_set->dev = port->dev;

Given that this is only used in a debug message, and that the addresses
are already non-ambiguous, you can probably remove this field.

> +		msi_set->base = port->base + PCIE_MSI_SET_BASE_REG +
> +				i * PCIE_MSI_SET_OFFSET;
> +		msi_set->msg_addr = port->reg_base + PCIE_MSI_SET_BASE_REG +
> +				    i * PCIE_MSI_SET_OFFSET;
> +
> +		writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
> +		writel_relaxed(upper_32_bits(msi_set->msg_addr),
> +			       port->base + PCIE_MSI_SET_ADDR_HI_BASE +
> +			       i * PCIE_MSI_SET_ADDR_HI_OFFSET);

Please a comment on what this is doing...

> +	}
> +
>  	return 0;
> +
> +err_msi_domain:
> +	irq_domain_remove(port->msi_bottom_domain);
> +err_msi_bottom_domain:
> +	irq_domain_remove(port->intx_domain);
> +
> +	return ret;
>  }
> 
>  static void mtk_pcie_irq_teardown(struct mtk_pcie_port *port)
> @@ -375,9 +600,34 @@ static void mtk_pcie_irq_teardown(struct
> mtk_pcie_port *port)
>  	if (port->intx_domain)
>  		irq_domain_remove(port->intx_domain);
> 
> +	if (port->msi_domain)
> +		irq_domain_remove(port->msi_domain);
> +
> +	if (port->msi_bottom_domain)
> +		irq_domain_remove(port->msi_bottom_domain);
> +
>  	irq_dispose_mapping(port->irq);
>  }
> 
> +static void mtk_pcie_msi_handler(struct mtk_pcie_port *port, int 
> set_idx)
> +{
> +	struct mtk_msi_set *msi_set = &port->msi_sets[set_idx];
> +	unsigned long msi_enable, msi_status;
> +	unsigned int virq;
> +	irq_hw_number_t bit, hwirq;
> +
> +	msi_enable = readl_relaxed(msi_set->base + 
> PCIE_MSI_SET_ENABLE_OFFSET);
> +	while ((msi_status =
> +		readl_relaxed(msi_set->base + PCIE_MSI_SET_STATUS_OFFSET) &
> +			      msi_enable)) {
> +		for_each_set_bit(bit, &msi_status, PCIE_MSI_IRQS_PER_SET) {
> +			hwirq = bit + set_idx * PCIE_MSI_IRQS_PER_SET;
> +			virq = irq_find_mapping(port->msi_bottom_domain, hwirq);
> +			generic_handle_irq(virq);
> +		}
> +	}

This doesn't look very readable. How about something like:

         do {
                 msi_status = readl_relaxed(...) & msi_enable;
                 if (!msi_status)
                         break;

                 for_each_set_bit(...) {
                         ...
                 }
         } while(true);

> +}
> +
>  static void mtk_pcie_irq_handler(struct irq_desc *desc)
>  {
>  	struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);
> @@ -398,6 +648,17 @@ static void mtk_pcie_irq_handler(struct irq_desc 
> *desc)
>  		}
>  	}
> 
> +	if (status & PCIE_MSI_ENABLE) {

Same comment has for INTx.

> +		irq_bit = PCIE_MSI_SHIFT;
> +		for_each_set_bit_from(irq_bit, &status, PCIE_MSI_SET_NUM +
> +				      PCIE_MSI_SHIFT) {
> +			mtk_pcie_msi_handler(port, irq_bit - PCIE_MSI_SHIFT);
> +
> +			writel_relaxed(BIT(irq_bit),
> +				       port->base + PCIE_INT_STATUS_REG);
> +		}
> +	}
> +
>  	chained_irq_exit(irqchip, desc);
>  }

Thanks,

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

Re: [v7,5/7] PCI: mediatek-gen3: Add MSI support

[Jianjun Wang]

On Tue, 2021-01-26 at 13:57 +0000, Marc Zyngier wrote:
> On 2021-01-13 11:39, Jianjun Wang wrote:
> > Add MSI support for MediaTek Gen3 PCIe controller.
> > 
> > This PCIe controller supports up to 256 MSI vectors, the MSI hardware
> > block diagram is as follows:
> > 
> >                   +-----+
> >                   | GIC |
> >                   +-----+
> >                      ^
> >                      |
> >                  port->irq
> >                      |
> >              +-+-+-+-+-+-+-+-+
> >              |0|1|2|3|4|5|6|7| (PCIe intc)
> >              +-+-+-+-+-+-+-+-+
> >               ^ ^           ^
> >               | |    ...    |
> >       +-------+ +------+    +-----------+
> >       |                |                |
> > +-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
> > |0|1|...|30|31|  |0|1|...|30|31|  |0|1|...|30|31| (MSI sets)
> > +-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
> >  ^ ^      ^  ^    ^ ^      ^  ^    ^ ^      ^  ^
> >  | |      |  |    | |      |  |    | |      |  |  (MSI vectors)
> >  | |      |  |    | |      |  |    | |      |  |
> > 
> >   (MSI SET0)       (MSI SET1)  ...   (MSI SET7)
> > 
> > With 256 MSI vectors supported, the MSI vectors are composed of 8 sets,
> > each set has its own address for MSI message, and supports 32 MSI 
> > vectors
> > to generate interrupt.
> > 
> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 261 ++++++++++++++++++++
> >  1 file changed, 261 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index 7979a2856c35..471d97cd1ef9 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/irqdomain.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > +#include <linux/msi.h>
> >  #include <linux/of_address.h>
> >  #include <linux/of_clk.h>
> >  #include <linux/of_pci.h>
> > @@ -52,11 +53,28 @@
> >  #define PCIE_LINK_STATUS_REG		0x154
> >  #define PCIE_PORT_LINKUP		BIT(8)
> > 
> > +#define PCIE_MSI_SET_NUM		8
> > +#define PCIE_MSI_IRQS_PER_SET		32
> > +#define PCIE_MSI_IRQS_NUM \
> > +	(PCIE_MSI_IRQS_PER_SET * (PCIE_MSI_SET_NUM))
> 
> Spurious inner bracketing.
> 
> > +
> >  #define PCIE_INT_ENABLE_REG		0x180
> > +#define PCIE_MSI_ENABLE			GENMASK(PCIE_MSI_SET_NUM + 8 - 1, 8)
> > +#define PCIE_MSI_SHIFT			8
> >  #define PCIE_INTX_SHIFT			24
> >  #define PCIE_INTX_MASK			GENMASK(27, 24)
> > 
> >  #define PCIE_INT_STATUS_REG		0x184
> > +#define PCIE_MSI_SET_ENABLE_REG		0x190
> > +#define PCIE_MSI_SET_ENABLE		GENMASK(PCIE_MSI_SET_NUM - 1, 0)
> > +
> > +#define PCIE_MSI_SET_BASE_REG		0xc00
> > +#define PCIE_MSI_SET_OFFSET		0x10
> > +#define PCIE_MSI_SET_STATUS_OFFSET	0x04
> > +#define PCIE_MSI_SET_ENABLE_OFFSET	0x08
> > +
> > +#define PCIE_MSI_SET_ADDR_HI_BASE	0xc80
> > +#define PCIE_MSI_SET_ADDR_HI_OFFSET	0x04
> > 
> >  #define PCIE_TRANS_TABLE_BASE_REG	0x800
> >  #define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
> > @@ -76,6 +94,18 @@
> >  #define PCIE_ATR_TLP_TYPE_MEM		PCIE_ATR_TLP_TYPE(0)
> >  #define PCIE_ATR_TLP_TYPE_IO		PCIE_ATR_TLP_TYPE(2)
> > 
> > +/**
> > + * struct mtk_pcie_msi - MSI information for each set
> > + * @dev: pointer to PCIe device
> > + * @base: IO mapped register base
> > + * @msg_addr: MSI message address
> > + */
> > +struct mtk_msi_set {
> > +	struct device *dev;
> > +	void __iomem *base;
> > +	phys_addr_t msg_addr;
> > +};
> > +
> >  /**
> >   * struct mtk_pcie_port - PCIe port information
> >   * @dev: pointer to PCIe device
> > @@ -88,6 +118,11 @@
> >   * @num_clks: PCIe clocks count for this port
> >   * @irq: PCIe controller interrupt number
> >   * @intx_domain: legacy INTx IRQ domain
> > + * @msi_domain: MSI IRQ domain
> > + * @msi_bottom_domain: MSI IRQ bottom domain
> > + * @msi_sets: MSI sets information
> > + * @lock: lock protecting IRQ bit map
> > + * @msi_irq_in_use: bit map for assigned MSI IRQ
> >   */
> >  struct mtk_pcie_port {
> >  	struct device *dev;
> > @@ -101,6 +136,11 @@ struct mtk_pcie_port {
> > 
> >  	int irq;
> >  	struct irq_domain *intx_domain;
> > +	struct irq_domain *msi_domain;
> > +	struct irq_domain *msi_bottom_domain;
> > +	struct mtk_msi_set msi_sets[PCIE_MSI_SET_NUM];
> > +	struct mutex lock;
> > +	DECLARE_BITMAP(msi_irq_in_use, PCIE_MSI_IRQS_NUM);
> >  };
> > 
> >  /**
> > @@ -243,6 +283,15 @@ static int mtk_pcie_startup_port(struct
> > mtk_pcie_port *port)
> >  		return err;
> >  	}
> > 
> > +	/* Enable MSI */
> > +	val = readl_relaxed(port->base + PCIE_MSI_SET_ENABLE_REG);
> > +	val |= PCIE_MSI_SET_ENABLE;
> > +	writel_relaxed(val, port->base + PCIE_MSI_SET_ENABLE_REG);
> > +
> > +	val = readl_relaxed(port->base + PCIE_INT_ENABLE_REG);
> > +	val |= PCIE_MSI_ENABLE;
> > +	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
> > +
> >  	/* Set PCIe translation windows */
> >  	resource_list_for_each_entry(entry, &host->windows) {
> >  		struct resource *res = entry->res;
> > @@ -286,6 +335,129 @@ static int mtk_pcie_set_affinity(struct irq_data 
> > *data,
> >  	return -EINVAL;
> >  }
> > 
> > +static struct irq_chip mtk_msi_irq_chip = {
> > +	.name = "MSI",
> > +	.irq_ack = irq_chip_ack_parent,
> > +};
> > +
> > +static struct msi_domain_info mtk_msi_domain_info = {
> > +	.flags		= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_PCI_MSIX |
> > +			   MSI_FLAG_USE_DEF_CHIP_OPS | MSI_FLAG_MULTI_PCI_MSI),
> > +	.chip		= &mtk_msi_irq_chip,
> > +};
> > +
> > +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg 
> > *msg)
> > +{
> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> > +	unsigned long hwirq;
> > +
> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> > +
> > +	msg->address_hi = upper_32_bits(msi_set->msg_addr);
> > +	msg->address_lo = lower_32_bits(msi_set->msg_addr);
> > +	msg->data = hwirq;
> > +	dev_dbg(msi_set->dev, "msi#%#lx address_hi %#x address_lo %#x data 
> > %d\n",
> > +		hwirq, msg->address_hi, msg->address_lo, msg->data);
> > +}
> > +
> > +static void mtk_msi_bottom_irq_ack(struct irq_data *data)
> > +{
> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> > +	unsigned long hwirq;
> > +
> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> > +
> > +	writel_relaxed(BIT(hwirq), msi_set->base + 
> > PCIE_MSI_SET_STATUS_OFFSET);
> > +}
> > +
> > +static struct irq_chip mtk_msi_bottom_irq_chip = {
> > +	.irq_ack		= mtk_msi_bottom_irq_ack,
> > +	.irq_compose_msi_msg	= mtk_compose_msi_msg,
> > +	.irq_set_affinity	= mtk_pcie_set_affinity,
> > +	.name			= "PCIe",
> 
> nit: "MSI", rather than "PCIe".
> 
> > +};
> > +
> > +static int mtk_msi_bottom_domain_alloc(struct irq_domain *domain,
> > +				       unsigned int virq, unsigned int nr_irqs,
> > +				       void *arg)
> > +{
> > +	struct mtk_pcie_port *port = domain->host_data;
> > +	struct mtk_msi_set *msi_set;
> > +	int i, hwirq, set_idx;
> > +
> > +	mutex_lock(&port->lock);
> > +
> > +	hwirq = bitmap_find_free_region(port->msi_irq_in_use, 
> > PCIE_MSI_IRQS_NUM,
> > +					order_base_2(nr_irqs));
> > +
> > +	mutex_unlock(&port->lock);
> > +
> > +	if (hwirq < 0)
> > +		return -ENOSPC;
> > +
> > +	set_idx = hwirq / PCIE_MSI_IRQS_PER_SET;
> > +	msi_set = &port->msi_sets[set_idx];
> > +
> > +	for (i = 0; i < nr_irqs; i++)
> > +		irq_domain_set_info(domain, virq + i, hwirq + i,
> > +				    &mtk_msi_bottom_irq_chip, msi_set,
> > +				    handle_edge_irq, NULL, NULL);
> > +
> > +	return 0;
> > +}
> > +
> > +static void mtk_msi_bottom_domain_free(struct irq_domain *domain,
> > +				       unsigned int virq, unsigned int nr_irqs)
> > +{
> > +	struct mtk_pcie_port *port = domain->host_data;
> > +	struct irq_data *data = irq_domain_get_irq_data(domain, virq);
> > +
> > +	mutex_lock(&port->lock);
> > +
> > +	bitmap_clear(port->msi_irq_in_use, data->hwirq, nr_irqs);
> > +
> > +	mutex_unlock(&port->lock);
> > +
> > +	irq_domain_free_irqs_common(domain, virq, nr_irqs);
> > +}
> > +
> > +static int mtk_msi_bottom_domain_activate(struct irq_domain *domain,
> > +					  struct irq_data *data, bool reserve)
> > +{
> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> > +	unsigned long hwirq;
> > +	u32 val;
> > +
> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> > +
> > +	val = readl_relaxed(msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
> > +	val |= BIT(hwirq);
> > +	writel_relaxed(val, msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
> 
> This isn't an activate. This is an unmask, which suffers from the same
> issue as its INTx sibling.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void mtk_msi_bottom_domain_deactivate(struct irq_domain 
> > *domain,
> > +					     struct irq_data *data)
> > +{
> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> > +	unsigned long hwirq;
> > +	u32 val;
> > +
> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> > +
> > +	val = readl_relaxed(msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
> > +	val &= ~BIT(hwirq);
> > +	writel_relaxed(val, msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
> > +}
> 
> Same thing, this is a mask. I don't think this block requires any
> activate/deactivate callbacks for its lower irqdomain.
> 
> As it stands, you can't mask a MSI at the low-level, which is
> pretty bad (you need to mask them at the PCI source, which can
> end-up disabling all vectors in the case of Multi-MSI).

Hi Marc,

Thanks for your review. 

This mtk_msi_bottom_domain is the parent domain of pci_msi_domain, but
the mask/unmask callback of pci_msi_domain does not call the callback of
its parent. Therefore if these functions are put in the mask/unmask
callbacks, they will not have a chance to be called. 
> 
> > +
> > +static const struct irq_domain_ops mtk_msi_bottom_domain_ops = {
> > +	.alloc = mtk_msi_bottom_domain_alloc,
> > +	.free = mtk_msi_bottom_domain_free,
> > +	.activate = mtk_msi_bottom_domain_activate,
> > +	.deactivate = mtk_msi_bottom_domain_deactivate,
> > +};
> > +
> >  static void mtk_intx_mask(struct irq_data *data)
> >  {
> >  	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> > @@ -350,6 +522,9 @@ static int mtk_pcie_init_irq_domains(struct
> > mtk_pcie_port *port,
> >  {
> >  	struct device *dev = port->dev;
> >  	struct device_node *intc_node;
> > +	struct fwnode_handle *fwnode = of_node_to_fwnode(node);
> > +	struct msi_domain_info *info;
> > +	int i, ret;
> > 
> >  	/* Setup INTx */
> >  	intc_node = of_get_child_by_name(node, "interrupt-controller");
> > @@ -365,7 +540,57 @@ static int mtk_pcie_init_irq_domains(struct
> > mtk_pcie_port *port,
> >  		return -ENODEV;
> >  	}
> > 
> > +	/* Setup MSI */
> > +	mutex_init(&port->lock);
> > +
> > +	port->msi_bottom_domain = irq_domain_add_linear(node, 
> > PCIE_MSI_IRQS_NUM,
> > +				  &mtk_msi_bottom_domain_ops, port);
> > +	if (!port->msi_bottom_domain) {
> > +		dev_info(dev, "failed to create MSI bottom domain\n");
> > +		ret = -ENODEV;
> > +		goto err_msi_bottom_domain;
> > +	}
> > +
> > +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> > +	if (!info) {
> > +		ret = -ENOMEM;
> > +		goto err_msi_bottom_domain;
> > +	}
> > +
> > +	memcpy(info, &mtk_msi_domain_info, sizeof(*info));
> 
> Why the memcpy()? There is nothing in mtk_msi_domain_info that is
> per-domain, and you should be able to use this structure for all
> ports, shouldn't you?

Yes, it used to update the info->chip_data for each port, but since the
msi_set has been used for msi_bottom_domain ,it has no effect anymore, I
will drop it in the next version, thanks. 
> 
> > +	info->chip_data = port;
> > +
> > +	port->msi_domain = pci_msi_create_irq_domain(fwnode, info,
> > +						     port->msi_bottom_domain);
> > +	if (!port->msi_domain) {
> > +		dev_info(dev, "failed to create MSI domain\n");
> > +		ret = -ENODEV;
> > +		goto err_msi_domain;
> > +	}
> > +
> > +	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
> > +		struct mtk_msi_set *msi_set = &port->msi_sets[i];
> > +
> > +		msi_set->dev = port->dev;
> 
> Given that this is only used in a debug message, and that the addresses
> are already non-ambiguous, you can probably remove this field.

OK, I will remove it in the next version.

> 
> > +		msi_set->base = port->base + PCIE_MSI_SET_BASE_REG +
> > +				i * PCIE_MSI_SET_OFFSET;
> > +		msi_set->msg_addr = port->reg_base + PCIE_MSI_SET_BASE_REG +
> > +				    i * PCIE_MSI_SET_OFFSET;
> > +
> > +		writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
> > +		writel_relaxed(upper_32_bits(msi_set->msg_addr),
> > +			       port->base + PCIE_MSI_SET_ADDR_HI_BASE +
> > +			       i * PCIE_MSI_SET_ADDR_HI_OFFSET);
> 
> Please a comment on what this is doing...

This codes are used to configure the capture address of each MSI set,
the lower 32 bits of MSI address should be written to msi_set->base, but
the strange thing is that the address where need to write the higher 32
bits are not near each set, they are start from
PCIE_MSI_SET_ADDR_HI_BASE, and have PCIE_MSI_SET_ADDR_HI_OFFSET apart.

That's why it looks so weired...

> 
> > +	}
> > +
> >  	return 0;
> > +
> > +err_msi_domain:
> > +	irq_domain_remove(port->msi_bottom_domain);
> > +err_msi_bottom_domain:
> > +	irq_domain_remove(port->intx_domain);
> > +
> > +	return ret;
> >  }
> > 
> >  static void mtk_pcie_irq_teardown(struct mtk_pcie_port *port)
> > @@ -375,9 +600,34 @@ static void mtk_pcie_irq_teardown(struct
> > mtk_pcie_port *port)
> >  	if (port->intx_domain)
> >  		irq_domain_remove(port->intx_domain);
> > 
> > +	if (port->msi_domain)
> > +		irq_domain_remove(port->msi_domain);
> > +
> > +	if (port->msi_bottom_domain)
> > +		irq_domain_remove(port->msi_bottom_domain);
> > +
> >  	irq_dispose_mapping(port->irq);
> >  }
> > 
> > +static void mtk_pcie_msi_handler(struct mtk_pcie_port *port, int 
> > set_idx)
> > +{
> > +	struct mtk_msi_set *msi_set = &port->msi_sets[set_idx];
> > +	unsigned long msi_enable, msi_status;
> > +	unsigned int virq;
> > +	irq_hw_number_t bit, hwirq;
> > +
> > +	msi_enable = readl_relaxed(msi_set->base + 
> > PCIE_MSI_SET_ENABLE_OFFSET);
> > +	while ((msi_status =
> > +		readl_relaxed(msi_set->base + PCIE_MSI_SET_STATUS_OFFSET) &
> > +			      msi_enable)) {
> > +		for_each_set_bit(bit, &msi_status, PCIE_MSI_IRQS_PER_SET) {
> > +			hwirq = bit + set_idx * PCIE_MSI_IRQS_PER_SET;
> > +			virq = irq_find_mapping(port->msi_bottom_domain, hwirq);
> > +			generic_handle_irq(virq);
> > +		}
> > +	}
> 
> This doesn't look very readable. How about something like:
> 
>          do {
>                  msi_status = readl_relaxed(...) & msi_enable;
>                  if (!msi_status)
>                          break;
> 
>                  for_each_set_bit(...) {
>                          ...
>                  }
>          } while(true);
> 

I will modify it in the next version, thanks.

> > +}
> > +
> >  static void mtk_pcie_irq_handler(struct irq_desc *desc)
> >  {
> >  	struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);
> > @@ -398,6 +648,17 @@ static void mtk_pcie_irq_handler(struct irq_desc 
> > *desc)
> >  		}
> >  	}
> > 
> > +	if (status & PCIE_MSI_ENABLE) {
> 
> Same comment has for INTx.
> 
> > +		irq_bit = PCIE_MSI_SHIFT;
> > +		for_each_set_bit_from(irq_bit, &status, PCIE_MSI_SET_NUM +
> > +				      PCIE_MSI_SHIFT) {
> > +			mtk_pcie_msi_handler(port, irq_bit - PCIE_MSI_SHIFT);
> > +
> > +			writel_relaxed(BIT(irq_bit),
> > +				       port->base + PCIE_INT_STATUS_REG);
> > +		}
> > +	}
> > +
> >  	chained_irq_exit(irqchip, desc);
> >  }
> 
> Thanks,
> 
>          M.

Re: [v7,5/7] PCI: mediatek-gen3: Add MSI support

[Marc Zyngier]

On 2021-01-27 12:31, Jianjun Wang wrote:
> On Tue, 2021-01-26 at 13:57 +0000, Marc Zyngier wrote:
>> On 2021-01-13 11:39, Jianjun Wang wrote:
>> > Add MSI support for MediaTek Gen3 PCIe controller.
>> >
>> > This PCIe controller supports up to 256 MSI vectors, the MSI hardware
>> > block diagram is as follows:
>> >
>> >                   +-----+
>> >                   | GIC |
>> >                   +-----+
>> >                      ^
>> >                      |
>> >                  port->irq
>> >                      |
>> >              +-+-+-+-+-+-+-+-+
>> >              |0|1|2|3|4|5|6|7| (PCIe intc)
>> >              +-+-+-+-+-+-+-+-+
>> >               ^ ^           ^
>> >               | |    ...    |
>> >       +-------+ +------+    +-----------+
>> >       |                |                |
>> > +-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
>> > |0|1|...|30|31|  |0|1|...|30|31|  |0|1|...|30|31| (MSI sets)
>> > +-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
>> >  ^ ^      ^  ^    ^ ^      ^  ^    ^ ^      ^  ^
>> >  | |      |  |    | |      |  |    | |      |  |  (MSI vectors)
>> >  | |      |  |    | |      |  |    | |      |  |
>> >
>> >   (MSI SET0)       (MSI SET1)  ...   (MSI SET7)
>> >
>> > With 256 MSI vectors supported, the MSI vectors are composed of 8 sets,
>> > each set has its own address for MSI message, and supports 32 MSI
>> > vectors
>> > to generate interrupt.
>> >
>> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
>> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
>> > ---
>> >  drivers/pci/controller/pcie-mediatek-gen3.c | 261 ++++++++++++++++++++
>> >  1 file changed, 261 insertions(+)
>> >
>> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
>> > b/drivers/pci/controller/pcie-mediatek-gen3.c
>> > index 7979a2856c35..471d97cd1ef9 100644
>> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
>> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
>> > @@ -14,6 +14,7 @@
>> >  #include <linux/irqdomain.h>
>> >  #include <linux/kernel.h>
>> >  #include <linux/module.h>
>> > +#include <linux/msi.h>
>> >  #include <linux/of_address.h>
>> >  #include <linux/of_clk.h>
>> >  #include <linux/of_pci.h>
>> > @@ -52,11 +53,28 @@
>> >  #define PCIE_LINK_STATUS_REG		0x154
>> >  #define PCIE_PORT_LINKUP		BIT(8)
>> >
>> > +#define PCIE_MSI_SET_NUM		8
>> > +#define PCIE_MSI_IRQS_PER_SET		32
>> > +#define PCIE_MSI_IRQS_NUM \
>> > +	(PCIE_MSI_IRQS_PER_SET * (PCIE_MSI_SET_NUM))
>> 
>> Spurious inner bracketing.
>> 
>> > +
>> >  #define PCIE_INT_ENABLE_REG		0x180
>> > +#define PCIE_MSI_ENABLE			GENMASK(PCIE_MSI_SET_NUM + 8 - 1, 8)
>> > +#define PCIE_MSI_SHIFT			8
>> >  #define PCIE_INTX_SHIFT			24
>> >  #define PCIE_INTX_MASK			GENMASK(27, 24)
>> >
>> >  #define PCIE_INT_STATUS_REG		0x184
>> > +#define PCIE_MSI_SET_ENABLE_REG		0x190
>> > +#define PCIE_MSI_SET_ENABLE		GENMASK(PCIE_MSI_SET_NUM - 1, 0)
>> > +
>> > +#define PCIE_MSI_SET_BASE_REG		0xc00
>> > +#define PCIE_MSI_SET_OFFSET		0x10
>> > +#define PCIE_MSI_SET_STATUS_OFFSET	0x04
>> > +#define PCIE_MSI_SET_ENABLE_OFFSET	0x08
>> > +
>> > +#define PCIE_MSI_SET_ADDR_HI_BASE	0xc80
>> > +#define PCIE_MSI_SET_ADDR_HI_OFFSET	0x04
>> >
>> >  #define PCIE_TRANS_TABLE_BASE_REG	0x800
>> >  #define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
>> > @@ -76,6 +94,18 @@
>> >  #define PCIE_ATR_TLP_TYPE_MEM		PCIE_ATR_TLP_TYPE(0)
>> >  #define PCIE_ATR_TLP_TYPE_IO		PCIE_ATR_TLP_TYPE(2)
>> >
>> > +/**
>> > + * struct mtk_pcie_msi - MSI information for each set
>> > + * @dev: pointer to PCIe device
>> > + * @base: IO mapped register base
>> > + * @msg_addr: MSI message address
>> > + */
>> > +struct mtk_msi_set {
>> > +	struct device *dev;
>> > +	void __iomem *base;
>> > +	phys_addr_t msg_addr;
>> > +};
>> > +
>> >  /**
>> >   * struct mtk_pcie_port - PCIe port information
>> >   * @dev: pointer to PCIe device
>> > @@ -88,6 +118,11 @@
>> >   * @num_clks: PCIe clocks count for this port
>> >   * @irq: PCIe controller interrupt number
>> >   * @intx_domain: legacy INTx IRQ domain
>> > + * @msi_domain: MSI IRQ domain
>> > + * @msi_bottom_domain: MSI IRQ bottom domain
>> > + * @msi_sets: MSI sets information
>> > + * @lock: lock protecting IRQ bit map
>> > + * @msi_irq_in_use: bit map for assigned MSI IRQ
>> >   */
>> >  struct mtk_pcie_port {
>> >  	struct device *dev;
>> > @@ -101,6 +136,11 @@ struct mtk_pcie_port {
>> >
>> >  	int irq;
>> >  	struct irq_domain *intx_domain;
>> > +	struct irq_domain *msi_domain;
>> > +	struct irq_domain *msi_bottom_domain;
>> > +	struct mtk_msi_set msi_sets[PCIE_MSI_SET_NUM];
>> > +	struct mutex lock;
>> > +	DECLARE_BITMAP(msi_irq_in_use, PCIE_MSI_IRQS_NUM);
>> >  };
>> >
>> >  /**
>> > @@ -243,6 +283,15 @@ static int mtk_pcie_startup_port(struct
>> > mtk_pcie_port *port)
>> >  		return err;
>> >  	}
>> >
>> > +	/* Enable MSI */
>> > +	val = readl_relaxed(port->base + PCIE_MSI_SET_ENABLE_REG);
>> > +	val |= PCIE_MSI_SET_ENABLE;
>> > +	writel_relaxed(val, port->base + PCIE_MSI_SET_ENABLE_REG);
>> > +
>> > +	val = readl_relaxed(port->base + PCIE_INT_ENABLE_REG);
>> > +	val |= PCIE_MSI_ENABLE;
>> > +	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
>> > +
>> >  	/* Set PCIe translation windows */
>> >  	resource_list_for_each_entry(entry, &host->windows) {
>> >  		struct resource *res = entry->res;
>> > @@ -286,6 +335,129 @@ static int mtk_pcie_set_affinity(struct irq_data
>> > *data,
>> >  	return -EINVAL;
>> >  }
>> >
>> > +static struct irq_chip mtk_msi_irq_chip = {
>> > +	.name = "MSI",
>> > +	.irq_ack = irq_chip_ack_parent,
>> > +};
>> > +
>> > +static struct msi_domain_info mtk_msi_domain_info = {
>> > +	.flags		= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_PCI_MSIX |
>> > +			   MSI_FLAG_USE_DEF_CHIP_OPS | MSI_FLAG_MULTI_PCI_MSI),
>> > +	.chip		= &mtk_msi_irq_chip,
>> > +};
>> > +
>> > +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg
>> > *msg)
>> > +{
>> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
>> > +	unsigned long hwirq;
>> > +
>> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
>> > +
>> > +	msg->address_hi = upper_32_bits(msi_set->msg_addr);
>> > +	msg->address_lo = lower_32_bits(msi_set->msg_addr);
>> > +	msg->data = hwirq;
>> > +	dev_dbg(msi_set->dev, "msi#%#lx address_hi %#x address_lo %#x data
>> > %d\n",
>> > +		hwirq, msg->address_hi, msg->address_lo, msg->data);
>> > +}
>> > +
>> > +static void mtk_msi_bottom_irq_ack(struct irq_data *data)
>> > +{
>> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
>> > +	unsigned long hwirq;
>> > +
>> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
>> > +
>> > +	writel_relaxed(BIT(hwirq), msi_set->base +
>> > PCIE_MSI_SET_STATUS_OFFSET);
>> > +}
>> > +
>> > +static struct irq_chip mtk_msi_bottom_irq_chip = {
>> > +	.irq_ack		= mtk_msi_bottom_irq_ack,
>> > +	.irq_compose_msi_msg	= mtk_compose_msi_msg,
>> > +	.irq_set_affinity	= mtk_pcie_set_affinity,
>> > +	.name			= "PCIe",
>> 
>> nit: "MSI", rather than "PCIe".
>> 
>> > +};
>> > +
>> > +static int mtk_msi_bottom_domain_alloc(struct irq_domain *domain,
>> > +				       unsigned int virq, unsigned int nr_irqs,
>> > +				       void *arg)
>> > +{
>> > +	struct mtk_pcie_port *port = domain->host_data;
>> > +	struct mtk_msi_set *msi_set;
>> > +	int i, hwirq, set_idx;
>> > +
>> > +	mutex_lock(&port->lock);
>> > +
>> > +	hwirq = bitmap_find_free_region(port->msi_irq_in_use,
>> > PCIE_MSI_IRQS_NUM,
>> > +					order_base_2(nr_irqs));
>> > +
>> > +	mutex_unlock(&port->lock);
>> > +
>> > +	if (hwirq < 0)
>> > +		return -ENOSPC;
>> > +
>> > +	set_idx = hwirq / PCIE_MSI_IRQS_PER_SET;
>> > +	msi_set = &port->msi_sets[set_idx];
>> > +
>> > +	for (i = 0; i < nr_irqs; i++)
>> > +		irq_domain_set_info(domain, virq + i, hwirq + i,
>> > +				    &mtk_msi_bottom_irq_chip, msi_set,
>> > +				    handle_edge_irq, NULL, NULL);
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +static void mtk_msi_bottom_domain_free(struct irq_domain *domain,
>> > +				       unsigned int virq, unsigned int nr_irqs)
>> > +{
>> > +	struct mtk_pcie_port *port = domain->host_data;
>> > +	struct irq_data *data = irq_domain_get_irq_data(domain, virq);
>> > +
>> > +	mutex_lock(&port->lock);
>> > +
>> > +	bitmap_clear(port->msi_irq_in_use, data->hwirq, nr_irqs);
>> > +
>> > +	mutex_unlock(&port->lock);
>> > +
>> > +	irq_domain_free_irqs_common(domain, virq, nr_irqs);
>> > +}
>> > +
>> > +static int mtk_msi_bottom_domain_activate(struct irq_domain *domain,
>> > +					  struct irq_data *data, bool reserve)
>> > +{
>> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
>> > +	unsigned long hwirq;
>> > +	u32 val;
>> > +
>> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
>> > +
>> > +	val = readl_relaxed(msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
>> > +	val |= BIT(hwirq);
>> > +	writel_relaxed(val, msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
>> 
>> This isn't an activate. This is an unmask, which suffers from the same
>> issue as its INTx sibling.
>> 
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +static void mtk_msi_bottom_domain_deactivate(struct irq_domain
>> > *domain,
>> > +					     struct irq_data *data)
>> > +{
>> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
>> > +	unsigned long hwirq;
>> > +	u32 val;
>> > +
>> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
>> > +
>> > +	val = readl_relaxed(msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
>> > +	val &= ~BIT(hwirq);
>> > +	writel_relaxed(val, msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
>> > +}
>> 
>> Same thing, this is a mask. I don't think this block requires any
>> activate/deactivate callbacks for its lower irqdomain.
>> 
>> As it stands, you can't mask a MSI at the low-level, which is
>> pretty bad (you need to mask them at the PCI source, which can
>> end-up disabling all vectors in the case of Multi-MSI).
> 
> Hi Marc,
> 
> Thanks for your review.
> 
> This mtk_msi_bottom_domain is the parent domain of pci_msi_domain, but
> the mask/unmask callback of pci_msi_domain does not call the callback 
> of
> its parent. Therefore if these functions are put in the mask/unmask
> callbacks, they will not have a chance to be called.

It is for you to wire the callbacks in the mtk_msi_irq_chip irqchip
so that the request can be forwarded to the parent, without relying
on the default callbacks:

static void mtk_mask_msi_irq(struct irq_data *d)
{
	pci_msi_mask_irq(d);
	irq_chip_mask_parent(d);
}

static void mtk_unmask_msi_irq(struct irq_data *d)
{
	pci_msi_unmask_irq(d);
	irq_chip_unmask_parent(d);
}

static struct irq_chip mtk_msi_irq_chip = {
        .name = "MSI",
        .irq_mask = mtk_mask_msi_irq,
        .irq_unmask = mtk_unmask_msi_irq,
        .irq_ack = irq_chip_ack_parent,
};

and turn your activate/deactivate into unmask/mask.

>> 
>> > +
>> > +static const struct irq_domain_ops mtk_msi_bottom_domain_ops = {
>> > +	.alloc = mtk_msi_bottom_domain_alloc,
>> > +	.free = mtk_msi_bottom_domain_free,
>> > +	.activate = mtk_msi_bottom_domain_activate,
>> > +	.deactivate = mtk_msi_bottom_domain_deactivate,
>> > +};
>> > +
>> >  static void mtk_intx_mask(struct irq_data *data)
>> >  {
>> >  	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
>> > @@ -350,6 +522,9 @@ static int mtk_pcie_init_irq_domains(struct
>> > mtk_pcie_port *port,
>> >  {
>> >  	struct device *dev = port->dev;
>> >  	struct device_node *intc_node;
>> > +	struct fwnode_handle *fwnode = of_node_to_fwnode(node);
>> > +	struct msi_domain_info *info;
>> > +	int i, ret;
>> >
>> >  	/* Setup INTx */
>> >  	intc_node = of_get_child_by_name(node, "interrupt-controller");
>> > @@ -365,7 +540,57 @@ static int mtk_pcie_init_irq_domains(struct
>> > mtk_pcie_port *port,
>> >  		return -ENODEV;
>> >  	}
>> >
>> > +	/* Setup MSI */
>> > +	mutex_init(&port->lock);
>> > +
>> > +	port->msi_bottom_domain = irq_domain_add_linear(node,
>> > PCIE_MSI_IRQS_NUM,
>> > +				  &mtk_msi_bottom_domain_ops, port);
>> > +	if (!port->msi_bottom_domain) {
>> > +		dev_info(dev, "failed to create MSI bottom domain\n");
>> > +		ret = -ENODEV;
>> > +		goto err_msi_bottom_domain;
>> > +	}
>> > +
>> > +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>> > +	if (!info) {
>> > +		ret = -ENOMEM;
>> > +		goto err_msi_bottom_domain;
>> > +	}
>> > +
>> > +	memcpy(info, &mtk_msi_domain_info, sizeof(*info));
>> 
>> Why the memcpy()? There is nothing in mtk_msi_domain_info that is
>> per-domain, and you should be able to use this structure for all
>> ports, shouldn't you?
> 
> Yes, it used to update the info->chip_data for each port, but since the
> msi_set has been used for msi_bottom_domain ,it has no effect anymore, 
> I
> will drop it in the next version, thanks.
>> 
>> > +	info->chip_data = port;
>> > +
>> > +	port->msi_domain = pci_msi_create_irq_domain(fwnode, info,
>> > +						     port->msi_bottom_domain);
>> > +	if (!port->msi_domain) {
>> > +		dev_info(dev, "failed to create MSI domain\n");
>> > +		ret = -ENODEV;
>> > +		goto err_msi_domain;
>> > +	}
>> > +
>> > +	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
>> > +		struct mtk_msi_set *msi_set = &port->msi_sets[i];
>> > +
>> > +		msi_set->dev = port->dev;
>> 
>> Given that this is only used in a debug message, and that the 
>> addresses
>> are already non-ambiguous, you can probably remove this field.
> 
> OK, I will remove it in the next version.
> 
>> 
>> > +		msi_set->base = port->base + PCIE_MSI_SET_BASE_REG +
>> > +				i * PCIE_MSI_SET_OFFSET;
>> > +		msi_set->msg_addr = port->reg_base + PCIE_MSI_SET_BASE_REG +
>> > +				    i * PCIE_MSI_SET_OFFSET;
>> > +
>> > +		writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
>> > +		writel_relaxed(upper_32_bits(msi_set->msg_addr),
>> > +			       port->base + PCIE_MSI_SET_ADDR_HI_BASE +
>> > +			       i * PCIE_MSI_SET_ADDR_HI_OFFSET);
>> 
>> Please a comment on what this is doing...
> 
> This codes are used to configure the capture address of each MSI set,
> the lower 32 bits of MSI address should be written to msi_set->base, 
> but
> the strange thing is that the address where need to write the higher 32
> bits are not near each set, they are start from
> PCIE_MSI_SET_ADDR_HI_BASE, and have PCIE_MSI_SET_ADDR_HI_OFFSET apart.
> 
> That's why it looks so weired...

OK. Just add a comment saying that this programs the MSI capture 
address.

Thanks,

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

Re: [v7,4/7] PCI: mediatek-gen3: Add INTx support

[Jianjun Wang]

On Tue, 2021-01-26 at 12:25 +0000, Marc Zyngier wrote:
> On 2021-01-13 11:39, Jianjun Wang wrote:
> > Add INTx support for MediaTek Gen3 PCIe controller.
> > 
> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 163 ++++++++++++++++++++
> >  1 file changed, 163 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index c00ea7c167de..7979a2856c35 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -9,6 +9,9 @@
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> >  #include <linux/iopoll.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +#include <linux/irqdomain.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/of_address.h>
> > @@ -49,6 +52,12 @@
> >  #define PCIE_LINK_STATUS_REG		0x154
> >  #define PCIE_PORT_LINKUP		BIT(8)
> > 
> > +#define PCIE_INT_ENABLE_REG		0x180
> > +#define PCIE_INTX_SHIFT			24
> > +#define PCIE_INTX_MASK			GENMASK(27, 24)
> 
> I guess this '24' is actually PCIE_INTX_SHIFT? In this case,
> please write it as
> 
> GENMASK(PCIE_INTX_SHIFT + PCI_NUM_INTX - 1, PCIE_INTX_SHIFT)
> 
> to make it clear that you are dealing with one bit per INTx.

Yes, I will fix it in the next version, thanks for your review.
> 
> > +
> > +#define PCIE_INT_STATUS_REG		0x184
> > +
> >  #define PCIE_TRANS_TABLE_BASE_REG	0x800
> >  #define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
> >  #define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8
> > @@ -77,6 +86,8 @@
> >   * @phy: PHY controller block
> >   * @clks: PCIe clocks
> >   * @num_clks: PCIe clocks count for this port
> > + * @irq: PCIe controller interrupt number
> > + * @intx_domain: legacy INTx IRQ domain
> >   */
> >  struct mtk_pcie_port {
> >  	struct device *dev;
> > @@ -87,6 +98,9 @@ struct mtk_pcie_port {
> >  	struct phy *phy;
> >  	struct clk_bulk_data *clks;
> >  	int num_clks;
> > +
> > +	int irq;
> > +	struct irq_domain *intx_domain;
> >  };
> > 
> >  /**
> > @@ -266,6 +280,149 @@ static int mtk_pcie_startup_port(struct
> > mtk_pcie_port *port)
> >  	return 0;
> >  }
> > 
> > +static int mtk_pcie_set_affinity(struct irq_data *data,
> > +				 const struct cpumask *mask, bool force)
> > +{
> > +	return -EINVAL;
> > +}
> > +
> > +static void mtk_intx_mask(struct irq_data *data)
> > +{
> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> > +	u32 val;
> > +
> > +	val = readl_relaxed(port->base + PCIE_INT_ENABLE_REG);
> > +	val &= ~BIT(data->hwirq + PCIE_INTX_SHIFT);
> > +	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
> 
> This is missing some locking. Otherwise, two concurrent mask/unmask
> for different interrupts will corrupt each other's state.
> 
> > +}
> > +
> > +static void mtk_intx_unmask(struct irq_data *data)
> > +{
> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> > +	u32 val;
> > +
> > +	val = readl_relaxed(port->base + PCIE_INT_ENABLE_REG);
> > +	val |= BIT(data->hwirq + PCIE_INTX_SHIFT);
> > +	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
> 
> Same thing here.
> 
> > +}
> > +
> > +/**
> > + * mtk_intx_eoi
> > + * @data: pointer to chip specific data
> > + *
> > + * As an emulated level IRQ, its interrupt status will remain
> > + * until the corresponding de-assert message is received; hence that
> > + * the status can only be cleared when the interrupt has been 
> > serviced.
> > + */
> > +static void mtk_intx_eoi(struct irq_data *data)
> > +{
> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> > +	unsigned long hwirq;
> > +
> > +	hwirq = data->hwirq + PCIE_INTX_SHIFT;
> > +	writel_relaxed(BIT(hwirq), port->base + PCIE_INT_STATUS_REG);
> > +}
> > +
> > +static struct irq_chip mtk_intx_irq_chip = {
> > +	.irq_mask		= mtk_intx_mask,
> > +	.irq_unmask		= mtk_intx_unmask,
> > +	.irq_eoi		= mtk_intx_eoi,
> > +	.irq_set_affinity	= mtk_pcie_set_affinity,
> > +	.name			= "PCIe",
> 
> nit: "PCIe" is not really descriptive. "INTx" would be a bit better.
> 
> > +};
> > +
> > +static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int 
> > irq,
> > +			     irq_hw_number_t hwirq)
> > +{
> > +	irq_set_chip_and_handler_name(irq, &mtk_intx_irq_chip,
> > +				      handle_fasteoi_irq, "INTx");
> > +	irq_set_chip_data(irq, domain->host_data);
> 
> You probably want to set the chip_data *before* wiring
> the handler, as otherwise you could end-up with a NULL
> pointer in any of the callbacks if the interrupt fires
> between the two.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops intx_domain_ops = {
> > +	.map = mtk_pcie_intx_map,
> > +};
> > +
> > +static int mtk_pcie_init_irq_domains(struct mtk_pcie_port *port,
> > +				     struct device_node *node)
> > +{
> > +	struct device *dev = port->dev;
> > +	struct device_node *intc_node;
> > +
> > +	/* Setup INTx */
> > +	intc_node = of_get_child_by_name(node, "interrupt-controller");
> > +	if (!intc_node) {
> > +		dev_err(dev, "missing PCIe Intc node\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	port->intx_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX,
> > +						  &intx_domain_ops, port);
> > +	if (!port->intx_domain) {
> > +		dev_err(dev, "failed to get INTx IRQ domain\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void mtk_pcie_irq_teardown(struct mtk_pcie_port *port)
> > +{
> > +	irq_set_chained_handler_and_data(port->irq, NULL, NULL);
> > +
> > +	if (port->intx_domain)
> > +		irq_domain_remove(port->intx_domain);
> > +
> > +	irq_dispose_mapping(port->irq);
> > +}
> > +
> > +static void mtk_pcie_irq_handler(struct irq_desc *desc)
> > +{
> > +	struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);
> > +	struct irq_chip *irqchip = irq_desc_get_chip(desc);
> > +	unsigned long status;
> > +	unsigned int virq;
> > +	irq_hw_number_t irq_bit = PCIE_INTX_SHIFT;
> > +
> > +	chained_irq_enter(irqchip, desc);
> > +
> > +	status = readl_relaxed(port->base + PCIE_INT_STATUS_REG);
> > +	if (status & PCIE_INTX_MASK) {
> 
> This "if (status & PCIE_INTX_MASK)" is already implicit from
> the for_each_set_bit_from() iterator, and you can drop it.
> 
> > +		for_each_set_bit_from(irq_bit, &status, PCI_NUM_INTX +
> > +				      PCIE_INTX_SHIFT) {
> > +			virq = irq_find_mapping(port->intx_domain,
> > +						irq_bit - PCIE_INTX_SHIFT);
> > +			generic_handle_irq(virq);
> > +		}
> > +	}
> > +
> > +	chained_irq_exit(irqchip, desc);
> > +}
> > +
> > +static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
> > +			      struct device_node *node)
> > +{
> > +	struct device *dev = port->dev;
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	int err;
> > +
> > +	err = mtk_pcie_init_irq_domains(port, node);
> > +	if (err) {
> > +		dev_err(dev, "failed to init PCIe IRQ domain\n");
> > +		return err;
> > +	}
> > +
> > +	port->irq = platform_get_irq(pdev, 0);
> > +	if (port->irq < 0)
> > +		return port->irq;
> > +
> > +	irq_set_chained_handler_and_data(port->irq, mtk_pcie_irq_handler, 
> > port);
> 
> You seem to be missing something that will mask all INTx interrupts
> as an initial state.

Yes, the INTx default state of this chip is disabled, but there are
still risks for driver use, I will add mask all INTx interrupts to
mtk_pcie_startup_port function in the next version.

Thanks.
> 
> > +
> > +	return 0;
> > +}
> > +
> >  static int mtk_pcie_clk_init(struct mtk_pcie_port *port)
> >  {
> >  	int ret;
> > @@ -388,6 +545,10 @@ static int mtk_pcie_setup(struct mtk_pcie_port 
> > *port)
> >  		goto err_setup;
> >  	}
> > 
> > +	err = mtk_pcie_setup_irq(port, dev->of_node);
> > +	if (err)
> > +		goto err_setup;
> > +
> >  	dev_info(dev, "PCIe link up success!\n");
> > 
> >  	return 0;
> > @@ -423,6 +584,7 @@ static int mtk_pcie_probe(struct platform_device 
> > *pdev)
> > 
> >  	err = pci_host_probe(host);
> >  	if (err) {
> > +		mtk_pcie_irq_teardown(port);
> >  		mtk_pcie_power_down(port);
> >  		return err;
> >  	}
> > @@ -440,6 +602,7 @@ static int mtk_pcie_remove(struct platform_device 
> > *pdev)
> >  	pci_remove_root_bus(host->bus);
> >  	pci_unlock_rescan_remove();
> > 
> > +	mtk_pcie_irq_teardown(port);
> >  	mtk_pcie_power_down(port);
> > 
> >  	return 0;
> 
> Thanks,
> 
>          M.

Re: [v7,5/7] PCI: mediatek-gen3: Add MSI support

[Jianjun Wang]

On Wed, 2021-01-27 at 13:05 +0000, Marc Zyngier wrote:
> On 2021-01-27 12:31, Jianjun Wang wrote:
> > On Tue, 2021-01-26 at 13:57 +0000, Marc Zyngier wrote:
> >> On 2021-01-13 11:39, Jianjun Wang wrote:
> >> > Add MSI support for MediaTek Gen3 PCIe controller.
> >> >
> >> > This PCIe controller supports up to 256 MSI vectors, the MSI hardware
> >> > block diagram is as follows:
> >> >
> >> >                   +-----+
> >> >                   | GIC |
> >> >                   +-----+
> >> >                      ^
> >> >                      |
> >> >                  port->irq
> >> >                      |
> >> >              +-+-+-+-+-+-+-+-+
> >> >              |0|1|2|3|4|5|6|7| (PCIe intc)
> >> >              +-+-+-+-+-+-+-+-+
> >> >               ^ ^           ^
> >> >               | |    ...    |
> >> >       +-------+ +------+    +-----------+
> >> >       |                |                |
> >> > +-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
> >> > |0|1|...|30|31|  |0|1|...|30|31|  |0|1|...|30|31| (MSI sets)
> >> > +-+-+---+--+--+  +-+-+---+--+--+  +-+-+---+--+--+
> >> >  ^ ^      ^  ^    ^ ^      ^  ^    ^ ^      ^  ^
> >> >  | |      |  |    | |      |  |    | |      |  |  (MSI vectors)
> >> >  | |      |  |    | |      |  |    | |      |  |
> >> >
> >> >   (MSI SET0)       (MSI SET1)  ...   (MSI SET7)
> >> >
> >> > With 256 MSI vectors supported, the MSI vectors are composed of 8 sets,
> >> > each set has its own address for MSI message, and supports 32 MSI
> >> > vectors
> >> > to generate interrupt.
> >> >
> >> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> >> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> >> > ---
> >> >  drivers/pci/controller/pcie-mediatek-gen3.c | 261 ++++++++++++++++++++
> >> >  1 file changed, 261 insertions(+)
> >> >
> >> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> >> > b/drivers/pci/controller/pcie-mediatek-gen3.c
> >> > index 7979a2856c35..471d97cd1ef9 100644
> >> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> >> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> >> > @@ -14,6 +14,7 @@
> >> >  #include <linux/irqdomain.h>
> >> >  #include <linux/kernel.h>
> >> >  #include <linux/module.h>
> >> > +#include <linux/msi.h>
> >> >  #include <linux/of_address.h>
> >> >  #include <linux/of_clk.h>
> >> >  #include <linux/of_pci.h>
> >> > @@ -52,11 +53,28 @@
> >> >  #define PCIE_LINK_STATUS_REG		0x154
> >> >  #define PCIE_PORT_LINKUP		BIT(8)
> >> >
> >> > +#define PCIE_MSI_SET_NUM		8
> >> > +#define PCIE_MSI_IRQS_PER_SET		32
> >> > +#define PCIE_MSI_IRQS_NUM \
> >> > +	(PCIE_MSI_IRQS_PER_SET * (PCIE_MSI_SET_NUM))
> >> 
> >> Spurious inner bracketing.
> >> 
> >> > +
> >> >  #define PCIE_INT_ENABLE_REG		0x180
> >> > +#define PCIE_MSI_ENABLE			GENMASK(PCIE_MSI_SET_NUM + 8 - 1, 8)
> >> > +#define PCIE_MSI_SHIFT			8
> >> >  #define PCIE_INTX_SHIFT			24
> >> >  #define PCIE_INTX_MASK			GENMASK(27, 24)
> >> >
> >> >  #define PCIE_INT_STATUS_REG		0x184
> >> > +#define PCIE_MSI_SET_ENABLE_REG		0x190
> >> > +#define PCIE_MSI_SET_ENABLE		GENMASK(PCIE_MSI_SET_NUM - 1, 0)
> >> > +
> >> > +#define PCIE_MSI_SET_BASE_REG		0xc00
> >> > +#define PCIE_MSI_SET_OFFSET		0x10
> >> > +#define PCIE_MSI_SET_STATUS_OFFSET	0x04
> >> > +#define PCIE_MSI_SET_ENABLE_OFFSET	0x08
> >> > +
> >> > +#define PCIE_MSI_SET_ADDR_HI_BASE	0xc80
> >> > +#define PCIE_MSI_SET_ADDR_HI_OFFSET	0x04
> >> >
> >> >  #define PCIE_TRANS_TABLE_BASE_REG	0x800
> >> >  #define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
> >> > @@ -76,6 +94,18 @@
> >> >  #define PCIE_ATR_TLP_TYPE_MEM		PCIE_ATR_TLP_TYPE(0)
> >> >  #define PCIE_ATR_TLP_TYPE_IO		PCIE_ATR_TLP_TYPE(2)
> >> >
> >> > +/**
> >> > + * struct mtk_pcie_msi - MSI information for each set
> >> > + * @dev: pointer to PCIe device
> >> > + * @base: IO mapped register base
> >> > + * @msg_addr: MSI message address
> >> > + */
> >> > +struct mtk_msi_set {
> >> > +	struct device *dev;
> >> > +	void __iomem *base;
> >> > +	phys_addr_t msg_addr;
> >> > +};
> >> > +
> >> >  /**
> >> >   * struct mtk_pcie_port - PCIe port information
> >> >   * @dev: pointer to PCIe device
> >> > @@ -88,6 +118,11 @@
> >> >   * @num_clks: PCIe clocks count for this port
> >> >   * @irq: PCIe controller interrupt number
> >> >   * @intx_domain: legacy INTx IRQ domain
> >> > + * @msi_domain: MSI IRQ domain
> >> > + * @msi_bottom_domain: MSI IRQ bottom domain
> >> > + * @msi_sets: MSI sets information
> >> > + * @lock: lock protecting IRQ bit map
> >> > + * @msi_irq_in_use: bit map for assigned MSI IRQ
> >> >   */
> >> >  struct mtk_pcie_port {
> >> >  	struct device *dev;
> >> > @@ -101,6 +136,11 @@ struct mtk_pcie_port {
> >> >
> >> >  	int irq;
> >> >  	struct irq_domain *intx_domain;
> >> > +	struct irq_domain *msi_domain;
> >> > +	struct irq_domain *msi_bottom_domain;
> >> > +	struct mtk_msi_set msi_sets[PCIE_MSI_SET_NUM];
> >> > +	struct mutex lock;
> >> > +	DECLARE_BITMAP(msi_irq_in_use, PCIE_MSI_IRQS_NUM);
> >> >  };
> >> >
> >> >  /**
> >> > @@ -243,6 +283,15 @@ static int mtk_pcie_startup_port(struct
> >> > mtk_pcie_port *port)
> >> >  		return err;
> >> >  	}
> >> >
> >> > +	/* Enable MSI */
> >> > +	val = readl_relaxed(port->base + PCIE_MSI_SET_ENABLE_REG);
> >> > +	val |= PCIE_MSI_SET_ENABLE;
> >> > +	writel_relaxed(val, port->base + PCIE_MSI_SET_ENABLE_REG);
> >> > +
> >> > +	val = readl_relaxed(port->base + PCIE_INT_ENABLE_REG);
> >> > +	val |= PCIE_MSI_ENABLE;
> >> > +	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
> >> > +
> >> >  	/* Set PCIe translation windows */
> >> >  	resource_list_for_each_entry(entry, &host->windows) {
> >> >  		struct resource *res = entry->res;
> >> > @@ -286,6 +335,129 @@ static int mtk_pcie_set_affinity(struct irq_data
> >> > *data,
> >> >  	return -EINVAL;
> >> >  }
> >> >
> >> > +static struct irq_chip mtk_msi_irq_chip = {
> >> > +	.name = "MSI",
> >> > +	.irq_ack = irq_chip_ack_parent,
> >> > +};
> >> > +
> >> > +static struct msi_domain_info mtk_msi_domain_info = {
> >> > +	.flags		= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_PCI_MSIX |
> >> > +			   MSI_FLAG_USE_DEF_CHIP_OPS | MSI_FLAG_MULTI_PCI_MSI),
> >> > +	.chip		= &mtk_msi_irq_chip,
> >> > +};
> >> > +
> >> > +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg
> >> > *msg)
> >> > +{
> >> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> >> > +	unsigned long hwirq;
> >> > +
> >> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> >> > +
> >> > +	msg->address_hi = upper_32_bits(msi_set->msg_addr);
> >> > +	msg->address_lo = lower_32_bits(msi_set->msg_addr);
> >> > +	msg->data = hwirq;
> >> > +	dev_dbg(msi_set->dev, "msi#%#lx address_hi %#x address_lo %#x data
> >> > %d\n",
> >> > +		hwirq, msg->address_hi, msg->address_lo, msg->data);
> >> > +}
> >> > +
> >> > +static void mtk_msi_bottom_irq_ack(struct irq_data *data)
> >> > +{
> >> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> >> > +	unsigned long hwirq;
> >> > +
> >> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> >> > +
> >> > +	writel_relaxed(BIT(hwirq), msi_set->base +
> >> > PCIE_MSI_SET_STATUS_OFFSET);
> >> > +}
> >> > +
> >> > +static struct irq_chip mtk_msi_bottom_irq_chip = {
> >> > +	.irq_ack		= mtk_msi_bottom_irq_ack,
> >> > +	.irq_compose_msi_msg	= mtk_compose_msi_msg,
> >> > +	.irq_set_affinity	= mtk_pcie_set_affinity,
> >> > +	.name			= "PCIe",
> >> 
> >> nit: "MSI", rather than "PCIe".
> >> 
> >> > +};
> >> > +
> >> > +static int mtk_msi_bottom_domain_alloc(struct irq_domain *domain,
> >> > +				       unsigned int virq, unsigned int nr_irqs,
> >> > +				       void *arg)
> >> > +{
> >> > +	struct mtk_pcie_port *port = domain->host_data;
> >> > +	struct mtk_msi_set *msi_set;
> >> > +	int i, hwirq, set_idx;
> >> > +
> >> > +	mutex_lock(&port->lock);
> >> > +
> >> > +	hwirq = bitmap_find_free_region(port->msi_irq_in_use,
> >> > PCIE_MSI_IRQS_NUM,
> >> > +					order_base_2(nr_irqs));
> >> > +
> >> > +	mutex_unlock(&port->lock);
> >> > +
> >> > +	if (hwirq < 0)
> >> > +		return -ENOSPC;
> >> > +
> >> > +	set_idx = hwirq / PCIE_MSI_IRQS_PER_SET;
> >> > +	msi_set = &port->msi_sets[set_idx];
> >> > +
> >> > +	for (i = 0; i < nr_irqs; i++)
> >> > +		irq_domain_set_info(domain, virq + i, hwirq + i,
> >> > +				    &mtk_msi_bottom_irq_chip, msi_set,
> >> > +				    handle_edge_irq, NULL, NULL);
> >> > +
> >> > +	return 0;
> >> > +}
> >> > +
> >> > +static void mtk_msi_bottom_domain_free(struct irq_domain *domain,
> >> > +				       unsigned int virq, unsigned int nr_irqs)
> >> > +{
> >> > +	struct mtk_pcie_port *port = domain->host_data;
> >> > +	struct irq_data *data = irq_domain_get_irq_data(domain, virq);
> >> > +
> >> > +	mutex_lock(&port->lock);
> >> > +
> >> > +	bitmap_clear(port->msi_irq_in_use, data->hwirq, nr_irqs);
> >> > +
> >> > +	mutex_unlock(&port->lock);
> >> > +
> >> > +	irq_domain_free_irqs_common(domain, virq, nr_irqs);
> >> > +}
> >> > +
> >> > +static int mtk_msi_bottom_domain_activate(struct irq_domain *domain,
> >> > +					  struct irq_data *data, bool reserve)
> >> > +{
> >> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> >> > +	unsigned long hwirq;
> >> > +	u32 val;
> >> > +
> >> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> >> > +
> >> > +	val = readl_relaxed(msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
> >> > +	val |= BIT(hwirq);
> >> > +	writel_relaxed(val, msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
> >> 
> >> This isn't an activate. This is an unmask, which suffers from the same
> >> issue as its INTx sibling.
> >> 
> >> > +
> >> > +	return 0;
> >> > +}
> >> > +
> >> > +static void mtk_msi_bottom_domain_deactivate(struct irq_domain
> >> > *domain,
> >> > +					     struct irq_data *data)
> >> > +{
> >> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> >> > +	unsigned long hwirq;
> >> > +	u32 val;
> >> > +
> >> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> >> > +
> >> > +	val = readl_relaxed(msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
> >> > +	val &= ~BIT(hwirq);
> >> > +	writel_relaxed(val, msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
> >> > +}
> >> 
> >> Same thing, this is a mask. I don't think this block requires any
> >> activate/deactivate callbacks for its lower irqdomain.
> >> 
> >> As it stands, you can't mask a MSI at the low-level, which is
> >> pretty bad (you need to mask them at the PCI source, which can
> >> end-up disabling all vectors in the case of Multi-MSI).
> > 
> > Hi Marc,
> > 
> > Thanks for your review.
> > 
> > This mtk_msi_bottom_domain is the parent domain of pci_msi_domain, but
> > the mask/unmask callback of pci_msi_domain does not call the callback 
> > of
> > its parent. Therefore if these functions are put in the mask/unmask
> > callbacks, they will not have a chance to be called.
> 
> It is for you to wire the callbacks in the mtk_msi_irq_chip irqchip
> so that the request can be forwarded to the parent, without relying
> on the default callbacks:
> 
> static void mtk_mask_msi_irq(struct irq_data *d)
> {
> 	pci_msi_mask_irq(d);
> 	irq_chip_mask_parent(d);
> }
> 
> static void mtk_unmask_msi_irq(struct irq_data *d)
> {
> 	pci_msi_unmask_irq(d);
> 	irq_chip_unmask_parent(d);
> }
> 
> static struct irq_chip mtk_msi_irq_chip = {
>         .name = "MSI",
>         .irq_mask = mtk_mask_msi_irq,
>         .irq_unmask = mtk_unmask_msi_irq,
>         .irq_ack = irq_chip_ack_parent,
> };
> 
> and turn your activate/deactivate into unmask/mask.

I will fix it in the next version ,thanks a lot.
> 
> >> 
> >> > +
> >> > +static const struct irq_domain_ops mtk_msi_bottom_domain_ops = {
> >> > +	.alloc = mtk_msi_bottom_domain_alloc,
> >> > +	.free = mtk_msi_bottom_domain_free,
> >> > +	.activate = mtk_msi_bottom_domain_activate,
> >> > +	.deactivate = mtk_msi_bottom_domain_deactivate,
> >> > +};
> >> > +
> >> >  static void mtk_intx_mask(struct irq_data *data)
> >> >  {
> >> >  	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> >> > @@ -350,6 +522,9 @@ static int mtk_pcie_init_irq_domains(struct
> >> > mtk_pcie_port *port,
> >> >  {
> >> >  	struct device *dev = port->dev;
> >> >  	struct device_node *intc_node;
> >> > +	struct fwnode_handle *fwnode = of_node_to_fwnode(node);
> >> > +	struct msi_domain_info *info;
> >> > +	int i, ret;
> >> >
> >> >  	/* Setup INTx */
> >> >  	intc_node = of_get_child_by_name(node, "interrupt-controller");
> >> > @@ -365,7 +540,57 @@ static int mtk_pcie_init_irq_domains(struct
> >> > mtk_pcie_port *port,
> >> >  		return -ENODEV;
> >> >  	}
> >> >
> >> > +	/* Setup MSI */
> >> > +	mutex_init(&port->lock);
> >> > +
> >> > +	port->msi_bottom_domain = irq_domain_add_linear(node,
> >> > PCIE_MSI_IRQS_NUM,
> >> > +				  &mtk_msi_bottom_domain_ops, port);
> >> > +	if (!port->msi_bottom_domain) {
> >> > +		dev_info(dev, "failed to create MSI bottom domain\n");
> >> > +		ret = -ENODEV;
> >> > +		goto err_msi_bottom_domain;
> >> > +	}
> >> > +
> >> > +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> >> > +	if (!info) {
> >> > +		ret = -ENOMEM;
> >> > +		goto err_msi_bottom_domain;
> >> > +	}
> >> > +
> >> > +	memcpy(info, &mtk_msi_domain_info, sizeof(*info));
> >> 
> >> Why the memcpy()? There is nothing in mtk_msi_domain_info that is
> >> per-domain, and you should be able to use this structure for all
> >> ports, shouldn't you?
> > 
> > Yes, it used to update the info->chip_data for each port, but since the
> > msi_set has been used for msi_bottom_domain ,it has no effect anymore, 
> > I
> > will drop it in the next version, thanks.
> >> 
> >> > +	info->chip_data = port;
> >> > +
> >> > +	port->msi_domain = pci_msi_create_irq_domain(fwnode, info,
> >> > +						     port->msi_bottom_domain);
> >> > +	if (!port->msi_domain) {
> >> > +		dev_info(dev, "failed to create MSI domain\n");
> >> > +		ret = -ENODEV;
> >> > +		goto err_msi_domain;
> >> > +	}
> >> > +
> >> > +	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
> >> > +		struct mtk_msi_set *msi_set = &port->msi_sets[i];
> >> > +
> >> > +		msi_set->dev = port->dev;
> >> 
> >> Given that this is only used in a debug message, and that the 
> >> addresses
> >> are already non-ambiguous, you can probably remove this field.
> > 
> > OK, I will remove it in the next version.
> > 
> >> 
> >> > +		msi_set->base = port->base + PCIE_MSI_SET_BASE_REG +
> >> > +				i * PCIE_MSI_SET_OFFSET;
> >> > +		msi_set->msg_addr = port->reg_base + PCIE_MSI_SET_BASE_REG +
> >> > +				    i * PCIE_MSI_SET_OFFSET;
> >> > +
> >> > +		writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
> >> > +		writel_relaxed(upper_32_bits(msi_set->msg_addr),
> >> > +			       port->base + PCIE_MSI_SET_ADDR_HI_BASE +
> >> > +			       i * PCIE_MSI_SET_ADDR_HI_OFFSET);
> >> 
> >> Please a comment on what this is doing...
> > 
> > This codes are used to configure the capture address of each MSI set,
> > the lower 32 bits of MSI address should be written to msi_set->base, 
> > but
> > the strange thing is that the address where need to write the higher 32
> > bits are not near each set, they are start from
> > PCIE_MSI_SET_ADDR_HI_BASE, and have PCIE_MSI_SET_ADDR_HI_OFFSET apart.
> > 
> > That's why it looks so weired...
> 
> OK. Just add a comment saying that this programs the MSI capture 
> address.
> 
> Thanks,
> 
>          M.