[PATCH v4 0/4] Add Airoha EN7581 PCIe support

[PATCH v4 0/4] Add Airoha EN7581 PCIe support

[Lorenzo Bianconi]

Introduce support for EN7581 SoC to mediatek-gen3 PCIe driver

Changes since v3:
- move phy initialization delay in pcie-phy driver
- rename en7581 PCIe reset delay
- fix compilation warning
Changes since v2:
- fix dt-bindings clock definitions
- fix mtk_pcie_of_match ordering
- add register definitions
- move pcie-phy registers configuration in pcie-phy driver
Changes since v1:
- remove register magic values
- remove delay magic values
- cosmetics
- fix dts binding for clock/reset

Lorenzo Bianconi (4):
  dt-bindings: PCI: mediatek-gen3: add support for Airoha EN7581
  PCI: mediatek-gen3: Add mtk_gen3_pcie_pdata data structure
  PCI: mediatek-gen3: Rely on reset_bulk APIs for PHY reset lines
  PCI: mediatek-gen3: Add Airoha EN7581 support

 .../bindings/pci/mediatek-pcie-gen3.yaml      |  68 ++++++-
 drivers/pci/controller/Kconfig                |   2 +-
 drivers/pci/controller/pcie-mediatek-gen3.c   | 180 ++++++++++++++++--
 3 files changed, 229 insertions(+), 21 deletions(-)

-- 
2.45.2

[PATCH v4 1/4] dt-bindings: PCI: mediatek-gen3: add support for Airoha EN7581

[Lorenzo Bianconi]

Introduce Airoha EN7581 entry in mediatek-gen3 PCIe controller binding

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 .../bindings/pci/mediatek-pcie-gen3.yaml      | 68 +++++++++++++++++--
 1 file changed, 63 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
index 76d742051f73..898c1be2d6a4 100644
--- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
+++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
@@ -53,6 +53,7 @@ properties:
               - mediatek,mt8195-pcie
           - const: mediatek,mt8192-pcie
       - const: mediatek,mt8192-pcie
+      - const: airoha,en7581-pcie
 
   reg:
     maxItems: 1
@@ -76,20 +77,20 @@ properties:
 
   resets:
     minItems: 1
-    maxItems: 2
+    maxItems: 3
 
   reset-names:
     minItems: 1
-    maxItems: 2
+    maxItems: 3
     items:
-      enum: [ phy, mac ]
+      enum: [ phy, mac, phy-lane0, phy-lane1, phy-lane2 ]
 
   clocks:
-    minItems: 4
+    minItems: 1
     maxItems: 6
 
   clock-names:
-    minItems: 4
+    minItems: 1
     maxItems: 6
 
   assigned-clocks:
@@ -147,6 +148,9 @@ allOf:
           const: mediatek,mt8192-pcie
     then:
       properties:
+        clocks:
+          minItems: 4
+
         clock-names:
           items:
             - const: pl_250m
@@ -155,6 +159,15 @@ allOf:
             - const: tl_32k
             - const: peri_26m
             - const: top_133m
+
+        resets:
+          minItems: 1
+          maxItems: 2
+
+        reset-names:
+          minItems: 1
+          maxItems: 2
+
   - if:
       properties:
         compatible:
@@ -164,6 +177,9 @@ allOf:
               - mediatek,mt8195-pcie
     then:
       properties:
+        clocks:
+          minItems: 4
+
         clock-names:
           items:
             - const: pl_250m
@@ -172,6 +188,15 @@ allOf:
             - const: tl_32k
             - const: peri_26m
             - const: peri_mem
+
+        resets:
+          minItems: 1
+          maxItems: 2
+
+        reset-names:
+          minItems: 1
+          maxItems: 2
+
   - if:
       properties:
         compatible:
@@ -180,6 +205,9 @@ allOf:
               - mediatek,mt7986-pcie
     then:
       properties:
+        clocks:
+          minItems: 4
+
         clock-names:
           items:
             - const: pl_250m
@@ -187,6 +215,36 @@ allOf:
             - const: peri_26m
             - const: top_133m
 
+        resets:
+          minItems: 1
+          maxItems: 2
+
+        reset-names:
+          minItems: 1
+          maxItems: 2
+
+  - if:
+      properties:
+        compatible:
+          const: airoha,en7581-pcie
+    then:
+      properties:
+        clocks:
+          maxItems: 1
+
+        clock-names:
+          items:
+            - const: sys-ck
+
+        resets:
+          minItems: 3
+
+        reset-names:
+          items:
+            - const: phy-lane0
+            - const: phy-lane1
+            - const: phy-lane2
+
 unevaluatedProperties: false
 
 examples:
-- 
2.45.2

[PATCH v4 2/4] PCI: mediatek-gen3: Add mtk_gen3_pcie_pdata data structure

[Lorenzo Bianconi]

Introduce mtk_gen3_pcie_pdata data structure in order to define
multiple callbacks for each supported SoC. This is a preliminary
patch to introduce EN7581 PCIe support.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 24 ++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 975b3024fb08..db0210803731 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -100,6 +100,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_gen3_pcie;
+
+/**
+ * struct mtk_gen3_pcie_pdata - differentiate between host generations
+ * @power_up: pcie power_up callback
+ */
+struct mtk_gen3_pcie_pdata {
+	int (*power_up)(struct mtk_gen3_pcie *pcie);
+};
+
 /**
  * struct mtk_msi_set - MSI information for each set
  * @base: IO mapped register base
@@ -131,6 +141,7 @@ struct mtk_msi_set {
  * @msi_sets: MSI sets information
  * @lock: lock protecting IRQ bit map
  * @msi_irq_in_use: bit map for assigned MSI IRQ
+ * @soc: pointer to SoC-dependent operations
  */
 struct mtk_gen3_pcie {
 	struct device *dev;
@@ -151,6 +162,8 @@ struct mtk_gen3_pcie {
 	struct mtk_msi_set msi_sets[PCIE_MSI_SET_NUM];
 	struct mutex lock;
 	DECLARE_BITMAP(msi_irq_in_use, PCIE_MSI_IRQS_NUM);
+
+	const struct mtk_gen3_pcie_pdata *soc;
 };
 
 /* LTSSM state in PCIE_LTSSM_STATUS_REG bit[28:24] */
@@ -904,7 +917,7 @@ static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
 	usleep_range(10, 20);
 
 	/* Don't touch the hardware registers before power up */
-	err = mtk_pcie_power_up(pcie);
+	err = pcie->soc->power_up(pcie);
 	if (err)
 		return err;
 
@@ -939,6 +952,7 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	pcie = pci_host_bridge_priv(host);
 
 	pcie->dev = dev;
+	pcie->soc = device_get_match_data(dev);
 	platform_set_drvdata(pdev, pcie);
 
 	err = mtk_pcie_setup(pcie);
@@ -1054,7 +1068,7 @@ static int mtk_pcie_resume_noirq(struct device *dev)
 	struct mtk_gen3_pcie *pcie = dev_get_drvdata(dev);
 	int err;
 
-	err = mtk_pcie_power_up(pcie);
+	err = pcie->soc->power_up(pcie);
 	if (err)
 		return err;
 
@@ -1074,8 +1088,12 @@ static const struct dev_pm_ops mtk_pcie_pm_ops = {
 				  mtk_pcie_resume_noirq)
 };
 
+static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_mt8192 = {
+	.power_up = mtk_pcie_power_up,
+};
+
 static const struct of_device_id mtk_pcie_of_match[] = {
-	{ .compatible = "mediatek,mt8192-pcie" },
+	{ .compatible = "mediatek,mt8192-pcie", .data = &mtk_pcie_soc_mt8192 },
 	{},
 };
 MODULE_DEVICE_TABLE(of, mtk_pcie_of_match);
-- 
2.45.2

[PATCH v4 3/4] PCI: mediatek-gen3: Rely on reset_bulk APIs for PHY reset lines

[Lorenzo Bianconi]

Use reset_bulk APIs to manage PHY reset lines. This is a preliminary
patch in order to add Airoha EN7581 PCIe support.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 45 +++++++++++++++------
 1 file changed, 33 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index db0210803731..438a5222d986 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -100,14 +100,21 @@
 #define PCIE_ATR_TLP_TYPE_MEM		PCIE_ATR_TLP_TYPE(0)
 #define PCIE_ATR_TLP_TYPE_IO		PCIE_ATR_TLP_TYPE(2)
 
+#define MAX_NUM_PHY_RESETS		1
+
 struct mtk_gen3_pcie;
 
 /**
  * struct mtk_gen3_pcie_pdata - differentiate between host generations
  * @power_up: pcie power_up callback
+ * @phy_resets: phy reset lines SoC data.
  */
 struct mtk_gen3_pcie_pdata {
 	int (*power_up)(struct mtk_gen3_pcie *pcie);
+	struct {
+		const char *id[MAX_NUM_PHY_RESETS];
+		int num_resets;
+	} phy_resets;
 };
 
 /**
@@ -128,7 +135,7 @@ struct mtk_msi_set {
  * @base: IO mapped register base
  * @reg_base: physical register base
  * @mac_reset: MAC reset control
- * @phy_reset: PHY reset control
+ * @phy_resets: PHY reset controllers
  * @phy: PHY controller block
  * @clks: PCIe clocks
  * @num_clks: PCIe clocks count for this port
@@ -148,7 +155,7 @@ struct mtk_gen3_pcie {
 	void __iomem *base;
 	phys_addr_t reg_base;
 	struct reset_control *mac_reset;
-	struct reset_control *phy_reset;
+	struct reset_control_bulk_data phy_resets[MAX_NUM_PHY_RESETS];
 	struct phy *phy;
 	struct clk_bulk_data *clks;
 	int num_clks;
@@ -788,10 +795,10 @@ static int mtk_pcie_setup_irq(struct mtk_gen3_pcie *pcie)
 
 static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
 {
+	int i, ret, num_resets = pcie->soc->phy_resets.num_resets;
 	struct device *dev = pcie->dev;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct resource *regs;
-	int ret;
 
 	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");
 	if (!regs)
@@ -804,12 +811,12 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
 
 	pcie->reg_base = regs->start;
 
-	pcie->phy_reset = devm_reset_control_get_optional_exclusive(dev, "phy");
-	if (IS_ERR(pcie->phy_reset)) {
-		ret = PTR_ERR(pcie->phy_reset);
-		if (ret != -EPROBE_DEFER)
-			dev_err(dev, "failed to get PHY reset\n");
+	for (i = 0; i < num_resets; i++)
+		pcie->phy_resets[i].id = pcie->soc->phy_resets.id[i];
 
+	ret = devm_reset_control_bulk_get_optional_shared(dev, num_resets, pcie->phy_resets);
+	if (ret) {
+		dev_err(dev, "failed to get PHY bulk reset\n");
 		return ret;
 	}
 
@@ -846,7 +853,11 @@ static int mtk_pcie_power_up(struct mtk_gen3_pcie *pcie)
 	int err;
 
 	/* PHY power on and enable pipe clock */
