linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [v8,0/7] PCI: mediatek: Add new generation controller support
@ 2021-02-24  6:11 Jianjun Wang
  2021-02-24  6:11 ` [v8,1/7] dt-bindings: PCI: mediatek-gen3: Add YAML schema Jianjun Wang
                   ` (6 more replies)
  0 siblings, 7 replies; 35+ messages in thread
From: Jianjun Wang @ 2021-02-24  6:11 UTC (permalink / raw)
  To: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee
  Cc: Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang,
	Jianjun Wang, youlin.pei, chuanjia.liu, qizhong.cheng,
	sin_jieyang, drinkcat, Rex-BC.Chen, anson.chuang

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

Changes in v8:
1. Add irq_clock to protect IRQ register access;
2. Mask all INTx interrupt when startup port;
3. Remove activate/deactivate callbacks from bottom_domain_ops;
4. Add unmask/mask callbacks in mtk_msi_bottom_irq_chip;
5. Add property information for reg-names.

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      | 181 ++++
 MAINTAINERS                                   |   1 +
 drivers/pci/controller/Kconfig                |  13 +
 drivers/pci/controller/Makefile               |   1 +
 drivers/pci/controller/pcie-mediatek-gen3.c   | 994 ++++++++++++++++++
 drivers/pci/pci.c                             |   1 +
 6 files changed, 1191 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


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

* [v8,1/7] dt-bindings: PCI: mediatek-gen3: Add YAML schema
  2021-02-24  6:11 [v8,0/7] PCI: mediatek: Add new generation controller support Jianjun Wang
@ 2021-02-24  6:11 ` Jianjun Wang
  2021-03-06 20:09   ` Rob Herring
  2021-02-24  6:11 ` [v8,2/7] PCI: Export pci_pio_to_address() for module use Jianjun Wang
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 35+ messages in thread
From: Jianjun Wang @ 2021-02-24  6:11 UTC (permalink / raw)
  To: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee
  Cc: Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang,
	Jianjun Wang, youlin.pei, chuanjia.liu, qizhong.cheng,
	sin_jieyang, drinkcat, Rex-BC.Chen, anson.chuang

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      | 181 ++++++++++++++++++
 1 file changed, 181 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..e7b1f9892da4
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
@@ -0,0 +1,181 @@
+# 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
+
+  reg-names:
+    items:
+      - const: pcie-mac
+
+  interrupts:
+    maxItems: 1
+
+  ranges:
+    minItems: 1
+    maxItems: 8
+
+  resets:
+    minItems: 1
+    maxItems: 2
+
+  reset-names:
+    minItems: 1
+    maxItems: 2
+    items:
+      - const: phy
+      - const: mac
+
+  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
+  - reg-names
+  - 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 2>,
+                     <&infracfg_rst 3>;
+            reset-names = "phy", "mac";
+
+            #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


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

* [v8,2/7] PCI: Export pci_pio_to_address() for module use
  2021-02-24  6:11 [v8,0/7] PCI: mediatek: Add new generation controller support Jianjun Wang
  2021-02-24  6:11 ` [v8,1/7] dt-bindings: PCI: mediatek-gen3: Add YAML schema Jianjun Wang
@ 2021-02-24  6:11 ` Jianjun Wang
  2021-02-24  6:11 ` [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192 Jianjun Wang
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 35+ messages in thread
From: Jianjun Wang @ 2021-02-24  6:11 UTC (permalink / raw)
  To: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee
  Cc: Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang,
	Jianjun Wang, youlin.pei, chuanjia.liu, qizhong.cheng,
	sin_jieyang, drinkcat, Rex-BC.Chen, anson.chuang

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 b9fecc25d213..7ce72a82bec5 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4047,6 +4047,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


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

* [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
  2021-02-24  6:11 [v8,0/7] PCI: mediatek: Add new generation controller support Jianjun Wang
  2021-02-24  6:11 ` [v8,1/7] dt-bindings: PCI: mediatek-gen3: Add YAML schema Jianjun Wang
  2021-02-24  6:11 ` [v8,2/7] PCI: Export pci_pio_to_address() for module use Jianjun Wang
@ 2021-02-24  6:11 ` Jianjun Wang
  2021-02-24 13:36   ` Krzysztof Wilczyński
  2021-03-11 12:38   ` Pali Rohár
  2021-02-24  6:11 ` [v8,4/7] PCI: mediatek-gen3: Add INTx support Jianjun Wang
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 35+ messages in thread
From: Jianjun Wang @ 2021-02-24  6:11 UTC (permalink / raw)
  To: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee
  Cc: Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang,
	Jianjun Wang, youlin.pei, chuanjia.liu, qizhong.cheng,
	sin_jieyang, drinkcat, Rex-BC.Chen, anson.chuang

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 | 457 ++++++++++++++++++++
 3 files changed, 471 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..c602beb9afec
--- /dev/null
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -0,0 +1,457 @@
+// 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/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;
+	}
+
+	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


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

* [v8,4/7] PCI: mediatek-gen3: Add INTx support
  2021-02-24  6:11 [v8,0/7] PCI: mediatek: Add new generation controller support Jianjun Wang
                   ` (2 preceding siblings ...)
  2021-02-24  6:11 ` [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192 Jianjun Wang
@ 2021-02-24  6:11 ` Jianjun Wang
  2021-02-24 14:24   ` Krzysztof Wilczyński
  2021-03-09 11:10   ` Marc Zyngier
  2021-02-24  6:11 ` [v8,5/7] PCI: mediatek-gen3: Add MSI support Jianjun Wang
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 35+ messages in thread
From: Jianjun Wang @ 2021-02-24  6:11 UTC (permalink / raw)
  To: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee
  Cc: Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang,
	Jianjun Wang, youlin.pei, chuanjia.liu, qizhong.cheng,
	sin_jieyang, drinkcat, Rex-BC.Chen, anson.chuang

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 | 176 ++++++++++++++++++++
 1 file changed, 176 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index c602beb9afec..8b3b5f838b69 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/pci.h>
@@ -45,6 +48,13 @@
 #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_ENABLE \
+	GENMASK(PCIE_INTX_SHIFT + PCI_NUM_INTX - 1, PCIE_INTX_SHIFT)
+
+#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
@@ -73,6 +83,9 @@
  * @phy: PHY controller block
  * @clks: PCIe clocks
  * @num_clks: PCIe clocks count for this port
+ * @irq: PCIe controller interrupt number
+ * @irq_lock: lock protecting IRQ register access
+ * @intx_domain: legacy INTx IRQ domain
  */
 struct mtk_pcie_port {
 	struct device *dev;
@@ -83,6 +96,10 @@ struct mtk_pcie_port {
 	struct phy *phy;
 	struct clk_bulk_data *clks;
 	int num_clks;
+
+	int irq;
+	raw_spinlock_t irq_lock;
+	struct irq_domain *intx_domain;
 };
 
 /**
@@ -199,6 +216,11 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
 	val |= PCI_CLASS(PCI_CLASS_BRIDGE_PCI << 8);
 	writel_relaxed(val, port->base + PCIE_PCI_IDS_1);
 
+	/* Mask all INTx interrupts */
+	val = readl_relaxed(port->base + PCIE_INT_ENABLE_REG);
+	val &= ~PCIE_INTX_ENABLE;
+	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
+
 	/* 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;
@@ -262,6 +284,154 @@ 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);
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	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);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static void mtk_intx_unmask(struct irq_data *data)
+{
+	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	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);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+/**
+ * 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_enable		= mtk_intx_unmask,
+	.irq_disable		= mtk_intx_mask,
+	.irq_mask		= mtk_intx_mask,
+	.irq_unmask		= mtk_intx_unmask,
+	.irq_eoi		= mtk_intx_eoi,
+	.irq_set_affinity	= mtk_pcie_set_affinity,
+	.name			= "INTx",
+};
+
+static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
+			     irq_hw_number_t hwirq)
+{
+	irq_set_chip_data(irq, domain->host_data);
+	irq_set_chip_and_handler_name(irq, &mtk_intx_irq_chip,
+				      handle_fasteoi_irq, "INTx");
+	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 *dev = port->dev;
+	struct device_node *intc_node, *node = dev->of_node;
+
+	raw_spin_lock_init(&port->irq_lock);
+
+	/* 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);
+	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 *dev = port->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	int err;
+
+	err = mtk_pcie_init_irq_domains(port);
+	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;
@@ -384,6 +554,10 @@ static int mtk_pcie_setup(struct mtk_pcie_port *port)
 		goto err_setup;
 	}
 
+	err = mtk_pcie_setup_irq(port);
+	if (err)
+		goto err_setup;
+
 	return 0;
 
 err_setup:
@@ -417,6 +591,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;
 	}
@@ -434,6 +609,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


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

* [v8,5/7] PCI: mediatek-gen3: Add MSI support
  2021-02-24  6:11 [v8,0/7] PCI: mediatek: Add new generation controller support Jianjun Wang
                   ` (3 preceding siblings ...)
  2021-02-24  6:11 ` [v8,4/7] PCI: mediatek-gen3: Add INTx support Jianjun Wang
@ 2021-02-24  6:11 ` Jianjun Wang
  2021-02-24 14:31   ` Krzysztof Wilczyński
                     ` (2 more replies)
  2021-02-24  6:11 ` [v8,6/7] PCI: mediatek-gen3: Add system PM support Jianjun Wang
  2021-02-24  6:11 ` [v8,7/7] MAINTAINERS: Add Jianjun Wang as MediaTek PCI co-maintainer Jianjun Wang
  6 siblings, 3 replies; 35+ messages in thread
From: Jianjun Wang @ 2021-02-24  6:11 UTC (permalink / raw)
  To: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee
  Cc: Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang,
	Jianjun Wang, youlin.pei, chuanjia.liu, qizhong.cheng,
	sin_jieyang, drinkcat, Rex-BC.Chen, anson.chuang

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 | 277 ++++++++++++++++++++
 1 file changed, 277 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 8b3b5f838b69..3cbec22ece0c 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/pci.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
@@ -48,12 +49,29 @@
 #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_ENABLE \
 	GENMASK(PCIE_INTX_SHIFT + PCI_NUM_INTX - 1, PCIE_INTX_SHIFT)
 
 #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
@@ -73,6 +91,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_msi - MSI information for each set
+ * @base: IO mapped register base
+ * @msg_addr: MSI message address
+ */
+struct mtk_msi_set {
+	void __iomem *base;
+	phys_addr_t msg_addr;
+};
+
 /**
  * struct mtk_pcie_port - PCIe port information
  * @dev: pointer to PCIe device
@@ -86,6 +114,11 @@
  * @irq: PCIe controller interrupt number
  * @irq_lock: lock protecting IRQ register access
  * @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;
@@ -100,6 +133,11 @@ struct mtk_pcie_port {
 	int irq;
 	raw_spinlock_t irq_lock;
 	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);
 };
 
 /**
@@ -197,6 +235,35 @@ static int mtk_pcie_set_trans_table(struct mtk_pcie_port *port,
 	return 0;
 }
 
+static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
+{
+	int i;
+	u32 val;
+
+	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);
+
+	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
+		struct mtk_msi_set *msi_set = &port->msi_sets[i];
+
+		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;
+
+		/* Configure the MSI capture address */
+		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);
+	}
+}
+
 static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
 {
 	struct resource_entry *entry;
@@ -247,6 +314,8 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
 		return err;
 	}
 
