Add Airoha EN7581 PCIe support

---
 .../bindings/pci/mediatek-pcie-gen3.yaml      | 68 +++++++++++++++++--
 1 file changed, 63 insertions(+), 5 deletions(-)

Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml +63 -5

index 76d742051f73..898c1be2d6a4 100644
@@ -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:

Signature · 1 line

2.45.2

Lorenzo Bianconi

author

2024-07-03 16:13 UTC #4 lore raw

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

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 <[email protected]>

Tested-by Zhengping Zhang <[email protected]>

---
 drivers/pci/controller/pcie-mediatek-gen3.c | 24 ++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

drivers/pci/controller/pcie-mediatek-gen3.c +21 -3

index 975b3024fb08..db0210803731 100644
@@ -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);

Signature · 1 line

2.45.2

Lorenzo Bianconi

author

2024-07-03 16:13 UTC #6 lore raw

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

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 <[email protected]>

Tested-by Zhengping Zhang <[email protected]>

---
 drivers/pci/controller/pcie-mediatek-gen3.c | 45 +++++++++++++++------
 1 file changed, 33 insertions(+), 12 deletions(-)

drivers/pci/controller/pcie-mediatek-gen3.c +33 ~~-12~~

index db0210803731..438a5222d986 100644
@@ -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[] = {

Signature · 1 line

2.45.2

Lorenzo Bianconi

author

2024-07-03 16:13 UTC #8 lore raw

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

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

Reviewed-by AngeloGioacchino Del Regno <[email protected]>

Tested-by Zhengping Zhang <[email protected]>

---
 drivers/pci/controller/Kconfig              |   2 +-
 drivers/pci/controller/pcie-mediatek-gen3.c | 113 +++++++++++++++++++-
 2 files changed, 113 insertions(+), 2 deletions(-)

drivers/pci/controller/Kconfig +1 -1

index e534c02ee34f..3bd6c9430010 100644
@@ -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.

drivers/pci/controller/pcie-mediatek-gen3.c +112 -1

157 lines not on this page · raw message

Signature · 1 line

2.45.2

AngeloGioacchino Del Regno

maintainer Reviewed-by on v4 1/4

2024-07-04 08:23 UTC #2 lore raw

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

Il 03/07/24 18:12, Lorenzo Bianconi ha scritto:

Quoted 4 lines

Introduce Airoha EN7581 entry in mediatek-gen3 PCIe controller binding
Reviewed-by Conor Dooley <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>

Reviewed-by AngeloGioacchino Del Regno <[email protected]>

Jianjun Wang (王建军)

Acked-by

2024-07-10 06:23 UTC #3 lore raw

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

Quoted 10 lines · AngeloGioacchino Del Regno

Il 03/07/24 18:12, Lorenzo Bianconi ha scritto:
Quoted 5 lines
Introduce Airoha EN7581 entry in mediatek-gen3 PCIe controller binding
Reviewed-by Conor Dooley <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <
[email protected]>

Jianjun Wang (王建军)

Acked-by on v4 2/4

2024-07-10 06:27 UTC #5 lore raw

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

Quoted 11 lines · Lorenzo Bianconi

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 <
[email protected]>
Tested-by Zhengping Zhang <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>

Jianjun Wang (王建军)

Acked-by on v4 3/4

2024-07-10 06:41 UTC #7 lore raw

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

Quoted 10 lines · Lorenzo Bianconi

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 <
[email protected]>
Tested-by Zhengping Zhang <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>

Jianjun Wang (王建军)

Acked-by on v4 4/4

2024-07-10 07:03 UTC #9 lore raw

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

Quoted 10 lines · Lorenzo Bianconi

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 <
[email protected]>
Tested-by Zhengping Zhang <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>

Lorenzo Bianconi

author

2024-08-20 08:46 UTC #25 lore raw

Quoted 23 lines

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

Quoted 8 lines

 .../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(-)

Signature · 1 line

2.45.2

Hi all,

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

Regards,
Lorenzo

Bjorn Helgaas

2024-08-20 14:01 UTC #26 lore raw

Quoted 28 lines · Lorenzo Bianconi

Quoted 28 lines
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(-)

Quoted 7 lines

Quoted 3 lines
Signature · 1 line
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

Krzysztof Wilczyński

Applied

2024-09-03 13:45 UTC #28 lore raw

In reply to Bjorn Helgaas

Hello,

Quoted 1 line

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

Krzysztof Wilczyński

2024-09-03 13:47 UTC #27 lore raw

Hello,

Quoted 4 lines

Quoted 1 line
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

Bjorn Helgaas

2024-11-05 21:33 UTC #10 lore raw

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

https://lore.kernel.org/linux-pci/[email protected]/
https://lore.kernel.org/r/[email protected]

[+cc Jim, Krishna, Vidya, Shashank]

Quoted 11 lines · Lorenzo Bianconi

Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3 PCIe controller driver.
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
(quoted diff) +5 -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)
...