-	reset_control_deassert(pcie->phy_reset);
+	err = reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
+	if (err) {
+		dev_err(dev, "failed to deassert PHYs\n");
+		return err;
+	}
 
 	err = phy_init(pcie->phy);
 	if (err) {
@@ -882,7 +893,7 @@ static int mtk_pcie_power_up(struct mtk_gen3_pcie *pcie)
 err_phy_on:
 	phy_exit(pcie->phy);
 err_phy_init:
-	reset_control_assert(pcie->phy_reset);
+	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
 
 	return err;
 }
@@ -897,7 +908,7 @@ static void mtk_pcie_power_down(struct mtk_gen3_pcie *pcie)
 
 	phy_power_off(pcie->phy);
 	phy_exit(pcie->phy);
-	reset_control_assert(pcie->phy_reset);
+	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
 }
 
 static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
@@ -908,11 +919,17 @@ static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
 	if (err)
 		return err;
 
+	/*
+	 * Deassert the line in order to avoid unbalance in deassert_count
+	 * counter since the bulk is shared.
+	 */
+	reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
 	/*
 	 * The controller may have been left out of reset by the bootloader
 	 * so make sure that we get a clean start by asserting resets here.
 	 */
-	reset_control_assert(pcie->phy_reset);
+	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
+
 	reset_control_assert(pcie->mac_reset);
 	usleep_range(10, 20);
 
@@ -1090,6 +1107,10 @@ static const struct dev_pm_ops mtk_pcie_pm_ops = {
 
 static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_mt8192 = {
 	.power_up = mtk_pcie_power_up,
+	.phy_resets = {
+		.id[0] = "phy",
+		.num_resets = 1,
+	},
 };
 
 static const struct of_device_id mtk_pcie_of_match[] = {
-- 
2.45.2

[PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Lorenzo Bianconi]

Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
PCIe controller driver.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/pci/controller/Kconfig              |   2 +-
 drivers/pci/controller/pcie-mediatek-gen3.c | 113 +++++++++++++++++++-
 2 files changed, 113 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index e534c02ee34f..3bd6c9430010 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -196,7 +196,7 @@ config PCIE_MEDIATEK
 
 config PCIE_MEDIATEK_GEN3
 	tristate "MediaTek Gen3 PCIe controller"
-	depends on ARCH_MEDIATEK || COMPILE_TEST
+	depends on ARCH_AIROHA || ARCH_MEDIATEK || COMPILE_TEST
 	depends on PCI_MSI
 	help
 	  Adds support for PCIe Gen3 MAC controller for MediaTek SoCs.
diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 438a5222d986..e064f467ced6 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -6,7 +6,9 @@
  * Author: Jianjun Wang <jianjun.wang@mediatek.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/delay.h>
 #include <linux/iopoll.h>
 #include <linux/irq.h>
@@ -15,6 +17,8 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/msi.h>
+#include <linux/of_device.h>
+#include <linux/of_pci.h>
 #include <linux/pci.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
@@ -29,6 +33,12 @@
 #define PCI_CLASS(class)		(class << 8)
 #define PCIE_RC_MODE			BIT(0)
 
+#define PCIE_EQ_PRESET_01_REG		0x100
+#define PCIE_VAL_LN0_DOWNSTREAM		GENMASK(6, 0)
+#define PCIE_VAL_LN0_UPSTREAM		GENMASK(14, 8)
+#define PCIE_VAL_LN1_DOWNSTREAM		GENMASK(22, 16)
+#define PCIE_VAL_LN1_UPSTREAM		GENMASK(30, 24)
+
 #define PCIE_CFGNUM_REG			0x140
 #define PCIE_CFG_DEVFN(devfn)		((devfn) & GENMASK(7, 0))
 #define PCIE_CFG_BUS(bus)		(((bus) << 8) & GENMASK(15, 8))
@@ -68,6 +78,14 @@
 #define PCIE_MSI_SET_ENABLE_REG		0x190
 #define PCIE_MSI_SET_ENABLE		GENMASK(PCIE_MSI_SET_NUM - 1, 0)
 
+#define PCIE_PIPE4_PIE8_REG		0x338
+#define PCIE_K_FINETUNE_MAX		GENMASK(5, 0)
+#define PCIE_K_FINETUNE_ERR		GENMASK(7, 6)
+#define PCIE_K_PRESET_TO_USE		GENMASK(18, 8)
+#define PCIE_K_PHYPARAM_QUERY		BIT(19)
+#define PCIE_K_QUERY_TIMEOUT		BIT(20)
+#define PCIE_K_PRESET_TO_USE_16G	GENMASK(31, 21)
+
 #define PCIE_MSI_SET_BASE_REG		0xc00
 #define PCIE_MSI_SET_OFFSET		0x10
 #define PCIE_MSI_SET_STATUS_OFFSET	0x04
@@ -100,7 +118,10 @@
 #define PCIE_ATR_TLP_TYPE_MEM		PCIE_ATR_TLP_TYPE(0)
 #define PCIE_ATR_TLP_TYPE_IO		PCIE_ATR_TLP_TYPE(2)
 
-#define MAX_NUM_PHY_RESETS		1
+#define MAX_NUM_PHY_RESETS		3
+
+/* Time in ms needed to complete PCIe reset on EN7581 SoC */
+#define PCIE_EN7581_RESET_TIME_MS	100
 
 struct mtk_gen3_pcie;
 
@@ -847,6 +868,85 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
 	return 0;
 }
 
+static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	int err;
+	u32 val;
+
+	/*
+	 * Wait for the time needed to complete the bulk assert in
+	 * mtk_pcie_setup for EN7581 SoC.
+	 */
+	mdelay(PCIE_EN7581_RESET_TIME_MS);
+
+	err = phy_init(pcie->phy);
+	if (err) {
+		dev_err(dev, "failed to initialize PHY\n");
+		return err;
+	}
+
+	err = phy_power_on(pcie->phy);
+	if (err) {
+		dev_err(dev, "failed to power on PHY\n");
+		goto err_phy_on;
+	}
+
+	err = reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
+	if (err) {
+		dev_err(dev, "failed to deassert PHYs\n");
+		goto err_phy_deassert;
+	}
+
+	/*
+	 * Wait for the time needed to complete the bulk de-assert above.
+	 * This time is specific for EN7581 SoC.
+	 */
+	mdelay(PCIE_EN7581_RESET_TIME_MS);
+
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+
+	err = clk_bulk_prepare(pcie->num_clks, pcie->clks);
+	if (err) {
+		dev_err(dev, "failed to prepare clock\n");
+		goto err_clk_prepare;
+	}
+
+	val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
+	      FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
+	      FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
+	      FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41);
+	writel_relaxed(val, pcie->base + PCIE_EQ_PRESET_01_REG);
+
+	val = PCIE_K_PHYPARAM_QUERY | PCIE_K_QUERY_TIMEOUT |
+	      FIELD_PREP(PCIE_K_PRESET_TO_USE_16G, 0x80) |
+	      FIELD_PREP(PCIE_K_PRESET_TO_USE, 0x2) |
+	      FIELD_PREP(PCIE_K_FINETUNE_MAX, 0xf);
+	writel_relaxed(val, pcie->base + PCIE_PIPE4_PIE8_REG);
+
+	err = clk_bulk_enable(pcie->num_clks, pcie->clks);
+	if (err) {
+		dev_err(dev, "failed to prepare clock\n");
+		goto err_clk_enable;
+	}
+
+	return 0;
+
+err_clk_enable:
+	clk_bulk_unprepare(pcie->num_clks, pcie->clks);
+err_clk_prepare:
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
+err_phy_deassert:
+	phy_power_off(pcie->phy);
+err_phy_on:
+	phy_exit(pcie->phy);
+
+	return err;
+}
+
 static int mtk_pcie_power_up(struct mtk_gen3_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
@@ -1113,7 +1213,18 @@ static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_mt8192 = {
 	},
 };
 
+static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_en7581 = {
+	.power_up = mtk_pcie_en7581_power_up,
+	.phy_resets = {
+		.id[0] = "phy-lane0",
+		.id[1] = "phy-lane1",
+		.id[2] = "phy-lane2",
+		.num_resets = 3,
+	},
+};
+
 static const struct of_device_id mtk_pcie_of_match[] = {
+	{ .compatible = "airoha,en7581-pcie", .data = &mtk_pcie_soc_en7581 },
 	{ .compatible = "mediatek,mt8192-pcie", .data = &mtk_pcie_soc_mt8192 },
 	{},
 };
-- 
2.45.2

Re: [PATCH v4 1/4] dt-bindings: PCI: mediatek-gen3: add support for Airoha EN7581

[AngeloGioacchino Del Regno]

Il 03/07/24 18:12, Lorenzo Bianconi ha scritto:
> Introduce Airoha EN7581 entry in mediatek-gen3 PCIe controller binding
> 
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v4 1/4] dt-bindings: PCI: mediatek-gen3: add support for Airoha EN7581

[Jianjun Wang (王建军)]

On Thu, 2024-07-04 at 10:23 +0200, AngeloGioacchino Del Regno wrote:
> Il 03/07/24 18:12, Lorenzo Bianconi ha scritto:
> > Introduce Airoha EN7581 entry in mediatek-gen3 PCIe controller
> > binding
> > 
> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> Reviewed-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> 

Acked-by: Jianjun Wang <jianjun.wang@mediatek.com>

Re: [PATCH v4 2/4] PCI: mediatek-gen3: Add mtk_gen3_pcie_pdata data structure

[Jianjun Wang (王建军)]

On Wed, 2024-07-03 at 18:12 +0200, Lorenzo Bianconi wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Introduce mtk_gen3_pcie_pdata data structure in order to define
> multiple callbacks for each supported SoC. This is a preliminary
> patch to introduce EN7581 PCIe support.
> 
> Reviewed-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Acked-by: Jianjun Wang <jianjun.wang@mediatek.com>

Re: [PATCH v4 3/4] PCI: mediatek-gen3: Rely on reset_bulk APIs for PHY reset lines

[Jianjun Wang (王建军)]