+	mtk_pcie_enable_msi(port);
+
 	/* Set PCIe translation windows */
 	resource_list_for_each_entry(entry, &host->windows) {
 		struct resource *res = entry->res;
@@ -290,6 +359,148 @@ static int mtk_pcie_set_affinity(struct irq_data *data,
 	return -EINVAL;
 }
 
+static void mtk_pcie_irq_mask(struct irq_data *data)
+{
+	pci_msi_mask_irq(data);
+	irq_chip_mask_parent(data);
+}
+
+static void mtk_pcie_irq_unmask(struct irq_data *data)
+{
+	pci_msi_unmask_irq(data);
+	irq_chip_unmask_parent(data);
+}
+
+static struct irq_chip mtk_msi_irq_chip = {
+	.name = "MSI",
+	.irq_enable = mtk_pcie_irq_unmask,
+	.irq_disable = mtk_pcie_irq_mask,
+	.irq_ack = irq_chip_ack_parent,
+	.irq_mask = mtk_pcie_irq_mask,
+	.irq_unmask = mtk_pcie_irq_unmask,
+};
+
+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);
+	struct mtk_pcie_port *port = data->domain->host_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(port->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 void mtk_msi_bottom_irq_mask(struct irq_data *data)
+{
+	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
+	struct mtk_pcie_port *port = data->domain->host_data;
+	unsigned long hwirq, flags;
+	u32 val;
+
+	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	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);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static void mtk_msi_bottom_irq_unmask(struct irq_data *data)
+{
+	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
+	struct mtk_pcie_port *port = data->domain->host_data;
+	unsigned long hwirq, flags;
+	u32 val;
+
+	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	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);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static struct irq_chip mtk_msi_bottom_irq_chip = {
+	.irq_ack		= mtk_msi_bottom_irq_ack,
+	.irq_mask		= mtk_msi_bottom_irq_mask,
+	.irq_unmask		= mtk_msi_bottom_irq_unmask,
+	.irq_compose_msi_msg	= mtk_compose_msi_msg,
+	.irq_set_affinity	= mtk_pcie_set_affinity,
+	.name			= "MSI",
+};
+
+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 const struct irq_domain_ops mtk_msi_bottom_domain_ops = {
+	.alloc = mtk_msi_bottom_domain_alloc,
+	.free = mtk_msi_bottom_domain_free,
+};
+
 static void mtk_intx_mask(struct irq_data *data)
 {
 	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
@@ -360,6 +571,7 @@ static int mtk_pcie_init_irq_domains(struct mtk_pcie_port *port)
 {
 	struct device *dev = port->dev;
 	struct device_node *intc_node, *node = dev->of_node;
+	int ret;
 
 	raw_spin_lock_init(&port->irq_lock);
 
@@ -377,7 +589,34 @@ 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;
+	}
+
+	port->msi_domain = pci_msi_create_irq_domain(dev->fwnode,
+						     &mtk_msi_domain_info,
+						     port->msi_bottom_domain);
+	if (!port->msi_domain) {
+		dev_info(dev, "failed to create MSI domain\n");
+		ret = -ENODEV;
+		goto err_msi_domain;
+	}
+
 	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)
@@ -387,9 +626,39 @@ 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);
+
+	do {
+		msi_status = readl_relaxed(msi_set->base +
+					   PCIE_MSI_SET_STATUS_OFFSET);
+		msi_status &= msi_enable;
+		if (!msi_status)
+			break;
+
+		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);
+		}
+	} while (true);
+}
+
 static void mtk_pcie_irq_handler(struct irq_desc *desc)
 {
 	struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);
@@ -408,6 +677,14 @@ static void mtk_pcie_irq_handler(struct irq_desc *desc)
 		generic_handle_irq(virq);
 	}
 
+	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


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

* [v8,6/7] PCI: mediatek-gen3: Add system PM support
  2021-02-24  6:11 [v8,0/7] PCI: mediatek: Add new generation controller support Jianjun Wang
                   ` (4 preceding siblings ...)
  2021-02-24  6:11 ` [v8,5/7] PCI: mediatek-gen3: Add MSI support Jianjun Wang
@ 2021-02-24  6:11 ` Jianjun Wang
  2021-02-24 14:10   ` Krzysztof Wilczyński
  2021-02-24  6:11 ` [v8,7/7] MAINTAINERS: Add Jianjun Wang as MediaTek PCI co-maintainer Jianjun Wang
  6 siblings, 1 reply; 35+ messages in thread
From: Jianjun Wang @ 2021-02-24  6:11 UTC (permalink / raw)
  To: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee
  Cc: Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang,
	Jianjun Wang, youlin.pei, chuanjia.liu, qizhong.cheng,
	sin_jieyang, drinkcat, Rex-BC.Chen, anson.chuang

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 | 84 +++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index fde9de591888..fd13540d37fe 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -45,6 +45,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)
@@ -73,6 +76,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
@@ -892,6 +898,83 @@ 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);
+
+	reset_control_assert(port->mac_reset);
+
+	phy_power_off(port->phy);
+	reset_control_assert(port->phy_reset);
+
+	return 0;
+}
+
+static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
+{
+	struct mtk_pcie_port *port = dev_get_drvdata(dev);
+	int err;
+
+	reset_control_deassert(port->phy_reset);
+	phy_power_on(port->phy);
+
+	reset_control_deassert(port->mac_reset);
+
+	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" },
 	{},
@@ -903,6 +986,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


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

* [v8,7/7] MAINTAINERS: Add Jianjun Wang as MediaTek PCI co-maintainer
  2021-02-24  6:11 [v8,0/7] PCI: mediatek: Add new generation controller support Jianjun Wang
                   ` (5 preceding siblings ...)
  2021-02-24  6:11 ` [v8,6/7] PCI: mediatek-gen3: Add system PM support Jianjun Wang
@ 2021-02-24  6:11 ` Jianjun Wang
  6 siblings, 0 replies; 35+ messages in thread
From: Jianjun Wang @ 2021-02-24  6:11 UTC (permalink / raw)
  To: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee
  Cc: Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang,
	Jianjun Wang, youlin.pei, chuanjia.liu, qizhong.cheng,
	sin_jieyang, drinkcat, Rex-BC.Chen, anson.chuang

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 546aa66428c9..bef7f4017473 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13826,6 +13826,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


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

* Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
  2021-02-24  6:11 ` [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192 Jianjun Wang
@ 2021-02-24 13:36   ` Krzysztof Wilczyński
  2021-02-25  3:07     ` Jianjun Wang
  2021-03-11 12:38   ` Pali Rohár
  1 sibling, 1 reply; 35+ messages in thread
From: Krzysztof Wilczyński @ 2021-02-24 13:36 UTC (permalink / raw)
  To: Jianjun Wang
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

Hi Jianjun,

Thank you for all the work here!

[...]
> + * 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

It would be "MAC" and "PHY" in the above.

[...]
> + * 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.

The kernel-doc above might be missing brief function description.  See
the following for more concrete example:

  https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

[...]
> +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",

The wording of this error message is a little confusing.

> +			num, (unsigned long long) cpu_addr,

No space between the bracket and the variable name.

[...]
> +	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;
> +	}
[...]

It would be "PHY" in the error messages above.

[...]
> +	if (err) {
> +		dev_err(dev, "clock init failed\n");
> +		goto err_clk_init;
> +	}
[...]

A nitpick, so feel free to ignore it, of course.  What about "failed to
initialize clock" to keep the style consistent.