Review

Quoted 10 lines

(quoted diff) +7 -0

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

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

Bjorn Helgaas

2024-11-06 20:32 UTC #13 lore raw

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

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

Quoted 5 lines · Lorenzo Bianconi

Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3 PCIe controller driver.
...
drivers/pci/controller/pcie-mediatek-gen3.c +1 -0
+static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)

drivers/pci/controller/pcie-mediatek-gen3.c static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie) · line 881

Quoted 10 lines

drivers/pci/controller/pcie-mediatek-gen3.c +10 -0

+{
+	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.

Quoted 35 lines

drivers/pci/controller/pcie-mediatek-gen3.c +34 -0

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

drivers/pci/controller/pcie-mediatek-gen3.c static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie) · line 926

Quoted 10 lines

drivers/pci/controller/pcie-mediatek-gen3.c +10 -0

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

drivers/pci/controller/pcie-mediatek-gen3.c static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie) · line 932

Quoted 5 lines

drivers/pci/controller/pcie-mediatek-gen3.c +5 -0

+	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

Lorenzo Bianconi

author

2024-11-06 22:40 UTC #14 lore raw

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

Quoted 5 lines · Lorenzo Bianconi

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

Quoted 11 lines

drivers/pci/controller/pcie-mediatek-gen3.c static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie) · line 881
Quoted 11 lines
drivers/pci/controller/pcie-mediatek-gen3.c +11 -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);

Hi Bjorn,

Quoted 4 lines

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

Quoted 8 lines

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?

Quoted 43 lines

drivers/pci/controller/pcie-mediatek-gen3.c +41 -0

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

Quoted 8 lines

drivers/pci/controller/pcie-mediatek-gen3.c static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie) · line 926
Quoted 3 lines
drivers/pci/controller/pcie-mediatek-gen3.c +3 -0
+	      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.

Quoted 11 lines

If the split is required, a comment about why would be helpful.
drivers/pci/controller/pcie-mediatek-gen3.c static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie) · line 932
Quoted 5 lines
drivers/pci/controller/pcie-mediatek-gen3.c +5 -0
+	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

Quoted 4 lines

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.

Regards,
Lorenzo

Quoted 2 lines

Bjorn

Jim Quinlan

2024-11-06 23:00 UTC #11 lore raw

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

In reply to Bjorn Helgaas

Quoted 25 lines · Bjorn Helgaas

[+cc Jim, Krishna, Vidya, Shashank]

Quoted 21 lines · Lorenzo Bianconi

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

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

(quoted diff) +12 -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)
...
 
+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?

Quoted 9 lines

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:

Quoted 6 lines

https://lore.kernel.org/linux-pci/[email protected]/
https://lore.kernel.org/r/[email protected]
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

Quoted 12 lines

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

Bjorn Helgaas

2024-11-06 23:31 UTC #15 lore raw

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