On Wed, 2024-07-03 at 18:12 +0200, Lorenzo Bianconi wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Use reset_bulk APIs to manage PHY reset lines. This is a preliminary
> patch in order to add Airoha EN7581 PCIe support.
> 
> Reviewed-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Acked-by: Jianjun Wang <jianjun.wang@mediatek.com>

Re: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Jianjun Wang (王建军)]

On Wed, 2024-07-03 at 18:12 +0200, Lorenzo Bianconi wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
> PCIe controller driver.
> 
> Reviewed-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Acked-by: Jianjun Wang <jianjun.wang@mediatek.com>

Re: [PATCH v4 0/4] Add Airoha EN7581 PCIe support

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1214 bytes --]

> Introduce support for EN7581 SoC to mediatek-gen3 PCIe driver
> 
> Changes since v3:
> - move phy initialization delay in pcie-phy driver
> - rename en7581 PCIe reset delay
> - fix compilation warning
> Changes since v2:
> - fix dt-bindings clock definitions
> - fix mtk_pcie_of_match ordering
> - add register definitions
> - move pcie-phy registers configuration in pcie-phy driver
> Changes since v1:
> - remove register magic values
> - remove delay magic values
> - cosmetics
> - fix dts binding for clock/reset
> 
> Lorenzo Bianconi (4):
>   dt-bindings: PCI: mediatek-gen3: add support for Airoha EN7581
>   PCI: mediatek-gen3: Add mtk_gen3_pcie_pdata data structure
>   PCI: mediatek-gen3: Rely on reset_bulk APIs for PHY reset lines
>   PCI: mediatek-gen3: Add Airoha EN7581 support
> 
>  .../bindings/pci/mediatek-pcie-gen3.yaml      |  68 ++++++-
>  drivers/pci/controller/Kconfig                |   2 +-
>  drivers/pci/controller/pcie-mediatek-gen3.c   | 180 ++++++++++++++++--
>  3 files changed, 229 insertions(+), 21 deletions(-)
> 
> -- 
> 2.45.2
> 

Hi all,

any update about this series? Am I supposed to do something? Thanks in advance.

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 0/4] Add Airoha EN7581 PCIe support

[Bjorn Helgaas]

On Tue, Aug 20, 2024 at 10:46:48AM +0200, Lorenzo Bianconi wrote:
> > Introduce support for EN7581 SoC to mediatek-gen3 PCIe driver
> > 
> > Changes since v3:
> > - move phy initialization delay in pcie-phy driver
> > - rename en7581 PCIe reset delay
> > - fix compilation warning
> > Changes since v2:
> > - fix dt-bindings clock definitions
> > - fix mtk_pcie_of_match ordering
> > - add register definitions
> > - move pcie-phy registers configuration in pcie-phy driver
> > Changes since v1:
> > - remove register magic values
> > - remove delay magic values
> > - cosmetics
> > - fix dts binding for clock/reset
> > 
> > Lorenzo Bianconi (4):
> >   dt-bindings: PCI: mediatek-gen3: add support for Airoha EN7581
> >   PCI: mediatek-gen3: Add mtk_gen3_pcie_pdata data structure
> >   PCI: mediatek-gen3: Rely on reset_bulk APIs for PHY reset lines
> >   PCI: mediatek-gen3: Add Airoha EN7581 support
> > 
> >  .../bindings/pci/mediatek-pcie-gen3.yaml      |  68 ++++++-
> >  drivers/pci/controller/Kconfig                |   2 +-
> >  drivers/pci/controller/pcie-mediatek-gen3.c   | 180 ++++++++++++++++--
> >  3 files changed, 229 insertions(+), 21 deletions(-)
> > 
> > -- 
> > 2.45.2
> > 
> 
> Hi all,
> 
> any update about this series? Am I supposed to do something? Thanks in advance.

No need to do anything, I think Krzysztof will likely pick this up
when he has a minute.

Bjorn

Re: [PATCH v4 0/4] Add Airoha EN7581 PCIe support

[Krzysztof Wilczyński]

Hello,

> Introduce support for EN7581 SoC to mediatek-gen3 PCIe driver

Applied to controller/mediatek-gen3, thank you!

[01/04] dt-bindings: PCI: mediatek-gen3: Add support for Airoha EN7581
        https://git.kernel.org/pci/pci/c/c6abd0eadec6

[02/04] PCI: mediatek-gen3: Add mtk_gen3_pcie_pdata data structure
        https://git.kernel.org/pci/pci/c/dc869a40d73e

[03/04] PCI: mediatek-gen3: Rely on reset_bulk APIs for PHY reset lines
        https://git.kernel.org/pci/pci/c/ee9eabbe3f0f

[04/04] PCI: mediatek-gen3: Add Airoha EN7581 support
        https://git.kernel.org/pci/pci/c/f6ab898356dd

	Krzysztof

Re: [PATCH v4 0/4] Add Airoha EN7581 PCIe support

[Krzysztof Wilczyński]

Hello,

> > any update about this series? Am I supposed to do something? Thanks in advance.
> 
> No need to do anything, I think Krzysztof will likely pick this up
> when he has a minute.

Everything looks good!  Bjorn, should be able to pull it for 6.12,
hopefully.  Apologies for the delay.

	Krzysztof

Re: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Bjorn Helgaas]

[+cc Jim, Krishna, Vidya, Shashank]

On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
> PCIe controller driver.

> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c

> +#define PCIE_EQ_PRESET_01_REG		0x100
> +#define PCIE_VAL_LN0_DOWNSTREAM		GENMASK(6, 0)
> +#define PCIE_VAL_LN0_UPSTREAM		GENMASK(14, 8)
> +#define PCIE_VAL_LN1_DOWNSTREAM		GENMASK(22, 16)
> +#define PCIE_VAL_LN1_UPSTREAM		GENMASK(30, 24)
> ...

> +static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
> +{
> ...

> +	val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
> +	      FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
> +	      FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
> +	      FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41);
> +	writel_relaxed(val, pcie->base + PCIE_EQ_PRESET_01_REG);

This looks like it might be for the Lane Equalization Control
registers (PCIe r6.0, sec 7.7.3.4)?

I would expect those values (0x47, 0x41) to be related to the platform
design, so maybe not completely determined by the SoC itself?  Jim and
Krishna have been working on DT schema for the equalization values,
which seems like the right place for them:

https://lore.kernel.org/linux-pci/20241018182247.41130-2-james.quinlan@broadcom.com/
https://lore.kernel.org/r/77d3a1a9-c22d-0fd3-5942-91b9a3d74a43@quicinc.com

Maybe that would be applicable here as well?  It would at least be
nice to use a common #define for the Lane Equalization Control
register offset from the capability base.

Although I see that no such #define exists in pci_regs.h, so I guess
there's nothing to do here yet.

The only users of equalization settings I could find so far are:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-tegra194.c?id=v6.11#n832
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-qcom-common.c?id=v6.12-rc1#n11
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-mediatek-gen3.c?id=v6.12-rc1#n909

Bjorn

Re: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Bjorn Helgaas]

On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
> PCIe controller driver.
> ...

> +static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	int err;
> +	u32 val;
> +
> +	/*
> +	 * Wait for the time needed to complete the bulk assert in
> +	 * mtk_pcie_setup for EN7581 SoC.
> +	 */
> +	mdelay(PCIE_EN7581_RESET_TIME_MS);

It looks wrong to me to do the assert and deassert in different
places:

  mtk_pcie_setup
    reset_control_bulk_assert(pcie->phy_resets)        <--
    mtk_pcie_en7581_power_up
      mdelay(PCIE_EN7581_RESET_TIME_MS)
      reset_control_bulk_deassert(pcie->phy_resets)    <--
      mdelay(PCIE_EN7581_RESET_TIME_MS)

That makes the code hard to understand.

> +	err = phy_init(pcie->phy);
> +	if (err) {
> +		dev_err(dev, "failed to initialize PHY\n");
> +		return err;
> +	}
> +
> +	err = phy_power_on(pcie->phy);
> +	if (err) {
> +		dev_err(dev, "failed to power on PHY\n");
> +		goto err_phy_on;
> +	}
> +
> +	err = reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
> +	if (err) {
> +		dev_err(dev, "failed to deassert PHYs\n");
> +		goto err_phy_deassert;
> +	}
> +
> +	/*
> +	 * Wait for the time needed to complete the bulk de-assert above.
> +	 * This time is specific for EN7581 SoC.
> +	 */
> +	mdelay(PCIE_EN7581_RESET_TIME_MS);
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);
> +

> +	err = clk_bulk_prepare(pcie->num_clks, pcie->clks);
> +	if (err) {
> +		dev_err(dev, "failed to prepare clock\n");
> +		goto err_clk_prepare;
> +	}
> +
> +	val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
> +	      FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
> +	      FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
> +	      FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41);
> +	writel_relaxed(val, pcie->base + PCIE_EQ_PRESET_01_REG);
> +
> +	val = PCIE_K_PHYPARAM_QUERY | PCIE_K_QUERY_TIMEOUT |
> +	      FIELD_PREP(PCIE_K_PRESET_TO_USE_16G, 0x80) |
> +	      FIELD_PREP(PCIE_K_PRESET_TO_USE, 0x2) |
> +	      FIELD_PREP(PCIE_K_FINETUNE_MAX, 0xf);
> +	writel_relaxed(val, pcie->base + PCIE_PIPE4_PIE8_REG);

Why is this equalization stuff in the middle between
clk_bulk_prepare() and clk_bulk_enable()?  Is the split an actual
requirement, or could we use clk_bulk_prepare_enable() here, like we
do in mtk_pcie_power_up()?

If the split is required, a comment about why would be helpful.

> +	err = clk_bulk_enable(pcie->num_clks, pcie->clks);
> +	if (err) {
> +		dev_err(dev, "failed to prepare clock\n");
> +		goto err_clk_enable;
> +	}

Per https://lore.kernel.org/r/ZypgYOn7dcYIoW4i@lore-desk,
REG_PCI_CONTROL is asserted/deasserted here by en7581_pci_enable().

Is this where PERST# is asserted?  If so, a comment to that effect
would be helpful.  Where is PERST# deasserted?  Where are the required
delays before deassert done?

Bjorn

Re: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 4118 bytes --]