[...]
> +	err = mtk_pcie_startup_port(port);
> +	if (err) {
> +		dev_err(dev, "PCIe startup failed\n");
[...]

Also a nitpick.  What about "failed to bring PCIe link up"?

Krzysztof

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

* Re: [v8,6/7] PCI: mediatek-gen3: Add system PM support
  2021-02-24  6:11 ` [v8,6/7] PCI: mediatek-gen3: Add system PM support Jianjun Wang
@ 2021-02-24 14:10   ` Krzysztof Wilczyński
  2021-02-25  3:34     ` Jianjun Wang
  0 siblings, 1 reply; 35+ messages in thread
From: Krzysztof Wilczyński @ 2021-02-24 14:10 UTC (permalink / raw)
  To: Jianjun Wang
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

Hi Jianjun,

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

So, "systems suspend" and "resume" hooks, correct?

> When system suspend, trigger the PCIe link to L2 state and pull down

It probably would be "the system suspends".

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

Similarly to the above: "the system resumes".

[...]
> +	if (err) {
> +		dev_err(port->dev, "can not enter L2 state\n");
> +		return err;
> +	}

Most likely you want "cannot" or "can't" in the above error message.

> +	/* 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");

Just a nitpick.  What about "entered L2 states successfully"?

[...]
> +	if (err) {
> +		dev_err(port->dev, "resume failed\n");
> +		return err;
> +	}

This error message does not quite convey that the mtk_pcie_startup_port()
was the function that failed, which is only a part of what you have to do
to successfully resume.

> +	dev_dbg(port->dev, "resume done\n");

A nitpick.  Probably not needed, as lack of error message would mean
that the device resumed successfully after being suspended.

Krzysztof

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

* Re: [v8,4/7] PCI: mediatek-gen3: Add INTx support
  2021-02-24  6:11 ` [v8,4/7] PCI: mediatek-gen3: Add INTx support Jianjun Wang
@ 2021-02-24 14:24   ` Krzysztof Wilczyński
  2021-02-25  3:10     ` Jianjun Wang
  2021-03-09 11:10   ` Marc Zyngier
  1 sibling, 1 reply; 35+ messages in thread
From: Krzysztof Wilczyński @ 2021-02-24 14:24 UTC (permalink / raw)
  To: Jianjun Wang
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

Hi Jianjun,

[...]
> +/**
> + * 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.
> + */
[...]

See my comment about the kernel-doc from the following:

  https://lore.kernel.org/linux-pci/YDZWUGcKet%2FlNWlF@rocinante/

[...]
> +	if (err) {
> +		dev_err(dev, "failed to init PCIe IRQ domain\n");
> +		return err;
> +	}
[...]

Just a nitpick.  What about using "initialize" in the above?

Krzysztof

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

* Re: [v8,5/7] PCI: mediatek-gen3: Add MSI support
  2021-02-24  6:11 ` [v8,5/7] PCI: mediatek-gen3: Add MSI support Jianjun Wang
@ 2021-02-24 14:31   ` Krzysztof Wilczyński
  2021-02-25  3:09     ` Jianjun Wang
  2021-03-09 11:23   ` Marc Zyngier
  2021-03-11  0:05   ` Pali Rohár
  2 siblings, 1 reply; 35+ messages in thread
From: Krzysztof Wilczyński @ 2021-02-24 14:31 UTC (permalink / raw)
  To: Jianjun Wang
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

Hi Jianjun,

[...]
> +static struct irq_chip mtk_msi_irq_chip = {
> +	.name = "MSI",
> +	.irq_enable = mtk_pcie_irq_unmask,
> +	.irq_disable = mtk_pcie_irq_mask,
> +	.irq_ack = irq_chip_ack_parent,
> +	.irq_mask = mtk_pcie_irq_mask,
> +	.irq_unmask = mtk_pcie_irq_unmask,
> +};

For consistency sake, what about aligning this like the
struct mtk_msi_bottom_irq_chip has been?  See immediately below.

[...]
> +static struct irq_chip mtk_msi_bottom_irq_chip = {
> +	.irq_ack		= mtk_msi_bottom_irq_ack,
> +	.irq_mask		= mtk_msi_bottom_irq_mask,
> +	.irq_unmask		= mtk_msi_bottom_irq_unmask,
> +	.irq_compose_msi_msg	= mtk_compose_msi_msg,
> +	.irq_set_affinity	= mtk_pcie_set_affinity,
> +	.name			= "MSI",
> +};

Krzysztof

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

* Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
  2021-02-24 13:36   ` Krzysztof Wilczyński
@ 2021-02-25  3:07     ` Jianjun Wang
  0 siblings, 0 replies; 35+ messages in thread
From: Jianjun Wang @ 2021-02-25  3:07 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

Hi Krzysztof,

Thanks for your review, I will fix these at next version.

Thanks.

On Wed, 2021-02-24 at 14:36 +0100, Krzysztof Wilczyński wrote:
> Hi Jianjun,
> 
> Thank you for all the work here!
> 
> [...]
> > + * 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
> 
> It would be "MAC" and "PHY" in the above.
> 
> [...]
> > + * 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.
> 
> The kernel-doc above might be missing brief function description.  See
> the following for more concrete example:
> 
>   https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
> 
> [...]
> > +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",
> 
> The wording of this error message is a little confusing.
> 
> > +			num, (unsigned long long) cpu_addr,
> 
> No space between the bracket and the variable name.
> 
> [...]
> > +	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;
> > +	}
> [...]
> 
> It would be "PHY" in the error messages above.
> 
> [...]
> > +	if (err) {
> > +		dev_err(dev, "clock init failed\n");
> > +		goto err_clk_init;
> > +	}
> [...]
> 
> A nitpick, so feel free to ignore it, of course.  What about "failed to
> initialize clock" to keep the style consistent.
> 
> [...]
> > +	err = mtk_pcie_startup_port(port);
> > +	if (err) {
> > +		dev_err(dev, "PCIe startup failed\n");
> [...]
> 
> Also a nitpick.  What about "failed to bring PCIe link up"?
> 
> Krzysztof


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

* Re: [v8,5/7] PCI: mediatek-gen3: Add MSI support
  2021-02-24 14:31   ` Krzysztof Wilczyński
@ 2021-02-25  3:09     ` Jianjun Wang
  0 siblings, 0 replies; 35+ messages in thread
From: Jianjun Wang @ 2021-02-25  3:09 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

Hi Krzysztof,

Thanks for your review, I will fix it at next version.

On Wed, 2021-02-24 at 15:31 +0100, Krzysztof Wilczyński wrote:
> Hi Jianjun,
> 
> [...]
> > +static struct irq_chip mtk_msi_irq_chip = {
> > +	.name = "MSI",
> > +	.irq_enable = mtk_pcie_irq_unmask,
> > +	.irq_disable = mtk_pcie_irq_mask,
> > +	.irq_ack = irq_chip_ack_parent,
> > +	.irq_mask = mtk_pcie_irq_mask,
> > +	.irq_unmask = mtk_pcie_irq_unmask,
> > +};
> 
> For consistency sake, what about aligning this like the
> struct mtk_msi_bottom_irq_chip has been?  See immediately below.
> 
> [...]
> > +static struct irq_chip mtk_msi_bottom_irq_chip = {
> > +	.irq_ack		= mtk_msi_bottom_irq_ack,
> > +	.irq_mask		= mtk_msi_bottom_irq_mask,
> > +	.irq_unmask		= mtk_msi_bottom_irq_unmask,
> > +	.irq_compose_msi_msg	= mtk_compose_msi_msg,
> > +	.irq_set_affinity	= mtk_pcie_set_affinity,
> > +	.name			= "MSI",
> > +};
> 
> Krzysztof

Thanks.

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

* Re: [v8,4/7] PCI: mediatek-gen3: Add INTx support
  2021-02-24 14:24   ` Krzysztof Wilczyński
@ 2021-02-25  3:10     ` Jianjun Wang
  0 siblings, 0 replies; 35+ messages in thread
From: Jianjun Wang @ 2021-02-25  3:10 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

Hi Krzysztof,

Thanks for your review, I will fix it at next version.

On Wed, 2021-02-24 at 15:24 +0100, Krzysztof Wilczyński wrote:
> Hi Jianjun,
> 
> [...]
> > +/**
> > + * 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.
> > + */
> [...]
> 
> See my comment about the kernel-doc from the following:
> 
>   https://lore.kernel.org/linux-pci/YDZWUGcKet%2FlNWlF@rocinante/
> 
> [...]
> > +	if (err) {
> > +		dev_err(dev, "failed to init PCIe IRQ domain\n");
> > +		return err;
> > +	}
> [...]
> 
> Just a nitpick.  What about using "initialize" in the above?
> 
> Krzysztof

Thanks.


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

* Re: [v8,6/7] PCI: mediatek-gen3: Add system PM support
  2021-02-24 14:10   ` Krzysztof Wilczyński
@ 2021-02-25  3:34     ` Jianjun Wang
  2021-02-25 22:00       ` Krzysztof Wilczyński
  0 siblings, 1 reply; 35+ messages in thread
From: Jianjun Wang @ 2021-02-25  3:34 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

Hi Krzysztof,

Thanks for your review,

On Wed, 2021-02-24 at 15:10 +0100, Krzysztof Wilczyński wrote:
> Hi Jianjun,
> 
> > Add suspend_noirq and resume_noirq callback functions to implement
> > PM system suspend hooks for MediaTek Gen3 PCIe controller.
> 
> So, "systems suspend" and "resume" hooks, correct?

The callback functions is suspend_noirq and resume_noirq, should I use
"systems suspend" and "resume" in the commit message?

> 
> > When system suspend, trigger the PCIe link to L2 state and pull down
> 
> It probably would be "the system suspends".
> 
> [...]
> > When system resum, the PCIe link should be re-established and the
> > related control register values should be restored.
> 
> Similarly to the above: "the system resumes".
> 
> [...]
> > +	if (err) {
> > +		dev_err(port->dev, "can not enter L2 state\n");
> > +		return err;
> > +	}
> 
> Most likely you want "cannot" or "can't" in the above error message.
> 
> > +	/* 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");
> 
> Just a nitpick.  What about "entered L2 states successfully"?
> 
> [...]
> > +	if (err) {
> > +		dev_err(port->dev, "resume failed\n");
> > +		return err;
> > +	}
> 
> This error message does not quite convey that the mtk_pcie_startup_port()
> was the function that failed, which is only a part of what you have to do
> to successfully resume.
> 
> > +	dev_dbg(port->dev, "resume done\n");
> 
> A nitpick.  Probably not needed, as lack of error message would mean
> that the device resumed successfully after being suspended.
> 
> Krzysztof

Thanks.


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

* Re: [v8,6/7] PCI: mediatek-gen3: Add system PM support
  2021-02-25  3:34     ` Jianjun Wang
@ 2021-02-25 22:00       ` Krzysztof Wilczyński
  2021-02-26 10:06         ` Jianjun Wang
  0 siblings, 1 reply; 35+ messages in thread
From: Krzysztof Wilczyński @ 2021-02-25 22:00 UTC (permalink / raw)
  To: Jianjun Wang
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

Hi Jianjun,

[...]
> Thanks for your review,

Thank YOU for all the work here!
 
[...]
> > > Add suspend_noirq and resume_noirq callback functions to implement
> > > PM system suspend hooks for MediaTek Gen3 PCIe controller.
> > 
> > So, "systems suspend" and "resume" hooks, correct?
> 
> The callback functions is suspend_noirq and resume_noirq, should I use
> "systems suspend" and "resume" in the commit message?
[...]


What I meant was something along these lines:

  Add suspend_noirq and resume_noirq callback functions to implement PM
  system suspend and resume hooks for the MediaTek Gen3 PCIe controller.
  
  When the system suspends, trigger the PCIe link to enter the L2 state
  and pull down the PERST# pin, gating the clocks of the MAC layer, and
  then power-off the physical layer to provide power-saving.
  
  When the system resumes, the PCIe link should be re-established and the
  related control register values should be restored.

The above is just a suggestion, thus feel tree to ignore it completely,
and it's heavily based on your original commit message.

Krzysztof

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

* Re: [v8,6/7] PCI: mediatek-gen3: Add system PM support
  2021-02-25 22:00       ` Krzysztof Wilczyński
@ 2021-02-26 10:06         ` Jianjun Wang
  0 siblings, 0 replies; 35+ messages in thread
From: Jianjun Wang @ 2021-02-26 10:06 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

Hi Krzysztof,

Thanks for your suggestion, I will fix it in the next version.

On Thu, 2021-02-25 at 23:00 +0100, Krzysztof Wilczyński wrote:
> Hi Jianjun,
> 
> [...]
> > Thanks for your review,
> 
> Thank YOU for all the work here!
>  
> [...]
> > > > Add suspend_noirq and resume_noirq callback functions to implement
> > > > PM system suspend hooks for MediaTek Gen3 PCIe controller.
> > > 
> > > So, "systems suspend" and "resume" hooks, correct?
> > 
> > The callback functions is suspend_noirq and resume_noirq, should I use
> > "systems suspend" and "resume" in the commit message?
> [...]
> 
> 
> What I meant was something along these lines:
> 
>   Add suspend_noirq and resume_noirq callback functions to implement PM
>   system suspend and resume hooks for the MediaTek Gen3 PCIe controller.
>   
>   When the system suspends, trigger the PCIe link to enter the L2 state
>   and pull down the PERST# pin, gating the clocks of the MAC layer, and
>   then power-off the physical layer to provide power-saving.
>   
>   When the system resumes, the PCIe link should be re-established and the
>   related control register values should be restored.
> 
> The above is just a suggestion, thus feel tree to ignore it completely,
> and it's heavily based on your original commit message.
> 
> Krzysztof

Thanks.


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

* Re: [v8,1/7] dt-bindings: PCI: mediatek-gen3: Add YAML schema
  2021-02-24  6:11 ` [v8,1/7] dt-bindings: PCI: mediatek-gen3: Add YAML schema Jianjun Wang
@ 2021-03-06 20:09   ` Rob Herring
  0 siblings, 0 replies; 35+ messages in thread
From: Rob Herring @ 2021-03-06 20:09 UTC (permalink / raw)
  To: Jianjun Wang
  Cc: linux-mediatek, Ryder Lee, linux-pci, youlin.pei, anson.chuang,
	Philipp Zabel, maz, Bjorn Helgaas, chuanjia.liu, devicetree,
	Rob Herring, Rex-BC.Chen, Matthias Brugger, Lorenzo Pieralisi,
	Sj Huang, sin_jieyang, drinkcat, linux-kernel, linux-arm-kernel,
	qizhong.cheng

On Wed, 24 Feb 2021 14:11:26 +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      | 181 ++++++++++++++++++
>  1 file changed, 181 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> 

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

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

* Re: [v8,4/7] PCI: mediatek-gen3: Add INTx support
  2021-02-24  6:11 ` [v8,4/7] PCI: mediatek-gen3: Add INTx support Jianjun Wang
  2021-02-24 14:24   ` Krzysztof Wilczyński
@ 2021-03-09 11:10   ` Marc Zyngier
  2021-03-10  3:05     ` Jianjun Wang
  1 sibling, 1 reply; 35+ messages in thread
From: Marc Zyngier @ 2021-03-09 11:10 UTC (permalink / raw)
  To: Jianjun Wang
  Cc: Bjorn Helgaas, Rob Herring, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

On Wed, 24 Feb 2021 06:11:29 +0000,
Jianjun Wang <jianjun.wang@mediatek.com> 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 | 176 ++++++++++++++++++++
>  1 file changed, 176 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index c602beb9afec..8b3b5f838b69 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/pci.h>
> @@ -45,6 +48,13 @@
>  #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_ENABLE \
> +	GENMASK(PCIE_INTX_SHIFT + PCI_NUM_INTX - 1, PCIE_INTX_SHIFT)
> +
> +#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
> @@ -73,6 +83,9 @@
>   * @phy: PHY controller block
>   * @clks: PCIe clocks
>   * @num_clks: PCIe clocks count for this port
> + * @irq: PCIe controller interrupt number
> + * @irq_lock: lock protecting IRQ register access
> + * @intx_domain: legacy INTx IRQ domain
>   */
>  struct mtk_pcie_port {
>  	struct device *dev;
> @@ -83,6 +96,10 @@ struct mtk_pcie_port {
>  	struct phy *phy;
>  	struct clk_bulk_data *clks;
>  	int num_clks;
> +
> +	int irq;
> +	raw_spinlock_t irq_lock;
> +	struct irq_domain *intx_domain;
>  };
>  
>  /**
> @@ -199,6 +216,11 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
>  	val |= PCI_CLASS(PCI_CLASS_BRIDGE_PCI << 8);
>  	writel_relaxed(val, port->base + PCIE_PCI_IDS_1);
>  
> +	/* Mask all INTx interrupts */
> +	val = readl_relaxed(port->base + PCIE_INT_ENABLE_REG);
> +	val &= ~PCIE_INTX_ENABLE;
> +	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
> +
>  	/* 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;
> @@ -262,6 +284,154 @@ 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);
> +	unsigned long flags;
> +	u32 val;
> +
> +	raw_spin_lock_irqsave(&port->irq_lock, flags);
> +	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);
> +	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
> +}
> +
> +static void mtk_intx_unmask(struct irq_data *data)
> +{
> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> +	unsigned long flags;
> +	u32 val;
> +
> +	raw_spin_lock_irqsave(&port->irq_lock, flags);
> +	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);
> +	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
> +}
> +
> +/**
> + * 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_enable		= mtk_intx_unmask,
> +	.irq_disable		= mtk_intx_mask,

Please get rid of enable/disable. Given that you already have
mask/unmask with the *same* implementation, this offers zero benefit.

> +	.irq_mask		= mtk_intx_mask,
> +	.irq_unmask		= mtk_intx_unmask,
> +	.irq_eoi		= mtk_intx_eoi,
> +	.irq_set_affinity	= mtk_pcie_set_affinity,
> +	.name			= "INTx",
> +};

[...]

Other that that, this look good to me.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

* Re: [v8,5/7] PCI: mediatek-gen3: Add MSI support
  2021-02-24  6:11 ` [v8,5/7] PCI: mediatek-gen3: Add MSI support Jianjun Wang
  2021-02-24 14:31   ` Krzysztof Wilczyński
@ 2021-03-09 11:23   ` Marc Zyngier
  2021-03-10  6:48     ` Jianjun Wang
  2021-03-11  0:05   ` Pali Rohár
  2 siblings, 1 reply; 35+ messages in thread
From: Marc Zyngier @ 2021-03-09 11:23 UTC (permalink / raw)
  To: Jianjun Wang
  Cc: Bjorn Helgaas, Rob Herring, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

On Wed, 24 Feb 2021 06:11:30 +0000,
Jianjun Wang <jianjun.wang@mediatek.com> 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 | 277 ++++++++++++++++++++
>  1 file changed, 277 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 8b3b5f838b69..3cbec22ece0c 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/pci.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> @@ -48,12 +49,29 @@
>  #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_ENABLE \
>  	GENMASK(PCIE_INTX_SHIFT + PCI_NUM_INTX - 1, PCIE_INTX_SHIFT)
>  
>  #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
> @@ -73,6 +91,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_msi - MSI information for each set
> + * @base: IO mapped register base
> + * @msg_addr: MSI message address
> + */
> +struct mtk_msi_set {
> +	void __iomem *base;
> +	phys_addr_t msg_addr;
> +};
> +
>  /**
>   * struct mtk_pcie_port - PCIe port information
>   * @dev: pointer to PCIe device
> @@ -86,6 +114,11 @@
>   * @irq: PCIe controller interrupt number
>   * @irq_lock: lock protecting IRQ register access
>   * @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;
> @@ -100,6 +133,11 @@ struct mtk_pcie_port {
>  	int irq;
>  	raw_spinlock_t irq_lock;
>  	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);
>  };
>  
>  /**
> @@ -197,6 +235,35 @@ static int mtk_pcie_set_trans_table(struct mtk_pcie_port *port,
>  	return 0;
>  }
>  
> +static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
> +{
> +	int i;
> +	u32 val;
> +
> +	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);

Shouldn't you configure the capture addresses *before* enabling
things? Is there any need for locking here, given that you are
modifying global registers?

> +
> +	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
> +		struct mtk_msi_set *msi_set = &port->msi_sets[i];
> +
> +		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;
> +
> +		/* Configure the MSI capture address */
> +		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);
> +	}
> +}
> +
>  static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
>  {
>  	struct resource_entry *entry;
> @@ -247,6 +314,8 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
>  		return err;
>  	}
>  
> +	mtk_pcie_enable_msi(port);
> +
>  	/* Set PCIe translation windows */
>  	resource_list_for_each_entry(entry, &host->windows) {
>  		struct resource *res = entry->res;
> @@ -290,6 +359,148 @@ static int mtk_pcie_set_affinity(struct irq_data *data,
>  	return -EINVAL;
>  }
>  
> +static void mtk_pcie_irq_mask(struct irq_data *data)

It'd be good if you used _msi_ in function names that deal with MSIs.

> +{
> +	pci_msi_mask_irq(data);
> +	irq_chip_mask_parent(data);
> +}
> +
> +static void mtk_pcie_irq_unmask(struct irq_data *data)
> +{
> +	pci_msi_unmask_irq(data);
> +	irq_chip_unmask_parent(data);
> +}
> +
> +static struct irq_chip mtk_msi_irq_chip = {
> +	.name = "MSI",
> +	.irq_enable = mtk_pcie_irq_unmask,
> +	.irq_disable = mtk_pcie_irq_mask,

Same comment as for the previous patch: enable/disable serve no
purpose here.

> +	.irq_ack = irq_chip_ack_parent,
> +	.irq_mask = mtk_pcie_irq_mask,
> +	.irq_unmask = mtk_pcie_irq_unmask,
> +};
> +
> +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),