> On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
> > PCIe controller driver.
> > ...
> 
> > +static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
> > +{
> > +	struct device *dev = pcie->dev;
> > +	int err;
> > +	u32 val;
> > +
> > +	/*
> > +	 * Wait for the time needed to complete the bulk assert in
> > +	 * mtk_pcie_setup for EN7581 SoC.
> > +	 */
> > +	mdelay(PCIE_EN7581_RESET_TIME_MS);

Hi Bjorn,

> 
> It looks wrong to me to do the assert and deassert in different
> places:
> 
>   mtk_pcie_setup
>     reset_control_bulk_assert(pcie->phy_resets)        <--
>     mtk_pcie_en7581_power_up
>       mdelay(PCIE_EN7581_RESET_TIME_MS)
>       reset_control_bulk_deassert(pcie->phy_resets)    <--
>       mdelay(PCIE_EN7581_RESET_TIME_MS)
> 
> That makes the code hard to understand.

The phy reset line was already asserted running reset_control_assert() in
mtk_pcie_setup() and de-asserted running reset_control_deassert() in
mtk_pcie_power_up() before adding EN7581 support. Moreover, EN7581 requires
to run phy_init()/phy_power_on() before de-asserting the phy reset lines.
I guess I can add a comment to make it more clear. Agree?

> 
> > +	err = phy_init(pcie->phy);
> > +	if (err) {
> > +		dev_err(dev, "failed to initialize PHY\n");
> > +		return err;
> > +	}
> > +
> > +	err = phy_power_on(pcie->phy);
> > +	if (err) {
> > +		dev_err(dev, "failed to power on PHY\n");
> > +		goto err_phy_on;
> > +	}
> > +
> > +	err = reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
> > +	if (err) {
> > +		dev_err(dev, "failed to deassert PHYs\n");
> > +		goto err_phy_deassert;
> > +	}
> > +
> > +	/*
> > +	 * Wait for the time needed to complete the bulk de-assert above.
> > +	 * This time is specific for EN7581 SoC.
> > +	 */
> > +	mdelay(PCIE_EN7581_RESET_TIME_MS);
> > +
> > +	pm_runtime_enable(dev);
> > +	pm_runtime_get_sync(dev);
> > +
> 
> > +	err = clk_bulk_prepare(pcie->num_clks, pcie->clks);
> > +	if (err) {
> > +		dev_err(dev, "failed to prepare clock\n");
> > +		goto err_clk_prepare;
> > +	}
> > +
> > +	val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
> > +	      FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
> > +	      FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
> > +	      FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41);
> > +	writel_relaxed(val, pcie->base + PCIE_EQ_PRESET_01_REG);
> > +
> > +	val = PCIE_K_PHYPARAM_QUERY | PCIE_K_QUERY_TIMEOUT |
> > +	      FIELD_PREP(PCIE_K_PRESET_TO_USE_16G, 0x80) |
> > +	      FIELD_PREP(PCIE_K_PRESET_TO_USE, 0x2) |
> > +	      FIELD_PREP(PCIE_K_FINETUNE_MAX, 0xf);
> > +	writel_relaxed(val, pcie->base + PCIE_PIPE4_PIE8_REG);
> 
> Why is this equalization stuff in the middle between
> clk_bulk_prepare() and clk_bulk_enable()?  Is the split an actual
> requirement, or could we use clk_bulk_prepare_enable() here, like we
> do in mtk_pcie_power_up()?

Nope, we can replace clk_bulk_enable() with clk_bulk_prepare_enable() and
remove clk_bulk_prepare() in mtk_pcie_en7581_power_up() since we actually
implements just enable callback for EN7581 in clk-en7523.c.

> 
> If the split is required, a comment about why would be helpful.
> 
> > +	err = clk_bulk_enable(pcie->num_clks, pcie->clks);
> > +	if (err) {
> > +		dev_err(dev, "failed to prepare clock\n");
> > +		goto err_clk_enable;
> > +	}
> 
> Per https://lore.kernel.org/r/ZypgYOn7dcYIoW4i@lore-desk,
> REG_PCI_CONTROL is asserted/deasserted here by en7581_pci_enable().

correct

> 
> Is this where PERST# is asserted?  If so, a comment to that effect
> would be helpful.  Where is PERST# deasserted?  Where are the required
> delays before deassert done?

I can add a comment in en7581_pci_enable() describing the PERST issue for
EN7581. Please note we have a 250ms delay in en7581_pci_enable() after
configuring REG_PCI_CONTROL register.

https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L396

Regards,
Lorenzo

> 
> Bjorn

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Jim Quinlan]

On Tue, Nov 5, 2024 at 4:33 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Jim, Krishna, Vidya, Shashank]
>
> On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
> > PCIe controller driver.
>
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
>
> > +#define PCIE_EQ_PRESET_01_REG                0x100
> > +#define PCIE_VAL_LN0_DOWNSTREAM              GENMASK(6, 0)
> > +#define PCIE_VAL_LN0_UPSTREAM                GENMASK(14, 8)
> > +#define PCIE_VAL_LN1_DOWNSTREAM              GENMASK(22, 16)
> > +#define PCIE_VAL_LN1_UPSTREAM                GENMASK(30, 24)
> > ...
>
> > +static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
> > +{
> > ...
>
> > +     val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
> > +           FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
> > +           FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
> > +           FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41);
> > +     writel_relaxed(val, pcie->base + PCIE_EQ_PRESET_01_REG);

Not sure it is worth the trouble to define fields.  In fact, you are
already combining fields (rec+trans) so why not go further and just
write each lane as a u16?
>
> This looks like it might be for the Lane Equalization Control
> registers (PCIe r6.0, sec 7.7.3.4)?
>
> I would expect those values (0x47, 0x41) to be related to the platform
> design, so maybe not completely determined by the SoC itself?  Jim and
> Krishna have been working on DT schema for the equalization values,
> which seems like the right place for them:
>
> https://lore.kernel.org/linux-pci/20241018182247.41130-2-james.quinlan@broadcom.com/
> https://lore.kernel.org/r/77d3a1a9-c22d-0fd3-5942-91b9a3d74a43@quicinc.com
>
> Maybe that would be applicable here as well?  It would at least be
> nice to use a common #define for the Lane Equalization Control
> register offset from the capability base.

FWIW, these registers are HwInit/RO.  In our (Broadcom) case we have
to write them using an internal  register block that is not visible in
the config space.  In other words, we do not use the cap offset.

Regards,
Jim
Broadcom STB/CM
>
> Although I see that no such #define exists in pci_regs.h, so I guess
> there's nothing to do here yet.
>
> The only users of equalization settings I could find so far are:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-tegra194.c?id=v6.11#n832
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-qcom-common.c?id=v6.12-rc1#n11
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-mediatek-gen3.c?id=v6.12-rc1#n909
>
> Bjorn
>

Re: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Bjorn Helgaas]

On Wed, Nov 06, 2024 at 11:40:28PM +0100, Lorenzo Bianconi wrote:
> > On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> > > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
> > > PCIe controller driver.
> > > ...
> > 
> > > +static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
> > > +{
> > > +	struct device *dev = pcie->dev;
> > > +	int err;
> > > +	u32 val;
> > > +
> > > +	/*
> > > +	 * Wait for the time needed to complete the bulk assert in
> > > +	 * mtk_pcie_setup for EN7581 SoC.
> > > +	 */
> > > +	mdelay(PCIE_EN7581_RESET_TIME_MS);
> 
> > It looks wrong to me to do the assert and deassert in different
> > places:
> > 
> >   mtk_pcie_setup
> >     reset_control_bulk_assert(pcie->phy_resets)        <--
> >     mtk_pcie_en7581_power_up
> >       mdelay(PCIE_EN7581_RESET_TIME_MS)
> >       reset_control_bulk_deassert(pcie->phy_resets)    <--
> >       mdelay(PCIE_EN7581_RESET_TIME_MS)
> > 
> > That makes the code hard to understand.
> 
> The phy reset line was already asserted running reset_control_assert() in
> mtk_pcie_setup() and de-asserted running reset_control_deassert() in
> mtk_pcie_power_up() before adding EN7581 support. Moreover, EN7581 requires
> to run phy_init()/phy_power_on() before de-asserting the phy reset lines.
> I guess I can add a comment to make it more clear. Agree?

I assume the first deassert(phy_resets) in mtk_pcie_setup() is not
paired with anything in this driver.

I think it would be better to pair the other assert/deasserts in the
same functions like the below.  Then it's easy to see the matching.

While looking at this, I noticed that we assert(mac_reset) in
mtk_pcie_setup(), but it's never deasserted for EN7581.

  mtk_pcie_setup
    reset_control_bulk_deassert(phy_resets)
    mtk_pcie_en7581_power_up
      reset_control_bulk_assert(phy_resets)  # move here
      reset_control_assert(mac_reset)        # move here
      mdelay(PCIE_EN7581_RESET_TIME_MS)
      phy_init
      phy_power_on
      reset_control_deassert(mac_reset)      # add; seems missing?
      reset_control_bulk_deassert(phy_resets)
      mdelay(PCIE_EN7581_RESET_TIME_MS)

  mtk_pcie_setup
    reset_control_bulk_deassert(phy_resets)
    mtk_pcie_power_up
      reset_control_bulk_assert(phy_resets)  # move here
      reset_control_assert(mac_reset)        # move here
      reset_control_bulk_deassert(phy_resets)
      phy_init
      phy_power_on
      reset_control_deassert(mac_reset)

> > > +	err = phy_init(pcie->phy);
> > > +	if (err) {
> > > +		dev_err(dev, "failed to initialize PHY\n");
> > > +		return err;
> > > +	}
> > > +
> > > +	err = phy_power_on(pcie->phy);
> > > +	if (err) {
> > > +		dev_err(dev, "failed to power on PHY\n");
> > > +		goto err_phy_on;
> > > +	}
> > > +
> > > +	err = reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
> > > +	if (err) {
> > > +		dev_err(dev, "failed to deassert PHYs\n");
> > > +		goto err_phy_deassert;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Wait for the time needed to complete the bulk de-assert above.
> > > +	 * This time is specific for EN7581 SoC.
> > > +	 */
> > > +	mdelay(PCIE_EN7581_RESET_TIME_MS);
> > > +
> > > +	pm_runtime_enable(dev);
> > > +	pm_runtime_get_sync(dev);
> > > +
> > 
> > > +	err = clk_bulk_prepare(pcie->num_clks, pcie->clks);
> > > +	if (err) {
> > > +		dev_err(dev, "failed to prepare clock\n");
> > > +		goto err_clk_prepare;
> > > +	}
> > > +
> > > +	val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
> > > +	      FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
> > > +	      FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
> > > +	      FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41);
> > > +	writel_relaxed(val, pcie->base + PCIE_EQ_PRESET_01_REG);
> > > +
> > > +	val = PCIE_K_PHYPARAM_QUERY | PCIE_K_QUERY_TIMEOUT |
> > > +	      FIELD_PREP(PCIE_K_PRESET_TO_USE_16G, 0x80) |
> > > +	      FIELD_PREP(PCIE_K_PRESET_TO_USE, 0x2) |
> > > +	      FIELD_PREP(PCIE_K_FINETUNE_MAX, 0xf);
> > > +	writel_relaxed(val, pcie->base + PCIE_PIPE4_PIE8_REG);
> > 
> > Why is this equalization stuff in the middle between
> > clk_bulk_prepare() and clk_bulk_enable()?  Is the split an actual
> > requirement, or could we use clk_bulk_prepare_enable() here, like we
> > do in mtk_pcie_power_up()?
> 
> Nope, we can replace clk_bulk_enable() with clk_bulk_prepare_enable() and
> remove clk_bulk_prepare() in mtk_pcie_en7581_power_up() since we actually
> implements just enable callback for EN7581 in clk-en7523.c.
> 
> > If the split is required, a comment about why would be helpful.
> > 
> > > +	err = clk_bulk_enable(pcie->num_clks, pcie->clks);
> > > +	if (err) {
> > > +		dev_err(dev, "failed to prepare clock\n");
> > > +		goto err_clk_enable;
> > > +	}
> > 
> > Per https://lore.kernel.org/r/ZypgYOn7dcYIoW4i@lore-desk,
> > REG_PCI_CONTROL is asserted/deasserted here by en7581_pci_enable().
> 
> correct
> 
> > Is this where PERST# is asserted?  If so, a comment to that effect
> > would be helpful.  Where is PERST# deasserted?  Where are the required
> > delays before deassert done?
> 
> I can add a comment in en7581_pci_enable() describing the PERST issue for
> EN7581. Please note we have a 250ms delay in en7581_pci_enable() after
> configuring REG_PCI_CONTROL register.
> 
> https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L396

Does that 250ms delay correspond to a PCIe mandatory delay, e.g.,
something like PCIE_T_PVPERL_MS?  I think it would be nice to have the
required PCI delays in this driver if possible so it's easy to verify
that they are all covered.

Bjorn

Re: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Bjorn Helgaas]