minor nit: keep the *_OPS flag on one line, and the *_PCI_* flags on
another.

> +	.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);
> +	struct mtk_pcie_port *port = data->domain->host_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(port->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 void mtk_msi_bottom_irq_mask(struct irq_data *data)
> +{
> +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> +	struct mtk_pcie_port *port = data->domain->host_data;
> +	unsigned long hwirq, flags;
> +	u32 val;
> +
> +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> +
> +	raw_spin_lock_irqsave(&port->irq_lock, flags);
> +	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);
> +	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
> +}
> +
> +static void mtk_msi_bottom_irq_unmask(struct irq_data *data)
> +{
> +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> +	struct mtk_pcie_port *port = data->domain->host_data;
> +	unsigned long hwirq, flags;
> +	u32 val;
> +
> +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> +
> +	raw_spin_lock_irqsave(&port->irq_lock, flags);
> +	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);
> +	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
> +}
> +
> +static struct irq_chip mtk_msi_bottom_irq_chip = {
> +	.irq_ack		= mtk_msi_bottom_irq_ack,
> +	.irq_mask		= mtk_msi_bottom_irq_mask,
> +	.irq_unmask		= mtk_msi_bottom_irq_unmask,
> +	.irq_compose_msi_msg	= mtk_compose_msi_msg,
> +	.irq_set_affinity	= mtk_pcie_set_affinity,
> +	.name			= "MSI",
> +};
> +
> +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 const struct irq_domain_ops mtk_msi_bottom_domain_ops = {
> +	.alloc = mtk_msi_bottom_domain_alloc,
> +	.free = mtk_msi_bottom_domain_free,
> +};
> +
>  static void mtk_intx_mask(struct irq_data *data)
>  {
>  	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> @@ -360,6 +571,7 @@ static int mtk_pcie_init_irq_domains(struct mtk_pcie_port *port)
>  {
>  	struct device *dev = port->dev;
>  	struct device_node *intc_node, *node = dev->of_node;
> +	int ret;
>  
>  	raw_spin_lock_init(&port->irq_lock);
>  
> @@ -377,7 +589,34 @@ 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;
> +	}
> +
> +	port->msi_domain = pci_msi_create_irq_domain(dev->fwnode,
> +						     &mtk_msi_domain_info,
> +						     port->msi_bottom_domain);
> +	if (!port->msi_domain) {
> +		dev_info(dev, "failed to create MSI domain\n");
> +		ret = -ENODEV;
> +		goto err_msi_domain;
> +	}
> +
>  	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)
> @@ -387,9 +626,39 @@ 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);
> +
> +	do {
> +		msi_status = readl_relaxed(msi_set->base +
> +					   PCIE_MSI_SET_STATUS_OFFSET);
> +		msi_status &= msi_enable;
> +		if (!msi_status)
> +			break;
> +
> +		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);
> +		}
> +	} while (true);
> +}
> +
>  static void mtk_pcie_irq_handler(struct irq_desc *desc)
>  {
>  	struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);
> @@ -408,6 +677,14 @@ static void mtk_pcie_irq_handler(struct irq_desc *desc)
>  		generic_handle_irq(virq);
>  	}
>  
> +	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);

Isn't this write the same thing you have for EOI in the INTx case?
While I could understand your description in that case (this is a
resampling operation), I don't get what this does here. Either this is
also an EOI, but your initial description doesn't make sense, or it is
an Ack, and it should be moved to the right place.

Which one is it?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

* Re: [v8,4/7] PCI: mediatek-gen3: Add INTx support
  2021-03-09 11:10   ` Marc Zyngier
@ 2021-03-10  3:05     ` Jianjun Wang
  0 siblings, 0 replies; 35+ messages in thread
From: Jianjun Wang @ 2021-03-10  3:05 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Bjorn Helgaas, Rob Herring, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

On Tue, 2021-03-09 at 11:10 +0000, Marc Zyngier wrote:
> On Wed, 24 Feb 2021 06:11:29 +0000,
> Jianjun Wang <jianjun.wang@mediatek.com> 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 | 176 ++++++++++++++++++++
> >  1 file changed, 176 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index c602beb9afec..8b3b5f838b69 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/pci.h>
> > @@ -45,6 +48,13 @@
> >  #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_ENABLE \
> > +	GENMASK(PCIE_INTX_SHIFT + PCI_NUM_INTX - 1, PCIE_INTX_SHIFT)
> > +
> > +#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
> > @@ -73,6 +83,9 @@
> >   * @phy: PHY controller block
> >   * @clks: PCIe clocks
> >   * @num_clks: PCIe clocks count for this port
> > + * @irq: PCIe controller interrupt number
> > + * @irq_lock: lock protecting IRQ register access
> > + * @intx_domain: legacy INTx IRQ domain
> >   */
> >  struct mtk_pcie_port {
> >  	struct device *dev;
> > @@ -83,6 +96,10 @@ struct mtk_pcie_port {
> >  	struct phy *phy;
> >  	struct clk_bulk_data *clks;
> >  	int num_clks;
> > +
> > +	int irq;
> > +	raw_spinlock_t irq_lock;
> > +	struct irq_domain *intx_domain;
> >  };
> >  
> >  /**
> > @@ -199,6 +216,11 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
> >  	val |= PCI_CLASS(PCI_CLASS_BRIDGE_PCI << 8);
> >  	writel_relaxed(val, port->base + PCIE_PCI_IDS_1);
> >  
> > +	/* Mask all INTx interrupts */
> > +	val = readl_relaxed(port->base + PCIE_INT_ENABLE_REG);
> > +	val &= ~PCIE_INTX_ENABLE;
> > +	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
> > +
> >  	/* 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;
> > @@ -262,6 +284,154 @@ 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);
> > +	unsigned long flags;
> > +	u32 val;
> > +
> > +	raw_spin_lock_irqsave(&port->irq_lock, flags);
> > +	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);
> > +	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
> > +}
> > +
> > +static void mtk_intx_unmask(struct irq_data *data)
> > +{
> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> > +	unsigned long flags;
> > +	u32 val;
> > +
> > +	raw_spin_lock_irqsave(&port->irq_lock, flags);
> > +	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);
> > +	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
> > +}
> > +
> > +/**
> > + * 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_enable		= mtk_intx_unmask,
> > +	.irq_disable		= mtk_intx_mask,
> 
> Please get rid of enable/disable. Given that you already have
> mask/unmask with the *same* implementation, this offers zero benefit.

Hi Marc,

Thanks for your review.

We need to support suspend/resume feature, the HW will be powered off
when the system is suspended, and its register value will be cleared. If
the enable/disable callback is not implemented, the unmask function will
not be called when the system resume, so INTx will remain disabled.

Can I keep the enable/disable callback? Or do we have any solutions to
restore the register value when the system resume?

Thanks.
> 
> > +	.irq_mask		= mtk_intx_mask,
> > +	.irq_unmask		= mtk_intx_unmask,
> > +	.irq_eoi		= mtk_intx_eoi,
> > +	.irq_set_affinity	= mtk_pcie_set_affinity,
> > +	.name			= "INTx",
> > +};
> 
> [...]
> 
> Other that that, this look good to me.
> 
> Thanks,
> 
> 	M.
> 


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

* Re: [v8,5/7] PCI: mediatek-gen3: Add MSI support
  2021-03-09 11:23   ` Marc Zyngier
@ 2021-03-10  6:48     ` Jianjun Wang
       [not found]       ` <87a6rbxs4w.wl-maz@kernel.org>
  0 siblings, 1 reply; 35+ messages in thread
From: Jianjun Wang @ 2021-03-10  6:48 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Bjorn Helgaas, Rob Herring, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

Hi Marc,

Thanks for your review.

On Tue, 2021-03-09 at 11:23 +0000, Marc Zyngier wrote:
> On Wed, 24 Feb 2021 06:11:30 +0000,
> Jianjun Wang <jianjun.wang@mediatek.com> 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 | 277 ++++++++++++++++++++
> >  1 file changed, 277 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index 8b3b5f838b69..3cbec22ece0c 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/pci.h>
> >  #include <linux/phy/phy.h>
> >  #include <linux/platform_device.h>
> > @@ -48,12 +49,29 @@
> >  #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_ENABLE \
> >  	GENMASK(PCIE_INTX_SHIFT + PCI_NUM_INTX - 1, PCIE_INTX_SHIFT)
> >  
> >  #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
> > @@ -73,6 +91,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_msi - MSI information for each set
> > + * @base: IO mapped register base
> > + * @msg_addr: MSI message address
> > + */
> > +struct mtk_msi_set {
> > +	void __iomem *base;
> > +	phys_addr_t msg_addr;
> > +};
> > +
> >  /**
> >   * struct mtk_pcie_port - PCIe port information
> >   * @dev: pointer to PCIe device
> > @@ -86,6 +114,11 @@
> >   * @irq: PCIe controller interrupt number
> >   * @irq_lock: lock protecting IRQ register access
> >   * @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;
> > @@ -100,6 +133,11 @@ struct mtk_pcie_port {
> >  	int irq;
> >  	raw_spinlock_t irq_lock;
> >  	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);
> >  };
> >  
> >  /**
> > @@ -197,6 +235,35 @@ static int mtk_pcie_set_trans_table(struct mtk_pcie_port *port,
> >  	return 0;
> >  }
> >  
> > +static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
> > +{
> > +	int i;
> > +	u32 val;
> > +
> > +	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);
> 
> Shouldn't you configure the capture addresses *before* enabling
> things? Is there any need for locking here, given that you are
> modifying global registers?

Yes, I will move these codes to the back of the configure capture
address in the next version.

I think the lock may not be needed because this function is only
executed once when driver probe.

> 
> > +
> > +	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
> > +		struct mtk_msi_set *msi_set = &port->msi_sets[i];
> > +
> > +		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;
> > +
> > +		/* Configure the MSI capture address */
> > +		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);
> > +	}
> > +}
> > +
> >  static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
> >  {
> >  	struct resource_entry *entry;
> > @@ -247,6 +314,8 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
> >  		return err;
> >  	}
> >  
> > +	mtk_pcie_enable_msi(port);
> > +
> >  	/* Set PCIe translation windows */
> >  	resource_list_for_each_entry(entry, &host->windows) {
> >  		struct resource *res = entry->res;
> > @@ -290,6 +359,148 @@ static int mtk_pcie_set_affinity(struct irq_data *data,
> >  	return -EINVAL;
> >  }
> >  
> > +static void mtk_pcie_irq_mask(struct irq_data *data)
> 
> It'd be good if you used _msi_ in function names that deal with MSIs.

OK, I will fix it in the next version.

> 
> > +{
> > +	pci_msi_mask_irq(data);
> > +	irq_chip_mask_parent(data);
> > +}
> > +
> > +static void mtk_pcie_irq_unmask(struct irq_data *data)
> > +{
> > +	pci_msi_unmask_irq(data);
> > +	irq_chip_unmask_parent(data);
> > +}
> > +
> > +static struct irq_chip mtk_msi_irq_chip = {
> > +	.name = "MSI",
> > +	.irq_enable = mtk_pcie_irq_unmask,
> > +	.irq_disable = mtk_pcie_irq_mask,
> 
> Same comment as for the previous patch: enable/disable serve no
> purpose here.

Replied in the previous patch, the enable/disable callback is used when
the system suspend/resume.

> 
> > +	.irq_ack = irq_chip_ack_parent,
> > +	.irq_mask = mtk_pcie_irq_mask,
> > +	.irq_unmask = mtk_pcie_irq_unmask,
> > +};
> > +
> > +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),
> 
> minor nit: keep the *_OPS flag on one line, and the *_PCI_* flags on
> another.

Sure, I will fix it in the next version.

> 
> > +	.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);
> > +	struct mtk_pcie_port *port = data->domain->host_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(port->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 void mtk_msi_bottom_irq_mask(struct irq_data *data)
> > +{
> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> > +	struct mtk_pcie_port *port = data->domain->host_data;
> > +	unsigned long hwirq, flags;
> > +	u32 val;
> > +
> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> > +
> > +	raw_spin_lock_irqsave(&port->irq_lock, flags);
> > +	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);
> > +	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
> > +}
> > +
> > +static void mtk_msi_bottom_irq_unmask(struct irq_data *data)
> > +{
> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> > +	struct mtk_pcie_port *port = data->domain->host_data;
> > +	unsigned long hwirq, flags;
> > +	u32 val;
> > +
> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> > +
> > +	raw_spin_lock_irqsave(&port->irq_lock, flags);
> > +	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);
> > +	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
> > +}
> > +
> > +static struct irq_chip mtk_msi_bottom_irq_chip = {
> > +	.irq_ack		= mtk_msi_bottom_irq_ack,
> > +	.irq_mask		= mtk_msi_bottom_irq_mask,
> > +	.irq_unmask		= mtk_msi_bottom_irq_unmask,
> > +	.irq_compose_msi_msg	= mtk_compose_msi_msg,
> > +	.irq_set_affinity	= mtk_pcie_set_affinity,
> > +	.name			= "MSI",
> > +};
> > +
> > +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 const struct irq_domain_ops mtk_msi_bottom_domain_ops = {
> > +	.alloc = mtk_msi_bottom_domain_alloc,
> > +	.free = mtk_msi_bottom_domain_free,
> > +};
> > +
> >  static void mtk_intx_mask(struct irq_data *data)
> >  {
> >  	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> > @@ -360,6 +571,7 @@ static int mtk_pcie_init_irq_domains(struct mtk_pcie_port *port)
> >  {
> >  	struct device *dev = port->dev;
> >  	struct device_node *intc_node, *node = dev->of_node;
> > +	int ret;
> >  
> >  	raw_spin_lock_init(&port->irq_lock);
> >  
> > @@ -377,7 +589,34 @@ 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;
> > +	}
> > +
> > +	port->msi_domain = pci_msi_create_irq_domain(dev->fwnode,
> > +						     &mtk_msi_domain_info,
> > +						     port->msi_bottom_domain);
> > +	if (!port->msi_domain) {
> > +		dev_info(dev, "failed to create MSI domain\n");
> > +		ret = -ENODEV;
> > +		goto err_msi_domain;
> > +	}
> > +
> >  	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)
> > @@ -387,9 +626,39 @@ 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);
> > +
> > +	do {
> > +		msi_status = readl_relaxed(msi_set->base +
> > +					   PCIE_MSI_SET_STATUS_OFFSET);
> > +		msi_status &= msi_enable;
> > +		if (!msi_status)
> > +			break;
> > +
> > +		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);
> > +		}
> > +	} while (true);
> > +}
> > +
> >  static void mtk_pcie_irq_handler(struct irq_desc *desc)
> >  {
> >  	struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);
> > @@ -408,6 +677,14 @@ static void mtk_pcie_irq_handler(struct irq_desc *desc)
> >  		generic_handle_irq(virq);
> >  	}
> >  
> > +	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);
> 
> Isn't this write the same thing you have for EOI in the INTx case?
> While I could understand your description in that case (this is a
> resampling operation), I don't get what this does here. Either this is
> also an EOI, but your initial description doesn't make sense, or it is
> an Ack, and it should be moved to the right place.
> 
> Which one is it?

I think it should be an EOI which used to clear the interrupt status of
a single set in the PCIe intc field, maybe I should move it to the end
of the mtk_pcie_msi_handler() function.

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

I would like to ask another question. In this interrupt architecture, we
cannot implement an affinity for PCIe interrupts, so we return a
negative value in the mtk_pcie_set_affinity callback as follows: 

+static int mtk_pcie_set_affinity(struct irq_data *data,
+                                const struct cpumask *mask, bool force)
+{
+       return -EINVAL;
+}

But there will always be error logs when hotplug a CPU:

~ # echo 0 > /sys/devices/system/cpu/cpu1/online
[   93.633059] IRQ255: set affinity failed(-22).
[   93.633624] IRQ256: set affinity failed(-22).
[   93.634222] CPU1: shutdown
[   93.634586] psci: CPU1 killed (polled 0 ms)

Or when the system suspends:

~ # echo mem > /sys/power/state
[   93.635145] cpuhp: cpu_off cluster=0, cpu=1
[  169.835653] PM: suspend entry (deep)
[  169.836717] Filesystems sync: 0.000 seconds
[  169.837924] Freezing user space processes ... (elapsed 0.001 seconds)
done.
[  169.839922] OOM killer disabled.
[  169.840336] Freezing remaining freezable tasks ... (elapsed 0.001
seconds) done.
[  169.844715] Disabling non-boot CPUs ...
[  169.846443] IRQ255: set affinity failed(-22).
[  169.847002] IRQ256: set affinity failed(-22).
[  169.847586] CPU2: shutdown
[  169.847943] psci: CPU2 killed (polled 0 ms)
[  169.848489] cpuhp: cpu_off cluster=0, cpu=2
[  169.850285] IRQ255: set affinity failed(-22).
[  169.851369] IRQ256: set affinity failed(-22).
...

Sometimes this can cause misunderstandings to users, do we have a chance
to prevent this error log?

> 
> Thanks,
> 
> 	M.
> 

Thanks.

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

* Re: [v8,5/7] PCI: mediatek-gen3: Add MSI support
  2021-02-24  6:11 ` [v8,5/7] PCI: mediatek-gen3: Add MSI support Jianjun Wang
  2021-02-24 14:31   ` Krzysztof Wilczyński
  2021-03-09 11:23   ` Marc Zyngier
@ 2021-03-11  0:05   ` Pali Rohár
  2021-03-11  8:19     ` Marc Zyngier
  2 siblings, 1 reply; 35+ messages in thread
From: Pali Rohár @ 2021-03-11  0:05 UTC (permalink / raw)
  To: Marc Zyngier, Jianjun Wang
  Cc: Bjorn Helgaas, Rob Herring, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

On Wednesday 24 February 2021 14:11:30 Jianjun Wang wrote:
> +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);

Marc, should not be there bitmap_release_region() with order_base_2()?

bitmap_release_region(port->msi_irq_in_use, data->hwirq, order_base_2(nr_irqs));

Because mtk_msi_bottom_domain_alloc() is allocating
order_base_2(nr_irqs) interrupts, not only nr_irqs.

> +
> +	mutex_unlock(&port->lock);
> +
> +	irq_domain_free_irqs_common(domain, virq, nr_irqs);
> +}

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

* Re: [v8,5/7] PCI: mediatek-gen3: Add MSI support
  2021-03-11  0:05   ` Pali Rohár
@ 2021-03-11  8:19     ` Marc Zyngier
  2021-03-11  9:50       ` Jianjun Wang
  0 siblings, 1 reply; 35+ messages in thread
From: Marc Zyngier @ 2021-03-11  8:19 UTC (permalink / raw)
  To: Pali Rohár, Jianjun Wang
  Cc: Bjorn Helgaas, Rob Herring, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

On 2021-03-11 00:05, Pali Rohár wrote:
> On Wednesday 24 February 2021 14:11:30 Jianjun Wang wrote:
>> +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);
> 
> Marc, should not be there bitmap_release_region() with order_base_2()?
> 
> bitmap_release_region(port->msi_irq_in_use, data->hwirq, 
> order_base_2(nr_irqs));
> 
> Because mtk_msi_bottom_domain_alloc() is allocating
> order_base_2(nr_irqs) interrupts, not only nr_irqs.

Indeed, good catch.