On Wed, Nov 06, 2024 at 06:00:08PM -0500, Jim Quinlan wrote:
> On Tue, Nov 5, 2024 at 4:33 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> > > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
> > > PCIe controller driver.
> >
> > > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> >
> > > +#define PCIE_EQ_PRESET_01_REG                0x100
> > > +#define PCIE_VAL_LN0_DOWNSTREAM              GENMASK(6, 0)
> > > +#define PCIE_VAL_LN0_UPSTREAM                GENMASK(14, 8)
> > > +#define PCIE_VAL_LN1_DOWNSTREAM              GENMASK(22, 16)
> > > +#define PCIE_VAL_LN1_UPSTREAM                GENMASK(30, 24)
> > > ...
> >
> > > +static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
> > > +{
> > > ...
> >
> > > +     val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
> > > +           FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
> > > +           FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
> > > +           FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41);
> > > +     writel_relaxed(val, pcie->base + PCIE_EQ_PRESET_01_REG);
> 
> Not sure it is worth the trouble to define fields.  In fact, you are
> already combining fields (rec+trans) so why not go further and just
> write each lane as a u16?
> >
> > This looks like it might be for the Lane Equalization Control
> > registers (PCIe r6.0, sec 7.7.3.4)?
> >
> > I would expect those values (0x47, 0x41) to be related to the platform
> > design, so maybe not completely determined by the SoC itself?  Jim and
> > Krishna have been working on DT schema for the equalization values,
> > which seems like the right place for them:
> >
> > https://lore.kernel.org/linux-pci/20241018182247.41130-2-james.quinlan@broadcom.com/
> > https://lore.kernel.org/r/77d3a1a9-c22d-0fd3-5942-91b9a3d74a43@quicinc.com
> >
> > Maybe that would be applicable here as well?  It would at least be
> > nice to use a common #define for the Lane Equalization Control
> > register offset from the capability base.
> 
> FWIW, these registers are HwInit/RO.  In our (Broadcom) case we have
> to write them using an internal register block that is not visible in
> the config space.  In other words, we do not use the cap offset.

Good point.  It looks like they're a mix of HwInit/RsvdP and
Hwinit/RO.  RsvdP is for writes, so I guess the config space registers
must be write-once and subsequently read-only until reset.  In any
case, mtk is using an internal register block as well, so a cap offset
wouldn't be useful.

Maybe it would still be worthwhile to define the fields themselves in
pci_regs.h so we can someday have common code to parse the DT
properties and assemble them.  Although I suppose there's no
requirement that the registers in the internal block even be laid out
the same as the config space register.

> > Although I see that no such #define exists in pci_regs.h, so I guess
> > there's nothing to do here yet.
> >
> > The only users of equalization settings I could find so far are:
> >
> >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-tegra194.c?id=v6.11#n832
> >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-qcom-common.c?id=v6.12-rc1#n11
> >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-mediatek-gen3.c?id=v6.12-rc1#n909
> >
> > Bjorn
> >

Re: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 6387 bytes --]

> On Wed, Nov 06, 2024 at 11:40:28PM +0100, Lorenzo Bianconi wrote:
> > > On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> > > > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
> > > > PCIe controller driver.
> > > > ...
> > > 
> > > > +static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
> > > > +{
> > > > +	struct device *dev = pcie->dev;
> > > > +	int err;
> > > > +	u32 val;
> > > > +
> > > > +	/*
> > > > +	 * Wait for the time needed to complete the bulk assert in
> > > > +	 * mtk_pcie_setup for EN7581 SoC.
> > > > +	 */
> > > > +	mdelay(PCIE_EN7581_RESET_TIME_MS);
> > 
> > > It looks wrong to me to do the assert and deassert in different
> > > places:
> > > 
> > >   mtk_pcie_setup
> > >     reset_control_bulk_assert(pcie->phy_resets)        <--
> > >     mtk_pcie_en7581_power_up
> > >       mdelay(PCIE_EN7581_RESET_TIME_MS)
> > >       reset_control_bulk_deassert(pcie->phy_resets)    <--
> > >       mdelay(PCIE_EN7581_RESET_TIME_MS)
> > > 
> > > That makes the code hard to understand.
> > 
> > The phy reset line was already asserted running reset_control_assert() in
> > mtk_pcie_setup() and de-asserted running reset_control_deassert() in
> > mtk_pcie_power_up() before adding EN7581 support. Moreover, EN7581 requires
> > to run phy_init()/phy_power_on() before de-asserting the phy reset lines.
> > I guess I can add a comment to make it more clear. Agree?
> 
> I assume the first deassert(phy_resets) in mtk_pcie_setup() is not
> paired with anything in this driver.

correct

> 
> I think it would be better to pair the other assert/deasserts in the
> same functions like the below.  Then it's easy to see the matching.

ack, I will post a fix for it

> 
> While looking at this, I noticed that we assert(mac_reset) in
> mtk_pcie_setup(), but it's never deasserted for EN7581.

ack, I will post a fix for it

> 
>   mtk_pcie_setup
>     reset_control_bulk_deassert(phy_resets)
>     mtk_pcie_en7581_power_up
>       reset_control_bulk_assert(phy_resets)  # move here
>       reset_control_assert(mac_reset)        # move here
>       mdelay(PCIE_EN7581_RESET_TIME_MS)
>       phy_init
>       phy_power_on
>       reset_control_deassert(mac_reset)      # add; seems missing?
>       reset_control_bulk_deassert(phy_resets)
>       mdelay(PCIE_EN7581_RESET_TIME_MS)
> 
>   mtk_pcie_setup
>     reset_control_bulk_deassert(phy_resets)
>     mtk_pcie_power_up
>       reset_control_bulk_assert(phy_resets)  # move here
>       reset_control_assert(mac_reset)        # move here
>       reset_control_bulk_deassert(phy_resets)
>       phy_init
>       phy_power_on
>       reset_control_deassert(mac_reset)
> 
> > > > +	err = phy_init(pcie->phy);
> > > > +	if (err) {
> > > > +		dev_err(dev, "failed to initialize PHY\n");
> > > > +		return err;
> > > > +	}
> > > > +
> > > > +	err = phy_power_on(pcie->phy);
> > > > +	if (err) {
> > > > +		dev_err(dev, "failed to power on PHY\n");
> > > > +		goto err_phy_on;
> > > > +	}
> > > > +
> > > > +	err = reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
> > > > +	if (err) {
> > > > +		dev_err(dev, "failed to deassert PHYs\n");
> > > > +		goto err_phy_deassert;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Wait for the time needed to complete the bulk de-assert above.
> > > > +	 * This time is specific for EN7581 SoC.
> > > > +	 */
> > > > +	mdelay(PCIE_EN7581_RESET_TIME_MS);
> > > > +
> > > > +	pm_runtime_enable(dev);
> > > > +	pm_runtime_get_sync(dev);
> > > > +
> > > 
> > > > +	err = clk_bulk_prepare(pcie->num_clks, pcie->clks);
> > > > +	if (err) {
> > > > +		dev_err(dev, "failed to prepare clock\n");
> > > > +		goto err_clk_prepare;
> > > > +	}
> > > > +
> > > > +	val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
> > > > +	      FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
> > > > +	      FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
> > > > +	      FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41);
> > > > +	writel_relaxed(val, pcie->base + PCIE_EQ_PRESET_01_REG);
> > > > +
> > > > +	val = PCIE_K_PHYPARAM_QUERY | PCIE_K_QUERY_TIMEOUT |
> > > > +	      FIELD_PREP(PCIE_K_PRESET_TO_USE_16G, 0x80) |
> > > > +	      FIELD_PREP(PCIE_K_PRESET_TO_USE, 0x2) |
> > > > +	      FIELD_PREP(PCIE_K_FINETUNE_MAX, 0xf);
> > > > +	writel_relaxed(val, pcie->base + PCIE_PIPE4_PIE8_REG);
> > > 
> > > Why is this equalization stuff in the middle between
> > > clk_bulk_prepare() and clk_bulk_enable()?  Is the split an actual
> > > requirement, or could we use clk_bulk_prepare_enable() here, like we
> > > do in mtk_pcie_power_up()?
> > 
> > Nope, we can replace clk_bulk_enable() with clk_bulk_prepare_enable() and
> > remove clk_bulk_prepare() in mtk_pcie_en7581_power_up() since we actually
> > implements just enable callback for EN7581 in clk-en7523.c.
> > 
> > > If the split is required, a comment about why would be helpful.
> > > 
> > > > +	err = clk_bulk_enable(pcie->num_clks, pcie->clks);
> > > > +	if (err) {
> > > > +		dev_err(dev, "failed to prepare clock\n");
> > > > +		goto err_clk_enable;
> > > > +	}
> > > 
> > > Per https://lore.kernel.org/r/ZypgYOn7dcYIoW4i@lore-desk,
> > > REG_PCI_CONTROL is asserted/deasserted here by en7581_pci_enable().
> > 
> > correct
> > 
> > > Is this where PERST# is asserted?  If so, a comment to that effect
> > > would be helpful.  Where is PERST# deasserted?  Where are the required
> > > delays before deassert done?
> > 
> > I can add a comment in en7581_pci_enable() describing the PERST issue for
> > EN7581. Please note we have a 250ms delay in en7581_pci_enable() after
> > configuring REG_PCI_CONTROL register.
> > 
> > https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L396
> 
> Does that 250ms delay correspond to a PCIe mandatory delay, e.g.,
> something like PCIE_T_PVPERL_MS?  I think it would be nice to have the
> required PCI delays in this driver if possible so it's easy to verify
> that they are all covered.

IIRC I just used the delay value used in the vendor sdk. I do not have a strong
opinion about it but I guess if we move it in the pcie-mediatek-gen3 driver, we
will need to add it in each driver where this clock is used. What do you think?

Regards,
Lorenzo

> 
> Bjorn

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Bjorn Helgaas]