Thanks,

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

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

* Re: [v8,5/7] PCI: mediatek-gen3: Add MSI support
       [not found]       ` <87a6rbxs4w.wl-maz@kernel.org>
@ 2021-03-11  9:47         ` Jianjun Wang
  0 siblings, 0 replies; 35+ messages in thread
From: Jianjun Wang @ 2021-03-11  9:47 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Bjorn Helgaas, Rob Herring, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

On Wed, 2021-03-10 at 09:41 +0000, Marc Zyngier wrote:
> On Wed, 10 Mar 2021 06:48:49 +0000,
> Jianjun Wang <jianjun.wang@mediatek.com> wrote:
> > > > +static struct irq_chip mtk_msi_irq_chip = {
> > > > +	.name = "MSI",
> > > > +	.irq_enable = mtk_pcie_irq_unmask,
> > > > +	.irq_disable = mtk_pcie_irq_mask,
> > > 
> > > Same comment as for the previous patch: enable/disable serve no
> > > purpose here.
> > 
> > Replied in the previous patch, the enable/disable callback is used when
> > the system suspend/resume.
> 
> As I said, your suspend/resume should be self contained, and not rely
> on the irq subsystem to restore a viable state.

OK, I will try to find another way to save and restore the enabled state
of interrupts when the system suspend/resume.

> 
> [...]
> 
> > > > @@ -408,6 +677,14 @@ static void mtk_pcie_irq_handler(struct irq_desc *desc)
> > > >  		generic_handle_irq(virq);
> > > >  	}
> > > >  
> > > > +	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);
> > > 
> > > Isn't this write the same thing you have for EOI in the INTx case?
> > > While I could understand your description in that case (this is a
> > > resampling operation), I don't get what this does here. Either this is
> > > also an EOI, but your initial description doesn't make sense, or it is
> > > an Ack, and it should be moved to the right place.
> > > 
> > > Which one is it?
> > 
> > I think it should be an EOI which used to clear the interrupt status of
> > a single set in the PCIe intc field, maybe I should move it to the end
> > of the mtk_pcie_msi_handler() function.
> 
> I doubt this is an EOI. If, as I suspect, it instructs the HW to clear
> the bit so that new pending bits can be recorded, it must take place
> *before* the interrupt is handled, or you may lose MSIs in the
> interval between the handling of the interrupt and the clearing of the
> pending bit. To satisfy this requirement, this should be an ACK, which
> is consistent with the way most MSI controllers such as this one work.

These bits are similar with the interrupt status of INTx, and the
interrupt status will remain until all the status of the corresponding
set are cleared. There is a while loop in mtk_pcie_msi_handler() which
is used to continuously polling and ACK the status of the MSI set, I
think the MSI may not be lose in this case.
 
> 
> > 
> >                   +-----+
> >                   | 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)
> > 
> > I would like to ask another question. In this interrupt architecture, we
> > cannot implement an affinity for PCIe interrupts, so we return a
> > negative value in the mtk_pcie_set_affinity callback as follows: 
> > 
> > +static int mtk_pcie_set_affinity(struct irq_data *data,
> > +                                const struct cpumask *mask, bool force)
> > +{
> > +       return -EINVAL;
> > +}
> > 
> > But there will always be error logs when hotplug a CPU:
> > 
> > ~ # echo 0 > /sys/devices/system/cpu/cpu1/online
> > [   93.633059] IRQ255: set affinity failed(-22).
> > [   93.633624] IRQ256: set affinity failed(-22).
> > [   93.634222] CPU1: shutdown
> > [   93.634586] psci: CPU1 killed (polled 0 ms)
> > 
> > Or when the system suspends:
> > 
> > ~ # echo mem > /sys/power/state
> > [   93.635145] cpuhp: cpu_off cluster=0, cpu=1
> > [  169.835653] PM: suspend entry (deep)
> > [  169.836717] Filesystems sync: 0.000 seconds
> > [  169.837924] Freezing user space processes ... (elapsed 0.001 seconds)
> > done.
> > [  169.839922] OOM killer disabled.
> > [  169.840336] Freezing remaining freezable tasks ... (elapsed 0.001
> > seconds) done.
> > [  169.844715] Disabling non-boot CPUs ...
> > [  169.846443] IRQ255: set affinity failed(-22).
> > [  169.847002] IRQ256: set affinity failed(-22).
> > [  169.847586] CPU2: shutdown
> > [  169.847943] psci: CPU2 killed (polled 0 ms)
> > [  169.848489] cpuhp: cpu_off cluster=0, cpu=2
> > [  169.850285] IRQ255: set affinity failed(-22).
> > [  169.851369] IRQ256: set affinity failed(-22).
> > ...
> > 
> > Sometimes this can cause misunderstandings to users, do we have a chance
> > to prevent this error log?
> 
> No. This HW doesn't allow MSIs to be individually retargeted, and the
> kernel isn't happy about that. That's one of the many reasons why
> hiding MSIs behind a mux (or two in your case) is a *very bad idea*.
> 
> Thanks,
> 
> 	M.
> 


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

* Re: [v8,5/7] PCI: mediatek-gen3: Add MSI support
  2021-03-11  8:19     ` Marc Zyngier
@ 2021-03-11  9:50       ` Jianjun Wang
  0 siblings, 0 replies; 35+ messages in thread
From: Jianjun Wang @ 2021-03-11  9:50 UTC (permalink / raw)
  To: Marc Zyngier, Pali Rohár
  Cc: Bjorn Helgaas, Rob Herring, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

On Thu, 2021-03-11 at 08:19 +0000, Marc Zyngier wrote:
> On 2021-03-11 00:05, Pali Rohár wrote:
> > On Wednesday 24 February 2021 14:11:30 Jianjun Wang wrote:
> >> +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);
> > 
> > Marc, should not be there bitmap_release_region() with order_base_2()?
> > 
> > bitmap_release_region(port->msi_irq_in_use, data->hwirq, 
> > order_base_2(nr_irqs));
> > 
> > Because mtk_msi_bottom_domain_alloc() is allocating
> > order_base_2(nr_irqs) interrupts, not only nr_irqs.
> 
> Indeed, good catch.

I will fix it in the next version, thanks for your review.

> 
> Thanks,
> 
>          M.


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

* Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
  2021-02-24  6:11 ` [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192 Jianjun Wang
  2021-02-24 13:36   ` Krzysztof Wilczyński
@ 2021-03-11 12:38   ` Pali Rohár
  2021-03-13  7:43     ` Jianjun Wang
  1 sibling, 1 reply; 35+ messages in thread