On Thu, Nov 07, 2024 at 08:39:43AM +0100, Lorenzo Bianconi wrote:
> > On Wed, Nov 06, 2024 at 11:40:28PM +0100, Lorenzo Bianconi wrote:
> > > > On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> > > > > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
> > > > > PCIe controller driver.
> > > > > ...

> > > > Is this where PERST# is asserted?  If so, a comment to that effect
> > > > would be helpful.  Where is PERST# deasserted?  Where are the required
> > > > delays before deassert done?
> > > 
> > > I can add a comment in en7581_pci_enable() describing the PERST issue for
> > > EN7581. Please note we have a 250ms delay in en7581_pci_enable() after
> > > configuring REG_PCI_CONTROL register.
> > > 
> > > https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L396
> > 
> > Does that 250ms delay correspond to a PCIe mandatory delay, e.g.,
> > something like PCIE_T_PVPERL_MS?  I think it would be nice to have the
> > required PCI delays in this driver if possible so it's easy to verify
> > that they are all covered.
> 
> IIRC I just used the delay value used in the vendor sdk. I do not
> have a strong opinion about it but I guess if we move it in the
> pcie-mediatek-gen3 driver, we will need to add it in each driver
> where this clock is used. What do you think?

I don't know what the 250ms delay is for.  If it is for a required PCI
delay, we should use the relevant standard #define for it, and it
should be in the PCI controller driver.  Otherwise it's impossible to
verify that all the drivers are doing the correct delays.

I don't know what other drivers are using that clock.  Are you
suggesting that it may be used in non-PCI situations where the
required delay might be different?  If another user requires 250ms,
but PCI requires only 100ms, I think it would be worth having separate
delays in each user so PCI wouldn't have to pay that extra 150ms.

Bjorn

Re: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2295 bytes --]

On Nov 07, Bjorn Helgaas wrote:
> On Thu, Nov 07, 2024 at 08:39:43AM +0100, Lorenzo Bianconi wrote:
> > > On Wed, Nov 06, 2024 at 11:40:28PM +0100, Lorenzo Bianconi wrote:
> > > > > On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> > > > > > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
> > > > > > PCIe controller driver.
> > > > > > ...
> 
> > > > > Is this where PERST# is asserted?  If so, a comment to that effect
> > > > > would be helpful.  Where is PERST# deasserted?  Where are the required
> > > > > delays before deassert done?
> > > > 
> > > > I can add a comment in en7581_pci_enable() describing the PERST issue for
> > > > EN7581. Please note we have a 250ms delay in en7581_pci_enable() after
> > > > configuring REG_PCI_CONTROL register.
> > > > 
> > > > https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L396
> > > 
> > > Does that 250ms delay correspond to a PCIe mandatory delay, e.g.,
> > > something like PCIE_T_PVPERL_MS?  I think it would be nice to have the
> > > required PCI delays in this driver if possible so it's easy to verify
> > > that they are all covered.
> > 
> > IIRC I just used the delay value used in the vendor sdk. I do not
> > have a strong opinion about it but I guess if we move it in the
> > pcie-mediatek-gen3 driver, we will need to add it in each driver
> > where this clock is used. What do you think?
> 
> I don't know what the 250ms delay is for.  If it is for a required PCI
> delay, we should use the relevant standard #define for it, and it
> should be in the PCI controller driver.  Otherwise it's impossible to
> verify that all the drivers are doing the correct delays.

ack, fine to me. Do you prefer to keep 250ms after clk_bulk_prepare_enable()
in mtk_pcie_en7581_power_up() or just use PCIE_T_PVPERL_MS (100)?
I can check if 100ms works properly.

Regards,
Lorenzo

> 
> I don't know what other drivers are using that clock.  Are you
> suggesting that it may be used in non-PCI situations where the
> required delay might be different?  If another user requires 250ms,
> but PCI requires only 100ms, I think it would be worth having separate
> delays in each user so PCI wouldn't have to pay that extra 150ms.
> 
> Bjorn

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Bjorn Helgaas]

On Thu, Nov 07, 2024 at 05:21:45PM +0100, Lorenzo Bianconi wrote:
> On Nov 07, Bjorn Helgaas wrote:
> > On Thu, Nov 07, 2024 at 08:39:43AM +0100, Lorenzo Bianconi wrote:
> > > > On Wed, Nov 06, 2024 at 11:40:28PM +0100, Lorenzo Bianconi wrote:
> > > > > > On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> > > > > > > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
> > > > > > > PCIe controller driver.
> > > > > > > ...
> > 
> > > > > > Is this where PERST# is asserted?  If so, a comment to that effect
> > > > > > would be helpful.  Where is PERST# deasserted?  Where are the required
> > > > > > delays before deassert done?
> > > > > 
> > > > > I can add a comment in en7581_pci_enable() describing the PERST issue for
> > > > > EN7581. Please note we have a 250ms delay in en7581_pci_enable() after
> > > > > configuring REG_PCI_CONTROL register.
> > > > > 
> > > > > https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L396
> > > > 
> > > > Does that 250ms delay correspond to a PCIe mandatory delay, e.g.,
> > > > something like PCIE_T_PVPERL_MS?  I think it would be nice to have the
> > > > required PCI delays in this driver if possible so it's easy to verify
> > > > that they are all covered.
> > > 
> > > IIRC I just used the delay value used in the vendor sdk. I do not
> > > have a strong opinion about it but I guess if we move it in the
> > > pcie-mediatek-gen3 driver, we will need to add it in each driver
> > > where this clock is used. What do you think?
> > 
> > I don't know what the 250ms delay is for.  If it is for a required PCI
> > delay, we should use the relevant standard #define for it, and it
> > should be in the PCI controller driver.  Otherwise it's impossible to
> > verify that all the drivers are doing the correct delays.
> 
> ack, fine to me. Do you prefer to keep 250ms after clk_bulk_prepare_enable()
> in mtk_pcie_en7581_power_up() or just use PCIE_T_PVPERL_MS (100)?
> I can check if 100ms works properly.

It's not clear to me where the relevant events are for these chips.

Do you have access to the PCIe CEM spec?  The diagram in r6.0, sec
2.2.1, is helpful.  It shows the required timings for Power Stable,
REFCLK Stable, PERST# deassert, etc.

Per sec 2.11.2, PERST# must be asserted for at least 100us (T_PERST),
PERST# must be asserted for at least 100ms after Power Stable
(T_PVPERL), and PERST# must be asserted for at least 100us after
REFCLK Stable.

It would be helpful if we could tell by reading the source where some
of these critical events happen, and that the relevant delays are
there.  For example, if PERST# is asserted/deasserted by
"clk_enable()" or similar, it's not at all obvious from the code, so
we should have a comment to that effect.

Bjorn

Re: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3286 bytes --]

> On Thu, Nov 07, 2024 at 05:21:45PM +0100, Lorenzo Bianconi wrote:
> > On Nov 07, Bjorn Helgaas wrote:
> > > On Thu, Nov 07, 2024 at 08:39:43AM +0100, Lorenzo Bianconi wrote:
> > > > > On Wed, Nov 06, 2024 at 11:40:28PM +0100, Lorenzo Bianconi wrote:
> > > > > > > On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> > > > > > > > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
> > > > > > > > PCIe controller driver.
> > > > > > > > ...
> > > 
> > > > > > > Is this where PERST# is asserted?  If so, a comment to that effect
> > > > > > > would be helpful.  Where is PERST# deasserted?  Where are the required
> > > > > > > delays before deassert done?
> > > > > > 
> > > > > > I can add a comment in en7581_pci_enable() describing the PERST issue for
> > > > > > EN7581. Please note we have a 250ms delay in en7581_pci_enable() after
> > > > > > configuring REG_PCI_CONTROL register.
> > > > > > 
> > > > > > https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L396
> > > > > 
> > > > > Does that 250ms delay correspond to a PCIe mandatory delay, e.g.,
> > > > > something like PCIE_T_PVPERL_MS?  I think it would be nice to have the
> > > > > required PCI delays in this driver if possible so it's easy to verify
> > > > > that they are all covered.
> > > > 
> > > > IIRC I just used the delay value used in the vendor sdk. I do not
> > > > have a strong opinion about it but I guess if we move it in the
> > > > pcie-mediatek-gen3 driver, we will need to add it in each driver
> > > > where this clock is used. What do you think?
> > > 
> > > I don't know what the 250ms delay is for.  If it is for a required PCI
> > > delay, we should use the relevant standard #define for it, and it
> > > should be in the PCI controller driver.  Otherwise it's impossible to
> > > verify that all the drivers are doing the correct delays.
> > 
> > ack, fine to me. Do you prefer to keep 250ms after clk_bulk_prepare_enable()
> > in mtk_pcie_en7581_power_up() or just use PCIE_T_PVPERL_MS (100)?
> > I can check if 100ms works properly.
> 
> It's not clear to me where the relevant events are for these chips.
> 
> Do you have access to the PCIe CEM spec?  The diagram in r6.0, sec
> 2.2.1, is helpful.  It shows the required timings for Power Stable,
> REFCLK Stable, PERST# deassert, etc.
> 
> Per sec 2.11.2, PERST# must be asserted for at least 100us (T_PERST),
> PERST# must be asserted for at least 100ms after Power Stable
> (T_PVPERL), and PERST# must be asserted for at least 100us after
> REFCLK Stable.
> 
> It would be helpful if we could tell by reading the source where some
> of these critical events happen, and that the relevant delays are
> there.  For example, if PERST# is asserted/deasserted by
> "clk_enable()" or similar, it's not at all obvious from the code, so
> we should have a comment to that effect.

I reviewed the vendor sdk and it just do something like in clk_enable():

	...
	val = readl(0x88);
	writel(val | BIT(16) | BIT(29) | BIT(26), 0x88);
	/*wait link up*/
	mdelay(1000);
	...

@Hui.Ma: is it fine use msleep(100) (so PCIE_T_PVPERL_MS) instead of msleep(1000)
(so PCIE_LINK_RETRAIN_TIMEOUT_MS)?

Regards,
Lorenzo

> 
> Bjorn

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

回复: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Hui Ma (马慧)]

> On Thu, Nov 07, 2024 at 05:21:45PM +0100, Lorenzo Bianconi wrote:
> > On Nov 07, Bjorn Helgaas wrote:
> > > On Thu, Nov 07, 2024 at 08:39:43AM +0100, Lorenzo Bianconi wrote:
> > > > > On Wed, Nov 06, 2024 at 11:40:28PM +0100, Lorenzo Bianconi wrote:
> > > > > > > On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> > > > > > > > Introduce support for Airoha EN7581 PCIe controller to 
> > > > > > > > mediatek-gen3 PCIe controller driver.
> > > > > > > > ...
> > > 
> > > > > > > Is this where PERST# is asserted?  If so, a comment to 
> > > > > > > that effect would be helpful.  Where is PERST# deasserted?  
> > > > > > > Where are the required delays before deassert done?
> > > > > > 
> > > > > > I can add a comment in en7581_pci_enable() describing the 
> > > > > > PERST issue for EN7581. Please note we have a 250ms delay in 
> > > > > > en7581_pci_enable() after configuring REG_PCI_CONTROL register.
> > > > > > 
> > > > > > https://github.com/torvalds/linux/blob/master/drivers/clk/cl
> > > > > > k-en7523.c#L396
> > > > > 
> > > > > Does that 250ms delay correspond to a PCIe mandatory delay, 
> > > > > e.g., something like PCIE_T_PVPERL_MS?  I think it would be 
> > > > > nice to have the required PCI delays in this driver if 
> > > > > possible so it's easy to verify that they are all covered.
> > > > 
> > > > IIRC I just used the delay value used in the vendor sdk. I do 
> > > > not have a strong opinion about it but I guess if we move it in 
> > > > the
> > > > pcie-mediatek-gen3 driver, we will need to add it in each driver 
> > > > where this clock is used. What do you think?
> > > 
> > > I don't know what the 250ms delay is for.  If it is for a required 
> > > PCI delay, we should use the relevant standard #define for it, and 
> > > it should be in the PCI controller driver.  Otherwise it's 
> > > impossible to verify that all the drivers are doing the correct delays.
> > 
> > ack, fine to me. Do you prefer to keep 250ms after 
> > clk_bulk_prepare_enable() in mtk_pcie_en7581_power_up() or just use PCIE_T_PVPERL_MS (100)?
> > I can check if 100ms works properly.
> 
> It's not clear to me where the relevant events are for these chips.
> 
> Do you have access to the PCIe CEM spec?  The diagram in r6.0, sec 
> 2.2.1, is helpful.  It shows the required timings for Power Stable, 
> REFCLK Stable, PERST# deassert, etc.
> 
> Per sec 2.11.2, PERST# must be asserted for at least 100us (T_PERST), 
> PERST# must be asserted for at least 100ms after Power Stable 
> (T_PVPERL), and PERST# must be asserted for at least 100us after 
> REFCLK Stable.
> 
> It would be helpful if we could tell by reading the source where some 
> of these critical events happen, and that the relevant delays are 
> there.  For example, if PERST# is asserted/deasserted by 
> "clk_enable()" or similar, it's not at all obvious from the code, so 
> we should have a comment to that effect.

>I reviewed the vendor sdk and it just do something like in clk_enable():
>
>	...
>	val = readl(0x88);
>	writel(val | BIT(16) | BIT(29) | BIT(26), 0x88);
>	/*wait link up*/
>	mdelay(1000);
>	...
>
>@Hui.Ma: is it fine use msleep(100) (so PCIE_T_PVPERL_MS) instead of msleep(1000) (so PCIE_LINK_RETRAIN_TIMEOUT_MS)?
Hi Lorenzo，
	I think msleep(1000) will be safer,because some device won't link up with msleep(100).
Regards,
Hui
>
>Regards,
>Lorenzo

> 
> Bjorn

Re: 回复: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Bjorn Helgaas]

On Fri, Nov 08, 2024 at 01:23:35AM +0000, Hui Ma (马慧) wrote:
> > On Thu, Nov 07, 2024 at 05:21:45PM +0100, Lorenzo Bianconi wrote:
> > > On Nov 07, Bjorn Helgaas wrote:
> > > > On Thu, Nov 07, 2024 at 08:39:43AM +0100, Lorenzo Bianconi wrote:
> > > > > > On Wed, Nov 06, 2024 at 11:40:28PM +0100, Lorenzo Bianconi wrote:
> > > > > > > > On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> > > > > > > > > Introduce support for Airoha EN7581 PCIe controller to 
> > > > > > > > > mediatek-gen3 PCIe controller driver.
> > > > > > > > > ...
> > > > 
> > > > > > > > Is this where PERST# is asserted?  If so, a comment to 
> > > > > > > > that effect would be helpful.  Where is PERST# deasserted?  
> > > > > > > > Where are the required delays before deassert done?
> > > > > > > 
> > > > > > > I can add a comment in en7581_pci_enable() describing the 
> > > > > > > PERST issue for EN7581. Please note we have a 250ms delay in 
> > > > > > > en7581_pci_enable() after configuring REG_PCI_CONTROL register.
> > > > > > > 
> > > > > > > https://github.com/torvalds/linux/blob/master/drivers/clk/cl
> > > > > > > k-en7523.c#L396
> > > > > > 
> > > > > > Does that 250ms delay correspond to a PCIe mandatory delay, 
> > > > > > e.g., something like PCIE_T_PVPERL_MS?  I think it would be 
> > > > > > nice to have the required PCI delays in this driver if 
> > > > > > possible so it's easy to verify that they are all covered.
> > > > > 
> > > > > IIRC I just used the delay value used in the vendor sdk. I
> > > > > do not have a strong opinion about it but I guess if we move
> > > > > it in the pcie-mediatek-gen3 driver, we will need to add it
> > > > > in each driver where this clock is used. What do you think?
> > > > 
> > > > I don't know what the 250ms delay is for.  If it is for a required 
> > > > PCI delay, we should use the relevant standard #define for it, and 
> > > > it should be in the PCI controller driver.  Otherwise it's 
> > > > impossible to verify that all the drivers are doing the correct delays.
> > > 
> > > ack, fine to me. Do you prefer to keep 250ms after 
> > > clk_bulk_prepare_enable() in mtk_pcie_en7581_power_up() or just use PCIE_T_PVPERL_MS (100)?
> > > I can check if 100ms works properly.
> > 
> > It's not clear to me where the relevant events are for these chips.
> > 
> > Do you have access to the PCIe CEM spec?  The diagram in r6.0, sec 
> > 2.2.1, is helpful.  It shows the required timings for Power Stable, 
> > REFCLK Stable, PERST# deassert, etc.
> > 
> > Per sec 2.11.2, PERST# must be asserted for at least 100us (T_PERST), 
> > PERST# must be asserted for at least 100ms after Power Stable 
> > (T_PVPERL), and PERST# must be asserted for at least 100us after 
> > REFCLK Stable.
> > 
> > It would be helpful if we could tell by reading the source where some 
> > of these critical events happen, and that the relevant delays are 
> > there.  For example, if PERST# is asserted/deasserted by 
> > "clk_enable()" or similar, it's not at all obvious from the code, so 
> > we should have a comment to that effect.
> 
> >I reviewed the vendor sdk and it just do something like in clk_enable():
> >
> >	...
> >	val = readl(0x88);
> >	writel(val | BIT(16) | BIT(29) | BIT(26), 0x88);
> >	/*wait link up*/
> >	mdelay(1000);
> >	...
> >
> >@Hui.Ma: is it fine use msleep(100) (so PCIE_T_PVPERL_MS) instead
> >of msleep(1000) (so PCIE_LINK_RETRAIN_TIMEOUT_MS)?
>
> 	I think msleep(1000) will be safer, because some device won't
> 	link up with msleep(100).

Do you have details about this?  I guess it only hurts mediatek, but
increasing the minimum time to bring up a PCI hierarchy by almost an
entire second is a pretty big deal.

If this delay corresponds to the required T_PVPERL delay and 100ms
isn't enough for some endpoints, those endpoints should fail with many
host controllers, not just mediatek, so I would suspect the mediatek
controller or a certain platform, not the endpoint itself.

If this corresponds to T_PVPERL and mediatek needs longer, I would
document that by using "PCIE_T_PVPERL_MS * 10" and adding a comment
about why (affected platform/device, hardware erratum, etc).

Bottom line, I don't really care what the value is, but I *would* like
to be able to read pcie-mediatek-gen3.c and see the point where PCI
power is stable, a delay of at least T_PVPERL, and where PERST# is
deasserted because that's the main timing requirement on software.

Bjorn

Re: 回复: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 5144 bytes --]