From: Pali Rohár @ 2021-03-11 12:38 UTC (permalink / raw)
  To: Jianjun Wang
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote:
> +static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
> +{
...
> +
> +	/* 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);

Hello! This is a new driver which introduce yet another custom timeout
prior PERST# signal for PCIe card is de-asserted. Timeouts for other
drivers I collected in older email [2].

Please look at my email [1] about PCIe Warm Reset if you have any clue
about it. Lorenzo and Rob already expressed that this timeout should not
be driver specific. But nobody was able to "decode" and "understand"
PCIe spec yet about these timeouts.

> +
> +	/* 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);

IIRC, you need to wait at least 100ms after de-asserting PERST# signal
as it is required by PCIe specs and also because experiments proved that
some Compex wifi cards (e.g. WLE900VX) are not detected if you do not
wait this minimal time.

> +	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;
> +	}

[1] - https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
[2] - https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/

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

* Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
  2021-03-11 12:38   ` Pali Rohár
@ 2021-03-13  7:43     ` Jianjun Wang
  2021-03-18  0:02       ` Pali Rohár
  0 siblings, 1 reply; 35+ messages in thread
From: Jianjun Wang @ 2021-03-13  7:43 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

On Thu, 2021-03-11 at 13:38 +0100, Pali Rohár wrote:
> On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote:
> > +static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
> > +{
> ...
> > +
> > +	/* 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);
> 
> Hello! This is a new driver which introduce yet another custom timeout
> prior PERST# signal for PCIe card is de-asserted. Timeouts for other
> drivers I collected in older email [2].
> 
> Please look at my email [1] about PCIe Warm Reset if you have any clue
> about it. Lorenzo and Rob already expressed that this timeout should not
> be driver specific. But nobody was able to "decode" and "understand"
> PCIe spec yet about these timeouts.

Hi Pali,

I think this is more like a platform specific timeout, which is used to
wait for the reference clocks to become stable and finish the reset flow
of HW blocks.

Here is the steps to start a link training in this HW:

1. Assert all reset signals which including the transaction layer, PIPE
interface and internal bus interface;

2. De-assert reset signals except the PERST#, this will make the
physical layer active and start to output the reference clock, but the
EP device remains in the reset state.
   Before releasing the PERST# signal, the HW blocks needs at least 10ms
to finish the reset flow, and ref-clk needs about 30us to become stable.

3. De-assert PERST# signal, wait LTSSM enter L0 state.

This 100ms timeout is reference to TPVPERL in the PCIe CEM spec. Since
we are in the kernel stage, the power supply has already stabled, this
timeout may not take that long.

> > +
> > +	/* 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);
> 
> IIRC, you need to wait at least 100ms after de-asserting PERST# signal
> as it is required by PCIe specs and also because experiments proved that
> some Compex wifi cards (e.g. WLE900VX) are not detected if you do not
> wait this minimal time.

Yes, this should be 100ms, I will fix it at next version, thanks for
your review.

Thanks.
> 
> > +	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;
> > +	}
> 
> [1] - https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
> [2] - https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/


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

* Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
  2021-03-13  7:43     ` Jianjun Wang
@ 2021-03-18  0:02       ` Pali Rohár
  2021-03-18  5:48         ` Jianjun Wang
  0 siblings, 1 reply; 35+ messages in thread
From: Pali Rohár @ 2021-03-18  0:02 UTC (permalink / raw)
  To: Jianjun Wang
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

On Saturday 13 March 2021 15:43:14 Jianjun Wang wrote:
> On Thu, 2021-03-11 at 13:38 +0100, Pali Rohár wrote:
> > On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote:
> > > +static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
> > > +{
> > ...
> > > +
> > > +	/* 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);
> > 
> > Hello! This is a new driver which introduce yet another custom timeout
> > prior PERST# signal for PCIe card is de-asserted. Timeouts for other
> > drivers I collected in older email [2].
> > 
> > Please look at my email [1] about PCIe Warm Reset if you have any clue
> > about it. Lorenzo and Rob already expressed that this timeout should not
> > be driver specific. But nobody was able to "decode" and "understand"
> > PCIe spec yet about these timeouts.
> 
> Hi Pali,
> 
> I think this is more like a platform specific timeout, which is used to
> wait for the reference clocks to become stable and finish the reset flow
> of HW blocks.
> 
> Here is the steps to start a link training in this HW:
> 
> 1. Assert all reset signals which including the transaction layer, PIPE
> interface and internal bus interface;
> 
> 2. De-assert reset signals except the PERST#, this will make the
> physical layer active and start to output the reference clock, but the
> EP device remains in the reset state.
>    Before releasing the PERST# signal, the HW blocks needs at least 10ms
> to finish the reset flow, and ref-clk needs about 30us to become stable.
> 
> 3. De-assert PERST# signal, wait LTSSM enter L0 state.
> 
> This 100ms timeout is reference to TPVPERL in the PCIe CEM spec. Since
> we are in the kernel stage, the power supply has already stabled, this
> timeout may not take that long.

I think that this is not platform specific timeout or platform specific
steps. This matches generic steps as defined in PCIe CEM spec, section
2.2.1. Initial Power-Up (G3 to S0).

What is platform specific is just how to achieve these steps.

Am I right?

...

TPVPERL is one of my timeout candidates as minimal required timeout for
Warm Reset. I have wrote it in email:

https://lore.kernel.org/linux-pci/20200430082245.xblvb7xeamm4e336@pali/

But I'm not sure as specially in none diagram is described just warm
reset as defined in mPCIe CEM (3.2.4.3. PERST# Signal).

...

Anyway, I would suggest to define constants for those timeouts. I guess
that in future we could be able to define "generic" timeout constants
which would not be in private driver section, but in some common header
file.

> > > +
> > > +	/* 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);
> > 
> > IIRC, you need to wait at least 100ms after de-asserting PERST# signal
> > as it is required by PCIe specs and also because experiments proved that
> > some Compex wifi cards (e.g. WLE900VX) are not detected if you do not
> > wait this minimal time.
> 
> Yes, this should be 100ms, I will fix it at next version, thanks for
> your review.

In past Bjorn suggested to use msleep(PCI_PM_D3COLD_WAIT); macro for
this step during reviewing aardvark driver.

https://lore.kernel.org/linux-pci/20190426161050.GA189964@google.com/

And next iteration used this PCI_PM_D3COLD_WAIT macro instead of 100:

https://lore.kernel.org/linux-pci/20190522213351.21366-2-repk@triplefau.lt/

> Thanks.
> > 
> > > +	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;
> > > +	}
> > 
> > [1] - https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
> > [2] - https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/
> 

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

* Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
  2021-03-18  0:02       ` Pali Rohár
@ 2021-03-18  5:48         ` Jianjun Wang
  2021-03-19 18:53           ` Pali Rohár
  2021-03-29 22:58           ` Pali Rohár
  0 siblings, 2 replies; 35+ messages in thread
From: Jianjun Wang @ 2021-03-18  5:48 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

On Thu, 2021-03-18 at 01:02 +0100, Pali Rohár wrote:
> On Saturday 13 March 2021 15:43:14 Jianjun Wang wrote:
> > On Thu, 2021-03-11 at 13:38 +0100, Pali Rohár wrote:
> > > On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote:
> > > > +static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
> > > > +{
> > > ...
> > > > +
> > > > +	/* 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);
> > > 
> > > Hello! This is a new driver which introduce yet another custom timeout
> > > prior PERST# signal for PCIe card is de-asserted. Timeouts for other
> > > drivers I collected in older email [2].
> > > 
> > > Please look at my email [1] about PCIe Warm Reset if you have any clue
> > > about it. Lorenzo and Rob already expressed that this timeout should not
> > > be driver specific. But nobody was able to "decode" and "understand"
> > > PCIe spec yet about these timeouts.
> > 
> > Hi Pali,
> > 
> > I think this is more like a platform specific timeout, which is used to
> > wait for the reference clocks to become stable and finish the reset flow
> > of HW blocks.
> > 
> > Here is the steps to start a link training in this HW:
> > 
> > 1. Assert all reset signals which including the transaction layer, PIPE
> > interface and internal bus interface;
> > 
> > 2. De-assert reset signals except the PERST#, this will make the
> > physical layer active and start to output the reference clock, but the
> > EP device remains in the reset state.
> >    Before releasing the PERST# signal, the HW blocks needs at least 10ms
> > to finish the reset flow, and ref-clk needs about 30us to become stable.
> > 
> > 3. De-assert PERST# signal, wait LTSSM enter L0 state.
> > 
> > This 100ms timeout is reference to TPVPERL in the PCIe CEM spec. Since
> > we are in the kernel stage, the power supply has already stabled, this
> > timeout may not take that long.
> 
> I think that this is not platform specific timeout or platform specific
> steps. This matches generic steps as defined in PCIe CEM spec, section
> 2.2.1. Initial Power-Up (G3 to S0).
> 
> What is platform specific is just how to achieve these steps.
> 
> Am I right?
> 
> ...
> 
> TPVPERL is one of my timeout candidates as minimal required timeout for
> Warm Reset. I have wrote it in email:
> 
> https://lore.kernel.org/linux-pci/20200430082245.xblvb7xeamm4e336@pali/
> 
> But I'm not sure as specially in none diagram is described just warm
> reset as defined in mPCIe CEM (3.2.4.3. PERST# Signal).
> 
> ...
> 
> Anyway, I would suggest to define constants for those timeouts. I guess
> that in future we could be able to define "generic" timeout constants
> which would not be in private driver section, but in some common header
> file.

I agree with this, but I'm not sure if we really need that long time in
the kernel stage, because the power supply has already stable and it's
really impact the boot time, especially when the platform have multi
ports and not connect any EP device, we need to wait 200ms for each port
when system bootup.

For this PCIe controller driver, I would like to change the timeout
value to 10ms to comply with the HW design, and save some boot time.

> 
> > > > +
> > > > +	/* 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);
> > > 
> > > IIRC, you need to wait at least 100ms after de-asserting PERST# signal
> > > as it is required by PCIe specs and also because experiments proved that
> > > some Compex wifi cards (e.g. WLE900VX) are not detected if you do not
> > > wait this minimal time.
> > 
> > Yes, this should be 100ms, I will fix it at next version, thanks for
> > your review.
> 
> In past Bjorn suggested to use msleep(PCI_PM_D3COLD_WAIT); macro for
> this step during reviewing aardvark driver.
> 
> https://lore.kernel.org/linux-pci/20190426161050.GA189964@google.com/
> 
> And next iteration used this PCI_PM_D3COLD_WAIT macro instead of 100:
> 
> https://lore.kernel.org/linux-pci/20190522213351.21366-2-repk@triplefau.lt/

Sure, I will use PCI_PM_D3COLD_WAIT macro instead in the next version.

Thanks.

> 
> > Thanks.
> > > 
> > > > +	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;
> > > > +	}
> > > 
> > > [1] - https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
> > > [2] - https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/
> > 


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

* Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
  2021-03-18  5:48         ` Jianjun Wang
@ 2021-03-19 18:53           ` Pali Rohár
  2021-03-23  1:31             ` Jianjun Wang
  2021-03-29 22:58           ` Pali Rohár
  1 sibling, 1 reply; 35+ messages in thread
From: Pali Rohár @ 2021-03-19 18:53 UTC (permalink / raw)
  To: Jianjun Wang
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

On Thursday 18 March 2021 13:48:07 Jianjun Wang wrote:
> On Thu, 2021-03-18 at 01:02 +0100, Pali Rohár wrote:
> > On Saturday 13 March 2021 15:43:14 Jianjun Wang wrote:
> > > On Thu, 2021-03-11 at 13:38 +0100, Pali Rohár wrote:
> > > > On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote:
> > > > > +static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
> > > > > +{
> > > > ...
> > > > > +
> > > > > +	/* 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);
> > > > 
> > > > Hello! This is a new driver which introduce yet another custom timeout
> > > > prior PERST# signal for PCIe card is de-asserted. Timeouts for other
> > > > drivers I collected in older email [2].
> > > > 
> > > > Please look at my email [1] about PCIe Warm Reset if you have any clue
> > > > about it. Lorenzo and Rob already expressed that this timeout should not
> > > > be driver specific. But nobody was able to "decode" and "understand"
> > > > PCIe spec yet about these timeouts.
> > > 
> > > Hi Pali,
> > > 
> > > I think this is more like a platform specific timeout, which is used to
> > > wait for the reference clocks to become stable and finish the reset flow
> > > of HW blocks.
> > > 
> > > Here is the steps to start a link training in this HW:
> > > 
> > > 1. Assert all reset signals which including the transaction layer, PIPE
> > > interface and internal bus interface;
> > > 
> > > 2. De-assert reset signals except the PERST#, this will make the
> > > physical layer active and start to output the reference clock, but the
> > > EP device remains in the reset state.
> > >    Before releasing the PERST# signal, the HW blocks needs at least 10ms
> > > to finish the reset flow, and ref-clk needs about 30us to become stable.
> > > 
> > > 3. De-assert PERST# signal, wait LTSSM enter L0 state.
> > > 
> > > This 100ms timeout is reference to TPVPERL in the PCIe CEM spec. Since
> > > we are in the kernel stage, the power supply has already stabled, this
> > > timeout may not take that long.
> > 
> > I think that this is not platform specific timeout or platform specific
> > steps. This matches generic steps as defined in PCIe CEM spec, section
> > 2.2.1. Initial Power-Up (G3 to S0).
> > 
> > What is platform specific is just how to achieve these steps.
> > 
> > Am I right?
> > 
> > ...
> > 
> > TPVPERL is one of my timeout candidates as minimal required timeout for
> > Warm Reset. I have wrote it in email:
> > 
> > https://lore.kernel.org/linux-pci/20200430082245.xblvb7xeamm4e336@pali/
> > 
> > But I'm not sure as specially in none diagram is described just warm
> > reset as defined in mPCIe CEM (3.2.4.3. PERST# Signal).
> > 
> > ...
> > 
> > Anyway, I would suggest to define constants for those timeouts. I guess
> > that in future we could be able to define "generic" timeout constants
> > which would not be in private driver section, but in some common header
> > file.
> 
> I agree with this, but I'm not sure if we really need that long time in
> the kernel stage, because the power supply has already stable and it's
> really impact the boot time, especially when the platform have multi
> ports and not connect any EP device, we need to wait 200ms for each port
> when system bootup.

Ports are independent. So you can initialize them in parallel, right?

If you initialize each port in separate worker then during msleep calls
kernel can schedule other kernel thread to run and so it does not
increase boot time. While pcie is sleeping kernel can do other things.
So the result is that whole boot time is not increased, just reordered.

> For this PCIe controller driver, I would like to change the timeout
> value to 10ms to comply with the HW design, and save some boot time.

In case you can connect _any_ PCIe card to your HW then you cannot
decrease or change timeouts required by PCIe specs. Otherwise there can
be a card which would not be initialized correctly.

I'm debugging driver for aardvark PCIe controller and I see that Compex
cards really needs these timeouts, otherwise link is down and card
cannot be detected.

So I guess that there can be also other cards which requires other
timeouts as specified in PCIe specs.

> > 
> > > > > +
> > > > > +	/* 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);
> > > > 
> > > > IIRC, you need to wait at least 100ms after de-asserting PERST# signal
> > > > as it is required by PCIe specs and also because experiments proved that
> > > > some Compex wifi cards (e.g. WLE900VX) are not detected if you do not
> > > > wait this minimal time.
> > > 
> > > Yes, this should be 100ms, I will fix it at next version, thanks for
> > > your review.
> > 
> > In past Bjorn suggested to use msleep(PCI_PM_D3COLD_WAIT); macro for
> > this step during reviewing aardvark driver.
> > 
> > https://lore.kernel.org/linux-pci/20190426161050.GA189964@google.com/
> > 
> > And next iteration used this PCI_PM_D3COLD_WAIT macro instead of 100:
> > 
> > https://lore.kernel.org/linux-pci/20190522213351.21366-2-repk@triplefau.lt/
> 
> Sure, I will use PCI_PM_D3COLD_WAIT macro instead in the next version.
> 
> Thanks.
> 
> > 
> > > Thanks.
> > > > 
> > > > > +	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;
> > > > > +	}
> > > > 
> > > > [1] - https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
> > > > [2] - https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/
> > > 
> 

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

* Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
  2021-03-19 18:53           ` Pali Rohár
@ 2021-03-23  1:31             ` Jianjun Wang
  2021-03-23 14:51               ` Pali Rohár
  0 siblings, 1 reply; 35+ messages in thread
From: Jianjun Wang @ 2021-03-23  1:31 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Bjorn Helgaas, Rob Herring, maz, Lorenzo Pieralisi, Ryder Lee,
	Philipp Zabel, Matthias Brugger, linux-pci, linux-mediatek,
	devicetree, linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

On Fri, 2021-03-19 at 19:53 +0100, Pali Rohár wrote:
> On Thursday 18 March 2021 13:48:07 Jianjun Wang wrote:
> > On Thu, 2021-03-18 at 01:02 +0100, Pali Rohár wrote:
> > > On Saturday 13 March 2021 15:43:14 Jianjun Wang wrote:
> > > > On Thu, 2021-03-11 at 13:38 +0100, Pali Rohár wrote:
> > > > > On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote:
> > > > > > +static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
> > > > > > +{
> > > > > ...
> > > > > > +
> > > > > > +	/* 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);
> > > > > 
> > > > > Hello! This is a new driver which introduce yet another custom timeout
> > > > > prior PERST# signal for PCIe card is de-asserted. Timeouts for other
> > > > > drivers I collected in older email [2].
> > > > > 
> > > > > Please look at my email [1] about PCIe Warm Reset if you have any clue
> > > > > about it. Lorenzo and Rob already expressed that this timeout should not
> > > > > be driver specific. But nobody was able to "decode" and "understand"
> > > > > PCIe spec yet about these timeouts.
> > > > 
> > > > Hi Pali,
> > > > 
> > > > I think this is more like a platform specific timeout, which is used to
> > > > wait for the reference clocks to become stable and finish the reset flow
> > > > of HW blocks.
> > > > 
> > > > Here is the steps to start a link training in this HW:
> > > > 
> > > > 1. Assert all reset signals which including the transaction layer, PIPE
> > > > interface and internal bus interface;
> > > > 
> > > > 2. De-assert reset signals except the PERST#, this will make the
> > > > physical layer active and start to output the reference clock, but the
> > > > EP device remains in the reset state.
> > > >    Before releasing the PERST# signal, the HW blocks needs at least 10ms
> > > > to finish the reset flow, and ref-clk needs about 30us to become stable.
> > > > 
> > > > 3. De-assert PERST# signal, wait LTSSM enter L0 state.
> > > > 
> > > > This 100ms timeout is reference to TPVPERL in the PCIe CEM spec. Since
> > > > we are in the kernel stage, the power supply has already stabled, this
> > > > timeout may not take that long.
> > > 
> > > I think that this is not platform specific timeout or platform specific
> > > steps. This matches generic steps as defined in PCIe CEM spec, section
> > > 2.2.1. Initial Power-Up (G3 to S0).
> > > 
> > > What is platform specific is just how to achieve these steps.
> > > 
> > > Am I right?
> > > 
> > > ...
> > > 
> > > TPVPERL is one of my timeout candidates as minimal required timeout for
> > > Warm Reset. I have wrote it in email:
> > > 
> > > https://lore.kernel.org/linux-pci/20200430082245.xblvb7xeamm4e336@pali/
> > > 
> > > But I'm not sure as specially in none diagram is described just warm
> > > reset as defined in mPCIe CEM (3.2.4.3. PERST# Signal).
> > > 
> > > ...
> > > 
> > > Anyway, I would suggest to define constants for those timeouts. I guess
> > > that in future we could be able to define "generic" timeout constants
> > > which would not be in private driver section, but in some common header
> > > file.
> > 
> > I agree with this, but I'm not sure if we really need that long time in
> > the kernel stage, because the power supply has already stable and it's
> > really impact the boot time, especially when the platform have multi
> > ports and not connect any EP device, we need to wait 200ms for each port
> > when system bootup.
> 
> Ports are independent. So you can initialize them in parallel, right?
> 
> If you initialize each port in separate worker then during msleep calls
> kernel can schedule other kernel thread to run and so it does not
> increase boot time. While pcie is sleeping kernel can do other things.
> So the result is that whole boot time is not increased, just reordered.
> 
> > For this PCIe controller driver, I would like to change the timeout
> > value to 10ms to comply with the HW design, and save some boot time.
> 
> In case you can connect _any_ PCIe card to your HW then you cannot
> decrease or change timeouts required by PCIe specs. Otherwise there can
> be a card which would not be initialized correctly.
> 
> I'm debugging driver for aardvark PCIe controller and I see that Compex
> cards really needs these timeouts, otherwise link is down and card
> cannot be detected.
> 
> So I guess that there can be also other cards which requires other
> timeouts as specified in PCIe specs.

OK, I'll keep this timeout value. 

One more question, is there any chance that we can put this linkup flow
to a more "standard" way, such as drivers provides the ops of the PERST#
pin and let the framework to decide how to start a link training, or we
just use macro to replace this timeout value in the future?

Thanks.

> 
> > > 
> > > > > > +
> > > > > > +	/* 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);
> > > > > 
> > > > > IIRC, you need to wait at least 100ms after de-asserting PERST# signal
> > > > > as it is required by PCIe specs and also because experiments proved that
> > > > > some Compex wifi cards (e.g. WLE900VX) are not detected if you do not
> > > > > wait this minimal time.
> > > > 
> > > > Yes, this should be 100ms, I will fix it at next version, thanks for
> > > > your review.
> > > 
> > > In past Bjorn suggested to use msleep(PCI_PM_D3COLD_WAIT); macro for
> > > this step during reviewing aardvark driver.
> > > 
> > > https://lore.kernel.org/linux-pci/20190426161050.GA189964@google.com/
> > > 
> > > And next iteration used this PCI_PM_D3COLD_WAIT macro instead of 100:
> > > 
> > > https://lore.kernel.org/linux-pci/20190522213351.21366-2-repk@triplefau.lt/
> > 
> > Sure, I will use PCI_PM_D3COLD_WAIT macro instead in the next version.
> > 
> > Thanks.
> > 
> > > 
> > > > Thanks.
> > > > > 
> > > > > > +	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;
> > > > > > +	}
> > > > > 
> > > > > [1] - https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
> > > > > [2] - https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/
> > > > 
> > 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek


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

* Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
  2021-03-23  1:31             ` Jianjun Wang
@ 2021-03-23 14:51               ` Pali Rohár
  0 siblings, 0 replies; 35+ messages in thread
From: Pali Rohár @ 2021-03-23 14:51 UTC (permalink / raw)
  To: Jianjun Wang
  Cc: Bjorn Helgaas, Alex Williamson, Amey Narkhede, Rob Herring, maz,
	Lorenzo Pieralisi, Ryder Lee, Philipp Zabel, Matthias Brugger,
	linux-pci, linux-mediatek, devicetree, linux-kernel,
	linux-arm-kernel, Sj Huang, youlin.pei, chuanjia.liu,
	qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen, anson.chuang

On Tuesday 23 March 2021 09:31:34 Jianjun Wang wrote:
> One more question, is there any chance that we can put this linkup flow
> to a more "standard" way, such as drivers provides the ops of the PERST#
> pin and let the framework to decide how to start a link training, or we
> just use macro to replace this timeout value in the future?

This is something about which I was thinking that could be useful for
pci-aardvark.c driver. But I was not sure if some other driver can
benefit from such "framework". But now I see that your driver is another
candidate which can benefit from it.

Currently there is no such "framework" in kernel and the hardest part
would be to design it.

Having this API would allow kernel to implement and export PCIe Warm
Reset (which is done via PERST# signal) and easily extend Amey's reset
patches to export also Warm Reset via sysfs.

But to implement this framework and using it for reset we first need to
answer questions which I have sent in email:
https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/

Bjorn, Alex: any opinion about PERST#?

Also see Enrico's email, where confirmed that there are platforms which
shares one PERST# signal for more endpoint cards:
https://lore.kernel.org/linux-pci/1da0fa2c-8056-9ae8-6ce4-ab645317772d@metux.net/

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

* Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
  2021-03-18  5:48         ` Jianjun Wang
  2021-03-19 18:53           ` Pali Rohár
@ 2021-03-29 22:58           ` Pali Rohár
  1 sibling, 0 replies; 35+ messages in thread
From: Pali Rohár @ 2021-03-29 22:58 UTC (permalink / raw)
  To: Jianjun Wang, Lorenzo Pieralisi
  Cc: Bjorn Helgaas, Rob Herring, maz, Ryder Lee, Philipp Zabel,
	Matthias Brugger, linux-pci, linux-mediatek, devicetree,
	linux-kernel, linux-arm-kernel, Sj Huang, youlin.pei,
	chuanjia.liu, qizhong.cheng, sin_jieyang, drinkcat, Rex-BC.Chen,
	anson.chuang

On Thursday 18 March 2021 13:48:07 Jianjun Wang wrote:
> On Thu, 2021-03-18 at 01:02 +0100, Pali Rohár wrote:
> > On Saturday 13 March 2021 15:43:14 Jianjun Wang wrote:
> > > On Thu, 2021-03-11 at 13:38 +0100, Pali Rohár wrote:
> > > > On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote:
> > > > > +
> > > > > +	/* 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);
> > > > 
> > > > IIRC, you need to wait at least 100ms after de-asserting PERST# signal
> > > > as it is required by PCIe specs and also because experiments proved that
> > > > some Compex wifi cards (e.g. WLE900VX) are not detected if you do not
> > > > wait this minimal time.
> > > 
> > > Yes, this should be 100ms, I will fix it at next version, thanks for
> > > your review.
> > 
> > In past Bjorn suggested to use msleep(PCI_PM_D3COLD_WAIT); macro for
> > this step during reviewing aardvark driver.
> > 
> > https://lore.kernel.org/linux-pci/20190426161050.GA189964@google.com/
> > 
> > And next iteration used this PCI_PM_D3COLD_WAIT macro instead of 100:
> > 
> > https://lore.kernel.org/linux-pci/20190522213351.21366-2-repk@triplefau.lt/
> 
> Sure, I will use PCI_PM_D3COLD_WAIT macro instead in the next version.
> 
> Thanks.

Anyway, now I found out that kernel has functions for this waiting:
pcie_wait_for_link_delay() and pcie_wait_for_link()

Function is called from pci_bridge_wait_for_secondary_bus().

But in current form it is not usable for native controller drivers.

This looks like another candidate for code de-duplication or providing
"framework".


Lorenzo, as maintainer of native controller drivers, do you have some
ideas about providing "framework", common functions or something for
avoiding to implement same code patterns in every native controller
driver, which is de-facto standard PCIe codepath? Including a way how to
export PERST# reset gpio?

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

end of thread, other threads:[~2021-03-29 22:59 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-24  6:11 [v8,0/7] PCI: mediatek: Add new generation controller support Jianjun Wang
2021-02-24  6:11 ` [v8,1/7] dt-bindings: PCI: mediatek-gen3: Add YAML schema Jianjun Wang
2021-03-06 20:09   ` Rob Herring
2021-02-24  6:11 ` [v8,2/7] PCI: Export pci_pio_to_address() for module use Jianjun Wang
2021-02-24  6:11 ` [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192 Jianjun Wang
2021-02-24 13:36   ` Krzysztof Wilczyński
2021-02-25  3:07     ` Jianjun Wang
2021-03-11 12:38   ` Pali Rohár
2021-03-13  7:43     ` Jianjun Wang
2021-03-18  0:02       ` Pali Rohár
2021-03-18  5:48         ` Jianjun Wang
2021-03-19 18:53           ` Pali Rohár
2021-03-23  1:31             ` Jianjun Wang
2021-03-23 14:51               ` Pali Rohár
2021-03-29 22:58           ` Pali Rohár
2021-02-24  6:11 ` [v8,4/7] PCI: mediatek-gen3: Add INTx support Jianjun Wang
2021-02-24 14:24   ` Krzysztof Wilczyński
2021-02-25  3:10     ` Jianjun Wang
2021-03-09 11:10   ` Marc Zyngier
2021-03-10  3:05     ` Jianjun Wang
2021-02-24  6:11 ` [v8,5/7] PCI: mediatek-gen3: Add MSI support Jianjun Wang
2021-02-24 14:31   ` Krzysztof Wilczyński
2021-02-25  3:09     ` Jianjun Wang
2021-03-09 11:23   ` Marc Zyngier
2021-03-10  6:48     ` Jianjun Wang
     [not found]       ` <87a6rbxs4w.wl-maz@kernel.org>
2021-03-11  9:47         ` Jianjun Wang
2021-03-11  0:05   ` Pali Rohár
2021-03-11  8:19     ` Marc Zyngier
2021-03-11  9:50       ` Jianjun Wang
2021-02-24  6:11 ` [v8,6/7] PCI: mediatek-gen3: Add system PM support Jianjun Wang
2021-02-24 14:10   ` Krzysztof Wilczyński
2021-02-25  3:34     ` Jianjun Wang
2021-02-25 22:00       ` Krzysztof Wilczyński
2021-02-26 10:06         ` Jianjun Wang
2021-02-24  6:11 ` [v8,7/7] MAINTAINERS: Add Jianjun Wang as MediaTek PCI co-maintainer Jianjun Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).