> On Fri, Nov 08, 2024 at 01:23:35AM +0000, Hui Ma (马慧) wrote:
> > > On Thu, Nov 07, 2024 at 05:21:45PM +0100, Lorenzo Bianconi wrote:
> > > > On Nov 07, Bjorn Helgaas wrote:
> > > > > On Thu, Nov 07, 2024 at 08:39:43AM +0100, Lorenzo Bianconi wrote:
> > > > > > > On Wed, Nov 06, 2024 at 11:40:28PM +0100, Lorenzo Bianconi wrote:
> > > > > > > > > On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> > > > > > > > > > Introduce support for Airoha EN7581 PCIe controller to 
> > > > > > > > > > mediatek-gen3 PCIe controller driver.
> > > > > > > > > > ...
> > > > > 
> > > > > > > > > Is this where PERST# is asserted?  If so, a comment to 
> > > > > > > > > that effect would be helpful.  Where is PERST# deasserted?  
> > > > > > > > > Where are the required delays before deassert done?
> > > > > > > > 
> > > > > > > > I can add a comment in en7581_pci_enable() describing the 
> > > > > > > > PERST issue for EN7581. Please note we have a 250ms delay in 
> > > > > > > > en7581_pci_enable() after configuring REG_PCI_CONTROL register.
> > > > > > > > 
> > > > > > > > https://github.com/torvalds/linux/blob/master/drivers/clk/cl
> > > > > > > > k-en7523.c#L396
> > > > > > > 
> > > > > > > Does that 250ms delay correspond to a PCIe mandatory delay, 
> > > > > > > e.g., something like PCIE_T_PVPERL_MS?  I think it would be 
> > > > > > > nice to have the required PCI delays in this driver if 
> > > > > > > possible so it's easy to verify that they are all covered.
> > > > > > 
> > > > > > IIRC I just used the delay value used in the vendor sdk. I
> > > > > > do not have a strong opinion about it but I guess if we move
> > > > > > it in the pcie-mediatek-gen3 driver, we will need to add it
> > > > > > in each driver where this clock is used. What do you think?
> > > > > 
> > > > > I don't know what the 250ms delay is for.  If it is for a required 
> > > > > PCI delay, we should use the relevant standard #define for it, and 
> > > > > it should be in the PCI controller driver.  Otherwise it's 
> > > > > impossible to verify that all the drivers are doing the correct delays.
> > > > 
> > > > ack, fine to me. Do you prefer to keep 250ms after 
> > > > clk_bulk_prepare_enable() in mtk_pcie_en7581_power_up() or just use PCIE_T_PVPERL_MS (100)?
> > > > I can check if 100ms works properly.
> > > 
> > > It's not clear to me where the relevant events are for these chips.
> > > 
> > > Do you have access to the PCIe CEM spec?  The diagram in r6.0, sec 
> > > 2.2.1, is helpful.  It shows the required timings for Power Stable, 
> > > REFCLK Stable, PERST# deassert, etc.
> > > 
> > > Per sec 2.11.2, PERST# must be asserted for at least 100us (T_PERST), 
> > > PERST# must be asserted for at least 100ms after Power Stable 
> > > (T_PVPERL), and PERST# must be asserted for at least 100us after 
> > > REFCLK Stable.
> > > 
> > > It would be helpful if we could tell by reading the source where some 
> > > of these critical events happen, and that the relevant delays are 
> > > there.  For example, if PERST# is asserted/deasserted by 
> > > "clk_enable()" or similar, it's not at all obvious from the code, so 
> > > we should have a comment to that effect.
> > 
> > >I reviewed the vendor sdk and it just do something like in clk_enable():
> > >
> > >	...
> > >	val = readl(0x88);
> > >	writel(val | BIT(16) | BIT(29) | BIT(26), 0x88);
> > >	/*wait link up*/
> > >	mdelay(1000);
> > >	...
> > >
> > >@Hui.Ma: is it fine use msleep(100) (so PCIE_T_PVPERL_MS) instead
> > >of msleep(1000) (so PCIE_LINK_RETRAIN_TIMEOUT_MS)?
> >
> > 	I think msleep(1000) will be safer, because some device won't
> > 	link up with msleep(100).
> 
> Do you have details about this?  I guess it only hurts mediatek, but
> increasing the minimum time to bring up a PCI hierarchy by almost an
> entire second is a pretty big deal.
> 
> If this delay corresponds to the required T_PVPERL delay and 100ms
> isn't enough for some endpoints, those endpoints should fail with many
> host controllers, not just mediatek, so I would suspect the mediatek
> controller or a certain platform, not the endpoint itself.
> 
> If this corresponds to T_PVPERL and mediatek needs longer, I would
> document that by using "PCIE_T_PVPERL_MS * 10" and adding a comment
> about why (affected platform/device, hardware erratum, etc).
> 
> Bottom line, I don't really care what the value is, but I *would* like
> to be able to read pcie-mediatek-gen3.c and see the point where PCI
> power is stable, a delay of at least T_PVPERL, and where PERST# is
> deasserted because that's the main timing requirement on software.

I run some testes using 100ms delay (PCIE_T_PVPERL_MS) after
clk_bulk_prepare_enable() in mtk_pcie_en7581_power_up() and it works fine for
me (I tested with a MT7915 WiFi PCIe nic connected to the PCIe sock).
Moreover, we already poll PCIE_LINK_STATUS_REG register to check the link
status in mtk_pcie_startup_port(), right? I guess we can proceed with 100ms
delay in mtk_pcie_en7581_power_up().

Regards,
Lorenzo

> 
> Bjorn

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

回复: 回复: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Hui Ma (马慧)]

> On Fri, Nov 08, 2024 at 01:23:35AM +0000, Hui Ma (马慧) wrote:
> > > On Thu, Nov 07, 2024 at 05:21:45PM +0100, Lorenzo Bianconi wrote:
> > > > On Nov 07, Bjorn Helgaas wrote:
> > > > > On Thu, Nov 07, 2024 at 08:39:43AM +0100, Lorenzo Bianconi wrote:
> > > > > > > On Wed, Nov 06, 2024 at 11:40:28PM +0100, Lorenzo Bianconi wrote:
> > > > > > > > > On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> > > > > > > > > > Introduce support for Airoha EN7581 PCIe controller 
> > > > > > > > > > to
> > > > > > > > > > mediatek-gen3 PCIe controller driver.
> > > > > > > > > > ...
> > > > > 
> > > > > > > > > Is this where PERST# is asserted?  If so, a comment to 
> > > > > > > > > that effect would be helpful.  Where is PERST# deasserted?
> > > > > > > > > Where are the required delays before deassert done?
> > > > > > > > 
> > > > > > > > I can add a comment in en7581_pci_enable() describing 
> > > > > > > > the PERST issue for EN7581. Please note we have a 250ms 
> > > > > > > > delay in
> > > > > > > > en7581_pci_enable() after configuring REG_PCI_CONTROL register.
> > > > > > > > 
> > > > > > > > https://github.com/torvalds/linux/blob/master/drivers/cl
> > > > > > > > k/cl
> > > > > > > > k-en7523.c#L396
> > > > > > > 
> > > > > > > Does that 250ms delay correspond to a PCIe mandatory 
> > > > > > > delay, e.g., something like PCIE_T_PVPERL_MS?  I think it 
> > > > > > > would be nice to have the required PCI delays in this 
> > > > > > > driver if possible so it's easy to verify that they are all covered.
> > > > > > 
> > > > > > IIRC I just used the delay value used in the vendor sdk. I 
> > > > > > do not have a strong opinion about it but I guess if we move 
> > > > > > it in the pcie-mediatek-gen3 driver, we will need to add it 
> > > > > > in each driver where this clock is used. What do you think?
> > > > > 
> > > > > I don't know what the 250ms delay is for.  If it is for a 
> > > > > required PCI delay, we should use the relevant standard 
> > > > > #define for it, and it should be in the PCI controller driver.  
> > > > > Otherwise it's impossible to verify that all the drivers are doing the correct delays.
> > > > 
> > > > ack, fine to me. Do you prefer to keep 250ms after
> > > > clk_bulk_prepare_enable() in mtk_pcie_en7581_power_up() or just use PCIE_T_PVPERL_MS (100)?
> > > > I can check if 100ms works properly.
> > > 
> > > It's not clear to me where the relevant events are for these chips.
> > > 
> > > Do you have access to the PCIe CEM spec?  The diagram in r6.0, sec 
> > > 2.2.1, is helpful.  It shows the required timings for Power 
> > > Stable, REFCLK Stable, PERST# deassert, etc.
> > > 
> > > Per sec 2.11.2, PERST# must be asserted for at least 100us 
> > > (T_PERST), PERST# must be asserted for at least 100ms after Power 
> > > Stable (T_PVPERL), and PERST# must be asserted for at least 100us 
> > > after REFCLK Stable.
> > > 
> > > It would be helpful if we could tell by reading the source where 
> > > some of these critical events happen, and that the relevant delays 
> > > are there.  For example, if PERST# is asserted/deasserted by 
> > > "clk_enable()" or similar, it's not at all obvious from the code, 
> > > so we should have a comment to that effect.
> > 
> > >I reviewed the vendor sdk and it just do something like in clk_enable():
> > >
> > >	...
> > >	val = readl(0x88);
> > >	writel(val | BIT(16) | BIT(29) | BIT(26), 0x88);
> > >	/*wait link up*/
> > >	mdelay(1000);
> > >	...
> > >
> > >@Hui.Ma: is it fine use msleep(100) (so PCIE_T_PVPERL_MS) instead 
> > >of msleep(1000) (so PCIE_LINK_RETRAIN_TIMEOUT_MS)?
> >
> > 	I think msleep(1000) will be safer, because some device won't
> > 	link up with msleep(100).
>> 
>> Do you have details about this?  I guess it only hurts mediatek, but 
>> increasing the minimum time to bring up a PCI hierarchy by almost an 
>> entire second is a pretty big deal.
>> 
>> If this delay corresponds to the required T_PVPERL delay and 100ms 
>> isn't enough for some endpoints, those endpoints should fail with many 
>> host controllers, not just mediatek, so I would suspect the mediatek 
>> controller or a certain platform, not the endpoint itself.
>> 
>> If this corresponds to T_PVPERL and mediatek needs longer, I would 
>> document that by using "PCIE_T_PVPERL_MS * 10" and adding a comment 
>> about why (affected platform/device, hardware erratum, etc).
>> 
>> Bottom line, I don't really care what the value is, but I *would* like 
>> to be able to read pcie-mediatek-gen3.c and see the point where PCI 
>> power is stable, a delay of at least T_PVPERL, and where PERST# is 
>> deasserted because that's the main timing requirement on software.

>>I run some testes using 100ms delay (PCIE_T_PVPERL_MS) after
>>clk_bulk_prepare_enable() in mtk_pcie_en7581_power_up() and it works fine for me (I tested with a MT7915 WiFi PCIe nic connected to the PCIe sock).
>>Moreover, we already poll PCIE_LINK_STATUS_REG register to check the link status in mtk_pcie_startup_port(), right? I guess we can proceed with 100ms delay in mtk_pcie_en7581_power_up().
Yes.

Hi Lorenzo/Bjorn，
	After our internal discussion and tests, we confirmed that a 100ms delay is enough.



Regards,
Hui

>Regards,
>Lorenzo

> 
> Bjorn